* [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes
@ 2016-06-15 17:56 Markus Armbruster
2016-06-15 17:56 ` [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place Markus Armbruster
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Markus Armbruster @ 2016-06-15 17:56 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, imammedo, ehabkost, marcel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place 2016-06-15 17:56 [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes Markus Armbruster @ 2016-06-15 17:56 ` Markus Armbruster 2016-06-15 19:16 ` Eric Blake 2016-06-16 7:26 ` Marcel Apfelbaum 2016-06-15 17:56 ` [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo Markus Armbruster 2016-06-30 5:18 ` [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes Markus Armbruster 2 siblings, 2 replies; 8+ messages in thread From: Markus Armbruster @ 2016-06-15 17:56 UTC (permalink / raw) To: qemu-devel; +Cc: mst, imammedo, ehabkost, marcel 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> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place 2016-06-15 17:56 ` [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place Markus Armbruster @ 2016-06-15 19:16 ` Eric Blake 2016-06-16 7:26 ` Marcel Apfelbaum 1 sibling, 0 replies; 8+ messages in thread From: Eric Blake @ 2016-06-15 19:16 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: marcel, imammedo, ehabkost, mst [-- Attachment #1: Type: text/plain, Size: 1350 bytes --] On 06/15/2016 11:56 AM, Markus Armbruster wrote: > 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> > --- > hw/pci-host/piix.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place 2016-06-15 17:56 ` [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place Markus Armbruster 2016-06-15 19:16 ` Eric Blake @ 2016-06-16 7:26 ` Marcel Apfelbaum 1 sibling, 0 replies; 8+ messages in thread From: Marcel Apfelbaum @ 2016-06-16 7:26 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: mst, imammedo, ehabkost On 06/15/2016 08:56 PM, Markus Armbruster wrote: > 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> > --- > 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, > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo 2016-06-15 17:56 [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes Markus Armbruster 2016-06-15 17:56 ` [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place Markus Armbruster @ 2016-06-15 17:56 ` Markus Armbruster 2016-06-15 19:22 ` Eric Blake 2016-06-16 7:30 ` Marcel Apfelbaum 2016-06-30 5:18 ` [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes Markus Armbruster 2 siblings, 2 replies; 8+ messages in thread From: Markus Armbruster @ 2016-06-15 17:56 UTC (permalink / raw) To: qemu-devel; +Cc: mst, imammedo, ehabkost, marcel 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> --- 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 8ca2032..02fc534 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -225,26 +225,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 */ @@ -1867,7 +1866,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; @@ -2015,7 +2014,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, @@ -2025,12 +2025,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) { @@ -2518,7 +2518,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; @@ -2526,7 +2526,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 */, @@ -2548,7 +2548,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 70f897e..8c2c1db 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); } @@ -182,9 +182,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 = { @@ -265,9 +265,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 9ca2309..36ac326 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -148,11 +148,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 c5c073d..949e3a9 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; ram_addr_t below_4g_mem_size; ram_addr_t above_4g_mem_size; uint64_t pci_hole64_size; -- 2.5.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo 2016-06-15 17:56 ` [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo Markus Armbruster @ 2016-06-15 19:22 ` Eric Blake 2016-06-16 7:30 ` Marcel Apfelbaum 1 sibling, 0 replies; 8+ messages in thread From: Eric Blake @ 2016-06-15 19:22 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: marcel, imammedo, ehabkost, mst [-- Attachment #1: Type: text/plain, Size: 1432 bytes --] On 06/15/2016 11:56 AM, Markus Armbruster wrote: > 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> > --- > 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(-) > > +++ b/include/hw/i386/pc.h > @@ -148,11 +148,6 @@ struct PCMachineClass { > > /* PC-style peripherals (also used by other machines). */ > > -typedef struct PcPciInfo { > - Range w32; > - Range w64; > -} PcPciInfo; Confusing indeed. Good riddance. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo 2016-06-15 17:56 ` [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo Markus Armbruster 2016-06-15 19:22 ` Eric Blake @ 2016-06-16 7:30 ` Marcel Apfelbaum 1 sibling, 0 replies; 8+ messages in thread From: Marcel Apfelbaum @ 2016-06-16 7:30 UTC (permalink / raw) To: Markus Armbruster, qemu-devel; +Cc: mst, imammedo, ehabkost On 06/15/2016 08:56 PM, Markus Armbruster wrote: > 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> > --- > 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 8ca2032..02fc534 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -225,26 +225,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 */ > @@ -1867,7 +1866,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; > @@ -2015,7 +2014,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, > @@ -2025,12 +2025,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) { > @@ -2518,7 +2518,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; > @@ -2526,7 +2526,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 */, > @@ -2548,7 +2548,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 70f897e..8c2c1db 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); > } > @@ -182,9 +182,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 = { > @@ -265,9 +265,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 9ca2309..36ac326 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -148,11 +148,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 c5c073d..949e3a9 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; > ram_addr_t below_4g_mem_size; > ram_addr_t above_4g_mem_size; > uint64_t pci_hole64_size; > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes 2016-06-15 17:56 [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes Markus Armbruster 2016-06-15 17:56 ` [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place Markus Armbruster 2016-06-15 17:56 ` [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo Markus Armbruster @ 2016-06-30 5:18 ` Markus Armbruster 2 siblings, 0 replies; 8+ messages in thread From: Markus Armbruster @ 2016-06-30 5:18 UTC (permalink / raw) To: qemu-devel; +Cc: mst, imammedo, ehabkost, marcel Ping? ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-06-30 5:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-15 17:56 [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes Markus Armbruster 2016-06-15 17:56 ` [Qemu-devel] [PATCH 1/2] piix: Set I440FXState member pci_info.w32 in one place Markus Armbruster 2016-06-15 19:16 ` Eric Blake 2016-06-16 7:26 ` Marcel Apfelbaum 2016-06-15 17:56 ` [Qemu-devel] [PATCH 2/2] pc: Eliminate PcPciInfo Markus Armbruster 2016-06-15 19:22 ` Eric Blake 2016-06-16 7:30 ` Marcel Apfelbaum 2016-06-30 5:18 ` [Qemu-devel] [PATCH 0/2] Clean up around the PCI holes Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).