Re: [PATCH v3 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

From: Juergen Gross
Date: Mon Feb 27 2023 - 02:13:49 EST


On 26.02.23 18:12, Michael Kelley (LINUX) wrote:
From: Juergen Gross <jgross@xxxxxxxx> Sent: Thursday, February 23, 2023 1:33 AM

When running virtualized, MTRR access can be reduced (e.g. in Xen PV
guests or when running as a SEV-SNP guest under Hyper-V). Typically
the hypervisor will reset the MTRR feature in CPUID data, resulting
in no MTRR memory type information being available for the kernel.

This has turned out to result in problems:

- Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
- Xen PV dom0 mapping memory as WB which should be UC- instead

Solve those problems by supporting to set a static MTRR state,
overwriting the empty state used today. In case such a state has been
set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
will only be used by mtrr_type_lookup(), as in all other cases
mtrr_enabled() is being checked, which will return false. Accept the
overwrite call only for selected cases when running as a guest.
Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
just refusing them.

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
V2:
- new patch
V3:
- omit fixed MTRRs, as those are currently not needed
- disable X86_FEATURE_MTRR instead of testing it
- provide a stub for !CONFIG_MTRR (Michael Kelley)
- use cpu_feature_enabled() (Boris Petkov)
- add tests for mtrr_overwrite_state() being allowed (Boris Petkov)
---
arch/x86/include/asm/mtrr.h | 8 ++++++
arch/x86/kernel/cpu/mtrr/generic.c | 43 ++++++++++++++++++++++++++++++
arch/x86/kernel/cpu/mtrr/mtrr.c | 9 +++++++
arch/x86/kernel/setup.c | 2 ++
4 files changed, 62 insertions(+)

diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index f0eeaf6e5f5f..f1cb81330a64 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -31,6 +31,8 @@
*/
# ifdef CONFIG_MTRR
void mtrr_bp_init(void);
+void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
+ mtrr_type def_type);
extern u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform);
extern void mtrr_save_fixed_ranges(void *);
extern void mtrr_save_state(void);
@@ -48,6 +50,12 @@ void mtrr_disable(void);
void mtrr_enable(void);
void mtrr_generic_set_state(void);
# else
+static inline void mtrr_overwrite_state(struct mtrr_var_range *var,
+ unsigned int num_var,
+ mtrr_type def_type)
+{
+}
+
static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
{
/*
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index ee09d359e08f..40c59d522f57 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -8,10 +8,12 @@
#include <linux/init.h>
#include <linux/io.h>
#include <linux/mm.h>
+#include <linux/cc_platform.h>

#include <asm/processor-flags.h>
#include <asm/cacheinfo.h>
#include <asm/cpufeature.h>
+#include <asm/hypervisor.h>
#include <asm/tlbflush.h>
#include <asm/mtrr.h>
#include <asm/msr.h>
@@ -240,6 +242,47 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64
*partial_end,
return mtrr_state.def_type;
}

+/**
+ * mtrr_overwrite_state - set static MTRR state
+ *
+ * Used to set MTRR state via different means (e.g. with data obtained from
+ * a hypervisor).
+ * Is allowed only for special cases when running virtualized. Must be called
+ * from the x86_init.hyper.init_platform() hook. X86_FEATURE_MTRR must be off.
+ */
+void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
+ mtrr_type def_type)
+{
+ unsigned int i;
+
+ if (WARN_ON(mtrr_state_set ||
+ hypervisor_is_type(X86_HYPER_NATIVE) ||
+ !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ||
+ (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&

With current upstream code, this test doesn't allow Hyper-V SNP vTOM VMs
to do the overwrite, as current upstream vTOM code doesn't participate in the
cc_platform_has() mechanism. That's being reworked in a separate patch
set. Can you add this test to cover the SNP vTOM case?

!hv_is_isolation_supported() &&

This is the same test used in __set_memory_enc_dec(), for example. You'll
have to add #include <asm/mshyperv.h>. There's already a stub that returns
'false' so that everything works when building with CONFIG_HYPERV=n.

Yes, this was an open question. In case nobody objects, I'll do the
modification.

Once my other patch set is accepted, I'll revise this to remove use of
hv_is_isolation_supported() outside of Hyper-V specific code, and use
the newer cc_platform_has() instead.

Thanks,


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature