[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 30/01/2017 18:31, 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        | 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>

Why did you drop the include xen/mm.h?

+#include <xen/rbtree.h>
+#include <xen/sched.h>
 #include <xen/sizes.h>
+#include <xen/domain.h>

All the header looks to be included in an alphabetical order except this one. Why?

 #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)

Please use unsigned for the size. Also it could be uint8_t.

Also, itt_addr should technically be a paddr_t.

+{
+    uint64_t cmd[4];
+
+    cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32);
+    cmd[1] = size & GENMASK(4, 0);

The mask here and the one below are not necessary and could hide a programming error.

It would make much sense to have an ASSERT(size < GITS_TYPER.IDbits) here and someone checking that the value is correct.

+    cmd[2] = itt_addr & GENMASK(51, 8);

For this one, we only want to make sure the bits 7:0 are all zeroes as the address is provided by the caller. So I would replace the mask by an ASSERT(!(its_addr & GENMASK(7, 0)).

+    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);

I would propagate the return of the ITS command as it may have failed because, for instance, the command queue is full.

+
+    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)


Please use uint32_t for the host_devid and guest_devid.

Also, what is bits? It looks like to me this is the number of bit supported for the event. It think it would be clearer to pass the number of events and compute the number of bits within the function.

Furthermore, looking at the code, I think it would be better to have two separate functions: one to add a device, the other to remove. Overall the code looks quite different for both and some parameter are not useful.

Lastly, I don't see any code here which prevent a device to be assigned to another domain or even wrong host/guest DeviceID and wrong number of events bits. Who will do that?

For checking if the device has been assigned to another, I think this will be done could be done outside.

In any case, as requested by Stefano on the previous version, a TODO in the code will be needed to avoid forgetting.

+{
+    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)

Coding style: 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 )

A printk(XENLOG_GUEST...) here would be useful to know which host DeviceID was associated to the guest deviceID.

+                return -EBUSY;
+
+            remove_mapped_guest_device(temp);

See my comment on the function about the error checking.

+
+            return 0;
+        }
+
+        if ( guest_devid < temp->guest_devid )
+            new = &((*new)->rb_right);
+        else
+            new = &((*new)->rb_left);
+    }
+
+    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.

That's will likely come with the PCI work. The idea so far is to parse the firmware tables in Xen and find out the root complex.

So would in fact expect this function to take a struct device in parameter and deal with it.


+     */
+    hw_its = list_first_entry(&host_its_list, struct host_its, entry);
+
+    ret = -ENOMEM;
+
+    itt_addr = _xmalloc(BIT(bits) * hw_its->itte_size, 256);

Please document why 256.

Also, from the spec (see 6.3.9 in IHI0069C) the behavior is unpredictable when the ITT is not all zero. So please use _xzalloc here.

+    if ( !itt_addr )
+        goto out_unlock;
+
+    dev = xmalloc(struct its_devices);

I would use xzalloc just in case we forgot to initialize a field.

+    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;

Duplicating the clean-up path multiple time is a call to error. I would prefer if you move the xfree in the out_unlock path. Note the xfree is able to handle NULL pointer.

+    }
+
+    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);
+    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);

Newline here please.

+    return ret;
+}
+
+/* Removing any connections a domain had to any ITS in the system. */
+void gicv3_its_unmap_all_devices(struct domain *d)

I don't see this function called anywhere in this series. I appreciate that we don't yet support guest ITS. But calling the function in the right place would be helpful. This would avoid us to miss some clean-up and get yet another XSA.

Also, this function should have a prototype defined in the header.

+{
+    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.

So this is slightly untrue. Performance matter when destroying a domain as Xen cannot be preempted. So if it is takes too long, you will have a impact on the overall system.

However, I think it would be fair to assume that all device will be deassigned before the ITS is destroyed.

So I would just drop this function. Note that we do the same assumption in the SMMU driver.

+     */
+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)

Can you explain why you have this function and gicv3_its_map_guest_device (with valid = 0) doing exactly the same?

We should avoid having two interfaces doing exactly the same. In this case, I would avoid to handle the unmapping in gicv3_its_map_guest_device.

Also, this function should have a prototype defined in the header.

+{
+    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);

The cmd_lock was introduced in the previous patch. Please move the initialization there.


         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;

The placement of those 2 lines are likely wrong. This should belong to the vITS and not the vgic-v3.

I think it would make sense to get a patch that introduces a skeleton for the vITS before this patch and start plumbing through.

+
     /*
      * 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;

Please use unsigned.

     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).

Coding style:

/*
 * Map a ...

+ * "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;

I think it should be -ENOSYS rather than -ENODEV.

+}

 #endif /* CONFIG_HAS_ITS */



Cheers,

--
Julien Grall

_______________________________________________
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®.