x86: Use microcode update from device tree for all processors

PATCH New Ivan Gorinov 3 participants 9 messages Apr 4, 2018 – Jun 12, 2018 x86

Share & tools

Ivan Gorinov

author

2018-04-04 23:07 UTC #0 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

The microcode update data block encoded in Device Tree is used by the bootstrap processor (BSP) but not passed to the other CPUs (AP).

If the bootstrap processor successfully performs a microcode update from Device Tree, use the same data block for the other processors.

Signed-off-by Ivan Gorinov <[email protected]>

---
 arch/x86/cpu/i386/cpu.c               |  3 ++-
 arch/x86/cpu/intel_common/car.S       |  2 ++
 arch/x86/cpu/intel_common/microcode.c | 10 +++++++---
 arch/x86/include/asm/microcode.h      |  1 +
 arch/x86/lib/fsp/fsp_car.S            |  2 ++
 5 files changed, 14 insertions(+), 4 deletions(-)

arch/x86/cpu/i386/cpu.c +2 -1

index aabdc94..05d8312 100644
@@ -26,6 +26,7 @@
 #include <asm/mp.h>
 #include <asm/msr.h>
 #include <asm/mtrr.h>
+#include <asm/microcode.h>
 #include <asm/processor-flags.h>
 
 DECLARE_GLOBAL_DATA_PTR;

@@ -586,7 +587,7 @@ int x86_mp_init(void)
 	mp_params.parallel_microcode_load = 0,
 	mp_params.flight_plan = &mp_steps[0];
 	mp_params.num_records = ARRAY_SIZE(mp_steps);
-	mp_params.microcode_pointer = 0;
+	mp_params.microcode_pointer = (void *)ucode_base;
 
 	if (mp_init(&mp_params)) {
 		printf("Warning: MP init failure\n");

arch/x86/cpu/intel_common/car.S +2 -0

index 6e0db96..099bf28 100644
@@ -240,4 +240,6 @@ _dt_ucode_base_size:
 .globl ucode_base
 ucode_base:	/* Declared in microcode.h */
 	.long	0			/* microcode base */
+.globl ucode_size
+ucode_size:	/* Declared in microcode.h */
 	.long	0			/* microcode size */

arch/x86/cpu/intel_common/microcode.c +7 -3

index 8813258..9321ae1 100644
@@ -44,8 +44,6 @@ static int microcode_decode_node(const void *blob, int node,
 	update->data = fdt_getprop(blob, node, "data", &update->size);
 	if (!update->data)
 		return -ENOENT;
-	update->data += UCODE_HEADER_LEN;
-	update->size -= UCODE_HEADER_LEN;
 
 	update->header_version = fdtdec_get_int(blob, node,
 						"intel,header-version", 0);

@@ -125,6 +123,7 @@ static void microcode_read_cpu(struct microcode_update *cpu)
 int microcode_update_intel(void)
 {
 	struct microcode_update cpu, update;
+	ulong address;
 	const void *blob = gd->fdt_blob;
 	int skipped;
 	int count;

@@ -168,7 +167,8 @@ int microcode_update_intel(void)
 			skipped++;
 			continue;
 		}
-		wrmsr(MSR_IA32_UCODE_WRITE, (ulong)update.data, 0);
+		address = (ulong)update.data + UCODE_HEADER_LEN;
+		wrmsr(MSR_IA32_UCODE_WRITE, address, 0);
 		rev = microcode_read_rev();
 		debug("microcode: updated to revision 0x%x date=%04x-%02x-%02x\n",
 		      rev, update.date_code & 0xffff,

@@ -179,5 +179,9 @@ int microcode_update_intel(void)
 			return -EFAULT;
 		}
 		count++;
+		if (!ucode_base) {
+			ucode_base = (ulong)update.data;
+			ucode_size = update.size;
+		}
 	} while (1);
 }

arch/x86/include/asm/microcode.h +1 -0

index 29bf060..b30b8b6 100644
@@ -11,6 +11,7 @@
 
 /* This is a declaration for ucode_base in start.S */
 extern u32 ucode_base;
+extern u32 ucode_size;
 
 /**
  * microcode_update_intel() - Apply microcode updates

arch/x86/lib/fsp/fsp_car.S +2 -0

index fbe8aef..157600a 100644
@@ -105,6 +105,8 @@ _dt_ucode_base_size:
 .globl ucode_base
 ucode_base:	/* Declared in micrcode.h */
 	.long	0			/* microcode base */
+.globl ucode_size
+ucode_size:	/* Declared in micrcode.h */
 	.long	0			/* microcode size */
 	.long	CONFIG_SYS_MONITOR_BASE	/* code region base */
 	.long	CONFIG_SYS_MONITOR_LEN	/* code region size */

Signature · 1 line

2.7.4

Andy Shevchenko

2018-04-05 11:07 UTC #1 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

Quoted 2 lines · Ivan Gorinov

The microcode update data block encoded in Device Tree is used by the bootstrap processor (BSP) but not passed to the other CPUs (AP).

BSP abbreviation is confusing. AP is even more looking to preceding words. I don't see either where they are used.

Quoted 8 lines

If the bootstrap processor successfully performs a microcode update from Device Tree, use the same data block for the other processors.
#include <asm/mp.h>
#include <asm/msr.h>
#include <asm/mtrr.h>
+#include <asm/microcode.h>

Keep it in order.

Quoted 1 line

#include <asm/processor-flags.h>

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Bin Meng

2018-04-05 15:31 UTC #2 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

In reply to Ivan Gorinov

Hi Ivan,

Quoted 3 lines · Ivan Gorinov

The microcode update data block encoded in Device Tree is used by the bootstrap processor (BSP) but not passed to the other CPUs (AP).

I don't understand what the bug is here. The AP microcode update is done in sipi_vector.S.

Quoted 12 lines

If the bootstrap processor successfully performs a microcode update from Device Tree, use the same data block for the other processors.
Signed-off-by Ivan Gorinov <[email protected]>
---
 arch/x86/cpu/i386/cpu.c               |  3 ++-
 arch/x86/cpu/intel_common/car.S       |  2 ++
 arch/x86/cpu/intel_common/microcode.c | 10 +++++++---
 arch/x86/include/asm/microcode.h      |  1 +
 arch/x86/lib/fsp/fsp_car.S            |  2 ++
 5 files changed, 14 insertions(+), 4 deletions(-)

Regards,
Bin

Ivan Gorinov

author

2018-04-17 18:29 UTC #3 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

Quoted 5 lines · Bin Meng

Quoted 2 lines
The microcode update data block encoded in Device Tree is used by the bootstrap processor (BSP) but not passed to the other CPUs (AP).
I don't understand what the bug is here. The AP microcode update is done in sipi_vector.S.

I have found how it works. When a ROM image is built, the binman tool looks for symbol '_dt_ucode_base_size' and updates position and size of the microcode update data in the ucode_base and ucode_size variables. The ucode_base pointer is used to update the bootstrap CPU very early, and the other CPUs later in the multiprocessing code.

On x86, binman is called from Makefile only if a ROM image is created:

    u-boot.rom: u-boot-x86-16bit.bin u-boot.bin \
    ...
        $(call if_changed,binman)

If there is no ROM image, ucode_base and ucode_size are not initialized and the microcode update data from DTB applied by microcode_update_intel() to the bootstrap CPU is not used by the multiprocessing code.

Bin Meng

2018-04-18 12:48 UTC #4 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

Hi Ivan,

Quoted 15 lines · Ivan Gorinov

Quoted 5 lines · Bin Meng
Quoted 2 lines
The microcode update data block encoded in Device Tree is used by the bootstrap processor (BSP) but not passed to the other CPUs (AP).
I don't understand what the bug is here. The AP microcode update is done in sipi_vector.S.
I have found how it works. When a ROM image is built, the binman tool looks for symbol '_dt_ucode_base_size' and updates position and size of the microcode update data in the ucode_base and ucode_size variables. The ucode_base pointer is used to update the bootstrap CPU very early, and the other CPUs later in the multiprocessing code.
On x86, binman is called from Makefile only if a ROM image is created:

Quoted 7 lines

    u-boot.rom: u-boot-x86-16bit.bin u-boot.bin \
    ...
        $(call if_changed,binman)
If there is no ROM image, ucode_base and ucode_size are not initialized and the microcode update data from DTB applied by microcode_update_intel() to the bootstrap CPU is not used by the multiprocessing code.

Correct. If it's not a ROM image, which means U-Boot is probably not the 1st stage bootloader, which means updating microcode is not necessary. So is there any issue with current implementation?

Regards,
Bin

Ivan Gorinov

author

2018-04-19 00:11 UTC #5 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

Hi Bin,

Quoted 15 lines · Bin Meng

Quoted 15 lines
Quoted 2 lines
I don't understand what the bug is here. The AP microcode update is done in sipi_vector.S.
I have found how it works. When a ROM image is built, the binman tool looks for symbol '_dt_ucode_base_size' and updates position and size of the microcode update data in the ucode_base and ucode_size variables. The ucode_base pointer is used to update the bootstrap CPU very early, and the other CPUs later in the multiprocessing code.
On x86, binman is called from Makefile only if a ROM image is created:
    u-boot.rom: u-boot-x86-16bit.bin u-boot.bin \
    ...
        $(call if_changed,binman)

Quoted 7 lines

Quoted 3 lines
If there is no ROM image, ucode_base and ucode_size are not initialized and the microcode update data from DTB applied by microcode_update_intel() to the bootstrap CPU is not used by the multiprocessing code.
Correct. If it's not a ROM image, which means U-Boot is probably not the 1st stage bootloader, which means updating microcode is not necessary. So is there any issue with current implementation?

If the 1st stage bootloader is running from the on-chip SRAM, there may be not enough space to include the microcode update data. In this case, U-Boot is a secondary boot loader but still has to update the microcode.

Bin Meng

2018-04-19 01:05 UTC #6 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

Hi Ivan,

Quoted 22 lines · Ivan Gorinov

Hi Bin,
Quoted 19 lines · Bin Meng
Quoted 18 lines
Quoted 2 lines
I don't understand what the bug is here. The AP microcode update is done in sipi_vector.S.
I have found how it works. When a ROM image is built, the binman tool looks for symbol '_dt_ucode_base_size' and updates position and size of the microcode update data in the ucode_base and ucode_size variables. The ucode_base pointer is used to update the bootstrap CPU very early, and the other CPUs later in the multiprocessing code.
On x86, binman is called from Makefile only if a ROM image is created:
    u-boot.rom: u-boot-x86-16bit.bin u-boot.bin \
    ...
        $(call if_changed,binman)
If there is no ROM image, ucode_base and ucode_size are not initialized and the microcode update data from DTB applied by microcode_update_intel() to the bootstrap CPU is not used by the multiprocessing code.

Quoted 7 lines

Quoted 3 lines
Correct. If it's not a ROM image, which means U-Boot is probably not the 1st stage bootloader, which means updating microcode is not necessary. So is there any issue with current implementation?
If the 1st stage bootloader is running from the on-chip SRAM, there may be not enough space to include the microcode update data. In this case, U-Boot is a secondary boot loader but still has to update the microcode.

Thanks for the information. Correct, if that's the case, then we should tune our codes to support that.

But I guess the "1st stage" bootloader is loaded by some on-chip BOOTROM to the internal SRAM?

Is the "1st stage" bootloader running from SRAM the U-Boot SPL? Or some proprietary implementation?

Regards,
Bin

Ivan Gorinov

author

2018-04-20 17:47 UTC #7 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

Hi Bin,

Quoted 13 lines · Bin Meng

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
If there is no ROM image, ucode_base and ucode_size are not initialized and the microcode update data from DTB applied by microcode_update_intel() to the bootstrap CPU is not used by the multiprocessing code.
Quoted 7 lines
Quoted 3 lines
Correct. If it's not a ROM image, which means U-Boot is probably not the 1st stage bootloader, which means updating microcode is not necessary. So is there any issue with current implementation?
If the 1st stage bootloader is running from the on-chip SRAM, there may be not enough space to include the microcode update data. In this case, U-Boot is a secondary boot loader but still has to update the microcode.

Quoted 5 lines

Thanks for the information. Correct, if that's the case, then we should tune our codes to support that.
But I guess the "1st stage" bootloader is loaded by some on-chip BOOTROM to the internal SRAM?

Correct.

Quoted 2 lines

Is the "1st stage" bootloader running from SRAM the U-Boot SPL? Or some proprietary implementation?

It's usually a proprietary implementation.

Bin Meng

2018-06-12 02:05 UTC #8 lore raw

PATCH 1/1 x86: Use microcode update from device tree for all processors

Hi Ivan,

Quoted 22 lines · Ivan Gorinov

Hi Bin,
Quoted 13 lines · Bin Meng
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
If there is no ROM image, ucode_base and ucode_size are not initialized and the microcode update data from DTB applied by microcode_update_intel() to the bootstrap CPU is not used by the multiprocessing code.
Quoted 7 lines
Quoted 3 lines
Correct. If it's not a ROM image, which means U-Boot is probably not the 1st stage bootloader, which means updating microcode is not necessary. So is there any issue with current implementation?
If the 1st stage bootloader is running from the on-chip SRAM, there may be not enough space to include the microcode update data. In this case, U-Boot is a secondary boot loader but still has to update the microcode.
Quoted 5 lines
Thanks for the information. Correct, if that's the case, then we should tune our codes to support that.
But I guess the "1st stage" bootloader is loaded by some on-chip BOOTROM to the internal SRAM?

Quoted 6 lines

Correct.
Quoted 2 lines
Is the "1st stage" bootloader running from SRAM the U-Boot SPL? Or some proprietary implementation?
It's usually a proprietary implementation.

Do you still see any problem with current U-Boot implementation on microcode update? If yes, can you please respin, and resend the patch, and describe what problem you are seeing? Otherwise, we can close this thread.

Regards,
Bin