Re: [PATCH 04/10] vfio: Use a struct of function pointers instead of a many symbol_get()'s

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, Apr 16, 2022 at 12:00:12AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Saturday, April 16, 2022 5:54 AM
> > 
> > On Fri, Apr 15, 2022 at 03:57:14AM +0000, Tian, Kevin wrote:
> > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > Sent: Friday, April 15, 2022 2:46 AM
> > > >
> > > > kvm and VFIO need to be coupled together however neither is willing to
> > > > tolerate a direct module dependency. Instead when kvm is given a VFIO
> > FD
> > > > it uses many symbol_get()'s to access VFIO.
> > > >
> > > > Provide a single VFIO function vfio_file_get_ops() which validates the
> > > > given struct file * is a VFIO file and then returns a struct of ops.
> > >
> > > VFIO has multiple files (container, group, and device). Here and other
> > > places seems to assume a VFIO file is just a group file. While it is correct
> > > in this external facing context, probably calling it 'VFIO group file' is
> > > clearer in various code comments and patch descriptions.
> > >
> > > >
> > > > Following patches will redo each of the symbol_get() calls into an
> > > > indirection through this ops struct.
> > > >
> > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >
> > >
> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > >
> > > Out of curiosity, how do you envision when iommufd is introduced?
> > > Will we need a generic ops abstraction so both vfio and iommufd
> > > register their own ops to keep kvm side generic or a new protocol
> > > will be introduced between iommufd and kvm?
> > 
> > I imagine using the vfio_device in all these context where the vfio
> > group is used, not iommufd. This keeps everything internal to vfio.
> > 
> 
> In this case although the uAPI is called KVM_DEV_VFIO_GROUP_ADD

Yes, down this path we'd probably alias it to KVM_DEV_VFIO_ADD_FD or
something.

> Qemu will pass in a device fd and with this series KVM doesn't care
> whether it's actually a device or group and just use struct file to call
> vfio_file_ops. correct?

Yes

> You probably remember there is one additional requirement when
> adding ENQCMD virtualization on Intel platform. KVM is required to
> setup a guest PASID to host PASID translation table in CPU vmcs
> structure to support ENQCMD in the guest. Following above direction
> I suppose KVM will provide a new interface to allow user pass in
>  [devfd, iommufd, guest_pasid] and then call a new vfio ops e.g.
> vfio_file_translate_guest_pasid(dev_file, iommufd, gpasid) to
> retrieve the host pasid. This sounds correct in concept as iommufd
> only knows host pasid and any g->h information is managed by
> vfio device driver.

I think there is no direct linkage of KVM to iommufd or VFIO for
ENQCMD.

The security nature of KVM is that the VM world should never have more
privilege than the hypervisor process running the KVM.

Therefore, when VM does a vENQCMD it must be equviliant to a physical
ENQCMD that the KVM process could already execute anyhow. Yes, Intel
wired ENQCMD to a single PASID, but we could imagine a system call
that allowed the process to change the PASID that ENQCMD uses from an
authorized list of PASIDs that the process has access to.

So, the linkage from iommufd is indirect. When iommufd does whatever
to install a PASID in the process's ENQCMD authorization table KVM can
be instructed to link that PASID inside the ENQCMD to a vPASID in the
VM.

As long as the PASID is in the process table KVM can allow the VM to
use it.

And it explains how userspace can actually use ENQCMD in a VFIO
scenario with iommufd, where obviously it needs to be in direct
control of what PASID ENQCMD generates and not be tied only to the
PASID associated with the mm_struct.

Jason