diff mbox series

[09/14] KVM: selftests: Fix vmxon_pa == vmcs12_pa == -1ull vmx_set_nested_state_test for !eVMCS case

Message ID 20231025152406.1879274-10-vkuznets@redhat.com
State New
Headers
Series KVM: x86: Make Hyper-V emulation optional |

Commit Message

Vitaly Kuznetsov Oct. 25, 2023, 3:24 p.m. UTC 
  The "vmxon_pa == vmcs12_pa == -1ull" test happens to work by accident: as
Enlightened VMCS is always supported, set_default_vmx_state() adds
'KVM_STATE_NESTED_EVMCS' to 'flags' and the following branch of
vmx_set_nested_state() is executed:

        if ((kvm_state->flags & KVM_STATE_NESTED_EVMCS) &&
            (!guest_can_use(vcpu, X86_FEATURE_VMX) ||
             !vmx->nested.enlightened_vmcs_enabled))
                        return -EINVAL;

as 'enlightened_vmcs_enabled' is false. In fact, "vmxon_pa == vmcs12_pa ==
-1ull" is a valid state when not tainted by wrong flags so the test should
aim for this branch:

        if (kvm_state->hdr.vmx.vmxon_pa == INVALID_GPA)
                return 0;

Test all this properly:
- Without KVM_STATE_NESTED_EVMCS in the flags, the expected return value is
'0'.
- With KVM_STATE_NESTED_EVMCS flag (when supported) set, the expected
return value is '-EINVAL' prior to enabling eVMCS and '0' after.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 .../kvm/x86_64/vmx_set_nested_state_test.c       | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Maxim Levitsky Nov. 7, 2023, 6:23 p.m. UTC | #1 
On Wed, 2023-10-25 at 17:24 +0200, Vitaly Kuznetsov wrote:
> The "vmxon_pa == vmcs12_pa == -1ull" test happens to work by accident: as
> Enlightened VMCS is always supported, set_default_vmx_state() adds
> 'KVM_STATE_NESTED_EVMCS' to 'flags' and the following branch of
> vmx_set_nested_state() is executed:
> 
>         if ((kvm_state->flags & KVM_STATE_NESTED_EVMCS) &&
>             (!guest_can_use(vcpu, X86_FEATURE_VMX) ||
>              !vmx->nested.enlightened_vmcs_enabled))
>                         return -EINVAL;
> 
> as 'enlightened_vmcs_enabled' is false. In fact, "vmxon_pa == vmcs12_pa ==
> -1ull" is a valid state when not tainted by wrong flags so the test should
> aim for this branch:
> 
>         if (kvm_state->hdr.vmx.vmxon_pa == INVALID_GPA)
>                 return 0;
> 
> Test all this properly:
> - Without KVM_STATE_NESTED_EVMCS in the flags, the expected return value is
> '0'.
> - With KVM_STATE_NESTED_EVMCS flag (when supported) set, the expected
> return value is '-EINVAL' prior to enabling eVMCS and '0' after.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  .../kvm/x86_64/vmx_set_nested_state_test.c       | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
> index 41ea7028a1f8..67a62a5a8895 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
> @@ -125,21 +125,25 @@ void test_vmx_nested_state(struct kvm_vcpu *vcpu)
>  
>  	/*
>  	 * Setting vmxon_pa == -1ull and vmcs_pa == -1ull exits early without
> -	 * setting the nested state but flags other than eVMCS must be clear.
> -	 * The eVMCS flag can be set if the enlightened VMCS capability has
> -	 * been enabled.
> +	 * setting the nested state. When the eVMCS flag is not set, the
> +	 * expected return value is '0'.
>  	 */
>  	set_default_vmx_state(state, state_sz);
> +	state->flags = 0;
>  	state->hdr.vmx.vmxon_pa = -1ull;
>  	state->hdr.vmx.vmcs12_pa = -1ull;
> -	test_nested_state_expect_einval(vcpu, state);
> +	test_nested_state(vcpu, state);
>  
> -	state->flags &= KVM_STATE_NESTED_EVMCS;
> +	/*
> +	 * When eVMCS is supported, the eVMCS flag can only be set if the
> +	 * enlightened VMCS capability has been enabled.
> +	 */
>  	if (have_evmcs) {
> +		state->flags = KVM_STATE_NESTED_EVMCS;
>  		test_nested_state_expect_einval(vcpu, state);
>  		vcpu_enable_evmcs(vcpu);
> +		test_nested_state(vcpu, state);
>  	}
> -	test_nested_state(vcpu, state);
>  
>  	/* It is invalid to have vmxon_pa == -1ull and SMM flags non-zero. */
>  	state->hdr.vmx.smm.flags = 1;


Makes sense, but I am not 100% sure that I haven't missed something.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
index 41ea7028a1f8..67a62a5a8895 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
@@ -125,21 +125,25 @@  void test_vmx_nested_state(struct kvm_vcpu *vcpu)
 
 	/*
 	 * Setting vmxon_pa == -1ull and vmcs_pa == -1ull exits early without
-	 * setting the nested state but flags other than eVMCS must be clear.
-	 * The eVMCS flag can be set if the enlightened VMCS capability has
-	 * been enabled.
+	 * setting the nested state. When the eVMCS flag is not set, the
+	 * expected return value is '0'.
 	 */
 	set_default_vmx_state(state, state_sz);
+	state->flags = 0;
 	state->hdr.vmx.vmxon_pa = -1ull;
 	state->hdr.vmx.vmcs12_pa = -1ull;
-	test_nested_state_expect_einval(vcpu, state);
+	test_nested_state(vcpu, state);
 
-	state->flags &= KVM_STATE_NESTED_EVMCS;
+	/*
+	 * When eVMCS is supported, the eVMCS flag can only be set if the
+	 * enlightened VMCS capability has been enabled.
+	 */
 	if (have_evmcs) {
+		state->flags = KVM_STATE_NESTED_EVMCS;
 		test_nested_state_expect_einval(vcpu, state);
 		vcpu_enable_evmcs(vcpu);
+		test_nested_state(vcpu, state);
 	}
-	test_nested_state(vcpu, state);
 
 	/* It is invalid to have vmxon_pa == -1ull and SMM flags non-zero. */
 	state->hdr.vmx.smm.flags = 1;