George,
what is the reason behind this changeset?
http://xenbits.xensource.com/ext/xenalyze.hg?rev/9fa7e4d2a3af
All my vmexit trace entries have size 4 for 64bit and 3 for 32bit.
Looking at the code in ./xen/arch/x86/hvm/vmx/vmx.c, HVMTRACE_ND() gets
size 3 for VMEXIT64. But HVMTRACE_ND does a
''sizeof(u32)*count+1'' in
xen-4.0.
The xen-unstable macro looks different. It was changed in this revision:
# 8 weeks ago: x86/hvm: fix extra size passed to __trace_var()
# revision 10: 9cebb977e9d8 (diff) (annotate)
# author: Keir Fraser <keir.fraser@citrix.com>
# date: Mon Sep 20 18:53:18 2010 +0100
I think this means most of the extra_words checks are bogus now, unless
the same change also goes into the 4.0 branch.
What should we do about this difference in tracedata?
Olaf
--- a/xenalyze.c Wed Nov 10 14:56:56 2010 +0000
+++ b/xenalyze.c Wed Nov 10 14:58:31 2010 +0000
@@ -4828,8 +4828,8 @@ void hvm_vmexit_process(struct record_in
};
} *r;
- if(ri->extra_words != 4
- && ri->extra_words != 3
+ if(ri->extra_words != 3
+ && ri->extra_words != 2
)
{
fprintf(warn, "FATAL: vmexit has unexpected extra words
%d!\n",
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Xenalyze should do a Xen version check and do the appropriate thing for 4.0 and earlier versus 4.1 and later. Changing visible behaviour of a Xen stable branch will just add to the confusion. -- Keir On 19/11/2010 09:23, "Olaf Hering" <olaf@aepfle.de> wrote:> George, > > what is the reason behind this changeset? > http://xenbits.xensource.com/ext/xenalyze.hg?rev/9fa7e4d2a3af > > All my vmexit trace entries have size 4 for 64bit and 3 for 32bit. > Looking at the code in ./xen/arch/x86/hvm/vmx/vmx.c, HVMTRACE_ND() gets > size 3 for VMEXIT64. But HVMTRACE_ND does a ''sizeof(u32)*count+1'' in > xen-4.0. > The xen-unstable macro looks different. It was changed in this revision: > > # 8 weeks ago: x86/hvm: fix extra size passed to __trace_var() > # revision 10: 9cebb977e9d8 (diff) (annotate) > # author: Keir Fraser <keir.fraser@citrix.com> > # date: Mon Sep 20 18:53:18 2010 +0100 > > I think this means most of the extra_words checks are bogus now, unless > the same change also goes into the 4.0 branch. > > What should we do about this difference in tracedata? > > > Olaf > > --- a/xenalyze.c Wed Nov 10 14:56:56 2010 +0000 > +++ b/xenalyze.c Wed Nov 10 14:58:31 2010 +0000 > @@ -4828,8 +4828,8 @@ void hvm_vmexit_process(struct record_in > }; > } *r; > > - if(ri->extra_words != 4 > - && ri->extra_words != 3 > + if(ri->extra_words != 3 > + && ri->extra_words != 2 > ) > { > fprintf(warn, "FATAL: vmexit has unexpected extra words %d!\n", > > > _______________________________________________ > 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
Yeah, doing some archeology... looks like the trace size for the HVMTRACE_ND macro is has an off-by-one error since domain_id and vcpu_id were removed from HVM trace records in September 2008. Tracing is definitely not a stable ABI between releases, but it should be reasonably stable after a release. Originally I tried having xenalyze detect and deal with different trace layouts, but it made the code just too much of a mess. I could have simply tagged xenalyze on a release, but then future improvements to xenalyze wouldn''t be able to be used on released versions of Xen. My most recent attempt is to have xenalyze always be compatible with -unstable, but to have back-patches (found in xenalyze.hg/back-patches/) to change the file format to earlier versions. But obviously that requires some more vigilance to making and testing back-patches for previous versions. If you have a better idea, I''m open to it. A bit more discipline -- doing an audit of the tracing after the feature freeze before each release -- would be helpful; some automated testing would be even more helpful. -George On 19/11/10 09:34, Keir Fraser wrote:> Xenalyze should do a Xen version check and do the appropriate thing for 4.0 > and earlier versus 4.1 and later. Changing visible behaviour of a Xen stable > branch will just add to the confusion. > > -- Keir > > On 19/11/2010 09:23, "Olaf Hering"<olaf@aepfle.de> wrote: > >> George, >> >> what is the reason behind this changeset? >> http://xenbits.xensource.com/ext/xenalyze.hg?rev/9fa7e4d2a3af >> >> All my vmexit trace entries have size 4 for 64bit and 3 for 32bit. >> Looking at the code in ./xen/arch/x86/hvm/vmx/vmx.c, HVMTRACE_ND() gets >> size 3 for VMEXIT64. But HVMTRACE_ND does a ''sizeof(u32)*count+1'' in >> xen-4.0. >> The xen-unstable macro looks different. It was changed in this revision: >> >> # 8 weeks ago: x86/hvm: fix extra size passed to __trace_var() >> # revision 10: 9cebb977e9d8 (diff) (annotate) >> # author: Keir Fraser<keir.fraser@citrix.com> >> # date: Mon Sep 20 18:53:18 2010 +0100 >> >> I think this means most of the extra_words checks are bogus now, unless >> the same change also goes into the 4.0 branch. >> >> What should we do about this difference in tracedata? >> >> >> Olaf >> >> --- a/xenalyze.c Wed Nov 10 14:56:56 2010 +0000 >> +++ b/xenalyze.c Wed Nov 10 14:58:31 2010 +0000 >> @@ -4828,8 +4828,8 @@ void hvm_vmexit_process(struct record_in >> }; >> } *r; >> >> - if(ri->extra_words != 4 >> -&& ri->extra_words != 3 >> + if(ri->extra_words != 3 >> +&& ri->extra_words != 2 >> ) >> { >> fprintf(warn, "FATAL: vmexit has unexpected extra words %d!\n", >> >> >> _______________________________________________ >> 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
On 19/11/2010 09:50, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:> Yeah, doing some archeology... looks like the trace size for the > HVMTRACE_ND macro is has an off-by-one error since domain_id and vcpu_id > were removed from HVM trace records in September 2008. > > Tracing is definitely not a stable ABI between releases, but it should > be reasonably stable after a release. > > Originally I tried having xenalyze detect and deal with different trace > layouts, but it made the code just too much of a mess. I could have > simply tagged xenalyze on a release, but then future improvements to > xenalyze wouldn''t be able to be used on released versions of Xen.If there are a broader range of incompatibilties between 4.0 and 4.1 then that makes sense. If the interface is pretty stable now, and it''s just one or two things like this, then maybe it was worth handling automatically. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, Nov 19, George Dunlap wrote:> If you have a better idea, I''m open to it. A bit more discipline -- > doing an audit of the tracing after the feature freeze before each > release -- would be helpful; some automated testing would be even > more helpful.I think the extra u32 is just padding and can be ignored. Whats the purpose of the ->extra_bytes checks? If its just a data integrity check, it can be removed because reading past the end of the record data should not harm. Maybe limit the loops which iterate ->extra_bytes to 7 because more doesnt fit in a trace record. Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
A lot of times not only the size of the record changes across releases, but the ordering of the content. Checking the sizes (and also things like checking for impossible values; e.g., RIP values that can''t actually be generated on real hardware) is meant so that a file change will probably cause a meaningful warning immediately, rather than just having strange results and me spending half a day trying to figure out what''s causing unexpected behavior, only to find that it was a change in the trace file layout I didn''t catch. We can easily set it up so that these kinds of sanity checks don''t stop xenalyze from processing -- in fact, the basic infrastructure is already there for classifying errors. I''ll post something next week that classifies unexpected sizes differently and allows Xen to keep processing the records. Olaf: BTW, the off-by-one count won''t result in short records; the trace code will copy as many bytes as it''s told into the trace buffer; so the integrity of the record metadata is maintained. (Technically, I suppose some information from the stack could leak; probably not a big issue, given that only root in dom0 can take traces.) -George On Fri, Nov 19, 2010 at 10:07 AM, Olaf Hering <olaf@aepfle.de> wrote:> On Fri, Nov 19, George Dunlap wrote: > >> If you have a better idea, I''m open to it. A bit more discipline -- >> doing an audit of the tracing after the feature freeze before each >> release -- would be helpful; some automated testing would be even >> more helpful. > > I think the extra u32 is just padding and can be ignored. > Whats the purpose of the ->extra_bytes checks? If its just a data > integrity check, it can be removed because reading past the end of the > record data should not harm. Maybe limit the loops which iterate > ->extra_bytes to 7 because more doesnt fit in a trace record. > > > Olaf > > > _______________________________________________ > 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