Re: [Xen-devel] [PATCH 5/5] x86: Rework MSR_TSC_AUX handling from scratch.

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 20, 2018 at 11:58:43AM +0000, Andrew Cooper wrote:
> There are many problems with MSR_TSC_AUX handling.
> 
> To being with, the RDPID instruction reads MSR_TSC_AUX, but it is only the
> RDTSCP feature which enumerates the MSR.  Therefore, RDPID functionally
> depends on RDTSCP.
> 
> For PV guests, we hide RDTSCP but advertise RDPID.  We also silently drop
> writes to MSR_TSC_AUX, which is very antisocial.  Therefore, enable RDTSCP for
> PV guests, which in turn allows RDPID to work.
> 
> To support RDTSCP properly for PV guests, the MSR_TSC_AUX handling is moved
> into the generic MSR policy infrastructure, and becomes common.  One
> improvement is that we will now reject invalid values, rather than silently
> truncating an accepting them.  This also causes the emulator to reject RDTSCP
> for guests without the features.
> 
> One complication is TSC_MODE_PVRDTSCP, in which Xen takes control of
> MSR_TSC_AUX and the reported value is actually the migration incarnation.  The
> previous behaviour of this mode was to silently drop writes, but as it is a
> break in the x86 ABI to start with, switch the semantics to be more sane, and
> have TSC_MODE_PVRDTSCP make the MSR properly read-only.
> 
> With PV guests getting to use MSR_TSC_AUX properly now, the MSR needs
> migrating.  Cope with it the common migration logic.  Care must be taken
> however to avoid sending the MSR if TSC_MODE_PVRDTSCP is active, as the
> receiving side will reject the guest_wrmsr().
> 
> What remains is that tsc_set_info() need to broadcast d->arch.incarnation to
> all vCPUs MSR block if in TSC_MODE_PVRDTSCP, so the context switching and
> emulation code functions correctly.

I have one likely stupid question about the PVRDTSCP, how does the
guest know it's actually using it? I'm not able to find any cpuid or
xenfeat to signal the guest it's running in PVRDTSCP mode, and thus
that MSR_TSC_AUX contains this magic incarnation value?

Which TBH seems quite pointless, the guest should be perfectly capable
of maintaining it's own count of migrations.

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0539551..ab24f87 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -792,7 +792,7 @@ static int hvm_save_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  
>          ctxt.tsc = hvm_get_guest_tsc_fixed(v, d->arch.hvm_domain.sync_tsc);
>  
> -        ctxt.msr_tsc_aux = hvm_msr_tsc_aux(v);
> +        /* ctxt.msr_tsc_aux omitted - now sent via generic MSR record. */
>  
>          hvm_get_segment_register(v, x86_seg_idtr, &seg);
>          ctxt.idtr_limit = seg.limit;
> @@ -1046,7 +1046,24 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>      if ( hvm_funcs.tsc_scaling.setup )
>          hvm_funcs.tsc_scaling.setup(v);
>  
> -    v->arch.hvm_vcpu.msr_tsc_aux = ctxt.msr_tsc_aux;
> +    /*
> +     * Backwards compatibility.  MSR_TSC_AUX contains different information
> +     * depending on whether TSC_MODE_PVRDTSCP is enabled.
> +     *
> +     * Before Xen 4.11, ctxt.msr_tsc_aux was sent unconditionally and 
> restored
> +     * here, but the value was otherwise ignored in TSC_MODE_PVRDTSCP.
> +     *
> +     * In 4.11, the logic was changed to send MSR_TSC_AUX via the generic MSR
> +     * mechanism if tsc_mode != TSC_MODE_PVRDTSCP.  If tsc_mode ==
> +     * TSC_MODE_PVRDTSCP, the tsc logic is responsibile for setting the
> +     * correct value.
> +     *
> +     * For compatibility with migration streams from before 4.11, we restore
> +     * from ctxt.msr_tsc_aux if the TSC code hasn't/isn't in charge, and 
> we've
> +     * not seen a value arrive in the generic MSR record.
> +     */
> +    if ( d->arch.tsc_mode != TSC_MODE_PVRDTSCP && !v->arch.msr->tsc_aux )
> +        v->arch.msr->tsc_aux = ctxt.msr_tsc_aux;

I'm having a hard time figuring out whether the MSRs have been loaded
at this point, I assume there's some guarantee that MSR will be loaded
before loading the CPU context?

The rest LGTM.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel