Xudong Hao
2013-Feb-22 03:23 UTC
[PATCH] qemu: define a TOM register to report the base of PCI
Define a TOM(top of memory) register to report the base of PCI memory, update
memory region dynamically.
The use case of this register defination is for Xen till now.
Signed-off-by: Xudong Hao <xudong.hao@intel.com>
Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
hw/pc.h | 4 ---
hw/pc_piix.c | 6 -----
hw/piix_pci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++---------------
3 files changed, 59 insertions(+), 30 deletions(-)
diff --git a/hw/pc.h b/hw/pc.h
index fbcf43d..2a60490 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -134,10 +134,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int
*piix_devfn,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
ram_addr_t ram_size,
- hwaddr pci_hole_start,
- hwaddr pci_hole_size,
- hwaddr pci_hole64_start,
- hwaddr pci_hole64_size,
MemoryRegion *pci_memory,
MemoryRegion *ram_memory);
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0a6923d..98cf467 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -130,12 +130,6 @@ static void pc_init1(MemoryRegion *system_memory,
if (pci_enabled) {
pci_bus = i440fx_init(&i440fx_state, &piix3_devfn,
&isa_bus, gsi,
system_memory, system_io, ram_size,
- below_4g_mem_size,
- 0x100000000ULL - below_4g_mem_size,
- 0x100000000ULL + above_4g_mem_size,
- (sizeof(hwaddr) == 4
- ? 0
- : ((uint64_t)1 << 62)),
pci_memory, ram_memory);
} else {
pci_bus = NULL;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 3d79c73..88bd688 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -86,6 +86,14 @@ struct PCII440FXState {
#define I440FX_PAM_SIZE 7
#define I440FX_SMRAM 0x72
+/* The maximum vaule of TOM(top of memory) register in I440FX
+ * is 1G, so it doesn''t meet any popular virutal machines, so
+ * define another register to report the base of PCI memory.
+ * Use four bytes 0xb0-0xb3 for this purpose, they are originally
+ * resevered for host bridge.
+ * */
+#define I440FX_PCI_HOLE_BASE 0xb0
+
static void piix3_set_irq(void *opaque, int pirq, int level);
static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pci_intx);
static void piix3_write_config_xen(PCIDevice *dev,
@@ -101,6 +109,43 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int
pci_intx)
return (pci_intx + slot_addend) & 3;
}
+
+static void i440fx_update_pci_mem_hole(PCII440FXState *f, bool del)
+{
+ ram_addr_t above_4g_mem_size;
+ hwaddr pci_hole_start, pci_hole_size, pci_hole64_start, pci_hole64_size;
+
+ pci_hole_start = pci_default_read_config(&f->dev,
I440FX_PCI_HOLE_BASE, 4);
+ pci_hole_size = 0x100000000ULL - pci_hole_start;
+
+ if (ram_size >= pci_hole_start) {
+ above_4g_mem_size = ram_size - pci_hole_start;
+ } else {
+ above_4g_mem_size = 0;
+ }
+ pci_hole64_start = 0x100000000ULL + above_4g_mem_size;
+ pci_hole64_size = sizeof(hwaddr) == 4 ? 0 : ((uint64_t)1 << 62);
+
+ if (del) {
+ memory_region_del_subregion(f->system_memory, &f->pci_hole);
+ if (pci_hole64_size) {
+ memory_region_del_subregion(f->system_memory,
&f->pci_hole_64bit);
+ }
+ }
+
+ memory_region_init_alias(&f->pci_hole, "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);
+ memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64",
+ f->pci_address_space,
+ pci_hole64_start, pci_hole64_size);
+ if (pci_hole64_size) {
+ memory_region_add_subregion(f->system_memory, pci_hole64_start,
+ &f->pci_hole_64bit);
+ }
+}
+
+
static void i440fx_update_memory_mappings(PCII440FXState *d)
{
int i;
@@ -136,6 +181,9 @@ static void i440fx_write_config(PCIDevice *dev,
range_covers_byte(address, len, I440FX_SMRAM)) {
i440fx_update_memory_mappings(d);
}
+ if (range_covers_byte(address, len, I440FX_PCI_HOLE_BASE)) {
+ i440fx_update_pci_mem_hole(d, true);
+ }
}
static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
@@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev)
d->dev.config[I440FX_SMRAM] = 0x02;
+ /* Emulate top of memory, here use 0xe0000000 as default val*/
+ if (xen_enabled()) {
+ d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0;
+ } else {
+ d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0;
+ }
+ d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00;
+ d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00;
+ d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00;
+
cpu_smm_register(&i440fx_set_smm, d);
return 0;
}
@@ -214,10 +272,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
ram_addr_t ram_size,
- hwaddr pci_hole_start,
- hwaddr pci_hole_size,
- hwaddr pci_hole64_start,
- hwaddr pci_hole64_size,
MemoryRegion *pci_address_space,
MemoryRegion *ram_memory)
{
@@ -244,16 +298,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
f->system_memory = address_space_mem;
f->pci_address_space = pci_address_space;
f->ram_memory = ram_memory;
- memory_region_init_alias(&f->pci_hole, "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);
- memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64",
- f->pci_address_space,
- pci_hole64_start, pci_hole64_size);
- if (pci_hole64_size) {
- memory_region_add_subregion(f->system_memory, pci_hole64_start,
- &f->pci_hole_64bit);
- }
memory_region_init_alias(&f->smram_region, "smram-region",
f->pci_address_space, 0xa0000, 0x20000);
memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
@@ -295,6 +339,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
(*pi440fx_state)->dev.config[0x57]=ram_size;
i440fx_update_memory_mappings(f);
+ i440fx_update_pci_mem_hole(f, false);
return b;
}
@@ -304,10 +349,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int
*piix3_devfn,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io,
ram_addr_t ram_size,
- hwaddr pci_hole_start,
- hwaddr pci_hole_size,
- hwaddr pci_hole64_start,
- hwaddr pci_hole64_size,
MemoryRegion *pci_memory, MemoryRegion *ram_memory)
{
@@ -315,8 +356,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int
*piix3_devfn,
b = i440fx_common_init("i440FX", pi440fx_state, piix3_devfn,
isa_bus, pic,
address_space_mem, address_space_io, ram_size,
- pci_hole_start, pci_hole_size,
- pci_hole64_start, pci_hole64_size,
pci_memory, ram_memory);
return b;
}
--
1.7.12.1
Jan Beulich
2013-Feb-22 09:03 UTC
Re: [PATCH] qemu: define a TOM register to report the base of PCI
>>> On 22.02.13 at 04:23, Xudong Hao <xudong.hao@intel.com> wrote: > @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev) > > d->dev.config[I440FX_SMRAM] = 0x02; > > + /* Emulate top of memory, here use 0xe0000000 as default val*/ > + if (xen_enabled()) { > + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0; > + } else { > + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0; > + } > + d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00; > + d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00; > + d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00; > + > cpu_smm_register(&i440fx_set_smm, d); > return 0; > }Isn''t this the wrong way round (big endian, when it needs to be little)? Or else, this read and calculation>+ pci_hole_start = pci_default_read_config(&f->dev, I440FX_PCI_HOLE_BASE, 4); >+ pci_hole_size = 0x100000000ULL - pci_hole_start;would seem wrong (e.g. if the granularity is meant to be 16M). And then again, wasting 4 bytes of config space on something that one ought to be allowed to expect to be at least 64k-aligned seems questionable too. hvmloader surely could align the value up to the next 64k boundary before writing the - then only word size - field. That would come closer to how real chipsets normally implement things like this. Jan
Stefano Stabellini
2013-Feb-22 12:35 UTC
Re: [PATCH] qemu: define a TOM register to report the base of PCI
On Fri, 22 Feb 2013, Xudong Hao wrote:> Define a TOM(top of memory) register to report the base of PCI memory, update > memory region dynamically. > > The use case of this register defination is for Xen till now. > > Signed-off-by: Xudong Hao <xudong.hao@intel.com> > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>Thanks for the patch! Aside from Jan''s comment on the pci config endianness, I would also like to see an #define somewhere in QEMU to specific what''s the default TOM. In particular I wouldn''t want 0xe0000000 (and 0xf0000000) to be hardcoded in more than one place.> hw/pc.h | 4 --- > hw/pc_piix.c | 6 ----- > hw/piix_pci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++--------------- > 3 files changed, 59 insertions(+), 30 deletions(-) > > diff --git a/hw/pc.h b/hw/pc.h > index fbcf43d..2a60490 100644 > --- a/hw/pc.h > +++ b/hw/pc.h > @@ -134,10 +134,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn, > MemoryRegion *address_space_mem, > MemoryRegion *address_space_io, > ram_addr_t ram_size, > - hwaddr pci_hole_start, > - hwaddr pci_hole_size, > - hwaddr pci_hole64_start, > - hwaddr pci_hole64_size, > MemoryRegion *pci_memory, > MemoryRegion *ram_memory); > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 0a6923d..98cf467 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -130,12 +130,6 @@ static void pc_init1(MemoryRegion *system_memory, > if (pci_enabled) { > pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi, > system_memory, system_io, ram_size, > - below_4g_mem_size, > - 0x100000000ULL - below_4g_mem_size, > - 0x100000000ULL + above_4g_mem_size, > - (sizeof(hwaddr) == 4 > - ? 0 > - : ((uint64_t)1 << 62)), > pci_memory, ram_memory); > } else { > pci_bus = NULL; > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index 3d79c73..88bd688 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -86,6 +86,14 @@ struct PCII440FXState { > #define I440FX_PAM_SIZE 7 > #define I440FX_SMRAM 0x72 > > +/* The maximum vaule of TOM(top of memory) register in I440FX > + * is 1G, so it doesn''t meet any popular virutal machines, so > + * define another register to report the base of PCI memory. > + * Use four bytes 0xb0-0xb3 for this purpose, they are originally > + * resevered for host bridge. > + * */ > +#define I440FX_PCI_HOLE_BASE 0xb0 > + > static void piix3_set_irq(void *opaque, int pirq, int level); > static PCIINTxRoute piix3_route_intx_pin_to_irq(void *opaque, int pci_intx); > static void piix3_write_config_xen(PCIDevice *dev, > @@ -101,6 +109,43 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx) > return (pci_intx + slot_addend) & 3; > } > > + > +static void i440fx_update_pci_mem_hole(PCII440FXState *f, bool del) > +{ > + ram_addr_t above_4g_mem_size; > + hwaddr pci_hole_start, pci_hole_size, pci_hole64_start, pci_hole64_size; > + > + pci_hole_start = pci_default_read_config(&f->dev, I440FX_PCI_HOLE_BASE, 4); > + pci_hole_size = 0x100000000ULL - pci_hole_start; > + > + if (ram_size >= pci_hole_start) { > + above_4g_mem_size = ram_size - pci_hole_start; > + } else { > + above_4g_mem_size = 0; > + } > + pci_hole64_start = 0x100000000ULL + above_4g_mem_size; > + pci_hole64_size = sizeof(hwaddr) == 4 ? 0 : ((uint64_t)1 << 62); > + > + if (del) { > + memory_region_del_subregion(f->system_memory, &f->pci_hole); > + if (pci_hole64_size) { > + memory_region_del_subregion(f->system_memory, &f->pci_hole_64bit); > + } > + } > + > + memory_region_init_alias(&f->pci_hole, "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); > + memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64", > + f->pci_address_space, > + pci_hole64_start, pci_hole64_size); > + if (pci_hole64_size) { > + memory_region_add_subregion(f->system_memory, pci_hole64_start, > + &f->pci_hole_64bit); > + } > +} > + > + > static void i440fx_update_memory_mappings(PCII440FXState *d) > { > int i; > @@ -136,6 +181,9 @@ static void i440fx_write_config(PCIDevice *dev, > range_covers_byte(address, len, I440FX_SMRAM)) { > i440fx_update_memory_mappings(d); > } > + if (range_covers_byte(address, len, I440FX_PCI_HOLE_BASE)) { > + i440fx_update_pci_mem_hole(d, true); > + } > } > > static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id) > @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev) > > d->dev.config[I440FX_SMRAM] = 0x02; > > + /* Emulate top of memory, here use 0xe0000000 as default val*/ > + if (xen_enabled()) { > + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0; > + } else { > + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0; > + } > + d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00; > + d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00; > + d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00; > + > cpu_smm_register(&i440fx_set_smm, d); > return 0; > } > @@ -214,10 +272,6 @@ static PCIBus *i440fx_common_init(const char *device_name, > MemoryRegion *address_space_mem, > MemoryRegion *address_space_io, > ram_addr_t ram_size, > - hwaddr pci_hole_start, > - hwaddr pci_hole_size, > - hwaddr pci_hole64_start, > - hwaddr pci_hole64_size, > MemoryRegion *pci_address_space, > MemoryRegion *ram_memory) > { > @@ -244,16 +298,6 @@ static PCIBus *i440fx_common_init(const char *device_name, > f->system_memory = address_space_mem; > f->pci_address_space = pci_address_space; > f->ram_memory = ram_memory; > - memory_region_init_alias(&f->pci_hole, "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); > - memory_region_init_alias(&f->pci_hole_64bit, "pci-hole64", > - f->pci_address_space, > - pci_hole64_start, pci_hole64_size); > - if (pci_hole64_size) { > - memory_region_add_subregion(f->system_memory, pci_hole64_start, > - &f->pci_hole_64bit); > - } > memory_region_init_alias(&f->smram_region, "smram-region", > f->pci_address_space, 0xa0000, 0x20000); > memory_region_add_subregion_overlap(f->system_memory, 0xa0000, > @@ -295,6 +339,7 @@ static PCIBus *i440fx_common_init(const char *device_name, > (*pi440fx_state)->dev.config[0x57]=ram_size; > > i440fx_update_memory_mappings(f); > + i440fx_update_pci_mem_hole(f, false); > > return b; > } > @@ -304,10 +349,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, > MemoryRegion *address_space_mem, > MemoryRegion *address_space_io, > ram_addr_t ram_size, > - hwaddr pci_hole_start, > - hwaddr pci_hole_size, > - hwaddr pci_hole64_start, > - hwaddr pci_hole64_size, > MemoryRegion *pci_memory, MemoryRegion *ram_memory) > > { > @@ -315,8 +356,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, > > b = i440fx_common_init("i440FX", pi440fx_state, piix3_devfn, isa_bus, pic, > address_space_mem, address_space_io, ram_size, > - pci_hole_start, pci_hole_size, > - pci_hole64_start, pci_hole64_size, > pci_memory, ram_memory); > return b; > } > -- > 1.7.12.1 >
Hao, Xudong
2013-Feb-22 15:37 UTC
Re: [PATCH] qemu: define a TOM register to report the base of PCI
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Friday, February 22, 2013 5:04 PM > To: Hao, Xudong > Cc: stefano.stabellini@eu.citrix.com; Zhang, Xiantao; xen-devel@lists.xen.org; > qemu-devel@nongnu.org; mst@redhat.com; aliguori@us.ibm.com > Subject: Re: [Xen-devel] [PATCH] qemu: define a TOM register to report the > base of PCI > > >>> On 22.02.13 at 04:23, Xudong Hao <xudong.hao@intel.com> wrote: > > @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev) > > > > d->dev.config[I440FX_SMRAM] = 0x02; > > > > + /* Emulate top of memory, here use 0xe0000000 as default val*/ > > + if (xen_enabled()) { > > + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0; > > + } else { > > + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0; > > + } > > + d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00; > > + d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00; > > + d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00; > > + > > cpu_smm_register(&i440fx_set_smm, d); > > return 0; > > } > > Isn''t this the wrong way round (big endian, when it needs to be > little)? >Jan, Here it should be little endian since the following config reading is little endian, I''ll correct it. Thanks your comments.> Or else, this read and calculation > > >+ pci_hole_start = pci_default_read_config(&f->dev, > I440FX_PCI_HOLE_BASE, 4); > >+ pci_hole_size = 0x100000000ULL - pci_hole_start; > > would seem wrong (e.g. if the granularity is meant to be 16M). > > And then again, wasting 4 bytes of config space on something that > one ought to be allowed to expect to be at least 64k-aligned seems > questionable too. hvmloader surely could align the value up to the > next 64k boundary before writing the - then only word size - field.Hvmloader did 64k-aligned, I''m not quite understand, could you help to point out? If considering wasting of config space, actually hvmloader only write 4 values in this register: 3.75G(0xf0000000), 3.5G(0xe0000000), 3G(0xc0000000), 2G(0x80000000), then 1 byte is enough for xen.> That would come closer to how real chipsets normally implement > things like this. > > Jan
Hao, Xudong
2013-Feb-22 15:42 UTC
Re: [PATCH] qemu: define a TOM register to report the base of PCI
> -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: Friday, February 22, 2013 8:35 PM > To: Hao, Xudong > Cc: aliguori@us.ibm.com; mst@redhat.com; qemu-devel@nongnu.org; > Stefano Stabellini; xen-devel@lists.xen.org; Zhang, Xiantao > Subject: Re: [PATCH] qemu: define a TOM register to report the base of PCI > > On Fri, 22 Feb 2013, Xudong Hao wrote: > > Define a TOM(top of memory) register to report the base of PCI memory, > update > > memory region dynamically. > > > > The use case of this register defination is for Xen till now. > > > > Signed-off-by: Xudong Hao <xudong.hao@intel.com> > > Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com> > > Thanks for the patch! > > Aside from Jan''s comment on the pci config endianness, I would also like > to see an #define somewhere in QEMU to specific what''s the default TOM. > In particular I wouldn''t want 0xe0000000 (and 0xf0000000) to be > hardcoded in more than one place. > >Good comments, I''ll define the default TOM out and replace the hardcode. Thanks, -Xudong
Jan Beulich
2013-Feb-22 16:44 UTC
Re: [PATCH] qemu: define a TOM register to report the base of PCI
>>> On 22.02.13 at 16:37, "Hao, Xudong" <xudong.hao@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> And then again, wasting 4 bytes of config space on something that >> one ought to be allowed to expect to be at least 64k-aligned seems >> questionable too. hvmloader surely could align the value up to the >> next 64k boundary before writing the - then only word size - field. > Hvmloader did 64k-aligned, I''m not quite understand, could you help to point > out? > > If considering wasting of config space, actually hvmloader only write 4 > values in this register: 3.75G(0xf0000000), 3.5G(0xe0000000), 3G(0xc0000000), > 2G(0x80000000), then 1 byte is enough for xen.I''d be fine with that too, but whether use 16M granularity here is acceptable going forward I don''t know. Jan