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

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

[Krish Sadhukhan]

v4 -> v5:
        1. The fix in patch# 1 has been modified. We are queue'ing the
           pending #DB intercept via nested_svm_vmexit() if the VMRUN is
           found to be single-stepped.
        2. In patch# 3, the assembly label for tracking the VMRUN RIP has
           been changed to u64* from void*.

[PATCH 1/4 v5] KVM: nSVM: If VMRUN is single-stepped, queue the #DB
[PATCH 2/4 v5] KVM: X86: Add a utility function to read current RIP
[PATCH 3/4 v5] KVM: nSVM: Add assembly label to VMRUN instruction
[PATCH 4/4 v5] nSVM: Test effect of host RFLAGS.TF on VMRUN

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

Krish Sadhukhan (1):
      KVM: nSVM: If VMRUN is single-stepped, queue the #DB intercept in nested_svm_vmexit()

 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 v5] KVM: nSVM: If VMRUN is single-stepped, queue the #DB intercept in nested_svm_vmexit()

[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, in nested_svm_vmexit(), for the
condition that the VMRUN instruction is being single-stepped and if so,
queues the pending #DB intercept so that the #DB is accounted for before
we execute L1's next instruction.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oraacle.com>
---
 arch/x86/kvm/svm/nested.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 35891d9a1099..713ce5cfb0db 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -720,6 +720,16 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	kvm_clear_exception_queue(&svm->vcpu);
 	kvm_clear_interrupt_queue(&svm->vcpu);
 
+	/*
+	 * If we are here following the completion of a VMRUN that
+	 * is being single-stepped, queue the pending #DB intercept
+	 * right now so that it an be accounted for before we execute
+	 * L1's next instruction.
+	 */
+	if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_VMRUN &&
+	    svm->vmcb->save.rflags & X86_EFLAGS_TF))
+		kvm_queue_exception(&(svm->vcpu), DB_VECTOR);
+
 	return 0;
 }
 
-- 
2.27.0

Re: [PATCH 1/4 v5] KVM: nSVM: If VMRUN is single-stepped, queue the #DB intercept in nested_svm_vmexit()

[Paolo Bonzini]

On 23/03/21 18:50, 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, in nested_svm_vmexit(), for the
> condition that the VMRUN instruction is being single-stepped and if so,
> queues the pending #DB intercept so that the #DB is accounted for before
> we execute L1's next instruction.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oraacle.com>
> ---
>   arch/x86/kvm/svm/nested.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 35891d9a1099..713ce5cfb0db 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -720,6 +720,16 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>   	kvm_clear_exception_queue(&svm->vcpu);
>   	kvm_clear_interrupt_queue(&svm->vcpu);
>   
> +	/*
> +	 * If we are here following the completion of a VMRUN that
> +	 * is being single-stepped, queue the pending #DB intercept
> +	 * right now so that it an be accounted for before we execute
> +	 * L1's next instruction.
> +	 */
> +	if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_VMRUN &&
> +	    svm->vmcb->save.rflags & X86_EFLAGS_TF))
> +		kvm_queue_exception(&(svm->vcpu), DB_VECTOR);
> +
>   	return 0;
>   }

Wouldn't the exit code always be SVM_EXIT_VMRUN after the vmcb01/vmcb02 
split?  I can take care of adding a WARN_ON myself, but I wouldn't mind 
if you checked that my reasoning is true. :)

Paolo

Re: [PATCH 1/4 v5] KVM: nSVM: If VMRUN is single-stepped, queue the #DB intercept in nested_svm_vmexit()

[Krish Sadhukhan]

On 3/26/21 10:32 AM, Paolo Bonzini wrote:
> On 23/03/21 18:50, 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, in nested_svm_vmexit(), for 
>> the
>> condition that the VMRUN instruction is being single-stepped and if so,
>> queues the pending #DB intercept so that the #DB is accounted for before
>> we execute L1's next instruction.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oraacle.com>
>> ---
>>   arch/x86/kvm/svm/nested.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 35891d9a1099..713ce5cfb0db 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -720,6 +720,16 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>>       kvm_clear_exception_queue(&svm->vcpu);
>>       kvm_clear_interrupt_queue(&svm->vcpu);
>>   +    /*
>> +     * If we are here following the completion of a VMRUN that
>> +     * is being single-stepped, queue the pending #DB intercept
>> +     * right now so that it an be accounted for before we execute
>> +     * L1's next instruction.
>> +     */
>> +    if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_VMRUN &&
>> +        svm->vmcb->save.rflags & X86_EFLAGS_TF))
>> +        kvm_queue_exception(&(svm->vcpu), DB_VECTOR);
>> +
>>       return 0;
>>   }
>
> Wouldn't the exit code always be SVM_EXIT_VMRUN after the 
> vmcb01/vmcb02 split?  I can take care of adding a WARN_ON myself, but 
> I wouldn't mind if you checked that my reasoning is true. :)


Looking at the patchset on "vmcb01/vmcb02 split", I see that we are calling

         svm_switch_vmcb(svm, &svm->vmcb01)

in nested_svm_vmexiit(). So, yes, by the time we reach the above check 
we should always have an exit code of SVM_EXIT_VMRUN.

>
> Paolo
>

[PATCH 2/4 v5] 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.

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 v5] KVM: 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.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
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..0cf3b97 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 u64 *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 v5] 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..46db49a 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 u64 *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 0/4 v5] nSVM: Test host RFLAGS.TF on VMRUN

[Paolo Bonzini]

On 23/03/21 18:50, Krish Sadhukhan wrote:
> v4 -> v5:
>          1. The fix in patch# 1 has been modified. We are queue'ing the
>             pending #DB intercept via nested_svm_vmexit() if the VMRUN is
>             found to be single-stepped.
>          2. In patch# 3, the assembly label for tracking the VMRUN RIP has
>             been changed to u64* from void*.
> 
> [PATCH 1/4 v5] KVM: nSVM: If VMRUN is single-stepped, queue the #DB
> [PATCH 2/4 v5] KVM: X86: Add a utility function to read current RIP
> [PATCH 3/4 v5] KVM: nSVM: Add assembly label to VMRUN instruction
> [PATCH 4/4 v5] nSVM: Test effect of host RFLAGS.TF on VMRUN
> 
>   arch/x86/kvm/svm/nested.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> Krish Sadhukhan (1):
>        KVM: nSVM: If VMRUN is single-stepped, queue the #DB intercept in nested_svm_vmexit()
> 
>   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
> 

Queued 1-3-4, 2 is not needed.

Paolo