Re: [SeaBIOS] [PATCH] biostables: Support SMBIOS 2.6+ UUID format

[Kevin O'Connor]

On Tue, Jan 12, 2016 at 02:22:41PM -0500, Cole Robinson wrote:
> SMBIOS 2.6+ stores the UUID in a different format, with the first 3
> fields in little endian format. This is what modern qemu delivers
> and what dmidecode also handles, so let's follow suit too. More
> info at this thread:
>
> http://www.seabios.org/pipermail/seabios/2015-November/010031.html
>
> The only place this affects is when reporting the UUID at startup.
> https://bugzilla.redhat.com/show_bug.cgi?id=1284259
>
> src/fw/biostables.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/src/fw/biostables.c b/src/fw/biostables.c
> index cb74396..2e15f82 100644
> --- a/src/fw/biostables.c
> +++ b/src/fw/biostables.c
> @@ -306,17 +306,46 @@ display_uuid(void)
>              if (memcmp(uuid, empty_uuid, sizeof(empty_uuid)) == 0)
>                  return;
>        printf("Machine UUID"
>               " %02x%02x%02x%02x"
>               "-%02x%02x"
>               "-%02x%02x"
>               "-%02x%02x"
>               "-%02x%02x%02x%02x%02x%02x\n"
>               , uuid[ 0], uuid[ 1], uuid[ 2], uuid[ 3]
>               , uuid[ 4], uuid[ 5]
>               , uuid[ 6], uuid[ 7]
>               , uuid[ 8], uuid[ 9]
>               , uuid[10], uuid[11], uuid[12], uuid[13], uuid[14], uuid[15]);
>        /*
>        * comment borrowed from dmidecode:
>        *
>        * As of version 2.6 of the SMBIOS specification, the first 3
>        * fields of the UUID are supposed to be encoded on little-endian.
>        * The specification says that this is the defacto standard,
>        * however I've seen systems following RFC 4122 instead and use
>        * network byte order, so I am reluctant to apply the byte-swapping
>        * for older versions.
>        */

I find this comment confusing - how about something like:

According to SMBIOS v2.6 the first three fields are encoded in
little-endian format.  Versions prior to v2.6 did not specify the
encoding, but we follow dmidecode and assume big-endian encoding.
>        if (SMBiosAddr->smbios_major_version > 2 ||
>            (SMBiosAddr->smbios_major_version == 2 &&
>             SMBiosAddr->smbios_minor_version >= 6)) {
>            printf("Machine UUID"
>                   " %02x%02x%02x%02x"
>                   "-%02x%02x"
>                   "-%02x%02x"
>                   "-%02x%02x"
>                   "-%02x%02x%02x%02x%02x%02x\n"
>                   , uuid[ 3], uuid[ 2], uuid[ 1], uuid[ 0]
>                   , uuid[ 5], uuid[ 4]
>                   , uuid[ 7], uuid[ 6]
>                   , uuid[ 8], uuid[ 9]
>                   , uuid[10], uuid[11], uuid[12]
>                   , uuid[13], uuid[14], uuid[15]);
>        } else {
>            printf("Machine UUID"
>                   " %02x%02x%02x%02x"
>                   "-%02x%02x"
>                   "-%02x%02x"
>                   "-%02x%02x"
>                   "-%02x%02x%02x%02x%02x%02x\n"
>                   , uuid[ 0], uuid[ 1], uuid[ 2], uuid[ 3]
>                   , uuid[ 4], uuid[ 5]
>                   , uuid[ 6], uuid[ 7]
>                   , uuid[ 8], uuid[ 9]
>                   , uuid[10], uuid[11], uuid[12]
>                   , uuid[13], uuid[14], uuid[15]);
>        }

The above is okay to me, but I'd like to get Gerd's comments as well,
as I think he had some concerns the last time it came up.

-Kevin

[PATCH] biostables: Support SMBIOS 2.6+ UUID format

[Cole Robinson]

On 01/14/2016 12:22 PM, Kevin O'Connor wrote:
> On Tue, Jan 12, 2016 at 02:22:41PM -0500, Cole Robinson wrote:
> > SMBIOS 2.6+ stores the UUID in a different format, with the first 3
> > fields in little endian format. This is what modern qemu delivers
> > and what dmidecode also handles, so let's follow suit too. More
> > info at this thread:
> >
> > http://www.seabios.org/pipermail/seabios/2015-November/010031.html
> >
> > The only place this affects is when reporting the UUID at startup.
> > https://bugzilla.redhat.com/show_bug.cgi?id=1284259
> >
> > src/fw/biostables.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/fw/biostables.c b/src/fw/biostables.c
> > index cb74396..2e15f82 100644
> > --- a/src/fw/biostables.c
> > +++ b/src/fw/biostables.c
> > @@ -306,17 +306,46 @@ display_uuid(void)
> >              if (memcmp(uuid, empty_uuid, sizeof(empty_uuid)) == 0)
> >                  return;
> >        printf("Machine UUID"
> >               " %02x%02x%02x%02x"
> >               "-%02x%02x"
> >               "-%02x%02x"
> >               "-%02x%02x"
> >               "-%02x%02x%02x%02x%02x%02x\n"
> >               , uuid[ 0], uuid[ 1], uuid[ 2], uuid[ 3]
> >               , uuid[ 4], uuid[ 5]
> >               , uuid[ 6], uuid[ 7]
> >               , uuid[ 8], uuid[ 9]
> >               , uuid[10], uuid[11], uuid[12], uuid[13], uuid[14], uuid[15]);
> >        /*
> >        * comment borrowed from dmidecode:
> >        *
> >        * As of version 2.6 of the SMBIOS specification, the first 3
> >        * fields of the UUID are supposed to be encoded on little-endian.
> >        * The specification says that this is the defacto standard,
> >        * however I've seen systems following RFC 4122 instead and use
> >        * network byte order, so I am reluctant to apply the byte-swapping
> >        * for older versions.
> >        */
>
> I find this comment confusing - how about something like:
>
> According to SMBIOS v2.6 the first three fields are encoded in
> little-endian format.  Versions prior to v2.6 did not specify the
> encoding, but we follow dmidecode and assume big-endian encoding.

That would be fine with me. If Gerd gives his ACK do you want to fix up the
comment or should I post a v2?

Thanks,
Cole

[PATCH] biostables: Support SMBIOS 2.6+ UUID format

[Laszlo Ersek]

On 01/14/16 18:22, Kevin O'Connor wrote:
> On Tue, Jan 12, 2016 at 02:22:41PM -0500, Cole Robinson wrote:
> > SMBIOS 2.6+ stores the UUID in a different format, with the first 3
> > fields in little endian format. This is what modern qemu delivers
> > and what dmidecode also handles, so let's follow suit too. More
> > info at this thread:
> >
> > http://www.seabios.org/pipermail/seabios/2015-November/010031.html
> >
> > The only place this affects is when reporting the UUID at startup.
> > https://bugzilla.redhat.com/show_bug.cgi?id=1284259
> >
> > src/fw/biostables.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/fw/biostables.c b/src/fw/biostables.c
> > index cb74396..2e15f82 100644
> > --- a/src/fw/biostables.c
> > +++ b/src/fw/biostables.c
> > @@ -306,17 +306,46 @@ display_uuid(void)
> >              if (memcmp(uuid, empty_uuid, sizeof(empty_uuid)) == 0)
> >                  return;
> >        printf("Machine UUID"
> >               " %02x%02x%02x%02x"
> >               "-%02x%02x"
> >               "-%02x%02x"
> >               "-%02x%02x"
> >               "-%02x%02x%02x%02x%02x%02x\n"
> >               , uuid[ 0], uuid[ 1], uuid[ 2], uuid[ 3]
> >               , uuid[ 4], uuid[ 5]
> >               , uuid[ 6], uuid[ 7]
> >               , uuid[ 8], uuid[ 9]
> >               , uuid[10], uuid[11], uuid[12], uuid[13], uuid[14], uuid[15]);
> >        /*
> >        * comment borrowed from dmidecode:
> >        *
> >        * As of version 2.6 of the SMBIOS specification, the first 3
> >        * fields of the UUID are supposed to be encoded on little-endian.
> >        * The specification says that this is the defacto standard,
> >        * however I've seen systems following RFC 4122 instead and use
> >        * network byte order, so I am reluctant to apply the byte-swapping
> >        * for older versions.
> >        */
>
> I find this comment confusing - how about something like:
>
> According to SMBIOS v2.6 the first three fields are encoded in
> little-endian format.  Versions prior to v2.6 did not specify the
> encoding, but we follow dmidecode and assume big-endian encoding.
> >        if (SMBiosAddr->smbios_major_version > 2 ||
> >            (SMBiosAddr->smbios_major_version == 2 &&
> >             SMBiosAddr->smbios_minor_version >= 6)) {
> >            printf("Machine UUID"
> >                   " %02x%02x%02x%02x"
> >                   "-%02x%02x"
> >                   "-%02x%02x"
> >                   "-%02x%02x"
> >                   "-%02x%02x%02x%02x%02x%02x\n"
> >                   , uuid[ 3], uuid[ 2], uuid[ 1], uuid[ 0]
> >                   , uuid[ 5], uuid[ 4]
> >                   , uuid[ 7], uuid[ 6]
> >                   , uuid[ 8], uuid[ 9]
> >                   , uuid[10], uuid[11], uuid[12]
> >                   , uuid[13], uuid[14], uuid[15]);
> >        } else {
> >            printf("Machine UUID"
> >                   " %02x%02x%02x%02x"
> >                   "-%02x%02x"
> >                   "-%02x%02x"
> >                   "-%02x%02x"
> >                   "-%02x%02x%02x%02x%02x%02x\n"
> >                   , uuid[ 0], uuid[ 1], uuid[ 2], uuid[ 3]
> >                   , uuid[ 4], uuid[ 5]
> >                   , uuid[ 6], uuid[ 7]
> >                   , uuid[ 8], uuid[ 9]
> >                   , uuid[10], uuid[11], uuid[12]
> >                   , uuid[13], uuid[14], uuid[15]);
> >        }
>
> The above is okay to me, but I'd like to get Gerd's comments as well,
> as I think he had some concerns the last time it came up.

I don't recall Gerd's comments from last time, but I think the case is
that some combination will be unavoidably broken.

Some older QEMU machine types exists (I checked them soon after Cole
posted this patch, but have forgotten them by now) that expose SMBIOS
version 2.8 but encode the data nonetheless in BE. If the above code is
fixed in SeaBIOS, then the UUID will be displayed incorrectly on those
old machine types; if the code is not fixed in SeaBIOS, then the UUID
will be displayed incorrectly on the recent machine types.

I think this SeaBIOS patch should go in; after all, this is the right
thing to do. Those old machine types are simply buggy (in this aspect)
in QEMU, the QEMU bugfix was restricted to new machine types for compat
reasons only. So I think this is the way forward.

(Of course this code runs on physical hardware as well, so users who
depend on the UUID display on their phys boxes might be surprised, when
they upgrade SeaBIOS.)

Ultimately this was broken by Microsoft, who ignored the preexistent BE
recommendation / practice, and went ahead with the LE encoding, ignoring
everyone else. (As I heard.) This created confusion, fragmented
implementations, and now we get to spread around the crap.

I think the patch should go in, but I agree the comment lifted from
dmidecode is confusing, and should be cleaned up.

Laszlo

[PATCH] biostables: Support SMBIOS 2.6+ UUID format

[Gerd Hoffmann]

Hi,
> > The above is okay to me, but I'd like to get Gerd's comments as well,
> > as I think he had some concerns the last time it came up.
>
> I don't recall Gerd's comments from last time, but I think the case is
> that some combination will be unavoidably broken.

Yes, that is the problem.  But there are both broken and fixed qemu
versions in the wild and there is no reliable way to figure whenever
qemu is broken or not.

So, fixing seabios and living with the breakage for some qemu versions
is the only option we have.  The problem should go away over time with
people moving on to newer versions.

Lets merge it (with the improved comment).

cheers,
  Gerd

[PATCH] biostables: Support SMBIOS 2.6+ UUID format

[Kevin O'Connor]

On Thu, Jan 14, 2016 at 12:24:29PM -0500, Cole Robinson wrote:
> On 01/14/2016 12:22 PM, Kevin O'Connor wrote:
> > According to SMBIOS v2.6 the first three fields are encoded in
> > little-endian format.  Versions prior to v2.6 did not specify the
> > encoding, but we follow dmidecode and assume big-endian encoding.
>
> That would be fine with me. If Gerd gives his ACK do you want to fix up the
> comment or should I post a v2?

Can you post a v2 and include a signed-off-by line?

Thanks,
-Kevin