Mailing List Archive: [PATCH] SVM: avoid VMSAVE in ctxt-switch-to

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

Oct 19, 2020, 6:40 AM

Post #1 of 9 (255 views)

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.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -984,7 +984,6 @@ static void svm_ctxt_switch_to(struct vc

svm_restore_dr(v);

- svm_vmsave_pa(per_cpu(host_vmcb, cpu));
vmcb->cleanbits.raw = 0;
svm_tsc_ratio_load(v);

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

andrew.cooper3 at citrix

Oct 19, 2020, 7:10 AM

Post #2 of 9 (255 views)

Permalink

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.

Is it possibly worth putting an assert checking the TR selector? That
ought to be good enough to catch stray init reordering problems.

> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Either way, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

jbeulich at suse

Oct 19, 2020, 7:21 AM

Post #3 of 9 (255 views)

Permalink

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.

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?

>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Either way, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan

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

andrew.cooper3 at citrix

Oct 19, 2020, 7:30 AM

Post #4 of 9 (255 views)

Permalink

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

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.

~Andrew

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

jbeulich at suse

Oct 19, 2020, 8:02 AM

Post #5 of 9 (255 views)

Permalink

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

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

andrew.cooper3 at citrix

Oct 19, 2020, 8:22 AM

Post #6 of 9 (255 views)

Permalink

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.

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

~Andrew

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

jbeulich at suse

Oct 19, 2020, 8:47 AM

Post #7 of 9 (255 views)

Permalink

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

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

andrew.cooper3 at citrix

Oct 19, 2020, 8:52 AM

Post #8 of 9 (255 views)

Permalink

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

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

jbeulich at suse

Oct 20, 2020, 12:00 AM

Post #9 of 9 (255 views)

Permalink

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