Re: kvm trace support for ppc

Re: kvm trace support for ppc

[Hollis Blanchard]

On Tuesday 13 May 2008 03:06:19 Tan, Li wrote:
> Hollisb,
> I have 2 more questions:
> 1. seems record won't be overwritten because current code is as following:
> /*
>  *  The relay channel is used in "no-overwrite" mode, it keeps trace of how
>  *  many times we encountered a full subbuffer, to tell user space app the
>  *  lost records there were.
>  */
> static int kvm_subbuf_start_callback(struct rchan_buf *buf, void *subbuf,
> 				     void *prev_subbuf, size_t prev_padding)
> {
> 	struct kvm_trace *kt;
> 
> 	if (!relay_buf_full(buf))
> 		return 1;
> 
> 	kt = buf->chan->private_data;
> 	atomic_inc(&kt->lost_records);
> 
> 	return 0;
> }
> 
> 2. Then needn't expose debugfs entry "kvmtrace-metadata", we can use 
existing relayfs to output struct metadata with kmagic, if we update code as 
following?
> static int kvm_subbuf_start_callback(struct rchan_buf *buf, void *subbuf,
> 				     void *prev_subbuf, size_t prev_padding)
> {
> 	struct kvm_trace *kt;
> 
> 	if (!relay_buf_full(buf))
> 	{
> 		if (!prev_subbuf) {
> 			//here is executed only once (when the channel is opened)
> 			subbuf_start_reserve(buf, sizeof(struct metadata));
> 			((struct metadata *)subbuf)->kmagic = 0x1234;
> 		}
> 		
> 		return 1;
> 	}
> 
> 	kt = buf->chan->private_data;
> 	atomic_inc(&kt->lost_records);
> 
> 	return 0;
> }

Ah, I didn't understand what the "lost records" handling was about. Given that 
it won't be lost, it would be OK for the kernel to export the header, and in 
that case I guess you would want it to be the same size as the other records. 
I'm not sure how I feel about that from a layering point of view, but at 
least it would be functional.

-- 
Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft 
Defy all challenges. Microsoft(R) Visual Studio 2008. 
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: kvm trace support for ppc

[Tan, Li]

[-- Attachment #1: Type: text/plain, Size: 2981 bytes --]



-----Original Message-----
From: Hollis Blanchard [mailto:hollisb@us.ibm.com] 
Sent: 2008年5月15日 5:37
To: Tan, Li
Cc: kvm-devel; kvm-ppc-devel@lists.sourceforge.net
Subject: Re: kvm trace support for ppc

On Tuesday 13 May 2008 03:06:19 Tan, Li wrote:
> Hollisb,
> I have 2 more questions:
> 1. seems record won't be overwritten because current code is as following:
> /*
>  *  The relay channel is used in "no-overwrite" mode, it keeps trace of how
>  *  many times we encountered a full subbuffer, to tell user space app the
>  *  lost records there were.
>  */
> static int kvm_subbuf_start_callback(struct rchan_buf *buf, void *subbuf,
> 				     void *prev_subbuf, size_t prev_padding)
> {
> 	struct kvm_trace *kt;
> 
> 	if (!relay_buf_full(buf))
> 		return 1;
> 
> 	kt = buf->chan->private_data;
> 	atomic_inc(&kt->lost_records);
> 
> 	return 0;
> }
> 
> 2. Then needn't expose debugfs entry "kvmtrace-metadata", we can use 
existing relayfs to output struct metadata with kmagic, if we update code as 
following?
> static int kvm_subbuf_start_callback(struct rchan_buf *buf, void *subbuf,
> 				     void *prev_subbuf, size_t prev_padding)
> {
> 	struct kvm_trace *kt;
> 
> 	if (!relay_buf_full(buf))
> 	{
> 		if (!prev_subbuf) {
> 			//here is executed only once (when the channel is opened)
> 			subbuf_start_reserve(buf, sizeof(struct metadata));
> 			((struct metadata *)subbuf)->kmagic = 0x1234;
> 		}
> 		
> 		return 1;
> 	}
> 
> 	kt = buf->chan->private_data;
> 	atomic_inc(&kt->lost_records);
> 
> 	return 0;
> }

Ah, I didn't understand what the "lost records" handling was about. Given that 
it won't be lost, it would be OK for the kernel to export the header, and in 
that case I guess you would want it to be the same size as the other records. 
I'm not sure how I feel about that from a layering point of view, but at 
least it would be functional.

[tan] The relay channel is used in "no-overwrite" mode, so it will lost any new items if encounters a full subbuffer, "lost records" is to count lost records. Yes, metadata is the same size as the other records.

This solution needn't change kvmtrace user app, only need to change kvmtrace_format.pl like following:
        if i == 1:
            line = sys.stdin.read(struct.calcsize(HDRREC))
            (kmgc, umgc, mgcpad) = struct.unpack(HDRREC, line)

            if kmgc == 0x1234:
                rev = False
            else:
                rev = True
	    continue

	# If i > 1:
        line = sys.stdin.read(struct.calcsize(HDRREC))
	(event, pid, vcpu_id) = struct.unpack(HDRREC, line)
	...
	if rev:
		event = reverse_int(event)
		pid = reverse_int(pid)
		vcpu_id = reverse_int(vcpu_id)
	    tsc = reverse_qword(tsc)
	    d1 = reverse_int(d1)
	    d2 = reverse_int(d2)
	    d3 = reverse_int(d3)
	    d4 = reverse_int(d4)
	    d5 = reverse_int(d5)

 -- 
Hollis Blanchard
IBM Linux Technology Center


[-- Attachment #2: Type: text/plain, Size: 230 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft 
Defy all challenges. Microsoft(R) Visual Studio 2008. 
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: kvm trace support for ppc

[Tan, Li]

[-- Attachment #1: Type: text/plain, Size: 3726 bytes --]

struct metadata {
		u32 kmagic; /* stores kernel defined metadata read from debugfs entry*/
		u32 umagic; /* stores userspace tool defined metadata */
		u32 extra;  /* it is redundant, only use to fit into record. */
	}
seems umagic and extra are no need now.
I guess they were designed as record pad to keep the format of trace log transparent to kvmtrace_format, but now kvmtrace_format has to know the format: the first record is metadata

So I’ll change to 
struct metadata {
		u32 kmagic; /* stores kernel defined metadata */
	}
Tan Li

-----Original Message-----
From: Tan, Li 
Sent: 2008年5月15日 9:20
To: 'Hollis Blanchard'
Cc: kvm-devel; kvm-ppc-devel@lists.sourceforge.net
Subject: RE: kvm trace support for ppc



-----Original Message-----
From: Hollis Blanchard [mailto:hollisb@us.ibm.com] 
Sent: 2008年5月15日 5:37
To: Tan, Li
Cc: kvm-devel; kvm-ppc-devel@lists.sourceforge.net
Subject: Re: kvm trace support for ppc

On Tuesday 13 May 2008 03:06:19 Tan, Li wrote:
> Hollisb,
> I have 2 more questions:
> 1. seems record won't be overwritten because current code is as following:
> /*
>  *  The relay channel is used in "no-overwrite" mode, it keeps trace of how
>  *  many times we encountered a full subbuffer, to tell user space app the
>  *  lost records there were.
>  */
> static int kvm_subbuf_start_callback(struct rchan_buf *buf, void *subbuf,
> 				     void *prev_subbuf, size_t prev_padding)
> {
> 	struct kvm_trace *kt;
> 
> 	if (!relay_buf_full(buf))
> 		return 1;
> 
> 	kt = buf->chan->private_data;
> 	atomic_inc(&kt->lost_records);
> 
> 	return 0;
> }
> 
> 2. Then needn't expose debugfs entry "kvmtrace-metadata", we can use 
existing relayfs to output struct metadata with kmagic, if we update code as 
following?
> static int kvm_subbuf_start_callback(struct rchan_buf *buf, void *subbuf,
> 				     void *prev_subbuf, size_t prev_padding)
> {
> 	struct kvm_trace *kt;
> 
> 	if (!relay_buf_full(buf))
> 	{
> 		if (!prev_subbuf) {
> 			//here is executed only once (when the channel is opened)
> 			subbuf_start_reserve(buf, sizeof(struct metadata));
> 			((struct metadata *)subbuf)->kmagic = 0x1234;
> 		}
> 		
> 		return 1;
> 	}
> 
> 	kt = buf->chan->private_data;
> 	atomic_inc(&kt->lost_records);
> 
> 	return 0;
> }

Ah, I didn't understand what the "lost records" handling was about. Given that 
it won't be lost, it would be OK for the kernel to export the header, and in 
that case I guess you would want it to be the same size as the other records. 
I'm not sure how I feel about that from a layering point of view, but at 
least it would be functional.

[tan] The relay channel is used in "no-overwrite" mode, so it will lost any new items if encounters a full subbuffer, "lost records" is to count lost records. Yes, metadata is the same size as the other records.

This solution needn't change kvmtrace user app, only need to change kvmtrace_format.pl like following:
        if i == 1:
            line = sys.stdin.read(struct.calcsize(HDRREC))
            (kmgc, umgc, mgcpad) = struct.unpack(HDRREC, line)

            if kmgc == 0x1234:
                rev = False
            else:
                rev = True
	    continue

	# If i > 1:
        line = sys.stdin.read(struct.calcsize(HDRREC))
	(event, pid, vcpu_id) = struct.unpack(HDRREC, line)
	...
	if rev:
		event = reverse_int(event)
		pid = reverse_int(pid)
		vcpu_id = reverse_int(vcpu_id)
	    tsc = reverse_qword(tsc)
	    d1 = reverse_int(d1)
	    d2 = reverse_int(d2)
	    d3 = reverse_int(d3)
	    d4 = reverse_int(d4)
	    d5 = reverse_int(d5)

 -- 
Hollis Blanchard
IBM Linux Technology Center


[-- Attachment #2: Type: text/plain, Size: 230 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft 
Defy all challenges. Microsoft(R) Visual Studio 2008. 
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

[PATCH] [RFC][PATCH] kvm: kvmtrace: kvm_trace in kernel for supporting big_endian

[Tan, Li]

>From 63283d32ad5faf0845fa0358fd71d119f4dc0e3d Mon Sep 17 00:00:00 2001
From: Tan Li <li.tan@intel.com>
Date: Mon, 19 May 2008 17:18:54 +0800
Subject: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvm_trace in kernel for
supporting big_endian

Currently kvmtrace is not portable. This will prevent from copying a
trace file from big-endian target to little-endian workstation for
analysis.

In the patch, kernel outputs metadata containing a magic number to trace
log.

Signed-off-by: Tan Li <li.tan@intel.com>

---
 include/linux/kvm.h  |    4 ++--
 virt/kvm/kvm_trace.c |   18 ++++++++++++------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index a281afe..ca08cb1 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -294,14 +294,14 @@ struct kvm_trace_rec {
 	__u32 vcpu_id;
 	union {
 		struct {
-			__u32 cycle_lo, cycle_hi;
+			__u64 cycle_u64;
 			__u32 extra_u32[KVM_TRC_EXTRA_MAX];
 		} cycle;
 		struct {
 			__u32 extra_u32[KVM_TRC_EXTRA_MAX];
 		} nocycle;
 	} u;
-};
+} __attribute__((packed));
 
 #define KVMIO 0xAE
 
diff --git a/virt/kvm/kvm_trace.c b/virt/kvm/kvm_trace.c
index 0e49547..58141f3 100644
--- a/virt/kvm/kvm_trace.c
+++ b/virt/kvm/kvm_trace.c
@@ -72,11 +72,7 @@ static void kvm_add_trace(void *probe_private, void
*call_data,
 	rec.cycle_in 	= p->cycle_in;
 
 	if (rec.cycle_in) {
-		u64 cycle = 0;
-
-		cycle = get_cycles();
-		rec.u.cycle.cycle_lo = (u32)cycle;
-		rec.u.cycle.cycle_hi = (u32)(cycle >> 32);
+		rec.u.cycle.cycle_u64 = get_cycles();
 
 		for (i = 0; i < rec.extra_u32; i++)
 			rec.u.cycle.extra_u32[i] = va_arg(*args, u32);
@@ -114,8 +110,18 @@ static int kvm_subbuf_start_callback(struct
rchan_buf *buf, void *subbuf,
 {
 	struct kvm_trace *kt;
 
-	if (!relay_buf_full(buf))
+	if (!relay_buf_full(buf)) {
+		if (!prev_subbuf) {
+			/*
+			 * executed only once when the channel is opened
+			 * save metadata as first record
+			 */
+			subbuf_start_reserve(buf, sizeof(u32));
+			*(u32 *)subbuf = 0x12345678;
+		}
+
 		return 1;
+	}
 
 	kt = buf->chan->private_data;
 	atomic_inc(&kt->lost_records);
-- 
1.5.3.4

[PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Tan, Li]

>From 9d39264bdfb00a1a717074e486a948a578547c50 Mon Sep 17 00:00:00 2001
From: Tan Li <li.tan@intel.com>
Date: Tue, 20 May 2008 09:14:41 +0800
Subject: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for
supporting big_endian

Currently kvmtrace is not portable, and prevent from copying a trace
file from big-endian target to little-endian workstation for analysis.

In the patch, kvmtrace_format reads and checks the magic number from
trace log. if needed, then change bytes order of all fields in records
followed.

Signed-off-by: Tan Li <li.tan@intel.com>

---
 user/kvmtrace_format |   46
++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/user/kvmtrace_format b/user/kvmtrace_format
index 9e7cfd4..386d855 100755
--- a/user/kvmtrace_format
+++ b/user/kvmtrace_format
@@ -55,6 +55,22 @@ def read_defs(defs_file):
 
     return defs
 
+def reverse_bytes(origin, bytes_cnt):
+	i = 0
+	rtn = 0L
+	while i < bytes_cnt:
+		i = i + 1
+	        b = origin & 0xff
+		origin = origin >> 8
+		rtn = (rtn << 8) + b
+	return rtn
+
+def reverse_int(origin):
+	return reverse_bytes(origin, 4)
+
+def reverse_qword(origin):
+	return reverse_bytes(origin, 8)
+
 def sighand(x,y):
     global interrupted
     interrupted = 1
@@ -100,18 +116,40 @@ D2REC  = "II"
 D3REC  = "III"
 D4REC  = "IIII"
 D5REC  = "IIIII"
+KMAGIC  = "I"
 
 last_tsc = 0
+reverse_flag = 0
 
 i=0
 
 while not interrupted:
     try:
         i=i+1
+
+        if i == 1:
+            line = sys.stdin.read(struct.calcsize(KMAGIC))
+            if not line:
+                break
+            kmgc = struct.unpack(KMAGIC, line)[0]
+
+	    # if "kvmtrace-metadata".kmagic != kmagic,
+	    # then need change byte order of all fields
+            if kmgc == 0x12345678:
+                reverse_flag = False
+            else:
+                reverse_flag = True
+	    continue
+
         line = sys.stdin.read(struct.calcsize(HDRREC))
         if not line:
             break
 	(event, pid, vcpu_id) = struct.unpack(HDRREC, line)
+
+	if reverse_flag:
+		event = reverse_int(event)
+		pid = reverse_int(pid)
+		vcpu_id = reverse_int(vcpu_id)
 
         n_data = event >> 28 & 0x7
         tsc_in = event >> 31
@@ -155,6 +193,14 @@ while not interrupted:
                 break
             (d1, d2, d3, d4, d5) = struct.unpack(D5REC, line)
 
+	if reverse_flag:
+	    tsc = reverse_qword(tsc)
+	    d1 = reverse_int(d1)
+	    d2 = reverse_int(d2)
+	    d3 = reverse_int(d3)
+	    d4 = reverse_int(d4)
+	    d5 = reverse_int(d5)
+
 	event &= 0x0fffffff
 
         # provide relative TSC
-- 
1.5.3.4

Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Avi Kivity]

Tan, Li wrote:
> From 9d39264bdfb00a1a717074e486a948a578547c50 Mon Sep 17 00:00:00 2001
> From: Tan Li <li.tan@intel.com>
> Date: Tue, 20 May 2008 09:14:41 +0800
> Subject: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for
> supporting big_endian
>
> Currently kvmtrace is not portable, and prevent from copying a trace
> file from big-endian target to little-endian workstation for analysis.
>
> In the patch, kvmtrace_format reads and checks the magic number from
> trace log. if needed, then change bytes order of all fields in records
> followed.
>
>  
> +def reverse_bytes(origin, bytes_cnt):
> +	i = 0
> +	rtn = 0L
> +	while i < bytes_cnt:
> +		i = i + 1
> +	        b = origin & 0xff
> +		origin = origin >> 8
> +		rtn = (rtn << 8) + b
> +	return rtn
> +
> +def reverse_int(origin):
> +	return reverse_bytes(origin, 4)
> +
> +def reverse_qword(origin):
> +	return reverse_bytes(origin, 8)
> +
>  def sighand(x,y):
>      global interrupted
>      interrupted = 1
> @@ -100,18 +116,40 @@ D2REC  = "II"
>  D3REC  = "III"
>  D4REC  = "IIII"
>  D5REC  = "IIIII"
> +KMAGIC  = "I"
>   

The python struct module supports reading big-endian and little-endian 
data.  So you can simply switch the format strings if you have the wrong 
endianness.

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

RE: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Tan, Li]

-----Original Message-----
From: Avi Kivity [mailto:avi@qumranet.com] 
Sent: 2008年5月21日 0:23
To: Tan, Li
Cc: kvm@vger.kernel.org
Subject: Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

Tan, Li wrote:
> From 9d39264bdfb00a1a717074e486a948a578547c50 Mon Sep 17 00:00:00 2001
> From: Tan Li <li.tan@intel.com>
> Date: Tue, 20 May 2008 09:14:41 +0800
> Subject: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for
> supporting big_endian
>
> Currently kvmtrace is not portable, and prevent from copying a trace
> file from big-endian target to little-endian workstation for analysis.
>
> In the patch, kvmtrace_format reads and checks the magic number from
> trace log. if needed, then change bytes order of all fields in records
> followed.
>
>  
> +def reverse_bytes(origin, bytes_cnt):
> +	i = 0
> +	rtn = 0L
> +	while i < bytes_cnt:
> +		i = i + 1
> +	        b = origin & 0xff
> +		origin = origin >> 8
> +		rtn = (rtn << 8) + b
> +	return rtn
> +
> +def reverse_int(origin):
> +	return reverse_bytes(origin, 4)
> +
> +def reverse_qword(origin):
> +	return reverse_bytes(origin, 8)
> +
>  def sighand(x,y):
>      global interrupted
>      interrupted = 1
> @@ -100,18 +116,40 @@ D2REC  = "II"
>  D3REC  = "III"
>  D4REC  = "IIII"
>  D5REC  = "IIIII"
> +KMAGIC  = "I"
>   

The python struct module supports reading big-endian and little-endian 
data.  So you can simply switch the format strings if you have the wrong 
endianness.

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

[tan] thanks for your comments. Then it looks like:
def reverse_int(origin):
        s = struct.pack('<I', origin)
        return struct.unpack('>I', s)

def reverse_qword(origin):
        s = struct.pack('<Q', origin)
        return struct.unpack('>Q', s)

Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Avi Kivity]

Tan, Li wrote:
> -----Original Message-----
> From: Avi Kivity [mailto:avi@qumranet.com] 
> Sent: 2008年5月21日 0:23
> To: Tan, Li
> Cc: kvm@vger.kernel.org
> Subject: Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian
>
> Tan, Li wrote:
>   
>> From 9d39264bdfb00a1a717074e486a948a578547c50 Mon Sep 17 00:00:00 2001
>> From: Tan Li <li.tan@intel.com>
>> Date: Tue, 20 May 2008 09:14:41 +0800
>> Subject: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for
>> supporting big_endian
>>
>> Currently kvmtrace is not portable, and prevent from copying a trace
>> file from big-endian target to little-endian workstation for analysis.
>>
>> In the patch, kvmtrace_format reads and checks the magic number from
>> trace log. if needed, then change bytes order of all fields in records
>> followed.
>>
>>  
>> +def reverse_bytes(origin, bytes_cnt):
>> +	i = 0
>> +	rtn = 0L
>> +	while i < bytes_cnt:
>> +		i = i + 1
>> +	        b = origin & 0xff
>> +		origin = origin >> 8
>> +		rtn = (rtn << 8) + b
>> +	return rtn
>> +
>> +def reverse_int(origin):
>> +	return reverse_bytes(origin, 4)
>> +
>> +def reverse_qword(origin):
>> +	return reverse_bytes(origin, 8)
>> +
>>  def sighand(x,y):
>>      global interrupted
>>      interrupted = 1
>> @@ -100,18 +116,40 @@ D2REC  = "II"
>>  D3REC  = "III"
>>  D4REC  = "IIII"
>>  D5REC  = "IIIII"
>> +KMAGIC  = "I"
>>   
>>     
>
> The python struct module supports reading big-endian and little-endian 
> data.  So you can simply switch the format strings if you have the wrong 
> endianness.
>
>   
> [tan] thanks for your comments. Then it looks like:
> def reverse_int(origin):
>         s = struct.pack('<I', origin)
>         return struct.unpack('>I', s)
>
> def reverse_qword(origin):
>         s = struct.pack('<Q', origin)
>         return struct.unpack('>Q', s)
>   

I meant, you can change the definitions of D3REC, D4REC, etc. and not
worry about reversing once you've decoded the magic word.

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

RE: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Tan, Li]

-----Original Message-----
From: Avi Kivity [mailto:avi@qumranet.com] 
Sent: 2008年5月21日 17:21
To: Tan, Li
Cc: kvm@vger.kernel.org
Subject: Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

Tan, Li wrote:
> -----Original Message-----
> From: Avi Kivity [mailto:avi@qumranet.com] 
> Sent: 2008年5月21日 0:23
> To: Tan, Li
> Cc: kvm@vger.kernel.org
> Subject: Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian
>
> Tan, Li wrote:
>   
>> From 9d39264bdfb00a1a717074e486a948a578547c50 Mon Sep 17 00:00:00 2001
>> From: Tan Li <li.tan@intel.com>
>> Date: Tue, 20 May 2008 09:14:41 +0800
>> Subject: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for
>> supporting big_endian
>>
>> Currently kvmtrace is not portable, and prevent from copying a trace
>> file from big-endian target to little-endian workstation for analysis.
>>
>> In the patch, kvmtrace_format reads and checks the magic number from
>> trace log. if needed, then change bytes order of all fields in records
>> followed.
>>
>>  
>> +def reverse_bytes(origin, bytes_cnt):
>> +	i = 0
>> +	rtn = 0L
>> +	while i < bytes_cnt:
>> +		i = i + 1
>> +	        b = origin & 0xff
>> +		origin = origin >> 8
>> +		rtn = (rtn << 8) + b
>> +	return rtn
>> +
>> +def reverse_int(origin):
>> +	return reverse_bytes(origin, 4)
>> +
>> +def reverse_qword(origin):
>> +	return reverse_bytes(origin, 8)
>> +
>>  def sighand(x,y):
>>      global interrupted
>>      interrupted = 1
>> @@ -100,18 +116,40 @@ D2REC  = "II"
>>  D3REC  = "III"
>>  D4REC  = "IIII"
>>  D5REC  = "IIIII"
>> +KMAGIC  = "I"
>>   
>>     
>
> The python struct module supports reading big-endian and little-endian 
> data.  So you can simply switch the format strings if you have the wrong 
> endianness.
>
>   
> [tan] thanks for your comments. Then it looks like:
> def reverse_int(origin):
>         s = struct.pack('<I', origin)
>         return struct.unpack('>I', s)
>
> def reverse_qword(origin):
>         s = struct.pack('<Q', origin)
>         return struct.unpack('>Q', s)
>   

I meant, you can change the definitions of D3REC, D4REC, etc. and not
worry about reversing once you've decoded the magic word.

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

[tan] so it looks like:
	 if reverse_flag == 0: 
			#    print "host and data file are same endian order"
			D3REC  = "III"
			D4REC  = "IIII"
			D5REC  = "IIIII"
	else:
		If pack('<h', 1) == pack('=h',1):
			#    print "host is Little Endian, data file is big endian"
			D3REC  = ">III"
			D4REC  = ">IIII"
			D5REC  = ">IIIII"
		elif pack('>h', 1) == pack('=h',1): 
			#    print "host is big Endian, data file is little endian"
			D3REC  = "<III"
			D4REC  = "<IIII"
			D5REC  = "<IIIII"

Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Avi Kivity]

Tan, Li wrote:
> I meant, you can change the definitions of D3REC, D4REC, etc. and not
> worry about reversing once you've decoded the magic word.
>
>   

> [tan] so it looks like:
> 	 if reverse_flag == 0: 
> 			#    print "host and data file are same endian order"
> 			D3REC  = "III"
> 			D4REC  = "IIII"
> 			D5REC  = "IIIII"
> 	else:
> 		If pack('<h', 1) == pack('=h',1):
> 			#    print "host is Little Endian, data file is big endian"
> 			D3REC  = ">III"
> 			D4REC  = ">IIII"
> 			D5REC  = ">IIIII"
> 		elif pack('>h', 1) == pack('=h',1): 
> 			#    print "host is big Endian, data file is little endian"
> 			D3REC  = "<III"
> 			D4REC  = "<IIII"
> 			D5REC  = "<IIIII"
>   

Not sure about the tests. You aren't looking at the data from the file,
how can you conclude it's little endian or big endian?

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

RE: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Tan, Li]

According to http://docs.python.org/lib/module-struct.html
Character Byte order Size and alignment 
@ native native 
= native standard 
< little-endian standard 
> big-endian standard 
! network (= big-endian) standard 

> [tan] so it looks like:
> 	 if reverse_flag == 0: 
> 			#    print "host and data file are same endian
order"
> 			D3REC  = "III"
> 	else:
#reverse_flag == 1: host and data file are DIFF endian order

> 		If pack('<h', 1) == pack('=h',1): # this implys host is
Little Endian, 
so data file is big endian"


> 			D3REC  = ">III"
> 		elif pack('>h', 1) == pack('=h',1): # this implys host
is big Endian, 
so data file is little endian"


> 			D3REC  = "<III"
> 			D4REC  = "<IIII"
> 			D5REC  = "<IIIII"
>   

Not sure about the tests. You aren't looking at the data from the file,
how can you conclude it's little endian or big endian?

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

Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Avi Kivity]

Tan, Li wrote:
> According to http://docs.python.org/lib/module-struct.html
> Character Byte order Size and alignment 
> @ native native 
> = native standard 
> < little-endian standard 
>   

Oh okay.  You are relying on the user to supply the reverse flag.

But you don't need to do that.  You can start with the formats as "<I" 
and similar.  Read the magic word.  If it mismatches, switch to ">I" and 
start again.

This way you get autodetection of the format, and shorter code as well.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

RE: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

[Tan, Li]

Yes, in this way only need to handle 2 situations, no need to have 3 situations.
Tan Li
-----Original Message-----
From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Avi Kivity
Sent: 2008年5月28日 19:17
To: Tan, Li
Cc: kvm@vger.kernel.org
Subject: Re: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvmtrace_format for supporting big_endian

Tan, Li wrote:
> According to http://docs.python.org/lib/module-struct.html
> Character Byte order Size and alignment 
> @ native native 
> = native standard 
> < little-endian standard 
>   

Oh okay.  You are relying on the user to supply the reverse flag.

But you don't need to do that.  You can start with the formats as "<I" 
and similar.  Read the magic word.  If it mismatches, switch to ">I" and 
start again.

This way you get autodetection of the format, and shorter code as well.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

--
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] [RFC][PATCH] kvm: kvmtrace: kvm_trace in kernel for supporting big_endian

[Avi Kivity]

Tan, Li wrote:
> From 63283d32ad5faf0845fa0358fd71d119f4dc0e3d Mon Sep 17 00:00:00 2001
> From: Tan Li <li.tan@intel.com>
> Date: Mon, 19 May 2008 17:18:54 +0800
> Subject: [PATCH] [RFC][PATCH] kvm: kvmtrace: kvm_trace in kernel for
> supporting big_endian
>
> Currently kvmtrace is not portable. This will prevent from copying a
> trace file from big-endian target to little-endian workstation for
> analysis.
>
> In the patch, kernel outputs metadata containing a magic number to trace
> log.
>
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index a281afe..ca08cb1 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -294,14 +294,14 @@ struct kvm_trace_rec {
>  	__u32 vcpu_id;
>  	union {
>  		struct {
> -			__u32 cycle_lo, cycle_hi;
> +			__u64 cycle_u64;
>  			__u32 extra_u32[KVM_TRC_EXTRA_MAX];
>  		} cycle;
>   

For this bit, you'll need to change 'II' to 'L' in the format, no?

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