From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Subject: Re: [PATCH] slow_map: minor improvements to ROM BAR handling Date: Tue, 22 Dec 2009 17:10:49 +0100 Message-ID: <4B30EF89.2000407@suse.de> References: <4B30CAF2.4040409@suse.de> <20091222151911.GC18541@redhat.com> <4B30E470.2030001@redhat.com> <4B30E4BB.8000507@suse.de> <20091222152806.GA18676@redhat.com> <4B30E783.7080903@suse.de> <4B30E82A.5030702@redhat.com> <4B30E891.9050908@suse.de> <4B30EA14.7020207@redhat.com> <4B30ED34.2000000@suse.de> <20091222160501.GC18676@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , kvm@vger.kernel.org To: "Michael S. Tsirkin" Return-path: Received: from cantor2.suse.de ([195.135.220.15]:40611 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751515AbZLVQKu (ORCPT ); Tue, 22 Dec 2009 11:10:50 -0500 In-Reply-To: <20091222160501.GC18676@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Michael S. Tsirkin wrote: > On Tue, Dec 22, 2009 at 05:00:52PM +0100, Alexander Graf wrote: > >> Avi Kivity wrote: >> >>> On 12/22/2009 05:41 PM, Alexander Graf wrote: >>> >>>>> We could certainly extend emulate.c to fetch instruction bytes from >>>>> userspace. It uses ->read_std() now, so we'd need to switch to >>>>> ->read_emulated() and add appropriate buffering. >>>>> >>>>> >>>> I thought the policy on emulate.c was to not have a full instruction >>>> emulator but only emulate instructions that do PT modifications or MMIO >>>> access? >>>> >>>> >>> It's not a policy, just laziness. With emulate_invalid_guest_state=1 >>> we need many more instructions. Of course I don't want to add >>> instructions just for the sake of it, since they will be untested. >>> >>> I'd much prefer not to run from mmio if possible - just pointing out >>> it's doable. >>> >> Right... >> >> >>>> emulator is _really_ small. It only does a few MMU specific >>>> instructions, a couple of privileged ones and MMIO accessing ones. >>>> >>>> >>> Btw, we're in the same situation with PowerPC here. The instruction >>> >>> Plus, you have a fixed length instruction length, likely more regular >>> too. I imagine powerpc is load/store, so you don't have to emulate a >>> zillion ALU instructions? >>> >> Well, it's certainly doable (and easier than on x86). But I'm on the >> same position as you on the x86 side. Why increase the emulator size at >> least 10 times if we don't have to? >> >> Either way, people will report bugs when / if they actually start >> executing code off MMIO. So let's not care too much about it for now. >> Just make sure the read-only check is in. >> >> Alex >> > > So I think all we need is this on top? > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > index 066fdb6..0c3c8f4 100644 > --- a/hw/device-assignment.c > +++ b/hw/device-assignment.c > @@ -233,7 +233,8 @@ static void assigned_dev_iomem_map_slow(PCIDevice *pci_dev, int region_num, > int m; > > DEBUG("slow map\n"); > - m = cpu_register_io_memory(slow_bar_read, slow_bar_write, region); > + m = cpu_register_io_memory(slow_bar_read, region_num == PCI_ROM_SLOT ? > + NULL : slow_bar_write, region); > cpu_register_physical_memory(e_phys, e_size, m); > > /* MSI-X MMIO page */ > I guess so, yes. I'd prefer a written out if statement though, but that's probably personal preference. Alex