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
