Add and use a generic copy_oldmem_page()

Add and use a generic copy_oldmem_page()

[Palmer Dabbelt]

While adding support for kexec, Nick recently copied the arm64
copy_oldmem_page() into the RISC-V port.  Since this is shared verbatim with
arm and arm64 already, I'd like to add a generic version and so we can use it
instead.  I haven't converted over the MIPS, PPC, or SH ports: while I think we
could figure out how to share a version, they're not exactly the same right
now.  S/390 and x86 are definitely meaningfully different.

Unless there are any objections I'll include the first patch along with the
RISC-V kexec support, which I'm hoping to have for 5.9.  The code, based on
5.8-rc4, is at
ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/palmer/linux.git -b
copy_oldmem_page .

[PATCH 1/3] lib: Add a generic copy_oldmem_page()

[Palmer Dabbelt]

From: Palmer Dabbelt <palmerdabbelt@google.com>

A version of this that is identical to the arm64 version was recently
copied into the RISC-V port while adding kexec() support.  Instead I'd
like to put a shared copy in lib/ and use it from the various ports.

Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 lib/Kconfig            |  3 +++
 lib/Makefile           |  2 ++
 lib/copy_oldmem_page.c | 51 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)
 create mode 100644 lib/copy_oldmem_page.c

diff --git a/lib/Kconfig b/lib/Kconfig
index df3f3da95990..25544abc9547 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -676,3 +676,6 @@ config GENERIC_LIB_CMPDI2
 
 config GENERIC_LIB_UCMPDI2
 	bool
+
+config GENERIC_LIB_COPY_OLDMEM_PAGE
+	bool
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..30d57d8b32b1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -318,3 +318,5 @@ obj-$(CONFIG_OBJAGG) += objagg.o
 # KUnit tests
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+
+obj-$(CONFIG_GENERIC_LIB_COPY_OLDMEM_PAGE) += copy_oldmem_page.o
diff --git a/lib/copy_oldmem_page.c b/lib/copy_oldmem_page.c
new file mode 100644
index 000000000000..f0090027218a
--- /dev/null
+++ b/lib/copy_oldmem_page.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Routines for doing kexec-based kdump
+ *
+ * Originally part of arch/arm64/kernel/crash_dump.c
+ * Copyright (C) 2017 Linaro Limited
+ * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ */
+
+#include <linux/io.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+/**
+ * copy_oldmem_page() - copy one page from old kernel memory
+ * @pfn: page frame number to be copied
+ * @buf: buffer where the copied page is placed
+ * @csize: number of bytes to copy
+ * @offset: offset in bytes into the page
+ * @userbuf: if set, @buf is in a user address space
+ *
+ * This function copies one page from old kernel memory into buffer pointed by
+ * @buf. If @buf is in userspace, set @userbuf to %1. Returns number of bytes
+ * copied or negative error in case of failure.
+ */
+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 = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
+	if (!vaddr)
+		return -ENOMEM;
+
+	if (userbuf) {
+		if (copy_to_user((char __user *)buf, vaddr + offset, csize)) {
+			memunmap(vaddr);
+			return -EFAULT;
+		}
+	} else {
+		memcpy(buf, vaddr + offset, csize);
+	}
+
+	memunmap(vaddr);
+
+	return csize;
+}
-- 
2.27.0.383.g050319c2ae-goog

[PATCH 2/3] arm: Use the new generic copy_oldmem_page()

[Palmer Dabbelt]

From: Palmer Dabbelt <palmerdabbelt@google.com>

This is exactly the same as the arm64 code, which I just into lib/ to
avoid copying it into the RISC-V port.  This builds with defconfig and with
CRASH_DUMP=y.

Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/arm/Kconfig             |  1 +
 arch/arm/kernel/Makefile     |  1 -
 arch/arm/kernel/crash_dump.c | 54 ------------------------------------
 3 files changed, 1 insertion(+), 55 deletions(-)
 delete mode 100644 arch/arm/kernel/crash_dump.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 2ac74904a3ce..dfbeb14e9673 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1933,6 +1933,7 @@ config ATAGS_PROC
 
 config CRASH_DUMP
 	bool "Build kdump crash kernel (EXPERIMENTAL)"
+	select GENERIC_LIB_COPY_OLDMEM_PAGE
 	help
 	  Generate crash dump after being started by kexec. This should
 	  be normally only set in special crash dump kernels which are
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 89e5d864e923..b5310a90dfe4 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -65,7 +65,6 @@ obj-$(CONFIG_KGDB)		+= kgdb.o patch.o
 obj-$(CONFIG_ARM_UNWIND)	+= unwind.o
 obj-$(CONFIG_HAVE_TCM)		+= tcm.o
 obj-$(CONFIG_OF)		+= devtree.o
-obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
 obj-$(CONFIG_SWP_EMULATE)	+= swp_emulate.o
 CFLAGS_swp_emulate.o		:= -Wa,-march=armv7-a
 obj-$(CONFIG_HAVE_HW_BREAKPOINT)	+= hw_breakpoint.o
diff --git a/arch/arm/kernel/crash_dump.c b/arch/arm/kernel/crash_dump.c
deleted file mode 100644
index 53cb92435392..000000000000
--- a/arch/arm/kernel/crash_dump.c
+++ /dev/null
@@ -1,54 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * arch/arm/kernel/crash_dump.c
- *
- * Copyright (C) 2010 Nokia Corporation.
- * Author: Mika Westerberg
- *
- * This code is taken from arch/x86/kernel/crash_dump_64.c
- *   Created by: Hariprasad Nellitheertha (hari@in.ibm.com)
- *   Copyright (C) IBM Corporation, 2004. All rights reserved
- */
-
-#include <linux/errno.h>
-#include <linux/crash_dump.h>
-#include <linux/uaccess.h>
-#include <linux/io.h>
-
-/**
- * copy_oldmem_page() - copy one page from old kernel memory
- * @pfn: page frame number to be copied
- * @buf: buffer where the copied page is placed
- * @csize: number of bytes to copy
- * @offset: offset in bytes into the page
- * @userbuf: if set, @buf is int he user address space
- *
- * This function copies one page from old kernel memory into buffer pointed by
- * @buf. If @buf is in userspace, set @userbuf to %1. Returns number of bytes
- * copied or negative error in case of failure.
- */
-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 = ioremap(__pfn_to_phys(pfn), PAGE_SIZE);
-	if (!vaddr)
-		return -ENOMEM;
-
-	if (userbuf) {
-		if (copy_to_user(buf, vaddr + offset, csize)) {
-			iounmap(vaddr);
-			return -EFAULT;
-		}
-	} else {
-		memcpy(buf, vaddr + offset, csize);
-	}
-
-	iounmap(vaddr);
-	return csize;
-}
-- 
2.27.0.383.g050319c2ae-goog

[PATCH 3/3] arm64: Use the new generic copy_oldmem_page()

[Palmer Dabbelt]

From: Palmer Dabbelt <palmerdabbelt@google.com>

This is exactly the same as the arm64 code, which I just into lib/ to
avoid copying it into the RISC-V port.  This builds with defconfig.

Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/arm64/Kconfig             |  1 +
 arch/arm64/kernel/crash_dump.c | 39 ----------------------------------
 2 files changed, 1 insertion(+), 39 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 66dc41fd49f2..55b27d56b163 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1109,6 +1109,7 @@ comment "Support for PE file signature verification disabled"
 
 config CRASH_DUMP
 	bool "Build kdump crash kernel"
+	select GENERIC_LIB_COPY_OLDMEM_PAGE
 	help
 	  Generate crash dump after being started by kexec. This should
 	  be normally only set in special crash dump kernels which are
diff --git a/arch/arm64/kernel/crash_dump.c b/arch/arm64/kernel/crash_dump.c
index e6e284265f19..197b92c249ba 100644
--- a/arch/arm64/kernel/crash_dump.c
+++ b/arch/arm64/kernel/crash_dump.c
@@ -13,45 +13,6 @@
 #include <linux/uaccess.h>
 #include <asm/memory.h>
 
-/**
- * copy_oldmem_page() - copy one page from old kernel memory
- * @pfn: page frame number to be copied
- * @buf: buffer where the copied page is placed
- * @csize: number of bytes to copy
- * @offset: offset in bytes into the page
- * @userbuf: if set, @buf is in a user address space
- *
- * This function copies one page from old kernel memory into buffer pointed by
- * @buf. If @buf is in userspace, set @userbuf to %1. Returns number of bytes
- * copied or negative error in case of failure.
- */
-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 = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
-	if (!vaddr)
-		return -ENOMEM;
-
-	if (userbuf) {
-		if (copy_to_user((char __user *)buf, vaddr + offset, csize)) {
-			memunmap(vaddr);
-			return -EFAULT;
-		}
-	} else {
-		memcpy(buf, vaddr + offset, csize);
-	}
-
-	memunmap(vaddr);
-
-	return csize;
-}
-
 /**
  * elfcorehdr_read - read from ELF core header
  * @buf: buffer where the data is placed
-- 
2.27.0.383.g050319c2ae-goog

Re: [PATCH 1/3] lib: Add a generic copy_oldmem_page()

[Christoph Hellwig]

On Fri, Jul 10, 2020 at 08:55:42PM -0700, Palmer Dabbelt 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 = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
> +	if (!vaddr)
> +		return -ENOMEM;

Doing a memremap for every page is very inefficient.  Also I don't see
why you'd want to even do that.  All memory is in the direct mapping
for RISC-V.  For other architecture that support highmem kmap_atomic_pfn
would do the job, which is what I'd use in a generic version.

Re: [PATCH 1/3] lib: Add a generic copy_oldmem_page()

[Arnd Bergmann]

On Mon, Jul 13, 2020 at 3:07 PM Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Jul 10, 2020 at 08:55:42PM -0700, Palmer Dabbelt 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 = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
> > +     if (!vaddr)
> > +             return -ENOMEM;
>
> Doing a memremap for every page is very inefficient.  Also I don't see
> why you'd want to even do that.  All memory is in the direct mapping
> for RISC-V.  For other architecture that support highmem kmap_atomic_pfn
> would do the job, which is what I'd use in a generic version.

I would expect the 'oldmem' data to not have a 'struct page', which would
be a problem at least for the generic implementation of kmap_atomic_pfn()

include/linux/highmem.h:#define kmap_atomic_pfn(pfn)
kmap_atomic(pfn_to_page(pfn))

kmap_atomic() might still work with a bogus page pointer if it
only transforms it back into a pfn, but some implementations just
have this one that cannot work:
static inline void *page_address(const struct page *page)
{
        return page->virtual;
}

I have not checked how the crash dump code works though, maybe
it does work after all.

       Arnd

Re: [PATCH 1/3] lib: Add a generic copy_oldmem_page()

[Christoph Hellwig]

On Mon, Jul 13, 2020 at 03:39:17PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 13, 2020 at 3:07 PM Christoph Hellwig <hch@infradead.org> wrote:
> > On Fri, Jul 10, 2020 at 08:55:42PM -0700, Palmer Dabbelt 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 = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
> > > +     if (!vaddr)
> > > +             return -ENOMEM;
> >
> > Doing a memremap for every page is very inefficient.  Also I don't see
> > why you'd want to even do that.  All memory is in the direct mapping
> > for RISC-V.  For other architecture that support highmem kmap_atomic_pfn
> > would do the job, which is what I'd use in a generic version.
> 
> I would expect the 'oldmem' data to not have a 'struct page', which would
> be a problem at least for the generic implementation of kmap_atomic_pfn()

Why do you expect it to not have a struct page?

Re: [PATCH 1/3] lib: Add a generic copy_oldmem_page()

[Arnd Bergmann]

On Tue, Jul 14, 2020 at 1:06 PM Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jul 13, 2020 at 03:39:17PM +0200, Arnd Bergmann wrote:
> > On Mon, Jul 13, 2020 at 3:07 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > On Fri, Jul 10, 2020 at 08:55:42PM -0700, Palmer Dabbelt 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 = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
> > > > +     if (!vaddr)
> > > > +             return -ENOMEM;
> > >
> > > Doing a memremap for every page is very inefficient.  Also I don't see
> > > why you'd want to even do that.  All memory is in the direct mapping
> > > for RISC-V.  For other architecture that support highmem kmap_atomic_pfn
> > > would do the job, which is what I'd use in a generic version.
> >
> > I would expect the 'oldmem' data to not have a 'struct page', which would
> > be a problem at least for the generic implementation of kmap_atomic_pfn()
>
> Why do you expect it to not have a struct page?

I was under the impression that the kdump kernel only accesses a small
amount of memory itself and would allocate its mem_map to fit that but
not pages of that were used by the kernel it is dumping.

       Arnd

Re: [PATCH 3/3] arm64: Use the new generic copy_oldmem_page()

[Catalin Marinas]

On Fri, Jul 10, 2020 at 08:55:44PM -0700, Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmerdabbelt@google.com>
> 
> This is exactly the same as the arm64 code, which I just into lib/ to
> avoid copying it into the RISC-V port.  This builds with defconfig.
> 
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 1/3] lib: Add a generic copy_oldmem_page()

[Ard Biesheuvel]

On Mon, 13 Jul 2020 at 16:06, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jul 10, 2020 at 08:55:42PM -0700, Palmer Dabbelt 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 = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
> > +     if (!vaddr)
> > +             return -ENOMEM;
>
> Doing a memremap for every page is very inefficient.

memremap(MEMREMAP_WB) will reuse the existing linear address if the PA
is covered by the linear mapping.

> Also I don't see
> why you'd want to even do that.  All memory is in the direct mapping
> for RISC-V.  For other architecture that support highmem kmap_atomic_pfn
> would do the job, which is what I'd use in a generic version.

Re: [PATCH 2/3] arm: Use the new generic copy_oldmem_page()

[Linus Walleij]

On Sat, Jul 11, 2020 at 5:59 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:

> From: Palmer Dabbelt <palmerdabbelt@google.com>
>
> This is exactly the same as the arm64 code, which I just into lib/ to
> avoid copying it into the RISC-V port.  This builds with defconfig and with
> CRASH_DUMP=y.
>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>

Looks good to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij