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