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

[Xin Li <xin@zytor.com>]

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