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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Michal,

Thank you for the review.

On Mon, Apr 13, 2026 at 12:28 PM Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>
>
>
> 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

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