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.

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

+}