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

[Jan Beulich <jbeulich@xxxxxxxx>]

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"?

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

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.

Jan