[PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[Xin Li (Intel)]

Commit 55617fb991df added a bunch of tests to the int $0x80 path, however
they are unnecessary and event wrong in fact under FRED.

First FRED distinguishes external interrupts from software interrupts,
thus int80_emulation() should NEVER be called for handling an external
interrupt, and then int80_is_external() should be skipped under FRED.

Second, the FRED kernel entry handler NEVER dispatches INTx, which is
of event type EVENT_TYPE_SWINT, so the user mode checking in
do_int80_emulation() is redundant, and should be skipped.

It might be even better to strip down do_int80_emulation() to a lean
fred_int80_emulation(), not to mention int80_emulation() does a
CLEAR_BRANCH_HISTORY.

Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 arch/x86/entry/common.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 6de50b80702e..aed745fc8333 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -174,6 +174,18 @@ static __always_inline bool int80_is_external(void)
 	const unsigned int offs = (0x80 / 32) * 0x10;
 	const u32 bit = BIT(0x80 % 32);
 
+	/*
+	 * FRED distinguishes external interrupts from software interrupts
+	 * with different event types, EVENT_TYPE_EXTINT v.s. EVENT_TYPE_SWINT,
+	 * thus we should NEVER get here when FRED is enabled, and then the
+	 * following APIC ISR check makes no sense.
+	 *
+	 * Furthermore, the external interrupt vector 0x80 is available as
+	 * a hardware interrupt under FRED.
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_FRED))
+		return false;
+
 	/* The local APIC on XENPV guests is fake */
 	if (cpu_feature_enabled(X86_FEATURE_XENPV))
 		return false;
@@ -211,8 +223,14 @@ __visible noinstr void do_int80_emulation(struct pt_regs *regs)
 {
 	int nr;
 
-	/* Kernel does not use INT $0x80! */
-	if (unlikely(!user_mode(regs))) {
+	/*
+	 * Kernel does not use INT $0x80!
+	 *
+	 * The FRED kernel entry handler NEVER dispatches INTx (panic or
+	 * oops otherwise), so it is redundant to check user mode here.
+	 */
+	if (!cpu_feature_enabled(X86_FEATURE_FRED) &&
+	    unlikely(!user_mode(regs))) {
 		irqentry_enter(regs);
 		instrumentation_begin();
 		panic("Unexpected external interrupt 0x80\n");

base-commit: 86d1b22a75cffa50160e4621da00311e6f6f48de
-- 
2.44.0

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[H. Peter Anvin]

On 4/12/24 16:40, Xin Li (Intel) wrote:
> Commit 55617fb991df added a bunch of tests to the int $0x80 path, however
> they are unnecessary and event wrong in fact under FRED.
> 
> First FRED distinguishes external interrupts from software interrupts,
> thus int80_emulation() should NEVER be called for handling an external
> interrupt, and then int80_is_external() should be skipped under FRED.
> 
> Second, the FRED kernel entry handler NEVER dispatches INTx, which is
> of event type EVENT_TYPE_SWINT, so the user mode checking in
> do_int80_emulation() is redundant, and should be skipped.
> 
> It might be even better to strip down do_int80_emulation() to a lean
> fred_int80_emulation(), not to mention int80_emulation() does a
> CLEAR_BRANCH_HISTORY.
> 
> Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
>   arch/x86/entry/common.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)

Note: this is the minimal bug fix versions and belongs in x86/urgent.

	-hpa

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[Borislav Petkov]

On Fri, Apr 12, 2024 at 04:40:58PM -0700, Xin Li (Intel) wrote:
> Commit 55617fb991df

Use the full commit abbreviation when mentioning commits:

"Commit

  55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")

..."

> added a bunch of tests to the int $0x80 path,

Added a bunch of tests?

What does that even mean?

> however they are unnecessary and event wrong in fact under FRED.

Are the bunch of tests wrong or is do_int80_emulation() simply the wrong
handler to use on a FRED?

> First FRED distinguishes external interrupts from software interrupts,
> thus int80_emulation() should NEVER be called for handling an external
> interrupt, and then int80_is_external() should be skipped under FRED.
> 
> Second, the FRED kernel entry handler NEVER dispatches INTx, which is
> of event type EVENT_TYPE_SWINT, so the user mode checking in
> do_int80_emulation() is redundant, and should be skipped.
> 
> It might be even better to strip down do_int80_emulation() to a lean
> fred_int80_emulation(), not to mention int80_emulation() does a
> CLEAR_BRANCH_HISTORY.

Yah, how about you do a FRED-specific INT80 handler instead of
sprinkling moar tests around? fred_intx() looks like the right place to
stuff it in...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[Xin Li]

On 4/16/2024 3:11 AM, Borislav Petkov wrote:
> On Fri, Apr 12, 2024 at 04:40:58PM -0700, Xin Li (Intel) wrote:
>> Commit 55617fb991df
> 
> Use the full commit abbreviation when mentioning commits:
> 
> "Commit
> 
>    55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")
> 
> .."

oh, I should have done it!

> 
>> added a bunch of tests to the int $0x80 path,
> 
> Added a bunch of tests?
> 

'checks' might be a better term to use?

> What does that even mean?
> 
>> however they are unnecessary and event wrong in fact under FRED.
> 
> Are the bunch of tests wrong or is do_int80_emulation() simply the wrong
> handler to use on a FRED?

I think the explanations are below, and you saw it a bit later.

BTW, comments are added around the code changes.

> 
>> First FRED distinguishes external interrupts from software interrupts,
>> thus int80_emulation() should NEVER be called for handling an external
>> interrupt, and then int80_is_external() should be skipped under FRED.
>>
>> Second, the FRED kernel entry handler NEVER dispatches INTx, which is
>> of event type EVENT_TYPE_SWINT, so the user mode checking in
>> do_int80_emulation() is redundant, and should be skipped.
>>
>> It might be even better to strip down do_int80_emulation() to a lean
>> fred_int80_emulation(), not to mention int80_emulation() does a
>> CLEAR_BRANCH_HISTORY.
> 
> Yah, how about you do a FRED-specific INT80 handler instead of
> sprinkling moar tests around? fred_intx() looks like the right place to
> stuff it in...
> 

I will add fred_int80_emulation(), which duplicates a big part of
do_int80_emulation(), however this seems more readable.

Thanks!
     Xin

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[H. Peter Anvin]

On April 16, 2024 3:11:47 AM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Fri, Apr 12, 2024 at 04:40:58PM -0700, Xin Li (Intel) wrote:
>> Commit 55617fb991df
>
>Use the full commit abbreviation when mentioning commits:
>
>"Commit
>
>  55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")
>
>..."
>
>> added a bunch of tests to the int $0x80 path,
>
>Added a bunch of tests?
>
>What does that even mean?
>
>> however they are unnecessary and event wrong in fact under FRED.
>
>Are the bunch of tests wrong or is do_int80_emulation() simply the wrong
>handler to use on a FRED?
>
>> First FRED distinguishes external interrupts from software interrupts,
>> thus int80_emulation() should NEVER be called for handling an external
>> interrupt, and then int80_is_external() should be skipped under FRED.
>> 
>> Second, the FRED kernel entry handler NEVER dispatches INTx, which is
>> of event type EVENT_TYPE_SWINT, so the user mode checking in
>> do_int80_emulation() is redundant, and should be skipped.
>> 
>> It might be even better to strip down do_int80_emulation() to a lean
>> fred_int80_emulation(), not to mention int80_emulation() does a
>> CLEAR_BRANCH_HISTORY.
>
>Yah, how about you do a FRED-specific INT80 handler instead of
>sprinkling moar tests around? fred_intx() looks like the right place to
>stuff it in...
>

The question was if you wanted a quick fix for x86/urgent. It's pretty obvious that a FRED fork of the int80 code is called for.

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[Borislav Petkov]

On Tue, Apr 16, 2024 at 11:05:09AM -0700, H. Peter Anvin wrote:
> The question was if you wanted a quick fix for x86/urgent. It's pretty
> obvious that a FRED fork of the int80 code is called for.

Yeah, I'm thinking the FRED fork would be even easier to send to Linus
now since no existing hw will break.

Btw, I presume that patch needs to have

Fixes: 55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")

?

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[H. Peter Anvin]

On 4/16/24 11:07, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 11:05:09AM -0700, H. Peter Anvin wrote:
>> The question was if you wanted a quick fix for x86/urgent. It's pretty
>> obvious that a FRED fork of the int80 code is called for.
> 
> Yeah, I'm thinking the FRED fork would be even easier to send to Linus
> now since no existing hw will break.

Looking at the resulting code (Xin's v2 path), I would agree with you. 
The code is downright trivial.

> Btw, I presume that patch needs to have
> 
> Fixes: 55617fb991df ("x86/entry: Do not allow external 0x80 interrupts")
> 

Yes. My fault, I gave Xin incorrect information w.r.t. Fixes: tags.

	-hpa

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[Borislav Petkov]

On Tue, Apr 16, 2024 at 11:11:44AM -0700, H. Peter Anvin wrote:
> Yes. My fault, I gave Xin incorrect information w.r.t. Fixes: tags.

No worries, I'll fix that up before applying.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v1 1/1] x86/fred: Fix int80 emulation for FRED

[H. Peter Anvin]

On 4/16/24 11:18, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 11:11:44AM -0700, H. Peter Anvin wrote:
>> Yes. My fault, I gave Xin incorrect information w.r.t. Fixes: tags.
> 
> No worries, I'll fix that up before applying.

Let me submit my comments (about comments) first :)

	-hpa