Hello Daniel Gröber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41750
to review the following change.
Change subject: spi/macronix: Add support for boot media protection ......................................................................
spi/macronix: Add support for boot media protection
Tested on a Lenovo T500 with MX25L6405D flash.
Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d Signed-off-by: Daniel Gröber dxld@darkboxed.org --- M src/drivers/spi/macronix.c M src/drivers/spi/spi_flash_internal.h A src/drivers/spi/spi_macronix.h A tests/drivers/Makefile.inc A tests/drivers/macronix-spi-test.c 5 files changed, 519 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/41750/1
diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index f3f7e2d..333276d 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -4,8 +4,10 @@ #include <commonlib/helpers.h> #include <spi_flash.h> #include <spi-generic.h> +#include <lib.h>
#include "spi_flash_internal.h" +#include "spi_macronix.h"
/* MX25xx-specific commands */ #define CMD_MX25XX_WREN 0x06 /* Write Enable */ @@ -23,6 +25,17 @@
#define MACRONIX_SR_WIP (1 << 0) /* Write-in-Progress */
+union status_reg { + u8 u; + struct { + u8 wip : 1; + u8 wel : 1; + u8 bp : 4; + u8 _res : 1; + u8 srwd : 1; + }; +}; + static const struct spi_flash_part_id flash_table[] = { { /* MX25L8005 */ @@ -33,16 +46,22 @@ /* MX25L1605D */ .id[0] = 0x2015, .nr_sectors_shift = 9, + .protection_granularity_shift = 16, + .bp_lower_th = 10, }, { /* MX25L3205D */ .id[0] = 0x2016, .nr_sectors_shift = 10, + .protection_granularity_shift = 16, + .bp_lower_th = 9, }, { /* MX25L6405D */ .id[0] = 0x2017, .nr_sectors_shift = 11, + .protection_granularity_shift = 17, + .bp_lower_th = 9, }, { /* MX25L12805D */ @@ -121,6 +140,270 @@ }, };
+/** + * Convert block protection bits, to a region. + */ +void macronix_bpbits_to_region(const struct spi_flash_part_id *params, + const u8 bp, + const size_t flash_size, + struct region *out) +{ + if (bp == 0xf) { + out->offset = 0; + out->size = flash_size; + } else if (bp == 0) { + out->offset = 0; + out->size = 0; + } else { + const u16 granularity_shift = + params->protection_granularity_shift; + + if (bp >= params->bp_lower_th) { + /* Lower half is protected */ + const size_t unprotected_size = + MIN(1<<granularity_shift<<(14 - bp), + flash_size); + + out->offset = 0; + out->size = flash_size - unprotected_size; + } else { + /* Upper half is protected */ + const size_t protected_size = + MIN(1<<(granularity_shift + bp - 1), + flash_size); + + out->offset = flash_size - protected_size; + out->size = protected_size; + } + } +} + +/** + * Convert a region to block protection bits. + * + * rv >= 0: converted bp value + * rv < 0: error + */ +int macronix_region_to_bpbits(const struct spi_flash_part_id *params, + const struct region *region, + const size_t flash_size) +{ + if (region->offset == 0 && region->size >= flash_size) { + return 0xf; + } else if (region->offset == 0 && region->size == 0) { + return 0; + } else if (region->offset >= flash_size || region->size >= flash_size) { + return -1; + } else { + const size_t gran_shift = params->protection_granularity_shift; + const u16 granularity = 1<<gran_shift; + + if (region->offset == 0) { /* Lower half should be protected */ + const size_t unprotected_size = + flash_size - region->size; + + const int is_valid = + IS_POWER_OF_2(unprotected_size) && + unprotected_size >= granularity; + if (!is_valid) + return -1; + + return 14 - log2(unprotected_size>>gran_shift); + } else { + const size_t protected_size = region->size; + + const int is_valid = + IS_POWER_OF_2(protected_size) && + protected_size >= granularity; + if (!is_valid) + return -1; + + return log2(protected_size>>gran_shift) + 1; + } + } +} + +static int macronix_flash_write_status(const struct spi_flash *flash, + const union status_reg val, + const union status_reg mask) +{ + int ret; + union status_reg status; + union status_reg new_status; + + if (val.u & ~mask.u) { + printk(BIOS_WARNING, "MACRONIX: given status reg value %02x" + " doesn't conform to mask %02x!", val.u, mask.u); + } + + ret = spi_flash_cmd_status(flash, &status.u); + if (ret) + return ret; + + new_status.u = (status.u & ~mask.u) | (val.u & mask.u); + + /* already configured right */ + if (new_status.u == status.u) + return 0; + + if (status.srwd) { + printk(BIOS_WARNING, "MACRONIX: status reg write is disabled!" + " Trying anyway."); + } + + ret = spi_flash_cmd(&flash->spi, flash->wren_cmd, NULL, 0); + if (ret) + return ret; + + ret = spi_flash_cmd(&flash->spi, CMD_MX25XX_WRSR, + &status, sizeof(status)); + if (ret) + return ret; + + ret = spi_flash_cmd_status(flash, &status.u); + if (ret) + return ret; + + if ((status.u & mask.u) != (val.u & mask.u)) { + printk(BIOS_WARNING, "MACRONIX: status reg write didn't take," + " is WP pin at GND?\n"); + return -1; + } + + return 0; +} + + +/* + * Read block protect bits from Status Reg. + * Converts block protection bits to a region. + * + * Returns: + * -1 on error + * 1 if region is covered by write protection + * 0 if a part of region isn't covered by write protection + */ +static int macronix_get_write_protection(const struct spi_flash *flash, + const struct region *region) +{ + const struct spi_flash_part_id *params; + struct region wp_region; + int ret; + + params = flash->part; + + if (!params) + return -1; + + if (!params->protection_granularity_shift) + return -1; + + union status_reg reg = { .u = 0 }; + + ret = spi_flash_cmd(&flash->spi, flash->status_cmd, ®.u, + sizeof(reg.u)); + if (ret) + return ret; + + macronix_bpbits_to_region(params, reg.bp, flash->size, &wp_region); + + if (!region_sz(&wp_region)) { + printk(BIOS_DEBUG, "MACRONIX: flash isn't protected\n"); + return 0; + } + + printk(BIOS_DEBUG, "MACRONIX: flash protected range 0x%08zx-0x%08zx\n", + region_offset(&wp_region), region_end(&wp_region)); + + return region_is_subregion(&wp_region, region); +} + +/* + * Protect a region starting from start of flash or end of flash. + * The caller must provide a supported protected region size. + * Writes block protect bits to Status Reg. + * Optionally lock the status register if lock_sreg is set with the provided + * mode. + * + * @param flash: The flash to operate on + * @param region: The region to write protect + * @param mode: Optional status register lock-down mode + * + * @return 0 on success + */ +static int +macronix_set_write_protection(const struct spi_flash *flash, + const struct region *region, + const enum spi_flash_status_reg_lockdown mode) +{ + const struct spi_flash_part_id *params; + union status_reg val, mask; + int bp; + int ret; + + printk(BIOS_DEBUG, "MACRONIX: want to set protection for" + " 0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); + + params = flash->part; + + if (!params) + return -1; + + if (!params->protection_granularity_shift) { + printk(BIOS_ERR, "MACRONIX: ERROR: spi_flash part doesn't support block protection\n"); + return -1; // TODO: Warning + } + + bp = macronix_region_to_bpbits(params, region, flash->size); + if (bp < 0) { + printk(BIOS_ERR, "MACRONIX: ERROR: unsupported region size\n"); + return -1; + } + + /* Write block protection bits */ + val = (union status_reg) { .bp = bp, }; + mask = (union status_reg) { .bp = ~0, .srwd = ~0, }; + + if (mode != SPI_WRITE_PROTECTION_PRESERVE) { + switch (mode) { + case SPI_WRITE_PROTECTION_NONE: + val.srwd = 0; + break; + case SPI_WRITE_PROTECTION_PIN: + val.srwd = 1; + break; + default: + printk(BIOS_ERR, "MACRONIX: ERROR: requested write" + " protection duration (%d) is not supported by" + " this chip\n", mode); + return -1; + } + } + + ret = macronix_flash_write_status(flash, val, mask); + if (ret) { + printk(BIOS_DEBUG, "MACRONIX: macronix_flash_write_status" + " failed: ret=%d\n", ret); + return ret; + } + + printk(BIOS_DEBUG, "MACRONIX: write-protection range is" + " 0x%08zx-0x%08zx\n", region_offset(region), region_end(region)); + + if (val.srwd) { + printk(BIOS_DEBUG, "MACRONIX: conditional WP# pin" + " hadrware protection is enabled.\n"); + } + + return ret; +} + + +static const struct spi_flash_protection_ops spi_flash_protection_ops = { + .get_write = macronix_get_write_protection, + .set_write = macronix_set_write_protection, +}; + const struct spi_flash_vendor_info spi_flash_macronix_vi = { .id = VENDOR_ID_MACRONIX, .page_size_shift = 8, @@ -129,4 +412,5 @@ .ids = flash_table, .nr_part_ids = ARRAY_SIZE(flash_table), .desc = &spi_flash_pp_0x20_sector_desc, + .prot_ops = &spi_flash_protection_ops, }; diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index b4d39b3..f23e440 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -71,9 +71,11 @@ uint16_t nr_sectors_shift: 4; uint16_t fast_read_dual_output_support : 1; uint16_t _reserved_for_flags: 3; - /* Block protection. Currently used by Winbond. */ + /* Block protection. */ uint16_t protection_granularity_shift : 5; - uint16_t bp_bits : 3; + uint16_t bp_bits : 3; /* Used by Winbond */ + /* bp values starting here protect the lower half */ + uint16_t bp_lower_th : 5; /* Used by Macronix */ };
struct spi_flash_ops_descriptor { diff --git a/src/drivers/spi/spi_macronix.h b/src/drivers/spi/spi_macronix.h new file mode 100644 index 0000000..da32f81 --- /dev/null +++ b/src/drivers/spi/spi_macronix.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef SPI_MACRONIX_H +#define SPI_MACRONIX_H + +void macronix_bpbits_to_region(const struct spi_flash_part_id *params, + const u8 bp, + const size_t flash_size, + struct region *out); + +int macronix_region_to_bpbits(const struct spi_flash_part_id *params, + const struct region *region, + const size_t flash_size); + +#endif /* SPI_MACRONIX_H */ diff --git a/tests/drivers/Makefile.inc b/tests/drivers/Makefile.inc new file mode 100644 index 0000000..691ddc5 --- /dev/null +++ b/tests/drivers/Makefile.inc @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0-only + +tests-y += macronix-spi-test + +macronix-spi-test-srcs += tests/drivers/macronix-spi-test.c +macronix-spi-test-srcs += src/drivers/spi/macronix.c +macronix-spi-test-srcs += src/commonlib/region.c +macronix-spi-test-srcs += src/commonlib/mem_pool.c +macronix-spi-test-srcs-mocks += do_printk region_is_subregion spi_flash_cmd diff --git a/tests/drivers/macronix-spi-test.c b/tests/drivers/macronix-spi-test.c new file mode 100644 index 0000000..232f069 --- /dev/null +++ b/tests/drivers/macronix-spi-test.c @@ -0,0 +1,207 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> + +#include <string.h> + +#include <stdio.h> /* debug */ + +#include <commonlib/helpers.h> +#include <spi_flash.h> +#include <spi-generic.h> +#include "../drivers/spi/spi_flash_internal.h" +#include "../drivers/spi/spi_macronix.h" + +struct bp_table { + uint16_t id; + size_t size; + struct block_region { + /* these are in units of 64k-byte blocks */ + size_t offset; + size_t size; + } wp_blocks[1<<4]; +} bp_table[] = { + { + /* MX25L1605D */ + .id = 0x2015, + .size = 0x200000, + .wp_blocks = { + /* off size */ + [0] = { 0, 0 }, + [1] = { 31, 1 }, + [2] = { 30, 2 }, + [3] = { 28, 4 }, + [4] = { 24, 8 }, + [5] = { 16, 16 }, + [6] = { 0, 32 }, + [7] = { 0, 32 }, + [8] = { 0, 32 }, + [9] = { 0, 32 }, + + [10] = { 0, 16 }, + [11] = { 0, 24 }, + [12] = { 0, 28 }, + [13] = { 0, 30 }, + [14] = { 0, 31 }, + [15] = { 0, 32 }, + }, + }, + { + /* MX25L3205D */ + .id = 0x2016, + .size = 0x400000, + .wp_blocks = { + /* off size */ + [0] = { 0, 0 }, + [1] = { 63, 1 }, + [2] = { 62, 2 }, + [3] = { 60, 4 }, + [4] = { 56, 8 }, + [5] = { 48, 16 }, + [6] = { 32, 32 }, + [7] = { 0, 64 }, + [8] = { 0, 64 }, + + [9] = { 0, 32 }, + [10] = { 0, 48 }, + [11] = { 0, 56 }, + [12] = { 0, 60 }, + [13] = { 0, 62 }, + [14] = { 0, 63 }, + [15] = { 0, 64 }, + }, + }, + { + /* MX25L6405D */ + .id = 0x2017, + .size = 0x800000, + .wp_blocks = { + /* off size */ + [0] = { 0, 0 }, + [1] = { 126, 2 }, + [2] = { 124, 4 }, + [3] = { 120, 8 }, + [4] = { 112, 16 }, + [5] = { 96, 32 }, + [6] = { 64, 64 }, + [7] = { 0, 128 }, + [8] = { 0, 128 }, + + [9] = { 0, 64 }, + [10] = { 0, 96 }, + [11] = { 0, 112 }, + [12] = { 0, 120 }, + [13] = { 0, 124 }, + [14] = { 0, 126 }, + [15] = { 0, 128 }, + }, + }, +}; + + +static void region_bpbits_roundtrip_test(void **state) +{ + int n_parts_tested; + + const struct spi_flash_vendor_info *vi = &spi_flash_macronix_vi; + + for (size_t i = 0; i < vi->nr_part_ids; i++) { + const struct spi_flash_part_id *id = &vi->ids[i]; + + if (!id->protection_granularity_shift) + continue; + + const struct bp_table *table = NULL; + for (uint16_t j = 0; j < ARRAY_SIZE(bp_table); j++) { + if (bp_table[j].id == id->id[0]) { + table = &bp_table[j]; + break; + } + } + + n_parts_tested++; + + assert_non_null(table); + + for (u8 bp = 0; bp <= 0xf; bp++) { + struct region expected, out = {-1, -1}; + struct region roundtripped = {-1, -1}; + + const struct block_region *wp_blocks = + &table->wp_blocks[bp]; + const size_t block_size = 1<<16; + + expected.offset = wp_blocks->offset * block_size; + expected.size = wp_blocks->size * block_size; + + macronix_bpbits_to_region(id, bp, table->size, &out); + + int bp1 = macronix_region_to_bpbits(id, &out, + table->size); + + /* we have to go through bpbits_to_region again as + * the bp<>range mapping isn't unique. For example + * bp=0b1111 always means everything is protected + * but bp values just after the largest upper-half + * protection also mean "all blocks protected" */ + macronix_bpbits_to_region(id, bp1, table->size, + &roundtripped); + +#if 0 + /* debugging */ + print_message("\nfor part 0x%04x bp=%d:\n", + id->id[0], (int)bp); + print_message("should: 0x%08zx-0x%08zx\n", + region_offset(&expected), + region_end(&expected)); + print_message("is: 0x%08zx-0x%08zx\n", + region_offset(&out), + region_end(&out)); + print_message("rt'ed: 0x%08zx-0x%08zx\n", + region_offset(&roundtripped), + region_end(&roundtripped)); + print_message("bp1: %d\n", bp1); +#endif + + assert_true(expected.size == out.size); + assert_true(expected.offset == out.offset); + + assert_true(expected.size == roundtripped.size); + assert_true(expected.offset == roundtripped.offset); + } + } + + /* Just making sure we actually tested something */ + assert_true(n_parts_tested >= 3); +} + + /* Mocks */ +int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, + size_t len) +{ + return -1; +} + +int do_printk(int msg_level, const char *fmt, ...) +{ + return -1; +} + +int spi_flash_cmd_status(const struct spi_flash *flash, u8 *reg) +{ + return -1; +} + +const struct spi_flash_ops_descriptor spi_flash_pp_0x20_sector_desc; + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(region_bpbits_roundtrip_test), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}