[PATCH] vfio: add a singleton check for vfio_group_pin_pages

[PATCH] vfio: add a singleton check for vfio_group_pin_pages

[Yan Zhao]

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

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.

Fixes: 95fc87b44104 (vfio: Selective dirty page tracking if IOMMU backed
device pins pages)

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
---
 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;
 
-- 
2.17.1

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

[Alex Williamson]

On Tue, 15 Sep 2020 08:30:42 +0800
Yan Zhao <yan.y.zhao@intel.com> 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,

Alex

> Fixes: 95fc87b44104 (vfio: Selective dirty page tracking if IOMMU backed
> device pins pages)
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
>  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;
>  

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

[Alex Williamson]

On Tue, 15 Sep 2020 13:03:01 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 15 Sep 2020 08:30:42 +0800
> Yan Zhao <yan.y.zhao@intel.com> 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,

Alex


> > Fixes: 95fc87b44104 (vfio: Selective dirty page tracking if IOMMU backed
> > device pins pages)
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > ---
> >  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;
> >    
> 

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@redhat.com> wrote:
> 
> > On Tue, 15 Sep 2020 08:30:42 +0800
> > Yan Zhao <yan.y.zhao@intel.com> 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@intel.com>
> > > ---
> > >  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;
> > >    
> > 
>