|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 06/26] ARM: GICv3 ITS: introduce device mapping
Hi Stefano,
On 05/01/17 00:13, Stefano Stabellini wrote:
> 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@xxxxxxx>
>> ---
>> 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?
They are indeed quite sparse, as they effectively are PCI IDs.
But AFAICS this list is never iterated in a hot path. There are three
users of the list:
- gicv3_its_map_device(), which is only called by the physdev_ops
handler, so only once at Dom0 boot, basically.
- its_remove_domain(), which has no caller at the moment and is just
here for the sake of completeness and to make sure we remove anything
that we created. But it isn't performance critical at all.
- find_guest_lpi(), which is called by gicv3_assign_guest_event() and
gicv3_lpi_change_vcpu(). Those are called by the MAPTI, DISCARD and MOVI
virtual ITS command handlers, but those commands are expected to be rare.
So going away from a list is not strictly necessary, IMHO.
But frankly I just chose a list because I know how it works by heart. I
guess I have to check how easy rbtrees or hash tables are to use in Xen,
as it sounds indeed sensible to use one of them (probably an rbtree)
instead.
> 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?
Not at the moment, and I wouldn't expect it to be the task of the ITS
emulation to take care of that. That sounds like something for the PCI
passthrough subsystem to check. This function here will eventually get
called by that layer once it has sanitized the request and prepares the
device for triggering MSIs.
For this series there is only one domain to consider (Dom0) and we check
the per-domain device list to avoid double assignment, so this is safe.
Cheers,
Andre.
>> + 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
>>
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |