[Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Aaron Lindsay]

The ARM PMU implementation currently contains a basic cycle counter, but it is
often useful to gather counts of other events and filter them based on
execution mode. These patches flesh out the implementations of various PMU
registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
represent arbitrary counter types, implement mode filtering, and add
instruction, cycle, and software increment events.

I aim to eventually add raising interrupts on counter overflow, but that is not
covered by this patchset. I think I have a reasonable grasp of the mechanics of
*how* to raise them, but am curious if anyone has thoughts on how to determine
*when* to raise them - we don't want to call into PMU code every time an
instruction is executed to check if any instruction counters have overflowed,
etc. The main candidate I've seen for doing this so far would be to set up a
QEMUTimer, but I haven't fully explored it. Does that seem plausible? Any
other/better ideas?


Changes from v2:

* Many minor fixups (splitting patches, style/comment fixes, etc.)
* Save off current cycle count during operations which may change whether a
  counter is enabled, ensuring time isn't lost (update to patch 5)
* Added the ability to have more than one el_change_hook, added hooks before EL
  changes (patches 7-9)
* Added proper handling of can_do_io during code-gen before and after the
  el_change_hooks (patch 8)
* Split off PMOVSSET register definitions so they are only enabled for v7ve+
  (added patch 15, update to 16)

Thanks for any feedback,
Aaron
 

Aaron Lindsay (22):
  target/arm: A53: Initialize PMCEID[01]
  target/arm: A15 PMCEID0 initialization style nit
  target/arm: Check PMCNTEN for whether PMCCNTR is enabled
  target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
  target/arm: Reorganize PMCCNTR read, write, sync
  target/arm: Mask PMU register writes based on PMCR_EL0.N
  target/arm: Fetch GICv3 state directly from CPUARMState
  target/arm: Support multiple EL change hooks
  target/arm: Add pre-EL change hooks
  target/arm: Allow EL change hooks to do IO
  target/arm: Fix bitmask for PMCCFILTR writes
  target/arm: Filter cycle counter based on PMCCFILTR_EL0
  target/arm: Allow AArch32 access for PMCCFILTR
  target/arm: Make PMOVSCLR 64 bits wide
  target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
  target/arm: Implement PMOVSSET
  target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
  target/arm: Add array for supported PMU events, generate PMCEID[01]
  target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
  target/arm: PMU: Add instruction and cycle events
  target/arm: PMU: Set PMCR.N to 4
  target/arm: Implement PMSWINC

 hw/intc/arm_gicv3_cpuif.c  |  10 +-
 target/arm/cpu.c           |  40 ++-
 target/arm/cpu.h           | 102 +++++---
 target/arm/cpu64.c         |   2 +
 target/arm/helper.c        | 616 ++++++++++++++++++++++++++++++++++++++-------
 target/arm/internals.h     |  14 +-
 target/arm/op_helper.c     |   8 +
 target/arm/translate-a64.c |   2 +
 target/arm/translate.c     |   4 +
 9 files changed, 651 insertions(+), 147 deletions(-)

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Aaron Lindsay]

A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
pmceid[01] are already being initialized to zero for both A15 and A57.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 991d764..8c4db31 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000000;
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
     cpu->dbgdidr = 0x3516d000;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Philippe Mathieu-Daudé]

Hi Aaron,

On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
> A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
> pmceid[01] are already being initialized to zero for both A15 and A57.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu64.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 991d764..8c4db31 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
>      cpu->id_isar5 = 0x00011121;
>      cpu->id_aa64pfr0 = 0x00002222;
>      cpu->id_aa64dfr0 = 0x10305106;
> +    cpu->pmceid0 = 0x00000000;
> +    cpu->pmceid1 = 0x00000000;
>      cpu->id_aa64isar0 = 0x00011120;
>      cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
>      cpu->dbgdidr = 0x3516d000;
> 

Maybe we can move this at a single place in arm_cpu_post_init():

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
        cpu->pmceid0 = 0x00000000;
        cpu->pmceid1 = 0x00000000;
    }

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Philippe Mathieu-Daudé]

On 03/18/2018 11:35 PM, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
>> A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
>> pmceid[01] are already being initialized to zero for both A15 and A57.
>>
>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>> ---
>>  target/arm/cpu64.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 991d764..8c4db31 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
>>      cpu->id_isar5 = 0x00011121;
>>      cpu->id_aa64pfr0 = 0x00002222;
>>      cpu->id_aa64dfr0 = 0x10305106;
>> +    cpu->pmceid0 = 0x00000000;
>> +    cpu->pmceid1 = 0x00000000;
>>      cpu->id_aa64isar0 = 0x00011120;
>>      cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
>>      cpu->dbgdidr = 0x3516d000;
>>
> 
> Maybe we can move this at a single place in arm_cpu_post_init():

Err, arm_cpu_reset() :)

> 
>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>         cpu->pmceid0 = 0x00000000;
>         cpu->pmceid1 = 0x00000000;
>     }
> 
> Regards,
> 
> Phil.
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Aaron Lindsay]

On Mar 18 23:35, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
> > A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
> > pmceid[01] are already being initialized to zero for both A15 and A57.
> > 
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu64.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 991d764..8c4db31 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
> >      cpu->id_isar5 = 0x00011121;
> >      cpu->id_aa64pfr0 = 0x00002222;
> >      cpu->id_aa64dfr0 = 0x10305106;
> > +    cpu->pmceid0 = 0x00000000;
> > +    cpu->pmceid1 = 0x00000000;
> >      cpu->id_aa64isar0 = 0x00011120;
> >      cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
> >      cpu->dbgdidr = 0x3516d000;
> > 
> 
> Maybe we can move this at a single place in arm_cpu_post_init():
> 
>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>         cpu->pmceid0 = 0x00000000;
>         cpu->pmceid1 = 0x00000000;
>     }

I like consolidating the initialization - though I think it can go in
arm_cpu_realizefn() with the preexisting PMU-related id_aa64dfr0
initialization since it is constant once you've chosen a type of
processor. One of the other patches in this set actually already adds
some PMCEID initialization there based on PMCR.N.

-Aaron

> 
> Regards,
> 
> Phil.

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Philippe Mathieu-Daudé]

On 03/19/2018 09:35 PM, Aaron Lindsay wrote:
> On Mar 18 23:35, Philippe Mathieu-Daudé wrote:
>> Hi Aaron,
>>
>> On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
>>> A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
>>> pmceid[01] are already being initialized to zero for both A15 and A57.
>>>
>>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>>> ---
>>>  target/arm/cpu64.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>>> index 991d764..8c4db31 100644
>>> --- a/target/arm/cpu64.c
>>> +++ b/target/arm/cpu64.c
>>> @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
>>>      cpu->id_isar5 = 0x00011121;
>>>      cpu->id_aa64pfr0 = 0x00002222;
>>>      cpu->id_aa64dfr0 = 0x10305106;
>>> +    cpu->pmceid0 = 0x00000000;
>>> +    cpu->pmceid1 = 0x00000000;
>>>      cpu->id_aa64isar0 = 0x00011120;
>>>      cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
>>>      cpu->dbgdidr = 0x3516d000;
>>>
>>
>> Maybe we can move this at a single place in arm_cpu_post_init():
>>
>>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>>         cpu->pmceid0 = 0x00000000;
>>         cpu->pmceid1 = 0x00000000;
>>     }
> 
> I like consolidating the initialization - though I think it can go in
> arm_cpu_realizefn() with the preexisting PMU-related id_aa64dfr0
> initialization since it is constant once you've chosen a type of
> processor. One of the other patches in this set actually already adds
> some PMCEID initialization there based on PMCR.N.

Indeed, arm_cpu_realizefn() is a good place.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Aaron Lindsay]

On Mar 20 02:03, Philippe Mathieu-Daudé wrote:
> On 03/19/2018 09:35 PM, Aaron Lindsay wrote:
> > On Mar 18 23:35, Philippe Mathieu-Daudé wrote:
> >> Hi Aaron,
> >>
> >> On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
> >>> A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
> >>> pmceid[01] are already being initialized to zero for both A15 and A57.
> >>>
> >>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> >>> ---
> >>>  target/arm/cpu64.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> >>> index 991d764..8c4db31 100644
> >>> --- a/target/arm/cpu64.c
> >>> +++ b/target/arm/cpu64.c
> >>> @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
> >>>      cpu->id_isar5 = 0x00011121;
> >>>      cpu->id_aa64pfr0 = 0x00002222;
> >>>      cpu->id_aa64dfr0 = 0x10305106;
> >>> +    cpu->pmceid0 = 0x00000000;
> >>> +    cpu->pmceid1 = 0x00000000;
> >>>      cpu->id_aa64isar0 = 0x00011120;
> >>>      cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
> >>>      cpu->dbgdidr = 0x3516d000;
> >>>
> >>
> >> Maybe we can move this at a single place in arm_cpu_post_init():
> >>
> >>     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
> >>         cpu->pmceid0 = 0x00000000;
> >>         cpu->pmceid1 = 0x00000000;
> >>     }
> > 
> > I like consolidating the initialization - though I think it can go in
> > arm_cpu_realizefn() with the preexisting PMU-related id_aa64dfr0
> > initialization since it is constant once you've chosen a type of
> > processor. One of the other patches in this set actually already adds
> > some PMCEID initialization there based on PMCR.N.
> 
> Indeed, arm_cpu_realizefn() is a good place.

I've consolidated the pmceid[01] initialization into arm_cpu_realizefn()
for v4.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 02/22] target/arm: A15 PMCEID0 initialization style nit

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 022d8c5..072cbbf 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1524,7 +1524,7 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
-    cpu->pmceid0 = 0x0000000;
+    cpu->pmceid0 = 0x00000000;
     cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 02/22] target/arm: A15 PMCEID0 initialization style nit

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 022d8c5..072cbbf 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1524,7 +1524,7 @@ static void cortex_a15_initfn(Object *obj)
>      cpu->id_pfr0 = 0x00001131;
>      cpu->id_pfr1 = 0x00011011;
>      cpu->id_dfr0 = 0x02010555;
> -    cpu->pmceid0 = 0x0000000;
> +    cpu->pmceid0 = 0x00000000;
>      cpu->pmceid1 = 0x00000000;
>      cpu->id_afr0 = 0x00000000;
>      cpu->id_mmfr0 = 0x10201105;

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 03/22] target/arm: Check PMCNTEN for whether PMCCNTR is enabled

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 09893e3..5e48982 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -994,7 +994,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 {
     /* This does not support checking PMCCFILTR_EL0 register */
 
-    if (!(env->cp15.c9_pmcr & PMCRE)) {
+    if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
         return false;
     }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 04/22] target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0

[Aaron Lindsay]

They share the same underlying state

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5e48982..5634561 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1318,7 +1318,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
       .writefn = pmselr_write, .raw_writefn = raw_write, },
     { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
-      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
+      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
       .readfn = pmccntr_read, .writefn = pmccntr_write32,
       .accessfn = pmreg_access_ccntr },
     { .name = "PMCCNTR_EL0", .state = ARM_CP_STATE_AA64,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 04/22] target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> They share the same underlying state
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5e48982..5634561 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1318,7 +1318,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
>        .writefn = pmselr_write, .raw_writefn = raw_write, },
>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> +      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
>        .readfn = pmccntr_read, .writefn = pmccntr_write32,
>        .accessfn = pmreg_access_ccntr },
>      { .name = "PMCCNTR_EL0", .state = ARM_CP_STATE_AA64,
> --

Does this fix an observed bug (presumably migration related),
or is it just something you saw in code inspection ?
Worth noting in the commit message if the former.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 04/22] target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0

[Aaron Lindsay]

On Apr 12 17:10, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > They share the same underlying state
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 5e48982..5634561 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1318,7 +1318,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
> >        .writefn = pmselr_write, .raw_writefn = raw_write, },
> >      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> > -      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> > +      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
> >        .readfn = pmccntr_read, .writefn = pmccntr_write32,
> >        .accessfn = pmreg_access_ccntr },
> >      { .name = "PMCCNTR_EL0", .state = ARM_CP_STATE_AA64,
> > --
> 
> Does this fix an observed bug (presumably migration related),
> or is it just something you saw in code inspection ?
> Worth noting in the commit message if the former.

Purely code inspection. I forget now, but I think I found it after
chasing down some other register issue in the vicinity and noticed this
while digging around.

-Aaron

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync

[Aaron Lindsay]

pmccntr_read and pmccntr_write contained duplicate code that was already
being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start
and pmccntr_op_finish, passing the clock value between the two, to avoid
losing time between the two calls.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 101 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 56 insertions(+), 45 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5634561..6480b80 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 
     return true;
 }
-
-void pmccntr_sync(CPUARMState *env)
+/*
+ * Ensure c15_ccnt is the guest-visible count so that operations such as
+ * enabling/disabling the counter or filtering, modifying the count itself,
+ * etc. can be done logically. This is essentially a no-op if the counter is
+ * not enabled at the time of the call.
+ *
+ * The current cycle count is returned so that it can be passed into the paired
+ * pmccntr_op_finish() call which must follow each call to pmccntr_op_start().
+ */
+uint64_t pmccntr_op_start(CPUARMState *env)
 {
-    uint64_t temp_ticks;
-
-    temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+    uint64_t cycles = 0;
+#ifndef CONFIG_USER_ONLY
+    cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#endif
+
+    if (arm_ccnt_enabled(env)) {
 
-    if (env->cp15.c9_pmcr & PMCRD) {
-        /* Increment once every 64 processor clock cycles */
-        temp_ticks /= 64;
+        uint64_t eff_cycles = cycles;
+        if (env->cp15.c9_pmcr & PMCRD) {
+            /* Increment once every 64 processor clock cycles */
+            eff_cycles /= 64;
+        }
+
+        env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt;
     }
+    return cycles;
+}
 
+/*
+ * If enabled, convert c15_ccnt back into the delta between the clock and the
+ * guest-visible count. A call to pmccntr_op_finish should follow every call to
+ * pmccntr_op_start.
+ */
+void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
+{
     if (arm_ccnt_enabled(env)) {
-        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+
+        if (env->cp15.c9_pmcr & PMCRD) {
+            /* Increment once every 64 processor clock cycles */
+            prev_cycles /= 64;
+        }
+
+        env->cp15.c15_ccnt = prev_cycles - env->cp15.c15_ccnt;
     }
 }
 
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    pmccntr_sync(env);
+    uint64_t saved_cycles = pmccntr_op_start(env);
 
     if (value & PMCRC) {
         /* The counter has been reset */
@@ -1032,26 +1062,16 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
 
-    pmccntr_sync(env);
+    pmccntr_op_finish(env, saved_cycles);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    uint64_t total_ticks;
-
-    if (!arm_ccnt_enabled(env)) {
-        /* Counter is disabled, do not change value */
-        return env->cp15.c15_ccnt;
-    }
-
-    total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-    if (env->cp15.c9_pmcr & PMCRD) {
-        /* Increment once every 64 processor clock cycles */
-        total_ticks /= 64;
-    }
-    return total_ticks - env->cp15.c15_ccnt;
+    uint64_t ret;
+    uint64_t saved_cycles = pmccntr_op_start(env);
+    ret = env->cp15.c15_ccnt;
+    pmccntr_op_finish(env, saved_cycles);
+    return ret;
 }
 
 static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1068,22 +1088,9 @@ static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
-    uint64_t total_ticks;
-
-    if (!arm_ccnt_enabled(env)) {
-        /* Counter is disabled, set the absolute value */
-        env->cp15.c15_ccnt = value;
-        return;
-    }
-
-    total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-    if (env->cp15.c9_pmcr & PMCRD) {
-        /* Increment once every 64 processor clock cycles */
-        total_ticks /= 64;
-    }
-    env->cp15.c15_ccnt = total_ticks - value;
+    uint64_t saved_cycles = pmccntr_op_start(env);
+    env->cp15.c15_ccnt = value;
+    pmccntr_op_finish(env, saved_cycles);
 }
 
 static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1096,7 +1103,11 @@ static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
 
 #else /* CONFIG_USER_ONLY */
 
-void pmccntr_sync(CPUARMState *env)
+uint64_t pmccntr_op_start(CPUARMState *env)
+{
+}
+
+void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
 }
 
@@ -1105,9 +1116,9 @@ void pmccntr_sync(CPUARMState *env)
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
-    pmccntr_sync(env);
+    uint64_t saved_cycles = pmccntr_op_start(env);
     env->cp15.pmccfiltr_el0 = value & 0x7E000000;
-    pmccntr_sync(env);
+    pmccntr_op_finish(env, saved_cycles);
 }
 
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> pmccntr_read and pmccntr_write contained duplicate code that was already
> being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start
> and pmccntr_op_finish, passing the clock value between the two, to avoid
> losing time between the two calls.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 101 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 56 insertions(+), 45 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5634561..6480b80 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
>
>      return true;
>  }
> -
> -void pmccntr_sync(CPUARMState *env)

If you configure your git to use the 'histogram' diff algorithm
("git config --global diff.algorithm histogram", or edit ~/.gitconfig
equivalently), does it make git format-patch make less of a mess
of this commit ?

> +/*
> + * Ensure c15_ccnt is the guest-visible count so that operations such as
> + * enabling/disabling the counter or filtering, modifying the count itself,
> + * etc. can be done logically. This is essentially a no-op if the counter is
> + * not enabled at the time of the call.
> + *
> + * The current cycle count is returned so that it can be passed into the paired
> + * pmccntr_op_finish() call which must follow each call to pmccntr_op_start().
> + */
> +uint64_t pmccntr_op_start(CPUARMState *env)
>  {
> -    uint64_t temp_ticks;
> -
> -    temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +    uint64_t cycles = 0;
> +#ifndef CONFIG_USER_ONLY
> +    cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>                            ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> +#endif

Is this ifdef necessary? You have a do-nothing version of
pmccntr_op_start() for CONFIG_USER_ONLY later on, so presumably
this one is already inside a suitable ifndef.

Otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync

[Aaron Lindsay]

On Apr 12 17:18, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > pmccntr_read and pmccntr_write contained duplicate code that was already
> > being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start
> > and pmccntr_op_finish, passing the clock value between the two, to avoid
> > losing time between the two calls.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/helper.c | 101 +++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 56 insertions(+), 45 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 5634561..6480b80 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
> >
> >      return true;
> >  }
> > -
> > -void pmccntr_sync(CPUARMState *env)
> 
> If you configure your git to use the 'histogram' diff algorithm
> ("git config --global diff.algorithm histogram", or edit ~/.gitconfig
> equivalently), does it make git format-patch make less of a mess
> of this commit ?

No, it doesn't seem to make much of a difference.

> > +/*
> > + * Ensure c15_ccnt is the guest-visible count so that operations such as
> > + * enabling/disabling the counter or filtering, modifying the count itself,
> > + * etc. can be done logically. This is essentially a no-op if the counter is
> > + * not enabled at the time of the call.
> > + *
> > + * The current cycle count is returned so that it can be passed into the paired
> > + * pmccntr_op_finish() call which must follow each call to pmccntr_op_start().
> > + */
> > +uint64_t pmccntr_op_start(CPUARMState *env)
> >  {
> > -    uint64_t temp_ticks;
> > -
> > -    temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > +    uint64_t cycles = 0;
> > +#ifndef CONFIG_USER_ONLY
> > +    cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> >                            ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> > +#endif
> 
> Is this ifdef necessary? You have a do-nothing version of
> pmccntr_op_start() for CONFIG_USER_ONLY later on, so presumably
> this one is already inside a suitable ifndef.

You're right that it is unnecessary as of this patch. A later patch
removes the surrounding `#ifndef CONFIG_USER_ONLY` when it is no longer
necessary at that level. It would be cleaner to add the #ifndef with
smaller scope at the same time - I'll make that change.  

> Otherwise
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Because I've modified how pmccntr_op_start/finish keep track of the
cycles so that they store the counter values differently for v4, I don't
feel comfortable adding your Reviewed-by. I apologize for the churn.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 06/22] target/arm: Mask PMU register writes based on PMCR_EL0.N

[Aaron Lindsay]

This is in preparation for enabling counters other than PMCCNTR

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6480b80..5d5c738 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -52,11 +52,6 @@ typedef struct V8M_SAttributes {
 static void v8m_security_lookup(CPUARMState *env, uint32_t address,
                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
                                 V8M_SAttributes *sattrs);
-
-/* Definitions for the PMCCNTR and PMCR registers */
-#define PMCRD   0x8
-#define PMCRC   0x4
-#define PMCRE   0x1
 #endif
 
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -906,6 +901,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
+/* Definitions for the PMU registers */
+#define PMCRN_MASK  0xf800
+#define PMCRN_SHIFT 11
+#define PMCRD   0x8
+#define PMCRC   0x4
+#define PMCRE   0x1
+
+#define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
+/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
+#define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                    bool isread)
 {
@@ -1124,14 +1130,14 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
-    value &= (1 << 31);
+    value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pmcnten |= value;
 }
 
 static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
-    value &= (1 << 31);
+    value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pmcnten &= ~value;
 }
 
@@ -1179,14 +1185,14 @@ static void pmintenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
     /* We have no event counters so only the C bit can be changed */
-    value &= (1 << 31);
+    value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pminten |= value;
 }
 
 static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
-    value &= (1 << 31);
+    value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pminten &= ~value;
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 06/22] target/arm: Mask PMU register writes based on PMCR_EL0.N

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> This is in preparation for enabling counters other than PMCCNTR
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 6480b80..5d5c738 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -52,11 +52,6 @@ typedef struct V8M_SAttributes {
>  static void v8m_security_lookup(CPUARMState *env, uint32_t address,
>                                  MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                  V8M_SAttributes *sattrs);
> -
> -/* Definitions for the PMCCNTR and PMCR registers */
> -#define PMCRD   0x8
> -#define PMCRC   0x4
> -#define PMCRE   0x1
>  #endif
>
>  static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> @@ -906,6 +901,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> +/* Definitions for the PMU registers */
> +#define PMCRN_MASK  0xf800
> +#define PMCRN_SHIFT 11
> +#define PMCRD   0x8
> +#define PMCRC   0x4
> +#define PMCRE   0x1
> +
> +#define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
> +/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
> +#define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))

These would be better as inline functions I think.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 07/22] target/arm: Fetch GICv3 state directly from CPUARMState

[Aaron Lindsay]

This eliminates the need for fetching it from el_change_hook_opaque, and
allows for supporting multiple el_change_hooks without having to hack
something together to find the registered opaque belonging to GICv3.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 hw/intc/arm_gicv3_cpuif.c | 10 ++--------
 target/arm/cpu.h          | 10 ----------
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 5cbafaf..801f91b 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -29,11 +29,7 @@ void gicv3_set_gicv3state(CPUState *cpu, GICv3CPUState *s)
 
 static GICv3CPUState *icc_cs_from_env(CPUARMState *env)
 {
-    /* Given the CPU, find the right GICv3CPUState struct.
-     * Since we registered the CPU interface with the EL change hook as
-     * the opaque pointer, we can just directly get from the CPU to it.
-     */
-    return arm_get_el_change_hook_opaque(arm_env_get_cpu(env));
+    return env->gicv3state;
 }
 
 static bool gicv3_use_ns_bank(CPUARMState *env)
@@ -2615,9 +2611,7 @@ void gicv3_init_cpuif(GICv3State *s)
          * it might be with code translated by CPU 0 but run by CPU 1, in
          * which case we'd get the wrong value.
          * So instead we define the regs with no ri->opaque info, and
-         * get back to the GICv3CPUState from the ARMCPU by reading back
-         * the opaque pointer from the el_change_hook, which we're going
-         * to need to register anyway.
+         * get back to the GICv3CPUState from the CPUARMState.
          */
         define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);
         if (arm_feature(&cpu->env, ARM_FEATURE_EL2)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1e7e1f8..f17592b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2904,16 +2904,6 @@ void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
                                  void *opaque);
 
 /**
- * arm_get_el_change_hook_opaque:
- * Return the opaque data that will be used by the el_change_hook
- * for this CPU.
- */
-static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
-{
-    return cpu->el_change_hook_opaque;
-}
-
-/**
  * aa32_vfp_dreg:
  * Return a pointer to the Dn register within env in 32-bit mode.
  */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 07/22] target/arm: Fetch GICv3 state directly from CPUARMState

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> This eliminates the need for fetching it from el_change_hook_opaque, and
> allows for supporting multiple el_change_hooks without having to hack
> something together to find the registered opaque belonging to GICv3.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  hw/intc/arm_gicv3_cpuif.c | 10 ++--------
>  target/arm/cpu.h          | 10 ----------
>  2 files changed, 2 insertions(+), 18 deletions(-)

I'm not wonderfully happy about this, because the original aim
here was to try to keep the GICv3 device end reasonably decoupled
from the CPU proper. On the other hand since d3a3e529626fb we've had
this pointer because we ended up needing it in the KVM reset codepath,
so we might as well use it here too. Maybe one day I'll come back
to this and clean it up.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c       | 15 ++++++++++-----
 target/arm/cpu.h       | 23 ++++++++++++-----------
 target/arm/internals.h |  7 ++++---
 3 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 072cbbf..5f782bf 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
          | CPU_INTERRUPT_EXITTB);
 }
 
-void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
+void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
                                  void *opaque)
 {
-    /* We currently only support registering a single hook function */
-    assert(!cpu->el_change_hook);
-    cpu->el_change_hook = hook;
-    cpu->el_change_hook_opaque = opaque;
+    ARMELChangeHook *entry;
+    entry = g_malloc0(sizeof (*entry));
+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
 }
 
 static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
@@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    QLIST_INIT(&cpu->el_change_hooks);
+
     /* Some features automatically imply others: */
     if (arm_feature(env, ARM_FEATURE_V8)) {
         set_feature(env, ARM_FEATURE_V7);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f17592b..3b45d3d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -633,11 +633,17 @@ typedef struct CPUARMState {
 
 /**
  * ARMELChangeHook:
- * type of a function which can be registered via arm_register_el_change_hook()
- * to get callbacks when the CPU changes its exception level or mode.
+ * Support registering functions with ARMELChangeHookFn's signature via
+ * arm_register_el_change_hook() to get callbacks when the CPU changes its
+ * exception level or mode.
  */
-typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque);
-
+typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque);
+typedef struct ARMELChangeHook ARMELChangeHook;
+struct ARMELChangeHook {
+    ARMELChangeHookFn *hook;
+    void *opaque;
+    QLIST_ENTRY(ARMELChangeHook) node;
+};
 
 /* These values map onto the return values for
  * QEMU_PSCI_0_2_FN_AFFINITY_INFO */
@@ -826,8 +832,7 @@ struct ARMCPU {
      */
     bool cfgend;
 
-    ARMELChangeHook *el_change_hook;
-    void *el_change_hook_opaque;
+    QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
 
     int32_t node_id; /* NUMA node this CPU belongs to */
 
@@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
  * CPU changes exception level or mode. The hook function will be
  * passed a pointer to the ARMCPU and the opaque data pointer passed
  * to this function when the hook was registered.
- *
- * Note that we currently only support registering a single hook function,
- * and will assert if this function is called twice.
- * This facility is intended for the use of the GICv3 emulation.
  */
-void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
+void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
                                  void *opaque);
 
 /**
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 47cc224..7df3eda 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
                                    int mmu_idx, MemTxAttrs attrs,
                                    MemTxResult response, uintptr_t retaddr);
 
-/* Call the EL change hook if one has been registered */
+/* Call any registered EL change hooks */
 static inline void arm_call_el_change_hook(ARMCPU *cpu)
 {
-    if (cpu->el_change_hook) {
-        cpu->el_change_hook(cpu, cpu->el_change_hook_opaque);
+    ARMELChangeHook *hook, *next;
+    QLIST_FOREACH_SAFE(hook, &cpu->el_change_hooks, node, next) {
+        hook->hook(cpu, hook->opaque);
     }
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.c       | 15 ++++++++++-----
>  target/arm/cpu.h       | 23 ++++++++++++-----------
>  target/arm/internals.h |  7 ++++---
>  3 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 072cbbf..5f782bf 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
>           | CPU_INTERRUPT_EXITTB);
>  }
>  
> -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
>                                   void *opaque)
>  {
> -    /* We currently only support registering a single hook function */
> -    assert(!cpu->el_change_hook);
> -    cpu->el_change_hook = hook;
> -    cpu->el_change_hook_opaque = opaque;
> +    ARMELChangeHook *entry;
> +    entry = g_malloc0(sizeof (*entry));

imho g_malloc() is enough.

> +
> +    entry->hook = hook;
> +    entry->opaque = opaque;
> +
> +    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
>  }
>  
>  static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
> @@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    QLIST_INIT(&cpu->el_change_hooks);
> +

You missed to fill arm_cpu_unrealizefn() with:

    QLIST_FOREACH(...) {
        QLIST_REMOVE(...);
        g_free(...);
    }

>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          set_feature(env, ARM_FEATURE_V7);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f17592b..3b45d3d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -633,11 +633,17 @@ typedef struct CPUARMState {
>  
>  /**
>   * ARMELChangeHook:
> - * type of a function which can be registered via arm_register_el_change_hook()
> - * to get callbacks when the CPU changes its exception level or mode.
> + * Support registering functions with ARMELChangeHookFn's signature via
> + * arm_register_el_change_hook() to get callbacks when the CPU changes its
> + * exception level or mode.
>   */
> -typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque);
> -
> +typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque);
> +typedef struct ARMELChangeHook ARMELChangeHook;
> +struct ARMELChangeHook {
> +    ARMELChangeHookFn *hook;
> +    void *opaque;
> +    QLIST_ENTRY(ARMELChangeHook) node;
> +};
>  
>  /* These values map onto the return values for
>   * QEMU_PSCI_0_2_FN_AFFINITY_INFO */
> @@ -826,8 +832,7 @@ struct ARMCPU {
>       */
>      bool cfgend;
>  
> -    ARMELChangeHook *el_change_hook;
> -    void *el_change_hook_opaque;
> +    QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
>  
>      int32_t node_id; /* NUMA node this CPU belongs to */
>  
> @@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
>   * CPU changes exception level or mode. The hook function will be
>   * passed a pointer to the ARMCPU and the opaque data pointer passed
>   * to this function when the hook was registered.
> - *
> - * Note that we currently only support registering a single hook function,
> - * and will assert if this function is called twice.
> - * This facility is intended for the use of the GICv3 emulation.
>   */
> -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
>                                   void *opaque);
>  
>  /**
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 47cc224..7df3eda 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>                                     int mmu_idx, MemTxAttrs attrs,
>                                     MemTxResult response, uintptr_t retaddr);
>  
> -/* Call the EL change hook if one has been registered */
> +/* Call any registered EL change hooks */
>  static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  {
> -    if (cpu->el_change_hook) {
> -        cpu->el_change_hook(cpu, cpu->el_change_hook_opaque);
> +    ARMELChangeHook *hook, *next;
> +    QLIST_FOREACH_SAFE(hook, &cpu->el_change_hooks, node, next) {
> +        hook->hook(cpu, hook->opaque);
>      }
>  }
>  
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Aaron Lindsay]

On Mar 18 23:41, Philippe Mathieu-Daudé wrote:
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.c       | 15 ++++++++++-----
> >  target/arm/cpu.h       | 23 ++++++++++++-----------
> >  target/arm/internals.h |  7 ++++---
> >  3 files changed, 26 insertions(+), 19 deletions(-)
> > 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 072cbbf..5f782bf 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
> >           | CPU_INTERRUPT_EXITTB);
> >  }
> >  
> > -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> > +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> >                                   void *opaque)
> >  {
> > -    /* We currently only support registering a single hook function */
> > -    assert(!cpu->el_change_hook);
> > -    cpu->el_change_hook = hook;
> > -    cpu->el_change_hook_opaque = opaque;
> > +    ARMELChangeHook *entry;
> > +    entry = g_malloc0(sizeof (*entry));
> 
> imho g_malloc() is enough.

It seems like the only difference is between initializing it to zero
(g_malloc0) and making it as uninitialized (g_malloc) for coverity. Are
there coding standards for when we should choose which?

> 
> > +
> > +    entry->hook = hook;
> > +    entry->opaque = opaque;
> > +
> > +    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
> >  }
> >  
> >  static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
> > @@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >  
> > +    QLIST_INIT(&cpu->el_change_hooks);
> > +
> 
> You missed to fill arm_cpu_unrealizefn() with:
> 
>     QLIST_FOREACH(...) {
>         QLIST_REMOVE(...);
>         g_free(...);
>     }

Do you mean arm_cpu_finalizefn()?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Philippe Mathieu-Daudé]

Le 20 mars 2018 9:45 PM, "Aaron Lindsay" <alindsay@codeaurora.org> a écrit :

On Mar 18 23:41, Philippe Mathieu-Daudé wrote:
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.c       | 15 ++++++++++-----
> >  target/arm/cpu.h       | 23 ++++++++++++-----------
> >  target/arm/internals.h |  7 ++++---
> >  3 files changed, 26 insertions(+), 19 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 072cbbf..5f782bf 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
> >           | CPU_INTERRUPT_EXITTB);
> >  }
> >
> > -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> > +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> >                                   void *opaque)
> >  {
> > -    /* We currently only support registering a single hook function */
> > -    assert(!cpu->el_change_hook);
> > -    cpu->el_change_hook = hook;
> > -    cpu->el_change_hook_opaque = opaque;
> > +    ARMELChangeHook *entry;
> > +    entry = g_malloc0(sizeof (*entry));
>
> imho g_malloc() is enough.

It seems like the only difference is between initializing it to zero
(g_malloc0) and making it as uninitialized (g_malloc) for coverity. Are
there coding standards for when we should choose which?


Since you initialize all members, bzero is not necessary; until someone add
another member to the structure. So your way is correct and safer, with a
ridiculous performance penalty.



>
> > +
> > +    entry->hook = hook;
> > +    entry->opaque = opaque;
> > +
> > +    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
> >  }
> >
> >  static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
> > @@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev,
Error **errp)
> >          return;
> >      }
> >
> > +    QLIST_INIT(&cpu->el_change_hooks);
> > +
>
> You missed to fill arm_cpu_unrealizefn() with:
>
>     QLIST_FOREACH(...) {
>         QLIST_REMOVE(...);
>         g_free(...);
>     }

Do you mean arm_cpu_finalizefn()?


Yes :)



-Aaron


--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.c       | 15 ++++++++++-----
>  target/arm/cpu.h       | 23 ++++++++++++-----------
>  target/arm/internals.h |  7 ++++---
>  3 files changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 072cbbf..5f782bf 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
>           | CPU_INTERRUPT_EXITTB);
>  }
>
> -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
>                                   void *opaque)
>  {
> -    /* We currently only support registering a single hook function */
> -    assert(!cpu->el_change_hook);
> -    cpu->el_change_hook = hook;
> -    cpu->el_change_hook_opaque = opaque;
> +    ARMELChangeHook *entry;
> +    entry = g_malloc0(sizeof (*entry));

g_new0(ARMELChangeHook, 1) is nicer for initializing a thing of
known size.

> +
> +    entry->hook = hook;
> +    entry->opaque = opaque;
> +
> +    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
>  }
>
>  static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
> @@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>
> +    QLIST_INIT(&cpu->el_change_hooks);
> +

If you put this in arm_cpu_initfn() you don't have to wonder
whether anybody's calling arm_register_el_change_hook() after
the CPU object has been created but before it is realized...

>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          set_feature(env, ARM_FEATURE_V7);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f17592b..3b45d3d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -633,11 +633,17 @@ typedef struct CPUARMState {
>
>  /**
>   * ARMELChangeHook:
> - * type of a function which can be registered via arm_register_el_change_hook()
> - * to get callbacks when the CPU changes its exception level or mode.
> + * Support registering functions with ARMELChangeHookFn's signature via
> + * arm_register_el_change_hook() to get callbacks when the CPU changes its
> + * exception level or mode.
>   */

This is supposed to be the doc comment for the type,
which I think was fine as it is, apart from the name change.
If you want to also have a doc comment for the struct type
that's fine but would be something different (and is less
necessary, because the struct type is internal to the CPU
whereas the function type is part of its API to the rest of
the system).

> -typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque);
> -
> +typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque);
> +typedef struct ARMELChangeHook ARMELChangeHook;
> +struct ARMELChangeHook {
> +    ARMELChangeHookFn *hook;
> +    void *opaque;
> +    QLIST_ENTRY(ARMELChangeHook) node;
> +};
>
>  /* These values map onto the return values for
>   * QEMU_PSCI_0_2_FN_AFFINITY_INFO */
> @@ -826,8 +832,7 @@ struct ARMCPU {
>       */
>      bool cfgend;
>
> -    ARMELChangeHook *el_change_hook;
> -    void *el_change_hook_opaque;
> +    QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
>
>      int32_t node_id; /* NUMA node this CPU belongs to */
>
> @@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
>   * CPU changes exception level or mode. The hook function will be
>   * passed a pointer to the ARMCPU and the opaque data pointer passed
>   * to this function when the hook was registered.
> - *
> - * Note that we currently only support registering a single hook function,
> - * and will assert if this function is called twice.
> - * This facility is intended for the use of the GICv3 emulation.
>   */
> -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
>                                   void *opaque);
>
>  /**
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 47cc224..7df3eda 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>                                     int mmu_idx, MemTxAttrs attrs,
>                                     MemTxResult response, uintptr_t retaddr);
>
> -/* Call the EL change hook if one has been registered */
> +/* Call any registered EL change hooks */
>  static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  {
> -    if (cpu->el_change_hook) {
> -        cpu->el_change_hook(cpu, cpu->el_change_hook_opaque);
> +    ARMELChangeHook *hook, *next;
> +    QLIST_FOREACH_SAFE(hook, &cpu->el_change_hooks, node, next) {
> +        hook->hook(cpu, hook->opaque);
>      }
>  }
>

thanks
-- PMM

[Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL change hooks

[Aaron Lindsay]

Because the design of the PMU requires that the counter values be
converted between their delta and guest-visible forms for mode
filtering, an additional hook which occurs before the EL is changed is
necessary.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c       | 13 +++++++++++++
 target/arm/cpu.h       | 12 ++++++++----
 target/arm/helper.c    | 14 ++++++++------
 target/arm/internals.h |  7 +++++++
 target/arm/op_helper.c |  8 ++++++++
 5 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5f782bf..a2cb21e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -55,6 +55,18 @@ static bool arm_cpu_has_work(CPUState *cs)
          | CPU_INTERRUPT_EXITTB);
 }
 
+void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
+                                 void *opaque)
+{
+    ARMELChangeHook *entry;
+    entry = g_malloc0(sizeof (*entry));
+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->pre_el_change_hooks, entry, node);
+}
+
 void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
                                  void *opaque)
 {
@@ -747,6 +759,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    QLIST_INIT(&cpu->pre_el_change_hooks);
     QLIST_INIT(&cpu->el_change_hooks);
 
     /* Some features automatically imply others: */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3b45d3d..b0ef727 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -832,6 +832,7 @@ struct ARMCPU {
      */
     bool cfgend;
 
+    QLIST_HEAD(, ARMELChangeHook) pre_el_change_hooks;
     QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
 
     int32_t node_id; /* NUMA node this CPU belongs to */
@@ -2895,12 +2896,15 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
 #endif
 
 /**
+ * arm_register_pre_el_change_hook:
  * arm_register_el_change_hook:
- * Register a hook function which will be called back whenever this
- * CPU changes exception level or mode. The hook function will be
- * passed a pointer to the ARMCPU and the opaque data pointer passed
- * to this function when the hook was registered.
+ * Register a hook function which will be called back before or after this CPU
+ * changes exception level or mode. The hook function will be passed a pointer
+ * to the ARMCPU and the opaque data pointer passed to this function when the
+ * hook was registered.
  */
+void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
+                                 void *opaque);
 void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
                                  void *opaque);
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5d5c738..50eaed7 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8253,6 +8253,14 @@ void arm_cpu_do_interrupt(CPUState *cs)
         return;
     }
 
+    /* Hooks may change global state so BQL should be held, also the
+     * BQL needs to be held for any modification of
+     * cs->interrupt_request.
+     */
+    g_assert(qemu_mutex_iothread_locked());
+
+    arm_call_pre_el_change_hook(cpu);
+
     assert(!excp_is_internal(cs->exception_index));
     if (arm_el_is_aa64(env, new_el)) {
         arm_cpu_do_interrupt_aarch64(cs);
@@ -8260,12 +8268,6 @@ void arm_cpu_do_interrupt(CPUState *cs)
         arm_cpu_do_interrupt_aarch32(cs);
     }
 
-    /* Hooks may change global state so BQL should be held, also the
-     * BQL needs to be held for any modification of
-     * cs->interrupt_request.
-     */
-    g_assert(qemu_mutex_iothread_locked());
-
     arm_call_el_change_hook(cpu);
 
     if (!kvm_enabled()) {
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 7df3eda..6ea6766 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -728,6 +728,13 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
                                    MemTxResult response, uintptr_t retaddr);
 
 /* Call any registered EL change hooks */
+static inline void arm_call_pre_el_change_hook(ARMCPU *cpu)
+{
+    ARMELChangeHook *hook, *next;
+    QLIST_FOREACH_SAFE(hook, &cpu->pre_el_change_hooks, node, next) {
+        hook->hook(cpu, hook->opaque);
+    }
+}
 static inline void arm_call_el_change_hook(ARMCPU *cpu)
 {
     ARMELChangeHook *hook, *next;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 7a88fd2..be417ce 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -496,6 +496,10 @@ void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, uint32_t mask)
 /* Write the CPSR for a 32-bit exception return */
 void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
 {
+    qemu_mutex_lock_iothread();
+    arm_call_pre_el_change_hook(arm_env_get_cpu(env));
+    qemu_mutex_unlock_iothread();
+
     cpsr_write(env, val, CPSR_ERET_MASK, CPSRWriteExceptionReturn);
 
     /* Generated code has already stored the new PC value, but
@@ -1013,6 +1017,10 @@ void HELPER(exception_return)(CPUARMState *env)
         goto illegal_return;
     }
 
+    qemu_mutex_lock_iothread();
+    arm_call_pre_el_change_hook(arm_env_get_cpu(env));
+    qemu_mutex_unlock_iothread();
+
     if (!return_to_aa64) {
         env->aarch64 = 0;
         /* We do a raw CPSR write because aarch64_sync_64_to_32()
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL change hooks

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Because the design of the PMU requires that the counter values be
> converted between their delta and guest-visible forms for mode
> filtering, an additional hook which occurs before the EL is changed is
> necessary.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.c       | 13 +++++++++++++
>  target/arm/cpu.h       | 12 ++++++++----
>  target/arm/helper.c    | 14 ++++++++------
>  target/arm/internals.h |  7 +++++++
>  target/arm/op_helper.c |  8 ++++++++
>  5 files changed, 44 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5f782bf..a2cb21e 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -55,6 +55,18 @@ static bool arm_cpu_has_work(CPUState *cs)
>           | CPU_INTERRUPT_EXITTB);
>  }
>
> +void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> +                                 void *opaque)
> +{
> +    ARMELChangeHook *entry;
> +    entry = g_malloc0(sizeof (*entry));

g_new0().

> +
> +    entry->hook = hook;
> +    entry->opaque = opaque;
> +
> +    QLIST_INSERT_HEAD(&cpu->pre_el_change_hooks, entry, node);
> +}
> +
>  void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
>                                   void *opaque)
>  {
> @@ -747,6 +759,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>
> +    QLIST_INIT(&cpu->pre_el_change_hooks);
>      QLIST_INIT(&cpu->el_change_hooks);
>
>      /* Some features automatically imply others: */
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 3b45d3d..b0ef727 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -832,6 +832,7 @@ struct ARMCPU {
>       */
>      bool cfgend;
>
> +    QLIST_HEAD(, ARMELChangeHook) pre_el_change_hooks;
>      QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
>
>      int32_t node_id; /* NUMA node this CPU belongs to */
> @@ -2895,12 +2896,15 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
>  #endif
>
>  /**
> + * arm_register_pre_el_change_hook:
>   * arm_register_el_change_hook:
> - * Register a hook function which will be called back whenever this
> - * CPU changes exception level or mode. The hook function will be
> - * passed a pointer to the ARMCPU and the opaque data pointer passed
> - * to this function when the hook was registered.
> + * Register a hook function which will be called back before or after this CPU
> + * changes exception level or mode. The hook function will be passed a pointer
> + * to the ARMCPU and the opaque data pointer passed to this function when the
> + * hook was registered.
>   */

I would just have one doc comment for each function, rather than
trying to share. (Some day we may actually autogenerate HTML docs
from these comments...)

Do we make the guarantee that if we call the pre-change hook
then we will definitely subsequently call the post-change hook?
(ie does the PMU hook rely on that?)

Otherwise looks OK.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL change hooks

[Aaron Lindsay]

On Apr 12 17:49, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > Because the design of the PMU requires that the counter values be
> > converted between their delta and guest-visible forms for mode
> > filtering, an additional hook which occurs before the EL is changed is
> > necessary.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.c       | 13 +++++++++++++
> >  target/arm/cpu.h       | 12 ++++++++----
> >  target/arm/helper.c    | 14 ++++++++------
> >  target/arm/internals.h |  7 +++++++
> >  target/arm/op_helper.c |  8 ++++++++
> >  5 files changed, 44 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 5f782bf..a2cb21e 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -55,6 +55,18 @@ static bool arm_cpu_has_work(CPUState *cs)
> >           | CPU_INTERRUPT_EXITTB);
> >  }
> >
> > +void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> > +                                 void *opaque)
> > +{
> > +    ARMELChangeHook *entry;
> > +    entry = g_malloc0(sizeof (*entry));
> 
> g_new0().
> 
> > +
> > +    entry->hook = hook;
> > +    entry->opaque = opaque;
> > +
> > +    QLIST_INSERT_HEAD(&cpu->pre_el_change_hooks, entry, node);
> > +}
> > +
> >  void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> >                                   void *opaque)
> >  {
> > @@ -747,6 +759,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >
> > +    QLIST_INIT(&cpu->pre_el_change_hooks);
> >      QLIST_INIT(&cpu->el_change_hooks);
> >
> >      /* Some features automatically imply others: */
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 3b45d3d..b0ef727 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -832,6 +832,7 @@ struct ARMCPU {
> >       */
> >      bool cfgend;
> >
> > +    QLIST_HEAD(, ARMELChangeHook) pre_el_change_hooks;
> >      QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
> >
> >      int32_t node_id; /* NUMA node this CPU belongs to */
> > @@ -2895,12 +2896,15 @@ static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
> >  #endif
> >
> >  /**
> > + * arm_register_pre_el_change_hook:
> >   * arm_register_el_change_hook:
> > - * Register a hook function which will be called back whenever this
> > - * CPU changes exception level or mode. The hook function will be
> > - * passed a pointer to the ARMCPU and the opaque data pointer passed
> > - * to this function when the hook was registered.
> > + * Register a hook function which will be called back before or after this CPU
> > + * changes exception level or mode. The hook function will be passed a pointer
> > + * to the ARMCPU and the opaque data pointer passed to this function when the
> > + * hook was registered.
> >   */
> 
> I would just have one doc comment for each function, rather than
> trying to share. (Some day we may actually autogenerate HTML docs
> from these comments...)
> 
> Do we make the guarantee that if we call the pre-change hook
> then we will definitely subsequently call the post-change hook?
> (ie does the PMU hook rely on that?)

Yes, the PMU relies on the pre- and post- hooks being called in pairs
since they drive the state machine of sorts that exists in the variables
holding the counter values/deltas. And unless I've really screwed up the
implementation, I believe the change hooks in my patchset make that
guarantee.

-Aaron

> 
> Otherwise looks OK.
> 
> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL change hooks

[Peter Maydell]

On 12 April 2018 at 18:01, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Apr 12 17:49, Peter Maydell wrote:
>> Do we make the guarantee that if we call the pre-change hook
>> then we will definitely subsequently call the post-change hook?
>> (ie does the PMU hook rely on that?)
>
> Yes, the PMU relies on the pre- and post- hooks being called in pairs
> since they drive the state machine of sorts that exists in the variables
> holding the counter values/deltas. And unless I've really screwed up the
> implementation, I believe the change hooks in my patchset make that
> guarantee.

Yes, I think I agree. We should document in the comment that
this is a guarantee that we make.

thanks
-- PMM

[Qemu-devel] [PATCH v3 10/22] target/arm: Allow EL change hooks to do IO

[Aaron Lindsay]

During code generation, surround CPSR writes and exception returns which
call the EL change hooks with gen_io_start/end. The immediate need is
for the PMU to access the clock and icount during EL change to support
mode filtering.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/translate-a64.c | 2 ++
 target/arm/translate.c     | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 31ff047..e1ae676 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1919,7 +1919,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
             unallocated_encoding(s);
             return;
         }
+        gen_io_start();
         gen_helper_exception_return(cpu_env);
+        gen_io_end();
         /* Must exit loop to check un-masked IRQs */
         s->base.is_jmp = DISAS_EXIT;
         return;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ba6ab7d..fd5871e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4536,7 +4536,9 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
      * appropriately depending on the new Thumb bit, so it must
      * be called after storing the new PC.
      */
+    gen_io_start();
     gen_helper_cpsr_write_eret(cpu_env, cpsr);
+    gen_io_end();
     tcg_temp_free_i32(cpsr);
     /* Must exit loop to check un-masked IRQs */
     s->base.is_jmp = DISAS_EXIT;
@@ -9828,7 +9830,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                 if (exc_return) {
                     /* Restore CPSR from SPSR.  */
                     tmp = load_cpu_field(spsr);
+                    gen_io_start();
                     gen_helper_cpsr_write_eret(cpu_env, tmp);
+                    gen_io_end();
                     tcg_temp_free_i32(tmp);
                     /* Must exit loop to check un-masked IRQs */
                     s->base.is_jmp = DISAS_EXIT;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 10/22] target/arm: Allow EL change hooks to do IO

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> During code generation, surround CPSR writes and exception returns which
> call the EL change hooks with gen_io_start/end. The immediate need is
> for the PMU to access the clock and icount during EL change to support
> mode filtering.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/translate-a64.c | 2 ++
>  target/arm/translate.c     | 4 ++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 31ff047..e1ae676 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1919,7 +1919,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
>              unallocated_encoding(s);
>              return;
>          }
> +        gen_io_start();
>          gen_helper_exception_return(cpu_env);
> +        gen_io_end();

You don't want to call gen_io_start() or gen_io_end() unless
tb_cflags(s->base.tb) & CF_USE_ICOUNT) is true.

(Ditto in the other cases below.)

>          /* Must exit loop to check un-masked IRQs */
>          s->base.is_jmp = DISAS_EXIT;
>          return;
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ba6ab7d..fd5871e 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -4536,7 +4536,9 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
>       * appropriately depending on the new Thumb bit, so it must
>       * be called after storing the new PC.
>       */
> +    gen_io_start();
>      gen_helper_cpsr_write_eret(cpu_env, cpsr);
> +    gen_io_end();
>      tcg_temp_free_i32(cpsr);
>      /* Must exit loop to check un-masked IRQs */
>      s->base.is_jmp = DISAS_EXIT;
> @@ -9828,7 +9830,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>                  if (exc_return) {
>                      /* Restore CPSR from SPSR.  */
>                      tmp = load_cpu_field(spsr);
> +                    gen_io_start();
>                      gen_helper_cpsr_write_eret(cpu_env, tmp);
> +                    gen_io_end();
>                      tcg_temp_free_i32(tmp);
>                      /* Must exit loop to check un-masked IRQs */
>                      s->base.is_jmp = DISAS_EXIT;
> --
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 10/22] target/arm: Allow EL change hooks to do IO

[Aaron Lindsay]

On Apr 12 17:53, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > During code generation, surround CPSR writes and exception returns which
> > call the EL change hooks with gen_io_start/end. The immediate need is
> > for the PMU to access the clock and icount during EL change to support
> > mode filtering.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/translate-a64.c | 2 ++
> >  target/arm/translate.c     | 4 ++++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> > index 31ff047..e1ae676 100644
> > --- a/target/arm/translate-a64.c
> > +++ b/target/arm/translate-a64.c
> > @@ -1919,7 +1919,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
> >              unallocated_encoding(s);
> >              return;
> >          }
> > +        gen_io_start();
> >          gen_helper_exception_return(cpu_env);
> > +        gen_io_end();
> 
> You don't want to call gen_io_start() or gen_io_end() unless
> tb_cflags(s->base.tb) & CF_USE_ICOUNT) is true.
> 
> (Ditto in the other cases below.)

I assume there's nothing tricky about this and updating this as follows
is sufficient?

> > +        if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
> > +            gen_io_start();
> > +        }
> >          gen_helper_exception_return(cpu_env);
> > +        if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
> > +            gen_io_end();
> > +        }

-Aaron

> 
> >          /* Must exit loop to check un-masked IRQs */
> >          s->base.is_jmp = DISAS_EXIT;
> >          return;
> > diff --git a/target/arm/translate.c b/target/arm/translate.c
> > index ba6ab7d..fd5871e 100644
> > --- a/target/arm/translate.c
> > +++ b/target/arm/translate.c
> > @@ -4536,7 +4536,9 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
> >       * appropriately depending on the new Thumb bit, so it must
> >       * be called after storing the new PC.
> >       */
> > +    gen_io_start();
> >      gen_helper_cpsr_write_eret(cpu_env, cpsr);
> > +    gen_io_end();
> >      tcg_temp_free_i32(cpsr);
> >      /* Must exit loop to check un-masked IRQs */
> >      s->base.is_jmp = DISAS_EXIT;
> > @@ -9828,7 +9830,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
> >                  if (exc_return) {
> >                      /* Restore CPSR from SPSR.  */
> >                      tmp = load_cpu_field(spsr);
> > +                    gen_io_start();
> >                      gen_helper_cpsr_write_eret(cpu_env, tmp);
> > +                    gen_io_end();
> >                      tcg_temp_free_i32(tmp);
> >                      /* Must exit loop to check un-masked IRQs */
> >                      s->base.is_jmp = DISAS_EXIT;
> > --
> > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
> > Qualcomm Technologies, Inc. is a member of the
> > Code Aurora Forum, a Linux Foundation Collaborative Project.
> >
> 
> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 10/22] target/arm: Allow EL change hooks to do IO

[Peter Maydell]

On 12 April 2018 at 18:08, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Apr 12 17:53, Peter Maydell wrote:
>> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
>> > During code generation, surround CPSR writes and exception returns which
>> > call the EL change hooks with gen_io_start/end. The immediate need is
>> > for the PMU to access the clock and icount during EL change to support
>> > mode filtering.
>> >
>> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>> > ---
>> >  target/arm/translate-a64.c | 2 ++
>> >  target/arm/translate.c     | 4 ++++
>> >  2 files changed, 6 insertions(+)
>> >
>> > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> > index 31ff047..e1ae676 100644
>> > --- a/target/arm/translate-a64.c
>> > +++ b/target/arm/translate-a64.c
>> > @@ -1919,7 +1919,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
>> >              unallocated_encoding(s);
>> >              return;
>> >          }
>> > +        gen_io_start();
>> >          gen_helper_exception_return(cpu_env);
>> > +        gen_io_end();
>>
>> You don't want to call gen_io_start() or gen_io_end() unless
>> tb_cflags(s->base.tb) & CF_USE_ICOUNT) is true.
>>
>> (Ditto in the other cases below.)
>
> I assume there's nothing tricky about this and updating this as follows
> is sufficient?

Yes, that's sufficient. (The other thing that needs to happen
for a gen_io_start/end is that the insn has to end the TB --
but in all these cases that's already true as they set
s->base.is_jmp = DISAS_EXIT.)

thanks
-- PMM

[Qemu-devel] [PATCH v3 11/22] target/arm: Fix bitmask for PMCCFILTR writes

[Aaron Lindsay]

It was shifted to the left one bit too few.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 50eaed7..0102357 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1123,7 +1123,7 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
     uint64_t saved_cycles = pmccntr_op_start(env);
-    env->cp15.pmccfiltr_el0 = value & 0x7E000000;
+    env->cp15.pmccfiltr_el0 = value & 0xfc000000;
     pmccntr_op_finish(env, saved_cycles);
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 11/22] target/arm: Fix bitmask for PMCCFILTR writes

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> It was shifted to the left one bit too few.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 50eaed7..0102357 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1123,7 +1123,7 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
>      uint64_t saved_cycles = pmccntr_op_start(env);
> -    env->cp15.pmccfiltr_el0 = value & 0x7E000000;
> +    env->cp15.pmccfiltr_el0 = value & 0xfc000000;
>      pmccntr_op_finish(env, saved_cycles);
>  }
>

I wonder why we got that one wrong.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Strictly speaking, bit 26 (M) should be visible only in
the AArch64 view of the register, not the AArch32 one,
but that's a separate issue.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 11/22] target/arm: Fix bitmask for PMCCFILTR writes

[Aaron Lindsay]

On Apr 12 17:41, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > It was shifted to the left one bit too few.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 50eaed7..0102357 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1123,7 +1123,7 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >                              uint64_t value)
> >  {
> >      uint64_t saved_cycles = pmccntr_op_start(env);
> > -    env->cp15.pmccfiltr_el0 = value & 0x7E000000;
> > +    env->cp15.pmccfiltr_el0 = value & 0xfc000000;
> >      pmccntr_op_finish(env, saved_cycles);
> >  }
> >
> 
> I wonder why we got that one wrong.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Strictly speaking, bit 26 (M) should be visible only in
> the AArch64 view of the register, not the AArch32 one,
> but that's a separate issue.

Right. I addressed this when I added AArch32 access for PMCCFILTR:
	[PATCH v3 13/22] target/arm: Allow AArch32 access for PMCCFILTR
	https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04910.html

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

The pmu_counter_filtered and pmu_op_start/finish functions are generic
(as opposed to PMCCNTR-specific) to allow for the implementation of
other events.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c    |  3 ++
 target/arm/cpu.h    | 37 +++++++++++++++++++----
 target/arm/helper.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 116 insertions(+), 11 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a2cb21e..b0d032c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -887,6 +887,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     if (!cpu->has_pmu) {
         unset_feature(env, ARM_FEATURE_PMU);
         cpu->id_aa64dfr0 &= ~0xf00;
+    } else {
+        arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
+        arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
     }
 
     if (!arm_feature(env, ARM_FEATURE_EL2)) {
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b0ef727..9c3b5ef 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -458,6 +458,11 @@ typedef struct CPUARMState {
          * was reset. Otherwise it stores the counter value
          */
         uint64_t c15_ccnt;
+        /* ccnt_cached_cycles is used to hold the last cycle count when
+         * c15_ccnt holds the guest-visible count instead of the delta during
+         * PMU operations which require this.
+         */
+        uint64_t ccnt_cached_cycles;
         uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
         uint64_t vpidr_el2; /* Virtualization Processor ID Register */
         uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
@@ -896,15 +901,35 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
                            void *puc);
 
 /**
- * pmccntr_sync
+ * pmccntr_op_start/finish
  * @env: CPUARMState
  *
- * Synchronises the counter in the PMCCNTR. This must always be called twice,
- * once before any action that might affect the timer and again afterwards.
- * The function is used to swap the state of the register if required.
- * This only happens when not in user mode (!CONFIG_USER_ONLY)
+ * Convert the counter in the PMCCNTR between its delta form (the typical mode
+ * when it's enabled) and the guest-visible value. These two calls must always
+ * surround any action which might affect the counter, and the return value
+ * from pmccntr_op_start must be supplied as the second argument to
+ * pmccntr_op_finish.
+ */
+uint64_t pmccntr_op_start(CPUARMState *env);
+void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles);
+
+/**
+ * pmu_op_start/finish
+ * @env: CPUARMState
+ *
+ * Convert all PMU counters between their delta form (the typical mode when
+ * they are enabled) and the guest-visible values. These two calls must
+ * surround any action which might affect the counters, and the return value
+ * from pmu_op_start must be supplied as the second argument to pmu_op_finish.
+ */
+uint64_t pmu_op_start(CPUARMState *env);
+void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles);
+
+/**
+ * Functions to register as EL change hooks for PMU mode filtering
  */
-void pmccntr_sync(CPUARMState *env);
+void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
+void pmu_post_el_change(ARMCPU *cpu, void *ignored);
 
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0102357..95b09d6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -908,6 +908,15 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRC   0x4
 #define PMCRE   0x1
 
+#define PMXEVTYPER_P          0x80000000
+#define PMXEVTYPER_U          0x40000000
+#define PMXEVTYPER_NSK        0x20000000
+#define PMXEVTYPER_NSU        0x10000000
+#define PMXEVTYPER_NSH        0x08000000
+#define PMXEVTYPER_M          0x04000000
+#define PMXEVTYPER_MT         0x02000000
+#define PMXEVTYPER_EVTCOUNT   0x000003ff
+
 #define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
@@ -998,7 +1007,7 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
 
 static inline bool arm_ccnt_enabled(CPUARMState *env)
 {
-    /* This does not support checking PMCCFILTR_EL0 register */
+    /* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered */
 
     if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
         return false;
@@ -1006,6 +1015,44 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 
     return true;
 }
+
+/* Returns true if the counter corresponding to the passed-in pmevtyper or
+ * pmccfiltr value is filtered using the current state */
+static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
+{
+    bool secure = arm_is_secure(env);
+    int el = arm_current_el(env);
+
+    bool P   = pmxevtyper & PMXEVTYPER_P;
+    bool U   = pmxevtyper & PMXEVTYPER_U;
+    bool NSK = pmxevtyper & PMXEVTYPER_NSK;
+    bool NSU = pmxevtyper & PMXEVTYPER_NSU;
+    bool NSH = pmxevtyper & PMXEVTYPER_NSH;
+    bool M   = pmxevtyper & PMXEVTYPER_M;
+
+    if (el == 1 && P) {
+        return true;
+    } else if (el == 0 && U) {
+        return true;
+    }
+
+    if (arm_feature(env, ARM_FEATURE_EL3)) {
+        if (el == 1 && !secure && NSK != P) {
+            return true;
+        } else if (el == 0 && !secure && NSU != U) {
+            return true;
+        } else if (el == 3 && secure && M != P) {
+            return true;
+        }
+    }
+
+    if (arm_feature(env, ARM_FEATURE_EL2) && el == 2 && !secure && !NSH) {
+        return true;
+    }
+
+    return false;
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,
@@ -1023,7 +1070,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
 #endif
 
-    if (arm_ccnt_enabled(env)) {
+    if (arm_ccnt_enabled(env) &&
+          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
 
         uint64_t eff_cycles = cycles;
         if (env->cp15.c9_pmcr & PMCRD) {
@@ -1043,7 +1091,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
  */
 void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
-    if (arm_ccnt_enabled(env)) {
+    if (arm_ccnt_enabled(env) &&
+          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
 
         if (env->cp15.c9_pmcr & PMCRD) {
             /* Increment once every 64 processor clock cycles */
@@ -1054,10 +1103,30 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
     }
 }
 
+uint64_t pmu_op_start(CPUARMState *env)
+{
+    return pmccntr_op_start(env);
+}
+
+void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
+{
+    pmccntr_op_finish(env, prev_cycles);
+}
+
+void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
+{
+    cpu->env.cp15.ccnt_cached_cycles = pmu_op_start(&cpu->env);
+}
+
+void pmu_post_el_change(ARMCPU *cpu, void *ignored)
+{
+    pmu_op_finish(&cpu->env, cpu->env.cp15.ccnt_cached_cycles);
+}
+
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    uint64_t saved_cycles = pmccntr_op_start(env);
+    uint64_t saved_cycles = pmu_op_start(env);
 
     if (value & PMCRC) {
         /* The counter has been reset */
@@ -1068,7 +1137,7 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
 
-    pmccntr_op_finish(env, saved_cycles);
+    pmu_op_finish(env, saved_cycles);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
@@ -1117,6 +1186,14 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
 }
 
+uint64_t pmu_op_start(CPUARMState *env)
+{
+}
+
+void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
+{
+}
+
 #endif
 
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> The pmu_counter_filtered and pmu_op_start/finish functions are generic
> (as opposed to PMCCNTR-specific) to allow for the implementation of
> other events.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.c    |  3 ++
>  target/arm/cpu.h    | 37 +++++++++++++++++++----
>  target/arm/helper.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 116 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a2cb21e..b0d032c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -887,6 +887,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (!cpu->has_pmu) {
>          unset_feature(env, ARM_FEATURE_PMU);
>          cpu->id_aa64dfr0 &= ~0xf00;
> +    } else {
> +        arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
> +        arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);

You probably don't want to do this if we're using KVM, as there
are some code paths where we call do_interrupt() when using KVM
which will trigger the hooks and likely do unexpected things.
(For instance kvm_arm_handle_debug() does this to set the guest
up to take debug exceptions.)

>      }
>
>      if (!arm_feature(env, ARM_FEATURE_EL2)) {
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index b0ef727..9c3b5ef 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -458,6 +458,11 @@ typedef struct CPUARMState {
>           * was reset. Otherwise it stores the counter value
>           */
>          uint64_t c15_ccnt;
> +        /* ccnt_cached_cycles is used to hold the last cycle count when
> +         * c15_ccnt holds the guest-visible count instead of the delta during
> +         * PMU operations which require this.
> +         */
> +        uint64_t ccnt_cached_cycles;

Can this ever hold valid state at a point when we need to do VM
migration, or is it purely temporary ?

>          uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
>          uint64_t vpidr_el2; /* Virtualization Processor ID Register */
>          uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
> @@ -896,15 +901,35 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
>                             void *puc);
>
>  /**
> - * pmccntr_sync
> + * pmccntr_op_start/finish

Shouldn't this doc change and prototype change have gone with the earlier
patch that changed the implementation?

>   * @env: CPUARMState
>   *
> - * Synchronises the counter in the PMCCNTR. This must always be called twice,
> - * once before any action that might affect the timer and again afterwards.
> - * The function is used to swap the state of the register if required.
> - * This only happens when not in user mode (!CONFIG_USER_ONLY)
> + * Convert the counter in the PMCCNTR between its delta form (the typical mode
> + * when it's enabled) and the guest-visible value. These two calls must always
> + * surround any action which might affect the counter, and the return value
> + * from pmccntr_op_start must be supplied as the second argument to
> + * pmccntr_op_finish.
> + */
> +uint64_t pmccntr_op_start(CPUARMState *env);
> +void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles);
> +
> +/**
> + * pmu_op_start/finish
> + * @env: CPUARMState
> + *
> + * Convert all PMU counters between their delta form (the typical mode when
> + * they are enabled) and the guest-visible values. These two calls must
> + * surround any action which might affect the counters, and the return value
> + * from pmu_op_start must be supplied as the second argument to pmu_op_finish.
> + */
> +uint64_t pmu_op_start(CPUARMState *env);
> +void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles);
> +
> +/**
> + * Functions to register as EL change hooks for PMU mode filtering
>   */
> -void pmccntr_sync(CPUARMState *env);
> +void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
> +void pmu_post_el_change(ARMCPU *cpu, void *ignored);
>
>  /* SCTLR bit meanings. Several bits have been reused in newer
>   * versions of the architecture; in that case we define constants
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0102357..95b09d6 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -908,6 +908,15 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>  #define PMCRC   0x4
>  #define PMCRE   0x1
>
> +#define PMXEVTYPER_P          0x80000000
> +#define PMXEVTYPER_U          0x40000000
> +#define PMXEVTYPER_NSK        0x20000000
> +#define PMXEVTYPER_NSU        0x10000000
> +#define PMXEVTYPER_NSH        0x08000000
> +#define PMXEVTYPER_M          0x04000000
> +#define PMXEVTYPER_MT         0x02000000
> +#define PMXEVTYPER_EVTCOUNT   0x000003ff
> +
>  #define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
>  /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
>  #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
> @@ -998,7 +1007,7 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
>
>  static inline bool arm_ccnt_enabled(CPUARMState *env)
>  {
> -    /* This does not support checking PMCCFILTR_EL0 register */
> +    /* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered */
>
>      if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
>          return false;
> @@ -1006,6 +1015,44 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
>
>      return true;
>  }
> +
> +/* Returns true if the counter corresponding to the passed-in pmevtyper or
> + * pmccfiltr value is filtered using the current state */

"is filtered (ie does not count events)" would make it clearer.

Nit: */ should be on a line of its own.

> +static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
> +{
> +    bool secure = arm_is_secure(env);
> +    int el = arm_current_el(env);
> +
> +    bool P   = pmxevtyper & PMXEVTYPER_P;
> +    bool U   = pmxevtyper & PMXEVTYPER_U;
> +    bool NSK = pmxevtyper & PMXEVTYPER_NSK;
> +    bool NSU = pmxevtyper & PMXEVTYPER_NSU;
> +    bool NSH = pmxevtyper & PMXEVTYPER_NSH;
> +    bool M   = pmxevtyper & PMXEVTYPER_M;

Lowercase for variable names, please.

> +
> +    if (el == 1 && P) {
> +        return true;
> +    } else if (el == 0 && U) {
> +        return true;
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> +        if (el == 1 && !secure && NSK != P) {
> +            return true;
> +        } else if (el == 0 && !secure && NSU != U) {
> +            return true;
> +        } else if (el == 3 && secure && M != P) {
> +            return true;
> +        }
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL2) && el == 2 && !secure && !NSH) {
> +        return true;
> +    }

This doesn't follow the structure that the Arm ARM pseudocode
uses, which makes it a bit tricky to review. (see AArch64.CountEvents()).
It also doesn't implement the same logic -- for instance this
code will return true if we're in NS EL1 and the U bit is set,
whereas the pseudocode returns true for NS EL1 only if U != NSU.

> +    return false;
> +}
> +
>  /*
>   * Ensure c15_ccnt is the guest-visible count so that operations such as
>   * enabling/disabling the counter or filtering, modifying the count itself,
> @@ -1023,7 +1070,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
>                            ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
>  #endif
>
> -    if (arm_ccnt_enabled(env)) {
> +    if (arm_ccnt_enabled(env) &&
> +          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
>
>          uint64_t eff_cycles = cycles;
>          if (env->cp15.c9_pmcr & PMCRD) {
> @@ -1043,7 +1091,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
>   */
>  void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
>  {
> -    if (arm_ccnt_enabled(env)) {
> +    if (arm_ccnt_enabled(env) &&
> +          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
>          if (env->cp15.c9_pmcr & PMCRD) {
>              /* Increment once every 64 processor clock cycles */
> @@ -1054,10 +1103,30 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
>      }
>  }
>
> +uint64_t pmu_op_start(CPUARMState *env)
> +{
> +    return pmccntr_op_start(env);
> +}

Do these two functions end up being different at the end
of the patchset?

> +
> +void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
> +{
> +    pmccntr_op_finish(env, prev_cycles);
> +}
> +
> +void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
> +{
> +    cpu->env.cp15.ccnt_cached_cycles = pmu_op_start(&cpu->env);
> +}
> +
> +void pmu_post_el_change(ARMCPU *cpu, void *ignored)
> +{
> +    pmu_op_finish(&cpu->env, cpu->env.cp15.ccnt_cached_cycles);
> +}
> +
>  static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t value)
>  {
> -    uint64_t saved_cycles = pmccntr_op_start(env);
> +    uint64_t saved_cycles = pmu_op_start(env);
>
>      if (value & PMCRC) {
>          /* The counter has been reset */
> @@ -1068,7 +1137,7 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->cp15.c9_pmcr &= ~0x39;
>      env->cp15.c9_pmcr |= (value & 0x39);
>
> -    pmccntr_op_finish(env, saved_cycles);
> +    pmu_op_finish(env, saved_cycles);
>  }
>
>  static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> @@ -1117,6 +1186,14 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
>  {
>  }
>
> +uint64_t pmu_op_start(CPUARMState *env)
> +{
> +}
> +
> +void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
> +{
> +}
> +
>  #endif
>
>  static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Apr 12 18:15, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > The pmu_counter_filtered and pmu_op_start/finish functions are generic
> > (as opposed to PMCCNTR-specific) to allow for the implementation of
> > other events.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.c    |  3 ++
> >  target/arm/cpu.h    | 37 +++++++++++++++++++----
> >  target/arm/helper.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 116 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index a2cb21e..b0d032c 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -887,6 +887,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >      if (!cpu->has_pmu) {
> >          unset_feature(env, ARM_FEATURE_PMU);
> >          cpu->id_aa64dfr0 &= ~0xf00;
> > +    } else {
> > +        arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
> > +        arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
> 
> You probably don't want to do this if we're using KVM, as there
> are some code paths where we call do_interrupt() when using KVM
> which will trigger the hooks and likely do unexpected things.
> (For instance kvm_arm_handle_debug() does this to set the guest
> up to take debug exceptions.)

Okay, I'll surround this with `if (!kvm_enabled())` for the next pass.
> 
> >      }
> >
> >      if (!arm_feature(env, ARM_FEATURE_EL2)) {
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index b0ef727..9c3b5ef 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -458,6 +458,11 @@ typedef struct CPUARMState {
> >           * was reset. Otherwise it stores the counter value
> >           */
> >          uint64_t c15_ccnt;
> > +        /* ccnt_cached_cycles is used to hold the last cycle count when
> > +         * c15_ccnt holds the guest-visible count instead of the delta during
> > +         * PMU operations which require this.
> > +         */
> > +        uint64_t ccnt_cached_cycles;
> 
> Can this ever hold valid state at a point when we need to do VM
> migration, or is it purely temporary ?

I believe that as of this version of the patch it is temporary and will
not need to be migrated. However, I believe it's going to be necessary
to have two variables to represent the state of each counter in order to
implement interrupt on overflow. 

> 
> >          uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
> >          uint64_t vpidr_el2; /* Virtualization Processor ID Register */
> >          uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
> > @@ -896,15 +901,35 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
> >                             void *puc);
> >
> >  /**
> > - * pmccntr_sync
> > + * pmccntr_op_start/finish
> 
> Shouldn't this doc change and prototype change have gone with the earlier
> patch that changed the implementation?

Hmmm, yes. Seems like I got pmccntr_op_start and pmu_op_start confused
when staging this.

> >   * @env: CPUARMState
> >   *
> > - * Synchronises the counter in the PMCCNTR. This must always be called twice,
> > - * once before any action that might affect the timer and again afterwards.
> > - * The function is used to swap the state of the register if required.
> > - * This only happens when not in user mode (!CONFIG_USER_ONLY)
> > + * Convert the counter in the PMCCNTR between its delta form (the typical mode
> > + * when it's enabled) and the guest-visible value. These two calls must always
> > + * surround any action which might affect the counter, and the return value
> > + * from pmccntr_op_start must be supplied as the second argument to
> > + * pmccntr_op_finish.
> > + */
> > +uint64_t pmccntr_op_start(CPUARMState *env);
> > +void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles);
> > +
> > +/**
> > + * pmu_op_start/finish
> > + * @env: CPUARMState
> > + *
> > + * Convert all PMU counters between their delta form (the typical mode when
> > + * they are enabled) and the guest-visible values. These two calls must
> > + * surround any action which might affect the counters, and the return value
> > + * from pmu_op_start must be supplied as the second argument to pmu_op_finish.
> > + */
> > +uint64_t pmu_op_start(CPUARMState *env);
> > +void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles);
> > +
> > +/**
> > + * Functions to register as EL change hooks for PMU mode filtering
> >   */
> > -void pmccntr_sync(CPUARMState *env);
> > +void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
> > +void pmu_post_el_change(ARMCPU *cpu, void *ignored);
> >
> >  /* SCTLR bit meanings. Several bits have been reused in newer
> >   * versions of the architecture; in that case we define constants
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 0102357..95b09d6 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -908,6 +908,15 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
> >  #define PMCRC   0x4
> >  #define PMCRE   0x1
> >
> > +#define PMXEVTYPER_P          0x80000000
> > +#define PMXEVTYPER_U          0x40000000
> > +#define PMXEVTYPER_NSK        0x20000000
> > +#define PMXEVTYPER_NSU        0x10000000
> > +#define PMXEVTYPER_NSH        0x08000000
> > +#define PMXEVTYPER_M          0x04000000
> > +#define PMXEVTYPER_MT         0x02000000
> > +#define PMXEVTYPER_EVTCOUNT   0x000003ff
> > +
> >  #define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
> >  /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
> >  #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
> > @@ -998,7 +1007,7 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
> >
> >  static inline bool arm_ccnt_enabled(CPUARMState *env)
> >  {
> > -    /* This does not support checking PMCCFILTR_EL0 register */
> > +    /* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered */
> >
> >      if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
> >          return false;
> > @@ -1006,6 +1015,44 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
> >
> >      return true;
> >  }
> > +
> > +/* Returns true if the counter corresponding to the passed-in pmevtyper or
> > + * pmccfiltr value is filtered using the current state */
> 
> "is filtered (ie does not count events)" would make it clearer.
> 
> Nit: */ should be on a line of its own.

Will do.

> 
> > +static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
> > +{
> > +    bool secure = arm_is_secure(env);
> > +    int el = arm_current_el(env);
> > +
> > +    bool P   = pmxevtyper & PMXEVTYPER_P;
> > +    bool U   = pmxevtyper & PMXEVTYPER_U;
> > +    bool NSK = pmxevtyper & PMXEVTYPER_NSK;
> > +    bool NSU = pmxevtyper & PMXEVTYPER_NSU;
> > +    bool NSH = pmxevtyper & PMXEVTYPER_NSH;
> > +    bool M   = pmxevtyper & PMXEVTYPER_M;
> 
> Lowercase for variable names, please.

Will do.

> > +
> > +    if (el == 1 && P) {
> > +        return true;
> > +    } else if (el == 0 && U) {
> > +        return true;
> > +    }
> > +
> > +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> > +        if (el == 1 && !secure && NSK != P) {
> > +            return true;
> > +        } else if (el == 0 && !secure && NSU != U) {
> > +            return true;
> > +        } else if (el == 3 && secure && M != P) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    if (arm_feature(env, ARM_FEATURE_EL2) && el == 2 && !secure && !NSH) {
> > +        return true;
> > +    }
> 
> This doesn't follow the structure that the Arm ARM pseudocode
> uses, which makes it a bit tricky to review. (see AArch64.CountEvents()).
> It also doesn't implement the same logic -- for instance this
> code will return true if we're in NS EL1 and the U bit is set,
> whereas the pseudocode returns true for NS EL1 only if U != NSU.

Okay, I'll take another look at this logic.

> > +    return false;
> > +}
> > +
> >  /*
> >   * Ensure c15_ccnt is the guest-visible count so that operations such as
> >   * enabling/disabling the counter or filtering, modifying the count itself,
> > @@ -1023,7 +1070,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
> >                            ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> >  #endif
> >
> > -    if (arm_ccnt_enabled(env)) {
> > +    if (arm_ccnt_enabled(env) &&
> > +          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
> >
> >          uint64_t eff_cycles = cycles;
> >          if (env->cp15.c9_pmcr & PMCRD) {
> > @@ -1043,7 +1091,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
> >   */
> >  void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
> >  {
> > -    if (arm_ccnt_enabled(env)) {
> > +    if (arm_ccnt_enabled(env) &&
> > +          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
> >          if (env->cp15.c9_pmcr & PMCRD) {
> >              /* Increment once every 64 processor clock cycles */
> > @@ -1054,10 +1103,30 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
> >      }
> >  }
> >
> > +uint64_t pmu_op_start(CPUARMState *env)
> > +{
> > +    return pmccntr_op_start(env);
> > +}
> 
> Do these two functions end up being different at the end
> of the patchset?

Yes.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Apr 12 13:36, Aaron Lindsay wrote:
> On Apr 12 18:15, Peter Maydell wrote:
> > On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > index b0ef727..9c3b5ef 100644
> > > --- a/target/arm/cpu.h
> > > +++ b/target/arm/cpu.h
> > > @@ -458,6 +458,11 @@ typedef struct CPUARMState {
> > >           * was reset. Otherwise it stores the counter value
> > >           */
> > >          uint64_t c15_ccnt;
> > > +        /* ccnt_cached_cycles is used to hold the last cycle count when
> > > +         * c15_ccnt holds the guest-visible count instead of the delta during
> > > +         * PMU operations which require this.
> > > +         */
> > > +        uint64_t ccnt_cached_cycles;
> > 
> > Can this ever hold valid state at a point when we need to do VM
> > migration, or is it purely temporary ?
> 
> I believe that as of this version of the patch it is temporary and will
> not need to be migrated. However, I believe it's going to be necessary
> to have two variables to represent the state of each counter in order to
> implement interrupt on overflow. 

Coming back around to this, I don't see a way around using two variables
to hold PMCCNTR's full state to make interrupt on overflow work. I
haven't been able to find other examples or documentation covering state
needing to be updated in more than one location for a given CP register
- do you know of any I've missed or have recommendations about how to
approach this?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Peter Maydell]

On 17 April 2018 at 16:21, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Apr 12 13:36, Aaron Lindsay wrote:
>> On Apr 12 18:15, Peter Maydell wrote:
>> > On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
>> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> > > index b0ef727..9c3b5ef 100644
>> > > --- a/target/arm/cpu.h
>> > > +++ b/target/arm/cpu.h
>> > > @@ -458,6 +458,11 @@ typedef struct CPUARMState {
>> > >           * was reset. Otherwise it stores the counter value
>> > >           */
>> > >          uint64_t c15_ccnt;
>> > > +        /* ccnt_cached_cycles is used to hold the last cycle count when
>> > > +         * c15_ccnt holds the guest-visible count instead of the delta during
>> > > +         * PMU operations which require this.
>> > > +         */
>> > > +        uint64_t ccnt_cached_cycles;
>> >
>> > Can this ever hold valid state at a point when we need to do VM
>> > migration, or is it purely temporary ?
>>
>> I believe that as of this version of the patch it is temporary and will
>> not need to be migrated. However, I believe it's going to be necessary
>> to have two variables to represent the state of each counter in order to
>> implement interrupt on overflow.
>
> Coming back around to this, I don't see a way around using two variables
> to hold PMCCNTR's full state to make interrupt on overflow work. I
> haven't been able to find other examples or documentation covering state
> needing to be updated in more than one location for a given CP register
> - do you know of any I've missed or have recommendations about how to
> approach this?

Can you explain the problem in more detail? In general it's a bit of
a red flag if you think you need more state storage space than the
hardware has, and I don't think there's any "hidden" state in the h/w here.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Apr 17 16:37, Peter Maydell wrote:
> On 17 April 2018 at 16:21, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > On Apr 12 13:36, Aaron Lindsay wrote:
> >> On Apr 12 18:15, Peter Maydell wrote:
> >> > On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> >> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >> > > index b0ef727..9c3b5ef 100644
> >> > > --- a/target/arm/cpu.h
> >> > > +++ b/target/arm/cpu.h
> >> > > @@ -458,6 +458,11 @@ typedef struct CPUARMState {
> >> > >           * was reset. Otherwise it stores the counter value
> >> > >           */
> >> > >          uint64_t c15_ccnt;
> >> > > +        /* ccnt_cached_cycles is used to hold the last cycle count when
> >> > > +         * c15_ccnt holds the guest-visible count instead of the delta during
> >> > > +         * PMU operations which require this.
> >> > > +         */
> >> > > +        uint64_t ccnt_cached_cycles;
> >> >
> >> > Can this ever hold valid state at a point when we need to do VM
> >> > migration, or is it purely temporary ?
> >>
> >> I believe that as of this version of the patch it is temporary and will
> >> not need to be migrated. However, I believe it's going to be necessary
> >> to have two variables to represent the state of each counter in order to
> >> implement interrupt on overflow.
> >
> > Coming back around to this, I don't see a way around using two variables
> > to hold PMCCNTR's full state to make interrupt on overflow work. I
> > haven't been able to find other examples or documentation covering state
> > needing to be updated in more than one location for a given CP register
> > - do you know of any I've missed or have recommendations about how to
> > approach this?
> 
> Can you explain the problem in more detail? In general it's a bit of
> a red flag if you think you need more state storage space than the
> hardware has, and I don't think there's any "hidden" state in the h/w here.

The critical difference between hardware and QEMU's PMU implementation
is that hardware detects overflow when the overflow actually happens,
which would be inefficient to do in software. Because QEMU stores a
delta from the 'real' count (i.e. the clock/icount) and only updates the
architectural counter values when necessary (when they're read/written),
checking for overflow is less straightforward than checking if
incrementing an individual counter by one flips the high-order bit from
1 to 0 as it happens. If the only information you have is the current
counter value, and don't know how many events have occurred since you
last checked or what the counter value was at that time, you can't tell
whether or not it overflowed.

I haven't come up with a way to correctly and reliably detect overflow
without storing additional information. I'll go ahead and post v4 with
my first-pass implementation of the overflow code and see if you see
something I'm missing or can think of a trick we can play to keep this
inside of one register value.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 13/22] target/arm: Allow AArch32 access for PMCCFILTR

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 95b09d6..d4f06e6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -917,6 +917,10 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMXEVTYPER_MT         0x02000000
 #define PMXEVTYPER_EVTCOUNT   0x000003ff
 
+#define PMCCFILTR             0xf8000000
+#define PMCCFILTR_M           PMXEVTYPER_M
+#define PMCCFILTR_EL0         (PMCCFILTR | PMCCFILTR_M)
+
 #define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
@@ -1200,10 +1204,26 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
     uint64_t saved_cycles = pmccntr_op_start(env);
-    env->cp15.pmccfiltr_el0 = value & 0xfc000000;
+    env->cp15.pmccfiltr_el0 = value & PMCCFILTR_EL0;
+    pmccntr_op_finish(env, saved_cycles);
+}
+
+static void pmccfiltr_write_a32(CPUARMState *env, const ARMCPRegInfo *ri,
+                            uint64_t value)
+{
+    uint64_t saved_cycles = pmccntr_op_start(env);
+    /* M is not accessible from AArch32 */
+    env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 & PMCCFILTR_M) |
+        (value & PMCCFILTR);
     pmccntr_op_finish(env, saved_cycles);
 }
 
+static uint64_t pmccfiltr_read_a32(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /* M is not visible in AArch32 */
+    return env->cp15.pmccfiltr_el0 & PMCCFILTR;
+}
+
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -1421,6 +1441,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .type = ARM_CP_IO,
       .readfn = pmccntr_read, .writefn = pmccntr_write, },
 #endif
+    { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7,
+      .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
+      .resetvalue = 0, },
     { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
       .writefn = pmccfiltr_write,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Aaron Lindsay]

This is a bug fix to ensure 64-bit reads of this register don't read
adjacent data.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9c3b5ef..fb2f983 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -367,7 +367,7 @@ typedef struct CPUARMState {
         uint32_t c9_data;
         uint64_t c9_pmcr; /* performance monitor control register */
         uint64_t c9_pmcnten; /* perf monitor counter enables */
-        uint32_t c9_pmovsr; /* perf monitor overflow status */
+        uint64_t c9_pmovsr; /* perf monitor overflow status */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint64_t c9_pmselr; /* perf monitor counter selection register */
         uint64_t c9_pminten; /* perf monitor interrupt enables */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Philippe Mathieu-Daudé]

Hi Aaron,

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> This is a bug fix to ensure 64-bit reads of this register don't read
> adjacent data.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9c3b5ef..fb2f983 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -367,7 +367,7 @@ typedef struct CPUARMState {
>          uint32_t c9_data;
>          uint64_t c9_pmcr; /* performance monitor control register */
>          uint64_t c9_pmcnten; /* perf monitor counter enables */
> -        uint32_t c9_pmovsr; /* perf monitor overflow status */
> +        uint64_t c9_pmovsr; /* perf monitor overflow status */

This doesn't look correct, since this reg is 32b.

I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:

    { .name = "PMOVSR", ...
-     ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+     ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
      .accessfn = pmreg_access,
      .writefn = pmovsr_write,
      .raw_writefn = raw_write },

>          uint32_t c9_pmuserenr; /* perf monitor user enable */
>          uint64_t c9_pmselr; /* perf monitor counter selection register */
>          uint64_t c9_pminten; /* perf monitor interrupt enables */
> 

Regards,

Phil.

Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Aaron Lindsay]

Phil,

On Mar 19 00:14, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > This is a bug fix to ensure 64-bit reads of this register don't read
> > adjacent data.
> > 
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9c3b5ef..fb2f983 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -367,7 +367,7 @@ typedef struct CPUARMState {
> >          uint32_t c9_data;
> >          uint64_t c9_pmcr; /* performance monitor control register */
> >          uint64_t c9_pmcnten; /* perf monitor counter enables */
> > -        uint32_t c9_pmovsr; /* perf monitor overflow status */
> > +        uint64_t c9_pmovsr; /* perf monitor overflow status */
> 
> This doesn't look correct, since this reg is 32b.
> 
> I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:
> 
>     { .name = "PMOVSR", ...
> -     ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> +     ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>       .accessfn = pmreg_access,
>       .writefn = pmovsr_write,
>       .raw_writefn = raw_write },

Nearly all of these PMU registers are 32 bits wide, but most of them are
implemented as 64-bit registers (PMCR, PMCNTEN*, PMSELR, PMINTEN* are a
few examples I see in this patch's context). My understanding is that
AArch64 register accesses are handled as 64 bits, even if the register
itself isn't that wide (though I haven't personally verified this). See
an earlier email from Peter from v2 of this patchset:

https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03983.html

Does this still look wrong to you? If so, I'll take a more thorough look
into how these accesses work.

> >          uint32_t c9_pmuserenr; /* perf monitor user enable */

Whatever we decide should likely be done to PMUSERENR too - I think I
overlooked this one before.

> >          uint64_t c9_pmselr; /* perf monitor counter selection register */
> >          uint64_t c9_pminten; /* perf monitor interrupt enables */
> > 
> 
> Regards,
> 
> Phil.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Peter Maydell]

On 19 March 2018 at 15:24, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Phil,
>
> On Mar 19 00:14, Philippe Mathieu-Daudé wrote:
>> Hi Aaron,
>>
>> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
>> > This is a bug fix to ensure 64-bit reads of this register don't read
>> > adjacent data.
>> >
>> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>> > ---
>> >  target/arm/cpu.h | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> > index 9c3b5ef..fb2f983 100644
>> > --- a/target/arm/cpu.h
>> > +++ b/target/arm/cpu.h
>> > @@ -367,7 +367,7 @@ typedef struct CPUARMState {
>> >          uint32_t c9_data;
>> >          uint64_t c9_pmcr; /* performance monitor control register */
>> >          uint64_t c9_pmcnten; /* perf monitor counter enables */
>> > -        uint32_t c9_pmovsr; /* perf monitor overflow status */
>> > +        uint64_t c9_pmovsr; /* perf monitor overflow status */
>>
>> This doesn't look correct, since this reg is 32b.
>>
>> I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:
>>
>>     { .name = "PMOVSR", ...
>> -     ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
>> +     ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>>       .accessfn = pmreg_access,
>>       .writefn = pmovsr_write,
>>       .raw_writefn = raw_write },
>
> Nearly all of these PMU registers are 32 bits wide, but most of them are
> implemented as 64-bit registers (PMCR, PMCNTEN*, PMSELR, PMINTEN* are a
> few examples I see in this patch's context). My understanding is that
> AArch64 register accesses are handled as 64 bits, even if the register
> itself isn't that wide (though I haven't personally verified this).

Correct. Technically there's no such thing as a 32-bit wide AArch64
system register -- that is just a shorthand in the Arm ARM for
"64-bit wide with the top 32-bits being RES0".

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Philippe Mathieu-Daudé]

On 03/19/2018 04:31 PM, Peter Maydell wrote:
> On 19 March 2018 at 15:24, Aaron Lindsay <alindsay@codeaurora.org> wrote:
>> Phil,
>>
>> On Mar 19 00:14, Philippe Mathieu-Daudé wrote:
>>> Hi Aaron,
>>>
>>> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
>>>> This is a bug fix to ensure 64-bit reads of this register don't read
>>>> adjacent data.
>>>>
>>>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>>>> ---
>>>>  target/arm/cpu.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>> index 9c3b5ef..fb2f983 100644
>>>> --- a/target/arm/cpu.h
>>>> +++ b/target/arm/cpu.h
>>>> @@ -367,7 +367,7 @@ typedef struct CPUARMState {
>>>>          uint32_t c9_data;
>>>>          uint64_t c9_pmcr; /* performance monitor control register */
>>>>          uint64_t c9_pmcnten; /* perf monitor counter enables */
>>>> -        uint32_t c9_pmovsr; /* perf monitor overflow status */
>>>> +        uint64_t c9_pmovsr; /* perf monitor overflow status */
>>>
>>> This doesn't look correct, since this reg is 32b.
>>>
>>> I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:
>>>
>>>     { .name = "PMOVSR", ...
>>> -     ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
>>> +     ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>>>       .accessfn = pmreg_access,
>>>       .writefn = pmovsr_write,
>>>       .raw_writefn = raw_write },
>>
>> Nearly all of these PMU registers are 32 bits wide, but most of them are
>> implemented as 64-bit registers (PMCR, PMCNTEN*, PMSELR, PMINTEN* are a
>> few examples I see in this patch's context). My understanding is that
>> AArch64 register accesses are handled as 64 bits, even if the register
>> itself isn't that wide (though I haven't personally verified this).
> 
> Correct. Technically there's no such thing as a 32-bit wide AArch64
> system register -- that is just a shorthand in the Arm ARM for
> "64-bit wide with the top 32-bits being RES0".

Ok, good to know. Thanks both for your explanation :)

Phil.

[Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c | 3 +++
 target/arm/cpu.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b0d032c..e544f1d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     /* Some features automatically imply others: */
     if (arm_feature(env, ARM_FEATURE_V8)) {
         set_feature(env, ARM_FEATURE_V7);
+        set_feature(env, ARM_FEATURE_V7VE);
         set_feature(env, ARM_FEATURE_ARM_DIV);
         set_feature(env, ARM_FEATURE_LPAE);
     }
@@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj)
 
     cpu->dtb_compatible = "arm,cortex-a7";
     set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
@@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
 
     cpu->dtb_compatible = "arm,cortex-a15";
     set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index fb2f983..cc1e2fb 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1439,6 +1439,7 @@ enum arm_features {
     ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
     ARM_FEATURE_THUMB2EE,
     ARM_FEATURE_V7MP,    /* v7 Multiprocessing Extensions */
+    ARM_FEATURE_V7VE,    /* v7 with Virtualization Extensions */
     ARM_FEATURE_V4T,
     ARM_FEATURE_V5,
     ARM_FEATURE_STRONGARM,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.c | 3 +++
>  target/arm/cpu.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b0d032c..e544f1d 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          set_feature(env, ARM_FEATURE_V7);
> +        set_feature(env, ARM_FEATURE_V7VE);
>          set_feature(env, ARM_FEATURE_ARM_DIV);
>          set_feature(env, ARM_FEATURE_LPAE);
>      }
> @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj)
>  
>      cpu->dtb_compatible = "arm,cortex-a7";
>      set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_V7VE);
>      set_feature(&cpu->env, ARM_FEATURE_VFP4);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
>      set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
> @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
>  
>      cpu->dtb_compatible = "arm,cortex-a15";
>      set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_V7VE);
>      set_feature(&cpu->env, ARM_FEATURE_VFP4);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
>      set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index fb2f983..cc1e2fb 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1439,6 +1439,7 @@ enum arm_features {
>      ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
>      ARM_FEATURE_THUMB2EE,
>      ARM_FEATURE_V7MP,    /* v7 Multiprocessing Extensions */
> +    ARM_FEATURE_V7VE,    /* v7 with Virtualization Extensions */
>      ARM_FEATURE_V4T,
>      ARM_FEATURE_V5,
>      ARM_FEATURE_STRONGARM,
> 

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.c | 3 +++
>  target/arm/cpu.h | 1 +
>  2 files changed, 4 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b0d032c..e544f1d 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      /* Some features automatically imply others: */
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          set_feature(env, ARM_FEATURE_V7);
> +        set_feature(env, ARM_FEATURE_V7VE);
>          set_feature(env, ARM_FEATURE_ARM_DIV);
>          set_feature(env, ARM_FEATURE_LPAE);
>      }
> @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj)
>
>      cpu->dtb_compatible = "arm,cortex-a7";
>      set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_V7VE);
>      set_feature(&cpu->env, ARM_FEATURE_VFP4);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
>      set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
> @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
>
>      cpu->dtb_compatible = "arm,cortex-a15";
>      set_feature(&cpu->env, ARM_FEATURE_V7);
> +    set_feature(&cpu->env, ARM_FEATURE_V7VE);
>      set_feature(&cpu->env, ARM_FEATURE_VFP4);
>      set_feature(&cpu->env, ARM_FEATURE_NEON);
>      set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index fb2f983..cc1e2fb 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1439,6 +1439,7 @@ enum arm_features {
>      ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
>      ARM_FEATURE_THUMB2EE,
>      ARM_FEATURE_V7MP,    /* v7 Multiprocessing Extensions */
> +    ARM_FEATURE_V7VE,    /* v7 with Virtualization Extensions */
>      ARM_FEATURE_V4T,
>      ARM_FEATURE_V5,
>      ARM_FEATURE_STRONGARM,

What's the difference between this and ARM_FEATURE_EL2 ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On Apr 12 18:17, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.c | 3 +++
> >  target/arm/cpu.h | 1 +
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index b0d032c..e544f1d 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >      /* Some features automatically imply others: */
> >      if (arm_feature(env, ARM_FEATURE_V8)) {
> >          set_feature(env, ARM_FEATURE_V7);
> > +        set_feature(env, ARM_FEATURE_V7VE);
> >          set_feature(env, ARM_FEATURE_ARM_DIV);
> >          set_feature(env, ARM_FEATURE_LPAE);
> >      }
> > @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj)
> >
> >      cpu->dtb_compatible = "arm,cortex-a7";
> >      set_feature(&cpu->env, ARM_FEATURE_V7);
> > +    set_feature(&cpu->env, ARM_FEATURE_V7VE);
> >      set_feature(&cpu->env, ARM_FEATURE_VFP4);
> >      set_feature(&cpu->env, ARM_FEATURE_NEON);
> >      set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
> > @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
> >
> >      cpu->dtb_compatible = "arm,cortex-a15";
> >      set_feature(&cpu->env, ARM_FEATURE_V7);
> > +    set_feature(&cpu->env, ARM_FEATURE_V7VE);
> >      set_feature(&cpu->env, ARM_FEATURE_VFP4);
> >      set_feature(&cpu->env, ARM_FEATURE_NEON);
> >      set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index fb2f983..cc1e2fb 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1439,6 +1439,7 @@ enum arm_features {
> >      ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
> >      ARM_FEATURE_THUMB2EE,
> >      ARM_FEATURE_V7MP,    /* v7 Multiprocessing Extensions */
> > +    ARM_FEATURE_V7VE,    /* v7 with Virtualization Extensions */
> >      ARM_FEATURE_V4T,
> >      ARM_FEATURE_V5,
> >      ARM_FEATURE_STRONGARM,
> 
> What's the difference between this and ARM_FEATURE_EL2 ?

I use ARM_FEATURE_V7VE in a later patch to guard against implementing
PMOVSSET on v7 machines which don't implement the virtualization
extensions
(http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html).
I could use ARM_FEATURE_EL2, but declaring that v7 machines supported
EL2 didn't feel right. I don't feel strongly one way or the other - how
do you prefer to handle this?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Peter Maydell]

On 17 April 2018 at 15:23, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Apr 12 18:17, Peter Maydell wrote:
>> What's the difference between this and ARM_FEATURE_EL2 ?
>
> I use ARM_FEATURE_V7VE in a later patch to guard against implementing
> PMOVSSET on v7 machines which don't implement the virtualization
> extensions
> (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html).
> I could use ARM_FEATURE_EL2, but declaring that v7 machines supported
> EL2 didn't feel right. I don't feel strongly one way or the other - how
> do you prefer to handle this?

So, the underlying issue here is that there's a QEMU specific
fudge going on. Architecturally, if the CPU implements the
Virtualization Extensions, then:
 * it has Hyp mode
 * it must also implement the Security Extensions
 * on reset it starts in the Secure world
 * it has LPAE
 * it has some stuff that is not inherently tied to having EL2,
   like the SDIV and UDIV instructions, and the presence of
   PMOVSSET

In an ideal world, we'd just have a feature flag that turned
all that on. Unfortunately, a combination of backwards compatibility
issues, the order in which various features were implemented
in QEMU, and the fact that KVM can't emulate a guest CPU with
the Security Extensions means that we want to be able to model
variants of some CPUs that don't really exist in real hardware:
Cortex-A15 and -A7 which only implement EL0/EL1 but still have
all the v7VE features that you can see from those ELs. But we
didn't really properly lay out guidelines for how the feature
bits should work in this case, with the result that we have
a bunch of local hacks (for instance get_S1prot() has a check
on the LPAE feature bit, since in practice that bit is set in
exactly the CPUs that have v7VE; and the UDIV/SDIV insns have
their own feature bits.)

So we should probably sort out this mess first, either by:

(a) state that we use ARM_FEATURE_LPAE for all checks for
features that are architecturally v7VE but which we want to
exist even on our v7VE-no-Hyp-no-Secure oddballs
(b) define an ARM_FEATURE_V7VE for them
(c) define separate feature bits for them individually

In any case we'd retain ARM_FEATURE_EL2 for "and really
has EL2/Hyp mode", and we'd want to do an audit of current
uses of various feature bits to see whether they followed
the new rules.

(For AArch64 things are a bit less awkward because the
architecture allows the idea of an implementation that
has EL2 but not EL3.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On Apr 17 16:00, Peter Maydell wrote:
> On 17 April 2018 at 15:23, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > On Apr 12 18:17, Peter Maydell wrote:
> >> What's the difference between this and ARM_FEATURE_EL2 ?
> >
> > I use ARM_FEATURE_V7VE in a later patch to guard against implementing
> > PMOVSSET on v7 machines which don't implement the virtualization
> > extensions
> > (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html).
> > I could use ARM_FEATURE_EL2, but declaring that v7 machines supported
> > EL2 didn't feel right. I don't feel strongly one way or the other - how
> > do you prefer to handle this?
> 
> So, the underlying issue here is that there's a QEMU specific
> fudge going on. Architecturally, if the CPU implements the
> Virtualization Extensions, then:
>  * it has Hyp mode
>  * it must also implement the Security Extensions
>  * on reset it starts in the Secure world
>  * it has LPAE
>  * it has some stuff that is not inherently tied to having EL2,
>    like the SDIV and UDIV instructions, and the presence of
>    PMOVSSET
> 
> In an ideal world, we'd just have a feature flag that turned
> all that on. Unfortunately, a combination of backwards compatibility
> issues, the order in which various features were implemented
> in QEMU, and the fact that KVM can't emulate a guest CPU with
> the Security Extensions means that we want to be able to model
> variants of some CPUs that don't really exist in real hardware:
> Cortex-A15 and -A7 which only implement EL0/EL1 but still have
> all the v7VE features that you can see from those ELs. But we
> didn't really properly lay out guidelines for how the feature
> bits should work in this case, with the result that we have
> a bunch of local hacks (for instance get_S1prot() has a check
> on the LPAE feature bit, since in practice that bit is set in
> exactly the CPUs that have v7VE; and the UDIV/SDIV insns have
> their own feature bits.)
> 
> So we should probably sort out this mess first, either by:
> 
> (a) state that we use ARM_FEATURE_LPAE for all checks for
> features that are architecturally v7VE but which we want to
> exist even on our v7VE-no-Hyp-no-Secure oddballs
Are you implying that this would only involve updating the comments to
match the existing implementation?

> (b) define an ARM_FEATURE_V7VE for them
This seems the most attractive to me, though perhaps that's partially
aspirational - it is closest to what would be happening in an ideal
world where QEMU fully supported V7VE for this hardware.

> (c) define separate feature bits for them individually

I'm hesitant to formulate a patch for this - I'm not confident I
understand the context and interactions of the pieces involved well
enough to contribute productively. That said, I understand your
motivation to fix this the right way and am willing to try with a little
more direction/hand-holding.

For instance, which CPUs are we talking about here - I only see both
ARM_FEATURE_ARM_DIV and ARM_FEATURE_LPAE advertised together (without
full V8) in cortex_a7_initfn and cortex_a15_initfn. I'm assuming that V8
implies the full set of V7VE features we're discussing here so whatever
we come up with as a solution would be automatically set in the
ARM_FEATURE_V8 "Some features automatically imply others:" `if`
statement in  arm_cpu_realizefn.

-Aaron

> In any case we'd retain ARM_FEATURE_EL2 for "and really
> has EL2/Hyp mode", and we'd want to do an audit of current
> uses of various feature bits to see whether they followed
> the new rules.
> 
> (For AArch64 things are a bit less awkward because the
> architecture allows the idea of an implementation that
> has EL2 but not EL3.)



-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On Apr 17 16:00, Peter Maydell wrote:
> On 17 April 2018 at 15:23, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > On Apr 12 18:17, Peter Maydell wrote:
> >> What's the difference between this and ARM_FEATURE_EL2 ?
> >
> > I use ARM_FEATURE_V7VE in a later patch to guard against implementing
> > PMOVSSET on v7 machines which don't implement the virtualization
> > extensions
> > (http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html).
> > I could use ARM_FEATURE_EL2, but declaring that v7 machines supported
> > EL2 didn't feel right. I don't feel strongly one way or the other - how
> > do you prefer to handle this?
> 
> So, the underlying issue here is that there's a QEMU specific
> fudge going on. Architecturally, if the CPU implements the
> Virtualization Extensions, then:
>  * it has Hyp mode
>  * it must also implement the Security Extensions
>  * on reset it starts in the Secure world
>  * it has LPAE
>  * it has some stuff that is not inherently tied to having EL2,
>    like the SDIV and UDIV instructions, and the presence of
>    PMOVSSET
> 
> In an ideal world, we'd just have a feature flag that turned
> all that on. Unfortunately, a combination of backwards compatibility
> issues, the order in which various features were implemented
> in QEMU, and the fact that KVM can't emulate a guest CPU with
> the Security Extensions means that we want to be able to model
> variants of some CPUs that don't really exist in real hardware:
> Cortex-A15 and -A7 which only implement EL0/EL1 but still have
> all the v7VE features that you can see from those ELs. But we
> didn't really properly lay out guidelines for how the feature
> bits should work in this case, with the result that we have
> a bunch of local hacks (for instance get_S1prot() has a check
> on the LPAE feature bit, since in practice that bit is set in
> exactly the CPUs that have v7VE; and the UDIV/SDIV insns have
> their own feature bits.)
> 
> So we should probably sort out this mess first, either by:
> 
> (a) state that we use ARM_FEATURE_LPAE for all checks for
> features that are architecturally v7VE but which we want to
> exist even on our v7VE-no-Hyp-no-Secure oddballs
> (b) define an ARM_FEATURE_V7VE for them
> (c) define separate feature bits for them individually

>From what I can tell, using ARM_FEATURE_LPAE to represent all the
almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be
supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex
R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in
kvm_arm_get_host_cpu_features() (and may be set/not set independently of
ARM_FEATURE_LPAE). It appears there is a need to independently
distinguish between them. The same reasoning also seems to rule out
option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with
option (c).

It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure
what else makes sense to do here. Am I missing something (I'm almost
hoping I am)?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Peter Maydell]

On 17 May 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Apr 17 16:00, Peter Maydell wrote:
>> So, the underlying issue here is that there's a QEMU specific
>> fudge going on. Architecturally, if the CPU implements the
>> Virtualization Extensions, then:
>>  * it has Hyp mode
>>  * it must also implement the Security Extensions
>>  * on reset it starts in the Secure world
>>  * it has LPAE
>>  * it has some stuff that is not inherently tied to having EL2,
>>    like the SDIV and UDIV instructions, and the presence of
>>    PMOVSSET
>>
>> In an ideal world, we'd just have a feature flag that turned
>> all that on. Unfortunately, a combination of backwards compatibility
>> issues, the order in which various features were implemented
>> in QEMU, and the fact that KVM can't emulate a guest CPU with
>> the Security Extensions means that we want to be able to model
>> variants of some CPUs that don't really exist in real hardware:
>> Cortex-A15 and -A7 which only implement EL0/EL1 but still have
>> all the v7VE features that you can see from those ELs. But we
>> didn't really properly lay out guidelines for how the feature
>> bits should work in this case, with the result that we have
>> a bunch of local hacks (for instance get_S1prot() has a check
>> on the LPAE feature bit, since in practice that bit is set in
>> exactly the CPUs that have v7VE; and the UDIV/SDIV insns have
>> their own feature bits.)
>>
>> So we should probably sort out this mess first, either by:
>>
>> (a) state that we use ARM_FEATURE_LPAE for all checks for
>> features that are architecturally v7VE but which we want to
>> exist even on our v7VE-no-Hyp-no-Secure oddballs
>> (b) define an ARM_FEATURE_V7VE for them
>> (c) define separate feature bits for them individually
>
> From what I can tell, using ARM_FEATURE_LPAE to represent all the
> almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be
> supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex
> R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in
> kvm_arm_get_host_cpu_features() (and may be set/not set independently of
> ARM_FEATURE_LPAE). It appears there is a need to independently
> distinguish between them.

We need (and already have) a separate feature bit for ARM_DIV
exactly because it's present on some CPUs which don't have V7VE;
so we don't want to roll that back into a V7VE feature bit.

> The same reasoning also seems to rule out
> option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with
> option (c).
>
> It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure
> what else makes sense to do here. Am I missing something (I'm almost
> hoping I am)?

Sorry, I forgot I hadn't replied to this email yet. Let's do this:

 * define a new ARM_FEATURE_V7VE:

  ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */

In arm_cpu_realizefn:

 * change the existing "FEATURE_V8 implies V7/ARM_DIV/LPAE" to
   "FEATURE_V8 implies V7VE"
 * below that put

   if (arm_feature(env, ARM_FEATURE_V7VE) {
       /* v7 Virtualization Extensions. In real hardware this implies
        * EL2 and also the presence of the Security Extensions.
        * For QEMU, for backwards-compatibility we implement some
        * CPUs or CPU configs which have no actual EL2 or EL3 but do
        * include the various other features that V7VE implies.
        * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
        * Security Extensions is ARM_FEATURE_EL3.
        */
       set_feature(env, ARM_FEATURE_ARM_DIV);
       set_feature(env, ARM_FEATURE_LPAE);
       set_feature(env, ARM_FEATURE_V7);
   }

 * in kvm_arm_get_host_cpu_features() in kvm32.c add
   set_feature(&features, ARM_FEATURE_V7VE);
   where we currently set V7, LPAE, etc.
   (by definition a host CPU which supports KVM has v7VE.)

 * perhaps some followon patches that correct checks that were
   testing something else and can now use the new V7VE feature bit.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On May 31 15:18, Peter Maydell wrote:
> On 17 May 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > On Apr 17 16:00, Peter Maydell wrote:
> >> So, the underlying issue here is that there's a QEMU specific
> >> fudge going on. Architecturally, if the CPU implements the
> >> Virtualization Extensions, then:
> >>  * it has Hyp mode
> >>  * it must also implement the Security Extensions
> >>  * on reset it starts in the Secure world
> >>  * it has LPAE
> >>  * it has some stuff that is not inherently tied to having EL2,
> >>    like the SDIV and UDIV instructions, and the presence of
> >>    PMOVSSET
> >>
> >> In an ideal world, we'd just have a feature flag that turned
> >> all that on. Unfortunately, a combination of backwards compatibility
> >> issues, the order in which various features were implemented
> >> in QEMU, and the fact that KVM can't emulate a guest CPU with
> >> the Security Extensions means that we want to be able to model
> >> variants of some CPUs that don't really exist in real hardware:
> >> Cortex-A15 and -A7 which only implement EL0/EL1 but still have
> >> all the v7VE features that you can see from those ELs. But we
> >> didn't really properly lay out guidelines for how the feature
> >> bits should work in this case, with the result that we have
> >> a bunch of local hacks (for instance get_S1prot() has a check
> >> on the LPAE feature bit, since in practice that bit is set in
> >> exactly the CPUs that have v7VE; and the UDIV/SDIV insns have
> >> their own feature bits.)
> >>
> >> So we should probably sort out this mess first, either by:
> >>
> >> (a) state that we use ARM_FEATURE_LPAE for all checks for
> >> features that are architecturally v7VE but which we want to
> >> exist even on our v7VE-no-Hyp-no-Secure oddballs
> >> (b) define an ARM_FEATURE_V7VE for them
> >> (c) define separate feature bits for them individually
> >
> > From what I can tell, using ARM_FEATURE_LPAE to represent all the
> > almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be
> > supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex
> > R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in
> > kvm_arm_get_host_cpu_features() (and may be set/not set independently of
> > ARM_FEATURE_LPAE). It appears there is a need to independently
> > distinguish between them.
> 
> We need (and already have) a separate feature bit for ARM_DIV
> exactly because it's present on some CPUs which don't have V7VE;
> so we don't want to roll that back into a V7VE feature bit.
> 
> > The same reasoning also seems to rule out
> > option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with
> > option (c).
> >
> > It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure
> > what else makes sense to do here. Am I missing something (I'm almost
> > hoping I am)?
> 
> Sorry, I forgot I hadn't replied to this email yet. Let's do this:

No problem at all - to be fair, my responses have been a bit sporadic,
too. I'm working on putting your suggestions into patch form, with one
clarification below. I also believe I owe you a response on another
thread or two before I push out the next version of this patchset.

> 
>  * define a new ARM_FEATURE_V7VE:
> 
>   ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */
> 
> In arm_cpu_realizefn:
> 
>  * change the existing "FEATURE_V8 implies V7/ARM_DIV/LPAE" to
>    "FEATURE_V8 implies V7VE"
>  * below that put
> 
>    if (arm_feature(env, ARM_FEATURE_V7VE) {
>        /* v7 Virtualization Extensions. In real hardware this implies
>         * EL2 and also the presence of the Security Extensions.
>         * For QEMU, for backwards-compatibility we implement some
>         * CPUs or CPU configs which have no actual EL2 or EL3 but do
>         * include the various other features that V7VE implies.
>         * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>         * Security Extensions is ARM_FEATURE_EL3.
>         */
>        set_feature(env, ARM_FEATURE_ARM_DIV);

Is it safe to assume from your comment above regarding keeping ARM_DIV
separate from V7VE that the inclusion of it here is an oversight and
that only LPAE and V7 should be set if V7VE is? (and that V8 should
now directly imply both V7VE and ARM_DIV?)

>        set_feature(env, ARM_FEATURE_LPAE);
>        set_feature(env, ARM_FEATURE_V7);
>    }
>
>  * in kvm_arm_get_host_cpu_features() in kvm32.c add
>    set_feature(&features, ARM_FEATURE_V7VE);
>    where we currently set V7, LPAE, etc.
>    (by definition a host CPU which supports KVM has v7VE.)

Thanks!

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Peter Maydell]

On 31 May 2018 at 21:39, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On May 31 15:18, Peter Maydell wrote:
>>    if (arm_feature(env, ARM_FEATURE_V7VE) {
>>        /* v7 Virtualization Extensions. In real hardware this implies
>>         * EL2 and also the presence of the Security Extensions.
>>         * For QEMU, for backwards-compatibility we implement some
>>         * CPUs or CPU configs which have no actual EL2 or EL3 but do
>>         * include the various other features that V7VE implies.
>>         * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
>>         * Security Extensions is ARM_FEATURE_EL3.
>>         */
>>        set_feature(env, ARM_FEATURE_ARM_DIV);
>
> Is it safe to assume from your comment above regarding keeping ARM_DIV
> separate from V7VE that the inclusion of it here is an oversight and
> that only LPAE and V7 should be set if V7VE is? (and that V8 should
> now directly imply both V7VE and ARM_DIV?)

No; V7VE always implies ARM_DIV. (ARM_DIV doesn't imply V7VE,
though, which is why it is a separate feature bit.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On Jun 01 09:57, Peter Maydell wrote:
> On 31 May 2018 at 21:39, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > On May 31 15:18, Peter Maydell wrote:
> >>    if (arm_feature(env, ARM_FEATURE_V7VE) {
> >>        /* v7 Virtualization Extensions. In real hardware this implies
> >>         * EL2 and also the presence of the Security Extensions.
> >>         * For QEMU, for backwards-compatibility we implement some
> >>         * CPUs or CPU configs which have no actual EL2 or EL3 but do
> >>         * include the various other features that V7VE implies.
> >>         * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
> >>         * Security Extensions is ARM_FEATURE_EL3.
> >>         */
> >>        set_feature(env, ARM_FEATURE_ARM_DIV);
> >
> > Is it safe to assume from your comment above regarding keeping ARM_DIV
> > separate from V7VE that the inclusion of it here is an oversight and
> > that only LPAE and V7 should be set if V7VE is? (and that V8 should
> > now directly imply both V7VE and ARM_DIV?)
> 
> No; V7VE always implies ARM_DIV. (ARM_DIV doesn't imply V7VE,
> though, which is why it is a separate feature bit.)

Okay, then I'm confused about some of the preexisting logic in
kvm_arm_get_host_cpu_features. The preexisting code in that function
sets ARM_DIV and THUMB_DIV based on the appropriate bits in ID_ISAR0. If
we already knew that
>    (by definition a host CPU which supports KVM has v7VE.)
and that all V7VE CPUs have ARM_DIV, why did the code there bother
checking ID_ISAR0 to begin with?

> switch (extract32(id_isar0, 24, 4)) {
> case 1:
>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
>     break;
> case 2:
>     set_feature(&features, ARM_FEATURE_ARM_DIV);
>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
>     break;
> default:
>     break;
> }

Should this switch/case be removed entirely?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Peter Maydell]

On 1 June 2018 at 16:34, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Jun 01 09:57, Peter Maydell wrote:
>> No; V7VE always implies ARM_DIV. (ARM_DIV doesn't imply V7VE,
>> though, which is why it is a separate feature bit.)
>
> Okay, then I'm confused about some of the preexisting logic in
> kvm_arm_get_host_cpu_features. The preexisting code in that function
> sets ARM_DIV and THUMB_DIV based on the appropriate bits in ID_ISAR0. If
> we already knew that
>>    (by definition a host CPU which supports KVM has v7VE.)
> and that all V7VE CPUs have ARM_DIV, why did the code there bother
> checking ID_ISAR0 to begin with?

Good question. I suspect I'd just forgotten that at the point
when I wrote that KVM code.

>> switch (extract32(id_isar0, 24, 4)) {
>> case 1:
>>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
>>     break;
>> case 2:
>>     set_feature(&features, ARM_FEATURE_ARM_DIV);
>>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
>>     break;
>> default:
>>     break;
>> }
>
> Should this switch/case be removed entirely?

Yes, I think so (and also the id_isar0 variable and the
idregs[] entry which arranges to initialize it). We should
just set the THUMB_DIV and ARM_DIV features explicitly.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On Jun 01 16:59, Peter Maydell wrote:
> On 1 June 2018 at 16:34, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> >> switch (extract32(id_isar0, 24, 4)) {
> >> case 1:
> >>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
> >>     break;
> >> case 2:
> >>     set_feature(&features, ARM_FEATURE_ARM_DIV);
> >>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
> >>     break;
> >> default:
> >>     break;
> >> }
> >
> > Should this switch/case be removed entirely?
> 
> Yes, I think so (and also the id_isar0 variable and the
> idregs[] entry which arranges to initialize it). We should
> just set the THUMB_DIV and ARM_DIV features explicitly.

Okay, thanks for straightening me out! I sensed there was some
inconsistency, but misjudged exactly where. I'll include this change in
my next patchset.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 16/22] target/arm: Implement PMOVSSET

[Aaron Lindsay]

Adding an array for v7VE+ CP registers was necessary so that PMOVSSET
wasn't defined for all v7 processors.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d4f06e6..f5e800e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1241,9 +1241,17 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                          uint64_t value)
 {
+    value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pmovsr &= ~value;
 }
 
+static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                         uint64_t value)
+{
+    value &= PMU_COUNTER_MASK(env);
+    env->cp15.c9_pmovsr |= value;
+}
+
 static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
@@ -1406,7 +1414,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
       .writefn = pmcntenclr_write },
     { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
-      .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+      .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
       .accessfn = pmreg_access,
       .writefn = pmovsr_write,
       .raw_writefn = raw_write },
@@ -1592,6 +1600,25 @@ static const ARMCPRegInfo v7mp_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
+static const ARMCPRegInfo v7ve_cp_reginfo[] = {
+    /* Performance monitor registers which are not implemented in v7 before
+     * v7ve:
+     */
+    { .name = "PMOVSSET", .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 3,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
+      .writefn = pmovsset_write,
+      .raw_writefn = raw_write },
+    { .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+      .writefn = pmovsset_write,
+      .raw_writefn = raw_write },
+    REGINFO_SENTINEL
+};
+
 static void teecr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
@@ -4943,6 +4970,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         !arm_feature(env, ARM_FEATURE_PMSA)) {
         define_arm_cp_regs(cpu, v7mp_cp_reginfo);
     }
+    if (arm_feature(env, ARM_FEATURE_V7VE)) {
+        define_arm_cp_regs(cpu, v7ve_cp_reginfo);
+    }
     if (arm_feature(env, ARM_FEATURE_V7)) {
         /* v7 performance monitor control register: same implementor
          * field as main ID register, and we implement only the cycle
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 16/22] target/arm: Implement PMOVSSET

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Adding an array for v7VE+ CP registers was necessary so that PMOVSSET
> wasn't defined for all v7 processors.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d4f06e6..f5e800e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1241,9 +1241,17 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                           uint64_t value)
>  {
> +    value &= PMU_COUNTER_MASK(env);
>      env->cp15.c9_pmovsr &= ~value;
>  }
>
> +static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                         uint64_t value)
> +{
> +    value &= PMU_COUNTER_MASK(env);
> +    env->cp15.c9_pmovsr |= value;
> +}
> +
>  static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                               uint64_t value)
>  {
> @@ -1406,7 +1414,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
>        .writefn = pmcntenclr_write },
>      { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
> -      .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> +      .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>        .accessfn = pmreg_access,
>        .writefn = pmovsr_write,
>        .raw_writefn = raw_write },

This change is half of a bug fix (the other half being to make the
field in the CPU struct be uint64_t rather than uint32_t). That
bug fix should be in a patch of its own.

pmuserenr has the same bug (uint32_t state field accessed by
a STATE_AA64 sysreg).

thanks
-- PMM

[Qemu-devel] [PATCH v3 17/22] target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f5e800e..2073d56 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1009,17 +1009,22 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
     return pmreg_access(env, ri, isread);
 }
 
-static inline bool arm_ccnt_enabled(CPUARMState *env)
+static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
 {
     /* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered */
-
-    if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
+    if (!(env->cp15.c9_pmcr & PMCRE) ||
+            !(env->cp15.c9_pmcnten & (1 << counter))) {
         return false;
     }
 
     return true;
 }
 
+static inline bool arm_ccnt_enabled(CPUARMState *env)
+{
+    return pmu_counter_enabled(env, 31);
+}
+
 /* Returns true if the counter corresponding to the passed-in pmevtyper or
  * pmccfiltr value is filtered using the current state */
 static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 17/22] target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled

[Peter Maydell]

On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f5e800e..2073d56 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1009,17 +1009,22 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
>      return pmreg_access(env, ri, isread);
>  }
>
> -static inline bool arm_ccnt_enabled(CPUARMState *env)
> +static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
>  {
>      /* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered */
> -
> -    if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
> +    if (!(env->cp15.c9_pmcr & PMCRE) ||
> +            !(env->cp15.c9_pmcnten & (1 << counter))) {
>          return false;
>      }
>
>      return true;
>  }
>
> +static inline bool arm_ccnt_enabled(CPUARMState *env)
> +{
> +    return pmu_counter_enabled(env, 31);
> +}

We only use arm_ccnt_enabled() in a couple of places, so
I would just make those callsites use pmu_counter_enabled(env, 31)
directly.

thanks
-- PMM

[Qemu-devel] [PATCH v3 18/22] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay]

This commit doesn't add any supported events, but provides the framework
for adding them. We store the pm_event structs in a simple array, and
provide the mapping from the event numbers to array indexes in
the supported_event_map array.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c    |  4 ++++
 target/arm/cpu.h    | 10 ++++++++++
 target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index e544f1d..69d6a80 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -889,6 +889,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         unset_feature(env, ARM_FEATURE_PMU);
         cpu->id_aa64dfr0 &= ~0xf00;
     } else {
+        uint64_t pmceid = get_pmceid(&cpu->env);
+        cpu->pmceid0 = pmceid & 0xffffffff;
+        cpu->pmceid1 = (pmceid >> 32) & 0xffffffff;
+
         arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
         arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
     }
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index cc1e2fb..19f005d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -931,6 +931,16 @@ void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles);
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
 void pmu_post_el_change(ARMCPU *cpu, void *ignored);
 
+/*
+ * get_pmceid
+ * @env: CPUARMState
+ *
+ * Return the PMCEID[01] register values corresponding to the counters which
+ * are supported given the current configuration (0 is low 32, 1 is high 32
+ * bits)
+ */
+uint64_t get_pmceid(CPUARMState *env);
+
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
  * for both old and new bit meanings. Code which tests against those
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2073d56..6a4f900 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -925,6 +925,43 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
 
+typedef struct pm_event {
+    uint16_t number; /* PMEVTYPER.evtCount is 10 bits wide */
+    /* If the event is supported on this CPU (used to generate PMCEID[01]) */
+    bool (*supported)(CPUARMState *);
+    /* Retrieve the current count of the underlying event. The programmed
+     * counters hold a difference from the return value from this function */
+    uint64_t (*get_count)(CPUARMState *);
+} pm_event;
+
+#define SUPPORTED_EVENT_SENTINEL UINT16_MAX
+static const pm_event pm_events[] = {
+    { .number = SUPPORTED_EVENT_SENTINEL }
+};
+static uint16_t supported_event_map[0x3f];
+
+/*
+ * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high
+ * 32). We also use it to build a map of ARM event numbers to indices in
+ * our pm_events array.
+ */
+uint64_t get_pmceid(CPUARMState *env)
+{
+    uint64_t pmceid = 0;
+    unsigned int i = 0;
+    while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) {
+        const pm_event *cnt = &pm_events[i];
+        if (cnt->number < 0x3f && cnt->supported(env)) {
+            pmceid |= (1 << cnt->number);
+            supported_event_map[cnt->number] = i;
+        } else {
+            supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL;
+        }
+        i++;
+    }
+    return pmceid;
+}
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                    bool isread)
 {
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 19/22] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER

[Aaron Lindsay]

Add arrays to hold the registers, the definitions themselves, access
functions, and add logic to reset counters when PMCR.P is set.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.h    |   7 +-
 target/arm/helper.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 207 insertions(+), 19 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 19f005d..7a74966 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -454,8 +454,9 @@ typedef struct CPUARMState {
         uint64_t oslsr_el1; /* OS Lock Status */
         uint64_t mdcr_el2;
         uint64_t mdcr_el3;
-        /* If the counter is enabled, this stores the last time the counter
-         * was reset. Otherwise it stores the counter value
+        /* If the pmccntr and pmevcntr counters are enabled, they store the
+         * offset the last time the counter was reset. Otherwise they store the
+         * counter value.
          */
         uint64_t c15_ccnt;
         /* ccnt_cached_cycles is used to hold the last cycle count when
@@ -463,6 +464,8 @@ typedef struct CPUARMState {
          * PMU operations which require this.
          */
         uint64_t ccnt_cached_cycles;
+        uint64_t c14_pmevcntr[31];
+        uint64_t c14_pmevtyper[31];
         uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
         uint64_t vpidr_el2; /* Virtualization Processor ID Register */
         uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6a4f900..2fa8308 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -906,6 +906,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRN_SHIFT 11
 #define PMCRD   0x8
 #define PMCRC   0x4
+#define PMCRP   0x2
 #define PMCRE   0x1
 
 #define PMXEVTYPER_P          0x80000000
@@ -931,7 +932,7 @@ typedef struct pm_event {
     bool (*supported)(CPUARMState *);
     /* Retrieve the current count of the underlying event. The programmed
      * counters hold a difference from the return value from this function */
-    uint64_t (*get_count)(CPUARMState *);
+    uint64_t (*get_count)(CPUARMState *, uint64_t cycles);
 } pm_event;
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
@@ -1054,6 +1055,21 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
         return false;
     }
 
+    if (counter != 31) {
+        /* If not checking PMCCNTR, ensure the counter is setup to an event we
+         * support */
+        uint16_t event = env->cp15.c14_pmevtyper[counter] & PMXEVTYPER_EVTCOUNT;
+        if (event > 0x3f) {
+            return false; /* We only support common architectural and
+                             microarchitectural events */
+        }
+
+        uint16_t event_idx = supported_event_map[event];
+        if (event_idx == SUPPORTED_EVENT_SENTINEL) {
+            return false;
+        }
+    }
+
     return true;
 }
 
@@ -1149,14 +1165,37 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
     }
 }
 
+static void pmu_sync_counter(CPUARMState *env, uint8_t counter, uint64_t cycles)
+{
+    if (pmu_counter_enabled(env, counter) &&
+        !pmu_counter_filtered(env, env->cp15.c14_pmevtyper[counter])) {
+
+        uint16_t event = env->cp15.c14_pmevtyper[counter] & PMXEVTYPER_EVTCOUNT;
+        uint16_t event_idx = supported_event_map[event];
+
+        uint64_t count = pm_events[event_idx].get_count(env, cycles);
+        env->cp15.c14_pmevcntr[counter] =
+            count - env->cp15.c14_pmevcntr[counter];
+    }
+}
+
 uint64_t pmu_op_start(CPUARMState *env)
 {
-    return pmccntr_op_start(env);
+    uint64_t saved_cycles = pmccntr_op_start(env);
+    unsigned int i;
+    for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+        pmu_sync_counter(env, i, saved_cycles);
+    }
+    return saved_cycles;
 }
 
 void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
     pmccntr_op_finish(env, prev_cycles);
+    unsigned int i;
+    for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+        pmu_sync_counter(env, i, prev_cycles);
+    }
 }
 
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
@@ -1179,6 +1218,13 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         env->cp15.c15_ccnt = 0;
     }
 
+    if (value & PMCRP) {
+        unsigned int i;
+        for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+            env->cp15.c14_pmevcntr[i] = 0;
+        }
+    }
+
     /* only the DP, X, D and E bits are writable */
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
@@ -1294,30 +1340,127 @@ static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmovsr |= value;
 }
 
-static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
-                             uint64_t value)
+static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value, const uint8_t counter)
 {
+    if (counter == 0x1f) {
+        pmccfiltr_write(env, ri, value);
+    } else if (counter < PMU_NUM_COUNTERS(env)) {
+        uint64_t cycles = 0;
+#ifndef CONFIG_USER_ONLY
+        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#endif
+        pmu_sync_counter(env, counter, cycles);
+        env->cp15.c14_pmevtyper[counter] = value & 0xfe0003ff;
+        pmu_sync_counter(env, counter, cycles);
+    }
     /* Attempts to access PMXEVTYPER are CONSTRAINED UNPREDICTABLE when
      * PMSELR value is equal to or greater than the number of implemented
      * counters, but not equal to 0x1f. We opt to behave as a RAZ/WI.
      */
-    if (env->cp15.c9_pmselr == 0x1f) {
-        pmccfiltr_write(env, ri, value);
+}
+
+static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                               const uint8_t counter)
+{
+    if (counter == 0x1f) {
+        return env->cp15.pmccfiltr_el0;
+    } else if (counter < PMU_NUM_COUNTERS(env)) {
+        return env->cp15.c14_pmevtyper[counter];
+    } else {
+      /* We opt to behave as a RAZ/WI when attempts to access PMXEVTYPER
+       * are CONSTRAINED UNPREDICTABLE. See comments in pmevtyper_write().
+       */
+        return 0;
     }
 }
 
+static void pmevtyper_writefn(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    pmevtyper_write(env, ri, value, counter);
+}
+
+static uint64_t pmevtyper_readfn(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    return pmevtyper_read(env, ri, counter);
+}
+
+static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value)
+{
+    pmevtyper_write(env, ri, value, env->cp15.c9_pmselr & 31);
+}
+
 static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    /* We opt to behave as a RAZ/WI when attempts to access PMXEVTYPER
-     * are CONSTRAINED UNPREDICTABLE. See comments in pmxevtyper_write().
-     */
-    if (env->cp15.c9_pmselr == 0x1f) {
-        return env->cp15.pmccfiltr_el0;
+    return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
+static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value, uint8_t counter)
+{
+    if (counter < PMU_NUM_COUNTERS(env)) {
+        uint64_t cycles = 0;
+#ifndef CONFIG_USER_ONLY
+        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#endif
+        env->cp15.c14_pmevcntr[counter] = value;
+        pmu_sync_counter(env, counter, cycles);
+    }
+    /* We opt to behave as a RAZ/WI when attempts to access PM[X]EVCNTR
+     * are CONSTRAINED UNPREDICTABLE. */
+}
+
+static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint8_t counter)
+{
+    if (counter < PMU_NUM_COUNTERS(env)) {
+        uint64_t ret;
+        uint64_t cycles = 0;
+#ifndef CONFIG_USER_ONLY
+        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#endif
+        pmu_sync_counter(env, counter, cycles);
+        ret = env->cp15.c14_pmevcntr[counter];
+        pmu_sync_counter(env, counter, cycles);
+        return ret;
     } else {
+      /* We opt to behave as a RAZ/WI when attempts to access PM[X]EVCNTR
+       * are CONSTRAINED UNPREDICTABLE. */
         return 0;
     }
 }
 
+static void pmevcntr_writefn(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    pmevcntr_write(env, ri, value, counter);
+}
+
+static uint64_t pmevcntr_readfn(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    return pmevcntr_read(env, ri, counter);
+}
+
+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 pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
 static void pmuserenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -1504,16 +1647,23 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
       .resetvalue = 0, },
     { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
-      .access = PL0_RW, .type = ARM_CP_NO_RAW, .accessfn = pmreg_access,
+      .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .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_NO_RAW, .accessfn = pmreg_access,
+      .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .accessfn = pmreg_access,
       .writefn = pmxevtyper_write, .readfn = pmxevtyper_read },
-    /* 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_xevcntr },
+      .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .accessfn = pmreg_access_xevcntr,
+      .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_NO_RAW | ARM_CP_IO,
+      .accessfn = pmreg_access_xevcntr,
+      .writefn = pmxevcntr_write, .readfn = pmxevcntr_read },
     { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
       .access = PL0_R | PL1_RW, .accessfn = access_tpm,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
@@ -4204,7 +4354,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
 #endif
     /* The only field of MDCR_EL2 that has a defined architectural reset value
      * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we
-     * don't impelment any PMU event counters, so using zero as a reset
+     * don't implement any PMU event counters, so using zero as a reset
      * value for MDCR_EL2 is okay
      */
     { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
@@ -5016,6 +5166,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_arm_cp_regs(cpu, v7ve_cp_reginfo);
     }
     if (arm_feature(env, ARM_FEATURE_V7)) {
+        unsigned int i;
         /* v7 performance monitor control register: same implementor
          * field as main ID register, and we implement only the cycle
          * count register.
@@ -5040,6 +5191,40 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         };
         define_one_arm_cp_reg(cpu, &pmcr);
         define_one_arm_cp_reg(cpu, &pmcr64);
+        for (i = 0; i < 31; i++) {
+            char *pmevcntr_name = g_strdup_printf("PMEVCNTR%d", i);
+            char *pmevcntr_el0_name = g_strdup_printf("PMEVCNTR%d_EL0", i);
+            char *pmevtyper_name = g_strdup_printf("PMEVTYPER%d", i);
+            char *pmevtyper_el0_name = g_strdup_printf("PMEVTYPER%d_EL0", i);
+            ARMCPRegInfo pmev_regs[] = {
+                { .name = pmevcntr_name, .cp = 15, .crn = 15,
+                  .crm = 8 | (3 & (i >> 3)), .opc1 = 0, .opc2 = i & 7,
+                  .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+                  .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn,
+                  .accessfn = pmreg_access },
+                { .name = pmevcntr_el0_name, .state = ARM_CP_STATE_AA64,
+                  .opc0 = 3, .opc1 = 3, .crn = 15, .crm = 8 | (3 & (i >> 3)),
+                  .opc2 = i & 7, .access = PL0_RW, .accessfn = pmreg_access,
+                  .type = ARM_CP_NO_RAW | ARM_CP_IO,
+                  .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn },
+                { .name = pmevtyper_name, .cp = 15, .crn = 15,
+                  .crm = 12 | (3 & (i >> 3)), .opc1 = 0, .opc2 = i & 7,
+                  .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+                  .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn,
+                  .accessfn = pmreg_access },
+                { .name = pmevtyper_el0_name, .state = ARM_CP_STATE_AA64,
+                  .opc0 = 3, .opc1 = 3, .crn = 15, .crm = 12 | (3 & (i >> 3)),
+                  .opc2 = i & 7, .access = PL0_RW, .accessfn = pmreg_access,
+                  .type = ARM_CP_NO_RAW | ARM_CP_IO,
+                  .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn },
+                REGINFO_SENTINEL
+            };
+            define_arm_cp_regs(cpu, pmev_regs);
+            g_free(pmevcntr_name);
+            g_free(pmevcntr_el0_name);
+            g_free(pmevtyper_name);
+            g_free(pmevtyper_el0_name);
+        }
 #endif
         ARMCPRegInfo clidr = {
             .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

The instruction event is only enabled when icount is used, cycles are
always supported. Always defining get_cycle_count (but altering its
behavior depending on CONFIG_USER_ONLY) allows us to remove some
CONFIG_USER_ONLY #defines throughout the rest of the code.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 99 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 47 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2fa8308..679897a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -15,6 +15,7 @@
 #include "arm_ldst.h"
 #include <zlib.h> /* For crc32 */
 #include "exec/semihost.h"
+#include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "fpu/softfloat.h"
 
@@ -935,8 +936,54 @@ typedef struct pm_event {
     uint64_t (*get_count)(CPUARMState *, uint64_t cycles);
 } pm_event;
 
+/*
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */
+static uint64_t get_cycle_count(CPUARMState *env)
+{
+#ifndef CONFIG_USER_ONLY
+    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#else
+    return 0;
+#endif
+}
+
+static bool event_always_supported(CPUARMState *env)
+{
+    return true;
+}
+
+#ifndef CONFIG_USER_ONLY
+static uint64_t cycles_get_count(CPUARMState *env, uint64_t cycles)
+{
+    return cycles;
+}
+
+static bool instructions_supported(CPUARMState *env)
+{
+    return use_icount == 1 /* Precise instruction counting */;
+}
+
+static uint64_t instructions_get_count(CPUARMState *env, uint64_t cycles)
+{
+    return (uint64_t)cpu_get_icount_raw();
+}
+#endif
+
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
+#ifndef CONFIG_USER_ONLY
+    { .number = 0x008, /* INST_RETIRED */
+      .supported = instructions_supported,
+      .get_count = instructions_get_count
+    },
+    { .number = 0x011, /* CPU_CYCLES */
+      .supported = event_always_supported,
+      .get_count = cycles_get_count
+    },
+#endif
     { .number = SUPPORTED_EVENT_SENTINEL }
 };
 static uint16_t supported_event_map[0x3f];
@@ -1016,8 +1063,6 @@ static CPAccessResult pmreg_access_swinc(CPUARMState *env,
     return pmreg_access(env, ri, isread);
 }
 
-#ifndef CONFIG_USER_ONLY
-
 static CPAccessResult pmreg_access_selr(CPUARMState *env,
                                         const ARMCPRegInfo *ri,
                                         bool isread)
@@ -1126,11 +1171,7 @@ static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
  */
 uint64_t pmccntr_op_start(CPUARMState *env)
 {
-    uint64_t cycles = 0;
-#ifndef CONFIG_USER_ONLY
-    cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-#endif
+    uint64_t cycles = get_cycle_count(env);
 
     if (arm_ccnt_enabled(env) &&
           !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
@@ -1268,26 +1309,6 @@ static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
     pmccntr_write(env, ri, deposit64(cur_val, 0, 32, value));
 }
 
-#else /* CONFIG_USER_ONLY */
-
-uint64_t pmccntr_op_start(CPUARMState *env)
-{
-}
-
-void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
-{
-}
-
-uint64_t pmu_op_start(CPUARMState *env)
-{
-}
-
-void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
-{
-}
-
-#endif
-
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -1346,11 +1367,7 @@ static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
     if (counter == 0x1f) {
         pmccfiltr_write(env, ri, value);
     } else if (counter < PMU_NUM_COUNTERS(env)) {
-        uint64_t cycles = 0;
-#ifndef CONFIG_USER_ONLY
-        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-#endif
+        uint64_t cycles = get_cycle_count(env);
         pmu_sync_counter(env, counter, cycles);
         env->cp15.c14_pmevtyper[counter] = value & 0xfe0003ff;
         pmu_sync_counter(env, counter, cycles);
@@ -1404,11 +1421,7 @@ static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value, uint8_t counter)
 {
     if (counter < PMU_NUM_COUNTERS(env)) {
-        uint64_t cycles = 0;
-#ifndef CONFIG_USER_ONLY
-        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-#endif
+        uint64_t cycles = get_cycle_count(env);
         env->cp15.c14_pmevcntr[counter] = value;
         pmu_sync_counter(env, counter, cycles);
     }
@@ -1420,12 +1433,8 @@ static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
                               uint8_t counter)
 {
     if (counter < PMU_NUM_COUNTERS(env)) {
-        uint64_t ret;
-        uint64_t cycles = 0;
-#ifndef CONFIG_USER_ONLY
-        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-#endif
+        uint64_t ret, cycles;
+        cycles = get_cycle_count(env);
         pmu_sync_counter(env, counter, cycles);
         ret = env->cp15.c14_pmevcntr[counter];
         pmu_sync_counter(env, counter, cycles);
@@ -1613,7 +1622,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
     /* Unimplemented so WI. */
     { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
       .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
-#ifndef CONFIG_USER_ONLY
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
       .access = PL0_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
@@ -1633,7 +1641,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL0_RW, .accessfn = pmreg_access_ccntr,
       .type = ARM_CP_IO,
       .readfn = pmccntr_read, .writefn = pmccntr_write, },
-#endif
     { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7,
       .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
       .access = PL0_RW, .accessfn = pmreg_access,
@@ -5171,7 +5178,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
          * field as main ID register, and we implement only the cycle
          * count register.
          */
-#ifndef CONFIG_USER_ONLY
         ARMCPRegInfo pmcr = {
             .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
             .access = PL0_RW,
@@ -5225,7 +5231,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             g_free(pmevtyper_name);
             g_free(pmevtyper_el0_name);
         }
-#endif
         ARMCPRegInfo clidr = {
             .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
             .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> The instruction event is only enabled when icount is used, cycles are
> always supported. Always defining get_cycle_count (but altering its
> behavior depending on CONFIG_USER_ONLY) allows us to remove some
> CONFIG_USER_ONLY #defines throughout the rest of the code.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/helper.c | 99 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 52 insertions(+), 47 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2fa8308..679897a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -15,6 +15,7 @@
>  #include "arm_ldst.h"
>  #include <zlib.h> /* For crc32 */
>  #include "exec/semihost.h"
> +#include "sysemu/cpus.h"
>  #include "sysemu/kvm.h"
>  #include "fpu/softfloat.h"
>  
> @@ -935,8 +936,54 @@ typedef struct pm_event {
>      uint64_t (*get_count)(CPUARMState *, uint64_t cycles);
>  } pm_event;
>  
> +/*
> + * Return the underlying cycle count for the PMU cycle counters. If we're in
> + * usermode, simply return 0.
> + */
> +static uint64_t get_cycle_count(CPUARMState *env)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> +#else
> +    return 0;
> +#endif
> +}
> +
> +static bool event_always_supported(CPUARMState *env)
> +{
> +    return true;
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +static uint64_t cycles_get_count(CPUARMState *env, uint64_t cycles)
> +{
> +    return cycles;
> +}
> +
> +static bool instructions_supported(CPUARMState *env)
> +{
> +    return use_icount == 1 /* Precise instruction counting */;
> +}
> +
> +static uint64_t instructions_get_count(CPUARMState *env, uint64_t cycles)
> +{
> +    return (uint64_t)cpu_get_icount_raw();
> +}
> +#endif
> +
>  #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
>  static const pm_event pm_events[] = {
> +#ifndef CONFIG_USER_ONLY
> +    { .number = 0x008, /* INST_RETIRED */
> +      .supported = instructions_supported,
> +      .get_count = instructions_get_count
> +    },
> +    { .number = 0x011, /* CPU_CYCLES */
> +      .supported = event_always_supported,
> +      .get_count = cycles_get_count
> +    },
> +#endif
>      { .number = SUPPORTED_EVENT_SENTINEL }
>  };
>  static uint16_t supported_event_map[0x3f];
> @@ -1016,8 +1063,6 @@ static CPAccessResult pmreg_access_swinc(CPUARMState *env,
>      return pmreg_access(env, ri, isread);
>  }
>  
> -#ifndef CONFIG_USER_ONLY
> -
>  static CPAccessResult pmreg_access_selr(CPUARMState *env,
>                                          const ARMCPRegInfo *ri,
>                                          bool isread)
> @@ -1126,11 +1171,7 @@ static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
>   */
>  uint64_t pmccntr_op_start(CPUARMState *env)
>  {
> -    uint64_t cycles = 0;
> -#ifndef CONFIG_USER_ONLY
> -    cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> -#endif
> +    uint64_t cycles = get_cycle_count(env);
>  
>      if (arm_ccnt_enabled(env) &&
>            !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
> @@ -1268,26 +1309,6 @@ static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
>      pmccntr_write(env, ri, deposit64(cur_val, 0, 32, value));
>  }
>  
> -#else /* CONFIG_USER_ONLY */
> -
> -uint64_t pmccntr_op_start(CPUARMState *env)
> -{
> -}
> -
> -void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
> -{
> -}
> -
> -uint64_t pmu_op_start(CPUARMState *env)
> -{
> -}
> -
> -void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
> -{
> -}
> -
> -#endif
> -
>  static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> @@ -1346,11 +1367,7 @@ static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      if (counter == 0x1f) {
>          pmccfiltr_write(env, ri, value);
>      } else if (counter < PMU_NUM_COUNTERS(env)) {
> -        uint64_t cycles = 0;
> -#ifndef CONFIG_USER_ONLY
> -        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> -#endif
> +        uint64_t cycles = get_cycle_count(env);
>          pmu_sync_counter(env, counter, cycles);
>          env->cp15.c14_pmevtyper[counter] = value & 0xfe0003ff;
>          pmu_sync_counter(env, counter, cycles);
> @@ -1404,11 +1421,7 @@ static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                               uint64_t value, uint8_t counter)
>  {
>      if (counter < PMU_NUM_COUNTERS(env)) {
> -        uint64_t cycles = 0;
> -#ifndef CONFIG_USER_ONLY
> -        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> -#endif
> +        uint64_t cycles = get_cycle_count(env);
>          env->cp15.c14_pmevcntr[counter] = value;
>          pmu_sync_counter(env, counter, cycles);
>      }
> @@ -1420,12 +1433,8 @@ static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>                                uint8_t counter)
>  {
>      if (counter < PMU_NUM_COUNTERS(env)) {
> -        uint64_t ret;
> -        uint64_t cycles = 0;
> -#ifndef CONFIG_USER_ONLY
> -        cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> -#endif
> +        uint64_t ret, cycles;
> +        cycles = get_cycle_count(env);
>          pmu_sync_counter(env, counter, cycles);
>          ret = env->cp15.c14_pmevcntr[counter];
>          pmu_sync_counter(env, counter, cycles);
> @@ -1613,7 +1622,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>      /* Unimplemented so WI. */
>      { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
>        .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
> -#ifndef CONFIG_USER_ONLY
>      { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
>        .access = PL0_RW, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
> @@ -1633,7 +1641,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .access = PL0_RW, .accessfn = pmreg_access_ccntr,
>        .type = ARM_CP_IO,
>        .readfn = pmccntr_read, .writefn = pmccntr_write, },
> -#endif
>      { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7,
>        .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
>        .access = PL0_RW, .accessfn = pmreg_access,
> @@ -5171,7 +5178,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>           * field as main ID register, and we implement only the cycle
>           * count register.
>           */
> -#ifndef CONFIG_USER_ONLY
>          ARMCPRegInfo pmcr = {
>              .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
>              .access = PL0_RW,
> @@ -5225,7 +5231,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              g_free(pmevtyper_name);
>              g_free(pmevtyper_el0_name);
>          }
> -#endif
>          ARMCPRegInfo clidr = {
>              .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
>              .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1,
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Philippe Mathieu-Daudé]

On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> The instruction event is only enabled when icount is used, cycles are
> always supported. Always defining get_cycle_count (but altering its
> behavior depending on CONFIG_USER_ONLY) allows us to remove some
> CONFIG_USER_ONLY #defines throughout the rest of the code.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
[...]>  #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
>  static const pm_event pm_events[] = {
> +#ifndef CONFIG_USER_ONLY
> +    { .number = 0x008, /* INST_RETIRED */

"Instruction architecturally executed" seems more explicit to me.

> +      .supported = instructions_supported,
> +      .get_count = instructions_get_count
> +    },
> +    { .number = 0x011, /* CPU_CYCLES */
> +      .supported = event_always_supported,
> +      .get_count = cycles_get_count
> +    },
> +#endif
[...]

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

On Mar 18 23:48, Philippe Mathieu-Daudé wrote:
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > The instruction event is only enabled when icount is used, cycles are
> > always supported. Always defining get_cycle_count (but altering its
> > behavior depending on CONFIG_USER_ONLY) allows us to remove some
> > CONFIG_USER_ONLY #defines throughout the rest of the code.
> > 
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> [...]>  #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
> >  static const pm_event pm_events[] = {
> > +#ifndef CONFIG_USER_ONLY
> > +    { .number = 0x008, /* INST_RETIRED */
> 
> "Instruction architecturally executed" seems more explicit to me.

I've updated v4 to include this wording as well.

> > +      .supported = instructions_supported,
> > +      .get_count = instructions_get_count
> > +    },
> > +    { .number = 0x011, /* CPU_CYCLES */
> > +      .supported = event_always_supported,
> > +      .get_count = cycles_get_count
> > +    },
> > +#endif
> [...]

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 21/22] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

This both advertises that we support four counters and adds them to the
implementation because the PMU_NUM_COUNTERS macro reads this value from
the PMCR.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 679897a..06e2e2c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1575,7 +1575,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_W, .type = ARM_CP_NOP },
     /* Performance monitors are implementation defined in v7,
      * but with an ARM recommended set of registers, which we
-     * follow (although we don't actually implement any counters)
+     * follow.
      *
      * Performance registers fall into three categories:
      *  (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR)
@@ -5192,7 +5192,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             .access = PL0_RW, .accessfn = pmreg_access,
             .type = ARM_CP_IO,
             .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
-            .resetvalue = cpu->midr & 0xff000000,
+            /* 4 counters enabled */
+            .resetvalue = (cpu->midr & 0xff000000) | (0x4 << PMCRN_SHIFT),
             .writefn = pmcr_write, .raw_writefn = raw_write,
         };
         define_one_arm_cp_reg(cpu, &pmcr);
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 22/22] target/arm: Implement PMSWINC

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 06e2e2c..4f8d11c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -955,6 +955,15 @@ static bool event_always_supported(CPUARMState *env)
     return true;
 }
 
+static uint64_t swinc_get_count(CPUARMState *env, uint64_t cycles)
+{
+    /*
+     * SW_INCR events are written directly to the pmevcntr's by writes to
+     * PMSWINC, so there is no underlying count maintained by the PMU itself
+     */
+    return 0;
+}
+
 #ifndef CONFIG_USER_ONLY
 static uint64_t cycles_get_count(CPUARMState *env, uint64_t cycles)
 {
@@ -974,6 +983,10 @@ static uint64_t instructions_get_count(CPUARMState *env, uint64_t cycles)
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
+    { .number = 0x000, /* SW_INCR */
+      .supported = event_always_supported,
+      .get_count = swinc_get_count
+    },
 #ifndef CONFIG_USER_ONLY
     { .number = 0x008, /* INST_RETIRED */
       .supported = instructions_supported,
@@ -1273,6 +1286,29 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     pmu_op_finish(env, saved_cycles);
 }
 
+static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                          uint64_t value)
+{
+    unsigned int i;
+    for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+        /* Increment a counter's count iff: */
+        if ((value & (1 << i)) && /* counter's bit is set */
+                /* counter is enabled and not filtered */
+                pmu_counter_enabled(env, i) &&
+                !pmu_counter_filtered(env, env->cp15.c14_pmevtyper[i]) &&
+                /* counter is SW_INCR */
+                (env->cp15.c14_pmevtyper[i] & PMXEVTYPER_EVTCOUNT) == 0x0) {
+            uint64_t cycles = 0;
+#ifndef CONFIG_USER_ONLY
+            cycles = get_cycle_count(env);
+#endif
+            pmu_sync_counter(env, i, cycles);
+            env->cp15.c14_pmevcntr[i]++;
+            pmu_sync_counter(env, i, cycles);
+        }
+    }
+}
+
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     uint64_t ret;
@@ -1619,9 +1655,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
       .writefn = pmovsr_write,
       .raw_writefn = raw_write },
-    /* Unimplemented so WI. */
     { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
-      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
+      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+      .writefn = pmswinc_write },
+    { .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 4,
+      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+      .writefn = pmswinc_write },
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
       .access = PL0_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1521232280-13089-1-git-send-email-alindsay@codeaurora.org
Subject: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org -> patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org
Switched to a new branch 'test'
d81791b184 target/arm: Implement PMSWINC
512124d018 target/arm: PMU: Set PMCR.N to 4
b748e97306 target/arm: PMU: Add instruction and cycle events
7e46a77f89 target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
82cffef855 target/arm: Add array for supported PMU events, generate PMCEID[01]
d635021d0a target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
3e1b438deb target/arm: Implement PMOVSSET
c13c832988 target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
9c80af4d82 target/arm: Make PMOVSCLR 64 bits wide
8ffbcd3dd8 target/arm: Allow AArch32 access for PMCCFILTR
7dc4ec9715 target/arm: Filter cycle counter based on PMCCFILTR_EL0
f1e40f9fff target/arm: Fix bitmask for PMCCFILTR writes
eb142b42b4 target/arm: Allow EL change hooks to do IO
d37186abdd target/arm: Add pre-EL change hooks
ae12e161da target/arm: Support multiple EL change hooks
9bfa99805d target/arm: Fetch GICv3 state directly from CPUARMState
881bee5e96 target/arm: Mask PMU register writes based on PMCR_EL0.N
890ad6472b target/arm: Reorganize PMCCNTR read, write, sync
922eec023b target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
5b1ce33c0b target/arm: Check PMCNTEN for whether PMCCNTR is enabled
2ecb09ae04 target/arm: A15 PMCEID0 initialization style nit
0f26a1568a target/arm: A53: Initialize PMCEID[01]

=== OUTPUT BEGIN ===
Checking PATCH 1/22: target/arm: A53: Initialize PMCEID[01]...
Checking PATCH 2/22: target/arm: A15 PMCEID0 initialization style nit...
Checking PATCH 3/22: target/arm: Check PMCNTEN for whether PMCCNTR is enabled...
Checking PATCH 4/22: target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0...
Checking PATCH 5/22: target/arm: Reorganize PMCCNTR read, write, sync...
Checking PATCH 6/22: target/arm: Mask PMU register writes based on PMCR_EL0.N...
Checking PATCH 7/22: target/arm: Fetch GICv3 state directly from CPUARMState...
Checking PATCH 8/22: target/arm: Support multiple EL change hooks...
ERROR: space prohibited between function name and open parenthesis '('
#26: FILE: target/arm/cpu.c:62:
+    entry = g_malloc0(sizeof (*entry));

total: 1 errors, 0 warnings, 87 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 9/22: target/arm: Add pre-EL change hooks...
ERROR: space prohibited between function name and open parenthesis '('
#26: FILE: target/arm/cpu.c:62:
+    entry = g_malloc0(sizeof (*entry));

total: 1 errors, 0 warnings, 110 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 10/22: target/arm: Allow EL change hooks to do IO...
Checking PATCH 11/22: target/arm: Fix bitmask for PMCCFILTR writes...
Checking PATCH 12/22: target/arm: Filter cycle counter based on PMCCFILTR_EL0...
Checking PATCH 13/22: target/arm: Allow AArch32 access for PMCCFILTR...
Checking PATCH 14/22: target/arm: Make PMOVSCLR 64 bits wide...
Checking PATCH 15/22: target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions...
Checking PATCH 16/22: target/arm: Implement PMOVSSET...
WARNING: line over 80 characters
#39: FILE: target/arm/helper.c:1417:
+      .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),

total: 0 errors, 1 warnings, 59 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 17/22: target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled...
Checking PATCH 18/22: target/arm: Add array for supported PMU events, generate PMCEID[01]...
Checking PATCH 19/22: target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER...
Checking PATCH 20/22: target/arm: PMU: Add instruction and cycle events...
Checking PATCH 21/22: target/arm: PMU: Set PMCR.N to 4...
Checking PATCH 22/22: target/arm: Implement PMSWINC...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Aaron Lindsay]

My apologies for the below style issues - I've already fixed them up for
v4...

-Aaron

On Mar 16 13:58, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 1521232280-13089-1-git-send-email-alindsay@codeaurora.org
> Subject: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]               patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org -> patchew/1521232280-13089-1-git-send-email-alindsay@codeaurora.org
> Switched to a new branch 'test'
> d81791b184 target/arm: Implement PMSWINC
> 512124d018 target/arm: PMU: Set PMCR.N to 4
> b748e97306 target/arm: PMU: Add instruction and cycle events
> 7e46a77f89 target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
> 82cffef855 target/arm: Add array for supported PMU events, generate PMCEID[01]
> d635021d0a target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
> 3e1b438deb target/arm: Implement PMOVSSET
> c13c832988 target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
> 9c80af4d82 target/arm: Make PMOVSCLR 64 bits wide
> 8ffbcd3dd8 target/arm: Allow AArch32 access for PMCCFILTR
> 7dc4ec9715 target/arm: Filter cycle counter based on PMCCFILTR_EL0
> f1e40f9fff target/arm: Fix bitmask for PMCCFILTR writes
> eb142b42b4 target/arm: Allow EL change hooks to do IO
> d37186abdd target/arm: Add pre-EL change hooks
> ae12e161da target/arm: Support multiple EL change hooks
> 9bfa99805d target/arm: Fetch GICv3 state directly from CPUARMState
> 881bee5e96 target/arm: Mask PMU register writes based on PMCR_EL0.N
> 890ad6472b target/arm: Reorganize PMCCNTR read, write, sync
> 922eec023b target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
> 5b1ce33c0b target/arm: Check PMCNTEN for whether PMCCNTR is enabled
> 2ecb09ae04 target/arm: A15 PMCEID0 initialization style nit
> 0f26a1568a target/arm: A53: Initialize PMCEID[01]
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/22: target/arm: A53: Initialize PMCEID[01]...
> Checking PATCH 2/22: target/arm: A15 PMCEID0 initialization style nit...
> Checking PATCH 3/22: target/arm: Check PMCNTEN for whether PMCCNTR is enabled...
> Checking PATCH 4/22: target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0...
> Checking PATCH 5/22: target/arm: Reorganize PMCCNTR read, write, sync...
> Checking PATCH 6/22: target/arm: Mask PMU register writes based on PMCR_EL0.N...
> Checking PATCH 7/22: target/arm: Fetch GICv3 state directly from CPUARMState...
> Checking PATCH 8/22: target/arm: Support multiple EL change hooks...
> ERROR: space prohibited between function name and open parenthesis '('
> #26: FILE: target/arm/cpu.c:62:
> +    entry = g_malloc0(sizeof (*entry));
> 
> total: 1 errors, 0 warnings, 87 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 9/22: target/arm: Add pre-EL change hooks...
> ERROR: space prohibited between function name and open parenthesis '('
> #26: FILE: target/arm/cpu.c:62:
> +    entry = g_malloc0(sizeof (*entry));
> 
> total: 1 errors, 0 warnings, 110 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 10/22: target/arm: Allow EL change hooks to do IO...
> Checking PATCH 11/22: target/arm: Fix bitmask for PMCCFILTR writes...
> Checking PATCH 12/22: target/arm: Filter cycle counter based on PMCCFILTR_EL0...
> Checking PATCH 13/22: target/arm: Allow AArch32 access for PMCCFILTR...
> Checking PATCH 14/22: target/arm: Make PMOVSCLR 64 bits wide...
> Checking PATCH 15/22: target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions...
> Checking PATCH 16/22: target/arm: Implement PMOVSSET...
> WARNING: line over 80 characters
> #39: FILE: target/arm/helper.c:1417:
> +      .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> 
> total: 0 errors, 1 warnings, 59 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> Checking PATCH 17/22: target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled...
> Checking PATCH 18/22: target/arm: Add array for supported PMU events, generate PMCEID[01]...
> Checking PATCH 19/22: target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER...
> Checking PATCH 20/22: target/arm: PMU: Add instruction and cycle events...
> Checking PATCH 21/22: target/arm: PMU: Set PMCR.N to 4...
> Checking PATCH 22/22: target/arm: Implement PMSWINC...
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3] RFC: target/arm: Send interrupts on PMU counter overflow

[Aaron Lindsay]

On Mar 16 16:30, Aaron Lindsay wrote:
> I aim to eventually add raising interrupts on counter overflow, but that is not
> covered by this patchset. I think I have a reasonable grasp of the mechanics of
> *how* to raise them, but am curious if anyone has thoughts on how to determine
> *when* to raise them - we don't want to call into PMU code every time an
> instruction is executed to check if any instruction counters have overflowed,
> etc. The main candidate I've seen for doing this so far would be to set up a
> QEMUTimer, but I haven't fully explored it. Does that seem plausible? Any
> other/better ideas?

I'm planning to post a full v4 of this patchset soon, pending a few
review fixes, but I figured I'd throw out an early version of a patch to
add interrupts on overflow in case it obviously has major issues that
will need to be addressed.

This patch sets up a QEMUTimer to get a callback when we expect counters
to next overflow and triggers an interrupt at that time.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c    |  11 +++++
 target/arm/cpu.h    |   7 +++
 target/arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 138 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index df27188..9108c6b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -740,6 +740,12 @@ static void arm_cpu_finalizefn(Object *obj)
         QLIST_REMOVE(hook, node);
         g_free(hook);
     }
+#ifndef CONFIG_USER_ONLY
+    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
+        timer_deinit(cpu->pmu_timer);
+        timer_free(cpu->pmu_timer);
+    }
+#endif
 }
 
 static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
@@ -907,6 +913,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
         arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
         arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
+
+#ifndef CONFIG_USER_ONLY
+        cpu->pmu_timer = timer_new(QEMU_CLOCK_VIRTUAL, 1, arm_pmu_timer_cb,
+                cpu);
+#endif
     } else {
         cpu->pmceid0 = 0x00000000;
         cpu->pmceid1 = 0x00000000;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5e6bbd3..bc0867f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -703,6 +703,8 @@ struct ARMCPU {
 
     /* Timers used by the generic (architected) timer */
     QEMUTimer *gt_timer[NUM_GTIMERS];
+    /* Timer used by the PMU */
+    QEMUTimer *pmu_timer;
     /* GPIO outputs for generic timer */
     qemu_irq gt_timer_outputs[NUM_GTIMERS];
     /* GPIO output for GICv3 maintenance interrupt signal */
@@ -934,6 +936,11 @@ void pmu_op_start(CPUARMState *env);
 void pmu_op_finish(CPUARMState *env);
 
 /**
+ * Called when a PMU counter is due to overflow
+ */
+void arm_pmu_timer_cb(void *opaque);
+
+/**
  * Functions to register as EL change hooks for PMU mode filtering
  */
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2147678..abe24dc 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -905,6 +905,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Definitions for the PMU registers */
 #define PMCRN_MASK  0xf800
 #define PMCRN_SHIFT 11
+#define PMCRLC  0x40
 #define PMCRD   0x8
 #define PMCRC   0x4
 #define PMCRP   0x2
@@ -919,6 +920,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMXEVTYPER_MT         0x02000000
 #define PMXEVTYPER_EVTCOUNT   0x000003ff
 
+#define PMEVCNTR_OVERFLOW_MASK ((uint64_t)1 << 31)
+
 #define PMCCFILTR             0xf8000000
 #define PMCCFILTR_M           PMXEVTYPER_M
 #define PMCCFILTR_EL0         (PMCCFILTR | PMCCFILTR_M)
@@ -934,6 +937,11 @@ typedef struct pm_event {
     /* Retrieve the current count of the underlying event. The programmed
      * counters hold a difference from the return value from this function */
     uint64_t (*get_count)(CPUARMState *);
+    /* Return how many nanoseconds it will take (at a minimum) for count events
+     * to occur. A negative value indicates the counter will never overflow, or
+     * that the counter has otherwise arranged for the overflow bit to be set
+     * and the PMU interrupt to be raised on overflow. */
+    int64_t (*ns_per_count)(uint64_t);
 } pm_event;
 
 static bool event_always_supported(CPUARMState *env)
@@ -950,6 +958,11 @@ static uint64_t swinc_get_count(CPUARMState *env)
     return 0;
 }
 
+static int64_t swinc_ns_per(uint64_t ignored)
+{
+    return -1;
+}
+
 /*
  * Return the underlying cycle count for the PMU cycle counters. If we're in
  * usermode, simply return 0.
@@ -965,6 +978,11 @@ static uint64_t cycles_get_count(CPUARMState *env)
 }
 
 #ifndef CONFIG_USER_ONLY
+static int64_t cycles_ns_per(uint64_t cycles)
+{
+    return ARM_CPU_FREQ/NANOSECONDS_PER_SECOND;
+}
+
 static bool instructions_supported(CPUARMState *env)
 {
     return use_icount == 1 /* Precise instruction counting */;
@@ -974,22 +992,30 @@ static uint64_t instructions_get_count(CPUARMState *env)
 {
     return (uint64_t)cpu_get_icount_raw();
 }
+
+static int64_t instructions_ns_per(uint64_t icount)
+{
+    return cpu_icount_to_ns((int64_t)icount);
+}
 #endif
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
     { .number = 0x000, /* SW_INCR */
       .supported = event_always_supported,
-      .get_count = swinc_get_count
+      .get_count = swinc_get_count,
+      .ns_per_count = swinc_ns_per
     },
 #ifndef CONFIG_USER_ONLY
     { .number = 0x008, /* INST_RETIRED, Instruction architecturally executed */
       .supported = instructions_supported,
-      .get_count = instructions_get_count
+      .get_count = instructions_get_count,
+      .ns_per_count = instructions_ns_per
     },
     { .number = 0x011, /* CPU_CYCLES, Cycle */
       .supported = event_always_supported,
-      .get_count = cycles_get_count
+      .get_count = cycles_get_count,
+      .ns_per_count = cycles_ns_per
     },
 #endif
     { .number = SUPPORTED_EVENT_SENTINEL }
@@ -1168,6 +1194,13 @@ static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
     return false;
 }
 
+static void pmu_update_irq(CPUARMState *env)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    qemu_set_irq(cpu->pmu_interrupt, (env->cp15.c9_pmcr & PMCRE) &&
+            (env->cp15.c9_pminten & env->cp15.c9_pmovsr));
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,
@@ -1186,7 +1219,18 @@ void pmccntr_op_start(CPUARMState *env)
             eff_cycles /= 64;
         }
 
-        env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt_delta;
+        uint64_t new_pmccntr = eff_cycles - env->cp15.c15_ccnt_delta;
+
+        unsigned int overflow_bit = (env->cp15.c9_pmcr & PMCRLC) ? 63 : 31;
+        uint64_t overflow_mask = (uint64_t)1 << overflow_bit;
+        if (!(new_pmccntr & overflow_mask) &&
+                (env->cp15.c15_ccnt & overflow_mask)) {
+            env->cp15.c9_pmovsr |= (1 << 31);
+            new_pmccntr &= ~overflow_mask;
+            pmu_update_irq(env);
+        }
+
+        env->cp15.c15_ccnt = new_pmccntr;
     }
     env->cp15.c15_ccnt_delta = cycles;
 }
@@ -1200,13 +1244,25 @@ void pmccntr_op_finish(CPUARMState *env)
 {
     if (arm_ccnt_enabled(env) &&
           !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
-        uint64_t prev_cycles = env->cp15.c15_ccnt_delta;
+#ifndef CONFIG_USER_ONLY
+        uint64_t delta = ((env->cp15.c9_pmcr & PMCRLC) ?
+                UINT64_MAX : UINT32_MAX) - (uint32_t)env->cp15.c15_ccnt;
+        int64_t overflow_in = cycles_ns_per(delta);
 
+        if (overflow_in >= 0)
+        {
+            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                overflow_in;
+            ARMCPU *cpu = arm_env_get_cpu(env);
+            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+        }
+#endif
+
+        uint64_t prev_cycles = env->cp15.c15_ccnt_delta;
         if (env->cp15.c9_pmcr & PMCRD) {
             /* Increment once every 64 processor clock cycles */
             prev_cycles /= 64;
         }
-
         env->cp15.c15_ccnt_delta = prev_cycles - env->cp15.c15_ccnt;
     }
 }
@@ -1220,8 +1276,16 @@ static void pmevcntr_op_start(CPUARMState *env, uint8_t counter)
 
     if (pmu_counter_enabled(env, counter) &&
             !pmu_counter_filtered(env, env->cp15.c14_pmevtyper[counter])) {
-        env->cp15.c14_pmevcntr[counter] =
-            count - env->cp15.c14_pmevcntr_delta[counter];
+
+        uint64_t new_pmevcntr = count - env->cp15.c14_pmevcntr_delta[counter];
+
+        if (!(new_pmevcntr & PMEVCNTR_OVERFLOW_MASK) &&
+                (env->cp15.c14_pmevcntr[counter] & PMEVCNTR_OVERFLOW_MASK)) {
+            env->cp15.c9_pmovsr |= (1 << counter);
+            new_pmevcntr &= ~PMEVCNTR_OVERFLOW_MASK;
+            pmu_update_irq(env);
+        }
+        env->cp15.c14_pmevcntr[counter] = new_pmevcntr;
     }
     env->cp15.c14_pmevcntr_delta[counter] = count;
 }
@@ -1230,6 +1294,21 @@ static void pmevcntr_op_finish(CPUARMState *env, uint8_t counter)
 {
     if (pmu_counter_enabled(env, counter) &&
             !pmu_counter_filtered(env, env->cp15.c14_pmevtyper[counter])) {
+#ifndef CONFIG_USER_ONLY
+        uint16_t event = env->cp15.c14_pmevtyper[counter] & PMXEVTYPER_EVTCOUNT;
+        uint16_t event_idx = supported_event_map[event];
+        uint64_t delta = UINT32_MAX - (uint32_t)env->cp15.c14_pmevcntr[counter];
+        int64_t overflow_in = pm_events[event_idx].ns_per_count(delta);
+
+        if (overflow_in >= 0)
+        {
+            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                overflow_in;
+            ARMCPU *cpu = arm_env_get_cpu(env);
+            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+        }
+#endif
+
         env->cp15.c14_pmevcntr_delta[counter] -=
             env->cp15.c14_pmevcntr[counter];
     }
@@ -1263,6 +1342,18 @@ void pmu_post_el_change(ARMCPU *cpu, void *ignored)
     pmu_op_finish(&cpu->env);
 }
 
+void arm_pmu_timer_cb(void *opaque) {
+    ARMCPU *cpu = opaque;
+
+    /* Update all the counter values based on the current underlying counts,
+     * triggering interrupts to be raised, if necessary. pmu_op_finish() also
+     * has the effect of setting the cpu->pmu_timer to the next earliest time a
+     * counter may expire.
+     */
+    pmu_op_start(&cpu->env);
+    pmu_op_finish(&cpu->env);
+}
+
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
@@ -1300,7 +1391,21 @@ static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
                 /* counter is SW_INCR */
                 (env->cp15.c14_pmevtyper[i] & PMXEVTYPER_EVTCOUNT) == 0x0) {
             pmevcntr_op_start(env, i);
-            env->cp15.c14_pmevcntr[i]++;
+
+            /* Detect if this write causes an overflow since we can't predict
+             * PMSWINC overflows like we can for other events
+             */
+            uint64_t new_pmswinc = env->cp15.c14_pmevcntr[i] + 1;
+
+            if (!(new_pmswinc & PMEVCNTR_OVERFLOW_MASK) &&
+                    (env->cp15.c14_pmevcntr[i] & PMEVCNTR_OVERFLOW_MASK)) {
+                env->cp15.c9_pmovsr |= (1 << i);
+                new_pmswinc &= ~PMEVCNTR_OVERFLOW_MASK;
+                pmu_update_irq(env);
+            }
+
+            env->cp15.c14_pmevcntr[i] = new_pmswinc;
+
             pmevcntr_op_finish(env, i);
         }
     }
@@ -1371,6 +1476,7 @@ static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pmcnten |= value;
+    pmu_update_irq(env);
 }
 
 static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1378,6 +1484,7 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pmcnten &= ~value;
+    pmu_update_irq(env);
 }
 
 static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1385,6 +1492,7 @@ static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pmovsr &= ~value;
+    pmu_update_irq(env);
 }
 
 static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1392,6 +1500,7 @@ static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pmovsr |= value;
+    pmu_update_irq(env);
 }
 
 static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1517,6 +1626,7 @@ static void pmintenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
     /* We have no event counters so only the C bit can be changed */
     value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pminten |= value;
+    pmu_update_irq(env);
 }
 
 static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1524,6 +1634,7 @@ static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= PMU_COUNTER_MASK(env);
     env->cp15.c9_pminten &= ~value;
+    pmu_update_irq(env);
 }
 
 static void vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Peter Maydell]

On 16 March 2018 at 20:30, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> The ARM PMU implementation currently contains a basic cycle counter, but it is
> often useful to gather counts of other events and filter them based on
> execution mode. These patches flesh out the implementations of various PMU
> registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
> represent arbitrary counter types, implement mode filtering, and add
> instruction, cycle, and software increment events.

Hi; sorry it's taken me a while to get to this (I've been focusing
on for-2.12 work). I've now reviewed most of the patches in this
set. My suggestion is that you fix the stuff I've commented on
and send out a v4, and then I can take some of the patches from
the start of that into target-arm.next, and I'll review the tail
end of the set then.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Aaron Lindsay]

On Apr 12 18:32, Peter Maydell wrote:
> On 16 March 2018 at 20:30, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > The ARM PMU implementation currently contains a basic cycle counter, but it is
> > often useful to gather counts of other events and filter them based on
> > execution mode. These patches flesh out the implementations of various PMU
> > registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
> > represent arbitrary counter types, implement mode filtering, and add
> > instruction, cycle, and software increment events.
> 
> Hi; sorry it's taken me a while to get to this (I've been focusing
> on for-2.12 work). I've now reviewed most of the patches in this
> set. My suggestion is that you fix the stuff I've commented on
> and send out a v4, and then I can take some of the patches from
> the start of that into target-arm.next, and I'll review the tail
> end of the set then.

Thanks for the review! I'll see if I can get a v4 out late this week or
early next.

I'll make a note of this when I send it out, but one thing to look out
for in the next version is that I had to reorganize a few things to make
the interrupt-on-overflow functionality work better. I think the changes
are fairly minor and limited to the later patches, with the possible
exception of using two variables per counter (because you have to know
what the previous counter value was in addition to the current value to
know if you've overflowed since your last check).

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.