diff mbox series

[for-5.0,v11,07/20] virtio-iommu: Implement map/unmap

Message ID 20191122182943.4656-8-eric.auger@redhat.com
State New
Headers show
Series VIRTIO-IOMMU device | expand

Commit Message

Eric Auger Nov. 22, 2019, 6:29 p.m. UTC 
This patch implements virtio_iommu_map/unmap.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v10 -> v11:
- revisit the implementation of unmap according to Peter's suggestion
- removed virt_addr and size from viommu_mapping struct
- use g_tree_lookup_extended()
- return VIRTIO_IOMMU_S_RANGE in case a mapping were
  to be split on unmap (instead of INVAL)

v5 -> v6:
- use new v0.6 fields
- replace error_report by qemu_log_mask

v3 -> v4:
- implement unmap semantics as specified in v0.4
---
 hw/virtio/trace-events   |  1 +
 hw/virtio/virtio-iommu.c | 65 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 2 deletions(-)

Comments

Jean-Philippe Brucker Dec. 10, 2019, 4:43 p.m. UTC | #1 
On Fri, Nov 22, 2019 at 07:29:30PM +0100, Eric Auger wrote:
> @@ -238,10 +244,35 @@ static int virtio_iommu_map(VirtIOIOMMU *s,
>      uint64_t virt_start = le64_to_cpu(req->virt_start);
>      uint64_t virt_end = le64_to_cpu(req->virt_end);
>      uint32_t flags = le32_to_cpu(req->flags);
> +    viommu_domain *domain;
> +    viommu_interval *interval;
> +    viommu_mapping *mapping;

Additional checks would be good. Most importantly we need to return
S_INVAL if we don't recognize a bit in flags (a MUST in the spec). It
might be good to check that addresses are aligned on the page granule as
well, and return S_RANGE if they aren't (a SHOULD in the spec), but I
don't care as much.

> +
> +    interval = g_malloc0(sizeof(*interval));
> +
> +    interval->low = virt_start;
> +    interval->high = virt_end;
> +
> +    domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
> +    if (!domain) {
> +        return VIRTIO_IOMMU_S_NOENT;

Leaks interval, I guess you could allocate it after this block.

Thanks,
Jean

> +    }
> +
> +    mapping = g_tree_lookup(domain->mappings, (gpointer)interval);
> +    if (mapping) {
> +        g_free(interval);
> +        return VIRTIO_IOMMU_S_INVAL;
> +    }
>  
>      trace_virtio_iommu_map(domain_id, virt_start, virt_end, phys_start, flags);
>  
> -    return VIRTIO_IOMMU_S_UNSUPP;
> +    mapping = g_malloc0(sizeof(*mapping));
> +    mapping->phys_addr = phys_start;
> +    mapping->flags = flags;
> +
> +    g_tree_insert(domain->mappings, interval, mapping);
> +
> +    return VIRTIO_IOMMU_S_OK;
Eric Auger Dec. 23, 2019, 9:42 a.m. UTC | #2 
Hi jean,

On 12/10/19 5:43 PM, Jean-Philippe Brucker wrote:
> On Fri, Nov 22, 2019 at 07:29:30PM +0100, Eric Auger wrote:
>> @@ -238,10 +244,35 @@ static int virtio_iommu_map(VirtIOIOMMU *s,
>>      uint64_t virt_start = le64_to_cpu(req->virt_start);
>>      uint64_t virt_end = le64_to_cpu(req->virt_end);
>>      uint32_t flags = le32_to_cpu(req->flags);
>> +    viommu_domain *domain;
>> +    viommu_interval *interval;
>> +    viommu_mapping *mapping;
> 
> Additional checks would be good. Most importantly we need to return
> S_INVAL if we don't recognize a bit in flags (a MUST in the spec). 
Sure

It
> might be good to check that addresses are aligned on the page granule as
> well, and return S_RANGE if they aren't (a SHOULD in the spec), but I
> don't care as much.
with KVM accelerated guest I don't have access to the guest page size,
hence the choice of not checking it.
> 
>> +
>> +    interval = g_malloc0(sizeof(*interval));
>> +
>> +    interval->low = virt_start;
>> +    interval->high = virt_end;
>> +
>> +    domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
>> +    if (!domain) {
>> +        return VIRTIO_IOMMU_S_NOENT;
> 
> Leaks interval, I guess you could allocate it after this block.
Sure

Thanks!

Eric
> 
> Thanks,
> Jean
> 
>> +    }
>> +
>> +    mapping = g_tree_lookup(domain->mappings, (gpointer)interval);
>> +    if (mapping) {
>> +        g_free(interval);
>> +        return VIRTIO_IOMMU_S_INVAL;
>> +    }
>>  
>>      trace_virtio_iommu_map(domain_id, virt_start, virt_end, phys_start, flags);
>>  
>> -    return VIRTIO_IOMMU_S_UNSUPP;
>> +    mapping = g_malloc0(sizeof(*mapping));
>> +    mapping->phys_addr = phys_start;
>> +    mapping->flags = flags;
>> +
>> +    g_tree_insert(domain->mappings, interval, mapping);
>> +
>> +    return VIRTIO_IOMMU_S_OK;
>
diff mbox series

Patch

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index a373bdebb3..f25359cee2 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -65,6 +65,7 @@  virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
 virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
 virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start, uint32_t flags) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"
 virtio_iommu_unmap(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
+virtio_iommu_unmap_done(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
 virtio_iommu_translate(const char *name, uint32_t rid, uint64_t iova, int flag) "mr=%s rid=%d addr=0x%"PRIx64" flag=%d"
 virtio_iommu_init_iommu_mr(char *iommu_mr) "init %s"
 virtio_iommu_get_endpoint(uint32_t ep_id) "Alloc endpoint=%d"
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 138d5b2a9c..f0a56833a2 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -18,6 +18,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "qemu/iov.h"
 #include "qemu-common.h"
 #include "hw/qdev-properties.h"
@@ -55,6 +56,11 @@  typedef struct viommu_interval {
     uint64_t high;
 } viommu_interval;
 
+typedef struct viommu_mapping {
+    uint64_t phys_addr;
+    uint32_t flags;
+} viommu_mapping;
+
 static inline uint16_t virtio_iommu_get_sid(IOMMUDevice *dev)
 {
     return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
@@ -238,10 +244,35 @@  static int virtio_iommu_map(VirtIOIOMMU *s,
     uint64_t virt_start = le64_to_cpu(req->virt_start);
     uint64_t virt_end = le64_to_cpu(req->virt_end);
     uint32_t flags = le32_to_cpu(req->flags);
+    viommu_domain *domain;
+    viommu_interval *interval;
+    viommu_mapping *mapping;
+
+    interval = g_malloc0(sizeof(*interval));
+
+    interval->low = virt_start;
+    interval->high = virt_end;
+
+    domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
+    if (!domain) {
+        return VIRTIO_IOMMU_S_NOENT;
+    }
+
+    mapping = g_tree_lookup(domain->mappings, (gpointer)interval);
+    if (mapping) {
+        g_free(interval);
+        return VIRTIO_IOMMU_S_INVAL;
+    }
 
     trace_virtio_iommu_map(domain_id, virt_start, virt_end, phys_start, flags);
 
-    return VIRTIO_IOMMU_S_UNSUPP;
+    mapping = g_malloc0(sizeof(*mapping));
+    mapping->phys_addr = phys_start;
+    mapping->flags = flags;
+
+    g_tree_insert(domain->mappings, interval, mapping);
+
+    return VIRTIO_IOMMU_S_OK;
 }
 
 static int virtio_iommu_unmap(VirtIOIOMMU *s,
@@ -250,10 +281,40 @@  static int virtio_iommu_unmap(VirtIOIOMMU *s,
     uint32_t domain_id = le32_to_cpu(req->domain);
     uint64_t virt_start = le64_to_cpu(req->virt_start);
     uint64_t virt_end = le64_to_cpu(req->virt_end);
+    viommu_mapping *iter_val;
+    viommu_interval interval, *iter_key;
+    viommu_domain *domain;
+    int ret = VIRTIO_IOMMU_S_OK;
 
     trace_virtio_iommu_unmap(domain_id, virt_start, virt_end);
 
-    return VIRTIO_IOMMU_S_UNSUPP;
+    domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
+    if (!domain) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: no domain\n", __func__);
+        return VIRTIO_IOMMU_S_NOENT;
+    }
+    interval.low = virt_start;
+    interval.high = virt_end;
+
+    while (g_tree_lookup_extended(domain->mappings, &interval,
+                                  (void **)&iter_key, (void**)&iter_val)) {
+        uint64_t current_low = iter_key->low;
+        uint64_t current_high = iter_key->high;
+
+        if (interval.low <= current_low && interval.high >= current_high) {
+            g_tree_remove(domain->mappings, iter_key);
+            trace_virtio_iommu_unmap_done(domain_id, current_low, current_high);
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: domain= %d Unmap [0x%"PRIx64",0x%"PRIx64"] forbidden as "
+                "it would split existing mapping [0x%"PRIx64", 0x%"PRIx64"]\n",
+                __func__, domain_id, interval.low, interval.high,
+                current_low, current_high);
+            ret = VIRTIO_IOMMU_S_RANGE;
+            break;
+        }
+    }
+    return ret;
 }
 
 static int virtio_iommu_iov_to_req(struct iovec *iov,