[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole st
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup |
Date: |
Tue, 29 Oct 2013 12:22:09 +0200 |
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,
> >