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@xxxxxxxxxx>
>> ---
>> 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
>>
>
