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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykola,

> On 25 Sep 2026, at 01:22, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> 
> 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.

Then we should at least leave a comment to make sure that this is
handled in that serie.

> ---
> 
> 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.

Ok then this will be an extra patch in the next version of the serie.

Cheers
Bertrand

> 
> Best regards,
> Mykola