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

Re: [Xen-devel] [PATCH v2 06/27] ARM: GICv3 ITS: introduce device mapping



Hi,

On 22/03/17 22:45, Stefano Stabellini wrote:
> On Thu, 16 Mar 2017, 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 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        | 207 
>> +++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/vgic-v3.c           |   3 +
>>  xen/include/asm-arm/domain.h     |   3 +
>>  xen/include/asm-arm/gic_v3_its.h |  18 ++++
>>  4 files changed, 231 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 5c11b0d..60b15b5 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -21,6 +21,8 @@
>>  #include <xen/lib.h>
>>  #include <xen/delay.h>
>>  #include <xen/mm.h>
>> +#include <xen/rbtree.h>
>> +#include <xen/sched.h>
>>  #include <xen/sizes.h>
>>  #include <asm/gic.h>
>>  #include <asm/gic_v3_defs.h>
>> @@ -32,6 +34,17 @@
>>  
>>  LIST_HEAD(host_its_list);
>>  
>> +struct its_devices {
>> +    struct rb_node rbnode;
>> +    struct host_its *hw_its;
>> +    void *itt_addr;
>> +    paddr_t guest_doorbell;
>> +    uint32_t host_devid;
>> +    uint32_t guest_devid;
>> +    uint32_t eventids;
>> +    uint32_t *host_lpis;
>> +};
>> +
>>  bool gicv3_its_host_has_its(void)
>>  {
>>      return !list_empty(&host_its_list);
>> @@ -149,6 +162,24 @@ static int its_send_cmd_mapc(struct host_its *its, 
>> uint32_t collection_id,
>>      return its_send_command(its, cmd);
>>  }
>>  
>> +static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
>> +                             uint8_t size_bits, paddr_t itt_addr, bool 
>> valid)
>> +{
>> +    uint64_t cmd[4];
>> +
>> +    if ( valid )
>> +    {
>> +        ASSERT(size_bits < 32);
>> +        ASSERT(!(itt_addr & ~GENMASK(51, 8)));
>> +    }
>> +    cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
>> +    cmd[1] = valid ? size_bits : 0x00;
>> +    cmd[2] = valid ? (itt_addr | GITS_VALID_BIT) : 0x00;
>> +    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(unsigned int cpu)
>>  {
>> @@ -379,6 +410,7 @@ static int gicv3_its_init_single_its(struct host_its 
>> *hw_its)
>>      devid_bits = min(devid_bits, max_its_device_bits);
>>      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++ )
>>      {
>> @@ -428,6 +460,180 @@ int gicv3_its_init(void)
>>      return 0;
>>  }
>>  
>> +static int remove_mapped_guest_device(struct its_devices *dev)
>> +{
>> +    int ret;
>> +
>> +    if ( dev->hw_its )
>> +    {
>> +        int ret = its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, 
>> false);
>> +        if ( ret )
>> +            return ret;
>> +    }
>> +
>> +    ret = gicv3_its_wait_commands(dev->hw_its);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    xfree(dev->itt_addr);
>> +    xfree(dev);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct host_its *gicv3_its_find_by_doorbell(paddr_t doorbell_address)
>> +{
>> +    struct host_its *hw_its;
>> +
>> +    list_for_each_entry(hw_its, &host_its_list, entry)
> 
> Does this need to take a spinlock to protect host_its_list? I guess not
> because the list is not modified after boot?

Exactly, I added a comment in v4 explaining this.

>> +    {
>> +        if ( hw_its->addr + ITS_DOORBELL_OFFSET == doorbell_address )
>> +            return hw_its;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static int compare_its_guest_devices(struct its_devices *dev,
>> +                                     paddr_t doorbell, uint32_t devid)
>> +{
>> +    if ( dev->guest_doorbell < doorbell )
>> +        return -1;
>> +
>> +    if ( dev->guest_doorbell > doorbell )
>> +        return 1;
>> +
>> +    if ( dev->guest_devid < devid )
>> +        return -1;
>> +
>> +    if ( dev->guest_devid > devid )
>> +        return 1;
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Map a hardware device, identified by a certain host ITS and its device ID
>> + * to domain d, a guest ITS (identified by its doorbell address) and device 
>> ID.
>> + * Also provide the number of events (MSIs) needed for that device.
>> + * This does not check if this particular hardware device is already mapped
>> + * at another domain, it is expected that this would be done by the caller.
>> + */
>> +int gicv3_its_map_guest_device(struct domain *d,
>> +                               paddr_t host_doorbell, uint32_t host_devid,
>> +                               paddr_t guest_doorbell, uint32_t guest_devid,
>> +                               uint32_t nr_events, bool valid)
>> +{
>> +    void *itt_addr = NULL;
>> +    struct host_its *hw_its;
>> +    struct its_devices *dev = NULL, *temp;
>> +    struct rb_node **new = &d->arch.vgic.its_devices.rb_node, *parent = 
>> NULL;
>> +    int ret = -ENOENT;
>> +
>> +    hw_its = gicv3_its_find_by_doorbell(host_doorbell);
>> +    if ( !hw_its )
>> +        return ret;
>> +
>> +    /* check for already existing mappings */
>> +    spin_lock(&d->arch.vgic.its_devices_lock);
>> +    while ( *new )
>> +    {
>> +        temp = rb_entry(*new, struct its_devices, rbnode);
>> +
>> +        parent = *new;
>> +        if ( !compare_its_guest_devices(temp, guest_doorbell, 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;
>> +
>> +            return remove_mapped_guest_device(temp);
>> +        }
>> +
> 
> Plese don't run compare_its_guest_devices twice with the same arguments,
> just store the return value.

Fixed in v3.

>> +        if ( compare_its_guest_devices(temp, guest_doorbell, guest_devid) > 
>> 0 )
>> +            new = &((*new)->rb_left);
>> +        else
>> +            new = &((*new)->rb_right);
>> +    }
>> +
>> +    if ( !valid )
>> +        goto out_unlock;
>> +
>> +    ret = -ENOMEM;
>> +
>> +    /* An Interrupt Translation Table needs to be 256-byte aligned. */
>> +    itt_addr = _xzalloc(nr_events * hw_its->itte_size, 256);
>> +    if ( !itt_addr )
>> +        goto out_unlock;
>> +
>> +    dev = xzalloc(struct its_devices);
>> +    if ( !dev )
>> +        goto out_unlock;
>> +
>> +    ret = its_send_cmd_mapd(hw_its, host_devid, fls(nr_events - 1) - 1,
>> +                            virt_to_maddr(itt_addr), true);
>> +    if ( ret )
>> +        goto out_unlock;
>> +
>> +    dev->itt_addr = itt_addr;
>> +    dev->hw_its = hw_its;
>> +    dev->guest_doorbell = guest_doorbell;
>> +    dev->guest_devid = guest_devid;
>> +    dev->host_devid = host_devid;
>> +    dev->eventids = nr_events;
>> +
>> +    rb_link_node(&dev->rbnode, parent, new);
>> +    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);
>> +    if ( dev )
>> +        xfree(dev->host_lpis);
> 
> Where is host_lpis allocated? Why is it freed here?

So I swapped this patch with the next one in v4, because this solves
this issue.

>> +    xfree(itt_addr);
>> +    xfree(dev);
>> +    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;
>> +    }
> 
> There is no need to use goto here, a simple while loop should suffice.

Fixed in v4.

>> +    spin_unlock(&d->arch.vgic.its_devices_lock);
>> +}
>> +
>>  /* 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)
>>  {
>> @@ -455,6 +661,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);
> 
> This change should be in the previous patch

Fixed in v4.

>>          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 8b493fb..3421ea0 100644
>> --- a/xen/include/asm-arm/gic_v3_its.h
>> +++ b/xen/include/asm-arm/gic_v3_its.h
>> @@ -48,6 +48,10 @@
>>  #define GITS_TYPER_DEVICE_ID_BITS(r)    (((r & GITS_TYPER_DEVIDS_MASK) >> \
>>                                                 GITS_TYPER_DEVIDS_SHIFT) + 1)
>>  #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) 
>> + 1)
>>  
>>  #define GITS_IIDR_VALUE                 0x34c
>>  
>> @@ -94,7 +98,10 @@
>>  #define GITS_CMD_MOVALL                 0x0e
>>  #define GITS_CMD_DISCARD                0x0f
>>  
>> +#define ITS_DOORBELL_OFFSET             0x10040
>> +
>>  #include <xen/device_tree.h>
>> +#include <xen/rbtree.h>
>>  
>>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>>  #define HOST_ITS_USES_PTA               (1U << 1)
>> @@ -108,6 +115,7 @@ struct host_its {
>>      void __iomem *its_base;
>>      spinlock_t cmd_lock;
>>      void *cmd_buf;
>> +    unsigned int itte_size;
>>      unsigned int flags;
>>  };
> 
> I would move itte_size and its initialization to the patch that
> introduced struct host_its.

Puh, OK. This wasn't as innocent as it looks like on the first glance,
so I created a new patch to introduce the host ITS init early and
separate this from the ITS table init.

Cheers,
Andre.


>> @@ -134,6 +142,16 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, 
>> bool use_pta);
>>  /* Map a collection for this host CPU to each host ITS. */
>>  int gicv3_its_setup_collection(unsigned int cpu);
>>  
>> +/*
>> + * Map a device on the host by allocating an ITT on the host (ITS).
>> + * "nr_event" 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,
>> +                               paddr_t host_doorbell, uint32_t host_devid,
>> +                               paddr_t guest_doorbell, uint32_t guest_devid,
>> +                               uint32_t nr_events, bool valid);
>> +
>>  #else
>>  
>>  static LIST_HEAD(host_its_list);
>> -- 
>> 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®.