aspeed/smc: Fix write incorrect data into flash in user mode

According to the design of ASPEED SPI controllers user mode, users write the
data to flash, the SPI drivers set the Control Register(0x10) bit 0 and 1
enter user mode. Then, SPI drivers send flash commands for writing data.
Finally, SPI drivers set the Control Register (0x10) bit 2 to stop
active control and restore bit 0 and 1.

According to the design of ASPEED SMC model, firmware writes the
Control Register and the "aspeed_smc_flash_update_ctrl" function is called.
Then, this function verify Control Register(0x10) bit 0 and 1. If it set user
mode, the value of s->snoop_index is SNOOP_START else SNOOP_OFF.
If s->snoop_index is SNOOP_START, the "aspeed_smc_do_snoop" function verify
the first incomming data is a new flash command and writes the corresponding
dummy bytes if need.

However, it did not check the current unselect status. If current unselect
status is "false" and firmware set the IO MODE by Control Register bit 31:28,
the value of s->snoop_index will be changed to SNOOP_START again and
"aspeed_smc_do_snoop" misunderstand that the incomming data is the new flash
command and it causes writing unexpected data into flash.

Example:
1. Firmware set user mode by Control Register bit 0 and 1(0x03)
2. SMC model set s->snoop SNOOP_START
3. Firmware set Quad Page Program with 4-Byte Address command (0x34)
4. SMC model verify this flash command and it needs 4 dummy bytes.
5. Firmware send 4 bytes address.
6. SMC model receives 4 bytes address
7. Firmware set QPI IO MODE by Control Register bit 31. (0x80000003)
8. SMC model verify new user mode by Control Register bit 0 and 1.
   Then, set s->snoop SNOOP_START again. (It is the wrong behavior.)
9. Firmware send 0xebd8c134 data and it should be written into flash.
   However, SMC model misunderstand that the first incoming data, 0x34,
   is the new command because the value of s->snoop is changed to SNOOP_START.
   Finally, SMC sned the incorrect data to flash model.

Introduce a new unselect attribute in AspeedSMCState to save the current
unselect status for user mode and set it "true" by default.
Update "aspeed_smc_flash_update_ctrl" function to check the previous unselect
status. If both new unselect status and previous unselect status is different,
update s->snoop_index value and call "aspeed_smc_flash_do_select".

Increase VMStateDescription version.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
[ clg: - Replaced VMSTATE_BOOL -> VMSTATE_BOOL_V ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index e3fdc66cb2b700769017f6e67f6db228e043a25c..033cbbb59b06b95a918a1aaa958a522fc61f2a68 100644 (file)
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -417,7 +417,7 @@ static void aspeed_smc_flash_do_select(AspeedSMCFlash *fl, bool unselect)
     AspeedSMCState *s = fl->controller;
 
     trace_aspeed_smc_flash_select(fl->cs, unselect ? "un" : "");
-
+    s->unselect = unselect;
     qemu_set_irq(s->cs_lines[fl->cs], unselect);
 }
 
@@ -677,22 +677,35 @@ static const MemoryRegionOps aspeed_smc_flash_ops = {
 static void aspeed_smc_flash_update_ctrl(AspeedSMCFlash *fl, uint32_t value)
 {
     AspeedSMCState *s = fl->controller;
-    bool unselect;
+    bool unselect = false;
+    uint32_t old_mode;
+    uint32_t new_mode;
+
+    old_mode = s->regs[s->r_ctrl0 + fl->cs] & CTRL_CMD_MODE_MASK;
+    new_mode = value & CTRL_CMD_MODE_MASK;
 
-    /* User mode selects the CS, other modes unselect */
-    unselect = (value & CTRL_CMD_MODE_MASK) != CTRL_USERMODE;
+    if (old_mode == CTRL_USERMODE) {
+        if (new_mode != CTRL_USERMODE) {
+            unselect = true;
+        }
 
-    /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */
-    if (!(s->regs[s->r_ctrl0 + fl->cs] & CTRL_CE_STOP_ACTIVE) &&
-        value & CTRL_CE_STOP_ACTIVE) {
-        unselect = true;
+        /* A change of CTRL_CE_STOP_ACTIVE from 0 to 1, unselects the CS */
+        if (!(s->regs[s->r_ctrl0 + fl->cs] & CTRL_CE_STOP_ACTIVE) &&
+            value & CTRL_CE_STOP_ACTIVE) {
+            unselect = true;
+        }
+    } else {
+        if (new_mode != CTRL_USERMODE) {
+            unselect = true;
+        }
     }
 
     s->regs[s->r_ctrl0 + fl->cs] = value;
 
-    s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START;
-
-    aspeed_smc_flash_do_select(fl, unselect);
+    if (unselect != s->unselect) {
+        s->snoop_index = unselect ? SNOOP_OFF : SNOOP_START;
+        aspeed_smc_flash_do_select(fl, unselect);
+    }
 }
 
 static void aspeed_smc_reset(DeviceState *d)
@@ -729,6 +742,8 @@ static void aspeed_smc_reset(DeviceState *d)
         qemu_set_irq(s->cs_lines[i], true);
     }
 
+    s->unselect = true;
+
     /* setup the default segment register values and regions for all */
     for (i = 0; i < asc->cs_num_max; ++i) {
         aspeed_smc_flash_set_segment_region(s, i,
@@ -1261,12 +1276,13 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_aspeed_smc = {
     .name = "aspeed.smc",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 2,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX),
         VMSTATE_UINT8(snoop_index, AspeedSMCState),
         VMSTATE_UINT8(snoop_dummies, AspeedSMCState),
+        VMSTATE_BOOL_V(unselect, AspeedSMCState, 3),
         VMSTATE_END_OF_LIST()
     }
 };

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 234dca32b0172799d362f3abfdd99e4d6c9f376b..25b95e740608d99fdf9f71599b0835c7f9ff01a7 100644 (file)
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -82,6 +82,7 @@ struct AspeedSMCState {
 
     uint8_t snoop_index;
     uint8_t snoop_dummies;
+    bool unselect;
 };
 
 typedef struct AspeedSegments {