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