[PATCH 0/5] kvmtrace: add powerpc support for KVM_TRACE

[PATCH 0/5] kvmtrace: add powerpc support for KVM_TRACE

[ehrhardt]

From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>

This series contains the revised patch set Jerone/Hollis posted last week
as well as the extension to use the KVM_TRACE infrastructure to trace
the embedded powerpc instruction emulation.

The series contains five patches (2 generic / 3 powerpc code):
Subject: [PATCH 1/5] kvmtrace: Remove use of bit fields in kvm trace structure v3
	Introducing a endian save way to store the flags event,tcs, num_data 
Subject: [PATCH 2/5] kvmtrace: make cycle calculation architecture aware
	let each architecture implement how to fill the trace cycle 64 bit field
Subject: [PATCH 3/5] kvmppc: kvmtrace: enable KVM_TRACE building for powerpc
	enable KVM_TRACE for powerpc in Kconfig/Makefile
Subject: [PATCH 4/5] kvmppc: kvmtrace: adds trace points for ppc tlb activity
	use KVM_TRACE to track tlb/shadow tlb activity on embedded powerpc
Subject: [PATCH 5/5] kvmppc: kvmtrace: trace powerpc instruction emulation
	use KVM_TRACE to track instruction emulation overhead

Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---

[diffstat]
 arch/powerpc/kvm/emulate.c   |    4 ++++
 b/arch/powerpc/kvm/44x_tlb.c |   15 ++++++++++++++-
 b/arch/powerpc/kvm/Kconfig   |   11 +++++++++++
 b/arch/powerpc/kvm/Makefile  |    6 ++++--
 b/arch/powerpc/kvm/emulate.c |    4 ++++
 b/arch/powerpc/kvm/powerpc.c |   25 +++++++++++++++++++++++++
 b/arch/x86/kvm/x86.c         |    5 +++++
 b/include/linux/kvm.h        |   17 ++++++++++++++---
 b/include/linux/kvm_host.h   |    2 ++
 b/virt/kvm/kvm_trace.c       |   19 ++++++++++---------
 include/linux/kvm.h          |    4 ++++
 virt/kvm/kvm_trace.c         |    2 +-
 12 files changed, 98 insertions(+), 16 deletions(-)

[PATCH 1/5] kvmtrace: Remove use of bit fields in kvm trace structure v3

[ehrhardt]

[PATCH 1/5] kvmtrace: Remove use of bit fields in kvm trace structure v3

From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
From: Jerone Young <jyoung5@us.ibm.com>

This patch fixes kvmtrace use on big endian systems. When using bit fields the
compiler will lay data out in the wrong order expected when laid down into a
file.
This fixes it by using one variable instead of using bit fields.

Updates in v3:
	- fixed macro definition bug in v2
	- ensured in macro operator order
	- fixed whitespace/indent issues
	- removed superfluous initialization

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---

[diffstat]
 include/linux/kvm.h  |   17 ++++++++++++++---
 virt/kvm/kvm_trace.c |   19 ++++++++++---------
 2 files changed, 24 insertions(+), 12 deletions(-)

[diff]

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -311,9 +311,13 @@
 
 /* This structure represents a single trace buffer record. */
 struct kvm_trace_rec {
-	__u32 event:28;
-	__u32 extra_u32:3;
-	__u32 cycle_in:1;
+	/* variable rec_val
+	 * is split into:
+	 * bits 0 - 27  -> event id
+	 * bits 28 -30  -> number of extra data args of size u32
+	 * bits 31      -> binary indicator for if tsc is in record
+	 */
+	__u32 rec_val;
 	__u32 pid;
 	__u32 vcpu_id;
 	union {
@@ -326,6 +330,13 @@
 		} nocycle;
 	} u;
 } __attribute__((packed));
+
+#define TRACE_REC_EVENT_ID(val) \
+		(0x0fffffff & (val))
+#define TRACE_REC_NUM_DATA_ARGS(val) \
+		(0x70000000 & ((val) << 28))
+#define TRACE_REC_TCS(val) \
+		(0x80000000 & ((val) << 31))
 
 #define KVMIO 0xAE
 
diff --git a/virt/kvm/kvm_trace.c b/virt/kvm/kvm_trace.c
--- a/virt/kvm/kvm_trace.c
+++ b/virt/kvm/kvm_trace.c
@@ -54,12 +54,13 @@
 	struct kvm_trace *kt = kvm_trace;
 	struct kvm_trace_rec rec;
 	struct kvm_vcpu *vcpu;
-	int    i, extra, size;
+	int    i, size;
+	u32    extra;
 
 	if (unlikely(kt->trace_state != KVM_TRACE_STATE_RUNNING))
 		return;
 
-	rec.event	= va_arg(*args, u32);
+	rec.rec_val	= TRACE_REC_EVENT_ID(va_arg(*args, u32));
 	vcpu		= va_arg(*args, struct kvm_vcpu *);
 	rec.pid		= current->tgid;
 	rec.vcpu_id	= vcpu->vcpu_id;
@@ -67,21 +68,21 @@
 	extra   	= va_arg(*args, u32);
 	WARN_ON(!(extra <= KVM_TRC_EXTRA_MAX));
 	extra 		= min_t(u32, extra, KVM_TRC_EXTRA_MAX);
-	rec.extra_u32   = extra;
 
-	rec.cycle_in 	= p->cycle_in;
-
-	if (rec.cycle_in) {
+	rec.rec_val |= TRACE_REC_TCS(p->cycle_in)
+			| TRACE_REC_NUM_DATA_ARGS(extra);
+	
+	if (p->cycle_in) {
 		rec.u.cycle.cycle_u64 = get_cycles();
 
-		for (i = 0; i < rec.extra_u32; i++)
+		for (i = 0; i < extra; i++)
 			rec.u.cycle.extra_u32[i] = va_arg(*args, u32);
 	} else {
-		for (i = 0; i < rec.extra_u32; i++)
+		for (i = 0; i < extra; i++)
 			rec.u.nocycle.extra_u32[i] = va_arg(*args, u32);
 	}
 
-	size = calc_rec_size(rec.cycle_in, rec.extra_u32 * sizeof(u32));
+	size = calc_rec_size(p->cycle_in, extra * sizeof(u32));
 	relay_write(kt->rchan, &rec, size);
 }
 

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

[ehrhardt]

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>
---

[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;
+}
+
 int kvm_arch_init(void *opaque)
 {
 	return 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2458,6 +2458,11 @@
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_pio_string);
 
+__u64 kvm_arch_trace_cycles()
+{
+	return get_cycles();
+}
+
 int kvm_arch_init(void *opaque)
 {
 	int r;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -278,6 +278,8 @@
 int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu);
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
 
+__u64 kvm_arch_trace_cycles(void);
+
 static inline void kvm_guest_enter(void)
 {
 	account_system_vtime(current);
diff --git a/virt/kvm/kvm_trace.c b/virt/kvm/kvm_trace.c
--- a/virt/kvm/kvm_trace.c
+++ b/virt/kvm/kvm_trace.c
@@ -74,7 +74,7 @@
 				| TRACE_REC_NUM_DATA_ARGS(extra);
 
 	if (p->cycle_in) {
-		rec.u.cycle.cycle_u64 = get_cycles();
+		rec.u.cycle.cycle_u64 = kvm_arch_trace_cycles();
 
 		for (i = 0; i < extra; i++)
 			rec.u.cycle.extra_u32[i] = va_arg(*args, u32);

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

[Hollis Blanchard]

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

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

[Christian Ehrhardt]

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

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

[Hollis Blanchard]

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>

"cycles" is a very poor name, because that's not really what we're
talking about at all. (Also, that function name made me wonder what a
"trace cycle" is. :)

I would strongly prefer using "timestamp" instead. It would be nice if
we could rename the data structure too, but I'd settle for only properly
naming the new architecture function hook.

In fact, if we want to be rigorous about it, it should really be
something like "nanoseconds" instead, so that userspace wouldn't need to
perform awkward conversions of "cycles" or "timebase ticks" to real
time. It looks like getnstimeofday() would do the trick, and that way we
wouldn't need an arch-specific hook at all.

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Christian Ehrhardt]

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>
>>     
>
> "cycles" is a very poor name, because that's not really what we're
> talking about at all. (Also, that function name made me wonder what a
> "trace cycle" is. :)
>
> I would strongly prefer using "timestamp" instead. It would be nice if
> we could rename the data structure too, but I'd settle for only properly
> naming the new architecture function hook.
>   
> In fact, if we want to be rigorous about it, it should really be
> something like "nanoseconds" instead, so that userspace wouldn't need to
> perform awkward conversions of "cycles" or "timebase ticks" to real
> time. It looks like getnstimeofday() would do the trick, and that way we
> wouldn't need an arch-specific hook at all
I agree that the naming is poor. I also wondered about it and just 
continued it to the kvm_arch function.
I'll change the naming - timestamp would be fine, but only if it is 
really time and not a cycle counter.
I experimented with the tv_nsec portion and the classic current_time and 
accuracy there was just far to low compared to the time base register 
data (5 to 20 instructions until tv.nssec portion changed).

I checked the getnstimeofday function you mentioned and compared 
accuracy again and yes that is exactly what I would like to use:
here a ittle example
263527418457 (+   36696)    ns=0x1c43286a
263527740677 (+  322220)   ns=0x1c4a880c (+ 75FA2 = 483234)
263527779757 (+   39080)    ns=0x1c4b6cae (+ E4A2   =  58530)
While the unit is not the same (explains the difference in the same line 
e.g. 39080 vs. 58530) the accuracy is the same.
this can be seen due to the fact that it changes by the same ratio all 
over the file.
For the example I had above
cycle = 322220/39080 = 8.25
nsec = 483234/58530 = 8.25

That leads me to the point where I completely agree with Hollis that the 
naming should be timestamp as well as that the function should rely on 
getnstimeofday() instead of get_cycles() and that way would remove the 
need kvm_arch_ function.

So the question that is left before changing that is, if the original 
author had something special in mind chosing cycles here.
I added Eric on CC for that.

I wait with my resubmission of the patch series until all architectures 
agree *hope* on using getnstimeofday() - after an ack from all sides I 
would revise my patch series and submit that changes alltogether.

-- 

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

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

[Hollis Blanchard]

On Wed, 2008-07-09 at 11:17 +0200, Christian Ehrhardt wrote:
> 
> So the question that is left before changing that is, if the original 
> author had something special in mind chosing cycles here.
> I added Eric on CC for that.
> 
> I wait with my resubmission of the patch series until all
> architectures agree *hope* on using getnstimeofday() - after an ack
> from all sides I would revise my patch series and submit that changes
> alltogether.

I got an email bounce from Eric the last time I tried to email him, so
I'm not sure he's still with Intel.

However, I don't think he had any special intention; I think he was just
porting xentrace to KVM.

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Yang, Sheng]

On Wednesday 09 July 2008 23:03:19 Hollis Blanchard wrote:
> On Wed, 2008-07-09 at 11:17 +0200, Christian Ehrhardt wrote:
> > So the question that is left before changing that is, if the
> > original author had something special in mind chosing cycles
> > here. I added Eric on CC for that.
> >
> > I wait with my resubmission of the patch series until all
> > architectures agree *hope* on using getnstimeofday() - after an
> > ack from all sides I would revise my patch series and submit that
> > changes alltogether.
>
> I got an email bounce from Eric the last time I tried to email him,
> so I'm not sure he's still with Intel.
>
> However, I don't think he had any special intention; I think he was
> just porting xentrace to KVM.

Eric had completed his internship in Intel, so...

I like the term "timestamp" too. I think he used "cycles" only because 
there is a function called get_cycles().

But instead of getnstimeofday(), I suggest using ktime_get() here. 
It's little more precise than getnstimeofday(), and ktime_t is more 
easily to be handled. And I think the overhead it brought can be 
ignored too.

--
Thanks
Yang, Sheng

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

[Yang, Sheng]

On Thursday 10 July 2008 18:22:15 Yang, Sheng wrote:
> On Wednesday 09 July 2008 23:03:19 Hollis Blanchard wrote:
> > On Wed, 2008-07-09 at 11:17 +0200, Christian Ehrhardt wrote:
> > > So the question that is left before changing that is, if the
> > > original author had something special in mind chosing cycles
> > > here. I added Eric on CC for that.
> > >
> > > I wait with my resubmission of the patch series until all
> > > architectures agree *hope* on using getnstimeofday() - after an
> > > ack from all sides I would revise my patch series and submit
> > > that changes alltogether.
> >
> > I got an email bounce from Eric the last time I tried to email
> > him, so I'm not sure he's still with Intel.
> >
> > However, I don't think he had any special intention; I think he
> > was just porting xentrace to KVM.
>
> Eric had completed his internship in Intel, so...

Drop Eric here...

-- 
Thanks
Yang, Sheng

>
> I like the term "timestamp" too. I think he used "cycles" only
> because there is a function called get_cycles().
>
> But instead of getnstimeofday(), I suggest using ktime_get() here.
> It's little more precise than getnstimeofday(), and ktime_t is more
> easily to be handled. And I think the overhead it brought can be
> ignored too.
>
> --
> Thanks
> Yang, Sheng
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Avi Kivity]

Yang, Sheng wrote:
> On Wednesday 09 July 2008 23:03:19 Hollis Blanchard wrote:
>   
>> On Wed, 2008-07-09 at 11:17 +0200, Christian Ehrhardt wrote:
>>     
>>> So the question that is left before changing that is, if the
>>> original author had something special in mind chosing cycles
>>> here. I added Eric on CC for that.
>>>
>>> I wait with my resubmission of the patch series until all
>>> architectures agree *hope* on using getnstimeofday() - after an
>>> ack from all sides I would revise my patch series and submit that
>>> changes alltogether.
>>>       
>> I got an email bounce from Eric the last time I tried to email him,
>> so I'm not sure he's still with Intel.
>>
>> However, I don't think he had any special intention; I think he was
>> just porting xentrace to KVM.
>>     
>
> Eric had completed his internship in Intel, so...
>
> I like the term "timestamp" too. I think he used "cycles" only because 
> there is a function called get_cycles().
>
> But instead of getnstimeofday(), I suggest using ktime_get() here. 
> It's little more precise than getnstimeofday(), and ktime_t is more 
> easily to be handled. And I think the overhead it brought can be 
> ignored too.
>   

What is the overhead of ktime_get()?

-- 
error compiling committee.c: too many arguments to function

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

[Yang, Sheng]

On Thursday 10 July 2008 21:32:29 Avi Kivity wrote:
> Yang, Sheng wrote:
> > On Wednesday 09 July 2008 23:03:19 Hollis Blanchard wrote:
> >> On Wed, 2008-07-09 at 11:17 +0200, Christian Ehrhardt wrote:
> >>> So the question that is left before changing that is, if the
> >>> original author had something special in mind chosing cycles
> >>> here. I added Eric on CC for that.
> >>>
> >>> I wait with my resubmission of the patch series until all
> >>> architectures agree *hope* on using getnstimeofday() - after an
> >>> ack from all sides I would revise my patch series and submit
> >>> that changes alltogether.
> >>
> >> I got an email bounce from Eric the last time I tried to email
> >> him, so I'm not sure he's still with Intel.
> >>
> >> However, I don't think he had any special intention; I think he
> >> was just porting xentrace to KVM.
> >
> > Eric had completed his internship in Intel, so...
> >
> > I like the term "timestamp" too. I think he used "cycles" only
> > because there is a function called get_cycles().
> >
> > But instead of getnstimeofday(), I suggest using ktime_get()
> > here. It's little more precise than getnstimeofday(), and ktime_t
> > is more easily to be handled. And I think the overhead it brought
> > can be ignored too.
>
> What is the overhead of ktime_get()?

Well, I just means it wrapped getnstimeofday(), and compared to 
rdtscll(), it got little overhead... :)

-- 
Thanks
Yang, Sheng

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

[Carsten Otte]

Avi Kivity wrote:
> What is the overhead of ktime_get()?
I think I'd like an arch specific timestamp. This way we could use our 
clock-cycle-granularity-non-privileged-timestamp instruction ;-). If 
we need a common implementation, I don't think there's much difference 
between different syscalls in terms of overhead.

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

[Hollis Blanchard]

On Fri, 2008-07-11 at 09:34 +0200, Carsten Otte wrote:
> Avi Kivity wrote:
> > What is the overhead of ktime_get()?
> I think I'd like an arch specific timestamp. This way we could use our 
> clock-cycle-granularity-non-privileged-timestamp instruction ;-). If 
> we need a common implementation, I don't think there's much difference 
> between different syscalls in terms of overhead.

Carsten tried to elaborate on IRC, but the summary is that a) time on
S390 is complicated, and b) they have a time source that yields finer
granularity than nanoseconds. So I'm inclined to say fine, we can have
an arch-specific hook, and almost every arch will just use
getnstimeofday().

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Avi Kivity]

Carsten Otte wrote:
> Avi Kivity wrote:
>> What is the overhead of ktime_get()?
> I think I'd like an arch specific timestamp. This way we could use our 
> clock-cycle-granularity-non-privileged-timestamp instruction ;-). If 
> we need a common implementation, I don't think there's much difference 
> between different syscalls in terms of overhead.

This is all in-kernel, so no syscalls.  Since I doubt you need sub-ns 
granularity for kvmtrace, can we do without an arch hook?

-- 
error compiling committee.c: too many arguments to function

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

[Christian Borntraeger]

Am Sonntag, 13. Juli 2008 schrieb Avi Kivity:
> Carsten Otte wrote:
> > Avi Kivity wrote:
> >> What is the overhead of ktime_get()?
> > I think I'd like an arch specific timestamp. This way we could use our 
> > clock-cycle-granularity-non-privileged-timestamp instruction ;-). If 
> > we need a common implementation, I don't think there's much difference 
> > between different syscalls in terms of overhead.
> 
> This is all in-kernel, so no syscalls.  Since I doubt you need sub-ns 
> granularity for kvmtrace, can we do without an arch hook?


Yes. I just talked with our kernel maintainers and we do not want another 
special case for s390. If ktime_get is fine for blktrace, scheduler and 
almost any other kernel core component - it is fine for tracing.

I also talked to Christian Ehrhardt and he will send an updated patch set 
soon. 

Christian

[PATCH 3/5] kvmppc: kvmtrace: enable KVM_TRACE building for powerpc

[ehrhardt]

From: Jerone Young <jyoung5@us.ibm.com>

This patch enables KVM_TRACE to build for PowerPC arch. This means just
adding sections to Kconfig and Makefile.

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---

[diffstat]
 Kconfig  |   11 +++++++++++
 Makefile |    6 ++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

[diff]
diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig
@@ -37,6 +37,17 @@ config KVM_BOOKE_HOST
 	  Provides host support for KVM on Book E PowerPC processors. Currently
 	  this works on 440 processors only.
 
+config KVM_TRACE
+	bool "KVM trace support"
+	depends on KVM && MARKERS && SYSFS
+	select RELAY
+	select DEBUG_FS
+	default n
+	---help---
+	  This option allows reading a trace of kvm-related events through
+	  relayfs.  Note the ABI is not considered stable and will be
+	  modified in future updates.
+
 source drivers/virtio/Kconfig
 
 endif # VIRTUALIZATION
diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
--- a/arch/powerpc/kvm/Makefile
+++ b/arch/powerpc/kvm/Makefile
@@ -4,9 +4,11 @@
 
 EXTRA_CFLAGS += -Ivirt/kvm -Iarch/powerpc/kvm
 
-common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o coalesced_mmio.o)
+common-objs-y = $(addprefix ../../../virt/kvm/, kvm_main.o coalesced_mmio.o)
 
-kvm-objs := $(common-objs) powerpc.o emulate.o booke_guest.o
+common-objs-$(CONFIG_KVM_TRACE)  += $(addprefix ../../../virt/kvm/, kvm_trace.o)
+
+kvm-objs := $(common-objs-y) powerpc.o emulate.o booke_guest.o
 obj-$(CONFIG_KVM) += kvm.o
 
 AFLAGS_booke_interrupts.o := -I$(obj)

[PATCH 4/5] kvmppc: kvmtrace: adds trace points for ppc tlb activity v2

[ehrhardt]

From: Jerone Young <jyoung5@us.ibm.com>
From: Jerone Young <jyoung5@us.ibm.com>

This patch adds trace points to track powerpc TLB activities using the
KVM_TRACE infrastructure.
Update to v1:
	- fixed wrong indents reported by checkpatch

Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---

[diffstat]
 arch/powerpc/kvm/44x_tlb.c |   15 ++++++++++++++-
 arch/powerpc/kvm/emulate.c |    4 ++++
 include/linux/kvm.h        |    3 +++
 3 files changed, 21 insertions(+), 1 deletion(-)

[diff]

diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
--- a/arch/powerpc/kvm/44x_tlb.c
+++ b/arch/powerpc/kvm/44x_tlb.c
@@ -19,6 +19,7 @@
 
 #include <linux/types.h>
 #include <linux/string.h>
+#include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/highmem.h>
 #include <asm/mmu-44x.h>
@@ -175,6 +176,10 @@
 	stlbe->word1 = (hpaddr & 0xfffffc00) | ((hpaddr >> 32) & 0xf);
 	stlbe->word2 = kvmppc_44x_tlb_shadow_attrib(flags,
 	                                            vcpu->arch.msr & MSR_PR);
+
+	KVMTRACE_5D(STLB_WRITE, vcpu, victim,
+			stlbe->tid, stlbe->word0, stlbe->word1, stlbe->word2,
+			handler);
 }
 
 void kvmppc_mmu_invalidate(struct kvm_vcpu *vcpu, u64 eaddr, u64 asid)
@@ -203,6 +208,9 @@
 
 		kvmppc_44x_shadow_release(vcpu, i);
 		stlbe->word0 = 0;
+		KVMTRACE_5D(STLB_INVAL, vcpu, i,
+				stlbe->tid, stlbe->word0, stlbe->word1,
+				stlbe->word2, handler);
 	}
 	up_write(&current->mm->mmap_sem);
 }
@@ -216,8 +224,13 @@
 	/* XXX Replace loop with fancy data structures. */
 	down_write(&current->mm->mmap_sem);
 	for (i = 0; i <= tlb_44x_hwater; i++) {
+		struct tlbe *stlbe = &vcpu->arch.shadow_tlb[i];
+
 		kvmppc_44x_shadow_release(vcpu, i);
-		vcpu->arch.shadow_tlb[i].word0 = 0;
+		stlbe->word0 = 0;
+		KVMTRACE_5D(STLB_INVAL, vcpu, i,
+				stlbe->tid, stlbe->word0, stlbe->word1,
+				stlbe->word2, handler);
 	}
 	up_write(&current->mm->mmap_sem);
 }
diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -169,6 +169,10 @@
 		 * are mapped on the fly. */
 		kvmppc_mmu_map(vcpu, eaddr, raddr >> PAGE_SHIFT, asid, flags);
 	}
+
+	KVMTRACE_5D(GTLB_WRITE, vcpu, index,
+			tlbe->tid, tlbe->word0, tlbe->word1, tlbe->word2,
+			handler);
 
 	return EMULATE_DONE;
 }
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -470,5 +470,8 @@
 #define KVM_TRC_LMSW             (KVM_TRC_HANDLER + 0x13)
 #define KVM_TRC_APIC_ACCESS      (KVM_TRC_HANDLER + 0x14)
 #define KVM_TRC_TDP_FAULT        (KVM_TRC_HANDLER + 0x15)
+#define KVM_TRC_GTLB_WRITE       (KVM_TRC_HANDLER + 0x16)
+#define KVM_TRC_STLB_WRITE       (KVM_TRC_HANDLER + 0x17)
+#define KVM_TRC_STLB_INVAL       (KVM_TRC_HANDLER + 0x18)
 
 #endif

[PATCH 5/5] kvmppc: kvmtrace: trace powerpc instruction emulation

[ehrhardt]

From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>

This patch adds trace points for the instruction emulation on embedded powerpc
utilizing the KVM_TRACE interface.
The userspace portion to map and analyze the new ppc trace records will follow
soon.

Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---

[diffstat]
 arch/powerpc/kvm/emulate.c |    4 ++++
 include/linux/kvm.h        |    1 +
 2 files changed, 5 insertions(+)

[diff]

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -769,6 +769,10 @@
 		break;
 	}
 
+	KVMTRACE_4D(PPC_INSTR, vcpu,
+			inst, vcpu->arch.pc, emulated,
+			(__u32)current_kernel_time().tv_nsec, entryexit);
+
 	if (advance)
 		vcpu->arch.pc += 4; /* Advance past emulated instruction. */
 
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -473,5 +473,6 @@
 #define KVM_TRC_GTLB_WRITE       (KVM_TRC_HANDLER + 0x16)
 #define KVM_TRC_STLB_WRITE       (KVM_TRC_HANDLER + 0x17)
 #define KVM_TRC_STLB_INVAL       (KVM_TRC_HANDLER + 0x18)
+#define KVM_TRC_PPC_INSTR        (KVM_TRC_HANDLER + 0x19)
 
 #endif