* [PATCH 0/2] KVM: selftests: Add tests for PEBS and MSR_IA32_DS_AREA @ 2023-06-08 11:34 Jinrong Liang 2023-06-08 11:34 ` [PATCH 1/2] KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA Jinrong Liang 2023-06-08 11:34 ` [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES Jinrong Liang 0 siblings, 2 replies; 6+ messages in thread From: Jinrong Liang @ 2023-06-08 11:34 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Wanpeng Li, Like Xu, Jinrong Liang, linux-kselftest, kvm, linux-kernel Hi, This patch series introduces two tests to further enhance and verify the functionality of the KVM subsystem. These tests focus on MSR_IA32_DS_AREA and MSR_IA32_PERF_CAPABILITIES. The first patch adds tests to verify the correct behavior when trying to set MSR_IA32_DS_AREA with a non-classical address. It checks that KVM is correctly faulting these non-classical addresses, ensuring the accuracy and stability of the KVM subsystem. The second patch includes a comprehensive PEBS test that checks all possible combinations of PEBS-related bits in MSR_IA32_PERF_CAPABILITIES. This helps to ensure the accuracy of PEBS functionality. Feedback and suggestions are welcomed and appreciated. Sincerely, Jinrong Liang Jinrong Liang (2): KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES .../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 171 ++++++++++++++++++ 1 file changed, 171 insertions(+) base-commit: 31b4fc3bc64aadd660c5bfa5178c86a7ba61e0f7 -- 2.31.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA 2023-06-08 11:34 [PATCH 0/2] KVM: selftests: Add tests for PEBS and MSR_IA32_DS_AREA Jinrong Liang @ 2023-06-08 11:34 ` Jinrong Liang 2023-06-28 21:48 ` Sean Christopherson 2023-06-08 11:34 ` [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES Jinrong Liang 1 sibling, 1 reply; 6+ messages in thread From: Jinrong Liang @ 2023-06-08 11:34 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Wanpeng Li, Like Xu, Jinrong Liang, linux-kselftest, kvm, linux-kernel From: Jinrong Liang <cloudliang@tencent.com> Tests have been added to this commit to check if setting MSR_IA32_DS_AREA with a non-classical address causes a fault. By verifying that KVM is correctly faulting non-classical addresses in MSR_IA32_DS_AREA, it helps ensure the accuracy and stability of the KVM subsystem. Signed-off-by: Jinrong Liang <cloudliang@tencent.com> --- .../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) 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 4c90f76930f9..02903084598f 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 @@ -19,6 +19,9 @@ #include "kvm_util.h" #include "vmx.h" +#define MAX_LINEAR_ADDR_MASK GENMASK_ULL(15, 8) +#define ADDR_OFS_BIT 8 + union perf_capabilities { struct { u64 lbr_format:6; @@ -232,6 +235,102 @@ static void test_lbr_perf_capabilities(union perf_capabilities host_cap) kvm_vm_free(vm); } +/* + * Generate a non-canonical address for a given number of address bits. + * @addr_bits: The number of address bits used in the system. + * + * This function calculates a non-canonical address by setting the most + * significant bit to 1 and adding an offset equal to the maximum value + * that can be represented by the remaining bits. This ensures that the + * generated address is outside the valid address range and is consistent. + */ +static inline uint64_t non_canonical_address(unsigned int addr_bits) +{ + return (1ULL << (addr_bits - 1)) + ((1ULL << (addr_bits - 1)) - 1); +} + +static uint64_t get_addr_bits(struct kvm_vcpu *vcpu) +{ + const struct kvm_cpuid_entry2 *kvm_entry; + unsigned int addr_bits; + struct kvm_sregs sregs; + + kvm_entry = get_cpuid_entry(kvm_get_supported_cpuid(), 0x80000008, 0); + addr_bits = (kvm_entry->eax & MAX_LINEAR_ADDR_MASK) >> ADDR_OFS_BIT; + /* + * Get the size of the virtual address space by checking the LA57 bit + * in the CR4 control register. If the LA57 bit is set, then the virtual + * address space is 57 bits. Otherwise, it's 48 bits. + */ + if (addr_bits != 32) { + vcpu_sregs_get(vcpu, &sregs); + addr_bits = (sregs.cr4 & X86_CR4_LA57) ? 57 : 48; + } + + return addr_bits; +} + +static void test_ds_guest_code(uint64_t bad_addr) +{ + uint8_t vector = 0; + + vector = wrmsr_safe(MSR_IA32_DS_AREA, bad_addr); + GUEST_SYNC(vector); +} + +/* Check if setting MSR_IA32_DS_AREA in guest and kvm userspace will fail. */ +static void test_ds_area_noncanonical_address(union perf_capabilities host_cap) +{ + struct kvm_vm *vm; + struct kvm_vcpu *vcpu; + unsigned int r, addr_bits; + uint64_t bad_addr, without_pebs_fmt_caps; + struct ucall uc; + + vm = vm_create_with_one_vcpu(&vcpu, test_ds_guest_code); + vm_init_descriptor_tables(vm); + vcpu_init_descriptor_tables(vcpu); + + /* Check that Setting MSR_IA32_DS_AREA with 0 should succeed. */ + r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, 0); + TEST_ASSERT(r, "Setting MSR_IA32_DS_AREA with 0 should succeed."); + + /* + * Check that if PEBS_FMT is not set setting MSR_IA32_DS_AREA will + * succeed. + */ + without_pebs_fmt_caps = host_cap.capabilities & ~PERF_CAP_PEBS_FORMAT; + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, without_pebs_fmt_caps); + r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, 1); + TEST_ASSERT(r, "Setting MSR_IA32_DS_AREA with bad addr should fail."); + + /* + * Check that setting MSR_IA32_DS_AREA in kvm userspace to use a + * non-canonical address should fail. + */ + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities); + addr_bits = get_addr_bits(vcpu); + bad_addr = non_canonical_address(addr_bits); + r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, bad_addr); + TEST_ASSERT(!r, "Setting MSR_IA32_DS_AREA with bad addr should fail."); + + /* + * Check that setting MSR_IA32_DS_AREA in guest to use a non-canonical + * address should cause the #GP. + */ + vcpu_args_set(vcpu, 1, bad_addr); + vcpu_run(vcpu); + + TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO); + get_ucall(vcpu, &uc); + TEST_ASSERT(uc.cmd == UCALL_SYNC, + "Received ucall other than UCALL_SYNC: %lu", uc.cmd); + TEST_ASSERT(uc.args[1] == GP_VECTOR, + "Setting MSR_IA32_DS_AREA with bad addr should fail."); + + kvm_vm_free(vm); +} + int main(int argc, char *argv[]) { union perf_capabilities host_cap; @@ -252,4 +351,5 @@ int main(int argc, char *argv[]) test_immutable_perf_capabilities(host_cap); test_guest_wrmsr_perf_capabilities(host_cap); test_lbr_perf_capabilities(host_cap); + test_ds_area_noncanonical_address(host_cap); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA 2023-06-08 11:34 ` [PATCH 1/2] KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA Jinrong Liang @ 2023-06-28 21:48 ` Sean Christopherson 0 siblings, 0 replies; 6+ messages in thread From: Sean Christopherson @ 2023-06-28 21:48 UTC (permalink / raw) To: Jinrong Liang Cc: Paolo Bonzini, Wanpeng Li, Like Xu, Jinrong Liang, linux-kselftest, kvm, linux-kernel On Thu, Jun 08, 2023, Jinrong Liang wrote: > From: Jinrong Liang <cloudliang@tencent.com> > > Tests have been added to this commit to check if setting > MSR_IA32_DS_AREA with a non-classical address causes a fault. By > verifying that KVM is correctly faulting non-classical addresses > in MSR_IA32_DS_AREA, it helps ensure the accuracy and stability > of the KVM subsystem. > > Signed-off-by: Jinrong Liang <cloudliang@tencent.com> > --- > .../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 100 ++++++++++++++++++ > 1 file changed, 100 insertions(+) > > 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 4c90f76930f9..02903084598f 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 > @@ -19,6 +19,9 @@ > #include "kvm_util.h" > #include "vmx.h" > > +#define MAX_LINEAR_ADDR_MASK GENMASK_ULL(15, 8) > +#define ADDR_OFS_BIT 8 Similar to other comments, please spell this out. Whatever "OFS" stands for, it isn't common knowledge (unless I'm way out of the loop). > + > union perf_capabilities { > struct { > u64 lbr_format:6; > @@ -232,6 +235,102 @@ static void test_lbr_perf_capabilities(union perf_capabilities host_cap) > kvm_vm_free(vm); > } > > +/* > + * Generate a non-canonical address for a given number of address bits. > + * @addr_bits: The number of address bits used in the system. > + * > + * This function calculates a non-canonical address by setting the most > + * significant bit to 1 and adding an offset equal to the maximum value > + * that can be represented by the remaining bits. This ensures that the > + * generated address is outside the valid address range and is consistent. > + */ > +static inline uint64_t non_canonical_address(unsigned int addr_bits) > +{ > + return (1ULL << (addr_bits - 1)) + ((1ULL << (addr_bits - 1)) - 1); > +} Eh, I don't know that I would > + > +static uint64_t get_addr_bits(struct kvm_vcpu *vcpu) > +{ > + const struct kvm_cpuid_entry2 *kvm_entry; > + unsigned int addr_bits; > + struct kvm_sregs sregs; > + > + kvm_entry = get_cpuid_entry(kvm_get_supported_cpuid(), 0x80000008, 0); > + addr_bits = (kvm_entry->eax & MAX_LINEAR_ADDR_MASK) >> ADDR_OFS_BIT; *sigh* X86_PROPERTY_MAX_VIRT_ADDR, or even better, vcpu->vm->va_bits. > + /* > + * Get the size of the virtual address space by checking the LA57 bit > + * in the CR4 control register. If the LA57 bit is set, then the virtual > + * address space is 57 bits. Otherwise, it's 48 bits. > + */ > + if (addr_bits != 32) { > + vcpu_sregs_get(vcpu, &sregs); > + addr_bits = (sregs.cr4 & X86_CR4_LA57) ? 57 : 48; > + } > + > + return addr_bits; (a) None of this is PMU specific. (b) Selftests don't support LA57 (yet). (c) IMO there's no reason to get this fancy. Just hardcode a value that's guaranteed to be non-canonical and call it good. E.g. #define NONCANONICAL (0xaaaULL << 48) That way the "bad" value can be used from the guest directly. Or if you want to handle LA57 right now and verify that the guest can set values that are canonical for LA57 but not for LA48, something like static inline vm_vaddr_t get_noncanonical_address(bool is_la57) { return is_la57 ? BIT(57) : BIT(48); } and then the guest can invoke it by reading and passing in the current CR4, i.e. still doesn't require splitting logic across the guest and host. > +} > + > +static void test_ds_guest_code(uint64_t bad_addr) > +{ > + uint8_t vector = 0; > + > + vector = wrmsr_safe(MSR_IA32_DS_AREA, bad_addr); > + GUEST_SYNC(vector); GUEST_ASSERT(vector == GP_VECTOR); Aaron's fancy printf() stuff is coming, and even without that, splitting logic across the guest and host is generally a bad idea as it makes it much harder to understand what the test does. > +} > + > +/* Check if setting MSR_IA32_DS_AREA in guest and kvm userspace will fail. */ > +static void test_ds_area_noncanonical_address(union perf_capabilities host_cap) > +{ > + struct kvm_vm *vm; > + struct kvm_vcpu *vcpu; > + unsigned int r, addr_bits; > + uint64_t bad_addr, without_pebs_fmt_caps; > + struct ucall uc; Reverse xmas tree. > + > + vm = vm_create_with_one_vcpu(&vcpu, test_ds_guest_code); > + vm_init_descriptor_tables(vm); > + vcpu_init_descriptor_tables(vcpu); > + > + /* Check that Setting MSR_IA32_DS_AREA with 0 should succeed. */ Drop the comment, the assert covers everything. > + r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, 0); > + TEST_ASSERT(r, "Setting MSR_IA32_DS_AREA with 0 should succeed."); Eh, I would just do vcpu_set_msr() and let it assert for you. > + /* > + * Check that if PEBS_FMT is not set setting MSR_IA32_DS_AREA will > + * succeed. I don't understand what this comment is trying to say. The below tests that writing MSR_IA32_DS_AREA *fails*, not succeeds. > + */ > + without_pebs_fmt_caps = host_cap.capabilities & ~PERF_CAP_PEBS_FORMAT; > + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, without_pebs_fmt_caps); > + r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, 1); > + TEST_ASSERT(r, "Setting MSR_IA32_DS_AREA with bad addr should fail."); Print the actual "bad" value, and be more descriptive, i.e. state *why* '1' is a bad value. > + > + /* > + * Check that setting MSR_IA32_DS_AREA in kvm userspace to use a > + * non-canonical address should fail. > + */ > + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, host_cap.capabilities); > + addr_bits = get_addr_bits(vcpu); > + bad_addr = non_canonical_address(addr_bits); > + r = _vcpu_set_msr(vcpu, MSR_IA32_DS_AREA, bad_addr); > + TEST_ASSERT(!r, "Setting MSR_IA32_DS_AREA with bad addr should fail."); Same comment here. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES 2023-06-08 11:34 [PATCH 0/2] KVM: selftests: Add tests for PEBS and MSR_IA32_DS_AREA Jinrong Liang 2023-06-08 11:34 ` [PATCH 1/2] KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA Jinrong Liang @ 2023-06-08 11:34 ` Jinrong Liang 2023-06-28 21:55 ` Sean Christopherson 1 sibling, 1 reply; 6+ messages in thread From: Jinrong Liang @ 2023-06-08 11:34 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Wanpeng Li, Like Xu, Jinrong Liang, linux-kselftest, kvm, linux-kernel From: Jinrong Liang <cloudliang@tencent.com> This commit adds a PEBS test that verifies all possible combinations of PEBS-related bits in MSR_IA32_PERF_CAPABILITIES. This comprehensive test ensures the accuracy of the PEBS feature. Signed-off-by: Jinrong Liang <cloudliang@tencent.com> --- .../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) 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 02903084598f..c1b1ba44bc26 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 @@ -21,6 +21,12 @@ #define MAX_LINEAR_ADDR_MASK GENMASK_ULL(15, 8) #define ADDR_OFS_BIT 8 +#define PMU_CAP_LBR_FMT 0x3f +#define PMU_CAP_SMM_FREEZE BIT_ULL(12) +#define PMU_CAP_FW_WRITES BIT_ULL(13) +#define PMU_CAP_PERF_METRICS_AVAILABLE BIT_ULL(PERF_CAP_METRICS_IDX) +#define PMU_CAP_PEBS_OUTPUT_PT_AVAIL BIT_ULL(PERF_CAP_PT_IDX) +#define PMU_CAP_PEBS_ALL (PERF_CAP_PEBS_MASK | PMU_CAP_PEBS_OUTPUT_PT_AVAIL) union perf_capabilities { struct { @@ -331,6 +337,70 @@ static void test_ds_area_noncanonical_address(union perf_capabilities host_cap) kvm_vm_free(vm); } +static void test_pebs_bit_combinations(union perf_capabilities host_cap) +{ + int ret; + uint64_t pebs_val, val; + struct kvm_vcpu *vcpu; + struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, NULL); + + TEST_REQUIRE(kvm_cpu_property(X86_PROPERTY_PMU_VERSION) > 1); + TEST_REQUIRE(host_cap.capabilities & PERF_CAP_PEBS_FORMAT); + TEST_REQUIRE(vcpu_get_msr(vcpu, MSR_IA32_MISC_ENABLE) & + MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL); + + /* + * Test if PEBS_REC_FMT is set and the value is the same as host, + * the other PEBS bits are allowed to be set only if they are the + * same as host. + */ + pebs_val = host_cap.capabilities & PMU_CAP_PEBS_ALL; + + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, pebs_val); + ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), + (u64)pebs_val); + + /* Test all PEBS bit combinations. */ + for (val = 0x0; val <= (~0ul & PMU_CAP_PEBS_ALL); val++) { + /* Skips values that are not related to PEBS. */ + if (val & (PMU_CAP_LBR_FMT | PMU_CAP_SMM_FREEZE | + PMU_CAP_FW_WRITES | PMU_CAP_PERF_METRICS_AVAILABLE)) + continue; + + /* + * Test that value of PEBS is rejected when the KVM doesn't + * supports Intel PT. + */ + if ((val & PMU_CAP_PEBS_OUTPUT_PT_AVAIL) && + (!(host_cap.capabilities & PMU_CAP_PEBS_OUTPUT_PT_AVAIL))) { + ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); + TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val); + + continue; + } + + /* + * Test that value of PEBS is rejected when carrying + * PEBS_REC_FMT if the value of PEBS is not equal to host. + */ + if ((val & PERF_CAP_PEBS_FORMAT) && val != pebs_val) { + ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); + TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val); + + continue; + } + + /* + * Test that PEBS bits can be written simultaneously or + * independently if PEBS_REC_FMT is not carried. + */ + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); + ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), val); + } + + kvm_vm_free(vm); +} + int main(int argc, char *argv[]) { union perf_capabilities host_cap; @@ -352,4 +422,5 @@ int main(int argc, char *argv[]) test_guest_wrmsr_perf_capabilities(host_cap); test_lbr_perf_capabilities(host_cap); test_ds_area_noncanonical_address(host_cap); + test_pebs_bit_combinations(host_cap); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES 2023-06-08 11:34 ` [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES Jinrong Liang @ 2023-06-28 21:55 ` Sean Christopherson 2023-06-30 8:59 ` Jinrong Liang 0 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2023-06-28 21:55 UTC (permalink / raw) To: Jinrong Liang Cc: Paolo Bonzini, Wanpeng Li, Like Xu, Jinrong Liang, linux-kselftest, kvm, linux-kernel On Thu, Jun 08, 2023, Jinrong Liang wrote: > From: Jinrong Liang <cloudliang@tencent.com> > > This commit adds a PEBS test that verifies all possible combinations > of PEBS-related bits in MSR_IA32_PERF_CAPABILITIES. This comprehensive > test ensures the accuracy of the PEBS feature. > > Signed-off-by: Jinrong Liang <cloudliang@tencent.com> > --- > .../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 71 +++++++++++++++++++ > 1 file changed, 71 insertions(+) > > 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 02903084598f..c1b1ba44bc26 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 > @@ -21,6 +21,12 @@ > > #define MAX_LINEAR_ADDR_MASK GENMASK_ULL(15, 8) > #define ADDR_OFS_BIT 8 > +#define PMU_CAP_LBR_FMT 0x3f > +#define PMU_CAP_SMM_FREEZE BIT_ULL(12) > +#define PMU_CAP_FW_WRITES BIT_ULL(13) > +#define PMU_CAP_PERF_METRICS_AVAILABLE BIT_ULL(PERF_CAP_METRICS_IDX) > +#define PMU_CAP_PEBS_OUTPUT_PT_AVAIL BIT_ULL(PERF_CAP_PT_IDX) > +#define PMU_CAP_PEBS_ALL (PERF_CAP_PEBS_MASK | PMU_CAP_PEBS_OUTPUT_PT_AVAIL) > > union perf_capabilities { > struct { > @@ -331,6 +337,70 @@ static void test_ds_area_noncanonical_address(union perf_capabilities host_cap) > kvm_vm_free(vm); > } > > +static void test_pebs_bit_combinations(union perf_capabilities host_cap) > +{ > + int ret; Reverse xmas tree. > + uint64_t pebs_val, val; > + struct kvm_vcpu *vcpu; > + struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, NULL); It's kinda silly, but I think it makes sense to wait until after all of the TEST_REQUIRE()s to create the VM+vCPU. > + > + TEST_REQUIRE(kvm_cpu_property(X86_PROPERTY_PMU_VERSION) > 1); > + TEST_REQUIRE(host_cap.capabilities & PERF_CAP_PEBS_FORMAT); > + TEST_REQUIRE(vcpu_get_msr(vcpu, MSR_IA32_MISC_ENABLE) & > + MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL); > + > + /* > + * Test if PEBS_REC_FMT is set and the value is the same as host, > + * the other PEBS bits are allowed to be set only if they are the > + * same as host. > + */ > + pebs_val = host_cap.capabilities & PMU_CAP_PEBS_ALL; > + > + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, pebs_val); > + ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), > + (u64)pebs_val); This cast shouldn't be necessary. And if you're going to split lines... ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), host_cap.capabilities & PMU_CAP_PEBS_ALL); Though isn't that flawed? E.g. will fail if MSR_IA32_PERF_CAPABILITIES has non-PEBS bits set. I think what you want is something like: guest_perf_caps = vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES); ASSERT_EQ(guest_perf_caps & PMU_CAP_PEBS_ALL, host_cap.capabilities & PMU_CAP_PEBS_ALL); > + > + /* Test all PEBS bit combinations. */ > + for (val = 0x0; val <= (~0ul & PMU_CAP_PEBS_ALL); val++) { > + /* Skips values that are not related to PEBS. */ > + if (val & (PMU_CAP_LBR_FMT | PMU_CAP_SMM_FREEZE | > + PMU_CAP_FW_WRITES | PMU_CAP_PERF_METRICS_AVAILABLE)) Align things by their scope, i.e. if (val & (PMU_CAP_LBR_FMT | PMU_CAP_SMM_FREEZE PMU_CAP_FW_WRITES | PMU_CAP_PERF_METRICS_AVAILABLE)) But even better would be to look for !PEBS, not some other values where it's not clear they exhaustively cover all !PEBS value. E.g. can't this be? if (val & ~PMU_CAP_PEBS_ALL) continue; > + continue; > + > + /* > + * Test that value of PEBS is rejected when the KVM doesn't Just "KVM", not "the KVM". > + * supports Intel PT. > + */ > + if ((val & PMU_CAP_PEBS_OUTPUT_PT_AVAIL) && > + (!(host_cap.capabilities & PMU_CAP_PEBS_OUTPUT_PT_AVAIL))) { > + ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); > + TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val); > + > + continue; > + } > + > + /* > + * Test that value of PEBS is rejected when carrying I don't quite follow what you mean by "carrying". Do you mean a non-zero value? > + * PEBS_REC_FMT if the value of PEBS is not equal to host. > + */ > + if ((val & PERF_CAP_PEBS_FORMAT) && val != pebs_val) { > + ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); > + TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val); > + > + continue; > + } > + > + /* > + * Test that PEBS bits can be written simultaneously or > + * independently if PEBS_REC_FMT is not carried. > + */ > + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); > + ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), val); > + } > + > + kvm_vm_free(vm); > +} ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES 2023-06-28 21:55 ` Sean Christopherson @ 2023-06-30 8:59 ` Jinrong Liang 0 siblings, 0 replies; 6+ messages in thread From: Jinrong Liang @ 2023-06-30 8:59 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Wanpeng Li, Like Xu, Jinrong Liang, linux-kselftest, kvm, linux-kernel Sean Christopherson <seanjc@google.com> 于2023年6月29日周四 05:55写道： > > On Thu, Jun 08, 2023, Jinrong Liang wrote: > > From: Jinrong Liang <cloudliang@tencent.com> > > > > This commit adds a PEBS test that verifies all possible combinations > > of PEBS-related bits in MSR_IA32_PERF_CAPABILITIES. This comprehensive > > test ensures the accuracy of the PEBS feature. > > > > Signed-off-by: Jinrong Liang <cloudliang@tencent.com> > > --- > > .../selftests/kvm/x86_64/vmx_pmu_caps_test.c | 71 +++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > > > 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 02903084598f..c1b1ba44bc26 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 > > @@ -21,6 +21,12 @@ > > > > #define MAX_LINEAR_ADDR_MASK GENMASK_ULL(15, 8) > > #define ADDR_OFS_BIT 8 > > +#define PMU_CAP_LBR_FMT 0x3f > > +#define PMU_CAP_SMM_FREEZE BIT_ULL(12) > > +#define PMU_CAP_FW_WRITES BIT_ULL(13) > > +#define PMU_CAP_PERF_METRICS_AVAILABLE BIT_ULL(PERF_CAP_METRICS_IDX) > > +#define PMU_CAP_PEBS_OUTPUT_PT_AVAIL BIT_ULL(PERF_CAP_PT_IDX) > > +#define PMU_CAP_PEBS_ALL (PERF_CAP_PEBS_MASK | PMU_CAP_PEBS_OUTPUT_PT_AVAIL) > > > > union perf_capabilities { > > struct { > > @@ -331,6 +337,70 @@ static void test_ds_area_noncanonical_address(union perf_capabilities host_cap) > > kvm_vm_free(vm); > > } > > > > +static void test_pebs_bit_combinations(union perf_capabilities host_cap) > > +{ > > + int ret; > > Reverse xmas tree. > > > + uint64_t pebs_val, val; > > + struct kvm_vcpu *vcpu; > > + struct kvm_vm *vm = vm_create_with_one_vcpu(&vcpu, NULL); > > It's kinda silly, but I think it makes sense to wait until after all of the > TEST_REQUIRE()s to create the VM+vCPU. > > > + > > + TEST_REQUIRE(kvm_cpu_property(X86_PROPERTY_PMU_VERSION) > 1); > > + TEST_REQUIRE(host_cap.capabilities & PERF_CAP_PEBS_FORMAT); > > + TEST_REQUIRE(vcpu_get_msr(vcpu, MSR_IA32_MISC_ENABLE) & > > + MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL); > > + > > + /* > > + * Test if PEBS_REC_FMT is set and the value is the same as host, > > + * the other PEBS bits are allowed to be set only if they are the > > + * same as host. > > + */ > > + pebs_val = host_cap.capabilities & PMU_CAP_PEBS_ALL; > > + > > + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, pebs_val); > > + ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), > > + (u64)pebs_val); > > This cast shouldn't be necessary. And if you're going to split lines... > > ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), > host_cap.capabilities & PMU_CAP_PEBS_ALL); > > Though isn't that flawed? E.g. will fail if MSR_IA32_PERF_CAPABILITIES has > non-PEBS bits set. I think what you want is something like: > > guest_perf_caps = vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES); > > ASSERT_EQ(guest_perf_caps & PMU_CAP_PEBS_ALL, > host_cap.capabilities & PMU_CAP_PEBS_ALL); > > > + > > + /* Test all PEBS bit combinations. */ > > + for (val = 0x0; val <= (~0ul & PMU_CAP_PEBS_ALL); val++) { > > + /* Skips values that are not related to PEBS. */ > > + if (val & (PMU_CAP_LBR_FMT | PMU_CAP_SMM_FREEZE | > > + PMU_CAP_FW_WRITES | PMU_CAP_PERF_METRICS_AVAILABLE)) > > Align things by their scope, i.e. > > if (val & (PMU_CAP_LBR_FMT | PMU_CAP_SMM_FREEZE > PMU_CAP_FW_WRITES | PMU_CAP_PERF_METRICS_AVAILABLE)) > > But even better would be to look for !PEBS, not some other values where it's not > clear they exhaustively cover all !PEBS value. E.g. can't this be? > > if (val & ~PMU_CAP_PEBS_ALL) > continue; > > > + continue; > > + > > + /* > > + * Test that value of PEBS is rejected when the KVM doesn't > > Just "KVM", not "the KVM". > > > + * supports Intel PT. > > + */ > > + if ((val & PMU_CAP_PEBS_OUTPUT_PT_AVAIL) && > > + (!(host_cap.capabilities & PMU_CAP_PEBS_OUTPUT_PT_AVAIL))) { > > + ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); > > + TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val); > > + > > + continue; > > + } > > + > > + /* > > + * Test that value of PEBS is rejected when carrying > > I don't quite follow what you mean by "carrying". Do you mean a non-zero value? I apologize for the confusion. Yes, by "carrying" I meant a non-zero value. I will revise the comment to clarify the meaning and make it more precise. > > > + * PEBS_REC_FMT if the value of PEBS is not equal to host. > > + */ > > + if ((val & PERF_CAP_PEBS_FORMAT) && val != pebs_val) { > > + ret = _vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); > > + TEST_ASSERT(!ret, "Bad PEBS auxiliary bits = 0x%lx didn't fail", val); > > + > > + continue; > > + } > > + > > + /* > > + * Test that PEBS bits can be written simultaneously or > > + * independently if PEBS_REC_FMT is not carried. > > + */ > > + vcpu_set_msr(vcpu, MSR_IA32_PERF_CAPABILITIES, val); > > + ASSERT_EQ(vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES), val); > > + } > > + > > + kvm_vm_free(vm); > > +} Thank you for all your valuable feedback and suggestions. Your guidance has been extremely helpful in improving the quality of the code. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-30 8:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-08 11:34 [PATCH 0/2] KVM: selftests: Add tests for PEBS and MSR_IA32_DS_AREA Jinrong Liang 2023-06-08 11:34 ` [PATCH 1/2] KVM: selftests: Test consistency of setting MSR_IA32_DS_AREA Jinrong Liang 2023-06-28 21:48 ` Sean Christopherson 2023-06-08 11:34 ` [PATCH 2/2] KVM: selftests: Add PEBS test for MSR_IA32_PERF_CAPABILITIES Jinrong Liang 2023-06-28 21:55 ` Sean Christopherson 2023-06-30 8:59 ` Jinrong Liang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox