Re: [PATCH v3 2/3] hw/smbios: fix table memory corruption with large memory vms

[Igor Mammedov]

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);
>          }
>