Change in coreboot[master]: [WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads
Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: [WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
[WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads
Update microcode using a core specific semaphore. The SDM and most of the BWGs suggest doing this.
Change-Id: I89a01f6ff94e7d35f1feded3ddb0c43caf7c6c5d Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S 2 files changed, 74 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44904/1
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 5807831..4c4de09 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -25,6 +25,7 @@ #include <symbols.h> #include <timer.h> #include <thread.h> +#include <lib.h>
#include <security/intel/stm/SmmStm.h>
@@ -97,6 +98,9 @@ uint32_t msr_count; uint32_t c_handler; atomic_t ap_count; + uint32_t microcode_lock_ht; /* 1 special microcode locking for Intel HT CPUs */ + uint32_t apic_core_id_mask; + uint32_t hyperthreading_lock[CONFIG_MAX_CPUS/2]; } __packed;
/* This also needs to match the assembly code for saved MSR encoding. */ @@ -216,6 +220,31 @@ sp->stack_top = ALIGN_DOWN((uintptr_t)&_estack, CONFIG_STACK_SIZE); /* Adjust the stack top to take into account cpu_info. */ sp->stack_top -= sizeof(struct cpu_info); + sp->microcode_lock_ht = !!(cpuid_edx(1) & CPUID_FEAURE_HTT); + if (sp->microcode_lock_ht) { + struct cpuid_result result; + unsigned int core_ids, apic_ids, threads; + + apic_ids = 1; + if (cpuid_eax(0) >= 1) + apic_ids = (cpuid_ebx(1) >> 16) & 0xff; + if (apic_ids == 0) + apic_ids = 1; + + core_ids = 1; + if (cpuid_eax(0) >= 4) { + result = cpuid_ext(4, 0); + core_ids += (result.eax >> 26) & 0x3f; + } + + threads = (apic_ids / core_ids); + + sp->apic_core_id_mask = 1UL << log2_ceil(core_ids); + sp->apic_core_id_mask <<= log2_ceil(threads); + } else { + sp->apic_core_id_mask = 0; + } + memset(sp->hyperthreading_lock, 0, sizeof(sp->hyperthreading_lock)); }
#define NUM_FIXED_MTRRS 11 diff --git a/src/cpu/x86/sipi_vector.S b/src/cpu/x86/sipi_vector.S index ba1ecb7..abf0549 100644 --- a/src/cpu/x86/sipi_vector.S +++ b/src/cpu/x86/sipi_vector.S @@ -3,6 +3,7 @@ #include <cpu/x86/cr.h> #include <cpu/amd/mtrr.h> #include <cpu/x86/msr.h> +#include <cpu/x86/lapic_def.h> #include <arch/ram_segs.h>
/* The SIPI vector is responsible for initializing the APs in the system. It @@ -33,6 +34,12 @@ .long 0 ap_count: .long 0 +apic_core_id_mask: +.long 0 +hyperthreading_lock: +.space CONFIG_MAX_CPUS/2 +microcode_lock_ht: +.long 0
#define CR0_CLEAR_FLAGS_CACHE_ENABLE (CR0_CD | CR0_NW) #define CR0_SET_FLAGS (CR0_CLEAR_FLAGS_CACHE_ENABLE | CR0_PE) @@ -115,6 +122,35 @@ test %edx, %edx jnz microcode_done
+ /* Determine if Hyper-Threading semaphore should be used */ + cmpl $0, microcode_lock_ht + je load_microcode + + /* Get this CPU's LAPIC ID */ + movl $(LOCAL_APIC_ADDR | LAPIC_ID), %esi + movl (%esi), %ecx + shr $24, %ecx + + movl apic_core_id_mask, %eax + andl %eax, %ecx +1: + bt $0, %eax + jc 1f + shr $1, %eax + shr $1, %ecx + jmp 1b +1: + /* ecx now contains the core ID */ + movl $(hyperthreading_lock), %ebx + shl $2, %ecx + addl %ecx, %ebx + +lock_ht_semaphore: + /* Lock the core specific semaphore */ + lock bts $0, (%ebx) + jc lock_ht_semaphore + jmp load_microcode + /* Determine if parallel microcode loading is allowed. */ cmpl $0xffffffff, microcode_lock je load_microcode @@ -136,6 +172,15 @@ wrmsr popa
+ /* Determine if Hyper-Threading semaphore should be used */ + cmpl $0, microcode_lock_ht + jne 1f + + xor %eax, %eax + mov %eax, (%ebx) + je microcode_done + +1: /* Unconditionally unlock microcode loading. */ cmpl $0xffffffff, microcode_lock je microcode_done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: [WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44904/1/src/cpu/x86/sipi_vector.S File src/cpu/x86/sipi_vector.S:
https://review.coreboot.org/c/coreboot/+/44904/1/src/cpu/x86/sipi_vector.S@4... PS1, Line 41: microcode_lock_ht: : .long 0 This doesn't match the C struct layout (should be placed earlier)
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44904
to look at the new patch set (#2).
Change subject: [WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
[WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads
Update microcode using a core specific semaphore. The SDM and most of the BWGs suggest doing this.
Change-Id: I89a01f6ff94e7d35f1feded3ddb0c43caf7c6c5d Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S 2 files changed, 76 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44904/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44904
to look at the new patch set (#3).
Change subject: [WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
[WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads
Update microcode using a core specific semaphore. The SDM and most of the BWGs suggest doing this.
Change-Id: I89a01f6ff94e7d35f1feded3ddb0c43caf7c6c5d Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S 2 files changed, 76 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44904/3
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44904
to look at the new patch set (#4).
Change subject: [WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
[WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads
Update microcode using a core specific semaphore. The SDM and most of the BWGs suggest doing this.
Change-Id: I89a01f6ff94e7d35f1feded3ddb0c43caf7c6c5d Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S 2 files changed, 80 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44904/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: [WIP] cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44904/4/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/44904/4/src/cpu/x86/mp_init.c@104 PS4, Line 104: uint32_t hyperthreading_lock[CONFIG_MAX_CPUS/2]; /* Initialize hyperthreading lock here */ line over 96 characters
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44904
to look at the new patch set (#5).
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
cpu/x86/mpinit: Serialize microcode updates for HT threads
This change affects Intel CPUs only. As most platforms are doing uCOde update using FIT, they aren't affect by this code either.
Update microcode using a core specific semaphore. The SDM and most of the BWGs suggest doing this.
Change-Id: I89a01f6ff94e7d35f1feded3ddb0c43caf7c6c5d Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector_c_handler.c M src/include/cpu/x86/mp.h 3 files changed, 55 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44904/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44904/5/src/cpu/x86/sipi_vector_c_h... File src/cpu/x86/sipi_vector_c_handler.c:
https://review.coreboot.org/c/coreboot/+/44904/5/src/cpu/x86/sipi_vector_c_h... PS5, Line 40: } else suspect code indent for conditional statements (8, 8)
https://review.coreboot.org/c/coreboot/+/44904/5/src/cpu/x86/sipi_vector_c_h... PS5, Line 56: if (params->microcode_lock_ht) { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/44904/5/src/include/cpu/x86/mp.h File src/include/cpu/x86/mp.h:
https://review.coreboot.org/c/coreboot/+/44904/5/src/include/cpu/x86/mp.h@16... PS5, Line 164: spinlock_t hyperthreading_lock[CONFIG_MAX_CPUS/2]; /* Initialize hyperthreading lock here */ line over 96 characters
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44904/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44904/5//COMMIT_MSG@10 PS5, Line 10: uCOde uCode
https://review.coreboot.org/c/coreboot/+/44904/5//COMMIT_MSG@13 PS5, Line 13: The SDM and most of the BWGs suggest doing this. Is it to improve the code, or also to fix concrete problems?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 5:
Trying to test, if QEMU x86 still works (though it doesn’t use microcode updates, right), it fails to build.
``` CC ramstage/arch/x86/cpu.o In file included from src/arch/x86/cpu.c:15: src/include/smp/spinlock.h:8: error: "DECLARE_SPIN_LOCK" redefined [-Werror] #define DECLARE_SPIN_LOCK(x)
In file included from src/include/cpu/x86/mp.h:6, from src/arch/x86/cpu.c:9: src/arch/x86/include/arch/smp/spinlock.h:20: note: this is the location of the previous definition #define DECLARE_SPIN_LOCK(x) \
In file included from src/arch/x86/cpu.c:15: src/include/smp/spinlock.h:9: error: "spin_is_locked" redefined [-Werror] #define spin_is_locked(lock) 0
In file included from src/include/cpu/x86/mp.h:6, from src/arch/x86/cpu.c:9: src/arch/x86/include/arch/smp/spinlock.h:30: note: this is the location of the previous definition #define spin_is_locked(x) (*(volatile char *)(&(x)->lock) <= 0)
In file included from src/arch/x86/cpu.c:15: src/include/smp/spinlock.h:10: error: "spin_unlock_wait" redefined [-Werror] #define spin_unlock_wait(lock) do {} while (0)
In file included from src/include/cpu/x86/mp.h:6, from src/arch/x86/cpu.c:9: src/arch/x86/include/arch/smp/spinlock.h:31: note: this is the location of the previous definition #define spin_unlock_wait(x) do { barrier(); } while (spin_is_locked(x)) ```
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44904/6/src/cpu/x86/sipi_vector_c_h... File src/cpu/x86/sipi_vector_c_handler.c:
https://review.coreboot.org/c/coreboot/+/44904/6/src/cpu/x86/sipi_vector_c_h... PS6, Line 40: } else suspect code indent for conditional statements (8, 8)
https://review.coreboot.org/c/coreboot/+/44904/6/src/cpu/x86/sipi_vector_c_h... PS6, Line 56: if (params->microcode_lock_ht) { braces {} are not necessary for any arm of this statement
https://review.coreboot.org/c/coreboot/+/44904/6/src/include/cpu/x86/mp.h File src/include/cpu/x86/mp.h:
https://review.coreboot.org/c/coreboot/+/44904/6/src/include/cpu/x86/mp.h@16... PS6, Line 164: spinlock_t hyperthreading_lock[CONFIG_MAX_CPUS/2]; /* Initialize hyperthreading lock here */ line over 96 characters
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44904
to look at the new patch set (#7).
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
cpu/x86/mpinit: Serialize microcode updates for HT threads
This change affects Intel CPUs only. As most platforms are doing uCOde update using FIT, they aren't affect by this code either.
Update microcode using a core specific semaphore. The SDM and most of the BWGs suggest doing this.
Change-Id: I89a01f6ff94e7d35f1feded3ddb0c43caf7c6c5d Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S M src/cpu/x86/sipi_vector_c_handler.c M src/include/cpu/x86/mp.h 4 files changed, 58 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44904/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44904/7/src/include/cpu/x86/mp.h File src/include/cpu/x86/mp.h:
https://review.coreboot.org/c/coreboot/+/44904/7/src/include/cpu/x86/mp.h@16... PS7, Line 164: spinlock_t hyperthreading_lock[CONFIG_MAX_CPUS/2]; /* Initialize hyperthreading lock here */ line over 96 characters
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44904
to look at the new patch set (#8).
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
cpu/x86/mpinit: Serialize microcode updates for HT threads
This change affects Intel CPUs only. As most platforms are doing uCOde update using FIT, they aren't affect by this code either.
Update microcode using a core specific semaphore. The SDM and most of the BWGs suggest doing this.
Change-Id: I89a01f6ff94e7d35f1feded3ddb0c43caf7c6c5d Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S M src/cpu/x86/sipi_vector_c_handler.c M src/include/cpu/x86/mp.h 4 files changed, 58 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/44904/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44904/8/src/include/cpu/x86/mp.h File src/include/cpu/x86/mp.h:
https://review.coreboot.org/c/coreboot/+/44904/8/src/include/cpu/x86/mp.h@16... PS8, Line 164: spinlock_t hyperthreading_lock[CONFIG_MAX_CPUS/2]; /* Initialize hyperthreading lock here */ line over 96 characters
Patrick Rudolph has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44904 )
Change subject: cpu/x86/mpinit: Serialize microcode updates for HT threads ......................................................................
Abandoned
patch series replaced by CB:49303
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)