> On Dec 10, 2020, at 4:48 PM, Michael Roth <michael.roth@xxxxxxx> wrote: > >> I think there are two reasonable ways to do this: >> >> 1. VMLOAD before STGI. This is obviously correct, and it's quite simple. > > For the testing I ended up putting it immediately after __svm_vcpu_run() > since that's where we restored GS base previously. Does that seem okay or did > you have another place in mind? Looks okay. If we get an NMI or MCE with the wrong MSR_GS_BASE, then we are toast, at least on Zen 2 and earlier. But that spot has GI == 0, so this won't happen. > >> >> 2. Save cpu_kernelmode_gs_base(cpu) before VM entry, and restore that >> value to MSR_GS_BASE using code like this (or its asm equivalent) >> before STGI: >> >> if (static_cpu_has(X86_FEATURE_FSGSBASE)) >> wrgsbase(base); >> else >> wrmsr... >> >> and then VMLOAD in the vcpu_put() path. >> >> I can't think of any reason to use loadsegment(), load_gs_index(), or >> savesegment() at all, nor can I think of any reason to touch >> MSR_KERNEL_GS_BASE or MSR_FS_BASE. > > I'm sort of lumping MSR_GS_BASE restoration in with everything else since I > don't fully understand what the original was code doing either and was content > to leave it be if we couldn't use VMLOAD to handle it without a performance > regression, but since it looks like we can use VMLOAD here instead I agree > we should just drop it all. The original code is entirely bogus. Don’t try to hard to understand how it’s correct — I’m pretty sure it’s not. In fact, I was planning to write a patch a lot like yours, but I don’t have an SVM-capable CPU to test on. In general, I suspect that you could delete all these fields from the structs, see what fails to compile, and fix it pretty easily. MSR_GS_BASE is kernel state. (On 32-bit, fs and maybe gs are kernel state.). Everything else is host *user* state and isn’t touched by normal kernel code.
