[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5263e77a-b987-c955-c328-040a11543c94@redhat.com>
Date: Mon, 21 Nov 2016 16:49:20 -0600
From: Wei Huang <wei@...hat.com>
To: Christopher Covington <cov@...eaurora.org>,
kvmarm@...ts.cs.columbia.edu
Cc: linux-arm-kernel@...ts.infradead.org, shannon.zhao@...aro.org,
kvm@...r.kernel.org, marc.zyngier@....com,
christoffer.dall@...aro.org, drjones@...hat.com,
will.deacon@....com, mark.rutland@....com, catalin.marinas@....com,
linux-kernel@...r.kernel.org
Subject: Re: [kvm-unit-tests PATCH v10 3/3] arm: pmu: Add CPI checking
On 11/21/2016 03:40 PM, Christopher Covington wrote:
> Hi Wei,
>
> On 11/21/2016 03:24 PM, Wei Huang wrote:
>> From: Christopher Covington <cov@...eaurora.org>
>
> I really appreciate your work on these patches. If for any or all of these
> you have more lines added/modified than me (or using any other better
> metric), please make sure to change the author to be you with
> `git commit --amend --reset-author` or equivalent.
Sure, I will if needed. Regarding your comments below, I will fix the
patch series after Drew's comments, if any.
>
>> Calculate the numbers of cycles per instruction (CPI) implied by ARM
>> PMU cycle counter values. The code includes a strict checking facility
>> intended for the -icount option in TCG mode in the configuration file.
>>
>> Signed-off-by: Christopher Covington <cov@...eaurora.org>
>> Signed-off-by: Wei Huang <wei@...hat.com>
>> ---
>> arm/pmu.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> arm/unittests.cfg | 14 +++++++
>> 2 files changed, 132 insertions(+), 1 deletion(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index 176b070..129ef1e 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -104,6 +104,25 @@ static inline uint32_t id_dfr0_read(void)
>> asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
>> return val;
>> }
>> +
>> +/*
>> + * Extra instructions inserted by the compiler would be difficult to compensate
>> + * for, so hand assemble everything between, and including, the PMCR accesses
>> + * to start and stop counting. Total cycles = isb + mcr + 2*loop = 2 + 2*loop.
^^^^^^^^^^^^
I will change the comment above to "Total instrs".
>> + */
>> +static inline void precise_cycles_loop(int loop, uint32_t pmcr)
>
> Nit: I would call this precise_instrs_loop. How many cycles it takes is
> IMPLEMENTATION DEFINED.
You are right. The cycle indeed depends on the design. Will fix.
>
>> +{
>> + asm volatile(
>> + " mcr p15, 0, %[pmcr], c9, c12, 0\n"
>> + " isb\n"
>> + "1: subs %[loop], %[loop], #1\n"
>> + " bgt 1b\n"
>
> Is there any chance we might need an isb here, to prevent the stop from happening
> before or during the loop? Where ISBs are required, the Linux best practice is to
In theory, I think this can happen when mcr is executed before all loop
instructions completed, causing pmccntr_read() to miss some cycles. But
QEMU TCG mode doesn't support out-order-execution. So the test
condition, "cpi > 0 && cycles != i * cpi", will never be TRUE. Because
cpi==0 in KVM, this same test condition won't be TRUE under KVM mode either.
> diligently comment why they are needed. Perhaps it would be a good habit to
> carry over into kvm-unit-tests.
Agreed. Most isb() instructions were added following CP15 writes (not
all CP15 writes, but at limited locations). We tried to follow what
Linux kernel does in perf_event.c. If you feel that any isb() place
needs special comment, I will be more than happy to add it.
<snip>
Powered by blists - more mailing lists