From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [BUG] Oops with KVM-27 Date: Sat, 16 Jun 2007 10:43:23 +0300 Message-ID: <4673949B.1070505@qumranet.com> References: <68676e00706101354n5fe7e1a9y12cb690cae2924e3@mail.gmail.com> <466CFD6D.2080201@qumranet.com> <20070612175246.GA5864@dreamland.darkstar.lan> <466FB1ED.3090905@qumranet.com> <20070613204948.GA14710@dreamland.darkstar.lan> <4670FBB5.70707@qumranet.com> <20070614225324.GA4088@dreamland.darkstar.lan> <20070614231359.GA5705@dreamland.darkstar.lan> <68676e00706141627s3cb87391sa0ee6711d2f7933f@mail.gmail.com> <467256AA.1040001@qumranet.com> <20070615214915.GA10536@dreamland.darkstar.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Avi Kivity , kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Return-path: In-Reply-To: <20070615214915.GA10536-sTXFmx6KbOnUXq0IF5SVAZ4oGUkBHcCu@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 Luca Tettamanti wrote: > Il Fri, Jun 15, 2007 at 12:06:50PM +0300, Avi Kivity ha scritto: > >>> After a bit of thinking: it's correct but removes an optimization; >>> furthermore it may miss other instructions that write to memory mapped >>> areas. >>> A more proper fix should be "force the writeback if dst.ptr is in some >>> kind of mmio area". >>> >>> >> I think we can just disable the optimization, and (in a separate patch) >> add it back in emulator_write_phys(), where we know we're modifying >> memory and not hitting mmio. >> > > Problem is that in emulator_write_phys() we've already lost track of the > previous (dst.orig_val) value. I moved up the decision in > Actually we haven't; just before the memcpy(), we can put a memcmp() to guard the kvm_mmu_pte_write(), which is the really expensive operation, especially with guest smp. > cmpxchg_emulated; unfortunately this means that the non-locked write > path (write_emulated) can't do this optimization, unless I change its > signature to include the old value. > > The first patch makes the writeback step uncoditional whenever we have a > destination operand (the mov check (d & Mov) may be superfluous, yes?). > The write-to-registry path still has the optimization that skips the > write if possible. > The mov check is in done since the destination is not read for moves; so the check for change would read uninitialized memory. I think we can simply remove the if (). For the register case, the check is more expensive that the write; for mmio, we don't want it; and for memory writes, we can put it in emulator_write_phys(). > Next one: I've splitted emulator_write_phys into emulator_write_phys_mem > (for normal RAM) and emulator_write_phys_mmio (for the rest). The > This is in order to properly emulate cmpxchg(), right? I don't think it's necessary since all that memory is write protected and the modifications happen under kvm->lock. > > I'm a bit confused about this test, found in emulator_write_phys > (original code): > > if (((gpa + bytes - 1) >> PAGE_SHIFT) != (gpa >> PAGE_SHIFT)) > return 0; > > AFAICT is makes the emulator skip the write if the modified area spans > across two different (physical) pages. When this happens write_emulated > does a MMIO write. I'd expect the function to load the 2 pages and do the > memory write on both instead. > This isn't skipping the write; instead it uses the mmio path to update memory instead of the direct path, as I was too lazy to code split page writes. It's also wrong in case someone uses nonaligned writes to update page tables, but no-one does that. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/