Re: [PATCH v3 2/4] xen/arm: its: separate ITS and host LPI quirk scopes

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykola,

Mykola Kvach <mykola_kvach@xxxxxxxx> writes:

> ITS quirks can impose restrictions on memory accessed by the ITS itself
> and on shared host LPI/Redistributor state. These scopes are not
> identical, so a single global ITS quirk state makes the host LPI policy
> depend implicitly on the quirks seen while initializing host ITSes.
>
> Add per-ITS quirk_flags to struct host_its and keep a separate
> host_lpi_flags state in the LPI code. The quirk table now records the
> ITS-private and host LPI scopes explicitly through its_flags and
> lpi_flags. The R-Car Gen4 quirk applies the same memory restrictions to
> both scopes, preserving the existing R-Car memory policy without relying
> on an implicit aggregation step.
>
> This also removes the assumption that all host ITSes must expose the
> same quirk state. ITS-derived host LPI restrictions are accumulated only
> from quirk entries that explicitly set lpi_flags.
>
> Use per-ITS quirk_flags for GITS_CBASER, GITS_BASER<n>, and ITT
> allocations. Use host_lpi_flags when allocating the property and pending
> tables and when programming GICR_PROPBASER and GICR_PENDBASER.
> Memory-related quirk bits are named GICV3_QUIRK_MEM_* and are translated
> by shared gicv3_mem_get_*() helpers.
>
> Collect both scopes during the ITS preparation pass, before host LPI
> state is allocated. Keep init-only annotations and includes in the
> implementation files rather than the public header.
>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
> Changes in v3:
> - Collect quirk flags during the ITS preparation pass added earlier.
> - Keep init-only annotations and includes in implementation files.
>
> Changes in v2:
> - Replace v1's single global ITS quirk flags and same-quirk validation
>   with explicit per-ITS and host LPI quirk scopes.
> - Drop the v1 ITS pre-initialization approach from this patch. In v2,
>   host LPI allocation was moved after ITS initialization by a separate
>   patch.
> - Drop DT dma-noncoherent handling from this patch; firmware-provided
>   non-coherency is handled separately with explicit ITS-node and
>   GIC-node scopes.
> - Keep host_lpi_flags owned by gic-v3-lpi.c and update it only with quirk
>   flags that explicitly apply to host LPI/Redistributor state.
> - Rename the current memory-related bits to GICV3_QUIRK_MEM_* and use
>   shared gicv3_mem_get_*() helpers for register attributes and allocation
>   flags.
> ---
>  xen/arch/arm/gic-v3-its.c             | 111 ++++++++++++--------------
>  xen/arch/arm/gic-v3-lpi.c             |  45 +++++++++--
>  xen/arch/arm/include/asm/gic_v3_its.h |  18 +++--
>  3 files changed, 100 insertions(+), 74 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 972825bf06..454fd0e0ef 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -52,43 +52,40 @@ struct its_device {
>      struct pending_irq *pend_irqs;      /* One struct per event */
>  };
>  
> -/*
> - * It is unlikely that a platform implements ITSes with different quirks,
> - * so assume they all share the same.
> - */
>  struct its_quirk {
>      const char *desc;
> -    bool (*init)(struct host_its *hw_its);
>      uint32_t iidr;
>      uint32_t mask;
> +    uint32_t its_flags;
> +    /*
> +     * lpi_flags are ORed into the global host LPI policy and must only
> +     * contain additive restrictions. Non-additive LPI quirks need explicit
> +     * handling.
> +     */
> +    uint32_t lpi_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,
> +        .its_flags = GICV3_QUIRK_MEM_NC_NS | GICV3_QUIRK_MEM_32BIT_ADDR,
> +        .lpi_flags = GICV3_QUIRK_MEM_NC_NS | GICV3_QUIRK_MEM_32BIT_ADDR,
>      },
>      {
>          /* Sentinel. */
>      }
>  };
>  
> -static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
> +static const struct its_quirk *__init gicv3_its_find_quirk(uint32_t iidr)
>  {
>      const struct its_quirk *quirks = its_quirks;
>  
> +    /*
> +     * The first matching quirk wins. More specific quirks must be listed
> +     * before broader IIDR-only entries.
> +     */
>      for ( ; quirks->desc; quirks++ )
>      {
>          if ( quirks->iidr == (quirks->mask & iidr) )
> @@ -98,53 +95,38 @@ static const struct its_quirk 
> *gicv3_its_find_quirk(uint32_t iidr)
>      return NULL;
>  }
>  
> -static void gicv3_its_enable_quirks(struct host_its *hw_its)
> +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);
>  
> -    if ( quirk && quirk->init(hw_its) )
> -        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;
> -    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));
> -
> -    list_for_each_entry(hw_its, &host_its_list, entry)
> +    if ( quirk )
>      {
> -        quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + 
> GITS_IIDR));
> -        BUG_ON(quirk != prev);
> -        prev = quirk;
> +        hw_its->quirk_flags |= quirk->its_flags;
> +        gicv3_lpi_update_host_flags(quirk->lpi_flags);
> +        printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
>      }
>  }
>  
> -uint64_t gicv3_its_get_cacheability(void)
> +uint64_t gicv3_mem_get_cacheability(uint32_t flags)
>  {
> -    if ( its_quirk_flags & HOST_ITS_WORKAROUND_NC_NS )
> +    if ( flags & GICV3_QUIRK_MEM_NC_NS )
>          return GIC_BASER_CACHE_nC;
>  
>      return GIC_BASER_CACHE_RaWaWb;
>  }
>  
> -uint64_t gicv3_its_get_shareability(void)
> +uint64_t gicv3_mem_get_shareability(uint32_t flags)
>  {
> -    if ( its_quirk_flags & HOST_ITS_WORKAROUND_NC_NS )
> +    if ( flags & GICV3_QUIRK_MEM_NC_NS )
>          return GIC_BASER_NonShareable;
>  
>      return GIC_BASER_InnerShareable;
>  }
>  
> -unsigned int gicv3_its_get_memflags(void)
> +unsigned int gicv3_mem_get_alloc_flags(uint32_t flags)
>  {
> -    if ( its_quirk_flags & HOST_ITS_WORKAROUND_32BIT_ADDR )
> +    if ( flags & GICV3_QUIRK_MEM_32BIT_ADDR )
>          return MEMF_bits(32);
>  
>      return 0;
> @@ -391,13 +373,17 @@ static void *its_map_cbaser(struct host_its *its)
>      uint64_t reg;
>      unsigned int order;
>      void *buffer;
> +    uint64_t cacheability = gicv3_mem_get_cacheability(its->quirk_flags);
> +    uint64_t shareability = gicv3_mem_get_shareability(its->quirk_flags);
> +    unsigned int memflags = gicv3_mem_get_alloc_flags(its->quirk_flags);
>  
> -    reg  = gicv3_its_get_shareability() << GITS_BASER_SHAREABILITY_SHIFT;
> -    reg |= GIC_BASER_CACHE_SameAsInner << 
> GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> -    reg |= gicv3_its_get_cacheability() << 
> GITS_BASER_INNER_CACHEABILITY_SHIFT;
> +    reg  = MASK_INSR(shareability, GITS_BASER_SHAREABILITY_MASK);
> +    reg |= MASK_INSR(GIC_BASER_CACHE_SameAsInner,
> +                     GITS_BASER_OUTER_CACHEABILITY_MASK);
> +    reg |= MASK_INSR(cacheability, GITS_BASER_INNER_CACHEABILITY_MASK);
>  
>      order = get_order_from_bytes(max(ITS_CMD_QUEUE_SZ, SZ_64K));
> -    buffer = alloc_xenheap_pages(order, gicv3_its_get_memflags());
> +    buffer = alloc_xenheap_pages(order, memflags);
>      if ( !buffer )
>          return NULL;
>  
> @@ -438,8 +424,8 @@ static void *its_map_cbaser(struct host_its *its)
>  /* The ITS BASE registers work with page sizes of 4K, 16K or 64K. */
>  #define BASER_PAGE_BITS(sz) ((sz) * 2 + 12)
>  
> -static int its_map_baser(void __iomem *basereg, uint64_t regc,
> -                         unsigned int nr_items)
> +static int its_map_baser(struct host_its *its, void __iomem *basereg,
> +                         uint64_t regc, unsigned int nr_items)
>  {
>      uint64_t attr, reg;
>      unsigned int entry_size = GITS_BASER_ENTRY_SIZE(regc);
> @@ -447,10 +433,14 @@ static int its_map_baser(void __iomem *basereg, 
> uint64_t regc,
>      unsigned int table_size;
>      unsigned int order;
>      void *buffer;
> +    uint64_t cacheability = gicv3_mem_get_cacheability(its->quirk_flags);
> +    uint64_t shareability = gicv3_mem_get_shareability(its->quirk_flags);
> +    unsigned int memflags = gicv3_mem_get_alloc_flags(its->quirk_flags);
>  
> -    attr  = gicv3_its_get_shareability() << GITS_BASER_SHAREABILITY_SHIFT;
> -    attr |= GIC_BASER_CACHE_SameAsInner << 
> GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> -    attr |= gicv3_its_get_cacheability() << 
> GITS_BASER_INNER_CACHEABILITY_SHIFT;
> +    attr  = MASK_INSR(shareability, GITS_BASER_SHAREABILITY_MASK);
> +    attr |= MASK_INSR(GIC_BASER_CACHE_SameAsInner,
> +                      GITS_BASER_OUTER_CACHEABILITY_MASK);
> +    attr |= MASK_INSR(cacheability, GITS_BASER_INNER_CACHEABILITY_MASK);
>  
>      /*
>       * Setup the BASE register with the attributes that we like. Then read
> @@ -464,7 +454,7 @@ retry:
>      table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
>  
>      order = get_order_from_bytes(max(table_size, 
> BIT(BASER_PAGE_BITS(pagesz), U)));
> -    buffer = alloc_xenheap_pages(order, gicv3_its_get_memflags());
> +    buffer = alloc_xenheap_pages(order, memflags);
>      if ( !buffer )
>          return -ENOMEM;
>  
> @@ -562,7 +552,7 @@ static int __init gicv3_its_prepare_single_its(struct 
> host_its *hw_its)
>      if ( ret )
>          return ret;
>  
> -    gicv3_its_enable_quirks(hw_its);
> +    gicv3_its_collect_quirks(hw_its);
>  
>      return 0;
>  }
> @@ -592,18 +582,19 @@ static int __init gicv3_its_init_single_its(struct 
> host_its *hw_its)
>          case GITS_BASER_TYPE_NONE:
>              continue;
>          case GITS_BASER_TYPE_DEVICE:
> -            ret = its_map_baser(basereg, reg, BIT(hw_its->devid_bits, UL));
> +            ret = its_map_baser(hw_its, basereg, reg,
> +                                BIT(hw_its->devid_bits, UL));

You are passing hw_its now, so there is no need to pass the last
argument, because its_map_baser() can calculate it by itself.

>              if ( ret )
>                  return ret;
>              break;
>          case GITS_BASER_TYPE_COLLECTION:
> -            ret = its_map_baser(basereg, reg, num_possible_cpus());
> +            ret = its_map_baser(hw_its, basereg, reg, num_possible_cpus());
>              if ( ret )
>                  return ret;
>              break;
>          /* In case this is a GICv4, provide a (dummy) vPE table as well. */
>          case GITS_BASER_TYPE_VCPU:
> -            ret = its_map_baser(basereg, reg, 1);
> +            ret = its_map_baser(hw_its, basereg, reg, 1);
>              if ( ret )
>                  return ret;
>              break;
> @@ -738,6 +729,7 @@ int gicv3_its_map_guest_device(struct domain *d,
>      struct its_device *dev = NULL;
>      struct rb_node **new = &d->arch.vgic.its_devices.rb_node, *parent = NULL;
>      int i, ret = -ENOENT;      /* "i" must be signed to check for >= 0 
> below. */
> +    unsigned int memflags;
>      unsigned int order;
>  
>      hw_its = gicv3_its_find_by_doorbell(host_doorbell);
> @@ -801,8 +793,9 @@ int gicv3_its_map_guest_device(struct domain *d,
>      ret = -ENOMEM;
>  
>      /* An Interrupt Translation Table needs to be 256-byte aligned. */
> +    memflags = gicv3_mem_get_alloc_flags(hw_its->quirk_flags);
>      order = get_order_from_bytes(max(nr_events * hw_its->itte_size, 256UL));
> -    itt_addr = alloc_xenheap_pages(order, gicv3_its_get_memflags());
> +    itt_addr = alloc_xenheap_pages(order, memflags);
>      if ( !itt_addr )
>          goto out_unlock;
>  
> @@ -1214,8 +1207,6 @@ int __init gicv3_its_init(unsigned int host_lpi_bits)
>              return ret;
>      }
>  
> -    gicv3_its_validate_quirks();
> -
>      if ( list_empty(&host_its_list) )
>          return 0;
>  
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 9ee338edc2..accfd48a74 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <xen/cpu.h>
> +#include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/mm.h>
>  #include <xen/param.h>
> @@ -78,9 +79,29 @@ struct lpi_redist_data {
>  
>  static DEFINE_PER_CPU(struct lpi_redist_data, lpi_redist);
>  
> +/*
> + * Host LPI flags are scoped to shared LPI/Redistributor state, not to an
> + * ITS instance. Arm IHI 0069H.b section 5.1.1 says "LPI configuration is
> + * global". Section 12.11.33 (GICR_PROPBASER) makes mismatched values
> + * UNPREDICTABLE for Redistributors that share an LPI Configuration table
> + * while GICR_CTLR.EnableLPIs == 1. Section 12.11.32 (GICR_PENDBASER)
> + * similarly makes mismatched OuterCache, Shareability or InnerCache values
> + * across Redistributors UNPREDICTABLE while GICR_CTLR.EnableLPIs == 1.
> + *
> + * Keep this policy in the LPI code and accumulate only explicit LPI/RD
> + * restrictions into it. Per-ITS restrictions stay in host_its::quirk_flags
> + * for GITS_CBASER, GITS_BASER<n> and ITT memory.
> + */
> +static uint32_t __ro_after_init host_lpi_flags;
> +
>  #define MAX_NR_HOST_LPIS   (lpi_data.max_host_lpi_ids - LPI_OFFSET)
>  #define HOST_LPIS_PER_PAGE      (PAGE_SIZE / sizeof(union host_lpi))
>  
> +void __init gicv3_lpi_update_host_flags(uint32_t flags)
> +{
> +    host_lpi_flags |= flags;
> +}
> +
>  static union host_lpi *gic_get_host_lpi(uint32_t plpi)
>  {
>      union host_lpi *block;
> @@ -228,6 +249,7 @@ static int gicv3_lpi_allocate_pendtable(unsigned int cpu)
>  {
>      void *pendtable;
>      unsigned int order;
> +    unsigned int memflags = gicv3_mem_get_alloc_flags(host_lpi_flags);
>  
>      if ( per_cpu(lpi_redist, cpu).pending_table )
>          return -EBUSY;
> @@ -239,7 +261,7 @@ static int gicv3_lpi_allocate_pendtable(unsigned int cpu)
>       * physically contiguous memory.
>       */
>      order = get_order_from_bytes(max(lpi_data.max_host_lpi_ids / 8, 
> (unsigned long)SZ_64K));
> -    pendtable = alloc_xenheap_pages(order, gicv3_its_get_memflags());
> +    pendtable = alloc_xenheap_pages(order, memflags);
>      if ( !pendtable )
>          return -ENOMEM;
>  
> @@ -262,6 +284,8 @@ static int gicv3_lpi_set_pendtable(void __iomem 
> *rdist_base)
>  {
>      const void *pendtable = this_cpu(lpi_redist).pending_table;
>      uint64_t val;
> +    uint64_t cacheability = gicv3_mem_get_cacheability(host_lpi_flags);
> +    uint64_t shareability = gicv3_mem_get_shareability(host_lpi_flags);
>  
>      /*
>       * The memory should have been allocated while preparing the CPU (or
> @@ -275,9 +299,10 @@ static int gicv3_lpi_set_pendtable(void __iomem 
> *rdist_base)
>  
>      ASSERT(!(virt_to_maddr(pendtable) & ~GENMASK(51, 16)));
>  
> -    val  = gicv3_its_get_cacheability() << 
> GICR_PENDBASER_INNER_CACHEABILITY_SHIFT;
> -    val |= GIC_BASER_CACHE_SameAsInner << 
> GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT;
> -    val |= gicv3_its_get_shareability() << GICR_PENDBASER_SHAREABILITY_SHIFT;
> +    val  = MASK_INSR(cacheability, GICR_PENDBASER_INNER_CACHEABILITY_MASK);
> +    val |= MASK_INSR(GIC_BASER_CACHE_SameAsInner,
> +                     GICR_PENDBASER_OUTER_CACHEABILITY_MASK);
> +    val |= MASK_INSR(shareability, GICR_PENDBASER_SHAREABILITY_MASK);
>      val |= GICR_PENDBASER_PTZ;
>      val |= virt_to_maddr(pendtable);
>  
> @@ -304,10 +329,13 @@ static int gicv3_lpi_set_proptable(void __iomem * 
> rdist_base)
>  {
>      uint64_t reg;
>      unsigned int order;
> +    uint64_t cacheability = gicv3_mem_get_cacheability(host_lpi_flags);
> +    uint64_t shareability = gicv3_mem_get_shareability(host_lpi_flags);
>  
> -    reg  = gicv3_its_get_cacheability() << 
> GICR_PROPBASER_INNER_CACHEABILITY_SHIFT;
> -    reg |= GIC_BASER_CACHE_SameAsInner << 
> GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT;
> -    reg |= gicv3_its_get_shareability() << GICR_PROPBASER_SHAREABILITY_SHIFT;
> +    reg  = MASK_INSR(cacheability, GICR_PROPBASER_INNER_CACHEABILITY_MASK);
> +    reg |= MASK_INSR(GIC_BASER_CACHE_SameAsInner,
> +                     GICR_PROPBASER_OUTER_CACHEABILITY_MASK);
> +    reg |= MASK_INSR(shareability, GICR_PROPBASER_SHAREABILITY_MASK);
>  
>      /*
>       * The property table is shared across all redistributors, so allocate
> @@ -317,9 +345,10 @@ static int gicv3_lpi_set_proptable(void __iomem * 
> rdist_base)
>      {
>          /* The property table holds one byte per LPI. */
>          void *table;
> +        unsigned int memflags = gicv3_mem_get_alloc_flags(host_lpi_flags);
>  
>          order = get_order_from_bytes(max(lpi_data.max_host_lpi_ids, 
> (unsigned long)SZ_4K));
> -        table = alloc_xenheap_pages(order, gicv3_its_get_memflags());
> +        table = alloc_xenheap_pages(order, memflags);
>  
>          if ( !table )
>              return -ENOMEM;
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h 
> b/xen/arch/arm/include/asm/gic_v3_its.h
> index e1322b94f8..9230705181 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -110,8 +110,9 @@
>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>  #define HOST_ITS_USES_PTA               (1U << 1)
>  
> -#define HOST_ITS_WORKAROUND_NC_NS       (1U << 0)
> -#define HOST_ITS_WORKAROUND_32BIT_ADDR  (1U << 1)
> +/* GICv3 memory-related quirk flags. */
> +#define GICV3_QUIRK_MEM_NC_NS           (1U << 0)
> +#define GICV3_QUIRK_MEM_32BIT_ADDR      (1U << 1)
>  
>  /* We allocate LPIs on the hosts in chunks of 32 to reduce handling 
> overhead. */
>  #define LPI_BLOCK                       32U
> @@ -128,6 +129,11 @@ struct host_its {
>      unsigned int itte_size;
>      spinlock_t cmd_lock;
>      void *cmd_buf;
> +    /*
> +     * Workaround flags scoped to this ITS instance, including memory
> +     * accessed through GITS_CBASER, GITS_BASER<n> and ITT memory.
> +     */
> +    uint32_t quirk_flags;
>      unsigned int flags;
>  };
>  
> @@ -157,6 +163,7 @@ int gicv3_lpi_init_rdist(void __iomem * rdist_base);
>  /* Initialize the host structures for LPIs and the host ITSes. */
>  int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits);
>  int gicv3_its_init(unsigned int host_lpi_bits);
> +void gicv3_lpi_update_host_flags(uint32_t flags);
>  
>  /* Store the physical address and ID for each redistributor as read from DT. 
> */
>  void gicv3_set_redist_address(paddr_t address, unsigned int redist_id);
> @@ -199,10 +206,9 @@ struct pending_irq *gicv3_assign_guest_event(struct 
> domain *d,
>  void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id,
>                                   uint32_t virt_lpi);
>  
> -/* ITS quirks handling. */

Maybe leave the comment? Just change "ITS" to "ITS/LPI" ?

> -uint64_t gicv3_its_get_cacheability(void);
> -uint64_t gicv3_its_get_shareability(void);
> -unsigned int gicv3_its_get_memflags(void);
> +uint64_t gicv3_mem_get_cacheability(uint32_t flags);
> +uint64_t gicv3_mem_get_shareability(uint32_t flags);
> +unsigned int gicv3_mem_get_alloc_flags(uint32_t flags);
>  
>  #else

-- 
WBR, Volodymyr