From: Alison Schofield <[email protected]>
MKTME detection was added in anticipation of full kernel support
that never followed. Aside from just good housekeeping, this
cleanup is inspired by users who are confused by the TME/MKTME
messaging during boot.
The first patch cleans up the TME & MKTME detection code and the
second patch removes the unused pconfig code.
Testing was done on a platform supporting MKTME using the BIOS
option to enable/disable MKTME prior to boot.
Alison Schofield (2):
x86/cpu: Remove useless work in detect_tme_early()
x86/pconfig: Remove unused MKTME pconfig code
arch/x86/include/asm/intel_pconfig.h | 65 ---------------------
arch/x86/kernel/cpu/Makefile | 2 +-
arch/x86/kernel/cpu/intel.c | 71 ++++-------------------
arch/x86/kernel/cpu/intel_pconfig.c | 84 ----------------------------
4 files changed, 13 insertions(+), 209 deletions(-)
delete mode 100644 arch/x86/include/asm/intel_pconfig.h
delete mode 100644 arch/x86/kernel/cpu/intel_pconfig.c
base-commit: 5a04007fb7a9a3f101fe551f0e47849f2cd1125b
--
2.37.3
From: Alison Schofield <[email protected]>
TME (Total Memory Encryption) and MKTME (Multi-Key Total Memory
Encryption) BIOS detection were introduced together here [1] and
are loosely coupled in the Intel CPU init code.
TME is a hardware only feature and its BIOS status is all that needs
to be shared with the kernel user: enabled or disabled. The TME
algorithm the BIOS is using and whether or not the kernel recognizes
that algorithm is useless to the kernel user.
MKTME is a hardware feature that requires kernel support. MKTME
detection code was added in advance of broader kernel support for
MKTME that never followed. So, rather than continuing to emit
needless and confusing messages about BIOS MKTME status, remove
most of the MKTME pieces from detect_tme_early().
Keep one important piece: when the BIOS is configured with MKTME
'on' any BIOS defined KeyID bits do take away from the physaddr bits
available in the kernel. Add a pr_info_once() informing about the
enabled keyids so the user can address (by rebooting with MKTME off)
if the user needs to recover the MKTME consumed bits.
There is no functional change for the user, only this change in boot
messages:
Before:
[] x86/tme: enabled by BIOS
[] x86/tme: Unknown policy is active: 0x2
[] x86/mktme: No known encryption algorithm is supported: 0x4
[] x86/mktme: enabled by BIOS
[] x86/mktme: 127 KeyIDs available
After:
[] x86/tme: enabled by BIOS
[] x86/mktme: BIOS enabled 127 keyids
[1] cb06d8e3d020 ("x86/tme: Detect if TME and MKTME is activated by BIOS")
Signed-off-by: Alison Schofield <[email protected]>
---
arch/x86/kernel/cpu/intel.c | 71 +++++++------------------------------
1 file changed, 12 insertions(+), 59 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 3c3e7e5695ba..83865897a2a7 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -190,83 +190,36 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
#define TME_ACTIVATE_LOCKED(x) (x & 0x1)
#define TME_ACTIVATE_ENABLED(x) (x & 0x2)
-#define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
-#define TME_ACTIVATE_POLICY_AES_XTS_128 0
-
#define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
-#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 void detect_tme_early(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;
+ 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");
- mktme_status = MKTME_DISABLED;
clear_cpu_cap(c, X86_FEATURE_TME);
return;
}
-
- if (mktme_status != MKTME_UNINITIALIZED)
- goto detect_keyid_bits;
-
- pr_info("x86/tme: enabled by BIOS\n");
-
- 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:
+ pr_info_once("x86/tme: enabled by BIOS\n");
keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
- nr_keyids = (1UL << keyid_bits) - 1;
- if (nr_keyids) {
- pr_info_once("x86/mktme: enabled by BIOS\n");
- pr_info_once("x86/mktme: %d KeyIDs available\n", nr_keyids);
- } else {
- pr_info_once("x86/mktme: disabled by BIOS\n");
- }
-
- if (mktme_status == MKTME_UNINITIALIZED) {
- /* MKTME is usable */
- mktme_status = MKTME_ENABLED;
- }
+ if (!keyid_bits)
+ return;
/*
- * KeyID bits effectively lower the number of physical address
- * bits. Update cpuinfo_x86::x86_phys_bits accordingly.
+ * KeyID bits are set by BIOS and can be present regardless
+ * of whether the kernel is using them. They effectively lower
+ * the number of physical address bits.
+ *
+ * Update cpuinfo_x86::x86_phys_bits accordingly.
*/
c->x86_phys_bits -= keyid_bits;
+ nr_keyids = (1UL << keyid_bits) - 1;
+
+ pr_info_once("x86/mktme: BIOS enabled %d keyids\n", nr_keyids);
}
static void early_init_intel(struct cpuinfo_x86 *c)
--
2.37.3
From: Alison Schofield <[email protected]>
Code supporting Intel PCONFIG targets was an early piece of enabling
for MKTME (Multi-Key Total Memory Encryption).
Since MKTME feature enablement did not follow into the kernel, remove
the unused PCONFIG code.
Signed-off-by: Alison Schofield <[email protected]>
---
arch/x86/include/asm/intel_pconfig.h | 65 ---------------------
arch/x86/kernel/cpu/Makefile | 2 +-
arch/x86/kernel/cpu/intel_pconfig.c | 84 ----------------------------
3 files changed, 1 insertion(+), 150 deletions(-)
delete mode 100644 arch/x86/include/asm/intel_pconfig.h
delete mode 100644 arch/x86/kernel/cpu/intel_pconfig.c
diff --git a/arch/x86/include/asm/intel_pconfig.h b/arch/x86/include/asm/intel_pconfig.h
deleted file mode 100644
index 994638ef171b..000000000000
--- a/arch/x86/include/asm/intel_pconfig.h
+++ /dev/null
@@ -1,65 +0,0 @@
-#ifndef _ASM_X86_INTEL_PCONFIG_H
-#define _ASM_X86_INTEL_PCONFIG_H
-
-#include <asm/asm.h>
-#include <asm/processor.h>
-
-enum pconfig_target {
- INVALID_TARGET = 0,
- MKTME_TARGET = 1,
- PCONFIG_TARGET_NR
-};
-
-int pconfig_target_supported(enum pconfig_target target);
-
-enum pconfig_leaf {
- MKTME_KEY_PROGRAM = 0,
- PCONFIG_LEAF_INVALID,
-};
-
-#define PCONFIG ".byte 0x0f, 0x01, 0xc5"
-
-/* Defines and structure for MKTME_KEY_PROGRAM of PCONFIG instruction */
-
-/* mktme_key_program::keyid_ctrl COMMAND, bits [7:0] */
-#define MKTME_KEYID_SET_KEY_DIRECT 0
-#define MKTME_KEYID_SET_KEY_RANDOM 1
-#define MKTME_KEYID_CLEAR_KEY 2
-#define MKTME_KEYID_NO_ENCRYPT 3
-
-/* mktme_key_program::keyid_ctrl ENC_ALG, bits [23:8] */
-#define MKTME_AES_XTS_128 (1 << 8)
-
-/* Return codes from the PCONFIG MKTME_KEY_PROGRAM */
-#define MKTME_PROG_SUCCESS 0
-#define MKTME_INVALID_PROG_CMD 1
-#define MKTME_ENTROPY_ERROR 2
-#define MKTME_INVALID_KEYID 3
-#define MKTME_INVALID_ENC_ALG 4
-#define MKTME_DEVICE_BUSY 5
-
-/* Hardware requires the structure to be 256 byte aligned. Otherwise #GP(0). */
-struct mktme_key_program {
- u16 keyid;
- u32 keyid_ctrl;
- u8 __rsvd[58];
- u8 key_field_1[64];
- u8 key_field_2[64];
-} __packed __aligned(256);
-
-static inline int mktme_key_program(struct mktme_key_program *key_program)
-{
- unsigned long rax = MKTME_KEY_PROGRAM;
-
- if (!pconfig_target_supported(MKTME_TARGET))
- return -ENXIO;
-
- asm volatile(PCONFIG
- : "=a" (rax), "=b" (key_program)
- : "0" (rax), "1" (key_program)
- : "memory", "cc");
-
- return rax;
-}
-
-#endif /* _ASM_X86_INTEL_PCONFIG_H */
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index eb4dbcdf41f1..ecd78a61da2e 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -34,7 +34,7 @@ obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_IA32_FEAT_CTL) += feat_ctl.o
ifdef CONFIG_CPU_SUP_INTEL
-obj-y += intel.o intel_pconfig.o tsx.o
+obj-y += intel.o tsx.o
obj-$(CONFIG_PM) += intel_epb.o
endif
obj-$(CONFIG_CPU_SUP_AMD) += amd.o
diff --git a/arch/x86/kernel/cpu/intel_pconfig.c b/arch/x86/kernel/cpu/intel_pconfig.c
deleted file mode 100644
index 5be2b1790282..000000000000
--- a/arch/x86/kernel/cpu/intel_pconfig.c
+++ /dev/null
@@ -1,84 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Intel PCONFIG instruction support.
- *
- * Copyright (C) 2017 Intel Corporation
- *
- * Author:
- * Kirill A. Shutemov <[email protected]>
- */
-#include <linux/bug.h>
-#include <linux/limits.h>
-
-#include <asm/cpufeature.h>
-#include <asm/intel_pconfig.h>
-
-#define PCONFIG_CPUID 0x1b
-
-#define PCONFIG_CPUID_SUBLEAF_MASK ((1 << 12) - 1)
-
-/* Subleaf type (EAX) for PCONFIG CPUID leaf (0x1B) */
-enum {
- PCONFIG_CPUID_SUBLEAF_INVALID = 0,
- PCONFIG_CPUID_SUBLEAF_TARGETID = 1,
-};
-
-/* Bitmask of supported targets */
-static u64 targets_supported __read_mostly;
-
-int pconfig_target_supported(enum pconfig_target target)
-{
- /*
- * We would need to re-think the implementation once we get > 64
- * PCONFIG targets. Spec allows up to 2^32 targets.
- */
- BUILD_BUG_ON(PCONFIG_TARGET_NR >= 64);
-
- if (WARN_ON_ONCE(target >= 64))
- return 0;
- return targets_supported & (1ULL << target);
-}
-
-static int __init intel_pconfig_init(void)
-{
- int subleaf;
-
- if (!boot_cpu_has(X86_FEATURE_PCONFIG))
- return 0;
-
- /*
- * Scan subleafs of PCONFIG CPUID leaf.
- *
- * Subleafs of the same type need not to be consecutive.
- *
- * Stop on the first invalid subleaf type. All subleafs after the first
- * invalid are invalid too.
- */
- for (subleaf = 0; subleaf < INT_MAX; subleaf++) {
- struct cpuid_regs regs;
-
- cpuid_count(PCONFIG_CPUID, subleaf,
- ®s.eax, ®s.ebx, ®s.ecx, ®s.edx);
-
- switch (regs.eax & PCONFIG_CPUID_SUBLEAF_MASK) {
- case PCONFIG_CPUID_SUBLEAF_INVALID:
- /* Stop on the first invalid subleaf */
- goto out;
- case PCONFIG_CPUID_SUBLEAF_TARGETID:
- /* Mark supported PCONFIG targets */
- if (regs.ebx < 64)
- targets_supported |= (1ULL << regs.ebx);
- if (regs.ecx < 64)
- targets_supported |= (1ULL << regs.ecx);
- if (regs.edx < 64)
- targets_supported |= (1ULL << regs.edx);
- break;
- default:
- /* Unknown CPUID.PCONFIG subleaf: ignore */
- break;
- }
- }
-out:
- return 0;
-}
-arch_initcall(intel_pconfig_init);
--
2.37.3
On Thu, Apr 25, 2024 at 09:24:51PM -0700, [email protected] wrote:
> From: Alison Schofield <[email protected]>
>
> MKTME detection was added in anticipation of full kernel support
> that never followed. Aside from just good housekeeping, this
> cleanup is inspired by users who are confused by the TME/MKTME
> messaging during boot.
>
> The first patch cleans up the TME & MKTME detection code and the
> second patch removes the unused pconfig code.
>
> Testing was done on a platform supporting MKTME using the BIOS
> option to enable/disable MKTME prior to boot.
>
>
> Alison Schofield (2):
> x86/cpu: Remove useless work in detect_tme_early()
> x86/pconfig: Remove unused MKTME pconfig code
Acked-by: Kirill A. Shutemov <[email protected]>
--
Kiryl Shutsemau / Kirill A. Shutemov
On Thu, 2024-04-25 at 21:24 -0700, [email protected] wrote:
> From: Alison Schofield <[email protected]>
>
> Code supporting Intel PCONFIG targets was an early piece of enabling
> for MKTME (Multi-Key Total Memory Encryption).
>
> Since MKTME feature enablement did not follow into the kernel, remove
> the unused PCONFIG code.
>
> Signed-off-by: Alison Schofield <[email protected]>
>
Acked-by: Kai Huang <[email protected]>
On Thu, 2024-04-25 at 21:24 -0700, [email protected] wrote:
> From: Alison Schofield <[email protected]>
>
> TME (Total Memory Encryption) and MKTME (Multi-Key Total Memory
> Encryption) BIOS detection were introduced together here [1] and
> are loosely coupled in the Intel CPU init code.
>
> TME is a hardware only feature and its BIOS status is all that needs
> to be shared with the kernel user: enabled or disabled. The TME
> algorithm the BIOS is using and whether or not the kernel recognizes
> that algorithm is useless to the kernel user.
>
> MKTME is a hardware feature that requires kernel support. MKTME
> detection code was added in advance of broader kernel support for
> MKTME that never followed. So, rather than continuing to emit
> needless and confusing messages about BIOS MKTME status, remove
> most of the MKTME pieces from detect_tme_early().
>
> Keep one important piece: when the BIOS is configured with MKTME
> 'on' any BIOS defined KeyID bits do take away from the physaddr bits
> available in the kernel. Add a pr_info_once() informing about the
> enabled keyids so the user can address (by rebooting with MKTME off)
> if the user needs to recover the MKTME consumed bits.
Nitpickings below:
The original code checks the MSR value consistency between BSP and APs,
but the new code removed that.
I think the changelog should mention why it is OK.
I guess perhaps we can say something like the machine shouldn't be able to
boot if BIOS configures TME activate MSR inconsistently among CPUs, so
don't bother to have the consistency check.
>
> There is no functional change for the user, only this change in boot
> messages:
>
> Before:
> [] x86/tme: enabled by BIOS
> [] x86/tme: Unknown policy is active: 0x2
> [] x86/mktme: No known encryption algorithm is supported: 0x4
> [] x86/mktme: enabled by BIOS
> [] x86/mktme: 127 KeyIDs available
>
> After:
> [] x86/tme: enabled by BIOS
> [] x86/mktme: BIOS enabled 127 keyids
Regarding to what to print:
1) IMHO, it's still better to print the algorithm here. The user/admin
may want to know what algorithm is enabled by the BIOS (especially there
are more than one that can be selected in the BIOS). E.g., the user may
find the default AES-XTS-128 (0) isn't secure enough and wants the more
secure algorithm using 256-bit key.
I understand we might not want to maintain a "value to name" table for
this so the kernel can print the algorithm in name, but it would be still
helpful if we just dump the raw value here like:
x86/tme: policy activated: 0x2
2) Given the kernel doesn't support MKTME, I think people may be more
interested in the reduced physical address bits, instead of how MKTME
keyIDs are activated.
x86/tme: MKTME enabled, physical address bits reduced from <xx> to <xx>.
>
> [1] cb06d8e3d020 ("x86/tme: Detect if TME and MKTME is activated by BIOS")
>
> Signed-off-by: Alison Schofield <[email protected]>
> ---
> arch/x86/kernel/cpu/intel.c | 71 +++++++------------------------------
> 1 file changed, 12 insertions(+), 59 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 3c3e7e5695ba..83865897a2a7 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -190,83 +190,36 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
> #define TME_ACTIVATE_LOCKED(x) (x & 0x1)
> #define TME_ACTIVATE_ENABLED(x) (x & 0x2)
>
> -#define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
> -#define TME_ACTIVATE_POLICY_AES_XTS_128 0
> -
> #define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
>
> -#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 void detect_tme_early(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;
> + 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");
> - mktme_status = MKTME_DISABLED;
> clear_cpu_cap(c, X86_FEATURE_TME);
> return;
> }
> -
> - if (mktme_status != MKTME_UNINITIALIZED)
> - goto detect_keyid_bits;
> -
> - pr_info("x86/tme: enabled by BIOS\n");
> -
> - 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:
> + pr_info_once("x86/tme: enabled by BIOS\n");
> keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> - nr_keyids = (1UL << keyid_bits) - 1;
> - if (nr_keyids) {
> - pr_info_once("x86/mktme: enabled by BIOS\n");
> - pr_info_once("x86/mktme: %d KeyIDs available\n", nr_keyids);
> - } else {
> - pr_info_once("x86/mktme: disabled by BIOS\n");
> - }
> -
> - if (mktme_status == MKTME_UNINITIALIZED) {
> - /* MKTME is usable */
> - mktme_status = MKTME_ENABLED;
> - }
> + if (!keyid_bits)
> + return;
>
> /*
> - * KeyID bits effectively lower the number of physical address
> - * bits. Update cpuinfo_x86::x86_phys_bits accordingly.
> + * KeyID bits are set by BIOS and can be present regardless
> + * of whether the kernel is using them. They effectively lower
> + * the number of physical address bits.
> + *
> + * Update cpuinfo_x86::x86_phys_bits accordingly.
> */
> c->x86_phys_bits -= keyid_bits;
> + nr_keyids = (1UL << keyid_bits) - 1;
> +
> + pr_info_once("x86/mktme: BIOS enabled %d keyids\n", nr_keyids);
If I read correctly, if you print
physical address bits reduced from <xx> to <xx>.
instead of the number of KeyIDs, you can even get rid of the 'nr_keyids'
variable.
On Mon, Apr 29, 2024 at 04:17:12AM -0700, Huang, Kai wrote:
> On Thu, 2024-04-25 at 21:24 -0700, [email protected] wrote:
> > From: Alison Schofield <[email protected]>
> >
> > TME (Total Memory Encryption) and MKTME (Multi-Key Total Memory
> > Encryption) BIOS detection were introduced together here [1] and
> > are loosely coupled in the Intel CPU init code.
> >
> > TME is a hardware only feature and its BIOS status is all that needs
> > to be shared with the kernel user: enabled or disabled. The TME
> > algorithm the BIOS is using and whether or not the kernel recognizes
> > that algorithm is useless to the kernel user.
> >
> > MKTME is a hardware feature that requires kernel support. MKTME
> > detection code was added in advance of broader kernel support for
> > MKTME that never followed. So, rather than continuing to emit
> > needless and confusing messages about BIOS MKTME status, remove
> > most of the MKTME pieces from detect_tme_early().
> >
> > Keep one important piece: when the BIOS is configured with MKTME
> > 'on' any BIOS defined KeyID bits do take away from the physaddr bits
> > available in the kernel. Add a pr_info_once() informing about the
> > enabled keyids so the user can address (by rebooting with MKTME off)
> > if the user needs to recover the MKTME consumed bits.
>
> Nitpickings below:
Hi Kai,
Thanks for the review -
>
> The original code checks the MSR value consistency between BSP and APs,
> but the new code removed that.
BSP? AP?
I understood the check you are referring to as being for pronouncing MKTME
as disabled. When MKTME is removed from consideration, as is done here, the
code doesn't need to remember and compare a 'state'. It examines tme_activate
of each CPU. If one is !_LOCKED or !_ENABLED, the 'not enabled by BIOS'
message is emitted and the cpu cap is cleared. If another CPU reports TME
as properly enabled before or after that failed CPU, it is still disabled
in the kernel view. So, we don't set a state, like was done for MKTME, but
the worst status is *the* TME status.
>
> I think the changelog should mention why it is OK.
>
> I guess perhaps we can say something like the machine shouldn't be able to
> boot if BIOS configures TME activate MSR inconsistently among CPUs, so
> don't bother to have the consistency check.
I basically understand what you are saying, but because I don't see
that explanation in the spec, I wouldn't write that this code relies
on it.
>
> >
> > There is no functional change for the user, only this change in boot
> > messages:
> >
> > Before:
> > [] x86/tme: enabled by BIOS
> > [] x86/tme: Unknown policy is active: 0x2
> > [] x86/mktme: No known encryption algorithm is supported: 0x4
> > [] x86/mktme: enabled by BIOS
> > [] x86/mktme: 127 KeyIDs available
> >
> > After:
> > [] x86/tme: enabled by BIOS
> > [] x86/mktme: BIOS enabled 127 keyids
>
> Regarding to what to print:
>
> 1) IMHO, it's still better to print the algorithm here. The user/admin
> may want to know what algorithm is enabled by the BIOS (especially there
> are more than one that can be selected in the BIOS). E.g., the user may
> find the default AES-XTS-128 (0) isn't secure enough and wants the more
> secure algorithm using 256-bit key.
>
> I understand we might not want to maintain a "value to name" table for
> this so the kernel can print the algorithm in name, but it would be still
> helpful if we just dump the raw value here like:
>
> x86/tme: policy activated: 0x2
The path that is cleaned up here never emitted the policies, either by
value or name. It only emitted a message if the policy was unknown. This
is a new (albeit little) feature request and seems it would be best
submitted as a separate patch.
>
> 2) Given the kernel doesn't support MKTME, I think people may be more
> interested in the reduced physical address bits, instead of how MKTME
> keyIDs are activated.
>
> x86/tme: MKTME enabled, physical address bits reduced from <xx> to <xx>.
I added something like this in v2:
x86/mktme: BIOS enabled: x86_phys_bits reduced by %d
I chose to report just the number of bits reduced because it is not
clear to me that this code is, or always will be, the only path
mucking with the number of phys addr bits. For that reason I didn't
want to spew absolute values. User can look up their 'final' number
using lscpu or /proc/cpu, right?
snip
> > +
> > + pr_info_once("x86/mktme: BIOS enabled %d keyids\n", nr_keyids);
>
> If I read correctly, if you print
>
> physical address bits reduced from <xx> to <xx>.
>
as I wrote above, didn't write it quite like that...
> instead of the number of KeyIDs, you can even get rid of the 'nr_keyids'
> variable.
Got rid of nr_keyids.
-- Alison