[RFC,11/34] x86/cpu/intel: Prepare MKTME for "address configuration" infrastructure
Commit Message
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.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---
b/arch/x86/kernel/cpu/intel.c | 174 +++++++++++++++++++++---------------------
1 file changed, 87 insertions(+), 87 deletions(-)
Comments
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.
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.
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");
- mktme_status = MKTME_DISABLED;
return 0;
}
- 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:
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;
}
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>
On Wed, Feb 28, 2024 at 10:48:19AM +1300, Huang, Kai wrote:
> > 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 ...
Right.
Dave, do you want a new version? Or can you fix it up on apply?
On 2/28/24 07:20, Kirill A. Shutemov wrote:
> On Wed, Feb 28, 2024 at 10:48:19AM +1300, Huang, Kai wrote:
>>> 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 ...
> Right.
>
> Dave, do you want a new version? Or can you fix it up on apply?
I've already got a version of your patch pulled into my tree. I did the
conversion there. Thanks for asking!
@@ -324,9 +324,96 @@ static void early_init_intel(struct cpui
detect_ht_early(c);
}
+#define MSR_IA32_TME_ACTIVATE 0x982
+
+/* Helpers to access TME_ACTIVATE MSR */
+#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(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;
+
+ 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;
+ 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:
+ 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;
+ }
+
+ /*
+ * KeyID bits effectively lower the number of physical address
+ * bits. Update cpuinfo_x86::x86_phys_bits accordingly.
+ */
+ c->x86_phys_bits -= keyid_bits;
+}
+
static void bsp_init_intel(struct cpuinfo_x86 *c)
{
resctrl_cpu_detect(c);
+
+ if (cpu_has(c, X86_FEATURE_TME))
+ detect_tme(c);
}
#ifdef CONFIG_X86_32
@@ -482,90 +569,6 @@ static void srat_detect_node(struct cpui
#endif
}
-#define MSR_IA32_TME_ACTIVATE 0x982
-
-/* Helpers to access TME_ACTIVATE MSR */
-#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(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;
-
- 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;
- 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:
- 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;
- }
-
- /*
- * KeyID bits effectively lower the number of physical address
- * bits. Update cpuinfo_x86::x86_phys_bits accordingly.
- */
- c->x86_phys_bits -= keyid_bits;
-}
-
static void init_cpuid_fault(struct cpuinfo_x86 *c)
{
u64 msr;
@@ -702,9 +705,6 @@ static void init_intel(struct cpuinfo_x8
init_ia32_feat_ctl(c);
- if (cpu_has(c, X86_FEATURE_TME))
- detect_tme(c);
-
init_intel_misc_features(c);
split_lock_init();