[PATCH] KVM: VMX: Do not trap VMFUNC instructions for L1 guests.

[PATCH] KVM: VMX: Do not trap VMFUNC instructions for L1 guests.

[Yu Zhang]

VMFUNC is not supported for L1 guests, and executing VMFUNC in
L1 shall generate a #UD directly. Just disable it in secondary
proc-based execution control for L1, instead of intercepting it
and inject the #UD again.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
 arch/x86/kvm/vmx/nested.c | 17 +++++------------
 arch/x86/kvm/vmx/vmx.c    |  4 +++-
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0c62352dda6a..8858c6c0979f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5793,11 +5793,11 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
 	u32 function = kvm_rax_read(vcpu);
 
 	/*
-	 * VMFUNC is only supported for nested guests, but we always enable the
-	 * secondary control for simplicity; for non-nested mode, fake that we
-	 * didn't by injecting #UD.
+	 * VMFUNC is only supported for nested guests, instead of triggering
+	 * a VM Exit, non-nested guests shall receive #UD directly.
 	 */
 	if (!is_guest_mode(vcpu)) {
+		vcpu_unimpl(vcpu, "vmx: unexpected vm exit EXIT_REASON_VMFUNC.\n");
 		kvm_queue_exception(vcpu, UD_VECTOR);
 		return 1;
 	}
@@ -6808,6 +6808,7 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
 		SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
 		SECONDARY_EXEC_RDRAND_EXITING |
 		SECONDARY_EXEC_ENABLE_INVPCID |
+		SECONDARY_EXEC_ENABLE_VMFUNC |
 		SECONDARY_EXEC_RDSEED_EXITING |
 		SECONDARY_EXEC_XSAVES |
 		SECONDARY_EXEC_TSC_SCALING;
@@ -6839,16 +6840,8 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
 				SECONDARY_EXEC_ENABLE_PML;
 			msrs->ept_caps |= VMX_EPT_AD_BIT;
 		}
-	}
 
-	if (cpu_has_vmx_vmfunc()) {
-		msrs->secondary_ctls_high |=
-			SECONDARY_EXEC_ENABLE_VMFUNC;
-		/*
-		 * Advertise EPTP switching unconditionally
-		 * since we emulate it
-		 */
-		if (enable_ept)
+		if (cpu_has_vmx_vmfunc())
 			msrs->vmfunc_controls =
 				VMX_VMFUNC_EPTP_SWITCHING;
 	}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 65f092e4a81b..9e17de62eb37 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4483,6 +4483,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
 	exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
 
+	/* VMFUNC is not supported for L1 guest, just disable it. */
+	exec_control &= ~SECONDARY_EXEC_ENABLE_VMFUNC;
+
 	/* SECONDARY_EXEC_DESC is enabled/disabled on writes to CR4.UMIP,
 	 * in vmx_set_cr4.  */
 	exec_control &= ~SECONDARY_EXEC_DESC;
@@ -6000,7 +6003,6 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[EXIT_REASON_RDSEED]                  = kvm_handle_invalid_op,
 	[EXIT_REASON_PML_FULL]		      = handle_pml_full,
 	[EXIT_REASON_INVPCID]                 = handle_invpcid,
-	[EXIT_REASON_VMFUNC]		      = handle_vmx_instruction,
 	[EXIT_REASON_PREEMPTION_TIMER]	      = handle_preemption_timer,
 	[EXIT_REASON_ENCLS]		      = handle_encls,
 	[EXIT_REASON_BUS_LOCK]                = handle_bus_lock_vmexit,
-- 
2.17.1

Re: [PATCH] KVM: VMX: Do not trap VMFUNC instructions for L1 guests.

[Paolo Bonzini]

On 11/7/22 09:27, Yu Zhang wrote:
> VMFUNC is not supported for L1 guests, and executing VMFUNC in
> L1 shall generate a #UD directly. Just disable it in secondary
> proc-based execution control for L1, instead of intercepting it
> and inject the #UD again.
> 
> Signed-off-by: Yu Zhang<yu.c.zhang@linux.intel.com>

Is this for TDX or similar?  The reason for a patch should be mentioned 
in the commit message.

Paolo

Re: [PATCH] KVM: VMX: Do not trap VMFUNC instructions for L1 guests.

[Sean Christopherson]

On Mon, Nov 07, 2022, Paolo Bonzini wrote:
> On 11/7/22 09:27, Yu Zhang wrote:
> > VMFUNC is not supported for L1 guests, and executing VMFUNC in
> > L1 shall generate a #UD directly. Just disable it in secondary
> > proc-based execution control for L1, instead of intercepting it
> > and inject the #UD again.
> > 
> > Signed-off-by: Yu Zhang<yu.c.zhang@linux.intel.com>
> 
> Is this for TDX or similar?  The reason for a patch should be mentioned in
> the commit message.

It's just a cleanup, but (a) it should be split over two patches as disabling
VMFUNC for L1 is technically a functional change, where as the changes to
nested_vmx_setup_ctls_msrs() are pure cleanups, and (b) the !guest_mode path in
handle_vmfunc() should either be removed or turned into a KVM_BUG_ON().

E.g.

---
 arch/x86/kvm/vmx/nested.c | 11 ++---------
 arch/x86/kvm/vmx/vmx.c    |  7 ++++++-
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0c62352dda6a..fa4130361187 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5792,15 +5792,8 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
 	struct vmcs12 *vmcs12;
 	u32 function = kvm_rax_read(vcpu);
 
-	/*
-	 * VMFUNC is only supported for nested guests, but we always enable the
-	 * secondary control for simplicity; for non-nested mode, fake that we
-	 * didn't by injecting #UD.
-	 */
-	if (!is_guest_mode(vcpu)) {
-		kvm_queue_exception(vcpu, UD_VECTOR);
-		return 1;
-	}
+	if (KVM_BUG_ON(!is_guest_mode(vcpu), vcpu->kvm))
+		return -EIO;
 
 	vmcs12 = get_vmcs12(vcpu);
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 63247c57c72c..5a66c3c16c2d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4487,6 +4487,12 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
 	exec_control &= ~SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
 
+	/*
+	 * KVM doesn't support VMFUNC for L1, but the control is set in KVM's
+	 * base configuration as KVM emulates VMFUNC[EPTP_SWITCHING] for L2.
+	 */
+	exec_control &= ~SECONDARY_EXEC_ENABLE_VMFUNC;
+
 	/* SECONDARY_EXEC_DESC is enabled/disabled on writes to CR4.UMIP,
 	 * in vmx_set_cr4.  */
 	exec_control &= ~SECONDARY_EXEC_DESC;
@@ -6004,7 +6010,6 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[EXIT_REASON_RDSEED]                  = kvm_handle_invalid_op,
 	[EXIT_REASON_PML_FULL]		      = handle_pml_full,
 	[EXIT_REASON_INVPCID]                 = handle_invpcid,
-	[EXIT_REASON_VMFUNC]		      = handle_vmx_instruction,
 	[EXIT_REASON_PREEMPTION_TIMER]	      = handle_preemption_timer,
 	[EXIT_REASON_ENCLS]		      = handle_encls,
 	[EXIT_REASON_BUS_LOCK]                = handle_bus_lock_vmexit,

base-commit: 07341b10fcbd5a7ef18225e0e9a8a40d91e3a2cc
-- 


and then the pure cleanup that is made possible because KVM now does:

	msrs->secondary_ctls_high = vmcs_conf->cpu_based_2nd_exec_ctrl;

---
 arch/x86/kvm/vmx/nested.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index fa4130361187..981bf5b3a319 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6801,6 +6801,7 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
 		SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
 		SECONDARY_EXEC_RDRAND_EXITING |
 		SECONDARY_EXEC_ENABLE_INVPCID |
+		SECONDARY_EXEC_ENABLE_VMFUNC |
 		SECONDARY_EXEC_RDSEED_EXITING |
 		SECONDARY_EXEC_XSAVES |
 		SECONDARY_EXEC_TSC_SCALING;
@@ -6832,18 +6833,13 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
 				SECONDARY_EXEC_ENABLE_PML;
 			msrs->ept_caps |= VMX_EPT_AD_BIT;
 		}
-	}
 
-	if (cpu_has_vmx_vmfunc()) {
-		msrs->secondary_ctls_high |=
-			SECONDARY_EXEC_ENABLE_VMFUNC;
 		/*
-		 * Advertise EPTP switching unconditionally
-		 * since we emulate it
+		 * Advertise EPTP switching irrespective of hardware support,
+		 * KVM emulates it in software so long as VMFUNC is supported.
 		 */
-		if (enable_ept)
-			msrs->vmfunc_controls =
-				VMX_VMFUNC_EPTP_SWITCHING;
+		if (cpu_has_vmx_vmfunc())
+			msrs->vmfunc_controls = VMX_VMFUNC_EPTP_SWITCHING;
 	}
 
 	/*

base-commit: 777dde94dd5e4328b419dcc5cb7118b39588eab1
-- 

Re: [PATCH] KVM: VMX: Do not trap VMFUNC instructions for L1 guests.

[Yu Zhang]

On Mon, Nov 07, 2022 at 05:36:21PM +0000, Sean Christopherson wrote:
> On Mon, Nov 07, 2022, Paolo Bonzini wrote:
> > On 11/7/22 09:27, Yu Zhang wrote:
> > > VMFUNC is not supported for L1 guests, and executing VMFUNC in
> > > L1 shall generate a #UD directly. Just disable it in secondary
> > > proc-based execution control for L1, instead of intercepting it
> > > and inject the #UD again.
> > > 
> > > Signed-off-by: Yu Zhang<yu.c.zhang@linux.intel.com>
> > 
> > Is this for TDX or similar?  The reason for a patch should be mentioned in
> > the commit message.
> 
> It's just a cleanup, but (a) it should be split over two patches as disabling
> VMFUNC for L1 is technically a functional change, where as the changes to
> nested_vmx_setup_ctls_msrs() are pure cleanups, and (b) the !guest_mode path in
> handle_vmfunc() should either be removed or turned into a KVM_BUG_ON().
> 

Got it. Will do in V2. And thanks!

B.R.
Yu

Re: [PATCH] KVM: VMX: Do not trap VMFUNC instructions for L1 guests.

[Yu Zhang]

On Mon, Nov 07, 2022 at 06:20:23PM +0100, Paolo Bonzini wrote:
> On 11/7/22 09:27, Yu Zhang wrote:
> > VMFUNC is not supported for L1 guests, and executing VMFUNC in
> > L1 shall generate a #UD directly. Just disable it in secondary
> > proc-based execution control for L1, instead of intercepting it
> > and inject the #UD again.
> > 
> > Signed-off-by: Yu Zhang<yu.c.zhang@linux.intel.com>
> 
> Is this for TDX or similar?  The reason for a patch should be mentioned in
> the commit message.

Thanks for your quick reply, Paolo. 

It is not a new feature. Just a clean up for VMFUNC, which is not
supported by KVM for L1 guest.

According to Intel SDM 25.5.6.2 - "General Operation of the VMFUNC
Instruction", The VMFUNC instruction causes an invalid-opcode exception
(#UD) if the “enable VM functions” VM-execution controls is 0 or the
value of EAX is greater than 63 (only VM functions 0–63 can be enable).
Otherwise, the instruction causes a VM exit if the bit at position
EAX is 0 in the VM-function controls (the selected VM function is not
enabled)

And since KVM only provides emulation of VMFUNC for nested guests,
it is uncessary for KVM to intercept it and reinject a #UD. So just
disable VMFUNC in VM-execution control for L1 guests.

But please feel free to educate me if I missed some backgrounds about
why this is enabled in the first place. Thanks!

B.R.
Yu