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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykola,

Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:

> 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]?

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