[PATCH v2 1/3] KVM: selftests: Test all possible "invalid" PERF_CAPABILITIES.LBR_FMT vals

[PATCH v2 1/3] KVM: selftests: Test all possible "invalid" PERF_CAPABILITIES.LBR_FMT vals

[Like Xu]

From: Sean Christopherson <seanjc@google.com>

Test all possible input values to verify that KVM rejects all values
except the exact host value.  Due to the LBR format affecting the core
functionality of LBRs, KVM can't emulate "other" formats, so even though
there are a variety of legal values, KVM should reject anything but an
exact host match.

Suggested-by: Like Xu <like.xu.linux@gmail.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
v1: https://lore.kernel.org/kvm/20220804073819.76460-1-likexu@tencent.com/
v1 -> v2 Changelog:
- Replace testcase #3 with fully exhaustive approach; (Sean)

 .../selftests/kvm/x86_64/vmx_pmu_caps_test.c    | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
index 6ec901dab61e..928c10e520c7 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
@@ -59,6 +59,7 @@ int main(int argc, char *argv[])
 	int ret;
 	union cpuid10_eax eax;
 	union perf_capabilities host_cap;
+	uint64_t val;
 
 	host_cap.capabilities = kvm_get_feature_msr(MSR_IA32_PERF_CAPABILITIES);
 	host_cap.capabilities &= (PMU_CAP_FW_WRITES | PMU_CAP_LBR_FMT);
@@ -91,11 +92,17 @@ int main(int argc, char *argv[])
 	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.lbr_format);
 	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), (u64)host_cap.lbr_format);
 
-	/* testcase 3, check invalid LBR format is rejected */
-	/* Note, on Arch LBR capable platforms, LBR_FMT in perf capability msr is 0x3f,
-	 * to avoid the failure, use a true invalid format 0x30 for the test. */
-	ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, 0x30);
-	TEST_ASSERT(ret == 0, "Bad PERF_CAPABILITIES didn't fail.");
+	/*
+	 * Testcase 3, check that an "invalid" LBR format is rejected.  Only an
+	 * exact match of the host's format (and 0/disabled) is allowed.
+	 */
+	for (val = 1; val <= PMU_CAP_LBR_FMT; val++) {
+		if (val == host_cap.lbr_format)
+			continue;
+
+		ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val);
+		TEST_ASSERT(!ret, "Bad LBR FMT = 0x%lx didn't fail", val);
+	}
 
 	printf("Completed perf capability tests.\n");
 	kvm_vm_free(vm);
-- 
2.37.1

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

[Like Xu]

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) and also increases the overhead.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/x86.c | 3 +++
 1 file changed, 3 insertions(+)

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)
-- 
2.37.1

[PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected

[Like Xu]

From: Like Xu <likexu@tencent.com>

KVM should also disallow changing the feature MSR PERF_CAPABILITIES after
KVM_RUN to prevent unexpected behavior. Implement run_vcpu() in a separate
thread approach and opportunistically rearrange test cases.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 .../selftests/kvm/x86_64/vmx_pmu_caps_test.c  | 49 +++++++++++++------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
index 928c10e520c7..0ee00fec8c2c 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_caps_test.c
@@ -13,13 +13,13 @@
 
 #define _GNU_SOURCE /* for program_invocation_short_name */
 #include <sys/ioctl.h>
+#include <pthread.h>
 
 #include "kvm_util.h"
 #include "vmx.h"
 
 #define PMU_CAP_FW_WRITES	(1ULL << 13)
 #define PMU_CAP_LBR_FMT		0x3f
-
 union cpuid10_eax {
 	struct {
 		unsigned int version_id:8;
@@ -46,17 +46,28 @@ union perf_capabilities {
 	u64	capabilities;
 };
 
+static struct kvm_vm *vm;
+static struct kvm_vcpu *vcpu;
+
 static void guest_code(void)
 {
 	wrmsr(MSR_IA32_PERF_CAPABILITIES, PMU_CAP_LBR_FMT);
 }
 
+static void *run_vcpu(void *ignore)
+{
+	vcpu_run(vcpu);
+
+	TEST_ASSERT(!_vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, 0),
+		    "Update PERF_CAPABILITIES after VCPU_RUN didn't fail.");
+
+	return NULL;
+}
+
 int main(int argc, char *argv[])
 {
+	pthread_t cpu_thread;
 	const struct kvm_cpuid_entry2 *entry_a_0;
-	struct kvm_vm *vm;
-	struct kvm_vcpu *vcpu;
-	int ret;
 	union cpuid10_eax eax;
 	union perf_capabilities host_cap;
 	uint64_t val;
@@ -65,7 +76,8 @@ int main(int argc, char *argv[])
 	host_cap.capabilities &= (PMU_CAP_FW_WRITES | PMU_CAP_LBR_FMT);
 
 	/* Create VM */
-	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	vm = vm_create(1);
+	vcpu = vm_vcpu_add(vm, 1, guest_code);
 
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_PDCM));
 
@@ -77,33 +89,40 @@ int main(int argc, char *argv[])
 
 	/* testcase 1, set capabilities when we have PDCM bit */
 	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, PMU_CAP_FW_WRITES);
-
-	/* check capabilities can be retrieved with KVM_GET_MSR */
 	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), PMU_CAP_FW_WRITES);
 
-	/* check whatever we write with KVM_SET_MSR is _not_ modified */
-	vcpu_run(vcpu);
-	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), PMU_CAP_FW_WRITES);
-
-	/* testcase 2, check valid LBR formats are accepted */
+	/* testcase 2, check value zero (which disables all features) is accepted */
 	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, 0);
 	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), 0);
 
+	/* testcase 3, check valid LBR formats are accepted */
 	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.lbr_format);
 	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), (u64)host_cap.lbr_format);
 
 	/*
-	 * Testcase 3, check that an "invalid" LBR format is rejected.  Only an
+	 * Testcase 4, check that an "invalid" LBR format is rejected.  Only an
 	 * exact match of the host's format (and 0/disabled) is allowed.
 	 */
 	for (val = 1; val <= PMU_CAP_LBR_FMT; val++) {
 		if (val == host_cap.lbr_format)
 			continue;
 
-		ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val);
-		TEST_ASSERT(!ret, "Bad LBR FMT = 0x%lx didn't fail", val);
+		TEST_ASSERT(!_vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val),
+			    "Bad LBR FMT = 0x%lx didn't fail", val);
 	}
 
+	/* Testcase 5, check whatever use space writes is _not_ modified after VCPU_RUN */
+	vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities);
+
+	pthread_create(&cpu_thread, NULL, run_vcpu, NULL);
+	pthread_join(cpu_thread, NULL);
+
+	TEST_ASSERT(!_vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, 0),
+		    "Update PERF_CAPABILITIES after VCPU_RUN didn't fail.");
+
+	ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), host_cap.capabilities);
+
 	printf("Completed perf capability tests.\n");
 	kvm_vm_free(vm);
+	return 0;
 }
-- 
2.37.1

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

[Sean Christopherson]

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.

Re: [PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected

[Sean Christopherson]

On Fri, Aug 05, 2022, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> KVM should also disallow changing the feature MSR PERF_CAPABILITIES after
> KVM_RUN to prevent unexpected behavior. Implement run_vcpu() in a separate
> thread approach and opportunistically rearrange test cases.
> 
> Signed-off-by: Like Xu <likexu@tencent.com>
> ---
>  .../selftests/kvm/x86_64/vmx_pmu_caps_test.c  | 49 +++++++++++++------

Rather than add a one-off PERF_CAPABILITIES test, I would rather we rename and
extend get_msr_index_features, e.g. call it feature_msrs_test and then have it
test all feature MSRs.