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

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/07/19 20:17, 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.
> 
> The new nomenclature for the dirty paths is "rare", not "full".
> 
> 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.

Yes, CPU-based controls are tricky and should not be changed.  But I
don't see a big issue apart from the CPU-based controls, and the other
checks can also be quite expensive---and the point of dirty_vmcs12 and
shadow VMCS is that we _can_ exclude them most of the time.

This is all 5.4 material anyway, I'll do some testing of Krish's patches
2-5.

Thanks,

Paolo

>   - 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.
> 
>   - 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.  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.