From: Ricardo Koller <ricarkol@google.com>
To: Reiji Watanabe <reijiw@google.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.linux.dev,
andrew.jones@linux.dev, maz@kernel.org, alexandru.elisei@arm.com,
eric.auger@redhat.com, oliver.upton@linux.dev
Subject: Re: [kvm-unit-tests PATCH v3 3/4] arm: pmu: Add tests for 64-bit overflows
Date: Tue, 24 Jan 2023 07:11:27 -0800 [thread overview]
Message-ID: <Y8/1H8abmaL1ISKu@google.com> (raw)
In-Reply-To: <CAAeT=FxoS2-cmMe-3FeXPXcvE4wNosZeZy2RGPXz5xisq5fj7A@mail.gmail.com>
On Wed, Jan 18, 2023 at 09:58:38PM -0800, Reiji Watanabe wrote:
> Hi Ricardo,
>
> On Mon, Jan 9, 2023 at 1:18 PM Ricardo Koller <ricarkol@google.com> 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>
>
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
>
> I have a few nits below.
>
> > ---
> > 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
>
> Nit: Perhaps renaming this to ALL_SET_32 might be more clear.
> Since the test is now aware of 64 bit counters, the name 'ALL_SET'
> makes me think that all 64 bits are set.
>
Sounds good, will do that in the next version.
> > #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();
> >
> > 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");
> >
> > - 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);
> >
> > 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;
> >
> > @@ -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);
> > 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();
>
> This isn't directly related to the patch.
> But, as bits of pmovsclr_el0 are already set (although interrupts
> are disabled), I would think it's good to clear pmovsclr_el0 here.
>
> Thank you,
> Reiji
>
>
>
> >
> > - 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]);
> > }
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >
next prev parent reply other threads:[~2023-01-24 15:11 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 [this message]
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
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/1H8abmaL1ISKu@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.