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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 6/8] x86/entry: Track the IST-ness of an entry for the exit paths
Date: Thu, 14 Sep 2023 20:44:06 +0100	[thread overview]
Message-ID: <fb9ec99d-9c76-82ff-9b82-22fff1a62c6c@citrix.com> (raw)
In-Reply-To: <762b58fe-a841-f92c-93a8-77dbb86bf118@suse.com>

On 14/09/2023 10:32 am, Jan Beulich wrote:
> 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?

For now, we only need to change behaviour based on ist_exit in
restore_all_xen.

But, we do get here in IST context, and I'm not interested in having to
re-do the analysis to determine if this is safe.  ist_exit is a global
property of exiting Xen, so should be kept correct from the outset.

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

What makes you think there is a relevance to the order of two assertions
in fully irqs-off code?

The checks are in the same order as the comment stating the invariants.

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

This is a 2Hz-at-most path.  The size of one instruction is not
something to care about.

But I did mean to use the %r12d form, so I'll go with that.  Everything
else depends on the behaviour of earlier logic.

~Andrew