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

Mykola Kvach posted 4 patches 1 week, 1 day ago
[PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent
Posted by Mykola Kvach 1 week, 1 day ago
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
Posted by Volodymyr Babchuk 1 week, 1 day ago
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
Posted by Mykola Kvach 1 week, 1 day ago
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
Posted by Volodymyr Babchuk 3 days, 1 hour ago
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