by Shaoqin Huang

[permalink] [raw]

Subject: Re: [PATCH v1 3/3] KVM: selftests: aarch64: Introduce pmu_event_filter_test

Hi Eric,

On 11/27/23 16:10, Eric Auger wrote:
> Hi Shaoqin,
>
> On 11/23/23 07:37, Shaoqin Huang wrote:
>> Introduce pmu_event_filter_test for arm64 platforms. The test configures
>> PMUv3 for a vCPU, and sets different pmu event filter for the vCPU, and
> filters
>> check if the guest can use those events which user allow and can't use
>> those events which use deny.
>>
>> This test refactor the create_vpmu_vm() and make it a wrapper for
>> __create_vpmu_vm(), which can let we do some extra init before
> which can let we do -> which allows some extra init code.

Copy that.

>> KVM_ARM_VCPU_PMU_V3_INIT.
>>
>> This test choose the branches_retired and the instructions_retired
>> event, and let guest use the two events in pmu. And check if the result
> Are you sure those events are supported?
>> is expected.
>>
>> Signed-off-by: Shaoqin Huang <[email protected]>
>> ---
>> tools/testing/selftests/kvm/Makefile | 1 +
>> .../kvm/aarch64/pmu_event_filter_test.c | 227 ++++++++++++++++++
>> .../selftests/kvm/include/aarch64/vpmu.h | 4 +
>> .../testing/selftests/kvm/lib/aarch64/vpmu.c | 14 +-
>> 4 files changed, 244 insertions(+), 2 deletions(-)
>> create mode 100644 tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index b60852c222ac..5f126e1a1dbf 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -155,6 +155,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer
>> TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>> TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
>> TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
>> +TEST_GEN_PROGS_aarch64 += aarch64/pmu_event_filter_test
>> TEST_GEN_PROGS_aarch64 += aarch64/psci_test
>> TEST_GEN_PROGS_aarch64 += aarch64/set_id_regs
>> TEST_GEN_PROGS_aarch64 += aarch64/smccc_filter
>> diff --git a/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
>> new file mode 100644
>> index 000000000000..a876f5c2033b
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/aarch64/pmu_event_filter_test.c
>> @@ -0,0 +1,227 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * pmu_event_filter_test - Test user limit pmu event for guest.
>> + *
>> + * Copyright (c) 2023 Red Hat, Inc.
>> + *
>> + * This test checks if the guest only see the limited pmu event that userspace
> sees
>> + * sets, if the gust can use those events which user allow, and if the guest
> s/gust/guest

Thanks, will correct it.

>> + * can't use those events which user deny.
>> + * It also checks set invalid filter return the expected error.
> it also checks that setting invalid filter ranges ...
>> + * This test runs only when KVM_CAP_ARM_PMU_V3 is supported on the host.
>> + */
>> +#include <kvm_util.h>
>> +#include <processor.h>
>> +#include <vgic.h>
>> +#include <vpmu.h>
>> +#include <test_util.h>
>> +#include <perf/arm_pmuv3.h>
>> +
>> +struct {
>> + uint64_t branches_retired;
>> + uint64_t instructions_retired;
>> +} pmc_results;
>> +
>> +static struct vpmu_vm *vpmu_vm;
>> +
>> +#define FILTER_NR 10
>> +
>> +struct test_desc {
>> + const char *name;
>> + void (*check_result)(void);
>> + struct kvm_pmu_event_filter filter[FILTER_NR];
>> +};
>> +
>> +#define __DEFINE_FILTER(base, num, act) \
>> + ((struct kvm_pmu_event_filter) { \
>> + .base_event = base, \
>> + .nevents = num, \
>> + .action = act, \
>> + })
>> +
>> +#define DEFINE_FILTER(base, act) __DEFINE_FILTER(base, 1, act)
>> +
>> +#define EMPTY_FILTER { 0 }
>> +
>> +#define SW_INCR 0x0
>> +#define INST_RETIRED 0x8
>> +#define BR_RETIERD 0x21
> looks like a typo

It's a typo error. Fixed it.

>> +
>> +#define NUM_BRANCHES 10
>> +
>> +static void run_and_measure_loop(void)
>> +{
>> + asm volatile(
>> + " mov x10, %[loop]\n"
>> + "1: sub x10, x10, #1\n"
>> + " cmp x10, #0x0\n"
>> + " b.gt 1b\n"
>> + :
>> + : [loop] "r" (NUM_BRANCHES)
>> + : "x10", "cc");
>> +}
>> +
>> +static void guest_code(void)
>> +{
>> + uint64_t pmcr = read_sysreg(pmcr_el0);
>> +
>> + pmu_disable_reset();
>> +
>> + write_pmevtypern(0, BR_RETIERD);
>> + write_pmevtypern(1, INST_RETIRED);
>> + enable_counter(0);
>> + enable_counter(1);
>> + write_sysreg(pmcr | ARMV8_PMU_PMCR_E, pmcr_el0);
>> +
>> + run_and_measure_loop();
>> +
>> + write_sysreg(pmcr, pmcr_el0);
>> +
>> + pmc_results.branches_retired = read_sysreg(pmevcntr0_el0);
>> + pmc_results.instructions_retired = read_sysreg(pmevcntr1_el0);
>> +
>> + GUEST_DONE();
> another direct way to see if the guest can use those filters is to read
> the PMCEIDx that indicates whether an event is supported.
>

Yes. That's the easist way. Why I do this is because I follow the x86
design.

>> +}
>> +
>> +static void pmu_event_filter_init(struct vpmu_vm *vm, void *arg)
>> +{
>> + struct kvm_device_attr attr = {
>> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
>> + };
>> + struct kvm_pmu_event_filter *filter = (struct kvm_pmu_event_filter *)arg;
>> +
>> + while (filter && filter->nevents != 0) {
>> + attr.addr = (uint64_t)filter;
>> + vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
>> + filter++;
>> + }
>> +}
>> +
>> +static void create_vpmu_vm_with_filter(void *guest_code,
>> + struct kvm_pmu_event_filter *filter)
>> +{
>> + vpmu_vm = __create_vpmu_vm(guest_code, pmu_event_filter_init, filter);
>> +}
>> +
>> +static void run_vcpu(struct kvm_vcpu *vcpu)
>> +{
>> + struct ucall uc;
>> +
>> + while (1) {
>> + vcpu_run(vcpu);
>> + switch (get_ucall(vcpu, &uc)) {
>> + case UCALL_DONE:
>> + return;
>> + default:
>> + TEST_FAIL("Unknown ucall %lu", uc.cmd);
>> + }
>> + }
>> +}
>> +
>> +static void check_pmc_counting(void)
>> +{
>> + uint64_t br = pmc_results.branches_retired;
>> + uint64_t ir = pmc_results.instructions_retired;
>> +
>> + TEST_ASSERT(br && br == NUM_BRANCHES, "Branch instructions retired = "
>> + "%lu (expected %u)", br, NUM_BRANCHES);
>> + TEST_ASSERT(ir, "Instructions retired = %lu (expected > 0)", ir);
>> +}
>> +
>> +static void check_pmc_not_counting(void)
>> +{
>> + uint64_t br = pmc_results.branches_retired;
>> + uint64_t ir = pmc_results.instructions_retired;
>> +
>> + TEST_ASSERT(!br, "Branch instructions retired = %lu (expected 0)", br);
>> + TEST_ASSERT(!ir, "Instructions retired = %lu (expected 0)", ir);
>> +}
>> +
>> +static void run_vcpu_and_sync_pmc_results(void)
>> +{
>> + memset(&pmc_results, 0, sizeof(pmc_results));
>> + sync_global_to_guest(vpmu_vm->vm, pmc_results);
>> +
>> + run_vcpu(vpmu_vm->vcpu);
>> +
>> + sync_global_from_guest(vpmu_vm->vm, pmc_results);
>> +}
>> +
>> +static void run_test(struct test_desc *t)
>> +{
>> + pr_debug("Test: %s\n", t->name);
>> +
>> + create_vpmu_vm_with_filter(guest_code, t->filter);
>> +
>> + run_vcpu_and_sync_pmc_results();
>> +
>> + t->check_result();
>> +
>> + destroy_vpmu_vm(vpmu_vm);
>> +}
>> +
>> +static struct test_desc tests[] = {
>> + {"without_filter", check_pmc_counting, { EMPTY_FILTER }},
>> + {"member_allow_filter", check_pmc_counting,
>> + {DEFINE_FILTER(SW_INCR, 0), DEFINE_FILTER(INST_RETIRED, 0),
>> + DEFINE_FILTER(BR_RETIERD, 0), EMPTY_FILTER}},
>> + {"member_deny_filter", check_pmc_not_counting,
>> + {DEFINE_FILTER(SW_INCR, 1), DEFINE_FILTER(INST_RETIRED, 1),
>> + DEFINE_FILTER(BR_RETIERD, 1), EMPTY_FILTER}},
>> + {"not_member_deny_filter", check_pmc_counting,
>> + {DEFINE_FILTER(SW_INCR, 1), EMPTY_FILTER}},
>> + {"not_member_allow_filter", check_pmc_not_counting,
>> + {DEFINE_FILTER(SW_INCR, 0), EMPTY_FILTER}},
>> + { 0 }
>> +};
>> +
>> +static void for_each_test(void)
>> +{
>> + struct test_desc *t;
>> +
>> + for (t = &tests[0]; t->name; t++)
>> + run_test(t);
>> +}
>> +
>> +static void set_invalid_filter(struct vpmu_vm *vm, void *arg)
>> +{
>> + struct kvm_pmu_event_filter invalid;
>> + struct kvm_device_attr attr = {
>> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
>> + .addr = (uint64_t)&invalid,
>> + };
>> + int ret = 0;
>> +
>> + /* The max event number is (1 << 16), set a range large than it. */
>> + invalid = __DEFINE_FILTER(BIT(15), BIT(15)+1, 0);
>> + ret = __vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
>> + TEST_ASSERT(ret && errno == EINVAL, "Set Invalid filter range "
>> + "ret = %d, errno = %d (expected ret = -1, errno = EINVAL)",
>> + ret, errno);
>> +
>> + ret = 0;
>> +
>> + /* Set the Invalid action. */
>> + invalid = __DEFINE_FILTER(0, 1, 3);
>> + ret = __vcpu_ioctl(vm->vcpu, KVM_SET_DEVICE_ATTR, &attr);
>> + TEST_ASSERT(ret && errno == EINVAL, "Set Invalid filter action "
>> + "ret = %d, errno = %d (expected ret = -1, errno = EINVAL)",
>> + ret, errno);
>> +}
>> +
>> +static void test_invalid_filter(void)
>> +{
>> + vpmu_vm = __create_vpmu_vm(guest_code, set_invalid_filter, NULL);
>> + destroy_vpmu_vm(vpmu_vm);
>> +}
>> +
>> +int main(void)
>> +{
>> + TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_PMU_V3));
>> +
>> + for_each_test();
>> +
>> + test_invalid_filter();
> I would introduce test_invalid_filter in a separate patch

Ok. I can split it into two.

Thanks,
Shaoqin

>> +}
>> diff --git a/tools/testing/selftests/kvm/include/aarch64/vpmu.h b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
>> index e0cc1ca1c4b7..db97bfb07996 100644
>> --- a/tools/testing/selftests/kvm/include/aarch64/vpmu.h
>> +++ b/tools/testing/selftests/kvm/include/aarch64/vpmu.h
>> @@ -18,6 +18,10 @@ struct vpmu_vm {
>> int gic_fd;
>> };
>>
>> +struct vpmu_vm *__create_vpmu_vm(void *guest_code,
>> + void (*init_pmu)(struct vpmu_vm *vm, void *arg),
>> + void *arg);
>> +
>> struct vpmu_vm *create_vpmu_vm(void *guest_code);
>>
>> void destroy_vpmu_vm(struct vpmu_vm *vpmu_vm);
>> diff --git a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> index b3de8fdc555e..76ea03d607f1 100644
>> --- a/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> +++ b/tools/testing/selftests/kvm/lib/aarch64/vpmu.c
>> @@ -7,8 +7,9 @@
>> #include <vpmu.h>
>> #include <perf/arm_pmuv3.h>
>>
>> -/* Create a VM that has one vCPU with PMUv3 configured. */
>> -struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> +struct vpmu_vm *__create_vpmu_vm(void *guest_code,
>> + void (*init_pmu)(struct vpmu_vm *vm, void *arg),
>> + void *arg)
>> {
>> struct kvm_vcpu_init init;
>> uint8_t pmuver;
>> @@ -50,12 +51,21 @@ struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> "Unexpected PMUVER (0x%x) on the vCPU with PMUv3", pmuver);
>>
>> /* Initialize vPMU */
>> + if (init_pmu)
>> + init_pmu(vpmu_vm, arg);
>> +
>> vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &irq_attr);
>> vcpu_ioctl(vpmu_vm->vcpu, KVM_SET_DEVICE_ATTR, &init_attr);
>>
>> return vpmu_vm;
>> }
>>
>> +/* Create a VM that has one vCPU with PMUv3 configured. */
>> +struct vpmu_vm *create_vpmu_vm(void *guest_code)
>> +{
>> + return __create_vpmu_vm(guest_code, NULL, NULL);
>> +}
>> +
>> void destroy_vpmu_vm(struct vpmu_vm *vpmu_vm)
>> {
>> close(vpmu_vm->gic_fd);
> Thanks
>
> Eric
>

--
Shaoqin