From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 arch/x86/kernel/relocate_kernel_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 17d41e6e1a4b..2b7fc59af373 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -158,7 +158,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	lidt	(%rsp)
 	addq	$10, %rsp
 
-	//int3
+	int3
 
 	/*
 	 * Clear X86_CR4_CET (if it was set) such that we can clear CR0_WP
-- 
2.48.1

* David Woodhouse <dwmw2@infradead.org> wrote:

> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  arch/x86/kernel/relocate_kernel_64.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 17d41e6e1a4b..2b7fc59af373 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -158,7 +158,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>  	lidt	(%rsp)
>  	addq	$10, %rsp
>  
> -	//int3
> +	int3

So this is all boot-serialized functionality with no SMP concerns 
whatsoever, right?

If yes then we could use something like this:

	static int exception_selftest = 1;

and add the INT3 point:

	int3
.globl after_int3
after_int3:

And do this in the early exception handler:

	...
	
	if (exception_selftest) {
		exception_selftest = 0;

		print_something_warm_and_fuzzy();

		IRET-to-after_int3;
	}

	...

	... regular exception path ...

... but all in assembly or so ;-)

This would make it reasonably certain that the most complex bits of 
this new debuging code are in working order, all the time.

Thanks,

	Ingo

On 13 March 2025 11:44:41 CET, Ingo Molnar <mingo@kernel.org> wrote:
>
>* David Woodhouse <dwmw2@infradead.org> wrote:
>
>> From: David Woodhouse <dwmw@amazon.co.uk>
>> 
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> ---
>>  arch/x86/kernel/relocate_kernel_64.S | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
>> index 17d41e6e1a4b..2b7fc59af373 100644
>> --- a/arch/x86/kernel/relocate_kernel_64.S
>> +++ b/arch/x86/kernel/relocate_kernel_64.S
>> @@ -158,7 +158,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>>  	lidt	(%rsp)
>>  	addq	$10, %rsp
>>  
>> -	//int3
>> +	int3
>
>So this is all boot-serialized functionality with no SMP concerns 
>whatsoever, right?
>
>If yes then we could use something like this:
>
>	static int exception_selftest = 1;
>
>and add the INT3 point:
>
>	int3
>.globl after_int3
>after_int3:
>
>And do this in the early exception handler:
>
>	...
>	
>	if (exception_selftest) {
>		exception_selftest = 0;
>
>		print_something_warm_and_fuzzy();
>
>		IRET-to-after_int3;
>	}
>
>	...
>
>	... regular exception path ...
>
>... but all in assembly or so ;-)
>
>This would make it reasonably certain that the most complex bits of 
>this new debuging code are in working order, all the time.
>
>Thanks,
>
>	Ingo

The exception handler already returns if the exception was int3, but not for anything else. Less so the "print something warm and fuzzy" part; it just does the same register dump. But we could change that.

I'm less keen on making it unconditional though. Kexec is a performance-critical path when every millisecond is perceived as guest steal time, and the serial output should only happen in production if something goes *wrong*.

And besides, most kexec users don't have early_printk enabled anyway so if we break them, this idea doesn't help.

* David Woodhouse <dwmw2@infradead.org> wrote:

> The exception handler already returns if the exception was int3, but 
> not for anything else. Less so the "print something warm and fuzzy" 
> part; it just does the same register dump. But we could change that.
> 
> I'm less keen on making it unconditional though. Kexec is a 
> performance-critical path when every millisecond is perceived as 
> guest steal time, and the serial output should only happen in 
> production if something goes *wrong*.
> 
> And besides, most kexec users don't have early_printk enabled anyway 
> so if we break them, this idea doesn't help.

So this check would only cause any real overhead if serial debugging is 
enabled - in which case there's already substantial overhead due to the 
serial console overhead (virtual or otherwise).

As to not printing anything unless the early serial console is enabled 
- that's fine, we'd still *break* if something doesn't work in this 
code path, so at least the exception handling machinery is kept 
well-tested. :-)

Thanks,

	Ingo

On Thu, 2025-03-13 at 18:06 +0100, Ingo Molnar wrote:
> 
> * David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > The exception handler already returns if the exception was int3, but 
> > not for anything else. Less so the "print something warm and fuzzy"
> > part; it just does the same register dump. But we could change that.
> > 
> > I'm less keen on making it unconditional though. Kexec is a 
> > performance-critical path when every millisecond is perceived as 
> > guest steal time, and the serial output should only happen in 
> > production if something goes *wrong*.
> > 
> > And besides, most kexec users don't have early_printk enabled anyway 
> > so if we break them, this idea doesn't help.
> 
> So this check would only cause any real overhead if serial debugging is 
> enabled - in which case there's already substantial overhead due to the 
> serial console overhead (virtual or otherwise).
>
> As to not printing anything unless the early serial console is enabled 
> - that's fine, we'd still *break* if something doesn't work in this 
> code path, so at least the exception handling machinery is kept 
> well-tested. :-)

I suppose so, although it's also fairly easy to test by adding an int3
into a kexec-jump test case like http://david.woodhou.se/loadret.c

But yeah, it's easy enough to make the int3 register dump a little less
scary and then remove the 'DO NOT MERGE' tag from the commit which adds
the unconditional int3.

I'm still trying to chase down the new objtool warning you noted (which
I swear I saw once but now can't reproduce), and I note I need to make
the early_printk hooks depend on (KEXEC_CORE && X86_64) since I removed
the separate KEXEC_DEBUG option and made it all unconditional.