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

  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.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/iommu.c    | 10 ++++++++++
 drivers/vfio/vfio_main.c |  1 +
 include/linux/iommu.h    |  4 ++++
 3 files changed, 15 insertions(+)
  

On 2023-01-10 14:31, 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.

Hmm, it sounds like this has quite some overlap of intent with the 
existing "enable_nesting" op, and my gut feeling is that it's not great 
to have two completely different "this is a VFIO domain" mechanisms... :/

Robin.

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   drivers/iommu/iommu.c    | 10 ++++++++++
>   drivers/vfio/vfio_main.c |  1 +
>   include/linux/iommu.h    |  4 ++++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 65a3b3d886dc..5116d5fe35f2 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3231,3 +3231,13 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
>   	return user;
>   }
>   EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
> +
> +void iommu_set_kvm(struct iommu_group *group, struct kvm *kvm)
> +{
> +	if (!group || !group->domain || !group->domain->ops)
> +		return;
> +
> +	if (group->domain->ops->set_kvm)
> +		group->domain->ops->set_kvm(group->domain, kvm);
> +}
> +EXPORT_SYMBOL_GPL(iommu_set_kvm);
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 2d168793d4e1..7641e3a0c986 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -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);
>   }
>   EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 3c9da1f8979e..43000231d3d7 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -42,6 +42,7 @@ struct notifier_block;
>   struct iommu_sva;
>   struct iommu_fault_event;
>   struct iommu_dma_cookie;
> +struct kvm;
>   
>   /* iommu fault flags */
>   #define IOMMU_FAULT_READ	0x0
> @@ -314,6 +315,8 @@ struct iommu_domain_ops {
>   				  unsigned long quirks);
>   
>   	void (*free)(struct iommu_domain *domain);
> +
> +	void (*set_kvm)(struct iommu_domain *domain, struct kvm *kvm);
>   };
>   
>   /**
> @@ -391,6 +394,7 @@ void iommu_device_sysfs_remove(struct iommu_device *iommu);
>   int  iommu_device_link(struct iommu_device   *iommu, struct device *link);
>   void iommu_device_unlink(struct iommu_device *iommu, struct device *link);
>   int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain);
> +void iommu_set_kvm(struct iommu_group *group, struct kvm *kvm);
>   
>   static inline struct iommu_device *dev_to_iommu_device(struct device *dev)
>   {
  

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.

> @@ -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

Jason
  

Hi Robin,

On 1/10/2023 10:11 PM, Robin Murphy wrote:
> On 2023-01-10 14:31, 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.
> 
> Hmm, it sounds like this has quite some overlap of intent with the 
> existing "enable_nesting" op, and my gut feeling is that it's not great 
> to have two completely different "this is a VFIO domain" mechanisms... :/
> 
> Robin.

Actually, the intention is to communicate KVM information, which is 
already available to the VFIO down to the AMD IOMMU driver layer. I am 
not sure if the enable_nesting() has enough information or the same 
intention since that only communicates VFIO domain information.

Thanks,
Suravee
  

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. This is an 
existing interface from kvm_vfio_set_group()

Here is the call-path:

kvm_vfio_file_set_kvm()
	vfio_file_set_kvm()
		iommu_set_kvm() <-- New interface
			amd_iommu_set_kvm()

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

>> @@ -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?

Thanks,
Suravee

> 
> Jason
  

On 2023-01-17 04:20, Suthikulpanit, Suravee wrote:
> Hi Robin,
> 
> On 1/10/2023 10:11 PM, Robin Murphy wrote:
>> On 2023-01-10 14:31, 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.
>>
>> Hmm, it sounds like this has quite some overlap of intent with the 
>> existing "enable_nesting" op, and my gut feeling is that it's not 
>> great to have two completely different "this is a VFIO domain" 
>> mechanisms... :/
>>
>> Robin.
> 
> Actually, the intention is to communicate KVM information, which is 
> already available to the VFIO down to the AMD IOMMU driver layer. I am 
> not sure if the enable_nesting() has enough information or the same 
> intention since that only communicates VFIO domain information.

Sure, but from the high level view, we have on the one hand an API for 
"I want to use this domain in a VM (with nested paging)" and on other an 
API for "I want to use nested paging in this domain (for a VM)", so 
clearly it would be most sensible to converge on a single API for what 
is ultimately one single overarching use-case.

I'm not claiming that the existing enable_nesting op is anywhere near 
the right design either; in fact I'm pretty sure it isn't, if the Arm 
SMMU drivers ever want to contemplate sharing stage 2 pagetables with 
KVM also.

Cheers,
Robin.
  

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