These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
Those helper functions for MMFR0 register will be used later by kexec_file
loader.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 48 +++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
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 <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
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(+)
@@ -5,12 +5,200 @@*Copyright(C)2018LinaroLimited*Author:AKASHITakahiro<takahiro.akashi@linaro.org>*+*Mostcodeisderivedfromarm64portofkexec-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"conststructkexec_file_ops*constkexec_file_loaders[]={NULL};++intarch_kimage_file_post_load_cleanup(structkimage*image)+{+vfree(image->arch.dtb);+image->arch.dtb=NULL;++returnkexec_image_post_load_cleanup_default(image);+}++staticintsetup_dtb(structkimage*image,+unsignedlonginitrd_load_addr,unsignedlonginitrd_len,+char*cmdline,unsignedlongcmdline_len,+void**dtb_buf,unsignedlong*dtb_buf_len)+{+void*buf=NULL;+size_tbuf_size;+intnodeoffset;+intret;++/* 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;+gotoout_err;+}++ret=fdt_open_into(initial_boot_params,buf,buf_size);+if(ret){+ret=-EINVAL;+gotoout_err;+}++nodeoffset=fdt_path_offset(buf,"/chosen");+if(nodeoffset<0){+ret=-EINVAL;+gotoout_err;+}++/* add bootargs */+if(cmdline){+ret=fdt_setprop_string(buf,nodeoffset,FDT_PSTR_BOOTARGS,+cmdline);+if(ret){+ret=-EINVAL;+gotoout_err;+}+}else{+ret=fdt_delprop(buf,nodeoffset,FDT_PSTR_BOOTARGS);+if(ret&&(ret!=-FDT_ERR_NOTFOUND)){+ret=-EINVAL;+gotoout_err;+}+}++/* add initrd-* */+if(initrd_load_addr){+ret=fdt_setprop_u64(buf,nodeoffset,FDT_PSTR_INITRD_STA,+initrd_load_addr);+if(ret){+ret=-EINVAL;+gotoout_err;+}++ret=fdt_setprop_u64(buf,nodeoffset,FDT_PSTR_INITRD_END,+initrd_load_addr+initrd_len);+if(ret){+ret=-EINVAL;+gotoout_err;+}+}else{+ret=fdt_delprop(buf,nodeoffset,FDT_PSTR_INITRD_STA);+if(ret&&(ret!=-FDT_ERR_NOTFOUND)){+ret=-EINVAL;+gotoout_err;+}++ret=fdt_delprop(buf,nodeoffset,FDT_PSTR_INITRD_END);+if(ret&&(ret!=-FDT_ERR_NOTFOUND)){+ret=-EINVAL;+gotoout_err;+}+}++/* trim a buffer */+fdt_pack(buf);+*dtb_buf=buf;+*dtb_buf_len=fdt_totalsize(buf);++return0;++out_err:+vfree(buf);+returnret;+}++intload_other_segments(structkimage*image,+unsignedlongkernel_load_addr,+unsignedlongkernel_size,+char*initrd,unsignedlonginitrd_len,+char*cmdline,unsignedlongcmdline_len)+{+structkexec_bufkbuf;+void*dtb=NULL;+unsignedlonginitrd_load_addr=0,dtb_len;+intret=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)++(unsignedlong)SZ_1G*32;+kbuf.top_down=false;++ret=kexec_add_buffer(&kbuf);+if(ret)+gotoout_err;+initrd_load_addr=kbuf.mem;++pr_debug("Loaded initrd@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");+gotoout_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)+gotoout_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);++return0;++out_err:+vfree(dtb);+returnret;+}
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 <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
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
@@ -0,0 +1,108 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Kexecimageloader++*Copyright(C)2018LinaroLimited+*Author:AKASHITakahiro<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>++staticintimage_probe(constchar*kernel_buf,unsignedlongkernel_len)+{+conststructarm64_image_header*h;++h=(conststructarm64_image_header*)(kernel_buf);++if(!h||(kernel_len<sizeof(*h))||+memcmp(&h->magic,ARM64_MAGIC,sizeof(h->magic)))+return-EINVAL;++return0;+}++staticvoid*image_load(structkimage*image,+char*kernel,unsignedlongkernel_len,+char*initrd,unsignedlonginitrd_len,+char*cmdline,unsignedlongcmdline_len)+{+structarm64_image_header*h;+u64flags,value;+structkexec_bufkbuf;+unsignedlongtext_offset;+structkexec_segment*kernel_segment;+intret;++/* Don't support old kernel */+h=(structarm64_image_header*)kernel;+if(!h->text_offset)+returnERR_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())+returnERR_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()))+returnERR_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)+returnERR_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);++returnERR_PTR(ret);+}++conststructkexec_file_opskexec_image_ops={+.probe=image_probe,+.load=image_load,+};
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 <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/include/asm/kexec.h | 4 +
arch/arm64/kernel/machine_kexec_file.c | 113 ++++++++++++++++++++++++-
2 files changed, 114 insertions(+), 3 deletions(-)
@@ -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;+returnkexec_image_post_load_cleanup_default(image);}
@@ -43,12 +51,29 @@ static int setup_dtb(struct kimage *image,void**dtb_buf,unsignedlong*dtb_buf_len){void*buf=NULL;-size_tbuf_size;+size_tbuf_size,range_size;intnodeoffset;intret;+/* check ranges against root's #address-cells and #size-cells */+if(image->type==KEXEC_TYPE_CRASH&&+(!of_fdt_cells_size_fitted(image->arch.elf_headers_mem,+image->arch.elf_headers_sz)||+!of_fdt_cells_size_fitted(crashk_res.start,+crashk_res.end-crashk_res.start+1))){+pr_err("Crash memory region doesn't fit into DT's root cell sizes.\n");+ret=-EINVAL;+gotoout_err;+}+/* duplicate dt blob */buf_size=fdt_totalsize(initial_boot_params);+range_size=of_fdt_reg_cells_size();++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 */
@@ -135,6 +176,43 @@ static int setup_dtb(struct kimage *image,returnret;}+staticintprepare_elf_headers(void**addr,unsignedlong*sz)+{+structcrash_mem*cmem;+unsignedintnr_ranges;+intret;+u64i;+phys_addr_tstart,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(structcrash_mem)++sizeof(structcrash_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);+returnret;+}+intload_other_segments(structkimage*image,unsignedlongkernel_load_addr,unsignedlongkernel_size,
@@ -142,14 +220,43 @@ int load_other_segments(struct kimage *image,char*cmdline,unsignedlongcmdline_len){structkexec_bufkbuf;-void*dtb=NULL;-unsignedlonginitrd_load_addr=0,dtb_len;+void*headers,*dtb=NULL;+unsignedlongheaders_sz,initrd_load_addr=0,dtb_len;intret=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");+gotoout_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);+gotoout_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;
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 <redacted>
Reviewed-by: James Morse <james.morse@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
---
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(-)
@@ -212,9 +212,17 @@ void machine_kexec(struct kimage *kimage)*usesphysicaladdressingtorelocatethenewimagetoitsfinal*positionandtransferscontroltotheimageentrypointwhenthe*relocationiscomplete.+*Inkexeccase,kimage->startpointstopurgatoryassumingthat+*kernelentryanddtbaddressareembeddedinpurgatoryby+*userspace(kexec-tools).+*Inkexec_filecase,thekernelstartsdirectlywithoutpurgatory.*/--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);+#endifBUG();/* Should never get here. */}
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 <redacted>
Reviewed-by: Ard Biesheuvel <redacted>
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(-)
@@ -166,7 +166,7 @@ struct mz_hdr {uint16_toem_info;/* oem specific */uint16_treserved1[10];/* reserved */uint32_tpeaddr;/* address of pe header */-charmessage[64];/* message to print */+charmessage[];/* message to print */};structmz_reloc{
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 <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Reviewed-by: James Morse <james.morse@arm.com>
---
arch/arm64/Kconfig | 24 ++++++++++++++++++++++++
arch/arm64/kernel/kexec_image.c | 15 +++++++++++++++
2 files changed, 39 insertions(+)
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 <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
---
arch/arm64/kernel/machine_kexec_file.c | 45 ++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
@@ -45,6 +47,32 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)returnkexec_image_post_load_cleanup_default(image);}+/* crng needs to have been initialized for providing kaslr-seed */+staticintrandom_ready;++staticvoidrandom_ready_notified(structrandom_ready_callback*unused)+{+random_ready=1;+}++staticstructrandom_ready_callbackrandom_ready_cb={+.func=random_ready_notified,+};++static__initintinit_random_ready_cb(void)+{+intret;++ret=add_random_ready_callback(&random_ready_cb);+if(ret==-EALREADY)+random_ready=1;+elseif(ret)+pr_warn("failed to add a callback for random_ready\n");++return0;+}+late_initcall(init_random_ready_cb)+staticintsetup_dtb(structkimage*image,unsignedlonginitrd_load_addr,unsignedlonginitrd_len,char*cmdline,unsignedlongcmdline_len,
@@ -53,6 +81,7 @@ static int setup_dtb(struct kimage *image,void*buf=NULL;size_tbuf_size,range_size;intnodeoffset;+u64value;intret;/* check ranges against root's #address-cells and #size-cells */
@@ -85,6 +114,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;
@@ -164,6 +195,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;+gotoout_err;+}+}else{+pr_notice("kaslr-seed won't be fed\n");+}+/* trim a buffer */fdt_pack(buf);*dtb_buf=buf;
From: Frank Rowand <hidden> Date: 2018-09-07 19:54:04
On 09/07/18 01:00, AKASHI Takahiro wrote:
quoted hunk
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
Frank,
Thank you for the comments. I will address all of them, but
have one question:
On Fri, Sep 07, 2018 at 12:53:58PM -0700, Frank Rowand wrote:
On 09/07/18 01:00, AKASHI Takahiro wrote:
quoted
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
+{
+ /* if *_cells >= 2, cells can hold 64-bit values anyway */
+ if ((dt_root_addr_cells == 1) && (base > U32_MAX))
+ return false;
+
+ if ((dt_root_size_cells == 1) && (size > U32_MAX))
+ return false;
Should also check that base + size does not wrap around.
What is the upper limit here?
For instance, #address_cells = <1> and #size_cells = <1>,
and can 'base + size' be over U32_MAX?
Assuming 'not' is quite reasonable, but it seems to me
that devicetree spec doesn't exclude it, as least I couldn't
find any notes about such a case.
(In my understands, #address_cells only restricts a size in 'reg' property.)
Thanks,
-Takahiro Akashi
From: Frank Rowand <hidden> Date: 2018-09-14 01:26:11
I was re-reading this while answering a later email in the thread. After reading
other patches in the series that were not sent to me, I have a better understanding
of the intent behind this patch, and some changes to my previous reply.
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.
This means most or all of my previous suggested function name changes are no longer
useful.
Please add devicetree at vger.kernel.org to the next version of this patch and to
the patches that use the functions in this patch.
On 09/07/18 12:53, Frank Rowand wrote:
On 09/07/18 01:00, AKASHI Takahiro wrote:
quoted
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
Global comment: this code should not be using the variables
dt_root_addr_cells and dt_root_size_cells. These variables are
__initdata.
The code that is using these helpers is acting upon a specific FDT
(copied from initial_boot_params). This code should be getting the
values of the root node's "#address-cells" and "#size-cells" from
the FDT.
Please add comment:
/* helper functions for arm64 kexec */
I'm not entirely sure of what the caller in 12/16 is trying to ensure
with this function.
(1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
does) is make sure that an address and size tuple are consistent with
the root properties "#address-cells" and "#size-cells".
The caller in 12/16 is using this check to validate values for the
properties "linux,elfcorehdr" and "linux,usable-memory-range".
(2) A more complete check _might_ be to ensure that the values also
specify memory that is available to the kernel. This memory is described
by the "reg" property of one or more "/memory" nodes.
This second check is probably what is actually desired.
One possible issue to note is that the binding for "linux,usable-memory-range"
suggests that available memory could be described by an EFI memory map.
I am not familiar with how or when an EFI memory map might exist instead
of the "/memory" nodes.
quoted
+{
+ /* if *_cells >= 2, cells can hold 64-bit values anyway */
+ if ((dt_root_addr_cells == 1) && (base > U32_MAX))
+ return false;
+
+ if ((dt_root_size_cells == 1) && (size > U32_MAX))
+ return false;
Should also check that base + size does not wrap around.
From: Frank Rowand <hidden> Date: 2018-09-14 01:42:41
On 09/09/18 19:38, AKASHI Takahiro wrote:
Frank,
Thank you for the comments. I will address all of them, but
have one question:
On Fri, Sep 07, 2018 at 12:53:58PM -0700, Frank Rowand wrote:
quoted
On 09/07/18 01:00, AKASHI Takahiro wrote:
quoted
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
+{
+ /* if *_cells >= 2, cells can hold 64-bit values anyway */
+ if ((dt_root_addr_cells == 1) && (base > U32_MAX))
+ return false;
+
+ if ((dt_root_size_cells == 1) && (size > U32_MAX))
+ return false;
Should also check that base + size does not wrap around.
What is the upper limit here?
For instance, #address_cells = <1> and #size_cells = <1>,
and can 'base + size' be over U32_MAX?
Assuming 'not' is quite reasonable, but it seems to me
that devicetree spec doesn't exclude it, as least I couldn't
find any notes about such a case.
(In my understands, #address_cells only restricts a size in 'reg' property.)
(See my other reply in this thread a few minutes ago -- the context of the
ranges is essentially what is valid for the "reg" property of the "/memory"
nodes.)
The "Devicetree Specification" does not specify whether a memory range can
wrap around. For example, can a region of size 0x2000 that wraps around
the highest 32 bit address be specified as:
/ memory at 0xfffff000 {
reg = <0xfffff000 0x2000>;
}
or must it be specified as (option 1):
/ memory at 0xfffff000 {
reg = <0xfffff000 0x1000 0x0 0x1000>
}
or (option 2):
/ memory at 0xfffff000 {
reg = <0xfffff000 0x1000>;
}
memory at 0 {
reg = <0x0 0x1000>;
}
I suggest you start a thread on the devicetree specification list asking
that the spec be updated to state whether wrap around is allowed.
From: Frank Rowand <hidden> Date: 2018-09-14 17:19:44
On 09/13/18 18:26, Frank Rowand wrote:
I was re-reading this while answering a later email in the thread. After reading
other patches in the series that were not sent to me, I have a better understanding
of the intent behind this patch, and some changes to my previous reply.
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.
This means most or all of my previous suggested function name changes are no longer
useful.
Please add devicetree at vger.kernel.org to the next version of this patch and to
the patches that use the functions in this patch.
On 09/07/18 12:53, Frank Rowand wrote:
quoted
On 09/07/18 01:00, AKASHI Takahiro wrote:
quoted
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
Global comment: this code should not be using the variables
dt_root_addr_cells and dt_root_size_cells. These variables are
__initdata.
The code that is using these helpers is acting upon a specific FDT
(copied from initial_boot_params). This code should be getting the
values of the root node's "#address-cells" and "#size-cells" from
the FDT.
There will be new functions available soon to return the values of
a node's "#address-cells" and "#size-cells" from an fdt. They are
fdt_address_cells() and fdt_size_cells().
Rob submitted the patch to add them yesterday in "[PATCH 3/3] scripts/dtc:
Update to upstream version v1.4.7-14-gc86da84d30e4" [1]
[1] https://lkml.kernel.org/r/[off-list ref]
-Frank
quoted
Please add comment:
/* helper functions for arm64 kexec */
I'm not entirely sure of what the caller in 12/16 is trying to ensure
with this function.
(1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
does) is make sure that an address and size tuple are consistent with
the root properties "#address-cells" and "#size-cells".
The caller in 12/16 is using this check to validate values for the
properties "linux,elfcorehdr" and "linux,usable-memory-range".
(2) A more complete check _might_ be to ensure that the values also
specify memory that is available to the kernel. This memory is described
by the "reg" property of one or more "/memory" nodes.
This second check is probably what is actually desired.
One possible issue to note is that the binding for "linux,usable-memory-range"
suggests that available memory could be described by an EFI memory map.
I am not familiar with how or when an EFI memory map might exist instead
of the "/memory" nodes.
quoted
quoted
+{
+ /* if *_cells >= 2, cells can hold 64-bit values anyway */
+ if ((dt_root_addr_cells == 1) && (base > U32_MAX))
+ return false;
+
+ if ((dt_root_size_cells == 1) && (size > U32_MAX))
+ return false;
Should also check that base + size does not wrap around.
Frank,
On Fri, Sep 14, 2018 at 10:19:38AM -0700, Frank Rowand wrote:
On 09/13/18 18:26, Frank Rowand wrote:
quoted
I was re-reading this while answering a later email in the thread. After reading
other patches in the series that were not sent to me, I have a better understanding
of the intent behind this patch, and some changes to my previous reply.
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.
This means most or all of my previous suggested function name changes are no longer
useful.
Please add devicetree at vger.kernel.org to the next version of this patch and to
the patches that use the functions in this patch.
On 09/07/18 12:53, Frank Rowand wrote:
quoted
On 09/07/18 01:00, AKASHI Takahiro wrote:
quoted
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
Global comment: this code should not be using the variables
dt_root_addr_cells and dt_root_size_cells. These variables are
__initdata.
The code that is using these helpers is acting upon a specific FDT
(copied from initial_boot_params). This code should be getting the
values of the root node's "#address-cells" and "#size-cells" from
the FDT.
There will be new functions available soon to return the values of
a node's "#address-cells" and "#size-cells" from an fdt. They are
fdt_address_cells() and fdt_size_cells().
Rob submitted the patch to add them yesterday in "[PATCH 3/3] scripts/dtc:
Update to upstream version v1.4.7-14-gc86da84d30e4" [1]
Will this patch go into mainline in v4.20 merge window?
Unfortunately, fdt_addresses.c where fdt_address_cells() and
fdt_size_cells() are defined is NOT compiled in the kernel.
I will submit a patch.
-Takahiro Akashi
-Frank
quoted
quoted
Please add comment:
/* helper functions for arm64 kexec */
I'm not entirely sure of what the caller in 12/16 is trying to ensure
with this function.
(1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
does) is make sure that an address and size tuple are consistent with
the root properties "#address-cells" and "#size-cells".
The caller in 12/16 is using this check to validate values for the
properties "linux,elfcorehdr" and "linux,usable-memory-range".
(2) A more complete check _might_ be to ensure that the values also
specify memory that is available to the kernel. This memory is described
by the "reg" property of one or more "/memory" nodes.
This second check is probably what is actually desired.
One possible issue to note is that the binding for "linux,usable-memory-range"
suggests that available memory could be described by an EFI memory map.
I am not familiar with how or when an EFI memory map might exist instead
of the "/memory" nodes.
quoted
quoted
+{
+ /* if *_cells >= 2, cells can hold 64-bit values anyway */
+ if ((dt_root_addr_cells == 1) && (base > U32_MAX))
+ return false;
+
+ if ((dt_root_size_cells == 1) && (size > U32_MAX))
+ return false;
Should also check that base + size does not wrap around.
From: Frank Rowand <hidden> Date: 2018-09-26 08:10:38
Hi Rob,
On 09/25/18 22:57, AKASHI Takahiro wrote:
Frank,
On Fri, Sep 14, 2018 at 10:19:38AM -0700, Frank Rowand wrote:
quoted
On 09/13/18 18:26, Frank Rowand wrote:
quoted
I was re-reading this while answering a later email in the thread. After reading
other patches in the series that were not sent to me, I have a better understanding
of the intent behind this patch, and some changes to my previous reply.
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.
This means most or all of my previous suggested function name changes are no longer
useful.
Please add devicetree at vger.kernel.org to the next version of this patch and to
the patches that use the functions in this patch.
On 09/07/18 12:53, Frank Rowand wrote:
quoted
On 09/07/18 01:00, AKASHI Takahiro wrote:
quoted
These functions will be used later to handle kexec-specific properties
in arm64's kexec_file implementation.
Signed-off-by: AKASHI Takahiro <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
---
drivers/of/fdt.c | 62 ++++++++++++++++++++++++++++++++++++++++--
include/linux/of_fdt.h | 10 +++++--
2 files changed, 68 insertions(+), 4 deletions(-)
Global comment: this code should not be using the variables
dt_root_addr_cells and dt_root_size_cells. These variables are
__initdata.
The code that is using these helpers is acting upon a specific FDT
(copied from initial_boot_params). This code should be getting the
values of the root node's "#address-cells" and "#size-cells" from
the FDT.
Can you confirm whether "[PATCH 3/3] scripts/dtc: ..." (see below) will
be in the v4.20 pull request?
quoted
There will be new functions available soon to return the values of
a node's "#address-cells" and "#size-cells" from an fdt. They are
fdt_address_cells() and fdt_size_cells().
Rob submitted the patch to add them yesterday in "[PATCH 3/3] scripts/dtc:
Update to upstream version v1.4.7-14-gc86da84d30e4" [1]
Will this patch go into mainline in v4.20 merge window?
Unfortunately, fdt_addresses.c where fdt_address_cells() and
fdt_size_cells() are defined is NOT compiled in the kernel.
I will submit a patch.
-Takahiro Akashi
quoted
-Frank
Thanks,
Frank
quoted
quoted
quoted
Please add comment:
/* helper functions for arm64 kexec */
I'm not entirely sure of what the caller in 12/16 is trying to ensure
with this function.
(1) At the minimum (and what the implementation in of_fdt_cells_size_fitted()
does) is make sure that an address and size tuple are consistent with
the root properties "#address-cells" and "#size-cells".
The caller in 12/16 is using this check to validate values for the
properties "linux,elfcorehdr" and "linux,usable-memory-range".
(2) A more complete check _might_ be to ensure that the values also
specify memory that is available to the kernel. This memory is described
by the "reg" property of one or more "/memory" nodes.
This second check is probably what is actually desired.
One possible issue to note is that the binding for "linux,usable-memory-range"
suggests that available memory could be described by an EFI memory map.
I am not familiar with how or when an EFI memory map might exist instead
of the "/memory" nodes.
quoted
quoted
+{
+ /* if *_cells >= 2, cells can hold 64-bit values anyway */
+ if ((dt_root_addr_cells == 1) && (base > U32_MAX))
+ return false;
+
+ if ((dt_root_size_cells == 1) && (size > U32_MAX))
+ return false;
Should also check that base + size does not wrap around.