On Mon, 21 Nov 2022 21:59:28 -0400 Jason Gunthorpe <jgg@xxxxxxxxxx> 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@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. > > 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
