| Message ID | 20220207113129.2701722-3-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/smbios: fix table memory corruption with large memory vms | expand |
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
On Mon, Feb 14, 2022 at 6:21 PM Igor Mammedov <imammedo@redhat.com> wrote: > > 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? I don't know why I had this pre conception of memory corruption and overlapping addresses! Even the BZ says table handles overlap. Grr ... doing too much multiplexing these days :(
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, + * 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); }
With the current smbios table assignment code, we can have only 512 DIMM slots (each DIMM of 16 GiB in size) before tables 17 and 19 conflict with their addresses. 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. 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(-)