Hi Heinrich
On Thu, 10 Jun 2021 at 13:16, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 6/9/21 5:14 PM, Ilias Apalodimas wrote:
> > We currently define the EFI support of an SMBIOS table as the third bit of
> > "BIOS Characteristics Extension Byte 1". The latest DMTF spec defines it
> > on "BIOS Characteristics Extension Byte 2".
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > ---
> > include/smbios.h | 2 +-
> > lib/smbios.c | 5 +++--
> > 2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/smbios.h b/include/smbios.h
> > index ffeefb47372d..fc49fc10b9d7 100644
> > --- a/include/smbios.h
> > +++ b/include/smbios.h
> > @@ -60,7 +60,7 @@ struct __packed smbios_entry {
> > #define BIOS_CHARACTERISTICS_SELECTABLE_BOOT (1 << 16)
> >
> > #define BIOS_CHARACTERISTICS_EXT1_ACPI (1 << 0)
> > -#define BIOS_CHARACTERISTICS_EXT1_UEFI (1 << 3)
> > +#define BIOS_CHARACTERISTICS_EXT2_UEFI (1 << 3)
> > #define BIOS_CHARACTERISTICS_EXT2_TARGET (1 << 2)
> >
> > struct __packed smbios_type0 {
> > diff --git a/lib/smbios.c b/lib/smbios.c
> > index 9eb226ec9fbd..abdd157a7084 100644
> > --- a/lib/smbios.c
> > +++ b/lib/smbios.c
> > @@ -214,6 +214,7 @@ static int smbios_write_type0(ulong *current, int
> > handle,
> > gd->smbios_version = ctx->last_str;
> > log_debug("smbios_version = %p: '%s'\n", gd->smbios_version,
> > gd->smbios_version);
> > + t->bios_characteristics_ext2 = 0;
> > #ifdef LOG_DEBUG
> > print_buffer((ulong)gd->smbios_version, gd->smbios_version,
> > 1, strlen(gd->smbios_version) + 1, 0);
> > @@ -229,9 +230,9 @@ static int smbios_write_type0(ulong *current, int
> > handle,
> > t->bios_characteristics_ext1 = BIOS_CHARACTERISTICS_EXT1_ACPI;
>
> This looks fishy. Where is t->bios_characteristics_ext1 initialized if
> CONFIG_GENERATE_ACPI_TABLE=n? efi_smbios_register() does not zero out
> the memory.
>
> We should initialize the field irrespective of the configuration and
> then use a bitwise or here.
>
> > #endif
> > #ifdef CONFIG_EFI_LOADER
> > - t->bios_characteristics_ext1 |= BIOS_CHARACTERISTICS_EXT1_UEFI;
> > + t->bios_characteristics_ext2 |= BIOS_CHARACTERISTICS_EXT2_UEFI;
> > #endif
> > - t->bios_characteristics_ext2 = BIOS_CHARACTERISTICS_EXT2_TARGET;
> > + t->bios_characteristics_ext2 |= BIOS_CHARACTERISTICS_EXT2_TARGET;
>
> Where is t->bios_characteristics_ext2 initialized?
> We don't want random bytes.
A few lines above, in this patchset
>
> @Simon:
> The usage of ulong instead of pointers in this module does not make
> sense to me. If the sandbox needs to call it, it should map its virtual
> addresses. We should not spill those conversion into non-sandbox code.
>
> Best regards
>
> Heinrich
>
> >
> > /* bios_major_release has only one byte, so drop century */
> > t->bios_major_release = U_BOOT_VERSION_NUM % 100;
> >
>
