diff mbox series

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

Message ID 20231227074025.2178825-8-sjg@chromium.org
State Superseded
Delegated to: Tom Rini
Headers show
Series smbios: Deal with tables beyond 4GB | expand

Commit Message

Simon Glass Dec. 27, 2023, 7:40 a.m. UTC 
All callers handle this alignment, so drop the unnecessary code. This
simplifies things a little.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

(no changes since v1)

 include/smbios.h | 5 +----
 lib/smbios.c     | 2 --
 2 files changed, 1 insertion(+), 6 deletions(-)

Comments

Ilias Apalodimas Dec. 27, 2023, 11:12 a.m. UTC | #1 
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@chromium.org> wrote:
>
> All callers handle this alignment, so drop the unnecessary code. This
> simplifies things a little.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@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

Thanks
/Ilias
>         start_addr = addr;
>
>         /*
> --
> 2.34.1
>
Simon Glass Dec. 28, 2023, 1:37 p.m. UTC | #2 
Hi Ilias,

On Wed, Dec 27, 2023 at 11:12 AM Ilias Apalodimas
<ilias.apalodimas@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@chromium.org> wrote:
> >
> > All callers handle this alignment, so drop the unnecessary code. This
> > simplifies things a little.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@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.

>
> Thanks
> /Ilias
> >         start_addr = addr;
> >
> >         /*
> > --
> > 2.34.1
> >

Regards,
Simon
Heinrich Schuchardt Dec. 28, 2023, 5:31 p.m. UTC | #3 
Am 28. Dezember 2023 14:37:19 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Ilias,
>
>On Wed, Dec 27, 2023 at 11:12 AM Ilias Apalodimas
><ilias.apalodimas@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@chromium.org> wrote:
>> >
>> > All callers handle this alignment, so drop the unnecessary code. This
>> > simplifies things a little.
>> >
>> > Signed-off-by: Simon Glass <sjg@chromium.org>
>> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@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
Simon Glass Dec. 28, 2023, 7:48 p.m. UTC | #4 
Hi Heinrich,

On Thu, Dec 28, 2023 at 5:36 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 28. Dezember 2023 14:37:19 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Ilias,
> >
> >On Wed, Dec 27, 2023 at 11:12 AM Ilias Apalodimas
> ><ilias.apalodimas@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@chromium.org> wrote:
> >> >
> >> > All callers handle this alignment, so drop the unnecessary code. This
> >> > simplifies things a little.
> >> >
> >> > Signed-off-by: Simon Glass <sjg@chromium.org>
> >> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@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
Heinrich Schuchardt Dec. 28, 2023, 8:55 p.m. UTC | #5 
Am 28. Dezember 2023 20:48:04 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Thu, Dec 28, 2023 at 5:36 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 28. Dezember 2023 14:37:19 MEZ schrieb Simon Glass <sjg@chromium.org>:
>> >Hi Ilias,
>> >
>> >On Wed, Dec 27, 2023 at 11:12 AM Ilias Apalodimas
>> ><ilias.apalodimas@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@chromium.org> wrote:
>> >> >
>> >> > All callers handle this alignment, so drop the unnecessary code. This
>> >> > simplifies things a little.
>> >> >
>> >> > Signed-off-by: Simon Glass <sjg@chromium.org>
>> >> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@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
Ilias Apalodimas Dec. 29, 2023, 6:15 a.m. UTC | #6 
On Thu, 28 Dec 2023 at 23:00, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 28. Dezember 2023 20:48:04 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Heinrich,
> >
> >On Thu, Dec 28, 2023 at 5:36 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >>
> >>
> >> Am 28. Dezember 2023 14:37:19 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >> >Hi Ilias,
> >> >
> >> >On Wed, Dec 27, 2023 at 11:12 AM Ilias Apalodimas
> >> ><ilias.apalodimas@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@chromium.org> wrote:
> >> >> >
> >> >> > All callers handle this alignment, so drop the unnecessary code. This
> >> >> > simplifies things a little.
> >> >> >
> >> >> > Signed-off-by: Simon Glass <sjg@chromium.org>
> >> >> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@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@linaro.org>
diff mbox series

Patch

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);
 	start_addr = addr;
 
 	/*