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

[chunjie.zhu@xxxxxxxxxx]

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

> On Mon, Nov 02, 2026 at 03:40:42PM +0800, chunjie.zhu@xxxxxxxxxx wrote:
> > On Fri, Oct 30, 2026 at 02:24:19 PM, Roger Pau Monne <roger@xxxxxxxxxxxxxx> 
> > 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@xxxxxxxxxxxxxx>
> > > ---
> > >  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.