[PATCH] x86/purgatory: provide config to disable purgatory

[PATCH] x86/purgatory: provide config to disable purgatory

[Usama Arif]

This can help in reducing boot time if purgatory is not needed
as the sha256 digest of kexec segments is no longer calculated
or verified if the config is disabled.

Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Reviewed-by: Fam Zheng <fam.zheng@bytedance.com>
---
 arch/powerpc/Kbuild               |  2 +-
 arch/powerpc/Kconfig              |  2 +-
 arch/s390/Kbuild                  |  2 +-
 arch/s390/Kconfig                 |  2 +-
 arch/s390/purgatory/Makefile      |  2 +-
 arch/x86/Kbuild                   |  2 +-
 arch/x86/Kconfig                  |  6 ++--
 arch/x86/kernel/kexec-bzimage64.c | 59 +++++++++++++++++++++------------------
 arch/x86/purgatory/Makefile       |  2 +-
 kernel/kexec_file.c               |  6 ++--
 10 files changed, 46 insertions(+), 39 deletions(-)

diff --git a/arch/powerpc/Kbuild b/arch/powerpc/Kbuild
index 22cd0d55a892..072e62d7898e 100644
--- a/arch/powerpc/Kbuild
+++ b/arch/powerpc/Kbuild
@@ -15,7 +15,7 @@ obj-$(CONFIG_KVM)  += kvm/
 
 obj-$(CONFIG_PERF_EVENTS) += perf/
 obj-$(CONFIG_KEXEC_CORE)  += kexec/
-obj-$(CONFIG_KEXEC_FILE)  += purgatory/
+obj-$(CONFIG_KEXEC_PURGATORY)  += purgatory/
 
 # for cleaning
 subdir- += boot
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index dea74d7717c0..58bdfd1abb44 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -557,7 +557,7 @@ config KEXEC_FILE
 	  for kernel and initramfs as opposed to a list of segments as is the
 	  case for the older kexec call.
 
-config ARCH_HAS_KEXEC_PURGATORY
+config KEXEC_PURGATORY
 	def_bool KEXEC_FILE
 
 config RELOCATABLE
diff --git a/arch/s390/Kbuild b/arch/s390/Kbuild
index 76e362277179..2ed4ee5cdf59 100644
--- a/arch/s390/Kbuild
+++ b/arch/s390/Kbuild
@@ -7,7 +7,7 @@ obj-$(CONFIG_S390_HYPFS_FS)	+= hypfs/
 obj-$(CONFIG_APPLDATA_BASE)	+= appldata/
 obj-y				+= net/
 obj-$(CONFIG_PCI)		+= pci/
-obj-$(CONFIG_ARCH_HAS_KEXEC_PURGATORY) += purgatory/
+obj-$(CONFIG_KEXEC_PURGATORY) += purgatory/
 
 # for cleaning
 subdir- += boot tools
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 2a5bb4f29cfe..d15bdaa0e198 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -538,7 +538,7 @@ config KEXEC_FILE
 	  kexec system call this system call takes file descriptors for the
 	  kernel and initramfs as arguments.
 
-config ARCH_HAS_KEXEC_PURGATORY
+config KEXEC_PURGATORY
 	def_bool y
 	depends on KEXEC_FILE
 
diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile
index 360ada80d20c..03cac6d7310a 100644
--- a/arch/s390/purgatory/Makefile
+++ b/arch/s390/purgatory/Makefile
@@ -51,4 +51,4 @@ $(obj)/purgatory.ro: $(obj)/purgatory $(obj)/purgatory.chk FORCE
 $(obj)/kexec-purgatory.o: $(obj)/kexec-purgatory.S $(obj)/purgatory.ro FORCE
 	$(call if_changed_rule,as_o_S)
 
-obj-$(CONFIG_ARCH_HAS_KEXEC_PURGATORY) += kexec-purgatory.o
+obj-$(CONFIG_KEXEC_PURGATORY) += kexec-purgatory.o
diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
index f384cb1a4f7a..9089438ed6d8 100644
--- a/arch/x86/Kbuild
+++ b/arch/x86/Kbuild
@@ -24,7 +24,7 @@ obj-$(CONFIG_IA32_EMULATION) += ia32/
 obj-y += platform/
 obj-y += net/
 
-obj-$(CONFIG_KEXEC_FILE) += purgatory/
+obj-$(CONFIG_KEXEC_PURGATORY) += purgatory/
 
 # for cleaning
 subdir- += boot tools
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 7399327d1eff..7efe6dbfdc67 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2000,8 +2000,10 @@ config KEXEC_FILE
 	  for kernel and initramfs as opposed to list of segments as
 	  accepted by previous system call.
 
-config ARCH_HAS_KEXEC_PURGATORY
-	def_bool KEXEC_FILE
+config KEXEC_PURGATORY
+	bool "A standalone relocatable object run between the 2 kernels during kexec"
+	depends on KEXEC_FILE
+	default y
 
 config KEXEC_SIG
 	bool "Verify kernel signature during kexec_file_load() syscall"
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 170d0fd68b1f..bf37a2c4ab8b 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -374,18 +374,19 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 			return ERR_PTR(ret);
 	}
 
-	/*
-	 * Load purgatory. For 64bit entry point, purgatory  code can be
-	 * anywhere.
-	 */
-	ret = kexec_load_purgatory(image, &pbuf);
-	if (ret) {
-		pr_err("Loading purgatory failed\n");
-		return ERR_PTR(ret);
-	}
-
-	pr_debug("Loaded purgatory at 0x%lx\n", pbuf.mem);
+	if (IS_ENABLED(CONFIG_KEXEC_PURGATORY)) {
+		/*
+		 * Load purgatory. For 64bit entry point, purgatory  code can be
+		 * anywhere.
+		 */
+		ret = kexec_load_purgatory(image, &pbuf);
+		if (ret) {
+			pr_err("Loading purgatory failed\n");
+			return ERR_PTR(ret);
+		}
 
+		pr_debug("Loaded purgatory at 0x%lx\n", pbuf.mem);
+	}
 
 	/*
 	 * Load Bootparams and cmdline and space for efi stuff.
@@ -466,28 +467,32 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 	params->hdr.type_of_loader = 0x0D << 4;
 	params->hdr.loadflags = 0;
 
-	/* Setup purgatory regs for entry */
-	ret = kexec_purgatory_get_set_symbol(image, "entry64_regs", &regs64,
-					     sizeof(regs64), 1);
-	if (ret)
-		goto out_free_params;
+	if (IS_ENABLED(CONFIG_KEXEC_PURGATORY)) {
+		/* Setup purgatory regs for entry */
+		ret = kexec_purgatory_get_set_symbol(image, "entry64_regs", &regs64,
+							sizeof(regs64), 1);
+		if (ret)
+			goto out_free_params;
+	}
 
 	regs64.rbx = 0; /* Bootstrap Processor */
 	regs64.rsi = bootparam_load_addr;
 	regs64.rip = kernel_load_addr + 0x200;
-	stack = kexec_purgatory_get_symbol_addr(image, "stack_end");
-	if (IS_ERR(stack)) {
-		pr_err("Could not find address of symbol stack_end\n");
-		ret = -EINVAL;
-		goto out_free_params;
-	}
 
-	regs64.rsp = (unsigned long)stack;
-	ret = kexec_purgatory_get_set_symbol(image, "entry64_regs", &regs64,
-					     sizeof(regs64), 0);
-	if (ret)
-		goto out_free_params;
+	if (IS_ENABLED(CONFIG_KEXEC_PURGATORY)) {
+		stack = kexec_purgatory_get_symbol_addr(image, "stack_end");
+		if (IS_ERR(stack)) {
+			pr_err("Could not find address of symbol stack_end\n");
+			ret = -EINVAL;
+			goto out_free_params;
+		}
 
+		regs64.rsp = (unsigned long)stack;
+		ret = kexec_purgatory_get_set_symbol(image, "entry64_regs", &regs64,
+							sizeof(regs64), 0);
+		if (ret)
+			goto out_free_params;
+	}
 	ret = setup_boot_parameters(image, params, bootparam_load_addr,
 				    efi_map_offset, efi_map_sz,
 				    efi_setup_data_offset);
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index 95ea17a9d20c..688b3f21be8f 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -81,4 +81,4 @@ quiet_cmd_bin2c = BIN2C   $@
 $(obj)/kexec-purgatory.c: $(obj)/purgatory.ro $(obj)/purgatory.chk FORCE
 	$(call if_changed,bin2c)
 
-obj-$(CONFIG_KEXEC_FILE)	+= kexec-purgatory.o
+obj-$(CONFIG_KEXEC_PURGATORY)	+= kexec-purgatory.o
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 8347fc158d2b..e5f4c2d27249 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -724,7 +724,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
 	struct kexec_sha_region *sha_regions;
 	struct purgatory_info *pi = &image->purgatory_info;
 
-	if (!IS_ENABLED(CONFIG_ARCH_HAS_KEXEC_PURGATORY))
+	if (!IS_ENABLED(CONFIG_KEXEC_PURGATORY))
 		return 0;
 
 	zero_buf = __va(page_to_pfn(ZERO_PAGE(0)) << PAGE_SHIFT);
@@ -829,7 +829,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
 	return ret;
 }
 
-#ifdef CONFIG_ARCH_HAS_KEXEC_PURGATORY
+#ifdef CONFIG_KEXEC_PURGATORY
 /*
  * kexec_purgatory_setup_kbuf - prepare buffer to load purgatory.
  * @pi:		Purgatory to be loaded.
@@ -1176,7 +1176,7 @@ int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name,
 
 	return 0;
 }
-#endif /* CONFIG_ARCH_HAS_KEXEC_PURGATORY */
+#endif /* CONFIG_KEXEC_PURGATORY */
 
 int crash_exclude_mem_range(struct crash_mem *mem,
 			    unsigned long long mstart, unsigned long long mend)
-- 
2.11.0

Re: [PATCH] x86/purgatory: provide config to disable purgatory

[Borislav Petkov]

On Tue, Nov 23, 2021 at 03:05:08PM +0000, Usama Arif wrote:
> This can help in reducing boot time if purgatory is not needed
> as the sha256 digest of kexec segments is no longer calculated
> or verified if the config is disabled.

I'd prefer a commit message to say:

"Disable purgatory because of real-life use case X. With it disabled,
booting a second kernel is sped up by Y."

"Just because" and adding yet another config option is not worth the
effort, otherwise.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/purgatory: provide config to disable purgatory

[Vivek Goyal]

On Tue, Nov 23, 2021 at 04:24:01PM +0100, Borislav Petkov wrote:
> On Tue, Nov 23, 2021 at 03:05:08PM +0000, Usama Arif wrote:
> > This can help in reducing boot time if purgatory is not needed
> > as the sha256 digest of kexec segments is no longer calculated
> > or verified if the config is disabled.
> 
> I'd prefer a commit message to say:
> 
> "Disable purgatory because of real-life use case X. With it disabled,
> booting a second kernel is sped up by Y."
> 
> "Just because" and adding yet another config option is not worth the
> effort, otherwise.

Agreed. What's the use case.

Also this cheksum is used to make sure purgatory is not corrupted. So
this is sort of saftey mechanism to make sure things are still the
same as we expected before we start executing this piece of code. Hence
this does not sound like an optional feature to me (even if it speeds
up things a bit).

BTW, how much speed up do you see.

Thanks
Vivek

Re: [External] Re: [PATCH] x86/purgatory: provide config to disable purgatory

[Eric W. Biederman]

Usama Arif <usama.arif@bytedance.com> writes:

> Hi,
>
> Thanks for your replies. I have submitted a v2 of the patch with a
> much more detailed commit message including reason for the patch and timing values.
>
> The time taken from reboot to running init process was measured
> with both purgatory enabled and disabled over 20 runs and the
> averages are:
> Purgatory disabled:
> - TSC = 3908766161 cycles
> - ktime = 606.8 ms
> Purgatory enabled:
> - TSC = 5005811885 cycles (28.1% worse)
> - ktime = 843.1 ms (38.9% worse)
>
>
> Our reason for this patch is that it helps reduce the downtime of servers when
> the host kernel managing multiple VMs needs to be updated via kexec,
> but it makes reboot with kexec much faster so should be a general improvement in
> boot time if purgatory is not needed and could have other usecases as well.
> I believe only x86, powerpc and s390 have purgatory supported, other platforms
> like arm64 dont have it implemented yet, so with the reboot time improvement seen,
> it would be a good idea to have the option to disable purgatory completely but set default to y.
> We also have the CONFIG_KEXEC_BZIMAGE_VERIFY_SIG which can be enabled to verify the next
> kernel image to be booted and purgatory can be completely skipped if
> not required.

CONFIG_KEXEC_BZIMAGE_VERIFY_SIG is something totally and completely
different.  It's job is to verify that the kernel to be booted comes
from a trusted source.   The sha256 verification in purgatory's job
is to verify that memory the kernel cares about was not corrupted
during the kexec process.

I believe when you say purgatory you are really talking about that
sha256 checksum.  It really is not possible to disable all of
the code that runs between kernels, as the old and the new kernel may
run at the same addresses.  Anything that runs between the two kernels
is what is referred to as purgatory.  Even if it is just a small
assembly stub.

That sha256 verification is always needed for kexec on panic, there are
by the nature of a kernel panic too many unknowns to have any confidence
the new kernel will not be corrupted in the process of kexec before it
gets started.

For an ordinary kexec it might be possible to say that you have a
reliable kernel shutdown process and you know for a fact that something
won't come along and corrupt the kernel.  I find that a questionable
assertion.  I haven't seen anyone yet whose focus when getting an
ordinary kexec to work as anything other than making certain all of the
drivers are shutdown properly.

I have seen countless times when a network packet comes in a the wrong
time and the target kernel's memory is corrupted before it gets far
enough to initialize the network driver.

For a 0.2s speed up you are talking about disabling all of the safety
checks in a very dangerous situation.  How much can you can in
performance by optimizing the sha256 implementation instead of using
what is essentially a reference implementation in basic C that I copied
from somewhere long ago.

Optimize the sha256 implementation and the memory copy loop and then
show how the tiny bit of time that is left is on a mission critical path
and must be removed.  Then we can reasonably talk about a config option
for disabling the sha256 implementation in the kexec in not-panic case.

That sha256 implementation in part so that we can all sleep at night
because we don't have to deal with very very strange heizenbugs.

Eric

Re: [External] Re: [PATCH] x86/purgatory: provide config to disable purgatory

[Usama Arif]

Hi,

(Resending the reply as my email client had updated and inserted html 
code and caused a bounceback from the mailing list, sorry about that.)

Thanks for your reply, I have responded with further comments/questions 
inline below, and also have provided some context for the patch at the 
start:

The patch is not introducing a new CONFIG option, as can be seen in the 
v2 patch diff. It is converting an existing CONFIG option that only 
enabled purgatory for specific architectures in which it has been 
implemented (x86, powerpc and s390) to an option that can that allow 
purgatory to be disabled (with default enabled) and only provides code 
to disable purgatory for x86 only. From what i see, purgatory is 
currently not yet implemented in other architectures like arm64 
kexec_file case and riscv, so this would on a high level,
provide only the option to make kexec on x86 similar to other the other 
architectures.

As a background to the discussion, the usecase we are aiming is to 
update the host kernel with kexec in servers which is managing multiple 
VMs, to cut down the downtime of servers as much as possible so that its 
not noticeable to VMs. The patch is aimed at x86 purgatory code 
specifically. We are targetting other optimizations as well in other 
areas in boot path to cut down the 600ms time much further, so that cut 
down of 200ms downtime is significant in the usecase described. I did 
have a few comments/questions about your reply, please see my responses 
below:


On 29/11/2021 16:53, Eric W. Biederman wrote:
> Usama Arif <usama.arif@bytedance.com> writes:
> 
>> Hi,
>>
>> Thanks for your replies. I have submitted a v2 of the patch with a
>> much more detailed commit message including reason for the patch and timing values.
>>
>> The time taken from reboot to running init process was measured
>> with both purgatory enabled and disabled over 20 runs and the
>> averages are:
>> Purgatory disabled:
>> - TSC = 3908766161 cycles
>> - ktime = 606.8 ms
>> Purgatory enabled:
>> - TSC = 5005811885 cycles (28.1% worse)
>> - ktime = 843.1 ms (38.9% worse)
>>
>>
>> Our reason for this patch is that it helps reduce the downtime of servers when
>> the host kernel managing multiple VMs needs to be updated via kexec,
>> but it makes reboot with kexec much faster so should be a general improvement in
>> boot time if purgatory is not needed and could have other usecases as well.
>> I believe only x86, powerpc and s390 have purgatory supported, other platforms
>> like arm64 dont have it implemented yet, so with the reboot time improvement seen,
>> it would be a good idea to have the option to disable purgatory completely but set default to y.
>> We also have the CONFIG_KEXEC_BZIMAGE_VERIFY_SIG which can be enabled to verify the next
>> kernel image to be booted and purgatory can be completely skipped if
>> not required.
> 
> CONFIG_KEXEC_BZIMAGE_VERIFY_SIG is something totally and completely
> different.  It's job is to verify that the kernel to be booted comes
> from a trusted source.   The sha256 verification in purgatory's job
> is to verify that memory the kernel cares about was not corrupted
> during the kexec process.
> 

Thanks, acknowledged.

> I believe when you say purgatory you are really talking about that
> sha256 checksum.  It really is not possible to disable all of
> the code that runs between kernels, as the old and the new kernel may
> run at the same addresses.  Anything that runs between the two kernels
> is what is referred to as purgatory.  Even if it is just a small
> assembly stub.
> 
 >

With this patch, i am trying to give an option (with default purgatory 
enabled)to disable purgatory completely on x86 only, i.e. no code 
running between 2 kernels on x86. From my understanding there is no 
purgatory in arm64 kexec_file case, in riscv and in some other archs as 
well, so I am not sure why its not possible to disable purgatory (all 
code running between 2 kernels) in x86? Unless i misunderstood something 
about the working of other platforms? In x86 case, from what i see 
relocate_kernel is still part of the older kernel and not purgatory, and 
with my patch, purgatory.ro is not built and kexec does execute 
successfully with purgatory disabled.

About your point for the old and the new kernel may run at the same 
addresses,i dont think that can happen as in bzImage64_load function its 
loaded using the kbuf struct. This doesnt happen in other architectures 
(for e.g. arm64) that dont implement purgatory and any of the tests I 
conducted with the patch applied due to the use of kbuf struct.

 From what i see in the code, purgatory in x86 specifically, has 2 main 
functions, sha256 verification and loading the %rsi register for 
bootparam_load_addr. In this patch purgatory was disabled, which 
resulted in sha256 verification disabled and bootparam_load_addr moved 
to relocate_kernel. Our analysis revealed that most of the time is spent 
in the sha256 verification, so I would be ok to reformat the patch
to make the sha verification optional and keep purgatory enabled if 
thats preferred? Although in my opinion the current patch of only 
providing an option to disable purgatory seems much better.


> That sha256 verification is always needed for kexec on panic, there are
> by the nature of a kernel panic too many unknowns to have any confidence
> the new kernel will not be corrupted in the process of kexec before it
> gets started.
> 
> For an ordinary kexec it might be possible to say that you have a
> reliable kernel shutdown process and you know for a fact that something
> won't come along and corrupt the kernel.  I find that a questionable
> assertion.  I haven't seen anyone yet whose focus when getting an
> ordinary kexec to work as anything other than making certain all of the
> drivers are shutdown properly.
> 
> I have seen countless times when a network packet comes in a the wrong
> time and the target kernel's memory is corrupted before it gets far
> enough to initialize the network driver. >

I agree that when doing testing and research, there are things that can
go wrong during kexec process, but i assume that the expectation is that
in production environment, when for e.g. updating a kernel with kexec in 
servers,the expectation is that kexec will execute successfully as long 
as there is nothing wrong in the old kernel and the new kernel.

Maybe I didn't understand this correctly, but if a network packet comes 
in and corrupts the kernel memory, wont it also cause a problem even 
when purgatory is enabled? I agree that issue like these would be caught 
earlier with purgatory, but then if something like this breaks the kexec 
process in a production environment where the old and new kernel are 
well tested, trusted and expected to work, wouldn't there be a much more 
fundamental issue with the reliability of the current kexec process.

> For a 0.2s speed up you are talking about disabling all of the safety
> checks in a very dangerous situation.  How much can you can in
> performance by optimizing the sha256 implementation instead of using
> what is essentially a reference implementation in basic C that I copied
> from somewhere long ago.
> 
> Optimize the sha256 implementation and the memory copy loop and then
> show how the tiny bit of time that is left is on a mission critical path
> and must be removed.  Then we can reasonably talk about a config option
> for disabling the sha256 implementation in the kexec in not-panic case.
> 
Thanks for this, I assume that over here you are suggesting for e.g. in 
x86 to replace the existing sha verification implementation in purgatory 
with the one in arch/x86/crypto/sha256_ssse3_glue.c? Also could you 
point to the memory copy loop to optimize? I can have a look at these 2 
options.

Even with these optimizations, i think the v2 patch should still be 
considered. The patch in the end is providing only an option to disable 
purgatory on x86 only, with the default value of keeping it enabled and 
not changing kexec behaviour. The CONFIG already existed, it is just 
renamed and now provides user the option to select to disable purgatory 
for x86, rather than it being architecture dependent.

Thanks again for the review and comments,
Usama


> That sha256 implementation in part so that we can all sleep at night
> because we don't have to deal with very very strange heizenbugs.
> 
> Eric
> 

Re: [External] Re: [PATCH] x86/purgatory: provide config to disable purgatory

[Usama Arif]

Hi,

I have sent a v3 of the patch with a much clearer commit message, please
let me know if there are any comments for the v3 patch or my responses 
below.

Thanks,
Usama

On 01/12/2021 10:29, Usama Arif wrote:
> Hi,
> 
> (Resending the reply as my email client had updated and inserted html 
> code and caused a bounceback from the mailing list, sorry about that.)
> 
> Thanks for your reply, I have responded with further comments/questions 
> inline below, and also have provided some context for the patch at the 
> start:
> 
> The patch is not introducing a new CONFIG option, as can be seen in the 
> v2 patch diff. It is converting an existing CONFIG option that only 
> enabled purgatory for specific architectures in which it has been 
> implemented (x86, powerpc and s390) to an option that can that allow 
> purgatory to be disabled (with default enabled) and only provides code 
> to disable purgatory for x86 only. From what i see, purgatory is 
> currently not yet implemented in other architectures like arm64 
> kexec_file case and riscv, so this would on a high level,
> provide only the option to make kexec on x86 similar to other the other 
> architectures.
> 
> As a background to the discussion, the usecase we are aiming is to 
> update the host kernel with kexec in servers which is managing multiple 
> VMs, to cut down the downtime of servers as much as possible so that its 
> not noticeable to VMs. The patch is aimed at x86 purgatory code 
> specifically. We are targetting other optimizations as well in other 
> areas in boot path to cut down the 600ms time much further, so that cut 
> down of 200ms downtime is significant in the usecase described. I did 
> have a few comments/questions about your reply, please see my responses 
> below:
> 
> 
> On 29/11/2021 16:53, Eric W. Biederman wrote:
>> Usama Arif <usama.arif@bytedance.com> writes:
>>
>>> Hi,
>>>
>>> Thanks for your replies. I have submitted a v2 of the patch with a
>>> much more detailed commit message including reason for the patch and 
>>> timing values.
>>>
>>> The time taken from reboot to running init process was measured
>>> with both purgatory enabled and disabled over 20 runs and the
>>> averages are:
>>> Purgatory disabled:
>>> - TSC = 3908766161 cycles
>>> - ktime = 606.8 ms
>>> Purgatory enabled:
>>> - TSC = 5005811885 cycles (28.1% worse)
>>> - ktime = 843.1 ms (38.9% worse)
>>>
>>>
>>> Our reason for this patch is that it helps reduce the downtime of 
>>> servers when
>>> the host kernel managing multiple VMs needs to be updated via kexec,
>>> but it makes reboot with kexec much faster so should be a general 
>>> improvement in
>>> boot time if purgatory is not needed and could have other usecases as 
>>> well.
>>> I believe only x86, powerpc and s390 have purgatory supported, other 
>>> platforms
>>> like arm64 dont have it implemented yet, so with the reboot time 
>>> improvement seen,
>>> it would be a good idea to have the option to disable purgatory 
>>> completely but set default to y.
>>> We also have the CONFIG_KEXEC_BZIMAGE_VERIFY_SIG which can be enabled 
>>> to verify the next
>>> kernel image to be booted and purgatory can be completely skipped if
>>> not required.
>>
>> CONFIG_KEXEC_BZIMAGE_VERIFY_SIG is something totally and completely
>> different.  It's job is to verify that the kernel to be booted comes
>> from a trusted source.   The sha256 verification in purgatory's job
>> is to verify that memory the kernel cares about was not corrupted
>> during the kexec process.
>>
> 
> Thanks, acknowledged.
> 
>> I believe when you say purgatory you are really talking about that
>> sha256 checksum.  It really is not possible to disable all of
>> the code that runs between kernels, as the old and the new kernel may
>> run at the same addresses.  Anything that runs between the two kernels
>> is what is referred to as purgatory.  Even if it is just a small
>> assembly stub.
>>
>  >
> 
> With this patch, i am trying to give an option (with default purgatory 
> enabled)to disable purgatory completely on x86 only, i.e. no code 
> running between 2 kernels on x86. From my understanding there is no 
> purgatory in arm64 kexec_file case, in riscv and in some other archs as 
> well, so I am not sure why its not possible to disable purgatory (all 
> code running between 2 kernels) in x86? Unless i misunderstood something 
> about the working of other platforms? In x86 case, from what i see 
> relocate_kernel is still part of the older kernel and not purgatory, and 
> with my patch, purgatory.ro is not built and kexec does execute 
> successfully with purgatory disabled.
> 
> About your point for the old and the new kernel may run at the same 
> addresses,i dont think that can happen as in bzImage64_load function its 
> loaded using the kbuf struct. This doesnt happen in other architectures 
> (for e.g. arm64) that dont implement purgatory and any of the tests I 
> conducted with the patch applied due to the use of kbuf struct.
> 
>  From what i see in the code, purgatory in x86 specifically, has 2 main 
> functions, sha256 verification and loading the %rsi register for 
> bootparam_load_addr. In this patch purgatory was disabled, which 
> resulted in sha256 verification disabled and bootparam_load_addr moved 
> to relocate_kernel. Our analysis revealed that most of the time is spent 
> in the sha256 verification, so I would be ok to reformat the patch
> to make the sha verification optional and keep purgatory enabled if 
> thats preferred? Although in my opinion the current patch of only 
> providing an option to disable purgatory seems much better.
> 
> 
>> That sha256 verification is always needed for kexec on panic, there are
>> by the nature of a kernel panic too many unknowns to have any confidence
>> the new kernel will not be corrupted in the process of kexec before it
>> gets started.
>>
>> For an ordinary kexec it might be possible to say that you have a
>> reliable kernel shutdown process and you know for a fact that something
>> won't come along and corrupt the kernel.  I find that a questionable
>> assertion.  I haven't seen anyone yet whose focus when getting an
>> ordinary kexec to work as anything other than making certain all of the
>> drivers are shutdown properly.
>>
>> I have seen countless times when a network packet comes in a the wrong
>> time and the target kernel's memory is corrupted before it gets far
>> enough to initialize the network driver. >
> 
> I agree that when doing testing and research, there are things that can
> go wrong during kexec process, but i assume that the expectation is that
> in production environment, when for e.g. updating a kernel with kexec in 
> servers,the expectation is that kexec will execute successfully as long 
> as there is nothing wrong in the old kernel and the new kernel.
> 
> Maybe I didn't understand this correctly, but if a network packet comes 
> in and corrupts the kernel memory, wont it also cause a problem even 
> when purgatory is enabled? I agree that issue like these would be caught 
> earlier with purgatory, but then if something like this breaks the kexec 
> process in a production environment where the old and new kernel are 
> well tested, trusted and expected to work, wouldn't there be a much more 
> fundamental issue with the reliability of the current kexec process.
> 
>> For a 0.2s speed up you are talking about disabling all of the safety
>> checks in a very dangerous situation.  How much can you can in
>> performance by optimizing the sha256 implementation instead of using
>> what is essentially a reference implementation in basic C that I copied
>> from somewhere long ago.
>>
>> Optimize the sha256 implementation and the memory copy loop and then
>> show how the tiny bit of time that is left is on a mission critical path
>> and must be removed.  Then we can reasonably talk about a config option
>> for disabling the sha256 implementation in the kexec in not-panic case.
>>
> Thanks for this, I assume that over here you are suggesting for e.g. in 
> x86 to replace the existing sha verification implementation in purgatory 
> with the one in arch/x86/crypto/sha256_ssse3_glue.c? Also could you 
> point to the memory copy loop to optimize? I can have a look at these 2 
> options.
> 
> Even with these optimizations, i think the v2 patch should still be 
> considered. The patch in the end is providing only an option to disable 
> purgatory on x86 only, with the default value of keeping it enabled and 
> not changing kexec behaviour. The CONFIG already existed, it is just 
> renamed and now provides user the option to select to disable purgatory 
> for x86, rather than it being architecture dependent.
> 
> Thanks again for the review and comments,
> Usama
> 
> 
>> That sha256 implementation in part so that we can all sleep at night
>> because we don't have to deal with very very strange heizenbugs.
>>
>> Eric
>>

Re: [PATCH] x86/purgatory: provide config to disable purgatory

[Usama Arif]

Hi,

Just wanted to check again if there were any comments on my responses 
below or the v3 of the patch at 
https://lore.kernel.org/lkml/20211206164724.2125489-1-usama.arif@bytedance.com/.

Thanks!
Usama

On 06/12/2021 16:51, Usama Arif wrote:
> Hi,
> 
> I have sent a v3 of the patch with a much clearer commit message, please
> let me know if there are any comments for the v3 patch or my responses 
> below.
> 
> Thanks,
> Usama
> 
> On 01/12/2021 10:29, Usama Arif wrote:
>> Hi,
>>
>> (Resending the reply as my email client had updated and inserted html 
>> code and caused a bounceback from the mailing list, sorry about that.)
>>
>> Thanks for your reply, I have responded with further 
>> comments/questions inline below, and also have provided some context 
>> for the patch at the start:
>>
>> The patch is not introducing a new CONFIG option, as can be seen in 
>> the v2 patch diff. It is converting an existing CONFIG option that 
>> only enabled purgatory for specific architectures in which it has been 
>> implemented (x86, powerpc and s390) to an option that can that allow 
>> purgatory to be disabled (with default enabled) and only provides code 
>> to disable purgatory for x86 only. From what i see, purgatory is 
>> currently not yet implemented in other architectures like arm64 
>> kexec_file case and riscv, so this would on a high level,
>> provide only the option to make kexec on x86 similar to other the 
>> other architectures.
>>
>> As a background to the discussion, the usecase we are aiming is to 
>> update the host kernel with kexec in servers which is managing 
>> multiple VMs, to cut down the downtime of servers as much as possible 
>> so that its not noticeable to VMs. The patch is aimed at x86 purgatory 
>> code specifically. We are targetting other optimizations as well in 
>> other areas in boot path to cut down the 600ms time much further, so 
>> that cut down of 200ms downtime is significant in the usecase 
>> described. I did have a few comments/questions about your reply, 
>> please see my responses below:
>>
>>
>> On 29/11/2021 16:53, Eric W. Biederman wrote:
>>> Usama Arif <usama.arif@bytedance.com> writes:
>>>
>>>> Hi,
>>>>
>>>> Thanks for your replies. I have submitted a v2 of the patch with a
>>>> much more detailed commit message including reason for the patch and 
>>>> timing values.
>>>>
>>>> The time taken from reboot to running init process was measured
>>>> with both purgatory enabled and disabled over 20 runs and the
>>>> averages are:
>>>> Purgatory disabled:
>>>> - TSC = 3908766161 cycles
>>>> - ktime = 606.8 ms
>>>> Purgatory enabled:
>>>> - TSC = 5005811885 cycles (28.1% worse)
>>>> - ktime = 843.1 ms (38.9% worse)
>>>>
>>>>
>>>> Our reason for this patch is that it helps reduce the downtime of 
>>>> servers when
>>>> the host kernel managing multiple VMs needs to be updated via kexec,
>>>> but it makes reboot with kexec much faster so should be a general 
>>>> improvement in
>>>> boot time if purgatory is not needed and could have other usecases 
>>>> as well.
>>>> I believe only x86, powerpc and s390 have purgatory supported, other 
>>>> platforms
>>>> like arm64 dont have it implemented yet, so with the reboot time 
>>>> improvement seen,
>>>> it would be a good idea to have the option to disable purgatory 
>>>> completely but set default to y.
>>>> We also have the CONFIG_KEXEC_BZIMAGE_VERIFY_SIG which can be 
>>>> enabled to verify the next
>>>> kernel image to be booted and purgatory can be completely skipped if
>>>> not required.
>>>
>>> CONFIG_KEXEC_BZIMAGE_VERIFY_SIG is something totally and completely
>>> different.  It's job is to verify that the kernel to be booted comes
>>> from a trusted source.   The sha256 verification in purgatory's job
>>> is to verify that memory the kernel cares about was not corrupted
>>> during the kexec process.
>>>
>>
>> Thanks, acknowledged.
>>
>>> I believe when you say purgatory you are really talking about that
>>> sha256 checksum.  It really is not possible to disable all of
>>> the code that runs between kernels, as the old and the new kernel may
>>> run at the same addresses.  Anything that runs between the two kernels
>>> is what is referred to as purgatory.  Even if it is just a small
>>> assembly stub.
>>>
>>  >
>>
>> With this patch, i am trying to give an option (with default purgatory 
>> enabled)to disable purgatory completely on x86 only, i.e. no code 
>> running between 2 kernels on x86. From my understanding there is no 
>> purgatory in arm64 kexec_file case, in riscv and in some other archs 
>> as well, so I am not sure why its not possible to disable purgatory 
>> (all code running between 2 kernels) in x86? Unless i misunderstood 
>> something about the working of other platforms? In x86 case, from what 
>> i see relocate_kernel is still part of the older kernel and not 
>> purgatory, and with my patch, purgatory.ro is not built and kexec does 
>> execute successfully with purgatory disabled.
>>
>> About your point for the old and the new kernel may run at the same 
>> addresses,i dont think that can happen as in bzImage64_load function 
>> its loaded using the kbuf struct. This doesnt happen in other 
>> architectures (for e.g. arm64) that dont implement purgatory and any 
>> of the tests I conducted with the patch applied due to the use of kbuf 
>> struct.
>>
>>  From what i see in the code, purgatory in x86 specifically, has 2 
>> main functions, sha256 verification and loading the %rsi register for 
>> bootparam_load_addr. In this patch purgatory was disabled, which 
>> resulted in sha256 verification disabled and bootparam_load_addr moved 
>> to relocate_kernel. Our analysis revealed that most of the time is 
>> spent in the sha256 verification, so I would be ok to reformat the patch
>> to make the sha verification optional and keep purgatory enabled if 
>> thats preferred? Although in my opinion the current patch of only 
>> providing an option to disable purgatory seems much better.
>>
>>
>>> That sha256 verification is always needed for kexec on panic, there are
>>> by the nature of a kernel panic too many unknowns to have any confidence
>>> the new kernel will not be corrupted in the process of kexec before it
>>> gets started.
>>>
>>> For an ordinary kexec it might be possible to say that you have a
>>> reliable kernel shutdown process and you know for a fact that something
>>> won't come along and corrupt the kernel.  I find that a questionable
>>> assertion.  I haven't seen anyone yet whose focus when getting an
>>> ordinary kexec to work as anything other than making certain all of the
>>> drivers are shutdown properly.
>>>
>>> I have seen countless times when a network packet comes in a the wrong
>>> time and the target kernel's memory is corrupted before it gets far
>>> enough to initialize the network driver. >
>>
>> I agree that when doing testing and research, there are things that can
>> go wrong during kexec process, but i assume that the expectation is that
>> in production environment, when for e.g. updating a kernel with kexec 
>> in servers,the expectation is that kexec will execute successfully as 
>> long as there is nothing wrong in the old kernel and the new kernel.
>>
>> Maybe I didn't understand this correctly, but if a network packet 
>> comes in and corrupts the kernel memory, wont it also cause a problem 
>> even when purgatory is enabled? I agree that issue like these would be 
>> caught earlier with purgatory, but then if something like this breaks 
>> the kexec process in a production environment where the old and new 
>> kernel are well tested, trusted and expected to work, wouldn't there 
>> be a much more fundamental issue with the reliability of the current 
>> kexec process.
>>
>>> For a 0.2s speed up you are talking about disabling all of the safety
>>> checks in a very dangerous situation.  How much can you can in
>>> performance by optimizing the sha256 implementation instead of using
>>> what is essentially a reference implementation in basic C that I copied
>>> from somewhere long ago.
>>>
>>> Optimize the sha256 implementation and the memory copy loop and then
>>> show how the tiny bit of time that is left is on a mission critical path
>>> and must be removed.  Then we can reasonably talk about a config option
>>> for disabling the sha256 implementation in the kexec in not-panic case.
>>>
>> Thanks for this, I assume that over here you are suggesting for e.g. 
>> in x86 to replace the existing sha verification implementation in 
>> purgatory with the one in arch/x86/crypto/sha256_ssse3_glue.c? Also 
>> could you point to the memory copy loop to optimize? I can have a look 
>> at these 2 options.
>>
>> Even with these optimizations, i think the v2 patch should still be 
>> considered. The patch in the end is providing only an option to 
>> disable purgatory on x86 only, with the default value of keeping it 
>> enabled and not changing kexec behaviour. The CONFIG already existed, 
>> it is just renamed and now provides user the option to select to 
>> disable purgatory for x86, rather than it being architecture dependent.
>>
>> Thanks again for the review and comments,
>> Usama
>>
>>
>>> That sha256 implementation in part so that we can all sleep at night
>>> because we don't have to deal with very very strange heizenbugs.
>>>
>>> Eric
>>>

Re: [PATCH] x86/purgatory: provide config to disable purgatory

[Baoquan He]

On 12/16/21 at 06:05pm, Usama Arif wrote:
> Hi,
> 
> Just wanted to check again if there were any comments on my responses below
> or the v3 of the patch at https://lore.kernel.org/lkml/20211206164724.2125489-1-usama.arif@bytedance.com/.

I will have a look. Could you please CC kexec mailing list too next time
when kexec/kdump related patch posted so that more people can help review
if concerned or interested?

I usually don't switch to lkml mail folder unless have to, since it
contains too many mails and take longer time.

> 
> Thanks!
> Usama
> 
> On 06/12/2021 16:51, Usama Arif wrote:
> > Hi,
> > 
> > I have sent a v3 of the patch with a much clearer commit message, please
> > let me know if there are any comments for the v3 patch or my responses
> > below.
> > 
> > Thanks,
> > Usama
> > 
> > On 01/12/2021 10:29, Usama Arif wrote:
> > > Hi,
> > > 
> > > (Resending the reply as my email client had updated and inserted
> > > html code and caused a bounceback from the mailing list, sorry about
> > > that.)
> > > 
> > > Thanks for your reply, I have responded with further
> > > comments/questions inline below, and also have provided some context
> > > for the patch at the start:
> > > 
> > > The patch is not introducing a new CONFIG option, as can be seen in
> > > the v2 patch diff. It is converting an existing CONFIG option that
> > > only enabled purgatory for specific architectures in which it has
> > > been implemented (x86, powerpc and s390) to an option that can that
> > > allow purgatory to be disabled (with default enabled) and only
> > > provides code to disable purgatory for x86 only. From what i see,
> > > purgatory is currently not yet implemented in other architectures
> > > like arm64 kexec_file case and riscv, so this would on a high level,
> > > provide only the option to make kexec on x86 similar to other the
> > > other architectures.
> > > 
> > > As a background to the discussion, the usecase we are aiming is to
> > > update the host kernel with kexec in servers which is managing
> > > multiple VMs, to cut down the downtime of servers as much as
> > > possible so that its not noticeable to VMs. The patch is aimed at
> > > x86 purgatory code specifically. We are targetting other
> > > optimizations as well in other areas in boot path to cut down the
> > > 600ms time much further, so that cut down of 200ms downtime is
> > > significant in the usecase described. I did have a few
> > > comments/questions about your reply, please see my responses below:
> > > 
> > > 
> > > On 29/11/2021 16:53, Eric W. Biederman wrote:
> > > > Usama Arif <usama.arif@bytedance.com> writes:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > Thanks for your replies. I have submitted a v2 of the patch with a
> > > > > much more detailed commit message including reason for the
> > > > > patch and timing values.
> > > > > 
> > > > > The time taken from reboot to running init process was measured
> > > > > with both purgatory enabled and disabled over 20 runs and the
> > > > > averages are:
> > > > > Purgatory disabled:
> > > > > - TSC = 3908766161 cycles
> > > > > - ktime = 606.8 ms
> > > > > Purgatory enabled:
> > > > > - TSC = 5005811885 cycles (28.1% worse)
> > > > > - ktime = 843.1 ms (38.9% worse)
> > > > > 
> > > > > 
> > > > > Our reason for this patch is that it helps reduce the
> > > > > downtime of servers when
> > > > > the host kernel managing multiple VMs needs to be updated via kexec,
> > > > > but it makes reboot with kexec much faster so should be a
> > > > > general improvement in
> > > > > boot time if purgatory is not needed and could have other
> > > > > usecases as well.
> > > > > I believe only x86, powerpc and s390 have purgatory
> > > > > supported, other platforms
> > > > > like arm64 dont have it implemented yet, so with the reboot
> > > > > time improvement seen,
> > > > > it would be a good idea to have the option to disable
> > > > > purgatory completely but set default to y.
> > > > > We also have the CONFIG_KEXEC_BZIMAGE_VERIFY_SIG which can
> > > > > be enabled to verify the next
> > > > > kernel image to be booted and purgatory can be completely skipped if
> > > > > not required.
> > > > 
> > > > CONFIG_KEXEC_BZIMAGE_VERIFY_SIG is something totally and completely
> > > > different.  It's job is to verify that the kernel to be booted comes
> > > > from a trusted source.   The sha256 verification in purgatory's job
> > > > is to verify that memory the kernel cares about was not corrupted
> > > > during the kexec process.
> > > > 
> > > 
> > > Thanks, acknowledged.
> > > 
> > > > I believe when you say purgatory you are really talking about that
> > > > sha256 checksum.  It really is not possible to disable all of
> > > > the code that runs between kernels, as the old and the new kernel may
> > > > run at the same addresses.  Anything that runs between the two kernels
> > > > is what is referred to as purgatory.  Even if it is just a small
> > > > assembly stub.
> > > > 
> > >  >
> > > 
> > > With this patch, i am trying to give an option (with default
> > > purgatory enabled)to disable purgatory completely on x86 only, i.e.
> > > no code running between 2 kernels on x86. From my understanding
> > > there is no purgatory in arm64 kexec_file case, in riscv and in some
> > > other archs as well, so I am not sure why its not possible to
> > > disable purgatory (all code running between 2 kernels) in x86?
> > > Unless i misunderstood something about the working of other
> > > platforms? In x86 case, from what i see relocate_kernel is still
> > > part of the older kernel and not purgatory, and with my patch,
> > > purgatory.ro is not built and kexec does execute successfully with
> > > purgatory disabled.
> > > 
> > > About your point for the old and the new kernel may run at the same
> > > addresses,i dont think that can happen as in bzImage64_load function
> > > its loaded using the kbuf struct. This doesnt happen in other
> > > architectures (for e.g. arm64) that dont implement purgatory and any
> > > of the tests I conducted with the patch applied due to the use of
> > > kbuf struct.
> > > 
> > >  From what i see in the code, purgatory in x86 specifically, has 2
> > > main functions, sha256 verification and loading the %rsi register
> > > for bootparam_load_addr. In this patch purgatory was disabled, which
> > > resulted in sha256 verification disabled and bootparam_load_addr
> > > moved to relocate_kernel. Our analysis revealed that most of the
> > > time is spent in the sha256 verification, so I would be ok to
> > > reformat the patch
> > > to make the sha verification optional and keep purgatory enabled if
> > > thats preferred? Although in my opinion the current patch of only
> > > providing an option to disable purgatory seems much better.
> > > 
> > > 
> > > > That sha256 verification is always needed for kexec on panic, there are
> > > > by the nature of a kernel panic too many unknowns to have any confidence
> > > > the new kernel will not be corrupted in the process of kexec before it
> > > > gets started.
> > > > 
> > > > For an ordinary kexec it might be possible to say that you have a
> > > > reliable kernel shutdown process and you know for a fact that something
> > > > won't come along and corrupt the kernel.  I find that a questionable
> > > > assertion.  I haven't seen anyone yet whose focus when getting an
> > > > ordinary kexec to work as anything other than making certain all of the
> > > > drivers are shutdown properly.
> > > > 
> > > > I have seen countless times when a network packet comes in a the wrong
> > > > time and the target kernel's memory is corrupted before it gets far
> > > > enough to initialize the network driver. >
> > > 
> > > I agree that when doing testing and research, there are things that can
> > > go wrong during kexec process, but i assume that the expectation is that
> > > in production environment, when for e.g. updating a kernel with
> > > kexec in servers,the expectation is that kexec will execute
> > > successfully as long as there is nothing wrong in the old kernel and
> > > the new kernel.
> > > 
> > > Maybe I didn't understand this correctly, but if a network packet
> > > comes in and corrupts the kernel memory, wont it also cause a
> > > problem even when purgatory is enabled? I agree that issue like
> > > these would be caught earlier with purgatory, but then if something
> > > like this breaks the kexec process in a production environment where
> > > the old and new kernel are well tested, trusted and expected to
> > > work, wouldn't there be a much more fundamental issue with the
> > > reliability of the current kexec process.
> > > 
> > > > For a 0.2s speed up you are talking about disabling all of the safety
> > > > checks in a very dangerous situation.  How much can you can in
> > > > performance by optimizing the sha256 implementation instead of using
> > > > what is essentially a reference implementation in basic C that I copied
> > > > from somewhere long ago.
> > > > 
> > > > Optimize the sha256 implementation and the memory copy loop and then
> > > > show how the tiny bit of time that is left is on a mission critical path
> > > > and must be removed.  Then we can reasonably talk about a config option
> > > > for disabling the sha256 implementation in the kexec in not-panic case.
> > > > 
> > > Thanks for this, I assume that over here you are suggesting for e.g.
> > > in x86 to replace the existing sha verification implementation in
> > > purgatory with the one in arch/x86/crypto/sha256_ssse3_glue.c? Also
> > > could you point to the memory copy loop to optimize? I can have a
> > > look at these 2 options.
> > > 
> > > Even with these optimizations, i think the v2 patch should still be
> > > considered. The patch in the end is providing only an option to
> > > disable purgatory on x86 only, with the default value of keeping it
> > > enabled and not changing kexec behaviour. The CONFIG already
> > > existed, it is just renamed and now provides user the option to
> > > select to disable purgatory for x86, rather than it being
> > > architecture dependent.
> > > 
> > > Thanks again for the review and comments,
> > > Usama
> > > 
> > > 
> > > > That sha256 implementation in part so that we can all sleep at night
> > > > because we don't have to deal with very very strange heizenbugs.
> > > > 
> > > > Eric
> > > > 
>