[PATCH 0/2] x86/cpu: TSC handling adjustments

[PATCH 0/2] x86/cpu: TSC handling adjustments

[Roger Pau Monne]

Hello,

The fist patch fixes a bug if started with TSC_AUX != 0, as then the
cached value won't match the hardware one.

Second patch fixes handling of TSC_ADJUST and TSC offset for HVM
domains, in an attempt to make it consistent with hardware behavior.

Thanks, Roger.

Roger Pau Monne (2):
  x86/cpu: keep cached TSC_AUX state coherent with hardware
  x86/hvm: sync APs TSC with the BSP at creation only

 xen/arch/x86/acpi/power.c |  3 +++
 xen/arch/x86/cpu/common.c |  5 +++++
 xen/arch/x86/hvm/domain.c |  5 -----
 xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
 xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
 xen/arch/x86/time.c       |  5 ++---
 6 files changed, 53 insertions(+), 18 deletions(-)

-- 
2.55.0

[PATCH 1/2] x86/cpu: keep cached TSC_AUX state coherent with hardware

[Roger Pau Monne]

On CPU bringup don't assume that TSC_AUX is 0, write 0 to it to ensure the
per-CPU cached state matches the hardware.  Also propagate the cached value
on resume from suspend, in case the register value has been reset during
suspension.

Fixes: cc0e45db2779 ("x86/pv: Avoid leaking other guests' MSR_TSC_AUX values into PV context")
Signed-off-by: Roger Pau Monné <roger@xenproject.org>
---
 xen/arch/x86/acpi/power.c | 3 +++
 xen/arch/x86/cpu/common.c | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 3452650a61d5..464846e73727 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -279,6 +279,9 @@ static int enter_state(u32 state)
         ci->last_spec_ctrl = default_xen_spec_ctrl;
     }
 
+    if ( cpu_has_msr_tsc_aux )
+        wrmsrns(MSR_TSC_AUX, this_cpu(tsc_aux));
+
     update_mcu_opt_ctrl();
     update_pb_opt_ctrl();
 
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 43785d19cc73..5beae0e87f49 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -926,6 +926,11 @@ void cpu_init(void)
 
 	/* Enable NMIs.  Our loader (e.g. Tboot) may have left them disabled. */
 	enable_nmis();
+
+	if (cpu_has_msr_tsc_aux) {
+		wrmsrns(MSR_TSC_AUX, 0);
+		this_cpu(tsc_aux) = 0;
+	}
 }
 
 void cpu_uninit(unsigned int cpu)
-- 
2.55.0

Re: [PATCH 1/2] x86/cpu: keep cached TSC_AUX state coherent with hardware

[Teddy Astie]

[-- Attachment #1.1: Type: text/plain, Size: 1638 bytes --]

Le 30/10/2026 à 14:26, Roger Pau Monne a écrit :
> On CPU bringup don't assume that TSC_AUX is 0, write 0 to it to ensure the
> per-CPU cached state matches the hardware.  Also propagate the cached value
> on resume from suspend, in case the register value has been reset during
> suspension.
> 
> Fixes: cc0e45db2779 ("x86/pv: Avoid leaking other guests' MSR_TSC_AUX values into PV context")
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> ---
>   xen/arch/x86/acpi/power.c | 3 +++
>   xen/arch/x86/cpu/common.c | 5 +++++
>   2 files changed, 8 insertions(+)
> 
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 3452650a61d5..464846e73727 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -279,6 +279,9 @@ static int enter_state(u32 state)
>           ci->last_spec_ctrl = default_xen_spec_ctrl;
>       }
>   
> +    if ( cpu_has_msr_tsc_aux )
> +        wrmsrns(MSR_TSC_AUX, this_cpu(tsc_aux));
> +
>       update_mcu_opt_ctrl();
>       update_pb_opt_ctrl();
>   
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 43785d19cc73..5beae0e87f49 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -926,6 +926,11 @@ void cpu_init(void)
>   
>   	/* Enable NMIs.  Our loader (e.g. Tboot) may have left them disabled. */
>   	enable_nmis();
> +
> +	if (cpu_has_msr_tsc_aux) {
> +		wrmsrns(MSR_TSC_AUX, 0);
> +		this_cpu(tsc_aux) = 0;
> +	}
>   }
>   
>   void cpu_uninit(unsigned int cpu)

Looks good to me.

Reviewed-by: Teddy Astie <teddy.astie@vates.tech>

Teddy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: [PATCH 1/2] x86/cpu: keep cached TSC_AUX state coherent with hardware

[Andrew Cooper]

On 30/10/2026 1:24 pm, Roger Pau Monne wrote:
> On CPU bringup don't assume that TSC_AUX is 0, write 0 to it to ensure the
> per-CPU cached state matches the hardware.  Also propagate the cached value
> on resume from suspend, in case the register value has been reset during
> suspension.
>
> Fixes: cc0e45db2779 ("x86/pv: Avoid leaking other guests' MSR_TSC_AUX values into PV context")
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> ---
>  xen/arch/x86/acpi/power.c | 3 +++
>  xen/arch/x86/cpu/common.c | 5 +++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 3452650a61d5..464846e73727 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -279,6 +279,9 @@ static int enter_state(u32 state)
>          ci->last_spec_ctrl = default_xen_spec_ctrl;
>      }
>  
> +    if ( cpu_has_msr_tsc_aux )
> +        wrmsrns(MSR_TSC_AUX, this_cpu(tsc_aux));
> +

This wants to be later, with percpu_traps_init().   It is not critical
state to restore this early.

~Andrew

Re: [PATCH 1/2] x86/cpu: keep cached TSC_AUX state coherent with hardware

[Roger Pau Monné]

On Mon, Nov 02, 2026 at 11:51:32AM +0000, Andrew Cooper wrote:
> On 30/10/2026 1:24 pm, Roger Pau Monne wrote:
> > On CPU bringup don't assume that TSC_AUX is 0, write 0 to it to ensure the
> > per-CPU cached state matches the hardware.  Also propagate the cached value
> > on resume from suspend, in case the register value has been reset during
> > suspension.
> >
> > Fixes: cc0e45db2779 ("x86/pv: Avoid leaking other guests' MSR_TSC_AUX values into PV context")
> > Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> > ---
> >  xen/arch/x86/acpi/power.c | 3 +++
> >  xen/arch/x86/cpu/common.c | 5 +++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> > index 3452650a61d5..464846e73727 100644
> > --- a/xen/arch/x86/acpi/power.c
> > +++ b/xen/arch/x86/acpi/power.c
> > @@ -279,6 +279,9 @@ static int enter_state(u32 state)
> >          ci->last_spec_ctrl = default_xen_spec_ctrl;
> >      }
> >  
> > +    if ( cpu_has_msr_tsc_aux )
> > +        wrmsrns(MSR_TSC_AUX, this_cpu(tsc_aux));
> > +
> 
> This wants to be later, with percpu_traps_init().   It is not critical
> state to restore this early.

I've adjust it.

Thanks, Roger.

[PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only

[Roger Pau Monne]

The current logic syncs the APs' TSC with the BSP both at reset and when
setting the vCPU state.  This doesn't match hardware behavior, where APs
are only in sync at platform start.

Move the TSC sync so it's done in hvm_vcpu_initialise(), that's called a
single time at vCPU creation.  This matches the hardware behavior.
INIT-SIPI and VCPUOP_initialise no longer re-sync a vCPU's TSC to vCPU 0's.

On migration the TSC offset for offline vCPUs is not transferred
(hvm_save_cpu_ctxt() skips offline vCPUs).  To avoid an unexpected skew
synchronize offline vCPUs with the first online vCPU, taking into account
the TSC_ADJUST value.

The clearing of TSC_ADJUST is also made consistent with hardware behavior:
clear it on S3 resume, but not across an INIT-SIPI or VCPUOP_initialise.
Additionally, on S3 resume start all vCPUs from a shared TSC reading, so
they are in sync.

Fixes: c31f8336e64e ("[HVM] Initialise AP TSC to same as BP TSC.")
Fixes: 6e0336351ed9 ("x86: Implement TSC adjust feature for HVM guest")
Signed-off-by: Roger Pau Monné <roger@xenproject.org>
---
 xen/arch/x86/hvm/domain.c |  5 -----
 xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
 xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
 xen/arch/x86/time.c       |  5 ++---
 4 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
index 978d8b222256..71f49ec7ce9f 100644
--- a/xen/arch/x86/hvm/domain.c
+++ b/xen/arch/x86/hvm/domain.c
@@ -312,11 +312,6 @@ int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
     hvm_set_segment_register(v, x86_seg_es, &es);
     hvm_set_segment_register(v, x86_seg_tss, &tr);
 
-    /* Sync AP's TSC with BSP's. */
-    v->arch.hvm.cache_tsc_offset =
-        d->vcpu[0]->arch.hvm.cache_tsc_offset;
-    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
-
     paging_update_paging_modes(v);
 
     hvmemul_cancel(v);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ef9c7cd05777..81e90cc8fc8e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1667,6 +1667,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
         /* Init guest TSC to start from zero. */
         hvm_set_guest_tsc(v, 0);
     }
+    else
+    {
+        /* Start in step with vCPU 0, as all CPUs do at power-on. */
+        v->arch.hvm.cache_tsc_offset = d->vcpu[0]->arch.hvm.cache_tsc_offset;
+        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
+    }
 
     return 0;
 
@@ -4021,13 +4027,6 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     hvm_set_segment_register(v, x86_seg_gdt, &reg);
     hvm_set_segment_register(v, x86_seg_idt, &reg);
 
-    /* Sync AP's TSC with BSP's. */
-    v->arch.hvm.cache_tsc_offset =
-        v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
-    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
-
-    v->arch.hvm.msr_tsc_adjust = 0;
-
     paging_update_paging_modes(v);
 
     v->arch.flags |= TF_kernel_mode;
@@ -4079,9 +4078,13 @@ static void hvm_s3_resume(struct domain *d)
     if ( test_and_clear_bool(d->arch.hvm.is_s3_suspended) )
     {
         struct vcpu *v;
+        uint64_t at_tsc = rdtsc();
 
         for_each_vcpu( d, v )
-            hvm_set_guest_tsc(v, 0);
+        {
+            v->arch.hvm.msr_tsc_adjust = 0;
+            hvm_set_guest_tsc_fixed(v, 0, at_tsc);
+        }
         domain_unpause(d);
     }
 }
diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
index 8ab64057064e..65ebfd384d69 100644
--- a/xen/arch/x86/hvm/save.c
+++ b/xen/arch/x86/hvm/save.c
@@ -289,6 +289,7 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
 {
     const struct hvm_save_header *hdr;
     struct hvm_save_descriptor *desc;
+    struct vcpu *v;
     int rc;
 
     if ( d->is_dying )
@@ -302,8 +303,6 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
     rc = arch_hvm_check(d, hdr);
     if ( real )
     {
-        struct vcpu *v;
-
         ASSERT(!rc);
 
         /*
@@ -341,6 +340,37 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
             /* Reset cursor for hvm_load(, true, ). */
             if ( !real )
                 h->cur = 0;
+            else
+            {
+                const struct vcpu *base = NULL;
+
+                /*
+                 * Offline vCPUs have no CPU record.  Adjust the TSC offset
+                 * based on the first online vCPU, taking into account
+                 * TSC_ADJUST which is saved regardless of whether the vCPU is
+                 * offline.
+                 */
+                for_each_vcpu(d, v)
+                    if ( !test_bit(_VPF_down, &v->pause_flags) )
+                    {
+                        base = v;
+                        break;
+                    }
+
+                if ( !base )
+                    return 0;
+
+                for_each_vcpu(d, v)
+                    if ( test_bit(_VPF_down, &v->pause_flags) )
+                    {
+                        v->arch.hvm.cache_tsc_offset =
+                            base->arch.hvm.cache_tsc_offset -
+                            base->arch.hvm.msr_tsc_adjust +
+                            v->arch.hvm.msr_tsc_adjust;
+                        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
+                    }
+            }
+
             return 0;
         }
 
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index f37fddc61e47..79c7123c0a5b 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -3087,9 +3087,8 @@ int tsc_set_info(struct domain *d,
              * set_tsc_offset() is called from hvm_vcpu_initialise() before
              * tsc_set_info(). New vtsc mode may require recomputing TSC
              * offset.
-             * We only need to do this for BSP during initial boot. APs will
-             * call set_tsc_offset() later from hvm_vcpu_reset_state() and they
-             * will sync their TSC to BSP's sync_tsc.
+             * We only need to do this for BSP during initial boot.  APs copy
+             * vCPU 0's offset when created in hvm_vcpu_initialise().
              */
             d->arch.hvm.sync_tsc = rdtsc();
             hvm_set_tsc_offset(d->vcpu[0],
-- 
2.55.0

[PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only

[chunjie.zhu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 7012 bytes --]

On Fri, Oct 30, 2026 at 02:24:19 PM, Roger Pau Monne <roger@xenproject.org> wrote:

> The current logic syncs the APs' TSC with the BSP both at reset and when
> setting the vCPU state.  This doesn't match hardware behavior, where APs
> are only in sync at platform start.

> Move the TSC sync so it's done in hvm_vcpu_initialise(), that's called a
> single time at vCPU creation.  This matches the hardware behavior.
> INIT-SIPI and VCPUOP_initialise no longer re-sync a vCPU's TSC to vCPU 0's.
>
> On migration the TSC offset for offline vCPUs is not transferred
> (hvm_save_cpu_ctxt() skips offline vCPUs).  To avoid an unexpected skew
> synchronize offline vCPUs with the first online vCPU, taking into account
> the TSC_ADJUST value.
>
> The clearing of TSC_ADJUST is also made consistent with hardware behavior:
> clear it on S3 resume, but not across an INIT-SIPI or VCPUOP_initialise.
> Additionally, on S3 resume start all vCPUs from a shared TSC reading, so
> they are in sync.
>
> Fixes: c31f8336e64e ("[HVM] Initialise AP TSC to same as BP TSC.")

This looks wrong.  c31f8336e64e only added the sync to hvm_bringup_ap(),
which no longer exists.  The code removed here comes from bcba667e12e3
(INIT-SIPI) and 192df6f9122d (VCPUOP_initialise).

> Fixes: 6e0336351ed9 ("x86: Implement TSC adjust feature for HVM guest")
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> ---
>  xen/arch/x86/hvm/domain.c |  5 -----
>  xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
>  xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
>  xen/arch/x86/time.c       |  5 ++---
>  4 files changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 978d8b222256..71f49ec7ce9f 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -312,11 +312,6 @@ int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
>      hvm_set_segment_register(v, x86_seg_es, &es);
>      hvm_set_segment_register(v, x86_seg_tss, &tr);
>
> -    /* Sync AP's TSC with BSP's. */
> -    v->arch.hvm.cache_tsc_offset =
> -        d->vcpu[0]->arch.hvm.cache_tsc_offset;
> -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> -
>      paging_update_paging_modes(v);
>
>      hvmemul_cancel(v);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef9c7cd05777..81e90cc8fc8e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1667,6 +1667,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
>          /* Init guest TSC to start from zero. */
>          hvm_set_guest_tsc(v, 0);
>      }
> +    else
> +    {
> +        /* Start in step with vCPU 0, as all CPUs do at power-on. */
> +        v->arch.hvm.cache_tsc_offset = d->vcpu[0]->arch.hvm.cache_tsc_offset;
> +        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> +    }
>
>      return 0;
>
> @@ -4021,13 +4027,6 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
>      hvm_set_segment_register(v, x86_seg_gdt, &reg);
>      hvm_set_segment_register(v, x86_seg_idt, &reg);
>
> -    /* Sync AP's TSC with BSP's. */
> -    v->arch.hvm.cache_tsc_offset =
> -        v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
> -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> -
> -    v->arch.hvm.msr_tsc_adjust = 0;
> -
>      paging_update_paging_modes(v);
>
>      v->arch.flags |= TF_kernel_mode;
> @@ -4079,9 +4078,13 @@ static void hvm_s3_resume(struct domain *d)
>      if ( test_and_clear_bool(d->arch.hvm.is_s3_suspended) )
>      {
>          struct vcpu *v;
> +        uint64_t at_tsc = rdtsc();
>
>          for_each_vcpu( d, v )
> -            hvm_set_guest_tsc(v, 0);
> +        {
> +            v->arch.hvm.msr_tsc_adjust = 0;
> +            hvm_set_guest_tsc_fixed(v, 0, at_tsc);
> +        }
>          domain_unpause(d);
>      }
>  }
> diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
> index 8ab64057064e..65ebfd384d69 100644
> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -289,6 +289,7 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
>  {
>      const struct hvm_save_header *hdr;
>      struct hvm_save_descriptor *desc;
> +    struct vcpu *v;
>      int rc;
>
>      if ( d->is_dying )
> @@ -302,8 +303,6 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
>      rc = arch_hvm_check(d, hdr);
>      if ( real )
>      {
> -        struct vcpu *v;
> -
>          ASSERT(!rc);
>
>          /*
> @@ -341,6 +340,37 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
>              /* Reset cursor for hvm_load(, true, ). */
>              if ( !real )
>                  h->cur = 0;
> +            else
> +            {
> +                const struct vcpu *base = NULL;
> +
> +                /*
> +                 * Offline vCPUs have no CPU record.  Adjust the TSC offset
> +                 * based on the first online vCPU, taking into account
> +                 * TSC_ADJUST which is saved regardless of whether the vCPU is
> +                 * offline.
> +                 */
> +                for_each_vcpu(d, v)
> +                    if ( !test_bit(_VPF_down, &v->pause_flags) )
> +                    {
> +                        base = v;
> +                        break;
> +                    }
> +
> +                if ( !base )
> +                    return 0;

This silently skips the offline vCPUs.  If that ever causes a problem it
will be hard to diagnose, so please add a comment or a log message.

> +
> +                for_each_vcpu(d, v)
> +                    if ( test_bit(_VPF_down, &v->pause_flags) )
> +                    {
> +                        v->arch.hvm.cache_tsc_offset =
> +                            base->arch.hvm.cache_tsc_offset -
> +                            base->arch.hvm.msr_tsc_adjust +
> +                            v->arch.hvm.msr_tsc_adjust;
> +                        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> +                    }
> +            }
> +
>              return 0;
>          }
>
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index f37fddc61e47..79c7123c0a5b 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -3087,9 +3087,8 @@ int tsc_set_info(struct domain *d,
>               * set_tsc_offset() is called from hvm_vcpu_initialise() before
>               * tsc_set_info(). New vtsc mode may require recomputing TSC
>               * offset.
> -             * We only need to do this for BSP during initial boot. APs will
> -             * call set_tsc_offset() later from hvm_vcpu_reset_state() and they
> -             * will sync their TSC to BSP's sync_tsc.
> +             * We only need to do this for BSP during initial boot.  APs copy
> +             * vCPU 0's offset when created in hvm_vcpu_initialise().
>               */
>              d->arch.hvm.sync_tsc = rdtsc();
>              hvm_set_tsc_offset(d->vcpu[0],
> -- 
> 2.55.0
>
>
>

Re: [PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only

[Roger Pau Monné]

On Mon, Nov 02, 2026 at 03:40:42PM +0800, chunjie.zhu@citrix.com wrote:
> On Fri, Oct 30, 2026 at 02:24:19 PM, Roger Pau Monne <roger@xenproject.org> wrote:
> 
> > The current logic syncs the APs' TSC with the BSP both at reset and when
> > setting the vCPU state.  This doesn't match hardware behavior, where APs
> > are only in sync at platform start.
> 
> > Move the TSC sync so it's done in hvm_vcpu_initialise(), that's called a
> > single time at vCPU creation.  This matches the hardware behavior.
> > INIT-SIPI and VCPUOP_initialise no longer re-sync a vCPU's TSC to vCPU 0's.
> >
> > On migration the TSC offset for offline vCPUs is not transferred
> > (hvm_save_cpu_ctxt() skips offline vCPUs).  To avoid an unexpected skew
> > synchronize offline vCPUs with the first online vCPU, taking into account
> > the TSC_ADJUST value.
> >
> > The clearing of TSC_ADJUST is also made consistent with hardware behavior:
> > clear it on S3 resume, but not across an INIT-SIPI or VCPUOP_initialise.
> > Additionally, on S3 resume start all vCPUs from a shared TSC reading, so
> > they are in sync.
> >
> > Fixes: c31f8336e64e ("[HVM] Initialise AP TSC to same as BP TSC.")
> 
> This looks wrong.  c31f8336e64e only added the sync to hvm_bringup_ap(),
> which no longer exists.  The code removed here comes from bcba667e12e3
> (INIT-SIPI) and 192df6f9122d (VCPUOP_initialise).

TBH it's a long tail of code movement.  The code in hvm_bringup_ap()
was latter moved into hvm_vcpu_initialise(), hence why that commit is
used as the Fixes tag.  Note that previous to c31f8336e64e the TSC
sync was not applied, as the chunk was protected with #if 0.


> > Fixes: 6e0336351ed9 ("x86: Implement TSC adjust feature for HVM guest")
> > Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> > ---
> >  xen/arch/x86/hvm/domain.c |  5 -----
> >  xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
> >  xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
> >  xen/arch/x86/time.c       |  5 ++---
> >  4 files changed, 45 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> > index 978d8b222256..71f49ec7ce9f 100644
> > --- a/xen/arch/x86/hvm/domain.c
> > +++ b/xen/arch/x86/hvm/domain.c
> > @@ -312,11 +312,6 @@ int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
> >      hvm_set_segment_register(v, x86_seg_es, &es);
> >      hvm_set_segment_register(v, x86_seg_tss, &tr);
> >
> > -    /* Sync AP's TSC with BSP's. */
> > -    v->arch.hvm.cache_tsc_offset =
> > -        d->vcpu[0]->arch.hvm.cache_tsc_offset;
> > -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > -
> >      paging_update_paging_modes(v);
> >
> >      hvmemul_cancel(v);
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ef9c7cd05777..81e90cc8fc8e 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1667,6 +1667,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
> >          /* Init guest TSC to start from zero. */
> >          hvm_set_guest_tsc(v, 0);
> >      }
> > +    else
> > +    {
> > +        /* Start in step with vCPU 0, as all CPUs do at power-on. */
> > +        v->arch.hvm.cache_tsc_offset = d->vcpu[0]->arch.hvm.cache_tsc_offset;
> > +        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > +    }
> >
> >      return 0;
> >
> > @@ -4021,13 +4027,6 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
> >      hvm_set_segment_register(v, x86_seg_gdt, &reg);
> >      hvm_set_segment_register(v, x86_seg_idt, &reg);
> >
> > -    /* Sync AP's TSC with BSP's. */
> > -    v->arch.hvm.cache_tsc_offset =
> > -        v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
> > -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > -
> > -    v->arch.hvm.msr_tsc_adjust = 0;
> > -
> >      paging_update_paging_modes(v);
> >
> >      v->arch.flags |= TF_kernel_mode;
> > @@ -4079,9 +4078,13 @@ static void hvm_s3_resume(struct domain *d)
> >      if ( test_and_clear_bool(d->arch.hvm.is_s3_suspended) )
> >      {
> >          struct vcpu *v;
> > +        uint64_t at_tsc = rdtsc();
> >
> >          for_each_vcpu( d, v )
> > -            hvm_set_guest_tsc(v, 0);
> > +        {
> > +            v->arch.hvm.msr_tsc_adjust = 0;
> > +            hvm_set_guest_tsc_fixed(v, 0, at_tsc);
> > +        }
> >          domain_unpause(d);
> >      }
> >  }
> > diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
> > index 8ab64057064e..65ebfd384d69 100644
> > --- a/xen/arch/x86/hvm/save.c
> > +++ b/xen/arch/x86/hvm/save.c
> > @@ -289,6 +289,7 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
> >  {
> >      const struct hvm_save_header *hdr;
> >      struct hvm_save_descriptor *desc;
> > +    struct vcpu *v;
> >      int rc;
> >
> >      if ( d->is_dying )
> > @@ -302,8 +303,6 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
> >      rc = arch_hvm_check(d, hdr);
> >      if ( real )
> >      {
> > -        struct vcpu *v;
> > -
> >          ASSERT(!rc);
> >
> >          /*
> > @@ -341,6 +340,37 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
> >              /* Reset cursor for hvm_load(, true, ). */
> >              if ( !real )
> >                  h->cur = 0;
> > +            else
> > +            {
> > +                const struct vcpu *base = NULL;
> > +
> > +                /*
> > +                 * Offline vCPUs have no CPU record.  Adjust the TSC offset
> > +                 * based on the first online vCPU, taking into account
> > +                 * TSC_ADJUST which is saved regardless of whether the vCPU is
> > +                 * offline.
> > +                 */
> > +                for_each_vcpu(d, v)
> > +                    if ( !test_bit(_VPF_down, &v->pause_flags) )
> > +                    {
> > +                        base = v;
> > +                        break;
> > +                    }
> > +
> > +                if ( !base )
> > +                    return 0;
> 
> This silently skips the offline vCPUs.  If that ever causes a problem it
> will be hard to diagnose, so please add a comment or a log message.

Is it a realistic situation to have a stream with all vCPUs offline?
I was about to add an ASSERT_UNREACHABLE() here, but that's not a wise
move since the restore stream is external, and could be crafted in
such a way.  Also, if there's no online vCPUs, does it matter to sync
the vCPUs' TSC?

Thanks, Roger.

Re: [PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only

[chunjie.zhu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 7401 bytes --]

On Mon, Nov 02, 2026 at 09:34:56 AM, Roger Pau =?utf-8?B?TW9ubsOp?= <roger@xenproject.org> wrote:

> On Mon, Nov 02, 2026 at 03:40:42PM +0800, chunjie.zhu@citrix.com wrote:
> > On Fri, Oct 30, 2026 at 02:24:19 PM, Roger Pau Monne <roger@xenproject.org> wrote:
> > 
> > > The current logic syncs the APs' TSC with the BSP both at reset and when
> > > setting the vCPU state.  This doesn't match hardware behavior, where APs
> > > are only in sync at platform start.
> > 
> > > Move the TSC sync so it's done in hvm_vcpu_initialise(), that's called a
> > > single time at vCPU creation.  This matches the hardware behavior.
> > > INIT-SIPI and VCPUOP_initialise no longer re-sync a vCPU's TSC to vCPU 0's.
> > >
> > > On migration the TSC offset for offline vCPUs is not transferred
> > > (hvm_save_cpu_ctxt() skips offline vCPUs).  To avoid an unexpected skew
> > > synchronize offline vCPUs with the first online vCPU, taking into account
> > > the TSC_ADJUST value.
> > >
> > > The clearing of TSC_ADJUST is also made consistent with hardware behavior:
> > > clear it on S3 resume, but not across an INIT-SIPI or VCPUOP_initialise.
> > > Additionally, on S3 resume start all vCPUs from a shared TSC reading, so
> > > they are in sync.
> > >
> > > Fixes: c31f8336e64e ("[HVM] Initialise AP TSC to same as BP TSC.")
> > 
> > This looks wrong.  c31f8336e64e only added the sync to hvm_bringup_ap(),
> > which no longer exists.  The code removed here comes from bcba667e12e3
> > (INIT-SIPI) and 192df6f9122d (VCPUOP_initialise).
>
> TBH it's a long tail of code movement.  The code in hvm_bringup_ap()
> was latter moved into hvm_vcpu_initialise(), hence why that commit is
> used as the Fixes tag.  Note that previous to c31f8336e64e the TSC
> sync was not applied, as the chunk was protected with #if 0.
>

Thanks for the explanation, that makes sense.

>
> > > Fixes: 6e0336351ed9 ("x86: Implement TSC adjust feature for HVM guest")
> > > Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> > > ---
> > >  xen/arch/x86/hvm/domain.c |  5 -----
> > >  xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
> > >  xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
> > >  xen/arch/x86/time.c       |  5 ++---
> > >  4 files changed, 45 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> > > index 978d8b222256..71f49ec7ce9f 100644
> > > --- a/xen/arch/x86/hvm/domain.c
> > > +++ b/xen/arch/x86/hvm/domain.c
> > > @@ -312,11 +312,6 @@ int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
> > >      hvm_set_segment_register(v, x86_seg_es, &es);
> > >      hvm_set_segment_register(v, x86_seg_tss, &tr);
> > >
> > > -    /* Sync AP's TSC with BSP's. */
> > > -    v->arch.hvm.cache_tsc_offset =
> > > -        d->vcpu[0]->arch.hvm.cache_tsc_offset;
> > > -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > > -
> > >      paging_update_paging_modes(v);
> > >
> > >      hvmemul_cancel(v);
> > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > > index ef9c7cd05777..81e90cc8fc8e 100644
> > > --- a/xen/arch/x86/hvm/hvm.c
> > > +++ b/xen/arch/x86/hvm/hvm.c
> > > @@ -1667,6 +1667,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
> > >          /* Init guest TSC to start from zero. */
> > >          hvm_set_guest_tsc(v, 0);
> > >      }
> > > +    else
> > > +    {
> > > +        /* Start in step with vCPU 0, as all CPUs do at power-on. */
> > > +        v->arch.hvm.cache_tsc_offset = d->vcpu[0]->arch.hvm.cache_tsc_offset;
> > > +        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > > +    }
> > >
> > >      return 0;
> > >
> > > @@ -4021,13 +4027,6 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
> > >      hvm_set_segment_register(v, x86_seg_gdt, &reg);
> > >      hvm_set_segment_register(v, x86_seg_idt, &reg);
> > >
> > > -    /* Sync AP's TSC with BSP's. */
> > > -    v->arch.hvm.cache_tsc_offset =
> > > -        v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
> > > -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > > -
> > > -    v->arch.hvm.msr_tsc_adjust = 0;
> > > -
> > >      paging_update_paging_modes(v);
> > >
> > >      v->arch.flags |= TF_kernel_mode;
> > > @@ -4079,9 +4078,13 @@ static void hvm_s3_resume(struct domain *d)
> > >      if ( test_and_clear_bool(d->arch.hvm.is_s3_suspended) )
> > >      {
> > >          struct vcpu *v;
> > > +        uint64_t at_tsc = rdtsc();
> > >
> > >          for_each_vcpu( d, v )
> > > -            hvm_set_guest_tsc(v, 0);
> > > +        {
> > > +            v->arch.hvm.msr_tsc_adjust = 0;
> > > +            hvm_set_guest_tsc_fixed(v, 0, at_tsc);
> > > +        }
> > >          domain_unpause(d);
> > >      }
> > >  }
> > > diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
> > > index 8ab64057064e..65ebfd384d69 100644
> > > --- a/xen/arch/x86/hvm/save.c
> > > +++ b/xen/arch/x86/hvm/save.c
> > > @@ -289,6 +289,7 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
> > >  {
> > >      const struct hvm_save_header *hdr;
> > >      struct hvm_save_descriptor *desc;
> > > +    struct vcpu *v;
> > >      int rc;
> > >
> > >      if ( d->is_dying )
> > > @@ -302,8 +303,6 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
> > >      rc = arch_hvm_check(d, hdr);
> > >      if ( real )
> > >      {
> > > -        struct vcpu *v;
> > > -
> > >          ASSERT(!rc);
> > >
> > >          /*
> > > @@ -341,6 +340,37 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
> > >              /* Reset cursor for hvm_load(, true, ). */
> > >              if ( !real )
> > >                  h->cur = 0;
> > > +            else
> > > +            {
> > > +                const struct vcpu *base = NULL;
> > > +
> > > +                /*
> > > +                 * Offline vCPUs have no CPU record.  Adjust the TSC offset
> > > +                 * based on the first online vCPU, taking into account
> > > +                 * TSC_ADJUST which is saved regardless of whether the vCPU is
> > > +                 * offline.
> > > +                 */
> > > +                for_each_vcpu(d, v)
> > > +                    if ( !test_bit(_VPF_down, &v->pause_flags) )
> > > +                    {
> > > +                        base = v;
> > > +                        break;
> > > +                    }
> > > +
> > > +                if ( !base )
> > > +                    return 0;
> > 
> > This silently skips the offline vCPUs.  If that ever causes a problem it
> > will be hard to diagnose, so please add a comment or a log message.
>
> Is it a realistic situation to have a stream with all vCPUs offline?
> I was about to add an ASSERT_UNREACHABLE() here, but that's not a wise
> move since the restore stream is external, and could be crafted in
> such a way.  Also, if there's no online vCPUs, does it matter to sync
> the vCPUs' TSC?
>
> Thanks, Roger.
>

Yes, agree. The restore stream might be used by external to exploit whole host.

The code block sits in the END-marker branch, all load handlers have already run, so
the restore can continue as usual. With no online vCPU there is no reference TSC to
sync against, and nothing is running which can observe the skew.

The code is good. I personally prefer adding comments to explain it, but I’m fine
either way on whether we add comments.

Re: [PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only

[Teddy Astie]

[-- Attachment #1.1: Type: text/plain, Size: 6999 bytes --]

Le 30/10/2026 à 14:26, Roger Pau Monne a écrit :
> The current logic syncs the APs' TSC with the BSP both at reset and when
> setting the vCPU state.  This doesn't match hardware behavior, where APs
> are only in sync at platform start.
> 
> Move the TSC sync so it's done in hvm_vcpu_initialise(), that's called a
> single time at vCPU creation.  This matches the hardware behavior.
> INIT-SIPI and VCPUOP_initialise no longer re-sync a vCPU's TSC to vCPU 0's.
> 
> On migration the TSC offset for offline vCPUs is not transferred
> (hvm_save_cpu_ctxt() skips offline vCPUs).  To avoid an unexpected skew
> synchronize offline vCPUs with the first online vCPU, taking into account
> the TSC_ADJUST value.
> 
> The clearing of TSC_ADJUST is also made consistent with hardware behavior:
> clear it on S3 resume, but not across an INIT-SIPI or VCPUOP_initialise.
> Additionally, on S3 resume start all vCPUs from a shared TSC reading, so
> they are in sync.
> 
> Fixes: c31f8336e64e ("[HVM] Initialise AP TSC to same as BP TSC.")
> Fixes: 6e0336351ed9 ("x86: Implement TSC adjust feature for HVM guest")
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> ---
>   xen/arch/x86/hvm/domain.c |  5 -----
>   xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
>   xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
>   xen/arch/x86/time.c       |  5 ++---
>   4 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 978d8b222256..71f49ec7ce9f 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -312,11 +312,6 @@ int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
>       hvm_set_segment_register(v, x86_seg_es, &es);
>       hvm_set_segment_register(v, x86_seg_tss, &tr);
>   
> -    /* Sync AP's TSC with BSP's. */
> -    v->arch.hvm.cache_tsc_offset =
> -        d->vcpu[0]->arch.hvm.cache_tsc_offset;
> -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> -
>       paging_update_paging_modes(v);
>   
>       hvmemul_cancel(v);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef9c7cd05777..81e90cc8fc8e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1667,6 +1667,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
>           /* Init guest TSC to start from zero. */
>           hvm_set_guest_tsc(v, 0);
>       }
> +    else
> +    {
> +        /* Start in step with vCPU 0, as all CPUs do at power-on. */
> +        v->arch.hvm.cache_tsc_offset = d->vcpu[0]->arch.hvm.cache_tsc_offset;
> +        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> +    }
>   
>       return 0;
>   
> @@ -4021,13 +4027,6 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
>       hvm_set_segment_register(v, x86_seg_gdt, &reg);
>       hvm_set_segment_register(v, x86_seg_idt, &reg);
>   
> -    /* Sync AP's TSC with BSP's. */
> -    v->arch.hvm.cache_tsc_offset =
> -        v->domain->vcpu[0]->arch.hvm.cache_tsc_offset;
> -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> -
> -    v->arch.hvm.msr_tsc_adjust = 0;
> -
>       paging_update_paging_modes(v);
>   
>       v->arch.flags |= TF_kernel_mode;
> @@ -4079,9 +4078,13 @@ static void hvm_s3_resume(struct domain *d)
>       if ( test_and_clear_bool(d->arch.hvm.is_s3_suspended) )
>       {
>           struct vcpu *v;
> +        uint64_t at_tsc = rdtsc();
>   
>           for_each_vcpu( d, v )
> -            hvm_set_guest_tsc(v, 0);
> +        {
> +            v->arch.hvm.msr_tsc_adjust = 0;
> +            hvm_set_guest_tsc_fixed(v, 0, at_tsc);
> +        }
>           domain_unpause(d);
>       }
>   }
> diff --git a/xen/arch/x86/hvm/save.c b/xen/arch/x86/hvm/save.c
> index 8ab64057064e..65ebfd384d69 100644
> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -289,6 +289,7 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
>   {
>       const struct hvm_save_header *hdr;
>       struct hvm_save_descriptor *desc;
> +    struct vcpu *v;
>       int rc;
>   
>       if ( d->is_dying )
> @@ -302,8 +303,6 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
>       rc = arch_hvm_check(d, hdr);
>       if ( real )
>       {
> -        struct vcpu *v;
> -
>           ASSERT(!rc);
>   
>           /*
> @@ -341,6 +340,37 @@ int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)
>               /* Reset cursor for hvm_load(, true, ). */
>               if ( !real )
>                   h->cur = 0;
> +            else
> +            {
> +                const struct vcpu *base = NULL;
> +
> +                /*
> +                 * Offline vCPUs have no CPU record.  Adjust the TSC offset
> +                 * based on the first online vCPU, taking into account
> +                 * TSC_ADJUST which is saved regardless of whether the vCPU is
> +                 * offline.
> +                 */
> +                for_each_vcpu(d, v)
> +                    if ( !test_bit(_VPF_down, &v->pause_flags) )> +                    {
> +                        base = v;
> +                        break;
> +                    }
> +
> +                if ( !base )
> +                    return 0;
> +
> +                for_each_vcpu(d, v)
> +                    if ( test_bit(_VPF_down, &v->pause_flags) )
> +                    {
> +                        v->arch.hvm.cache_tsc_offset =
> +                            base->arch.hvm.cache_tsc_offset -
> +                            base->arch.hvm.msr_tsc_adjust +
> +                            v->arch.hvm.msr_tsc_adjust;
> +                        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> +                    }> +            }
> +
>               return 0;
>           }

nit: I would prefer having test_bit(_VPF_down, &v->pause_flags) replaced 
with is_vcpu_online() equivalents for clarity.

>   
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index f37fddc61e47..79c7123c0a5b 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -3087,9 +3087,8 @@ int tsc_set_info(struct domain *d,
>                * set_tsc_offset() is called from hvm_vcpu_initialise() before
>                * tsc_set_info(). New vtsc mode may require recomputing TSC
>                * offset.
> -             * We only need to do this for BSP during initial boot. APs will
> -             * call set_tsc_offset() later from hvm_vcpu_reset_state() and they
> -             * will sync their TSC to BSP's sync_tsc.
> +             * We only need to do this for BSP during initial boot.  APs copy
> +             * vCPU 0's offset when created in hvm_vcpu_initialise().
>                */
>               d->arch.hvm.sync_tsc = rdtsc();
>               hvm_set_tsc_offset(d->vcpu[0],

Teddy


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: [PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only

[Andrew Cooper]

On 30/10/2026 1:24 pm, Roger Pau Monne wrote:
> The current logic syncs the APs' TSC with the BSP both at reset and when
> setting the vCPU state.  This doesn't match hardware behavior, where APs
> are only in sync at platform start.
>
> Move the TSC sync so it's done in hvm_vcpu_initialise(), that's called a
> single time at vCPU creation.  This matches the hardware behavior.
> INIT-SIPI and VCPUOP_initialise no longer re-sync a vCPU's TSC to vCPU 0's.

Yeah, this is very buggy.   All TSC values are 0'd at #RESET and
unchanged for INIT.

>
> On migration the TSC offset for offline vCPUs is not transferred
> (hvm_save_cpu_ctxt() skips offline vCPUs).  To avoid an unexpected skew
> synchronize offline vCPUs with the first online vCPU, taking into account
> the TSC_ADJUST value.
>
> The clearing of TSC_ADJUST is also made consistent with hardware behavior:
> clear it on S3 resume, but not across an INIT-SIPI or VCPUOP_initialise.
> Additionally, on S3 resume start all vCPUs from a shared TSC reading, so
> they are in sync.
>
> Fixes: c31f8336e64e ("[HVM] Initialise AP TSC to same as BP TSC.")
> Fixes: 6e0336351ed9 ("x86: Implement TSC adjust feature for HVM guest")
> Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> ---
>  xen/arch/x86/hvm/domain.c |  5 -----
>  xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
>  xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
>  xen/arch/x86/time.c       |  5 ++---
>  4 files changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 978d8b222256..71f49ec7ce9f 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -312,11 +312,6 @@ int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
>      hvm_set_segment_register(v, x86_seg_es, &es);
>      hvm_set_segment_register(v, x86_seg_tss, &tr);
>  
> -    /* Sync AP's TSC with BSP's. */
> -    v->arch.hvm.cache_tsc_offset =
> -        d->vcpu[0]->arch.hvm.cache_tsc_offset;
> -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> -
>      paging_update_paging_modes(v);
>  
>      hvmemul_cancel(v);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef9c7cd05777..81e90cc8fc8e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1667,6 +1667,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
>          /* Init guest TSC to start from zero. */
>          hvm_set_guest_tsc(v, 0);
>      }
> +    else
> +    {
> +        /* Start in step with vCPU 0, as all CPUs do at power-on. */
> +        v->arch.hvm.cache_tsc_offset = d->vcpu[0]->arch.hvm.cache_tsc_offset;
> +        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> +    }

I agree that the current behaviour is very broken, but this adjustment
isn't correct either.  For starters, it's not possible to have a
non-zero value here[1].

I'm quite concerned with this patch.  You're adjusting at least 3
separate things.

~Andrew

[1] Well, it is, but only by the toolstack racing hypercalls, which we
do not care about supporting.

Re: [PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only

[Roger Pau Monné]

On Mon, Nov 02, 2026 at 01:44:06PM +0000, Andrew Cooper wrote:
> On 30/10/2026 1:24 pm, Roger Pau Monne wrote:
> > The current logic syncs the APs' TSC with the BSP both at reset and when
> > setting the vCPU state.  This doesn't match hardware behavior, where APs
> > are only in sync at platform start.
> >
> > Move the TSC sync so it's done in hvm_vcpu_initialise(), that's called a
> > single time at vCPU creation.  This matches the hardware behavior.
> > INIT-SIPI and VCPUOP_initialise no longer re-sync a vCPU's TSC to vCPU 0's.
> 
> Yeah, this is very buggy.   All TSC values are 0'd at #RESET and
> unchanged for INIT.
> 
> >
> > On migration the TSC offset for offline vCPUs is not transferred
> > (hvm_save_cpu_ctxt() skips offline vCPUs).  To avoid an unexpected skew
> > synchronize offline vCPUs with the first online vCPU, taking into account
> > the TSC_ADJUST value.
> >
> > The clearing of TSC_ADJUST is also made consistent with hardware behavior:
> > clear it on S3 resume, but not across an INIT-SIPI or VCPUOP_initialise.
> > Additionally, on S3 resume start all vCPUs from a shared TSC reading, so
> > they are in sync.
> >
> > Fixes: c31f8336e64e ("[HVM] Initialise AP TSC to same as BP TSC.")
> > Fixes: 6e0336351ed9 ("x86: Implement TSC adjust feature for HVM guest")
> > Signed-off-by: Roger Pau Monné <roger@xenproject.org>
> > ---
> >  xen/arch/x86/hvm/domain.c |  5 -----
> >  xen/arch/x86/hvm/hvm.c    | 19 +++++++++++--------
> >  xen/arch/x86/hvm/save.c   | 34 ++++++++++++++++++++++++++++++++--
> >  xen/arch/x86/time.c       |  5 ++---
> >  4 files changed, 45 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> > index 978d8b222256..71f49ec7ce9f 100644
> > --- a/xen/arch/x86/hvm/domain.c
> > +++ b/xen/arch/x86/hvm/domain.c
> > @@ -312,11 +312,6 @@ int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx)
> >      hvm_set_segment_register(v, x86_seg_es, &es);
> >      hvm_set_segment_register(v, x86_seg_tss, &tr);
> >  
> > -    /* Sync AP's TSC with BSP's. */
> > -    v->arch.hvm.cache_tsc_offset =
> > -        d->vcpu[0]->arch.hvm.cache_tsc_offset;
> > -    hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > -
> >      paging_update_paging_modes(v);
> >  
> >      hvmemul_cancel(v);
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ef9c7cd05777..81e90cc8fc8e 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1667,6 +1667,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
> >          /* Init guest TSC to start from zero. */
> >          hvm_set_guest_tsc(v, 0);
> >      }
> > +    else
> > +    {
> > +        /* Start in step with vCPU 0, as all CPUs do at power-on. */
> > +        v->arch.hvm.cache_tsc_offset = d->vcpu[0]->arch.hvm.cache_tsc_offset;
> > +        hvm_set_tsc_offset(v, v->arch.hvm.cache_tsc_offset);
> > +    }
> 
> I agree that the current behaviour is very broken, but this adjustment
> isn't correct either.  For starters, it's not possible to have a
> non-zero value here[1].

I'm possibly confused, but d->vcpu[0]->arch.hvm.cache_tsc_offset will
very likely be different than 0 here (as long as vCPU are created in
order), as it's adjusted by the hvm_set_guest_tsc() call done above
for vCPU 0?

> I'm quite concerned with this patch.  You're adjusting at least 3
> separate things.

Hm, it has grown beyond the initial scope I had for it.  I could
certainly split parts of it, I've presented it as a single change to
avoid introducing intermediate breakage.

Thanks, Roger.