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 <kwankhede@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
