The following commit has been merged into the x86/boot branch of tip:
Commit-ID: 78a509fba9c9b1fcb77f95b7c6be30da3d24823a
Gitweb: https://git.kernel.org/tip/78a509fba9c9b1fcb77f95b7c6be30da3d24823a
Author: Jun'ichi Nomura <[email protected]>
AuthorDate: Wed, 29 Nov 2023 15:44:49 -05:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Thu, 30 Nov 2023 09:55:40 +01:00
x86/boot: Ignore NMIs during very early boot
When there are two racing NMIs on x86, the first NMI invokes NMI handler and
the 2nd NMI is latched until IRET is executed.
If panic on NMI and panic kexec are enabled, the first NMI triggers
panic and starts booting the next kernel via kexec. Note that the 2nd
NMI is still latched. During the early boot of the next kernel, once
an IRET is executed as a result of a page fault, then the 2nd NMI is
unlatched and invokes the NMI handler.
However, NMI handler is not set up at the early stage of boot, which
results in a boot failure.
Avoid such problems by setting up a NOP handler for early NMIs.
[ mingo: Refined the changelog. ]
Signed-off-by: Jun'ichi Nomura <[email protected]>
Signed-off-by: Derek Barbosa <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
arch/x86/boot/compressed/ident_map_64.c | 5 +++++
arch/x86/boot/compressed/idt_64.c | 1 +
arch/x86/boot/compressed/idt_handlers_64.S | 1 +
arch/x86/boot/compressed/misc.h | 1 +
4 files changed, 8 insertions(+)
diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 473ba59..d040080 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -386,3 +386,8 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
*/
kernel_add_identity_map(address, end);
}
+
+void do_boot_nmi_trap(struct pt_regs *regs, unsigned long error_code)
+{
+ /* Empty handler to ignore NMI during early boot */
+}
diff --git a/arch/x86/boot/compressed/idt_64.c b/arch/x86/boot/compressed/idt_64.c
index 3cdf94b..d100284 100644
--- a/arch/x86/boot/compressed/idt_64.c
+++ b/arch/x86/boot/compressed/idt_64.c
@@ -61,6 +61,7 @@ void load_stage2_idt(void)
boot_idt_desc.address = (unsigned long)boot_idt;
set_idt_entry(X86_TRAP_PF, boot_page_fault);
+ set_idt_entry(X86_TRAP_NMI, boot_nmi_trap);
#ifdef CONFIG_AMD_MEM_ENCRYPT
/*
diff --git a/arch/x86/boot/compressed/idt_handlers_64.S b/arch/x86/boot/compressed/idt_handlers_64.S
index 22890e1..4d03c85 100644
--- a/arch/x86/boot/compressed/idt_handlers_64.S
+++ b/arch/x86/boot/compressed/idt_handlers_64.S
@@ -70,6 +70,7 @@ SYM_FUNC_END(\name)
.code64
EXCEPTION_HANDLER boot_page_fault do_boot_page_fault error_code=1
+EXCEPTION_HANDLER boot_nmi_trap do_boot_nmi_trap error_code=0
#ifdef CONFIG_AMD_MEM_ENCRYPT
EXCEPTION_HANDLER boot_stage1_vc do_vc_no_ghcb error_code=1
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index c0d502b..bc2f0f1 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -196,6 +196,7 @@ static inline void cleanup_exception_handling(void) { }
/* IDT Entry Points */
void boot_page_fault(void);
+void boot_nmi_trap(void);
void boot_stage1_vc(void);
void boot_stage2_vc(void);
On Thu, Nov 30, 2023 at 08:59:44AM -0000, tip-bot2 for Jun'ichi Nomura wrote:
> +void do_boot_nmi_trap(struct pt_regs *regs, unsigned long error_code)
> +{
> + /* Empty handler to ignore NMI during early boot */
It might be good to issue something here to say that a spurious NMI got
ignored.
Something ala
error_putstr("Spurious early NMI ignored.\n");
so that we at least say that we ignored an NMI and not have it disappear
unnoticed.
Hmmm.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
* Borislav Petkov <[email protected]> wrote:
> On Thu, Nov 30, 2023 at 08:59:44AM -0000, tip-bot2 for Jun'ichi Nomura wrote:
> > +void do_boot_nmi_trap(struct pt_regs *regs, unsigned long error_code)
> > +{
> > + /* Empty handler to ignore NMI during early boot */
>
> It might be good to issue something here to say that a spurious NMI got
> ignored.
>
> Something ala
>
> error_putstr("Spurious early NMI ignored.\n");
>
> so that we at least say that we ignored an NMI and not have it disappear
> unnoticed.
That makes sense. Jun'ichi-san, could you please send a patch for this?
Thanks,
Ingo
> From: Ingo Molnar <[email protected]>
> * Borislav Petkov <[email protected]> wrote:
> > On Thu, Nov 30, 2023 at 08:59:44AM -0000, tip-bot2 for Jun'ichi Nomura wrote:
> > > +void do_boot_nmi_trap(struct pt_regs *regs, unsigned long error_code)
> > > +{
> > > + /* Empty handler to ignore NMI during early boot */
> >
> > It might be good to issue something here to say that a spurious NMI got
> > ignored.
> >
> > Something ala
> >
> > error_putstr("Spurious early NMI ignored.\n");
> >
> > so that we at least say that we ignored an NMI and not have it disappear
> > unnoticed.
>
> That makes sense. Jun'ichi-san, could you please send a patch for this?
Thank you for comments. I currently don't have an environment to send a
patch without white space damage. Could it be possible for you to apply
the attached one?
--
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
> > From: Ingo Molnar <[email protected]>
> > * Borislav Petkov <[email protected]> wrote:
> > > On Thu, Nov 30, 2023 at 08:59:44AM -0000, tip-bot2 for Jun'ichi Nomura wrote:
> > > > +void do_boot_nmi_trap(struct pt_regs *regs, unsigned long error_code)
> > > > +{
> > > > + /* Empty handler to ignore NMI during early boot */
> > >
> > > It might be good to issue something here to say that a spurious NMI got
> > > ignored.
> > >
> > > Something ala
> > >
> > > error_putstr("Spurious early NMI ignored.\n");
> > >
> > > so that we at least say that we ignored an NMI and not have it disappear
> > > unnoticed.
> >
> > That makes sense. Jun'ichi-san, could you please send a patch for this?
>
> Thank you for comments. I currently don't have an environment to send a
> patch without white space damage. Could it be possible for you to apply
> the attached one?
Sorry for the noise. Finally I could send it out here without broken tabs.
https://lore.kernel.org/lkml/[email protected]/
--
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
The following commit has been merged into the x86/boot branch of tip:
Commit-ID: ac456ca0af4fe9630cf84e7efd20b7f7bf596aab
Gitweb: https://git.kernel.org/tip/ac456ca0af4fe9630cf84e7efd20b7f7bf596aab
Author: NOMURA JUNICHI(野村 淳一) <[email protected]>
AuthorDate: Fri, 02 Feb 2024 03:51:58
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Tue, 06 Feb 2024 10:51:11 +01:00
x86/boot: Add a message about ignored early NMIs
Commit
78a509fba9c9 ("x86/boot: Ignore NMIs during very early boot")
added an empty handler in early boot stage to avoid boot failure due to
spurious NMIs.
Add a diagnostic message to show that early NMIs have occurred.
[ bp: Touchups. ]
[ Committer note: tested by stopping the guest really early and
injecting NMIs through qemu's monitor. Result:
early console in setup code
Spurious early NMIs ignored: 13
... ]
Suggested-by: Borislav Petkov <[email protected]>
Suggested-by: H. Peter Anvin <[email protected]>
Signed-off-by: Jun'ichi Nomura <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Acked-by: Kirill A. Shutemov <[email protected]>
Link: https://lore.kernel.org/lkml/20231130103339.GCZWhlA196uRklTMNF@fat_crate.local
---
arch/x86/boot/compressed/ident_map_64.c | 2 +-
arch/x86/boot/compressed/misc.c | 7 +++++++
arch/x86/boot/compressed/misc.h | 1 +
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index d040080..4a029ba 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -389,5 +389,5 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
void do_boot_nmi_trap(struct pt_regs *regs, unsigned long error_code)
{
- /* Empty handler to ignore NMI during early boot */
+ spurious_nmi_count++;
}
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index bf2aac4..bd6857a 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -52,6 +52,7 @@ struct port_io_ops pio_ops;
memptr free_mem_ptr;
memptr free_mem_end_ptr;
+int spurious_nmi_count;
static char *vidmem;
static int vidport;
@@ -506,6 +507,12 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
/* Disable exception handling before booting the kernel */
cleanup_exception_handling();
+ if (spurious_nmi_count) {
+ error_putstr("Spurious early NMIs ignored: ");
+ error_putdec(spurious_nmi_count);
+ error_putstr("\n");
+ }
+
return output + entry_offset;
}
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 6502bc6..b353a7b 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -59,6 +59,7 @@ extern char _head[], _end[];
/* misc.c */
extern memptr free_mem_ptr;
extern memptr free_mem_end_ptr;
+extern int spurious_nmi_count;
void *malloc(int size);
void free(void *where);
void __putstr(const char *s);
Hi Borislav Petkov,
My main job is to develop driver software based on arm64 features.
Sometimes I also
help to analyze and solve problems found by other departments on x86
servers, and
contribute repair patches to the community.
I sent you the almost same patch before
(https://lore.kernel.org/all/[email protected]/),
but you kept struggling with my grammar rather than the code logic itself,
and even questioned my motives for sending the patch.
(https://lore.kernel.org/all/[email protected]/)
Until just now, I saw your completely different responses to the same
patch.
I'm not pointing this out to change anything, but in the hope that other
people or
my colleagues would avoid encountering similar things.
Regards,
Zeng Heng
Hi Zeng Heng,
On Wed, Apr 03, 2024 at 02:32:45PM +0800, Zeng Heng wrote:
> Until just now, I saw your completely different responses to the same patch.
Lemme explain how I see the situation.
You sent a patch:
https://lore.kernel.org/all/[email protected]/
which had a commit message which tried to explain what happens. And
I tried to parse your commit message and understand what you're trying
to do but there never was a clear explanation.
When I read "If kdump is enabled, when using mce_inject to inject
errors..." then I think, oh great, more experiments. ;-\
And no, I don't want to add code to early boot just to make some weird
experiments happy.
Yeah yeah, an MCE can happen very early but until a real reproducer, I'm
not convinced.
Now that other patch's commit message has at least a bit more clear
explanation how you can *actually* cause this. And I still would've
asked how *exactly* this happens but it is kinda clear: you can run perf
and generate an NMI storm and then have two back-to-back NMIs.
And I'm still not crazy about having an empty early NMI handler either
thus I suggested to make it at least say something so that we're aware
that early NMIs have happened.
So if it is not clear *why* a patch is being done, then it goes nowhere.
Because you'll go your merry way and "develop driver software based on
arm64 features" or whatever else you get to do but the maintainers will
be left to be dealing with your code indefinitely.
I hope this makes it more clear.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette