<html><head></head><body><div class="ydpb2eae16ayahoo-style-wrap" style="font-family:Helvetica Neue, Helvetica, Arial, sans-serif;font-size:13px;"><div></div>
<div><br></div><div><br></div>
</div><div id="ydpb8ccb2eayahoo_quoted_8406838175" class="ydpb8ccb2eayahoo_quoted">
<div style="font-family:'Helvetica Neue', Helvetica, Arial, sans-serif;font-size:13px;color:#26282a;">
<div>
On Wednesday, October 25, 2023 at 04:58:20 AM CDT, Baoquan He <bhe@redhat.com> wrote:
</div>
<div><br></div>
<div><br></div>
<div>On 10/24/23 at 03:17pm, Arnd Bergmann wrote:<br clear="none">> On Tue, Oct 24, 2023, at 14:44, Baoquan He wrote:<br clear="none">> > Just add people and mailing list to CC since I didn't find this mail in<br clear="none">> > my box, just drag it via 'b4 am'.<br clear="none">> ><br clear="none">> > On 10/23/23 at 01:01pm, Arnd Bergmann wrote:<br clear="none">> > ......<br clear="none">> <br clear="none">> >> diff --git a/kernel/Kconfig.kexec b/kernel/Kconfig.kexec<br clear="none">> >> index 7aff28ded2f48..bfc636d64ff2b 100644<br clear="none">> >> --- a/kernel/Kconfig.kexec<br clear="none">> >> +++ b/kernel/Kconfig.kexec<br clear="none">> >> @@ -36,6 +36,7 @@ config KEXEC<br clear="none">> >> config KEXEC_FILE<br clear="none">> >> bool "Enable kexec file based system call"<br clear="none">> >> depends on ARCH_SUPPORTS_KEXEC_FILE<br clear="none">> >> + depends on CRYPTO_SHA256=y || !ARCH_SUPPORTS_KEXEC_PURGATORY<br clear="none">> ><br clear="none">> > I am not sure if the logic is correct. In theory, kexec_file code<br clear="none">> > utilizes purgatory to verify the checksum digested during kernel loading<br clear="none">> > when try to jump to the kernel. That means kexec_file depends on<br clear="none">> > purgatory, but not contrary?<br clear="none">> <br clear="none">> The expression I wrote is a bit confusing, but I think this just<br clear="none">> keeps the existing behavior:<br clear="none">> <br clear="none">> - on architectures that select ARCH_SUPPORTS_KEXEC_PURGATORY<br clear="none">> (powerpc, riscv, s390 and x86), we also require CRYPTO_SHA256<br clear="none">> to be built-in.<br clear="none">> - on architectures that do not have ARCH_SUPPORTS_KEXEC_PURGATORY<br clear="none">> (arm64 and parisc), CRYPTO_SHA256 is not used and can be disabled<br clear="none">> or =m.<br clear="none">> <br clear="none">> Since ARCH_SUPPORTS_KEXEC_PURGATORY is a 'bool' symbol, it could<br clear="none">> be written as<br clear="none">> <br clear="none">> depends on (ARCH_SUPPORTS_KEXEC_PURGATORY && CRYPTO_SHA256=y) \<br clear="none">> || !ARCH_SUPPORTS_KEXEC_PURGATORY<br clear="none"><br clear="none">Yes, this seems to be clearer to me. Thanks.<br clear="none"><br clear="none">> <br clear="none">> if you find that clearer. I see that the second patch<br clear="none">> actually gets this wrong, it should actually do<br clear="none">> <br clear="none">> select CRYPTO if ARCH_SUPPORTS_KEXEC_PURGATORY<br clear="none">> select CRYPTO_SHA256 if ARCH_SUPPORTS_KEXEC_PURGATORY<br clear="none"><br clear="none">Yeah, makes sense to me.<br clear="none"><br clear="none">Hi Eric,<br clear="none"><br clear="none"><div>Do you have comment about these?</div><div><br></div><div dir="ltr" data-setdir="false">[eric]: The original goal of the conversion patches was to consolidate, but keep existing behavior.</div><div dir="ltr" data-setdir="false">Then I had to break existing behavior a bit by tying CRASH to KEXEC. I was hoping to have</div><div dir="ltr" data-setdir="false">avoided introducing new problems, but looks like there was an escape. At any rate, I think</div><div dir="ltr" data-setdir="false">the ideas here are on track and necessary. Unfortunately at the moment I'm not at a place</div><div dir="ltr" data-setdir="false">where I can test.<br></div><br clear="none">> <br clear="none">> > With these changes, we can achieve the goal to avoid building issue,<br clear="none">> > whereas the code logic becomes confusing. E.g people could disable<br clear="none">> > CONFIG_KEXEC_FILE, but still get purgatory code built in which is<br clear="none">> > totally useless.<br clear="none">> ><br clear="none">> > Not sure if I think too much over this.<br clear="none">> <br clear="none">> I see your point here, and I would suggest changing the<br clear="none">> CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY symbol to just indicate<br clear="none">> the availability of the purgatory code for the arch, rather<br clear="none">> than actually controlling the code itself. I already mentioned<br clear="none">> this for s390, but riscv would need the same thing on top.<br clear="none">> <br clear="none">> I think the change below should address your concern.<br clear="none">> <br clear="none">> Arnd<br clear="none">> <br clear="none">> diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c<br clear="none">> index e60fbd8660c4..3ac341d296db 100644<br clear="none">> --- a/arch/riscv/kernel/elf_kexec.c<br clear="none">> +++ b/arch/riscv/kernel/elf_kexec.c<br clear="none">> @@ -266,7 +266,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,<br clear="none">> cmdline = modified_cmdline;<br clear="none">> }<br clear="none">> <br clear="none">> -#ifdef CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY<br clear="none">> +#ifdef CONFIG_KEXEC_FILE<br clear="none">> /* Add purgatory to the image */<br clear="none">> kbuf.top_down = true;<br clear="none">> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;<br clear="none">> @@ -280,7 +280,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,<br clear="none">> sizeof(kernel_start), 0);<br clear="none">> if (ret)<br clear="none">> pr_err("Error update purgatory ret=%d\n", ret);<br clear="none">> -#endif /* CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY */<br clear="none">> +#endif /* CONFIG_KEXEC_FILE */<br clear="none"><br clear="none">If so, we don't need the CONFIG_KEXEC_FILE ifdeffery because the<br clear="none">file elf_kexec.c relied on CONFIG_KEXEC_FILE enabling to build in.<br clear="none">We can just remove the "#ifdef CONFIG_KEXEC_FILE..#endif" as x86 does.<div class="ydpb8ccb2eayqt2550855536" id="ydpb8ccb2eayqtfd10119"><br clear="none"><br clear="none">> <br clear="none">> /* Add the initrd to the image */<br clear="none">> if (initrd != NULL) {<br clear="none">> diff --git a/arch/riscv/Kbuild b/arch/riscv/Kbuild<br clear="none">> index d25ad1c19f88..ab181d187c23 100644<br clear="none">> --- a/arch/riscv/Kbuild<br clear="none">> +++ b/arch/riscv/Kbuild<br clear="none">> @@ -5,7 +5,7 @@ obj-$(CONFIG_BUILTIN_DTB) += boot/dts/<br clear="none">> obj-y += errata/<br clear="none">> obj-$(CONFIG_KVM) += kvm/<br clear="none">> <br clear="none">> -obj-$(CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY) += purgatory/<br clear="none">> +obj-$(CONFIG_KEXEC_FILE) += purgatory/<br clear="none">> <br clear="none">> # for cleaning<br clear="none">> subdir- += boot<br clear="none">> diff --git a/arch/s390/Kbuild b/arch/s390/Kbuild<br clear="none">> index a5d3503b353c..361aa01dbd49 100644<br clear="none">> --- a/arch/s390/Kbuild<br clear="none">> +++ b/arch/s390/Kbuild<br clear="none">> @@ -7,7 +7,7 @@ obj-$(CONFIG_S390_HYPFS) += hypfs/<br clear="none">> obj-$(CONFIG_APPLDATA_BASE) += appldata/<br clear="none">> obj-y += net/<br clear="none">> obj-$(CONFIG_PCI) += pci/<br clear="none">> -obj-$(CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY) += purgatory/<br clear="none">> +obj-$(CONFIG_KEXEC_FILE) += purgatory/<br clear="none">> <br clear="none">> # for cleaning<br clear="none">> subdir- += boot tools<br clear="none">> <br clear="none"><br clear="none"></div></div>
</div>
</div></body></html>