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 <jbeul...@suse.com>
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
