Re: [PATCH v8 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, Apr 22, 2026 at 6:57 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > +
> > +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);
>
> I’m looking into the implementation of gicv3_do_wait_for_rwp() and I see
> it’s polling on bit 31 (UWP) instead of bit 3 (RWP)?
>
> Not related to this patch but I feel we need to raise this.

Good catch, thanks.

UWP does have SGI-related semantics, but it is not the same as redistributor
RWP. The existing helper is used as an RWP wait helper after redistributor
register writes, so the redistributor path should poll GICR_CTLR.RWP rather
than GICR_CTLR.UWP.

I will send a separate prerequisite patch to make the redistributor
path use 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.
> > @@ -373,7 +434,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;
> > +
> > +        ret = gicv3_lpi_disable_lpis(rdist_base);
> > +        if ( ret == -EBUSY )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "GICv3: CPU%d: 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 bc07f97c16..34fb065afc 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -274,8 +274,8 @@ static void gicv3_enable_sre(void)
> >     isb();
> > }
> >
> > -/* Wait for completion of a distributor change */
> > -static void gicv3_do_wait_for_rwp(void __iomem *base)
> > +/* Wait for completion of a distributor/redistributor write-pending 
> > change. */
> > +int gicv3_do_wait_for_rwp(void __iomem *base)
> > {
> >     uint32_t val;
> >     bool timeout = false;
> > @@ -295,17 +295,22 @@ static void gicv3_do_wait_for_rwp(void __iomem *base)
> >     } 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);
> > +    (void)gicv3_do_wait_for_rwp(GICD);
> > }
> >
> > static void gicv3_redist_wait_for_rwp(void)
> > {
> > -    gicv3_do_wait_for_rwp(GICD_RDIST_BASE);
> > +    (void)gicv3_do_wait_for_rwp(GICD_RDIST_BASE);
> > }
> >
> > static void gicv3_wait_for_rwp(int irq)
> > @@ -925,7 +930,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”,
>
> This should be the other way around? %u for smp_processor_id() and %d for ret?

You're right, thanks. I will fix the format string.

>
> >                                smp_processor_id(), ret);
> > diff --git a/xen/arch/arm/include/asm/gic_v3_its.h 
> > b/xen/arch/arm/include/asm/gic_v3_its.h
> > index fc5a84892c..081bd19180 100644
> > --- a/xen/arch/arm/include/asm/gic_v3_its.h
> > +++ b/xen/arch/arm/include/asm/gic_v3_its.h
>
> Why this header and not gic.h?

You're right, this prototype is not ITS-specific. I will move it to gic.h.

Best regards,
Mykola

>
> > @@ -133,6 +133,7 @@ struct host_its {
> >
> > /* Map a collection for this host CPU to each host ITS. */
> > int gicv3_its_setup_collection(unsigned int cpu);
> > +int gicv3_do_wait_for_rwp(void __iomem *base);
> >
> > #ifdef CONFIG_HAS_ITS
> >
> >
>
> The rest looks ok to me!
>
> Cheers,
> Luca
>
>
>