Re: [PATCH v2 01/10] hvm/hpet: Add manual unit test code.

[Don Slutz <dslutz@verizon.com>]

On 04/10/14 02:11, Jan Beulich wrote:
>>>> On 09.04.14 at 20:35,<dslutz@verizon.com>  wrote:

>> In a xen source tree (with this patch applied):
>>
>>     cd tools/tests/vhpet
>>
>>     xen_source=../../..
>>     sed -e "/#include/d" -e "1i#include \"emul.h\"\n" <$xen_source/xen/arch/x86/hvm/hpet.c >hpet.c
>>     cp $xen_source/xen/include/asm-x86/hpet.h .
>>
>>     gcc -g -o test_vhpet hpet.c main.c
> At least up to here this clearly should be in a Makefile, just like e.g.
> done for the x86 instruction emulator test.

Opps, I just noticed that the "optional" got dropped.  I had not planned
on this being required to get these patches applied.  I have not found
a simple way to use git-format-patch and have just 1 subject different.

Will work on using a makefile.  I will be happy to make this into a much
better test case. Since that will not happen quickly, it make sense to me
to drop just this 1 patch and start a new set that includes it.

I have no real clue on how to get it to be part of "normal" testing.

This is because "make test" gives me:

...
building 'checkpoint' extension
gcc -pthread -fno-strict-aliasing -O2 -g -pipe -Wall 
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector 
--param=ssp-buffer-size=4 -m64 -mtune=generic -D_GNU_SOURCE -fPIC 
-fwrapv -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic 
-D_GNU_SOURCE -fPIC -fwrapv -fPIC -I../../tools/include 
-I../../tools/libxc -I../../tools/xenstore -I/usr/include/python2.7 -c 
xen/lowlevel/checkpoint/checkpoint.c -o 
build/temp.linux-x86_64-2.7/xen/lowlevel/checkpoint/checkpoint.o 
-fno-strict-aliasing -Werror
gcc -pthread -fno-strict-aliasing -O2 -g -pipe -Wall 
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector 
--param=ssp-buffer-size=4 -m64 -mtune=generic -D_GNU_SOURCE -fPIC 
-fwrapv -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic 
-D_GNU_SOURCE -fPIC -fwrapv -fPIC -I../../tools/include 
-I../../tools/libxc -I../../tools/xenstore -I/usr/include/python2.7 -c 
xen/lowlevel/checkpoint/libcheckpoint.c -o 
build/temp.linux-x86_64-2.7/xen/lowlevel/checkpoint/libcheckpoint.o 
-fno-strict-aliasing -Werror
xen/lowlevel/checkpoint/libcheckpoint.c: In function 'stop_suspend_thread':
xen/lowlevel/checkpoint/libcheckpoint.c:842:7: error: variable 'err' set 
but not used [-Werror=unused-but-set-variable]
cc1: all warnings being treated as errors
error: command 'gcc' failed with exit status 1
Build failed 0x100
make[1]: *** [test] Error 1
make[1]: Leaving directory `/home/don/xen/tools/python'
make: *** [test] Error 2
dcs-xen-54:~/xen>make tests
make: *** No rule to make target `tests'.  Stop.
dcs-xen-54:~/xen>make check
make: *** No rule to make target `check'.  Stop.
dcs-xen-54:~/xen>e Makefile
dcs-xen-54:~/xen>make -C tools test
make: Entering directory `/home/don/xen/tools'
make: *** No rule to make target `test'.  Stop.
make: Leaving directory `/home/don/xen/tools'


And 4.3.1 on CentOS 5.10:

...
======================================================================
ERROR: Invalid Test (xen.xm.tests.test_create)
----------------------------------------------------------------------
Traceback (most recent call last):
   File "test.py", line 634, in get_suite
     mod = package_import(modname)
   File "test.py", line 608, in package_import
     mod = __import__(modname)
   File 
"/home/don/xen-4.3.1/tools/python/build/lib.linux-x86_64-2.4/xen/xm/tests/test_create.py", 
line 6, in ?
     import xen.xend.XendOptions
   File 
"/home/don/xen-4.3.1/tools/python/build/lib.linux-x86_64-2.4/xen/xend/XendOptions.py", 
line 35, in ?
     from xen.util import auxbin
   File 
"/home/don/xen-4.3.1/tools/python/build/lib.linux-x86_64-2.4/xen/util/auxbin.py", 
line 22, in ?
     from xen.util.path import *
ImportError: No module named path

----------------------------------------------------------------------
Ran 5 tests in 0.048s

FAILED (errors=3)
make[1]: Leaving directory `/home/don/xen-4.3.1/tools/python'





>>     ./test_vhpet >foo
>>
>>
>> Most of this is in main.c's comment.  Will add to commit message also.
> Just referring to the comments there would be enough I guess. But
> I'm still struggling to see how this would reproduce the Linux side
> issue, and not just some random problem manifesting in similar
> symptoms.

I will attempt to answer this.


During my looking for what was happening, I added debug code
that would allow me to force "diff" to selected values.  This was
how I found out that "-diff > HPET_TINY_TIME_SPAN" would cause
linux to report this and crash.  On closer looking into this, I was
able to determine that the use extInt path would also fail even if I
got xen to provide this interrupt.  The reason being that

  (uint32_t)(-HPET_TINY_TIME_SPAN-1)

Sets the  "delta" (ns) to more then 60 seconds in the future.  And
the timer test happens in the 1st 5 seconds of a linux boot on my
test server.

So I send out the v1 patch.

Later I found out that any value that is > ~3 seconds would also
cause a linux crash even if xen is changed to provide extInt
interrupts.  (And you said in:

http://lists.xen.org/archives/html/xen-devel/2014-02/msg01857.html

But in the end I think you're barking up the wrong tree: All of this
code would be of no interest at all if Linux didn't for some reason go
into that fallback code. And with that fallback code existing mainly
(if not exclusively) to deal with (half-)broken hardware/firmware, we
should really make sure our emulation isn't broken (i.e. the fallback
never being made use of). So finding the "TBD reason" is what is
really needed.


(This e-mail also has the way I was able to get xen to "handle" extInt.
and so far this "adjustment" to xen is not ready to be posted as a fix;
including a good reason to add the code.)


This caused me to send lots of time reading the hpet spec
and hopefully understanding most of it.  At the same time
(and while waiting for a longer running test of Windows Vista)
I would add additional debug output and trace output along with
changes.  The the turn around time was slow enough that
it made sense to me to take the code and run it standalone.

This is what this code does.  It allows you to use an almost unmodified
copy of hpet.c in a stand alone mode that needs nothing else from
the xen source tree nor even have xen installed.

Also during the long test time I noticed that using xen-hvmctx
the output of cmp was not changing, but master clock was.
This is what leads me to patch #7:

hvm/hpet: Call hpet_get_comparator during hpet_save.

Which is not needed do to the way hpet_get_comparator() works
until patch #10:

hvm/hpet: handle 1st period special

Which will not work correctly without patch #7.

Also during the long time run I found out  using "xentrace: Add
TRC_HVM_EMUL" (at the testing time it was TRC_HW_VCHIP, but
that is mostly a rename) and some hvm_debug code I had added
showed that the delta between guest_time_hpet() was offten ~200.
However I would some times get much larger values.

I also found out that linux expects that "delta" should be "period".

This was the key to finding the issue that patch #3:

hvm/hpet: Only set comparator or period not both

Fixes.  However since the spec says that "delta" can be any
relation to  "period"; using what linux wanted ("delta" should
never be greater then "period") would be a bad change.

Since the server that would report:

..MP-BIOS bug: 8254 timer..

Stopped doing this, and we were unable to reproduce this
anywhere else; the best I could do was guess that some how
the delta between the 2 calls to guest_time_hpet() in:


     tn_cmp   = hpet_get_comparator(h, tn);
     cur_tick = hpet_read_maincounter(h);


Would be a possible cause.  It was not until patches #2 to #9
had been coded and tested and I was still looking for a good way
to do patch #10 (most of my attempts would break something
else) was I able to find the right set of conditions to get the test
code to report on a bad "delta".


I am not sure at what time I noticed patch #9:

hvm/hpet: Correctly limit period to a maximum.

This patch set order is not in the order of issues found.  Just
one that makes sense to me and was easy to separate into
only 1 change per patch.

     -Don Slutz

> Jan
>