arm64: Mediate access to GICv3 sysregs at EL2

The errata will require to emulate the GIC virtual CPU interface in Xen. Because the hypervisor will update its internal state of the vGIC, we want to avoid messing up with it. So the errata is handled separately from the rest of the hypervisor.

New file vgic-v3-sr.c is added which will hold trap and emulate code for group0 / group1 registers. Workaround for cavium Errata 30115 needs this emulation code.

vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync in subsequent patches based on errata macros.

Signed-off-by Manish Jaggi <[email protected]>

---
 xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/traps.h |  2 ++
 2 files changed, 62 insertions(+)
 create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c

xen/arch/arm/arm64/vgic-v3-sr.c new file +0 -0

new file mode 100644
index 0000000000..39ab1ed6ca

Quoted 25 lines · Manish Jaggi

The errata will require to emulate the GIC virtual CPU interface in Xen. Because the hypervisor will update its internal state of the vGIC, we want to avoid messing up with it. So the errata is handled separately from the rest of the hypervisor.

New file vgic-v3-sr.c is added which will hold trap and emulate code for group0 / group1 registers. Workaround for cavium Errata 30115 needs this emulation code.

vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync in subsequent patches based on errata macros.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/traps.h |  2 ++
  2 files changed, 62 insertions(+)

create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c

xen/arch/arm/arm64/vgic-v3-sr.c new file +0 -0

new file mode 100644
index 0000000000..39ab1ed6ca

xen/arch/arm/arm64/vgic-v3-sr.c line 8

Quoted 8 lines

xen/arch/arm/arm64/vgic-v3-sr.c new file +8 -0

@@ -0,0 +1,60 @@
+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to emulate group0/group1 traps for handling
+ * cavium erratum 30115
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Cavium.

IANAL, but I don't think this copyright notice is correct.

I wrote about 90% of this series, and the copyright for that code is owned by ARM, and licensed under the GPLv2. You have the right to duplicate that code and do almost whatever you want with (within the limits of the GPLv2), but you still don't own the copyright.

I suggest you get in touch with your legal department for clarification on the matter.

Thanks,

	M.

Quoted 41 lines · Marc Zyngier

Quoted 25 lines · Manish Jaggi

The errata will require to emulate the GIC virtual CPU interface in Xen. Because the hypervisor will update its internal state of the vGIC, we want to avoid messing up with it. So the errata is handled separately from the rest of the hypervisor.

New file vgic-v3-sr.c is added which will hold trap and emulate code for group0 / group1 registers. Workaround for cavium Errata 30115 needs this emulation code.

vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync in subsequent patches based on errata macros.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/traps.h |  2 ++
  2 files changed, 62 insertions(+)

create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c

xen/arch/arm/arm64/vgic-v3-sr.c new file +0 -0

new file mode 100644
index 0000000000..39ab1ed6ca

xen/arch/arm/arm64/vgic-v3-sr.c line 8

Quoted 8 lines

xen/arch/arm/arm64/vgic-v3-sr.c new file +8 -0

@@ -0,0 +1,60 @@
+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to emulate group0/group1 traps for handling
+ * cavium erratum 30115
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Cavium.

IANAL, but I don't think this copyright notice is correct.

I wrote about 90% of this series, and the copyright for that code is owned by ARM, and licensed under the GPLv2. You have the right to duplicate that code and do almost whatever you want with (within the limits of the GPLv2), but you still don't own the copyright.

Quoted 2 lines

I suggest you get in touch with your legal department for clarification on the matter.

I will remove the copyright line, and add this
Original Author: Marc Zyngier <>
Ported to Xen by: Manish Jaggi <>

Quoted 44 lines · Manish Jaggi

Quoted 41 lines · Marc Zyngier

Quoted 25 lines · Manish Jaggi

The errata will require to emulate the GIC virtual CPU interface in Xen. Because the hypervisor will update its internal state of the vGIC, we want to avoid messing up with it. So the errata is handled separately from the rest of the hypervisor.

New file vgic-v3-sr.c is added which will hold trap and emulate code for group0 / group1 registers. Workaround for cavium Errata 30115 needs this emulation code.

vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync in subsequent patches based on errata macros.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c   | 60 +++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/traps.h |  2 ++
  2 files changed, 62 insertions(+)

create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c

xen/arch/arm/arm64/vgic-v3-sr.c new file +0 -0

new file mode 100644
index 0000000000..39ab1ed6ca

xen/arch/arm/arm64/vgic-v3-sr.c line 8

Quoted 8 lines

xen/arch/arm/arm64/vgic-v3-sr.c new file +8 -0

@@ -0,0 +1,60 @@
+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to emulate group0/group1 traps for handling
+ * cavium erratum 30115
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Cavium.

IANAL, but I don't think this copyright notice is correct.

I wrote about 90% of this series, and the copyright for that code is owned by ARM, and licensed under the GPLv2. You have the right to duplicate that code and do almost whatever you want with (within the limits of the GPLv2), but you still don't own the copyright.

Quoted 5 lines

Quoted 2 lines

I suggest you get in touch with your legal department for clarification on the matter.

I will remove the copyright line, and add this
Original Author: Marc Zyngier <>
Ported to Xen by: Manish Jaggi <>

You're missing the point. I don't give a damn about the authorship (I'm not exactly proud to have written that code).

The problem at hand is the
ARM copyright, which should be preserved (as no-one in Cavium wrote a single line of the original code).

Quoted 6 lines · Marc Zyngier

Quoted 5 lines · Manish Jaggi

Quoted 2 lines · Marc Zyngier

Quoted 1 line · Manish Jaggi

The errata will require to emulate the GIC virtual CPU interface in

Xen.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Because the hypervisor will update its internal state of the vGIC,

we want

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

to avoid messing up with it. So the errata is handled separately

from the

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

rest of the hypervisor.

New file vgic-v3-sr.c is added which will hold trap and emulate code for group0 / group1 registers. Workaround for cavium Errata 30115 needs this emulation code.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync in subsequent patches based on errata macros.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c   | 60

+++++++++++++++++++++++++++++++++++++++

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

  xen/include/asm-arm/arm64/traps.h |  2 ++
  2 files changed, 62 insertions(+)

create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c

(quoted diff) +0 -0

b/xen/arch/arm/arm64/vgic-v3-sr.c

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

xen/arch/arm/arm64/vgic-v3-sr.c line 8

Quoted 13 lines

new file mode 100644
index 0000000000..39ab1ed6ca

xen/arch/arm/arm64/vgic-v3-sr.c new file +8 -0

@@ -0,0 +1,60 @@
+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to emulate group0/group1 traps for handling
+ * cavium erratum 30115
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Cavium.

IANAL, but I don't think this copyright notice is correct.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I wrote about 90% of this series, and the copyright for that code is owned by ARM, and licensed under the GPLv2. You have the right to duplicate that code and do almost whatever you want with (within the limits of the GPLv2), but you still don't own the copyright.

I suggest you get in touch with your legal department for

clarification

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

on the matter.

I will remove the copyright line, and add this
Original Author: Marc Zyngier <>
Ported to Xen by: Manish Jaggi <>

You're missing the point. I don't give a damn about the authorship (I'm not exactly proud to have written that code).

:-D

Quoted 3 lines

The problem at hand is the
ARM copyright, which should be preserved (as no-one in Cavium wrote a single line of the original code).

I have been asking myself similar questions for a while now when we import code from Linux. The copyright/author line at the top of the file is somewhat arbitrary as every person that touched the code has copyright over her modifications. This is why at some point I thought we had to retain the full list of Signed-off-by lines that ever touched the code in question, but then, reading the DCO terms one more time:

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

I checked in the Linux kernel when code was imported from the Xen hypervisor to KVM a long time ago, and only a singled Signed-off-by of the person importing the code was used.

This is how I came to the conclusion that actually we don't need to do anything special, although it would be nice as a courtesy to retain the copyright/author lines at the top of the file. It would also be nice to have the original Linux commit id in the commit message to help tracking the history.

But I would love to have a clear guideline on this from a lawyer.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Hi,

Sorry for the formatting.

On Wed, 28 Mar 2018, 04:18 Stefano Stabellini, <[email protected] <mailto:[email protected]>> wrote:

On Tue, 27 Mar 2018, Marc Zyngier wrote:
> On 27/03/18 11:10, Manish Jaggi wrote:
> >
> >
> > On 03/27/2018 03:31 PM, Marc Zyngier wrote:
> >> On 27/03/18 10:07, Manish Jaggi wrote:
> >>> The errata will require to emulate the GIC virtual CPU interface in Xen.
> >>> Because the hypervisor will update its internal state of the vGIC, we want
> >>> to avoid messing up with it. So the errata is handled separately from the
> >>> rest of the hypervisor.
> >>>
> >>> New file vgic-v3-sr.c is added which will hold trap and emulate code
> >>> for group0 / group1 registers. Workaround for cavium Errata 30115
> >>> needs this emulation code.
> >>>
> >>> vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
> >>> in subsequent patches based on errata macros.
> >>>
> >>> Signed-off-by: Manish Jaggi <[email protected] <mailto:[email protected]>>
> >>> ---
> >>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60
+++++++++++++++++++++++++++++++++++++++
> >>>   xen/include/asm-arm/arm64/traps.h |  2 ++
> >>>   2 files changed, 62 insertions(+)
> >>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c > >>>
> >>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> >>> new file mode 100644
> >>> index 0000000000..39ab1ed6ca
> >>> --- /dev/null
> >>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> >>> @@ -0,0 +1,60 @@
> >>> +/*
> >>> + * xen/arch/arm/arm64/vgic-v3-sr.c
> >>> + *
> >>> + * Code to emulate group0/group1 traps for handling > >>> + * cavium erratum 30115
> >>> + *
> >>> + * Manish Jaggi <[email protected] <mailto:[email protected]>>
> >>> + * Copyright (c) 2018 Cavium.
> >> IANAL, but I don't think this copyright notice is correct. > >>
> >> I wrote about 90% of this series, and the copyright for that code is
> >> owned by ARM, and licensed under the GPLv2. You have the right to > >> duplicate that code and do almost whatever you want with (within the
> >> limits of the GPLv2), but you still don't own the copyright. > >>
> >> I suggest you get in touch with your legal department for clarification
> >> on the matter.
> > I will remove the copyright line, and add this
> > Original Author: Marc Zyngier <>
> > Ported to Xen by: Manish Jaggi <>
>
> You're missing the point. I don't give a damn about the authorship (I'm
> not exactly proud to have written that code).

:-D

> The problem at hand is the
> ARM copyright, which should be preserved (as no-one in Cavium wrote a
> single line of the original code).

I have been asking myself similar questions for a while now when we import code from Linux. The copyright/author line at the top of the file
is somewhat arbitrary as every person that touched the code has copyright over her modifications. This is why at some point I thought we
had to retain the full list of Signed-off-by lines that ever touched the
code in question, but then, reading the DCO terms one more time:

 (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

I checked in the Linux kernel when code was imported from the Xen hypervisor to KVM a long time ago, and only a singled Signed-off-by of the person importing the code was used.

It does not mean it was a good practice ;).

This is how I came to the conclusion that actually we don't need to do anything special, although it would be nice as a courtesy to retain the
copyright/author lines at the top of the file. It would also be nice to
have the original Linux commit id in the commit message to help tracking
the history.

We had a similar discussion on the spectre patches a couple of months ago. The original author is useful to know if we need to relicense some files.

Having just the commit id from Linux would create more overhead.

Lars suggested to create a file listing where all the work come from. For commit, I think it would be ok to just add the Signed-off in the commit and retain copyright in the code.

Of course, it is ok add the Cavium one if code is changed. But no copyright should be dropped.

Cheers,

But I would love to have a clear guideline on this from a lawyer.

_______________________________________________
Xen-devel mailing list
[email protected] <mailto:[email protected]>
https://lists.xenproject.org/mailman/listinfo/xen-devel

Quoted 119 lines · Julien Grall

Hi,

Sorry for the formatting.

On Wed, 28 Mar 2018, 04:18 Stefano Stabellini, <[email protected] <mailto:[email protected]>> wrote:

On Tue, 27 Mar 2018, Marc Zyngier wrote:
> On 27/03/18 11:10, Manish Jaggi wrote:
> >
> >
> > On 03/27/2018 03:31 PM, Marc Zyngier wrote:
> >> On 27/03/18 10:07, Manish Jaggi wrote:
> >>> The errata will require to emulate the GIC virtual CPU interface in Xen.
> >>> Because the hypervisor will update its internal state of the vGIC, we want
> >>> to avoid messing up with it. So the errata is handled separately from the
> >>> rest of the hypervisor.
> >>>
> >>> New file vgic-v3-sr.c is added which will hold trap and emulate code
> >>> for group0 / group1 registers. Workaround for cavium Errata 30115
> >>> needs this emulation code.
> >>>
> >>> vgic_v3_handle_cpuif_access would be called from do_trap_guest_sync
> >>> in subsequent patches based on errata macros.
> >>>
> >>> Signed-off-by: Manish Jaggi <[email protected] <mailto:[email protected]>>
> >>> ---
> >>>   xen/arch/arm/arm64/vgic-v3-sr.c   | 60
+++++++++++++++++++++++++++++++++++++++
> >>>   xen/include/asm-arm/arm64/traps.h |  2 ++
> >>>   2 files changed, 62 insertions(+)
> >>>   create mode 100644 xen/arch/arm/arm64/vgic-v3-sr.c > >>>
> >>> diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
> >>> new file mode 100644
> >>> index 0000000000..39ab1ed6ca
> >>> --- /dev/null
> >>> +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> >>> @@ -0,0 +1,60 @@
> >>> +/*
> >>> + * xen/arch/arm/arm64/vgic-v3-sr.c
> >>> + *
> >>> + * Code to emulate group0/group1 traps for handling > >>> + * cavium erratum 30115
> >>> + *
> >>> + * Manish Jaggi <[email protected] <mailto:[email protected]>>
> >>> + * Copyright (c) 2018 Cavium.
> >> IANAL, but I don't think this copyright notice is correct. > >>
> >> I wrote about 90% of this series, and the copyright for that code is
> >> owned by ARM, and licensed under the GPLv2. You have the right to > >> duplicate that code and do almost whatever you want with (within the
> >> limits of the GPLv2), but you still don't own the copyright. > >>
> >> I suggest you get in touch with your legal department for clarification
> >> on the matter.
> > I will remove the copyright line, and add this
> > Original Author: Marc Zyngier <>
> > Ported to Xen by: Manish Jaggi <>
>
> You're missing the point. I don't give a damn about the authorship (I'm
> not exactly proud to have written that code).

:-D

> The problem at hand is the
> ARM copyright, which should be preserved (as no-one in Cavium wrote a
> single line of the original code).

I have been asking myself similar questions for a while now when we import code from Linux. The copyright/author line at the top of the file
is somewhat arbitrary as every person that touched the code has copyright over her modifications. This is why at some point I thought we
had to retain the full list of Signed-off-by lines that ever touched the
code in question, but then, reading the DCO terms one more time:

 (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

I checked in the Linux kernel when code was imported from the Xen hypervisor to KVM a long time ago, and only a singled Signed-off-by of the person importing the code was used.

It does not mean it was a good practice ;).

This is how I came to the conclusion that actually we don't need to do anything special, although it would be nice as a courtesy to retain the
copyright/author lines at the top of the file. It would also be nice to
have the original Linux commit id in the commit message to help tracking
the history.

Quoted 8 lines

We had a similar discussion on the spectre patches a couple of months ago. The original author is useful to know if we need to relicense some files.

Having just the commit id from Linux would create more overhead.

Lars suggested to create a file listing where all the work come from. For commit, I think it would be ok to just add the Signed-off in the commit and retain copyright in the code.

Of course, it is ok add the Cavium one if code is changed. But no copyright should be dropped.

How about

+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to emulate group0/group1 traps for handling
+ * cavium erratum 30115
+ *
+ * This file merges code from Linux virt/kvm/arm/hyp/vgic-v3-sr.c
+ * Copyright (C) 2012-2015 - ARM Ltd
+ * Original Author: Marc Zyngier <[email protected]>
+ * Code Ported to Xen by: Manish Jaggi <[email protected]>
...

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
 3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
  */
 
 #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
 #include <asm/regs.h>
 #include <asm/system.h>
 #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
 /*
  * returns true if the register is emulated.
  */

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/sysregs.h |  1 +
  xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
  3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
   */
 
  #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
  #include <asm/regs.h>
  #include <asm/system.h>
  #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
  /*
   * returns true if the register is emulated.
   */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

      switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
      {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

This comment applies throughout the series.

Thanks,

	M.

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/sysregs.h |  1 +
  xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
  3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
   */
 
  #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
  #include <asm/regs.h>
  #include <asm/system.h>
  #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
  /*
   * returns true if the register is emulated.
   */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

      switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
      {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Quoted 109 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
   xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
   xen/include/asm-arm/arm64/sysregs.h |  1 +
   xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
   3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
    */
 
   #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
   #include <asm/regs.h>
   #include <asm/system.h>
   #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
   /*
    * returns true if the register is emulated.
    */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

       switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
       {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Thanks,

	M.

Quoted 117 lines · Marc Zyngier

Quoted 109 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
   xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
   xen/include/asm-arm/arm64/sysregs.h |  1 +
   xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
   3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
    */
 
   #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
   #include <asm/regs.h>
   #include <asm/system.h>
   #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
   /*
    * returns true if the register is emulated.
    */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

       switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
       {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

Quoted 2 lines

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Do you want me to change both to match with your patch or it is ok?

Quoted 120 lines · Manish Jaggi

Quoted 118 lines · Marc Zyngier

Quoted 108 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
    xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
    xen/include/asm-arm/arm64/sysregs.h |  1 +
    xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
    3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
     */
 
    #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
    #include <asm/regs.h>
    #include <asm/system.h>
    #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
    /*
     * returns true if the register is emulated.
     */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

        switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
        {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Quoted 4 lines

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're pointlessly changing the flow. Again, that's for the maintainers to comment, but you should at the very least indicate that change in the commit log.

	M.

Quoted 124 lines · Marc Zyngier

Quoted 123 lines · Manish Jaggi

Quoted 112 lines · Marc Zyngier

Quoted 108 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
    xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
    xen/include/asm-arm/arm64/sysregs.h |  1 +
    xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
    3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
     */
 
    #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
    #include <asm/regs.h>
    #include <asm/system.h>
    #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
    /*
     * returns true if the register is emulated.
     */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

        switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
        {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Quoted 6 lines

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Quoted 5 lines

Quoted 1 line

Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're pointlessly changing the flow. Again, that's for the maintainers to comment, but you should at the very least indicate that change in the commit log.

I did in the cover letter

Quoted 125 lines · Manish Jaggi

Quoted 123 lines · Marc Zyngier

Quoted 122 lines · Manish Jaggi

Quoted 112 lines · Marc Zyngier

Quoted 108 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
     xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
     xen/include/asm-arm/arm64/sysregs.h |  1 +
     xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
     3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
      */
 
     #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
     #include <asm/regs.h>
     #include <asm/system.h>
     #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
     /*
      * returns true if the register is emulated.
      */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

         switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
         {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Quoted 6 lines

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're pointlessly changing the flow. Again, that's for the maintainers to comment, but you should at the very least indicate that change in the commit log.

I did in the cover letter

which, crucially, doesn't end-up in the commit. Oh well.

	M.

Quoted 132 lines · Marc Zyngier

Quoted 131 lines · Manish Jaggi

Quoted 120 lines · Marc Zyngier

Quoted 119 lines · Manish Jaggi

Quoted 118 lines · Marc Zyngier

Quoted 108 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
     xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
     xen/include/asm-arm/arm64/sysregs.h |  1 +
     xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
     3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
      */
 
     #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
     #include <asm/regs.h>
     #include <asm/system.h>
     #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
     /*
      * returns true if the register is emulated.
      */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

         switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
         {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Quoted 8 lines

Quoted 4 lines

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're pointlessly changing the flow. Again, that's for the maintainers to comment, but you should at the very least indicate that change in the commit log.

I did in the cover letter

Quoted 1 line

which, crucially, doesn't end-up in the commit. Oh well.

I am working on to address the two points, will send v3 later today.
- will remove emulate functions
- and pass vmcr as a param.

Will it be possible to review the remaining part of the code, so that I can address
other comments in v3 as well.

Quoted 138 lines · Manish Jaggi

Quoted 125 lines · Marc Zyngier

Quoted 124 lines · Manish Jaggi

Quoted 123 lines · Marc Zyngier

Quoted 122 lines · Manish Jaggi

Quoted 112 lines · Marc Zyngier

Quoted 108 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
      xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
      xen/include/asm-arm/arm64/sysregs.h |  1 +
      xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
      3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
       */
 
      #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
      #include <asm/regs.h>
      #include <asm/system.h>
      #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
      /*
       * returns true if the register is emulated.
       */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

          switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
          {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Quoted 6 lines

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're pointlessly changing the flow. Again, that's for the maintainers to comment, but you should at the very least indicate that change in the commit log.

I did in the cover letter

which, crucially, doesn't end-up in the commit. Oh well.

I am working on to address the two points, will send v3 later today.
- will remove emulate functions
- and pass vmcr as a param.

Quoted 3 lines

Will it be possible to review the remaining part of the code, so that I can address
other comments in v3 as well.

I suggest you wait until some other folks have a chance to properly review the series. You only posted the stuff this morning, give them a chance. A week between two versions is probably the right timing.

	M.

Quoted 139 lines · Marc Zyngier

Quoted 138 lines · Manish Jaggi

Quoted 125 lines · Marc Zyngier

Quoted 124 lines · Manish Jaggi

Quoted 123 lines · Marc Zyngier

Quoted 122 lines · Manish Jaggi

Quoted 112 lines · Marc Zyngier

Quoted 108 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
      xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
      xen/include/asm-arm/arm64/sysregs.h |  1 +
      xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
      3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
       */
 
      #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
      #include <asm/regs.h>
      #include <asm/system.h>
      #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
      /*
       * returns true if the register is emulated.
       */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

          switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
          {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Quoted 6 lines

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're pointlessly changing the flow. Again, that's for the maintainers to comment, but you should at the very least indicate that change in the commit log.

I did in the cover letter

which, crucially, doesn't end-up in the commit. Oh well.

I am working on to address the two points, will send v3 later today.
- will remove emulate functions
- and pass vmcr as a param.

Quoted 6 lines

Quoted 3 lines

Will it be possible to review the remaining part of the code, so that I can address
other comments in v3 as well.

I suggest you wait until some other folks have a chance to properly review the series. You only posted the stuff this morning, give them a chance. A week between two versions is probably the right timing.

Xen 4.11 window closes this week, so I have only few days. I am hoping to get your ack before that.

Quoted 146 lines · Manish Jaggi

Quoted 144 lines · Marc Zyngier

Quoted 132 lines · Manish Jaggi

Quoted 131 lines · Marc Zyngier

Quoted 130 lines · Manish Jaggi

Quoted 120 lines · Marc Zyngier

Quoted 119 lines · Manish Jaggi

Quoted 118 lines · Marc Zyngier

Quoted 108 lines · Manish Jaggi

Quoted 98 lines · Marc Zyngier

Quoted 97 lines · Manish Jaggi

This patch is ported to xen from linux commit
d70c7b31a60f2458f35c226131f2a01a7a98b6cf
KVM: arm64: vgic-v3: Add ICV_BPR1_EL1 handler

Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1 register, which is located in the ICH_VMCR_EL2.BPR1 field.

Signed-off-by Manish Jaggi <[email protected]>

---
       xen/arch/arm/arm64/vgic-v3-sr.c     | 70 +++++++++++++++++++++++++++++++++++++
       xen/include/asm-arm/arm64/sysregs.h |  1 +
       xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
       3 files changed, 77 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +66 -0

index 39ab1ed6ca..ed4254acf9 100644
@@ -18,10 +18,76 @@
        */
 
       #include <asm/current.h>
+#include <asm/gic_v3_defs.h>
       #include <asm/regs.h>
       #include <asm/system.h>
       #include <asm/traps.h>
 
+#define vtr_to_nr_pre_bits(v)     ((((uint32_t)(v) >> 26) & 7) + 1)
+
+static int vgic_v3_bpr_min(void)
+{
+    /* See Pseudocode for VPriorityGroup */
+    return 8 - vtr_to_nr_pre_bits(READ_SYSREG32(ICH_VTR_EL2));
+}
+
+static unsigned int vgic_v3_get_bpr0(uint32_t vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int vgic_v3_get_bpr1(uint32_t vmcr)
+{
+    unsigned int bpr;
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+    {
+        bpr = vgic_v3_get_bpr0(vmcr);
+        if ( bpr < 7 )
+            bpr++;
+    }
+    else
+        bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
+
+    return bpr;
+}
+
+static void vgic_v3_read_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    set_user_reg(regs, regidx, vgic_v3_get_bpr1(vmcr));
+}
+
+static void vgic_v3_write_bpr1(struct cpu_user_regs *regs, int regidx)
+{
+    register_t val = get_user_reg(regs, regidx);
+    uint8_t bpr_min = vgic_v3_bpr_min();
+    uint32_t vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( vmcr & ICH_VMCR_CBPR_MASK )
+        return;
+
+    /* Enforce BPR limiting */
+    if ( val < bpr_min )
+        val = bpr_min;
+
+    val <<= ICH_VMCR_BPR1_SHIFT;
+    val &= ICH_VMCR_BPR1_MASK;
+    vmcr &= ~ICH_VMCR_BPR1_MASK;
+    vmcr |= val;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+static void vreg_emulate_bpr1(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    if ( hsr.sysreg.read )
+        vgic_v3_read_bpr1(regs, hsr.sysreg.reg);
+    else
+        vgic_v3_write_bpr1(regs, hsr.sysreg.reg);
+}
+
       /*
        * returns true if the register is emulated.
        */

@@ -40,6 +106,10 @@ bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs)
 

Quoted 9 lines

xen/arch/arm/arm64/vgic-v3-sr.c bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs) · line 111

Quoted 5 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

           switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
           {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+         vreg_emulate_bpr1(regs, hsr);
+         break;

What is the rational for indirecting through a function and moving the reading of VMCR to the leaf functions? I appreciate that this doesn't change much, but since this is a port of existing code, it will make more complex the port of potential fixes.

Quoted 3 lines

I used xen template of handling sysreg traps
If you see the file xen/arch/arm/arm64/sysreg.c
a handle_XXX function is used throughout...

Sure, but this is not sysreg.c. This is a separate file for a reason (i.e. it is imported code). Anyway, that's for the Xen maintainers to decide.

More importantly, my other question still stand: most trap functions do require VMCR as an input. Why moving it to the leaf functions?

Quoted 8 lines

Quoted 4 lines

Same reason, I was keeping the interface same of all handle_XXX functions handle_XXX(regs, hsr, ...)

Do you want me to change both to match with your patch or it is ok?

My preference would be to keep the code as initially written, as you're pointlessly changing the flow. Again, that's for the maintainers to comment, but you should at the very least indicate that change in the commit log.

I did in the cover letter

Quoted 8 lines

Quoted 1 line

which, crucially, doesn't end-up in the commit. Oh well.

I am working on to address the two points, will send v3 later today.
- will remove emulate functions
- and pass vmcr as a param.

Will it be possible to review the remaining part of the code, so that I can address
other comments in v3 as well.

I suggest you wait until some other folks have a chance to properly review the series. You only posted the stuff this morning, give them a chance. A week between two versions is probably the right timing.

Quoted 2 lines

Xen 4.11 window closes this week, so I have only few days. I am hoping to get your ack before that.

My Ack is not the maintainers' (which you'd need anyway), and your series is not the only one I need to review. I don't plan on having another look at it this week anyway.

As for the Xen deadline, I'm sure there will be another one.

	M.

Quoted 11 lines · Marc Zyngier

Quoted 10 lines · Manish Jaggi

Quoted 8 lines · Marc Zyngier

Quoted 7 lines · Manish Jaggi

Quoted 6 lines · Marc Zyngier

Quoted 5 lines · Manish Jaggi

Quoted 4 lines · Marc Zyngier

Quoted 3 lines · Manish Jaggi

Quoted 2 lines · Marc Zyngier

Quoted 1 line · Manish Jaggi

On 03/27/2018 04:00 PM, Marc Zyngier wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line · Manish Jaggi

Will it be possible to review the remaining part of the code, so that I can address
other comments in v3 as well.

I suggest you wait until some other folks have a chance to properly review the series. You only posted the stuff this morning, give them a chance. A week between two versions is probably the right timing.

Xen 4.11 window closes this week, so I have only few days.

Quoted 6 lines

Quoted 1 line

I am hoping to get your ack before that.

My Ack is not the maintainers' (which you'd need anyway), and your series is not the only one I need to review. I don't plan on having another look at it this week anyway.

As for the Xen deadline, I'm sure there will be another one.

Hi Julien,
Is it prudent to send v3 with the two changes of marc suggested in this mail ? Since this is a errata workaround, and hopefully it should make it to 4.11. I have tried to address the v1 comments in v2, could you please have a look at it.

This patch is ported to xen from linux commit
b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

Add a handler for writing the guest's view of the ICC_EOIR1_EL1 register. This involves dropping the priority of the interrupt, and deactivating it if required (EOImode == 0).

Signed-off-by Manish Jaggi <[email protected]>

---
 xen/arch/arm/arm64/vgic-v3-sr.c     | 136 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |   1 +
 xen/include/asm-arm/gic_v3_defs.h   |   4 ++
 3 files changed, 141 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +1 -0

index 026d64506f..e32ec01f56 100644
@@ -33,6 +33,7 @@
 
 #define ICC_IAR1_EL1_SPURIOUS    0x3ff
 #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
 
 static int vgic_v3_bpr_min(void)
 {

xen/arch/arm/arm64/vgic-v3-sr.c +6 -0

+
+    for ( i = 0; i < used_lrs; i++ )
+    {
+        uint64_t val = gicv3_ich_read_lr(i);
+
+        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&

Quoted 31 lines · Manish Jaggi

This patch is ported to xen from linux commit
b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

Add a handler for writing the guest's view of the ICC_EOIR1_EL1 register. This involves dropping the priority of the interrupt, and deactivating it if required (EOImode == 0).

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c     | 136 ++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/sysregs.h |   1 +
  xen/include/asm-arm/gic_v3_defs.h   |   4 ++
  3 files changed, 141 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +1 -0

index 026d64506f..e32ec01f56 100644
@@ -33,6 +33,7 @@
 
  #define ICC_IAR1_EL1_SPURIOUS    0x3ff
  #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
 
  static int vgic_v3_bpr_min(void)
  {

@@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr)
      vgic_v3_read_iar(regs, hsr);
  }
 

xen/arch/arm/arm64/vgic-v3-sr.c static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr) · line 489

Quoted 4 lines

xen/arch/arm/arm64/vgic-v3-sr.c +4 -0

+static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
+{
+    int i;
+    unsigned int used_lrs =  gic_get_num_lrs();

This is quite a departure from the existing code. KVM always allocate LRs sequentially, and used_lrs represents the current upper bound.

Here,
you seem to be looking at *all* the LRs. Is that safe?

Are you
guaranteed not to have any stale state?

In any case, the change should be documented.

Quoted 6 lines

xen/arch/arm/arm64/vgic-v3-sr.c +6 -0

+
+    for ( i = 0; i < used_lrs; i++ )
+    {
+        uint64_t val = gicv3_ich_read_lr(i);
+
+        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&

xen/arch/arm/arm64/vgic-v3-sr.c static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr) · line 505

Quoted 10 lines

xen/arch/arm/arm64/vgic-v3-sr.c +10 -0

+            (val & ICH_LR_ACTIVE_BIT) )
+        {
+            *lr_val = val;
+            return i;
+        }
+    }
+
+    *lr_val = ICC_IAR1_EL1_SPURIOUS;
+    return -1;
+}

Thanks,

	M.

Quoted 29 lines · Marc Zyngier

Quoted 28 lines · Manish Jaggi

This patch is ported to xen from linux commit
b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

Add a handler for writing the guest's view of the ICC_EOIR1_EL1 register. This involves dropping the priority of the interrupt, and deactivating it if required (EOImode == 0).

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c     | 136
++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/sysregs.h |   1 +
  xen/include/asm-arm/gic_v3_defs.h   |   4 ++

  3 files changed, 141 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +1 -0

index 026d64506f..e32ec01f56 100644
@@ -33,6 +33,7 @@
     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
   #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
     static int vgic_v3_bpr_min(void)
   {

@@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
 cpu_user_regs *regs, const union hsr hsr)

Quoted 8 lines

xen/arch/arm/arm64/vgic-v3-sr.c static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr) · line 489

Quoted 6 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

       vgic_v3_read_iar(regs, hsr);
   }
   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
+{
+    int i;
+    unsigned int used_lrs =  gic_get_num_lrs();

This is quite a departure from the existing code. KVM always allocate LRs sequentially, and used_lrs represents the current upper bound.

IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.

xen/arch/arm/gic.c:
gic_raise_guest_irq
    gic_find_unused_lr

Quoted 2 lines

Here,
you seem to be looking at *all* the LRs. Is that safe?

IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but that is static in gic.c

To do something like
+for_each_set_bit(i, lr_mask, nr_lrs)
+ {
+      u64 val = __gic_v3_get_lr(i);
+      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+     /* Not pending in the state? */
+     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT) +         continue;

I need to do some jugglery to make lr_mask visible outside of xen/arch/arm/gic.c The easiest would be to add an extern function, harder way would be to add it in gic_hw_operations

- vgic_v3_highest_priority_lr iterates is interested in used LR's which sre in Pending state.
- emulating IAR is done with interrupts disabled
- iterating over all the LRs and finding which ones are in Pending.

Quoted 2 lines

  Are you
guaranteed not to have any stale state?

I would request Stefano/Andre/Julien to comment here...

Quoted 22 lines

In any case, the change should be documented.

Quoted 6 lines

xen/arch/arm/arm64/vgic-v3-sr.c +6 -0

+
+    for ( i = 0; i < used_lrs; i++ )
+    {
+        uint64_t val = gicv3_ich_read_lr(i);
+
+        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&

xen/arch/arm/arm64/vgic-v3-sr.c static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr) · line 505

Quoted 10 lines

xen/arch/arm/arm64/vgic-v3-sr.c +10 -0

+            (val & ICH_LR_ACTIVE_BIT) )
+        {
+            *lr_val = val;
+            return i;
+        }
+    }
+
+    *lr_val = ICC_IAR1_EL1_SPURIOUS;
+    return -1;
+}

Thanks,

    M.

Quoted 59 lines · Manish Jaggi

Quoted 29 lines · Marc Zyngier

Quoted 28 lines · Manish Jaggi

This patch is ported to xen from linux commit
b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

Add a handler for writing the guest's view of the ICC_EOIR1_EL1 register. This involves dropping the priority of the interrupt, and deactivating it if required (EOImode == 0).

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c     | 136
++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/sysregs.h |   1 +
  xen/include/asm-arm/gic_v3_defs.h   |   4 ++

  3 files changed, 141 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +1 -0

index 026d64506f..e32ec01f56 100644
@@ -33,6 +33,7 @@
     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
   #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
     static int vgic_v3_bpr_min(void)
   {

@@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
 cpu_user_regs *regs, const union hsr hsr)

Quoted 8 lines

xen/arch/arm/arm64/vgic-v3-sr.c static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr) · line 489

Quoted 6 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

       vgic_v3_read_iar(regs, hsr);
   }
   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
+{
+    int i;
+    unsigned int used_lrs =  gic_get_num_lrs();

This is quite a departure from the existing code. KVM always allocate LRs sequentially, and used_lrs represents the current upper bound.

IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.

xen/arch/arm/gic.c:
gic_raise_guest_irq
    gic_find_unused_lr

Quoted 2 lines

Here,
you seem to be looking at *all* the LRs. Is that safe?

IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but that is static in gic.c

To do something like
+for_each_set_bit(i, lr_mask, nr_lrs)
+ {
+      u64 val = __gic_v3_get_lr(i);
+      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+     /* Not pending in the state? */
+     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT) +         continue;

Quoted 9 lines

I need to do some jugglery to make lr_mask visible outside of xen/arch/arm/gic.c The easiest would be to add an extern function, harder way would be to add it in gic_hw_operations

- vgic_v3_highest_priority_lr iterates is interested in used LR's which sre in Pending state.
- emulating IAR is done with interrupts disabled
- iterating over all the LRs and finding which ones are in Pending.

Just to add I was answering for using num_lrs for used_lrs, above was for IAR flow. This holds the same for EOIR flow as well.

The bigger point is unless I add some jugglery to access static value outside gic.c this is the only solution.

Stefano/Andre/Julien
Please suggest if there is some better way...

Quoted 2 lines

Quoted 2 lines

  Are you
guaranteed not to have any stale state?

Quoted 78 lines · Manish Jaggi

Quoted 65 lines · Manish Jaggi

Quoted 30 lines · Marc Zyngier

Quoted 29 lines · Manish Jaggi

This patch is ported to xen from linux commit
b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

Add a handler for writing the guest's view of the ICC_EOIR1_EL1 register. This involves dropping the priority of the interrupt, and deactivating it if required (EOImode == 0).

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c     | 136
++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/sysregs.h |   1 +
  xen/include/asm-arm/gic_v3_defs.h   |   4 ++

  3 files changed, 141 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +1 -0

index 026d64506f..e32ec01f56 100644
@@ -33,6 +33,7 @@
     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
   #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
     static int vgic_v3_bpr_min(void)
   {

@@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct
 cpu_user_regs *regs, const union hsr hsr)

Quoted 8 lines

xen/arch/arm/arm64/vgic-v3-sr.c static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr) · line 489

Quoted 6 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

       vgic_v3_read_iar(regs, hsr);
   }
   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
+{
+    int i;
+    unsigned int used_lrs =  gic_get_num_lrs();

This is quite a departure from the existing code. KVM always allocate LRs sequentially, and used_lrs represents the current upper bound.

IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.

xen/arch/arm/gic.c:
gic_raise_guest_irq
    gic_find_unused_lr

Quoted 2 lines

Here,
you seem to be looking at *all* the LRs. Is that safe?

IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but that is static in gic.c

To do something like
+for_each_set_bit(i, lr_mask, nr_lrs)
+ {
+      u64 val = __gic_v3_get_lr(i);
+      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+     /* Not pending in the state? */
+     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
+         continue;

I need to do some jugglery to make lr_mask visible outside of xen/arch/arm/gic.c
The easiest would be to add an extern function, harder way would be to add it in gic_hw_operations

Quoted 6 lines

- vgic_v3_highest_priority_lr iterates is interested in used LR's which sre in Pending state.
- emulating IAR is done with interrupts disabled
- iterating over all the LRs and finding which ones are in Pending.

Just to add I was answering for using num_lrs for used_lrs, above was for IAR flow.
This holds the same for EOIR flow as well.

Quoted 6 lines

The bigger point is unless I add some jugglery to access static value outside gic.c
this is the only solution.

Stefano/Andre/Julien
Please suggest if there is some better way...

lr_mask is already exported. So I am not sure what you need here.

However, despite the fact the variable is living in gic.c it is only used by the old vGIC. Newer vGIC is based on KVM, so used_lrs would be fine to use.

For the old vGIC, see below.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

  Are you
guaranteed not to have any stale state?

LRs are zeroed when unused and AFAICT they should always be accurate. Stefano can you confirm it?

So it would be ok go through all the LRs (thought with a performance hit).

Hi,

Quoted 77 lines · Manish Jaggi

Quoted 59 lines · Manish Jaggi

Quoted 29 lines · Marc Zyngier

Quoted 28 lines · Manish Jaggi

This patch is ported to xen from linux commit
b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

Add a handler for writing the guest's view of the ICC_EOIR1_EL1 register. This involves dropping the priority of the interrupt, and deactivating it if required (EOImode == 0).

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/arm64/vgic-v3-sr.c     | 136
++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/sysregs.h |   1 +
  xen/include/asm-arm/gic_v3_defs.h   |   4 ++

  3 files changed, 141 insertions(+)

xen/arch/arm/arm64/vgic-v3-sr.c +1 -0

index 026d64506f..e32ec01f56 100644
@@ -33,6 +33,7 @@
     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
   #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
     static int vgic_v3_bpr_min(void)
   {

@@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
 cpu_user_regs *regs, const union hsr hsr)

Quoted 8 lines

xen/arch/arm/arm64/vgic-v3-sr.c static void vreg_emulate_iar(struct cpu_user_regs *regs, const union hsr hsr) · line 489

Quoted 6 lines

xen/arch/arm/arm64/vgic-v3-sr.c +3 -0

       vgic_v3_read_iar(regs, hsr);
   }
   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
+{
+    int i;
+    unsigned int used_lrs =  gic_get_num_lrs();

This is quite a departure from the existing code. KVM always allocate LRs sequentially, and used_lrs represents the current upper bound.

IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.

xen/arch/arm/gic.c:
gic_raise_guest_irq
    gic_find_unused_lr

Quoted 2 lines

Here,
you seem to be looking at *all* the LRs. Is that safe?

IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but that is static in gic.c

To do something like
+for_each_set_bit(i, lr_mask, nr_lrs)
+ {
+      u64 val = __gic_v3_get_lr(i);
+      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+     /* Not pending in the state? */
+     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT) +         continue;

Quoted 9 lines

I need to do some jugglery to make lr_mask visible outside of xen/arch/arm/gic.c The easiest would be to add an extern function, harder way would be to add it in gic_hw_operations

- vgic_v3_highest_priority_lr iterates is interested in used LR's which sre in Pending state.
- emulating IAR is done with interrupts disabled
- iterating over all the LRs and finding which ones are in Pending.

Just to add I was answering for using num_lrs for used_lrs, above was for IAR flow. This holds the same for EOIR flow as well.

The bigger point is unless I add some jugglery to access static value outside gic.c this is the only solution.