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

[Heinrich Schuchardt]

Am 28. Dezember 2023 20:48:04 MEZ schrieb Simon Glass <sjg at chromium.org>:
>Hi Heinrich,
>
>On Thu, Dec 28, 2023 at 5:36 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>>
>>
>> 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.
>
>OK. So perhaps I should reword the comment to say that any
>arch-specific alignment must be respected by the caller?

Just mentioning the requirement from the spec in the function description should be good enough.

Best regards

Heinrich

>
>Regards,
>Simon