On 03/02/21 17:23, Igor Mammedov wrote: > On Mon, 1 Mar 2021 11:48:33 +0100 > David Hildenbrand <da...@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.fran...@xilinx.com> >> Cc: Paolo Bonzini <pbonz...@redhat.com> >> Cc: "Michael S. Tsirkin" <m...@redhat.com> >> Cc: Igor Mammedov <imamm...@redhat.com> >> Cc: Peter Maydell <peter.mayd...@linaro.org> >> Cc: Shannon Zhao <shannon.zha...@gmail.com> >> Cc: Marcel Apfelbaum <marcel.apfelb...@gmail.com> >> Cc: Paolo Bonzini <pbonz...@redhat.com> >> Cc: Richard Henderson <richard.hender...@linaro.org> >> Cc: qemu-...@nongnu.org >> Signed-off-by: David Hildenbrand <da...@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