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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Wed, Sep 23, 2026 at 3:21 AM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi,
>
> Mykola Kvach <mykola_kvach@xxxxxxxx> 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@xxxxxxxx>
> > ---
> > 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?

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

>
> > +}
> > +
> > +static const struct its_quirk_match_iidr rcar_gen4_iidr __initconst = {
> > +    /* Implementer 0x43b identifies Arm Ltd. */
> > +    .iidr = 0x0201743b,
> > +    .mask = 0xffffffffU,
> > +};
> > +
> > +static const struct its_quirk its_quirks[] __initconstrel = {
> >      {
> > -        .desc        = "R-Car Gen4",
> > -        .iidr        = 0x0201743b,
> > -        .mask        = 0xffffffffU,
> > +        .desc = "R-Car Gen4",
> > +        .match = gicv3_its_match_quirk_gen4,
> > +        .data = &rcar_gen4_iidr,
> >          .its_flags = GICV3_QUIRK_MEM_NC_NS | GICV3_QUIRK_MEM_32BIT_ADDR,
> >          .lpi_flags = GICV3_QUIRK_MEM_NC_NS | GICV3_QUIRK_MEM_32BIT_ADDR,
> >      },
> > @@ -78,18 +119,21 @@ static const struct its_quirk its_quirks[] = {
> >      }
> >  };
> >
> > -static const struct its_quirk *__init gicv3_its_find_quirk(uint32_t iidr)
> > +static const struct its_quirk *__init gicv3_its_find_quirk(
> > +    const struct host_its *hw_its)
> >  {
> > -    const struct its_quirk *quirks = its_quirks;
> > +    const struct its_quirk *quirk;
>
> Is this change really required?

Agreed, the rename is unnecessary. A single entry can carry several
quirk flags, so the existing name also makes sense. I'll keep quirks.

Best regards,
Mykola