|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86/hvm: sync APs TSC with the BSP at creation only
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.
> > 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, ®);
> > hvm_set_segment_register(v, x86_seg_idt, ®);
> >
> > - /* 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.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |