Message ID | a01862b8-6e16-5ddc-7f48-2d3bed2f34b6@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | SVM: avoid VMSAVE in ctxt-switch-to | expand |
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>
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
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
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
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
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
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
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
--- 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);
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>