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(+)
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)
{
--
2.32.0On 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 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 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
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)
> {
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
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.
© 2016 - 2025 Red Hat, Inc.