[Qemu-devel] [PATCH v2 0/2] Clean up around the PCI holes

[Qemu-devel] [PATCH v2 0/2] Clean up around the PCI holes

[Markus Armbruster]

Two weeks on list, two R-bys, time to merge.  Michael, the orthodox
route would be through your tree, but I can do a pull request myself
if you're busy.

v2: Trivially rebased

Markus Armbruster (2):
  piix: Set I440FXState member pci_info.w32 in one place
  pc: Eliminate PcPciInfo

 hw/i386/acpi-build.c      | 43 ++++++++++++++++++++++---------------------
 hw/pci-host/piix.c        | 12 +++++-------
 hw/pci-host/q35.c         | 12 ++++++------
 include/hw/i386/pc.h      |  5 -----
 include/hw/pci-host/q35.h |  2 +-
 5 files changed, 34 insertions(+), 40 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v2 1/2] piix: Set I440FXState member pci_info.w32 in one place

[Markus Armbruster]

Range pci_info.w32 records the location of the PCI hole.

It's initialized to empty when QOM zeroes I440FXState.  That's a fine
value for a still unknown PCI hole.

i440fx_init() sets pci_info.w32.begin = below_4g_mem_size.  Changes
the PCI hole from empty to [below_4g_mem_size, UINT64_MAX].  That's a
bogus value.

i440fx_pcihost_initfn() sets pci_info.end = IO_APIC_DEFAULT_ADDRESS.
Since i440fx_init() ran already, this changes the PCI hole to
[below_4g_mem_size, IO_APIC_DEFAULT_ADDRESS-1].  That's the correct
value.

Setting the bounds of the PCI hole in two separate places is
confusing, and begs the question whether the bogus intermediate value
could be used by something, or what would happen if we somehow managed
to realize an i440FX device without having run the board init function
i440fx_init() first.

Avoid the confusion by setting the (constant) upper bound along with
the lower bound in i440fx_init().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci-host/piix.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index df2b0e2..c63c424 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -263,7 +263,6 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
 static void i440fx_pcihost_initfn(Object *obj)
 {
     PCIHostState *s = PCI_HOST_BRIDGE(obj);
-    I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
 
     memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
                           "pci-conf-idx", 4);
@@ -285,8 +284,6 @@ static void i440fx_pcihost_initfn(Object *obj)
     object_property_add(obj, PCI_HOST_PROP_PCI_HOLE64_END, "int",
                         i440fx_pcihost_get_pci_hole64_end,
                         NULL, NULL, NULL, NULL);
-
-    d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
 static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
@@ -348,6 +345,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
 
     i440fx = I440FX_PCI_HOST_BRIDGE(dev);
     i440fx->pci_info.w32.begin = below_4g_mem_size;
+    i440fx->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 
     /* setup pci memory mapping */
     pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
-- 
2.5.5

[Qemu-devel] [PATCH v2 2/2] pc: Eliminate PcPciInfo

[Markus Armbruster]

PcPciInfo has two (ill-named) members: Range w32 is the PCI hole, and
w64 is the PCI64 hole.

Three users:

* I440FXState and MCHPCIState have a member PcPciInfo pci_info, but
  only pci_info.w32 is actually used.  This is confusing.  Replace by
  Range pci_hole.

* acpi_build() uses auto PcPciInfo pci_info to forward both PCI holes
  from acpi_get_pci_info() to build_dsdt().  Replace by two variables
  Range pci_hole, pci_hole64.  Rename acpi_get_pci_info() to
  acpi_get_pci_holes().

PcPciInfo is now unused; drop it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c      | 43 ++++++++++++++++++++++---------------------
 hw/pci-host/piix.c        | 10 +++++-----
 hw/pci-host/q35.c         | 12 ++++++------
 include/hw/i386/pc.h      |  5 -----
 include/hw/pci-host/q35.h |  2 +-
 5 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5a594be..576c7af 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -229,26 +229,25 @@ static Object *acpi_get_i386_pci_host(void)
     return OBJECT(host);
 }
 
-static void acpi_get_pci_info(PcPciInfo *info)
+static void acpi_get_pci_holes(Range *hole, Range *hole64)
 {
     Object *pci_host;
 
-
     pci_host = acpi_get_i386_pci_host();
     g_assert(pci_host);
 
-    info->w32.begin = object_property_get_int(pci_host,
-                                              PCI_HOST_PROP_PCI_HOLE_START,
-                                              NULL);
-    info->w32.end = object_property_get_int(pci_host,
-                                            PCI_HOST_PROP_PCI_HOLE_END,
-                                            NULL);
-    info->w64.begin = object_property_get_int(pci_host,
-                                              PCI_HOST_PROP_PCI_HOLE64_START,
-                                              NULL);
-    info->w64.end = object_property_get_int(pci_host,
-                                            PCI_HOST_PROP_PCI_HOLE64_END,
+    hole->begin = object_property_get_int(pci_host,
+                                          PCI_HOST_PROP_PCI_HOLE_START,
+                                          NULL);
+    hole->end = object_property_get_int(pci_host,
+                                        PCI_HOST_PROP_PCI_HOLE_END,
+                                        NULL);
+    hole64->begin = object_property_get_int(pci_host,
+                                            PCI_HOST_PROP_PCI_HOLE64_START,
                                             NULL);
+    hole64->end = object_property_get_int(pci_host,
+                                          PCI_HOST_PROP_PCI_HOLE64_END,
+                                          NULL);
 }
 
 #define ACPI_PORT_SMI_CMD           0x00b2 /* TODO: this is APM_CNT_IOPORT */
@@ -1890,7 +1889,7 @@ static Aml *build_q35_osc_method(void)
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker,
            AcpiPmInfo *pm, AcpiMiscInfo *misc,
-           PcPciInfo *pci, MachineState *machine)
+           Range *pci_hole, Range *pci_hole64, MachineState *machine)
 {
     CrsRangeEntry *entry;
     Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
@@ -2047,7 +2046,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
                          AML_CACHEABLE, AML_READ_WRITE,
                          0, 0x000A0000, 0x000BFFFF, 0, 0x00020000));
 
-    crs_replace_with_free_ranges(mem_ranges, pci->w32.begin, pci->w32.end - 1);
+    crs_replace_with_free_ranges(mem_ranges,
+                                 pci_hole->begin, pci_hole->end - 1);
     for (i = 0; i < mem_ranges->len; i++) {
         entry = g_ptr_array_index(mem_ranges, i);
         aml_append(crs,
@@ -2057,12 +2057,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
                              0, entry->limit - entry->base + 1));
     }
 
-    if (pci->w64.begin) {
+    if (pci_hole64->begin) {
         aml_append(crs,
             aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
                              AML_CACHEABLE, AML_READ_WRITE,
-                             0, pci->w64.begin, pci->w64.end - 1, 0,
-                             pci->w64.end - pci->w64.begin));
+                             0, pci_hole64->begin, pci_hole64->end - 1, 0,
+                             pci_hole64->end - pci_hole64->begin));
     }
 
     if (misc->tpm_version != TPM_VERSION_UNSPEC) {
@@ -2554,7 +2554,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     AcpiPmInfo pm;
     AcpiMiscInfo misc;
     AcpiMcfgInfo mcfg;
-    PcPciInfo pci;
+    Range pci_hole, pci_hole64;
     uint8_t *u;
     size_t aml_len = 0;
     GArray *tables_blob = tables->table_data;
@@ -2562,7 +2562,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 
     acpi_get_pm_info(&pm);
     acpi_get_misc_info(&misc);
-    acpi_get_pci_info(&pci);
+    acpi_get_pci_holes(&pci_hole, &pci_hole64);
     acpi_get_slic_oem(&slic_oem);
 
     table_offsets = g_array_new(false, true /* clear */,
@@ -2584,7 +2584,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
-    build_dsdt(tables_blob, tables->linker, &pm, &misc, &pci, machine);
+    build_dsdt(tables_blob, tables->linker, &pm, &misc,
+               &pci_hole, &pci_hole64, machine);
 
     /* Count the size of the DSDT and SSDT, we will need it for legacy
      * sizing of ACPI tables.
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index c63c424..8db0f09 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -48,7 +48,7 @@
 
 typedef struct I440FXState {
     PCIHostState parent_obj;
-    PcPciInfo pci_info;
+    Range pci_hole;
     uint64_t pci_hole64_size;
     uint32_t short_root_bus;
 } I440FXState;
@@ -221,7 +221,7 @@ static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
                                               Error **errp)
 {
     I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
-    uint32_t value = s->pci_info.w32.begin;
+    uint32_t value = s->pci_hole.begin;
 
     visit_type_uint32(v, name, &value, errp);
 }
@@ -231,7 +231,7 @@ static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v,
                                             Error **errp)
 {
     I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
-    uint32_t value = s->pci_info.w32.end;
+    uint32_t value = s->pci_hole.end;
 
     visit_type_uint32(v, name, &value, errp);
 }
@@ -344,8 +344,8 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
     f->ram_memory = ram_memory;
 
     i440fx = I440FX_PCI_HOST_BRIDGE(dev);
-    i440fx->pci_info.w32.begin = below_4g_mem_size;
-    i440fx->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
+    i440fx->pci_hole.begin = below_4g_mem_size;
+    i440fx->pci_hole.end = IO_APIC_DEFAULT_ADDRESS;
 
     /* setup pci memory mapping */
     pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 03be05d..6dff62a 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -73,7 +73,7 @@ static void q35_host_get_pci_hole_start(Object *obj, Visitor *v,
                                         Error **errp)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
-    uint32_t value = s->mch.pci_info.w32.begin;
+    uint32_t value = s->mch.pci_hole.begin;
 
     visit_type_uint32(v, name, &value, errp);
 }
@@ -83,7 +83,7 @@ static void q35_host_get_pci_hole_end(Object *obj, Visitor *v,
                                       Error **errp)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
-    uint32_t value = s->mch.pci_info.w32.end;
+    uint32_t value = s->mch.pci_hole.end;
 
     visit_type_uint32(v, name, &value, errp);
 }
@@ -202,9 +202,9 @@ static void q35_host_initfn(Object *obj)
      * it's not a power of two, which means an MTRR
      * can't cover it exactly.
      */
-    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
+    s->mch.pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
         MCH_HOST_BRIDGE_PCIEXBAR_MAX;
-    s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
+    s->mch.pci_hole.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
 static const TypeInfo q35_host_info = {
@@ -285,9 +285,9 @@ static void mch_update_pciexbar(MCHPCIState *mch)
      * which means an MTRR can't cover it exactly.
      */
     if (enable) {
-        mch->pci_info.w32.begin = addr + length;
+        mch->pci_hole.begin = addr + length;
     } else {
-        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
+        mch->pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
     }
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7e43b20..d44e5e9 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -150,11 +150,6 @@ struct PCMachineClass {
 
 /* PC-style peripherals (also used by other machines).  */
 
-typedef struct PcPciInfo {
-    Range w32;
-    Range w64;
-} PcPciInfo;
-
 #define ACPI_PM_PROP_S3_DISABLED "disable_s3"
 #define ACPI_PM_PROP_S4_DISABLED "disable_s4"
 #define ACPI_PM_PROP_S4_VAL "s4_val"
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 1075f3e..33b547f 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -55,7 +55,7 @@ typedef struct MCHPCIState {
     MemoryRegion smram_region, open_high_smram;
     MemoryRegion smram, low_smram, high_smram;
     MemoryRegion tseg_blackhole, tseg_window;
-    PcPciInfo pci_info;
+    Range pci_hole;
     uint64_t below_4g_mem_size;
     uint64_t above_4g_mem_size;
     uint64_t pci_hole64_size;
-- 
2.5.5