| Message ID | 4-v3-50561e12d92b+313-vfio_iommufd_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Connect VFIO to IOMMUFD | expand |
On Wed, 16 Nov 2022 17:05:29 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > This legacy module knob has become uAPI, when set on the vfio_iommu_type1 > it disables some security protections in the iommu drivers. Move the > storage for this knob to vfio_main.c so that iommufd can access it too. > > The may need enhancing as we learn more about how necessary > allow_unsafe_interrupts is in the current state of the world. If vfio > container is disabled then this option will not be available to the user. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Yi Liu <yi.l.liu@intel.com> > Tested-by: Lixiao Yang <lixiao.yang@intel.com> > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > Tested-by: Yu He <yu.he@intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.h | 2 ++ > drivers/vfio/vfio_iommu_type1.c | 5 ++--- > drivers/vfio/vfio_main.c | 3 +++ > 3 files changed, 7 insertions(+), 3 deletions(-) It's really quite trivial to convert to a vfio_iommu.ko module to host a separate option for this. Half of the patch below is undoing what's done here. Is your only concern with this approach that we use a few KB more memory for the separate module? I don't think a per-device sysfs setting makes a lot of sense, if we're on a host w/o MSI isolation, all devices are affected. Thanks, Alex Makefile | 4 +++- iommufd.c | 12 +++++++++++- vfio.h | 2 -- vfio_iommu_type1.c | 5 +++-- vfio_main.c | 6 +++--- 5 files changed, 20 insertions(+), 9 deletions(-) diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index b953517dc70f..34b7d91112e5 100644 --- a/drivers/vfio/Makefile +++ b/drivers/vfio/Makefile @@ -5,9 +5,11 @@ obj-$(CONFIG_VFIO) += vfio.o vfio-y += vfio_main.o \ iova_bitmap.o -vfio-$(CONFIG_IOMMUFD) += iommufd.o vfio-$(CONFIG_VFIO_CONTAINER) += container.o +obj-$(CONFIG_IOMMUFD) += vfio_iommufd.o +vfio_iommufd-y += iommufd.o + obj-$(CONFIG_VFIO_VIRQFD) += vfio_virqfd.o obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index dad8935d71f7..b6b4038ba036 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -10,6 +10,12 @@ MODULE_IMPORT_NS(IOMMUFD); MODULE_IMPORT_NS(IOMMUFD_VFIO); +static bool allow_unsafe_interrupts; +module_param_named(allow_unsafe_interrupts, + allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(allow_unsafe_interrupts, + "Enable VFIO IOMMUFD support on platforms without MSI/X isolation facilities."); + int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) { u32 ioas_id; @@ -47,6 +53,7 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) vdev->ops->unbind_iommufd(vdev); return ret; } +EXPORT_SYMBOL_GPL(vfio_iommufd_bind); void vfio_iommufd_unbind(struct vfio_device *vdev) { @@ -55,6 +62,7 @@ void vfio_iommufd_unbind(struct vfio_device *vdev) if (vdev->ops->unbind_iommufd) vdev->ops->unbind_iommufd(vdev); } +EXPORT_SYMBOL_GPL(vfio_iommufd_unbind); /* * The physical standard ops mean that the iommufd_device is bound to the @@ -92,7 +100,7 @@ int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id) unsigned int flags = 0; int rc; - if (vfio_allow_unsafe_interrupts) + if (allow_unsafe_interrupts) flags |= IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT; rc = iommufd_device_attach(vdev->iommufd_device, pt_id, flags); if (rc) @@ -159,3 +167,5 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id) return 0; } EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_attach_ioas); + +MODULE_LICENSE("GPL v2"); diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index 3378714a7462..ce5fe3fc493b 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -216,6 +216,4 @@ extern bool vfio_noiommu __read_mostly; enum { vfio_noiommu = false }; #endif -extern bool vfio_allow_unsafe_interrupts; - #endif diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 186e33a006d3..23c24fe98c00 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -44,8 +44,9 @@ #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" #define DRIVER_DESC "Type1 IOMMU driver for VFIO" +static bool allow_unsafe_interrupts; module_param_named(allow_unsafe_interrupts, - vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); + allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); MODULE_PARM_DESC(allow_unsafe_interrupts, "Enable VFIO IOMMU support for on platforms without interrupt remapping support."); @@ -2281,7 +2282,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP, vfio_iommu_device_capable); - if (!vfio_allow_unsafe_interrupts && !msi_remap) { + if (!allow_unsafe_interrupts && !msi_remap) { pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n", __func__); ret = -EPERM; diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index f3c48b8c4562..48b3aa8582aa 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -42,6 +42,9 @@ #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" #define DRIVER_DESC "VFIO - User Level meta-driver" +MODULE_IMPORT_NS(IOMMUFD); +MODULE_IMPORT_NS(IOMMUFD_VFIO); + static struct vfio { struct class *class; struct list_head group_list; @@ -52,9 +55,6 @@ static struct vfio { struct ida device_ida; } vfio; -bool vfio_allow_unsafe_interrupts; -EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts); - static DEFINE_XARRAY(vfio_device_set_xa); static const struct file_operations vfio_group_fops;
On Thu, Nov 17, 2022 at 01:14:51PM -0700, Alex Williamson wrote: > On Wed, 16 Nov 2022 17:05:29 -0400 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > This legacy module knob has become uAPI, when set on the vfio_iommu_type1 > > it disables some security protections in the iommu drivers. Move the > > storage for this knob to vfio_main.c so that iommufd can access it too. > > > > The may need enhancing as we learn more about how necessary > > allow_unsafe_interrupts is in the current state of the world. If vfio > > container is disabled then this option will not be available to the user. > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > Tested-by: Yi Liu <yi.l.liu@intel.com> > > Tested-by: Lixiao Yang <lixiao.yang@intel.com> > > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > > Tested-by: Yu He <yu.he@intel.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/vfio/vfio.h | 2 ++ > > drivers/vfio/vfio_iommu_type1.c | 5 ++--- > > drivers/vfio/vfio_main.c | 3 +++ > > 3 files changed, 7 insertions(+), 3 deletions(-) > > It's really quite trivial to convert to a vfio_iommu.ko module to host > a separate option for this. Half of the patch below is undoing what's > done here. Is your only concern with this approach that we use a few > KB more memory for the separate module? My main dislike is that it just seems arbitary to shunt iommufd support to a module when it is always required by vfio.ko. In general if you have a module that is only ever used by 1 other module, you should probably just combine them. It saves memory and simplifies operation (eg you don't have to unload a zoo of modules during development testing) If it wasn't for the module option ABI problem I would propse to merge vfio_type1/spapr into vfio.ko too - vfio.ko doesn't work without them and the module soft dependencies are hint something is weird here. An alternative suggestion is to just retain a stub vfio_iommu_type1.ko which only exposes the module option if iommufd is enabled. At least this would preserve the semi-ABI. However, if you think this fits your vision for VFIO better I will take it. > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 186e33a006d3..23c24fe98c00 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -44,8 +44,9 @@ > #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" > #define DRIVER_DESC "Type1 IOMMU driver for VFIO" > > +static bool allow_unsafe_interrupts; > module_param_named(allow_unsafe_interrupts, > - vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); > + allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); > MODULE_PARM_DESC(allow_unsafe_interrupts, > "Enable VFIO IOMMU support for on platforms without > interrupt remapping support."); Except this, I think we still should have iommufd compat with the current module ABI, so this should still get moved into vfio.ko and both vfio_iommu_type1.ko and vfio_iommufd.ko should jointly manipulate the same memory with their module options. Thanks, Jason
On Fri, 18 Nov 2022 11:36:14 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Thu, Nov 17, 2022 at 01:14:51PM -0700, Alex Williamson wrote: > > On Wed, 16 Nov 2022 17:05:29 -0400 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > This legacy module knob has become uAPI, when set on the vfio_iommu_type1 > > > it disables some security protections in the iommu drivers. Move the > > > storage for this knob to vfio_main.c so that iommufd can access it too. > > > > > > The may need enhancing as we learn more about how necessary > > > allow_unsafe_interrupts is in the current state of the world. If vfio > > > container is disabled then this option will not be available to the user. > > > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > > Tested-by: Yi Liu <yi.l.liu@intel.com> > > > Tested-by: Lixiao Yang <lixiao.yang@intel.com> > > > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > > > Tested-by: Yu He <yu.he@intel.com> > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > --- > > > drivers/vfio/vfio.h | 2 ++ > > > drivers/vfio/vfio_iommu_type1.c | 5 ++--- > > > drivers/vfio/vfio_main.c | 3 +++ > > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > It's really quite trivial to convert to a vfio_iommu.ko module to host > > a separate option for this. Half of the patch below is undoing what's > > done here. Is your only concern with this approach that we use a few > > KB more memory for the separate module? > > My main dislike is that it just seems arbitary to shunt iommufd > support to a module when it is always required by vfio.ko. In general > if you have a module that is only ever used by 1 other module, you > should probably just combine them. It saves memory and simplifies > operation (eg you don't have to unload a zoo of modules during > development testing) These are all great reasons for why iommufd should host this option, as it's fundamentally part of the DMA isolation of the device, which vfio relies on iommufd to provide in this case. Adding a module option to vfio.ko conflicts with the existing option on vfio_iommu_type1.ko, which leads to the problem of duplicate module options manipulating the same variable, or options that disappear when other modules are deprecated, which are both issues I have with the originally proposed code. Since iommufd won't accept such an option, we maintain the logical association via modularizing the vfio interface to iommufd. > If it wasn't for the module option ABI problem I would propse to merge > vfio_type1/spapr into vfio.ko too - vfio.ko doesn't work without them > and the module soft dependencies are hint something is weird here. In fact no-iommu mode works w/o them. > An alternative suggestion is to just retain a stub vfio_iommu_type1.ko > which only exposes the module option if iommufd is enabled. At least > this would preserve the semi-ABI. See below. > However, if you think this fits your vision for VFIO better I will > take it. > > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > > index 186e33a006d3..23c24fe98c00 100644 > > --- a/drivers/vfio/vfio_iommu_type1.c > > +++ b/drivers/vfio/vfio_iommu_type1.c > > @@ -44,8 +44,9 @@ > > #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" > > #define DRIVER_DESC "Type1 IOMMU driver for VFIO" > > > > +static bool allow_unsafe_interrupts; > > module_param_named(allow_unsafe_interrupts, > > - vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); > > + allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); > > MODULE_PARM_DESC(allow_unsafe_interrupts, > > "Enable VFIO IOMMU support for on platforms without > > interrupt remapping support."); > > Except this, I think we still should have iommufd compat with the > current module ABI, so this should still get moved into vfio.ko and > both vfio_iommu_type1.ko and vfio_iommufd.ko should jointly manipulate > the same memory with their module options. Modules implicitly interacting in this way is exactly what I find so terrible in the original proposal. The idea of a stub type1 module to preserve that uAPI was only proposed as a known terrible solution to the problem. The more I think about it, the less convinced I am that the kernel has a responsibility to automatically carry forward the behavior of a module option for a module that's no longer used. The only way we make use of IOMMUFD is if either the distribution has opt'd to disable VFIO_CONTAINER and enable IOMMUFD emulation or userspace has opt'd to pass an iommufd to vfio rather than a container. The former could only be forced after a deprecation period for VFIO_CONTAINER, after which I think it's fair to require a re-opt-in by the user. In the latter case, userspace is intentionally choosing to use a highly compatible uAPI, but nonetheless, it's still a different uAPI. Thus the proposal here for a separate, but equivalent module option in the vfio interface to iommufd. Let's also not forget that at it's core, this option is an opt-in to allow less secure configurations. It's prudent to tread lightly with automatically carrying it forward. Thanks, Alex
On Fri, Nov 18, 2022 at 01:36:46PM -0700, Alex Williamson wrote: > On Fri, 18 Nov 2022 11:36:14 -0400 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > On Thu, Nov 17, 2022 at 01:14:51PM -0700, Alex Williamson wrote: > > > On Wed, 16 Nov 2022 17:05:29 -0400 > > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > > > This legacy module knob has become uAPI, when set on the vfio_iommu_type1 > > > > it disables some security protections in the iommu drivers. Move the > > > > storage for this knob to vfio_main.c so that iommufd can access it too. > > > > > > > > The may need enhancing as we learn more about how necessary > > > > allow_unsafe_interrupts is in the current state of the world. If vfio > > > > container is disabled then this option will not be available to the user. > > > > > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > > > Tested-by: Yi Liu <yi.l.liu@intel.com> > > > > Tested-by: Lixiao Yang <lixiao.yang@intel.com> > > > > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > > > > Tested-by: Yu He <yu.he@intel.com> > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > > --- > > > > drivers/vfio/vfio.h | 2 ++ > > > > drivers/vfio/vfio_iommu_type1.c | 5 ++--- > > > > drivers/vfio/vfio_main.c | 3 +++ > > > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > > > It's really quite trivial to convert to a vfio_iommu.ko module to host > > > a separate option for this. Half of the patch below is undoing what's > > > done here. Is your only concern with this approach that we use a few > > > KB more memory for the separate module? > > > > My main dislike is that it just seems arbitary to shunt iommufd > > support to a module when it is always required by vfio.ko. In general > > if you have a module that is only ever used by 1 other module, you > > should probably just combine them. It saves memory and simplifies > > operation (eg you don't have to unload a zoo of modules during > > development testing) > > These are all great reasons for why iommufd should host this option, as > it's fundamentally part of the DMA isolation of the device, which vfio > relies on iommufd to provide in this case. Fine, lets do that. > > Except this, I think we still should have iommufd compat with the > > current module ABI, so this should still get moved into vfio.ko and > > both vfio_iommu_type1.ko and vfio_iommufd.ko should jointly manipulate > > the same memory with their module options. > > Modules implicitly interacting in this way is exactly what I find so > terrible in the original proposal. The idea of a stub type1 module to > preserve that uAPI was only proposed as a known terrible solution to the > problem. And I take it you prefer we remove this compat code as well and just leave the module option on vfio_type1 non-working? > think it's fair to require a re-opt-in by the user. In the latter > case, userspace is intentionally choosing to use a highly compatible > uAPI, but nonetheless, it's still a different uAPI. Well, the later case is likely going to be a choice made by the distribution, eg I would expect that libvirt will start automatically favoring iommufd if it is available. So, instructions someone might find saying to tweak the module option and then use libvirt are going to stop working at some point. Thanks, Jason
On Mon, 21 Nov 2022 21:59:28 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Fri, Nov 18, 2022 at 01:36:46PM -0700, Alex Williamson wrote: > > On Fri, 18 Nov 2022 11:36:14 -0400 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > On Thu, Nov 17, 2022 at 01:14:51PM -0700, Alex Williamson wrote: > > > > On Wed, 16 Nov 2022 17:05:29 -0400 > > > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > > > > > This legacy module knob has become uAPI, when set on the vfio_iommu_type1 > > > > > it disables some security protections in the iommu drivers. Move the > > > > > storage for this knob to vfio_main.c so that iommufd can access it too. > > > > > > > > > > The may need enhancing as we learn more about how necessary > > > > > allow_unsafe_interrupts is in the current state of the world. If vfio > > > > > container is disabled then this option will not be available to the user. > > > > > > > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > > > > Tested-by: Yi Liu <yi.l.liu@intel.com> > > > > > Tested-by: Lixiao Yang <lixiao.yang@intel.com> > > > > > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > > > > > Tested-by: Yu He <yu.he@intel.com> > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > > > --- > > > > > drivers/vfio/vfio.h | 2 ++ > > > > > drivers/vfio/vfio_iommu_type1.c | 5 ++--- > > > > > drivers/vfio/vfio_main.c | 3 +++ > > > > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > > > > > It's really quite trivial to convert to a vfio_iommu.ko module to host > > > > a separate option for this. Half of the patch below is undoing what's > > > > done here. Is your only concern with this approach that we use a few > > > > KB more memory for the separate module? > > > > > > My main dislike is that it just seems arbitary to shunt iommufd > > > support to a module when it is always required by vfio.ko. In general > > > if you have a module that is only ever used by 1 other module, you > > > should probably just combine them. It saves memory and simplifies > > > operation (eg you don't have to unload a zoo of modules during > > > development testing) > > > > These are all great reasons for why iommufd should host this option, as > > it's fundamentally part of the DMA isolation of the device, which vfio > > relies on iommufd to provide in this case. > > Fine, lets do that. > > > > Except this, I think we still should have iommufd compat with the > > > current module ABI, so this should still get moved into vfio.ko and > > > both vfio_iommu_type1.ko and vfio_iommufd.ko should jointly manipulate > > > the same memory with their module options. > > > > Modules implicitly interacting in this way is exactly what I find so > > terrible in the original proposal. The idea of a stub type1 module to > > preserve that uAPI was only proposed as a known terrible solution to the > > problem. > > And I take it you prefer we remove this compat code as well and just > leave the module option on vfio_type1 non-working? In the case where userspace provides an iommufd for the container, yes, the type1 module then isn't involved. > > think it's fair to require a re-opt-in by the user. In the latter > > case, userspace is intentionally choosing to use a highly compatible > > uAPI, but nonetheless, it's still a different uAPI. > > Well, the later case is likely going to be a choice made by the > distribution, eg I would expect that libvirt will start automatically > favoring iommufd if it is available. > > So, instructions someone might find saying to tweak the module option > and then use libvirt are going to stop working at some point. libvirt doesn't currently pass any file descriptors for vfio devices in QEMU, so we're looking a ways down the road here. Once QEMU gains native iommufd support, libvirt will launch QEMU in a way that explicitly enables this iommufd support. I'd expect libvirt might implement this by creating a "vfio-legacy" vs "vfio-iommufd" driver option, where "vfio" becomes an alias to one of those. That allows both a staged transition and a fallback for issues. I'd expect a first debugging step would be to fallback to legacy support. But ultimately, yes, in the rare case that this option is actually necessary, the admin would need to re-opt-in, and hopefully a dmesg log from iommufd would make it apparent this is the problem. I just can't wrap my head around shared module options and stub drivers being a sane solution simply to make this potentially rare condition more transparent w/o reminding user's their setup has a vulnerability. BTW, what is the actual expected use case of passing an iommufd as a vfio container? I have doubts that it'd make sense to have QEMU look for an iommufd in place of a vfio container for anything beyond yet another means for early testing of iommufd. Thanks, Alex
> BTW, what is the actual expected use case of passing an iommufd as a > vfio container? I have doubts that it'd make sense to have QEMU look > for an iommufd in place of a vfio container for anything beyond yet > another means for early testing of iommufd. Thanks, I don't think there is one in production for qemu. For something like DPDK I can imagine replacing the open logic to use vfio device cdevs and iommufd, but keeping the rest of the logic the same so the FD looks and feels like a VFIO container. There is little value in replacing the VFIO map/unmap/etc ioctls with the IOMMUFD equivalents. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, November 23, 2022 1:41 AM > > > BTW, what is the actual expected use case of passing an iommufd as a > > vfio container? I have doubts that it'd make sense to have QEMU look > > for an iommufd in place of a vfio container for anything beyond yet > > another means for early testing of iommufd. Thanks, One case is that the admin links /dev/vfio/vfio to /dev/iommu then all legacy vfio applications are implicitly converted to use iommufd as vfio container. > > I don't think there is one in production for qemu. > > For something like DPDK I can imagine replacing the open logic to use > vfio device cdevs and iommufd, but keeping the rest of the logic the > same so the FD looks and feels like a VFIO container. There is little > value in replacing the VFIO map/unmap/etc ioctls with the IOMMUFD > equivalents. > I'm not sure the value of entering this mixed world. I could envision dpdk starting to add cdev/iommufd support when it wants to use new features (pasid, iopf, etc.) which are available only via iommufd native api. Before that point it just stays with full vfio legacy.
On Wed, Nov 23, 2022 at 01:21:30AM +0000, Tian, Kevin wrote: > I'm not sure the value of entering this mixed world. I could envision > dpdk starting to add cdev/iommufd support when it wants to use > new features (pasid, iopf, etc.) which are available only via iommufd > native api. Before that point it just stays with full vfio legacy. I think the value is for the distro that would benefit from getting apps validated and running on iommufd with the least effort invested. So I'd like to see all the vfio apps we can convert to the new device interface and iommufd just to be converted, even if they don't make use of new features. It gives a consistent experiance and support knowledge base if everything uses one interface. Jason
On Mon, Nov 21, 2022 at 09:59:28PM -0400, Jason Gunthorpe wrote: > On Fri, Nov 18, 2022 at 01:36:46PM -0700, Alex Williamson wrote: > > On Fri, 18 Nov 2022 11:36:14 -0400 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > On Thu, Nov 17, 2022 at 01:14:51PM -0700, Alex Williamson wrote: > > > > On Wed, 16 Nov 2022 17:05:29 -0400 > > > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > > > > > This legacy module knob has become uAPI, when set on the vfio_iommu_type1 > > > > > it disables some security protections in the iommu drivers. Move the > > > > > storage for this knob to vfio_main.c so that iommufd can access it too. > > > > > > > > > > The may need enhancing as we learn more about how necessary > > > > > allow_unsafe_interrupts is in the current state of the world. If vfio > > > > > container is disabled then this option will not be available to the user. > > > > > > > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > > > > Tested-by: Yi Liu <yi.l.liu@intel.com> > > > > > Tested-by: Lixiao Yang <lixiao.yang@intel.com> > > > > > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > > > > > Tested-by: Yu He <yu.he@intel.com> > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > > > --- > > > > > drivers/vfio/vfio.h | 2 ++ > > > > > drivers/vfio/vfio_iommu_type1.c | 5 ++--- > > > > > drivers/vfio/vfio_main.c | 3 +++ > > > > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > > > > > It's really quite trivial to convert to a vfio_iommu.ko module to host > > > > a separate option for this. Half of the patch below is undoing what's > > > > done here. Is your only concern with this approach that we use a few > > > > KB more memory for the separate module? > > > > > > My main dislike is that it just seems arbitary to shunt iommufd > > > support to a module when it is always required by vfio.ko. In general > > > if you have a module that is only ever used by 1 other module, you > > > should probably just combine them. It saves memory and simplifies > > > operation (eg you don't have to unload a zoo of modules during > > > development testing) > > > > These are all great reasons for why iommufd should host this option, as > > it's fundamentally part of the DMA isolation of the device, which vfio > > relies on iommufd to provide in this case. > > Fine, lets do that. It looks like this: diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 07d4dcc0dbf5e1..6d088af776034b 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -9,6 +9,13 @@ #include "io_pagetable.h" #include "iommufd_private.h" +static bool allow_unsafe_interrupts; +module_param(allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC( + allow_unsafe_interrupts, + "Allow IOMMUFD to bind to devices even if the platform cannot isolate " + "the MSI interrupt window. Enabling this is a security weakness."); + /* * A iommufd_device object represents the binding relationship between a * consuming driver and the iommufd. These objects are created/destroyed by @@ -127,8 +134,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD); static int iommufd_device_setup_msi(struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt, - phys_addr_t sw_msi_start, - unsigned int flags) + phys_addr_t sw_msi_start) { int rc; @@ -174,12 +180,11 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev, * historical compat with VFIO allow a module parameter to ignore the * insecurity. */ - if (!(flags & IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT)) + if (!allow_unsafe_interrupts) return -EPERM; - dev_warn( idev->dev, - "Device interrupts cannot be isolated by the IOMMU, this platform in insecure. Use an \"allow_unsafe_interrupts\" module parameter to override\n"); + "MSI interrupt window cannot be isolated by the IOMMU, this platform in insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n"); return 0; } @@ -195,8 +200,7 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt, } static int iommufd_device_do_attach(struct iommufd_device *idev, - struct iommufd_hw_pagetable *hwpt, - unsigned int flags) + struct iommufd_hw_pagetable *hwpt) { phys_addr_t sw_msi_start = 0; int rc; @@ -226,7 +230,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, if (rc) goto out_unlock; - rc = iommufd_device_setup_msi(idev, hwpt, sw_msi_start, flags); + rc = iommufd_device_setup_msi(idev, hwpt, sw_msi_start); if (rc) goto out_iova; @@ -268,8 +272,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, * Automatic domain selection will never pick a manually created domain. */ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, - struct iommufd_ioas *ioas, - unsigned int flags) + struct iommufd_ioas *ioas) { struct iommufd_hw_pagetable *hwpt; int rc; @@ -284,7 +287,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, if (!hwpt->auto_domain) continue; - rc = iommufd_device_do_attach(idev, hwpt, flags); + rc = iommufd_device_do_attach(idev, hwpt); /* * -EINVAL means the domain is incompatible with the device. @@ -303,7 +306,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, } hwpt->auto_domain = true; - rc = iommufd_device_do_attach(idev, hwpt, flags); + rc = iommufd_device_do_attach(idev, hwpt); if (rc) goto out_abort; list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list); @@ -324,7 +327,6 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, * @idev: device to attach * @pt_id: Input a IOMMUFD_OBJ_IOAS, or IOMMUFD_OBJ_HW_PAGETABLE * Output the IOMMUFD_OBJ_HW_PAGETABLE ID - * @flags: Optional flags * * This connects the device to an iommu_domain, either automatically or manually * selected. Once this completes the device could do DMA. @@ -332,8 +334,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, * The caller should return the resulting pt_id back to userspace. * This function is undone by calling iommufd_device_detach(). */ -int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id, - unsigned int flags) +int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) { struct iommufd_object *pt_obj; int rc; @@ -347,7 +348,7 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id, struct iommufd_hw_pagetable *hwpt = container_of(pt_obj, struct iommufd_hw_pagetable, obj); - rc = iommufd_device_do_attach(idev, hwpt, flags); + rc = iommufd_device_do_attach(idev, hwpt); if (rc) goto out_put_pt_obj; @@ -360,7 +361,7 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id, struct iommufd_ioas *ioas = container_of(pt_obj, struct iommufd_ioas, obj); - rc = iommufd_device_auto_get_domain(idev, ioas, flags); + rc = iommufd_device_auto_get_domain(idev, ioas); if (rc) goto out_put_pt_obj; break; diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index 5a7ce4d9fbae0a..da50feb24b6e1d 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -108,12 +108,9 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_physical_unbind); int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id) { - unsigned int flags = 0; int rc; - if (vfio_allow_unsafe_interrupts) - flags |= IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT; - rc = iommufd_device_attach(vdev->iommufd_device, pt_id, flags); + rc = iommufd_device_attach(vdev->iommufd_device, pt_id); if (rc) return rc; vdev->iommufd_attached = true; diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index 3378714a746274..ce5fe3fc493b4e 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -216,6 +216,4 @@ extern bool vfio_noiommu __read_mostly; enum { vfio_noiommu = false }; #endif -extern bool vfio_allow_unsafe_interrupts; - #endif diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 186e33a006d314..23c24fe98c00d4 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -44,8 +44,9 @@ #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" #define DRIVER_DESC "Type1 IOMMU driver for VFIO" +static bool allow_unsafe_interrupts; module_param_named(allow_unsafe_interrupts, - vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); + allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); MODULE_PARM_DESC(allow_unsafe_interrupts, "Enable VFIO IOMMU support for on platforms without interrupt remapping support."); @@ -2281,7 +2282,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP, vfio_iommu_device_capable); - if (!vfio_allow_unsafe_interrupts && !msi_remap) { + if (!allow_unsafe_interrupts && !msi_remap) { pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n", __func__); ret = -EPERM; diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 451a07eb702b34..593d45f43a16ba 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -52,9 +52,6 @@ static struct vfio { struct ida device_ida; } vfio; -bool vfio_allow_unsafe_interrupts; -EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts); - static DEFINE_XARRAY(vfio_device_set_xa); static const struct file_operations vfio_group_fops; diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index bf2b3ea5f90fd2..9d1afd417215d0 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -21,11 +21,7 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx, struct device *dev, u32 *id); void iommufd_device_unbind(struct iommufd_device *idev); -enum { - IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT = 1 << 0, -}; -int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id, - unsigned int flags); +int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id); void iommufd_device_detach(struct iommufd_device *idev); struct iommufd_access_ops {
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, November 24, 2022 3:48 AM > > On Mon, Nov 21, 2022 at 09:59:28PM -0400, Jason Gunthorpe wrote: > > On Fri, Nov 18, 2022 at 01:36:46PM -0700, Alex Williamson wrote: > > > On Fri, 18 Nov 2022 11:36:14 -0400 > > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > > > On Thu, Nov 17, 2022 at 01:14:51PM -0700, Alex Williamson wrote: > > > > > On Wed, 16 Nov 2022 17:05:29 -0400 > > > > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > > > > > > > This legacy module knob has become uAPI, when set on the > vfio_iommu_type1 > > > > > > it disables some security protections in the iommu drivers. Move the > > > > > > storage for this knob to vfio_main.c so that iommufd can access it > too. > > > > > > > > > > > > The may need enhancing as we learn more about how necessary > > > > > > allow_unsafe_interrupts is in the current state of the world. If vfio > > > > > > container is disabled then this option will not be available to the > user. > > > > > > > > > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > > > > > Tested-by: Yi Liu <yi.l.liu@intel.com> > > > > > > Tested-by: Lixiao Yang <lixiao.yang@intel.com> > > > > > > Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> > > > > > > Tested-by: Yu He <yu.he@intel.com> > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > > > > --- > > > > > > drivers/vfio/vfio.h | 2 ++ > > > > > > drivers/vfio/vfio_iommu_type1.c | 5 ++--- > > > > > > drivers/vfio/vfio_main.c | 3 +++ > > > > > > 3 files changed, 7 insertions(+), 3 deletions(-) > > > > > > > > > > It's really quite trivial to convert to a vfio_iommu.ko module to host > > > > > a separate option for this. Half of the patch below is undoing what's > > > > > done here. Is your only concern with this approach that we use a few > > > > > KB more memory for the separate module? > > > > > > > > My main dislike is that it just seems arbitary to shunt iommufd > > > > support to a module when it is always required by vfio.ko. In general > > > > if you have a module that is only ever used by 1 other module, you > > > > should probably just combine them. It saves memory and simplifies > > > > operation (eg you don't have to unload a zoo of modules during > > > > development testing) > > > > > > These are all great reasons for why iommufd should host this option, as > > > it's fundamentally part of the DMA isolation of the device, which vfio > > > relies on iommufd to provide in this case. > > > > Fine, lets do that. > > It looks like this: > > diff --git a/drivers/iommu/iommufd/device.c > b/drivers/iommu/iommufd/device.c > index 07d4dcc0dbf5e1..6d088af776034b 100644 > --- a/drivers/iommu/iommufd/device.c > +++ b/drivers/iommu/iommufd/device.c > @@ -9,6 +9,13 @@ > #include "io_pagetable.h" > #include "iommufd_private.h" > > +static bool allow_unsafe_interrupts; > +module_param(allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); > +MODULE_PARM_DESC( > + allow_unsafe_interrupts, > + "Allow IOMMUFD to bind to devices even if the platform cannot > isolate " > + "the MSI interrupt window. Enabling this is a security weakness."); > + > /* > * A iommufd_device object represents the binding relationship between a > * consuming driver and the iommufd. These objects are created/destroyed > by > @@ -127,8 +134,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, > IOMMUFD); > > static int iommufd_device_setup_msi(struct iommufd_device *idev, > struct iommufd_hw_pagetable *hwpt, > - phys_addr_t sw_msi_start, > - unsigned int flags) > + phys_addr_t sw_msi_start) > { > int rc; > > @@ -174,12 +180,11 @@ static int iommufd_device_setup_msi(struct > iommufd_device *idev, > * historical compat with VFIO allow a module parameter to ignore > the > * insecurity. > */ > - if (!(flags & > IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT)) > + if (!allow_unsafe_interrupts) > return -EPERM; > - keep the blank line. > dev_warn( > idev->dev, > - "Device interrupts cannot be isolated by the IOMMU, this > platform in insecure. Use an \"allow_unsafe_interrupts\" module parameter > to override\n"); > + "MSI interrupt window cannot be isolated by the IOMMU, > this platform in insecure. Use the \"allow_unsafe_interrupts\" module s/in/is/ > parameter to override\n"); > return 0; > } > > @@ -195,8 +200,7 @@ static bool > iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt, > } > > static int iommufd_device_do_attach(struct iommufd_device *idev, > - struct iommufd_hw_pagetable *hwpt, > - unsigned int flags) > + struct iommufd_hw_pagetable *hwpt) > { > phys_addr_t sw_msi_start = 0; > int rc; > @@ -226,7 +230,7 @@ static int iommufd_device_do_attach(struct > iommufd_device *idev, > if (rc) > goto out_unlock; > > - rc = iommufd_device_setup_msi(idev, hwpt, sw_msi_start, flags); > + rc = iommufd_device_setup_msi(idev, hwpt, sw_msi_start); > if (rc) > goto out_iova; > > @@ -268,8 +272,7 @@ static int iommufd_device_do_attach(struct > iommufd_device *idev, > * Automatic domain selection will never pick a manually created domain. > */ > static int iommufd_device_auto_get_domain(struct iommufd_device *idev, > - struct iommufd_ioas *ioas, > - unsigned int flags) > + struct iommufd_ioas *ioas) > { > struct iommufd_hw_pagetable *hwpt; > int rc; > @@ -284,7 +287,7 @@ static int iommufd_device_auto_get_domain(struct > iommufd_device *idev, > if (!hwpt->auto_domain) > continue; > > - rc = iommufd_device_do_attach(idev, hwpt, flags); > + rc = iommufd_device_do_attach(idev, hwpt); > > /* > * -EINVAL means the domain is incompatible with the device. > @@ -303,7 +306,7 @@ static int iommufd_device_auto_get_domain(struct > iommufd_device *idev, > } > hwpt->auto_domain = true; > > - rc = iommufd_device_do_attach(idev, hwpt, flags); > + rc = iommufd_device_do_attach(idev, hwpt); > if (rc) > goto out_abort; > list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list); > @@ -324,7 +327,6 @@ static int iommufd_device_auto_get_domain(struct > iommufd_device *idev, > * @idev: device to attach > * @pt_id: Input a IOMMUFD_OBJ_IOAS, or > IOMMUFD_OBJ_HW_PAGETABLE > * Output the IOMMUFD_OBJ_HW_PAGETABLE ID > - * @flags: Optional flags > * > * This connects the device to an iommu_domain, either automatically or > manually > * selected. Once this completes the device could do DMA. > @@ -332,8 +334,7 @@ static int iommufd_device_auto_get_domain(struct > iommufd_device *idev, > * The caller should return the resulting pt_id back to userspace. > * This function is undone by calling iommufd_device_detach(). > */ > -int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id, > - unsigned int flags) > +int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) > { > struct iommufd_object *pt_obj; > int rc; > @@ -347,7 +348,7 @@ int iommufd_device_attach(struct iommufd_device > *idev, u32 *pt_id, > struct iommufd_hw_pagetable *hwpt = > container_of(pt_obj, struct iommufd_hw_pagetable, > obj); > > - rc = iommufd_device_do_attach(idev, hwpt, flags); > + rc = iommufd_device_do_attach(idev, hwpt); > if (rc) > goto out_put_pt_obj; > > @@ -360,7 +361,7 @@ int iommufd_device_attach(struct iommufd_device > *idev, u32 *pt_id, > struct iommufd_ioas *ioas = > container_of(pt_obj, struct iommufd_ioas, obj); > > - rc = iommufd_device_auto_get_domain(idev, ioas, flags); > + rc = iommufd_device_auto_get_domain(idev, ioas); > if (rc) > goto out_put_pt_obj; > break; > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c > index 5a7ce4d9fbae0a..da50feb24b6e1d 100644 > --- a/drivers/vfio/iommufd.c > +++ b/drivers/vfio/iommufd.c > @@ -108,12 +108,9 @@ > EXPORT_SYMBOL_GPL(vfio_iommufd_physical_unbind); > > int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id) > { > - unsigned int flags = 0; > int rc; > > - if (vfio_allow_unsafe_interrupts) > - flags |= > IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT; > - rc = iommufd_device_attach(vdev->iommufd_device, pt_id, flags); > + rc = iommufd_device_attach(vdev->iommufd_device, pt_id); > if (rc) > return rc; > vdev->iommufd_attached = true; > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > index 3378714a746274..ce5fe3fc493b4e 100644 > --- a/drivers/vfio/vfio.h > +++ b/drivers/vfio/vfio.h > @@ -216,6 +216,4 @@ extern bool vfio_noiommu __read_mostly; > enum { vfio_noiommu = false }; > #endif > > -extern bool vfio_allow_unsafe_interrupts; > - > #endif > diff --git a/drivers/vfio/vfio_iommu_type1.c > b/drivers/vfio/vfio_iommu_type1.c > index 186e33a006d314..23c24fe98c00d4 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -44,8 +44,9 @@ > #define DRIVER_AUTHOR "Alex Williamson > <alex.williamson@redhat.com>" > #define DRIVER_DESC "Type1 IOMMU driver for VFIO" > > +static bool allow_unsafe_interrupts; > module_param_named(allow_unsafe_interrupts, > - vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); > + allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); > MODULE_PARM_DESC(allow_unsafe_interrupts, > "Enable VFIO IOMMU support for on platforms without > interrupt remapping support."); > > @@ -2281,7 +2282,7 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > iommu_group_for_each_dev(iommu_group, (void > *)IOMMU_CAP_INTR_REMAP, > vfio_iommu_device_capable); > > - if (!vfio_allow_unsafe_interrupts && !msi_remap) { > + if (!allow_unsafe_interrupts && !msi_remap) { > pr_warn("%s: No interrupt remapping support. Use the > module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support > on this platform\n", > __func__); > ret = -EPERM; > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index 451a07eb702b34..593d45f43a16ba 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -52,9 +52,6 @@ static struct vfio { > struct ida device_ida; > } vfio; > > -bool vfio_allow_unsafe_interrupts; > -EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts); > - > static DEFINE_XARRAY(vfio_device_set_xa); > static const struct file_operations vfio_group_fops; > > diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h > index bf2b3ea5f90fd2..9d1afd417215d0 100644 > --- a/include/linux/iommufd.h > +++ b/include/linux/iommufd.h > @@ -21,11 +21,7 @@ struct iommufd_device *iommufd_device_bind(struct > iommufd_ctx *ictx, > struct device *dev, u32 *id); > void iommufd_device_unbind(struct iommufd_device *idev); > > -enum { > - IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT = 1 << 0, > -}; > -int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id, > - unsigned int flags); > +int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id); > void iommufd_device_detach(struct iommufd_device *idev); > > struct iommufd_access_ops { this looks good to me: Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, November 24, 2022 12:38 AM > > On Wed, Nov 23, 2022 at 01:21:30AM +0000, Tian, Kevin wrote: > > > I'm not sure the value of entering this mixed world. I could envision > > dpdk starting to add cdev/iommufd support when it wants to use > > new features (pasid, iopf, etc.) which are available only via iommufd > > native api. Before that point it just stays with full vfio legacy. > > I think the value is for the distro that would benefit from getting > apps validated and running on iommufd with the least effort invested. > > So I'd like to see all the vfio apps we can convert to the new device > interface and iommufd just to be converted, even if they don't make > use of new features. > We'll certainly change Qemu but then it will support either legacy vfio or new iommufd native. No intermediate step like above. what'd be the practice to push other vfio apps to do such conversion?
On Thu, Nov 24, 2022 at 05:30:00AM +0000, Tian, Kevin wrote:
> what'd be the practice to push other vfio apps to do such conversion?
If you are operating a custom hypervisor environment there is security
and maintenance value in minimizing the kernel code enabled.
So eg needing iommufd turned on to run qemu and vfio but also needing
vfio container & group turned on to run dpdk as part of the OVS is not
desirable compared to just updating dpdk.
Jason
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index f95f4925b83bbd..54e5a8e0834ccb 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -130,4 +130,6 @@ extern bool vfio_noiommu __read_mostly; enum { vfio_noiommu = false }; #endif +extern bool vfio_allow_unsafe_interrupts; + #endif diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 23c24fe98c00d4..186e33a006d314 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -44,9 +44,8 @@ #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" #define DRIVER_DESC "Type1 IOMMU driver for VFIO" -static bool allow_unsafe_interrupts; module_param_named(allow_unsafe_interrupts, - allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); + vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); MODULE_PARM_DESC(allow_unsafe_interrupts, "Enable VFIO IOMMU support for on platforms without interrupt remapping support."); @@ -2282,7 +2281,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP, vfio_iommu_device_capable); - if (!allow_unsafe_interrupts && !msi_remap) { + if (!vfio_allow_unsafe_interrupts && !msi_remap) { pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n", __func__); ret = -EPERM; diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 8c2dcb481ae10b..e1fec1db6a3c93 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -51,6 +51,9 @@ static struct vfio { struct ida device_ida; } vfio; +bool vfio_allow_unsafe_interrupts; +EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts); + static DEFINE_XARRAY(vfio_device_set_xa); static const struct file_operations vfio_group_fops;