On Sat, Aug 20, 2022 at 11:25:24AM +0200, Juergen Gross wrote:
When booting or resuming the system MTRR state is saved on the boot processor and then this state is loaded into MTRRs of all other cpus.
s/cpu/CPU/g
Pls check all commit messages.
During update of the MTRRs the MTRR mechanism needs to be disabled by writing the related MSR. The old contents of this MSR are saved in a set of static variables and later those static variables are used to restore the MSR.
In case the MSR contents need to be modified on a cpu due to the MSR not having been initialized properly by the BIOS, the related update function is modifying the static variables accordingly.
Unfortunately the MTRR state update is usually running on all cpus at the same time, so using just one set of static variables for all cpus is racy in case the MSR contents differ across cpus.
Fix that by using percpu variables for saving the MSR contents.
Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross jgross@suse.com
I thought adding a "Fixes:" tag for the kernel's initial git commit would maybe be entertaining, but without being really helpful. The percpu variables were preferred over on-stack ones in order to avoid more code churn in followup patches decoupling PAT from MTRR support.
So if that thing has been broken for so long and no one noticed, we could just as well not backport to stable at all...
V2:
- new patch
arch/x86/kernel/cpu/mtrr/generic.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c index 558108296f3c..3d185fcf08ca 100644 --- a/arch/x86/kernel/cpu/mtrr/generic.c +++ b/arch/x86/kernel/cpu/mtrr/generic.c @@ -679,7 +679,8 @@ static bool set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr) return changed; } -static u32 deftype_lo, deftype_hi; +static DEFINE_PER_CPU(u32, deftype_lo); +static DEFINE_PER_CPU(u32, deftype_hi);
My APM says that the high 32 bits of the MTRRdefType MSR are reserved and MBZ. So you can drop the _hi thing and use 0 everywhere. Or rather a dummy = 0 var because of that damn rdmsr() macro.
Or simply use a
u64 deftype;
use the rdmsrl/wrmsrl() variants and split it when passing to umtrr_wrmsr() because that thing wants 2 32s.
/**
- set_mtrr_state - Set the MTRR state for this CPU.
@@ -691,6 +692,7 @@ static unsigned long set_mtrr_state(void) { unsigned long change_mask = 0; unsigned int i;
- u32 *lo = this_cpu_ptr(&deftype_lo);
The tip-tree preferred ordering of variable declarations at the beginning of a function is reverse fir tree order::
struct long_struct_name *descriptive_name; unsigned long foo, bar; unsigned int tmp; int ret;
The above is faster to parse than the reverse ordering::
int ret; unsigned int tmp; unsigned long foo, bar; struct long_struct_name *descriptive_name;
And even more so than random ordering::
unsigned long foo, bar; int ret; struct long_struct_name *descriptive_name; unsigned int tmp;
Please check all your patches.
for (i = 0; i < num_var_ranges; i++) { if (set_mtrr_var_ranges(i, &mtrr_state.var_ranges[i])) @@ -704,10 +706,10 @@ static unsigned long set_mtrr_state(void) * Set_mtrr_restore restores the old value of MTRRdefType, * so to set it we fiddle with the saved value: */
- if ((deftype_lo & 0xff) != mtrr_state.def_type
|| ((deftype_lo & 0xc00) >> 10) != mtrr_state.enabled) {
- if ((*lo & 0xff) != mtrr_state.def_type
|| ((*lo & 0xc00) >> 10) != mtrr_state.enabled) {
deftype_lo = (deftype_lo & ~0xcff) | mtrr_state.def_type |
*lo = (*lo & ~0xcff) | mtrr_state.def_type | (mtrr_state.enabled << 10);@@ -729,6 +731,8 @@ static DEFINE_RAW_SPINLOCK(set_atomicity_lock); static void prepare_set(void) __acquires(set_atomicity_lock) { unsigned long cr0;
- u32 *lo = this_cpu_ptr(&deftype_lo);
- u32 *hi = this_cpu_ptr(&deftype_hi);
You don't need the pointers here - this_cpu_read() should be enough.
/* * Note that this is not ideal @@ -763,10 +767,10 @@ static void prepare_set(void) __acquires(set_atomicity_lock) flush_tlb_local(); /* Save MTRR state */
- rdmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
- rdmsr(MSR_MTRRdefType, *lo, *hi);
/* Disable MTRRs, and set the default type to uncached */
- mtrr_wrmsr(MSR_MTRRdefType, deftype_lo & ~0xcff, deftype_hi);
- mtrr_wrmsr(MSR_MTRRdefType, *lo & ~0xcff, *hi);
/* Again, only flush caches if we have to. */ if (!static_cpu_has(X86_FEATURE_SELFSNOOP)) @@ -775,12 +779,15 @@ static void prepare_set(void) __acquires(set_atomicity_lock) static void post_set(void) __releases(set_atomicity_lock) {
- u32 *lo = this_cpu_ptr(&deftype_lo);
- u32 *hi = this_cpu_ptr(&deftype_hi);
Ditto.
Thx.