[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2] support access to more performance registers
|
From: |
Peter Maydell |
|
Subject: |
Re: [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode |
|
Date: |
Mon, 8 Dec 2014 16:30:19 +0000 |
On 6 December 2014 at 21:01, Chengyu Song <address@hidden> wrote:
> In AA64 mode, certain system registers are access through MSR/MRS
> instructions instead of MCR/MRC. This patch added more such registers:
>
> /* ARMv8 manual rev A.d, D7.4.10 */
> PMINTENCLR_EL1
>
> /* ARMv8 manual rev A.d, D7.4.11 */
> PMINTENSET_EL1
>
> /* ARMv8 manual rev A.d, D7.4.12 */
> PMOVSCLR_EL0
>
> /* ARMv8 manual rev A.d, D7.4.13 */
> PMOVSSET_EL0
>
> /* ARMv8 manual rev A.d, D7.4.14 */
> PMSELR_EL0
>
> /* ARMv8 manual rev A.d, D7.4.15 */
> PMSWINC_EL0
>
> /* ARMv8 manual rev A.d, D7.4.16 */
> PMUSERENR_EL0
>
> /* ARMv8 manual rev A.d, D7.4.17 */
> PMXEVCNTR_EL0
>
> /* ARMv8 manual rev A.d, D7.4.18 */
> PMXEVTYPER_EL0
>
> It also added two AA32 registers:
>
> /* ARMv8 manual rev A.d, G6.4.13 */
> PMCCFILTR
>
> /* ARMv8 manual rev A.d, G6.4.13 */
> PMOVSSET
>
> Finally, it also fixed ARM_CP_NO_MIGRATE for some registers.
Thanks for sending this patch. I've provided some detailed
review below -- there are quite a lot of comments but the
fixes needed are all very minor.
> Signed-off-by: Chengyu Song <address@hidden>
> ---
> target-arm/helper.c | 70
> +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b74d348..a87a185 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -708,6 +708,12 @@ static void pmovsr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> env->cp15.c9_pmovsr &= ~value;
> }
In this patchset you add 64-bit accessors to several registers
which were previously 32-bit only. For this to work you need
to change the fields in CPUARMState from uint32_t to uint64_t for:
c9_pmovsr
c9_pmxevtyper
c9_pmuserenr
c9_pminten
> +static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t value)
> +{
> + env->cp15.c9_pmovsr |= value;
You need "value &= (1 << 31);" first, because the bits
[30:0] are RAZ/WI. (We can get away without this in the clear-bits
accessor because the lower bits in the register are already zero.)
> +}
> +
> static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t value)
> {
> @@ -824,6 +830,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> { .name = "PMCNTENSET_EL0", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 1,
> .access = PL0_RW, .accessfn = pmreg_access,
> + .type = ARM_CP_NO_MIGRATE,
I don't think this is correct -- this is the struct that migrates
the PMCNTEN state. The other registers (32-bit accessors and the
64-bit clear accessor) are all accessing the same underlying state
as this one, so they are marked NO_MIGRATE, but one register in
the group has to be migrated.
> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), .resetvalue = 0,
> .writefn = pmcntenset_write, .raw_writefn = raw_write },
> { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2
> = 2,
> @@ -842,16 +849,42 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> .accessfn = pmreg_access,
> .writefn = pmovsr_write,
> - .raw_writefn = raw_write },
> + .raw_writefn = raw_write,
> + .type = ARM_CP_NO_MIGRATE },
> + { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3,
> + .access = PL0_RW, .accessfn = pmreg_access,
> + .type = ARM_CP_NO_MIGRATE,
This shouldn't have the ARM_CP_NO_MIGRATE flag, because it's
the one that lets us migrate the state. (We can't use the SET
stanza as we do with most of the other set/clr register pairs,
because ARMv7 doesn't have the set register.)
> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> + .writefn = pmovsr_write, .raw_writefn = raw_write },
> + { .name = "PMOVSSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 =
> 3,
Please keep to the order cp/opc1/crn/crm/opc2.
> + .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> + .accessfn = pmreg_access,
> + .writefn = pmovsset_write,
> + .raw_writefn = raw_write,
> + .type = ARM_CP_NO_MIGRATE },
The PMOVSSET register doesn't exist in ARMv7, so this stanza
needs to go into the v8_cp_reginfo[] array.
> + { .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3,
> + .access = PL0_RW, .accessfn = pmreg_access,
> + .type = ARM_CP_NO_MIGRATE,
> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> + .writefn = pmovsset_write, .raw_writefn = raw_write },
OK, but please put this in the v8_cp_reginfo[] array next to
PMOVSSET, so that all our handling of PMOVSSET registers is in
one place.
> /* Unimplemented so WI. */
> { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
> .access = PL0_W, .accessfn = pmreg_access, .type = ARM_CP_NOP },
> + { .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 4,
> + .access = PL0_W, .accessfn = pmreg_access, .type = ARM_CP_NOP },
OK.
> /* Since we don't implement any events, writing to PMSELR is
> UNPREDICTABLE.
> * We choose to RAZ/WI.
> */
> { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> .accessfn = pmreg_access },
> + { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
> + .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> + .accessfn = pmreg_access },
OK
> #ifndef CONFIG_USER_ONLY
> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> @@ -863,6 +896,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> .type = ARM_CP_IO,
> .readfn = pmccntr_read, .writefn = pmccntr_write, },
> #endif
> + { .name = "PMCCFILTR", .cp = 15, .crn = 14, .crm = 15, .opc1 = 3, .opc2
> = 7,
The ARM ARM says this is opc1 = 0. Also could you order these
fields cp/opc1/crn/crm/opc2, please?
As with PMOVSSET, this register isn't present in ARMv7,
so this stanza needs to be in v8_cp_reginfo[]. I suggest you
move the existing PMCCFILTR_EL0 along with it, so we keep all
the PMCCFILTR handling in one place.
> + .writefn = pmccfiltr_write,
> + .access = PL0_RW, .accessfn = pmreg_access,
> + .type = ARM_CP_IO,
> + .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
> + .resetvalue = 0, },
> { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
> .writefn = pmccfiltr_write,
> @@ -875,17 +914,39 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> .accessfn = pmreg_access, .writefn = pmxevtyper_write,
> .raw_writefn = raw_write },
> + { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1,
> + .access = PL0_RW,
> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> + .accessfn = pmreg_access, .writefn = pmxevtyper_write,
> + .raw_writefn = raw_write },
OK.
> /* Unimplemented, RAZ/WI. */
> { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 =
> 2,
> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> .accessfn = pmreg_access },
> + { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2,
> + .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> + .accessfn = pmreg_access },
OK.
> { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 =
> 0,
> .access = PL0_R | PL1_RW,
> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
> .resetvalue = 0,
> .writefn = pmuserenr_write, .raw_writefn = raw_write },
> + { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
> + .access = PL0_R | PL1_RW,
> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
> + .resetvalue = 0,
> + .writefn = pmuserenr_write, .raw_writefn = raw_write },
OK.
> { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2
> = 1,
> - .access = PL1_RW,
> + .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> + .resetvalue = 0,
> + .writefn = pmintenset_write, .raw_writefn = raw_write },
> + { .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 1,
> + .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
> .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> .resetvalue = 0,
> .writefn = pmintenset_write, .raw_writefn = raw_write },
You can do these two with a single struct, because the
encodings line up nicely:
{ .name = "PMINTENSET", .state = ARM_CP_STATE_BOTH,
.cp = 15, .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 1,
.access = PL1_RW,
.fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
.resetvalue = 0,
.writefn = pmintenset_write, .raw_writefn = raw_write },
> @@ -893,6 +954,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
> .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> .resetvalue = 0, .writefn = pmintenclr_write, },
> + { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
> + .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14,.opc2 = 2,
> + .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> + .resetvalue = 0, .writefn = pmintenclr_write, },
Similarly you can merge this with the existing PMINTENCLR to
give:
{ .name = "PMINTENCLR", .state = ARM_CP_STATE_BOTH,
.cp = 15, .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
.access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
.fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
.resetvalue = 0, .writefn = pmintenclr_write, },
> { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
> .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
> .access = PL1_RW, .writefn = vbar_write,
For the record, while I was looking at this patch I noticed
the following problems with our current perf monitor implementation:
(1) we don't actually implement setting the PMOVS bit on overflow
(2) we don't allow access to the PMCCFILTR via PMXEVTYPER
(3) we don't implement PMCEID0/1
However none of these are new in ARMv8, so I'm happy to
just put them on my list of "minor things that are wrong
in QEMU" until somebody finds they have a guest that needs
them fixing.
thanks
-- PMM