On 10/07/19 18:15, Sean Christopherson wrote:
> 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;
> }
That makes sense, though it would be somewhat awkward:
dirty_checks = EXEC_CONTROLS | HOST_STATE_FSGS |
ENTRY_CONTROLS_INTRINFO | GUEST_STATE_EFER;
if (vmx->nested.dirty_vmcs12)
dirty_checks |= ENTRY_CONTROLS_FULL | EXIT_CONTROLS |
HOST_STATE_FULL | GUEST_STATE_FULL;
Paolo
