On 6 December 2014 at 21:01, Chengyu Song <cson...@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 <cson...@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