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

Re: [Xen-devel] [PATCH v5 11/30] ARM: GICv3 ITS: introduce device mapping



Hi Andre,

A couple of more comments.

On 04/06/2017 12:19 AM, Andre Przywara wrote:
+static int remove_mapped_guest_device(struct its_devices *dev)
+{
+    int ret = 0;
+    unsigned int i;
+

I would add a TODO here to think about the interaction with the guest vITS when removing a device whilst it is still in-use. Such as when the vLPI is still inflight or the ITT has not been unmapped.

It still possible to do a MAPD (V=0) before whilst vLPI associated to this device are inflight. This would lead to issue when freeing pending_irq as they would become NULL.

Maybe for this version we can think to never remove a device from DOM0 (i.e MAPD(V=0) will not call gicv3_its_map_guest_device).

This would prevent all the issue possible with pending_irq. But this would need to be fixed before guest support (*hint* this should be added in the TODO in the cover letter ;)).

+    if ( dev->hw_its )
+        /* MAPD also discards all events with this device ID. */
+        ret = its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, false);
+
+    for ( i = 0; i < dev->eventids / LPI_BLOCK; i++ )
+        gicv3_free_host_lpi_block(dev->host_lpi_blocks[i]);
+
+    /* Make sure the MAPD command above is really executed. */
+    if ( !ret )
+        ret = gicv3_its_wait_commands(dev->hw_its);
+
+    /* This should never happen, but just in case ... */
+    if ( ret )
+        printk(XENLOG_WARNING "Can't unmap host ITS device 0x%x\n",
+               dev->host_devid);
+
+    xfree(dev->itt_addr);
+    xfree(dev->pend_irqs);
+    xfree(dev->host_lpi_blocks);
+    xfree(dev);
+
+    return 0;

[...]

 /*
  * On the host ITS @its, map @nr_events consecutive LPIs.
  * The mapping connects a device @devid and event @eventid pair to LPI @lpi,
@@ -510,6 +616,163 @@ static int gicv3_its_map_host_events(struct host_its *its,
     return gicv3_its_wait_commands(its);
 }

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

My understanding if the number of event cannot be 0. You would start from 1 and it is possible to have up to 2^32 events which will overflow.

So I think we should use uint64_t for nr_events.

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