[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[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).

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 <ivan.gorinov@intel.com>
---
 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(-)

diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
index aabdc94..05d8312 100644
--- a/arch/x86/cpu/i386/cpu.c
+++ b/arch/x86/cpu/i386/cpu.c
@@ -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");
diff --git a/arch/x86/cpu/intel_common/car.S b/arch/x86/cpu/intel_common/car.S
index 6e0db96..099bf28 100644
--- a/arch/x86/cpu/intel_common/car.S
+++ b/arch/x86/cpu/intel_common/car.S
@@ -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 */
diff --git a/arch/x86/cpu/intel_common/microcode.c b/arch/x86/cpu/intel_common/microcode.c
index 8813258..9321ae1 100644
--- a/arch/x86/cpu/intel_common/microcode.c
+++ b/arch/x86/cpu/intel_common/microcode.c
@@ -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);
 }
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 29bf060..b30b8b6 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -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
diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S
index fbe8aef..157600a 100644
--- a/arch/x86/lib/fsp/fsp_car.S
+++ b/arch/x86/lib/fsp/fsp_car.S
@@ -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 */
-- 
2.7.4

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Andy Shevchenko]

On Wed, 2018-04-04 at 16:07 -0700, Ivan Gorinov wrote:
> 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.

> 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.

>  #include <asm/processor-flags.h>

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Bin Meng]

Hi Ivan,

On Thu, Apr 5, 2018 at 7:07 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> 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.

> 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 <ivan.gorinov@intel.com>
> ---
>  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

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Ivan Gorinov]

On Thu, Apr 05, 2018 at 09:31:34AM -0600, Bin Meng wrote:
> > 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.

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Bin Meng]

Hi Ivan,

On Wed, Apr 18, 2018 at 2:29 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> On Thu, Apr 05, 2018 at 09:31:34AM -0600, Bin Meng wrote:
>> > 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.

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

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Ivan Gorinov]

Hi Bin,

On Wed, Apr 18, 2018 at 06:48:59AM -0600, Bin Meng wrote:
> >> 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.
> 
> 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.

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Bin Meng]

Hi Ivan,

On Thu, Apr 19, 2018 at 8:11 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> Hi Bin,
>
> On Wed, Apr 18, 2018 at 06:48:59AM -0600, Bin Meng wrote:
>> >> 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.
>>
>> 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

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Ivan Gorinov]

Hi Bin,

On Wed, Apr 18, 2018 at 07:05:28PM -0600, Bin Meng wrote:
> >> >
> >> > 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.
> 
> 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.

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

It's usually a proprietary implementation.

[U-Boot] [PATCH] x86: Use microcode update from device tree for all processors

[Bin Meng]

Hi Ivan,

On Sat, Apr 21, 2018 at 1:47 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> Hi Bin,
>
> On Wed, Apr 18, 2018 at 07:05:28PM -0600, Bin Meng wrote:
>> >> >
>> >> > 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.
>>
>> 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.
>
>> 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