Hi Duan, Andrew,
I am looking at the following commit:
commit 9f9c9cbb60576a1518d0bf93fb8e499cffccf377
Author: Zhenzhong Duan <[email protected]>
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
Jean Delvare wrote:
> Hi Duan, Andrew,
>
> I am looking at the following commit:
>
> commit 9f9c9cbb60576a1518d0bf93fb8e499cffccf377
> Author: Zhenzhong Duan <[email protected]>
> 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
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 <[email protected]>
> > 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
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
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
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
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
On Thu, 25 Apr 2013 04:52:42 -0500 Robin Holt <[email protected]> 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;
_
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