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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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.

> +    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?

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

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