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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel