Re: [PATCH] SVM: avoid VMSAVE in ctxt-switch-to

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] SVM: avoid VMSAVE in ctxt-switch-to
Date: Tue, 20 Oct 2020 09:00:42 +0200	[thread overview]
Message-ID: <746db5e1-f78f-2446-c877-4abfa4ba8f39@suse.com> (raw)
In-Reply-To: <e8ee8c84-f949-c882-07a6-58079987a308@citrix.com>

On 19.10.2020 17:52, Andrew Cooper wrote:
> On 19/10/2020 16:47, Jan Beulich wrote:
>> On 19.10.2020 17:22, Andrew Cooper wrote:
>>> On 19/10/2020 16:02, Jan Beulich wrote:
>>>> On 19.10.2020 16:30, Andrew Cooper wrote:
>>>>> On 19/10/2020 15:21, Jan Beulich wrote:
>>>>>> On 19.10.2020 16:10, Andrew Cooper wrote:
>>>>>>> On 19/10/2020 14:40, Jan Beulich wrote:
>>>>>>>> Of the state saved by the insn and reloaded by the corresponding VMLOAD
>>>>>>>> - TR, syscall, and sysenter state are invariant while having Xen's state
>>>>>>>>   loaded,
>>>>>>>> - FS, GS, and LDTR are not used by Xen and get suitably set in PV
>>>>>>>>   context switch code.
>>>>>>> I think it would be helpful to state that Xen's state is suitably cached
>>>>>>> in _svm_cpu_up(), as this does now introduce an ordering dependency
>>>>>>> during boot.
>>>>>> I've added a sentence.
>>>>>>
>>>>>>> Is it possibly worth putting an assert checking the TR selector?  That
>>>>>>> ought to be good enough to catch stray init reordering problems.
>>>>>> How would checking just the TR selector help? If other pieces of TR
>>>>>> or syscall/sysenter were out of sync, we'd be hosed, too.
>>>>> They're far less likely to move relative to tr, than everything relative
>>>>> to hvm_up().
>>>>>
>>>>>> I'm also not sure what exactly you mean to do for such an assertion:
>>>>>> Merely check the host VMCB field against TSS_SELECTOR, or do an
>>>>>> actual STR to be closer to what the VMSAVE actually did?
>>>>> ASSERT(str() == TSS_SELECTOR);
>>>> Oh, that's odd. How would this help with the VMCB?
>>> It wont.
>>>
>>> We're not checking the behaviour of the VMSAVE instruction.  We just
>>> want to sanity check that %tr is already configured.
>> TR not already being configured is out of question in a function
>> involved in context switching, don't you think? I thought you're
>> worried about the VMCB not being set up correctly? Or are you in
>> the end asking for the suggested assertion to go into
>> _svm_cpu_up(), yet I didn't understand it that way?
> 
> I meant in _svm_cpu_up().  It is only at at __init time where the new
> implicit dependency is created.

Okay, so just a misunderstanding on my part. I've done as you've
suggested, but I'd like to note that load_system_tables() runs
(often far) earlier than percpu_traps_init(), and hence ordering
issues with the latter are more likely. In fact the most worrying
case is its use by reinit_bsp_stack(), which is not a problem
only because the only relevant stack relative adjustment done is
the writing of SYSENTER_ESP, which gets skipped for AMD.

Jan