* [PATCH v15 01/16] asm-generic: add kexec_file_load system call to unistd.h
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 02/16] kexec_file: make kexec_image_post_load_cleanup_default() global AKASHI Takahiro
` (14 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
The initial user of this system call number is arm64.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
include/uapi/asm-generic/unistd.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index df4bedb9b01c..acea91e49523 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -736,9 +736,11 @@ __SYSCALL(__NR_statx, sys_statx)
__SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
#define __NR_rseq 293
__SYSCALL(__NR_rseq, sys_rseq)
+#define __NR_kexec_file_load 294
+__SYSCALL(__NR_kexec_file_load, sys_kexec_file_load)
#undef __NR_syscalls
-#define __NR_syscalls 294
+#define __NR_syscalls 295
/*
* 32 bit systems traditionally used different
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 02/16] kexec_file: make kexec_image_post_load_cleanup_default() global
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 01/16] asm-generic: add kexec_file_load system call to unistd.h AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 03/16] s390, kexec_file: drop arch_kexec_mem_walk() AKASHI Takahiro
` (13 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
Change this function from static to global so that arm64 can implement
its own arch_kimage_file_post_load_cleanup() later using
kexec_image_post_load_cleanup_default().
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Acked-by: Dave Young <dyoung@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
---
include/linux/kexec.h | 1 +
kernel/kexec_file.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 9e4e638fb505..49ab758f4d91 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -143,6 +143,7 @@ extern const struct kexec_file_ops * const kexec_file_loaders[];
int kexec_image_probe_default(struct kimage *image, void *buf,
unsigned long buf_len);
+int kexec_image_post_load_cleanup_default(struct kimage *image);
/**
* struct kexec_buf - parameters for finding a place for a buffer in memory
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index c6a3b6851372..63c7ce1c0c3e 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -78,7 +78,7 @@ void * __weak arch_kexec_kernel_image_load(struct kimage *image)
return kexec_image_load_default(image);
}
-static int kexec_image_post_load_cleanup_default(struct kimage *image)
+int kexec_image_post_load_cleanup_default(struct kimage *image)
{
if (!image->fops || !image->fops->cleanup)
return 0;
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 03/16] s390, kexec_file: drop arch_kexec_mem_walk()
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 01/16] asm-generic: add kexec_file_load system call to unistd.h AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 02/16] kexec_file: make kexec_image_post_load_cleanup_default() global AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 04/16] powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem() AKASHI Takahiro
` (12 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
Since s390 already knows where to locate buffers, calling
arch_kexec_mem_walk() has no sense. So we can just drop it as kbuf->mem
indicates this while all other architectures sets it to 0 initially.
This change is a preparatory work for the next patch, where all the
variant memory walks, either on system resource or memblock, will be
put in one common place so that it will satisfy all the architectures'
need.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Philipp Rudo <prudo@linux.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
---
arch/s390/kernel/machine_kexec_file.c | 10 ----------
include/linux/kexec.h | 8 ++++++++
kernel/kexec_file.c | 4 ++++
3 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index f413f57f8d20..32023b4f9dc0 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -134,16 +134,6 @@ int kexec_file_add_initrd(struct kimage *image, struct s390_load_data *data,
return ret;
}
-/*
- * The kernel is loaded to a fixed location. Turn off kexec_locate_mem_hole
- * and provide kbuf->mem by hand.
- */
-int arch_kexec_walk_mem(struct kexec_buf *kbuf,
- int (*func)(struct resource *, void *))
-{
- return 1;
-}
-
int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
Elf_Shdr *section,
const Elf_Shdr *relsec,
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 49ab758f4d91..f378cb786f1b 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -145,6 +145,14 @@ int kexec_image_probe_default(struct kimage *image, void *buf,
unsigned long buf_len);
int kexec_image_post_load_cleanup_default(struct kimage *image);
+/*
+ * If kexec_buf.mem is set to this value, kexec_locate_mem_hole()
+ * will try to allocate free memory. Arch may overwrite it.
+ */
+#ifndef KEXEC_BUF_MEM_UNKNOWN
+#define KEXEC_BUF_MEM_UNKNOWN 0
+#endif
+
/**
* struct kexec_buf - parameters for finding a place for a buffer in memory
* @image: kexec image in which memory to search.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 63c7ce1c0c3e..0fcaa86219d1 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -534,6 +534,10 @@ int kexec_locate_mem_hole(struct kexec_buf *kbuf)
{
int ret;
+ /* Arch knows where to place */
+ if (kbuf->mem != KEXEC_BUF_MEM_UNKNOWN)
+ return 0;
+
ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
return ret == 1 ? 0 : -EADDRNOTAVAIL;
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 04/16] powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem()
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (2 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 03/16] s390, kexec_file: drop arch_kexec_mem_walk() AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-30 10:08 ` Dave Young
2018-09-28 6:48 ` [PATCH v15 05/16] kexec_file: kexec_walk_memblock() only walks a dedicated region at kdump AKASHI Takahiro
` (11 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro,
Eric W. Biederman
Memblock list is another source for usable system memory layout.
So move powerpc's arch_kexec_walk_mem() to common code so that other
memblock-based architectures, particularly arm64, can also utilise it.
A moved function is now renamed to kexec_walk_memblock() and integrated
into kexec_locate_mem_hole(), which will now be usable for all
architectures with no need for overriding arch_kexec_walk_mem().
With this change, arch_kexec_walk_mem() need no longer be a weak function,
and was now renamed to kexec_walk_resources().
Since powerpc doesn't support kdump in its kexec_file_load(), the current
kexec_walk_memblock() won't work for kdump either in this form, this will
be fixed in the next patch.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Acked-by: Dave Young <dyoung@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Acked-by: James Morse <james.morse@arm.com>
---
arch/powerpc/kernel/machine_kexec_file_64.c | 54 -------------------
include/linux/kexec.h | 2 -
kernel/kexec_file.c | 60 +++++++++++++++++++--
3 files changed, 57 insertions(+), 59 deletions(-)
diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c b/arch/powerpc/kernel/machine_kexec_file_64.c
index c77e95e9b384..0d20c7ad40fa 100644
--- a/arch/powerpc/kernel/machine_kexec_file_64.c
+++ b/arch/powerpc/kernel/machine_kexec_file_64.c
@@ -24,7 +24,6 @@
#include <linux/slab.h>
#include <linux/kexec.h>
-#include <linux/memblock.h>
#include <linux/of_fdt.h>
#include <linux/libfdt.h>
#include <asm/ima.h>
@@ -46,59 +45,6 @@ int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
return kexec_image_probe_default(image, buf, buf_len);
}
-/**
- * arch_kexec_walk_mem - call func(data) for each unreserved memory block
- * @kbuf: Context info for the search. Also passed to @func.
- * @func: Function to call for each memory block.
- *
- * This function is used by kexec_add_buffer and kexec_locate_mem_hole
- * to find unreserved memory to load kexec segments into.
- *
- * Return: The memory walk will stop when func returns a non-zero value
- * and that value will be returned. If all free regions are visited without
- * func returning non-zero, then zero will be returned.
- */
-int arch_kexec_walk_mem(struct kexec_buf *kbuf,
- int (*func)(struct resource *, void *))
-{
- int ret = 0;
- u64 i;
- phys_addr_t mstart, mend;
- struct resource res = { };
-
- if (kbuf->top_down) {
- for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
- &mstart, &mend, NULL) {
- /*
- * In memblock, end points to the first byte after the
- * range while in kexec, end points to the last byte
- * in the range.
- */
- res.start = mstart;
- res.end = mend - 1;
- ret = func(&res, kbuf);
- if (ret)
- break;
- }
- } else {
- for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
- NULL) {
- /*
- * In memblock, end points to the first byte after the
- * range while in kexec, end points to the last byte
- * in the range.
- */
- res.start = mstart;
- res.end = mend - 1;
- ret = func(&res, kbuf);
- if (ret)
- break;
- }
- }
-
- return ret;
-}
-
/**
* setup_purgatory - initialize the purgatory's global variables
* @image: kexec image.
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index f378cb786f1b..d58d1f2fab10 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -192,8 +192,6 @@ int __weak arch_kexec_apply_relocations(struct purgatory_info *pi,
const Elf_Shdr *relsec,
const Elf_Shdr *symtab);
-int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
- int (*func)(struct resource *, void *));
extern int kexec_add_buffer(struct kexec_buf *kbuf);
int kexec_locate_mem_hole(struct kexec_buf *kbuf);
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 0fcaa86219d1..370d7eab49fe 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -16,6 +16,7 @@
#include <linux/file.h>
#include <linux/slab.h>
#include <linux/kexec.h>
+#include <linux/memblock.h>
#include <linux/mutex.h>
#include <linux/list.h>
#include <linux/fs.h>
@@ -501,6 +502,55 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
return locate_mem_hole_bottom_up(start, end, kbuf);
}
+#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_ARCH_DISCARD_MEMBLOCK)
+static int kexec_walk_memblock(struct kexec_buf *kbuf,
+ int (*func)(struct resource *, void *))
+{
+ int ret = 0;
+ u64 i;
+ phys_addr_t mstart, mend;
+ struct resource res = { };
+
+ if (kbuf->top_down) {
+ for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
+ &mstart, &mend, NULL) {
+ /*
+ * In memblock, end points to the first byte after the
+ * range while in kexec, end points to the last byte
+ * in the range.
+ */
+ res.start = mstart;
+ res.end = mend - 1;
+ ret = func(&res, kbuf);
+ if (ret)
+ break;
+ }
+ } else {
+ for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
+ NULL) {
+ /*
+ * In memblock, end points to the first byte after the
+ * range while in kexec, end points to the last byte
+ * in the range.
+ */
+ res.start = mstart;
+ res.end = mend - 1;
+ ret = func(&res, kbuf);
+ if (ret)
+ break;
+ }
+ }
+
+ return ret;
+}
+#else
+static int kexec_walk_memblock(struct kexec_buf *kbuf,
+ int (*func)(struct resource *, void *))
+{
+ return 0;
+}
+#endif
+
/**
* arch_kexec_walk_mem - call func(data) on free memory regions
* @kbuf: Context info for the search. Also passed to @func.
@@ -510,8 +560,8 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
* and that value will be returned. If all free regions are visited without
* func returning non-zero, then zero will be returned.
*/
-int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
- int (*func)(struct resource *, void *))
+static int kexec_walk_resources(struct kexec_buf *kbuf,
+ int (*func)(struct resource *, void *))
{
if (kbuf->image->type == KEXEC_TYPE_CRASH)
return walk_iomem_res_desc(crashk_res.desc,
@@ -538,7 +588,11 @@ int kexec_locate_mem_hole(struct kexec_buf *kbuf)
if (kbuf->mem != KEXEC_BUF_MEM_UNKNOWN)
return 0;
- ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
+ if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
+ !IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
+ ret = kexec_walk_memblock(kbuf, locate_mem_hole_callback);
+ else
+ ret = kexec_walk_resources(kbuf, locate_mem_hole_callback);
return ret == 1 ? 0 : -EADDRNOTAVAIL;
}
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v15 04/16] powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem()
2018-09-28 6:48 ` [PATCH v15 04/16] powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem() AKASHI Takahiro
@ 2018-09-30 10:08 ` Dave Young
2018-10-02 6:20 ` AKASHI Takahiro
0 siblings, 1 reply; 42+ messages in thread
From: Dave Young @ 2018-09-30 10:08 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
bhe, arnd, schwidefsky, heiko.carstens, ard.biesheuvel, bhsharma,
kexec, linux-kernel, prudo, james.morse, Eric W. Biederman,
linux-arm-kernel
Hi AKASHI,
On 09/28/18 at 03:48pm, AKASHI Takahiro wrote:
> Memblock list is another source for usable system memory layout.
> So move powerpc's arch_kexec_walk_mem() to common code so that other
> memblock-based architectures, particularly arm64, can also utilise it.
> A moved function is now renamed to kexec_walk_memblock() and integrated
> into kexec_locate_mem_hole(), which will now be usable for all
> architectures with no need for overriding arch_kexec_walk_mem().
>
> With this change, arch_kexec_walk_mem() need no longer be a weak function,
> and was now renamed to kexec_walk_resources().
>
> Since powerpc doesn't support kdump in its kexec_file_load(), the current
> kexec_walk_memblock() won't work for kdump either in this form, this will
> be fixed in the next patch.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Acked-by: Dave Young <dyoung@redhat.com>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Baoquan He <bhe@redhat.com>
> Acked-by: James Morse <james.morse@arm.com>
[snip]
> +
> /**
> * arch_kexec_walk_mem - call func(data) on free memory regions
The function name should be updated as well.
> * @kbuf: Context info for the search. Also passed to @func.
> @@ -510,8 +560,8 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> * and that value will be returned. If all free regions are visited without
> * func returning non-zero, then zero will be returned.
> */
> -int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> - int (*func)(struct resource *, void *))
> +static int kexec_walk_resources(struct kexec_buf *kbuf,
> + int (*func)(struct resource *, void *))
> {
> if (kbuf->image->type == KEXEC_TYPE_CRASH)
> return walk_iomem_res_desc(crashk_res.desc,
> @@ -538,7 +588,11 @@ int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> if (kbuf->mem != KEXEC_BUF_MEM_UNKNOWN)
> return 0;
>
> - ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> + if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> + !IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> + ret = kexec_walk_memblock(kbuf, locate_mem_hole_callback);
> + else
> + ret = kexec_walk_resources(kbuf, locate_mem_hole_callback);
>
> return ret == 1 ? 0 : -EADDRNOTAVAIL;
> }
> --
> 2.19.0
>
Thanks
Dave
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 04/16] powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem()
2018-09-30 10:08 ` Dave Young
@ 2018-10-02 6:20 ` AKASHI Takahiro
0 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-10-02 6:20 UTC (permalink / raw)
To: Dave Young
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
bhe, arnd, schwidefsky, heiko.carstens, ard.biesheuvel, bhsharma,
kexec, linux-kernel, prudo, james.morse, Eric W. Biederman,
linux-arm-kernel
On Sun, Sep 30, 2018 at 06:08:34PM +0800, Dave Young wrote:
> Hi AKASHI,
>
> On 09/28/18 at 03:48pm, AKASHI Takahiro wrote:
> > Memblock list is another source for usable system memory layout.
> > So move powerpc's arch_kexec_walk_mem() to common code so that other
> > memblock-based architectures, particularly arm64, can also utilise it.
> > A moved function is now renamed to kexec_walk_memblock() and integrated
> > into kexec_locate_mem_hole(), which will now be usable for all
> > architectures with no need for overriding arch_kexec_walk_mem().
> >
> > With this change, arch_kexec_walk_mem() need no longer be a weak function,
> > and was now renamed to kexec_walk_resources().
> >
> > Since powerpc doesn't support kdump in its kexec_file_load(), the current
> > kexec_walk_memblock() won't work for kdump either in this form, this will
> > be fixed in the next patch.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Acked-by: Dave Young <dyoung@redhat.com>
> > Cc: Vivek Goyal <vgoyal@redhat.com>
> > Cc: Baoquan He <bhe@redhat.com>
> > Acked-by: James Morse <james.morse@arm.com>
>
> [snip]
>
> > +
> > /**
> > * arch_kexec_walk_mem - call func(data) on free memory regions
>
> The function name should be updated as well.
Ah, thank you.
-Takahiro Akashi
> > * @kbuf: Context info for the search. Also passed to @func.
> > @@ -510,8 +560,8 @@ static int locate_mem_hole_callback(struct resource *res, void *arg)
> > * and that value will be returned. If all free regions are visited without
> > * func returning non-zero, then zero will be returned.
> > */
> > -int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
> > - int (*func)(struct resource *, void *))
> > +static int kexec_walk_resources(struct kexec_buf *kbuf,
> > + int (*func)(struct resource *, void *))
> > {
> > if (kbuf->image->type == KEXEC_TYPE_CRASH)
> > return walk_iomem_res_desc(crashk_res.desc,
> > @@ -538,7 +588,11 @@ int kexec_locate_mem_hole(struct kexec_buf *kbuf)
> > if (kbuf->mem != KEXEC_BUF_MEM_UNKNOWN)
> > return 0;
> >
> > - ret = arch_kexec_walk_mem(kbuf, locate_mem_hole_callback);
> > + if (IS_ENABLED(CONFIG_HAVE_MEMBLOCK) &&
> > + !IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> > + ret = kexec_walk_memblock(kbuf, locate_mem_hole_callback);
> > + else
> > + ret = kexec_walk_resources(kbuf, locate_mem_hole_callback);
> >
> > return ret == 1 ? 0 : -EADDRNOTAVAIL;
> > }
> > --
> > 2.19.0
> >
>
> Thanks
> Dave
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 05/16] kexec_file: kexec_walk_memblock() only walks a dedicated region at kdump
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (3 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 04/16] powerpc, kexec_file: factor out memblock-based arch_kexec_walk_mem() AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-30 10:10 ` Dave Young
2018-09-28 6:48 ` [PATCH v15 06/16] of/fdt: add helper functions for handling properties AKASHI Takahiro
` (10 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
In kdump case, there exists only one dedicated memblock region as usable
memory (crashk_res). With this patch, kexec_walk_memblock() runs a given
callback function on this region.
Cosmetic change: 0 to MEMBLOCK_NONE at for_each_free_mem_range*()
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Dave Young <dyoung@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
---
kernel/kexec_file.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 370d7eab49fe..2f615a7968dd 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -511,8 +511,11 @@ static int kexec_walk_memblock(struct kexec_buf *kbuf,
phys_addr_t mstart, mend;
struct resource res = { };
+ if (kbuf->image->type == KEXEC_TYPE_CRASH)
+ return func(&crashk_res, kbuf);
+
if (kbuf->top_down) {
- for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
+ for_each_free_mem_range_reverse(i, NUMA_NO_NODE, MEMBLOCK_NONE,
&mstart, &mend, NULL) {
/*
* In memblock, end points to the first byte after the
@@ -526,8 +529,8 @@ static int kexec_walk_memblock(struct kexec_buf *kbuf,
break;
}
} else {
- for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
- NULL) {
+ for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
+ &mstart, &mend, NULL) {
/*
* In memblock, end points to the first byte after the
* range while in kexec, end points to the last byte
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v15 05/16] kexec_file: kexec_walk_memblock() only walks a dedicated region at kdump
2018-09-28 6:48 ` [PATCH v15 05/16] kexec_file: kexec_walk_memblock() only walks a dedicated region at kdump AKASHI Takahiro
@ 2018-09-30 10:10 ` Dave Young
0 siblings, 0 replies; 42+ messages in thread
From: Dave Young @ 2018-09-30 10:10 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
bhe, arnd, schwidefsky, heiko.carstens, ard.biesheuvel, bhsharma,
kexec, linux-kernel, prudo, james.morse, linux-arm-kernel
On 09/28/18 at 03:48pm, AKASHI Takahiro wrote:
> In kdump case, there exists only one dedicated memblock region as usable
> memory (crashk_res). With this patch, kexec_walk_memblock() runs a given
> callback function on this region.
>
> Cosmetic change: 0 to MEMBLOCK_NONE at for_each_free_mem_range*()
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Baoquan He <bhe@redhat.com>
Acked-by: Dave Young <dyoung@redhat.com>
> ---
> kernel/kexec_file.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 370d7eab49fe..2f615a7968dd 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -511,8 +511,11 @@ static int kexec_walk_memblock(struct kexec_buf *kbuf,
> phys_addr_t mstart, mend;
> struct resource res = { };
>
> + if (kbuf->image->type == KEXEC_TYPE_CRASH)
> + return func(&crashk_res, kbuf);
> +
> if (kbuf->top_down) {
> - for_each_free_mem_range_reverse(i, NUMA_NO_NODE, 0,
> + for_each_free_mem_range_reverse(i, NUMA_NO_NODE, MEMBLOCK_NONE,
> &mstart, &mend, NULL) {
> /*
> * In memblock, end points to the first byte after the
> @@ -526,8 +529,8 @@ static int kexec_walk_memblock(struct kexec_buf *kbuf,
> break;
> }
> } else {
> - for_each_free_mem_range(i, NUMA_NO_NODE, 0, &mstart, &mend,
> - NULL) {
> + for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE,
> + &mstart, &mend, NULL) {
> /*
> * In memblock, end points to the first byte after the
> * range while in kexec, end points to the last byte
> --
> 2.19.0
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
Thanks
Dave
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (4 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 05/16] kexec_file: kexec_walk_memblock() only walks a dedicated region at kdump AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 9:04 ` kbuild test robot
2018-09-28 13:44 ` Rob Herring
2018-09-28 6:48 ` [PATCH v15 07/16] arm64: add image head flag definitions AKASHI Takahiro
` (9 subsequent siblings)
15 siblings, 2 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro, Rob Herring,
Frank Rowand, devicetree
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
---
drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
include/linux/of_fdt.h | 4 +++
2 files changed, 60 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 800ad252cf9c..c65c31562ccb 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
#include <linux/debugfs.h>
#include <linux/serial_core.h>
#include <linux/sysfs.h>
+#include <linux/types.h>
#include <asm/setup.h> /* for COMMAND_LINE_SIZE */
#include <asm/page.h>
@@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
#endif
#endif /* CONFIG_OF_EARLY_FLATTREE */
+
+#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
+#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
+
+int fdt_prop_len(const char *prop_name, int len)
+{
+ return (strlen(prop_name) + 1) +
+ sizeof(struct fdt_property) +
+ FDT_TAGALIGN(len);
+}
+
+static void fill_property(void *buf, u64 val64, int cells)
+{
+ __be32 val32;
+
+ while (cells) {
+ val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
+ memcpy(buf, &val32, sizeof(val32));
+ buf += sizeof(val32);
+ }
+}
+
+int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
+ u64 addr, u64 size)
+{
+ int addr_cells, size_cells;
+ char buf[sizeof(__be32) * 2 * 2];
+ /* assume dt_root_[addr|size]_cells <= 2 */
+ void *prop;
+ size_t buf_size;
+
+ addr_cells = fdt_address_cells(fdt, 0);
+ if (addr_cells < 0)
+ return addr_cells;
+ size_cells = fdt_size_cells(fdt, 0);
+ if (size_cells < 0)
+ return size_cells;
+
+ /* if *_cells >= 2, cells can hold 64-bit values anyway */
+ if ((addr_cells == 1) && (addr > U32_MAX))
+ return -FDT_ERR_BADVALUE;
+
+ if ((size_cells == 1) && (size > U32_MAX))
+ return -FDT_ERR_BADVALUE;
+
+ buf_size = (addr_cells + size_cells) * sizeof(u32);
+ prop = buf;
+
+ fill_property(prop, addr, addr_cells);
+ prop += addr_cells * sizeof(u32);
+
+ fill_property(prop, size, size_cells);
+
+ return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
+}
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index b9cd9ebdf9b9..842af6ea92ea 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
static inline void unflatten_and_copy_device_tree(void) {}
#endif /* CONFIG_OF_EARLY_FLATTREE */
+int fdt_prop_len(const char *prop_name, int len);
+int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
+ u64 addr, u64 size);
+
#endif /* __ASSEMBLY__ */
#endif /* _LINUX_OF_FDT_H */
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-09-28 6:48 ` [PATCH v15 06/16] of/fdt: add helper functions for handling properties AKASHI Takahiro
@ 2018-09-28 9:04 ` kbuild test robot
2018-09-28 13:44 ` Rob Herring
1 sibling, 0 replies; 42+ messages in thread
From: kbuild test robot @ 2018-09-28 9:04 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: kbuild-all, catalin.marinas, will.deacon, dhowells, vgoyal,
herbert, davem, dyoung, bhe, arnd, schwidefsky, heiko.carstens,
prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro, Rob Herring,
Frank Rowand, devicetree
[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]
Hi AKASHI,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.19-rc5 next-20180928]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/AKASHI-Takahiro/arm64-kexec-add-kexec_file_load-support/20180928-151042
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: i386-randconfig-s0-201838 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/of/fdt.o: In function `fdt_setprop_reg':
>> drivers/of/fdt.c:1358: undefined reference to `fdt_address_cells'
>> drivers/of/fdt.c:1361: undefined reference to `fdt_size_cells'
vim +1358 drivers/of/fdt.c
1348
1349 int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
1350 u64 addr, u64 size)
1351 {
1352 int addr_cells, size_cells;
1353 char buf[sizeof(__be32) * 2 * 2];
1354 /* assume dt_root_[addr|size]_cells <= 2 */
1355 void *prop;
1356 size_t buf_size;
1357
> 1358 addr_cells = fdt_address_cells(fdt, 0);
1359 if (addr_cells < 0)
1360 return addr_cells;
> 1361 size_cells = fdt_size_cells(fdt, 0);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31880 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-09-28 6:48 ` [PATCH v15 06/16] of/fdt: add helper functions for handling properties AKASHI Takahiro
2018-09-28 9:04 ` kbuild test robot
@ 2018-09-28 13:44 ` Rob Herring
2018-09-28 21:13 ` Frank Rowand
` (2 more replies)
1 sibling, 3 replies; 42+ messages in thread
From: Rob Herring @ 2018-09-28 13:44 UTC (permalink / raw)
To: AKASHI, Takahiro, David Gibson
Cc: Catalin Marinas, Will Deacon, David Howells, Vivek Goyal,
Herbert Xu, David Miller, dyoung, Baoquan He, Arnd Bergmann,
Martin Schwidefsky, Heiko Carstens, prudo, Ard Biesheuvel,
James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, Frank Rowand, devicetree
+David Gibson
On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> These functions will be used later to handle kexec-specific properties
> in arm64's kexec_file implementation.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: devicetree@vger.kernel.org
> ---
> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/of_fdt.h | 4 +++
> 2 files changed, 60 insertions(+)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 800ad252cf9c..c65c31562ccb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -25,6 +25,7 @@
> #include <linux/debugfs.h>
> #include <linux/serial_core.h>
> #include <linux/sysfs.h>
> +#include <linux/types.h>
>
> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> #include <asm/page.h>
> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> #endif
>
> #endif /* CONFIG_OF_EARLY_FLATTREE */
> +
> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> +
> +int fdt_prop_len(const char *prop_name, int len)
> +{
> + return (strlen(prop_name) + 1) +
> + sizeof(struct fdt_property) +
> + FDT_TAGALIGN(len);
Looks like you are using this to calculate how much space you need to
allocate in addition to the current DTB for a couple of new or
replaced properties. I'm not sure that this calculation is completely
accurate. And it is strange there doesn't seem to be any libfdt
function for this already. It would be simpler to just add some fixed
additional amount.
Maybe David G has comments on this?
> +}
> +
The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
should go into libfdt, but I'm fine with having it in the kernel for
now.
> +static void fill_property(void *buf, u64 val64, int cells)
> +{
> + __be32 val32;
> +
> + while (cells) {
> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> + memcpy(buf, &val32, sizeof(val32));
> + buf += sizeof(val32);
This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
BTW, for purposes of moving to libfdt, we'll need the authors'
(Masahiro Yamada and Hans de Goede) permission to dual license.
> + }
> +}
> +
> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> + u64 addr, u64 size)
> +{
> + int addr_cells, size_cells;
> + char buf[sizeof(__be32) * 2 * 2];
> + /* assume dt_root_[addr|size]_cells <= 2 */
> + void *prop;
> + size_t buf_size;
> +
> + addr_cells = fdt_address_cells(fdt, 0);
> + if (addr_cells < 0)
> + return addr_cells;
> + size_cells = fdt_size_cells(fdt, 0);
> + if (size_cells < 0)
> + return size_cells;
> +
> + /* if *_cells >= 2, cells can hold 64-bit values anyway */
> + if ((addr_cells == 1) && (addr > U32_MAX))
> + return -FDT_ERR_BADVALUE;
> +
> + if ((size_cells == 1) && (size > U32_MAX))
> + return -FDT_ERR_BADVALUE;
> +
> + buf_size = (addr_cells + size_cells) * sizeof(u32);
> + prop = buf;
> +
> + fill_property(prop, addr, addr_cells);
> + prop += addr_cells * sizeof(u32);
> +
> + fill_property(prop, size, size_cells);
> +
> + return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> +}
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index b9cd9ebdf9b9..842af6ea92ea 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> static inline void unflatten_and_copy_device_tree(void) {}
> #endif /* CONFIG_OF_EARLY_FLATTREE */
>
> +int fdt_prop_len(const char *prop_name, int len);
> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> + u64 addr, u64 size);
> +
> #endif /* __ASSEMBLY__ */
> #endif /* _LINUX_OF_FDT_H */
> --
> 2.19.0
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-09-28 13:44 ` Rob Herring
@ 2018-09-28 21:13 ` Frank Rowand
2018-10-05 5:06 ` AKASHI, Takahiro
2018-10-02 4:47 ` David Gibson
[not found] ` <20181005030849.GK32578@linaro.org>
2 siblings, 1 reply; 42+ messages in thread
From: Frank Rowand @ 2018-09-28 21:13 UTC (permalink / raw)
To: Rob Herring, AKASHI, Takahiro, David Gibson
Cc: Catalin Marinas, Will Deacon, David Howells, Vivek Goyal,
Herbert Xu, David Miller, dyoung, Baoquan He, Arnd Bergmann,
Martin Schwidefsky, Heiko Carstens, prudo, Ard Biesheuvel,
James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
On 09/28/18 06:44, Rob Herring wrote:
> +David Gibson
>
> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
>>
>> These functions will be used later to handle kexec-specific properties
>> in arm64's kexec_file implementation.
As I requested in version 14:
The intent of the helper functions is related to properties whose values are
tuples of the same format as the "reg" property of the "/memory" nodes. For
example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
the "/chosen" node.
The patch header and the function names should be updated to reflect this intent.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: devicetree@vger.kernel.org
>> ---
Missing list of changes since version 14.
>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of_fdt.h | 4 +++
>> 2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 800ad252cf9c..c65c31562ccb 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -25,6 +25,7 @@
>> #include <linux/debugfs.h>
>> #include <linux/serial_core.h>
>> #include <linux/sysfs.h>
>> +#include <linux/types.h>
>>
>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
>> #include <asm/page.h>
>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>> #endif
>>
>> #endif /* CONFIG_OF_EARLY_FLATTREE */
In v14 I requested:
Please add comment:
/* helper functions for arm64 kexec */
>> +
>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
>> +
>> +int fdt_prop_len(const char *prop_name, int len)
In v14, I requested:
Please rename as fdt_len_added_prop()
I'm not really happy with my suggested name, but do not have a
better one yet. As Rob notes, maybe David G will have a helpful
comment.
>> +{
>> + return (strlen(prop_name) + 1) +
>> + sizeof(struct fdt_property) +
>> + FDT_TAGALIGN(len);
>
> Looks like you are using this to calculate how much space you need to
> allocate in addition to the current DTB for a couple of new or
> replaced properties. I'm not sure that this calculation is completely
> accurate. And it is strange there doesn't seem to be any libfdt
> function for this already. It would be simpler to just add some fixed
> additional amount.
>
> Maybe David G has comments on this?
>
>> +}
>> +
>
> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> should go into libfdt, but I'm fine with having it in the kernel for
> now.
>
>> +static void fill_property(void *buf, u64 val64, int cells)
In v14 I requested:
Please rename as cpu64_to_fdt_cells()
>> +{
>> + __be32 val32;
>> +
>> + while (cells) {
>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>> + memcpy(buf, &val32, sizeof(val32));
>> + buf += sizeof(val32);
>
> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>
> BTW, for purposes of moving to libfdt, we'll need the authors'
> (Masahiro Yamada and Hans de Goede) permission to dual license.
>
>> + }
>> +}
>> +
>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>> + u64 addr, u64 size)
>> +{
>> + int addr_cells, size_cells;
unsigned
>> + char buf[sizeof(__be32) * 2 * 2];
>> + /* assume dt_root_[addr|size]_cells <= 2 */
>> + void *prop;
>> + size_t buf_size;
>> +
>> + addr_cells = fdt_address_cells(fdt, 0);
>> + if (addr_cells < 0)
>> + return addr_cells;
>> + size_cells = fdt_size_cells(fdt, 0);
>> + if (size_cells < 0)
>> + return size_cells;
>> +
>> + /* if *_cells >= 2, cells can hold 64-bit values anyway */
>> + if ((addr_cells == 1) && (addr > U32_MAX))
>> + return -FDT_ERR_BADVALUE;
>> +
>> + if ((size_cells == 1) && (size > U32_MAX))
>> + return -FDT_ERR_BADVALUE;
In v14 I requested:
Should also check that base + size does not wrap around.
>> +
>> + buf_size = (addr_cells + size_cells) * sizeof(u32);
>> + prop = buf;
>> +
>> + fill_property(prop, addr, addr_cells);
>> + prop += addr_cells * sizeof(u32);
>> +
>> + fill_property(prop, size, size_cells);
>> +
>> + return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>> +}
>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>> index b9cd9ebdf9b9..842af6ea92ea 100644
>> --- a/include/linux/of_fdt.h
>> +++ b/include/linux/of_fdt.h
>> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
>> static inline void unflatten_and_copy_device_tree(void) {}
>> #endif /* CONFIG_OF_EARLY_FLATTREE */
>>
>> +int fdt_prop_len(const char *prop_name, int len);
>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>> + u64 addr, u64 size);
>> +
>> #endif /* __ASSEMBLY__ */
>> #endif /* _LINUX_OF_FDT_H */
>> --
>> 2.19.0
>>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-09-28 21:13 ` Frank Rowand
@ 2018-10-05 5:06 ` AKASHI, Takahiro
2018-10-05 5:09 ` David Gibson
2018-10-09 18:02 ` Frank Rowand
0 siblings, 2 replies; 42+ messages in thread
From: AKASHI, Takahiro @ 2018-10-05 5:06 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, David Gibson, Catalin Marinas, Will Deacon,
David Howells, Vivek Goyal, Herbert Xu, David Miller, dyoung,
Baoquan He, Arnd Bergmann, Martin Schwidefsky, Heiko Carstens,
prudo, Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
Frank,
# I haven't reply to your comments.
On Fri, Sep 28, 2018 at 02:13:58PM -0700, Frank Rowand wrote:
> On 09/28/18 06:44, Rob Herring wrote:
> > +David Gibson
> >
> > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> >>
> >> These functions will be used later to handle kexec-specific properties
> >> in arm64's kexec_file implementation.
>
> As I requested in version 14:
>
> The intent of the helper functions is related to properties whose values are
> tuples of the same format as the "reg" property of the "/memory" nodes. For
> example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
> the "/chosen" node.
>
> The patch header and the function names should be updated to reflect this intent.
I agree regarding the patch header.
>
> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Frank Rowand <frowand.list@gmail.com>
> >> Cc: devicetree@vger.kernel.org
> >> ---
>
> Missing list of changes since version 14.
Sorry for the inconvenience, but a whole change list goes into
the cover letter, not individual patches.
>
> >> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/of_fdt.h | 4 +++
> >> 2 files changed, 60 insertions(+)
> >>
> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >> index 800ad252cf9c..c65c31562ccb 100644
> >> --- a/drivers/of/fdt.c
> >> +++ b/drivers/of/fdt.c
> >> @@ -25,6 +25,7 @@
> >> #include <linux/debugfs.h>
> >> #include <linux/serial_core.h>
> >> #include <linux/sysfs.h>
> >> +#include <linux/types.h>
> >>
> >> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> >> #include <asm/page.h>
> >> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >> #endif
> >>
> >> #endif /* CONFIG_OF_EARLY_FLATTREE */
>
> In v14 I requested:
>
> Please add comment:
>
> /* helper functions for arm64 kexec */
Okay.
>
> >> +
> >> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> >> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> >> +
> >> +int fdt_prop_len(const char *prop_name, int len)
>
> In v14, I requested:
>
> Please rename as fdt_len_added_prop()
Anyhow, I will drop this function, preferring to new
fdt_[address|size]_cells().
> I'm not really happy with my suggested name, but do not have a
> better one yet. As Rob notes, maybe David G will have a helpful
> comment.
>
> >> +{
> >> + return (strlen(prop_name) + 1) +
> >> + sizeof(struct fdt_property) +
> >> + FDT_TAGALIGN(len);
> >
> > Looks like you are using this to calculate how much space you need to
> > allocate in addition to the current DTB for a couple of new or
> > replaced properties. I'm not sure that this calculation is completely
> > accurate. And it is strange there doesn't seem to be any libfdt
> > function for this already. It would be simpler to just add some fixed
> > additional amount.
> >
> > Maybe David G has comments on this?
I'm not quit sure why it's not that accurate, but as I said in a reply to
David's comment, I will take your suggestion.
> >> +}
> >> +
> >
> > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > should go into libfdt, but I'm fine with having it in the kernel for
> > now.
> >
> >> +static void fill_property(void *buf, u64 val64, int cells)
>
> In v14 I requested:
>
> Please rename as cpu64_to_fdt_cells()
I don't mind, but this function may be dropped if Rob sticks to
u-boot's fdt_pack_reg() over my fdt_setprop_reg().
>
> >> +{
> >> + __be32 val32;
> >> +
> >> + while (cells) {
> >> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >> + memcpy(buf, &val32, sizeof(val32));
> >> + buf += sizeof(val32);
> >
> > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >
> > BTW, for purposes of moving to libfdt, we'll need the authors'
> > (Masahiro Yamada and Hans de Goede) permission to dual license.
> >
> >> + }
> >> +}
> >> +
> >> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >> + u64 addr, u64 size)
> >> +{
> >> + int addr_cells, size_cells;
>
> unsigned
fdt_[address|size]_cell() returns an int.
>
> >> + char buf[sizeof(__be32) * 2 * 2];
> >> + /* assume dt_root_[addr|size]_cells <= 2 */
> >> + void *prop;
> >> + size_t buf_size;
> >> +
> >> + addr_cells = fdt_address_cells(fdt, 0);
> >> + if (addr_cells < 0)
> >> + return addr_cells;
> >> + size_cells = fdt_size_cells(fdt, 0);
> >> + if (size_cells < 0)
> >> + return size_cells;
> >> +
> >> + /* if *_cells >= 2, cells can hold 64-bit values anyway */
> >> + if ((addr_cells == 1) && (addr > U32_MAX))
> >> + return -FDT_ERR_BADVALUE;
> >> +
> >> + if ((size_cells == 1) && (size > U32_MAX))
> >> + return -FDT_ERR_BADVALUE;
>
> In v14 I requested:
>
> Should also check that base + size does not wrap around.
Okay, I will start discussion, as you have suggested, in devicetree-spec ML.
Thanks,
-Takahiro Akashi
>
> >> +
> >> + buf_size = (addr_cells + size_cells) * sizeof(u32);
> >> + prop = buf;
> >> +
> >> + fill_property(prop, addr, addr_cells);
> >> + prop += addr_cells * sizeof(u32);
> >> +
> >> + fill_property(prop, size, size_cells);
> >> +
> >> + return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> >> +}
> >> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> >> index b9cd9ebdf9b9..842af6ea92ea 100644
> >> --- a/include/linux/of_fdt.h
> >> +++ b/include/linux/of_fdt.h
> >> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> >> static inline void unflatten_and_copy_device_tree(void) {}
> >> #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>
> >> +int fdt_prop_len(const char *prop_name, int len);
> >> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >> + u64 addr, u64 size);
> >> +
> >> #endif /* __ASSEMBLY__ */
> >> #endif /* _LINUX_OF_FDT_H */
> >> --
> >> 2.19.0
> >>
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-05 5:06 ` AKASHI, Takahiro
@ 2018-10-05 5:09 ` David Gibson
2018-10-09 18:02 ` Frank Rowand
1 sibling, 0 replies; 42+ messages in thread
From: David Gibson @ 2018-10-05 5:09 UTC (permalink / raw)
To: AKASHI, Takahiro, Frank Rowand, Rob Herring, Catalin Marinas,
Will Deacon, David Howells, Vivek Goyal, Herbert Xu,
David Miller, dyoung, Baoquan He, Arnd Bergmann,
Martin Schwidefsky, Heiko Carstens, prudo, Ard Biesheuvel,
James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]
On Fri, Oct 05, 2018 at 02:06:29PM +0900, AKASHI, Takahiro wrote:
[snip]
> > >> +{
> > >> + return (strlen(prop_name) + 1) +
> > >> + sizeof(struct fdt_property) +
> > >> + FDT_TAGALIGN(len);
> > >
> > > Looks like you are using this to calculate how much space you need to
> > > allocate in addition to the current DTB for a couple of new or
> > > replaced properties. I'm not sure that this calculation is completely
> > > accurate. And it is strange there doesn't seem to be any libfdt
> > > function for this already. It would be simpler to just add some fixed
> > > additional amount.
> > >
> > > Maybe David G has comments on this?
>
> I'm not quit sure why it's not that accurate, but as I said in a reply to
> David's comment, I will take your suggestion.
I think it is accurate, but it is not robust. It might not be
accurate if we ever make further tweaks to the libfdt format. libfdt
is supposed to abstract those changes away, but if you make
assumptions like this, you break that abstraction.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-05 5:06 ` AKASHI, Takahiro
2018-10-05 5:09 ` David Gibson
@ 2018-10-09 18:02 ` Frank Rowand
2018-10-10 0:30 ` David Gibson
2018-10-10 1:04 ` AKASHI, Takahiro
1 sibling, 2 replies; 42+ messages in thread
From: Frank Rowand @ 2018-10-09 18:02 UTC (permalink / raw)
To: AKASHI, Takahiro, Rob Herring, David Gibson, Catalin Marinas,
Will Deacon, David Howells, Vivek Goyal, Herbert Xu,
David Miller, dyoung, Baoquan He, Arnd Bergmann,
Martin Schwidefsky, Heiko Carstens, prudo, Ard Biesheuvel,
James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
On 10/04/18 22:06, AKASHI, Takahiro wrote:
> Frank,
>
> # I haven't reply to your comments.
>
> On Fri, Sep 28, 2018 at 02:13:58PM -0700, Frank Rowand wrote:
>> On 09/28/18 06:44, Rob Herring wrote:
>>> +David Gibson
>>>
>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
>>> <takahiro.akashi@linaro.org> wrote:
>>>>
>>>> These functions will be used later to handle kexec-specific properties
>>>> in arm64's kexec_file implementation.
>>
>> As I requested in version 14:
>>
>> The intent of the helper functions is related to properties whose values are
>> tuples of the same format as the "reg" property of the "/memory" nodes. For
>> example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
>> the "/chosen" node.
>>
>> The patch header and the function names should be updated to reflect this intent.
>
> I agree regarding the patch header.
>
>>
>>>>
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>> Cc: devicetree@vger.kernel.org
>>>> ---
>>
>> Missing list of changes since version 14.
>
> Sorry for the inconvenience, but a whole change list goes into
> the cover letter, not individual patches.
That works for me, but please add us to the distro list for the cover letter.
>>>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/of_fdt.h | 4 +++
>>>> 2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>> index 800ad252cf9c..c65c31562ccb 100644
>>>> --- a/drivers/of/fdt.c
>>>> +++ b/drivers/of/fdt.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/debugfs.h>
>>>> #include <linux/serial_core.h>
>>>> #include <linux/sysfs.h>
>>>> +#include <linux/types.h>
>>>>
>>>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
>>>> #include <asm/page.h>
>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>>> #endif
>>>>
>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
>>
>> In v14 I requested:
>>
>> Please add comment:
>>
>> /* helper functions for arm64 kexec */
>
> Okay.
>
>>
>>>> +
>>>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
>>>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
>>>> +
>>>> +int fdt_prop_len(const char *prop_name, int len)
>>
>> In v14, I requested:
>>
>> Please rename as fdt_len_added_prop()
>
> Anyhow, I will drop this function, preferring to new
> fdt_[address|size]_cells().
>
>> I'm not really happy with my suggested name, but do not have a
>> better one yet. As Rob notes, maybe David G will have a helpful
>> comment.
>>
>>>> +{
>>>> + return (strlen(prop_name) + 1) +
>>>> + sizeof(struct fdt_property) +
>>>> + FDT_TAGALIGN(len);
>>>
>>> Looks like you are using this to calculate how much space you need to
>>> allocate in addition to the current DTB for a couple of new or
>>> replaced properties. I'm not sure that this calculation is completely
>>> accurate. And it is strange there doesn't seem to be any libfdt
>>> function for this already. It would be simpler to just add some fixed
>>> additional amount.
>>>
>>> Maybe David G has comments on this?
>
> I'm not quit sure why it's not that accurate, but as I said in a reply to
> David's comment, I will take your suggestion.
>
>>>> +}
>>>> +
>>>
>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
>>> should go into libfdt, but I'm fine with having it in the kernel for
>>> now.
>>>
>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>
>> In v14 I requested:
>>
>> Please rename as cpu64_to_fdt_cells()
>
> I don't mind, but this function may be dropped if Rob sticks to
> u-boot's fdt_pack_reg() over my fdt_setprop_reg().
I have another comment a couple of minutes ago in the longer thread
about this function.
>>
>>>> +{
>>>> + __be32 val32;
>>>> +
>>>> + while (cells) {
>>>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>> + memcpy(buf, &val32, sizeof(val32));
>>>> + buf += sizeof(val32);
>>>
>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>>>
>>> BTW, for purposes of moving to libfdt, we'll need the authors'
>>> (Masahiro Yamada and Hans de Goede) permission to dual license.
>>>
>>>> + }
>>>> +}
>>>> +
>>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>>> + u64 addr, u64 size)
>>>> +{
>>>> + int addr_cells, size_cells;
>>
>> unsigned
>
> fdt_[address|size]_cell() returns an int.
I stand corrected. They take advantage of the fact that valid values
are in the range 0..4 and return a negative value for error.
>>>> + char buf[sizeof(__be32) * 2 * 2];
>>>> + /* assume dt_root_[addr|size]_cells <= 2 */
>>>> + void *prop;
>>>> + size_t buf_size;
>>>> +
>>>> + addr_cells = fdt_address_cells(fdt, 0);
>>>> + if (addr_cells < 0)
>>>> + return addr_cells;
>>>> + size_cells = fdt_size_cells(fdt, 0);
>>>> + if (size_cells < 0)
>>>> + return size_cells;
>>>> +
>>>> + /* if *_cells >= 2, cells can hold 64-bit values anyway */
>>>> + if ((addr_cells == 1) && (addr > U32_MAX))
>>>> + return -FDT_ERR_BADVALUE;
>>>> +
>>>> + if ((size_cells == 1) && (size > U32_MAX))
>>>> + return -FDT_ERR_BADVALUE;
>>
>> In v14 I requested:
>>
>> Should also check that base + size does not wrap around.
>
> Okay, I will start discussion, as you have suggested, in devicetree-spec ML.
>
> Thanks,
> -Takahiro Akashi
>
>>
>>>> +
>>>> + buf_size = (addr_cells + size_cells) * sizeof(u32);
>>>> + prop = buf;
>>>> +
>>>> + fill_property(prop, addr, addr_cells);
>>>> + prop += addr_cells * sizeof(u32);
>>>> +
>>>> + fill_property(prop, size, size_cells);
>>>> +
>>>> + return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
>>>> +}
>>>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>>>> index b9cd9ebdf9b9..842af6ea92ea 100644
>>>> --- a/include/linux/of_fdt.h
>>>> +++ b/include/linux/of_fdt.h
>>>> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
>>>> static inline void unflatten_and_copy_device_tree(void) {}
>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>
>>>> +int fdt_prop_len(const char *prop_name, int len);
>>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
>>>> + u64 addr, u64 size);
>>>> +
>>>> #endif /* __ASSEMBLY__ */
>>>> #endif /* _LINUX_OF_FDT_H */
>>>> --
>>>> 2.19.0
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-09 18:02 ` Frank Rowand
@ 2018-10-10 0:30 ` David Gibson
2018-10-10 1:04 ` AKASHI, Takahiro
1 sibling, 0 replies; 42+ messages in thread
From: David Gibson @ 2018-10-10 0:30 UTC (permalink / raw)
To: Frank Rowand
Cc: AKASHI, Takahiro, Rob Herring, Catalin Marinas, Will Deacon,
David Howells, Vivek Goyal, Herbert Xu, David Miller, dyoung,
Baoquan He, Arnd Bergmann, Martin Schwidefsky, Heiko Carstens,
prudo, Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
[-- Attachment #1: Type: text/plain, Size: 952 bytes --]
On Tue, Oct 09, 2018 at 11:02:29AM -0700, Frank Rowand wrote:
> On 10/04/18 22:06, AKASHI, Takahiro wrote:
[snip]
> >>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>>> + u64 addr, u64 size)
> >>>> +{
> >>>> + int addr_cells, size_cells;
> >>
> >> unsigned
> >
> > fdt_[address|size]_cell() returns an int.
>
> I stand corrected. They take advantage of the fact that valid values
> are in the range 0..4 and return a negative value for error.
I'm not sure if the spec formally defines that these values will fit
in a positive 'int'. But if addresses are 8GiB each, you're going to
have much bigger problems putting it in a device tree than this
function.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-09 18:02 ` Frank Rowand
2018-10-10 0:30 ` David Gibson
@ 2018-10-10 1:04 ` AKASHI, Takahiro
1 sibling, 0 replies; 42+ messages in thread
From: AKASHI, Takahiro @ 2018-10-10 1:04 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, David Gibson, Catalin Marinas, Will Deacon,
David Howells, Vivek Goyal, Herbert Xu, David Miller, dyoung,
Baoquan He, Arnd Bergmann, Martin Schwidefsky, Heiko Carstens,
prudo, Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
Frank,
On Tue, Oct 09, 2018 at 11:02:29AM -0700, Frank Rowand wrote:
> On 10/04/18 22:06, AKASHI, Takahiro wrote:
> > Frank,
> >
> > # I haven't reply to your comments.
> >
> > On Fri, Sep 28, 2018 at 02:13:58PM -0700, Frank Rowand wrote:
> >> On 09/28/18 06:44, Rob Herring wrote:
> >>> +David Gibson
> >>>
> >>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> >>> <takahiro.akashi@linaro.org> wrote:
> >>>>
> >>>> These functions will be used later to handle kexec-specific properties
> >>>> in arm64's kexec_file implementation.
> >>
> >> As I requested in version 14:
> >>
> >> The intent of the helper functions is related to properties whose values are
> >> tuples of the same format as the "reg" property of the "/memory" nodes. For
> >> example, the "linux,usable-memory-range" and "linux,elfcoredhr" properties of
> >> the "/chosen" node.
> >>
> >> The patch header and the function names should be updated to reflect this intent.
> >
> > I agree regarding the patch header.
> >
> >>
> >>>>
> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>> Cc: Rob Herring <robh+dt@kernel.org>
> >>>> Cc: Frank Rowand <frowand.list@gmail.com>
> >>>> Cc: devicetree@vger.kernel.org
> >>>> ---
> >>
> >> Missing list of changes since version 14.
> >
> > Sorry for the inconvenience, but a whole change list goes into
> > the cover letter, not individual patches.
>
> That works for me, but please add us to the distro list for the cover letter.
Sure, I will try to do so.
>
> >>>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> >>>> include/linux/of_fdt.h | 4 +++
> >>>> 2 files changed, 60 insertions(+)
> >>>>
> >>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>>> index 800ad252cf9c..c65c31562ccb 100644
> >>>> --- a/drivers/of/fdt.c
> >>>> +++ b/drivers/of/fdt.c
> >>>> @@ -25,6 +25,7 @@
> >>>> #include <linux/debugfs.h>
> >>>> #include <linux/serial_core.h>
> >>>> #include <linux/sysfs.h>
> >>>> +#include <linux/types.h>
> >>>>
> >>>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> >>>> #include <asm/page.h>
> >>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >>>> #endif
> >>>>
> >>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>
> >> In v14 I requested:
> >>
> >> Please add comment:
> >>
> >> /* helper functions for arm64 kexec */
> >
> > Okay.
> >
> >>
> >>>> +
> >>>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> >>>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> >>>> +
> >>>> +int fdt_prop_len(const char *prop_name, int len)
> >>
> >> In v14, I requested:
> >>
> >> Please rename as fdt_len_added_prop()
> >
> > Anyhow, I will drop this function, preferring to new
> > fdt_[address|size]_cells().
> >
> >> I'm not really happy with my suggested name, but do not have a
> >> better one yet. As Rob notes, maybe David G will have a helpful
> >> comment.
> >>
> >>>> +{
> >>>> + return (strlen(prop_name) + 1) +
> >>>> + sizeof(struct fdt_property) +
> >>>> + FDT_TAGALIGN(len);
> >>>
> >>> Looks like you are using this to calculate how much space you need to
> >>> allocate in addition to the current DTB for a couple of new or
> >>> replaced properties. I'm not sure that this calculation is completely
> >>> accurate. And it is strange there doesn't seem to be any libfdt
> >>> function for this already. It would be simpler to just add some fixed
> >>> additional amount.
> >>>
> >>> Maybe David G has comments on this?
> >
> > I'm not quit sure why it's not that accurate, but as I said in a reply to
> > David's comment, I will take your suggestion.
> >
> >>>> +}
> >>>> +
> >>>
> >>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> >>> should go into libfdt, but I'm fine with having it in the kernel for
> >>> now.
> >>>
> >>>> +static void fill_property(void *buf, u64 val64, int cells)
> >>
> >> In v14 I requested:
> >>
> >> Please rename as cpu64_to_fdt_cells()
> >
> > I don't mind, but this function may be dropped if Rob sticks to
> > u-boot's fdt_pack_reg() over my fdt_setprop_reg().
>
> I have another comment a couple of minutes ago in the longer thread
> about this function.
Okay.
-Takahiro Akashi
>
> >>
> >>>> +{
> >>>> + __be32 val32;
> >>>> +
> >>>> + while (cells) {
> >>>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >>>> + memcpy(buf, &val32, sizeof(val32));
> >>>> + buf += sizeof(val32);
> >>>
> >>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >>>
> >>> BTW, for purposes of moving to libfdt, we'll need the authors'
> >>> (Masahiro Yamada and Hans de Goede) permission to dual license.
> >>>
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>>> + u64 addr, u64 size)
> >>>> +{
> >>>> + int addr_cells, size_cells;
> >>
> >> unsigned
> >
> > fdt_[address|size]_cell() returns an int.
>
> I stand corrected. They take advantage of the fact that valid values
> are in the range 0..4 and return a negative value for error.
>
>
> >>>> + char buf[sizeof(__be32) * 2 * 2];
> >>>> + /* assume dt_root_[addr|size]_cells <= 2 */
> >>>> + void *prop;
> >>>> + size_t buf_size;
> >>>> +
> >>>> + addr_cells = fdt_address_cells(fdt, 0);
> >>>> + if (addr_cells < 0)
> >>>> + return addr_cells;
> >>>> + size_cells = fdt_size_cells(fdt, 0);
> >>>> + if (size_cells < 0)
> >>>> + return size_cells;
> >>>> +
> >>>> + /* if *_cells >= 2, cells can hold 64-bit values anyway */
> >>>> + if ((addr_cells == 1) && (addr > U32_MAX))
> >>>> + return -FDT_ERR_BADVALUE;
> >>>> +
> >>>> + if ((size_cells == 1) && (size > U32_MAX))
> >>>> + return -FDT_ERR_BADVALUE;
> >>
> >> In v14 I requested:
> >>
> >> Should also check that base + size does not wrap around.
> >
> > Okay, I will start discussion, as you have suggested, in devicetree-spec ML.
> >
> > Thanks,
> > -Takahiro Akashi
> >
> >>
> >>>> +
> >>>> + buf_size = (addr_cells + size_cells) * sizeof(u32);
> >>>> + prop = buf;
> >>>> +
> >>>> + fill_property(prop, addr, addr_cells);
> >>>> + prop += addr_cells * sizeof(u32);
> >>>> +
> >>>> + fill_property(prop, size, size_cells);
> >>>> +
> >>>> + return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> >>>> +}
> >>>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> >>>> index b9cd9ebdf9b9..842af6ea92ea 100644
> >>>> --- a/include/linux/of_fdt.h
> >>>> +++ b/include/linux/of_fdt.h
> >>>> @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> >>>> static inline void unflatten_and_copy_device_tree(void) {}
> >>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>>>
> >>>> +int fdt_prop_len(const char *prop_name, int len);
> >>>> +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> >>>> + u64 addr, u64 size);
> >>>> +
> >>>> #endif /* __ASSEMBLY__ */
> >>>> #endif /* _LINUX_OF_FDT_H */
> >>>> --
> >>>> 2.19.0
> >>>>
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-09-28 13:44 ` Rob Herring
2018-09-28 21:13 ` Frank Rowand
@ 2018-10-02 4:47 ` David Gibson
2018-10-02 9:04 ` AKASHI, Takahiro
[not found] ` <20181005030849.GK32578@linaro.org>
2 siblings, 1 reply; 42+ messages in thread
From: David Gibson @ 2018-10-02 4:47 UTC (permalink / raw)
To: Rob Herring
Cc: AKASHI, Takahiro, Catalin Marinas, Will Deacon, David Howells,
Vivek Goyal, Herbert Xu, David Miller, dyoung, Baoquan He,
Arnd Bergmann, Martin Schwidefsky, Heiko Carstens, prudo,
Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, Frank Rowand, devicetree
[-- Attachment #1: Type: text/plain, Size: 5219 bytes --]
On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> +David Gibson
>
> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > These functions will be used later to handle kexec-specific properties
> > in arm64's kexec_file implementation.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > ---
> > drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/of_fdt.h | 4 +++
> > 2 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 800ad252cf9c..c65c31562ccb 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -25,6 +25,7 @@
> > #include <linux/debugfs.h>
> > #include <linux/serial_core.h>
> > #include <linux/sysfs.h>
> > +#include <linux/types.h>
> >
> > #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> > #include <asm/page.h>
> > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> > #endif
> >
> > #endif /* CONFIG_OF_EARLY_FLATTREE */
> > +
> > +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> > +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> > +
> > +int fdt_prop_len(const char *prop_name, int len)
> > +{
> > + return (strlen(prop_name) + 1) +
> > + sizeof(struct fdt_property) +
> > + FDT_TAGALIGN(len);
>
> Looks like you are using this to calculate how much space you need to
> allocate in addition to the current DTB for a couple of new or
> replaced properties. I'm not sure that this calculation is completely
> accurate. And it is strange there doesn't seem to be any libfdt
> function for this already. It would be simpler to just add some fixed
> additional amount.
>
> Maybe David G has comments on this?
Hrm. Yeah, the approach I'd suggest would be to just add a fixed
amount, and if your functions return -FDT_ERR_NOSPACE, add some more.
That's pretty much the expected way of working with fdts in write
mode.
If that's really not suitable, we could look at adding a helper to
estimate this in libfdt. Or, actually, I suspect taking an existing
libfdt internal helper, polishing and exporting it.
>
> > +}
> > +
>
> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> should go into libfdt, but I'm fine with having it in the kernel for
> now.
>
> > +static void fill_property(void *buf, u64 val64, int cells)
> > +{
> > + __be32 val32;
> > +
> > + while (cells) {
> > + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > + memcpy(buf, &val32, sizeof(val32));
> > + buf += sizeof(val32);
>
> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>
> BTW, for purposes of moving to libfdt, we'll need the authors'
> (Masahiro Yamada and Hans de Goede) permission to dual license.
>
> > + }
> > +}
> > +
> > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > + u64 addr, u64 size)
> > +{
> > + int addr_cells, size_cells;
> > + char buf[sizeof(__be32) * 2 * 2];
> > + /* assume dt_root_[addr|size]_cells <= 2 */
> > + void *prop;
> > + size_t buf_size;
> > +
> > + addr_cells = fdt_address_cells(fdt, 0);
> > + if (addr_cells < 0)
> > + return addr_cells;
> > + size_cells = fdt_size_cells(fdt, 0);
> > + if (size_cells < 0)
> > + return size_cells;
> > +
> > + /* if *_cells >= 2, cells can hold 64-bit values anyway */
> > + if ((addr_cells == 1) && (addr > U32_MAX))
> > + return -FDT_ERR_BADVALUE;
> > +
> > + if ((size_cells == 1) && (size > U32_MAX))
> > + return -FDT_ERR_BADVALUE;
> > +
> > + buf_size = (addr_cells + size_cells) * sizeof(u32);
> > + prop = buf;
> > +
> > + fill_property(prop, addr, addr_cells);
> > + prop += addr_cells * sizeof(u32);
> > +
> > + fill_property(prop, size, size_cells);
> > +
> > + return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> > +}
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index b9cd9ebdf9b9..842af6ea92ea 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> > static inline void unflatten_and_copy_device_tree(void) {}
> > #endif /* CONFIG_OF_EARLY_FLATTREE */
> >
> > +int fdt_prop_len(const char *prop_name, int len);
> > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > + u64 addr, u64 size);
> > +
> > #endif /* __ASSEMBLY__ */
> > #endif /* _LINUX_OF_FDT_H */
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-02 4:47 ` David Gibson
@ 2018-10-02 9:04 ` AKASHI, Takahiro
0 siblings, 0 replies; 42+ messages in thread
From: AKASHI, Takahiro @ 2018-10-02 9:04 UTC (permalink / raw)
To: David Gibson
Cc: Rob Herring, Catalin Marinas, Will Deacon, David Howells,
Vivek Goyal, Herbert Xu, David Miller, dyoung, Baoquan He,
Arnd Bergmann, Martin Schwidefsky, Heiko Carstens, prudo,
Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, Frank Rowand, devicetree
On Tue, Oct 02, 2018 at 02:47:10PM +1000, David Gibson wrote:
> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> > +David Gibson
> >
> > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > These functions will be used later to handle kexec-specific properties
> > > in arm64's kexec_file implementation.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: devicetree@vger.kernel.org
> > > ---
> > > drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/of_fdt.h | 4 +++
> > > 2 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 800ad252cf9c..c65c31562ccb 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > > #include <linux/debugfs.h>
> > > #include <linux/serial_core.h>
> > > #include <linux/sysfs.h>
> > > +#include <linux/types.h>
> > >
> > > #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> > > #include <asm/page.h>
> > > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> > > #endif
> > >
> > > #endif /* CONFIG_OF_EARLY_FLATTREE */
> > > +
> > > +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> > > +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> > > +
> > > +int fdt_prop_len(const char *prop_name, int len)
> > > +{
> > > + return (strlen(prop_name) + 1) +
> > > + sizeof(struct fdt_property) +
> > > + FDT_TAGALIGN(len);
> >
> > Looks like you are using this to calculate how much space you need to
> > allocate in addition to the current DTB for a couple of new or
> > replaced properties. I'm not sure that this calculation is completely
> > accurate. And it is strange there doesn't seem to be any libfdt
> > function for this already. It would be simpler to just add some fixed
> > additional amount.
> >
> > Maybe David G has comments on this?
>
> Hrm. Yeah, the approach I'd suggest would be to just add a fixed
> amount, and if your functions return -FDT_ERR_NOSPACE, add some more.
> That's pretty much the expected way of working with fdts in write
> mode.
I will try to follow this approach.
Thanks,
-Takahiro Akashi
> If that's really not suitable, we could look at adding a helper to
> estimate this in libfdt. Or, actually, I suspect taking an existing
> libfdt internal helper, polishing and exporting it.
>
> >
> > > +}
> > > +
> >
> > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > should go into libfdt, but I'm fine with having it in the kernel for
> > now.
> >
> > > +static void fill_property(void *buf, u64 val64, int cells)
> > > +{
> > > + __be32 val32;
> > > +
> > > + while (cells) {
> > > + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > > + memcpy(buf, &val32, sizeof(val32));
> > > + buf += sizeof(val32);
> >
> > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >
> > BTW, for purposes of moving to libfdt, we'll need the authors'
> > (Masahiro Yamada and Hans de Goede) permission to dual license.
> >
> > > + }
> > > +}
> > > +
> > > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > > + u64 addr, u64 size)
> > > +{
> > > + int addr_cells, size_cells;
> > > + char buf[sizeof(__be32) * 2 * 2];
> > > + /* assume dt_root_[addr|size]_cells <= 2 */
> > > + void *prop;
> > > + size_t buf_size;
> > > +
> > > + addr_cells = fdt_address_cells(fdt, 0);
> > > + if (addr_cells < 0)
> > > + return addr_cells;
> > > + size_cells = fdt_size_cells(fdt, 0);
> > > + if (size_cells < 0)
> > > + return size_cells;
> > > +
> > > + /* if *_cells >= 2, cells can hold 64-bit values anyway */
> > > + if ((addr_cells == 1) && (addr > U32_MAX))
> > > + return -FDT_ERR_BADVALUE;
> > > +
> > > + if ((size_cells == 1) && (size > U32_MAX))
> > > + return -FDT_ERR_BADVALUE;
> > > +
> > > + buf_size = (addr_cells + size_cells) * sizeof(u32);
> > > + prop = buf;
> > > +
> > > + fill_property(prop, addr, addr_cells);
> > > + prop += addr_cells * sizeof(u32);
> > > +
> > > + fill_property(prop, size, size_cells);
> > > +
> > > + return fdt_setprop(fdt, nodeoffset, name, buf, buf_size);
> > > +}
> > > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > > index b9cd9ebdf9b9..842af6ea92ea 100644
> > > --- a/include/linux/of_fdt.h
> > > +++ b/include/linux/of_fdt.h
> > > @@ -108,5 +108,9 @@ static inline void unflatten_device_tree(void) {}
> > > static inline void unflatten_and_copy_device_tree(void) {}
> > > #endif /* CONFIG_OF_EARLY_FLATTREE */
> > >
> > > +int fdt_prop_len(const char *prop_name, int len);
> > > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > > + u64 addr, u64 size);
> > > +
> > > #endif /* __ASSEMBLY__ */
> > > #endif /* _LINUX_OF_FDT_H */
> > >
> >
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 42+ messages in thread
[parent not found: <20181005030849.GK32578@linaro.org>]
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
[not found] ` <20181005030849.GK32578@linaro.org>
@ 2018-10-05 13:23 ` Rob Herring
2018-10-09 0:37 ` AKASHI, Takahiro
0 siblings, 1 reply; 42+ messages in thread
From: Rob Herring @ 2018-10-05 13:23 UTC (permalink / raw)
To: AKASHI, Takahiro, David Gibson, Catalin Marinas, Will Deacon,
David Howells, Vivek Goyal, Herbert Xu, David Miller, dyoung,
Baoquan He, Arnd Bergmann, Martin Schwidefsky, Heiko Carstens,
prudo, Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, Frank Rowand, devicetree
On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Rob,
>
> # I haven't replied to this comment yet.
>
> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> > +David Gibson
> >
> > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > These functions will be used later to handle kexec-specific properties
> > > in arm64's kexec_file implementation.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: devicetree@vger.kernel.org
> > > ---
> > > drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/of_fdt.h | 4 +++
> > > 2 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 800ad252cf9c..c65c31562ccb 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > > #include <linux/debugfs.h>
> > > #include <linux/serial_core.h>
> > > #include <linux/sysfs.h>
> > > +#include <linux/types.h>
> > >
> > > #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> > > #include <asm/page.h>
> > > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> > > #endif
> > >
> > > #endif /* CONFIG_OF_EARLY_FLATTREE */
> > > +
> > > +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> > > +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> > > +
> > > +int fdt_prop_len(const char *prop_name, int len)
> > > +{
> > > + return (strlen(prop_name) + 1) +
> > > + sizeof(struct fdt_property) +
> > > + FDT_TAGALIGN(len);
> >
> > Looks like you are using this to calculate how much space you need to
> > allocate in addition to the current DTB for a couple of new or
> > replaced properties. I'm not sure that this calculation is completely
> > accurate. And it is strange there doesn't seem to be any libfdt
> > function for this already. It would be simpler to just add some fixed
> > additional amount.
> >
> > Maybe David G has comments on this?
> >
> > > +}
> > > +
> >
> > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > should go into libfdt, but I'm fine with having it in the kernel for
> > now.
>
> I'd like to have this function in the kernel for now.
>
> > > +static void fill_property(void *buf, u64 val64, int cells)
> > > +{
> > > + __be32 val32;
> > > +
> > > + while (cells) {
> > > + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > > + memcpy(buf, &val32, sizeof(val32));
> > > + buf += sizeof(val32);
> >
> > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>
> Are you sure?
> I originally implemented this function in a similar way that fdt_pack_reg()
> was, but, you suggested, in your past comment[1], that we'd be better to
> have of_read_number()-like implementation.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
Yeah, you're right. Plus, I'm not sure the u-boot one would work for
unaligned accesses with armv5 and earlier h/w.
My only comment then is I think you can drop the U32_MAX masking.
Rob
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-05 13:23 ` Rob Herring
@ 2018-10-09 0:37 ` AKASHI, Takahiro
2018-10-09 17:47 ` Frank Rowand
0 siblings, 1 reply; 42+ messages in thread
From: AKASHI, Takahiro @ 2018-10-09 0:37 UTC (permalink / raw)
To: Rob Herring
Cc: David Gibson, Catalin Marinas, Will Deacon, David Howells,
Vivek Goyal, Herbert Xu, David Miller, dyoung, Baoquan He,
Arnd Bergmann, Martin Schwidefsky, Heiko Carstens, prudo,
Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, Frank Rowand, devicetree
On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Rob,
> >
> > # I haven't replied to this comment yet.
> >
> > On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> > > +David Gibson
> > >
> > > On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > These functions will be used later to handle kexec-specific properties
> > > > in arm64's kexec_file implementation.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > > Cc: devicetree@vger.kernel.org
> > > > ---
> > > > drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/of_fdt.h | 4 +++
> > > > 2 files changed, 60 insertions(+)
> > > >
> > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > index 800ad252cf9c..c65c31562ccb 100644
> > > > --- a/drivers/of/fdt.c
> > > > +++ b/drivers/of/fdt.c
> > > > @@ -25,6 +25,7 @@
> > > > #include <linux/debugfs.h>
> > > > #include <linux/serial_core.h>
> > > > #include <linux/sysfs.h>
> > > > +#include <linux/types.h>
> > > >
> > > > #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> > > > #include <asm/page.h>
> > > > @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> > > > #endif
> > > >
> > > > #endif /* CONFIG_OF_EARLY_FLATTREE */
> > > > +
> > > > +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> > > > +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> > > > +
> > > > +int fdt_prop_len(const char *prop_name, int len)
> > > > +{
> > > > + return (strlen(prop_name) + 1) +
> > > > + sizeof(struct fdt_property) +
> > > > + FDT_TAGALIGN(len);
> > >
> > > Looks like you are using this to calculate how much space you need to
> > > allocate in addition to the current DTB for a couple of new or
> > > replaced properties. I'm not sure that this calculation is completely
> > > accurate. And it is strange there doesn't seem to be any libfdt
> > > function for this already. It would be simpler to just add some fixed
> > > additional amount.
> > >
> > > Maybe David G has comments on this?
> > >
> > > > +}
> > > > +
> > >
> > > The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> > > should go into libfdt, but I'm fine with having it in the kernel for
> > > now.
> >
> > I'd like to have this function in the kernel for now.
> >
> > > > +static void fill_property(void *buf, u64 val64, int cells)
> > > > +{
> > > > + __be32 val32;
> > > > +
> > > > + while (cells) {
> > > > + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> > > > + memcpy(buf, &val32, sizeof(val32));
> > > > + buf += sizeof(val32);
> > >
> > > This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >
> > Are you sure?
> > I originally implemented this function in a similar way that fdt_pack_reg()
> > was, but, you suggested, in your past comment[1], that we'd be better to
> > have of_read_number()-like implementation.
> >
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
>
> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
> unaligned accesses with armv5 and earlier h/w.
>
> My only comment then is I think you can drop the U32_MAX masking.
Okay, then I will leave this function, yet renaming it to
cpu64_to_fdt_cells() after Frank's comment.
Thanks,
-Takahiro Akashi
>
> Rob
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-09 0:37 ` AKASHI, Takahiro
@ 2018-10-09 17:47 ` Frank Rowand
2018-10-10 1:14 ` AKASHI, Takahiro
0 siblings, 1 reply; 42+ messages in thread
From: Frank Rowand @ 2018-10-09 17:47 UTC (permalink / raw)
To: AKASHI, Takahiro, Rob Herring, David Gibson, Catalin Marinas,
Will Deacon, David Howells, Vivek Goyal, Herbert Xu,
David Miller, dyoung, Baoquan He, Arnd Bergmann,
Martin Schwidefsky, Heiko Carstens, prudo, Ard Biesheuvel,
James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree, Frank Rowand
On 10/08/18 17:37, AKASHI, Takahiro wrote:
> On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
>> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
>> <takahiro.akashi@linaro.org> wrote:
>>>
>>> Rob,
>>>
>>> # I haven't replied to this comment yet.
>>>
>>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
>>>> +David Gibson
>>>>
>>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
>>>> <takahiro.akashi@linaro.org> wrote:
>>>>>
>>>>> These functions will be used later to handle kexec-specific properties
>>>>> in arm64's kexec_file implementation.
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>>> Cc: devicetree@vger.kernel.org
>>>>> ---
>>>>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>> include/linux/of_fdt.h | 4 +++
>>>>> 2 files changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>> index 800ad252cf9c..c65c31562ccb 100644
>>>>> --- a/drivers/of/fdt.c
>>>>> +++ b/drivers/of/fdt.c
>>>>> @@ -25,6 +25,7 @@
>>>>> #include <linux/debugfs.h>
>>>>> #include <linux/serial_core.h>
>>>>> #include <linux/sysfs.h>
>>>>> +#include <linux/types.h>
>>>>>
>>>>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
>>>>> #include <asm/page.h>
>>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>>>> #endif
>>>>>
>>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>> +
>>>>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
>>>>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
>>>>> +
>>>>> +int fdt_prop_len(const char *prop_name, int len)
>>>>> +{
>>>>> + return (strlen(prop_name) + 1) +
>>>>> + sizeof(struct fdt_property) +
>>>>> + FDT_TAGALIGN(len);
>>>>
>>>> Looks like you are using this to calculate how much space you need to
>>>> allocate in addition to the current DTB for a couple of new or
>>>> replaced properties. I'm not sure that this calculation is completely
>>>> accurate. And it is strange there doesn't seem to be any libfdt
>>>> function for this already. It would be simpler to just add some fixed
>>>> additional amount.
>>>>
>>>> Maybe David G has comments on this?
>>>>
>>>>> +}
>>>>> +
>>>>
>>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
>>>> should go into libfdt, but I'm fine with having it in the kernel for
>>>> now.
>>>
>>> I'd like to have this function in the kernel for now.
>>>
>>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>>>> +{
>>>>> + __be32 val32;
>>>>> +
>>>>> + while (cells) {
>>>>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>>> + memcpy(buf, &val32, sizeof(val32));
>>>>> + buf += sizeof(val32);
>>>>
>>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>>>
>>> Are you sure?
>>> I originally implemented this function in a similar way that fdt_pack_reg()
>>> was, but, you suggested, in your past comment[1], that we'd be better to
>>> have of_read_number()-like implementation.
>>>
>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
>>
>> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
>> unaligned accesses with armv5 and earlier h/w.
>>
>> My only comment then is I think you can drop the U32_MAX masking.
>
> Okay, then I will leave this function, yet renaming it to
> cpu64_to_fdt_cells() after Frank's comment.
I have second guessed myself and do not like the name I suggested
because what the function really does is either cpu32 to be32 or
cpu64 to be64.
I agree with Rob that readability is important here. Instead of
having a fill_property() function, how about having inline code,
something like (untested even for thinkos):
prop = buf;
if (addr_cells == 1) {
*(__be32 *)prop = cpu32_to_be32(addr);
prop += 4;
} else {
*(__be64 *)prop = cpu64_to_be64(addr);
prop += 8;
}
if (size_cells == 1)
*(__be32 *)prop = cpu32_to_be32(size);
else
*(__be64 *)prop = cpu64_to_be64(size);
You might want to also give Rob a chance to bike shed on this
suggestion.
-Frank
>
> Thanks,
> -Takahiro Akashi
>>
>> Rob
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-09 17:47 ` Frank Rowand
@ 2018-10-10 1:14 ` AKASHI, Takahiro
2018-10-10 2:44 ` Frank Rowand
0 siblings, 1 reply; 42+ messages in thread
From: AKASHI, Takahiro @ 2018-10-10 1:14 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, David Gibson, Catalin Marinas, Will Deacon,
David Howells, Vivek Goyal, Herbert Xu, David Miller, dyoung,
Baoquan He, Arnd Bergmann, Martin Schwidefsky, Heiko Carstens,
prudo, Ard Biesheuvel, James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
Frank, Rob,
On Tue, Oct 09, 2018 at 10:47:15AM -0700, Frank Rowand wrote:
> On 10/08/18 17:37, AKASHI, Takahiro wrote:
> > On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
> >> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
> >> <takahiro.akashi@linaro.org> wrote:
> >>>
> >>> Rob,
> >>>
> >>> # I haven't replied to this comment yet.
> >>>
> >>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
> >>>> +David Gibson
> >>>>
> >>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
> >>>> <takahiro.akashi@linaro.org> wrote:
> >>>>>
> >>>>> These functions will be used later to handle kexec-specific properties
> >>>>> in arm64's kexec_file implementation.
> >>>>>
> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>> Cc: Rob Herring <robh+dt@kernel.org>
> >>>>> Cc: Frank Rowand <frowand.list@gmail.com>
> >>>>> Cc: devicetree@vger.kernel.org
> >>>>> ---
> >>>>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> >>>>> include/linux/of_fdt.h | 4 +++
> >>>>> 2 files changed, 60 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>>>> index 800ad252cf9c..c65c31562ccb 100644
> >>>>> --- a/drivers/of/fdt.c
> >>>>> +++ b/drivers/of/fdt.c
> >>>>> @@ -25,6 +25,7 @@
> >>>>> #include <linux/debugfs.h>
> >>>>> #include <linux/serial_core.h>
> >>>>> #include <linux/sysfs.h>
> >>>>> +#include <linux/types.h>
> >>>>>
> >>>>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
> >>>>> #include <asm/page.h>
> >>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
> >>>>> #endif
> >>>>>
> >>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
> >>>>> +
> >>>>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> >>>>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
> >>>>> +
> >>>>> +int fdt_prop_len(const char *prop_name, int len)
> >>>>> +{
> >>>>> + return (strlen(prop_name) + 1) +
> >>>>> + sizeof(struct fdt_property) +
> >>>>> + FDT_TAGALIGN(len);
> >>>>
> >>>> Looks like you are using this to calculate how much space you need to
> >>>> allocate in addition to the current DTB for a couple of new or
> >>>> replaced properties. I'm not sure that this calculation is completely
> >>>> accurate. And it is strange there doesn't seem to be any libfdt
> >>>> function for this already. It would be simpler to just add some fixed
> >>>> additional amount.
> >>>>
> >>>> Maybe David G has comments on this?
> >>>>
> >>>>> +}
> >>>>> +
> >>>>
> >>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
> >>>> should go into libfdt, but I'm fine with having it in the kernel for
> >>>> now.
> >>>
> >>> I'd like to have this function in the kernel for now.
> >>>
> >>>>> +static void fill_property(void *buf, u64 val64, int cells)
> >>>>> +{
> >>>>> + __be32 val32;
> >>>>> +
> >>>>> + while (cells) {
> >>>>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
> >>>>> + memcpy(buf, &val32, sizeof(val32));
> >>>>> + buf += sizeof(val32);
> >>>>
> >>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
> >>>
> >>> Are you sure?
> >>> I originally implemented this function in a similar way that fdt_pack_reg()
> >>> was, but, you suggested, in your past comment[1], that we'd be better to
> >>> have of_read_number()-like implementation.
> >>>
> >>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
> >>
> >> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
> >> unaligned accesses with armv5 and earlier h/w.
> >>
> >> My only comment then is I think you can drop the U32_MAX masking.
> >
> > Okay, then I will leave this function, yet renaming it to
> > cpu64_to_fdt_cells() after Frank's comment.
>
> I have second guessed myself and do not like the name I suggested
> because what the function really does is either cpu32 to be32 or
> cpu64 to be64.
Okay.
> I agree with Rob that readability is important here. Instead of
> having a fill_property() function, how about having inline code,
> something like (untested even for thinkos):
>
> prop = buf;
>
> if (addr_cells == 1) {
> *(__be32 *)prop = cpu32_to_be32(addr);
> prop += 4;
> } else {
> *(__be64 *)prop = cpu64_to_be64(addr);
> prop += 8;
> }
>
> if (size_cells == 1)
> *(__be32 *)prop = cpu32_to_be32(size);
> else
> *(__be64 *)prop = cpu64_to_be64(size);
>
> You might want to also give Rob a chance to bike shed on this
> suggestion.
Basically, I don't care either way, but what Rob suggested
is that some architecture(s) might not handle correctly
unaligned memory access here.
I just want to stay tuned with Rob.
Thanks,
-Takahiro Akashi
> -Frank
>
> >
> > Thanks,
> > -Takahiro Akashi
> >>
> >> Rob
> >
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 06/16] of/fdt: add helper functions for handling properties
2018-10-10 1:14 ` AKASHI, Takahiro
@ 2018-10-10 2:44 ` Frank Rowand
0 siblings, 0 replies; 42+ messages in thread
From: Frank Rowand @ 2018-10-10 2:44 UTC (permalink / raw)
To: AKASHI, Takahiro, Rob Herring, David Gibson, Catalin Marinas,
Will Deacon, David Howells, Vivek Goyal, Herbert Xu,
David Miller, dyoung, Baoquan He, Arnd Bergmann,
Martin Schwidefsky, Heiko Carstens, prudo, Ard Biesheuvel,
James Morse, bhsharma, kexec,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel, devicetree
On 10/09/18 18:14, AKASHI, Takahiro wrote:
> Frank, Rob,
>
> On Tue, Oct 09, 2018 at 10:47:15AM -0700, Frank Rowand wrote:
>> On 10/08/18 17:37, AKASHI, Takahiro wrote:
>>> On Fri, Oct 05, 2018 at 08:23:57AM -0500, Rob Herring wrote:
>>>> On Thu, Oct 4, 2018 at 10:07 PM AKASHI, Takahiro
>>>> <takahiro.akashi@linaro.org> wrote:
>>>>>
>>>>> Rob,
>>>>>
>>>>> # I haven't replied to this comment yet.
>>>>>
>>>>> On Fri, Sep 28, 2018 at 08:44:42AM -0500, Rob Herring wrote:
>>>>>> +David Gibson
>>>>>>
>>>>>> On Fri, Sep 28, 2018 at 1:48 AM AKASHI Takahiro
>>>>>> <takahiro.akashi@linaro.org> wrote:
>>>>>>>
>>>>>>> These functions will be used later to handle kexec-specific properties
>>>>>>> in arm64's kexec_file implementation.
>>>>>>>
>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>> ---
>>>>>>> drivers/of/fdt.c | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>> include/linux/of_fdt.h | 4 +++
>>>>>>> 2 files changed, 60 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>>>>>> index 800ad252cf9c..c65c31562ccb 100644
>>>>>>> --- a/drivers/of/fdt.c
>>>>>>> +++ b/drivers/of/fdt.c
>>>>>>> @@ -25,6 +25,7 @@
>>>>>>> #include <linux/debugfs.h>
>>>>>>> #include <linux/serial_core.h>
>>>>>>> #include <linux/sysfs.h>
>>>>>>> +#include <linux/types.h>
>>>>>>>
>>>>>>> #include <asm/setup.h> /* for COMMAND_LINE_SIZE */
>>>>>>> #include <asm/page.h>
>>>>>>> @@ -1323,3 +1324,58 @@ late_initcall(of_fdt_raw_init);
>>>>>>> #endif
>>>>>>>
>>>>>>> #endif /* CONFIG_OF_EARLY_FLATTREE */
>>>>>>> +
>>>>>>> +#define FDT_ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
>>>>>>> +#define FDT_TAGALIGN(x) (FDT_ALIGN((x), FDT_TAGSIZE))
>>>>>>> +
>>>>>>> +int fdt_prop_len(const char *prop_name, int len)
>>>>>>> +{
>>>>>>> + return (strlen(prop_name) + 1) +
>>>>>>> + sizeof(struct fdt_property) +
>>>>>>> + FDT_TAGALIGN(len);
>>>>>>
>>>>>> Looks like you are using this to calculate how much space you need to
>>>>>> allocate in addition to the current DTB for a couple of new or
>>>>>> replaced properties. I'm not sure that this calculation is completely
>>>>>> accurate. And it is strange there doesn't seem to be any libfdt
>>>>>> function for this already. It would be simpler to just add some fixed
>>>>>> additional amount.
>>>>>>
>>>>>> Maybe David G has comments on this?
>>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> The rest of this should go in drivers/of/fdt_address.c. Ultimately, it
>>>>>> should go into libfdt, but I'm fine with having it in the kernel for
>>>>>> now.
>>>>>
>>>>> I'd like to have this function in the kernel for now.
>>>>>
>>>>>>> +static void fill_property(void *buf, u64 val64, int cells)
>>>>>>> +{
>>>>>>> + __be32 val32;
>>>>>>> +
>>>>>>> + while (cells) {
>>>>>>> + val32 = cpu_to_fdt32((val64 >> (32 * (--cells))) & U32_MAX);
>>>>>>> + memcpy(buf, &val32, sizeof(val32));
>>>>>>> + buf += sizeof(val32);
>>>>>>
>>>>>> This is kind of hard to read. I would copy u-boot's fdt_pack_reg function.
>>>>>
>>>>> Are you sure?
>>>>> I originally implemented this function in a similar way that fdt_pack_reg()
>>>>> was, but, you suggested, in your past comment[1], that we'd be better to
>>>>> have of_read_number()-like implementation.
>>>>>
>>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-May/579118.html
>>>>
>>>> Yeah, you're right. Plus, I'm not sure the u-boot one would work for
>>>> unaligned accesses with armv5 and earlier h/w.
>>>>
>>>> My only comment then is I think you can drop the U32_MAX masking.
>>>
>>> Okay, then I will leave this function, yet renaming it to
>>> cpu64_to_fdt_cells() after Frank's comment.
>>
>> I have second guessed myself and do not like the name I suggested
>> because what the function really does is either cpu32 to be32 or
>> cpu64 to be64.
>
> Okay.
>
>> I agree with Rob that readability is important here. Instead of
>> having a fill_property() function, how about having inline code,
>> something like (untested even for thinkos):
>>
>> prop = buf;
>>
>> if (addr_cells == 1) {
>> *(__be32 *)prop = cpu32_to_be32(addr);
>> prop += 4;
>> } else {
>> *(__be64 *)prop = cpu64_to_be64(addr);
>> prop += 8;
>> }
>>
>> if (size_cells == 1)
>> *(__be32 *)prop = cpu32_to_be32(size);
>> else
>> *(__be64 *)prop = cpu64_to_be64(size);
>>
>> You might want to also give Rob a chance to bike shed on this
>> suggestion.
>
> Basically, I don't care either way, but what Rob suggested
> is that some architecture(s) might not handle correctly
> unaligned memory access here.
Good point about unaligned memory access. That rules out my
suggestion above. But if using the fill_property() implementation,
either come up with a name that better describes what the function
does or add a function header comment that gives the reader a
clue as to what the function accomplishes.
-Frank
> I just want to stay tuned with Rob.
>
> Thanks,
> -Takahiro Akashi
>
>> -Frank
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>>
>>>> Rob
>>>
>>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 07/16] arm64: add image head flag definitions
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (5 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 06/16] of/fdt: add helper functions for handling properties AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-10-01 12:52 ` Mark Rutland
2018-09-28 6:48 ` [PATCH v15 08/16] arm64: cpufeature: add MMFR0 helper functions AKASHI Takahiro
` (8 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
Those image head's flags will be used later by kexec_file loader.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Acked-by: James Morse <james.morse@arm.com>
---
arch/arm64/include/asm/boot.h | 15 +++++++++++++++
arch/arm64/kernel/head.S | 2 +-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/boot.h b/arch/arm64/include/asm/boot.h
index 355e552a9175..0bab7eed3012 100644
--- a/arch/arm64/include/asm/boot.h
+++ b/arch/arm64/include/asm/boot.h
@@ -5,6 +5,21 @@
#include <asm/sizes.h>
+#define ARM64_MAGIC "ARM\x64"
+
+#define HEAD_FLAG_BE_SHIFT 0
+#define HEAD_FLAG_PAGE_SIZE_SHIFT 1
+#define HEAD_FLAG_BE_MASK 0x1
+#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
+
+#define HEAD_FLAG_BE 1
+#define HEAD_FLAG_PAGE_SIZE_4K 1
+#define HEAD_FLAG_PAGE_SIZE_16K 2
+#define HEAD_FLAG_PAGE_SIZE_64K 3
+
+#define head_flag_field(flags, field) \
+ (((flags) >> field##_SHIFT) & field##_MASK)
+
/*
* arm64 requires the DTB to be 8 byte aligned and
* not exceed 2MB in size.
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index b0853069702f..8cbac6232ed1 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -91,7 +91,7 @@ _head:
.quad 0 // reserved
.quad 0 // reserved
.quad 0 // reserved
- .ascii "ARM\x64" // Magic number
+ .ascii ARM64_MAGIC // Magic number
#ifdef CONFIG_EFI
.long pe_header - _head // Offset to the PE header.
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v15 07/16] arm64: add image head flag definitions
2018-09-28 6:48 ` [PATCH v15 07/16] arm64: add image head flag definitions AKASHI Takahiro
@ 2018-10-01 12:52 ` Mark Rutland
2018-10-02 7:59 ` AKASHI Takahiro
0 siblings, 1 reply; 42+ messages in thread
From: Mark Rutland @ 2018-10-01 12:52 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens, prudo,
ard.biesheuvel, james.morse, bhsharma, kexec, linux-arm-kernel,
linux-kernel
On Fri, Sep 28, 2018 at 03:48:32PM +0900, AKASHI Takahiro wrote:
> Those image head's flags will be used later by kexec_file loader.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Acked-by: James Morse <james.morse@arm.com>
> ---
> arch/arm64/include/asm/boot.h | 15 +++++++++++++++
> arch/arm64/kernel/head.S | 2 +-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/boot.h b/arch/arm64/include/asm/boot.h
> index 355e552a9175..0bab7eed3012 100644
> --- a/arch/arm64/include/asm/boot.h
> +++ b/arch/arm64/include/asm/boot.h
> @@ -5,6 +5,21 @@
>
> #include <asm/sizes.h>
>
> +#define ARM64_MAGIC "ARM\x64"
> +
> +#define HEAD_FLAG_BE_SHIFT 0
> +#define HEAD_FLAG_PAGE_SIZE_SHIFT 1
> +#define HEAD_FLAG_BE_MASK 0x1
> +#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
> +
> +#define HEAD_FLAG_BE 1
These already exist in some form in arch/arm64/kernel/image.h; can we
please factor those out rather than duplicating them?
I'd be happy if you'd update image.h to use the new HEAD_FLAG_* names,
and removed the old definitions.
> +#define HEAD_FLAG_PAGE_SIZE_4K 1
> +#define HEAD_FLAG_PAGE_SIZE_16K 2
> +#define HEAD_FLAG_PAGE_SIZE_64K 3
I appreciate these are new, since we didn't previously need definitions.
Thanks,
Mark.
> +
> +#define head_flag_field(flags, field) \
> + (((flags) >> field##_SHIFT) & field##_MASK)
> +
> /*
> * arm64 requires the DTB to be 8 byte aligned and
> * not exceed 2MB in size.
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index b0853069702f..8cbac6232ed1 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -91,7 +91,7 @@ _head:
> .quad 0 // reserved
> .quad 0 // reserved
> .quad 0 // reserved
> - .ascii "ARM\x64" // Magic number
> + .ascii ARM64_MAGIC // Magic number
> #ifdef CONFIG_EFI
> .long pe_header - _head // Offset to the PE header.
>
> --
> 2.19.0
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 07/16] arm64: add image head flag definitions
2018-10-01 12:52 ` Mark Rutland
@ 2018-10-02 7:59 ` AKASHI Takahiro
2018-10-09 15:04 ` Mark Rutland
0 siblings, 1 reply; 42+ messages in thread
From: AKASHI Takahiro @ 2018-10-02 7:59 UTC (permalink / raw)
To: Mark Rutland
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens, prudo,
ard.biesheuvel, james.morse, bhsharma, kexec, linux-arm-kernel,
linux-kernel
Hi Mark,
On Mon, Oct 01, 2018 at 01:52:26PM +0100, Mark Rutland wrote:
> On Fri, Sep 28, 2018 at 03:48:32PM +0900, AKASHI Takahiro wrote:
> > Those image head's flags will be used later by kexec_file loader.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Acked-by: James Morse <james.morse@arm.com>
> > ---
> > arch/arm64/include/asm/boot.h | 15 +++++++++++++++
> > arch/arm64/kernel/head.S | 2 +-
> > 2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/boot.h b/arch/arm64/include/asm/boot.h
> > index 355e552a9175..0bab7eed3012 100644
> > --- a/arch/arm64/include/asm/boot.h
> > +++ b/arch/arm64/include/asm/boot.h
> > @@ -5,6 +5,21 @@
> >
> > #include <asm/sizes.h>
> >
> > +#define ARM64_MAGIC "ARM\x64"
> > +
> > +#define HEAD_FLAG_BE_SHIFT 0
> > +#define HEAD_FLAG_PAGE_SIZE_SHIFT 1
> > +#define HEAD_FLAG_BE_MASK 0x1
> > +#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
> > +
> > +#define HEAD_FLAG_BE 1
>
> These already exist in some form in arch/arm64/kernel/image.h; can we
> please factor those out rather than duplicating them?
Sure.
> I'd be happy if you'd update image.h to use the new HEAD_FLAG_* names,
> and removed the old definitions.
I want to make sure two things;
1. Do you assume all the existing __HEAD_FLAG_xyz's NOT be renamed
(say, to HEAD_FLAG_xyz)?
2. Do you mind removing this check in image.h?
(we also need to manage 'CONFIG_EFI' part of image.h.)
> #ifndef LINKER_SCRIPT
> #error This file should only be included in vmlinux.lds.S
> #endif
Thanks,
-Takahiro Akashi
> > +#define HEAD_FLAG_PAGE_SIZE_4K 1
> > +#define HEAD_FLAG_PAGE_SIZE_16K 2
> > +#define HEAD_FLAG_PAGE_SIZE_64K 3
>
> I appreciate these are new, since we didn't previously need definitions.
>
> Thanks,
> Mark.
>
> > +
> > +#define head_flag_field(flags, field) \
> > + (((flags) >> field##_SHIFT) & field##_MASK)
> > +
> > /*
> > * arm64 requires the DTB to be 8 byte aligned and
> > * not exceed 2MB in size.
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index b0853069702f..8cbac6232ed1 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -91,7 +91,7 @@ _head:
> > .quad 0 // reserved
> > .quad 0 // reserved
> > .quad 0 // reserved
> > - .ascii "ARM\x64" // Magic number
> > + .ascii ARM64_MAGIC // Magic number
> > #ifdef CONFIG_EFI
> > .long pe_header - _head // Offset to the PE header.
> >
> > --
> > 2.19.0
> >
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 07/16] arm64: add image head flag definitions
2018-10-02 7:59 ` AKASHI Takahiro
@ 2018-10-09 15:04 ` Mark Rutland
2018-10-10 1:59 ` AKASHI Takahiro
0 siblings, 1 reply; 42+ messages in thread
From: Mark Rutland @ 2018-10-09 15:04 UTC (permalink / raw)
To: AKASHI Takahiro, catalin.marinas, will.deacon, dhowells, vgoyal,
herbert, davem, dyoung, bhe, arnd, schwidefsky, heiko.carstens,
prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel
On Tue, Oct 02, 2018 at 04:59:40PM +0900, AKASHI Takahiro wrote:
> Hi Mark,
>
> On Mon, Oct 01, 2018 at 01:52:26PM +0100, Mark Rutland wrote:
> > On Fri, Sep 28, 2018 at 03:48:32PM +0900, AKASHI Takahiro wrote:
> > > Those image head's flags will be used later by kexec_file loader.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Acked-by: James Morse <james.morse@arm.com>
> > > ---
> > > arch/arm64/include/asm/boot.h | 15 +++++++++++++++
> > > arch/arm64/kernel/head.S | 2 +-
> > > 2 files changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/include/asm/boot.h b/arch/arm64/include/asm/boot.h
> > > index 355e552a9175..0bab7eed3012 100644
> > > --- a/arch/arm64/include/asm/boot.h
> > > +++ b/arch/arm64/include/asm/boot.h
> > > @@ -5,6 +5,21 @@
> > >
> > > #include <asm/sizes.h>
> > >
> > > +#define ARM64_MAGIC "ARM\x64"
> > > +
> > > +#define HEAD_FLAG_BE_SHIFT 0
> > > +#define HEAD_FLAG_PAGE_SIZE_SHIFT 1
> > > +#define HEAD_FLAG_BE_MASK 0x1
> > > +#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
> > > +
> > > +#define HEAD_FLAG_BE 1
> >
> > These already exist in some form in arch/arm64/kernel/image.h; can we
> > please factor those out rather than duplicating them?
>
> Sure.
>
> > I'd be happy if you'd update image.h to use the new HEAD_FLAG_* names,
> > and removed the old definitions.
>
> I want to make sure two things;
>
> 1. Do you assume all the existing __HEAD_FLAG_xyz's NOT be renamed
> (say, to HEAD_FLAG_xyz)?
I'm perfectly happy for them to be renamed, I just don't want duplicate
definitions.
Let's rename them to ARM64_IMAGE_FLAG_<foo>, and place them in a new
header, arch/arm64/include/asm/image.h, which arch/arm64/kernel/image.h
can include and make use of.
>
> 2. Do you mind removing this check in image.h?
> (we also need to manage 'CONFIG_EFI' part of image.h.)
What exactly do we need from that? AFAICT that's all linker script
stuff that shouldn't matter for kexec.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 07/16] arm64: add image head flag definitions
2018-10-09 15:04 ` Mark Rutland
@ 2018-10-10 1:59 ` AKASHI Takahiro
0 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-10-10 1:59 UTC (permalink / raw)
To: Mark Rutland
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens, prudo,
ard.biesheuvel, james.morse, bhsharma, kexec, linux-arm-kernel,
linux-kernel
Mark,
On Tue, Oct 09, 2018 at 04:04:05PM +0100, Mark Rutland wrote:
> On Tue, Oct 02, 2018 at 04:59:40PM +0900, AKASHI Takahiro wrote:
> > Hi Mark,
> >
> > On Mon, Oct 01, 2018 at 01:52:26PM +0100, Mark Rutland wrote:
> > > On Fri, Sep 28, 2018 at 03:48:32PM +0900, AKASHI Takahiro wrote:
> > > > Those image head's flags will be used later by kexec_file loader.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Will Deacon <will.deacon@arm.com>
> > > > Acked-by: James Morse <james.morse@arm.com>
> > > > ---
> > > > arch/arm64/include/asm/boot.h | 15 +++++++++++++++
> > > > arch/arm64/kernel/head.S | 2 +-
> > > > 2 files changed, 16 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/boot.h b/arch/arm64/include/asm/boot.h
> > > > index 355e552a9175..0bab7eed3012 100644
> > > > --- a/arch/arm64/include/asm/boot.h
> > > > +++ b/arch/arm64/include/asm/boot.h
> > > > @@ -5,6 +5,21 @@
> > > >
> > > > #include <asm/sizes.h>
> > > >
> > > > +#define ARM64_MAGIC "ARM\x64"
> > > > +
> > > > +#define HEAD_FLAG_BE_SHIFT 0
> > > > +#define HEAD_FLAG_PAGE_SIZE_SHIFT 1
> > > > +#define HEAD_FLAG_BE_MASK 0x1
> > > > +#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
> > > > +
> > > > +#define HEAD_FLAG_BE 1
> > >
> > > These already exist in some form in arch/arm64/kernel/image.h; can we
> > > please factor those out rather than duplicating them?
> >
> > Sure.
> >
> > > I'd be happy if you'd update image.h to use the new HEAD_FLAG_* names,
> > > and removed the old definitions.
> >
> > I want to make sure two things;
> >
> > 1. Do you assume all the existing __HEAD_FLAG_xyz's NOT be renamed
> > (say, to HEAD_FLAG_xyz)?
>
> I'm perfectly happy for them to be renamed, I just don't want duplicate
> definitions.
IMO, it's not duplication.
HEAD_FLAG_xyz is a definition of a specific field. On the other hand,
__HEAD_FLAG_xyz is a value to be put in a kernel header.
To be clear, please look at my current code attached below.
> Let's rename them to ARM64_IMAGE_FLAG_<foo>, and place them in a new
> header, arch/arm64/include/asm/image.h, which arch/arm64/kernel/image.h
> can include and make use of.
Okay.
> > 2. Do you mind removing this check in image.h?
> > (we also need to manage 'CONFIG_EFI' part of image.h.)
>
> What exactly do we need from that? AFAICT that's all linker script
> stuff that shouldn't matter for kexec.
You're right if all the definitions, as you suggested above,
are moved to a new header, asm/image.h.
-Takahiro Akashi
> Thanks,
> Mark.
arch/arm64/kernel/image.h
---8<---
/*
* Linker script macros to generate Image header fields.
*
* Copyright (C) 2014 ARM Ltd.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
#ifndef __ASM_IMAGE_H
#define __ASM_IMAGE_H
#define HEAD_FLAG_BE_SHIFT 0
#define HEAD_FLAG_PAGE_SIZE_SHIFT (HEAD_FLAG_BE_SHIFT + 1)
#define HEAD_FLAG_PHYS_BASE_SHIFT (HEAD_FLAG_PAGE_SIZE_SHIFT + 2)
#define HEAD_FLAG_BE_MASK 0x1
#define HEAD_FLAG_PAGE_SIZE_MASK 0x3
#define HEAD_FLAG_PHYS_BASE_MASK 0x1
#define HEAD_FLAG_LE 0
#define HEAD_FLAG_BE 1
#define HEAD_FLAG_PAGE_SIZE_4K 1
#define HEAD_FLAG_PAGE_SIZE_16K 2
#define HEAD_FLAG_PAGE_SIZE_64K 3
#define HEAD_FLAG_PHYS_BASE 1
#define head_flag_field(flags, field) \
(((flags) >> field##_SHIFT) & field##_MASK)
#ifdef LINKER_SCRIPT
/*
* There aren't any ELF relocations we can use to endian-swap values known only
* at link time (e.g. the subtraction of two symbol addresses), so we must get
* the linker to endian-swap certain values before emitting them.
*
* Note that, in order for this to work when building the ELF64 PIE executable
* (for KASLR), these values should not be referenced via R_AARCH64_ABS64
* relocations, since these are fixed up at runtime rather than at build time
* when PIE is in effect. So we need to split them up in 32-bit high and low
* words.
*/
#ifdef CONFIG_CPU_BIG_ENDIAN
#define DATA_LE32(data) \
((((data) & 0x000000ff) << 24) | \
(((data) & 0x0000ff00) << 8) | \
(((data) & 0x00ff0000) >> 8) | \
(((data) & 0xff000000) >> 24))
#else
#define DATA_LE32(data) ((data) & 0xffffffff)
#endif
#define DEFINE_IMAGE_LE64(sym, data) \
sym##_lo32 = DATA_LE32((data) & 0xffffffff); \
sym##_hi32 = DATA_LE32((data) >> 32)
#define __HEAD_FLAG(field) (__HEAD_FLAG_##field << \
HEAD_FLAG_##field##_SHIFT)
#ifdef CONFIG_CPU_BIG_ENDIAN
#define __HEAD_FLAG_BE HEAD_FLAG_BE
#else
#define __HEAD_FLAG_BE HEAD_FLAG_LE
#endif
#define __HEAD_FLAG_PAGE_SIZE ((PAGE_SHIFT - 10) / 2)
#define __HEAD_FLAG_PHYS_BASE HEAD_FLAG_PHYS_BASE
#define __HEAD_FLAGS (__HEAD_FLAG(BE) | \
__HEAD_FLAG(PAGE_SIZE) | \
__HEAD_FLAG(PHYS_BASE))
/*
* These will output as part of the Image header, which should be little-endian
* regardless of the endianness of the kernel. While constant values could be
* endian swapped in head.S, all are done here for consistency.
*/
#define HEAD_SYMBOLS \
DEFINE_IMAGE_LE64(_kernel_size_le, _end - _text); \
DEFINE_IMAGE_LE64(_kernel_offset_le, TEXT_OFFSET); \
DEFINE_IMAGE_LE64(_kernel_flags_le, __HEAD_FLAGS);
#ifdef CONFIG_EFI
__efistub_stext_offset = stext - _text;
/*
* Prevent the symbol aliases below from being emitted into the kallsyms
* table, by forcing them to be absolute symbols (which are conveniently
* ignored by scripts/kallsyms) rather than section relative symbols.
* The distinction is only relevant for partial linking, and only for symbols
* that are defined within a section declaration (which is not the case for
* the definitions below) so the resulting values will be identical.
*/
#define KALLSYMS_HIDE(sym) ABSOLUTE(sym)
/*
* The EFI stub has its own symbol namespace prefixed by __efistub_, to
* isolate it from the kernel proper. The following symbols are legally
* accessed by the stub, so provide some aliases to make them accessible.
* Only include data symbols here, or text symbols of functions that are
* guaranteed to be safe when executed at another offset than they were
* linked at. The routines below are all implemented in assembler in a
* position independent manner
*/
__efistub_memcmp = KALLSYMS_HIDE(__pi_memcmp);
__efistub_memchr = KALLSYMS_HIDE(__pi_memchr);
__efistub_memcpy = KALLSYMS_HIDE(__pi_memcpy);
__efistub_memmove = KALLSYMS_HIDE(__pi_memmove);
__efistub_memset = KALLSYMS_HIDE(__pi_memset);
__efistub_strlen = KALLSYMS_HIDE(__pi_strlen);
__efistub_strnlen = KALLSYMS_HIDE(__pi_strnlen);
__efistub_strcmp = KALLSYMS_HIDE(__pi_strcmp);
__efistub_strncmp = KALLSYMS_HIDE(__pi_strncmp);
__efistub_strrchr = KALLSYMS_HIDE(__pi_strrchr);
__efistub___flush_dcache_area = KALLSYMS_HIDE(__pi___flush_dcache_area);
#ifdef CONFIG_KASAN
__efistub___memcpy = KALLSYMS_HIDE(__pi_memcpy);
__efistub___memmove = KALLSYMS_HIDE(__pi_memmove);
__efistub___memset = KALLSYMS_HIDE(__pi_memset);
#endif
__efistub__text = KALLSYMS_HIDE(_text);
__efistub__end = KALLSYMS_HIDE(_end);
__efistub__edata = KALLSYMS_HIDE(_edata);
__efistub_screen_info = KALLSYMS_HIDE(screen_info);
#endif /* CONFIG_EFI */
#endif /* LINKER_SCRIPT */
#endif /* __ASM_IMAGE_H */
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 08/16] arm64: cpufeature: add MMFR0 helper functions
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (6 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 07/16] arm64: add image head flag definitions AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 09/16] arm64: enable KEXEC_FILE config AKASHI Takahiro
` (7 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
Those helper functions for MMFR0 register will be used later by kexec_file
loader.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 48 +++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 1717ba1db35d..cd90b5252d6d 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -486,11 +486,59 @@ static inline bool system_supports_32bit_el0(void)
return cpus_have_const_cap(ARM64_HAS_32BIT_EL0);
}
+static inline bool system_supports_4kb_granule(void)
+{
+ u64 mmfr0;
+ u32 val;
+
+ mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+ val = cpuid_feature_extract_unsigned_field(mmfr0,
+ ID_AA64MMFR0_TGRAN4_SHIFT);
+
+ return val == ID_AA64MMFR0_TGRAN4_SUPPORTED;
+}
+
+static inline bool system_supports_64kb_granule(void)
+{
+ u64 mmfr0;
+ u32 val;
+
+ mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+ val = cpuid_feature_extract_unsigned_field(mmfr0,
+ ID_AA64MMFR0_TGRAN64_SHIFT);
+
+ return val == ID_AA64MMFR0_TGRAN64_SUPPORTED;
+}
+
+static inline bool system_supports_16kb_granule(void)
+{
+ u64 mmfr0;
+ u32 val;
+
+ mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+ val = cpuid_feature_extract_unsigned_field(mmfr0,
+ ID_AA64MMFR0_TGRAN16_SHIFT);
+
+ return val == ID_AA64MMFR0_TGRAN16_SUPPORTED;
+}
+
static inline bool system_supports_mixed_endian_el0(void)
{
return id_aa64mmfr0_mixed_endian_el0(read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1));
}
+static inline bool system_supports_mixed_endian(void)
+{
+ u64 mmfr0;
+ u32 val;
+
+ mmfr0 = read_sanitised_ftr_reg(SYS_ID_AA64MMFR0_EL1);
+ val = cpuid_feature_extract_unsigned_field(mmfr0,
+ ID_AA64MMFR0_BIGENDEL_SHIFT);
+
+ return val == 0x1;
+}
+
static inline bool system_supports_fpsimd(void)
{
return !cpus_have_const_cap(ARM64_HAS_NO_FPSIMD);
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 09/16] arm64: enable KEXEC_FILE config
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (7 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 08/16] arm64: cpufeature: add MMFR0 helper functions AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 10/16] arm64: kexec_file: load initrd and device-tree AKASHI Takahiro
` (6 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
Modify arm64/Kconfig to enable kexec_file_load support.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Acked-by: James Morse <james.morse@arm.com>
---
arch/arm64/Kconfig | 9 +++++++++
arch/arm64/kernel/Makefile | 3 ++-
arch/arm64/kernel/machine_kexec_file.c | 16 ++++++++++++++++
3 files changed, 27 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kernel/machine_kexec_file.c
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1b1a0e95c751..5eb18d0cb513 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -843,6 +843,15 @@ config KEXEC
but it is independent of the system firmware. And like a reboot
you can start any kernel with it, not just Linux.
+config KEXEC_FILE
+ bool "kexec file based system call"
+ select KEXEC_CORE
+ help
+ This is new version of kexec system call. This system call is
+ file based and takes file descriptors as system call argument
+ for kernel and initramfs as opposed to list of segments as
+ accepted by previous system call.
+
config CRASH_DUMP
bool "Build kdump crash kernel"
help
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 4c8b13bede80..030a39bff117 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -49,8 +49,9 @@ arm64-obj-$(CONFIG_ARM64_ACPI_PARKING_PROTOCOL) += acpi_parking_protocol.o
arm64-obj-$(CONFIG_PARAVIRT) += paravirt.o
arm64-obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
arm64-obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o
-arm64-obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o \
+arm64-obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \
cpu-reset.o
+arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o
arm64-obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o
arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
arm64-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
new file mode 100644
index 000000000000..c38a8048ed00
--- /dev/null
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * kexec_file for arm64
+ *
+ * Copyright (C) 2018 Linaro Limited
+ * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ *
+ */
+
+#define pr_fmt(fmt) "kexec_file: " fmt
+
+#include <linux/kexec.h>
+
+const struct kexec_file_ops * const kexec_file_loaders[] = {
+ NULL
+};
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 10/16] arm64: kexec_file: load initrd and device-tree
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (8 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 09/16] arm64: enable KEXEC_FILE config AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel AKASHI Takahiro
` (5 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
load_other_segments() is expected to allocate and place all the necessary
memory segments other than kernel, including initrd and device-tree
blob (and elf core header for crash).
While most of the code was borrowed from kexec-tools' counterpart,
users may not be allowed to specify dtb explicitly, instead, the dtb
presented by the original boot loader is reused.
arch_kimage_kernel_post_load_cleanup() is responsible for freeing arm64-
specific data allocated in load_other_segments().
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/include/asm/kexec.h | 17 +++
arch/arm64/kernel/machine_kexec_file.c | 188 +++++++++++++++++++++++++
2 files changed, 205 insertions(+)
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index e17f0529a882..157b2897d911 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -93,6 +93,23 @@ static inline void crash_prepare_suspend(void) {}
static inline void crash_post_resume(void) {}
#endif
+#ifdef CONFIG_KEXEC_FILE
+#define ARCH_HAS_KIMAGE_ARCH
+
+struct kimage_arch {
+ void *dtb;
+ unsigned long dtb_mem;
+};
+
+struct kimage;
+
+extern int arch_kimage_file_post_load_cleanup(struct kimage *image);
+extern int load_other_segments(struct kimage *image,
+ unsigned long kernel_load_addr, unsigned long kernel_size,
+ char *initrd, unsigned long initrd_len,
+ char *cmdline, unsigned long cmdline_len);
+#endif
+
#endif /* __ASSEMBLY__ */
#endif
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index c38a8048ed00..f53f14bd1700 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -5,12 +5,200 @@
* Copyright (C) 2018 Linaro Limited
* Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
*
+ * Most code is derived from arm64 port of kexec-tools
*/
#define pr_fmt(fmt) "kexec_file: " fmt
+#include <linux/ioport.h>
+#include <linux/kernel.h>
#include <linux/kexec.h>
+#include <linux/libfdt.h>
+#include <linux/memblock.h>
+#include <linux/of_fdt.h>
+#include <linux/types.h>
+#include <asm/byteorder.h>
+
+/* relevant device tree properties */
+#define FDT_PSTR_INITRD_STA "linux,initrd-start"
+#define FDT_PSTR_INITRD_END "linux,initrd-end"
+#define FDT_PSTR_BOOTARGS "bootargs"
const struct kexec_file_ops * const kexec_file_loaders[] = {
NULL
};
+
+int arch_kimage_file_post_load_cleanup(struct kimage *image)
+{
+ vfree(image->arch.dtb);
+ image->arch.dtb = NULL;
+
+ return kexec_image_post_load_cleanup_default(image);
+}
+
+static int setup_dtb(struct kimage *image,
+ unsigned long initrd_load_addr, unsigned long initrd_len,
+ char *cmdline, unsigned long cmdline_len,
+ void **dtb_buf, unsigned long *dtb_buf_len)
+{
+ void *buf = NULL;
+ size_t buf_size;
+ int nodeoffset;
+ int ret;
+
+ /* duplicate dt blob */
+ buf_size = fdt_totalsize(initial_boot_params);
+
+ if (initrd_load_addr) {
+ /* can be redundant, but trimmed at the end */
+ buf_size += fdt_prop_len(FDT_PSTR_INITRD_STA, sizeof(u64));
+ buf_size += fdt_prop_len(FDT_PSTR_INITRD_END, sizeof(u64));
+ }
+
+ if (cmdline)
+ /* can be redundant, but trimmed at the end */
+ buf_size += fdt_prop_len(FDT_PSTR_BOOTARGS, cmdline_len);
+
+ buf = vmalloc(buf_size);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto out_err;
+ }
+
+ ret = fdt_open_into(initial_boot_params, buf, buf_size);
+ if (ret) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+
+ nodeoffset = fdt_path_offset(buf, "/chosen");
+ if (nodeoffset < 0) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+
+ /* add bootargs */
+ if (cmdline) {
+ ret = fdt_setprop_string(buf, nodeoffset, FDT_PSTR_BOOTARGS,
+ cmdline);
+ if (ret) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+ } else {
+ ret = fdt_delprop(buf, nodeoffset, FDT_PSTR_BOOTARGS);
+ if (ret && (ret != -FDT_ERR_NOTFOUND)) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+ }
+
+ /* add initrd-* */
+ if (initrd_load_addr) {
+ ret = fdt_setprop_u64(buf, nodeoffset, FDT_PSTR_INITRD_STA,
+ initrd_load_addr);
+ if (ret) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+
+ ret = fdt_setprop_u64(buf, nodeoffset, FDT_PSTR_INITRD_END,
+ initrd_load_addr + initrd_len);
+ if (ret) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+ } else {
+ ret = fdt_delprop(buf, nodeoffset, FDT_PSTR_INITRD_STA);
+ if (ret && (ret != -FDT_ERR_NOTFOUND)) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+
+ ret = fdt_delprop(buf, nodeoffset, FDT_PSTR_INITRD_END);
+ if (ret && (ret != -FDT_ERR_NOTFOUND)) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+ }
+
+ /* trim a buffer */
+ fdt_pack(buf);
+ *dtb_buf = buf;
+ *dtb_buf_len = fdt_totalsize(buf);
+
+ return 0;
+
+out_err:
+ vfree(buf);
+ return ret;
+}
+
+int load_other_segments(struct kimage *image,
+ unsigned long kernel_load_addr,
+ unsigned long kernel_size,
+ char *initrd, unsigned long initrd_len,
+ char *cmdline, unsigned long cmdline_len)
+{
+ struct kexec_buf kbuf;
+ void *dtb = NULL;
+ unsigned long initrd_load_addr = 0, dtb_len;
+ int ret = 0;
+
+ kbuf.image = image;
+ /* not allocate anything below the kernel */
+ kbuf.buf_min = kernel_load_addr + kernel_size;
+
+ /* load initrd */
+ if (initrd) {
+ kbuf.buffer = initrd;
+ kbuf.bufsz = initrd_len;
+ kbuf.mem = 0;
+ kbuf.memsz = initrd_len;
+ kbuf.buf_align = 0;
+ /* within 1GB-aligned window of up to 32GB in size */
+ kbuf.buf_max = round_down(kernel_load_addr, SZ_1G)
+ + (unsigned long)SZ_1G * 32;
+ kbuf.top_down = false;
+
+ ret = kexec_add_buffer(&kbuf);
+ if (ret)
+ goto out_err;
+ initrd_load_addr = kbuf.mem;
+
+ pr_debug("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+ initrd_load_addr, initrd_len, initrd_len);
+ }
+
+ /* load dtb blob */
+ ret = setup_dtb(image, initrd_load_addr, initrd_len,
+ cmdline, cmdline_len, &dtb, &dtb_len);
+ if (ret) {
+ pr_err("Preparing for new dtb failed\n");
+ goto out_err;
+ }
+
+ kbuf.buffer = dtb;
+ kbuf.bufsz = dtb_len;
+ kbuf.mem = 0;
+ kbuf.memsz = dtb_len;
+ /* not across 2MB boundary */
+ kbuf.buf_align = SZ_2M;
+ kbuf.buf_max = ULONG_MAX;
+ kbuf.top_down = true;
+
+ ret = kexec_add_buffer(&kbuf);
+ if (ret)
+ goto out_err;
+ image->arch.dtb = dtb;
+ image->arch.dtb_mem = kbuf.mem;
+
+ pr_debug("Loaded dtb at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+ kbuf.mem, dtb_len, dtb_len);
+
+ return 0;
+
+out_err:
+ vfree(dtb);
+ return ret;
+}
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (9 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 10/16] arm64: kexec_file: load initrd and device-tree AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-10-09 15:28 ` Mark Rutland
2018-09-28 6:48 ` [PATCH v15 12/16] arm64: kexec_file: add crash dump support AKASHI Takahiro
` (4 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
This patch provides kexec_file_ops for "Image"-format kernel. In this
implementation, a binary is always loaded with a fixed offset identified
in text_offset field of its header.
Regarding signature verification for trusted boot, this patch doesn't
contains CONFIG_KEXEC_VERIFY_SIG support, which is to be added later
in this series, but file-attribute-based verification is still a viable
option by enabling IMA security subsystem.
You can sign(label) a to-be-kexec'ed kernel image on target file system
with:
$ evmctl ima_sign --key /path/to/private_key.pem Image
On live system, you must have IMA enforced with, at least, the following
security policy:
"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig"
See more details about IMA here:
https://sourceforge.net/p/linux-ima/wiki/Home/
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/include/asm/kexec.h | 28 +++++++
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/kexec_image.c | 108 +++++++++++++++++++++++++
arch/arm64/kernel/machine_kexec_file.c | 1 +
4 files changed, 138 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kernel/kexec_image.c
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 157b2897d911..5e673481b3a3 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -101,6 +101,34 @@ struct kimage_arch {
unsigned long dtb_mem;
};
+/**
+ * struct arm64_image_header - arm64 kernel image header
+ * See Documentation/arm64/booting.txt for details
+ *
+ * @mz_magic: DOS header magic number ('MZ', optional)
+ * @code1: Instruction (branch to stext)
+ * @text_offset: Image load offset
+ * @image_size: Effective image size
+ * @flags: Bit-field flags
+ * @reserved: Reserved
+ * @magic: Magic number
+ * @pe_header: Offset to PE COFF header (optional)
+ **/
+
+struct arm64_image_header {
+ __le16 mz_magic; /* also code0 */
+ __le16 pad;
+ __le32 code1;
+ __le64 text_offset;
+ __le64 image_size;
+ __le64 flags;
+ __le64 reserved[3];
+ __le32 magic;
+ __le32 pe_header;
+};
+
+extern const struct kexec_file_ops kexec_image_ops;
+
struct kimage;
extern int arch_kimage_file_post_load_cleanup(struct kimage *image);
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 030a39bff117..48868255f09c 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -51,7 +51,7 @@ arm64-obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
arm64-obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o
arm64-obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \
cpu-reset.o
-arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o
+arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o
arm64-obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o
arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
arm64-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
new file mode 100644
index 000000000000..d64f5e9f9d22
--- /dev/null
+++ b/arch/arm64/kernel/kexec_image.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Kexec image loader
+
+ * Copyright (C) 2018 Linaro Limited
+ * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
+ */
+
+#define pr_fmt(fmt) "kexec_file(Image): " fmt
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/kexec.h>
+#include <linux/string.h>
+#include <asm/boot.h>
+#include <asm/byteorder.h>
+#include <asm/cpufeature.h>
+#include <asm/memory.h>
+
+static int image_probe(const char *kernel_buf, unsigned long kernel_len)
+{
+ const struct arm64_image_header *h;
+
+ h = (const struct arm64_image_header *)(kernel_buf);
+
+ if (!h || (kernel_len < sizeof(*h)) ||
+ memcmp(&h->magic, ARM64_MAGIC, sizeof(h->magic)))
+ return -EINVAL;
+
+ return 0;
+}
+
+static void *image_load(struct kimage *image,
+ char *kernel, unsigned long kernel_len,
+ char *initrd, unsigned long initrd_len,
+ char *cmdline, unsigned long cmdline_len)
+{
+ struct arm64_image_header *h;
+ u64 flags, value;
+ struct kexec_buf kbuf;
+ unsigned long text_offset;
+ struct kexec_segment *kernel_segment;
+ int ret;
+
+ /* Don't support old kernel */
+ h = (struct arm64_image_header *)kernel;
+ if (!h->text_offset)
+ return ERR_PTR(-EINVAL);
+
+ /* Check cpu features */
+ flags = le64_to_cpu(h->flags);
+ value = head_flag_field(flags, HEAD_FLAG_BE);
+ if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
+ ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
+ if (!system_supports_mixed_endian())
+ return ERR_PTR(-EINVAL);
+
+ value = head_flag_field(flags, HEAD_FLAG_PAGE_SIZE);
+ if (((value == HEAD_FLAG_PAGE_SIZE_4K) &&
+ !system_supports_4kb_granule()) ||
+ ((value == HEAD_FLAG_PAGE_SIZE_64K) &&
+ !system_supports_64kb_granule()) ||
+ ((value == HEAD_FLAG_PAGE_SIZE_16K) &&
+ !system_supports_16kb_granule()))
+ return ERR_PTR(-EINVAL);
+
+ /* Load the kernel */
+ kbuf.image = image;
+ kbuf.buf_min = 0;
+ kbuf.buf_max = ULONG_MAX;
+ kbuf.top_down = false;
+
+ kbuf.buffer = kernel;
+ kbuf.bufsz = kernel_len;
+ kbuf.mem = 0;
+ kbuf.memsz = le64_to_cpu(h->image_size);
+ text_offset = le64_to_cpu(h->text_offset);
+ kbuf.buf_align = MIN_KIMG_ALIGN;
+
+ /* Adjust kernel segment with TEXT_OFFSET */
+ kbuf.memsz += text_offset;
+
+ ret = kexec_add_buffer(&kbuf);
+ if (ret)
+ return ERR_PTR(ret);
+
+ kernel_segment = &image->segment[image->nr_segments - 1];
+ kernel_segment->mem += text_offset;
+ kernel_segment->memsz -= text_offset;
+ image->start = kernel_segment->mem;
+
+ pr_debug("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+ kernel_segment->mem, kbuf.bufsz,
+ kernel_segment->memsz);
+
+ /* Load additional data */
+ ret = load_other_segments(image,
+ kernel_segment->mem, kernel_segment->memsz,
+ initrd, initrd_len, cmdline, cmdline_len);
+
+ return ERR_PTR(ret);
+}
+
+const struct kexec_file_ops kexec_image_ops = {
+ .probe = image_probe,
+ .load = image_load,
+};
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index f53f14bd1700..05fb2d4e6fef 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -25,6 +25,7 @@
#define FDT_PSTR_BOOTARGS "bootargs"
const struct kexec_file_ops * const kexec_file_loaders[] = {
+ &kexec_image_ops,
NULL
};
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel
2018-09-28 6:48 ` [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel AKASHI Takahiro
@ 2018-10-09 15:28 ` Mark Rutland
2018-10-10 6:52 ` AKASHI Takahiro
0 siblings, 1 reply; 42+ messages in thread
From: Mark Rutland @ 2018-10-09 15:28 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens, prudo,
ard.biesheuvel, james.morse, bhsharma, kexec, linux-arm-kernel,
linux-kernel
On Fri, Sep 28, 2018 at 03:48:36PM +0900, AKASHI Takahiro wrote:
> This patch provides kexec_file_ops for "Image"-format kernel. In this
> implementation, a binary is always loaded with a fixed offset identified
> in text_offset field of its header.
>
> Regarding signature verification for trusted boot, this patch doesn't
> contains CONFIG_KEXEC_VERIFY_SIG support, which is to be added later
> in this series, but file-attribute-based verification is still a viable
> option by enabling IMA security subsystem.
>
> You can sign(label) a to-be-kexec'ed kernel image on target file system
> with:
> $ evmctl ima_sign --key /path/to/private_key.pem Image
>
> On live system, you must have IMA enforced with, at least, the following
> security policy:
> "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig"
>
> See more details about IMA here:
> https://sourceforge.net/p/linux-ima/wiki/Home/
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
> arch/arm64/include/asm/kexec.h | 28 +++++++
> arch/arm64/kernel/Makefile | 2 +-
> arch/arm64/kernel/kexec_image.c | 108 +++++++++++++++++++++++++
> arch/arm64/kernel/machine_kexec_file.c | 1 +
> 4 files changed, 138 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/kernel/kexec_image.c
>
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> index 157b2897d911..5e673481b3a3 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -101,6 +101,34 @@ struct kimage_arch {
> unsigned long dtb_mem;
> };
>
> +/**
> + * struct arm64_image_header - arm64 kernel image header
> + * See Documentation/arm64/booting.txt for details
> + *
> + * @mz_magic: DOS header magic number ('MZ', optional)
Please just call this code0. If CONFIG_EFI is disabled, it is not 'MZ'.
> + * @code1: Instruction (branch to stext)
> + * @text_offset: Image load offset
> + * @image_size: Effective image size
> + * @flags: Bit-field flags
> + * @reserved: Reserved
> + * @magic: Magic number
> + * @pe_header: Offset to PE COFF header (optional)
> + **/
> +
> +struct arm64_image_header {
> + __le16 mz_magic; /* also code0 */
> + __le16 pad;
Likewise, just have __le32 code0 here, please.
> + __le32 code1;
> + __le64 text_offset;
> + __le64 image_size;
> + __le64 flags;
> + __le64 reserved[3];
> + __le32 magic;
> + __le32 pe_header;
> +};
> +
> +extern const struct kexec_file_ops kexec_image_ops;
> +
> struct kimage;
>
> extern int arch_kimage_file_post_load_cleanup(struct kimage *image);
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 030a39bff117..48868255f09c 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -51,7 +51,7 @@ arm64-obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
> arm64-obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o
> arm64-obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \
> cpu-reset.o
> -arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o
> +arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o
> arm64-obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o
> arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
> arm64-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
> diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
> new file mode 100644
> index 000000000000..d64f5e9f9d22
> --- /dev/null
> +++ b/arch/arm64/kernel/kexec_image.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kexec image loader
> +
> + * Copyright (C) 2018 Linaro Limited
> + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> + */
> +
> +#define pr_fmt(fmt) "kexec_file(Image): " fmt
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/kexec.h>
> +#include <linux/string.h>
> +#include <asm/boot.h>
> +#include <asm/byteorder.h>
> +#include <asm/cpufeature.h>
> +#include <asm/memory.h>
> +
> +static int image_probe(const char *kernel_buf, unsigned long kernel_len)
> +{
> + const struct arm64_image_header *h;
> +
> + h = (const struct arm64_image_header *)(kernel_buf);
> +
> + if (!h || (kernel_len < sizeof(*h)) ||
> + memcmp(&h->magic, ARM64_MAGIC, sizeof(h->magic)))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static void *image_load(struct kimage *image,
> + char *kernel, unsigned long kernel_len,
> + char *initrd, unsigned long initrd_len,
> + char *cmdline, unsigned long cmdline_len)
> +{
> + struct arm64_image_header *h;
> + u64 flags, value;
> + struct kexec_buf kbuf;
> + unsigned long text_offset;
> + struct kexec_segment *kernel_segment;
> + int ret;
> +
> + /* Don't support old kernel */
> + h = (struct arm64_image_header *)kernel;
> + if (!h->text_offset)
> + return ERR_PTR(-EINVAL);
It's entirely valid for TEXT_OFFSET to be zero when
RANDOMIZE_TEXT_OFFSET is selected.
I think you meant to check image_size here.
We could do with a better comment, too, e.g.
/*
* We require a kernel with an unambiguous Image header. Per
* Documentation/booting.txt, this is the case when image_size
* is non-zero (practically speaking, since v3.17).
*/
if (!h->image_size)
return ERR_PTR(-EINVAL);
> +
> + /* Check cpu features */
> + flags = le64_to_cpu(h->flags);
> + value = head_flag_field(flags, HEAD_FLAG_BE);
> + if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
> + ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
> + if (!system_supports_mixed_endian())
> + return ERR_PTR(-EINVAL);
I think this can be simplified:
bool be_image = head_flag_field(flags, HEAD_FLAG_BE);
bool be_kernel = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
if ((be_image != be_kernel) && !system_supports_mixed_endian)
return ERR_PTR(-EINVAL);
... though do we need to police this at all? It's arguably policy given
the new image has to be signed anyway), and there are other fields that
could fall into that category in future.
> +
> + value = head_flag_field(flags, HEAD_FLAG_PAGE_SIZE);
> + if (((value == HEAD_FLAG_PAGE_SIZE_4K) &&
> + !system_supports_4kb_granule()) ||
> + ((value == HEAD_FLAG_PAGE_SIZE_64K) &&
> + !system_supports_64kb_granule()) ||
> + ((value == HEAD_FLAG_PAGE_SIZE_16K) &&
> + !system_supports_16kb_granule()))
> + return ERR_PTR(-EINVAL);
... likewise here?
> +
> + /* Load the kernel */
> + kbuf.image = image;
> + kbuf.buf_min = 0;
> + kbuf.buf_max = ULONG_MAX;
> + kbuf.top_down = false;
> +
> + kbuf.buffer = kernel;
> + kbuf.bufsz = kernel_len;
> + kbuf.mem = 0;
> + kbuf.memsz = le64_to_cpu(h->image_size);
> + text_offset = le64_to_cpu(h->text_offset);
> + kbuf.buf_align = MIN_KIMG_ALIGN;
> +
> + /* Adjust kernel segment with TEXT_OFFSET */
> + kbuf.memsz += text_offset;
It's very surprising at first glance to add text_offset here, then undo
that below. This should have a better comment explaining what we're
doing.
> +
> + ret = kexec_add_buffer(&kbuf);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + kernel_segment = &image->segment[image->nr_segments - 1];
I'm confused here. When/how can the image have muliple segments?
> + kernel_segment->mem += text_offset;
> + kernel_segment->memsz -= text_offset;
> + image->start = kernel_segment->mem;
As above, I don't like the fact that we're altering the result of
kexec_add_buffer here. It feels fragile, even if it works today.
Can we teach the core kexec buffer code that we need an offset from an
aligned base?
Thanks,
Mark.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel
2018-10-09 15:28 ` Mark Rutland
@ 2018-10-10 6:52 ` AKASHI Takahiro
2018-10-10 9:47 ` Mark Rutland
0 siblings, 1 reply; 42+ messages in thread
From: AKASHI Takahiro @ 2018-10-10 6:52 UTC (permalink / raw)
To: Mark Rutland
Cc: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens, prudo,
ard.biesheuvel, james.morse, bhsharma, kexec, linux-arm-kernel,
linux-kernel
Mark,
On Tue, Oct 09, 2018 at 04:28:00PM +0100, Mark Rutland wrote:
> On Fri, Sep 28, 2018 at 03:48:36PM +0900, AKASHI Takahiro wrote:
> > This patch provides kexec_file_ops for "Image"-format kernel. In this
> > implementation, a binary is always loaded with a fixed offset identified
> > in text_offset field of its header.
> >
> > Regarding signature verification for trusted boot, this patch doesn't
> > contains CONFIG_KEXEC_VERIFY_SIG support, which is to be added later
> > in this series, but file-attribute-based verification is still a viable
> > option by enabling IMA security subsystem.
> >
> > You can sign(label) a to-be-kexec'ed kernel image on target file system
> > with:
> > $ evmctl ima_sign --key /path/to/private_key.pem Image
> >
> > On live system, you must have IMA enforced with, at least, the following
> > security policy:
> > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig"
> >
> > See more details about IMA here:
> > https://sourceforge.net/p/linux-ima/wiki/Home/
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Reviewed-by: James Morse <james.morse@arm.com>
> > ---
> > arch/arm64/include/asm/kexec.h | 28 +++++++
> > arch/arm64/kernel/Makefile | 2 +-
> > arch/arm64/kernel/kexec_image.c | 108 +++++++++++++++++++++++++
> > arch/arm64/kernel/machine_kexec_file.c | 1 +
> > 4 files changed, 138 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm64/kernel/kexec_image.c
> >
> > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > index 157b2897d911..5e673481b3a3 100644
> > --- a/arch/arm64/include/asm/kexec.h
> > +++ b/arch/arm64/include/asm/kexec.h
> > @@ -101,6 +101,34 @@ struct kimage_arch {
> > unsigned long dtb_mem;
> > };
> >
> > +/**
> > + * struct arm64_image_header - arm64 kernel image header
> > + * See Documentation/arm64/booting.txt for details
> > + *
> > + * @mz_magic: DOS header magic number ('MZ', optional)
>
> Please just call this code0. If CONFIG_EFI is disabled, it is not 'MZ'.
How about this?
(This definition will go into a new header, asm/image.h.)
---8<---
/*
* struct arm64_image_header - arm64 kernel image header
* See Documentation/arm64/booting.txt for details
*
* @code0: Executable code, or
* @mz_header alternatively used for part of MZ header
* @code1: Executable code
* @text_offset: Image load offset
* @image_size: Effective Image size
* @flags: kernel flags
* @reserved: reserved
* @magic: Magic number
* @reserved5: reserved, or
* @pe_header: alternatively used for PE COFF offset
*/
struct arm64_image_header {
union {
__le32 code0;
struct {
__le16 magic;
__le16 pad;
} mz_header;
};
__le32 code1;
__le64 text_offset;
__le64 image_size;
__le64 flags;
__le64 reserved[3];
__le32 magic;
union {
__le32 reserved5;
__le32 pe_header;
};
};
--->8---
> > + * @code1: Instruction (branch to stext)
> > + * @text_offset: Image load offset
> > + * @image_size: Effective image size
> > + * @flags: Bit-field flags
> > + * @reserved: Reserved
> > + * @magic: Magic number
> > + * @pe_header: Offset to PE COFF header (optional)
> > + **/
> > +
> > +struct arm64_image_header {
> > + __le16 mz_magic; /* also code0 */
> > + __le16 pad;
>
> Likewise, just have __le32 code0 here, please.
>
> > + __le32 code1;
> > + __le64 text_offset;
> > + __le64 image_size;
> > + __le64 flags;
> > + __le64 reserved[3];
> > + __le32 magic;
> > + __le32 pe_header;
> > +};
> > +
> > +extern const struct kexec_file_ops kexec_image_ops;
> > +
> > struct kimage;
> >
> > extern int arch_kimage_file_post_load_cleanup(struct kimage *image);
> > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> > index 030a39bff117..48868255f09c 100644
> > --- a/arch/arm64/kernel/Makefile
> > +++ b/arch/arm64/kernel/Makefile
> > @@ -51,7 +51,7 @@ arm64-obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
> > arm64-obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o
> > arm64-obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \
> > cpu-reset.o
> > -arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o
> > +arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o
> > arm64-obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o
> > arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
> > arm64-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
> > diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
> > new file mode 100644
> > index 000000000000..d64f5e9f9d22
> > --- /dev/null
> > +++ b/arch/arm64/kernel/kexec_image.c
> > @@ -0,0 +1,108 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Kexec image loader
> > +
> > + * Copyright (C) 2018 Linaro Limited
> > + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > + */
> > +
> > +#define pr_fmt(fmt) "kexec_file(Image): " fmt
> > +
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/kexec.h>
> > +#include <linux/string.h>
> > +#include <asm/boot.h>
> > +#include <asm/byteorder.h>
> > +#include <asm/cpufeature.h>
> > +#include <asm/memory.h>
> > +
> > +static int image_probe(const char *kernel_buf, unsigned long kernel_len)
> > +{
> > + const struct arm64_image_header *h;
> > +
> > + h = (const struct arm64_image_header *)(kernel_buf);
> > +
> > + if (!h || (kernel_len < sizeof(*h)) ||
> > + memcmp(&h->magic, ARM64_MAGIC, sizeof(h->magic)))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static void *image_load(struct kimage *image,
> > + char *kernel, unsigned long kernel_len,
> > + char *initrd, unsigned long initrd_len,
> > + char *cmdline, unsigned long cmdline_len)
> > +{
> > + struct arm64_image_header *h;
> > + u64 flags, value;
> > + struct kexec_buf kbuf;
> > + unsigned long text_offset;
> > + struct kexec_segment *kernel_segment;
> > + int ret;
> > +
> > + /* Don't support old kernel */
> > + h = (struct arm64_image_header *)kernel;
> > + if (!h->text_offset)
> > + return ERR_PTR(-EINVAL);
>
> It's entirely valid for TEXT_OFFSET to be zero when
> RANDOMIZE_TEXT_OFFSET is selected.
>
> I think you meant to check image_size here.
Right, it's been a bug since the first appearance in v10.
> We could do with a better comment, too, e.g.
>
> /*
> * We require a kernel with an unambiguous Image header. Per
> * Documentation/booting.txt, this is the case when image_size
> * is non-zero (practically speaking, since v3.17).
> */
> if (!h->image_size)
> return ERR_PTR(-EINVAL);
>
> > +
> > + /* Check cpu features */
> > + flags = le64_to_cpu(h->flags);
> > + value = head_flag_field(flags, HEAD_FLAG_BE);
> > + if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
> > + ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
> > + if (!system_supports_mixed_endian())
> > + return ERR_PTR(-EINVAL);
>
> I think this can be simplified:
>
> bool be_image = head_flag_field(flags, HEAD_FLAG_BE);
> bool be_kernel = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
>
> if ((be_image != be_kernel) && !system_supports_mixed_endian)
> return ERR_PTR(-EINVAL);
Okay.
> ... though do we need to police this at all? It's arguably policy given
> the new image has to be signed anyway), and there are other fields that
> could fall into that category in future.
The aim here is to prevent any images from being loaded
when there is no question that a new image will never be successfully
kexec'ed since the core on a given SoC obviously doesn't support cpu
features that are assumed and required by a image.
I believe that this check is a good and easy practice to avoid possible
failures before execution.
> > +
> > + value = head_flag_field(flags, HEAD_FLAG_PAGE_SIZE);
> > + if (((value == HEAD_FLAG_PAGE_SIZE_4K) &&
> > + !system_supports_4kb_granule()) ||
> > + ((value == HEAD_FLAG_PAGE_SIZE_64K) &&
> > + !system_supports_64kb_granule()) ||
> > + ((value == HEAD_FLAG_PAGE_SIZE_16K) &&
> > + !system_supports_16kb_granule()))
> > + return ERR_PTR(-EINVAL);
>
> ... likewise here?
>
> > +
> > + /* Load the kernel */
> > + kbuf.image = image;
> > + kbuf.buf_min = 0;
> > + kbuf.buf_max = ULONG_MAX;
> > + kbuf.top_down = false;
> > +
> > + kbuf.buffer = kernel;
> > + kbuf.bufsz = kernel_len;
> > + kbuf.mem = 0;
> > + kbuf.memsz = le64_to_cpu(h->image_size);
> > + text_offset = le64_to_cpu(h->text_offset);
> > + kbuf.buf_align = MIN_KIMG_ALIGN;
> > +
> > + /* Adjust kernel segment with TEXT_OFFSET */
> > + kbuf.memsz += text_offset;
>
> It's very surprising at first glance to add text_offset here, then undo
> that below. This should have a better comment explaining what we're
> doing.
To respect TEXT_OFFSET particularly for older kernels.
Will add some comments here.
> > +
> > + ret = kexec_add_buffer(&kbuf);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + kernel_segment = &image->segment[image->nr_segments - 1];
>
> I'm confused here. When/how can the image have muliple segments?
kexec_add_buffer() allocates one contiguous region, and
so nr_segments should be 1. But I think using nr_segments is resilient
if some buffer may be added before this line.
(Historically, in my old patches, a buffer for elfcoreheader was added
before a kernel segument was added to image->segment[].)
> > + kernel_segment->mem += text_offset;
> > + kernel_segment->memsz -= text_offset;
> > + image->start = kernel_segment->mem;
>
> As above, I don't like the fact that we're altering the result of
> kexec_add_buffer here. It feels fragile, even if it works today.
>
> Can we teach the core kexec buffer code that we need an offset from an
> aligned base?
Adding another parameter to kexec_add_buffer() would be simple to do,
but I doubt that there is any difference since how to fill an allocated
segment with some data is totally up to the caller (arch-specific).
(That said, let me think twice.)
Thanks,
-Takahiro Akashi
> Thanks,
> Mark.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel
2018-10-10 6:52 ` AKASHI Takahiro
@ 2018-10-10 9:47 ` Mark Rutland
0 siblings, 0 replies; 42+ messages in thread
From: Mark Rutland @ 2018-10-10 9:47 UTC (permalink / raw)
To: AKASHI Takahiro, catalin.marinas, will.deacon, dhowells, vgoyal,
herbert, davem, dyoung, bhe, arnd, schwidefsky, heiko.carstens,
prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel
On Wed, Oct 10, 2018 at 03:52:37PM +0900, AKASHI Takahiro wrote:
> Mark,
>
> On Tue, Oct 09, 2018 at 04:28:00PM +0100, Mark Rutland wrote:
> > On Fri, Sep 28, 2018 at 03:48:36PM +0900, AKASHI Takahiro wrote:
> > > This patch provides kexec_file_ops for "Image"-format kernel. In this
> > > implementation, a binary is always loaded with a fixed offset identified
> > > in text_offset field of its header.
> > >
> > > Regarding signature verification for trusted boot, this patch doesn't
> > > contains CONFIG_KEXEC_VERIFY_SIG support, which is to be added later
> > > in this series, but file-attribute-based verification is still a viable
> > > option by enabling IMA security subsystem.
> > >
> > > You can sign(label) a to-be-kexec'ed kernel image on target file system
> > > with:
> > > $ evmctl ima_sign --key /path/to/private_key.pem Image
> > >
> > > On live system, you must have IMA enforced with, at least, the following
> > > security policy:
> > > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig"
> > >
> > > See more details about IMA here:
> > > https://sourceforge.net/p/linux-ima/wiki/Home/
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Reviewed-by: James Morse <james.morse@arm.com>
> > > ---
> > > arch/arm64/include/asm/kexec.h | 28 +++++++
> > > arch/arm64/kernel/Makefile | 2 +-
> > > arch/arm64/kernel/kexec_image.c | 108 +++++++++++++++++++++++++
> > > arch/arm64/kernel/machine_kexec_file.c | 1 +
> > > 4 files changed, 138 insertions(+), 1 deletion(-)
> > > create mode 100644 arch/arm64/kernel/kexec_image.c
> > >
> > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > index 157b2897d911..5e673481b3a3 100644
> > > --- a/arch/arm64/include/asm/kexec.h
> > > +++ b/arch/arm64/include/asm/kexec.h
> > > @@ -101,6 +101,34 @@ struct kimage_arch {
> > > unsigned long dtb_mem;
> > > };
> > >
> > > +/**
> > > + * struct arm64_image_header - arm64 kernel image header
> > > + * See Documentation/arm64/booting.txt for details
> > > + *
> > > + * @mz_magic: DOS header magic number ('MZ', optional)
> >
> > Please just call this code0. If CONFIG_EFI is disabled, it is not 'MZ'.
>
> How about this?
> (This definition will go into a new header, asm/image.h.)
>
> ---8<---
> /*
> * struct arm64_image_header - arm64 kernel image header
> * See Documentation/arm64/booting.txt for details
> *
> * @code0: Executable code, or
> * @mz_header alternatively used for part of MZ header
> * @code1: Executable code
> * @text_offset: Image load offset
> * @image_size: Effective Image size
> * @flags: kernel flags
> * @reserved: reserved
> * @magic: Magic number
> * @reserved5: reserved, or
> * @pe_header: alternatively used for PE COFF offset
> */
>
> struct arm64_image_header {
> union {
> __le32 code0;
> struct {
> __le16 magic;
> __le16 pad;
> } mz_header;
> };
> __le32 code1;
> __le64 text_offset;
> __le64 image_size;
> __le64 flags;
> __le64 reserved[3];
> __le32 magic;
> union {
> __le32 reserved5;
> __le32 pe_header;
> };
> };
Do we care about the MZ header?
The definition of the Image header in Documentation/arm64/booting.txt is:
u32 code0; /* Executable code */
u32 code1; /* Executable code */
u64 text_offset; /* Image load offset, little endian */
u64 image_size; /* Effective Image size, little endian */
u64 flags; /* kernel flags, little endian */
u64 res2 = 0; /* reserved */
u64 res3 = 0; /* reserved */
u64 res4 = 0; /* reserved */
u32 magic = 0x644d5241; /* Magic number, little endian, "ARM\x64" */
u32 res5; /* reserved (used for PE COFF offset) */
I'd prefer that we aligned our header definition with that, rather than
diverging from it.
If we need to look at the MZ magic to determine if the Image is a valid PE
binary, can't we just cast to the existing struct mz_hdr from <linux/pe.h> to
do that?
[...]
> > > + /* Check cpu features */
> > > + flags = le64_to_cpu(h->flags);
> > > + value = head_flag_field(flags, HEAD_FLAG_BE);
> > > + if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
> > > + ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
> > > + if (!system_supports_mixed_endian())
> > > + return ERR_PTR(-EINVAL);
> >
> > I think this can be simplified:
> >
> > bool be_image = head_flag_field(flags, HEAD_FLAG_BE);
> > bool be_kernel = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
> >
> > if ((be_image != be_kernel) && !system_supports_mixed_endian)
> > return ERR_PTR(-EINVAL);
>
> Okay.
>
> > ... though do we need to police this at all? It's arguably policy given
> > the new image has to be signed anyway), and there are other fields that
> > could fall into that category in future.
>
> The aim here is to prevent any images from being loaded
> when there is no question that a new image will never be successfully
> kexec'ed since the core on a given SoC obviously doesn't support cpu
> features that are assumed and required by a image.
>
> I believe that this check is a good and easy practice to avoid possible
> failures before execution.
My only concern is that this is arguably placing some policy in the
kernel, and I don't want to set the expectation that we'll do this for
other things in future, as that becomes a maintenance nightmare.
I'm not necessarily opposed to these specific checks, given they're
simple. Just wanted to make sure that we've thought about it.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v15 12/16] arm64: kexec_file: add crash dump support
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (10 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 11/16] arm64: kexec_file: allow for loading Image-format kernel AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 13/16] arm64: kexec_file: invoke the kernel without purgatory AKASHI Takahiro
` (3 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro, Rob Herring,
Frank Rowand, devicetree
Enabling crash dump (kdump) includes
* prepare contents of ELF header of a core dump file, /proc/vmcore,
using crash_prepare_elf64_headers(), and
* add two device tree properties, "linux,usable-memory-range" and
"linux,elfcorehdr", which represent respectively a memory range
to be used by crash dump kernel and the header's location
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
---
arch/arm64/include/asm/kexec.h | 4 +
arch/arm64/kernel/machine_kexec_file.c | 109 ++++++++++++++++++++++++-
2 files changed, 110 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index 5e673481b3a3..1b2c27026ae0 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h
@@ -99,6 +99,10 @@ static inline void crash_post_resume(void) {}
struct kimage_arch {
void *dtb;
unsigned long dtb_mem;
+ /* Core ELF header buffer */
+ void *elf_headers;
+ unsigned long elf_headers_mem;
+ unsigned long elf_headers_sz;
};
/**
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 05fb2d4e6fef..4b5ea256c84c 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -16,10 +16,14 @@
#include <linux/libfdt.h>
#include <linux/memblock.h>
#include <linux/of_fdt.h>
+#include <linux/slab.h>
#include <linux/types.h>
+#include <linux/vmalloc.h>
#include <asm/byteorder.h>
/* relevant device tree properties */
+#define FDT_PSTR_KEXEC_ELFHDR "linux,elfcorehdr"
+#define FDT_PSTR_MEM_RANGE "linux,usable-memory-range"
#define FDT_PSTR_INITRD_STA "linux,initrd-start"
#define FDT_PSTR_INITRD_END "linux,initrd-end"
#define FDT_PSTR_BOOTARGS "bootargs"
@@ -34,6 +38,10 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
vfree(image->arch.dtb);
image->arch.dtb = NULL;
+ vfree(image->arch.elf_headers);
+ image->arch.elf_headers = NULL;
+ image->arch.elf_headers_sz = 0;
+
return kexec_image_post_load_cleanup_default(image);
}
@@ -43,12 +51,25 @@ static int setup_dtb(struct kimage *image,
void **dtb_buf, unsigned long *dtb_buf_len)
{
void *buf = NULL;
- size_t buf_size;
+ int addr_cells, size_cells;
+ size_t buf_size, range_size;
int nodeoffset;
int ret;
/* duplicate dt blob */
buf_size = fdt_totalsize(initial_boot_params);
+ addr_cells = fdt_address_cells(initial_boot_params, 0);
+ if (addr_cells < 0)
+ return -EINVAL;
+ size_cells = fdt_size_cells(initial_boot_params, 0);
+ if (size_cells < 0)
+ return -EINVAL;
+ range_size = (addr_cells + size_cells) * sizeof(u32);
+
+ if (image->type == KEXEC_TYPE_CRASH) {
+ buf_size += fdt_prop_len(FDT_PSTR_KEXEC_ELFHDR, range_size);
+ buf_size += fdt_prop_len(FDT_PSTR_MEM_RANGE, range_size);
+ }
if (initrd_load_addr) {
/* can be redundant, but trimmed at the end */
@@ -78,6 +99,22 @@ static int setup_dtb(struct kimage *image,
goto out_err;
}
+ if (image->type == KEXEC_TYPE_CRASH) {
+ /* add linux,elfcorehdr */
+ ret = fdt_setprop_reg(buf, nodeoffset, FDT_PSTR_KEXEC_ELFHDR,
+ image->arch.elf_headers_mem,
+ image->arch.elf_headers_sz);
+ if (ret)
+ goto out_err;
+
+ /* add linux,usable-memory-range */
+ ret = fdt_setprop_reg(buf, nodeoffset, FDT_PSTR_MEM_RANGE,
+ crashk_res.start,
+ crashk_res.end - crashk_res.start + 1);
+ if (ret)
+ goto out_err;
+ }
+
/* add bootargs */
if (cmdline) {
ret = fdt_setprop_string(buf, nodeoffset, FDT_PSTR_BOOTARGS,
@@ -135,6 +172,43 @@ static int setup_dtb(struct kimage *image,
return ret;
}
+static int prepare_elf_headers(void **addr, unsigned long *sz)
+{
+ struct crash_mem *cmem;
+ unsigned int nr_ranges;
+ int ret;
+ u64 i;
+ phys_addr_t start, end;
+
+ nr_ranges = 1; /* for exclusion of crashkernel region */
+ for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
+ MEMBLOCK_NONE, &start, &end, NULL)
+ nr_ranges++;
+
+ cmem = kmalloc(sizeof(struct crash_mem) +
+ sizeof(struct crash_mem_range) * nr_ranges, GFP_KERNEL);
+ if (!cmem)
+ return -ENOMEM;
+
+ cmem->max_nr_ranges = nr_ranges;
+ cmem->nr_ranges = 0;
+ for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE,
+ MEMBLOCK_NONE, &start, &end, NULL) {
+ cmem->ranges[cmem->nr_ranges].start = start;
+ cmem->ranges[cmem->nr_ranges].end = end - 1;
+ cmem->nr_ranges++;
+ }
+
+ /* Exclude crashkernel region */
+ ret = crash_exclude_mem_range(cmem, crashk_res.start, crashk_res.end);
+
+ if (!ret)
+ ret = crash_prepare_elf64_headers(cmem, true, addr, sz);
+
+ kfree(cmem);
+ return ret;
+}
+
int load_other_segments(struct kimage *image,
unsigned long kernel_load_addr,
unsigned long kernel_size,
@@ -142,14 +216,43 @@ int load_other_segments(struct kimage *image,
char *cmdline, unsigned long cmdline_len)
{
struct kexec_buf kbuf;
- void *dtb = NULL;
- unsigned long initrd_load_addr = 0, dtb_len;
+ void *headers, *dtb = NULL;
+ unsigned long headers_sz, initrd_load_addr = 0, dtb_len;
int ret = 0;
kbuf.image = image;
/* not allocate anything below the kernel */
kbuf.buf_min = kernel_load_addr + kernel_size;
+ /* load elf core header */
+ if (image->type == KEXEC_TYPE_CRASH) {
+ ret = prepare_elf_headers(&headers, &headers_sz);
+ if (ret) {
+ pr_err("Preparing elf core header failed\n");
+ goto out_err;
+ }
+
+ kbuf.buffer = headers;
+ kbuf.bufsz = headers_sz;
+ kbuf.mem = 0;
+ kbuf.memsz = headers_sz;
+ kbuf.buf_align = SZ_64K; /* largest supported page size */
+ kbuf.buf_max = ULONG_MAX;
+ kbuf.top_down = true;
+
+ ret = kexec_add_buffer(&kbuf);
+ if (ret) {
+ vfree(headers);
+ goto out_err;
+ }
+ image->arch.elf_headers = headers;
+ image->arch.elf_headers_mem = kbuf.mem;
+ image->arch.elf_headers_sz = headers_sz;
+
+ pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+ image->arch.elf_headers_mem, headers_sz, headers_sz);
+ }
+
/* load initrd */
if (initrd) {
kbuf.buffer = initrd;
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 13/16] arm64: kexec_file: invoke the kernel without purgatory
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (11 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 12/16] arm64: kexec_file: add crash dump support AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 14/16] include: pe.h: remove message[] from mz header definition AKASHI Takahiro
` (2 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
On arm64, purgatory would do almost nothing. So just invoke secondary
kernel directly by jumping into its entry code.
While, in this case, cpu_soft_restart() must be called with dtb address
in the fifth argument, the behavior still stays compatible with kexec_load
case as long as the argument is null.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: James Morse <james.morse@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/cpu-reset.S | 8 ++++----
arch/arm64/kernel/machine_kexec.c | 12 ++++++++++--
arch/arm64/kernel/relocate_kernel.S | 3 ++-
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S
index 8021b46c9743..a2be30275a73 100644
--- a/arch/arm64/kernel/cpu-reset.S
+++ b/arch/arm64/kernel/cpu-reset.S
@@ -22,11 +22,11 @@
* __cpu_soft_restart(el2_switch, entry, arg0, arg1, arg2) - Helper for
* cpu_soft_restart.
*
- * @el2_switch: Flag to indicate a swich to EL2 is needed.
+ * @el2_switch: Flag to indicate a switch to EL2 is needed.
* @entry: Location to jump to for soft reset.
- * arg0: First argument passed to @entry.
- * arg1: Second argument passed to @entry.
- * arg2: Third argument passed to @entry.
+ * arg0: First argument passed to @entry. (relocation list)
+ * arg1: Second argument passed to @entry.(physical kernel entry)
+ * arg2: Third argument passed to @entry. (physical dtb address)
*
* Put the CPU into the same state as it would be if it had been reset, and
* branch to what would be the reset vector. It must be executed with the
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 922add8adb74..aa9c94113700 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -212,9 +212,17 @@ void machine_kexec(struct kimage *kimage)
* uses physical addressing to relocate the new image to its final
* position and transfers control to the image entry point when the
* relocation is complete.
+ * In kexec case, kimage->start points to purgatory assuming that
+ * kernel entry and dtb address are embedded in purgatory by
+ * userspace (kexec-tools).
+ * In kexec_file case, the kernel starts directly without purgatory.
*/
-
- cpu_soft_restart(reboot_code_buffer_phys, kimage->head, kimage->start, 0);
+ cpu_soft_restart(reboot_code_buffer_phys, kimage->head, kimage->start,
+#ifdef CONFIG_KEXEC_FILE
+ kimage->arch.dtb_mem);
+#else
+ 0);
+#endif
BUG(); /* Should never get here. */
}
diff --git a/arch/arm64/kernel/relocate_kernel.S b/arch/arm64/kernel/relocate_kernel.S
index f407e422a720..95fd94209aae 100644
--- a/arch/arm64/kernel/relocate_kernel.S
+++ b/arch/arm64/kernel/relocate_kernel.S
@@ -32,6 +32,7 @@
ENTRY(arm64_relocate_new_kernel)
/* Setup the list loop variables. */
+ mov x18, x2 /* x18 = dtb address */
mov x17, x1 /* x17 = kimage_start */
mov x16, x0 /* x16 = kimage_head */
raw_dcache_line_size x15, x0 /* x15 = dcache line size */
@@ -107,7 +108,7 @@ ENTRY(arm64_relocate_new_kernel)
isb
/* Start new image. */
- mov x0, xzr
+ mov x0, x18
mov x1, xzr
mov x2, xzr
mov x3, xzr
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 14/16] include: pe.h: remove message[] from mz header definition
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (12 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 13/16] arm64: kexec_file: invoke the kernel without purgatory AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 15/16] arm64: kexec_file: add kernel signature verification support AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 16/16] arm64: kexec_file: add kaslr support AKASHI Takahiro
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
message[] field won't be part of the definition of mz header.
This change is crucial for enabling kexec_file_load on arm64 because
arm64's "Image" binary, as in PE format, doesn't have any data for it and
accordingly the following check in pefile_parse_binary() will fail:
chkaddr(cursor, mz->peaddr, sizeof(*pe));
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
---
include/linux/pe.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/pe.h b/include/linux/pe.h
index 143ce75be5f0..3482b18a48b5 100644
--- a/include/linux/pe.h
+++ b/include/linux/pe.h
@@ -166,7 +166,7 @@ struct mz_hdr {
uint16_t oem_info; /* oem specific */
uint16_t reserved1[10]; /* reserved */
uint32_t peaddr; /* address of pe header */
- char message[64]; /* message to print */
+ char message[]; /* message to print */
};
struct mz_reloc {
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 15/16] arm64: kexec_file: add kernel signature verification support
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (13 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 14/16] include: pe.h: remove message[] from mz header definition AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
2018-09-28 6:48 ` [PATCH v15 16/16] arm64: kexec_file: add kaslr support AKASHI Takahiro
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
With this patch, kernel verification can be done without IMA security
subsystem enabled. Turn on CONFIG_KEXEC_VERIFY_SIG instead.
On x86, a signature is embedded into a PE file (Microsoft's format) header
of binary. Since arm64's "Image" can also be seen as a PE file as far as
CONFIG_EFI is enabled, we adopt this format for kernel signing.
You can create a signed kernel image with:
$ sbsign --key ${KEY} --cert ${CERT} Image
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/Kconfig | 24 ++++++++++++++++++++++++
arch/arm64/kernel/kexec_image.c | 15 +++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5eb18d0cb513..e7de9500bf0b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -852,6 +852,30 @@ config KEXEC_FILE
for kernel and initramfs as opposed to list of segments as
accepted by previous system call.
+config KEXEC_VERIFY_SIG
+ bool "Verify kernel signature during kexec_file_load() syscall"
+ depends on KEXEC_FILE
+ help
+ Select this option to verify a signature with loaded kernel
+ image. If configured, any attempt of loading a image without
+ valid signature will fail.
+
+ In addition to that option, you need to enable signature
+ verification for the corresponding kernel image type being
+ loaded in order for this to work.
+
+config KEXEC_IMAGE_VERIFY_SIG
+ bool "Enable Image signature verification support"
+ default y
+ depends on KEXEC_VERIFY_SIG
+ depends on EFI && SIGNED_PE_FILE_VERIFICATION
+ help
+ Enable Image signature verification support.
+
+comment "Support for PE file signature verification disabled"
+ depends on KEXEC_VERIFY_SIG
+ depends on !EFI || !SIGNED_PE_FILE_VERIFICATION
+
config CRASH_DUMP
bool "Build kdump crash kernel"
help
diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
index d64f5e9f9d22..578d358632d0 100644
--- a/arch/arm64/kernel/kexec_image.c
+++ b/arch/arm64/kernel/kexec_image.c
@@ -13,6 +13,7 @@
#include <linux/kernel.h>
#include <linux/kexec.h>
#include <linux/string.h>
+#include <linux/verification.h>
#include <asm/boot.h>
#include <asm/byteorder.h>
#include <asm/cpufeature.h>
@@ -28,6 +29,9 @@ static int image_probe(const char *kernel_buf, unsigned long kernel_len)
memcmp(&h->magic, ARM64_MAGIC, sizeof(h->magic)))
return -EINVAL;
+ pr_debug("PE format: %s\n",
+ memcmp(&h->mz_magic, "MZ", 2) ? "no" : "yes");
+
return 0;
}
@@ -102,7 +106,18 @@ static void *image_load(struct kimage *image,
return ERR_PTR(ret);
}
+#ifdef CONFIG_KEXEC_IMAGE_VERIFY_SIG
+static int image_verify_sig(const char *kernel, unsigned long kernel_len)
+{
+ return verify_pefile_signature(kernel, kernel_len, NULL,
+ VERIFYING_KEXEC_PE_SIGNATURE);
+}
+#endif
+
const struct kexec_file_ops kexec_image_ops = {
.probe = image_probe,
.load = image_load,
+#ifdef CONFIG_KEXEC_IMAGE_VERIFY_SIG
+ .verify_sig = image_verify_sig,
+#endif
};
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v15 16/16] arm64: kexec_file: add kaslr support
2018-09-28 6:48 [PATCH v15 00/16] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
` (14 preceding siblings ...)
2018-09-28 6:48 ` [PATCH v15 15/16] arm64: kexec_file: add kernel signature verification support AKASHI Takahiro
@ 2018-09-28 6:48 ` AKASHI Takahiro
15 siblings, 0 replies; 42+ messages in thread
From: AKASHI Takahiro @ 2018-09-28 6:48 UTC (permalink / raw)
To: catalin.marinas, will.deacon, dhowells, vgoyal, herbert, davem,
dyoung, bhe, arnd, schwidefsky, heiko.carstens
Cc: prudo, ard.biesheuvel, james.morse, bhsharma, kexec,
linux-arm-kernel, linux-kernel, AKASHI Takahiro
Adding "kaslr-seed" to dtb enables triggering kaslr, or kernel virtual
address randomization, at secondary kernel boot. We always do this as
it will have no harm on kaslr-incapable kernel.
We don't have any "switch" to turn off this feature directly, but still
can suppress it by passing "nokaslr" as a kernel boot argument.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/machine_kexec_file.c | 45 ++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index 4b5ea256c84c..c2ca7c921e7a 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -16,6 +16,7 @@
#include <linux/libfdt.h>
#include <linux/memblock.h>
#include <linux/of_fdt.h>
+#include <linux/random.h>
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/vmalloc.h>
@@ -27,6 +28,7 @@
#define FDT_PSTR_INITRD_STA "linux,initrd-start"
#define FDT_PSTR_INITRD_END "linux,initrd-end"
#define FDT_PSTR_BOOTARGS "bootargs"
+#define FDT_PSTR_KASLR_SEED "kaslr-seed"
const struct kexec_file_ops * const kexec_file_loaders[] = {
&kexec_image_ops,
@@ -45,6 +47,32 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
return kexec_image_post_load_cleanup_default(image);
}
+/* crng needs to have been initialized for providing kaslr-seed */
+static int random_ready;
+
+static void random_ready_notified(struct random_ready_callback *unused)
+{
+ random_ready = 1;
+}
+
+static struct random_ready_callback random_ready_cb = {
+ .func = random_ready_notified,
+};
+
+static __init int init_random_ready_cb(void)
+{
+ int ret;
+
+ ret = add_random_ready_callback(&random_ready_cb);
+ if (ret == -EALREADY)
+ random_ready = 1;
+ else if (ret)
+ pr_warn("failed to add a callback for random_ready\n");
+
+ return 0;
+}
+late_initcall(init_random_ready_cb)
+
static int setup_dtb(struct kimage *image,
unsigned long initrd_load_addr, unsigned long initrd_len,
char *cmdline, unsigned long cmdline_len,
@@ -54,6 +82,7 @@ static int setup_dtb(struct kimage *image,
int addr_cells, size_cells;
size_t buf_size, range_size;
int nodeoffset;
+ u64 value;
int ret;
/* duplicate dt blob */
@@ -81,6 +110,8 @@ static int setup_dtb(struct kimage *image,
/* can be redundant, but trimmed at the end */
buf_size += fdt_prop_len(FDT_PSTR_BOOTARGS, cmdline_len);
+ buf_size += fdt_prop_len(FDT_PSTR_KASLR_SEED, sizeof(u64));
+
buf = vmalloc(buf_size);
if (!buf) {
ret = -ENOMEM;
@@ -160,6 +191,20 @@ static int setup_dtb(struct kimage *image,
}
}
+ /* add kaslr-seed */
+ fdt_delprop(buf, nodeoffset, FDT_PSTR_KASLR_SEED);
+ if (random_ready) {
+ get_random_bytes(&value, sizeof(value));
+ ret = fdt_setprop_u64(buf, nodeoffset, FDT_PSTR_KASLR_SEED,
+ value);
+ if (ret) {
+ ret = -EINVAL;
+ goto out_err;
+ }
+ } else {
+ pr_notice("kaslr-seed won't be fed\n");
+ }
+
/* trim a buffer */
fdt_pack(buf);
*dtb_buf = buf;
--
2.19.0
^ permalink raw reply related [flat|nested] 42+ messages in thread