hw/block/m25p80.c | 103 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 91 insertions(+), 12 deletions(-)
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.2Hi 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
>
>
© 2016 - 2024 Red Hat, Inc.