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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykola,

Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:

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

Okay, I see your point. But, if you are saying that dma-noncoherent is
orthogonal to quirks, why it is handled by the quirk code? I expect
quirks to be internally coherent (no pun intended). So yes, for me, the
first option sounds better. But let's wait for opinion from maintainers.

-- 
WBR, Volodymyr