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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

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@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);
> > +    }
> 
> 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.