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
<address@hidden> wrote:
> On Wed, Jun 25, 2014 at 8:39 AM, Alistair Francis
> <address@hidden> wrote:
>> On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington
>> <address@hidden> 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 <address@hidden>
>>>> ---
>>>>
>>>>  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.
>>>
>>
>