Re: [PATCH] x86/tsx: Cope with TSX deprecation on SKL/KBL/CFL/WHL

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/06/2021 07:36, Jan Beulich wrote:
> On 08.06.2021 19:05, Andrew Cooper wrote:
>> --- a/xen/arch/x86/tsx.c
>> +++ b/xen/arch/x86/tsx.c
>> @@ -60,6 +60,38 @@ void tsx_init(void)
>>               */
>>  
>>              /*
>> +             * Probe for the June 2021 microcode which de-features TSX on
>> +             * client parts.  (Note - this is a subset of parts impacted by
>> +             * the memory ordering errata.)
>> +             *
>> +             * RTM_ALWAYS_ABORT enumerates the new functionality, but is 
>> also
>> +             * read as zero if TSX_FORCE_ABORT.ENABLE_RTM has been set 
>> before
>> +             * we run.
>> +             *
>> +             * Undo this behaviour in Xen's view of the world.
>> +             */
>> +            bool has_rtm_always_abort = cpu_has_rtm_always_abort;
>> +
>> +            if ( !has_rtm_always_abort )
>> +            {
>> +                uint64_t val;
>> +
>> +                rdmsrl(MSR_TSX_FORCE_ABORT, val);
>> +
>> +                if ( val & TSX_ENABLE_RTM )
>> +                    has_rtm_always_abort = true;
>> +            }
>> +
>> +            /*
>> +             * Always force RTM_ALWAYS_ABORT to be visibile, even if it
>> +             * currently is.  If the user explicitly opts to enable TSX, 
>> we'll
>> +             * set TSX_FORCE_ABORT.ENABLE_RTM and hide RTM_ALWAYS_ABORT from
>> +             * the general CPUID scan later.
>> +             */
>> +            if ( has_rtm_always_abort )
>> +                setup_force_cpu_cap(X86_FEATURE_RTM_ALWAYS_ABORT);
> I understand the "we'll set" part, but I don't think "we'll hide"
> anything explicitly. Aiui it is ...
>
>> @@ -131,9 +170,36 @@ void tsx_init(void)
>>          /* Check bottom bit only.  Higher bits are various sentinels. */
>>          rtm_disabled = !(opt_tsx & 1);
>>  
>> -        lo &= ~TSX_FORCE_ABORT_RTM;
>> -        if ( rtm_disabled )
>> -            lo |= TSX_FORCE_ABORT_RTM;
>> +        lo &= ~(TSX_FORCE_ABORT_RTM | TSX_CPUID_CLEAR | TSX_ENABLE_RTM);
>> +
>> +        if ( cpu_has_rtm_always_abort )
>> +        {
>> +            /*
>> +             * June 2021 microcode, on a client part with TSX de-featured:
>> +             *  - There are no mitigations for the TSX memory ordering 
>> errata.
>> +             *  - Performance counter 3 works.  (I.e. it isn't being used by
>> +             *    microcode to work around the memory ordering errata.)
>> +             *  - TSX_FORCE_ABORT.FORCE_ABORT_RTM is fixed 
>> read1/write-discard.
>> +             *  - TSX_FORCE_ABORT.TSX_CPUID_CLEAR can be used to hide the
>> +             *    HLE/RTM CPUID bits.
>> +             *  - TSX_FORCE_ABORT.ENABLE_RTM may be used to opt in to
>> +             *    re-enabling RTM, at the users own risk.
>> +             */
>> +            lo |= rtm_disabled ? TSX_CPUID_CLEAR : TSX_ENABLE_RTM;
> ... the setting of TSX_ENABLE_RTM here which, as a result, causes
> RTM_ALWAYS_ABORT to be clear. If that's correct, perhaps the wording
> in that earlier comment would better be something like "we'll set
> TSX_FORCE_ABORT.ENABLE_RTM and hence cause RTM_ALWAYS_ABORT to be
> hidden from the general CPUID scan later"?

Yes - that is the intended meaning.  I'll adjust.

> If this understanding of mine is correct, then preferably with some
> suitable adjustment to the comment wording
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> Also Intel recommends this for SDVs only - can we consider such a
> setup supported (not to speak of security supported) at all? I guess
> you mean to express this by saying "at their own risk" in the
> cmdline doc? If so, perhaps mentioning this in SUPPORT.md would be
> a good thing nevertheless, notwithstanding the fact that we're not
> really good at expressing there how command line option use affects
> support status.

I think this is too fine grained to be expressed in SUPPORT.md, but
given that there is a very clear specific issue, I wouldn't consider
this an unsupported configuration overall.

I don't expect people to want to use TSX on these CPUs in the first
place (which was a factor in choosing off-by-default), but if they do,
there is one specific memory ordering issue of read/writes with a
committed transaction.

None of our code uses RTM, so issues in Xen which manifest with tsx=1
won't be related to TSX being enabled.

Obviously, if someone does report an issue, we can tell them to
re-confirm the issue without tsx=1 just to rule out interactions, but I
don't think it is likely for it to be relevant to reported issues.

~Andrew