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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2024 15:22, Andrew Cooper wrote:
> 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.

In which way? You don't probe XBEGIN to see whether you get back #UD. And
you also don't probe the MSR to see whether written bits are discarded.

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

Hmm, at the risk of upsetting you: Is a suspicion really enough for a
firm statement in a comment?

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

The ucode ahead of being loaded into CPUs is, sure. But once in the CPU
(and there may not be any loading at least in theory), it's not anymore.
It becomes part of the CPU then, albeit I still wouldn't call it "hardware".

Plus saying "firmware" suggests that firmware vendors could do anything
about the situation, when I don't think they can.

Jan