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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/02/18 17:03, Wei Liu wrote:
> On Tue, Feb 20, 2018 at 11:58:43AM +0000, Andrew Cooper wrote:
>> There are many problems with MSR_TSC_AUX handling.
>>
>> To being with, the RDPID instruction reads MSR_TSC_AUX, but it is only the
>      ^
>      begin
>
>> 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 an accepting them.  This also causes the emulator to reject RDTSCP
>              ^
>              and
>
>> for guests without the features.
>>
>>  
> [...]
>> @@ -1284,7 +1285,18 @@ long arch_do_domctl(
>>                  for ( j = 0; j < ARRAY_SIZE(msrs_to_send); ++j )
>>                  {
>>                      uint64_t val;
>> -                    int rc = guest_rdmsr(v, msrs_to_send[j], &val);
>> +                    int rc;
>> +
>> +                    /*
>> +                     * Skip MSR_TSC_AUX if using TSC_MODE_PVRDTSCP.  In this
>> +                     * case, the MSR is read-only, and should be rejected if
>> +                     * seen on the restore side.
>> +                     */
>> +                    if ( msrs_to_send[j] == MSR_TSC_AUX &&
>> +                         d->arch.tsc_mode == TSC_MODE_PVRDTSCP )
>> +                        continue;
> Shouldn't we increment incarnation and send it over to the remote end?
> Or send the original value and let the remote increments it?

incarnation, and its increments, is handled in tsc_set_info(), which is
keyed off the TSC_INFO record in the migration stream.  That side of
things "already works" (FSVO "works").

The check here is to cause us to skip everything to do with migrating
MSR_TSC_AUX if we think the TSC code is responsible for the value.

> Frankly I'm not sure how guest is supposed to use that value, but
> letting the receiving end restart at 0, which can end up using the same
> incarnation as before seems conceptually wrong to me. The more so you
> mention a lot of migration's in your previous patches.

I don't understand what thought and planning lead to TSC_MODE_PVRDTSCP.

From an implementation side of things, it was also responsible for
introducing a binary incompatibility into the migration stream between
3.3 and 3.4 which caused sporadic loss of interrupts on migrate, and was
sufficiently complicated to track down that it only got fixed in 4.2 
(c/s 620ce29a2d45, but not that that change is an accurate reflection of
how much stress and effort went into tracking the issue down).

> This is not disagreeing with the point that IA32_TSC_AUX should be
> read-only for PV guest.

Any guest running with TSC_MODE_PVRDTSCP.  It supposedly works for HVM
guests as well.

> And +1 for the unification of HVM and PV code for handling this.

An observant person might notice that all MSR handing is starting to
become common, and this might be following suite from CPUID a few
release ago.  This totally isn't on purpose...

~Andrew

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