Re: [Xen-devel] [PATCH for-4.12 v2] arm: gic-v3: deactivate SGI/PPI during initialization

[Peng Fan <peng.fan@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> Sent: 2019年2月3日 0:04
> To: Peng Fan <peng.fan@xxxxxxx>; sstabellini@xxxxxxxxxx; jgross@xxxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH for-4.12 v2] arm: gic-v3: deactivate SGI/PPI during
> initialization
> 
> 
> 
> On 2/2/19 12:54 PM, Peng Fan wrote:
> > Hi Julien
> 
> Hi Peng,
> 
> >> -----Original Message-----
> >> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> >> Sent: 2019年2月1日 18:41
> >> To: Peng Fan <peng.fan@xxxxxxx>; sstabellini@xxxxxxxxxx;
> >> jgross@xxxxxxxx
> >> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH for-4.12 v2] arm: gic-v3: deactivate SGI/PPI
> >> during initialization
> >>
> >> Hi,
> >>
> >> We spoke about SPIs in the previous version. Why aren't they
> >> de-activated here?
> >
> > According to GIC-V3 " 8.9.5 GICD_ICACTIVER<n>, Interrupt Clear-Active
> Registers, n = 0 - 31"
> > "
> > Some or all RW fields of this register have defined reset values.
> > When this register has an architecturally-defined reset value, this field
> resets to 0.
> > "
> 
> I can't find this wording in the last spec (IHI0069E). Can you please give the
> version of the specific version of the spec when quoting it?

The spec I use is IHI0069D_gic_architecture_specification. Will add the version 
in
future patches when needed.

> 
> >
> > So I think we no need to deactivated, because it has reset values 0
> 
> The specification is about the reset value from the hardware, it does not tell
> you how the firmware (or the previous kernel when using kexec) is going to
> leave the interrupts. For instance, as I pointed out in a different thread, 
> Xen
> may reboot with SPIs activated. This can happen if you receive the
> reboot/power off request while handling an SPI.

Thanks, just sent out v3 including the deactivation of SPI.

> 
> >
> >>
> >> On 1/30/19 2:00 PM, 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 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 deactivate the SGI interrupt, because the deactivate_irq
> >>> operation is at the end of do_sgi. During the next boot of Xen, CPU0
> >>> will issue GIC_SGI_CALL_FUNCTION to other CPUs. As the Active state
> >>> for SGI is left untouched during the reboot, the
> >>> GIC_SGI_CALL_FUNCTION will still be active on the non-boot CPUs.
> >>> This means the interrupt cannot be triggered again until it get
> deactivated.
> >>>
> >>> And according to IHI0069D_gic_architecture_specification, chapter
> >>> "8.11.3 GICR_ICACTIVER0, Interrupt Clear-Active Register 0", the RW
> >>> field of GICR_ICACTIVER0 resets to a value that is architecturally
> >> UNKNOWN.
> >>>
> >>> So set a fixed value during gic-v3 initialization to make sure
> >>> interrupts are in deactivated state.
> >>
> >> It is a bit unclear what you mean by "fixed value" here. The only
> >> thing you do is clearing active state. So a better wording is "So
> >> make sure all interrupts are deactivated at during initialization by 
> >> clearing
> the state".
> >>
> >> How about SPIs?
> >
> > Replied above. I could add the following to the patch if you think it
> > needed, I am not sure whether it is valid that SPI is in active state when 
> > Xen
> booting.
> 
> See above, I think this is valid and can happen today.
> 
> >
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index
> > 1c1d2604f3..5b9c5559a7 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -622,9 +622,12 @@ static void __init gicv3_dist_init(void)
> >           writel_relaxed(priority, GICD + GICD_IPRIORITYR + (i / 4) * 4);
> >       }
> >
> > -    /* Disable all global interrupts */
> > +    /* Disable/deactivate all global interrupts */
> >       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 32 )
> > +    {
> >           writel_relaxed(0xffffffff, GICD + GICD_ICENABLER + (i / 32)
> > * 4);
> > +        writel_relaxed(0xffffffff, GICD + GICD_ICACTIVER + (i / 32) * 4);
> > +    }
> >
> >       /*
> >        * Configure SPIs as non-secure Group-1. This will only matter
> 
> This chunk looks good. I will also write a patch for GICv2 doing the same.

Thanks,
Peng.

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