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

[Jason Gunthorpe <jgg@nvidia.com>]

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