Re: [PATCH v1] acpi: increase maximum size for "etc/table-loader" blob

[Laszlo Ersek]

On 03/02/21 17:23, Igor Mammedov wrote:
> On Mon,  1 Mar 2021 11:48:33 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
> CCing Laszlo,
>
> to make sure there is no complications from firmware side (especially
> when migration is progress, we've ironed it out for main tables blob
> but my memory is a bit fussy about issues we had to deal with if there
> were any)

You might have the following thread in mind:

  Invalid blob size on NVDIMM hot-add
  
http://mid.mail-archive.com/5FC3163CFD30C246ABAA99954A238FA83F3FB328@lhreml524-mbs.china.huawei.com

That was a problem with the re-generation of the ACPI payload, which
wouldn't fit in the originally allocated space.

I don't know if that problem is related to the current patch -- it seems
to be?

More comments below:


>> The resizeable memory region that is created for the cmd blob has a maximum
>> size of ACPI_BUILD_ALIGN_SIZE - 4k. This used to be sufficient, however,

The expression "ACPI_BUILD_ALIGN_SIZE - 4k" makes no sense to me.
ACPI_BUILD_ALIGN_SIZE is #defined in "hw/i386/acpi-build.c" as 0x1000,
so the difference (ACPI_BUILD_ALIGN_SIZE - 4k) is zero.

(1) Did you mean "ACPI_BUILD_ALIGN_SIZE -- 4k"? IOW, did you mean to
quote the value of the macro?

If you mean an em dash, then please use an em dash, not a hyphen (or
please use parens).


>> as we try fitting in additional data (e.g., vmgenid, nvdimm, intel-iommu),
>> we require more than 4k and can crash QEMU when trying to resize the
>> resizeable memory region beyond its maximum size:
>>   $ build/qemu-system-x86_64 --enable-kvm \
>>       -machine q35,nvdimm=on \
>>       -smp 1 \
>>       -cpu host \
>>       -m size=2G,slots=8,maxmem=4G \
>>       -object memory-backend-file,id=mem0,mem-path=/tmp/nvdimm,size=256M \
>>       -device nvdimm,label-size=131072,memdev=mem0,id=nvdimm0,slot=1 \
>>       -nodefaults \
>>       -device vmgenid \
>>       -device intel-iommu
>>
>> Results in:
>>   Unexpected error in qemu_ram_resize() at ../softmmu/physmem.c:1850:
>>   qemu-system-x86_64: Size too large: /rom@etc/table-loader:
>>     0x2000 > 0x1000: Invalid argument
>>
>> We try growing the resizeable memory region (resizeable RAMBlock) beyond
>> its maximum size. Let's increase the maximum size from 4k to 64k, which
>> should be good enough for the near future.

The existent code calls acpi_align_size(), for resizing the ACPI blobs
(the GArray objects).

(Unfortunately, the acpi_align_size() function is duplicated between
"hw/i386/acpi-build.c" and "hw/arm/virt-acpi-build.c", which seems
unjustified -- but anyway, I digress.)

This seems to come from commit 868270f23d8d ("acpi-build: tweak acpi
migration limits", 2014-07-29) and commit 451b157041d2 ("acpi: Align the
size to 128k", 2020-12-08).

(2) Why is the logic added in those commits insufficient?

What is the exact call tree that triggers the above error?


>> Migration is not concerned with the maximum size of a RAMBlock, only
>> with the used size - so existing setups are not affected. Of course, we
>> cannot migrate a VM that would have crash when started on older QEMU from
>> new QEMU to older QEMU without failing early on the destination when
>> synchronizing the RAM state:
>>     qemu-system-x86_64: Size too large: /rom@etc/table-loader: 0x2000 > 
>> 0x1000: Invalid argument
>>     qemu-system-x86_64: error while loading state for instance 0x0 of device 
>> 'ram'
>>     qemu-system-x86_64: load of migration failed: Invalid argument
>>
>> While at it, replace "etc/table-loader" by ACPI_BUILD_LOADER_FILE in
>> the microvm.
>>
>> Note: we could warn for problematic setups that migration might not
>> always be possible - similar to how we handle the table blob; or we
>> could disallow setups that would have crashed until now for compat
>> machines. But I am not sure if the effort (messing compat machine
>> properties) is worth it as we fail migration in a safe way early.
>>
>> Cc: Alistair Francis <alistair.francis@xilinx.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/arm/virt-acpi-build.c    | 3 ++-
>>  hw/i386/acpi-build.c        | 3 ++-
>>  hw/i386/acpi-microvm.c      | 3 ++-
>>  include/hw/acpi/aml-build.h | 1 +
>>  4 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index f9c9df916c..a91550de6f 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -865,7 +865,8 @@ void virt_acpi_setup(VirtMachineState *vms)
>>
>>      build_state->linker_mr =
>>          acpi_add_rom_blob(virt_acpi_build_update, build_state,
>> -                          tables.linker->cmd_blob, ACPI_BUILD_LOADER_FILE, 
>> 0);
>> +                          tables.linker->cmd_blob, ACPI_BUILD_LOADER_FILE,
>> +                          ACPI_BUILD_LOADER_MAX_SIZE);
>>
>>      fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, 
>> tables.tcpalog->data,
>>                      acpi_data_len(tables.tcpalog));
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 31a5f6f4a5..a75138ea5a 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2524,7 +2524,8 @@ void acpi_setup(void)
>>
>>      build_state->linker_mr =
>>          acpi_add_rom_blob(acpi_build_update, build_state,
>> -                          tables.linker->cmd_blob, ACPI_BUILD_LOADER_FILE, 
>> 0);
>> +                          tables.linker->cmd_blob, ACPI_BUILD_LOADER_FILE,
>> +                          ACPI_BUILD_LOADER_MAX_SIZE);
>>
>>      fw_cfg_add_file(x86ms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
>> index 54b3af478a..fe8a965fe6 100644
>> --- a/hw/i386/acpi-microvm.c
>> +++ b/hw/i386/acpi-microvm.c
>> @@ -255,7 +255,8 @@ void acpi_setup_microvm(MicrovmMachineState *mms)
>>                        ACPI_BUILD_TABLE_MAX_SIZE);
>>      acpi_add_rom_blob(acpi_build_no_update, NULL,
>>                        tables.linker->cmd_blob,
>> -                      "etc/table-loader", 0);
>> +                      ACPI_BUILD_LOADER_FILE,
>> +                      ACPI_BUILD_LOADER_MAX_SIZE);
>>      acpi_add_rom_blob(acpi_build_no_update, NULL,
>>                        tables.rsdp,
>>                        ACPI_BUILD_RSDP_FILE, 0);

(3) Why are we using a different "tool" here, from the previous
approach? We're no longer setting the GArray sizes; instead, we make the
"rom->romsize" fields diverge from -- put differently, grow beyond --
"rom->datasize". Why is that useful? What are the consequences?

Where is it ensured that data between "rom->datasize" and "rom->romsize"
reads as zeroes?

Is this change guest-visible at all?


>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 380d3e3924..93cdfd4006 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -6,6 +6,7 @@
>>
>>  /* Reserve RAM space for tables: add another order of magnitude. */
>>  #define ACPI_BUILD_TABLE_MAX_SIZE         0x200000
>> +#define ACPI_BUILD_LOADER_MAX_SIZE        0x40000
>>
>>  #define ACPI_BUILD_APPNAME6 "BOCHS "
>>  #define ACPI_BUILD_APPNAME8 "BXPC    "
>

The commit message says "Let's increase the maximum size from 4k to
64k", and I have two problems with that:

(4a) I have no idea where the current "4k" size comes from. (In case the
4k refers to ACPI_BUILD_ALIGN_SIZE, then why are we not changing that
macro?)

(4b) The new macro ACPI_BUILD_LOADER_MAX_SIZE does not express 64KB,
contrary to the commit message: it expresses 256KB.

I could test this patch with OVMF and ArmVirtQemu of course, technically
speaking, but right now I'm not convinced the patch is *worth* testing,
as-is. Minimally, point (4b) appears to need a fix.


... The code is really difficult to understand; consider the following
macros:

- ACPI_BUILD_TABLE_MAX_SIZE  [include/hw/acpi/aml-build.h]
- ACPI_BUILD_ALIGN_SIZE      [hw/i386/acpi-build.c]
- ACPI_BUILD_TABLE_SIZE      [hw/i386/acpi-build.c, hw/arm/virt-acpi-build.c]
- ACPI_BUILD_LOADER_MAX_SIZE [include/hw/acpi/aml-build.h] -- being added now

I don't have the slightest idea why we need all of these macros.

Thanks
Laszlo