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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

Still rather more invasive than I was hoping for a quick sanity check
that ought never to fire.

~Andrew