[PATCH 0/3 v2] kvm-unit-tests: nVMX: Test base and limit fields of guest GDTR and IDTR

[PATCH 0/3 v2] kvm-unit-tests: nVMX: Test base and limit fields of guest GDTR and IDTR

[Krish Sadhukhan]

v1 -> v2:
        This only change is in patch# 1 where I have removed the '#ifdef __x86_64__' guard.


[PATCH 1/3 v2] KVM: kvm-unit-tests: nVMX: Test GUEST_BASE_GDTR and
[PATCH 2/3 v2] KVM: kvm-unit-tests: nVMX: Optimize test_guest_dr7() by
[PATCH 3/3 v2] KVM: kvm-unit-tests: nVMX: Test GUEST_LIMIT_GDTR and

 x86/vmx_tests.c | 50 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

Krish Sadhukhan (3):
      kvm-unit-tests: nVMX: Test GUEST_BASE_GDTR and GUEST_BASE_IDTR on vmentry 
      kvm-unit-tests: nVMX: Optimize test_guest_dr7() by factoring out the loops
      kvm-unit-tests: nVMX: Test GUEST_LIMIT_GDTR and GUEST_LIMIT_IDTR on vmentr

[PATCH 1/3 v2] kvm-unit-tests: nVMX: Test GUEST_BASE_GDTR and GUEST_BASE_IDTR on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Guest Descriptor-Table Registers" in Intel
SDM vol 3C, the following check is performed on the Guest Descriptor-Table
Registers on vmentry of nested guests:

    - On processors that support Intel 64 architecture, the base-address
      fields must contain canonical addresses.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/vmx_tests.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 68f93d3..4308ef3 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7827,6 +7827,9 @@ static void vmx_guest_state_area_test(void)
 	test_load_guest_perf_global_ctrl();
 	test_load_guest_bndcfgs();
 
+	test_canonical(GUEST_BASE_GDTR, "GUEST_BASE_GDTR", false);
+	test_canonical(GUEST_BASE_IDTR, "GUEST_BASE_IDTR", false);
+
 	/*
 	 * Let the guest finish execution
 	 */
-- 
1.8.3.1

[PATCH 2/3 v2] kvm-unit-tests: nVMX: Optimize test_guest_dr7() by factoring out the loops into a macro

[Krish Sadhukhan]

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

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 4308ef3..7dd8bfb 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7704,6 +7704,19 @@ static void vmx_host_state_area_test(void)
 	test_load_host_perf_global_ctrl();
 }
 
+#define TEST_GUEST_VMCS_FIELD_RESERVED_BITS(start, end, inc, fld, str_name,\
+					    val, msg, xfail)		\
+{									\
+	u64 tmp;							\
+	int i;								\
+									\
+	for (i = start; i <= end; i = i + inc) {			\
+		tmp = val | (1ull << i);				\
+		vmcs_write(fld, tmp);					\
+		test_guest_state(msg, xfail, val, str_name);		\
+	}								\
+}
+
 /*
  * If the "load debug controls" VM-entry control is 1, bits 63:32 in
  * the DR7 field must be 0.
@@ -7714,26 +7727,17 @@ static void test_guest_dr7(void)
 {
 	u32 ent_saved = vmcs_read(ENT_CONTROLS);
 	u64 dr7_saved = vmcs_read(GUEST_DR7);
-	u64 val;
-	int i;
 
 	if (ctrl_enter_rev.set & ENT_LOAD_DBGCTLS) {
-		vmcs_clear_bits(ENT_CONTROLS, ENT_LOAD_DBGCTLS);
-		for (i = 0; i < 64; i++) {
-			val = 1ull << i;
-			vmcs_write(GUEST_DR7, val);
-			test_guest_state("ENT_LOAD_DBGCTLS disabled", false,
-					 val, "GUEST_DR7");
-		}
+		vmcs_write(ENT_CONTROLS, ent_saved & ~ENT_LOAD_DBGCTLS);
+		TEST_GUEST_VMCS_FIELD_RESERVED_BITS(0, 63, 4, GUEST_DR7,
+		    "GUEST_DR7", dr7_saved, "ENT_LOAD_DBGCTLS disabled", false);
 	}
 	if (ctrl_enter_rev.clr & ENT_LOAD_DBGCTLS) {
-		vmcs_set_bits(ENT_CONTROLS, ENT_LOAD_DBGCTLS);
-		for (i = 0; i < 64; i++) {
-			val = 1ull << i;
-			vmcs_write(GUEST_DR7, val);
-			test_guest_state("ENT_LOAD_DBGCTLS enabled", i >= 32,
-					 val, "GUEST_DR7");
-		}
+		vmcs_write(ENT_CONTROLS, ent_saved | ENT_LOAD_DBGCTLS);
+		TEST_GUEST_VMCS_FIELD_RESERVED_BITS(0, 63, 4, GUEST_DR7,
+		    "GUEST_DR7", dr7_saved, "ENT_LOAD_DBGCTLS enabled",
+		    i >= 32);
 	}
 	vmcs_write(GUEST_DR7, dr7_saved);
 	vmcs_write(ENT_CONTROLS, ent_saved);
-- 
1.8.3.1

[PATCH 3/3 v2] kvm-unit-tests: nVMX: Test GUEST_LIMIT_GDTR and GUEST_LIMIT_IDTR on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Guest Descriptor-Table Registers" in Intel
SDM vol 3C, the following checks are performed on the Guest Descriptor-Table
Registers on vmentry of nested guests:

    - Bits 31:16 of each limit field must be 0.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 x86/vmx_tests.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 7dd8bfb..d13b34d 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7834,6 +7834,17 @@ static void vmx_guest_state_area_test(void)
 	test_canonical(GUEST_BASE_GDTR, "GUEST_BASE_GDTR", false);
 	test_canonical(GUEST_BASE_IDTR, "GUEST_BASE_IDTR", false);
 
+	u32 guest_desc_limit_saved = vmcs_read(GUEST_LIMIT_GDTR);
+	TEST_GUEST_VMCS_FIELD_RESERVED_BITS(16, 31, 4, GUEST_LIMIT_GDTR,
+	    "GUEST_LIMIT_GDTR", guest_desc_limit_saved, "GUEST_LIMIT_GDTR",
+	    true);
+	vmcs_write(GUEST_LIMIT_GDTR, guest_desc_limit_saved);
+	guest_desc_limit_saved = vmcs_read(GUEST_LIMIT_IDTR);
+	TEST_GUEST_VMCS_FIELD_RESERVED_BITS(16, 31, 4, GUEST_LIMIT_IDTR,
+	    "GUEST_LIMIT_IDTR", guest_desc_limit_saved, "GUEST_LIMIT_IDTR",
+	    true);
+	vmcs_write(GUEST_LIMIT_IDTR, guest_desc_limit_saved);
+
 	/*
 	 * Let the guest finish execution
 	 */
-- 
1.8.3.1

Re: [PATCH 2/3 v2] kvm-unit-tests: nVMX: Optimize test_guest_dr7() by factoring out the loops into a macro

[Sean Christopherson]

I don't think "optimize" is the word you're looking for.  Moving code into
a macro doesn't optimize anything, the only thing it does is consolidate
code.

On Fri, Jun 05, 2020 at 07:20:21PM +0000, Krish Sadhukhan wrote:
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  x86/vmx_tests.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 4308ef3..7dd8bfb 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -7704,6 +7704,19 @@ static void vmx_host_state_area_test(void)
>  	test_load_host_perf_global_ctrl();
>  }
>  
> +#define TEST_GUEST_VMCS_FIELD_RESERVED_BITS(start, end, inc, fld, str_name,\
> +					    val, msg, xfail)		\
> +{									\
> +	u64 tmp;							\
> +	int i;								\
> +									\
> +	for (i = start; i <= end; i = i + inc) {			\

The "i = i + inc" is weird, not to mention a functional change as the callers
are passing in '4', i.e. this only checks every fourth bit.

IMO this whole macro is overkill and doesn't help readability in the callers,
there are too many parameters to cross reference.  What about adding a more
simple helper to iterate over every bit, e.g. 

	for_each_bit(0, 63, val) {
		vmcs_write(GUEST_DR7, val);
		test_guest_state("ENT_LOAD_DBGCTLS disabled", false,
				 val, "GUEST_DR7");
	}

and

        for_each_bit(0, 63, val) {
                vmcs_write(GUEST_DR7, val);
                test_guest_state("ENT_LOAD_DBGCTLS enabled", val >> 32,
                                 val, "GUEST_DR7");
        }


I'm guessing the for_each_bit() thing can be reused in other flows besides
guest state checks.

> +		tmp = val | (1ull << i);				\
> +		vmcs_write(fld, tmp);					\
> +		test_guest_state(msg, xfail, val, str_name);		\
> +	}								\
> +}
> +
>  /*
>   * If the "load debug controls" VM-entry control is 1, bits 63:32 in
>   * the DR7 field must be 0.
> @@ -7714,26 +7727,17 @@ static void test_guest_dr7(void)
>  {
>  	u32 ent_saved = vmcs_read(ENT_CONTROLS);
>  	u64 dr7_saved = vmcs_read(GUEST_DR7);
> -	u64 val;
> -	int i;
>  
>  	if (ctrl_enter_rev.set & ENT_LOAD_DBGCTLS) {
> -		vmcs_clear_bits(ENT_CONTROLS, ENT_LOAD_DBGCTLS);
> -		for (i = 0; i < 64; i++) {
> -			val = 1ull << i;
> -			vmcs_write(GUEST_DR7, val);
> -			test_guest_state("ENT_LOAD_DBGCTLS disabled", false,
> -					 val, "GUEST_DR7");
> -		}
> +		vmcs_write(ENT_CONTROLS, ent_saved & ~ENT_LOAD_DBGCTLS);
> +		TEST_GUEST_VMCS_FIELD_RESERVED_BITS(0, 63, 4, GUEST_DR7,
> +		    "GUEST_DR7", dr7_saved, "ENT_LOAD_DBGCTLS disabled", false);
>  	}
>  	if (ctrl_enter_rev.clr & ENT_LOAD_DBGCTLS) {
> -		vmcs_set_bits(ENT_CONTROLS, ENT_LOAD_DBGCTLS);
> -		for (i = 0; i < 64; i++) {
> -			val = 1ull << i;
> -			vmcs_write(GUEST_DR7, val);
> -			test_guest_state("ENT_LOAD_DBGCTLS enabled", i >= 32,
> -					 val, "GUEST_DR7");
> -		}
> +		vmcs_write(ENT_CONTROLS, ent_saved | ENT_LOAD_DBGCTLS);
> +		TEST_GUEST_VMCS_FIELD_RESERVED_BITS(0, 63, 4, GUEST_DR7,
> +		    "GUEST_DR7", dr7_saved, "ENT_LOAD_DBGCTLS enabled",
> +		    i >= 32);
>  	}
>  	vmcs_write(GUEST_DR7, dr7_saved);
>  	vmcs_write(ENT_CONTROLS, ent_saved);
> -- 
> 1.8.3.1
> 

Re: [PATCH 2/3 v2] kvm-unit-tests: nVMX: Optimize test_guest_dr7() by factoring out the loops into a macro

[Paolo Bonzini]

On 05/06/20 21:58, Sean Christopherson wrote:
> I don't think "optimize" is the word you're looking for.  Moving code into
> a macro doesn't optimize anything, the only thing it does is consolidate
> code.
> 
> On Fri, Jun 05, 2020 at 07:20:21PM +0000, Krish Sadhukhan wrote:
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>>  x86/vmx_tests.c | 36 ++++++++++++++++++++----------------
>>  1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index 4308ef3..7dd8bfb 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -7704,6 +7704,19 @@ static void vmx_host_state_area_test(void)
>>  	test_load_host_perf_global_ctrl();
>>  }
>>  
>> +#define TEST_GUEST_VMCS_FIELD_RESERVED_BITS(start, end, inc, fld, str_name,\
>> +					    val, msg, xfail)		\
>> +{									\
>> +	u64 tmp;							\
>> +	int i;								\
>> +									\
>> +	for (i = start; i <= end; i = i + inc) {			\
> 
> The "i = i + inc" is weird, not to mention a functional change as the callers
> are passing in '4', i.e. this only checks every fourth bit.
> 
> IMO this whole macro is overkill and doesn't help readability in the callers,
> there are too many parameters to cross reference.  What about adding a more
> simple helper to iterate over every bit, e.g. 
> 
> 	for_each_bit(0, 63, val) {
> 		vmcs_write(GUEST_DR7, val);
> 		test_guest_state("ENT_LOAD_DBGCTLS disabled", false,
> 				 val, "GUEST_DR7");
> 	}
> 
> and
> 
>         for_each_bit(0, 63, val) {
>                 vmcs_write(GUEST_DR7, val);
>                 test_guest_state("ENT_LOAD_DBGCTLS enabled", val >> 32,
>                                  val, "GUEST_DR7");
>         }
> 
> 
> I'm guessing the for_each_bit() thing can be reused in other flows besides
> guest state checks.

I agree, and I've not queued this patch (I used v1 because there were
other #ifdef __x86_64__ anyway, and sent a patch to get rid of them all).

Paolo

>> +		tmp = val | (1ull << i);				\
>> +		vmcs_write(fld, tmp);					\
>> +		test_guest_state(msg, xfail, val, str_name);		\
>> +	}								\
>> +}
>> +
>>  /*
>>   * If the "load debug controls" VM-entry control is 1, bits 63:32 in
>>   * the DR7 field must be 0.
>> @@ -7714,26 +7727,17 @@ static void test_guest_dr7(void)
>>  {
>>  	u32 ent_saved = vmcs_read(ENT_CONTROLS);
>>  	u64 dr7_saved = vmcs_read(GUEST_DR7);
>> -	u64 val;
>> -	int i;
>>  
>>  	if (ctrl_enter_rev.set & ENT_LOAD_DBGCTLS) {
>> -		vmcs_clear_bits(ENT_CONTROLS, ENT_LOAD_DBGCTLS);
>> -		for (i = 0; i < 64; i++) {
>> -			val = 1ull << i;
>> -			vmcs_write(GUEST_DR7, val);
>> -			test_guest_state("ENT_LOAD_DBGCTLS disabled", false,
>> -					 val, "GUEST_DR7");
>> -		}
>> +		vmcs_write(ENT_CONTROLS, ent_saved & ~ENT_LOAD_DBGCTLS);
>> +		TEST_GUEST_VMCS_FIELD_RESERVED_BITS(0, 63, 4, GUEST_DR7,
>> +		    "GUEST_DR7", dr7_saved, "ENT_LOAD_DBGCTLS disabled", false);
>>  	}
>>  	if (ctrl_enter_rev.clr & ENT_LOAD_DBGCTLS) {
>> -		vmcs_set_bits(ENT_CONTROLS, ENT_LOAD_DBGCTLS);
>> -		for (i = 0; i < 64; i++) {
>> -			val = 1ull << i;
>> -			vmcs_write(GUEST_DR7, val);
>> -			test_guest_state("ENT_LOAD_DBGCTLS enabled", i >= 32,
>> -					 val, "GUEST_DR7");
>> -		}
>> +		vmcs_write(ENT_CONTROLS, ent_saved | ENT_LOAD_DBGCTLS);
>> +		TEST_GUEST_VMCS_FIELD_RESERVED_BITS(0, 63, 4, GUEST_DR7,
>> +		    "GUEST_DR7", dr7_saved, "ENT_LOAD_DBGCTLS enabled",
>> +		    i >= 32);
>>  	}
>>  	vmcs_write(GUEST_DR7, dr7_saved);
>>  	vmcs_write(ENT_CONTROLS, ent_saved);
>> -- 
>> 1.8.3.1
>>
>