Re: [PATCH v9 03/13] xen/arm: gic-v3: tolerate retained redistributor LPI state across CPU_OFF

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the review.

On Wed, May 13, 2026 at 5:53 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 12 May 2026, at 18:07, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> >
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > PSCI does not guarantee that a GICv3 redistributor is powered down across
> > CPU_OFF -> CPU_ON.
> >
> > DEN0022F.b says CPU_OFF powers down the calling core (5.5) and CPU_ON
> > brings the core back with a defined initial CPU state (5.6, 6.4).
> > However, PSCI leaves interrupt migration and GIC re-initialization to the
> > supervisory software/firmware stack: the caller must migrate interrupts
> > away before CPU_OFF (5.5.2), and the execution context that is lost in a
> > powerdown state must be saved and restored by software (6.8). PSCI also
> > calls out GIC management explicitly in 6.8, including retargeting SPIs,
> > preventing PPIs/SGIs from targeting a powered down CPU, and reinitializing
> > the CPU interface after CPU_ON.
> >
> > This matches the GIC architecture. IHI0069H.b Chapter 11.1 requires the PE
> > and CPU interface to share a power domain, but explicitly allows the
> > associated redistributor, distributor, and ITS to remain powered while the
> > PE and CPU interface are off. All other GIC power-management behavior is
> > IMPLEMENTATION DEFINED. DEN0050D Chapter 4.2, "Generic Interrupt
> > Controller (GIC)", says the GICv3 redistributor may live either in the AP
> > core power domain or in a relatively always-on parent domain. So after
> > CPU_OFF -> CPU_ON a secondary CPU can legitimately come back to a live
> > redistributor with GICR_CTLR.EnableLPIs still set.
> >
> > Handle that case in the LPI setup path instead of assuming a fully reset
> > redistributor.
> >
> > The LPI path needs special care because the GIC spec makes redistributor
> > LPI state sticky and partially implementation defined. IHI0069H.b 5.1.1
> > and 5.1.2 say that changing GICR_PROPBASER or GICR_PENDBASER while
> > GICR_CTLR.EnableLPIs == 1 is UNPREDICTABLE. After clearing EnableLPIs,
> > software must wait for GICR_CTLR.RWP == 0 before touching the pending
> > table. The architecture also permits implementations where, once
> > EnableLPIs has been set, clearing it again is not guaranteed to work.
> > Where an ITS is present, the spec strongly recommends moving LPIs to
> > another redistributor before clearing EnableLPIs.
> >
> > Because of that, treat a retained EnableLPIs state as valid when the
> > redistributor still points at Xen's expected PROPBASER/PENDBASER tables.
> > Only try to clear EnableLPIs when the retained configuration does not
> > match Xen's state, and wait for RWP before reprogramming the tables.
> >
> > This is also consistent with platform firmware reality: PSCI and the GIC
> > architecture allow platform-specific redistributor power handling, and not
> > all platform firmware implementations force a full redistributor power-off
> > through implementation-defined controls during CPU_OFF. Xen therefore needs
> > to tolerate retained redistributor state on secondary CPU bring-up.
> >
> > Keep gicv3_populate_rdist() resident as well, because gicv3_cpu_init()
> > reuses it on secondary CPU bring-up after init.
> >
> > Tested using Xen's non-boot CPU disable/enable path on Arm
> > FVP_Base_RevC-2xAEMvA, both with and without:
> > -C gic_distributor.allow-LPIEN-clear=1
> > -C gic_distributor.GICR-clear-enable-supported=1
> > and on Orange Pi 5.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
>
> I understand you will send a separate patch to fix RWP.
>
> Looks ok to me, I see you’ve touched some printk in gicv3_populate_rdist() 
> which
> were wrongly having %d for smp_processor_id(), sometimes maintainers are not
> really ok with that when the changes are not related to the commit, but apart 
> from that:

That change was intentional.

In the previous version of this series, you pointed out the
wrong format specifier for smp_processor_id() in one of these
printk()s.

Although it was not directly related to the functional change in
this patch, I decided to fix the other similar occurrences in
gicv3_populate_rdist() as well, for consistency.

>
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>
> Cheers,
> Luca
>
>

Best regards,
Mykola