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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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