| Message ID | 20180623022058.10935-9-takahiro.akashi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Akashi, On 23/06/18 03:20, AKASHI Takahiro wrote: > 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(). > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > index c38a8048ed00..7115c4f915dc 100644 > --- a/arch/arm64/kernel/machine_kexec_file.c > +++ b/arch/arm64/kernel/machine_kexec_file.c > +static int setup_dtb(struct kimage *image, > + unsigned long initrd_load_addr, unsigned long initrd_len, > + char *cmdline, unsigned long cmdline_len, > + char **dtb_buf, size_t *dtb_buf_len) > +{ > + char *buf = NULL; > + size_t buf_size; > + int nodeoffset; > + u64 value; > + int ret; > + > + /* duplicate dt blob */ > + buf_size = fdt_totalsize(initial_boot_params); > + > + if (initrd_load_addr) { > + buf_size += fdt_prop_len("linux,initrd-start", sizeof(u64)); > + buf_size += fdt_prop_len("linux,initrd-end", sizeof(u64)); > + } > + > + if (cmdline) > + buf_size += fdt_prop_len("bootargs", cmdline_len + 1); (a comment here about the possibility of an embedded NULL may avoid surprises later) > + buf = vmalloc(buf_size); > + if (!buf) { > + ret = -ENOMEM; > + goto out_err; > + } > + > + ret = fdt_open_into(initial_boot_params, buf, buf_size); > + if (ret) > + goto out_err; > + > + nodeoffset = fdt_path_offset(buf, "/chosen"); > + if (nodeoffset < 0) > + goto out_err; This doesn't update 'ret', so we return the 0/success from above... > + > + /* add bootargs */ > + if (cmdline) { > + ret = fdt_setprop(buf, nodeoffset, "bootargs", > + cmdline, cmdline_len + 1); > + if (ret) > + goto out_err; > + } So (cmdline_len == 0) from user-space means keep the old cmdline. Sounds sensible. Is this documented anywhere? powerpc does the opposite, it deletes the bootargs in this case. Are we happy making his a per-arch thing? > + /* add initrd-* */ > + if (initrd_load_addr) { > + value = cpu_to_fdt64(initrd_load_addr); > + ret = fdt_setprop_u64(buf, nodeoffset, "linux,initrd-start", > + value); > + if (ret) > + goto out_err; > + > + value = cpu_to_fdt64(initrd_load_addr + initrd_len); > + ret = fdt_setprop_u64(buf, nodeoffset, "linux,initrd-end", > + value); > + if (ret) > + goto out_err; > + } Won't this do the same with the initrd? Re-use the old values if no new one was provided? I can't find anything that deletes the property by the time we come to kexec, and the old physical addresses may have been re-used by who-knows-what. (powerpc has some code to: /* If there's no new initrd, delete the old initrd's info. */) Thanks, James
James, On Tue, Jul 03, 2018 at 05:32:09PM +0100, James Morse wrote: > Hi Akashi, > > On 23/06/18 03:20, AKASHI Takahiro wrote: > > 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(). > > > > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > > index c38a8048ed00..7115c4f915dc 100644 > > --- a/arch/arm64/kernel/machine_kexec_file.c > > +++ b/arch/arm64/kernel/machine_kexec_file.c > > > +static int setup_dtb(struct kimage *image, > > + unsigned long initrd_load_addr, unsigned long initrd_len, > > + char *cmdline, unsigned long cmdline_len, > > + char **dtb_buf, size_t *dtb_buf_len) > > +{ > > + char *buf = NULL; > > + size_t buf_size; > > + int nodeoffset; > > + u64 value; > > + int ret; > > + > > + /* duplicate dt blob */ > > + buf_size = fdt_totalsize(initial_boot_params); > > + > > + if (initrd_load_addr) { > > + buf_size += fdt_prop_len("linux,initrd-start", sizeof(u64)); > > + buf_size += fdt_prop_len("linux,initrd-end", sizeof(u64)); > > + } > > + > > + if (cmdline) > > + buf_size += fdt_prop_len("bootargs", cmdline_len + 1); > > (a comment here about the possibility of an embedded NULL may avoid surprises later) > > > > + buf = vmalloc(buf_size); > > + if (!buf) { > > + ret = -ENOMEM; > > + goto out_err; > > + } > > + > > + ret = fdt_open_into(initial_boot_params, buf, buf_size); > > + if (ret) > > + goto out_err; > > + > > + nodeoffset = fdt_path_offset(buf, "/chosen"); > > + if (nodeoffset < 0) > > + goto out_err; > > This doesn't update 'ret', so we return the 0/success from above... OK. Will fix it. > > > + > > + /* add bootargs */ > > + if (cmdline) { > > + ret = fdt_setprop(buf, nodeoffset, "bootargs", > > + cmdline, cmdline_len + 1); > > + if (ret) > > + goto out_err; > > + } > > So (cmdline_len == 0) from user-space means keep the old cmdline. Sounds > sensible. Is this documented anywhere? Very good catch, James. To be a bit surprise, --append (and --cmdline) option belongs to arch- specific options in terms of implementation. Apparently, a standard man page of kexec(8) say little about it, in particular, for device-tree-based system. As far as arm64 is concerned, the fact is a bit complicated. User space kexec accepts --append as well as --reuse-cmdline, and along with yet another --dtb option, a resulting command line for new kernel (or bootargs in new dtb) would look to be: --append=A | --reuse-cmdline | --dtb | | n | y(bootargs=B) n n S(*1) B(*2) n y S S(*3) y n A A y y S+A S+A(*4) where S: the cmdline parameters of the running system (equal to bootargs in system's dtb) You are talking about case(1) above. Given that we can have an explicit option, --reuse-cmdline, the cmdline in (1) should be NULL. Meanwhile, we specify --dtb here, which means that we want to re-use system's dtb, implying that we also want to reuse a cmdline parameter. (This can be arguable, though) So I would say that we have both reasons to go for and go against. # Likewise, # I wonder why the cmdnline would not be B or A+B, respectively, in case of (3) and (4). But it's a different issue. > powerpc does the opposite, it deletes the bootargs in this case. Are we happy > making his a per-arch thing? My compromise solution is: a.to maintain compatibility with powerpc at system call level, that is, replacing bootargs in new dtb if user explicitly specifies cmdline argument, otherwise nullifying bootargs, b.yet maintain compatibility with arm64's kexec behavior at command line interface level. If neither --append nor --dtb is not specified, user space kexec will reuse the system's command line whether or not --reuse-cmdline is used. (Do you follow me?) So kexec and kexec_file on arm64 will still behave in exactly the same way, but differently from ppc at command level for now. The point is that, if we might want or need to change this behavior (at any time in the future), we would only have to modify user space kexec. Kernel semantics will never break. (b) requires additional small modification on kexec-tools, but kexec_file support is yet to be upstreamed anyway. > > > + /* add initrd-* */ > > + if (initrd_load_addr) { > > + value = cpu_to_fdt64(initrd_load_addr); > > + ret = fdt_setprop_u64(buf, nodeoffset, "linux,initrd-start", > > + value); > > + if (ret) > > + goto out_err; > > + > > + value = cpu_to_fdt64(initrd_load_addr + initrd_len); > > + ret = fdt_setprop_u64(buf, nodeoffset, "linux,initrd-end", > > + value); > > + if (ret) > > + goto out_err; > > + } > > Won't this do the same with the initrd? Re-use the old values if no new one was > provided? I can't find anything that deletes the property by the time we come to > kexec, and the old physical addresses may have been re-used by who-knows-what. > > (powerpc has some code to: /* If there's no new initrd, delete the old initrd's > info. */) Yeah, I think that deleting properties would be a right way to go. Will fix. Thanks, -Takahiro AKASHI > > > Thanks, > > James
Hi Akashi, On 10/07/18 08:37, AKASHI Takahiro wrote: > On Tue, Jul 03, 2018 at 05:32:09PM +0100, James Morse wrote: >> On 23/06/18 03:20, AKASHI Takahiro wrote: >>> 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(). >>> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c >>> index c38a8048ed00..7115c4f915dc 100644 >>> --- a/arch/arm64/kernel/machine_kexec_file.c >>> +++ b/arch/arm64/kernel/machine_kexec_file.c >> >>> +static int setup_dtb(struct kimage *image, >>> + unsigned long initrd_load_addr, unsigned long initrd_len, >>> + char *cmdline, unsigned long cmdline_len, >>> + char **dtb_buf, size_t *dtb_buf_len) >>> +{ [..] >>> + /* add bootargs */ >>> + if (cmdline) { >>> + ret = fdt_setprop(buf, nodeoffset, "bootargs", >>> + cmdline, cmdline_len + 1); >>> + if (ret) >>> + goto out_err; >>> + } >> >> So (cmdline_len == 0) from user-space means keep the old cmdline. Sounds >> sensible. Is this documented anywhere? > To be a bit surprise, --append (and --cmdline) option belongs to arch- > specific options in terms of implementation. Apparently, a standard man page > of kexec(8) say little about it, in particular, for device-tree-based system. > > As far as arm64 is concerned, the fact is a bit complicated. > User space kexec accepts --append as well as --reuse-cmdline, and > along with yet another --dtb option, a resulting command line for new > kernel (or bootargs in new dtb) would look to be: > > --append=A | --reuse-cmdline | --dtb > | | n | y(bootargs=B) > > n n S(*1) B(*2) > n y S S(*3) > y n A A > y y S+A S+A(*4) > > where S: the cmdline parameters of the running system > (equal to bootargs in system's dtb) (I'm afraid I don't understand this table, but the text below helps...) > You are talking about case(1) above. > > Given that we can have an explicit option, --reuse-cmdline, the cmdline > in (1) should be NULL. Meanwhile, we specify --dtb here, which means > that we want to re-use system's dtb, implying that we also want to > reuse a cmdline parameter. (This can be arguable, though) Sounds like this is all user-space's problem! > So I would say that we have both reasons to go for and go against. > > # Likewise, > # I wonder why the cmdnline would not be B or A+B, respectively, > in case of (3) and (4). But it's a different issue. > >> powerpc does the opposite, it deletes the bootargs in this case. Are we happy >> making his a per-arch thing? > > My compromise solution is: > a.to maintain compatibility with powerpc at system call level, This sounds like the right thing to do. > that is, > replacing bootargs in new dtb if user explicitly specifies cmdline > argument, otherwise nullifying bootargs, > b.yet maintain compatibility with arm64's kexec behavior at command line > interface level. If neither --append nor --dtb is not specified, > user space kexec will reuse the system's command line whether or not > --reuse-cmdline is used. (a and b aren't options this time: you're proposing to do a-and-b) > (Do you follow me?) I think so: The syscall will behave the same as powerpc meaning the kernel will never re-use the existing cmdline in the dtb, even if that means (trying to) boot without one. If user-space wants to re-use the command line, it should read /proc/cmdline and feed the string back into the kernel. > So kexec and kexec_file on arm64 will still behave in exactly the same way, > but differently from ppc at command level for now. > The point is that, if we might want or need to change this behavior > (at any time in the future), we would only have to modify user space kexec. > Kernel semantics will never break. > > (b) requires additional small modification on kexec-tools, but kexec_file > support is yet to be upstreamed anyway. Makes sense! Thanks, James
On Tue, Jul 10, 2018 at 04:25:03PM +0100, James Morse wrote: > Hi Akashi, > > On 10/07/18 08:37, AKASHI Takahiro wrote: > > On Tue, Jul 03, 2018 at 05:32:09PM +0100, James Morse wrote: > >> On 23/06/18 03:20, AKASHI Takahiro wrote: > >>> 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(). > > >>> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > >>> index c38a8048ed00..7115c4f915dc 100644 > >>> --- a/arch/arm64/kernel/machine_kexec_file.c > >>> +++ b/arch/arm64/kernel/machine_kexec_file.c > >> > >>> +static int setup_dtb(struct kimage *image, > >>> + unsigned long initrd_load_addr, unsigned long initrd_len, > >>> + char *cmdline, unsigned long cmdline_len, > >>> + char **dtb_buf, size_t *dtb_buf_len) > >>> +{ > > [..] > > >>> + /* add bootargs */ > >>> + if (cmdline) { > >>> + ret = fdt_setprop(buf, nodeoffset, "bootargs", > >>> + cmdline, cmdline_len + 1); > >>> + if (ret) > >>> + goto out_err; > >>> + } > >> > >> So (cmdline_len == 0) from user-space means keep the old cmdline. Sounds > >> sensible. Is this documented anywhere? > > > To be a bit surprise, --append (and --cmdline) option belongs to arch- > > specific options in terms of implementation. Apparently, a standard man page > > of kexec(8) say little about it, in particular, for device-tree-based system. > > > > As far as arm64 is concerned, the fact is a bit complicated. > > User space kexec accepts --append as well as --reuse-cmdline, and > > along with yet another --dtb option, a resulting command line for new > > kernel (or bootargs in new dtb) would look to be: > > > > --append=A | --reuse-cmdline | --dtb > > | | n | y(bootargs=B) > > > > n n S(*1) B(*2) > > n y S S(*3) > > y n A A > > y y S+A S+A(*4) > > > > where S: the cmdline parameters of the running system > > (equal to bootargs in system's dtb) > > (I'm afraid I don't understand this table, but the text below helps...) (Really? I believed that it was obvious.) > > > You are talking about case(1) above. > > > > Given that we can have an explicit option, --reuse-cmdline, the cmdline > > in (1) should be NULL. Meanwhile, we specify --dtb here, which means > > that we want to re-use system's dtb, implying that we also want to > > reuse a cmdline parameter. (This can be arguable, though) > > Sounds like this is all user-space's problem! I dare not say it's a bug :) > > > So I would say that we have both reasons to go for and go against. > > > > # Likewise, > > # I wonder why the cmdnline would not be B or A+B, respectively, > > in case of (3) and (4). But it's a different issue. > > > >> powerpc does the opposite, it deletes the bootargs in this case. Are we happy > >> making his a per-arch thing? > > > > My compromise solution is: > > a.to maintain compatibility with powerpc at system call level, > > This sounds like the right thing to do. > > > > that is, > > replacing bootargs in new dtb if user explicitly specifies cmdline > > argument, otherwise nullifying bootargs, > > b.yet maintain compatibility with arm64's kexec behavior at command line > > interface level. If neither --append nor --dtb is not specified, > > user space kexec will reuse the system's command line whether or not > > --reuse-cmdline is used. > > (a and b aren't options this time: you're proposing to do a-and-b) Yes. > > > (Do you follow me?) > > I think so: > The syscall will behave the same as powerpc meaning the kernel will never re-use > the existing cmdline in the dtb, even if that means (trying to) boot without one. > > If user-space wants to re-use the command line, it should read /proc/cmdline and > feed the string back into the kernel. Thank you for rephrasing. > > > So kexec and kexec_file on arm64 will still behave in exactly the same way, > > but differently from ppc at command level for now. > > > The point is that, if we might want or need to change this behavior > > (at any time in the future), we would only have to modify user space kexec. > > Kernel semantics will never break. > > > > (b) requires additional small modification on kexec-tools, but kexec_file > > support is yet to be upstreamed anyway. > > Makes sense! The next version will go with this approach. -Takahiro AKASHI > > Thanks, > > James
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h index e17f0529a882..01bbf6cebf12 100644 --- a/arch/arm64/include/asm/kexec.h +++ b/arch/arm64/include/asm/kexec.h @@ -93,6 +93,22 @@ 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 { + phys_addr_t dtb_mem; + void *dtb_buf; +}; + +struct kimage; + +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..7115c4f915dc 100644 --- a/arch/arm64/kernel/machine_kexec_file.c +++ b/arch/arm64/kernel/machine_kexec_file.c @@ -5,12 +5,167 @@ * 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> const struct kexec_file_ops * const kexec_file_loaders[] = { NULL }; + +int arch_kimage_file_post_load_cleanup(struct kimage *image) +{ + vfree(image->arch.dtb_buf); + image->arch.dtb_buf = 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, + char **dtb_buf, size_t *dtb_buf_len) +{ + char *buf = NULL; + size_t buf_size; + int nodeoffset; + u64 value; + int ret; + + /* duplicate dt blob */ + buf_size = fdt_totalsize(initial_boot_params); + + if (initrd_load_addr) { + buf_size += fdt_prop_len("linux,initrd-start", sizeof(u64)); + buf_size += fdt_prop_len("linux,initrd-end", sizeof(u64)); + } + + if (cmdline) + buf_size += fdt_prop_len("bootargs", cmdline_len + 1); + + buf = vmalloc(buf_size); + if (!buf) { + ret = -ENOMEM; + goto out_err; + } + + ret = fdt_open_into(initial_boot_params, buf, buf_size); + if (ret) + goto out_err; + + nodeoffset = fdt_path_offset(buf, "/chosen"); + if (nodeoffset < 0) + goto out_err; + + /* add bootargs */ + if (cmdline) { + ret = fdt_setprop(buf, nodeoffset, "bootargs", + cmdline, cmdline_len + 1); + if (ret) + goto out_err; + } + + /* add initrd-* */ + if (initrd_load_addr) { + value = cpu_to_fdt64(initrd_load_addr); + ret = fdt_setprop_u64(buf, nodeoffset, "linux,initrd-start", + value); + if (ret) + goto out_err; + + value = cpu_to_fdt64(initrd_load_addr + initrd_len); + ret = fdt_setprop_u64(buf, nodeoffset, "linux,initrd-end", + value); + if (ret) + 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; + unsigned long initrd_load_addr = 0; + char *dtb = NULL; + unsigned long dtb_len = 0; + 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.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.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_mem = kbuf.mem; + image->arch.dtb_buf = dtb; + + 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; +}
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> --- arch/arm64/include/asm/kexec.h | 16 +++ arch/arm64/kernel/machine_kexec_file.c | 155 +++++++++++++++++++++++++ 2 files changed, 171 insertions(+)