Change in ...coreboot[master]: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling
Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32795
Change subject: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling ......................................................................
{arch,cpu}/x86, drivers/intel: Restore cpu_index error handling
Previously cpu_index() always succeeded, but since commit 095c931 (src/arch/x86: Use core apic id to get cpu_index()) it is now possible for it to indicate an error by returning -1. This commit adds error handling for all calls to cpu_index(), and restores several checks that were removed in 7c712bb (Fix code that would trip -Wtype-limits) but are now needed.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Change-Id: I5436eed4cb5675f916924eb9670db04592a8b927 --- M src/arch/x86/cpu.c M src/arch/x86/exception.c M src/arch/x86/include/arch/cpu.h M src/cpu/x86/mp_init.c M src/cpu/x86/pae/pgtbl.c M src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c 6 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/32795/1
diff --git a/src/arch/x86/cpu.c b/src/arch/x86/cpu.c index fb4c7b6..e6c9435 100644 --- a/src/arch/x86/cpu.c +++ b/src/arch/x86/cpu.c @@ -350,7 +350,7 @@ * Hence new logic to use cpuid to fetch lapic id and matches with * cpus_default_apic_id[] variable to return correct cpu_index(). */ -unsigned long cpu_index(void) +int cpu_index(void) { int i; int lapic_id = initial_lapicid(); diff --git a/src/arch/x86/exception.c b/src/arch/x86/exception.c index b00777a..b88f4a7 100644 --- a/src/arch/x86/exception.c +++ b/src/arch/x86/exception.c @@ -502,7 +502,7 @@ } #else /* !CONFIG_GDB_STUB */ #define MDUMP_SIZE 0x80 - unsigned int logical_processor = 0; + int logical_processor = 0;
#if ENV_RAMSTAGE logical_processor = cpu_index(); diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index 481ee9d..078ea17 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -379,6 +379,6 @@ * Hence new logic to use cpuid to fetch lapic id and matches with * cpus_default_apic_id[] variable to return correct cpu_index(). */ -unsigned long cpu_index(void); +int cpu_index(void);
#endif /* ARCH_CPU_H */ diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 8957515..b7b8fe2 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -870,13 +870,20 @@ int i; int cpus_accepted; struct stopwatch sw; - int cur_cpu = cpu_index(); + int cur_cpu;
if (!CONFIG(PARALLEL_MP_AP_WORK)) { printk(BIOS_ERR, "APs already parked. PARALLEL_MP_AP_WORK not selected.\n"); return -1; }
+ cur_cpu = cpu_index(); + + if (cur_cpu < 0) { + printk(BIOS_ERR, "Invalid CPU index.\n"); + return -1; + } + /* Signal to all the APs to run the func. */ for (i = 0; i < ARRAY_SIZE(ap_callbacks); i++) { if (cur_cpu == i) @@ -918,6 +925,12 @@ return;
cur_cpu = cpu_index(); + + if (cur_cpu < 0) { + printk(BIOS_ERR, "Invalid CPU index.\n"); + return; + } + per_cpu_slot = &ap_callbacks[cur_cpu];
while (1) { diff --git a/src/cpu/x86/pae/pgtbl.c b/src/cpu/x86/pae/pgtbl.c index 062ee40..9c92134 100644 --- a/src/cpu/x86/pae/pgtbl.c +++ b/src/cpu/x86/pae/pgtbl.c @@ -116,12 +116,12 @@ static struct pg_table pgtbl[CONFIG_MAX_CPUS] __attribute__((aligned(4096))); static unsigned long mapped_window[CONFIG_MAX_CPUS]; - unsigned long index; + int index; unsigned long window; void *result; int i; index = cpu_index(); - if (index >= CONFIG_MAX_CPUS) + if (index < 0) return MAPPING_ERROR; window = page >> 10; if (window != mapped_window[index]) { diff --git a/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c b/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c index cdc98e0..31d75a5 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c @@ -45,6 +45,9 @@ efi_uintn_t processor_number, efi_processor_information *processor_info_buffer) { + if (cpu_index() < 0) + return FSP_DEVICE_ERROR; + if (processor_info_buffer == NULL) return FSP_INVALID_PARAMETER;
@@ -68,6 +71,9 @@ efi_ap_procedure procedure, efi_boolean_t ignored3, efi_uintn_t timeout_usec, void *argument) { + if (cpu_index() < 0) + return FSP_DEVICE_ERROR; + if (procedure == NULL) return FSP_INVALID_PARAMETER;
@@ -85,6 +91,9 @@ efi_ap_procedure procedure, efi_uintn_t processor_number, efi_uintn_t timeout_usec, void *argument) { + if (cpu_index() < 0) + return FSP_DEVICE_ERROR; + if (processor_number > get_cpu_count()) return FSP_NOT_FOUND;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32795 )
Change subject: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... File src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c:
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 48: if (cpu_index() < 0) code indent should use tabs where possible
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 48: if (cpu_index() < 0) please, no spaces at the start of a line
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 74: if (cpu_index() < 0) code indent should use tabs where possible
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 74: if (cpu_index() < 0) please, no spaces at the start of a line
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 94: if (cpu_index() < 0) code indent should use tabs where possible
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 94: if (cpu_index() < 0) please, no spaces at the start of a line
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32795
to look at the new patch set (#2).
Change subject: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling ......................................................................
{arch,cpu}/x86, drivers/intel: Restore cpu_index error handling
Previously cpu_index() always succeeded, but since commit 095c931 (src/arch/x86: Use core apic id to get cpu_index()) it is now possible for it to indicate an error by returning -1. This commit adds error handling for all calls to cpu_index(), and restores several checks that were removed in commit 7c712bb (Fix code that would trip -Wtype-limits) but are now needed.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Change-Id: I5436eed4cb5675f916924eb9670db04592a8b927 --- M src/arch/x86/cpu.c M src/arch/x86/exception.c M src/arch/x86/include/arch/cpu.h M src/cpu/x86/mp_init.c M src/cpu/x86/pae/pgtbl.c M src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c 6 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/32795/2
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32795 )
Change subject: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... File src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c:
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 48: if (cpu_index() < 0)
please, no spaces at the start of a line
Done
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 48: if (cpu_index() < 0)
code indent should use tabs where possible
Done
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 74: if (cpu_index() < 0)
code indent should use tabs where possible
Done
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 74: if (cpu_index() < 0)
please, no spaces at the start of a line
Done
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 94: if (cpu_index() < 0)
please, no spaces at the start of a line
Done
https://review.coreboot.org/#/c/32795/1/src/drivers/intel/fsp2_0/ppi/mp_serv... PS1, Line 94: if (cpu_index() < 0)
code indent should use tabs where possible
Done
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32795 )
Change subject: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling ......................................................................
Patch Set 2:
Nip this in the bud before Coverity flags it
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32795 )
Change subject: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling ......................................................................
Patch Set 2: Code-Review+2
Stefan Reinauer has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32795 )
Change subject: {arch,cpu}/x86, drivers/intel: Restore cpu_index error handling ......................................................................
{arch,cpu}/x86, drivers/intel: Restore cpu_index error handling
Previously cpu_index() always succeeded, but since commit 095c931 (src/arch/x86: Use core apic id to get cpu_index()) it is now possible for it to indicate an error by returning -1. This commit adds error handling for all calls to cpu_index(), and restores several checks that were removed in commit 7c712bb (Fix code that would trip -Wtype-limits) but are now needed.
Signed-off-by: Jacob Garber jgarber1@ualberta.ca Change-Id: I5436eed4cb5675f916924eb9670db04592a8b927 Reviewed-on: https://review.coreboot.org/c/coreboot/+/32795 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org --- M src/arch/x86/cpu.c M src/arch/x86/exception.c M src/arch/x86/include/arch/cpu.h M src/cpu/x86/mp_init.c M src/cpu/x86/pae/pgtbl.c M src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c 6 files changed, 28 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved
diff --git a/src/arch/x86/cpu.c b/src/arch/x86/cpu.c index fb4c7b6..e6c9435 100644 --- a/src/arch/x86/cpu.c +++ b/src/arch/x86/cpu.c @@ -350,7 +350,7 @@ * Hence new logic to use cpuid to fetch lapic id and matches with * cpus_default_apic_id[] variable to return correct cpu_index(). */ -unsigned long cpu_index(void) +int cpu_index(void) { int i; int lapic_id = initial_lapicid(); diff --git a/src/arch/x86/exception.c b/src/arch/x86/exception.c index b00777a..b88f4a7 100644 --- a/src/arch/x86/exception.c +++ b/src/arch/x86/exception.c @@ -502,7 +502,7 @@ } #else /* !CONFIG_GDB_STUB */ #define MDUMP_SIZE 0x80 - unsigned int logical_processor = 0; + int logical_processor = 0;
#if ENV_RAMSTAGE logical_processor = cpu_index(); diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index 481ee9d..078ea17 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -379,6 +379,6 @@ * Hence new logic to use cpuid to fetch lapic id and matches with * cpus_default_apic_id[] variable to return correct cpu_index(). */ -unsigned long cpu_index(void); +int cpu_index(void);
#endif /* ARCH_CPU_H */ diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 8957515..b7b8fe2 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -870,13 +870,20 @@ int i; int cpus_accepted; struct stopwatch sw; - int cur_cpu = cpu_index(); + int cur_cpu;
if (!CONFIG(PARALLEL_MP_AP_WORK)) { printk(BIOS_ERR, "APs already parked. PARALLEL_MP_AP_WORK not selected.\n"); return -1; }
+ cur_cpu = cpu_index(); + + if (cur_cpu < 0) { + printk(BIOS_ERR, "Invalid CPU index.\n"); + return -1; + } + /* Signal to all the APs to run the func. */ for (i = 0; i < ARRAY_SIZE(ap_callbacks); i++) { if (cur_cpu == i) @@ -918,6 +925,12 @@ return;
cur_cpu = cpu_index(); + + if (cur_cpu < 0) { + printk(BIOS_ERR, "Invalid CPU index.\n"); + return; + } + per_cpu_slot = &ap_callbacks[cur_cpu];
while (1) { diff --git a/src/cpu/x86/pae/pgtbl.c b/src/cpu/x86/pae/pgtbl.c index 062ee40..9c92134 100644 --- a/src/cpu/x86/pae/pgtbl.c +++ b/src/cpu/x86/pae/pgtbl.c @@ -116,12 +116,12 @@ static struct pg_table pgtbl[CONFIG_MAX_CPUS] __attribute__((aligned(4096))); static unsigned long mapped_window[CONFIG_MAX_CPUS]; - unsigned long index; + int index; unsigned long window; void *result; int i; index = cpu_index(); - if (index >= CONFIG_MAX_CPUS) + if (index < 0) return MAPPING_ERROR; window = page >> 10; if (window != mapped_window[index]) { diff --git a/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c b/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c index cdc98e0..e26701b 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c @@ -45,6 +45,9 @@ efi_uintn_t processor_number, efi_processor_information *processor_info_buffer) { + if (cpu_index() < 0) + return FSP_DEVICE_ERROR; + if (processor_info_buffer == NULL) return FSP_INVALID_PARAMETER;
@@ -68,6 +71,9 @@ efi_ap_procedure procedure, efi_boolean_t ignored3, efi_uintn_t timeout_usec, void *argument) { + if (cpu_index() < 0) + return FSP_DEVICE_ERROR; + if (procedure == NULL) return FSP_INVALID_PARAMETER;
@@ -85,6 +91,9 @@ efi_ap_procedure procedure, efi_uintn_t processor_number, efi_uintn_t timeout_usec, void *argument) { + if (cpu_index() < 0) + return FSP_DEVICE_ERROR; + if (processor_number > get_cpu_count()) return FSP_NOT_FOUND;
-
build bot (Jenkins) (Code Review) -
Jacob Garber (Code Review)
-
Stefan Reinauer (Code Review)