[PATCH] x86: don't pass negative time to gtime_to_gtsc() (try 2)

[PATCH] x86: don't pass negative time to gtime_to_gtsc() (try 2)

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

This mostly reverts commit eb60be3d ("x86: don't pass negative time to
gtime_to_gtsc()") and instead corrects __update_vcpu_system_time()'s
handling of this_cpu(cpu_time).stime_local_stamp dating back before the
start of a HVM guest (which would otherwise lead to a negative value
getting passed to gtime_to_gtsc(), causing scale_delta() to produce
meaningless output).

Flushing the value to zero was wrong, and printing a message for
something that can validly happen wasn't very useful either.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -823,16 +823,13 @@ static void __update_vcpu_system_time(st
             struct pl_time *pl = &v->domain->arch.hvm_domain.pl_time;
 
             stime += pl->stime_offset + v->arch.hvm_vcpu.stime_offset;
-            if ( (s64)stime < 0 )
-            {
-                printk(XENLOG_G_WARNING "d%dv%d: bogus time %" PRId64
-                       " (offsets %" PRId64 "/%" PRId64 ")\n",
-                       d->domain_id, v->vcpu_id, stime,
-		       pl->stime_offset, v->arch.hvm_vcpu.stime_offset);
-                stime = 0;
-            }
+            if ( stime >= 0 )
+                tsc_stamp = gtime_to_gtsc(d, stime);
+            else
+                tsc_stamp = -gtime_to_gtsc(d, -stime);
         }
-        tsc_stamp = gtime_to_gtsc(d, stime);
+        else
+            tsc_stamp = gtime_to_gtsc(d, stime);
     }
     else
     {




[-- Attachment #2: x86-HVM-early-vcpu-system-time.patch --]
[-- Type: text/plain, Size: 1552 bytes --]

x86: don't pass negative time to gtime_to_gtsc() (try 2)

This mostly reverts commit eb60be3d ("x86: don't pass negative time to
gtime_to_gtsc()") and instead corrects __update_vcpu_system_time()'s
handling of this_cpu(cpu_time).stime_local_stamp dating back before the
start of a HVM guest (which would otherwise lead to a negative value
getting passed to gtime_to_gtsc(), causing scale_delta() to produce
meaningless output).

Flushing the value to zero was wrong, and printing a message for
something that can validly happen wasn't very useful either.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -823,16 +823,13 @@ static void __update_vcpu_system_time(st
             struct pl_time *pl = &v->domain->arch.hvm_domain.pl_time;
 
             stime += pl->stime_offset + v->arch.hvm_vcpu.stime_offset;
-            if ( (s64)stime < 0 )
-            {
-                printk(XENLOG_G_WARNING "d%dv%d: bogus time %" PRId64
-                       " (offsets %" PRId64 "/%" PRId64 ")\n",
-                       d->domain_id, v->vcpu_id, stime,
-		       pl->stime_offset, v->arch.hvm_vcpu.stime_offset);
-                stime = 0;
-            }
+            if ( stime >= 0 )
+                tsc_stamp = gtime_to_gtsc(d, stime);
+            else
+                tsc_stamp = -gtime_to_gtsc(d, -stime);
         }
-        tsc_stamp = gtime_to_gtsc(d, stime);
+        else
+            tsc_stamp = gtime_to_gtsc(d, stime);
     }
     else
     {

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86: don't pass negative time to gtime_to_gtsc() (try 2)

[Keir Fraser]

On 10/06/2013 13:15, "Jan Beulich" <JBeulich@suse.com> wrote:

> This mostly reverts commit eb60be3d ("x86: don't pass negative time to
> gtime_to_gtsc()") and instead corrects __update_vcpu_system_time()'s
> handling of this_cpu(cpu_time).stime_local_stamp dating back before the
> start of a HVM guest (which would otherwise lead to a negative value
> getting passed to gtime_to_gtsc(), causing scale_delta() to produce
> meaningless output).
> 
> Flushing the value to zero was wrong, and printing a message for
> something that can validly happen wasn't very useful either.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -823,16 +823,13 @@ static void __update_vcpu_system_time(st
>              struct pl_time *pl = &v->domain->arch.hvm_domain.pl_time;
>  
>              stime += pl->stime_offset + v->arch.hvm_vcpu.stime_offset;
> -            if ( (s64)stime < 0 )
> -            {
> -                printk(XENLOG_G_WARNING "d%dv%d: bogus time %" PRId64
> -                       " (offsets %" PRId64 "/%" PRId64 ")\n",
> -                       d->domain_id, v->vcpu_id, stime,
> -         pl->stime_offset, v->arch.hvm_vcpu.stime_offset);
> -                stime = 0;
> -            }
> +            if ( stime >= 0 )
> +                tsc_stamp = gtime_to_gtsc(d, stime);
> +            else
> +                tsc_stamp = -gtime_to_gtsc(d, -stime);
>          }
> -        tsc_stamp = gtime_to_gtsc(d, stime);
> +        else
> +            tsc_stamp = gtime_to_gtsc(d, stime);
>      }
>      else
>      {
> 
> 
> 

Re: [PATCH] x86: don't pass negative time to gtime_to_gtsc() (try 2)

[George Dunlap]

On Mon, Jun 10, 2013 at 1:15 PM, Jan Beulich <JBeulich@suse.com> wrote:
> This mostly reverts commit eb60be3d ("x86: don't pass negative time to
> gtime_to_gtsc()") and instead corrects __update_vcpu_system_time()'s
> handling of this_cpu(cpu_time).stime_local_stamp dating back before the
> start of a HVM guest (which would otherwise lead to a negative value
> getting passed to gtime_to_gtsc(), causing scale_delta() to produce
> meaningless output).
>
> Flushing the value to zero was wrong, and printing a message for
> something that can validly happen wasn't very useful either.

Has this actually caused problems, or is this just a theoretical fix?

 -George

Re: [PATCH] x86: don't pass negative time to gtime_to_gtsc() (try 2)

[Jan Beulich]

>>> On 10.06.13 at 15:44, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Mon, Jun 10, 2013 at 1:15 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> This mostly reverts commit eb60be3d ("x86: don't pass negative time to
>> gtime_to_gtsc()") and instead corrects __update_vcpu_system_time()'s
>> handling of this_cpu(cpu_time).stime_local_stamp dating back before the
>> start of a HVM guest (which would otherwise lead to a negative value
>> getting passed to gtime_to_gtsc(), causing scale_delta() to produce
>> meaningless output).
>>
>> Flushing the value to zero was wrong, and printing a message for
>> something that can validly happen wasn't very useful either.
> 
> Has this actually caused problems, or is this just a theoretical fix?

The commit this undoes was done because of a crash observed
on the test infrastructure. Recently, the log message that got
added there was found in another test infrastructure log, getting
me to (hopefully) understand what the underlying issue is, leading
to the fix here.

Jan

Re: [PATCH] x86: don't pass negative time to gtime_to_gtsc() (try 2)

[George Dunlap]

On 10/06/13 14:48, Jan Beulich wrote:
>>>> On 10.06.13 at 15:44, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>> On Mon, Jun 10, 2013 at 1:15 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>> This mostly reverts commit eb60be3d ("x86: don't pass negative time to
>>> gtime_to_gtsc()") and instead corrects __update_vcpu_system_time()'s
>>> handling of this_cpu(cpu_time).stime_local_stamp dating back before the
>>> start of a HVM guest (which would otherwise lead to a negative value
>>> getting passed to gtime_to_gtsc(), causing scale_delta() to produce
>>> meaningless output).
>>>
>>> Flushing the value to zero was wrong, and printing a message for
>>> something that can validly happen wasn't very useful either.
>> Has this actually caused problems, or is this just a theoretical fix?
> The commit this undoes was done because of a crash observed
> on the test infrastructure. Recently, the log message that got
> added there was found in another test infrastructure log, getting
> me to (hopefully) understand what the underlying issue is, leading
> to the fix here.

The thing is this:  That line "tsc_stamp = -gtime_to_tsc(-stime)" looks 
risky; it's the kind of logic that is easy to get wrong (like the "^val" 
instead of "&~val" thing).

The code as it is has been tested from April until now and nobody has 
complained.  With this fix, we're starting over from scratch on our 
"testing clock".

So unless this is known to cause an actual problem (other than just 
unnecessary console messages), I'd be inclined to say it needs to wait 
until 4.3.1.

  -George

Re: [PATCH] x86: don't pass negative time to gtime_to_gtsc() (try 2)

[Jan Beulich]

>>> On 10.06.13 at 15:53, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 10/06/13 14:48, Jan Beulich wrote:
>>>>> On 10.06.13 at 15:44, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
>>> On Mon, Jun 10, 2013 at 1:15 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> This mostly reverts commit eb60be3d ("x86: don't pass negative time to
>>>> gtime_to_gtsc()") and instead corrects __update_vcpu_system_time()'s
>>>> handling of this_cpu(cpu_time).stime_local_stamp dating back before the
>>>> start of a HVM guest (which would otherwise lead to a negative value
>>>> getting passed to gtime_to_gtsc(), causing scale_delta() to produce
>>>> meaningless output).
>>>>
>>>> Flushing the value to zero was wrong, and printing a message for
>>>> something that can validly happen wasn't very useful either.
>>> Has this actually caused problems, or is this just a theoretical fix?
>> The commit this undoes was done because of a crash observed
>> on the test infrastructure. Recently, the log message that got
>> added there was found in another test infrastructure log, getting
>> me to (hopefully) understand what the underlying issue is, leading
>> to the fix here.
> 
> The thing is this:  That line "tsc_stamp = -gtime_to_tsc(-stime)" looks 
> risky; it's the kind of logic that is easy to get wrong (like the "^val" 
> instead of "&~val" thing).
> 
> The code as it is has been tested from April until now and nobody has 
> complained.  With this fix, we're starting over from scratch on our 
> "testing clock".
> 
> So unless this is known to cause an actual problem (other than just 
> unnecessary console messages), I'd be inclined to say it needs to wait 
> until 4.3.1.

While I don't see as much of a risk here as you do, I'm fine with
holding this back if you prefer so.

Jan