Re: [RFC PATCH v2 06/26] ARM: GICv3 ITS: introduce device mapping

[Stefano Stabellini <sstabellini@kernel.org>]

On Thu, 22 Dec 2016, Andre Przywara wrote:
> The ITS uses device IDs to map LPIs to a device. Dom0 will later use
> those IDs, which we directly pass on to the host.
> For this we have to map each device that Dom0 may request to a host
> ITS device with the same identifier.
> Allocate the respective memory and enter each device into a list to
> later be able to iterate over it or to easily teardown guests.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic-its.c        | 118 ++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic.c           |   3 ++
>  xen/include/asm-arm/domain.h  |   2 +
>  xen/include/asm-arm/gic-its.h |  22 ++++++++
>  4 files changed, 145 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
> index d0f5fd1..e157c6b 100644
> --- a/xen/arch/arm/gic-its.c
> +++ b/xen/arch/arm/gic-its.c
> @@ -21,6 +21,7 @@
>  #include <xen/err.h>
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
> +#include <xen/sched.h>
>  #include <xen/sizes.h>
>  #include <asm/p2m.h>
>  #include <asm/io.h>
> @@ -108,6 +109,21 @@ static int its_send_cmd_mapc(struct host_its *its, int collection_id, int cpu)
>      return its_send_command(its, cmd);
>  }
>  
> +static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
> +                             int size, uint64_t itt_addr, bool valid)
> +{
> +    uint64_t cmd[4];
> +
> +    cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
> +    cmd[1] = size & GENMASK(4, 0);
> +    cmd[2] = itt_addr & GENMASK(51, 8);
> +    if ( valid )
> +        cmd[2] |= BIT_ULL(63);
> +    cmd[3] = 0x00;
> +
> +    return its_send_command(its, cmd);
> +}
> +
>  /* Set up the (1:1) collection mapping for the given host CPU. */
>  void gicv3_its_setup_collection(int cpu)
>  {
> @@ -237,6 +253,7 @@ int gicv3_its_init(struct host_its *hw_its)
>  
>      reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
>      hw_its->pta = !!(reg & GITS_TYPER_PTA);
> +    hw_its->itte_size = ((reg >> 4) & 0xf) + 1;

Please #define all numbers


>      for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
>      {
> @@ -358,6 +375,107 @@ int gicv3_lpi_init_host_lpis(unsigned int hw_lpi_bits)
>      return 0;
>  }
>  
> +static void remove_mapped_guest_device(struct its_devices *dev)
> +{
> +    if ( dev->hw_its )
> +        its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, false);
> +
> +    xfree(dev->itt_addr);
> +    xfree(dev);
> +}
> +
> +int gicv3_its_map_device(struct domain *d, int host_devid, int guest_devid,
> +                         int bits, bool valid)
> +{
> +    void *itt_addr = NULL;
> +    struct its_devices *dev, *temp;
> +    struct host_its *hw_its;
> +    int ret;
> +
> +    /* check for already existing mappings */
> +    spin_lock(&d->arch.vgic.its_devices_lock);
> +    list_for_each_entry_safe(dev, temp, &d->arch.vgic.its_devices, entry)
> +    {
> +        if ( dev->guest_devid != guest_devid )
> +            continue;
> +
> +        if ( !valid )
> +            list_del(&dev->entry);
> +
> +        spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +        if ( valid )
> +            return -EBUSY;
> +
> +        remove_mapped_guest_device(dev);
> +
> +        return 0;
> +    }
> +    spin_unlock(&d->arch.vgic.its_devices_lock);

Compared to the previous version, now the list is per-domain, which is
better for DomUs, but not for Dom0. In the case of Dom0 the number of
iterations will be the same.

I suggest we move to a different data structure, such as an hashtable or
an rbtree. If devids were guaranteed to be dense, then we could store
struct its_devices pointers in an array and direct access them, but I
don't think they are?

If devids are sparse, using a per-domain structure could be a good idea,
but we still need to prevent a device from being assigned to two domains
simultaneously. Is there a check to avoid that?


> +    if ( !valid )
> +        return -ENOENT;
> +
> +    /* TODO: Work out the correct hardware ITS to use here.
> +     * Maybe a per-platform function: devid -> ITS?
> +     * Or parsing the DT to find the msi_parent?
> +     * Or get Dom0 to give us this information?
> +     * For now just use the first ITS.
> +     */
> +    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
> +
> +    itt_addr = _xmalloc(BIT(bits) * hw_its->itte_size, 256);
> +    if ( !itt_addr )
> +        return -ENOMEM;
> +
> +    dev = xmalloc(struct its_devices);
> +    if ( !dev )
> +    {
> +        xfree(itt_addr);
> +        return -ENOMEM;
> +    }
> +
> +    ret = its_send_cmd_mapd(hw_its, host_devid, bits - 1,
> +                            virt_to_maddr(itt_addr), true);
> +    if (ret) {
> +        xfree(itt_addr);
> +        xfree(dev);
> +        return ret;
> +    }
> +
> +    dev->itt_addr = itt_addr;
> +    dev->hw_its = hw_its;
> +    dev->guest_devid = guest_devid;
> +    dev->host_devid = host_devid;
> +    dev->eventids = BIT(bits);
> +
> +    spin_lock(&d->arch.vgic.its_devices_lock);
> +    list_add_tail(&dev->entry, &d->arch.vgic.its_devices);
> +    spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +    return 0;
> +}
> +
> +/* Removing any connections a domain had to any ITS in the system. */
> +int its_remove_domain(struct domain *d)
> +{
> +    struct its_devices *dev, *temp;
> +
> +retry:
> +
> +    spin_lock(&d->arch.vgic.its_devices_lock);
> +    list_for_each_entry_safe(dev, temp, &d->arch.vgic.its_devices, entry)
> +    {
> +        list_del(&dev->entry);
> +        spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +        remove_mapped_guest_device(dev);
> +        goto retry;
> +    }
> +
> +    return 0;
> +}
> +
>  /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
>  void gicv3_its_dt_init(const struct dt_device_node *node)
>  {
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 364d5f0..de77aaa 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -156,6 +156,9 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>      for ( i = 0; i < NR_GIC_SGI; i++ )
>          set_bit(i, d->arch.vgic.allocated_irqs);
>  
> +    spin_lock_init(&d->arch.vgic.its_devices_lock);
> +    INIT_LIST_HEAD(&d->arch.vgic.its_devices);
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 2d6fbb1..8ccc32a 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -109,6 +109,8 @@ struct arch_domain
>          } *rdist_regions;
>          int nr_regions;                     /* Number of rdist regions */
>          uint32_t rdist_stride;              /* Re-Distributor stride */
> +        struct list_head its_devices;       /* devices mapped to an ITS */
> +        spinlock_t its_devices_lock;        /* protects the its_devices list */
>  #endif
>      } vgic;
>  
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 68e5f63..525a29d 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -93,9 +93,19 @@ struct host_its {
>      void __iomem *its_base;
>      spinlock_t cmd_lock;
>      void *cmd_buf;
> +    int itte_size;
>      bool pta;
>  };
>  
> +struct its_devices {
> +    struct list_head entry;
> +    struct host_its *hw_its;
> +    void *itt_addr;
> +    uint32_t guest_devid;
> +    uint32_t host_devid;
> +    uint32_t eventids;
> +};
> +
>  extern struct list_head host_its_list;
>  
>  #ifdef CONFIG_HAS_ITS
> @@ -119,6 +129,13 @@ void gicv3_set_redist_addr(paddr_t address, int redist_id);
>  /* Map a collection for this host CPU to each host ITS. */
>  void gicv3_its_setup_collection(int cpu);
>  
> +/* Map a device on the host by allocating an ITT on the host (ITS).
> + * "bits" specifies how many events (interrupts) this device will need.
> + * Setting "valid" to false deallocates the device.
> + */
> +int gicv3_its_map_device(struct domain *d, int host_devid, int guest_devid,
> +                         int bits, bool valid);
> +
>  #else
>  
>  static inline void gicv3_its_dt_init(const struct dt_device_node *node)
> @@ -146,6 +163,11 @@ static inline void gicv3_set_redist_addr(paddr_t address, int redist_id)
>  static inline void gicv3_its_setup_collection(int cpu)
>  {
>  }
> +static inline int gicv3_its_map_device(struct domain *d, int host_devid,
> +                         int guest_devid, int bits, bool valid)
> +{
> +    return -ENODEV;
> +}
>  
>  #endif /* CONFIG_HAS_ITS */
>  
> -- 
> 2.9.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel