[PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

[PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

[Hou Wenlong]

When I reviewed the FRED code and attempted to implement a FRED-like
event delivery for my PV guest, I encountered two problems which I may
have misunderstood.

One issue is that FRED can be disabled in trap_init(), but
sysvec_install() can be called before trap_init(), thus the system
interrupt handler is not installed into the IDT if FRED is disabled
later. Initially, I attempted to parse the cmdline and decide whether to
enable or disable FRED after parse_early_param(). However, I ultimately
chose to always install the system handler into the IDT in
sysvec_install(), which is simple and should be sufficient.

Another problem is that the page fault handler (exc_page_fault()) is
installed into the IDT before FRED is enabled. Consequently, if a #PF is
triggered in this gap, the handler would receive the wrong CR2 from the
stack if FRED feature is present. To address this, I added a page fault
entry stub for FRED similar to the debug entry. However, I'm uncertain
whether this is enough reason to add a new entry. Perhaps a static key
may suffice to indicate whether FRED setup is completed and the handler
can use it.

Note: I didn't test them on the Intel emulator, as I'm not familiar with
configuring it to boot with my compiled kernel.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Xin Li <xin3.li@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Hou Wenlong (2):
  x86/fred: Always install system interrupt handler into IDT
  x86/fred: Add a page fault entry stub for FRED

 arch/x86/entry/entry_fred.c     |  2 +-
 arch/x86/include/asm/idtentry.h | 15 +++++++++++++--
 arch/x86/mm/fault.c             | 19 +++++++++++++++----
 3 files changed, 29 insertions(+), 7 deletions(-)

--
2.31.1

[PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Hou Wenlong]

In sysvec_install(), the system interrupt handler is not installed into
the IDT when the FRED feature is present, but FRED can be disabled
in trap_init(). However, sysvec_install() can be used
before trap_init(), e.g., the HYPERVISOR_CALLBACK_VECTOR handler is
registered in kvm_guest_init(), which is called by setup_arch() before
trap_init(). If FRED is disabled later, then the spurious_interrupt()
handler will be used due to the handler not being installed into the
IDT. Therefore, always install system interrupt handler into IDT.

Fixes: 3810da12710a ("x86/fred: Add a fred= cmdline param")
Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/include/asm/idtentry.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index d4f24499b256..daee9f7765bc 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -470,8 +470,7 @@ static inline void fred_install_sysvec(unsigned int vector, const idtentry_t fun
 #define sysvec_install(vector, function) {				\
 	if (cpu_feature_enabled(X86_FEATURE_FRED))			\
 		fred_install_sysvec(vector, function);			\
-	else								\
-		idt_install_sysvec(vector, asm_##function);		\
+	idt_install_sysvec(vector, asm_##function);			\
 }
 
 #else /* !__ASSEMBLY__ */
-- 
2.31.1

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Xin Li]

On 6/21/2024 6:12 AM, Hou Wenlong wrote:
> In sysvec_install(), the system interrupt handler is not installed into
> the IDT when the FRED feature is present, but FRED can be disabled
> in trap_init(). However, sysvec_install() can be used
> before trap_init(), e.g., the HYPERVISOR_CALLBACK_VECTOR handler is
> registered in kvm_guest_init(), which is called by setup_arch() before
> trap_init(). If FRED is disabled later, then the spurious_interrupt()
> handler will be used due to the handler not being installed into the
> IDT. Therefore, always install system interrupt handler into IDT.
> 
> Fixes: 3810da12710a ("x86/fred: Add a fred= cmdline param")
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>   arch/x86/include/asm/idtentry.h | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
> index d4f24499b256..daee9f7765bc 100644
> --- a/arch/x86/include/asm/idtentry.h
> +++ b/arch/x86/include/asm/idtentry.h
> @@ -470,8 +470,7 @@ static inline void fred_install_sysvec(unsigned int vector, const idtentry_t fun
>   #define sysvec_install(vector, function) {				\
>   	if (cpu_feature_enabled(X86_FEATURE_FRED))			\
>   		fred_install_sysvec(vector, function);			\
> -	else								\
> -		idt_install_sysvec(vector, asm_##function);		\

empty line, it improves readability.

And please put a comment here to explain why this is unconditionally
done for IDT.

> +	idt_install_sysvec(vector, asm_##function);			\
>   }
>   
>   #else /* !__ASSEMBLY__ */

Thanks!
     Xin

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Thomas Gleixner]

On Tue, Jun 25 2024 at 02:19, Xin Li wrote:
> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
>> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
>> index d4f24499b256..daee9f7765bc 100644
>> --- a/arch/x86/include/asm/idtentry.h
>> +++ b/arch/x86/include/asm/idtentry.h
>> @@ -470,8 +470,7 @@ static inline void fred_install_sysvec(unsigned int vector, const idtentry_t fun
>>   #define sysvec_install(vector, function) {				\
>>   	if (cpu_feature_enabled(X86_FEATURE_FRED))			\
>>   		fred_install_sysvec(vector, function);			\
>> -	else								\
>> -		idt_install_sysvec(vector, asm_##function);		\
>
> empty line, it improves readability.
>
> And please put a comment here to explain why this is unconditionally
> done for IDT.

Wait. If we need this during early boot, then why don't we enable FRED
earlier?

Thanks,

        tglx

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Xin Li]

On 6/25/2024 5:31 AM, Thomas Gleixner wrote:
> On Tue, Jun 25 2024 at 02:19, Xin Li wrote:
>> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
>>> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
>>> index d4f24499b256..daee9f7765bc 100644
>>> --- a/arch/x86/include/asm/idtentry.h
>>> +++ b/arch/x86/include/asm/idtentry.h
>>> @@ -470,8 +470,7 @@ static inline void fred_install_sysvec(unsigned int vector, const idtentry_t fun
>>>    #define sysvec_install(vector, function) {				\
>>>    	if (cpu_feature_enabled(X86_FEATURE_FRED))			\
>>>    		fred_install_sysvec(vector, function);			\
>>> -	else								\
>>> -		idt_install_sysvec(vector, asm_##function);		\
>>
>> empty line, it improves readability.
>>
>> And please put a comment here to explain why this is unconditionally
>> done for IDT.
> 
> Wait. If we need this during early boot, then why don't we enable FRED
> earlier?

Unconditionally call idt_install_sysvec() is still needed, right?

But this sounds a smart move to me!  Because it helps to deal with not
only the initialization order issue, but also the following cases in a
longer term:

1) BIOS enables FRED and keeps it enabled when transferring control to
    Linux.  Then we just need to disable FRED when it is disabled in the
    kernel command line.

2) IDT support is removed from a kernel config thus a kernel binary, say
    a new kernel config CONFIG_X86_IDT is added and set to N.

And we need to:

1) Find a place to enable FRED as early as possible if not yet enabled.

2) Disable FRED when fred=off is in the kernel command line.

Anything I missed?

Thanks!
     Xin

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Thomas Gleixner]

On Tue, Jun 25 2024 at 09:30, Xin Li wrote:

> On 6/25/2024 5:31 AM, Thomas Gleixner wrote:
>> On Tue, Jun 25 2024 at 02:19, Xin Li wrote:
>>> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
>>>> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
>>>> index d4f24499b256..daee9f7765bc 100644
>>>> --- a/arch/x86/include/asm/idtentry.h
>>>> +++ b/arch/x86/include/asm/idtentry.h
>>>> @@ -470,8 +470,7 @@ static inline void fred_install_sysvec(unsigned int vector, const idtentry_t fun
>>>>    #define sysvec_install(vector, function) {				\
>>>>    	if (cpu_feature_enabled(X86_FEATURE_FRED))			\
>>>>    		fred_install_sysvec(vector, function);			\
>>>> -	else								\
>>>> -		idt_install_sysvec(vector, asm_##function);		\
>>>
>>> empty line, it improves readability.
>>>
>>> And please put a comment here to explain why this is unconditionally
>>> done for IDT.
>> 
>> Wait. If we need this during early boot, then why don't we enable FRED
>> earlier?
>
> Unconditionally call idt_install_sysvec() is still needed, right?

Not when we enable FRED early enough.

> But this sounds a smart move to me!  Because it helps to deal with not
> only the initialization order issue, but also the following cases in a
> longer term:
>
> 1) BIOS enables FRED and keeps it enabled when transferring control to
>     Linux.  Then we just need to disable FRED when it is disabled in the
>     kernel command line.
>
> 2) IDT support is removed from a kernel config thus a kernel binary, say
>     a new kernel config CONFIG_X86_IDT is added and set to N.
>
> And we need to:
>
> 1) Find a place to enable FRED as early as possible if not yet enabled.
>
> 2) Disable FRED when fred=off is in the kernel command line.
>
> Anything I missed?

No.

Thanks,

        tglx

RE: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Li, Xin3]

> >>> And please put a comment here to explain why this is unconditionally
> >>> done for IDT.
> >>
> >> Wait. If we need this during early boot, then why don't we enable
> >> FRED earlier?
> >
> > Unconditionally call idt_install_sysvec() is still needed, right?
> 
> Not when we enable FRED early enough.
> 

What if FRED is disabled in the kernel command line?

"fred=off" gets parsed in a later stage of the kernel initialization sequence.

Thanks!
    Xin

RE: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Thomas Gleixner]

On Tue, Jun 25 2024 at 17:08, Li, Xin3 wrote:
>> >>> And please put a comment here to explain why this is unconditionally
>> >>> done for IDT.
>> >>
>> >> Wait. If we need this during early boot, then why don't we enable
>> >> FRED earlier?
>> >
>> > Unconditionally call idt_install_sysvec() is still needed, right?
>> 
>> Not when we enable FRED early enough.
>> 
>
> What if FRED is disabled in the kernel command line?
>
> "fred=off" gets parsed in a later stage of the kernel initialization sequence.

You can parse it early on. There are examples for that like encryption
and other things which need to be decided way before the real parsing
happens.

Thanks,

        tglx

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Hou Wenlong]

On Tue, Jun 25, 2024 at 05:19:20PM +0800, Xin Li wrote:
> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
> >In sysvec_install(), the system interrupt handler is not installed into
> >the IDT when the FRED feature is present, but FRED can be disabled
> >in trap_init(). However, sysvec_install() can be used
> >before trap_init(), e.g., the HYPERVISOR_CALLBACK_VECTOR handler is
> >registered in kvm_guest_init(), which is called by setup_arch() before
> >trap_init(). If FRED is disabled later, then the spurious_interrupt()
> >handler will be used due to the handler not being installed into the
> >IDT. Therefore, always install system interrupt handler into IDT.
> >
> >Fixes: 3810da12710a ("x86/fred: Add a fred= cmdline param")
> >Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> >---
> >  arch/x86/include/asm/idtentry.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
> >index d4f24499b256..daee9f7765bc 100644
> >--- a/arch/x86/include/asm/idtentry.h
> >+++ b/arch/x86/include/asm/idtentry.h
> >@@ -470,8 +470,7 @@ static inline void fred_install_sysvec(unsigned int vector, const idtentry_t fun
> >  #define sysvec_install(vector, function) {				\
> >  	if (cpu_feature_enabled(X86_FEATURE_FRED))			\
> >  		fred_install_sysvec(vector, function);			\
> >-	else								\
> >-		idt_install_sysvec(vector, asm_##function);		\
> 
> empty line, it improves readability.
> 
> And please put a comment here to explain why this is unconditionally
> done for IDT.
> 
Hi Xin,

It seems preferable to parse the FRED command line and disable FRED
early instead of using this method. As mentioned in my cover letter, I
initially attempted to fix the problem this way (by parsing the command
line in cpu_parse_early_param()). Should I send a new patch for it, or
will you be covering it in your work to enable FRED early?

Thanks!

> >+	idt_install_sysvec(vector, asm_##function);			\
> >  }
> >  #else /* !__ASSEMBLY__ */
> 
> Thanks!
>     Xin

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Xin Li]

On 6/28/2024 2:36 AM, Hou Wenlong wrote:
> Hi Xin,
> 
> It seems preferable to parse the FRED command line and disable FRED
> early instead of using this method. As mentioned in my cover letter, I
> initially attempted to fix the problem this way (by parsing the command
> line in cpu_parse_early_param()). Should I send a new patch for it, or
> will you be covering it in your work to enable FRED early?
> 

I have done it in my patches that enables FRED early, but if you want,
you can post it, because you're a key contributor in this work.

Thanks!
     Xin

> Thanks!
> 
>>> +	idt_install_sysvec(vector, asm_##function);			\
>>>   }
>>>   #else /* !__ASSEMBLY__ */
>>
>> Thanks!
>>      Xin
> 

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Xin Li]

On 6/28/2024 8:18 AM, Xin Li wrote:
> On 6/28/2024 2:36 AM, Hou Wenlong wrote:
>> Hi Xin,
>>
>> It seems preferable to parse the FRED command line and disable FRED
>> early instead of using this method. As mentioned in my cover letter, I
>> initially attempted to fix the problem this way (by parsing the command
>> line in cpu_parse_early_param()). Should I send a new patch for it, or
>> will you be covering it in your work to enable FRED early?
>>
> 
> I have done it in my patches that enables FRED early, but if you want,
> you can post it, because you're a key contributor in this work.
> 

Please let me know if you want to do it.

Thanks!
     Xin

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Hou Wenlong]

On Wed, Jul 03, 2024 at 01:16:29AM +0800, Xin Li wrote:
> On 6/28/2024 8:18 AM, Xin Li wrote:
> >On 6/28/2024 2:36 AM, Hou Wenlong wrote:
> >>Hi Xin,
> >>
> >>It seems preferable to parse the FRED command line and disable FRED
> >>early instead of using this method. As mentioned in my cover letter, I
> >>initially attempted to fix the problem this way (by parsing the command
> >>line in cpu_parse_early_param()). Should I send a new patch for it, or
> >>will you be covering it in your work to enable FRED early?
> >>
> >
> >I have done it in my patches that enables FRED early, but if you want,
> >you can post it, because you're a key contributor in this work.
> >
> 
> Please let me know if you want to do it.
> 
Hi Xin,

Sorry for forgetting to reply. I think it would be better for the fix to
be covered in your work.

Thanks!

> Thanks!
>     Xin

Re: [PATCH 1/2] x86/fred: Always install system interrupt handler into IDT

[Xin Li]

On 7/2/2024 7:44 PM, Hou Wenlong wrote:
> On Wed, Jul 03, 2024 at 01:16:29AM +0800, Xin Li wrote:
>> On 6/28/2024 8:18 AM, Xin Li wrote:
>>> On 6/28/2024 2:36 AM, Hou Wenlong wrote:
>>>> Hi Xin,
>>>>
>>>> It seems preferable to parse the FRED command line and disable FRED
>>>> early instead of using this method. As mentioned in my cover letter, I
>>>> initially attempted to fix the problem this way (by parsing the command
>>>> line in cpu_parse_early_param()). Should I send a new patch for it, or
>>>> will you be covering it in your work to enable FRED early?
>>>>
>>>
>>> I have done it in my patches that enables FRED early, but if you want,
>>> you can post it, because you're a key contributor in this work.
>>>
>>
>> Please let me know if you want to do it.
>>
> Hi Xin,
> 
> Sorry for forgetting to reply. I think it would be better for the fix to
> be covered in your work.

NP, thanks!

[PATCH 2/2] x86/fred: Add a page fault entry stub for FRED

[Hou Wenlong]

The page fault handler is installed into the IDT before FRED setup is
done. During this gap, the handler would retrieve the wrong CR2 from the
stack if FRED is present and a #PF is triggered. Similar to the debug
entry, a page fault entry stub for FRED should be added.

Fixes: 14619d912b65 ("x86/fred: FRED entry/exit and dispatch code")
Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/entry/entry_fred.c     |  2 +-
 arch/x86/include/asm/idtentry.h | 12 ++++++++++++
 arch/x86/mm/fault.c             | 19 +++++++++++++++----
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/arch/x86/entry/entry_fred.c b/arch/x86/entry/entry_fred.c
index f004a4dc74c2..73ea483fec87 100644
--- a/arch/x86/entry/entry_fred.c
+++ b/arch/x86/entry/entry_fred.c
@@ -181,7 +181,7 @@ static noinstr void fred_hwexc(struct pt_regs *regs, unsigned long error_code)
 {
 	/* Optimize for #PF. That's the only exception which matters performance wise */
 	if (likely(regs->fred_ss.vector == X86_TRAP_PF))
-		return exc_page_fault(regs, error_code);
+		return fred_exc_page_fault(regs, error_code);
 
 	switch (regs->fred_ss.vector) {
 	case X86_TRAP_DE: return exc_divide_error(regs);
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index daee9f7765bc..a0fd9cfa08d6 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -86,6 +86,7 @@ static __always_inline void __##func(struct pt_regs *regs)
 #define DECLARE_IDTENTRY_ERRORCODE(vector, func)			\
 	asmlinkage void asm_##func(void);				\
 	asmlinkage void xen_asm_##func(void);				\
+	void fred_##func(struct pt_regs *regs, unsigned long error_code);\
 	__visible void func(struct pt_regs *regs, unsigned long error_code)
 
 /**
@@ -180,6 +181,17 @@ noinstr void fred_##func(struct pt_regs *regs)
 #define DEFINE_IDTENTRY_RAW_ERRORCODE(func)				\
 __visible noinstr void func(struct pt_regs *regs, unsigned long error_code)
 
+/**
+ * DEFINE_FRED_RAW_ERRORCODE - Emit code for raw FRED entry points
+ * @func:	Function name of the entry point
+ *
+ * @func is called from the FRED event dispatcher with interrupts disabled.
+ *
+ * See @DEFINE_IDTENTRY_RAW_ERRORCODE for further details.
+ */
+#define DEFINE_FREDENTRY_RAW_ERRORCODE(func)			\
+__visible noinstr void fred_##func(struct pt_regs *regs, unsigned long error_code)
+
 /**
  * DECLARE_IDTENTRY_IRQ - Declare functions for device interrupt IDT entry
  *			  points (common/spurious)
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index e6c469b323cc..712dcf491daa 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1490,12 +1490,10 @@ handle_page_fault(struct pt_regs *regs, unsigned long error_code,
 	}
 }
 
-DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
+static noinstr void __exc_page_fault(struct pt_regs *regs, unsigned long error_code,
+				     unsigned long address)
 {
 	irqentry_state_t state;
-	unsigned long address;
-
-	address = cpu_feature_enabled(X86_FEATURE_FRED) ? fred_event_data(regs) : read_cr2();
 
 	prefetchw(&current->mm->mmap_lock);
 
@@ -1541,3 +1539,16 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
 
 	irqentry_exit(regs, state);
 }
+
+DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
+{
+	__exc_page_fault(regs, error_code, read_cr2());
+}
+
+#ifdef CONFIG_X86_FRED
+DEFINE_FREDENTRY_RAW_ERRORCODE(exc_page_fault)
+{
+	/* FRED #PF stores CR2 on the stack. */
+	__exc_page_fault(regs, error_code, fred_event_data(regs));
+}
+#endif
-- 
2.31.1

Re: [PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

[Xin Li]

On 6/21/2024 6:12 AM, Hou Wenlong wrote:
> When I reviewed the FRED code and attempted to implement a FRED-like
> event delivery for my PV guest, I encountered two problems which I may
> have misunderstood.

Hi Wenlong,

Thanks for bringing the issues up.

> 
> One issue is that FRED can be disabled in trap_init(), but
> sysvec_install() can be called before trap_init(), thus the system
> interrupt handler is not installed into the IDT if FRED is disabled
> later. Initially, I attempted to parse the cmdline and decide whether to
> enable or disable FRED after parse_early_param(). However, I ultimately
> chose to always install the system handler into the IDT in
> sysvec_install(), which is simple and should be sufficient.

Which module with a system vector gets initialized before trap_init() is
called?

> Another problem is that the page fault handler (exc_page_fault()) is
> installed into the IDT before FRED is enabled. Consequently, if a #PF is
> triggered in this gap, the handler would receive the wrong CR2 from the
> stack if FRED feature is present. To address this, I added a page fault
> entry stub for FRED similar to the debug entry. However, I'm uncertain
> whether this is enough reason to add a new entry. Perhaps a static key
> may suffice to indicate whether FRED setup is completed and the handler
> can use it.

How could a #PF get triggered during that gap?

Initialization time funnies are really unpleasant.

Thanks!
     Xin

Re: [PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

[Hou Wenlong]

On Sat, Jun 22, 2024 at 08:31:26AM +0800, Xin Li wrote:
> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
> >When I reviewed the FRED code and attempted to implement a FRED-like
> >event delivery for my PV guest, I encountered two problems which I may
> >have misunderstood.
> 
> Hi Wenlong,
> 
> Thanks for bringing the issues up.
> 
Thanks for your kind reply.
 
> >
> >One issue is that FRED can be disabled in trap_init(), but
> >sysvec_install() can be called before trap_init(), thus the system
> >interrupt handler is not installed into the IDT if FRED is disabled
> >later. Initially, I attempted to parse the cmdline and decide whether to
> >enable or disable FRED after parse_early_param(). However, I ultimately
> >chose to always install the system handler into the IDT in
> >sysvec_install(), which is simple and should be sufficient.
> 
> Which module with a system vector gets initialized before trap_init() is
> called?
>
Sorry, I didn't mention the case here. I see sysvec_install() is used
only in the guest part (KVM, HYPERV) currently. For example, the KVM
guest will register the HYPERVISOR_CALLBACK_VECTOR APF handler in
kvm_guest_init(), which is called before trap_init(). So if only the FRED
handler is registered and FRED is disabled in trap_init() later, then the
IDT handler of the APF handler is missing.

> >Another problem is that the page fault handler (exc_page_fault()) is
> >installed into the IDT before FRED is enabled. Consequently, if a #PF is
> >triggered in this gap, the handler would receive the wrong CR2 from the
> >stack if FRED feature is present. To address this, I added a page fault
> >entry stub for FRED similar to the debug entry. However, I'm uncertain
> >whether this is enough reason to add a new entry. Perhaps a static key
> >may suffice to indicate whether FRED setup is completed and the handler
> >can use it.
> 
> How could a #PF get triggered during that gap?
> 
> Initialization time funnies are really unpleasant.
>
I'm not sure if there will be a #PF during that gap; I just received the
wrong fault address when I made a mistake in that gap and a #PF
occurred. Before idt_setup_early_pf(), the registered page fault handler
is do_early_exception(), which uses native_read_cr2(). However, after
that, the page fault handler had been changed to exc_page_fault(), so if
something bad happened and an unexpected #PF occurred, the fault address
in the error output will be wrong, although the CR2 in __show_regs() is
correct. I'm not sure if this matters or not since the kernel will panic
at that time.

Thanks!
> Thanks!
>     Xin

Re: [PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

[Xin Li]

On 6/23/2024 11:21 PM, Hou Wenlong wrote:
> On Sat, Jun 22, 2024 at 08:31:26AM +0800, Xin Li wrote:
>> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
>>> One issue is that FRED can be disabled in trap_init(), but
>>> sysvec_install() can be called before trap_init(), thus the system
>>> interrupt handler is not installed into the IDT if FRED is disabled
>>> later. Initially, I attempted to parse the cmdline and decide whether to
>>> enable or disable FRED after parse_early_param(). However, I ultimately
>>> chose to always install the system handler into the IDT in
>>> sysvec_install(), which is simple and should be sufficient.
>>
>> Which module with a system vector gets initialized before trap_init() is
>> called?
>>
> Sorry, I didn't mention the case here. I see sysvec_install() is used
> only in the guest part (KVM, HYPERV) currently. For example, the KVM
> guest will register the HYPERVISOR_CALLBACK_VECTOR APF handler in
> kvm_guest_init(), which is called before trap_init(). So if only the FRED
> handler is registered and FRED is disabled in trap_init() later, then the
> IDT handler of the APF handler is missing.

This is a bug!  Your fix looks good to me.

>>> Another problem is that the page fault handler (exc_page_fault()) is
>>> installed into the IDT before FRED is enabled. Consequently, if a #PF is
>>> triggered in this gap, the handler would receive the wrong CR2 from the
>>> stack if FRED feature is present. To address this, I added a page fault
>>> entry stub for FRED similar to the debug entry. However, I'm uncertain
>>> whether this is enough reason to add a new entry. Perhaps a static key
>>> may suffice to indicate whether FRED setup is completed and the handler
>>> can use it.
>>
>> How could a #PF get triggered during that gap?
>>
>> Initialization time funnies are really unpleasant.
>>
> I'm not sure if there will be a #PF during that gap; I just received the
> wrong fault address when I made a mistake in that gap and a #PF
> occurred. Before idt_setup_early_pf(), the registered page fault handler
> is do_early_exception(), which uses native_read_cr2(). However, after
> that, the page fault handler had been changed to exc_page_fault(), so if
> something bad happened and an unexpected #PF occurred, the fault address
> in the error output will be wrong, although the CR2 in __show_regs() is
> correct. I'm not sure if this matters or not since the kernel will panic
> at that time.

So this doesn't sound a real problem, right?

We could simply do:

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index e6c469b323cc..e500777ed2b4 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1495,7 +1495,7 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
            irqentry_state_t state;
            unsigned long address;

-       address = cpu_feature_enabled(X86_FEATURE_FRED) ? 
fred_event_data(regs) : read_cr2();
+       address = native_read_cr4() & X86_CR4_FRED ? 
fred_event_data(regs) : read_cr2();

            prefetchw(&current->mm->mmap_lock);


But the page fault handler is an extreme hot path, it's not worth it.

Thanks!
        Xin

Re: [PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

[H. Peter Anvin]

On June 25, 2024 2:09:29 AM PDT, Xin Li <xin@zytor.com> wrote:
>On 6/23/2024 11:21 PM, Hou Wenlong wrote:
>> On Sat, Jun 22, 2024 at 08:31:26AM +0800, Xin Li wrote:
>>> On 6/21/2024 6:12 AM, Hou Wenlong wrote:
>>>> One issue is that FRED can be disabled in trap_init(), but
>>>> sysvec_install() can be called before trap_init(), thus the system
>>>> interrupt handler is not installed into the IDT if FRED is disabled
>>>> later. Initially, I attempted to parse the cmdline and decide whether to
>>>> enable or disable FRED after parse_early_param(). However, I ultimately
>>>> chose to always install the system handler into the IDT in
>>>> sysvec_install(), which is simple and should be sufficient.
>>> 
>>> Which module with a system vector gets initialized before trap_init() is
>>> called?
>>> 
>> Sorry, I didn't mention the case here. I see sysvec_install() is used
>> only in the guest part (KVM, HYPERV) currently. For example, the KVM
>> guest will register the HYPERVISOR_CALLBACK_VECTOR APF handler in
>> kvm_guest_init(), which is called before trap_init(). So if only the FRED
>> handler is registered and FRED is disabled in trap_init() later, then the
>> IDT handler of the APF handler is missing.
>
>This is a bug!  Your fix looks good to me.
>
>>>> Another problem is that the page fault handler (exc_page_fault()) is
>>>> installed into the IDT before FRED is enabled. Consequently, if a #PF is
>>>> triggered in this gap, the handler would receive the wrong CR2 from the
>>>> stack if FRED feature is present. To address this, I added a page fault
>>>> entry stub for FRED similar to the debug entry. However, I'm uncertain
>>>> whether this is enough reason to add a new entry. Perhaps a static key
>>>> may suffice to indicate whether FRED setup is completed and the handler
>>>> can use it.
>>> 
>>> How could a #PF get triggered during that gap?
>>> 
>>> Initialization time funnies are really unpleasant.
>>> 
>> I'm not sure if there will be a #PF during that gap; I just received the
>> wrong fault address when I made a mistake in that gap and a #PF
>> occurred. Before idt_setup_early_pf(), the registered page fault handler
>> is do_early_exception(), which uses native_read_cr2(). However, after
>> that, the page fault handler had been changed to exc_page_fault(), so if
>> something bad happened and an unexpected #PF occurred, the fault address
>> in the error output will be wrong, although the CR2 in __show_regs() is
>> correct. I'm not sure if this matters or not since the kernel will panic
>> at that time.
>
>So this doesn't sound a real problem, right?
>
>We could simply do:
>
>diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>index e6c469b323cc..e500777ed2b4 100644
>--- a/arch/x86/mm/fault.c
>+++ b/arch/x86/mm/fault.c
>@@ -1495,7 +1495,7 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
>           irqentry_state_t state;
>           unsigned long address;
>
>-       address = cpu_feature_enabled(X86_FEATURE_FRED) ? fred_event_data(regs) : read_cr2();
>+       address = native_read_cr4() & X86_CR4_FRED ? fred_event_data(regs) : read_cr2();
>
>           prefetchw(&current->mm->mmap_lock);
>
>
>But the page fault handler is an extreme hot path, it's not worth it.
>
>Thanks!
>       Xin
>
>
>
>

Reading CR4 is insane from a performance perspective. Also, there is pretty much never a reason to, since CR4 is programmed by the OS.

But this is basically insane. We should enable FRED from the point we cut over from the early exception vector. That is: 

Early IDT → Final IDT
or
Early IDT → FRED

But not

Early IDT → Final IDT → FRED

Eventually we should enable FRED for early exceptions too (which ought to be quite trivial, but makes the whole CLI enable/disable a bit of a mess.)

Re: [PATCH 0/2] x86/fred: Fix two problems during the FRED initialization

[Xin Li]

On 6/25/2024 10:24 AM, H. Peter Anvin wrote:
> On June 25, 2024 2:09:29 AM PDT, Xin Li <xin@zytor.com> wrote:
>> On 6/23/2024 11:21 PM, Hou Wenlong wrote:
>>> I'm not sure if there will be a #PF during that gap; I just received the
>>> wrong fault address when I made a mistake in that gap and a #PF
>>> occurred. Before idt_setup_early_pf(), the registered page fault handler
>>> is do_early_exception(), which uses native_read_cr2(). However, after
>>> that, the page fault handler had been changed to exc_page_fault(), so if
>>> something bad happened and an unexpected #PF occurred, the fault address
>>> in the error output will be wrong, although the CR2 in __show_regs() is
>>> correct. I'm not sure if this matters or not since the kernel will panic
>>> at that time.
>>
>> So this doesn't sound a real problem, right?
>>
>> We could simply do:
>>
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index e6c469b323cc..e500777ed2b4 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -1495,7 +1495,7 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
>>            irqentry_state_t state;
>>            unsigned long address;
>>
>> -       address = cpu_feature_enabled(X86_FEATURE_FRED) ? fred_event_data(regs) : read_cr2();
>> +       address = native_read_cr4() & X86_CR4_FRED ? fred_event_data(regs) : read_cr2();
>>
>>            prefetchw(&current->mm->mmap_lock);
>>
>>
>> But the page fault handler is an extreme hot path, it's not worth it.
>>
>> Thanks!
>>        Xin


> Reading CR4 is insane from a performance perspective. Also, there is pretty much never a reason to, since CR4 is programmed by the OS.

Agreed!

> 
> But this is basically insane. We should enable FRED from the point we cut over from the early exception vector. That is:
> 
> Early IDT → Final IDT
> or
> Early IDT → FRED
> 
> But not
> 
> Early IDT → Final IDT → FRED
> 
> Eventually we should enable FRED for early exceptions too (which ought to be quite trivial, but makes the whole CLI enable/disable a bit of a mess.)
> 

I think you and tglx are talking the same thing :)

Thanks!
     Xin