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

[Hollis Blanchard <hollisb@us.ibm.com>]

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.

> ---
> 
> [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).

-- 
Hollis Blanchard
IBM Linux Technology Center