Re: [PATCH 2/4] xen/arm: its: add platform match callback for ITS quirks

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

Thank you for the review.

On Wed, Mar 25, 2026 at 4:45 PM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > Extend ITS quirk lookup with an optional match callback so that
> > platforms sharing the same IIDR can still be distinguished.
> >
> > Use the board compatible string to positively identify Renesas R-Car
> > Gen4 before applying ITS workaround flags, preventing false matches
> > on other SoCs that happen to use the same GIC IP block.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> >  xen/arch/arm/gic-v3-its.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> > index 00524b43a3..c40629731f 100644
> > --- a/xen/arch/arm/gic-v3-its.c
> > +++ b/xen/arch/arm/gic-v3-its.c
> > @@ -57,6 +57,7 @@ struct its_device {
> >   */
> >  struct its_quirk {
> >      const char *desc;
> > +    bool (*match)(const struct host_its *hw_its);
>
> If you are introducing match predicate, then why do you need...
>
> >      uint32_t iidr;
> >      uint32_t mask;
> >      uint32_t flags;
>
> these? You can use a predicate function to match against iidr

The rationale for keeping iidr/mask while adding match() is to keep
the quirk table declarative and easy to read. The match() callback is
meant only as an optional refinement for ambiguous cases where IIDR
alone is not sufficient to identify the platform.

In this design, iidr/mask remains the primary match key. If matching
were made entirely callback-based, the standard IIDR comparison would
have to move into callback code as well. That would make quirk entries
more open-coded and less data-driven, while the current split keeps the
common case simple and structured.

This is also close to what Linux does: IIDR-based matching remains the
generic declarative mechanism, and platform-specific checks such as
compatible strings are added only where needed.

That said, I agree that the callbacks introduced in this series are all
doing roughly the same kind of platform identification. A reasonable
follow-up cleanup would be to model this more generically, for example
by adding an optional compatible string list to struct its_quirk, and
reserving match() for cases that cannot be expressed through static
data.

So the intent here was to keep the table clean, with matching logic
effectively being:

  quirk_match = IIDR match && (no extra match rule || extra match passes)

If you prefer, I can rework this either into a fully callback-based
scheme, or introduce generic compatible-string matching in this series
and drop the match() callback for now.

>
> > @@ -64,11 +65,24 @@ struct its_quirk {
> >
> >  static uint32_t __ro_after_init its_quirk_flags;
> >
> > +static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
> > +{
> > +    if ( !hw_its->dt_node )
> > +        return false;
> > +
> > +    if ( !dt_machine_is_compatible("renesas,r8a779f0") &&
> > +         !dt_machine_is_compatible("renesas,r8a779g0") )
> > +        return false;
> > +
> > +    return true;
> > +}
> > +
> >  static const struct its_quirk its_quirks[] = {
> >      {
> >          .desc  = "R-Car Gen4",
> >          .iidr  = 0x0201743b,
> >          .mask  = 0xffffffffU,
> > +        .match = gicv3_its_match_quirk_gen4,
> >          .flags = HOST_ITS_WORKAROUND_NC_NS |
> >                   HOST_ITS_WORKAROUND_32BIT_ADDR,
> >      },
> > @@ -77,7 +91,8 @@ static const struct its_quirk its_quirks[] = {
> >      }
> >  };
> >
> > -static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
> > +static const struct its_quirk *gicv3_its_find_quirk(
> > +    const struct host_its *hw_its, uint32_t iidr)
> >  {
> >      const struct its_quirk *quirk = its_quirks;
> >
> > @@ -86,7 +101,8 @@ static const struct its_quirk 
> > *gicv3_its_find_quirk(uint32_t iidr)
> >          if ( quirk->iidr != (quirk->mask & iidr) )
> >              continue;
> >
> > -        return quirk;
> > +        if ( !quirk->match || quirk->match(hw_its) )
> > +            return quirk;

Also, while reviewing gicv3_its_find_quirk() I realized that the
current first-match semantics may not scale well. Since the table
supports partial IIDR masks, we could have a broad entry covering
an entire GIC family alongside a narrower entry for a specific
platform. With first-match, only one of them would ever apply, so
their flags could never be combined. The same issue applies to the
match() callback: if an entry with match() is checked first and
fails, the loop does continue, but if it succeeds, all subsequent
entries for the same IIDR -- whether with different masks or different
match() predicates -- are skipped entirely.

If others agree, I will switch to accumulating flags from all
matching entries in v2.


Best regards,
Mykola

> >      }
> >
> >      return NULL;
> > @@ -99,7 +115,7 @@ static uint32_t gicv3_its_collect_quirks(const struct 
> > host_its *hw_its,
> >      uint32_t flags = 0;
> >      uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
> >
> > -    quirk = gicv3_its_find_quirk(iidr);
> > +    quirk = gicv3_its_find_quirk(hw_its, iidr);
> >      if ( quirk )
> >          flags |= quirk->flags;
>
> --
> WBR, Volodymyr