[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/3] hw/smbios: fix table memory corruption with large mem
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v2 2/3] hw/smbios: fix table memory corruption with large memory vms |
Date: |
Mon, 14 Feb 2022 13:50:54 +0100 |
On Mon, 7 Feb 2022 17:01:28 +0530
Ani Sinha <ani@anisinha.ca> wrote:
> With the current smbios table assignment code, we can have only 512 DIMM slots
it's a bit confusing, since it's not DIMM slots in QEMU sense (we do not expose
DIMM devices via SMBIOS/E820). So maybe clarify here that initial RAM is split
into 16GB (with 'DIMM' type ) chunks/entries when it's described in SMBIOS
table 17.
> (each DIMM of 16 GiB in size) before tables 17 and 19 conflict with their
> addresses.
Are you sure it's addresses that are wrong?
> A guest with more than 8 TiB of memory will hit this limitation and
> would fail with the following assertion in isa-debugcon:
>
> ASSERT_EFI_ERROR (Status = Already started)
> ASSERT
> /builddir/build/BUILD/edk2-ca407c7246bf/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c(125):
> !EFI_ERROR (Status)
>
> This change adds an additional offset between tables 17 and 19 when
> configuring
> VMs larger than 8 TiB of memory. The value of the offset is calculated
> to be equal to the additional space required to be reserved between the tables
> in order to accomodate more DIMM devices without the table memories colliding.
s/DIMM devices/DIMM entries/
not sure what 'table memories colliding' is, maybe rephrase and
be more accurate about what happens here.
> In normal cases where the VM memory is smaller or equal to 8 TiB, this offset
> value is 0. Hence in this case, no additional memory space is reserved and
> table addresses remain as before.
>
> Since table addresses are altered for large memory VMs, this change can break
> migration in those cases. However, in those situations, qemu crashes anyway
> without this fix and hence we do not preserve the old bug by introducing
> compat knobs/machine types.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2023977
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
> hw/smbios/smbios.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 56b412ce35..d7de740363 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -799,12 +799,13 @@ static void smbios_build_type_17_table(unsigned
> instance, uint64_t size)
> SMBIOS_BUILD_TABLE_POST;
> }
>
> -static void smbios_build_type_19_table(unsigned instance,
> +static void smbios_build_type_19_table(unsigned instance, unsigned offset,
> uint64_t start, uint64_t size)
> {
> uint64_t end, start_kb, end_kb;
>
> - SMBIOS_BUILD_TABLE_PRE(19, T19_BASE + instance, true); /* required */
> + SMBIOS_BUILD_TABLE_PRE(19, T19_BASE + offset + instance,
> + true); /* required */
>
> end = start + size - 1;
> assert(end > start);
> @@ -996,7 +997,7 @@ void smbios_get_tables(MachineState *ms,
> uint8_t **anchor, size_t *anchor_len,
> Error **errp)
> {
> - unsigned i, dimm_cnt;
> + unsigned i, dimm_cnt, offset;
>
> if (smbios_legacy) {
> *tables = *anchor = NULL;
> @@ -1026,6 +1027,16 @@ void smbios_get_tables(MachineState *ms,
>
> dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) /
> MAX_DIMM_SZ;
>
> + /*
> + * The offset determines if we need to keep additional space betweeen
> + * table 17 and table 19 so that they do not overlap. For example,
it's not tables that overlap, but something else
> + * for a VM with larger than 8 TB guest memory and DIMM size of 16
> GiB,
> + * the default space between the two tables (T19_BASE - T17_BASE =
> 512)
> + * is not enough.
> + */
> + offset = (dimm_cnt > (T19_BASE - T17_BASE)) ? \
> + dimm_cnt - (T19_BASE - T17_BASE) : 0;
> +
> smbios_build_type_16_table(dimm_cnt);
>
> for (i = 0; i < dimm_cnt; i++) {
> @@ -1033,7 +1044,7 @@ void smbios_get_tables(MachineState *ms,
> }
>
> for (i = 0; i < mem_array_size; i++) {
> - smbios_build_type_19_table(i, mem_array[i].address,
> + smbios_build_type_19_table(i, offset, mem_array[i].address,
> mem_array[i].length);
> }
>
other than used language/terminology, the rest looks fine tom