[PATCH] target/arm: Configure number of pmu counters

[PATCH] target/arm: Configure number of pmu counters

[Sai Pavan Boddu]

Default the pmu counters to 4 and configure it a 6 for a53 cores.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 target/arm/cpu.c    | 3 +++
 target/arm/cpu.h    | 3 +++
 target/arm/cpu64.c  | 1 +
 target/arm/helper.c | 2 +-
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 6b382fc..805a692 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1051,6 +1051,9 @@ static void arm_cpu_initfn(Object *obj)
     cpu->psci_version = 1; /* By default assume PSCI v0.1 */
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
 
+    /* set number of pmu counters to 4 */
+    cpu->pmcrn = 4;
+
     if (tcg_enabled()) {
         cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
     }
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ac857bd..3b47ba8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -879,6 +879,9 @@ struct ARMCPU {
      */
     int32_t core_count;
 
+    /* Number of pmu counters */
+    uint8_t pmcrn;
+
     /* The instance init functions for implementation-specific subclasses
      * set these fields to specify the implementation-dependent values of
      * various constant registers and reset values of non-constant
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index dd69618..76c879a 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -190,6 +190,7 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->gic_vpribits = 5;
     cpu->gic_vprebits = 5;
     define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
+    cpu->pmcrn = 6;
 }
 
 static void aarch64_a72_initfn(Object *obj)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 44d6666..4afbefb 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6534,7 +6534,7 @@ static void define_pmu_regs(ARMCPU *cpu)
      * field as main ID register, and we implement four counters in
      * addition to the cycle count register.
      */
-    unsigned int i, pmcrn = 4;
+    unsigned int i, pmcrn = cpu->pmcrn;
     ARMCPRegInfo pmcr = {
         .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
         .access = PL0_RW,
-- 
2.7.4

Re: [PATCH] target/arm: Configure number of pmu counters

[Edgar E. Iglesias]

On Mon, Aug 31, 2020 at 05:18:42PM +0530, Sai Pavan Boddu wrote:
> Default the pmu counters to 4 and configure it a 6 for a53 cores.

Your commit message looks like it needs a little rewording.

I would probably split this into two patches, one that adds pmcrn
and another that modifies the A53.

> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  target/arm/cpu.c    | 3 +++
>  target/arm/cpu.h    | 3 +++
>  target/arm/cpu64.c  | 1 +
>  target/arm/helper.c | 2 +-
>  4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 6b382fc..805a692 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1051,6 +1051,9 @@ static void arm_cpu_initfn(Object *obj)
>      cpu->psci_version = 1; /* By default assume PSCI v0.1 */
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
>  
> +    /* set number of pmu counters to 4 */

/* Set default number of PMU counters */

That way if the default changes you don't need to edit both code and comment.

With those changes:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> +    cpu->pmcrn = 4;
> +
>      if (tcg_enabled()) {
>          cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
>      }
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ac857bd..3b47ba8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -879,6 +879,9 @@ struct ARMCPU {
>       */
>      int32_t core_count;
>  
> +    /* Number of pmu counters */
> +    uint8_t pmcrn;
> +
>      /* The instance init functions for implementation-specific subclasses
>       * set these fields to specify the implementation-dependent values of
>       * various constant registers and reset values of non-constant
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index dd69618..76c879a 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -190,6 +190,7 @@ static void aarch64_a53_initfn(Object *obj)
>      cpu->gic_vpribits = 5;
>      cpu->gic_vprebits = 5;
>      define_arm_cp_regs(cpu, cortex_a72_a57_a53_cp_reginfo);
> +    cpu->pmcrn = 6;
>  }
>  
>  static void aarch64_a72_initfn(Object *obj)
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 44d6666..4afbefb 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6534,7 +6534,7 @@ static void define_pmu_regs(ARMCPU *cpu)
>       * field as main ID register, and we implement four counters in
>       * addition to the cycle count register.
>       */
> -    unsigned int i, pmcrn = 4;
> +    unsigned int i, pmcrn = cpu->pmcrn;
>      ARMCPRegInfo pmcr = {
>          .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
>          .access = PL0_RW,
> -- 
> 2.7.4
> 
> 

Re: [PATCH] target/arm: Configure number of pmu counters

[Peter Maydell]

On Mon, 31 Aug 2020 at 12:44, Sai Pavan Boddu
<sai.pavan.boddu@xilinx.com> wrote:
>
> Default the pmu counters to 4 and configure it a 6 for a53 cores.
>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  target/arm/cpu.c    | 3 +++
>  target/arm/cpu.h    | 3 +++
>  target/arm/cpu64.c  | 1 +
>  target/arm/helper.c | 2 +-
>  4 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 6b382fc..805a692 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1051,6 +1051,9 @@ static void arm_cpu_initfn(Object *obj)
>      cpu->psci_version = 1; /* By default assume PSCI v0.1 */
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
>
> +    /* set number of pmu counters to 4 */
> +    cpu->pmcrn = 4;
> +
>      if (tcg_enabled()) {
>          cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
>      }
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ac857bd..3b47ba8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -879,6 +879,9 @@ struct ARMCPU {
>       */
>      int32_t core_count;
>
> +    /* Number of pmu counters */
> +    uint8_t pmcrn;
> +
>      /* The instance init functions for implementation-specific subclasses
>       * set these fields to specify the implementation-dependent values of
>       * various constant registers and reset values of non-constant

Rather than doing this, I think the better approach would be to
switch to treating PMCR_EL0 as an ID register in the same way
we do for other varies-per-CPU ID registers:

 * new field uint64_t pmcr_el0 in the ARMISARegisters sub-struct of ARMCPU
 * each CPU's initfn needs to set this (sorry, this is going to mean
   a fair amount of digging through TRMs to find the right values)
 * define_pmu_regs() just uses the isar.pmcr_el0 field as the resetvalue
   for PMCR_EL0
 * define_pmu_regs() needs to use pmu_num_counters() to get the number
   of counters for its loops

Side note: looking at the code I see we don't implement the effect
that MDCR_EL2.HPMN has on this register... (and probably we don't
in general get the EL2 handling of the PMU right in other places too).

thanks
-- PMM

RE: [PATCH] target/arm: Configure number of pmu counters

[Sai Pavan Boddu]

Hi Peter,

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Tuesday, September 1, 2020 6:18 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>; Philippe Mathieu-
> Daudé <philmd@redhat.com>; Andrew Jones <drjones@redhat.com>; Paolo
> Bonzini <pbonzini@redhat.com>; Alex Bennée <alex.bennee@linaro.org>;
> Eric Auger <eric.auger@redhat.com>; Edgar Iglesias <edgari@xilinx.com>;
> qemu-arm <qemu-arm@nongnu.org>; QEMU Developers <qemu-
> devel@nongnu.org>
> Subject: Re: [PATCH] target/arm: Configure number of pmu counters
> 
> On Mon, 31 Aug 2020 at 12:44, Sai Pavan Boddu
> <sai.pavan.boddu@xilinx.com> wrote:
> >
> > Default the pmu counters to 4 and configure it a 6 for a53 cores.
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  target/arm/cpu.c    | 3 +++
> >  target/arm/cpu.h    | 3 +++
> >  target/arm/cpu64.c  | 1 +
> >  target/arm/helper.c | 2 +-
> >  4 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c index
> > 6b382fc..805a692 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1051,6 +1051,9 @@ static void arm_cpu_initfn(Object *obj)
> >      cpu->psci_version = 1; /* By default assume PSCI v0.1 */
> >      cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
> >
> > +    /* set number of pmu counters to 4 */
> > +    cpu->pmcrn = 4;
> > +
> >      if (tcg_enabled()) {
> >          cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
> >      }
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h index
> > ac857bd..3b47ba8 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -879,6 +879,9 @@ struct ARMCPU {
> >       */
> >      int32_t core_count;
> >
> > +    /* Number of pmu counters */
> > +    uint8_t pmcrn;
> > +
> >      /* The instance init functions for implementation-specific subclasses
> >       * set these fields to specify the implementation-dependent values of
> >       * various constant registers and reset values of non-constant
> 
> Rather than doing this, I think the better approach would be to switch to
> treating PMCR_EL0 as an ID register in the same way we do for other varies-
> per-CPU ID registers:
> 
>  * new field uint64_t pmcr_el0 in the ARMISARegisters sub-struct of ARMCPU
>  * each CPU's initfn needs to set this (sorry, this is going to mean
>    a fair amount of digging through TRMs to find the right values)
>  * define_pmu_regs() just uses the isar.pmcr_el0 field as the resetvalue
>    for PMCR_EL0
>  * define_pmu_regs() needs to use pmu_num_counters() to get the number
>    of counters for its loops
> 
> Side note: looking at the code I see we don't implement the effect that
> MDCR_EL2.HPMN has on this register... (and probably we don't in general
> get the EL2 handling of the PMU right in other places too).
[Sai Pavan Boddu] Sure, I will get back on this. I need to understand more.

Thanks,
Sai Pavan
> 
> thanks
> -- PMM