January 2023 - flashrom-gerrit

[S] Change in flashrom[master]: programmer.h: Guard against sending spi commands on non-spi mst
by Edward O'Callaghan (Code Review) Jan. 8, 2023

Jan. 8, 2023

Attention is currently required from: Sam McNally, Felix Singer, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 ) Change subject: programmer.h: Guard against sending spi commands on non-spi mst ...................................................................... Patch Set 2: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71269/comment/08fd139d_ef244c11 PS1, Line 1937: spi_master_no_4ba_modes > I feel like the code needs a bit of work.. […] Sam, https://review.coreboot.org/c/flashrom/+/71742 -- To view, visit https://review.coreboot.org/c/flashrom/+/71269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b Gerrit-Change-Number: 71269 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 08 Jan 2023 01:42:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Rewrite prepare_chip_access() 4BA enablement
by Edward O'Callaghan (Code Review) Jan. 8, 2023

Jan. 8, 2023

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71742 ) Change subject: flashrom.c: Rewrite prepare_chip_access() 4BA enablement ...................................................................... flashrom.c: Rewrite prepare_chip_access() 4BA enablement Factor out the 4BA enablement logic into its own function and break down predicates into a series of easy to follow base-cases complete with comments. TEST=`$ sudo ./flashrom -p internal -r /tmp/bios`. on a Found chipset "Intel Tiger Lake U Premium". Change-Id: I10a426331cb2a3485e3dd786fa771e20870cbd84 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 57 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/71742/1 diff --git a/flashrom.c b/flashrom.c index 822594b..f7499c3 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1905,6 +1905,45 @@ return 0; } +static int enable_4ba_if_required(struct flashctx *const flash) +{ + /* Check if chip too small to need 4BA. */ + if (flash->chip->total_size <= 16 * 1024) { + return 0; + } + + /* Check if SPI bus with 4BA enabled has a SPI master missing 4BA mode. */ + if (!spi_master_no_4ba_modes(flash)) { + return 0; + } + + /* Check if SPI bus with 4BA has a SPI master that supports a 4BA mode. */ + if (!spi_master_4ba(flash)) { + msg_cerr("Programmer doesn't support this chip. Aborting.\n"); + return 1; + } + + /* Check chip supports native 4BA instructions and if not bail out. */ + if ((flash->chip->feature_bits & FEATURE_4BA_NATIVE) != FEATURE_4BA_NATIVE) { + msg_cerr("Chip does not support 4BA Native instructions. Aborting.\n"); + return 1; + } + + /* Check if chip is on a chipbus that supports 4BA addressing mode. */ + if (!spi_chip_4ba(flash)) { + return 0; + } + + /* Enable/disable 4-byte addressing mode. */ + int ret = spi_master_4ba(flash) ? spi_enter_4ba(flash): spi_exit_4ba(flash); + if (ret) { + msg_cerr("Failed to set correct 4BA mode! Aborting.\n"); + return 1; + } + + return 0; +} + int prepare_flash_access(struct flashctx *const flash, const bool read_it, const bool write_it, const bool erase_it, const bool verify_it) @@ -1933,30 +1972,7 @@ flash->address_high_byte = -1; flash->in_4ba_mode = false; - /* Be careful about 4BA chips and broken masters */ - if (flash->chip->total_size > 16 * 1024 && spi_master_no_4ba_modes(flash)) { - /* If we can't use native instructions, bail out */ - if ((flash->chip->feature_bits & FEATURE_4BA_NATIVE) != FEATURE_4BA_NATIVE - || !spi_master_4ba(flash)) { - msg_cerr("Programmer doesn't support this chip. Aborting.\n"); - return 1; - } - } - - /* Enable/disable 4-byte addressing mode if flash chip supports it */ - if (spi_chip_4ba(flash)) { - int ret; - if (spi_master_4ba(flash)) - ret = spi_enter_4ba(flash); - else - ret = spi_exit_4ba(flash); - if (ret) { - msg_cerr("Failed to set correct 4BA mode! Aborting.\n"); - return 1; - } - } - - return 0; + return enable_4ba_if_required(flash); } void finalize_flash_access(struct flashctx *const flash) -- To view, visit https://review.coreboot.org/c/flashrom/+/71742 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I10a426331cb2a3485e3dd786fa771e20870cbd84 Gerrit-Change-Number: 71742 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: programmer.h: Guard against sending spi commands on non-spi mst
by Edward O'Callaghan (Code Review) Jan. 7, 2023

Jan. 7, 2023

Attention is currently required from: Sam McNally, Felix Singer, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 ) Change subject: programmer.h: Guard against sending spi commands on non-spi mst ...................................................................... Patch Set 2: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71269/comment/3751906d_36a73bbc PS1, Line 1937: spi_master_no_4ba_modes > Could we skip this entire section on 4ba for non-SPI masters? I feel like the code needs a bit of work.. Can we just fix the immediate issue first (>16MB flashing is broken on ichspi). -- To view, visit https://review.coreboot.org/c/flashrom/+/71269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b Gerrit-Change-Number: 71269 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 07 Jan 2023 09:16:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashchips.c: Add support for IS25WQ040
by Vasily Galkin (Code Review) Jan. 7, 2023

Jan. 7, 2023

Attention is currently required from: Felix Singer, Vasily Galkin, Thomas Heijligen, roman.stingler(a)gmail.com. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, roman.stingler(a)gmail.com, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58179 to look at the new patch set (#5). Change subject: flashchips.c: Add support for IS25WQ040 ...................................................................... flashchips.c: Add support for IS25WQ040 Based on https://github.com/flashrom/flashrom/pull/204 squashed with fixes of IS25WQ040 size: it is 4Mbits, not 4MBytes, see https://www.issi.com/WW/pdf/25WQ020-040.pdf Tested read, write and erase with ft2232_spi-based "Tigard" programmer. Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Signed-off-by: Roman Stingler <roman.stingler(a)gmail.com> Signed-off-by: Vasily Galkin <galkin-vv(a)ya.ru> --- M flashchips.c M include/flashchips.h 2 files changed, 60 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/58179/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/58179 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Gerrit-Change-Number: 58179 Gerrit-PatchSet: 5 Gerrit-Owner: Vasily Galkin Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: roman.stingler(a)gmail.com Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Vasily Galkin Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: roman.stingler(a)gmail.com Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashchips.c: Add support for IS25WQ040
by Vasily Galkin (Code Review) Jan. 7, 2023

Jan. 7, 2023

Attention is currently required from: Felix Singer, Thomas Heijligen, roman.stingler(a)gmail.com. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, roman.stingler(a)gmail.com, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58179 to look at the new patch set (#4). Change subject: flashchips.c: Add support for IS25WQ040 ...................................................................... flashchips.c: Add support for IS25WQ040 Based on https://github.com/flashrom/flashrom/pull/204 squashed with fixes of IS25WQ040 size: it is 4Mbits, not 4MBytes, see https://www.issi.com/WW/pdf/25WQ020-040.pdf Tested read, write and erase with ft2232_spi-based "Tigard" programmer. Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Signed-off-by: Roman Stingler <roman.stingler(a)gmail.com> Signed-off-by: Vasily Galkin <galkin-vv(a)ya.ru> --- M flashchips.c M include/flashchips.h 2 files changed, 60 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/58179/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/58179 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Gerrit-Change-Number: 58179 Gerrit-PatchSet: 4 Gerrit-Owner: Vasily Galkin Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: roman.stingler(a)gmail.com Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: roman.stingler(a)gmail.com Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: chipset_enable.c: Add TL UP3 and UP4/Y id's
by Sam McNally (Code Review) Jan. 6, 2023

Jan. 6, 2023

Attention is currently required from: Edward O'Callaghan, Angel Pons. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71576 ) Change subject: chipset_enable.c: Add TL UP3 and UP4/Y id's ...................................................................... Patch Set 1: (1 comment) File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/71576/comment/637d65cb_688eb081 PS1, Line 2083: {0x8086, 0xa083, B_S, DEP, "Intel", "Tiger Lake U Premium 3", enable_flash_pch500}, > Let's keep it, but mark it as untested (change `DEP` to `NT`). Sure; going off the coreboot #define, I think this should be "Tiger Lake Base UP4" or "Tiger Lake U Base", depending on the naming scheme used for the others.. -- To view, visit https://review.coreboot.org/c/flashrom/+/71576 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I77120612f7a770ae1319f4cd82913fa465f355f5 Gerrit-Change-Number: 71576 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 06 Jan 2023 10:50:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: programmer.h: Guard against sending spi commands on non-spi mst
by Sam McNally (Code Review) Jan. 6, 2023

Jan. 6, 2023

Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 ) Change subject: programmer.h: Guard against sending spi commands on non-spi mst ...................................................................... Patch Set 2: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71269/comment/d2310077_4c7d1571 PS1, Line 1937: spi_master_no_4ba_modes > sort of but not really. This is a chip bustype check not a master bustype check. […] Could we skip this entire section on 4ba for non-SPI masters? -- To view, visit https://review.coreboot.org/c/flashrom/+/71269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b Gerrit-Change-Number: 71269 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 06 Jan 2023 10:48:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests/: Add subregion alignment unit-test
by Evan Benn (Code Review) Jan. 6, 2023

Jan. 6, 2023

Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71659 ) Change subject: tests/: Add subregion alignment unit-test ...................................................................... Patch Set 2: (17 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/71659/comment/a709f11e_40075213 PS2, Line 7: tests/: Add subregion alignment unit-test is a subregion a thing? maybe it is not the alignment but the size relative to erase sizes thats important? https://review.coreboot.org/c/flashrom/+/71659/comment/7fba71d2_252033e7 PS2, Line 9: A written region that is sized below that of the erasure granularity I don't understand the bug from this description File tests/chip.c: https://review.coreboot.org/c/flashrom/+/71659/comment/5586f951_4838fef1 PS2, Line 110: va_list logfile_args; you dont need to va_copy because you only pass ap to vfprintf https://review.coreboot.org/c/flashrom/+/71659/comment/bc8150ef_1cbe26a8 PS2, Line 114: output_type = stderr; I don't know the motivation for printing the messages, so I also dont know the motivation for splitting them into two different streams, curious to learn though https://review.coreboot.org/c/flashrom/+/71659/comment/1e0e9263_e6658fa8 PS2, Line 116: if (level <= verbose_screen) { verbose_screen is part of `cli_output.c`, maybe something else here? https://review.coreboot.org/c/flashrom/+/71659/comment/fb83b4b7_1e1024df PS2, Line 118: /* msg_*spew often happens inside chip accessors in possibly why do we need to fflush at all? https://review.coreboot.org/c/flashrom/+/71659/comment/4a7a5156_58a84076 PS2, Line 444: void write_chip_subregion_with_dummyflasher_test_success(void **state) // XXX this is the first mention of a 'subregion' in flashrom, maybe there is an existing word you can choose, but I think its just a 'region'. maybe the test is 'write_region_smaller_than_erase_granularity' https://review.coreboot.org/c/flashrom/+/71659/comment/942e87bc_b03b1f47 PS2, Line 458: struct flashchip mock_chip = chip_W25Q128_V; do you want to assert something about the size of the erasers here? as much for documentation as ensuring it doesnt get modified accidently https://review.coreboot.org/c/flashrom/+/71659/comment/389cfb5d_b58bb732 PS2, Line 466: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC this can be a const int instead of a define https://review.coreboot.org/c/flashrom/+/71659/comment/005ad273_3e93ca71 PS2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}. im not sure what this is telling me https://review.coreboot.org/c/flashrom/+/71659/comment/45070264_b17b9b3a PS2, Line 471: uint8_t *newcontents = calloc(1, mock_chip_size); memsetting so calloc could be malloc https://review.coreboot.org/c/flashrom/+/71659/comment/49603d7c_332879b5 PS2, Line 477: flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, false); suspect these flags are important what is being tested, but its not documented here or https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/thi… or https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/thi… so i dont actually know what this does https://review.coreboot.org/c/flashrom/+/71659/comment/56f20945_29dc8e05 PS2, Line 478: flashrom_set_log_callback((flashrom_log_callback *)&unittest_print_cb); why are we setting up a print callback? We also dont seem to undo this at the end, which will be tricky because the macros return early. maybe this should go at the unit test top level https://review.coreboot.org/c/flashrom/+/71659/comment/d91320c0_1f4bb962 PS2, Line 485: flashrom_layout_release(layout); flashctx->layout is dangling. could set it to default_layout or NULL, which seem to do the same thing but neither way is documented https://review.coreboot.org/c/flashrom/+/71659/comment/2fe11234_1c758365 PS2, Line 502: printf("Subregion chip W op..\n"); write the word https://review.coreboot.org/c/flashrom/+/71659/comment/ac67e596_0ffdf83e PS2, Line 508: //flashrom_layout_set(&flashctx, NULL); // use default layout. the layout you use is the same as the default or NULL layout, does that not work? https://review.coreboot.org/c/flashrom/+/71659/comment/f11e3614_95d8a45d PS2, Line 514: assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, mock_chip_size)); maybe documenting right here what the predicted bug is would be useful. does the test fail here or at the write step? Does this test catch a real bug on some patch? I would like to have a look. -- To view, visit https://review.coreboot.org/c/flashrom/+/71659 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Gerrit-Change-Number: 71659 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 06 Jan 2023 03:35:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests/: Add subregion alignment unit-test
by Edward O'Callaghan (Code Review) Jan. 5, 2023

Jan. 5, 2023

Attention is currently required from: Sam McNally, Anastasia Klimchuk. Hello Sam McNally, build bot (Jenkins), Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71659 to look at the new patch set (#2). Change subject: tests/: Add subregion alignment unit-test ...................................................................... tests/: Add subregion alignment unit-test A written region that is sized below that of the erasure granularity can result in a incorrectly read region that does not include prior content within the region before the write op. This was dealt with in ChromeOS downstream by expanding out the read to match the erase granularity however does not seem to impact upstream. Add a unit-test to avoid regression as this is important behaviour to cover. Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M tests/chip.c M tests/tests.c M tests/tests.h 3 files changed, 120 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/71659/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/71659 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Gerrit-Change-Number: 71659 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashrom.c: squash subregion fix
by Edward O'Callaghan (Code Review) Jan. 5, 2023

Jan. 5, 2023

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71680 ) Change subject: flashrom.c: squash subregion fix ...................................................................... flashrom.c: squash subregion fix Change-Id: I0cce707ed88f6c4b600ba76926bb9e60b93e066b Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 82 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/80/71680/1 diff --git a/flashrom.c b/flashrom.c index 62f38f8..6f5d54c 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1187,6 +1187,65 @@ return chip - flashchips; } +/* + * Gets the lowest erase granularity; it is used when + * deciding if the layout map needs to be adjusted such that erase boundaries + * match this granularity. Returns -1 if unsuccessful. + */ +static int get_required_erase_size(struct flashctx *flash) +{ + int i, erase_size_found = 0; + unsigned int required_erase_size; + /* + * Find eraseable block size for read alignment. + * FIXME: This assumes the smallest block erase size is useable + * by erase_and_write_flash(). + */ + required_erase_size = ~0; + for (i = 0; i < NUM_ERASEFUNCTIONS; i++) { + struct block_eraser eraser = flash->chip->block_erasers[i]; + int j; + for (j = 0; j < NUM_ERASEREGIONS; j++) { + unsigned int size = eraser.eraseblocks[j].size; + if (size && (size < required_erase_size)) { + required_erase_size = size; + erase_size_found = 1; + } + } + } + /* likely an error in flashchips[] */ + if (!erase_size_found) { + msg_cerr("%s: No usable erase size found.\n", __func__); + return -1; + } + return required_erase_size; +} +static int round_to_erasable_block_boundary(const int required_erase_size, + const struct romentry *entry, + chipoff_t *rounded_start, + chipsize_t* rounded_len) { + unsigned int start_align, len_align; + const struct flash_region *region = &entry->region; + if (required_erase_size < 0) + return 1; + /* round down to nearest eraseable block boundary */ + start_align = region->start % required_erase_size; + *rounded_start = region->start - start_align; + /* round up to nearest eraseable block boundary */ + *rounded_len = region->end - *rounded_start + 1; + len_align = *rounded_len % required_erase_size; + if (len_align) + *rounded_len = *rounded_len + required_erase_size - len_align; + if (start_align || len_align) { + msg_gdbg("\n%s: Re-aligned partial read due to eraseable " + "block size requirement:\n\tstart: 0x%06x, " + "len: 0x%06x, aligned start: 0x%06x, len: 0x%06x\n", + __func__, region->start, region->end - region->start + 1, + *rounded_start, *rounded_len); + } + return 0; +} + /** * @brief Reads the included layout regions into a buffer. * @@ -1198,15 +1257,22 @@ * @return 0 on success, * 1 if any read fails. */ -static int read_by_layout(struct flashctx *const flashctx, uint8_t *const buffer) +static int read_by_layout(struct flashctx *const flashctx, uint8_t *const buffer, + bool align_to_erasable_block_boundary) { const struct flashrom_layout *const layout = get_layout(flashctx); const struct romentry *entry = NULL; + const int required_erase_size = get_required_erase_size(flashctx); while ((entry = layout_next_included(layout, entry))) { const struct flash_region *region = &entry->region; - const chipoff_t region_start = region->start; - const chipsize_t region_len = region->end - region->start + 1; + chipoff_t region_start = region->start; + chipsize_t region_len = region->end - region->start + 1; + + if (align_to_erasable_block_boundary && + round_to_erasable_block_boundary(required_erase_size, entry, + &region_start, &region_len)) + return 1; if (read_flash(flashctx, buffer + region_start, region_start, region_len)) return 1; @@ -1386,7 +1452,7 @@ if (info->curcontents) { msg_cinfo("Reading current flash chip contents... "); - if (read_by_layout(flashctx, info->curcontents)) { + if (read_by_layout(flashctx, info->curcontents, true)) { /* Now we are truly screwed. Read failed as well. */ msg_cerr("Can't read anymore! Aborting.\n"); /* We have no idea about the flash chip contents, so @@ -1990,7 +2056,7 @@ msg_cinfo("Reading flash... "); int ret = 1; - if (read_by_layout(flashctx, buffer)) { + if (read_by_layout(flashctx, buffer, false)) { msg_cerr("Read operation failed!\n"); msg_cinfo("FAILED.\n"); goto _finalize_ret; @@ -2089,7 +2155,7 @@ } memcpy(curcontents, oldcontents, flash_size); } else { - if (read_by_layout(flashctx, curcontents)) { + if (read_by_layout(flashctx, curcontents, true)) { msg_cinfo("FAILED.\n"); goto _finalize_ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/71680 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0cce707ed88f6c4b600ba76926bb9e60b93e066b Gerrit-Change-Number: 71680 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0