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

[Sean Christopherson <sean.j.christopherson@intel.com>]

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
>