* [PATCH v2 1/3] KVM: selftests: Test all possible "invalid" PERF_CAPABILITIES.LBR_FMT vals @ 2022-08-05 8:37 Like Xu 2022-08-05 8:37 ` [PATCH 2/3] KVM: x86: Reject writes to PERF_CAPABILITIES feature MSR after KVM_RUN Like Xu 2022-08-05 8:37 ` [PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected Like Xu 0 siblings, 2 replies; 5+ messages in thread From: Like Xu @ 2022-08-05 8:37 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] KVM: x86: Reject writes to PERF_CAPABILITIES feature MSR after KVM_RUN 2022-08-05 8:37 [PATCH v2 1/3] KVM: selftests: Test all possible "invalid" PERF_CAPABILITIES.LBR_FMT vals Like Xu @ 2022-08-05 8:37 ` Like Xu 2022-08-05 16:29 ` Sean Christopherson 2022-08-05 8:37 ` [PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected Like Xu 1 sibling, 1 reply; 5+ messages in thread From: Like Xu @ 2022-08-05 8:37 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] KVM: x86: Reject writes to PERF_CAPABILITIES feature MSR after KVM_RUN 2022-08-05 8:37 ` [PATCH 2/3] KVM: x86: Reject writes to PERF_CAPABILITIES feature MSR after KVM_RUN Like Xu @ 2022-08-05 16:29 ` Sean Christopherson 0 siblings, 0 replies; 5+ messages in thread From: Sean Christopherson @ 2022-08-05 16:29 UTC (permalink / raw) To: Like Xu; +Cc: Paolo Bonzini, kvm, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected 2022-08-05 8:37 [PATCH v2 1/3] KVM: selftests: Test all possible "invalid" PERF_CAPABILITIES.LBR_FMT vals Like Xu 2022-08-05 8:37 ` [PATCH 2/3] KVM: x86: Reject writes to PERF_CAPABILITIES feature MSR after KVM_RUN Like Xu @ 2022-08-05 8:37 ` Like Xu 2022-08-05 16:32 ` Sean Christopherson 1 sibling, 1 reply; 5+ messages in thread From: Like Xu @ 2022-08-05 8:37 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected 2022-08-05 8:37 ` [PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected Like Xu @ 2022-08-05 16:32 ` Sean Christopherson 0 siblings, 0 replies; 5+ messages in thread From: Sean Christopherson @ 2022-08-05 16:32 UTC (permalink / raw) To: Like Xu; +Cc: Paolo Bonzini, kvm, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-05 16:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-05 8:37 [PATCH v2 1/3] KVM: selftests: Test all possible "invalid" PERF_CAPABILITIES.LBR_FMT vals Like Xu 2022-08-05 8:37 ` [PATCH 2/3] KVM: x86: Reject writes to PERF_CAPABILITIES feature MSR after KVM_RUN Like Xu 2022-08-05 16:29 ` Sean Christopherson 2022-08-05 8:37 ` [PATCH 3/3] KVM: selftests: Test writing PERF_CAPABILITIES after KVM_RUN is rejected Like Xu 2022-08-05 16:32 ` Sean Christopherson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).