From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58967) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTHfd-0005vX-Rs for qemu-devel@nongnu.org; Tue, 09 Feb 2016 18:26:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTHfc-0005Ft-33 for qemu-devel@nongnu.org; Tue, 09 Feb 2016 18:26:09 -0500 Received: from mail-ob0-x243.google.com ([2607:f8b0:4003:c01::243]:36105) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTHfb-0005FY-Sg for qemu-devel@nongnu.org; Tue, 09 Feb 2016 18:26:08 -0500 Received: by mail-ob0-x243.google.com with SMTP id wg8so245047obc.3 for ; Tue, 09 Feb 2016 15:26:07 -0800 (PST) MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: References: <309860ef711e07dc7eee7133c4dd228cd8c343b7.1454720020.git.alistair.francis@xilinx.com> From: Alistair Francis Date: Tue, 9 Feb 2016 15:25:37 -0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v2 2/5] target-arm: Add Some of the performance monitor registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Aaron Lindsay , QEMU Developers , Nathan Rossi , Peter Crosthwaite , Christopher Covington , Alistair Francis On Tue, Feb 9, 2016 at 9:32 AM, Peter Maydell wrote: > On 6 February 2016 at 00:55, Alistair Francis > wrote: >> This patch adds the following registers including read and write functions: >> PMSELR, PMSELR_EL0, PMXEVCNTR, PMXEVCNTR_EL0, PMXEVTYPER and PMXEVTYPER_EL0. >> >> Signed-off-by: Aaron Lindsay >> Signed-off-by: Alistair Francis >> Tested-by: Nathan Rossi > > Is there any benefit to implementing all the type and counter > registers and the multiplex selection, when we don't actually > support any counters yet? Thanks for reviewing these. The others (patch 1, 3 and 4) are more important to me, so I'm sending them out so they can be applied then I'll look at these after. Thanks, Alistair > >> --- >> >> target-arm/cpu.h | 6 ++++ >> target-arm/helper.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++------- >> 2 files changed, 84 insertions(+), 11 deletions(-) >> >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index b8b3364..5c31c56 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -117,6 +117,8 @@ typedef struct ARMGenericTimer { >> #define GTIMER_SEC 3 >> #define NUM_GTIMERS 4 >> >> +#define NUM_PMU_COUNTERS 4 >> + >> typedef struct { >> uint64_t raw_tcr; >> uint32_t mask; >> @@ -300,6 +302,7 @@ typedef struct CPUARMState { >> uint32_t c9_pmxevtyper; /* perf monitor event type */ >> uint32_t c9_pmuserenr; /* perf monitor user enable */ >> uint32_t c9_pminten; /* perf monitor interrupt enables */ >> + uint32_t c9_pmselr; /* perf monitor event counter selection */ >> union { /* Memory attribute redirection */ >> struct { >> #ifdef HOST_WORDS_BIGENDIAN >> @@ -361,6 +364,9 @@ typedef struct CPUARMState { >> uint64_t tpidruro_ns; >> uint64_t tpidrro_el[1]; >> }; >> + uint32_t c14_pmccfiltr; /* Performance Monitor Filter Register */ >> + uint32_t c14_pmevcntr[NUM_PMU_COUNTERS]; >> + uint32_t c14_pmevtyper[NUM_PMU_COUNTERS]; >> uint64_t c14_cntfrq; /* Counter Frequency register */ >> uint64_t c14_cntkctl; /* Timer Control register */ >> uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */ >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 66aa406..164853f 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -807,6 +807,58 @@ void pmccntr_sync(CPUARMState *env) >> >> #endif >> >> +static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri, >> + uint64_t value) >> +{ >> + env->cp15.c9_pmselr = value & 31; >> +} >> + >> +static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri, >> + const uint8_t idx) >> +{ >> + if (idx >= NUM_PMU_COUNTERS) { >> + return arm_cp_read_zero(env, ri); >> + } >> + return env->cp15.c14_pmevcntr[idx]; >> +} >> + >> +static uint64_t pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri) >> +{ >> + return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31); >> +} >> + >> +static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri, >> + uint64_t value, const uint8_t idx) >> +{ >> + if (idx >= NUM_PMU_COUNTERS) { >> + return arm_cp_write_ignore(env, ri, value); > > arm_cp_write_ignore() does nothing, as you might expect from the name, > so why call it? > > You might mention that this is CONSTRAINED UNPREDICTABLE and we > are choosing to RAZ/WI. > >> + } >> + env->cp15.c14_pmevcntr[idx] = value; >> +} >> + >> +static void pmxevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri, >> + uint64_t value) >> +{ >> + pmevcntr_write(env, ri, value, env->cp15.c9_pmselr & 31); >> +} >> + >> +static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri, >> + const uint8_t idx) >> +{ >> + if (idx >= NUM_PMU_COUNTERS) { >> + return arm_cp_read_zero(env, ri); > > You could just return 0. > >> + } >> + return env->cp15.c14_pmevtyper[idx]; >> +} >> + >> +static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri) >> +{ >> + if (!!(env->cp15.c9_pmselr & 31)) { >> + return env->cp15.c14_pmccfiltr; >> + } >> + return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31); >> +} >> + >> static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, >> uint64_t value) >> { >> @@ -986,8 +1038,32 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { >> * 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 }, >> + .access = PL0_RW, .type = ARM_CP_ALIAS, >> + .accessfn = pmreg_access, .writefn = pmselr_write, >> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr) }, >> + { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64, >> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5, >> + .access = PL0_RW, .accessfn = pmreg_access, >> + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr), >> + .writefn = pmselr_write, .resetvalue = 0 }, >> + { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2, >> + .access = PL0_RW, .type = ARM_CP_ALIAS, >> + .accessfn = pmreg_access, .writefn = pmxevcntr_write, >> + .readfn = pmxevcntr_read }, >> + { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64, >> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2, >> + .access = PL0_RW, .type = ARM_CP_ALIAS, >> + .accessfn = pmreg_access, .writefn = pmxevcntr_write, >> + .readfn = pmxevcntr_read }, >> + { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, >> + .access = PL0_RW, .type = ARM_CP_ALIAS, >> + .accessfn = pmreg_access, .writefn = pmxevtyper_write, >> + .readfn = pmxevtyper_read }, >> + { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64, >> + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1, >> + .access = PL0_RW, .type = ARM_CP_ALIAS, >> + .accessfn = pmreg_access, .writefn = pmxevtyper_write, >> + .readfn = pmxevtyper_read }, > > This isn't going to handle migration correctly, because there are > a whole pile of data values behind these registers. > > It may be easiest to implement the PMEVTYPER_EL0 and PMEVCNTR_EL0 > registers (which are PMUv3 only and provide un-multiplexed access to > the type and counter registers), gate them so they're present but not > guest-accessible on PMUv2 and below, and use them for migrating the > state. Then the X multiplexed registers can be simple CP_ALIASes as > you have them here. > >> #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, >> @@ -1006,15 +1082,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { >> .type = ARM_CP_IO, >> .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), >> .resetvalue = 0, }, >> - { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .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 = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0, >> .access = PL0_R | PL1_RW, >> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr), >> -- >> 2.5.0 >> > > thanks > -- PMM >