Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup
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.
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 = 0xe000;
}
-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, 0x1ULL + 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, 0xa, 0x2);
memory_region_add_subregion_overlap(f-system_memory, 0xa,
Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup
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 = 0xe000;
}
-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, 0x1ULL +
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, 0xa, 0x2);
memory_region_add_subregion_overlap(f-system_memory, 0xa,
Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup
On Tue, 29 Oct 2013 12:22:09 +0200
Michael S. Tsirkin m...@redhat.com 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 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?
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 :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 = 0xe000;
}
-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, 0x1ULL +
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, 0xa, 0x2);
memory_region_add_subregion_overlap(f-system_memory, 0xa,
Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup
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. 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? Sorry I didn't answer this question. I think it's a bug - PCI hole should not affect migration.
Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup
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'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 = 0xe000;
}
-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, 0x1ULL + 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, 0xa, 0x2);
memory_region_add_subregion_overlap(f-system_memory, 0xa,
