Re: [PATCH] arm/its: Decode BASER cacheability field before comparing

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Fri, Apr 10, 2026 at 1:45 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 10.04.2026 09:08, Mykola Kvach wrote:
> > On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 10.04.2026 08:09, Mykola Kvach wrote:
> >>> --- a/xen/arch/arm/gic-v3-its.c
> >>> +++ b/xen/arch/arm/gic-v3-its.c
> >>> @@ -496,7 +496,8 @@ retry:
> >>>          }
> >>>          attr = regc & BASER_ATTR_MASK;
> >>>      }
> >>> -    if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= 
> >>> GIC_BASER_CACHE_nC )
> >>> +    if ( ((regc & GITS_BASER_INNER_CACHEABILITY_MASK) >>
> >>> +          GITS_BASER_INNER_CACHEABILITY_SHIFT) <= GIC_BASER_CACHE_nC )
> >>
> >> Are you aware of MASK_EXTR()? This is one of the cases that we have it for.
> >> Really all *_SHIFT constants should be purged, as they can be calculated
> >> from their *_MASK counterparts (leveraging MASK_{EXTR,INSR}() to keep the
> >> code readable).
> >
> > I wasn't aware of this macro, thanks. I will take a look.
> >
> >> Further, doesn't gicv3_lpi_set_proptable() have the same issue with
> >> GICR_PROPBASER_INNER_CACHEABILITY_MASK?
> >
> > Fortunately, GIC_BASER_NonShareable is equal to zero, so the condition
> > there is not affected.
>
> I fear I don't follow. In
>
>     if ( (reg & GICR_PROPBASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC 
> )
>
> where does GIC_BASER_NonShareable come into play?

Sorry, I missed that part of your comment; clearly, I haven't had enough
coffee yet.

You are right: gicv3_lpi_set_proptable() has the exact same issue and
needs fixing too. Thanks for catching that.

It is interesting that we did not hit this during GICv4 testing.


Best regards,
Mykola

>
> Jan