[RFC PATCH 0/5] KVM: x86: exit to user space on unhandled MSR accesses

[RFC PATCH 0/5] KVM: x86: exit to user space on unhandled MSR accesses

[Peter Hornyack]

There are numerous MSRs that kvm does not currently handle. On Intel
platforms we have observed guest VMs accessing some of these MSRs (for
example, MSR_PLATFORM_INFO) and behaving poorly (to the point of guest OS
crashes) when they receive a GP fault because the MSR is not emulated. This
patchset adds a new kvm exit path for unhandled MSR accesses that allows
user space to emulate additional MSRs without having to implement them in
kvm.

The core of the patchset modifies the vmx handle_rdmsr and handle_wrmsr
functions to exit to user space on MSR reads/writes that kvm can't handle
itself. Then, on the return path into kvm we check for outstanding user
space MSR completions and either complete the MSR access successfully or
inject a GP fault as kvm would do by default. This new exit path must be
enabled for the vm via the KVM_CAP_UNHANDLED_MSR_EXITS capability.

In the future we plan to extend this functionality to allow user space to
register the MSRs that it would like to handle itself, even if kvm already
provides an implementation. In the long-term we will move the
implementation of all non-performance-sensitive MSRs to user space,
reducing the potential attack surface of kvm and allowing us to respond to
bugs more quickly.

This patchset has been tested with our non-qemu user space hypervisor on
vmx platforms; svm support is not implemented.

Peter Hornyack (5):
  KVM: x86: refactor vmx rdmsr/wrmsr completion into new functions
  KVM: add KVM_EXIT_MSR exit reason and capability.
  KVM: x86: add msr_exits_supported to kvm_x86_ops
  KVM: x86: enable unhandled MSR exits for vmx
  KVM: x86: add trace events for unhandled MSR exits

 Documentation/virtual/kvm/api.txt |  48 +++++++++++++++
 arch/x86/include/asm/kvm_host.h   |   2 +
 arch/x86/kvm/svm.c                |   6 ++
 arch/x86/kvm/trace.h              |  28 +++++++++
 arch/x86/kvm/vmx.c                | 126 ++++++++++++++++++++++++++++++++++----
 arch/x86/kvm/x86.c                |  13 ++++
 include/trace/events/kvm.h        |   2 +-
 include/uapi/linux/kvm.h          |  14 +++++
 8 files changed, 227 insertions(+), 12 deletions(-)

-- 
2.5.0.276.gf5e568e

[RFC PATCH 1/5] KVM: x86: refactor vmx rdmsr/wrmsr completion into new functions

[Peter Hornyack]

After handling a rdmsr or wrmsr, refactor the success and failure code
paths into separate functions. This will allow us to also complete or
fail MSR accesses on the entry path from userspace into kvm.

Signed-off-by: Peter Hornyack <peterhornyack@google.com>
---
 arch/x86/kvm/vmx.c | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 37eae551857c..acc38e27d221 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5463,6 +5463,34 @@ static int handle_cpuid(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static void complete_rdmsr(struct kvm_vcpu *vcpu, const struct msr_data *msr)
+{
+	trace_kvm_msr_read(msr->index, msr->data);
+
+	/* FIXME: handling of bits 32:63 of rax, rdx */
+	vcpu->arch.regs[VCPU_REGS_RAX] = msr->data & -1u;
+	vcpu->arch.regs[VCPU_REGS_RDX] = (msr->data >> 32) & -1u;
+	skip_emulated_instruction(vcpu);
+}
+
+static void fail_rdmsr(struct kvm_vcpu *vcpu, const struct msr_data *msr)
+{
+	trace_kvm_msr_read_ex(msr->index);
+	kvm_inject_gp(vcpu, 0);
+}
+
+static void complete_wrmsr(struct kvm_vcpu *vcpu, const struct msr_data *msr)
+{
+	trace_kvm_msr_write(msr->index, msr->data);
+	skip_emulated_instruction(vcpu);
+}
+
+static void fail_wrmsr(struct kvm_vcpu *vcpu, const struct msr_data *msr)
+{
+	trace_kvm_msr_write_ex(msr->index, msr->data);
+	kvm_inject_gp(vcpu, 0);
+}
+
 static int handle_rdmsr(struct kvm_vcpu *vcpu)
 {
 	u32 ecx = vcpu->arch.regs[VCPU_REGS_RCX];
@@ -5471,17 +5499,12 @@ static int handle_rdmsr(struct kvm_vcpu *vcpu)
 	msr_info.index = ecx;
 	msr_info.host_initiated = false;
 	if (vmx_get_msr(vcpu, &msr_info)) {
-		trace_kvm_msr_read_ex(ecx);
-		kvm_inject_gp(vcpu, 0);
+		fail_rdmsr(vcpu, &msr_info);
 		return 1;
 	}
 
-	trace_kvm_msr_read(ecx, msr_info.data);
+	complete_rdmsr(vcpu, &msr_info);
 
-	/* FIXME: handling of bits 32:63 of rax, rdx */
-	vcpu->arch.regs[VCPU_REGS_RAX] = msr_info.data & -1u;
-	vcpu->arch.regs[VCPU_REGS_RDX] = (msr_info.data >> 32) & -1u;
-	skip_emulated_instruction(vcpu);
 	return 1;
 }
 
@@ -5496,13 +5519,12 @@ static int handle_wrmsr(struct kvm_vcpu *vcpu)
 	msr.index = ecx;
 	msr.host_initiated = false;
 	if (kvm_set_msr(vcpu, &msr) != 0) {
-		trace_kvm_msr_write_ex(ecx, data);
-		kvm_inject_gp(vcpu, 0);
+		fail_wrmsr(vcpu, &msr);
 		return 1;
 	}
 
-	trace_kvm_msr_write(ecx, data);
-	skip_emulated_instruction(vcpu);
+	complete_wrmsr(vcpu, &msr);
+
 	return 1;
 }
 
-- 
2.5.0.276.gf5e568e

[RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Peter Hornyack]

Define KVM_EXIT_MSR, a new exit reason for accesses to MSRs that kvm
does not handle. Define KVM_CAP_UNHANDLED_MSR_EXITS, a vm-wide
capability that guards the new exit reason and which can be enabled via
the KVM_ENABLE_CAP ioctl.

Signed-off-by: Peter Hornyack <peterhornyack@google.com>
---
 Documentation/virtual/kvm/api.txt | 48 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h          | 14 ++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index a4ebcb712375..bda540b3dd03 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3302,6 +3302,36 @@ Valid values for 'type' are:
    to ignore the request, or to gather VM memory core dump and/or
    reset/shutdown of the VM.
 
+		/* KVM_EXIT_MSR */
+		struct {
+#define KVM_EXIT_MSR_RDMSR             1
+#define KVM_EXIT_MSR_WRMSR             2
+#define KVM_EXIT_MSR_COMPLETION_FAILED 3
+			__u8 direction; /* out */
+#define KVM_EXIT_MSR_UNHANDLED 1
+#define KVM_EXIT_MSR_HANDLED   2
+			__u8 handled;   /* in */
+			__u32 index;    /* i.e. ecx; out */
+			__u64 data;     /* out (wrmsr) / in (rdmsr) */
+		} msr;
+
+If exit_reason is KVM_EXIT_MSR, then the vcpu has executed a rdmsr or wrmsr
+instruction which could not be satisfied by kvm. The msr struct is used for
+both output to and input from user space. direction indicates whether the
+instruction was rdmsr or wrmsr, and index is the target MSR number held in
+ecx. User space must not modify index. data holds the payload from a wrmsr or
+must be filled in with a payload on a rdmsr.
+
+On the return path into kvm, user space should set handled to
+KVM_EXIT_MSR_HANDLED if it successfully handled the MSR access; otherwise,
+handled should be set to KVM_EXIT_MSR_UNHANDLED, which will cause a general
+protection fault to be injected into the vcpu. If an error occurs during the
+return into kvm, the vcpu will not be run and another KVM_EXIT_MSR will be
+generated with direction KVM_EXIT_MSR_COMPLETION_FAILED.
+
+KVM_EXIT_MSR can only occur when KVM_CAP_UNHANDLED_MSR_EXITS has been enabled;
+a detailed description of this capability is below.
+
 		/* Fix the size of the union. */
 		char padding[256];
 	};
@@ -3620,6 +3650,24 @@ struct {
 
 KVM handlers should exit to userspace with rc = -EREMOTE.
 
+7.5 KVM_CAP_UNHANDLED_MSR_EXITS
+
+Architectures: x86 (vmx-only)
+Parameters: args[0] enables or disables unhandled MSR exits
+Returns: 0 on success; -1 on error
+
+This capability enables exits to user space on unhandled MSR accesses.
+
+When enabled (args[0] != 0), when the guest accesses an MSR that kvm does not
+handle kvm will exit to user space with the reason KVM_EXIT_MSR. When disabled
+(by default, or with args[0] == 0), when the guest accesses an MSR that kvm
+does not handle a GP fault is immediately injected into the guest.
+
+Currently only implemented for vmx; attempts to enable this capability on svm
+systems will return an error. Also, note that this capability is overridden if
+the kvm module's ignore_msrs flag is set, in which case unhandled MSR accesses
+are simply ignored and the guest is re-entered immediately.
+
 
 8. Other capabilities.
 ----------------------
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 0d831f94f8a8..43d2d1e15ac4 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -183,6 +183,7 @@ struct kvm_s390_skeys {
 #define KVM_EXIT_EPR              23
 #define KVM_EXIT_SYSTEM_EVENT     24
 #define KVM_EXIT_S390_STSI        25
+#define KVM_EXIT_MSR              26
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */
@@ -330,6 +331,18 @@ struct kvm_run {
 			__u8 sel1;
 			__u16 sel2;
 		} s390_stsi;
+		/* KVM_EXIT_MSR */
+		struct {
+#define KVM_EXIT_MSR_RDMSR             1
+#define KVM_EXIT_MSR_WRMSR             2
+#define KVM_EXIT_MSR_COMPLETION_FAILED 3
+			__u8 direction; /* out */
+#define KVM_EXIT_MSR_UNHANDLED 1
+#define KVM_EXIT_MSR_HANDLED   2
+			__u8 handled;   /* in */
+			__u32 index;    /* i.e. ecx; out */
+			__u64 data;     /* out (wrmsr) / in (rdmsr) */
+		} msr;
 		/* Fix the size of the union. */
 		char padding[256];
 	};
@@ -819,6 +832,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_DISABLE_QUIRKS 116
 #define KVM_CAP_X86_SMM 117
 #define KVM_CAP_MULTI_ADDRESS_SPACE 118
+#define KVM_CAP_UNHANDLED_MSR_EXITS 119
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.5.0.276.gf5e568e

[RFC PATCH 3/5] KVM: x86: add msr_exits_supported to kvm_x86_ops

[Peter Hornyack]

msr_exits_supported will be checked when user space attempts to enable
the KVM_CAP_UNHANDLED_MSR_EXITS capability for the vm. This is needed
because MSR exit support will be implemented for vmx but not svm later
in this patchset.

Signed-off-by: Peter Hornyack <peterhornyack@google.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/svm.c              | 6 ++++++
 arch/x86/kvm/vmx.c              | 6 ++++++
 3 files changed, 13 insertions(+)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c12e845f59e6..a6e145b1e271 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -854,6 +854,7 @@ struct kvm_x86_ops {
 	void (*handle_external_intr)(struct kvm_vcpu *vcpu);
 	bool (*mpx_supported)(void);
 	bool (*xsaves_supported)(void);
+	bool (*msr_exits_supported)(void);
 
 	int (*check_nested_events)(struct kvm_vcpu *vcpu, bool external_intr);
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 74d825716f4f..bcbb56f49b9f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -4249,6 +4249,11 @@ static bool svm_xsaves_supported(void)
 	return false;
 }
 
+static bool svm_msr_exits_supported(void)
+{
+	return false;
+}
+
 static bool svm_has_wbinvd_exit(void)
 {
 	return true;
@@ -4540,6 +4545,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.invpcid_supported = svm_invpcid_supported,
 	.mpx_supported = svm_mpx_supported,
 	.xsaves_supported = svm_xsaves_supported,
+	.msr_exits_supported = svm_msr_exits_supported,
 
 	.set_supported_cpuid = svm_set_supported_cpuid,
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index acc38e27d221..27fec385d79d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8161,6 +8161,11 @@ static bool vmx_xsaves_supported(void)
 		SECONDARY_EXEC_XSAVES;
 }
 
+static bool vmx_msr_exits_supported(void)
+{
+	return false;
+}
+
 static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
 {
 	u32 exit_intr_info;
@@ -10413,6 +10418,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.handle_external_intr = vmx_handle_external_intr,
 	.mpx_supported = vmx_mpx_supported,
 	.xsaves_supported = vmx_xsaves_supported,
+	.msr_exits_supported = vmx_msr_exits_supported,
 
 	.check_nested_events = vmx_check_nested_events,
 
-- 
2.5.0.276.gf5e568e

[RFC PATCH 4/5] KVM: x86: enable unhandled MSR exits for vmx

[Peter Hornyack]

Set the vm's unhandled_msr_exits flag when user space calls the
KVM_ENABLE_CAP ioctl with KVM_CAP_UNHANDLED_MSR_EXITS. After kvm fails
to handle a guest rdmsr or wrmsr, check this flag and exit to user space
with KVM_EXIT_MSR rather than immediately injecting a GP fault.

On reentry into kvm, use the complete_userspace_io callback path to call
vmx_complete_userspace_msr. Complete the MSR access if user space was
able to handle it successfully, or fail the MSR access and inject a GP
fault if user space could not handle the access.

Signed-off-by: Peter Hornyack <peterhornyack@google.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/vmx.c              | 74 ++++++++++++++++++++++++++++++++++++++++-
 arch/x86/kvm/x86.c              | 12 +++++++
 3 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a6e145b1e271..1b06cea06a8e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -682,6 +682,7 @@ struct kvm_arch {
 	u32 bsp_vcpu_id;
 
 	u64 disabled_quirks;
+	bool unhandled_msr_exits;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 27fec385d79d..ba26d382d785 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -32,6 +32,7 @@
 #include <linux/slab.h>
 #include <linux/tboot.h>
 #include <linux/hrtimer.h>
+#include <uapi/linux/kvm.h>
 #include "kvm_cache_regs.h"
 #include "x86.h"
 
@@ -5491,6 +5492,53 @@ static void fail_wrmsr(struct kvm_vcpu *vcpu, const struct msr_data *msr)
 	kvm_inject_gp(vcpu, 0);
 }
 
+/*
+ * On success, returns 1 so that __vcpu_run() will happen next. On error,
+ * returns 0.
+ */
+static int vmx_complete_userspace_msr(struct kvm_vcpu *vcpu)
+{
+	struct msr_data msr;
+
+	if (vcpu->run->msr.index != vcpu->arch.regs[VCPU_REGS_RCX]) {
+		pr_debug("msr.index 0x%x changed, does not match ecx 0x%lx\n",
+			 vcpu->run->msr.index,
+			 vcpu->arch.regs[VCPU_REGS_RCX]);
+		goto err_out;
+	}
+	msr.index = vcpu->run->msr.index;
+	msr.data = vcpu->run->msr.data;
+	msr.host_initiated = false;
+
+	switch (vcpu->run->msr.direction) {
+	case KVM_EXIT_MSR_RDMSR:
+		if (vcpu->run->msr.handled == KVM_EXIT_MSR_HANDLED)
+			complete_rdmsr(vcpu, &msr);
+		else
+			fail_rdmsr(vcpu, &msr);
+		break;
+	case KVM_EXIT_MSR_WRMSR:
+		if (vcpu->run->msr.handled == KVM_EXIT_MSR_HANDLED)
+			complete_wrmsr(vcpu, &msr);
+		else
+			fail_wrmsr(vcpu, &msr);
+		break;
+	default:
+		pr_debug("bad msr.direction %u\n", vcpu->run->msr.direction);
+		goto err_out;
+	}
+
+	return 1;
+err_out:
+	vcpu->run->exit_reason = KVM_EXIT_MSR;
+	vcpu->run->msr.direction = KVM_EXIT_MSR_COMPLETION_FAILED;
+	return 0;
+}
+
+/*
+ * Returns 1 if the rdmsr handling is complete; returns 0 if kvm should exit to
+ * user space to handle this rdmsr.
+ */
 static int handle_rdmsr(struct kvm_vcpu *vcpu)
 {
 	u32 ecx = vcpu->arch.regs[VCPU_REGS_RCX];
@@ -5499,6 +5547,16 @@ static int handle_rdmsr(struct kvm_vcpu *vcpu)
 	msr_info.index = ecx;
 	msr_info.host_initiated = false;
 	if (vmx_get_msr(vcpu, &msr_info)) {
+		if (vcpu->kvm->arch.unhandled_msr_exits) {
+			vcpu->run->exit_reason = KVM_EXIT_MSR;
+			vcpu->run->msr.direction = KVM_EXIT_MSR_RDMSR;
+			vcpu->run->msr.index = msr_info.index;
+			vcpu->run->msr.data = 0;
+			vcpu->run->msr.handled = 0;
+			vcpu->arch.complete_userspace_io =
+					vmx_complete_userspace_msr;
+			return 0;
+		}
 		fail_rdmsr(vcpu, &msr_info);
 		return 1;
 	}
@@ -5508,6 +5566,10 @@ static int handle_rdmsr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+/*
+ * Returns 1 if the wrmsr handling is complete; returns 0 if kvm should exit to
+ * user space to handle this wrmsr.
+ */
 static int handle_wrmsr(struct kvm_vcpu *vcpu)
 {
 	struct msr_data msr;
@@ -5519,6 +5581,16 @@ static int handle_wrmsr(struct kvm_vcpu *vcpu)
 	msr.index = ecx;
 	msr.host_initiated = false;
 	if (kvm_set_msr(vcpu, &msr) != 0) {
+		if (vcpu->kvm->arch.unhandled_msr_exits) {
+			vcpu->run->exit_reason = KVM_EXIT_MSR;
+			vcpu->run->msr.direction = KVM_EXIT_MSR_WRMSR;
+			vcpu->run->msr.index = msr.index;
+			vcpu->run->msr.data = msr.data;
+			vcpu->run->msr.handled = 0;
+			vcpu->arch.complete_userspace_io =
+					vmx_complete_userspace_msr;
+			return 0;
+		}
 		fail_wrmsr(vcpu, &msr);
 		return 1;
 	}
@@ -8163,7 +8235,7 @@ static bool vmx_xsaves_supported(void)
 
 static bool vmx_msr_exits_supported(void)
 {
-	return false;
+	return true;
 }
 
 static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4bbc2a1676c9..5c22f4655741 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2461,6 +2461,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ENABLE_CAP_VM:
 	case KVM_CAP_DISABLE_QUIRKS:
 	case KVM_CAP_SET_BOOT_CPU_ID:
+	case KVM_CAP_UNHANDLED_MSR_EXITS:
 #ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
 	case KVM_CAP_ASSIGN_DEV_IRQ:
 	case KVM_CAP_PCI_2_3:
@@ -3568,6 +3569,17 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		kvm->arch.disabled_quirks = cap->args[0];
 		r = 0;
 		break;
+	case KVM_CAP_UNHANDLED_MSR_EXITS:
+		if (kvm_x86_ops->msr_exits_supported()) {
+			if (cap->args[0])
+				kvm->arch.unhandled_msr_exits = true;
+			else
+				kvm->arch.unhandled_msr_exits = false;
+			r = 0;
+		} else {
+			r = -EINVAL;
+		}
+		break;
 	default:
 		r = -EINVAL;
 		break;
-- 
2.5.0.276.gf5e568e

[RFC PATCH 5/5] KVM: x86: add trace events for unhandled MSR exits

[Peter Hornyack]

Add trace_kvm_userspace_msr and call it when user space reenters kvm
after KVM_EXIT_MSR.

Add KVM_EXIT_MSR to kvm_trace_exit_reason list.

Signed-off-by: Peter Hornyack <peterhornyack@google.com>
---
 arch/x86/kvm/trace.h       | 28 ++++++++++++++++++++++++++++
 arch/x86/kvm/vmx.c         |  4 ++++
 arch/x86/kvm/x86.c         |  1 +
 include/trace/events/kvm.h |  2 +-
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 4eae7c35ddf5..6d144d424896 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -330,6 +330,34 @@ TRACE_EVENT(kvm_msr,
 #define trace_kvm_msr_read_ex(ecx)         trace_kvm_msr(0, ecx, 0, true)
 #define trace_kvm_msr_write_ex(ecx, data)  trace_kvm_msr(1, ecx, data, true)
 
+TRACE_EVENT(kvm_userspace_msr,
+	TP_PROTO(u8 direction, u8 handled, u32 index, u64 data),
+	TP_ARGS(direction, handled, index, data),
+
+	TP_STRUCT__entry(
+		__field(u8, direction)
+		__field(u8, handled)
+		__field(u32, index)
+		__field(u64, data)
+	),
+
+	TP_fast_assign(
+		__entry->direction	= direction;
+		__entry->handled	= handled;
+		__entry->index		= index;
+		__entry->data		= data;
+	),
+
+	TP_printk("userspace %s %x = 0x%llx, %s",
+		  __entry->direction == KVM_EXIT_MSR_RDMSR ? "rdmsr" :
+		  __entry->direction == KVM_EXIT_MSR_WRMSR ? "wrmsr" :
+							     "unknown!",
+		  __entry->index, __entry->data,
+		  __entry->handled == KVM_EXIT_MSR_UNHANDLED ? "unhandled" :
+		  __entry->handled == KVM_EXIT_MSR_HANDLED   ? "handled" :
+							       "unknown!")
+);
+
 /*
  * Tracepoint for guest CR access.
  */
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ba26d382d785..46d276235f78 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5500,6 +5500,10 @@ static int vmx_complete_userspace_msr(struct kvm_vcpu *vcpu)
 {
 	struct msr_data msr;
 
+	trace_kvm_userspace_msr(vcpu->run->msr.direction,
+				vcpu->run->msr.handled, vcpu->run->msr.index,
+				vcpu->run->msr.data);
+
 	if (vcpu->run->msr.index != vcpu->arch.regs[VCPU_REGS_RCX]) {
 		pr_debug("msr.index 0x%x changed, does not match ecx 0x%lx\n",
 			 vcpu->run->msr.index,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5c22f4655741..cc74ba1d01e6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8040,6 +8040,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_inj_virq);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_page_fault);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_msr);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_userspace_msr);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_cr);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmrun);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_nested_vmexit);
diff --git a/include/trace/events/kvm.h b/include/trace/events/kvm.h
index a44062da684b..aa6ce656d658 100644
--- a/include/trace/events/kvm.h
+++ b/include/trace/events/kvm.h
@@ -14,7 +14,7 @@
 	ERSN(SHUTDOWN), ERSN(FAIL_ENTRY), ERSN(INTR), ERSN(SET_TPR),	\
 	ERSN(TPR_ACCESS), ERSN(S390_SIEIC), ERSN(S390_RESET), ERSN(DCR),\
 	ERSN(NMI), ERSN(INTERNAL_ERROR), ERSN(OSI), ERSN(PAPR_HCALL),	\
-	ERSN(S390_UCONTROL), ERSN(WATCHDOG), ERSN(S390_TSCH)
+	ERSN(S390_UCONTROL), ERSN(WATCHDOG), ERSN(S390_TSCH), ERSN(MSR)
 
 TRACE_EVENT(kvm_userspace_exit,
 	    TP_PROTO(__u32 reason, int errno),
-- 
2.5.0.276.gf5e568e

Re: [RFC PATCH 0/5] KVM: x86: exit to user space on unhandled MSR accesses

[Bandan Das]

Peter Hornyack <peterhornyack@google.com> writes:

> There are numerous MSRs that kvm does not currently handle. On Intel
> platforms we have observed guest VMs accessing some of these MSRs (for
> example, MSR_PLATFORM_INFO) and behaving poorly (to the point of guest OS
> crashes) when they receive a GP fault because the MSR is not emulated. This
> patchset adds a new kvm exit path for unhandled MSR accesses that allows
> user space to emulate additional MSRs without having to implement them in
> kvm.

^^ So, I am trying to understand this motivation. A while back when 
a patch was posted to emulate MSR_PLATFORM_INFO, it was rejected.
Why ? Because it seemed impossible to emulate it correctly (most concerns were
related to migration iirc). Although I haven't reviewed all patches in this series
yet, what I understand from the above message is-"It's ok to emulate
MSR_PLATFORM_INFO *incorrectly* as long as we are doing it in the user space."

I understand the part where it makes sense to move stuff to userspace.
But if kvm isn't emulating certain msrs yet, either we should add support,
or they haven't been added because it's not possible to emulate them
correctly. The logic that it's probably ok to let userspace do the (incorrect)
emulation is something I don't understand. It seems like the next in line
is to let userspace emulate thier own version of unimplemented x86 instructions.


> The core of the patchset modifies the vmx handle_rdmsr and handle_wrmsr
> functions to exit to user space on MSR reads/writes that kvm can't handle
> itself. Then, on the return path into kvm we check for outstanding user
> space MSR completions and either complete the MSR access successfully or
> inject a GP fault as kvm would do by default. This new exit path must be
> enabled for the vm via the KVM_CAP_UNHANDLED_MSR_EXITS capability.
>
> In the future we plan to extend this functionality to allow user space to
> register the MSRs that it would like to handle itself, even if kvm already
> provides an implementation. In the long-term we will move the

I seriously hope we don't do this!

Bandan
> implementation of all non-performance-sensitive MSRs to user space,
> reducing the potential attack surface of kvm and allowing us to respond to
> bugs more quickly.
>
> This patchset has been tested with our non-qemu user space hypervisor on
> vmx platforms; svm support is not implemented.
>
> Peter Hornyack (5):
>   KVM: x86: refactor vmx rdmsr/wrmsr completion into new functions
>   KVM: add KVM_EXIT_MSR exit reason and capability.
>   KVM: x86: add msr_exits_supported to kvm_x86_ops
>   KVM: x86: enable unhandled MSR exits for vmx
>   KVM: x86: add trace events for unhandled MSR exits
>
>  Documentation/virtual/kvm/api.txt |  48 +++++++++++++++
>  arch/x86/include/asm/kvm_host.h   |   2 +
>  arch/x86/kvm/svm.c                |   6 ++
>  arch/x86/kvm/trace.h              |  28 +++++++++
>  arch/x86/kvm/vmx.c                | 126 ++++++++++++++++++++++++++++++++++----
>  arch/x86/kvm/x86.c                |  13 ++++
>  include/trace/events/kvm.h        |   2 +-
>  include/uapi/linux/kvm.h          |  14 +++++
>  8 files changed, 227 insertions(+), 12 deletions(-)

Re: [RFC PATCH 0/5] KVM: x86: exit to user space on unhandled MSR accesses

[Peter Hornyack]

On Wed, Aug 19, 2015 at 2:43 PM, Bandan Das <bsd@redhat.com> wrote:
> Peter Hornyack <peterhornyack@google.com> writes:
>
>> There are numerous MSRs that kvm does not currently handle. On Intel
>> platforms we have observed guest VMs accessing some of these MSRs (for
>> example, MSR_PLATFORM_INFO) and behaving poorly (to the point of guest OS
>> crashes) when they receive a GP fault because the MSR is not emulated. This
>> patchset adds a new kvm exit path for unhandled MSR accesses that allows
>> user space to emulate additional MSRs without having to implement them in
>> kvm.
>
> ^^ So, I am trying to understand this motivation. A while back when
> a patch was posted to emulate MSR_PLATFORM_INFO, it was rejected.
> Why ? Because it seemed impossible to emulate it correctly (most concerns were
> related to migration iirc). Although I haven't reviewed all patches in this series
> yet, what I understand from the above message is-"It's ok to emulate
> MSR_PLATFORM_INFO *incorrectly* as long as we are doing it in the user space."
>
> I understand the part where it makes sense to move stuff to userspace.
> But if kvm isn't emulating certain msrs yet, either we should add support,
> or they haven't been added because it's not possible to emulate them
> correctly. The logic that it's probably ok to let userspace do the (incorrect)
> emulation is something I don't understand.

I wasn't aware of the past discussion of MSR_PLATFORM_INFO, I'll
search for it - thanks for pointing it out.

MSR_PLATFORM_INFO is just one example though. In addition to the
benefits I already mentioned for user space MSR emulation (agility,
reduced attack surface), this patchset offers a benefit even if user
space declines to emulate the MSR by returning into kvm with
KVM_EXIT_MSR_UNHANDLED: it makes it easier to monitor in the first
place, via logging mechanisms in user space instead of in the kernel,
for when guests are accessing MSRs that kvm doesn't implement.

This patchset has already revealed a few other MSRs (IA32_MPERF,
IA32_APERF) that guests in our test environment are accessing but
which kvm doesn't implement yet. I haven't yet investigated deeply
what these MSRs are used for and how they might be emulated (or if
they can't actually be emulated correctly), but if we do discover an
unimplemented non-performance-sensitive MSR that we could emulate
correctly, with this patchset we would quickly just implement it in
user space.

> It seems like the next in line
> is to let userspace emulate thier own version of unimplemented x86 instructions.
>
>
>> The core of the patchset modifies the vmx handle_rdmsr and handle_wrmsr
>> functions to exit to user space on MSR reads/writes that kvm can't handle
>> itself. Then, on the return path into kvm we check for outstanding user
>> space MSR completions and either complete the MSR access successfully or
>> inject a GP fault as kvm would do by default. This new exit path must be
>> enabled for the vm via the KVM_CAP_UNHANDLED_MSR_EXITS capability.
>>
>> In the future we plan to extend this functionality to allow user space to
>> register the MSRs that it would like to handle itself, even if kvm already
>> provides an implementation. In the long-term we will move the
>
> I seriously hope we don't do this!
>
> Bandan
>> implementation of all non-performance-sensitive MSRs to user space,
>> reducing the potential attack surface of kvm and allowing us to respond to
>> bugs more quickly.
>>
>> This patchset has been tested with our non-qemu user space hypervisor on
>> vmx platforms; svm support is not implemented.
>>
>> Peter Hornyack (5):
>>   KVM: x86: refactor vmx rdmsr/wrmsr completion into new functions
>>   KVM: add KVM_EXIT_MSR exit reason and capability.
>>   KVM: x86: add msr_exits_supported to kvm_x86_ops
>>   KVM: x86: enable unhandled MSR exits for vmx
>>   KVM: x86: add trace events for unhandled MSR exits
>>
>>  Documentation/virtual/kvm/api.txt |  48 +++++++++++++++
>>  arch/x86/include/asm/kvm_host.h   |   2 +
>>  arch/x86/kvm/svm.c                |   6 ++
>>  arch/x86/kvm/trace.h              |  28 +++++++++
>>  arch/x86/kvm/vmx.c                | 126 ++++++++++++++++++++++++++++++++++----
>>  arch/x86/kvm/x86.c                |  13 ++++
>>  include/trace/events/kvm.h        |   2 +-
>>  include/uapi/linux/kvm.h          |  14 +++++
>>  8 files changed, 227 insertions(+), 12 deletions(-)

Re: [RFC PATCH 4/5] KVM: x86: enable unhandled MSR exits for vmx

[Bandan Das]

Peter Hornyack <peterhornyack@google.com> writes:

> Set the vm's unhandled_msr_exits flag when user space calls the
> KVM_ENABLE_CAP ioctl with KVM_CAP_UNHANDLED_MSR_EXITS. After kvm fails
> to handle a guest rdmsr or wrmsr, check this flag and exit to user space
> with KVM_EXIT_MSR rather than immediately injecting a GP fault.
>
> On reentry into kvm, use the complete_userspace_io callback path to call
> vmx_complete_userspace_msr. Complete the MSR access if user space was
> able to handle it successfully, or fail the MSR access and inject a GP
> fault if user space could not handle the access.
>
...
> +static int vmx_complete_userspace_msr(struct kvm_vcpu *vcpu)
> +{
> +	struct msr_data msr;
> +
> +	if (vcpu->run->msr.index != vcpu->arch.regs[VCPU_REGS_RCX]) {
> +		pr_debug("msr.index 0x%x changed, does not match ecx 0x%lx\n",
> +			 vcpu->run->msr.index,
> +			 vcpu->arch.regs[VCPU_REGS_RCX]);
> +		goto err_out;
> +	}
> +	msr.index = vcpu->run->msr.index;
> +	msr.data = vcpu->run->msr.data;
> +	msr.host_initiated = false;
> +
> +	switch (vcpu->run->msr.direction) {
> +	case KVM_EXIT_MSR_RDMSR:
> +		if (vcpu->run->msr.handled == KVM_EXIT_MSR_HANDLED)
> +			complete_rdmsr(vcpu, &msr);
> +		else
> +			fail_rdmsr(vcpu, &msr);
> +		break;

Should we check for MSR_UNHANDLED ? Could be "debugging relief" if userspace is
filling it up with random crap! I think it should be ok if the trace function
catches that too.

> +	case KVM_EXIT_MSR_WRMSR:
> +		if (vcpu->run->msr.handled == KVM_EXIT_MSR_HANDLED)
> +			complete_wrmsr(vcpu, &msr);
> +		else
> +			fail_wrmsr(vcpu, &msr);
> +		break;
> +	default:
> +		pr_debug("bad msr.direction %u\n", vcpu->run->msr.direction);
> +		goto err_out;
> +	}
> +
> +	return 1;
> +err_out:
> +	vcpu->run->exit_reason = KVM_EXIT_MSR;
> +	vcpu->run->msr.direction = KVM_EXIT_MSR_COMPLETION_FAILED;
> +	return 0;
> +}
> +
> +/*
> + * Returns 1 if the rdmsr handling is complete; returns 0 if kvm should exit to
> + * user space to handle this rdmsr.
> + */
>  static int handle_rdmsr(struct kvm_vcpu *vcpu)
>  {
>  	u32 ecx = vcpu->arch.regs[VCPU_REGS_RCX];
> @@ -5499,6 +5547,16 @@ static int handle_rdmsr(struct kvm_vcpu *vcpu)
>  	msr_info.index = ecx;
>  	msr_info.host_initiated = false;
>  	if (vmx_get_msr(vcpu, &msr_info)) {
> +		if (vcpu->kvm->arch.unhandled_msr_exits) {
> +			vcpu->run->exit_reason = KVM_EXIT_MSR;
> +			vcpu->run->msr.direction = KVM_EXIT_MSR_RDMSR;
> +			vcpu->run->msr.index = msr_info.index;
> +			vcpu->run->msr.data = 0;
> +			vcpu->run->msr.handled = 0;
> +			vcpu->arch.complete_userspace_io =
> +					vmx_complete_userspace_msr;
> +			return 0;
> +		}
>  		fail_rdmsr(vcpu, &msr_info);
>  		return 1;
>  	}
> @@ -5508,6 +5566,10 @@ static int handle_rdmsr(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> +/*
> + * Returns 1 if the wrmsr handling is complete; returns 0 if kvm should exit to
> + * user space to handle this wrmsr.
> + */
>  static int handle_wrmsr(struct kvm_vcpu *vcpu)
>  {
>  	struct msr_data msr;
> @@ -5519,6 +5581,16 @@ static int handle_wrmsr(struct kvm_vcpu *vcpu)
>  	msr.index = ecx;
>  	msr.host_initiated = false;
>  	if (kvm_set_msr(vcpu, &msr) != 0) {
> +		if (vcpu->kvm->arch.unhandled_msr_exits) {

I think kvm should ultimately decide which msrs it can let userspace
handle even if userspace has explicitly enabled the ioctl.

Bandan

> +			vcpu->run->exit_reason = KVM_EXIT_MSR;
> +			vcpu->run->msr.direction = KVM_EXIT_MSR_WRMSR;
> +			vcpu->run->msr.index = msr.index;
> +			vcpu->run->msr.data = msr.data;
> +			vcpu->run->msr.handled = 0;
> +			vcpu->arch.complete_userspace_io =
> +					vmx_complete_userspace_msr;
> +			return 0;
> +		}
>  		fail_wrmsr(vcpu, &msr);
>  		return 1;
>  	}
> @@ -8163,7 +8235,7 @@ static bool vmx_xsaves_supported(void)
>  
>  static bool vmx_msr_exits_supported(void)
>  {
> -	return false;
> +	return true;
>  }
>  
>  static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4bbc2a1676c9..5c22f4655741 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2461,6 +2461,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ENABLE_CAP_VM:
>  	case KVM_CAP_DISABLE_QUIRKS:
>  	case KVM_CAP_SET_BOOT_CPU_ID:
> +	case KVM_CAP_UNHANDLED_MSR_EXITS:
>  #ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
>  	case KVM_CAP_ASSIGN_DEV_IRQ:
>  	case KVM_CAP_PCI_2_3:
> @@ -3568,6 +3569,17 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		kvm->arch.disabled_quirks = cap->args[0];
>  		r = 0;
>  		break;
> +	case KVM_CAP_UNHANDLED_MSR_EXITS:
> +		if (kvm_x86_ops->msr_exits_supported()) {
> +			if (cap->args[0])
> +				kvm->arch.unhandled_msr_exits = true;
> +			else
> +				kvm->arch.unhandled_msr_exits = false;
> +			r = 0;
> +		} else {
> +			r = -EINVAL;
> +		}
> +		break;
>  	default:
>  		r = -EINVAL;
>  		break;

Re: [RFC PATCH 3/5] KVM: x86: add msr_exits_supported to kvm_x86_ops

[Bandan Das]

Peter Hornyack <peterhornyack@google.com> writes:

> msr_exits_supported will be checked when user space attempts to enable
> the KVM_CAP_UNHANDLED_MSR_EXITS capability for the vm. This is needed
> because MSR exit support will be implemented for vmx but not svm later
> in this patchset.

Is svm future work ? :) Are there any such svm msrs ?


> Signed-off-by: Peter Hornyack <peterhornyack@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/svm.c              | 6 ++++++
>  arch/x86/kvm/vmx.c              | 6 ++++++
>  3 files changed, 13 insertions(+)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c12e845f59e6..a6e145b1e271 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -854,6 +854,7 @@ struct kvm_x86_ops {
>  	void (*handle_external_intr)(struct kvm_vcpu *vcpu);
>  	bool (*mpx_supported)(void);
>  	bool (*xsaves_supported)(void);
> +	bool (*msr_exits_supported)(void);
>  
>  	int (*check_nested_events)(struct kvm_vcpu *vcpu, bool external_intr);
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 74d825716f4f..bcbb56f49b9f 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -4249,6 +4249,11 @@ static bool svm_xsaves_supported(void)
>  	return false;
>  }
>  
> +static bool svm_msr_exits_supported(void)
> +{
> +	return false;
> +}
> +
>  static bool svm_has_wbinvd_exit(void)
>  {
>  	return true;
> @@ -4540,6 +4545,7 @@ static struct kvm_x86_ops svm_x86_ops = {
>  	.invpcid_supported = svm_invpcid_supported,
>  	.mpx_supported = svm_mpx_supported,
>  	.xsaves_supported = svm_xsaves_supported,
> +	.msr_exits_supported = svm_msr_exits_supported,
>  
>  	.set_supported_cpuid = svm_set_supported_cpuid,
>  
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index acc38e27d221..27fec385d79d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8161,6 +8161,11 @@ static bool vmx_xsaves_supported(void)
>  		SECONDARY_EXEC_XSAVES;
>  }
>  
> +static bool vmx_msr_exits_supported(void)
> +{
> +	return false;
> +}
> +
>  static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx)
>  {
>  	u32 exit_intr_info;
> @@ -10413,6 +10418,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
>  	.handle_external_intr = vmx_handle_external_intr,
>  	.mpx_supported = vmx_mpx_supported,
>  	.xsaves_supported = vmx_xsaves_supported,
> +	.msr_exits_supported = vmx_msr_exits_supported,
>  
>  	.check_nested_events = vmx_check_nested_events,

Re: [RFC PATCH 0/5] KVM: x86: exit to user space on unhandled MSR accesses

[Bandan Das]

Peter Hornyack <peterhornyack@google.com> writes:

> On Wed, Aug 19, 2015 at 2:43 PM, Bandan Das <bsd@redhat.com> wrote:
>> Peter Hornyack <peterhornyack@google.com> writes:
>>
>>> There are numerous MSRs that kvm does not currently handle. On Intel
>>> platforms we have observed guest VMs accessing some of these MSRs (for
>>> example, MSR_PLATFORM_INFO) and behaving poorly (to the point of guest OS
>>> crashes) when they receive a GP fault because the MSR is not emulated. This
>>> patchset adds a new kvm exit path for unhandled MSR accesses that allows
>>> user space to emulate additional MSRs without having to implement them in
>>> kvm.
>>
>> ^^ So, I am trying to understand this motivation. A while back when
>> a patch was posted to emulate MSR_PLATFORM_INFO, it was rejected.
>> Why ? Because it seemed impossible to emulate it correctly (most concerns were
>> related to migration iirc). Although I haven't reviewed all patches in this series
>> yet, what I understand from the above message is-"It's ok to emulate
>> MSR_PLATFORM_INFO *incorrectly* as long as we are doing it in the user space."
>>
>> I understand the part where it makes sense to move stuff to userspace.
>> But if kvm isn't emulating certain msrs yet, either we should add support,
>> or they haven't been added because it's not possible to emulate them
>> correctly. The logic that it's probably ok to let userspace do the (incorrect)
>> emulation is something I don't understand.
>
> I wasn't aware of the past discussion of MSR_PLATFORM_INFO, I'll
> search for it - thanks for pointing it out.
>
> MSR_PLATFORM_INFO is just one example though. In addition to the
> benefits I already mentioned for user space MSR emulation (agility,
> reduced attack surface), this patchset offers a benefit even if user
> space declines to emulate the MSR by returning into kvm with
> KVM_EXIT_MSR_UNHANDLED: it makes it easier to monitor in the first
> place, via logging mechanisms in user space instead of in the kernel,
> for when guests are accessing MSRs that kvm doesn't implement.

Agreed, that would be more useful then how it's being handled currently.

> This patchset has already revealed a few other MSRs (IA32_MPERF,
> IA32_APERF) that guests in our test environment are accessing but
> which kvm doesn't implement yet. I haven't yet investigated deeply
> what these MSRs are used for and how they might be emulated (or if
> they can't actually be emulated correctly), but if we do discover an
> unimplemented non-performance-sensitive MSR that we could emulate
> correctly, with this patchset we would quickly just implement it in
> user space.

Ok, makes sense. But it seems like this could be an area of abuse as
well. For example, this could lessen the incentive to add emulation for
new msrs in kvm and rather just emulate them in userspace ?

However, as you mentioned, it's probably ok as long as there's a clear
demarcation of which msrs this is done for. This shouldn't be
the default behavior.

On a different note, if there's a related qemu change, can you please
point me to it ?

Thanks,
Bandan

>> It seems like the next in line
>> is to let userspace emulate thier own version of unimplemented x86 instructions.
>>
>>
>>> The core of the patchset modifies the vmx handle_rdmsr and handle_wrmsr
>>> functions to exit to user space on MSR reads/writes that kvm can't handle
>>> itself. Then, on the return path into kvm we check for outstanding user
>>> space MSR completions and either complete the MSR access successfully or
>>> inject a GP fault as kvm would do by default. This new exit path must be
>>> enabled for the vm via the KVM_CAP_UNHANDLED_MSR_EXITS capability.
>>>
>>> In the future we plan to extend this functionality to allow user space to
>>> register the MSRs that it would like to handle itself, even if kvm already
>>> provides an implementation. In the long-term we will move the
>>
>> I seriously hope we don't do this!
>>
>> Bandan
>>> implementation of all non-performance-sensitive MSRs to user space,
>>> reducing the potential attack surface of kvm and allowing us to respond to
>>> bugs more quickly.
>>>
>>> This patchset has been tested with our non-qemu user space hypervisor on
>>> vmx platforms; svm support is not implemented.
>>>
>>> Peter Hornyack (5):
>>>   KVM: x86: refactor vmx rdmsr/wrmsr completion into new functions
>>>   KVM: add KVM_EXIT_MSR exit reason and capability.
>>>   KVM: x86: add msr_exits_supported to kvm_x86_ops
>>>   KVM: x86: enable unhandled MSR exits for vmx
>>>   KVM: x86: add trace events for unhandled MSR exits
>>>
>>>  Documentation/virtual/kvm/api.txt |  48 +++++++++++++++
>>>  arch/x86/include/asm/kvm_host.h   |   2 +
>>>  arch/x86/kvm/svm.c                |   6 ++
>>>  arch/x86/kvm/trace.h              |  28 +++++++++
>>>  arch/x86/kvm/vmx.c                | 126 ++++++++++++++++++++++++++++++++++----
>>>  arch/x86/kvm/x86.c                |  13 ++++
>>>  include/trace/events/kvm.h        |   2 +-
>>>  include/uapi/linux/kvm.h          |  14 +++++
>>>  8 files changed, 227 insertions(+), 12 deletions(-)

Re: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Paolo Bonzini]

On 18/08/2015 20:46, Peter Hornyack wrote:
> Define KVM_EXIT_MSR, a new exit reason for accesses to MSRs that kvm
> does not handle. Define KVM_CAP_UNHANDLED_MSR_EXITS, a vm-wide
> capability that guards the new exit reason and which can be enabled via
> the KVM_ENABLE_CAP ioctl.
> 
> Signed-off-by: Peter Hornyack <peterhornyack@google.com>
> ---
>  Documentation/virtual/kvm/api.txt | 48 +++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          | 14 ++++++++++++
>  2 files changed, 62 insertions(+)

Let's instead add:

- KVM_CAP_MSR_EXITS

- KVM_CAP_ENABLE_MSR_EXIT

- KVM_CAP_DISABLE_MSR_EXIT

and 3 kinds of exits: KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
KVM_EXIT_MSR_AFTER_WRITE.  The first two use four fields (type, msr,
data, handled) and #GP if handled=0 is zero on the next entry.  The last
one uses the first three fields and can be used for HyperV.

Paolo

Re: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Peter Hornyack]

On Fri, Dec 18, 2015 at 1:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 18/08/2015 20:46, Peter Hornyack wrote:
>> Define KVM_EXIT_MSR, a new exit reason for accesses to MSRs that kvm
>> does not handle. Define KVM_CAP_UNHANDLED_MSR_EXITS, a vm-wide
>> capability that guards the new exit reason and which can be enabled via
>> the KVM_ENABLE_CAP ioctl.
>>
>> Signed-off-by: Peter Hornyack <peterhornyack@google.com>
>> ---
>>  Documentation/virtual/kvm/api.txt | 48 +++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/kvm.h          | 14 ++++++++++++
>>  2 files changed, 62 insertions(+)
>
> Let's instead add:
>
> - KVM_CAP_MSR_EXITS
>
> - KVM_CAP_ENABLE_MSR_EXIT
>
> - KVM_CAP_DISABLE_MSR_EXIT
>
> and 3 kinds of exits: KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
> KVM_EXIT_MSR_AFTER_WRITE.  The first two use four fields (type, msr,
> data, handled) and #GP if handled=0 is zero on the next entry.  The last
> one uses the first three fields and can be used for HyperV.
>
> Paolo

Ok. I'm working on these adjustments now, will send the updated
patchset on Monday.

Peter

Re: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Peter Hornyack]

On Fri, Dec 18, 2015 at 1:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 18/08/2015 20:46, Peter Hornyack wrote:
>> Define KVM_EXIT_MSR, a new exit reason for accesses to MSRs that kvm
>> does not handle. Define KVM_CAP_UNHANDLED_MSR_EXITS, a vm-wide
>> capability that guards the new exit reason and which can be enabled via
>> the KVM_ENABLE_CAP ioctl.
>>
>> Signed-off-by: Peter Hornyack <peterhornyack@google.com>
>> ---
>>  Documentation/virtual/kvm/api.txt | 48 +++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/kvm.h          | 14 ++++++++++++
>>  2 files changed, 62 insertions(+)
>
> Let's instead add:
>
> - KVM_CAP_MSR_EXITS
>
> - KVM_CAP_ENABLE_MSR_EXIT
>
> - KVM_CAP_DISABLE_MSR_EXIT
>
> and 3 kinds of exits: KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
> KVM_EXIT_MSR_AFTER_WRITE.  The first two use four fields (type, msr,
> data, handled) and #GP if handled=0 is zero on the next entry.  The last
> one uses the first three fields and can be used for HyperV.
>
> Paolo

Paolo, I've included an updated version of this patch below. Does this
match the API that you had in mind? If so, I'll continue adjusting the
rest of the code and will send the entire new patchset.

This updated version of the API seems more cumbersome to me (three new
capabilities, three exit reasons when just one or two might suffice)
than the original interface I used, but maybe you have a good reason
for that. Also, will it be ok to have just one capability to enable
all three kinds of exits, or will KVM_EXIT_MSR_AFTER_WRITE want a
separate capability?


commit a684d520ed62cf0db4495e5197d5bf722e4f8109
Author: Peter Hornyack <peterhornyack@google.com>
Date:   Fri Dec 18 14:44:04 2015 -0800

    KVM: add capabilities and exit reasons for MSRs.

    Define KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE, and
    KVM_EXIT_MSR_AFTER_WRITE, new exit reasons for accesses to MSRs that kvm
    does not handle or that user space needs to be notified about. Define the
    KVM_CAP_MSR_EXITS, KVM_CAP_ENABLE_MSR_EXITS, and KVM_CAP_DISABLE_MSR_EXITS
    capabilities to control these new exits for a VM.

diff --git a/Documentation/virtual/kvm/api.txt
b/Documentation/virtual/kvm/api.txt
index 053f613fc9a9..3bba3248df3d 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -3359,6 +3359,43 @@ Hyper-V SynIC state change. Notification is
used to remap SynIC
 event/message pages and to enable/disable SynIC messages/events processing
 in userspace.

+ /*
+ * KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
+ * KVM_EXIT_MSR_AFTER_WRITE
+ */
+ struct {
+ __u32 index;    /* i.e. ecx; out */
+ __u64 data;     /* out (wrmsr) / in (rdmsr) */
+#define KVM_EXIT_MSR_COMPLETION_FAILED 1
+ __u64 type;     /* out */
+#define KVM_EXIT_MSR_UNHANDLED 0
+#define KVM_EXIT_MSR_HANDLED   1
+ __u8 handled;   /* in */
+ } msr;
+
+If exit_reason is KVM_EXIT_MSR_READ or KVM_EXIT_MSR_WRITE, then the vcpu has
+executed a rdmsr or wrmsr instruction which could not be satisfied by kvm. The
+msr struct is used for both output to and input from user space. index is the
+target MSR number held in ecx; user space must not modify this field. data
+holds the payload from a wrmsr or must be filled in with a payload on a rdmsr.
+For a normal exit, type will be 0.
+
+On the return path into kvm, user space should set handled to
+KVM_EXIT_MSR_HANDLED if it successfully handled the MSR access. Otherwise,
+handled should be set to KVM_EXIT_MSR_UNHANDLED, which will cause a general
+protection fault to be injected into the vcpu. If an error occurs during the
+return into kvm, the vcpu will not be run and another exit will be generated
+with type set to KVM_EXIT_MSR_COMPLETION_FAILED.
+
+If exit_reason is KVM_EXIT_MSR_AFTER_WRITE, then the vcpu has executed a wrmsr
+instruction which is handled by kvm but which user space may need to be
+notified about. index and data are set as described above; the value of type
+depends on the MSR that was written. handled is ignored on reentry into kvm.
+
+KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE, and KVM_EXIT_MSR_AFTER_WRITE can only
+occur when KVM_CAP_MSR_EXITS is present and KVM_CAP_ENABLE_MSR_EXITS has been
+set. A detailed description of these capabilities is below.
+
  /* Fix the size of the union. */
  char padding[256];
  };
@@ -3697,6 +3734,26 @@ a KVM_EXIT_IOAPIC_EOI vmexit will be reported
to userspace.
 Fails if VCPU has already been created, or if the irqchip is already in the
 kernel (i.e. KVM_CREATE_IRQCHIP has already been called).

+7.6 KVM_CAP_ENABLE_MSR_EXITS, KVM_CAP_DISABLE_MSR_EXITS
+
+Architectures: x86 (vmx-only)
+Parameters: none
+Returns: 0 on success, -1 on error
+
+These capabilities enable and disable exits to user space for certain guest MSR
+accesses. These capabilities are only available if KVM_CHECK_EXTENSION
+indicates that KVM_CAP_MSR_EXITS is present.
+
+When enabled, kvm will exit to user space when the guest reads
+an MSR that kvm does not handle (KVM_EXIT_MSR_READ), writes an MSR that kvm
+does not handle (KVM_EXIT_MSR_WRITE), or writes an MSR that kvm handles but for
+which user space should be notified (KVM_EXIT_MSR_AFTER_WRITE).
+
+These exits are currently only implemented for vmx. Also, note that if the kvm
+module's ignore_msrs flag is set then KVM_EXIT_MSR_READ and KVM_EXIT_MSR_WRITE
+will not be generated, and unhandled MSR accesses will simply be ignored and
+the guest re-entered immediately.
+

 8. Other capabilities.
 ----------------------
@@ -3726,3 +3783,11 @@ In order to use SynIC, it has to be activated
by setting this
 capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
 will disable the use of APIC hardware virtualization even if supported
 by the CPU, as it's incompatible with SynIC auto-EOI behavior.
+
+8.3 KVM_CAP_MSR_EXITS
+
+Architectures: x86 (vmx-only)
+
+This capability, if KVM_CHECK_EXTENSION indicates that it is available, means
+that the kernel implements the KVM_CAP_ENABLE_MSR_EXITS and
+KVM_CAP_DISABLE_MSR_EXITS capabilities for VMs.
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 6e32f7599081..431fd1ec0d06 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -199,6 +199,9 @@ struct kvm_hyperv_exit {
 #define KVM_EXIT_S390_STSI        25
 #define KVM_EXIT_IOAPIC_EOI       26
 #define KVM_EXIT_HYPERV           27
+#define KVM_EXIT_MSR_READ         28
+#define KVM_EXIT_MSR_WRITE        29
+#define KVM_EXIT_MSR_AFTER_WRITE  30

 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */
@@ -355,6 +358,18 @@ struct kvm_run {
  } eoi;
  /* KVM_EXIT_HYPERV */
  struct kvm_hyperv_exit hyperv;
+ /*
+ * KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
+ * KVM_EXIT_MSR_AFTER_WRITE
+ */
+ struct {
+ __u32 index;    /* i.e. ecx; out */
+ __u64 data;     /* out (wrmsr) / in (rdmsr) */
+ __u64 type;     /* out */
+#define KVM_EXIT_MSR_UNHANDLED 0
+#define KVM_EXIT_MSR_HANDLED   1
+ __u8 handled;   /* in */
+ } msr;
  /* Fix the size of the union. */
  char padding[256];
  };
@@ -849,6 +864,9 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_SPLIT_IRQCHIP 121
 #define KVM_CAP_IOEVENTFD_ANY_LENGTH 122
 #define KVM_CAP_HYPERV_SYNIC 123
+#define KVM_CAP_MSR_EXITS 124
+#define KVM_CAP_DISABLE_MSR_EXITS 125
+#define KVM_CAP_ENABLE_MSR_EXITS 126

 #ifdef KVM_CAP_IRQ_ROUTING

RE: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Pavel Fedin]

 Hello!

> commit a684d520ed62cf0db4495e5197d5bf722e4f8109
> Author: Peter Hornyack <peterhornyack@google.com>
> Date:   Fri Dec 18 14:44:04 2015 -0800
> 
>     KVM: add capabilities and exit reasons for MSRs.
> 
>     Define KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE, and
>     KVM_EXIT_MSR_AFTER_WRITE, new exit reasons for accesses to MSRs that kvm
>     does not handle or that user space needs to be notified about. Define the
>     KVM_CAP_MSR_EXITS, KVM_CAP_ENABLE_MSR_EXITS, and KVM_CAP_DISABLE_MSR_EXITS
>     capabilities to control these new exits for a VM.
> 
> diff --git a/Documentation/virtual/kvm/api.txt
> b/Documentation/virtual/kvm/api.txt
> index 053f613fc9a9..3bba3248df3d 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3359,6 +3359,43 @@ Hyper-V SynIC state change. Notification is
> used to remap SynIC
>  event/message pages and to enable/disable SynIC messages/events processing
>  in userspace.
> 
> + /*
> + * KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
> + * KVM_EXIT_MSR_AFTER_WRITE
> + */
> + struct {
> + __u32 index;    /* i.e. ecx; out */
> + __u64 data;     /* out (wrmsr) / in (rdmsr) */
> +#define KVM_EXIT_MSR_COMPLETION_FAILED 1
> + __u64 type;     /* out */
> +#define KVM_EXIT_MSR_UNHANDLED 0
> +#define KVM_EXIT_MSR_HANDLED   1
> + __u8 handled;   /* in */
> + } msr;
> +
> +If exit_reason is KVM_EXIT_MSR_READ or KVM_EXIT_MSR_WRITE, then the vcpu has
> +executed a rdmsr or wrmsr instruction which could not be satisfied by kvm. The
> +msr struct is used for both output to and input from user space. index is the
> +target MSR number held in ecx; user space must not modify this field. data

 In 'index', you meant?
 I would enlarge it to __u64 and use generalized encoding, the same as for KVM_SET_ONE_REG ioctl. I already wrote about it.
 And i would use simply "REG" instead of "MSR" denotion. Because on different architectures they can have different names (e. g. on ARM32 they are called "coprocessor registers" and on ARM64 these are "system registers"), however the common thing between them is that it is some special CPU register, access to which can be trapped and emulated. Therefore KVM_EXIT_REG_xxx.

> +holds the payload from a wrmsr or must be filled in with a payload on a rdmsr.
> +For a normal exit, type will be 0.
> +
> +On the return path into kvm, user space should set handled to
> +KVM_EXIT_MSR_HANDLED if it successfully handled the MSR access. Otherwise,
> +handled should be set to KVM_EXIT_MSR_UNHANDLED, which will cause a general
> +protection fault to be injected into the vcpu. If an error occurs during the
> +return into kvm, the vcpu will not be run and another exit will be generated
> +with type set to KVM_EXIT_MSR_COMPLETION_FAILED.
> +
> +If exit_reason is KVM_EXIT_MSR_AFTER_WRITE, then the vcpu has executed a wrmsr
> +instruction which is handled by kvm but which user space may need to be
> +notified about. index and data are set as described above; the value of type
> +depends on the MSR that was written. handled is ignored on reentry into kvm.

1. Is there any real need to distinguish between KVM_EXIT_MSR_WRITE and KVM_EXIT_MSR_AFTER_WRITE ? IMHO from userland's point of view these are the same.
2. Why do WRITE and READ have to be different exit codes? We could use something like "u8 is_write" in our structure, this would be more in line with PIO and MMIO handling.

> +
> +KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE, and KVM_EXIT_MSR_AFTER_WRITE can only
> +occur when KVM_CAP_MSR_EXITS is present and KVM_CAP_ENABLE_MSR_EXITS has been
> +set. A detailed description of these capabilities is below.
> +
>   /* Fix the size of the union. */
>   char padding[256];
>   };
> @@ -3697,6 +3734,26 @@ a KVM_EXIT_IOAPIC_EOI vmexit will be reported
> to userspace.
>  Fails if VCPU has already been created, or if the irqchip is already in the
>  kernel (i.e. KVM_CREATE_IRQCHIP has already been called).
> 
> +7.6 KVM_CAP_ENABLE_MSR_EXITS, KVM_CAP_DISABLE_MSR_EXITS
> +
> +Architectures: x86 (vmx-only)
> +Parameters: none
> +Returns: 0 on success, -1 on error
> +
> +These capabilities enable and disable exits to user space for certain guest MSR
> +accesses. These capabilities are only available if KVM_CHECK_EXTENSION
> +indicates that KVM_CAP_MSR_EXITS is present.
> +
> +When enabled, kvm will exit to user space when the guest reads
> +an MSR that kvm does not handle (KVM_EXIT_MSR_READ), writes an MSR that kvm
> +does not handle (KVM_EXIT_MSR_WRITE), or writes an MSR that kvm handles but for
> +which user space should be notified (KVM_EXIT_MSR_AFTER_WRITE).
> +
> +These exits are currently only implemented for vmx. Also, note that if the kvm
> +module's ignore_msrs flag is set then KVM_EXIT_MSR_READ and KVM_EXIT_MSR_WRITE
> +will not be generated, and unhandled MSR accesses will simply be ignored and
> +the guest re-entered immediately.
> +
> 
>  8. Other capabilities.
>  ----------------------
> @@ -3726,3 +3783,11 @@ In order to use SynIC, it has to be activated
> by setting this
>  capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
>  will disable the use of APIC hardware virtualization even if supported
>  by the CPU, as it's incompatible with SynIC auto-EOI behavior.
> +
> +8.3 KVM_CAP_MSR_EXITS
> +
> +Architectures: x86 (vmx-only)
> +
> +This capability, if KVM_CHECK_EXTENSION indicates that it is available, means
> +that the kernel implements the KVM_CAP_ENABLE_MSR_EXITS and
> +KVM_CAP_DISABLE_MSR_EXITS capabilities for VMs.
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6e32f7599081..431fd1ec0d06 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -199,6 +199,9 @@ struct kvm_hyperv_exit {
>  #define KVM_EXIT_S390_STSI        25
>  #define KVM_EXIT_IOAPIC_EOI       26
>  #define KVM_EXIT_HYPERV           27
> +#define KVM_EXIT_MSR_READ         28
> +#define KVM_EXIT_MSR_WRITE        29
> +#define KVM_EXIT_MSR_AFTER_WRITE  30
> 
>  /* For KVM_EXIT_INTERNAL_ERROR */
>  /* Emulate instruction failed. */
> @@ -355,6 +358,18 @@ struct kvm_run {
>   } eoi;
>   /* KVM_EXIT_HYPERV */
>   struct kvm_hyperv_exit hyperv;
> + /*
> + * KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
> + * KVM_EXIT_MSR_AFTER_WRITE
> + */
> + struct {
> + __u32 index;    /* i.e. ecx; out */
> + __u64 data;     /* out (wrmsr) / in (rdmsr) */
> + __u64 type;     /* out */
> +#define KVM_EXIT_MSR_UNHANDLED 0
> +#define KVM_EXIT_MSR_HANDLED   1
> + __u8 handled;   /* in */
> + } msr;
>   /* Fix the size of the union. */
>   char padding[256];
>   };
> @@ -849,6 +864,9 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_SPLIT_IRQCHIP 121
>  #define KVM_CAP_IOEVENTFD_ANY_LENGTH 122
>  #define KVM_CAP_HYPERV_SYNIC 123
> +#define KVM_CAP_MSR_EXITS 124
> +#define KVM_CAP_DISABLE_MSR_EXITS 125
> +#define KVM_CAP_ENABLE_MSR_EXITS 126
> 
>  #ifdef KVM_CAP_IRQ_ROUTING

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

['Roman Kagan']

On Tue, Dec 22, 2015 at 10:24:13AM +0300, Pavel Fedin wrote:
> > +On the return path into kvm, user space should set handled to
> > +KVM_EXIT_MSR_HANDLED if it successfully handled the MSR access. Otherwise,
> > +handled should be set to KVM_EXIT_MSR_UNHANDLED, which will cause a general
> > +protection fault to be injected into the vcpu. If an error occurs during the
> > +return into kvm, the vcpu will not be run and another exit will be generated
> > +with type set to KVM_EXIT_MSR_COMPLETION_FAILED.
> > +
> > +If exit_reason is KVM_EXIT_MSR_AFTER_WRITE, then the vcpu has executed a wrmsr
> > +instruction which is handled by kvm but which user space may need to be
> > +notified about. index and data are set as described above; the value of type
> > +depends on the MSR that was written. handled is ignored on reentry into kvm.
> 
> 1. Is there any real need to distinguish between KVM_EXIT_MSR_WRITE and KVM_EXIT_MSR_AFTER_WRITE ? IMHO from userland's point of view these are the same.

Indeed.  Perhaps the kernel can set .handled to true to let userspace
know it already took care of it, instead of introducing yet another
exit_reason.  The field would need to be marked in/out, then.

Roman.

RE: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Pavel Fedin]

 Hello!

> > 1. Is there any real need to distinguish between KVM_EXIT_MSR_WRITE and
> KVM_EXIT_MSR_AFTER_WRITE ? IMHO from userland's point of view these are the same.
> 
> Indeed.  Perhaps the kernel can set .handled to true to let userspace
> know it already took care of it, instead of introducing yet another
> exit_reason.  The field would need to be marked in/out, then.

 I'm not sure that you need even this. Anyway, particular MSRs are function-specific, and if you're emulating an MSR in userspace,
then, i believe, you know the function behind it. And it's IMHO safe to just know that SynIC MSRs have some extra handling in
kernel. And i believe this has no direct impact on userland's behavior.
 But, you better know the details.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

['Roman Kagan']

On Tue, Dec 22, 2015 at 03:51:52PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > > 1. Is there any real need to distinguish between KVM_EXIT_MSR_WRITE and
> > KVM_EXIT_MSR_AFTER_WRITE ? IMHO from userland's point of view these are the same.
> > 
> > Indeed.  Perhaps the kernel can set .handled to true to let userspace
> > know it already took care of it, instead of introducing yet another
> > exit_reason.  The field would need to be marked in/out, then.
> 
>  I'm not sure that you need even this. Anyway, particular MSRs are function-specific, and if you're emulating an MSR in userspace,
> then, i believe, you know the function behind it. And it's IMHO safe to just know that SynIC MSRs have some extra handling in
> kernel. And i believe this has no direct impact on userland's behavior.

It has: unlike the scenario that was the original motivation for Peter's
patches, where the the userspace wanted to handle register accesses
which the kernel *didn't*, in case of SynIC the userspace wants do
something about MSR accesses *only* if the kernel *also* handles them.

I guess that was the reason why Paolo suggested an extra exit_reason,
and I think .handled field can be used to pass that information instead.

You're probably right that, at least in SynIC case, it should be safe to
assume that, if all the SynIC setup succeeded, the corresponding MSR
accesses would only trigger exits when the kernel processed them
appropriately.

But the proposed use of .handled costs basically nothing, and it may
prove useful in general (as a conisistency proof, if anything).

Roman.

RE: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Pavel Fedin]

 Hello!

> It has: unlike the scenario that was the original motivation for Peter's
> patches, where the the userspace wanted to handle register accesses
> which the kernel *didn't*, in case of SynIC the userspace wants do
> something about MSR accesses *only* if the kernel *also* handles them.

 Well... I believe, that qemu knows if we are instantiating SynIC. And, if we are, it knows that the kernel will do something about
it. Otherwise these registers don't exist, and, by the way, the guest is not expected to touch them, is it?

> I guess that was the reason why Paolo suggested an extra exit_reason,
> and I think .handled field can be used to pass that information instead.

[skip]

> But the proposed use of .handled costs basically nothing, and it may
> prove useful in general (as a conisistency proof, if anything).

 Well... May be... So, i'm OK with it.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [RFC PATCH 2/5] KVM: add KVM_EXIT_MSR exit reason and capability.

[Peter Hornyack]

On Mon, Dec 21, 2015 at 11:24 PM, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>> commit a684d520ed62cf0db4495e5197d5bf722e4f8109
>> Author: Peter Hornyack <peterhornyack@google.com>
>> Date:   Fri Dec 18 14:44:04 2015 -0800
>>
>>     KVM: add capabilities and exit reasons for MSRs.
>>
>>     Define KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE, and
>>     KVM_EXIT_MSR_AFTER_WRITE, new exit reasons for accesses to MSRs that kvm
>>     does not handle or that user space needs to be notified about. Define the
>>     KVM_CAP_MSR_EXITS, KVM_CAP_ENABLE_MSR_EXITS, and KVM_CAP_DISABLE_MSR_EXITS
>>     capabilities to control these new exits for a VM.
>>
>> diff --git a/Documentation/virtual/kvm/api.txt
>> b/Documentation/virtual/kvm/api.txt
>> index 053f613fc9a9..3bba3248df3d 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -3359,6 +3359,43 @@ Hyper-V SynIC state change. Notification is
>> used to remap SynIC
>>  event/message pages and to enable/disable SynIC messages/events processing
>>  in userspace.
>>
>> + /*
>> + * KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
>> + * KVM_EXIT_MSR_AFTER_WRITE
>> + */
>> + struct {
>> + __u32 index;    /* i.e. ecx; out */
>> + __u64 data;     /* out (wrmsr) / in (rdmsr) */
>> +#define KVM_EXIT_MSR_COMPLETION_FAILED 1
>> + __u64 type;     /* out */
>> +#define KVM_EXIT_MSR_UNHANDLED 0
>> +#define KVM_EXIT_MSR_HANDLED   1
>> + __u8 handled;   /* in */
>> + } msr;
>> +
>> +If exit_reason is KVM_EXIT_MSR_READ or KVM_EXIT_MSR_WRITE, then the vcpu has
>> +executed a rdmsr or wrmsr instruction which could not be satisfied by kvm. The
>> +msr struct is used for both output to and input from user space. index is the
>> +target MSR number held in ecx; user space must not modify this field. data
>
>  In 'index', you meant?

I'll just remove "held in ecx."

>  I would enlarge it to __u64 and use generalized encoding, the same as for KVM_SET_ONE_REG ioctl. I already wrote about it.

Ok, I would be open to using this sort of encoding.

>  And i would use simply "REG" instead of "MSR" denotion. Because on different architectures they can have different names (e. g. on ARM32 they are called "coprocessor registers" and on ARM64 these are "system registers"), however the common thing between them is that it is some special CPU register, access to which can be trapped and emulated. Therefore KVM_EXIT_REG_xxx.

Sure.

>> +holds the payload from a wrmsr or must be filled in with a payload on a rdmsr.
>> +For a normal exit, type will be 0.
>> +
>> +On the return path into kvm, user space should set handled to
>> +KVM_EXIT_MSR_HANDLED if it successfully handled the MSR access. Otherwise,
>> +handled should be set to KVM_EXIT_MSR_UNHANDLED, which will cause a general
>> +protection fault to be injected into the vcpu. If an error occurs during the
>> +return into kvm, the vcpu will not be run and another exit will be generated
>> +with type set to KVM_EXIT_MSR_COMPLETION_FAILED.
>> +
>> +If exit_reason is KVM_EXIT_MSR_AFTER_WRITE, then the vcpu has executed a wrmsr
>> +instruction which is handled by kvm but which user space may need to be
>> +notified about. index and data are set as described above; the value of type
>> +depends on the MSR that was written. handled is ignored on reentry into kvm.
>
> 1. Is there any real need to distinguish between KVM_EXIT_MSR_WRITE and KVM_EXIT_MSR_AFTER_WRITE ? IMHO from userland's point of view these are the same.

I think I agree with you Pavel, but we could leave the .handled field
as Roman suggests in case user space really does need to distinguish
between these two.

> 2. Why do WRITE and READ have to be different exit codes? We could use something like "u8 is_write" in our structure, this would be more in line with PIO and MMIO handling.

I agree - this is what I had in my original patchset. I updated the
proposed API based on Paolo's suggestions but I'd prefer to go back to
just a single exit reason.

I will send a v2 patchset for KVM_EXIT_REG_IO with the generalized
register encoding.

>> +
>> +KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE, and KVM_EXIT_MSR_AFTER_WRITE can only
>> +occur when KVM_CAP_MSR_EXITS is present and KVM_CAP_ENABLE_MSR_EXITS has been
>> +set. A detailed description of these capabilities is below.
>> +
>>   /* Fix the size of the union. */
>>   char padding[256];
>>   };
>> @@ -3697,6 +3734,26 @@ a KVM_EXIT_IOAPIC_EOI vmexit will be reported
>> to userspace.
>>  Fails if VCPU has already been created, or if the irqchip is already in the
>>  kernel (i.e. KVM_CREATE_IRQCHIP has already been called).
>>
>> +7.6 KVM_CAP_ENABLE_MSR_EXITS, KVM_CAP_DISABLE_MSR_EXITS
>> +
>> +Architectures: x86 (vmx-only)
>> +Parameters: none
>> +Returns: 0 on success, -1 on error
>> +
>> +These capabilities enable and disable exits to user space for certain guest MSR
>> +accesses. These capabilities are only available if KVM_CHECK_EXTENSION
>> +indicates that KVM_CAP_MSR_EXITS is present.
>> +
>> +When enabled, kvm will exit to user space when the guest reads
>> +an MSR that kvm does not handle (KVM_EXIT_MSR_READ), writes an MSR that kvm
>> +does not handle (KVM_EXIT_MSR_WRITE), or writes an MSR that kvm handles but for
>> +which user space should be notified (KVM_EXIT_MSR_AFTER_WRITE).
>> +
>> +These exits are currently only implemented for vmx. Also, note that if the kvm
>> +module's ignore_msrs flag is set then KVM_EXIT_MSR_READ and KVM_EXIT_MSR_WRITE
>> +will not be generated, and unhandled MSR accesses will simply be ignored and
>> +the guest re-entered immediately.
>> +
>>
>>  8. Other capabilities.
>>  ----------------------
>> @@ -3726,3 +3783,11 @@ In order to use SynIC, it has to be activated
>> by setting this
>>  capability via KVM_ENABLE_CAP ioctl on the vcpu fd. Note that this
>>  will disable the use of APIC hardware virtualization even if supported
>>  by the CPU, as it's incompatible with SynIC auto-EOI behavior.
>> +
>> +8.3 KVM_CAP_MSR_EXITS
>> +
>> +Architectures: x86 (vmx-only)
>> +
>> +This capability, if KVM_CHECK_EXTENSION indicates that it is available, means
>> +that the kernel implements the KVM_CAP_ENABLE_MSR_EXITS and
>> +KVM_CAP_DISABLE_MSR_EXITS capabilities for VMs.
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 6e32f7599081..431fd1ec0d06 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -199,6 +199,9 @@ struct kvm_hyperv_exit {
>>  #define KVM_EXIT_S390_STSI        25
>>  #define KVM_EXIT_IOAPIC_EOI       26
>>  #define KVM_EXIT_HYPERV           27
>> +#define KVM_EXIT_MSR_READ         28
>> +#define KVM_EXIT_MSR_WRITE        29
>> +#define KVM_EXIT_MSR_AFTER_WRITE  30
>>
>>  /* For KVM_EXIT_INTERNAL_ERROR */
>>  /* Emulate instruction failed. */
>> @@ -355,6 +358,18 @@ struct kvm_run {
>>   } eoi;
>>   /* KVM_EXIT_HYPERV */
>>   struct kvm_hyperv_exit hyperv;
>> + /*
>> + * KVM_EXIT_MSR_READ, KVM_EXIT_MSR_WRITE,
>> + * KVM_EXIT_MSR_AFTER_WRITE
>> + */
>> + struct {
>> + __u32 index;    /* i.e. ecx; out */
>> + __u64 data;     /* out (wrmsr) / in (rdmsr) */
>> + __u64 type;     /* out */
>> +#define KVM_EXIT_MSR_UNHANDLED 0
>> +#define KVM_EXIT_MSR_HANDLED   1
>> + __u8 handled;   /* in */
>> + } msr;
>>   /* Fix the size of the union. */
>>   char padding[256];
>>   };
>> @@ -849,6 +864,9 @@ struct kvm_ppc_smmu_info {
>>  #define KVM_CAP_SPLIT_IRQCHIP 121
>>  #define KVM_CAP_IOEVENTFD_ANY_LENGTH 122
>>  #define KVM_CAP_HYPERV_SYNIC 123
>> +#define KVM_CAP_MSR_EXITS 124
>> +#define KVM_CAP_DISABLE_MSR_EXITS 125
>> +#define KVM_CAP_ENABLE_MSR_EXITS 126
>>
>>  #ifdef KVM_CAP_IRQ_ROUTING
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>

Peter