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

["Roger Pau Monné" <roger@xenproject.org>]

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.