Re: [Qemu-devel] [RFC v2 6/8] virtio-iommu: Implement the translation and commands

[Auger Eric <eric.auger@redhat.com>]

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:peterx@redhat.com]
>>> Sent: Friday, July 14, 2017 7:48 AM
>>> To: Eric Auger <eric.auger@redhat.com>
>>> Cc: eric.auger.pro@gmail.com; peter.maydell@linaro.org;
>>> alex.williamson@redhat.com; mst@redhat.com; qemu-arm@nongnu.org;
>>> qemu-devel@nongnu.org; jean-philippe.brucker@arm.com;
>>> wei@redhat.com; kevin.tian@intel.com; Bharat Bhushan
>>> <bharat.bhushan@nxp.com>; marc.zyngier@arm.com; tn@semihalf.com;
>>> will.deacon@arm.com; drjones@redhat.com; robin.murphy@arm.com;
>>> christoffer.dall@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.auger@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.

Thanks

Eric
> 
> Sorry I didn't read that when posting. It is what I mean.  Thanks,
>