[PATCH 0/4 v4] nSVM: Test host RFLAGS.TF on VMRUN

[PATCH 0/4 v4] nSVM: Test host RFLAGS.TF on VMRUN

[Krish Sadhukhan]

v3 -> v4:
        1. Patch# 1 fixes the problem differently from what v3 did. In the new
           fix, svm_vcpu_run() for L1 first checks if the previous #VMEXIT from
           L2 was a VMRUN #VMEXIT and if that VMRUN is being single-stepped. If
           both of these conditions are satisfied, it synthesizes a #DB 
           intercept to account for the pending RFLAGS.TF. This prevents the
           instruction next to VMRUN from being executed before taking care of
           the pending RFLAGS.TF.
        2. in Patch# 4, in host_rflags_test(), the call to vmmcall() has been
           moved down. 

[PATCH 1/4 v4] KVM: nSVM: Trigger synthetic #DB intercept following completion of single-stepped VMRUN instruction
[PATCH 2/4 v4] KVM: X86: Add a utility function to read current RIP
[PATCH 3/4 v4] nSVM: Add assembly label to VMRUN instruction
[PATCH 4/4 v4] nSVM: Test effect of host RFLAGS.TF on VMRUN

 arch/x86/kvm/svm/svm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Krish Sadhukhan (1):
      KVM: Trigger synthetic #DB intercept following completion of single-stepped VMRUN instruction

 lib/x86/processor.h |   7 ++++
 x86/svm.c           |  16 ++++++--
 x86/svm_tests.c     | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+), 4 deletions(-)

Krish Sadhukhan (3):
      KVM: X86: Add a utility function to read current RIP
      KVM: nSVM: Add assembly label to VMRUN instruction
      nSVM: Test effect of host RFLAGS.TF on VMRUN

[PATCH 1/4 v4] KVM: nSVM: Trigger synthetic #DB intercept following completion of single-stepped VMRUN instruction

[Krish Sadhukhan]

According to APM, the #DB intercept for a single-stepped VMRUN must happen
after the completion of that instruction, when the guest does #VMEXIT to
the host. However, in the current implementation of KVM, the #DB intercept
for a single-stepped VMRUN happens after the completion of the instruction
that follows the VMRUN instruction. When the #DB intercept handler is
invoked, it shows the RIP of the instruction that follows VMRUN, instead of
of VMRUN itself. This is an incorrect RIP as far as single-stepping VMRUN
is concerned.

This patch fixes the problem by checking for the condtion that the VMRUN
instruction is being single-stepped and if so, triggers a synthetic #DB
intercept so that the #DB for the VMRUN is accounted for at the right time.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oraacle.com>
---
 arch/x86/kvm/svm/svm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 58a45bb139f8..085aa02f584d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3825,6 +3825,21 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
 
 	trace_kvm_entry(vcpu);
 
+	if (unlikely(to_svm(vcpu)->vmcb->control.exit_code == SVM_EXIT_VMRUN &&
+	    to_svm(vcpu)->vmcb->save.rflags & X86_EFLAGS_TF)) {
+		/*
+		 * We are here following a VMRUN that is being
+		 * single-stepped. The #DB intercept that is due for this
+		 * single-stepping, will only be triggered when we execute
+		 * the next VCPU instruction via _svm_vcpu_run(). But it
+		 * will be too late. So we fake a #DB intercept by setting
+		 * the appropriate exit code and returning to our caller
+		 * right from here so that the due #DB can be accounted for.
+		 */
+		svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + DB_VECTOR;
+		return EXIT_FASTPATH_NONE;
+	}
+
 	svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
 	svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];
 	svm->vmcb->save.rip = vcpu->arch.regs[VCPU_REGS_RIP];
-- 
2.27.0

[PATCH 2/4 v4] KVM: X86: Add a utility function to read current RIP

[Krish Sadhukhan]

Some tests may require to know the current RIP to compares tests results. Add
a function to read current RIP. This will be used by a test in this series.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 lib/x86/processor.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 87112bd..5604874 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -489,6 +489,13 @@ static inline unsigned long long rdtsc(void)
 	return r;
 }
 
+static inline u64 read_rip(void)
+{
+	u64 val;
+	asm volatile ("lea (%%rip),%0" : "=r"(val) : : "memory");
+	return val;
+}
+
 /*
  * Per the advice in the SDM, volume 2, the sequence "mfence; lfence"
  * executed immediately before rdtsc ensures that rdtsc will be
-- 
2.27.0

[PATCH 3/4 v4] nSVM: Add assembly label to VMRUN instruction

[Krish Sadhukhan]

Add an assembly label to the VMRUN instruction so that its RIP can be known
to test cases. This will be used by the test in the next patch.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/x86/svm.c b/x86/svm.c
index a1808c7..77fba8b 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -208,14 +208,15 @@ struct regs get_regs(void)
 
 struct svm_test *v2_test;
 
-#define ASM_VMRUN_CMD                           \
+#define ASM_PRE_VMRUN_CMD                       \
                 "vmload %%rax\n\t"              \
                 "mov regs+0x80, %%r15\n\t"      \
                 "mov %%r15, 0x170(%%rax)\n\t"   \
                 "mov regs, %%r15\n\t"           \
                 "mov %%r15, 0x1f8(%%rax)\n\t"   \
                 LOAD_GPR_C                      \
-                "vmrun %%rax\n\t"               \
+
+#define ASM_POST_VMRUN_CMD                      \
                 SAVE_GPR_C                      \
                 "mov 0x170(%%rax), %%r15\n\t"   \
                 "mov %%r15, regs+0x80\n\t"      \
@@ -232,7 +233,9 @@ int svm_vmrun(void)
 	regs.rdi = (ulong)v2_test;
 
 	asm volatile (
-		ASM_VMRUN_CMD
+		ASM_PRE_VMRUN_CMD
+                "vmrun %%rax\n\t"               \
+		ASM_POST_VMRUN_CMD
 		:
 		: "a" (virt_to_phys(vmcb))
 		: "memory", "r15");
@@ -240,6 +243,8 @@ int svm_vmrun(void)
 	return (vmcb->control.exit_code);
 }
 
+extern void *vmrun_rip;
+
 static void test_run(struct svm_test *test)
 {
 	u64 vmcb_phys = virt_to_phys(vmcb);
@@ -258,7 +263,10 @@ static void test_run(struct svm_test *test)
 			"sti \n\t"
 			"call *%c[PREPARE_GIF_CLEAR](%[test]) \n \t"
 			"mov %[vmcb_phys], %%rax \n\t"
-			ASM_VMRUN_CMD
+			ASM_PRE_VMRUN_CMD
+			".global vmrun_rip\n\t"		\
+			"vmrun_rip: vmrun %%rax\n\t"    \
+			ASM_POST_VMRUN_CMD
 			"cli \n\t"
 			"stgi"
 			: // inputs clobbered by the guest:
-- 
2.27.0

[PATCH 4/4 v4] nSVM: Test effect of host RFLAGS.TF on VMRUN

[Krish Sadhukhan]

According to section "VMRUN and TF/RF Bits in EFLAGS" in AMD APM vol 2,

    "From the host point of view, VMRUN acts like a single instruction,
     even though an arbitrary number of guest instructions may execute
     before a #VMEXIT effectively completes the VMRUN. As a single
     host instruction, VMRUN interacts with EFLAGS.TF like ordinary
     instructions. EFLAGS.TF causes a #DB trap after the VMRUN completes
     on the host side (i.e., after the #VMEXIT from the guest).

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/svm_tests.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 29a0b59..fd990f2 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -2002,6 +2002,118 @@ static bool init_intercept_check(struct svm_test *test)
     return init_intercept;
 }
 
+/*
+ * Setting host EFLAGS.TF causes a #DB trap after the VMRUN completes on the
+ * host side (i.e., after the #VMEXIT from the guest).
+ *
+ * [AMD APM]
+ */
+static volatile u8 host_rflags_guest_main_flag = 0;
+static volatile u8 host_rflags_db_handler_flag = 0;
+static volatile bool host_rflags_ss_on_vmrun = false;
+static volatile bool host_rflags_vmrun_reached = false;
+static volatile bool host_rflags_set_tf = false;
+static u64 post_vmrun_rip;
+
+extern void *vmrun_rip;
+
+static void host_rflags_db_handler(struct ex_regs *r)
+{
+	if (host_rflags_ss_on_vmrun) {
+		if (host_rflags_vmrun_reached) {
+			r->rflags &= ~X86_EFLAGS_TF;
+			post_vmrun_rip = r->rip;
+		} else {
+			if (r->rip == (u64)(&vmrun_rip))
+				host_rflags_vmrun_reached = true;
+		}
+	} else {
+		r->rflags &= ~X86_EFLAGS_TF;
+	}
+}
+
+static void host_rflags_prepare(struct svm_test *test)
+{
+	default_prepare(test);
+	handle_exception(DB_VECTOR, host_rflags_db_handler);
+	set_test_stage(test, 0);
+}
+
+static void host_rflags_prepare_gif_clear(struct svm_test *test)
+{
+	if (host_rflags_set_tf)
+		write_rflags(read_rflags() | X86_EFLAGS_TF);
+}
+
+static void host_rflags_test(struct svm_test *test)
+{
+	while (1) {
+		if (get_test_stage(test) > 0 && host_rflags_set_tf &&
+		    (!host_rflags_ss_on_vmrun) &&
+		    (!host_rflags_db_handler_flag))
+			host_rflags_guest_main_flag = 1;
+		if (get_test_stage(test) == 3)
+			break;
+		vmmcall();
+	}
+}
+
+static bool host_rflags_finished(struct svm_test *test)
+{
+	switch (get_test_stage(test)) {
+	case 0:
+		if (vmcb->control.exit_code != SVM_EXIT_VMMCALL) {
+			report(false, "Unexpected VMEXIT. Exit reason 0x%x",
+			    vmcb->control.exit_code);
+			return true;
+		}
+		vmcb->save.rip += 3;
+		/*
+		 * Setting host EFLAGS.TF not immediately before VMRUN, causes
+		 * #DB trap before first guest instruction is executed
+		 */
+		host_rflags_set_tf = true;
+		break;
+	case 1:
+		if (vmcb->control.exit_code != SVM_EXIT_VMMCALL ||
+		    (!host_rflags_guest_main_flag)) {
+			report(false, "Unexpected VMEXIT or #DB handler"
+			    " invoked before guest main. Exit reason 0x%x",
+			    vmcb->control.exit_code);
+			return true;
+		}
+		vmcb->save.rip += 3;
+		/*
+		 * Setting host EFLAGS.TF immediately before VMRUN, causes #DB
+		 * trap after VMRUN completes on the host side (i.e., after
+		 * VMEXIT from guest).
+		 */
+		host_rflags_ss_on_vmrun = true;
+		break;
+	case 2:
+		if (vmcb->control.exit_code != SVM_EXIT_VMMCALL ||
+		    post_vmrun_rip - (u64)(&vmrun_rip) != 3) {
+			report(false, "Unexpected VMEXIT or RIP mismatch."
+			    " Exit reason 0x%x, VMRUN RIP: %lx, post-VMRUN"
+			    " RIP: %lx", vmcb->control.exit_code,
+			    (u64)(&vmrun_rip), post_vmrun_rip);
+			return true;
+		}
+		host_rflags_set_tf = false;
+		vmcb->save.rip += 3;
+		break;
+	default:
+		return true;
+	}
+	inc_test_stage(test);
+	return get_test_stage(test) == 4;
+}
+
+static bool host_rflags_check(struct svm_test *test)
+{
+	return get_test_stage(test) == 3;
+}
+
 #define TEST(name) { #name, .v2 = name }
 
 /*
@@ -2492,6 +2604,9 @@ struct svm_test svm_tests[] = {
     { "svm_init_intercept_test", smp_supported, init_intercept_prepare,
       default_prepare_gif_clear, init_intercept_test,
       init_intercept_finished, init_intercept_check, .on_vcpu = 2 },
+    { "host_rflags", default_supported, host_rflags_prepare,
+      host_rflags_prepare_gif_clear, host_rflags_test,
+      host_rflags_finished, host_rflags_check },
     TEST(svm_cr4_osxsave_test),
     TEST(svm_guest_state_test),
     TEST(svm_vmrun_errata_test),
-- 
2.27.0

Re: [PATCH 3/4 v4] nSVM: Add assembly label to VMRUN instruction

[Paolo Bonzini]

On 22/03/21 19:10, Krish Sadhukhan wrote:
> +extern void *vmrun_rip;

This is not a void*, it can be (for example) an uint8_t.

Paolo

Re: [PATCH 1/4 v4] KVM: nSVM: Trigger synthetic #DB intercept following completion of single-stepped VMRUN instruction

[Paolo Bonzini]

On 22/03/21 19:10, Krish Sadhukhan wrote:
> According to APM, the #DB intercept for a single-stepped VMRUN must happen
> after the completion of that instruction, when the guest does #VMEXIT to
> the host. However, in the current implementation of KVM, the #DB intercept
> for a single-stepped VMRUN happens after the completion of the instruction
> that follows the VMRUN instruction. When the #DB intercept handler is
> invoked, it shows the RIP of the instruction that follows VMRUN, instead of
> of VMRUN itself. This is an incorrect RIP as far as single-stepping VMRUN
> is concerned.
> 
> This patch fixes the problem by checking for the condtion that the VMRUN
> instruction is being single-stepped and if so, triggers a synthetic #DB
> intercept so that the #DB for the VMRUN is accounted for at the right time.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oraacle.com>
> ---
>   arch/x86/kvm/svm/svm.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 58a45bb139f8..085aa02f584d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3825,6 +3825,21 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>   
>   	trace_kvm_entry(vcpu);
>   
> +	if (unlikely(to_svm(vcpu)->vmcb->control.exit_code == SVM_EXIT_VMRUN &&
> +	    to_svm(vcpu)->vmcb->save.rflags & X86_EFLAGS_TF)) {
> +		/*
> +		 * We are here following a VMRUN that is being
> +		 * single-stepped. The #DB intercept that is due for this
> +		 * single-stepping, will only be triggered when we execute
> +		 * the next VCPU instruction via _svm_vcpu_run(). But it
> +		 * will be too late. So we fake a #DB intercept by setting
> +		 * the appropriate exit code and returning to our caller
> +		 * right from here so that the due #DB can be accounted for.
> +		 */
> +		svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + DB_VECTOR;
> +		return EXIT_FASTPATH_NONE;
> +	}
> +
>   	svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
>   	svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];
>   	svm->vmcb->save.rip = vcpu->arch.regs[VCPU_REGS_RIP];
> 

Thanks for the test...  Here I wonder if doing it on the nested vmexit, 
and using kvm_queue_exception, would be clearer.  This VMCB patching is 
quite mysterious.

Paolo

Re: [PATCH 1/4 v4] KVM: nSVM: Trigger synthetic #DB intercept following completion of single-stepped VMRUN instruction

[Krish Sadhukhan]

On 3/22/21 12:08 PM, Paolo Bonzini wrote:
> On 22/03/21 19:10, Krish Sadhukhan wrote:
>> According to APM, the #DB intercept for a single-stepped VMRUN must 
>> happen
>> after the completion of that instruction, when the guest does #VMEXIT to
>> the host. However, in the current implementation of KVM, the #DB 
>> intercept
>> for a single-stepped VMRUN happens after the completion of the 
>> instruction
>> that follows the VMRUN instruction. When the #DB intercept handler is
>> invoked, it shows the RIP of the instruction that follows VMRUN, 
>> instead of
>> of VMRUN itself. This is an incorrect RIP as far as single-stepping 
>> VMRUN
>> is concerned.
>>
>> This patch fixes the problem by checking for the condtion that the VMRUN
>> instruction is being single-stepped and if so, triggers a synthetic #DB
>> intercept so that the #DB for the VMRUN is accounted for at the right 
>> time.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oraacle.com>
>> ---
>>   arch/x86/kvm/svm/svm.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 58a45bb139f8..085aa02f584d 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3825,6 +3825,21 @@ static __no_kcsan fastpath_t 
>> svm_vcpu_run(struct kvm_vcpu *vcpu)
>>         trace_kvm_entry(vcpu);
>>   +    if (unlikely(to_svm(vcpu)->vmcb->control.exit_code == 
>> SVM_EXIT_VMRUN &&
>> +        to_svm(vcpu)->vmcb->save.rflags & X86_EFLAGS_TF)) {
>> +        /*
>> +         * We are here following a VMRUN that is being
>> +         * single-stepped. The #DB intercept that is due for this
>> +         * single-stepping, will only be triggered when we execute
>> +         * the next VCPU instruction via _svm_vcpu_run(). But it
>> +         * will be too late. So we fake a #DB intercept by setting
>> +         * the appropriate exit code and returning to our caller
>> +         * right from here so that the due #DB can be accounted for.
>> +         */
>> +        svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + DB_VECTOR;
>> +        return EXIT_FASTPATH_NONE;
>> +    }
>> +
>>       svm->vmcb->save.rax = vcpu->arch.regs[VCPU_REGS_RAX];
>>       svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];
>>       svm->vmcb->save.rip = vcpu->arch.regs[VCPU_REGS_RIP];
>>
>
> Thanks for the test...  Here I wonder if doing it on the nested 
> vmexit, and using kvm_queue_exception, would be clearer.


Doing it in nested_svm_vmexit() also works. I will send out v5. Thanks !

> This VMCB patching is quite mysterious.
>
> Paolo
>