* [PATCH v3 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
@ 2022-06-17 22:02 Iris Chen
2022-06-17 22:48 ` Dan Zhang
2022-06-20 9:10 ` Francisco Iglesias
0 siblings, 2 replies; 3+ messages in thread
From: Iris Chen @ 2022-06-17 22:02 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>
---
Thanks everyone for your comments. This is a v3 patch that addresses all
suggestions (moving write_enable to decode_new_cmd).
I am waiting on some feedback from Dan's (dz4list@gmail.com) patch
regarding adding a STATE_STANDBY state.
Currently, all tests are passing.
hw/block/m25p80.c | 77 ++++++++++++++++++++++++++++++++++++++---------
1 file changed, 62 insertions(+), 15 deletions(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 81ba3da4df..12a59ca57c 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -27,12 +27,14 @@
#include "hw/qdev-properties.h"
#include "hw/qdev-properties-system.h"
#include "hw/ssi/ssi.h"
+#include "hw/irq.h"
#include "migration/vmstate.h"
#include "qemu/bitops.h"
#include "qemu/log.h"
#include "qemu/module.h"
#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "qapi/visitor.h"
#include "trace.h"
#include "qom/object.h"
@@ -472,11 +474,13 @@ struct Flash {
uint8_t spansion_cr2v;
uint8_t spansion_cr3v;
uint8_t spansion_cr4v;
+ bool wp_level;
bool write_enable;
bool four_bytes_address_mode;
bool reset_enable;
bool quad_enable;
bool aai_enable;
+ bool status_register_write_disabled;
uint8_t ear;
int64_t dirty_page;
@@ -723,6 +727,8 @@ static void complete_collecting_data(Flash *s)
flash_erase(s, s->cur_addr, s->cmd_in_progress);
break;
case WRSR:
+ s->status_register_write_disabled = extract32(s->data[0], 7, 1);
+
switch (get_man(s)) {
case MAN_SPANSION:
s->quad_enable = !!(s->data[1] & 0x02);
@@ -1165,22 +1171,34 @@ static void decode_new_cmd(Flash *s, uint32_t value)
break;
case WRSR:
- if (s->write_enable) {
- switch (get_man(s)) {
- case MAN_SPANSION:
- s->needed_bytes = 2;
- s->state = STATE_COLLECTING_DATA;
- break;
- case MAN_MACRONIX:
- s->needed_bytes = 2;
- s->state = STATE_COLLECTING_VAR_LEN_DATA;
- break;
- default:
- s->needed_bytes = 1;
- s->state = STATE_COLLECTING_DATA;
- }
- s->pos = 0;
+ /*
+ * If WP# is low and status_register_write_disabled is high,
+ * status register writes are disabled.
+ * This is also called "hardware protected mode" (HPM). All other
+ * combinations of the two states are called "software protected mode"
+ * (SPM), and status register writes are permitted.
+ */
+ if ((s->wp_level == 0 && s->status_register_write_disabled)
+ || !s->write_enable) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: Status register write is disabled!\n");
+ break;
}
+
+ switch (get_man(s)) {
+ case MAN_SPANSION:
+ s->needed_bytes = 2;
+ s->state = STATE_COLLECTING_DATA;
+ break;
+ case MAN_MACRONIX:
+ s->needed_bytes = 2;
+ s->state = STATE_COLLECTING_VAR_LEN_DATA;
+ break;
+ default:
+ s->needed_bytes = 1;
+ s->state = STATE_COLLECTING_DATA;
+ }
+ s->pos = 0;
break;
case WRDI:
@@ -1195,6 +1213,8 @@ 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;
+
if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
s->data[0] |= (!!s->quad_enable) << 6;
}
@@ -1484,6 +1504,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx)
return r;
}
+static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int level)
+{
+ Flash *s = M25P80(opaque);
+ /* WP# is just a single pin. */
+ assert(n == 0);
+ s->wp_level = !!level;
+}
+
static void m25p80_realize(SSIPeripheral *ss, Error **errp)
{
Flash *s = M25P80(ss);
@@ -1515,12 +1543,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
s->storage = blk_blockalign(NULL, s->size);
memset(s->storage, 0xFF, s->size);
}
+
+ qdev_init_gpio_in_named(DEVICE(s),
+ m25p80_write_protect_pin_irq_handler, "WP#", 1);
}
static void m25p80_reset(DeviceState *d)
{
Flash *s = M25P80(d);
+ s->wp_level = true;
+ s->status_register_write_disabled = false;
+
reset_memory(s);
}
@@ -1587,6 +1621,18 @@ static const VMStateDescription vmstate_m25p80_aai_enable = {
}
};
+static const VMStateDescription vmstate_m25p80_write_protect = {
+ .name = "m25p80/write_protect",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = false,
+ .fields = (VMStateField[]) {
+ VMSTATE_BOOL(wp_level, Flash),
+ VMSTATE_BOOL(status_register_write_disabled, Flash),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription vmstate_m25p80 = {
.name = "m25p80",
.version_id = 0,
@@ -1618,6 +1664,7 @@ static const VMStateDescription vmstate_m25p80 = {
.subsections = (const VMStateDescription * []) {
&vmstate_m25p80_data_read_loop,
&vmstate_m25p80_aai_enable,
+ &vmstate_m25p80_write_protect,
NULL
}
};
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
2022-06-17 22:02 [PATCH v3 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection Iris Chen
@ 2022-06-17 22:48 ` Dan Zhang
2022-06-20 9:10 ` Francisco Iglesias
1 sibling, 0 replies; 3+ messages in thread
From: Dan Zhang @ 2022-06-17 22:48 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
On Fri, Jun 17, 2022 at 03:02:45PM -0700, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
> Thanks everyone for your comments. This is a v3 patch that addresses all
> suggestions (moving write_enable to decode_new_cmd).
> I am waiting on some feedback from Dan's (dz4list@gmail.com) patch
> regarding adding a STATE_STANDBY state.
The STATE_STANDBY can be use to handle the current code's potential issue:
The two data bytes of the WRSR instruction will get interprted as the
new command when abort because of HPM is on. If the data bytes coincident
be a legimtate command, the model will introduce tricky behavior when
HPM is on.
BRs
Dan
>
> Currently, all tests are passing.
>
> hw/block/m25p80.c | 77 ++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 81ba3da4df..12a59ca57c 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -27,12 +27,14 @@
> #include "hw/qdev-properties.h"
> #include "hw/qdev-properties-system.h"
> #include "hw/ssi/ssi.h"
> +#include "hw/irq.h"
> #include "migration/vmstate.h"
> #include "qemu/bitops.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> #include "qemu/error-report.h"
> #include "qapi/error.h"
> +#include "qapi/visitor.h"
> #include "trace.h"
> #include "qom/object.h"
>
> @@ -472,11 +474,13 @@ struct Flash {
> uint8_t spansion_cr2v;
> uint8_t spansion_cr3v;
> uint8_t spansion_cr4v;
> + bool wp_level;
> bool write_enable;
> bool four_bytes_address_mode;
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> + bool status_register_write_disabled;
> uint8_t ear;
>
> int64_t dirty_page;
> @@ -723,6 +727,8 @@ static void complete_collecting_data(Flash *s)
> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> break;
> case WRSR:
> + s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> +
> switch (get_man(s)) {
> case MAN_SPANSION:
> s->quad_enable = !!(s->data[1] & 0x02);
> @@ -1165,22 +1171,34 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> break;
>
> case WRSR:
> - if (s->write_enable) {
> - switch (get_man(s)) {
> - case MAN_SPANSION:
> - s->needed_bytes = 2;
> - s->state = STATE_COLLECTING_DATA;
> - break;
> - case MAN_MACRONIX:
> - s->needed_bytes = 2;
> - s->state = STATE_COLLECTING_VAR_LEN_DATA;
> - break;
> - default:
> - s->needed_bytes = 1;
> - s->state = STATE_COLLECTING_DATA;
> - }
> - s->pos = 0;
> + /*
> + * If WP# is low and status_register_write_disabled is high,
> + * status register writes are disabled.
> + * This is also called "hardware protected mode" (HPM). All other
> + * combinations of the two states are called "software protected mode"
> + * (SPM), and status register writes are permitted.
> + */
> + if ((s->wp_level == 0 && s->status_register_write_disabled)
> + || !s->write_enable) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: Status register write is disabled!\n");
> + break;
> }
> +
> + switch (get_man(s)) {
> + case MAN_SPANSION:
> + s->needed_bytes = 2;
> + s->state = STATE_COLLECTING_DATA;
> + break;
> + case MAN_MACRONIX:
> + s->needed_bytes = 2;
> + s->state = STATE_COLLECTING_VAR_LEN_DATA;
> + break;
> + default:
> + s->needed_bytes = 1;
> + s->state = STATE_COLLECTING_DATA;
> + }
> + s->pos = 0;
> break;
>
> case WRDI:
> @@ -1195,6 +1213,8 @@ 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;
> +
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> }
> @@ -1484,6 +1504,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx)
> return r;
> }
>
> +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int level)
> +{
> + Flash *s = M25P80(opaque);
> + /* WP# is just a single pin. */
> + assert(n == 0);
> + s->wp_level = !!level;
> +}
> +
> static void m25p80_realize(SSIPeripheral *ss, Error **errp)
> {
> Flash *s = M25P80(ss);
> @@ -1515,12 +1543,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
> s->storage = blk_blockalign(NULL, s->size);
> memset(s->storage, 0xFF, s->size);
> }
> +
> + qdev_init_gpio_in_named(DEVICE(s),
> + m25p80_write_protect_pin_irq_handler, "WP#", 1);
> }
>
> static void m25p80_reset(DeviceState *d)
> {
> Flash *s = M25P80(d);
>
> + s->wp_level = true;
> + s->status_register_write_disabled = false;
> +
> reset_memory(s);
> }
>
> @@ -1587,6 +1621,18 @@ static const VMStateDescription vmstate_m25p80_aai_enable = {
> }
> };
>
> +static const VMStateDescription vmstate_m25p80_write_protect = {
> + .name = "m25p80/write_protect",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = false,
> + .fields = (VMStateField[]) {
> + VMSTATE_BOOL(wp_level, Flash),
> + VMSTATE_BOOL(status_register_write_disabled, Flash),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_m25p80 = {
> .name = "m25p80",
> .version_id = 0,
> @@ -1618,6 +1664,7 @@ static const VMStateDescription vmstate_m25p80 = {
> .subsections = (const VMStateDescription * []) {
> &vmstate_m25p80_data_read_loop,
> &vmstate_m25p80_aai_enable,
> + &vmstate_m25p80_write_protect,
> NULL
> }
> };
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
2022-06-17 22:02 [PATCH v3 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection Iris Chen
2022-06-17 22:48 ` Dan Zhang
@ 2022-06-20 9:10 ` Francisco Iglesias
1 sibling, 0 replies; 3+ messages in thread
From: Francisco Iglesias @ 2022-06-20 9:10 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 Jun 17] Fri 15:02:45, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
> Thanks everyone for your comments. This is a v3 patch that addresses all
> suggestions (moving write_enable to decode_new_cmd).
> I am waiting on some feedback from Dan's (dz4list@gmail.com) patch
> regarding adding a STATE_STANDBY state.
>
> Currently, all tests are passing.
>
> hw/block/m25p80.c | 77 ++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 62 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 81ba3da4df..12a59ca57c 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -27,12 +27,14 @@
> #include "hw/qdev-properties.h"
> #include "hw/qdev-properties-system.h"
> #include "hw/ssi/ssi.h"
> +#include "hw/irq.h"
> #include "migration/vmstate.h"
> #include "qemu/bitops.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> #include "qemu/error-report.h"
> #include "qapi/error.h"
> +#include "qapi/visitor.h"
We can remove above two new header includes (sorry for missing this last time).
> #include "trace.h"
> #include "qom/object.h"
>
> @@ -472,11 +474,13 @@ struct Flash {
> uint8_t spansion_cr2v;
> uint8_t spansion_cr3v;
> uint8_t spansion_cr4v;
> + bool wp_level;
> bool write_enable;
> bool four_bytes_address_mode;
> bool reset_enable;
> bool quad_enable;
> bool aai_enable;
> + bool status_register_write_disabled;
> uint8_t ear;
>
> int64_t dirty_page;
> @@ -723,6 +727,8 @@ static void complete_collecting_data(Flash *s)
> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> break;
> case WRSR:
> + s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> +
> switch (get_man(s)) {
> case MAN_SPANSION:
> s->quad_enable = !!(s->data[1] & 0x02);
> @@ -1165,22 +1171,34 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> break;
>
> case WRSR:
> - if (s->write_enable) {
> - switch (get_man(s)) {
> - case MAN_SPANSION:
> - s->needed_bytes = 2;
> - s->state = STATE_COLLECTING_DATA;
> - break;
> - case MAN_MACRONIX:
> - s->needed_bytes = 2;
> - s->state = STATE_COLLECTING_VAR_LEN_DATA;
> - break;
> - default:
> - s->needed_bytes = 1;
> - s->state = STATE_COLLECTING_DATA;
> - }
> - s->pos = 0;
> + /*
> + * If WP# is low and status_register_write_disabled is high,
> + * status register writes are disabled.
> + * This is also called "hardware protected mode" (HPM). All other
> + * combinations of the two states are called "software protected mode"
> + * (SPM), and status register writes are permitted.
> + */
> + if ((s->wp_level == 0 && s->status_register_write_disabled)
> + || !s->write_enable) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: Status register write is disabled!\n");
> + break;
> }
> +
> + switch (get_man(s)) {
> + case MAN_SPANSION:
> + s->needed_bytes = 2;
> + s->state = STATE_COLLECTING_DATA;
> + break;
> + case MAN_MACRONIX:
> + s->needed_bytes = 2;
> + s->state = STATE_COLLECTING_VAR_LEN_DATA;
> + break;
> + default:
> + s->needed_bytes = 1;
> + s->state = STATE_COLLECTING_DATA;
> + }
> + s->pos = 0;
> break;
>
> case WRDI:
> @@ -1195,6 +1213,8 @@ 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;
> +
> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
> s->data[0] |= (!!s->quad_enable) << 6;
> }
> @@ -1484,6 +1504,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx)
> return r;
> }
>
> +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int level)
> +{
> + Flash *s = M25P80(opaque);
> + /* WP# is just a single pin. */
> + assert(n == 0);
> + s->wp_level = !!level;
> +}
> +
> static void m25p80_realize(SSIPeripheral *ss, Error **errp)
> {
> Flash *s = M25P80(ss);
> @@ -1515,12 +1543,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
> s->storage = blk_blockalign(NULL, s->size);
> memset(s->storage, 0xFF, s->size);
> }
> +
> + qdev_init_gpio_in_named(DEVICE(s),
> + m25p80_write_protect_pin_irq_handler, "WP#", 1);
> }
>
> static void m25p80_reset(DeviceState *d)
> {
> Flash *s = M25P80(d);
>
> + s->wp_level = true;
> + s->status_register_write_disabled = false;
> +
> reset_memory(s);
> }
>
> @@ -1587,6 +1621,18 @@ static const VMStateDescription vmstate_m25p80_aai_enable = {
> }
> };
>
> +static const VMStateDescription vmstate_m25p80_write_protect = {
> + .name = "m25p80/write_protect",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = false,
Above is a function, something like:
static bool m25p80_wp_level_srwd_needed(void *opaque)
{
Flash *s = (Flash *)opaque;
return !s->wp_level || s->status_register_write_disabled;
}
.needed = m25p80_wp_level_srwd_needed,
Looks good otherwise!
Thanks,
Best regards,
Francisco
> + .fields = (VMStateField[]) {
> + VMSTATE_BOOL(wp_level, Flash),
> + VMSTATE_BOOL(status_register_write_disabled, Flash),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static const VMStateDescription vmstate_m25p80 = {
> .name = "m25p80",
> .version_id = 0,
> @@ -1618,6 +1664,7 @@ static const VMStateDescription vmstate_m25p80 = {
> .subsections = (const VMStateDescription * []) {
> &vmstate_m25p80_data_read_loop,
> &vmstate_m25p80_aai_enable,
> + &vmstate_m25p80_write_protect,
> NULL
> }
> };
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-20 9:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-17 22:02 [PATCH v3 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection Iris Chen
2022-06-17 22:48 ` Dan Zhang
2022-06-20 9:10 ` Francisco Iglesias
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).