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