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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Oleksandr,

Thank you for the review.

On Tue, Apr 28, 2026 at 9:42 PM Oleksandr Tyshchenko
<olekstysh@xxxxxxxxx> 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@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.
> >
> >
>
> 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]