[PATCH v3] smbios: arm64: Ensure table is written at a good address

[Ilias Apalodimas]

Hi Simon,

On Tue, 21 Nov 2023 at 04:58, Simon Glass <sjg at chromium.org> wrote:
>
> U-Boot typically sets up its malloc() pool near the top of memory. On
> ARM64 systems this can result in an SMBIOS table above 4GB which is
> not supported by SMBIOSv2.
>
> Work around this problem by automatically choosing an address below 4GB
> (but as high as possible), if needed.
>
> Tell efi_loader about the address in the same way as we do for ACPI
> tables.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> Please see previous discussion here:
>
>   https://patchwork.ozlabs.org/project/uboot/patch/
>   20231025123117.v2.1.I054cad60e00f8cfde39256c7c7d9c960987fb9be at changeid/
>
> Changes in v3:
> - Add RISC-V to the condition, too
> - Update the commit message
>
> Changes in v2:
> - Update to search for a suitable area automatically, if enabled
>
>  lib/Kconfig                 | 13 ++++++++
>  lib/efi_loader/efi_smbios.c | 63 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 19649517a39b..c70faeeebed5 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -998,6 +998,19 @@ config GENERATE_SMBIOS_TABLE
>           See also SMBIOS_SYSINFO which allows SMBIOS values to be provided in
>           the devicetree.
>
> +config SMBIOS_TABLE_FIXED
> +       bool "Place the SMBIOS table at a special address"
> +       depends on GENERATE_SMBIOS_TABLE && SMBIOS && EFI_LOADER
> +       depends on ARM64 || RISCV
> +       default y
> +       help
> +         Use this option to place the SMBIOS table at a special address.
> +
> +         U-Boot typically sets up its malloc() pool near the top of memory. On
> +         ARM64 systems this can result in an SMBIOS table above 4GB which is
> +         not supported by SMBIOSv2. This option works around this problem by
> +         chosing an address just below 4GB, if needed.
> +
>  endmenu
>
>  config LIB_RATIONAL
> diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
> index 48446f654d9b..566f206af5b6 100644
> --- a/lib/efi_loader/efi_smbios.c
> +++ b/lib/efi_loader/efi_smbios.c
> @@ -47,6 +47,60 @@ efi_status_t efi_smbios_register(void)
>                                                map_sysmem(addr, 0));
>  }
>
> +/**
> + * find_addr_below() - Find a usable region below the given max_addr
> + *
> + * Check if *addrp is suitable to define a memory region which finishes below
> + * @max_addr + @req_size. If so, do nothing and return 0
> + *
> + * As a backup, if CONFIG_SMBIOS_TABLE_FIXED is enabled, search for a
> + * 4KB-aligned DRAM region which is large enough. Make sure it is below U-Boot's
> + * stack space, assumed to be 64KB.
> + *
> + * @max_addr: Maximum address that can be used (region must finish before here)
> + * @req:size: Required size of region
> + * @addrp: On entry: Current proposed address; on exit, holds the new address,
> + *     on success
> + * Return 0 if OK (existing region was OK, or a new one was found), -ENOSPC if
> + * nothing suitable was found
> + */
> +static int find_addr_below(ulong max_addr, ulong req_size, ulong *addrp)
> +{
> +       struct bd_info *bd = gd->bd;
> +       ulong max_base;
> +       int i;
> +
> +       max_base = max_addr - req_size;
> +       if (*addrp <= max_base)
> +               return 0;
> +
> +       if (!IS_ENABLED(CONFIG_SMBIOS_TABLE_FIXED))
> +               return -ENOSPC;
> +
> +       /* Make sure that the base is at least 64KB below the stack */
> +       max_base = min(max_base,
> +                      ALIGN(gd->start_addr_sp - SZ_64K - req_size, SZ_4K));
> +
> +       for (i = CONFIG_NR_DRAM_BANKS - 1; i >= 0; i--) {
> +               ulong start = bd->bi_dram[i].start;
> +               ulong size = bd->bi_dram[i].size;
> +               ulong addr;
> +
> +               /* chose an address at most req_size bytes before the end */
> +               addr = min(max_base, start - req_size + size);
> +
> +               /* check this is in the range */
> +               if (addr >= start && addr + req_size < start + size) {
> +                       *addrp = addr;
> +                       log_warning("Forcing SMBIOS table to address %lx\n",
> +                                   addr);
> +                       return 0;
> +               }
> +       }
> +
> +       return -ENOSPC;
> +}

This function doesn't really belong here. We should move it to the mem
subsystem.
AFAICT this is a hotfix for 2024.01. So I prefer picking this [0]
instead, since the smbios table generation is still controlled by EFI

[0] https://lore.kernel.org/u-boot/20231120222558.32014-1-heinrich.schuchardt@canonical.com/

Thanks
/Ilias
> +
>  static int install_smbios_table(void)
>  {
>         ulong addr;
> @@ -61,7 +115,14 @@ static int install_smbios_table(void)
>                 return log_msg_ret("mem", -ENOMEM);
>
>         addr = map_to_sysmem(buf);
> -       if (!write_smbios_table(addr)) {
> +
> +       /*
> +        * Deal with a fixed address if needed. For simplicity we assume that
> +        * the SMBIOS-table size is <64KB. If a suitable address cannot be
> +        * found, then write_smbios_table() returns an error.
> +        */
> +       if (find_addr_below(SZ_4G - 1, SZ_64K, &addr) ||
> +           !write_smbios_table(addr)) {
>                 log_err("Failed to write SMBIOS table\n");
>                 return log_msg_ret("smbios", -EINVAL);
>         }
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>