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

[Igor Mammedov]

On Tue, 29 Oct 2013 12:22:09 +0200
"Michael S. Tsirkin" <address@hidden> 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" <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?

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,
> > > 
>