Re: [PATCH] vfio: add a singleton check for vfio_group_pin_pages

[Yan Zhao]

On Tue, Sep 15, 2020 at 01:30:11PM -0600, Alex Williamson wrote:
> On Tue, 15 Sep 2020 13:03:01 -0600
> Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
> 
> > On Tue, 15 Sep 2020 08:30:42 +0800
> > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote:
> > 
> > > vfio_pin_pages() and vfio_group_pin_pages() are used purely to mark dirty
> > > pages to devices with IOMMU backend as they already have all VM pages
> > > pinned at VM startup.  
> > 
> > This is wrong.  The entire initial basis of mdev devices is for
> > non-IOMMU backed devices which provide mediation outside of the scope
> > of the IOMMU.  That mediation includes interpreting device DMA
> > programming and making use of the vfio_pin_pages() interface to
> > translate and pin IOVA address to HPA.  Marking pages dirty is a
> > secondary feature.
> > 
> > > when there're multiple devices in the vfio group, the dirty pages
> > > marked through pin_pages interface by one device is not useful as the
> > > other devices may access and dirty any VM pages.  
> > 
> > I don't know of any cases where there are multiple devices in a group
> > that would make use of this interface, however, all devices within a
> > group necessarily share an IOMMU context and any one device dirtying a
> > page will dirty that page for all devices, so I don't see that this is
> > a valid statement either.
> > 
> > > So added a check such that only singleton IOMMU groups can pin pages
> > > in vfio_group_pin_pages. for mdevs, there's always only one dev in a
> > > vfio group.
> > > This is a fix to the commit below that added a singleton IOMMU group
> > > check in vfio_pin_pages.  
> > 
> > None of the justification above is accurate, please try again.  Thanks,
> 
> FWIW, I think this should read something like "Page pinning is used
> both to translate and pin device mappings for DMA purpose, as well as
> to indicate to the IOMMU backend to limit the dirty page scope to those
> pages that have been pinned, in the case of an IOMMU backed device.  To
> support this, the vfio_pin_pages() interface limits itself to only
> singleton groups such that the IOMMU backend can consider dirty page
> scope only at the group level.  Implement the same requirement for the
> vfio_group_pin_pages() interface."  Thanks,
> 
yes, I'm sorry that I didn't express the meaning clearly.
will resend it using this version.

Thanks
Yan


> 
> 
> > > Fixes: 95fc87b44104 (vfio: Selective dirty page tracking if IOMMU backed
> > > device pins pages)
> > > 
> > > Signed-off-by: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > > ---
> > >  drivers/vfio/vfio.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > > index 5e6e0511b5aa..2f0fa272ebf2 100644
> > > --- a/drivers/vfio/vfio.c
> > > +++ b/drivers/vfio/vfio.c
> > > @@ -2053,6 +2053,9 @@ int vfio_group_pin_pages(struct vfio_group *group,
> > >  	if (!group || !user_iova_pfn || !phys_pfn || !npage)
> > >  		return -EINVAL;
> > >  
> > > +	if (group->dev_counter > 1)
> > > +		return  -EINVAL;
> > > +
> > >  	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
> > >  		return -E2BIG;
> > >    
> > 
>