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
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 > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 >> >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 >> >>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 >>> >>> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel