December 2021 - flashrom-gerrit

Change in flashrom[master]: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config()
by Nikolai Artemiev (Code Review) 29 Dec '21

29 Dec '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58479 ) Change subject: libflashrom,writeprotect: add flashrom_wp_{read,write}_chip_config() ...................................................................... Patch Set 23: (16 comments) Patchset: PS22: > Thinking further about this, there are more reasons to tie […] This will be a fairly long comment and cover some things mentioned in other comments, but it's probably easiest to put everything here: I've thought more about what sort of API should be exposed through libflashrom and I don't think exposing the wp_chip_config structure is a good idea, even if the structure is opaque to libflashrom users. In addition to the config being to tied to the specific chip as you mention, properly sequencing read/modify/write/verify operations is unnecessary work and it increases the risk that introducing a new chip will break existing assumptions in the future. Another reason to simplify the API is that linux's MTD interface exposes a much simpler protection API. It's really inconvenient to have two APIs for SPI/MTD protection and unifying them makes sense if we don't lost much IMO. Specifically MTD gives us these operations: - MEMLOCK: Enable HW protection and set range - MEMUNLOCK: Disable protection / clear range - MEMISLOCKED: Get current protection range / HW protection status. The final libdflashrom API / functionality would be something like: - flashrom_wp_protect_flash(flashctx *, flashsize_t *start, flashsize_t *len) // Read config, set HW protection, set range, write, verify - flashrom_wp_unprotect_flash(flashctx *) // Read config, unset HW protection, set empty range, write, verify - flashrom_wp_get_protection_status(flashctx *, flashsize_t *start, flashsize_t *len, bool *hw_prot_enabled) // Read config, decode range / mode, write values to output vars We can of course expand with additional functions if they're useful, but for now I think these will cover most standard use cases and provide a more flexible API. The only major thing the new API doesn't allow is setting a protection range without enabling HW protection, but that would just get undone by flashrom or whatever other tool someone next uses to write the flash. As for migrating the current code, I plan to keep the current writeprotect functions, but keep them inside writeprotect.c and only expose the new functions. Some things that need to be shared will probably get moved to writeprotect.h, like the chip config structure. What do you think? File libflashrom.h: https://review.coreboot.org/c/flashrom/+/58479/comment/2dc379c2_f68bd077 PS22, Line 131: const > We should be careful with `const` here. Flash access goes very deep down […] Done File writeprotect.c: https://review.coreboot.org/c/flashrom/+/58479/comment/e7b9526e_6fae517b PS22, Line 35: 0 > Will this ever be consumed? Good point. I thought it would be helpful to ensure it is zeroed if the bit isn't present, but the structure is zero-initialized anyway so we don't need to do that here. https://review.coreboot.org/c/flashrom/+/58479/comment/cbf7d480_3ccb70ae PS22, Line 59: * > No dangling asterisk, please. I've made the comment a bit bigger, I think the asterisks are ok now since there are leading and trailing comment lines. https://review.coreboot.org/c/flashrom/+/58479/comment/d9aa7786_76ca184c PS22, Line 59: intrepret > int*er*pret Done https://review.coreboot.org/c/flashrom/+/58479/comment/b7e234e5_66393683 PS22, Line 61: tmp > rather `unused` or `ignored`? Done https://review.coreboot.org/c/flashrom/+/58479/comment/eefce310_37c9f4b7 PS22, Line 63: *cfg = (struct wp_chip_config) {0}; > Won't this be overwritten where it matters? iow. […] I think I might have relied on this in an earlier patch set but I don't remember now. I think it's kind of nice to zero it out before we do anything, else but I can drop it if you want. https://review.coreboot.org/c/flashrom/+/58479/comment/02149fac_a29c6663 PS22, Line 70: r( > Missing space. Done https://review.coreboot.org/c/flashrom/+/58479/comment/926f9597_d5f22b5d PS22, Line 70: size_t > Declarations inside a `for` made trouble in some environment. Please avoid it for now. Done https://review.coreboot.org/c/flashrom/+/58479/comment/694d2b85_2eb0d8a4 PS22, Line 70: for(size_t i = 0; bits->bp[i].reg != INVALID_REG; i++) { > Generally, it's preferred to loop over the target array. This way you […] Done https://review.coreboot.org/c/flashrom/+/58479/comment/37c2fc13_d2beeb61 PS22, Line 75: r( > Missing space. Done https://review.coreboot.org/c/flashrom/+/58479/comment/bb2ddb25_6cc35253 PS22, Line 84: * > No dangling asterisk, please. Or maybe just use the long comment style […] Done https://review.coreboot.org/c/flashrom/+/58479/comment/da2b5e4b_56f87daa PS22, Line 122: INVALID_REG > This is very confusing. I see it's skipped because the mask would be 0, […] I've changed it to (INVALID_REG + 1), perhaps STATUS1 would be better though. I'll change it if you want. https://review.coreboot.org/c/flashrom/+/58479/comment/1b44b521_87138102 PS22, Line 126: = 0 > Why initialize? Done https://review.coreboot.org/c/flashrom/+/58479/comment/00045ff9_57888c6e PS22, Line 144: configuraitons > configura*ti*ons Done https://review.coreboot.org/c/flashrom/+/58479/comment/37c351de_7c726a76 PS22, Line 145: which of two configurations should be used if they both select the same : * protection range > Does this really work? How can we say, for instance, that SRP1 being […] This was kind of an adaptation of an earlier function that just compared ranges so they could be sorted and a preferred range encoding could be chosen. I realized that it could also be used to compare most bits in the config structure, so I extended it to allow all bits to be compared, which is useful when we want to verify a config we have read back from a chip. In this context "preferred" is arbitrary, it should only be used to pick between two configurations if both of them are acceptable. -- To view, visit https://review.coreboot.org/c/flashrom/+/58479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3ad25708c3321b8fb0216c3eaf6ffc07616537ad Gerrit-Change-Number: 58479 Gerrit-PatchSet: 23 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Wed, 29 Dec 2021 11:31:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Move do_*() helpers into cli_classic.c
by Edward O'Callaghan (Code Review) 28 Dec '21

28 Dec '21

Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60070 ) Change subject: flashrom.c: Move do_*() helpers into cli_classic.c ...................................................................... Patch Set 4: -Code-Review -- To view, visit https://review.coreboot.org/c/flashrom/+/60070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Gerrit-Change-Number: 60070 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 29 Dec 2021 00:40:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: build: Remove cli_classic need for internal symbols [WIP]
by Edward O'Callaghan (Code Review) 28 Dec '21

28 Dec '21

Attention is currently required from: Edward O'Callaghan. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60074 to look at the new patch set (#3). Change subject: build: Remove cli_classic need for internal symbols [WIP] ...................................................................... build: Remove cli_classic need for internal symbols [WIP] cli_classic should now only use the libflashrom interface. BUG=b:208132085 TEST=`make && meson/ninja`. Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M Makefile M meson.build 2 files changed, 2 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/60074/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/60074 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161 Gerrit-Change-Number: 60074 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom: Drop read_flash_to_file() usage
by Edward O'Callaghan (Code Review) 28 Dec '21

28 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 ) Change subject: flashrom: Drop read_flash_to_file() usage ...................................................................... Patch Set 7: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/59291/comment/170c4ccf_02c36805 PS4, Line 635: if (map_flash(&flashes[0]) != 0) { : free(flashes[0].chip); : ret = 1; : goto out_shutdown; : } : msg_cinfo("Please note that forced reads most likely contain garbage.\n"); : ret = do_read(&flashes[0], filename); : unmap_flash(&flashes[0]); > > That is a good point, I removed them. […] 100% makes sense, I put the easy part out into https://review.coreboot.org/c/flashrom/+/60430 and kept this hunk in this change-id so the discussion thread doesn't get mixed up. Good call on the PWM, will try to jig something up at home outside the lab to see if I can make that happen. -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 7 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 29 Dec 2021 00:04:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Edward O'Callaghan (Code Review) 28 Dec '21

28 Dec '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/60430 ) Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... flashrom: Convert do_read() into a libflashrom user Aspire towards a goal of making cli_classic more of just a user of libflashrom than having quasi-parallel paths in flashrom.c This converts the do_read() provider wrapper into a pure libflashrom user. BUG=b:208132085 TEST=`$ sudo ./flashrom -p internal -r /tmp/bios.bin` Change-Id: Id2addadb891c482ee3f69da806062d7a88776675 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 17 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/60430/1 diff --git a/flashrom.c b/flashrom.c index 48d953b..445db30 100644 --- a/flashrom.c +++ b/flashrom.c @@ -2193,13 +2193,27 @@ int do_read(struct flashctx *const flash, const char *const filename) { - if (prepare_flash_access(flash, true, false, false, false)) + int ret; + + unsigned long size = flash->chip->total_size * 1024; + unsigned char *buf = calloc(size, sizeof(unsigned char)); + if (!buf) { + msg_gerr("Memory allocation failed!\n"); return 1; + } - const int ret = read_flash_to_file(flash, filename); + ret = flashrom_image_read(flash, buf, size); + if (ret > 0) + goto free_out; - finalize_flash_access(flash); + if (write_buf_to_include_args(flash, buf)) { + ret = 1; + goto free_out; + } + ret = write_buf_to_file(buf, size, filename); +free_out: + free(buf); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/60430 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id2addadb891c482ee3f69da806062d7a88776675 Gerrit-Change-Number: 60430 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashrom: Drop read_flash_to_file() usage
by Edward O'Callaghan (Code Review) 28 Dec '21

28 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Hello build bot (Jenkins), Daniel Campello, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59291 to look at the new patch set (#7). Change subject: flashrom: Drop read_flash_to_file() usage ...................................................................... flashrom: Drop read_flash_to_file() usage Aspire towards a goal of making cli_classic more of just a user of libflashrom than having quasi-parallel paths in flashrom.c This converts remaining read_flash_to_file() usage to the do_read() provider wrapper around libflashrom. BUG=b:208132085 TEST=<TODO> Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c M flash.h M flashrom.c 3 files changed, 2 insertions(+), 42 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/59291/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 7 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom.c: Move do_*() helpers into cli_classic.c
by Edward O'Callaghan (Code Review) 28 Dec '21

28 Dec '21

Attention is currently required from: Anastasia Klimchuk. Hello build bot (Jenkins), Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/60070 to look at the new patch set (#3). Change subject: flashrom.c: Move do_*() helpers into cli_classic.c ...................................................................... flashrom.c: Move do_*() helpers into cli_classic.c These helpers are only used by the CLI logic and so we localise them here to move towards cli_classic being a pure libflashrom user. BUG=b:208132085 TEST=`make` Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c M flash.h M flashrom.c 3 files changed, 96 insertions(+), 100 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/60070/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/60070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433 Gerrit-Change-Number: 60070 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Nico Huber (Code Review) 28 Dec '21

28 Dec '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59291 ) Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... Patch Set 6: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/59291/comment/7fca2f0f_8765753e PS4, Line 635: if (map_flash(&flashes[0]) != 0) { : free(flashes[0].chip); : ret = 1; : goto out_shutdown; : } : msg_cinfo("Please note that forced reads most likely contain garbage.\n"); : ret = do_read(&flashes[0], filename); : unmap_flash(&flashes[0]); > That is a good point, I removed them. Luckily force should by-pass the safety checks called from prepare_flash_access() WDYT? I would just put it into its own commit for now. If you want to refactor this CLI code that's not a bad idea, I stumbled over this path myself many times. But it seems rather orthogonal to the do_it() conversion. Also, the forced-read path should be easy to test, e.g. put a PWM signal on MISO and try to read that. Or start by testing it without any signal at all, to see if the control path still works. -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 28 Dec 2021 14:06:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom: Convert do_read() into a libflashrom user
by Edward O'Callaghan (Code Review) 27 Dec '21

27 Dec '21

Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello, Anastasia Klimchuk. Hello build bot (Jenkins), Daniel Campello, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59291 to look at the new patch set (#6). Change subject: flashrom: Convert do_read() into a libflashrom user ...................................................................... flashrom: Convert do_read() into a libflashrom user Aspire towards a goal of making cli_classic more of just a user of libflashrom than having quasi-parallel paths in flashrom.c This converts the do_read() provider wrapper into a pure libflashrom user. BUG=b:208132085 TEST=`$ sudo ./flashrom -p internal -r /tmp/bios.bin` Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c M flash.h M flashrom.c 3 files changed, 19 insertions(+), 45 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/59291/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/59291 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73 Gerrit-Change-Number: 59291 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: SFDP: drop static table length check to make newer SFDP revisions work
by Michael Niewöhner (Code Review) 27 Dec '21

27 Dec '21

Attention is currently required from: Edward O'Callaghan, Felix Singer, Nico Huber, Angel Pons, Anastasia Klimchuk. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60055 ) Change subject: SFDP: drop static table length check to make newer SFDP revisions work ...................................................................... Patch Set 2: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/60055/comment/962afb88_c137d855 PS1, Line 28: ***RFC*** We could maintain a list of revisions/lengths but is it really : worth it? : > IMHO, if the SFDP specs *mandate* a specific length, we could check for it. However, if that is not the case, I think it won't be worth it in the long run. It does indirectly by specifying the mandatory table entries count. However, for the optional ones the length apparently is table type specific. Revisions, table lengths: ``` pre-JESD216 (Intel): 4 DWORDs (16b) JESD216 (r1.0): 9 DWORDs (36b) JESD216A (r1.5 [sic!]): 16 DWORDS (64b) JESD216B (r1.6): same len but definition of prev. rsv bits JESD216C (r1.7**): 20 DWORDs (80b) JESD216D (r1.8**): no table changes JESD216D.01 (r1.8**): no table changes JESD216E (r1.9**): no table changes JESD216F (r1.A): 23 DWORDs (92b) ``` ** I couldn't find JESD216C,D,D.01,E anywhere, so r1.7-r1.9 are just educated guesses based on some mentions in the document history > We still need to check the revision number to properly parse the data (i.e. only parse newer fields if the revision is high enough). Currently, we don't really need since we don't need any of the new fields. That may change, though. We could do the check where some higher field is being read, then. Commit Message: https://review.coreboot.org/c/flashrom/+/60055/comment/f6f64602_5ccf18f3 PS2, Line 9: JESD216F (1.6) released in September 2021 : adds three new DWORDs JESD216A r1.5: adds 5 new DWORDs. ... later revisions add even more... so any chip with SFDP >= r1.5 is broken currently https://review.coreboot.org/c/flashrom/+/60055/comment/64a31d91_95235cd3 PS2, Line 11: 32 9*4=36. https://review.coreboot.org/c/flashrom/+/60055/comment/49387239_881d16a6 PS2, Line 24: let's add the output here ``` Probing for Unknown SFDP-capable chip, 0 kB: SFDP revision = 1.0 SFDP number of parameter headers is 2 (NPH = 1). SFDP parameter table header 0/1: ID 0x00, version 1.0 Length 36 B, Parameter Table Pointer 0x000030 SFDP parameter table header 1/1: ID 0xc8, version 1.0 Length 12 B, Parameter Table Pointer 0x000060 ``` ``` Probing for Unknown SFDP-capable chip, 0 kB: SFDP revision = 1.6 SFDP number of parameter headers is 2 (NPH = 1). SFDP parameter table header 0/1: ID 0x00, version 1.6 Length 64 B, Parameter Table Pointer 0x000030 SFDP parameter table header 1/1: ID 0xc8, version 1.0 Length 12 B, Parameter Table Pointer 0x000090 ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/60055 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id84cde4ebc805d68e2984e8041fbc48d7ceebe34 Gerrit-Change-Number: 60055 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <edward.ocallaghan(a)koparo.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 27 Dec 2021 19:49:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2021