James,

On Fri, May 11, 2018 at 06:07:06PM +0100, James Morse wrote:

Hi Akashi,

On 07/05/18 08:21, AKASHI Takahiro wrote:

quoted

On Tue, May 01, 2018 at 06:46:11PM +0100, James Morse wrote:

quoted

On 25/04/18 07:26, AKASHI Takahiro wrote:

quoted

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.

quoted

quoted

quoted

diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
index e4de1223715f..3cba4161818a 100644
--- a/arch/arm64/include/asm/kexec.h
+++ b/arch/arm64/include/asm/kexec.h

@@ -102,6 +102,56 @@ struct kimage_arch {
 	void *dtb_buf;
 };
 
+/**
+ * struct arm64_image_header - arm64 kernel image header
+ *
+ * @pe_sig: Optional PE format 'MZ' signature

To be precise, this is NOT a PE signature but MS-DOS header's magic.
(There is another "PE" signature in PE COFF file header pointed to by
'pe_header'.)
I will correct its name.

quoted

quoted

quoted

+ * @branch_code: Instruction to branch to stext
+ * @text_offset: Image load offset, little endian
+ * @image_size: Effective image size, little endian
+ * @flags:
+ *	Bit 0: Kernel endianness. 0=little endian, 1=big endian

Page size? What about 'phys_base'?, (whatever that is...)
Probably best to refer to Documentation/arm64/booting.txt here, its the
authoritative source of what these fields mean.

While we don't care other bit fields for now, I will add the reference
to the Documentation file.

Thanks, I don't want to create a second, incomplete set of documentation!

I will leave a minimum of description of parameters here.

quoted

quoted

quoted

+	u64 reserved[3];
+	u8 magic[4];
+	u32 pe_header;
+};

I'm surprised we don't have a definition for this already, I guess its always
done in asm. We have kernel/image.h that holds some of this stuff, if we are
going to validate the flags, is it worth adding the code there, (and moving it
to include/asm)?

A comment at the beginning of this file says,
    #ifndef LINKER_SCRIPT
    #error This file should only be included in vmlinux.lds.S
    #endif
Let me think about.

Ah, I missed that.

Having two definitions of something makes me nervous that they can become
different... looks like that header belongs to the linker, and shouldn't be used
here then.

OK.

quoted

quoted

I guess you skip the MZ prefix as its not present for !EFI?

Correct, but MZ checking in probe function is just an informative message.

quoted

CONFIG_KEXEC_IMAGE_VERIFY_SIG depends on the fact that the file
format is PE (that is, EFI is enabled).

So if the signature checking is enabled, its already been checked.

The signature, either MZ or PE, in a file will be actually checked
in verify_pefile_signature().

quoted

quoted

Could we check branch_code is non-zero, and text-offset points within image-size?

We could do it, but I don't think this check is very useful.

quoted

We could check that this platform supports the page-size/endian config that this
Image was built with... We get a message from the EFI stub if the page-size
can't be supported, it would be nice to do the same here (as we can).

There is no restriction on page-size or endianness for kexec.

No, but it won't boot if the hardware doesn't support it. The kernel will spin
at a magic address that is, difficult, to debug without JTAG. The bug report
will be "it didn't boot".

OK.
Added sanity checks for cpu features, endianness as well as page size.

quoted

What will be the purpose of this check?

These values are in the header so that the bootloader can check them, then print
a meaningful error. Here, kexec_file_load() is playing the part of the bootloader.

I'm assuming kexec_file_load() can only be used to kexec linux... unlike regular
kexec. Is this where I'm going wrong?

quoted

quoted

quoted

diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
new file mode 100644
index 000000000000..4dd524ad6611
--- /dev/null
+++ b/arch/arm64/kernel/kexec_image.c

@@ -0,0 +1,79 @@

quoted

+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 kexec_buf kbuf;
+	struct arm64_image_header *h = (struct arm64_image_header *)kernel;
+	unsigned long text_offset;
+	int ret;
+
+	/* 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.memsz = le64_to_cpu(h->image_size);
+	text_offset = le64_to_cpu(h->text_offset);
+	kbuf.buf_align = SZ_2M;

quoted

+	/* Adjust kernel segment with TEXT_OFFSET */
+	kbuf.memsz += text_offset;
+
+	ret = kexec_add_buffer(&kbuf);
+	if (ret)
+		goto out;
+
+	image->arch.kern_segment = image->nr_segments - 1;

You only seem to use kern_segment here, and in load_other_segments() called
below. Could it not be a local variable passed in? Instead of arch-specific data
we keep forever?

No, kern_segment is also used in load_other_segments() in machine_kexec_file.c.
To optimize memory hole allocation logic in locate_mem_hole_callback(),
we need to know the exact range of kernel image (start and end).

That's the second user. My badly-made point is one calls the other, but passes
the data via some until-kexec lifetime struct. (its not important, just an
indicator this worked differently in the past and hasn't been cleaned up).
I meant something like [0].

OK, but instead of adding kern_seg, I want to change the interface to:

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

This way, we will in future be able to address an issue I mentioned in
my previous e-mail. (If we support vmlinux, the kernel occupies two segments
for text and data, respectively.)

Thanks,
-Takahiro AKASHI