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

[Avi Kivity <avi@redhat.com>]

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