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

[Julien Grall <julien.grall@xxxxxxx>]

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?

> 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);

Cheers,

--
Julien Grall

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