Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30845
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
mb/emulation/qemu-i440fx: change file handling
Reduce the number of fw_find_file calls by returning the file structure at fw_cfg_check_file. The file structure can then be used to allocate memory and access the file content directly without recurrence searching. Remove now unnecessary function fw_cfg_load_file.
Fixed breaking function calls and add include guard at fw_cfg_if.h.
Change-Id: I48cc943aaa999e4323e9d7e5dd666c5316533dcc Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com --- M src/mainboard/emulation/qemu-i440fx/fw_cfg.c M src/mainboard/emulation/qemu-i440fx/fw_cfg.h M src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h M src/mainboard/emulation/qemu-i440fx/northbridge.c 4 files changed, 39 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/30845/1
diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c index f6a3de7..9b394f2 100644 --- a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c +++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c @@ -81,28 +81,16 @@ return NULL; }
-int fw_cfg_check_file(const char *name) +int fw_cfg_check_file(FWCfgFile *file, const char *name) { - FWCfgFile *file; - + FWCfgFile *f; if (!fw_cfg_present()) return -1; - file = fw_cfg_find_file(name); - if (!file) + f = fw_cfg_find_file(name); + if (!f) return -1; - return file->size; -} - -void fw_cfg_load_file(const char *name, void *dst) -{ - FWCfgFile *file; - - if (!fw_cfg_present()) - return; - file = fw_cfg_find_file(name); - if (!file) - return; - fw_cfg_get(file->select, dst, file->size); + *file = *f; + return 0; }
int fw_cfg_max_cpus(void) @@ -202,21 +190,21 @@
unsigned long fw_cfg_acpi_tables(unsigned long start) { + FWCfgFile f; BiosLinkerLoaderEntry *s; unsigned long *addrs, current; uint8_t *ptr; - int rc, i, j, src, dst, max; + int i, j, src, dst, max;
- rc = fw_cfg_check_file("etc/table-loader"); - if (rc < 0) + if (fw_cfg_check_file(&f, "etc/table-loader")) return 0;
printk(BIOS_DEBUG, "QEMU: found ACPI tables in fw_cfg.\n");
- max = rc / sizeof(*s); - s = malloc(rc); + max = f.size / sizeof(*s); + s = malloc(f.size); addrs = malloc(max * sizeof(*addrs)); - fw_cfg_load_file("etc/table-loader", s); + fw_cfg_get(f.select, s, f.size);
current = start; for (i = 0; i < max && s[i].command != 0; i++) { @@ -227,14 +215,14 @@ switch (s[i].command) { case BIOS_LINKER_LOADER_COMMAND_ALLOCATE: current = ALIGN(current, s[i].alloc.align); - rc = fw_cfg_check_file(s[i].alloc.file); - if (rc < 0) + if (fw_cfg_check_file(&f, s[i].alloc.file)) goto err; + printk(BIOS_DEBUG, "QEMU: loading "%s" to 0x%lx (len %d)\n", - s[i].alloc.file, current, rc); - fw_cfg_load_file(s[i].alloc.file, (void*)current); + s[i].alloc.file, current, f.size); + fw_cfg_get(f.select, (void *)current, sizeof(current)); addrs[i] = current; - current += rc; + current += f.size; break;
case BIOS_LINKER_LOADER_COMMAND_ADD_POINTER: @@ -400,15 +388,16 @@ */ unsigned long fw_cfg_smbios_tables(int *handle, unsigned long *current) { + FWCfgFile f; struct smbios_type0 *t0; unsigned long start, end; - int len, ret, i, count = 1; + int ret, i, count = 1; char *str;
- len = fw_cfg_check_file("etc/smbios/smbios-tables"); - if (len < 0) + if (fw_cfg_check_file(&f, "etc/smbios/smbios-tables")) return 0; - printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", len); + + printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size);
/* * Search backwards for "coreboot" (first string in type0 table, @@ -434,7 +423,7 @@ * We'll exclude the end marker as coreboot will add one. */ printk(BIOS_DEBUG, "QEMU: loading smbios tables to 0x%lx\n", start); - fw_cfg_load_file("etc/smbios/smbios-tables", (void*)start); + fw_cfg_get(f.select, (void *)start, sizeof(start)); end = start; do { t0 = (struct smbios_type0*)end; @@ -442,7 +431,7 @@ break; end = smbios_next(end); count++; - } while (end < start + len); + } while (end < start + f.size);
/* final fixups. */ ret = end - *current; diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg.h b/src/mainboard/emulation/qemu-i440fx/fw_cfg.h index 5bf16af..b5cdb92 100644 --- a/src/mainboard/emulation/qemu-i440fx/fw_cfg.h +++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg.h @@ -10,9 +10,13 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ +#ifndef FW_CFG_H +#define FW_CFG_H +#include "fw_cfg_if.h"
void fw_cfg_get(int entry, void *dst, int dstlen); -int fw_cfg_check_file(const char *name); -void fw_cfg_load_file(const char *name, void *dst); +int fw_cfg_check_file(FWCfgFile *file, const char *name); int fw_cfg_max_cpus(void); unsigned long fw_cfg_smbios_tables(int *handle, unsigned long *current); + +#endif /* FW_CFG_H */ diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h b/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h index 8f1e24c..46aee9b 100644 --- a/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h +++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h @@ -16,6 +16,8 @@ * This are the qemu firmware config interface defines and structs. * Copyed over from qemu soure tree, include/hw/nvram/fw_cfg.h */ +#ifndef FW_CFG_IF_H +#define FW_CFG_IF_H
#include <stdint.h>
@@ -90,3 +92,5 @@ uint8_t tabletype; uint16_t fieldoffset; } FwCfgSmbios; + +#endif /* FW_CFG_IF_H */ diff --git a/src/mainboard/emulation/qemu-i440fx/northbridge.c b/src/mainboard/emulation/qemu-i440fx/northbridge.c index 764e8a0..0ff4c54 100644 --- a/src/mainboard/emulation/qemu-i440fx/northbridge.c +++ b/src/mainboard/emulation/qemu-i440fx/northbridge.c @@ -59,17 +59,16 @@ struct resource *res; unsigned long tomk = 0, high; int idx = 10; - int size; + FWCfgFile f;
pci_domain_read_resources(dev);
- size = fw_cfg_check_file("etc/e820"); - if (size > 0) { + if (!fw_cfg_check_file(&f, "etc/e820") && f.size > 0) { /* supported by qemu 1.7+ */ - FwCfgE820Entry *list = malloc(size); + FwCfgE820Entry *list = malloc(f.size); int i; - fw_cfg_load_file("etc/e820", list); - for (i = 0; i < size/sizeof(*list); i++) { + fw_cfg_get(f.select, list, f.size); + for (i = 0; i < f.size / sizeof(*list); i++) { switch (list[i].type) { case 1: /* RAM */ printk(BIOS_DEBUG, "QEMU: e820/ram: 0x%08llx +0x%08llx\n",
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30845/1/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:
https://review.coreboot.org/#/c/30845/1/src/mainboard/emulation/qemu-i440fx/... PS1, Line 400: printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30845/2/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:
https://review.coreboot.org/#/c/30845/2/src/mainboard/emulation/qemu-i440fx/... PS2, Line 400: printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/30845/3/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:
https://review.coreboot.org/#/c/30845/3/src/mainboard/emulation/qemu-i440fx/... PS3, Line 400: printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/30845/4/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:
https://review.coreboot.org/#/c/30845/4/src/mainboard/emulation/qemu-i440fx/... PS4, Line 400: printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size); line over 80 characters
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/30845/4/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.h:
https://review.coreboot.org/#/c/30845/4/src/mainboard/emulation/qemu-i440fx/... PS4, Line 15: f I'd prefer not to include the private header. The only solution I see would be to introduce additional functions to return file->size and file->select.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/30845/4/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.h:
https://review.coreboot.org/#/c/30845/4/src/mainboard/emulation/qemu-i440fx/... PS4, Line 15: f
I'd prefer not to include the private header. […]
northbridge.c already uses this header. I don't think it's private.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30845/5/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:
https://review.coreboot.org/#/c/30845/5/src/mainboard/emulation/qemu-i440fx/... PS5, Line 400: printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/30845/6/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:
https://review.coreboot.org/#/c/30845/6/src/mainboard/emulation/qemu-i440fx/... PS6, Line 400: printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size); line over 80 characters
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/30845/7/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:
https://review.coreboot.org/#/c/30845/7/src/mainboard/emulation/qemu-i440fx/... PS7, Line 400: printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size); line over 80 characters
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30845 )
Change subject: mb/emulation/qemu-i440fx: change file handling ......................................................................
mb/emulation/qemu-i440fx: change file handling
Reduce the number of fw_find_file calls by returning the file structure at fw_cfg_check_file. The file structure can then be used to allocate memory and access the file content directly without recurrence searching. Remove now unnecessary function fw_cfg_load_file.
Fixed breaking function calls and add include guard at fw_cfg_if.h.
Change-Id: I48cc943aaa999e4323e9d7e5dd666c5316533dcc Signed-off-by: Thomas Heijligen thomas.heijligen@secunet.com Reviewed-on: https://review.coreboot.org/c/30845 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/mainboard/emulation/qemu-i440fx/fw_cfg.c M src/mainboard/emulation/qemu-i440fx/fw_cfg.h M src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h M src/mainboard/emulation/qemu-i440fx/northbridge.c 4 files changed, 39 insertions(+), 43 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c index f6a3de7..9b394f2 100644 --- a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c +++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c @@ -81,28 +81,16 @@ return NULL; }
-int fw_cfg_check_file(const char *name) +int fw_cfg_check_file(FWCfgFile *file, const char *name) { - FWCfgFile *file; - + FWCfgFile *f; if (!fw_cfg_present()) return -1; - file = fw_cfg_find_file(name); - if (!file) + f = fw_cfg_find_file(name); + if (!f) return -1; - return file->size; -} - -void fw_cfg_load_file(const char *name, void *dst) -{ - FWCfgFile *file; - - if (!fw_cfg_present()) - return; - file = fw_cfg_find_file(name); - if (!file) - return; - fw_cfg_get(file->select, dst, file->size); + *file = *f; + return 0; }
int fw_cfg_max_cpus(void) @@ -202,21 +190,21 @@
unsigned long fw_cfg_acpi_tables(unsigned long start) { + FWCfgFile f; BiosLinkerLoaderEntry *s; unsigned long *addrs, current; uint8_t *ptr; - int rc, i, j, src, dst, max; + int i, j, src, dst, max;
- rc = fw_cfg_check_file("etc/table-loader"); - if (rc < 0) + if (fw_cfg_check_file(&f, "etc/table-loader")) return 0;
printk(BIOS_DEBUG, "QEMU: found ACPI tables in fw_cfg.\n");
- max = rc / sizeof(*s); - s = malloc(rc); + max = f.size / sizeof(*s); + s = malloc(f.size); addrs = malloc(max * sizeof(*addrs)); - fw_cfg_load_file("etc/table-loader", s); + fw_cfg_get(f.select, s, f.size);
current = start; for (i = 0; i < max && s[i].command != 0; i++) { @@ -227,14 +215,14 @@ switch (s[i].command) { case BIOS_LINKER_LOADER_COMMAND_ALLOCATE: current = ALIGN(current, s[i].alloc.align); - rc = fw_cfg_check_file(s[i].alloc.file); - if (rc < 0) + if (fw_cfg_check_file(&f, s[i].alloc.file)) goto err; + printk(BIOS_DEBUG, "QEMU: loading "%s" to 0x%lx (len %d)\n", - s[i].alloc.file, current, rc); - fw_cfg_load_file(s[i].alloc.file, (void*)current); + s[i].alloc.file, current, f.size); + fw_cfg_get(f.select, (void *)current, sizeof(current)); addrs[i] = current; - current += rc; + current += f.size; break;
case BIOS_LINKER_LOADER_COMMAND_ADD_POINTER: @@ -400,15 +388,16 @@ */ unsigned long fw_cfg_smbios_tables(int *handle, unsigned long *current) { + FWCfgFile f; struct smbios_type0 *t0; unsigned long start, end; - int len, ret, i, count = 1; + int ret, i, count = 1; char *str;
- len = fw_cfg_check_file("etc/smbios/smbios-tables"); - if (len < 0) + if (fw_cfg_check_file(&f, "etc/smbios/smbios-tables")) return 0; - printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", len); + + printk(BIOS_DEBUG, "QEMU: found smbios tables in fw_cfg (len %d).\n", f.size);
/* * Search backwards for "coreboot" (first string in type0 table, @@ -434,7 +423,7 @@ * We'll exclude the end marker as coreboot will add one. */ printk(BIOS_DEBUG, "QEMU: loading smbios tables to 0x%lx\n", start); - fw_cfg_load_file("etc/smbios/smbios-tables", (void*)start); + fw_cfg_get(f.select, (void *)start, sizeof(start)); end = start; do { t0 = (struct smbios_type0*)end; @@ -442,7 +431,7 @@ break; end = smbios_next(end); count++; - } while (end < start + len); + } while (end < start + f.size);
/* final fixups. */ ret = end - *current; diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg.h b/src/mainboard/emulation/qemu-i440fx/fw_cfg.h index 5bf16af..b5cdb92 100644 --- a/src/mainboard/emulation/qemu-i440fx/fw_cfg.h +++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg.h @@ -10,9 +10,13 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ +#ifndef FW_CFG_H +#define FW_CFG_H +#include "fw_cfg_if.h"
void fw_cfg_get(int entry, void *dst, int dstlen); -int fw_cfg_check_file(const char *name); -void fw_cfg_load_file(const char *name, void *dst); +int fw_cfg_check_file(FWCfgFile *file, const char *name); int fw_cfg_max_cpus(void); unsigned long fw_cfg_smbios_tables(int *handle, unsigned long *current); + +#endif /* FW_CFG_H */ diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h b/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h index 8f1e24c..46aee9b 100644 --- a/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h +++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg_if.h @@ -16,6 +16,8 @@ * This are the qemu firmware config interface defines and structs. * Copyed over from qemu soure tree, include/hw/nvram/fw_cfg.h */ +#ifndef FW_CFG_IF_H +#define FW_CFG_IF_H
#include <stdint.h>
@@ -90,3 +92,5 @@ uint8_t tabletype; uint16_t fieldoffset; } FwCfgSmbios; + +#endif /* FW_CFG_IF_H */ diff --git a/src/mainboard/emulation/qemu-i440fx/northbridge.c b/src/mainboard/emulation/qemu-i440fx/northbridge.c index 764e8a0..0ff4c54 100644 --- a/src/mainboard/emulation/qemu-i440fx/northbridge.c +++ b/src/mainboard/emulation/qemu-i440fx/northbridge.c @@ -59,17 +59,16 @@ struct resource *res; unsigned long tomk = 0, high; int idx = 10; - int size; + FWCfgFile f;
pci_domain_read_resources(dev);
- size = fw_cfg_check_file("etc/e820"); - if (size > 0) { + if (!fw_cfg_check_file(&f, "etc/e820") && f.size > 0) { /* supported by qemu 1.7+ */ - FwCfgE820Entry *list = malloc(size); + FwCfgE820Entry *list = malloc(f.size); int i; - fw_cfg_load_file("etc/e820", list); - for (i = 0; i < size/sizeof(*list); i++) { + fw_cfg_get(f.select, list, f.size); + for (i = 0; i < f.size / sizeof(*list); i++) { switch (list[i].type) { case 1: /* RAM */ printk(BIOS_DEBUG, "QEMU: e820/ram: 0x%08llx +0x%08llx\n",
-
build bot (Jenkins) (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Thomas Heijligen (Code Review)