Messages in this thread | | | Date | Tue, 17 Jan 2023 10:19:49 -0400 | From | Jason Gunthorpe <> | Subject | Re: [PATCH 3/4] iommu: Introduce IOMMU call-back for processing struct KVM assigned to VFIO |
| |
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
| |