Re: [PATCH 0/5] KVM: nVMX: Skip vmentry checks that are necessary only if VMCS12 is dirty

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

On 07/08/2019 11:17 AM, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 03:11:42AM -0400, Krish Sadhukhan wrote:
>> The following functions,
>>
>> 	nested_vmx_check_controls
>> 	nested_vmx_check_host_state
>> 	nested_vmx_check_guest_state
>>
>> do a number of vmentry checks for VMCS12. However, not all of these checks need
>> to be executed on every vmentry. This patchset makes some of these vmentry
>> checks optional based on the state of VMCS12 in that if VMCS12 is dirty, only
>> then the checks will be executed. This will reduce performance impact on
>> vmentry of nested guests.
> All of these patches break vmx_set_nested_state(), which sets dirty_vmcs12
> only after the aforementioned consistency checks pass.

Perhaps vmx_set_nested_state() can set dirty_vmcs12 right before the 
consistency checks are done ? I see no difference in correctness. Also, 
it calls set_current_vmptr() which anyway sets dirty_vmcs12 for valid VMCSs.

>
> The new nomenclature for the dirty paths is "rare", not "full".
OK.
>
> In general, I dislike directly associating the consistency checks with
> dirty_vmcs12.
>
>    - It's difficult to assess the correctness of the resulting code, e.g.
>      changing CPU_BASED_VM_EXEC_CONTROL doesn't set dirty_vmcs12, which
>      calls into question any and all SECONDARY_VM_EXEC_CONTROL checks since
>      an L1 could toggle CPU_BASED_ACTIVATE_SECONDARY_CONTROLS.
>
>    - We lose the existing organization of the consistency checks, e.g.
>      similar checks get arbitrarily split into separate flows based on
>      the rarity of the field changing.

Initially, I was thinking of inserting the check for dirty_vmcs12 right 
in place of each of the checks without having to move them to separate 
functions. That approach saves the separation of the checks but results 
in poor readability. Hence I adopted the current approach.

>
>    - The performance gains are likely minimal since the majority of checks
>      can't be skipped due to the coarseness of dirty_vmcs12.
>
> Rather than a quick and dirty (pun intended) change to use dirty_vmcs12,
> I think we should have some amount of dedicated infrastructure for
> optimizing consistency checks from the get go, e.g. perhaps something
> similar to how eVMCS categorizes fields.
Are you referring to the categorization done in 
copy_vmcs12_to_enlightened() ? If so, what is the basis for 
categorization in there ?
We can re-order the checks in 
nested_vmx_check_{controls,host_state,guest_state} based on dirty_vmcs  
to create an initial framework for controlling the consistency checks. 
The only disadvantage will be that such an ordering will be completely 
off from how the SDM describes the checks.

>   The initial usage could be very
> coarse grained, e.g. based purely on dirty_vmcs12, but having the
> infrastructure would make it easier to reason about the correctness of
> the code.  Future patches could then refine the triggerring of checks to
> achieve better optimization, e.g. skipping the vast majority of checks
> when L1 is simply toggling CPU_BASED_VIRTUAL_INTR_PENDING.
It seems you are suggesting a finer granularity up to each VMCS field 
instead of groups of VMCS fields ? Then we need a per-field flag to 
track its modification and that seems an overkill.