From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47973) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vb7IX-0007Sb-EE for qemu-devel@nongnu.org; Tue, 29 Oct 2013 07:17:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vb7IQ-0004Ck-Ki for qemu-devel@nongnu.org; Tue, 29 Oct 2013 07:17:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19583) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vb7IQ-0004CY-C8 for qemu-devel@nongnu.org; Tue, 29 Oct 2013 07:17:14 -0400 Date: Tue, 29 Oct 2013 12:17:04 +0100 From: Igor Mammedov Message-ID: <20131029121704.3915efcd@nial.usersys.redhat.com> In-Reply-To: <20131029102209.GA17681@redhat.com> References: <1381913354-8815-1-git-send-email-imammedo@redhat.com> <20131016092045.GA21233@redhat.com> <20131029110856.3e5ead43@nial.usersys.redhat.com> <20131029102209.GA17681@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: armbru@redhat.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, kraxel@redhat.com, aliguori@amazon.com, pbonzini@redhat.com, afaerber@suse.de On Tue, 29 Oct 2013 12:22:09 +0200 "Michael S. Tsirkin" wrote: > On Tue, Oct 29, 2013 at 11:08:56AM +0100, Igor Mammedov wrote: > > On Wed, 16 Oct 2013 12:20:45 +0300 > > "Michael S. Tsirkin" wrote: > > > > > On Wed, Oct 16, 2013 at 10:49:10AM +0200, Igor Mammedov wrote: > > > > * simplify PCI address space mapping into system address space, > > > > replacing code duplication in piix/q53 PCs with helper function > > > > > > I think this does not go far enough. > > > > > > I was always wondering about PCI hole in QEMU. > > > Some real PCs have a "PCI hole" where PCI > > > masks real memory, but PIIX does not do this, > > > instead PCI is whenever RAM does not mask it. > > > > > > So it looks like the hole concept is a left-over > > > from when we didn't have priorities in the memory API. > > > How about we remove them? > > > See patch below. > > > I did a quick boot test and it seems to work, of course > > > it needs much more testing. > > > It's on top of Marcel's series adding negative priorities, > > > so works on top of the acpi branch or the pci branch. > > I have done quite thorough testing and it works well except of > > one caveat, it breaks migration due to different memory regions > > layout. > > Interesting. We don't migrate PCI memory regions so what's going on? I'm sorry for noise, it was a false alarm due to wrong testing configuration. It was failing with: " qemu-system-x86_64: pci_add_option_rom: failed to find romfile "efi-e1000.rom" Unknown ramblock "0000:03.0/e1000.rom", cannot accept migration " Caused by incorrect -L option. It works fine when correct BIOS path is provided. One more questions about whether we should disable "etc/pci-info" fw_cfg for 1.7 again since Seabios doesn't use it (and it looks like it's not going to do so)? > > > So we'll have to keep an old aliasing scheme at least for old > > machine types. Having that in mind do we still want to add > > an extra implementation as you suggested? > > > > > > > > I'm also wondering about the smram region - it uses > > > priority 1 but does not say why. > > > Need to check what does it overlap with, and why. > > > > > > > > > > > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > > > index bad3953..988516a 100644 > > > --- a/hw/pci-host/piix.c > > > +++ b/hw/pci-host/piix.c > > > @@ -102,8 +102,6 @@ struct PCII440FXState { > > > MemoryRegion *system_memory; > > > MemoryRegion *pci_address_space; > > > MemoryRegion *ram_memory; > > > - MemoryRegion pci_hole; > > > - MemoryRegion pci_hole_64bit; > > > PAMMemoryRegion pam_regions[13]; > > > MemoryRegion smram_region; > > > uint8_t smm_enabled; > > > @@ -326,7 +324,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > > > PCII440FXState *f; > > > unsigned i; > > > I440FXState *i440fx; > > > - uint64_t pci_hole64_size; > > > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE); > > > s = PCI_HOST_BRIDGE(dev); > > > @@ -354,23 +351,9 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > > > i440fx->pci_info.w32.begin = 0xe0000000; > > > } > > > > > > - memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space, > > > - pci_hole_start, pci_hole_size); > > > - memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole); > > > - > > > - pci_hole64_size = pci_host_get_hole64_size(i440fx->pci_hole64_size); > > > - > > > - pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size, > > > - pci_hole64_size); > > > - memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64", > > > - f->pci_address_space, > > > - i440fx->pci_info.w64.begin, > > > - pci_hole64_size); > > > - if (pci_hole64_size) { > > > - memory_region_add_subregion(f->system_memory, > > > - i440fx->pci_info.w64.begin, > > > - &f->pci_hole_64bit); > > > - } > > > + /* Set to lower priority than RAM */ > > > + memory_region_add_subregion_overlap(f->system_memory, 0x0, > > > + f->pci_address_space, -1); > > > memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region", > > > f->pci_address_space, 0xa0000, 0x20000); > > > memory_region_add_subregion_overlap(f->system_memory, 0xa0000, > > > >