arm/its: Decode BASER cacheability field before comparing

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

Posted by Mykola Kvach 2 days, 14 hours ago

From: Mykola Kvach <mykola_kvach@epam.com>

GITS_BASER_INNER_CACHEABILITY_MASK is a shifted mask. Comparing the
masked but unshifted value against GIC_BASER_CACHE_nC, which is an
unshifted value, leads to incorrect detection of non-cacheable
table mappings.

Shift the masked value to properly detect if the BASER backing memory
requires flushing.

Fixes: 05238012b86 ("ARM: GICv3 ITS: allocate device and collection table")
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/arch/arm/gic-v3-its.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 9ba068c46f..6a46bcc8af 100644
--- 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 )
         clean_and_invalidate_dcache_va_range(buffer, table_size);
 
     /* If the host accepted our page size, we are done. */
-- 
2.43.0

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

Posted by Jan Beulich 2 days, 14 hours ago

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).

Further, doesn't gicv3_lpi_set_proptable() have the same issue with
GICR_PROPBASER_INNER_CACHEABILITY_MASK?

Jan

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

Posted by Mykola Kvach 2 days, 9 hours ago

On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@suse.com> 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).
>
> Further, doesn't gicv3_lpi_set_proptable() have the same issue with
> GICR_PROPBASER_INNER_CACHEABILITY_MASK?

Is it acceptable to include Fixes tags for two different commits in
a single patch, or would it be better to split it? Both issues are
logically identical, just in different functions.


Best regards,
Mykola

>
> Jan

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

Posted by Andrew Cooper 2 days, 9 hours ago

On 10/04/2026 12:30 pm, Mykola Kvach wrote:
> On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@suse.com> 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).
>>
>> Further, doesn't gicv3_lpi_set_proptable() have the same issue with
>> GICR_PROPBASER_INNER_CACHEABILITY_MASK?
> Is it acceptable to include Fixes tags for two different commits in
> a single patch, or would it be better to split it? Both issues are
> logically identical, just in different functions.

Multiple fixes tags is entirely fine, and we have several examples in
fight even now.

I'd absolutely recommend one patch with two fixes tags in this case.

~Andrew

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

Posted by Mykola Kvach 2 days, 9 hours ago

Hi Andrew,

Thank you for the review.

On Fri, Apr 10, 2026 at 2:33 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 10/04/2026 12:30 pm, Mykola Kvach wrote:
> > On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@suse.com> 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).
> >>
> >> Further, doesn't gicv3_lpi_set_proptable() have the same issue with
> >> GICR_PROPBASER_INNER_CACHEABILITY_MASK?
> > Is it acceptable to include Fixes tags for two different commits in
> > a single patch, or would it be better to split it? Both issues are
> > logically identical, just in different functions.
>
> Multiple fixes tags is entirely fine, and we have several examples in
> fight even now.
>
> I'd absolutely recommend one patch with two fixes tags in this case.

Understood, thanks.


Best regards,
Mykola

>
> ~Andrew

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

Posted by Mykola Kvach 2 days, 13 hours ago

Hi Jan,

Thank you for the review.

On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@suse.com> 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.

We may want to align that condition for consistency, but I would prefer
to keep this patch focused on the actual bug fix and avoid unrelated
changes.


Best regards,
Mykola

>
> Jan

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

Posted by Jan Beulich 2 days, 10 hours ago

On 10.04.2026 09:08, Mykola Kvach wrote:
> On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@suse.com> 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?

Jan

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

Posted by Mykola Kvach 2 days, 10 hours ago

On Fri, Apr 10, 2026 at 1:45 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.04.2026 09:08, Mykola Kvach wrote:
> > On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@suse.com> 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

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

Posted by Andrew Cooper 2 days, 11 hours ago

On 10/04/2026 8:08 am, Mykola Kvach wrote:
> Hi Jan,
>
> Thank you for the review.
>
> On Fri, Apr 10, 2026 at 9:40 AM Jan Beulich <jbeulich@suse.com> 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.

As a general rule of thumb, where you've got a _MASK/_SHIFT pair, you
should be using MASK_INSR/EXTR and delete the shift constant.

It is is ~half the code (visually), and removes an entire class of
errors (mismatched constants).

~Andrew