On Wed, Jun 07, 2017 at 06:01:25PM +0200, Eric Auger wrote:
> This patch adds the actual implementation for the translation routine
> and the virtio-iommu commands.
>
> Signed-off-by: Eric Auger <eric.au...@redhat.com>
[...]
> static int virtio_iommu_attach(VirtIOIOMMU *s,
> struct virtio_iommu_req_attach *req)
> @@ -95,10 +135,34 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
> uint32_t asid = le32_to_cpu(req->address_space);
> uint32_t devid = le32_to_cpu(req->device);
> uint32_t reserved = le32_to_cpu(req->reserved);
> + viommu_as *as;
> + viommu_dev *dev;
>
> trace_virtio_iommu_attach(asid, devid, reserved);
>
> - return VIRTIO_IOMMU_S_UNSUPP;
> + dev = g_tree_lookup(s->devices, GUINT_TO_POINTER(devid));
> + if (dev) {
> + return -1;
> + }
> +
> + as = g_tree_lookup(s->address_spaces, GUINT_TO_POINTER(asid));
> + if (!as) {
> + as = g_malloc0(sizeof(*as));
> + as->id = asid;
> + as->mappings = g_tree_new_full((GCompareDataFunc)interval_cmp,
> + NULL, NULL, (GDestroyNotify)g_free);
> + g_tree_insert(s->address_spaces, GUINT_TO_POINTER(asid), as);
> + trace_virtio_iommu_new_asid(asid);
> + }
> +
> + dev = g_malloc0(sizeof(*dev));
> + dev->as = as;
> + dev->id = devid;
> + as->nr_devices++;
> + trace_virtio_iommu_new_devid(devid);
> + g_tree_insert(s->devices, GUINT_TO_POINTER(devid), dev);
Here do we need to record something like a refcount for address space?
Since...
> +
> + return VIRTIO_IOMMU_S_OK;
> }
>
> static int virtio_iommu_detach(VirtIOIOMMU *s,
> @@ -106,10 +170,13 @@ static int virtio_iommu_detach(VirtIOIOMMU *s,
> {
> uint32_t devid = le32_to_cpu(req->device);
> uint32_t reserved = le32_to_cpu(req->reserved);
> + int ret;
>
> trace_virtio_iommu_detach(devid, reserved);
>
> - return VIRTIO_IOMMU_S_UNSUPP;
> + ret = g_tree_remove(s->devices, GUINT_TO_POINTER(devid));
... here when detach, imho we should check the refcount: if there is
no device using specific address space, we should release the address
space as well.
Otherwise there would have no way to destroy an address space?
> +
> + return ret ? VIRTIO_IOMMU_S_OK : VIRTIO_IOMMU_S_INVAL;
> }
[...]
> static int virtio_iommu_unmap(VirtIOIOMMU *s,
> @@ -133,10 +227,64 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
> uint64_t virt_addr = le64_to_cpu(req->virt_addr);
> uint64_t size = le64_to_cpu(req->size);
> uint32_t flags = le32_to_cpu(req->flags);
> + viommu_mapping *mapping;
> + viommu_interval interval;
> + viommu_as *as;
>
> trace_virtio_iommu_unmap(asid, virt_addr, size, flags);
>
> - return VIRTIO_IOMMU_S_UNSUPP;
> + as = g_tree_lookup(s->address_spaces, GUINT_TO_POINTER(asid));
> + if (!as) {
> + error_report("%s: no as", __func__);
> + return VIRTIO_IOMMU_S_INVAL;
> + }
> + interval.low = virt_addr;
> + interval.high = virt_addr + size - 1;
> +
> + mapping = g_tree_lookup(as->mappings, (gpointer)&interval);
> +
> + while (mapping) {
> + viommu_interval current;
> + uint64_t low = mapping->virt_addr;
> + uint64_t high = mapping->virt_addr + mapping->size - 1;
> +
> + current.low = low;
> + current.high = high;
> +
> + if (low == interval.low && size >= mapping->size) {
> + g_tree_remove(as->mappings, (gpointer)¤t);
> + interval.low = high + 1;
> + trace_virtio_iommu_unmap_left_interval(current.low, current.high,
> + interval.low, interval.high);
> + } else if (high == interval.high && size >= mapping->size) {
> + trace_virtio_iommu_unmap_right_interval(current.low,
> current.high,
> + interval.low, interval.high);
> + g_tree_remove(as->mappings, (gpointer)¤t);
> + interval.high = low - 1;
> + } else if (low > interval.low && high < interval.high) {
> + trace_virtio_iommu_unmap_inc_interval(current.low, current.high);
> + g_tree_remove(as->mappings, (gpointer)¤t);
> + } else {
> + break;
> + }
> + if (interval.low >= interval.high) {
> + return VIRTIO_IOMMU_S_OK;
> + } else {
> + mapping = g_tree_lookup(as->mappings, (gpointer)&interval);
> + }
> + }
IIUC for unmap here we are very strict - a extreme case is that when
an address space is just created (so no mapping inside), if we send
one UNMAP to a range, it'll fail currently, right? Should we loosen
it?
IMHO as long as we make sure all the mappings in the range of an UNMAP
request are destroyed, then we are good. I think at least both vfio
api and vt-d emuation have this assumption. But maybe I am wrong.
Please correct me if so.
> +
> + if (mapping) {
> + error_report("****** %s: Unmap 0x%"PRIx64" size=0x%"PRIx64
> + " from 0x%"PRIx64" size=0x%"PRIx64" is not supported",
> + __func__, interval.low, size,
> + mapping->virt_addr, mapping->size);
> + } else {
> + error_report("****** %s: no mapping for [0x%"PRIx64",0x%"PRIx64"]",
> + __func__, interval.low, interval.high);
> + }
> +
> + return VIRTIO_IOMMU_S_INVAL;
> }
Thanks,
--
Peter Xu
