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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 18, 2024 at 02:05:54PM +0100, Alejandro Vallejo wrote:
> Moves sti directly after the cr2 read and immediately after the #PF
> handler.

I think you need to add some context about why this is needed, iow:
avoid corrupting %cr2 if a nested 3PF happens.

> While in the area, remove redundant q suffix to a movq in entry.S
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

One nit below.

> ---
> Got lost alongside other patches. Here's the promised v2.
> 
> pipeline: 
> https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/1458699639
> v1: 
> https://lore.kernel.org/xen-devel/20240911145823.12066-1-alejandro.vallejo@xxxxxxxxx/
> 
> v2:
>   * (cosmetic), add whitespace after comma
>   * Added ASSERT(local_irq_is_enabled()) to do_page_fault()
>   * Only re-enable interrupts if they were enabled in the interrupted
>     context.
> ---
>  xen/arch/x86/traps.c        |  8 ++++++++
>  xen/arch/x86/x86_64/entry.S | 20 ++++++++++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 708136f62558..a9c2c607eb08 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1600,6 +1600,14 @@ void asmlinkage do_page_fault(struct cpu_user_regs 
> *regs)
>  
>      addr = read_cr2();
>  
> +    /*
> +     * Don't re-enable interrupts if we were running an IRQ-off region when
> +     * we hit the page fault, or we'll break that code.
> +     */
> +    ASSERT(!local_irq_is_enabled());
> +    if ( regs->flags & X86_EFLAGS_IF )
> +        local_irq_enable();
> +
>      /* fixup_page_fault() might change regs->error_code, so cache it here. */
>      error_code = regs->error_code;
>  
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index b8482de8ee5b..218e5ea85efb 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -844,9 +844,9 @@ handle_exception_saved:
>  #elif !defined(CONFIG_PV)
>          ASSERT_CONTEXT_IS_XEN
>  #endif /* CONFIG_PV */
> -        sti
> -1:      movq  %rsp,%rdi
> -        movzbl UREGS_entry_vector(%rsp),%eax
> +.Ldispatch_handlers:

Maybe 'dispatch_exception', since it's only exceptions that are
handled here? dispatch_handlers seems a bit too generic, but no strong
opinion.

Thanks, Roger.