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

[Reinette Chatre]

Hi Kohei,

Thank you very much for catching this issue. This fix is not specific to
or required by the driver you are creating in this series so you could also
extract this patch and submit it separately as a fix to resctrl.

When you do resubmit there are a few style related points that I highlight here,
the fix itself looks good.

For the subject, please use "x86/resctrl:" prefix in the subject. 

On 4/19/2022 8:02 PM, Kohei Tarumizu wrote:
> The current pseudo_lock.c code overwrittes the value of the

overwrittes -> overwrites

> MSR_MISC_FEATURE_CONTROL to 0 even if the original value is not 0.
> Therefore, modify it to save and restore the original values.
> 

This needs a Fixes tag. A few patches are impacted by this fix:
 
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@xxxxxxxxxxx>
> ---
>  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..2d713c20f55f 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;
> @@ -873,12 +875,14 @@ static int measure_cycles_lat_fn(void *_plr)
>  	struct pseudo_lock_region *plr = _plr;
>  	unsigned long i;
>  	u64 start, end;
> +	u32 saved_low, saved_high;
>  	void *mem_r;

Please do follow the current style of using "reverse fir tree order".
More information in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/maintainer-tip.rst#n587

>  
>  	local_irq_disable();
>  	/*
>  	 * 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);
> @@ -945,6 +949,7 @@ static int measure_residency_fn(struct perf_event_attr *miss_attr,
>  	unsigned long i;
>  	void *mem_r;
>  	u64 tmp;
> +	u32 saved_low, saved_high;

Same as above.

>  
>  	miss_event = perf_event_create_kernel_counter(miss_attr, plr->cpu,
>  						      NULL, NULL, NULL);
> @@ -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);

Thank you very much.

Reinette