|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |