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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Volodymyr,

Thank you for the review.

On Wed, Mar 25, 2026 at 4:42 PM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
>
> Hi Mykola,
>
> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> > ---
> >  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