Re: [PATCH v4 3/8] VMX: tertiary execution control infrastructure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2024 12:50, Roger Pau Monné wrote:
> On Thu, Jan 11, 2024 at 10:00:10AM +0100, Jan Beulich wrote:
>> @@ -503,6 +538,9 @@ static int vmx_init_vmcs_config(bool bsp
>>              "Secondary Exec Control",
>>              vmx_secondary_exec_control, _vmx_secondary_exec_control);
>>          mismatch |= cap_check(
>> +            "Tertiary Exec Control",
>> +            vmx_tertiary_exec_control, _vmx_tertiary_exec_control);
> 
> I know it's done to match the surrounding style, but couldn't you move
> the name parameter one line up, and then limit the call to two lines?
> 
> (I don't think it will compromise readability).

You mean like this:

        mismatch |= cap_check("Tertiary Exec Control",
            vmx_tertiary_exec_control, _vmx_tertiary_exec_control);

? No, I view this as a mix of two possible styles. If the string literal
was moved up, the other legitimate style would only be

        mismatch |= cap_check("Tertiary Exec Control",
                              vmx_tertiary_exec_control,
                              _vmx_tertiary_exec_control);

aiui (again extending over 3 lines). Yet none of this is written down
anywhere.

But anyway - consistency with surrounding code trumps here, I think.

>> @@ -2068,10 +2111,12 @@ void vmcs_dump_vcpu(struct vcpu *v)
>>                 vmr(HOST_PERF_GLOBAL_CTRL));
>>  
>>      printk("*** Control State ***\n");
>> -    printk("PinBased=%08x CPUBased=%08x SecondaryExec=%08x\n",
>> +    printk("PinBased=%08x CPUBased=%08x\n",
>>             vmr32(PIN_BASED_VM_EXEC_CONTROL),
>> -           vmr32(CPU_BASED_VM_EXEC_CONTROL),
>> -           vmr32(SECONDARY_VM_EXEC_CONTROL));
>> +           vmr32(CPU_BASED_VM_EXEC_CONTROL));
>> +    printk("SecondaryExec=%08x TertiaryExec=%08lx\n",
> 
> For consistency, shouldn't TertiaryExec use 016 instead of 08 (as it's
> a 64bit filed).

Perhaps, assuming we'll gets bits 32 and populated sooner or later.
However, I view 16-digit literal numbers as hard to read, so I'd be
inclined to insert a separator (e.g. an underscore) between the low
and high halves. Thoughts?

>> @@ -260,6 +262,13 @@ extern u32 vmx_vmentry_control;
>>  #define SECONDARY_EXEC_NOTIFY_VM_EXITING        0x80000000U
>>  extern u32 vmx_secondary_exec_control;
>>  
>> +#define TERTIARY_EXEC_LOADIWKEY_EXITING         BIT(0, UL)
>> +#define TERTIARY_EXEC_ENABLE_HLAT               BIT(1, UL)
>> +#define TERTIARY_EXEC_EPT_PAGING_WRITE          BIT(2, UL)
>> +#define TERTIARY_EXEC_GUEST_PAGING_VERIFY       BIT(3, UL)
>> +#define TERTIARY_EXEC_IPI_VIRT                  BIT(4, UL)
> 
> While at it, my copy of the SDM also has:
> 
> #define TERTIARY_EXEC_VIRT_SPEC_CTRL               BIT(7, UL)

Ah yes, this must have appeared in the over 9 months that have
passed since I originally wrote this patch.

>> --- a/xen/arch/x86/include/asm/msr-index.h
>> +++ b/xen/arch/x86/include/asm/msr-index.h
>> @@ -347,6 +347,7 @@
>>  #define MSR_IA32_VMX_TRUE_EXIT_CTLS             0x48f
>>  #define MSR_IA32_VMX_TRUE_ENTRY_CTLS            0x490
>>  #define MSR_IA32_VMX_VMFUNC                     0x491
>> +#define MSR_IA32_VMX_PROCBASED_CTLS3            0x492
> 
> Shouldn't this be added above the "Legacy MSR constants in need of
> cleanup.  No new MSRs below this comment." line?

Now this is a question I'd like to forward to Andrew. Imo grouping the
new MSR with the other VMX ones is more important than respecting that
comment. But yes, I could of course add yet another patch to move the
entire block up first ...

>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -760,6 +760,12 @@ void vmx_update_secondary_exec_control(s
>>                    v->arch.hvm.vmx.secondary_exec_control);
>>  }
>>  
>> +void vmx_update_tertiary_exec_control(struct vcpu *v)
> 
> const vcpu?

Hmm, yes - overly blind copy-and-paste.

Jan