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

Cheers
Bertrand

> +
> +        ret = gicv3_lpi_disable_lpis(rdist_base);
> +        if ( ret == -EBUSY )
> +        {
> +            printk(XENLOG_ERR
> +                   "GICv3: CPU%u: LPIs still enabled with unexpected 
> redistributor tables\n",
> +                   smp_processor_id());
> +            return -EINVAL;
> +        }
> +        if ( ret )
> +            return ret;
> +    }
> 
>     ret = gicv3_lpi_set_pendtable(rdist_base);
>     if ( ret )
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index acdac22953..b16888ad84 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -275,7 +275,7 @@ static void gicv3_enable_sre(void)
> }
> 
> /* Wait for completion of a distributor/redistributor change */
> -static void gicv3_do_wait_for_rwp(void __iomem *base, uint32_t rwp_bit)
> +int gicv3_do_wait_for_rwp(void __iomem *base, uint32_t rwp_bit)
> {
>     uint32_t val;
>     bool timeout = false;
> @@ -299,17 +299,22 @@ static void gicv3_do_wait_for_rwp(void __iomem *base, 
> uint32_t rwp_bit)
>     } while ( 1 );
> 
>     if ( timeout )
> +    {
>         dprintk(XENLOG_ERR, "RWP timeout\n");
> +        return -ETIMEDOUT;
> +    }
> +
> +    return 0;
> }
> 
> static void gicv3_dist_wait_for_rwp(void)
> {
> -    gicv3_do_wait_for_rwp(GICD, GICD_CTLR_RWP);
> +    (void)gicv3_do_wait_for_rwp(GICD, GICD_CTLR_RWP);
> }
> 
> static void gicv3_redist_wait_for_rwp(void)
> {
> -    gicv3_do_wait_for_rwp(GICD_RDIST_BASE, GICR_CTLR_RWP);
> +    (void)gicv3_do_wait_for_rwp(GICD_RDIST_BASE, GICR_CTLR_RWP);
> }
> 
> static void gicv3_wait_for_rwp(int irq)
> @@ -863,7 +868,7 @@ static bool gicv3_enable_lpis(void)
>     return true;
> }
> 
> -static int __init gicv3_populate_rdist(void)
> +static int gicv3_populate_rdist(void)
> {
>     int i;
>     uint32_t aff;
> @@ -931,7 +936,7 @@ static int __init gicv3_populate_rdist(void)
>                     gicv3_set_redist_address(rdist_addr, procnum);
> 
>                     ret = gicv3_lpi_init_rdist(ptr);
> -                    if ( ret && ret != -ENODEV )
> +                    if ( ret && ret != -ENODEV && ret != -EBUSY )
>                     {
>                         printk("GICv3: CPU%d: Cannot initialize LPIs: %u\n",
>                                smp_processor_id(), ret);
> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> index 29bb9a89a4..68003ab116 100644
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -301,6 +301,10 @@ extern int gicv_setup(struct domain *d);
> extern void gic_save_state(struct vcpu *v);
> extern void gic_restore_state(struct vcpu *v);
> 
> +#ifdef CONFIG_GICV3
> +int gicv3_do_wait_for_rwp(void __iomem *base, uint32_t rwp_bit);
> +#endif
> +
> #ifdef CONFIG_SYSTEM_SUSPEND
> /* Suspend/resume */
> extern int gic_suspend(void);
> -- 
> 2.43.0
>