Re: [PATCH v2] arm/gicv3: Decode cacheability fields before comparing

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 10/04/2026 19:34, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> 
> GITS_BASER_INNER_CACHEABILITY_MASK and
> GICR_PROPBASER_INNER_CACHEABILITY_MASK are shifted masks. Comparing the
> masked but unshifted values against GIC_BASER_CACHE_nC, which is an
> unshifted enum value, leads to incorrect detection of non-cacheable
> GITS_CBASER command queue, GITS_BASER tables, and GICR_PROPBASER
> mappings.
> 
> Use MASK_EXTR() to decode these cacheability fields before comparing
> against GIC_BASER_CACHE_nC, so the backing memory is flushed when
> required.
> 
> Fixes: 8ed8d21373be ("ARM: GICv3 ITS: map ITS command buffer")
> Fixes: 05238012b86d ("ARM: GICv3 ITS: allocate device and collection table")
> Fixes: c9b939863c89 ("ARM: GICv3: allocate LPI pending and property table")
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
> Changes in v2:
> - use MASK_EXTR() instead of open-coding the BASER field shift
> - fix the analogous PROPBASER cacheability comparison in
>   gicv3_lpi_set_proptable()
> - fix the CBASER command queue cacheability check as well
> ---
>  xen/arch/arm/gic-v3-its.c | 6 ++++--
>  xen/arch/arm/gic-v3-lpi.c | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 9ba068c46f..e87465d2ff 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -424,7 +424,8 @@ static void *its_map_cbaser(struct host_its *its)
>       * If the command queue memory is mapped as uncached, we need to flush
>       * it on every access.
>       */
> -    if ( !(reg & GITS_BASER_INNER_CACHEABILITY_MASK) )
You don't seem to mention this change. This one does not compare to
GIC_BASER_CACHE_nC and checks against 0, which means we are on the safe side. If
you still want to change it, then you should also look few lines above where we
have:
if ( (reg & GITS_BASER_SHAREABILITY_MASK) == 0 )

> +    if ( MASK_EXTR(reg, GITS_BASER_INNER_CACHEABILITY_MASK) <=
> +         GIC_BASER_CACHE_nC )
This is a functional change. Previously we where comparing against 0 and now you
compare against <= 1

~Michal

>      {
>          its->flags |= HOST_ITS_FLUSH_CMD_QUEUE;
>          printk(XENLOG_WARNING "using non-cacheable ITS command queue\n");
> @@ -496,7 +497,8 @@ retry:
>          }
>          attr = regc & BASER_ATTR_MASK;
>      }
> -    if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC )
> +    if ( MASK_EXTR(regc, GITS_BASER_INNER_CACHEABILITY_MASK) <=
> +         GIC_BASER_CACHE_nC )
>          clean_and_invalidate_dcache_va_range(buffer, table_size);
>  
>      /* If the host accepted our page size, we are done. */
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index de5052e5cf..9ee338edc2 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -351,7 +351,8 @@ static int gicv3_lpi_set_proptable(void __iomem * 
> rdist_base)
>      }
>  
>      /* Remember that we have to flush the property table if non-cacheable. */
> -    if ( (reg & GICR_PROPBASER_INNER_CACHEABILITY_MASK) <= 
> GIC_BASER_CACHE_nC )
> +    if ( MASK_EXTR(reg, GICR_PROPBASER_INNER_CACHEABILITY_MASK) <=
> +         GIC_BASER_CACHE_nC )
>      {
>          lpi_data.flags |= LPI_PROPTABLE_NEEDS_FLUSHING;
>          /* Update the redistributors knowledge about the attributes. */