Hi Peter,
> -----Original Message-----
> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Friday, July 14, 2017 7:48 AM
> To: Eric Auger <eric.au...@redhat.com>
> Cc: eric.auger....@gmail.com; peter.mayd...@linaro.org;
> alex.william...@redhat.com; m...@redhat.com; qemu-...@nongnu.org;
> qemu-devel@nongnu.org; jean-philippe.bruc...@arm.com;
> w...@redhat.com; kevin.t...@intel.com; Bharat Bhushan
> <bharat.bhus...@nxp.com>; marc.zyng...@arm.com; t...@semihalf.com;
> will.dea...@arm.com; drjo...@redhat.com; robin.mur...@arm.com;
> christoffer.d...@linaro.org
> Subject: Re: [Qemu-devel] [RFC v2 6/8] virtio-iommu: Implement the
> translation and commands
>
> 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...
We are using "nr_devices" as number of devices attached to an address-space
>
> > +
> > + 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?
Here if nr_devices == 0 then release the address space, is that ok?
This is how I implemented as part of VFIO integration over this patch series.
"[RFC PATCH 2/2] virtio-iommu: vfio integration with virtio-iommu"
Thanks
-Bharat
>
> > +
> > + 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
