From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] Use cmpxchg for pte updates on walk_addr() Date: Sun, 09 Dec 2007 10:47:04 +0200 Message-ID: <475BAB88.7070907@qumranet.com> References: <20071206150409.GA31735@dmt> <47581418.8000506@qumranet.com> <20071207023237.GA2841@dmt> <4758D4D2.8090208@qumranet.com> <20071207224718.GA31752@dmt> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel To: Marcelo Tosatti Return-path: In-Reply-To: <20071207224718.GA31752@dmt> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Marcelo Tosatti wrote: > On Fri, Dec 07, 2007 at 07:06:26AM +0200, Avi Kivity wrote: > >> Marcelo Tosatti wrote: >> >>> Right, patch at end of the message restarts the process if the pte >>> changes under the walker. The goto is pretty ugly, but I fail to see any >>> elegant way of doing that. Ideas? >>> >>> >>> >> goto is fine for that. But there's a subtle livelock here: suppose vcpu >> 0 is in guest mode with continuously updating a memory location. vcpu 1 >> is faulting with that memory location acting as a pte. While we're in >> kernel mode, we aren't responding to signals like we should; so we need >> to abort the walk and let the guest retry; that way we go through the >> signal_pending() check. >> >> However, this is an intrusive change, so let's start with the goto and >> drop it later in favor or an abort. >> >> >>>>> @@ -1510,6 +1510,9 @@ static int emulator_write_phys(struct kvm_vcpu >>>>> *vcpu, gpa_t gpa, >>>>> { >>>>> int ret; >>>>> >>>>> + /* No need for kvm_cmpxchg_guest_pte here, its the guest >>>>> + * responsability to synchronize pte updates and page faults. >>>>> + */ >>>>> ret = kvm_write_guest(vcpu->kvm, gpa, val, bytes); >>>>> if (ret < 0) >>>>> return 0; >>>>> >>>>> >>>> Hmm. What if an i386 pae guest carefully uses cmpxchg8b to atomically >>>> set a pte? kvm_write_guest() doesn't guarantee atomicity, so an >>>> intended atomic write can be seen splitted by the guest walker doing a >>>> concurrent walk. >>>> >>>> >>> True, an atomic write is needed... a separate patch for that seems more >>> appropriate. >>> >>> >>> >>> >> Yes. >> > > Hi Avi, > > How about the following > > diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c > index c70ac33..8678651 100644 > --- a/drivers/kvm/x86.c > +++ b/drivers/kvm/x86.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -1589,6 +1590,34 @@ static int emulator_cmpxchg_emulated(unsigned long addr, > reported = 1; > printk(KERN_WARNING "kvm: emulating exchange as write\n"); > } > + > + /* guests cmpxchg8b have to be emulated atomically */ > + if (bytes == 8) { > It's only needed on i386, as 8-byte copy_to_user should be atomic on x86_64. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. ------------------------------------------------------------------------- SF.Net email is sponsored by: Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php