kvm/vfio: Manage KVM IOMMU coherency with virtual VFIO device

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Quoted 20 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 5 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

POWER does not support (at least boos3s - "server", not sure about others) this cache-non-coherent stuff at all.

Regarding reusing this device with external API for POWER - I posted a patch which introduces KVM device to link KVM with IOMMU but besides the list of groups registered in KVM, it also provides the way to find a group by LIOBN (logical bus number) which is used in DMA map/unmap hypercalls. So in my case kvm_vfio_group struct needs LIOBN and it would be nice to have there window_size too (for a quick boundary check). I am not sure we want to mix everything here.

It is in "[PATCH v10 12/13] KVM: PPC: Add support for IOMMU in-kernel handling" if you are interested (kvmppc_spapr_tce_iommu_device).

Quoted 28 lines · Alexey Kardashevskiy

Quoted 20 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 5 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Quoted 2 lines

POWER does not support (at least boos3s - "server", not sure about others) this cache-non-coherent stuff at all.

Then it's easy for your IOMMU API interface to return always cache coherent or never cache coherent or whatever ;)

Quoted 10 lines

Regarding reusing this device with external API for POWER - I posted a patch which introduces KVM device to link KVM with IOMMU but besides the list of groups registered in KVM, it also provides the way to find a group by LIOBN (logical bus number) which is used in DMA map/unmap hypercalls. So in my case kvm_vfio_group struct needs LIOBN and it would be nice to have there window_size too (for a quick boundary check). I am not sure we want to mix everything here.

It is in "[PATCH v10 12/13] KVM: PPC: Add support for IOMMU in-kernel handling" if you are interested (kvmppc_spapr_tce_iommu_device).

Yes, I stole the code to get the vfio symbols from your code. The convergence I was hoping to achieve is that KVM doesn't really want to know about VFIO and vica versa. We can therefore at least limit the intrusion by sharing a common device. Obviously for you it will need some extra interfaces to associate an LIOBN to a group, but we keep both the kernel an userspace cleaner by avoiding duplication where we can. Is this really not extensible to your usage?

Quoted 46 lines · Alex Williamson

Quoted 28 lines · Alexey Kardashevskiy

Quoted 20 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 5 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Quoted 2 lines

POWER does not support (at least boos3s - "server", not sure about others) this cache-non-coherent stuff at all.

Then it's easy for your IOMMU API interface to return always cache coherent or never cache coherent or whatever ;)

Quoted 10 lines

Regarding reusing this device with external API for POWER - I posted a patch which introduces KVM device to link KVM with IOMMU but besides the list of groups registered in KVM, it also provides the way to find a group by LIOBN (logical bus number) which is used in DMA map/unmap hypercalls. So in my case kvm_vfio_group struct needs LIOBN and it would be nice to have there window_size too (for a quick boundary check). I am not sure we want to mix everything here.

It is in "[PATCH v10 12/13] KVM: PPC: Add support for IOMMU in-kernel handling" if you are interested (kvmppc_spapr_tce_iommu_device).

Quoted 7 lines

Yes, I stole the code to get the vfio symbols from your code. The convergence I was hoping to achieve is that KVM doesn't really want to know about VFIO and vica versa. We can therefore at least limit the intrusion by sharing a common device. Obviously for you it will need some extra interfaces to associate an LIOBN to a group, but we keep both the kernel an userspace cleaner by avoiding duplication where we can. Is this really not extensible to your usage?

Well, I do not have a good taste for this kind of stuff so I cannot tell for sure. I can reuse this device and hack it to do whatever I need but how?

There are 2 things I need from KVM device:
1. associate IOMMU group with LIOBN

2. get a pointer to an IOMMU group by LIOBN (used to get ppc's IOMMU table pointer which is an IOMMU data of an IOMMU group so we could take a shortcut here).

There are 3 possible interfaces to use:
A. set/get attributes
B. ioctl
C. additional API

What I posted a week ago uses A for 1 and C for 2. Now we move this to virt/kvm/vfio.c.

A for 1 is more or less ok but how exactly? Yet another attribute? Platform specific "bus ID"? In your patch attr->addr is not really an address (to be accessed with get_user()) but just an fd.

For 2 - there are already A and B interfaces so we do not want C, right? What kind of a good device has a backdoor? :) But A and B do not have access control to prevent the user space from receiving a IOMMU group pointer (which it would not be able to use anyway but still). Do we care about this (I do not)? And using B (ioctls) within the kernel - I cannot say I saw many usages of this.

I am pretty sure we will spend some time (not much) arguing about these things and when we agree on something, then some of KVM maintainers will step in and say that there is 1001st way of doing this and - goto start. And after all, this still won't be a device as it does not emulate anything present in the real hardware, this is just yet another interface to KVM and some ways of using it won't be natural for somebody. /me sighs.

Quoted 55 lines · Alexey Kardashevskiy

Quoted 53 lines · Alex Williamson

Quoted 28 lines · Alexey Kardashevskiy

Quoted 20 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 5 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Quoted 2 lines

POWER does not support (at least boos3s - "server", not sure about others) this cache-non-coherent stuff at all.

Then it's easy for your IOMMU API interface to return always cache coherent or never cache coherent or whatever ;)

Quoted 10 lines

Regarding reusing this device with external API for POWER - I posted a patch which introduces KVM device to link KVM with IOMMU but besides the list of groups registered in KVM, it also provides the way to find a group by LIOBN (logical bus number) which is used in DMA map/unmap hypercalls. So in my case kvm_vfio_group struct needs LIOBN and it would be nice to have there window_size too (for a quick boundary check). I am not sure we want to mix everything here.

It is in "[PATCH v10 12/13] KVM: PPC: Add support for IOMMU in-kernel handling" if you are interested (kvmppc_spapr_tce_iommu_device).

Yes, I stole the code to get the vfio symbols from your code. The convergence I was hoping to achieve is that KVM doesn't really want to know about VFIO and vica versa. We can therefore at least limit the intrusion by sharing a common device. Obviously for you it will need some extra interfaces to associate an LIOBN to a group, but we keep both the kernel an userspace cleaner by avoiding duplication where we can. Is this really not extensible to your usage?

Quoted 5 lines

Well, I do not have a good taste for this kind of stuff so I cannot tell for sure. I can reuse this device and hack it to do whatever I need but how?

There are 2 things I need from KVM device:
1. associate IOMMU group with LIOBN

Does QEMU know this? We could set another attribute to specify the LIOBN for a group.

Quoted 3 lines

2. get a pointer to an IOMMU group by LIOBN (used to get ppc's IOMMU table pointer which is an IOMMU data of an IOMMU group so we could take a shortcut here).

Once we have a VFIO device with a VFIO group added and the LIOBN attribute set, isn't this just a matter of some access code?

Quoted 4 lines

There are 3 possible interfaces to use:
A. set/get attributes
B. ioctl
C. additional API

I think we need to differentiate user interfaces from kernel interfaces. Within the kernel, we make no guarantees about interfaces and we can change them to meet our needs. That includes the vfio external user interface. For userspace, we need to be careful not to break things. I suggest we use the set/get/has attribute interface that's already part of KVM for the user interface.

Quoted 2 lines

What I posted a week ago uses A for 1 and C for 2. Now we move this to virt/kvm/vfio.c.

I don't care where it lives, other than we both have a need for it, so it seems like the core of it should not live in architecture specific directories.

Quoted 3 lines

A for 1 is more or less ok but how exactly? Yet another attribute? Platform specific "bus ID"? In your patch attr->addr is not really an address (to be accessed with get_user()) but just an fd.

Is that a problem?

Quoted 6 lines

For 2 - there are already A and B interfaces so we do not want C, right? What kind of a good device has a backdoor? :) But A and B do not have access control to prevent the user space from receiving a IOMMU group pointer (which it would not be able to use anyway but still). Do we care about this (I do not)? And using B (ioctls) within the kernel - I cannot say I saw many usages of this.

For kernel internal things we don't want to invent new ioctls or use the KVM device get_attr interface. Note that I didn't provide a get_attr interface for anything there now. I think we just want to create a kernel internal interface, ie. function calls.

Quoted 6 lines

I am pretty sure we will spend some time (not much) arguing about these things and when we agree on something, then some of KVM maintainers will step in and say that there is 1001st way of doing this and - goto start. And after all, this still won't be a device as it does not emulate anything present in the real hardware, this is just yet another interface to KVM and some ways of using it won't be natural for somebody. /me sighs.

And thus this RFC with somewhat rough code to try to get buyin. Thanks,

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Signed-off-by Alex Williamson <[email protected]>

---
 Documentation/virtual/kvm/devices/vfio.txt |   22 +++
 arch/x86/include/asm/kvm_host.h            |    1 
 arch/x86/kvm/Makefile                      |    2 
 arch/x86/kvm/vmx.c                         |    5 -
 arch/x86/kvm/x86.c                         |    5 -
 include/linux/kvm_host.h                   |    1 
 include/uapi/linux/kvm.h                   |    4 
 virt/kvm/kvm_main.c                        |    3 
 virt/kvm/vfio.c                            |  237 ++++++++++++++++++++++++++++
 9 files changed, 275 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/virtual/kvm/devices/vfio.txt
 create mode 100644 virt/kvm/vfio.c

Documentation/virtual/kvm/devices/vfio.txt new file +22 -0

new file mode 100644
index 0000000..831e6a6
@@ -0,0 +1,22 @@
+VFIO virtual device
+===================
+
+Device types supported:
+  KVM_DEV_TYPE_VFIO
+
+Only one VFIO instance may be created per VM.  The created device
+tracks VFIO groups in use by the VM and features of those groups
+important to the correctness and acceleration of the VM.  As groups
+are enabled and disabled for use by the VM, KVM should be updated
+about their presence.  When registered with KVM, a reference to the
+VFIO-group is held by KVM.
+
+Groups:
+  KVM_DEV_VFIO_ADD_GROUP
+  KVM_DEV_VFIO_DEL_GROUP
+
+Each takes a int32_t file descriptor for kvm_device_attr.addr and
+does not support any group device kvm_device_attr.attr.
+
+RFC - Should we use Group KVM_DEV_VFIO_GROUP with Attributes
+      KVM_DEV_VFIO_GROUP_ADD & DEL?

arch/x86/include/asm/kvm_host.h +1 -0

index c76ff74..5b9350d 100644
@@ -588,6 +588,7 @@ struct kvm_arch {
 
 	spinlock_t pvclock_gtod_sync_lock;
 	bool use_master_clock;
+	bool vfio_noncoherent;
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
 

arch/x86/kvm/Makefile +1 -1

index bf4fb04..25d22b2 100644
@@ -9,7 +9,7 @@ KVM := ../../../virt/kvm
 
 kvm-y			+= $(KVM)/kvm_main.o $(KVM)/ioapic.o \
 				$(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o \
-				$(KVM)/eventfd.o $(KVM)/irqchip.o
+				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
 kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)	+= $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 

arch/x86/kvm/vmx.c +3 -2

index 1f1da43..94f7786 100644
@@ -7395,8 +7395,9 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	 */
 	if (is_mmio)
 		ret = MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
-	else if (vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
+	else if (vcpu->kvm->arch.vfio_noncoherent ||
+		 vcpu->kvm->arch.iommu_domain &&
+		 !(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
 		ret = kvm_get_guest_memory_type(vcpu, gfn) <<
 		      VMX_EPT_MT_EPTE_SHIFT;
 	else

arch/x86/kvm/x86.c +3 -2

index e5ca72a..406ba6f 100644
@@ -2715,8 +2715,9 @@ static void wbinvd_ipi(void *garbage)
 
 static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 {
-	return vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY);
+	return vcpu->kvm->arch.vfio_noncoherent ||
+	       (vcpu->kvm->arch.iommu_domain &&
+		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY));
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)

include/linux/kvm_host.h +1 -0

index ca645a0..615f0c3 100644
@@ -1065,6 +1065,7 @@ struct kvm_device *kvm_device_from_filp(struct file *filp);
 
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
+extern struct kvm_device_ops kvm_vfio_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 

include/uapi/linux/kvm.h +4 -0

index 99c2533..8869616 100644
@@ -843,6 +843,10 @@ struct kvm_device_attr {
 #define KVM_DEV_TYPE_FSL_MPIC_20	1
 #define KVM_DEV_TYPE_FSL_MPIC_42	2
 #define KVM_DEV_TYPE_XICS		3
+#define KVM_DEV_TYPE_VFIO		4
+
+#define KVM_DEV_VFIO_ADD_GROUP		1
+#define KVM_DEV_VFIO_DEL_GROUP		2
 
 /*
  * ioctls for VM fds

virt/kvm/kvm_main.c +3 -0

index d9cad4d..1a20425 100644
@@ -2269,6 +2269,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		ops = &kvm_xics_ops;
 		break;
 #endif
+	case KVM_DEV_TYPE_VFIO:
+		ops = &kvm_vfio_ops;
+		break;
 	default:
 		return -ENODEV;
 	}

virt/kvm/vfio.c new file +237 -0

new file mode 100644
index 0000000..9a2faff
@@ -0,0 +1,237 @@
+/*
+ * VFIO bridge
+ *
+ * Copyright (C) 2013 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/kvm_host.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+
+struct kvm_vfio_group {
+	struct list_head node;
+	struct vfio_group *vfio_group;
+};
+
+struct kvm_vfio {
+	struct list_head group_list;
+	struct mutex lock;
+};
+
+static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
+{
+	struct vfio_group *vfio_group;
+	struct vfio_group *(*fn)(struct file *);
+
+	fn = symbol_get(vfio_group_get_external_user);
+	if (!fn)
+		return ERR_PTR(-EINVAL);
+
+	vfio_group = fn(filep);
+
+	symbol_put(vfio_group_get_external_user);
+
+	return vfio_group;
+}
+
+static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
+{
+	void (*fn)(struct vfio_group *);
+
+	fn = symbol_get(vfio_group_put_external_user);
+	if (!fn)
+		return;
+
+	fn(vfio_group);
+
+	symbol_put(vfio_group_put_external_user);
+}
+
+static int kvm_vfio_external_user_check_extension(struct vfio_group *vfio_group,
+						  unsigned long arg)
+{
+	int (*fn)(struct vfio_group *, unsigned long);
+	int ret;
+
+	fn = symbol_get(vfio_external_user_check_extension);
+	if (!fn)
+		return -EINVAL;
+
+	ret = fn(vfio_group, arg);
+
+	symbol_put(vfio_group_put_external_user);
+
+	return ret;
+}
+
+static void kvm_vfio_update_iommu_coherency(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	bool coherent = true;
+	struct kvm_vfio_group *kvg;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (!kvm_vfio_external_user_check_extension(kvg->vfio_group,
+					VFIO_IOMMU_CAP_CACHE_COHERENCY)) {
+			coherent = false;
+			break;
+		}
+	}
+
+	mutex_unlock(&kv->lock);
+
+	dev->kvm->arch.vfio_noncoherent = !coherent;
+}
+
+static int kvm_vfio_set_attr(struct kvm_device *dev,
+			     struct kvm_device_attr *attr)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct fd f;
+	struct vfio_group *vfio_group;
+	struct kvm_vfio_group *kvg;
+	int ret;
+
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group == vfio_group) {
+				mutex_unlock(&kv->lock);
+				kvm_vfio_group_put_external_user(vfio_group);
+				return -EEXIST;
+			}
+		}
+
+		kvg = kzalloc(sizeof(*kvg), GFP_KERNEL);
+		if (!kvg) {
+			mutex_unlock(&kv->lock);
+			kvm_vfio_group_put_external_user(vfio_group);
+			return -ENOMEM;
+		}
+
+		list_add_tail(&kvg->node, &kv->group_list);
+		kvg->vfio_group = vfio_group;
+
+		mutex_unlock(&kv->lock);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return 0;
+
+	case KVM_DEV_VFIO_DEL_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		ret = -ENOENT;
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group != vfio_group)
+				continue;
+
+			list_del(&kvg->node);
+			kvm_vfio_group_put_external_user(kvg->vfio_group);
+			kfree(kvg);
+			ret = 0;
+			break;
+		}
+
+		mutex_unlock(&kv->lock);
+		kvm_vfio_group_put_external_user(vfio_group);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+
+static int kvm_vfio_has_attr(struct kvm_device *dev,
+				   struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+	case KVM_DEV_VFIO_DEL_GROUP:
+		return 0;
+	}
+
+	return -ENXIO;
+}
+
+static void kvm_vfio_destroy(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct kvm_vfio_group *kvg, *tmp;
+
+	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
+		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		list_del(&kvg->node);
+		kfree(kvg);
+	}
+
+	dev->kvm->arch.vfio_noncoherent = false;
+	kfree(kv);
+}
+
+static int kvm_vfio_create(struct kvm_device *dev, u32 type)
+{
+	struct kvm_device *tmp;
+	struct kvm_vfio *kv;
+
+	/* Only one VFIO "device" per VM */
+	list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
+		if (tmp->ops == &kvm_vfio_ops)
+			return -EBUSY;
+
+	kv = kzalloc(sizeof(*kv), GFP_KERNEL);
+	if (!kv)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&kv->group_list);
+	mutex_init(&kv->lock);
+
+	dev->private = kv;
+
+	return 0;
+}
+
+struct kvm_device_ops kvm_vfio_ops = {
+	.name = "kvm-vfio",
+	.create = kvm_vfio_create,
+	.destroy = kvm_vfio_destroy,
+	.set_attr = kvm_vfio_set_attr,
+	.has_attr = kvm_vfio_has_attr,
+};

Quoted 8 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

Quoted 11 lines

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

So how is the isolation handled then?

How is this better than a ioctl to grant WBINVD to guest? kvm char device can be opened by any user,
so any user can grant itself these priveledges. What did I miss?

Quoted 390 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Signed-off-by Alex Williamson <[email protected]>

---
 Documentation/virtual/kvm/devices/vfio.txt |   22 +++
 arch/x86/include/asm/kvm_host.h            |    1 
 arch/x86/kvm/Makefile                      |    2 
 arch/x86/kvm/vmx.c                         |    5 -
 arch/x86/kvm/x86.c                         |    5 -
 include/linux/kvm_host.h                   |    1 
 include/uapi/linux/kvm.h                   |    4 
 virt/kvm/kvm_main.c                        |    3 
 virt/kvm/vfio.c                            |  237 ++++++++++++++++++++++++++++
 9 files changed, 275 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/virtual/kvm/devices/vfio.txt
 create mode 100644 virt/kvm/vfio.c

Documentation/virtual/kvm/devices/vfio.txt new file +22 -0

new file mode 100644
index 0000000..831e6a6
@@ -0,0 +1,22 @@
+VFIO virtual device
+===================
+
+Device types supported:
+  KVM_DEV_TYPE_VFIO
+
+Only one VFIO instance may be created per VM.  The created device
+tracks VFIO groups in use by the VM and features of those groups
+important to the correctness and acceleration of the VM.  As groups
+are enabled and disabled for use by the VM, KVM should be updated
+about their presence.  When registered with KVM, a reference to the
+VFIO-group is held by KVM.
+
+Groups:
+  KVM_DEV_VFIO_ADD_GROUP
+  KVM_DEV_VFIO_DEL_GROUP
+
+Each takes a int32_t file descriptor for kvm_device_attr.addr and
+does not support any group device kvm_device_attr.attr.
+
+RFC - Should we use Group KVM_DEV_VFIO_GROUP with Attributes
+      KVM_DEV_VFIO_GROUP_ADD & DEL?

arch/x86/include/asm/kvm_host.h +1 -0

index c76ff74..5b9350d 100644
@@ -588,6 +588,7 @@ struct kvm_arch {
 
 	spinlock_t pvclock_gtod_sync_lock;
 	bool use_master_clock;
+	bool vfio_noncoherent;
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
 

arch/x86/kvm/Makefile +1 -1

index bf4fb04..25d22b2 100644
@@ -9,7 +9,7 @@ KVM := ../../../virt/kvm
 
 kvm-y			+= $(KVM)/kvm_main.o $(KVM)/ioapic.o \
 				$(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o \
-				$(KVM)/eventfd.o $(KVM)/irqchip.o
+				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
 kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)	+= $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 

arch/x86/kvm/vmx.c +3 -2

index 1f1da43..94f7786 100644
@@ -7395,8 +7395,9 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	 */
 	if (is_mmio)
 		ret = MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
-	else if (vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
+	else if (vcpu->kvm->arch.vfio_noncoherent ||
+		 vcpu->kvm->arch.iommu_domain &&
+		 !(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
 		ret = kvm_get_guest_memory_type(vcpu, gfn) <<
 		      VMX_EPT_MT_EPTE_SHIFT;
 	else

arch/x86/kvm/x86.c +3 -2

index e5ca72a..406ba6f 100644
@@ -2715,8 +2715,9 @@ static void wbinvd_ipi(void *garbage)
 
 static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 {
-	return vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY);
+	return vcpu->kvm->arch.vfio_noncoherent ||
+	       (vcpu->kvm->arch.iommu_domain &&
+		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY));
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)

include/linux/kvm_host.h +1 -0

index ca645a0..615f0c3 100644
@@ -1065,6 +1065,7 @@ struct kvm_device *kvm_device_from_filp(struct file *filp);
 
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
+extern struct kvm_device_ops kvm_vfio_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 

include/uapi/linux/kvm.h +4 -0

index 99c2533..8869616 100644
@@ -843,6 +843,10 @@ struct kvm_device_attr {
 #define KVM_DEV_TYPE_FSL_MPIC_20	1
 #define KVM_DEV_TYPE_FSL_MPIC_42	2
 #define KVM_DEV_TYPE_XICS		3
+#define KVM_DEV_TYPE_VFIO		4
+
+#define KVM_DEV_VFIO_ADD_GROUP		1
+#define KVM_DEV_VFIO_DEL_GROUP		2
 
 /*
  * ioctls for VM fds

virt/kvm/kvm_main.c +3 -0

index d9cad4d..1a20425 100644
@@ -2269,6 +2269,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		ops = &kvm_xics_ops;
 		break;
 #endif
+	case KVM_DEV_TYPE_VFIO:
+		ops = &kvm_vfio_ops;
+		break;
 	default:
 		return -ENODEV;
 	}

virt/kvm/vfio.c new file +237 -0

new file mode 100644
index 0000000..9a2faff
@@ -0,0 +1,237 @@
+/*
+ * VFIO bridge
+ *
+ * Copyright (C) 2013 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/kvm_host.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+
+struct kvm_vfio_group {
+	struct list_head node;
+	struct vfio_group *vfio_group;
+};
+
+struct kvm_vfio {
+	struct list_head group_list;
+	struct mutex lock;
+};
+
+static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
+{
+	struct vfio_group *vfio_group;
+	struct vfio_group *(*fn)(struct file *);
+
+	fn = symbol_get(vfio_group_get_external_user);
+	if (!fn)
+		return ERR_PTR(-EINVAL);
+
+	vfio_group = fn(filep);
+
+	symbol_put(vfio_group_get_external_user);
+
+	return vfio_group;
+}
+
+static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
+{
+	void (*fn)(struct vfio_group *);
+
+	fn = symbol_get(vfio_group_put_external_user);
+	if (!fn)
+		return;
+
+	fn(vfio_group);
+
+	symbol_put(vfio_group_put_external_user);
+}
+
+static int kvm_vfio_external_user_check_extension(struct vfio_group *vfio_group,
+						  unsigned long arg)
+{
+	int (*fn)(struct vfio_group *, unsigned long);
+	int ret;
+
+	fn = symbol_get(vfio_external_user_check_extension);
+	if (!fn)
+		return -EINVAL;
+
+	ret = fn(vfio_group, arg);
+
+	symbol_put(vfio_group_put_external_user);
+
+	return ret;
+}
+
+static void kvm_vfio_update_iommu_coherency(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	bool coherent = true;
+	struct kvm_vfio_group *kvg;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (!kvm_vfio_external_user_check_extension(kvg->vfio_group,
+					VFIO_IOMMU_CAP_CACHE_COHERENCY)) {
+			coherent = false;
+			break;
+		}
+	}
+
+	mutex_unlock(&kv->lock);
+
+	dev->kvm->arch.vfio_noncoherent = !coherent;
+}
+
+static int kvm_vfio_set_attr(struct kvm_device *dev,
+			     struct kvm_device_attr *attr)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct fd f;
+	struct vfio_group *vfio_group;
+	struct kvm_vfio_group *kvg;
+	int ret;
+
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group == vfio_group) {
+				mutex_unlock(&kv->lock);
+				kvm_vfio_group_put_external_user(vfio_group);
+				return -EEXIST;
+			}
+		}
+
+		kvg = kzalloc(sizeof(*kvg), GFP_KERNEL);
+		if (!kvg) {
+			mutex_unlock(&kv->lock);
+			kvm_vfio_group_put_external_user(vfio_group);
+			return -ENOMEM;
+		}
+
+		list_add_tail(&kvg->node, &kv->group_list);
+		kvg->vfio_group = vfio_group;
+
+		mutex_unlock(&kv->lock);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return 0;
+
+	case KVM_DEV_VFIO_DEL_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		ret = -ENOENT;
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group != vfio_group)
+				continue;
+
+			list_del(&kvg->node);
+			kvm_vfio_group_put_external_user(kvg->vfio_group);
+			kfree(kvg);
+			ret = 0;
+			break;
+		}
+
+		mutex_unlock(&kv->lock);
+		kvm_vfio_group_put_external_user(vfio_group);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+
+static int kvm_vfio_has_attr(struct kvm_device *dev,
+				   struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+	case KVM_DEV_VFIO_DEL_GROUP:
+		return 0;
+	}
+
+	return -ENXIO;
+}
+
+static void kvm_vfio_destroy(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct kvm_vfio_group *kvg, *tmp;
+
+	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
+		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		list_del(&kvg->node);
+		kfree(kvg);
+	}
+
+	dev->kvm->arch.vfio_noncoherent = false;
+	kfree(kv);
+}
+
+static int kvm_vfio_create(struct kvm_device *dev, u32 type)
+{
+	struct kvm_device *tmp;
+	struct kvm_vfio *kv;
+
+	/* Only one VFIO "device" per VM */
+	list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
+		if (tmp->ops == &kvm_vfio_ops)
+			return -EBUSY;
+
+	kv = kzalloc(sizeof(*kv), GFP_KERNEL);
+	if (!kv)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&kv->group_list);
+	mutex_init(&kv->lock);
+
+	dev->private = kv;
+
+	return 0;
+}
+
+struct kvm_device_ops kvm_vfio_ops = {
+	.name = "kvm-vfio",
+	.create = kvm_vfio_create,
+	.destroy = kvm_vfio_destroy,
+	.set_attr = kvm_vfio_set_attr,
+	.has_attr = kvm_vfio_has_attr,
+};

Quoted 22 lines · Michael S. Tsirkin

Quoted 8 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

Quoted 11 lines

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 1 line

So how is the isolation handled then?

By isolation above, I'm only talking about the intrusive-ness of the code and "leaking" vfio knowledge into KVM. Nothing to do with device isolation.

Quoted 4 lines

How is this better than a ioctl to grant WBINVD to guest? kvm char device can be opened by any user,
so any user can grant itself these priveledges. What did I miss?

With this interface the caller must have physical access to one or more devices via vfio groups and each of those groups must be configured into one or more IOMMU domains. Furthermore, at least one of the IOMMU domains must not include the IOMMU_CAP_CACHE_COHERENCY capability. So it's actually quite a significantly higher hurdle than an ioctl open to anyone and the number of VMs on a given host capable of doing this is bound by the number of IOMMU groups. We do not however verify that a vfio device is actually in use by the VM, but I don't think there's a way to do that from KVM and I'm not sure that it's important to do so. I believe having access to physical hardware is already a sufficient granting of privilege to enable things like WBINVD. Thanks,

Alex

Quoted 390 lines

Quoted 390 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Signed-off-by Alex Williamson <[email protected]>

---
 Documentation/virtual/kvm/devices/vfio.txt |   22 +++
 arch/x86/include/asm/kvm_host.h            |    1 
 arch/x86/kvm/Makefile                      |    2 
 arch/x86/kvm/vmx.c                         |    5 -
 arch/x86/kvm/x86.c                         |    5 -
 include/linux/kvm_host.h                   |    1 
 include/uapi/linux/kvm.h                   |    4 
 virt/kvm/kvm_main.c                        |    3 
 virt/kvm/vfio.c                            |  237 ++++++++++++++++++++++++++++
 9 files changed, 275 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/virtual/kvm/devices/vfio.txt
 create mode 100644 virt/kvm/vfio.c

Documentation/virtual/kvm/devices/vfio.txt new file +22 -0

new file mode 100644
index 0000000..831e6a6
@@ -0,0 +1,22 @@
+VFIO virtual device
+===================
+
+Device types supported:
+  KVM_DEV_TYPE_VFIO
+
+Only one VFIO instance may be created per VM.  The created device
+tracks VFIO groups in use by the VM and features of those groups
+important to the correctness and acceleration of the VM.  As groups
+are enabled and disabled for use by the VM, KVM should be updated
+about their presence.  When registered with KVM, a reference to the
+VFIO-group is held by KVM.
+
+Groups:
+  KVM_DEV_VFIO_ADD_GROUP
+  KVM_DEV_VFIO_DEL_GROUP
+
+Each takes a int32_t file descriptor for kvm_device_attr.addr and
+does not support any group device kvm_device_attr.attr.
+
+RFC - Should we use Group KVM_DEV_VFIO_GROUP with Attributes
+      KVM_DEV_VFIO_GROUP_ADD & DEL?

arch/x86/include/asm/kvm_host.h +1 -0

index c76ff74..5b9350d 100644
@@ -588,6 +588,7 @@ struct kvm_arch {
 
 	spinlock_t pvclock_gtod_sync_lock;
 	bool use_master_clock;
+	bool vfio_noncoherent;
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
 

arch/x86/kvm/Makefile +1 -1

index bf4fb04..25d22b2 100644
@@ -9,7 +9,7 @@ KVM := ../../../virt/kvm
 
 kvm-y			+= $(KVM)/kvm_main.o $(KVM)/ioapic.o \
 				$(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o \
-				$(KVM)/eventfd.o $(KVM)/irqchip.o
+				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
 kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)	+= $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 

arch/x86/kvm/vmx.c +3 -2

index 1f1da43..94f7786 100644
@@ -7395,8 +7395,9 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	 */
 	if (is_mmio)
 		ret = MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
-	else if (vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
+	else if (vcpu->kvm->arch.vfio_noncoherent ||
+		 vcpu->kvm->arch.iommu_domain &&
+		 !(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
 		ret = kvm_get_guest_memory_type(vcpu, gfn) <<
 		      VMX_EPT_MT_EPTE_SHIFT;
 	else

arch/x86/kvm/x86.c +3 -2

index e5ca72a..406ba6f 100644
@@ -2715,8 +2715,9 @@ static void wbinvd_ipi(void *garbage)
 
 static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 {
-	return vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY);
+	return vcpu->kvm->arch.vfio_noncoherent ||
+	       (vcpu->kvm->arch.iommu_domain &&
+		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY));
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)

include/linux/kvm_host.h +1 -0

index ca645a0..615f0c3 100644
@@ -1065,6 +1065,7 @@ struct kvm_device *kvm_device_from_filp(struct file *filp);
 
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
+extern struct kvm_device_ops kvm_vfio_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 

include/uapi/linux/kvm.h +4 -0

index 99c2533..8869616 100644
@@ -843,6 +843,10 @@ struct kvm_device_attr {
 #define KVM_DEV_TYPE_FSL_MPIC_20	1
 #define KVM_DEV_TYPE_FSL_MPIC_42	2
 #define KVM_DEV_TYPE_XICS		3
+#define KVM_DEV_TYPE_VFIO		4
+
+#define KVM_DEV_VFIO_ADD_GROUP		1
+#define KVM_DEV_VFIO_DEL_GROUP		2
 
 /*
  * ioctls for VM fds

virt/kvm/kvm_main.c +3 -0

index d9cad4d..1a20425 100644
@@ -2269,6 +2269,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		ops = &kvm_xics_ops;
 		break;
 #endif
+	case KVM_DEV_TYPE_VFIO:
+		ops = &kvm_vfio_ops;
+		break;
 	default:
 		return -ENODEV;
 	}

virt/kvm/vfio.c new file +237 -0

new file mode 100644
index 0000000..9a2faff
@@ -0,0 +1,237 @@
+/*
+ * VFIO bridge
+ *
+ * Copyright (C) 2013 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/kvm_host.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+
+struct kvm_vfio_group {
+	struct list_head node;
+	struct vfio_group *vfio_group;
+};
+
+struct kvm_vfio {
+	struct list_head group_list;
+	struct mutex lock;
+};
+
+static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
+{
+	struct vfio_group *vfio_group;
+	struct vfio_group *(*fn)(struct file *);
+
+	fn = symbol_get(vfio_group_get_external_user);
+	if (!fn)
+		return ERR_PTR(-EINVAL);
+
+	vfio_group = fn(filep);
+
+	symbol_put(vfio_group_get_external_user);
+
+	return vfio_group;
+}
+
+static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
+{
+	void (*fn)(struct vfio_group *);
+
+	fn = symbol_get(vfio_group_put_external_user);
+	if (!fn)
+		return;
+
+	fn(vfio_group);
+
+	symbol_put(vfio_group_put_external_user);
+}
+
+static int kvm_vfio_external_user_check_extension(struct vfio_group *vfio_group,
+						  unsigned long arg)
+{
+	int (*fn)(struct vfio_group *, unsigned long);
+	int ret;
+
+	fn = symbol_get(vfio_external_user_check_extension);
+	if (!fn)
+		return -EINVAL;
+
+	ret = fn(vfio_group, arg);
+
+	symbol_put(vfio_group_put_external_user);
+
+	return ret;
+}
+
+static void kvm_vfio_update_iommu_coherency(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	bool coherent = true;
+	struct kvm_vfio_group *kvg;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (!kvm_vfio_external_user_check_extension(kvg->vfio_group,
+					VFIO_IOMMU_CAP_CACHE_COHERENCY)) {
+			coherent = false;
+			break;
+		}
+	}
+
+	mutex_unlock(&kv->lock);
+
+	dev->kvm->arch.vfio_noncoherent = !coherent;
+}
+
+static int kvm_vfio_set_attr(struct kvm_device *dev,
+			     struct kvm_device_attr *attr)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct fd f;
+	struct vfio_group *vfio_group;
+	struct kvm_vfio_group *kvg;
+	int ret;
+
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group == vfio_group) {
+				mutex_unlock(&kv->lock);
+				kvm_vfio_group_put_external_user(vfio_group);
+				return -EEXIST;
+			}
+		}
+
+		kvg = kzalloc(sizeof(*kvg), GFP_KERNEL);
+		if (!kvg) {
+			mutex_unlock(&kv->lock);
+			kvm_vfio_group_put_external_user(vfio_group);
+			return -ENOMEM;
+		}
+
+		list_add_tail(&kvg->node, &kv->group_list);
+		kvg->vfio_group = vfio_group;
+
+		mutex_unlock(&kv->lock);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return 0;
+
+	case KVM_DEV_VFIO_DEL_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		ret = -ENOENT;
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group != vfio_group)
+				continue;
+
+			list_del(&kvg->node);
+			kvm_vfio_group_put_external_user(kvg->vfio_group);
+			kfree(kvg);
+			ret = 0;
+			break;
+		}
+
+		mutex_unlock(&kv->lock);
+		kvm_vfio_group_put_external_user(vfio_group);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+
+static int kvm_vfio_has_attr(struct kvm_device *dev,
+				   struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+	case KVM_DEV_VFIO_DEL_GROUP:
+		return 0;
+	}
+
+	return -ENXIO;
+}
+
+static void kvm_vfio_destroy(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct kvm_vfio_group *kvg, *tmp;
+
+	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
+		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		list_del(&kvg->node);
+		kfree(kvg);
+	}
+
+	dev->kvm->arch.vfio_noncoherent = false;
+	kfree(kv);
+}
+
+static int kvm_vfio_create(struct kvm_device *dev, u32 type)
+{
+	struct kvm_device *tmp;
+	struct kvm_vfio *kv;
+
+	/* Only one VFIO "device" per VM */
+	list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
+		if (tmp->ops == &kvm_vfio_ops)
+			return -EBUSY;
+
+	kv = kzalloc(sizeof(*kv), GFP_KERNEL);
+	if (!kv)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&kv->group_list);
+	mutex_init(&kv->lock);
+
+	dev->private = kv;
+
+	return 0;
+}
+
+struct kvm_device_ops kvm_vfio_ops = {
+	.name = "kvm-vfio",
+	.create = kvm_vfio_create,
+	.destroy = kvm_vfio_destroy,
+	.set_attr = kvm_vfio_set_attr,
+	.has_attr = kvm_vfio_has_attr,
+};

Quoted 46 lines · Alex Williamson

Quoted 22 lines · Michael S. Tsirkin

Quoted 8 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

Quoted 11 lines

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 1 line

So how is the isolation handled then?

By isolation above, I'm only talking about the intrusive-ness of the code and "leaking" vfio knowledge into KVM. Nothing to do with device isolation.

Quoted 4 lines

How is this better than a ioctl to grant WBINVD to guest? kvm char device can be opened by any user,
so any user can grant itself these priveledges. What did I miss?

With this interface the caller must have physical access to one or more devices via vfio groups and each of those groups must be configured into one or more IOMMU domains. Furthermore, at least one of the IOMMU domains must not include the IOMMU_CAP_CACHE_COHERENCY capability. So it's actually quite a significantly higher hurdle than an ioctl open to anyone and the number of VMs on a given host capable of doing this is bound by the number of IOMMU groups. We do not however verify that a vfio device is actually in use by the VM, but I don't think there's a way to do that from KVM and I'm not sure that it's important to do so. I believe having access to physical hardware is already a sufficient granting of privilege to enable things like WBINVD. Thanks,

Quoted 1 line

Alex

Fair enough, but how about revoking the priveledge? For example, device can be removed by hotplug - does priveledge remain?
Is this important at all?

Quoted 392 lines

Quoted 390 lines

Quoted 390 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Signed-off-by Alex Williamson <[email protected]>

---
 Documentation/virtual/kvm/devices/vfio.txt |   22 +++
 arch/x86/include/asm/kvm_host.h            |    1 
 arch/x86/kvm/Makefile                      |    2 
 arch/x86/kvm/vmx.c                         |    5 -
 arch/x86/kvm/x86.c                         |    5 -
 include/linux/kvm_host.h                   |    1 
 include/uapi/linux/kvm.h                   |    4 
 virt/kvm/kvm_main.c                        |    3 
 virt/kvm/vfio.c                            |  237 ++++++++++++++++++++++++++++
 9 files changed, 275 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/virtual/kvm/devices/vfio.txt
 create mode 100644 virt/kvm/vfio.c

Documentation/virtual/kvm/devices/vfio.txt new file +22 -0

new file mode 100644
index 0000000..831e6a6
@@ -0,0 +1,22 @@
+VFIO virtual device
+===================
+
+Device types supported:
+  KVM_DEV_TYPE_VFIO
+
+Only one VFIO instance may be created per VM.  The created device
+tracks VFIO groups in use by the VM and features of those groups
+important to the correctness and acceleration of the VM.  As groups
+are enabled and disabled for use by the VM, KVM should be updated
+about their presence.  When registered with KVM, a reference to the
+VFIO-group is held by KVM.
+
+Groups:
+  KVM_DEV_VFIO_ADD_GROUP
+  KVM_DEV_VFIO_DEL_GROUP
+
+Each takes a int32_t file descriptor for kvm_device_attr.addr and
+does not support any group device kvm_device_attr.attr.
+
+RFC - Should we use Group KVM_DEV_VFIO_GROUP with Attributes
+      KVM_DEV_VFIO_GROUP_ADD & DEL?

arch/x86/include/asm/kvm_host.h +1 -0

index c76ff74..5b9350d 100644
@@ -588,6 +588,7 @@ struct kvm_arch {
 
 	spinlock_t pvclock_gtod_sync_lock;
 	bool use_master_clock;
+	bool vfio_noncoherent;
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
 

arch/x86/kvm/Makefile +1 -1

index bf4fb04..25d22b2 100644
@@ -9,7 +9,7 @@ KVM := ../../../virt/kvm
 
 kvm-y			+= $(KVM)/kvm_main.o $(KVM)/ioapic.o \
 				$(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o \
-				$(KVM)/eventfd.o $(KVM)/irqchip.o
+				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
 kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)	+= $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 

arch/x86/kvm/vmx.c +3 -2

index 1f1da43..94f7786 100644
@@ -7395,8 +7395,9 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	 */
 	if (is_mmio)
 		ret = MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
-	else if (vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
+	else if (vcpu->kvm->arch.vfio_noncoherent ||
+		 vcpu->kvm->arch.iommu_domain &&
+		 !(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
 		ret = kvm_get_guest_memory_type(vcpu, gfn) <<
 		      VMX_EPT_MT_EPTE_SHIFT;
 	else

arch/x86/kvm/x86.c +3 -2

index e5ca72a..406ba6f 100644
@@ -2715,8 +2715,9 @@ static void wbinvd_ipi(void *garbage)
 
 static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 {
-	return vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY);
+	return vcpu->kvm->arch.vfio_noncoherent ||
+	       (vcpu->kvm->arch.iommu_domain &&
+		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY));
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)

include/linux/kvm_host.h +1 -0

index ca645a0..615f0c3 100644
@@ -1065,6 +1065,7 @@ struct kvm_device *kvm_device_from_filp(struct file *filp);
 
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
+extern struct kvm_device_ops kvm_vfio_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 

include/uapi/linux/kvm.h +4 -0

index 99c2533..8869616 100644
@@ -843,6 +843,10 @@ struct kvm_device_attr {
 #define KVM_DEV_TYPE_FSL_MPIC_20	1
 #define KVM_DEV_TYPE_FSL_MPIC_42	2
 #define KVM_DEV_TYPE_XICS		3
+#define KVM_DEV_TYPE_VFIO		4
+
+#define KVM_DEV_VFIO_ADD_GROUP		1
+#define KVM_DEV_VFIO_DEL_GROUP		2
 
 /*
  * ioctls for VM fds

virt/kvm/kvm_main.c +3 -0

index d9cad4d..1a20425 100644
@@ -2269,6 +2269,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		ops = &kvm_xics_ops;
 		break;
 #endif
+	case KVM_DEV_TYPE_VFIO:
+		ops = &kvm_vfio_ops;
+		break;
 	default:
 		return -ENODEV;
 	}

virt/kvm/vfio.c new file +237 -0

new file mode 100644
index 0000000..9a2faff
@@ -0,0 +1,237 @@
+/*
+ * VFIO bridge
+ *
+ * Copyright (C) 2013 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/kvm_host.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+
+struct kvm_vfio_group {
+	struct list_head node;
+	struct vfio_group *vfio_group;
+};
+
+struct kvm_vfio {
+	struct list_head group_list;
+	struct mutex lock;
+};
+
+static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
+{
+	struct vfio_group *vfio_group;
+	struct vfio_group *(*fn)(struct file *);
+
+	fn = symbol_get(vfio_group_get_external_user);
+	if (!fn)
+		return ERR_PTR(-EINVAL);
+
+	vfio_group = fn(filep);
+
+	symbol_put(vfio_group_get_external_user);
+
+	return vfio_group;
+}
+
+static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
+{
+	void (*fn)(struct vfio_group *);
+
+	fn = symbol_get(vfio_group_put_external_user);
+	if (!fn)
+		return;
+
+	fn(vfio_group);
+
+	symbol_put(vfio_group_put_external_user);
+}
+
+static int kvm_vfio_external_user_check_extension(struct vfio_group *vfio_group,
+						  unsigned long arg)
+{
+	int (*fn)(struct vfio_group *, unsigned long);
+	int ret;
+
+	fn = symbol_get(vfio_external_user_check_extension);
+	if (!fn)
+		return -EINVAL;
+
+	ret = fn(vfio_group, arg);
+
+	symbol_put(vfio_group_put_external_user);
+
+	return ret;
+}
+
+static void kvm_vfio_update_iommu_coherency(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	bool coherent = true;
+	struct kvm_vfio_group *kvg;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (!kvm_vfio_external_user_check_extension(kvg->vfio_group,
+					VFIO_IOMMU_CAP_CACHE_COHERENCY)) {
+			coherent = false;
+			break;
+		}
+	}
+
+	mutex_unlock(&kv->lock);
+
+	dev->kvm->arch.vfio_noncoherent = !coherent;
+}
+
+static int kvm_vfio_set_attr(struct kvm_device *dev,
+			     struct kvm_device_attr *attr)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct fd f;
+	struct vfio_group *vfio_group;
+	struct kvm_vfio_group *kvg;
+	int ret;
+
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group == vfio_group) {
+				mutex_unlock(&kv->lock);
+				kvm_vfio_group_put_external_user(vfio_group);
+				return -EEXIST;
+			}
+		}
+
+		kvg = kzalloc(sizeof(*kvg), GFP_KERNEL);
+		if (!kvg) {
+			mutex_unlock(&kv->lock);
+			kvm_vfio_group_put_external_user(vfio_group);
+			return -ENOMEM;
+		}
+
+		list_add_tail(&kvg->node, &kv->group_list);
+		kvg->vfio_group = vfio_group;
+
+		mutex_unlock(&kv->lock);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return 0;
+
+	case KVM_DEV_VFIO_DEL_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		ret = -ENOENT;
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group != vfio_group)
+				continue;
+
+			list_del(&kvg->node);
+			kvm_vfio_group_put_external_user(kvg->vfio_group);
+			kfree(kvg);
+			ret = 0;
+			break;
+		}
+
+		mutex_unlock(&kv->lock);
+		kvm_vfio_group_put_external_user(vfio_group);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+
+static int kvm_vfio_has_attr(struct kvm_device *dev,
+				   struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+	case KVM_DEV_VFIO_DEL_GROUP:
+		return 0;
+	}
+
+	return -ENXIO;
+}
+
+static void kvm_vfio_destroy(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct kvm_vfio_group *kvg, *tmp;
+
+	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
+		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		list_del(&kvg->node);
+		kfree(kvg);
+	}
+
+	dev->kvm->arch.vfio_noncoherent = false;
+	kfree(kv);
+}
+
+static int kvm_vfio_create(struct kvm_device *dev, u32 type)
+{
+	struct kvm_device *tmp;
+	struct kvm_vfio *kv;
+
+	/* Only one VFIO "device" per VM */
+	list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
+		if (tmp->ops == &kvm_vfio_ops)
+			return -EBUSY;
+
+	kv = kzalloc(sizeof(*kv), GFP_KERNEL);
+	if (!kv)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&kv->group_list);
+	mutex_init(&kv->lock);
+
+	dev->private = kv;
+
+	return 0;
+}
+
+struct kvm_device_ops kvm_vfio_ops = {
+	.name = "kvm-vfio",
+	.create = kvm_vfio_create,
+	.destroy = kvm_vfio_destroy,
+	.set_attr = kvm_vfio_set_attr,
+	.has_attr = kvm_vfio_has_attr,
+};

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Signed-off-by Alex Williamson <[email protected]>

---
 Documentation/virtual/kvm/devices/vfio.txt |   22 +++
 arch/x86/include/asm/kvm_host.h            |    1 
 arch/x86/kvm/Makefile                      |    2 
 arch/x86/kvm/vmx.c                         |    5 -
 arch/x86/kvm/x86.c                         |    5 -
 include/linux/kvm_host.h                   |    1 
 include/uapi/linux/kvm.h                   |    4 
 virt/kvm/kvm_main.c                        |    3 
 virt/kvm/vfio.c                            |  237 ++++++++++++++++++++++++++++
 9 files changed, 275 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/virtual/kvm/devices/vfio.txt
 create mode 100644 virt/kvm/vfio.c

Documentation/virtual/kvm/devices/vfio.txt new file +22 -0

new file mode 100644
index 0000000..831e6a6
@@ -0,0 +1,22 @@
+VFIO virtual device
+===================
+
+Device types supported:
+  KVM_DEV_TYPE_VFIO
+
+Only one VFIO instance may be created per VM.  The created device
+tracks VFIO groups in use by the VM and features of those groups
+important to the correctness and acceleration of the VM.  As groups
+are enabled and disabled for use by the VM, KVM should be updated
+about their presence.  When registered with KVM, a reference to the
+VFIO-group is held by KVM.
+
+Groups:
+  KVM_DEV_VFIO_ADD_GROUP
+  KVM_DEV_VFIO_DEL_GROUP
+
+Each takes a int32_t file descriptor for kvm_device_attr.addr and
+does not support any group device kvm_device_attr.attr.
+
+RFC - Should we use Group KVM_DEV_VFIO_GROUP with Attributes
+      KVM_DEV_VFIO_GROUP_ADD & DEL?

arch/x86/include/asm/kvm_host.h +1 -0

index c76ff74..5b9350d 100644
@@ -588,6 +588,7 @@ struct kvm_arch {
 
 	spinlock_t pvclock_gtod_sync_lock;
 	bool use_master_clock;
+	bool vfio_noncoherent;
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
 

arch/x86/kvm/Makefile +1 -1

index bf4fb04..25d22b2 100644
@@ -9,7 +9,7 @@ KVM := ../../../virt/kvm
 
 kvm-y			+= $(KVM)/kvm_main.o $(KVM)/ioapic.o \
 				$(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o \
-				$(KVM)/eventfd.o $(KVM)/irqchip.o
+				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
 kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)	+= $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 

arch/x86/kvm/vmx.c +3 -2

index 1f1da43..94f7786 100644
@@ -7395,8 +7395,9 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	 */
 	if (is_mmio)
 		ret = MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
-	else if (vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
+	else if (vcpu->kvm->arch.vfio_noncoherent ||
+		 vcpu->kvm->arch.iommu_domain &&
+		 !(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
 		ret = kvm_get_guest_memory_type(vcpu, gfn) <<
 		      VMX_EPT_MT_EPTE_SHIFT;
 	else

arch/x86/kvm/x86.c +3 -2

index e5ca72a..406ba6f 100644
@@ -2715,8 +2715,9 @@ static void wbinvd_ipi(void *garbage)
 
 static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 {
-	return vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY);
+	return vcpu->kvm->arch.vfio_noncoherent ||
+	       (vcpu->kvm->arch.iommu_domain &&
+		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY));
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)

include/linux/kvm_host.h +1 -0

index ca645a0..615f0c3 100644
@@ -1065,6 +1065,7 @@ struct kvm_device *kvm_device_from_filp(struct file *filp);
 
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
+extern struct kvm_device_ops kvm_vfio_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 

include/uapi/linux/kvm.h +4 -0

index 99c2533..8869616 100644
@@ -843,6 +843,10 @@ struct kvm_device_attr {
 #define KVM_DEV_TYPE_FSL_MPIC_20	1
 #define KVM_DEV_TYPE_FSL_MPIC_42	2
 #define KVM_DEV_TYPE_XICS		3
+#define KVM_DEV_TYPE_VFIO		4
+
+#define KVM_DEV_VFIO_ADD_GROUP		1
+#define KVM_DEV_VFIO_DEL_GROUP		2
 
 /*
  * ioctls for VM fds

virt/kvm/kvm_main.c +3 -0

index d9cad4d..1a20425 100644
@@ -2269,6 +2269,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		ops = &kvm_xics_ops;
 		break;
 #endif
+	case KVM_DEV_TYPE_VFIO:
+		ops = &kvm_vfio_ops;
+		break;
 	default:
 		return -ENODEV;
 	}

virt/kvm/vfio.c new file +227 -0

new file mode 100644
index 0000000..9a2faff
@@ -0,0 +1,237 @@
+/*
+ * VFIO bridge
+ *
+ * Copyright (C) 2013 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/kvm_host.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+
+struct kvm_vfio_group {
+	struct list_head node;
+	struct vfio_group *vfio_group;
+};
+
+struct kvm_vfio {
+	struct list_head group_list;
+	struct mutex lock;
+};
+
+static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
+{
+	struct vfio_group *vfio_group;
+	struct vfio_group *(*fn)(struct file *);
+
+	fn = symbol_get(vfio_group_get_external_user);
+	if (!fn)
+		return ERR_PTR(-EINVAL);
+
+	vfio_group = fn(filep);
+
+	symbol_put(vfio_group_get_external_user);
+
+	return vfio_group;
+}
+
+static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
+{
+	void (*fn)(struct vfio_group *);
+
+	fn = symbol_get(vfio_group_put_external_user);
+	if (!fn)
+		return;
+
+	fn(vfio_group);
+
+	symbol_put(vfio_group_put_external_user);
+}
+
+static int kvm_vfio_external_user_check_extension(struct vfio_group *vfio_group,
+						  unsigned long arg)
+{
+	int (*fn)(struct vfio_group *, unsigned long);
+	int ret;
+
+	fn = symbol_get(vfio_external_user_check_extension);
+	if (!fn)
+		return -EINVAL;
+
+	ret = fn(vfio_group, arg);
+
+	symbol_put(vfio_group_put_external_user);
+
+	return ret;
+}
+
+static void kvm_vfio_update_iommu_coherency(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	bool coherent = true;
+	struct kvm_vfio_group *kvg;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (!kvm_vfio_external_user_check_extension(kvg->vfio_group,
+					VFIO_IOMMU_CAP_CACHE_COHERENCY)) {
+			coherent = false;
+			break;
+		}
+	}
+
+	mutex_unlock(&kv->lock);
+
+	dev->kvm->arch.vfio_noncoherent = !coherent;
+}
+
+static int kvm_vfio_set_attr(struct kvm_device *dev,
+			     struct kvm_device_attr *attr)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct fd f;
+	struct vfio_group *vfio_group;
+	struct kvm_vfio_group *kvg;
+	int ret;
+
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group == vfio_group) {
+				mutex_unlock(&kv->lock);
+				kvm_vfio_group_put_external_user(vfio_group);
+				return -EEXIST;
+			}
+		}
+
+		kvg = kzalloc(sizeof(*kvg), GFP_KERNEL);
+		if (!kvg) {
+			mutex_unlock(&kv->lock);
+			kvm_vfio_group_put_external_user(vfio_group);
+			return -ENOMEM;
+		}
+
+		list_add_tail(&kvg->node, &kv->group_list);
+		kvg->vfio_group = vfio_group;
+
+		mutex_unlock(&kv->lock);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return 0;
+
+	case KVM_DEV_VFIO_DEL_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		ret = -ENOENT;
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group != vfio_group)
+				continue;
+
+			list_del(&kvg->node);
+			kvm_vfio_group_put_external_user(kvg->vfio_group);
+			kfree(kvg);
+			ret = 0;
+			break;
+		}
+
+		mutex_unlock(&kv->lock);
+		kvm_vfio_group_put_external_user(vfio_group);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+
+static int kvm_vfio_has_attr(struct kvm_device *dev,
+				   struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+	case KVM_DEV_VFIO_DEL_GROUP:
+		return 0;
+	}
+
+	return -ENXIO;
+}
+
+static void kvm_vfio_destroy(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct kvm_vfio_group *kvg, *tmp;
+
+	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
+		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		list_del(&kvg->node);
+		kfree(kvg);
+	}
+
+	dev->kvm->arch.vfio_noncoherent = false;
+	kfree(kv);
+}
+
+static int kvm_vfio_create(struct kvm_device *dev, u32 type)
+{
+	struct kvm_device *tmp;
+	struct kvm_vfio *kv;
+
+	/* Only one VFIO "device" per VM */
+	list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
+		if (tmp->ops == &kvm_vfio_ops)
+			return -EBUSY;
+
+	kv = kzalloc(sizeof(*kv), GFP_KERNEL);
+	if (!kv)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&kv->group_list);
+	mutex_init(&kv->lock);
+
+	dev->private = kv;
+

Quoted 8 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

Quoted 12 lines

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Looks good overall to me, one things though: to use legacy device assignment one needs root permission, so only root user can enable WBINVD emulation.

Who does this permission checking here? Is only root
allowed to create non coherent group with vfio?

Quoted 380 lines

The one intrusion is the resulting flag indicating the coherency state. For this RFC it's placed on the x86 kvm_arch struct, however I know POWER has interest in using the VFIO external user interface, and I'm hoping we can share a common KVM-VFIO device. Perhaps they care about No-Snoop handling as well or the code can be #ifdef'd.

Signed-off-by Alex Williamson <[email protected]>

---
 Documentation/virtual/kvm/devices/vfio.txt |   22 +++
 arch/x86/include/asm/kvm_host.h            |    1 
 arch/x86/kvm/Makefile                      |    2 
 arch/x86/kvm/vmx.c                         |    5 -
 arch/x86/kvm/x86.c                         |    5 -
 include/linux/kvm_host.h                   |    1 
 include/uapi/linux/kvm.h                   |    4 
 virt/kvm/kvm_main.c                        |    3 
 virt/kvm/vfio.c                            |  237 ++++++++++++++++++++++++++++
 9 files changed, 275 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/virtual/kvm/devices/vfio.txt
 create mode 100644 virt/kvm/vfio.c

Documentation/virtual/kvm/devices/vfio.txt new file +22 -0

new file mode 100644
index 0000000..831e6a6
@@ -0,0 +1,22 @@
+VFIO virtual device
+===================
+
+Device types supported:
+  KVM_DEV_TYPE_VFIO
+
+Only one VFIO instance may be created per VM.  The created device
+tracks VFIO groups in use by the VM and features of those groups
+important to the correctness and acceleration of the VM.  As groups
+are enabled and disabled for use by the VM, KVM should be updated
+about their presence.  When registered with KVM, a reference to the
+VFIO-group is held by KVM.
+
+Groups:
+  KVM_DEV_VFIO_ADD_GROUP
+  KVM_DEV_VFIO_DEL_GROUP
+
+Each takes a int32_t file descriptor for kvm_device_attr.addr and
+does not support any group device kvm_device_attr.attr.
+
+RFC - Should we use Group KVM_DEV_VFIO_GROUP with Attributes
+      KVM_DEV_VFIO_GROUP_ADD & DEL?

arch/x86/include/asm/kvm_host.h +1 -0

index c76ff74..5b9350d 100644
@@ -588,6 +588,7 @@ struct kvm_arch {
 
 	spinlock_t pvclock_gtod_sync_lock;
 	bool use_master_clock;
+	bool vfio_noncoherent;
 	u64 master_kernel_ns;
 	cycle_t master_cycle_now;
 

arch/x86/kvm/Makefile +1 -1

index bf4fb04..25d22b2 100644
@@ -9,7 +9,7 @@ KVM := ../../../virt/kvm
 
 kvm-y			+= $(KVM)/kvm_main.o $(KVM)/ioapic.o \
 				$(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o \
-				$(KVM)/eventfd.o $(KVM)/irqchip.o
+				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
 kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)	+= $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 

arch/x86/kvm/vmx.c +3 -2

index 1f1da43..94f7786 100644
@@ -7395,8 +7395,9 @@ static u64 vmx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	 */
 	if (is_mmio)
 		ret = MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
-	else if (vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
+	else if (vcpu->kvm->arch.vfio_noncoherent ||
+		 vcpu->kvm->arch.iommu_domain &&
+		 !(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY))
 		ret = kvm_get_guest_memory_type(vcpu, gfn) <<
 		      VMX_EPT_MT_EPTE_SHIFT;
 	else

arch/x86/kvm/x86.c +3 -2

index e5ca72a..406ba6f 100644
@@ -2715,8 +2715,9 @@ static void wbinvd_ipi(void *garbage)
 
 static bool need_emulate_wbinvd(struct kvm_vcpu *vcpu)
 {
-	return vcpu->kvm->arch.iommu_domain &&
-		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY);
+	return vcpu->kvm->arch.vfio_noncoherent ||
+	       (vcpu->kvm->arch.iommu_domain &&
+		!(vcpu->kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY));
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)

include/linux/kvm_host.h +1 -0

index ca645a0..615f0c3 100644
@@ -1065,6 +1065,7 @@ struct kvm_device *kvm_device_from_filp(struct file *filp);
 
 extern struct kvm_device_ops kvm_mpic_ops;
 extern struct kvm_device_ops kvm_xics_ops;
+extern struct kvm_device_ops kvm_vfio_ops;
 
 #ifdef CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT
 

include/uapi/linux/kvm.h +4 -0

index 99c2533..8869616 100644
@@ -843,6 +843,10 @@ struct kvm_device_attr {
 #define KVM_DEV_TYPE_FSL_MPIC_20	1
 #define KVM_DEV_TYPE_FSL_MPIC_42	2
 #define KVM_DEV_TYPE_XICS		3
+#define KVM_DEV_TYPE_VFIO		4
+
+#define KVM_DEV_VFIO_ADD_GROUP		1
+#define KVM_DEV_VFIO_DEL_GROUP		2
 
 /*
  * ioctls for VM fds

virt/kvm/kvm_main.c +3 -0

index d9cad4d..1a20425 100644
@@ -2269,6 +2269,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 		ops = &kvm_xics_ops;
 		break;
 #endif
+	case KVM_DEV_TYPE_VFIO:
+		ops = &kvm_vfio_ops;
+		break;
 	default:
 		return -ENODEV;
 	}

virt/kvm/vfio.c new file +227 -0

new file mode 100644
index 0000000..9a2faff
@@ -0,0 +1,237 @@
+/*
+ * VFIO bridge
+ *
+ * Copyright (C) 2013 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/file.h>
+#include <linux/kvm_host.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+
+struct kvm_vfio_group {
+	struct list_head node;
+	struct vfio_group *vfio_group;
+};
+
+struct kvm_vfio {
+	struct list_head group_list;
+	struct mutex lock;
+};
+
+static struct vfio_group *kvm_vfio_group_get_external_user(struct file *filep)
+{
+	struct vfio_group *vfio_group;
+	struct vfio_group *(*fn)(struct file *);
+
+	fn = symbol_get(vfio_group_get_external_user);
+	if (!fn)
+		return ERR_PTR(-EINVAL);
+
+	vfio_group = fn(filep);
+
+	symbol_put(vfio_group_get_external_user);
+
+	return vfio_group;
+}
+
+static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
+{
+	void (*fn)(struct vfio_group *);
+
+	fn = symbol_get(vfio_group_put_external_user);
+	if (!fn)
+		return;
+
+	fn(vfio_group);
+
+	symbol_put(vfio_group_put_external_user);
+}
+
+static int kvm_vfio_external_user_check_extension(struct vfio_group *vfio_group,
+						  unsigned long arg)
+{
+	int (*fn)(struct vfio_group *, unsigned long);
+	int ret;
+
+	fn = symbol_get(vfio_external_user_check_extension);
+	if (!fn)
+		return -EINVAL;
+
+	ret = fn(vfio_group, arg);
+
+	symbol_put(vfio_group_put_external_user);
+
+	return ret;
+}
+
+static void kvm_vfio_update_iommu_coherency(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	bool coherent = true;
+	struct kvm_vfio_group *kvg;
+
+	mutex_lock(&kv->lock);
+
+	list_for_each_entry(kvg, &kv->group_list, node) {
+		if (!kvm_vfio_external_user_check_extension(kvg->vfio_group,
+					VFIO_IOMMU_CAP_CACHE_COHERENCY)) {
+			coherent = false;
+			break;
+		}
+	}
+
+	mutex_unlock(&kv->lock);
+
+	dev->kvm->arch.vfio_noncoherent = !coherent;
+}
+
+static int kvm_vfio_set_attr(struct kvm_device *dev,
+			     struct kvm_device_attr *attr)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct fd f;
+	struct vfio_group *vfio_group;
+	struct kvm_vfio_group *kvg;
+	int ret;
+
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group == vfio_group) {
+				mutex_unlock(&kv->lock);
+				kvm_vfio_group_put_external_user(vfio_group);
+				return -EEXIST;
+			}
+		}
+
+		kvg = kzalloc(sizeof(*kvg), GFP_KERNEL);
+		if (!kvg) {
+			mutex_unlock(&kv->lock);
+			kvm_vfio_group_put_external_user(vfio_group);
+			return -ENOMEM;
+		}
+
+		list_add_tail(&kvg->node, &kv->group_list);
+		kvg->vfio_group = vfio_group;
+
+		mutex_unlock(&kv->lock);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return 0;
+
+	case KVM_DEV_VFIO_DEL_GROUP:
+		f = fdget(attr->addr);
+		if (!f.file)
+			return -EBADF;
+
+		vfio_group = kvm_vfio_group_get_external_user(f.file);
+		fdput(f);
+
+		if (IS_ERR(vfio_group))
+			return PTR_ERR(vfio_group);
+
+		ret = -ENOENT;
+
+		mutex_lock(&kv->lock);
+
+		list_for_each_entry(kvg, &kv->group_list, node) {
+			if (kvg->vfio_group != vfio_group)
+				continue;
+
+			list_del(&kvg->node);
+			kvm_vfio_group_put_external_user(kvg->vfio_group);
+			kfree(kvg);
+			ret = 0;
+			break;
+		}
+
+		mutex_unlock(&kv->lock);
+		kvm_vfio_group_put_external_user(vfio_group);
+
+		kvm_vfio_update_iommu_coherency(dev);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+
+static int kvm_vfio_has_attr(struct kvm_device *dev,
+				   struct kvm_device_attr *attr)
+{
+	switch (attr->group) {
+	case KVM_DEV_VFIO_ADD_GROUP:
+	case KVM_DEV_VFIO_DEL_GROUP:
+		return 0;
+	}
+
+	return -ENXIO;
+}
+
+static void kvm_vfio_destroy(struct kvm_device *dev)
+{
+	struct kvm_vfio *kv = dev->private;
+	struct kvm_vfio_group *kvg, *tmp;
+
+	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
+		kvm_vfio_group_put_external_user(kvg->vfio_group);
+		list_del(&kvg->node);
+		kfree(kvg);
+	}
+
+	dev->kvm->arch.vfio_noncoherent = false;
+	kfree(kv);
+}
+
+static int kvm_vfio_create(struct kvm_device *dev, u32 type)
+{
+	struct kvm_device *tmp;
+	struct kvm_vfio *kv;
+
+	/* Only one VFIO "device" per VM */
+	list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
+		if (tmp->ops == &kvm_vfio_ops)
+			return -EBUSY;
+
+	kv = kzalloc(sizeof(*kv), GFP_KERNEL);
+	if (!kv)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&kv->group_list);
+	mutex_init(&kv->lock);
+
+	dev->private = kv;
+

virt/kvm/vfio.c line 237

Quoted 10 lines

virt/kvm/vfio.c new file +10 -0

+	return 0;
+}
+
+struct kvm_device_ops kvm_vfio_ops = {
+	.name = "kvm-vfio",
+	.create = kvm_vfio_create,
+	.destroy = kvm_vfio_destroy,
+	.set_attr = kvm_vfio_set_attr,
+	.has_attr = kvm_vfio_has_attr,
+};

--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 21 lines · Gleb Natapov

Quoted 20 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 3 lines

Looks good overall to me, one things though: to use legacy device assignment one needs root permission, so only root user can enable WBINVD emulation.

That's not entirely accurate, legacy device assignment can be used by a non-root user, libvirt does this all the time. The part that requires root access is opening the pci-sysfs config file, the rest can be managed via file permissions on the remaining sysfs files.

Quoted 2 lines

Who does this permission checking here? Is only root
allowed to create non coherent group with vfio?

With vfio the user is granted permission by giving them access to the vfio group file (/dev/vfio/$GROUP) and binding all the devices in the group to vfio. That enables the user to create a container (~iommu domain) with the group attached to it. Only then will the vfio external user interface provide a reference to the group and enable this wbinvd support. So, wbinvd emulation should only be available to a user that "own" a vfio group and has it configured for use with this interface.

Quoted 22 lines · Alex Williamson

Quoted 21 lines · Gleb Natapov

Quoted 20 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 9 lines

Quoted 3 lines

Looks good overall to me, one things though: to use legacy device assignment one needs root permission, so only root user can enable WBINVD emulation.

That's not entirely accurate, legacy device assignment can be used by a non-root user, libvirt does this all the time. The part that requires root access is opening the pci-sysfs config file, the rest can be managed via file permissions on the remaining sysfs files.

So how libvirt manages to do that as non-root user if pci-sysfs config file needs root permission. I didn't mean to say that legacy code checks for root explicitly, what I meant is that at some point root permission is needed.

Quoted 10 lines

Quoted 2 lines

Who does this permission checking here? Is only root
allowed to create non coherent group with vfio?

With vfio the user is granted permission by giving them access to the vfio group file (/dev/vfio/$GROUP) and binding all the devices in the group to vfio. That enables the user to create a container (~iommu domain) with the group attached to it. Only then will the vfio external user interface provide a reference to the group and enable this wbinvd support. So, wbinvd emulation should only be available to a user that "own" a vfio group and has it configured for use with this interface.

What is the default permission of /dev/vfio/$GROUP?

Quoted 32 lines · Gleb Natapov

Quoted 22 lines · Alex Williamson

Quoted 21 lines · Gleb Natapov

Quoted 20 lines · Alex Williamson

So far we've succeeded at making KVM and VFIO mostly unaware of each other, but there's any important point where that breaks down. Intel VT-d hardware may or may not support snoop control. When snoop control is available, intel-iommu promotes No-Snoop transactions on PCIe to be cache coherent. That allows KVM to handle things like the x86 WBINVD opcode as a nop. When the hardware does not support this, KVM must implement a hardware visible WBINVD for the guest.

We could simply let userspace tell KVM how to handle WBINVD, but it's privileged for a reason. Allowing an arbitrary user to enable physical WBINVD gives them a more access to the hardware. Previously, this has only been enabled for guests supporting legacy PCI device assignment. In such cases it's necessary for proper guest execution. We therefore create a new KVM-VFIO virtual device. The user can add and remove VFIO groups to this device via file descriptors. KVM makes use of the VFIO external user interface to validate that the user has access to physical hardware and gets the coherency state of the IOMMU from VFIO. This provides equivalent functionality to legacy KVM assignment, while keeping (nearly) all the bits isolated.

Quoted 9 lines

Quoted 3 lines

Looks good overall to me, one things though: to use legacy device assignment one needs root permission, so only root user can enable WBINVD emulation.

That's not entirely accurate, legacy device assignment can be used by a non-root user, libvirt does this all the time. The part that requires root access is opening the pci-sysfs config file, the rest can be managed via file permissions on the remaining sysfs files.

Quoted 4 lines

So how libvirt manages to do that as non-root user if pci-sysfs config file needs root permission. I didn't mean to say that legacy code checks for root explicitly, what I meant is that at some point root permission is needed.

Yes, libvirt needs admin permission for legacy to bind to pci-stub, change permission on sysfs files and pass an opened pci config sysfs file descriptor. For vfio libvirt needs admin permission to bind to vfio-pci and change group file permission. From that perspective the admin requirement is similar.