diff mbox

[v7,05/11] arm64: kexec_file: create purgatory

Message ID 20171204025801.12161-6-takahiro.akashi@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

AKASHI Takahiro Dec. 4, 2017, 2:57 a.m. UTC
This is a basic purgatory, or a kind of glue code between the two kernels,
for arm64.

Since purgatory is assumed to be relocatable (not executable) object by
kexec generic code, arch_kexec_apply_relocations_add() is required in
general. Arm64's purgatory, however, is a simple asm and all the references
can be resolved as local, no re-linking is needed here.

Please note that even if we don't support digest check at purgatory we
need purgatory_sha_regions and purgatory_sha256_digest as they are
referenced by generic kexec code.

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/Makefile           |  1 +
 arch/arm64/purgatory/Makefile | 24 +++++++++++++++++++
 arch/arm64/purgatory/entry.S  | 55 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+)
 create mode 100644 arch/arm64/purgatory/Makefile
 create mode 100644 arch/arm64/purgatory/entry.S

Comments

James Morse Feb. 7, 2018, 6:37 p.m. UTC | #1
Hi Akashi,

On 04/12/17 02:57, AKASHI Takahiro wrote:
> This is a basic purgatory, or a kind of glue code between the two kernels,
> for arm64.
> 
> Since purgatory is assumed to be relocatable (not executable) object by
> kexec generic code, arch_kexec_apply_relocations_add() is required in
> general. Arm64's purgatory, however, is a simple asm and all the references
> can be resolved as local, no re-linking is needed here.
> 
> Please note that even if we don't support digest check at purgatory we

(You knew what I was going to ask!)


> need purgatory_sha_regions and purgatory_sha256_digest as they are
> referenced by generic kexec code.

As somewhere to store the values? If we aren't doing the validation could we add
something about why not to the commit message? I think its because we only worry
about memory corruption for kdump, and for kdump we unmap the crash-kernel
region during normal-operation to prevent it getting corrupted.

As we aren't doing the hash validation, could we hide its core-code behind some
ARCH_HAS_KEXEC_PURGATORY_HASH, instead of defining dummy symbols and doing
unnecessary work to fill them in?


> diff --git a/arch/arm64/purgatory/entry.S b/arch/arm64/purgatory/entry.S
> new file mode 100644
> index 000000000000..fe6e968076db
> --- /dev/null
> +++ b/arch/arm64/purgatory/entry.S
> @@ -0,0 +1,55 @@
> +/*
> + * kexec core purgatory
> + */
> +#include <linux/linkage.h>
> +#include <uapi/linux/kexec.h>
> +
> +#define SHA256_DIGEST_SIZE	32 /* defined in crypto/sha.h */
> +
> +.text
> +
> +ENTRY(purgatory_start)
> +	/* Start new image. */
> +	ldr	x17, __kernel_entry
> +	ldr	x0, __dtb_addr
> +	mov	x1, xzr
> +	mov	x2, xzr
> +	mov	x3, xzr
> +	br	x17
> +END(purgatory_start)

Is this what arm64_relocate_new_kernel() drops into? I thought that had the
kernel boot register values already so we wouldn't need another trampoline for
kexec_file_load()...

.. but now that I look, it doesn't have the DTB, presumably because for regular
kexec we don't know where user-space put it.

Could we add some x0_for_kexec that is 0 by default (if that's the ABI), or the
DTB address for kexec_file_load()? This would avoid this extra trampoline, and
patching in the values from load_other_segments().

I'd love to avoid an in-kernel purgatory! (its code with funny
compile/link/relocation requirements that is impossible to debug)


Thanks,

James
AKASHI Takahiro Feb. 9, 2018, 12:40 p.m. UTC | #2
On Wed, Feb 07, 2018 at 06:37:16PM +0000, James Morse wrote:
> Hi Akashi,
> 
> On 04/12/17 02:57, AKASHI Takahiro wrote:
> > This is a basic purgatory, or a kind of glue code between the two kernels,
> > for arm64.
> > 
> > Since purgatory is assumed to be relocatable (not executable) object by
> > kexec generic code, arch_kexec_apply_relocations_add() is required in
> > general. Arm64's purgatory, however, is a simple asm and all the references
> > can be resolved as local, no re-linking is needed here.
> > 
> > Please note that even if we don't support digest check at purgatory we
> 
> (You knew what I was going to ask!)

Yes, definitely.

> 
> > need purgatory_sha_regions and purgatory_sha256_digest as they are
> > referenced by generic kexec code.
> 
> As somewhere to store the values? If we aren't doing the validation could we add
> something about why not to the commit message? I think its because we only worry
> about memory corruption for kdump, and for kdump we unmap the crash-kernel
> region during normal-operation to prevent it getting corrupted.
> 
> As we aren't doing the hash validation, could we hide its core-code behind some
> ARCH_HAS_KEXEC_PURGATORY_HASH, instead of defining dummy symbols and doing
> unnecessary work to fill them in?

Yes, this is one idea.
But as you mentioned below, adding a purgatory for arm64's kexec_file
does make little sense as I've already removed digest check code after
MarkR's comment.

> 
> > diff --git a/arch/arm64/purgatory/entry.S b/arch/arm64/purgatory/entry.S
> > new file mode 100644
> > index 000000000000..fe6e968076db
> > --- /dev/null
> > +++ b/arch/arm64/purgatory/entry.S
> > @@ -0,0 +1,55 @@
> > +/*
> > + * kexec core purgatory
> > + */
> > +#include <linux/linkage.h>
> > +#include <uapi/linux/kexec.h>
> > +
> > +#define SHA256_DIGEST_SIZE	32 /* defined in crypto/sha.h */
> > +
> > +.text
> > +
> > +ENTRY(purgatory_start)
> > +	/* Start new image. */
> > +	ldr	x17, __kernel_entry
> > +	ldr	x0, __dtb_addr
> > +	mov	x1, xzr
> > +	mov	x2, xzr
> > +	mov	x3, xzr
> > +	br	x17
> > +END(purgatory_start)
> 
> Is this what arm64_relocate_new_kernel() drops into? I thought that had the
> kernel boot register values already so we wouldn't need another trampoline for
> kexec_file_load()...

Indeed

> .. but now that I look, it doesn't have the DTB, presumably because for regular
> kexec we don't know where user-space put it.
> 
> Could we add some x0_for_kexec that is 0 by default (if that's the ABI), or the

First, I didn't get what you meaned here.
After managing to modify my code, I found that we could re-use
cpu_soft_restart(), especially, the fifth argument, which is currently
contant 0, but we will be able to pass dtb address here.
In turn, we can also use this argument to determine, in relocate_new_kernel(),
whether we should call puragatory (kexec_load) or directly jump into the kernel
(kexec_file_load).


> DTB address for kexec_file_load()? This would avoid this extra trampoline, and
> patching in the values from load_other_segments().
> 
> I'd love to avoid an in-kernel purgatory! (its code with funny
> compile/link/relocation requirements that is impossible to debug)

Lovely!

I really appreicated your valuable comments.
and more on other patches comming?

-Takahiro AKASHI

> 
> Thanks,
> 
> James
diff mbox

Patch

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index b481b4a7c011..0f0742e98c08 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -113,6 +113,7 @@  core-$(CONFIG_XEN) += arch/arm64/xen/
 core-$(CONFIG_CRYPTO) += arch/arm64/crypto/
 libs-y		:= arch/arm64/lib/ $(libs-y)
 core-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
+core-$(CONFIG_KEXEC_FILE) += arch/arm64/purgatory/
 
 # Default target when executing plain make
 boot		:= arch/arm64/boot
diff --git a/arch/arm64/purgatory/Makefile b/arch/arm64/purgatory/Makefile
new file mode 100644
index 000000000000..c2127a2cbd51
--- /dev/null
+++ b/arch/arm64/purgatory/Makefile
@@ -0,0 +1,24 @@ 
+OBJECT_FILES_NON_STANDARD := y
+
+purgatory-y := entry.o
+
+targets += $(purgatory-y)
+PURGATORY_OBJS = $(addprefix $(obj)/,$(purgatory-y))
+
+LDFLAGS_purgatory.ro := -e purgatory_start -r --no-undefined \
+					-nostdlib -z nodefaultlib
+targets += purgatory.ro
+
+$(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
+		$(call if_changed,ld)
+
+targets += kexec_purgatory.c
+
+CMD_BIN2C = $(objtree)/scripts/basic/bin2c
+quiet_cmd_bin2c = BIN2C $@
+	cmd_bin2c = $(CMD_BIN2C) kexec_purgatory < $< > $@
+
+$(obj)/kexec_purgatory.c: $(obj)/purgatory.ro FORCE
+	$(call if_changed,bin2c)
+
+obj-${CONFIG_KEXEC_FILE}	+= kexec_purgatory.o
diff --git a/arch/arm64/purgatory/entry.S b/arch/arm64/purgatory/entry.S
new file mode 100644
index 000000000000..fe6e968076db
--- /dev/null
+++ b/arch/arm64/purgatory/entry.S
@@ -0,0 +1,55 @@ 
+/*
+ * kexec core purgatory
+ */
+#include <linux/linkage.h>
+#include <uapi/linux/kexec.h>
+
+#define SHA256_DIGEST_SIZE	32 /* defined in crypto/sha.h */
+
+.text
+
+ENTRY(purgatory_start)
+	/* Start new image. */
+	ldr	x17, __kernel_entry
+	ldr	x0, __dtb_addr
+	mov	x1, xzr
+	mov	x2, xzr
+	mov	x3, xzr
+	br	x17
+END(purgatory_start)
+
+/*
+ * data section:
+ * kernel_entry and dtb_addr are global but also labelled as local,
+ * "__xxx:", to avoid unwanted re-linking.
+ *
+ * purgatory_sha_regions and purgatory_sha256_digest are referenced
+ * by kexec generic code and so must exist, but not actually used
+ * here because hash check is not that useful in purgatory.
+ */
+.align 3
+
+.globl kernel_entry
+kernel_entry:
+__kernel_entry:
+	.quad	0
+END(kernel_entry)
+
+.globl dtb_addr
+dtb_addr:
+__dtb_addr:
+	.quad	0
+END(dtb_addr)
+
+.globl purgatory_sha_regions
+purgatory_sha_regions:
+	.rept	KEXEC_SEGMENT_MAX
+	.quad	0
+	.quad	0
+	.endr
+END(purgatory_sha_regions)
+
+.globl purgatory_sha256_digest
+purgatory_sha256_digest:
+        .skip   SHA256_DIGEST_SIZE
+END(purgatory_sha256_digest)