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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/02/18 14:04, Jan Beulich wrote:
>>>> 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?

cpu_has_rdpid already implies cpu_has_rdtscp in practice, making the ||
redundant.  No hardware exists with rdpid but without rdtscp.

Xen's CPUID logic gets updated to cope in patch 5, but the only place
this can currently go wrong is when running Xen virtualised under Xen
with PVRDTSC enabled, and I don't think we need to care about this case.

>
>> @@ -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()?

rdtsc() is named after the instruction.

There is no wrtsc instruction, and naming the function like that hides
its purpose.

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

Ok, although there is on chance of asyncrhonous writing 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.

I'm afraid I disagree.

~Andrew

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