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" <m...@redhat.com> 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? > 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, > >