On TME enabled platform, BIOS publishes MTRR taking into account Total
Memory Encryption (TME) reserved bits.
generic_get_mtrr() performs a sanity check of the MTRRs relying on the
`phys_hi_rsvd' variable which is set using the cpuinfo_x86 structure
`x86_phys_bits' field. But at the time the generic_get_mtrr()
function is ran the `x86_phys_bits' has not been updated by
detect_tme() when TME is enabled.
Since the x86_phys_bits does not reflect yet the real maximal physical
address size yet generic_get_mtrr() complains by logging the following
messages.
mtrr: your BIOS has configured an incorrect mask, fixing it.
mtrr: your BIOS has configured an incorrect mask, fixing it.
[...]
For `x86_phys_bits' to be updated before generic_get_mtrr() runs, this
patch moves the detect_tme() call from init_intel() to
early_init_intel().
Signed-off-by: Jeremy Compostella <[email protected]>
---
arch/x86/kernel/cpu/intel.c | 174 ++++++++++++++++++------------------
1 file changed, 87 insertions(+), 87 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index be4045628fd3..34c54432bf00 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -184,6 +184,90 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
return false;
}
+#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 early_init_intel(struct cpuinfo_x86 *c)
{
u64 misc_enable;
@@ -335,6 +419,9 @@ static void early_init_intel(struct cpuinfo_x86 *c)
*/
if (detect_extended_topology_early(c) < 0)
detect_ht_early(c);
+
+ if (cpu_has(c, X86_FEATURE_TME))
+ detect_tme(c);
}
static void bsp_init_intel(struct cpuinfo_x86 *c)
@@ -495,90 +582,6 @@ static void srat_detect_node(struct cpuinfo_x86 *c)
#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;
@@ -715,9 +718,6 @@ static void init_intel(struct cpuinfo_x86 *c)
init_ia32_feat_ctl(c);
- if (cpu_has(c, X86_FEATURE_TME))
- detect_tme(c);
-
init_intel_misc_features(c);
split_lock_init();
--
2.40.1
Hi Jeremy,
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index be4045628fd3..34c54432bf00 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -184,6 +184,90 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
> return false;
> }
>
> +#define MSR_IA32_TME_ACTIVATE 0x982
I know you're just moving the definitions, however we usually define MSRs
and their bits in arch/x86/include/asm/msr-index.h.
> +
> +/* Helpers to access TME_ACTIVATE MSR */
> +#define TME_ACTIVATE_LOCKED(x) (x & 0x1)
> +#define TME_ACTIVATE_ENABLED(x) (x & 0x2)
What about:
#define TME_ACTIVATE_LOCKED(x) (x & BIT(0))
#define TME_ACTIVATE_ENABLED(x) (x & BIT(1))
> +
> +#define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
And:
/* Bits 7:4 are TME activate policy bits */
#define TME_ACTIVATE_POLICY_OFFSET 4
#define TME_ACTIVATE_POLICY_MASK 0xf
#define TME_ACTIVATE_POLICY(x) \
((x >> TME_ACTIVATE_POLICY_OFFSET) & TME_ACTIVATE_POLICY_MASK)
> +#define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
> +
> +#define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0xffff) /* Bits
> 63:48 */
ditto
> @@ -335,6 +419,9 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> */
> if (detect_extended_topology_early(c) < 0)
> detect_ht_early(c);
> +
Please add a comment here explaining why detect_tme() needs to be called
in early_init_intel().
> + if (cpu_has(c, X86_FEATURE_TME))
> + detect_tme(c);
> }
>
> static void bsp_init_intel(struct cpuinfo_x86 *c)
Thanks!
Xin
On Sat, 2023-09-23 at 07:37 +0000, Li, Xin3 wrote:
> Hi Jeremy,
>
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index be4045628fd3..34c54432bf00 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -184,6 +184,90 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
> > return false;
> > }
> >
> > +#define MSR_IA32_TME_ACTIVATE 0x982
>
> I know you're just moving the definitions, however we usually define MSRs
> and their bits in arch/x86/include/asm/msr-index.h.
>
> > +
> > +/* Helpers to access TME_ACTIVATE MSR */
> > +#define TME_ACTIVATE_LOCKED(x) (x & 0x1)
> > +#define TME_ACTIVATE_ENABLED(x) (x & 0x2)
>
> What about:
>
> #define TME_ACTIVATE_LOCKED(x) (x & BIT(0))
> #define TME_ACTIVATE_ENABLED(x) (x & BIT(1))
(Also + Kirill who is the author of detect_tme())
By moving these bit definition to msr-index.h, IMHO we should just define the
macros for the bit positions, but remove the (x) part. This is consistent with
all other existing definitions.
Something like:
#define TME_ACTIVATE_LOCKED BIT(0)
...
>
> > +
> > +#define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
>
> And:
>
> /* Bits 7:4 are TME activate policy bits */
> #define TME_ACTIVATE_POLICY_OFFSET 4
> #define TME_ACTIVATE_POLICY_MASK 0xf
> #define TME_ACTIVATE_POLICY(x) \
> ((x >> TME_ACTIVATE_POLICY_OFFSET) & TME_ACTIVATE_POLICY_MASK)
>
> > +#define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
> > +
> > +#define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0xffff) /* Bits
> > 63:48 */
>
> ditto
>
> > @@ -335,6 +419,9 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> > */
> > if (detect_extended_topology_early(c) < 0)
> > detect_ht_early(c);
> > +
>
> Please add a comment here explaining why detect_tme() needs to be called
> in early_init_intel().
Also this works because init_intel() also calls early_init_intel(). I noticed
before that people don't like this. So maybe it's worth adding some extra words
saying this, in case people may want to stop calling early_init_intel() from
init_intel() in the future, but I am not sure whether this is worth pointing
out.
Or, should we change to call detect_tme() from bsp_init_intel() instead of
early_init_intel(), and still keep calling detect_tme() from init_intel()?
This also avoids calling detect_tme() twice for BSP IIUC, because AFAICT
early_init_intel() is called twice for BSP.
>
> > + if (cpu_has(c, X86_FEATURE_TME))
> > + detect_tme(c);
> > }
> >
> > static void bsp_init_intel(struct cpuinfo_x86 *c)
>
> Thanks!
> Xin