Re: [RFC][PATCH 11/34] x86/cpu/intel: Prepare MKTME for "address configuration" infrastructure

Huang, Kai
2024-02-27T21:48:33Z

On 27/02/2024 1:14 am, Kirill A. Shutemov wrote:
> On Fri, Feb 23, 2024 at 08:22:16AM -0800, Dave Hansen wrote:
>> On 2/23/24 03:33, Kirill A. Shutemov wrote:
>>> On Thu, Feb 22, 2024 at 10:39:41AM -0800, Dave Hansen wrote:
>>>> From: Dave Hansen <dave.hansen@linux.intel.com>
>>>>
>>>> Intel also does memory encryption and also fiddles with the physical
>>>> address bits. This is currently called for *each* CPU, but practically
>>>> only done on the boot CPU because of 'mktme_status'.
>>>>
>>>> Move it from the "each CPU" ->c_init() function to ->c_bsp_init() where
>>>> the whole thing only gets called once ever. This also necessitates moving
>>>> detect_tme() and its entourage around in the file.
>>> The state machine around mktme_state doesn't make sense if we only call it
>>> on boot CPU, so detect_tme() can be drastically simplified. I can do it on
>>> top of the patchset.
>>
>> That would be great. Looking at it again, the (tme_activate !=
>> tme_activate_cpu0) block is total cruft now. It probably just needs to
>> get moved to secondary CPU startup.
>
> I have never saw the check to be useful. I think it can be just dropped.
>
> The patch below makes detect_tme() only enumerate TME and MKTME. And
> report number of keyid bits. Kernel doesn't care about anything else.
>
> Any comments?
>
> From 1080535093d21f025d46fd610de5ad788591f4b5 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Mon, 26 Feb 2024 14:01:01 +0200
> Subject: [PATCH] x86/cpu/intel: Simplify detect_tme()
>
> The detect_tme() function is now only called by the boot CPU. The logic
> for cross-checking TME configuration between CPUs is no longer used. It
> has never identified a real problem and can be safely removed.
>
> The kernel does not use MKTME and is not concerned with MKTME policy or
> encryption algorithms. There is no need to check them.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
> arch/x86/kernel/cpu/intel.c | 44 ++-----------------------------------
> 1 file changed, 2 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 4192aa4576f4..60918b49344c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -329,55 +329,20 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> #define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0xffff) /* Bits 63:48 */
> #define TME_ACTIVATE_CRYPTO_AES_XTS_128 1
>
> -/* Values for mktme_status (SW only construct) */
> -#define MKTME_ENABLED 0
> -#define MKTME_DISABLED 1
> -#define MKTME_UNINITIALIZED 2
> -static int mktme_status = MKTME_UNINITIALIZED;
> -
> static int detect_tme(struct cpuinfo_x86 *c)
> {
> - u64 tme_activate, tme_policy, tme_crypto_algs;
> - int keyid_bits = 0, nr_keyids = 0;
> - static u64 tme_activate_cpu0 = 0;
> + int keyid_bits, nr_keyids;
> + u64 tme_activate;
>
> rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
>
> - if (mktme_status != MKTME_UNINITIALIZED) {
> - if (tme_activate != tme_activate_cpu0) {
> - /* Broken BIOS? */
> - pr_err_once("x86/tme: configuration is inconsistent between CPUs\n");
> - pr_err_once("x86/tme: MKTME is not usable\n");
> - mktme_status = MKTME_DISABLED;
> -
> - /* Proceed. We may need to exclude bits from x86_phys_bits. */
> - }
> - } else {
> - tme_activate_cpu0 = tme_activate;
> - }
> -
> if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
> pr_info_once("x86/tme: not enabled by BIOS\n");

Since now detect_tme() is only called on BSP, seems we can change to
pr_info(), which is used ...
> - mktme_status = MKTME_DISABLED;
> return 0;
> }
>
> - if (mktme_status != MKTME_UNINITIALIZED)
> - goto detect_keyid_bits;
> -
> pr_info("x86/tme: enabled by BIOS\n");

... right here anyway.

>
> - tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> - if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> - pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
> -
> - tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> - if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> - pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
> - tme_crypto_algs);
> - mktme_status = MKTME_DISABLED;
> - }
> -detect_keyid_bits:
> keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> nr_keyids = (1UL << keyid_bits) - 1;
> if (nr_keyids) {
> @@ -387,11 +352,6 @@ static int detect_tme(struct cpuinfo_x86 *c)
> pr_info_once("x86/mktme: disabled by BIOS\n");
> }
>
> - if (mktme_status == MKTME_UNINITIALIZED) {
> - /* MKTME is usable */
> - mktme_status = MKTME_ENABLED;
> - }
> -
> return keyid_bits;

Other than that the code change LGTM.

Reviewed-by: Kai Huang <kai.huang@intel.com>