| Message ID | 20220214141237.47946-3-ani@anisinha.ca |
|---|---|
| State | New |
| Headers | show |
| Series | hw/smbios: fix table memory corruption with large memory vms | expand |
On Mon, 14 Feb 2022 19:42:35 +0530 Ani Sinha <ani@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 <ani@anisinha.ca> with above fixed Reviewed-by: Igor Mammedov <imammedo@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); > } >
On Thu, 17 Feb 2022, Igor Mammedov wrote: > On Mon, 14 Feb 2022 19:42:35 +0530 > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2023977 > s/buglink/Resolves/ > OK I am curious about this one. Per https://www.qemu.org/docs/master/devel/submitting-a-patch.html , If your patch fixes a bug in the gitlab bug tracker, please add a line with “Resolves: <URL-of-the-bug>” to the commit message, too. Gitlab can close bugs automatically once commits with the “Resolved:” keyword get merged into the master branch of the project. And if your patch addresses a bug in another public bug tracker, you can also use a line with “Buglink: <URL-of-the-bug>” for reference here, too. So I considered redhar BZ as a public bug tracker as well. Does the BZ also automatically close bugs when we use "Resolves:"? Should we update the instructions here?
On Thu, 17 Feb 2022 19:02:05 +0530 (IST) Ani Sinha <ani@anisinha.ca> wrote: > On Thu, 17 Feb 2022, Igor Mammedov wrote: > > > On Mon, 14 Feb 2022 19:42:35 +0530 > > > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2023977 > > s/buglink/Resolves/ > > > > OK I am curious about this one. > > Per https://www.qemu.org/docs/master/devel/submitting-a-patch.html , > > If your patch fixes a bug in the gitlab bug tracker, please add a line > with “Resolves: <URL-of-the-bug>” to the commit message, too. Gitlab can > close bugs automatically once commits with the “Resolved:” keyword get > merged into the master branch of the project. And if your patch addresses > a bug in another public bug tracker, you can also use a line with > “Buglink: <URL-of-the-bug>” for reference here, too. > > So I considered redhar BZ as a public bug tracker as well. Does the BZ > also automatically close bugs when we use "Resolves:"? Should we update > the instructions here? I might have been wrong suggesting "Resolves" tag, it will/should not affect bugzilla. Looking at history, shows a zoo of used tags in bugzilla case, so I'd guess Buglink is fine as any other.
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); }
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 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(-) changelog: v3: reworded the commit log and comment in code.