[RFC PATCH 00/10] arm64: Mediate access to GICv3 sysregs at EL2

[RFC PATCH 00/10] arm64: Mediate access to GICv3 sysregs at EL2

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]

The current RFC patchset is a subset of [1], as it handleing only Group1 traps
as a PoC. Most of the trap code is added in vsysreg.c. Trap handler function is kept
independent of the usual guest trap handling code. 
Looking for feedback on this approach.  

The errata has been validated on Cavium ThunderX platform.

Steps to reporduce the errata
- Boot Xen with 2 cores.
- Disable group1 interrupts in domU kernel
- start domU, the kill and start again.
One of the Xen core would hang.

Code in this patchset fixes this issue.

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026029.html

Manish Jaggi (10):
  arm64: Add CONFIG_VGIC_ERRATA
  arm64: Add hook to handle guest GICv3 sysreg accesses
  arm64: Add ICV_BPR1_EL1 handler
  arm64: Add ICV_IGRPEN1_EL1 handler
  arm64: Add accessors for the ICH_APxRn_EL2 registers
  arm64: Expose gicv3_ich_read/write_lr
  arm64: Add ICV_IAR1_EL1 handler
  arm64: Add ICV_EOIR1_EL1 handler
  arm64: Add a handler for ICV_HPPIR1_EL1
  arm64: Enable Trapping of Group1 registers which is controlled by
    command line

 xen/arch/arm/Kconfig                |   9 +
 xen/arch/arm/arm64/vsysreg.c        | 564 ++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic-v3.c               |  21 +-
 xen/arch/arm/traps.c                |  11 +
 xen/include/asm-arm/arm64/sysregs.h |   5 +
 xen/include/asm-arm/arm64/traps.h   |   2 +
 xen/include/asm-arm/gic.h           |   1 +
 xen/include/asm-arm/gic_v3.h        |   7 +
 xen/include/asm-arm/gic_v3_defs.h   |  30 ++
 9 files changed, 649 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/asm-arm/gic_v3.h

-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 01/10] Add CONFIG_VGIC_ERRATA

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

Add a config option to enable VGIC Errata Code in Xen. Platforms which do not
have this errta can compile out this feature.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/Kconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index f58019d6ed..2966e3a3d3 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -59,6 +59,15 @@ config SBSA_VUART_CONSOLE
 
 endmenu
 
+menu "Errata Workarounds"
+       depends on ARM_64
+
+config VGIC_ERRATA
+       bool "Handler code for emulation of Group0/1 vGIC registers for VGC Errata"
+       depends on ARM_64
+
+endmenu
+
 menu "ARM errata workaround via the alternative framework"
 	depends on HAS_ALTERNATIVE
 
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

In order to start handling guest access to GICv3 system registers,
let's add a hook that will get called when we trap a system register
access.
This handling code is kept independent of other traps.
Set CONFIG_VGIC_ERRATA to enable this code.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vsysreg.c      | 17 +++++++++++++++++
 xen/arch/arm/traps.c              | 11 +++++++++++
 xen/include/asm-arm/arm64/traps.h |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index c57ac12503..a5c17e460f 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -219,6 +219,23 @@ void do_sysreg(struct cpu_user_regs *regs,
     regs->pc += 4;
 }
 
+#ifdef CONFIG_VGIC_ERRATA
+int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    int ret = 0;
+
+    local_irq_disable();
+    switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
+    {
+    default:
+        ret = -1;
+        break;
+    }
+    local_irq_enable();
+
+    return ret;
+}
+#endif
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
 {
     const union hsr hsr = { .bits = regs->hsr };
 
+#ifdef CONFIG_VGIC_ERRATA
+    int ret;
+
+    ret  = do_fixup_vgic_errata(regs,hsr);
+    if ( !ret )
+    {
+        advance_pc(regs, hsr);
+        return;
+    }
+#endif
+
     enter_hypervisor_head(regs);
 
     switch (hsr.ec) {
diff --git a/xen/include/asm-arm/arm64/traps.h b/xen/include/asm-arm/arm64/traps.h
index 2379b578cb..7378a1b022 100644
--- a/xen/include/asm-arm/arm64/traps.h
+++ b/xen/include/asm-arm/arm64/traps.h
@@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
 
 void do_sysreg(struct cpu_user_regs *regs,
                const union hsr hsr);
+int do_fixup_vgic_errata(struct cpu_user_regs *regs,
+               const union hsr hsr);
 
 #endif /* __ASM_ARM64_TRAPS__ */
 /*
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 03/10] arm64: Add ICV_BPR1_EL1 handler

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

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 <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vsysreg.c        | 71 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 xen/include/asm-arm/gic_v3_defs.h   |  6 ++++
 3 files changed, 78 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index a5c17e460f..91f755a3c6 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -22,6 +22,7 @@
 #include <asm/regs.h>
 #include <asm/traps.h>
 #include <asm/vtimer.h>
+#include <asm/gic_v3_defs.h>
 
 void do_sysreg(struct cpu_user_regs *regs,
                const union hsr hsr)
@@ -220,13 +221,83 @@ void do_sysreg(struct cpu_user_regs *regs,
 }
 
 #ifdef CONFIG_VGIC_ERRATA
+#define vtr_to_nr_pre_bits(v)     ((((u32)(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(u32 vmcr)
+{
+    return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
+}
+
+static unsigned int __vgic_v3_get_bpr1(u32 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)
+{
+    u32 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)
+{
+    u64 val = get_user_reg(regs, regidx);
+    u8 bpr_min = __vgic_v3_bpr_min();
+    u32 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);
+}
+
+void handle_bpr1(struct cpu_user_regs *regs, int regidx, bool read,
+                 const union hsr hsr)
+{
+    if ( read )
+        __vgic_v3_read_bpr1(regs, regidx);
+    else
+        __vgic_v3_write_bpr1(regs, regidx);
+}
+
 int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int ret = 0;
+    int regidx = hsr.sysreg.reg;
 
     local_irq_disable();
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
+    case HSR_SYSREG_ICC_BPR1_EL1:
+        handle_bpr1(regs, regidx, hsr.sysreg.read, hsr);
+        break;
+
     default:
         ret = -1;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 084d2a1e5d..025a27b0b4 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -89,6 +89,7 @@
 #define HSR_SYSREG_ICC_ASGI1R_EL1 HSR_SYSREG(3,1,c12,c11,6)
 #define HSR_SYSREG_ICC_SGI0R_EL1  HSR_SYSREG(3,2,c12,c11,7)
 #define HSR_SYSREG_ICC_SRE_EL1    HSR_SYSREG(3,0,c12,c12,5)
+#define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 65c9dc47cf..68a34cc353 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -157,6 +157,12 @@
 
 #define GICH_VMCR_EOI                (1 << 9)
 #define GICH_VMCR_VENG1              (1 << 1)
+#define ICH_VMCR_CBPR_SHIFT          4
+#define ICH_VMCR_CBPR_MASK           (1 << ICH_VMCR_CBPR_SHIFT)
+#define ICH_VMCR_BPR0_SHIFT          21
+#define ICH_VMCR_BPR0_MASK           (7 << ICH_VMCR_BPR0_SHIFT)
+#define ICH_VMCR_BPR1_SHIFT          18
+#define ICH_VMCR_BPR1_MASK           (7 << ICH_VMCR_BPR1_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 04/10] arm64: Add ICV_IGRPEN1_EL1 handler

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

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

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vsysreg.c        | 32 ++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 xen/include/asm-arm/gic_v3_defs.h   |  2 ++
 3 files changed, 35 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 91f755a3c6..ba4a5ab7e1 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -286,6 +286,34 @@ void handle_bpr1(struct cpu_user_regs *regs, int regidx, bool read,
         __vgic_v3_write_bpr1(regs, regidx);
 }
 
+static void  __vgic_v3_read_igrpen1(struct cpu_user_regs *regs, int regidx)
+{
+    u32 vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+    set_user_reg(regs, regidx, !!(vmcr & ICH_VMCR_ENG1_MASK));
+}
+
+static void  __vgic_v3_write_igrpen1(struct cpu_user_regs *regs, int regidx)
+{
+    u64 val = get_user_reg(regs, regidx);
+    u32 vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    if ( val & 1 )
+        vmcr |= ICH_VMCR_ENG1_MASK;
+    else
+        vmcr &= ~ICH_VMCR_ENG1_MASK;
+
+    WRITE_SYSREG32(vmcr, ICH_VMCR_EL2);
+}
+
+void handle_igrpen1(struct cpu_user_regs *regs, int regidx, bool read,
+                 const union hsr hsr)
+{
+    if ( read )
+        __vgic_v3_read_igrpen1(regs, regidx);
+    else
+        __vgic_v3_write_igrpen1(regs, regidx);
+}
+
 int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int ret = 0;
@@ -298,6 +326,10 @@ int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
         handle_bpr1(regs, regidx, hsr.sysreg.read, hsr);
         break;
 
+    case HSR_SYSREG_ICC_IGRPEN1_EL1:
+        handle_igrpen1(regs, regidx, hsr.sysreg.read, hsr);
+        break;
+
     default:
         ret = -1;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 025a27b0b4..731cabc74a 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -90,6 +90,7 @@
 #define HSR_SYSREG_ICC_SGI0R_EL1  HSR_SYSREG(3,2,c12,c11,7)
 #define HSR_SYSREG_ICC_SRE_EL1    HSR_SYSREG(3,0,c12,c12,5)
 #define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
+#define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 68a34cc353..ff8bda37d1 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -163,6 +163,8 @@
 #define ICH_VMCR_BPR0_MASK           (7 << ICH_VMCR_BPR0_SHIFT)
 #define ICH_VMCR_BPR1_SHIFT          18
 #define ICH_VMCR_BPR1_MASK           (7 << ICH_VMCR_BPR1_SHIFT)
+#define ICH_VMCR_ENG1_SHIFT          1
+#define ICH_VMCR_ENG1_MASK           (1 << ICH_VMCR_ENG1_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 05/10] arm64: Add accessors for the ICH_APxRn_EL2 registers

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

define accessors that take the register number as a parameter.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vsysreg.c | 92 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index ba4a5ab7e1..a698af21f2 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -314,6 +314,98 @@ void handle_igrpen1(struct cpu_user_regs *regs, int regidx, bool read,
         __vgic_v3_write_igrpen1(regs, regidx);
 }
 
+void  __vgic_v3_write_ap0rn(u32 val, int n)
+{
+    switch (n)
+    {
+    case 0:
+        WRITE_SYSREG32(val, ICH_AP0R0_EL2);
+        break;
+    case 1:
+        WRITE_SYSREG32(val, ICH_AP0R1_EL2);
+        break;
+    case 2:
+        WRITE_SYSREG32(val, ICH_AP0R2_EL2);
+        break;
+    case 3:
+        WRITE_SYSREG32(val, ICH_AP0R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+}
+
+void __vgic_v3_write_ap1rn(u32 val, int n)
+{
+    switch (n)
+    {
+    case 0:
+        WRITE_SYSREG32(val, ICH_AP1R0_EL2);
+        break;
+    case 1:
+        WRITE_SYSREG32(val, ICH_AP1R1_EL2);
+        break;
+    case 2:
+        WRITE_SYSREG32(val, ICH_AP1R2_EL2);
+        break;
+    case 3:
+        WRITE_SYSREG32(val, ICH_AP1R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+}
+
+u32  __vgic_v3_read_ap0rn(int n)
+{
+    u32 val;
+
+    switch (n)
+    {
+    case 0:
+        val = READ_SYSREG32(ICH_AP0R0_EL2);
+        break;
+    case 1:
+        val = READ_SYSREG32(ICH_AP0R1_EL2);
+        break;
+    case 2:
+        val = READ_SYSREG32(ICH_AP0R2_EL2);
+        break;
+    case 3:
+        val = READ_SYSREG32(ICH_AP0R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+
+    return val;
+}
+
+u32  __vgic_v3_read_ap1rn(int n)
+{
+    u32 val;
+
+    switch (n)
+    {
+    case 0:
+        val = READ_SYSREG32(ICH_AP1R0_EL2);
+        break;
+    case 1:
+        val = READ_SYSREG32(ICH_AP1R1_EL2);
+        break;
+    case 2:
+        val = READ_SYSREG32(ICH_AP1R2_EL2);
+        break;
+    case 3:
+        val = READ_SYSREG32(ICH_AP1R3_EL2);
+        break;
+    default:
+        unreachable();
+    }
+
+    return val;
+}
+
 int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int ret = 0;
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 06/10] Expose gicv3_ich_read/write_lr

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

gicv3_ich_read/write_lr functions are static in gic-v3.c
This patch creates wrapper functions which can be used from outside the file.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/gic-v3.c        | 10 ++++++++++
 xen/include/asm-arm/gic_v3.h |  7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 473e26111f..5dba8bc932 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -184,6 +184,11 @@ static uint64_t gicv3_ich_read_lr(int lr)
     }
 }
 
+uint64_t __gicv3_ich_read_lr(int lr)
+{
+    return gicv3_ich_read_lr(lr);
+}
+
 static void gicv3_ich_write_lr(int lr, uint64_t val)
 {
     switch ( lr )
@@ -242,6 +247,11 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
     isb();
 }
 
+void __gicv3_ich_write_lr(int lr, uint64_t val)
+{
+    return gicv3_ich_write_lr(lr, val);
+}
+
 /*
  * System Register Enable (SRE). Enable to access CPU & Virtual
  * interface registers as system registers in EL2
diff --git a/xen/include/asm-arm/gic_v3.h b/xen/include/asm-arm/gic_v3.h
new file mode 100644
index 0000000000..544aad5932
--- /dev/null
+++ b/xen/include/asm-arm/gic_v3.h
@@ -0,0 +1,7 @@
+#ifndef GICV3_H
+#define GICV3_H
+
+uint64_t __gicv3_ich_read_lr(int lr);
+void __gicv3_ich_write_lr(int lr, uint64_t val);
+
+#endif
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 07/10] arm64: Add ICV_IAR1_EL1 handler

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

Add a handler for reading the guest's view of the ICC_IAR1_EL1
register. This involves finding the highest priority Group-1
interrupt, checking against both PMR and the active group
priority, activating the interrupt and setting the group
priority as active.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vsysreg.c        | 192 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |   1 +
 xen/include/asm-arm/gic_v3_defs.h   |  18 ++++
 3 files changed, 211 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index a698af21f2..44c74d4217 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -23,6 +23,7 @@
 #include <asm/traps.h>
 #include <asm/vtimer.h>
 #include <asm/gic_v3_defs.h>
+#include <asm/gic_v3.h>
 
 void do_sysreg(struct cpu_user_regs *regs,
                const union hsr hsr)
@@ -222,6 +223,13 @@ void do_sysreg(struct cpu_user_regs *regs,
 
 #ifdef CONFIG_VGIC_ERRATA
 #define vtr_to_nr_pre_bits(v)     ((((u32)(v) >> 26) & 7) + 1)
+#define vtr_to_nr_apr_regs(v)    (1 << (vtr_to_nr_pre_bits(v) - 5))
+
+#define ESR_ELx_SYS64_ISS_CRM_SHIFT 1
+#define ESR_ELx_SYS64_ISS_CRM_MASK (0xf << ESR_ELx_SYS64_ISS_CRM_SHIFT)
+
+#define ICC_IAR1_EL1_SPURIOUS    0x3ff
+#define VGIC_MAX_SPI             1019
 
 static int  __vgic_v3_bpr_min(void)
 {
@@ -406,6 +414,186 @@ u32  __vgic_v3_read_ap1rn(int n)
     return val;
 }
 
+static int  __vgic_v3_get_group(const union hsr hsr)
+{
+    u8 crm = (hsr.bits & ESR_ELx_SYS64_ISS_CRM_MASK) >>
+              ESR_ELx_SYS64_ISS_CRM_SHIFT;
+
+    return crm != 8;
+}
+
+unsigned int gic_get_num_lrs(void)
+{
+    uint32_t vtr;
+
+    vtr = READ_SYSREG32(ICH_VTR_EL2);
+    return (vtr & GICH_VTR_NRLRGS) + 1;
+}
+
+static int __vgic_v3_highest_priority_lr(struct cpu_user_regs *regs,
+                                         u32 vmcr, u64 *lr_val)
+{
+    int i, lr = -1;
+    unsigned int used_lrs =  gic_get_num_lrs();
+    u8 priority = GICV3_IDLE_PRIORITY;
+
+    for ( i = 0; i < used_lrs; i++ )
+    {
+        u64 val =  __gicv3_ich_read_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;
+
+        /* Group-0 interrupt, but Group-0 disabled? */
+        if ( !(val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG0_MASK) )
+            continue;
+
+        /* Group-1 interrupt, but Group-1 disabled? */
+        if ( (val & ICH_LR_GROUP) && !(vmcr & ICH_VMCR_ENG1_MASK) )
+            continue;
+
+        /* Not the highest priority? */
+        if ( lr_prio >= priority )
+            continue;
+
+        /* This is a candidate */
+        priority = lr_prio;
+        *lr_val = val;
+        lr = i;
+    }
+
+    if ( lr == -1 )
+        *lr_val = ICC_IAR1_EL1_SPURIOUS;
+
+    return lr;
+}
+
+static int  __vgic_v3_get_highest_active_priority(void)
+{
+    int i;
+    u32 hap = 0;
+    u8 nr_apr_regs = vtr_to_nr_apr_regs(READ_SYSREG32(ICH_VTR_EL2));
+
+    for ( i = 0; i < nr_apr_regs; i++ )
+    {
+        u32 val;
+
+        /*
+         * The ICH_AP0Rn_EL2 and ICH_AP1Rn_EL2 registers
+         * contain the active priority levels for this VCPU
+         * for the maximum number of supported priority
+         * levels, and we return the full priority level only
+         * if the BPR is programmed to its minimum, otherwise
+         * we return a combination of the priority level and
+         * subpriority, as determined by the setting of the
+         * BPR, but without the full subpriority.
+         */
+        val  = __vgic_v3_read_ap0rn(i);
+        val |= __vgic_v3_read_ap1rn(i);
+        if ( !val )
+        {
+            hap += 32;
+            continue;
+        }
+
+        return (hap + __ffs(val)) << __vgic_v3_bpr_min();
+    }
+
+    return GICV3_IDLE_PRIORITY;
+}
+
+/*
+ * Convert a priority to a preemption level, taking the relevant BPR
+ * into account by zeroing the sub-priority bits.
+ */
+static u8  __vgic_v3_pri_to_pre(u8 pri, u32 vmcr, int grp)
+{
+    unsigned int bpr;
+
+    if ( !grp )
+        bpr = __vgic_v3_get_bpr0(vmcr) + 1;
+    else
+        bpr = __vgic_v3_get_bpr1(vmcr);
+
+    return pri & (GENMASK(7, 0) << bpr);
+}
+
+/*
+ * The priority value is independent of any of the BPR values, so we
+ * normalize it using the minumal BPR value. This guarantees that no
+ * matter what the guest does with its BPR, we can always set/get the
+ * same value of a priority.
+ */
+static void  __vgic_v3_set_active_priority(u8 pri, u32 vmcr, int grp)
+{
+    u8 pre, ap;
+    u32 val;
+    int apr;
+
+    pre = __vgic_v3_pri_to_pre(pri, vmcr, grp);
+    ap = pre >> __vgic_v3_bpr_min();
+    apr = ap / 32;
+
+    if ( !grp )
+    {
+        val = __vgic_v3_read_ap0rn(apr);
+        __vgic_v3_write_ap0rn(val | BIT(ap % 32), apr);
+    }
+    else
+    {
+        val = __vgic_v3_read_ap1rn(apr);
+        __vgic_v3_write_ap1rn(val | BIT(ap % 32), apr);
+    }
+}
+
+static void  __vgic_v3_read_iar(struct cpu_user_regs *regs, int regidx,
+                                const union hsr hsr)
+{
+    u64 lr_val;
+    u8 lr_prio, pmr;
+    int lr, grp;
+
+    u32 vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+    grp = __vgic_v3_get_group(hsr);
+
+    lr = __vgic_v3_highest_priority_lr(regs, vmcr, &lr_val);
+    if ( lr < 0 )
+        goto spurious;
+
+    if ( grp != !!(lr_val & ICH_LR_GROUP) )
+        goto spurious;
+
+    pmr = (vmcr & ICH_VMCR_PMR_MASK) >> ICH_VMCR_PMR_SHIFT;
+    lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+    if ( pmr <= lr_prio )
+        goto spurious;
+
+    if ( __vgic_v3_get_highest_active_priority() <=
+         __vgic_v3_pri_to_pre(lr_prio, vmcr, grp) )
+        goto spurious;
+
+    lr_val &= ~ICH_LR_STATE;
+    /* No active state for LPIs */
+    if ( (lr_val & ICH_LR_VIRTUAL_ID_MASK) <= VGIC_MAX_SPI )
+        lr_val |= ICH_LR_ACTIVE_BIT;
+
+    __gicv3_ich_write_lr(lr, lr_val);
+    __vgic_v3_set_active_priority(lr_prio, vmcr, grp);
+    set_user_reg(regs, regidx,  lr_val & ICH_LR_VIRTUAL_ID_MASK);
+
+    return;
+
+spurious:
+     set_user_reg(regs, regidx, ICC_IAR1_EL1_SPURIOUS);
+}
+
+void handle_iar(struct cpu_user_regs *regs, int regidx, const union hsr hsr)
+{
+    __vgic_v3_read_iar(regs, regidx, hsr);
+}
+
 int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int ret = 0;
@@ -422,6 +610,10 @@ int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
         handle_igrpen1(regs, regidx, hsr.sysreg.read, hsr);
         break;
 
+    case HSR_SYSREG_ICC_IAR1_EL1:
+        handle_iar(regs, regidx, hsr);
+        break;
+
     default:
         ret = -1;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 731cabc74a..53d2251840 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -91,6 +91,7 @@
 #define HSR_SYSREG_ICC_SRE_EL1    HSR_SYSREG(3,0,c12,c12,5)
 #define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
 #define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
+#define HSR_SYSREG_ICC_IAR1_EL1   HSR_SYSREG(3,0,c12,c12,0)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index ff8bda37d1..f22549a228 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -68,6 +68,8 @@
 #define GICV3_GICD_IIDR_VAL          0x34c
 #define GICV3_GICR_IIDR_VAL          GICV3_GICD_IIDR_VAL
 
+#define GICV3_IDLE_PRIORITY       0xff
+
 #define GICR_CTLR                    (0x0000)
 #define GICR_IIDR                    (0x0004)
 #define GICR_TYPER                   (0x0008)
@@ -165,6 +167,10 @@
 #define ICH_VMCR_BPR1_MASK           (7 << ICH_VMCR_BPR1_SHIFT)
 #define ICH_VMCR_ENG1_SHIFT          1
 #define ICH_VMCR_ENG1_MASK           (1 << ICH_VMCR_ENG1_SHIFT)
+#define ICH_VMCR_ENG0_SHIFT          0
+#define ICH_VMCR_ENG0_MASK           (1 << ICH_VMCR_ENG0_SHIFT)
+#define ICH_VMCR_PMR_SHIFT           24
+#define ICH_VMCR_PMR_MASK            (0xffUL << ICH_VMCR_PMR_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
@@ -182,6 +188,18 @@
 #define GICH_LR_GRP1                 (1UL<<60)
 #define GICH_LR_HW                   (1UL<<61)
 
+#define ICH_LR_PRIORITY_SHIFT        48
+#define ICH_LR_PRIORITY_MASK         (0xffULL << ICH_LR_PRIORITY_SHIFT)
+#define ICH_LR_EOI                   (1ULL << 41)
+#define ICH_LR_GROUP                 (1ULL << 60)
+#define ICH_LR_HW                    (1ULL << 61)
+#define ICH_LR_STATE                 (3ULL << 62)
+#define ICH_LR_PENDING_BIT           (1ULL << 62)
+#define ICH_LR_ACTIVE_BIT            (1ULL << 63)
+#define ICH_LR_PHYS_ID_SHIFT         32
+#define ICH_LR_PHYS_ID_MASK          (0x3ffULL << ICH_LR_PHYS_ID_SHIFT)
+#define ICH_LR_VIRTUAL_ID_MASK       ((1ULL << 32) - 1)
+
 #define GICH_VTR_NRLRGS              0x3f
 #define GICH_VTR_PRIBITS_MASK        0x7
 #define GICH_VTR_PRIBITS_SHIFT       29
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 08/10] Add ICV_EOIR1_EL1 handler

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

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.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vsysreg.c        | 136 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |   1 +
 xen/include/asm-arm/gic_v3_defs.h   |   4 ++
 3 files changed, 141 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 44c74d4217..393d167e56 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -230,6 +230,7 @@ void do_sysreg(struct cpu_user_regs *regs,
 
 #define ICC_IAR1_EL1_SPURIOUS    0x3ff
 #define VGIC_MAX_SPI             1019
+#define VGIC_MIN_LPI             8192
 
 static int  __vgic_v3_bpr_min(void)
 {
@@ -594,6 +595,137 @@ void handle_iar(struct cpu_user_regs *regs, int regidx, const union hsr hsr)
     __vgic_v3_read_iar(regs, regidx, hsr);
 }
 
+static int  __vgic_v3_find_active_lr(int intid, u64 *lr_val)
+{
+    int i;
+    unsigned int used_lrs =  gic_get_num_lrs();
+
+    for ( i = 0; i < used_lrs; i++ )
+    {
+        u64 val = __gicv3_ich_read_lr(i);
+
+        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
+            (val & ICH_LR_ACTIVE_BIT) )
+        {
+            *lr_val = val;
+            return i;
+        }
+    }
+
+    *lr_val = ICC_IAR1_EL1_SPURIOUS;
+    return -1;
+}
+
+static int  __vgic_v3_clear_highest_active_priority(void)
+{
+    u32 hap = 0;
+    int i;
+    u8 nr_apr_regs = vtr_to_nr_apr_regs(READ_SYSREG32(ICH_VTR_EL2));
+
+    for ( i = 0; i < nr_apr_regs; i++ )
+    {
+        u32 ap0, ap1;
+        int c0, c1;
+
+        ap0 = __vgic_v3_read_ap0rn(i);
+        ap1 = __vgic_v3_read_ap1rn(i);
+        if ( !ap0 && !ap1 )
+        {
+            hap += 32;
+            continue;
+        }
+
+        c0 = ap0 ? __ffs(ap0) : 32;
+        c1 = ap1 ? __ffs(ap1) : 32;
+
+        /* Always clear the LSB, which is the highest priority */
+        if (c0 < c1)
+        {
+            ap0 &= ~BIT(c0);
+            __vgic_v3_write_ap0rn(ap0, i);
+            hap += c0;
+        }
+        else
+        {
+            ap1 &= ~BIT(c1);
+            __vgic_v3_write_ap1rn(ap1, i);
+            hap += c1;
+        }
+
+        /* Rescale to 8 bits of priority */
+        return hap << __vgic_v3_bpr_min();
+    }
+
+    return GICV3_IDLE_PRIORITY;
+}
+
+static void  __vgic_v3_clear_active_lr(int lr, u64 lr_val)
+{
+    lr_val &= ~ICH_LR_ACTIVE_BIT;
+    if (lr_val & ICH_LR_HW)
+    {
+        u32 pid;
+
+        pid = (lr_val & ICH_LR_PHYS_ID_MASK) >> ICH_LR_PHYS_ID_SHIFT;
+        WRITE_SYSREG32(pid, ICC_DIR_EL1);
+    }
+    __gicv3_ich_write_lr(lr, lr_val);
+}
+
+static void  __vgic_v3_bump_eoicount(void)
+{
+    u32 hcr;
+
+    hcr = READ_SYSREG32(ICH_HCR_EL2);
+    hcr += 1 << ICH_HCR_EOIcount_SHIFT;
+    WRITE_SYSREG32(hcr, ICH_HCR_EL2);
+}
+
+static void  __vgic_v3_write_eoir(struct cpu_user_regs *regs, int regidx,
+                                  const union hsr hsr)
+{
+    u32 vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+    u32 vid = get_user_reg(regs, regidx);
+    u64 lr_val;
+    u8 lr_prio, act_prio;
+    int lr, grp;
+
+    grp = __vgic_v3_get_group(hsr);
+
+    /* Drop priority in any case */
+    act_prio = __vgic_v3_clear_highest_active_priority();
+
+    /* If EOIing an LPI, no deactivate to be performed */
+    if ( vid >= VGIC_MIN_LPI )
+        return;
+
+    /* EOImode == 1, nothing to be done here */
+    if ( vmcr & ICH_VMCR_EOIM_MASK )
+        return;
+
+    lr = __vgic_v3_find_active_lr(vid, &lr_val);
+    if ( lr == -1 )
+    {
+        __vgic_v3_bump_eoicount();
+        return;
+    }
+
+    lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
+
+    /* If priorities or group do not match, the guest has fscked-up. */
+    if ( grp != !!(lr_val & ICH_LR_GROUP) ||
+         __vgic_v3_pri_to_pre(lr_prio, vmcr, grp) != act_prio )
+        return;
+
+    /* Let's now perform the deactivation */
+    __vgic_v3_clear_active_lr(lr, lr_val);
+}
+
+void handle_eoi(struct cpu_user_regs *regs, int regidx, const union hsr hsr)
+{
+    __vgic_v3_write_eoir(regs, regidx, hsr);
+}
+
 int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int ret = 0;
@@ -614,6 +746,10 @@ int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
         handle_iar(regs, regidx, hsr);
         break;
 
+    case HSR_SYSREG_ICC_EOIR1_EL1:
+        handle_eoi(regs, regidx, hsr);
+        break;
+
     default:
         ret = -1;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index 53d2251840..f9110ebf9c 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -92,6 +92,7 @@
 #define HSR_SYSREG_ICC_BPR1_EL1   HSR_SYSREG(3,0,c12,c12,3)
 #define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
 #define HSR_SYSREG_ICC_IAR1_EL1   HSR_SYSREG(3,0,c12,c12,0)
+#define HSR_SYSREG_ICC_EOIR1_EL1  HSR_SYSREG(3,0,c12,c12,1)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index f22549a228..1bbcfd1c50 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -171,6 +171,10 @@
 #define ICH_VMCR_ENG0_MASK           (1 << ICH_VMCR_ENG0_SHIFT)
 #define ICH_VMCR_PMR_SHIFT           24
 #define ICH_VMCR_PMR_MASK            (0xffUL << ICH_VMCR_PMR_SHIFT)
+#define ICH_VMCR_EOIM_SHIFT          9
+#define ICH_VMCR_EOIM_MASK           (1 << ICH_VMCR_EOIM_SHIFT)
+#define ICH_HCR_EOIcount_SHIFT       27
+#define ICH_HCR_EOIcount_MASK        (0x1f << ICH_HCR_EOIcount_SHIFT)
 
 #define GICH_LR_VIRTUAL_MASK         0xffff
 #define GICH_LR_VIRTUAL_SHIFT        0
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 09/10] arm64: Add a handler for ICV_HPPIR1_EL1

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

Add a handler for reading the guest's view of the ICV_HPPIR1_EL1
register. This is a simple parsing of the available LRs, extracting the
highest available interrupt.

Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/arm64/vsysreg.c        | 24 ++++++++++++++++++++++++
 xen/include/asm-arm/arm64/sysregs.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 393d167e56..9bee678002 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -726,6 +726,26 @@ void handle_eoi(struct cpu_user_regs *regs, int regidx, const union hsr hsr)
     __vgic_v3_write_eoir(regs, regidx, hsr);
 }
 
+void handle_hppir1(struct cpu_user_regs *regs, int regidx, const union hsr hsr)
+{
+    u64 lr_val;
+    int lr, lr_grp, grp;
+    u32 vmcr = READ_SYSREG32(ICH_VMCR_EL2);
+
+    grp = __vgic_v3_get_group(hsr);
+    lr = __vgic_v3_highest_priority_lr(regs, vmcr, &lr_val);
+
+    if ( lr == -1 )
+        goto spurious;
+
+    lr_grp = !!(lr_val & ICH_LR_GROUP);
+    if ( lr_grp != grp )
+        lr_val = ICC_IAR1_EL1_SPURIOUS;
+
+spurious:
+    set_user_reg(regs, regidx, lr_val & ICH_LR_VIRTUAL_ID_MASK);
+}
+
 int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int ret = 0;
@@ -750,6 +770,10 @@ int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
         handle_eoi(regs, regidx, hsr);
         break;
 
+    case HSR_SYSREG_ICC_HPPIR1_EL1:
+        handle_hppir1(regs, regidx, hsr);
+        break;
+
     default:
         ret = -1;
         break;
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h
index f9110ebf9c..c23c4a33b2 100644
--- a/xen/include/asm-arm/arm64/sysregs.h
+++ b/xen/include/asm-arm/arm64/sysregs.h
@@ -93,6 +93,7 @@
 #define HSR_SYSREG_ICC_IGRPEN1_EL1 HSR_SYSREG(3,0,c12,c12,7)
 #define HSR_SYSREG_ICC_IAR1_EL1   HSR_SYSREG(3,0,c12,c12,0)
 #define HSR_SYSREG_ICC_EOIR1_EL1  HSR_SYSREG(3,0,c12,c12,1)
+#define HSR_SYSREG_ICC_HPPIR1_EL1 HSR_SYSREG(3,0,c12,c12,2)
 #define HSR_SYSREG_CONTEXTIDR_EL1 HSR_SYSREG(3,0,c13,c0,1)
 
 #define HSR_SYSREG_PMCR_EL0       HSR_SYSREG(3,3,c9,c12,0)
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 10/10] Enable Trapping of Group1 registers which is controlled by command line

[mjaggi]

From: Manish Jaggi <manish.jaggi@cavium.com>

In order to be able to trap Group-1 GICv3 system registers, we need to
set ICH_HCR_EL2.TALL1 before entering the guest. This is controlled by
the command line parameter group1_trap.

Singed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
---
 xen/arch/arm/gic-v3.c     | 11 ++++++++++-
 xen/include/asm-arm/gic.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 5dba8bc932..f22877c468 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -833,9 +833,12 @@ static void gicv3_cpu_disable(void)
     isb();
 }
 
+static unsigned int group1_trap = 0;
+integer_param("group1_trap", group1_trap);
+
 static void gicv3_hyp_init(void)
 {
-    uint32_t vtr;
+    uint32_t vtr, reg32;
 
     vtr = READ_SYSREG32(ICH_VTR_EL2);
     gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
@@ -847,6 +850,12 @@ static void gicv3_hyp_init(void)
 
     WRITE_SYSREG32(GICH_VMCR_EOI | GICH_VMCR_VENG1, ICH_VMCR_EL2);
     WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
+
+    reg32 = READ_SYSREG32(ICH_HCR_EL2);
+    if ( group1_trap )
+        reg32 |= GICH_HCR_TALL1;
+
+    WRITE_SYSREG32(reg32, ICH_HCR_EL2);
 }
 
 /* Set up the per-CPU parts of the GIC for a secondary CPU */
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index d3d7bda50d..e4c77fefd6 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -117,6 +117,7 @@
 #define GICH_HCR_VGRP0DIE (1 << 5)
 #define GICH_HCR_VGRP1EIE (1 << 6)
 #define GICH_HCR_VGRP1DIE (1 << 7)
+#define GICH_HCR_TALL1    (1 << 12)
 
 #define GICH_MISR_EOI     (1 << 0)
 #define GICH_MISR_U       (1 << 1)
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 01/10] Add CONFIG_VGIC_ERRATA

[Julien Grall]

Hi Manish,

On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
> 
> Add a config option to enable VGIC Errata Code in Xen. Platforms which do not
> have this errta can compile out this feature.

s/errta/errata/

> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>   xen/arch/arm/Kconfig | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index f58019d6ed..2966e3a3d3 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -59,6 +59,15 @@ config SBSA_VUART_CONSOLE
>   
>   endmenu
>   
> +menu "Errata Workarounds"
> +       depends on ARM_64

I would much prefer to see the memu "ARM errata workaround via..." 
renamed to "Errata Workarounds". So we have only one menu with all 
workarounds.

> +
> +config VGIC_ERRATA
> +       bool "Handler code for emulation of Group0/1 vGIC registers for VGC Errata"

The title does not help the user to know when to select this option or not.

It looks like to me you want to do something similar to commit 
690a341577f9 "arm64: Add workaround for Cavium Thunder erratum 30115" in 
Linux where the config is called CAVIUM_ERRATA_30115 and a proper 
description explaining the platform affected.

> +       depends on ARM_64

I think this should depend on HAS_GICV3.

> +
> +endmenu
> +
>   menu "ARM errata workaround via the alternative framework"
>   	depends on HAS_ALTERNATIVE
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Julien Grall]

Hi Manish,

On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
> 
> In order to start handling guest access to GICv3 system registers,
> let's add a hook that will get called when we trap a system register
> access.
> This handling code is kept independent of other traps.
> Set CONFIG_VGIC_ERRATA to enable this code.

The commit message should explain the rationale behind calling 
do_fixup_vgic_errata from do_trap_guest_sync.


> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>   xen/arch/arm/arm64/vsysreg.c      | 17 +++++++++++++++++
>   xen/arch/arm/traps.c              | 11 +++++++++++
>   xen/include/asm-arm/arm64/traps.h |  2 ++
>   3 files changed, 30 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index c57ac12503..a5c17e460f 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -219,6 +219,23 @@ void do_sysreg(struct cpu_user_regs *regs,
>       regs->pc += 4;
>   }
>   
> +#ifdef CONFIG_VGIC_ERRATA
> +int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)

Can we have a separate file to emulate vgic system registers? We might 
want to re-use some of the code for 32-bit guests in the future. Also as 
far as I can tell, this will only cover GICv3 system registers. To be 
more specific it will only be cpu interface register.

So I think you should name vgic_v3_handle_cpuif_access(...).

> +{
> +    int ret = 0;

You are returning either -1 or 0. Can we please use bool in that case?

> +
> +    local_irq_disable();
> +    switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
You will enter in do_trap_guest_sync for various reasons. So you should 
check that exception class correspond to a trapped of system 
instructions (e.g HSR_EC_SYSREG). Otherwise you will emulate sysreg on 
other encoding by mistake.

> +    {
> +    default:
> +        ret = -1;
> +        break;
> +    }
> +    local_irq_enable();
> +
> +    return ret;
> +}
> +#endif
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f6f6de3691..d4f0581d33 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>   {
>       const union hsr hsr = { .bits = regs->hsr };
>   
> +#ifdef CONFIG_VGIC_ERRATA
> +    int ret;
> +
> +    ret  = do_fixup_vgic_errata(regs,hsr);

Most likely distros will have the errata built in Xen because they want 
the hypervisor to run on many platforms. And we don't want to affect the 
  on platform that are not affected by the errata.

So we want to skip the call. Have a look at CHECK_WORKAROUND_HELPER.

> +    if ( !ret )
> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +#endif
> +
>       enter_hypervisor_head(regs);
>   
>       switch (hsr.ec) {
> diff --git a/xen/include/asm-arm/arm64/traps.h b/xen/include/asm-arm/arm64/traps.h
> index 2379b578cb..7378a1b022 100644
> --- a/xen/include/asm-arm/arm64/traps.h
> +++ b/xen/include/asm-arm/arm64/traps.h
> @@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
>   
>   void do_sysreg(struct cpu_user_regs *regs,
>                  const union hsr hsr);
> +int do_fixup_vgic_errata(struct cpu_user_regs *regs,
> +               const union hsr hsr);
>   
>   #endif /* __ASM_ARM64_TRAPS__ */
>   /*
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/10] arm64: Add ICV_BPR1_EL1 handler

[Julien Grall]

Hi Manish,

On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
> 
> 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.

This commit (and likely the followings) is coming from Linux, right? If 
it matches commit from Linux, then you need to keep tags and point to 
the Linux commit. See commit 7762c2d6f4 in Xen as an example to how to 
do it.

If you make changes for Xen, then write "Adapted for Xen...".

But looking at the patch the major difference is you use Xen coding 
style. The rest is pretty much use Xen name for access register and 
adding missing define.

I think it would be beneficial for Xen to re-use Linux code. The 
compatibility layer should be very limited. Stefano any opinions?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 06/10] Expose gicv3_ich_read/write_lr

[Julien Grall]

Hi,

On 16/01/18 15:43, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
> 
> gicv3_ich_read/write_lr functions are static in gic-v3.c
> This patch creates wrapper functions which can be used from outside the file.
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>   xen/arch/arm/gic-v3.c        | 10 ++++++++++
>   xen/include/asm-arm/gic_v3.h |  7 +++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 473e26111f..5dba8bc932 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -184,6 +184,11 @@ static uint64_t gicv3_ich_read_lr(int lr)
>       }
>   }
>   
> +uint64_t __gicv3_ich_read_lr(int lr)

I see no reason to have a wrapper with exactly the same parameters. Just 
export the current one.

But I think I would prefer the function to be redefined in the cpu if 
implementation. So we vGIC errata is fully separated from the rest of Xen.

> +{
> +    return gicv3_ich_read_lr(lr);
> +}
> +
>   static void gicv3_ich_write_lr(int lr, uint64_t val)
>   {
>       switch ( lr )
> @@ -242,6 +247,11 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>       isb();
>   }
>   
> +void __gicv3_ich_write_lr(int lr, uint64_t val)
> +{
> +    return gicv3_ich_write_lr(lr, val);
> +}
> +
>   /*
>    * System Register Enable (SRE). Enable to access CPU & Virtual
>    * interface registers as system registers in EL2
> diff --git a/xen/include/asm-arm/gic_v3.h b/xen/include/asm-arm/gic_v3.h
> new file mode 100644
> index 0000000000..544aad5932
> --- /dev/null
> +++ b/xen/include/asm-arm/gic_v3.h
> @@ -0,0 +1,7 @@
> +#ifndef GICV3_H
> +#define GICV3_H
> +
> +uint64_t __gicv3_ich_read_lr(int lr);
> +void __gicv3_ich_write_lr(int lr, uint64_t val);
> +
> +#endif
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 10/10] Enable Trapping of Group1 registers which is controlled by command line

[Julien Grall]

Hi Manish,

On 16/01/18 15:43, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
> 
> In order to be able to trap Group-1 GICv3 system registers, we need to
> set ICH_HCR_EL2.TALL1 before entering the guest. This is controlled by
> the command line parameter group1_trap.

I was expecting a patch to enable group1_trap by default on affected 
platform.

> 
> Singed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>   xen/arch/arm/gic-v3.c     | 11 ++++++++++-
>   xen/include/asm-arm/gic.h |  1 +
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 5dba8bc932..f22877c468 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -833,9 +833,12 @@ static void gicv3_cpu_disable(void)
>       isb();
>   }
>   
> +static unsigned int group1_trap = 0;
> +integer_param("group1_trap", group1_trap);

New parameter should be describe in docs/misc/xen-command-line.markdown.

Also, you likely want to use a boolean_param here.

> +
>   static void gicv3_hyp_init(void)
>   {
> -    uint32_t vtr;
> +    uint32_t vtr, reg32;
>   
>       vtr = READ_SYSREG32(ICH_VTR_EL2);
>       gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
> @@ -847,6 +850,12 @@ static void gicv3_hyp_init(void)
>   
>       WRITE_SYSREG32(GICH_VMCR_EOI | GICH_VMCR_VENG1, ICH_VMCR_EL2);
>       WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
> +
> +    reg32 = READ_SYSREG32(ICH_HCR_EL2);

There are no point to read ICH_HCR_EL2. You know the value (see the line 
above).

So this code could simplified as:

reg32 = GICH_HCR_EN;
reg32 |= (group1_trap) ? GICH_HCR_TALL1 : 0;

WRITE_SYSREG32(reg32, ICH_HCR_EL2);

> +    if ( group1_trap )
> +        reg32 |= GICH_HCR_TALL1;
> +
> +    WRITE_SYSREG32(reg32, ICH_HCR_EL2);
>   }
>   
>   /* Set up the per-CPU parts of the GIC for a secondary CPU */
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index d3d7bda50d..e4c77fefd6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -117,6 +117,7 @@
>   #define GICH_HCR_VGRP0DIE (1 << 5)
>   #define GICH_HCR_VGRP1EIE (1 << 6)
>   #define GICH_HCR_VGRP1DIE (1 << 7)
> +#define GICH_HCR_TALL1    (1 << 12)
>   
>   #define GICH_MISR_EOI     (1 << 0)
>   #define GICH_MISR_U       (1 << 1) >

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Julien Grall]

Hi,

I forgot to mention one thing about the placement of do_fixup_vgic_errata.

On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f6f6de3691..d4f0581d33 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>   {
>       const union hsr hsr = { .bits = regs->hsr };
>   
> +#ifdef CONFIG_VGIC_ERRATA
> +    int ret;
> +
> +    ret  = do_fixup_vgic_errata(regs,hsr);
> +    if ( !ret )
> +    {
> +        advance_pc(regs, hsr);
> +        return;

I am fully aware that I suggested this solution and still support that 
the vGIC errata should be fully separated. After all, it deals with 
hardware bug and the errata will just update the LRs as the hardware 
would do.

enter_hypervisor_head() will sync the LRs state to the internal vGIC 
state. leave_hypervisor_head() will process pending softirq and 
write/update the LRs based on the internal vGIC state.

As you rightfully did, the do_fixup_vgic_errata should be called before 
syncing the LRs. However, even if you return early here, you will still 
execute leave_hypervisor_tail(). This mean that pending softirqs will be 
processed and potentially the vCPU rescheduled. Because the LRs were not 
synced (enter_hypervisor_head()) was not called, then the vGIC state 
will not out-of-date and would lead to all sort of potential issues.

As the vGIC errata implies trapping the register such as IAR1 (reading 
interrupt), we want to get a fastpath for it (e.g not trying to execute 
softirq...). So I think we should bypass leave_hypervisor_tail(). I am 
not entirely sure how to do it nicely thought.

Cheers,

> +    }
> +#endif
> +
>       enter_hypervisor_head(regs);
>   
>       switch (hsr.ec) {
> diff --git a/xen/include/asm-arm/arm64/traps.h b/xen/include/asm-arm/arm64/traps.h
> index 2379b578cb..7378a1b022 100644
> --- a/xen/include/asm-arm/arm64/traps.h
> +++ b/xen/include/asm-arm/arm64/traps.h
> @@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
>   
>   void do_sysreg(struct cpu_user_regs *regs,
>                  const union hsr hsr);
> +int do_fixup_vgic_errata(struct cpu_user_regs *regs,
> +               const union hsr hsr);
>   
>   #endif /* __ASM_ARM64_TRAPS__ */
>   /*
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Manish Jaggi]

On 01/25/2018 11:37 PM, Julien Grall wrote:
> Hi,
>
> I forgot to mention one thing about the placement of 
> do_fixup_vgic_errata.
>
> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index f6f6de3691..d4f0581d33 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
>> *regs)
>>   {
>>       const union hsr hsr = { .bits = regs->hsr };
>>   +#ifdef CONFIG_VGIC_ERRATA
>> +    int ret;
>> +
>> +    ret  = do_fixup_vgic_errata(regs,hsr);
>> +    if ( !ret )
>> +    {
>> +        advance_pc(regs, hsr);
>> +        return;
>
> I am fully aware that I suggested this solution and still support that 
> the vGIC errata should be fully separated. After all, it deals with 
> hardware bug and the errata will just update the LRs as the hardware 
> would do.
>
> enter_hypervisor_head() will sync the LRs state to the internal vGIC 
> state. leave_hypervisor_head() will process pending softirq and 
> write/update the LRs based on the internal vGIC state.
>
> As you rightfully did, the do_fixup_vgic_errata should be called 
> before syncing the LRs. However, even if you return early here, you 
> will still execute leave_hypervisor_tail(). This mean that pending 
> softirqs will be processed and potentially the vCPU rescheduled. 
> Because the LRs were not synced (enter_hypervisor_head()) was not 
> called, then the vGIC state will not out-of-date and would lead to all 
> sort of potential issues.
>
> As the vGIC errata implies trapping the register such as IAR1 (reading 
> interrupt), we want to get a fastpath for it (e.g not trying to 
> execute softirq...). So I think we should bypass 
> leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
> thought.
>

How about adding a check for group1_trap enable in leave_hypervisor_tail().

void leave_hypervisor_tail(void)
{
+if (group1_trap)
+   return;
     while (1)


> Cheers,
>
>> +    }
>> +#endif
>> +
>>       enter_hypervisor_head(regs);
>>         switch (hsr.ec) {
>> diff --git a/xen/include/asm-arm/arm64/traps.h 
>> b/xen/include/asm-arm/arm64/traps.h
>> index 2379b578cb..7378a1b022 100644
>> --- a/xen/include/asm-arm/arm64/traps.h
>> +++ b/xen/include/asm-arm/arm64/traps.h
>> @@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs 
>> *regs, int instr_len);
>>     void do_sysreg(struct cpu_user_regs *regs,
>>                  const union hsr hsr);
>> +int do_fixup_vgic_errata(struct cpu_user_regs *regs,
>> +               const union hsr hsr);
>>     #endif /* __ASM_ARM64_TRAPS__ */
>>   /*
>>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 10/10] Enable Trapping of Group1 registers which is controlled by command line

[Manish Jaggi]

On 01/25/2018 10:30 PM, Julien Grall wrote:
> Hi Manish,
>
> On 16/01/18 15:43, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <manish.jaggi@cavium.com>
>>
>> In order to be able to trap Group-1 GICv3 system registers, we need to
>> set ICH_HCR_EL2.TALL1 before entering the guest. This is controlled by
>> the command line parameter group1_trap.
>
> I was expecting a patch to enable group1_trap by default on affected 
> platform.
ok I will add another patch for that.
>
[...]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 06/10] Expose gicv3_ich_read/write_lr

[Manish Jaggi]

On 01/25/2018 10:22 PM, Julien Grall wrote:
> Hi,
>
> On 16/01/18 15:43, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <manish.jaggi@cavium.com>
>>
>> gicv3_ich_read/write_lr functions are static in gic-v3.c
>> This patch creates wrapper functions which can be used from outside 
>> the file.
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>> ---
>>   xen/arch/arm/gic-v3.c        | 10 ++++++++++
>>   xen/include/asm-arm/gic_v3.h |  7 +++++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 473e26111f..5dba8bc932 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -184,6 +184,11 @@ static uint64_t gicv3_ich_read_lr(int lr)
>>       }
>>   }
>>   +uint64_t __gicv3_ich_read_lr(int lr)
>
> I see no reason to have a wrapper with exactly the same parameters. 
> Just export the current one.
>
> But I think I would prefer the function to be redefined in the cpu if 
> implementation. So we vGIC errata is fully separated from the rest of 
> Xen.
ok, will do.
>
>> +{
>> +    return gicv3_ich_read_lr(lr);
>> +}
>> +
>>   static void gicv3_ich_write_lr(int lr, uint64_t val)
>>   {
>>       switch ( lr )
>> @@ -242,6 +247,11 @@ static void gicv3_ich_write_lr(int lr, uint64_t 
>> val)
>>       isb();
>>   }
>>   +void __gicv3_ich_write_lr(int lr, uint64_t val)
>> +{
>> +    return gicv3_ich_write_lr(lr, val);
>> +}
>> +
>>   /*
>>    * System Register Enable (SRE). Enable to access CPU & Virtual
>>    * interface registers as system registers in EL2
>> diff --git a/xen/include/asm-arm/gic_v3.h b/xen/include/asm-arm/gic_v3.h
>> new file mode 100644
>> index 0000000000..544aad5932
>> --- /dev/null
>> +++ b/xen/include/asm-arm/gic_v3.h
>> @@ -0,0 +1,7 @@
>> +#ifndef GICV3_H
>> +#define GICV3_H
>> +
>> +uint64_t __gicv3_ich_read_lr(int lr);
>> +void __gicv3_ich_write_lr(int lr, uint64_t val);
>> +
>> +#endif
>>
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/10] arm64: Add ICV_BPR1_EL1 handler

[Manish Jaggi]

On 01/25/2018 10:14 PM, Julien Grall wrote:
> Hi Manish,
>
> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <manish.jaggi@cavium.com>
>>
>> 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.
>
> This commit (and likely the followings) is coming from Linux, right? 
> If it matches commit from Linux, then you need to keep tags and point 
> to the Linux commit. See commit 7762c2d6f4 in Xen as an example to how 
> to do it.
>
> If you make changes for Xen, then write "Adapted for Xen...".
ok
>
> But looking at the patch the major difference is you use Xen coding 
> style. The rest is pretty much use Xen name for access register and 
> adding missing define.
>
> I think it would be beneficial for Xen to re-use Linux code. The 
> compatibility layer should be very limited. Stefano any opinions?
So when you mean linux code, you refer to a patch or importing the code 
file from linux ?
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Julien Grall]

Hi Manish,

On 01/02/18 08:51, Manish Jaggi wrote:
> On 01/25/2018 11:37 PM, Julien Grall wrote:
>> Hi,
>>
>> I forgot to mention one thing about the placement of 
>> do_fixup_vgic_errata.
>>
>> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index f6f6de3691..d4f0581d33 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
>>> *regs)
>>>   {
>>>       const union hsr hsr = { .bits = regs->hsr };
>>>   +#ifdef CONFIG_VGIC_ERRATA
>>> +    int ret;
>>> +
>>> +    ret  = do_fixup_vgic_errata(regs,hsr);
>>> +    if ( !ret )
>>> +    {
>>> +        advance_pc(regs, hsr);
>>> +        return;
>>
>> I am fully aware that I suggested this solution and still support that 
>> the vGIC errata should be fully separated. After all, it deals with 
>> hardware bug and the errata will just update the LRs as the hardware 
>> would do.
>>
>> enter_hypervisor_head() will sync the LRs state to the internal vGIC 
>> state. leave_hypervisor_head() will process pending softirq and 
>> write/update the LRs based on the internal vGIC state.
>>
>> As you rightfully did, the do_fixup_vgic_errata should be called 
>> before syncing the LRs. However, even if you return early here, you 
>> will still execute leave_hypervisor_tail(). This mean that pending 
>> softirqs will be processed and potentially the vCPU rescheduled. 
>> Because the LRs were not synced (enter_hypervisor_head()) was not 
>> called, then the vGIC state will not out-of-date and would lead to all 
>> sort of potential issues.
>>
>> As the vGIC errata implies trapping the register such as IAR1 (reading 
>> interrupt), we want to get a fastpath for it (e.g not trying to 
>> execute softirq...). So I think we should bypass 
>> leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
>> thought.
>>
> 
> How about adding a check for group1_trap enable in leave_hypervisor_tail().
> 
> void leave_hypervisor_tail(void)
> {
> +if (group1_trap)
> +   return;

I guess you mean the variable you introduced in patch #10. In that case, 
this would be totally wrong. You only want to skip 
leave_hypervisor_tail() when you are handling a ICV_* System registers.

What you want is adding a bool in the structure cpu_info to tell whether 
leave_hypervisor_tail() should be skipped.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 03/10] arm64: Add ICV_BPR1_EL1 handler

[Julien Grall]

Hi Manish,

On 01/02/18 08:57, Manish Jaggi wrote:
> On 01/25/2018 10:14 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>>> From: Manish Jaggi <manish.jaggi@cavium.com>
>>>
>>> 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.
>>
>> This commit (and likely the followings) is coming from Linux, right? 
>> If it matches commit from Linux, then you need to keep tags and point 
>> to the Linux commit. See commit 7762c2d6f4 in Xen as an example to how 
>> to do it.
>>
>> If you make changes for Xen, then write "Adapted for Xen...".
> ok
>>
>> But looking at the patch the major difference is you use Xen coding 
>> style. The rest is pretty much use Xen name for access register and 
>> adding missing define.
>>
>> I think it would be beneficial for Xen to re-use Linux code. The 
>> compatibility layer should be very limited. Stefano any opinions?
> So when you mean linux code, you refer to a patch or importing the code 
> file from linux ?

Importing the code from Linux. And then a patch to adapt for Xen (it 
should be fairly small).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 01/10] Add CONFIG_VGIC_ERRATA

[Manish Jaggi]

Hi Julien,


On 01/25/2018 07:18 PM, Julien Grall wrote:
> Hi Manish,
>
> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <manish.jaggi@cavium.com>
>>
>> Add a config option to enable VGIC Errata Code in Xen. Platforms 
>> which do not
>> have this errta can compile out this feature.
>
> s/errta/errata/
>
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>> ---
>>   xen/arch/arm/Kconfig | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index f58019d6ed..2966e3a3d3 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -59,6 +59,15 @@ config SBSA_VUART_CONSOLE
>>     endmenu
>>   +menu "Errata Workarounds"
>> +       depends on ARM_64
>
> I would much prefer to see the memu "ARM errata workaround via..." 
> renamed to "Errata Workarounds". So we have only one menu with all 
> workarounds.
I am not sure it would work as
menu "ARM errata workaround via the alternative framework"
         depends on HAS_ALTERNATIVE

which may not be correct for vGIC errata.
>
>> +
>> +config VGIC_ERRATA
>> +       bool "Handler code for emulation of Group0/1 vGIC registers 
>> for VGC Errata"
>
> The title does not help the user to know when to select this option or 
> not.
>
> It looks like to me you want to do something similar to commit 
> 690a341577f9 "arm64: Add workaround for Cavium Thunder erratum 30115" 
> in Linux where the config is called CAVIUM_ERRATA_30115 and a proper 
> description explaining the platform affected.
>
ok
>> +       depends on ARM_64
>
> I think this should depend on HAS_GICV3.
>
ok
>> +
>> +endmenu
>> +
>>   menu "ARM errata workaround via the alternative framework"
>>       depends on HAS_ALTERNATIVE
>>
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 01/10] Add CONFIG_VGIC_ERRATA

[Julien Grall]

On 21/02/18 15:17, Manish Jaggi wrote:
> Hi Julien,
> 
> 
> On 01/25/2018 07:18 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>>> From: Manish Jaggi <manish.jaggi@cavium.com>
>>>
>>> Add a config option to enable VGIC Errata Code in Xen. Platforms 
>>> which do not
>>> have this errta can compile out this feature.
>>
>> s/errta/errata/
>>
>>>
>>> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
>>> ---
>>>   xen/arch/arm/Kconfig | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index f58019d6ed..2966e3a3d3 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -59,6 +59,15 @@ config SBSA_VUART_CONSOLE
>>>     endmenu
>>>   +menu "Errata Workarounds"
>>> +       depends on ARM_64
>>
>> I would much prefer to see the memu "ARM errata workaround via..." 
>> renamed to "Errata Workarounds". So we have only one menu with all 
>> workarounds.
> I am not sure it would work as
> menu "ARM errata workaround via the alternative framework"
>          depends on HAS_ALTERNATIVE
> 
> which may not be correct for vGIC errata.

vGIC errata are likely going to be based on alternative. So the call to 
the function could be avoided on non-affected platform.

But as alternative is supported unconditionally by both Arm64 and Arm32, 
this depends could just be dropped.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Manish Jaggi]

On 02/01/2018 04:24 PM, Julien Grall wrote:
> Hi Manish,
>
> On 01/02/18 08:51, Manish Jaggi wrote:
>> On 01/25/2018 11:37 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> I forgot to mention one thing about the placement of 
>>> do_fixup_vgic_errata.
>>>
>>> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index f6f6de3691..d4f0581d33 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
>>>> *regs)
>>>>   {
>>>>       const union hsr hsr = { .bits = regs->hsr };
>>>>   +#ifdef CONFIG_VGIC_ERRATA
>>>> +    int ret;
>>>> +
>>>> +    ret  = do_fixup_vgic_errata(regs,hsr);
>>>> +    if ( !ret )
>>>> +    {
>>>> +        advance_pc(regs, hsr);
>>>> +        return;
>>>
>>> I am fully aware that I suggested this solution and still support 
>>> that the vGIC errata should be fully separated. After all, it deals 
>>> with hardware bug and the errata will just update the LRs as the 
>>> hardware would do.
>>>
>>> enter_hypervisor_head() will sync the LRs state to the internal vGIC 
>>> state. leave_hypervisor_head() will process pending softirq and 
>>> write/update the LRs based on the internal vGIC state.
>>>
>>> As you rightfully did, the do_fixup_vgic_errata should be called 
>>> before syncing the LRs. However, even if you return early here, you 
>>> will still execute leave_hypervisor_tail(). This mean that pending 
>>> softirqs will be processed and potentially the vCPU rescheduled. 
>>> Because the LRs were not synced (enter_hypervisor_head()) was not 
>>> called, then the vGIC state will not out-of-date and would lead to 
>>> all sort of potential issues.
>>>
>>> As the vGIC errata implies trapping the register such as IAR1 
>>> (reading interrupt), we want to get a fastpath for it (e.g not 
>>> trying to execute softirq...). So I think we should bypass 
>>> leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
>>> thought.
>>>
>>
>> How about adding a check for group1_trap enable in 
>> leave_hypervisor_tail().
>>
>> void leave_hypervisor_tail(void)
>> {
>> +if (group1_trap)
>> +   return;
>
> I guess you mean the variable you introduced in patch #10. In that 
> case, this would be totally wrong. You only want to skip 
> leave_hypervisor_tail() when you are handling a ICV_* System registers.
>
> What you want is adding a bool in the structure cpu_info to tell 
> whether leave_hypervisor_tail() should be skipped.
>
Why would it be wrong? I don't follow our point.
Also adding a flag in cpu_info would be overkill, use of a global 
variable is simpler.
I am planning to remove group1_trap as a command line, and use it as

static void gicv3_hyp_init(void)
{
...
#ifdef CONFIG_CAVIUM_ERRATUM_30115
     if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
         group1_trap = 1;
#endif
...
}

> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Manish Jaggi]

On 02/26/2018 12:12 PM, Manish Jaggi wrote:
>
>
> On 02/01/2018 04:24 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 01/02/18 08:51, Manish Jaggi wrote:
>>> On 01/25/2018 11:37 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> I forgot to mention one thing about the placement of 
>>>> do_fixup_vgic_errata.
>>>>
>>>> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>>> index f6f6de3691..d4f0581d33 100644
>>>>> --- a/xen/arch/arm/traps.c
>>>>> +++ b/xen/arch/arm/traps.c
>>>>> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct 
>>>>> cpu_user_regs *regs)
>>>>>   {
>>>>>       const union hsr hsr = { .bits = regs->hsr };
>>>>>   +#ifdef CONFIG_VGIC_ERRATA
>>>>> +    int ret;
>>>>> +
>>>>> +    ret  = do_fixup_vgic_errata(regs,hsr);
>>>>> +    if ( !ret )
>>>>> +    {
>>>>> +        advance_pc(regs, hsr);
>>>>> +        return;
>>>>
>>>> I am fully aware that I suggested this solution and still support 
>>>> that the vGIC errata should be fully separated. After all, it deals 
>>>> with hardware bug and the errata will just update the LRs as the 
>>>> hardware would do.
>>>>
>>>> enter_hypervisor_head() will sync the LRs state to the internal 
>>>> vGIC state. leave_hypervisor_head() will process pending softirq 
>>>> and write/update the LRs based on the internal vGIC state.
>>>>
>>>> As you rightfully did, the do_fixup_vgic_errata should be called 
>>>> before syncing the LRs. However, even if you return early here, you 
>>>> will still execute leave_hypervisor_tail(). This mean that pending 
>>>> softirqs will be processed and potentially the vCPU rescheduled. 
>>>> Because the LRs were not synced (enter_hypervisor_head()) was not 
>>>> called, then the vGIC state will not out-of-date and would lead to 
>>>> all sort of potential issues.
>>>>
>>>> As the vGIC errata implies trapping the register such as IAR1 
>>>> (reading interrupt), we want to get a fastpath for it (e.g not 
>>>> trying to execute softirq...). So I think we should bypass 
>>>> leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
>>>> thought.
>>>>
>>>
>>> How about adding a check for group1_trap enable in 
>>> leave_hypervisor_tail().
>>>
>>> void leave_hypervisor_tail(void)
>>> {
>>> +if (group1_trap)
>>> +   return;
>>
>> I guess you mean the variable you introduced in patch #10. In that 
>> case, this would be totally wrong. You only want to skip 
>> leave_hypervisor_tail() when you are handling a ICV_* System registers.
>>
>> What you want is adding a bool in the structure cpu_info to tell 
>> whether leave_hypervisor_tail() should be skipped.
>>
> Why would it be wrong? I don't follow our point.
> Also adding a flag in cpu_info would be overkill, use of a global 
> variable is simpler.
> I am planning to remove group1_trap as a command line, and use it as
>
> static void gicv3_hyp_init(void)
> {
> ...
> #ifdef CONFIG_CAVIUM_ERRATUM_30115
>     if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
>         group1_trap = 1;
> #endif
> ...
> }
Attaching test patch

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 473e26111f..581c07b274 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -64,6 +64,7 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
  #define GICD                   (gicv3.map_dbase)
  #define GICD_RDIST_BASE        (this_cpu(rbase))
  #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
+static bool group1_trap = 0;

  /*
   * Saves all 16(Max) LR registers. Though number of LRs implemented
@@ -825,7 +826,7 @@ static void gicv3_cpu_disable(void)

  static void gicv3_hyp_init(void)
  {
-    uint32_t vtr;
+    uint32_t vtr, reg32;

      vtr = READ_SYSREG32(ICH_VTR_EL2);
      gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
@@ -837,6 +838,10 @@ static void gicv3_hyp_init(void)

      WRITE_SYSREG32(GICH_VMCR_EOI | GICH_VMCR_VENG1, ICH_VMCR_EL2);
      WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
+
+    reg32 = GICH_HCR_EN;
+    reg32 |= (group1_trap) ? GICH_HCR_TALL1 : 0;
+    WRITE_SYSREG32(reg32, ICH_HCR_EL2);
  }

  /* Set up the per-CPU parts of the GIC for a secondary CPU */
@@ -1651,6 +1656,11 @@ static int __init gicv3_init(void)
          return -ENODEV;
      }

+#ifdef CONFIG_CAVIUM_ERRATUM_30115
+    if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
+        group1_trap = 1;
+#endif
+
      if ( acpi_disabled )
          gicv3_dt_init();
      else
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..dbee0c322f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2295,6 +2295,10 @@ void do_trap_fiq(struct cpu_user_regs *regs)

  void leave_hypervisor_tail(void)
  {
+#ifdef CONFIG_CAVIUM_ERRATUM_30115
+    if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
+        return;
+#endif
      while (1)
      {
          local_irq_disable();
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index d3d7bda50d..e4c77fefd6 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -117,6 +117,7 @@
  #define GICH_HCR_VGRP0DIE (1 << 5)
  #define GICH_HCR_VGRP1EIE (1 << 6)
  #define GICH_HCR_VGRP1DIE (1 << 7)
+#define GICH_HCR_TALL1    (1 << 12)

  #define GICH_MISR_EOI     (1 << 0)
  #define GICH_MISR_U       (1 << 1)

>
>> Cheers,
>>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Julien Grall]

Hi Manish,

On 26/02/18 06:42, Manish Jaggi wrote:
> 
> 
> On 02/01/2018 04:24 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 01/02/18 08:51, Manish Jaggi wrote:
>>> On 01/25/2018 11:37 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> I forgot to mention one thing about the placement of 
>>>> do_fixup_vgic_errata.
>>>>
>>>> On 16/01/18 15:42, mjaggi@caviumnetworks.com wrote:
>>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>>> index f6f6de3691..d4f0581d33 100644
>>>>> --- a/xen/arch/arm/traps.c
>>>>> +++ b/xen/arch/arm/traps.c
>>>>> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs 
>>>>> *regs)
>>>>>   {
>>>>>       const union hsr hsr = { .bits = regs->hsr };
>>>>>   +#ifdef CONFIG_VGIC_ERRATA
>>>>> +    int ret;
>>>>> +
>>>>> +    ret  = do_fixup_vgic_errata(regs,hsr);
>>>>> +    if ( !ret )
>>>>> +    {
>>>>> +        advance_pc(regs, hsr);
>>>>> +        return;
>>>>
>>>> I am fully aware that I suggested this solution and still support 
>>>> that the vGIC errata should be fully separated. After all, it deals 
>>>> with hardware bug and the errata will just update the LRs as the 
>>>> hardware would do.
>>>>
>>>> enter_hypervisor_head() will sync the LRs state to the internal vGIC 
>>>> state. leave_hypervisor_head() will process pending softirq and 
>>>> write/update the LRs based on the internal vGIC state.
>>>>
>>>> As you rightfully did, the do_fixup_vgic_errata should be called 
>>>> before syncing the LRs. However, even if you return early here, you 
>>>> will still execute leave_hypervisor_tail(). This mean that pending 
>>>> softirqs will be processed and potentially the vCPU rescheduled. 
>>>> Because the LRs were not synced (enter_hypervisor_head()) was not 
>>>> called, then the vGIC state will not out-of-date and would lead to 
>>>> all sort of potential issues.
>>>>
>>>> As the vGIC errata implies trapping the register such as IAR1 
>>>> (reading interrupt), we want to get a fastpath for it (e.g not 
>>>> trying to execute softirq...). So I think we should bypass 
>>>> leave_hypervisor_tail(). I am not entirely sure how to do it nicely 
>>>> thought.
>>>>
>>>
>>> How about adding a check for group1_trap enable in 
>>> leave_hypervisor_tail().
>>>
>>> void leave_hypervisor_tail(void)
>>> {
>>> +if (group1_trap)
>>> +   return;
>>
>> I guess you mean the variable you introduced in patch #10. In that 
>> case, this would be totally wrong. You only want to skip 
>> leave_hypervisor_tail() when you are handling a ICV_* System registers.
>>
>> What you want is adding a bool in the structure cpu_info to tell 
>> whether leave_hypervisor_tail() should be skipped.
>>
> Why would it be wrong? I don't follow our point.

As I mentioned in one of my previous e-mail, leave_hypervisor_tail() 
will process pending softirq and write/update the LRs based on the 
internal vGIC state. Softirq are used for scheduling, handling timer,...

So unless you want to make Xen unusable on Thunder-X, you really don't 
want to bypass leave_hypervisor_tail() in all the cases. Hence the 
suggest of a flag in cpu_info to provide a fastpath in certain cases.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel