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);
     uint32_t iidr;
     uint32_t mask;
     uint32_t flags;
@@ -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;
     }
 
     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;
 
-- 
2.43.0

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

> @@ -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;
>      }
>  
>      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

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.

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

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

On Tue, Mar 31, 2026 at 3:26 AM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
> 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.

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

Hi Mykola,

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

> On Tue, Mar 31, 2026 at 3:26 AM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>> 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.
>
> 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