New subject: [L] Change in coreboot[master]: spi/macronix: Add support for boot media protection

26 May 2020

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, &reg.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);
+}
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/41750
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Icb92ba9f92746f8c7886220b31d86c130f906c6d
Gerrit-Change-Number: 41750
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Gröber (dxld)
Gerrit-Reviewer: Daniel Gröber dxld@darkboxed.org
Gerrit-Reviewer: Martin Roth martinroth@google.com
Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com
Gerrit-MessageType: newchange



    