From: Ricardo Koller <ricarkol@google.com>
To: Eric Auger <eric.auger@redhat.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev,
andrew.jones@linux.dev, maz@kernel.org, alexandru.elisei@arm.com,
oliver.upton@linux.dev, reijiw@google.com
Subject: Re: [kvm-unit-tests PATCH v3 3/4] arm: pmu: Add tests for 64-bit overflows
Date: Tue, 24 Jan 2023 07:26:11 -0800 [thread overview]
Message-ID: <Y8/4kxSdOP2QrJVm@google.com> (raw)
In-Reply-To: <8bfc9b2b-0bad-a6dc-3c23-f67ad3d56741@redhat.com>
On Mon, Jan 23, 2023 at 09:33:50PM +0100, Eric Auger wrote:
> Hi Ricardo,
>
> On 1/9/23 22:17, Ricardo Koller wrote:
> > Modify all tests checking overflows to support both 32 (PMCR_EL0.LP == 0)
> > and 64-bit overflows (PMCR_EL0.LP == 1). 64-bit overflows are only
> > supported on PMUv3p5.
> >
> > Note that chained tests do not implement "overflow_at_64bits == true".
> > That's because there are no CHAIN events when "PMCR_EL0.LP == 1" (for more
> > details see AArch64.IncrementEventCounter() pseudocode in the ARM ARM DDI
> > 0487H.a, J1.1.1 "aarch64/debug").
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> > arm/pmu.c | 116 +++++++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 75 insertions(+), 41 deletions(-)
> >
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 0d06b59..72d0f50 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -28,6 +28,7 @@
> > #define PMU_PMCR_X (1 << 4)
> > #define PMU_PMCR_DP (1 << 5)
> > #define PMU_PMCR_LC (1 << 6)
> > +#define PMU_PMCR_LP (1 << 7)
> > #define PMU_PMCR_N_SHIFT 11
> > #define PMU_PMCR_N_MASK 0x1f
> > #define PMU_PMCR_ID_SHIFT 16
> > @@ -56,9 +57,13 @@
> >
> > #define ALL_SET 0x00000000FFFFFFFFULL
> > #define ALL_CLEAR 0x0000000000000000ULL
> > -#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL
> > +#define PRE_OVERFLOW_32 0x00000000FFFFFFF0ULL
> > +#define PRE_OVERFLOW_64 0xFFFFFFFFFFFFFFF0ULL
> > #define PRE_OVERFLOW2 0x00000000FFFFFFDCULL
> >
> > +#define PRE_OVERFLOW(__overflow_at_64bits) \
> > + (__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
> > +
> > #define PMU_PPI 23
> >
> > struct pmu {
> > @@ -449,8 +454,10 @@ static bool check_overflow_prerequisites(bool overflow_at_64bits)
> > static void test_basic_event_count(bool overflow_at_64bits)
> > {
> > uint32_t implemented_counter_mask, non_implemented_counter_mask;
> > - uint32_t counter_mask;
> > + uint64_t pre_overflow = PRE_OVERFLOW(overflow_at_64bits);
> > + uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > uint32_t events[] = {CPU_CYCLES, INST_RETIRED};
> > + uint32_t counter_mask;
> >
> > if (!satisfy_prerequisites(events, ARRAY_SIZE(events)) ||
> > !check_overflow_prerequisites(overflow_at_64bits))
> > @@ -472,13 +479,13 @@ static void test_basic_event_count(bool overflow_at_64bits)
> > * clear cycle and all event counters and allow counter enablement
> > * through PMCNTENSET. LC is RES1.
> > */
> > - set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
> > + set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P | pmcr_lp);
> > isb();
> > - report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC), "pmcr: reset counters");
> > + report(get_pmcr() == (pmu.pmcr_ro | PMU_PMCR_LC | pmcr_lp), "pmcr: reset counters");
> >
> > /* Preset counter #0 to pre overflow value to trigger an overflow */
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > - report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > + report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> > "counter #0 preset to pre-overflow value");
> > report(!read_regn_el0(pmevcntr, 1), "counter #1 is 0");
> >
> > @@ -531,6 +538,8 @@ static void test_mem_access(bool overflow_at_64bits)
> > {
> > void *addr = malloc(PAGE_SIZE);
> > uint32_t events[] = {MEM_ACCESS, MEM_ACCESS};
> > + uint64_t pre_overflow = PRE_OVERFLOW(overflow_at_64bits);
> > + uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> >
> > if (!satisfy_prerequisites(events, ARRAY_SIZE(events)) ||
> > !check_overflow_prerequisites(overflow_at_64bits))
> > @@ -542,7 +551,7 @@ static void test_mem_access(bool overflow_at_64bits)
> > write_regn_el0(pmevtyper, 1, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> > isb();
> > - mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > report_info("counter #0 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 0));
> > report_info("counter #1 is %ld (MEM_ACCESS)", read_regn_el0(pmevcntr, 1));
> > /* We may measure more than 20 mem access depending on the core */
> > @@ -552,11 +561,11 @@ static void test_mem_access(bool overflow_at_64bits)
> >
> > pmu_reset();
> >
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > - write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > + write_regn_el0(pmevcntr, 1, pre_overflow);
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> > isb();
> > - mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> > + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > report(read_sysreg(pmovsclr_el0) == 0x3,
> > "Ran 20 mem accesses with expected overflows on both counters");
> > report_info("cnt#0 = %ld cnt#1=%ld overflow=0x%lx",
> > @@ -566,8 +575,10 @@ static void test_mem_access(bool overflow_at_64bits)
> >
> > static void test_sw_incr(bool overflow_at_64bits)
> > {
> > + uint64_t pre_overflow = PRE_OVERFLOW(overflow_at_64bits);
> > + uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > uint32_t events[] = {SW_INCR, SW_INCR};
> > - uint64_t cntr0 = (PRE_OVERFLOW + 100) & pmevcntr_mask();
> > + uint64_t cntr0 = (pre_overflow + 100) & pmevcntr_mask();
> > int i;
> >
> > if (!satisfy_prerequisites(events, ARRAY_SIZE(events)) ||
> > @@ -581,7 +592,7 @@ static void test_sw_incr(bool overflow_at_64bits)
> > /* enable counters #0 and #1 */
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> >
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > isb();
> >
> > for (i = 0; i < 100; i++)
> > @@ -589,14 +600,14 @@ static void test_sw_incr(bool overflow_at_64bits)
> >
> > isb();
> > report_info("SW_INCR counter #0 has value %ld", read_regn_el0(pmevcntr, 0));
> > - report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
> > + report(read_regn_el0(pmevcntr, 0) == pre_overflow,
> > "PWSYNC does not increment if PMCR.E is unset");
> >
> > pmu_reset();
> >
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> > - set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > + set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > isb();
> >
> > for (i = 0; i < 100; i++)
> > @@ -614,6 +625,7 @@ static void test_sw_incr(bool overflow_at_64bits)
> > static void test_chained_counters(bool unused)
> > {
> > uint32_t events[] = {CPU_CYCLES, CHAIN};
> > + uint64_t all_set = pmevcntr_mask();
> ALL_SET_32 may be enough, as suggested by Reiji;
I think we still want to conditionally use ALL_SET_64 for the
pre-overflow value of the odd counter in the case of PMUv3p5.
More below.
> >
> > if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > return;
> > @@ -624,7 +636,7 @@ static void test_chained_counters(bool unused)
> > write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> > /* enable counters #0 and #1 */
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
> >
> > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >
> > @@ -636,26 +648,26 @@ static void test_chained_counters(bool unused)
> > pmu_reset();
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> >
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
> > write_regn_el0(pmevcntr, 1, 0x1);
> > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> > report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained incr #2");
As we want to test the overflow of the chained counter, we need
ALL_SET_64 for its preoverflow value.
> >
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > - write_regn_el0(pmevcntr, 1, ALL_SET);
> > + write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
> > + write_regn_el0(pmevcntr, 1, all_set);
> and here
> >
> > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > - report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > + report(read_regn_el0(pmevcntr, 1) == 0, "CHAIN counter #1 wrapped");
> > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters");
> > }
> >
> > static void test_chained_sw_incr(bool unused)
> > {
> > uint32_t events[] = {SW_INCR, CHAIN};
> > - uint64_t cntr0 = (PRE_OVERFLOW + 100) & pmevcntr_mask();
> > + uint64_t cntr0 = (PRE_OVERFLOW_32 + 100) & pmevcntr_mask();
> > uint64_t cntr1 = (ALL_SET + 1) & pmevcntr_mask();
> > int i;
> the chained counter changes may be put in a separate patch to help the
> reviewer focus on the new 64b overflow tests.
Got it, will try that for the next version.
> >
> > @@ -669,7 +681,7 @@ static void test_chained_sw_incr(bool unused)
> > /* enable counters #0 and #1 */
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> >
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
> > set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > isb();
> >
> > @@ -687,7 +699,7 @@ static void test_chained_sw_incr(bool unused)
> > pmu_reset();
> >
> > write_regn_el0(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0);
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
> > write_regn_el0(pmevcntr, 1, ALL_SET);
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> > set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > @@ -726,7 +738,7 @@ static void test_chain_promotion(bool unused)
> >
> > /* Only enable even counter */
> > pmu_reset();
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
> > write_sysreg_s(0x1, PMCNTENSET_EL0);
> > isb();
> >
> > @@ -856,6 +868,9 @@ static bool expect_interrupts(uint32_t bitmap)
> >
> > static void test_overflow_interrupt(bool overflow_at_64bits)
> > {
> > + uint64_t pre_overflow = PRE_OVERFLOW(overflow_at_64bits);
> > + uint64_t all_set = pmevcntr_mask();
> > + uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> > uint32_t events[] = {MEM_ACCESS, SW_INCR};
> > void *addr = malloc(PAGE_SIZE);
> > int i;
> > @@ -874,16 +889,16 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> > write_regn_el0(pmevtyper, 1, SW_INCR | PMEVTYPER_EXCLUDE_EL0);
> > write_sysreg_s(0x3, PMCNTENSET_EL0);
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > - write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > + write_regn_el0(pmevcntr, 1, pre_overflow);
> > isb();
> >
> > /* interrupts are disabled (PMINTENSET_EL1 == 0) */
> >
> > - mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> I noticed that undocumented change. Is it done on purpose?
Yes, it's intentional. It conditionally sets PMCR.LP for the next
subtest when overflow_at_64bits==true. That's also why the pre-overflow
value is close to ALL_SET_64 (and not ALL_SET_32).
> > report(expect_interrupts(0), "no overflow interrupt after preset");
> >
> > - set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > + set_pmcr(pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > isb();
> >
> > for (i = 0; i < 100; i++)
> > @@ -898,12 +913,12 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> >
> > pmu_reset_stats();
> >
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > - write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > + write_regn_el0(pmevcntr, 1, pre_overflow);
> > write_sysreg(ALL_SET, pmintenset_el1);
> > isb();
> >
> > - mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > + mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > for (i = 0; i < 100; i++)
> > write_sysreg(0x3, pmswinc_el0);
> >
> > @@ -912,25 +927,40 @@ static void test_overflow_interrupt(bool overflow_at_64bits)
> > report(expect_interrupts(0x3),
> > "overflow interrupts expected on #0 and #1");
> >
> > - /* promote to 64-b */
> > + /*
> > + * promote to 64-b:
> > + *
> > + * This only applies to the !overflow_at_64bits case, as
> > + * overflow_at_64bits doesn't implement CHAIN events. The
> > + * overflow_at_64bits case just checks that chained counters are
> > + * not incremented when PMCR.LP == 1.
> > + */
> >
> > pmu_reset_stats();
> >
> > write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > isb();
> > - mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > - report(expect_interrupts(0x1),
> > - "expect overflow interrupt on 32b boundary");
> > + mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > + report(expect_interrupts(0x1), "expect overflow interrupt");
> >
> > /* overflow on odd counter */
> > pmu_reset_stats();
> > - write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > - write_regn_el0(pmevcntr, 1, ALL_SET);
> > + write_regn_el0(pmevcntr, 0, pre_overflow);
> > + write_regn_el0(pmevcntr, 1, all_set);
> > isb();
> > - mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> > - report(expect_interrupts(0x3),
> > - "expect overflow interrupt on even and odd counter");
> > + mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> > + if (overflow_at_64bits) {
> > + report(expect_interrupts(0x1),
> > + "expect overflow interrupt on even counter");
> > + report(read_regn_el0(pmevcntr, 1) == all_set,
> > + "Odd counter did not change");
> > + } else {
> > + report(expect_interrupts(0x3),
> > + "expect overflow interrupt on even and odd counter");
> > + report(read_regn_el0(pmevcntr, 1) != all_set,
> > + "Odd counter wrapped");
> > + }
> > }
> > #endif
> >
> > @@ -1131,10 +1161,13 @@ int main(int argc, char *argv[])
> > report_prefix_pop();
> > } else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
> > run_test(argv[1], test_basic_event_count, false);
> > + run_test(argv[1], test_basic_event_count, true);
> > } else if (strcmp(argv[1], "pmu-mem-access") == 0) {
> > run_test(argv[1], test_mem_access, false);
> > + run_test(argv[1], test_mem_access, true);
> > } else if (strcmp(argv[1], "pmu-sw-incr") == 0) {
> > run_test(argv[1], test_sw_incr, false);
> > + run_test(argv[1], test_sw_incr, true);
> > } else if (strcmp(argv[1], "pmu-chained-counters") == 0) {
> > run_test(argv[1], test_chained_counters, false);
> > } else if (strcmp(argv[1], "pmu-chained-sw-incr") == 0) {
> > @@ -1143,6 +1176,7 @@ int main(int argc, char *argv[])
> > run_test(argv[1], test_chain_promotion, false);
> > } else if (strcmp(argv[1], "pmu-overflow-interrupt") == 0) {
> > run_test(argv[1], test_overflow_interrupt, false);
> > + run_test(argv[1], test_overflow_interrupt, true);
> > } else {
> > report_abort("Unknown sub-test '%s'", argv[1]);
> > }
> Thanks
>
> Eric
>
next prev parent reply other threads:[~2023-01-24 15:26 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-09 21:17 [kvm-unit-tests PATCH v3 0/4] arm: pmu: Add support for PMUv3p5 Ricardo Koller
2023-01-09 21:17 ` [kvm-unit-tests PATCH v3 1/4] arm: pmu: Fix overflow checks for PMUv3p5 long counters Ricardo Koller
2023-01-09 21:42 ` Oliver Upton
2023-01-23 20:16 ` Eric Auger
2023-01-09 21:17 ` [kvm-unit-tests PATCH v3 2/4] arm: pmu: Prepare for testing 64-bit overflows Ricardo Koller
2023-01-12 5:56 ` Reiji Watanabe
2023-01-13 15:20 ` Ricardo Koller
2023-01-09 21:17 ` [kvm-unit-tests PATCH v3 3/4] arm: pmu: Add tests for " Ricardo Koller
2023-01-19 5:58 ` Reiji Watanabe
2023-01-24 15:11 ` Ricardo Koller
2023-01-25 2:19 ` Ricardo Koller
2023-01-25 4:11 ` Reiji Watanabe
2023-01-25 7:55 ` Eric Auger
2023-01-25 14:17 ` Ricardo Koller
2023-01-23 20:33 ` Eric Auger
2023-01-24 15:26 ` Ricardo Koller [this message]
2023-01-24 20:15 ` Eric Auger
2023-01-26 16:45 ` Ricardo Koller
2023-01-09 21:17 ` [kvm-unit-tests PATCH v3 4/4] arm: pmu: Print counter values as hexadecimals Ricardo Koller
2023-01-09 21:43 ` Oliver Upton
2023-01-23 20:17 ` Eric Auger
2023-01-25 4:37 ` Reiji Watanabe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y8/4kxSdOP2QrJVm@google.com \
--to=ricarkol@google.com \
--cc=alexandru.elisei@arm.com \
--cc=andrew.jones@linux.dev \
--cc=eric.auger@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.linux.dev \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=reijiw@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.