memcpy_fromio in dmi_scan.c

memcpy_fromio in dmi_scan.c

[Jean Delvare]

Hi Duan, Andrew,

I am looking at the following commit:

commit 9f9c9cbb60576a1518d0bf93fb8e499cffccf377
Author: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Date:   Thu Dec 20 15:05:14 2012 -0800

    drivers/firmware/dmi_scan.c: fetch dmi version from SMBIOS if it exists

And I am worried about calls to memcpy_fromio(), or lack thereof. Before
this commit, the code would take great care to always call
memcpy_fromio() to get data from the 0xF0000-0xFFFFF memory range (BIOS
data) and operate on that copy. After this commit, the code is happily
calling memcmp() directly on an __iomem pointer. It seems to be harmless
on x86, but it will break on IA64, won't it?

-- 
Jean Delvare
Suse L3

Re: memcpy_fromio in dmi_scan.c

[DuanZhenzhong]

Jean Delvare wrote:
> Hi Duan, Andrew,
>
> I am looking at the following commit:
>
> commit 9f9c9cbb60576a1518d0bf93fb8e499cffccf377
> Author: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Date:   Thu Dec 20 15:05:14 2012 -0800
>
>     drivers/firmware/dmi_scan.c: fetch dmi version from SMBIOS if it exists
>
> And I am worried about calls to memcpy_fromio(), or lack thereof. Before
> this commit, the code would take great care to always call
> memcpy_fromio() to get data from the 0xF0000-0xFFFFF memory range (BIOS
> data) and operate on that copy. After this commit, the code is happily
> calling memcmp() directly on an __iomem pointer. It seems to be harmless
> on x86, but it will break on IA64, won't it?
>
>   
Hi Jean,
What's the impact of reading bios data directly on IA64?
Sorry I have little knowledge about IA64.

zduan

Re: memcpy_fromio in dmi_scan.c

[Jean Delvare]

Le Tuesday 23 April 2013 à 11:25 +0800, DuanZhenzhong a écrit :
> Jean Delvare wrote:
> > Hi Duan, Andrew,
> >
> > I am looking at the following commit:
> >
> > commit 9f9c9cbb60576a1518d0bf93fb8e499cffccf377
> > Author: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> > Date:   Thu Dec 20 15:05:14 2012 -0800
> >
> >     drivers/firmware/dmi_scan.c: fetch dmi version from SMBIOS if it exists
> >
> > And I am worried about calls to memcpy_fromio(), or lack thereof. Before
> > this commit, the code would take great care to always call
> > memcpy_fromio() to get data from the 0xF0000-0xFFFFF memory range (BIOS
> > data) and operate on that copy. After this commit, the code is happily
> > calling memcmp() directly on an __iomem pointer. It seems to be harmless
> > on x86, but it will break on IA64, won't it?
> >
> >   
> Hi Jean,
> What's the impact of reading bios data directly on IA64?
> Sorry I have little knowledge about IA64.

I don't have much knowledge about IA64 either. All I see is that while
x86 implements memcpy_fromio() with memcpy [1], ia64 implements it with
readb [2]. There must be a reason for that, and I can only suppose that
memcpy on __iomem pointers doesn't work on IA64. If memcpy doesn't work
then I can't see memcmp working.

[1] http://lxr.linux.no/#linux+v3.8.8/arch/x86/include/asm/io.h#L209
[2] http://lxr.linux.no/#linux+v3.8.8/arch/ia64/lib/io.c#L10

Tony, Fenghua? Did anyone test DMI support on IA64 in kernel 3.8.0+,
3.4.28+, 3.2.38+ or 3.061+?

-- 
Jean Delvare
Suse L3

RE: memcpy_fromio in dmi_scan.c

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1062 bytes --]

> I don't have much knowledge about IA64 either. All I see is that while
> x86 implements memcpy_fromio() with memcpy [1], ia64 implements it with
> readb [2]. There must be a reason for that, and I can only suppose that
> memcpy on __iomem pointers doesn't work on IA64. If memcpy doesn't work
> then I can't see memcmp working.

On most platforms readb() just ends up doing a regular dereference of the
address ... so I'd expect that memcmp would work just fine.  The exception
is the old SGI sn2 which end up calling ___sn_readb() ... which does something
weird with sn_dma_flush() after doing the dereference of *addr.  I don't
really understand what is going on here - but I assume that it is for real
PCI iomapped memory, as opposed to random bits of BIOS reserved memory
that we used ioremap() to get a virtual handle on.

Not sure how to test ... what would I run to exercise this code?

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: memcpy_fromio in dmi_scan.c

[Jean Delvare]

Hi Tony,

Thanks for stepping in.

Le Tuesday 23 April 2013 à 22:00 +0000, Luck, Tony a écrit :
> > I don't have much knowledge about IA64 either. All I see is that while
> > x86 implements memcpy_fromio() with memcpy [1], ia64 implements it with
> > readb [2]. There must be a reason for that, and I can only suppose that
> > memcpy on __iomem pointers doesn't work on IA64. If memcpy doesn't work
> > then I can't see memcmp working.
> 
> On most platforms readb() just ends up doing a regular dereference of the
> address ... so I'd expect that memcmp would work just fine.  The exception
> is the old SGI sn2 which end up calling ___sn_readb() ... which does something
> weird with sn_dma_flush() after doing the dereference of *addr.  I don't
> really understand what is going on here - but I assume that it is for real
> PCI iomapped memory, as opposed to random bits of BIOS reserved memory
> that we used ioremap() to get a virtual handle on.

Ah, OK. I tested on a HP RX8640 server and to my surprised it worked
fine. But now your explanation tells me why. Now I also tested on a
Supermicro I8QBH, and it worked. But on a SGI SN2 I get the following in
dmesg (with kernel 3.0.70, which has the suspicious code changes):

DMI not present or invalid.

> Not sure how to test ... what would I run to exercise this code?

It is executed at boot time, if it works you get something like:

SMBIOS 2.7 present.

or:

DMI present.

in dmesg. If not, you get instead:

DMI not present or invalid.

That being said, "my" SN2 machine was previously running kernel 3.0.34
which has the old dmi_scan code and it also said "DMI not present or
invalid." Plus dmidecode fails on this machine with:

/sys/firmware/efi/systab: SMBIOS entry point missing

So it might as well be that DMI support on SN2 was already broken.
Please let me know your findings on other SN2 machines.

Anyway, either memcpy_fromio is needed and the code should be fixed, or
it isn't and the code should be simplified. The current state makes
little sense.

What is strange is that the call to memcpy_fromio was added a long time
ago, long before dmi_scan was enabled on ia64.

-- 
Jean Delvare
Suse L3

RE: memcpy_fromio in dmi_scan.c

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 610 bytes --]

> That being said, "my" SN2 machine was previously running kernel 3.0.34
> which has the old dmi_scan code and it also said "DMI not present or
> invalid." Plus dmidecode fails on this machine with:
>
> /sys/firmware/efi/systab: SMBIOS entry point missing
>
> So it might as well be that DMI support on SN2 was already broken.
> Please let me know your findings on other SN2 machines.

I don't have an sn2 to test. Added Robin Holt to Cc list

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: memcpy_fromio in dmi_scan.c

[Zhenzhong Duan]

On 2013-04-25 03:22, Jean Delvare wrote:
> What is strange is that the call to memcpy_fromio was added a long 
> time ago, long before dmi_scan was enabled on ia64. 
I think memcpy_fromio is used to copy bios data to a mem buffer to speed 
up read access.

zduan

Re: memcpy_fromio in dmi_scan.c

[Robin Holt]

On Wed, Apr 24, 2013 at 08:16:50PM +0000, Luck, Tony wrote:
> > That being said, "my" SN2 machine was previously running kernel 3.0.34
> > which has the old dmi_scan code and it also said "DMI not present or
> > invalid." Plus dmidecode fails on this machine with:
> >
> > /sys/firmware/efi/systab: SMBIOS entry point missing
> >
> > So it might as well be that DMI support on SN2 was already broken.
> > Please let me know your findings on other SN2 machines.
> 
> I don't have an sn2 to test. Added Robin Holt to Cc list

I will try and boot an sn2 machine first thing tomorrow morning.
I don't recall dmidecode being broken, but that was quite a while ago.
I thought the license validation code we used on sn2 used data from the
dmidecode output.

Robin

Re: memcpy_fromio in dmi_scan.c

[Robin Holt]

On Wed, Apr 24, 2013 at 09:07:24PM -0500, Robin Holt wrote:
> On Wed, Apr 24, 2013 at 08:16:50PM +0000, Luck, Tony wrote:
> > > That being said, "my" SN2 machine was previously running kernel 3.0.34
> > > which has the old dmi_scan code and it also said "DMI not present or
> > > invalid." Plus dmidecode fails on this machine with:
> > >
> > > /sys/firmware/efi/systab: SMBIOS entry point missing
> > >
> > > So it might as well be that DMI support on SN2 was already broken.
> > > Please let me know your findings on other SN2 machines.
> > 
> > I don't have an sn2 to test. Added Robin Holt to Cc list
> 
> I will try and boot an sn2 machine first thing tomorrow morning.
> I don't recall dmidecode being broken, but that was quite a while ago.
> I thought the license validation code we used on sn2 used data from the
> dmidecode output.

 # uname -r
3.0.13-0.27-default
 # dmidecode
# dmidecode 2.9
/sys/firmware/efi/systab: SMBIOS entry point missing


It looks like it is broken there as well.  This is running a SLES11SP2
kernel with no local changes.

dmesg show:
DMI not present or invalid.

Thanks,
Robin

Re: memcpy_fromio in dmi_scan.c

[Robin Holt]

On Wed, Apr 24, 2013 at 09:22:08PM +0200, Jean Delvare wrote:
> Hi Tony,
> 
> Thanks for stepping in.
> 
> Le Tuesday 23 April 2013 à 22:00 +0000, Luck, Tony a écrit :
> > > I don't have much knowledge about IA64 either. All I see is that while
> > > x86 implements memcpy_fromio() with memcpy [1], ia64 implements it with
> > > readb [2]. There must be a reason for that, and I can only suppose that
> > > memcpy on __iomem pointers doesn't work on IA64. If memcpy doesn't work
> > > then I can't see memcmp working.
> > 
> > On most platforms readb() just ends up doing a regular dereference of the
> > address ... so I'd expect that memcmp would work just fine.  The exception
> > is the old SGI sn2 which end up calling ___sn_readb() ... which does something
> > weird with sn_dma_flush() after doing the dereference of *addr.  I don't

sn_dma_flush has a good description at the beginning of it.  It is only
related to activity on PCI attached devices and a memcmp should be fine.
It must be that our prom produces a bad DMI table.

Robin

Re: memcpy_fromio in dmi_scan.c

[Andrew Morton]

On Thu, 25 Apr 2013 04:52:42 -0500 Robin Holt <holt@sgi.com> wrote:

> On Wed, Apr 24, 2013 at 09:07:24PM -0500, Robin Holt wrote:
> > On Wed, Apr 24, 2013 at 08:16:50PM +0000, Luck, Tony wrote:
> > > > That being said, "my" SN2 machine was previously running kernel 3.0.34
> > > > which has the old dmi_scan code and it also said "DMI not present or
> > > > invalid." Plus dmidecode fails on this machine with:
> > > >
> > > > /sys/firmware/efi/systab: SMBIOS entry point missing
> > > >
> > > > So it might as well be that DMI support on SN2 was already broken.
> > > > Please let me know your findings on other SN2 machines.
> > > 
> > > I don't have an sn2 to test. Added Robin Holt to Cc list
> > 
> > I will try and boot an sn2 machine first thing tomorrow morning.
> > I don't recall dmidecode being broken, but that was quite a while ago.
> > I thought the license validation code we used on sn2 used data from the
> > dmidecode output.
> 
>  # uname -r
> 3.0.13-0.27-default
>  # dmidecode
> # dmidecode 2.9
> /sys/firmware/efi/systab: SMBIOS entry point missing
> 
> 
> It looks like it is broken there as well.  This is running a SLES11SP2
> kernel with no local changes.
> 
> dmesg show:
> DMI not present or invalid.
> 

So what's the fix?  This?

--- a/drivers/firmware/dmi_scan.c~a
+++ a/drivers/firmware/dmi_scan.c
@@ -500,9 +500,12 @@ void __init dmi_scan_machine(void)
 			goto error;
 
 		for (q = p; q < p + 0x10000; q += 16) {
-			if (memcmp(q, "_SM_", 4) == 0 && q - p <= 0xFFE0)
+			char buf[5];
+
+			memcpy_from_io(buf, q, 5);
+			if (memcmp(buf, "_SM_", 4) == 0 && q - p <= 0xFFE0)
 				rc = smbios_present(q);
-			else if (memcmp(q, "_DMI_", 5) == 0)
+			else if (memcmp(buf, "_DMI_", 5) == 0)
 				rc = dmi_present(q);
 			else
 				continue;
_

Re: memcpy_fromio in dmi_scan.c

[Jean Delvare]

Hi Andrew,

Sorry for the late reply.

Le Thursday 25 April 2013 à 13:37 -0700, Andrew Morton a écrit :
> So what's the fix?  This?
> 
> --- a/drivers/firmware/dmi_scan.c~a
> +++ a/drivers/firmware/dmi_scan.c
> @@ -500,9 +500,12 @@ void __init dmi_scan_machine(void)
>  			goto error;
>  
>  		for (q = p; q < p + 0x10000; q += 16) {
> -			if (memcmp(q, "_SM_", 4) == 0 && q - p <= 0xFFE0)
> +			char buf[5];
> +
> +			memcpy_from_io(buf, q, 5);
> +			if (memcmp(buf, "_SM_", 4) == 0 && q - p <= 0xFFE0)
>  				rc = smbios_present(q);
> -			else if (memcmp(q, "_DMI_", 5) == 0)
> +			else if (memcmp(buf, "_DMI_", 5) == 0)
>  				rc = dmi_present(q);
>  			else
>  				continue;

That would have been it, yes, but meanwhile Ben Hutchings reworked the
whole thing and that fixed the problem I reported as a side effect:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/firmware/dmi_scan.c?id=79bae42d51a5d498500c890c19ef76df41d2bf59

I'd suggest leaving the stable kernels alone until someone complains, as
there doesn't seem to be any issue in practice (the only machines where
it would possibly matter are already hopelessly broken.)

All I can say is that these dmi_scan patches should never have made it
to stable kernels in the first place, as they infringed several rules
stable patches are supposed to comply with.

-- 
Jean Delvare
Suse L3