Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31518
Change subject: vendorcode/siemens: Save currently open hwi file name ......................................................................
vendorcode/siemens: Save currently open hwi file name
On every call of hwilib_find_blocks() the CBFS file will be mapped and the contents are parsed to get the offsets for every single block. This is not needed if the CBFS file name is the same for the different calls.
This patch adds a storage for the currently open CBFS file name in CAR_GLOBAL and checks on each call if the file to open is already open. If yes, the file will not be mapped again which saves execution time.
Test=Booted mc_tcu3, mc_bdx1 and mc_apl1 and verified that hwinfo.hex is only mapped once across several following hwilib_find_blocks() calls. In addition a test was done to ensure that files with different names get mapped correctly.
Change-Id: Id69e0f6c914c2b8e4551fd8a4fb7d452d176afb3 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/vendorcode/siemens/hwilib/hwilib.c 1 file changed, 19 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/31518/1
diff --git a/src/vendorcode/siemens/hwilib/hwilib.c b/src/vendorcode/siemens/hwilib/hwilib.c index f15937b..b618304 100644 --- a/src/vendorcode/siemens/hwilib/hwilib.c +++ b/src/vendorcode/siemens/hwilib/hwilib.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2014 Siemens AG + * Copyright (C) 2014-2019 Siemens AG * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -39,6 +39,7 @@ #define EIB_FEATRUE_OFFSET 0x00e #define LEN_MAGIC_NUM 0x007 #define BLOCK_MAGIC "H1W2M3I" +#define HWI_MAX_NAME_LEN 32
/* Define all supported block types. */ enum { @@ -79,6 +80,9 @@ */ static uint16_t all_blk_size[MAX_BLOCK_NUM] CAR_GLOBAL;
+/* Storage for the cbfs file name of the currently open hwi file. */ +static char current_hwi[HWI_MAX_NAME_LEN] CAR_GLOBAL; +
static uint32_t hwilib_read_bytes (const struct param_info *param, uint8_t *dst, uint32_t maxlen); @@ -469,11 +473,21 @@ uint32_t next_offset = 1; uint8_t **blk_ptr = car_get_var_ptr(&all_blocks[0]); uint16_t *all_blk_size_ptr = car_get_var_ptr(&all_blk_size[0]); + char *curr_hwi_name_ptr = car_get_var_ptr(¤t_hwi); size_t filesize = 0;
/* Check for a valid parameter */ if (!hwi_filename) return CB_ERR_ARG; + /* Check if this file is already open. If yes, just leave as there is + * nothing left to do here. */ + if (curr_hwi_name_ptr && + !strncmp(curr_hwi_name_ptr, hwi_filename, HWI_MAX_NAME_LEN)) { + printk(BIOS_SPEW, "HWILIB: File "%s" already open.\n", + hwi_filename); + return CB_SUCCESS; + } + ptr = cbfs_boot_map_with_leak(hwi_filename, CBFS_TYPE_RAW, &filesize); if (!ptr) { printk(BIOS_ERR,"HWILIB: Missing file "%s" in cbfs.\n", @@ -533,8 +547,11 @@ } /* We should have found at least one valid block */ if (blk_ptr[BLK_HIB] || blk_ptr[BLK_SIB] || blk_ptr[BLK_EIB] || - blk_ptr[BLK_XIB]) + blk_ptr[BLK_XIB]) { + /* Save currently opened hwi filename. */ + strncpy(curr_hwi_name_ptr, hwi_filename, HWI_MAX_NAME_LEN); return CB_SUCCESS; + } else return CB_ERR; }
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31518 )
Change subject: vendorcode/siemens: Save currently open hwi file name ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31518/1/src/vendorcode/siemens/hwilib/hwilib... File src/vendorcode/siemens/hwilib/hwilib.c:
https://review.coreboot.org/#/c/31518/1/src/vendorcode/siemens/hwilib/hwilib... PS1, Line 483: * I think you don't need that for short comments.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31518 )
Change subject: vendorcode/siemens: Save currently open hwi file name ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31518/1/src/vendorcode/siemens/hwilib/hwilib... File src/vendorcode/siemens/hwilib/hwilib.c:
https://review.coreboot.org/#/c/31518/1/src/vendorcode/siemens/hwilib/hwilib... PS1, Line 483: *
I think you don't need that for short comments.
Yes, that's true. I need to improve, happens too often lately.
Hello Mario Scheithauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31518
to look at the new patch set (#2).
Change subject: vendorcode/siemens: Save currently open hwi file name ......................................................................
vendorcode/siemens: Save currently open hwi file name
On every call of hwilib_find_blocks() the CBFS file will be mapped and the contents are parsed to get the offsets for every single block. This is not needed if the CBFS file name is the same for the different calls.
This patch adds a storage for the currently open CBFS file name in CAR_GLOBAL and checks on each call if the file to open is already open. If yes, the file will not be mapped again which saves execution time.
Test=Booted mc_tcu3, mc_bdx1 and mc_apl1 and verified that hwinfo.hex is only mapped once across several following hwilib_find_blocks() calls. In addition a test was done to ensure that files with different names get mapped correctly.
Change-Id: Id69e0f6c914c2b8e4551fd8a4fb7d452d176afb3 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/vendorcode/siemens/hwilib/hwilib.c 1 file changed, 19 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/31518/2
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31518 )
Change subject: vendorcode/siemens: Save currently open hwi file name ......................................................................
Patch Set 2: Code-Review+2
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31518 )
Change subject: vendorcode/siemens: Save currently open hwi file name ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31518 )
Change subject: vendorcode/siemens: Save currently open hwi file name ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Nice!
https://review.coreboot.org/#/c/31518/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31518/2//COMMIT_MSG@7 PS2, Line 7: Save Maybe even *Cache*?
s/open/opened/ though I am not sure about that.
https://review.coreboot.org/#/c/31518/2//COMMIT_MSG@15 PS2, Line 15: which saves execution time Could you please add hard numbers for your platforms?
Hello Mario Scheithauer, Paul Menzel, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31518
to look at the new patch set (#3).
Change subject: vendorcode/siemens: Cache currently opened hwi file name ......................................................................
vendorcode/siemens: Cache currently opened hwi file name
On every call of hwilib_find_blocks() the CBFS file will be mapped and the contents are parsed to get the offsets for every single block. This is not needed if the CBFS file name is the same for the different calls.
This patch adds a storage for the currently opened CBFS file name in CAR_GLOBAL and checks on each call if the file to open is already open. If yes, the file will not be mapped again which saves execution time.
Test=Booted mc_tcu3, mc_bdx1 and mc_apl1 and verified that hwinfo.hex is only mapped once across several following hwilib_find_blocks() calls. In addition a test was done to ensure that files with different names get mapped correctly.
Change-Id: Id69e0f6c914c2b8e4551fd8a4fb7d452d176afb3 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/vendorcode/siemens/hwilib/hwilib.c 1 file changed, 19 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/31518/3
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31518 )
Change subject: vendorcode/siemens: Cache currently opened hwi file name ......................................................................
Patch Set 3: Code-Review+2
Werner Zeh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31518 )
Change subject: vendorcode/siemens: Cache currently opened hwi file name ......................................................................
vendorcode/siemens: Cache currently opened hwi file name
On every call of hwilib_find_blocks() the CBFS file will be mapped and the contents are parsed to get the offsets for every single block. This is not needed if the CBFS file name is the same for the different calls.
This patch adds a storage for the currently opened CBFS file name in CAR_GLOBAL and checks on each call if the file to open is already open. If yes, the file will not be mapped again which saves execution time.
Test=Booted mc_tcu3, mc_bdx1 and mc_apl1 and verified that hwinfo.hex is only mapped once across several following hwilib_find_blocks() calls. In addition a test was done to ensure that files with different names get mapped correctly.
Change-Id: Id69e0f6c914c2b8e4551fd8a4fb7d452d176afb3 Signed-off-by: Werner Zeh werner.zeh@siemens.com Reviewed-on: https://review.coreboot.org/c/31518 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/vendorcode/siemens/hwilib/hwilib.c 1 file changed, 19 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved
diff --git a/src/vendorcode/siemens/hwilib/hwilib.c b/src/vendorcode/siemens/hwilib/hwilib.c index f15937b..75e7cdb 100644 --- a/src/vendorcode/siemens/hwilib/hwilib.c +++ b/src/vendorcode/siemens/hwilib/hwilib.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2014 Siemens AG + * Copyright (C) 2014-2019 Siemens AG * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -39,6 +39,7 @@ #define EIB_FEATRUE_OFFSET 0x00e #define LEN_MAGIC_NUM 0x007 #define BLOCK_MAGIC "H1W2M3I" +#define HWI_MAX_NAME_LEN 32
/* Define all supported block types. */ enum { @@ -79,6 +80,9 @@ */ static uint16_t all_blk_size[MAX_BLOCK_NUM] CAR_GLOBAL;
+/* Storage for the cbfs file name of the currently open hwi file. */ +static char current_hwi[HWI_MAX_NAME_LEN] CAR_GLOBAL; +
static uint32_t hwilib_read_bytes (const struct param_info *param, uint8_t *dst, uint32_t maxlen); @@ -469,11 +473,21 @@ uint32_t next_offset = 1; uint8_t **blk_ptr = car_get_var_ptr(&all_blocks[0]); uint16_t *all_blk_size_ptr = car_get_var_ptr(&all_blk_size[0]); + char *curr_hwi_name_ptr = car_get_var_ptr(¤t_hwi); size_t filesize = 0;
/* Check for a valid parameter */ if (!hwi_filename) return CB_ERR_ARG; + /* Check if this file is already open. If yes, just leave as there is + nothing left to do here. */ + if (curr_hwi_name_ptr && + !strncmp(curr_hwi_name_ptr, hwi_filename, HWI_MAX_NAME_LEN)) { + printk(BIOS_SPEW, "HWILIB: File "%s" already open.\n", + hwi_filename); + return CB_SUCCESS; + } + ptr = cbfs_boot_map_with_leak(hwi_filename, CBFS_TYPE_RAW, &filesize); if (!ptr) { printk(BIOS_ERR,"HWILIB: Missing file "%s" in cbfs.\n", @@ -533,8 +547,11 @@ } /* We should have found at least one valid block */ if (blk_ptr[BLK_HIB] || blk_ptr[BLK_SIB] || blk_ptr[BLK_EIB] || - blk_ptr[BLK_XIB]) + blk_ptr[BLK_XIB]) { + /* Save currently opened hwi filename. */ + strncpy(curr_hwi_name_ptr, hwi_filename, HWI_MAX_NAME_LEN); return CB_SUCCESS; + } else return CB_ERR; }
-
Mario Scheithauer (Code Review) -
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Werner Zeh (Code Review)