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

[Igor Mammedov <imammedo@redhat.com>]

On Mon,  1 Mar 2021 11:48:33 +0100
David Hildenbrand <david@redhat.com> wrote:

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

I don't see what's here that would make cmd_blob go above 4k.
can you try identify what actually fills it up (perhaps we have a hidden bug elsewhere)?


> 
> 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.
> 
> 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);
> 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    "