Re: [PATCH] x86/traps: Re-enable IRQs after reading cr2 in the #PF handler

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Thu Sep 12, 2024 at 10:49 AM BST, Andrew Cooper wrote:
> On 11/09/2024 3:58 pm, Alejandro Vallejo wrote:
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index b8482de8ee..ef803f6288 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -844,8 +844,7 @@ handle_exception_saved:
> >  #elif !defined(CONFIG_PV)
> >          ASSERT_CONTEXT_IS_XEN
> >  #endif /* CONFIG_PV */
> > -        sti
> > -1:      movq  %rsp,%rdi
> > +1:      mov   %rsp,%rdi
> >          movzbl UREGS_entry_vector(%rsp),%eax
> >  #ifdef CONFIG_PERF_COUNTERS
> >          lea   per_cpu__perfcounters(%rip), %rcx
>
> I'm afraid this isn't correctly.  The STI is only on one of two paths to
> the dispatch logic.
>
> Right now, you're re-enabling interrupts even if #PF hits an irqs-off
> region in Xen.
>
> You must not enabled IRQs if going via the exception_with_ints_disabled
> path, which is the user of that 1: label immediately after STI.
>
> ~Andrew

Well, darn. That's a well-hidden Waldo.

I'll send a v2 with conditional enables on C and assembly, and a change of that
label from "1" to ".Lfoo" to clearly imply the control flow might take a
backflip from several miles down the file.

Cheers,
Alejandro