Re: [Xen-devel] [PATCH 2/5] x86/pv: Avoid leaking other guests' MSR_TSC_AUX values into PV context

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.02.18 at 12:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1533,9 +1533,9 @@ void paravirt_ctxt_switch_to(struct vcpu *v)
>      if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
>          activate_debugregs(v);
>  
> -    if ( (v->domain->arch.tsc_mode ==  TSC_MODE_PVRDTSCP) &&
> -         boot_cpu_has(X86_FEATURE_RDTSCP) )
> -        write_rdtscp_aux(v->domain->arch.incarnation);
> +    if ( cpu_has_rdtscp )
> +        wrmsr_tsc_aux(v->domain->arch.tsc_mode == TSC_MODE_PVRDTSCP
> +                      ? v->domain->arch.incarnation : 0);

Wouldn't the conditional better check cpu_has_rdtscp || cpu_has_rdpid
(then also better matching the description)? And if so, then perhaps
also in other, pre-existing conditionals?

> @@ -210,6 +208,20 @@ static inline void write_efer(uint64_t val)
>  
>  DECLARE_PER_CPU(u32, ler_msr);
>  
> +DECLARE_PER_CPU(uint32_t, tsc_aux);
> +
> +/* Lazy update of MSR_TSC_AUX */
> +static inline void wrmsr_tsc_aux(uint32_t val)

Along the lines of rdtsc() (which also reads an MSR in reality)
wrtsc_aux()?

> +{
> +    uint32_t *this_tsc_aux = &this_cpu(tsc_aux);
> +
> +    if ( *this_tsc_aux != val )
> +    {
> +        wrmsr(MSR_TSC_AUX, val, 0);
> +        *this_tsc_aux = val;

I think it is generally more safe to write the cached value first, so
that some asynchronous writer would pick up the intended new
value. But obviously that's marginal here.

With at least the adjustments to the conditionals
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
If, otoh, you disagree, then we'll need to settle on something
first.

Jan


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