All of lore.kernel.org
 help / color / mirror / Atom feed
From: Aaron Lindsay <alindsay@codeaurora.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm <qemu-arm@nongnu.org>,
	Alistair Francis <alistair.francis@xilinx.com>,
	Wei Huang <wei@redhat.com>,
	Peter Crosthwaite <crosthwaite.peter@gmail.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Michael Spradling <mspradli@codeaurora.org>,
	Digant Desai <digantd@codeaurora.org>
Subject: Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0
Date: Thu, 12 Apr 2018 13:36:25 -0400	[thread overview]
Message-ID: <20180412173625.GK24561@codeaurora.org> (raw)
In-Reply-To: <CAFEAcA_UP-DgPZPL8-sMAs9TFHiUrfya8LAxAnO2U_WJAMH6yg@mail.gmail.com>

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.

  reply	other threads:[~2018-04-12 17:36 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-16 20:30 [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3 Aaron Lindsay
2018-03-16 20:30 ` [Qemu-devel] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01] Aaron Lindsay
2018-03-18 22:35   ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2018-03-18 22:57     ` Philippe Mathieu-Daudé
2018-03-19 20:35     ` Aaron Lindsay
2018-03-20  1:03       ` Philippe Mathieu-Daudé
2018-03-21 15:17         ` Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 02/22] target/arm: A15 PMCEID0 initialization style nit Aaron Lindsay
2018-04-12 16:07   ` Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 03/22] target/arm: Check PMCNTEN for whether PMCCNTR is enabled Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 04/22] target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0 Aaron Lindsay
2018-04-12 16:10   ` Peter Maydell
2018-04-12 16:56     ` Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync Aaron Lindsay
2018-04-12 16:18   ` Peter Maydell
2018-04-13 13:51     ` Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 06/22] target/arm: Mask PMU register writes based on PMCR_EL0.N Aaron Lindsay
2018-04-12 16:24   ` Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 07/22] target/arm: Fetch GICv3 state directly from CPUARMState Aaron Lindsay
2018-04-12 16:28   ` Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 08/22] target/arm: Support multiple EL change hooks Aaron Lindsay
2018-03-18 22:41   ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2018-03-20 20:45     ` Aaron Lindsay
2018-03-20 21:01       ` Philippe Mathieu-Daudé
2018-04-12 16:36   ` [Qemu-devel] " Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL " Aaron Lindsay
2018-04-12 16:49   ` Peter Maydell
2018-04-12 17:01     ` Aaron Lindsay
2018-04-12 17:21       ` Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 10/22] target/arm: Allow EL change hooks to do IO Aaron Lindsay
2018-04-12 16:53   ` Peter Maydell
2018-04-12 17:08     ` Aaron Lindsay
2018-04-12 17:21       ` Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 11/22] target/arm: Fix bitmask for PMCCFILTR writes Aaron Lindsay
2018-04-12 16:41   ` Peter Maydell
2018-04-13 18:15     ` Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0 Aaron Lindsay
2018-04-12 17:15   ` Peter Maydell
2018-04-12 17:36     ` Aaron Lindsay [this message]
2018-04-17 15:21       ` Aaron Lindsay
2018-04-17 15:37         ` Peter Maydell
2018-04-17 20:03           ` Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 13/22] target/arm: Allow AArch32 access for PMCCFILTR Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide Aaron Lindsay
2018-03-18 23:14   ` Philippe Mathieu-Daudé
2018-03-19 15:24     ` Aaron Lindsay
2018-03-19 15:31       ` Peter Maydell
2018-03-20  1:01         ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions Aaron Lindsay
2018-03-18 22:42   ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2018-04-12 17:17   ` [Qemu-devel] " Peter Maydell
2018-04-17 14:23     ` Aaron Lindsay
2018-04-17 15:00       ` Peter Maydell
2018-04-24 20:35         ` Aaron Lindsay
2018-05-17 19:31         ` Aaron Lindsay
2018-05-31 14:18           ` Peter Maydell
2018-05-31 20:39             ` Aaron Lindsay
2018-06-01  8:57               ` Peter Maydell
2018-06-01 15:34                 ` Aaron Lindsay
2018-06-01 15:59                   ` Peter Maydell
2018-06-01 19:12                     ` Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 16/22] target/arm: Implement PMOVSSET Aaron Lindsay
2018-04-12 17:28   ` Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 17/22] target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled Aaron Lindsay
2018-04-12 17:29   ` Peter Maydell
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 18/22] target/arm: Add array for supported PMU events, generate PMCEID[01] Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 19/22] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events Aaron Lindsay
2018-03-18 22:43   ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2018-03-18 22:48   ` Philippe Mathieu-Daudé
2018-03-19 17:36     ` Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 21/22] target/arm: PMU: Set PMCR.N to 4 Aaron Lindsay
2018-03-16 20:31 ` [Qemu-devel] [PATCH v3 22/22] target/arm: Implement PMSWINC Aaron Lindsay
2018-03-16 20:58 ` [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3 no-reply
2018-03-17  0:01   ` Aaron Lindsay
2018-04-12 17:17 ` [Qemu-devel] [PATCH v3] RFC: target/arm: Send interrupts on PMU counter overflow Aaron Lindsay
2018-04-12 17:32 ` [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3 Peter Maydell
2018-04-12 19:34   ` Aaron Lindsay

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180412173625.GK24561@codeaurora.org \
    --to=alindsay@codeaurora.org \
    --cc=alistair.francis@xilinx.com \
    --cc=crosthwaite.peter@gmail.com \
    --cc=digantd@codeaurora.org \
    --cc=mspradli@codeaurora.org \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=wei@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.