Re: [PATCH 6/9] x86/entry: Track the IST-ness of an entry for the exit paths

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.09.2023 17:00, Andrew Cooper wrote:
> Use %r12 to hold an ist_exit boolean.  This register is zero elsewhere in the
> entry/exit asm, so it only needs setting in the IST path.
> 
> As this is subtle and fragile, add check_ist_exit() to be used in debugging
> builds to cross-check that the ist_exit boolean matches the entry vector.
> 
> Write check_ist_exit() it in C, because it's debug only and the logic more
> complicated than I care to maintain in asm.
> 
> For now, we only need to use this signal in the exit-to-Xen path, but some
> exit-to-guest paths happen in IST context too.  Check the correctness in all
> exit paths to avoid the logic bitrotting.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

I understand you then didn't like the idea of macro-izing ...

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -117,8 +117,15 @@ compat_process_trap:
>          call  compat_create_bounce_frame
>          jmp   compat_test_all_events
>  
> -/* %rbx: struct vcpu, interrupts disabled */
> +/* %rbx: struct vcpu, %r12: ist_exit, interrupts disabled */
>  ENTRY(compat_restore_all_guest)
> +
> +#ifdef CONFIG_DEBUG
> +        mov   %rsp, %rdi
> +        mov   %r12, %rsi
> +        call  check_ist_exit
> +#endif
> +
>          ASSERT_INTERRUPTS_DISABLED
>          mov   $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), %r11d
>          and   UREGS_eflags(%rsp),%r11d
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -142,8 +142,15 @@ process_trap:
>  
>          .section .text.entry, "ax", @progbits
>  
> -/* %rbx: struct vcpu, interrupts disabled */
> +/* %rbx: struct vcpu, %r12: ist_exit, interrupts disabled */
>  restore_all_guest:
> +
> +#ifdef CONFIG_DEBUG
> +        mov   %rsp, %rdi
> +        mov   %r12, %rsi
> +        call  check_ist_exit
> +#endif
> +
>          ASSERT_INTERRUPTS_DISABLED
>  
>          /* Stash guest SPEC_CTRL value while we can read struct vcpu. */
> @@ -659,8 +666,15 @@ ENTRY(early_page_fault)
>          .section .text.entry, "ax", @progbits
>  
>          ALIGN
> -/* No special register assumptions. */
> +/* %r12=ist_exit */
>  restore_all_xen:
> +
> +#ifdef CONFIG_DEBUG
> +        mov   %rsp, %rdi
> +        mov   %r12, %rsi
> +        call  check_ist_exit
> +#endif

... these three instances of identical code you add, along the lines of
ASSERT_INTERRUPTS_DISABLED?

Jan