* [PATCH 1/1] hw: m25p80: add W# pin and SRWD bit for write protection
@ 2022-05-26 2:12 Iris Chen
2022-06-03 8:49 ` Cédric Le Goater
0 siblings, 1 reply; 2+ messages in thread
From: Iris Chen @ 2022-05-26 2:12 UTC (permalink / raw)
Cc: irischenlj, pdel, qemu-devel, qemu-arm, clg, patrick, alistair,
kwolf, hreitz, peter.maydell, andrew, joel, thuth, lvivier,
pbonzini, qemu-block, Iris Chen
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(+)
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();
--
2.30.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH 1/1] hw: m25p80: add W# pin and SRWD bit for write protection
2022-05-26 2:12 [PATCH 1/1] hw: m25p80: add W# pin and SRWD bit for write protection Iris Chen
@ 2022-06-03 8:49 ` Cédric Le Goater
0 siblings, 0 replies; 2+ messages in thread
From: Cédric Le Goater @ 2022-06-03 8:49 UTC (permalink / raw)
To: Iris Chen
Cc: pdel, qemu-devel, qemu-arm, patrick, alistair, kwolf, hreitz,
peter.maydell, andrew, joel, thuth, lvivier, pbonzini, qemu-block,
Iris Chen
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();
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-06-03 8:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-26 2:12 [PATCH 1/1] hw: m25p80: add W# pin and SRWD bit for write protection Iris Chen
2022-06-03 8:49 ` Cédric Le Goater
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).