Re: [PATCH 2/5] kvmtrace: make cycle calculation architecture aware

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

Hollis Blanchard wrote:
> On Mon, 2008-07-07 at 15:56 +0200, ehrhardt@linux.vnet.ibm.com wrote:
>   
>> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>
>> The current implementation of kvmtrace uses always a 64 bit cycle variable,
>> but get_cycles() which is used to fill it is "unsigned long" which might be 32
>> bit.
>> This reduces the accuracy e.g. on embedded powerpc since we would have a 64bit
>> value but get_cycle() only returns the low 32 bit.
>> To solve that this patch introduces kvm_arch_trace_cycles() which allows us
>> to make this calculation architecture aware. That way every architecture can
>> insert whatever fits best for their "kvmtrace cycle counter".
>>
>> Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>     
>
> Just one comment below...
>
> BTW, because this breaks the ia64 and s390 builds, it might be nice to
> CC the appropriate list/maintainer directly.
>
>   
you're right - I'll add dummy stubs for ia64&s390 using the classic 
get_cycle() call and resubmit the series.
>> ---
>>
>> [diffstat]
>>  arch/powerpc/kvm/powerpc.c |   25 +++++++++++++++++++++++++
>>  arch/x86/kvm/x86.c         |    5 +++++
>>  include/linux/kvm_host.h   |    2 ++
>>  virt/kvm/kvm_trace.c       |    2 +-
>>  4 files changed, 33 insertions(+), 1 deletion(-)
>>
>> [diff]
>>
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -521,6 +521,31 @@
>>  	return r;
>>  }
>>
>> +/*
>> + * We need a 64 bit value here and the default get_cycles has only 32bit (tbl)
>> + * on 32bit ppc. Since we have a 64bit counter for that we provide it here in
>> + * full resolution for the trace records.
>> +*/
>> +__u64 kvm_arch_trace_cycles()
>> +{
>> +	unsigned long ruval;
>> +	unsigned long ruval2;
>> +	unsigned long rlval;
>> +
>> +	/* get a consistant pair of upper/lower timebase (no wrap occured) */
>> +	asm volatile(
>> +		"loop:\n"
>> +		  "mftbu %0\n"
>> +		  "mftbl %1\n"
>> +		  "mftbu %2\n"
>> +		  "cmpw %0, %2\n"
>> +		  "bne loop"
>> +		: "=r" (ruval), "=r" (rlval), "=r" (ruval2)
>> +	);
>> +
>> +	return (((__u64)ruval) << 32) | rlval;
>> +}
>>     
>
> You should use get_tb() here (see asm-powerpc/time.h).
>   
yep does the same, I didn't see it and coded the asm from the processor 
manual hint at the time base register.
Anyway - I'll change the code to get_tb() - thanks for the hint.

-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization