September 2024 - flashrom-gerrit

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) Sept. 12, 2024

Sept. 12, 2024

Attention is currently required from: Miklós Márton, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Patch Set 5: (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/84203/comment/7908cf76_19f413eb?us… : PS3, Line 487: for (int i = 0; opcode[i]; i++) { > > I would say "chips that don't have erase operation capability, and no erase opcodes"? […] Thank you so much for explanations! It's so useful. I made a first attempt to add a comment for FEATURE_NO_ERASE in the header include/flash.h. But please tell me if it's a correct comment. File flashrom.c: https://review.coreboot.org/c/flashrom/+/84203/comment/22c7d77c_7399555d?us… : PS4, Line 485: !(flash->chip->feature_bits & FEATURE_NO_ERASE) > I don't think you need this check, and for a chip that doesn't need erase but supports a fast erase […] Yep, removed the check. I wasn't entirely sure which way to go, so I left it in the previous patchset (but now removed). Checking opcode for null is enough. -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Sep 2024 03:34:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) Sept. 12, 2024

Sept. 12, 2024

Attention is currently required from: Anastasia Klimchuk, Miklós Márton. Hello Miklós Márton, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84203?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Fix FEATURE_NO_ERASE chips and add test for them New check was added to `check_block_eraser` in commit 0f389aea9e630c3b21547a5dd8dbe572a8502853 but it was not handling FEATURE_NO_ERASE chips. This patch fixes processing such chips and adds test to run write and verify with dummyflasher for FEATURE_NO_ERASE chips. Ticket: https://ticket.coreboot.org/issues/553 Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M flashrom.c M include/flash.h M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 76 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/84203/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Peter Marheine (Code Review) Sept. 11, 2024

Sept. 11, 2024

Attention is currently required from: Anastasia Klimchuk, Miklós Márton. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Patch Set 4: (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/84203/comment/369f0e75_d38fa01e?us… : PS3, Line 487: for (int i = 0; opcode[i]; i++) { > I would say "chips that don't have erase operation capability, and no erase opcodes"? I don't think it's correct to say that, but we kind of need to *decide* what meaning this has because is only has a few users and none of them are currently well-documented. There are currently only two cases where `FEATURE_NO_ERASE` can be enabled: * `linux_mtd` enables it if the kernel reports `MTD_NO_ERASE`, which is described by a comment as "no erase necessary" * M95M02 EEPROM has it enabled statically For MTD devices, the description seems fairly clear that there may be erase functionality but it doesn't need to be erased before writing. M92M02 doesn't have an erase command, but that's common for EEPROMs because there's usually on reason to do bulk erases. I [found one person claiming some EEPROMs have erase commands](https://forums.anandtech.com/threads/erasing-an-eeprom-chip.13271… but no specific part numbers aside from built-in EEPROMs in microcontrollers which often do have dedicated erase commands. Given these, I think we need to use the same interpretation as Linux does, and make `FEATURE_NO_ERASE` indicate that erase is not required before writing but can still be done (and on some chips erasing may be faster than writing an "erased" value). File flashrom.c: https://review.coreboot.org/c/flashrom/+/84203/comment/00c840ed_6c823244?us… : PS4, Line 485: !(flash->chip->feature_bits & FEATURE_NO_ERASE) I don't think you need this check, and for a chip that doesn't need erase but supports a fast erase it would be undesirable. -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 11 Sep 2024 01:44:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) Sept. 10, 2024

Sept. 10, 2024

Attention is currently required from: Miklós Márton, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Patch Set 4: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/84203/comment/db3d63e0_6e7fe218?us… : PS3, Line 487: for (int i = 0; opcode[i]; i++) { > This will always crash if spi_get_opcode_from_erasefn doesn't know the eraser, which should probably be fixed to gracefully bail out. 100% yes, I added a check for null. There are 22 opcodes known to `spi_get_opcode_from_erasefn` and 34 erase functions known to `lookup_erase_func_ptr` ! oh :( yes, we need to check for null. About `FEATURE_NO_ERASE`, I thought those are chips that don't have any erase opcodes, and they just don't support erase operation at all - whether it's erase-before-write or just erase, doesn't matter. The way the feature is implemented is that when such a chip is asked to be erased, the special `SPI_BLOCK_ERASE_EMULATION` is quietly doing write instead of erase, and the end result for the chip memory is the same. So, I added a check for null, but haven't added any comment to FEATURE_NO_ERASE yet. If I would be adding a comment, I would say "chips that don't have erase operation capability, and no erase opcodes"? -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 10 Sep 2024 11:44:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) Sept. 10, 2024

Sept. 10, 2024

Attention is currently required from: Anastasia Klimchuk, Miklós Márton. Hello Miklós Márton, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84203?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Fix FEATURE_NO_ERASE chips and add test for them New check was added to `check_block_eraser` in commit 0f389aea9e630c3b21547a5dd8dbe572a8502853 but it was not handling FEATURE_NO_ERASE chips. This patch fixes processing such chips and adds test to run write and verify with dummyflasher for FEATURE_NO_ERASE chips. Ticket: https://ticket.coreboot.org/issues/553 Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M flashrom.c M tests/chip.c M tests/tests.c M tests/tests.h 4 files changed, 73 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/84203/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: erasure_layout: Fix init_eraseblock segmentation fault
by Anastasia Klimchuk (Code Review) Sept. 10, 2024

Sept. 10, 2024

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84234?usp=email ) Change subject: erasure_layout: Fix init_eraseblock segmentation fault ...................................................................... erasure_layout: Fix init_eraseblock segmentation fault Fix a segmentation fault that is caused by accessing an invalid "subedata" pointer on the last iteration of the init_eraseblock loop. Instead, short circuit the loop condition to check the sub block index first, and do not access the invalid pointer if it is the last sub block. Issue was encountered in: - OS: OpenBSD 7.5 amd64 - Compiler: clang 16.0.6 - Chip: Macronix MX25U6435E/F BUG=https://ticket.coreboot.org/issues/555 Change-Id: I61bf0d93aa9f0b2b420b146be16fcd5124f0dc5d Signed-off-by: Grant Pannell <grant(a)digitaldj.net> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84234 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: DigitalDJ Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M erasure_layout.c 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved DigitalDJ: Looks good to me, but someone else must approve diff --git a/erasure_layout.c b/erasure_layout.c index c1368e7..c3a415b 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -52,8 +52,8 @@ edata->first_sub_block_index = *sub_block_index; struct eraseblock_data *subedata = &layout[idx - 1].layout_list[*sub_block_index]; - while (subedata->start_addr >= start_addr && subedata->end_addr <= end_addr && - *sub_block_index < layout[idx-1].block_count) { + while (*sub_block_index < layout[idx-1].block_count && + subedata->start_addr >= start_addr && subedata->end_addr <= end_addr) { (*sub_block_index)++; subedata++; } -- To view, visit https://review.coreboot.org/c/flashrom/+/84234?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I61bf0d93aa9f0b2b420b146be16fcd5124f0dc5d Gerrit-Change-Number: 84234 Gerrit-PatchSet: 6 Gerrit-Owner: DigitalDJ Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: DigitalDJ Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in flashrom[main]: ichspi: Restore Sector erase opcode after send command
by Bill XIE (Code Review) Sept. 9, 2024

Sept. 9, 2024

Bill XIE has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email ) Change subject: ichspi: Restore Sector erase opcode after send command ...................................................................... ichspi: Restore Sector erase opcode after send command ich_spi_send_command() and ich_spi_send_multicommand() used to overwrite the "Sector erase" opcode with the opcode for command via reprogram_opcode_on_the_fly(), but not restore it, causing the "Sector erase" opcode may get lost after sending commands, leaving only "Bulk erase" opcode which erase the whole chip available. Now, restore_se_opcode() is introduced to restore the "Sector erase" opcode if it is overwritten, and ich_spi_send_command() and ich_spi_send_multicommand() will call it before returning. Fix:https://ticket.coreboot.org/issues/556 Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64 Signed-off-by: persmule <persmule(a)hardenedlinux.org> --- M ichspi.c 1 file changed, 27 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/84253/1 diff --git a/ichspi.c b/ichspi.c index d01f2f3..e4aabf0 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1140,10 +1140,18 @@ } } -static int ich_spi_send_command(const struct flashctx *flash, unsigned int writecnt, - unsigned int readcnt, - const unsigned char *writearr, - unsigned char *readarr) +/* Restore the "Sector erase" opcode if it has been overwritten */ +static void restore_se_opcode(void) +{ + if (find_opcode(curopcodes, JEDEC_SE) == -1) + reprogram_opcode_on_the_fly(JEDEC_SE, JEDEC_SE_OUTSIZE, JEDEC_SE_INSIZE); +} + +/* Internal operation to send command */ +static int _ich_spi_send_command(const struct flashctx *flash, unsigned int writecnt, + unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr) { int result; int opcode_index = -1; @@ -1269,6 +1277,17 @@ return result; } +static int ich_spi_send_command(const struct flashctx *flash, unsigned int writecnt, + unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr) +{ + int result = _ich_spi_send_command(flash, writecnt, readcnt, writearr, readarr); + /* Restore the "Sector erase" opcode before returning */ + restore_se_opcode(); + return result; +} + #define MAX_FD_REGIONS 16 struct fd_region { const char* name; @@ -1811,12 +1830,14 @@ * preoppos matched, this is a normal opcode. */ } - ret = ich_spi_send_command(flash, cmds->writecnt, cmds->readcnt, - cmds->writearr, cmds->readarr); + ret = _ich_spi_send_command(flash, cmds->writecnt, cmds->readcnt, + cmds->writearr, cmds->readarr); /* Reset the type of all opcodes to non-atomic. */ for (i = 0; i < 8; i++) curopcodes->opcode[i].atomic = 0; } + /* Restore the "Sector erase" opcode before returning */ + restore_se_opcode(); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/84253?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64 Gerrit-Change-Number: 84253 Gerrit-PatchSet: 1 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>

1 0

[S] Change in flashrom[main]: ichspi: Add RaptorPoint PCH support
by Anastasia Klimchuk (Code Review) Sept. 8, 2024

Sept. 8, 2024

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/83854?usp=email ) Change subject: ichspi: Add RaptorPoint PCH support ...................................................................... ichspi: Add RaptorPoint PCH support Based on public Intel 700 Series PCH datasheet, DOC 743835 rev 004. The IDs of IoT chipset SKUs (ending with E) can only be found in "12th Gen Intel® Core™ Processors Family (Formerly Known as Alder Lake -S) for IoT Platforms External Design Specification (EDS) Addendum" DOC 634528 rev 2.7 (NDA). TEST=Probe flash on Z790 chipset. Run the ich_descriptors_tool and check the output is correct as expected. Change-Id: I13ac52d5400c0e2260e12d605077fc2182c379ef Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/83854 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M chipset_enable.c M doc/release_notes/devel.rst M ich_descriptors.c M ichspi.c M include/programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 6 files changed, 45 insertions(+), 2 deletions(-) Approvals: Sergii Dmytruk: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified diff --git a/chipset_enable.c b/chipset_enable.c index 9f0a0bb..2adc425 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -606,6 +606,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_ELKHART_LAKE: @@ -714,6 +715,7 @@ break; case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_C740_SERIES_EMMITSBURG: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: @@ -751,6 +753,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_APOLLO_LAKE: @@ -1023,6 +1026,11 @@ return enable_flash_pch100_or_c620(cfg, dev, name, 0x1f, 5, CHIPSET_600_SERIES_ALDER_POINT); } +static int enable_flash_pch700(const struct programmer_cfg *cfg, struct pci_dev *const dev, const char *const name) +{ + return enable_flash_pch100_or_c620(cfg, dev, name, 0x1f, 5, CHIPSET_700_SERIES_RAPTOR_POINT); +} + static int enable_flash_mtl(const struct programmer_cfg *cfg, struct pci_dev *const dev, const char *const name) { return enable_flash_pch100_or_c620(cfg, dev, name, 0x1f, 5, CHIPSET_METEOR_LAKE); @@ -2196,9 +2204,19 @@ {0x8086, 0x7a83, B_S, NT, "Intel", "Q670", enable_flash_pch600}, {0x8086, 0x7a84, B_S, DEP, "Intel", "Z690", enable_flash_pch600}, {0x8086, 0x7a88, B_S, NT, "Intel", "W680", enable_flash_pch600}, - {0x8086, 0x7a8a, B_S, NT, "Intel", "W685", enable_flash_pch600}, {0x8086, 0x7a8d, B_S, NT, "Intel", "WM690", enable_flash_pch600}, {0x8086, 0x7a8c, B_S, NT, "Intel", "HM670", enable_flash_pch600}, + {0x8086, 0x7a90, B_S, NT, "Intel", "R680E", enable_flash_pch600}, + {0x8086, 0x7a91, B_S, NT, "Intel", "Q670E", enable_flash_pch600}, + {0x8086, 0x7a92, B_S, NT, "Intel", "H610E", enable_flash_pch600}, + {0x8086, 0x7a8a, B_S, NT, "Intel", "W790", enable_flash_pch700}, + {0x8086, 0x7a04, B_S, DEP, "Intel", "Z790", enable_flash_pch700}, + {0x8086, 0x7a05, B_S, NT, "Intel", "H770", enable_flash_pch700}, + {0x8086, 0x7a06, B_S, NT, "Intel", "B760", enable_flash_pch700}, + {0x8086, 0x7a0c, B_S, NT, "Intel", "HM770", enable_flash_pch700}, + {0x8086, 0x7a0d, B_S, NT, "Intel", "WM790", enable_flash_pch700}, + {0x8086, 0x7a14, B_S, NT, "Intel", "C262", enable_flash_pch700}, + {0x8086, 0x7a13, B_S, NT, "Intel", "C266", enable_flash_pch700}, {0x8086, 0x7e23, B_S, DEP, "Intel", "Meteor Lake-P/M", enable_flash_mtl}, {0x8086, 0xe323, B_S, DEP, "Intel", "Panther Lake-U/H 12Xe", enable_flash_ptl}, {0x8086, 0xe423, B_S, DEP, "Intel", "Panther Lake-H 4Xe", enable_flash_ptl}, diff --git a/doc/release_notes/devel.rst b/doc/release_notes/devel.rst index 5243f7c..919d364 100644 --- a/doc/release_notes/devel.rst +++ b/doc/release_notes/devel.rst @@ -31,3 +31,7 @@ The ECAM has been supported for a very long time, most platforms should support it. For those platforms don't support ECAM, libpci will terminate the process by exit. + +Chipset support +=============== +Added Raptor Point PCH support. diff --git a/ich_descriptors.c b/ich_descriptors.c index eaf44b0..c436fab 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -49,6 +49,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_ELKHART_LAKE: @@ -80,6 +81,7 @@ case CHIPSET_C740_SERIES_EMMITSBURG: case CHIPSET_APOLLO_LAKE: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_GEMINI_LAKE: @@ -221,6 +223,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_APOLLO_LAKE: @@ -320,6 +323,7 @@ return freq_str[2][value]; case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_C740_SERIES_EMMITSBURG: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: @@ -371,6 +375,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_APOLLO_LAKE: @@ -512,6 +517,7 @@ cs == CHIPSET_400_SERIES_COMET_POINT || cs == CHIPSET_500_SERIES_TIGER_POINT || cs == CHIPSET_600_SERIES_ALDER_POINT || + cs == CHIPSET_700_SERIES_RAPTOR_POINT || cs == CHIPSET_C740_SERIES_EMMITSBURG || cs == CHIPSET_JASPER_LAKE || cs == CHIPSET_METEOR_LAKE || @@ -1115,6 +1121,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_GEMINI_LAKE: @@ -1277,6 +1284,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_APOLLO_LAKE: @@ -1324,6 +1332,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_METEOR_LAKE: case CHIPSET_PANTHER_LAKE: case CHIPSET_APOLLO_LAKE: diff --git a/ichspi.c b/ichspi.c index f74fb05..d01f2f3 100644 --- a/ichspi.c +++ b/ichspi.c @@ -2106,6 +2106,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_JASPER_LAKE: @@ -2147,6 +2148,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_JASPER_LAKE: @@ -2210,6 +2212,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_JASPER_LAKE: @@ -2291,6 +2294,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_JASPER_LAKE: @@ -2332,6 +2336,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_500_SERIES_TIGER_POINT: case CHIPSET_600_SERIES_ALDER_POINT: + case CHIPSET_700_SERIES_RAPTOR_POINT: case CHIPSET_APOLLO_LAKE: case CHIPSET_GEMINI_LAKE: case CHIPSET_JASPER_LAKE: @@ -2371,6 +2376,7 @@ ich_gen == CHIPSET_400_SERIES_COMET_POINT || ich_gen == CHIPSET_500_SERIES_TIGER_POINT || ich_gen == CHIPSET_600_SERIES_ALDER_POINT || + ich_gen == CHIPSET_700_SERIES_RAPTOR_POINT || ich_gen == CHIPSET_C740_SERIES_EMMITSBURG)) { msg_pdbg("Enabling hardware sequencing by default for 100+ series PCH.\n"); ich_spi_mode = ich_hwseq; diff --git a/include/programmer.h b/include/programmer.h index 77e79ae..5ed9c8a 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -355,6 +355,7 @@ CHIPSET_400_SERIES_COMET_POINT, CHIPSET_500_SERIES_TIGER_POINT, CHIPSET_600_SERIES_ALDER_POINT, + CHIPSET_700_SERIES_RAPTOR_POINT, CHIPSET_APOLLO_LAKE, CHIPSET_GEMINI_LAKE, CHIPSET_JASPER_LAKE, diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index 09587f7..ec77a88 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -140,6 +140,7 @@ "\t- \"400\" or \"comet\" for Intel's 400 series chipsets.\n" "\t- \"500\" or \"tiger\" for Intel's 500 series chipsets.\n" "\t- \"600\" or \"alder\" for Intel's 600 series chipsets.\n" +"\t- \"700\" or \"raptor\" for Intel's 700 series chipsets.\n" "If '-d' is specified some regions such as the BIOS image as seen by the CPU or\n" "the GbE blob that is required to initialize the GbE are also dumped to files.\n", argv[0], argv[0]); @@ -237,8 +238,12 @@ else if ((strcmp(csn, "500") == 0) || (strcmp(csn, "tiger") == 0)) cs = CHIPSET_500_SERIES_TIGER_POINT; - else if (strcmp(csn, "600") == 0) + else if ((strcmp(csn, "600") == 0) || + (strcmp(csn, "alder") == 0)) cs = CHIPSET_600_SERIES_ALDER_POINT; + else if ((strcmp(csn, "700") == 0) || + (strcmp(csn, "raptor") == 0)) + cs = CHIPSET_700_SERIES_RAPTOR_POINT; else if (strcmp(csn, "apollo") == 0) cs = CHIPSET_APOLLO_LAKE; else if (strcmp(csn, "gemini") == 0) -- To view, visit https://review.coreboot.org/c/flashrom/+/83854?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I13ac52d5400c0e2260e12d605077fc2182c379ef Gerrit-Change-Number: 83854 Gerrit-PatchSet: 9 Gerrit-Owner: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Filip Gołaś <filip.golas(a)3mdeb.com> Gerrit-CC: Michał Kopeć <michal.kopec(a)3mdeb.com>

1 0

[M] Change in flashrom[main]: Ensure verify operation completed in full if chip memory modified
by Anastasia Klimchuk (Code Review) Sept. 8, 2024

Sept. 8, 2024

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84078?usp=email ) Change subject: Ensure verify operation completed in full if chip memory modified ...................................................................... Ensure verify operation completed in full if chip memory modified The patch adds new functionality to the test: tracking the areas of chip memory that were modified (i.e. by erase or write operation), and then checking those areas were completely covered by verify operation. The test operates over the mock chip memory of 16 bytes, so it is possible to track each byte which was modified, and assert that is has been verified afterwards. Adding the test found a bug which is fixed in this commit: Post-cleanup after processing unaligned region for the case when end region needs to be extended to align with erase block. Writing was done correctly, but post-processing of newcontents could cause one-off offset at the end of the region, which would make verification appear false-negative (see test cases #16-19). Change-Id: I3c5d55a0deb20f23f4072caac8c0dce04cc98fd4 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84078 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- M erasure_layout.c M tests/erase_func_algo.c 2 files changed, 138 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Peter Marheine: Looks good to me, approved diff --git a/erasure_layout.c b/erasure_layout.c index a7eaa2d..c1368e7 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -399,7 +399,7 @@ } if (old_end_buf) { - memcpy(newcontents + old_end, old_end_buf, end_buf_len); + memcpy(newcontents + old_end + 1, old_end_buf, end_buf_len); free(old_end_buf); } diff --git a/tests/erase_func_algo.c b/tests/erase_func_algo.c index 576923f..b2a09dc 100644 --- a/tests/erase_func_algo.c +++ b/tests/erase_func_algo.c @@ -57,6 +57,8 @@ struct all_state { uint8_t buf[MIN_REAL_CHIP_SIZE]; /* Buffer emulating the memory of the mock chip. */ + bool was_modified[MIN_REAL_CHIP_SIZE]; /* Which bytes were modified, 0x1 if byte was modified. */ + bool was_verified[MIN_REAL_CHIP_SIZE]; /* Which bytes were verified, 0x1 if byte was verified. */ struct erase_invoke eraseblocks_actual[MOCK_CHIP_SIZE]; /* The actual order of eraseblocks invocations. */ unsigned int eraseblocks_actual_ind; /* Actual number of eraseblocks invocations. */ const struct test_case* current_test_case; /* Currently executed test case. */ @@ -64,29 +66,46 @@ static int read_chip(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - if (start + len <= MOCK_CHIP_SIZE) + if (start < MOCK_CHIP_SIZE) LOG_READ_WRITE_FUNC; assert_in_range(start + len, 0, MIN_REAL_CHIP_SIZE); memcpy(buf, &g_state.buf[start], len); + + /* If these bytes were modified before => current read op is verify op, track it */ + bool bytes_modified = false; + for (unsigned int i = start; i < start + len; i++) + if (g_state.was_modified[i]) { + bytes_modified = true; + break; + } + if (bytes_modified) + memset(&g_state.was_verified[start], true, len); + return 0; } static int write_chip(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) { - if (start + len <= MOCK_CHIP_SIZE) + if (start < MOCK_CHIP_SIZE) LOG_READ_WRITE_FUNC; assert_in_range(start + len, 0, MIN_REAL_CHIP_SIZE); memcpy(&g_state.buf[start], buf, len); + + /* Track the bytes were written */ + memset(&g_state.was_modified[start], true, len); + /* Clear the records of previous verification, if there were any */ + memset(&g_state.was_verified[start], false, len); + return 0; } static int block_erase_chip_tagged(struct flashctx *flash, enum block_erase_func erase_func, unsigned int blockaddr, unsigned int blocklen) { - if (blockaddr + blocklen <= MOCK_CHIP_SIZE) { + if (blockaddr < MOCK_CHIP_SIZE) { LOG_ERASE_FUNC; /* Register eraseblock invocation. */ @@ -101,6 +120,12 @@ assert_in_range(blockaddr + blocklen, 0, MIN_REAL_CHIP_SIZE); memset(&g_state.buf[blockaddr], ERASE_VALUE, blocklen); + + /* Track the bytes were erased */ + memset(&g_state.was_modified[blockaddr], true, blocklen); + /* Clear the records of previous verification, if there were any */ + memset(&g_state.was_verified[blockaddr], false, blocklen); + return 0; } @@ -196,9 +221,15 @@ BLOCK_ERASE_FUNC(4) BLOCK_ERASE_FUNC(5) -static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, +/* + * Returns the offset how far we need to verify mock chip memory. + * Which is minimum out of MOCK_CHIP_SIZE and the end of the logical layout. + */ +static chipoff_t setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, const char *programmer_param, struct test_case *current_test_case) { + chipoff_t verify_end_boundary = MOCK_CHIP_SIZE - 1; + g_test_write_injector = write_chip; g_test_read_injector = read_chip; /* Each erasefunc corresponds to an operation that erases a block of @@ -223,6 +254,14 @@ memset(g_state.eraseblocks_actual, 0, MOCK_CHIP_SIZE * sizeof(struct erase_invoke)); g_state.eraseblocks_actual_ind = 0; + /* Clear the tracking of each byte modified. */ + memset(g_state.was_modified, false, MIN_REAL_CHIP_SIZE); + /* Clear the tracking of each byte verified. */ + memset(g_state.was_verified, false, MIN_REAL_CHIP_SIZE); + + /* Set the flag to verify after writing on chip */ + flashrom_flag_set(flashctx, FLASHROM_FLAG_VERIFY_AFTER_WRITE, true); + flashctx->chip = current_test_case->chip; printf("Creating layout ... "); @@ -236,6 +275,12 @@ current_test_case->regions[i].end, current_test_case->regions[i].name)); assert_int_equal(0, flashrom_layout_include_region(*layout, current_test_case->regions[i].name)); + + if (current_test_case->regions[i].end < MOCK_CHIP_SIZE - 1) + verify_end_boundary = current_test_case->regions[i].end; + else + verify_end_boundary = MOCK_CHIP_SIZE - 1; + i++; } @@ -252,6 +297,8 @@ /* Assignment below normally happens while probing, but this test is not probing. */ flashctx->mst = &registered_masters[0]; printf("done\n"); + + return verify_end_boundary; } static void teardown_chip(struct flashrom_layout **layout) @@ -771,7 +818,7 @@ /* * Setup all test cases with protected region. - * Protected region is the same for all test cases, between bytes 8 - 15. + * Protected region is the same for all test cases, between bytes START_PROTECTED_REGION and up to END_PROTECTED_REGION. */ static struct test_case test_cases_protected_region[] = { { @@ -1045,7 +1092,7 @@ struct flashrom_layout *layout; - setup_chip(&flashctx, &layout, param, current_test_case); + const chipoff_t verify_end_boundary = setup_chip(&flashctx, &layout, param, current_test_case); printf("%s started.\n", current_test_case->erase_test_name); int ret = flashrom_flash_erase(&flashctx); @@ -1060,6 +1107,13 @@ int eraseblocks_invocations = (g_state.eraseblocks_actual_ind == current_test_case->eraseblocks_expected_ind); + int chip_verified = 1; + for (unsigned int i = 0; i <= verify_end_boundary; i++) + if (g_state.was_modified[i] && !g_state.was_verified[i]) { + chip_verified = 0; /* byte was modified, but not verified after */ + printf("Error: byte 0x%x, modified: %d, verified: %d\n", i, g_state.was_modified[i], g_state.was_verified[i]); + } + if (chip_erased) printf("Erased chip memory state for %s is CORRECT\n", current_test_case->erase_test_name); @@ -1083,10 +1137,18 @@ current_test_case->eraseblocks_expected_ind, g_state.eraseblocks_actual_ind); + if (chip_verified) + printf("Erased chip memory state for %s was verified successfully\n", + current_test_case->erase_test_name); + else + printf("Erased chip memory state for %s was NOT verified completely\n", + current_test_case->erase_test_name); + all_erase_tests_result |= ret; all_erase_tests_result |= !chip_erased; all_erase_tests_result |= !eraseblocks_in_order; all_erase_tests_result |= !eraseblocks_invocations; + all_erase_tests_result |= !chip_verified; teardown_chip(&layout); @@ -1108,7 +1170,7 @@ struct flashrom_layout *layout; - setup_chip(&flashctx, &layout, param, current_test_case); + const chipoff_t verify_end_boundary = setup_chip(&flashctx, &layout, param, current_test_case); memcpy(&newcontents, current_test_case->written_buf, MOCK_CHIP_SIZE); printf("%s started.\n", current_test_case->write_test_name); @@ -1117,6 +1179,13 @@ int chip_written = !memcmp(g_state.buf, current_test_case->written_buf, MOCK_CHIP_SIZE); + int chip_verified = 1; + for (unsigned int i = 0; i <= verify_end_boundary; i++) + if (g_state.was_modified[i] && !g_state.was_verified[i]) { + chip_verified = 0; /* the byte was modified, but not verified after */ + printf("Error: byte 0x%x, modified: %d, verified: %d\n", i, g_state.was_modified[i], g_state.was_verified[i]); + } + if (chip_written) printf("Written chip memory state for %s is CORRECT\n", current_test_case->write_test_name); @@ -1124,8 +1193,16 @@ printf("Written chip memory state for %s is WRONG\n", current_test_case->write_test_name); + if (chip_verified) + printf("Written chip memory state for %s was verified successfully\n", + current_test_case->write_test_name); + else + printf("Written chip memory state for %s was NOT verified completely\n", + current_test_case->write_test_name); + all_write_test_result |= ret; all_write_test_result |= !chip_written; + all_write_test_result |= !chip_verified; teardown_chip(&layout); @@ -1147,7 +1224,7 @@ region->name = strdup("protected"); region->start = START_PROTECTED_REGION; region->end = END_PROTECTED_REGION; - region->read_prot = true; + region->read_prot = false; region->write_prot = true; } else { region->name = strdup("tail"); @@ -1197,6 +1274,12 @@ } memset(&g_state.buf[blockaddr], ERASE_VALUE, blocklen); + + /* Track the bytes were erased */ + memset(&g_state.was_modified[blockaddr], true, blocklen); + /* Clear the records of previous verification, if there were any */ + memset(&g_state.was_verified[blockaddr], false, blocklen); + return 0; } @@ -1229,7 +1312,7 @@ struct flashrom_layout *layout; - setup_chip(&flashctx, &layout, param, current_test_case); + const chipoff_t verify_end_boundary = setup_chip(&flashctx, &layout, param, current_test_case); // This test needs special block erase to emulate protected regions. memcpy(g_test_erase_injector, @@ -1266,6 +1349,13 @@ int eraseblocks_invocations = (g_state.eraseblocks_actual_ind == current_test_case->eraseblocks_expected_ind); + int chip_verified = 1; + for (unsigned int i = 0; i <= verify_end_boundary; i++) + if (g_state.was_modified[i] && !g_state.was_verified[i]) { + chip_verified = 0; /* byte was modified, but not verified after */ + printf("Error: byte 0x%x, modified: %d, verified: %d\n", i, g_state.was_modified[i], g_state.was_verified[i]); + } + if (chip_erased) printf("Erased chip memory state for %s is CORRECT\n", current_test_case->erase_test_name); @@ -1289,10 +1379,18 @@ current_test_case->eraseblocks_expected_ind, g_state.eraseblocks_actual_ind); + if (chip_verified) + printf("Erased chip memory state for %s was verified successfully\n", + current_test_case->erase_test_name); + else + printf("Erased chip memory state for %s was NOT verified completely\n", + current_test_case->erase_test_name); + all_erase_tests_result |= ret; all_erase_tests_result |= !chip_erased; all_erase_tests_result |= !eraseblocks_in_order; all_erase_tests_result |= !eraseblocks_invocations; + all_erase_tests_result |= !chip_verified; teardown_chip(&layout); @@ -1313,11 +1411,12 @@ int all_write_tests_result = 0; struct flashrom_flashctx flashctx = { 0 }; uint8_t newcontents[MIN_BUF_SIZE]; + uint8_t newcontents_protected[MIN_BUF_SIZE]; const char *param = ""; /* Default values for all params. */ struct flashrom_layout *layout; - setup_chip(&flashctx, &layout, param, current_test_case); + const chipoff_t verify_end_boundary = setup_chip(&flashctx, &layout, param, current_test_case); memcpy(&newcontents, current_test_case->written_buf, MOCK_CHIP_SIZE); // This test needs special block erase to emulate protected regions. @@ -1334,6 +1433,9 @@ flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS, true); flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNREADABLE_REGIONS, true); + flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, false); + /* We need to manually trigger verify op after write, because of protected regions */ + flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_AFTER_WRITE, false); // We use dummyflasher programmer in tests, but for this test we need to // replace dummyflasher's default get_region fn with test one. @@ -1347,8 +1449,25 @@ int ret = flashrom_image_write(&flashctx, &newcontents, MIN_BUF_SIZE, NULL); printf("%s returned %d.\n", current_test_case->write_test_name, ret); + /* Expected end result leaves protected region untouched */ + memcpy(&newcontents_protected, current_test_case->written_protected_buf, MOCK_CHIP_SIZE); + /* Outside of MOCK_CHIP_SIZE newcontents are not initialised in test cases, so just copy */ + memcpy(&newcontents_protected[MOCK_CHIP_SIZE], + &newcontents[MOCK_CHIP_SIZE], + MIN_BUF_SIZE - MOCK_CHIP_SIZE); + printf("%s verification started.\n", current_test_case->write_test_name); + ret = flashrom_image_verify(&flashctx, &newcontents_protected, MIN_BUF_SIZE); + printf("%s verification returned %d.\n", current_test_case->write_test_name, ret); + int chip_written = !memcmp(g_state.buf, current_test_case->written_protected_buf, MOCK_CHIP_SIZE); + int chip_verified = 1; + for (unsigned int i = 0; i <= verify_end_boundary; i++) + if (g_state.was_modified[i] && !g_state.was_verified[i]) { + chip_verified = 0; /* byte was modified, but not verified after */ + printf("Error: byte 0x%x, modified: %d, verified: %d\n", i, g_state.was_modified[i], g_state.was_verified[i]); + } + if (chip_written) printf("Written chip memory state for %s is CORRECT\n", current_test_case->write_test_name); @@ -1356,8 +1475,16 @@ printf("Written chip memory state for %s is WRONG\n", current_test_case->write_test_name); + if (chip_verified) + printf("Written chip memory state for %s was verified successfully\n", + current_test_case->write_test_name); + else + printf("Written chip memory state for %s was NOT verified completely\n", + current_test_case->write_test_name); + all_write_tests_result |= ret; all_write_tests_result |= !chip_written; + all_write_tests_result |= !chip_verified; teardown_chip(&layout); -- To view, visit https://review.coreboot.org/c/flashrom/+/84078?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3c5d55a0deb20f23f4072caac8c0dce04cc98fd4 Gerrit-Change-Number: 84078 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[XS] Change in flashrom[main]: erasure_layout: Fix init_eraseblock segmentation fault
by Anastasia Klimchuk (Code Review) Sept. 7, 2024

Sept. 7, 2024

Attention is currently required from: Aarya, DigitalDJ. Anastasia Klimchuk has posted comments on this change by DigitalDJ. ( https://review.coreboot.org/c/flashrom/+/84234?usp=email ) Change subject: erasure_layout: Fix init_eraseblock segmentation fault ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Patchset: PS5: Just for future: you don't need to vote on your own patch, we assume the author agrees with the own patch (otherwise why would you send it? :) ). In the dev guidelines we require the patch to be approved by at least one person who is not the author. Exception is when patch is co-developed by multiple authors, then they all approve that work. You don't have to change anything here, leave your vote as is, it's not an issue. That's just info for future! -- To view, visit https://review.coreboot.org/c/flashrom/+/84234?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I61bf0d93aa9f0b2b420b146be16fcd5124f0dc5d Gerrit-Change-Number: 84234 Gerrit-PatchSet: 5 Gerrit-Owner: DigitalDJ Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: DigitalDJ Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: DigitalDJ Gerrit-Comment-Date: Sat, 07 Sep 2024 08:44:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0