Re: [kvm-unit-tests PATCH v4 3/3] x86: Add test coverage for nested_vmx_reflect_vmexit() testing

From: Sean Christopherson <seanjc@google.com>
To: Aaron Lewis <aaronlewis@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com
Subject: Re: [kvm-unit-tests PATCH v4 3/3] x86: Add test coverage for nested_vmx_reflect_vmexit() testing
Date: Fri, 21 Jan 2022 18:00:00 +0000	[thread overview]
Message-ID: <Yer0oCazOfKXs4t3@google.com> (raw)
In-Reply-To: <20220121155855.213852-4-aaronlewis@google.com>

On Fri, Jan 21, 2022, Aaron Lewis wrote:
> Add a framework and test cases to ensure exceptions that occur in L2 are
> forwarded to the correct place by nested_vmx_reflect_vmexit().
> 
> Add testing for exceptions: #GP, #UD, #DE, #DB, #BP, and #AC.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> Change-Id: I0196071571671f06165983b5055ed7382fa3e1fb

Don't forget to strip the Change-Id before posting.

> ---
>  x86/unittests.cfg |   9 +++-
>  x86/vmx_tests.c   | 129 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 137 insertions(+), 1 deletion(-)
> 
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 9a70ba3..6ec7a98 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -288,7 +288,7 @@ arch = i386
>  
>  [vmx]
>  file = vmx.flat
> -extra_params = -cpu max,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_test -atomic_switch_overflow_msrs_test -vmx_init_signal_test -vmx_apic_passthrough_tpr_threshold_test -apic_reg_virt_test -virt_x2apic_mode_test -vmx_pf_exception_test -vmx_pf_no_vpid_test -vmx_pf_invvpid_test -vmx_pf_vpid_test"
> +extra_params = -cpu max,+vmx -append "-exit_monitor_from_l2_test -ept_access* -vmx_smp* -vmx_vmcs_shadow_test -atomic_switch_overflow_msrs_test -vmx_init_signal_test -vmx_apic_passthrough_tpr_threshold_test -apic_reg_virt_test -virt_x2apic_mode_test -vmx_pf_exception_test -vmx_pf_no_vpid_test -vmx_pf_invvpid_test -vmx_pf_vpid_test -vmx_exception_test"
>  arch = x86_64
>  groups = vmx
>  
> @@ -390,6 +390,13 @@ arch = x86_64
>  groups = vmx nested_exception
>  check = /sys/module/kvm_intel/parameters/allow_smaller_maxphyaddr=Y
>  
> +[vmx_exception_test]
> +file = vmx.flat
> +extra_params = -cpu max,+vmx -append vmx_exception_test
> +arch = x86_64
> +groups = vmx nested_exception
> +timeout = 10

Leave this out (for now), including it in the main "vmx" test is sufficient.
I'm definitely in favor of splitting up the "vmx" behemoth, but it's probably
best to do that in a separate commit/series so that we can waste time bikeshedding
over how to organize things :-)

> +
>  [debug]
>  file = debug.flat
>  arch = x86_64
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 3d57ed6..af6f33b 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -21,6 +21,7 @@
>  #include "smp.h"
>  #include "delay.h"
>  #include "access.h"
> +#include "x86/usermode.h"
>  
>  #define VPID_CAP_INVVPID_TYPES_SHIFT 40
>  
> @@ -10701,6 +10702,133 @@ static void vmx_pf_vpid_test(void)
>  	__vmx_pf_vpid_test(invalidate_tlb_new_vpid, 1);
>  }
>  
> +static void vmx_l2_gp_test(void)
> +{
> +	*(volatile u64 *)NONCANONICAL = 0;
> +}
> +
> +static void vmx_l2_ud_test(void)
> +{
> +	asm volatile ("ud2");
> +}
> +
> +static void vmx_l2_de_test(void)
> +{
> +	asm volatile (
> +		"xor %%eax, %%eax\n\t"
> +		"xor %%ebx, %%ebx\n\t"
> +		"xor %%edx, %%edx\n\t"
> +		"idiv %%ebx\n\t"
> +		::: "eax", "ebx", "edx");
> +}
> +
> +static void vmx_l2_bp_test(void)
> +{
> +	asm volatile ("int3");
> +}
> +
> +static void vmx_l2_db_test(void)
> +{
> +	write_rflags(read_rflags() | X86_EFLAGS_TF);
> +}
> +
> +static uint64_t usermode_callback(void)
> +{
> +	/* Trigger an #AC by writing 8 bytes to a 4-byte aligned address. */
> +	asm volatile(
> +		"sub $0x10, %rsp\n\t"
> +		"movq $0, 0x4(%rsp)\n\t"
> +		"add $0x10, %rsp\n\t");

Sorry, didn't look closely at this before.  This can simply be:

	asm volatile("movq $0, 0x4(%rsp)\n\t");

as the access is expected to fault.  Or if you want to be paranoid about not
overwriting the stack:

	asm volatile("movq $0, -0x4(%rsp)\n\t");

It's probably also a good idea to call out that the stack is aligned on a 16-byte
boundary.  If you were trying to guarnatee alignment, then you would need to use
AND instead of SUB.  E.g.

        asm volatile("push  %rbp\n\t"
                     "movq  %rsp, %rbp\n\t"
                     "andq  $-0x10, %rsp\n\t"
                     "movq  $0, -0x4(%rsp)\n\t"
                     "movq  %rbp, %rsp\n\t"
                     "popq  %rbp\n\t");

But my vote would be to just add a comment, I would consider it a test bug if the
stack isn't properly aligned.

> +
> +	return 0;
> +}
> +
> +static void vmx_l2_ac_test(void)
> +{
> +	bool raised_vector = false;

Nit, hit_ac or so is more intuive.

> +
> +	write_cr0(read_cr0() | X86_CR0_AM);
> +	write_rflags(read_rflags() | X86_EFLAGS_AC);
> +
> +	run_in_user(usermode_callback, AC_VECTOR, 0, 0, 0, 0, &raised_vector);
> +	report(raised_vector, "#AC vector raised from usermode in L2");

And continuing the nits, "Usermode #AC handled in L2".

> +	vmcall();
> +}
> +