April 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: ichspi: Add support for Ice Lake
by Nico Huber (Code Review) 21 Apr '22

21 Apr '22

Attention is currently required from: Subrata Banik, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63584 ) Change subject: ichspi: Add support for Ice Lake ...................................................................... Patch Set 1: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/63584/comment/afd2d940_23a77e9a PS1, Line 7: ichspi: Add support for Ice Lake > Not sure what you mean by "undone" as those add legitimate new chipset support, seems like hyperbole. I mean the enum additions should be undone. Not the chipset support that could have been added with much less changes. > I believe you mean to say there should be some refactors happening to help consolidate so many enum branches. As you pointed out Nico, you tried a number of times in the past and didn't manage to trivially do it. Refactoring it might be difficult. Not adding unnecessary clutter is an orthogonal problem. > > In any case, I actually did upload a start towards making progress in that direction in CB:63321 but it has been sitting unreviewed for a few weeks at the moment. I believe the theme of that patch can be applied to a few other places towards a direction of having most of the chipset static details in a lookup table and a switch. > > More careful review of SPI Programmer Guides between generations is required to find better common patterns. Well, let me know if you found something that definitely makes things better. In the meantime, please avoid unnecessary enum additions. File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/63584/comment/e3617614_c50169ff PS1, Line 2149: 0x34a4 > Subrata, flashrom can be used in "odd" situations when folks are porting coreboot to boards in the community. I think this is what Nico is alluding to here as to make flashrom as robust as possible to allow for flashing. It's actually much simpler. We can avoid any regression by not removing the valid LPC ID. Nothing about robustness. Just in case that 0x34a4 is actually hidden on a system where flashrom already works, we wouldn't break it. > > Should DID's in the table used for detection of hidden situations perhaps have > " (hidden)" suffix and a more detailed code comment explaining so we can avoid confusion? Feel like this is it's own patch. As mentioned elsewhere, calling the function that deals with hidden devices when there are no hidden devices can simply be avoided. Then the code would be self-documenting. -- To view, visit https://review.coreboot.org/c/flashrom/+/63584 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If000b152c6e454617a7ad0245d16f8b1dea97fa0 Gerrit-Change-Number: 63584 Gerrit-PatchSet: 1 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 21 Apr 2022 09:17:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: [WIP] meson: rework the programmer selection
by Thomas Heijligen (Code Review) 21 Apr '22

21 Apr '22

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Peter Marheine. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63724 ) Change subject: [WIP] meson: rework the programmer selection ...................................................................... Patch Set 3: (4 comments) File meson.build: https://review.coreboot.org/c/flashrom/+/63724/comment/d86b9542_d0cda658 PS2, Line 98: # 'deps' : list[dep], # fallback: [] > Could each dep list additional files that it requires? It looks like every programmer that depends o […] I'll have a deeper look at it. But I don't think so. I would prefer to have all code needed to build a programmer in the programmers `srcs` list. The code in `pcidev.c` is just a bunch of functions that is shared between programmers. https://review.coreboot.org/c/flashrom/+/63724/comment/48a9fd69_1c9537e8 PS2, Line 418: p_x.get('deps',[]) > deps is already defaulted to [] above, so this doesn't need to specify a fallback. Done https://review.coreboot.org/c/flashrom/+/63724/comment/866b8e0e_a2764976 PS2, Line 427: elif selected and available > Seems clearer to nest these, since it isn't immediately obvious that this is the same condition as a […] I found non nested if statements easier. We would need more than 2 levels of nesting to get this right. selected (y) / \ (n) available selected_all+available (y) / \ (n) (y) / \ (n) deps_found error() deps_found selected_auto+available+deps_found (y) / \ (no) (y) / \ (n) (y) | active=true error() active=true error() active=true https://review.coreboot.org/c/flashrom/+/63724/comment/9e5ad60e_648c3d48 PS2, Line 464: if host_machine.system() in systems_serial_support > Could we avoid building this if no programmer needs serial? Might need a bit of rework to express it […] I haven't looked deeply into the serial code how to build it the bust way. But I think there is code shared between the programmer (`serial.c`) and code to support the operating system (`custom_baud/_linux/.c`). For now I've put it to the respective programmer. -- To view, visit https://review.coreboot.org/c/flashrom/+/63724 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib44b26e3748fc71f116184082b4aed0bb208b4c1 Gerrit-Change-Number: 63724 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-CC: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 21 Apr 2022 08:24:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: meson: ich_descriptors_tool remove unneeded dependencies
by Angel Pons (Code Review) 21 Apr '22

21 Apr '22

Attention is currently required from: Thomas Heijligen. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63757 ) Change subject: meson: ich_descriptors_tool remove unneeded dependencies ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/63757/comment/74fb1847_f2da97be PS1, Line 7: meson: ich_descriptors_tool remove unneeded dependencies How about: util/ich_descriptors_tool: Remove unneeded meson dependencies -- To view, visit https://review.coreboot.org/c/flashrom/+/63757 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ice1437cb294729b6af0e24f0a02692459b7a1412 Gerrit-Change-Number: 63757 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Comment-Date: Thu, 21 Apr 2022 08:14:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: meson: ich_descriptors_tool remove unneeded dependencies
by Thomas Heijligen (Code Review) 21 Apr '22

21 Apr '22

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63757 ) Change subject: meson: ich_descriptors_tool remove unneeded dependencies ...................................................................... meson: ich_descriptors_tool remove unneeded dependencies Change-Id: Ice1437cb294729b6af0e24f0a02692459b7a1412 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> --- M util/ich_descriptors_tool/meson.build 1 file changed, 0 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/57/63757/1 diff --git a/util/ich_descriptors_tool/meson.build b/util/ich_descriptors_tool/meson.build index b5bf09e..cd22176 100644 --- a/util/ich_descriptors_tool/meson.build +++ b/util/ich_descriptors_tool/meson.build @@ -4,9 +4,6 @@ 'ich_descriptors_tool.c', '../../ich_descriptors.c', ], - dependencies : [ - deps, - ], include_directories : include_directories('../..'), c_args : [ '-DICH_DESCRIPTORS_FROM_DUMP_ONLY', -- To view, visit https://review.coreboot.org/c/flashrom/+/63757 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ice1437cb294729b6af0e24f0a02692459b7a1412 Gerrit-Change-Number: 63757 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: [WIP] meson: rework the programmer selection
by Thomas Heijligen (Code Review) 21 Apr '22

21 Apr '22

Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk. Hello build bot (Jenkins), Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/63724 to look at the new patch set (#3). Change subject: [WIP] meson: rework the programmer selection ...................................................................... [WIP] meson: rework the programmer selection This is a WIP implementation for a new programmer selection. It should work by now but has some known and unknown bugs. (see Gerrit Comments) - Each dependency is optional and get mandetory when the programmer is selected - Each programmer is defined through an entry in the programmer dictionary - The entry lists systems & cpu_families where the programmer is supported, dependencies, sources, flags. The group parameter is for selecting a set of programmer, the default parameter is to include the programmer to the `all` and `auto` set. - If the programmer get's selectet the `active` entry is set to true. The `active` entry can be used afterwards to determin if the programmer should be included in sources, config, documentation Change-Id: Ib44b26e3748fc71f116184082b4aed0bb208b4c1 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> --- M meson.build M meson_options.txt 2 files changed, 404 insertions(+), 338 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/24/63724/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/63724 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib44b26e3748fc71f116184082b4aed0bb208b4c1 Gerrit-Change-Number: 63724 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-CC: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Close GitHub PRs and issues automatically
by Edward O'Callaghan (Code Review) 21 Apr '22

21 Apr '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63671 ) Change subject: Close GitHub PRs and issues automatically ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/63671 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8076f0fb964970ffd05f355b9d1e33a65aa7a3c8 Gerrit-Change-Number: 63671 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 21 Apr 2022 06:10:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic: add writeprotect CLI and update man page
by Edward O'Callaghan (Code Review) 21 Apr '22

21 Apr '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/58738 ) Change subject: cli_classic: add writeprotect CLI and update man page ...................................................................... cli_classic: add writeprotect CLI and update man page Tested with: GD25LQ128, GD25Q32, GD25Q256. BUG=b:195381327,b:153800563 BRANCH=none TEST=flashrom -{r,w,E} TEST=flashrom --wp-{enable,disable,range,region,list,status} Change-Id: I499f521781ee8999921996517802c0c0c641d869 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/58738 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> --- M cli_classic.c M flashrom.8.tmpl 2 files changed, 347 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved Sergii Dmytruk: Looks good to me, but someone else must approve diff --git a/cli_classic.c b/cli_classic.c index c543ff7..a0c18d4 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -59,6 +59,13 @@ " -N | --noverify-all verify included regions only (cf. -i)\n" " -x | --extract extract regions to files\n" " -l | --layout <layoutfile> read ROM layout from <layoutfile>\n" + " --wp-disable disable write protection\n" + " --wp-enable enable write protection\n" + " --wp-list list supported write protection ranges\n" + " --wp-status show write protection status\n" + " --wp-range=<start>,<len> set write protection range (use --wp-range=0,0\n" + " to unprotect the entire flash)\n" + " --wp-region <region> set write protection region\n" " --flash-name read out the detected flash name\n" " --flash-size read out the detected flash size\n" " --fmap read ROM layout from fmap embedded in ROM\n" @@ -119,6 +126,237 @@ return true; } +static int parse_wp_range(uint32_t *start, uint32_t *len) +{ + char *endptr = NULL, *token = NULL; + + if (!optarg) { + msg_gerr("Error: No wp-range values provided\n"); + return -1; + } + + token = strtok(optarg, ","); + if (!token) { + msg_gerr("Error: Invalid wp-range argument format\n"); + return -1; + } + *start = strtoul(token, &endptr, 0); + + token = strtok(NULL, ","); + if (!token) { + msg_gerr("Error: Invalid wp-range argument format\n"); + return -1; + } + *len = strtoul(token, &endptr, 0); + + return 0; +} + +static int print_wp_range(struct flashrom_flashctx *flash, size_t start, size_t len) +{ + /* Start address and length */ + msg_ginfo("start=0x%08zx length=0x%08zx ", start, len); + + /* Easily readable description like 'none' or 'lower 1/8' */ + size_t chip_len = flashrom_flash_getsize(flash); + + if (len == 0) { + msg_ginfo("(none)"); + } else if (len == chip_len) { + msg_ginfo("(all)"); + } else { + const char *location = ""; + if (start == 0) + location = "lower "; + if (start == chip_len - len) + location = "upper "; + + /* Remove common factors of 2 to simplify */ + /* the (range_len/chip_len) fraction. */ + while ((chip_len % 2) == 0 && (len % 2) == 0) { + chip_len /= 2; + len /= 2; + } + + msg_ginfo("(%s%zu/%zu)", location, len, chip_len); + } + + return 0; +} + +static const char *get_wp_error_str(int err) +{ + switch (err) { + case FLASHROM_WP_ERR_CHIP_UNSUPPORTED: + return "WP operations are not implemented for this chip"; + case FLASHROM_WP_ERR_READ_FAILED: + return "failed to read the current WP configuration"; + case FLASHROM_WP_ERR_WRITE_FAILED: + return "failed to write the new WP configuration"; + case FLASHROM_WP_ERR_VERIFY_FAILED: + return "unexpected WP configuration read back from chip"; + case FLASHROM_WP_ERR_MODE_UNSUPPORTED: + return "the requested protection mode is not supported"; + case FLASHROM_WP_ERR_RANGE_UNSUPPORTED: + return "the requested protection range is not supported"; + case FLASHROM_WP_ERR_RANGE_LIST_UNAVAILABLE: + return "could not determine what protection ranges are available"; + } + return "unknown WP error"; +} + +static int wp_cli( + struct flashctx *flash, + bool enable_wp, + bool disable_wp, + bool print_wp_status, + bool print_wp_ranges, + bool set_wp_range, + uint32_t wp_start, + uint32_t wp_len) +{ + if (print_wp_ranges) { + struct flashrom_wp_ranges *list; + enum flashrom_wp_result ret = flashrom_wp_get_available_ranges(&list, flash); + if (ret != FLASHROM_WP_OK) { + msg_gerr("Failed to get list of protection ranges: %s\n", + get_wp_error_str(ret)); + return 1; + } + size_t count = flashrom_wp_ranges_get_count(list); + + msg_ginfo("Available protection ranges:\n"); + for (size_t i = 0; i < count; i++) { + size_t start, len; + + flashrom_wp_ranges_get_range(&start, &len, list, i); + msg_ginfo("\t"); + print_wp_range(flash, start, len); + msg_ginfo("\n"); + } + + flashrom_wp_ranges_release(list); + } + + if (set_wp_range || disable_wp || enable_wp) { + enum flashrom_wp_mode old_mode = FLASHROM_WP_MODE_DISABLED; + struct flashrom_wp_cfg *cfg = NULL; + enum flashrom_wp_result ret = flashrom_wp_cfg_new(&cfg); + + if (ret == FLASHROM_WP_OK) + ret = flashrom_wp_read_cfg(cfg, flash); + + if (ret == FLASHROM_WP_OK) { + /* Store current WP mode for printing help text if */ + /* changing the cfg fails. */ + old_mode = flashrom_wp_get_mode(cfg); + + if (set_wp_range) + flashrom_wp_set_range(cfg, wp_start, wp_len); + + if (disable_wp) + flashrom_wp_set_mode(cfg, FLASHROM_WP_MODE_DISABLED); + + if (enable_wp) + flashrom_wp_set_mode(cfg, FLASHROM_WP_MODE_HARDWARE); + + ret = flashrom_wp_write_cfg(flash, cfg); + } + + flashrom_wp_cfg_release(cfg); + + if (ret != FLASHROM_WP_OK) { + msg_gerr("Failed to apply new WP settings: %s\n", + get_wp_error_str(ret)); + + /* Warn user if active WP is likely to have caused failure */ + if (ret == FLASHROM_WP_ERR_VERIFY_FAILED) { + switch (old_mode) { + case FLASHROM_WP_MODE_HARDWARE: + msg_gerr("Note: hardware status register protection is enabled. " + "The chip's WP# pin must be set to an inactive voltage " + "level to be able to change the WP settings.\n"); + break; + case FLASHROM_WP_MODE_POWER_CYCLE: + msg_gerr("Note: power-cycle status register protection is enabled. " + "A power-off, power-on cycle is usually required to change " + "the chip's WP settings.\n"); + break; + case FLASHROM_WP_MODE_PERMANENT: + msg_gerr("Note: permanent status register protection is enabled. " + "The chip's WP settings cannot be modified.\n"); + break; + default: + break; + } + } + + return 1; + } + + if (disable_wp) + msg_ginfo("Disabled hardware protection\n"); + + if (enable_wp) + msg_ginfo("Enabled hardware protection\n"); + + if (set_wp_range) { + msg_ginfo("Activated protection range: "); + print_wp_range(flash, wp_start, wp_len); + msg_ginfo("\n"); + } + } + + if (print_wp_status) { + size_t start, len; + enum flashrom_wp_mode mode; + struct flashrom_wp_cfg *cfg = NULL; + enum flashrom_wp_result ret = flashrom_wp_cfg_new(&cfg); + + if (ret == FLASHROM_WP_OK) + ret = flashrom_wp_read_cfg(cfg, flash); + + if (ret != FLASHROM_WP_OK) { + msg_gerr("Failed to get WP status: %s\n", + get_wp_error_str(ret)); + + flashrom_wp_cfg_release(cfg); + return 1; + } + + flashrom_wp_get_range(&start, &len, cfg); + mode = flashrom_wp_get_mode(cfg); + flashrom_wp_cfg_release(cfg); + + msg_ginfo("Protection range: "); + print_wp_range(flash, start, len); + msg_ginfo("\n"); + + msg_ginfo("Protection mode: "); + switch (mode) { + case FLASHROM_WP_MODE_DISABLED: + msg_ginfo("disabled"); + break; + case FLASHROM_WP_MODE_HARDWARE: + msg_ginfo("hardware"); + break; + case FLASHROM_WP_MODE_POWER_CYCLE: + msg_ginfo("power_cycle"); + break; + case FLASHROM_WP_MODE_PERMANENT: + msg_ginfo("permanent"); + break; + default: + msg_ginfo("unknown"); + break; + } + msg_ginfo("\n"); + } + + return 0; +} + + static int do_read(struct flashctx *const flash, const char *const filename) { int ret; @@ -229,6 +467,9 @@ int list_supported_wiki = 0; #endif int flash_name = 0, flash_size = 0; + int enable_wp = 0, disable_wp = 0, print_wp_status = 0; + int set_wp_range = 0, set_wp_region = 0, print_wp_ranges = 0; + uint32_t wp_start = 0, wp_len = 0; int read_it = 0, extract_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int dont_verify_it = 0, dont_verify_all = 0, list_supported = 0, operation_specified = 0; struct flashrom_layout *layout = NULL; @@ -240,6 +481,12 @@ OPTION_FLASH_CONTENTS, OPTION_FLASH_NAME, OPTION_FLASH_SIZE, + OPTION_WP_STATUS, + OPTION_WP_SET_RANGE, + OPTION_WP_SET_REGION, + OPTION_WP_ENABLE, + OPTION_WP_DISABLE, + OPTION_WP_LIST, }; int ret = 0; @@ -264,6 +511,12 @@ {"flash-name", 0, NULL, OPTION_FLASH_NAME}, {"flash-size", 0, NULL, OPTION_FLASH_SIZE}, {"get-size", 0, NULL, OPTION_FLASH_SIZE}, // (deprecated): back compatibility. + {"wp-status", 0, NULL, OPTION_WP_STATUS}, + {"wp-list", 0, NULL, OPTION_WP_LIST}, + {"wp-range", 1, NULL, OPTION_WP_SET_RANGE}, + {"wp-region", 1, NULL, OPTION_WP_SET_REGION}, + {"wp-enable", 0, NULL, OPTION_WP_ENABLE}, + {"wp-disable", 0, NULL, OPTION_WP_DISABLE}, {"list-supported", 0, NULL, 'L'}, {"list-supported-wiki", 0, NULL, 'z'}, {"programmer", 1, NULL, 'p'}, @@ -281,6 +534,7 @@ char *tempstr = NULL; char *pparam = NULL; struct layout_include_args *include_args = NULL; + char *wp_region = NULL; /* * Safety-guard against a user who has (mistakenly) closed @@ -409,6 +663,28 @@ cli_classic_validate_singleop(&operation_specified); flash_size = 1; break; + case OPTION_WP_STATUS: + print_wp_status = 1; + break; + case OPTION_WP_LIST: + print_wp_ranges = 1; + break; + case OPTION_WP_SET_RANGE: + if (parse_wp_range(&wp_start, &wp_len) < 0) + cli_classic_abort_usage("Incorrect wp-range arguments provided.\n"); + + set_wp_range = 1; + break; + case OPTION_WP_SET_REGION: + set_wp_region = 1; + wp_region = strdup(optarg); + break; + case OPTION_WP_ENABLE: + enable_wp = 1; + break; + case OPTION_WP_DISABLE: + disable_wp = 1; + break; case 'L': cli_classic_validate_singleop(&operation_specified); list_supported = 1; @@ -681,15 +957,29 @@ goto out_shutdown; } - const bool any_op = read_it || write_it || verify_it || erase_it || - flash_name || flash_size || extract_it; + const bool any_wp_op = + set_wp_range || set_wp_region || enable_wp || + disable_wp || print_wp_status || print_wp_ranges; + const bool any_op = read_it || write_it || verify_it || erase_it || + flash_name || flash_size || extract_it || any_wp_op; if (!any_op) { msg_ginfo("No operations were specified.\n"); goto out_shutdown; } + if (enable_wp && disable_wp) { + msg_ginfo("Error: --wp-enable and --wp-disable are mutually exclusive\n"); + ret = 1; + goto out_shutdown; + } + if (set_wp_range && set_wp_region) { + msg_gerr("Error: Cannot use both --wp-range and --wp-region simultaneously.\n"); + ret = 1; + goto out_shutdown; + } + if (flash_name) { if (fill_flash->chip->vendor && fill_flash->chip->name) { printf("vendor=\"%s\" name=\"%s\"\n", @@ -744,6 +1034,28 @@ goto out_shutdown; } flashrom_layout_set(fill_flash, layout); + + if (any_wp_op) { + if (set_wp_region && wp_region) { + ret = flashrom_layout_get_region_range(layout, wp_region, &wp_start, &wp_len); + if (ret) + goto out_release; + set_wp_range = true; + } + ret = wp_cli( + fill_flash, + enable_wp, + disable_wp, + print_wp_status, + print_wp_ranges, + set_wp_range, + wp_start, + wp_len + ); + if (ret) + goto out_release; + } + flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE, !!force); #if CONFIG_INTERNAL == 1 flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE_BOARDMISMATCH, !!force_boardmismatch); @@ -777,6 +1089,7 @@ else if (verify_it) ret = do_verify(fill_flash, filename); +out_release: flashrom_layout_release(layout); out_shutdown: flashrom_programmer_shutdown(NULL); diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index bf8960f..db6d619 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -48,8 +48,10 @@ \fB\-p\fR <programmername>[:<parameters>] [\fB\-c\fR <chipname>] (\fB\-\-flash\-name\fR|\fB\-\-flash\-size\fR| [\fB\-E\fR|\fB\-x\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] - [(\fB\-l\fR <file>|\fB\-\-ifd|\fB \-\-fmap\fR|\fB\-\-fmap-file\fR <file>) + [(\fB\-l\fR <file>|\fB\-\-ifd\fR|\fB \-\-fmap\fR|\fB\-\-fmap-file\fR <file>) [\fB\-i\fR <image>[:<file>]]] + [\fB\-\-wp\-status\fR] [\fB\-\-wp\-list\fR] [\fB\-\-wp\-enable\fR|\fB\-\-wp\-disable\fR] + [\fB\-\-wp\-range\fR <start>,<length>|\fB\-\-wp\-region\fR <region>] [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR])] [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] .SH DESCRIPTION @@ -285,11 +287,38 @@ .sp .B " flashrom \-p prog \-l <layout> \-i foo:foo.bin -i bar:bar.bin -w rom.bin .TP +.B "\-\-wp\-status" +Prints the flash's current status register protection mode and write protection +range. +.TP +.B "\-\-wp\-list" +Prints a list of all protection ranges that the flash supports. +.TP +.B "\-\-wp\-enable" +Enables hardware status register protection (SRP) if the flash supports it. +Once SRP is enabled, operations that change the flash's status registers +(including \fB\-\-wp\-disable\fR and \fB\-\-wp\-range\fR) can only be performed +if the flash's #WP pin is at an inactive logic level. +.TP +.B "\-\-wp\-disable" +Disables status register protection if the flash allows it. +.TP +.B "\-\-wp\-range <start>,<length>" +Configures the flash to protect a range of addresses from <start> to (<start> + +<length> - 1), bounds inclusive. The range must be supported by the flash, see +\fB\-\-wp\-list\fR. +.TP +.B "\-\-wp\-region <region>" +Same as \fB\-\-wp\-range\fR but protects the range occupied by an image region. +This option requires a image layout to be specified, see \fB\-\-layout\fR. The +region must be supported by the flash, see +\fB\-\-wp\-list\fR. +.TP .B "\-\-flash\-name" -Prints out the detected flash chips name. +Prints out the detected flash chip's name. .TP .B "\-\-flash\-size" -Prints out the detected flash chips size. +Prints out the detected flash chip's size. .TP .B "\-\-flash\-contents <ref\-file>" The file contents of -- To view, visit https://review.coreboot.org/c/flashrom/+/58738 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I499f521781ee8999921996517802c0c0c641d869 Gerrit-Change-Number: 58738 Gerrit-PatchSet: 80 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: flashrom.c: wip check_access write/erase path
by Edward O'Callaghan (Code Review) 21 Apr '22

21 Apr '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/63751 ) Change subject: flashrom.c: wip check_access write/erase path ...................................................................... flashrom.c: wip check_access write/erase path Change-Id: I15ec3131211548ab91be6866f48c5201c9865aa3 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 15 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/63751/1 diff --git a/flashrom.c b/flashrom.c index 9728c65..c032dc5 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1267,6 +1267,11 @@ info->region_start = entry->start; info->region_end = entry->end; + const chipsize_t region_len = entry->end - entry->start + 1; + int ret = check_access(flashctx, entry->start, region_len, 1); + if (!ret) + continue; + size_t j; int error = 1; /* retry as long as it's 1 */ for (j = 0; j < NUM_ERASEFUNCTIONS; ++j) { @@ -1370,6 +1375,11 @@ erase_len - starthere, &starthere, flashctx->chip->gran))) { if (!writecount++) msg_cdbg("W"); + + ret = check_access(flashctx, info->erase_start + starthere, lenhere, 1); + if (!ret) + continue; + /* Needs the partial write function signature. */ if (flashctx->chip->write(flashctx, backup_contents + starthere, info->erase_start + starthere, lenhere)) @@ -1474,6 +1484,11 @@ erase_len - starthere, &starthere, flashctx->chip->gran))) { if (!writecount++) msg_cdbg("W"); + + ret = check_access(flashctx, info->erase_start + starthere, lenhere, 1); + if (!ret) + continue; + /* Needs the partial write function signature. */ if (flashctx->chip->write(flashctx, newcontents + starthere, info->erase_start + starthere, lenhere)) -- To view, visit https://review.coreboot.org/c/flashrom/+/63751 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I15ec3131211548ab91be6866f48c5201c9865aa3 Gerrit-Change-Number: 63751 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Prepare for IFD check_access support
by Edward O'Callaghan (Code Review) 21 Apr '22

21 Apr '22

Attention is currently required from: Damien Zammit, Subrata Banik. Hello Damien Zammit, Subrata Banik, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62983 to look at the new patch set (#2). Change subject: Prepare for IFD check_access support ...................................................................... Prepare for IFD check_access support Based off work in collaboration with Damien Zammit. The following is intended to allow flashrom to query the spi or opaque master for access permissions before attempting to access a given region. This comes up in cases such as the ichspi driver where the CSME can deny regions access. Change-Id: If7bdcac2a4109efbc70abc2d6ae0561f39449004 Signed-off-by: Damien Zammit <damien(a)zamaudio.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipdrivers.h M flash.h M flashrom.c M opaque.c M programmer.h M spi.c M spi.h 7 files changed, 42 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/62983/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62983 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If7bdcac2a4109efbc70abc2d6ae0561f39449004 Gerrit-Change-Number: 62983 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Damien Zammit Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Damien Zammit Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: raiden_debug_spi: Add more informative error message when WP is enabled
by Edward O'Callaghan (Code Review) 21 Apr '22

21 Apr '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/62909 ) Change subject: raiden_debug_spi: Add more informative error message when WP is enabled ...................................................................... raiden_debug_spi: Add more informative error message when WP is enabled Current error messages are not very helpful when attempting to flash a target that has WP enabled. This change checks for the USB_SPI_DISABLED error that occurs in this case and gives a more informative error message. BUG=b:210645611 TEST=Tested with WP enabled and disable to verify that error message is displayed properly Signed-off-by: Robert Zieba <robertzieba(a)google.com> Change-Id: Ib1e8383baa9c3ea41ab1079af12e3dc8cdff90ae Reviewed-on: https://review.coreboot.org/c/flashrom/+/62909 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M raiden_debug_spi.c 1 file changed, 17 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/raiden_debug_spi.c b/raiden_debug_spi.c index 65ce32b..521539f 100644 --- a/raiden_debug_spi.c +++ b/raiden_debug_spi.c @@ -343,6 +343,7 @@ #include "usb_device.h" #include <libusb.h> +#include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> @@ -983,6 +984,22 @@ return status; } + /* + * Check if we received an error from the device. An error will have no + * response data, just the packet_id and status_code. + */ + const size_t err_packet_size = sizeof(struct usb_spi_response_v2) - + USB_SPI_PAYLOAD_SIZE_V2_RESPONSE; + if (rsp_config.packet_size == err_packet_size && + rsp_config.packet_v2.rsp_start.status_code != + USB_SPI_SUCCESS) { + status = rsp_config.packet_v2.rsp_start.status_code; + if (status == USB_SPI_DISABLED) { + msg_perr("Raiden: Target SPI bridge is disabled (is WP enabled?)\n"); + return status; + } + } + msg_perr("Raiden: Packet is not a valid config\n" " config attempt = %d\n" " packet id = %u\n" -- To view, visit https://review.coreboot.org/c/flashrom/+/62909 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib1e8383baa9c3ea41ab1079af12e3dc8cdff90ae Gerrit-Change-Number: 62909 Gerrit-PatchSet: 6 Gerrit-Owner: Robert Zieba <robertzieba(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Diana Zigterman <dzigterman(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Eric Peers <epeers(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rob Barnes <robbarnes(a)google.com> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2022