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

[Peng Fan <peng.fan@xxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of Peng Fan
> Sent: 2019年1月30日 21:24
> To: Julien Grall <julien.grall@xxxxxxx>; sstabellini@xxxxxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andre Przywara
> <andre.przywara@xxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] arm: gic-v3: clear GICR active interrupts
> 
> 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);

Just have a following up question, according to 
IHI0069D_gic_architecture_specification
"8.11.3 GICR_ICACTIVER0, Interrupt Clear-Active Register 0 "
This field resets to a value that is architecturally UNKNOWN,
Do we need to take SPI into consideration as the following in gic_cpu_init?
for (i = 0; i < nr_lines; i +=32)
        writel_relaxed(0xffffffff, GICD_RDIST_SGI_BASE + GICR_ICACTIVER0 + (i / 
32) * 4);

and move nr_lines out from gic_dist_init to a static global varaiable.

Thanks,
Peng.

> 
> Thanks,
> Peng.
> 
> >
> > Cheers,
> >
> > --
> > Julien Grall
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> xenproject.org%2Fmailman%2Flistinfo%2Fxen-devel&amp;data=02%7C01%7
> Cpeng.fan%40nxp.com%7Cba187bcf60464fda80f308d686b63c4a%7C686ea1
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636844514593523457&amp;s
> data=2opQQEkJAqAon3amajrlQqGvKuZqiUbJp2BBRYrEzfQ%3D&amp;reserve
> d=0
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel