qemu-arm
[Top][All Lists]
Advanced

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

Re: [Qemu-arm] [Qemu-devel] [RFC v2 6/8] virtio-iommu: Implement the tra


From: Bharat Bhushan
Subject: Re: [Qemu-arm] [Qemu-devel] [RFC v2 6/8] virtio-iommu: Implement the translation and commands
Date: Thu, 3 Aug 2017 10:48:15 +0000

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:address@hidden
> Sent: Monday, July 31, 2017 6:38 PM
> To: Peter Xu <address@hidden>; Bharat Bhushan
> <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; address@hidden
> 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:address@hidden
> >>> Sent: Friday, July 14, 2017 7:48 AM
> >>> To: Eric Auger <address@hidden>
> >>> Cc: address@hidden; address@hidden;
> >>> address@hidden; address@hidden; qemu-
> address@hidden;
> >>> address@hidden; address@hidden;
> >>> address@hidden; address@hidden; Bharat Bhushan
> >>> <address@hidden>; address@hidden;
> address@hidden;
> >>> address@hidden; address@hidden; address@hidden;
> >>> address@hidden
> >>> 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 <address@hidden>
> >>>
> >>> [...]
> >>>
> >>>>  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,
> >

reply via email to

[Prev in Thread] Current Thread [Next in Thread]