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

[Peter Delevoryas]

> 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

> 
>> +            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
>> 
>>