From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jesse <jdutton@neuraliq.com>
Subject: Re: [PATCH]: pointer to vmcs getting lost
Date: Fri, 01 Aug 2008 16:36:03 -0700
Message-ID: <48939DE3.5070504@neuraliq.com>
References: <48938BCC.2030402@neuraliq.com> <20080801232411.GA3486@dmt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from IP-012-129-246-136.Alorica.com ([12.129.246.136]:46851 "EHLO
	mail.neuraliq.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org
	with ESMTP id S1751726AbYHAXpM (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 1 Aug 2008 19:45:12 -0400
In-Reply-To: <20080801232411.GA3486@dmt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Thanks for the feedback. Comments inline.

Marcelo Tosatti wrote:
> Hi Jesse,
>
> On Fri, Aug 01, 2008 at 03:18:52PM -0700, Jesse wrote:
>   
>> Greetings,
>>
>> I noticed a race condition when running two guests simultaneously and  
>> debugging both guests (on 64-bit intel cpus). Periodically I would get  
>> errors from the vmread, vmwrite, or vmresume instructions. Some research  
>> revealed that these errors were being caused by having an invalid vmcs  
>> loaded. Further, I found that the vmcs is a per_cpu variable, which I  
>> believe means that any reference to it is invalid after a context  
>> switch. (Corrections appreciated). This means that the vmcs must be  
>> reloaded each time the process is switched to. 
>>     
>
> The preempt notifiers will do that for you.
>   
Right, but they won't call VMPTRLD. For some reason this matters (for 
intel chips), even if the variable ends up back in the same place, as 
far as I can tell.
>   
>> The patch below fixed the  
>> problem for me.
>>
>> This patch does three things.
>> 1. Extends the critical section in __vcpu_run to include the handling of  
>> vmexits, where many of the vmread/writes occur.
>> 2. Perform a vcpu_load after we enter the critical section, and after we  
>> return from kvm_resched.
>> 3. Move the call to kvm_guest_debug_pre into the critical section  
>> (because it calls vmread/write).
>>     
>
> Wouldnt it suffice to move ->guest_debug_pre into the non preemptable
> section? http://article.gmane.org/gmane.comp.emulators.kvm.devel/20244 
>   
Excellent. I hadn't seen that patch yet. However, many of the 
vmreads/vmwrites that failed in my testing were in the exit handlers. 
And a calling VMPTRLD (in vcpu_load) explicitly on entering the critical 
section secures any other vmcs concurrency problems.
> I haven't tested that patch though.
>
>