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
