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

[Alex Williamson]

On Thu, 11 Aug 2016 19:52:06 +0530
Kirti Wankhede <address@hidden> wrote:

> 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.

Yes, so pages are going to get pinned once for the iommu api domain and
once for the mediated domain, and accounting needs to be updated for
those domains coming and going in any order.  Thanks,

Alex