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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? I thought
you want the VMCB field checked (which is what we're going to
have host state loaded from, and which hence is what shouldn't
be wrong). If the assert as you suggests passes, it means
nothing towards our environment after the next VM exit.

> The problem with the other state is that it compiletime/runtime
> dependent, and we don't want to be opencoding the logic a second time.

Right, but the assertion should be useful at least in some way,
which may make it unavoidable to duplicate some of the logic.
In effect the assertion as you're suggesting it does, too, in
that it further implies VMCB.trsel == TSS_SELECTOR.

Jan