coreboot-gerrit April 2023

coreboot-gerrit@coreboot.org

1 participants
5761 discussions

[M] Change in coreboot[master]: acpi/acpi.c: Follow spec more closely for MADT
by Maximilian Brune (Code Review) 06 Apr '23

06 Apr '23

Attention is currently required from: Lance Zhao, Tarun Tuli, Subrata Banik, Benjamin Doron, Paul Menzel, Arthur Heymans. Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69225 ) Change subject: acpi/acpi.c: Follow spec more closely for MADT ...................................................................... Patch Set 25: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/69225 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a98560760b662a7ba7efb46f5f7882fb0f7bb1f Gerrit-Change-Number: 69225 Gerrit-PatchSet: 25 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.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: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 06 Apr 2023 15:41:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in coreboot[master]: Reland drivers/intel/dptf: Add multiple fan support under dptf
by Lean Sheng Tan (Code Review) 06 Apr '23

06 Apr '23

Attention is currently required from: Lance Zhao, Subrata Banik, Caveh Jalali, Paul Menzel, Reka Norman, Tim Wawrzynczak, Sumeet R Pawnikar, Maximilian Brune, Boris Mittelberg. Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73249 ) Change subject: Reland drivers/intel/dptf: Add multiple fan support under dptf ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/73249 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id07d279ff962253c22be9d395ed7be0d732aeaa7 Gerrit-Change-Number: 73249 Gerrit-PatchSet: 4 Gerrit-Owner: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Thu, 06 Apr 2023 15:39:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: acpi/acpi.c: Follow spec more closely for MADT
by Lean Sheng Tan (Code Review) 06 Apr '23

06 Apr '23

Attention is currently required from: Lance Zhao, Tarun Tuli, Subrata Banik, Benjamin Doron, Maximilian Brune, Arthur Heymans. Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69225 ) Change subject: acpi/acpi.c: Follow spec more closely for MADT ...................................................................... Patch Set 25: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/69225 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a98560760b662a7ba7efb46f5f7882fb0f7bb1f Gerrit-Change-Number: 69225 Gerrit-PatchSet: 25 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 06 Apr 2023 15:32:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: drivers/fsp2_0/mp_service_ppi: Use struct device to fill in buffer
by Lean Sheng Tan (Code Review) 06 Apr '23

06 Apr '23

Attention is currently required from: Subrata Banik, Patrick Rudolph, Maximilian Brune, Arthur Heymans, Werner Zeh, Andrey Petrov. Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69223 ) Change subject: drivers/fsp2_0/mp_service_ppi: Use struct device to fill in buffer ...................................................................... Patch Set 24: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/69223 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7322b43f5b95dda5fbe81e7427f5269c9d6f8755 Gerrit-Change-Number: 69223 Gerrit-PatchSet: 24 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Comment-Date: Thu, 06 Apr 2023 15:29:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: cpu/x86/topology: Add code to fill in topology on struct path
by Lean Sheng Tan (Code Review) 06 Apr '23

06 Apr '23

Lean Sheng Tan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/69222 ) ( 19 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: cpu/x86/topology: Add code to fill in topology on struct path ...................................................................... cpu/x86/topology: Add code to fill in topology on struct path This is needed to generate MADT and SRAT where lapicid for threads need to be added last. When CPUID leaf '0xB' is not present assume some defaults that would result in identical ACPI code generation. Change-Id: I2210eb9b663dd90941a64132aa7154440dc7e5a9 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/69222 Reviewed-by: Maximilian Brune <maximilian.brune(a)9elements.com> Reviewed-by: Patrick Rudolph <siro(a)das-labor.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/cpu/x86/Makefile.inc M src/cpu/x86/mp_init.c A src/cpu/x86/topology.c M src/device/cpu_device.c A src/include/cpu/x86/topology.h M src/include/device/device.h 6 files changed, 141 insertions(+), 19 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Maximilian Brune: Looks good to me, but someone else must approve diff --git a/src/cpu/x86/Makefile.inc b/src/cpu/x86/Makefile.inc index 2381b6f..cb76343 100644 --- a/src/cpu/x86/Makefile.inc +++ b/src/cpu/x86/Makefile.inc @@ -65,3 +65,5 @@ $(call src-to-obj,$(TARGET_STAGE),$(SIPI_BIN).manual): $(SIPI_BIN) @printf " OBJCOPY $(subst $(obj)/,,$(@))\n" cd $(dir $<); $(OBJCOPY_rmodules_$(ARCH-$(TARGET_STAGE)-y)) -I binary $(notdir $<) $(target-objcopy) $(abspath $@) + +ramstage-y += topology.c diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index a5d2ae6..4e92b75 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -13,6 +13,7 @@ #include <cpu/x86/msr.h> #include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> +#include <cpu/x86/topology.h> #include <cpu/x86/mp.h> #include <delay.h> #include <device/device.h> @@ -203,6 +204,8 @@ dev->path.apic.initial_lapicid = initial_lapicid(); dev->enabled = 1; + set_cpu_topology_from_leaf_b(dev); + if (cpu_is_intel()) printk(BIOS_INFO, "AP: slot %u apic_id %x, MCU rev: 0x%08x\n", index, dev->path.apic.apic_id, get_current_microcode_rev()); diff --git a/src/cpu/x86/topology.c b/src/cpu/x86/topology.c new file mode 100644 index 0000000..6c8d7fd --- /dev/null +++ b/src/cpu/x86/topology.c @@ -0,0 +1,104 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <cpu/cpu.h> +#include <device/device.h> +#include <cpu/x86/topology.h> + +#define CPUID_EXTENDED_CPU_TOPOLOGY 0x0b +#define LEVEL_TYPE_CORE 2 +#define LEVEL_TYPE_SMT 1 + +#define CPUID_CPU_TOPOLOGY(x, val) \ + (((val) >> CPUID_CPU_TOPOLOGY_##x##_SHIFT) & CPUID_CPU_TOPOLOGY_##x##_MASK) + +#define CPUID_CPU_TOPOLOGY_LEVEL_TYPE_SHIFT 0x8 +#define CPUID_CPU_TOPOLOGY_LEVEL_TYPE_MASK 0xff +#define CPUID_CPU_TOPOLOGY_LEVEL(res) CPUID_CPU_TOPOLOGY(LEVEL_TYPE, (res).ecx) + +#define CPUID_CPU_TOPOLOGY_LEVEL_BITS_SHIFT 0x0 +#define CPUID_CPU_TOPOLOGY_LEVEL_BITS_MASK 0x1f +#define CPUID_CPU_TOPOLOGY_THREAD_BITS(res) CPUID_CPU_TOPOLOGY(LEVEL_BITS, (res).eax) +#define CPUID_CPU_TOPOLOGY_CORE_BITS(res, threadbits) \ + ((CPUID_CPU_TOPOLOGY(LEVEL_BITS, (res).eax)) - threadbits) + +/* Get number of bits for core ID and SMT ID */ +static enum cb_err get_cpu_core_thread_bits(uint32_t *core_bits, uint32_t *thread_bits) +{ + struct cpuid_result cpuid_regs; + int level_num, cpu_id_op = 0; + const uint32_t cpuid_max_func = cpuid_get_max_func(); + + /* + * Not all CPUs support this, those won't get topology filled in here. + * CPU specific code can do this however. + */ + if (cpuid_max_func < CPUID_EXTENDED_CPU_TOPOLOGY) + return CB_ERR; + + cpu_id_op = CPUID_EXTENDED_CPU_TOPOLOGY; + + *core_bits = level_num = 0; + cpuid_regs = cpuid_ext(cpu_id_op, level_num); + + /* + * Sub-leaf index 0 enumerates SMT level, some AMD CPUs leave this CPUID leaf + * reserved so bail out. Cpu specific code can fill in the topology later. + */ + if (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs) != LEVEL_TYPE_SMT) + return CB_ERR; + + *thread_bits = CPUID_CPU_TOPOLOGY_THREAD_BITS(cpuid_regs); + do { + level_num++; + cpuid_regs = cpuid_ext(cpu_id_op, level_num); + if (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs) == LEVEL_TYPE_CORE) { + *core_bits = CPUID_CPU_TOPOLOGY_CORE_BITS(cpuid_regs, *thread_bits); + break; + } + /* Stop when level type is invalid i.e 0 */ + } while (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs)); + + return CB_SUCCESS; +} + +static void set_cpu_topology(struct device *cpu, unsigned int node, + unsigned int package, unsigned int core, + unsigned int thread) +{ + cpu->path.apic.node_id = node; + cpu->path.apic.package_id = package; + cpu->path.apic.core_id = core; + cpu->path.apic.thread_id = thread; +} + +void set_cpu_topology_from_leaf_b(struct device *cpu) +{ + static uint32_t core_bits, thread_bits; + static enum cb_err core_thread_bits_ret; + static bool done = false; + if (!done) { + core_thread_bits_ret = get_cpu_core_thread_bits(&core_bits, &thread_bits); + done = true; + } + + const uint32_t apicid = cpu->path.apic.initial_lapicid; + uint32_t package_id, core_id, thread_id; + /* + * If leaf_b does not exist set the following best-guess defaults: + * - 1 package + * - no SMP + * - core_id = apicid + * CPU specific code can always update these later on. + */ + if (core_thread_bits_ret != CB_SUCCESS) { + package_id = 0; + core_id = apicid; + thread_id = 0; + } else { + package_id = apicid >> (thread_bits + core_bits); + core_id = (apicid >> thread_bits) & ((1 << core_bits) - 1); + thread_id = apicid & ((1 << thread_bits) - 1); + } + + set_cpu_topology(cpu, 0, package_id, core_id, thread_id); +} diff --git a/src/device/cpu_device.c b/src/device/cpu_device.c index 9185cc6..be28ff6 100644 --- a/src/device/cpu_device.c +++ b/src/device/cpu_device.c @@ -29,13 +29,3 @@ return cpu; } - -void set_cpu_topology(struct device *cpu, unsigned int node, - unsigned int package, unsigned int core, - unsigned int thread) -{ - cpu->path.apic.node_id = node; - cpu->path.apic.package_id = package; - cpu->path.apic.core_id = core; - cpu->path.apic.thread_id = thread; -} diff --git a/src/include/cpu/x86/topology.h b/src/include/cpu/x86/topology.h new file mode 100644 index 0000000..db29d09 --- /dev/null +++ b/src/include/cpu/x86/topology.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef CPU_X86_TOPOLOGY_H +#define CPU_X86_TOPOLOGY_H + +#include <device/device.h> + +/* Fill in the topology in struct path APIC based CPUID EAX=0xb. + * If leaf 0xb is not supported or is not implemented then no topology + * will be filled in. + */ +void set_cpu_topology_from_leaf_b(struct device *cpu); + +#endif diff --git a/src/include/device/device.h b/src/include/device/device.h index 8a663f0..831141f 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -240,15 +240,6 @@ int dev_count_cpu(void); struct device *add_cpu_device(struct bus *cpu_bus, unsigned int apic_id, int enabled); -void set_cpu_topology(struct device *cpu, unsigned int node, - unsigned int package, unsigned int core, unsigned int thread); - -#define amd_cpu_topology(cpu, node, core) \ - set_cpu_topology(cpu, node, 0, core, 0) - -#define intel_cpu_topology(cpu, package, core, thread) \ - set_cpu_topology(cpu, 0, package, core, thread) - void mp_init_cpus(DEVTREE_CONST struct bus *cpu_bus); static inline void mp_cpu_bus_init(struct device *dev) { -- To view, visit https://review.coreboot.org/c/coreboot/+/69222 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2210eb9b663dd90941a64132aa7154440dc7e5a9 Gerrit-Change-Number: 69222 Gerrit-PatchSet: 23 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daocheng Bu <daocheng.bu(a)intel.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: Ziang Wang <ziang.wang(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[master]: cpu/mp_init.c: Only enable CPUs once they execute code
by Lean Sheng Tan (Code Review) 06 Apr '23

06 Apr '23

Lean Sheng Tan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/68892 ) ( 23 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: cpu/mp_init.c: Only enable CPUs once they execute code ...................................................................... cpu/mp_init.c: Only enable CPUs once they execute code On some systems the BSP cannot know how many CPUs are present in the system. A typical use case is a multi socket system. Setting the enable flag only on CPUs that actually exist makes it more flexible. Change-Id: I6c8042b4d6127239175924f996f735bf9c83c6e8 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/68892 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Patrick Rudolph <siro(a)das-labor.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/cpu/x86/mp_init.c 1 file changed, 20 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index df6bc4b..a5d2ae6 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -201,6 +201,7 @@ /* Fix up APIC id with reality. */ dev->path.apic.apic_id = lapicid(); dev->path.apic.initial_lapicid = initial_lapicid(); + dev->enabled = 1; if (cpu_is_intel()) printk(BIOS_INFO, "AP: slot %u apic_id %x, MCU rev: 0x%08x\n", index, @@ -387,6 +388,7 @@ continue; } new->name = processor_name; + new->enabled = 0; /* Runtime will enable it */ } return max_cpus; -- To view, visit https://review.coreboot.org/c/coreboot/+/68892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6c8042b4d6127239175924f996f735bf9c83c6e8 Gerrit-Change-Number: 68892 Gerrit-PatchSet: 27 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.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-MessageType: merged

1 0

[M] Change in coreboot[master]: mb/google/myst: Add initial I2C configuration
by Jon Murphy (Code Review) 06 Apr '23

06 Apr '23

Attention is currently required from: Raul Rangel, Eric Lai, Karthik Ramasubramanian, Mark Hasemeyer. Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74108 ) Change subject: mb/google/myst: Add initial I2C configuration ...................................................................... Patch Set 23: (1 comment) File src/mainboard/google/myst/variants/myst/overridetree.cb: https://review.coreboot.org/c/coreboot/+/74108/comment/de38d09b_5c4c31d5 PS23, Line 32: device domain 0 on end # domain > if you want change this better in https://review.coreboot. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/74108 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I44668295fb6ed03992df9d9fc075792e181d1a8a Gerrit-Change-Number: 74108 Gerrit-PatchSet: 23 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Comment-Date: Thu, 06 Apr 2023 15:23:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: cpu/smm_module_loader.c: Fix up CPU index locally
by Lean Sheng Tan (Code Review) 06 Apr '23

06 Apr '23

Lean Sheng Tan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/64802 ) ( 27 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: cpu/smm_module_loader.c: Fix up CPU index locally ...................................................................... cpu/smm_module_loader.c: Fix up CPU index locally Don't pass the stub params to the mp_init code. Change-Id: I070bc00ae5e5bceb6c5b90ea833cc057dd41f6cc Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/64802 Reviewed-by: Patrick Rudolph <siro(a)das-labor.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/include/cpu/x86/smm.h 3 files changed, 31 insertions(+), 20 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 01ca64e..df6bc4b 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -744,16 +744,6 @@ } } -static void adjust_smm_apic_id_map(struct smm_loader_params *smm_params) -{ - struct smm_stub_params *stub_params = smm_params->stub_params; - - int i = 0; - for (struct device *dev = g_cpu_bus->children; dev; dev = dev->sibling) - if (dev->enabled) - stub_params->apic_id_to_cpu[i++] = dev->path.apic.initial_lapicid; -} - static enum cb_err install_relocation_handler(int num_cpus, size_t save_state_size) { if (CONFIG(X86_SMM_SKIP_RELOCATION_HANDLER)) @@ -770,7 +760,6 @@ printk(BIOS_ERR, "%s: smm setup failed\n", __func__); return CB_ERR; } - adjust_smm_apic_id_map(&smm_params); return CB_SUCCESS; } @@ -796,8 +785,6 @@ if (smm_load_module(smbase, smsize, &smm_params)) return CB_ERR; - adjust_smm_apic_id_map(&smm_params); - return CB_SUCCESS; } diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 6cd9956..47a2831 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -5,7 +5,9 @@ #include <commonlib/helpers.h> #include <commonlib/region.h> #include <console/console.h> +#include <cpu/cpu.h> #include <cpu/x86/smm.h> +#include <device/device.h> #include <rmodule.h> #include <stdio.h> #include <string.h> @@ -267,12 +269,21 @@ stub_params->fxsave_area = (uintptr_t)fxsave_area; stub_params->fxsave_area_size = FXSAVE_SIZE; - /* Initialize the APIC id to CPU number table to be 1:1 */ - for (int i = 0; i < params->num_cpus; i++) - stub_params->apic_id_to_cpu[i] = i; + /* This runs on the BSP. All the APs are its siblings */ + struct cpu_info *info = cpu_info(); + if (!info || !info->cpu) { + printk(BIOS_ERR, "%s: Failed to find BSP struct device\n", __func__); + return -1; + } + int i = 0; + for (struct device *dev = info->cpu; dev; dev = dev->sibling) + if (dev->enabled) + stub_params->apic_id_to_cpu[i++] = dev->path.apic.initial_lapicid; - /* Allow the initiator to manipulate SMM stub parameters. */ - params->stub_params = stub_params; + if (i != params->num_cpus) { + printk(BIOS_ERR, "%s: Failed to set up apic map correctly\n", __func__); + return -1; + } printk(BIOS_DEBUG, "%s: stack_top = 0x%x\n", __func__, stub_params->stack_top); printk(BIOS_DEBUG, "%s: per cpu stack_size = 0x%x\n", __func__, diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index efafa53..4c97f21 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -156,8 +156,6 @@ size_t num_concurrent_save_states; smm_handler_t handler; - - struct smm_stub_params *stub_params; }; /* All of these return 0 on success, < 0 on failure. */ -- To view, visit https://review.coreboot.org/c/coreboot/+/64802 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I070bc00ae5e5bceb6c5b90ea833cc057dd41f6cc Gerrit-Change-Number: 64802 Gerrit-PatchSet: 32 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: cpu/x86/mp_init.c: Generate a C header to get start32 offset
by Lean Sheng Tan (Code Review) 06 Apr '23

06 Apr '23

Lean Sheng Tan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/64625 ) ( 27 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: cpu/x86/mp_init.c: Generate a C header to get start32 offset ...................................................................... cpu/x86/mp_init.c: Generate a C header to get start32 offset In the current design the relocatable parameters are used to know the offset of the 32bit startpoint. This requires back and forward interaction between the stub, the loader and the mp init code. This makes the code hard to read. This is static information known at buildtime, so a better way to deal with this is to generate a header that contains this offset. Change-Id: Ic01badd2af11a6e1dbc27c8e928916fedf104b5b Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/64625 Reviewed-by: Patrick Rudolph <siro(a)das-labor.org> Reviewed-by: Maximilian Brune <maximilian.brune(a)9elements.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/cpu/x86/Makefile.inc M src/cpu/x86/mp_init.c M src/cpu/x86/smm/smm_module_loader.c M src/cpu/x86/smm/smm_stub.S A src/cpu/x86/smm_start32_offset.h.template M src/include/cpu/x86/smm.h 6 files changed, 49 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Maximilian Brune: Looks good to me, but someone else must approve diff --git a/src/cpu/x86/Makefile.inc b/src/cpu/x86/Makefile.inc index 3ef3a90..2381b6f 100644 --- a/src/cpu/x86/Makefile.inc +++ b/src/cpu/x86/Makefile.inc @@ -8,6 +8,20 @@ subdirs-y += cache subdirs-$(CONFIG_PARALLEL_MP) += name + +ifeq ($(CONFIG_HAVE_SMI_HANDLER),y) +$(obj)/ramstage/cpu/x86/smm_start32_offset.h: $(dir)/smm_start32_offset.h.template $(obj)/smmstub/smmstub.elf + cp $< $@.temp + sed -i 's/##START32_OFFSET##/0x'$$($(NM_smmstub) -an $(obj)/smmstub/smmstub.elf | grep smm_trampolin | cut -d' ' -f1)'/' $@.temp + mv $@.temp $@ +else +$(obj)/ramstage/cpu/x86/smm_start32_offset.h: $(dir)/smm_start32_offset.h.template + cp $< $@.temp + sed -i 's/##START32_OFFSET##/0x0/' $@.temp + mv $@.temp $@ +endif + +$(call src-to-obj,ramstage,$(dir)/mp_init.c): $(obj)/ramstage/cpu/x86/smm_start32_offset.h ramstage-$(CONFIG_PARALLEL_MP) += mp_init.c ramstage-y += backup_default_smm.c diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 2909022..01ca64e 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -24,6 +24,9 @@ #include <thread.h> #include <types.h> +/* Generated header */ +#include <ramstage/cpu/x86/smm_start32_offset.h> + #include <security/intel/stm/SmmStm.h> struct mp_callback { @@ -672,7 +675,6 @@ uintptr_t perm_smbase; size_t perm_smsize; size_t smm_save_state_size; - uintptr_t reloc_start32_offset; bool do_smm; } mp_state; @@ -738,7 +740,7 @@ stm_setup(mseg, p->cpu, perm_smbase, mp_state.perm_smbase, - mp_state.reloc_start32_offset); + SMM_START32_OFFSET); } } @@ -770,8 +772,6 @@ } adjust_smm_apic_id_map(&smm_params); - mp_state.reloc_start32_offset = smm_params.stub_params->start32_offset; - return CB_SUCCESS; } diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 6452707..6cd9956 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -277,8 +277,6 @@ printk(BIOS_DEBUG, "%s: stack_top = 0x%x\n", __func__, stub_params->stack_top); printk(BIOS_DEBUG, "%s: per cpu stack_size = 0x%x\n", __func__, stub_params->stack_size); - printk(BIOS_DEBUG, "%s: runtime.start32_offset = 0x%x\n", __func__, - stub_params->start32_offset); printk(BIOS_DEBUG, "%s: runtime.smm_size = 0x%zx\n", __func__, smm_size); smm_stub_place_staggered_entry_points(params); diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 02532a4..19e9c50 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -31,9 +31,6 @@ * into the table. */ apic_to_cpu_num: .fill CONFIG_MAX_CPUS,2,0xffff -/* allows the STM to bring up SMM in 32-bit mode */ -start32_offset: -.long smm_trampoline32 - _start .data /* Provide fallback stack to use when a valid CPU number cannot be found. */ diff --git a/src/cpu/x86/smm_start32_offset.h.template b/src/cpu/x86/smm_start32_offset.h.template new file mode 100644 index 0000000..023bca7 --- /dev/null +++ b/src/cpu/x86/smm_start32_offset.h.template @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _SMM_START32_OFFSET_H +#define _SMM_START32_OFFSET_H + +/* This gets filled in after building the SMM stub */ +#define SMM_START32_OFFSET ##START32_OFFSET## + +#endif diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index d281972..efafa53 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -107,8 +107,6 @@ * contiguous like the 1:1 mapping it is up to the caller of the stub * loader to adjust this mapping. */ u16 apic_id_to_cpu[CONFIG_MAX_CPUS]; - /* STM's 32bit entry into SMI handler */ - u32 start32_offset; } __packed; /* smm_handler_t is called with arg of smm_module_params pointer. */ -- To view, visit https://review.coreboot.org/c/coreboot/+/64625 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic01badd2af11a6e1dbc27c8e928916fedf104b5b Gerrit-Change-Number: 64625 Gerrit-PatchSet: 32 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> 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-MessageType: merged

1 0

[S] Change in coreboot[master]: mb/google/myst: Add FW_CONFIG
by Jon Murphy (Code Review) 06 Apr '23

06 Apr '23

Attention is currently required from: Raul Rangel, Eric Lai, Karthik Ramasubramanian, Mark Hasemeyer. Jon Murphy has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74102 ) Change subject: mb/google/myst: Add FW_CONFIG ...................................................................... Patch Set 21: (1 comment) File src/mainboard/google/myst/variants/myst/overridetree.cb: https://review.coreboot.org/c/coreboot/+/74102/comment/6951e913_681c52f9 PS4, Line 20: > e.g. https://review.coreboot. […] EMMC's are on a bayhub nvme bridge, and for the most part to us they're just slightly different nvme. We don't need a specific fw_config for it. This is how we worked it on Skyrim as well. OEM's may choose to handle it differently, but our preference is not to. In general we only add FW_CONFIG items if they can't be derived or probed otherwise, or affect boot performance. -- To view, visit https://review.coreboot.org/c/coreboot/+/74102 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If74c3649d4e8d174d9fe00a4b896c2351ee3ab19 Gerrit-Change-Number: 74102 Gerrit-PatchSet: 21 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Comment-Date: Thu, 06 Apr 2023 15:18:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jon Murphy <jpmurphy(a)google.com> Comment-In-Reply-To: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2023