Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Michael S. Tsirkin]

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" <address@hidden> 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,
> >