Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
On Thu, May 25, 2023 at 03:03:54AM +, Liu, Yi L wrote: > vIOMMU may introduce some performance deduction if there > are frequent map/unmap. DPDK doesn't do that. And once you turn on the HW IOMMU you negate alot of the micro performance wins of bypassing. Maybe there is still some argument about giant/huge pages or something. > without vIOMMU is supposed to be more robust. But I'm not > sure if the noiommu userspace will adapt to cdev noiommu. > Perhaps yes if group may be deprecated in future. I think that is more a distro question, and we don't have to answer it fully now. Jason
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
On Wed, May 24, 2023 at 09:31:42AM -0600, Alex Williamson wrote: > If a user creates an ioas within an iommufd, attaches a device to that > ioas and populates it with mappings, wouldn't the user expect the > device to have access to and honor those mappings? I think that's the > path we're headed down if we report a successful attach of a noiommu > device to an ioas. I understand we are going to drop no-iommu from this series, so this below is not relavent. But to clarify my general design idea here again The IOAS contains the mappings that userspace would like to use with no-iommu. Userspace would use a new IOCTL to pin and return the DMA addr's of those exact mappings. So attaching a noiommu to an IOAS is a necessary operation that should succeed. It doesn't make full API sense until we also get an ioctl to return the dma_addr_t lists. What is special about no-iommu is that the mapppings have to go through the special ioctl API to pin and translate, the IOVA cannot be used natively as a dma_addr. The IOAS is still used and still related to the device, it just for pinning and dma_addr generation not HW isolation. > We need to keep in mind that noiommu was meant to be a minimally > intrusive mechanism to provide a dummy vfio IOMMU backend and satisfy > the group requirements, solely for the purpose of making use of the > vfio device interface and without providing any DMA mapping services or > expectations. Well, no-iommu turned into a total hack job as soon as it wrongly relied on mlock() and /proc/ files to function. Even within its defined limitations this is an incorrect way to use the mm and DMA APIs. Memory under DMA must be locked using pin_user_pages(), mlock is not a substitution. I expect this is functionally broken these days, under some workloads, on certain kernel configurations. Even if we don't fully implement it, I prefer to imagine a design where no-iommu is implemented correctly and orient things toward that. > beyond the minimal code trickery of the legacy implementation. I hate > to ask, but could we reiterate our requirements for noiommu as a part of > the native iommufd interface for vfio? The nested userspace requirement > is gone now that hypervisors have vIOMMU support, so my assumption is > that this is only for bare metal systems without an IOMMU, which > ideally are less and less prevalent. I understood there was some desire for DPDK users to do this for higher performance on some systems. > that are actually going to adopt the noiommu cdev interface? What > terrible things happen if noiommu only exists in the vfio group compat > interface to iommufd and at some distant point in the future dies when > that gets disabled? I think it is fine, it is only for DPDK and if DPDK people really really care about this then they can implement it properly someday. I'm quite happy if we say we will not put no-iommu into the device cdev until it is put in fully correctly without relying on mlock/etc. Then the API construction would make alot more sense. Jason
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
> From: Alex Williamson
> Sent: Friday, May 26, 2023 12:00 AM
>
> On Thu, 25 May 2023 03:03:54 +
> "Liu, Yi L" wrote:
>
> > > From: Alex Williamson
> > > Sent: Wednesday, May 24, 2023 11:32 PM
> > >
> > > On Wed, 24 May 2023 02:12:14 +
> > > "Liu, Yi L" wrote:
> > >
> > > > > From: Alex Williamson
> > > > > Sent: Tuesday, May 23, 2023 11:50 PM
> > > > >
> > > > > On Tue, 23 May 2023 01:20:17 +
> > > > > "Liu, Yi L" wrote:
> > > > >
> > > > > > > From: Alex Williamson
> > > > > > > Sent: Tuesday, May 23, 2023 6:16 AM
> > > > > > >
> > > > > > > On Sat, 13 May 2023 06:28:24 -0700
> > > > > > > Yi Liu wrote:
> > > > > > >
> > > > > > > > return kasprintf(GFP_KERNEL, "vfio/devices/%s",
> > > > > > > > dev_name(dev));
> > > > > > > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > > > > > > > index 83575b65ea01..799ea322a7d4 100644
> > > > > > > > --- a/drivers/vfio/iommufd.c
> > > > > > > > +++ b/drivers/vfio/iommufd.c
> > > > > > > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct
> > > > > > > > vfio_device_file
> *df)
> > > > > > > > vdev->ops->unbind_iommufd(vdev);
> > > > > > > > }
> > > > > > > >
> > > > > > > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> > > > > > > > +{
> > > > > > > > + lockdep_assert_held(>dev_set->lock);
> > > > > > > > +
> > > > > > > > + if (vfio_device_is_noiommu(vdev))
> > > > > > > > + return 0;
> > > > > > >
> > > > > > > Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL?
> > > > > > > We
> > > > > > > return success and copy back the provided pt_id, why would a user
> > > > > > > not
> > > > > > > consider it a bug that they can't use whatever value was there
> > > > > > > with
> > > > > > > iommufd?
> > > > > >
> > > > > > Yes, this is the question I asked in [1]. At that time, it appears
> > > > > > to me
> > > > > > that better to allow it [2]. Maybe it's more suitable to ask it
> > > > > > here.
> > > > >
> > > > > From an API perspective it seems wrong. We return success without
> > > > > doing anything. A user would be right to consider it a bug that the
> > > > > attach operation works but there's not actually any association to the
> > > > > IOAS. Thanks,
> > > >
> > > > The current version is kind of tradeoff based on prior remarks when
> > > > I asked the question. As prior comment[2], it appears to me the attach
> > > > shall success for noiommu devices as well, but per your remark it seems
> > > > not in plan. So anyway, we may just fail the attach/detach for noiommu
> > > > devices. Is it?
> > >
> > > If a user creates an ioas within an iommufd, attaches a device to that
> > > ioas and populates it with mappings, wouldn't the user expect the
> > > device to have access to and honor those mappings? I think that's the
> > > path we're headed down if we report a successful attach of a noiommu
> > > device to an ioas.
> >
> > makes sense. Let's just fail attach/detach for noiommu devices.
> >
> > >
> > > We need to keep in mind that noiommu was meant to be a minimally
> > > intrusive mechanism to provide a dummy vfio IOMMU backend and satisfy
> > > the group requirements, solely for the purpose of making use of the
> > > vfio device interface and without providing any DMA mapping services or
> > > expectations. IMO, an argument that we need the attach op to succeed in
> > > order to avoid too much disruption in userspace code is nonsense. On
> > > the contrary, userspace needs to be very aware of this difference and
> > > we shouldn't invest effort trying to make noiommu more convenient to
> > > use. It's inherently unsafe.
> > >
> > > I'm not fond of what a mess noiommu has become with cdev, we're well
> > > beyond the minimal code trickery of the legacy implementation. I hate
> > > to ask, but could we reiterate our requirements for noiommu as a part of
> > > the native iommufd interface for vfio? The nested userspace requirement
> > > is gone now that hypervisors have vIOMMU support, so my assumption is
> > > that this is only for bare metal systems without an IOMMU, which
> > > ideally are less and less prevalent. Are there any noiommu userspaces
> > > that are actually going to adopt the noiommu cdev interface? What
> > > terrible things happen if noiommu only exists in the vfio group compat
> > > interface to iommufd and at some distant point in the future dies when
> > > that gets disabled?
> >
> > vIOMMU may introduce some performance deduction if there
> > are frequent map/unmap.
>
> We use passthrough mode of the vIOMMU to negate that overhead for guest
> drivers and vfio drivers have typically learned by now that dynamic
> mappings using the vfio type1 mapping API are a bad idea.
Yes, this can avoid this overhead.
>
> > As far as I know, some cloud service
> > providers are more willing to use noiommu mode within VM.
>
> Sure, the VM itself is still isolated by the host IOMMU, but it's
> clearly an extra
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
On Thu, 25 May 2023 03:03:54 +
"Liu, Yi L" wrote:
> > From: Alex Williamson
> > Sent: Wednesday, May 24, 2023 11:32 PM
> >
> > On Wed, 24 May 2023 02:12:14 +
> > "Liu, Yi L" wrote:
> >
> > > > From: Alex Williamson
> > > > Sent: Tuesday, May 23, 2023 11:50 PM
> > > >
> > > > On Tue, 23 May 2023 01:20:17 +
> > > > "Liu, Yi L" wrote:
> > > >
> > > > > > From: Alex Williamson
> > > > > > Sent: Tuesday, May 23, 2023 6:16 AM
> > > > > >
> > > > > > On Sat, 13 May 2023 06:28:24 -0700
> > > > > > Yi Liu wrote:
> > > > > >
> > > > > > > return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> > > > > > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > > > > > > index 83575b65ea01..799ea322a7d4 100644
> > > > > > > --- a/drivers/vfio/iommufd.c
> > > > > > > +++ b/drivers/vfio/iommufd.c
> > > > > > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct
> > > > > > > vfio_device_file *df)
> > > > > > > vdev->ops->unbind_iommufd(vdev);
> > > > > > > }
> > > > > > >
> > > > > > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> > > > > > > +{
> > > > > > > + lockdep_assert_held(>dev_set->lock);
> > > > > > > +
> > > > > > > + if (vfio_device_is_noiommu(vdev))
> > > > > > > + return 0;
> > > > > >
> > > > > > Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL? We
> > > > > > return success and copy back the provided pt_id, why would a user
> > > > > > not
> > > > > > consider it a bug that they can't use whatever value was there with
> > > > > > iommufd?
> > > > >
> > > > > Yes, this is the question I asked in [1]. At that time, it appears to
> > > > > me
> > > > > that better to allow it [2]. Maybe it's more suitable to ask it here.
> > > > >
> > > >
> > > > From an API perspective it seems wrong. We return success without
> > > > doing anything. A user would be right to consider it a bug that the
> > > > attach operation works but there's not actually any association to the
> > > > IOAS. Thanks,
> > >
> > > The current version is kind of tradeoff based on prior remarks when
> > > I asked the question. As prior comment[2], it appears to me the attach
> > > shall success for noiommu devices as well, but per your remark it seems
> > > not in plan. So anyway, we may just fail the attach/detach for noiommu
> > > devices. Is it?
> >
> > If a user creates an ioas within an iommufd, attaches a device to that
> > ioas and populates it with mappings, wouldn't the user expect the
> > device to have access to and honor those mappings? I think that's the
> > path we're headed down if we report a successful attach of a noiommu
> > device to an ioas.
>
> makes sense. Let's just fail attach/detach for noiommu devices.
>
> >
> > We need to keep in mind that noiommu was meant to be a minimally
> > intrusive mechanism to provide a dummy vfio IOMMU backend and satisfy
> > the group requirements, solely for the purpose of making use of the
> > vfio device interface and without providing any DMA mapping services or
> > expectations. IMO, an argument that we need the attach op to succeed in
> > order to avoid too much disruption in userspace code is nonsense. On
> > the contrary, userspace needs to be very aware of this difference and
> > we shouldn't invest effort trying to make noiommu more convenient to
> > use. It's inherently unsafe.
> >
> > I'm not fond of what a mess noiommu has become with cdev, we're well
> > beyond the minimal code trickery of the legacy implementation. I hate
> > to ask, but could we reiterate our requirements for noiommu as a part of
> > the native iommufd interface for vfio? The nested userspace requirement
> > is gone now that hypervisors have vIOMMU support, so my assumption is
> > that this is only for bare metal systems without an IOMMU, which
> > ideally are less and less prevalent. Are there any noiommu userspaces
> > that are actually going to adopt the noiommu cdev interface? What
> > terrible things happen if noiommu only exists in the vfio group compat
> > interface to iommufd and at some distant point in the future dies when
> > that gets disabled?
>
> vIOMMU may introduce some performance deduction if there
> are frequent map/unmap.
We use passthrough mode of the vIOMMU to negate that overhead for guest
drivers and vfio drivers have typically learned by now that dynamic
mappings using the vfio type1 mapping API are a bad idea.
> As far as I know, some cloud service
> providers are more willing to use noiommu mode within VM.
Sure, the VM itself is still isolated by the host IOMMU, but it's
clearly an extra maintenance and development burden when we should
instead be encouraging those use cases to use vIOMMU rather than
porting to a different noiommu uAPI. Even if the host is not exposed,
any sort of security and support best practices in the guest should
favor a vIOMMU solution.
> Besides the performance consideration, using a booting a VM
> without vIOMMU
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
> From: Alex Williamson
> Sent: Wednesday, May 24, 2023 11:32 PM
>
> On Wed, 24 May 2023 02:12:14 +
> "Liu, Yi L" wrote:
>
> > > From: Alex Williamson
> > > Sent: Tuesday, May 23, 2023 11:50 PM
> > >
> > > On Tue, 23 May 2023 01:20:17 +
> > > "Liu, Yi L" wrote:
> > >
> > > > > From: Alex Williamson
> > > > > Sent: Tuesday, May 23, 2023 6:16 AM
> > > > >
> > > > > On Sat, 13 May 2023 06:28:24 -0700
> > > > > Yi Liu wrote:
> > > > >
> > > > > > return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> > > > > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > > > > > index 83575b65ea01..799ea322a7d4 100644
> > > > > > --- a/drivers/vfio/iommufd.c
> > > > > > +++ b/drivers/vfio/iommufd.c
> > > > > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct
> > > > > > vfio_device_file *df)
> > > > > > vdev->ops->unbind_iommufd(vdev);
> > > > > > }
> > > > > >
> > > > > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> > > > > > +{
> > > > > > + lockdep_assert_held(>dev_set->lock);
> > > > > > +
> > > > > > + if (vfio_device_is_noiommu(vdev))
> > > > > > + return 0;
> > > > >
> > > > > Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL? We
> > > > > return success and copy back the provided pt_id, why would a user not
> > > > > consider it a bug that they can't use whatever value was there with
> > > > > iommufd?
> > > >
> > > > Yes, this is the question I asked in [1]. At that time, it appears to me
> > > > that better to allow it [2]. Maybe it's more suitable to ask it here.
> > >
> > > From an API perspective it seems wrong. We return success without
> > > doing anything. A user would be right to consider it a bug that the
> > > attach operation works but there's not actually any association to the
> > > IOAS. Thanks,
> >
> > The current version is kind of tradeoff based on prior remarks when
> > I asked the question. As prior comment[2], it appears to me the attach
> > shall success for noiommu devices as well, but per your remark it seems
> > not in plan. So anyway, we may just fail the attach/detach for noiommu
> > devices. Is it?
>
> If a user creates an ioas within an iommufd, attaches a device to that
> ioas and populates it with mappings, wouldn't the user expect the
> device to have access to and honor those mappings? I think that's the
> path we're headed down if we report a successful attach of a noiommu
> device to an ioas.
makes sense. Let's just fail attach/detach for noiommu devices.
>
> We need to keep in mind that noiommu was meant to be a minimally
> intrusive mechanism to provide a dummy vfio IOMMU backend and satisfy
> the group requirements, solely for the purpose of making use of the
> vfio device interface and without providing any DMA mapping services or
> expectations. IMO, an argument that we need the attach op to succeed in
> order to avoid too much disruption in userspace code is nonsense. On
> the contrary, userspace needs to be very aware of this difference and
> we shouldn't invest effort trying to make noiommu more convenient to
> use. It's inherently unsafe.
>
> I'm not fond of what a mess noiommu has become with cdev, we're well
> beyond the minimal code trickery of the legacy implementation. I hate
> to ask, but could we reiterate our requirements for noiommu as a part of
> the native iommufd interface for vfio? The nested userspace requirement
> is gone now that hypervisors have vIOMMU support, so my assumption is
> that this is only for bare metal systems without an IOMMU, which
> ideally are less and less prevalent. Are there any noiommu userspaces
> that are actually going to adopt the noiommu cdev interface? What
> terrible things happen if noiommu only exists in the vfio group compat
> interface to iommufd and at some distant point in the future dies when
> that gets disabled?
vIOMMU may introduce some performance deduction if there
are frequent map/unmap. As far as I know, some cloud service
providers are more willing to use noiommu mode within VM.
Besides the performance consideration, using a booting a VM
without vIOMMU is supposed to be more robust. But I'm not
sure if the noiommu userspace will adapt to cdev noiommu.
Perhaps yes if group may be deprecated in future.
> > btw. Should we document it somewhere as well? E.g. noiommu userspace
> > does not support attach/detach? Userspace should know it is opening
> > noiommu devices.
>
> Documentation never hurts. This is such a specialized use case I'm not
> sure we've bothered to do much documentation for noiommu previously.
Seems no, I didn't find special documentation for noiommu. Perhaps
a comment in the source code is enough. Depends on your taste.
Regards,
Yi Liu
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
On Wed, 24 May 2023 02:12:14 +
"Liu, Yi L" wrote:
> > From: Alex Williamson
> > Sent: Tuesday, May 23, 2023 11:50 PM
> >
> > On Tue, 23 May 2023 01:20:17 +
> > "Liu, Yi L" wrote:
> >
> > > > From: Alex Williamson
> > > > Sent: Tuesday, May 23, 2023 6:16 AM
> > > >
> > > > On Sat, 13 May 2023 06:28:24 -0700
> > > > Yi Liu wrote:
> > > >
> > > > > This adds ioctl for userspace to attach device cdev fd to and detach
> > > > > from IOAS/hw_pagetable managed by iommufd.
> > > > >
> > > > > VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS,
> > > > > hw_pagetable
> > > > > managed by iommufd. Attach can be
> > > > > undo by VFIO_DEVICE_DETACH_IOMMUFD_PT
> > > > > or device fd close.
> > > > > VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the
> > > > > current
> > attached
> > > > > IOAS or hw_pagetable managed by
> > > > > iommufd.
> > > > >
> > > > > Tested-by: Yanting Jiang
> > > > > Tested-by: Shameer Kolothum
> > > > > Signed-off-by: Yi Liu
> > > > > ---
> > > > > drivers/vfio/device_cdev.c | 66
> > > > > ++
> > > > > drivers/vfio/iommufd.c | 18 +++
> > > > > drivers/vfio/vfio.h| 18 +++
> > > > > drivers/vfio/vfio_main.c | 8 +
> > > > > include/uapi/linux/vfio.h | 52 ++
> > > > > 5 files changed, 162 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > > > > index 291cc678a18b..3f14edb80a93 100644
> > > > > --- a/drivers/vfio/device_cdev.c
> > > > > +++ b/drivers/vfio/device_cdev.c
> > > > > @@ -174,6 +174,72 @@ long vfio_device_ioctl_bind_iommufd(struct
> > vfio_device_file
> > > > *df,
> > > > > return ret;
> > > > > }
> > > > >
> > > > > +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> > > > > + struct vfio_device_attach_iommufd_pt
> > > > > __user *arg)
> > > > > +{
> > > > > + struct vfio_device *device = df->device;
> > > > > + struct vfio_device_attach_iommufd_pt attach;
> > > > > + unsigned long minsz;
> > > > > + int ret;
> > > > > +
> > > > > + minsz = offsetofend(struct vfio_device_attach_iommufd_pt,
> > > > > pt_id);
> > > > > +
> > > > > + if (copy_from_user(, arg, minsz))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + if (attach.argsz < minsz || attach.flags)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + /* ATTACH only allowed for cdev fds */
> > > > > + if (df->group)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + mutex_lock(>dev_set->lock);
> > > > > + ret = vfio_iommufd_attach(device, _id);
> > > > > + if (ret)
> > > > > + goto out_unlock;
> > > > > +
> > > > > + ret = copy_to_user(>pt_id, _id,
> > > > > +sizeof(attach.pt_id)) ? -EFAULT : 0;
> > > > > + if (ret)
> > > > > + goto out_detach;
> > > > > + mutex_unlock(>dev_set->lock);
> > > > > +
> > > > > + return 0;
> > > > > +
> > > > > +out_detach:
> > > > > + vfio_iommufd_detach(device);
> > > > > +out_unlock:
> > > > > + mutex_unlock(>dev_set->lock);
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> > > > > + struct vfio_device_detach_iommufd_pt
> > > > > __user *arg)
> > > > > +{
> > > > > + struct vfio_device *device = df->device;
> > > > > + struct vfio_device_detach_iommufd_pt detach;
> > > > > + unsigned long minsz;
> > > > > +
> > > > > + minsz = offsetofend(struct vfio_device_detach_iommufd_pt,
> > > > > flags);
> > > > > +
> > > > > + if (copy_from_user(, arg, minsz))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + if (detach.argsz < minsz || detach.flags)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + /* DETACH only allowed for cdev fds */
> > > > > + if (df->group)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + mutex_lock(>dev_set->lock);
> > > > > + vfio_iommufd_detach(device);
> > > > > + mutex_unlock(>dev_set->lock);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static char *vfio_device_devnode(const struct device *dev, umode_t
> > > > > *mode)
> > > > > {
> > > > > return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> > > > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > > > > index 83575b65ea01..799ea322a7d4 100644
> > > > > --- a/drivers/vfio/iommufd.c
> > > > > +++ b/drivers/vfio/iommufd.c
> > > > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct vfio_device_file
> > > > > *df)
> > > > > vdev->ops->unbind_iommufd(vdev);
> > > > > }
> > > > >
> > > > > +int vfio_iommufd_attach(struct
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
> From: Alex Williamson
> Sent: Tuesday, May 23, 2023 11:50 PM
>
> On Tue, 23 May 2023 01:20:17 +
> "Liu, Yi L" wrote:
>
> > > From: Alex Williamson
> > > Sent: Tuesday, May 23, 2023 6:16 AM
> > >
> > > On Sat, 13 May 2023 06:28:24 -0700
> > > Yi Liu wrote:
> > >
> > > > This adds ioctl for userspace to attach device cdev fd to and detach
> > > > from IOAS/hw_pagetable managed by iommufd.
> > > >
> > > > VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS,
> > > > hw_pagetable
> > > >managed by iommufd. Attach can be
> > > >undo by VFIO_DEVICE_DETACH_IOMMUFD_PT
> > > >or device fd close.
> > > > VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the current
> attached
> > > >IOAS or hw_pagetable managed by
> > > > iommufd.
> > > >
> > > > Tested-by: Yanting Jiang
> > > > Tested-by: Shameer Kolothum
> > > > Signed-off-by: Yi Liu
> > > > ---
> > > > drivers/vfio/device_cdev.c | 66 ++
> > > > drivers/vfio/iommufd.c | 18 +++
> > > > drivers/vfio/vfio.h| 18 +++
> > > > drivers/vfio/vfio_main.c | 8 +
> > > > include/uapi/linux/vfio.h | 52 ++
> > > > 5 files changed, 162 insertions(+)
> > > >
> > > > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > > > index 291cc678a18b..3f14edb80a93 100644
> > > > --- a/drivers/vfio/device_cdev.c
> > > > +++ b/drivers/vfio/device_cdev.c
> > > > @@ -174,6 +174,72 @@ long vfio_device_ioctl_bind_iommufd(struct
> vfio_device_file
> > > *df,
> > > > return ret;
> > > > }
> > > >
> > > > +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> > > > +struct vfio_device_attach_iommufd_pt
> > > > __user *arg)
> > > > +{
> > > > + struct vfio_device *device = df->device;
> > > > + struct vfio_device_attach_iommufd_pt attach;
> > > > + unsigned long minsz;
> > > > + int ret;
> > > > +
> > > > + minsz = offsetofend(struct vfio_device_attach_iommufd_pt,
> > > > pt_id);
> > > > +
> > > > + if (copy_from_user(, arg, minsz))
> > > > + return -EFAULT;
> > > > +
> > > > + if (attach.argsz < minsz || attach.flags)
> > > > + return -EINVAL;
> > > > +
> > > > + /* ATTACH only allowed for cdev fds */
> > > > + if (df->group)
> > > > + return -EINVAL;
> > > > +
> > > > + mutex_lock(>dev_set->lock);
> > > > + ret = vfio_iommufd_attach(device, _id);
> > > > + if (ret)
> > > > + goto out_unlock;
> > > > +
> > > > + ret = copy_to_user(>pt_id, _id,
> > > > + sizeof(attach.pt_id)) ? -EFAULT : 0;
> > > > + if (ret)
> > > > + goto out_detach;
> > > > + mutex_unlock(>dev_set->lock);
> > > > +
> > > > + return 0;
> > > > +
> > > > +out_detach:
> > > > + vfio_iommufd_detach(device);
> > > > +out_unlock:
> > > > + mutex_unlock(>dev_set->lock);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> > > > +struct vfio_device_detach_iommufd_pt
> > > > __user *arg)
> > > > +{
> > > > + struct vfio_device *device = df->device;
> > > > + struct vfio_device_detach_iommufd_pt detach;
> > > > + unsigned long minsz;
> > > > +
> > > > + minsz = offsetofend(struct vfio_device_detach_iommufd_pt,
> > > > flags);
> > > > +
> > > > + if (copy_from_user(, arg, minsz))
> > > > + return -EFAULT;
> > > > +
> > > > + if (detach.argsz < minsz || detach.flags)
> > > > + return -EINVAL;
> > > > +
> > > > + /* DETACH only allowed for cdev fds */
> > > > + if (df->group)
> > > > + return -EINVAL;
> > > > +
> > > > + mutex_lock(>dev_set->lock);
> > > > + vfio_iommufd_detach(device);
> > > > + mutex_unlock(>dev_set->lock);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static char *vfio_device_devnode(const struct device *dev, umode_t
> > > > *mode)
> > > > {
> > > > return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> > > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > > > index 83575b65ea01..799ea322a7d4 100644
> > > > --- a/drivers/vfio/iommufd.c
> > > > +++ b/drivers/vfio/iommufd.c
> > > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct vfio_device_file
> > > > *df)
> > > > vdev->ops->unbind_iommufd(vdev);
> > > > }
> > > >
> > > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> > > > +{
> > > > + lockdep_assert_held(>dev_set->lock);
> > > > +
> > > > + if (vfio_device_is_noiommu(vdev))
> > > > + return 0;
> > >
> > > Isn't this an invalid operation for a noiommu cdev,
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
On Tue, 23 May 2023 01:20:17 +
"Liu, Yi L" wrote:
> > From: Alex Williamson
> > Sent: Tuesday, May 23, 2023 6:16 AM
> >
> > On Sat, 13 May 2023 06:28:24 -0700
> > Yi Liu wrote:
> >
> > > This adds ioctl for userspace to attach device cdev fd to and detach
> > > from IOAS/hw_pagetable managed by iommufd.
> > >
> > > VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS,
> > > hw_pagetable
> > > managed by iommufd. Attach can be
> > > undo by VFIO_DEVICE_DETACH_IOMMUFD_PT
> > > or device fd close.
> > > VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the current
> > > attached
> > > IOAS or hw_pagetable managed by iommufd.
> > >
> > > Tested-by: Yanting Jiang
> > > Tested-by: Shameer Kolothum
> > > Signed-off-by: Yi Liu
> > > ---
> > > drivers/vfio/device_cdev.c | 66 ++
> > > drivers/vfio/iommufd.c | 18 +++
> > > drivers/vfio/vfio.h| 18 +++
> > > drivers/vfio/vfio_main.c | 8 +
> > > include/uapi/linux/vfio.h | 52 ++
> > > 5 files changed, 162 insertions(+)
> > >
> > > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > > index 291cc678a18b..3f14edb80a93 100644
> > > --- a/drivers/vfio/device_cdev.c
> > > +++ b/drivers/vfio/device_cdev.c
> > > @@ -174,6 +174,72 @@ long vfio_device_ioctl_bind_iommufd(struct
> > > vfio_device_file
> > *df,
> > > return ret;
> > > }
> > >
> > > +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> > > + struct vfio_device_attach_iommufd_pt __user *arg)
> > > +{
> > > + struct vfio_device *device = df->device;
> > > + struct vfio_device_attach_iommufd_pt attach;
> > > + unsigned long minsz;
> > > + int ret;
> > > +
> > > + minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
> > > +
> > > + if (copy_from_user(, arg, minsz))
> > > + return -EFAULT;
> > > +
> > > + if (attach.argsz < minsz || attach.flags)
> > > + return -EINVAL;
> > > +
> > > + /* ATTACH only allowed for cdev fds */
> > > + if (df->group)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(>dev_set->lock);
> > > + ret = vfio_iommufd_attach(device, _id);
> > > + if (ret)
> > > + goto out_unlock;
> > > +
> > > + ret = copy_to_user(>pt_id, _id,
> > > +sizeof(attach.pt_id)) ? -EFAULT : 0;
> > > + if (ret)
> > > + goto out_detach;
> > > + mutex_unlock(>dev_set->lock);
> > > +
> > > + return 0;
> > > +
> > > +out_detach:
> > > + vfio_iommufd_detach(device);
> > > +out_unlock:
> > > + mutex_unlock(>dev_set->lock);
> > > + return ret;
> > > +}
> > > +
> > > +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> > > + struct vfio_device_detach_iommufd_pt __user *arg)
> > > +{
> > > + struct vfio_device *device = df->device;
> > > + struct vfio_device_detach_iommufd_pt detach;
> > > + unsigned long minsz;
> > > +
> > > + minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
> > > +
> > > + if (copy_from_user(, arg, minsz))
> > > + return -EFAULT;
> > > +
> > > + if (detach.argsz < minsz || detach.flags)
> > > + return -EINVAL;
> > > +
> > > + /* DETACH only allowed for cdev fds */
> > > + if (df->group)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(>dev_set->lock);
> > > + vfio_iommufd_detach(device);
> > > + mutex_unlock(>dev_set->lock);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static char *vfio_device_devnode(const struct device *dev, umode_t *mode)
> > > {
> > > return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > > index 83575b65ea01..799ea322a7d4 100644
> > > --- a/drivers/vfio/iommufd.c
> > > +++ b/drivers/vfio/iommufd.c
> > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct vfio_device_file *df)
> > > vdev->ops->unbind_iommufd(vdev);
> > > }
> > >
> > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> > > +{
> > > + lockdep_assert_held(>dev_set->lock);
> > > +
> > > + if (vfio_device_is_noiommu(vdev))
> > > + return 0;
> >
> > Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL? We
> > return success and copy back the provided pt_id, why would a user not
> > consider it a bug that they can't use whatever value was there with
> > iommufd?
>
> Yes, this is the question I asked in [1]. At that time, it appears to me
> that better to allow it [2]. Maybe it's more suitable to ask it here.
>From an API perspective it seems wrong. We return success without
doing anything. A user would be right to consider it a bug that the
attach operation works but there's not actually any association to the
IOAS. Thanks,
Alex
> [1]
> https://lore.kernel.org/kvm/c203f11f-4d9f-cf43-03ab-e41a858bd...@intel.com/
> [2]
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
> From: Alex Williamson
> Sent: Tuesday, May 23, 2023 6:16 AM
>
> On Sat, 13 May 2023 06:28:24 -0700
> Yi Liu wrote:
>
> > This adds ioctl for userspace to attach device cdev fd to and detach
> > from IOAS/hw_pagetable managed by iommufd.
> >
> > VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS, hw_pagetable
> >managed by iommufd. Attach can be
> >undo by VFIO_DEVICE_DETACH_IOMMUFD_PT
> >or device fd close.
> > VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the current
> > attached
> >IOAS or hw_pagetable managed by iommufd.
> >
> > Tested-by: Yanting Jiang
> > Tested-by: Shameer Kolothum
> > Signed-off-by: Yi Liu
> > ---
> > drivers/vfio/device_cdev.c | 66 ++
> > drivers/vfio/iommufd.c | 18 +++
> > drivers/vfio/vfio.h| 18 +++
> > drivers/vfio/vfio_main.c | 8 +
> > include/uapi/linux/vfio.h | 52 ++
> > 5 files changed, 162 insertions(+)
> >
> > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > index 291cc678a18b..3f14edb80a93 100644
> > --- a/drivers/vfio/device_cdev.c
> > +++ b/drivers/vfio/device_cdev.c
> > @@ -174,6 +174,72 @@ long vfio_device_ioctl_bind_iommufd(struct
> > vfio_device_file
> *df,
> > return ret;
> > }
> >
> > +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> > +struct vfio_device_attach_iommufd_pt __user *arg)
> > +{
> > + struct vfio_device *device = df->device;
> > + struct vfio_device_attach_iommufd_pt attach;
> > + unsigned long minsz;
> > + int ret;
> > +
> > + minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
> > +
> > + if (copy_from_user(, arg, minsz))
> > + return -EFAULT;
> > +
> > + if (attach.argsz < minsz || attach.flags)
> > + return -EINVAL;
> > +
> > + /* ATTACH only allowed for cdev fds */
> > + if (df->group)
> > + return -EINVAL;
> > +
> > + mutex_lock(>dev_set->lock);
> > + ret = vfio_iommufd_attach(device, _id);
> > + if (ret)
> > + goto out_unlock;
> > +
> > + ret = copy_to_user(>pt_id, _id,
> > + sizeof(attach.pt_id)) ? -EFAULT : 0;
> > + if (ret)
> > + goto out_detach;
> > + mutex_unlock(>dev_set->lock);
> > +
> > + return 0;
> > +
> > +out_detach:
> > + vfio_iommufd_detach(device);
> > +out_unlock:
> > + mutex_unlock(>dev_set->lock);
> > + return ret;
> > +}
> > +
> > +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> > +struct vfio_device_detach_iommufd_pt __user *arg)
> > +{
> > + struct vfio_device *device = df->device;
> > + struct vfio_device_detach_iommufd_pt detach;
> > + unsigned long minsz;
> > +
> > + minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
> > +
> > + if (copy_from_user(, arg, minsz))
> > + return -EFAULT;
> > +
> > + if (detach.argsz < minsz || detach.flags)
> > + return -EINVAL;
> > +
> > + /* DETACH only allowed for cdev fds */
> > + if (df->group)
> > + return -EINVAL;
> > +
> > + mutex_lock(>dev_set->lock);
> > + vfio_iommufd_detach(device);
> > + mutex_unlock(>dev_set->lock);
> > +
> > + return 0;
> > +}
> > +
> > static char *vfio_device_devnode(const struct device *dev, umode_t *mode)
> > {
> > return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > index 83575b65ea01..799ea322a7d4 100644
> > --- a/drivers/vfio/iommufd.c
> > +++ b/drivers/vfio/iommufd.c
> > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct vfio_device_file *df)
> > vdev->ops->unbind_iommufd(vdev);
> > }
> >
> > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> > +{
> > + lockdep_assert_held(>dev_set->lock);
> > +
> > + if (vfio_device_is_noiommu(vdev))
> > + return 0;
>
> Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL? We
> return success and copy back the provided pt_id, why would a user not
> consider it a bug that they can't use whatever value was there with
> iommufd?
Yes, this is the question I asked in [1]. At that time, it appears to me
that better to allow it [2]. Maybe it's more suitable to ask it here.
[1] https://lore.kernel.org/kvm/c203f11f-4d9f-cf43-03ab-e41a858bd...@intel.com/
[2] https://lore.kernel.org/kvm/zffuyhqid+ltub%...@nvidia.com/
> > +
> > + return vdev->ops->attach_ioas(vdev, pt_id);
> > +}
> > +
> > +void vfio_iommufd_detach(struct vfio_device *vdev)
> > +{
> > + lockdep_assert_held(>dev_set->lock);
> > +
> > + if (!vfio_device_is_noiommu(vdev))
> > + vdev->ops->detach_ioas(vdev);
> > +}
> > +
> > struct iommufd_ctx *vfio_iommufd_physical_ictx(struct vfio_device *vdev)
> > {
> > if (vdev->iommufd_device)
> > diff --git
Re: [Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT
On Sat, 13 May 2023 06:28:24 -0700
Yi Liu wrote:
> This adds ioctl for userspace to attach device cdev fd to and detach
> from IOAS/hw_pagetable managed by iommufd.
>
> VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS, hw_pagetable
> managed by iommufd. Attach can be
> undo by VFIO_DEVICE_DETACH_IOMMUFD_PT
> or device fd close.
> VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the current
> attached
> IOAS or hw_pagetable managed by iommufd.
>
> Tested-by: Yanting Jiang
> Tested-by: Shameer Kolothum
> Signed-off-by: Yi Liu
> ---
> drivers/vfio/device_cdev.c | 66 ++
> drivers/vfio/iommufd.c | 18 +++
> drivers/vfio/vfio.h| 18 +++
> drivers/vfio/vfio_main.c | 8 +
> include/uapi/linux/vfio.h | 52 ++
> 5 files changed, 162 insertions(+)
>
> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> index 291cc678a18b..3f14edb80a93 100644
> --- a/drivers/vfio/device_cdev.c
> +++ b/drivers/vfio/device_cdev.c
> @@ -174,6 +174,72 @@ long vfio_device_ioctl_bind_iommufd(struct
> vfio_device_file *df,
> return ret;
> }
>
> +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> + struct vfio_device_attach_iommufd_pt __user *arg)
> +{
> + struct vfio_device *device = df->device;
> + struct vfio_device_attach_iommufd_pt attach;
> + unsigned long minsz;
> + int ret;
> +
> + minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
> +
> + if (copy_from_user(, arg, minsz))
> + return -EFAULT;
> +
> + if (attach.argsz < minsz || attach.flags)
> + return -EINVAL;
> +
> + /* ATTACH only allowed for cdev fds */
> + if (df->group)
> + return -EINVAL;
> +
> + mutex_lock(>dev_set->lock);
> + ret = vfio_iommufd_attach(device, _id);
> + if (ret)
> + goto out_unlock;
> +
> + ret = copy_to_user(>pt_id, _id,
> +sizeof(attach.pt_id)) ? -EFAULT : 0;
> + if (ret)
> + goto out_detach;
> + mutex_unlock(>dev_set->lock);
> +
> + return 0;
> +
> +out_detach:
> + vfio_iommufd_detach(device);
> +out_unlock:
> + mutex_unlock(>dev_set->lock);
> + return ret;
> +}
> +
> +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> + struct vfio_device_detach_iommufd_pt __user *arg)
> +{
> + struct vfio_device *device = df->device;
> + struct vfio_device_detach_iommufd_pt detach;
> + unsigned long minsz;
> +
> + minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
> +
> + if (copy_from_user(, arg, minsz))
> + return -EFAULT;
> +
> + if (detach.argsz < minsz || detach.flags)
> + return -EINVAL;
> +
> + /* DETACH only allowed for cdev fds */
> + if (df->group)
> + return -EINVAL;
> +
> + mutex_lock(>dev_set->lock);
> + vfio_iommufd_detach(device);
> + mutex_unlock(>dev_set->lock);
> +
> + return 0;
> +}
> +
> static char *vfio_device_devnode(const struct device *dev, umode_t *mode)
> {
> return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> index 83575b65ea01..799ea322a7d4 100644
> --- a/drivers/vfio/iommufd.c
> +++ b/drivers/vfio/iommufd.c
> @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct vfio_device_file *df)
> vdev->ops->unbind_iommufd(vdev);
> }
>
> +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> +{
> + lockdep_assert_held(>dev_set->lock);
> +
> + if (vfio_device_is_noiommu(vdev))
> + return 0;
Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL? We
return success and copy back the provided pt_id, why would a user not
consider it a bug that they can't use whatever value was there with
iommufd?
> +
> + return vdev->ops->attach_ioas(vdev, pt_id);
> +}
> +
> +void vfio_iommufd_detach(struct vfio_device *vdev)
> +{
> + lockdep_assert_held(>dev_set->lock);
> +
> + if (!vfio_device_is_noiommu(vdev))
> + vdev->ops->detach_ioas(vdev);
> +}
> +
> struct iommufd_ctx *vfio_iommufd_physical_ictx(struct vfio_device *vdev)
> {
> if (vdev->iommufd_device)
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 8b359a7794be..50553f67600f 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -241,6 +241,8 @@ int vfio_iommufd_bind(struct vfio_device_file *df);
> void vfio_iommufd_unbind(struct vfio_device_file *df);
> int vfio_iommufd_compat_attach_ioas(struct vfio_device *device,
> struct iommufd_ctx *ictx);
> +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id);
> +void vfio_iommufd_detach(struct
