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.
