Hi Oliver, On Fri, Sep 15, 2023 at 2:00 PM Oliver Upton <oliver.upton@xxxxxxxxx> wrote: > > Hi Raghu, > > On Thu, Aug 17, 2023 at 12:30:27AM +0000, Raghavendra Rao Ananta wrote: > > From: Reiji Watanabe <reijiw@xxxxxxxxxx> > > > > Introduce vpmu_counter_access test for arm64 platforms. > > The test configures PMUv3 for a vCPU, sets PMCR_EL0.N for the vCPU, > > and check if the guest can consistently see the same number of the > > PMU event counters (PMCR_EL0.N) that userspace sets. > > This test case is done with each of the PMCR_EL0.N values from > > 0 to 31 (With the PMCR_EL0.N values greater than the host value, > > the test expects KVM_SET_ONE_REG for the PMCR_EL0 to fail). > > > > Signed-off-by: Reiji Watanabe <reijiw@xxxxxxxxxx> > > Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx> > > --- > > tools/testing/selftests/kvm/Makefile | 1 + > > .../kvm/aarch64/vpmu_counter_access.c | 235 ++++++++++++++++++ > > 2 files changed, 236 insertions(+) > > create mode 100644 tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c > > > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > > index c692cc86e7da8..a1599e2b82e38 100644 > > --- a/tools/testing/selftests/kvm/Makefile > > +++ b/tools/testing/selftests/kvm/Makefile > > @@ -148,6 +148,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/smccc_filter > > TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config > > TEST_GEN_PROGS_aarch64 += aarch64/vgic_init > > TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq > > +TEST_GEN_PROGS_aarch64 += aarch64/vpmu_counter_access > > TEST_GEN_PROGS_aarch64 += access_tracking_perf_test > > TEST_GEN_PROGS_aarch64 += demand_paging_test > > TEST_GEN_PROGS_aarch64 += dirty_log_test > > diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c b/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c > > new file mode 100644 > > index 0000000000000..d0afec07948ef > > --- /dev/null > > +++ b/tools/testing/selftests/kvm/aarch64/vpmu_counter_access.c > > @@ -0,0 +1,235 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * vpmu_counter_access - Test vPMU event counter access > > + * > > + * Copyright (c) 2022 Google LLC. > > + * > > + * This test checks if the guest can see the same number of the PMU event > > + * counters (PMCR_EL0.N) that userspace sets. > > + * This test runs only when KVM_CAP_ARM_PMU_V3 is supported on the host. > > + */ > > +#include <kvm_util.h> > > +#include <processor.h> > > +#include <test_util.h> > > +#include <vgic.h> > > +#include <perf/arm_pmuv3.h> > > +#include <linux/bitfield.h> > > + > > +/* The max number of the PMU event counters (excluding the cycle counter) */ > > +#define ARMV8_PMU_MAX_GENERAL_COUNTERS (ARMV8_PMU_MAX_COUNTERS - 1) > > + > > +struct vpmu_vm { > > + struct kvm_vm *vm; > > + struct kvm_vcpu *vcpu; > > + int gic_fd; > > +}; > > + > > nit: this test is single threaded, so there will only ever be a single > instance of a VM at a time. Dynamically allocating a backing structure > doesn't add any value, IMO. > > You can just get away with using globals. > Probably. I can try to have a single global. > > +/* > > + * Create a guest with one vCPU, and attempt to set the PMCR_EL0.N for > > + * the vCPU to @pmcr_n, which is larger than the host value. > > + * The attempt should fail as @pmcr_n is too big to set for the vCPU. > > + */ > > +static void run_error_test(uint64_t pmcr_n) > > +{ > > + struct vpmu_vm *vpmu_vm; > > + struct kvm_vcpu *vcpu; > > + int ret; > > + uint64_t pmcr, pmcr_orig; > > + > > + pr_debug("Error test with pmcr_n %lu (larger than the host)\n", pmcr_n); > > + vpmu_vm = create_vpmu_vm(guest_code); > > + vcpu = vpmu_vm->vcpu; > > + > > + /* Update the PMCR_EL0.N with @pmcr_n */ > > + vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_PMCR_EL0), &pmcr_orig); > > + pmcr = pmcr_orig & ~ARMV8_PMU_PMCR_N; > > + pmcr |= (pmcr_n << ARMV8_PMU_PMCR_N_SHIFT); > > + > > + /* This should fail as @pmcr_n is too big to set for the vCPU */ > > + ret = __vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_PMCR_EL0), pmcr); > > + TEST_ASSERT(ret, "Setting PMCR to 0x%lx (orig PMCR 0x%lx) didn't fail", > > + pmcr, pmcr_orig); > > The failure pattern for this should now be the write to PMCR_EL0.N had > no effect. > Right. I'll make the change. Thank you. Raghavendra > -- > Thanks, > Oliver