On Mon, 14 Feb 2022 19:42:35 +0530 Ani Sinha <a...@anisinha.ca> wrote:
> The current smbios table implementation splits the main memory in 16 GiB > (DIMM like) chunks. With the current smbios table assignment code, we can have > only 512 such chunks before the 16 bit handle numbers in the header for tables > 17 and 19 conflict. 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 handle numbers > 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 > in order to accomodate more DIMM entries without the table handles colliding. > 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 handle numbers are reserved and > table handle values remain as before. > > As table handles are altered for large memory VMs, this change can break > migration in those cases. However, in those situations, qemu crashes anyway 1. pls explain how migration breaks 2. it's not QEMU which crashes > 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 s/buglink/Resolves/ > Signed-off-by: Ani Sinha <a...@anisinha.ca> with above fixed Reviewed-by: Igor Mammedov <imamm...@redhat.com> > --- > hw/smbios/smbios.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > changelog: > v3: reworded the commit log and comment in code. > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index 56b412ce35..44c53797a4 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 header handle numbers so that they do > + * not overlap. For example, for a VM with larger than 8 TB guest > + * memory and DIMM like chunks 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); > } >
