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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2023 22:27, Andrew Cooper wrote:
> --- 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

Without having peeked ahead, is there any use of %r12 going to appear
on this path? I thought it's only going to be restore_all_xen?

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -142,10 +142,16 @@ process_trap:
>  
>          .section .text.entry, "ax", @progbits
>  
> -/* %rbx: struct vcpu, interrupts disabled */
> +/* %rbx: struct vcpu, %r12: ist_exit, interrupts disabled */
>  restore_all_guest:
> -        ASSERT_INTERRUPTS_DISABLED
>  
> +#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. */
>          mov VCPU_arch_msrs(%rbx), %rdx
>          mov VCPUMSR_spec_ctrl_raw(%rdx), %r15d

Even here I don't think I see a need for the addition. Plus if the check
is warranted here, is it really necessary for it to live ahead of the
interrupts-disabled check? Further, seeing that being marco-ized, would
it make sense to have a CHECK_IST_EXIT macro in case more than a single
use site remains?

> @@ -1087,6 +1100,10 @@ handle_ist_exception:
>  .L_ist_dispatch_done:
>          mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
>          mov   %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
> +
> +        /* This is an IST exit */
> +        mov   $1, %r12
> +
>          cmpb  $X86_EXC_NMI, UREGS_entry_vector(%rsp)
>          jne   ret_from_intr

Could I talk you into using a less than 7-byte insn here? "or $1, %r12"
would be 4 bytes, "mov $1, %r12b", "inc %r12", and "not %r12" would be
just 3. All have certain downsides, yes, but I wonder whether switching
isn't worth it. Even "mov $1, %r12d" would be at least one byte less,
without any downsides. And the OR and INC variants would allow the
remaining 63 bits to be used for another purpose down the road.

Jan