Re: [PATCH v3 3/4] xen/arm: its: refactor ITS quirk matching

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi,

Mykola Kvach <mykola_kvach@xxxxxxxx> writes:

> ITS quirks are matched only by IIDR and mask fields stored in each table
> entry. That is too coarse when the same GIC IP block appears on several
> platforms but a workaround is valid only for some of them.
>
> Replace the fixed IIDR fields with a generic match(hw_its, data)
> callback and an opaque data pointer. Add an IIDR matcher as a reusable
> building block and use it from the R-Car Gen4 matcher after checking the
> Renesas machine compatibles.
>
> This intentionally narrows the R-Car Gen4 quirk. Previously every ITS
> with IIDR 0x0201743b matched. Now it matches only a DT-discovered ITS on
> an r8a779f0 or r8a779g0 machine. ACPI-discovered ITSes and the same IIDR
> on other platforms no longer match.
>
> Keep first-match semantics explicit. Assert that non-sentinel entries
> provide a matcher and that IIDR matching receives match data. Retain
> runtime guards so malformed entries cannot cause a NULL function call
> or data dereference in non-debug builds. Place the matcher data and
> table in init-only read-only sections.
>
> The matched entry still supplies separate ITS and LPI flags; this patch
> only changes how the entry is selected.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
> Changes in v3:
> - Document the intentional narrowing of the R-Car Gen4 match.
> - Add the non-debug NULL-data guard.
> - Put the matcher data and table in init-only read-only sections.
>
> Changes in v2:
> - Replace v1's optional platform callback plus fixed IIDR/mask fields
>   with a single generic match(hw_its, data) selector.
> - Add a reusable IIDR matcher and use it after the R-Car Gen4
>   machine-compatible checks.
> - Document that the R-Car Gen4 quirk remains DT-only.
> - Keep the split ITS and host LPI quirk scopes when applying the matched
>   entry.
> - Document first-match ordering in the lookup path and guard against
>   entries without a match callback or IIDR match data.
> ---
>  xen/arch/arm/gic-v3-its.c | 73 +++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 454fd0e0ef..f52232ca40 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -54,8 +54,8 @@ struct its_device {
>  
>  struct its_quirk {
>      const char *desc;
> -    uint32_t iidr;
> -    uint32_t mask;
> +    bool (*match)(const struct host_its *hw_its, const void *data);
> +    const void *data;
>      uint32_t its_flags;
>      /*
>       * lpi_flags are ORed into the global host LPI policy and must only
> @@ -65,11 +65,52 @@ struct its_quirk {
>      uint32_t lpi_flags;
>  };
>  
> -static const struct its_quirk its_quirks[] = {
> +struct its_quirk_match_iidr {
> +    uint32_t iidr;
> +    uint32_t mask;
> +};
> +
> +static bool __init gicv3_its_match_iidr(const struct host_its *hw_its,
> +                                        const void *data)
> +{
> +    const struct its_quirk_match_iidr *match;
> +    uint32_t iidr;
> +
> +    ASSERT(data);
> +
> +    if ( !data )
> +        return false;

It looks strange to have both ASSERT and then this check.

Maybe it is worth to remove ASSERT and print a warning in a debug build instead?

> +
> +    match = data;
> +    iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
> +
> +    return (iidr & match->mask) == match->iidr;
> +}
> +
> +static bool __init gicv3_its_match_quirk_gen4(const struct host_its *hw_its,
> +                                              const void *data)
> +{
> +    if ( !hw_its->dt_node )
> +        return false;
> +
> +    if ( !dt_machine_is_compatible("renesas,r8a779f0") &&
> +         !dt_machine_is_compatible("renesas,r8a779g0") )
> +        return false;
> +
> +    return gicv3_its_match_iidr(hw_its, data);

Do you really need to match IIDR in this case? You already know that
platform requires the quirk.

> +}
> +
> +static const struct its_quirk_match_iidr rcar_gen4_iidr __initconst = {
> +    /* Implementer 0x43b identifies Arm Ltd. */
> +    .iidr = 0x0201743b,
> +    .mask = 0xffffffffU,
> +};
> +
> +static const struct its_quirk its_quirks[] __initconstrel = {
>      {
> -        .desc        = "R-Car Gen4",
> -        .iidr        = 0x0201743b,
> -        .mask        = 0xffffffffU,
> +        .desc = "R-Car Gen4",
> +        .match = gicv3_its_match_quirk_gen4,
> +        .data = &rcar_gen4_iidr,
>          .its_flags = GICV3_QUIRK_MEM_NC_NS | GICV3_QUIRK_MEM_32BIT_ADDR,
>          .lpi_flags = GICV3_QUIRK_MEM_NC_NS | GICV3_QUIRK_MEM_32BIT_ADDR,
>      },
> @@ -78,18 +119,21 @@ static const struct its_quirk its_quirks[] = {
>      }
>  };
>  
> -static const struct its_quirk *__init gicv3_its_find_quirk(uint32_t iidr)
> +static const struct its_quirk *__init gicv3_its_find_quirk(
> +    const struct host_its *hw_its)
>  {
> -    const struct its_quirk *quirks = its_quirks;
> +    const struct its_quirk *quirk;

Is this change really required?

>  
>      /*
> -     * The first matching quirk wins. More specific quirks must be listed
> -     * before broader IIDR-only entries.
> +     * The first matching quirk wins. Entries that match a specific platform
> +     * must be listed before broader IIDR-only entries.
>       */
> -    for ( ; quirks->desc; quirks++ )
> +    for ( quirk = its_quirks; quirk->desc; quirk++ )
>      {
> -        if ( quirks->iidr == (quirks->mask & iidr) )
> -            return quirks;
> +        ASSERT(quirk->match);
> +
> +        if ( quirk->match && quirk->match(hw_its, quirk->data) )
> +            return quirk;
>      }
>  
>      return NULL;
> @@ -97,8 +141,7 @@ static const struct its_quirk *__init 
> gicv3_its_find_quirk(uint32_t iidr)
>  
>  static void __init gicv3_its_collect_quirks(struct host_its *hw_its)
>  {
> -    uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
> -    const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
> +    const struct its_quirk *quirk = gicv3_its_find_quirk(hw_its);
>  
>      if ( quirk )
>      {

-- 
WBR, Volodymyr