On Sun, Jul 05, 2020 at 12:40:25PM +0300, Maxim Levitsky wrote: > > Rather than compute the mask every time, it can be computed once on module > > load and stashed in a global. Note, there's a RFC series[*] to support > > reprobing bugs at runtime, but that has bigger issues with existing KVM > > functionality to be addressed, i.e. it's not our problem, yet :-). > > > > [*] https://lkml.kernel.org/r/1593703107-8852-1-git-send-email-mihai.carabas@xxxxxxxxxx > > Thanks for the pointer! > > Note though that the above code only runs once, since after a single > successful (non #GP) set of it to non-zero value, it is cleared in MSR bitmap > for both reads and writes on both VMX and SVM. For me the performance is secondary to documenting the fact that the host valid bits are fixed for a given instance of the kernel. There's enough going on in kvm_spec_ctrl_valid_bits_host() that's it's not super easy to see that it's a "constant" value. > This is done because of performance reasons which in this case are more > important than absolute correctness. Thus to some extent the guest checks in > the above are pointless. > > If you ask me, I would just remove the kvm_spec_ctrl_valid_bits, and pass > this msr to guest right away and not on first access. That would unnecessarily penalize guests that don't utilize the MSR as KVM would need to do a RDMSR on every VM-Exit to grab the guest's value. One oddity with this whole thing is that by passing through the MSR, KVM is allowing the guest to write bits it doesn't know about, which is definitely not normal. It also means the guest could write bits that the host VMM can't. Somehwat crazy idea inbound... rather than calculating the valid bits in software, what if we throw the value at the CPU and see if it fails? At least that way the host and guest are subject to the same rules. E.g. --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2062,11 +2062,19 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) return 1; - if (data & ~kvm_spec_ctrl_valid_bits(vcpu)) - return 1; - + ret = 0; vmx->spec_ctrl = data; - if (!data) + + local_irq_disable(); + if (rdmsrl_safe(MSR_IA32_SPEC_CTRL, &data)) + ret = 1; + else if (wrmsrl_safe(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl)) + ret = 1; + else + wrmsrl(MSR_IA32_SPEC_CTRL, data)) + local_irq_enable(); + + if (ret || !vmx->spec_ctrl) break; /*
