Re: [PATCH 3/3] VMX: Enhance invalid guest state emulation

Avi Kivity
2009-08-31T13:50:23Z

On 08/31/2009 04:39 PM, Mohammed Gamal wrote:
>>> local_irq_enable();
>>> preempt_enable();
>>>
>>>
>> These are now wrong, since handle_invalid_exit() is called with interrupts
>> and preemption enabled.
>>
>>
> Do you mean vmx_handle_exit() ?
>

Yes.

>>> /*
>>> @@ -3405,9 +3412,12 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
>>> /* If we need to emulate an MMIO from handle_invalid_guest_state
>>> * we just return 0 */
>>> if (vmx->emulation_required&& emulate_invalid_guest_state) {
>>> - if (guest_state_valid(vcpu))
>>> + if (guest_state_valid(vcpu)) {
>>> vmx->emulation_required = 0;
>>> - return vmx->invalid_state_emulation_result !=
>>> EMULATE_DO_MMIO;
>>> + return vmx->invalid_state_emulation_result !=
>>> EMULATE_DO_MMIO;
>>>
>>>
>> This looks fishy. Can't say exactly why but vmx_handle_exit() should only
>> depend on the current guest execution, not the previous guest execution.
>>
> I still can't quite get the problem here!
>

The design of the main loop is that you can have a save/restore cycle
(or live migration) after __vcpu_run(). So abything in __vcpu_run() and
the function it calls can only depend on state set previously in
__vcpu_run(), or on state that is loaded from KVM_SET_REGS and similar
ioctls.

In this case I think vmx->invalid_state_emulation_result is not set
previously to its use within __vcpu_run().

>>> /* Access CR3 don't cause VMExit in paging mode, so we need
>>> @@ -3603,12 +3613,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>>> if (unlikely(!cpu_has_virtual_nmis()&& vmx->soft_vnmi_blocked))
>>> vmx->entry_time = ktime_get();
>>>
>>> - /* Handle invalid guest state instead of entering VMX */
>>> - if (vmx->emulation_required&& emulate_invalid_guest_state) {
>>> - handle_invalid_guest_state(vcpu);
>>> - return;
>>> - }
>>> -
>>>
>>>
>> Don't we still need to return here? Otherwise we attempt guest entry
>> needlessly.
>>
> But how would a vmexit be triggered (thus calling vmx_handle_exit() )?
>

vmx_handle_exit() will be called in any case.

> I personally prefer if we can start emulation before attempting guest
> entry, but how can we tell vmx_vcpu_run() to return to userspace if it
> doesn't return a value, I don't feel that changing it to return a
> value would be a wise thing to do too, no?
>

vmx_vcpu_run() can simply return, setting an internal vmx flag. Then
vmx_handle_exit() can notice the flag, emulate, and handle the result of
this emulation.

--
error compiling committee.c: too many arguments to function