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
