[PATCH RFC] dmi-scan: Use little-endian for the first 3 fields of the UUID.

[PATCH RFC] dmi-scan: Use little-endian for the first 3 fields of the UUID.

[Anisse Astier]

From: Pascal VITOUX <pascal@substantiel.fr>

 - Get SMBIOS version.
 - Byte-swap the first 3 fields of the UUID (DMI type 1) as off SMBIOS
   version 2.6.

This patch is an adaptation of Jean Delvare patches for dmidecode
rev1.100, rev1.01 and rev1.119.
http://cvs.savannah.gnu.org/viewvc/dmidecode/dmidecode.c?root=dmidecode&view=log

It is intended to get the same uuid from dmidecode tool as from sysfs kernel
tree, more compliant with SMBIOS specification.
Therefore this patch will have the kernel return a different UUID if you
are using a recent BIOS implementing SMBIOS >= 2.6.

Signed-off-by: Pascal VITOUX <pascal@substantiel.fr>
Signed-off-by: Anisse Astier <anisse@astier.eu>
---
Hi,

I'd like to get some feedback on this patch. It doesn't modify the
API/ABI, but modifies the value returned by kernel on a given hardware,
so it could potentially break a (very) badly written app.

Disclaimer: Although I've applied my Signed-off-by, Pascal and I work for
the same company.

Regards,
Anisse

---
 drivers/firmware/dmi_scan.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index e28e41668..b6278a7 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -99,6 +99,7 @@ static void dmi_table(u8 *buf, int len, int num,
 static u32 dmi_base;
 static u16 dmi_len;
 static u16 dmi_num;
+static u16 dmi_ver;
 
 static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 		void *))
@@ -169,7 +170,18 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, int inde
 	if (!s)
 		return;
 
-	sprintf(s, "%pUB", d);
+	/*
+	 * As off 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.
+	 */
+	if (dmi_ver >= 0x0206)
+		sprintf(s, "%pUL", d);
+	else
+		sprintf(s, "%pUB", d);
 
         dmi_ident[slot] = s;
 }
@@ -400,9 +412,29 @@ static int __init dmi_present(const char __iomem *p)
 		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
 			(buf[9] << 8) | buf[8];
 
+		/* SMBIOS version */
+		dmi_ver = (*(u8 *)(p - 0x10 + 0x06) << 8) +
+			*(u8 *)(p - 0x10 + 0x07);
+
+		/* Some BIOS report weird SMBIOS version, fix that up */
+		switch (dmi_ver) {
+		case 0x021F:
+			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
+			       31, 3);
+			dmi_ver = 0x0203;
+			break;
+		case 0x0233:
+			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
+			       51, 6);
+			dmi_ver = 0x0206;
+			break;
+		}
+		printk(KERN_INFO "SMBIOS version %d.%d.\n", dmi_ver >> 8,
+		       dmi_ver & 0xFF);
+
 		/*
 		 * DMI version 0.0 means that the real version is taken from
-		 * the SMBIOS version, which we don't know at this point.
+		 * the SMBIOS version.
 		 */
 		if (buf[14] != 0)
 			printk(KERN_INFO "DMI %d.%d present.\n",
-- 
1.7.3.2

Re: [PATCH RFC] dmi-scan: Use little-endian for the first 3 fields of the UUID.

[Bjorn Helgaas]

On Wednesday, January 19, 2011 10:54:11 am Anisse Astier wrote:
> 
> From: Pascal VITOUX <pascal@substantiel.fr>
> 
>  - Get SMBIOS version.
>  - Byte-swap the first 3 fields of the UUID (DMI type 1) as off SMBIOS
>    version 2.6.
> 
> This patch is an adaptation of Jean Delvare patches for dmidecode
> rev1.100, rev1.01 and rev1.119.
> http://cvs.savannah.gnu.org/viewvc/dmidecode/dmidecode.c?root=dmidecode&view=log
> 
> It is intended to get the same uuid from dmidecode tool as from sysfs kernel
> tree, more compliant with SMBIOS specification.
> Therefore this patch will have the kernel return a different UUID if you
> are using a recent BIOS implementing SMBIOS >= 2.6.

It seems like a good idea to have /sys/class/dmi/id/product_uuid
match what dmidecode reports.  Does dmidecode use the same algorithm
(testing dmi_ver against 0x0206)?

It'd be nice to mention that /sys/... filename explicitly in the
changelog, and maybe even include an example from a system where
they previously did not match, and now do.

I don't have an opinion on the SMBIOS version stuff, but that looks
like it should be split into a separate patch.

Bjorn

> Signed-off-by: Pascal VITOUX <pascal@substantiel.fr>
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> ---
> Hi,
> 
> I'd like to get some feedback on this patch. It doesn't modify the
> API/ABI, but modifies the value returned by kernel on a given hardware,
> so it could potentially break a (very) badly written app.
> 
> Disclaimer: Although I've applied my Signed-off-by, Pascal and I work for
> the same company.
> 
> Regards,
> Anisse
> 
> ---
>  drivers/firmware/dmi_scan.c |   36 ++++++++++++++++++++++++++++++++++--
>  1 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index e28e41668..b6278a7 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -99,6 +99,7 @@ static void dmi_table(u8 *buf, int len, int num,
>  static u32 dmi_base;
>  static u16 dmi_len;
>  static u16 dmi_num;
> +static u16 dmi_ver;
>  
>  static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>  		void *))
> @@ -169,7 +170,18 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, int inde
>  	if (!s)
>  		return;
>  
> -	sprintf(s, "%pUB", d);
> +	/*
> +	 * As off 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.
> +	 */
> +	if (dmi_ver >= 0x0206)
> +		sprintf(s, "%pUL", d);
> +	else
> +		sprintf(s, "%pUB", d);
>  
>          dmi_ident[slot] = s;
>  }
> @@ -400,9 +412,29 @@ static int __init dmi_present(const char __iomem *p)
>  		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
>  			(buf[9] << 8) | buf[8];
>  
> +		/* SMBIOS version */
> +		dmi_ver = (*(u8 *)(p - 0x10 + 0x06) << 8) +
> +			*(u8 *)(p - 0x10 + 0x07);
> +
> +		/* Some BIOS report weird SMBIOS version, fix that up */
> +		switch (dmi_ver) {
> +		case 0x021F:
> +			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
> +			       31, 3);
> +			dmi_ver = 0x0203;
> +			break;
> +		case 0x0233:
> +			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
> +			       51, 6);
> +			dmi_ver = 0x0206;
> +			break;
> +		}
> +		printk(KERN_INFO "SMBIOS version %d.%d.\n", dmi_ver >> 8,
> +		       dmi_ver & 0xFF);
> +
>  		/*
>  		 * DMI version 0.0 means that the real version is taken from
> -		 * the SMBIOS version, which we don't know at this point.
> +		 * the SMBIOS version.
>  		 */
>  		if (buf[14] != 0)
>  			printk(KERN_INFO "DMI %d.%d present.\n",
> 

Re: [PATCH RFC] dmi-scan: Use little-endian for the first 3 fields of the UUID.

[Jean Delvare]

Hi Anisse,

On Wed, 19 Jan 2011 18:54:11 +0100, Anisse Astier wrote:
> 
> From: Pascal VITOUX <pascal@substantiel.fr>
> 
>  - Get SMBIOS version.
>  - Byte-swap the first 3 fields of the UUID (DMI type 1) as off SMBIOS
>    version 2.6.
> 
> This patch is an adaptation of Jean Delvare patches for dmidecode
> rev1.100, rev1.01 and rev1.119.
> http://cvs.savannah.gnu.org/viewvc/dmidecode/dmidecode.c?root=dmidecode&view=log

Note that I am not particularly proud of this patch, and even thought
of reverting it at some point. It is possible that dmidecode 2.12 will
have different code for UUID decoding. The current behavior would stay
the default, but the user would be able to force one decoding order
through a command line option.  This was suggested a long time ago:
http://www.mail-archive.com/dmidecode-devel@nongnu.org/msg00096.html
I disregarded the proposal back then. Now... Well, I'm still not sure :(

> It is intended to get the same uuid from dmidecode tool as from sysfs kernel
> tree, more compliant with SMBIOS specification.

I understand that it makes sense to have dmidecode and the kernel
return the same value. But the problem is that it appears that, in
practice, vendors don't follow the recommendations of the SMBIOS 2.6
specifications. On most machines I've checked, the network byte order
is used, per RFC4122.

Also, while the change in dmidecode was applied at the time SMBIOS 2.6
implementations were rare, this is no longer true. So a change now may
have consequences.

> Therefore this patch will have the kernel return a different UUID if you
> are using a recent BIOS implementing SMBIOS >= 2.6.
> 
> Signed-off-by: Pascal VITOUX <pascal@substantiel.fr>
> Signed-off-by: Anisse Astier <anisse@astier.eu>
> ---
> Hi,
> 
> I'd like to get some feedback on this patch. It doesn't modify the
> API/ABI, but modifies the value returned by kernel on a given hardware,
> so it could potentially break a (very) badly written app.

I fail to see why an application relying on the UUID would qualify as
"badly written". The UUID seems like a reasonable thing to use for any
inventory application, as a mean to uniquely identify machines. Using
it for licensing purpose also makes sense (whether you you like
proprietary software or not is irrelevant.)

This is a desperate case anyway, there simply is no clean solution to
this problem. Whoever suggested the addition of this note in the SMBIOS
2.6 specification should be banned permanently from ever contributing
to any technical document.

> Disclaimer: Although I've applied my Signed-off-by, Pascal and I work for
> the same company.
> 
> Regards,
> Anisse
> 
> ---
>  drivers/firmware/dmi_scan.c |   36 ++++++++++++++++++++++++++++++++++--
>  1 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index e28e41668..b6278a7 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -99,6 +99,7 @@ static void dmi_table(u8 *buf, int len, int num,
>  static u32 dmi_base;
>  static u16 dmi_len;
>  static u16 dmi_num;
> +static u16 dmi_ver;
>  
>  static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
>  		void *))
> @@ -169,7 +170,18 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, int inde
>  	if (!s)
>  		return;
>  
> -	sprintf(s, "%pUB", d);
> +	/*
> +	 * As off 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.
> +	 */
> +	if (dmi_ver >= 0x0206)
> +		sprintf(s, "%pUL", d);
> +	else
> +		sprintf(s, "%pUB", d);
>  
>          dmi_ident[slot] = s;
>  }
> @@ -400,9 +412,29 @@ static int __init dmi_present(const char __iomem *p)
>  		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
>  			(buf[9] << 8) | buf[8];
>  
> +		/* SMBIOS version */
> +		dmi_ver = (*(u8 *)(p - 0x10 + 0x06) << 8) +
> +			*(u8 *)(p - 0x10 + 0x07);

You can't do that, sorry. Only the 15 bytes of the _DMI_ entry point
have been iomap'd, the 16 bytes of the _SM_ entry point (which may not
even exist!) have not. I suspect you'll have to rework the loop in
dmi_scan_machine() and the way it calls dmi_present() quite a bit.

> +
> +		/* Some BIOS report weird SMBIOS version, fix that up */
> +		switch (dmi_ver) {
> +		case 0x021F:
> +			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
> +			       31, 3);
> +			dmi_ver = 0x0203;
> +			break;
> +		case 0x0233:
> +			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
> +			       51, 6);
> +			dmi_ver = 0x0206;
> +			break;
> +		}

Note that I've added one more fixup meanwhile:
  http://cvs.savannah.gnu.org/viewvc/dmidecode/dmidecode.c?root=dmidecode&r1=1.145&r2=1.146

> +		printk(KERN_INFO "SMBIOS version %d.%d.\n", dmi_ver >> 8,
> +		       dmi_ver & 0xFF);
> +
>  		/*
>  		 * DMI version 0.0 means that the real version is taken from
> -		 * the SMBIOS version, which we don't know at this point.
> +		 * the SMBIOS version.

Changing the comment without changing the code that follows it makes no
sense.

>  		 */
>  		if (buf[14] != 0)
>  			printk(KERN_INFO "DMI %d.%d present.\n",


-- 
Jean Delvare

Re: [PATCH RFC] dmi-scan: Use little-endian for the first 3 fields of the UUID.

[Anisse Astier]

Hi Bjorn,

On Wed, 19 Jan 2011 12:01:02 -0700, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote :

> On Wednesday, January 19, 2011 10:54:11 am Anisse Astier wrote:
> > 
> > From: Pascal VITOUX <pascal@substantiel.fr>
> > 
> >  - Get SMBIOS version.
> >  - Byte-swap the first 3 fields of the UUID (DMI type 1) as off SMBIOS
> >    version 2.6.
> > 
> > This patch is an adaptation of Jean Delvare patches for dmidecode
> > rev1.100, rev1.01 and rev1.119.
> > http://cvs.savannah.gnu.org/viewvc/dmidecode/dmidecode.c?root=dmidecode&view=log
> > 
> > It is intended to get the same uuid from dmidecode tool as from sysfs kernel
> > tree, more compliant with SMBIOS specification.
> > Therefore this patch will have the kernel return a different UUID if you
> > are using a recent BIOS implementing SMBIOS >= 2.6.
> 
> It seems like a good idea to have /sys/class/dmi/id/product_uuid
> match what dmidecode reports.  Does dmidecode use the same algorithm
> (testing dmi_ver against 0x0206)?
Yes, it does.

> 
> It'd be nice to mention that /sys/... filename explicitly in the
> changelog, and maybe even include an example from a system where
> they previously did not match, and now do.
Indeed.

> 
> I don't have an opinion on the SMBIOS version stuff, but that looks
> like it should be split into a separate patch.
It seems it's wrong anyway and will need more patch splitting (see Jean
Delvare's email).

Thanks for the review,

Anisse

Re: [PATCH RFC] dmi-scan: Use little-endian for the first 3 fields of the UUID.

[Anisse Astier]

Hi Jean,

Thank you for taking the time to answer and review,

On Wed, 19 Jan 2011 21:39:36 +0100, Jean Delvare <khali@linux-fr.org> wrote :

> Hi Anisse,
> 
> On Wed, 19 Jan 2011 18:54:11 +0100, Anisse Astier wrote:
> > 
> > From: Pascal VITOUX <pascal@substantiel.fr>
> > 
> >  - Get SMBIOS version.
> >  - Byte-swap the first 3 fields of the UUID (DMI type 1) as off SMBIOS
> >    version 2.6.
> > 
> > This patch is an adaptation of Jean Delvare patches for dmidecode
> > rev1.100, rev1.01 and rev1.119.
> > http://cvs.savannah.gnu.org/viewvc/dmidecode/dmidecode.c?root=dmidecode&view=log
> 
> Note that I am not particularly proud of this patch, and even thought
> of reverting it at some point. It is possible that dmidecode 2.12 will
> have different code for UUID decoding. The current behavior would stay
> the default, but the user would be able to force one decoding order
> through a command line option.  This was suggested a long time ago:
> http://www.mail-archive.com/dmidecode-devel@nongnu.org/msg00096.html
> I disregarded the proposal back then. Now... Well, I'm still not sure :(
> 
> > It is intended to get the same uuid from dmidecode tool as from sysfs kernel
> > tree, more compliant with SMBIOS specification.
> 
> I understand that it makes sense to have dmidecode and the kernel
> return the same value. But the problem is that it appears that, in
> practice, vendors don't follow the recommendations of the SMBIOS 2.6
> specifications. On most machines I've checked, the network byte order
> is used, per RFC4122.
> 
> Also, while the change in dmidecode was applied at the time SMBIOS 2.6
> implementations were rare, this is no longer true. So a change now may
> have consequences.

Yes, unfortunately.

> 
> > Therefore this patch will have the kernel return a different UUID if you
> > are using a recent BIOS implementing SMBIOS >= 2.6.
> > 
> > Signed-off-by: Pascal VITOUX <pascal@substantiel.fr>
> > Signed-off-by: Anisse Astier <anisse@astier.eu>
> > ---
> > Hi,
> > 
> > I'd like to get some feedback on this patch. It doesn't modify the
> > API/ABI, but modifies the value returned by kernel on a given hardware,
> > so it could potentially break a (very) badly written app.
> 
> I fail to see why an application relying on the UUID would qualify as
> "badly written". The UUID seems like a reasonable thing to use for any
Yep, my  bad, that's not what I meant. Replace "break" by "crash", and
badly written app by "app that doesn't fail graciously if returned a
different value".


> inventory application, as a mean to uniquely identify machines. Using
> it for licensing purpose also makes sense (whether you you like
> proprietary software or not is irrelevant.)
Glad we agree, that's what we use it for.

> 
> This is a desperate case anyway, there simply is no clean solution to
> this problem. Whoever suggested the addition of this note in the SMBIOS
> 2.6 specification should be banned permanently from ever contributing
> to any technical document.
What do you suggest we do ?

Something needs to be done about that. You already have a problem if
you're relying on DMI UUIDs. An example of an app  using HAL(obsolete, I
know): Old versions of HAL would call dmidecode to provide the fetch the
DMI UUID. But if you upgrade to a more recent version, it will get the
information directly from kernel. And now you have it, a different UUID
for the same hardware.


Now, I can see three different "solutions" to bring kernel and dmidecode
on par :
 (1) we keep the current kernel behavior and next dmidecode version changes
     its default. It will break applications relying on dmidecode.
 (2) we keep dmidecode default and change kernel behavior. It will break
     applications relying on kernel DMI UUID.
 (3) we add a new kernel sysfs file that provides the UUID with the
     SMBIOS 2.6 behavior. Current breakage is patchable in apps by
     switching them to the new file.

> 
> > Disclaimer: Although I've applied my Signed-off-by, Pascal and I work for
> > the same company.
> > 
> > Regards,
> > Anisse
> > 
> > ---
> >  drivers/firmware/dmi_scan.c |   36 ++++++++++++++++++++++++++++++++++--
> >  1 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > index e28e41668..b6278a7 100644
> > --- a/drivers/firmware/dmi_scan.c
> > +++ b/drivers/firmware/dmi_scan.c
> > @@ -99,6 +99,7 @@ static void dmi_table(u8 *buf, int len, int num,
> >  static u32 dmi_base;
> >  static u16 dmi_len;
> >  static u16 dmi_num;
> > +static u16 dmi_ver;
> >  
> >  static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
> >  		void *))
> > @@ -169,7 +170,18 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, int inde
> >  	if (!s)
> >  		return;
> >  
> > -	sprintf(s, "%pUB", d);
> > +	/*
> > +	 * As off 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.
> > +	 */
> > +	if (dmi_ver >= 0x0206)
> > +		sprintf(s, "%pUL", d);
> > +	else
> > +		sprintf(s, "%pUB", d);
> >  
> >          dmi_ident[slot] = s;
> >  }
> > @@ -400,9 +412,29 @@ static int __init dmi_present(const char __iomem *p)
> >  		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
> >  			(buf[9] << 8) | buf[8];
> >  
> > +		/* SMBIOS version */
> > +		dmi_ver = (*(u8 *)(p - 0x10 + 0x06) << 8) +
> > +			*(u8 *)(p - 0x10 + 0x07);
> 
> You can't do that, sorry. Only the 15 bytes of the _DMI_ entry point
> have been iomap'd, the 16 bytes of the _SM_ entry point (which may not
> even exist!) have not. I suspect you'll have to rework the loop in
> dmi_scan_machine() and the way it calls dmi_present() quite a bit.
Indeed, I didn't notice that.

I think the loop is safe, because we ioremap 64k of data. Only the
first call to dmi_present seems to be risky. (just FYI, code worked in
our tests)

> 
> > +
> > +		/* Some BIOS report weird SMBIOS version, fix that up */
> > +		switch (dmi_ver) {
> > +		case 0x021F:
> > +			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
> > +			       31, 3);
> > +			dmi_ver = 0x0203;
> > +			break;
> > +		case 0x0233:
> > +			printk(KERN_INFO "SMBIOS version fixup (2.%d -> 2.%d).\n",
> > +			       51, 6);
> > +			dmi_ver = 0x0206;
> > +			break;
> > +		}
> 
> Note that I've added one more fixup meanwhile:
>   http://cvs.savannah.gnu.org/viewvc/dmidecode/dmidecode.c?root=dmidecode&r1=1.145&r2=1.146
Yes, this should be added too. As you have guessed, we wrote this code
prior to this new fixup.

> 
> > +		printk(KERN_INFO "SMBIOS version %d.%d.\n", dmi_ver >> 8,
> > +		       dmi_ver & 0xFF);
> > +
> >  		/*
> >  		 * DMI version 0.0 means that the real version is taken from
> > -		 * the SMBIOS version, which we don't know at this point.
> > +		 * the SMBIOS version.
> 
> Changing the comment without changing the code that follows it makes no
> sense.
Indeed, the comment didn't make sense without changes, but the following
code (and previous printf) either.

Does this look better (removing previous printf):
                /*
                 * DMI version 0.0 means that the real version is taken from
                 * the SMBIOS version.
                 */
                if (buf[14] != 0)
                        printk(KERN_INFO "DMI %d.%d present.\n",
                               buf[14] >> 4, buf[14] & 0xF);
                else
                        printk(KERN_INFO "DMI %d.%d(SMBIOS) present.\n", dmi_ver >> 8,
                       dmi_ver & 0xFF);



Regards,
Anisse

> 
> >  		 */
> >  		if (buf[14] != 0)
> >  			printk(KERN_INFO "DMI %d.%d present.\n",
> 
> 

Re: [PATCH RFC] dmi-scan: Use little-endian for the first 3 fields of the UUID.

[Jean Delvare]

Hi Anisse,

Sorry for the late reply.

On Thu, 20 Jan 2011 18:03:14 +0100, Anisse Astier wrote:
> Hi Jean,
> 
> Thank you for taking the time to answer and review,
> 
> On Wed, 19 Jan 2011 21:39:36 +0100, Jean Delvare <khali@linux-fr.org> wrote :
> > (...) The UUID seems like a reasonable thing to use for any
> > inventory application, as a mean to uniquely identify machines. Using
> > it for licensing purpose also makes sense (whether you you like
> > proprietary software or not is irrelevant.)
> Glad we agree, that's what we use it for.

I would suggest application authors using the UUID for licensing
purposes to consider the two forms of the UUID as equivalent. That is,
get the UUID from dmidecode or the kernel or HAL or whatever, byte-swap
it, and have the license checking code test both variants and succeed if
either works. This will by far lead to the best user experience.

For inventory purposes, the problem is more complex, but it would
probably make sense to add some code to detect duplicate entries. The
likeliness that a given company has two systems with UUIDs being the
byte-swapped counterpart of each other is very, very small, so this
case could be detected and reported (or silently worked around.)

> > This is a desperate case anyway, there simply is no clean solution to
> > this problem. Whoever suggested the addition of this note in the SMBIOS
> > 2.6 specification should be banned permanently from ever contributing
> > to any technical document.
> What do you suggest we do ?
> 
> Something needs to be done about that. You already have a problem if
> you're relying on DMI UUIDs. An example of an app  using HAL(obsolete, I
> know): Old versions of HAL would call dmidecode to provide the fetch the
> DMI UUID. But if you upgrade to a more recent version, it will get the
> information directly from kernel. And now you have it, a different UUID
> for the same hardware.
> 
> 
> Now, I can see three different "solutions" to bring kernel and dmidecode
> on par :
>  (1) we keep the current kernel behavior and next dmidecode version changes
>      its default. It will break applications relying on dmidecode.

I don't really want to do this. While I now believe that changing the
decoding based on SMBIOS version in dmidecode 2.10 was a mistake, it
happened too long ago to change it now. If anything, I'll add an option
to force the decoding either way, but I won't change the default again.

The worst case I've seen with dmidecode 2.10 (which is what you want to
implement in the kernel) was during a BIOS update of one of my
machines. The board manufacturer switched from SMBIOS 2.5 to SMBIOS 2.6
as part of the BIOS update, presumably so that they could report new
CPU models properly. But they did NOT change the order of the UUID
bytes. As a result, dmidecode 2.10 reported a different UUID before and
after the BIOS update. While you can claim this is the board
manufacturer's fault, the issue still exists for the user in the end.

>  (2) we keep dmidecode default and change kernel behavior. It will break
>      applications relying on kernel DMI UUID.

The only advantage with this option compared to #1 is that at least we
comply with the SMBIOS specification.

>  (3) we add a new kernel sysfs file that provides the UUID with the
>      SMBIOS 2.6 behavior. Current breakage is patchable in apps by
>      switching them to the new file.

This avoids changing an existing interface. But I'm not sure if it
actually solves your problem with HAL, as presumably HAL will still use
the original product_uuid attribute, with no chance for a fix as, as I
understand it, HAL is no longer developed.

So it may be preferable to change the default to what dmidecode 2.10
does (i.e. option #2), and introduce a new file (e.g.
product_uuid_rfc4122) implementing the original behavior, so that
application have the option to switch to this file if they want to
preserve compatibility (I call this option #4).

This will add some confusion to have two uuid attributes, but hopefully
we can live with this.

I have no strong opinion between options #2 and #4. Anyway, as I said
before, the damage is already done by the SMBIOS specification folks.
They managed to confuse OS developers and board makers alike. Whatever
we do can't solve it completely.

> > > (...)
> > > @@ -400,9 +412,29 @@ static int __init dmi_present(const char __iomem *p)
> > >  		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
> > >  			(buf[9] << 8) | buf[8];
> > >  
> > > +		/* SMBIOS version */
> > > +		dmi_ver = (*(u8 *)(p - 0x10 + 0x06) << 8) +
> > > +			*(u8 *)(p - 0x10 + 0x07);
> > 
> > You can't do that, sorry. Only the 15 bytes of the _DMI_ entry point
> > have been iomap'd, the 16 bytes of the _SM_ entry point (which may not
> > even exist!) have not. I suspect you'll have to rework the loop in
> > dmi_scan_machine() and the way it calls dmi_present() quite a bit.
> Indeed, I didn't notice that.
> 
> I think the loop is safe, because we ioremap 64k of data. Only the
> first call to dmi_present seems to be risky. (just FYI, code worked in
> our tests)

I am not familiar with memcpy_fromio(), but I can only guess that it is
there for a reason. Maybe it worked in your test but would break on
another architecture. I can see that the implementation of
memcpy_fromio is arch-specific.

> > > (...)
> > > +		printk(KERN_INFO "SMBIOS version %d.%d.\n", dmi_ver >> 8,
> > > +		       dmi_ver & 0xFF);
> > > +
> > >  		/*
> > >  		 * DMI version 0.0 means that the real version is taken from
> > > -		 * the SMBIOS version, which we don't know at this point.
> > > +		 * the SMBIOS version.
> > 
> > Changing the comment without changing the code that follows it makes no
> > sense.
> Indeed, the comment didn't make sense without changes, but the following
> code (and previous printf) either.
> 
> Does this look better (removing previous printf):
>                 /*
>                  * DMI version 0.0 means that the real version is taken from
>                  * the SMBIOS version.
>                  */
>                 if (buf[14] != 0)
>                         printk(KERN_INFO "DMI %d.%d present.\n",
>                                buf[14] >> 4, buf[14] & 0xF);
>                 else
>                         printk(KERN_INFO "DMI %d.%d(SMBIOS) present.\n", dmi_ver >> 8,
>                        dmi_ver & 0xFF);

Something like this, yes. Except that you don't have to make a
different string for both cases, this doesn't add any value and wastes
memory. Also, the version from the _SM_ entry point should be preferred
over the _DMI_ version when both are available, as the _SM_ one has
more room for the version (which doesn't matter today, but will later
if we ever reach SMBIOS specification 2.16.) Lastly, I see no reason to
not set dmi_ver from the _DMI_ entry point if the _SM_ entry point
isn't available.

-- 
Jean Delvare