Re: [PATCH v2 2/2] KVM: VMX: untangle VMXON revision_id setting when using eVMCS

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> On Thu, Mar 05, 2020 at 07:37:25PM +0100, Vitaly Kuznetsov wrote:
>> As stated in alloc_vmxon_regions(), VMXON region needs to be tagged with
>> revision id from MSR_IA32_VMX_BASIC even in case of eVMCS. The logic to
>> do so is not very straightforward: first, we set
>> hdr.revision_id = KVM_EVMCS_VERSION in alloc_vmcs_cpu() just to reset it
>> back to vmcs_config.revision_id in alloc_vmxon_regions(). Simplify this by
>> introducing 'enum vmcs_type' parameter to alloc_vmcs_cpu().
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>
> ...
>
>> +	 * However, even though not explicitly documented by TLFS, VMXArea
>> +	 * passed as VMXON argument should still be marked with revision_id
>> +	 * reported by physical CPU.
>
> LOL, nice.
>
>
>> +	 */
>> +	if (type != VMXON_REGION && static_branch_unlikely(&enable_evmcs))
>>  		vmcs->hdr.revision_id = KVM_EVMCS_VERSION;
>>  	else
>>  		vmcs->hdr.revision_id = vmcs_config.revision_id;
>>  
>> -	if (shadow)
>> +	if (type == SHADOW_VMCS_REGION)
>>  		vmcs->hdr.shadow_vmcs = 1;
>>  	return vmcs;
>>  }
>
>> -struct vmcs *alloc_vmcs_cpu(bool shadow, int cpu, gfp_t flags);
>> +enum vmcs_type {
>> +	VMXON_REGION,
>> +	VMCS_REGION,
>> +	SHADOW_VMCS_REGION,
>> +};
>> +
>> +struct vmcs *alloc_vmcs_cpu(enum vmcs_type type, int cpu, gfp_t flags);
>>  void free_vmcs(struct vmcs *vmcs);
>>  int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs);
>>  void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs);
>> @@ -498,8 +504,8 @@ void loaded_vmcs_clear(struct loaded_vmcs *loaded_vmcs);
>>  
>>  static inline struct vmcs *alloc_vmcs(bool shadow)
>
> I think it'd be cleaner overall to take "enum vmcs_type" in alloc_vmcs().
> Then the ternary operator goes away and the callers (all two of 'em) are
> self-documenting.

Ya, it didn't seem to be needed with my initial suggestion to rename
alloc_vmcs_cpu() to alloc_vmx_area_cpu() because in case we think of
VMXON region as something different from VMCS we have only two options:
normal VMCS or shadow VMCS and bool flag works perfectly. 

v3 is on the way, stay tuned!

-- 
Vitaly