Re: [PATCH] x86/tsx: Cope with RTM_ALWAYS_ABORT vs RTM mismatch

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/2024 1:45 pm, Jan Beulich wrote:
> On 04.04.2024 12:41, Andrew Cooper wrote:
>> @@ -9,6 +10,7 @@
>>   *  -1 => Default, altered to 0/1 (if unspecified) by:
>>   *                 - TAA heuristics/settings for speculative safety
>>   *                 - "TSX vs PCR3" select for TSX memory ordering safety
>> + *  -2 => Implicit tsx=0 (from RTM_ALWAYS_ABORT vs RTM mismatch)
>>   *  -3 => Implicit tsx=1 (feed-through from spec-ctrl=0)
>>   *
>>   * This is arranged such that the bottom bit encodes whether TSX is actually
>> @@ -114,11 +116,50 @@ void tsx_init(void)
>>  
>>          if ( cpu_has_tsx_force_abort )
>>          {
>> +            uint64_t val;
>> +
>>              /*
>> -             * On an early TSX-enable Skylake part subject to the memory
>> +             * On an early TSX-enabled Skylake part subject to the memory
>>               * ordering erratum, with at least the March 2019 microcode.
>>               */
>>  
>> +            rdmsrl(MSR_TSX_FORCE_ABORT, val);
>> +
>> +            /*
>> +             * At the time of writing (April 2024), it was discovered that
>> +             * some parts (e.g. CoffeeLake 8th Gen, 06-9e-0a, ucode 0xf6)
>> +             * advertise RTM_ALWAYS_ABORT, but XBEGIN instructions #UD.  
>> Other
>> +             * similar parts (e.g. KabyLake Xeon-E3, 06-9e-09, ucode 0xf8)
>> +             * operate as expected.
>> +             *
>> +             * In this case:
>> +             *  - RTM_ALWAYS_ABORT and MSR_TSX_FORCE_ABORT are enumerated.
>> +             *  - XBEGIN instructions genuinely #UD.
>> +             *  - MSR_TSX_FORCE_ABORT is write-discard and fails to hold its
>> +             *    value.
>> +             *  - HLE and RTM are not enumerated, despite
>> +             *    MSR_TSX_FORCE_ABORT.TSX_CPUID_CLEAR being clear.
> Of these 4 items you use the first and last here. It took me some time to
> figure that the middle two are (aiui) only informational, and that you
> assume that first and last together are sufficient to uniquely identify
> the problematic parts. Separating the two groups a little might be helpful.

All 4 points are relevant to the if() expression.

>
> For the write-discard property, how was that determined? Does it affect all
> writable bits?

Marek kindly ran a debugging patch for me last night to try and figure
out what was going on.

Currently, Xen tries to set 0x2 (TSX_CPUID_CLEAR) and debugging showed
it being read back as 0.

I didn't check anything else, but I have a strong suspicion that I know
exactly what's going wrong here.

The property the if() condition is mainly looking for is !RTM &&
!(MSR_TFA.CPUID_CLEAR) because that's an illegal state in a

>
>> +             * Spot this case, and treat it as if no TSX is available at 
>> all.
>> +             * This will prevent Xen from thinking it's safe to offer 
>> HLE/RTM
>> +             * to VMs.
>> +             */
>> +            if ( val == 0 && cpu_has_rtm_always_abort && !cpu_has_rtm )
>> +            {
>> +                printk(XENLOG_ERR
>> +                       "FIRMWARE BUG: CPU %02x-%02x-%02x, ucode 0x%08x: 
>> RTM_ALWAYS_ABORT vs RTM mismatch\n",
> This isn't really firmware, is it? At least I wouldn't call microcode
> (assuming that's where the bad behavior is rooted) firmware.

Microcode is absolutely part of the system firmware.

>
>> +                       boot_cpu_data.x86, boot_cpu_data.x86_model,
>> +                       boot_cpu_data.x86_mask, this_cpu(cpu_sig).rev);
>> +
>> +                setup_clear_cpu_cap(X86_FEATURE_RTM_ALWAYS_ABORT);
> Instead of the "goto" below, wouldn't it be better to also force
> has_rtm_always_abort to false along with this, thus skipping the
> setup_force_cpu_cap(X86_FEATURE_RTM_ALWAYS_ABORT) further down?

I considered that and dismissed it.  It is more fragile, in a case were
really do want to treat this case as if TSX genuinely doesn't exist.

>  That would
> leave things a little less awkward flow-wise, imo. The one thing not
> becoming clear from the commentary above is whether cpu_has_tsx_ctrl might
> be true, and hence RTM/HLE still becoming (wrongly) set, if done that way.

MSR_TSX_CTRL and MSR_TSX_FORCE_ABORT exist on disjoint sets of CPUs. 
(The split being MDS_NO).

This is discussed explicitly lower down in the function, beyond the if (
once ) block.

~Andrew