[PATCH] x86/resctrl: Fix to restore to original value when re-enabling hardware prefetch register

[PATCH] x86/resctrl: Fix to restore to original value when re-enabling hardware prefetch register

[Kohei Tarumizu]

The current pseudo_lock.c code overwrites the value of the
MSR_MISC_FEATURE_CONTROL to 0 even if the original value is not 0.
Therefore, modify it to save and restore the original values.

Fixes: 018961ae5579 ("x86/intel_rdt: Pseudo-lock region creation/removal core")
Fixes: 443810fe6160 ("x86/intel_rdt: Create debugfs files for pseudo-locking testing")
Fixes: 8a2fc0e1bc0c ("x86/intel_rdt: More precise L2 hit/miss measurements")
Signed-off-by: Kohei Tarumizu <tarumizu.kohei@fujitsu.com>
---
This patch was previously one of the following my patch series:.

https://lore.kernel.org/lkml/20220420030223.689259-7-tarumizu.kohei@fujitsu.com/

However, it doesn't depend on my patch series, so I extracted it as a
separate patch.

Best regards,
Kohei Tarumizu
---
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index db813f819ad6..4d8398986f78 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -420,6 +420,7 @@ static int pseudo_lock_fn(void *_rdtgrp)
 	struct pseudo_lock_region *plr = rdtgrp->plr;
 	u32 rmid_p, closid_p;
 	unsigned long i;
+	u64 saved_msr;
 #ifdef CONFIG_KASAN
 	/*
 	 * The registers used for local register variables are also used
@@ -463,6 +464,7 @@ static int pseudo_lock_fn(void *_rdtgrp)
 	 * the buffer and evict pseudo-locked memory read earlier from the
 	 * cache.
 	 */
+	saved_msr = __rdmsr(MSR_MISC_FEATURE_CONTROL);
 	__wrmsr(MSR_MISC_FEATURE_CONTROL, prefetch_disable_bits, 0x0);
 	closid_p = this_cpu_read(pqr_state.cur_closid);
 	rmid_p = this_cpu_read(pqr_state.cur_rmid);
@@ -514,7 +516,7 @@ static int pseudo_lock_fn(void *_rdtgrp)
 	__wrmsr(IA32_PQR_ASSOC, rmid_p, closid_p);
 
 	/* Re-enable the hardware prefetcher(s) */
-	wrmsr(MSR_MISC_FEATURE_CONTROL, 0x0, 0x0);
+	wrmsrl(MSR_MISC_FEATURE_CONTROL, saved_msr);
 	local_irq_enable();
 
 	plr->thread_done = 1;
@@ -871,6 +873,7 @@ bool rdtgroup_pseudo_locked_in_hierarchy(struct rdt_domain *d)
 static int measure_cycles_lat_fn(void *_plr)
 {
 	struct pseudo_lock_region *plr = _plr;
+	u32 saved_low, saved_high;
 	unsigned long i;
 	u64 start, end;
 	void *mem_r;
@@ -879,6 +882,7 @@ static int measure_cycles_lat_fn(void *_plr)
 	/*
 	 * Disable hardware prefetchers.
 	 */
+	rdmsr(MSR_MISC_FEATURE_CONTROL, saved_low, saved_high);
 	wrmsr(MSR_MISC_FEATURE_CONTROL, prefetch_disable_bits, 0x0);
 	mem_r = READ_ONCE(plr->kmem);
 	/*
@@ -895,7 +899,7 @@ static int measure_cycles_lat_fn(void *_plr)
 		end = rdtsc_ordered();
 		trace_pseudo_lock_mem_latency((u32)(end - start));
 	}
-	wrmsr(MSR_MISC_FEATURE_CONTROL, 0x0, 0x0);
+	wrmsr(MSR_MISC_FEATURE_CONTROL, saved_low, saved_high);
 	local_irq_enable();
 	plr->thread_done = 1;
 	wake_up_interruptible(&plr->lock_thread_wq);
@@ -940,6 +944,7 @@ static int measure_residency_fn(struct perf_event_attr *miss_attr,
 	u64 hits_before = 0, hits_after = 0, miss_before = 0, miss_after = 0;
 	struct perf_event *miss_event, *hit_event;
 	int hit_pmcnum, miss_pmcnum;
+	u32 saved_low, saved_high;
 	unsigned int line_size;
 	unsigned int size;
 	unsigned long i;
@@ -973,6 +978,7 @@ static int measure_residency_fn(struct perf_event_attr *miss_attr,
 	/*
 	 * Disable hardware prefetchers.
 	 */
+	rdmsr(MSR_MISC_FEATURE_CONTROL, saved_low, saved_high);
 	wrmsr(MSR_MISC_FEATURE_CONTROL, prefetch_disable_bits, 0x0);
 
 	/* Initialize rest of local variables */
@@ -1031,7 +1037,7 @@ static int measure_residency_fn(struct perf_event_attr *miss_attr,
 	 */
 	rmb();
 	/* Re-enable hardware prefetchers */
-	wrmsr(MSR_MISC_FEATURE_CONTROL, 0x0, 0x0);
+	wrmsr(MSR_MISC_FEATURE_CONTROL, saved_low, saved_high);
 	local_irq_enable();
 out_hit:
 	perf_event_release_kernel(hit_event);
-- 
2.27.0

Re: [PATCH] x86/resctrl: Fix to restore to original value when re-enabling hardware prefetch register

[Reinette Chatre]

On 5/17/2022 9:55 PM, Kohei Tarumizu wrote:
> The current pseudo_lock.c code overwrites the value of the
> MSR_MISC_FEATURE_CONTROL to 0 even if the original value is not 0.
> Therefore, modify it to save and restore the original values.
> 
> Fixes: 018961ae5579 ("x86/intel_rdt: Pseudo-lock region creation/removal core")
> Fixes: 443810fe6160 ("x86/intel_rdt: Create debugfs files for pseudo-locking testing")
> Fixes: 8a2fc0e1bc0c ("x86/intel_rdt: More precise L2 hit/miss measurements")
> Signed-off-by: Kohei Tarumizu <tarumizu.kohei@fujitsu.com>
> ---

Thank you very much for catching this.

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH] x86/resctrl: Fix to restore to original value when re-enabling hardware prefetch register

[Dave Hansen]

On 5/17/22 21:55, Kohei Tarumizu wrote:
> The current pseudo_lock.c code overwrites the value of the
> MSR_MISC_FEATURE_CONTROL to 0 even if the original value is not 0.
> Therefore, modify it to save and restore the original values.
> 
> Fixes: 018961ae5579 ("x86/intel_rdt: Pseudo-lock region creation/removal core")
> Fixes: 443810fe6160 ("x86/intel_rdt: Create debugfs files for pseudo-locking testing")
> Fixes: 8a2fc0e1bc0c ("x86/intel_rdt: More precise L2 hit/miss measurements")
> Signed-off-by: Kohei Tarumizu <tarumizu.kohei@fujitsu.com>

Those commits are pretty old.  Is there any reason this is not stable@
material?

Re: [PATCH] x86/resctrl: Fix to restore to original value when re-enabling hardware prefetch register

[Reinette Chatre]

Hi Dave,

On 5/24/2022 11:41 AM, Dave Hansen wrote:
> On 5/17/22 21:55, Kohei Tarumizu wrote:
>> The current pseudo_lock.c code overwrites the value of the
>> MSR_MISC_FEATURE_CONTROL to 0 even if the original value is not 0.
>> Therefore, modify it to save and restore the original values.
>>
>> Fixes: 018961ae5579 ("x86/intel_rdt: Pseudo-lock region creation/removal core")
>> Fixes: 443810fe6160 ("x86/intel_rdt: Create debugfs files for pseudo-locking testing")
>> Fixes: 8a2fc0e1bc0c ("x86/intel_rdt: More precise L2 hit/miss measurements")
>> Signed-off-by: Kohei Tarumizu <tarumizu.kohei@fujitsu.com>
> 
> Those commits are pretty old.  Is there any reason this is not stable@
> material?

While those commits are old the backport will not be trivial since some
lines affected did change since these original patches. I did not think it would
be worth the effort considering (1) the niche usages of this code, and (2) while these
patches can be found since v4.19 this code remains the only place in the kernel
that modifies this MSR (until the rest of Kohei's series lands). Apart from this
there is no particular reason why it cannot go to stable, we can surely make the
adjustments to make it palatable for v4.19.

Reinette