wrong vmexit size in xenalyze

wrong vmexit size in xenalyze

[Olaf Hering]

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

Re: wrong vmexit size in xenalyze

[Keir Fraser]

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

Re: wrong vmexit size in xenalyze

[George Dunlap]

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

Re: wrong vmexit size in xenalyze

[Keir Fraser]

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

Re: wrong vmexit size in xenalyze

[Olaf Hering]

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

Re: wrong vmexit size in xenalyze

[George Dunlap]

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
>