1. Patchew
  2. Xen
  3. View series

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

Mykola Kvach posted 1 patch 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/7c20230fcd388ebe398ce19a21a1cd3c2c73d5fa.1775839987.git.mykola._5Fkvach@epam.com
There is a newer version of this series
xen/arch/arm/gic-v3-its.c | 6 ++++--
xen/arch/arm/gic-v3-lpi.c | 3 ++-
2 files changed, 6 insertions(+), 3 deletions(-)
[PATCH v2] arm/gicv3: Decode cacheability fields before comparing
Posted by Mykola Kvach 3 months ago 
From: Mykola Kvach <mykola_kvach@epam.com>

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@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
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) )
+    if ( MASK_EXTR(reg, GITS_BASER_INNER_CACHEABILITY_MASK) <=
+         GIC_BASER_CACHE_nC )
     {
         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. */
-- 
2.43.0
Re: [PATCH v2] arm/gicv3: Decode cacheability fields before comparing
Posted by Orzel, Michal 3 months ago 

On 10/04/2026 19:34, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@epam.com>
> 
> 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@epam.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> 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. */
Re: [PATCH v2] arm/gicv3: Decode cacheability fields before comparing
Posted by Mykola Kvach 2 months, 2 weeks ago 
Hi Michal,

Thank you for the review.

On Mon, Apr 13, 2026 at 12:28 PM Orzel, Michal <michal.orzel@amd.com> wrote:
>
>
>
> On 10/04/2026 19:34, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > 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@epam.com>
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > 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

You are right, the CBASER hunk is not fixing the same shifted-mask
comparison bug as the BASER/PROPBASER hunks.

My intention was to also handle InnerCache == 0b001, which is Normal
Inner Non-cacheable, so the command queue should need flushing in that
case as well. I agree this is a separate functional change.

I will drop the CBASER hunk from this patch to keep it focused on the
BASER/PROPBASER shifted-mask fix. I will send the CBASER change
separately with a dedicated explanation.

Best regards,
Mykola

>
> ~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. */
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.