Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 7 Sep 2020 12:06:47 +0200
From:   Borislav Petkov <bp@suse.de>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        kitsunyan <kitsunyan@airmail.cc>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/msr: do not warn on writes to OC_MAILBOX
Message-ID: <20200907100647.GB10657@zn.tnic>
References: <20200907094843.1949-1-Jason@zx2c4.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200907094843.1949-1-Jason@zx2c4.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+ Srinivas.
+ kitsunyan.

On Mon, Sep 07, 2020 at 11:48:43AM +0200, Jason A. Donenfeld wrote:
> Popular tools, like intel-undervolt, use MSR 0x150 to control the CPU
> voltage offset. In fact, evidently the intel_turbo_max_3 driver in-tree
> also uses this MSR. So, teach the kernel's MSR list about this, so that
> intel-undervolt and other such tools don't spew warnings to dmesg, while
> unifying the constant used throughout the kernel.
> 
> Fixes: a7e1f67ed29f ("x86/msr: Filter MSR writes")
> Cc: Borislav Petkov <bp@suse.de>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  arch/x86/include/asm/msr-index.h         | 2 ++
>  arch/x86/kernel/msr.c                    | 5 ++++-
>  drivers/platform/x86/intel_turbo_max_3.c | 6 +++---
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 2859ee4f39a8..0bcb3604d0e2 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -132,6 +132,8 @@
>  #define MSR_IA32_MCU_OPT_CTRL		0x00000123
>  #define RNGDS_MITG_DIS			BIT(0)
>  
> +#define MSR_IA32_OC_MAILBOX		0x00000150
> +
>  #define MSR_IA32_SYSENTER_CS		0x00000174
>  #define MSR_IA32_SYSENTER_ESP		0x00000175
>  #define MSR_IA32_SYSENTER_EIP		0x00000176
> diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
> index 49dcfb85e773..64f6200681e3 100644
> --- a/arch/x86/kernel/msr.c
> +++ b/arch/x86/kernel/msr.c
> @@ -86,8 +86,11 @@ static int filter_write(u32 reg)
>  	default: break;
>  	}
>  
> -	if (reg == MSR_IA32_ENERGY_PERF_BIAS)
> +	switch (reg) {
> +	case MSR_IA32_ENERGY_PERF_BIAS:
> +	case MSR_IA32_OC_MAILBOX:
>  		return 0;
> +	}
>  
>  	pr_err_ratelimited("Write to unrecognized MSR 0x%x by %s\n"
>  			   "Please report to x86@kernel.org\n",
> diff --git a/drivers/platform/x86/intel_turbo_max_3.c b/drivers/platform/x86/intel_turbo_max_3.c
> index 892140b62898..991cdbc3295b 100644
> --- a/drivers/platform/x86/intel_turbo_max_3.c
> +++ b/drivers/platform/x86/intel_turbo_max_3.c
> @@ -17,8 +17,8 @@
>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
> +#include <asm/msr.h>
>  
> -#define MSR_OC_MAILBOX			0x150
>  #define MSR_OC_MAILBOX_CMD_OFFSET	32
>  #define MSR_OC_MAILBOX_RSP_OFFSET	32
>  #define MSR_OC_MAILBOX_BUSY_BIT		63
> @@ -41,14 +41,14 @@ static int get_oc_core_priority(unsigned int cpu)
>  	value = cmd << MSR_OC_MAILBOX_CMD_OFFSET;
>  	/* Set the busy bit to indicate OS is trying to issue command */
>  	value |=  BIT_ULL(MSR_OC_MAILBOX_BUSY_BIT);
> -	ret = wrmsrl_safe(MSR_OC_MAILBOX, value);
> +	ret = wrmsrl_safe(MSR_IA32_OC_MAILBOX, value);
>  	if (ret) {
>  		pr_debug("cpu %d OC mailbox write failed\n", cpu);
>  		return ret;
>  	}
>  
>  	for (i = 0; i < OC_MAILBOX_RETRY_COUNT; ++i) {
> -		ret = rdmsrl_safe(MSR_OC_MAILBOX, &value);
> +		ret = rdmsrl_safe(MSR_IA32_OC_MAILBOX, &value);
>  		if (ret) {
>  			pr_debug("cpu %d OC mailbox read failed\n", cpu);
>  			break;
> -- 

Actually, we added the filtering to catch exactly such misuses and,
lemme check what is the proper word now... /me checks, aha, adding new
MSRs to the "passlist" is the wrong thing to do.

Srinivas, can you pls convert this in-tree driver to use a proper sysfs
interface for that mailbox MSR and also work with the intel-undervolt
author - I hope I have the right person CCed from the git repo on github
- to come up with a proper interface so that we can drop this MSR use
too.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg