On Fri, 18 Nov 2022 11:36:14 -0400 Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > Tested-by: Yi Liu <yi.l.liu@xxxxxxxxx> > > > Tested-by: Lixiao Yang <lixiao.yang@xxxxxxxxx> > > > Tested-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx> > > > Tested-by: Yu He <yu.he@xxxxxxxxx> > > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx>" > > #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
