KVM: selftests: restore special vmmcall code layout needed by the harness

[PATCH] KVM: selftests: restore special vmmcall code layout needed by the harness

Posted by Paolo Bonzini 2 years, 9 months ago

Commit 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values
in vmmcall()/vmcall()", 2022-11-21) broke the svm_nested_soft_inject_test
because it placed a "pop rbp" instruction after vmmcall.  While this is
correct and mimics what is done in the VMX case, this particular test
expects a ud2 instruction right after the vmmcall, so that it can skip
over it in the L1 part of the test.

Inline a suitably-modified version of vmmcall() to restore the
functionality of the test.

Fixes: 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values in vmmcall()/vmcall()"
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .../kvm/x86_64/svm_nested_soft_inject_test.c        | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
index e497ace629c1..b34980d45648 100644
--- a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
+++ b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
@@ -41,8 +41,17 @@ static void guest_int_handler(struct ex_regs *regs)
 static void l2_guest_code_int(void)
 {
 	GUEST_ASSERT_1(int_fired == 1, int_fired);
-	vmmcall();
-	ud2();
+
+	/*
+         * Same as the vmmcall() function, but with a ud2 sneaked after the
+         * vmmcall.  The caller injects an exception with the return address
+         * increased by 2, so the "pop rbp" must be after the ud2 and we cannot
+	 * use vmmcall() directly.
+         */
+	__asm__ __volatile__("push %%rbp; vmmcall; ud2; pop %%rbp"
+                             : : "a"(0xdeadbeef), "c"(0xbeefdead)
+                             : "rbx", "rdx", "rsi", "rdi", "r8", "r9",
+                               "r10", "r11", "r12", "r13", "r14", "r15");
 
 	GUEST_ASSERT_1(bp_fired == 1, bp_fired);
 	hlt();
-- 
2.31.1

Re: [PATCH] KVM: selftests: restore special vmmcall code layout needed by the harness

Posted by Sean Christopherson 2 years, 9 months ago

On Wed, Nov 30, 2022, Paolo Bonzini wrote:
> Commit 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values
> in vmmcall()/vmcall()", 2022-11-21) broke the svm_nested_soft_inject_test
> because it placed a "pop rbp" instruction after vmmcall.  While this is
> correct and mimics what is done in the VMX case, this particular test
> expects a ud2 instruction right after the vmmcall, so that it can skip
> over it in the L1 part of the test.
> 
> Inline a suitably-modified version of vmmcall() to restore the
> functionality of the test.
>
> Fixes: 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values in vmmcall()/vmcall()"
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

We really, really need to save/restore guest GPRs in L1 when handling exits from L2.

For now,

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH] KVM: selftests: restore special vmmcall code layout needed by the harness

Posted by Vitaly Kuznetsov 2 years, 9 months ago

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Nov 30, 2022, Paolo Bonzini wrote:
>> Commit 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values
>> in vmmcall()/vmcall()", 2022-11-21) broke the svm_nested_soft_inject_test
>> because it placed a "pop rbp" instruction after vmmcall.  While this is
>> correct and mimics what is done in the VMX case, this particular test
>> expects a ud2 instruction right after the vmmcall, so that it can skip
>> over it in the L1 part of the test.
>> 
>> Inline a suitably-modified version of vmmcall() to restore the
>> functionality of the test.
>>
>> Fixes: 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values in vmmcall()/vmcall()"
>> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>
> We really, really need to save/restore guest GPRs in L1 when handling exits from L2.

+1, the amount of stuff we do to workaround the shortcoming (and time
we waste debugging) is getting ridiculously high. 

>
> For now,
>
> Reviewed-by: Sean Christopherson <seanjc@google.com>
>

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH] KVM: selftests: restore special vmmcall code layout needed by the harness

Posted by Maxim Levitsky 2 years, 9 months ago

On Thu, 2022-12-01 at 10:28 +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Wed, Nov 30, 2022, Paolo Bonzini wrote:
> > > Commit 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values
> > > in vmmcall()/vmcall()", 2022-11-21) broke the svm_nested_soft_inject_test
> > > because it placed a "pop rbp" instruction after vmmcall.  While this is
> > > correct and mimics what is done in the VMX case, this particular test
> > > expects a ud2 instruction right after the vmmcall, so that it can skip
> > > over it in the L1 part of the test.
> > > 
> > > Inline a suitably-modified version of vmmcall() to restore the
> > > functionality of the test.
> > > 
> > > Fixes: 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values in vmmcall()/vmcall()"
> > > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > 
> > We really, really need to save/restore guest GPRs in L1 when handling exits from L2.
> 
> +1, the amount of stuff we do to workaround the shortcoming (and time
> we waste debugging) is getting ridiculously high. 
> 
> > For now,
> > 
> > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > 
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 

I didn't notice this fix and also found this issue.

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

Best regards,
	Maxim Levitsky

Re: [PATCH] KVM: selftests: restore special vmmcall code layout needed by the harness

Posted by mlevitsk@redhat.com 2 years, 8 months ago

On Thu, 2022-12-01 at 15:48 +0200, Maxim Levitsky wrote:
> On Thu, 2022-12-01 at 10:28 +0100, Vitaly Kuznetsov wrote:
> > Sean Christopherson <seanjc@google.com> writes:
> > 
> > > On Wed, Nov 30, 2022, Paolo Bonzini wrote:
> > > > Commit 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values
> > > > in vmmcall()/vmcall()", 2022-11-21) broke the svm_nested_soft_inject_test
> > > > because it placed a "pop rbp" instruction after vmmcall.  While this is
> > > > correct and mimics what is done in the VMX case, this particular test
> > > > expects a ud2 instruction right after the vmmcall, so that it can skip
> > > > over it in the L1 part of the test.
> > > > 
> > > > Inline a suitably-modified version of vmmcall() to restore the
> > > > functionality of the test.
> > > > 
> > > > Fixes: 8fda37cf3d41 ("KVM: selftests: Stuff RAX/RCX with 'safe' values in vmmcall()/vmcall()"
> > > > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > ---
> > > 
> > > We really, really need to save/restore guest GPRs in L1 when handling exits from L2.
> > 
> > +1, the amount of stuff we do to workaround the shortcoming (and time
> > we waste debugging) is getting ridiculously high. 
> > 
> > > For now,
> > > 
> > > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > > 
> > 
> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > 
> 
> I didn't notice this fix and also found this issue.
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Best regards,
> 	Maxim Levitsky

Seems that this patch got through the cracks,
pinging so someone else won't need to debug this
test too.

Best regards,
	Maxim Levitsky