[PATCH 0/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[PATCH 0/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Jia Zhang]

The fix further reduces the impact for the BDW model which has to launch
a machine reset in order to run microcode update in BIOS. This point is
important for some vendors without the concern about machine reboot in
order to fix up Spectre v2.

Jia Zhang (2):
      x86/intel: introduce platform_id
      x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

 arch/x86/include/asm/processor.h      |  1 +
 arch/x86/kernel/cpu/intel.c           |  7 +++++++
 arch/x86/kernel/cpu/microcode/intel.c | 16 ++++++++++++++--
 3 files changed, 22 insertions(+), 2 deletions(-)

[PATCH 1/2] x86/intel: introduce platform_id

[Jia Zhang]

Platform ID retrieved from MSR_IA32_PLATFORM_ID may be used as
a filtration condition in some cases.

Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
---
 arch/x86/include/asm/processor.h | 1 +
 arch/x86/kernel/cpu/intel.c      | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index d3a67fb..c0b4d47 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -133,6 +133,7 @@ struct cpuinfo_x86 {
 	u16			cpu_index;
 	u32			microcode;
 	unsigned		initialized : 1;
+	u8			platform_id;
 } __randomize_layout;
 
 struct cpuid_regs {
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index b1af220..cee0554 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -229,6 +229,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 	}
 
 	check_mpx_erratum(c);
+
+	if ((c->x86 > 6) || (c->x86_model >= 5)) {
+		u32 val[2];
+
+		rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
+		c->platform_id = 1 << ((val[1] >> 18) & 7);
+	}
 }
 
 #ifdef CONFIG_X86_32
-- 
1.8.3.1

Re: [PATCH 1/2] x86/intel: introduce platform_id

[Borislav Petkov]

On Mon, Jan 15, 2018 at 01:43:22PM +0800, Jia Zhang wrote:
> Platform ID retrieved from MSR_IA32_PLATFORM_ID may be used as
> a filtration condition in some cases.
> 
> Signed-off-by: Jia Zhang <zhang.jia@linux.alibaba.com>
> ---
>  arch/x86/include/asm/processor.h | 1 +
>  arch/x86/kernel/cpu/intel.c      | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index d3a67fb..c0b4d47 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -133,6 +133,7 @@ struct cpuinfo_x86 {
>  	u16			cpu_index;
>  	u32			microcode;
>  	unsigned		initialized : 1;
> +	u8			platform_id;

Platform ID in cpuinfo?

Nope, I don't think so.

>  } __randomize_layout;
>  
>  struct cpuid_regs {
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index b1af220..cee0554 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -229,6 +229,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	}
>  
>  	check_mpx_erratum(c);
> +
> +	if ((c->x86 > 6) || (c->x86_model >= 5)) {
> +		u32 val[2];
> +
> +		rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
> +		c->platform_id = 1 << ((val[1] >> 18) & 7);
> +	}

collect_cpu_info() in arch/x86/kernel/cpu/microcode/intel.c already does
that so you already have that info without duplicating it.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH 2/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Jia Zhang]

The commit b94b73733171
("x86/microcode/intel: Extend BDW late-loading with a revision check")
reduces the impact of erratum BDF90 for Broadwell process model. Now,
the impact can be reduced further through adding the checks for
platform id and the size of LLC per core.

This fix is useful to reduce the impact for microcode update launched
by BIOS with a must machine reset.

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 | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index d9e460f..8bf09eb 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -906,18 +906,30 @@ 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);
+
+	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 &&
+	    c->platform_id == 0xef &&
 	    c->microcode < 0x0b000021) {
 		pr_err_once("Erratum BDF90: late loading with revision < 0x0b000021 (0x%x) disabled.\n", c->microcode);
 		pr_err_once("Please consider either early loading through initrd/built-in or a potential BIOS update.\n");
-- 
1.8.3.1

Re: [PATCH 2/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Borislav Petkov]

On Mon, Jan 15, 2018 at 01:43:23PM +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. Now,
> the impact can be reduced further through adding the checks for
> platform id and the size of LLC per core.
> 
> This fix is useful to reduce the impact for microcode update launched
> by BIOS with a must machine reset.
> 
> For more details, see erratum BDF90 in document #334165 (Intel Xeon
> Processor E7-8800/4800 v4 Product Family Specification Update) from
> September 2017.

You must be looking at something NDA or so but this:

https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/xeon-e7-v4-spec-update.pdf

doesn't have none of that

> +         llc_size_per_core(c) > 2621440 &&
> +         c->platform_id == 0xef &&

fun.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Jia Zhang]

Yes I'm wrong with platform id so drop it.

Jia

在 2018/1/15 下午6:10, Borislav Petkov 写道:
> On Mon, Jan 15, 2018 at 01:43:23PM +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. Now,
>> the impact can be reduced further through adding the checks for
>> platform id and the size of LLC per core.
>>
>> This fix is useful to reduce the impact for microcode update launched
>> by BIOS with a must machine reset.
>>
>> For more details, see erratum BDF90 in document #334165 (Intel Xeon
>> Processor E7-8800/4800 v4 Product Family Specification Update) from
>> September 2017.
> 
> You must be looking at something NDA or so but this:
> 
> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/xeon-e7-v4-spec-update.pdf
> 
> doesn't have none of that
> 
>> +         llc_size_per_core(c) > 2621440 &&
>> +         c->platform_id == 0xef &&
> 
> fun.
> 

Re: [PATCH 2/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Borislav Petkov]

On Mon, Jan 15, 2018 at 09:06:07PM +0800, Jia Zhang wrote:
> Yes I'm wrong with platform id so drop it.

Also, please do not top-post.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Henrique de Moraes Holschuh]

On Mon, 15 Jan 2018, Jia Zhang wrote:
> For more details, see erratum BDF90 in document #334165 (Intel Xeon
> Processor E7-8800/4800 v4 Product Family Specification Update) from
> September 2017.

For the record, this erratum may well affect some E5v4 as well.
Anything with a LLC/core ratio >= 2.5 is potentially affected as far as
I could tell when I took a serious look at it months ago (based only on
crash reports and public information).

It would be safer to just blacklist by sig == 0x406f1, revision <
0x0b00021, and LLC/core ratio >= 2.5, ignoring platform IDs.

>  	/*
>  	 * 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 &&
> +	    c->platform_id == 0xef &&
>  	    c->microcode < 0x0b000021) {
>  		pr_err_once("Erratum BDF90: late loading with revision < 0x0b000021 (0x%x) disabled.\n", c->microcode);
>  		pr_err_once("Please consider either early loading through initrd/built-in or a potential BIOS update.\n");

The c->platform_id test looks wrong.  The processor will only have a
single bit set, it is the microcode update that has more than a single
bit set.

And do you really want 0xef?  That is everyhing the public available
microcode updates can be applied to in the first place, so even a
corrected test would be useless (it would always match) unless you
actually expect to find never-seen-in-the-wild platform mask 0x10?

-- 
  Henrique Holschuh

Re: [PATCH 2/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Jia Zhang]

在 2018/1/15 下午7:48, Henrique de Moraes Holschuh 写道:
> On Mon, 15 Jan 2018, Jia Zhang wrote:
>> For more details, see erratum BDF90 in document #334165 (Intel Xeon
>> Processor E7-8800/4800 v4 Product Family Specification Update) from
>> September 2017.
> 
> For the record, this erratum may well affect some E5v4 as well.
> Anything with a LLC/core ratio >= 2.5 is potentially affected as far as
> I could tell when I took a serious look at it months ago (based only on
> crash reports and public information).
> 
> It would be safer to just blacklist by sig == 0x406f1, revision <
> 0x0b00021, and LLC/core ratio >= 2.5, ignoring platform IDs.
> 
>>  	/*
>>  	 * 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 &&
>> +	    c->platform_id == 0xef &&
>>  	    c->microcode < 0x0b000021) {
>>  		pr_err_once("Erratum BDF90: late loading with revision < 0x0b000021 (0x%x) disabled.\n", c->microcode);
>>  		pr_err_once("Please consider either early loading through initrd/built-in or a potential BIOS update.\n");
> 
> The c->platform_id test looks wrong.  The processor will only have a
> single bit set, it is the microcode update that has more than a single
> bit set.
> 
> And do you really want 0xef?  That is everyhing the public available
> microcode updates can be applied to in the first place, so even a
> corrected test would be useless (it would always match) unless you
> actually expect to find never-seen-in-the-wild platform mask 0x10?

Yes you are right. That platform id is used in microcode update for
matching compare. I will drop platform id in v2.

Jia

> 

Re: [PATCH 2/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Jia Zhang]

在 2018/1/15 下午7:48, Henrique de Moraes Holschuh 写道:
> On Mon, 15 Jan 2018, Jia Zhang wrote:
>> For more details, see erratum BDF90 in document #334165 (Intel Xeon
>> Processor E7-8800/4800 v4 Product Family Specification Update) from
>> September 2017.
> 
> For the record, this erratum may well affect some E5v4 as well.
> Anything with a LLC/core ratio >= 2.5 is potentially affected as far as
> I could tell when I took a serious look at it months ago (based only on
> crash reports and public information).
> 
> It would be safer to just blacklist by sig == 0x406f1, revision <
> 0x0b00021, and LLC/core ratio >= 2.5, ignoring platform IDs.

By the way, I have another BDW processor with 40MB LLC and 16 cores.
2.5MB (40MB/16) is safe.

Thanks,
Jia

> 
>>  	/*
>>  	 * 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 &&
>> +	    c->platform_id == 0xef &&
>>  	    c->microcode < 0x0b000021) {
>>  		pr_err_once("Erratum BDF90: late loading with revision < 0x0b000021 (0x%x) disabled.\n", c->microcode);
>>  		pr_err_once("Please consider either early loading through initrd/built-in or a potential BIOS update.\n");
> 
> The c->platform_id test looks wrong.  The processor will only have a
> single bit set, it is the microcode update that has more than a single
> bit set.
> 
> And do you really want 0xef?  That is everyhing the public available
> microcode updates can be applied to in the first place, so even a
> corrected test would be useless (it would always match) unless you
> actually expect to find never-seen-in-the-wild platform mask 0x10?
> 

Re: [PATCH 0/2] x86/microcode/intel: Extend BDW late-loading with platform id and LLC check

[Borislav Petkov]

On Mon, Jan 15, 2018 at 01:43:21PM +0800, Jia Zhang wrote:
> The fix further reduces the impact for the BDW model which has to launch
> a machine reset in order to run microcode update in BIOS. This point is
> important for some vendors without the concern about machine reboot in
> order to fix up Spectre v2.

That's a ridiculous excuse - those vendors need to reboot to get these
fixes first. So what is the real reason?

Why can't they simply move to early loading like the rest of the world?

So srsly, how many times are we going to fix this erratum? Are you going
to come up with yet another fix *after* this one?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.