On Wed, Jul 10, 2019 at 04:35:46PM +0200, Paolo Bonzini wrote:
> 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.
No argument there. My thought was do something like the following so that
all of the "which checks should we perform" logic is consolidated in a
single location and not spread piecemeal throughout the checks themselves.
static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
{
unsigned long dirty_checks;
...
if (vmx->nested.dirty_vmcs12)
dirty_checks = ENTRY_CONTROLS | EXIT_CONTROLS | HOST_STATE |
GUEST_STATE;
else
dirty_checks = 0;
}
