October 2018 - flashrom-gerrit

Change in flashrom[master]: flashchips: Add IS25LP256 and IS25WP256
by Nico Huber (Code Review) 29 Oct '18

29 Oct '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/29306 ) Change subject: flashchips: Add IS25LP256 and IS25WP256 ...................................................................... Patch Set 1: Code-Review+2 (2 comments) https://review.coreboot.org/#/c/29306/1/flashchips.c File flashchips.c: https://review.coreboot.org/#/c/29306/1/flashchips.c@7387 PS1, Line 7387: /* FOUR_BYTE_ADDR: supports 4-bytes addressing mode */ doesn't add any information https://review.coreboot.org/#/c/29306/1/flashchips.c@7399 PS1, Line 7399: .block_erase = spi_block_erase_20, Also aliased as d7. I wouldn't mind if you don't want to add it. But a comment would be nice. -- To view, visit https://review.coreboot.org/29306 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf7a224abcde5f7935d9ef88309f78207de60a7a Gerrit-Change-Number: 29306 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 29 Oct 2018 20:57:20 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: flashchips: Add W25Q256JV support
by Nico Huber (Code Review) 29 Oct '18

29 Oct '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/29305 ) Change subject: flashchips: Add W25Q256JV support ...................................................................... Patch Set 1: Code-Review+2 (1 comment) https://review.coreboot.org/#/c/29305/1/flashchips.c File flashchips.c: https://review.coreboot.org/#/c/29305/1/flashchips.c@15561 PS1, Line 15561: /* FOUR_BYTE_ADDR: supports 4-bytes addressing mode */ doesn't add any information (I guess the copied comment above is only a leftover) -- To view, visit https://review.coreboot.org/29305 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76d7362777d364594d2a733d7e478741b0bef7c4 Gerrit-Change-Number: 29305 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 29 Oct 2018 20:42:05 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: flashchips: Add IS25LP256 and IS25WP256
by David Hendricks (Code Review) 28 Oct '18

28 Oct '18

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/29306 Change subject: flashchips: Add IS25LP256 and IS25WP256 ...................................................................... flashchips: Add IS25LP256 and IS25WP256 Tested IS25LP256 using Raspberry Pi and Dediprog SF600 programmers. Tested IS25WP256 using Dediprog SF600. Change-Id: Idf7a224abcde5f7935d9ef88309f78207de60a7a Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> --- M flashchips.c M flashchips.h 2 files changed, 100 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/29306/1 diff --git a/flashchips.c b/flashchips.c index 8065283..bf56048 100644 --- a/flashchips.c +++ b/flashchips.c @@ -7376,6 +7376,55 @@ { .vendor = "ISSI", + .name = "IS25LP256", + .bustype = BUS_SPI, + .manufacture_id = ISSI_ID_SPI, + .model_id = ISSI_IS25LP256, + .total_size = 32768, + .page_size = 256, + /* supports SFDP */ + /* OTP: 1024B total; read 0x68; write 0x62, erase 0x64, read ID 0x4B */ + /* FOUR_BYTE_ADDR: supports 4-bytes addressing mode */ + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA | FEATURE_4BA_ENTER_EAR7, + .tested = TEST_OK_PREW, + .probe = probe_spi_rdid, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 8192} }, + .block_erase = spi_block_erase_21, + }, { + .eraseblocks = { {4 * 1024, 8192} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 1024} }, + .block_erase = spi_block_erase_5c, + }, { + .eraseblocks = { {32 * 1024, 1024} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 512} }, + .block_erase = spi_block_erase_dc, + }, { + .eraseblocks = { {64 * 1024, 512} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {32 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {32 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, + .unlock = spi_disable_blockprotect, + .write = spi_chip_write_256, + .read = spi_chip_read, + .voltage = {2300, 3600}, + }, + + { + .vendor = "ISSI", .name = "IS25WP032", .bustype = BUS_SPI, .manufacture_id = ISSI_ID_SPI, @@ -7502,6 +7551,55 @@ { .vendor = "ISSI", + .name = "IS25WP256", + .bustype = BUS_SPI, + .manufacture_id = ISSI_ID_SPI, + .model_id = ISSI_IS25WP256, + .total_size = 32768, + .page_size = 256, + /* supports SFDP */ + /* OTP: 1024B total; read 0x68; write 0x62, erase 0x64, read ID 0x4B */ + /* FOUR_BYTE_ADDR: supports 4-bytes addressing mode */ + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA | FEATURE_4BA_ENTER_EAR7, + .tested = TEST_OK_PREW, + .probe = probe_spi_rdid, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 8192} }, + .block_erase = spi_block_erase_21, + }, { + .eraseblocks = { {4 * 1024, 8192} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 1024} }, + .block_erase = spi_block_erase_5c, + }, { + .eraseblocks = { {32 * 1024, 1024} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 512} }, + .block_erase = spi_block_erase_dc, + }, { + .eraseblocks = { {64 * 1024, 512} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {32 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {32 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, + .unlock = spi_disable_blockprotect, + .write = spi_chip_write_256, + .read = spi_chip_read, + .voltage = {1650, 1950}, + }, + + { + .vendor = "ISSI", .name = "IS29GL064B", .bustype = BUS_PARALLEL, .manufacture_id = ISSI_ID, diff --git a/flashchips.h b/flashchips.h index 218cb5b..310056e 100644 --- a/flashchips.h +++ b/flashchips.h @@ -456,9 +456,11 @@ #define ISSI_ID_SPI 0x9D /* ISSI ID used for SPI flash, see also PMC_ID_NOPREFIX */ #define ISSI_IS25LP064 0x6017 #define ISSI_IS25LP128 0x6018 +#define ISSI_IS25LP256 0x6019 #define ISSI_IS25WP032 0x7016 #define ISSI_IS25WP064 0x7017 #define ISSI_IS25WP128 0x7018 +#define ISSI_IS25WP256 0x7019 #define ISSI_PMC_IS29GL032B 0xF9 #define ISSI_PMC_IS29GL032T 0xF6 #define ISSI_PMC_IS29GL064B 0x7E1000 -- To view, visit https://review.coreboot.org/29306 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Idf7a224abcde5f7935d9ef88309f78207de60a7a Gerrit-Change-Number: 29306 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>

1 0

Change in flashrom[master]: [WIP]dediprog: Implement 4-byte-address support
by David Hendricks (Code Review) 28 Oct '18

28 Oct '18

David Hendricks has posted comments on this change. ( https://review.coreboot.org/28804 ) Change subject: [WIP]dediprog: Implement 4-byte-address support ...................................................................... Patch Set 1: Code-Review-1 (1 comment) I was able to test this and it works with one small change (see comments). I used an SF600 with firmware 7.2.21, with W25Q256JVFM and MX25L25735FMI and wrote to 1MB ranges in the lower and upper halves of the chip. https://review.coreboot.org/#/c/28804/1/dediprog.c File dediprog.c: https://review.coreboot.org/#/c/28804/1/dediprog.c@399 PS1, Line 399: data_packet[3] = WRITE_MODE_4B_ADDR_256B_PAGE_PGM_0x12 For some reason I cannot explain, this needs to be WRITE_MODE_4B_ADDR_256B_PAGE_PGM. That is the only way I have been able to successfully write using native 4BA commands. I am not sure what WRITE_MODE_4B_ADDR_256B_PAGE_PGM_0x12 is supposed to do... -- To view, visit https://review.coreboot.org/28804 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I665d0806aec469a3509620a760815861fbe22841 Gerrit-Change-Number: 28804 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Ronald G. Minnich <rminnich(a)gmail.com> Gerrit-Comment-Date: Sun, 28 Oct 2018 00:14:48 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: flashchips: Add W25Q256JV support
by David Hendricks (Code Review) 27 Oct '18

27 Oct '18

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/29305 Change subject: flashchips: Add W25Q256JV support ...................................................................... flashchips: Add W25Q256JV support Similar to W25Q256FV, but it supports the native 4BA page program instruction (12h). Note that the variant with QE enabled by default shares the device ID of the W25Q256FV. Tested using a Raspberry Pi. Change-Id: I76d7362777d364594d2a733d7e478741b0bef7c4 Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> --- M flashchips.c M flashchips.h 2 files changed, 49 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/29305/1 diff --git a/flashchips.c b/flashchips.c index 8065283..bbd5cad 100644 --- a/flashchips.c +++ b/flashchips.c @@ -15550,6 +15550,53 @@ { .vendor = "Winbond", + .name = "W25Q256JV_M", + .bustype = BUS_SPI, + .manufacture_id = WINBOND_NEX_ID, + .model_id = WINBOND_NEX_W25Q256JV_M, + .total_size = 32768, + .page_size = 256, + /* supports SFDP */ + /* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44, read ID 0x4B */ + /* FOUR_BYTE_ADDR: supports 4-bytes addressing mode */ + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA, + .tested = TEST_OK_PREW, + .probe = probe_spi_rdid, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 8192} }, + .block_erase = spi_block_erase_21, + }, { + .eraseblocks = { {4 * 1024, 8192} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 1024} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 512} }, + .block_erase = spi_block_erase_dc, + }, { + .eraseblocks = { {64 * 1024, 512} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {32 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {32 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, + .printlock = spi_prettyprint_status_register_plain, /* TODO: improve */ + .unlock = spi_disable_blockprotect, + .write = spi_chip_write_256, + .read = spi_chip_read, + .voltage = {2700, 3600}, + }, + + { + .vendor = "Winbond", .name = "W25Q20.W", .bustype = BUS_SPI, .manufacture_id = WINBOND_NEX_ID, diff --git a/flashchips.h b/flashchips.h index 218cb5b..429acef 100644 --- a/flashchips.h +++ b/flashchips.h @@ -916,7 +916,7 @@ #define WINBOND_NEX_W25Q32_V 0x4016 /* W25Q32BV; W25Q32FV in SPI mode (default) */ #define WINBOND_NEX_W25Q64_V 0x4017 /* W25Q64BV, W25Q64CV; W25Q64FV in SPI mode (default) */ #define WINBOND_NEX_W25Q128_V 0x4018 /* W25Q128BV; W25Q128FV in SPI mode (default) */ -#define WINBOND_NEX_W25Q256_V 0x4019 /* W25Q256FV */ +#define WINBOND_NEX_W25Q256_V 0x4019 /* W25Q256FV or W25Q256JV_Q (QE=1) */ #define WINBOND_NEX_W25Q20_W 0x5012 /* W25Q20BW */ #define WINBOND_NEX_W25Q40BW 0x5013 /* W25Q40BW */ #define WINBOND_NEX_W25Q80BW 0x5014 /* W25Q80BW */ @@ -927,6 +927,7 @@ #define WINBOND_NEX_W25Q64_W 0x6017 /* W25Q64DW; W25Q64FV in QPI mode */ #define WINBOND_NEX_W25Q128_W 0x6018 /* W25Q128FW; W25Q128FV in QPI mode */ #define WINBOND_NEX_W25Q128_V_M 0x7018 /* W25Q128JVSM */ +#define WINBOND_NEX_W25Q256JV_M 0x7019 /* W25Q256JV_M (QE=0) */ #define WINBOND_ID 0xDA /* Winbond */ #define WINBOND_W19B160BB 0x49 -- To view, visit https://review.coreboot.org/29305 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I76d7362777d364594d2a733d7e478741b0bef7c4 Gerrit-Change-Number: 29305 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>

1 0

Change in flashrom[master]: add newer pci infrastructure
by Luc Verhaegen (Code Review) 23 Oct '18

23 Oct '18

Luc Verhaegen has posted comments on this change. ( https://review.coreboot.org/29078 ) Change subject: add newer pci infrastructure ...................................................................... Patch Set 2: (15 comments) https://review.coreboot.org/#/c/29078/2//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@9 PS2, Line 9: This code does not negatively affect existing pci support and existing : pci devices, > This reads like "This can't hurt, please wave through.", please […] Has nothing to do with waving through, but everything to do with "will not touch nor break support based on the other infrastructure". No existing drivers need to be force ported. https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@16 PS2, Line 16: but means that support for other operating systems still : needs to be cobbled together. > Why not fall back to the existing dev-mem code on other OS'? The base device detection struct has changed. This is central to the existing /dev/mem code. As it stands now, it is limited to linux and to just the one programmer that uses it. Anyone can come along and fix either. We have not broken existing programmers, and we should not have broken support for other platforms. Attempting to do a more thorough job of this quickly dissolves into a combination of: * a porting nightmare of existing programmers * a porting nightmare to all supported platforms Which in turn would completely deadlock this and future infrastructural changes. This way, we get to add a sensible ati_spi programmer with 240 supported pci devices (today), in a reasonable timeframe with reasonable infrastructure. Users of other platforms and/or other programmers can follow as and when they have the time. https://review.coreboot.org/#/c/29078/2/pcidev.c File pcidev.c: https://review.coreboot.org/#/c/29078/2/pcidev.c@354 PS2, Line 354: */ > What is with these empty comments? Please remove (or fill). I have been using this like forever to both have a visual marker for function start, and to attempt to help me fill it in in future. Can be removed as stated. https://review.coreboot.org/#/c/29078/2/pcidev.c@356 PS2, Line 356: void * > This makes C's type checking even weaker. Why not declare the parameter […] To be able to use register_shutdown() on this directly. compiler messages: error: passing argument 1 of 'xxx_shutdown' from incompatible pointer type [-Werror] note: expected 'int (*)(void *)' but argument is of type 'int (*)(struct flashrom_pci_device *)' https://review.coreboot.org/#/c/29078/2/pcidev.c@361 PS2, Line 361: cleanup > `clean up` Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@367 PS2, Line 367: device->sysfs_path = NULL; > What for? we release it before it could be dereferenced again. Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@404 PS2, Line 404: return NULL; > leaking `pcidev_bdf` Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@410 PS2, Line 410: for (dev = pacc->devices; dev; dev = dev->next) > Please always use braces if multiple lines follow. Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@411 PS2, Line 411: if (pci_filter_match(&filter, dev)) { > Please turn into […] Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@412 PS2, Line 412: > no space before semicolon Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@424 PS2, Line 424: if (name) > Please use a conditional expression or just: […] Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@452 PS2, Line 452: card > s/card/device/ ? it doesn't have to be a "card" Direct copy-paste from above. https://review.coreboot.org/#/c/29078/2/pcidev.c@465 PS2, Line 465: - 1, > `- 1` is not necessary Ack https://review.coreboot.org/#/c/29078/2/pcidev.c@467 PS2, Line 467: 0 > found_dev->domain ? Ack https://review.coreboot.org/#/c/29078/2/programmer.h File programmer.h: https://review.coreboot.org/#/c/29078/2/programmer.h@852 PS2, Line 852: #if NEED_PCI == 1 > Why guard? are there conflicting alternative definitions? Iirc, struct pci_dev only exists when NEED_PCI is defined. Not sure whether there is a placeholder for that outside of NEED_PCI. -- To view, visit https://review.coreboot.org/29078 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59af37cba5cb78014e0714c654db2501151f605e Gerrit-Change-Number: 29078 Gerrit-PatchSet: 2 Gerrit-Owner: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Tue, 23 Oct 2018 22:47:41 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: add newer pci infrastructure
by Nico Huber (Code Review) 23 Oct '18

23 Oct '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/29078 ) Change subject: add newer pci infrastructure ...................................................................... Patch Set 2: (15 comments) Just a kick-off, this will take time. Didn't look ahead yet, so I might still have comments on the semantics later. The course looks good. https://review.coreboot.org/#/c/29078/2//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@9 PS2, Line 9: This code does not negatively affect existing pci support and existing : pci devices, This reads like "This can't hurt, please wave through.", please remove/rephrase. https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@16 PS2, Line 16: but means that support for other operating systems still : needs to be cobbled together. Why not fall back to the existing dev-mem code on other OS'? https://review.coreboot.org/#/c/29078/2/pcidev.c File pcidev.c: https://review.coreboot.org/#/c/29078/2/pcidev.c@354 PS2, Line 354: */ What is with these empty comments? Please remove (or fill). https://review.coreboot.org/#/c/29078/2/pcidev.c@356 PS2, Line 356: void * This makes C's type checking even weaker. Why not declare the parameter as `struct flashrom_pci_device *`? You could still pass a `void *`; just not any pointer. https://review.coreboot.org/#/c/29078/2/pcidev.c@361 PS2, Line 361: cleanup `clean up` https://review.coreboot.org/#/c/29078/2/pcidev.c@367 PS2, Line 367: device->sysfs_path = NULL; What for? we release it before it could be dereferenced again. https://review.coreboot.org/#/c/29078/2/pcidev.c@404 PS2, Line 404: return NULL; leaking `pcidev_bdf` https://review.coreboot.org/#/c/29078/2/pcidev.c@410 PS2, Line 410: for (dev = pacc->devices; dev; dev = dev->next) Please always use braces if multiple lines follow. https://review.coreboot.org/#/c/29078/2/pcidev.c@411 PS2, Line 411: if (pci_filter_match(&filter, dev)) { Please turn into if (!pci_filter_match(&filter, dev)) continue; to save the extra indentation level. https://review.coreboot.org/#/c/29078/2/pcidev.c@412 PS2, Line 412: no space before semicolon https://review.coreboot.org/#/c/29078/2/pcidev.c@424 PS2, Line 424: if (name) Please use a conditional expression or just: if (!name) name = "<unknown pciids>"; which should also fix the strdup() call below. https://review.coreboot.org/#/c/29078/2/pcidev.c@452 PS2, Line 452: card s/card/device/ ? it doesn't have to be a "card" https://review.coreboot.org/#/c/29078/2/pcidev.c@465 PS2, Line 465: - 1, `- 1` is not necessary https://review.coreboot.org/#/c/29078/2/pcidev.c@467 PS2, Line 467: 0 found_dev->domain ? https://review.coreboot.org/#/c/29078/2/programmer.h File programmer.h: https://review.coreboot.org/#/c/29078/2/programmer.h@852 PS2, Line 852: #if NEED_PCI == 1 Why guard? are there conflicting alternative definitions? -- To view, visit https://review.coreboot.org/29078 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59af37cba5cb78014e0714c654db2501151f605e Gerrit-Change-Number: 29078 Gerrit-PatchSet: 2 Gerrit-Owner: Luc Verhaegen <libv(a)skynet.be> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Tue, 23 Oct 2018 22:07:01 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: dmi: Remove nonsense guard; Makefile handles it
by Nico Huber (Code Review) 23 Oct '18

23 Oct '18

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/28715 ) Change subject: dmi: Remove nonsense guard; Makefile handles it ...................................................................... dmi: Remove nonsense guard; Makefile handles it Change-Id: If4216be1f9ed308e4580c36d0356480e637ffc82 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/28715 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> --- M dmi.c 1 file changed, 0 insertions(+), 4 deletions(-) Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved diff --git a/dmi.c b/dmi.c index 4137821..79321ce 100644 --- a/dmi.c +++ b/dmi.c @@ -34,8 +34,6 @@ #include "flash.h" #include "programmer.h" -#if defined(__i386__) || defined(__x86_64__) - /* Enable SMBIOS decoding. Currently legacy DMI decoding is enough. */ #define SM_SUPPORT 0 @@ -481,5 +479,3 @@ return 0; } - -#endif // defined(__i386__) || defined(__x86_64__) -- To view, visit https://review.coreboot.org/28715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If4216be1f9ed308e4580c36d0356480e637ffc82 Gerrit-Change-Number: 28715 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: flashchips: Mark MX25L25635F as tested
by Nico Huber (Code Review) 23 Oct '18

23 Oct '18

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/29202 ) Change subject: flashchips: Mark MX25L25635F as tested ...................................................................... flashchips: Mark MX25L25635F as tested As reported by Kasper Revsbech on 2018-10-19. Change-Id: Icf05288c4e7e34af2e3f4b951457df695078847d Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/29202 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/flashchips.c b/flashchips.c index 4a66176..8065283 100644 --- a/flashchips.c +++ b/flashchips.c @@ -8473,7 +8473,7 @@ .page_size = 256, /* OTP: 512B total; enter 0xB1, exit 0xC1 */ .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PREW, .probe = probe_spi_rdid, .probe_timing = TIMING_ZERO, .block_erasers = -- To view, visit https://review.coreboot.org/29202 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Icf05288c4e7e34af2e3f4b951457df695078847d Gerrit-Change-Number: 29202 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: Add support to get layout from fmap (e.g. coreboot rom)
by Nico Huber (Code Review) 23 Oct '18

23 Oct '18

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/23203 ) Change subject: Add support to get layout from fmap (e.g. coreboot rom) ...................................................................... Add support to get layout from fmap (e.g. coreboot rom) Flashmap, or simply fmap, is a binary data format for describing region offsets, sizes, and certain attributes and is widely used by coreboot. This patch adds support for the fmap data format version 1.1 and adds --fmap and --fmap-file arguments. Using --fmap will make flashrom to search the ROM content for fmap data. Using --fmap-file will make flashrom search a supplied file for fmap data. An example of how to update the COREBOOT region of a ROM: flashrom -p programmer --fmap -w coreboot.rom -i COREBOOT flashrom -p programmer --fmap-file coreboot.rom -w coreboot.rom -i COREBOOT The fmap functions are mostly copied from cbfstool. Currently it is made mutually exclusive with other layout options until we are more clever about this input. Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Signed-off-by: David Hendricks <dhendricks(a)fb.com> Reviewed-on: https://review.coreboot.org/23203 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Tested-by: David Hendricks <david.hendricks(a)gmail.com> Reviewed-by: Werner Zeh <werner.zeh(a)siemens.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M Makefile M cli_classic.c M flashrom.8.tmpl A fmap.c A fmap.h M libflashrom.c M libflashrom.h 7 files changed, 648 insertions(+), 10 deletions(-) Approvals: build bot (Jenkins): Verified David Hendricks: Verified Nico Huber: Looks good to me, approved Werner Zeh: Looks good to me, approved diff --git a/Makefile b/Makefile index b13cf7e..1ff578c 100644 --- a/Makefile +++ b/Makefile @@ -536,7 +536,7 @@ ############################################################################### # Library code. -LIB_OBJS = libflashrom.o layout.o flashrom.o udelay.o programmer.o helpers.o ich_descriptors.o +LIB_OBJS = libflashrom.o layout.o flashrom.o udelay.o programmer.o helpers.o ich_descriptors.o fmap.o ############################################################################### # Frontend related stuff. diff --git a/cli_classic.c b/cli_classic.c index 119c6f8..df9fa67 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -25,6 +25,7 @@ #include <getopt.h> #include "flash.h" #include "flashchips.h" +#include "fmap.h" #include "programmer.h" #include "libflashrom.h" @@ -53,6 +54,8 @@ " -n | --noverify don't auto-verify\n" " -N | --noverify-all verify included regions only (cf. -i)\n" " -l | --layout <layoutfile> read ROM layout from <layoutfile>\n" + " --fmap read ROM layout from fmap embedded in ROM\n" + " --fmap-file <fmapfile> read ROM layout from fmap in <fmapfile>\n" " --ifd read layout from an Intel Firmware Descriptor\n" " -i | --image <name> only flash image <name> from flash layout\n" " -o | --output <logfile> log output to <logfile>\n" @@ -97,7 +100,7 @@ struct flashctx *fill_flash; const char *name; int namelen, opt, i, j; - int startchip = -1, chipcount = 0, option_index = 0, force = 0, ifd = 0; + int startchip = -1, chipcount = 0, option_index = 0, force = 0, ifd = 0, fmap = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; #endif @@ -107,6 +110,8 @@ enum programmer prog = PROGRAMMER_INVALID; enum { OPTION_IFD = 0x0100, + OPTION_FMAP, + OPTION_FMAP_FILE, OPTION_FLASH_CONTENTS, }; int ret = 0; @@ -124,6 +129,8 @@ {"force", 0, NULL, 'f'}, {"layout", 1, NULL, 'l'}, {"ifd", 0, NULL, OPTION_IFD}, + {"fmap", 0, NULL, OPTION_FMAP}, + {"fmap-file", 1, NULL, OPTION_FMAP_FILE}, {"image", 1, NULL, 'i'}, {"flash-contents", 1, NULL, OPTION_FLASH_CONTENTS}, {"list-supported", 0, NULL, 'L'}, @@ -138,6 +145,7 @@ char *filename = NULL; char *referencefile = NULL; char *layoutfile = NULL; + char *fmapfile = NULL; #ifndef STANDALONE char *logfile = NULL; #endif /* !STANDALONE */ @@ -230,6 +238,10 @@ fprintf(stderr, "Error: --layout and --ifd both specified. Aborting.\n"); cli_classic_abort_usage(); } + if (fmap) { + fprintf(stderr, "Error: --layout and --fmap-file both specified. Aborting.\n"); + cli_classic_abort_usage(); + } layoutfile = strdup(optarg); break; case OPTION_IFD: @@ -237,8 +249,45 @@ fprintf(stderr, "Error: --layout and --ifd both specified. Aborting.\n"); cli_classic_abort_usage(); } + if (fmap) { + fprintf(stderr, "Error: --fmap-file and --ifd both specified. Aborting.\n"); + cli_classic_abort_usage(); + } ifd = 1; break; + case OPTION_FMAP_FILE: + if (fmap) { + fprintf(stderr, "Error: --fmap or --fmap-file specified " + "more than once. Aborting.\n"); + cli_classic_abort_usage(); + } + if (ifd) { + fprintf(stderr, "Error: --fmap-file and --ifd both specified. Aborting.\n"); + cli_classic_abort_usage(); + } + if (layoutfile) { + fprintf(stderr, "Error: --fmap-file and --layout both specified. Aborting.\n"); + cli_classic_abort_usage(); + } + fmapfile = strdup(optarg); + fmap = 1; + break; + case OPTION_FMAP: + if (fmap) { + fprintf(stderr, "Error: --fmap or --fmap-file specified " + "more than once. Aborting.\n"); + cli_classic_abort_usage(); + } + if (ifd) { + fprintf(stderr, "Error: --fmap and --ifd both specified. Aborting.\n"); + cli_classic_abort_usage(); + } + if (layoutfile) { + fprintf(stderr, "Error: --layout and --fmap both specified. Aborting.\n"); + cli_classic_abort_usage(); + } + fmap = 1; + break; case 'i': tempstr = strdup(optarg); if (register_include_arg(tempstr)) { @@ -360,6 +409,9 @@ if (layoutfile && check_filename(layoutfile, "layout")) { cli_classic_abort_usage(); } + if (fmapfile && check_filename(fmapfile, "fmap")) { + cli_classic_abort_usage(); + } if (referencefile && check_filename(referencefile, "reference")) { cli_classic_abort_usage(); } @@ -399,7 +451,8 @@ ret = 1; goto out; } - if (!ifd && process_include_args(get_global_layout())) { + + if (!ifd && !fmap && process_include_args(get_global_layout())) { ret = 1; goto out; } @@ -558,6 +611,38 @@ process_include_args(layout))) { ret = 1; goto out_shutdown; + } else if (fmap && fmapfile) { + struct stat s; + if (stat(fmapfile, &s) != 0) { + msg_gerr("Failed to stat fmapfile \"%s\"\n", fmapfile); + ret = 1; + goto out_shutdown; + } + + size_t fmapfile_size = s.st_size; + uint8_t *fmapfile_buffer = malloc(fmapfile_size); + if (!fmapfile_buffer) { + ret = 1; + goto out_shutdown; + } + + if (read_buf_from_file(fmapfile_buffer, fmapfile_size, fmapfile)) { + ret = 1; + free(fmapfile_buffer); + goto out_shutdown; + } + + if (flashrom_layout_read_fmap_from_buffer(&layout, fill_flash, fmapfile_buffer, fmapfile_size) || + process_include_args(layout)) { + ret = 1; + free(fmapfile_buffer); + goto out_shutdown; + } + free(fmapfile_buffer); + } else if (fmap && (flashrom_layout_read_fmap_from_rom(&layout, fill_flash, 0, + fill_flash->chip->total_size * 1024) || process_include_args(layout))) { + ret = 1; + goto out_shutdown; } flashrom_layout_set(fill_flash, layout); diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index f882dc6..c557af7 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -44,12 +44,10 @@ .SH NAME flashrom \- detect, read, write, verify and erase flash chips .SH SYNOPSIS -.B flashrom \fR[\fB\-h\fR|\fB\-R\fR|\fB\-L\fR|\fB\-z\fR|\ -\fB\-p\fR <programmername>[:<parameters>] - [\fB\-E\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] \ -[\fB\-c\fR <chipname>] - [(\fB\-l\fR <file>|\fB\-\-ifd\fR) [\fB\-i\fR <image>]] \ -[\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR]] +.B flashrom \fR[\fB\-h\fR|\fB\-R\fR|\fB\-L\fR|\fB\-z\fR|\fB\-p\fR <programmername>[:<parameters>] + [\fB\-E\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] [\fB\-c\fR <chipname>] + [(\fB\-l\fR <file>|\fB\-\-ifd|\fB \-\-fmap\fR|\fB\-\-fmap-file\fR <file>) [\fB\-i\fR <image>]] + [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR]] [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] .SH DESCRIPTION .B flashrom @@ -195,6 +193,34 @@ .sp Overlapping sections are not supported. .TP +.B "\-\-fmap" +Read layout from fmap in flash chip. +.sp +flashrom supports the fmap binary format which is commonly used by coreboot +for partitioning a flash chip. The on-chip fmap will be read and used to generate +the layout. +.sp +If you only want to update the +.BR "COREBOOT" +region defined in the fmap, run +.sp +.B " flashrom -p prog \-\-fmap \-\-image COREBOOT \-w some.rom" +.TP +.B "\-\-fmap-file <file>" +Read layout from a +.BR <file> +containing binary fmap (e.g. coreboot roms). +.sp +flashrom supports the fmap binary format which is commonly used by coreboot +for partitioning a flash chip. The fmap in the specified file will be read and +used to generate the layout. +.sp +If you only want to update the +.BR "COREBOOT" +region defined in the binary fmap file, run +.sp +.B " flashrom \-p prog \-\-fmap-file some.rom \-\-image COREBOOT \-w some.rom" +.TP .B "\-\-ifd" Read ROM layout from Intel Firmware Descriptor. .sp diff --git a/fmap.c b/fmap.c new file mode 100644 index 0000000..d44b7fa --- /dev/null +++ b/fmap.c @@ -0,0 +1,333 @@ +/* + * Copyright 2015, Google Inc. + * Copyright 2018-present, Facebook Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + */ + +#include <ctype.h> +#include <stdlib.h> +#include <string.h> +#include <sys/types.h> +#include "flash.h" +#include "fmap.h" + +static size_t fmap_size(const struct fmap *fmap) +{ + return sizeof(*fmap) + (fmap->nareas * sizeof(struct fmap_area)); +} + +static int is_valid_fmap(const struct fmap *fmap) +{ + if (memcmp(fmap, FMAP_SIGNATURE, strlen(FMAP_SIGNATURE)) != 0) + return 0; + /* strings containing the magic tend to fail here */ + if (fmap->ver_major > FMAP_VER_MAJOR) + return 0; + if (fmap->ver_minor > FMAP_VER_MINOR) + return 0; + /* a basic consistency check: flash address space size should be larger + * than the size of the fmap data structure */ + if (fmap->size < fmap_size(fmap)) + return 0; + + /* fmap-alikes along binary data tend to fail on having a valid, + * null-terminated string in the name field.*/ + int i; + for (i = 0; i < FMAP_STRLEN; i++) { + if (fmap->name[i] == 0) + break; + if (!isgraph(fmap->name[i])) + return 0; + if (i == FMAP_STRLEN - 1) { + /* name is specified to be null terminated single-word string + * without spaces. We did not break in the 0 test, we know it + * is a printable spaceless string but we're seeing FMAP_STRLEN + * symbols, which is one too many. + */ + return 0; + } + } + return 1; + +} + +/** + * @brief Do a brute-force linear search for fmap in provided buffer + * + * @param[in] buffer The buffer to search + * @param[in] len Length (in bytes) to search + * + * @return offset in buffer where fmap is found if successful + * -1 to indicate that fmap was not found + * -2 to indicate fmap is truncated or exceeds buffer + len + */ +static off_t fmap_lsearch(const uint8_t *buf, size_t len) +{ + off_t offset; + bool fmap_found = 0; + + for (offset = 0; offset <= len - sizeof(struct fmap); offset++) { + if (is_valid_fmap((struct fmap *)&buf[offset])) { + fmap_found = 1; + break; + } + } + + if (!fmap_found) + return -1; + + if (offset + fmap_size((struct fmap *)&buf[offset]) > len) { + msg_gerr("fmap size exceeds buffer boundary.\n"); + return -2; + } + + return offset; +} + +/** + * @brief Read fmap from provided buffer and copy it to fmap_out + * + * @param[out] fmap_out Double-pointer to location to store fmap contents. + * Caller must free allocated fmap contents. + * @param[in] buf Buffer to search + * @param[in] len Length (in bytes) to search + * + * @return 0 if successful + * 1 to indicate error + * 2 to indicate fmap is not found + */ +int fmap_read_from_buffer(struct fmap **fmap_out, const uint8_t *const buf, size_t len) +{ + off_t offset = fmap_lsearch(buf, len); + if (offset < 0) { + msg_gdbg("Unable to find fmap in provided buffer.\n"); + return 2; + } else { + msg_gdbg("Found fmap at offset 0x%06zx\n", (size_t)offset); + } + + const struct fmap *fmap = (const struct fmap *)(buf + offset); + *fmap_out = malloc(fmap_size(fmap)); + if (*fmap_out == NULL) { + msg_gerr("Out of memory.\n"); + return 1; + } + + memcpy(*fmap_out, fmap, fmap_size(fmap)); + return 0; +} + +static int fmap_lsearch_rom(struct fmap **fmap_out, + struct flashctx *const flashctx, size_t rom_offset, size_t len) +{ + int ret = -1; + uint8_t *buf; + + if (prepare_flash_access(flashctx, true, false, false, false)) + goto _finalize_ret; + + /* likely more memory than we need, but it simplifies handling and + * printing offsets to keep them uniform with what's on the ROM */ + buf = malloc(rom_offset + len); + if (!buf) { + msg_gerr("Out of memory.\n"); + goto _finalize_ret; + } + + ret = flashctx->chip->read(flashctx, buf + rom_offset, rom_offset, len); + if (ret) { + msg_pdbg("Cannot read ROM contents.\n"); + goto _free_ret; + } + + ret = fmap_read_from_buffer(fmap_out, buf + rom_offset, len); +_free_ret: + free(buf); +_finalize_ret: + finalize_flash_access(flashctx); + return ret; +} + +static int fmap_bsearch_rom(struct fmap **fmap_out, struct flashctx *const flashctx, + size_t rom_offset, size_t len, int min_stride) +{ + size_t stride, fmap_len = 0; + int ret = 1, fmap_found = 0, check_offset_0 = 1; + struct fmap *fmap; + const unsigned int chip_size = flashctx->chip->total_size * 1024; + const int sig_len = strlen(FMAP_SIGNATURE); + + if (rom_offset + len > flashctx->chip->total_size * 1024) + return 1; + + if (len < sizeof(*fmap)) + return 1; + + if (prepare_flash_access(flashctx, true, false, false, false)) + return 1; + + fmap = malloc(sizeof(struct fmap)); + if (!fmap) { + msg_gerr("Out of memory.\n"); + goto _free_ret; + } + + /* + * For efficient operation, we start with the largest stride possible + * and then decrease the stride on each iteration. Also, check for a + * remainder when modding the offset with the previous stride. This + * makes it so that each offset is only checked once. + * + * Zero (rom_offset == 0) is a special case and is handled using a + * variable to track whether or not we've checked it. + */ + size_t offset; + for (stride = chip_size / 2; stride >= min_stride; stride /= 2) { + if (stride > len) + continue; + + for (offset = rom_offset; + offset <= rom_offset + len - sizeof(struct fmap); + offset += stride) { + if ((offset % (stride * 2) == 0) && (offset != 0)) + continue; + if (offset == 0 && !check_offset_0) + continue; + check_offset_0 = 0; + + /* Read errors are considered non-fatal since we may + * encounter locked regions and want to continue. */ + if (flashctx->chip->read(flashctx, (uint8_t *)fmap, offset, sig_len)) { + /* + * Print in verbose mode only to avoid excessive + * messages for benign errors. Subsequent error + * prints should be done as usual. + */ + msg_cdbg("Cannot read %d bytes at offset %zu\n", sig_len, offset); + continue; + } + + if (memcmp(fmap, FMAP_SIGNATURE, sig_len) != 0) + continue; + + if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sig_len, + offset + sig_len, sizeof(*fmap) - sig_len)) { + msg_cerr("Cannot read %zu bytes at offset %06zx\n", + sizeof(*fmap) + sig_len, offset + sig_len); + continue; + } + + if (is_valid_fmap(fmap)) { + msg_gdbg("fmap found at offset 0x%06zx\n", offset); + fmap_found = 1; + break; + } else { + msg_gerr("fmap signature found at %zu but header is invalid.\n", offset); + ret = 2; + } + } + + if (fmap_found) + break; + } + + if (!fmap_found) + goto _free_ret; + + fmap_len = fmap_size(fmap); + struct fmap *tmp = fmap; + fmap = realloc(fmap, fmap_len); + if (!fmap) { + msg_gerr("Failed to realloc.\n"); + free(tmp); + goto _free_ret; + } + + if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sizeof(*fmap), + offset + sizeof(*fmap), fmap_len - sizeof(*fmap))) { + msg_cerr("Cannot read %zu bytes at offset %06zx\n", + fmap_len - sizeof(*fmap), offset + sizeof(*fmap)); + /* Treat read failure to be fatal since this + * should be a valid, usable fmap. */ + ret = 2; + goto _free_ret; + } + + *fmap_out = fmap; + ret = 0; +_free_ret: + if (ret) + free(fmap); + finalize_flash_access(flashctx); + return ret; +} + +/** + * @brief Read fmap from ROM + * + * @param[out] fmap_out Double-pointer to location to store fmap contents. + * Caller must free allocated fmap contents. + * @param[in] flashctx Flash context + * @param[in] rom_offset Offset in ROM to begin search + * @param[in] len Length to search relative to rom_offset + * + * @return 0 on success, + * 2 if the fmap couldn't be read, + * 1 on any other error. + */ +int fmap_read_from_rom(struct fmap **fmap_out, + struct flashctx *const flashctx, size_t rom_offset, size_t len) +{ + int ret; + + if (!flashctx || !flashctx->chip) + return 1; + + /* + * Binary search is used at first to see if we can find an fmap quickly + * in a usual location (often at a power-of-2 offset). However, once we + * reach a small enough stride the transaction overhead will reverse the + * speed benefit of using bsearch at which point we need to use brute- + * force instead. + * + * TODO: Since flashrom is often used with high-latency external + * programmers we should not be overly aggressive with bsearch. + */ + ret = fmap_bsearch_rom(fmap_out, flashctx, rom_offset, len, 256); + if (ret) { + msg_gdbg("Binary search failed, trying linear search...\n"); + ret = fmap_lsearch_rom(fmap_out, flashctx, rom_offset, len); + } + + return ret; +} diff --git a/fmap.h b/fmap.h new file mode 100644 index 0000000..be5f8bc --- /dev/null +++ b/fmap.h @@ -0,0 +1,71 @@ +/* + * Copyright 2015, Google Inc. + * Copyright 2018-present, Facebook Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * Alternatively, this software may be distributed under the terms of the + * GNU General Public License ("GPL") version 2 as published by the Free + * Software Foundation. + */ + +#ifndef __FMAP_H__ +#define __FMAP_H__ 1 + +#include <inttypes.h> +#include <stdbool.h> + +#define FMAP_SIGNATURE "__FMAP__" +#define FMAP_VER_MAJOR 1 /* this header's FMAP minor version */ +#define FMAP_VER_MINOR 1 /* this header's FMAP minor version */ +#define FMAP_STRLEN 32 /* maximum length for strings */ + +struct fmap_area { + uint32_t offset; /* offset relative to base */ + uint32_t size; /* size in bytes */ + uint8_t name[FMAP_STRLEN]; /* descriptive name */ + uint16_t flags; /* flags for this area */ +} __attribute__((packed)); + +struct fmap { + uint8_t signature[8]; /* "__FMAP__" */ + uint8_t ver_major; /* major version */ + uint8_t ver_minor; /* minor version */ + uint64_t base; /* address of the firmware binary */ + uint32_t size; /* size of firmware binary in bytes */ + uint8_t name[FMAP_STRLEN]; /* name of this firmware binary */ + uint16_t nareas; /* number of areas described by + fmap_areas[] below */ + struct fmap_area areas[]; +} __attribute__((packed)); + +int fmap_read_from_buffer(struct fmap **fmap_out, const uint8_t *buf, size_t len); +int fmap_read_from_rom(struct fmap **fmap_out, struct flashctx *const flashctx, size_t rom_offset, size_t len); + + +#endif /* __FMAP_H__*/ diff --git a/libflashrom.c b/libflashrom.c index 34e881a..f90a22c 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -25,6 +25,7 @@ #include <stdarg.h> #include "flash.h" +#include "fmap.h" #include "programmer.h" #include "layout.h" #include "hwaccess.h" @@ -382,6 +383,124 @@ #endif } +static int flashrom_layout_parse_fmap(struct flashrom_layout **layout, + struct flashctx *const flashctx, const struct fmap *const fmap) +{ + int i; + struct flashrom_layout *l = get_global_layout(); + + if (!fmap || !l) + return 1; + + if (l->num_entries + fmap->nareas > MAX_ROMLAYOUT) { + msg_gerr("Cannot add fmap entries to layout - Too many entries.\n"); + return 1; + } + + for (i = 0; i < fmap->nareas; i++) { + l->entries[l->num_entries].start = fmap->areas[i].offset; + l->entries[l->num_entries].end = fmap->areas[i].offset + fmap->areas[i].size - 1; + l->entries[l->num_entries].included = false; + memset(l->entries[l->num_entries].name, 0, sizeof(l->entries[i].name)); + memcpy(l->entries[l->num_entries].name, fmap->areas[i].name, + min(FMAP_STRLEN, sizeof(l->entries[i].name))); + msg_gdbg("fmap %08x - %08x named %s\n", + l->entries[l->num_entries].start, + l->entries[l->num_entries].end, + l->entries[l->num_entries].name); + l->num_entries++; + } + + *layout = l; + return 0; +} + +/** + * @brief Read a layout by searching the flash chip for fmap. + * + * @param[out] layout Points to a struct flashrom_layout pointer that + * gets set if the fmap is read and parsed successfully. + * @param[in] flashctx Flash context + * @param[in] offset Offset to begin searching for fmap. + * @param[in] offset Length of address space to search. + * + * @return 0 on success, + * 3 if fmap parsing isn't implemented for the host, + * 2 if the fmap couldn't be read, + * 1 on any other error. + */ +int flashrom_layout_read_fmap_from_rom(struct flashrom_layout **const layout, + struct flashctx *const flashctx, off_t offset, size_t len) +{ +#ifndef __FLASHROM_LITTLE_ENDIAN__ + return 3; +#else + struct fmap *fmap = NULL; + int ret = 0; + + msg_gdbg("Attempting to read fmap from ROM content.\n"); + if (fmap_read_from_rom(&fmap, flashctx, offset, len)) { + msg_gerr("Failed to read fmap from ROM.\n"); + return 1; + } + + msg_gdbg("Adding fmap layout to global layout.\n"); + if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { + msg_gerr("Failed to add fmap regions to layout.\n"); + ret = 1; + } + + free(fmap); + return ret; +#endif +} + +/** + * @brief Read a layout by searching buffer for fmap. + * + * @param[out] layout Points to a struct flashrom_layout pointer that + * gets set if the fmap is read and parsed successfully. + * @param[in] flashctx Flash context + * @param[in] buffer Buffer to search in + * @param[in] size Size of buffer to search + * + * @return 0 on success, + * 3 if fmap parsing isn't implemented for the host, + * 2 if the fmap couldn't be read, + * 1 on any other error. + */ +int flashrom_layout_read_fmap_from_buffer(struct flashrom_layout **const layout, + struct flashctx *const flashctx, const uint8_t *const buf, size_t size) +{ +#ifndef __FLASHROM_LITTLE_ENDIAN__ + return 3; +#else + struct fmap *fmap = NULL; + int ret = 1; + + if (!buf || !size) + goto _ret; + + msg_gdbg("Attempting to read fmap from buffer.\n"); + if (fmap_read_from_buffer(&fmap, buf, size)) { + msg_gerr("Failed to read fmap from buffer.\n"); + goto _ret; + } + + msg_gdbg("Adding fmap layout to global layout.\n"); + if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) { + msg_gerr("Failed to add fmap regions to layout.\n"); + goto _free_ret; + } + + ret = 0; +_free_ret: + free(fmap); +_ret: + return ret; +#endif +} + /** * @brief Free a layout. * diff --git a/libflashrom.h b/libflashrom.h index 04907b2..4fbcd35 100644 --- a/libflashrom.h +++ b/libflashrom.h @@ -61,8 +61,12 @@ struct flashrom_layout; 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); +int flashrom_layout_read_fmap_from_buffer(struct flashrom_layout **layout, + struct flashrom_flashctx *, const uint8_t *buf, size_t len); int flashrom_layout_include_region(struct flashrom_layout *, const char *name); void flashrom_layout_release(struct flashrom_layout *); void flashrom_layout_set(struct flashrom_flashctx *, const struct flashrom_layout *); -#endif /* !__LIBFLASHROM_H__ */ \ No newline at end of file +#endif /* !__LIBFLASHROM_H__ */ -- To view, visit https://review.coreboot.org/23203 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Gerrit-Change-Number: 23203 Gerrit-PatchSet: 27 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2018