Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Alistair Francis]

On Thu, Jun 26, 2014 at 10:37 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> 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?
>

So I sent a second version, still calling the function twice as in this version.
Still more then happy to discuss changes to stop calling the function
twice if it is an issue for anyone. Although I think it works well

>>
>> 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.
>>>>
>>>
>>

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Peter Crosthwaite]

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
>>
>>

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Peter Crosthwaite]

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
>
>

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Alistair Francis]

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.
>>>
>>
>

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Peter Crosthwaite]

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.
>>
>

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Alistair Francis]

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.
>

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Christopher Covington]

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

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

[Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Alistair Francis]

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);
 }
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