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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.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: Fri, 15 Sep 2023 09:13:24 +0200	[thread overview]
Message-ID: <b6640dbb-05e5-7114-dc70-012c27dd96e7@suse.com> (raw)
In-Reply-To: <fb9ec99d-9c76-82ff-9b82-22fff1a62c6c@citrix.com>

On 14.09.2023 21:44, Andrew Cooper wrote:
> 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.

Would be nice to mention this just-in-case aspect in the description.

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

You explicitly making it more churn than strictly needed. IOW I was
simply wondering whether I was overlooking some aspect.

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

If that's the only criteria, then okay (but still slightly odd to
see more churn than necessary).

Jan