coreboot-gerrit November 2022

coreboot-gerrit@coreboot.org

1 participants
7286 discussions

[S] Change in coreboot[master]: soc/intel/meteorlake: Fix set but unused variable
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/66264 ) Change subject: soc/intel/meteorlake: Fix set but unused variable ...................................................................... soc/intel/meteorlake: Fix set but unused variable Clang complains about this. Change-Id: Ibe1de3057c17b4aa8ecbd87fac598e43294584e3 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/66264 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/soc/intel/meteorlake/systemagent.c 1 file changed, 17 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/src/soc/intel/meteorlake/systemagent.c b/src/soc/intel/meteorlake/systemagent.c index 675f9fb..c6c1ada 100644 --- a/src/soc/intel/meteorlake/systemagent.c +++ b/src/soc/intel/meteorlake/systemagent.c @@ -83,6 +83,8 @@ return; } + /* Remove once commented line below is enabled */ + (void)soc_config; /* UPDATEME: Need to enable later */ //set_power_limits(MOBILE_SKU_PL1_TIME_SEC, soc_config); } -- To view, visit https://review.coreboot.org/c/coreboot/+/66264 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibe1de3057c17b4aa8ecbd87fac598e43294584e3 Gerrit-Change-Number: 66264 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[master]: nb/intel/ironlake: Work around unused variable warning
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/69237 ) Change subject: nb/intel/ironlake: Work around unused variable warning ...................................................................... nb/intel/ironlake: Work around unused variable warning It's not clear whether this variable should actually be used or not so leave it be with a FIXME comment. Change-Id: I4892600bfec55830acae56d2b293947c2d9ddd07 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/69237 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/northbridge/intel/ironlake/raminit.c 1 file changed, 18 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/src/northbridge/intel/ironlake/raminit.c b/src/northbridge/intel/ironlake/raminit.c index 634ba90..e3b0b33 100644 --- a/src/northbridge/intel/ironlake/raminit.c +++ b/src/northbridge/intel/ironlake/raminit.c @@ -782,6 +782,8 @@ some_delay_2_halfcycles_ceil - 1; if (!info->revision_flag_1) some_delay_2_halfcycles_floor++; + /* FIXME: this variable is unused. Should it be used? */ + (void)some_delay_2_halfcycles_floor; info->some_delay_2_halfcycles_ceil = some_delay_2_halfcycles_ceil; info->some_delay_3_ps_rounded = some_delay_3_ps_rounded; if ((info->populated_ranks[0][0][0] && info->populated_ranks[0][1][0]) -- To view, visit https://review.coreboot.org/c/coreboot/+/69237 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4892600bfec55830acae56d2b293947c2d9ddd07 Gerrit-Change-Number: 69237 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[L] Change in coreboot[master]: drivers/generic/ioapic: Drop poor implementation
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Attention is currently required from: Stefan Ott, Nico Huber, Kyösti Mälkki, Alexander Couzens. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69488 ) Change subject: drivers/generic/ioapic: Drop poor implementation ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/69488 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I383d55ba2bc0800423617215e0bfdfad5136e9ac Gerrit-Change-Number: 69488 Gerrit-PatchSet: 2 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Stefan Ott <coreboot(a)desire.ch> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Ott <coreboot(a)desire.ch> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Comment-Date: Sat, 12 Nov 2022 14:42:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/gigabyte/ga-945gcm-s2c,skl: Drop HAVE_MP_TABLE
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Attention is currently required from: Kyösti Mälkki. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69490 ) Change subject: mb/gigabyte/ga-945gcm-s2c,skl: Drop HAVE_MP_TABLE ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/69490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I71a209e95ae1fe8c1c90b61c6ac0fb0e7bcc7eca Gerrit-Change-Number: 69490 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Sat, 12 Nov 2022 14:39:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/gigabyte/ga-945gcm-s2c,skl: Drop HAVE_MP_TABLE
by Kyösti Mälkki (Code Review) 12 Nov '22

12 Nov '22

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/69490 ) Change subject: mb/gigabyte/ga-945gcm-s2c,skl: Drop HAVE_MP_TABLE ...................................................................... mb/gigabyte/ga-945gcm-s2c,skl: Drop HAVE_MP_TABLE The weak implementation of write_smp_table() is not useful without DRIVERS_GENERIC_IOAPIC and related entries in devicetree.cb. No interrupt routing entries are present in the generated MP table. Change-Id: I71a209e95ae1fe8c1c90b61c6ac0fb0e7bcc7eca Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/mainboard/gigabyte/ga-945gcm-s2l/Kconfig 1 file changed, 15 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/69490/1 diff --git a/src/mainboard/gigabyte/ga-945gcm-s2l/Kconfig b/src/mainboard/gigabyte/ga-945gcm-s2l/Kconfig index bbf46f7..4b55c66 100644 --- a/src/mainboard/gigabyte/ga-945gcm-s2l/Kconfig +++ b/src/mainboard/gigabyte/ga-945gcm-s2l/Kconfig @@ -11,7 +11,6 @@ select SUPERIO_ITE_IT8718F select HAVE_OPTION_TABLE select HAVE_CMOS_DEFAULT - select HAVE_MP_TABLE select HAVE_ACPI_TABLES select HAVE_ACPI_RESUME select BOARD_ROMSIZE_KB_512 -- To view, visit https://review.coreboot.org/c/coreboot/+/69490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I71a209e95ae1fe8c1c90b61c6ac0fb0e7bcc7eca Gerrit-Change-Number: 69490 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: newchange

1 0

[L] Change in coreboot[master]: drivers/generic/ioapic: Drop poor implementation
by Kyösti Mälkki (Code Review) 12 Nov '22

12 Nov '22

Attention is currently required from: Stefan Ott, Nico Huber, Alexander Couzens. Hello Stefan Ott, build bot (Jenkins), Nico Huber, Alexander Couzens, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/69488 to look at the new patch set (#2). Change subject: drivers/generic/ioapic: Drop poor implementation ...................................................................... drivers/generic/ioapic: Drop poor implementation This disables MP table generation for the affected boards since interrupt routing entries would now be completely missing. The mechanism itself is flawed and redundant. The mapping of integrated PCI devices' INTx pins to IOAPIC pins is dependent of configuration registers and needs not appear in the devicetree.cb files at all. The write_smp_table implementation would skip writing any entry delivering to destination IOAPIC ID 0. This does not follow MP table specification. There were duplicate calls to register_new_ioapic_gsi0(), with another present under southbridge LPC device. Change-Id: I383d55ba2bc0800423617215e0bfdfad5136e9ac Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/Kconfig M src/arch/x86/mpspec.c D src/drivers/generic/ioapic/Kconfig D src/drivers/generic/ioapic/Makefile.inc D src/drivers/generic/ioapic/chip.h D src/drivers/generic/ioapic/ioapic.c M src/mainboard/lenovo/t400/Kconfig M src/mainboard/lenovo/t400/devicetree.cb M src/mainboard/lenovo/x200/Kconfig M src/mainboard/lenovo/x200/devicetree.cb M src/mainboard/roda/rk9/Kconfig M src/mainboard/roda/rk9/devicetree.cb M src/mainboard/samsung/lumpy/Kconfig M src/mainboard/samsung/lumpy/devicetree.cb 14 files changed, 50 insertions(+), 303 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/69488/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/69488 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I383d55ba2bc0800423617215e0bfdfad5136e9ac Gerrit-Change-Number: 69488 Gerrit-PatchSet: 2 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Stefan Ott <coreboot(a)desire.ch> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Ott <coreboot(a)desire.ch> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: cpu/x86: Set up cpu_info storage using MSR
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Attention is currently required from: Raul Rangel. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68913 ) Change subject: cpu/x86: Set up cpu_info storage using MSR ...................................................................... Patch Set 7: (1 comment) File src/arch/x86/include/arch/cpu.h: https://review.coreboot.org/c/coreboot/+/68913/comment/12406a2a_80bba07d PS5, Line 159: msr_t msr = rdmsr(IA32_GS_BASE); : ci = (struct cpu_info *)(uintptr_t)msr.raw; : return ci; > Oh, I didn't notice you changed the semantics. x86_32 %gs points to `segment_data`, while x86_64 points to `cpu_infos`. Can you leave the semantics how they were so it's not confusing? I think if you leave %gs pointing to `segment_data` you can leave this function unchanged since the `mov %gs:0` should correctly work. Oh nice catch. I fixed the sematics so that setting the GDT segment has the same semantics as setting the MSR. -- To view, visit https://review.coreboot.org/c/coreboot/+/68913 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id38d3c84ef757c0b322ec0d387f57043147cb447 Gerrit-Change-Number: 68913 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Comment-Date: Sat, 12 Nov 2022 14:29:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: cpu/x86: Set up cpu_info storage using MSR
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Attention is currently required from: Raul Rangel. Hello build bot (Jenkins), Marc Jones, Raul Rangel, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/68913 to look at the new patch set (#8). Change subject: cpu/x86: Set up cpu_info storage using MSR ...................................................................... cpu/x86: Set up cpu_info storage using MSR This MSR is available on all systems supporting long mode. This is needed to run coreboot stages above 4G as gs_base can only be set to 64bit values using the msr. Change-Id: Id38d3c84ef757c0b322ec0d387f57043147cb447 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/arch/x86/cpu.c M src/arch/x86/include/arch/cpu.h M src/include/cpu/x86/msr.h 3 files changed, 42 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/68913/8 -- To view, visit https://review.coreboot.org/c/coreboot/+/68913 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id38d3c84ef757c0b322ec0d387f57043147cb447 Gerrit-Change-Number: 68913 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: cpu/x86: Set thread local storage in C code
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/69435 ) Change subject: cpu/x86: Set thread local storage in C code ...................................................................... cpu/x86: Set thread local storage in C code Doing this in C code is way easier to understand. Also the thread local storage is now in .bss instead of the AP stack. This makes it more robust against stack overflows, as APs stacks overflow in each other. TESTED: work on qemu. Change-Id: I19d3285daf97798a2d28408b5601ad991e29e718 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/69435 Reviewed-by: Raul Rangel <rrangel(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/arch/x86/c_start.S M src/arch/x86/cpu.c M src/arch/x86/include/arch/cpu.h D src/cpu/x86/cpu_info.S.inc M src/cpu/x86/mp_init.c M src/cpu/x86/sipi_vector.S 6 files changed, 74 insertions(+), 110 deletions(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved diff --git a/src/arch/x86/c_start.S b/src/arch/x86/c_start.S index 02a9b89..5b7052e 100644 --- a/src/arch/x86/c_start.S +++ b/src/arch/x86/c_start.S @@ -1,6 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -#include <cpu/x86/cpu_info.S.inc> #include <cpu/x86/post_code.h> #include <arch/ram_segs.h> @@ -78,21 +77,6 @@ movl $_estack, %esp andl $(~(CONFIG_STACK_SIZE-1)), %esp - push_cpu_info - - /* Allocate the per_cpu_segment_data on the stack */ - push_per_cpu_segment_data - - /* - * Update the BSP's per_cpu_segment_descriptor to point to the - * per_cpu_segment_data that was allocated on the stack. - */ - set_segment_descriptor_base $per_cpu_segment_descriptors, %esp - - mov $per_cpu_segment_selector, %eax - movl (%eax), %eax - mov %eax, %gs - /* * Now we are finished. Memory is up, data is copied and * bss is cleared. Now we call the main routine and diff --git a/src/arch/x86/cpu.c b/src/arch/x86/cpu.c index f4cb83a..6a8ddef 100644 --- a/src/arch/x86/cpu.c +++ b/src/arch/x86/cpu.c @@ -6,6 +6,7 @@ #include <cpu/cpu.h> #include <post.h> #include <string.h> +#include <cpu/x86/gdt.h> #include <cpu/x86/mp.h> #include <cpu/x86/lapic.h> #include <cpu/x86/tsc.h> @@ -340,3 +341,41 @@ } return -1; } + +/* cpu_info() looks at address 0 at the base of %gs for a pointer to struct cpu_info */ +static struct per_cpu_segment_data segment_data[CONFIG_MAX_CPUS]; +static struct cpu_info cpu_infos[CONFIG_MAX_CPUS]; + +enum cb_err set_cpu_info(unsigned int index, struct device *cpu) +{ + if (index >= ARRAY_SIZE(cpu_infos)) + return CB_ERR; + + if (!cpu) + return CB_ERR; + + const struct cpu_info info = { .cpu = cpu, .index = index}; + cpu_infos[index] = info; + segment_data[index].cpu_info = &cpu_infos[index]; + + struct segment_descriptor { + uint16_t segment_limit_0_15; + uint16_t base_address_0_15; + uint8_t base_address_16_23; + uint8_t attrs[2]; + uint8_t base_address_24_31; + } *segment_descriptor = (void *)&per_cpu_segment_descriptors; + + segment_descriptor[index].base_address_0_15 = (uintptr_t)&segment_data[index] & 0xffff; + segment_descriptor[index].base_address_16_23 = ((uintptr_t)&segment_data[index] >> 16) & 0xff; + segment_descriptor[index].base_address_24_31 = ((uintptr_t)&segment_data[index] >> 24) & 0xff; + + const unsigned int cpu_segment = per_cpu_segment_selector + (index << 3); + + __asm__ __volatile__ ("mov %0, %%gs\n" + : + : "r" (cpu_segment) + : ); + + return CB_SUCCESS; +} diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index 0087a79..c7c6b4e 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -147,11 +147,13 @@ struct cpu_info *cpu_info; }; +enum cb_err set_cpu_info(unsigned int index, struct device *cpu); + static inline struct cpu_info *cpu_info(void) { struct cpu_info *ci = NULL; - __asm__("mov %%gs:%c[offset], %[ci]" + __asm__ __volatile__("mov %%gs:%c[offset], %[ci]" : [ci] "=r" (ci) : [offset] "i" (offsetof(struct per_cpu_segment_data, cpu_info)) ); diff --git a/src/cpu/x86/cpu_info.S.inc b/src/cpu/x86/cpu_info.S.inc deleted file mode 100644 index 6dca920..0000000 --- a/src/cpu/x86/cpu_info.S.inc +++ /dev/null @@ -1,72 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-or-later */ - -/* - * Pushes a 32-bit register onto the stack. - * - * There are two possible code sections where this code can be included: - * .code32 and .code64 - * - * Doing a `push %eax` while in a .code64 section will result in a compiler - * error. This macro manually pushes the 32-bit register onto the stack so we - * can share the code between 32 and 64 bit builds. - */ -.macro pushr reg:req -#if ENV_X86_64 - movl $0, -4(%esp) - movl \reg, -8(%esp) - sub $8, %esp -#else - push \reg -#endif -.endm - -/* Push struct cpu_info */ -.macro push_cpu_info index=$0 - pushr \index /* index (size_t) */ - pushr $0 /* *cpu */ -.endm - -/* Push struct per_cpu_segment_data */ -.macro push_per_cpu_segment_data cpu_info_pointer=%esp - pushr \cpu_info_pointer /* *cpu_info */ -.endm - -/* - * Sets the base address in the segment descriptor array. - * - * A segment descriptor has the following structure: - * struct { - * uint16_t segment_limit_0_15; - * uint16_t base_address_0_15; - * uint8_t base_address_16_23; - * uint8_t attrs[2]; - * uint8_t base_address_24_31; - * }; - * - * @desc_array: Address of the descriptor table - * @base: Address to set in the descriptor - * @desc_index: Index of the descriptor in the table. Defaults to 0. Must be a - * register if specified. - * - * Clobbers %eax, %ebx. - */ -.macro set_segment_descriptor_base desc_array:req, base:req, desc_index - mov \base, %eax - - mov \desc_array, %ebx - -.ifb \desc_index - movw %ax, 2(%ebx) - shr $16, %eax - movb %al, 4(%ebx) - shr $8, %eax - movb %al, 7(%ebx) -.else - movw %ax, 2(%ebx, \desc_index, 8) - shr $16, %eax - movb %al, 4(%ebx, \desc_index, 8) - shr $8, %eax - movb %al, 7(%ebx, \desc_index, 8) -.endif - -.endm diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 0e4a571..758550c 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -176,20 +176,19 @@ /* By the time APs call ap_init() caching has been setup, and microcode has * been loaded. */ -static void asmlinkage ap_init(void) +static asmlinkage void ap_init(unsigned int index) { - struct cpu_info *info = cpu_info(); - /* Ensure the local APIC is enabled */ enable_lapic(); setup_lapic_interrupts(); struct device *dev = g_cpu_bus->children; - for (unsigned int i = info->index; i > 0; i--) + for (unsigned int i = index; i > 0; i--) dev = dev->sibling; - info->cpu = dev; + set_cpu_info(index, dev); + struct cpu_info *info = cpu_info(); cpu_add_map_entry(info->index); /* Fix up APIC id with reality. */ @@ -540,6 +539,7 @@ } /* Find the device structure for the boot CPU. */ + set_cpu_info(0, bsp); info = cpu_info(); info->cpu = bsp; info->cpu->name = processor_name; diff --git a/src/cpu/x86/sipi_vector.S b/src/cpu/x86/sipi_vector.S index b8cac96..39973db 100644 --- a/src/cpu/x86/sipi_vector.S +++ b/src/cpu/x86/sipi_vector.S @@ -1,6 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -#include <cpu/x86/cpu_info.S.inc> #include <cpu/x86/cr.h> #include <cpu/amd/mtrr.h> #include <cpu/x86/msr.h> @@ -104,19 +103,8 @@ subl %eax, %edx mov %edx, %esp - push_cpu_info index=%ecx - push_per_cpu_segment_data - - /* - * Update the AP's per_cpu_segment_descriptor to point to the - * per_cpu_segment_data that was allocated on the stack. - */ - set_segment_descriptor_base per_cpu_segment_descriptors, %esp, %ecx - - mov %ecx, %eax - shl $3, %eax /* The index is << 3 in the segment selector */ - add per_cpu_segment_selector, %eax - mov %eax, %gs + /* Save CPU number for calling the AP entry */ + push %ecx /* * The following code only needs to run on Intel platforms and thus the caller @@ -230,15 +218,19 @@ mov %eax, %cr4 #endif + pop %edi /* Retrieve cpu index */ andl $0xfffffff0, %esp /* ensure stack alignment */ #if ENV_X86_64 - /* entry64.inc preserves ebx. */ + /* entry64.inc preserves ebx, esi, edi, ebp */ #include <cpu/x86/64bit/entry64.inc> - movabs c_handler, %eax call *%rax #else + push $0 + push $0 + push $0 + push %edi mov c_handler, %eax call *%eax #endif -- To view, visit https://review.coreboot.org/c/coreboot/+/69435 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I19d3285daf97798a2d28408b5601ad991e29e718 Gerrit-Change-Number: 69435 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: include/cpu/msr.h: transform into an union
by Arthur Heymans (Code Review) 12 Nov '22

12 Nov '22

Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/68572 ) Change subject: include/cpu/msr.h: transform into an union ...................................................................... include/cpu/msr.h: transform into an union This makes it easier to get the content of an msr into a full 64bit variable. Change-Id: I1b026cd3807fd68d805051a74b3d31fcde1c5626 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/68572 Reviewed-by: Raul Rangel <rrangel(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/cpu/x86/mtrr/earlymtrr.c M src/include/cpu/x86/msr.h M src/include/cpu/x86/msr_access.h M src/soc/intel/common/block/sgx/sgx.c 4 files changed, 43 insertions(+), 36 deletions(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c index aa430d5..b6cc737 100644 --- a/src/cpu/x86/mtrr/earlymtrr.c +++ b/src/cpu/x86/mtrr/earlymtrr.c @@ -55,7 +55,7 @@ void clear_all_var_mtrr(void) { - msr_t mtrr = {0, 0}; + msr_t mtrr = { .raw = 0 }; int vcnt; int i; diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h index f4e3985..33eb457 100644 --- a/src/include/cpu/x86/msr.h +++ b/src/include/cpu/x86/msr.h @@ -285,7 +285,7 @@ static inline uint64_t msr_read(unsigned int reg) { msr_t msr = rdmsr(reg); - return (((uint64_t)msr.hi << 32) | msr.lo); + return msr.raw; } /** @@ -296,10 +296,7 @@ */ static inline void msr_write(unsigned int reg, uint64_t value) { - msr_t msr = { - .lo = (unsigned int)value, - .hi = (unsigned int)(value >> 32) - }; + msr_t msr = { .raw = value }; wrmsr(reg, msr); } @@ -315,10 +312,8 @@ msr_t msr; msr = rdmsr(reg); - msr.lo &= (unsigned int)~unset; - msr.hi &= (unsigned int)~(unset >> 32); - msr.lo |= (unsigned int)set; - msr.hi |= (unsigned int)(set >> 32); + msr.raw &= ~unset; + msr.raw |= set; wrmsr(reg, msr); } diff --git a/src/include/cpu/x86/msr_access.h b/src/include/cpu/x86/msr_access.h index a7f72fd..33029cc 100644 --- a/src/include/cpu/x86/msr_access.h +++ b/src/include/cpu/x86/msr_access.h @@ -6,10 +6,14 @@ #ifndef __ASSEMBLER__ #include <types.h> -typedef struct msr_struct { - unsigned int lo; - unsigned int hi; +typedef union msr_union { + struct { + unsigned int lo; + unsigned int hi; + }; + uint64_t raw; } msr_t; +_Static_assert(sizeof(msr_t) == sizeof(uint64_t), "Incorrect size for msr_t"); #if CONFIG(SOC_SETS_MSRS) msr_t soc_msr_read(unsigned int index); diff --git a/src/soc/intel/common/block/sgx/sgx.c b/src/soc/intel/common/block/sgx/sgx.c index 3008911..88fe174 100644 --- a/src/soc/intel/common/block/sgx/sgx.c +++ b/src/soc/intel/common/block/sgx/sgx.c @@ -24,14 +24,7 @@ void prmrr_core_configure(void) { - union { - uint64_t data64; - struct { - uint32_t lo; - uint32_t hi; - } data32; - } prmrr_base, prmrr_mask; - msr_t msr; + msr_t prmrr_base, prmrr_mask; /* * Software Developer's Manual Volume 4: @@ -45,38 +38,36 @@ return; /* PRMRR_PHYS_MASK is in scope "Core" */ - msr = rdmsr(MSR_PRMRR_PHYS_MASK); + prmrr_mask = rdmsr(MSR_PRMRR_PHYS_MASK); /* If it is locked don't attempt to write PRMRR MSRs. */ - if (msr.lo & PRMRR_PHYS_MASK_LOCK) + if (prmrr_mask.lo & PRMRR_PHYS_MASK_LOCK) return; /* PRMRR base and mask are read from the UNCORE PRMRR MSRs * that are already set in FSP-M. */ - if (soc_get_uncore_prmmr_base_and_mask(&prmrr_base.data64, - &prmrr_mask.data64) < 0) { + if (soc_get_uncore_prmmr_base_and_mask(&prmrr_base.raw, + &prmrr_mask.raw) < 0) { printk(BIOS_ERR, "SGX: Failed to get PRMRR base and mask\n"); return; } - if (!prmrr_base.data32.lo) { + if (!prmrr_base.lo) { printk(BIOS_ERR, "SGX Error: Uncore PRMRR is not set!\n"); return; } - printk(BIOS_INFO, "SGX: prmrr_base = 0x%llx\n", prmrr_base.data64); - printk(BIOS_INFO, "SGX: prmrr_mask = 0x%llx\n", prmrr_mask.data64); + printk(BIOS_INFO, "SGX: prmrr_base = 0x%llx\n", prmrr_base.raw); + printk(BIOS_INFO, "SGX: prmrr_mask = 0x%llx\n", prmrr_mask.raw); /* Program core PRMRR MSRs. * - Set cache writeback mem attrib in PRMRR base MSR * - Clear the valid bit in PRMRR mask MSR * - Lock PRMRR MASK MSR */ - prmrr_base.data32.lo |= MTRR_TYPE_WRBACK; - wrmsr(MSR_PRMRR_PHYS_BASE, (msr_t) {.lo = prmrr_base.data32.lo, - .hi = prmrr_base.data32.hi}); - prmrr_mask.data32.lo &= ~PRMRR_PHYS_MASK_VALID; - prmrr_mask.data32.lo |= PRMRR_PHYS_MASK_LOCK; - wrmsr(MSR_PRMRR_PHYS_MASK, (msr_t) {.lo = prmrr_mask.data32.lo, - .hi = prmrr_mask.data32.hi}); + prmrr_base.lo |= MTRR_TYPE_WRBACK; + wrmsr(MSR_PRMRR_PHYS_BASE, prmrr_base); + prmrr_mask.lo &= ~PRMRR_PHYS_MASK_VALID; + prmrr_mask.lo |= PRMRR_PHYS_MASK_LOCK; + wrmsr(MSR_PRMRR_PHYS_MASK, prmrr_mask); } static int is_prmrr_set(void) @@ -133,7 +124,7 @@ { /* TODO - the Owner Epoch update mechanism is not determined yet, * for PoC just write '0's to the MSRs. */ - msr_t msr = {0, 0}; + msr_t msr = { .raw = 0 }; /* SGX_OWNEREPOCH is in scope "Package" */ wrmsr(MSR_SGX_OWNEREPOCH0, msr); -- To view, visit https://review.coreboot.org/c/coreboot/+/68572 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1b026cd3807fd68d805051a74b3d31fcde1c5626 Gerrit-Change-Number: 68572 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2022