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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> This version is far more simple than checking VMCB.trsel, which will
> require a map_domain_page().

In the general case yes, but in the most common case (PV also
enabled) we have a mapping already (host_vmcb_va).

Jan