From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Riepe Subject: Re: [ANNOUNCE] kvm-17 release Date: Sat, 24 Mar 2007 13:29:26 +0100 Message-ID: <460519A6.1020005@mr511.de> References: <45FFE14F.6020808@qumranet.com> <4602D9D5.7060806@mr511.de> <46039EEA.4070504@qumranet.com> <46048C34.4080300@osadl.org> <46049B0C.40501@osadl.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel To: Carsten Emde Return-path: In-Reply-To: <46049B0C.40501-Q945KHDl0DbYtjvyW6yDsg@public.gmane.org> 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 Hi! Carsten Emde wrote: > Actually, the above line can be preserved, if we move the previous line > down by 1: Was it a simple race? > > --- kvm_main-17.c 2007-03-24 02:09:00.000000000 +0100 > +++ kvm_main.c 2007-03-24 04:10:59.000000000 +0100 > @@ -1574,8 +1574,8 @@ > > if (kvm_run->mmio_completed) { > memcpy(vcpu->mmio_data, kvm_run->mmio.data, 8); > - vcpu->mmio_read_completed = 1; > emulate_instruction(vcpu, kvm_run, vcpu->mmio_fault_cr2, 0); > + vcpu->mmio_read_completed = 1; > } > > vcpu->mmio_needed = 0; > As far as I understand, vcpu->mmio_read_completed indicates that read data is present in vcpu->mmio_data. If the flag isn't set, emulator_read_emulated() - which is called by emulate_instruction() - will try to read the data *again*. While that's supposed to be okay for ordinary memory, it might make memory mapped i/o devices fail. On the other hand, emulator_read_emulated() clears the flag to indicate that the emulated read has actually happened - which means that another read may drop us into userspace for emulation again. There is, however, no other place where the flag is reset, so setting it after emulate_instruction() may affect the *next* emulated instruction. I think that the flag should be cleared after the call, rather than set. Besides that, vcpu->mmio_read_completed seems to guard a call to do_interrupt_requests() in vmx_vcpu_run(). I guess the reason is that interrupts must not be processed if the current instruction has already been partially emulated. Or did I get something wrong? There's another point here that bothers me: The result of emulate_instruction() isn't checked. Are you sure that it never fails, and that it's safe to proceed to kvm_arch_ops->run(), aka vmx_vcpu_run() in case it's an Intel CPU? -- Michael "Tired" Riepe X-Tired: Each morning I get up I die a little ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV