[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 07/28] ARM: GICv3 ITS: introduce device mapping



Hi Andre,

On Tue, Jan 31, 2017 at 12:01 AM, Andre Przywara <andre.przywara@xxxxxxx> 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 an rbtree to
> later be able to iterate over it or to easily teardown guests.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic-v3-its.c        | 188 
> ++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/vgic-v3.c           |   3 +
>  xen/include/asm-arm/domain.h     |   3 +
>  xen/include/asm-arm/gic_v3_its.h |  28 ++++++
>  4 files changed, 221 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 6578e8a..4a3a394 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -21,8 +21,10 @@
>  #include <xen/device_tree.h>
>  #include <xen/delay.h>
>  #include <xen/libfdt/libfdt.h>
> -#include <xen/mm.h>
> +#include <xen/rbtree.h>
> +#include <xen/sched.h>
>  #include <xen/sizes.h>
> +#include <xen/domain.h>
>  #include <asm/gic.h>
>  #include <asm/gic_v3_defs.h>
>  #include <asm/gic_v3_its.h>
> @@ -94,6 +96,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] |= GITS_VALID_BIT;
> +    cmd[3] = 0x00;
> +
> +    return its_send_command(its, cmd);
> +}
> +
>  /* Set up the (1:1) collection mapping for the given host CPU. */
>  int gicv3_its_setup_collection(int cpu)
>  {
> @@ -293,6 +310,7 @@ int gicv3_its_init(struct host_its *hw_its)
>      reg = readq_relaxed(hw_its->its_base + GITS_TYPER);
>      if ( reg & GITS_TYPER_PTA )
>          hw_its->flags |= HOST_ITS_USES_PTA;
> +    hw_its->itte_size = GITS_TYPER_ITT_SIZE(reg);
>
>      for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
>      {
> @@ -339,6 +357,173 @@ int gicv3_its_init(struct host_its *hw_its)
>      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_guest_device(struct domain *d, int host_devid,
> +                               int guest_devid, int bits, bool valid)
> +{
> +    void *itt_addr = NULL;
> +    struct its_devices *dev, *temp;
> +    struct rb_node **new = &d->arch.vgic.its_devices.rb_node, *parent = NULL;
> +    struct host_its *hw_its;
> +    int ret;
> +
> +    /* check for already existing mappings */
> +    spin_lock(&d->arch.vgic.its_devices_lock);
> +    while (*new)
> +    {
> +        temp = rb_entry(*new, struct its_devices, rbnode);
> +
> +        if ( temp->guest_devid == guest_devid )
> +        {
> +            if ( !valid )
> +                rb_erase(&temp->rbnode, &d->arch.vgic.its_devices);
> +
> +            spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +            if ( valid )
> +                return -EBUSY;
> +
> +            remove_mapped_guest_device(temp);
> +
> +            return 0;
> +        }
> +
> +        if ( guest_devid < temp->guest_devid )
> +            new = &((*new)->rb_right);

Shouldn't be rb_left here?
> +        else
> +            new = &((*new)->rb_left);
Shouldn't be rb_right here?

> +    }

I see duplicate code in find and inserting node into rb_tree.

> +
> +    if ( !valid )
> +    {
> +        ret = -ENOENT;
> +        goto out_unlock;
> +    }
> +
> +    /*
> +     * 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);
> +
> +    ret = -ENOMEM;
> +
> +    itt_addr = _xmalloc(BIT(bits) * hw_its->itte_size, 256);
> +    if ( !itt_addr )
> +        goto out_unlock;
> +
> +    dev = xmalloc(struct its_devices);
> +    if ( !dev )
> +    {
> +        xfree(itt_addr);
> +        goto out_unlock;
> +    }
> +
> +    ret = its_send_cmd_mapd(hw_its, host_devid, bits - 1,
> +                            virt_to_maddr(itt_addr), true);
> +    if ( ret )
> +    {
> +        xfree(itt_addr);
> +        xfree(dev);
> +        goto out_unlock;
> +    }
> +
> +    dev->itt_addr = itt_addr;
> +    dev->hw_its = hw_its;
> +    dev->guest_devid = guest_devid;
> +    dev->host_devid = host_devid;
> +    dev->eventids = BIT(bits);
> +
> +    rb_link_node(&dev->rbnode, parent, new);

parent is not updated before inserting. So whole tree is not
managed properly.

> +    rb_insert_color(&dev->rbnode, &d->arch.vgic.its_devices);
> +
> +    spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +    return 0;
> +
> +out_unlock:
> +    spin_unlock(&d->arch.vgic.its_devices_lock);
> +    return ret;
> +}
> +
> +/* Removing any connections a domain had to any ITS in the system. */
> +void gicv3_its_unmap_all_devices(struct domain *d)
> +{
> +    struct rb_node *victim;
> +    struct its_devices *dev;
> +
> +    /*
> +     * This is an easily readable, yet inefficient implementation.
> +     * It uses the provided iteration wrapper and erases each node, which
> +     * possibly triggers rebalancing.
> +     * This seems overkill since we are going to abolish the whole tree, but
> +     * avoids an open-coded re-implementation of the traversal functions with
> +     * some recursive function calls.
> +     * Performance does not matter here, since we are destroying a domain.
> +     */
> +restart:
> +    spin_lock(&d->arch.vgic.its_devices_lock);
> +    if ( (victim = rb_first(&d->arch.vgic.its_devices)) )
> +    {
> +        dev = rb_entry(victim, struct its_devices, rbnode);
> +        rb_erase(victim, &d->arch.vgic.its_devices);
> +
> +        spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +        remove_mapped_guest_device(dev);
> +
> +        goto restart;
> +    }
> +
> +    spin_unlock(&d->arch.vgic.its_devices_lock);
> +}
> +
> +int gicv3_its_unmap_device(struct domain *d, int guest_devid)
> +{
> +    struct rb_node *node;
> +
> +    spin_lock(&d->arch.vgic.its_devices_lock);
> +    node = d->arch.vgic.its_devices.rb_node;
> +    while (node)
> +    {
> +        struct its_devices *dev = rb_entry(node, struct its_devices, rbnode);
> +
> +        if ( dev->guest_devid > guest_devid )
> +        {
> +            node = node->rb_left;
> +            continue;
> +        }
> +        if ( dev->guest_devid < guest_devid )
> +        {
> +            node = node->rb_right;
> +            continue;
> +        }
> +
> +        rb_erase(node, &d->arch.vgic.its_devices);
> +
> +        spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +        remove_mapped_guest_device(dev);
> +
> +        return 0;
> +
> +    }
> +    spin_unlock(&d->arch.vgic.its_devices_lock);
> +
> +    return -ENOENT;
> +}
> +
>  /* 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)
>  {
> @@ -369,6 +554,7 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
>          its_data->addr = addr;
>          its_data->size = size;
>          its_data->dt_node = its;
> +        spin_lock_init(&its_data->cmd_lock);
>
>          printk("GICv3: Found ITS @0x%lx\n", addr);
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index d61479d..1fadb00 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1450,6 +1450,9 @@ static int vgic_v3_domain_init(struct domain *d)
>      d->arch.vgic.nr_regions = rdist_count;
>      d->arch.vgic.rdist_regions = rdist_regions;
>
> +    spin_lock_init(&d->arch.vgic.its_devices_lock);
> +    d->arch.vgic.its_devices = RB_ROOT;
> +
>      /*
>       * Domain 0 gets the hardware address.
>       * Guests get the virtual platform layout.
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 2d6fbb1..00b9c1a 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -11,6 +11,7 @@
>  #include <asm/gic.h>
>  #include <public/hvm/params.h>
>  #include <xen/serial.h>
> +#include <xen/rbtree.h>
>
>  struct hvm_domain
>  {
> @@ -109,6 +110,8 @@ struct arch_domain
>          } *rdist_regions;
>          int nr_regions;                     /* Number of rdist regions */
>          uint32_t rdist_stride;              /* Re-Distributor stride */
> +        struct rb_root its_devices;         /* devices mapped to an ITS */
> +        spinlock_t its_devices_lock;        /* protects the its_devices tree 
> */
>  #endif
>      } vgic;
>
> diff --git a/xen/include/asm-arm/gic_v3_its.h 
> b/xen/include/asm-arm/gic_v3_its.h
> index 8288185..9c5dcf3 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -42,6 +42,10 @@
>
>  #define GITS_TYPER_PTA                  BIT_ULL(19)
>  #define GITS_TYPER_IDBITS_SHIFT         8
> +#define GITS_TYPER_ITT_SIZE_SHIFT       4
> +#define GITS_TYPER_ITT_SIZE_MASK        (0xfUL << GITS_TYPER_ITT_SIZE_SHIFT)
> +#define GITS_TYPER_ITT_SIZE(r)          (((r) & GITS_TYPER_ITT_SIZE_MASK) >> 
> \
> +                                                GITS_TYPER_ITT_SIZE_SHIFT)
>
>  #define GITS_IIDR_VALUE                 0x34c
>
> @@ -88,6 +92,7 @@
>
>  #ifndef __ASSEMBLY__
>  #include <xen/device_tree.h>
> +#include <xen/rbtree.h>
>
>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>  #define HOST_ITS_USES_PTA               (1U << 1)
> @@ -101,9 +106,19 @@ struct host_its {
>      void __iomem *its_base;
>      spinlock_t cmd_lock;
>      void *cmd_buf;
> +    int itte_size;
>      unsigned int flags;
>  };
>
> +struct its_devices {
> +    struct rb_node rbnode;
> +    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
> @@ -128,6 +143,13 @@ uint64_t gicv3_get_redist_address(int cpu, bool use_pta);
>  /* Map a collection for this host CPU to each host ITS. */
>  int 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_guest_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)
> @@ -156,6 +178,12 @@ static inline int gicv3_its_setup_collection(int cpu)
>  {
>      return 0;
>  }
> +static inline int gicv3_its_map_guest_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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.