On Wed, Mar 13, 2019 at 01:23:00PM +0100, Igor Mammedov wrote: >On Wed, 13 Mar 2019 12:42:53 +0800 >Wei Yang <richardw.y...@linux.intel.com> wrote: > >> Now we have two identical build_mcfg function. >> >> Extract them to aml-build.c. >> >> Signed-off-by: Wei Yang <richardw.y...@linux.intel.com> >> --- >> hw/acpi/aml-build.c | 30 ++++++++++++++++++++++++++++++ >> hw/arm/virt-acpi-build.c | 16 ---------------- >> hw/i386/acpi-build.c | 31 +------------------------------ >> include/hw/acpi/aml-build.h | 1 + >> 4 files changed, 32 insertions(+), 46 deletions(-) >> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >> index 555c24f21d..58d3b8f31d 100644 >> --- a/hw/acpi/aml-build.c >> +++ b/hw/acpi/aml-build.c > >I don't like polluting aml-build.c with PCI stuff, >we have a lot of PCI related code that needs generalizing >lets create a new file for that, something like >hw/acpi/pci.c + include/hw/acpi/pci.h > >> @@ -25,6 +25,7 @@ >> #include "qemu/bswap.h" >> #include "qemu/bitops.h" >> #include "sysemu/numa.h" >> +#include "hw/pci/pcie_host.h" >> >> static GArray *build_alloc_array(void) >> { >> @@ -1870,3 +1871,32 @@ build_hdr: >> build_header(linker, tbl, (void *)(tbl->data + fadt_start), >> "FACP", tbl->len - fadt_start, f->rev, oem_id, >> oem_table_id); >> } >> + >> +void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) >> +{ >> + AcpiTableMcfg *mcfg; >> + const char *sig; >> + int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); >> + >> + mcfg = acpi_data_push(table_data, len); >> + mcfg->allocation[0].address = cpu_to_le64(info->mcfg_base); >> + /* Only a single allocation so no need to play with segments */ >> + mcfg->allocation[0].pci_segment = cpu_to_le16(0); >> + mcfg->allocation[0].start_bus_number = 0; >> + mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - >> 1); > >> + /* >> + * MCFG is used for ECAM which can be enabled or disabled by guest. >> + * To avoid table size changes (which create migration issues), >> + * always create the table even if there are no allocations, >> + * but set the signature to a reserved value in this case. >> + * ACPI spec requires OSPMs to ignore such tables. >> + */ >> + if (info->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) { >> + /* Reserved signature: ignored by OSPM */ >> + sig = "QEMU"; >> + } else { >> + sig = "MCFG"; >> + } >I'd leave these hack at acpi-build.c, just push it up call chain.
Assign sig in acpi-build.c and pass it to build_mcfg()? >More over we don't really need it since resizeable memory region was >introduced. > >So we need to keep table_blob size only for legacy usecase (pre resizable) >and for that just padding table_blob on required size would be sufficient, >there is no need to create dummy QEMU table. >As for newer machines (since resizeable memory region) we don't need to >do even that i.e. just skip table generation altogether if guest disabled it. > I am lost at this place. sig is a part of ACPI table header, you mean the sig is not necessary to be set in ACPI table header? "skip table generation" means remove build_header() in build_mcfg()? >> + build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL); >> +} >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index ae7858a79a..92d8fccb00 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -557,22 +557,6 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg) >> return true; >> } >> >> -static void >> -build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) >> -{ >> - AcpiTableMcfg *mcfg; >> - int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); >> - >> - mcfg = acpi_data_push(table_data, len); >> - mcfg->allocation[0].address = cpu_to_le64(info->mcfg_base); >> - /* Only a single allocation so no need to play with segments */ >> - mcfg->allocation[0].pci_segment = cpu_to_le16(0); >> - mcfg->allocation[0].start_bus_number = 0; >> - mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - >> 1); >> - >> - build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, >> NULL); >> -} >> - >> /* GTDT */ >> static void >> build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index c5b1c3be99..b537a39d42 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -2392,35 +2392,6 @@ build_srat(GArray *table_data, BIOSLinker *linker, >> MachineState *machine) >> table_data->len - srat_start, 1, NULL, NULL); >> } >> >> -static void >> -build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) >> -{ >> - AcpiTableMcfg *mcfg; >> - const char *sig; >> - int len = sizeof(*mcfg) + 1 * sizeof(mcfg->allocation[0]); >> - >> - mcfg = acpi_data_push(table_data, len); >> - mcfg->allocation[0].address = cpu_to_le64(info->mcfg_base); >> - /* Only a single allocation so no need to play with segments */ >> - mcfg->allocation[0].pci_segment = cpu_to_le16(0); >> - mcfg->allocation[0].start_bus_number = 0; >> - mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - >> 1); >> - >> - /* MCFG is used for ECAM which can be enabled or disabled by guest. >> - * To avoid table size changes (which create migration issues), >> - * always create the table even if there are no allocations, >> - * but set the signature to a reserved value in this case. >> - * ACPI spec requires OSPMs to ignore such tables. >> - */ >> - if (info->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) { >> - /* Reserved signature: ignored by OSPM */ >> - sig = "QEMU"; >> - } else { >> - sig = "MCFG"; >> - } >> - build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL); >> -} >> - >> /* >> * VT-d spec 8.1 DMA Remapping Reporting Structure >> * (version Oct. 2014 or later) >> @@ -2687,7 +2658,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState >> *machine) >> } >> if (acpi_get_mcfg(&mcfg)) { >> acpi_add_table(table_offsets, tables_blob); >> - build_mcfg_q35(tables_blob, tables->linker, &mcfg); >> + build_mcfg(tables_blob, tables->linker, &mcfg); >> } >> if (x86_iommu_get_default()) { >> IommuType IOMMUType = x86_iommu_get_type(); >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h >> index b63b85d67c..8f2ea3679f 100644 >> --- a/include/hw/acpi/aml-build.h >> +++ b/include/hw/acpi/aml-build.h >> @@ -423,4 +423,5 @@ void build_slit(GArray *table_data, BIOSLinker *linker); >> >> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, >> const char *oem_id, const char *oem_table_id); >> +void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info); >> #endif > -- Wei Yang Help you, Help me