Message-ID: <768aa720-1db1-81ca-4d0d-adf31f4d134b@amd.com>
Date:   Thu, 15 Aug 2019 13:47:24 +0000
From:   "Lendacky, Thomas" <Thomas.Lendacky@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, Chen Yu <yu.c.chen@...el.com>,
        Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h

On 8/15/19 2:21 AM, Borislav Petkov wrote:
> On Wed, Aug 14, 2019 at 09:17:41PM +0000, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@....com>
>>
>> There have been reports of RDRAND issues after resuming from suspend on
>> some AMD family 15h and family 16h systems. This issue stems from BIOS
>> not performing the proper steps during resume to ensure RDRAND continues
>> to function properly.
> 
> If this happens only during suspend/resume, this probably should
> be done only on configurations which have CONFIG_SUSPEND and/or
> CONFIG_HIBERNATION enabled. I'm assuming BIOS does init it properly

Sure, that makes sense. I'll tie it to CONFIG_PM_SLEEP since that is what
arch/x86/power/cpu.c is dependent on.

> at least during boot - I mean, they should've passed some sort of a
> certification.
> 
> OTOH, if the breakage happens on resume, they clearly didn't test the
> BIOS suspend/resume. I mean, I'm not at all surprised - it is f*cking
> BIOS. News at 11.
> 
>> RDRAND support is indicated by CPUID Fn00000001_ECX[30]. This bit can be
>> reset by clearing MSR C001_1004[62]. Any software that checks for RDRAND
>> support using CPUID, including the kernel,  will believe that RDRAND is
>> not supported.
>>
>> Update the CPU initialization to clear the RDRAND CPUID bit for any family
>> 15h and 16h processor that supports RDRAND. If it is known that the family
>> 15h or family 16h system does not have an RDRAND resume issue or that the
>> system will not be placed in suspend, the "rdrand_force" kernel parameter
>> can be used to stop the clearing of the RDRAND CPUID bit.
>>
>> Additionally, update the suspend and resume path to save and restore the
>> MSR C001_1004 value to ensure that the RDRAND CPUID setting remains in
>> place after resuming from suspend.
>>
>> Note, that clearing the RDRAND CPUID bit does not prevent a processor
>> that normally supports the RDRAND instruction from executing the RDRAND
>> instruction. So any code that determined the support based on family and
>> model won't #UD.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>> ---
>>  .../admin-guide/kernel-parameters.txt         |  8 ++
>>  arch/x86/include/asm/msr-index.h              |  1 +
>>  arch/x86/kernel/cpu/amd.c                     | 42 ++++++++++
>>  arch/x86/power/cpu.c                          | 83 ++++++++++++++++---
>>  4 files changed, 121 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 47d981a86e2f..f47eb33958c1 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4090,6 +4090,14 @@
>>  			Run specified binary instead of /init from the ramdisk,
>>  			used for early userspace startup. See initrd.
>>  
>> +	rdrand_force	[X86]
>> +			On certain AMD processors, the advertisement of the
>> +			RDRAND instruction has been disabled by the kernel
>> +			because of buggy BIOS support, specifically around the
>> +			suspend/resume path. This option allows for overriding
>> +			that decision if it is known that the BIOS support for
>> +			RDRAND is not buggy on the system.
>> +
>>  	rdt=		[HW,X86,RDT]
>>  			Turn on/off individual RDT features. List is:
>>  			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 6b4fc2788078..29ae2b66b9e9 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -381,6 +381,7 @@
>>  #define MSR_AMD64_PATCH_LEVEL		0x0000008b
>>  #define MSR_AMD64_TSC_RATIO		0xc0000104
>>  #define MSR_AMD64_NB_CFG		0xc001001f
>> +#define MSR_AMD64_CPUID_FN_00000001	0xc0011004
> 
> I know the PPR has all the 0s but let's write it
> 
> MSR_AMD64_CPUID_FN_1
> 
> so that it is readable in the kernel.

Ok, will do.

> 
>>  #define MSR_AMD64_PATCH_LOADER		0xc0010020
>>  #define MSR_AMD64_OSVW_ID_LENGTH	0xc0010140
>>  #define MSR_AMD64_OSVW_STATUS		0xc0010141
>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index 3afe07d602dd..86ff1464302b 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -804,6 +804,40 @@ static void init_amd_ln(struct cpuinfo_x86 *c)
>>  	msr_set_bit(MSR_AMD64_DE_CFG, 31);
>>  }
>>  
>> +static bool rdrand_force;
>> +
>> +static int __init rdrand_force_cmdline(char *str)
>> +{
>> +	rdrand_force = true;
>> +
>> +	return 0;
>> +}
>> +early_param("rdrand_force", rdrand_force_cmdline);
> 
> Let's make this a more generic param:
> 
> 	rdrand=force[, ...]
> 
> in case we wanna add some more opts here later.

Sure, I can do that. Do we want to tie this into the nordrand option and
add rdrand=off or keep that separate?

> 
>> +
>> +static void init_hide_rdrand(struct cpuinfo_x86 *c)
> 
> clear_rdrand_cpuid_bit()
> 
> is what this function does.

Ok.

> 
>> +{
>> +	/*
>> +	 * The nordrand option can clear X86_FEATURE_RDRAND, so check for
>> +	 * RDRAND support using the CPUID function directly.
>> +	 */
>> +	if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
>> +		return;
>> +
>> +	msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
>> +	clear_cpu_cap(c, X86_FEATURE_RDRAND);
>> +	pr_info_once("hiding RDRAND via CPUID\n");
> 
> No need for that I guess - that's visible in /proc/cpuinfo.

I think this is a clearer indication that the action has taken place.

> 
>> +}
>> +
>> +static void init_amd_jg(struct cpuinfo_x86 *c)
>> +{
>> +	/*
>> +	 * Some BIOS implementations do not restore proper RDRAND support
>> +	 * across suspend and resume. Check on whether to hide the RDRAND
>> +	 * instruction support via CPUID.
>> +	 */
>> +	init_hide_rdrand(c);
>> +}
>> +
>>  static void init_amd_bd(struct cpuinfo_x86 *c)
>>  {
>>  	u64 value;
>> @@ -818,6 +852,13 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
>>  			wrmsrl_safe(MSR_F15H_IC_CFG, value);
>>  		}
>>  	}
>> +
>> +	/*
>> +	 * Some BIOS implementations do not restore proper RDRAND support
>> +	 * across suspend and resume. Check on whether to hide the RDRAND
>> +	 * instruction support via CPUID.
>> +	 */
>> +	init_hide_rdrand(c);
>>  }
>>  
>>  static void init_amd_zn(struct cpuinfo_x86 *c)
>> @@ -860,6 +901,7 @@ static void init_amd(struct cpuinfo_x86 *c)
>>  	case 0x10: init_amd_gh(c); break;
>>  	case 0x12: init_amd_ln(c); break;
>>  	case 0x15: init_amd_bd(c); break;
>> +	case 0x16: init_amd_jg(c); break;
>>  	case 0x17: init_amd_zn(c); break;
>>  	}
>> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
>> index 1c58d8982728..146c4fd90c3d 100644
>> --- a/arch/x86/power/cpu.c
>> +++ b/arch/x86/power/cpu.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/smp.h>
>>  #include <linux/perf_event.h>
>>  #include <linux/tboot.h>
>> +#include <linux/dmi.h>
>>  
>>  #include <asm/pgtable.h>
>>  #include <asm/proto.h>
>> @@ -23,7 +24,7 @@
>>  #include <asm/debugreg.h>
>>  #include <asm/cpu.h>
>>  #include <asm/mmu_context.h>
>> -#include <linux/dmi.h>
>> +#include <asm/cpu_device_id.h>
>>  
>>  #ifdef CONFIG_X86_32
>>  __visible unsigned long saved_context_ebx;
>> @@ -393,15 +394,14 @@ static int __init bsp_pm_check_init(void)
>>  
>>  core_initcall(bsp_pm_check_init);
>>  
>> -static int msr_init_context(const u32 *msr_id, const int total_num)
>> +static int msr_build_context(const u32 *msr_id, const int num)
>>  {
>> -	int i = 0;
>> +	struct saved_msrs *saved_msrs = &saved_context.saved_msrs;
>>  	struct saved_msr *msr_array;
>> +	int total_num;
>> +	int i, j;
>>  
>> -	if (saved_context.saved_msrs.array || saved_context.saved_msrs.num > 0) {
>> -		pr_err("x86/pm: MSR quirk already applied, please check your DMI match table.\n");
>> -		return -EINVAL;
>> -	}
>> +	total_num = saved_msrs->num + num;
>>  
>>  	msr_array = kmalloc_array(total_num, sizeof(struct saved_msr), GFP_KERNEL);
>>  	if (!msr_array) {
>> @@ -409,19 +409,27 @@ static int msr_init_context(const u32 *msr_id, const int total_num)
>>  		return -ENOMEM;
>>  	}
>>  
>> -	for (i = 0; i < total_num; i++) {
>> -		msr_array[i].info.msr_no	= msr_id[i];
>> +	if (saved_msrs->array) {
>> +		/* Copy previous MSR save requests */
>> +		memcpy(msr_array, saved_msrs->array,
>> +		       sizeof(struct saved_msr) * saved_msrs->num);
> 
> Why do you need to copy those? Why can't you use the infrastructure like
> msr_initialize_bdw() does?

Not sure what you mean. We can't use the DMI stuff for this. So now, with
the x86 family checks, if anyone adds some DMI stuff or x86 family stuff
in the future that matches both the DMI and x86 family checks, this will
be called more than once and so you need to copy any previous settings and
add the new ones.

> 
>> +		kfree(saved_msrs->array);
>> +	}
>> +
>> +	for (i = saved_msrs->num, j = 0; i < total_num; i++, j++) {
>> +		msr_array[i].info.msr_no	= msr_id[j];
>>  		msr_array[i].valid		= false;
>>  		msr_array[i].info.reg.q		= 0;
>>  	}
>> -	saved_context.saved_msrs.num	= total_num;
>> -	saved_context.saved_msrs.array	= msr_array;
>> +	saved_msrs->num   = total_num;
>> +	saved_msrs->array = msr_array;
>>  
>>  	return 0;
>>  }
>>  
>>  /*
>> - * The following section is a quirk framework for problematic BIOSen:
>> + * The following sections are a quirk framework for problematic BIOSen:
>>   * Sometimes MSRs are modified by the BIOSen after suspended to
>>   * RAM, this might cause unexpected behavior after wakeup.
>>   * Thus we save/restore these specified MSRs across suspend/resume
>> @@ -436,7 +444,7 @@ static int msr_initialize_bdw(const struct dmi_system_id *d)
>>  	u32 bdw_msr_id[] = { MSR_IA32_THERM_CONTROL };
>>  
>>  	pr_info("x86/pm: %s detected, MSR saving is needed during suspending.\n", d->ident);
>> -	return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
>> +	return msr_build_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
>>  }
>>  
>>  static const struct dmi_system_id msr_save_dmi_table[] = {
>> @@ -451,9 +459,58 @@ static const struct dmi_system_id msr_save_dmi_table[] = {
>>  	{}
>>  };
>>  
>> +static int msr_save_cpuid_features(const struct x86_cpu_id *c)
>> +{
>> +	u32 cpuid_msr_id[] = {
>> +		MSR_AMD64_CPUID_FN_00000001,
>> +	};
>> +
>> +	pr_info("x86/pm: family %#hx cpu detected, MSR saving is needed during suspending.\n",
>> +		c->family);
>> +
>> +	return msr_build_context(cpuid_msr_id, ARRAY_SIZE(cpuid_msr_id));
>> +}
>> +
>> +static const struct x86_cpu_id msr_save_cpu_table[] = {
>> +	{
>> +		.vendor = X86_VENDOR_AMD,
>> +		.family = 0x15,
>> +		.model = X86_MODEL_ANY,
>> +		.feature = X86_FEATURE_ANY,
>> +		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
>> +	},
>> +	{
>> +		.vendor = X86_VENDOR_AMD,
>> +		.family = 0x16,
>> +		.model = X86_MODEL_ANY,
>> +		.feature = X86_FEATURE_ANY,
>> +		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
>> +	},
>> +	{}
> 
> I think you can make that table a single entry by setting
> 
> 	.vendor  = X86_VENDOR_AMD,
> 	...
> 	.feature = X86_FEATURE_RDRAND,
> 
> and then checking family in msr_save_cpuid_features().

Except that X86_FEATURE_RDRAND isn't set anymore. I could create a new
software feature that is set when the CPUID bit is cleared if that's
preferred.

Thanks,
Tom

> 
> Thx.
> 

Powered by blists - more mailing lists