On 29/01/2018 11:37, David Woodhouse wrote: > On Mon, 2018-01-29 at 10:43 +0100, KarimAllah Ahmed wrote: >> On 01/29/2018 09:46 AM, David Woodhouse wrote: >>> Reading the code and comparing with the SDM, I can't see where we're >>> ever setting VM_EXIT_MSR_STORE_{ADDR,COUNT} except in the nested >>> case... >> Hmmm ... you are probably right! I think all users of this interface >> always trap + update save area and never passthrough the MSR. That is >> why only LOAD is needed *so far*. >> >> Okay, let me sort this out in v3 then. > > I'm starting to think a variant of Ashok's patch might actually be the > simpler approach, and not "premature optimisation". Especially if we > need to support the !cpu_has_vmx_msr_bitmaps() case? That case is awfully slow anyway, it doesn't matter, but the direct-access flag would simply be always 0 if you have no MSR bitmaps. > Start with vmx->spec_ctrl set to zero. When first touched, make it > passthrough (but not atomically switched) and set a flag (e.g. > "spec_ctrl_live") which triggers the 'restore_branch_speculation' and > 'save_and_restrict_branch_speculation' behaviours. Except don't use > those macros. Those can look something like > > /* If this vCPU has touched SPEC_CTRL then restore its value if needed */ > if (vmx->spec_ctrl_live && vmx->spec_ctrl) > wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl); > /* vmentry is serialising on affected CPUs, so the conditional branch is safe */ > > > ... and, respectively, ... > > /* If this vCPU has touched SPEC_CTRL then save its value and ensure we have zero */ > if (vmx->spec_ctrl_live) { > rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl); > if (vmx->spec_ctrl) > wrmsrl(MSR_IA32_SPEC_CTRL, 0); > } > > Perhaps we can ditch the separate 'spec_ctrl_live' flag and check the > pass-through MSR bitmap directly, in the case that it exists? Probably a cache miss, or even a TLB miss if you're unlucky, so the separate flag is okay. Paolo