diff mbox series

[v13,45/48] KVM: selftests: Introduce rdmsr_from_l2() and use it for MSR-Bitmap tests

Message ID 20221101145426.251680-46-vkuznets@redhat.com
State New
Headers
Series KVM: x86: hyper-v: Fine-grained TLB flush + L2 TLB flush features |

Commit Message

Vitaly Kuznetsov Nov. 1, 2022, 2:54 p.m. UTC 
  Hyper-V MSR-Bitmap tests do RDMSR from L2 to exit to L1. While 'evmcs_test'
correctly clobbers all GPRs (which are not preserved), 'hyperv_svm_test'
does not. Introduce and use common rdmsr_from_l2() to avoid code
duplication and remove hardcoding of MSRs.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 .../selftests/kvm/include/x86_64/processor.h  |  9 +++++++
 .../testing/selftests/kvm/x86_64/evmcs_test.c | 24 ++++---------------
 .../selftests/kvm/x86_64/hyperv_svm_test.c    |  8 +++----
 3 files changed, 17 insertions(+), 24 deletions(-)

Comments

Sean Christopherson Nov. 1, 2022, 4:11 p.m. UTC | #1 
On Tue, Nov 01, 2022, Vitaly Kuznetsov wrote:
> Hyper-V MSR-Bitmap tests do RDMSR from L2 to exit to L1. While 'evmcs_test'
> correctly clobbers all GPRs (which are not preserved), 'hyperv_svm_test'
> does not. Introduce and use common rdmsr_from_l2() to avoid code
> duplication and remove hardcoding of MSRs.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h  |  9 +++++++
>  .../testing/selftests/kvm/x86_64/evmcs_test.c | 24 ++++---------------
>  .../selftests/kvm/x86_64/hyperv_svm_test.c    |  8 +++----
>  3 files changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index fbaf0b6cec4b..a14b7e4ea7c4 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -520,6 +520,15 @@ static inline void cpu_relax(void)
>  		"hlt\n"	\
>  		)
>  
> +/* Exit to L1 from L2 with RDMSR instruction */
> +static inline void rdmsr_from_l2(uint32_t msr)

I would prefer keeping this helper out of common x86-64 code, even if it means
duplicating code across multiple Hyper-V tests until the L1 VM-Enter/VM-Exit
sequences get cleaned up.  The name is misleading, e.g. it doesn't really read
the MSR since there are no outputs, and while we could obviously fix that with a
rename or a generic DO_VMEXIT_FROM_L2() macro, I would rather fix the underlying
problem of the world switches clobbering L2 state.  That way all the helpers that
exist for L1 can be used verbatim for L2 instead of needing dedicated helpers for
every instruction that is used to trigger a VM-Exit.
Vitaly Kuznetsov Nov. 1, 2022, 4:27 p.m. UTC | #2 
Sean Christopherson <seanjc@google.com> writes:

> On Tue, Nov 01, 2022, Vitaly Kuznetsov wrote:
>> Hyper-V MSR-Bitmap tests do RDMSR from L2 to exit to L1. While 'evmcs_test'
>> correctly clobbers all GPRs (which are not preserved), 'hyperv_svm_test'
>> does not. Introduce and use common rdmsr_from_l2() to avoid code
>> duplication and remove hardcoding of MSRs.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  .../selftests/kvm/include/x86_64/processor.h  |  9 +++++++
>>  .../testing/selftests/kvm/x86_64/evmcs_test.c | 24 ++++---------------
>>  .../selftests/kvm/x86_64/hyperv_svm_test.c    |  8 +++----
>>  3 files changed, 17 insertions(+), 24 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
>> index fbaf0b6cec4b..a14b7e4ea7c4 100644
>> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
>> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
>> @@ -520,6 +520,15 @@ static inline void cpu_relax(void)
>>  		"hlt\n"	\
>>  		)
>>  
>> +/* Exit to L1 from L2 with RDMSR instruction */
>> +static inline void rdmsr_from_l2(uint32_t msr)
>
> I would prefer keeping this helper out of common x86-64 code, even if it means
> duplicating code across multiple Hyper-V tests until the L1 VM-Enter/VM-Exit
> sequences get cleaned up.  The name is misleading, e.g. it doesn't really read
> the MSR since there are no outputs

It's somewhat similar to vmcall()/vmmcall() which are only used to exit
from L2 to L1 (and thus nobody complained that all the register values
are random) and not issue a hypercall and return some value.

> and while we could obviously fix that with a
> rename or a generic DO_VMEXIT_FROM_L2() macro, I would rather fix the underlying
> problem of the world switches clobbering L2 state.  That way all the helpers that
> exist for L1 can be used verbatim for L2 instead of needing dedicated helpers for
> every instruction that is used to trigger a VM-Exit.
>

I'm fine with keeping two copies of this in Hyper-V tests for now, of course.
Sean Christopherson Nov. 1, 2022, 5:26 p.m. UTC | #3 
On Tue, Nov 01, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Tue, Nov 01, 2022, Vitaly Kuznetsov wrote:
> >> Hyper-V MSR-Bitmap tests do RDMSR from L2 to exit to L1. While 'evmcs_test'
> >> correctly clobbers all GPRs (which are not preserved), 'hyperv_svm_test'
> >> does not. Introduce and use common rdmsr_from_l2() to avoid code
> >> duplication and remove hardcoding of MSRs.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  .../selftests/kvm/include/x86_64/processor.h  |  9 +++++++
> >>  .../testing/selftests/kvm/x86_64/evmcs_test.c | 24 ++++---------------
> >>  .../selftests/kvm/x86_64/hyperv_svm_test.c    |  8 +++----
> >>  3 files changed, 17 insertions(+), 24 deletions(-)
> >> 
> >> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> >> index fbaf0b6cec4b..a14b7e4ea7c4 100644
> >> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> >> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> >> @@ -520,6 +520,15 @@ static inline void cpu_relax(void)
> >>  		"hlt\n"	\
> >>  		)
> >>  
> >> +/* Exit to L1 from L2 with RDMSR instruction */
> >> +static inline void rdmsr_from_l2(uint32_t msr)
> >
> > I would prefer keeping this helper out of common x86-64 code, even if it means
> > duplicating code across multiple Hyper-V tests until the L1 VM-Enter/VM-Exit
> > sequences get cleaned up.  The name is misleading, e.g. it doesn't really read
> > the MSR since there are no outputs
> 
> It's somewhat similar to vmcall()/vmmcall() which are only used to exit
> from L2 to L1 (and thus nobody complained that all the register values
> are random) and not issue a hypercall and return some value.

Sort of.  VMCALL/VMMCALL are unique in that they have no meaning (ignoring VMX's
STM) other than what is given to them by the hypervisor/software on VM-Exit.  RDMSR
on the other hand (and literally every other instruction), has architecturally
defined behavior and thus expectations beyond generating a VM-Exit.

I do think we should clean up the VMCALL/VMMCALL code to remove the clobbers
if/when the VM-Enter/VM-Exit sequences are fixed, and maybe make them more generic,
e.g. to allow reusing helpers for L1 and L2.  But, because the meaning of VMCALL/VMMCALL
is software-defined, we'll always need a selftests specific L2=>L1 hypercall,
e.g. to ensure L0 forwards the hypercall to L1.
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index fbaf0b6cec4b..a14b7e4ea7c4 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -520,6 +520,15 @@  static inline void cpu_relax(void)
 		"hlt\n"	\
 		)
 
+/* Exit to L1 from L2 with RDMSR instruction */
+static inline void rdmsr_from_l2(uint32_t msr)
+{
+	/* Currently, L1 doesn't preserve GPRs during vmexits. */
+	__asm__ __volatile__ ("rdmsr" : : "c"(msr) :
+			      "rax", "rbx", "rdx", "rsi", "rdi", "r8", "r9",
+			      "r10", "r11", "r12", "r13", "r14", "r15");
+}
+
 bool is_intel_cpu(void);
 bool is_amd_cpu(void);
 
diff --git a/tools/testing/selftests/kvm/x86_64/evmcs_test.c b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
index 74f076ba574b..a9f511c192c2 100644
--- a/tools/testing/selftests/kvm/x86_64/evmcs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
@@ -30,22 +30,6 @@  static void guest_nmi_handler(struct ex_regs *regs)
 {
 }
 
-/* Exits to L1 destroy GRPs! */
-static inline void rdmsr_fs_base(void)
-{
-	__asm__ __volatile__ ("mov $0xc0000100, %%rcx; rdmsr" : : :
-			      "rax", "rbx", "rcx", "rdx",
-			      "rsi", "rdi", "r8", "r9", "r10", "r11", "r12",
-			      "r13", "r14", "r15");
-}
-static inline void rdmsr_gs_base(void)
-{
-	__asm__ __volatile__ ("mov $0xc0000101, %%rcx; rdmsr" : : :
-			      "rax", "rbx", "rcx", "rdx",
-			      "rsi", "rdi", "r8", "r9", "r10", "r11", "r12",
-			      "r13", "r14", "r15");
-}
-
 void l2_guest_code(void)
 {
 	GUEST_SYNC(7);
@@ -58,11 +42,11 @@  void l2_guest_code(void)
 	vmcall();
 
 	/* MSR-Bitmap tests */
-	rdmsr_fs_base(); /* intercepted */
-	rdmsr_fs_base(); /* intercepted */
-	rdmsr_gs_base(); /* not intercepted */
+	rdmsr_from_l2(MSR_FS_BASE); /* intercepted */
+	rdmsr_from_l2(MSR_FS_BASE); /* intercepted */
+	rdmsr_from_l2(MSR_GS_BASE); /* not intercepted */
 	vmcall();
-	rdmsr_gs_base(); /* intercepted */
+	rdmsr_from_l2(MSR_GS_BASE); /* intercepted */
 
 	/* Done, exit to L1 and never come back.  */
 	vmcall();
diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c b/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
index 1c3fc38b4f15..e30419766c8a 100644
--- a/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
+++ b/tools/testing/selftests/kvm/x86_64/hyperv_svm_test.c
@@ -30,11 +30,11 @@  void l2_guest_code(void)
 	vmmcall();
 
 	/* MSR-Bitmap tests */
-	rdmsr(MSR_FS_BASE); /* intercepted */
-	rdmsr(MSR_FS_BASE); /* intercepted */
-	rdmsr(MSR_GS_BASE); /* not intercepted */
+	rdmsr_from_l2(MSR_FS_BASE); /* intercepted */
+	rdmsr_from_l2(MSR_FS_BASE); /* intercepted */
+	rdmsr_from_l2(MSR_GS_BASE); /* not intercepted */
 	vmmcall();
-	rdmsr(MSR_GS_BASE); /* intercepted */
+	rdmsr_from_l2(MSR_GS_BASE); /* intercepted */
 
 	GUEST_SYNC(5);