Messages in this thread Patch in this message | | | Date | Mon, 20 Mar 2023 20:05:43 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs |
| |
On Mon, Mar 06, 2023 at 05:34:16PM +0100, Juergen Gross wrote: > +/** > + * 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) ||
Why that check?
> + !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) || > + (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && > + !hv_is_isolation_supported() && > + !cpu_feature_enabled(X86_FEATURE_XENPV) && > + !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))))
This is unparseable. Please split it into separate checks:
if (WARN_ON(mtrr_state_set)) return;
if (WARN_ON(!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))) return;
...
and add comments above each one why we're testing this.
> + 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) {
I'm guessing the proper detection of that weird state should be:
/* * Check for the software overwrite of MTRR state, only for generic case. * See mtrr_overwrite_state(). */ if (!cpu_feature_enabled(X86_FEATURE_MTRR) && 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; > + } > +
-- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
| |