Hi Raghu,
On Thu, Aug 17, 2023 at 12:30:27AM +0000, Raghavendra Rao Ananta wrote:
> From: Reiji Watanabe <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> ---
> 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.
> +/*
> + * 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.
--
Thanks,
Oliver
Hi Oliver,
On Fri, Sep 15, 2023 at 2:00 PM Oliver Upton <[email protected]> wrote:
>
> Hi Raghu,
>
> On Thu, Aug 17, 2023 at 12:30:27AM +0000, Raghavendra Rao Ananta wrote:
> > From: Reiji Watanabe <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Raghavendra Rao Ananta <[email protected]>
> > ---
> > 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