[Xen-devel] [PATCH 0/2] PV shim timekeeping fixes

[Xen-devel] [PATCH 0/2] PV shim timekeeping fixes

[Igor Druzhinin]

Igor Druzhinin (2):
  x86/shim: suspend and resume platform time correctly
  x86/time: report correct frequency of Xen PV clocksource

 xen/arch/x86/pv/shim.c |  7 ++++++-
 xen/arch/x86/time.c    | 29 ++++++++++-------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] x86/shim: suspend and resume platform time correctly

[Igor Druzhinin]

Similarly to S3, platform time needs to be saved on guest suspend
and restored on resume respectively. This should account for expected
jumps in PV clock counter value after resume. time_suspend/resume()
are safe to use in PVH setting as is since any existing operations
with PIT that they do would simply be ignored there.

Additionally, add resume callback for Xen PV clocksource to avoid
its breakage on migration.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 xen/arch/x86/pv/shim.c |  7 ++++++-
 xen/arch/x86/time.c    | 12 +++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 7a898fd..6b26eaa 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -325,9 +325,13 @@ int pv_shim_shutdown(uint8_t reason)
         if ( v != current )
             vcpu_pause_by_systemcontroller(v);
 
+    /* Prepare timekeeping code to suspend.*/
+    time_suspend();
+
     rc = xen_hypercall_shutdown(SHUTDOWN_suspend);
     if ( rc )
     {
+        time_resume();
         for_each_vcpu ( d, v )
             if ( v != current )
                 vcpu_unpause_by_systemcontroller(v);
@@ -335,8 +339,9 @@ int pv_shim_shutdown(uint8_t reason)
         return rc;
     }
 
-    /* Resume the shim itself first. */
+    /* Resume the shim itself and timekeeping first. */
     hypervisor_resume();
+    time_resume();
 
     /*
      * ATM there's nothing Xen can do if the console/store pfn changes,
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index f6b26f8..7e7a62e 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -565,6 +565,7 @@ static struct platform_timesource __initdata plt_tsc =
  *
  * Xen clock source is a variant of TSC source.
  */
+static uint64_t xen_timer_last;
 
 static uint64_t xen_timer_cpu_frequency(void)
 {
@@ -610,7 +611,6 @@ static uint64_t read_xen_timer(void)
     uint32_t version;
     uint64_t ret;
     uint64_t last;
-    static uint64_t last_value;
 
     do {
         version = info->version & ~1;
@@ -626,20 +626,26 @@ static uint64_t read_xen_timer(void)
 
     /* Maintain a monotonic global value */
     do {
-        last = read_atomic(&last_value);
+        last = read_atomic(&xen_timer_last);
         if ( ret < last )
             return last;
-    } while ( unlikely(cmpxchg(&last_value, last, ret) != last) );
+    } while ( unlikely(cmpxchg(&xen_timer_last, last, ret) != last) );
 
     return ret;
 }
 
+static void resume_xen_timer(struct platform_timesource *pts)
+{
+    write_atomic(&xen_timer_last, 0);
+}
+
 static struct platform_timesource __initdata plt_xen_timer =
 {
     .id = "xen",
     .name = "XEN PV CLOCK",
     .read_counter = read_xen_timer,
     .init = init_xen_timer,
+    .resume = resume_xen_timer,
     .counter_bits = 63,
 };
 #endif
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] x86/time: report correct frequency of Xen PV clocksource

[Igor Druzhinin]

The value of the counter represents the number of nanoseconds
since host boot. That means the correct frequency is always 1GHz.

This inconsistency caused time to go slower in PV shim on most
platforms.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 xen/arch/x86/time.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 7e7a62e..95840c4 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -567,27 +567,12 @@ static struct platform_timesource __initdata plt_tsc =
  */
 static uint64_t xen_timer_last;
 
-static uint64_t xen_timer_cpu_frequency(void)
-{
-    struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
-    uint64_t freq;
-
-    freq = 1000000000ULL << 32;
-    do_div(freq, info->tsc_to_system_mul);
-    if ( info->tsc_shift < 0 )
-        freq <<= -info->tsc_shift;
-    else
-        freq >>= info->tsc_shift;
-
-    return freq;
-}
-
 static int64_t __init init_xen_timer(struct platform_timesource *pts)
 {
     if ( !xen_guest )
         return 0;
 
-    pts->frequency = xen_timer_cpu_frequency();
+    pts->frequency = 1000000000ULL;
 
     return pts->frequency;
 }
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] x86/shim: suspend and resume platform time correctly

[Roger Pau Monné]

On Tue, Feb 04, 2020 at 03:40:24PM +0000, Igor Druzhinin wrote:
> Similarly to S3, platform time needs to be saved on guest suspend
> and restored on resume respectively. This should account for expected
> jumps in PV clock counter value after resume. time_suspend/resume()
> are safe to use in PVH setting as is since any existing operations
> with PIT that they do would simply be ignored there.

There's also an attempt to fiddle with HPET, which I think it's just a
no-op.

Just to be on the safe side it might be better to pass a new parameter
to time_resume in order to signal whether PIT/HPET should even be
attempted to be resumed?

The rest LGTM, thanks for tracking this down.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/time: report correct frequency of Xen PV clocksource

[Roger Pau Monné]

On Tue, Feb 04, 2020 at 03:40:25PM +0000, Igor Druzhinin wrote:
> The value of the counter represents the number of nanoseconds
> since host boot. That means the correct frequency is always 1GHz.
> 
> This inconsistency caused time to go slower in PV shim on most
> platforms.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

With one nit below.

> ---
>  xen/arch/x86/time.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 7e7a62e..95840c4 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -567,27 +567,12 @@ static struct platform_timesource __initdata plt_tsc =
>   */
>  static uint64_t xen_timer_last;
>  
> -static uint64_t xen_timer_cpu_frequency(void)
> -{
> -    struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
> -    uint64_t freq;
> -
> -    freq = 1000000000ULL << 32;
> -    do_div(freq, info->tsc_to_system_mul);
> -    if ( info->tsc_shift < 0 )
> -        freq <<= -info->tsc_shift;
> -    else
> -        freq >>= info->tsc_shift;
> -
> -    return freq;
> -}
> -
>  static int64_t __init init_xen_timer(struct platform_timesource *pts)
>  {
>      if ( !xen_guest )
>          return 0;
>  
> -    pts->frequency = xen_timer_cpu_frequency();
> +    pts->frequency = 1000000000ULL;

You can init this field below at declaration, since it's a static
value.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] x86/shim: suspend and resume platform time correctly

[Igor Druzhinin]

On 04/02/2020 17:17, Roger Pau Monné wrote:
> On Tue, Feb 04, 2020 at 03:40:24PM +0000, Igor Druzhinin wrote:
>> Similarly to S3, platform time needs to be saved on guest suspend
>> and restored on resume respectively. This should account for expected
>> jumps in PV clock counter value after resume. time_suspend/resume()
>> are safe to use in PVH setting as is since any existing operations
>> with PIT that they do would simply be ignored there.
> 
> There's also an attempt to fiddle with HPET, which I think it's just a
> no-op.
> 
> Just to be on the safe side it might be better to pass a new parameter
> to time_resume in order to signal whether PIT/HPET should even be
> attempted to be resumed?

Both of preinit_pit() and _disable_pit_irq() already called in PV shim
during boot. So it might be better to include a condition right inside
them to cover that case as well.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86/time: report correct frequency of Xen PV clocksource

[Igor Druzhinin]

On 04/02/2020 17:28, Roger Pau Monné wrote:
> On Tue, Feb 04, 2020 at 03:40:25PM +0000, Igor Druzhinin wrote:
>> The value of the counter represents the number of nanoseconds
>> since host boot. That means the correct frequency is always 1GHz.
>>
>> This inconsistency caused time to go slower in PV shim on most
>> platforms.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Actually, this patch is buggy and causes a system to always detect 1GHz
processor. I'll send a v2.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] x86/shim: suspend and resume platform time correctly

[Igor Druzhinin]

On 04/02/2020 17:43, Igor Druzhinin wrote:
> On 04/02/2020 17:17, Roger Pau Monné wrote:
>> On Tue, Feb 04, 2020 at 03:40:24PM +0000, Igor Druzhinin wrote:
>>> Similarly to S3, platform time needs to be saved on guest suspend
>>> and restored on resume respectively. This should account for expected
>>> jumps in PV clock counter value after resume. time_suspend/resume()
>>> are safe to use in PVH setting as is since any existing operations
>>> with PIT that they do would simply be ignored there.
>>
>> There's also an attempt to fiddle with HPET, which I think it's just a
>> no-op.
>>
>> Just to be on the safe side it might be better to pass a new parameter
>> to time_resume in order to signal whether PIT/HPET should even be
>> attempted to be resumed?
> 
> Both of preinit_pit() and _disable_pit_irq() already called in PV shim
> during boot. So it might be better to include a condition right inside
> them to cover that case as well.

On second thought, it might not be exactly true that those devices are
non-present (at least for potential PV-in-HVM case).
So I'll leave it as is for v2.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel