Re: [PATCH v3 04/11] vfio: Move storage of allow_unsafe_interrupts to vfio_main.c

[Jason Gunthorpe <jgg@nvidia.com>]

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