[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 9 Jun 2021 13:19:03 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=N+Ooql3dIeG18DuyM6dRW/eyiblxjPfAJKbTM/sKP0A=; b=CEpOHJMjOE4LYZMQM1x8WgWJe/EECyvm9k1AC0v9t5AmL9wwgcmPCXV2sIyhVSBI4MmXXJdu5VcqWSHxqIdHGIBOsdcpukElcwcKnAiUIKNcZ+vkIJyzJQPvm/MDdqf7PyPolYZ31nRA5Fje29NRu0yFJ7o0A9XxoxNVGIAUfGAxDEx2hR8wcNXLTT8hmKJqjzf7VyvJH71iumVbIlp+veixzWLsTXuovjp+NWBscwU1Is1e0PnqkW3ZYD+wcHfC8q1z0XeKI6utI3CYqOp/3hHRJkjKX6OH0QhFxGNQ3kvM6fXr0dRvH5FlTM8HvDhBVfAVs9ESd0vb5qfemW6/Yg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FchuKULF//jPlvdShuEdq1DPvZF90lnLfNHx6KeRDk8RMKfFNPdKx/DIznFiM4zRkMz9uayFxsl6Y4nssHsBx1HtbdywbA4LUp5QfZcKt/hPrKUk5sfF6tO0/KWazE8av5Dx0siYJZX+L70N8kTAhzcMU6dmNBTBvSZnL+awF5BohP6j6gHmWqE1L8UEp+qkw7ZKW+kc5zD4IAaYY5Sn+RT5DpIpdgQvPyTlW5iiz8+KFTUcGk53aGE6aS5rvULLqk0xuHVt6uEpTQs2JXyEuJA5nZsTV+TeGokCQVnnViCsmD68C2NRHzdmrjwDmfhqExIAau4floJWotwLUVAheA==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 09 Jun 2021 12:19:24 +0000
  • Ironport-hdrordr: A9a23:vGQN3K69jOGimzc10gPXwSqBI+orL9Y04lQ7vn2ZFiY7TiXIra yTdaoguCMc6AxxZJkh8erwXZVoMkmsiqKdhrNhQYtKPTOWxVdASbsN0WKM+UyZJ8STzJ876U 4kSdkFNDSSNykIsS+Z2njALz9I+rDum8rJ9ISuukuFDzsaD52Ihz0JejpzeXcGIjWua6BJdq Z0qvA33AZJLh8sH7WG7zQ+Lqf+juyOsKijTQ8NBhYh5gXLpTS06ITiGxzd+hsFSTtAzZor7G CAymXCl+SemsD+7iWZ+37Y7pxQltek4txfBPaUgsxQDjn3kA6naKloRrXHljEop+OE7kosjb D30lkdFvU2z0mUUnC+oBPr1QWl+i0p8WXexViRhmamidDlRRohYvAxx75xQ1/80Q4Nrdt82K VE0yayrJxMFy7Nmyz7+pzhSwxqrEypunAv+NRjzEC3abFuLIO5kLZvu3+8SPw7bWTHAcEcYa lT5fjnlbNrmQjwVQGBgoEHq+bcLEjaHX+9MwI/U4KuomBrdN0Q9TpQ+CUlpAZ2yHsKcegO2w 31CNUdqFhwdL5hUUtcPpZNfSLlMB2AffrzWFjiaWgPQ5t3RU4l7aSHu4kI2A==
  • Ironport-sdr: bpcb30iZyb3rr96Es1eOanX/FpOv068DReRijKSId/vUyL0ur2xo4ZBtXhd/3+A2ufWwTYkEoy 9ANIfEvDHK41/z7KCxU73FLuvVUFi3x04SKFc0IDZAAXOPPTFxXhmfpdmYIF0wr89Po5OsU3sw emjOxlMoPR85Pf5eI+aMTJTJBWAZ88+rDtz04wT3GPqw6/SBcvr9Kb3XMDa+7s3bhkNXle7Iqh t7fO1t5OjzJKxrOo77tke8NT7taS5fs7x7HgcYq+Iy1XnFZNSyBI3qLuF1FUN4bss00PPUaNMK iC0=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.