- flashrom-gerrit - mail.coreboot.org

[L] Change in flashrom[master]: Add support for VIA VL805 USB 3 XHCI flashing
by Thomas Heijligen (Code Review) 13 Feb '23

13 Feb '23

Attention is currently required from: ChrisEric1 CECL, Nicholas Chin. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72057 ) Change subject: Add support for VIA VL805 USB 3 XHCI flashing ...................................................................... Patch Set 3: (14 comments) Patchset: PS3: Hi Cristopher, fist of all: Thank you for contributing! This looks already quite good. Nevertheless I've a few thinks (most of them are quite small): * Can you move the changes in `flashchips.c` into an own commit * The entry for the meson buildsystem ist missing (meson.build:148ff, in alphabetic order) ``` 'vl805' : { 'systems' : systems_hwaccess, 'deps' : [ libpci ], 'groups' : [ group_pci, group_internal ], 'srcs' : files('vl805.c', 'pcidev.c'), 'flags' : [ '-DCONFIG_VL805=1' ], }, ``` * Can you share a little bit about the 'original driver' and the 'logs' you mentioned in the comments? File flashrom.8.tmpl: https://review.coreboot.org/c/flashrom/+/72057/comment/dc08e98d_90a1f463 PS3, Line 1371: VL806 Is there a different between VL805 and VL806? This is the only mention of the second chip File vl805.c: https://review.coreboot.org/c/flashrom/+/72057/comment/c7b0fe3b_e32443d1 PS3, Line 4: * Copyright (C) 2019, 2020 Carl-Daniel Hailfinger This is your copyright https://review.coreboot.org/c/flashrom/+/72057/comment/cbe1f3cd_9fbc68df PS3, Line 27: //#include "hwaccess.h" Please remove this comment. https://review.coreboot.org/c/flashrom/+/72057/comment/e6bcdfea_568b951f PS3, Line 28: #include "hwaccess_x86_io.h" This header is not needed, because the programmer doesn't do anything with I/O ports. (see line 137) https://review.coreboot.org/c/flashrom/+/72057/comment/718912f2_570babfc PS3, Line 31: extern const struct dev_entry devs_vl805[]; This is not needed. https://review.coreboot.org/c/flashrom/+/72057/comment/ba24dc43_400b35b0 PS3, Line 33: const struct dev_entry devs_vl805[] = { Make this `static`. It is only used in this file. https://review.coreboot.org/c/flashrom/+/72057/comment/52f5f643_97b62c1e PS3, Line 38: static struct pci_dev *dev = NULL; Can you please move this global into a programmer data struct, like it is done on e.g. the it8212.c programmer. https://review.coreboot.org/c/flashrom/+/72057/comment/1700ac6a_23201f0d PS3, Line 53: /* Some of the registers have unknown purpose and are just used inside the init sequence replay. */ Can you put these defines below the includes, ontop of all functions? https://review.coreboot.org/c/flashrom/+/72057/comment/76a7c3b1_21a76f19 PS3, Line 72: uint32_t indata = 0; : unsigned int curwritecnt = 0; : unsigned int curreadcnt = 0; These variables don't need to be initialized. https://review.coreboot.org/c/flashrom/+/72057/comment/6b8c3f70_3c5c602b PS3, Line 107: static const struct spi_master spi_master_vl805 = { Can this controller handle SPI flashes with 4-byte addresses? Then you should add `.features = SPI_MASTER_4BA` https://review.coreboot.org/c/flashrom/+/72057/comment/531b4192_a4ffa1d1 PS3, Line 131: int vl805_init(const struct programmer_cfg *cfg); This prototype is not needed if the function is static. https://review.coreboot.org/c/flashrom/+/72057/comment/6baa6082_11900120 PS3, Line 133: int vl805_init(const struct programmer_cfg *cfg) this should be `static` https://review.coreboot.org/c/flashrom/+/72057/comment/d9f74b91_a1b81306 PS3, Line 137: #if defined(__i386__) || defined(__x86_64__) : if (rget_io_perms()) : return 1; : #endif This can be removed since the programmer doesn't deal with I/O ports -- To view, visit https://review.coreboot.org/c/flashrom/+/72057 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I71435afcacdf97e14d627e35bce3d29de9657f38 Gerrit-Change-Number: 72057 Gerrit-PatchSet: 3 Gerrit-Owner: ChrisEric1 CECL <christopherericlentocha(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Attention: ChrisEric1 CECL <christopherericlentocha(a)gmail.com> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Mon, 13 Feb 2023 10:32:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: meson make: use VERSION file
by Edward O'Callaghan (Code Review) 13 Feb '23

13 Feb '23

Attention is currently required from: Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72657 ) Change subject: meson make: use VERSION file ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72657 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idc17eadb397b3c579bddfbf9ae6bf1b171f5dfb7 Gerrit-Change-Number: 72657 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 13 Feb 2023 09:16:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XL] Change in flashrom[master]: move manpage to sphinx
by Edward O'Callaghan (Code Review) 13 Feb '23

13 Feb '23

Attention is currently required from: Thomas Heijligen. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72619 ) Change subject: move manpage to sphinx ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72619 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iee9f1164c5913e47385e6f7d51dc7775a58b5a67 Gerrit-Change-Number: 72619 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Mon, 13 Feb 2023 09:15:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: dummyflasher: use new API to register shutdown function
by Anastasia Klimchuk (Code Review) 13 Feb '23

13 Feb '23

Attention is currently required from: Edward O'Callaghan, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 ) Change subject: dummyflasher: use new API to register shutdown function ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea Gerrit-Change-Number: 72408 Gerrit-PatchSet: 4 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> 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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Mon, 13 Feb 2023 05:08:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Add positive check to verify_fail_test
by Edward O'Callaghan (Code Review) 13 Feb '23

13 Feb '23

Attention is currently required from: Evan Benn. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71974 ) Change subject: flashrom_tester: Add positive check to verify_fail_test ...................................................................... Patch Set 10: (1 comment) File util/flashrom_tester/flashrom/src/cmd.rs: https://review.coreboot.org/c/flashrom/+/71974/comment/57c43d70_996dd5c8 PS10, Line 274: Some this doesn't type check. https://review.coreboot.org/c/flashrom/+/72674 I think the forward fix is to drop the `Some()` constructor but perhaps this patch wasn't tested somehow? -- To view, visit https://review.coreboot.org/c/flashrom/+/71974 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibbcc97086466b67cfab4f6c32140bb5f2c456beb Gerrit-Change-Number: 71974 Gerrit-PatchSet: 10 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Mon, 13 Feb 2023 05:06:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Add positive check to verify_fail_test
by Edward O'Callaghan (Code Review) 13 Feb '23

13 Feb '23

Edward O'Callaghan has created a revert of this change. ( https://review.coreboot.org/c/flashrom/+/71974 ) Change subject: flashrom_tester: Add positive check to verify_fail_test ...................................................................... -- To view, visit https://review.coreboot.org/c/flashrom/+/71974 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibbcc97086466b67cfab4f6c32140bb5f2c456beb Gerrit-Change-Number: 71974 Gerrit-PatchSet: 10 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: revert

1 0

[M] Change in flashrom[master]: Revert "flashrom_tester: Add positive check to verify_fail_test"
by Edward O'Callaghan (Code Review) 13 Feb '23

13 Feb '23

Attention is currently required from: Peter Marheine, Evan Benn. Hello build bot (Jenkins), Evan Benn, Peter Marheine, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/72674 to review the following change. Change subject: Revert "flashrom_tester: Add positive check to verify_fail_test" ...................................................................... Revert "flashrom_tester: Add positive check to verify_fail_test" This reverts commit 5735529d6209388079340a6fbd55e222a1c4c1b9. Reason for revert: Fails to type-check in verify_region_from_file(). Change-Id: Ib8c99de7aca672f42b3159082e2e63e0fefef08f --- M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/tests.rs 4 files changed, 15 insertions(+), 50 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/72674/1 diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index 1f13a8e..85cbb12 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -269,18 +269,6 @@ Ok(()) } - fn verify_region_from_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> { - let opts = FlashromOpt { - io_opt: Some(IOOpt::Verify(OperationArgs::RegionFileRegion( - region, path, None, - ))), - ..Default::default() - }; - - self.dispatch(opts, "verify_region_from_file")?; - Ok(()) - } - fn erase(&self) -> Result<(), FlashromError> { let opts = FlashromOpt { io_opt: Some(IOOpt::Erase), diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs index 5e1747b..bf09e6d 100644 --- a/util/flashrom_tester/flashrom/src/flashromlib.rs +++ b/util/flashrom_tester/flashrom/src/flashromlib.rs @@ -152,27 +152,6 @@ Ok(()) } - fn verify_region_from_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> { - let mut layout = self.flashrom.borrow_mut().layout_read_fmap_from_rom()?; - layout.include_region(region)?; - let range = layout.get_region_range(region)?; - let region_data = fs::read(path).map_err(|error| error.to_string())?; - if region_data.len() != range.len() { - return Err(format!( - "verify region range ({}) does not match provided file size ({})", - range.len(), - region_data.len() - ) - .into()); - } - let mut buf = vec![0; self.get_size()? as usize]; - buf[range].copy_from_slice(&region_data); - self.flashrom - .borrow_mut() - .image_verify(&buf, Some(layout))?; - Ok(()) - } - fn erase(&self) -> Result<(), FlashromError> { self.flashrom.borrow_mut().erase()?; Ok(()) diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index 41393e8..7c86649 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -133,8 +133,7 @@ /// Read the whole flash to the file specified by `path`. fn read_into_file(&self, path: &Path) -> Result<(), FlashromError>; - /// Read only a region of the flash into the file specified by `path`. Note - /// the first byte written to the file is the first byte from the region. + /// Read only a region of the flash. fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError>; /// Write the whole flash to the file specified by `path`. @@ -153,10 +152,6 @@ /// Verify the whole flash against the file specified by `path`. fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError>; - /// Verify only the region against the file specified by `path`. - /// Note the first byte in the file is matched against the first byte of the region. - fn verify_region_from_file(&self, path: &Path, region: &str) -> Result<(), FlashromError>; - /// Erase the whole flash. fn erase(&self) -> Result<(), FlashromError>; diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs index 721a789..847bfec 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -44,7 +44,6 @@ use std::sync::atomic::AtomicBool; const ELOG_FILE: &str = "/tmp/elog.file"; -const FW_MAIN_B_PATH: &str = "/tmp/FW_MAIN_B.bin"; /// Iterate over tests, yielding only those tests with names matching filter_names. /// @@ -313,16 +312,7 @@ /// Check that flashrom 'verify' will fail if the provided data does not match the chip data. fn verify_fail_test(env: &mut TestEnv) -> TestResult { - env.ensure_golden()?; - // Verify that verify is Ok when the data matches. We verify only a region - // and not the whole chip because coprocessors or firmware may have written - // some data in other regions. - env.cmd - .read_region_into_file(FW_MAIN_B_PATH.as_ref(), "FW_MAIN_B")?; - env.cmd - .verify_region_from_file(FW_MAIN_B_PATH.as_ref(), "FW_MAIN_B")?; - - // Verify that verify is false when the data does not match + // Comparing the flash contents to random data says they're not the same. match env.verify(env.random_data_file()) { Ok(_) => Err("Verification says flash is full of random data".into()), Err(_) => Ok(()), -- To view, visit https://review.coreboot.org/c/flashrom/+/72674 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib8c99de7aca672f42b3159082e2e63e0fefef08f Gerrit-Change-Number: 72674 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom.c: Add new erase_by_layout and walk_by_layout implementations
by Anastasia Klimchuk (Code Review) 13 Feb '23

13 Feb '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/71173 ) ( 22 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom.c: Add new erase_by_layout and walk_by_layout implementations ...................................................................... flashrom.c: Add new erase_by_layout and walk_by_layout implementations Add [erase,walk]_by_layout_new to use optimised implementations of the erase function selection algorithm. Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6 Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/71173 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Thomas Heijligen <src(a)posteo.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M flashrom.c 1 file changed, 98 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Thomas Heijligen: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/flashrom.c b/flashrom.c index 1790ad9..d17e04b 100644 --- a/flashrom.c +++ b/flashrom.c @@ -33,6 +33,7 @@ #include "programmer.h" #include "hwaccess_physmap.h" #include "chipdrivers.h" +#include "erasure_layout.h" static bool use_legacy_erase_path = true; @@ -1537,11 +1538,54 @@ return walk_by_layout(flashctx, &info, &erase_block, &all_skipped); } +static int erase_by_layout_new(struct flashctx *const flashctx) +{ + bool all_skipped = true; + const uint32_t flash_size = flashctx->chip->total_size * 1024; + uint8_t* curcontents = malloc(flash_size); + uint8_t* newcontents = malloc(flash_size); + struct erase_layout *erase_layout; + create_erase_layout(flashctx, &erase_layout); + int ret = 0; + + //erase layout creation failed + if (!erase_layout) { + ret = 1; + goto _ret; + } + + //not enough memory + if (!curcontents || !newcontents) { + ret = 1; + goto _ret; + } + + memset(curcontents, ~ERASED_VALUE(flashctx), flash_size); + memset(newcontents, ERASED_VALUE(flashctx), flash_size); + + const struct flashrom_layout *const flash_layout = get_layout(flashctx); + const struct romentry *entry = NULL; + while ((entry = layout_next_included(flash_layout, entry))) { + ret = erase_write(flashctx, entry->region.start, entry->region.end, curcontents, newcontents, erase_layout, &all_skipped); + if (ret) { + ret = 1; + msg_cerr("Erase Failed"); + goto _ret; + } + } + +_ret: + free(curcontents); + free(newcontents); + free_erase_layout(erase_layout, count_usable_erasers(flashctx)); + return ret; +} + static int erase_by_layout(struct flashctx *const flashctx) { if (use_legacy_erase_path) return erase_by_layout_legacy(flashctx); - return 1; /* unimplemented. */ + return erase_by_layout_new(flashctx); } static int read_erase_write_block(struct flashctx *const flashctx, @@ -1662,13 +1706,48 @@ return walk_by_layout(flashctx, &info, read_erase_write_block, all_skipped); } +static int write_by_layout_new(struct flashctx *const flashctx, + void *const curcontents, const void *const newcontents, + bool *all_skipped) +{ + const int erasefn_count = count_usable_erasers(flashctx); + int ret = 1; + + const struct flashrom_layout *const flash_layout = get_layout(flashctx); + struct erase_layout *erase_layout; + create_erase_layout(flashctx, &erase_layout); + + if (!flash_layout) { + goto _ret; + } + if (!erase_layout) { + goto _ret; + } + + const struct romentry *entry = NULL; + while ((entry = layout_next_included(flash_layout, entry))) { + ret = erase_write(flashctx, entry->region.start, + entry->region.end, + curcontents, + (uint8_t *)newcontents, + erase_layout, all_skipped); + if (ret) { + msg_cerr("Write Failed!"); + goto _ret; + } + } +_ret: + free_erase_layout(erase_layout, erasefn_count); + return ret; +} + static int write_by_layout(struct flashctx *const flashctx, uint8_t *const curcontents, const uint8_t *const newcontents, bool *all_skipped) { if (use_legacy_erase_path) return write_by_layout_legacy(flashctx, curcontents, newcontents, all_skipped); - return 1; /* unimplemented. */ + return write_by_layout_new(flashctx, curcontents, newcontents, all_skipped); } /** -- To view, visit https://review.coreboot.org/c/flashrom/+/71173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id79ae943eb9d6a817da28381db477725834faaf6 Gerrit-Change-Number: 71173 Gerrit-PatchSet: 24 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: flashrom.c: Trivial code style fix
by Sam McNally (Code Review) 13 Feb '23

13 Feb '23

Attention is currently required from: Edward O'Callaghan. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72968 ) Change subject: flashrom.c: Trivial code style fix ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72968 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iacdb62067a8d22261d4eabe73ad96168eb11417c Gerrit-Change-Number: 72968 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> 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-Comment-Date: Mon, 13 Feb 2023 04:55:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in flashrom[master]: flashrom.c: Add functions for new erase selection algorithm
by Anastasia Klimchuk (Code Review) 13 Feb '23

13 Feb '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/65844 ) Change subject: flashrom.c: Add functions for new erase selection algorithm ...................................................................... flashrom.c: Add functions for new erase selection algorithm 1) Add function to flatten out the addresses of the flash chip as per the different erase functions. This function will return a list of layouts which is dynamically allocated. So after use all the layouts as well as the list itself should be freed. The free_erase_layout function does that. 2) Add function to align start and end address of the region (in struct walk_info) to some erase sector boundaries and modify the region start and end addresses to match nearest erase sector boundaries. This function will be used in the new algorithm for erase function selection. 3) Add function that returns a list of sectors (as seen by the first erase function) that need erasing. 4) Add a function to call the erase algorithm. Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/65844 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Thomas Heijligen <src(a)posteo.de> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M Makefile A erasure_layout.c A include/erasure_layout.h M meson.build 4 files changed, 480 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Thomas Heijligen: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/Makefile b/Makefile index 2ebd370..b37e807 100644 --- a/Makefile +++ b/Makefile @@ -388,7 +388,7 @@ ############################################################################### # Library code. -LIB_OBJS = libflashrom.o layout.o flashrom.o udelay.o parallel.o programmer.o programmer_table.o \ +LIB_OBJS = libflashrom.o layout.o erasure_layout.o flashrom.o udelay.o parallel.o programmer.o programmer_table.o \ helpers.o helpers_fileio.o ich_descriptors.o fmap.o platform/endian_$(ENDIAN).o platform/memaccess.o diff --git a/erasure_layout.c b/erasure_layout.c new file mode 100644 index 0000000..05376de --- /dev/null +++ b/erasure_layout.c @@ -0,0 +1,406 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2022 Aarya Chaumal + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <limits.h> +#include <stdbool.h> +#include <stdlib.h> +#include <limits.h> +#include <string.h> + +#include "include/flash.h" +#include "include/layout.h" +#include "include/erasure_layout.h" + +static size_t calculate_block_count(const struct flashchip *chip, size_t eraser_idx) +{ + size_t block_count = 0; + + chipoff_t addr = 0; + for (size_t i = 0; addr < chip->total_size * 1024; i++) { + const struct eraseblock *block = &chip->block_erasers[eraser_idx].eraseblocks[i]; + block_count += block->count; + addr += block->size * block->count; + } + + return block_count; +} + +static void init_eraseblock(struct erase_layout *layout, size_t idx, size_t block_num, + chipoff_t start_addr, chipoff_t end_addr, size_t *sub_block_index) +{ + struct eraseblock_data *edata = &layout[idx].layout_list[block_num]; + edata->start_addr = start_addr; + edata->end_addr = end_addr; + edata->selected = false; + edata->block_num = block_num; + + if (!idx) + return; + + 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) { + (*sub_block_index)++; + subedata++; + } + edata->last_sub_block_index = *sub_block_index - 1; +} + +/* + * @brief Function to free the created erase_layout + * + * @param layout pointer to allocated layout + * @param erasefn_count number of erase functions for which the layout was created + * + */ +void free_erase_layout(struct erase_layout *layout, unsigned int erasefn_count) +{ + if (!layout) + return; + for (size_t i = 0; i < erasefn_count; i++) { + free(layout[i].layout_list); + } + free(layout); +} + +/* + * @brief Function to create an erase layout + * + * @param flashctx flash context + * @param e_layout address to the pointer to store the layout + * @return 0 on success, + * -1 if layout creation fails + * + * This function creates a layout of which erase functions erase which regions + * of the flash chip. This helps to optimally select the erase functions for + * erase/write operations. + */ +int create_erase_layout(struct flashctx *const flashctx, struct erase_layout **e_layout) +{ + const struct flashchip *chip = flashctx->chip; + const size_t erasefn_count = count_usable_erasers(flashctx); + struct erase_layout *layout = calloc(erasefn_count, sizeof(struct erase_layout)); + + if (!layout) { + msg_gerr("Out of memory!\n"); + return -1; + } + + if (!erasefn_count) { + msg_gerr("No erase functions supported\n"); + return 0; + } + + size_t layout_idx = 0; + for (size_t eraser_idx = 0; eraser_idx < NUM_ERASEFUNCTIONS; eraser_idx++) { + if (check_block_eraser(flashctx, eraser_idx, 0)) + continue; + + layout[layout_idx].eraser = &chip->block_erasers[eraser_idx]; + const size_t block_count = calculate_block_count(flashctx->chip, eraser_idx); + size_t sub_block_index = 0; + + layout[layout_idx].block_count = block_count; + layout[layout_idx].layout_list = (struct eraseblock_data *)calloc(block_count, + sizeof(struct eraseblock_data)); + + if (!layout[layout_idx].layout_list) { + free_erase_layout(layout, layout_idx); + return -1; + } + + size_t block_num = 0; + chipoff_t start_addr = 0; + + for (int i = 0; block_num < block_count; i++) { + const struct eraseblock *block = &chip->block_erasers[eraser_idx].eraseblocks[i]; + + for (size_t num = 0; num < block->count; num++) { + chipoff_t end_addr = start_addr + block->size - 1; + init_eraseblock(layout, layout_idx, block_num, + start_addr, end_addr, &sub_block_index); + block_num += 1; + start_addr = end_addr + 1; + } + } + layout_idx++; + } + + *e_layout = layout; + return layout_idx; +} + +/* + * @brief Function to align start and address of the region boundaries + * + * @param layout erase layout + * @param flashctx flash context + * @param region_start pointer to start address of the region to align + * @param region_end pointer to end address of the region to align + * + * This function aligns start and end address of the region (in struct walk_info) + * to some erase sector boundaries and modify the region start and end addresses + * to match nearest erase sector boundaries. This function will be used in the + * new algorithm for erase function selection. + */ +static void align_region(const struct erase_layout *layout, struct flashctx *const flashctx, + chipoff_t *region_start, chipoff_t *region_end) +{ + chipoff_t start_diff = UINT_MAX, end_diff = UINT_MAX; + const size_t erasefn_count = count_usable_erasers(flashctx); + for (size_t i = 0; i < erasefn_count; i++) { + for (size_t j = 0; j < layout[i].block_count; j++) { + const struct eraseblock_data *ll = &layout[i].layout_list[j]; + if (ll->start_addr <= *region_start) + start_diff = (*region_start - ll->start_addr) > start_diff ? + start_diff : (*region_start - ll->start_addr); + if (ll->end_addr >= *region_end) + end_diff = (ll->end_addr - *region_end) > end_diff ? + end_diff : (ll->end_addr - *region_end); + } + } + + if (start_diff) { + msg_cinfo("Region start not sector aligned! Extending start boundaries...\n"); + *region_start = *region_start - start_diff; + } + if (end_diff) { + msg_cinfo("Region end not sector aligned! Extending end boundaries...\n"); + *region_end = *region_end + end_diff; + } +} + +/* + * @brief Function to select the list of sectors that need erasing + * + * @param flashctx flash context + * @param layout erase layout + * @param findex index of the erase function + * @param block_num index of the block to erase according to the erase function index + * @param curcontents buffer containg the current contents of the flash + * @param newcontents buffer containg the new contents of the flash + * @param rstart start address of the region + * @rend rend end address of the region + */ +static void select_erase_functions(struct flashctx *flashctx, const struct erase_layout *layout, + size_t findex, size_t block_num, uint8_t *curcontents, uint8_t *newcontents, + chipoff_t rstart, chipoff_t rend) +{ + struct eraseblock_data *ll = &layout[findex].layout_list[block_num]; + if (!findex) { + if (ll->start_addr >= rstart && ll->end_addr <= rend) { + chipoff_t start_addr = ll->start_addr; + chipoff_t end_addr = ll->end_addr; + const chipsize_t erase_len = end_addr - start_addr + 1; + const uint8_t erased_value = ERASED_VALUE(flashctx); + ll->selected = need_erase(curcontents + start_addr, newcontents + start_addr, erase_len, + flashctx->chip->gran, erased_value); + } + } else { + int count = 0; + const int sub_block_start = ll->first_sub_block_index; + const int sub_block_end = ll->last_sub_block_index; + + for (int j = sub_block_start; j <= sub_block_end; j++) { + select_erase_functions(flashctx, layout, findex - 1, j, curcontents, newcontents, + rstart, rend); + if (layout[findex - 1].layout_list[j].selected) + count++; + } + + const int total_blocks = sub_block_end - sub_block_start + 1; + if (count && count > total_blocks/2) { + if (ll->start_addr >= rstart && ll->end_addr <= rend) { + for (int j = sub_block_start; j <= sub_block_end; j++) + layout[findex - 1].layout_list[j].selected = false; + ll->selected = true; + } + } + } +} + +/* + * @brief wrapper to use the erase algorithm + * + * @param flashctx flash context + * @param region_start start address of the region + * @param region_end end address of the region + * @param curcontents buffer containg the current contents of the flash + * @param newcontents buffer containg the new contents of the flash + * @param erase_layout erase layout + * @param all_skipped pointer to the flag to chec if any block was erased + */ +int erase_write(struct flashctx *const flashctx, chipoff_t region_start, chipoff_t region_end, + uint8_t *curcontents, uint8_t *newcontents, + struct erase_layout *erase_layout, bool *all_skipped) +{ + const size_t erasefn_count = count_usable_erasers(flashctx); + int ret = 0; + size_t i; + chipoff_t old_start = region_start, old_end = region_end; + align_region(erase_layout, flashctx, &region_start, &region_end); + + uint8_t *old_start_buf = NULL, *old_end_buf = NULL; + old_start_buf = (uint8_t *)malloc(old_start - region_start); + if (!old_start_buf) { + msg_cerr("Not enough memory!\n"); + ret = -1; + goto _end; + } + old_end_buf = (uint8_t *)malloc(region_end - old_end); + if (!old_end_buf) { + msg_cerr("Not enough memory!\n"); + ret = -1; + goto _end; + } + + if (old_start - region_start) { + read_flash(flashctx, curcontents + region_start, region_start, old_start - region_start); + memcpy(old_start_buf, newcontents + region_start, old_start - region_start); + memcpy(newcontents + region_start, curcontents + region_start, old_start - region_start); + } + if (region_end - old_end) { + read_flash(flashctx, curcontents + old_end, old_end, region_end - old_end); + memcpy(old_end_buf, newcontents + old_end, region_end - old_end); + memcpy(newcontents + old_end, curcontents + old_end, region_end - old_end); + } + + // select erase functions + for (i = 0; i < erase_layout[erasefn_count - 1].block_count; i++) { + if (erase_layout[erasefn_count - 1].layout_list[i].start_addr <= region_end && + region_start <= erase_layout[erasefn_count - 1].layout_list[i].end_addr) + select_erase_functions(flashctx, erase_layout, + erasefn_count - 1, i, + curcontents, newcontents, + region_start, region_end); + } + + for (i = 0; i < erasefn_count; i++) { + for (size_t j = 0; j < erase_layout[i].block_count; j++) { + if (!erase_layout[i].layout_list[j].selected) continue; + + // erase + chipoff_t start_addr = erase_layout[i].layout_list[j].start_addr; + unsigned int block_len = erase_layout[i].layout_list[j].end_addr - start_addr + 1; + const uint8_t erased_value = ERASED_VALUE(flashctx); + // execute erase + erasefunc_t *erasefn = lookup_erase_func_ptr(erase_layout[i].eraser); + + if (!flashctx->flags.skip_unwritable_regions) { + if (check_for_unwritable_regions(flashctx, start_addr, block_len)) + goto _end; + } + + unsigned int len; + for (unsigned int addr = start_addr; addr < start_addr + block_len; addr += len) { + struct flash_region region; + get_flash_region(flashctx, addr, &region); + + len = min(start_addr + block_len, region.end) - addr; + + if (region.write_prot) { + msg_gdbg("%s: cannot erase inside %s " + "region (%#08x..%#08x), skipping range (%#08x..%#08x).\n", + __func__, region.name, + region.start, region.end - 1, + addr, addr + len - 1); + free(region.name); + continue; + } + + msg_gdbg("%s: %s region (%#08x..%#08x) is " + "writable, erasing range (%#08x..%#08x).\n", + __func__, region.name, + region.start, region.end - 1, + addr, addr + len - 1); + free(region.name); + + if (erasefn(flashctx, addr, len)) + goto _end; + if (check_erased_range(flashctx, addr, len)) { + msg_cerr("ERASE FAILED!\n"); + goto _end; + } + } + + ret = erasefn(flashctx, start_addr, block_len); + if (ret) { + msg_cerr("Failed to execute erase command " + "for offset %#x to %#x.\n", + start_addr, start_addr + block_len); + ret = -1; + goto _end; + } + + // adjust curcontents + memset(curcontents+start_addr, erased_value, block_len); + // after erase make it unselected again + erase_layout[i].layout_list[j].selected = false; + msg_cdbg("E(%x:%x)", start_addr, start_addr + block_len - 1); + // verify erase + ret = check_erased_range(flashctx, start_addr, block_len); + if (ret) { + msg_cerr("Verifying flash. Erase failed for range %#x : %#x, Abort.\n", + start_addr, start_addr + block_len - 1); + goto _end; + } + + *all_skipped = false; + } + } + + // write + unsigned int start_here = 0, len_here = 0, erase_len = region_end - region_start + 1; + while ((len_here = get_next_write(curcontents + region_start + start_here, + newcontents + region_start + start_here, + erase_len - start_here, &start_here, + flashctx->chip->gran))) { + // execute write + ret = write_flash(flashctx, + newcontents + region_start + start_here, + region_start + start_here, len_here); + if (ret) { + msg_cerr("Write failed at %#zx, Abort.\n", i); + ret = -1; + goto _end; + } + + // adjust curcontents + memcpy(curcontents + region_start + start_here, + newcontents + region_start + start_here, len_here); + msg_cdbg("W(%x:%x)", region_start + start_here, region_start + start_here + len_here - 1); + + *all_skipped = false; + } + // verify write + ret = verify_range(flashctx, newcontents + region_start, region_start, region_end - region_start); + if (ret) { + msg_cerr("Verifying flash. Write failed for range %#x : %#x, Abort.\n", + region_start, region_end); + goto _end; + } + +_end: + memcpy(newcontents + region_start, old_start_buf, old_start - region_start); + memcpy(newcontents + old_end, old_end_buf, region_end - old_end); + + free(old_start_buf); + free(old_end_buf); + + msg_cinfo("Erase/write done from %x to %x\n", region_start, region_end); + return ret; +} diff --git a/include/erasure_layout.h b/include/erasure_layout.h new file mode 100644 index 0000000..b8b5f08 --- /dev/null +++ b/include/erasure_layout.h @@ -0,0 +1,41 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2022 Aarya Chaumal + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __ERASURE_LAYOUT_H__ +#define __ERASURE_LAYOUT_H__ 1 + +struct eraseblock_data { + chipoff_t start_addr; + chipoff_t end_addr; + bool selected; + size_t block_num; + size_t first_sub_block_index; + size_t last_sub_block_index; +}; + +struct erase_layout { + struct eraseblock_data* layout_list; + size_t block_count; + const struct block_eraser *eraser; +}; + +void free_erase_layout(struct erase_layout *layout, unsigned int erasefn_count); +int create_erase_layout(struct flashctx *const flashctx, struct erase_layout **erase_layout); +int erase_write(struct flashctx *const flashctx, chipoff_t region_start, chipoff_t region_end, + uint8_t* curcontents, uint8_t* newcontents, + struct erase_layout *erase_layout, bool *all_skipped); + +#endif /* !__ERASURE_LAYOUT_H__ */ diff --git a/meson.build b/meson.build index 57ac438..2575fe1 100644 --- a/meson.build +++ b/meson.build @@ -53,6 +53,7 @@ 'bitbang_spi.c', 'edi.c', 'en29lv640b.c', + 'erasure_layout.c', 'flashchips.c', 'flashrom.c', 'fmap.c', -- To view, visit https://review.coreboot.org/c/flashrom/+/65844 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic57ca1cca3d1646543f6b5939ba9c35db8d08256 Gerrit-Change-Number: 65844 Gerrit-PatchSet: 89 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit