Re: [PATCH v2 1/2] hw: m25p80: add WP# pin and SRWD bit for write protection

[Dan Zhang]

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 +0000, Peter Delevoryas wrote:
> > 
> > 
> > > On Jun 9, 2022, at 12:22 PM, Francisco Iglesias 
> > > <frasse.iglesias@gmail.com> 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 <irischenlj@gmail.com>
> > >> 
> > >> Signed-off-by: Iris Chen <irischenlj@gmail.com>
> > >> ---
> > >> 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 executed once the Hardware Protected Mode (HPM) 
is entered.
```
the best place to check HPM is before the command execution in function
complete_collecting_data(). This can help avoiding decode the WRSR input
data as new command.

BTW, maybe STATE_STANDBY is better than STATE_HIZ, much easy to
understand.
> 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)
> > >>     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,
> > >>     WREN = 0x6,
> > >> +    SRWD = 0x80,
> > >>     RESET_ENABLE = 0x66,
> > >>     RESET_MEMORY = 0x99,
> > >>     EN_4BYTE_ADDR = 0xB7,
> > >> -- 
> > >> 2.30.2
> > >> 
> > >> 
> >