[PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator

[PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator

[Sean Christopherson]

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

[PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant

[Sean Christopherson]

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

Re: [PATCH v2 01/13] KVM: x86: Refactor I/O emulation helpers to provide vcpu-only variant

[Vitaly Kuznetsov]

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

[PATCH v2 02/13] KVM: x86: Explicitly pass an exception struct to check_intercept

[Sean Christopherson]

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

[PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c

[Sean Christopherson]

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

Re: [PATCH v2 03/13] KVM: x86: Move emulation-only helpers to emulate.c

[Vitaly Kuznetsov]

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

[PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context

[Sean Christopherson]

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

Re: [PATCH v2 04/13] KVM: x86: Refactor R/W page helper to take the emulation context

[Vitaly Kuznetsov]

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

[PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context

[Sean Christopherson]

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

Re: [PATCH v2 05/13] KVM: x86: Refactor emulated exception injection to take the emul context

[Vitaly Kuznetsov]

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

[PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context

[Sean Christopherson]

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

Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context

[Vitaly Kuznetsov]

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

Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context

[Sean Christopherson]

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,

Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context

[Paolo Bonzini]

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

Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context

[Sean Christopherson]

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".

Re: [PATCH v2 06/13] KVM: x86: Refactor emulate tracepoint to explicitly take context

[Paolo Bonzini]

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

[PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() to explicitly take context

[Sean Christopherson]

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

Re: [PATCH v2 07/13] KVM: x86: Refactor init_emulate_ctxt() to explicitly take context

[Vitaly Kuznetsov]

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

[PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context

[Sean Christopherson]

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

Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context

[Vitaly Kuznetsov]

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

Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context

[Paolo Bonzini]

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

Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context

[Sean Christopherson]

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.

Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context

[Vitaly Kuznetsov]

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

Re: [PATCH v2 08/13] KVM: x86: Dynamically allocate per-vCPU emulation context

[Sean Christopherson]

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
> 

[PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory

[Sean Christopherson]

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

Re: [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory

[Vitaly Kuznetsov]

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

Re: [PATCH v2 09/13] KVM: x86: Move kvm_emulate.h into KVM's private directory

[Paolo Bonzini]

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

[PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context

[Sean Christopherson]

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

Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context

[Vitaly Kuznetsov]

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

Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context

[Paolo Bonzini]

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

Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context

[Sean Christopherson]

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, ...)

Re: [PATCH v2 10/13] KVM: x86: Shrink the usercopy region of the emulation context

[Sean Christopherson]

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.

[PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error

[Sean Christopherson]

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

Re: [PATCH v2 11/13] KVM: x86: Add helper to "handle" internal emulation error

[Vitaly Kuznetsov]

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

[PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator

[Sean Christopherson]

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

Re: [PATCH v2 12/13] KVM: x86: Add variable to control existence of emulator

[Vitaly Kuznetsov]

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

[PATCH v2 13/13] KVM: x86: Allow userspace to disable the kernel's emulator

[Sean Christopherson]

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

Re: [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator

[Paolo Bonzini]

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%)
> 

Re: [PATCH v2 00/13] KVM: x86: Allow userspace to disable the emulator

[Sean Christopherson]

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!