diff mbox series

[1/1] hw: m25p80: add W# pin and SRWD bit for write protection

Message ID 20220526021216.1031533-1-irischenlj@fb.com (mailing list archive)
State New, archived
Headers show
Series [1/1] hw: m25p80: add W# pin and SRWD bit for write protection | expand

Commit Message

Iris Chen May 26, 2022, 2:12 a.m. UTC 
From: Iris Chen <irischenlj@gmail.com>

Add the W# pin and SRWD bit which control the status register write
ability.

Signed-off-by: Iris Chen <irischenlj@gmail.com>
---
 hw/block/m25p80.c             | 72 +++++++++++++++++++++++++++++++++++
 tests/qtest/aspeed_smc-test.c | 62 ++++++++++++++++++++++++++++++
 2 files changed, 134 insertions(+)

Comments

Cédric Le Goater June 3, 2022, 8:49 a.m. UTC | #1 
Hello Iris,

On 5/26/22 04:12, Iris Chen wrote:
> From: Iris Chen <irischenlj@gmail.com>
> 
> Add the W# pin and SRWD bit which control the status register write
> ability.

may be replace W# by WP# (for write protect)

> 
> Signed-off-by: Iris Chen <irischenlj@gmail.com>
> ---
>   hw/block/m25p80.c             | 72 +++++++++++++++++++++++++++++++++++
>   tests/qtest/aspeed_smc-test.c | 62 ++++++++++++++++++++++++++++++
>   2 files changed, 134 insertions(+)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 81ba3da4df..c845fa08d4 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 write_protect_pin;

I would call this attribute 'wp_level' since it is an assertion level
of the WP# pin.

>       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 W# 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->write_protect_pin == 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,15 @@ 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);
> +    bool wp = !!level;

this extra variable looks superfluous. minor

> +    /* W# is just a single pin. */
> +    assert(n == 0);
> +    s->write_protect_pin = wp;
> +}
> +
>   static void m25p80_realize(SSIPeripheral *ss, Error **errp)
>   {
>       Flash *s = M25P80(ss);
> @@ -1515,12 +1545,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, "W#", 1);
>   }
>   
>   static void m25p80_reset(DeviceState *d)
>   {
>       Flash *s = M25P80(d);
>   
> +    s->write_protect_pin = true;
> +    s->status_register_write_disabled = false;

Are we ok with these defaults ? We wouldn't want to break existing
implementation.


>       reset_memory(s);
>   }
>   
> @@ -1601,6 +1637,7 @@ static const VMStateDescription vmstate_m25p80 = {
>           VMSTATE_UINT8(needed_bytes, Flash),
>           VMSTATE_UINT8(cmd_in_progress, Flash),
>           VMSTATE_UINT32(cur_addr, Flash),
> +        VMSTATE_BOOL(write_protect_pin, Flash),

and what about status_register_write_disabled ?

>           VMSTATE_BOOL(write_enable, Flash),
>           VMSTATE_BOOL(reset_enable, Flash),
>           VMSTATE_UINT8(ear, Flash),
> @@ -1622,6 +1659,38 @@ static const VMStateDescription vmstate_m25p80 = {
>       }
>   };
>   
> +static void m25p80_get_write_protect_pin(Object *obj,
> +                                       Visitor *v,
> +                                       const char *name,
> +                                       void *opaque,
> +                                       Error **errp)
> +{
> +    Flash *s = M25P80(obj);
> +    bool value;
> +
> +    value = s->write_protect_pin;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void m25p80_set_write_protect_pin(Object *obj,
> +                                       Visitor *v,
> +                                       const char *name,
> +                                       void *opaque,
> +                                       Error **errp)
> +{
> +    Flash *s = M25P80(obj);
> +    bool value;
> +    qemu_irq w;
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    w = qdev_get_gpio_in_named(DEVICE(s), "W#", 0);
> +    qemu_set_irq(w, value);
> +}
> +
>   static void m25p80_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1636,6 +1705,9 @@ static void m25p80_class_init(ObjectClass *klass, void *data)
>       device_class_set_props(dc, m25p80_properties);
>       dc->reset = m25p80_reset;
>       mc->pi = data;
> +
> +    object_class_property_add(klass, "W#", "bool", m25p80_get_write_protect_pin,
> +                              m25p80_set_write_protect_pin, NULL, NULL);


I think you can simply use qtest_set_irq_in() from the test to raise the
interrupt and avoid adding a property to do the same.


>   }
>   
>   static const TypeInfo m25p80_info = {
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c


I would include the test in a second patch.

Thanks,

C.

> index ec233315e6..45c0d812c7 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,
> @@ -390,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: W# 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);
> +
> +    /* W# 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);
> +
> +    /* W# low and SRWD low -> status register writable */
> +    qtest_qom_set_bool(global_qtest,
> +                       "/machine/soc/fmc/ssi.0/child[0]", "W#", false);
> +    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);
> +
> +    /* W# 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_qom_set_bool(global_qtest,
> +                       "/machine/soc/fmc/ssi.0/child[0]", "W#", true);
> +    flash_reset();
> +}
> +
>   static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
>   
>   int main(int argc, char **argv)
> @@ -416,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();
>
diff mbox series

Patch

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 81ba3da4df..c845fa08d4 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 write_protect_pin;
     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 W# 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->write_protect_pin == 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,15 @@  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);
+    bool wp = !!level;
+    /* W# is just a single pin. */
+    assert(n == 0);
+    s->write_protect_pin = wp;
+}
+
 static void m25p80_realize(SSIPeripheral *ss, Error **errp)
 {
     Flash *s = M25P80(ss);
@@ -1515,12 +1545,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, "W#", 1);
 }
 
 static void m25p80_reset(DeviceState *d)
 {
     Flash *s = M25P80(d);
 
+    s->write_protect_pin = true;
+    s->status_register_write_disabled = false;
+
     reset_memory(s);
 }
 
@@ -1601,6 +1637,7 @@  static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_UINT8(needed_bytes, Flash),
         VMSTATE_UINT8(cmd_in_progress, Flash),
         VMSTATE_UINT32(cur_addr, Flash),
+        VMSTATE_BOOL(write_protect_pin, Flash),
         VMSTATE_BOOL(write_enable, Flash),
         VMSTATE_BOOL(reset_enable, Flash),
         VMSTATE_UINT8(ear, Flash),
@@ -1622,6 +1659,38 @@  static const VMStateDescription vmstate_m25p80 = {
     }
 };
 
+static void m25p80_get_write_protect_pin(Object *obj,
+                                       Visitor *v,
+                                       const char *name,
+                                       void *opaque,
+                                       Error **errp)
+{
+    Flash *s = M25P80(obj);
+    bool value;
+
+    value = s->write_protect_pin;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void m25p80_set_write_protect_pin(Object *obj,
+                                       Visitor *v,
+                                       const char *name,
+                                       void *opaque,
+                                       Error **errp)
+{
+    Flash *s = M25P80(obj);
+    bool value;
+    qemu_irq w;
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    w = qdev_get_gpio_in_named(DEVICE(s), "W#", 0);
+    qemu_set_irq(w, value);
+}
+
 static void m25p80_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1636,6 +1705,9 @@  static void m25p80_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, m25p80_properties);
     dc->reset = m25p80_reset;
     mc->pi = data;
+
+    object_class_property_add(klass, "W#", "bool", m25p80_get_write_protect_pin,
+                              m25p80_set_write_protect_pin, NULL, NULL);
 }
 
 static const TypeInfo m25p80_info = {
diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
index ec233315e6..45c0d812c7 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,
@@ -390,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: W# 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);
+
+    /* W# 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);
+
+    /* W# low and SRWD low -> status register writable */
+    qtest_qom_set_bool(global_qtest,
+                       "/machine/soc/fmc/ssi.0/child[0]", "W#", false);
+    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);
+
+    /* W# 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_qom_set_bool(global_qtest,
+                       "/machine/soc/fmc/ssi.0/child[0]", "W#", true);
+    flash_reset();
+}
+
 static char tmp_path[] = "/tmp/qtest.m25p80.XXXXXX";
 
 int main(int argc, char **argv)
@@ -416,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();