[Qemu-devel] [RFC PATCH 0/3] Extract build_mcfg

[Qemu-devel] [RFC PATCH 0/3] Extract build_mcfg

[Wei Yang]

This patch set tries to generalize MCFG table build process.

[1]: use acpi_get_mcfg() to get necessary information to build MCFG
[2]: remove an unnecessary variable mcfg_start
[3]: Extract build_mcfg to aml-build.c

Wei Yang (3):
  hw/arm/virt-acpi-build: use acpi_get_mcfg() to calculate bus number
  hw/arm/virt-acpi-build: remove unnecessary variable mcfg_start
  hw/acpi: Extract build_mcfg

 hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 27 ++++++++++-----------------
 hw/i386/acpi-build.c        | 36 +-----------------------------------
 include/hw/acpi/aml-build.h |  6 ++++++
 4 files changed, 47 insertions(+), 52 deletions(-)

-- 
2.19.1

[Qemu-devel] [RFC PATCH 1/3] hw/arm/virt-acpi-build: use acpi_get_mcfg() to calculate bus number

[Wei Yang]

To build MCFG, two information is necessary:

    * bus number
    * base address

Abstract these two information to AcpiMcfgInfo so that build_mcfg and
build_mcfg_q35 will have the same declaration.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/arm/virt-acpi-build.c    | 28 ++++++++++++++++++++--------
 hw/i386/acpi-build.c        |  5 -----
 include/hw/acpi/aml-build.h |  5 +++++
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 57679a89bf..7713c2d809 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -545,23 +545,32 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                  "SRAT", table_data->len - srat_start, 3, NULL, NULL);
 }
 
-static void
-build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
+static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
 {
-    AcpiTableMcfg *mcfg;
+    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
     const MemMapEntry *memmap = vms->memmap;
     int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam);
+
+    mcfg->mcfg_base = memmap[ecam_id].base;
+    mcfg->mcfg_size = memmap[ecam_id].size;
+
+    return true;
+}
+
+static void
+build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
+{
+    AcpiTableMcfg *mcfg;
     int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
     int mcfg_start = table_data->len;
 
     mcfg = acpi_data_push(table_data, len);
-    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
+    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(memmap[ecam_id].size - 1);
+    mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1);
 
     build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
                  "MCFG", table_data->len - mcfg_start, 1, NULL, NULL);
@@ -779,6 +788,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     GArray *table_offsets;
     unsigned dsdt, xsdt;
+    AcpiMcfgInfo mcfg;
     GArray *tables_blob = tables->table_data;
 
     table_offsets = g_array_new(false, true /* clear */,
@@ -802,8 +812,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker, vms);
 
-    acpi_add_table(table_offsets, tables_blob);
-    build_mcfg(tables_blob, tables->linker, vms);
+    if (acpi_get_mcfg(&mcfg)) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_mcfg(tables_blob, tables->linker, &mcfg);
+    }
 
     acpi_add_table(table_offsets, tables_blob);
     build_spcr(tables_blob, tables->linker, vms);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9ecc96dcc7..c5b1c3be99 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -86,11 +86,6 @@
 /* Default IOAPIC ID */
 #define ACPI_BUILD_IOAPIC_ID 0x0
 
-typedef struct AcpiMcfgInfo {
-    uint64_t mcfg_base;
-    uint32_t mcfg_size;
-} AcpiMcfgInfo;
-
 typedef struct AcpiPmInfo {
     bool s3_disabled;
     bool s4_disabled;
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 1a563ad756..b63b85d67c 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -223,6 +223,11 @@ struct AcpiBuildTables {
     BIOSLinker *linker;
 } AcpiBuildTables;
 
+typedef struct AcpiMcfgInfo {
+    uint64_t mcfg_base;
+    uint32_t mcfg_size;
+} AcpiMcfgInfo;
+
 /**
  * init_aml_allocator:
  *
-- 
2.19.1

Re: [Qemu-arm] [RFC PATCH 1/3] hw/arm/virt-acpi-build: use acpi_get_mcfg() to calculate bus number

[Igor Mammedov]

On Wed, 13 Mar 2019 12:42:51 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> To build MCFG, two information is necessary:
> 
>     * bus number
>     * base address
> 
> Abstract these two information to AcpiMcfgInfo so that build_mcfg and
> build_mcfg_q35 will have the same declaration.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/arm/virt-acpi-build.c    | 28 ++++++++++++++++++++--------
>  hw/i386/acpi-build.c        |  5 -----
>  include/hw/acpi/aml-build.h |  5 +++++
>  3 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 57679a89bf..7713c2d809 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -545,23 +545,32 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                   "SRAT", table_data->len - srat_start, 3, NULL, NULL);
>  }
>  
> -static void
> -build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>  {
> -    AcpiTableMcfg *mcfg;
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
>      const MemMapEntry *memmap = vms->memmap;
>      int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam);
> +
> +    mcfg->mcfg_base = memmap[ecam_id].base;
> +    mcfg->mcfg_size = memmap[ecam_id].size;
> +
> +    return true;
> +}
> +
> +static void
> +build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
> +{
> +    AcpiTableMcfg *mcfg;
>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
>      int mcfg_start = table_data->len;
>  
>      mcfg = acpi_data_push(table_data, len);
> -    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
> +    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(memmap[ecam_id].size - 1);
> +    mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1);
>  
>      build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
>                   "MCFG", table_data->len - mcfg_start, 1, NULL, NULL);
> @@ -779,6 +788,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      GArray *table_offsets;
>      unsigned dsdt, xsdt;
> +    AcpiMcfgInfo mcfg;
>      GArray *tables_blob = tables->table_data;
>  
>      table_offsets = g_array_new(false, true /* clear */,
> @@ -802,8 +812,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_gtdt(tables_blob, tables->linker, vms);
>  
> -    acpi_add_table(table_offsets, tables_blob);
> -    build_mcfg(tables_blob, tables->linker, vms);
> +    if ( acpi_get_mcfg(&mcfg)) {
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_mcfg(tables_blob, tables->linker, &mcfg);
> +    }

acpi_get_mcfg used only once and always true so one doesn't need condition
I'd go for simpler:

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index d7e2e4885b..a28e04c249 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -803,8 +803,13 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     build_gtdt(tables_blob, tables->linker, vms);
 
     acpi_add_table(table_offsets, tables_blob);
-    build_mcfg(tables_blob, tables->linker, vms);
-
+    {
+        const AcpiTableMcfg mcfg = {
+           .mcfg_base = vms->memmap[VIRT_ECAM_ID(vms->highmem_ecam)].base;
+           .mcfg_size = vms->memmap[VIRT_ECAM_ID(vms->highmem_ecam)].size;
+        };
+        build_mcfg(tables_blob, tables->linker, &mcfg);
+    }
     acpi_add_table(table_offsets, tables_blob);
     build_spcr(tables_blob, tables->linker, vms);

and get rid of acpi_get_mcfg() helper function

>      acpi_add_table(table_offsets, tables_blob);
>      build_spcr(tables_blob, tables->linker, vms);
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9ecc96dcc7..c5b1c3be99 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -86,11 +86,6 @@
>  /* Default IOAPIC ID */
>  #define ACPI_BUILD_IOAPIC_ID 0x0
>  
> -typedef struct AcpiMcfgInfo {
> -    uint64_t mcfg_base;
> -    uint32_t mcfg_size;
> -} AcpiMcfgInfo;
> -
>  typedef struct AcpiPmInfo {
>      bool s3_disabled;
>      bool s4_disabled;
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 1a563ad756..b63b85d67c 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -223,6 +223,11 @@ struct AcpiBuildTables {
>      BIOSLinker *linker;
>  } AcpiBuildTables;
>  
> +typedef struct AcpiMcfgInfo {
> +    uint64_t mcfg_base;
> +    uint32_t mcfg_size;
mcfg_ prefix in fields probably isn't necessary.

> +} AcpiMcfgInfo;
> +
>  /**
>   * init_aml_allocator:
>   *

Re: [Qemu-devel] [RFC PATCH 1/3] hw/arm/virt-acpi-build: use acpi_get_mcfg() to calculate bus number

[Wei Yang]

On Wed, Mar 13, 2019 at 01:00:22PM +0100, Igor Mammedov wrote:
>On Wed, 13 Mar 2019 12:42:51 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> To build MCFG, two information is necessary:
>> 
>>     * bus number
>>     * base address
>> 
>> Abstract these two information to AcpiMcfgInfo so that build_mcfg and
>> build_mcfg_q35 will have the same declaration.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/arm/virt-acpi-build.c    | 28 ++++++++++++++++++++--------
>>  hw/i386/acpi-build.c        |  5 -----
>>  include/hw/acpi/aml-build.h |  5 +++++
>>  3 files changed, 25 insertions(+), 13 deletions(-)
>> 
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 57679a89bf..7713c2d809 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -545,23 +545,32 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>                   "SRAT", table_data->len - srat_start, 3, NULL, NULL);
>>  }
>>  
>> -static void
>> -build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>> +static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>>  {
>> -    AcpiTableMcfg *mcfg;
>> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
>>      const MemMapEntry *memmap = vms->memmap;
>>      int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam);
>> +
>> +    mcfg->mcfg_base = memmap[ecam_id].base;
>> +    mcfg->mcfg_size = memmap[ecam_id].size;
>> +
>> +    return true;
>> +}
>> +
>> +static void
>> +build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
>> +{
>> +    AcpiTableMcfg *mcfg;
>>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
>>      int mcfg_start = table_data->len;
>>  
>>      mcfg = acpi_data_push(table_data, len);
>> -    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
>> +    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(memmap[ecam_id].size - 1);
>> +    mcfg->allocation[0].end_bus_number = PCIE_MMCFG_BUS(info->mcfg_size - 1);
>>  
>>      build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
>>                   "MCFG", table_data->len - mcfg_start, 1, NULL, NULL);
>> @@ -779,6 +788,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>      GArray *table_offsets;
>>      unsigned dsdt, xsdt;
>> +    AcpiMcfgInfo mcfg;
>>      GArray *tables_blob = tables->table_data;
>>  
>>      table_offsets = g_array_new(false, true /* clear */,
>> @@ -802,8 +812,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>      acpi_add_table(table_offsets, tables_blob);
>>      build_gtdt(tables_blob, tables->linker, vms);
>>  
>> -    acpi_add_table(table_offsets, tables_blob);
>> -    build_mcfg(tables_blob, tables->linker, vms);
>> +    if ( acpi_get_mcfg(&mcfg)) {
>> +        acpi_add_table(table_offsets, tables_blob);
>> +        build_mcfg(tables_blob, tables->linker, &mcfg);
>> +    }
>
>acpi_get_mcfg used only once and always true so one doesn't need condition
>I'd go for simpler:
>
>diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>index d7e2e4885b..a28e04c249 100644
>--- a/hw/arm/virt-acpi-build.c
>+++ b/hw/arm/virt-acpi-build.c
>@@ -803,8 +803,13 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>     build_gtdt(tables_blob, tables->linker, vms);
> 
>     acpi_add_table(table_offsets, tables_blob);
>-    build_mcfg(tables_blob, tables->linker, vms);
>-
>+    {
>+        const AcpiTableMcfg mcfg = {
>+           .mcfg_base = vms->memmap[VIRT_ECAM_ID(vms->highmem_ecam)].base;
>+           .mcfg_size = vms->memmap[VIRT_ECAM_ID(vms->highmem_ecam)].size;
>+        };
>+        build_mcfg(tables_blob, tables->linker, &mcfg);
>+    }
>     acpi_add_table(table_offsets, tables_blob);
>     build_spcr(tables_blob, tables->linker, vms);
>
>and get rid of acpi_get_mcfg() helper function
>

That's reasonable.

>>      acpi_add_table(table_offsets, tables_blob);
>>      build_spcr(tables_blob, tables->linker, vms);
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 9ecc96dcc7..c5b1c3be99 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -86,11 +86,6 @@
>>  /* Default IOAPIC ID */
>>  #define ACPI_BUILD_IOAPIC_ID 0x0
>>  
>> -typedef struct AcpiMcfgInfo {
>> -    uint64_t mcfg_base;
>> -    uint32_t mcfg_size;
>> -} AcpiMcfgInfo;
>> -
>>  typedef struct AcpiPmInfo {
>>      bool s3_disabled;
>>      bool s4_disabled;
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 1a563ad756..b63b85d67c 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -223,6 +223,11 @@ struct AcpiBuildTables {
>>      BIOSLinker *linker;
>>  } AcpiBuildTables;
>>  
>> +typedef struct AcpiMcfgInfo {
>> +    uint64_t mcfg_base;
>> +    uint32_t mcfg_size;
>mcfg_ prefix in fields probably isn't necessary.
>

Hmm... I may split this to another patch to address.

>> +} AcpiMcfgInfo;
>> +
>>  /**
>>   * init_aml_allocator:
>>   *
>

-- 
Wei Yang
Help you, Help me

[Qemu-arm] [RFC PATCH 2/3] hw/arm/virt-acpi-build: remove unnecessary variable mcfg_start

[Wei Yang]

mcfg_start points to the start of MCFG table and is used in
build_header. While this information could be derived from mcfg.

This patch removes the unnecessary variable mcfg_start.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/arm/virt-acpi-build.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 7713c2d809..ae7858a79a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -562,18 +562,15 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
 {
     AcpiTableMcfg *mcfg;
     int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
-    int mcfg_start = table_data->len;
 
     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 *)(table_data->data + mcfg_start),
-                 "MCFG", table_data->len - mcfg_start, 1, NULL, NULL);
+    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
 }
 
 /* GTDT */
-- 
2.19.1

Re: [Qemu-arm] [RFC PATCH 2/3] hw/arm/virt-acpi-build: remove unnecessary variable mcfg_start

[Igor Mammedov]

On Wed, 13 Mar 2019 12:42:52 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> mcfg_start points to the start of MCFG table and is used in
> build_header. While this information could be derived from mcfg.
> 
> This patch removes the unnecessary variable mcfg_start.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/arm/virt-acpi-build.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 7713c2d809..ae7858a79a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -562,18 +562,15 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
>  {
>      AcpiTableMcfg *mcfg;
>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
> -    int mcfg_start = table_data->len;
>  
>      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 *)(table_data->data + mcfg_start),
> -                 "MCFG", table_data->len - mcfg_start, 1, NULL, NULL);
> +    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
>  }
>  
>  /* GTDT */

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

[Wei Yang]

Now we have two identical build_mcfg function.

Extract them to aml-build.c.

Signed-off-by: Wei Yang <richardw.yang@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
@@ -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";
+    }
+    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
-- 
2.19.1

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

[Igor Mammedov]

On Wed, 13 Mar 2019 12:42:53 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> Now we have two identical build_mcfg function.
> 
> Extract them to aml-build.c.
> 
> Signed-off-by: Wei Yang <richardw.yang@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.
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.

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

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

[Wei Yang]

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.yang@linux.intel.com> wrote:
>
>> Now we have two identical build_mcfg function.
>> 
>> Extract them to aml-build.c.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@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

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

[Igor Mammedov]

On Wed, 13 Mar 2019 13:33:59 +0000
Wei Yang <richard.weiyang@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.yang@linux.intel.com> wrote:
> >  
> >> Now we have two identical build_mcfg function.
> >> 
> >> Extract them to aml-build.c.
> >> 
> >> Signed-off-by: Wei Yang <richardw.yang@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  
> >  
> 

Re: [Qemu-arm] [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 <richard.weiyang@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.yang@linux.intel.com> wrote:
>> >  
>> >> Now we have two identical build_mcfg function.
>> >> 
>> >> Extract them to aml-build.c.
>> >> 
>> >> Signed-off-by: Wei Yang <richardw.yang@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.
>

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

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

[Igor Mammedov]

On Wed, 13 Mar 2019 21:31:37 +0000
Wei Yang <richard.weiyang@gmail.com> wrote:

> On Wed, Mar 13, 2019 at 05:09:43PM +0100, Igor Mammedov wrote:
> >On Wed, 13 Mar 2019 13:33:59 +0000
> >Wei Yang <richard.weiyang@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.yang@linux.intel.com> wrote:
> >> >    
> >> >> Now we have two identical build_mcfg function.
> >> >> 
> >> >> Extract them to aml-build.c.
> >> >> 
> >> >> Signed-off-by: Wei Yang <richardw.yang@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.
> >  
> 
> 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?
it's QEMU only, you need to find a version of QEMU (machine type)
which didn't have re-sizable MemoryRegion and the next version most likely
would have a knob somewhere in machine class definition saying that we don't
care about sizes anymore or care about sizes only for previous machine types.

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

[Wei Yang]

On Thu, Mar 14, 2019 at 10:18:30AM +0100, Igor Mammedov wrote:
>On Wed, 13 Mar 2019 21:31:37 +0000
>Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Wed, Mar 13, 2019 at 05:09:43PM +0100, Igor Mammedov wrote:
>> >On Wed, 13 Mar 2019 13:33:59 +0000
>> >Wei Yang <richard.weiyang@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.yang@linux.intel.com> wrote:
>> >> >    
>> >> >> Now we have two identical build_mcfg function.
>> >> >> 
>> >> >> Extract them to aml-build.c.
>> >> >> 
>> >> >> Signed-off-by: Wei Yang <richardw.yang@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.
>> >  
>> 
>> 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?
>it's QEMU only, you need to find a version of QEMU (machine type)
>which didn't have re-sizable MemoryRegion and the next version most likely
>would have a knob somewhere in machine class definition saying that we don't
>care about sizes anymore or care about sizes only for previous machine types.

Ok, let me find when we introduced re-sizable MemoryRegion.

Seems I still have some uncertain concepts. Let me discover it one by
one :-)

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Thu, Mar 14, 2019 at 10:18:30AM +0100, Igor Mammedov wrote:
>On Wed, 13 Mar 2019 21:31:37 +0000
>Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Wed, Mar 13, 2019 at 05:09:43PM +0100, Igor Mammedov wrote:
>> >On Wed, 13 Mar 2019 13:33:59 +0000
>> >Wei Yang <richard.weiyang@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.yang@linux.intel.com> wrote:
>> >> >    
>> >> >> Now we have two identical build_mcfg function.
>> >> >> 
>> >> >> Extract them to aml-build.c.
>> >> >> 
>> >> >> Signed-off-by: Wei Yang <richardw.yang@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 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?
>it's QEMU only, you need to find a version of QEMU (machine type)
>which didn't have re-sizable MemoryRegion and the next version most likely
>would have a knob somewhere in machine class definition saying that we don't
>care about sizes anymore or care about sizes only for previous machine types.

I have got something, but not sure this is correct. So I'd like to
summarize them here first.

The following commits are ordered in timeline.

1. First we introduced resizable MR.

    a1666142db 2015-01-08 acpi-build: make ROMs RAM blocks resizeable
    60786ef339 2015-01-08 memory: API to allocate resizeable RAM MR

2. Then acpi_ram_update in introduced

    339240b5cd 2015-04-27 acpi-build: remove dependency from ram_addr.h
    42d859001d 2015-02-26 acpi-build: fix ACPI RAM management

3. After that we introduce 2.4 machine type

    5cb50e0acc 2015-04-27 pc: add 2.4 machine types

But I don't find a comment stating we don't care about sizes anymore.

Below is my understanding of the dummy table history. 

Before resizable MR was introduced, the dummy table is used to be a
place holder so that we could add real table in acpi_build_update().
Othersize the new ACPI table is bigger to be placed into
build_state->table_mr.

If this is correct, after commit a1666142db ACPI table has the
capability to expand itself and not necessary to pre-allocate the dummy
table. Can I say after commit a1666142db, we don't need to put a fake
table in ACPI?

Another confusion is in the comment of build_mcfg(). In which aspect, it
relates to migration? Is this before resizable MR introduced?

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Sat, Mar 16, 2019 at 10:31:24AM +0000, Wei Yang wrote:
>On Thu, Mar 14, 2019 at 10:18:30AM +0100, Igor Mammedov wrote:
>>On Wed, 13 Mar 2019 21:31:37 +0000
>>Wei Yang <richard.weiyang@gmail.com> wrote:
>>
>>> On Wed, Mar 13, 2019 at 05:09:43PM +0100, Igor Mammedov wrote:
>>> >On Wed, 13 Mar 2019 13:33:59 +0000
>>> >Wei Yang <richard.weiyang@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.yang@linux.intel.com> wrote:
>>> >> >    
>>> >> >> Now we have two identical build_mcfg function.
>>> >> >> 
>>> >> >> Extract them to aml-build.c.
>>> >> >> 
>>> >> >> Signed-off-by: Wei Yang <richardw.yang@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 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?
>>it's QEMU only, you need to find a version of QEMU (machine type)
>>which didn't have re-sizable MemoryRegion and the next version most likely
>>would have a knob somewhere in machine class definition saying that we don't
>>care about sizes anymore or care about sizes only for previous machine types.
>
>I have got something, but not sure this is correct. So I'd like to
>summarize them here first.
>
>The following commits are ordered in timeline.
>
>1. First we introduced resizable MR.
>
>    a1666142db 2015-01-08 acpi-build: make ROMs RAM blocks resizeable
>    60786ef339 2015-01-08 memory: API to allocate resizeable RAM MR
>
>2. Then acpi_ram_update in introduced
>
>    339240b5cd 2015-04-27 acpi-build: remove dependency from ram_addr.h
>    42d859001d 2015-02-26 acpi-build: fix ACPI RAM management
>
>3. After that we introduce 2.4 machine type
>
>    5cb50e0acc 2015-04-27 pc: add 2.4 machine types
>
>But I don't find a comment stating we don't care about sizes anymore.
>
>Below is my understanding of the dummy table history. 
>
>Before resizable MR was introduced, the dummy table is used to be a
>place holder so that we could add real table in acpi_build_update().
>Othersize the new ACPI table is bigger to be placed into
>build_state->table_mr.
>
>If this is correct, after commit a1666142db ACPI table has the
>capability to expand itself and not necessary to pre-allocate the dummy
>table. Can I say after commit a1666142db, we don't need to put a fake
>table in ACPI?
>
>Another confusion is in the comment of build_mcfg(). In which aspect, it
>relates to migration? Is this before resizable MR introduced?
>

Hi, Igor

I guess you may be busy with other stuff. I'd appreciate it a lot if you could
spare some time slot to take a look at this one. Thanks :-)

Have a great day~

>-- 
>Wei Yang
>Help you, Help me

-- 
Wei Yang
Help you, Help me

Re: [Qemu-arm] [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 <richard.weiyang@gmail.com> wrote:
>
>> 
>> 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.
>

Hi, Igor

It took me sometime to go through the migration infrastructure.

Before continuing, I'd like to talk about what I understand to make sure my
direction is correct.

ACPI has a structure named AcpiBuildState, which contains all related
information. During migration, those data in AcpiBuildState should be
transferred to destination, e.g. table_mr, rsdp_mr and link_mr.

In the case related to mcfg, the problem lies in table_mr. And the reason
breaking migration is the size of table_mr is different between source and
destination.(This reason is a guess from those change logs and mails.)

The migration infrastructure has several SaveStateEntry to help migrate
different elements. The one with name "ram" take charge of RAMBlock. So this
SaveStateEntry and its ops is the next step for me to investigate. And from
this to see the effect of different size MemoryRegion during migration.

Is this sounds correct?

-- 
Wei Yang
Help you, Help me

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 <richard.weiyang@gmail.com> wrote:
>
>> 
>> 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.
>

Hi, Igor

It took me sometime to go through the migration infrastructure.

Before continuing, I'd like to talk about what I understand to make sure my
direction is correct.

ACPI has a structure named AcpiBuildState, which contains all related
information. During migration, those data in AcpiBuildState should be
transferred to destination, e.g. table_mr, rsdp_mr and link_mr.

In the case related to mcfg, the problem lies in table_mr. And the reason
breaking migration is the size of table_mr is different between source and
destination.(This reason is a guess from those change logs and mails.)

The migration infrastructure has several SaveStateEntry to help migrate
different elements. The one with name "ram" take charge of RAMBlock. So this
SaveStateEntry and its ops is the next step for me to investigate. And from
this to see the effect of different size MemoryRegion during migration.

Is this sounds correct?

-- 
Wei Yang
Help you, Help me

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

[Igor Mammedov]

On Tue, 2 Apr 2019 11:53:43 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Wed, Mar 13, 2019 at 05:09:43PM +0100, Igor Mammedov wrote:
> >On Wed, 13 Mar 2019 13:33:59 +0000
> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >  
> >> 
> >> 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.
> >  
> 
> Hi, Igor
> 
> It took me sometime to go through the migration infrastructure.
> 
> Before continuing, I'd like to talk about what I understand to make sure my
> direction is correct.
> 
> ACPI has a structure named AcpiBuildState, which contains all related
> information. During migration, those data in AcpiBuildState should be
> transferred to destination, e.g. table_mr, rsdp_mr and link_mr.
> 
> In the case related to mcfg, the problem lies in table_mr. And the reason
> breaking migration is the size of table_mr is different between source and
> destination.(This reason is a guess from those change logs and mails.)
This is about right if I recall it correctly.
 
> The migration infrastructure has several SaveStateEntry to help migrate
> different elements. The one with name "ram" take charge of RAMBlock. So this
> SaveStateEntry and its ops is the next step for me to investigate. And from
> this to see the effect of different size MemoryRegion during migration.
I don't think that you need to dig in migration mechanics so deep.
For our purpose of finding QEMU&machine version where migration between
size mismatched MemoryRegions (or RAMBlocks) is fixed is sufficient.

Aside from trying to grasp how migration works internally, you can try
to simulate[1] problem and bisect it to a commit that fixed it.
It still not easy due to amount of combinations you'd need to try,
but it's probably much easier than trying to figure out issue just
by reading code.

1) to stimulate you need to recreate conditions for table_mr jumping
from initial padded size to the next padded size after adding bridge
so you'd have reproducer which makes table_mr differ in size.

If I recall correctly in that time conditions were created by large
amount hotpluggble CPUs (maxcpus - CLI option) and then addidng
PCI bridges.
I'd just hack QEMU to print table_mr size in acpi_build_update()
to find coldplug CLI where we jump to the next padded size and then
use it for bisection with bridge[s] hotplug on source and migrating that
without reboot to target where hotplugged bridges are on CLI (one has to
configure target so it would have all devices that source has including
hotplugged ones)

> Is this sounds correct?
> 

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

[Igor Mammedov]

On Tue, 2 Apr 2019 11:53:43 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Wed, Mar 13, 2019 at 05:09:43PM +0100, Igor Mammedov wrote:
> >On Wed, 13 Mar 2019 13:33:59 +0000
> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >  
> >> 
> >> 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.
> >  
> 
> Hi, Igor
> 
> It took me sometime to go through the migration infrastructure.
> 
> Before continuing, I'd like to talk about what I understand to make sure my
> direction is correct.
> 
> ACPI has a structure named AcpiBuildState, which contains all related
> information. During migration, those data in AcpiBuildState should be
> transferred to destination, e.g. table_mr, rsdp_mr and link_mr.
> 
> In the case related to mcfg, the problem lies in table_mr. And the reason
> breaking migration is the size of table_mr is different between source and
> destination.(This reason is a guess from those change logs and mails.)
This is about right if I recall it correctly.
 
> The migration infrastructure has several SaveStateEntry to help migrate
> different elements. The one with name "ram" take charge of RAMBlock. So this
> SaveStateEntry and its ops is the next step for me to investigate. And from
> this to see the effect of different size MemoryRegion during migration.
I don't think that you need to dig in migration mechanics so deep.
For our purpose of finding QEMU&machine version where migration between
size mismatched MemoryRegions (or RAMBlocks) is fixed is sufficient.

Aside from trying to grasp how migration works internally, you can try
to simulate[1] problem and bisect it to a commit that fixed it.
It still not easy due to amount of combinations you'd need to try,
but it's probably much easier than trying to figure out issue just
by reading code.

1) to stimulate you need to recreate conditions for table_mr jumping
from initial padded size to the next padded size after adding bridge
so you'd have reproducer which makes table_mr differ in size.

If I recall correctly in that time conditions were created by large
amount hotpluggble CPUs (maxcpus - CLI option) and then addidng
PCI bridges.
I'd just hack QEMU to print table_mr size in acpi_build_update()
to find coldplug CLI where we jump to the next padded size and then
use it for bisection with bridge[s] hotplug on source and migrating that
without reboot to target where hotplugged bridges are on CLI (one has to
configure target so it would have all devices that source has including
hotplugged ones)

> Is this sounds correct?
> 

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

[Wei Yang]

On Tue, Apr 02, 2019 at 08:15:12AM +0200, Igor Mammedov wrote:
>On Tue, 2 Apr 2019 11:53:43 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
> 
>> The migration infrastructure has several SaveStateEntry to help migrate
>> different elements. The one with name "ram" take charge of RAMBlock. So this
>> SaveStateEntry and its ops is the next step for me to investigate. And from
>> this to see the effect of different size MemoryRegion during migration.
>I don't think that you need to dig in migration mechanics so deep.
>For our purpose of finding QEMU&machine version where migration between
>size mismatched MemoryRegions (or RAMBlocks) is fixed is sufficient.
>

ok.

>Aside from trying to grasp how migration works internally, you can try
>to simulate[1] problem and bisect it to a commit that fixed it.
>It still not easy due to amount of combinations you'd need to try,
>but it's probably much easier than trying to figure out issue just
>by reading code.
>
>1) to stimulate you need to recreate conditions for table_mr jumping
>from initial padded size to the next padded size after adding bridge
>so you'd have reproducer which makes table_mr differ in size.
>
>If I recall correctly in that time conditions were created by large
>amount hotpluggble CPUs (maxcpus - CLI option) and then addidng
>PCI bridges.
>I'd just hack QEMU to print table_mr size in acpi_build_update()
>to find coldplug CLI where we jump to the next padded size and then
>use it for bisection with bridge[s] hotplug on source and migrating that
>without reboot to target where hotplugged bridges are on CLI (one has to
>configure target so it would have all devices that source has including
>hotplugged ones)
>

Igor,

I got some confusion on how to re-produce this case. To be specific, how to 
expand table_mr from initial padded size to next padded size.

Let's see a normal hotplug case first. 

    I did one test to see the how a guest with hot-plug cpu migrate to
    destination.  It looks the migration fails if I just do hot-plug at
    source. So I have to do hot-plug both at source and at destination. This
    will expand the table_mr both at source and destination.

Then let's see the effect of hotplug more devices to exceed original padded
size. There are two cases, before re-sizable MemoryRegion and after.

1) Before re-sizable MemoryRegion introduced

    Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
    this size never gets bigger, if I am right. To be accurate, the table_blob
    would grow to next padded size if we hot-add more cpus/pci bridge, but we
    just copy the original size of MemoryRegion. Even without migration, the
    ACPI table is corrupted when we expand to next padded size.
    
    Is my understanding correct here?

2) After re-sizable MemoryRegion introduced

    This time both tabl_blob and MemoryRegion grows when it expand to next
    padded size. Since we need to hot-add device at both side, ACPI table
    grows at the same pace.

    Every thing looks good, until one of it exceed the resizable
    MemoryRegion's max size. (Not sure this is possible in reality, while
    possible in theory). Actually, this looks like case 1) when resizable
    MemoryRegion is not introduced. The too big ACPI table get corrupted.

So if my understanding is correct, the procedure you mentioned "expand from
initial padded size to next padded size" only applies to two different max
size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
table itself.

Then when we look at

    commit 07fb61760cdea7c3f1b9c897513986945bca8e89
    Author: Paolo Bonzini <pbonzini@redhat.com>
    Date:   Mon Jul 28 17:34:15 2014 +0200
    
        pc: hack for migration compatibility from QEMU 2.0
    
This fix ACPI migration issue before resizable MemoryRegion is
introduced(introduced in 2015-01-08). This looks expand to next padded size
always corrupt ACPI table at that time. And it make me think expand to next
padded size is not the procedure we should do?

And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
MemoryRegion has to be the same size at both side. So I guess the problem
doesn't lie in hotplug but in "main table" size difference.

For example, we have two version of Qemu: v1 and v2. Their "main table" size
is:

    v1: 3990
    v2: 4020

At this point, their ACPI table all padded to 4k, which is the same.

Then we create a machine with 1 more vcpu by these two versions. This will
expand the table to:

    v1: 4095
    v2: 4125

After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
migration is broken.

If this analysis is correct, the relationship between migration failure and
ACPI table is "the change of ACPI table size". Any size change of any
ACPI table would break migration. While of course, since we pad the table,
only some combinations of tables would result in a visible real size change in
MemoryRegion.

Then the principle for future ACPI development is to keep all ACPI table size
unchanged.

Now let's back to mcfg table. As the comment mentioned, guest could
enable/disable MCFG, so the code here reserve table no matter it is enabled or
not. This behavior ensures ACPI table size not changed. So do we need to find
the machine type as you suggested before?

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Tue, Apr 02, 2019 at 08:15:12AM +0200, Igor Mammedov wrote:
>On Tue, 2 Apr 2019 11:53:43 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
> 
>> The migration infrastructure has several SaveStateEntry to help migrate
>> different elements. The one with name "ram" take charge of RAMBlock. So this
>> SaveStateEntry and its ops is the next step for me to investigate. And from
>> this to see the effect of different size MemoryRegion during migration.
>I don't think that you need to dig in migration mechanics so deep.
>For our purpose of finding QEMU&machine version where migration between
>size mismatched MemoryRegions (or RAMBlocks) is fixed is sufficient.
>

ok.

>Aside from trying to grasp how migration works internally, you can try
>to simulate[1] problem and bisect it to a commit that fixed it.
>It still not easy due to amount of combinations you'd need to try,
>but it's probably much easier than trying to figure out issue just
>by reading code.
>
>1) to stimulate you need to recreate conditions for table_mr jumping
>from initial padded size to the next padded size after adding bridge
>so you'd have reproducer which makes table_mr differ in size.
>
>If I recall correctly in that time conditions were created by large
>amount hotpluggble CPUs (maxcpus - CLI option) and then addidng
>PCI bridges.
>I'd just hack QEMU to print table_mr size in acpi_build_update()
>to find coldplug CLI where we jump to the next padded size and then
>use it for bisection with bridge[s] hotplug on source and migrating that
>without reboot to target where hotplugged bridges are on CLI (one has to
>configure target so it would have all devices that source has including
>hotplugged ones)
>

Igor,

I got some confusion on how to re-produce this case. To be specific, how to 
expand table_mr from initial padded size to next padded size.

Let's see a normal hotplug case first. 

    I did one test to see the how a guest with hot-plug cpu migrate to
    destination.  It looks the migration fails if I just do hot-plug at
    source. So I have to do hot-plug both at source and at destination. This
    will expand the table_mr both at source and destination.

Then let's see the effect of hotplug more devices to exceed original padded
size. There are two cases, before re-sizable MemoryRegion and after.

1) Before re-sizable MemoryRegion introduced

    Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
    this size never gets bigger, if I am right. To be accurate, the table_blob
    would grow to next padded size if we hot-add more cpus/pci bridge, but we
    just copy the original size of MemoryRegion. Even without migration, the
    ACPI table is corrupted when we expand to next padded size.
    
    Is my understanding correct here?

2) After re-sizable MemoryRegion introduced

    This time both tabl_blob and MemoryRegion grows when it expand to next
    padded size. Since we need to hot-add device at both side, ACPI table
    grows at the same pace.

    Every thing looks good, until one of it exceed the resizable
    MemoryRegion's max size. (Not sure this is possible in reality, while
    possible in theory). Actually, this looks like case 1) when resizable
    MemoryRegion is not introduced. The too big ACPI table get corrupted.

So if my understanding is correct, the procedure you mentioned "expand from
initial padded size to next padded size" only applies to two different max
size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
table itself.

Then when we look at

    commit 07fb61760cdea7c3f1b9c897513986945bca8e89
    Author: Paolo Bonzini <pbonzini@redhat.com>
    Date:   Mon Jul 28 17:34:15 2014 +0200
    
        pc: hack for migration compatibility from QEMU 2.0
    
This fix ACPI migration issue before resizable MemoryRegion is
introduced(introduced in 2015-01-08). This looks expand to next padded size
always corrupt ACPI table at that time. And it make me think expand to next
padded size is not the procedure we should do?

And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
MemoryRegion has to be the same size at both side. So I guess the problem
doesn't lie in hotplug but in "main table" size difference.

For example, we have two version of Qemu: v1 and v2. Their "main table" size
is:

    v1: 3990
    v2: 4020

At this point, their ACPI table all padded to 4k, which is the same.

Then we create a machine with 1 more vcpu by these two versions. This will
expand the table to:

    v1: 4095
    v2: 4125

After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
migration is broken.

If this analysis is correct, the relationship between migration failure and
ACPI table is "the change of ACPI table size". Any size change of any
ACPI table would break migration. While of course, since we pad the table,
only some combinations of tables would result in a visible real size change in
MemoryRegion.

Then the principle for future ACPI development is to keep all ACPI table size
unchanged.

Now let's back to mcfg table. As the comment mentioned, guest could
enable/disable MCFG, so the code here reserve table no matter it is enabled or
not. This behavior ensures ACPI table size not changed. So do we need to find
the machine type as you suggested before?

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Tue, Apr 02, 2019 at 08:15:12AM +0200, Igor Mammedov wrote:
>On Tue, 2 Apr 2019 11:53:43 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
> 
>> The migration infrastructure has several SaveStateEntry to help migrate
>> different elements. The one with name "ram" take charge of RAMBlock. So this
>> SaveStateEntry and its ops is the next step for me to investigate. And from
>> this to see the effect of different size MemoryRegion during migration.
>I don't think that you need to dig in migration mechanics so deep.
>For our purpose of finding QEMU&machine version where migration between
>size mismatched MemoryRegions (or RAMBlocks) is fixed is sufficient.
>

ok.

>Aside from trying to grasp how migration works internally, you can try
>to simulate[1] problem and bisect it to a commit that fixed it.
>It still not easy due to amount of combinations you'd need to try,
>but it's probably much easier than trying to figure out issue just
>by reading code.
>
>1) to stimulate you need to recreate conditions for table_mr jumping
>from initial padded size to the next padded size after adding bridge
>so you'd have reproducer which makes table_mr differ in size.
>
>If I recall correctly in that time conditions were created by large
>amount hotpluggble CPUs (maxcpus - CLI option) and then addidng
>PCI bridges.
>I'd just hack QEMU to print table_mr size in acpi_build_update()
>to find coldplug CLI where we jump to the next padded size and then
>use it for bisection with bridge[s] hotplug on source and migrating that
>without reboot to target where hotplugged bridges are on CLI (one has to
>configure target so it would have all devices that source has including
>hotplugged ones)
>

Igor,

I got some confusion on how to re-produce this case. To be specific, how to 
expand table_mr from initial padded size to next padded size.

Let's see a normal hotplug case first. 

    I did one test to see the how a guest with hot-plug cpu migrate to
    destination.  It looks the migration fails if I just do hot-plug at
    source. So I have to do hot-plug both at source and at destination. This
    will expand the table_mr both at source and destination.

Then let's see the effect of hotplug more devices to exceed original padded
size. There are two cases, before re-sizable MemoryRegion and after.

1) Before re-sizable MemoryRegion introduced

    Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
    this size never gets bigger, if I am right. To be accurate, the table_blob
    would grow to next padded size if we hot-add more cpus/pci bridge, but we
    just copy the original size of MemoryRegion. Even without migration, the
    ACPI table is corrupted when we expand to next padded size.
    
    Is my understanding correct here?

2) After re-sizable MemoryRegion introduced

    This time both tabl_blob and MemoryRegion grows when it expand to next
    padded size. Since we need to hot-add device at both side, ACPI table
    grows at the same pace.

    Every thing looks good, until one of it exceed the resizable
    MemoryRegion's max size. (Not sure this is possible in reality, while
    possible in theory). Actually, this looks like case 1) when resizable
    MemoryRegion is not introduced. The too big ACPI table get corrupted.

So if my understanding is correct, the procedure you mentioned "expand from
initial padded size to next padded size" only applies to two different max
size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
table itself.

Then when we look at

    commit 07fb61760cdea7c3f1b9c897513986945bca8e89
    Author: Paolo Bonzini <pbonzini@redhat.com>
    Date:   Mon Jul 28 17:34:15 2014 +0200
    
        pc: hack for migration compatibility from QEMU 2.0
    
This fix ACPI migration issue before resizable MemoryRegion is
introduced(introduced in 2015-01-08). This looks expand to next padded size
always corrupt ACPI table at that time. And it make me think expand to next
padded size is not the procedure we should do?

And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
MemoryRegion has to be the same size at both side. So I guess the problem
doesn't lie in hotplug but in "main table" size difference.

For example, we have two version of Qemu: v1 and v2. Their "main table" size
is:

    v1: 3990
    v2: 4020

At this point, their ACPI table all padded to 4k, which is the same.

Then we create a machine with 1 more vcpu by these two versions. This will
expand the table to:

    v1: 4095
    v2: 4125

After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
migration is broken.

If this analysis is correct, the relationship between migration failure and
ACPI table is "the change of ACPI table size". Any size change of any
ACPI table would break migration. While of course, since we pad the table,
only some combinations of tables would result in a visible real size change in
MemoryRegion.

Then the principle for future ACPI development is to keep all ACPI table size
unchanged.

Now let's back to mcfg table. As the comment mentioned, guest could
enable/disable MCFG, so the code here reserve table no matter it is enabled or
not. This behavior ensures ACPI table size not changed. So do we need to find
the machine type as you suggested before?

-- 
Wei Yang
Help you, Help me

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

[Igor Mammedov]

On Fri, 5 Apr 2019 16:55:30 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Tue, Apr 02, 2019 at 08:15:12AM +0200, Igor Mammedov wrote:
> >On Tue, 2 Apr 2019 11:53:43 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >
> >   
> >> The migration infrastructure has several SaveStateEntry to help migrate
> >> different elements. The one with name "ram" take charge of RAMBlock. So this
> >> SaveStateEntry and its ops is the next step for me to investigate. And from
> >> this to see the effect of different size MemoryRegion during migration.  
> >I don't think that you need to dig in migration mechanics so deep.
> >For our purpose of finding QEMU&machine version where migration between
> >size mismatched MemoryRegions (or RAMBlocks) is fixed is sufficient.
> >  
> 
> ok.
> 
> >Aside from trying to grasp how migration works internally, you can try
> >to simulate[1] problem and bisect it to a commit that fixed it.
> >It still not easy due to amount of combinations you'd need to try,
> >but it's probably much easier than trying to figure out issue just
> >by reading code.
> >
> >1) to stimulate you need to recreate conditions for table_mr jumping
> >from initial padded size to the next padded size after adding bridge
> >so you'd have reproducer which makes table_mr differ in size.
> >
> >If I recall correctly in that time conditions were created by large
> >amount hotpluggble CPUs (maxcpus - CLI option) and then addidng
> >PCI bridges.
> >I'd just hack QEMU to print table_mr size in acpi_build_update()
> >to find coldplug CLI where we jump to the next padded size and then
> >use it for bisection with bridge[s] hotplug on source and migrating that
> >without reboot to target where hotplugged bridges are on CLI (one has to
> >configure target so it would have all devices that source has including
> >hotplugged ones)
> >  
> 
> Igor,
> 
> I got some confusion on how to re-produce this case. To be specific, how to 
> expand table_mr from initial padded size to next padded size.

here is reproducer:

$ qemu-system-x86_64-2.2 -M pc-i440fx-2.2 -smp 1,maxcpus=255 -monitor stdio `for i in {1..28}; do echo  "--device pci-bridge,chassis_nr=$i"; done` `for i in {1..24}; do printf --  " --device pci-bridge,bus=pci.1,addr=%x.0,chassis_nr=$i" $i ; done

## hotplug a bridge an migrate to file:

(qemu) device_add pci-bridge,bus=pci.1,addr=19.0,chassis_nr=25
(qemu) migrate "exec:gzip -c > STATEFILE.gz"

## exit and start destination

qemu-system-x86_64  -M pc-i440fx-2.2 -smp 1,maxcpus=255 -monitor stdio `for i in {1..28}; do echo  "--device pci-bridge,chassis_nr=$i"; done` `for i in {1..25}; do printf --  " --device pci-bridge,bus=pci.1,addr=%x.0,chassis_nr=$i" $i ; done`  -incoming "exec: gzip -c -d STATEFILE.gz"

# in case destination is QEMU-2.2 it will fail with:
  Length mismatch: /rom@etc/acpi/tables: 0x20000 in != 0x40000

# in case destination is QEMU-2.3 it will work as expected


> 
> Let's see a normal hotplug case first. 
> 
>     I did one test to see the how a guest with hot-plug cpu migrate to
>     destination.  It looks the migration fails if I just do hot-plug at
>     source. So I have to do hot-plug both at source and at destination. This
>     will expand the table_mr both at source and destination.
> 
> Then let's see the effect of hotplug more devices to exceed original padded
> size. There are two cases, before re-sizable MemoryRegion and after.
> 
> 1) Before re-sizable MemoryRegion introduced
> 
>     Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
>     this size never gets bigger, if I am right. To be accurate, the table_blob
>     would grow to next padded size if we hot-add more cpus/pci bridge, but we
>     just copy the original size of MemoryRegion. Even without migration, the
>     ACPI table is corrupted when we expand to next padded size.
>     
>     Is my understanding correct here?
> 
> 2) After re-sizable MemoryRegion introduced
> 
>     This time both tabl_blob and MemoryRegion grows when it expand to next
>     padded size. Since we need to hot-add device at both side, ACPI table
>     grows at the same pace.
> 
>     Every thing looks good, until one of it exceed the resizable
>     MemoryRegion's max size. (Not sure this is possible in reality, while
>     possible in theory). Actually, this looks like case 1) when resizable
>     MemoryRegion is not introduced. The too big ACPI table get corrupted.
> 
> So if my understanding is correct, the procedure you mentioned "expand from
> initial padded size to next padded size" only applies to two different max
> size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
> table itself.
> 
> Then when we look at
> 
>     commit 07fb61760cdea7c3f1b9c897513986945bca8e89
>     Author: Paolo Bonzini <pbonzini@redhat.com>
>     Date:   Mon Jul 28 17:34:15 2014 +0200
>     
>         pc: hack for migration compatibility from QEMU 2.0
>     
> This fix ACPI migration issue before resizable MemoryRegion is
> introduced(introduced in 2015-01-08). This looks expand to next padded size
> always corrupt ACPI table at that time. And it make me think expand to next
> padded size is not the procedure we should do?
> 
> And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
> MemoryRegion has to be the same size at both side. So I guess the problem
> doesn't lie in hotplug but in "main table" size difference.

It's true only for pre-resizable MemoryRegion QEMU versions,
after that size doesn't affect migration anymore.


> For example, we have two version of Qemu: v1 and v2. Their "main table" size
> is:
> 
>     v1: 3990
>     v2: 4020
> 
> At this point, their ACPI table all padded to 4k, which is the same.
> 
> Then we create a machine with 1 more vcpu by these two versions. This will
> expand the table to:
> 
>     v1: 4095
>     v2: 4125
> 
> After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
> migration is broken.
> 
> If this analysis is correct, the relationship between migration failure and
> ACPI table is "the change of ACPI table size". Any size change of any
you should make distinction between used_length and max_length here.
Migration puts on wire used_length and that's what matter for keeping migration
working.

> ACPI table would break migration. While of course, since we pad the table,
> only some combinations of tables would result in a visible real size change in
> MemoryRegion.
> 
> Then the principle for future ACPI development is to keep all ACPI table size
> unchanged.
once again it applies only for QEMU (versions < 2.1) and that was
the problem (i.e. there always would be configurations that would create
differently sized tables regardless of arbitrary size we would preallocate)
resizable MemoryRegions solved.
 
> Now let's back to mcfg table. As the comment mentioned, guest could
> enable/disable MCFG, so the code here reserve table no matter it is enabled or
> not. This behavior ensures ACPI table size not changed. So do we need to find
> the machine type as you suggested before?
We should be able to drop mcgf 'padding' hack since machine version
which was introduced in the QEMU version that introduced resizable MemoryRegion
as well.

I'll send a patch to address that

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

[Igor Mammedov]

On Fri, 5 Apr 2019 16:55:30 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Tue, Apr 02, 2019 at 08:15:12AM +0200, Igor Mammedov wrote:
> >On Tue, 2 Apr 2019 11:53:43 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >
> >   
> >> The migration infrastructure has several SaveStateEntry to help migrate
> >> different elements. The one with name "ram" take charge of RAMBlock. So this
> >> SaveStateEntry and its ops is the next step for me to investigate. And from
> >> this to see the effect of different size MemoryRegion during migration.  
> >I don't think that you need to dig in migration mechanics so deep.
> >For our purpose of finding QEMU&machine version where migration between
> >size mismatched MemoryRegions (or RAMBlocks) is fixed is sufficient.
> >  
> 
> ok.
> 
> >Aside from trying to grasp how migration works internally, you can try
> >to simulate[1] problem and bisect it to a commit that fixed it.
> >It still not easy due to amount of combinations you'd need to try,
> >but it's probably much easier than trying to figure out issue just
> >by reading code.
> >
> >1) to stimulate you need to recreate conditions for table_mr jumping
> >from initial padded size to the next padded size after adding bridge
> >so you'd have reproducer which makes table_mr differ in size.
> >
> >If I recall correctly in that time conditions were created by large
> >amount hotpluggble CPUs (maxcpus - CLI option) and then addidng
> >PCI bridges.
> >I'd just hack QEMU to print table_mr size in acpi_build_update()
> >to find coldplug CLI where we jump to the next padded size and then
> >use it for bisection with bridge[s] hotplug on source and migrating that
> >without reboot to target where hotplugged bridges are on CLI (one has to
> >configure target so it would have all devices that source has including
> >hotplugged ones)
> >  
> 
> Igor,
> 
> I got some confusion on how to re-produce this case. To be specific, how to 
> expand table_mr from initial padded size to next padded size.

here is reproducer:

$ qemu-system-x86_64-2.2 -M pc-i440fx-2.2 -smp 1,maxcpus=255 -monitor stdio `for i in {1..28}; do echo  "--device pci-bridge,chassis_nr=$i"; done` `for i in {1..24}; do printf --  " --device pci-bridge,bus=pci.1,addr=%x.0,chassis_nr=$i" $i ; done

## hotplug a bridge an migrate to file:

(qemu) device_add pci-bridge,bus=pci.1,addr=19.0,chassis_nr=25
(qemu) migrate "exec:gzip -c > STATEFILE.gz"

## exit and start destination

qemu-system-x86_64  -M pc-i440fx-2.2 -smp 1,maxcpus=255 -monitor stdio `for i in {1..28}; do echo  "--device pci-bridge,chassis_nr=$i"; done` `for i in {1..25}; do printf --  " --device pci-bridge,bus=pci.1,addr=%x.0,chassis_nr=$i" $i ; done`  -incoming "exec: gzip -c -d STATEFILE.gz"

# in case destination is QEMU-2.2 it will fail with:
  Length mismatch: /rom@etc/acpi/tables: 0x20000 in != 0x40000

# in case destination is QEMU-2.3 it will work as expected


> 
> Let's see a normal hotplug case first. 
> 
>     I did one test to see the how a guest with hot-plug cpu migrate to
>     destination.  It looks the migration fails if I just do hot-plug at
>     source. So I have to do hot-plug both at source and at destination. This
>     will expand the table_mr both at source and destination.
> 
> Then let's see the effect of hotplug more devices to exceed original padded
> size. There are two cases, before re-sizable MemoryRegion and after.
> 
> 1) Before re-sizable MemoryRegion introduced
> 
>     Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
>     this size never gets bigger, if I am right. To be accurate, the table_blob
>     would grow to next padded size if we hot-add more cpus/pci bridge, but we
>     just copy the original size of MemoryRegion. Even without migration, the
>     ACPI table is corrupted when we expand to next padded size.
>     
>     Is my understanding correct here?
> 
> 2) After re-sizable MemoryRegion introduced
> 
>     This time both tabl_blob and MemoryRegion grows when it expand to next
>     padded size. Since we need to hot-add device at both side, ACPI table
>     grows at the same pace.
> 
>     Every thing looks good, until one of it exceed the resizable
>     MemoryRegion's max size. (Not sure this is possible in reality, while
>     possible in theory). Actually, this looks like case 1) when resizable
>     MemoryRegion is not introduced. The too big ACPI table get corrupted.
> 
> So if my understanding is correct, the procedure you mentioned "expand from
> initial padded size to next padded size" only applies to two different max
> size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
> table itself.
> 
> Then when we look at
> 
>     commit 07fb61760cdea7c3f1b9c897513986945bca8e89
>     Author: Paolo Bonzini <pbonzini@redhat.com>
>     Date:   Mon Jul 28 17:34:15 2014 +0200
>     
>         pc: hack for migration compatibility from QEMU 2.0
>     
> This fix ACPI migration issue before resizable MemoryRegion is
> introduced(introduced in 2015-01-08). This looks expand to next padded size
> always corrupt ACPI table at that time. And it make me think expand to next
> padded size is not the procedure we should do?
> 
> And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
> MemoryRegion has to be the same size at both side. So I guess the problem
> doesn't lie in hotplug but in "main table" size difference.

It's true only for pre-resizable MemoryRegion QEMU versions,
after that size doesn't affect migration anymore.


> For example, we have two version of Qemu: v1 and v2. Their "main table" size
> is:
> 
>     v1: 3990
>     v2: 4020
> 
> At this point, their ACPI table all padded to 4k, which is the same.
> 
> Then we create a machine with 1 more vcpu by these two versions. This will
> expand the table to:
> 
>     v1: 4095
>     v2: 4125
> 
> After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
> migration is broken.
> 
> If this analysis is correct, the relationship between migration failure and
> ACPI table is "the change of ACPI table size". Any size change of any
you should make distinction between used_length and max_length here.
Migration puts on wire used_length and that's what matter for keeping migration
working.

> ACPI table would break migration. While of course, since we pad the table,
> only some combinations of tables would result in a visible real size change in
> MemoryRegion.
> 
> Then the principle for future ACPI development is to keep all ACPI table size
> unchanged.
once again it applies only for QEMU (versions < 2.1) and that was
the problem (i.e. there always would be configurations that would create
differently sized tables regardless of arbitrary size we would preallocate)
resizable MemoryRegions solved.
 
> Now let's back to mcfg table. As the comment mentioned, guest could
> enable/disable MCFG, so the code here reserve table no matter it is enabled or
> not. This behavior ensures ACPI table size not changed. So do we need to find
> the machine type as you suggested before?
We should be able to drop mcgf 'padding' hack since machine version
which was introduced in the QEMU version that introduced resizable MemoryRegion
as well.

I'll send a patch to address that

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

[Wei Yang]

On Tue, Apr 09, 2019 at 04:54:15PM +0200, Igor Mammedov wrote:
>> 
>> Let's see a normal hotplug case first. 
>> 
>>     I did one test to see the how a guest with hot-plug cpu migrate to
>>     destination.  It looks the migration fails if I just do hot-plug at
>>     source. So I have to do hot-plug both at source and at destination. This
>>     will expand the table_mr both at source and destination.
>> 
>> Then let's see the effect of hotplug more devices to exceed original padded
>> size. There are two cases, before re-sizable MemoryRegion and after.
>> 
>> 1) Before re-sizable MemoryRegion introduced
>> 
>>     Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
>>     this size never gets bigger, if I am right. To be accurate, the table_blob
>>     would grow to next padded size if we hot-add more cpus/pci bridge, but we
>>     just copy the original size of MemoryRegion. Even without migration, the
>>     ACPI table is corrupted when we expand to next padded size.
>>     
>>     Is my understanding correct here?
>> 
>> 2) After re-sizable MemoryRegion introduced
>> 
>>     This time both tabl_blob and MemoryRegion grows when it expand to next
>>     padded size. Since we need to hot-add device at both side, ACPI table
>>     grows at the same pace.
>> 
>>     Every thing looks good, until one of it exceed the resizable
>>     MemoryRegion's max size. (Not sure this is possible in reality, while
>>     possible in theory). Actually, this looks like case 1) when resizable
>>     MemoryRegion is not introduced. The too big ACPI table get corrupted.
>> 
>> So if my understanding is correct, the procedure you mentioned "expand from
>> initial padded size to next padded size" only applies to two different max
>> size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
>> table itself.
>> 
>> Then when we look at
>> 
>>     commit 07fb61760cdea7c3f1b9c897513986945bca8e89
>>     Author: Paolo Bonzini <pbonzini@redhat.com>
>>     Date:   Mon Jul 28 17:34:15 2014 +0200
>>     
>>         pc: hack for migration compatibility from QEMU 2.0
>>     
>> This fix ACPI migration issue before resizable MemoryRegion is
>> introduced(introduced in 2015-01-08). This looks expand to next padded size
>> always corrupt ACPI table at that time. And it make me think expand to next
>> padded size is not the procedure we should do?
>> 
>> And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
>> MemoryRegion has to be the same size at both side. So I guess the problem
>> doesn't lie in hotplug but in "main table" size difference.
>
>It's true only for pre-resizable MemoryRegion QEMU versions,
>after that size doesn't affect migration anymore.
>
>
>> For example, we have two version of Qemu: v1 and v2. Their "main table" size
>> is:
>> 
>>     v1: 3990
>>     v2: 4020
>> 
>> At this point, their ACPI table all padded to 4k, which is the same.
>> 
>> Then we create a machine with 1 more vcpu by these two versions. This will
>> expand the table to:
>> 
>>     v1: 4095
>>     v2: 4125
>> 
>> After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
>> migration is broken.
>> 
>> If this analysis is correct, the relationship between migration failure and
>> ACPI table is "the change of ACPI table size". Any size change of any
>you should make distinction between used_length and max_length here.
>Migration puts on wire used_length and that's what matter for keeping migration
>working.
>
>> ACPI table would break migration. While of course, since we pad the table,
>> only some combinations of tables would result in a visible real size change in
>> MemoryRegion.
>> 
>> Then the principle for future ACPI development is to keep all ACPI table size
>> unchanged.
>once again it applies only for QEMU (versions < 2.1) and that was
>the problem (i.e. there always would be configurations that would create
>differently sized tables regardless of arbitrary size we would preallocate)
>resizable MemoryRegions solved.
> 
>> Now let's back to mcfg table. As the comment mentioned, guest could
>> enable/disable MCFG, so the code here reserve table no matter it is enabled or
>> not. This behavior ensures ACPI table size not changed. So do we need to find
>> the machine type as you suggested before?
>We should be able to drop mcgf 'padding' hack since machine version
>which was introduced in the QEMU version that introduced resizable MemoryRegion
>as well.
>
>I'll send a patch to address that

Hi, Igor,

We have found the qemu version 2.1 which is with resizable MemoryRegion
enabled and q35 will stop support version before 2.3. The concern about ACPI
mcfg table breaking live migration is solved, right?

If so, I would prepare mcfg refactor patch based on your cleanup.

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Tue, Apr 09, 2019 at 04:54:15PM +0200, Igor Mammedov wrote:
>> 
>> Let's see a normal hotplug case first. 
>> 
>>     I did one test to see the how a guest with hot-plug cpu migrate to
>>     destination.  It looks the migration fails if I just do hot-plug at
>>     source. So I have to do hot-plug both at source and at destination. This
>>     will expand the table_mr both at source and destination.
>> 
>> Then let's see the effect of hotplug more devices to exceed original padded
>> size. There are two cases, before re-sizable MemoryRegion and after.
>> 
>> 1) Before re-sizable MemoryRegion introduced
>> 
>>     Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
>>     this size never gets bigger, if I am right. To be accurate, the table_blob
>>     would grow to next padded size if we hot-add more cpus/pci bridge, but we
>>     just copy the original size of MemoryRegion. Even without migration, the
>>     ACPI table is corrupted when we expand to next padded size.
>>     
>>     Is my understanding correct here?
>> 
>> 2) After re-sizable MemoryRegion introduced
>> 
>>     This time both tabl_blob and MemoryRegion grows when it expand to next
>>     padded size. Since we need to hot-add device at both side, ACPI table
>>     grows at the same pace.
>> 
>>     Every thing looks good, until one of it exceed the resizable
>>     MemoryRegion's max size. (Not sure this is possible in reality, while
>>     possible in theory). Actually, this looks like case 1) when resizable
>>     MemoryRegion is not introduced. The too big ACPI table get corrupted.
>> 
>> So if my understanding is correct, the procedure you mentioned "expand from
>> initial padded size to next padded size" only applies to two different max
>> size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
>> table itself.
>> 
>> Then when we look at
>> 
>>     commit 07fb61760cdea7c3f1b9c897513986945bca8e89
>>     Author: Paolo Bonzini <pbonzini@redhat.com>
>>     Date:   Mon Jul 28 17:34:15 2014 +0200
>>     
>>         pc: hack for migration compatibility from QEMU 2.0
>>     
>> This fix ACPI migration issue before resizable MemoryRegion is
>> introduced(introduced in 2015-01-08). This looks expand to next padded size
>> always corrupt ACPI table at that time. And it make me think expand to next
>> padded size is not the procedure we should do?
>> 
>> And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
>> MemoryRegion has to be the same size at both side. So I guess the problem
>> doesn't lie in hotplug but in "main table" size difference.
>
>It's true only for pre-resizable MemoryRegion QEMU versions,
>after that size doesn't affect migration anymore.
>
>
>> For example, we have two version of Qemu: v1 and v2. Their "main table" size
>> is:
>> 
>>     v1: 3990
>>     v2: 4020
>> 
>> At this point, their ACPI table all padded to 4k, which is the same.
>> 
>> Then we create a machine with 1 more vcpu by these two versions. This will
>> expand the table to:
>> 
>>     v1: 4095
>>     v2: 4125
>> 
>> After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
>> migration is broken.
>> 
>> If this analysis is correct, the relationship between migration failure and
>> ACPI table is "the change of ACPI table size". Any size change of any
>you should make distinction between used_length and max_length here.
>Migration puts on wire used_length and that's what matter for keeping migration
>working.
>
>> ACPI table would break migration. While of course, since we pad the table,
>> only some combinations of tables would result in a visible real size change in
>> MemoryRegion.
>> 
>> Then the principle for future ACPI development is to keep all ACPI table size
>> unchanged.
>once again it applies only for QEMU (versions < 2.1) and that was
>the problem (i.e. there always would be configurations that would create
>differently sized tables regardless of arbitrary size we would preallocate)
>resizable MemoryRegions solved.
> 
>> Now let's back to mcfg table. As the comment mentioned, guest could
>> enable/disable MCFG, so the code here reserve table no matter it is enabled or
>> not. This behavior ensures ACPI table size not changed. So do we need to find
>> the machine type as you suggested before?
>We should be able to drop mcgf 'padding' hack since machine version
>which was introduced in the QEMU version that introduced resizable MemoryRegion
>as well.
>
>I'll send a patch to address that

Hi, Igor,

We have found the qemu version 2.1 which is with resizable MemoryRegion
enabled and q35 will stop support version before 2.3. The concern about ACPI
mcfg table breaking live migration is solved, right?

If so, I would prepare mcfg refactor patch based on your cleanup.

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Tue, Apr 09, 2019 at 04:54:15PM +0200, Igor Mammedov wrote:
>> 
>> Let's see a normal hotplug case first. 
>> 
>>     I did one test to see the how a guest with hot-plug cpu migrate to
>>     destination.  It looks the migration fails if I just do hot-plug at
>>     source. So I have to do hot-plug both at source and at destination. This
>>     will expand the table_mr both at source and destination.
>> 
>> Then let's see the effect of hotplug more devices to exceed original padded
>> size. There are two cases, before re-sizable MemoryRegion and after.
>> 
>> 1) Before re-sizable MemoryRegion introduced
>> 
>>     Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
>>     this size never gets bigger, if I am right. To be accurate, the table_blob
>>     would grow to next padded size if we hot-add more cpus/pci bridge, but we
>>     just copy the original size of MemoryRegion. Even without migration, the
>>     ACPI table is corrupted when we expand to next padded size.
>>     
>>     Is my understanding correct here?
>> 
>> 2) After re-sizable MemoryRegion introduced
>> 
>>     This time both tabl_blob and MemoryRegion grows when it expand to next
>>     padded size. Since we need to hot-add device at both side, ACPI table
>>     grows at the same pace.
>> 
>>     Every thing looks good, until one of it exceed the resizable
>>     MemoryRegion's max size. (Not sure this is possible in reality, while
>>     possible in theory). Actually, this looks like case 1) when resizable
>>     MemoryRegion is not introduced. The too big ACPI table get corrupted.
>> 
>> So if my understanding is correct, the procedure you mentioned "expand from
>> initial padded size to next padded size" only applies to two different max
>> size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
>> table itself.
>> 
>> Then when we look at
>> 
>>     commit 07fb61760cdea7c3f1b9c897513986945bca8e89
>>     Author: Paolo Bonzini <pbonzini@redhat.com>
>>     Date:   Mon Jul 28 17:34:15 2014 +0200
>>     
>>         pc: hack for migration compatibility from QEMU 2.0
>>     
>> This fix ACPI migration issue before resizable MemoryRegion is
>> introduced(introduced in 2015-01-08). This looks expand to next padded size
>> always corrupt ACPI table at that time. And it make me think expand to next
>> padded size is not the procedure we should do?
>> 
>> And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
>> MemoryRegion has to be the same size at both side. So I guess the problem
>> doesn't lie in hotplug but in "main table" size difference.
>
>It's true only for pre-resizable MemoryRegion QEMU versions,
>after that size doesn't affect migration anymore.
>
>
>> For example, we have two version of Qemu: v1 and v2. Their "main table" size
>> is:
>> 
>>     v1: 3990
>>     v2: 4020
>> 
>> At this point, their ACPI table all padded to 4k, which is the same.
>> 
>> Then we create a machine with 1 more vcpu by these two versions. This will
>> expand the table to:
>> 
>>     v1: 4095
>>     v2: 4125
>> 
>> After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
>> migration is broken.
>> 
>> If this analysis is correct, the relationship between migration failure and
>> ACPI table is "the change of ACPI table size". Any size change of any
>you should make distinction between used_length and max_length here.
>Migration puts on wire used_length and that's what matter for keeping migration
>working.
>
>> ACPI table would break migration. While of course, since we pad the table,
>> only some combinations of tables would result in a visible real size change in
>> MemoryRegion.
>> 
>> Then the principle for future ACPI development is to keep all ACPI table size
>> unchanged.
>once again it applies only for QEMU (versions < 2.1) and that was
>the problem (i.e. there always would be configurations that would create
>differently sized tables regardless of arbitrary size we would preallocate)
>resizable MemoryRegions solved.
> 
>> Now let's back to mcfg table. As the comment mentioned, guest could
>> enable/disable MCFG, so the code here reserve table no matter it is enabled or
>> not. This behavior ensures ACPI table size not changed. So do we need to find
>> the machine type as you suggested before?
>We should be able to drop mcgf 'padding' hack since machine version
>which was introduced in the QEMU version that introduced resizable MemoryRegion
>as well.
>
>I'll send a patch to address that

Hi, Igor,

We have found the qemu version 2.1 which is with resizable MemoryRegion
enabled and q35 will stop support version before 2.3. The concern about ACPI
mcfg table breaking live migration is solved, right?

If so, I would prepare mcfg refactor patch based on your cleanup.

-- 
Wei Yang
Help you, Help me

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

[Igor Mammedov]

On Fri, 12 Apr 2019 13:44:07 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Tue, Apr 09, 2019 at 04:54:15PM +0200, Igor Mammedov wrote:
> >> 
> >> Let's see a normal hotplug case first. 
> >> 
> >>     I did one test to see the how a guest with hot-plug cpu migrate to
> >>     destination.  It looks the migration fails if I just do hot-plug at
> >>     source. So I have to do hot-plug both at source and at destination. This
> >>     will expand the table_mr both at source and destination.
> >> 
> >> Then let's see the effect of hotplug more devices to exceed original padded
> >> size. There are two cases, before re-sizable MemoryRegion and after.
> >> 
> >> 1) Before re-sizable MemoryRegion introduced
> >> 
> >>     Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
> >>     this size never gets bigger, if I am right. To be accurate, the table_blob
> >>     would grow to next padded size if we hot-add more cpus/pci bridge, but we
> >>     just copy the original size of MemoryRegion. Even without migration, the
> >>     ACPI table is corrupted when we expand to next padded size.
> >>     
> >>     Is my understanding correct here?
> >> 
> >> 2) After re-sizable MemoryRegion introduced
> >> 
> >>     This time both tabl_blob and MemoryRegion grows when it expand to next
> >>     padded size. Since we need to hot-add device at both side, ACPI table
> >>     grows at the same pace.
> >> 
> >>     Every thing looks good, until one of it exceed the resizable
> >>     MemoryRegion's max size. (Not sure this is possible in reality, while
> >>     possible in theory). Actually, this looks like case 1) when resizable
> >>     MemoryRegion is not introduced. The too big ACPI table get corrupted.
> >> 
> >> So if my understanding is correct, the procedure you mentioned "expand from
> >> initial padded size to next padded size" only applies to two different max
> >> size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
> >> table itself.
> >> 
> >> Then when we look at
> >> 
> >>     commit 07fb61760cdea7c3f1b9c897513986945bca8e89
> >>     Author: Paolo Bonzini <pbonzini@redhat.com>
> >>     Date:   Mon Jul 28 17:34:15 2014 +0200
> >>     
> >>         pc: hack for migration compatibility from QEMU 2.0
> >>     
> >> This fix ACPI migration issue before resizable MemoryRegion is
> >> introduced(introduced in 2015-01-08). This looks expand to next padded size
> >> always corrupt ACPI table at that time. And it make me think expand to next
> >> padded size is not the procedure we should do?
> >> 
> >> And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
> >> MemoryRegion has to be the same size at both side. So I guess the problem
> >> doesn't lie in hotplug but in "main table" size difference.  
> >
> >It's true only for pre-resizable MemoryRegion QEMU versions,
> >after that size doesn't affect migration anymore.
> >
> >  
> >> For example, we have two version of Qemu: v1 and v2. Their "main table" size
> >> is:
> >> 
> >>     v1: 3990
> >>     v2: 4020
> >> 
> >> At this point, their ACPI table all padded to 4k, which is the same.
> >> 
> >> Then we create a machine with 1 more vcpu by these two versions. This will
> >> expand the table to:
> >> 
> >>     v1: 4095
> >>     v2: 4125
> >> 
> >> After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
> >> migration is broken.
> >> 
> >> If this analysis is correct, the relationship between migration failure and
> >> ACPI table is "the change of ACPI table size". Any size change of any  
> >you should make distinction between used_length and max_length here.
> >Migration puts on wire used_length and that's what matter for keeping migration
> >working.
> >  
> >> ACPI table would break migration. While of course, since we pad the table,
> >> only some combinations of tables would result in a visible real size change in
> >> MemoryRegion.
> >> 
> >> Then the principle for future ACPI development is to keep all ACPI table size
> >> unchanged.  
> >once again it applies only for QEMU (versions < 2.1) and that was
> >the problem (i.e. there always would be configurations that would create
> >differently sized tables regardless of arbitrary size we would preallocate)
> >resizable MemoryRegions solved.
> >   
> >> Now let's back to mcfg table. As the comment mentioned, guest could
> >> enable/disable MCFG, so the code here reserve table no matter it is enabled or
> >> not. This behavior ensures ACPI table size not changed. So do we need to find
> >> the machine type as you suggested before?  
> >We should be able to drop mcgf 'padding' hack since machine version
> >which was introduced in the QEMU version that introduced resizable MemoryRegion
> >as well.
> >
> >I'll send a patch to address that  
> 
> Hi, Igor,
> 
> We have found the qemu version 2.1 which is with resizable MemoryRegion
> enabled and q35 will stop support version before 2.3. The concern about ACPI
> mcfg table breaking live migration is solved, right?
> 
> If so, I would prepare mcfg refactor patch based on your cleanup.
yes, just base your patches on top of
 "[PATCH for-4.1] q35: acpi: do not create dummy MCFG  table"

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

[Igor Mammedov]

On Fri, 12 Apr 2019 13:44:07 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Tue, Apr 09, 2019 at 04:54:15PM +0200, Igor Mammedov wrote:
> >> 
> >> Let's see a normal hotplug case first. 
> >> 
> >>     I did one test to see the how a guest with hot-plug cpu migrate to
> >>     destination.  It looks the migration fails if I just do hot-plug at
> >>     source. So I have to do hot-plug both at source and at destination. This
> >>     will expand the table_mr both at source and destination.
> >> 
> >> Then let's see the effect of hotplug more devices to exceed original padded
> >> size. There are two cases, before re-sizable MemoryRegion and after.
> >> 
> >> 1) Before re-sizable MemoryRegion introduced
> >> 
> >>     Before re-sizable MemoryRegion introduced, we just pad table_mr to 4K. And
> >>     this size never gets bigger, if I am right. To be accurate, the table_blob
> >>     would grow to next padded size if we hot-add more cpus/pci bridge, but we
> >>     just copy the original size of MemoryRegion. Even without migration, the
> >>     ACPI table is corrupted when we expand to next padded size.
> >>     
> >>     Is my understanding correct here?
> >> 
> >> 2) After re-sizable MemoryRegion introduced
> >> 
> >>     This time both tabl_blob and MemoryRegion grows when it expand to next
> >>     padded size. Since we need to hot-add device at both side, ACPI table
> >>     grows at the same pace.
> >> 
> >>     Every thing looks good, until one of it exceed the resizable
> >>     MemoryRegion's max size. (Not sure this is possible in reality, while
> >>     possible in theory). Actually, this looks like case 1) when resizable
> >>     MemoryRegion is not introduced. The too big ACPI table get corrupted.
> >> 
> >> So if my understanding is correct, the procedure you mentioned "expand from
> >> initial padded size to next padded size" only applies to two different max
> >> size resizable MemoryRegion. For other cases, the procedure corrupt the ACPI
> >> table itself.
> >> 
> >> Then when we look at
> >> 
> >>     commit 07fb61760cdea7c3f1b9c897513986945bca8e89
> >>     Author: Paolo Bonzini <pbonzini@redhat.com>
> >>     Date:   Mon Jul 28 17:34:15 2014 +0200
> >>     
> >>         pc: hack for migration compatibility from QEMU 2.0
> >>     
> >> This fix ACPI migration issue before resizable MemoryRegion is
> >> introduced(introduced in 2015-01-08). This looks expand to next padded size
> >> always corrupt ACPI table at that time. And it make me think expand to next
> >> padded size is not the procedure we should do?
> >> 
> >> And my colleague Wei Wang(in cc) mentioned, to make migration succeed, the
> >> MemoryRegion has to be the same size at both side. So I guess the problem
> >> doesn't lie in hotplug but in "main table" size difference.  
> >
> >It's true only for pre-resizable MemoryRegion QEMU versions,
> >after that size doesn't affect migration anymore.
> >
> >  
> >> For example, we have two version of Qemu: v1 and v2. Their "main table" size
> >> is:
> >> 
> >>     v1: 3990
> >>     v2: 4020
> >> 
> >> At this point, their ACPI table all padded to 4k, which is the same.
> >> 
> >> Then we create a machine with 1 more vcpu by these two versions. This will
> >> expand the table to:
> >> 
> >>     v1: 4095
> >>     v2: 4125
> >> 
> >> After padding, v1's ACPI table size is still 4k but v2's is 8k. Now the
> >> migration is broken.
> >> 
> >> If this analysis is correct, the relationship between migration failure and
> >> ACPI table is "the change of ACPI table size". Any size change of any  
> >you should make distinction between used_length and max_length here.
> >Migration puts on wire used_length and that's what matter for keeping migration
> >working.
> >  
> >> ACPI table would break migration. While of course, since we pad the table,
> >> only some combinations of tables would result in a visible real size change in
> >> MemoryRegion.
> >> 
> >> Then the principle for future ACPI development is to keep all ACPI table size
> >> unchanged.  
> >once again it applies only for QEMU (versions < 2.1) and that was
> >the problem (i.e. there always would be configurations that would create
> >differently sized tables regardless of arbitrary size we would preallocate)
> >resizable MemoryRegions solved.
> >   
> >> Now let's back to mcfg table. As the comment mentioned, guest could
> >> enable/disable MCFG, so the code here reserve table no matter it is enabled or
> >> not. This behavior ensures ACPI table size not changed. So do we need to find
> >> the machine type as you suggested before?  
> >We should be able to drop mcgf 'padding' hack since machine version
> >which was introduced in the QEMU version that introduced resizable MemoryRegion
> >as well.
> >
> >I'll send a patch to address that  
> 
> Hi, Igor,
> 
> We have found the qemu version 2.1 which is with resizable MemoryRegion
> enabled and q35 will stop support version before 2.3. The concern about ACPI
> mcfg table breaking live migration is solved, right?
> 
> If so, I would prepare mcfg refactor patch based on your cleanup.
yes, just base your patches on top of
 "[PATCH for-4.1] q35: acpi: do not create dummy MCFG  table"

Re: [Qemu-arm] [Qemu-devel] [RFC PATCH 0/3] Extract build_mcfg

[Igor Mammedov]

On Wed, 13 Mar 2019 12:42:50 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> This patch set tries to generalize MCFG table build process.
> 
> [1]: use acpi_get_mcfg() to get necessary information to build MCFG
> [2]: remove an unnecessary variable mcfg_start
> [3]: Extract build_mcfg to aml-build.c
> 
> Wei Yang (3):
>   hw/arm/virt-acpi-build: use acpi_get_mcfg() to calculate bus number
>   hw/arm/virt-acpi-build: remove unnecessary variable mcfg_start
>   hw/acpi: Extract build_mcfg

I'd add ano more patch on top
which converts from packed structure to build_append_int_noprefix() API

> 
>  hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    | 27 ++++++++++-----------------
>  hw/i386/acpi-build.c        | 36 +-----------------------------------
>  include/hw/acpi/aml-build.h |  6 ++++++
>  4 files changed, 47 insertions(+), 52 deletions(-)
> 

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

[Wei Yang]

On Wed, Mar 13, 2019 at 01:30:39PM +0100, Igor Mammedov wrote:
>On Wed, 13 Mar 2019 12:42:50 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> This patch set tries to generalize MCFG table build process.
>> 
>> [1]: use acpi_get_mcfg() to get necessary information to build MCFG
>> [2]: remove an unnecessary variable mcfg_start
>> [3]: Extract build_mcfg to aml-build.c
>> 
>> Wei Yang (3):
>>   hw/arm/virt-acpi-build: use acpi_get_mcfg() to calculate bus number
>>   hw/arm/virt-acpi-build: remove unnecessary variable mcfg_start
>>   hw/acpi: Extract build_mcfg
>
>I'd add ano more patch on top
>which converts from packed structure to build_append_int_noprefix() API
>

You mean get rid of type AcpiTableMcfg, and write raw data to blob with
build_append_int_noprefix()?

I don't get the benefits of doing so.

>> 
>>  hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
>>  hw/arm/virt-acpi-build.c    | 27 ++++++++++-----------------
>>  hw/i386/acpi-build.c        | 36 +-----------------------------------
>>  include/hw/acpi/aml-build.h |  6 ++++++
>>  4 files changed, 47 insertions(+), 52 deletions(-)
>> 
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-arm] [Qemu-devel] [RFC PATCH 0/3] Extract build_mcfg

[Igor Mammedov]

On Wed, 13 Mar 2019 13:24:48 +0000
Wei Yang <richard.weiyang@gmail.com> wrote:

> On Wed, Mar 13, 2019 at 01:30:39PM +0100, Igor Mammedov wrote:
> >On Wed, 13 Mar 2019 12:42:50 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >  
> >> This patch set tries to generalize MCFG table build process.
> >> 
> >> [1]: use acpi_get_mcfg() to get necessary information to build MCFG
> >> [2]: remove an unnecessary variable mcfg_start
> >> [3]: Extract build_mcfg to aml-build.c
> >> 
> >> Wei Yang (3):
> >>   hw/arm/virt-acpi-build: use acpi_get_mcfg() to calculate bus number
> >>   hw/arm/virt-acpi-build: remove unnecessary variable mcfg_start
> >>   hw/acpi: Extract build_mcfg  
> >
> >I'd add ano more patch on top
> >which converts from packed structure to build_append_int_noprefix() API
> >  
> 
> You mean get rid of type AcpiTableMcfg, and write raw data to blob with
> build_append_int_noprefix()?
yes,

> 
> I don't get the benefits of doing so.
see commit 5d7a334f7

 
> >> 
> >>  hw/acpi/aml-build.c         | 30 ++++++++++++++++++++++++++++++
> >>  hw/arm/virt-acpi-build.c    | 27 ++++++++++-----------------
> >>  hw/i386/acpi-build.c        | 36 +-----------------------------------
> >>  include/hw/acpi/aml-build.h |  6 ++++++
> >>  4 files changed, 47 insertions(+), 52 deletions(-)
> >>   
> >  
>