Re: [PATCH v3 3/4] xen/arm: its: refactor ITS quirk matching

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Mykola,

Mykola Kvach <xakep.amatop@gmail.com> writes:

> On Wed, Sep 23, 2026 at 3:21 AM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>> Hi,
>>
>> Mykola Kvach <mykola_kvach@epam.com> writes:
>>
>> > ITS quirks are matched only by IIDR and mask fields stored in each table
>> > entry. That is too coarse when the same GIC IP block appears on several
>> > platforms but a workaround is valid only for some of them.
>> >
>> > Replace the fixed IIDR fields with a generic match(hw_its, data)
>> > callback and an opaque data pointer. Add an IIDR matcher as a reusable
>> > building block and use it from the R-Car Gen4 matcher after checking the
>> > Renesas machine compatibles.
>> >
>> > This intentionally narrows the R-Car Gen4 quirk. Previously every ITS
>> > with IIDR 0x0201743b matched. Now it matches only a DT-discovered ITS on
>> > an r8a779f0 or r8a779g0 machine. ACPI-discovered ITSes and the same IIDR
>> > on other platforms no longer match.
>> >
>> > Keep first-match semantics explicit. Assert that non-sentinel entries
>> > provide a matcher and that IIDR matching receives match data. Retain
>> > runtime guards so malformed entries cannot cause a NULL function call
>> > or data dereference in non-debug builds. Place the matcher data and
>> > table in init-only read-only sections.
>> >
>> > The matched entry still supplies separate ITS and LPI flags; this patch
>> > only changes how the entry is selected.
>> >
>> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>> > ---
>> > Changes in v3:
>> > - Document the intentional narrowing of the R-Car Gen4 match.
>> > - Add the non-debug NULL-data guard.
>> > - Put the matcher data and table in init-only read-only sections.
>> >
>> > Changes in v2:
>> > - Replace v1's optional platform callback plus fixed IIDR/mask fields
>> >   with a single generic match(hw_its, data) selector.
>> > - Add a reusable IIDR matcher and use it after the R-Car Gen4
>> >   machine-compatible checks.
>> > - Document that the R-Car Gen4 quirk remains DT-only.
>> > - Keep the split ITS and host LPI quirk scopes when applying the matched
>> >   entry.
>> > - Document first-match ordering in the lookup path and guard against
>> >   entries without a match callback or IIDR match data.
>> > ---
>> >  xen/arch/arm/gic-v3-its.c | 73 +++++++++++++++++++++++++++++++--------
>> >  1 file changed, 58 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> > index 454fd0e0ef..f52232ca40 100644
>> > --- a/xen/arch/arm/gic-v3-its.c
>> > +++ b/xen/arch/arm/gic-v3-its.c
>> > @@ -54,8 +54,8 @@ struct its_device {
>> >
>> >  struct its_quirk {
>> >      const char *desc;
>> > -    uint32_t iidr;
>> > -    uint32_t mask;
>> > +    bool (*match)(const struct host_its *hw_its, const void *data);
>> > +    const void *data;
>> >      uint32_t its_flags;
>> >      /*
>> >       * lpi_flags are ORed into the global host LPI policy and must only
>> > @@ -65,11 +65,52 @@ struct its_quirk {
>> >      uint32_t lpi_flags;
>> >  };
>> >
>> > -static const struct its_quirk its_quirks[] = {
>> > +struct its_quirk_match_iidr {
>> > +    uint32_t iidr;
>> > +    uint32_t mask;
>> > +};
>> > +
>> > +static bool __init gicv3_its_match_iidr(const struct host_its *hw_its,
>> > +                                        const void *data)
>> > +{
>> > +    const struct its_quirk_match_iidr *match;
>> > +    uint32_t iidr;
>> > +
>> > +    ASSERT(data);
>> > +
>> > +    if ( !data )
>> > +        return false;
>>
>> It looks strange to have both ASSERT and then this check.
>>
>> Maybe it is worth to remove ASSERT and print a warning in a debug build instead?
>
> The ASSERT catches a programming error in the static quirk table in
> debug builds, while the explicit NULL check prevents a dereference
> when assertions are disabled.
>
> However, returning false skips a potentially required workaround,
> which could cause a less obvious failure later. This also raises the
> question of whether continuing is appropriate in non-debug builds.
>
> Would you prefer the debug warning you suggested, keeping ASSERT
> with an additional warning in non-debug builds, or using BUG_ON(!data)
> to stop immediately in all builds?

Thinking about this... Yes, I believe that should be BUG_ON()

>
>>
>> > +
>> > +    match = data;
>> > +    iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
>> > +
>> > +    return (iidr & match->mask) == match->iidr;
>> > +}
>> > +
>> > +static bool __init gicv3_its_match_quirk_gen4(const struct host_its *hw_its,
>> > +                                              const void *data)
>> > +{
>> > +    if ( !hw_its->dt_node )
>> > +        return false;
>> > +
>> > +    if ( !dt_machine_is_compatible("renesas,r8a779f0") &&
>> > +         !dt_machine_is_compatible("renesas,r8a779g0") )
>> > +        return false;
>> > +
>> > +    return gicv3_its_match_iidr(hw_its, data);
>>
>> Do you really need to match IIDR in this case? You already know that
>> platform requires the quirk.
>
> I would prefer to retain the IIDR check. The machine compatible does
> not identify the exact ITS variant and revision. Keeping both checks
> preserves the existing IIDR restriction while narrowing the match to
> the affected platforms.
>
> Linux also uses both checks for the R-Car Gen4 DMA32 workaround:
> the quirk entry matches IIDR 0x0201743b with mask 0xffffffff, and
> its_enable_dma32() additionally checks the machine compatible.

Okay then.

[...]

-- 
WBR, Volodymyr