Re: [Qemu-devel] [PATCH v2 for-2.1 2/2] pc: hack for migration compatibility from QEMU 2.0

[Laszlo Ersek <lersek@redhat.com>]

On 07/24/14 16:32, Paolo Bonzini wrote:
> Changing the ACPI table size causes migration to break, and the memory
> hotplug work opened our eyes on how horribly we were breaking things in
> 2.0 already.
> 
> The ACPI table size is rounded to the next 4k, which one would think
> gives some headroom.  In practice this is not the case, because the user
> can control the ACPI table size (each CPU adds 97 bytes to the SSDT and
> 8 to the MADT) and so some "-smp" values will break the 4k boundary and
> fail to migrate.  Similarly, PCI bridges add ~1870 bytes to the SSDT.
> 
> To fix this, hard-code 64k as the maximum ACPI table size, which
> (despite being an order of magnitude smaller than 640k) should be enough
> for everyone.
> 
> To fix migration from QEMU 2.0, compute the payload size of QEMU 2.0
> and always use that one.  The previous patch shrunk the ACPI tables
> enough that the QEMU 2.0 size should always be enough.
> 
> Migration from QEMU 1.7 should work for guests that have a number of CPUs
> other than 12, 13, 14, 54, 55, 56, 97, 98, 139, 140.  It was already
> broken from QEMU 1.7 to QEMU 2.0 in the same way, though.
> 
> Even with this patch, QEMU 1.7 and 2.0 have two different ideas of
> "-M pc-i440fx-2.0" when there are PCI bridges.  Igor sent a patch to
> adopt the QEMU 1.7 definition.  I think distributions should apply
> it if they move directly from QEMU 1.7 to 2.1+ without ever packaging
> version 2.0.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	replace magic constants with #defines [Igor]
> 	remove stray line from comment [Laszlo]

I compared this too with its v1 counterpart, and it looks good. I have
one question (just curiosity): the following paragraph was dropped from
the commit message -- why?

-Non-AML tables can change depending on the configuration (especially
-MADT, SRAT, HPET) but they remain the same between QEMU 2.0 and 2.1,
-so we only compute our padding based on the sizes of the SSDT and DSDT.

I think this remains true in v2 as well:
- "aml_len" and "legacy_aml_len" still "only" cover the DSDT and the
SSDT, and
- the non-AML tables (eg. the MADT, now spelled out in the commit
message), although they may grow with the number of CPUs, continue to
remain the same between 2.0 and 2.1.

IOW, I think you could have kept this paragraph if you wanted to. Was it
an oversight to drop it, or did the paragraph contain something
incorrect (in v1) that I'm unaware of? Or is it just redundant?

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo