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

[Ilias Apalodimas]

On Thu, 28 Dec 2023 at 23:00, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> 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.
> >

Thanks Heinrich, this makes sense

> >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

Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>