BISECTED x86: avoid qword access in memcpy_*io

PATCH In review Hidetoshi Seto 2 participants 5 messages Jul 21, 2010 – Jul 22, 2010

Subsystems (2)

x86 SCSI

lore

Share & tools

lore

Hidetoshi Seto

author

2010-07-21 01:21 UTC #0 lore raw

PATCH 1/1 BISECTED x86: avoid qword access in memcpy_*io

With v2.6.35-rc5, my x86-64 server doesn't boot but reports a Completer Abort on lpfc card.

The result of git-bisect is:
6175ddf06b6172046a329e3abfd9c901a43efd2e is the first bad commit commit 6175ddf06b6172046a329e3abfd9c901a43efd2e
Author: Brian Gerst <[email protected]>
Date: Fri Feb 5 09:37:07 2010 -0500
x86: Clean up mem*io functions.

What I found are:
- memcpy for 64bit uses movq if count >= 64 (arch/x86/lib/memcpy_64.S)
- memcpy_toio and memcpy_fromio have changed to use this memcpy by the above commit.
- my debug shows that lpfc calls memcpy_toio with not-qword-aligned addresses and count >= 64, e.g.:

     memcpy_toio(0xffffc900118de004, 0xffff88047293d614, 124);
   and it seems that it comes from:
   [drivers/scsi/lpfc/lpfc_sli.c]
    4929   /* First copy mbox command data to HBA SLIM, skip past first
    4930      word */
    4931   to_slim = phba->MBslimaddr + sizeof (uint32_t);
    4932   lpfc_memcpy_to_slim(to_slim, &mb->un.varWords[0],
    4933               MAILBOX_CMD_SIZE - sizeof (uint32_t));

Still I'm not sure what is wrong in software or hardware, however I suppose that qword access to iomem is not always safe, so it will be OK to back to use __inline_memcpy which uses movsl.

I confirmed that my server (w/ lpfc) boots with 35-rc5 + this patch.

Signed-off-by Hidetoshi Seto <[email protected]>

---
 arch/x86/include/asm/io.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

arch/x86/include/asm/io.h +8 -0

index 30a3e97..e15a74a 100644
@@ -227,13 +227,21 @@ memset_io(volatile void __iomem *addr, unsigned char val, size_t count)
 static inline void
 memcpy_fromio(void *dst, const volatile void __iomem *src, size_t count)
 {
+#ifdef CONFIG_X86_64
+	__inline_memcpy(dst, (const void __force *)src, count);
+#else
 	memcpy(dst, (const void __force *)src, count);
+#endif
 }
 
 static inline void
 memcpy_toio(volatile void __iomem *dst, const void *src, size_t count)
 {
+#ifdef CONFIG_X86_64
+	__inline_memcpy((void __force *)dst, src, count);
+#else
 	memcpy((void __force *)dst, src, count);
+#endif
 }
 
 /*

Signature · 1 line

1.7.1.1

H. Peter Anvin

reviewer

2010-07-21 02:48 UTC #1 lore raw

Quoted 25 lines · Hidetoshi Seto

With v2.6.35-rc5, my x86-64 server doesn't boot but reports a Completer Abort on lpfc card.
The result of git-bisect is:
6175ddf06b6172046a329e3abfd9c901a43efd2e is the first bad commit commit 6175ddf06b6172046a329e3abfd9c901a43efd2e
Author: Brian Gerst <[email protected]>
Date: Fri Feb 5 09:37:07 2010 -0500
x86: Clean up mem*io functions.
What I found are:
- memcpy for 64bit uses movq if count >= 64 (arch/x86/lib/memcpy_64.S)
- memcpy_toio and memcpy_fromio have changed to use this memcpy by the above commit.
- my debug shows that lpfc calls memcpy_toio with not-qword-aligned addresses and count >= 64, e.g.:
     memcpy_toio(0xffffc900118de004, 0xffff88047293d614, 124);
   and it seems that it comes from:
   [drivers/scsi/lpfc/lpfc_sli.c]
    4929   /* First copy mbox command data to HBA SLIM, skip past first
    4930      word */
    4931   to_slim = phba->MBslimaddr + sizeof (uint32_t);
    4932   lpfc_memcpy_to_slim(to_slim, &mb->un.varWords[0],
    4933               MAILBOX_CMD_SIZE - sizeof (uint32_t));

Quoted 7 lines

Still I'm not sure what is wrong in software or hardware, however I suppose that qword access to iomem is not always safe, so it will be OK to back to use __inline_memcpy which uses movsl.
I confirmed that my server (w/ lpfc) boots with 35-rc5 + this patch.
Signed-off-by Hidetoshi Seto <[email protected]>

A driver should not use the memcpy-like instructions if it isn't set up to act as memory (meaning it can handle arbitrary byte enables.)

The function it should be using is called, fairly counterintuitively, __iowrite32_copy(). It really should be called memcpy_toio32() or something similar.

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Hidetoshi Seto

author

2010-07-22 01:24 UTC #2 lore raw

Quoted 30 lines · H. Peter Anvin

Quoted 29 lines · Hidetoshi Seto
With v2.6.35-rc5, my x86-64 server doesn't boot but reports a Completer Abort on lpfc card.
The result of git-bisect is:
6175ddf06b6172046a329e3abfd9c901a43efd2e is the first bad commit commit 6175ddf06b6172046a329e3abfd9c901a43efd2e
Author: Brian Gerst <[email protected]>
Date: Fri Feb 5 09:37:07 2010 -0500
x86: Clean up mem*io functions.
What I found are:
- memcpy for 64bit uses movq if count >= 64 (arch/x86/lib/memcpy_64.S)
- memcpy_toio and memcpy_fromio have changed to use this memcpy by the above commit.
- my debug shows that lpfc calls memcpy_toio with not-qword-aligned addresses and count >= 64, e.g.:
     memcpy_toio(0xffffc900118de004, 0xffff88047293d614, 124);
   and it seems that it comes from:
   [drivers/scsi/lpfc/lpfc_sli.c]
    4929   /* First copy mbox command data to HBA SLIM, skip past first
    4930      word */
    4931   to_slim = phba->MBslimaddr + sizeof (uint32_t);
    4932   lpfc_memcpy_to_slim(to_slim, &mb->un.varWords[0],
    4933               MAILBOX_CMD_SIZE - sizeof (uint32_t));
Still I'm not sure what is wrong in software or hardware, however I suppose that qword access to iomem is not always safe, so it will be OK to back to use __inline_memcpy which uses movsl.

Quoted 6 lines

Quoted 3 lines
I confirmed that my server (w/ lpfc) boots with 35-rc5 + this patch.
Signed-off-by Hidetoshi Seto <[email protected]>
A driver should not use the memcpy-like instructions if it isn't set up to act as memory (meaning it can handle arbitrary byte enables.)

So then is this a problem of lpfc driver?
James, could you agree on that?

Quoted 5 lines

The function it should be using is called, fairly counterintuitively, __iowrite32_copy(). It really should be called memcpy_toio32() or something similar.
-hpa

It seems that lpfc already implemented lpfc_memcpy_{to,from}_slim() as such memcpy_*io32, but limited use of it to on big endian platforms only. Now lpfc can move to use it always, right?

Thanks,
H.Seto

H. Peter Anvin

reviewer

2010-07-22 02:34 UTC #3 lore raw

Yes, that would make sense.

Quoted 55 lines · Hidetoshi Seto

Quoted 30 lines · H. Peter Anvin
Quoted 29 lines · Hidetoshi Seto
With v2.6.35-rc5, my x86-64 server doesn't boot but reports a Completer Abort on lpfc card.
The result of git-bisect is:
6175ddf06b6172046a329e3abfd9c901a43efd2e is the first bad commit commit 6175ddf06b6172046a329e3abfd9c901a43efd2e
Author: Brian Gerst <[email protected]>
Date: Fri Feb 5 09:37:07 2010 -0500
x86: Clean up mem*io functions.
What I found are:
- memcpy for 64bit uses movq if count >= 64 (arch/x86/lib/memcpy_64.S)
- memcpy_toio and memcpy_fromio have changed to use this memcpy by the above commit.
- my debug shows that lpfc calls memcpy_toio with not-qword-aligned addresses and count >= 64, e.g.:
     memcpy_toio(0xffffc900118de004, 0xffff88047293d614, 124);
   and it seems that it comes from:
   [drivers/scsi/lpfc/lpfc_sli.c]
    4929   /* First copy mbox command data to HBA SLIM, skip past first
    4930      word */
    4931   to_slim = phba->MBslimaddr + sizeof (uint32_t);
    4932   lpfc_memcpy_to_slim(to_slim, &mb->un.varWords[0],
    4933               MAILBOX_CMD_SIZE - sizeof (uint32_t));
Still I'm not sure what is wrong in software or hardware, however I suppose that qword access to iomem is not always safe, so it will be OK to back to use __inline_memcpy which uses movsl.
Quoted 6 lines
Quoted 3 lines
I confirmed that my server (w/ lpfc) boots with 35-rc5 + this patch.
Signed-off-by Hidetoshi Seto <[email protected]>
A driver should not use the memcpy-like instructions if it isn't set up to act as memory (meaning it can handle arbitrary byte enables.)
So then is this a problem of lpfc driver?
James, could you agree on that?
Quoted 5 lines
The function it should be using is called, fairly counterintuitively, __iowrite32_copy(). It really should be called memcpy_toio32() or something similar.
-hpa
It seems that lpfc already implemented lpfc_memcpy_{to,from}_slim() as such memcpy_*io32, but limited use of it to on big endian platforms only. Now lpfc can move to use it always, right?
Thanks,
H.Seto

Signature · 1 line

Sent from my mobile phone.  Please pardon any lack of formatting.

H. Peter Anvin

reviewer

2010-07-22 04:00 UTC #4 lore raw

In reply to Hidetoshi Seto

Quoted 5 lines · Hidetoshi Seto

It seems that lpfc already implemented lpfc_memcpy_{to,from}_slim() as such memcpy_*io32, but limited use of it to on big endian platforms only. Now lpfc can move to use it always, right?

What it probably should do is instead of open-coding this stuff use __iowrite32_copy and we probably should have a version without double underscores which byteswaps on bigendian.

Personally I would not object to seeing a patch renaming __iowrite32_copy() to memcpy_toio32()... especially since it doesn't have the "memory or I/O space" property of iowrite and friends.

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.