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

[Francisco Iglesias <frasse.iglesias@gmail.com>]

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