Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts

[Peng Fan <peng.fan@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> Sent: 2019年1月22日 18:55
> To: Peng Fan <peng.fan@xxxxxxx>; sstabellini@xxxxxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andre Przywara
> <andre.przywara@xxxxxxx>
> Subject: Re: [PATCH] arm: gic-v3: clear GICR active interrupts
> 
> Hi Peng,
> 
> The commit title is a bit confusing. It suggests that all interrupts should be
> deactivated at boot, however you are only deactivating the SGIs.

"arm: gic-v3: deactivate SGI interrupts" seems better.

> 
> On 1/22/19 2:35 AM, Peng Fan wrote:
> > On i.MX8, we implemented partition reboot which means Cortex-A reboot
> > will not impact M4 cores and System control Unit core. However GICv3
> > is not reset because hardware design.
> 
> What do you mean by hardware design? Is it a defect?

Sorry. This is a wrong information. It is not defect, it is required that we 
not reset GIC
when implementing partition reboot. We support Cluster reboot without affecting
the other Cluster, so gic is not reset.

> 
> >
> > The gic-v3 controller is configured with EOImode to 1, so during xen
> > reboot, GIC_SGI_CALL_FUNCTION interrupt from CPU0 to other CPUs, but
> > stop_cpu never return, that means other CPUs have no chance to
> > deactive the interrupt. During xen booting again, CPU0 will issue
> > GIC_SGI_CALL_FUNCTION to other CPUs. Because
> GIC_SGI_CALL_FUNCTION of
> > other CPUs are active during the last reboot, interrupts could not be
> > triggered unless we deactive the interrupt first.
> 
>  From the description here, I think it not very sane to go to sleep with an
> interrupt activate.
> 
> A better solution would be to move the deactivation earlier on in do_sgi
> (maybe after eoi_irq) so we call stop_cpu() with all interrupts disabled.

Agree.

> 
> >
> > So let's deactive the interrupts during GICv3 initialization to fix
> 
> s/deactivate/

Fix in V2.

> 
> > this issue.
> 
> Similarly to the commit title, you wrote the commit message very generically.

Are you ok with the following patch?

    arm: gic: deactivate sgi immediately after eoi

    On i.MX8, we implemented partition reboot which means Cortex-A reboot
    will not impact M4 cores and System control Unit core. However GICv3
    is not reset because we also need to support A72 Cluster reboot without
    affecting A53 Cluster.

    The gic-v3 controller is configured with EOImode to 1, so during xen
    reboot, there is a function call "smp_call_function(halt_this_cpu, NULL, 
0);"
    ,but halt_this_cpu never return, that means other CPUs have no chance to
    deactive the SGI interrupt, because the deactivate_irq operation is at
    the end of do_sgi. During xen booting again, CPU0 will issue
    GIC_SGI_CALL_FUNCTION to other CPUs. Because GIC_SGI_CALL_FUNCTION of
    other CPUs are active during the last reboot, interrupts could not be
    triggered unless we deactivate the interrupt first.

    To fix this issue, let's move the deactivate_irq operation just after
    eoi_irq, then the SGI interrupt will be in deactive state when
    smp_call_function_interrupt.

    Signed-off-by: Peng Fan <peng.fan@xxxxxxx>

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec706..300fdbd9ae 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -347,6 +347,8 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi 
sgi)

     /* Lower the priority */
     gic_hw_ops->eoi_irq(desc);
+    /* Deactivate */
+    gic_hw_ops->deactivate_irq(desc);

     /*
      * Ensure any shared data written by the CPU sending
@@ -370,9 +372,6 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi 
sgi)
         panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id());
         break;
     }
-
-    /* Deactivate */
-    gic_hw_ops->deactivate_irq(desc);
 }

Thanks,
Peng.
> 
> 
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >   xen/arch/arm/gic-v3.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index 6fbc106757..643d4a33f0 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -824,8 +824,12 @@ static int gicv3_cpu_init(void)
> >       priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI <<
> 8 |
> >                   GIC_PRI_IPI);
> >       for (i = 0; i < NR_GIC_SGI; i += 4)
> > +    {
> >           writel_relaxed(priority,
> >                   GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4)
> * 4);
> > +        writel_relaxed(0xffffffff,
> > +                GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i / 4) *
> 4);
> > +    }
> >
> >       priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 | GIC_PRI_IRQ
> << 8 |
> >                   GIC_PRI_IRQ);
> >
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel