Extend the vCPU migration test to also validate the vPMU's
functionality when set up for overflow conditions.
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
.../testing/selftests/kvm/aarch64/vpmu_test.c | 223 ++++++++++++++++--
1 file changed, 198 insertions(+), 25 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_test.c b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
index 0c9d801f4e602..066dc17fa3906 100644
--- a/tools/testing/selftests/kvm/aarch64/vpmu_test.c
+++ b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
@@ -21,7 +21,9 @@
*
* 4. Since the PMU registers are per-cpu, stress KVM by frequently
* migrating the guest vCPU to random pCPUs in the system, and check
- * if the vPMU is still behaving as expected.
+ * if the vPMU is still behaving as expected. The sub-tests include
+ * testing basic functionalities such as basic counters behavior,
+ * overflow, and overflow interrupts.
*
* Copyright (c) 2022 Google LLC.
*
@@ -41,13 +43,27 @@
#include <sys/sysinfo.h>
#include "delay.h"
+#include "gic.h"
+#include "spinlock.h"
/* The max number of the PMU event counters (excluding the cycle counter) */
#define ARMV8_PMU_MAX_GENERAL_COUNTERS (ARMV8_PMU_MAX_COUNTERS - 1)
+/* The cycle counter bit position that's common among the PMU registers */
+#define ARMV8_PMU_CYCLE_COUNTER_IDX 31
+
/* The max number of event numbers that's supported */
#define ARMV8_PMU_MAX_EVENTS 64
+#define PMU_IRQ 23
+
+#define COUNT_TO_OVERFLOW 0xFULL
+#define PRE_OVERFLOW_32 (GENMASK(31, 0) - COUNT_TO_OVERFLOW + 1)
+#define PRE_OVERFLOW_64 (GENMASK(63, 0) - COUNT_TO_OVERFLOW + 1)
+
+#define GICD_BASE_GPA 0x8000000ULL
+#define GICR_BASE_GPA 0x80A0000ULL
+
#define msecs_to_usecs(msec) ((msec) * 1000LL)
/*
@@ -162,6 +178,17 @@ static inline void write_sel_evtyper(int sel, unsigned long val)
isb();
}
+static inline void write_pmovsclr(unsigned long val)
+{
+ write_sysreg(val, pmovsclr_el0);
+ isb();
+}
+
+static unsigned long read_pmovsclr(void)
+{
+ return read_sysreg(pmovsclr_el0);
+}
+
static inline void enable_counter(int idx)
{
uint64_t v = read_sysreg(pmcntenset_el0);
@@ -178,11 +205,33 @@ static inline void disable_counter(int idx)
isb();
}
+static inline void enable_irq(int idx)
+{
+ uint64_t v = read_sysreg(pmcntenset_el0);
+
+ write_sysreg(BIT(idx) | v, pmintenset_el1);
+ isb();
+}
+
+static inline void disable_irq(int idx)
+{
+ uint64_t v = read_sysreg(pmcntenset_el0);
+
+ write_sysreg(BIT(idx) | v, pmintenclr_el1);
+ isb();
+}
+
static inline uint64_t read_cycle_counter(void)
{
return read_sysreg(pmccntr_el0);
}
+static inline void write_cycle_counter(uint64_t v)
+{
+ write_sysreg(v, pmccntr_el0);
+ isb();
+}
+
static inline void reset_cycle_counter(void)
{
uint64_t v = read_sysreg(pmcr_el0);
@@ -289,6 +338,15 @@ struct guest_data {
static struct guest_data guest_data;
+/* Data to communicate among guest threads */
+struct guest_irq_data {
+ uint32_t pmc_idx_bmap;
+ uint32_t irq_received_bmap;
+ struct spinlock lock;
+};
+
+static struct guest_irq_data guest_irq_data;
+
#define VCPU_MIGRATIONS_TEST_ITERS_DEF 1000
#define VCPU_MIGRATIONS_TEST_MIGRATION_FREQ_MS 2
@@ -322,6 +380,79 @@ static void guest_sync_handler(struct ex_regs *regs)
expected_ec = INVALID_EC;
}
+static void guest_validate_irq(int pmc_idx, uint32_t pmovsclr, uint32_t pmc_idx_bmap)
+{
+ /*
+ * Fail if there's an interrupt from unexpected PMCs.
+ * All the expected events' IRQs may not arrive at the same time.
+ * Hence, check if the interrupt is valid only if it's expected.
+ */
+ if (pmovsclr & BIT(pmc_idx)) {
+ GUEST_ASSERT_3(pmc_idx_bmap & BIT(pmc_idx), pmc_idx, pmovsclr, pmc_idx_bmap);
+ write_pmovsclr(BIT(pmc_idx));
+ }
+}
+
+static void guest_irq_handler(struct ex_regs *regs)
+{
+ uint32_t pmc_idx_bmap;
+ uint64_t i, pmcr_n = get_pmcr_n();
+ uint32_t pmovsclr = read_pmovsclr();
+ unsigned int intid = gic_get_and_ack_irq();
+
+ /* No other IRQ apart from the PMU IRQ is expected */
+ GUEST_ASSERT_1(intid == PMU_IRQ, intid);
+
+ spin_lock(&guest_irq_data.lock);
+ pmc_idx_bmap = READ_ONCE(guest_irq_data.pmc_idx_bmap);
+
+ for (i = 0; i < pmcr_n; i++)
+ guest_validate_irq(i, pmovsclr, pmc_idx_bmap);
+ guest_validate_irq(ARMV8_PMU_CYCLE_COUNTER_IDX, pmovsclr, pmc_idx_bmap);
+
+ /* Mark IRQ as recived for the corresponding PMCs */
+ WRITE_ONCE(guest_irq_data.irq_received_bmap, pmovsclr);
+ spin_unlock(&guest_irq_data.lock);
+
+ gic_set_eoi(intid);
+}
+
+static int pmu_irq_received(int pmc_idx)
+{
+ bool irq_received;
+
+ spin_lock(&guest_irq_data.lock);
+ irq_received = READ_ONCE(guest_irq_data.irq_received_bmap) & BIT(pmc_idx);
+ WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
+ spin_unlock(&guest_irq_data.lock);
+
+ return irq_received;
+}
+
+static void pmu_irq_init(int pmc_idx)
+{
+ write_pmovsclr(BIT(pmc_idx));
+
+ spin_lock(&guest_irq_data.lock);
+ WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
+ WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap | BIT(pmc_idx));
+ spin_unlock(&guest_irq_data.lock);
+
+ enable_irq(pmc_idx);
+}
+
+static void pmu_irq_exit(int pmc_idx)
+{
+ write_pmovsclr(BIT(pmc_idx));
+
+ spin_lock(&guest_irq_data.lock);
+ WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
+ WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
+ spin_unlock(&guest_irq_data.lock);
+
+ disable_irq(pmc_idx);
+}
+
/*
* Run the given operation that should trigger an exception with the
* given exception class. The exception handler (guest_sync_handler)
@@ -420,12 +551,20 @@ static void execute_precise_instrs(int num, uint32_t pmcr)
precise_instrs_loop(loop, pmcr);
}
-static void test_instructions_count(int pmc_idx, bool expect_count)
+static void test_instructions_count(int pmc_idx, bool expect_count, bool test_overflow)
{
int i;
struct pmc_accessor *acc;
- uint64_t cnt;
- int instrs_count = 100;
+ uint64_t cntr_val = 0;
+ int instrs_count = 500;
+
+ if (test_overflow) {
+ /* Overflow scenarios can only be tested when a count is expected */
+ GUEST_ASSERT_1(expect_count, pmc_idx);
+
+ cntr_val = PRE_OVERFLOW_32;
+ pmu_irq_init(pmc_idx);
+ }
enable_counter(pmc_idx);
@@ -433,41 +572,68 @@ static void test_instructions_count(int pmc_idx, bool expect_count)
for (i = 0; i < ARRAY_SIZE(pmc_accessors); i++) {
acc = &pmc_accessors[i];
- pmu_disable_reset();
-
+ acc->write_cntr(pmc_idx, cntr_val);
acc->write_typer(pmc_idx, ARMV8_PMUV3_PERFCTR_INST_RETIRED);
- /* Enable the PMU and execute precisely number of instructions as a workload */
- execute_precise_instrs(instrs_count, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
+ /*
+ * Enable the PMU and execute a precise number of instructions as a workload.
+ * Since execute_precise_instrs() disables the PMU at the end, 'instrs_count'
+ * should have enough instructions to raise an IRQ.
+ */
+ execute_precise_instrs(instrs_count, ARMV8_PMU_PMCR_E);
- /* If a count is expected, the counter should be increased by 'instrs_count' */
- cnt = acc->read_cntr(pmc_idx);
- GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
- i, expect_count, cnt, instrs_count);
+ /*
+ * If an overflow is expected, only check for the overflag flag.
+ * As overflow interrupt is enabled, the interrupt would add additional
+ * instructions and mess up the precise instruction count. Hence, measure
+ * the instructions count only when the test is not set up for an overflow.
+ */
+ if (test_overflow) {
+ GUEST_ASSERT_2(pmu_irq_received(pmc_idx), pmc_idx, i);
+ } else {
+ uint64_t cnt = acc->read_cntr(pmc_idx);
+
+ GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
+ pmc_idx, i, cnt, expect_count);
+ }
}
- disable_counter(pmc_idx);
+ if (test_overflow)
+ pmu_irq_exit(pmc_idx);
}
-static void test_cycles_count(bool expect_count)
+static void test_cycles_count(bool expect_count, bool test_overflow)
{
uint64_t cnt;
- pmu_enable();
- reset_cycle_counter();
+ if (test_overflow) {
+ /* Overflow scenarios can only be tested when a count is expected */
+ GUEST_ASSERT(expect_count);
+
+ write_cycle_counter(PRE_OVERFLOW_64);
+ pmu_irq_init(ARMV8_PMU_CYCLE_COUNTER_IDX);
+ } else {
+ reset_cycle_counter();
+ }
/* Count cycles in EL0 and EL1 */
write_pmccfiltr(0);
enable_cycle_counter();
+ /* Enable the PMU and execute precisely number of instructions as a workload */
+ execute_precise_instrs(500, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
cnt = read_cycle_counter();
/*
* If a count is expected by the test, the cycle counter should be increased by
- * at least 1, as there is at least one instruction between enabling the
+ * at least 1, as there are a number of instructions between enabling the
* counter and reading the counter.
*/
GUEST_ASSERT_2(expect_count == (cnt > 0), cnt, expect_count);
+ if (test_overflow) {
+ GUEST_ASSERT_2(pmu_irq_received(ARMV8_PMU_CYCLE_COUNTER_IDX), cnt, expect_count);
+ pmu_irq_exit(ARMV8_PMU_CYCLE_COUNTER_IDX);
+ }
disable_cycle_counter();
pmu_disable_reset();
@@ -477,19 +643,28 @@ static void test_event_count(uint64_t event, int pmc_idx, bool expect_count)
{
switch (event) {
case ARMV8_PMUV3_PERFCTR_INST_RETIRED:
- test_instructions_count(pmc_idx, expect_count);
+ test_instructions_count(pmc_idx, expect_count, false);
break;
case ARMV8_PMUV3_PERFCTR_CPU_CYCLES:
- test_cycles_count(expect_count);
+ test_cycles_count(expect_count, false);
break;
}
}
static void test_basic_pmu_functionality(void)
{
+ local_irq_disable();
+ gic_init(GIC_V3, 1, (void *)GICD_BASE_GPA, (void *)GICR_BASE_GPA);
+ gic_irq_enable(PMU_IRQ);
+ local_irq_enable();
+
/* Test events on generic and cycle counters */
- test_instructions_count(0, true);
- test_cycles_count(true);
+ test_instructions_count(0, true, false);
+ test_cycles_count(true, false);
+
+ /* Test overflow with interrupts on generic and cycle counters */
+ test_instructions_count(0, true, true);
+ test_cycles_count(true, true);
}
/*
@@ -813,9 +988,6 @@ static void guest_code(void)
GUEST_DONE();
}
-#define GICD_BASE_GPA 0x8000000ULL
-#define GICR_BASE_GPA 0x80A0000ULL
-
static unsigned long *
set_event_filters(struct kvm_vcpu *vcpu, struct kvm_pmu_event_filter *pmu_event_filters)
{
@@ -866,7 +1038,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
struct kvm_vcpu *vcpu;
struct kvm_vcpu_init init;
uint8_t pmuver, ec;
- uint64_t dfr0, irq = 23;
+ uint64_t dfr0, irq = PMU_IRQ;
struct vpmu_vm *vpmu_vm;
struct kvm_device_attr irq_attr = {
.group = KVM_ARM_VCPU_PMU_V3_CTRL,
@@ -883,6 +1055,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
vpmu_vm->vm = vm = vm_create(1);
vm_init_descriptor_tables(vm);
+ vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
/* Catch exceptions for easier debugging */
for (ec = 0; ec < ESR_EC_NUM; ec++) {
--
2.39.1.581.gbfd45094c4-googHi Raghu,
On Tue, Feb 14, 2023 at 5:07 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
>
> Extend the vCPU migration test to also validate the vPMU's
> functionality when set up for overflow conditions.
>
> Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> ---
> .../testing/selftests/kvm/aarch64/vpmu_test.c | 223 ++++++++++++++++--
> 1 file changed, 198 insertions(+), 25 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_test.c b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> index 0c9d801f4e602..066dc17fa3906 100644
> --- a/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> +++ b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> @@ -21,7 +21,9 @@
> *
> * 4. Since the PMU registers are per-cpu, stress KVM by frequently
> * migrating the guest vCPU to random pCPUs in the system, and check
> - * if the vPMU is still behaving as expected.
> + * if the vPMU is still behaving as expected. The sub-tests include
> + * testing basic functionalities such as basic counters behavior,
> + * overflow, and overflow interrupts.
> *
> * Copyright (c) 2022 Google LLC.
> *
> @@ -41,13 +43,27 @@
> #include <sys/sysinfo.h>
>
> #include "delay.h"
> +#include "gic.h"
> +#include "spinlock.h"
>
> /* The max number of the PMU event counters (excluding the cycle counter) */
> #define ARMV8_PMU_MAX_GENERAL_COUNTERS (ARMV8_PMU_MAX_COUNTERS - 1)
>
> +/* The cycle counter bit position that's common among the PMU registers */
> +#define ARMV8_PMU_CYCLE_COUNTER_IDX 31
> +
> /* The max number of event numbers that's supported */
> #define ARMV8_PMU_MAX_EVENTS 64
>
> +#define PMU_IRQ 23
> +
> +#define COUNT_TO_OVERFLOW 0xFULL
> +#define PRE_OVERFLOW_32 (GENMASK(31, 0) - COUNT_TO_OVERFLOW + 1)
> +#define PRE_OVERFLOW_64 (GENMASK(63, 0) - COUNT_TO_OVERFLOW + 1)
> +
> +#define GICD_BASE_GPA 0x8000000ULL
> +#define GICR_BASE_GPA 0x80A0000ULL
> +
> #define msecs_to_usecs(msec) ((msec) * 1000LL)
>
> /*
> @@ -162,6 +178,17 @@ static inline void write_sel_evtyper(int sel, unsigned long val)
> isb();
> }
>
> +static inline void write_pmovsclr(unsigned long val)
> +{
> + write_sysreg(val, pmovsclr_el0);
> + isb();
> +}
> +
> +static unsigned long read_pmovsclr(void)
> +{
> + return read_sysreg(pmovsclr_el0);
> +}
> +
> static inline void enable_counter(int idx)
> {
> uint64_t v = read_sysreg(pmcntenset_el0);
> @@ -178,11 +205,33 @@ static inline void disable_counter(int idx)
> isb();
> }
>
> +static inline void enable_irq(int idx)
> +{
> + uint64_t v = read_sysreg(pmcntenset_el0);
> +
> + write_sysreg(BIT(idx) | v, pmintenset_el1);
> + isb();
> +}
> +
> +static inline void disable_irq(int idx)
> +{
> + uint64_t v = read_sysreg(pmcntenset_el0);
> +
> + write_sysreg(BIT(idx) | v, pmintenclr_el1);
> + isb();
> +}
> +
> static inline uint64_t read_cycle_counter(void)
> {
> return read_sysreg(pmccntr_el0);
> }
>
> +static inline void write_cycle_counter(uint64_t v)
> +{
> + write_sysreg(v, pmccntr_el0);
> + isb();
> +}
> +
> static inline void reset_cycle_counter(void)
> {
> uint64_t v = read_sysreg(pmcr_el0);
> @@ -289,6 +338,15 @@ struct guest_data {
>
> static struct guest_data guest_data;
>
> +/* Data to communicate among guest threads */
> +struct guest_irq_data {
> + uint32_t pmc_idx_bmap;
> + uint32_t irq_received_bmap;
> + struct spinlock lock;
> +};
> +
> +static struct guest_irq_data guest_irq_data;
> +
> #define VCPU_MIGRATIONS_TEST_ITERS_DEF 1000
> #define VCPU_MIGRATIONS_TEST_MIGRATION_FREQ_MS 2
>
> @@ -322,6 +380,79 @@ static void guest_sync_handler(struct ex_regs *regs)
> expected_ec = INVALID_EC;
> }
>
> +static void guest_validate_irq(int pmc_idx, uint32_t pmovsclr, uint32_t pmc_idx_bmap)
Can you please add a comment about what is pmc_idx_bmap ?
> +{
> + /*
> + * Fail if there's an interrupt from unexpected PMCs.
> + * All the expected events' IRQs may not arrive at the same time.
> + * Hence, check if the interrupt is valid only if it's expected.
> + */
> + if (pmovsclr & BIT(pmc_idx)) {
> + GUEST_ASSERT_3(pmc_idx_bmap & BIT(pmc_idx), pmc_idx, pmovsclr, pmc_idx_bmap);
> + write_pmovsclr(BIT(pmc_idx));
> + }
> +}
> +
> +static void guest_irq_handler(struct ex_regs *regs)
> +{
> + uint32_t pmc_idx_bmap;
> + uint64_t i, pmcr_n = get_pmcr_n();
> + uint32_t pmovsclr = read_pmovsclr();
> + unsigned int intid = gic_get_and_ack_irq();
> +
> + /* No other IRQ apart from the PMU IRQ is expected */
> + GUEST_ASSERT_1(intid == PMU_IRQ, intid);
> +
> + spin_lock(&guest_irq_data.lock);
Could you explain why this lock is required in this patch ??
If this is used to serialize the interrupt context code and
the normal (non-interrupt) context code, you might want to
disable the IRQ ? Using the spin lock won't work well for
that if the interrupt handler is invoked while the normal
context code grabs the lock.
Having said that, since execute_precise_instrs() disables the PMU
via PMCR, and does isb after that, I don't think the overflow
interrupt is delivered while the normal context code is in
pmu_irq_*() anyway.
> + pmc_idx_bmap = READ_ONCE(guest_irq_data.pmc_idx_bmap);
> +
> + for (i = 0; i < pmcr_n; i++)
> + guest_validate_irq(i, pmovsclr, pmc_idx_bmap);
> + guest_validate_irq(ARMV8_PMU_CYCLE_COUNTER_IDX, pmovsclr, pmc_idx_bmap);
> +
> + /* Mark IRQ as recived for the corresponding PMCs */
> + WRITE_ONCE(guest_irq_data.irq_received_bmap, pmovsclr);
> + spin_unlock(&guest_irq_data.lock);
> +
> + gic_set_eoi(intid);
> +}
> +
> +static int pmu_irq_received(int pmc_idx)
> +{
> + bool irq_received;
> +
> + spin_lock(&guest_irq_data.lock);
> + irq_received = READ_ONCE(guest_irq_data.irq_received_bmap) & BIT(pmc_idx);
> + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> + spin_unlock(&guest_irq_data.lock);
> +
> + return irq_received;
> +}
> +
> +static void pmu_irq_init(int pmc_idx)
> +{
> + write_pmovsclr(BIT(pmc_idx));
> +
> + spin_lock(&guest_irq_data.lock);
> + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> + WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap | BIT(pmc_idx));
> + spin_unlock(&guest_irq_data.lock);
> +
> + enable_irq(pmc_idx);
> +}
> +
> +static void pmu_irq_exit(int pmc_idx)
> +{
> + write_pmovsclr(BIT(pmc_idx));
> +
> + spin_lock(&guest_irq_data.lock);
> + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> + WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> + spin_unlock(&guest_irq_data.lock);
> +
> + disable_irq(pmc_idx);
> +}
> +
> /*
> * Run the given operation that should trigger an exception with the
> * given exception class. The exception handler (guest_sync_handler)
> @@ -420,12 +551,20 @@ static void execute_precise_instrs(int num, uint32_t pmcr)
> precise_instrs_loop(loop, pmcr);
> }
>
> -static void test_instructions_count(int pmc_idx, bool expect_count)
> +static void test_instructions_count(int pmc_idx, bool expect_count, bool test_overflow)
> {
> int i;
> struct pmc_accessor *acc;
> - uint64_t cnt;
> - int instrs_count = 100;
> + uint64_t cntr_val = 0;
> + int instrs_count = 500;
Can we set instrs_count based on the value we set for cntr_val?
(so that instrs_count can be adjusted automatically when we change the
value of cntr_val ?)
> +
> + if (test_overflow) {
> + /* Overflow scenarios can only be tested when a count is expected */
> + GUEST_ASSERT_1(expect_count, pmc_idx);
> +
> + cntr_val = PRE_OVERFLOW_32;
> + pmu_irq_init(pmc_idx);
> + }
>
> enable_counter(pmc_idx);
>
> @@ -433,41 +572,68 @@ static void test_instructions_count(int pmc_idx, bool expect_count)
> for (i = 0; i < ARRAY_SIZE(pmc_accessors); i++) {
> acc = &pmc_accessors[i];
>
> - pmu_disable_reset();
> -
> + acc->write_cntr(pmc_idx, cntr_val);
> acc->write_typer(pmc_idx, ARMV8_PMUV3_PERFCTR_INST_RETIRED);
>
> - /* Enable the PMU and execute precisely number of instructions as a workload */
> - execute_precise_instrs(instrs_count, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
> + /*
> + * Enable the PMU and execute a precise number of instructions as a workload.
> + * Since execute_precise_instrs() disables the PMU at the end, 'instrs_count'
> + * should have enough instructions to raise an IRQ.
> + */
> + execute_precise_instrs(instrs_count, ARMV8_PMU_PMCR_E);
>
> - /* If a count is expected, the counter should be increased by 'instrs_count' */
> - cnt = acc->read_cntr(pmc_idx);
> - GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
> - i, expect_count, cnt, instrs_count);
> + /*
> + * If an overflow is expected, only check for the overflag flag.
> + * As overflow interrupt is enabled, the interrupt would add additional
> + * instructions and mess up the precise instruction count. Hence, measure
> + * the instructions count only when the test is not set up for an overflow.
> + */
> + if (test_overflow) {
> + GUEST_ASSERT_2(pmu_irq_received(pmc_idx), pmc_idx, i);
> + } else {
> + uint64_t cnt = acc->read_cntr(pmc_idx);
> +
> + GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
> + pmc_idx, i, cnt, expect_count);
> + }
> }
>
> - disable_counter(pmc_idx);
> + if (test_overflow)
> + pmu_irq_exit(pmc_idx);
> }
>
> -static void test_cycles_count(bool expect_count)
> +static void test_cycles_count(bool expect_count, bool test_overflow)
> {
> uint64_t cnt;
>
> - pmu_enable();
> - reset_cycle_counter();
> + if (test_overflow) {
> + /* Overflow scenarios can only be tested when a count is expected */
> + GUEST_ASSERT(expect_count);
> +
> + write_cycle_counter(PRE_OVERFLOW_64);
> + pmu_irq_init(ARMV8_PMU_CYCLE_COUNTER_IDX);
> + } else {
> + reset_cycle_counter();
> + }
>
> /* Count cycles in EL0 and EL1 */
> write_pmccfiltr(0);
> enable_cycle_counter();
>
> + /* Enable the PMU and execute precisely number of instructions as a workload */
Can you please add a comment why we do this (500 times) iterations ?
Can we set the iteration number based on the initial value of the
cycle counter ?
> + execute_precise_instrs(500, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
> cnt = read_cycle_counter();
>
> /*
> * If a count is expected by the test, the cycle counter should be increased by
> - * at least 1, as there is at least one instruction between enabling the
> + * at least 1, as there are a number of instructions between enabling the
> * counter and reading the counter.
> */
"at least 1" doesn't seem to be consistent with the GUEST_ASSERT_2 below
when test_overflow is true, considering the initial value of the cycle counter.
Shouldn't this GUEST_ASSERT_2 be executed only if test_overflow is false ?
(Or do you want to adjust the comment ?)
> GUEST_ASSERT_2(expect_count == (cnt > 0), cnt, expect_count);
> + if (test_overflow) {
> + GUEST_ASSERT_2(pmu_irq_received(ARMV8_PMU_CYCLE_COUNTER_IDX), cnt, expect_count);
> + pmu_irq_exit(ARMV8_PMU_CYCLE_COUNTER_IDX);
> + }
>
> disable_cycle_counter();
> pmu_disable_reset();
> @@ -477,19 +643,28 @@ static void test_event_count(uint64_t event, int pmc_idx, bool expect_count)
> {
> switch (event) {
> case ARMV8_PMUV3_PERFCTR_INST_RETIRED:
> - test_instructions_count(pmc_idx, expect_count);
> + test_instructions_count(pmc_idx, expect_count, false);
> break;
> case ARMV8_PMUV3_PERFCTR_CPU_CYCLES:
> - test_cycles_count(expect_count);
> + test_cycles_count(expect_count, false);
> break;
> }
> }
>
> static void test_basic_pmu_functionality(void)
> {
> + local_irq_disable();
> + gic_init(GIC_V3, 1, (void *)GICD_BASE_GPA, (void *)GICR_BASE_GPA);
> + gic_irq_enable(PMU_IRQ);
> + local_irq_enable();
> +
> /* Test events on generic and cycle counters */
> - test_instructions_count(0, true);
> - test_cycles_count(true);
> + test_instructions_count(0, true, false);
> + test_cycles_count(true, false);
> +
> + /* Test overflow with interrupts on generic and cycle counters */
> + test_instructions_count(0, true, true);
> + test_cycles_count(true, true);
> }
>
> /*
> @@ -813,9 +988,6 @@ static void guest_code(void)
> GUEST_DONE();
> }
>
> -#define GICD_BASE_GPA 0x8000000ULL
> -#define GICR_BASE_GPA 0x80A0000ULL
> -
> static unsigned long *
> set_event_filters(struct kvm_vcpu *vcpu, struct kvm_pmu_event_filter *pmu_event_filters)
> {
> @@ -866,7 +1038,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
> struct kvm_vcpu *vcpu;
> struct kvm_vcpu_init init;
> uint8_t pmuver, ec;
> - uint64_t dfr0, irq = 23;
> + uint64_t dfr0, irq = PMU_IRQ;
> struct vpmu_vm *vpmu_vm;
> struct kvm_device_attr irq_attr = {
> .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> @@ -883,6 +1055,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
>
> vpmu_vm->vm = vm = vm_create(1);
> vm_init_descriptor_tables(vm);
> + vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
>
> /* Catch exceptions for easier debugging */
> for (ec = 0; ec < ESR_EC_NUM; ec++) {
> --
> 2.39.1.581.gbfd45094c4-goog
>
Thanks,
Reiji
Hi Reiji,
On Mon, Mar 6, 2023 at 10:10 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Raghu,
>
> On Tue, Feb 14, 2023 at 5:07 PM Raghavendra Rao Ananta
> <rananta@google.com> wrote:
> >
> > Extend the vCPU migration test to also validate the vPMU's
> > functionality when set up for overflow conditions.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > ---
> > .../testing/selftests/kvm/aarch64/vpmu_test.c | 223 ++++++++++++++++--
> > 1 file changed, 198 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_test.c b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > index 0c9d801f4e602..066dc17fa3906 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > @@ -21,7 +21,9 @@
> > *
> > * 4. Since the PMU registers are per-cpu, stress KVM by frequently
> > * migrating the guest vCPU to random pCPUs in the system, and check
> > - * if the vPMU is still behaving as expected.
> > + * if the vPMU is still behaving as expected. The sub-tests include
> > + * testing basic functionalities such as basic counters behavior,
> > + * overflow, and overflow interrupts.
> > *
> > * Copyright (c) 2022 Google LLC.
> > *
> > @@ -41,13 +43,27 @@
> > #include <sys/sysinfo.h>
> >
> > #include "delay.h"
> > +#include "gic.h"
> > +#include "spinlock.h"
> >
> > /* The max number of the PMU event counters (excluding the cycle counter) */
> > #define ARMV8_PMU_MAX_GENERAL_COUNTERS (ARMV8_PMU_MAX_COUNTERS - 1)
> >
> > +/* The cycle counter bit position that's common among the PMU registers */
> > +#define ARMV8_PMU_CYCLE_COUNTER_IDX 31
> > +
> > /* The max number of event numbers that's supported */
> > #define ARMV8_PMU_MAX_EVENTS 64
> >
> > +#define PMU_IRQ 23
> > +
> > +#define COUNT_TO_OVERFLOW 0xFULL
> > +#define PRE_OVERFLOW_32 (GENMASK(31, 0) - COUNT_TO_OVERFLOW + 1)
> > +#define PRE_OVERFLOW_64 (GENMASK(63, 0) - COUNT_TO_OVERFLOW + 1)
> > +
> > +#define GICD_BASE_GPA 0x8000000ULL
> > +#define GICR_BASE_GPA 0x80A0000ULL
> > +
> > #define msecs_to_usecs(msec) ((msec) * 1000LL)
> >
> > /*
> > @@ -162,6 +178,17 @@ static inline void write_sel_evtyper(int sel, unsigned long val)
> > isb();
> > }
> >
> > +static inline void write_pmovsclr(unsigned long val)
> > +{
> > + write_sysreg(val, pmovsclr_el0);
> > + isb();
> > +}
> > +
> > +static unsigned long read_pmovsclr(void)
> > +{
> > + return read_sysreg(pmovsclr_el0);
> > +}
> > +
> > static inline void enable_counter(int idx)
> > {
> > uint64_t v = read_sysreg(pmcntenset_el0);
> > @@ -178,11 +205,33 @@ static inline void disable_counter(int idx)
> > isb();
> > }
> >
> > +static inline void enable_irq(int idx)
> > +{
> > + uint64_t v = read_sysreg(pmcntenset_el0);
> > +
> > + write_sysreg(BIT(idx) | v, pmintenset_el1);
> > + isb();
> > +}
> > +
> > +static inline void disable_irq(int idx)
> > +{
> > + uint64_t v = read_sysreg(pmcntenset_el0);
> > +
> > + write_sysreg(BIT(idx) | v, pmintenclr_el1);
> > + isb();
> > +}
> > +
> > static inline uint64_t read_cycle_counter(void)
> > {
> > return read_sysreg(pmccntr_el0);
> > }
> >
> > +static inline void write_cycle_counter(uint64_t v)
> > +{
> > + write_sysreg(v, pmccntr_el0);
> > + isb();
> > +}
> > +
> > static inline void reset_cycle_counter(void)
> > {
> > uint64_t v = read_sysreg(pmcr_el0);
> > @@ -289,6 +338,15 @@ struct guest_data {
> >
> > static struct guest_data guest_data;
> >
> > +/* Data to communicate among guest threads */
> > +struct guest_irq_data {
> > + uint32_t pmc_idx_bmap;
> > + uint32_t irq_received_bmap;
> > + struct spinlock lock;
> > +};
> > +
> > +static struct guest_irq_data guest_irq_data;
> > +
> > #define VCPU_MIGRATIONS_TEST_ITERS_DEF 1000
> > #define VCPU_MIGRATIONS_TEST_MIGRATION_FREQ_MS 2
> >
> > @@ -322,6 +380,79 @@ static void guest_sync_handler(struct ex_regs *regs)
> > expected_ec = INVALID_EC;
> > }
> >
> > +static void guest_validate_irq(int pmc_idx, uint32_t pmovsclr, uint32_t pmc_idx_bmap)
>
> Can you please add a comment about what is pmc_idx_bmap ?
>
Of course! Now that I see, it's not that clear. It's actually the
bitmap of the PMC(s) that we should expect an interrupt from. I'll a
comment in v2.
>
> > +{
> > + /*
> > + * Fail if there's an interrupt from unexpected PMCs.
> > + * All the expected events' IRQs may not arrive at the same time.
> > + * Hence, check if the interrupt is valid only if it's expected.
> > + */
> > + if (pmovsclr & BIT(pmc_idx)) {
> > + GUEST_ASSERT_3(pmc_idx_bmap & BIT(pmc_idx), pmc_idx, pmovsclr, pmc_idx_bmap);
> > + write_pmovsclr(BIT(pmc_idx));
> > + }
> > +}
> > +
> > +static void guest_irq_handler(struct ex_regs *regs)
> > +{
> > + uint32_t pmc_idx_bmap;
> > + uint64_t i, pmcr_n = get_pmcr_n();
> > + uint32_t pmovsclr = read_pmovsclr();
> > + unsigned int intid = gic_get_and_ack_irq();
> > +
> > + /* No other IRQ apart from the PMU IRQ is expected */
> > + GUEST_ASSERT_1(intid == PMU_IRQ, intid);
> > +
> > + spin_lock(&guest_irq_data.lock);
>
> Could you explain why this lock is required in this patch ??
> If this is used to serialize the interrupt context code and
> the normal (non-interrupt) context code, you might want to
> disable the IRQ ? Using the spin lock won't work well for
> that if the interrupt handler is invoked while the normal
> context code grabs the lock.
> Having said that, since execute_precise_instrs() disables the PMU
> via PMCR, and does isb after that, I don't think the overflow
> interrupt is delivered while the normal context code is in
> pmu_irq_*() anyway.
>
I think you are right. At least in the current state of the patch, we
don't need this lock, nor do we explicitly have to enable/disable IRQs
to deal with a race. I've checked further patches as well, and even in
the case of multi-vCPU config, we wouldn't need it as the
guest_irq_data is per-cpu.
(Probably I introduced it by forward-thinking things). Thanks for
catching this. I'll remove it in v2.
> > + pmc_idx_bmap = READ_ONCE(guest_irq_data.pmc_idx_bmap);
> > +
> > + for (i = 0; i < pmcr_n; i++)
> > + guest_validate_irq(i, pmovsclr, pmc_idx_bmap);
> > + guest_validate_irq(ARMV8_PMU_CYCLE_COUNTER_IDX, pmovsclr, pmc_idx_bmap);
> > +
> > + /* Mark IRQ as recived for the corresponding PMCs */
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, pmovsclr);
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + gic_set_eoi(intid);
> > +}
> > +
> > +static int pmu_irq_received(int pmc_idx)
> > +{
> > + bool irq_received;
> > +
> > + spin_lock(&guest_irq_data.lock);
> > + irq_received = READ_ONCE(guest_irq_data.irq_received_bmap) & BIT(pmc_idx);
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + return irq_received;
> > +}
> > +
> > +static void pmu_irq_init(int pmc_idx)
> > +{
> > + write_pmovsclr(BIT(pmc_idx));
> > +
> > + spin_lock(&guest_irq_data.lock);
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap | BIT(pmc_idx));
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + enable_irq(pmc_idx);
> > +}
> > +
> > +static void pmu_irq_exit(int pmc_idx)
> > +{
> > + write_pmovsclr(BIT(pmc_idx));
> > +
> > + spin_lock(&guest_irq_data.lock);
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + disable_irq(pmc_idx);
> > +}
> > +
> > /*
> > * Run the given operation that should trigger an exception with the
> > * given exception class. The exception handler (guest_sync_handler)
> > @@ -420,12 +551,20 @@ static void execute_precise_instrs(int num, uint32_t pmcr)
> > precise_instrs_loop(loop, pmcr);
> > }
> >
> > -static void test_instructions_count(int pmc_idx, bool expect_count)
> > +static void test_instructions_count(int pmc_idx, bool expect_count, bool test_overflow)
> > {
> > int i;
> > struct pmc_accessor *acc;
> > - uint64_t cnt;
> > - int instrs_count = 100;
> > + uint64_t cntr_val = 0;
> > + int instrs_count = 500;
>
> Can we set instrs_count based on the value we set for cntr_val?
> (so that instrs_count can be adjusted automatically when we change the
> value of cntr_val ?)
>
Sure, I can do that to keep things safe.
> > +
> > + if (test_overflow) {
> > + /* Overflow scenarios can only be tested when a count is expected */
> > + GUEST_ASSERT_1(expect_count, pmc_idx);
> > +
> > + cntr_val = PRE_OVERFLOW_32;
> > + pmu_irq_init(pmc_idx);
> > + }
> >
> > enable_counter(pmc_idx);
> >
> > @@ -433,41 +572,68 @@ static void test_instructions_count(int pmc_idx, bool expect_count)
> > for (i = 0; i < ARRAY_SIZE(pmc_accessors); i++) {
> > acc = &pmc_accessors[i];
> >
> > - pmu_disable_reset();
> > -
> > + acc->write_cntr(pmc_idx, cntr_val);
> > acc->write_typer(pmc_idx, ARMV8_PMUV3_PERFCTR_INST_RETIRED);
> >
> > - /* Enable the PMU and execute precisely number of instructions as a workload */
> > - execute_precise_instrs(instrs_count, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
> > + /*
> > + * Enable the PMU and execute a precise number of instructions as a workload.
> > + * Since execute_precise_instrs() disables the PMU at the end, 'instrs_count'
> > + * should have enough instructions to raise an IRQ.
> > + */
> > + execute_precise_instrs(instrs_count, ARMV8_PMU_PMCR_E);
> >
> > - /* If a count is expected, the counter should be increased by 'instrs_count' */
> > - cnt = acc->read_cntr(pmc_idx);
> > - GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
> > - i, expect_count, cnt, instrs_count);
> > + /*
> > + * If an overflow is expected, only check for the overflag flag.
> > + * As overflow interrupt is enabled, the interrupt would add additional
> > + * instructions and mess up the precise instruction count. Hence, measure
> > + * the instructions count only when the test is not set up for an overflow.
> > + */
> > + if (test_overflow) {
> > + GUEST_ASSERT_2(pmu_irq_received(pmc_idx), pmc_idx, i);
> > + } else {
> > + uint64_t cnt = acc->read_cntr(pmc_idx);
> > +
> > + GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
> > + pmc_idx, i, cnt, expect_count);
> > + }
> > }
> >
> > - disable_counter(pmc_idx);
> > + if (test_overflow)
> > + pmu_irq_exit(pmc_idx);
> > }
> >
> > -static void test_cycles_count(bool expect_count)
> > +static void test_cycles_count(bool expect_count, bool test_overflow)
> > {
> > uint64_t cnt;
> >
> > - pmu_enable();
> > - reset_cycle_counter();
> > + if (test_overflow) {
> > + /* Overflow scenarios can only be tested when a count is expected */
> > + GUEST_ASSERT(expect_count);
> > +
> > + write_cycle_counter(PRE_OVERFLOW_64);
> > + pmu_irq_init(ARMV8_PMU_CYCLE_COUNTER_IDX);
> > + } else {
> > + reset_cycle_counter();
> > + }
> >
> > /* Count cycles in EL0 and EL1 */
> > write_pmccfiltr(0);
> > enable_cycle_counter();
> >
> > + /* Enable the PMU and execute precisely number of instructions as a workload */
>
> Can you please add a comment why we do this (500 times) iterations ?
> Can we set the iteration number based on the initial value of the
> cycle counter ?
>
I believe I have a comment explaining it in the upcoming patches.
Should've had it on this
one though. I'll move it in v2.
> > + execute_precise_instrs(500, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
> > cnt = read_cycle_counter();
> >
> > /*
> > * If a count is expected by the test, the cycle counter should be increased by
> > - * at least 1, as there is at least one instruction between enabling the
> > + * at least 1, as there are a number of instructions between enabling the
> > * counter and reading the counter.
> > */
>
> "at least 1" doesn't seem to be consistent with the GUEST_ASSERT_2 below
> when test_overflow is true, considering the initial value of the cycle counter.
> Shouldn't this GUEST_ASSERT_2 be executed only if test_overflow is false ?
> (Or do you want to adjust the comment ?)
>
Yes, I may have to tweak the comment to make things clear.
> > GUEST_ASSERT_2(expect_count == (cnt > 0), cnt, expect_count);
> > + if (test_overflow) {
> > + GUEST_ASSERT_2(pmu_irq_received(ARMV8_PMU_CYCLE_COUNTER_IDX), cnt, expect_count);
> > + pmu_irq_exit(ARMV8_PMU_CYCLE_COUNTER_IDX);
> > + }
> >
> > disable_cycle_counter();
> > pmu_disable_reset();
> > @@ -477,19 +643,28 @@ static void test_event_count(uint64_t event, int pmc_idx, bool expect_count)
> > {
> > switch (event) {
> > case ARMV8_PMUV3_PERFCTR_INST_RETIRED:
> > - test_instructions_count(pmc_idx, expect_count);
> > + test_instructions_count(pmc_idx, expect_count, false);
> > break;
> > case ARMV8_PMUV3_PERFCTR_CPU_CYCLES:
> > - test_cycles_count(expect_count);
> > + test_cycles_count(expect_count, false);
> > break;
> > }
> > }
> >
> > static void test_basic_pmu_functionality(void)
> > {
> > + local_irq_disable();
> > + gic_init(GIC_V3, 1, (void *)GICD_BASE_GPA, (void *)GICR_BASE_GPA);
> > + gic_irq_enable(PMU_IRQ);
> > + local_irq_enable();
> > +
> > /* Test events on generic and cycle counters */
> > - test_instructions_count(0, true);
> > - test_cycles_count(true);
> > + test_instructions_count(0, true, false);
> > + test_cycles_count(true, false);
> > +
> > + /* Test overflow with interrupts on generic and cycle counters */
> > + test_instructions_count(0, true, true);
> > + test_cycles_count(true, true);
> > }
> >
> > /*
> > @@ -813,9 +988,6 @@ static void guest_code(void)
> > GUEST_DONE();
> > }
> >
> > -#define GICD_BASE_GPA 0x8000000ULL
> > -#define GICR_BASE_GPA 0x80A0000ULL
> > -
> > static unsigned long *
> > set_event_filters(struct kvm_vcpu *vcpu, struct kvm_pmu_event_filter *pmu_event_filters)
> > {
> > @@ -866,7 +1038,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
> > struct kvm_vcpu *vcpu;
> > struct kvm_vcpu_init init;
> > uint8_t pmuver, ec;
> > - uint64_t dfr0, irq = 23;
> > + uint64_t dfr0, irq = PMU_IRQ;
> > struct vpmu_vm *vpmu_vm;
> > struct kvm_device_attr irq_attr = {
> > .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> > @@ -883,6 +1055,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
> >
> > vpmu_vm->vm = vm = vm_create(1);
> > vm_init_descriptor_tables(vm);
> > + vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
> >
> > /* Catch exceptions for easier debugging */
> > for (ec = 0; ec < ESR_EC_NUM; ec++) {
> > --
> > 2.39.1.581.gbfd45094c4-goog
> >
>
> Thanks,
> Reiji
>
On Mon, Mar 6, 2023 at 10:09 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Raghu,
>
> On Tue, Feb 14, 2023 at 5:07 PM Raghavendra Rao Ananta
> <rananta@google.com> wrote:
> >
> > Extend the vCPU migration test to also validate the vPMU's
> > functionality when set up for overflow conditions.
> >
> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > ---
> > .../testing/selftests/kvm/aarch64/vpmu_test.c | 223 ++++++++++++++++--
> > 1 file changed, 198 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vpmu_test.c b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > index 0c9d801f4e602..066dc17fa3906 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vpmu_test.c
> > @@ -21,7 +21,9 @@
> > *
> > * 4. Since the PMU registers are per-cpu, stress KVM by frequently
> > * migrating the guest vCPU to random pCPUs in the system, and check
> > - * if the vPMU is still behaving as expected.
> > + * if the vPMU is still behaving as expected. The sub-tests include
> > + * testing basic functionalities such as basic counters behavior,
> > + * overflow, and overflow interrupts.
> > *
> > * Copyright (c) 2022 Google LLC.
> > *
> > @@ -41,13 +43,27 @@
> > #include <sys/sysinfo.h>
> >
> > #include "delay.h"
> > +#include "gic.h"
> > +#include "spinlock.h"
> >
> > /* The max number of the PMU event counters (excluding the cycle counter) */
> > #define ARMV8_PMU_MAX_GENERAL_COUNTERS (ARMV8_PMU_MAX_COUNTERS - 1)
> >
> > +/* The cycle counter bit position that's common among the PMU registers */
> > +#define ARMV8_PMU_CYCLE_COUNTER_IDX 31
> > +
> > /* The max number of event numbers that's supported */
> > #define ARMV8_PMU_MAX_EVENTS 64
> >
> > +#define PMU_IRQ 23
> > +
> > +#define COUNT_TO_OVERFLOW 0xFULL
> > +#define PRE_OVERFLOW_32 (GENMASK(31, 0) - COUNT_TO_OVERFLOW + 1)
> > +#define PRE_OVERFLOW_64 (GENMASK(63, 0) - COUNT_TO_OVERFLOW + 1)
Reset values of PMCR_EL0.LP and PMCR_EL0.LC are UNKNOWN.
As the test seems to expect a 64-bit overflow on the cycle counter
and a 32-bit overflow on the other counters, the guest code should
explicitly clear PMCR_EL0.LP and set PMCR_EL0.LC, before the test.
Thanks,
Reiji
> > +
> > +#define GICD_BASE_GPA 0x8000000ULL
> > +#define GICR_BASE_GPA 0x80A0000ULL
> > +
> > #define msecs_to_usecs(msec) ((msec) * 1000LL)
> >
> > /*
> > @@ -162,6 +178,17 @@ static inline void write_sel_evtyper(int sel, unsigned long val)
> > isb();
> > }
> >
> > +static inline void write_pmovsclr(unsigned long val)
> > +{
> > + write_sysreg(val, pmovsclr_el0);
> > + isb();
> > +}
> > +
> > +static unsigned long read_pmovsclr(void)
> > +{
> > + return read_sysreg(pmovsclr_el0);
> > +}
> > +
> > static inline void enable_counter(int idx)
> > {
> > uint64_t v = read_sysreg(pmcntenset_el0);
> > @@ -178,11 +205,33 @@ static inline void disable_counter(int idx)
> > isb();
> > }
> >
> > +static inline void enable_irq(int idx)
> > +{
> > + uint64_t v = read_sysreg(pmcntenset_el0);
> > +
> > + write_sysreg(BIT(idx) | v, pmintenset_el1);
> > + isb();
> > +}
> > +
> > +static inline void disable_irq(int idx)
> > +{
> > + uint64_t v = read_sysreg(pmcntenset_el0);
> > +
> > + write_sysreg(BIT(idx) | v, pmintenclr_el1);
> > + isb();
> > +}
> > +
> > static inline uint64_t read_cycle_counter(void)
> > {
> > return read_sysreg(pmccntr_el0);
> > }
> >
> > +static inline void write_cycle_counter(uint64_t v)
> > +{
> > + write_sysreg(v, pmccntr_el0);
> > + isb();
> > +}
> > +
> > static inline void reset_cycle_counter(void)
> > {
> > uint64_t v = read_sysreg(pmcr_el0);
> > @@ -289,6 +338,15 @@ struct guest_data {
> >
> > static struct guest_data guest_data;
> >
> > +/* Data to communicate among guest threads */
> > +struct guest_irq_data {
> > + uint32_t pmc_idx_bmap;
> > + uint32_t irq_received_bmap;
> > + struct spinlock lock;
> > +};
> > +
> > +static struct guest_irq_data guest_irq_data;
> > +
> > #define VCPU_MIGRATIONS_TEST_ITERS_DEF 1000
> > #define VCPU_MIGRATIONS_TEST_MIGRATION_FREQ_MS 2
> >
> > @@ -322,6 +380,79 @@ static void guest_sync_handler(struct ex_regs *regs)
> > expected_ec = INVALID_EC;
> > }
> >
> > +static void guest_validate_irq(int pmc_idx, uint32_t pmovsclr, uint32_t pmc_idx_bmap)
>
> Can you please add a comment about what is pmc_idx_bmap ?
>
>
> > +{
> > + /*
> > + * Fail if there's an interrupt from unexpected PMCs.
> > + * All the expected events' IRQs may not arrive at the same time.
> > + * Hence, check if the interrupt is valid only if it's expected.
> > + */
> > + if (pmovsclr & BIT(pmc_idx)) {
> > + GUEST_ASSERT_3(pmc_idx_bmap & BIT(pmc_idx), pmc_idx, pmovsclr, pmc_idx_bmap);
> > + write_pmovsclr(BIT(pmc_idx));
> > + }
> > +}
> > +
> > +static void guest_irq_handler(struct ex_regs *regs)
> > +{
> > + uint32_t pmc_idx_bmap;
> > + uint64_t i, pmcr_n = get_pmcr_n();
> > + uint32_t pmovsclr = read_pmovsclr();
> > + unsigned int intid = gic_get_and_ack_irq();
> > +
> > + /* No other IRQ apart from the PMU IRQ is expected */
> > + GUEST_ASSERT_1(intid == PMU_IRQ, intid);
> > +
> > + spin_lock(&guest_irq_data.lock);
>
> Could you explain why this lock is required in this patch ??
> If this is used to serialize the interrupt context code and
> the normal (non-interrupt) context code, you might want to
> disable the IRQ ? Using the spin lock won't work well for
> that if the interrupt handler is invoked while the normal
> context code grabs the lock.
> Having said that, since execute_precise_instrs() disables the PMU
> via PMCR, and does isb after that, I don't think the overflow
> interrupt is delivered while the normal context code is in
> pmu_irq_*() anyway.
>
> > + pmc_idx_bmap = READ_ONCE(guest_irq_data.pmc_idx_bmap);
> > +
> > + for (i = 0; i < pmcr_n; i++)
> > + guest_validate_irq(i, pmovsclr, pmc_idx_bmap);
> > + guest_validate_irq(ARMV8_PMU_CYCLE_COUNTER_IDX, pmovsclr, pmc_idx_bmap);
> > +
> > + /* Mark IRQ as recived for the corresponding PMCs */
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, pmovsclr);
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + gic_set_eoi(intid);
> > +}
> > +
> > +static int pmu_irq_received(int pmc_idx)
> > +{
> > + bool irq_received;
> > +
> > + spin_lock(&guest_irq_data.lock);
> > + irq_received = READ_ONCE(guest_irq_data.irq_received_bmap) & BIT(pmc_idx);
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + return irq_received;
> > +}
> > +
> > +static void pmu_irq_init(int pmc_idx)
> > +{
> > + write_pmovsclr(BIT(pmc_idx));
> > +
> > + spin_lock(&guest_irq_data.lock);
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap | BIT(pmc_idx));
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + enable_irq(pmc_idx);
> > +}
> > +
> > +static void pmu_irq_exit(int pmc_idx)
> > +{
> > + write_pmovsclr(BIT(pmc_idx));
> > +
> > + spin_lock(&guest_irq_data.lock);
> > + WRITE_ONCE(guest_irq_data.irq_received_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + WRITE_ONCE(guest_irq_data.pmc_idx_bmap, guest_irq_data.pmc_idx_bmap & ~BIT(pmc_idx));
> > + spin_unlock(&guest_irq_data.lock);
> > +
> > + disable_irq(pmc_idx);
> > +}
> > +
> > /*
> > * Run the given operation that should trigger an exception with the
> > * given exception class. The exception handler (guest_sync_handler)
> > @@ -420,12 +551,20 @@ static void execute_precise_instrs(int num, uint32_t pmcr)
> > precise_instrs_loop(loop, pmcr);
> > }
> >
> > -static void test_instructions_count(int pmc_idx, bool expect_count)
> > +static void test_instructions_count(int pmc_idx, bool expect_count, bool test_overflow)
> > {
> > int i;
> > struct pmc_accessor *acc;
> > - uint64_t cnt;
> > - int instrs_count = 100;
> > + uint64_t cntr_val = 0;
> > + int instrs_count = 500;
>
> Can we set instrs_count based on the value we set for cntr_val?
> (so that instrs_count can be adjusted automatically when we change the
> value of cntr_val ?)
>
> > +
> > + if (test_overflow) {
> > + /* Overflow scenarios can only be tested when a count is expected */
> > + GUEST_ASSERT_1(expect_count, pmc_idx);
> > +
> > + cntr_val = PRE_OVERFLOW_32;
> > + pmu_irq_init(pmc_idx);
> > + }
> >
> > enable_counter(pmc_idx);
> >
> > @@ -433,41 +572,68 @@ static void test_instructions_count(int pmc_idx, bool expect_count)
> > for (i = 0; i < ARRAY_SIZE(pmc_accessors); i++) {
> > acc = &pmc_accessors[i];
> >
> > - pmu_disable_reset();
> > -
> > + acc->write_cntr(pmc_idx, cntr_val);
> > acc->write_typer(pmc_idx, ARMV8_PMUV3_PERFCTR_INST_RETIRED);
> >
> > - /* Enable the PMU and execute precisely number of instructions as a workload */
> > - execute_precise_instrs(instrs_count, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
> > + /*
> > + * Enable the PMU and execute a precise number of instructions as a workload.
> > + * Since execute_precise_instrs() disables the PMU at the end, 'instrs_count'
> > + * should have enough instructions to raise an IRQ.
> > + */
> > + execute_precise_instrs(instrs_count, ARMV8_PMU_PMCR_E);
> >
> > - /* If a count is expected, the counter should be increased by 'instrs_count' */
> > - cnt = acc->read_cntr(pmc_idx);
> > - GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
> > - i, expect_count, cnt, instrs_count);
> > + /*
> > + * If an overflow is expected, only check for the overflag flag.
> > + * As overflow interrupt is enabled, the interrupt would add additional
> > + * instructions and mess up the precise instruction count. Hence, measure
> > + * the instructions count only when the test is not set up for an overflow.
> > + */
> > + if (test_overflow) {
> > + GUEST_ASSERT_2(pmu_irq_received(pmc_idx), pmc_idx, i);
> > + } else {
> > + uint64_t cnt = acc->read_cntr(pmc_idx);
> > +
> > + GUEST_ASSERT_4(expect_count == (cnt == instrs_count),
> > + pmc_idx, i, cnt, expect_count);
> > + }
> > }
> >
> > - disable_counter(pmc_idx);
> > + if (test_overflow)
> > + pmu_irq_exit(pmc_idx);
> > }
> >
> > -static void test_cycles_count(bool expect_count)
> > +static void test_cycles_count(bool expect_count, bool test_overflow)
> > {
> > uint64_t cnt;
> >
> > - pmu_enable();
> > - reset_cycle_counter();
> > + if (test_overflow) {
> > + /* Overflow scenarios can only be tested when a count is expected */
> > + GUEST_ASSERT(expect_count);
> > +
> > + write_cycle_counter(PRE_OVERFLOW_64);
> > + pmu_irq_init(ARMV8_PMU_CYCLE_COUNTER_IDX);
> > + } else {
> > + reset_cycle_counter();
> > + }
> >
> > /* Count cycles in EL0 and EL1 */
> > write_pmccfiltr(0);
> > enable_cycle_counter();
> >
> > + /* Enable the PMU and execute precisely number of instructions as a workload */
>
> Can you please add a comment why we do this (500 times) iterations ?
> Can we set the iteration number based on the initial value of the
> cycle counter ?
>
> > + execute_precise_instrs(500, read_sysreg(pmcr_el0) | ARMV8_PMU_PMCR_E);
> > cnt = read_cycle_counter();
> >
> > /*
> > * If a count is expected by the test, the cycle counter should be increased by
> > - * at least 1, as there is at least one instruction between enabling the
> > + * at least 1, as there are a number of instructions between enabling the
> > * counter and reading the counter.
> > */
>
> "at least 1" doesn't seem to be consistent with the GUEST_ASSERT_2 below
> when test_overflow is true, considering the initial value of the cycle counter.
> Shouldn't this GUEST_ASSERT_2 be executed only if test_overflow is false ?
> (Or do you want to adjust the comment ?)
>
> > GUEST_ASSERT_2(expect_count == (cnt > 0), cnt, expect_count);
> > + if (test_overflow) {
> > + GUEST_ASSERT_2(pmu_irq_received(ARMV8_PMU_CYCLE_COUNTER_IDX), cnt, expect_count);
> > + pmu_irq_exit(ARMV8_PMU_CYCLE_COUNTER_IDX);
> > + }
> >
> > disable_cycle_counter();
> > pmu_disable_reset();
> > @@ -477,19 +643,28 @@ static void test_event_count(uint64_t event, int pmc_idx, bool expect_count)
> > {
> > switch (event) {
> > case ARMV8_PMUV3_PERFCTR_INST_RETIRED:
> > - test_instructions_count(pmc_idx, expect_count);
> > + test_instructions_count(pmc_idx, expect_count, false);
> > break;
> > case ARMV8_PMUV3_PERFCTR_CPU_CYCLES:
> > - test_cycles_count(expect_count);
> > + test_cycles_count(expect_count, false);
> > break;
> > }
> > }
> >
> > static void test_basic_pmu_functionality(void)
> > {
> > + local_irq_disable();
> > + gic_init(GIC_V3, 1, (void *)GICD_BASE_GPA, (void *)GICR_BASE_GPA);
> > + gic_irq_enable(PMU_IRQ);
> > + local_irq_enable();
> > +
> > /* Test events on generic and cycle counters */
> > - test_instructions_count(0, true);
> > - test_cycles_count(true);
> > + test_instructions_count(0, true, false);
> > + test_cycles_count(true, false);
> > +
> > + /* Test overflow with interrupts on generic and cycle counters */
> > + test_instructions_count(0, true, true);
> > + test_cycles_count(true, true);
> > }
> >
> > /*
> > @@ -813,9 +988,6 @@ static void guest_code(void)
> > GUEST_DONE();
> > }
> >
> > -#define GICD_BASE_GPA 0x8000000ULL
> > -#define GICR_BASE_GPA 0x80A0000ULL
> > -
> > static unsigned long *
> > set_event_filters(struct kvm_vcpu *vcpu, struct kvm_pmu_event_filter *pmu_event_filters)
> > {
> > @@ -866,7 +1038,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
> > struct kvm_vcpu *vcpu;
> > struct kvm_vcpu_init init;
> > uint8_t pmuver, ec;
> > - uint64_t dfr0, irq = 23;
> > + uint64_t dfr0, irq = PMU_IRQ;
> > struct vpmu_vm *vpmu_vm;
> > struct kvm_device_attr irq_attr = {
> > .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> > @@ -883,6 +1055,7 @@ create_vpmu_vm(void *guest_code, struct kvm_pmu_event_filter *pmu_event_filters)
> >
> > vpmu_vm->vm = vm = vm_create(1);
> > vm_init_descriptor_tables(vm);
> > + vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);
> >
> > /* Catch exceptions for easier debugging */
> > for (ec = 0; ec < ESR_EC_NUM; ec++) {
> > --
> > 2.39.1.581.gbfd45094c4-goog
> >
>
> Thanks,
> Reiji
© 2016 - 2025 Red Hat, Inc.