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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 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.