Re: [PATCH 3/4] iommu: Introduce IOMMU call-back for processing struct KVM assigned to VFIO

[Jason Gunthorpe]

On Tue, Jan 17, 2023 at 12:31:07PM +0700, Suthikulpanit, Suravee wrote:
> Hi Jason,
> 
> On 1/13/2023 10:35 PM, Jason Gunthorpe wrote:
> > On Tue, Jan 10, 2023 at 08:31:36AM -0600, Suravee Suthikulpanit wrote:
> > > Currently, VFIO provide an kvm_vfio_file_set_kvm() interface for assigning
> > > a KVM structure to a VFIO group. The information in struct KVM is also
> > > useful for IOMMU drivers when setting up VFIO domain.
> > > 
> > > Introduce struct iommu_domain_ops.set_kvm call-back function to allow
> > > IOMMU drivers to provide call-back to process the struct KVM
> > > assigned.
> > 
> > Also NAK
> > 
> > Connecting the iommu driver to KVM has to be properly architected
> > though iommufd.
> > 
> 
> My understanding is the kvm_vfio_file_set_kvm() from the following
> call-path:
> 
> * kvm_vfio_group_add()
> * kvm_vfio_group_del()
> * kvm_vfio_destroy()
> 
> to attach/detach KVM to/from a particular VFIO domain. 

No, it has nothing to do with a VFIO domain.

It is intended to connect the KVM to a VFIO device for use in
architecture specific ways (primarily s390), and to support
broken-by-design code in GVT's mdev.

We currenly have no connection between kvm and the iommu domain at
all.

> Could you please elaborate what you have in mind for a properly architected
> interface via iommufd?

You'd need to explain what this is trying to do. As I said, I want to
see a comprehensive VFIO solution for CC from the people interested in
it that supports all three major architectures currently available. I
really don't want to see three different almost-the-same but
unmaintainable different versions of this.

Frankly I'm really not clear what role the IOMMU driver should be
playing in CC at all, certainly not with details about what AMD's
design requires.

AFAIK ARM expects the the IOMMU will be controlled by the realm
manager. How can AMD be different from this and still be secure? The
translation of IOVA for DMA is a security critical operation. I would
expect the KVM page table and the IOMMU S2 to be hardwired together.

So if you need hypervisor involvment you need to start there by
defining what exactly your architecture needs for iommu programming
and create a special iommu_domain that encapsulates whatever that is.

> > > @@ -1652,6 +1652,7 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> > >   	mutex_lock(&group->group_lock);
> > >   	group->kvm = kvm;
> > > +	iommu_set_kvm(group->iommu_group, kvm);
> > >   	mutex_unlock(&group->group_lock);
> > >   }
> > 
> > This also has obvious lifetime bugs
> 
> Could you please also elaborate on this part? For detaching case, KVM is
> NULL, and the same information is passed to the IOMMU driver to handle the
> detaching case. Am I missing anything?

The kvm pointer is only valid so long as the group_lock is held.

Jason