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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Bertrand,

Thank you for the review.

On Wed, Sep 23, 2026 at 6:36 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 27 Aug 2026, at 16:31, Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
> >
> > 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>
> > Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > ---
> > Changes in v10:
> > - Drop unrelated gicv3_populate_rdist() printk() format cleanups to keep
> >  the patch focused on retained redistributor LPI state.
> >
> > Changes in v9:
> > - move gicv3_do_wait_for_rwp prototype from its related header to gic.h
> > - drop __init from gicv3_populate_rdist(), which is reused on secondary
> >  CPU bring-up after boot
> > - changed print format for smp_processor_id in gicv3_populate_rdist func
> > - cosmetic changes
> > ---
> > xen/arch/arm/gic-v3-lpi.c      | 77 +++++++++++++++++++++++++++++++++-
> > xen/arch/arm/gic-v3.c          | 15 ++++---
> > xen/arch/arm/include/asm/gic.h |  4 ++
> > 3 files changed, 90 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> > index 9ee338edc2..847da26ff7 100644
> > --- a/xen/arch/arm/gic-v3-lpi.c
> > +++ b/xen/arch/arm/gic-v3-lpi.c
> > @@ -81,6 +81,13 @@ static DEFINE_PER_CPU(struct lpi_redist_data, 
> > lpi_redist);
> > #define MAX_NR_HOST_LPIS   (lpi_data.max_host_lpi_ids - LPI_OFFSET)
> > #define HOST_LPIS_PER_PAGE      (PAGE_SIZE / sizeof(union host_lpi))
> >
> > +#define GICR_PROPBASER_XEN_MASK  GENMASK_ULL(51, 12)
> > +/*
> > + * For retained redistributor state, match the pending table by address 
> > only.
> > + * Attribute bits such as PTZ may not read back with the programmed value.
> > + */
> > +#define GICR_PENDBASER_XEN_MASK  GENMASK_ULL(51, 16)
> > +
> > static union host_lpi *gic_get_host_lpi(uint32_t plpi)
> > {
> >     union host_lpi *block;
> > @@ -296,6 +303,60 @@ static int gicv3_lpi_set_pendtable(void __iomem 
> > *rdist_base)
> >     return 0;
> > }
> >
> > +static uint64_t gicv3_lpi_expected_proptable(void)
> > +{
> > +    return virt_to_maddr(lpi_data.lpi_property);
> > +}
> > +
> > +static uint64_t gicv3_lpi_expected_pendtable(void)
> > +{
> > +    return virt_to_maddr(this_cpu(lpi_redist).pending_table);
> > +}
> > +
> > +static bool gicv3_lpi_tables_match(void __iomem *rdist_base)
> > +{
> > +    uint64_t propbase, pendbase;
> > +
> > +    if ( !lpi_data.lpi_property || !this_cpu(lpi_redist).pending_table )
> > +        return false;
> > +
> > +    propbase = readq_relaxed(rdist_base + GICR_PROPBASER);
> > +    pendbase = readq_relaxed(rdist_base + GICR_PENDBASER);
> > +
> > +    return ((propbase & GICR_PROPBASER_XEN_MASK) ==
> > +            (gicv3_lpi_expected_proptable() & GICR_PROPBASER_XEN_MASK)) &&
> > +           ((pendbase & GICR_PENDBASER_XEN_MASK) ==
> > +            (gicv3_lpi_expected_pendtable() & GICR_PENDBASER_XEN_MASK));
> > +}
> > +
> > +static int gicv3_lpi_disable_lpis(void __iomem *rdist_base)
> > +{
> > +    uint32_t reg = readl_relaxed(rdist_base + GICR_CTLR);
> > +    int ret;
> > +
> > +    if ( !(reg & GICR_CTLR_ENABLE_LPIS) )
> > +        return 0;
> > +
> > +    writel_relaxed(reg & ~GICR_CTLR_ENABLE_LPIS, rdist_base + GICR_CTLR);
> > +
> > +    /*
> > +     * The spec only guarantees programmability when we have observed the 
> > bit
> > +     * cleared. Where clearing is supported, RWP must reach 0 before 
> > touching
> > +     * PROPBASER/PENDBASER again.
> > +     */
> > +    wmb();
> > +
> > +    ret = gicv3_do_wait_for_rwp(rdist_base, GICR_CTLR_RWP);
> > +    if ( ret )
> > +        return ret;
> > +
> > +    reg = readl_relaxed(rdist_base + GICR_CTLR);
> > +    if ( reg & GICR_CTLR_ENABLE_LPIS )
> > +        return -EBUSY;
> > +
> > +    return 0;
> > +}
> > +
> > /*
> >  * Tell a redistributor about the (shared) property table, allocating one
> >  * if not already done.
> > @@ -374,7 +435,21 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base)
> >     /* Make sure LPIs are disabled before setting up the tables. */
> >     reg = readl_relaxed(rdist_base + GICR_CTLR);
> >     if ( reg & GICR_CTLR_ENABLE_LPIS )
> > -        return -EBUSY;
> > +    {
> > +        if ( gicv3_lpi_tables_match(rdist_base) )
> > +            return -EBUSY;
>
> I am wondering if there is a corner case when a CPU is unplugged and then
> plugged back in. free_percpu_area() eventually frees the per-CPU area
> containing lpi_redist.pending_table, but not the table itself. On the next
> cpu_up(), gicv3_lpi_allocate_pendtable() allocates a new table, and I cannot
> find where the old one is freed.
>
> If the redistributor kept EnableLPIs=1 and GICR_PENDBASER pointing to the
> old table, wouldn't gicv3_lpi_tables_match() fail?
>
> What will happen if EnableLPIs cannot be cleared ? (i think this is something
> possible in the hardware).

The sequence you describe would need to be handled when adding
CPU hotplug support on Arm. There is currently no runtime caller
for such an offline/online cycle outside system suspend/resume.

During system suspend, the common code preserves the per-CPU area.
The following GICv3 suspend/resume patch also skips pending-table
allocation on resume, so the existing table and pointer are
reused.

The pending-table lifetime across normal CPU hotplug should be
handled by the CPU hotplug series.
---

While checking CPU bring-up failures during resume, I found a
separate issue in the common cleanup code. Both CPU_UP_CANCELED
and CPU_RESUME_FAILED can call free_percpu_area() for the same CPU
during resume. The release metadata, including the rcu_head, is
itself stored in that CPU's per-CPU area.

If the first release has completed, another call would access
invalid per-CPU state. Otherwise, it can queue the same rcu_head
again. The timer and CPU-pool callbacks on CPU_RESUME_FAILED also
still need the per-CPU area.

On x86, park_offline_cpus prevents this particular release path.

I plan to address this in a separate preparatory patch in this
series, keeping the per-CPU area available until the final
CPU_RESUME_FAILED cleanup and releasing it only once.

Best regards,
Mykola