[Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table

Igor Mammedov posted 1 patch 4 years, 11 months ago
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1554822037-329838-1-git-send-email-imammedo@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Igor Mammedov <imammedo@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <rth@twiddle.net>
hw/i386/acpi-build.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
[Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Igor Mammedov 4 years, 11 months ago
Dummy table (with signature "QEMU") creation came from original SeaBIOS
codebase. And QEMU would have to keep it around if there were Q35 machine
that depended on keeping ACPI tables blob constant size. Luckily there
were no versioned Q35 machine types before commit:
  (since 2.3) a1666142db acpi-build: make ROMs RAM blocks resizeable
which obsoleted need to keep ACPI tables blob the same size on source/destination.

Considering the 1st versioned machine is pc-q35-2.4, the dummy table
is not really necessary and it's safe to drop it without breaking
cross version migration in both directions unconditionally.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 416da31..8671e25 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2401,7 +2401,6 @@ 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);
@@ -2411,19 +2410,7 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
     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);
+    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
 }
 
 /*
@@ -2592,6 +2579,9 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
     }
     mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
     qobject_unref(o);
+    if (mcfg->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) {
+        return false;
+    }
 
     o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
     assert(o);
-- 
2.7.4


Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Wei Yang 4 years, 11 months ago
On Tue, Apr 09, 2019 at 05:00:37PM +0200, Igor Mammedov wrote:
>Dummy table (with signature "QEMU") creation came from original SeaBIOS
>codebase. And QEMU would have to keep it around if there were Q35 machine
>that depended on keeping ACPI tables blob constant size. Luckily there
>were no versioned Q35 machine types before commit:
>  (since 2.3) a1666142db acpi-build: make ROMs RAM blocks resizeable
>which obsoleted need to keep ACPI tables blob the same size on source/destination.
>

I am not sure getting the "versioned Q35" correctly. Seems originally there is
q35-1.4?

commit bf3caa3dc17552b323cec6831301a22cfc98ecd5
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Fri Feb 8 14:06:15 2013 +0100

    pc: add compatibility machine types for 1.4
    
    Adds both pc-i440fx-1.4 and pc-q35-1.4.



-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Igor Mammedov 4 years, 11 months ago
On Wed, 10 Apr 2019 09:12:31 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Tue, Apr 09, 2019 at 05:00:37PM +0200, Igor Mammedov wrote:
> >Dummy table (with signature "QEMU") creation came from original SeaBIOS
> >codebase. And QEMU would have to keep it around if there were Q35 machine
> >that depended on keeping ACPI tables blob constant size. Luckily there
> >were no versioned Q35 machine types before commit:
> >  (since 2.3) a1666142db acpi-build: make ROMs RAM blocks resizeable
> >which obsoleted need to keep ACPI tables blob the same size on source/destination.
> >  
> 
> I am not sure getting the "versioned Q35" correctly. Seems originally there is
> q35-1.4?
> 
> commit bf3caa3dc17552b323cec6831301a22cfc98ecd5
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Fri Feb 8 14:06:15 2013 +0100
> 
>     pc: add compatibility machine types for 1.4
>     
>     Adds both pc-i440fx-1.4 and pc-q35-1.4.

current upstream has only pc-q35-2.4 as earliest versioned Q35
machine type (try to run: qemu-system-x86_64 -M help)

Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Wei Yang 4 years, 11 months ago
On Wed, Apr 10, 2019 at 11:08:33AM +0200, Igor Mammedov wrote:
>On Wed, 10 Apr 2019 09:12:31 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Tue, Apr 09, 2019 at 05:00:37PM +0200, Igor Mammedov wrote:
>> >Dummy table (with signature "QEMU") creation came from original SeaBIOS
>> >codebase. And QEMU would have to keep it around if there were Q35 machine
>> >that depended on keeping ACPI tables blob constant size. Luckily there
>> >were no versioned Q35 machine types before commit:
>> >  (since 2.3) a1666142db acpi-build: make ROMs RAM blocks resizeable
>> >which obsoleted need to keep ACPI tables blob the same size on source/destination.
>> >  
>> 
>> I am not sure getting the "versioned Q35" correctly. Seems originally there is
>> q35-1.4?
>> 
>> commit bf3caa3dc17552b323cec6831301a22cfc98ecd5
>> Author: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Fri Feb 8 14:06:15 2013 +0100
>> 
>>     pc: add compatibility machine types for 1.4
>>     
>>     Adds both pc-i440fx-1.4 and pc-q35-1.4.
>
>current upstream has only pc-q35-2.4 as earliest versioned Q35
>machine type (try to run: qemu-system-x86_64 -M help)

Yes, I see those old type are removed. This means we don't want to support
those old types, right? Because those old types can't be migrated to latest
upstream qemu.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Igor Mammedov 4 years, 11 months ago
On Wed, 10 Apr 2019 22:01:53 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Wed, Apr 10, 2019 at 11:08:33AM +0200, Igor Mammedov wrote:
> >On Wed, 10 Apr 2019 09:12:31 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >  
> >> On Tue, Apr 09, 2019 at 05:00:37PM +0200, Igor Mammedov wrote:  
> >> >Dummy table (with signature "QEMU") creation came from original SeaBIOS
> >> >codebase. And QEMU would have to keep it around if there were Q35 machine
> >> >that depended on keeping ACPI tables blob constant size. Luckily there
> >> >were no versioned Q35 machine types before commit:
> >> >  (since 2.3) a1666142db acpi-build: make ROMs RAM blocks resizeable
> >> >which obsoleted need to keep ACPI tables blob the same size on source/destination.
> >> >    
> >> 
> >> I am not sure getting the "versioned Q35" correctly. Seems originally there is
> >> q35-1.4?
> >> 
> >> commit bf3caa3dc17552b323cec6831301a22cfc98ecd5
> >> Author: Paolo Bonzini <pbonzini@redhat.com>
> >> Date:   Fri Feb 8 14:06:15 2013 +0100
> >> 
> >>     pc: add compatibility machine types for 1.4
> >>     
> >>     Adds both pc-i440fx-1.4 and pc-q35-1.4.  
> >
> >current upstream has only pc-q35-2.4 as earliest versioned Q35
> >machine type (try to run: qemu-system-x86_64 -M help)  
> 
> Yes, I see those old type are removed. This means we don't want to support
> those old types, right? Because those old types can't be migrated to latest
> upstream qemu.

Exactly


Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Wei Yang 4 years, 11 months ago
On Tue, Apr 09, 2019 at 05:00:37PM +0200, Igor Mammedov wrote:
>Dummy table (with signature "QEMU") creation came from original SeaBIOS
>codebase. And QEMU would have to keep it around if there were Q35 machine
>that depended on keeping ACPI tables blob constant size. Luckily there
>were no versioned Q35 machine types before commit:
>  (since 2.3) a1666142db acpi-build: make ROMs RAM blocks resizeable
>which obsoleted need to keep ACPI tables blob the same size on source/destination.

I think we should keep table blob the same size on source/destination.

But with resizable MemoryRegion, we don't need to reserve some dummy space.
Because we can expand it later.

>
>Considering the 1st versioned machine is pc-q35-2.4, the dummy table
>is not really necessary and it's safe to drop it without breaking
>cross version migration in both directions unconditionally.
>
>Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>---
> hw/i386/acpi-build.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>index 416da31..8671e25 100644
>--- a/hw/i386/acpi-build.c
>+++ b/hw/i386/acpi-build.c
>@@ -2401,7 +2401,6 @@ 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);
>@@ -2411,19 +2410,7 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
>     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.

I want to cnfirm what is "enabled or disabled by guest" here.

If we don't reserve mcfg and "guest" enable mcfg during running, the ACPI
table size changed. But the destination still has the original table size,
since destination "guest" keep sleep during this period.

Now the migration would face table size difference and break migration?

>-     * 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);
>+    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
> }
> 
> /*
>@@ -2592,6 +2579,9 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>     }
>     mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
>     qobject_unref(o);
>+    if (mcfg->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) {
>+        return false;
>+    }
> 
>     o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
>     assert(o);
>-- 
>2.7.4

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Igor Mammedov 4 years, 11 months ago
On Wed, 10 Apr 2019 22:27:56 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

[...]
> >@@ -2411,19 +2410,7 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
> >     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.  
> 
> I want to cnfirm what is "enabled or disabled by guest" here.

Firmware theoretically during PCI initialization may disable ECAM support
and that's when we do no need MCFG. In practice that's not happening
(SeaBIOS or UEFI) but we in case there is out there a firmware that does
disable ECAM we do not generate MCFG.

Note:
ACPI tables generated twice, 1st when QEMU starts and the second time
when firmware accesses fwcfg to read blobs for the 1st time.
The later happens after PCI subsystem was initialized by firmware.
At that time we know if ECAM was enabled or not.

> If we don't reserve mcfg and "guest" enable mcfg during running, the ACPI
> table size changed. But the destination still has the original table size,
> since destination "guest" keep sleep during this period.
> 
> Now the migration would face table size difference

with commit a1666142db we do not care as all the tables created on
source will be migrated to destination as is overwriting whatever blobs
destination created on startup.

> and break migration?
nope,

to help you figure out why it works
look at what following git commits did:
  git log c8d6f66ae7..a1666142db
and pay attention to 'used_length'

> 
> >-     * 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);
> >+    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
> > }
> > 
> > /*
> >@@ -2592,6 +2579,9 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> >     }
> >     mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
> >     qobject_unref(o);
> >+    if (mcfg->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) {
> >+        return false;
> >+    }
> > 
> >     o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
> >     assert(o);
> >-- 
> >2.7.4  
> 


Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Wei Yang 4 years, 11 months ago
On Wed, Apr 10, 2019 at 05:01:50PM +0200, Igor Mammedov wrote:
>On Wed, 10 Apr 2019 22:27:56 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>[...]
>> >@@ -2411,19 +2410,7 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
>> >     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.  
>> 
>> I want to cnfirm what is "enabled or disabled by guest" here.
>
>Firmware theoretically during PCI initialization may disable ECAM support
>and that's when we do no need MCFG. In practice that's not happening
>(SeaBIOS or UEFI) but we in case there is out there a firmware that does
>disable ECAM we do not generate MCFG.
>
>Note:
>ACPI tables generated twice, 1st when QEMU starts and the second time
>when firmware accesses fwcfg to read blobs for the 1st time.
>The later happens after PCI subsystem was initialized by firmware.
>At that time we know if ECAM was enabled or not.
>

That's much clear, thanks :-)

So this is the guest BIOS instead of guest kernel who may disable/enable it.

>> If we don't reserve mcfg and "guest" enable mcfg during running, the ACPI
>> table size changed. But the destination still has the original table size,
>> since destination "guest" keep sleep during this period.
>> 
>> Now the migration would face table size difference
>
>with commit a1666142db we do not care as all the tables created on
>source will be migrated to destination as is overwriting whatever blobs
>destination created on startup.
>
>> and break migration?
>nope,
>
>to help you figure out why it works
>look at what following git commits did:
>  git log c8d6f66ae7..a1666142db
>and pay attention to 'used_length'
>

To be honest, this is what I feel confused in your previous reply.

First I want to confirm both fields in RAMBlock affects the migration:

* used_length
* max_length

Both of them should be the same on both source/destination, otherwise the
migration would fail.

Then I thought the migration would be broken if source/destination has
different knowledge about acpi table size. Because this will introduce
different value of used_length, even we have resizable MemoryRegion.

The 1st time ACPI generation flow:

    acpi_add_rom_blob
        rom_add_blob
            rom_set_mr
                memory_region_init_resizable_ram
                    qemu_ram_alloc_resizable
    		        new_block->used_length = size
    		        new_block->max_length = max_size

The 2nd time ACPI generation flow:

    acpi_ram_update
        memory_regioin_ram_resize
            qemu_ram_resize
                block->used_length = new_size

The max_length is always the same, while used_length would be changed to the
actual table_blob size.

In case source/destination has different knowledge about acpi table size, the
table_blob size(even after aligned) could be different.

This is why I thought there is still some chance to break migration after
resizable MemoryRegion.

Do I miss something?

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Igor Mammedov 4 years, 11 months ago
On Thu, 11 Apr 2019 09:32:11 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Wed, Apr 10, 2019 at 05:01:50PM +0200, Igor Mammedov wrote:
> >On Wed, 10 Apr 2019 22:27:56 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >
> >[...]  
> >> >@@ -2411,19 +2410,7 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
> >> >     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.    
> >> 
> >> I want to cnfirm what is "enabled or disabled by guest" here.  
> >
> >Firmware theoretically during PCI initialization may disable ECAM support
> >and that's when we do no need MCFG. In practice that's not happening
> >(SeaBIOS or UEFI) but we in case there is out there a firmware that does
> >disable ECAM we do not generate MCFG.
> >
> >Note:
> >ACPI tables generated twice, 1st when QEMU starts and the second time
> >when firmware accesses fwcfg to read blobs for the 1st time.
> >The later happens after PCI subsystem was initialized by firmware.
> >At that time we know if ECAM was enabled or not.
> >  
> 
> That's much clear, thanks :-)
> 
> So this is the guest BIOS instead of guest kernel who may disable/enable it.
> 
> >> If we don't reserve mcfg and "guest" enable mcfg during running, the ACPI
> >> table size changed. But the destination still has the original table size,
> >> since destination "guest" keep sleep during this period.
> >> 
> >> Now the migration would face table size difference  
> >
> >with commit a1666142db we do not care as all the tables created on
> >source will be migrated to destination as is overwriting whatever blobs
> >destination created on startup.
> >  
> >> and break migration?  
> >nope,
> >
> >to help you figure out why it works
> >look at what following git commits did:
> >  git log c8d6f66ae7..a1666142db
> >and pay attention to 'used_length'
> >  
> 
> To be honest, this is what I feel confused in your previous reply.
> 
> First I want to confirm both fields in RAMBlock affects the migration:
> 
> * used_length
> * max_length
> 
> Both of them should be the same on both source/destination, otherwise the
> migration would fail.
well, it works fine for me.
Where do you see max_length being used during migration?


> Then I thought the migration would be broken if source/destination has
> different knowledge about acpi table size. Because this will introduce
> different value of used_length, even we have resizable MemoryRegion.
> 
> The 1st time ACPI generation flow:
> 
>     acpi_add_rom_blob
>         rom_add_blob
>             rom_set_mr
>                 memory_region_init_resizable_ram
>                     qemu_ram_alloc_resizable
>     		        new_block->used_length = size
>     		        new_block->max_length = max_size
> 
> The 2nd time ACPI generation flow:
> 
>     acpi_ram_update
>         memory_regioin_ram_resize
>             qemu_ram_resize
>                 block->used_length = new_size
> 
> The max_length is always the same, while used_length would be changed to the
> actual table_blob size.
> 
> In case source/destination has different knowledge about acpi table size, the
> table_blob size(even after aligned) could be different.
> 
> This is why I thought there is still some chance to break migration after
> resizable MemoryRegion.
> 
> Do I miss something?
yes, you did, max_length does not influence migration stream.
see what above mentioned commits and ram_load() -> "if (length != block->used_length)" do.



Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Wei Yang 4 years, 11 months ago
On Thu, Apr 11, 2019 at 01:46:27PM +0200, Igor Mammedov wrote:
>On Thu, 11 Apr 2019 09:32:11 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>

[...]

>> To be honest, this is what I feel confused in your previous reply.
>> 
>> First I want to confirm both fields in RAMBlock affects the migration:
>> 
>> * used_length
>> * max_length
>> 
>> Both of them should be the same on both source/destination, otherwise the
>> migration would fail.
>well, it works fine for me.
>Where do you see max_length being used during migration?
>

I asked my colleague, but seems you are right.

>
>> Then I thought the migration would be broken if source/destination has
>> different knowledge about acpi table size. Because this will introduce
>> different value of used_length, even we have resizable MemoryRegion.
>> 
>> The 1st time ACPI generation flow:
>> 
>>     acpi_add_rom_blob
>>         rom_add_blob
>>             rom_set_mr
>>                 memory_region_init_resizable_ram
>>                     qemu_ram_alloc_resizable
>>     		        new_block->used_length = size
>>     		        new_block->max_length = max_size
>> 
>> The 2nd time ACPI generation flow:
>> 
>>     acpi_ram_update
>>         memory_regioin_ram_resize
>>             qemu_ram_resize
>>                 block->used_length = new_size
>> 
>> The max_length is always the same, while used_length would be changed to the
>> actual table_blob size.
>> 
>> In case source/destination has different knowledge about acpi table size, the
>> table_blob size(even after aligned) could be different.
>> 
>> This is why I thought there is still some chance to break migration after
>> resizable MemoryRegion.
>> 
>> Do I miss something?
>yes, you did, max_length does not influence migration stream.
>see what above mentioned commits and ram_load() -> "if (length != block->used_length)" do.
>

I see this in the code.

So the destination will check length and adjust self's length if it
doesn't equal to the source.

Thanks :-)

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH for-4.1] q35: acpi: do not create dummy MCFG table
Posted by Wei Yang 4 years, 11 months ago
On Tue, Apr 09, 2019 at 05:00:37PM +0200, Igor Mammedov wrote:
>Dummy table (with signature "QEMU") creation came from original SeaBIOS
>codebase. And QEMU would have to keep it around if there were Q35 machine
>that depended on keeping ACPI tables blob constant size. Luckily there
>were no versioned Q35 machine types before commit:
>  (since 2.3) a1666142db acpi-build: make ROMs RAM blocks resizeable
>which obsoleted need to keep ACPI tables blob the same size on source/destination.
>
>Considering the 1st versioned machine is pc-q35-2.4, the dummy table
>is not really necessary and it's safe to drop it without breaking
>cross version migration in both directions unconditionally.
>
>Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

>---
> hw/i386/acpi-build.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>index 416da31..8671e25 100644
>--- a/hw/i386/acpi-build.c
>+++ b/hw/i386/acpi-build.c
>@@ -2401,7 +2401,6 @@ 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);
>@@ -2411,19 +2410,7 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info)
>     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);
>+    build_header(linker, table_data, (void *)mcfg, "MCFG", len, 1, NULL, NULL);
> }
> 
> /*
>@@ -2592,6 +2579,9 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>     }
>     mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
>     qobject_unref(o);
>+    if (mcfg->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) {
>+        return false;
>+    }
> 
>     o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
>     assert(o);
>-- 
>2.7.4
>

-- 
Wei Yang
Help you, Help me