Re: [Xen-devel] [xen-unstable test] 18092: tolerable FAIL

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.06.13 at 11:52, Tim Deegan <tim@xxxxxxx> wrote:
> At 08:52 +0100 on 07 Jun (1370595174), Jan Beulich wrote:
>> >>> On 07.06.13 at 08:47, xen.org <ian.jackson@xxxxxxxxxxxxx> wrote:
>> > flight 18092 xen-unstable real [real]
>> > http://www.chiark.greenend.org.uk/~xensrcts/logs/18092/ 
>> > 
>> > Failures :-/ but no regressions.
>> > 
>> > Tests which are failing intermittently (not blocking):
>> >  test-amd64-amd64-xl-qemuu-winxpsp3  8 guest-saverestore     fail pass in 
>> > 18090
>> 
>> So commit eb60be3dd870aecfa47bed1118069680389c15f7 ("x86:
>> don't pass negative time to gtime_to_gtsc()") caught something
>> here after the first reboot of the Windows install in the guest:
>> 
>> Jun  7 02:35:44.623032 (XEN) d2v0: bogus time -19766120 (offsets 
> -362881846364/0)
>> 
>> (and many more instances of this during the following about 1.5 sec).
>> 
>> Looking at the involved code again, I realize that pl->stime_offset
>> gets set from calling get_s_time(), yet the calculation in
>> __update_vcpu_system_time() starts from
>> this_cpu(cpu_time).stime_local_stamp, which validly can be before
>> the value the initializing get_s_time() invocation returned. So stime
>> can validly be negative here, and calculating tsc_stamp based on
>> the flushed-to-zero stime value is incorrect (and we really ought to
>> set tsc_timestamp to a value wrapped downwards through zero -
>> question is whether all possible guest calculations would cope with
>> that - Linux'es clearly would).
> 
> Hmm.  The calculation specified in the public header will work: it uses
> plain subtraction on 64-bit unsigned integers.  So for once we can claim
> that the ABI is documented on this point. :)  
> 
> But wait -- this is in an 'is_hvm_domain' block.  I thought PV drivers
> in HVM guests used HVMOP_get_time rather than calculating NOW()
> themselves, because they don't know the TSC offset.  Or is that only on
> Windows, where the TSC is controlled by non-PV parts of the kernel?
> 
> Either way, fixing gtime_to_gtsc() to handle stime < 0 sounds right.

Actually, I don't think that would be the proper course of action -
I continue to think that this function should only be called with
non-negative (i.e. unsigned) deltas. Instead I think the caller should
take care of calling it with the negated stime, and then doing with
the result whatever is appropriate - the question was whether we
can assume that users can deal with the effectively underflowed
TSC stamp that wpuld result here. If, as you say, we take what the
public header has as ABI specification, then we can safely assume so.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel