| Message ID | 20221202045527.3646838-2-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: pmu: Add support for PMUv3p5 | expand |
Hi, On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > for overflowing at 32 or 64-bits. The consequence is that tests that check > the counter values after overflowing should not assume that values will be > wrapped around 32-bits: they overflow into the other half of the 64-bit > counters on PMUv3p5. > > Fix tests by correctly checking overflowing-counters against the expected > 64-bit value. > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > --- > arm/pmu.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/arm/pmu.c b/arm/pmu.c > index cd47b14..eeac984 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -54,10 +54,10 @@ > #define EXT_COMMON_EVENTS_LOW 0x4000 > #define EXT_COMMON_EVENTS_HIGH 0x403F > > -#define ALL_SET 0xFFFFFFFF > -#define ALL_CLEAR 0x0 > -#define PRE_OVERFLOW 0xFFFFFFF0 > -#define PRE_OVERFLOW2 0xFFFFFFDC > +#define ALL_SET 0x00000000FFFFFFFFULL > +#define ALL_CLEAR 0x0000000000000000ULL > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > #define PMU_PPI 23 > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > static void test_sw_incr(void) > { > uint32_t events[] = {SW_INCR, SW_INCR}; > + uint64_t cntr0; > int i; > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > write_sysreg(0x3, pmswinc_el0); > > isb(); > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > - report(read_regn_el0(pmevcntr, 1) == 100, > - "counter #0 after + 100 SW_INCR"); > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is implemented. But it doesn't say anywhere that versions newer than p5 are required to implement PMUv3p5. For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7 implementations. My interpretation of that is that it is not forbidden for an implementer to cherry-pick this version on older versions of the architecture where PMUv3p5 is not implemented. Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the counter definitions in the architecture? Also, I found the meaning of those numbers to be quite cryptic. Perhaps something like this would be more resilient to changes to the value of PRE_OVERFLOW and easier to understand: + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? + (uint32_t)PRE_OVERFLOW + 100 : + (uint64_t)PRE_OVERFLOW + 100; I haven't tested the code, would that work? Thanks, Alex > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > report(read_sysreg(pmovsclr_el0) == 0x1, > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > static void test_chained_counters(void) > { > uint32_t events[] = {CPU_CYCLES, CHAIN}; > + uint64_t cntr1; > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > return; > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > 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"); > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); > + > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > } > > static void test_chained_sw_incr(void) > { > uint32_t events[] = {SW_INCR, CHAIN}; > + uint64_t cntr0, cntr1; > int i; > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > write_sysreg(0x1, pmswinc_el0); > > isb(); > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > report((read_sysreg(pmovsclr_el0) == 0x3) && > - (read_regn_el0(pmevcntr, 1) == 0) && > - (read_regn_el0(pmevcntr, 0) == 84), > - "expected overflows and values after 100 SW_INCR/CHAIN"); > + (read_regn_el0(pmevcntr, 1) == cntr0) && > + (read_regn_el0(pmevcntr, 0) == cntr1), > + "expected overflows and values after 100 SW_INCR/CHAIN"); > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > } > -- > 2.39.0.rc0.267.gcb52ba06e7-goog >
On Fri, 09 Dec 2022 17:47:14 +0000, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi, > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > the counter values after overflowing should not assume that values will be > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > counters on PMUv3p5. > > > > Fix tests by correctly checking overflowing-counters against the expected > > 64-bit value. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arm/pmu.c | 29 ++++++++++++++++++----------- > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > index cd47b14..eeac984 100644 > > --- a/arm/pmu.c > > +++ b/arm/pmu.c > > @@ -54,10 +54,10 @@ > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > -#define ALL_SET 0xFFFFFFFF > > -#define ALL_CLEAR 0x0 > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > +#define ALL_SET 0x00000000FFFFFFFFULL > > +#define ALL_CLEAR 0x0000000000000000ULL > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > #define PMU_PPI 23 > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > static void test_sw_incr(void) > > { > > uint32_t events[] = {SW_INCR, SW_INCR}; > > + uint64_t cntr0; > > int i; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > write_sysreg(0x3, pmswinc_el0); > > > > isb(); > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > - report(read_regn_el0(pmevcntr, 1) == 100, > > - "counter #0 after + 100 SW_INCR"); > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is > implemented. But it doesn't say anywhere that versions newer than p5 are > required to implement PMUv3p5. And I don't think it needs to say it, because there is otherwise no way for SW to discover whether 64bit counters are implemented or not. > > For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7 > implementations. My interpretation of that is that it is not forbidden for > an implementer to cherry-pick this version on older versions of the > architecture where PMUv3p5 is not implemented. I'm sorry to have to say that, but I find your suggestion that PMUv3p7 could be implemented without supporting the full gamut of PMUv3p5 ludicrous. Please look back at the ARM ARM, specially at the tiny section titled "Alternative ID scheme used for the Performance Monitors Extension version" (DDI0487I.a, D17.1.3, page 5553), and the snipped of C code that performs exactly this check: <quote> if (value != 0xF and value >= number) { // do something that relies on version 'number' of the feature } </quote> Replace 'value' with 7 (PMUv3p7), 'number' with 6 (PMUv3p5), and you get the exact property that you pretend doesn't exist, allowing you to rely on PMUv3p5 to be implemented when the HW has PMUv3p7. > Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the > counter definitions in the architecture? No, that'd be totally wrong. You need to check your understanding of how the ID registers work. M.
Hi Marc, On Sat, Dec 10, 2022 at 11:01:53AM +0000, Marc Zyngier wrote: > On Fri, 09 Dec 2022 17:47:14 +0000, > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > > Hi, > > > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > > the counter values after overflowing should not assume that values will be > > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > > counters on PMUv3p5. > > > > > > Fix tests by correctly checking overflowing-counters against the expected > > > 64-bit value. > > > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > > --- > > > arm/pmu.c | 29 ++++++++++++++++++----------- > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > > index cd47b14..eeac984 100644 > > > --- a/arm/pmu.c > > > +++ b/arm/pmu.c > > > @@ -54,10 +54,10 @@ > > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > > > -#define ALL_SET 0xFFFFFFFF > > > -#define ALL_CLEAR 0x0 > > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > > +#define ALL_SET 0x00000000FFFFFFFFULL > > > +#define ALL_CLEAR 0x0000000000000000ULL > > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > > > #define PMU_PPI 23 > > > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > > static void test_sw_incr(void) > > > { > > > uint32_t events[] = {SW_INCR, SW_INCR}; > > > + uint64_t cntr0; > > > int i; > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > > write_sysreg(0x3, pmswinc_el0); > > > > > > isb(); > > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > > - report(read_regn_el0(pmevcntr, 1) == 100, > > > - "counter #0 after + 100 SW_INCR"); > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > > Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is > > implemented. But it doesn't say anywhere that versions newer than p5 are > > required to implement PMUv3p5. > > And I don't think it needs to say it, because there is otherwise no > way for SW to discover whether 64bit counters are implemented or not. > > > > > For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7 > > implementations. My interpretation of that is that it is not forbidden for > > an implementer to cherry-pick this version on older versions of the > > architecture where PMUv3p5 is not implemented. > > I'm sorry to have to say that, but I find your suggestion that PMUv3p7 > could be implemented without supporting the full gamut of PMUv3p5 > ludicrous. > > Please look back at the ARM ARM, specially at the tiny section titled > "Alternative ID scheme used for the Performance Monitors Extension > version" (DDI0487I.a, D17.1.3, page 5553), and the snipped of C code > that performs exactly this check: > > <quote> > if (value != 0xF and value >= number) { > // do something that relies on version 'number' of the feature > } > </quote> > > Replace 'value' with 7 (PMUv3p7), 'number' with 6 (PMUv3p5), and you > get the exact property that you pretend doesn't exist, allowing you to > rely on PMUv3p5 to be implemented when the HW has PMUv3p7. > > > Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the > > counter definitions in the architecture? > > No, that'd be totally wrong. You need to check your understanding of > how the ID registers work. A simple "hey, you're wrong here, the PMU extensions do not follow the principles of the ID scheme for fields in ID registers" would have sufficed. Guess you never made a silly mistake ever, right? Otherwise, good job encouraging people to help review KVM/arm64 patches ;) Thanks, Alex
Alex, On Sun, 11 Dec 2022 11:40:39 +0000, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > A simple "hey, you're wrong here, the PMU extensions do not follow the > principles of the ID scheme for fields in ID registers" would have > sufficed. This is what I did, and saved you the hassle of looking it up. > Guess you never made a silly mistake ever, right? It's not so much about making a silly mistake. I do that all the time. But it is about the way you state these things, and the weight that your reviews carry. You're a trusted reviewer, with a lot of experience, and posting with an @arm.com address: what you say in a public forum sticks. When you assert that the author is wrong, they will take it at face value. > Otherwise, good job encouraging people to help review KVM/arm64 patches ;) What is the worse: no review? or a review that spreads confusion? Think about it. I'm all for being nice, but I will call bullshit when I see it asserted by people with a certain level of authority. And I've long made up my mind about the state of the KVM/arm64 review process -- reviews rarely come from people who have volunteered to do so, but instead from those who have either a vested interest in it, or an ulterior motive. Hey ho... M.
Hi, On Mon, Dec 12, 2022 at 09:05:02AM +0000, Marc Zyngier wrote: > Alex, > > On Sun, 11 Dec 2022 11:40:39 +0000, > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > > A simple "hey, you're wrong here, the PMU extensions do not follow the > > principles of the ID scheme for fields in ID registers" would have > > sufficed. > > This is what I did, and saved you the hassle of looking it up. The comment was about how you went about it, not about proving someone wrong. As expressive as it might be, I don't think that calling someone's suggestion "ludicrous" from the position of authority associated with being a maintainer is constructive; and can also be interpreted as a personal attack (you used **your** suggestion, not **this** suggestion). I didn't interpret it that way, just saying that it can be. > > > Guess you never made a silly mistake ever, right? > > It's not so much about making a silly mistake. I do that all the time. > But it is about the way you state these things, and the weight that > your reviews carry. You're a trusted reviewer, with a lot of > experience, and posting with an @arm.com address: what you say in a > public forum sticks. When you assert that the author is wrong, they > will take it at face value. This is how I stated things: "Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is implemented. But it doesn't say anywhere that versions newer than p5 are required to implement PMUv3p5." -> patently false, easily provable with the Arm ARM and by logic (as you did). My entire argument was based on this, so once this has been proven false, I would say that the rest of my argument falls apart. "For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7 implementations. **My interpretation** of that is that it is not forbidden for an implementer to cherry-pick this version on older versions of the architecture where PMUv3p5 is not implemented." -> emphasis on the "my interpretation"; also easy to prove false because PMUv3p5+ is required to implement PMUv3p5, as per the architecture. "**Maybe** the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the counter definitions in the architecture?" -> emphasis on the "maybe", and the question mark at the end. My intention wasn't to dictate something, my intention was to have a conversation about the patch, with the mindset that I might be wrong. What made you get the idea that I was asserting that the author is wrong? Where by "asserting the author is wrong" I understand framing my comment in such a way as to leave no room for further discussions. Or did you mean something else by that? Or, to put it another way, what about the way I stated things could have been done better (other than not being wrong, obviously)? > > > Otherwise, good job encouraging people to help review KVM/arm64 patches ;) > > What is the worse: no review? or a review that spreads confusion? > Think about it. I'm all for being nice, but I will call bullshit when That wasn't about calling people out on their mistakes. I was saying that the way you "call bullshit", as you put it, might be a put off for some people. Call me naive, but I like to think that not everyone that comments on a patch does it because they have to. > I see it asserted by people with a certain level of authority. > > And I've long made up my mind about the state of the KVM/arm64 review > process -- reviews rarely come from people who have volunteered to do > so, but instead from those who have either a vested interest in it, or > an ulterior motive. Hey ho... I genuinely don't know what to make of this. I can't even tell if it's directed at me or not. Thanks, Alex
On Fri, Dec 09, 2022 at 05:47:14PM +0000, Alexandru Elisei wrote: > Hi, > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > the counter values after overflowing should not assume that values will be > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > counters on PMUv3p5. > > > > Fix tests by correctly checking overflowing-counters against the expected > > 64-bit value. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arm/pmu.c | 29 ++++++++++++++++++----------- > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > index cd47b14..eeac984 100644 > > --- a/arm/pmu.c > > +++ b/arm/pmu.c > > @@ -54,10 +54,10 @@ > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > -#define ALL_SET 0xFFFFFFFF > > -#define ALL_CLEAR 0x0 > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > +#define ALL_SET 0x00000000FFFFFFFFULL > > +#define ALL_CLEAR 0x0000000000000000ULL > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > #define PMU_PPI 23 > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > static void test_sw_incr(void) > > { > > uint32_t events[] = {SW_INCR, SW_INCR}; > > + uint64_t cntr0; > > int i; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > write_sysreg(0x3, pmswinc_el0); > > > > isb(); > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > - report(read_regn_el0(pmevcntr, 1) == 100, > > - "counter #0 after + 100 SW_INCR"); > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is > implemented. But it doesn't say anywhere that versions newer than p5 are > required to implement PMUv3p5. > > For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7 > implementations. My interpretation of that is that it is not forbidden for > an implementer to cherry-pick this version on older versions of the > architecture where PMUv3p5 is not implemented. > > Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the > counter definitions in the architecture? > > Also, I found the meaning of those numbers to be quite cryptic. Perhaps > something like this would be more resilient to changes to the value of > PRE_OVERFLOW and easier to understand: > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? > + (uint32_t)PRE_OVERFLOW + 100 : > + (uint64_t)PRE_OVERFLOW + 100; > > I haven't tested the code, would that work? Just checked. That works. It's much cleaner, thank you very much. Will send a v2 a the end of the week, maybe after some more reviews. > > Thanks, > Alex > > > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > report(read_sysreg(pmovsclr_el0) == 0x1, > > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > > static void test_chained_counters(void) > > { > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > + uint64_t cntr1; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > return; > > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > > > 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"); > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); > > + > > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > > } > > > > static void test_chained_sw_incr(void) > > { > > uint32_t events[] = {SW_INCR, CHAIN}; > > + uint64_t cntr0, cntr1; > > int i; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > > write_sysreg(0x1, pmswinc_el0); > > > > isb(); > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > report((read_sysreg(pmovsclr_el0) == 0x3) && > > - (read_regn_el0(pmevcntr, 1) == 0) && > > - (read_regn_el0(pmevcntr, 0) == 84), > > - "expected overflows and values after 100 SW_INCR/CHAIN"); > > + (read_regn_el0(pmevcntr, 1) == cntr0) && > > + (read_regn_el0(pmevcntr, 0) == cntr1), > > + "expected overflows and values after 100 SW_INCR/CHAIN"); > > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > } > > -- > > 2.39.0.rc0.267.gcb52ba06e7-goog > >
Hi, Some more comments below. On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > for overflowing at 32 or 64-bits. The consequence is that tests that check > the counter values after overflowing should not assume that values will be > wrapped around 32-bits: they overflow into the other half of the 64-bit > counters on PMUv3p5. > > Fix tests by correctly checking overflowing-counters against the expected > 64-bit value. > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > --- > arm/pmu.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/arm/pmu.c b/arm/pmu.c > index cd47b14..eeac984 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -54,10 +54,10 @@ > #define EXT_COMMON_EVENTS_LOW 0x4000 > #define EXT_COMMON_EVENTS_HIGH 0x403F > > -#define ALL_SET 0xFFFFFFFF > -#define ALL_CLEAR 0x0 > -#define PRE_OVERFLOW 0xFFFFFFF0 > -#define PRE_OVERFLOW2 0xFFFFFFDC > +#define ALL_SET 0x00000000FFFFFFFFULL > +#define ALL_CLEAR 0x0000000000000000ULL > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > #define PMU_PPI 23 > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > static void test_sw_incr(void) > { > uint32_t events[] = {SW_INCR, SW_INCR}; > + uint64_t cntr0; > int i; > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > write_sysreg(0x3, pmswinc_el0); > > isb(); > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > - report(read_regn_el0(pmevcntr, 1) == 100, > - "counter #0 after + 100 SW_INCR"); > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > report(read_sysreg(pmovsclr_el0) == 0x1, > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > static void test_chained_counters(void) > { > uint32_t events[] = {CPU_CYCLES, CHAIN}; > + uint64_t cntr1; > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > return; > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > 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"); > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); It looks to me like the intention of the test was to check that the counter programmed with the CHAIN event wraps, judging from the report message. I think it would be interesting to keep that by programming counter #1 with ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value against 0. Alternatively, the report message can be modified, and "wrapped" replaced with "incremented" (or something like that), to avoid confusion. > + > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > } > > static void test_chained_sw_incr(void) > { > uint32_t events[] = {SW_INCR, CHAIN}; > + uint64_t cntr0, cntr1; > int i; > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > write_sysreg(0x1, pmswinc_el0); > > isb(); > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > report((read_sysreg(pmovsclr_el0) == 0x3) && > - (read_regn_el0(pmevcntr, 1) == 0) && > - (read_regn_el0(pmevcntr, 0) == 84), > - "expected overflows and values after 100 SW_INCR/CHAIN"); > + (read_regn_el0(pmevcntr, 1) == cntr0) && > + (read_regn_el0(pmevcntr, 0) == cntr1), This is hard to parse, it would be better if counter 0 is compared against cntr0 and counter 1 against cntr1. Also, same suggestion here, looks like the test wants to check that the odd-numbered counter wraps around when counting the CHAIN event. Thanks, Alex > + "expected overflows and values after 100 SW_INCR/CHAIN"); > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > } > -- > 2.39.0.rc0.267.gcb52ba06e7-goog >
On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote: > Hi, > > Some more comments below. > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > the counter values after overflowing should not assume that values will be > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > counters on PMUv3p5. > > > > Fix tests by correctly checking overflowing-counters against the expected > > 64-bit value. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arm/pmu.c | 29 ++++++++++++++++++----------- > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > index cd47b14..eeac984 100644 > > --- a/arm/pmu.c > > +++ b/arm/pmu.c > > @@ -54,10 +54,10 @@ > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > -#define ALL_SET 0xFFFFFFFF > > -#define ALL_CLEAR 0x0 > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > +#define ALL_SET 0x00000000FFFFFFFFULL > > +#define ALL_CLEAR 0x0000000000000000ULL > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > #define PMU_PPI 23 > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > static void test_sw_incr(void) > > { > > uint32_t events[] = {SW_INCR, SW_INCR}; > > + uint64_t cntr0; > > int i; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > write_sysreg(0x3, pmswinc_el0); > > > > isb(); > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > - report(read_regn_el0(pmevcntr, 1) == 100, > > - "counter #0 after + 100 SW_INCR"); > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > report(read_sysreg(pmovsclr_el0) == 0x1, > > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > > static void test_chained_counters(void) > > { > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > + uint64_t cntr1; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > return; > > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > > > 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"); > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); > > It looks to me like the intention of the test was to check that the counter > programmed with the CHAIN event wraps, judging from the report message. > Ah, right. Yeah, that message is confusing. It should be the short version of "Inrementing at 32-bits resulted in the right value". > I think it would be interesting to keep that by programming counter #1 with > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value > against 0. The last commit adds tests using ALL_SET64. Tests can be run in two modes: overflow_at_64bits and not. However, this test, test_chained_counters(), and all other chained tests only use the !overflow_at_64bits mode (even after the last commit). The reason is that there are no CHAIN events when overflowing at 64-bits (more details in the commit message). But, don't worry, there are lots of tests that check wrapping at 64-bits (overflow_at_64bits=true). > Alternatively, the report message can be modified, and "wrapped" > replaced with "incremented" (or something like that), to avoid confusion. > > > + > > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > > } > > > > static void test_chained_sw_incr(void) > > { > > uint32_t events[] = {SW_INCR, CHAIN}; > > + uint64_t cntr0, cntr1; > > int i; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > > write_sysreg(0x1, pmswinc_el0); > > > > isb(); > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > report((read_sysreg(pmovsclr_el0) == 0x3) && > > - (read_regn_el0(pmevcntr, 1) == 0) && > > - (read_regn_el0(pmevcntr, 0) == 84), > > - "expected overflows and values after 100 SW_INCR/CHAIN"); > > + (read_regn_el0(pmevcntr, 1) == cntr0) && > > + (read_regn_el0(pmevcntr, 0) == cntr1), > > This is hard to parse, it would be better if counter 0 is compared against > cntr0 and counter 1 against cntr1. Ah, yes, code is correct but that's indeed confusing. > > Also, same suggestion here, looks like the test wants to check that the > odd-numbered counter wraps around when counting the CHAIN event. Ack. Will update for v2. Thanks! Ricardo > > Thanks, > Alex > > > + "expected overflows and values after 100 SW_INCR/CHAIN"); > > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > } > > -- > > 2.39.0.rc0.267.gcb52ba06e7-goog > >
Hi, On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote: > On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote: > > Hi, > > > > Some more comments below. > > > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > > the counter values after overflowing should not assume that values will be > > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > > counters on PMUv3p5. > > > > > > Fix tests by correctly checking overflowing-counters against the expected > > > 64-bit value. > > > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > > --- > > > arm/pmu.c | 29 ++++++++++++++++++----------- > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > > index cd47b14..eeac984 100644 > > > --- a/arm/pmu.c > > > +++ b/arm/pmu.c > > > @@ -54,10 +54,10 @@ > > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > > > -#define ALL_SET 0xFFFFFFFF > > > -#define ALL_CLEAR 0x0 > > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > > +#define ALL_SET 0x00000000FFFFFFFFULL > > > +#define ALL_CLEAR 0x0000000000000000ULL > > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > > > #define PMU_PPI 23 > > > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > > static void test_sw_incr(void) > > > { > > > uint32_t events[] = {SW_INCR, SW_INCR}; > > > + uint64_t cntr0; > > > int i; > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > > write_sysreg(0x3, pmswinc_el0); > > > > > > isb(); > > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > > - report(read_regn_el0(pmevcntr, 1) == 100, > > > - "counter #0 after + 100 SW_INCR"); > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > > > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > > > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > report(read_sysreg(pmovsclr_el0) == 0x1, > > > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > > > static void test_chained_counters(void) > > > { > > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > > + uint64_t cntr1; > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > return; > > > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > > > > > 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"); > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); > > > > It looks to me like the intention of the test was to check that the counter > > programmed with the CHAIN event wraps, judging from the report message. > > > > Ah, right. Yeah, that message is confusing. It should be the short > version of "Inrementing at 32-bits resulted in the right value". > > > I think it would be interesting to keep that by programming counter #1 with > > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value > > against 0. > > The last commit adds tests using ALL_SET64. Tests can be run in two > modes: overflow_at_64bits and not. However, this test, > test_chained_counters(), and all other chained tests only use the > !overflow_at_64bits mode (even after the last commit). The reason is > that there are no CHAIN events when overflowing at 64-bits (more details > in the commit message). But, don't worry, there are lots of tests that > check wrapping at 64-bits (overflow_at_64bits=true). What I was suggesting is this (change on top of this series, not on top of this patch, to get access to ALL_SET_AT): diff --git a/arm/pmu.c b/arm/pmu.c index 3cb563b1abe4..fd7f20fc6c52 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits) static void test_chained_counters(bool overflow_at_64bits) { uint32_t events[] = {CPU_CYCLES, CHAIN}; - uint64_t cntr1; if (!satisfy_prerequisites(events, ARRAY_SIZE(events), overflow_at_64bits)) @@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits) 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, 1, ALL_SET_AT(overflow_at_64bits)); precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E); report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0)); - cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; - report(read_regn_el0(pmevcntr, 1) == cntr1, "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"); } The counters are 64bit, but the CHAIN event should still work because PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM DDI 0487I.a, page D17-6461). Thanks, Alex > > > Alternatively, the report message can be modified, and "wrapped" > > replaced with "incremented" (or something like that), to avoid confusion. > > > > > + > > > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > > > } > > > > > > static void test_chained_sw_incr(void) > > > { > > > uint32_t events[] = {SW_INCR, CHAIN}; > > > + uint64_t cntr0, cntr1; > > > int i; > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > > > write_sysreg(0x1, pmswinc_el0); > > > > > > isb(); > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > report((read_sysreg(pmovsclr_el0) == 0x3) && > > > - (read_regn_el0(pmevcntr, 1) == 0) && > > > - (read_regn_el0(pmevcntr, 0) == 84), > > > - "expected overflows and values after 100 SW_INCR/CHAIN"); > > > + (read_regn_el0(pmevcntr, 1) == cntr0) && > > > + (read_regn_el0(pmevcntr, 0) == cntr1), > > > > This is hard to parse, it would be better if counter 0 is compared against > > cntr0 and counter 1 against cntr1. > > Ah, yes, code is correct but that's indeed confusing. > > > > > Also, same suggestion here, looks like the test wants to check that the > > odd-numbered counter wraps around when counting the CHAIN event. > > Ack. Will update for v2. > > Thanks! > Ricardo > > > > > Thanks, > > Alex > > > > > + "expected overflows and values after 100 SW_INCR/CHAIN"); > > > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > } > > > -- > > > 2.39.0.rc0.267.gcb52ba06e7-goog > > >
On Tue, Dec 13, 2022 at 04:43:38PM +0000, Alexandru Elisei wrote: > Hi, > > On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote: > > On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote: > > > Hi, > > > > > > Some more comments below. > > > > > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > > > the counter values after overflowing should not assume that values will be > > > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > > > counters on PMUv3p5. > > > > > > > > Fix tests by correctly checking overflowing-counters against the expected > > > > 64-bit value. > > > > > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > > > --- > > > > arm/pmu.c | 29 ++++++++++++++++++----------- > > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > > > index cd47b14..eeac984 100644 > > > > --- a/arm/pmu.c > > > > +++ b/arm/pmu.c > > > > @@ -54,10 +54,10 @@ > > > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > > > > > -#define ALL_SET 0xFFFFFFFF > > > > -#define ALL_CLEAR 0x0 > > > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > > > +#define ALL_SET 0x00000000FFFFFFFFULL > > > > +#define ALL_CLEAR 0x0000000000000000ULL > > > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > > > > > #define PMU_PPI 23 > > > > > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > > > static void test_sw_incr(void) > > > > { > > > > uint32_t events[] = {SW_INCR, SW_INCR}; > > > > + uint64_t cntr0; > > > > int i; > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > > > write_sysreg(0x3, pmswinc_el0); > > > > > > > > isb(); > > > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > > > - report(read_regn_el0(pmevcntr, 1) == 100, > > > > - "counter #0 after + 100 SW_INCR"); > > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > > > > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > > > > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > > report(read_sysreg(pmovsclr_el0) == 0x1, > > > > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > > > > static void test_chained_counters(void) > > > > { > > > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > > > + uint64_t cntr1; > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > return; > > > > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > > > > > > > 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"); > > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); > > > > > > It looks to me like the intention of the test was to check that the counter > > > programmed with the CHAIN event wraps, judging from the report message. > > > > > > > Ah, right. Yeah, that message is confusing. It should be the short > > version of "Inrementing at 32-bits resulted in the right value". > > > > > I think it would be interesting to keep that by programming counter #1 with > > > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value > > > against 0. > > > > The last commit adds tests using ALL_SET64. Tests can be run in two > > modes: overflow_at_64bits and not. However, this test, > > test_chained_counters(), and all other chained tests only use the > > !overflow_at_64bits mode (even after the last commit). The reason is > > that there are no CHAIN events when overflowing at 64-bits (more details > > in the commit message). But, don't worry, there are lots of tests that > > check wrapping at 64-bits (overflow_at_64bits=true). > > What I was suggesting is this (change on top of this series, not on top of > this patch, to get access to ALL_SET_AT): Ooh, I see, I agree: it would be better to check that the odd counter increments from ~0ULL to 0 when using 64-bit counters. > > diff --git a/arm/pmu.c b/arm/pmu.c > index 3cb563b1abe4..fd7f20fc6c52 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits) > static void test_chained_counters(bool overflow_at_64bits) > { > uint32_t events[] = {CPU_CYCLES, CHAIN}; > - uint64_t cntr1; > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events), > overflow_at_64bits)) > @@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits) > 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, 1, ALL_SET_AT(overflow_at_64bits)); The only change is that this should be: ALL_SET_AT(pmu.version >= ID_DFR0_PMU_V3_8_5) Because "overflow_at_64bits" implies PMCR_EL0.LP = 1. > > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E); > report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0)); > - cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > - report(read_regn_el0(pmevcntr, 1) == cntr1, "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"); > } > > The counters are 64bit, but the CHAIN event should still work because > PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM > DDI 0487I.a, page D17-6461). > > Thanks, > Alex > > > > > > Alternatively, the report message can be modified, and "wrapped" > > > replaced with "incremented" (or something like that), to avoid confusion. > > > > > > > + > > > > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > > > > } > > > > > > > > static void test_chained_sw_incr(void) > > > > { > > > > uint32_t events[] = {SW_INCR, CHAIN}; > > > > + uint64_t cntr0, cntr1; > > > > int i; > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > > > > write_sysreg(0x1, pmswinc_el0); > > > > > > > > isb(); > > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > > report((read_sysreg(pmovsclr_el0) == 0x3) && > > > > - (read_regn_el0(pmevcntr, 1) == 0) && > > > > - (read_regn_el0(pmevcntr, 0) == 84), > > > > - "expected overflows and values after 100 SW_INCR/CHAIN"); > > > > + (read_regn_el0(pmevcntr, 1) == cntr0) && > > > > + (read_regn_el0(pmevcntr, 0) == cntr1), > > > > > > This is hard to parse, it would be better if counter 0 is compared against > > > cntr0 and counter 1 against cntr1. > > > > Ah, yes, code is correct but that's indeed confusing. > > > > > > > > Also, same suggestion here, looks like the test wants to check that the > > > odd-numbered counter wraps around when counting the CHAIN event. > > > > Ack. Will update for v2. > > > > Thanks! > > Ricardo > > > > > > > > Thanks, > > > Alex > > > > > > > + "expected overflows and values after 100 SW_INCR/CHAIN"); > > > > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > > } > > > > -- > > > > 2.39.0.rc0.267.gcb52ba06e7-goog > > > >
Hi, On Tue, Dec 13, 2022 at 10:01:06AM -0800, Ricardo Koller wrote: > On Tue, Dec 13, 2022 at 04:43:38PM +0000, Alexandru Elisei wrote: > > Hi, > > > > On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote: > > > On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote: > > > > Hi, > > > > > > > > Some more comments below. > > > > > > > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > > > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > > > > the counter values after overflowing should not assume that values will be > > > > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > > > > counters on PMUv3p5. > > > > > > > > > > Fix tests by correctly checking overflowing-counters against the expected > > > > > 64-bit value. > > > > > > > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > > > > --- > > > > > arm/pmu.c | 29 ++++++++++++++++++----------- > > > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > > > > index cd47b14..eeac984 100644 > > > > > --- a/arm/pmu.c > > > > > +++ b/arm/pmu.c > > > > > @@ -54,10 +54,10 @@ > > > > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > > > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > > > > > > > -#define ALL_SET 0xFFFFFFFF > > > > > -#define ALL_CLEAR 0x0 > > > > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > > > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > > > > +#define ALL_SET 0x00000000FFFFFFFFULL > > > > > +#define ALL_CLEAR 0x0000000000000000ULL > > > > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > > > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > > > > > > > #define PMU_PPI 23 > > > > > > > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > > > > static void test_sw_incr(void) > > > > > { > > > > > uint32_t events[] = {SW_INCR, SW_INCR}; > > > > > + uint64_t cntr0; > > > > > int i; > > > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > > > > write_sysreg(0x3, pmswinc_el0); > > > > > > > > > > isb(); > > > > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > > > > - report(read_regn_el0(pmevcntr, 1) == 100, > > > > > - "counter #0 after + 100 SW_INCR"); > > > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > > > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > > > > > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > > > > > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > > > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > > > report(read_sysreg(pmovsclr_el0) == 0x1, > > > > > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > > > > > static void test_chained_counters(void) > > > > > { > > > > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > > > > + uint64_t cntr1; > > > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > > return; > > > > > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > > > > > > > > > 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"); > > > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > > > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); > > > > > > > > It looks to me like the intention of the test was to check that the counter > > > > programmed with the CHAIN event wraps, judging from the report message. > > > > > > > > > > Ah, right. Yeah, that message is confusing. It should be the short > > > version of "Inrementing at 32-bits resulted in the right value". > > > > > > > I think it would be interesting to keep that by programming counter #1 with > > > > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value > > > > against 0. > > > > > > The last commit adds tests using ALL_SET64. Tests can be run in two > > > modes: overflow_at_64bits and not. However, this test, > > > test_chained_counters(), and all other chained tests only use the > > > !overflow_at_64bits mode (even after the last commit). The reason is > > > that there are no CHAIN events when overflowing at 64-bits (more details > > > in the commit message). But, don't worry, there are lots of tests that > > > check wrapping at 64-bits (overflow_at_64bits=true). > > > > What I was suggesting is this (change on top of this series, not on top of > > this patch, to get access to ALL_SET_AT): > > Ooh, I see, I agree: it would be better to check that the odd counter > increments from ~0ULL to 0 when using 64-bit counters. > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > index 3cb563b1abe4..fd7f20fc6c52 100644 > > --- a/arm/pmu.c > > +++ b/arm/pmu.c > > @@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits) > > static void test_chained_counters(bool overflow_at_64bits) > > { > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > - uint64_t cntr1; > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events), > > overflow_at_64bits)) > > @@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits) > > 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, 1, ALL_SET_AT(overflow_at_64bits)); > > The only change is that this should be: > > ALL_SET_AT(pmu.version >= ID_DFR0_PMU_V3_8_5) > > Because "overflow_at_64bits" implies PMCR_EL0.LP = 1. Right, and test_chained_counters() is never called with overflow_at_64bits == true. How about renaming the parameter overflow_at_64bits -> unused (or something like that) to make it clear that the function does the same thing regardless of the value? Thanks, Alex > > > > > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E); > > report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0)); > > - cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > - report(read_regn_el0(pmevcntr, 1) == cntr1, "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"); > > } > > > > The counters are 64bit, but the CHAIN event should still work because > > PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM > > DDI 0487I.a, page D17-6461). > > > > Thanks, > > Alex > > > > > > > > > Alternatively, the report message can be modified, and "wrapped" > > > > replaced with "incremented" (or something like that), to avoid confusion. > > > > > > > > > + > > > > > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > > > > > } > > > > > > > > > > static void test_chained_sw_incr(void) > > > > > { > > > > > uint32_t events[] = {SW_INCR, CHAIN}; > > > > > + uint64_t cntr0, cntr1; > > > > > int i; > > > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > > > > > write_sysreg(0x1, pmswinc_el0); > > > > > > > > > > isb(); > > > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > > > report((read_sysreg(pmovsclr_el0) == 0x3) && > > > > > - (read_regn_el0(pmevcntr, 1) == 0) && > > > > > - (read_regn_el0(pmevcntr, 0) == 84), > > > > > - "expected overflows and values after 100 SW_INCR/CHAIN"); > > > > > + (read_regn_el0(pmevcntr, 1) == cntr0) && > > > > > + (read_regn_el0(pmevcntr, 0) == cntr1), > > > > > > > > This is hard to parse, it would be better if counter 0 is compared against > > > > cntr0 and counter 1 against cntr1. > > > > > > Ah, yes, code is correct but that's indeed confusing. > > > > > > > > > > > Also, same suggestion here, looks like the test wants to check that the > > > > odd-numbered counter wraps around when counting the CHAIN event. > > > > > > Ack. Will update for v2. > > > > > > Thanks! > > > Ricardo > > > > > > > > > > > Thanks, > > > > Alex > > > > > > > > > + "expected overflows and values after 100 SW_INCR/CHAIN"); > > > > > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > > > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > > > } > > > > > -- > > > > > 2.39.0.rc0.267.gcb52ba06e7-goog > > > > >
On Wed, Dec 14, 2022 at 10:46:05AM +0000, Alexandru Elisei wrote: > Hi, > > On Tue, Dec 13, 2022 at 10:01:06AM -0800, Ricardo Koller wrote: > > On Tue, Dec 13, 2022 at 04:43:38PM +0000, Alexandru Elisei wrote: > > > Hi, > > > > > > On Tue, Dec 13, 2022 at 08:21:24AM -0800, Ricardo Koller wrote: > > > > On Tue, Dec 13, 2022 at 12:36:14PM +0000, Alexandru Elisei wrote: > > > > > Hi, > > > > > > > > > > Some more comments below. > > > > > > > > > > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote: > > > > > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured > > > > > > for overflowing at 32 or 64-bits. The consequence is that tests that check > > > > > > the counter values after overflowing should not assume that values will be > > > > > > wrapped around 32-bits: they overflow into the other half of the 64-bit > > > > > > counters on PMUv3p5. > > > > > > > > > > > > Fix tests by correctly checking overflowing-counters against the expected > > > > > > 64-bit value. > > > > > > > > > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > > > > > --- > > > > > > arm/pmu.c | 29 ++++++++++++++++++----------- > > > > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > > > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > > > > > index cd47b14..eeac984 100644 > > > > > > --- a/arm/pmu.c > > > > > > +++ b/arm/pmu.c > > > > > > @@ -54,10 +54,10 @@ > > > > > > #define EXT_COMMON_EVENTS_LOW 0x4000 > > > > > > #define EXT_COMMON_EVENTS_HIGH 0x403F > > > > > > > > > > > > -#define ALL_SET 0xFFFFFFFF > > > > > > -#define ALL_CLEAR 0x0 > > > > > > -#define PRE_OVERFLOW 0xFFFFFFF0 > > > > > > -#define PRE_OVERFLOW2 0xFFFFFFDC > > > > > > +#define ALL_SET 0x00000000FFFFFFFFULL > > > > > > +#define ALL_CLEAR 0x0000000000000000ULL > > > > > > +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL > > > > > > +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL > > > > > > > > > > > > #define PMU_PPI 23 > > > > > > > > > > > > @@ -538,6 +538,7 @@ static void test_mem_access(void) > > > > > > static void test_sw_incr(void) > > > > > > { > > > > > > uint32_t events[] = {SW_INCR, SW_INCR}; > > > > > > + uint64_t cntr0; > > > > > > int i; > > > > > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > > > @@ -572,9 +573,9 @@ static void test_sw_incr(void) > > > > > > write_sysreg(0x3, pmswinc_el0); > > > > > > > > > > > > isb(); > > > > > > - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); > > > > > > - report(read_regn_el0(pmevcntr, 1) == 100, > > > > > > - "counter #0 after + 100 SW_INCR"); > > > > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > > > > + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); > > > > > > + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); > > > > > > report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", > > > > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > > > > report(read_sysreg(pmovsclr_el0) == 0x1, > > > > > > @@ -584,6 +585,7 @@ static void test_sw_incr(void) > > > > > > static void test_chained_counters(void) > > > > > > { > > > > > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > > > > > + uint64_t cntr1; > > > > > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > > > return; > > > > > > @@ -618,13 +620,16 @@ static void test_chained_counters(void) > > > > > > > > > > > > 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"); > > > > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > > > > + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); > > > > > > > > > > It looks to me like the intention of the test was to check that the counter > > > > > programmed with the CHAIN event wraps, judging from the report message. > > > > > > > > > > > > > Ah, right. Yeah, that message is confusing. It should be the short > > > > version of "Inrementing at 32-bits resulted in the right value". > > > > > > > > > I think it would be interesting to keep that by programming counter #1 with > > > > > ~0ULL when PMUv3p5 (maybe call it ALL_SET64?) and test the counter value > > > > > against 0. > > > > > > > > The last commit adds tests using ALL_SET64. Tests can be run in two > > > > modes: overflow_at_64bits and not. However, this test, > > > > test_chained_counters(), and all other chained tests only use the > > > > !overflow_at_64bits mode (even after the last commit). The reason is > > > > that there are no CHAIN events when overflowing at 64-bits (more details > > > > in the commit message). But, don't worry, there are lots of tests that > > > > check wrapping at 64-bits (overflow_at_64bits=true). > > > > > > What I was suggesting is this (change on top of this series, not on top of > > > this patch, to get access to ALL_SET_AT): > > > > Ooh, I see, I agree: it would be better to check that the odd counter > > increments from ~0ULL to 0 when using 64-bit counters. > > > > > > > > diff --git a/arm/pmu.c b/arm/pmu.c > > > index 3cb563b1abe4..fd7f20fc6c52 100644 > > > --- a/arm/pmu.c > > > +++ b/arm/pmu.c > > > @@ -607,7 +607,6 @@ static void test_sw_incr(bool overflow_at_64bits) > > > static void test_chained_counters(bool overflow_at_64bits) > > > { > > > uint32_t events[] = {CPU_CYCLES, CHAIN}; > > > - uint64_t cntr1; > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events), > > > overflow_at_64bits)) > > > @@ -639,12 +638,11 @@ static void test_chained_counters(bool overflow_at_64bits) > > > 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, 1, ALL_SET_AT(overflow_at_64bits)); > > > > The only change is that this should be: > > > > ALL_SET_AT(pmu.version >= ID_DFR0_PMU_V3_8_5) > > > > Because "overflow_at_64bits" implies PMCR_EL0.LP = 1. > > Right, and test_chained_counters() is never called with overflow_at_64bits > == true. > > How about renaming the parameter overflow_at_64bits -> unused (or something > like that) to make it clear that the function does the same thing > regardless of the value? Sounds good, will do for v2. > > Thanks, > Alex > > > > > > > > > precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E); > > > report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0)); > > > - cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > - report(read_regn_el0(pmevcntr, 1) == cntr1, "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"); > > > } > > > > > > The counters are 64bit, but the CHAIN event should still work because > > > PMCR_EL0.LP is 0, according to the event description in the Arm ARM (ARM > > > DDI 0487I.a, page D17-6461). > > > > > > Thanks, > > > Alex > > > > > > > > > > > > Alternatively, the report message can be modified, and "wrapped" > > > > > replaced with "incremented" (or something like that), to avoid confusion. > > > > > > > > > > > + > > > > > > report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); > > > > > > } > > > > > > > > > > > > static void test_chained_sw_incr(void) > > > > > > { > > > > > > uint32_t events[] = {SW_INCR, CHAIN}; > > > > > > + uint64_t cntr0, cntr1; > > > > > > int i; > > > > > > > > > > > > if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) > > > > > > @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) > > > > > > write_sysreg(0x1, pmswinc_el0); > > > > > > > > > > > > isb(); > > > > > > + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; > > > > > > + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; > > > > > > report((read_sysreg(pmovsclr_el0) == 0x3) && > > > > > > - (read_regn_el0(pmevcntr, 1) == 0) && > > > > > > - (read_regn_el0(pmevcntr, 0) == 84), > > > > > > - "expected overflows and values after 100 SW_INCR/CHAIN"); > > > > > > + (read_regn_el0(pmevcntr, 1) == cntr0) && > > > > > > + (read_regn_el0(pmevcntr, 0) == cntr1), > > > > > > > > > > This is hard to parse, it would be better if counter 0 is compared against > > > > > cntr0 and counter 1 against cntr1. > > > > > > > > Ah, yes, code is correct but that's indeed confusing. > > > > > > > > > > > > > > Also, same suggestion here, looks like the test wants to check that the > > > > > odd-numbered counter wraps around when counting the CHAIN event. > > > > > > > > Ack. Will update for v2. > > > > > > > > Thanks! > > > > Ricardo > > > > > > > > > > > > > > Thanks, > > > > > Alex > > > > > > > > > > > + "expected overflows and values after 100 SW_INCR/CHAIN"); > > > > > > report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), > > > > > > read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); > > > > > > } > > > > > > -- > > > > > > 2.39.0.rc0.267.gcb52ba06e7-goog > > > > > >
diff --git a/arm/pmu.c b/arm/pmu.c index cd47b14..eeac984 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -54,10 +54,10 @@ #define EXT_COMMON_EVENTS_LOW 0x4000 #define EXT_COMMON_EVENTS_HIGH 0x403F -#define ALL_SET 0xFFFFFFFF -#define ALL_CLEAR 0x0 -#define PRE_OVERFLOW 0xFFFFFFF0 -#define PRE_OVERFLOW2 0xFFFFFFDC +#define ALL_SET 0x00000000FFFFFFFFULL +#define ALL_CLEAR 0x0000000000000000ULL +#define PRE_OVERFLOW 0x00000000FFFFFFF0ULL +#define PRE_OVERFLOW2 0x00000000FFFFFFDCULL #define PMU_PPI 23 @@ -538,6 +538,7 @@ static void test_mem_access(void) static void test_sw_incr(void) { uint32_t events[] = {SW_INCR, SW_INCR}; + uint64_t cntr0; int i; if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) @@ -572,9 +573,9 @@ static void test_sw_incr(void) write_sysreg(0x3, pmswinc_el0); isb(); - report(read_regn_el0(pmevcntr, 0) == 84, "counter #1 after + 100 SW_INCR"); - report(read_regn_el0(pmevcntr, 1) == 100, - "counter #0 after + 100 SW_INCR"); + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; + report(read_regn_el0(pmevcntr, 0) == cntr0, "counter #0 after + 100 SW_INCR"); + report(read_regn_el0(pmevcntr, 1) == 100, "counter #1 after + 100 SW_INCR"); report_info("counter values after 100 SW_INCR #0=%ld #1=%ld", read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); report(read_sysreg(pmovsclr_el0) == 0x1, @@ -584,6 +585,7 @@ static void test_sw_incr(void) static void test_chained_counters(void) { uint32_t events[] = {CPU_CYCLES, CHAIN}; + uint64_t cntr1; if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) return; @@ -618,13 +620,16 @@ static void test_chained_counters(void) 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"); + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; + report(read_regn_el0(pmevcntr, 1) == cntr1, "CHAIN counter #1 wrapped"); + report(read_sysreg(pmovsclr_el0) == 0x3, "overflow on even and odd counters"); } static void test_chained_sw_incr(void) { uint32_t events[] = {SW_INCR, CHAIN}; + uint64_t cntr0, cntr1; int i; if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) @@ -665,10 +670,12 @@ static void test_chained_sw_incr(void) write_sysreg(0x1, pmswinc_el0); isb(); + cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 0 : ALL_SET + 1; + cntr1 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100; report((read_sysreg(pmovsclr_el0) == 0x3) && - (read_regn_el0(pmevcntr, 1) == 0) && - (read_regn_el0(pmevcntr, 0) == 84), - "expected overflows and values after 100 SW_INCR/CHAIN"); + (read_regn_el0(pmevcntr, 1) == cntr0) && + (read_regn_el0(pmevcntr, 0) == cntr1), + "expected overflows and values after 100 SW_INCR/CHAIN"); report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1)); }
PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured for overflowing at 32 or 64-bits. The consequence is that tests that check the counter values after overflowing should not assume that values will be wrapped around 32-bits: they overflow into the other half of the 64-bit counters on PMUv3p5. Fix tests by correctly checking overflowing-counters against the expected 64-bit value. Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arm/pmu.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)