Hello Angel Pons, Arthur Heymans, David Hendricks, Thomas Heijligen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/33543
to review the following change.
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
layout: Introduce flashrom_layout_empty()
It initializes an empty layout. Currently the maximum number of entries has to be specified, which will vanisch once we use dynamic allocation.
Change-Id: I2ae7246493ff592e631cce924777925c7825e398 Signed-off-by: Nico Huber nico.h@gmx.de --- M cli_classic.c M flash.h M flashrom.c M ich_descriptors.c M ich_descriptors.h M layout.c M libflashrom.c M libflashrom.h 8 files changed, 54 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/33543/1
diff --git a/cli_classic.c b/cli_classic.c index 2e07612..a98be47 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -673,8 +673,10 @@ out_shutdown: programmer_shutdown(); out: - for (i = 0; i < chipcount; i++) + for (i = 0; i < chipcount; i++) { + flashrom_layout_release(flashes[i].default_layout); free(flashes[i].chip); + }
layout_cleanup(&include_args); free(filename); diff --git a/flash.h b/flash.h index 5bbfa0a..84de7a7 100644 --- a/flash.h +++ b/flash.h @@ -252,7 +252,7 @@ chipaddr virtual_registers; struct registered_master *mst; const struct flashrom_layout *layout; - struct single_layout fallback_layout; + struct flashrom_layout *default_layout; struct { bool force; bool force_boardmismatch; diff --git a/flashrom.c b/flashrom.c index e355731..3436a59 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1331,15 +1331,12 @@ if (!flash->chip) return -1;
- /* Fill fallback layout covering the whole chip. */ - struct single_layout *const fallback = &flash->fallback_layout; - fallback->base.entries = &fallback->entry; - fallback->base.max_entries = 1; - fallback->base.num_entries = 0; - if (flashrom_layout_add_region(&fallback->base, + /* Fill default layout covering the whole chip. */ + if (flashrom_layout_empty(&flash->default_layout, 1) || + flashrom_layout_add_region(flash->default_layout, 0, flash->chip->total_size * 1024 - 1, "complete flash") || - flashrom_layout_include_region(&fallback->base, "complete flash")) - return -1; + flashrom_layout_include_region(flash->default_layout, "complete flash")) + return -1;
tmp = flashbuses_to_text(flash->chip->bustype); msg_cinfo("%s %s flash chip "%s" (%d kB, %s) ", force ? "Assuming" : "Found", diff --git a/ich_descriptors.c b/ich_descriptors.c index b2c5d4c..8383801 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -1156,7 +1156,9 @@ * 1 if the descriptor couldn't be parsed, * 2 when out of memory. */ -int layout_from_ich_descriptors(struct ich_layout *const layout, const void *const dump, const size_t len) +int layout_from_ich_descriptors( + struct flashrom_layout **const layout, + const void *const dump, const size_t len) { static const char *const regions[] = { "fd", "bios", "me", "gbe", "pd", "reg5", "bios2", "reg7", "ec", "reg9", "ie", @@ -1168,10 +1170,8 @@ if (read_ich_descriptors_from_dump(dump, len, &cs, &desc)) return 1;
- memset(layout, 0x00, sizeof(*layout)); - layout->base.entries = layout->entries; - layout->base.max_entries = ARRAY_SIZE(layout->entries); - layout->base.num_entries = 0; + if (flashrom_layout_empty(layout, ARRAY_SIZE(regions))) + return 2;
ssize_t i; for (i = 0; i < min(ich_number_of_regions(cs, &desc.content), ARRAY_SIZE(regions)); ++i) { @@ -1179,8 +1179,11 @@ const chipoff_t limit = ICH_FREG_LIMIT(desc.region.FLREGs[i]); if (limit <= base) continue; - if (flashrom_layout_add_region(&layout->base, base, limit, regions[i])) + if (flashrom_layout_add_region(*layout, base, limit, regions[i])) { + flashrom_layout_release(*layout); + *layout = NULL; return 2; + } } return 0; } diff --git a/ich_descriptors.h b/ich_descriptors.h index 4225286..498af21 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -562,11 +562,6 @@ struct ich_desc_upper_map upper; };
-struct ich_layout { - struct flashrom_layout base; - struct romentry entries[MAX_NUM_FLREGS]; -}; - ssize_t ich_number_of_regions(enum ich_chipset cs, const struct ich_desc_content *content); ssize_t ich_number_of_masters(enum ich_chipset cs, const struct ich_desc_content *content);
@@ -585,6 +580,6 @@ int read_ich_descriptors_via_fdo(enum ich_chipset cs, void *spibar, struct ich_descriptors *desc); int getFCBA_component_density(enum ich_chipset cs, const struct ich_descriptors *desc, uint8_t idx);
-int layout_from_ich_descriptors(struct ich_layout *, const void *dump, size_t len); +int layout_from_ich_descriptors(struct flashrom_layout **, const void *dump, size_t len);
#endif /* __ICH_DESCRIPTORS_H__ */ diff --git a/layout.c b/layout.c index 0978a67..8198f17 100644 --- a/layout.c +++ b/layout.c @@ -37,7 +37,7 @@ if (flashctx->layout && flashctx->layout->num_entries) return flashctx->layout; else - return &flashctx->fallback_layout.base; + return flashctx->default_layout; }
#ifndef __LIBPAYLOAD__ @@ -270,6 +270,29 @@ */
/** + * @brief Create a new, empty layout. + * + * @param layout Pointer to returned layout reference. + * @param count Number of layout entries to allocate. + * + * @return 0 on success, + * 1 if out of memory. + */ +int flashrom_layout_empty(struct flashrom_layout **const layout, const unsigned int count) +{ + *layout = malloc(sizeof(**layout) + count * sizeof(struct romentry)); + if (!*layout) { + msg_gerr("Error creating layout: %s\n", strerror(errno)); + return 1; + } + + (*layout)->entries = (void *)((char *)*layout + sizeof(**layout)); + (*layout)->max_entries = count; + (*layout)->num_entries = 0; + return 0; +} + +/** * @brief Add another region to an existing layout. * * @param layout The existing layout. diff --git a/libflashrom.c b/libflashrom.c index aa43e65..d891944 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -205,13 +205,14 @@ ret = 0; /* We found one chip, now check that there is no second match. */ if (probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0) != -1) { + flashrom_layout_release(second_flashctx.default_layout); ret = 3; break; } } } if (ret) { - free(*flashctx); + flashrom_flash_release(*flashctx); *flashctx = NULL; } return ret; @@ -235,6 +236,7 @@ */ void flashrom_flash_release(struct flashrom_flashctx *const flashctx) { + flashrom_layout_release(flashctx->default_layout); free(flashctx); }
@@ -310,16 +312,10 @@ #ifndef __FLASHROM_LITTLE_ENDIAN__ return 6; #else - struct ich_layout dump_layout; + struct flashrom_layout *dump_layout, *chip_layout; int ret = 1;
void *const desc = malloc(0x1000); - struct ich_layout *const chip_layout = malloc(sizeof(*chip_layout)); - if (!desc || !chip_layout) { - msg_gerr("Out of memory!\n"); - goto _free_ret; - } - if (prepare_flash_access(flashctx, true, false, false, false)) goto _free_ret;
@@ -332,7 +328,7 @@ } msg_cinfo("done.\n");
- if (layout_from_ich_descriptors(chip_layout, desc, 0x1000)) { + if (layout_from_ich_descriptors(&chip_layout, desc, 0x1000)) { msg_cerr("Couldn't parse the descriptor!\n"); ret = 3; goto _finalize_ret; @@ -345,11 +341,11 @@ goto _finalize_ret; }
- const struct romentry *chip_entry = layout_next(&chip_layout->base, NULL); - const struct romentry *dump_entry = layout_next(&dump_layout.base, NULL); + const struct romentry *chip_entry = layout_next(chip_layout, NULL); + const struct romentry *dump_entry = layout_next(dump_layout, NULL); while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) { - chip_entry = layout_next(&chip_layout->base, chip_entry); - dump_entry = layout_next(&dump_layout.base, dump_entry); + chip_entry = layout_next(chip_layout, chip_entry); + dump_entry = layout_next(dump_layout, dump_entry); } if (chip_entry || dump_entry) { msg_cerr("Descriptors don't match!\n"); @@ -365,7 +361,7 @@ finalize_flash_access(flashctx); _free_ret: if (ret) - free(chip_layout); + flashrom_layout_release(chip_layout); free(desc); return ret; #endif diff --git a/libflashrom.h b/libflashrom.h index 2b84bd4..90f3e2c 100644 --- a/libflashrom.h +++ b/libflashrom.h @@ -62,6 +62,7 @@ int flashrom_image_verify(struct flashrom_flashctx *, const void *buffer, size_t buffer_len);
struct flashrom_layout; +int flashrom_layout_empty(struct flashrom_layout **layout, unsigned int count); int flashrom_layout_read_from_ifd(struct flashrom_layout **, struct flashrom_flashctx *, const void *dump, size_t len); int flashrom_layout_read_fmap_from_rom(struct flashrom_layout **, struct flashrom_flashctx *, off_t offset, size_t length);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33543/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33543/1//COMMIT_MSG@10 PS1, Line 10: vanisch oops
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://review.coreboot.org/c/flashrom/+/33543/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/33543/2//COMMIT_MSG@10 PS2, Line 10: vanisch oops indeed
https://review.coreboot.org/c/flashrom/+/33543/2/layout.c File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/2/layout.c@284 PS2, Line 284: out of memory. Very minor: rather, if malloc fails?
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Hello build bot (Jenkins), David Hendricks, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Daniel Campello, Arthur Heymans, Anastasia Klimchuk, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33543
to look at the new patch set (#3).
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
layout: Introduce flashrom_layout_empty()
It initializes an empty layout. Currently the maximum number of entries has to be specified, which will vanish once we use dynamic allocation per entry.
We replace the two special cases `single_layout` and `ich_layout` with dynamically allocated layouts. As a result, we have to take care to release the `default_layout` in a flashctx once we are done with it.
Change-Id: I2ae7246493ff592e631cce924777925c7825e398 Signed-off-by: Nico Huber nico.h@gmx.de --- M cli_classic.c M flash.h M flashrom.c M ich_descriptors.c M ich_descriptors.h M layout.c M libflashrom.c M libflashrom.h 8 files changed, 55 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/33543/3
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
Patch Set 3:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/33543/comment/c0110d32_f852e467 PS1, Line 10: vanisch
oops
Done
Commit Message:
https://review.coreboot.org/c/flashrom/+/33543/comment/b4388b1d_fa7b22db PS2, Line 10: vanisch
oops indeed
Done
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/5b85c158_419071f2 PS2, Line 284: out of memory.
Very minor: rather, if malloc fails?
That would be implementation specific. I don't think the libflashrom API should leak such details.
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
Patch Set 3:
(1 comment)
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/1a83199f_92fd2f30 PS3, Line 353: flashrom_layout_release(second_flashctx.default_layout); It seems error-prone to require the caller to clean up the partially-initialized second_flashctx here. Can probe_flash do that cleanup?
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
Patch Set 3:
(1 comment)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/33543/comment/c3f20a13_5a38d9ff PS3, Line 111: int flashrom_layout_empty(struct flashrom_layout **, unsigned int count); flashrom_layout_new might be clearer, since it's not obvious whether 'empty' means it will create a new (empty) layout or empty an existing layout.
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_empty() ......................................................................
Patch Set 3:
(2 comments)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/33543/comment/6393345c_d19adf4c PS3, Line 111: int flashrom_layout_empty(struct flashrom_layout **, unsigned int count);
flashrom_layout_new might be clearer, since it's not obvious whether 'empty' means it will create a […]
Full ack. I also wondered about this during rebase. :)
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/363fc92e_217af8c4 PS3, Line 353: flashrom_layout_release(second_flashctx.default_layout);
It seems error-prone to require the caller to clean up the partially-initialized second_flashctx her […]
It's actually fully initialized. In a follow-up I add the missing `free(second_flashctx.chip)`. We could add an interim function that does both, I suppose. But in the long run we should probably move the allocation of the flash context into probe_flash(), then flashrom_flash_release() could always be used. What do you think? (would probably be easier if we'd make the classic CLI use the libflashrom API first)
The underlying problem is that the probe_flash() API mixes pre- allocated with dynamically allocated things :-(
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Hello build bot (Jenkins), David Hendricks, Thomas Heijligen, Edward O'Callaghan, Daniel Campello, Angel Pons, Arthur Heymans, Anastasia Klimchuk, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33543
to look at the new patch set (#4).
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
layout: Introduce flashrom_layout_new()
It initializes an empty layout. Currently the maximum number of entries has to be specified, which will vanish once we use dynamic allocation per entry.
We replace the two special cases `single_layout` and `ich_layout` with dynamically allocated layouts. As a result, we have to take care to release the `default_layout` in a flashctx once we are done with it.
Change-Id: I2ae7246493ff592e631cce924777925c7825e398 Signed-off-by: Nico Huber nico.h@gmx.de --- M cli_classic.c M flash.h M flashrom.c M ich_descriptors.c M ich_descriptors.h M layout.c M libflashrom.c M libflashrom.h 8 files changed, 55 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/33543/4
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 4:
(1 comment)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/33543/comment/e50c44af_80e00a09 PS3, Line 111: int flashrom_layout_empty(struct flashrom_layout **, unsigned int count);
Full ack. I also wondered about this during rebase. […]
Done
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 5:
(1 comment)
File flash.h:
https://review.coreboot.org/c/flashrom/+/33543/comment/624bb623_dea047e9 PS5, Line 268: struct flashrom_layout *default_layout; Is single_layout used anywhere else? I see that struct ich_layout is deleted in this patch, but commit message mentions both ich_layout and single_layout.
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 5:
(1 comment)
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/8ccecf50_8dcbf12a PS3, Line 353: flashrom_layout_release(second_flashctx.default_layout);
It's actually fully initialized. In a follow-up I add the missing […]
If we're fighting with probe_flash being entangled with other things then doing this seems okay for now. Being able to use flashrom_flash_release and not knowing the internals of the flashctx seems like the right long-term solution.
Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 5:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/c78dfdc6_16d150b1 PS5, Line 397: malloc use calloc as it will always zero the heap.
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7:
(3 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/33543/comment/a2f73282_0bd27cfe PS5, Line 268: struct flashrom_layout *default_layout;
Is single_layout used anywhere else? I see that struct ich_layout is deleted in this patch, but comm […]
Removed, thanks for spotting!
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/4c64d47c_25a07f3c PS2, Line 284: out of memory.
That would be implementation specific. I don't think the libflashrom […]
Ping?
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/578e0e0d_31156dd7 PS5, Line 397: malloc
use calloc as it will always zero the heap.
Can you elaborate why it would be useful in this case?
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Hello build bot (Jenkins), David Hendricks, Thomas Heijligen, Edward O'Callaghan, Daniel Campello, Angel Pons, Arthur Heymans, Anastasia Klimchuk, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33543
to look at the new patch set (#7).
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
layout: Introduce flashrom_layout_new()
It initializes an empty layout. Currently the maximum number of entries has to be specified, which will vanish once we use dynamic allocation per entry.
We replace the two special cases `single_layout` and `ich_layout` with dynamically allocated layouts. As a result, we have to take care to release the `default_layout` in a flashctx once we are done with it.
Change-Id: I2ae7246493ff592e631cce924777925c7825e398 Signed-off-by: Nico Huber nico.h@gmx.de --- M cli_classic.c M flash.h M flashrom.c M ich_descriptors.c M ich_descriptors.h M layout.c M layout.h M libflashrom.c M libflashrom.h 9 files changed, 55 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/33543/7
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/909089db_94f9e654 PS5, Line 397: malloc
Can you elaborate why it would be useful in this case?
It seems reasonable to be defensive here, to more predictably catch use of uninitialized entries.
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/05012cdb_b4079b14 PS5, Line 397: malloc
It seems reasonable to be defensive here, to more predictably catch use of uninitialized entries.
I am not used to such errors, so I need much more details about the expectations. How does it look like when you catch an unwanted 0-initialization? How is it better compared to some random value? What is the expected effect on programming habits? For instance, I'd expect people to be lulled in false confidence that things are usually 0-initialized (instead of learning C semantics properly). That's just an example; I'd need something that outweighs such concerns, then I'd be happy to support this pattern.
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/daef5314_33e71349 PS5, Line 397: malloc
I am not used to such errors, so I need much more details about […]
My main thought is that it becomes easier to debug failures because they'll crash predictably rather than possibly not crashing or behaving erratically.
Maybe Ed had something else in mind; I don't have a very strong opinion either way, considering it would be well within the realm of environment configuration to ask libc to zero or randomize allocated memory as a debugging option.
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/150e299b_d7a5670b PS5, Line 397: malloc
My main thought is that it becomes easier to debug failures because they'll crash predictably rather than possibly not crashing or behaving erratically.
Are you saying that it's easier to debug failures when zeroing things out because the resulting undefined behavior is more predictable?
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7: Code-Review+2
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/e676029c_50124546 PS5, Line 397: malloc
My main thought is that it becomes easier to debug failures because they'll crash predictably rath […]
Yes, that was the idea. It seems like an area where the performance is not important, so it seems reasonable to zero-initialize since it can prevent bugs or simplify debugging. But ultimately the choice of one or the other isn't very important.
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 7: Code-Review+2
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello. Hello build bot (Jenkins), David Hendricks, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Daniel Campello, Arthur Heymans, Anastasia Klimchuk, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/33543
to look at the new patch set (#8).
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
layout: Introduce flashrom_layout_new()
It initializes an empty layout. Currently the maximum number of entries has to be specified, which will vanish once we use dynamic allocation per entry.
We replace the two special cases `single_layout` and `ich_layout` with dynamically allocated layouts. As a result, we have to take care to release the `default_layout` in a flashctx once we are done with it.
Change-Id: I2ae7246493ff592e631cce924777925c7825e398 Signed-off-by: Nico Huber nico.h@gmx.de --- M cli_classic.c M flash.h M flashrom.c M ich_descriptors.c M ich_descriptors.h M layout.c M layout.h M libflashrom.c M libflashrom.h 9 files changed, 58 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/33543/8
Attention is currently required from: Edward O'Callaghan, Angel Pons, Daniel Campello, Peter Marheine. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 8:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/9f9ba7fa_7cca42de PS5, Line 397: malloc
Yes, that was the idea. […]
I added an initializer now. Should be clear that unmentioned fields are 0 this way. Let me know what you think.
I'd prefer an anonymous struct. But I think it's better to start using new features (C11) after a release.
Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello, Peter Marheine. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 8: Code-Review+2
Attention is currently required from: Nico Huber, Daniel Campello, Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/33543/comment/0eba2f0a_f2e3e222 PS2, Line 284: out of memory.
Ping?
Ack
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/33543 )
Change subject: layout: Introduce flashrom_layout_new() ......................................................................
layout: Introduce flashrom_layout_new()
It initializes an empty layout. Currently the maximum number of entries has to be specified, which will vanish once we use dynamic allocation per entry.
We replace the two special cases `single_layout` and `ich_layout` with dynamically allocated layouts. As a result, we have to take care to release the `default_layout` in a flashctx once we are done with it.
Change-Id: I2ae7246493ff592e631cce924777925c7825e398 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/flashrom/+/33543 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M cli_classic.c M flash.h M flashrom.c M ich_descriptors.c M ich_descriptors.h M layout.c M layout.h M libflashrom.c M libflashrom.h 9 files changed, 58 insertions(+), 42 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c index 158110b..fc91989 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -854,8 +854,10 @@ out_shutdown: programmer_shutdown(); out: - for (i = 0; i < chipcount; i++) + for (i = 0; i < chipcount; i++) { + flashrom_layout_release(flashes[i].default_layout); free(flashes[i].chip); + }
layout_cleanup(&include_args); free(filename); diff --git a/flash.h b/flash.h index dd8e156..3912e73 100644 --- a/flash.h +++ b/flash.h @@ -265,7 +265,7 @@ chipaddr virtual_registers; struct registered_master *mst; const struct flashrom_layout *layout; - struct single_layout fallback_layout; + struct flashrom_layout *default_layout; struct { bool force; bool force_boardmismatch; diff --git a/flashrom.c b/flashrom.c index ffba5c4..7c91fa1 100644 --- a/flashrom.c +++ b/flashrom.c @@ -822,15 +822,12 @@ if (!flash->chip) return -1;
- /* Fill fallback layout covering the whole chip. */ - struct single_layout *const fallback = &flash->fallback_layout; - fallback->base.entries = &fallback->entry; - fallback->base.capacity = 1; - fallback->base.num_entries = 0; - if (flashrom_layout_add_region(&fallback->base, + /* Fill default layout covering the whole chip. */ + if (flashrom_layout_new(&flash->default_layout, 1) || + flashrom_layout_add_region(flash->default_layout, 0, flash->chip->total_size * 1024 - 1, "complete flash") || - flashrom_layout_include_region(&fallback->base, "complete flash")) - return -1; + flashrom_layout_include_region(flash->default_layout, "complete flash")) + return -1;
tmp = flashbuses_to_text(flash->chip->bustype); msg_cinfo("%s %s flash chip "%s" (%d kB, %s) ", force ? "Assuming" : "Found", diff --git a/ich_descriptors.c b/ich_descriptors.c index ad1a88b..5e6c7fb 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -1261,7 +1261,9 @@ * 1 if the descriptor couldn't be parsed, * 2 when out of memory. */ -int layout_from_ich_descriptors(struct ich_layout *const layout, const void *const dump, const size_t len) +int layout_from_ich_descriptors( + struct flashrom_layout **const layout, + const void *const dump, const size_t len) { static const char *const regions[] = { "fd", "bios", "me", "gbe", "pd", "reg5", "bios2", "reg7", "ec", "reg9", "ie", @@ -1277,10 +1279,8 @@ return 1; }
- memset(layout, 0x00, sizeof(*layout)); - layout->base.entries = layout->entries; - layout->base.capacity = ARRAY_SIZE(layout->entries); - layout->base.num_entries = 0; + if (flashrom_layout_new(layout, ARRAY_SIZE(regions))) + return 2;
ssize_t i; const ssize_t nr = MIN(ich_number_of_regions(cs, &desc.content), (ssize_t)ARRAY_SIZE(regions)); @@ -1289,8 +1289,11 @@ const chipoff_t limit = ICH_FREG_LIMIT(desc.region.FLREGs[i]); if (limit <= base) continue; - if (flashrom_layout_add_region(&layout->base, base, limit, regions[i])) + if (flashrom_layout_add_region(*layout, base, limit, regions[i])) { + flashrom_layout_release(*layout); + *layout = NULL; return 2; + } } return 0; } diff --git a/ich_descriptors.h b/ich_descriptors.h index 727a31b..7683ed0 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -564,11 +564,6 @@ struct ich_desc_upper_map upper; };
-struct ich_layout { - struct flashrom_layout base; - struct romentry entries[MAX_NUM_FLREGS]; -}; - ssize_t ich_number_of_regions(enum ich_chipset cs, const struct ich_desc_content *content); ssize_t ich_number_of_masters(enum ich_chipset cs, const struct ich_desc_content *content);
@@ -587,6 +582,6 @@ int read_ich_descriptors_via_fdo(enum ich_chipset cs, void *spibar, struct ich_descriptors *desc); int getFCBA_component_density(enum ich_chipset cs, const struct ich_descriptors *desc, uint8_t idx);
-int layout_from_ich_descriptors(struct ich_layout *, const void *dump, size_t len); +int layout_from_ich_descriptors(struct flashrom_layout **, const void *dump, size_t len);
#endif /* __ICH_DESCRIPTORS_H__ */ diff --git a/layout.c b/layout.c index 2f56b27..ede476a 100644 --- a/layout.c +++ b/layout.c @@ -38,7 +38,7 @@ if (flashctx->layout && flashctx->layout->num_entries) return flashctx->layout; else - return &flashctx->fallback_layout.base; + return flashctx->default_layout; }
static struct romentry *mutable_layout_next( @@ -384,6 +384,32 @@ */
/** + * @brief Create a new, empty layout. + * + * @param layout Pointer to returned layout reference. + * @param count Number of layout entries to allocate. + * + * @return 0 on success, + * 1 if out of memory. + */ +int flashrom_layout_new(struct flashrom_layout **const layout, const unsigned int count) +{ + *layout = malloc(sizeof(**layout) + count * sizeof(struct romentry)); + if (!*layout) { + msg_gerr("Error creating layout: %s\n", strerror(errno)); + return 1; + } + + const struct flashrom_layout tmp = { + .entries = (void *)((char *)*layout + sizeof(**layout)), + .capacity = count, + .num_entries = 0, + }; + **layout = tmp; + return 0; +} + +/** * @brief Add another region to an existing layout. * * @param layout The existing layout. diff --git a/layout.h b/layout.h index 2a0f869..c05d1e7 100644 --- a/layout.h +++ b/layout.h @@ -52,11 +52,6 @@ size_t num_entries; };
-struct single_layout { - struct flashrom_layout base; - struct romentry entry; -}; - struct layout_include_args { char *name; char *file; diff --git a/libflashrom.c b/libflashrom.c index c5c4c89..dd5cb00 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -330,13 +330,14 @@ ret = 0; /* We found one chip, now check that there is no second match. */ if (probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0) != -1) { + flashrom_layout_release(second_flashctx.default_layout); ret = 3; break; } } } if (ret) { - free(*flashctx); + flashrom_flash_release(*flashctx); *flashctx = NULL; } return ret; @@ -360,6 +361,7 @@ */ void flashrom_flash_release(struct flashrom_flashctx *const flashctx) { + flashrom_layout_release(flashctx->default_layout); free(flashctx); }
@@ -435,16 +437,10 @@ #ifndef __FLASHROM_LITTLE_ENDIAN__ return 6; #else - struct ich_layout dump_layout; + struct flashrom_layout *dump_layout, *chip_layout; int ret = 1;
void *const desc = malloc(0x1000); - struct ich_layout *const chip_layout = malloc(sizeof(*chip_layout)); - if (!desc || !chip_layout) { - msg_gerr("Out of memory!\n"); - goto _free_ret; - } - if (prepare_flash_access(flashctx, true, false, false, false)) goto _free_ret;
@@ -457,7 +453,7 @@ } msg_cinfo("done.\n");
- if (layout_from_ich_descriptors(chip_layout, desc, 0x1000)) { + if (layout_from_ich_descriptors(&chip_layout, desc, 0x1000)) { msg_cerr("Couldn't parse the descriptor!\n"); ret = 3; goto _finalize_ret; @@ -470,12 +466,13 @@ goto _finalize_ret; }
- const struct romentry *chip_entry = layout_next(&chip_layout->base, NULL); - const struct romentry *dump_entry = layout_next(&dump_layout.base, NULL); + const struct romentry *chip_entry = layout_next(chip_layout, NULL); + const struct romentry *dump_entry = layout_next(dump_layout, NULL); while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) { - chip_entry = layout_next(&chip_layout->base, chip_entry); - dump_entry = layout_next(&dump_layout.base, dump_entry); + chip_entry = layout_next(chip_layout, chip_entry); + dump_entry = layout_next(dump_layout, dump_entry); } + flashrom_layout_release(dump_layout); if (chip_entry || dump_entry) { msg_cerr("Descriptors don't match!\n"); ret = 5; @@ -490,7 +487,7 @@ finalize_flash_access(flashctx); _free_ret: if (ret) - free(chip_layout); + flashrom_layout_release(chip_layout); free(desc); return ret; #endif diff --git a/libflashrom.h b/libflashrom.h index d04eeb6..b13b3fb 100644 --- a/libflashrom.h +++ b/libflashrom.h @@ -106,6 +106,7 @@ int flashrom_image_verify(struct flashrom_flashctx *, const void *buffer, size_t buffer_len);
struct flashrom_layout; +int flashrom_layout_new(struct flashrom_layout **, unsigned int count); int flashrom_layout_read_from_ifd(struct flashrom_layout **, struct flashrom_flashctx *, const void *dump, size_t len); int flashrom_layout_read_fmap_from_rom(struct flashrom_layout **, struct flashrom_flashctx *, off_t offset, size_t length);
-
Anastasia Klimchuk (Code Review) -
Angel Pons (Code Review)
-
Edward O'Callaghan (Code Review)
-
Nico Huber (Code Review)
-
Peter Marheine (Code Review)