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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Mykola,

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

> Hi Volodymyr,
>
> Thank you for the review.
>
> On Wed, Mar 25, 2026 at 4:45 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>> Hi Mykola,
>>
>> Mykola Kvach <xakep.amatop@gmail.com> writes:
>>
>> > From: Mykola Kvach <mykola_kvach@epam.com>
>> >
>> > 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@epam.com>
>> > ---
>> >  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.

-- 
WBR, Volodymyr