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

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

On Wed, Feb 21, 2018 at 11:36:15AM +0000, Andrew Cooper wrote:
> There are many problems with MSR_TSC_AUX handling.
> 
> To begin 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 and 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, so it is moved into the common MSR logic for PV guests.  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().  The HVM side is tweaked as
> well to only send/receive hvm_hw_cpu.msr_tsc_aux when the TSC logic isn't in
> control of the value.
> 
> 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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just one comment nit below.

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0539551..e45f6df 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -792,7 +792,9 @@ 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);
> +        /* Only send MSR_TSC_AUX if it isn't being handled by the TSC logic. 
> */
> +        if ( d->arch.tsc_mode != TSC_MODE_PVRDTSCP )
> +            ctxt.msr_tsc_aux = v->arch.msr->tsc_aux;
>  
>          hvm_get_segment_register(v, x86_seg_idtr, &seg);
>          ctxt.idtr_limit = seg.limit;
> @@ -1046,7 +1048,9 @@ 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;
> +    /* Only accept MSR_TSC_AUX if it isn't being handled by the TSC logic. */
               ^ set?

We actually accept it. Ie: Xen doesn't don't error out when
msr_tsc_aux is set and d->arch.tsc_mode == TSC_MODE_PVRDTSCP, or else
we would break backwards compatibility.

Thanks, Roger.

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