Messages in this thread | | | From | "Michael Kelley (LINUX)" <> | Subject | RE: [PATCH v3 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs | Date | Sun, 26 Feb 2023 17:12:04 +0000 |
| |
From: Juergen Gross <jgross@suse.com> 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@suse.com> > --- > 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.
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.
Michael
> + !cpu_feature_enabled(X86_FEATURE_XENPV) && > + !cpu_feature_enabled(X86_FEATURE_TDX_GUEST)))) > + return; > + > + /* Disable MTRR in order to disable MTRR modifications. */ > + setup_clear_cpu_cap(X86_FEATURE_MTRR); > + > + if (var) { > + if (num_var > MTRR_MAX_VAR_RANGES) { > + pr_warn("Trying to overwrite MTRR state with %u variable entries\n", > + num_var); > + num_var = MTRR_MAX_VAR_RANGES; > + } > + for (i = 0; i < num_var; i++) > + mtrr_state.var_ranges[i] = var[i]; > + num_var_ranges = num_var; > + } > + > + mtrr_state.def_type = def_type; > + mtrr_state.enabled |= MTRR_STATE_MTRR_ENABLED; > + > + mtrr_state_set = 1; > +} > + > /** > * mtrr_type_lookup - look up memory type in MTRR > * > diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c > index 7596ebeab929..5fe62ee0361b 100644 > --- a/arch/x86/kernel/cpu/mtrr/mtrr.c > +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c > @@ -666,6 +666,15 @@ void __init mtrr_bp_init(void) > const char *why = "(not available)"; > unsigned int phys_addr; > > + if (mtrr_state.enabled) { > + /* Software overwrite of MTRR state, only for generic case. */ > + mtrr_calc_physbits(true); > + init_table(); > + pr_info("MTRRs set to read-only\n"); > + > + return; > + } > + > phys_addr = mtrr_calc_physbits(boot_cpu_has(X86_FEATURE_MTRR)); > > if (boot_cpu_has(X86_FEATURE_MTRR)) { > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 16babff771bd..0cccfeb67c3a 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -1037,6 +1037,8 @@ void __init setup_arch(char **cmdline_p) > /* > * VMware detection requires dmi to be available, so this > * needs to be done after dmi_setup(), for the boot CPU. > + * For some guest types (Xen PV, SEV-SNP, TDX) it is required to be > + * called before cache_bp_init() for setting up MTRR state. > */ > init_hypervisor_platform(); > > -- > 2.35.3
| |