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

[Simon Glass]

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?

Regards,
Simon