On Wed, 13 Mar 2019 13:33:59 +0000
Wei Yang <richard.weiy...@gmail.com> wrote:
> 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()?
nope, see more below
> >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()?
I mean do not call build_mcfg() at all when you don't have to.
And when you need to keep table_blob the same size (for old machines)
using acpi_data_push() to reserve space instead of build_mcfg(sig="QEMU")
might just work as well. it's still hack but it can live in x86 specific
acpi_build() keeping build_mcfg() generic.
As for defining what to use as criteria to decide when we need to keep
table_blob size the same, I don't remember history of it, so I'd suggest
to look at commit a1666142, study history of acpi_ram_update() and
legacy_acpi_table_size to figure out since which machine type one doesn't
have to keep table_blob size the same.
>
> >> + 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
> >
>
