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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Tue, Mar 31, 2026 at 3:26 AM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > 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.
>
> Well, I don't think that introducing "compatible" string matching will
> do any good. Actually, I think that it will introduce more problems.
>
> What you can do, is to introduce an additional data:
>
> struct its_quirk {
>         const char *desc;
>         bool (*match)(const struct host_its *hw_its, void *priv);
>         void *priv;
>         uint32_t flags;
> };
>
> struct its_iidr_match {
>         uint32_t iidr;
>         uint32_t mask;
> };
>
> static bool iidr_match(const struct host_its *hw_its, void *priv);
> static bool platform_compatbile_match(const struct host_its *hw_its, void 
> *priv);
>
> static struct its_quirk quirks[] = {
>         {.match = iidr_match,
>          .priv = &(struct its_iidr_match) {.iidr = 0xaaaa, .mask = 0xbbbb}},
>         {.match = platform_compatbile_match,
>          .priv = "renesas,r8a779g0"},
> };
>
> Something like that. In this way you can use either a generic predicate
> function or implement your own for more complex cases.
>
> >
> >>
> >> > @@ -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.
>
> I don't think that there is a good use case for this right now, so
> personally I'd skip flags accumulation. Just write a comment that code
> stops and first match, so more specific quirks should go first.

I see the point about not mixing an open-coded DT property check with
the generic quirk matching path in the first patch of this series.

However, taken together with your comment here, that seems to pull the
design in two different directions.

My concern is that dma-noncoherent is not really an alternative
platform quirk, but an orthogonal ITS property that may need to
coexist with other quirks matched via IIDR, machine compatible, or a
custom match() callback.

With the current first-match semantics, if dma-noncoherent is promoted
to a regular struct its_quirk entry, then only one entry would apply,
and we could not combine it with another platform-specific quirk for
the same ITS. In that model, moving dma-noncoherent into the table
would actually make the behavior less generic, not more.

So I think there are two consistent options:

1. keep first-match semantics and leave dma-noncoherent as a separate
additive property, or
2. move dma-noncoherent into the quirk table and switch the lookup to
accumulate flags from all matching entries.

That is why I brought up accumulation in the first place:
dma-noncoherent looks like a concrete case where quirks are
composable rather than mutually exclusive.


Best regards,
Mykola

>
> --
> WBR, Volodymyr