Skip to contentSkip to conversation

xen: fix MSR xentrace output

PATCH In review Christoph Egger 4 participants 5 messages Aug 3, 2010 – Aug 4, 2010

lore
Share & tools
lore

Christoph Egger

author
#0 lore raw

PATCH 1/1 xen: fix MSR xentrace output

Hi!

Attached patch corrects MSR read/write trace output. Also avoid duplicate MSR read/write lines in xentrace output. MSR and value are mixed up.

Signed-off-by Christoph Egger <[email protected]>
Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/hvm/hvm.c +6 -3
diff -r 2c6ae364ed7b xen/arch/x86/hvm/hvm.c
@@ -2105,11 +2105,12 @@ int hvm_msr_read_intercept(unsigned int 
         }
     }
 
-    HVMTRACE_3D(MSR_READ, (uint32_t)*msr_content, (uint32_t)(*msr_content >> 32), msr);
+    HVMTRACE_3D(MSR_READ, msr, (uint32_t)*msr_content, (uint32_t)(*msr_content >> 32));
 
     return X86EMUL_OKAY;
 
 gp_fault:
+    HVMTRACE_3D(MSR_READ, msr, (uint32_t)*msr_content, (uint32_t)(*msr_content >> 32));
     hvm_inject_exception(TRAP_gp_fault, 0, 0);
     return X86EMUL_EXCEPTION;
 }

@@ -2121,8 +2122,6 @@ int hvm_msr_write_intercept(unsigned int
     uint32_t cpuid[4];
     int ret;
 
-    HVMTRACE_3D(MSR_WRITE, (uint32_t)msr_content, (uint32_t)(msr_content >> 32), msr);
-
     hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]);
     mtrr = !!(cpuid[3] & bitmaskof(X86_FEATURE_MTRR));
 

@@ -2203,9 +2202,13 @@ int hvm_msr_write_intercept(unsigned int
             return hvm_funcs.msr_write_intercept(msr, msr_content);
     }
 
+    HVMTRACE_3D(MSR_WRITE, msr, (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
+
     return X86EMUL_OKAY;
 
 gp_fault:
+    HVMTRACE_3D(MSR_WRITE, msr, (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
+
     hvm_inject_exception(TRAP_gp_fault, 0, 0);
     return X86EMUL_EXCEPTION;
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw

NACK for discussion.

What do you mean they're "mixed up"? Putting the 64-bit value first makes it easy to define a structure you can just point directly at the binary data. If xentrace_format is different, wouldnt' it be easier to change it than the hypervisor?

-George

Quoted 23 lines · Christoph Egger

Hi!

Attached patch corrects MSR read/write trace output. Also avoid duplicate MSR read/write lines in xentrace output. MSR and value are mixed up.

Signed-off-by Christoph Egger <[email protected]>

--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#2 lore raw

And, why are you moving the trace things around? In the write case, you're pointlessly duplicating code.

I can see the point of adding a case for failed reads, so you can see what the msr address was that failed. But in that case I'd probably trace a value of 0 or -1 for msr content.

-George

Quoted 35 lines · George Dunlap

NACK for discussion.

What do you mean they're "mixed up"?  Putting the 64-bit value first makes it easy to define a structure you can just point directly at the binary data.  If xentrace_format is different, wouldnt' it be easier to change it than the hypervisor?

 -George

Quoted 23 lines · Christoph Egger

Hi!

Attached patch corrects MSR read/write trace output. Also avoid duplicate MSR read/write lines in xentrace output. MSR and value are mixed up.

Signed-off-by Christoph Egger <[email protected]>

--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#3 lore raw
In reply to George Dunlap

Well, some of the various MSR_READ/WRITE traces are wrong one way or the other. The vmx/svm-specific trace points have since the beginning of time been ordered msr_index,msr_low,msr_high. It's the new trace points added by you to hvm.c that are the 'novel' way round (msr_low,msr_high,msr_index). Also the proliferation of trace points is stupid: the vmx/svm-specific ones could easily be got rid of and be on a common exit path from the hvm-generic intercept functions instead. The movement and duplication of the MSR_WRITE trace points in Christoph's patch is especially egregious, as the svm/vmx-specific trace points can simply be deleted.

-- Keir

Quoted 35 lines · George Dunlap

NACK for discussion.

What do you mean they're "mixed up"? Putting the 64-bit value first makes it easy to define a structure you can just point directly at the binary data. If xentrace_format is different, wouldnt' it be easier to change it than the hypervisor?

-George

Quoted 23 lines · Christoph Egger

Hi!

Attached patch corrects MSR read/write trace output. Also avoid duplicate MSR read/write lines in xentrace output. MSR and value are mixed up.

Signed-off-by Christoph Egger <[email protected]>

--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#4 lore raw

Ah, I see -- when I added the MSR tracing to hvm.c, I didn't realize that there were still MSR traces in the svm/vmx files. Mea culpa -- duplicated trace values are bad, and having inconsistent ordering for the parameters in the traces is unacceptable.

I withdraw my NACK.

-George

Quoted 52 lines · Keir Fraser

Well, some of the various MSR_READ/WRITE traces are wrong one way or the other. The vmx/svm-specific trace points have since the beginning of time been ordered msr_index,msr_low,msr_high. It's the new trace points added by you to hvm.c that are the 'novel' way round (msr_low,msr_high,msr_index). Also the proliferation of trace points is stupid: the vmx/svm-specific ones could easily be got rid of and be on a common exit path from the hvm-generic intercept functions instead. The movement and duplication of the MSR_WRITE trace points in Christoph's patch is especially egregious, as the svm/vmx-specific trace points can simply be deleted.

 -- Keir

Quoted 35 lines · George Dunlap

NACK for discussion.

What do you mean they're "mixed up"?  Putting the 64-bit value first makes it easy to define a structure you can just point directly at the binary data.  If xentrace_format is different, wouldnt' it be easier to change it than the hypervisor?

 -George

Quoted 23 lines · Christoph Egger

Hi!

Attached patch corrects MSR read/write trace output. Also avoid duplicate MSR read/write lines in xentrace output. MSR and value are mixed up.

Signed-off-by Christoph Egger <[email protected]>

--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel