[PATCH] xen: fix MSR xentrace output

[PATCH] xen: fix MSR xentrace output

[Christoph Egger]

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


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 <Christoph.Egger@amd.com>

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

[-- Attachment #2: xen_fixtrace.diff --]
[-- Type: text/x-diff, Size: 1396 bytes --]

diff -r 2c6ae364ed7b xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Mon Aug 02 11:00:56 2010 +0100
+++ b/xen/arch/x86/hvm/hvm.c	Tue Aug 03 18:21:41 2010 +0200
@@ -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;
 }

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xen: fix MSR xentrace output

[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


On Tue, Aug 3, 2010 at 5:24 PM, Christoph Egger <Christoph.Egger@amd.com> wrote:
>
> 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 <Christoph.Egger@amd.com>
>
> --
> ---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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>

Re: [PATCH] xen: fix MSR xentrace output

[George Dunlap]

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

On Tue, Aug 3, 2010 at 5:39 PM, George Dunlap <dunlapg@umich.edu> wrote:
> 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
>
>
> On Tue, Aug 3, 2010 at 5:24 PM, Christoph Egger <Christoph.Egger@amd.com> wrote:
>>
>> 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 <Christoph.Egger@amd.com>
>>
>> --
>> ---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
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>
>>
>

Re: [PATCH] xen: fix MSR xentrace output

[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

On 03/08/2010 17:39, "George Dunlap" <dunlapg@umich.edu> wrote:

> 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
> 
> 
> On Tue, Aug 3, 2010 at 5:24 PM, Christoph Egger <Christoph.Egger@amd.com>
> wrote:
>> 
>> 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 <Christoph.Egger@amd.com>
>> 
>> --
>> ---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
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>> 
>> 

Re: [PATCH] xen: fix MSR xentrace output

[George Dunlap]

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

On Tue, Aug 3, 2010 at 5:57 PM, Keir Fraser <keir.fraser@eu.citrix.com> wrote:
> 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
>
> On 03/08/2010 17:39, "George Dunlap" <dunlapg@umich.edu> wrote:
>
>> 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
>>
>>
>> On Tue, Aug 3, 2010 at 5:24 PM, Christoph Egger <Christoph.Egger@amd.com>
>> wrote:
>>>
>>> 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 <Christoph.Egger@amd.com>
>>>
>>> --
>>> ---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
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
>>>
>>>
>
>
>