* [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode
@ 2014-12-06 21:01 Chengyu Song
2014-12-08 16:30 ` Peter Maydell
0 siblings, 1 reply; 4+ messages in thread
From: Chengyu Song @ 2014-12-06 21:01 UTC (permalink / raw)
To: qemu-devel; +Cc: Chengyu Song
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.
Signed-off-by: Chengyu Song <csong84@gatech.edu>
---
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;
}
+static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+ env->cp15.c9_pmovsr |= value;
+}
+
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,
.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,
+ .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,
+ .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+ .accessfn = pmreg_access,
+ .writefn = pmovsset_write,
+ .raw_writefn = raw_write,
+ .type = ARM_CP_NO_MIGRATE },
+ { .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 },
/* 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 },
/* 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 },
#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,
+ .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 },
/* 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 },
{ .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 },
{ .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 },
@@ -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, },
{ .name = "VBAR", .state = ARM_CP_STATE_BOTH,
.opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
.access = PL1_RW, .writefn = vbar_write,
--
2.1.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode
2014-12-06 21:01 [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode Chengyu Song
@ 2014-12-08 16:30 ` Peter Maydell
2014-12-08 20:14 ` Chengyu Song
0 siblings, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2014-12-08 16:30 UTC (permalink / raw)
To: Chengyu Song; +Cc: QEMU Developers
On 6 December 2014 at 21:01, Chengyu Song <csong84@gatech.edu> 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 <csong84@gatech.edu>
> ---
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode
2014-12-08 16:30 ` Peter Maydell
@ 2014-12-08 20:14 ` Chengyu Song
2014-12-08 21:00 ` Peter Maydell
0 siblings, 1 reply; 4+ messages in thread
From: Chengyu Song @ 2014-12-08 20:14 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers
Hello Peter,
> 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.
I'm not very familiar with the CP emulation, especially the NO_MIGRATE flag, so thank you very much for the comment.
> 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
According to the manual, all these registers are remaining as 32-bit registers, so there is no need to extend them to unit64_t.
>
>> +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.)
Agree. Because QEMU does not support events now, the rest bit should be RAZ/WI. But in the pmovsr_write function, there is no such masking, so I'm not sure if this is necessary. Could you confirm this? If so, I will fix that function as well.
>> +}
>> +
>> 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.
So in general, if one underlying state is accessed through multiple registers, then one register should be able to migrate and all other should not, is this understand correct? In my V1 patch, I didn't add the NO_MIGRATE flag and Christopher suggested I should add this flag, so I added.
> Please keep to the order cp/opc1/crn/crm/opc2.
>
> The PMOVSSET register doesn't exist in ARMv7, so this stanza
> needs to go into the v8_cp_reginfo[] array.
Let me check the v7 manual and move v8 registers to the correct array.
>> + { .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?
Yes, I can. This is the AA32 mapping of the register, so what I was thinking is to make AA32 registers in the old form cp/crn/crm/opc1/opc2, and use the new cp(opc0)/opc1/crn/crm/opc2 form for AA64 registers.
> 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.
I see. So the old code already has the problem, and it's better to fix it as well.
>> { .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 },
I'm not quite sure about this, because each register should only be accessible in its corresponding mode: PMINTENSET in AA32, and PMINTENSET_EL1 in AA64. Wouldn't setting STATE_BOTH allow accessing PMINTENSET_EL1 in AA32 mode?
Thanks,
Chengyu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode
2014-12-08 20:14 ` Chengyu Song
@ 2014-12-08 21:00 ` Peter Maydell
0 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2014-12-08 21:00 UTC (permalink / raw)
To: Chengyu Song; +Cc: QEMU Developers
On 8 December 2014 at 20:14, Chengyu Song <csong84@gatech.edu> wrote:
> Hello Peter,
>
>> 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.
>
> I'm not very familiar with the CP emulation, especially the NO_MIGRATE
> flag, so thank you very much for the comment.
>
>> 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
>
> According to the manual, all these registers are remaining as 32-bit
> registers, so there is no need to extend them to unit64_t.
No, they must be extended. Where the ARM ARM describes an AArch64
register as "32 bit" this is a shorthand for "64 bit register where
the top 32 bits are RES0". In QEMU we always implement all AArch64
registers by doing 64 bit accesses to their CPUARMState fields,
and so the fields must be 64 bits wide or we will read/write to
a neighbouring field as well.
>>> +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.)
>
> Agree. Because QEMU does not support events now, the rest bit should
> be RAZ/WI. But in the pmovsr_write function, there is no such masking,
> so I'm not sure if this is necessary.
As I say in the bracketed comment above, we don't need to mask
in pmovsr_write: the lower bits in the register are already
zero and so even if the "value" written by the guest is non-zero
in those bits the "&" operation will not be able to set them.
>>> +}
>>> +
>>> 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.
>
> So in general, if one underlying state is accessed through multiple
> registers, then one register should be able to migrate and all other
> should not, is this understand correct?
Yes, that's correct. The general approach we use is to have the
one register that migrates be the 64-bit version, with the 32
bit version marked NO_MIGRATE.
>> Please keep to the order cp/opc1/crn/crm/opc2.
>>
>> The PMOVSSET register doesn't exist in ARMv7, so this stanza
>> needs to go into the v8_cp_reginfo[] array.
>
> Let me check the v7 manual and move v8 registers to the correct array.
>
>>> + { .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?
>
> Yes, I can. This is the AA32 mapping of the register, so what
> I was thinking is to make AA32 registers in the old form
> cp/crn/crm/opc1/opc2, and use the new cp(opc0)/opc1/crn/crm/opc2
> form for AA64 registers.
We've settled on using the same order for both. If you look at
the v8 ARM ARM you'll see that the documentation also uses this
same order for AArch32 registers. (Some older documentation had
a different order, which is partly why QEMU is inconsistent.)
>> 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.
>
> I see. So the old code already has the problem, and it's better
> to fix it as well.
Yes, sort of -- but it's not an actual bug, since PMCCFILTR_EL0
is an AArch64 register and there are no CPUs before v8 which can
have those registers. It is conceptually in the wrong place, though.
>>> { .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 },
>
> I'm not quite sure about this, because each register should only
> be accessible in its corresponding mode: PMINTENSET in AA32,
> and PMINTENSET_EL1 in AA64. Wouldn't setting STATE_BOTH allow
> accessing PMINTENSET_EL1 in AA32 mode?
The point is that both these registers are identical: they have
the same access rights, field offset, reset value and write
access functions. Using STATE_BOTH is exactly the same as
having two separate structs. (The .name field will end up being
the same in both cases, but we use that only for debugging QEMU,
so it doesn't matter that it's not strictly the architectural
name.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-08 21:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-06 21:01 [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode Chengyu Song
2014-12-08 16:30 ` Peter Maydell
2014-12-08 20:14 ` Chengyu Song
2014-12-08 21:00 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.