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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/09/2023 12:02 pm, Jan Beulich wrote:
> 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>

Thanks.

>
> 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?

There's no header that's unique to just the entry.S's, and it's only 3
instructions that need a very specific stack and state layout.

The SPEC_CTRL_* macros are already a giant source of pain, and for 3
instructions I don't think the complexity of the abstraction is worth it.


Furthermore, I've got some fixes to the other ASSERT_* macros which are
going to make them a bit more like this.

~Andrew