Re: [Qemu-devel] [kvm-unit-tests PATCHv5 2/3] arm: pmu: Check cycle count increases

[Andrew Jones <drjones@redhat.com>]

On Mon, Oct 26, 2015 at 04:58:43PM +0100, Andrew Jones wrote:
> On Mon, Oct 26, 2015 at 11:38:49AM -0400, Christopher Covington wrote:
> > Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
> > even for the smallest delta of two subsequent reads.
> > 
> > Signed-off-by: Christopher Covington <cov@codeaurora.org>
> > ---
> >  arm/pmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 42d0ee1..c44d708 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -14,6 +14,8 @@
> >   */
> >  #include "libcflat.h"
> >  
> > +#define NR_SAMPLES 10
> > +
> >  #if defined(__arm__)
> >  static inline uint32_t get_pmcr(void)
> >  {
> > @@ -22,6 +24,25 @@ static inline uint32_t get_pmcr(void)
> >  	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
> >  	return ret;
> >  }
> > +
> > +static inline void set_pmcr(uint32_t pmcr)
> > +{
> > +	asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (pmcr));
> > +}
> > +
> > +/*
> > + * While PMCCNTR can be accessed as a 64 bit coprocessor register, returning 64
> > + * bits doesn't seem worth the trouble when differential usage of the result is
> > + * expected (with differences that can easily fit in 32 bits). So just return
> > + * the lower 32 bits of the cycle count in AArch32.
> > + */
> > +static inline unsigned long get_pmccntr(void)
> > +{
> > +	unsigned long cycles;
> > +
> > +	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
> > +	return cycles;
> > +}
> >  #elif defined(__aarch64__)
> >  static inline uint32_t get_pmcr(void)
> >  {
> > @@ -30,6 +51,19 @@ static inline uint32_t get_pmcr(void)
> >  	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
> >  	return ret;
> >  }
> > +
> > +static inline void set_pmcr(uint32_t pmcr)
> > +{
> > +	asm volatile("msr pmcr_el0, %0" : : "r" (pmcr));
> > +}
> > +
> > +static inline unsigned long get_pmccntr(void)
> > +{
> > +	unsigned long cycles;
> > +
> > +	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
> > +	return cycles;
> > +}
> >  #endif
> >  
> >  struct pmu_data {
> > @@ -72,11 +106,37 @@ static bool check_pmcr(void)
> >  	return pmu.implementer != 0;
> >  }
> >  
> > +/*
> > + * Ensure that the cycle counter progresses between back-to-back reads.
> > + */
> > +static bool check_cycles_increase(void)
> > +{
> > +	struct pmu_data pmu = { {0} };
> 
> One set of {} is enough, and looks better.

Ah, just read your cover letter and now see that this was done on purpose.
So your compiler complains about {0}? Is there a problem besides the
warning? If not, then I'm still a bit inclined to keep the code neat. The
warnings will go away with compiler updates.

Thanks,
drew
> 
> > +
> > +	pmu.enable = 1;
> > +	set_pmcr(pmu.pmcr_el0);
> > +
> > +	for (int i = 0; i < NR_SAMPLES; i++) {
> > +		unsigned long a, b;
> > +
> > +		a = get_pmccntr();
> > +		b = get_pmccntr();
> > +
> > +		if (a >= b) {
> > +			printf("Read %ld then %ld.\n", a, b);
> > +			return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  int main(void)
> >  {
> >  	report_prefix_push("pmu");
> >  
> >  	report("Control register", check_pmcr());
> > +	report("Monotonically increasing cycle count", check_cycles_increase());
> >  
> >  	return report_summary();
> >  }
> > -- 
> > Qualcomm Innovation Center, Inc.
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> > 
> >
> 
> Otherwise
> Reviewed-by: Andrew Jones <drjones@redhat.com>