Change in ...coreboot[master]: drivers/smmstore: Fix some issues
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30432 Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Make the API ARCH independent (no dependency on size_t) - clean up the code a little Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c M src/include/smmstore.h M src/soc/intel/common/block/smm/smihandler.c 3 files changed, 86 insertions(+), 67 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/1 diff --git a/src/drivers/smmstore/store.c b/src/drivers/smmstore/store.c index 409949a..0a62b3c 100644 --- a/src/drivers/smmstore/store.c +++ b/src/drivers/smmstore/store.c @@ -62,8 +62,8 @@ if (cbfs_locate_file_in_region(&file, CONFIG_SMMSTORE_REGION, CONFIG_SMMSTORE_FILENAME, NULL) < 0) { - printk(BIOS_WARNING, "smm store: " - "Unable to find SMM store file in region '%s'\n", + printk(BIOS_WARNING, + "smm store: Unable to find SMM store file in region '%s'\n", CONFIG_SMMSTORE_REGION); return -1; } @@ -87,14 +87,13 @@ * returns 0 on success, -1 on failure * writes up to `*bufsize` bytes into `buf` and updates `*bufsize` */ -int smmstore_read_region(void *buf, ssize_t *bufsize) +int smmstore_read_region(void *buf, uint32_t *bufsize) { struct region_device store; if (bufsize == NULL) return -1; - *bufsize = 0; if (lookup_store(&store) < 0) { printk(BIOS_WARNING, "reading region failed\n"); return -1; @@ -109,6 +108,66 @@ return 0; } +static enum cb_err scan_end(ssize_t *end) +{ + struct region_device store; + + if (lookup_store(&store) < 0) { + printk(BIOS_WARNING, "reading region failed\n"); + return CB_ERR; + } + ssize_t data_sz = region_device_sz(&store); + + /* scan for end */ + *end = 0; + uint32_t k_sz, v_sz; + while (*end < data_sz) { + /* make odd corner cases identifiable, eg. invalid v_sz */ + k_sz = 0; + + if (rdev_readat(&store, &k_sz, *end, sizeof(k_sz)) < 0) { + printk(BIOS_WARNING, "failed reading key size\n"); + return CB_ERR; + } + + /* found the end */ + if (k_sz == 0xffffffff) + break; + + /* something is fishy here: + * Avoid wrapping (since data_size < MAX_UINT32_T / 2) while + * other problems are covered by the loop condition + */ + if (k_sz > data_sz) { + printk(BIOS_WARNING, "key size out of bounds\n"); + return CB_ERR; + } + + if (rdev_readat(&store, &v_sz, *end + 4, sizeof(v_sz)) < 0) { + printk(BIOS_WARNING, "failed reading value size\n"); + return CB_ERR; + } + + if (v_sz > data_sz) { + printk(BIOS_WARNING, "value size out of bounds\n"); + return CB_ERR; + } + + *end += sizeof(k_sz) + sizeof(v_sz) + k_sz + v_sz + 1; + *end = ALIGN_UP(*end, sizeof(uint32_t)); + } + + printk(BIOS_WARNING, "used smm store size might be 0x%zx bytes\n", *end); + + if (k_sz != 0xffffffff) { + printk(BIOS_WARNING, + "eof of data marker looks invalid: 0x%x\n", k_sz); + return CB_ERR; + } + + return CB_SUCCESS; + +} /* * Append data to region * @@ -124,78 +183,38 @@ return -1; } - ssize_t data_sz = region_device_sz(&store); - - /* scan for end */ - ssize_t end = 0; - uint32_t k_sz, v_sz; - while (end < data_sz) { - /* make odd corner cases identifiable, eg. invalid v_sz */ - k_sz = 0; - - if (rdev_readat(&store, &k_sz, end, sizeof(k_sz)) < 0) { - printk(BIOS_WARNING, "failed reading key size\n"); - return -1; - } - - /* found the end */ - if (k_sz == 0xffffffff) - break; - - /* something is fishy here: - * Avoid wrapping (since data_size < MAX_UINT32_T / 2) while - * other problems are covered by the loop condition - */ - if (k_sz > data_sz) { - printk(BIOS_WARNING, "key size out of bounds\n"); - return -1; - } - - if (rdev_readat(&store, &v_sz, end + 4, sizeof(v_sz)) < 0) { - printk(BIOS_WARNING, "failed reading value size\n"); - return -1; - } - - if (v_sz > data_sz) { - printk(BIOS_WARNING, "value size out of bounds\n"); - return -1; - } - - end += 8 + k_sz + v_sz + 1; - end = ALIGN_UP(end, sizeof(uint32_t)); - } - - printk(BIOS_WARNING, "used smm store size might be 0x%zx bytes\n", end); - - if (k_sz != 0xffffffff) { - printk(BIOS_WARNING, - "eof of data marker looks invalid: 0x%x\n", k_sz); + ssize_t end; + if (scan_end(&end) == CB_ERR) return -1; - } printk(BIOS_WARNING, "used size looks legit\n"); printk(BIOS_WARNING, "open (%zx, %zx) for writing\n", region_device_offset(&store), region_device_sz(&store)); - if (boot_device_rw_subregion(&store.region, &store) < 0) { + if (boot_device_rw_subregion(region_device_region(&store), &store) < 0) { printk(BIOS_WARNING, "couldn't open store for writing\n"); return -1; } - uint32_t record_sz = 8 + key_sz + value_sz + 1; - if (end + record_sz >= data_sz) { + struct region subregion = + { + .offset = end, + .size = sizeof(key_sz) + sizeof(value_sz) + key_sz + value_sz + 1, + }; + + if (region_is_subregion(region_device_region(&store), &subregion)) { printk(BIOS_WARNING, "not enough space for new data\n"); return -1; } - if (rdev_writeat(&store, &key_sz, end, 4) != 4) { + if (rdev_writeat(&store, &key_sz, end, sizeof(key_sz) != sizeof(key_sz))) { printk(BIOS_WARNING, "failed writing key size\n"); } - end += 4; - if (rdev_writeat(&store, &value_sz, end, 4) != 4) { + end += sizeof(key_sz); + if (rdev_writeat(&store, &value_sz, end, sizeof(value_sz) != sizeof(key_sz))) { printk(BIOS_WARNING, "failed writing value size\n"); } - end += 4; + end += sizeof(value_sz); if (rdev_writeat(&store, key, end, key_sz) != key_sz) { printk(BIOS_WARNING, "failed writing key data\n"); } @@ -205,7 +224,7 @@ } end += value_sz; uint8_t nul = 0; - if (rdev_writeat(&store, &nul, end, 1) != 1) { + if (rdev_writeat(&store, &nul, end, sizeof(nul)) != sizeof(nul)) { printk(BIOS_WARNING, "failed writing termination\n"); } diff --git a/src/include/smmstore.h b/src/include/smmstore.h index a535c5b..30a8191 100644 --- a/src/include/smmstore.h +++ b/src/include/smmstore.h @@ -28,22 +28,22 @@ #define SMMSTORE_CMD_APPEND 3 struct smmstore_params_read { - void *buf; - ssize_t bufsize; + uint32_t *buf; + uint32_t bufsize; }; struct smmstore_params_append { - void *key; - size_t keysize; - void *val; - size_t valsize; + uint32_t *key; + uint32_t keysize; + uint32_t *val; + uint32_t valsize; }; /* SMM responder */ uint32_t smmstore_exec(uint8_t command, void *param); /* implementation */ -int smmstore_read_region(void *buf, ssize_t *bufsize); +int smmstore_read_region(void *buf, uint32_t *bufsize); int smmstore_append_data(void *key, uint32_t key_sz, void *value, uint32_t value_sz); int smmstore_clear_region(void); diff --git a/src/soc/intel/common/block/smm/smihandler.c b/src/soc/intel/common/block/smm/smihandler.c index 2ffc00f..c125c0c 100644 --- a/src/soc/intel/common/block/smm/smihandler.c +++ b/src/soc/intel/common/block/smm/smihandler.c @@ -311,7 +311,7 @@ reg_ebx = save_state_ops->get_reg(io_smi, RBX); /* drivers/smmstore/smi.c */ - ret = smmstore_exec(sub_command, (void *)reg_ebx); + ret = smmstore_exec(sub_command, (uintptr_t *)reg_ebx); save_state_ops->set_reg(io_smi, RAX, ret); } -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: newchange
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 6: (6 comments) https://review.coreboot.org/#/c/30432/6//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/30432/6//COMMIT_MSG@11 PS6, Line 11: no dependency on size_t I don't understand the point of this line item. API can freely use size_t as it shouldn't matter. https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@90 PS6, Line 90: uint32_t I don't understand the point of this change. There's not reason one can't use ssize_t or size_t. https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@105 PS6, Line 105: bufsize I don't like the change of the type, but this is a nonsense check now that bufsize is pointing to an unsigned integer. https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@187 PS6, Line 187: if (scan_end(&end) == CB_ERR) We're doing lookup_store() inside of scan_end(). Why are we duplicating those paths? Can we combine these paths so we're not duplicating work? Similarly, I would expect lookup_store() to return a writable region_device. The fact that it differs based on IS_ENABLED(CONFIG_SMMSTORE_IN_CBFS) is odd. https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@201 PS6, Line 201: 1 you open coded 1 here, but didn't below w/ nul. I think we should be consistent. https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@230 PS6, Line 230: return 0; We're just always successful it seems? -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 16 Jan 2019 22:07:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 7: (7 comments) https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c@193 PS7, Line 193: if (boot_device_rw_subregion(region_device_region(&store), &store) < 0) { line over 80 characters https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c@200 PS7, Line 200: .size = sizeof(key_sz) + sizeof(value_sz) + key_sz + value_sz + 1, line over 80 characters https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c@208 PS7, Line 208: if (rdev_writeat(&store, &key_sz, end, sizeof(key_sz)) != sizeof(key_sz)) { line over 80 characters https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c@208 PS7, Line 208: if (rdev_writeat(&store, &key_sz, end, sizeof(key_sz)) != sizeof(key_sz)) { braces {} are not necessary for single statement blocks https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c@212 PS7, Line 212: if (rdev_writeat(&store, &value_sz, end, sizeof(value_sz)) != sizeof(key_sz)) { line over 80 characters https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c@212 PS7, Line 212: if (rdev_writeat(&store, &value_sz, end, sizeof(value_sz)) != sizeof(key_sz)) { braces {} are not necessary for single statement blocks https://review.coreboot.org/#/c/30432/7/src/drivers/smmstore/store.c@225 PS7, Line 225: if (rdev_writeat(&store, &nul, end, sizeof(nul)) != sizeof(nul)) { braces {} are not necessary for single statement blocks -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 01 May 2019 20:13:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#8). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 58 insertions(+), 41 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/8 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 8: (4 comments) https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c@188 PS8, Line 188: if (boot_device_rw_subregion(region_device_region(&store), &store) < 0) { line over 80 characters https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c@195 PS8, Line 195: .size = sizeof(key_sz) + sizeof(value_sz) + key_sz + value_sz + sizeof(nul), line over 80 characters https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c@203 PS8, Line 203: if (rdev_writeat(&store, &key_sz, end, sizeof(key_sz)) != sizeof(key_sz)) { line over 80 characters https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c@208 PS8, Line 208: if (rdev_writeat(&store, &value_sz, end, sizeof(value_sz)) != sizeof(key_sz)) { line over 80 characters -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 29 May 2019 16:57:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 8: (2 comments) https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c@94 PS8, Line 94: -1 CB_ERR (here, and rest of file?) https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c@107 PS8, Line 107: 0 CB_SUCCESS (other instances as well?) -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 29 May 2019 19:56:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 8: (2 comments) https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@187 PS6, Line 187: if (scan_end(&end) == CB_ERR)
We're doing lookup_store() inside of scan_end(). […] We don't seem to have functions to return cbfsfiles as writable region?
https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/8/src/drivers/smmstore/store.c@94 PS8, Line 94: -1
CB_ERR (here, and rest of file?) The interface is defined above and the function calling it also uses plain numbers.
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Sun, 09 Jun 2019 09:22:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Comment-In-Reply-To: Matt DeVillier <matt.devillier@gmail.com> Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#9). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 62 insertions(+), 41 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 9 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 9: (1 comment) https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/6/src/drivers/smmstore/store.c@187 PS6, Line 187: if (scan_end(&end) == CB_ERR)
We don't seem to have functions to return cbfsfiles as writable region? No, we don't. cbfs files are not writeable. You can use the incoherent_rdev api, though. See src/drivers/mrc_cache/mrc_cache.c for its usage.
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 9 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Mon, 10 Jun 2019 14:32:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 9: (3 comments) https://review.coreboot.org/#/c/30432/9/src/drivers/smmstore/store.c File src/drivers/smmstore/store.c: https://review.coreboot.org/#/c/30432/9/src/drivers/smmstore/store.c@200 PS9, Line 200: if (region_is_subregion(region_device_region(&store), &subregion)) { If you just use rdev_chain() you'll have a region_device that is strictly the are you are about to write. Similarly, the subregion checking is already done in rdev_chain(). i.e. if (rdev_chain(&store, &store, end, size)) fail; https://review.coreboot.org/#/c/30432/9/src/drivers/smmstore/store.c@212 PS9, Line 212: key_sz value_sz https://review.coreboot.org/#/c/30432/9/src/drivers/smmstore/store.c@227 PS9, Line 227: if (rdev_writeat(&store, &nul, end, sizeof(nul)) != sizeof(nul)) { Would it be better to have the equivalent of FILE such that the offset is tracked and bumped when reading/writing? -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 9 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Mon, 10 Jun 2019 15:43:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#10). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access UNTESTED. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 106 insertions(+), 58 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/10 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 10 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#11). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access UNTESTED. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 104 insertions(+), 59 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/11 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 11 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 11: (3 comments) https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... PS6, Line 187: if (scan_end(&end) == CB_ERR)
No, we don't. cbfs files are not writeable. You can use the incoherent_rdev api, though. […] Done
https://review.coreboot.org/c/coreboot/+/30432/9/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/9/src/drivers/smmstore/store.... PS9, Line 200: if (region_is_subregion(region_device_region(&store), &subregion)) {
If you just use rdev_chain() you'll have a region_device that is strictly the are you are about to w […] Done
https://review.coreboot.org/c/coreboot/+/30432/9/src/drivers/smmstore/store.... PS9, Line 212: key_sz
value_sz Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 11 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 23 Sep 2019 12:11:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#12). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access UNTESTED. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 104 insertions(+), 59 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/12 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 12 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 12: (2 comments) https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... PS12, Line 58: struct region_device *rdev; This is just a pointer that is uninitialized and isn't pointing to a valid object. https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... PS12, Line 106: *rstore = *store_rdev; This will not work. The incoherent_rdev object needs to persist through the lifetime of its use. The implementation assumes it can access the outer object through the use of the embedded regino_device object. Likewise the read and write region_device objects also need to persist as the icoherent_rdev object just holds a reference to the objects themselves. https://github.com/coreboot/coreboot/blob/master/src/commonlib/include/commo... -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 12 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 23 Sep 2019 23:06:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#13). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access UNTESTED. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 105 insertions(+), 59 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/13 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 13 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 13: (6 comments) https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 60: rdev.region *region_device_region(&rdev); https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 87: store_rdev This change seems like quite the short cut. store_rdev only is used once below. However, the comment I made on patchset 12 on line 106 equally applies still as you have violated the conditions needed to use the API. https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 118: int smmstore_read_region(void *buf, ssize_t *bufsize) There are 3 APIs to SMM store from the interface: 1. smmstore_read_region() 2. smmstore_append_data() 3. smmstore_clear_region() All 3 need to determine the location of the region itself. They are all one-shot and expect no runtime state to persist this compliation unit outside of those calls. #1 only needs read access #2 needs read and write access #3 only needs write access If you want one function to rule them all then you will need to construct the incoherent_rdev correctly. The code below should work the way you want: static int lookup_store(struct region_device *rstore) { static struct region_device read_rdev, write_rdev; static struct incoherent_rdev store_irdev; struct region region; struct region_device *rdev; if (lookup_store_region(®ion)) return -1; if (boot_device_ro_subregion(®ion, &read_rdev) < 0) return -1; if (boot_device_rw_subregion(®ion, &write_rdev) < 0) return -1; rdev = incoherent_rdev_init(&store_irdev, ®ion, &read_rdev, &write_rdev); if (rdev == NULL) return -1; return rdev_chain(rstore, rdev, 0, region_device_sz(rdev)); } https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 211: data_sz I don't understand why one is passing in data_sz scan_end() can obtain it directly since 'store' is being passed in. And since end is onl used once, by this function, you can just adjust store within scan_end() to have store only point to the remaining region. Then below you would rdev_chain(..., 0, size) for a bounds check. https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 219: &store) < 0) { You already have a writable rdev by way of using incoherent_rdev in lookup_store(). https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 231: end This should be 0. The rdev_chain() tracks the window of the region it is operating on. As such the offsets are relative to that: (0, size]. Therefore end needs to be reset to 0 to start the writes. -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 13 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Sep 2019 15:47:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#14). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access UNTESTED. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 101 insertions(+), 63 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/14 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 14 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#15). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access UNTESTED. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 101 insertions(+), 63 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/15 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 15 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 15: (4 comments) https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 97: *bufsize = 0; Did you want to get rid of this? The SMI external interface might assume that things are zero'd out on failure? I'm not really sure of the expectation of what would happen with SMMSTORE_RET_FAILURE from the callee. https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 136: ssize_t *end There's no need for this. https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 185: you can do this here: if (rdev_chain(store, store, end, data_sz - end)) return CB_ERR; return CB_SUCCESS; Upon returning from this function. The region device is pointing at the next section that is writable. https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 217: end And this would be 0 once scan_end() has positioned the region device correctly. And it can also be named correctly: offset. -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 15 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Sep 2019 16:25:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#16). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access UNTESTED. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 104 insertions(+), 64 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/16 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 16 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 16: Code-Review+1 (1 comment) https://review.coreboot.org/c/coreboot/+/30432/16//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/30432/16//COMMIT_MSG@15 PS16, Line 15: UNTESTED. Should be tested. -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 16 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Sep 2019 20:16:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 16: (1 comment) https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 97: *bufsize = 0;
Did you want to get rid of this? The SMI external interface might assume that things are zero'd out […] I got rid of it because the variable tx was always 0. I can put this statement in the "reading region failed" case, given that it likely was here to ensure that things are zero'd out?
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 16 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Sep 2019 20:17:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Gerrit-MessageType: comment
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 16: (1 comment) https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 97: *bufsize = 0;
I got rid of it because the variable tx was always 0. […] It was more of a larger question I don't have an answer on. The SMI callee (from OS or something else) may expect the object to be 0'd. It was sort of implicit as I don't see a comment w.r.t. expectations. Patrick should know what the expectations are of the callee.
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 16 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Sep 2019 20:26:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 16: (1 comment) https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 97: *bufsize = 0;
It was more of a larger question I don't have an answer on. […] It wasn't part of the contract, but the idea was indeed that a stupid client would figure out that it's supposed to read 0 bytes instead of working with whatever happens to be in there.
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 16 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 01 Oct 2019 15:42:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 16: Code-Review+1 tested working on google/wolf (after hooking up lynxpoint sb) -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 16 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 25 Nov 2019 23:43:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#17). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - use an incoherent rdev to potentially speed up reading access TESTED on google/wolf with out of tree patch to hook up smmstore to sb/intel/lynxpoint. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 104 insertions(+), 64 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/17 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 17 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 17: (18 comments) https://review.coreboot.org/c/coreboot/+/30432/6//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/30432/6//COMMIT_MSG@11 PS6, Line 11: no dependency on size_t
I don't understand the point of this line item. API can freely use size_t as it shouldn't matter. Ack
https://review.coreboot.org/c/coreboot/+/30432/16//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/30432/16//COMMIT_MSG@15 PS16, Line 15: UNTESTED.
Should be tested. Done
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... PS6, Line 90: uint32_t
I don't understand the point of this change. There's not reason one can't use ssize_t or size_t. Ack
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... PS6, Line 105: bufsize
I don't like the change of the type, but this is a nonsense check now that bufsize is pointing to an […] Done
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... PS6, Line 201: 1
you open coded 1 here, but didn't below w/ nul. I think we should be consistent. Done
https://review.coreboot.org/c/coreboot/+/30432/6/src/drivers/smmstore/store.... PS6, Line 230: return 0;
We're just always successful it seems? Done
https://review.coreboot.org/c/coreboot/+/30432/8/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/8/src/drivers/smmstore/store.... PS8, Line 94: -1
The interface is defined above and the function calling it also uses plain numbers. Ack
https://review.coreboot.org/c/coreboot/+/30432/8/src/drivers/smmstore/store.... PS8, Line 107: 0
CB_SUCCESS (other instances as well?) Ack
https://review.coreboot.org/c/coreboot/+/30432/9/src/drivers/smmstore/store.... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/9/src/drivers/smmstore/store.... PS9, Line 227: if (rdev_writeat(&store, &nul, end, sizeof(nul)) != sizeof(nul)) {
Would it be better to have the equivalent of FILE such that the offset is tracked and bumped when re […] And an equivalent of fseek etc? That's getting complex fast and I'd prefer to see more places where it might be useful before we go there. But definitely something to keep in mind if this pattern proliferates.
https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... PS12, Line 58: struct region_device *rdev;
This is just a pointer that is uninitialized and isn't pointing to a valid object. Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 60: rdev.region
*region_device_region(&rdev); Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 87: store_rdev
This change seems like quite the short cut. store_rdev only is used once below. […] Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 211: data_sz
I don't understand why one is passing in data_sz scan_end() can obtain it directly since 'store' is […] Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 219: &store) < 0) {
You already have a writable rdev by way of using incoherent_rdev in lookup_store(). Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 231: end
This should be 0. The rdev_chain() tracks the window of the region it is operating on. […] Done
https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 136: ssize_t *end
There's no need for this. Done
https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 185:
you can do this here: […] Done
https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 217: end
And this would be 0 once scan_end() has positioned the region device correctly. […] Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 17 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 26 Nov 2019 12:57:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Comment-In-Reply-To: Matt DeVillier <matt.devillier@gmail.com> Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 17: (1 comment) https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/15/src/drivers/smmstore/store... PS15, Line 97: *bufsize = 0;
It wasn't part of the contract, but the idea was indeed that a stupid client would figure out that i […] So we're now expecting clients to look for the return code. I'm fine with that.
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 17 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 26 Nov 2019 13:00:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Comment-In-Reply-To: Patrick Georgi <pgeorgi@google.com> Gerrit-MessageType: comment
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 17: (2 comments) https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/12/src/drivers/smmstore/store... PS12, Line 106: *rstore = *store_rdev;
This will not work. The incoherent_rdev object needs to persist through the lifetime of its use. […] Done
https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/13/src/drivers/smmstore/store... PS13, Line 118: int smmstore_read_region(void *buf, ssize_t *bufsize)
There are 3 APIs to SMM store from the interface: […] Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 17 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 26 Nov 2019 13:53:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin <adurbin@chromium.org> Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 17: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 17 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 26 Nov 2019 13:58:15 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 17: (2 comments) https://review.coreboot.org/c/coreboot/+/30432/17//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/30432/17//COMMIT_MSG@13 PS17, Line 13: - use an incoherent rdev to potentially speed up reading access The commit only adds the static function `lookup_store_region()`? https://review.coreboot.org/c/coreboot/+/30432/17/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/17/src/drivers/smmstore/store... PS17, Line 46: static int lookup_store_region(struct region *region) Use CB_SUCCESS and friends. -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 17 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Assignee: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 26 Nov 2019 18:43:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Matthew Garrett, Aaron Durbin, Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/30432 to look at the new patch set (#18). Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - Clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - Use an incoherent rdev to potentially speed up reading access TESTED on google/wolf with out of tree patch to hook up smmstore to sb/intel/lynxpoint. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/drivers/smmstore/store.c 1 file changed, 104 insertions(+), 64 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30432/18 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 18 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Assignee: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 18: (2 comments) https://review.coreboot.org/c/coreboot/+/30432/17//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/30432/17//COMMIT_MSG@13 PS17, Line 13: - use an incoherent rdev to potentially speed up reading access
The commit only adds the static function `lookup_store_region()`?
It's a helper function so I don't think it's worth mentioning here. https://review.coreboot.org/c/coreboot/+/30432/17/src/drivers/smmstore/store... File src/drivers/smmstore/store.c: https://review.coreboot.org/c/coreboot/+/30432/17/src/drivers/smmstore/store... PS17, Line 46: static int lookup_store_region(struct region *region)
Use CB_SUCCESS and friends. Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 18 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Assignee: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 26 Nov 2019 22:34:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... Patch Set 18: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 18 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Assignee: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 29 Nov 2019 08:56:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/30432 ) Change subject: drivers/smmstore: Fix some issues ...................................................................... drivers/smmstore: Fix some issues This fixes the following: - Fix smmstore_read_region to actually read stuff - Clean up the code a little - Change the loglevel for non error messages to BIOS_DEBUG - Use an incoherent rdev to potentially speed up reading access TESTED on google/wolf with out of tree patch to hook up smmstore to sb/intel/lynxpoint. Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/30432 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Patrick Georgi <pgeorgi@google.com> --- M src/drivers/smmstore/store.c 1 file changed, 104 insertions(+), 64 deletions(-) Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved diff --git a/src/drivers/smmstore/store.c b/src/drivers/smmstore/store.c index dc4a0cf..23d2af0 100644 --- a/src/drivers/smmstore/store.c +++ b/src/drivers/smmstore/store.c @@ -43,6 +43,33 @@ * crash/reboot could clear out all variables. */ +static enum cb_err lookup_store_region(struct region *region) +{ + if (CONFIG(SMMSTORE_IN_CBFS)) { + struct cbfsf file; + if (cbfs_locate_file_in_region(&file, + CONFIG_SMMSTORE_REGION, + CONFIG_SMMSTORE_FILENAME, NULL) < 0) { + printk(BIOS_WARNING, + "smm store: Unable to find SMM store file in region '%s'\n", + CONFIG_SMMSTORE_REGION); + return CB_ERR; + } + struct region_device rdev; + cbfs_file_data(&rdev, &file); + *region = *region_device_region(&rdev); + } else { + if (fmap_locate_area(CONFIG_SMMSTORE_REGION, region)) { + printk(BIOS_WARNING, + "smm store: Unable to find SMM store FMAP region '%s'\n", + CONFIG_SMMSTORE_REGION); + return CB_ERR; + } + } + + return CB_SUCCESS; +} + /* * Return a region device that points into the store file. * @@ -57,28 +84,26 @@ */ static int lookup_store(struct region_device *rstore) { - struct cbfsf file; - if (CONFIG(SMMSTORE_IN_CBFS)) { - if (cbfs_locate_file_in_region(&file, - CONFIG_SMMSTORE_REGION, - CONFIG_SMMSTORE_FILENAME, NULL) < 0) { - printk(BIOS_WARNING, "smm store: " - "Unable to find SMM store file in region '%s'\n", - CONFIG_SMMSTORE_REGION); - return -1; - } + static struct region_device read_rdev, write_rdev; + static struct incoherent_rdev store_irdev; + struct region region; + const struct region_device *rdev; - cbfs_file_data(rstore, &file); - } else { - if (fmap_locate_area_as_rdev_rw(CONFIG_SMMSTORE_REGION, rstore)) { - printk(BIOS_WARNING, - "smm store: Unable to find SMM store FMAP region '%s'\n", - CONFIG_SMMSTORE_REGION); - return -1; - } - } + if (lookup_store_region(®ion) != CB_SUCCESS) + return -1; - return 0; + if (boot_device_ro_subregion(®ion, &read_rdev) < 0) + return -1; + + if (boot_device_rw_subregion(®ion, &write_rdev) < 0) + return -1; + + rdev = incoherent_rdev_init(&store_irdev, ®ion, &read_rdev, &write_rdev); + + if (rdev == NULL) + return -1; + + return rdev_chain(rstore, rdev, 0, region_device_sz(rdev)); } /* @@ -94,13 +119,12 @@ if (bufsize == NULL) return -1; - *bufsize = 0; if (lookup_store(&store) < 0) { printk(BIOS_WARNING, "reading region failed\n"); return -1; } - ssize_t tx = min(*bufsize, region_device_sz(&store)); + ssize_t tx = MIN(*bufsize, region_device_sz(&store)); *bufsize = rdev_readat(&store, buf, 0, tx); if (*bufsize < 0) @@ -109,33 +133,19 @@ return 0; } -/* - * Append data to region - * - * Returns 0 on success, -1 on failure - */ -int smmstore_append_data(void *key, uint32_t key_sz, - void *value, uint32_t value_sz) +static enum cb_err scan_end(struct region_device *store) { - struct region_device store; - - if (lookup_store(&store) < 0) { - printk(BIOS_WARNING, "reading region failed\n"); - return -1; - } - - ssize_t data_sz = region_device_sz(&store); - /* scan for end */ ssize_t end = 0; uint32_t k_sz, v_sz; + const ssize_t data_sz = region_device_sz(store); while (end < data_sz) { /* make odd corner cases identifiable, eg. invalid v_sz */ k_sz = 0; - if (rdev_readat(&store, &k_sz, end, sizeof(k_sz)) < 0) { + if (rdev_readat(store, &k_sz, end, sizeof(k_sz)) < 0) { printk(BIOS_WARNING, "failed reading key size\n"); - return -1; + return CB_ERR; } /* found the end */ @@ -148,65 +158,95 @@ */ if (k_sz > data_sz) { printk(BIOS_WARNING, "key size out of bounds\n"); - return -1; + return CB_ERR; } - if (rdev_readat(&store, &v_sz, end + 4, sizeof(v_sz)) < 0) { + if (rdev_readat(store, &v_sz, end + sizeof(k_sz), sizeof(v_sz)) < 0) { printk(BIOS_WARNING, "failed reading value size\n"); - return -1; + return CB_ERR; } if (v_sz > data_sz) { printk(BIOS_WARNING, "value size out of bounds\n"); - return -1; + return CB_ERR; } - end += 8 + k_sz + v_sz + 1; + end += sizeof(k_sz) + sizeof(v_sz) + k_sz + v_sz + 1; end = ALIGN_UP(end, sizeof(uint32_t)); } - printk(BIOS_WARNING, "used smm store size might be 0x%zx bytes\n", end); + printk(BIOS_DEBUG, "used smm store size might be 0x%zx bytes\n", end); if (k_sz != 0xffffffff) { printk(BIOS_WARNING, "eof of data marker looks invalid: 0x%x\n", k_sz); + return CB_ERR; + } + + if (rdev_chain(store, store, end, data_sz - end)) + return CB_ERR; + + return CB_SUCCESS; + +} +/* + * Append data to region + * + * Returns 0 on success, -1 on failure + */ +int smmstore_append_data(void *key, uint32_t key_sz, void *value, + uint32_t value_sz) +{ + struct region_device store; + + if (lookup_store(&store) < 0) { + printk(BIOS_WARNING, "reading region failed\n"); return -1; } - printk(BIOS_WARNING, "used size looks legit\n"); + ssize_t offset = 0; + ssize_t size; + uint8_t nul = 0; + if (scan_end(&store) != CB_SUCCESS) + return -1; - printk(BIOS_WARNING, "open (%zx, %zx) for writing\n", + printk(BIOS_DEBUG, "used size looks legit\n"); + + printk(BIOS_DEBUG, "open (%zx, %zx) for writing\n", region_device_offset(&store), region_device_sz(&store)); - if (boot_device_rw_subregion(&store.region, &store) < 0) { - printk(BIOS_WARNING, "couldn't open store for writing\n"); - return -1; - } - uint32_t record_sz = 8 + key_sz + value_sz + 1; - if (end + record_sz >= data_sz) { + size = sizeof(key_sz) + sizeof(value_sz) + key_sz + value_sz + + sizeof(nul); + if (rdev_chain(&store, &store, 0, size)) { printk(BIOS_WARNING, "not enough space for new data\n"); return -1; } - if (rdev_writeat(&store, &key_sz, end, 4) != 4) { + if (rdev_writeat(&store, &key_sz, offset, sizeof(key_sz)) + != sizeof(key_sz)) { printk(BIOS_WARNING, "failed writing key size\n"); + return -1; } - end += 4; - if (rdev_writeat(&store, &value_sz, end, 4) != 4) { + offset += sizeof(key_sz); + if (rdev_writeat(&store, &value_sz, offset, sizeof(value_sz)) + != sizeof(value_sz)) { printk(BIOS_WARNING, "failed writing value size\n"); + return -1; } - end += 4; - if (rdev_writeat(&store, key, end, key_sz) != key_sz) { + offset += sizeof(value_sz); + if (rdev_writeat(&store, key, offset, key_sz) != key_sz) { printk(BIOS_WARNING, "failed writing key data\n"); + return -1; } - end += key_sz; - if (rdev_writeat(&store, value, end, value_sz) != value_sz) { + offset += key_sz; + if (rdev_writeat(&store, value, offset, value_sz) != value_sz) { printk(BIOS_WARNING, "failed writing value data\n"); + return -1; } - end += value_sz; - uint8_t nul = 0; - if (rdev_writeat(&store, &nul, end, 1) != 1) { + offset += value_sz; + if (rdev_writeat(&store, &nul, offset, sizeof(nul)) != sizeof(nul)) { printk(BIOS_WARNING, "failed writing termination\n"); + return -1; } return 0; -- To view, visit https://review.coreboot.org/c/coreboot/+/30432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I629be25d2a9b65796ae8f7a700b6bdab57b91b22 Gerrit-Change-Number: 30432 Gerrit-PatchSet: 19 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Assignee: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Matt DeVillier <matt.devillier@gmail.com> Gerrit-Reviewer: Matthew Garrett <mjg59@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
participants (6)
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Matt DeVillier (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)