[PATCH 0/1 v2] nSVM: Test host RFLAGS.TF on VMRUN

[PATCH 0/1 v2] nSVM: Test host RFLAGS.TF on VMRUN

[Krish Sadhukhan]

v1 -> v2:
	i) Patch# 1 from v1 has been removed since Paolo has already applied it.

	ii) Patch# 2 from v1 has been removed since we don't need the helpers.
	    The test now uses the 'prepare_gif_clear' callback in order to set
	    host RFLAGS.TF. The test also triggers a #UD on the VMRUN
	    instruction in order to find out its RIP which is then used by the
	    #DB handler as the begginning of the sub-test that tests Single-
	    Stepping right on VMRUN.


[PATCH 1/1 v2] nSVM: Test effect of host RFLAGS.TF on VMRUN

 x86/svm_tests.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

Krish Sadhukhan (1):
      nSVM: Test effect of host RFLAGS.TF on VMRUN

[PATCH 1/1 v2] 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 | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 29a0b59..def5880 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -2002,6 +2002,132 @@ 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 u64 vmrun_rip;
+static volatile bool host_rflags_vmrun_reached = false;
+static volatile bool host_rflags_set_tf = false;
+
+static void host_rflags_ud_handler(struct ex_regs *r)
+{
+	vmrun_rip = r->rip;
+	wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_SVME);
+}
+
+static void host_rflags_db_handler(struct ex_regs *r)
+{
+	if (host_rflags_ss_on_vmrun) {
+		if (host_rflags_vmrun_reached) {
+			host_rflags_db_handler_flag =
+			    (host_rflags_guest_main_flag == 1) ? 2 : 1;
+			r->rflags &= ~X86_EFLAGS_TF;
+		} else {
+			if (r->rip == vmrun_rip) {
+				host_rflags_guest_main_flag =
+				    host_rflags_db_handler_flag = 0;
+				host_rflags_vmrun_reached = true;
+			}
+		}
+	} else {
+		host_rflags_db_handler_flag =
+		    (host_rflags_guest_main_flag == 1) ? 2 : 1;
+		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);
+	/*
+	 * We trigger a #UD in order to find out the RIP of VMRUN instruction
+	 */
+	wrmsr(MSR_EFER, rdmsr(MSR_EFER) & ~EFER_SVME);
+	handle_exception(UD_VECTOR, host_rflags_ud_handler);
+}
+
+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_guest_main_flag =
+			    (host_rflags_db_handler_flag == 1) ? 2 : 1;
+		vmmcall();
+		if (get_test_stage(test) == 3)
+			break;
+	}
+}
+
+static bool host_rflags_finished(struct svm_test *test)
+{
+	switch (get_test_stage(test)) {
+	case 0:
+		if (vmcb->control.exit_code != SVM_EXIT_VMMCALL &&
+		    host_rflags_db_handler_flag != 0 && host_rflags_guest_main_flag != 0) {
+			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_db_handler_flag != 1 && host_rflags_guest_main_flag != 2) {
+			report(false, "Unexpected VMEXIT. Exit reason 0x%x",
+			    vmcb->control.exit_code);
+			return true;
+		}
+		host_rflags_guest_main_flag = host_rflags_db_handler_flag = 0;
+		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 &&
+		    host_rflags_db_handler_flag != 2 && host_rflags_guest_main_flag != 1) {
+			report(false, "Unexpected VMEXIT. Exit reason 0x%x",
+			    vmcb->control.exit_code);
+			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 +2618,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 1/1 v2] nSVM: Test effect of host RFLAGS.TF on VMRUN

[Paolo Bonzini]

On 05/02/21 00:29, Krish Sadhukhan wrote:
> 
> +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);
> +	/*
> +	 * We trigger a #UD in order to find out the RIP of VMRUN instruction
> +	 */
> +	wrmsr(MSR_EFER, rdmsr(MSR_EFER) & ~EFER_SVME);
> +	handle_exception(UD_VECTOR, host_rflags_ud_handler);
> +}
> +

I think you'd get the RIP of VMLOAD, not VMRUN.

Maybe something like:

diff --git a/x86/svm.c b/x86/svm.c
index a1808c7..88d8452 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -208,14 +208,14 @@ struct regs get_regs(void)

  struct svm_test *v2_test;

-#define ASM_VMRUN_CMD                           \
+#define ASM_PRE_VMRUN                           \
                  "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"               \
+                LOAD_GPR_C
+#define ASM_POST_VMRUN                          \
                  SAVE_GPR_C                      \
                  "mov 0x170(%%rax), %%r15\n\t"   \
                  "mov %%r15, regs+0x80\n\t"      \
@@ -232,7 +232,9 @@ int svm_vmrun(void)
  	regs.rdi = (ulong)v2_test;

  	asm volatile (
-		ASM_VMRUN_CMD
+		ASM_PRE_VMRUN
+                "vmrun %%rax\n\t"
+		ASM_POST_VMRUN
  		:
  		: "a" (virt_to_phys(vmcb))
  		: "memory", "r15");
@@ -240,6 +242,7 @@ int svm_vmrun(void)
  	return (vmcb->control.exit_code);
  }

+extern unsigned char vmrun_rip;
  static void test_run(struct svm_test *test)
  {
  	u64 vmcb_phys = virt_to_phys(vmcb);
@@ -258,7 +261,9 @@ 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
+                        "vmrun_rip: vmrun %%rax\n\t"
+		        ASM_POST_VMRUN
  			"cli \n\t"
  			"stgi"
  			: // inputs clobbered by the guest:

(untested)

Paolo

Re: [PATCH 1/1 v2] nSVM: Test effect of host RFLAGS.TF on VMRUN

[Krish Sadhukhan]

On 2/5/21 12:14 AM, Paolo Bonzini wrote:
> On 05/02/21 00:29, Krish Sadhukhan wrote:
>>
>> +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);
>> +    /*
>> +     * We trigger a #UD in order to find out the RIP of VMRUN 
>> instruction
>> +     */
>> +    wrmsr(MSR_EFER, rdmsr(MSR_EFER) & ~EFER_SVME);
>> +    handle_exception(UD_VECTOR, host_rflags_ud_handler);
>> +}
>> +
>
> I think you'd get the RIP of VMLOAD, not VMRUN.
>
> Maybe something like:
>
> diff --git a/x86/svm.c b/x86/svm.c
> index a1808c7..88d8452 100644
> --- a/x86/svm.c
> +++ b/x86/svm.c
> @@ -208,14 +208,14 @@ struct regs get_regs(void)
>
>  struct svm_test *v2_test;
>
> -#define ASM_VMRUN_CMD                           \
> +#define ASM_PRE_VMRUN                           \
>                  "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"               \
> +                LOAD_GPR_C
> +#define ASM_POST_VMRUN                          \
>                  SAVE_GPR_C                      \
>                  "mov 0x170(%%rax), %%r15\n\t"   \
>                  "mov %%r15, regs+0x80\n\t"      \
> @@ -232,7 +232,9 @@ int svm_vmrun(void)
>      regs.rdi = (ulong)v2_test;
>
>      asm volatile (
> -        ASM_VMRUN_CMD
> +        ASM_PRE_VMRUN
> +                "vmrun %%rax\n\t"
> +        ASM_POST_VMRUN
>          :
>          : "a" (virt_to_phys(vmcb))
>          : "memory", "r15");
> @@ -240,6 +242,7 @@ int svm_vmrun(void)
>      return (vmcb->control.exit_code);
>  }
>
> +extern unsigned char vmrun_rip;
>  static void test_run(struct svm_test *test)
>  {
>      u64 vmcb_phys = virt_to_phys(vmcb);
> @@ -258,7 +261,9 @@ 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
> +                        "vmrun_rip: vmrun %%rax\n\t"
> +                ASM_POST_VMRUN
>              "cli \n\t"
>              "stgi"
>              : // inputs clobbered by the guest:
>
> (untested)
>
> Paolo
>

Thanks for the suggestion. I have implemented it in v3 that I have sent out.

The reason why my test (v2) had passed, was because virtual 
VMLOAD/VMSAVE is enabled by default and no #VMEXIT happens due to 
intercepts being disabled (via init_vmcb()). So my #UD handler didn't 
get invoked on VMLOAD but got invoked on VMRUN.

This brings out an interesting point. When intercept for VMLOAD/VMSAVE 
is disabled, there is no effect of unsetting EFER.SVME on VMLOAD/VMLOAD, 
even thought APM vol 2 says it should generate a #UD. I couldn't find 
any text in the APM that describes the effect of unsetting EFER.SVME on 
the virtual VMLOAD/VMSAVE. Is this the expected behavior of the SVM 
hardware or is this a bug in KVM and KVM should handle this ?

I plan to add some more tests based on the correct behavior of virtual 
VMLOAD/VMSAVE.

Re: [PATCH 1/1 v2] nSVM: Test effect of host RFLAGS.TF on VMRUN

[Paolo Bonzini]

On 23/02/21 21:28, Krish Sadhukhan wrote:
> I couldn't find any text in the APM that describes the effect of 
> unsetting EFER.SVME on the virtual VMLOAD/VMSAVE. Is this the expected 
> behavior of the SVM hardware or is this a bug in KVM and KVM should 
> handle this ?

The guest always runs with EFER.SVME=1 (it's part of the VMRUN 
requirements), so yeah, vVMLOAD/VMSAVE must be enabled only if the guest 
has EFER.SVME=1.  Nice!

Paolo