Re: [PATCH 2/3] KVM: x86: Reject writes to PERF_CAPABILITIES feature MSR after KVM_RUN

[Sean Christopherson <seanjc@google.com>]

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.