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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.18 at 15:22, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

The question isn't whether such hardware actually exists, but
whether the architecture allows for such hardware to exist in
theory. The dependency you add in patch 5 implies you think so;
I continue to think that the more sane behavior would be if the
MSR was tied to either flag.

Jan


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