Re: [Qemu-devel] [PATCH v6 3/4] vfio iommu: Add support for mediated devices

[Kirti Wankhede]

Thanks Alex. I'll take care of suggested nits and rename structures and
function.

On 8/10/2016 12:30 AM, Alex Williamson wrote:
> On Thu, 4 Aug 2016 00:33:53 +0530
> Kirti Wankhede <address@hidden> wrote:
>
...

>>
>> +/*
>> + * Pin a set of guest PFNs and return their associated host PFNs for
mediated
>> + * domain only.
>
> Why only mediated domain?  What assumption is specific to a mediated
> domain other than unnecessarily passing an mdev_device?
>
>> + * @user_pfn [in]: array of user/guest PFNs
>> + * @npage [in]: count of array elements
>> + * @prot [in] : protection flags
>> + * @phys_pfn[out] : array of host PFNs
>> + */
>> +long vfio_pin_pages(struct mdev_device *mdev, unsigned long *user_pfn,
>
> Why use and mdev_device here?  We only reference the struct device to
> get the drvdata.  (dev also not listed above in param description)
>

Ok.

>> +                long npage, int prot, unsigned long *phys_pfn)
>> +{
>> +    struct vfio_device *device;
>> +    struct vfio_container *container;
>> +    struct vfio_iommu_driver *driver;
>> +    ssize_t ret = -EINVAL;
>> +
>> +    if (!mdev || !user_pfn || !phys_pfn)
>> +            return -EINVAL;
>> +
>> +    device = dev_get_drvdata(&mdev->dev);
>> +
>> +    if (!device || !device->group)
>> +            return -EINVAL;
>> +
>> +    container = device->group->container;
>
> This doesn't seem like a valid way to get a reference to the container
> and in fact there is no reference at all.  I think you need to use
> vfio_device_get_from_dev(), check and increment container_users around
> the callback, abort on noiommu groups, and check for viability.
>

Thanks for pointing that out. I'll change it as suggested.

>
>
> I see how you're trying to only do accounting when there is only an
> mdev (local) domain, but the devices attached to the normal iommu API
> domain can go away at any point.  Where do we re-establish accounting
> should the pinning from those devices be removed?  I don't see that as
> being an optional support case since userspace can already do this.
>

I missed this case. So in that case, when
vfio_iommu_type1_detach_group() for iommu group for that device is
called and it is the last entry in iommu capable domain_list, it should
re-iterate through pfn_list of mediated_domain and do the accounting,
right? Then we also have to update accounting when iommu capable device
is hotplugged while mediated_domain already exist.

Thanks,
Kirti