Re: [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  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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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  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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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  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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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  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 
> ---
>  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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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 
>  #include 
>  #include 
> +#include 

Keep it in order.

>  #include 

-- 
Andy Shevchenko 
Intel Finland Oy
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[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 
---
 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 
 #include 
 #include 
+#include 
 #include 
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -586,7 +587,7 @@ int x86_mp_init(void)
mp_params.parallel_microcode_load = 0,
mp_params.flight_plan = _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(_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", >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 & 0x,
@@ -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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot