| Message ID | 20220805083744.78767-2-likexu@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] KVM: selftests: Test all possible "invalid" PERF_CAPABILITIES.LBR_FMT vals | expand |
On Fri, Aug 05, 2022, Like Xu wrote: > From: Like Xu <likexu@tencent.com> > > KVM may do the "wrong" thing if userspace changes PERF_CAPABILITIES > after running the vCPU, i.e. after KVM_RUN. Similar to disallowing CPUID > changes after KVM_RUN, KVM should also disallow changing the feature MSRs > (conservatively starting from PERF_CAPABILITIES) after KVM_RUN to prevent > unexpected behavior. > > Applying the same logic to most feature msrs in do_set_msr() may > reduce the flexibility (one odd but reasonable user space may want > per-vcpu control, feature by feature) I don't follow this argument. vcpu->arch.last_vmentry_cpu is per-vCPU, nothing prevents userspace from defining > and also increases the overhead. Yes, there is a small amount of overhead, but it's trivial compared to the massive switch statements in {svm,vmx}_set_msr(). And there are multiple ways to make the overhead practically meaningless. Ha! Modern compilers are fantastic. If we special case the VMX MSRs, which isn't a terrible idea anyways (it's a good excuse to add proper defines instead of open coding "MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC" everywhere), then the lookup can use a compile-time constant array and generate what is effectively a switch statement without actually needed a switch statemen, and without needed to play macro games are have redundant lists (maintenance burden). E.g. with some prep work, the lookup can be this /* * All feature MSRs except uCode revID, which tracks the currently loaded uCode * patch, are immutable once the vCPU model is defined. Special case the VMX * MSRs to keep the primarly lookup loop short. */ static bool kvm_is_immutable_feature_msr(u32 msr) { int i; if (msr >= KVM_FIRST_VMX_FEATURE_MSR && msr <= KVM_LAST_VMX_FEATURE_MSR) return true; for (i = 0; i < ARRAY_SIZE(msr_based_features_all_except_vmx); i++) { if (msr == msr_based_features_all_except_vmx[i]) return msr != MSR_IA32_UCODE_REV; } return false; } which gcc translates to a series of CMP+JCC/SETcc instructions. 0x00000000000291d0 <+48>: 8d 86 80 fb ff ff lea -0x480(%rsi),%eax 0x00000000000291d6 <+54>: 83 f8 11 cmp $0x11,%eax 0x00000000000291d9 <+57>: 76 65 jbe 0x29240 <do_set_msr+160> 0x00000000000291db <+59>: 81 fe 29 10 01 c0 cmp $0xc0011029,%esi 0x00000000000291e1 <+65>: 74 5d je 0x29240 <do_set_msr+160> 0x00000000000291e3 <+67>: 81 fe 8b 00 00 00 cmp $0x8b,%esi 0x00000000000291e9 <+73>: 0f 94 c0 sete %al 0x00000000000291ec <+76>: 81 fe 0a 01 00 00 cmp $0x10a,%esi 0x00000000000291f2 <+82>: 0f 94 c2 sete %dl 0x00000000000291f5 <+85>: 08 d0 or %dl,%al 0x00000000000291f7 <+87>: 75 3e jne 0x29237 <do_set_msr+151> 0x00000000000291f9 <+89>: 81 fe 45 03 00 00 cmp $0x345,%esi 0x00000000000291ff <+95>: 74 36 je 0x29237 <do_set_msr+151> I'll send a series.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 33560bfa0cac..3fb933bfb3bd 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3540,6 +3540,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) if (!msr_info->host_initiated) return 1; + if (vcpu->arch.last_vmentry_cpu != -1 && + vcpu->arch.perf_capabilities != data) + return 1; if (kvm_get_msr_feature(&msr_ent)) return 1; if (data & ~msr_ent.data)