Re: [Qemu-devel] [RFC PATCH 3/3] hw/acpi: Extract build_mcfg

[Wei Yang]

On Wed, Mar 13, 2019 at 05:09:43PM +0100, Igor Mammedov wrote:
>On Wed, 13 Mar 2019 13:33:59 +0000
>Wei Yang <address@hidden> 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 <address@hidden> wrote:
>> >  
>> >> Now we have two identical build_mcfg function.
>> >> 
>> >> Extract them to aml-build.c.
>> >> 
>> >> Signed-off-by: Wei Yang <address@hidden>
>> >> ---
>> >>  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.
>

Seems got your idea.

>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.
>

OK, let me study the history first.

BTW, the legacy here is hardware specification level or qemu software
design level?

-- 
Wei Yang
Help you, Help me