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

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

[Iris Chen]

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

[PATCH v2 2/2] hw: m25p80: add tests for write protect

[Iris Chen]

Signed-off-by: Iris Chen <irischenlj@fb.com>
---
Include the tests in a separate patch. Using qtest_set_irq_in() as per review.

 tests/qtest/aspeed_smc-test.c | 60 +++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index c5d97d4410..7786addfb8 100644
--- a/tests/qtest/aspeed_smc-test.c
+++ b/tests/qtest/aspeed_smc-test.c
@@ -392,6 +392,64 @@ static void test_read_status_reg(void)
     flash_reset();
 }
 
+static void test_status_reg_write_protection(void)
+{
+    uint8_t r;
+
+    spi_conf(CONF_ENABLE_W0);
+
+    /* default case: WP# is high and SRWD is low -> status register writable */
+    spi_ctrl_start_user();
+    writeb(ASPEED_FLASH_BASE, WREN);
+    /* test ability to write SRWD */
+    writeb(ASPEED_FLASH_BASE, WRSR);
+    writeb(ASPEED_FLASH_BASE, SRWD);
+    writeb(ASPEED_FLASH_BASE, RDSR);
+    r = readb(ASPEED_FLASH_BASE);
+    spi_ctrl_stop_user();
+    g_assert_cmphex(r & SRWD, ==, SRWD);
+
+    /* WP# high and SRWD high -> status register writable */
+    spi_ctrl_start_user();
+    writeb(ASPEED_FLASH_BASE, WREN);
+    /* test ability to write SRWD */
+    writeb(ASPEED_FLASH_BASE, WRSR);
+    writeb(ASPEED_FLASH_BASE, 0);
+    writeb(ASPEED_FLASH_BASE, RDSR);
+    r = readb(ASPEED_FLASH_BASE);
+    spi_ctrl_stop_user();
+    g_assert_cmphex(r & SRWD, ==, 0);
+
+    /* WP# low and SRWD low -> status register writable */
+    qtest_set_irq_in(global_qtest,
+                     "/machine/soc/fmc/ssi.0/child[0]", "WP#", 0, 0);
+    spi_ctrl_start_user();
+    writeb(ASPEED_FLASH_BASE, WREN);
+    /* test ability to write SRWD */
+    writeb(ASPEED_FLASH_BASE, WRSR);
+    writeb(ASPEED_FLASH_BASE, SRWD);
+    writeb(ASPEED_FLASH_BASE, RDSR);
+    r = readb(ASPEED_FLASH_BASE);
+    spi_ctrl_stop_user();
+    g_assert_cmphex(r & SRWD, ==, SRWD);
+
+    /* WP# low and SRWD high -> status register NOT writable */
+    spi_ctrl_start_user();
+    writeb(ASPEED_FLASH_BASE, WREN);
+    /* test ability to write SRWD */
+    writeb(ASPEED_FLASH_BASE, WRSR);
+    writeb(ASPEED_FLASH_BASE, 0);
+    writeb(ASPEED_FLASH_BASE, RDSR);
+    r = readb(ASPEED_FLASH_BASE);
+    spi_ctrl_stop_user();
+    /* write is not successful */
+    g_assert_cmphex(r & SRWD, ==, SRWD);
+
+    qtest_set_irq_in(global_qtest,
+                     "/machine/soc/fmc/ssi.0/child[0]", "WP#", 0, 1);
+    flash_reset();
+}
+
 static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
 
 int main(int argc, char **argv)
@@ -418,6 +476,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ast2400/smc/read_page_mem", test_read_page_mem);
     qtest_add_func("/ast2400/smc/write_page_mem", test_write_page_mem);
     qtest_add_func("/ast2400/smc/read_status_reg", test_read_status_reg);
+    qtest_add_func("/ast2400/smc/status_reg_write_protection",
+                   test_status_reg_write_protection);
 
     ret = g_test_run();
 
-- 
2.30.2

Re: [PATCH v2 2/2] hw: m25p80: add tests for write protect

[Thomas Huth]

On 09/06/2022 05.13, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
> Include the tests in a separate patch. Using qtest_set_irq_in() as per review.
> 
>   tests/qtest/aspeed_smc-test.c | 60 +++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
> 
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index c5d97d4410..7786addfb8 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -392,6 +392,64 @@ static void test_read_status_reg(void)
>       flash_reset();
>   }
>   
> +static void test_status_reg_write_protection(void)
> +{
> +    uint8_t r;
> +
> +    spi_conf(CONF_ENABLE_W0);
> +
> +    /* default case: WP# is high and SRWD is low -> status register writable */
> +    spi_ctrl_start_user();
> +    writeb(ASPEED_FLASH_BASE, WREN);
> +    /* test ability to write SRWD */
> +    writeb(ASPEED_FLASH_BASE, WRSR);
> +    writeb(ASPEED_FLASH_BASE, SRWD);
> +    writeb(ASPEED_FLASH_BASE, RDSR);
> +    r = readb(ASPEED_FLASH_BASE);
> +    spi_ctrl_stop_user();
> +    g_assert_cmphex(r & SRWD, ==, SRWD);
> +
> +    /* WP# high and SRWD high -> status register writable */
> +    spi_ctrl_start_user();
> +    writeb(ASPEED_FLASH_BASE, WREN);
> +    /* test ability to write SRWD */
> +    writeb(ASPEED_FLASH_BASE, WRSR);
> +    writeb(ASPEED_FLASH_BASE, 0);
> +    writeb(ASPEED_FLASH_BASE, RDSR);
> +    r = readb(ASPEED_FLASH_BASE);
> +    spi_ctrl_stop_user();
> +    g_assert_cmphex(r & SRWD, ==, 0);
> +
> +    /* WP# low and SRWD low -> status register writable */
> +    qtest_set_irq_in(global_qtest,
> +                     "/machine/soc/fmc/ssi.0/child[0]", "WP#", 0, 0);
> +    spi_ctrl_start_user();
> +    writeb(ASPEED_FLASH_BASE, WREN);
> +    /* test ability to write SRWD */
> +    writeb(ASPEED_FLASH_BASE, WRSR);
> +    writeb(ASPEED_FLASH_BASE, SRWD);
> +    writeb(ASPEED_FLASH_BASE, RDSR);
> +    r = readb(ASPEED_FLASH_BASE);
> +    spi_ctrl_stop_user();
> +    g_assert_cmphex(r & SRWD, ==, SRWD);
> +
> +    /* WP# low and SRWD high -> status register NOT writable */
> +    spi_ctrl_start_user();
> +    writeb(ASPEED_FLASH_BASE, WREN);
> +    /* test ability to write SRWD */
> +    writeb(ASPEED_FLASH_BASE, WRSR);
> +    writeb(ASPEED_FLASH_BASE, 0);
> +    writeb(ASPEED_FLASH_BASE, RDSR);
> +    r = readb(ASPEED_FLASH_BASE);
> +    spi_ctrl_stop_user();
> +    /* write is not successful */
> +    g_assert_cmphex(r & SRWD, ==, SRWD);
> +
> +    qtest_set_irq_in(global_qtest,
> +                     "/machine/soc/fmc/ssi.0/child[0]", "WP#", 0, 1);
> +    flash_reset();
> +}

FWIW, I'd prefer if we could use qtest_writeb / qtest_readb for new code 
instead of writeb / readb, but well, the whole file is already written that 
way, so this is only "consistent" ... so:

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Cédric Le Goater]

On 6/9/22 05:13, 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.


Reviewed-by: Cédric Le Goater <clg@kaod.org>

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,

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

[Francisco Iglesias]

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.

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

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

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

[Dan Zhang]

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

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

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

[Cédric Le Goater]

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

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

[PATCH] hw:w25p80: Add STATE_STANDBY to handle incorrect command

[Dan Zhang]

---
 hw/block/m25p80.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index b6bd430a99..3bb0466dca 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -423,6 +423,7 @@ typedef enum {
     STATE_COLLECTING_DATA,
     STATE_COLLECTING_VAR_LEN_DATA,
     STATE_READING_DATA,
+    STATE_STANDBY,
 } CMDState;
 
 typedef enum {
@@ -1218,6 +1219,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
             || !s->write_enable) {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "M25P80: Status register write is disabled!\n");
+	    qemu_log_mask(LOG_GUEST_ERROR,
+                          "M25P80: switch to standby, re-aseert CS to reactivate \n");
+	    s->state = STATE_STANDBY;
             break;
         }
 
@@ -1472,6 +1476,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx)
                           s->cur_addr, (uint8_t)tx);
 
     switch (s->state) {
+    case STATE_STANDBY:
+	r = 0xFFFFFFFF; /* StandBy state SO shall be HiZ */
+	break;
 
     case STATE_PAGE_PROGRAM:
         trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
-- 
2.34.3

Re: [PATCH] hw:w25p80: Add STATE_STANDBY to handle incorrect command

[Dan Zhang]

Hi Cedric,

I am sorry that accidently submit a pre-view code change as a patch using the
git-sendmail. 
I originally mean to copy the following code in email reply and let
commnity get better understand my proposal.

Let me submit a formal patch in seperate thread. And will remove the
code using this STATE_STANDBY state, as those code shall be in @iris WP#
patch.

BRs
Dan

On Tue, Jun 14, 2022 at 09:02:46AM -0700, Dan Zhang wrote:
> ---
>  hw/block/m25p80.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index b6bd430a99..3bb0466dca 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -423,6 +423,7 @@ typedef enum {
>      STATE_COLLECTING_DATA,
>      STATE_COLLECTING_VAR_LEN_DATA,
>      STATE_READING_DATA,
> +    STATE_STANDBY,
>  } CMDState;
>  
>  typedef enum {
> @@ -1218,6 +1219,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>              || !s->write_enable) {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "M25P80: Status register write is disabled!\n");
> +	    qemu_log_mask(LOG_GUEST_ERROR,
> +                          "M25P80: switch to standby, re-aseert CS to reactivate \n");
> +	    s->state = STATE_STANDBY;
>              break;
>          }
>  
> @@ -1472,6 +1476,9 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx)
>                            s->cur_addr, (uint8_t)tx);
>  
>      switch (s->state) {
> +    case STATE_STANDBY:
> +	r = 0xFFFFFFFF; /* StandBy state SO shall be HiZ */
> +	break;
>  
>      case STATE_PAGE_PROGRAM:
>          trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
> -- 
> 2.34.3
> 

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

[Dan Zhang]

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