Hi Eric, > -----Original Message----- > From: Auger Eric [mailto:eric.au...@redhat.com] > Sent: Monday, July 31, 2017 6:38 PM > To: Peter Xu <pet...@redhat.com>; Bharat Bhushan > <bharat.bhus...@nxp.com> > Cc: w...@redhat.com; peter.mayd...@linaro.org; kevin.t...@intel.com; > drjo...@redhat.com; m...@redhat.com; jean-philippe.bruc...@arm.com; > t...@semihalf.com; will.dea...@arm.com; qemu-devel@nongnu.org; > alex.william...@redhat.com; qemu-...@nongnu.org; > marc.zyng...@arm.com; robin.mur...@arm.com; > christoffer.d...@linaro.org; eric.auger....@gmail.com > Subject: Re: [Qemu-devel] [RFC v2 6/8] virtio-iommu: Implement the > translation and commands > > Hi Peter, Bharat, > > On 17/07/2017 03:28, Peter Xu wrote: > > On Fri, Jul 14, 2017 at 06:40:34AM +0000, Bharat Bhushan wrote: > >> 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- > a...@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" > > glib provides g_tree_ref/g_tree_unref. I think the most elegant is to use > g_tree_ref when adding a device and decrementing when removing a > device, as suggested by Peter. > > "if the reference count drops to 0, all keys and values will be destroyed (if > destroy functions were specified) and all memory allocated by tree will be > released." > > That way we should be able to remove nr_devices from viommu_as > > Bharat, if this breaks your implementation I will let you re-introduce > nr_devices as part of the VFIO patch.
We need to unmap() everything in physical-iommu when last device is detached, seems like g_tree_ref/unref() does not give any handle for this. I will go ahead with adding nr_devices with VFIO integration patches. Thanks -Bharat > > Thanks > > Eric > > > > Sorry I didn't read that when posting. It is what I mean. Thanks, > >
