Hi
While using dd command to retrive the dump from /dev/oldmem, there comes
a rare case where pfn value is zero. In this case the __ioremap() call
returns
NULL and hence copying fails.
# dd if=/dev/oldmem of=/dev/null
dd: reading `/dev/oldmem': Bad address
0+0 records in
0+0 records out
0 bytes (0 B) copied, 0.000121 seconds, 0.0 kB/s
Attached is a patch to fix this problem. During such rare cases don't call
__ioremap() to do the address translation, instead use __va() .
Tested with 2.6.18.
Thanks
-Sachin
On Thu, 2006-09-21 at 10:07 +0530, Sachin P. Sant wrote:
Hi
While using dd command to retrive the dump from /dev/oldmem, there comes
a rare case where pfn value is zero. In this case the __ioremap() call
returns
NULL and hence copying fails.
# dd if=/dev/oldmem of=/dev/null
dd: reading `/dev/oldmem': Bad address
0+0 records in
0+0 records out
0 bytes (0 B) copied, 0.000121 seconds, 0.0 kB/s
Attached is a patch to fix this problem. During such rare cases don't call
__ioremap() to do the address translation, instead use __va() .
It's not really rare, it's just when we're reading /dev/oldmem directly.
We can actually use the __va() trick for the whole linear mapping rather
than just pfn 0, which saves the ioremap. We also shouldn't really be
trying to iounmap(__va(0)).
So perhaps something more like this? Although it's a bit ugly because of
the need to conditionally call iounmap().
Paul you said we shouldn't be using ioremap(), but I really can't find
an alternative - map_vm_area() looks close but it requires struct pages
which we don't have. And I think your main objection was a cacheable
mapping, which we're not doing anyway by calling __ioremap().
...
Fix /dev/oldmem for kdump
A change to __ioremap() broke reading /dev/oldmem because we're no
longer able to ioremap pfn 0 (d177c207ba16b1db31283e2d1fee7ad4a863584b).
We actually don't need to ioremap for anything that's part of the linear
mapping, so just read it directly.
Also make sure we're only reading one page or less at a time.
Index: to-merge/arch/powerpc/kernel/crash_dump.c
===================================================================
--- to-merge.orig/arch/powerpc/kernel/crash_dump.c
+++ to-merge/arch/powerpc/kernel/crash_dump.c
@@ -80,6 +80,20 @@ static int __init parse_savemaxmem(char
}
__setup("savemaxmem=", parse_savemaxmem);
+
+static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
+ unsigned long offset, int userbuf)
+{
+ if (userbuf) {
+ if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
+ return -EFAULT;
+ }
+ } else
+ memcpy(buf, (vaddr + offset), csize);
+
+ return csize;
+}
+
/**
* copy_oldmem_page - copy one page from "oldmem"
* @pfn: page frame number to be copied@@ -101,16 +115,16 @@ ssize_t copy_oldmem_page(unsigned long p
if (!csize)
return 0;
- vaddr = __ioremap(pfn << PAGE_SHIFT, PAGE_SIZE, 0);
+ csize = min(csize, PAGE_SIZE);
- if (userbuf) {
- if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
- iounmap(vaddr);
- return -EFAULT;
- }
- } else
- memcpy(buf, (vaddr + offset), csize);
+ if (pfn < max_pfn) {
+ vaddr = __va(pfn << PAGE_SHIFT);
+ csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
+ } else {
+ vaddr = __ioremap(pfn << PAGE_SHIFT, PAGE_SIZE, 0);
+ csize = copy_oldmem_vaddr(vaddr, buf, csize, offset, userbuf);
+ iounmap(vaddr);
+ }
- iounmap(vaddr);
return csize;
}On Thu, Sep 21, 2006 at 09:02:37PM +1000, Michael Ellerman wrote:
On Thu, 2006-09-21 at 10:07 +0530, Sachin P. Sant wrote:
quoted
Hi
While using dd command to retrive the dump from /dev/oldmem, there comes
a rare case where pfn value is zero. In this case the __ioremap() call
returns
NULL and hence copying fails.
# dd if=/dev/oldmem of=/dev/null
dd: reading `/dev/oldmem': Bad address
0+0 records in
0+0 records out
0 bytes (0 B) copied, 0.000121 seconds, 0.0 kB/s
Attached is a patch to fix this problem. During such rare cases don't call
__ioremap() to do the address translation, instead use __va() .
It's not really rare, it's just when we're reading /dev/oldmem directly.
We can actually use the __va() trick for the whole linear mapping rather
than just pfn 0, which saves the ioremap. We also shouldn't really be
trying to iounmap(__va(0)).
Makes sense. We can take advantage of linear mappings mapped till max_pfn
and avoid ioremap().
+
+static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
+ unsigned long offset, int userbuf)
+{
+ if (userbuf) {
+ if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
+ return -EFAULT;
+ }
Probably you can get rid of above pair of braces as there is only single
statement under if.
- if (userbuf) {
- if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
- iounmap(vaddr);
- return -EFAULT;
- }
- } else
- memcpy(buf, (vaddr + offset), csize);
+ if (pfn < max_pfn) {
Should this be (pfn <= max_pfn) ?
-Vivek
Michael Ellerman wrote:
quoted
Attached is a patch to fix this problem. During such rare cases don't call
__ioremap() to do the address translation, instead use __va() .
It's not really rare, it's just when we're reading /dev/oldmem directly.
That's true. Since we don't try to copy raw dump from /dev/oldmem very
often, we haven't come across this problem. Hence rare .. but as
michael said always recreatable while using dd command with /dev/oldmem.
We can actually use the __va() trick for the whole linear mapping rather
than just pfn 0, which saves the ioremap. We also shouldn't really be
trying to iounmap(__va(0)).
Yes. Makes sense. Agreed.
So perhaps something more like this? Although it's a bit ugly because of
the need to conditionally call iounmap().
< snip >
Thanks
-Sachin
On Thu, 2006-09-21 at 10:10 -0400, Vivek Goyal wrote:
On Thu, Sep 21, 2006 at 09:02:37PM +1000, Michael Ellerman wrote:
quoted
+
+static size_t copy_oldmem_vaddr(void *vaddr, char *buf, size_t csize,
+ unsigned long offset, int userbuf)
+{
+ if (userbuf) {
+ if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
+ return -EFAULT;
+ }
Probably you can get rid of above pair of braces as there is only single
statement under if.
Yep.
quoted
- if (userbuf) {
- if (copy_to_user((char __user *)buf, (vaddr + offset), csize)) {
- iounmap(vaddr);
- return -EFAULT;
- }
- } else
- memcpy(buf, (vaddr + offset), csize);
+ if (pfn < max_pfn) {
Should this be (pfn <= max_pfn) ?
No, max_pfn is badly named. It seems to actually be the total number of
pages == the first pfn past the end of the linear mapping.
But it'd be cleaner to use page_is_ram(), I'll do a new patch.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person