mbox series

[v3,00/22] More fully implement ARM PMUv3

Message ID 1521232280-13089-1-git-send-email-alindsay@codeaurora.org
Headers show
Series More fully implement ARM PMUv3 | expand

Message

Aaron Lindsay March 16, 2018, 8:30 p.m. UTC 
The ARM PMU implementation currently contains a basic cycle counter, but it is
often useful to gather counts of other events and filter them based on
execution mode. These patches flesh out the implementations of various PMU
registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
represent arbitrary counter types, implement mode filtering, and add
instruction, cycle, and software increment events.

I aim to eventually add raising interrupts on counter overflow, but that is not
covered by this patchset. I think I have a reasonable grasp of the mechanics of
*how* to raise them, but am curious if anyone has thoughts on how to determine
*when* to raise them - we don't want to call into PMU code every time an
instruction is executed to check if any instruction counters have overflowed,
etc. The main candidate I've seen for doing this so far would be to set up a
QEMUTimer, but I haven't fully explored it. Does that seem plausible? Any
other/better ideas?


Changes from v2:

* Many minor fixups (splitting patches, style/comment fixes, etc.)
* Save off current cycle count during operations which may change whether a
  counter is enabled, ensuring time isn't lost (update to patch 5)
* Added the ability to have more than one el_change_hook, added hooks before EL
  changes (patches 7-9)
* Added proper handling of can_do_io during code-gen before and after the
  el_change_hooks (patch 8)
* Split off PMOVSSET register definitions so they are only enabled for v7ve+
  (added patch 15, update to 16)

Thanks for any feedback,
Aaron
 

Aaron Lindsay (22):
  target/arm: A53: Initialize PMCEID[01]
  target/arm: A15 PMCEID0 initialization style nit
  target/arm: Check PMCNTEN for whether PMCCNTR is enabled
  target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
  target/arm: Reorganize PMCCNTR read, write, sync
  target/arm: Mask PMU register writes based on PMCR_EL0.N
  target/arm: Fetch GICv3 state directly from CPUARMState
  target/arm: Support multiple EL change hooks
  target/arm: Add pre-EL change hooks
  target/arm: Allow EL change hooks to do IO
  target/arm: Fix bitmask for PMCCFILTR writes
  target/arm: Filter cycle counter based on PMCCFILTR_EL0
  target/arm: Allow AArch32 access for PMCCFILTR
  target/arm: Make PMOVSCLR 64 bits wide
  target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
  target/arm: Implement PMOVSSET
  target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
  target/arm: Add array for supported PMU events, generate PMCEID[01]
  target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
  target/arm: PMU: Add instruction and cycle events
  target/arm: PMU: Set PMCR.N to 4
  target/arm: Implement PMSWINC

 hw/intc/arm_gicv3_cpuif.c  |  10 +-
 target/arm/cpu.c           |  40 ++-
 target/arm/cpu.h           | 102 +++++---
 target/arm/cpu64.c         |   2 +
 target/arm/helper.c        | 616 ++++++++++++++++++++++++++++++++++++++-------
 target/arm/internals.h     |  14 +-
 target/arm/op_helper.c     |   8 +
 target/arm/translate-a64.c |   2 +
 target/arm/translate.c     |   4 +
 9 files changed, 651 insertions(+), 147 deletions(-)

Comments

no-reply@patchew.org March 16, 2018, 8:58 p.m. UTC | #1 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1521232280-13089-1-git-send-email-alindsay@codeaurora.org
Subject: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org -> patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org
Switched to a new branch 'test'
d81791b184 target/arm: Implement PMSWINC
512124d018 target/arm: PMU: Set PMCR.N to 4
b748e97306 target/arm: PMU: Add instruction and cycle events
7e46a77f89 target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
82cffef855 target/arm: Add array for supported PMU events, generate PMCEID[01]
d635021d0a target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
3e1b438deb target/arm: Implement PMOVSSET
c13c832988 target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
9c80af4d82 target/arm: Make PMOVSCLR 64 bits wide
8ffbcd3dd8 target/arm: Allow AArch32 access for PMCCFILTR
7dc4ec9715 target/arm: Filter cycle counter based on PMCCFILTR_EL0
f1e40f9fff target/arm: Fix bitmask for PMCCFILTR writes
eb142b42b4 target/arm: Allow EL change hooks to do IO
d37186abdd target/arm: Add pre-EL change hooks
ae12e161da target/arm: Support multiple EL change hooks
9bfa99805d target/arm: Fetch GICv3 state directly from CPUARMState
881bee5e96 target/arm: Mask PMU register writes based on PMCR_EL0.N
890ad6472b target/arm: Reorganize PMCCNTR read, write, sync
922eec023b target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
5b1ce33c0b target/arm: Check PMCNTEN for whether PMCCNTR is enabled
2ecb09ae04 target/arm: A15 PMCEID0 initialization style nit
0f26a1568a target/arm: A53: Initialize PMCEID[01]

=== OUTPUT BEGIN ===
Checking PATCH 1/22: target/arm: A53: Initialize PMCEID[01]...
Checking PATCH 2/22: target/arm: A15 PMCEID0 initialization style nit...
Checking PATCH 3/22: target/arm: Check PMCNTEN for whether PMCCNTR is enabled...
Checking PATCH 4/22: target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0...
Checking PATCH 5/22: target/arm: Reorganize PMCCNTR read, write, sync...
Checking PATCH 6/22: target/arm: Mask PMU register writes based on PMCR_EL0.N...
Checking PATCH 7/22: target/arm: Fetch GICv3 state directly from CPUARMState...
Checking PATCH 8/22: target/arm: Support multiple EL change hooks...
ERROR: space prohibited between function name and open parenthesis '('
#26: FILE: target/arm/cpu.c:62:
+    entry = g_malloc0(sizeof (*entry));

total: 1 errors, 0 warnings, 87 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 9/22: target/arm: Add pre-EL change hooks...
ERROR: space prohibited between function name and open parenthesis '('
#26: FILE: target/arm/cpu.c:62:
+    entry = g_malloc0(sizeof (*entry));

total: 1 errors, 0 warnings, 110 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 10/22: target/arm: Allow EL change hooks to do IO...
Checking PATCH 11/22: target/arm: Fix bitmask for PMCCFILTR writes...
Checking PATCH 12/22: target/arm: Filter cycle counter based on PMCCFILTR_EL0...
Checking PATCH 13/22: target/arm: Allow AArch32 access for PMCCFILTR...
Checking PATCH 14/22: target/arm: Make PMOVSCLR 64 bits wide...
Checking PATCH 15/22: target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions...
Checking PATCH 16/22: target/arm: Implement PMOVSSET...
WARNING: line over 80 characters
#39: FILE: target/arm/helper.c:1417:
+      .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),

total: 0 errors, 1 warnings, 59 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 17/22: target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled...
Checking PATCH 18/22: target/arm: Add array for supported PMU events, generate PMCEID[01]...
Checking PATCH 19/22: target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER...
Checking PATCH 20/22: target/arm: PMU: Add instruction and cycle events...
Checking PATCH 21/22: target/arm: PMU: Set PMCR.N to 4...
Checking PATCH 22/22: target/arm: Implement PMSWINC...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Aaron Lindsay March 17, 2018, 12:01 a.m. UTC | #2 
My apologies for the below style issues - I've already fixed them up for
v4...

-Aaron

On Mar 16 13:58, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 1521232280-13089-1-git-send-email-alindsay@codeaurora.org
> Subject: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]               patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org -> patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org
> Switched to a new branch 'test'
> d81791b184 target/arm: Implement PMSWINC
> 512124d018 target/arm: PMU: Set PMCR.N to 4
> b748e97306 target/arm: PMU: Add instruction and cycle events
> 7e46a77f89 target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
> 82cffef855 target/arm: Add array for supported PMU events, generate PMCEID[01]
> d635021d0a target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
> 3e1b438deb target/arm: Implement PMOVSSET
> c13c832988 target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
> 9c80af4d82 target/arm: Make PMOVSCLR 64 bits wide
> 8ffbcd3dd8 target/arm: Allow AArch32 access for PMCCFILTR
> 7dc4ec9715 target/arm: Filter cycle counter based on PMCCFILTR_EL0
> f1e40f9fff target/arm: Fix bitmask for PMCCFILTR writes
> eb142b42b4 target/arm: Allow EL change hooks to do IO
> d37186abdd target/arm: Add pre-EL change hooks
> ae12e161da target/arm: Support multiple EL change hooks
> 9bfa99805d target/arm: Fetch GICv3 state directly from CPUARMState
> 881bee5e96 target/arm: Mask PMU register writes based on PMCR_EL0.N
> 890ad6472b target/arm: Reorganize PMCCNTR read, write, sync
> 922eec023b target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
> 5b1ce33c0b target/arm: Check PMCNTEN for whether PMCCNTR is enabled
> 2ecb09ae04 target/arm: A15 PMCEID0 initialization style nit
> 0f26a1568a target/arm: A53: Initialize PMCEID[01]
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/22: target/arm: A53: Initialize PMCEID[01]...
> Checking PATCH 2/22: target/arm: A15 PMCEID0 initialization style nit...
> Checking PATCH 3/22: target/arm: Check PMCNTEN for whether PMCCNTR is enabled...
> Checking PATCH 4/22: target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0...
> Checking PATCH 5/22: target/arm: Reorganize PMCCNTR read, write, sync...
> Checking PATCH 6/22: target/arm: Mask PMU register writes based on PMCR_EL0.N...
> Checking PATCH 7/22: target/arm: Fetch GICv3 state directly from CPUARMState...
> Checking PATCH 8/22: target/arm: Support multiple EL change hooks...
> ERROR: space prohibited between function name and open parenthesis '('
> #26: FILE: target/arm/cpu.c:62:
> +    entry = g_malloc0(sizeof (*entry));
> 
> total: 1 errors, 0 warnings, 87 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 9/22: target/arm: Add pre-EL change hooks...
> ERROR: space prohibited between function name and open parenthesis '('
> #26: FILE: target/arm/cpu.c:62:
> +    entry = g_malloc0(sizeof (*entry));
> 
> total: 1 errors, 0 warnings, 110 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 10/22: target/arm: Allow EL change hooks to do IO...
> Checking PATCH 11/22: target/arm: Fix bitmask for PMCCFILTR writes...
> Checking PATCH 12/22: target/arm: Filter cycle counter based on PMCCFILTR_EL0...
> Checking PATCH 13/22: target/arm: Allow AArch32 access for PMCCFILTR...
> Checking PATCH 14/22: target/arm: Make PMOVSCLR 64 bits wide...
> Checking PATCH 15/22: target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions...
> Checking PATCH 16/22: target/arm: Implement PMOVSSET...
> WARNING: line over 80 characters
> #39: FILE: target/arm/helper.c:1417:
> +      .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> 
> total: 0 errors, 1 warnings, 59 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> Checking PATCH 17/22: target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled...
> Checking PATCH 18/22: target/arm: Add array for supported PMU events, generate PMCEID[01]...
> Checking PATCH 19/22: target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER...
> Checking PATCH 20/22: target/arm: PMU: Add instruction and cycle events...
> Checking PATCH 21/22: target/arm: PMU: Set PMCR.N to 4...
> Checking PATCH 22/22: target/arm: Implement PMSWINC...
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org
Peter Maydell April 12, 2018, 5:32 p.m. UTC | #3 
On 16 March 2018 at 20:30, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> The ARM PMU implementation currently contains a basic cycle counter, but it is
> often useful to gather counts of other events and filter them based on
> execution mode. These patches flesh out the implementations of various PMU
> registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
> represent arbitrary counter types, implement mode filtering, and add
> instruction, cycle, and software increment events.

Hi; sorry it's taken me a while to get to this (I've been focusing
on for-2.12 work). I've now reviewed most of the patches in this
set. My suggestion is that you fix the stuff I've commented on
and send out a v4, and then I can take some of the patches from
the start of that into target-arm.next, and I'll review the tail
end of the set then.

thanks
-- PMM
Aaron Lindsay April 12, 2018, 7:34 p.m. UTC | #4 
On Apr 12 18:32, Peter Maydell wrote:
> On 16 March 2018 at 20:30, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > The ARM PMU implementation currently contains a basic cycle counter, but it is
> > often useful to gather counts of other events and filter them based on
> > execution mode. These patches flesh out the implementations of various PMU
> > registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
> > represent arbitrary counter types, implement mode filtering, and add
> > instruction, cycle, and software increment events.
> 
> Hi; sorry it's taken me a while to get to this (I've been focusing
> on for-2.12 work). I've now reviewed most of the patches in this
> set. My suggestion is that you fix the stuff I've commented on
> and send out a v4, and then I can take some of the patches from
> the start of that into target-arm.next, and I'll review the tail
> end of the set then.

Thanks for the review! I'll see if I can get a v4 out late this week or
early next.

I'll make a note of this when I send it out, but one thing to look out
for in the next version is that I had to reorganize a few things to make
the interrupt-on-overflow functionality work better. I think the changes
are fairly minor and limited to the later patches, with the possible
exception of using two variables per counter (because you have to know
what the previous counter value was in addition to the current value to
know if you've overflowed since your last check).

-Aaron