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月30日 20:06
> 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
> 
> Replying to this thread again.
> 
> On 22/01/2019 10:54, Julien Grall wrote:
> > 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.
> >
> > 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?
> >
> >>
> >> 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.
> 
> It looks like I was wrong here. There are case where you can't avoid that (see
> my answer to your other patch) and the boot code cannot rely on the activate
> state of interrupt. So they have to be cleaned at boot.
> 
> This also have to be done for PPI and SPIs. Peng, would you mind to do that
> patch?

I'll send out v2 patch soon.

> 
> >
> > 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.
> >
> >>
> >> So let's deactive the interrupts during GICv3 initialization to fix
> >
> > s/deactivate/
> >
> >> this issue.
> >
> > Similarly to the commit title, you wrote the commit message very
> generically.
> >
> >
> >>
> >> 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);
> 
> Each ICACTIVER0 registers hold the active bit for 32 interrupts. However, this
> code assumes the register only hold 4 interrupts. So this will write to
> unwanted area.
> 
> There are only 16 SGIs, so it fits in one write to ICACTIVER0. As wrote above,
> you also need to deactivate the PPIs. So the following should be enough:
> 
> /*
>   * The activate state is unknown at boot, so make sure all SGIs and PPIs are
>   * de-activated.
>   */
> writel_relaxed(0xffffffff, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0);

Thanks,
Peng.

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel