Re: [kvm-unit-tests PATCH 07/16] x86: Add a simple test for SYSENTER instruction.

[Sean Christopherson <seanjc@google.com>]

On Thu, Oct 20, 2022, Maxim Levitsky wrote:
> Run the test with Intel's vendor ID and in the long mode,
> to test the emulation of this instruction on AMD.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  x86/Makefile.x86_64 |   2 +
>  x86/sysenter.c      | 127 ++++++++++++++++++++++++++++++++++++++++++++
>  x86/unittests.cfg   |   5 ++
>  3 files changed, 134 insertions(+)
>  create mode 100644 x86/sysenter.c
> 
> diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
> index 865da07d..8ce53650 100644
> --- a/x86/Makefile.x86_64
> +++ b/x86/Makefile.x86_64
> @@ -33,6 +33,7 @@ tests += $(TEST_DIR)/vmware_backdoors.$(exe)
>  tests += $(TEST_DIR)/rdpru.$(exe)
>  tests += $(TEST_DIR)/pks.$(exe)
>  tests += $(TEST_DIR)/pmu_lbr.$(exe)
> +tests += $(TEST_DIR)/sysenter.$(exe)
>  
>  
>  ifeq ($(CONFIG_EFI),y)
> @@ -60,3 +61,4 @@ $(TEST_DIR)/hyperv_clock.$(bin): $(TEST_DIR)/hyperv_clock.o
>  $(TEST_DIR)/vmx.$(bin): $(TEST_DIR)/vmx_tests.o
>  $(TEST_DIR)/svm.$(bin): $(TEST_DIR)/svm_tests.o
>  $(TEST_DIR)/svm_npt.$(bin): $(TEST_DIR)/svm_npt.o
> +$(TEST_DIR)/sysenter.o: CFLAGS += -Wa,-mintel64
> diff --git a/x86/sysenter.c b/x86/sysenter.c
> new file mode 100644
> index 00000000..6c32fea4
> --- /dev/null
> +++ b/x86/sysenter.c
> @@ -0,0 +1,127 @@
> +#include "alloc.h"
> +#include "libcflat.h"
> +#include "processor.h"
> +#include "msr.h"
> +#include "desc.h"
> +
> +
> +// undefine this to run the syscall instruction in 64 bit mode.
> +// this won't work on AMD due to disabled code in the emulator.
> +#define COMP32

Why not run the test in both 32-bit and 64-bit mode, and skip the 64-bit mode
version if the vCPU model is AMD?

> +
> +int main(int ac, char **av)
> +{
> +    extern void sysenter_target(void);
> +    extern void test_done(void);

Tabs instead of spaces.

> +
> +    setup_vm();
> +
> +    int gdt_index = 0x50 >> 3;
> +    ulong rax = 0xDEAD;
> +
> +    /* init the sysenter GDT block */
> +    /*gdt64[gdt_index+0] = gdt64[KERNEL_CS >> 3];
> +    gdt64[gdt_index+1] = gdt64[KERNEL_DS >> 3];
> +    gdt64[gdt_index+2] = gdt64[USER_CS >> 3];
> +    gdt64[gdt_index+3] = gdt64[USER_DS >> 3];*/
> +
> +    /* init the sysenter msrs*/
> +    wrmsr(MSR_IA32_SYSENTER_CS, gdt_index << 3);
> +    wrmsr(MSR_IA32_SYSENTER_ESP, 0xAAFFFFFFFF);
> +    wrmsr(MSR_IA32_SYSENTER_EIP, (uint64_t)sysenter_target);
> +
> +    u8 *thunk = (u8*)malloc(50);
> +    u8 *tmp = thunk;
> +
> +    printf("Thunk at 0x%lx\n", (u64)thunk);
> +
> +    /* movabs test_done, %rdx*/
> +    *tmp++ = 0x48; *tmp++ = 0xBA;
> +    *(u64 *)tmp = (uint64_t)test_done; tmp += 8;
> +    /* jmp %%rdx*/
> +    *tmp++ = 0xFF; *tmp++ = 0xe2;
> +
> +    asm volatile (

Can we add a helper sysenter_asm.S or whatever instead of making this a gigantic
inline asm blob?  And then have separate routines for 32-bit vs. 64-bit?  That'd
require a bit of code duplication, but macros could be used to dedup the common
parts if necessary.

And with a .S file, I believe there's no need to dynamically generate the thunk,
e.g. pass the jump target through a GPR that's not modified/used by SYSENTER.

> +#ifdef COMP32
> +        "# switch to comp32, mode prior to running the test\n"
> +        "ljmpl *1f\n"
> +        "1:\n"
> +        ".long 1f\n"
> +        ".long " xstr(KERNEL_CS32) "\n"
> +        "1:\n"
> +        ".code32\n"
> +#else
> +		"# store the 64 bit thunk address to rdx\n"
> +		"mov %[thunk], %%rdx\n"
> +#endif