| Message ID | 20171204025801.12161-6-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: kexec: add kexec_file_load() support | expand |
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
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 --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)
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 -- 2.14.1