Re: [PATCH v4 7/7] smbios: Require the caller to align the SMBIOS table

[Heinrich Schuchardt]

Am 28. Dezember 2023 14:37:19 MEZ schrieb Simon Glass <sjg at chromium.org>:
>Hi Ilias,
>
>On Wed, Dec 27, 2023 at 11:12 AM Ilias Apalodimas
><ilias.apalodimas at linaro.org> wrote:
>>
>> Hi Simon,
>>
>> I commented on v3 as well, but in case you miss that
>>
>> On Wed, 27 Dec 2023 at 09:40, Simon Glass <sjg at chromium.org> wrote:
>> >
>> > All callers handle this alignment, so drop the unnecessary code. This
>> > simplifies things a little.
>> >
>> > Signed-off-by: Simon Glass <sjg at chromium.org>
>> > Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> > ---
>> >
>> > (no changes since v1)
>> >
>> >  include/smbios.h | 5 +----
>> >  lib/smbios.c     | 2 --
>> >  2 files changed, 1 insertion(+), 6 deletions(-)
>> >
>> > diff --git a/include/smbios.h b/include/smbios.h
>> > index 77be58887a2..879b8a814b8 100644
>> > --- a/include/smbios.h
>> > +++ b/include/smbios.h
>> > @@ -258,12 +258,9 @@ static inline void fill_smbios_header(void *table, int type,
>> >   *
>> >   * This writes SMBIOS table at a given address.
>> >   *
>> > - * @addr:      start address to write SMBIOS table. If this is not
>> > - *             16-byte-aligned then it will be aligned before the table is
>> > - *             written.
>> > + * @addr:      start address to write SMBIOS table (must be 16-byte-aligned)
>> >   * Return:     end address of SMBIOS table (and start address for next entry)
>> >   *             or NULL in case of an error
>> > - *
>> >   */
>> >  ulong write_smbios_table(ulong addr);
>> >
>> > diff --git a/lib/smbios.c b/lib/smbios.c
>> > index 7f79d969c92..cfd451e4088 100644
>> > --- a/lib/smbios.c
>> > +++ b/lib/smbios.c
>> > @@ -563,8 +563,6 @@ ulong write_smbios_table(ulong addr)
>> >                 ctx.dev = NULL;
>> >         }
>> >
>> > -       /* 16 byte align the table address */
>> > -       addr = ALIGN(addr, 16);
>>
>> I think this is wrong.  It will break SMBIOS on a user error. I am
>> fine replacing that with a check instead and error out if the address
>> is not 16b aligned
>
>Well this is why the comment says it must be aligned. This function is
>not called willy nilly, from code we control. Checking for alignment
>at runtime creates confusion and adds to code size.

The SMBIOS tables themself have no alignment requirement. Only on non UEFI x86 system presenting a UEFI entry point in the range 000F0000h to 000FFFFFh this copy of the entry point has to be 16 byte aligned.

Best regards

Heinrich

>
>>
>> Thanks
>> /Ilias
>> >         start_addr = addr;
>> >
>> >         /*
>> > --
>> > 2.34.1
>> >
>
>Regards,
>Simon