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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.18 at 16:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 23/02/18 15:05, Jan Beulich wrote:
>>>>> On 21.02.18 at 12:36, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -175,6 +175,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
>>> uint64_t *val)
>>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>          break;
>>>  
>>> +    case MSR_TSC_AUX:
>>> +        if ( !cp->extd.rdtscp )
>>> +            goto gp_fault;
>> Isn't this breaking the PV use without the feature flag set, but
>> running in TSC_MODE_PVRDTSCP? I.e. don't you want
>>
>>         if ( !cp->extd.rdtscp &&
>>              d->arch.tsc_mode != TSC_MODE_PVRDTSCP )
>>
>> ? Remember there are two cases, one being that the host has the
>> feature flag set, but the guest has it clear, and the other being
>> that both have it clear. Since in the former case the guest can read
>> the MSR through RDTSCP, I think the MSR access ought to be
>> allowed too.
> 
> There is at least a 3rd case, of no hardware RDTSCP support, which is
> why we also emulate it in emul-invl-op.c

Well, that's the "both have it clear" case I've mentioned above.

> A guest trying to use PVRDTSC necessarily needs out-of-band signalling
> to set it up.  I do not think it is reasonable or appropriate to retain
> the ABI breakage of completing reads of the MSR when the instruction
> should be architecturally unavailable.

Well, one can take either position, so I'm not going to object to
you not wanting to make the change.

>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -2178,7 +2178,18 @@ void tsc_set_info(struct domain *d,
>>>          }
>>>          break;
>>>      }
>>> +
>>>      d->arch.incarnation = incarnation + 1;
>>> +
>>> +    if ( d->arch.tsc_mode == TSC_MODE_PVRDTSCP )
>>> +    {
>>> +        struct vcpu *v;
>>> +
>>> +        /* Distribute incarnation into each vcpu's MSR_TSC_AUX. */
>>> +        for_each_vcpu ( d, v )
>>> +            v->arch.msr->tsc_aux = d->arch.incarnation;
>>> +    }
>> This not needing a lock might warrant a comment (the domain is
>> [explicitly or implicitly] paused when coming here).
> 
> This new piece of logic isn't any different to the rest of
> tsc_set_info() WRT being paused.  It is explicitly paused at this point.

True.

Jan


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