From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH 3/3] VMX: Enhance invalid guest state emulation
Date: Mon, 31 Aug 2009 16:50:22 +0300
Message-ID: <4A9BD51E.6080601@redhat.com>
References: <1251470963-14542-1-git-send-email-m.gamal005@gmail.com>	 <4A9BC379.2060804@redhat.com> <52d4a3890908310639g4bf478b0t426e5c4fbfb0359f@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: KVM list <kvm@vger.kernel.org>
To: Mohammed Gamal <m.gamal005@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:12182 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750734AbZHaNuX (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 31 Aug 2009 09:50:23 -0400
In-Reply-To: <52d4a3890908310639g4bf478b0t426e5c4fbfb0359f@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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