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@xxxxxxxxx> 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