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

[Igor Mammedov <imammedo@redhat.com>]

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