LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: James Morse <james.morse@arm.com>
To: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: catalin.marinas@arm.com, will.deacon@arm.com,
	dhowells@redhat.com, vgoyal@redhat.com,
	herbert@gondor.apana.org.au, davem@davemloft.net,
	dyoung@redhat.com, bhe@redhat.com, arnd@arndb.de,
	ard.biesheuvel@linaro.org, bhsharma@redhat.com,
	kexec@lists.infradead.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 05/11] arm64: kexec_file: load initrd and device-tree
Date: Tue, 15 May 2018 17:20:00 +0100	[thread overview]
Message-ID: <c10624c0-9962-af38-1dd5-656902180ace@arm.com> (raw)
In-Reply-To: <20180425062629.29404-6-takahiro.akashi@linaro.org>

Hi Akashi,

On 25/04/18 07:26, 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 a boot loader is reused.

(Nit: "a boot loader" -> "the original boot loader")

> 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 f9ebf54ca247..b3b9b1725d8a 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -13,7 +13,26 @@
>  #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>
> +
> +static int __dt_root_addr_cells;
> +static int __dt_root_size_cells;

> @@ -55,3 +74,144 @@ int arch_kexec_walk_mem(struct kexec_buf *kbuf,
>  
>  	return ret;
>  }
> +
> +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 range_len;
> +	int ret;
> +
> +	/* duplicate dt blob */
> +	buf_size = fdt_totalsize(initial_boot_params);
> +	range_len = (__dt_root_addr_cells + __dt_root_size_cells) * sizeof(u32);

These two cells values are 0 here. Did you want
arch_kexec_file_init() in patch 7 in this patch?

Ah, range_len isn't used, so, did you want the cells values and this range_len
thing in in patch 7!?


> +
> +	if (initrd_load_addr)
> +		buf_size += fdt_prop_len("linux,initrd-start", sizeof(u64))
> +				+ fdt_prop_len("linux,initrd-end", sizeof(u64));
> +
> +	if (cmdline)
> +		buf_size += fdt_prop_len("bootargs", cmdline_len + 1);

I can't find where fdt_prop_len() .... oh, patch 7. fdt_prop_len() doesn't look
like the sort of thing that should be created here, but I agree there isn't an
existing API to do this.

(This must be why powerpc guesses that the fdt won't be more than double in size).


> +	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);

fdt_setprop_string()?


> +		if (ret)
> +			goto out_err;
> +	}
> +
> +	/* add initrd-* */
> +	if (initrd_load_addr) {
> +		value = cpu_to_fdt64(initrd_load_addr);
> +		ret = fdt_setprop(buf, nodeoffset, "linux,initrd-start",
> +				&value, sizeof(value));

sizeof(value) was assumed to be the same as sizeof(u64) earlier.
fdt_setprop_u64()?


> +		if (ret)
> +			goto out_err;
> +
> +		value = cpu_to_fdt64(initrd_load_addr + initrd_len);
> +		ret = fdt_setprop(buf, nodeoffset, "linux,initrd-end",
> +				&value, sizeof(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;
> +}

While powerpc has some similar code for updating the initrd and cmdline, it
makes different assumptions about the size of the dt, and has different behavior
for memreserve. (looks like we don't expect the initramfs to be memreserved).
Lets leave unifying that stuff where possible for the future.


> +int load_other_segments(struct kimage *image,
> +			char *initrd, unsigned long initrd_len,
> +			char *cmdline, unsigned long cmdline_len)
> +{
> +	struct kexec_segment *kern_seg;
> +	struct kexec_buf kbuf;
> +	unsigned long initrd_load_addr = 0;
> +	char *dtb = NULL;
> +	unsigned long dtb_len = 0;
> +	int ret = 0;
> +
> +	kern_seg = &image->segment[image->arch.kern_segment];
> +	kbuf.image = image;
> +	/* not allocate anything below the kernel */
> +	kbuf.buf_min = kern_seg->mem + kern_seg->memsz;

> +	/* load initrd */
> +	if (initrd) {
> +		kbuf.buffer = initrd;
> +		kbuf.bufsz = initrd_len;
> +		kbuf.memsz = initrd_len;

> +		kbuf.buf_align = 0;

I'm surprised there initrd has no alignment requirement, but kexec_add_buffer()
rounds this up to PAGE_SIZE.


> +		/* within 1GB-aligned window of up to 32GB in size */
> +		kbuf.buf_max = round_down(kern_seg->mem, 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);
> +	image->arch.dtb_buf = NULL;

Won't kimage_file_post_load_cleanup() always be called if we return an error
here? Why not leave the free()ing until then?


> +	return ret;
> +}



Thanks,

James

  reply	other threads:[~2018-05-15 16:23 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-25  6:26 [PATCH v9 00/11] arm64: kexec: add kexec_file_load() support AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 01/11] asm-generic: add kexec_file_load system call to unistd.h AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 02/11] kexec_file: make kexec_image_post_load_cleanup_default() global AKASHI Takahiro
2018-04-28  9:45   ` Dave Young
2018-05-01 17:46   ` James Morse
2018-05-07  4:40     ` AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 03/11] arm64: kexec_file: invoke the kernel without purgatory AKASHI Takahiro
2018-05-01 17:46   ` James Morse
2018-05-07  5:22     ` AKASHI Takahiro
2018-05-11 17:03       ` James Morse
2018-05-15  4:45         ` AKASHI Takahiro
2018-05-15 16:15           ` James Morse
2018-05-18  6:22             ` AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 04/11] arm64: kexec_file: allocate memory walking through memblock list AKASHI Takahiro
2018-05-01 17:46   ` James Morse
2018-05-07  5:59     ` AKASHI Takahiro
2018-05-15  4:35       ` AKASHI Takahiro
2018-05-15 16:17         ` James Morse
2018-05-17  2:10       ` Baoquan He
2018-05-17  2:15         ` Baoquan He
2018-05-17 18:04           ` James Morse
2018-05-18  1:37             ` Baoquan He
2018-05-18  5:07               ` AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 05/11] arm64: kexec_file: load initrd and device-tree AKASHI Takahiro
2018-05-15 16:20   ` James Morse [this message]
2018-05-18  7:11     ` AKASHI Takahiro
2018-05-18  7:42       ` AKASHI Takahiro
2018-05-18 15:59         ` James Morse
2018-04-25  6:26 ` [PATCH v9 06/11] arm64: kexec_file: allow for loading Image-format kernel AKASHI Takahiro
2018-05-01 17:46   ` James Morse
2018-05-07  7:21     ` AKASHI Takahiro
2018-05-11 17:07       ` James Morse
2018-05-15  5:13         ` AKASHI Takahiro
2018-05-15 17:14           ` James Morse
2018-05-21  9:32             ` AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 07/11] arm64: kexec_file: add crash dump support AKASHI Takahiro
2018-05-15 17:11   ` James Morse
2018-05-16  8:34     ` James Morse
2018-05-18  9:58       ` AKASHI Takahiro
2018-05-16 10:06     ` James Morse
2018-05-18  9:50       ` AKASHI Takahiro
2018-05-18 10:39     ` AKASHI Takahiro
2018-05-18 16:00       ` James Morse
2018-05-21  9:46         ` AKASHI Takahiro
2018-05-15 17:12   ` James Morse
2018-05-18 15:35     ` Rob Herring
2018-05-21 10:14       ` AKASHI Takahiro
2018-05-24 14:25         ` Rob Herring
2018-04-25  6:26 ` [PATCH v9 08/11] arm64: enable KEXEC_FILE config AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 09/11] include: pe.h: remove message[] from mz header definition AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 10/11] arm64: kexec_file: add kernel signature verification support AKASHI Takahiro
2018-04-25  6:26 ` [PATCH v9 11/11] arm64: kexec_file: add kaslr support AKASHI Takahiro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c10624c0-9962-af38-1dd5-656902180ace@arm.com \
    --to=james.morse@arm.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=arnd@arndb.de \
    --cc=bhe@redhat.com \
    --cc=bhsharma@redhat.com \
    --cc=catalin.marinas@arm.com \
    --cc=davem@davemloft.net \
    --cc=dhowells@redhat.com \
    --cc=dyoung@redhat.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=kexec@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=takahiro.akashi@linaro.org \
    --cc=vgoyal@redhat.com \
    --cc=will.deacon@arm.com \
    --subject='Re: [PATCH v9 05/11] arm64: kexec_file: load initrd and device-tree' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).