All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator
@ 2020-02-18 23:29 Sean Christopherson
  2020-02-18 23:29 ` [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant Sean Christopherson
                   ` (13 more replies)
  0 siblings, 14 replies; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

The primary intent of this series is to dynamically allocate the emulator
and get KVM to a state where the emulator *could* be disabled at some
point in the future.  Actually allowing userspace to disable the emulator
was a minor change at that point, so I threw it in.

Dynamically allocating the emulator shrinks the size of x86 vcpus by
~2.5k bytes, which is important because 'struct vcpu_vmx' has once again
fattened up and squeaked past the PAGE_ALLOC_COSTLY_ORDER threshold.
Moving the emulator to its own allocation gives us some breathing room
for the near future, and has some other nice side effects.

As for disabling the emulator... in the not-too-distant future, I expect
there will be use cases that can truly disable KVM's emulator, e.g. for
security (KVM's and/or the guests).  I don't have a strong opinion on
whether or not KVM should actually allow userspace to disable the emulator
without a concrete use case (unless there already is a use case?), which
is why that part is done in its own tiny patch.

Running without an emulator has been "tested" in the sense that the
selftests that don't require emulation continue to pass, and everything
else fails with the expected "emulation error".

v2:
  - Rebase to kvm/queue, 2c2787938512 ("KVM: selftests: Stop ...")

Sean Christopherson (13):
  KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant
  KVM: x86: Explicitly pass an exception struct to check_intercept
  KVM: x86: Move emulation-only helpers to emulate.c
  KVM: x86: Refactor R/W page helper to take the emulation context
  KVM: x86: Refactor emulated exception injection to take the emul
    context
  KVM: x86: Refactor emulate tracepoint to explicitly take context
  KVM: x86: Refactor init_emulate_ctxt() to explicitly take context
  KVM: x86: Dynamically allocate per-vCPU emulation context
  KVM: x86: Move kvm_emulate.h into KVM's private directory
  KVM: x86: Shrink the usercopy region of the emulation context
  KVM: x86: Add helper to "handle" internal emulation error
  KVM: x86: Add variable to control existence of emulator
  KVM: x86: Allow userspace to disable the kernel's emulator

 arch/x86/include/asm/kvm_host.h             |  12 +-
 arch/x86/kvm/emulate.c                      |  13 +-
 arch/x86/{include/asm => kvm}/kvm_emulate.h |   9 +-
 arch/x86/kvm/mmu/mmu.c                      |   1 +
 arch/x86/kvm/svm.c                          |   5 +-
 arch/x86/kvm/trace.h                        |  22 +--
 arch/x86/kvm/vmx/vmx.c                      |  15 +-
 arch/x86/kvm/x86.c                          | 193 +++++++++++++-------
 arch/x86/kvm/x86.h                          |  12 +-
 9 files changed, 183 insertions(+), 99 deletions(-)
 rename arch/x86/{include/asm => kvm}/kvm_emulate.h (99%)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 15:16   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 02/13] KVM: x86: Explicitly pass an exception struct to check_intercept Sean Christopherson
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Add variants of the I/O helpers that take a vCPU instead of an emulation
context.  This will eventually allow KVM to limit use of the emulation
context to the full emulation path.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fbabb2f06273..6554abef631f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5959,11 +5959,9 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
 	return 0;
 }
 
-static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
-				    int size, unsigned short port, void *val,
-				    unsigned int count)
+static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
+			   unsigned short port, void *val, unsigned int count)
 {
-	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
 	int ret;
 
 	if (vcpu->arch.pio.count)
@@ -5983,17 +5981,30 @@ static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
 	return 0;
 }
 
-static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
-				     int size, unsigned short port,
-				     const void *val, unsigned int count)
+static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
+				    int size, unsigned short port, void *val,
+				    unsigned int count)
 {
-	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return emulator_pio_in(emul_to_vcpu(ctxt), size, port, val, count);
 
+}
+
+static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
+			    unsigned short port, const void *val,
+			    unsigned int count)
+{
 	memcpy(vcpu->arch.pio_data, val, size * count);
 	trace_kvm_pio(KVM_PIO_OUT, port, size, count, vcpu->arch.pio_data);
 	return emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
 }
 
+static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
+				     int size, unsigned short port,
+				     const void *val, unsigned int count)
+{
+	return emulator_pio_out(emul_to_vcpu(ctxt), size, port, val, count);
+}
+
 static unsigned long get_segment_base(struct kvm_vcpu *vcpu, int seg)
 {
 	return kvm_x86_ops->get_segment_base(vcpu, seg);
@@ -6930,8 +6941,8 @@ static int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size,
 			    unsigned short port)
 {
 	unsigned long val = kvm_rax_read(vcpu);
-	int ret = emulator_pio_out_emulated(&vcpu->arch.emulate_ctxt,
-					    size, port, &val, 1);
+	int ret = emulator_pio_out(vcpu, size, port, &val, 1);
+
 	if (ret)
 		return ret;
 
@@ -6967,11 +6978,10 @@ static int complete_fast_pio_in(struct kvm_vcpu *vcpu)
 	val = (vcpu->arch.pio.size < 4) ? kvm_rax_read(vcpu) : 0;
 
 	/*
-	 * Since vcpu->arch.pio.count == 1 let emulator_pio_in_emulated perform
+	 * Since vcpu->arch.pio.count == 1 let emulator_pio_in perform
 	 * the copy and tracing
 	 */
-	emulator_pio_in_emulated(&vcpu->arch.emulate_ctxt, vcpu->arch.pio.size,
-				 vcpu->arch.pio.port, &val, 1);
+	emulator_pio_in(vcpu, vcpu->arch.pio.size, vcpu->arch.pio.port, &val, 1);
 	kvm_rax_write(vcpu, val);
 
 	return kvm_skip_emulated_instruction(vcpu);
@@ -6986,8 +6996,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
 	/* For size less than 4 we merge, else we zero extend */
 	val = (size < 4) ? kvm_rax_read(vcpu) : 0;
 
-	ret = emulator_pio_in_emulated(&vcpu->arch.emulate_ctxt, size, port,
-				       &val, 1);
+	ret = emulator_pio_in(vcpu, size, port, &val, 1);
 	if (ret) {
 		kvm_rax_write(vcpu, val);
 		return ret;
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 02/13] KVM: x86: Explicitly pass an exception struct to check_intercept
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
  2020-02-18 23:29 ` [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-18 23:29 ` [PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c Sean Christopherson
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Explicitly pass an exception struct when checking for intercept from
the emulator, which eliminates the last reference to arch.emulate_ctxt
in vendor specific code.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 3 ++-
 arch/x86/kvm/svm.c              | 3 ++-
 arch/x86/kvm/vmx/vmx.c          | 8 ++++----
 arch/x86/kvm/x86.c              | 3 ++-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4dffbc10d3f8..c750cd957558 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1160,7 +1160,8 @@ struct kvm_x86_ops {
 
 	int (*check_intercept)(struct kvm_vcpu *vcpu,
 			       struct x86_instruction_info *info,
-			       enum x86_intercept_stage stage);
+			       enum x86_intercept_stage stage,
+			       struct x86_exception *exception);
 	void (*handle_exit_irqoff)(struct kvm_vcpu *vcpu,
 		enum exit_fastpath_completion *exit_fastpath);
 	bool (*mpx_supported)(void);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a3e32d61d60c..ae62ea454158 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -6180,7 +6180,8 @@ static const struct __x86_intercept {
 
 static int svm_check_intercept(struct kvm_vcpu *vcpu,
 			       struct x86_instruction_info *info,
-			       enum x86_intercept_stage stage)
+			       enum x86_intercept_stage stage,
+			       struct x86_exception *exception)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	int vmexit, ret = X86EMUL_CONTINUE;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9a6664886f2e..09bb0d98afeb 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7100,10 +7100,10 @@ static void vmx_request_immediate_exit(struct kvm_vcpu *vcpu)
 
 static int vmx_check_intercept(struct kvm_vcpu *vcpu,
 			       struct x86_instruction_info *info,
-			       enum x86_intercept_stage stage)
+			       enum x86_intercept_stage stage,
+			       struct x86_exception *exception)
 {
 	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
-	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
 
 	/*
 	 * RDPID causes #UD if disabled through secondary execution controls.
@@ -7111,8 +7111,8 @@ static int vmx_check_intercept(struct kvm_vcpu *vcpu,
 	 */
 	if (info->intercept == x86_intercept_rdtscp &&
 	    !nested_cpu_has2(vmcs12, SECONDARY_EXEC_RDTSCP)) {
-		ctxt->exception.vector = UD_VECTOR;
-		ctxt->exception.error_code_valid = false;
+		exception->vector = UD_VECTOR;
+		exception->error_code_valid = false;
 		return X86EMUL_PROPAGATE_FAULT;
 	}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6554abef631f..409bf35f26fd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6267,7 +6267,8 @@ static int emulator_intercept(struct x86_emulate_ctxt *ctxt,
 			      struct x86_instruction_info *info,
 			      enum x86_intercept_stage stage)
 {
-	return kvm_x86_ops->check_intercept(emul_to_vcpu(ctxt), info, stage);
+	return kvm_x86_ops->check_intercept(emul_to_vcpu(ctxt), info, stage,
+					    &ctxt->exception);
 }
 
 static bool emulator_get_cpuid(struct x86_emulate_ctxt *ctxt,
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
  2020-02-18 23:29 ` [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant Sean Christopherson
  2020-02-18 23:29 ` [PATCH v2 02/13] KVM: x86: Explicitly pass an exception struct to check_intercept Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 15:23   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context Sean Christopherson
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Move ctxt_virt_addr_bits() and emul_is_noncanonical_address() from x86.h
to emulate.c.  This eliminates all references to struct x86_emulate_ctxt
from x86.h, and sets the stage for a future patch to stop including
kvm_emulate.h in asm/kvm_host.h.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/emulate.c | 11 +++++++++++
 arch/x86/kvm/x86.h     | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index ddbc61984227..1e394cb190ce 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -673,6 +673,17 @@ static void set_segment_selector(struct x86_emulate_ctxt *ctxt, u16 selector,
 	ctxt->ops->set_segment(ctxt, selector, &desc, base3, seg);
 }
 
+static inline u8 ctxt_virt_addr_bits(struct x86_emulate_ctxt *ctxt)
+{
+	return (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_LA57) ? 57 : 48;
+}
+
+static inline bool emul_is_noncanonical_address(u64 la,
+						struct x86_emulate_ctxt *ctxt)
+{
+	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
+}
+
 /*
  * x86 defines three classes of vector instructions: explicitly
  * aligned, explicitly unaligned, and the rest, which change behaviour
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 3624665acee4..8409842a25d9 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -149,11 +149,6 @@ static inline u8 vcpu_virt_addr_bits(struct kvm_vcpu *vcpu)
 	return kvm_read_cr4_bits(vcpu, X86_CR4_LA57) ? 57 : 48;
 }
 
-static inline u8 ctxt_virt_addr_bits(struct x86_emulate_ctxt *ctxt)
-{
-	return (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_LA57) ? 57 : 48;
-}
-
 static inline u64 get_canonical(u64 la, u8 vaddr_bits)
 {
 	return ((int64_t)la << (64 - vaddr_bits)) >> (64 - vaddr_bits);
@@ -164,12 +159,6 @@ static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu)
 	return get_canonical(la, vcpu_virt_addr_bits(vcpu)) != la;
 }
 
-static inline bool emul_is_noncanonical_address(u64 la,
-						struct x86_emulate_ctxt *ctxt)
-{
-	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
-}
-
 static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
 					gva_t gva, gfn_t gfn, unsigned access)
 {
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (2 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 15:24   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context Sean Christopherson
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Invert the vcpu->context derivation in emulator_read_write_onepage() in
preparation for dynamically allocating the emulation context.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 409bf35f26fd..772e704e8083 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5720,14 +5720,14 @@ static const struct read_write_emulator_ops write_emultor = {
 static int emulator_read_write_onepage(unsigned long addr, void *val,
 				       unsigned int bytes,
 				       struct x86_exception *exception,
-				       struct kvm_vcpu *vcpu,
+				       struct x86_emulate_ctxt *ctxt,
 				       const struct read_write_emulator_ops *ops)
 {
 	gpa_t gpa;
 	int handled, ret;
 	bool write = ops->write;
 	struct kvm_mmio_fragment *frag;
-	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
 
 	/*
 	 * If the exit was due to a NPF we may already have a GPA.
@@ -5791,7 +5791,7 @@ static int emulator_read_write(struct x86_emulate_ctxt *ctxt,
 
 		now = -addr & ~PAGE_MASK;
 		rc = emulator_read_write_onepage(addr, val, now, exception,
-						 vcpu, ops);
+						 ctxt, ops);
 
 		if (rc != X86EMUL_CONTINUE)
 			return rc;
@@ -5803,7 +5803,7 @@ static int emulator_read_write(struct x86_emulate_ctxt *ctxt,
 	}
 
 	rc = emulator_read_write_onepage(addr, val, bytes, exception,
-					 vcpu, ops);
+					 ctxt, ops);
 	if (rc != X86EMUL_CONTINUE)
 		return rc;
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (3 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 15:25   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context Sean Christopherson
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Invert the vcpu->context derivation in inject_emulated_exception() in
preparation for dynamically allocating the emulation context.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 772e704e8083..79d1113ad6e7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6399,9 +6399,10 @@ static void toggle_interruptibility(struct kvm_vcpu *vcpu, u32 mask)
 	}
 }
 
-static bool inject_emulated_exception(struct kvm_vcpu *vcpu)
+static bool inject_emulated_exception(struct x86_emulate_ctxt *ctxt)
 {
-	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+
 	if (ctxt->exception.vector == PF_VECTOR)
 		return kvm_propagate_fault(vcpu, &ctxt->exception);
 
@@ -6806,7 +6807,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 				 */
 				WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR ||
 					     exception_type(ctxt->exception.vector) == EXCPT_TRAP);
-				inject_emulated_exception(vcpu);
+				inject_emulated_exception(ctxt);
 				return 1;
 			}
 			return handle_emulation_failure(vcpu, emulation_type);
@@ -6860,7 +6861,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 
 	if (ctxt->have_exception) {
 		r = 1;
-		if (inject_emulated_exception(vcpu))
+		if (inject_emulated_exception(ctxt))
 			return r;
 	} else if (vcpu->arch.pio.count) {
 		if (!vcpu->arch.pio.in) {
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (4 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 17:11   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() " Sean Christopherson
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Explicitly pass the emulation context to the emulate tracepoint in
preparation of dynamically allocation the emulation context.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/trace.h | 22 +++++++++++-----------
 arch/x86/kvm/x86.c   | 13 ++++++++-----
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index f194dd058470..5605000ca5f6 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -731,8 +731,9 @@ TRACE_EVENT(kvm_skinit,
 	})
 
 TRACE_EVENT(kvm_emulate_insn,
-	TP_PROTO(struct kvm_vcpu *vcpu, __u8 failed),
-	TP_ARGS(vcpu, failed),
+	TP_PROTO(struct kvm_vcpu *vcpu, struct x86_emulate_ctxt *ctxt,
+		 __u8 failed),
+	TP_ARGS(vcpu, ctxt, failed),
 
 	TP_STRUCT__entry(
 		__field(    __u64, rip                       )
@@ -745,13 +746,10 @@ TRACE_EVENT(kvm_emulate_insn,
 
 	TP_fast_assign(
 		__entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS);
-		__entry->len = vcpu->arch.emulate_ctxt.fetch.ptr
-			       - vcpu->arch.emulate_ctxt.fetch.data;
-		__entry->rip = vcpu->arch.emulate_ctxt._eip - __entry->len;
-		memcpy(__entry->insn,
-		       vcpu->arch.emulate_ctxt.fetch.data,
-		       15);
-		__entry->flags = kei_decode_mode(vcpu->arch.emulate_ctxt.mode);
+		__entry->len = ctxt->fetch.ptr - ctxt->fetch.data;
+		__entry->rip = ctxt->_eip - __entry->len;
+		memcpy(__entry->insn, ctxt->fetch.data, 15);
+		__entry->flags = kei_decode_mode(ctxt->mode);
 		__entry->failed = failed;
 		),
 
@@ -764,8 +762,10 @@ TRACE_EVENT(kvm_emulate_insn,
 		)
 	);
 
-#define trace_kvm_emulate_insn_start(vcpu) trace_kvm_emulate_insn(vcpu, 0)
-#define trace_kvm_emulate_insn_failed(vcpu) trace_kvm_emulate_insn(vcpu, 1)
+#define trace_kvm_emulate_insn_start(vcpu, ctxt)	\
+	trace_kvm_emulate_insn(vcpu, ctxt, 0)
+#define trace_kvm_emulate_insn_failed(vcpu, ctxt)	\
+	trace_kvm_emulate_insn(vcpu, ctxt, 1)
 
 TRACE_EVENT(
 	vcpu_match_mmio,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 79d1113ad6e7..69d3dd64d90c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6460,10 +6460,13 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 }
 EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
 
-static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
+static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt,
+				    int emulation_type)
 {
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+
 	++vcpu->stat.insn_emulation_fail;
-	trace_kvm_emulate_insn_failed(vcpu);
+	trace_kvm_emulate_insn_failed(vcpu, ctxt);
 
 	if (emulation_type & EMULTYPE_VMWARE_GP) {
 		kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
@@ -6788,7 +6791,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 
 		r = x86_decode_insn(ctxt, insn, insn_len);
 
-		trace_kvm_emulate_insn_start(vcpu);
+		trace_kvm_emulate_insn_start(vcpu, ctxt);
 		++vcpu->stat.insn_emulation;
 		if (r != EMULATION_OK)  {
 			if ((emulation_type & EMULTYPE_TRAP_UD) ||
@@ -6810,7 +6813,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 				inject_emulated_exception(ctxt);
 				return 1;
 			}
-			return handle_emulation_failure(vcpu, emulation_type);
+			return handle_emulation_failure(ctxt, emulation_type);
 		}
 	}
 
@@ -6856,7 +6859,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 					emulation_type))
 			return 1;
 
-		return handle_emulation_failure(vcpu, emulation_type);
+		return handle_emulation_failure(ctxt, emulation_type);
 	}
 
 	if (ctxt->have_exception) {
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() to explicitly take context
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (5 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 17:13   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context Sean Christopherson
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Explicitly pass the emulation context when initializing said context in
preparation of dynamically allocation the emulation context.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 69d3dd64d90c..0e67f90db9a6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6414,9 +6414,9 @@ static bool inject_emulated_exception(struct x86_emulate_ctxt *ctxt)
 	return false;
 }
 
-static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
+static void init_emulate_ctxt(struct x86_emulate_ctxt *ctxt)
 {
-	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
 	int cs_db, cs_l;
 
 	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
@@ -6443,7 +6443,7 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
 	int ret;
 
-	init_emulate_ctxt(vcpu);
+	init_emulate_ctxt(ctxt);
 
 	ctxt->op_bytes = 2;
 	ctxt->ad_bytes = 2;
@@ -6770,7 +6770,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 	kvm_clear_exception_queue(vcpu);
 
 	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
-		init_emulate_ctxt(vcpu);
+		init_emulate_ctxt(ctxt);
 
 		/*
 		 * We will reenter on the same instruction since
@@ -8943,7 +8943,7 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
 	int ret;
 
-	init_emulate_ctxt(vcpu);
+	init_emulate_ctxt(ctxt);
 
 	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
 				   has_error_code, error_code);
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (6 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() " Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 17:29   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory Sean Christopherson
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Allocate the emulation context instead of embedding it in struct
kvm_vcpu_arch.

Dynamic allocation provides several benefits:

  - Shrinks the size x86 vcpus by ~2.5k bytes, dropping them back below
    the PAGE_ALLOC_COSTLY_ORDER threshold.
  - Allows for dropping the include of kvm_emulate.h from asm/kvm_host.h
    and moving kvm_emulate.h into KVM's private directory.
  - Allows a reducing KVM's attack surface by shrinking the amount of
    vCPU data that is exposed to usercopy.
  - Allows a future patch to disable the emulator entirely, which may or
    may not be a realistic endeavor.

Mark the entire struct as valid for usercopy to maintain existing
behavior with respect to hardened usercopy.  Future patches can shrink
the usercopy range to cover only what is necessary.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_emulate.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  2 +-
 arch/x86/kvm/x86.c                 | 61 ++++++++++++++++++++++++++----
 3 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 03946eb3e2b9..2f0a600efdff 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -293,6 +293,7 @@ enum x86emul_mode {
 #define X86EMUL_SMM_INSIDE_NMI_MASK  (1 << 7)
 
 struct x86_emulate_ctxt {
+	void *vcpu;
 	const struct x86_emulate_ops *ops;
 
 	/* Register state before/after emulation. */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c750cd957558..e069f71667b1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -678,7 +678,7 @@ struct kvm_vcpu_arch {
 
 	/* emulate context */
 
-	struct x86_emulate_ctxt emulate_ctxt;
+	struct x86_emulate_ctxt *emulate_ctxt;
 	bool emulate_regs_need_sync_to_vcpu;
 	bool emulate_regs_need_sync_from_vcpu;
 	int (*complete_userspace_io)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0e67f90db9a6..5ab7d4283185 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -81,7 +81,7 @@ u64 __read_mostly kvm_mce_cap_supported = MCG_CTL_P | MCG_SER_P;
 EXPORT_SYMBOL_GPL(kvm_mce_cap_supported);
 
 #define emul_to_vcpu(ctxt) \
-	container_of(ctxt, struct kvm_vcpu, arch.emulate_ctxt)
+	((struct kvm_vcpu *)(ctxt)->vcpu)
 
 /* EFER defaults:
  * - enable syscall per default because its emulated by KVM
@@ -230,6 +230,19 @@ u64 __read_mostly host_xcr0;
 struct kmem_cache *x86_fpu_cache;
 EXPORT_SYMBOL_GPL(x86_fpu_cache);
 
+static struct kmem_cache *x86_emulator_cache;
+
+static struct kmem_cache *kvm_alloc_emulator_cache(void)
+{
+	return kmem_cache_create_usercopy("x86_emulator",
+					  sizeof(struct x86_emulate_ctxt),
+					  __alignof__(struct x86_emulate_ctxt),
+					  SLAB_ACCOUNT,
+					  0,
+					  sizeof(struct x86_emulate_ctxt),
+					  NULL);
+}
+
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
 
 static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
@@ -6414,6 +6427,23 @@ static bool inject_emulated_exception(struct x86_emulate_ctxt *ctxt)
 	return false;
 }
 
+static struct x86_emulate_ctxt *alloc_emulate_ctxt(struct kvm_vcpu *vcpu)
+{
+	struct x86_emulate_ctxt *ctxt;
+
+	ctxt = kmem_cache_zalloc(x86_emulator_cache, GFP_KERNEL_ACCOUNT);
+	if (!ctxt) {
+		pr_err("kvm: failed to allocate vcpu's emulator\n");
+		return NULL;
+	}
+
+	ctxt->vcpu = vcpu;
+	ctxt->ops = &emulate_ops;
+	vcpu->arch.emulate_ctxt = ctxt;
+
+	return ctxt;
+}
+
 static void init_emulate_ctxt(struct x86_emulate_ctxt *ctxt)
 {
 	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
@@ -6440,7 +6470,7 @@ static void init_emulate_ctxt(struct x86_emulate_ctxt *ctxt)
 
 void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 {
-	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
 	int ret;
 
 	init_emulate_ctxt(ctxt);
@@ -6756,7 +6786,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 			    int emulation_type, void *insn, int insn_len)
 {
 	int r;
-	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
 	bool writeback = true;
 	bool write_fault_to_spt = vcpu->arch.write_fault_to_shadow_pgtable;
 
@@ -7339,10 +7369,16 @@ int kvm_arch_init(void *opaque)
 		goto out;
 	}
 
+	x86_emulator_cache = kvm_alloc_emulator_cache();
+	if (!x86_emulator_cache) {
+		pr_err("kvm: failed to allocate cache for x86 emulator\n");
+		goto out_free_x86_fpu_cache;
+	}
+
 	shared_msrs = alloc_percpu(struct kvm_shared_msrs);
 	if (!shared_msrs) {
 		printk(KERN_ERR "kvm: failed to allocate percpu kvm_shared_msrs\n");
-		goto out_free_x86_fpu_cache;
+		goto out_free_x86_emulator_cache;
 	}
 
 	r = kvm_mmu_module_init();
@@ -7375,6 +7411,8 @@ int kvm_arch_init(void *opaque)
 
 out_free_percpu:
 	free_percpu(shared_msrs);
+out_free_x86_emulator_cache:
+	kmem_cache_destroy(x86_emulator_cache);
 out_free_x86_fpu_cache:
 	kmem_cache_destroy(x86_fpu_cache);
 out:
@@ -8754,7 +8792,7 @@ static void __get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 		 * that usually, but some bad designed PV devices (vmware
 		 * backdoor interface) need this to work
 		 */
-		emulator_writeback_register_cache(&vcpu->arch.emulate_ctxt);
+		emulator_writeback_register_cache(vcpu->arch.emulate_ctxt);
 		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
 	}
 	regs->rax = kvm_rax_read(vcpu);
@@ -8940,7 +8978,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
 		    int reason, bool has_error_code, u32 error_code)
 {
-	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
 	int ret;
 
 	init_emulate_ctxt(ctxt);
@@ -9273,7 +9311,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	struct page *page;
 	int r;
 
-	vcpu->arch.emulate_ctxt.ops = &emulate_ops;
 	if (!irqchip_in_kernel(vcpu->kvm) || kvm_vcpu_is_reset_bsp(vcpu))
 		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	else
@@ -9311,11 +9348,14 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 				GFP_KERNEL_ACCOUNT))
 		goto fail_free_mce_banks;
 
+	if (!alloc_emulate_ctxt(vcpu))
+		goto free_wbinvd_dirty_mask;
+
 	vcpu->arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
 						GFP_KERNEL_ACCOUNT);
 	if (!vcpu->arch.user_fpu) {
 		pr_err("kvm: failed to allocate userspace's fpu\n");
-		goto free_wbinvd_dirty_mask;
+		goto free_emulate_ctxt;
 	}
 
 	vcpu->arch.guest_fpu = kmem_cache_zalloc(x86_fpu_cache,
@@ -9357,6 +9397,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
 free_user_fpu:
 	kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
+free_emulate_ctxt:
+	kmem_cache_free(x86_emulator_cache, vcpu->arch.emulate_ctxt);
 free_wbinvd_dirty_mask:
 	free_cpumask_var(vcpu->arch.wbinvd_dirty_mask);
 fail_free_mce_banks:
@@ -9409,6 +9451,9 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 
 	kvm_x86_ops->vcpu_free(vcpu);
 
+	if (vcpu->arch.emulate_ctxt)
+		kmem_cache_free(x86_emulator_cache, vcpu->arch.emulate_ctxt);
+
 	free_cpumask_var(vcpu->arch.wbinvd_dirty_mask);
 	kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
 	kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (7 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 17:38   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context Sean Christopherson
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Now that the emulation context is dynamically allocated and not embedded
in struct kvm_vcpu, move its header, kvm_emulate.h, out of the public
asm directory and into KVM's private x86 directory.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h             | 5 ++++-
 arch/x86/kvm/emulate.c                      | 2 +-
 arch/x86/{include/asm => kvm}/kvm_emulate.h | 0
 arch/x86/kvm/mmu/mmu.c                      | 1 +
 arch/x86/kvm/x86.c                          | 1 +
 arch/x86/kvm/x86.h                          | 1 +
 6 files changed, 8 insertions(+), 2 deletions(-)
 rename arch/x86/{include/asm => kvm}/kvm_emulate.h (100%)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e069f71667b1..0dfe11f30d7f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -182,7 +182,10 @@ enum exit_fastpath_completion {
 	EXIT_FASTPATH_SKIP_EMUL_INS,
 };
 
-#include <asm/kvm_emulate.h>
+struct x86_emulate_ctxt;
+struct x86_exception;
+enum x86_intercept;
+enum x86_intercept_stage;
 
 #define KVM_NR_MEM_OBJS 40
 
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 1e394cb190ce..0bceb3f71220 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -20,7 +20,7 @@
 
 #include <linux/kvm_host.h>
 #include "kvm_cache_regs.h"
-#include <asm/kvm_emulate.h>
+#include "kvm_emulate.h"
 #include <linux/stringify.h>
 #include <asm/fpu/api.h>
 #include <asm/debugreg.h>
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
similarity index 100%
rename from arch/x86/include/asm/kvm_emulate.h
rename to arch/x86/kvm/kvm_emulate.h
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 7011a4e54866..d9064be28089 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -19,6 +19,7 @@
 #include "mmu.h"
 #include "x86.h"
 #include "kvm_cache_regs.h"
+#include "kvm_emulate.h"
 #include "cpuid.h"
 
 #include <linux/kvm_host.h>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5ab7d4283185..370af9fe0f5b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -22,6 +22,7 @@
 #include "i8254.h"
 #include "tss.h"
 #include "kvm_cache_regs.h"
+#include "kvm_emulate.h"
 #include "x86.h"
 #include "cpuid.h"
 #include "pmu.h"
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 8409842a25d9..f3c6e55eb5d9 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -5,6 +5,7 @@
 #include <linux/kvm_host.h>
 #include <asm/pvclock.h>
 #include "kvm_cache_regs.h"
+#include "kvm_emulate.h"
 
 #define KVM_DEFAULT_PLE_GAP		128
 #define KVM_VMX_DEFAULT_PLE_WINDOW	4096
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (8 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 17:51   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error Sean Christopherson
                   ` (3 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Shuffle a few operand structs to the end of struct x86_emulate_ctxt and
update the cache creation to whitelist only the region of the emulation
context that is expected to be copied to/from user memory, e.g. the
instruction operands, registers, and fetch/io/mem caches.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/kvm_emulate.h |  8 +++++---
 arch/x86/kvm/x86.c         | 12 ++++++------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 2f0a600efdff..82f712d5c692 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -322,9 +322,6 @@ struct x86_emulate_ctxt {
 	u8 intercept;
 	u8 op_bytes;
 	u8 ad_bytes;
-	struct operand src;
-	struct operand src2;
-	struct operand dst;
 	int (*execute)(struct x86_emulate_ctxt *ctxt);
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
 	/*
@@ -349,6 +346,11 @@ struct x86_emulate_ctxt {
 	u8 seg_override;
 	u64 d;
 	unsigned long _eip;
+
+	/* Here begins the usercopy section. */
+	struct operand src;
+	struct operand src2;
+	struct operand dst;
 	struct operand memop;
 	/* Fields above regs are cleared together. */
 	unsigned long _regs[NR_VCPU_REGS];
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 370af9fe0f5b..e1eaca65756b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -235,13 +235,13 @@ static struct kmem_cache *x86_emulator_cache;
 
 static struct kmem_cache *kvm_alloc_emulator_cache(void)
 {
-	return kmem_cache_create_usercopy("x86_emulator",
-					  sizeof(struct x86_emulate_ctxt),
+	unsigned int useroffset = offsetof(struct x86_emulate_ctxt, src);
+	unsigned int size = sizeof(struct x86_emulate_ctxt);
+
+	return kmem_cache_create_usercopy("x86_emulator", size,
 					  __alignof__(struct x86_emulate_ctxt),
-					  SLAB_ACCOUNT,
-					  0,
-					  sizeof(struct x86_emulate_ctxt),
-					  NULL);
+					  SLAB_ACCOUNT, useroffset,
+					  size - useroffset, NULL);
 }
 
 static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (9 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 17:52   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator Sean Christopherson
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Add a helper to set the appropriate error codes in vcpu->run when
emulation fails (future patches will add additional failure scenarios).

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e1eaca65756b..7bffdc6f9e1b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6491,6 +6491,14 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 }
 EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
 
+static int internal_emulation_error(struct kvm_vcpu *vcpu)
+{
+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+	vcpu->run->internal.ndata = 0;
+	return 0;
+}
+
 static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt,
 				    int emulation_type)
 {
@@ -6504,21 +6512,13 @@ static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt,
 		return 1;
 	}
 
-	if (emulation_type & EMULTYPE_SKIP) {
-		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-		vcpu->run->internal.ndata = 0;
-		return 0;
-	}
+	if (emulation_type & EMULTYPE_SKIP)
+		return internal_emulation_error(vcpu);
 
 	kvm_queue_exception(vcpu, UD_VECTOR);
 
-	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
-		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-		vcpu->run->internal.ndata = 0;
-		return 0;
-	}
+	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0)
+		return internal_emulation_error(vcpu);
 
 	return 1;
 }
@@ -8986,12 +8986,8 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
 
 	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
 				   has_error_code, error_code);
-	if (ret) {
-		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
-		vcpu->run->internal.ndata = 0;
-		return 0;
-	}
+	if (ret)
+		return internal_emulation_error(vcpu);
 
 	kvm_rip_write(vcpu, ctxt->eip);
 	kvm_set_rflags(vcpu, ctxt->eflags);
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (10 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-02-26 18:01   ` Vitaly Kuznetsov
  2020-02-18 23:29 ` [PATCH v2 13/13] KVM: x86: Allow userspace to disable the kernel's emulator Sean Christopherson
  2020-03-02 18:42 ` [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Paolo Bonzini
  13 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Add a global variable to control whether or not the emulator is enabled,
and make all necessary changes to gracefully handle reaching emulation
paths with the emulator disabled.

Running with VMX's unrestricted guest disabled requires special
consideration due to its use of kvm_inject_realmode_interrupt().  When
unrestricted guest is disabled, KVM emulates interrupts and exceptions
when the processor is in real mode, but does so without going through
the standard emulator loop.  Ideally, kvm_inject_realmode_interrupt()
would only log the interrupt and defer actual emulation to the standard
run loop, but that is a non-trivial change and a waste of resources
given that unrestricted guest is supported on all CPUs shipped within
the last decade.  Similarly, dirtying up the event injection stack for
such a legacy feature is undesirable.  To avoid the conundrum, prevent
disabling both the emulator and unrestricted guest.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/svm.c              |  2 +-
 arch/x86/kvm/vmx/vmx.c          |  7 ++++++-
 arch/x86/kvm/x86.c              | 18 +++++++++++++++---
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0dfe11f30d7f..c4baac32a291 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1050,7 +1050,7 @@ struct kvm_x86_ops {
 	int (*hardware_enable)(void);
 	void (*hardware_disable)(void);
 	int (*check_processor_compatibility)(void);/* __init */
-	int (*hardware_setup)(void);               /* __init */
+	int (*hardware_setup)(bool enable_emulator); /* __init */
 	void (*hardware_unsetup)(void);            /* __exit */
 	bool (*cpu_has_accelerated_tpr)(void);
 	bool (*has_emulated_msr)(int index);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index ae62ea454158..810139b3bfe4 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1350,7 +1350,7 @@ static __init void svm_adjust_mmio_mask(void)
 	kvm_mmu_set_mmio_spte_mask(mask, mask, PT_WRITABLE_MASK | PT_USER_MASK);
 }
 
-static __init int svm_hardware_setup(void)
+static __init int svm_hardware_setup(bool enable_emulator)
 {
 	int cpu;
 	struct page *iopm_pages;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 09bb0d98afeb..e05d36f63b73 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7548,7 +7548,7 @@ static bool vmx_apic_init_signal_blocked(struct kvm_vcpu *vcpu)
 	return to_vmx(vcpu)->nested.vmxon;
 }
 
-static __init int hardware_setup(void)
+static __init int hardware_setup(bool enable_emulator)
 {
 	unsigned long host_bndcfgs;
 	struct desc_ptr dt;
@@ -7595,6 +7595,11 @@ static __init int hardware_setup(void)
 	if (!cpu_has_virtual_nmis())
 		enable_vnmi = 0;
 
+	if (!enable_emulator && !enable_unrestricted_guest) {
+		pr_warn("kvm: unrestricted guest disabled, emulator must be enabled\n");
+		return -EIO;
+	}
+
 	/*
 	 * set_apic_access_page_addr() is used to reload apic access
 	 * page upon invalidation.  No need to do anything if not
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7bffdc6f9e1b..f9134e1104c2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -159,6 +159,8 @@ EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
 static bool __read_mostly force_emulation_prefix = false;
 module_param(force_emulation_prefix, bool, S_IRUGO);
 
+static const bool enable_emulator = true;
+
 int __read_mostly pi_inject_timer = -1;
 module_param(pi_inject_timer, bint, S_IRUGO | S_IWUSR);
 
@@ -6474,6 +6476,9 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
 	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
 	int ret;
 
+	if (WARN_ON_ONCE(!ctxt))
+		return;
+
 	init_emulate_ctxt(ctxt);
 
 	ctxt->op_bytes = 2;
@@ -6791,6 +6796,9 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 	bool writeback = true;
 	bool write_fault_to_spt = vcpu->arch.write_fault_to_shadow_pgtable;
 
+	if (!ctxt)
+		return internal_emulation_error(vcpu);
+
 	vcpu->arch.l1tf_flush_l1d = true;
 
 	/*
@@ -8785,7 +8793,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 
 static void __get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	if (vcpu->arch.emulate_regs_need_sync_to_vcpu) {
+	if (vcpu->arch.emulate_regs_need_sync_to_vcpu &&
+	    !(WARN_ON_ONCE(!vcpu->arch.emulate_ctxt))) {
 		/*
 		 * We are here if userspace calls get_regs() in the middle of
 		 * instruction emulation. Registers state needs to be copied
@@ -8982,6 +8991,9 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
 	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
 	int ret;
 
+	if (!ctxt)
+		return internal_emulation_error(vcpu);
+
 	init_emulate_ctxt(ctxt);
 
 	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
@@ -9345,7 +9357,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 				GFP_KERNEL_ACCOUNT))
 		goto fail_free_mce_banks;
 
-	if (!alloc_emulate_ctxt(vcpu))
+	if (enable_emulator && !alloc_emulate_ctxt(vcpu))
 		goto free_wbinvd_dirty_mask;
 
 	vcpu->arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
@@ -9651,7 +9663,7 @@ int kvm_arch_hardware_setup(void)
 {
 	int r;
 
-	r = kvm_x86_ops->hardware_setup();
+	r = kvm_x86_ops->hardware_setup(enable_emulator);
 	if (r != 0)
 		return r;
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v2 13/13] KVM: x86: Allow userspace to disable the kernel's emulator
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (11 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator Sean Christopherson
@ 2020-02-18 23:29 ` Sean Christopherson
  2020-03-02 18:42 ` [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Paolo Bonzini
  13 siblings, 0 replies; 39+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:29 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Expose the emulator control to userspace via a module param.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f9134e1104c2..ca3aa185f5ee 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -159,7 +159,8 @@ EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
 static bool __read_mostly force_emulation_prefix = false;
 module_param(force_emulation_prefix, bool, S_IRUGO);
 
-static const bool enable_emulator = true;
+static bool __read_mostly enable_emulator = true;
+module_param_named(emulator, enable_emulator, bool, 0440);
 
 int __read_mostly pi_inject_timer = -1;
 module_param(pi_inject_timer, bint, S_IRUGO | S_IWUSR);
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant
  2020-02-18 23:29 ` [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant Sean Christopherson
@ 2020-02-26 15:16   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 15:16 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Add variants of the I/O helpers that take a vCPU instead of an emulation
> context.  This will eventually allow KVM to limit use of the emulation
> context to the full emulation path.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fbabb2f06273..6554abef631f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5959,11 +5959,9 @@ static int emulator_pio_in_out(struct kvm_vcpu *vcpu, int size,
>  	return 0;
>  }
>  
> -static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> -				    int size, unsigned short port, void *val,
> -				    unsigned int count)
> +static int emulator_pio_in(struct kvm_vcpu *vcpu, int size,
> +			   unsigned short port, void *val, unsigned int count)
>  {
> -	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
>  	int ret;
>  
>  	if (vcpu->arch.pio.count)
> @@ -5983,17 +5981,30 @@ static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
>  	return 0;
>  }
>  
> -static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
> -				     int size, unsigned short port,
> -				     const void *val, unsigned int count)
> +static int emulator_pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> +				    int size, unsigned short port, void *val,
> +				    unsigned int count)
>  {
> -	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> +	return emulator_pio_in(emul_to_vcpu(ctxt), size, port, val, count);
>  
> +}
> +
> +static int emulator_pio_out(struct kvm_vcpu *vcpu, int size,
> +			    unsigned short port, const void *val,
> +			    unsigned int count)
> +{
>  	memcpy(vcpu->arch.pio_data, val, size * count);
>  	trace_kvm_pio(KVM_PIO_OUT, port, size, count, vcpu->arch.pio_data);
>  	return emulator_pio_in_out(vcpu, size, port, (void *)val, count, false);
>  }
>  
> +static int emulator_pio_out_emulated(struct x86_emulate_ctxt *ctxt,
> +				     int size, unsigned short port,
> +				     const void *val, unsigned int count)
> +{
> +	return emulator_pio_out(emul_to_vcpu(ctxt), size, port, val, count);
> +}
> +
>  static unsigned long get_segment_base(struct kvm_vcpu *vcpu, int seg)
>  {
>  	return kvm_x86_ops->get_segment_base(vcpu, seg);
> @@ -6930,8 +6941,8 @@ static int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size,
>  			    unsigned short port)
>  {
>  	unsigned long val = kvm_rax_read(vcpu);
> -	int ret = emulator_pio_out_emulated(&vcpu->arch.emulate_ctxt,
> -					    size, port, &val, 1);
> +	int ret = emulator_pio_out(vcpu, size, port, &val, 1);
> +
>  	if (ret)
>  		return ret;
>  
> @@ -6967,11 +6978,10 @@ static int complete_fast_pio_in(struct kvm_vcpu *vcpu)
>  	val = (vcpu->arch.pio.size < 4) ? kvm_rax_read(vcpu) : 0;
>  
>  	/*
> -	 * Since vcpu->arch.pio.count == 1 let emulator_pio_in_emulated perform
> +	 * Since vcpu->arch.pio.count == 1 let emulator_pio_in perform
>  	 * the copy and tracing
>  	 */
> -	emulator_pio_in_emulated(&vcpu->arch.emulate_ctxt, vcpu->arch.pio.size,
> -				 vcpu->arch.pio.port, &val, 1);
> +	emulator_pio_in(vcpu, vcpu->arch.pio.size, vcpu->arch.pio.port, &val, 1);
>  	kvm_rax_write(vcpu, val);
>  
>  	return kvm_skip_emulated_instruction(vcpu);
> @@ -6986,8 +6996,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
>  	/* For size less than 4 we merge, else we zero extend */
>  	val = (size < 4) ? kvm_rax_read(vcpu) : 0;
>  
> -	ret = emulator_pio_in_emulated(&vcpu->arch.emulate_ctxt, size, port,
> -				       &val, 1);
> +	ret = emulator_pio_in(vcpu, size, port, &val, 1);
>  	if (ret) {
>  		kvm_rax_write(vcpu, val);
>  		return ret;

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c
  2020-02-18 23:29 ` [PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c Sean Christopherson
@ 2020-02-26 15:23   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 15:23 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Move ctxt_virt_addr_bits() and emul_is_noncanonical_address() from x86.h
> to emulate.c.  This eliminates all references to struct x86_emulate_ctxt
> from x86.h, and sets the stage for a future patch to stop including
> kvm_emulate.h in asm/kvm_host.h.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/emulate.c | 11 +++++++++++
>  arch/x86/kvm/x86.h     | 11 -----------
>  2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index ddbc61984227..1e394cb190ce 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -673,6 +673,17 @@ static void set_segment_selector(struct x86_emulate_ctxt *ctxt, u16 selector,
>  	ctxt->ops->set_segment(ctxt, selector, &desc, base3, seg);
>  }
>  
> +static inline u8 ctxt_virt_addr_bits(struct x86_emulate_ctxt *ctxt)
> +{
> +	return (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_LA57) ? 57 : 48;
> +}
> +
> +static inline bool emul_is_noncanonical_address(u64 la,
> +						struct x86_emulate_ctxt *ctxt)
> +{
> +	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
> +}
> +
>  /*
>   * x86 defines three classes of vector instructions: explicitly
>   * aligned, explicitly unaligned, and the rest, which change behaviour
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 3624665acee4..8409842a25d9 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -149,11 +149,6 @@ static inline u8 vcpu_virt_addr_bits(struct kvm_vcpu *vcpu)
>  	return kvm_read_cr4_bits(vcpu, X86_CR4_LA57) ? 57 : 48;
>  }
>  
> -static inline u8 ctxt_virt_addr_bits(struct x86_emulate_ctxt *ctxt)
> -{
> -	return (ctxt->ops->get_cr(ctxt, 4) & X86_CR4_LA57) ? 57 : 48;
> -}
> -
>  static inline u64 get_canonical(u64 la, u8 vaddr_bits)
>  {
>  	return ((int64_t)la << (64 - vaddr_bits)) >> (64 - vaddr_bits);
> @@ -164,12 +159,6 @@ static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu)
>  	return get_canonical(la, vcpu_virt_addr_bits(vcpu)) != la;
>  }
>  
> -static inline bool emul_is_noncanonical_address(u64 la,
> -						struct x86_emulate_ctxt *ctxt)
> -{
> -	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
> -}
> -
>  static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
>  					gva_t gva, gfn_t gfn, unsigned access)
>  {

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context
  2020-02-18 23:29 ` [PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context Sean Christopherson
@ 2020-02-26 15:24   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 15:24 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Invert the vcpu->context derivation in emulator_read_write_onepage() in
> preparation for dynamically allocating the emulation context.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/x86.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 409bf35f26fd..772e704e8083 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5720,14 +5720,14 @@ static const struct read_write_emulator_ops write_emultor = {
>  static int emulator_read_write_onepage(unsigned long addr, void *val,
>  				       unsigned int bytes,
>  				       struct x86_exception *exception,
> -				       struct kvm_vcpu *vcpu,
> +				       struct x86_emulate_ctxt *ctxt,
>  				       const struct read_write_emulator_ops *ops)
>  {
>  	gpa_t gpa;
>  	int handled, ret;
>  	bool write = ops->write;
>  	struct kvm_mmio_fragment *frag;
> -	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> +	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
>  
>  	/*
>  	 * If the exit was due to a NPF we may already have a GPA.
> @@ -5791,7 +5791,7 @@ static int emulator_read_write(struct x86_emulate_ctxt *ctxt,
>  
>  		now = -addr & ~PAGE_MASK;
>  		rc = emulator_read_write_onepage(addr, val, now, exception,
> -						 vcpu, ops);
> +						 ctxt, ops);
>  
>  		if (rc != X86EMUL_CONTINUE)
>  			return rc;
> @@ -5803,7 +5803,7 @@ static int emulator_read_write(struct x86_emulate_ctxt *ctxt,
>  	}
>  
>  	rc = emulator_read_write_onepage(addr, val, bytes, exception,
> -					 vcpu, ops);
> +					 ctxt, ops);
>  	if (rc != X86EMUL_CONTINUE)
>  		return rc;

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context
  2020-02-18 23:29 ` [PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context Sean Christopherson
@ 2020-02-26 15:25   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 15:25 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Invert the vcpu->context derivation in inject_emulated_exception() in
> preparation for dynamically allocating the emulation context.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/x86.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 772e704e8083..79d1113ad6e7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6399,9 +6399,10 @@ static void toggle_interruptibility(struct kvm_vcpu *vcpu, u32 mask)
>  	}
>  }
>  
> -static bool inject_emulated_exception(struct kvm_vcpu *vcpu)
> +static bool inject_emulated_exception(struct x86_emulate_ctxt *ctxt)
>  {
> -	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> +	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> +
>  	if (ctxt->exception.vector == PF_VECTOR)
>  		return kvm_propagate_fault(vcpu, &ctxt->exception);
>  
> @@ -6806,7 +6807,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  				 */
>  				WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR ||
>  					     exception_type(ctxt->exception.vector) == EXCPT_TRAP);
> -				inject_emulated_exception(vcpu);
> +				inject_emulated_exception(ctxt);
>  				return 1;
>  			}
>  			return handle_emulation_failure(vcpu, emulation_type);
> @@ -6860,7 +6861,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  
>  	if (ctxt->have_exception) {
>  		r = 1;
> -		if (inject_emulated_exception(vcpu))
> +		if (inject_emulated_exception(ctxt))
>  			return r;
>  	} else if (vcpu->arch.pio.count) {
>  		if (!vcpu->arch.pio.in) {

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context
  2020-02-18 23:29 ` [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context Sean Christopherson
@ 2020-02-26 17:11   ` Vitaly Kuznetsov
  2020-03-03 16:48     ` Sean Christopherson
  0 siblings, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 17:11 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Explicitly pass the emulation context to the emulate tracepoint in
> preparation of dynamically allocation the emulation context.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/trace.h | 22 +++++++++++-----------
>  arch/x86/kvm/x86.c   | 13 ++++++++-----
>  2 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index f194dd058470..5605000ca5f6 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -731,8 +731,9 @@ TRACE_EVENT(kvm_skinit,
>  	})
>  
>  TRACE_EVENT(kvm_emulate_insn,
> -	TP_PROTO(struct kvm_vcpu *vcpu, __u8 failed),
> -	TP_ARGS(vcpu, failed),
> +	TP_PROTO(struct kvm_vcpu *vcpu, struct x86_emulate_ctxt *ctxt,
> +		 __u8 failed),
> +	TP_ARGS(vcpu, ctxt, failed),
>  
>  	TP_STRUCT__entry(
>  		__field(    __u64, rip                       )
> @@ -745,13 +746,10 @@ TRACE_EVENT(kvm_emulate_insn,
>  
>  	TP_fast_assign(
>  		__entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS);

This seems the only usage of 'vcpu' parameter now; I checked and even
after switching to dynamic emulation context allocation we still set
ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter
here then (and use ctxt->vcpu instead)?

> -		__entry->len = vcpu->arch.emulate_ctxt.fetch.ptr
> -			       - vcpu->arch.emulate_ctxt.fetch.data;
> -		__entry->rip = vcpu->arch.emulate_ctxt._eip - __entry->len;
> -		memcpy(__entry->insn,
> -		       vcpu->arch.emulate_ctxt.fetch.data,
> -		       15);
> -		__entry->flags = kei_decode_mode(vcpu->arch.emulate_ctxt.mode);
> +		__entry->len = ctxt->fetch.ptr - ctxt->fetch.data;
> +		__entry->rip = ctxt->_eip - __entry->len;
> +		memcpy(__entry->insn, ctxt->fetch.data, 15);
> +		__entry->flags = kei_decode_mode(ctxt->mode);
>  		__entry->failed = failed;
>  		),
>  
> @@ -764,8 +762,10 @@ TRACE_EVENT(kvm_emulate_insn,
>  		)
>  	);
>  
> -#define trace_kvm_emulate_insn_start(vcpu) trace_kvm_emulate_insn(vcpu, 0)
> -#define trace_kvm_emulate_insn_failed(vcpu) trace_kvm_emulate_insn(vcpu, 1)
> +#define trace_kvm_emulate_insn_start(vcpu, ctxt)	\
> +	trace_kvm_emulate_insn(vcpu, ctxt, 0)
> +#define trace_kvm_emulate_insn_failed(vcpu, ctxt)	\
> +	trace_kvm_emulate_insn(vcpu, ctxt, 1)
>  
>  TRACE_EVENT(
>  	vcpu_match_mmio,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 79d1113ad6e7..69d3dd64d90c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6460,10 +6460,13 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>  }
>  EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
>  
> -static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> +static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt,
> +				    int emulation_type)
>  {
> +	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> +
>  	++vcpu->stat.insn_emulation_fail;
> -	trace_kvm_emulate_insn_failed(vcpu);
> +	trace_kvm_emulate_insn_failed(vcpu, ctxt);
>  
>  	if (emulation_type & EMULTYPE_VMWARE_GP) {
>  		kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
> @@ -6788,7 +6791,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  
>  		r = x86_decode_insn(ctxt, insn, insn_len);
>  
> -		trace_kvm_emulate_insn_start(vcpu);
> +		trace_kvm_emulate_insn_start(vcpu, ctxt);
>  		++vcpu->stat.insn_emulation;
>  		if (r != EMULATION_OK)  {
>  			if ((emulation_type & EMULTYPE_TRAP_UD) ||
> @@ -6810,7 +6813,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  				inject_emulated_exception(ctxt);
>  				return 1;
>  			}
> -			return handle_emulation_failure(vcpu, emulation_type);
> +			return handle_emulation_failure(ctxt, emulation_type);
>  		}
>  	}
>  
> @@ -6856,7 +6859,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  					emulation_type))
>  			return 1;
>  
> -		return handle_emulation_failure(vcpu, emulation_type);
> +		return handle_emulation_failure(ctxt, emulation_type);
>  	}
>  
>  	if (ctxt->have_exception) {

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() to explicitly take context
  2020-02-18 23:29 ` [PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() " Sean Christopherson
@ 2020-02-26 17:13   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 17:13 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Explicitly pass the emulation context when initializing said context in
> preparation of dynamically allocation the emulation context.

"The said said context" :-)

>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/x86.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 69d3dd64d90c..0e67f90db9a6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6414,9 +6414,9 @@ static bool inject_emulated_exception(struct x86_emulate_ctxt *ctxt)
>  	return false;
>  }
>  
> -static void init_emulate_ctxt(struct kvm_vcpu *vcpu)
> +static void init_emulate_ctxt(struct x86_emulate_ctxt *ctxt)
>  {
> -	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> +	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
>  	int cs_db, cs_l;
>  
>  	kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
> @@ -6443,7 +6443,7 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>  	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
>  	int ret;
>  
> -	init_emulate_ctxt(vcpu);
> +	init_emulate_ctxt(ctxt);
>  
>  	ctxt->op_bytes = 2;
>  	ctxt->ad_bytes = 2;
> @@ -6770,7 +6770,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  	kvm_clear_exception_queue(vcpu);
>  
>  	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
> -		init_emulate_ctxt(vcpu);
> +		init_emulate_ctxt(ctxt);
>  
>  		/*
>  		 * We will reenter on the same instruction since
> @@ -8943,7 +8943,7 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>  	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
>  	int ret;
>  
> -	init_emulate_ctxt(vcpu);
> +	init_emulate_ctxt(ctxt);
>  
>  	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
>  				   has_error_code, error_code);

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context
  2020-02-18 23:29 ` [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context Sean Christopherson
@ 2020-02-26 17:29   ` Vitaly Kuznetsov
  2020-03-03 10:26     ` Paolo Bonzini
  2020-03-03 16:52     ` Sean Christopherson
  0 siblings, 2 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 17:29 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Allocate the emulation context instead of embedding it in struct
> kvm_vcpu_arch.
>
> Dynamic allocation provides several benefits:
>
>   - Shrinks the size x86 vcpus by ~2.5k bytes, dropping them back below
>     the PAGE_ALLOC_COSTLY_ORDER threshold.
>   - Allows for dropping the include of kvm_emulate.h from asm/kvm_host.h
>     and moving kvm_emulate.h into KVM's private directory.
>   - Allows a reducing KVM's attack surface by shrinking the amount of
>     vCPU data that is exposed to usercopy.
>   - Allows a future patch to disable the emulator entirely, which may or
>     may not be a realistic endeavor.
>
> Mark the entire struct as valid for usercopy to maintain existing
> behavior with respect to hardened usercopy.  Future patches can shrink
> the usercopy range to cover only what is necessary.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/include/asm/kvm_emulate.h |  1 +
>  arch/x86/include/asm/kvm_host.h    |  2 +-
>  arch/x86/kvm/x86.c                 | 61 ++++++++++++++++++++++++++----
>  3 files changed, 55 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index 03946eb3e2b9..2f0a600efdff 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -293,6 +293,7 @@ enum x86emul_mode {
>  #define X86EMUL_SMM_INSIDE_NMI_MASK  (1 << 7)
>  
>  struct x86_emulate_ctxt {
> +	void *vcpu;

Why 'void *'? I changed this to 'struct kvm_vcpu *' and it seems to
compile just fine...

>  	const struct x86_emulate_ops *ops;
>  
>  	/* Register state before/after emulation. */
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c750cd957558..e069f71667b1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -678,7 +678,7 @@ struct kvm_vcpu_arch {
>  
>  	/* emulate context */
>  
> -	struct x86_emulate_ctxt emulate_ctxt;
> +	struct x86_emulate_ctxt *emulate_ctxt;
>  	bool emulate_regs_need_sync_to_vcpu;
>  	bool emulate_regs_need_sync_from_vcpu;
>  	int (*complete_userspace_io)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0e67f90db9a6..5ab7d4283185 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -81,7 +81,7 @@ u64 __read_mostly kvm_mce_cap_supported = MCG_CTL_P | MCG_SER_P;
>  EXPORT_SYMBOL_GPL(kvm_mce_cap_supported);
>  
>  #define emul_to_vcpu(ctxt) \
> -	container_of(ctxt, struct kvm_vcpu, arch.emulate_ctxt)
> +	((struct kvm_vcpu *)(ctxt)->vcpu)
>  
>  /* EFER defaults:
>   * - enable syscall per default because its emulated by KVM
> @@ -230,6 +230,19 @@ u64 __read_mostly host_xcr0;
>  struct kmem_cache *x86_fpu_cache;
>  EXPORT_SYMBOL_GPL(x86_fpu_cache);
>  
> +static struct kmem_cache *x86_emulator_cache;
> +
> +static struct kmem_cache *kvm_alloc_emulator_cache(void)
> +{
> +	return kmem_cache_create_usercopy("x86_emulator",
> +					  sizeof(struct x86_emulate_ctxt),
> +					  __alignof__(struct x86_emulate_ctxt),
> +					  SLAB_ACCOUNT,
> +					  0,
> +					  sizeof(struct x86_emulate_ctxt),
> +					  NULL);
> +}
> +
>  static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);
>  
>  static inline void kvm_async_pf_hash_reset(struct kvm_vcpu *vcpu)
> @@ -6414,6 +6427,23 @@ static bool inject_emulated_exception(struct x86_emulate_ctxt *ctxt)
>  	return false;
>  }
>  
> +static struct x86_emulate_ctxt *alloc_emulate_ctxt(struct kvm_vcpu *vcpu)
> +{
> +	struct x86_emulate_ctxt *ctxt;
> +
> +	ctxt = kmem_cache_zalloc(x86_emulator_cache, GFP_KERNEL_ACCOUNT);
> +	if (!ctxt) {
> +		pr_err("kvm: failed to allocate vcpu's emulator\n");
> +		return NULL;
> +	}
> +
> +	ctxt->vcpu = vcpu;
> +	ctxt->ops = &emulate_ops;
> +	vcpu->arch.emulate_ctxt = ctxt;
> +
> +	return ctxt;
> +}
> +
>  static void init_emulate_ctxt(struct x86_emulate_ctxt *ctxt)
>  {
>  	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> @@ -6440,7 +6470,7 @@ static void init_emulate_ctxt(struct x86_emulate_ctxt *ctxt)
>  
>  void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>  {
> -	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> +	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>  	int ret;
>  
>  	init_emulate_ctxt(ctxt);
> @@ -6756,7 +6786,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  			    int emulation_type, void *insn, int insn_len)
>  {
>  	int r;
> -	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> +	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>  	bool writeback = true;
>  	bool write_fault_to_spt = vcpu->arch.write_fault_to_shadow_pgtable;
>  
> @@ -7339,10 +7369,16 @@ int kvm_arch_init(void *opaque)
>  		goto out;
>  	}
>  
> +	x86_emulator_cache = kvm_alloc_emulator_cache();
> +	if (!x86_emulator_cache) {
> +		pr_err("kvm: failed to allocate cache for x86 emulator\n");
> +		goto out_free_x86_fpu_cache;
> +	}
> +
>  	shared_msrs = alloc_percpu(struct kvm_shared_msrs);
>  	if (!shared_msrs) {
>  		printk(KERN_ERR "kvm: failed to allocate percpu kvm_shared_msrs\n");
> -		goto out_free_x86_fpu_cache;
> +		goto out_free_x86_emulator_cache;
>  	}
>  
>  	r = kvm_mmu_module_init();
> @@ -7375,6 +7411,8 @@ int kvm_arch_init(void *opaque)
>  
>  out_free_percpu:
>  	free_percpu(shared_msrs);
> +out_free_x86_emulator_cache:
> +	kmem_cache_destroy(x86_emulator_cache);
>  out_free_x86_fpu_cache:
>  	kmem_cache_destroy(x86_fpu_cache);
>  out:
> @@ -8754,7 +8792,7 @@ static void __get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>  		 * that usually, but some bad designed PV devices (vmware
>  		 * backdoor interface) need this to work
>  		 */
> -		emulator_writeback_register_cache(&vcpu->arch.emulate_ctxt);
> +		emulator_writeback_register_cache(vcpu->arch.emulate_ctxt);
>  		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
>  	}
>  	regs->rax = kvm_rax_read(vcpu);
> @@ -8940,7 +8978,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>  		    int reason, bool has_error_code, u32 error_code)
>  {
> -	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> +	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>  	int ret;
>  
>  	init_emulate_ctxt(ctxt);
> @@ -9273,7 +9311,6 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	struct page *page;
>  	int r;
>  
> -	vcpu->arch.emulate_ctxt.ops = &emulate_ops;
>  	if (!irqchip_in_kernel(vcpu->kvm) || kvm_vcpu_is_reset_bsp(vcpu))
>  		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>  	else
> @@ -9311,11 +9348,14 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  				GFP_KERNEL_ACCOUNT))
>  		goto fail_free_mce_banks;
>  
> +	if (!alloc_emulate_ctxt(vcpu))
> +		goto free_wbinvd_dirty_mask;
> +
>  	vcpu->arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
>  						GFP_KERNEL_ACCOUNT);
>  	if (!vcpu->arch.user_fpu) {
>  		pr_err("kvm: failed to allocate userspace's fpu\n");
> -		goto free_wbinvd_dirty_mask;
> +		goto free_emulate_ctxt;
>  	}
>  
>  	vcpu->arch.guest_fpu = kmem_cache_zalloc(x86_fpu_cache,
> @@ -9357,6 +9397,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
>  free_user_fpu:
>  	kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
> +free_emulate_ctxt:
> +	kmem_cache_free(x86_emulator_cache, vcpu->arch.emulate_ctxt);
>  free_wbinvd_dirty_mask:
>  	free_cpumask_var(vcpu->arch.wbinvd_dirty_mask);
>  fail_free_mce_banks:
> @@ -9409,6 +9451,9 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>  
>  	kvm_x86_ops->vcpu_free(vcpu);
>  
> +	if (vcpu->arch.emulate_ctxt)
> +		kmem_cache_free(x86_emulator_cache, vcpu->arch.emulate_ctxt);

Checking for NULL here seems superfluous as we create the context in
kvm_arch_vcpu_create() unconditionally. I'd suggest we move the check to 
"[PATCH v2 12/13] KVM: x86: Add variable to control existence of
emulator" where 'enable_emulator' global is added.

> +
>  	free_cpumask_var(vcpu->arch.wbinvd_dirty_mask);
>  	kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
>  	kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory
  2020-02-18 23:29 ` [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory Sean Christopherson
@ 2020-02-26 17:38   ` Vitaly Kuznetsov
  2020-03-03 10:27     ` Paolo Bonzini
  0 siblings, 1 reply; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 17:38 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Now that the emulation context is dynamically allocated and not embedded
> in struct kvm_vcpu, move its header, kvm_emulate.h, out of the public
> asm directory and into KVM's private x86 directory.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h             | 5 ++++-
>  arch/x86/kvm/emulate.c                      | 2 +-
>  arch/x86/{include/asm => kvm}/kvm_emulate.h | 0
>  arch/x86/kvm/mmu/mmu.c                      | 1 +
>  arch/x86/kvm/x86.c                          | 1 +
>  arch/x86/kvm/x86.h                          | 1 +
>  6 files changed, 8 insertions(+), 2 deletions(-)
>  rename arch/x86/{include/asm => kvm}/kvm_emulate.h (100%)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index e069f71667b1..0dfe11f30d7f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -182,7 +182,10 @@ enum exit_fastpath_completion {
>  	EXIT_FASTPATH_SKIP_EMUL_INS,
>  };
>  
> -#include <asm/kvm_emulate.h>
> +struct x86_emulate_ctxt;

Not this patchset's problem (and particular forward declaration is
likely needed), but 

$ grep 'struct x86_emulate_ctxt' arch/x86/include/asm/kvm_host.h
struct x86_emulate_ctxt;
	struct x86_emulate_ctxt *emulate_ctxt;
struct x86_emulate_ctxt;

The second forward declaration is not needed and this patch (or
patchset) may be a good place to get rid of it)

> +struct x86_exception;
> +enum x86_intercept;
> +enum x86_intercept_stage;
>  
>  #define KVM_NR_MEM_OBJS 40
>  
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 1e394cb190ce..0bceb3f71220 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -20,7 +20,7 @@
>  
>  #include <linux/kvm_host.h>
>  #include "kvm_cache_regs.h"
> -#include <asm/kvm_emulate.h>
> +#include "kvm_emulate.h"
>  #include <linux/stringify.h>
>  #include <asm/fpu/api.h>
>  #include <asm/debugreg.h>
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> similarity index 100%
> rename from arch/x86/include/asm/kvm_emulate.h
> rename to arch/x86/kvm/kvm_emulate.h
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 7011a4e54866..d9064be28089 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -19,6 +19,7 @@
>  #include "mmu.h"
>  #include "x86.h"
>  #include "kvm_cache_regs.h"
> +#include "kvm_emulate.h"
>  #include "cpuid.h"
>  
>  #include <linux/kvm_host.h>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5ab7d4283185..370af9fe0f5b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -22,6 +22,7 @@
>  #include "i8254.h"
>  #include "tss.h"
>  #include "kvm_cache_regs.h"
> +#include "kvm_emulate.h"
>  #include "x86.h"
>  #include "cpuid.h"
>  #include "pmu.h"
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 8409842a25d9..f3c6e55eb5d9 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -5,6 +5,7 @@
>  #include <linux/kvm_host.h>
>  #include <asm/pvclock.h>
>  #include "kvm_cache_regs.h"
> +#include "kvm_emulate.h"
>  
>  #define KVM_DEFAULT_PLE_GAP		128
>  #define KVM_VMX_DEFAULT_PLE_WINDOW	4096

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context
  2020-02-18 23:29 ` [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context Sean Christopherson
@ 2020-02-26 17:51   ` Vitaly Kuznetsov
  2020-03-02 18:40     ` Paolo Bonzini
  2020-03-02 19:13     ` Sean Christopherson
  0 siblings, 2 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 17:51 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Shuffle a few operand structs to the end of struct x86_emulate_ctxt and
> update the cache creation to whitelist only the region of the emulation
> context that is expected to be copied to/from user memory, e.g. the
> instruction operands, registers, and fetch/io/mem caches.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/kvm_emulate.h |  8 +++++---
>  arch/x86/kvm/x86.c         | 12 ++++++------
>  2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 2f0a600efdff..82f712d5c692 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -322,9 +322,6 @@ struct x86_emulate_ctxt {
>  	u8 intercept;
>  	u8 op_bytes;
>  	u8 ad_bytes;
> -	struct operand src;
> -	struct operand src2;
> -	struct operand dst;
>  	int (*execute)(struct x86_emulate_ctxt *ctxt);
>  	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
>  	/*
> @@ -349,6 +346,11 @@ struct x86_emulate_ctxt {
>  	u8 seg_override;
>  	u64 d;
>  	unsigned long _eip;
> +
> +	/* Here begins the usercopy section. */
> +	struct operand src;
> +	struct operand src2;
> +	struct operand dst;

Out of pure curiosity, how certain are we that this is going to be
enough for userspaces?

>  	struct operand memop;
>  	/* Fields above regs are cleared together. */
>  	unsigned long _regs[NR_VCPU_REGS];
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 370af9fe0f5b..e1eaca65756b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -235,13 +235,13 @@ static struct kmem_cache *x86_emulator_cache;
>  
>  static struct kmem_cache *kvm_alloc_emulator_cache(void)
>  {
> -	return kmem_cache_create_usercopy("x86_emulator",
> -					  sizeof(struct x86_emulate_ctxt),
> +	unsigned int useroffset = offsetof(struct x86_emulate_ctxt, src);
> +	unsigned int size = sizeof(struct x86_emulate_ctxt);
> +
> +	return kmem_cache_create_usercopy("x86_emulator", size,
>  					  __alignof__(struct x86_emulate_ctxt),
> -					  SLAB_ACCOUNT,
> -					  0,
> -					  sizeof(struct x86_emulate_ctxt),
> -					  NULL);
> +					  SLAB_ACCOUNT, useroffset,
> +					  size - useroffset, NULL);
>  }
>  
>  static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt);

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error
  2020-02-18 23:29 ` [PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error Sean Christopherson
@ 2020-02-26 17:52   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 17:52 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Add a helper to set the appropriate error codes in vcpu->run when
> emulation fails (future patches will add additional failure scenarios).
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/x86.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e1eaca65756b..7bffdc6f9e1b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6491,6 +6491,14 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>  }
>  EXPORT_SYMBOL_GPL(kvm_inject_realmode_interrupt);
>  
> +static int internal_emulation_error(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> +	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> +	vcpu->run->internal.ndata = 0;
> +	return 0;
> +}
> +
>  static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt,
>  				    int emulation_type)
>  {
> @@ -6504,21 +6512,13 @@ static int handle_emulation_failure(struct x86_emulate_ctxt *ctxt,
>  		return 1;
>  	}
>  
> -	if (emulation_type & EMULTYPE_SKIP) {
> -		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> -		vcpu->run->internal.ndata = 0;
> -		return 0;
> -	}
> +	if (emulation_type & EMULTYPE_SKIP)
> +		return internal_emulation_error(vcpu);
>  
>  	kvm_queue_exception(vcpu, UD_VECTOR);
>  
> -	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0) {
> -		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> -		vcpu->run->internal.ndata = 0;
> -		return 0;
> -	}
> +	if (!is_guest_mode(vcpu) && kvm_x86_ops->get_cpl(vcpu) == 0)
> +		return internal_emulation_error(vcpu);
>  
>  	return 1;
>  }
> @@ -8986,12 +8986,8 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>  
>  	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
>  				   has_error_code, error_code);
> -	if (ret) {
> -		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> -		vcpu->run->internal.ndata = 0;
> -		return 0;
> -	}
> +	if (ret)
> +		return internal_emulation_error(vcpu);
>  
>  	kvm_rip_write(vcpu, ctxt->eip);
>  	kvm_set_rflags(vcpu, ctxt->eflags);

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator
  2020-02-18 23:29 ` [PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator Sean Christopherson
@ 2020-02-26 18:01   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-26 18:01 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Add a global variable to control whether or not the emulator is enabled,
> and make all necessary changes to gracefully handle reaching emulation
> paths with the emulator disabled.
>
> Running with VMX's unrestricted guest disabled requires special
> consideration due to its use of kvm_inject_realmode_interrupt().  When
> unrestricted guest is disabled, KVM emulates interrupts and exceptions
> when the processor is in real mode, but does so without going through
> the standard emulator loop.  Ideally, kvm_inject_realmode_interrupt()
> would only log the interrupt and defer actual emulation to the standard
> run loop, but that is a non-trivial change and a waste of resources
> given that unrestricted guest is supported on all CPUs shipped within
> the last decade.  Similarly, dirtying up the event injection stack for
> such a legacy feature is undesirable.  To avoid the conundrum, prevent
> disabling both the emulator and unrestricted guest.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  2 +-
>  arch/x86/kvm/svm.c              |  2 +-
>  arch/x86/kvm/vmx/vmx.c          |  7 ++++++-
>  arch/x86/kvm/x86.c              | 18 +++++++++++++++---
>  4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 0dfe11f30d7f..c4baac32a291 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1050,7 +1050,7 @@ struct kvm_x86_ops {
>  	int (*hardware_enable)(void);
>  	void (*hardware_disable)(void);
>  	int (*check_processor_compatibility)(void);/* __init */
> -	int (*hardware_setup)(void);               /* __init */
> +	int (*hardware_setup)(bool enable_emulator); /* __init */
>  	void (*hardware_unsetup)(void);            /* __exit */
>  	bool (*cpu_has_accelerated_tpr)(void);
>  	bool (*has_emulated_msr)(int index);
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ae62ea454158..810139b3bfe4 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1350,7 +1350,7 @@ static __init void svm_adjust_mmio_mask(void)
>  	kvm_mmu_set_mmio_spte_mask(mask, mask, PT_WRITABLE_MASK | PT_USER_MASK);
>  }
>  
> -static __init int svm_hardware_setup(void)
> +static __init int svm_hardware_setup(bool enable_emulator)
>  {
>  	int cpu;
>  	struct page *iopm_pages;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 09bb0d98afeb..e05d36f63b73 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7548,7 +7548,7 @@ static bool vmx_apic_init_signal_blocked(struct kvm_vcpu *vcpu)
>  	return to_vmx(vcpu)->nested.vmxon;
>  }
>  
> -static __init int hardware_setup(void)
> +static __init int hardware_setup(bool enable_emulator)
>  {
>  	unsigned long host_bndcfgs;
>  	struct desc_ptr dt;
> @@ -7595,6 +7595,11 @@ static __init int hardware_setup(void)
>  	if (!cpu_has_virtual_nmis())
>  		enable_vnmi = 0;
>  
> +	if (!enable_emulator && !enable_unrestricted_guest) {
> +		pr_warn("kvm: unrestricted guest disabled, emulator must be enabled\n");
> +		return -EIO;
> +	}
> +
>  	/*
>  	 * set_apic_access_page_addr() is used to reload apic access
>  	 * page upon invalidation.  No need to do anything if not
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7bffdc6f9e1b..f9134e1104c2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -159,6 +159,8 @@ EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>  static bool __read_mostly force_emulation_prefix = false;
>  module_param(force_emulation_prefix, bool, S_IRUGO);
>  
> +static const bool enable_emulator = true;
> +
>  int __read_mostly pi_inject_timer = -1;
>  module_param(pi_inject_timer, bint, S_IRUGO | S_IWUSR);
>  
> @@ -6474,6 +6476,9 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip)
>  	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>  	int ret;
>  
> +	if (WARN_ON_ONCE(!ctxt))
> +		return;
> +
>  	init_emulate_ctxt(ctxt);
>  
>  	ctxt->op_bytes = 2;
> @@ -6791,6 +6796,9 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  	bool writeback = true;
>  	bool write_fault_to_spt = vcpu->arch.write_fault_to_shadow_pgtable;
>  
> +	if (!ctxt)
> +		return internal_emulation_error(vcpu);
> +
>  	vcpu->arch.l1tf_flush_l1d = true;
>  
>  	/*
> @@ -8785,7 +8793,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  
>  static void __get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
>  {
> -	if (vcpu->arch.emulate_regs_need_sync_to_vcpu) {
> +	if (vcpu->arch.emulate_regs_need_sync_to_vcpu &&
> +	    !(WARN_ON_ONCE(!vcpu->arch.emulate_ctxt))) {
>  		/*
>  		 * We are here if userspace calls get_regs() in the middle of
>  		 * instruction emulation. Registers state needs to be copied
> @@ -8982,6 +8991,9 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>  	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>  	int ret;
>  
> +	if (!ctxt)
> +		return internal_emulation_error(vcpu);
> +
>  	init_emulate_ctxt(ctxt);
>  
>  	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
> @@ -9345,7 +9357,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  				GFP_KERNEL_ACCOUNT))
>  		goto fail_free_mce_banks;
>  
> -	if (!alloc_emulate_ctxt(vcpu))
> +	if (enable_emulator && !alloc_emulate_ctxt(vcpu))
>  		goto free_wbinvd_dirty_mask;
>  
>  	vcpu->arch.user_fpu = kmem_cache_zalloc(x86_fpu_cache,
> @@ -9651,7 +9663,7 @@ int kvm_arch_hardware_setup(void)
>  {
>  	int r;
>  
> -	r = kvm_x86_ops->hardware_setup();
> +	r = kvm_x86_ops->hardware_setup(enable_emulator);
>  	if (r != 0)
>  		return r;

I'm not sure that emulator disablement should _only_ be a global
varaiable, i.e. why can't we do it for some VMs and allow for others?
(we can even run different userspaces for different needs). This,
however, is complementary.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

(I hope we have some userspaces which are already onboard)

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context
  2020-02-26 17:51   ` Vitaly Kuznetsov
@ 2020-03-02 18:40     ` Paolo Bonzini
  2020-03-02 19:19       ` Sean Christopherson
  2020-03-02 19:13     ` Sean Christopherson
  1 sibling, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2020-03-02 18:40 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

On 26/02/20 18:51, Vitaly Kuznetsov wrote:
>> +
>> +	/* Here begins the usercopy section. */
>> +	struct operand src;
>> +	struct operand src2;
>> +	struct operand dst;
> Out of pure curiosity, how certain are we that this is going to be
> enough for userspaces?
> 

And also, where exactly are the user copies done?

Paolo


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator
  2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
                   ` (12 preceding siblings ...)
  2020-02-18 23:29 ` [PATCH v2 13/13] KVM: x86: Allow userspace to disable the kernel's emulator Sean Christopherson
@ 2020-03-02 18:42 ` Paolo Bonzini
  2020-03-02 20:02   ` Sean Christopherson
  13 siblings, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2020-03-02 18:42 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On 19/02/20 00:29, Sean Christopherson wrote:
> The primary intent of this series is to dynamically allocate the emulator
> and get KVM to a state where the emulator *could* be disabled at some
> point in the future.  Actually allowing userspace to disable the emulator
> was a minor change at that point, so I threw it in.
> 
> Dynamically allocating the emulator shrinks the size of x86 vcpus by
> ~2.5k bytes, which is important because 'struct vcpu_vmx' has once again
> fattened up and squeaked past the PAGE_ALLOC_COSTLY_ORDER threshold.
> Moving the emulator to its own allocation gives us some breathing room
> for the near future, and has some other nice side effects.
> 
> As for disabling the emulator... in the not-too-distant future, I expect
> there will be use cases that can truly disable KVM's emulator, e.g. for
> security (KVM's and/or the guests).  I don't have a strong opinion on
> whether or not KVM should actually allow userspace to disable the emulator
> without a concrete use case (unless there already is a use case?), which
> is why that part is done in its own tiny patch.
> 
> Running without an emulator has been "tested" in the sense that the
> selftests that don't require emulation continue to pass, and everything
> else fails with the expected "emulation error".

I agree with Vitaly that, if we want this, it should be a KVM_ENABLE_CAP
instead.  The first 10 patches are very nice cleanups though so I plan
to apply them (with Vitaly's suggested nits for review) after you answer
the question on patch 10.

Paolo

> 
> v2:
>   - Rebase to kvm/queue, 2c2787938512 ("KVM: selftests: Stop ...")
> 
> Sean Christopherson (13):
>   KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant
>   KVM: x86: Explicitly pass an exception struct to check_intercept
>   KVM: x86: Move emulation-only helpers to emulate.c
>   KVM: x86: Refactor R/W page helper to take the emulation context
>   KVM: x86: Refactor emulated exception injection to take the emul
>     context
>   KVM: x86: Refactor emulate tracepoint to explicitly take context
>   KVM: x86: Refactor init_emulate_ctxt() to explicitly take context
>   KVM: x86: Dynamically allocate per-vCPU emulation context
>   KVM: x86: Move kvm_emulate.h into KVM's private directory
>   KVM: x86: Shrink the usercopy region of the emulation context
>   KVM: x86: Add helper to "handle" internal emulation error
>   KVM: x86: Add variable to control existence of emulator
>   KVM: x86: Allow userspace to disable the kernel's emulator
> 
>  arch/x86/include/asm/kvm_host.h             |  12 +-
>  arch/x86/kvm/emulate.c                      |  13 +-
>  arch/x86/{include/asm => kvm}/kvm_emulate.h |   9 +-
>  arch/x86/kvm/mmu/mmu.c                      |   1 +
>  arch/x86/kvm/svm.c                          |   5 +-
>  arch/x86/kvm/trace.h                        |  22 +--
>  arch/x86/kvm/vmx/vmx.c                      |  15 +-
>  arch/x86/kvm/x86.c                          | 193 +++++++++++++-------
>  arch/x86/kvm/x86.h                          |  12 +-
>  9 files changed, 183 insertions(+), 99 deletions(-)
>  rename arch/x86/{include/asm => kvm}/kvm_emulate.h (99%)
> 


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context
  2020-02-26 17:51   ` Vitaly Kuznetsov
  2020-03-02 18:40     ` Paolo Bonzini
@ 2020-03-02 19:13     ` Sean Christopherson
  1 sibling, 0 replies; 39+ messages in thread
From: Sean Christopherson @ 2020-03-02 19:13 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

On Wed, Feb 26, 2020 at 06:51:02PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > Shuffle a few operand structs to the end of struct x86_emulate_ctxt and
> > update the cache creation to whitelist only the region of the emulation
> > context that is expected to be copied to/from user memory, e.g. the
> > instruction operands, registers, and fetch/io/mem caches.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/kvm_emulate.h |  8 +++++---
> >  arch/x86/kvm/x86.c         | 12 ++++++------
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> > index 2f0a600efdff..82f712d5c692 100644
> > --- a/arch/x86/kvm/kvm_emulate.h
> > +++ b/arch/x86/kvm/kvm_emulate.h
> > @@ -322,9 +322,6 @@ struct x86_emulate_ctxt {
> >  	u8 intercept;
> >  	u8 op_bytes;
> >  	u8 ad_bytes;
> > -	struct operand src;
> > -	struct operand src2;
> > -	struct operand dst;
> >  	int (*execute)(struct x86_emulate_ctxt *ctxt);
> >  	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
> >  	/*
> > @@ -349,6 +346,11 @@ struct x86_emulate_ctxt {
> >  	u8 seg_override;
> >  	u64 d;
> >  	unsigned long _eip;
> > +
> > +	/* Here begins the usercopy section. */
> > +	struct operand src;
> > +	struct operand src2;
> > +	struct operand dst;
> 
> Out of pure curiosity, how certain are we that this is going to be
> enough for userspaces?

This is purely related to in-kernel hardening, there's no concern about
this being "enough" because it's not directly visible to userspace.

usercopy refers to the kernel copying data to/from user memory.  When
running with CONFIG_HARDENED_USERCOPY=y, copy_{to,from}_user performs
extra checks to warn if the kernel is doing something potentially
dangerous.  For this code specifically, it will warn if user data is
being copied into a struct and the affected range within the struct isn't
explicitly annotated as being safe for usercopy.  The idea is that the
kernel shouldn't be copying user data into variables that the kernel
expects are fully under its control.

Currently, KVM marks the entire struct x86_emulate_ctxt as being "safe"
for usercopy, which is sub-optimal because HARDENED_USERCOPY wouldn't be
able to detect KVM bugs, e.g. if "enum x86emul_mode mode" were overwritten
by copy_from_user().  Shuffling the emulator fields so that the all fields
that are used for usercopy are clustered together allows KVM to use a more
precise declaration for which fields are "safe", e.g. the theoretical @mode
bug would now be caught.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context
  2020-03-02 18:40     ` Paolo Bonzini
@ 2020-03-02 19:19       ` Sean Christopherson
  0 siblings, 0 replies; 39+ messages in thread
From: Sean Christopherson @ 2020-03-02 19:19 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On Mon, Mar 02, 2020 at 07:40:27PM +0100, Paolo Bonzini wrote:
> On 26/02/20 18:51, Vitaly Kuznetsov wrote:
> >> +
> >> +	/* Here begins the usercopy section. */
> >> +	struct operand src;
> >> +	struct operand src2;
> >> +	struct operand dst;
> > Out of pure curiosity, how certain are we that this is going to be
> > enough for userspaces?
> > 
> 
> And also, where exactly are the user copies done?

Anything that funnels into ctxt->ops->read_std() or ctxt->ops->write_std(),
e.g.

	if (ctxt->src2.type == OP_MEM) {
		rc = segmented_read(ctxt, ctxt->src2.addr.mem,
				    &ctxt->src2.val, ctxt->src2.bytes);
		if (rc != X86EMUL_CONTINUE)
			goto done;
	}


segmented_read() : @data = &ctxt->src2.val
|
|-> read_emulated()
    |
    |-> ctxt->ops->read_emulated() / emulator_read_emulated()
        |
        |-> emulator_read_write()
            |
            |-> emulator_read_write_onepage()
                |
                |-> ops->read_write_emulate() / read_emulate()
                    |
                    |-> kvm_vcpu_read_guest()
                        |
                        ...
                          |-> __kvm_read_guest_page()
                              |
                              |-> __copy_from_user(data, ...)

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator
  2020-03-02 18:42 ` [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Paolo Bonzini
@ 2020-03-02 20:02   ` Sean Christopherson
  0 siblings, 0 replies; 39+ messages in thread
From: Sean Christopherson @ 2020-03-02 20:02 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On Mon, Mar 02, 2020 at 07:42:31PM +0100, Paolo Bonzini wrote:
> On 19/02/20 00:29, Sean Christopherson wrote:
> > The primary intent of this series is to dynamically allocate the emulator
> > and get KVM to a state where the emulator *could* be disabled at some
> > point in the future.  Actually allowing userspace to disable the emulator
> > was a minor change at that point, so I threw it in.
> > 
> > Dynamically allocating the emulator shrinks the size of x86 vcpus by
> > ~2.5k bytes, which is important because 'struct vcpu_vmx' has once again
> > fattened up and squeaked past the PAGE_ALLOC_COSTLY_ORDER threshold.
> > Moving the emulator to its own allocation gives us some breathing room
> > for the near future, and has some other nice side effects.
> > 
> > As for disabling the emulator... in the not-too-distant future, I expect
> > there will be use cases that can truly disable KVM's emulator, e.g. for
> > security (KVM's and/or the guests).  I don't have a strong opinion on
> > whether or not KVM should actually allow userspace to disable the emulator
> > without a concrete use case (unless there already is a use case?), which
> > is why that part is done in its own tiny patch.
> > 
> > Running without an emulator has been "tested" in the sense that the
> > selftests that don't require emulation continue to pass, and everything
> > else fails with the expected "emulation error".
> 
> I agree with Vitaly that, if we want this, it should be a KVM_ENABLE_CAP
> instead.  The first 10 patches are very nice cleanups though so I plan
> to apply them (with Vitaly's suggested nits for review) after you answer
> the question on patch 10.

Works for me, thanks!

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context
  2020-02-26 17:29   ` Vitaly Kuznetsov
@ 2020-03-03 10:26     ` Paolo Bonzini
  2020-03-03 14:57       ` Sean Christopherson
  2020-03-03 16:52     ` Sean Christopherson
  1 sibling, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2020-03-03 10:26 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

On 26/02/20 18:29, Vitaly Kuznetsov wrote:
>>  struct x86_emulate_ctxt {
>> +	void *vcpu;
> Why 'void *'? I changed this to 'struct kvm_vcpu *' and it seems to
> compile just fine...
> 

I guess because it's really just an opaque pointer; using void* ensures
that the emulator doesn't break the emulator ops abstraction.

Paolo


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory
  2020-02-26 17:38   ` Vitaly Kuznetsov
@ 2020-03-03 10:27     ` Paolo Bonzini
  0 siblings, 0 replies; 39+ messages in thread
From: Paolo Bonzini @ 2020-03-03 10:27 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

On 26/02/20 18:38, Vitaly Kuznetsov wrote:
> Not this patchset's problem (and particular forward declaration is
> likely needed), but 
> 
> $ grep 'struct x86_emulate_ctxt' arch/x86/include/asm/kvm_host.h
> struct x86_emulate_ctxt;
> 	struct x86_emulate_ctxt *emulate_ctxt;
> struct x86_emulate_ctxt;
> 
> The second forward declaration is not needed and this patch (or
> patchset) may be a good place to get rid of it)
> 

Squashed, thanks.

Paolo


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context
  2020-03-03 10:26     ` Paolo Bonzini
@ 2020-03-03 14:57       ` Sean Christopherson
  2020-03-03 16:18         ` Vitaly Kuznetsov
  0 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-03-03 14:57 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On Tue, Mar 03, 2020 at 11:26:21AM +0100, Paolo Bonzini wrote:
> On 26/02/20 18:29, Vitaly Kuznetsov wrote:
> >>  struct x86_emulate_ctxt {
> >> +	void *vcpu;
> > Why 'void *'? I changed this to 'struct kvm_vcpu *' and it seems to
> > compile just fine...
> > 
> 
> I guess because it's really just an opaque pointer; using void* ensures
> that the emulator doesn't break the emulator ops abstraction.

Ya, it prevents the emulator from directly deferencing the vcpu.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context
  2020-03-03 14:57       ` Sean Christopherson
@ 2020-03-03 16:18         ` Vitaly Kuznetsov
  0 siblings, 0 replies; 39+ messages in thread
From: Vitaly Kuznetsov @ 2020-03-03 16:18 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> On Tue, Mar 03, 2020 at 11:26:21AM +0100, Paolo Bonzini wrote:
>> On 26/02/20 18:29, Vitaly Kuznetsov wrote:
>> >>  struct x86_emulate_ctxt {
>> >> +	void *vcpu;
>> > Why 'void *'? I changed this to 'struct kvm_vcpu *' and it seems to
>> > compile just fine...
>> > 
>> 
>> I guess because it's really just an opaque pointer; using void* ensures
>> that the emulator doesn't break the emulator ops abstraction.
>
> Ya, it prevents the emulator from directly deferencing the vcpu.
>

Makes sense, a comment like /* Should never be dereferenced by the
emulater */ would've helped)

-- 
Vitaly


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context
  2020-02-26 17:11   ` Vitaly Kuznetsov
@ 2020-03-03 16:48     ` Sean Christopherson
  2020-03-03 17:29       ` Paolo Bonzini
  0 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-03-03 16:48 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

On Wed, Feb 26, 2020 at 06:11:25PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > Explicitly pass the emulation context to the emulate tracepoint in
> > preparation of dynamically allocation the emulation context.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/trace.h | 22 +++++++++++-----------
> >  arch/x86/kvm/x86.c   | 13 ++++++++-----
> >  2 files changed, 19 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index f194dd058470..5605000ca5f6 100644
> > --- a/arch/x86/kvm/trace.h
> > +++ b/arch/x86/kvm/trace.h
> > @@ -731,8 +731,9 @@ TRACE_EVENT(kvm_skinit,
> >  	})
> >  
> >  TRACE_EVENT(kvm_emulate_insn,
> > -	TP_PROTO(struct kvm_vcpu *vcpu, __u8 failed),
> > -	TP_ARGS(vcpu, failed),
> > +	TP_PROTO(struct kvm_vcpu *vcpu, struct x86_emulate_ctxt *ctxt,
> > +		 __u8 failed),
> > +	TP_ARGS(vcpu, ctxt, failed),
> >  
> >  	TP_STRUCT__entry(
> >  		__field(    __u64, rip                       )
> > @@ -745,13 +746,10 @@ TRACE_EVENT(kvm_emulate_insn,
> >  
> >  	TP_fast_assign(
> >  		__entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS);
> 
> This seems the only usage of 'vcpu' parameter now; I checked and even
> after switching to dynamic emulation context allocation we still set
> ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter
> here then (and use ctxt->vcpu instead)?

Hmm, ya, not sure what I was thinking here.

> > -		__entry->len = vcpu->arch.emulate_ctxt.fetch.ptr
> > -			       - vcpu->arch.emulate_ctxt.fetch.data;
> > -		__entry->rip = vcpu->arch.emulate_ctxt._eip - __entry->len;
> > -		memcpy(__entry->insn,
> > -		       vcpu->arch.emulate_ctxt.fetch.data,
> > -		       15);
> > -		__entry->flags = kei_decode_mode(vcpu->arch.emulate_ctxt.mode);
> > +		__entry->len = ctxt->fetch.ptr - ctxt->fetch.data;
> > +		__entry->rip = ctxt->_eip - __entry->len;
> > +		memcpy(__entry->insn, ctxt->fetch.data, 15);
> > +		__entry->flags = kei_decode_mode(ctxt->mode);
> >  		__entry->failed = failed;
> >  		),
> >  
> > @@ -764,8 +762,10 @@ TRACE_EVENT(kvm_emulate_insn,
> >  		)
> >  	);
> >  
> > -#define trace_kvm_emulate_insn_start(vcpu) trace_kvm_emulate_insn(vcpu, 0)
> > -#define trace_kvm_emulate_insn_failed(vcpu) trace_kvm_emulate_insn(vcpu, 1)
> > +#define trace_kvm_emulate_insn_start(vcpu, ctxt)	\
> > +	trace_kvm_emulate_insn(vcpu, ctxt, 0)
> > +#define trace_kvm_emulate_insn_failed(vcpu, ctxt)	\
> > +	trace_kvm_emulate_insn(vcpu, ctxt, 1)
> >  
> >  TRACE_EVENT(
> >  	vcpu_match_mmio,

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context
  2020-02-26 17:29   ` Vitaly Kuznetsov
  2020-03-03 10:26     ` Paolo Bonzini
@ 2020-03-03 16:52     ` Sean Christopherson
  1 sibling, 0 replies; 39+ messages in thread
From: Sean Christopherson @ 2020-03-03 16:52 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

On Wed, Feb 26, 2020 at 06:29:56PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> > @@ -9409,6 +9451,9 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
> >  
> >  	kvm_x86_ops->vcpu_free(vcpu);
> >  
> > +	if (vcpu->arch.emulate_ctxt)
> > +		kmem_cache_free(x86_emulator_cache, vcpu->arch.emulate_ctxt);
> 
> Checking for NULL here seems superfluous as we create the context in
> kvm_arch_vcpu_create() unconditionally. I'd suggest we move the check to 
> "[PATCH v2 12/13] KVM: x86: Add variable to control existence of
> emulator" where 'enable_emulator' global is added.

Ya, checking here is premature.

> > +
> >  	free_cpumask_var(vcpu->arch.wbinvd_dirty_mask);
> >  	kmem_cache_free(x86_fpu_cache, vcpu->arch.user_fpu);
> >  	kmem_cache_free(x86_fpu_cache, vcpu->arch.guest_fpu);
> 
> -- 
> Vitaly
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context
  2020-03-03 16:48     ` Sean Christopherson
@ 2020-03-03 17:29       ` Paolo Bonzini
  2020-03-03 17:42         ` Sean Christopherson
  0 siblings, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2020-03-03 17:29 UTC (permalink / raw)
  To: Sean Christopherson, Vitaly Kuznetsov
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel

On 03/03/20 17:48, Sean Christopherson wrote:
>>>  	TP_fast_assign(
>>>  		__entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS);
>> This seems the only usage of 'vcpu' parameter now; I checked and even
>> after switching to dynamic emulation context allocation we still set
>> ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter
>> here then (and use ctxt->vcpu instead)?
> Hmm, ya, not sure what I was thinking here.
> 

As long as we have one use of vcpu, I'd rather skip this patch and
adjust patch 8 to use "->".  Even the other "explicitly take context"
parts are kinda debatable since you still have to do emul_to_vcpu.
Throwing a handful of

- 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
+ 	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;

into patch 8 is not bad at all and limits the churn.

Paolo


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context
  2020-03-03 17:29       ` Paolo Bonzini
@ 2020-03-03 17:42         ` Sean Christopherson
  2020-03-03 17:44           ` Paolo Bonzini
  0 siblings, 1 reply; 39+ messages in thread
From: Sean Christopherson @ 2020-03-03 17:42 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On Tue, Mar 03, 2020 at 06:29:30PM +0100, Paolo Bonzini wrote:
> On 03/03/20 17:48, Sean Christopherson wrote:
> >>>  	TP_fast_assign(
> >>>  		__entry->csbase = kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS);
> >> This seems the only usage of 'vcpu' parameter now; I checked and even
> >> after switching to dynamic emulation context allocation we still set
> >> ctxt->vcpu in alloc_emulate_ctxt(), can we get rid of 'vcpu' parameter
> >> here then (and use ctxt->vcpu instead)?
> > Hmm, ya, not sure what I was thinking here.
> > 
> 
> As long as we have one use of vcpu, I'd rather skip this patch and
> adjust patch 8 to use "->".  Even the other "explicitly take context"
> parts are kinda debatable since you still have to do emul_to_vcpu.
> Throwing a handful of
> 
> - 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> + 	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
> 
> into patch 8 is not bad at all and limits the churn.

Hmm, I'd prefer to explicitly pass @ctxt, even for the tracepoint.  I get
that it's technically unnecessary churn, but explicitly passing @ctxt means
that every funcition that grabs arch.emulate_ctxt (all three of 'em) checks
for a NULL ctxt.  That makes it trivial to visually audit that there's no
risk of a bad pointer dereference, and IMO having @ctxt in the prototype
is helpful to see "oh, this helper is called from within the emulator".

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context
  2020-03-03 17:42         ` Sean Christopherson
@ 2020-03-03 17:44           ` Paolo Bonzini
  0 siblings, 0 replies; 39+ messages in thread
From: Paolo Bonzini @ 2020-03-03 17:44 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On 03/03/20 18:42, Sean Christopherson wrote:
>> As long as we have one use of vcpu, I'd rather skip this patch and
>> adjust patch 8 to use "->".  Even the other "explicitly take context"
>> parts are kinda debatable since you still have to do emul_to_vcpu.
>> Throwing a handful of
>>
>> - 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
>> + 	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>>
>> into patch 8 is not bad at all and limits the churn.
> Hmm, I'd prefer to explicitly pass @ctxt, even for the tracepoint.  I get
> that it's technically unnecessary churn, but explicitly passing @ctxt means
> that every funcition that grabs arch.emulate_ctxt (all three of 'em) checks
> for a NULL ctxt.  That makes it trivial to visually audit that there's no
> risk of a bad pointer dereference, and IMO having @ctxt in the prototype
> is helpful to see "oh, this helper is called from within the emulator".
> 

That's a good rationale, but I believe this refactoring belongs more in
the "disable emulator" part than this one.

Paolo


^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2020-03-03 17:44 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 23:29 [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Sean Christopherson
2020-02-18 23:29 ` [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant Sean Christopherson
2020-02-26 15:16   ` Vitaly Kuznetsov
2020-02-18 23:29 ` [PATCH v2 02/13] KVM: x86: Explicitly pass an exception struct to check_intercept Sean Christopherson
2020-02-18 23:29 ` [PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c Sean Christopherson
2020-02-26 15:23   ` Vitaly Kuznetsov
2020-02-18 23:29 ` [PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context Sean Christopherson
2020-02-26 15:24   ` Vitaly Kuznetsov
2020-02-18 23:29 ` [PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context Sean Christopherson
2020-02-26 15:25   ` Vitaly Kuznetsov
2020-02-18 23:29 ` [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context Sean Christopherson
2020-02-26 17:11   ` Vitaly Kuznetsov
2020-03-03 16:48     ` Sean Christopherson
2020-03-03 17:29       ` Paolo Bonzini
2020-03-03 17:42         ` Sean Christopherson
2020-03-03 17:44           ` Paolo Bonzini
2020-02-18 23:29 ` [PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() " Sean Christopherson
2020-02-26 17:13   ` Vitaly Kuznetsov
2020-02-18 23:29 ` [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context Sean Christopherson
2020-02-26 17:29   ` Vitaly Kuznetsov
2020-03-03 10:26     ` Paolo Bonzini
2020-03-03 14:57       ` Sean Christopherson
2020-03-03 16:18         ` Vitaly Kuznetsov
2020-03-03 16:52     ` Sean Christopherson
2020-02-18 23:29 ` [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory Sean Christopherson
2020-02-26 17:38   ` Vitaly Kuznetsov
2020-03-03 10:27     ` Paolo Bonzini
2020-02-18 23:29 ` [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context Sean Christopherson
2020-02-26 17:51   ` Vitaly Kuznetsov
2020-03-02 18:40     ` Paolo Bonzini
2020-03-02 19:19       ` Sean Christopherson
2020-03-02 19:13     ` Sean Christopherson
2020-02-18 23:29 ` [PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error Sean Christopherson
2020-02-26 17:52   ` Vitaly Kuznetsov
2020-02-18 23:29 ` [PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator Sean Christopherson
2020-02-26 18:01   ` Vitaly Kuznetsov
2020-02-18 23:29 ` [PATCH v2 13/13] KVM: x86: Allow userspace to disable the kernel's emulator Sean Christopherson
2020-03-02 18:42 ` [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator Paolo Bonzini
2020-03-02 20:02   ` Sean Christopherson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.