Subject: Re: [PATCH v2] x86/microcode/intel: Extend BDW late-loading with LLC
 size check
To: Borislav Petkov <bp@alien8.de>, tony.luck@intel.com
References: <1516021917-48335-1-git-send-email-zhang.jia@linux.alibaba.com>
 <20180115184616.r6pypjegywyd7ncm@pd.tnic>
Cc: hmh@hmh.eng.br, mingo@redhat.com, hpa@zytor.com,
        tglx@linutronix.de, x86@kernel.org, linux-kernel@vger.kernel.org
From: Jia Zhang <zhang.jia@linux.alibaba.com>
Message-ID: <2ddaecd3-c121-cb37-219e-0e7b1d17c22e@linux.alibaba.com>
Date: Tue, 16 Jan 2018 09:14:44 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20180115184616.r6pypjegywyd7ncm@pd.tnic>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org


在 2018/1/16 上午2:46, Borislav Petkov 写道:
> On Mon, Jan 15, 2018 at 09:11:57PM +0800, Jia Zhang wrote:
>> The commit b94b73733171
>> ("x86/microcode/intel: Extend BDW late-loading with a revision check")
>> reduces the impact of erratum BDF90 for Broadwell process model.
>> Actually, the impact can be reduced further through adding the checks
>> for the size of LLC per core.
>>
>> For more details, see erratum BDF90 in document #334165 (Intel Xeon
>> Processor E7-8800/4800 v4 Product Family Specification Update) from
>> September 2017.
>>
>> Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
>> ---
>>  arch/x86/kernel/cpu/microcode/intel.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
>> index d9e460f..9143cf2 100644
>> --- a/arch/x86/kernel/cpu/microcode/intel.c
>> +++ b/arch/x86/kernel/cpu/microcode/intel.c
>> @@ -906,18 +906,29 @@ static int get_ucode_fw(void *to, const void *from, size_t n)
>>  	return 0;
>>  }
>>  
>> +static int llc_size_per_core(struct cpuinfo_x86 *c)
>> +{
>> +	u64 llc_size = c->x86_cache_size * 1024;
>> +
>> +	do_div(llc_size, c->x86_max_cores);
> 
> This is done per-CPU - I don't want it to do the same division for each
> core. Do it once at driver init only for that model and cache it.

How about this?

--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -908,10 +908,13 @@ static int get_ucode_fw(void *to, const void
*from, size_t n)

 static int llc_size_per_core(struct cpuinfo_x86 *c)
 {
-       u64 llc_size = c->x86_cache_size * 1024;
-
-       do_div(llc_size, c->x86_max_cores);
+       static u64 llc_size;

+       if (unlikely(!llc_size)) {
+               llc_size = c->x86_cache_size * 1024;
+               do_div(llc_size, c->x86_max_cores);
+       }
+
        return (int)llc_size;
 }


or driver init style?

@@ -996,5 +999,7 @@ struct microcode_ops * __init init_intel_microcode(void)
                return NULL;
        }

+       llc_size_per_core = calc_llc_size_per_core(c);
+
        return &microcode_intel_ops;
 }

or more generic?

@@ -984,6 +987,7 @@ static int get_ucode_user(void *to, const void
*from, size_t n)
        .request_microcode_fw             = request_microcode_fw,
        .collect_cpu_info                 = collect_cpu_info,
        .apply_microcode                  = apply_microcode_intel,
+       .is_blacklisted                   = is_blacklisted,
 };

> 
>> +
>> +	return (int)llc_size;
>> +}
>> +
>>  static bool is_blacklisted(unsigned int cpu)
>>  {
>>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>>  
>>  	/*
>>  	 * Late loading on model 79 with microcode revision less than 0x0b000021
>> -	 * may result in a system hang. This behavior is documented in item
>> -	 * BDF90, #334165 (Intel Xeon Processor E7-8800/4800 v4 Product Family).
>> +	 * and LLC size per core bigger than 2.5MB may result in a system hang.
>> +	 * This behavior is documented in item BDF90, #334165 (Intel Xeon
>> +	 * Processor E7-8800/4800 v4 Product Family).
>>  	 */
>>  	if (c->x86 == 6 &&
>>  	    c->x86_model == INTEL_FAM6_BROADWELL_X &&
>>  	    c->x86_mask == 0x01 &&
>> +	    llc_size_per_core(c) > 2621440 &&
> 
> I'm not taking this: this looks like a bunch of voodoo magic numbers.
> Please get someone from Intel to explain first.

Tony, could you clarify this?

Thanks,
Jia

>