[PATCH 0/4] xen/arm: ITS quirk handling fixes and board-specific matches

[PATCH 0/4] xen/arm: ITS quirk handling fixes and board-specific matches

[Mykola Kvach]

From: Mykola Kvach <mykola_kvach@epam.com>

This series cleans up ARM GICv3 ITS quirk handling and fixes the point at
which quirk-derived memory attributes become visible to the LPI setup path.

The first patch switches the quirk handling from per-entry init callbacks to
declarative flags and folds in the DT dma-noncoherent property when deriving
the effective ITS attributes.

The second patch extends quirk matching with an optional platform callback so
that boards sharing the same IIDR can still be distinguished reliably. This
is then used by the third patch to add the Orange Pi 5 ITS quirk for
RK3588/RK3588S boards.

Finally, the last patch moves ITS quirk discovery and validation earlier in
the boot flow, before host LPI tables are initialized, so the boot CPU does
not allocate and program LPI data structures with default attributes when the
platform requires different ones.

Mykola Kvach (4):
  xen/arm: its: collect quirk flags and honor dma-noncoherent
  xen/arm: its: add platform match callback for ITS quirks
  xen/arm: its: add Orange Pi 5 ITS quirk
  xen/arm: its: pre-initialize ITS quirks before LPI setup

 xen/arch/arm/gic-v3-its.c             | 129 +++++++++++++++++++-------
 xen/arch/arm/gic-v3.c                 |   7 ++
 xen/arch/arm/include/asm/gic_v3_its.h |   5 +
 3 files changed, 107 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent

[Mykola Kvach]

From: Mykola Kvach <mykola_kvach@epam.com>

Replace the per-quirk init callback with declarative flags in
struct its_quirk, and introduce gicv3_its_collect_quirks() to gather
the effective workaround flags from both the IIDR-matched quirk entry
and the "dma-noncoherent" device-tree property.

This lets non-coherent platforms force non-cacheable ITS table
attributes even when no IIDR quirk entry matches.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/arch/arm/gic-v3-its.c | 70 ++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 9ba068c46f..00524b43a3 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -57,71 +57,87 @@ struct its_device {
  */
 struct its_quirk {
     const char *desc;
-    bool (*init)(struct host_its *hw_its);
     uint32_t iidr;
     uint32_t mask;
+    uint32_t flags;
 };
 
 static uint32_t __ro_after_init its_quirk_flags;
 
-static bool gicv3_its_enable_quirk_gen4(struct host_its *hw_its)
-{
-    its_quirk_flags |= HOST_ITS_WORKAROUND_NC_NS |
-        HOST_ITS_WORKAROUND_32BIT_ADDR;
-
-    return true;
-}
-
 static const struct its_quirk its_quirks[] = {
     {
-        .desc	= "R-Car Gen4",
-        .iidr	= 0x0201743b,
-        .mask	= 0xffffffffU,
-        .init	= gicv3_its_enable_quirk_gen4,
+        .desc  = "R-Car Gen4",
+        .iidr  = 0x0201743b,
+        .mask  = 0xffffffffU,
+        .flags = HOST_ITS_WORKAROUND_NC_NS |
+                 HOST_ITS_WORKAROUND_32BIT_ADDR,
     },
     {
         /* Sentinel. */
     }
 };
 
-static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
+static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
 {
-    const struct its_quirk *quirks = its_quirks;
+    const struct its_quirk *quirk = its_quirks;
 
-    for ( ; quirks->desc; quirks++ )
+    for ( ; quirk->desc; quirk++ )
     {
-        if ( quirks->iidr == (quirks->mask & iidr) )
-            return (struct its_quirk *)quirks;
+        if ( quirk->iidr != (quirk->mask & iidr) )
+            continue;
+
+        return quirk;
     }
 
     return NULL;
 }
 
-static void gicv3_its_enable_quirks(struct host_its *hw_its)
+static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
+                                         const struct its_quirk **matched_quirk)
 {
+    const struct its_quirk *quirk;
+    uint32_t flags = 0;
     uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
-    const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
 
-    if ( quirk && quirk->init(hw_its) )
+    quirk = gicv3_its_find_quirk(iidr);
+    if ( quirk )
+        flags |= quirk->flags;
+
+    if ( hw_its->dt_node &&
+         dt_property_read_bool(hw_its->dt_node, "dma-noncoherent") )
+        flags |= HOST_ITS_WORKAROUND_NC_NS;
+
+    if ( matched_quirk )
+        *matched_quirk = quirk;
+
+    return flags;
+}
+
+static void gicv3_its_enable_quirks(struct host_its *hw_its)
+{
+    const struct its_quirk *quirk;
+
+    its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
+
+    if ( quirk )
         printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
 }
 
 static void gicv3_its_validate_quirks(void)
 {
-    const struct its_quirk *quirk = NULL, *prev = NULL;
+    uint32_t quirks, prev_quirks;
     const struct host_its *hw_its;
 
     if ( list_empty(&host_its_list) )
         return;
 
     hw_its = list_first_entry(&host_its_list, struct host_its, entry);
-    prev = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
+    prev_quirks = gicv3_its_collect_quirks(hw_its, NULL);
 
-    list_for_each_entry(hw_its, &host_its_list, entry)
+    list_for_each_entry_continue(hw_its, &host_its_list, entry)
     {
-        quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
-        BUG_ON(quirk != prev);
-        prev = quirk;
+        quirks = gicv3_its_collect_quirks(hw_its, NULL);
+        BUG_ON(quirks != prev_quirks);
     }
 }
 
-- 
2.43.0

Re: [PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent

[Volodymyr Babchuk]

Hi Mykola,

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

> From: Mykola Kvach <mykola_kvach@epam.com>
>
> Replace the per-quirk init callback with declarative flags in
> struct its_quirk, and introduce gicv3_its_collect_quirks() to gather
> the effective workaround flags from both the IIDR-matched quirk entry
> and the "dma-noncoherent" device-tree property.
>
> This lets non-coherent platforms force non-cacheable ITS table
> attributes even when no IIDR quirk entry matches.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
>  xen/arch/arm/gic-v3-its.c | 70 ++++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 9ba068c46f..00524b43a3 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -57,71 +57,87 @@ struct its_device {
>   */
>  struct its_quirk {
>      const char *desc;
> -    bool (*init)(struct host_its *hw_its);
>      uint32_t iidr;
>      uint32_t mask;
> +    uint32_t flags;
>  };
>  
>  static uint32_t __ro_after_init its_quirk_flags;
>  
> -static bool gicv3_its_enable_quirk_gen4(struct host_its *hw_its)
> -{
> -    its_quirk_flags |= HOST_ITS_WORKAROUND_NC_NS |
> -        HOST_ITS_WORKAROUND_32BIT_ADDR;
> -
> -    return true;
> -}
> -
>  static const struct its_quirk its_quirks[] = {
>      {
> -        .desc	= "R-Car Gen4",
> -        .iidr	= 0x0201743b,
> -        .mask	= 0xffffffffU,
> -        .init	= gicv3_its_enable_quirk_gen4,
> +        .desc  = "R-Car Gen4",
> +        .iidr  = 0x0201743b,
> +        .mask  = 0xffffffffU,
> +        .flags = HOST_ITS_WORKAROUND_NC_NS |
> +                 HOST_ITS_WORKAROUND_32BIT_ADDR,
>      },
>      {
>          /* Sentinel. */
>      }
>  };
>  
> -static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
> +static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
>  {
> -    const struct its_quirk *quirks = its_quirks;
> +    const struct its_quirk *quirk = its_quirks;
>  
> -    for ( ; quirks->desc; quirks++ )
> +    for ( ; quirk->desc; quirk++ )
>      {
> -        if ( quirks->iidr == (quirks->mask & iidr) )
> -            return (struct its_quirk *)quirks;
> +        if ( quirk->iidr != (quirk->mask & iidr) )
> +            continue;
> +
> +        return quirk;
>      }
>  
>      return NULL;
>  }
>  
> -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> +static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
> +                                         const struct its_quirk **matched_quirk)
>  {
> +    const struct its_quirk *quirk;
> +    uint32_t flags = 0;
>      uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
> -    const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
>  
> -    if ( quirk && quirk->init(hw_its) )
> +    quirk = gicv3_its_find_quirk(iidr);
> +    if ( quirk )
> +        flags |= quirk->flags;
> +
> +    if ( hw_its->dt_node &&
> +         dt_property_read_bool(hw_its->dt_node, "dma-noncoherent") )
> +        flags |= HOST_ITS_WORKAROUND_NC_NS;
> +
> +    if ( matched_quirk )
> +        *matched_quirk = quirk;
> +
> +    return flags;
> +}
> +
> +static void gicv3_its_enable_quirks(struct host_its *hw_its)
> +{
> +    const struct its_quirk *quirk;
> +
> +    its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
> +
> +    if ( quirk )
>          printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
>  }
>  
>  static void gicv3_its_validate_quirks(void)
>  {
> -    const struct its_quirk *quirk = NULL, *prev = NULL;
> +    uint32_t quirks, prev_quirks;
>      const struct host_its *hw_its;
>  
>      if ( list_empty(&host_its_list) )
>          return;
>  
>      hw_its = list_first_entry(&host_its_list, struct host_its, entry);
> -    prev = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
> +    prev_quirks = gicv3_its_collect_quirks(hw_its, NULL);
>  
> -    list_for_each_entry(hw_its, &host_its_list, entry)
> +    list_for_each_entry_continue(hw_its, &host_its_list, entry)
>      {
> -        quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
> -        BUG_ON(quirk != prev);
> -        prev = quirk;
> +        quirks = gicv3_its_collect_quirks(hw_its, NULL);
> +        BUG_ON(quirks != prev_quirks);

I know it was in the previous version, but as you are already touching
this... This is not Xen BUG(). This is a platform problem. So you need
to panic here. Something like

 if (quirks != prev_quirks) 
        panic("Different ITS instances has different quirks")


Also, I want to point out that you are not validating "dma-noncoherent"
quirk here. I mean, some ITS entries can have this property, some other
- don't. This makes me think that you need to promote this
"dma-noncoherent" quirk from open coded check to a `struct
its_quirk` entry, so it will be handled in generic way.


-- 
WBR, Volodymyr

Re: [PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent

[Mykola Kvach]

Hi Volodymyr,

Thank you for the review.

On Wed, Mar 25, 2026 at 4:42 PM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@gmail.com> writes:
>
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > Replace the per-quirk init callback with declarative flags in
> > struct its_quirk, and introduce gicv3_its_collect_quirks() to gather
> > the effective workaround flags from both the IIDR-matched quirk entry
> > and the "dma-noncoherent" device-tree property.
> >
> > This lets non-coherent platforms force non-cacheable ITS table
> > attributes even when no IIDR quirk entry matches.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> >  xen/arch/arm/gic-v3-its.c | 70 ++++++++++++++++++++++++---------------
> >  1 file changed, 43 insertions(+), 27 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> > index 9ba068c46f..00524b43a3 100644
> > --- a/xen/arch/arm/gic-v3-its.c
> > +++ b/xen/arch/arm/gic-v3-its.c
> > @@ -57,71 +57,87 @@ struct its_device {
> >   */
> >  struct its_quirk {
> >      const char *desc;
> > -    bool (*init)(struct host_its *hw_its);
> >      uint32_t iidr;
> >      uint32_t mask;
> > +    uint32_t flags;
> >  };
> >
> >  static uint32_t __ro_after_init its_quirk_flags;
> >
> > -static bool gicv3_its_enable_quirk_gen4(struct host_its *hw_its)
> > -{
> > -    its_quirk_flags |= HOST_ITS_WORKAROUND_NC_NS |
> > -        HOST_ITS_WORKAROUND_32BIT_ADDR;
> > -
> > -    return true;
> > -}
> > -
> >  static const struct its_quirk its_quirks[] = {
> >      {
> > -        .desc        = "R-Car Gen4",
> > -        .iidr        = 0x0201743b,
> > -        .mask        = 0xffffffffU,
> > -        .init        = gicv3_its_enable_quirk_gen4,
> > +        .desc  = "R-Car Gen4",
> > +        .iidr  = 0x0201743b,
> > +        .mask  = 0xffffffffU,
> > +        .flags = HOST_ITS_WORKAROUND_NC_NS |
> > +                 HOST_ITS_WORKAROUND_32BIT_ADDR,
> >      },
> >      {
> >          /* Sentinel. */
> >      }
> >  };
> >
> > -static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
> > +static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
> >  {
> > -    const struct its_quirk *quirks = its_quirks;
> > +    const struct its_quirk *quirk = its_quirks;
> >
> > -    for ( ; quirks->desc; quirks++ )
> > +    for ( ; quirk->desc; quirk++ )
> >      {
> > -        if ( quirks->iidr == (quirks->mask & iidr) )
> > -            return (struct its_quirk *)quirks;
> > +        if ( quirk->iidr != (quirk->mask & iidr) )
> > +            continue;
> > +
> > +        return quirk;
> >      }
> >
> >      return NULL;
> >  }
> >
> > -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> > +static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
> > +                                         const struct its_quirk **matched_quirk)
> >  {
> > +    const struct its_quirk *quirk;
> > +    uint32_t flags = 0;
> >      uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
> > -    const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
> >
> > -    if ( quirk && quirk->init(hw_its) )
> > +    quirk = gicv3_its_find_quirk(iidr);
> > +    if ( quirk )
> > +        flags |= quirk->flags;
> > +
> > +    if ( hw_its->dt_node &&
> > +         dt_property_read_bool(hw_its->dt_node, "dma-noncoherent") )
> > +        flags |= HOST_ITS_WORKAROUND_NC_NS;
> > +
> > +    if ( matched_quirk )
> > +        *matched_quirk = quirk;
> > +
> > +    return flags;
> > +}
> > +
> > +static void gicv3_its_enable_quirks(struct host_its *hw_its)
> > +{
> > +    const struct its_quirk *quirk;
> > +
> > +    its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
> > +
> > +    if ( quirk )
> >          printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
> >  }
> >
> >  static void gicv3_its_validate_quirks(void)
> >  {
> > -    const struct its_quirk *quirk = NULL, *prev = NULL;
> > +    uint32_t quirks, prev_quirks;
> >      const struct host_its *hw_its;
> >
> >      if ( list_empty(&host_its_list) )
> >          return;
> >
> >      hw_its = list_first_entry(&host_its_list, struct host_its, entry);
> > -    prev = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
> > +    prev_quirks = gicv3_its_collect_quirks(hw_its, NULL);
> >
> > -    list_for_each_entry(hw_its, &host_its_list, entry)
> > +    list_for_each_entry_continue(hw_its, &host_its_list, entry)
> >      {
> > -        quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
> > -        BUG_ON(quirk != prev);
> > -        prev = quirk;
> > +        quirks = gicv3_its_collect_quirks(hw_its, NULL);
> > +        BUG_ON(quirks != prev_quirks);
>
> I know it was in the previous version, but as you are already touching
> this... This is not Xen BUG(). This is a platform problem. So you need
> to panic here. Something like
>
>  if (quirks != prev_quirks)
>         panic("Different ITS instances has different quirks")

Ack.


>
>
> Also, I want to point out that you are not validating "dma-noncoherent"
> quirk here. I mean, some ITS entries can have this property, some other
> - don't. This makes me think that you need to promote this
> "dma-noncoherent" quirk from open coded check to a `struct
> its_quirk` entry, so it will be handled in generic way.

Just to clarify your point about dma-noncoherent:

In the current version it is already part of the effective quirk set,
because gicv3_its_validate_quirks() compares the flags returned by
gicv3_its_collect_quirks(), and those already include the
dma-noncoherent DT property.

So is your concern that DT-derived ITS properties such as
dma-noncoherent should also go through the same common quirk/collection
plumbing as the other ITS workaround sources, rather than being handled
as a separate open-coded check, similar to how Linux models this [1]?

Or did you have something else in mind?


Best regards,
Mykola

[1] https://elixir.bootlin.com/linux/v6.19.9/source/drivers/irqchip/irq-gic-v3-its.c#L4973

>
>
> --
> WBR, Volodymyr

Re: [PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent

[Volodymyr Babchuk]

Hi Mykola,

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

> Hi Volodymyr,
>
> Thank you for the review.
>
> On Wed, Mar 25, 2026 at 4:42 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>> Hi Mykola,
>>
>> Mykola Kvach <xakep.amatop@gmail.com> writes:
>>
>> > From: Mykola Kvach <mykola_kvach@epam.com>
>> >
>> > Replace the per-quirk init callback with declarative flags in
>> > struct its_quirk, and introduce gicv3_its_collect_quirks() to gather
>> > the effective workaround flags from both the IIDR-matched quirk entry
>> > and the "dma-noncoherent" device-tree property.
>> >
>> > This lets non-coherent platforms force non-cacheable ITS table
>> > attributes even when no IIDR quirk entry matches.
>> >
>> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>> > ---
>> >  xen/arch/arm/gic-v3-its.c | 70 ++++++++++++++++++++++++---------------
>> >  1 file changed, 43 insertions(+), 27 deletions(-)
>> >
>> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> > index 9ba068c46f..00524b43a3 100644
>> > --- a/xen/arch/arm/gic-v3-its.c
>> > +++ b/xen/arch/arm/gic-v3-its.c
>> > @@ -57,71 +57,87 @@ struct its_device {
>> >   */
>> >  struct its_quirk {
>> >      const char *desc;
>> > -    bool (*init)(struct host_its *hw_its);
>> >      uint32_t iidr;
>> >      uint32_t mask;
>> > +    uint32_t flags;
>> >  };
>> >
>> >  static uint32_t __ro_after_init its_quirk_flags;
>> >
>> > -static bool gicv3_its_enable_quirk_gen4(struct host_its *hw_its)
>> > -{
>> > -    its_quirk_flags |= HOST_ITS_WORKAROUND_NC_NS |
>> > -        HOST_ITS_WORKAROUND_32BIT_ADDR;
>> > -
>> > -    return true;
>> > -}
>> > -
>> >  static const struct its_quirk its_quirks[] = {
>> >      {
>> > -        .desc        = "R-Car Gen4",
>> > -        .iidr        = 0x0201743b,
>> > -        .mask        = 0xffffffffU,
>> > -        .init        = gicv3_its_enable_quirk_gen4,
>> > +        .desc  = "R-Car Gen4",
>> > +        .iidr  = 0x0201743b,
>> > +        .mask  = 0xffffffffU,
>> > +        .flags = HOST_ITS_WORKAROUND_NC_NS |
>> > +                 HOST_ITS_WORKAROUND_32BIT_ADDR,
>> >      },
>> >      {
>> >          /* Sentinel. */
>> >      }
>> >  };
>> >
>> > -static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
>> > +static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
>> >  {
>> > -    const struct its_quirk *quirks = its_quirks;
>> > +    const struct its_quirk *quirk = its_quirks;
>> >
>> > -    for ( ; quirks->desc; quirks++ )
>> > +    for ( ; quirk->desc; quirk++ )
>> >      {
>> > -        if ( quirks->iidr == (quirks->mask & iidr) )
>> > -            return (struct its_quirk *)quirks;
>> > +        if ( quirk->iidr != (quirk->mask & iidr) )
>> > +            continue;
>> > +
>> > +        return quirk;
>> >      }
>> >
>> >      return NULL;
>> >  }
>> >
>> > -static void gicv3_its_enable_quirks(struct host_its *hw_its)
>> > +static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
>> > +                                         const struct its_quirk **matched_quirk)
>> >  {
>> > +    const struct its_quirk *quirk;
>> > +    uint32_t flags = 0;
>> >      uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
>> > -    const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
>> >
>> > -    if ( quirk && quirk->init(hw_its) )
>> > +    quirk = gicv3_its_find_quirk(iidr);
>> > +    if ( quirk )
>> > +        flags |= quirk->flags;
>> > +
>> > +    if ( hw_its->dt_node &&
>> > +         dt_property_read_bool(hw_its->dt_node, "dma-noncoherent") )
>> > +        flags |= HOST_ITS_WORKAROUND_NC_NS;
>> > +
>> > +    if ( matched_quirk )
>> > +        *matched_quirk = quirk;
>> > +
>> > +    return flags;
>> > +}
>> > +
>> > +static void gicv3_its_enable_quirks(struct host_its *hw_its)
>> > +{
>> > +    const struct its_quirk *quirk;
>> > +
>> > +    its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
>> > +
>> > +    if ( quirk )
>> >          printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
>> >  }
>> >
>> >  static void gicv3_its_validate_quirks(void)
>> >  {
>> > -    const struct its_quirk *quirk = NULL, *prev = NULL;
>> > +    uint32_t quirks, prev_quirks;
>> >      const struct host_its *hw_its;
>> >
>> >      if ( list_empty(&host_its_list) )
>> >          return;
>> >
>> >      hw_its = list_first_entry(&host_its_list, struct host_its, entry);
>> > -    prev = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
>> > +    prev_quirks = gicv3_its_collect_quirks(hw_its, NULL);
>> >
>> > -    list_for_each_entry(hw_its, &host_its_list, entry)
>> > +    list_for_each_entry_continue(hw_its, &host_its_list, entry)
>> >      {
>> > -        quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
>> > -        BUG_ON(quirk != prev);
>> > -        prev = quirk;
>> > +        quirks = gicv3_its_collect_quirks(hw_its, NULL);
>> > +        BUG_ON(quirks != prev_quirks);
>>
>> I know it was in the previous version, but as you are already touching
>> this... This is not Xen BUG(). This is a platform problem. So you need
>> to panic here. Something like
>>
>>  if (quirks != prev_quirks)
>>         panic("Different ITS instances has different quirks")
>
> Ack.
>
>
>>
>>
>> Also, I want to point out that you are not validating "dma-noncoherent"
>> quirk here. I mean, some ITS entries can have this property, some other
>> - don't. This makes me think that you need to promote this
>> "dma-noncoherent" quirk from open coded check to a `struct
>> its_quirk` entry, so it will be handled in generic way.
>
> Just to clarify your point about dma-noncoherent:
>
> In the current version it is already part of the effective quirk set,
> because gicv3_its_validate_quirks() compares the flags returned by
> gicv3_its_collect_quirks(), and those already include the
> dma-noncoherent DT property.
>
> So is your concern that DT-derived ITS properties such as
> dma-noncoherent should also go through the same common quirk/collection
> plumbing as the other ITS workaround sources, rather than being handled
> as a separate open-coded check, similar to how Linux models this [1]?

Yes, exactly this. I see no point in introducing generic predicate
function and then not using it. This will confuse anyone who tries to
read the code.

-- 
WBR, Volodymyr

Re: [PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent

[Oleksandr Tyshchenko]

On 3/25/26 17:47, Mykola Kvach wrote:
> Hi Volodymyr,

Hello Mykola and Volodymyr


> 
> Thank you for the review.
> 
> On Wed, Mar 25, 2026 at 4:42 PM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>> Hi Mykola,
>>
>> Mykola Kvach <xakep.amatop@gmail.com> writes:
>>
>>> From: Mykola Kvach <mykola_kvach@epam.com>
>>>
>>> Replace the per-quirk init callback with declarative flags in
>>> struct its_quirk, and introduce gicv3_its_collect_quirks() to gather
>>> the effective workaround flags from both the IIDR-matched quirk entry
>>> and the "dma-noncoherent" device-tree property.
>>>
>>> This lets non-coherent platforms force non-cacheable ITS table
>>> attributes even when no IIDR quirk entry matches.
>>>
>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>>> ---
>>>   xen/arch/arm/gic-v3-its.c | 70 ++++++++++++++++++++++++---------------
>>>   1 file changed, 43 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>>> index 9ba068c46f..00524b43a3 100644
>>> --- a/xen/arch/arm/gic-v3-its.c
>>> +++ b/xen/arch/arm/gic-v3-its.c
>>> @@ -57,71 +57,87 @@ struct its_device {
>>>    */
>>>   struct its_quirk {
>>>       const char *desc;
>>> -    bool (*init)(struct host_its *hw_its);
>>>       uint32_t iidr;
>>>       uint32_t mask;
>>> +    uint32_t flags;
>>>   };
>>>
>>>   static uint32_t __ro_after_init its_quirk_flags;
>>>
>>> -static bool gicv3_its_enable_quirk_gen4(struct host_its *hw_its)
>>> -{
>>> -    its_quirk_flags |= HOST_ITS_WORKAROUND_NC_NS |
>>> -        HOST_ITS_WORKAROUND_32BIT_ADDR;
>>> -
>>> -    return true;
>>> -}
>>> -
>>>   static const struct its_quirk its_quirks[] = {
>>>       {
>>> -        .desc        = "R-Car Gen4",
>>> -        .iidr        = 0x0201743b,
>>> -        .mask        = 0xffffffffU,
>>> -        .init        = gicv3_its_enable_quirk_gen4,
>>> +        .desc  = "R-Car Gen4",
>>> +        .iidr  = 0x0201743b,
>>> +        .mask  = 0xffffffffU,
>>> +        .flags = HOST_ITS_WORKAROUND_NC_NS |
>>> +                 HOST_ITS_WORKAROUND_32BIT_ADDR,
>>>       },
>>>       {
>>>           /* Sentinel. */
>>>       }
>>>   };
>>>
>>> -static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
>>> +static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
>>>   {
>>> -    const struct its_quirk *quirks = its_quirks;
>>> +    const struct its_quirk *quirk = its_quirks;
>>>
>>> -    for ( ; quirks->desc; quirks++ )
>>> +    for ( ; quirk->desc; quirk++ )
>>>       {
>>> -        if ( quirks->iidr == (quirks->mask & iidr) )
>>> -            return (struct its_quirk *)quirks;
>>> +        if ( quirk->iidr != (quirk->mask & iidr) )
>>> +            continue;
>>> +
>>> +        return quirk;
>>>       }
>>>
>>>       return NULL;
>>>   }
>>>
>>> -static void gicv3_its_enable_quirks(struct host_its *hw_its)
>>> +static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
>>> +                                         const struct its_quirk **matched_quirk)
>>>   {
>>> +    const struct its_quirk *quirk;
>>> +    uint32_t flags = 0;
>>>       uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
>>> -    const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
>>>
>>> -    if ( quirk && quirk->init(hw_its) )
>>> +    quirk = gicv3_its_find_quirk(iidr);
>>> +    if ( quirk )
>>> +        flags |= quirk->flags;
>>> +
>>> +    if ( hw_its->dt_node &&
>>> +         dt_property_read_bool(hw_its->dt_node, "dma-noncoherent") )
>>> +        flags |= HOST_ITS_WORKAROUND_NC_NS;
>>> +
>>> +    if ( matched_quirk )
>>> +        *matched_quirk = quirk;
>>> +
>>> +    return flags;
>>> +}
>>> +
>>> +static void gicv3_its_enable_quirks(struct host_its *hw_its)
>>> +{
>>> +    const struct its_quirk *quirk;
>>> +
>>> +    its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
>>> +
>>> +    if ( quirk )
>>>           printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
>>>   }
>>>
>>>   static void gicv3_its_validate_quirks(void)
>>>   {
>>> -    const struct its_quirk *quirk = NULL, *prev = NULL;
>>> +    uint32_t quirks, prev_quirks;
>>>       const struct host_its *hw_its;
>>>
>>>       if ( list_empty(&host_its_list) )
>>>           return;
>>>
>>>       hw_its = list_first_entry(&host_its_list, struct host_its, entry);
>>> -    prev = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
>>> +    prev_quirks = gicv3_its_collect_quirks(hw_its, NULL);
>>>
>>> -    list_for_each_entry(hw_its, &host_its_list, entry)
>>> +    list_for_each_entry_continue(hw_its, &host_its_list, entry)
>>>       {
>>> -        quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
>>> -        BUG_ON(quirk != prev);
>>> -        prev = quirk;
>>> +        quirks = gicv3_its_collect_quirks(hw_its, NULL);
>>> +        BUG_ON(quirks != prev_quirks);
>>
>> I know it was in the previous version, but as you are already touching
>> this... This is not Xen BUG(). This is a platform problem. So you need
>> to panic here. Something like
>>
>>   if (quirks != prev_quirks)
>>          panic("Different ITS instances has different quirks")
> 
> Ack.
> 
> 

I agree that a quirk mismatch is a platform problem. Yes, the current 
design uses global flags, making it unable to handle mixed quirks, 
leading to the failure on mismatch.

Please note, I am not saying a panic() is wrong here and I am not 
requesting any changes here; I was just wondering why this is handled 
differently than the SMMUv3 driver. I am just thinking out loud.

SMMUv3 driver handles feature mismatches by gracefully degrading. When 
it finds an SMMU device that does not support ARM_SMMU_FEAT_COHERENCY, 
it disables that feature for the entire platform (so the P2M code has to 
clean the cache when updating ptes). It does not panic. How the ITS and 
SMMUv3 drivers are different in that regard? Why could not we apply the 
same "worst-case" logic here?
For example:
- if any ITS device requires non-cacheable memory, then all ITS memory 
allocations should use non-cacheable memory.
- if any ITS device requires 32-bit addresses, then all ITS memory 
allocations should be constrained to 32-bits.

This would be consistent with the SMMU precedent and would allow the 
system to boot and function correctly, but with the performance 
characteristics of the worst ITS device in the system.

Or I really missed something?











[snip]

Re: [PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent

[Mykola Kvach]

Hi Oleksandr,

Thank you for the review.

On Tue, Apr 28, 2026 at 9:42 PM Oleksandr Tyshchenko
<olekstysh@gmail.com> wrote:
>
>
>
> On 3/25/26 17:47, Mykola Kvach wrote:
> > Hi Volodymyr,
>
> Hello Mykola and Volodymyr
>
>
> >
> > Thank you for the review.
> >
> > On Wed, Mar 25, 2026 at 4:42 PM Volodymyr Babchuk
> > <Volodymyr_Babchuk@epam.com> wrote:
> >>
> >> Hi Mykola,
> >>
> >> Mykola Kvach <xakep.amatop@gmail.com> writes:
> >>
> >>> From: Mykola Kvach <mykola_kvach@epam.com>
> >>>
> >>> Replace the per-quirk init callback with declarative flags in
> >>> struct its_quirk, and introduce gicv3_its_collect_quirks() to gather
> >>> the effective workaround flags from both the IIDR-matched quirk entry
> >>> and the "dma-noncoherent" device-tree property.
> >>>
> >>> This lets non-coherent platforms force non-cacheable ITS table
> >>> attributes even when no IIDR quirk entry matches.
> >>>
> >>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> >>> ---
> >>>   xen/arch/arm/gic-v3-its.c | 70 ++++++++++++++++++++++++---------------
> >>>   1 file changed, 43 insertions(+), 27 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> >>> index 9ba068c46f..00524b43a3 100644
> >>> --- a/xen/arch/arm/gic-v3-its.c
> >>> +++ b/xen/arch/arm/gic-v3-its.c
> >>> @@ -57,71 +57,87 @@ struct its_device {
> >>>    */
> >>>   struct its_quirk {
> >>>       const char *desc;
> >>> -    bool (*init)(struct host_its *hw_its);
> >>>       uint32_t iidr;
> >>>       uint32_t mask;
> >>> +    uint32_t flags;
> >>>   };
> >>>
> >>>   static uint32_t __ro_after_init its_quirk_flags;
> >>>
> >>> -static bool gicv3_its_enable_quirk_gen4(struct host_its *hw_its)
> >>> -{
> >>> -    its_quirk_flags |= HOST_ITS_WORKAROUND_NC_NS |
> >>> -        HOST_ITS_WORKAROUND_32BIT_ADDR;
> >>> -
> >>> -    return true;
> >>> -}
> >>> -
> >>>   static const struct its_quirk its_quirks[] = {
> >>>       {
> >>> -        .desc        = "R-Car Gen4",
> >>> -        .iidr        = 0x0201743b,
> >>> -        .mask        = 0xffffffffU,
> >>> -        .init        = gicv3_its_enable_quirk_gen4,
> >>> +        .desc  = "R-Car Gen4",
> >>> +        .iidr  = 0x0201743b,
> >>> +        .mask  = 0xffffffffU,
> >>> +        .flags = HOST_ITS_WORKAROUND_NC_NS |
> >>> +                 HOST_ITS_WORKAROUND_32BIT_ADDR,
> >>>       },
> >>>       {
> >>>           /* Sentinel. */
> >>>       }
> >>>   };
> >>>
> >>> -static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
> >>> +static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
> >>>   {
> >>> -    const struct its_quirk *quirks = its_quirks;
> >>> +    const struct its_quirk *quirk = its_quirks;
> >>>
> >>> -    for ( ; quirks->desc; quirks++ )
> >>> +    for ( ; quirk->desc; quirk++ )
> >>>       {
> >>> -        if ( quirks->iidr == (quirks->mask & iidr) )
> >>> -            return (struct its_quirk *)quirks;
> >>> +        if ( quirk->iidr != (quirk->mask & iidr) )
> >>> +            continue;
> >>> +
> >>> +        return quirk;
> >>>       }
> >>>
> >>>       return NULL;
> >>>   }
> >>>
> >>> -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> >>> +static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
> >>> +                                         const struct its_quirk **matched_quirk)
> >>>   {
> >>> +    const struct its_quirk *quirk;
> >>> +    uint32_t flags = 0;
> >>>       uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
> >>> -    const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
> >>>
> >>> -    if ( quirk && quirk->init(hw_its) )
> >>> +    quirk = gicv3_its_find_quirk(iidr);
> >>> +    if ( quirk )
> >>> +        flags |= quirk->flags;
> >>> +
> >>> +    if ( hw_its->dt_node &&
> >>> +         dt_property_read_bool(hw_its->dt_node, "dma-noncoherent") )
> >>> +        flags |= HOST_ITS_WORKAROUND_NC_NS;
> >>> +
> >>> +    if ( matched_quirk )
> >>> +        *matched_quirk = quirk;
> >>> +
> >>> +    return flags;
> >>> +}
> >>> +
> >>> +static void gicv3_its_enable_quirks(struct host_its *hw_its)
> >>> +{
> >>> +    const struct its_quirk *quirk;
> >>> +
> >>> +    its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
> >>> +
> >>> +    if ( quirk )
> >>>           printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
> >>>   }
> >>>
> >>>   static void gicv3_its_validate_quirks(void)
> >>>   {
> >>> -    const struct its_quirk *quirk = NULL, *prev = NULL;
> >>> +    uint32_t quirks, prev_quirks;
> >>>       const struct host_its *hw_its;
> >>>
> >>>       if ( list_empty(&host_its_list) )
> >>>           return;
> >>>
> >>>       hw_its = list_first_entry(&host_its_list, struct host_its, entry);
> >>> -    prev = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
> >>> +    prev_quirks = gicv3_its_collect_quirks(hw_its, NULL);
> >>>
> >>> -    list_for_each_entry(hw_its, &host_its_list, entry)
> >>> +    list_for_each_entry_continue(hw_its, &host_its_list, entry)
> >>>       {
> >>> -        quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
> >>> -        BUG_ON(quirk != prev);
> >>> -        prev = quirk;
> >>> +        quirks = gicv3_its_collect_quirks(hw_its, NULL);
> >>> +        BUG_ON(quirks != prev_quirks);
> >>
> >> I know it was in the previous version, but as you are already touching
> >> this... This is not Xen BUG(). This is a platform problem. So you need
> >> to panic here. Something like
> >>
> >>   if (quirks != prev_quirks)
> >>          panic("Different ITS instances has different quirks")
> >
> > Ack.
> >
> >
>
> I agree that a quirk mismatch is a platform problem. Yes, the current
> design uses global flags, making it unable to handle mixed quirks,
> leading to the failure on mismatch.
>
> Please note, I am not saying a panic() is wrong here and I am not
> requesting any changes here; I was just wondering why this is handled
> differently than the SMMUv3 driver. I am just thinking out loud.
>
> SMMUv3 driver handles feature mismatches by gracefully degrading. When
> it finds an SMMU device that does not support ARM_SMMU_FEAT_COHERENCY,
> it disables that feature for the entire platform (so the P2M code has to
> clean the cache when updating ptes). It does not panic. How the ITS and
> SMMUv3 drivers are different in that regard? Why could not we apply the
> same "worst-case" logic here?
> For example:
> - if any ITS device requires non-cacheable memory, then all ITS memory
> allocations should use non-cacheable memory.
> - if any ITS device requires 32-bit addresses, then all ITS memory
> allocations should be constrained to 32-bits.
>
> This would be consistent with the SMMU precedent and would allow the
> system to boot and function correctly, but with the performance
> characteristics of the worst ITS device in the system.
>
> Or I really missed something?

Yes, I think this makes sense.

The current series still treats the ITS workaround state as global.
Patch 4 only moves the quirk setup earlier, before the LPI tables are
allocated. However, the effective flags are still taken from a single
ITS instance, and the validation logic still requires all ITS instances
to report the same flag set.

For the currently supported quirks this is probably too strict. Both
HOST_ITS_WORKAROUND_NC_NS and HOST_ITS_WORKAROUND_32BIT_ADDR are
conservative restrictions on memory attributes/allocation. Using
non-cacheable attributes for all ITS-related allocations, or constraining
them to 32-bit addresses, should also be safe for ITS instances that do
not require these restrictions.

So I agree that the SMMUv3-style worst-case policy is a better fit here.
I will rework this in v2 so that Xen aggregates the effective flags from
all host ITS instances instead of requiring exact equality.

I will also document that this aggregation is only valid for conservative
workaround flags. A future non-composable quirk should either remain
per-ITS or get explicit validation logic.

Thanks for pointing out the SMMUv3 precedent.

Best regards,
Mykola

>
>
>
>
>
>
>
>
>
>
>
> [snip]

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

[Mykola Kvach]

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

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

[Volodymyr Babchuk]

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

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

[Mykola Kvach]

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

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

[Volodymyr Babchuk]

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

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

[Mykola Kvach]

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

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

[Volodymyr Babchuk]

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

[PATCH 3/4] xen/arm: its: add Orange Pi 5 ITS quirk

[Mykola Kvach]

From: Mykola Kvach <mykola_kvach@epam.com>

Add an ITS quirk entry for Orange Pi 5 boards based on Rockchip
RK3588/RK3588S and match it via the platform compatible string.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/arch/arm/gic-v3-its.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index c40629731f..ee432088cd 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -77,6 +77,18 @@ static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
     return true;
 }
 
+static bool gicv3_its_match_quirk_opi5(const struct host_its *hw_its)
+{
+    if ( !hw_its->dt_node )
+        return false;
+
+    if ( !dt_machine_is_compatible("rockchip,rk3588") &&
+         !dt_machine_is_compatible("rockchip,rk3588s") )
+        return false;
+
+    return true;
+}
+
 static const struct its_quirk its_quirks[] = {
     {
         .desc  = "R-Car Gen4",
@@ -86,6 +98,13 @@ static const struct its_quirk its_quirks[] = {
         .flags = HOST_ITS_WORKAROUND_NC_NS |
                  HOST_ITS_WORKAROUND_32BIT_ADDR,
     },
+    {
+        .desc  = "Orange Pi 5",
+        .iidr  = 0x0201743b,
+        .mask  = 0xffffffffU,
+        .match = gicv3_its_match_quirk_opi5,
+        .flags = HOST_ITS_WORKAROUND_32BIT_ADDR,
+    },
     {
         /* Sentinel. */
     }
-- 
2.43.0

[PATCH 4/4] xen/arm: its: pre-initialize ITS quirks before LPI setup

[Mykola Kvach]

From: Mykola Kvach <mykola_kvach@epam.com>

In the current initialization flow, gicv3_init() calls gicv3_dist_init()
before gicv3_its_init().

When LPIs are supported, gicv3_dist_init() calls
gicv3_lpi_init_host_lpis(), which initializes host LPI state and allocates
the boot CPU pending table before ITS quirk flags are computed. Non-boot
CPUs allocate their pending tables later from the CPU_UP_PREPARE notifier,
while redistributor LPI programming happens separately in
gicv3_lpi_init_rdist().

This means the boot CPU LPI setup can observe default ITS memory attributes
before dma-noncoherent and other ITS quirks are applied.

Introduce gicv3_its_preinit() and call it before gicv3_dist_init(). This
keeps the actual ITS hardware initialization in gicv3_its_init(), but moves
ITS discovery, quirk validation and quirk flag setup early enough for the
subsequent LPI initialization to use the correct attributes.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
TODO: Think about separating Redistributor/LPI attributes from ITS.
---
 xen/arch/arm/gic-v3-its.c             | 36 +++++++++++++++++----------
 xen/arch/arm/gic-v3.c                 |  7 ++++++
 xen/arch/arm/include/asm/gic_v3_its.h |  5 ++++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index ee432088cd..0251d91630 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -63,6 +63,7 @@ struct its_quirk {
     uint32_t flags;
 };
 
+/* TODO: Separate Redistributor/LPI attributes from ITS quirks. */
 static uint32_t __ro_after_init its_quirk_flags;
 
 static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
@@ -148,9 +149,15 @@ static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
     return flags;
 }
 
-static void gicv3_its_enable_quirks(struct host_its *hw_its)
+static void gicv3_its_enable_quirks(void)
 {
     const struct its_quirk *quirk;
+    const struct host_its *hw_its;
+
+    if ( list_empty(&host_its_list) )
+        return;
+
+    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
 
     its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
 
@@ -603,16 +610,10 @@ static int gicv3_its_init_single_its(struct host_its *hw_its)
     uint64_t reg;
     int i, ret;
 
-    hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
-    if ( !hw_its->its_base )
-        return -ENOMEM;
-
     ret = gicv3_disable_its(hw_its);
     if ( ret )
         return ret;
 
-    gicv3_its_enable_quirks(hw_its);
-
     reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
     hw_its->devid_bits = GITS_TYPER_DEVICE_ID_BITS(reg);
     hw_its->evid_bits = GITS_TYPER_EVENT_ID_BITS(reg);
@@ -1161,6 +1162,11 @@ static void add_to_host_its_list(paddr_t addr, paddr_t size,
     its_data->size = size;
     its_data->dt_node = node;
 
+    its_data->its_base = ioremap_nocache(its_data->addr, its_data->size);
+    if ( !its_data->its_base )
+        panic("GICv3: Cannot map ITS frame: 0x%lx, 0x%lx\n",
+            its_data->addr, its_data->size);
+
     printk("GICv3: Found ITS @0x%lx\n", addr);
 
     list_add_tail(&its_data->entry, &host_its_list);
@@ -1238,16 +1244,22 @@ static void gicv3_its_acpi_init(void)
 
 #endif
 
-int gicv3_its_init(void)
+void __init gicv3_its_preinit(void)
 {
-    struct host_its *hw_its;
-    int ret;
-
     if ( acpi_disabled )
         gicv3_its_dt_init(dt_interrupt_controller);
     else
         gicv3_its_acpi_init();
 
+    gicv3_its_validate_quirks();
+    gicv3_its_enable_quirks();
+}
+
+int gicv3_its_init(void)
+{
+    struct host_its *hw_its;
+    int ret;
+
     list_for_each_entry(hw_its, &host_its_list, entry)
     {
         ret = gicv3_its_init_single_its(hw_its);
@@ -1255,8 +1267,6 @@ int gicv3_its_init(void)
             return ret;
     }
 
-    gicv3_its_validate_quirks();
-
     return 0;
 }
 
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index bc07f97c16..6e44d23d64 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1974,6 +1974,13 @@ static int __init gicv3_init(void)
 
     spin_lock(&gicv3.lock);
 
+    if ( gic_dist_supports_lpis() )
+        /*
+         * Apply ITS quirks before gicv3_dist_init() sets up host LPIs,
+         * so pending tables use the correct ITS memory attributes.
+         */
+        gicv3_its_preinit();
+
     gicv3_dist_init();
 
     if ( gic_dist_supports_lpis() )
diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h
index fc5a84892c..e1d7522ea5 100644
--- a/xen/arch/arm/include/asm/gic_v3_its.h
+++ b/xen/arch/arm/include/asm/gic_v3_its.h
@@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
 
 /* Initialize the host structures for LPIs and the host ITSes. */
 int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
+void gicv3_its_preinit(void);
 int gicv3_its_init(void);
 
 /* Store the physical address and ID for each redistributor as read from DT. */
@@ -219,6 +220,10 @@ static inline int gicv3_its_deny_access(struct domain *d)
     return 0;
 }
 
+static inline void gicv3_its_preinit(void)
+{
+}
+
 static inline bool gicv3_its_host_has_its(void)
 {
     return false;
-- 
2.43.0

Re: [PATCH 4/4] xen/arm: its: pre-initialize ITS quirks before LPI setup

[Volodymyr Babchuk]

Hi,

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

> From: Mykola Kvach <mykola_kvach@epam.com>
>
> In the current initialization flow, gicv3_init() calls gicv3_dist_init()
> before gicv3_its_init().
>
> When LPIs are supported, gicv3_dist_init() calls
> gicv3_lpi_init_host_lpis(), which initializes host LPI state and allocates
> the boot CPU pending table before ITS quirk flags are computed. Non-boot
> CPUs allocate their pending tables later from the CPU_UP_PREPARE notifier,
> while redistributor LPI programming happens separately in
> gicv3_lpi_init_rdist().
>
> This means the boot CPU LPI setup can observe default ITS memory attributes
> before dma-noncoherent and other ITS quirks are applied.
>
> Introduce gicv3_its_preinit() and call it before gicv3_dist_init(). This
> keeps the actual ITS hardware initialization in gicv3_its_init(), but moves
> ITS discovery, quirk validation and quirk flag setup early enough for the
> subsequent LPI initialization to use the correct attributes.

I must say that it was hard to review this patch, because it does more
description says.

>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> TODO: Think about separating Redistributor/LPI attributes from ITS.
> ---
>  xen/arch/arm/gic-v3-its.c             | 36 +++++++++++++++++----------
>  xen/arch/arm/gic-v3.c                 |  7 ++++++
>  xen/arch/arm/include/asm/gic_v3_its.h |  5 ++++
>  3 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index ee432088cd..0251d91630 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -63,6 +63,7 @@ struct its_quirk {
>      uint32_t flags;
>  };
>  
> +/* TODO: Separate Redistributor/LPI attributes from ITS quirks. */
>  static uint32_t __ro_after_init its_quirk_flags;
>  
>  static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
> @@ -148,9 +149,15 @@ static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
>      return flags;
>  }
>  
> -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> +static void gicv3_its_enable_quirks(void)
>  {
>      const struct its_quirk *quirk;
> +    const struct host_its *hw_its;
> +
> +    if ( list_empty(&host_its_list) )
> +        return;
> +
> +    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
>  
>      its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
>  
> @@ -603,16 +610,10 @@ static int gicv3_its_init_single_its(struct host_its *hw_its)
>      uint64_t reg;
>      int i, ret;
>  
> -    hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
> -    if ( !hw_its->its_base )
> -        return -ENOMEM;
> -
>      ret = gicv3_disable_its(hw_its);
>      if ( ret )
>          return ret;
>  
> -    gicv3_its_enable_quirks(hw_its);
> -
>      reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
>      hw_its->devid_bits = GITS_TYPER_DEVICE_ID_BITS(reg);
>      hw_its->evid_bits = GITS_TYPER_EVENT_ID_BITS(reg);
> @@ -1161,6 +1162,11 @@ static void add_to_host_its_list(paddr_t addr, paddr_t size,
>      its_data->size = size;
>      its_data->dt_node = node;
>  
> +    its_data->its_base = ioremap_nocache(its_data->addr, its_data->size);

It is very confusing that add_to_host_its_list() function ioremaps
ITS. It should not do this. Or at least renamed it to
map_and_add_to_host_its_list() or consider moving this to
gicv3_its_preinit(), maybe?

> +    if ( !its_data->its_base )
> +        panic("GICv3: Cannot map ITS frame: 0x%lx, 0x%lx\n",
> +            its_data->addr, its_data->size);
> +
>      printk("GICv3: Found ITS @0x%lx\n", addr);
>  
>      list_add_tail(&its_data->entry, &host_its_list);
> @@ -1238,16 +1244,22 @@ static void gicv3_its_acpi_init(void)
>  
>  #endif
>  
> -int gicv3_its_init(void)
> +void __init gicv3_its_preinit(void)
>  {
> -    struct host_its *hw_its;
> -    int ret;
> -
>      if ( acpi_disabled )
>          gicv3_its_dt_init(dt_interrupt_controller);
>      else
>          gicv3_its_acpi_init();
>  
> +    gicv3_its_validate_quirks();
> +    gicv3_its_enable_quirks();
> +}
> +
> +int gicv3_its_init(void)
> +{
> +    struct host_its *hw_its;
> +    int ret;
> +
>      list_for_each_entry(hw_its, &host_its_list, entry)
>      {
>          ret = gicv3_its_init_single_its(hw_its);
> @@ -1255,8 +1267,6 @@ int gicv3_its_init(void)
>              return ret;
>      }
>  
> -    gicv3_its_validate_quirks();
> -
>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index bc07f97c16..6e44d23d64 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1974,6 +1974,13 @@ static int __init gicv3_init(void)
>  
>      spin_lock(&gicv3.lock);
>  
> +    if ( gic_dist_supports_lpis() )
> +        /*
> +         * Apply ITS quirks before gicv3_dist_init() sets up host LPIs,
> +         * so pending tables use the correct ITS memory attributes.
> +         */
> +        gicv3_its_preinit();
> +
>      gicv3_dist_init();
>  
>      if ( gic_dist_supports_lpis() )
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h
> index fc5a84892c..e1d7522ea5 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
>  
>  /* Initialize the host structures for LPIs and the host ITSes. */
>  int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
> +void gicv3_its_preinit(void);
>  int gicv3_its_init(void);
>  
>  /* Store the physical address and ID for each redistributor as read from DT. */
> @@ -219,6 +220,10 @@ static inline int gicv3_its_deny_access(struct domain *d)
>      return 0;
>  }
>  
> +static inline void gicv3_its_preinit(void)
> +{
> +}
> +
>  static inline bool gicv3_its_host_has_its(void)
>  {
>      return false;

-- 
WBR, Volodymyr

Re: [PATCH 4/4] xen/arm: its: pre-initialize ITS quirks before LPI setup

[Oleksandr Tyshchenko]

On 3/25/26 12:38, Mykola Kvach wrote:

Hello Mykola

> From: Mykola Kvach <mykola_kvach@epam.com>
> 
> In the current initialization flow, gicv3_init() calls gicv3_dist_init()
> before gicv3_its_init().
> 
> When LPIs are supported, gicv3_dist_init() calls
> gicv3_lpi_init_host_lpis(), which initializes host LPI state and allocates
> the boot CPU pending table before ITS quirk flags are computed. Non-boot
> CPUs allocate their pending tables later from the CPU_UP_PREPARE notifier,
> while redistributor LPI programming happens separately in
> gicv3_lpi_init_rdist().
> 
> This means the boot CPU LPI setup can observe default ITS memory attributes
> before dma-noncoherent and other ITS quirks are applied.
> 
> Introduce gicv3_its_preinit() and call it before gicv3_dist_init(). This
> keeps the actual ITS hardware initialization in gicv3_its_init(), but moves
> ITS discovery, quirk validation and quirk flag setup early enough for the
> subsequent LPI initialization to use the correct attributes.


Have you considered an alternative approach that might be less invasive? 
I am thinking of something the other way around: perhaps we could 
allocate the LPI pending table for the boot CPU later.

Would a diff below work?


---
  xen/arch/arm/gic-v3-lpi.c             | 9 +++++++++
  xen/arch/arm/gic-v3.c                 | 2 ++
  xen/arch/arm/include/asm/gic_v3_its.h | 6 ++++++
  3 files changed, 17 insertions(+)

diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
index 9ee338edc2..61cc45d347 100644
--- a/xen/arch/arm/gic-v3-lpi.c
+++ b/xen/arch/arm/gic-v3-lpi.c
@@ -450,6 +450,15 @@ int gicv3_lpi_init_host_lpis(unsigned int 
host_lpi_bits)

      printk("GICv3: using at most %lu LPIs on the host.\n", 
MAX_NR_HOST_LPIS);

+    return rc;
+}
+
+int gicv3_lpi_post_init_host_lpis(void)
+{
+    int rc;
+
+    ASSERT(smp_processor_id() == 0);
+
      /* Register the CPU notifier and allocate memory for the boot CPU */
      register_cpu_notifier(&cpu_nfb);
      rc = gicv3_lpi_allocate_pendtable(smp_processor_id());
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 7f365cdbe9..8b9059c5c9 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1981,6 +1981,8 @@ static int __init gicv3_init(void)
          res = gicv3_its_init();
          if ( res )
              panic("GICv3: ITS: initialization failed: %d\n", res);
+
+        gicv3_lpi_post_init_host_lpis();
      }

      res = gicv3_cpu_init();
diff --git a/xen/arch/arm/include/asm/gic_v3_its.h 
b/xen/arch/arm/include/asm/gic_v3_its.h
index fc5a84892c..288cc1d42f 100644
--- a/xen/arch/arm/include/asm/gic_v3_its.h
+++ b/xen/arch/arm/include/asm/gic_v3_its.h
@@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);

  /* Initialize the host structures for LPIs and the host ITSes. */
  int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
+int gicv3_lpi_post_init_host_lpis(void);
  int gicv3_its_init(void);

  /* Store the physical address and ID for each redistributor as read 
from DT. */
@@ -245,6 +246,11 @@ static inline int gicv3_lpi_init_host_lpis(unsigned 
int host_lpi_bits)
      return 0;
  }

+static inline int gicv3_lpi_post_init_host_lpis(void)
+{
+    return 0;
+}
+
  static inline int gicv3_its_init(void)
  {
      return 0;
-- 
2.34.1


> 
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> TODO: Think about separating Redistributor/LPI attributes from ITS.
> ---
>   xen/arch/arm/gic-v3-its.c             | 36 +++++++++++++++++----------
>   xen/arch/arm/gic-v3.c                 |  7 ++++++
>   xen/arch/arm/include/asm/gic_v3_its.h |  5 ++++
>   3 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index ee432088cd..0251d91630 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -63,6 +63,7 @@ struct its_quirk {
>       uint32_t flags;
>   };
>   
> +/* TODO: Separate Redistributor/LPI attributes from ITS quirks. */
>   static uint32_t __ro_after_init its_quirk_flags;
>   
>   static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
> @@ -148,9 +149,15 @@ static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
>       return flags;
>   }
>   
> -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> +static void gicv3_its_enable_quirks(void)
>   {
>       const struct its_quirk *quirk;
> +    const struct host_its *hw_its;
> +
> +    if ( list_empty(&host_its_list) )
> +        return;
> +
> +    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
>   
>       its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
>   
> @@ -603,16 +610,10 @@ static int gicv3_its_init_single_its(struct host_its *hw_its)
>       uint64_t reg;
>       int i, ret;
>   
> -    hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
> -    if ( !hw_its->its_base )
> -        return -ENOMEM;
> -
>       ret = gicv3_disable_its(hw_its);
>       if ( ret )
>           return ret;
>   
> -    gicv3_its_enable_quirks(hw_its);
> -
>       reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
>       hw_its->devid_bits = GITS_TYPER_DEVICE_ID_BITS(reg);
>       hw_its->evid_bits = GITS_TYPER_EVENT_ID_BITS(reg);
> @@ -1161,6 +1162,11 @@ static void add_to_host_its_list(paddr_t addr, paddr_t size,
>       its_data->size = size;
>       its_data->dt_node = node;
>   
> +    its_data->its_base = ioremap_nocache(its_data->addr, its_data->size);
> +    if ( !its_data->its_base )
> +        panic("GICv3: Cannot map ITS frame: 0x%lx, 0x%lx\n",
> +            its_data->addr, its_data->size);
> +
>       printk("GICv3: Found ITS @0x%lx\n", addr);
>   
>       list_add_tail(&its_data->entry, &host_its_list);
> @@ -1238,16 +1244,22 @@ static void gicv3_its_acpi_init(void)
>   
>   #endif
>   
> -int gicv3_its_init(void)
> +void __init gicv3_its_preinit(void)
>   {
> -    struct host_its *hw_its;
> -    int ret;
> -
>       if ( acpi_disabled )
>           gicv3_its_dt_init(dt_interrupt_controller);
>       else
>           gicv3_its_acpi_init();
>   
> +    gicv3_its_validate_quirks();
> +    gicv3_its_enable_quirks();
> +}
> +
> +int gicv3_its_init(void)
> +{
> +    struct host_its *hw_its;
> +    int ret;
> +
>       list_for_each_entry(hw_its, &host_its_list, entry)
>       {
>           ret = gicv3_its_init_single_its(hw_its);
> @@ -1255,8 +1267,6 @@ int gicv3_its_init(void)
>               return ret;
>       }
>   
> -    gicv3_its_validate_quirks();
> -
>       return 0;
>   }
>   
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index bc07f97c16..6e44d23d64 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1974,6 +1974,13 @@ static int __init gicv3_init(void)
>   
>       spin_lock(&gicv3.lock);
>   
> +    if ( gic_dist_supports_lpis() )
> +        /*
> +         * Apply ITS quirks before gicv3_dist_init() sets up host LPIs,
> +         * so pending tables use the correct ITS memory attributes.
> +         */
> +        gicv3_its_preinit();
> +
>       gicv3_dist_init();
>   
>       if ( gic_dist_supports_lpis() )
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h
> index fc5a84892c..e1d7522ea5 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
>   
>   /* Initialize the host structures for LPIs and the host ITSes. */
>   int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
> +void gicv3_its_preinit(void);
>   int gicv3_its_init(void);
>   
>   /* Store the physical address and ID for each redistributor as read from DT. */
> @@ -219,6 +220,10 @@ static inline int gicv3_its_deny_access(struct domain *d)
>       return 0;
>   }
>   
> +static inline void gicv3_its_preinit(void)
> +{
> +}
> +
>   static inline bool gicv3_its_host_has_its(void)
>   {
>       return false;

Re: [PATCH 4/4] xen/arm: its: pre-initialize ITS quirks before LPI setup

[Mykola Kvach]

Hi Oleksandr,

Thank you for the review.

On Tue, Apr 28, 2026 at 7:31 PM Oleksandr Tyshchenko
<olekstysh@gmail.com> wrote:
>
>
>
> On 3/25/26 12:38, Mykola Kvach wrote:
>
> Hello Mykola
>
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > In the current initialization flow, gicv3_init() calls gicv3_dist_init()
> > before gicv3_its_init().
> >
> > When LPIs are supported, gicv3_dist_init() calls
> > gicv3_lpi_init_host_lpis(), which initializes host LPI state and allocates
> > the boot CPU pending table before ITS quirk flags are computed. Non-boot
> > CPUs allocate their pending tables later from the CPU_UP_PREPARE notifier,
> > while redistributor LPI programming happens separately in
> > gicv3_lpi_init_rdist().
> >
> > This means the boot CPU LPI setup can observe default ITS memory attributes
> > before dma-noncoherent and other ITS quirks are applied.
> >
> > Introduce gicv3_its_preinit() and call it before gicv3_dist_init(). This
> > keeps the actual ITS hardware initialization in gicv3_its_init(), but moves
> > ITS discovery, quirk validation and quirk flag setup early enough for the
> > subsequent LPI initialization to use the correct attributes.
>
>
> Have you considered an alternative approach that might be less invasive?
> I am thinking of something the other way around: perhaps we could
> allocate the LPI pending table for the boot CPU later.
>
> Would a diff below work?
>
>
> ---
>   xen/arch/arm/gic-v3-lpi.c             | 9 +++++++++
>   xen/arch/arm/gic-v3.c                 | 2 ++
>   xen/arch/arm/include/asm/gic_v3_its.h | 6 ++++++
>   3 files changed, 17 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 9ee338edc2..61cc45d347 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -450,6 +450,15 @@ int gicv3_lpi_init_host_lpis(unsigned int
> host_lpi_bits)
>
>       printk("GICv3: using at most %lu LPIs on the host.\n",
> MAX_NR_HOST_LPIS);
>
> +    return rc;
> +}
> +
> +int gicv3_lpi_post_init_host_lpis(void)
> +{
> +    int rc;
> +
> +    ASSERT(smp_processor_id() == 0);
> +
>       /* Register the CPU notifier and allocate memory for the boot CPU */
>       register_cpu_notifier(&cpu_nfb);
>       rc = gicv3_lpi_allocate_pendtable(smp_processor_id());
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 7f365cdbe9..8b9059c5c9 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1981,6 +1981,8 @@ static int __init gicv3_init(void)
>           res = gicv3_its_init();
>           if ( res )
>               panic("GICv3: ITS: initialization failed: %d\n", res);
> +
> +        gicv3_lpi_post_init_host_lpis();
>       }
>
>       res = gicv3_cpu_init();
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h
> b/xen/arch/arm/include/asm/gic_v3_its.h
> index fc5a84892c..288cc1d42f 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
>
>   /* Initialize the host structures for LPIs and the host ITSes. */
>   int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
> +int gicv3_lpi_post_init_host_lpis(void);
>   int gicv3_its_init(void);
>
>   /* Store the physical address and ID for each redistributor as read
> from DT. */
> @@ -245,6 +246,11 @@ static inline int gicv3_lpi_init_host_lpis(unsigned
> int host_lpi_bits)
>       return 0;
>   }
>
> +static inline int gicv3_lpi_post_init_host_lpis(void)
> +{
> +    return 0;
> +}
> +
>   static inline int gicv3_its_init(void)
>   {
>       return 0;

Yes, I think this direction is better and less invasive.
Thank you for the suggestion.

Best regards,
Mykola

> --
> 2.34.1
>
>
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > TODO: Think about separating Redistributor/LPI attributes from ITS.
> > ---
> >   xen/arch/arm/gic-v3-its.c             | 36 +++++++++++++++++----------
> >   xen/arch/arm/gic-v3.c                 |  7 ++++++
> >   xen/arch/arm/include/asm/gic_v3_its.h |  5 ++++
> >   3 files changed, 35 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> > index ee432088cd..0251d91630 100644
> > --- a/xen/arch/arm/gic-v3-its.c
> > +++ b/xen/arch/arm/gic-v3-its.c
> > @@ -63,6 +63,7 @@ struct its_quirk {
> >       uint32_t flags;
> >   };
> >
> > +/* TODO: Separate Redistributor/LPI attributes from ITS quirks. */
> >   static uint32_t __ro_after_init its_quirk_flags;
> >
> >   static bool gicv3_its_match_quirk_gen4(const struct host_its *hw_its)
> > @@ -148,9 +149,15 @@ static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
> >       return flags;
> >   }
> >
> > -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> > +static void gicv3_its_enable_quirks(void)
> >   {
> >       const struct its_quirk *quirk;
> > +    const struct host_its *hw_its;
> > +
> > +    if ( list_empty(&host_its_list) )
> > +        return;
> > +
> > +    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
> >
> >       its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
> >
> > @@ -603,16 +610,10 @@ static int gicv3_its_init_single_its(struct host_its *hw_its)
> >       uint64_t reg;
> >       int i, ret;
> >
> > -    hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
> > -    if ( !hw_its->its_base )
> > -        return -ENOMEM;
> > -
> >       ret = gicv3_disable_its(hw_its);
> >       if ( ret )
> >           return ret;
> >
> > -    gicv3_its_enable_quirks(hw_its);
> > -
> >       reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
> >       hw_its->devid_bits = GITS_TYPER_DEVICE_ID_BITS(reg);
> >       hw_its->evid_bits = GITS_TYPER_EVENT_ID_BITS(reg);
> > @@ -1161,6 +1162,11 @@ static void add_to_host_its_list(paddr_t addr, paddr_t size,
> >       its_data->size = size;
> >       its_data->dt_node = node;
> >
> > +    its_data->its_base = ioremap_nocache(its_data->addr, its_data->size);
> > +    if ( !its_data->its_base )
> > +        panic("GICv3: Cannot map ITS frame: 0x%lx, 0x%lx\n",
> > +            its_data->addr, its_data->size);
> > +
> >       printk("GICv3: Found ITS @0x%lx\n", addr);
> >
> >       list_add_tail(&its_data->entry, &host_its_list);
> > @@ -1238,16 +1244,22 @@ static void gicv3_its_acpi_init(void)
> >
> >   #endif
> >
> > -int gicv3_its_init(void)
> > +void __init gicv3_its_preinit(void)
> >   {
> > -    struct host_its *hw_its;
> > -    int ret;
> > -
> >       if ( acpi_disabled )
> >           gicv3_its_dt_init(dt_interrupt_controller);
> >       else
> >           gicv3_its_acpi_init();
> >
> > +    gicv3_its_validate_quirks();
> > +    gicv3_its_enable_quirks();
> > +}
> > +
> > +int gicv3_its_init(void)
> > +{
> > +    struct host_its *hw_its;
> > +    int ret;
> > +
> >       list_for_each_entry(hw_its, &host_its_list, entry)
> >       {
> >           ret = gicv3_its_init_single_its(hw_its);
> > @@ -1255,8 +1267,6 @@ int gicv3_its_init(void)
> >               return ret;
> >       }
> >
> > -    gicv3_its_validate_quirks();
> > -
> >       return 0;
> >   }
> >
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index bc07f97c16..6e44d23d64 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -1974,6 +1974,13 @@ static int __init gicv3_init(void)
> >
> >       spin_lock(&gicv3.lock);
> >
> > +    if ( gic_dist_supports_lpis() )
> > +        /*
> > +         * Apply ITS quirks before gicv3_dist_init() sets up host LPIs,
> > +         * so pending tables use the correct ITS memory attributes.
> > +         */
> > +        gicv3_its_preinit();
> > +
> >       gicv3_dist_init();
> >
> >       if ( gic_dist_supports_lpis() )
> > diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h
> > index fc5a84892c..e1d7522ea5 100644
> > --- a/xen/arch/arm/include/asm/gic_v3_its.h
> > +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> > @@ -156,6 +156,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
> >
> >   /* Initialize the host structures for LPIs and the host ITSes. */
> >   int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
> > +void gicv3_its_preinit(void);
> >   int gicv3_its_init(void);
> >
> >   /* Store the physical address and ID for each redistributor as read from DT. */
> > @@ -219,6 +220,10 @@ static inline int gicv3_its_deny_access(struct domain *d)
> >       return 0;
> >   }
> >
> > +static inline void gicv3_its_preinit(void)
> > +{
> > +}
> > +
> >   static inline bool gicv3_its_host_has_its(void)
> >   {
> >       return false;
>

Ping: Re: [PATCH 0/4] xen/arm: ITS quirk handling fixes and board-specific matches

[Mykola Kvach]

Hi all,

Gentle ping on this series.

There are a few review comments that I can address in a v2, but one
point seems to need maintainer guidance before I rework the series.

In particular, there was a discussion around whether dma-noncoherent
should remain an additive property handled separately from the ITS quirk
table, or whether it should be modelled as part of the quirk matching
machinery, which would likely require accumulating flags from all
matching entries rather than keeping the current first-match semantics.

Could maintainers please share their preference here? I am happy to
prepare a v2 once the expected direction is clear.

Thanks,
Mykola

On Wed, Mar 25, 2026 at 12:41 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>
> From: Mykola Kvach <mykola_kvach@epam.com>
>
> This series cleans up ARM GICv3 ITS quirk handling and fixes the point at
> which quirk-derived memory attributes become visible to the LPI setup path.
>
> The first patch switches the quirk handling from per-entry init callbacks to
> declarative flags and folds in the DT dma-noncoherent property when deriving
> the effective ITS attributes.
>
> The second patch extends quirk matching with an optional platform callback so
> that boards sharing the same IIDR can still be distinguished reliably. This
> is then used by the third patch to add the Orange Pi 5 ITS quirk for
> RK3588/RK3588S boards.
>
> Finally, the last patch moves ITS quirk discovery and validation earlier in
> the boot flow, before host LPI tables are initialized, so the boot CPU does
> not allocate and program LPI data structures with default attributes when the
> platform requires different ones.
>
> Mykola Kvach (4):
>   xen/arm: its: collect quirk flags and honor dma-noncoherent
>   xen/arm: its: add platform match callback for ITS quirks
>   xen/arm: its: add Orange Pi 5 ITS quirk
>   xen/arm: its: pre-initialize ITS quirks before LPI setup
>
>  xen/arch/arm/gic-v3-its.c             | 129 +++++++++++++++++++-------
>  xen/arch/arm/gic-v3.c                 |   7 ++
>  xen/arch/arm/include/asm/gic_v3_its.h |   5 +
>  3 files changed, 107 insertions(+), 34 deletions(-)
>
> --
> 2.43.0
>