From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Ehrhardt Date: Wed, 09 Jul 2008 08:25:43 +0000 Subject: Re: [PATCH 2/5] kvmtrace: make cycle calculation architecture aware Message-Id: <48747607.2010509@linux.vnet.ibm.com> List-Id: References: <1215439013-11480-1-git-send-email-ehrhardt@linux.vnet.ibm.com> <1215439013-11480-3-git-send-email-ehrhardt@linux.vnet.ibm.com> <1215445501.11175.4.camel@localhost.localdomain> In-Reply-To: <1215445501.11175.4.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Hollis Blanchard Cc: kvm@vger.kernel.org, avi@qumranet.com, kvm-ppc@vger.kernel.org Hollis Blanchard wrote: > On Mon, 2008-07-07 at 15:56 +0200, ehrhardt@linux.vnet.ibm.com wrote: > =20 >> From: Christian Ehrhardt >> >> The current implementation of kvmtrace uses always a 64 bit cycle variab= le, >> 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 >> =20 > > Just one comment below... > > BTW, because this breaks the ia64 and s390 builds, it might be nice to > CC the appropriate list/maintainer directly. > > =20 you're right - I'll add dummy stubs for ia64&s390 using the classic=20 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 32bi= t (tbl) >> + * on 32bit ppc. Since we have a 64bit counter for that we provide it h= ere 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" >> + : "=3Dr" (ruval), "=3Dr" (rlval), "=3Dr" (ruval2) >> + ); >> + >> + return (((__u64)ruval) << 32) | rlval; >> +} >> =20 > > You should use get_tb() here (see asm-powerpc/time.h). > =20 yep does the same, I didn't see it and coded the asm from the processor=20 manual hint at the time base register. Anyway - I'll change the code to get_tb() - thanks for the hint. --=20 Gr=FCsse / regards,=20 Christian Ehrhardt IBM Linux Technology Center, Open Virtualization