by Vivek Goyal

[permalink] [raw]

Subject: Re: [PATCH 8/10] kdump: x86_64 save cpu registers upon crash

On Fri, Nov 18, 2005 at 02:52:33PM -0700, Eric W. Biederman wrote:
>
> > diff -puN arch/x86_64/kernel/crash.c~x86_64-save-cpu-registers-upon-crash
> > arch/x86_64/kernel/crash.c
> > ---
> > linux-2.6.15-rc1-1M-dynamic/arch/x86_64/kernel/crash.c~x86_64-save-cpu-registers-upon-crash
>
> > #ifdef CONFIG_SMP
> > static atomic_t waiting_for_crash_ipi;
> >
> > @@ -38,6 +106,7 @@ static int crash_nmi_callback(struct pt_
> > return 1;
> > local_irq_disable();
> >
> > + crash_save_this_cpu(regs, cpu);
> > disable_local_APIC();
> > atomic_dec(&waiting_for_crash_ipi);
> > /* Assume hlt works */
> > @@ -113,4 +182,5 @@ void machine_crash_shutdown(struct pt_re
> > disable_IO_APIC();
> > #endif
> >
> > + crash_save_self(regs);
> > }
>
> Where did this disable_local_APIC and disable_IO_APIC on x86_64 come
> from? I know we had it on x86 but that was supposed to be a stop gap
> until we have the real fix. Now I know it needs a little more
> debugging but the real fix has been written. Putting it there on
> x86_64 makes the code less reliable and it allows things to start
> depending on it.
>

Hi Eric,

Initially we had written the patch without disable_local_APIC and
disable_IO_APIC only. But realized later that fix provided by you to
move apic initialization in init_IRQ has not been merged yet.

Like i386, here also this is a stop gap solution only till your patch is
merged. After that we shall drop this code.

I understand that it is less reliable but at least it provides us a base
to move forward and test kdump on x86_64 and address the other issues.

Thanks
Vivek

2005-11-23 14:01:12

by Rachita Kothiyal

[permalink] [raw]

Subject: Re: [Fastboot] Re: [PATCH 9/10] kdump: read previous kernel's memory

On Thu, Nov 17, 2005 at 02:20:23PM -0800, Andrew Morton wrote:
> Vivek Goyal <[email protected]> wrote:
> >
> > +ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
> > + size_t csize, unsigned long offset, int userbuf)
> > +{
> > + void *vaddr;
> > +
> > + if (!csize)
> > + return 0;
> > +
> > + vaddr = kmap_atomic_pfn(pfn, KM_PTE0);
> > +
> > + if (userbuf) {
> > + if (copy_to_user(buf, (vaddr + offset), csize)) {
> > + kunmap_atomic(vaddr, KM_PTE0);
> > + return -EFAULT;
>
> The copy_*_user() inside kmap_atomic() is problematic.
>
> On some configs (eg, x86, highmem) the process is running atomically, hence
> the copy_*_user() will *refuse* to fault in the user's page if it's not
> present. Because pagefaulting involves doing things which sleep.
>
> So
>
> a) This code will generate might_sleep() warnings at runtime and
>
> b) It'll return -EFAULT for user pages which haven't been faulted in yet.
>
>
> We do all sorts of gruesome tricks in mm/filemap.c to get around all this.
> I don't think your code is as performance-sensitive, so a suitable fix
> might be to double-copy the data. Make sure that the same physical page is
> used as a bounce page for each copy (ie: get the caller to pass it in) and
> that page will be cache-hot and the performance should be acceptable.
>
> If it really is performance-sensitive then you'll need to play filemap.c
> games. It'd be better to use a sleeping kmap instead, if poss. That's
> kmap().
>
> Please send an incremental patch when it's sorted.

Hi Andrew

Sending along the incremental patch as suggested.

In this patch, a global buffer page is introduced, where the page
from the previous kernel's memory is copied, before copying it
out to a user buffer. This will take care of the issue of
copy_to_user() page faulting in an atomic context.

This patch has been generated against 2.6.15-rc2-mm1.
Kindly review.

Thanks
Rachita

o This patch allocates a page to copy the previous kernel's memory
before we copy it onto a user buffer using copy_to_user(), thereby
taking care of the scenario of a possible page fault in an atomic
context.

Signed-off-by: Rachita Kothiyal <[email protected]>
---

arch/i386/kernel/crash_dump.c | 39 +++++++++++++++++++++++++++++++++------
1 files changed, 33 insertions(+), 6 deletions(-)

diff -puN arch/i386/kernel/crash_dump.c~double_copy_read_oldmem arch/i386/kernel/crash_dump.c
--- linux-2.6.15-rc2-mm1/arch/i386/kernel/crash_dump.c~double_copy_read_oldmem 2005-11-23 17:50:07.258543864 +0530
+++ linux-2.6.15-rc2-mm1-rachita/arch/i386/kernel/crash_dump.c 2005-11-23 17:50:36.779056064 +0530
@@ -11,6 +11,8 @@

#include <asm/uaccess.h>

+static void *kdump_buf_page;
+
/**
* copy_oldmem_page - copy one page from "oldmem"
* @pfn: page frame number to be copied
@@ -23,6 +25,10 @@
*
* Copy a page from "oldmem". For this page, there is no pte mapped
* in the current kernel. We stitch up a pte, similar to kmap_atomic.
+ *
+ * Calling copy_to_user() in atomic context is not desirable. Hence first
+ * copying the data to a pre-allocated kernel page and then copying to user
+ * space in non-atomic context.
*/
ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
size_t csize, unsigned long offset, int userbuf)
@@ -34,14 +40,35 @@ ssize_t copy_oldmem_page(unsigned long p

vaddr = kmap_atomic_pfn(pfn, KM_PTE0);

- if (userbuf) {
- if (copy_to_user(buf, (vaddr + offset), csize)) {
- kunmap_atomic(vaddr, KM_PTE0);
+ if (!userbuf) {
+ memcpy(buf, (vaddr + offset), csize);
+ kunmap_atomic(vaddr, KM_PTE0);
+ } else {
+ if (!kdump_buf_page) {
+ printk(KERN_WARNING "Kdump: Kdump buffer page not"
+ " allocated\n");
return -EFAULT;
}
- } else
- memcpy(buf, (vaddr + offset), csize);
+ copy_page(kdump_buf_page, vaddr);
+ kunmap_atomic(vaddr, KM_PTE0);
+ if (copy_to_user(buf, (kdump_buf_page + offset), csize))
+ return -EFAULT;
+ }

- kunmap_atomic(vaddr, KM_PTE0);
return csize;
}
+
+static int __init kdump_buf_page_init(void)
+{
+ int ret = 0;
+
+ kdump_buf_page = kmalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!kdump_buf_page) {
+ printk(KERN_WARNING "Kdump: Failed to allocate kdump buffer"
+ " page\n");
+ ret = -ENOMEM;
+ }
+
+ return ret;
+}
+arch_initcall(kdump_buf_page_init);
_