Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
Hello Dan
On 6/14/22 07:45, Dan Zhang wrote:
Just find out how to use mutt to reply all in the thread.
repeat the previous comments. Add STATE_HIZ to handle decode_new_command
aborting gracefully.
On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
wrote:
Hi Iris,
Looks good some, a couple of comments below.
On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
From: Iris Chen
Signed-off-by: Iris Chen
---
Addressed all comments from V1. The biggest change: removed
object_class_property_add.
hw/block/m25p80.c | 37 +++
tests/qtest/aspeed_smc-test.c | 2 ++
2 files changed, 39 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
flash_erase(s, s->cur_addr, s->cmd_in_progress);
break;
case WRSR:
+/*
+ * 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) {
'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
command, otherwise the state machinery will not advance to this function
(meaning that above check for !s->write_enable will never hit as far as I can
tell). A suggestion is to move the check for wp_level and
status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
Oh good catch! Yes actually, in our fork, we also removed the write_enable
guard in decode_new_cmd. We either need both checks in decode_new_cmd,
or both checks in complete_collecting_data.
I think we had some difficulty deciding whether to block command decoding,
or to decode and ignore the command if restrictions are enabled.
The reason being that, in the qtest, the WRSR command code gets ignored, and
then the subsequent write data gets interpreted as some random command code.
We had elected to decode and ignore the command, but I think the
datasheet actually describes that the command won’t be decoded successfully,
so you’re probably right, we should put this logic in decode_new_cmd.
Most likely, the qtest will also need to be modified to reset the transfer
state machine after a blocked write command. I can’t remember if
exiting and re-entering user mode is sufficient for that, but something
like that is probably possible.
Thanks for catching this!
Peter
I am proposing add a CMDState: STATE_HIZ to handle command decode fail
situation. When decode_new_command need abort the decoding and ignore
following
on input bytes of this transaction, set the state to STATE_HIZ.
And m25p80_transfer8() will ignore all the following on byte when in
this state.
This is to simulating the real device operation behavior
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
```
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
```
Could you please send a patch ?
Thanks,
C.
BRs
Dan Zhang
+qemu_log_mask(LOG_GUEST_ERROR,
+ "M25P80: Status register write is disabled!\n");
+break;
+}
+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);
@@ -1195,6 +1214,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 +1505,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss,
uint32_t tx)
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
On Mon, Jun 13, 2022 at 10:45:34PM -0700, Dan Zhang wrote:
> Just find out how to use mutt to reply all in the thread.
> repeat the previous comments. Add STATE_HIZ to handle decode_new_command
> aborting gracefully.
>
> On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
> >
> >
> > > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
> > > wrote:
> > >
> > > Hi Iris,
> > >
> > > Looks good some, a couple of comments below.
> > >
> > > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> > >> From: Iris Chen
> > >>
> > >> Signed-off-by: Iris Chen
> > >> ---
> > >> Addressed all comments from V1. The biggest change: removed
> > >> object_class_property_add.
> > >>
> > >> hw/block/m25p80.c | 37 +++
> > >> tests/qtest/aspeed_smc-test.c | 2 ++
> > >> 2 files changed, 39 insertions(+)
> > >>
> > >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > >> index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
> > >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> > >> break;
> > >> case WRSR:
> > >> +/*
> > >> + * 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) {
> > >
> > > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > > command, otherwise the state machinery will not advance to this function
> > > (meaning that above check for !s->write_enable will never hit as far as I
> > > can
> > > tell). A suggestion is to move the check for wp_level and
> > > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
> >
> > Oh good catch! Yes actually, in our fork, we also removed the write_enable
> > guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> > or both checks in complete_collecting_data.
> >
> > I think we had some difficulty deciding whether to block command decoding,
> > or to decode and ignore the command if restrictions are enabled.
> >
> > The reason being that, in the qtest, the WRSR command code gets ignored, and
> > then the subsequent write data gets interpreted as some random command code.
> > We had elected to decode and ignore the command, but I think the
> > datasheet actually describes that the command won’t be decoded successfully,
> > so you’re probably right, we should put this logic in decode_new_cmd.
> >
> > Most likely, the qtest will also need to be modified to reset the transfer
> > state machine after a blocked write command. I can’t remember if
> > exiting and re-entering user mode is sufficient for that, but something
> > like that is probably possible.
> >
> > Thanks for catching this!
> > Peter
> >
>
> I am proposing add a CMDState: STATE_HIZ to handle command decode fail
> situation. When decode_new_command need abort the decoding and ignore
> following
> on input bytes of this transaction, set the state to STATE_HIZ.
> And m25p80_transfer8() will ignore all the following on byte when in
> this state.
>
> This is to simulating the real device operation behavior
> i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
> ```
> 2. When an incorrect command is written to this device, it enters
> standby mode and stays in standby mode until the next CS# falling edge.
> In standby mode, This device's SO pin should be High-Z.
> ```
If don't want to consider WRSR command when HPM activated is "incorrect
command" and enter into standby mode, then according to data sheet in WRSR
section
```
The WRSR instruction cannot be
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
Just find out how to use mutt to reply all in the thread.
repeat the previous comments. Add STATE_HIZ to handle decode_new_command
aborting gracefully.
On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
>
>
> > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
> > wrote:
> >
> > Hi Iris,
> >
> > Looks good some, a couple of comments below.
> >
> > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> >> From: Iris Chen
> >>
> >> Signed-off-by: Iris Chen
> >> ---
> >> Addressed all comments from V1. The biggest change: removed
> >> object_class_property_add.
> >>
> >> hw/block/m25p80.c | 37 +++
> >> tests/qtest/aspeed_smc-test.c | 2 ++
> >> 2 files changed, 39 insertions(+)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
> >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >> break;
> >> case WRSR:
> >> +/*
> >> + * 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) {
> >
> > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > command, otherwise the state machinery will not advance to this function
> > (meaning that above check for !s->write_enable will never hit as far as I
> > can
> > tell). A suggestion is to move the check for wp_level and
> > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
>
> Oh good catch! Yes actually, in our fork, we also removed the write_enable
> guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> or both checks in complete_collecting_data.
>
> I think we had some difficulty deciding whether to block command decoding,
> or to decode and ignore the command if restrictions are enabled.
>
> The reason being that, in the qtest, the WRSR command code gets ignored, and
> then the subsequent write data gets interpreted as some random command code.
> We had elected to decode and ignore the command, but I think the
> datasheet actually describes that the command won’t be decoded successfully,
> so you’re probably right, we should put this logic in decode_new_cmd.
>
> Most likely, the qtest will also need to be modified to reset the transfer
> state machine after a blocked write command. I can’t remember if
> exiting and re-entering user mode is sufficient for that, but something
> like that is probably possible.
>
> Thanks for catching this!
> Peter
>
I am proposing add a CMDState: STATE_HIZ to handle command decode fail
situation. When decode_new_command need abort the decoding and ignore
following
on input bytes of this transaction, set the state to STATE_HIZ.
And m25p80_transfer8() will ignore all the following on byte when in
this state.
This is to simulating the real device operation behavior
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
```
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
```
BRs
Dan Zhang
> >
> >> +qemu_log_mask(LOG_GUEST_ERROR,
> >> + "M25P80: Status register write is disabled!\n");
> >> +break;
> >> +}
> >> +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);
> >> @@ -1195,6 +1214,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> >>
> >> case
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
On Thu, Jun 09, 2022 at 08:06:00PM +, Peter Delevoryas wrote:
>
>
> > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
> > wrote:
> >
> > Hi Iris,
> >
> > Looks good some, a couple of comments below.
> >
> > On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> >> From: Iris Chen
> >>
> >> Signed-off-by: Iris Chen
> >> ---
> >> Addressed all comments from V1. The biggest change: removed
> >> object_class_property_add.
> >>
> >> hw/block/m25p80.c | 37 +++
> >> tests/qtest/aspeed_smc-test.c | 2 ++
> >> 2 files changed, 39 insertions(+)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
> >> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >> break;
> >> case WRSR:
> >> +/*
> >> + * 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) {
> >
> > 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> > command, otherwise the state machinery will not advance to this function
> > (meaning that above check for !s->write_enable will never hit as far as I
> > can
> > tell). A suggestion is to move the check for wp_level and
> > status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
>
> Oh good catch! Yes actually, in our fork, we also removed the write_enable
> guard in decode_new_cmd. We either need both checks in decode_new_cmd,
> or both checks in complete_collecting_data.
>
> I think we had some difficulty deciding whether to block command decoding,
> or to decode and ignore the command if restrictions are enabled.
>
> The reason being that, in the qtest, the WRSR command code gets ignored, and
> then the subsequent write data gets interpreted as some random command code.
> We had elected to decode and ignore the command, but I think the
> datasheet actually describes that the command won’t be decoded successfully,
> so you’re probably right, we should put this logic in decode_new_cmd.
>
> Most likely, the qtest will also need to be modified to reset the transfer
> state machine after a blocked write command. I can’t remember if
> exiting and re-entering user mode is sufficient for that, but something
> like that is probably possible.
>
> Thanks for catching this!
> Peter
>
I am proposing add a CMDState: STATE_HIZ to handle command decode fail
situation. When decode_new_command need abort the decoding and ignore following
on input bytes of this transaction, set the state to STATE_HIZ.
And m25p80_transfer8() will ignore all the following on byte when in this state.
This is to simulating the real device operation behavior
i.e. Macronix MX66L1G45G data sheet section 8 DEVICE OPERATION described
`
2. When an incorrect command is written to this device, it enters
standby mode and stays in standby mode until the next CS# falling edge.
In standby mode, This device's SO pin should be High-Z.
`
> >
> >> +qemu_log_mask(LOG_GUEST_ERROR,
> >> + "M25P80: Status register write is disabled!\n");
> >> +break;
> >> +}
> >> +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);
> >> @@ -1195,6 +1214,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 ||
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
Hi Iris,
Looks good some, a couple of comments below.
On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
> From: Iris Chen
>
> Signed-off-by: Iris Chen
> ---
> Addressed all comments from V1. The biggest change: removed
> object_class_property_add.
>
> hw/block/m25p80.c | 37 +++
> tests/qtest/aspeed_smc-test.c | 2 ++
> 2 files changed, 39 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
> flash_erase(s, s->cur_addr, s->cmd_in_progress);
> break;
> case WRSR:
> +/*
> + * 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) {
'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
command, otherwise the state machinery will not advance to this function
(meaning that above check for !s->write_enable will never hit as far as I can
tell). A suggestion is to move the check for wp_level and
status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "M25P80: Status register write is disabled!\n");
> +break;
> +}
> +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);
> @@ -1195,6 +1214,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 +1505,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 +1544,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);
> }
>
> @@ -1601,6 +1636,8 @@ static const VMStateDescription vmstate_m25p80 = {
> VMSTATE_UINT8(needed_bytes, Flash),
> VMSTATE_UINT8(cmd_in_progress, Flash),
> VMSTATE_UINT32(cur_addr, Flash),
> +VMSTATE_BOOL(wp_level, Flash),
> +VMSTATE_BOOL(status_register_write_disabled, Flash),
Above needs to be added through a subsection, you can see commit 465ef47abe3
for an example an also read about this in docs/devel/migration.rst.
Thank you,
Best regads,
Francisco Iglesias
> VMSTATE_BOOL(write_enable, Flash),
> VMSTATE_BOOL(reset_enable, Flash),
> VMSTATE_UINT8(ear, Flash),
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index ec233315e6..c5d97d4410 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -56,7 +56,9 @@ enum {
> BULK_ERASE = 0xc7,
> READ = 0x03,
> PP = 0x02,
> +WRSR = 0x1,
>
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
> On Jun 9, 2022, at 12:22 PM, Francisco Iglesias
> wrote:
>
> Hi Iris,
>
> Looks good some, a couple of comments below.
>
> On [2022 Jun 08] Wed 20:13:19, Iris Chen wrote:
>> From: Iris Chen
>>
>> Signed-off-by: Iris Chen
>> ---
>> Addressed all comments from V1. The biggest change: removed
>> object_class_property_add.
>>
>> hw/block/m25p80.c | 37 +++
>> tests/qtest/aspeed_smc-test.c | 2 ++
>> 2 files changed, 39 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
>> flash_erase(s, s->cur_addr, s->cmd_in_progress);
>> break;
>> case WRSR:
>> +/*
>> + * 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) {
>
> 'write_enable' needs to be true in 'decode_new_cmd' when issueing the WRSR
> command, otherwise the state machinery will not advance to this function
> (meaning that above check for !s->write_enable will never hit as far as I can
> tell). A suggestion is to move the check for wp_level and
> status_reg_wr_disabled into 'decode_new_cmd' to for keeping it consistent.
Oh good catch! Yes actually, in our fork, we also removed the write_enable
guard in decode_new_cmd. We either need both checks in decode_new_cmd,
or both checks in complete_collecting_data.
I think we had some difficulty deciding whether to block command decoding,
or to decode and ignore the command if restrictions are enabled.
The reason being that, in the qtest, the WRSR command code gets ignored, and
then the subsequent write data gets interpreted as some random command code.
We had elected to decode and ignore the command, but I think the
datasheet actually describes that the command won’t be decoded successfully,
so you’re probably right, we should put this logic in decode_new_cmd.
Most likely, the qtest will also need to be modified to reset the transfer
state machine after a blocked write command. I can’t remember if
exiting and re-entering user mode is sufficient for that, but something
like that is probably possible.
Thanks for catching this!
Peter
>
>> +qemu_log_mask(LOG_GUEST_ERROR,
>> + "M25P80: Status register write is disabled!\n");
>> +break;
>> +}
>> +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);
>> @@ -1195,6 +1214,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 +1505,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 +1544,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
Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
On 6/9/22 05:13, Iris Chen wrote:
From: Iris Chen
Signed-off-by: Iris Chen
---
Addressed all comments from V1. The biggest change: removed
object_class_property_add.
Reviewed-by: Cédric Le Goater
Thanks,
C.
hw/block/m25p80.c | 37 +++
tests/qtest/aspeed_smc-test.c | 2 ++
2 files changed, 39 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
flash_erase(s, s->cur_addr, s->cmd_in_progress);
break;
case WRSR:
+/*
+ * 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;
+}
+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);
@@ -1195,6 +1214,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 +1505,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 +1544,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);
}
@@ -1601,6 +1636,8 @@ static const VMStateDescription vmstate_m25p80 = {
VMSTATE_UINT8(needed_bytes, Flash),
VMSTATE_UINT8(cmd_in_progress, Flash),
VMSTATE_UINT32(cur_addr, Flash),
+VMSTATE_BOOL(wp_level, Flash),
+VMSTATE_BOOL(status_register_write_disabled, Flash),
VMSTATE_BOOL(write_enable, Flash),
VMSTATE_BOOL(reset_enable, Flash),
VMSTATE_UINT8(ear, Flash),
diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index ec233315e6..c5d97d4410 100644
--- a/tests/qtest/aspeed_smc-test.c
+++ b/tests/qtest/aspeed_smc-test.c
@@ -56,7 +56,9 @@ enum {
BULK_ERASE = 0xc7,
READ = 0x03,
PP = 0x02,
+WRSR = 0x1,
WREN = 0x6,
+SRWD = 0x80,
RESET_ENABLE = 0x66,
RESET_MEMORY = 0x99,
EN_4BYTE_ADDR = 0xB7,
[PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection
From: Iris Chen
Signed-off-by: Iris Chen
---
Addressed all comments from V1. The biggest change: removed
object_class_property_add.
hw/block/m25p80.c | 37 +++
tests/qtest/aspeed_smc-test.c | 2 ++
2 files changed, 39 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 81ba3da4df..1a20bd55d4 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,21 @@ static void complete_collecting_data(Flash *s)
flash_erase(s, s->cur_addr, s->cmd_in_progress);
break;
case WRSR:
+/*
+ * 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;
+}
+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);
@@ -1195,6 +1214,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 +1505,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 +1544,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);
}
@@ -1601,6 +1636,8 @@ static const VMStateDescription vmstate_m25p80 = {
VMSTATE_UINT8(needed_bytes, Flash),
VMSTATE_UINT8(cmd_in_progress, Flash),
VMSTATE_UINT32(cur_addr, Flash),
+VMSTATE_BOOL(wp_level, Flash),
+VMSTATE_BOOL(status_register_write_disabled, Flash),
VMSTATE_BOOL(write_enable, Flash),
VMSTATE_BOOL(reset_enable, Flash),
VMSTATE_UINT8(ear, Flash),
diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index ec233315e6..c5d97d4410 100644
--- a/tests/qtest/aspeed_smc-test.c
+++ b/tests/qtest/aspeed_smc-test.c
@@ -56,7 +56,9 @@ enum {
BULK_ERASE = 0xc7,
READ = 0x03,
PP = 0x02,
+WRSR = 0x1,
WREN = 0x6,
+SRWD = 0x80,
RESET_ENABLE = 0x66,
RESET_MEMORY = 0x99,
EN_4BYTE_ADDR = 0xB7,
--
2.30.2
