* [PATCH 0/2] Add Block Protect (BP) and Top Bottom (TB) bits for write protect
@ 2022-06-27 18:52 Iris Chen
2022-06-27 18:52 ` [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom " Iris Chen
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Iris Chen @ 2022-06-27 18:52 UTC (permalink / raw)
Cc: irischenlj, pdel, qemu-devel, qemu-arm, clg, patrick, alistair,
kwolf, hreitz, peter.maydell, andrew, joel, thuth, lvivier,
pbonzini, qemu-block, dz4list
Hey everyone,
Adding the 3 block protection bits and top bottom bits which configure which parts
of the flash are writable and read-only.
These patches are based on previous patches I just submitted:
hw: m25p80: fixing individual test failure when tests are running in isolation
hw: m25p80: add WP# pin and SRWD bit for write protection
hw: m25p80: add tests for write protect (WP# and SRWD bit)
The tests that were added iterates through every different memory protection
case so it takes a little longer to run. Let me know what you think about that
and if that's okay or not.
Thanks,
Iris
Iris Chen (2):
hw: m25p80: Add Block Protect and Top Bottom bits for write protect
hw: m25p80: add tests for BP and TB bit write protect
hw/block/m25p80.c | 74 +++++++++++++++++++----
tests/qtest/aspeed_smc-test.c | 111 ++++++++++++++++++++++++++++++++++
2 files changed, 173 insertions(+), 12 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-06-27 18:52 [PATCH 0/2] Add Block Protect (BP) and Top Bottom (TB) bits for write protect Iris Chen
@ 2022-06-27 18:52 ` Iris Chen
2022-07-01 7:11 ` Cédric Le Goater
` (2 more replies)
2022-06-27 18:52 ` [PATCH 2/2] hw: m25p80: add tests for BP and TB bit " Iris Chen
` (2 subsequent siblings)
3 siblings, 3 replies; 13+ messages in thread
From: Iris Chen @ 2022-06-27 18:52 UTC (permalink / raw)
Cc: irischenlj, pdel, qemu-devel, qemu-arm, clg, patrick, alistair,
kwolf, hreitz, peter.maydell, andrew, joel, thuth, lvivier,
pbonzini, qemu-block, dz4list
Signed-off-by: Iris Chen <irischenlj@fb.com>
---
hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
1 file changed, 62 insertions(+), 12 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 50b523e5b1..0156a70f5e 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -38,21 +38,19 @@
#include "trace.h"
#include "qom/object.h"
-/* Fields for FlashPartInfo->flags */
-
-/* erase capabilities */
-#define ER_4K 1
-#define ER_32K 2
-/* set to allow the page program command to write 0s back to 1. Useful for
- * modelling EEPROM with SPI flash command set
- */
-#define EEPROM 0x100
-
/* 16 MiB max in 3 byte address mode */
#define MAX_3BYTES_SIZE 0x1000000
-
#define SPI_NOR_MAX_ID_LEN 6
+/* Fields for FlashPartInfo->flags */
+enum spi_nor_option_flags {
+ ER_4K = BIT(0),
+ ER_32K = BIT(1),
+ EEPROM = BIT(2),
+ SNOR_F_HAS_SR_TB = BIT(3),
+ SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
+};
+
typedef struct FlashPartInfo {
const char *part_name;
/*
@@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
{ INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
- { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
+ { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
+ ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
{ INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
{ INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
@@ -480,6 +479,11 @@ struct Flash {
bool reset_enable;
bool quad_enable;
bool aai_enable;
+ bool block_protect0;
+ bool block_protect1;
+ bool block_protect2;
+ bool block_protect3;
+ bool top_bottom_bit;
bool status_register_write_disabled;
uint8_t ear;
@@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
return;
}
+ uint32_t block_protect_value = (s->block_protect3 << 3) |
+ (s->block_protect2 << 2) |
+ (s->block_protect1 << 1) |
+ (s->block_protect0 << 0);
+
+ uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
+ uint32_t sector = addr / s->pi->sector_size;
+
+ /* top_bottom_bit == 0 means TOP */
+ if (!s->top_bottom_bit) {
+ if (block_protect_value > 0 &&
+ s->pi->n_sectors <= sector + num_protected_sectors) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: write with write protect!\n");
+ return;
+ }
+ } else {
+ if (block_protect_value > 0 && sector < num_protected_sectors) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: write with write protect!\n");
+ return;
+ }
+ }
if ((prev ^ data) & data) {
trace_m25p80_programming_zero_to_one(s, addr, prev, data);
@@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
break;
case WRSR:
s->status_register_write_disabled = extract32(s->data[0], 7, 1);
+ s->block_protect0 = extract32(s->data[0], 2, 1);
+ s->block_protect1 = extract32(s->data[0], 3, 1);
+ s->block_protect2 = extract32(s->data[0], 4, 1);
+ if (s->pi->flags & SNOR_F_HAS_SR_TB) {
+ s->top_bottom_bit = extract32(s->data[0], 5, 1);
+ }
+ if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
+ s->block_protect3 = extract32(s->data[0], 6, 1);
+ }
switch (get_man(s)) {
case MAN_SPANSION:
@@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
case RDSR:
s->data[0] = (!!s->write_enable) << 1;
s->data[0] |= (!!s->status_register_write_disabled) << 7;
+ s->data[0] |= (!!s->block_protect0) << 2;
+ s->data[0] |= (!!s->block_protect1) << 3;
+ s->data[0] |= (!!s->block_protect2) << 4;
+ if (s->pi->flags & SNOR_F_HAS_SR_TB) {
+ s->data[0] |= (!!s->top_bottom_bit) << 5;
+ }
+ if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
+ s->data[0] |= (!!s->block_protect3) << 6;
+ }
if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
s->data[0] |= (!!s->quad_enable) << 6;
@@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
s->wp_level = true;
s->status_register_write_disabled = false;
+ s->block_protect0 = false;
+ s->block_protect1 = false;
+ s->block_protect2 = false;
+ s->block_protect3 = false;
+ s->top_bottom_bit = false;
reset_memory(s);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] hw: m25p80: add tests for BP and TB bit write protect
2022-06-27 18:52 [PATCH 0/2] Add Block Protect (BP) and Top Bottom (TB) bits for write protect Iris Chen
2022-06-27 18:52 ` [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom " Iris Chen
@ 2022-06-27 18:52 ` Iris Chen
2022-07-01 7:24 ` Cédric Le Goater
2022-07-07 2:16 ` [PATCH v2] hw: m25p80: Add Block Protect and Top Bottom bits for " Iris Chen
2022-07-08 16:45 ` [PATCH v3] " Iris Chen
3 siblings, 1 reply; 13+ messages in thread
From: Iris Chen @ 2022-06-27 18:52 UTC (permalink / raw)
Cc: irischenlj, pdel, qemu-devel, qemu-arm, clg, patrick, alistair,
kwolf, hreitz, peter.maydell, andrew, joel, thuth, lvivier,
pbonzini, qemu-block, dz4list
Signed-off-by: Iris Chen <irischenlj@fb.com>
---
tests/qtest/aspeed_smc-test.c | 111 ++++++++++++++++++++++++++++++++++
1 file changed, 111 insertions(+)
diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index 1258687eac..05ce941566 100644
--- a/tests/qtest/aspeed_smc-test.c
+++ b/tests/qtest/aspeed_smc-test.c
@@ -192,6 +192,24 @@ static void read_page_mem(uint32_t addr, uint32_t *page)
}
}
+static void write_page_mem(uint32_t addr, uint32_t write_value)
+{
+ spi_ctrl_setmode(CTRL_WRITEMODE, PP);
+
+ for (int i = 0; i < FLASH_PAGE_SIZE / 4; i++) {
+ writel(ASPEED_FLASH_BASE + addr + i * 4, write_value);
+ }
+}
+
+static void assert_page_mem(uint32_t addr, uint32_t expected_value)
+{
+ uint32_t page[FLASH_PAGE_SIZE / 4];
+ read_page_mem(addr, page);
+ for (int i = 0; i < FLASH_PAGE_SIZE / 4; i++) {
+ g_assert_cmphex(page[i], ==, expected_value);
+ }
+}
+
static void test_erase_sector(void)
{
uint32_t some_page_addr = 0x600 * FLASH_PAGE_SIZE;
@@ -501,6 +519,95 @@ static void test_status_reg_write_protection(void)
flash_reset();
}
+static void test_write_block_protect(void)
+{
+ uint32_t sector_size = 65536;
+ uint32_t n_sectors = 512;
+
+ spi_ce_ctrl(1 << CRTL_EXTENDED0);
+ spi_conf(CONF_ENABLE_W0);
+
+ uint32_t bp_bits = 0b0;
+
+ for (int i = 0; i < 16; i++) {
+ bp_bits = ((i & 0b1000) << 3) | ((i & 0b0111) << 2);
+
+ spi_ctrl_start_user();
+ writeb(ASPEED_FLASH_BASE, WREN);
+ writeb(ASPEED_FLASH_BASE, BULK_ERASE);
+ writeb(ASPEED_FLASH_BASE, WREN);
+ writeb(ASPEED_FLASH_BASE, WRSR);
+ writeb(ASPEED_FLASH_BASE, bp_bits);
+ writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
+ writeb(ASPEED_FLASH_BASE, WREN);
+ spi_ctrl_stop_user();
+
+ uint32_t num_protected_sectors = i ? MIN(1 << (i - 1), n_sectors) : 0;
+ uint32_t protection_start = n_sectors - num_protected_sectors;
+ uint32_t protection_end = n_sectors;
+
+ for (int sector = 0; sector < n_sectors; sector++) {
+ uint32_t addr = sector * sector_size;
+
+ assert_page_mem(addr, 0xffffffff);
+ write_page_mem(addr, make_be32(0xabcdef12));
+
+ uint32_t expected_value = protection_start <= sector
+ && sector < protection_end
+ ? 0xffffffff : 0xabcdef12;
+
+ assert_page_mem(addr, expected_value);
+ }
+ }
+
+ flash_reset();
+}
+
+static void test_write_block_protect_bottom_bit(void)
+{
+ uint32_t sector_size = 65536;
+ uint32_t n_sectors = 512;
+
+ spi_ce_ctrl(1 << CRTL_EXTENDED0);
+ spi_conf(CONF_ENABLE_W0);
+
+ /* top bottom bit is enabled */
+ uint32_t bp_bits = 0b00100 << 3;
+
+ for (int i = 0; i < 16; i++) {
+ bp_bits = (((i & 0b1000) | 0b0100) << 3) | ((i & 0b0111) << 2);
+
+ spi_ctrl_start_user();
+ writeb(ASPEED_FLASH_BASE, WREN);
+ writeb(ASPEED_FLASH_BASE, BULK_ERASE);
+ writeb(ASPEED_FLASH_BASE, WREN);
+ writeb(ASPEED_FLASH_BASE, WRSR);
+ writeb(ASPEED_FLASH_BASE, bp_bits);
+ writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
+ writeb(ASPEED_FLASH_BASE, WREN);
+ spi_ctrl_stop_user();
+
+ uint32_t num_protected_sectors = i ? MIN(1 << (i - 1), n_sectors) : 0;
+ uint32_t protection_start = 0;
+ uint32_t protection_end = num_protected_sectors;
+
+ for (int sector = 0; sector < n_sectors; sector++) {
+ uint32_t addr = sector * sector_size;
+
+ assert_page_mem(addr, 0xffffffff);
+ write_page_mem(addr, make_be32(0xabcdef12));
+
+ uint32_t expected_value = protection_start <= sector
+ && sector < protection_end
+ ? 0xffffffff : 0xabcdef12;
+
+ assert_page_mem(addr, expected_value);
+ }
+ }
+
+ flash_reset();
+}
+
static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
int main(int argc, char **argv)
@@ -529,6 +636,10 @@ int main(int argc, char **argv)
qtest_add_func("/ast2400/smc/read_status_reg", test_read_status_reg);
qtest_add_func("/ast2400/smc/status_reg_write_protection",
test_status_reg_write_protection);
+ qtest_add_func("/ast2400/smc/write_block_protect",
+ test_write_block_protect);
+ qtest_add_func("/ast2400/smc/write_block_protect_bottom_bit",
+ test_write_block_protect_bottom_bit);
flash_reset();
ret = g_test_run();
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-06-27 18:52 ` [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom " Iris Chen
@ 2022-07-01 7:11 ` Cédric Le Goater
2022-07-01 11:40 ` Francisco Iglesias
2022-07-04 20:50 ` Cédric Le Goater
2 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-07-01 7:11 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, patrick, alistair, kwolf, hreitz,
peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
Even for a simple addition a quick intro is always nice to have.
On 6/27/22 20:52, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
Too bad that we are modeling 5 bits with 5 bools but given the layout
it makes the code easier to read. LGTM.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 62 insertions(+), 12 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 50b523e5b1..0156a70f5e 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -38,21 +38,19 @@
> #include "trace.h"
> #include "qom/object.h"
>
> -/* Fields for FlashPartInfo->flags */
> -
> -/* erase capabilities */
> -#define ER_4K 1
> -#define ER_32K 2
> -/* set to allow the page program command to write 0s back to 1. Useful for
> - * modelling EEPROM with SPI flash command set
> - */
> -#define EEPROM 0x100
> -
> /* 16 MiB max in 3 byte address mode */
> #define MAX_3BYTES_SIZE 0x1000000
> -
> #define SPI_NOR_MAX_ID_LEN 6
>
> +/* Fields for FlashPartInfo->flags */
> +enum spi_nor_option_flags {
> + ER_4K = BIT(0),
> + ER_32K = BIT(1),
> + EEPROM = BIT(2),
> + SNOR_F_HAS_SR_TB = BIT(3),
> + SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
> +};
> +
> typedef struct FlashPartInfo {
> const char *part_name;
> /*
> @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
> { INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
> - { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
> + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
> + ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
> { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
> { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
> @@ -480,6 +479,11 @@ struct Flash {
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> + bool block_protect0;
> + bool block_protect1;
> + bool block_protect2;
> + bool block_protect3;
> + bool top_bottom_bit;
> bool status_register_write_disabled;
> uint8_t ear;
>
> @@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
> return;
> }
> + uint32_t block_protect_value = (s->block_protect3 << 3) |
> + (s->block_protect2 << 2) |
> + (s->block_protect1 << 1) |
> + (s->block_protect0 << 0);
> +
> + uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
> + uint32_t sector = addr / s->pi->sector_size;
> +
> + /* top_bottom_bit == 0 means TOP */
> + if (!s->top_bottom_bit) {
> + if (block_protect_value > 0 &&
> + s->pi->n_sectors <= sector + num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + } else {
> + if (block_protect_value > 0 && sector < num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + }
>
> if ((prev ^ data) & data) {
> trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> @@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
> break;
> case WRSR:
> s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> + s->block_protect0 = extract32(s->data[0], 2, 1);
> + s->block_protect1 = extract32(s->data[0], 3, 1);
> + s->block_protect2 = extract32(s->data[0], 4, 1);
> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> + s->top_bottom_bit = extract32(s->data[0], 5, 1);
> + }
> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> + s->block_protect3 = extract32(s->data[0], 6, 1);
> + }
>
> switch (get_man(s)) {
> case MAN_SPANSION:
> @@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> case RDSR:
> s->data[0] = (!!s->write_enable) << 1;
> s->data[0] |= (!!s->status_register_write_disabled) << 7;
> + s->data[0] |= (!!s->block_protect0) << 2;
> + s->data[0] |= (!!s->block_protect1) << 3;
> + s->data[0] |= (!!s->block_protect2) << 4;
> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> + s->data[0] |= (!!s->top_bottom_bit) << 5;
> + }
> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> + s->data[0] |= (!!s->block_protect3) << 6;
> + }
>
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> @@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
>
> s->wp_level = true;
> s->status_register_write_disabled = false;
> + s->block_protect0 = false;
> + s->block_protect1 = false;
> + s->block_protect2 = false;
> + s->block_protect3 = false;
> + s->top_bottom_bit = false;
>
> reset_memory(s);
> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hw: m25p80: add tests for BP and TB bit write protect
2022-06-27 18:52 ` [PATCH 2/2] hw: m25p80: add tests for BP and TB bit " Iris Chen
@ 2022-07-01 7:24 ` Cédric Le Goater
0 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-07-01 7:24 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, patrick, alistair, kwolf, hreitz,
peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
On 6/27/22 20:52, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
Palmettos are getting old (2014). We might want to change the machine
model to an ast2600 one.
Anyhow, these are very good tests for both m25p80 and aspeed-smc models.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> tests/qtest/aspeed_smc-test.c | 111 ++++++++++++++++++++++++++++++++++
> 1 file changed, 111 insertions(+)
>
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index 1258687eac..05ce941566 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -192,6 +192,24 @@ static void read_page_mem(uint32_t addr, uint32_t *page)
> }
> }
>
> +static void write_page_mem(uint32_t addr, uint32_t write_value)
> +{
> + spi_ctrl_setmode(CTRL_WRITEMODE, PP);
> +
> + for (int i = 0; i < FLASH_PAGE_SIZE / 4; i++) {
> + writel(ASPEED_FLASH_BASE + addr + i * 4, write_value);
> + }
> +}
> +
> +static void assert_page_mem(uint32_t addr, uint32_t expected_value)
> +{
> + uint32_t page[FLASH_PAGE_SIZE / 4];
> + read_page_mem(addr, page);
> + for (int i = 0; i < FLASH_PAGE_SIZE / 4; i++) {
> + g_assert_cmphex(page[i], ==, expected_value);
> + }
> +}
> +
> static void test_erase_sector(void)
> {
> uint32_t some_page_addr = 0x600 * FLASH_PAGE_SIZE;
> @@ -501,6 +519,95 @@ static void test_status_reg_write_protection(void)
> flash_reset();
> }
>
> +static void test_write_block_protect(void)
> +{
> + uint32_t sector_size = 65536;
> + uint32_t n_sectors = 512;
> +
> + spi_ce_ctrl(1 << CRTL_EXTENDED0);
> + spi_conf(CONF_ENABLE_W0);
> +
> + uint32_t bp_bits = 0b0;
> +
> + for (int i = 0; i < 16; i++) {
> + bp_bits = ((i & 0b1000) << 3) | ((i & 0b0111) << 2);
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, WREN);
> + writeb(ASPEED_FLASH_BASE, BULK_ERASE);
> + writeb(ASPEED_FLASH_BASE, WREN);
> + writeb(ASPEED_FLASH_BASE, WRSR);
> + writeb(ASPEED_FLASH_BASE, bp_bits);
> + writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
> + writeb(ASPEED_FLASH_BASE, WREN);
> + spi_ctrl_stop_user();
> +
> + uint32_t num_protected_sectors = i ? MIN(1 << (i - 1), n_sectors) : 0;
> + uint32_t protection_start = n_sectors - num_protected_sectors;
> + uint32_t protection_end = n_sectors;
> +
> + for (int sector = 0; sector < n_sectors; sector++) {
> + uint32_t addr = sector * sector_size;
> +
> + assert_page_mem(addr, 0xffffffff);
> + write_page_mem(addr, make_be32(0xabcdef12));
> +
> + uint32_t expected_value = protection_start <= sector
> + && sector < protection_end
> + ? 0xffffffff : 0xabcdef12;
> +
> + assert_page_mem(addr, expected_value);
> + }
> + }
> +
> + flash_reset();
> +}
> +
> +static void test_write_block_protect_bottom_bit(void)
> +{
> + uint32_t sector_size = 65536;
> + uint32_t n_sectors = 512;
> +
> + spi_ce_ctrl(1 << CRTL_EXTENDED0);
> + spi_conf(CONF_ENABLE_W0);
> +
> + /* top bottom bit is enabled */
> + uint32_t bp_bits = 0b00100 << 3;
> +
> + for (int i = 0; i < 16; i++) {
> + bp_bits = (((i & 0b1000) | 0b0100) << 3) | ((i & 0b0111) << 2);
> +
> + spi_ctrl_start_user();
> + writeb(ASPEED_FLASH_BASE, WREN);
> + writeb(ASPEED_FLASH_BASE, BULK_ERASE);
> + writeb(ASPEED_FLASH_BASE, WREN);
> + writeb(ASPEED_FLASH_BASE, WRSR);
> + writeb(ASPEED_FLASH_BASE, bp_bits);
> + writeb(ASPEED_FLASH_BASE, EN_4BYTE_ADDR);
> + writeb(ASPEED_FLASH_BASE, WREN);
> + spi_ctrl_stop_user();
> +
> + uint32_t num_protected_sectors = i ? MIN(1 << (i - 1), n_sectors) : 0;
> + uint32_t protection_start = 0;
> + uint32_t protection_end = num_protected_sectors;
> +
> + for (int sector = 0; sector < n_sectors; sector++) {
> + uint32_t addr = sector * sector_size;
> +
> + assert_page_mem(addr, 0xffffffff);
> + write_page_mem(addr, make_be32(0xabcdef12));
> +
> + uint32_t expected_value = protection_start <= sector
> + && sector < protection_end
> + ? 0xffffffff : 0xabcdef12;
> +
> + assert_page_mem(addr, expected_value);
> + }
> + }
> +
> + flash_reset();
> +}
> +
> static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>
> int main(int argc, char **argv)
> @@ -529,6 +636,10 @@ int main(int argc, char **argv)
> qtest_add_func("/ast2400/smc/read_status_reg", test_read_status_reg);
> qtest_add_func("/ast2400/smc/status_reg_write_protection",
> test_status_reg_write_protection);
> + qtest_add_func("/ast2400/smc/write_block_protect",
> + test_write_block_protect);
> + qtest_add_func("/ast2400/smc/write_block_protect_bottom_bit",
> + test_write_block_protect_bottom_bit);
>
> flash_reset();
> ret = g_test_run();
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-06-27 18:52 ` [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom " Iris Chen
2022-07-01 7:11 ` Cédric Le Goater
@ 2022-07-01 11:40 ` Francisco Iglesias
2022-07-01 12:23 ` Cédric Le Goater
2022-07-04 20:50 ` Cédric Le Goater
2 siblings, 1 reply; 13+ messages in thread
From: Francisco Iglesias @ 2022-07-01 11:40 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, clg, patrick, alistair, kwolf,
hreitz, peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
Hi Iris,
Looks good, a couple of minor comments below!
On [2022 Jun 27] Mon 11:52:33, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
> hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 62 insertions(+), 12 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 50b523e5b1..0156a70f5e 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -38,21 +38,19 @@
> #include "trace.h"
> #include "qom/object.h"
>
> -/* Fields for FlashPartInfo->flags */
> -
> -/* erase capabilities */
> -#define ER_4K 1
> -#define ER_32K 2
> -/* set to allow the page program command to write 0s back to 1. Useful for
> - * modelling EEPROM with SPI flash command set
> - */
> -#define EEPROM 0x100
> -
> /* 16 MiB max in 3 byte address mode */
> #define MAX_3BYTES_SIZE 0x1000000
> -
> #define SPI_NOR_MAX_ID_LEN 6
>
> +/* Fields for FlashPartInfo->flags */
> +enum spi_nor_option_flags {
(A suggestion is to s/nor/flash/ above (and s/SNOR_F_// below) since there
looks to be nand flashes as W25N01GV using the protocol to).
> + ER_4K = BIT(0),
> + ER_32K = BIT(1),
> + EEPROM = BIT(2),
> + SNOR_F_HAS_SR_TB = BIT(3),
> + SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
> +};
> +
> typedef struct FlashPartInfo {
> const char *part_name;
> /*
> @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
> { INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
> - { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
> + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
> + ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
> { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
> { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
> @@ -480,6 +479,11 @@ struct Flash {
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> + bool block_protect0;
> + bool block_protect1;
> + bool block_protect2;
> + bool block_protect3;
> + bool top_bottom_bit;
> bool status_register_write_disabled;
> uint8_t ear;
>
> @@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
> return;
> }
> + uint32_t block_protect_value = (s->block_protect3 << 3) |
> + (s->block_protect2 << 2) |
> + (s->block_protect1 << 1) |
> + (s->block_protect0 << 0);
> +
> + uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
> + uint32_t sector = addr / s->pi->sector_size;
> +
> + /* top_bottom_bit == 0 means TOP */
Indentation needs minor fixing on above lines, also the declarations should
be at the top of the function.
> + if (!s->top_bottom_bit) {
> + if (block_protect_value > 0 &&
> + s->pi->n_sectors <= sector + num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + } else {
> + if (block_protect_value > 0 && sector < num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + }
>
> if ((prev ^ data) & data) {
> trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> @@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
> break;
> case WRSR:
> s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> + s->block_protect0 = extract32(s->data[0], 2, 1);
> + s->block_protect1 = extract32(s->data[0], 3, 1);
> + s->block_protect2 = extract32(s->data[0], 4, 1);
> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> + s->top_bottom_bit = extract32(s->data[0], 5, 1);
> + }
> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> + s->block_protect3 = extract32(s->data[0], 6, 1);
> + }
>
> switch (get_man(s)) {
> case MAN_SPANSION:
> @@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> case RDSR:
> s->data[0] = (!!s->write_enable) << 1;
> s->data[0] |= (!!s->status_register_write_disabled) << 7;
> + s->data[0] |= (!!s->block_protect0) << 2;
> + s->data[0] |= (!!s->block_protect1) << 3;
> + s->data[0] |= (!!s->block_protect2) << 4;
> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> + s->data[0] |= (!!s->top_bottom_bit) << 5;
> + }
> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> + s->data[0] |= (!!s->block_protect3) << 6;
> + }
>
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> @@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
>
> s->wp_level = true;
> s->status_register_write_disabled = false;
> + s->block_protect0 = false;
> + s->block_protect1 = false;
> + s->block_protect2 = false;
> + s->block_protect3 = false;
> + s->top_bottom_bit = false;
We need to place above ones in a subsection in the vmstate (similar to the your
previous patch).
Looks good to me otherwise!
Thanks!
Best regards,
Francisco Iglesias
>
> reset_memory(s);
> }
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-07-01 11:40 ` Francisco Iglesias
@ 2022-07-01 12:23 ` Cédric Le Goater
2022-07-01 13:16 ` Francisco Iglesias
0 siblings, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2022-07-01 12:23 UTC (permalink / raw)
To: Francisco Iglesias, Iris Chen
Cc: pdel, qemu-devel, qemu-arm, patrick, alistair, kwolf, hreitz,
peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
On 7/1/22 13:40, Francisco Iglesias wrote:
> Hi Iris,
>
> Looks good, a couple of minor comments below!
>
> On [2022 Jun 27] Mon 11:52:33, Iris Chen wrote:
>> Signed-off-by: Iris Chen <irischenlj@fb.com>
>> ---
>> hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 62 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 50b523e5b1..0156a70f5e 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -38,21 +38,19 @@
>> #include "trace.h"
>> #include "qom/object.h"
>>
>> -/* Fields for FlashPartInfo->flags */
>> -
>> -/* erase capabilities */
>> -#define ER_4K 1
>> -#define ER_32K 2
>> -/* set to allow the page program command to write 0s back to 1. Useful for
>> - * modelling EEPROM with SPI flash command set
>> - */
>> -#define EEPROM 0x100
>> -
>> /* 16 MiB max in 3 byte address mode */
>> #define MAX_3BYTES_SIZE 0x1000000
>> -
>> #define SPI_NOR_MAX_ID_LEN 6
>>
>> +/* Fields for FlashPartInfo->flags */
>> +enum spi_nor_option_flags {
>
> (A suggestion is to s/nor/flash/ above (and s/SNOR_F_// below) since there
> looks to be nand flashes as W25N01GV using the protocol to).
>
>> + ER_4K = BIT(0),
>> + ER_32K = BIT(1),
>> + EEPROM = BIT(2),
>> + SNOR_F_HAS_SR_TB = BIT(3),
>> + SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
>> +};
>> +
>> typedef struct FlashPartInfo {
>> const char *part_name;
>> /*
>> @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
>> { INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
>> { INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
>> { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
>> - { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
>> + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
>> + ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
>> { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
>> { INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
>> { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
>> @@ -480,6 +479,11 @@ struct Flash {
>> bool reset_enable;
>> bool quad_enable;
>> bool aai_enable;
>> + bool block_protect0;
>> + bool block_protect1;
>> + bool block_protect2;
>> + bool block_protect3;
>> + bool top_bottom_bit;
>> bool status_register_write_disabled;
>> uint8_t ear;
>>
>> @@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
>> qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
>> return;
>> }
>> + uint32_t block_protect_value = (s->block_protect3 << 3) |
>> + (s->block_protect2 << 2) |
>> + (s->block_protect1 << 1) |
>> + (s->block_protect0 << 0);
>> +
>> + uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
>> + uint32_t sector = addr / s->pi->sector_size;
>> +
>> + /* top_bottom_bit == 0 means TOP */
>
> Indentation needs minor fixing on above lines, also the declarations should
> be at the top of the function.
I agree in that case it would be better to have at the top
but checkpatch does not complain. What's the rule ?
For loop indexes, I do prefer to declare in the block
statement.
>
>> + if (!s->top_bottom_bit) {
>> + if (block_protect_value > 0 &&
>> + s->pi->n_sectors <= sector + num_protected_sectors) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "M25P80: write with write protect!\n");
>> + return;
>> + }
>> + } else {
>> + if (block_protect_value > 0 && sector < num_protected_sectors) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "M25P80: write with write protect!\n");
>> + return;
>> + }
>> + }
>>
>> if ((prev ^ data) & data) {
>> trace_m25p80_programming_zero_to_one(s, addr, prev, data);
>> @@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
>> break;
>> case WRSR:
>> s->status_register_write_disabled = extract32(s->data[0], 7, 1);
>> + s->block_protect0 = extract32(s->data[0], 2, 1);
>> + s->block_protect1 = extract32(s->data[0], 3, 1);
>> + s->block_protect2 = extract32(s->data[0], 4, 1);
>> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
>> + s->top_bottom_bit = extract32(s->data[0], 5, 1);
>> + }
>> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
>> + s->block_protect3 = extract32(s->data[0], 6, 1);
>> + }
>>
>> switch (get_man(s)) {
>> case MAN_SPANSION:
>> @@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>> case RDSR:
>> s->data[0] = (!!s->write_enable) << 1;
>> s->data[0] |= (!!s->status_register_write_disabled) << 7;
>> + s->data[0] |= (!!s->block_protect0) << 2;
>> + s->data[0] |= (!!s->block_protect1) << 3;
>> + s->data[0] |= (!!s->block_protect2) << 4;
>> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
>> + s->data[0] |= (!!s->top_bottom_bit) << 5;
>> + }
>> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
>> + s->data[0] |= (!!s->block_protect3) << 6;
>> + }
>>
>> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
>> s->data[0] |= (!!s->quad_enable) << 6;
>> @@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
>>
>> s->wp_level = true;
>> s->status_register_write_disabled = false;
>> + s->block_protect0 = false;
>> + s->block_protect1 = false;
>> + s->block_protect2 = false;
>> + s->block_protect3 = false;
>> + s->top_bottom_bit = false;
>
> We need to place above ones in a subsection in the vmstate (similar to the your
> previous patch).
Ah yes. I keep forgetting these ...
Thanks for the second look Francisco.
C.
>
> Looks good to me otherwise!
>
> Thanks!
> Best regards,
> Francisco Iglesias
>
>>
>> reset_memory(s);
>> }
>> --
>> 2.30.2
>>
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-07-01 12:23 ` Cédric Le Goater
@ 2022-07-01 13:16 ` Francisco Iglesias
0 siblings, 0 replies; 13+ messages in thread
From: Francisco Iglesias @ 2022-07-01 13:16 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Iris Chen, pdel, qemu-devel, qemu-arm, patrick, alistair, kwolf,
hreitz, peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
On [2022 Jul 01] Fri 14:23:17, Cédric Le Goater wrote:
> On 7/1/22 13:40, Francisco Iglesias wrote:
> > Hi Iris,
> >
> > Looks good, a couple of minor comments below!
> >
> > On [2022 Jun 27] Mon 11:52:33, Iris Chen wrote:
> > > Signed-off-by: Iris Chen <irischenlj@fb.com>
> > > ---
> > > hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
> > > 1 file changed, 62 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > index 50b523e5b1..0156a70f5e 100644
> > > --- a/hw/block/m25p80.c
> > > +++ b/hw/block/m25p80.c
> > > @@ -38,21 +38,19 @@
> > > #include "trace.h"
> > > #include "qom/object.h"
> > > -/* Fields for FlashPartInfo->flags */
> > > -
> > > -/* erase capabilities */
> > > -#define ER_4K 1
> > > -#define ER_32K 2
> > > -/* set to allow the page program command to write 0s back to 1. Useful for
> > > - * modelling EEPROM with SPI flash command set
> > > - */
> > > -#define EEPROM 0x100
> > > -
> > > /* 16 MiB max in 3 byte address mode */
> > > #define MAX_3BYTES_SIZE 0x1000000
> > > -
> > > #define SPI_NOR_MAX_ID_LEN 6
> > > +/* Fields for FlashPartInfo->flags */
> > > +enum spi_nor_option_flags {
> >
> > (A suggestion is to s/nor/flash/ above (and s/SNOR_F_// below) since there
> > looks to be nand flashes as W25N01GV using the protocol to).
> >
> > > + ER_4K = BIT(0),
> > > + ER_32K = BIT(1),
> > > + EEPROM = BIT(2),
> > > + SNOR_F_HAS_SR_TB = BIT(3),
> > > + SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
> > > +};
> > > +
> > > typedef struct FlashPartInfo {
> > > const char *part_name;
> > > /*
> > > @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
> > > { INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
> > > { INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> > > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
> > > - { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
> > > + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
> > > + ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
> > > { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> > > { INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
> > > { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
> > > @@ -480,6 +479,11 @@ struct Flash {
> > > bool reset_enable;
> > > bool quad_enable;
> > > bool aai_enable;
> > > + bool block_protect0;
> > > + bool block_protect1;
> > > + bool block_protect2;
> > > + bool block_protect3;
> > > + bool top_bottom_bit;
> > > bool status_register_write_disabled;
> > > uint8_t ear;
> > > @@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> > > qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
> > > return;
> > > }
> > > + uint32_t block_protect_value = (s->block_protect3 << 3) |
> > > + (s->block_protect2 << 2) |
> > > + (s->block_protect1 << 1) |
> > > + (s->block_protect0 << 0);
> > > +
> > > + uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
> > > + uint32_t sector = addr / s->pi->sector_size;
> > > +
> > > + /* top_bottom_bit == 0 means TOP */
> >
> > Indentation needs minor fixing on above lines, also the declarations should
> > be at the top of the function.
>
Hi Cédric,
> I agree in that case it would be better to have at the top
> but checkpatch does not complain. What's the rule ?
>
Below is taken from docs/devel/style.rst:
"
Declarations
============
Mixed declarations (interleaving statements and declarations within
blocks) are generally not allowed; declarations should be at the beginning
of blocks.
Every now and then, an exception is made for declarations inside a
#ifdef or #ifndef block: if the code looks nicer, such declarations can
be placed at the top of the block even if there are statements above.
On the other hand, however, it's often best to move that #ifdef/#ifndef
block to a separate function altogether
"
I understand above as this is generally not allowed but in case you would
prefer to keep the declarations as they are placed one can see this as 'code
looks nicer' (to me the loop case is often cleaner in the way you are
mentioning). I should though have been more clear that 'moving to the top' was
a suggestion for not breaking this 'general rule' (also in case I've
interpreted this wrongly), sorry for that!
Best regards,
Francisco Iglesias
> For loop indexes, I do prefer to declare in the block
> statement.
>
> >
> > > + if (!s->top_bottom_bit) {
> > > + if (block_protect_value > 0 &&
> > > + s->pi->n_sectors <= sector + num_protected_sectors) {
> > > + qemu_log_mask(LOG_GUEST_ERROR,
> > > + "M25P80: write with write protect!\n");
> > > + return;
> > > + }
> > > + } else {
> > > + if (block_protect_value > 0 && sector < num_protected_sectors) {
> > > + qemu_log_mask(LOG_GUEST_ERROR,
> > > + "M25P80: write with write protect!\n");
> > > + return;
> > > + }
> > > + }
> > > if ((prev ^ data) & data) {
> > > trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> > > @@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
> > > break;
> > > case WRSR:
> > > s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> > > + s->block_protect0 = extract32(s->data[0], 2, 1);
> > > + s->block_protect1 = extract32(s->data[0], 3, 1);
> > > + s->block_protect2 = extract32(s->data[0], 4, 1);
> > > + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> > > + s->top_bottom_bit = extract32(s->data[0], 5, 1);
> > > + }
> > > + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> > > + s->block_protect3 = extract32(s->data[0], 6, 1);
> > > + }
> > > switch (get_man(s)) {
> > > case MAN_SPANSION:
> > > @@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> > > case RDSR:
> > > s->data[0] = (!!s->write_enable) << 1;
> > > s->data[0] |= (!!s->status_register_write_disabled) << 7;
> > > + s->data[0] |= (!!s->block_protect0) << 2;
> > > + s->data[0] |= (!!s->block_protect1) << 3;
> > > + s->data[0] |= (!!s->block_protect2) << 4;
> > > + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> > > + s->data[0] |= (!!s->top_bottom_bit) << 5;
> > > + }
> > > + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> > > + s->data[0] |= (!!s->block_protect3) << 6;
> > > + }
> > > if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> > > s->data[0] |= (!!s->quad_enable) << 6;
> > > @@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
> > > s->wp_level = true;
> > > s->status_register_write_disabled = false;
> > > + s->block_protect0 = false;
> > > + s->block_protect1 = false;
> > > + s->block_protect2 = false;
> > > + s->block_protect3 = false;
> > > + s->top_bottom_bit = false;
> >
> > We need to place above ones in a subsection in the vmstate (similar to the your
> > previous patch).
>
> Ah yes. I keep forgetting these ...
>
> Thanks for the second look Francisco.
>
> C.
>
>
> >
> > Looks good to me otherwise!
> >
> > Thanks!
> > Best regards,
> > Francisco Iglesias
> >
> > > reset_memory(s);
> > > }
> > > --
> > > 2.30.2
> > >
> > >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-06-27 18:52 ` [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom " Iris Chen
2022-07-01 7:11 ` Cédric Le Goater
2022-07-01 11:40 ` Francisco Iglesias
@ 2022-07-04 20:50 ` Cédric Le Goater
2 siblings, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2022-07-04 20:50 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, patrick, alistair, kwolf, hreitz,
peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
On 6/27/22 20:52, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
> hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 62 insertions(+), 12 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 50b523e5b1..0156a70f5e 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -38,21 +38,19 @@
> #include "trace.h"
> #include "qom/object.h"
>
> -/* Fields for FlashPartInfo->flags */
> -
> -/* erase capabilities */
> -#define ER_4K 1
> -#define ER_32K 2
> -/* set to allow the page program command to write 0s back to 1. Useful for
> - * modelling EEPROM with SPI flash command set
> - */
> -#define EEPROM 0x100
> -
> /* 16 MiB max in 3 byte address mode */
> #define MAX_3BYTES_SIZE 0x1000000
> -
> #define SPI_NOR_MAX_ID_LEN 6
>
> +/* Fields for FlashPartInfo->flags */
> +enum spi_nor_option_flags {
> + ER_4K = BIT(0),
> + ER_32K = BIT(1),
> + EEPROM = BIT(2),
> + SNOR_F_HAS_SR_TB = BIT(3),
> + SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
> +};
> +
> typedef struct FlashPartInfo {
> const char *part_name;
> /*
> @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
> { INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
> - { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
> + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
> + ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
> { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
> { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
> @@ -480,6 +479,11 @@ struct Flash {
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> + bool block_protect0;
> + bool block_protect1;
> + bool block_protect2;
> + bool block_protect3;
> + bool top_bottom_bit;
> bool status_register_write_disabled;
> uint8_t ear;
>
> @@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
> return;
> }
> + uint32_t block_protect_value = (s->block_protect3 << 3) |
> + (s->block_protect2 << 2) |
> + (s->block_protect1 << 1) |
> + (s->block_protect0 << 0);
> +
> + uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
Something wrong will occur if all block_protect[0123] are zeroes.
C.
> + uint32_t sector = addr / s->pi->sector_size;
> +
> + /* top_bottom_bit == 0 means TOP */
> + if (!s->top_bottom_bit) {
> + if (block_protect_value > 0 &&
> + s->pi->n_sectors <= sector + num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + } else {
> + if (block_protect_value > 0 && sector < num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + }
>
> if ((prev ^ data) & data) {
> trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> @@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
> break;
> case WRSR:
> s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> + s->block_protect0 = extract32(s->data[0], 2, 1);
> + s->block_protect1 = extract32(s->data[0], 3, 1);
> + s->block_protect2 = extract32(s->data[0], 4, 1);
> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> + s->top_bottom_bit = extract32(s->data[0], 5, 1);
> + }
> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> + s->block_protect3 = extract32(s->data[0], 6, 1);
> + }
>
> switch (get_man(s)) {
> case MAN_SPANSION:
> @@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> case RDSR:
> s->data[0] = (!!s->write_enable) << 1;
> s->data[0] |= (!!s->status_register_write_disabled) << 7;
> + s->data[0] |= (!!s->block_protect0) << 2;
> + s->data[0] |= (!!s->block_protect1) << 3;
> + s->data[0] |= (!!s->block_protect2) << 4;
> + if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> + s->data[0] |= (!!s->top_bottom_bit) << 5;
> + }
> + if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> + s->data[0] |= (!!s->block_protect3) << 6;
> + }
>
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> @@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
>
> s->wp_level = true;
> s->status_register_write_disabled = false;
> + s->block_protect0 = false;
> + s->block_protect1 = false;
> + s->block_protect2 = false;
> + s->block_protect3 = false;
> + s->top_bottom_bit = false;
>
> reset_memory(s);
> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-06-27 18:52 [PATCH 0/2] Add Block Protect (BP) and Top Bottom (TB) bits for write protect Iris Chen
2022-06-27 18:52 ` [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom " Iris Chen
2022-06-27 18:52 ` [PATCH 2/2] hw: m25p80: add tests for BP and TB bit " Iris Chen
@ 2022-07-07 2:16 ` Iris Chen
2022-07-07 10:36 ` Francisco Iglesias
2022-07-08 16:45 ` [PATCH v3] " Iris Chen
3 siblings, 1 reply; 13+ messages in thread
From: Iris Chen @ 2022-07-07 2:16 UTC (permalink / raw)
Cc: irischenlj, pdel, qemu-devel, qemu-arm, clg, patrick, alistair,
kwolf, hreitz, peter.maydell, andrew, joel, thuth, lvivier,
pbonzini, qemu-block, dz4list
Signed-off-by: Iris Chen <irischenlj@fb.com>
---
Addressing all comments.
In reponse to this comment:
"Something wrong will occur if all block_protect[0123] are zeroes",
the code actually ignores num_protected_sectors when block_protect_value = 0
which happens when block_protect[0123] are zeroes.
You can refer to the bottom block in v1, where we only look at cases when
block_protect_value > 0 so it is actually handled.
Regardless, since we were setting num_protected_sectors
in either case, I have refactored the code to make it more clear.
hw/block/m25p80.c | 103 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 91 insertions(+), 12 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 50b523e5b1..bddea9853b 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -38,21 +38,19 @@
#include "trace.h"
#include "qom/object.h"
-/* Fields for FlashPartInfo->flags */
-
-/* erase capabilities */
-#define ER_4K 1
-#define ER_32K 2
-/* set to allow the page program command to write 0s back to 1. Useful for
- * modelling EEPROM with SPI flash command set
- */
-#define EEPROM 0x100
-
/* 16 MiB max in 3 byte address mode */
#define MAX_3BYTES_SIZE 0x1000000
-
#define SPI_NOR_MAX_ID_LEN 6
+/* Fields for FlashPartInfo->flags */
+enum spi_option_flags {
+ ER_4K = BIT(0),
+ ER_32K = BIT(1),
+ EEPROM = BIT(2),
+ HAS_SR_TB = BIT(3),
+ HAS_SR_BP3_BIT6 = BIT(4),
+};
+
typedef struct FlashPartInfo {
const char *part_name;
/*
@@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
{ INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
- { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
+ { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
+ ER_4K | HAS_SR_BP3_BIT6 | HAS_SR_TB) },
{ INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
{ INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
@@ -480,6 +479,11 @@ struct Flash {
bool reset_enable;
bool quad_enable;
bool aai_enable;
+ bool block_protect0;
+ bool block_protect1;
+ bool block_protect2;
+ bool block_protect3;
+ bool top_bottom_bit;
bool status_register_write_disabled;
uint8_t ear;
@@ -626,11 +630,36 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
uint32_t page = addr / s->pi->page_size;
uint8_t prev = s->storage[s->cur_addr];
+ uint32_t block_protect_value = (s->block_protect3 << 3) |
+ (s->block_protect2 << 2) |
+ (s->block_protect1 << 1) |
+ (s->block_protect0 << 0);
+
if (!s->write_enable) {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
return;
}
+ if (block_protect_value > 0) {
+ uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
+ uint32_t sector = addr / s->pi->sector_size;
+
+ /* top_bottom_bit == 0 means TOP */
+ if (!s->top_bottom_bit) {
+ if (s->pi->n_sectors <= sector + num_protected_sectors) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: write with write protect!\n");
+ return;
+ }
+ } else {
+ if (sector < num_protected_sectors) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: write with write protect!\n");
+ return;
+ }
+ }
+ }
+
if ((prev ^ data) & data) {
trace_m25p80_programming_zero_to_one(s, addr, prev, data);
}
@@ -728,6 +757,15 @@ static void complete_collecting_data(Flash *s)
break;
case WRSR:
s->status_register_write_disabled = extract32(s->data[0], 7, 1);
+ s->block_protect0 = extract32(s->data[0], 2, 1);
+ s->block_protect1 = extract32(s->data[0], 3, 1);
+ s->block_protect2 = extract32(s->data[0], 4, 1);
+ if (s->pi->flags & HAS_SR_TB) {
+ s->top_bottom_bit = extract32(s->data[0], 5, 1);
+ }
+ if (s->pi->flags & HAS_SR_BP3_BIT6) {
+ s->block_protect3 = extract32(s->data[0], 6, 1);
+ }
switch (get_man(s)) {
case MAN_SPANSION:
@@ -1213,6 +1251,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
case RDSR:
s->data[0] = (!!s->write_enable) << 1;
s->data[0] |= (!!s->status_register_write_disabled) << 7;
+ s->data[0] |= (!!s->block_protect0) << 2;
+ s->data[0] |= (!!s->block_protect1) << 3;
+ s->data[0] |= (!!s->block_protect2) << 4;
+ if (s->pi->flags & HAS_SR_TB) {
+ s->data[0] |= (!!s->top_bottom_bit) << 5;
+ }
+ if (s->pi->flags & HAS_SR_BP3_BIT6) {
+ s->data[0] |= (!!s->block_protect3) << 6;
+ }
if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
s->data[0] |= (!!s->quad_enable) << 6;
@@ -1553,6 +1600,11 @@ static void m25p80_reset(DeviceState *d)
s->wp_level = true;
s->status_register_write_disabled = false;
+ s->block_protect0 = false;
+ s->block_protect1 = false;
+ s->block_protect2 = false;
+ s->block_protect3 = false;
+ s->top_bottom_bit = false;
reset_memory(s);
}
@@ -1639,6 +1691,32 @@ static const VMStateDescription vmstate_m25p80_write_protect = {
}
};
+static bool m25p80_block_protect_needed(void *opaque)
+{
+ Flash *s = (Flash *)opaque;
+
+ return s->block_protect0 ||
+ s->block_protect1 ||
+ s->block_protect2 ||
+ s->block_protect3 ||
+ s->top_bottom_bit;
+}
+
+static const VMStateDescription vmstate_m25p80_block_protect = {
+ .name = "m25p80/block_protect",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = m25p80_block_protect_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_BOOL(block_protect0, Flash),
+ VMSTATE_BOOL(block_protect1, Flash),
+ VMSTATE_BOOL(block_protect2, Flash),
+ VMSTATE_BOOL(block_protect3, Flash),
+ VMSTATE_BOOL(top_bottom_bit, Flash),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_m25p80 = {
.name = "m25p80",
.version_id = 0,
@@ -1671,6 +1749,7 @@ static const VMStateDescription vmstate_m25p80 = {
&vmstate_m25p80_data_read_loop,
&vmstate_m25p80_aai_enable,
&vmstate_m25p80_write_protect,
+ &vmstate_m25p80_block_protect,
NULL
}
};
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-07-07 2:16 ` [PATCH v2] hw: m25p80: Add Block Protect and Top Bottom bits for " Iris Chen
@ 2022-07-07 10:36 ` Francisco Iglesias
0 siblings, 0 replies; 13+ messages in thread
From: Francisco Iglesias @ 2022-07-07 10:36 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, clg, patrick, alistair, kwolf,
hreitz, peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
Hi Iris
On [2022 Jul 06] Wed 19:16:26, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
A couple of suggestions below if you would like to go for a v3 but otherwise:
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Thanks,
Best regards,
Francisco Iglesias
> ---
> Addressing all comments.
> In reponse to this comment:
> "Something wrong will occur if all block_protect[0123] are zeroes",
> the code actually ignores num_protected_sectors when block_protect_value = 0
> which happens when block_protect[0123] are zeroes.
>
> You can refer to the bottom block in v1, where we only look at cases when
> block_protect_value > 0 so it is actually handled.
> Regardless, since we were setting num_protected_sectors
> in either case, I have refactored the code to make it more clear.
>
> hw/block/m25p80.c | 103 ++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 91 insertions(+), 12 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 50b523e5b1..bddea9853b 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -38,21 +38,19 @@
> #include "trace.h"
> #include "qom/object.h"
>
> -/* Fields for FlashPartInfo->flags */
> -
> -/* erase capabilities */
> -#define ER_4K 1
> -#define ER_32K 2
> -/* set to allow the page program command to write 0s back to 1. Useful for
> - * modelling EEPROM with SPI flash command set
> - */
> -#define EEPROM 0x100
> -
> /* 16 MiB max in 3 byte address mode */
> #define MAX_3BYTES_SIZE 0x1000000
> -
> #define SPI_NOR_MAX_ID_LEN 6
>
> +/* Fields for FlashPartInfo->flags */
> +enum spi_option_flags {
A suggestion is to rename to spi_flash_option_flags (for being more clear that
it is flash option and not a SPI option).
> + ER_4K = BIT(0),
> + ER_32K = BIT(1),
> + EEPROM = BIT(2),
> + HAS_SR_TB = BIT(3),
> + HAS_SR_BP3_BIT6 = BIT(4),
> +};
> +
> typedef struct FlashPartInfo {
> const char *part_name;
> /*
> @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
> { INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
> - { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
> + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
> + ER_4K | HAS_SR_BP3_BIT6 | HAS_SR_TB) },
> { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
> { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
> @@ -480,6 +479,11 @@ struct Flash {
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> + bool block_protect0;
> + bool block_protect1;
> + bool block_protect2;
> + bool block_protect3;
> + bool top_bottom_bit;
> bool status_register_write_disabled;
> uint8_t ear;
>
> @@ -626,11 +630,36 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> uint32_t page = addr / s->pi->page_size;
> uint8_t prev = s->storage[s->cur_addr];
>
A cosmetic suggestion is to remove above blank line to keep the declarations
together.
> + uint32_t block_protect_value = (s->block_protect3 << 3) |
> + (s->block_protect2 << 2) |
> + (s->block_protect1 << 1) |
> + (s->block_protect0 << 0);
> +
> if (!s->write_enable) {
> qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
> return;
> }
>
> + if (block_protect_value > 0) {
> + uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
> + uint32_t sector = addr / s->pi->sector_size;
> +
> + /* top_bottom_bit == 0 means TOP */
> + if (!s->top_bottom_bit) {
> + if (s->pi->n_sectors <= sector + num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + } else {
> + if (sector < num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + }
> + }
> +
> if ((prev ^ data) & data) {
> trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> }
> @@ -728,6 +757,15 @@ static void complete_collecting_data(Flash *s)
> break;
> case WRSR:
> s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> + s->block_protect0 = extract32(s->data[0], 2, 1);
> + s->block_protect1 = extract32(s->data[0], 3, 1);
> + s->block_protect2 = extract32(s->data[0], 4, 1);
> + if (s->pi->flags & HAS_SR_TB) {
> + s->top_bottom_bit = extract32(s->data[0], 5, 1);
> + }
> + if (s->pi->flags & HAS_SR_BP3_BIT6) {
> + s->block_protect3 = extract32(s->data[0], 6, 1);
> + }
>
> switch (get_man(s)) {
> case MAN_SPANSION:
> @@ -1213,6 +1251,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> case RDSR:
> s->data[0] = (!!s->write_enable) << 1;
> s->data[0] |= (!!s->status_register_write_disabled) << 7;
> + s->data[0] |= (!!s->block_protect0) << 2;
> + s->data[0] |= (!!s->block_protect1) << 3;
> + s->data[0] |= (!!s->block_protect2) << 4;
> + if (s->pi->flags & HAS_SR_TB) {
> + s->data[0] |= (!!s->top_bottom_bit) << 5;
> + }
> + if (s->pi->flags & HAS_SR_BP3_BIT6) {
> + s->data[0] |= (!!s->block_protect3) << 6;
> + }
>
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> @@ -1553,6 +1600,11 @@ static void m25p80_reset(DeviceState *d)
>
> s->wp_level = true;
> s->status_register_write_disabled = false;
> + s->block_protect0 = false;
> + s->block_protect1 = false;
> + s->block_protect2 = false;
> + s->block_protect3 = false;
> + s->top_bottom_bit = false;
>
> reset_memory(s);
> }
> @@ -1639,6 +1691,32 @@ static const VMStateDescription vmstate_m25p80_write_protect = {
> }
> };
>
> +static bool m25p80_block_protect_needed(void *opaque)
> +{
> + Flash *s = (Flash *)opaque;
> +
> + return s->block_protect0 ||
> + s->block_protect1 ||
> + s->block_protect2 ||
> + s->block_protect3 ||
> + s->top_bottom_bit;
> +}
> +
> +static const VMStateDescription vmstate_m25p80_block_protect = {
> + .name = "m25p80/block_protect",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = m25p80_block_protect_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_BOOL(block_protect0, Flash),
> + VMSTATE_BOOL(block_protect1, Flash),
> + VMSTATE_BOOL(block_protect2, Flash),
> + VMSTATE_BOOL(block_protect3, Flash),
> + VMSTATE_BOOL(top_bottom_bit, Flash),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_m25p80 = {
> .name = "m25p80",
> .version_id = 0,
> @@ -1671,6 +1749,7 @@ static const VMStateDescription vmstate_m25p80 = {
> &vmstate_m25p80_data_read_loop,
> &vmstate_m25p80_aai_enable,
> &vmstate_m25p80_write_protect,
> + &vmstate_m25p80_block_protect,
> NULL
> }
> };
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-06-27 18:52 [PATCH 0/2] Add Block Protect (BP) and Top Bottom (TB) bits for write protect Iris Chen
` (2 preceding siblings ...)
2022-07-07 2:16 ` [PATCH v2] hw: m25p80: Add Block Protect and Top Bottom bits for " Iris Chen
@ 2022-07-08 16:45 ` Iris Chen
2022-07-08 17:44 ` Francisco Iglesias
3 siblings, 1 reply; 13+ messages in thread
From: Iris Chen @ 2022-07-08 16:45 UTC (permalink / raw)
Cc: irischenlj, pdel, qemu-devel, qemu-arm, clg, patrick, alistair,
kwolf, hreitz, peter.maydell, andrew, joel, thuth, lvivier,
pbonzini, qemu-block, dz4list
Signed-off-by: Iris Chen <irischenlj@fb.com>
---
Cosmetic suggestions addressed.
hw/block/m25p80.c | 102 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 90 insertions(+), 12 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 50b523e5b1..f3b401cf90 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -38,21 +38,19 @@
#include "trace.h"
#include "qom/object.h"
-/* Fields for FlashPartInfo->flags */
-
-/* erase capabilities */
-#define ER_4K 1
-#define ER_32K 2
-/* set to allow the page program command to write 0s back to 1. Useful for
- * modelling EEPROM with SPI flash command set
- */
-#define EEPROM 0x100
-
/* 16 MiB max in 3 byte address mode */
#define MAX_3BYTES_SIZE 0x1000000
-
#define SPI_NOR_MAX_ID_LEN 6
+/* Fields for FlashPartInfo->flags */
+enum spi_flash_option_flags {
+ ER_4K = BIT(0),
+ ER_32K = BIT(1),
+ EEPROM = BIT(2),
+ HAS_SR_TB = BIT(3),
+ HAS_SR_BP3_BIT6 = BIT(4),
+};
+
typedef struct FlashPartInfo {
const char *part_name;
/*
@@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
{ INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
- { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
+ { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
+ ER_4K | HAS_SR_BP3_BIT6 | HAS_SR_TB) },
{ INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
{ INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
@@ -480,6 +479,11 @@ struct Flash {
bool reset_enable;
bool quad_enable;
bool aai_enable;
+ bool block_protect0;
+ bool block_protect1;
+ bool block_protect2;
+ bool block_protect3;
+ bool top_bottom_bit;
bool status_register_write_disabled;
uint8_t ear;
@@ -625,12 +629,36 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
{
uint32_t page = addr / s->pi->page_size;
uint8_t prev = s->storage[s->cur_addr];
+ uint32_t block_protect_value = (s->block_protect3 << 3) |
+ (s->block_protect2 << 2) |
+ (s->block_protect1 << 1) |
+ (s->block_protect0 << 0);
if (!s->write_enable) {
qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
return;
}
+ if (block_protect_value > 0) {
+ uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
+ uint32_t sector = addr / s->pi->sector_size;
+
+ /* top_bottom_bit == 0 means TOP */
+ if (!s->top_bottom_bit) {
+ if (s->pi->n_sectors <= sector + num_protected_sectors) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: write with write protect!\n");
+ return;
+ }
+ } else {
+ if (sector < num_protected_sectors) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: write with write protect!\n");
+ return;
+ }
+ }
+ }
+
if ((prev ^ data) & data) {
trace_m25p80_programming_zero_to_one(s, addr, prev, data);
}
@@ -728,6 +756,15 @@ static void complete_collecting_data(Flash *s)
break;
case WRSR:
s->status_register_write_disabled = extract32(s->data[0], 7, 1);
+ s->block_protect0 = extract32(s->data[0], 2, 1);
+ s->block_protect1 = extract32(s->data[0], 3, 1);
+ s->block_protect2 = extract32(s->data[0], 4, 1);
+ if (s->pi->flags & HAS_SR_TB) {
+ s->top_bottom_bit = extract32(s->data[0], 5, 1);
+ }
+ if (s->pi->flags & HAS_SR_BP3_BIT6) {
+ s->block_protect3 = extract32(s->data[0], 6, 1);
+ }
switch (get_man(s)) {
case MAN_SPANSION:
@@ -1213,6 +1250,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
case RDSR:
s->data[0] = (!!s->write_enable) << 1;
s->data[0] |= (!!s->status_register_write_disabled) << 7;
+ s->data[0] |= (!!s->block_protect0) << 2;
+ s->data[0] |= (!!s->block_protect1) << 3;
+ s->data[0] |= (!!s->block_protect2) << 4;
+ if (s->pi->flags & HAS_SR_TB) {
+ s->data[0] |= (!!s->top_bottom_bit) << 5;
+ }
+ if (s->pi->flags & HAS_SR_BP3_BIT6) {
+ s->data[0] |= (!!s->block_protect3) << 6;
+ }
if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
s->data[0] |= (!!s->quad_enable) << 6;
@@ -1553,6 +1599,11 @@ static void m25p80_reset(DeviceState *d)
s->wp_level = true;
s->status_register_write_disabled = false;
+ s->block_protect0 = false;
+ s->block_protect1 = false;
+ s->block_protect2 = false;
+ s->block_protect3 = false;
+ s->top_bottom_bit = false;
reset_memory(s);
}
@@ -1639,6 +1690,32 @@ static const VMStateDescription vmstate_m25p80_write_protect = {
}
};
+static bool m25p80_block_protect_needed(void *opaque)
+{
+ Flash *s = (Flash *)opaque;
+
+ return s->block_protect0 ||
+ s->block_protect1 ||
+ s->block_protect2 ||
+ s->block_protect3 ||
+ s->top_bottom_bit;
+}
+
+static const VMStateDescription vmstate_m25p80_block_protect = {
+ .name = "m25p80/block_protect",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = m25p80_block_protect_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_BOOL(block_protect0, Flash),
+ VMSTATE_BOOL(block_protect1, Flash),
+ VMSTATE_BOOL(block_protect2, Flash),
+ VMSTATE_BOOL(block_protect3, Flash),
+ VMSTATE_BOOL(top_bottom_bit, Flash),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_m25p80 = {
.name = "m25p80",
.version_id = 0,
@@ -1671,6 +1748,7 @@ static const VMStateDescription vmstate_m25p80 = {
&vmstate_m25p80_data_read_loop,
&vmstate_m25p80_aai_enable,
&vmstate_m25p80_write_protect,
+ &vmstate_m25p80_block_protect,
NULL
}
};
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] hw: m25p80: Add Block Protect and Top Bottom bits for write protect
2022-07-08 16:45 ` [PATCH v3] " Iris Chen
@ 2022-07-08 17:44 ` Francisco Iglesias
0 siblings, 0 replies; 13+ messages in thread
From: Francisco Iglesias @ 2022-07-08 17:44 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, clg, patrick, alistair, kwolf,
hreitz, peter.maydell, andrew, joel, thuth, lvivier, pbonzini,
qemu-block, dz4list
On [2022 Jul 08] Fri 09:45:52, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> ---
> Cosmetic suggestions addressed.
>
> hw/block/m25p80.c | 102 ++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 90 insertions(+), 12 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 50b523e5b1..f3b401cf90 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -38,21 +38,19 @@
> #include "trace.h"
> #include "qom/object.h"
>
> -/* Fields for FlashPartInfo->flags */
> -
> -/* erase capabilities */
> -#define ER_4K 1
> -#define ER_32K 2
> -/* set to allow the page program command to write 0s back to 1. Useful for
> - * modelling EEPROM with SPI flash command set
> - */
> -#define EEPROM 0x100
> -
> /* 16 MiB max in 3 byte address mode */
> #define MAX_3BYTES_SIZE 0x1000000
> -
> #define SPI_NOR_MAX_ID_LEN 6
>
> +/* Fields for FlashPartInfo->flags */
> +enum spi_flash_option_flags {
> + ER_4K = BIT(0),
> + ER_32K = BIT(1),
> + EEPROM = BIT(2),
> + HAS_SR_TB = BIT(3),
> + HAS_SR_BP3_BIT6 = BIT(4),
> +};
> +
> typedef struct FlashPartInfo {
> const char *part_name;
> /*
> @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
> { INFO("n25q512a11", 0x20bb20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512a13", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
> - { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) },
> + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512,
> + ER_4K | HAS_SR_BP3_BIT6 | HAS_SR_TB) },
> { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) },
> { INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
> { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
> @@ -480,6 +479,11 @@ struct Flash {
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> + bool block_protect0;
> + bool block_protect1;
> + bool block_protect2;
> + bool block_protect3;
> + bool top_bottom_bit;
> bool status_register_write_disabled;
> uint8_t ear;
>
> @@ -625,12 +629,36 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> {
> uint32_t page = addr / s->pi->page_size;
> uint8_t prev = s->storage[s->cur_addr];
> + uint32_t block_protect_value = (s->block_protect3 << 3) |
> + (s->block_protect2 << 2) |
> + (s->block_protect1 << 1) |
> + (s->block_protect0 << 0);
>
> if (!s->write_enable) {
> qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
> return;
> }
>
> + if (block_protect_value > 0) {
> + uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
> + uint32_t sector = addr / s->pi->sector_size;
> +
> + /* top_bottom_bit == 0 means TOP */
> + if (!s->top_bottom_bit) {
> + if (s->pi->n_sectors <= sector + num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + } else {
> + if (sector < num_protected_sectors) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: write with write protect!\n");
> + return;
> + }
> + }
> + }
> +
> if ((prev ^ data) & data) {
> trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> }
> @@ -728,6 +756,15 @@ static void complete_collecting_data(Flash *s)
> break;
> case WRSR:
> s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> + s->block_protect0 = extract32(s->data[0], 2, 1);
> + s->block_protect1 = extract32(s->data[0], 3, 1);
> + s->block_protect2 = extract32(s->data[0], 4, 1);
> + if (s->pi->flags & HAS_SR_TB) {
> + s->top_bottom_bit = extract32(s->data[0], 5, 1);
> + }
> + if (s->pi->flags & HAS_SR_BP3_BIT6) {
> + s->block_protect3 = extract32(s->data[0], 6, 1);
> + }
>
> switch (get_man(s)) {
> case MAN_SPANSION:
> @@ -1213,6 +1250,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> case RDSR:
> s->data[0] = (!!s->write_enable) << 1;
> s->data[0] |= (!!s->status_register_write_disabled) << 7;
> + s->data[0] |= (!!s->block_protect0) << 2;
> + s->data[0] |= (!!s->block_protect1) << 3;
> + s->data[0] |= (!!s->block_protect2) << 4;
> + if (s->pi->flags & HAS_SR_TB) {
> + s->data[0] |= (!!s->top_bottom_bit) << 5;
> + }
> + if (s->pi->flags & HAS_SR_BP3_BIT6) {
> + s->data[0] |= (!!s->block_protect3) << 6;
> + }
>
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> @@ -1553,6 +1599,11 @@ static void m25p80_reset(DeviceState *d)
>
> s->wp_level = true;
> s->status_register_write_disabled = false;
> + s->block_protect0 = false;
> + s->block_protect1 = false;
> + s->block_protect2 = false;
> + s->block_protect3 = false;
> + s->top_bottom_bit = false;
>
> reset_memory(s);
> }
> @@ -1639,6 +1690,32 @@ static const VMStateDescription vmstate_m25p80_write_protect = {
> }
> };
>
> +static bool m25p80_block_protect_needed(void *opaque)
> +{
> + Flash *s = (Flash *)opaque;
> +
> + return s->block_protect0 ||
> + s->block_protect1 ||
> + s->block_protect2 ||
> + s->block_protect3 ||
> + s->top_bottom_bit;
> +}
> +
> +static const VMStateDescription vmstate_m25p80_block_protect = {
> + .name = "m25p80/block_protect",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = m25p80_block_protect_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_BOOL(block_protect0, Flash),
> + VMSTATE_BOOL(block_protect1, Flash),
> + VMSTATE_BOOL(block_protect2, Flash),
> + VMSTATE_BOOL(block_protect3, Flash),
> + VMSTATE_BOOL(top_bottom_bit, Flash),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_m25p80 = {
> .name = "m25p80",
> .version_id = 0,
> @@ -1671,6 +1748,7 @@ static const VMStateDescription vmstate_m25p80 = {
> &vmstate_m25p80_data_read_loop,
> &vmstate_m25p80_aai_enable,
> &vmstate_m25p80_write_protect,
> + &vmstate_m25p80_block_protect,
> NULL
> }
> };
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-07-08 17:51 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 18:52 [PATCH 0/2] Add Block Protect (BP) and Top Bottom (TB) bits for write protect Iris Chen
2022-06-27 18:52 ` [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom " Iris Chen
2022-07-01 7:11 ` Cédric Le Goater
2022-07-01 11:40 ` Francisco Iglesias
2022-07-01 12:23 ` Cédric Le Goater
2022-07-01 13:16 ` Francisco Iglesias
2022-07-04 20:50 ` Cédric Le Goater
2022-06-27 18:52 ` [PATCH 2/2] hw: m25p80: add tests for BP and TB bit " Iris Chen
2022-07-01 7:24 ` Cédric Le Goater
2022-07-07 2:16 ` [PATCH v2] hw: m25p80: Add Block Protect and Top Bottom bits for " Iris Chen
2022-07-07 10:36 ` Francisco Iglesias
2022-07-08 16:45 ` [PATCH v3] " Iris Chen
2022-07-08 17:44 ` Francisco Iglesias
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.