| Message ID | b392c38bcf744556a340efaedd30cd8c6ce726bc.1389846449.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 16 January 2014 04:31, Alistair Francis <alistair.francis@xilinx.com> wrote: > This patch implements the ARM PMCCNTR register including > the disable and reset components of the PMCR register. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > This patch assumes that non-invasive debugging is not permitted > when determining if the counter is disabled > > target-arm/cpu.c | 2 ++ > target-arm/cpu.h | 3 +++ > target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++++++--- > target-arm/helper.h | 9 +++++++++ > 4 files changed, 54 insertions(+), 3 deletions(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 52efd5d..a185959 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -76,6 +76,8 @@ static void arm_cpu_reset(CPUState *s) > > acc->parent_reset(s); > > + env->emm_time = 0; > + > memset(env, 0, offsetof(CPUARMState, breakpoints)); > g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu); > env->vfp.xregs[ARM_VFP_FPSID] = cpu->reset_fpsid; > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 198b6b8..8c73a56 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -215,8 +215,11 @@ typedef struct CPUARMState { > uint32_t c15_diagnostic; /* diagnostic register */ > uint32_t c15_power_diagnostic; > uint32_t c15_power_control; /* power control */ > + uint32_t c15_ccnt; /* Performance Monitors Cycle Count */ > } cp15; > > + int64_t emm_time; /* Used to hold the total emmulation time - in ns */ This looks bogus. You don't need to save the total emulation time, because you can always get it by calling qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL). (Also it won't get migrated, but the correct fix for that is to get rid of it, see below.) > + > /* System registers (AArch64) */ > struct { > uint64_t tpidr_el0; > diff --git a/target-arm/helper.c b/target-arm/helper.c > index c708f15..72404cf 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -505,9 +505,16 @@ static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, > if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) { > return EXCP_UDEF; > } > - /* only the DP, X, D and E bits are writable */ > + /* only the DP, X, D and E bits are writeable */ "writable" is a valid spelling and a quick grep shows it's actually used much more in qemu than "writeable". > env->cp15.c9_pmcr &= ~0x39; > env->cp15.c9_pmcr |= (value & 0x39); > + > + if (value & PMCRC) { > + /* Reset the counter */ > + env->emm_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + env->cp15.c15_ccnt = 0; This is the wrong way to do this kind of thing. Don't store the actual value of a resettable count register; store the difference between the CLOCK_VIRTUAL count and the guest-visible value. Then you don't have to separately keep both emm_time and c15_ccnt as state. > + } > + > return 0; > } > > @@ -584,6 +591,35 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri, > return 0; > } > > +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri, > + uint64_t *value) > +{ > + int64_t clock_value; > + > + /* This assumes that non-invasive debugging is not permitted */ > + if (env->cp15.c9_pmcr & PMCRDP || > + !(env->cp15.c9_pmcr & PMCRE)) { > + /* Counter is disabled, do not change value */ > + *value = env->cp15.c15_ccnt; > + return 0; > + } > + > + clock_value = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + > + if (env->cp15.c9_pmcr & PMCRDP) { > + /* Increment once every 64 processor clock cycles */ > + env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \ > + env->emm_time))/64; Why the line continuation character? > + } else { > + env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \ > + env->emm_time)); > + } > + > + env->emm_time = clock_value; > + *value = env->cp15.c15_ccnt; > + return 0; > +} > + > static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri, > uint64_t *value) > { > @@ -644,9 +680,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { > */ > { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5, > .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, > - /* Unimplemented, RAZ/WI. XXX PMUSERENR */ > { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0, > - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, > + .access = PL1_RW, .readfn = pmccntr_read, > + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt), > + .resetvalue = 0, .type = ARM_CP_IO }, > { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, > .access = PL0_RW, > .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper), > diff --git a/target-arm/helper.h b/target-arm/helper.h > index 70872df..5b5dc0a 100644 > --- a/target-arm/helper.h > +++ b/target-arm/helper.h > @@ -492,6 +492,15 @@ DEF_HELPER_3(neon_qzip32, void, env, i32, i32) > DEF_HELPER_4(crypto_aese, void, env, i32, i32, i32) > DEF_HELPER_4(crypto_aesmc, void, env, i32, i32, i32) > > +/* Definitions for the PMCCNTR and PMCR registers */ > +#define PMCRDP 0x20 > +#define PMCRD 0x8 > +#define PMCRC 0x4 > +#define PMCRE 0x1 > + > +#define CLOCKNSTOTICKS(input) ((((input * get_ticks_per_sec())/1000000000) >> 8) & \ > + 0xFFFFFFFF) > + This is absolutely the wrong header for this kind of thing. Also that macro is rather misleadingly named, looks pretty ugly and is not really necessary given it's basically used in just one place. thanks -- PMM
On Sun, Jan 19, 2014 at 8:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 16 January 2014 04:31, Alistair Francis <alistair.francis@xilinx.com> wrote: >> This patch implements the ARM PMCCNTR register including >> the disable and reset components of the PMCR register. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> This patch assumes that non-invasive debugging is not permitted >> when determining if the counter is disabled >> >> target-arm/cpu.c | 2 ++ >> target-arm/cpu.h | 3 +++ >> target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++++++--- >> target-arm/helper.h | 9 +++++++++ >> 4 files changed, 54 insertions(+), 3 deletions(-) >> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index 52efd5d..a185959 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -76,6 +76,8 @@ static void arm_cpu_reset(CPUState *s) >> >> acc->parent_reset(s); >> >> + env->emm_time = 0; >> + >> memset(env, 0, offsetof(CPUARMState, breakpoints)); >> g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu); >> env->vfp.xregs[ARM_VFP_FPSID] = cpu->reset_fpsid; >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 198b6b8..8c73a56 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -215,8 +215,11 @@ typedef struct CPUARMState { >> uint32_t c15_diagnostic; /* diagnostic register */ >> uint32_t c15_power_diagnostic; >> uint32_t c15_power_control; /* power control */ >> + uint32_t c15_ccnt; /* Performance Monitors Cycle Count */ >> } cp15; >> >> + int64_t emm_time; /* Used to hold the total emmulation time - in ns */ > > This looks bogus. You don't need to save the total emulation > time, because you can always get it by calling > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL). > (Also it won't get migrated, but the correct fix for that is to get > rid of it, see below.) > It means something slightly different doesnt it? It's the emulation time last time the ccnt was synced. Looking below its always updated syncronously with ccnt itself and never on its own. Perhaps a rename to make that clearer. >> + >> /* System registers (AArch64) */ >> struct { >> uint64_t tpidr_el0; >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index c708f15..72404cf 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -505,9 +505,16 @@ static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, >> if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) { >> return EXCP_UDEF; >> } >> - /* only the DP, X, D and E bits are writable */ >> + /* only the DP, X, D and E bits are writeable */ > > "writable" is a valid spelling and a quick grep shows it's > actually used much more in qemu than "writeable". > Yes out of scope change, please revert. >> env->cp15.c9_pmcr &= ~0x39; >> env->cp15.c9_pmcr |= (value & 0x39); >> + >> + if (value & PMCRC) { >> + /* Reset the counter */ >> + env->emm_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> + env->cp15.c15_ccnt = 0; > > This is the wrong way to do this kind of thing. Don't store > the actual value of a resettable count register; store the > difference between the CLOCK_VIRTUAL count and > the guest-visible value. Then you don't have to separately > keep both emm_time and c15_ccnt as state. > Is the state minimisation really worth that pain? If only reset was being implemented, it could be done as a simple case of snapping clock-virtual as "clock_when_ccnt_reset" then removing the CCNT state entirely but the problem is it does make the implementation of the disable feature somewhat difficult. If the timer is disabled this "diff" needs to start changing as emulation time progresses. If I disable, wait for 1s CLOCK_VIRTUAL time then renable, i need to recalulate this diff -= 1. The only way that calculation can be done is with a second variable, which snapshots CLOCK_VIRTUAL when disabled happened so this diff can be recalculated. Its also awkward as if you disable for long enough (or immediately on emulation start), "clock_when_ccnt_reset" can go negative. What's missing from this patch is there is no sense of syncing the two variables when you disable, re-enable or change the freqency scaler (which needs to be fixed). All of these features can cause the timer to change frequency while it is at a non-zero value, which means this diff variable would need continual warping every time one of these frequency affecting settings changed. Regards, Peter >> + } >> + >> return 0; >> } >> >> @@ -584,6 +591,35 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri, >> return 0; >> } >> >> +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri, >> + uint64_t *value) >> +{ >> + int64_t clock_value; >> + >> + /* This assumes that non-invasive debugging is not permitted */ >> + if (env->cp15.c9_pmcr & PMCRDP || >> + !(env->cp15.c9_pmcr & PMCRE)) { >> + /* Counter is disabled, do not change value */ >> + *value = env->cp15.c15_ccnt; >> + return 0; >> + } >> + >> + clock_value = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >> + >> + if (env->cp15.c9_pmcr & PMCRDP) { >> + /* Increment once every 64 processor clock cycles */ >> + env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \ >> + env->emm_time))/64; > > Why the line continuation character? > >> + } else { >> + env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \ >> + env->emm_time)); >> + } >> + >> + env->emm_time = clock_value; >> + *value = env->cp15.c15_ccnt; >> + return 0; >> +} >> + >> static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri, >> uint64_t *value) >> { >> @@ -644,9 +680,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { >> */ >> { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5, >> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, >> - /* Unimplemented, RAZ/WI. XXX PMUSERENR */ >> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0, >> - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, >> + .access = PL1_RW, .readfn = pmccntr_read, >> + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt), >> + .resetvalue = 0, .type = ARM_CP_IO }, >> { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, >> .access = PL0_RW, >> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper), >> diff --git a/target-arm/helper.h b/target-arm/helper.h >> index 70872df..5b5dc0a 100644 >> --- a/target-arm/helper.h >> +++ b/target-arm/helper.h >> @@ -492,6 +492,15 @@ DEF_HELPER_3(neon_qzip32, void, env, i32, i32) >> DEF_HELPER_4(crypto_aese, void, env, i32, i32, i32) >> DEF_HELPER_4(crypto_aesmc, void, env, i32, i32, i32) >> >> +/* Definitions for the PMCCNTR and PMCR registers */ >> +#define PMCRDP 0x20 >> +#define PMCRD 0x8 >> +#define PMCRC 0x4 >> +#define PMCRE 0x1 >> + >> +#define CLOCKNSTOTICKS(input) ((((input * get_ticks_per_sec())/1000000000) >> 8) & \ >> + 0xFFFFFFFF) >> + > > This is absolutely the wrong header for this kind of thing. Also that > macro is rather misleadingly named, looks pretty ugly and is not > really necessary given it's basically used in just one place. > > thanks > -- PMM >
On 19 January 2014 00:48, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sun, Jan 19, 2014 at 8:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 16 January 2014 04:31, Alistair Francis <alistair.francis@xilinx.com> wrote: >>> env->cp15.c9_pmcr &= ~0x39; >>> env->cp15.c9_pmcr |= (value & 0x39); >>> + >>> + if (value & PMCRC) { >>> + /* Reset the counter */ >>> + env->emm_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>> + env->cp15.c15_ccnt = 0; >> >> This is the wrong way to do this kind of thing. Don't store >> the actual value of a resettable count register; store the >> difference between the CLOCK_VIRTUAL count and >> the guest-visible value. Then you don't have to separately >> keep both emm_time and c15_ccnt as state. >> > > Is the state minimisation really worth that pain? You have to do calculation on read and write anyway, because obviously we don't keep the state field continuously updated to the value the guest needs to see. (Actually the lack of a writefn hook for this register looks like a bug to me.) So it doesn't IMHO make much difference in terms of code complexity, and it keeps the CPUState tidy (in particular, you don't end up with a random field which is "this pertains to a single cp15 register, and it holds state, but it's lurking randomly at the top level"). I think the migration is really the key issue though: if you design the state such that simply writing the incoming-migration value to the register via the raw-write hook (and reading it via raw-read on an outbound migration) is sufficient, then it all just works. If you have this extra bit of state lurking about then it is all going to need nasty special casing, and I went to quite a lot of effort to make sure that cp15 registers didn't need individual special casing. > If only reset was > being implemented, it could be done as a simple case of snapping > clock-virtual as "clock_when_ccnt_reset" then removing the CCNT state > entirely but the problem is it does make the implementation of the > disable feature somewhat difficult. If the timer is disabled this > "diff" needs to start changing as emulation time progresses. If I > disable, wait for 1s CLOCK_VIRTUAL time then renable, i need to > recalulate this diff -= 1. The only way that calculation can be done > is with a second variable, which snapshots CLOCK_VIRTUAL when disabled > happened so this diff can be recalculated. Its also awkward as if you > disable for long enough (or immediately on emulation start), > "clock_when_ccnt_reset" can go negative. Typically for a disablable counter, when it's disabled you keep "current disabled value" in the state field, and then when it's reenabled you switch back to "diff from clock". thanks -- PMM
On Sun, Jan 19, 2014 at 11:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 January 2014 00:48, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Sun, Jan 19, 2014 at 8:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 16 January 2014 04:31, Alistair Francis <alistair.francis@xilinx.com> wrote: >>>> env->cp15.c9_pmcr &= ~0x39; >>>> env->cp15.c9_pmcr |= (value & 0x39); >>>> + >>>> + if (value & PMCRC) { >>>> + /* Reset the counter */ >>>> + env->emm_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); >>>> + env->cp15.c15_ccnt = 0; >>> >>> This is the wrong way to do this kind of thing. Don't store >>> the actual value of a resettable count register; store the >>> difference between the CLOCK_VIRTUAL count and >>> the guest-visible value. Then you don't have to separately >>> keep both emm_time and c15_ccnt as state. >>> >> >> Is the state minimisation really worth that pain? > > You have to do calculation on read and write anyway, because > obviously we don't keep the state field continuously updated > to the value the guest needs to see. (Actually the lack of > a writefn hook for this register looks like a bug to me.) Yes it should be easy enough. So it doesn't > IMHO make much difference in terms of code complexity, and it > keeps the CPUState tidy (in particular, you don't end up with a > random field which is "this pertains to a single cp15 register, and > it holds state, but it's lurking randomly at the top level"). I think > the migration is really the key issue though: if you design the > state such that simply writing the incoming-migration value > to the register via the raw-write hook (and reading it via > raw-read on an outbound migration) is sufficient, then it all > just works. If you have this extra bit of state lurking about then > it is all going to need nasty special casing, and I went to quite > a lot of effort to make sure that cp15 registers didn't need > individual special casing. > FWIW, is there anything wrong with just migrating the second variable on the side? Is there no code path for migration of random ARM state thats not a CP? >> If only reset was >> being implemented, it could be done as a simple case of snapping >> clock-virtual as "clock_when_ccnt_reset" then removing the CCNT state >> entirely but the problem is it does make the implementation of the >> disable feature somewhat difficult. If the timer is disabled this >> "diff" needs to start changing as emulation time progresses. If I >> disable, wait for 1s CLOCK_VIRTUAL time then renable, i need to >> recalulate this diff -= 1. The only way that calculation can be done >> is with a second variable, which snapshots CLOCK_VIRTUAL when disabled >> happened so this diff can be recalculated. Its also awkward as if you >> disable for long enough (or immediately on emulation start), >> "clock_when_ccnt_reset" can go negative. > > Typically for a disablable counter, when it's disabled you keep > "current disabled value" in the state field, and then when it's > reenabled you switch back to "diff from clock". > Ouch. Thats a variable with mixed meaning. You still have the amkwardness however of this diff needing warping every time you change the frequency. And it can go negative when you change from high frequency to low frequency w/o a reset. Although none of this is a functional showstopper I guess. So to run with your idea, the register can be defined as a dual meaning uint64_t to simplify migration but a comment should be added to the effect that this value does represent the actual register state: /* * NOT always the value of the counter. If the counter * is enabled, it is the virtual clock time corresponding * to when the counter was reset. If the counter is disabled, * it is value of the counter. */ uint64_t c15_ccnt; Regards, Peter > thanks > -- PMM >
On 19 January 2014 01:39, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sun, Jan 19, 2014 at 11:06 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: > So it doesn't >> IMHO make much difference in terms of code complexity, and it >> keeps the CPUState tidy (in particular, you don't end up with a >> random field which is "this pertains to a single cp15 register, and >> it holds state, but it's lurking randomly at the top level"). I think >> the migration is really the key issue though: if you design the >> state such that simply writing the incoming-migration value >> to the register via the raw-write hook (and reading it via >> raw-read on an outbound migration) is sufficient, then it all >> just works. If you have this extra bit of state lurking about then >> it is all going to need nasty special casing, and I went to quite >> a lot of effort to make sure that cp15 registers didn't need >> individual special casing. >> > > FWIW, is there anything wrong with just migrating the second variable > on the side? Is there no code path for migration of random ARM state > thats not a CP? It would break migration between TCG and KVM -- the assumption is that cp registers only need to be read and written via the raw_read/write interface. There is obviously a mechanism for migrating ARM state that isn't a coprocessor (you put an entry for it in the vmstate structs in machine.c), but you need to figure out what that state looks like for KVM as well and special case it in the KVM state syncing functions. >>> If only reset was >>> being implemented, it could be done as a simple case of snapping >>> clock-virtual as "clock_when_ccnt_reset" then removing the CCNT state >>> entirely but the problem is it does make the implementation of the >>> disable feature somewhat difficult. If the timer is disabled this >>> "diff" needs to start changing as emulation time progresses. If I >>> disable, wait for 1s CLOCK_VIRTUAL time then renable, i need to >>> recalulate this diff -= 1. The only way that calculation can be done >>> is with a second variable, which snapshots CLOCK_VIRTUAL when disabled >>> happened so this diff can be recalculated. Its also awkward as if you >>> disable for long enough (or immediately on emulation start), >>> "clock_when_ccnt_reset" can go negative. >> >> Typically for a disablable counter, when it's disabled you keep >> "current disabled value" in the state field, and then when it's >> reenabled you switch back to "diff from clock". >> > > Ouch. Thats a variable with mixed meaning. You still have the > amkwardness however of this diff needing warping every time you change > the frequency. And it can go negative when you change from high > frequency to low frequency w/o a reset. Although none of this is a > functional showstopper I guess. I would assume that frequency doesn't change much, so to the extent we care about efficiency we should make the read path simple and do recalculation on frequency changes. > So to run with your idea, the register can be defined as a dual > meaning uint64_t to simplify migration but a comment should be added > to the effect that this value does represent the actual register > state: Yeah, if you like. (Actually I think it's ok to have more state in the CPU struct, as long as it's always valid to save and restore just by saving and restoring the value through the raw accessors. If you really want to have two fields then you should put them both in the cp15 bit of the struct though; compare the handling of the generic timer cp14_timer fields.) Your comment seems a bit longwinded to me, though, since from my POV this is just the obvious way to implement a counter. thanks -- PMM
I have made those changes you both mentioned above and submitted v2 of my series. There is now only one extra variable in the CPUARMState struct. On Sun, Jan 19, 2014 at 9:20 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 January 2014 01:39, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Sun, Jan 19, 2014 at 11:06 AM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >> So it doesn't >>> IMHO make much difference in terms of code complexity, and it >>> keeps the CPUState tidy (in particular, you don't end up with a >>> random field which is "this pertains to a single cp15 register, and >>> it holds state, but it's lurking randomly at the top level"). I think >>> the migration is really the key issue though: if you design the >>> state such that simply writing the incoming-migration value >>> to the register via the raw-write hook (and reading it via >>> raw-read on an outbound migration) is sufficient, then it all >>> just works. If you have this extra bit of state lurking about then >>> it is all going to need nasty special casing, and I went to quite >>> a lot of effort to make sure that cp15 registers didn't need >>> individual special casing. >>> >> >> FWIW, is there anything wrong with just migrating the second variable >> on the side? Is there no code path for migration of random ARM state >> thats not a CP? > > It would break migration between TCG and KVM -- the assumption > is that cp registers only need to be read and written via the > raw_read/write interface. There is obviously a mechanism for > migrating ARM state that isn't a coprocessor (you put an entry > for it in the vmstate structs in machine.c), but you need to > figure out what that state looks like for KVM as well and special > case it in the KVM state syncing functions. > >>>> If only reset was >>>> being implemented, it could be done as a simple case of snapping >>>> clock-virtual as "clock_when_ccnt_reset" then removing the CCNT state >>>> entirely but the problem is it does make the implementation of the >>>> disable feature somewhat difficult. If the timer is disabled this >>>> "diff" needs to start changing as emulation time progresses. If I >>>> disable, wait for 1s CLOCK_VIRTUAL time then renable, i need to >>>> recalulate this diff -= 1. The only way that calculation can be done >>>> is with a second variable, which snapshots CLOCK_VIRTUAL when disabled >>>> happened so this diff can be recalculated. Its also awkward as if you >>>> disable for long enough (or immediately on emulation start), >>>> "clock_when_ccnt_reset" can go negative. >>> >>> Typically for a disablable counter, when it's disabled you keep >>> "current disabled value" in the state field, and then when it's >>> reenabled you switch back to "diff from clock". >>> >> >> Ouch. Thats a variable with mixed meaning. You still have the >> amkwardness however of this diff needing warping every time you change >> the frequency. And it can go negative when you change from high >> frequency to low frequency w/o a reset. Although none of this is a >> functional showstopper I guess. > > I would assume that frequency doesn't change much, so to the > extent we care about efficiency we should make the read > path simple and do recalculation on frequency changes. > >> So to run with your idea, the register can be defined as a dual >> meaning uint64_t to simplify migration but a comment should be added >> to the effect that this value does represent the actual register >> state: > > Yeah, if you like. (Actually I think it's ok to have more state in the > CPU struct, as long as it's always valid to save and restore just > by saving and restoring the value through the raw accessors. > If you really want to have two fields then you should put them both > in the cp15 bit of the struct though; compare the handling of the > generic timer cp14_timer fields.) Your comment seems a bit > longwinded to me, though, since from my POV this is just the > obvious way to implement a counter. > > thanks > -- PMM >
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 52efd5d..a185959 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -76,6 +76,8 @@ static void arm_cpu_reset(CPUState *s) acc->parent_reset(s); + env->emm_time = 0; + memset(env, 0, offsetof(CPUARMState, breakpoints)); g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu); env->vfp.xregs[ARM_VFP_FPSID] = cpu->reset_fpsid; diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 198b6b8..8c73a56 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -215,8 +215,11 @@ typedef struct CPUARMState { uint32_t c15_diagnostic; /* diagnostic register */ uint32_t c15_power_diagnostic; uint32_t c15_power_control; /* power control */ + uint32_t c15_ccnt; /* Performance Monitors Cycle Count */ } cp15; + int64_t emm_time; /* Used to hold the total emmulation time - in ns */ + /* System registers (AArch64) */ struct { uint64_t tpidr_el0; diff --git a/target-arm/helper.c b/target-arm/helper.c index c708f15..72404cf 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -505,9 +505,16 @@ static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) { return EXCP_UDEF; } - /* only the DP, X, D and E bits are writable */ + /* only the DP, X, D and E bits are writeable */ env->cp15.c9_pmcr &= ~0x39; env->cp15.c9_pmcr |= (value & 0x39); + + if (value & PMCRC) { + /* Reset the counter */ + env->emm_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); + env->cp15.c15_ccnt = 0; + } + return 0; } @@ -584,6 +591,35 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri, return 0; } +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri, + uint64_t *value) +{ + int64_t clock_value; + + /* This assumes that non-invasive debugging is not permitted */ + if (env->cp15.c9_pmcr & PMCRDP || + !(env->cp15.c9_pmcr & PMCRE)) { + /* Counter is disabled, do not change value */ + *value = env->cp15.c15_ccnt; + return 0; + } + + clock_value = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); + + if (env->cp15.c9_pmcr & PMCRDP) { + /* Increment once every 64 processor clock cycles */ + env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \ + env->emm_time))/64; + } else { + env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \ + env->emm_time)); + } + + env->emm_time = clock_value; + *value = env->cp15.c15_ccnt; + return 0; +} + static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) { @@ -644,9 +680,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { */ { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5, .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, - /* Unimplemented, RAZ/WI. XXX PMUSERENR */ { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0, - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 }, + .access = PL1_RW, .readfn = pmccntr_read, + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt), + .resetvalue = 0, .type = ARM_CP_IO }, { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper), diff --git a/target-arm/helper.h b/target-arm/helper.h index 70872df..5b5dc0a 100644 --- a/target-arm/helper.h +++ b/target-arm/helper.h @@ -492,6 +492,15 @@ DEF_HELPER_3(neon_qzip32, void, env, i32, i32) DEF_HELPER_4(crypto_aese, void, env, i32, i32, i32) DEF_HELPER_4(crypto_aesmc, void, env, i32, i32, i32) +/* Definitions for the PMCCNTR and PMCR registers */ +#define PMCRDP 0x20 +#define PMCRD 0x8 +#define PMCRC 0x4 +#define PMCRE 0x1 + +#define CLOCKNSTOTICKS(input) ((((input * get_ticks_per_sec())/1000000000) >> 8) & \ + 0xFFFFFFFF) + #ifdef TARGET_AARCH64 #include "helper-a64.h" #endif
This patch implements the ARM PMCCNTR register including the disable and reset components of the PMCR register. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- This patch assumes that non-invasive debugging is not permitted when determining if the counter is disabled target-arm/cpu.c | 2 ++ target-arm/cpu.h | 3 +++ target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++++++--- target-arm/helper.h | 9 +++++++++ 4 files changed, 54 insertions(+), 3 deletions(-)