| Message ID | 3217ac3f31ad96dd4811309d9a68e5af0886e628.1403572003.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 06/23/2014 09:12 PM, Alistair Francis wrote: > Call the new pmccntr_sync() function when there is a possibility > of swapping ELs (I.E. when there is an exception) > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > > target-arm/helper-a64.c | 5 +++++ > target-arm/helper.c | 7 +++++++ > target-arm/op_helper.c | 6 ++++++ > 3 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c > index 2b4ce6a..b61174f 100644 > --- a/target-arm/helper-a64.c > +++ b/target-arm/helper-a64.c > @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > target_ulong addr = env->cp15.vbar_el[1]; > int i; > > + pmccntr_sync(env); > + > if (arm_current_pl(env) == 0) { > if (env->aarch64) { > addr += 0x400; > @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > addr += 0x100; > break; > default: > + pmccntr_sync(env); > cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > } > > @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > > env->pc = addr; > cs->interrupt_request |= CPU_INTERRUPT_EXITTB; > + > + pmccntr_sync(env); > } The double calls seem unwieldy. I think it could be made into a single function call if there was access, perhaps as a second parameter or maybe as a static variable, to both the previous and current state so the function could tell whether there is no transition, enable->disable, or disable->enable. Christopher
On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington <cov@codeaurora.org> wrote: > On 06/23/2014 09:12 PM, Alistair Francis wrote: >> Call the new pmccntr_sync() function when there is a possibility >> of swapping ELs (I.E. when there is an exception) >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> >> target-arm/helper-a64.c | 5 +++++ >> target-arm/helper.c | 7 +++++++ >> target-arm/op_helper.c | 6 ++++++ >> 3 files changed, 18 insertions(+), 0 deletions(-) >> >> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c >> index 2b4ce6a..b61174f 100644 >> --- a/target-arm/helper-a64.c >> +++ b/target-arm/helper-a64.c >> @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >> target_ulong addr = env->cp15.vbar_el[1]; >> int i; >> >> + pmccntr_sync(env); >> + >> if (arm_current_pl(env) == 0) { >> if (env->aarch64) { >> addr += 0x400; >> @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >> addr += 0x100; >> break; >> default: >> + pmccntr_sync(env); >> cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); >> } >> >> @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >> >> env->pc = addr; >> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >> + >> + pmccntr_sync(env); >> } > > The double calls seem unwieldy. I think it could be made into a single > function call if there was access, perhaps as a second parameter or maybe as a > static variable, to both the previous and current state so the function could > tell whether there is no transition, enable->disable, or disable->enable. > The problem with a parameter is that the state of the enabled register needs to be saved at the start of any code that will enable/disable the register. So it ends up being just as messy. Static variables won't work if there are multiple CPUs. I guess an array of statics could work, but I don't like that method I feel that just calling the function twice ends up being neat and works pretty well > Christopher > > -- > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by the Linux Foundation. >
On Wed, Jun 25, 2014 at 8:39 AM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington > <cov@codeaurora.org> wrote: >> On 06/23/2014 09:12 PM, Alistair Francis wrote: >>> Call the new pmccntr_sync() function when there is a possibility >>> of swapping ELs (I.E. when there is an exception) >>> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> --- >>> >>> target-arm/helper-a64.c | 5 +++++ >>> target-arm/helper.c | 7 +++++++ >>> target-arm/op_helper.c | 6 ++++++ >>> 3 files changed, 18 insertions(+), 0 deletions(-) >>> >>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c >>> index 2b4ce6a..b61174f 100644 >>> --- a/target-arm/helper-a64.c >>> +++ b/target-arm/helper-a64.c >>> @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >>> target_ulong addr = env->cp15.vbar_el[1]; >>> int i; >>> >>> + pmccntr_sync(env); >>> + >>> if (arm_current_pl(env) == 0) { >>> if (env->aarch64) { >>> addr += 0x400; >>> @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >>> addr += 0x100; >>> break; >>> default: >>> + pmccntr_sync(env); >>> cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); Not sure you need this, there is not much point in causing your side effects right before an assertion (via cpu_abort). >>> } >>> >>> @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >>> >>> env->pc = addr; >>> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >>> + >>> + pmccntr_sync(env); >>> } >> >> The double calls seem unwieldy. I think it could be made into a single >> function call if there was access, perhaps as a second parameter or maybe as a >> static variable, to both the previous and current state so the function could >> tell whether there is no transition, enable->disable, or disable->enable. >> > > The problem with a parameter is that the state of the enabled register needs > to be saved at the start of any code that will enable/disable the register. So > it ends up being just as messy. > > Static variables won't work if there are multiple CPUs. I guess an > array of statics > could work, but I don't like that method > > I feel that just calling the function twice ends up being neat and > works pretty well > Theres a third option. Create a new function that explicitly changes EL: arm_change_el(int new) { sync(); env->el = new; sync(); } And update the interrupt path functions to use it instead of direct env manipulation. The advantage of this, is others can also add el switching side effects in one place. I doubt this is the last time we will want to trap EL changes for system level side effects. Regards, Peter >> Christopher >> >> -- >> Employee of Qualcomm Innovation Center, Inc. >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> hosted by the Linux Foundation. >> >
On Wed, Jun 25, 2014 at 9:07 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Wed, Jun 25, 2014 at 8:39 AM, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington >> <cov@codeaurora.org> wrote: >>> On 06/23/2014 09:12 PM, Alistair Francis wrote: >>>> Call the new pmccntr_sync() function when there is a possibility >>>> of swapping ELs (I.E. when there is an exception) >>>> >>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>>> --- >>>> >>>> target-arm/helper-a64.c | 5 +++++ >>>> target-arm/helper.c | 7 +++++++ >>>> target-arm/op_helper.c | 6 ++++++ >>>> 3 files changed, 18 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c >>>> index 2b4ce6a..b61174f 100644 >>>> --- a/target-arm/helper-a64.c >>>> +++ b/target-arm/helper-a64.c >>>> @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >>>> target_ulong addr = env->cp15.vbar_el[1]; >>>> int i; >>>> >>>> + pmccntr_sync(env); >>>> + >>>> if (arm_current_pl(env) == 0) { >>>> if (env->aarch64) { >>>> addr += 0x400; >>>> @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >>>> addr += 0x100; >>>> break; >>>> default: >>>> + pmccntr_sync(env); >>>> cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > > Not sure you need this, there is not much point in causing your side > effects right before an assertion (via cpu_abort). I figured it doesn't really matter. Plus it could make debugging easier if someone was monitoring the registers? > >>>> } >>>> >>>> @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >>>> >>>> env->pc = addr; >>>> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >>>> + >>>> + pmccntr_sync(env); >>>> } >>> >>> The double calls seem unwieldy. I think it could be made into a single >>> function call if there was access, perhaps as a second parameter or maybe as a >>> static variable, to both the previous and current state so the function could >>> tell whether there is no transition, enable->disable, or disable->enable. >>> >> >> The problem with a parameter is that the state of the enabled register needs >> to be saved at the start of any code that will enable/disable the register. So >> it ends up being just as messy. >> >> Static variables won't work if there are multiple CPUs. I guess an >> array of statics >> could work, but I don't like that method >> >> I feel that just calling the function twice ends up being neat and >> works pretty well >> > > Theres a third option. Create a new function that explicitly changes EL: > > arm_change_el(int new) { > sync(); > env->el = new; > sync(); > } > > And update the interrupt path functions to use it instead of direct > env manipulation. > > The advantage of this, is others can also add el switching side > effects in one place. I doubt this is the last time we will want to > trap EL changes for system level side effects. That doesn't really fix the problem, because the sync function will still need to exist as there are other places where the counter can be enabled/disabled. So it just moves it to a different place. I also feel that that's pretty much what the *cpu_do_interrupt() functions already do Could that also interfere with the work that is being done to support EL2 and 3? > > Regards, > Peter > >>> Christopher >>> >>> -- >>> Employee of Qualcomm Innovation Center, Inc. >>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >>> hosted by the Linux Foundation. >>> >> >
On Tue, Jun 24, 2014 at 11:12 AM, Alistair Francis <alistair.francis@xilinx.com> wrote: > Call the new pmccntr_sync() function when there is a possibility > of swapping ELs (I.E. when there is an exception) > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > > target-arm/helper-a64.c | 5 +++++ > target-arm/helper.c | 7 +++++++ > target-arm/op_helper.c | 6 ++++++ > 3 files changed, 18 insertions(+), 0 deletions(-) > > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c > index 2b4ce6a..b61174f 100644 > --- a/target-arm/helper-a64.c > +++ b/target-arm/helper-a64.c > @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > target_ulong addr = env->cp15.vbar_el[1]; > int i; > > + pmccntr_sync(env); > + > if (arm_current_pl(env) == 0) { > if (env->aarch64) { > addr += 0x400; > @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > addr += 0x100; > break; > default: > + pmccntr_sync(env); Can you localise just the one sync->sync pair around the el change logic itself to avoid having to catch all the return paths? Regards, Peter > cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > } > > @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > > env->pc = addr; > cs->interrupt_request |= CPU_INTERRUPT_EXITTB; > + > + pmccntr_sync(env); > } > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 3e0a9a1..7c0a57f 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -3417,6 +3417,8 @@ void arm_cpu_do_interrupt(CPUState *cs) > > assert(!IS_M(env)); > > + pmccntr_sync(env); > + > arm_log_exception(cs->exception_index); > > /* TODO: Vectored interrupt controller. */ > @@ -3447,6 +3449,7 @@ void arm_cpu_do_interrupt(CPUState *cs) > && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) { > env->regs[0] = do_arm_semihosting(env); > qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n"); > + pmccntr_sync(env); > return; > } > } > @@ -3465,6 +3468,7 @@ void arm_cpu_do_interrupt(CPUState *cs) > env->regs[15] += 2; > env->regs[0] = do_arm_semihosting(env); > qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n"); > + pmccntr_sync(env); > return; > } > } > @@ -3508,6 +3512,7 @@ void arm_cpu_do_interrupt(CPUState *cs) > offset = 4; > break; > default: > + pmccntr_sync(env); > cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > return; /* Never happens. Keep compiler happy. */ > } > @@ -3540,6 +3545,8 @@ void arm_cpu_do_interrupt(CPUState *cs) > env->regs[14] = env->regs[15] + offset; > env->regs[15] = addr; > cs->interrupt_request |= CPU_INTERRUPT_EXITTB; > + > + pmccntr_sync(env); > } > > /* Check section/page access permissions. > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index 9c1ef52..07ab30b 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env) > uint32_t spsr = env->banked_spsr[spsr_idx]; > int new_el, i; > > + pmccntr_sync(env); > + > if (env->pstate & PSTATE_SP) { > env->sp_el[cur_el] = env->xregs[31]; > } else { > @@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env) > env->pc = env->elr_el[cur_el]; > } > > + pmccntr_sync(env); > + > return; > > illegal_return: > @@ -433,6 +437,8 @@ illegal_return: > spsr &= PSTATE_NZCV | PSTATE_DAIF; > spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF); > pstate_write(env, spsr); > + > + pmccntr_sync(env); > } > > /* ??? Flag setting arithmetic is awkward because we need to do comparisons. > -- > 1.7.1 > >
On Thu, Jun 26, 2014 at 9:40 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jun 24, 2014 at 11:12 AM, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> Call the new pmccntr_sync() function when there is a possibility >> of swapping ELs (I.E. when there is an exception) >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> >> target-arm/helper-a64.c | 5 +++++ >> target-arm/helper.c | 7 +++++++ >> target-arm/op_helper.c | 6 ++++++ >> 3 files changed, 18 insertions(+), 0 deletions(-) >> >> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c >> index 2b4ce6a..b61174f 100644 >> --- a/target-arm/helper-a64.c >> +++ b/target-arm/helper-a64.c >> @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >> target_ulong addr = env->cp15.vbar_el[1]; >> int i; >> >> + pmccntr_sync(env); >> + >> if (arm_current_pl(env) == 0) { >> if (env->aarch64) { >> addr += 0x400; >> @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >> addr += 0x100; >> break; >> default: >> + pmccntr_sync(env); > > Can you localise just the one sync->sync pair around the el change > logic itself to avoid having to catch all the return paths? > Doh, reviewed the wrong version. Sry for the noise. Regards, Peter > Regards, > Peter > >> cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); >> } >> >> @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) >> >> env->pc = addr; >> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >> + >> + pmccntr_sync(env); >> } >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 3e0a9a1..7c0a57f 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -3417,6 +3417,8 @@ void arm_cpu_do_interrupt(CPUState *cs) >> >> assert(!IS_M(env)); >> >> + pmccntr_sync(env); >> + >> arm_log_exception(cs->exception_index); >> >> /* TODO: Vectored interrupt controller. */ >> @@ -3447,6 +3449,7 @@ void arm_cpu_do_interrupt(CPUState *cs) >> && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) { >> env->regs[0] = do_arm_semihosting(env); >> qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n"); >> + pmccntr_sync(env); >> return; >> } >> } >> @@ -3465,6 +3468,7 @@ void arm_cpu_do_interrupt(CPUState *cs) >> env->regs[15] += 2; >> env->regs[0] = do_arm_semihosting(env); >> qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n"); >> + pmccntr_sync(env); >> return; >> } >> } >> @@ -3508,6 +3512,7 @@ void arm_cpu_do_interrupt(CPUState *cs) >> offset = 4; >> break; >> default: >> + pmccntr_sync(env); >> cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); >> return; /* Never happens. Keep compiler happy. */ >> } >> @@ -3540,6 +3545,8 @@ void arm_cpu_do_interrupt(CPUState *cs) >> env->regs[14] = env->regs[15] + offset; >> env->regs[15] = addr; >> cs->interrupt_request |= CPU_INTERRUPT_EXITTB; >> + >> + pmccntr_sync(env); >> } >> >> /* Check section/page access permissions. >> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c >> index 9c1ef52..07ab30b 100644 >> --- a/target-arm/op_helper.c >> +++ b/target-arm/op_helper.c >> @@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env) >> uint32_t spsr = env->banked_spsr[spsr_idx]; >> int new_el, i; >> >> + pmccntr_sync(env); >> + >> if (env->pstate & PSTATE_SP) { >> env->sp_el[cur_el] = env->xregs[31]; >> } else { >> @@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env) >> env->pc = env->elr_el[cur_el]; >> } >> >> + pmccntr_sync(env); >> + >> return; >> >> illegal_return: >> @@ -433,6 +437,8 @@ illegal_return: >> spsr &= PSTATE_NZCV | PSTATE_DAIF; >> spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF); >> pstate_write(env, spsr); >> + >> + pmccntr_sync(env); >> } >> >> /* ??? Flag setting arithmetic is awkward because we need to do comparisons. >> -- >> 1.7.1 >> >>
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c index 2b4ce6a..b61174f 100644 --- a/target-arm/helper-a64.c +++ b/target-arm/helper-a64.c @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs) target_ulong addr = env->cp15.vbar_el[1]; int i; + pmccntr_sync(env); + if (arm_current_pl(env) == 0) { if (env->aarch64) { addr += 0x400; @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) addr += 0x100; break; default: + pmccntr_sync(env); cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); } @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) env->pc = addr; cs->interrupt_request |= CPU_INTERRUPT_EXITTB; + + pmccntr_sync(env); } diff --git a/target-arm/helper.c b/target-arm/helper.c index 3e0a9a1..7c0a57f 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -3417,6 +3417,8 @@ void arm_cpu_do_interrupt(CPUState *cs) assert(!IS_M(env)); + pmccntr_sync(env); + arm_log_exception(cs->exception_index); /* TODO: Vectored interrupt controller. */ @@ -3447,6 +3449,7 @@ void arm_cpu_do_interrupt(CPUState *cs) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) { env->regs[0] = do_arm_semihosting(env); qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n"); + pmccntr_sync(env); return; } } @@ -3465,6 +3468,7 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[15] += 2; env->regs[0] = do_arm_semihosting(env); qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n"); + pmccntr_sync(env); return; } } @@ -3508,6 +3512,7 @@ void arm_cpu_do_interrupt(CPUState *cs) offset = 4; break; default: + pmccntr_sync(env); cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); return; /* Never happens. Keep compiler happy. */ } @@ -3540,6 +3545,8 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[14] = env->regs[15] + offset; env->regs[15] = addr; cs->interrupt_request |= CPU_INTERRUPT_EXITTB; + + pmccntr_sync(env); } /* Check section/page access permissions. diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index 9c1ef52..07ab30b 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env) uint32_t spsr = env->banked_spsr[spsr_idx]; int new_el, i; + pmccntr_sync(env); + if (env->pstate & PSTATE_SP) { env->sp_el[cur_el] = env->xregs[31]; } else { @@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env) env->pc = env->elr_el[cur_el]; } + pmccntr_sync(env); + return; illegal_return: @@ -433,6 +437,8 @@ illegal_return: spsr &= PSTATE_NZCV | PSTATE_DAIF; spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF); pstate_write(env, spsr); + + pmccntr_sync(env); } /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception) Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- target-arm/helper-a64.c | 5 +++++ target-arm/helper.c | 7 +++++++ target-arm/op_helper.c | 6 ++++++ 3 files changed, 18 insertions(+), 0 deletions(-)