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.
