| Message ID | 20200915003042.14273-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: add a singleton check for vfio_group_pin_pages | expand |
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; >
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; > > >
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; > > > > > >
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;
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(+)