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

On 28-May-26 02:25, Mykola Kvach wrote:
> From: Mykola Kvach <[email protected]>
> 
> ITS quirks are currently matched only by IIDR and mask fields stored in
> each table entry. That is too coarse for integrations where the same GIC
> IP block can appear in several platforms but the workaround is only valid
> for a subset of boards.
> 
> 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. The R-Car Gen4 platform refinement is DT-only;
> ACPI-discovered ITSes do not match it.
> 
> Keep first-match semantics explicit. Assert that non-sentinel entries
> provide a matcher and that IIDR matching receives match data, but keep
> runtime guards so a malformed table entry does not become a NULL function
> call or NULL data dereference in non-debug builds. The matched entry still
> supplies separate ITS and LPI flags; this patch only changes how the entry
> is selected.
> 
> Signed-off-by: Mykola Kvach <[email protected]>
> ---
> 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 | 67 +++++++++++++++++++++++++++++++--------
>  1 file changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index dc48a84789..e055914763 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -53,8 +53,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
> @@ -64,11 +64,48 @@ struct its_quirk {
>      uint32_t lpi_flags;
>  };
>  
> +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);
> +
> +    match = data;
> +    iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
> +
> +    return (iidr & match->mask) == match->iidr;
The commit message says you keep a runtime guard so malformed table data
does not become a NULL data dereference in non-debug builds, but there is
no such guard - ASSERT() compiles out and match->mask is then read from
NULL.

> +}
> +
> +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") )
Given that IIDR 0x0201743b is not Renesas-specific as you mention in the cover
letter, this is a behavior changed and should be mentioned in the commit msg
(cover letter is not in git).

> +        return false;
> +
> +    return gicv3_its_match_iidr(hw_its, data);
> +}
> +
> +static const struct its_quirk_match_iidr rcar_gen4_iidr = {
__initconst

> +    .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,
>      },
> @@ -77,18 +114,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;
>  
>      /*
> -     * 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;
> @@ -96,8 +136,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 )
>      {

~Michal