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(-)
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
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 "
--
2.29.2
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 "
On 02.03.21 10:06, Igor Mammedov wrote: > 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)? VM initialization: bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' -> new table size: 128 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9659 -> new table size: 256 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 384 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 512 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 640 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9659 - 9903 -> new table size: 768 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9903 - 10023 -> new table size: 896 bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' -> new table size: 1024 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' -> new table size: 1280 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10023 - 10225 -> new table size: 1408 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10225 - 10281 -> new table size: 1536 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10281 - 10505 -> new table size: 1664 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10505 - 10577 -> new table size: 1792 bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' -> new table size: 1920 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' -> new table size: 2048 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10577 - 11471 -> new table size: 2176 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11471 - 11695 -> new table size: 2304 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11695 - 11735 -> new table size: 2432 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 2560 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 2688 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 2816 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 2944 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3072 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3200 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3328 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3456 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3584 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11735 - 11807 -> new table size: 3712 bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' -> new table size: 3840 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' -> new table size: 3968 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 -> new table size: 4096 When the bios/guest boots up: bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' -> new table size: 128 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9769 -> new table size: 256 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 384 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 512 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 640 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9769 - 10013 -> new table size: 768 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10013 - 10133 -> new table size: 896 bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' -> new table size: 1024 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' -> new table size: 1280 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10133 - 10335 -> new table size: 1408 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10335 - 10391 -> new table size: 1536 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10391 - 10615 -> new table size: 1664 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10615 - 10675 -> new table size: 1792 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10675 - 10747 -> new table size: 1920 bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' -> new table size: 2048 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' -> new table size: 2176 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10747 - 11641 -> new table size: 2304 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11641 - 11865 -> new table size: 2432 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11865 - 11905 -> new table size: 2560 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 2688 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 2816 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 2944 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3072 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3200 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3328 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3456 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3584 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3712 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' -> new table size: 3840 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11905 - 11981 -> new table size: 3968 bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' -> new table size: 4096 bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' -> new table size: 4224 bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 -> new table size: 4352 -- Thanks, David / dhildenb
On Tue, Mar 02, 2021 at 10:43:55AM +0100, David Hildenbrand wrote: > On 02.03.21 10:06, Igor Mammedov wrote: > > 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)? > > VM initialization: > > bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' > -> new table size: 128 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9659 > -> new table size: 256 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 384 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 512 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 640 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9659 - 9903 > -> new table size: 768 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9903 - 10023 > -> new table size: 896 > bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' > -> new table size: 1024 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' > -> new table size: 1280 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10023 - 10225 > -> new table size: 1408 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10225 - 10281 > -> new table size: 1536 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10281 - 10505 > -> new table size: 1664 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10505 - 10577 > -> new table size: 1792 > bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' > -> new table size: 1920 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' > -> new table size: 2048 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10577 - 11471 > -> new table size: 2176 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11471 - 11695 > -> new table size: 2304 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11695 - 11735 > -> new table size: 2432 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 2560 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 2688 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 2816 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 2944 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3072 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3200 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3328 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3456 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3584 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11735 - 11807 > -> new table size: 3712 > bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > -> new table size: 3840 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > -> new table size: 3968 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > -> new table size: 4096 > > > When the bios/guest boots up: > > bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' > -> new table size: 128 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9769 > -> new table size: 256 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 384 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 512 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 640 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9769 - 10013 > -> new table size: 768 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10013 - 10133 > -> new table size: 896 > bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' > -> new table size: 1024 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' > -> new table size: 1280 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10133 - 10335 > -> new table size: 1408 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10335 - 10391 > -> new table size: 1536 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10391 - 10615 > -> new table size: 1664 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10615 - 10675 > -> new table size: 1792 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10675 - 10747 > -> new table size: 1920 > bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' > -> new table size: 2048 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' > -> new table size: 2176 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10747 - 11641 > -> new table size: 2304 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11641 - 11865 > -> new table size: 2432 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11865 - 11905 > -> new table size: 2560 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 2688 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 2816 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 2944 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3072 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3200 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3328 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3456 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3584 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3712 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3840 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11905 - 11981 > -> new table size: 3968 > bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > -> new table size: 4096 > bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > -> new table size: 4224 > bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > -> new table size: 4352 yea it's because each command has space for 2 file names, so it's size is 128 bytes. So just 32 commands is 4k. Question is what is the extra table and why isn't it there before boot? -> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > -> new table size: 3840 -> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11905 - 11981 > -> new table size: 3968 -> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 +> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > -> new table size: 4096 +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' +> -> new table size: 4224 +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 +> -> new table size: 4352 simply put I would expect the command blob size to be the same before and after migration. > > > -- > Thanks, > > David / dhildenb
On 02.03.21 11:07, Michael S. Tsirkin wrote: > On Tue, Mar 02, 2021 at 10:43:55AM +0100, David Hildenbrand wrote: >> On 02.03.21 10:06, Igor Mammedov wrote: >>> 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)? >> >> VM initialization: >> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' >> -> new table size: 128 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9659 >> -> new table size: 256 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 384 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 512 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 640 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9659 - 9903 >> -> new table size: 768 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9903 - 10023 >> -> new table size: 896 >> bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' >> -> new table size: 1024 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' >> -> new table size: 1280 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10023 - 10225 >> -> new table size: 1408 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10225 - 10281 >> -> new table size: 1536 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10281 - 10505 >> -> new table size: 1664 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10505 - 10577 >> -> new table size: 1792 >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' >> -> new table size: 1920 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' >> -> new table size: 2048 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10577 - 11471 >> -> new table size: 2176 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11471 - 11695 >> -> new table size: 2304 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11695 - 11735 >> -> new table size: 2432 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 2560 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 2688 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 2816 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 2944 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3072 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3200 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3328 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3456 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3584 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11735 - 11807 >> -> new table size: 3712 >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' >> -> new table size: 3840 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' >> -> new table size: 3968 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 >> -> new table size: 4096 >> >> >> When the bios/guest boots up: >> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' >> -> new table size: 128 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9769 >> -> new table size: 256 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 384 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 512 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 640 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9769 - 10013 >> -> new table size: 768 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10013 - 10133 >> -> new table size: 896 >> bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' >> -> new table size: 1024 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' >> -> new table size: 1280 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10133 - 10335 >> -> new table size: 1408 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10335 - 10391 >> -> new table size: 1536 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10391 - 10615 >> -> new table size: 1664 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10615 - 10675 >> -> new table size: 1792 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10675 - 10747 >> -> new table size: 1920 >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' >> -> new table size: 2048 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' >> -> new table size: 2176 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10747 - 11641 >> -> new table size: 2304 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11641 - 11865 >> -> new table size: 2432 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11865 - 11905 >> -> new table size: 2560 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 2688 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 2816 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 2944 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3072 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3200 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3328 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3456 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3584 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3712 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' >> -> new table size: 3840 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11905 - 11981 >> -> new table size: 3968 >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' >> -> new table size: 4096 >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' >> -> new table size: 4224 >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 >> -> new table size: 4352 > > yea it's because each command has space for 2 file names, so it's size > is 128 bytes. So just 32 commands is 4k. > > Question is what is the extra table and why isn't it there before boot? > > -> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > > -> new table size: 3840 > -> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11905 - 11981 > > -> new table size: 3968 > -> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > +> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > > -> new table size: 4096 > +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > +> -> new table size: 4224 > +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > +> -> new table size: 4352 > > simply put I would expect the command blob size to be the same before > and after migration. Seems to be the "MCFG" table. I only see that pop up when the guest boots. It depends on acpi_get_mcfg(). When the VM is created, it is not mapped (mcfg->base == PCIE_BASE_ADDR_UNMAPPED), thus we don't create the table. Once the bios configured/mapped it (wild guess), we create the table. Anyhow, we seem to end up > 4k in the end. -- Thanks, David / dhildenb
On Tue, 2 Mar 2021 11:32:09 +0100 David Hildenbrand <david@redhat.com> wrote: > On 02.03.21 11:07, Michael S. Tsirkin wrote: > > On Tue, Mar 02, 2021 at 10:43:55AM +0100, David Hildenbrand wrote: > >> On 02.03.21 10:06, Igor Mammedov wrote: > >>> 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)? > >> > >> VM initialization: > >> > >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' > >> -> new table size: 128 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9659 > >> -> new table size: 256 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 384 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 512 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 640 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9659 - 9903 > >> -> new table size: 768 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9903 - 10023 > >> -> new table size: 896 > >> bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' > >> -> new table size: 1024 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' > >> -> new table size: 1280 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10023 - 10225 > >> -> new table size: 1408 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10225 - 10281 > >> -> new table size: 1536 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10281 - 10505 > >> -> new table size: 1664 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10505 - 10577 > >> -> new table size: 1792 > >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' > >> -> new table size: 1920 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' > >> -> new table size: 2048 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10577 - 11471 > >> -> new table size: 2176 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11471 - 11695 > >> -> new table size: 2304 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11695 - 11735 > >> -> new table size: 2432 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 2560 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 2688 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 2816 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 2944 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3072 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3200 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3328 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3456 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3584 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11735 - 11807 > >> -> new table size: 3712 > >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > >> -> new table size: 3840 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > >> -> new table size: 3968 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > >> -> new table size: 4096 > >> > >> > >> When the bios/guest boots up: > >> > >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables' > >> -> new table size: 128 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 64 - 9769 > >> -> new table size: 256 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 384 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 512 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 640 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 9769 - 10013 > >> -> new table size: 768 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10013 - 10133 > >> -> new table size: 896 > >> bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid' > >> -> new table size: 1024 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/vmgenid_guid' > >> -> new table size: 1280 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10133 - 10335 > >> -> new table size: 1408 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10335 - 10391 > >> -> new table size: 1536 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10391 - 10615 > >> -> new table size: 1664 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10615 - 10675 > >> -> new table size: 1792 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10675 - 10747 > >> -> new table size: 1920 > >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem' > >> -> new table size: 2048 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/nvdimm-mem' > >> -> new table size: 2176 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 10747 - 11641 > >> -> new table size: 2304 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11641 - 11865 > >> -> new table size: 2432 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11865 - 11905 > >> -> new table size: 2560 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 2688 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 2816 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 2944 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3072 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3200 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3328 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3456 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3584 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3712 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > >> -> new table size: 3840 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11905 - 11981 > >> -> new table size: 3968 > >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > >> -> new table size: 4096 > >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > >> -> new table size: 4224 > >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > >> -> new table size: 4352 > > > > yea it's because each command has space for 2 file names, so it's size > > is 128 bytes. So just 32 commands is 4k. > > > > Question is what is the extra table and why isn't it there before boot? > > > > -> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > > +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 'etc/acpi/tables' > > > -> new table size: 3840 > > -> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > > +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for range 11905 - 11981 > > > -> new table size: 3968 > > -> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > > +> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp' > > > -> new table size: 4096 > > +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 'etc/acpi/tables' > > +> -> new table size: 4224 > > +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for range 0 - 20 > > +> -> new table size: 4352 > > > > simply put I would expect the command blob size to be the same before > > and after migration. > > Seems to be the "MCFG" table. I only see that pop up when the guest boots. > > It depends on acpi_get_mcfg(). When the VM is created, it is not mapped > (mcfg->base == PCIE_BASE_ADDR_UNMAPPED), thus we don't create the table. > Once the bios configured/mapped it (wild guess), we create the table. > > Anyhow, we seem to end up > 4k in the end. Patch looks good to me, maybe add to commit message current number of entries limit and how we reach over it in reproducer above, plus nasty effect of tables that appears at runtime (like MCFG). With that Reviewed-by: Igor Mammedov <imammedo@redhat.com>
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) > 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 > > 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 "
On 02.03.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) Resizing RAMBlocks on the source *while* migrating is indeed problematic (and on the destination during actual postcopy phase) - I shared patches last year that abort migration in case we ever run into that issue, still have to follow up on that. But to run into that here, I guess we would have to start migration before the BIOS comes up such that we actually resize on the source while migration is active. -- Thanks, David / dhildenb
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
>>> 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).
Yes, or rather use ACPI_BUILD_ALIGN_SIZE (4k).
>
>
>>> 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?
We are dealing with different blobs here (tables_blob vs. cmd_blob).
>
> What is the exact call tree that triggers the above error?
[...]
acpi_build_update()->acpi_build_update()->memory_region_ram_resize()->qemu_ram_resize()
A longer calltrace can be found in https://bugzilla.redhat.com/show_bug.cgi?id=1927159.
>>> +++ 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?
See ACPI_BUILD_TABLE_MAX_SIZE handling just in the acpi_add_rom_blob() above.
>
> Where is it ensured that data between "rom->datasize" and "rom->romsize"
> reads as zeroes?
We only expose the current memory_region_size() to our guest, which is
always multiples of 4k pages.
rom->datasize and rom->romsize will be multiple of 4k AFAIKs.
acpi_align_size()-> g_array_set_size() will take care of zeroing out
any unused parts within a single 4k page.
So all unused, guest-visible part should always be 0 I think.
>
>
>>> 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?)
Changing ACPI_BUILD_ALIGN_SIZE would affect the legacy_table_size in
acpi_build() - so that can't be right.
What would also work is something like (to be improved)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 45ad2f9533..49cfedddc8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -81,6 +81,8 @@
#define ACPI_BUILD_LEGACY_CPU_AML_SIZE 97
#define ACPI_BUILD_ALIGN_SIZE 0x1000
+#define ACPI_BUILD_LOADER_ALIGN_SIZE 0x2000
+
#define ACPI_BUILD_TABLE_SIZE 0x20000
/* #define DEBUG_ACPI_BUILD */
@@ -2613,10 +2615,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
error_printf("Try removing CPUs, NUMA nodes, memory slots"
" or PCI bridges.");
}
- acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
+ acpi_align_size(tables_blob, ACPI_BUILD_ALIGN_SIZE);
}
- acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_ALIGN_SIZE);
+ acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_LOADER_ALIGN_SIZE);
At least for hw/i386/acpi-build.c.
We will end up creating the resizeable memory region/RAMBlock always with
a size=maximum_size=8k. (could also go for 64k here)
The only downside is that we might expose a bigger area to the
guest than necessary (e.g., 8k instead of 4k) and will e.g., migrate
8k instead of 4k (not that we care).
On incoming migration from older QEMU versions, we should be able to just
shrink back from 8k to 4k - so migration from older QEMY versions should
continue working just fine.
>
> (4b) The new macro ACPI_BUILD_LOADER_MAX_SIZE does not express 64KB,
> contrary to the commit message: it expresses 256KB.
Indeed, thanks for noticing that - not that it wouldn't really
affect your testing in case the maximum size is bigger than necessary ;)
>
> ... The code is really difficult to understand; consider the following
> macros:
Yes, it is.
Thanks!
--
Thanks,
David / dhildenb
On Tue, Mar 02, 2021 at 07:43:40PM +0100, David Hildenbrand 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,
> >
> > 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).
>
> Yes, or rather use ACPI_BUILD_ALIGN_SIZE (4k).
>
> >
> >
> > > > 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?
>
> We are dealing with different blobs here (tables_blob vs. cmd_blob).
>
> >
> > What is the exact call tree that triggers the above error?
>
> [...]
>
> acpi_build_update()->acpi_build_update()->memory_region_ram_resize()->qemu_ram_resize()
>
> A longer calltrace can be found in https://bugzilla.redhat.com/show_bug.cgi?id=1927159.
>
> > > > +++ 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?
>
> See ACPI_BUILD_TABLE_MAX_SIZE handling just in the acpi_add_rom_blob() above.
>
> >
> > Where is it ensured that data between "rom->datasize" and "rom->romsize"
> > reads as zeroes?
> We only expose the current memory_region_size() to our guest, which is
> always multiples of 4k pages.
>
> rom->datasize and rom->romsize will be multiple of 4k AFAIKs.
>
> acpi_align_size()-> g_array_set_size() will take care of zeroing out
> any unused parts within a single 4k page.
>
> So all unused, guest-visible part should always be 0 I think.
>
> >
> >
> > > > 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?)
>
> Changing ACPI_BUILD_ALIGN_SIZE would affect the legacy_table_size in
> acpi_build() - so that can't be right.
>
> What would also work is something like (to be improved)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 45ad2f9533..49cfedddc8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -81,6 +81,8 @@
> #define ACPI_BUILD_LEGACY_CPU_AML_SIZE 97
> #define ACPI_BUILD_ALIGN_SIZE 0x1000
> +#define ACPI_BUILD_LOADER_ALIGN_SIZE 0x2000
> +
> #define ACPI_BUILD_TABLE_SIZE 0x20000
> /* #define DEBUG_ACPI_BUILD */
> @@ -2613,10 +2615,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> error_printf("Try removing CPUs, NUMA nodes, memory slots"
> " or PCI bridges.");
> }
> - acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> + acpi_align_size(tables_blob, ACPI_BUILD_ALIGN_SIZE);
> }
> - acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_ALIGN_SIZE);
> + acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_LOADER_ALIGN_SIZE);
>
>
> At least for hw/i386/acpi-build.c.
>
> We will end up creating the resizeable memory region/RAMBlock always with
> a size=maximum_size=8k. (could also go for 64k here)
>
> The only downside is that we might expose a bigger area to the
> guest than necessary (e.g., 8k instead of 4k) and will e.g., migrate
> 8k instead of 4k (not that we care).
>
>
> On incoming migration from older QEMU versions, we should be able to just
> shrink back from 8k to 4k - so migration from older QEMY versions should
> continue working just fine.
what about migration to old qemu?
> >
> > (4b) The new macro ACPI_BUILD_LOADER_MAX_SIZE does not express 64KB,
> > contrary to the commit message: it expresses 256KB.
>
> Indeed, thanks for noticing that - not that it wouldn't really
> affect your testing in case the maximum size is bigger than necessary ;)
>
> >
> > ... The code is really difficult to understand; consider the following
> > macros:
>
> Yes, it is.
>
> Thanks!
>
> --
> Thanks,
>
> David / dhildenb
On 03.03.21 10:43, Michael S. Tsirkin wrote:
> On Tue, Mar 02, 2021 at 07:43:40PM +0100, David Hildenbrand 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,
>>>
>>> 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).
>>
>> Yes, or rather use ACPI_BUILD_ALIGN_SIZE (4k).
>>
>>>
>>>
>>>>> 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?
>>
>> We are dealing with different blobs here (tables_blob vs. cmd_blob).
>>
>>>
>>> What is the exact call tree that triggers the above error?
>>
>> [...]
>>
>> acpi_build_update()->acpi_build_update()->memory_region_ram_resize()->qemu_ram_resize()
>>
>> A longer calltrace can be found in https://bugzilla.redhat.com/show_bug.cgi?id=1927159.
>>
>>>>> +++ 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?
>>
>> See ACPI_BUILD_TABLE_MAX_SIZE handling just in the acpi_add_rom_blob() above.
>>
>>>
>>> Where is it ensured that data between "rom->datasize" and "rom->romsize"
>>> reads as zeroes?
>> We only expose the current memory_region_size() to our guest, which is
>> always multiples of 4k pages.
>>
>> rom->datasize and rom->romsize will be multiple of 4k AFAIKs.
>>
>> acpi_align_size()-> g_array_set_size() will take care of zeroing out
>> any unused parts within a single 4k page.
>>
>> So all unused, guest-visible part should always be 0 I think.
>>
>>>
>>>
>>>>> 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?)
>>
>> Changing ACPI_BUILD_ALIGN_SIZE would affect the legacy_table_size in
>> acpi_build() - so that can't be right.
>>
>> What would also work is something like (to be improved)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 45ad2f9533..49cfedddc8 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -81,6 +81,8 @@
>> #define ACPI_BUILD_LEGACY_CPU_AML_SIZE 97
>> #define ACPI_BUILD_ALIGN_SIZE 0x1000
>> +#define ACPI_BUILD_LOADER_ALIGN_SIZE 0x2000
>> +
>> #define ACPI_BUILD_TABLE_SIZE 0x20000
>> /* #define DEBUG_ACPI_BUILD */
>> @@ -2613,10 +2615,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>> error_printf("Try removing CPUs, NUMA nodes, memory slots"
>> " or PCI bridges.");
>> }
>> - acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>> + acpi_align_size(tables_blob, ACPI_BUILD_ALIGN_SIZE);
>> }
>> - acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_ALIGN_SIZE);
>> + acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_LOADER_ALIGN_SIZE);
>>
>>
>> At least for hw/i386/acpi-build.c.
>>
>> We will end up creating the resizeable memory region/RAMBlock always with
>> a size=maximum_size=8k. (could also go for 64k here)
>>
>> The only downside is that we might expose a bigger area to the
>> guest than necessary (e.g., 8k instead of 4k) and will e.g., migrate
>> 8k instead of 4k (not that we care).
>>
>>
>> On incoming migration from older QEMU versions, we should be able to just
>> shrink back from 8k to 4k - so migration from older QEMY versions should
>> continue working just fine.
>
> what about migration to old qemu?
We seem to have replied at just the same time. See my other mail :)
--
Thanks,
David / dhildenb
On 02.03.21 19:43, David Hildenbrand 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,
>>
>> 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).
>
> Yes, or rather use ACPI_BUILD_ALIGN_SIZE (4k).
>
>>
>>
>>>> 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?
>
> We are dealing with different blobs here (tables_blob vs. cmd_blob).
>
>>
>> What is the exact call tree that triggers the above error?
>
> [...]
>
> acpi_build_update()->acpi_build_update()->memory_region_ram_resize()->qemu_ram_resize()
>
> A longer calltrace can be found in https://bugzilla.redhat.com/show_bug.cgi?id=1927159.
>
>>>> +++ 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?
>
> See ACPI_BUILD_TABLE_MAX_SIZE handling just in the acpi_add_rom_blob() above.
>
>>
>> Where is it ensured that data between "rom->datasize" and "rom->romsize"
>> reads as zeroes?
> We only expose the current memory_region_size() to our guest, which is
> always multiples of 4k pages.
>
> rom->datasize and rom->romsize will be multiple of 4k AFAIKs.
>
> acpi_align_size()-> g_array_set_size() will take care of zeroing out
> any unused parts within a single 4k page.
>
> So all unused, guest-visible part should always be 0 I think.
>
>>
>>
>>>> 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?)
>
> Changing ACPI_BUILD_ALIGN_SIZE would affect the legacy_table_size in
> acpi_build() - so that can't be right.
>
> What would also work is something like (to be improved)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 45ad2f9533..49cfedddc8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -81,6 +81,8 @@
> #define ACPI_BUILD_LEGACY_CPU_AML_SIZE 97
> #define ACPI_BUILD_ALIGN_SIZE 0x1000
>
> +#define ACPI_BUILD_LOADER_ALIGN_SIZE 0x2000
> +
> #define ACPI_BUILD_TABLE_SIZE 0x20000
>
> /* #define DEBUG_ACPI_BUILD */
> @@ -2613,10 +2615,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> error_printf("Try removing CPUs, NUMA nodes, memory slots"
> " or PCI bridges.");
> }
> - acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> + acpi_align_size(tables_blob, ACPI_BUILD_ALIGN_SIZE);
> }
>
> - acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_ALIGN_SIZE);
> + acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_LOADER_ALIGN_SIZE);
>
>
> At least for hw/i386/acpi-build.c.
>
> We will end up creating the resizeable memory region/RAMBlock always with
> a size=maximum_size=8k. (could also go for 64k here)
>
> The only downside is that we might expose a bigger area to the
> guest than necessary (e.g., 8k instead of 4k) and will e.g., migrate
> 8k instead of 4k (not that we care).
>
>
> On incoming migration from older QEMU versions, we should be able to just
> shrink back from 8k to 4k - so migration from older QEMY versions should
> continue working just fine.
Correction: Older QEMU versions (e.g., before
62be4e3a5041e84304aa23637da623a205c53ecc) did not support resizeable RAM
MemoryRegions / RAMBlocks. This affects ~ < QEMU v2.3.0.
So unconditionally changing the size of the cmd_blob memory region
(e.g., 4k -> 8k) would most probably break migration from never QEMU to
older QEMU (v2.2.0.). Not sure if we really care.
@MST, Igor what's your take?
--
Thanks,
David / dhildenb
On Wed, 3 Mar 2021 10:49:08 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 02.03.21 19:43, David Hildenbrand 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,
> >>
> >> 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).
> >
> > Yes, or rather use ACPI_BUILD_ALIGN_SIZE (4k).
> >
> >>
> >>
> >>>> 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?
> >
> > We are dealing with different blobs here (tables_blob vs. cmd_blob).
> >
> >>
> >> What is the exact call tree that triggers the above error?
> >
> > [...]
> >
> > acpi_build_update()->acpi_build_update()->memory_region_ram_resize()->qemu_ram_resize()
> >
> > A longer calltrace can be found in https://bugzilla.redhat.com/show_bug.cgi?id=1927159.
> >
> >>>> +++ 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?
> >
> > See ACPI_BUILD_TABLE_MAX_SIZE handling just in the acpi_add_rom_blob() above.
> >
> >>
> >> Where is it ensured that data between "rom->datasize" and "rom->romsize"
> >> reads as zeroes?
> > We only expose the current memory_region_size() to our guest, which is
> > always multiples of 4k pages.
> >
> > rom->datasize and rom->romsize will be multiple of 4k AFAIKs.
> >
> > acpi_align_size()-> g_array_set_size() will take care of zeroing out
> > any unused parts within a single 4k page.
> >
> > So all unused, guest-visible part should always be 0 I think.
> >
> >>
> >>
> >>>> 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?)
> >
> > Changing ACPI_BUILD_ALIGN_SIZE would affect the legacy_table_size in
> > acpi_build() - so that can't be right.
> >
> > What would also work is something like (to be improved)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 45ad2f9533..49cfedddc8 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -81,6 +81,8 @@
> > #define ACPI_BUILD_LEGACY_CPU_AML_SIZE 97
> > #define ACPI_BUILD_ALIGN_SIZE 0x1000
> >
> > +#define ACPI_BUILD_LOADER_ALIGN_SIZE 0x2000
> > +
> > #define ACPI_BUILD_TABLE_SIZE 0x20000
> >
> > /* #define DEBUG_ACPI_BUILD */
> > @@ -2613,10 +2615,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > error_printf("Try removing CPUs, NUMA nodes, memory slots"
> > " or PCI bridges.");
> > }
> > - acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> > + acpi_align_size(tables_blob, ACPI_BUILD_ALIGN_SIZE);
> > }
> >
> > - acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_ALIGN_SIZE);
> > + acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_LOADER_ALIGN_SIZE);
> >
> >
> > At least for hw/i386/acpi-build.c.
> >
> > We will end up creating the resizeable memory region/RAMBlock always with
> > a size=maximum_size=8k. (could also go for 64k here)
> >
> > The only downside is that we might expose a bigger area to the
> > guest than necessary (e.g., 8k instead of 4k) and will e.g., migrate
> > 8k instead of 4k (not that we care).
> >
> >
> > On incoming migration from older QEMU versions, we should be able to just
> > shrink back from 8k to 4k - so migration from older QEMY versions should
> > continue working just fine.
>
> Correction: Older QEMU versions (e.g., before
> 62be4e3a5041e84304aa23637da623a205c53ecc) did not support resizeable RAM
> MemoryRegions / RAMBlocks. This affects ~ < QEMU v2.3.0.
>
> So unconditionally changing the size of the cmd_blob memory region
> (e.g., 4k -> 8k) would most probably break migration from never QEMU to
> older QEMU (v2.2.0.). Not sure if we really care.
>
> @MST, Igor what's your take?
We shouldn't change aligned size (an alignment value), since it's what goes
on migration stream wire.
Changing max should not affect migrations stream directly.
In most cases ping-pong migration should work as both sides will have
the same configuration, in unlikely case newer QEMU goes over current 4k,
it will jump to the next aligned size (8k) and migration will fail cleanly
due size mismatch and it can't be made any more prettier.
(similar to border cases when we switched to resizable regions for main tables
blob)
On Wed, Mar 03, 2021 at 04:26:39PM +0100, Igor Mammedov wrote:
> On Wed, 3 Mar 2021 10:49:08 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
> > On 02.03.21 19:43, David Hildenbrand 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,
> > >>
> > >> 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).
> > >
> > > Yes, or rather use ACPI_BUILD_ALIGN_SIZE (4k).
> > >
> > >>
> > >>
> > >>>> 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?
> > >
> > > We are dealing with different blobs here (tables_blob vs. cmd_blob).
> > >
> > >>
> > >> What is the exact call tree that triggers the above error?
> > >
> > > [...]
> > >
> > > acpi_build_update()->acpi_build_update()->memory_region_ram_resize()->qemu_ram_resize()
> > >
> > > A longer calltrace can be found in https://bugzilla.redhat.com/show_bug.cgi?id=1927159.
> > >
> > >>>> +++ 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?
> > >
> > > See ACPI_BUILD_TABLE_MAX_SIZE handling just in the acpi_add_rom_blob() above.
> > >
> > >>
> > >> Where is it ensured that data between "rom->datasize" and "rom->romsize"
> > >> reads as zeroes?
> > > We only expose the current memory_region_size() to our guest, which is
> > > always multiples of 4k pages.
> > >
> > > rom->datasize and rom->romsize will be multiple of 4k AFAIKs.
> > >
> > > acpi_align_size()-> g_array_set_size() will take care of zeroing out
> > > any unused parts within a single 4k page.
> > >
> > > So all unused, guest-visible part should always be 0 I think.
> > >
> > >>
> > >>
> > >>>> 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?)
> > >
> > > Changing ACPI_BUILD_ALIGN_SIZE would affect the legacy_table_size in
> > > acpi_build() - so that can't be right.
> > >
> > > What would also work is something like (to be improved)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 45ad2f9533..49cfedddc8 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -81,6 +81,8 @@
> > > #define ACPI_BUILD_LEGACY_CPU_AML_SIZE 97
> > > #define ACPI_BUILD_ALIGN_SIZE 0x1000
> > >
> > > +#define ACPI_BUILD_LOADER_ALIGN_SIZE 0x2000
> > > +
> > > #define ACPI_BUILD_TABLE_SIZE 0x20000
> > >
> > > /* #define DEBUG_ACPI_BUILD */
> > > @@ -2613,10 +2615,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> > > error_printf("Try removing CPUs, NUMA nodes, memory slots"
> > > " or PCI bridges.");
> > > }
> > > - acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> > > + acpi_align_size(tables_blob, ACPI_BUILD_ALIGN_SIZE);
> > > }
> > >
> > > - acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_ALIGN_SIZE);
> > > + acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_LOADER_ALIGN_SIZE);
> > >
> > >
> > > At least for hw/i386/acpi-build.c.
> > >
> > > We will end up creating the resizeable memory region/RAMBlock always with
> > > a size=maximum_size=8k. (could also go for 64k here)
> > >
> > > The only downside is that we might expose a bigger area to the
> > > guest than necessary (e.g., 8k instead of 4k) and will e.g., migrate
> > > 8k instead of 4k (not that we care).
> > >
> > >
> > > On incoming migration from older QEMU versions, we should be able to just
> > > shrink back from 8k to 4k - so migration from older QEMY versions should
> > > continue working just fine.
> >
> > Correction: Older QEMU versions (e.g., before
> > 62be4e3a5041e84304aa23637da623a205c53ecc) did not support resizeable RAM
> > MemoryRegions / RAMBlocks. This affects ~ < QEMU v2.3.0.
> >
> > So unconditionally changing the size of the cmd_blob memory region
> > (e.g., 4k -> 8k) would most probably break migration from never QEMU to
> > older QEMU (v2.2.0.). Not sure if we really care.
> >
> > @MST, Igor what's your take?
> We shouldn't change aligned size (an alignment value), since it's what goes
> on migration stream wire.
> Changing max should not affect migrations stream directly.
> In most cases ping-pong migration should work as both sides will have
> the same configuration, in unlikely case newer QEMU goes over current 4k,
> it will jump to the next aligned size (8k) and migration will fail cleanly
> due size mismatch and it can't be made any more prettier.
> (similar to border cases when we switched to resizable regions for main tables
> blob)
Right. We can backort the change in the stable tree too.
I do think that we should add some kind of entry to the
command though when mcfg is disabled so the size doesn't change
like that. Will avoid weird failures if there's a convoluted config
which overflows again, it will fail cleanly on qemu start.
How about a dummy SSDT?
--
MST
On 03/02/21 19:43, David Hildenbrand wrote:
> We are dealing with different blobs here (tables_blob vs. cmd_blob).
OK, thanks -- this was the important bit I was missing. Over time I've
lost track of the actual set of fw_cfg blobs that QEMU exposes, for the
purposes of the ACPI linker/loader.
I've looked up the acpi_add_rom_blob() calls in "hw/i386/acpi-build.c"
and "hw/arm/virt-acpi-build.c":
hw name max_size notes
------- ------------------------------------------- ------------------------------------ ------
virt ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
virt ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
virt ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (below)
i386 ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
i386 ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
i386 ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 d70414a5788c, 358774d780ee8
microvm ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
microvm "etc/table-loader" 0 no macro for name???
microvm ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (above)
(I notice there are some other (optional) fw_cfg blobs too, related TPM,
vmgenid, nvdimm etc, using fw_cfg_add_file() rather than
acpi_add_rom_blob() -- so those are immutable (never regenerated). I
definitely needed this reminder...)
So, my observations:
(1) microvm open-codes "etc/table-loader", rather than using the macro
ACPI_BUILD_LOADER_FILE.
The proposed patch corrects it, which I welcome per se. However, it
should arguably be a separate patch. I found it distracting, in spite of
the commit message highlighting it. I don't insist though, I'm
admittedly rusty on this code.
(2) The proposed patch sets "max_size" to ACPI_BUILD_LOADER_MAX_SIZE for
each ACPI_BUILD_LOADER_FILE. Makes sense, upon constructing / reviewing
the above table.
(I'm no longer sure if tweaking the alignment were the preferable path
forward.)
Either way, I'd request including the above table in the commit message.
(Maybe drop the "notes" column.)
(3) The above 9 invocations are *all* of the acpi_add_rom_blob()
invocations. I find the interface brittle. It's not helpful to have so
many macros for the names and the max sizes. We should have a table with
three entries and -- minimally -- two columns, specifying name and
max_size -- possibly some more call arguments, if such can be extracted.
We should also have an enum type for selecting a row in this table, and
then acpi_add_rom_blob() should be called with an enum constant.
Of course, talk is cheap. :)
(4) When do we plan to introduce a nonzero "max_size" for
ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")?
Is the current zero value a time bomb?
Put differently: acpi_add_rom_blob() should be *impossible* to call with
"max_size=0", arguably. *Whenever* we call acpi_add_rom_blob(), we do
that because the blob is resizable (mutable) -- but that also means we
should have a safety margin, does it not? So calling acpi_add_rom_blob()
with "max_size=0" looks self-contradictory.
FWIW, this could be covered by the table proposed in point (3).
In total, I don't disagree with the patch (beyond the fact that the new
macro's value doesn't match the commit message), functionally speaking.
However, wrt. readability, I think the patch further complicates the
code. I'd suggest five patches:
#1 -- use "etc/table-loader" via the proper macro name in "microvm",
#2 -- rework acpi_add_rom_blob() for using a table of constants + an
enum type,
#3 -- bump the "max_size" field for ACPI_BUILD_LOADER_FILE, for the
current symptom,
#4 -- set a nonzero "max_size" for the remaining ACPI_BUILD_RSDP_FILE,
for "future-proofing",
#5 -- in the new acpi_add_rom_blob() implementation, taking the enum,
assert(max_size != 0).
(I haven't thought through what this would mean for migration, forward
or backward; I'm just brain-storming.)
Thanks
Laszlo
On 03.03.21 16:03, Laszlo Ersek wrote:
> On 03/02/21 19:43, David Hildenbrand wrote:
>
>> We are dealing with different blobs here (tables_blob vs. cmd_blob).
>
> OK, thanks -- this was the important bit I was missing. Over time I've
> lost track of the actual set of fw_cfg blobs that QEMU exposes, for the
> purposes of the ACPI linker/loader.
>
> I've looked up the acpi_add_rom_blob() calls in "hw/i386/acpi-build.c"
> and "hw/arm/virt-acpi-build.c":
>
> hw name max_size notes
> ------- ------------------------------------------- ------------------------------------ ------
>
> virt ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> virt ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> virt ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (below)
>
> i386 ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> i386 ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> i386 ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 d70414a5788c, 358774d780ee8
>
> microvm ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> microvm "etc/table-loader" 0 no macro for name???
> microvm ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (above)
>
> (I notice there are some other (optional) fw_cfg blobs too, related TPM,
> vmgenid, nvdimm etc, using fw_cfg_add_file() rather than
> acpi_add_rom_blob() -- so those are immutable (never regenerated). I
> definitely needed this reminder...)
>
> So, my observations:
>
> (1) microvm open-codes "etc/table-loader", rather than using the macro
> ACPI_BUILD_LOADER_FILE.
>
> The proposed patch corrects it, which I welcome per se. However, it
> should arguably be a separate patch. I found it distracting, in spite of
> the commit message highlighting it. I don't insist though, I'm
> admittedly rusty on this code.
>
>
> (2) The proposed patch sets "max_size" to ACPI_BUILD_LOADER_MAX_SIZE for
> each ACPI_BUILD_LOADER_FILE. Makes sense, upon constructing / reviewing
> the above table.
>
> (I'm no longer sure if tweaking the alignment were the preferable path
> forward.)
>
> Either way, I'd request including the above table in the commit message.
> (Maybe drop the "notes" column.)
>
>
> (3) The above 9 invocations are *all* of the acpi_add_rom_blob()
> invocations. I find the interface brittle. It's not helpful to have so
> many macros for the names and the max sizes. We should have a table with
> three entries and -- minimally -- two columns, specifying name and
> max_size -- possibly some more call arguments, if such can be extracted.
> We should also have an enum type for selecting a row in this table, and
> then acpi_add_rom_blob() should be called with an enum constant.
>
> Of course, talk is cheap. :)
>
>
> (4) When do we plan to introduce a nonzero "max_size" for
> ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")?
>
> Is the current zero value a time bomb?
>
> Put differently: acpi_add_rom_blob() should be *impossible* to call with
> "max_size=0", arguably. *Whenever* we call acpi_add_rom_blob(), we do
> that because the blob is resizable (mutable) -- but that also means we
> should have a safety margin, does it not? So calling acpi_add_rom_blob()
> with "max_size=0" looks self-contradictory.
>
> FWIW, this could be covered by the table proposed in point (3).
>
>
> In total, I don't disagree with the patch (beyond the fact that the new
> macro's value doesn't match the commit message), functionally speaking.
> However, wrt. readability, I think the patch further complicates the
> code. I'd suggest five patches:
>
> #1 -- use "etc/table-loader" via the proper macro name in "microvm",
>
> #2 -- rework acpi_add_rom_blob() for using a table of constants + an
> enum type,
>
> #3 -- bump the "max_size" field for ACPI_BUILD_LOADER_FILE, for the
> current symptom,
>
> #4 -- set a nonzero "max_size" for the remaining ACPI_BUILD_RSDP_FILE,
> for "future-proofing",
>
> #5 -- in the new acpi_add_rom_blob() implementation, taking the enum,
> assert(max_size != 0).
>
Mostly sounds sane to me, however, I'm leaning towards putting the real
fix upfront (so we e.g., can easily backport to stable) and doing all
the refactorings on top.
I think we have an agreement that the current approach is the right one.
I'll look into it today. Thanks!
--
Thanks,
David / dhildenb
On Wed, 3 Mar 2021 16:03:36 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/02/21 19:43, David Hildenbrand wrote:
>
> > We are dealing with different blobs here (tables_blob vs. cmd_blob).
>
> OK, thanks -- this was the important bit I was missing. Over time I've
> lost track of the actual set of fw_cfg blobs that QEMU exposes, for the
> purposes of the ACPI linker/loader.
>
> I've looked up the acpi_add_rom_blob() calls in "hw/i386/acpi-build.c"
> and "hw/arm/virt-acpi-build.c":
>
> hw name max_size notes
> ------- ------------------------------------------- ------------------------------------ ------
>
> virt ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> virt ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> virt ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (below)
>
> i386 ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> i386 ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> i386 ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 d70414a5788c, 358774d780ee8
>
> microvm ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> microvm "etc/table-loader" 0 no macro for name???
> microvm ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (above)
>
> (I notice there are some other (optional) fw_cfg blobs too, related TPM,
> vmgenid, nvdimm etc, using fw_cfg_add_file() rather than
> acpi_add_rom_blob() -- so those are immutable (never regenerated). I
> definitely needed this reminder...)
most of them are just guest RAM reservations (guest/hose exchange buffer)
and "etc/tpm/config" seems to immutable for specific configuration
> So, my observations:
>
> (1) microvm open-codes "etc/table-loader", rather than using the macro
> ACPI_BUILD_LOADER_FILE.
>
> The proposed patch corrects it, which I welcome per se. However, it
> should arguably be a separate patch. I found it distracting, in spite of
> the commit message highlighting it. I don't insist though, I'm
> admittedly rusty on this code.
>
>
> (2) The proposed patch sets "max_size" to ACPI_BUILD_LOADER_MAX_SIZE for
> each ACPI_BUILD_LOADER_FILE. Makes sense, upon constructing / reviewing
> the above table.
>
> (I'm no longer sure if tweaking the alignment were the preferable path
> forward.)
>
> Either way, I'd request including the above table in the commit message.
> (Maybe drop the "notes" column.)
>
>
> (3) The above 9 invocations are *all* of the acpi_add_rom_blob()
> invocations. I find the interface brittle. It's not helpful to have so
> many macros for the names and the max sizes. We should have a table with
> three entries and -- minimally -- two columns, specifying name and
> max_size -- possibly some more call arguments, if such can be extracted.
> We should also have an enum type for selecting a row in this table, and
> then acpi_add_rom_blob() should be called with an enum constant.
>
> Of course, talk is cheap. :)
>
>
> (4) When do we plan to introduce a nonzero "max_size" for
> ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")?
>
> Is the current zero value a time bomb?
it's not likely to go over 4k, but if we enforce max_size!=0 we may set it 4k,
which it's aligned to anyways.
> Put differently: acpi_add_rom_blob() should be *impossible* to call with
> "max_size=0", arguably. *Whenever* we call acpi_add_rom_blob(), we do
> that because the blob is resizable (mutable) -- but that also means we
> should have a safety margin, does it not? So calling acpi_add_rom_blob()
> with "max_size=0" looks self-contradictory.
main use-case for using acpi_add_rom_blob() is for mutable blobs,
so that all these blobs were transferred during migration to the destination,
to ensure that guest sees consistent data set (from source instead of mix of
source/dst blobs).
Resize came later on, when we got sick of ad-hock (align)/size bumping of
"etc/acpi/tables" in configurations where size was on verge of crossing
border to the next aligned size and related knobs to keep that mess
migratable.
>
> FWIW, this could be covered by the table proposed in point (3).
>
>
> In total, I don't disagree with the patch (beyond the fact that the new
> macro's value doesn't match the commit message), functionally speaking.
> However, wrt. readability, I think the patch further complicates the
> code. I'd suggest five patches:
>
> #1 -- use "etc/table-loader" via the proper macro name in "microvm",
>
> #2 -- rework acpi_add_rom_blob() for using a table of constants + an
> enum type,
>
> #3 -- bump the "max_size" field for ACPI_BUILD_LOADER_FILE, for the
> current symptom,
>
> #4 -- set a nonzero "max_size" for the remaining ACPI_BUILD_RSDP_FILE,
> for "future-proofing",
>
> #5 -- in the new acpi_add_rom_blob() implementation, taking the enum,
> assert(max_size != 0).
>
> (I haven't thought through what this would mean for migration, forward
> or backward; I'm just brain-storming.)
>
> Thanks
> Laszlo
On Wed, Mar 03, 2021 at 05:09:16PM +0100, Igor Mammedov wrote:
> On Wed, 3 Mar 2021 16:03:36 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
> > On 03/02/21 19:43, David Hildenbrand wrote:
> >
> > > We are dealing with different blobs here (tables_blob vs. cmd_blob).
> >
> > OK, thanks -- this was the important bit I was missing. Over time I've
> > lost track of the actual set of fw_cfg blobs that QEMU exposes, for the
> > purposes of the ACPI linker/loader.
> >
> > I've looked up the acpi_add_rom_blob() calls in "hw/i386/acpi-build.c"
> > and "hw/arm/virt-acpi-build.c":
> >
> > hw name max_size notes
> > ------- ------------------------------------------- ------------------------------------ ------
> >
> > virt ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> > virt ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> > virt ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (below)
> >
> > i386 ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> > i386 ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> > i386 ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 d70414a5788c, 358774d780ee8
> >
> > microvm ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> > microvm "etc/table-loader" 0 no macro for name???
> > microvm ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (above)
> >
> > (I notice there are some other (optional) fw_cfg blobs too, related TPM,
> > vmgenid, nvdimm etc, using fw_cfg_add_file() rather than
> > acpi_add_rom_blob() -- so those are immutable (never regenerated). I
> > definitely needed this reminder...)
>
> most of them are just guest RAM reservations (guest/hose exchange buffer)
> and "etc/tpm/config" seems to immutable for specific configuration
>
>
> > So, my observations:
> >
> > (1) microvm open-codes "etc/table-loader", rather than using the macro
> > ACPI_BUILD_LOADER_FILE.
> >
> > The proposed patch corrects it, which I welcome per se. However, it
> > should arguably be a separate patch. I found it distracting, in spite of
> > the commit message highlighting it. I don't insist though, I'm
> > admittedly rusty on this code.
> >
> >
> > (2) The proposed patch sets "max_size" to ACPI_BUILD_LOADER_MAX_SIZE for
> > each ACPI_BUILD_LOADER_FILE. Makes sense, upon constructing / reviewing
> > the above table.
> >
> > (I'm no longer sure if tweaking the alignment were the preferable path
> > forward.)
> >
> > Either way, I'd request including the above table in the commit message.
> > (Maybe drop the "notes" column.)
> >
> >
> > (3) The above 9 invocations are *all* of the acpi_add_rom_blob()
> > invocations. I find the interface brittle. It's not helpful to have so
> > many macros for the names and the max sizes. We should have a table with
> > three entries and -- minimally -- two columns, specifying name and
> > max_size -- possibly some more call arguments, if such can be extracted.
> > We should also have an enum type for selecting a row in this table, and
> > then acpi_add_rom_blob() should be called with an enum constant.
> >
> > Of course, talk is cheap. :)
> >
> >
> > (4) When do we plan to introduce a nonzero "max_size" for
> > ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")?
> >
> > Is the current zero value a time bomb?
>
> it's not likely to go over 4k, but if we enforce max_size!=0 we may set it 4k,
> which it's aligned to anyways.
Right. BTW there is an alternative I did not think of earlier.
Lots of tables are actually fixed. We currently let guest calculate
the checksum for all tables but that is not a must. We could prefill the
checksum for most of them and cut the size by almost half.
This fixes the issues in a way that seems cleaner to me as
it migrates both ways for all configs and saves some resources.
I'm not against making it resizeable too though.
>
> > Put differently: acpi_add_rom_blob() should be *impossible* to call with
> > "max_size=0", arguably. *Whenever* we call acpi_add_rom_blob(), we do
> > that because the blob is resizable (mutable) -- but that also means we
> > should have a safety margin, does it not? So calling acpi_add_rom_blob()
> > with "max_size=0" looks self-contradictory.
>
> main use-case for using acpi_add_rom_blob() is for mutable blobs,
> so that all these blobs were transferred during migration to the destination,
> to ensure that guest sees consistent data set (from source instead of mix of
> source/dst blobs).
>
> Resize came later on, when we got sick of ad-hock (align)/size bumping of
> "etc/acpi/tables" in configurations where size was on verge of crossing
> border to the next aligned size and related knobs to keep that mess
> migratable.
>
> >
> > FWIW, this could be covered by the table proposed in point (3).
> >
> >
> > In total, I don't disagree with the patch (beyond the fact that the new
> > macro's value doesn't match the commit message), functionally speaking.
> > However, wrt. readability, I think the patch further complicates the
> > code. I'd suggest five patches:
> >
> > #1 -- use "etc/table-loader" via the proper macro name in "microvm",
> >
> > #2 -- rework acpi_add_rom_blob() for using a table of constants + an
> > enum type,
> >
> > #3 -- bump the "max_size" field for ACPI_BUILD_LOADER_FILE, for the
> > current symptom,
> >
> > #4 -- set a nonzero "max_size" for the remaining ACPI_BUILD_RSDP_FILE,
> > for "future-proofing",
> >
> > #5 -- in the new acpi_add_rom_blob() implementation, taking the enum,
> > assert(max_size != 0).
> >
> > (I haven't thought through what this would mean for migration, forward
> > or backward; I'm just brain-storming.)
> >
> > Thanks
> > Laszlo
On Wed, Mar 03, 2021 at 11:21:45AM -0500, Michael S. Tsirkin wrote:
> On Wed, Mar 03, 2021 at 05:09:16PM +0100, Igor Mammedov wrote:
> > On Wed, 3 Mar 2021 16:03:36 +0100
> > Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > > On 03/02/21 19:43, David Hildenbrand wrote:
> > >
> > > > We are dealing with different blobs here (tables_blob vs. cmd_blob).
> > >
> > > OK, thanks -- this was the important bit I was missing. Over time I've
> > > lost track of the actual set of fw_cfg blobs that QEMU exposes, for the
> > > purposes of the ACPI linker/loader.
> > >
> > > I've looked up the acpi_add_rom_blob() calls in "hw/i386/acpi-build.c"
> > > and "hw/arm/virt-acpi-build.c":
> > >
> > > hw name max_size notes
> > > ------- ------------------------------------------- ------------------------------------ ------
> > >
> > > virt ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> > > virt ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> > > virt ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (below)
> > >
> > > i386 ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> > > i386 ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
> > > i386 ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 d70414a5788c, 358774d780ee8
> > >
> > > microvm ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
> > > microvm "etc/table-loader" 0 no macro for name???
> > > microvm ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (above)
> > >
> > > (I notice there are some other (optional) fw_cfg blobs too, related TPM,
> > > vmgenid, nvdimm etc, using fw_cfg_add_file() rather than
> > > acpi_add_rom_blob() -- so those are immutable (never regenerated). I
> > > definitely needed this reminder...)
> >
> > most of them are just guest RAM reservations (guest/hose exchange buffer)
> > and "etc/tpm/config" seems to immutable for specific configuration
> >
> >
> > > So, my observations:
> > >
> > > (1) microvm open-codes "etc/table-loader", rather than using the macro
> > > ACPI_BUILD_LOADER_FILE.
> > >
> > > The proposed patch corrects it, which I welcome per se. However, it
> > > should arguably be a separate patch. I found it distracting, in spite of
> > > the commit message highlighting it. I don't insist though, I'm
> > > admittedly rusty on this code.
> > >
> > >
> > > (2) The proposed patch sets "max_size" to ACPI_BUILD_LOADER_MAX_SIZE for
> > > each ACPI_BUILD_LOADER_FILE. Makes sense, upon constructing / reviewing
> > > the above table.
> > >
> > > (I'm no longer sure if tweaking the alignment were the preferable path
> > > forward.)
> > >
> > > Either way, I'd request including the above table in the commit message.
> > > (Maybe drop the "notes" column.)
> > >
> > >
> > > (3) The above 9 invocations are *all* of the acpi_add_rom_blob()
> > > invocations. I find the interface brittle. It's not helpful to have so
> > > many macros for the names and the max sizes. We should have a table with
> > > three entries and -- minimally -- two columns, specifying name and
> > > max_size -- possibly some more call arguments, if such can be extracted.
> > > We should also have an enum type for selecting a row in this table, and
> > > then acpi_add_rom_blob() should be called with an enum constant.
> > >
> > > Of course, talk is cheap. :)
> > >
> > >
> > > (4) When do we plan to introduce a nonzero "max_size" for
> > > ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")?
> > >
> > > Is the current zero value a time bomb?
> >
> > it's not likely to go over 4k, but if we enforce max_size!=0 we may set it 4k,
> > which it's aligned to anyways.
>
> Right. BTW there is an alternative I did not think of earlier.
>
> Lots of tables are actually fixed. We currently let guest calculate
> the checksum for all tables but that is not a must. We could prefill the
> checksum for most of them and cut the size by almost half.
>
> This fixes the issues in a way that seems cleaner to me as
> it migrates both ways for all configs and saves some resources.
> I'm not against making it resizeable too though.
Hmm I tried and I take it back. There are just 5 tables that are
immuatable. Not sure it's worth the trouble.
>
> >
> > > Put differently: acpi_add_rom_blob() should be *impossible* to call with
> > > "max_size=0", arguably. *Whenever* we call acpi_add_rom_blob(), we do
> > > that because the blob is resizable (mutable) -- but that also means we
> > > should have a safety margin, does it not? So calling acpi_add_rom_blob()
> > > with "max_size=0" looks self-contradictory.
> >
> > main use-case for using acpi_add_rom_blob() is for mutable blobs,
> > so that all these blobs were transferred during migration to the destination,
> > to ensure that guest sees consistent data set (from source instead of mix of
> > source/dst blobs).
> >
> > Resize came later on, when we got sick of ad-hock (align)/size bumping of
> > "etc/acpi/tables" in configurations where size was on verge of crossing
> > border to the next aligned size and related knobs to keep that mess
> > migratable.
> >
> > >
> > > FWIW, this could be covered by the table proposed in point (3).
> > >
> > >
> > > In total, I don't disagree with the patch (beyond the fact that the new
> > > macro's value doesn't match the commit message), functionally speaking.
> > > However, wrt. readability, I think the patch further complicates the
> > > code. I'd suggest five patches:
> > >
> > > #1 -- use "etc/table-loader" via the proper macro name in "microvm",
> > >
> > > #2 -- rework acpi_add_rom_blob() for using a table of constants + an
> > > enum type,
> > >
> > > #3 -- bump the "max_size" field for ACPI_BUILD_LOADER_FILE, for the
> > > current symptom,
> > >
> > > #4 -- set a nonzero "max_size" for the remaining ACPI_BUILD_RSDP_FILE,
> > > for "future-proofing",
> > >
> > > #5 -- in the new acpi_add_rom_blob() implementation, taking the enum,
> > > assert(max_size != 0).
> > >
> > > (I haven't thought through what this would mean for migration, forward
> > > or backward; I'm just brain-storming.)
> > >
> > > Thanks
> > > Laszlo
On 03.03.21 17:09, Igor Mammedov wrote:
> On Wed, 3 Mar 2021 16:03:36 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 03/02/21 19:43, David Hildenbrand wrote:
>>
>>> We are dealing with different blobs here (tables_blob vs. cmd_blob).
>>
>> OK, thanks -- this was the important bit I was missing. Over time I've
>> lost track of the actual set of fw_cfg blobs that QEMU exposes, for the
>> purposes of the ACPI linker/loader.
>>
>> I've looked up the acpi_add_rom_blob() calls in "hw/i386/acpi-build.c"
>> and "hw/arm/virt-acpi-build.c":
>>
>> hw name max_size notes
>> ------- ------------------------------------------- ------------------------------------ ------
>>
>> virt ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
>> virt ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
>> virt ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (below)
>>
>> i386 ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
>> i386 ACPI_BUILD_LOADER_FILE ("etc/table-loader") 0 n/a
>> i386 ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 d70414a5788c, 358774d780ee8
>>
>> microvm ACPI_BUILD_TABLE_FILE ("etc/acpi/tables") ACPI_BUILD_TABLE_MAX_SIZE (0x200000) n/a
>> microvm "etc/table-loader" 0 no macro for name???
>> microvm ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp") 0 simply modeled on i386 (above)
>>
>> (I notice there are some other (optional) fw_cfg blobs too, related TPM,
>> vmgenid, nvdimm etc, using fw_cfg_add_file() rather than
>> acpi_add_rom_blob() -- so those are immutable (never regenerated). I
>> definitely needed this reminder...)
>
> most of them are just guest RAM reservations (guest/hose exchange buffer)
> and "etc/tpm/config" seems to immutable for specific configuration
>
>
>> So, my observations:
>>
>> (1) microvm open-codes "etc/table-loader", rather than using the macro
>> ACPI_BUILD_LOADER_FILE.
>>
>> The proposed patch corrects it, which I welcome per se. However, it
>> should arguably be a separate patch. I found it distracting, in spite of
>> the commit message highlighting it. I don't insist though, I'm
>> admittedly rusty on this code.
>>
>>
>> (2) The proposed patch sets "max_size" to ACPI_BUILD_LOADER_MAX_SIZE for
>> each ACPI_BUILD_LOADER_FILE. Makes sense, upon constructing / reviewing
>> the above table.
>>
>> (I'm no longer sure if tweaking the alignment were the preferable path
>> forward.)
>>
>> Either way, I'd request including the above table in the commit message.
>> (Maybe drop the "notes" column.)
>>
>>
>> (3) The above 9 invocations are *all* of the acpi_add_rom_blob()
>> invocations. I find the interface brittle. It's not helpful to have so
>> many macros for the names and the max sizes. We should have a table with
>> three entries and -- minimally -- two columns, specifying name and
>> max_size -- possibly some more call arguments, if such can be extracted.
>> We should also have an enum type for selecting a row in this table, and
>> then acpi_add_rom_blob() should be called with an enum constant.
>>
>> Of course, talk is cheap. :)
>>
>>
>> (4) When do we plan to introduce a nonzero "max_size" for
>> ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")?
>>
>> Is the current zero value a time bomb?
>
> it's not likely to go over 4k, but if we enforce max_size!=0 we may set it 4k,
> which it's aligned to anyways.
Interestingly, the size is not aligned.
We end up calling rom_add_blob() with a size like "22". The memory
region we create has size=22 / max_size=22. (max_size is not effective,
we rely on the one from the underlying RAMBlock).
The resizeable RAMBlock, however, aligns both up to full pages (e.g., 4k
/ 4k), so we can later grow it > 22 bytes.
Doesn't really matter in practice I guess, because we always expose full
pages to the guest, and migrate full pages.
One corner case could be shrinking e.g., from 22 bytes to 14 bytes. I am
not sure if we would zero-out these 8 bytes somewhere. Not sure if that
is of any relevance.
Beautiful code.
--
Thanks,
David / dhildenb
© 2016 - 2024 Red Hat, Inc.