+ 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 <b...@suse.de>
> Signed-off-by: Jason A. Donenfeld <ja...@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 x...@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
