[Qemu-devel] [PATCH v3 0/9] hw/block/pflash_cfi01: Add DeviceReset() handler

[Qemu-devel] [PATCH v3 0/9] hw/block/pflash_cfi01: Add DeviceReset() handler

[Philippe Mathieu-Daudé]

The pflash device lacks a reset() function.
When a machine is resetted, the flash might be in an
inconsistent state, leading to unexpected behavior:
https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Resolve this issue by adding a DeviceReset() handler.

Fix also two minor issues, and clean a bit the codebase.

Since v1: https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg00962.html
- addressed Laszlo review comments

Since v2:
- consider migration (Laszlo, Peter)

$ git backport-diff -u v2
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/9:[----] [--] 'hw/block/pflash_cfi01: Removed an unused timer'
002/9:[0008] [FC] 'hw/block/pflash_cfi01: Use the correct READ_ARRAY value'
003/9:[----] [-C] 'hw/block/pflash_cfi01: Extract pflash_mode_read_array()'
004/9:[----] [--] 'hw/block/pflash_cfi01: Start state machine as READY to accept commands'
005/9:[----] [--] 'hw/block/pflash_cfi01: Add the DeviceReset() handler'
006/9:[----] [--] 'hw/block/pflash_cfi01: Simplify CFI_QUERY processing'
007/9:[----] [--] 'hw/block/pflash_cfi01: Improve command comments'
008/9:[----] [--] 'hw/block/pflash_cfi01: Replace DPRINTF by qemu_log_mask(GUEST_ERROR)'
009/9:[----] [--] 'hw/block/pflash_cfi01: Hold the PRI table offset in a variable'

Functional differences on patch #2 are the 6 lines added for
migration of the 'cmd' field, and the updated commit description.

Regards,

Phil.

Philippe Mathieu-Daudé (9):
  hw/block/pflash_cfi01: Removed an unused timer
  hw/block/pflash_cfi01: Use the correct READ_ARRAY value
  hw/block/pflash_cfi01: Extract pflash_mode_read_array()
  hw/block/pflash_cfi01: Start state machine as READY to accept commands
  hw/block/pflash_cfi01: Add the DeviceReset() handler
  hw/block/pflash_cfi01: Simplify CFI_QUERY processing
  hw/block/pflash_cfi01: Improve command comments
  hw/block/pflash_cfi01: Replace DPRINTF by qemu_log_mask(GUEST_ERROR)
  hw/block/pflash_cfi01: Hold the PRI table offset in a variable

 hw/block/pflash_cfi01.c | 148 ++++++++++++++++++++++------------------
 hw/block/trace-events   |   1 +
 2 files changed, 81 insertions(+), 68 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/9] hw/block/pflash_cfi01: Removed an unused timer

[Philippe Mathieu-Daudé]

The 'CFI02' NOR flash was introduced in commit 29133e9a0fff, with
timing modelled. One year later, the CFI01 model was introduced
(commit 05ee37ebf630) based on the CFI02 model. As noted in the
header, "It does not support timings". 12 years later, we never
had to model the device timings. Time to remove the unused timer,
we can still add it back if required.

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Fixed commit description (Laszlo)
---
 hw/block/pflash_cfi01.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index db4a246b22..9e34fd4e82 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -42,7 +42,6 @@
 #include "hw/block/flash.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
-#include "qemu/timer.h"
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
 #include "qemu/host-utils.h"
@@ -90,7 +89,6 @@ struct PFlashCFI01 {
     uint8_t cfi_table[0x52];
     uint64_t counter;
     unsigned int writeblock_size;
-    QEMUTimer *timer;
     MemoryRegion mem;
     char *name;
     void *storage;
@@ -114,18 +112,6 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
-static void pflash_timer (void *opaque)
-{
-    PFlashCFI01 *pfl = opaque;
-
-    trace_pflash_timer_expired(pfl->cmd);
-    /* Reset flash */
-    pfl->status ^= 0x80;
-    memory_region_rom_device_set_romd(&pfl->mem, true);
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
-}
-
 /* Perform a CFI query based on the bank width of the flash.
  * If this code is called we know we have a device_width set for
  * this flash.
@@ -774,7 +760,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
     pfl->wcycle = 0;
     pfl->cmd = 0;
     pfl->status = 0;
-- 
2.20.1

[Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Philippe Mathieu-Daudé]

In the "Read Array Flowchart" the command has a value of 0xFF.

In the document [*] the "Read Array Flowchart", the READ_ARRAY
command has a value of 0xff.

Use the correct value in the pflash model.

There is no change of behavior in the guest, because:
- when the guest were sending 0xFF, the reset_flash label
  was setting the command value as 0x00
- 0x00 was used internally for READ_ARRAY

To keep migration behaving correctly, we have to increase
the VMState version. When migrating from an older version,
we use the correct command value.

[*] "Common Flash Interface (CFI) and Command Sets"
    (Intel Application Note 646)
    Appendix B "Basic Command Set"

Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v3: Handle migrating the 'cmd' field.

Since Laszlo stated he did not test migration [*], I'm keeping his
test tag, because the change with v2 has no impact in the tests
he ran.

Likewise I'm keeping John and Alistair tags, but I'd like an extra
review for the migration change, thanks!

[*] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00679.html
---
 hw/block/pflash_cfi01.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 9e34fd4e82..58cbef0588 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -100,7 +100,7 @@ static int pflash_post_load(void *opaque, int version_id);
 
 static const VMStateDescription vmstate_pflash = {
     .name = "pflash_cfi01",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .post_load = pflash_post_load,
     .fields = (VMStateField[]) {
@@ -277,10 +277,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
         /* This should never happen : reset state & treat it as a read */
         DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
         pfl->wcycle = 0;
-        pfl->cmd = 0;
+        pfl->cmd = 0xff;
         /* fall through to read code */
-    case 0x00:
-        /* Flash area read */
+    case 0xff: /* Read Array */
         ret = pflash_data_read(pfl, offset, width, be);
         break;
     case 0x10: /* Single byte program */
@@ -448,8 +447,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     case 0:
         /* read mode */
         switch (cmd) {
-        case 0x00: /* ??? */
-            goto reset_flash;
         case 0x10: /* Single Byte Program */
         case 0x40: /* Single Byte Program */
             DPRINTF("%s: Single Byte Program\n", __func__);
@@ -526,7 +523,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             if (cmd == 0xd0) { /* confirm */
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
-            } else if (cmd == 0xff) { /* read array mode */
+            } else if (cmd == 0xff) { /* Read Array */
                 goto reset_flash;
             } else
                 goto error_flash;
@@ -553,7 +550,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             } else if (cmd == 0x01) {
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
-            } else if (cmd == 0xff) {
+            } else if (cmd == 0xff) { /* read array mode */
                 goto reset_flash;
             } else {
                 DPRINTF("%s: Unknown (un)locking command\n", __func__);
@@ -645,7 +642,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     trace_pflash_reset();
     memory_region_rom_device_set_romd(&pfl->mem, true);
     pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pfl->cmd = 0xff;
 }
 
 
@@ -761,7 +758,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pfl->cmd = 0xff;
     pfl->status = 0;
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
@@ -1001,5 +998,11 @@ static int pflash_post_load(void *opaque, int version_id)
         pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
                                                         pfl);
     }
+    if (version_id < 2) {
+        /* v1 used incorrect value of 0x00 for the READ_ARRAY command. */
+        if (pfl->cmd == 0x00) {
+            pfl->cmd =  0xff;
+        }
+    }
     return 0;
 }
-- 
2.20.1

[Qemu-devel] [PATCH v3 3/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array()

[Philippe Mathieu-Daudé]

The same pattern is used when setting the flash in READ_ARRAY mode:
- Set the state machine command to READ_ARRAY
- Reset the write_cycle counter
- Reset the memory region in ROMD

Refactor the current code by extracting this pattern.
It is used twice:
- On a write access (on command failure, error, or explicitly asked)
- When the device is initialized. Here the ROMD mode is hidden
  by the memory_region_init_rom_device() call.

Rename the 'reset_flash' as 'mode_read_array' to make explicit we
do not reset the device, we simply set its internal state machine
in the READ_ARRAY mode. We do not reset the status register error
bits, as a device reset would do.

Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 36 ++++++++++++++++++++----------------
 hw/block/trace-events   |  1 +
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 58cbef0588..81fbdbde7f 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -112,6 +112,14 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
+static void pflash_mode_read_array(PFlashCFI01 *pfl)
+{
+    trace_pflash_mode_read_array();
+    pfl->cmd = 0xff; /* Read Array */
+    pfl->wcycle = 0;
+    memory_region_rom_device_set_romd(&pfl->mem, true);
+}
+
 /* Perform a CFI query based on the bank width of the flash.
  * If this code is called we know we have a device_width set for
  * this flash.
@@ -469,7 +477,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
         case 0x50: /* Clear status bits */
             DPRINTF("%s: Clear status bits\n", __func__);
             pfl->status = 0x0;
-            goto reset_flash;
+            goto mode_read_array;
         case 0x60: /* Block (un)lock */
             DPRINTF("%s: Block unlock\n", __func__);
             break;
@@ -494,10 +502,10 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             break;
         case 0xf0: /* Probe for AMD flash */
             DPRINTF("%s: Probe for AMD flash\n", __func__);
-            goto reset_flash;
+            goto mode_read_array;
         case 0xff: /* Read array mode */
             DPRINTF("%s: Read array mode\n", __func__);
-            goto reset_flash;
+            goto mode_read_array;
         default:
             goto error_flash;
         }
@@ -524,7 +532,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
             } else if (cmd == 0xff) { /* Read Array */
-                goto reset_flash;
+                goto mode_read_array;
             } else
                 goto error_flash;
 
@@ -551,15 +559,15 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
             } else if (cmd == 0xff) { /* read array mode */
-                goto reset_flash;
+                goto mode_read_array;
             } else {
                 DPRINTF("%s: Unknown (un)locking command\n", __func__);
-                goto reset_flash;
+                goto mode_read_array;
             }
             break;
         case 0x98:
             if (cmd == 0xff) {
-                goto reset_flash;
+                goto mode_read_array;
             } else {
                 DPRINTF("%s: leaving query mode\n", __func__);
             }
@@ -619,7 +627,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                     " the data is already written to storage!\n"
                     "Flash device reset into READ mode.\n",
                     __func__);
-                goto reset_flash;
+                goto mode_read_array;
             }
             break;
         default:
@@ -629,7 +637,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     default:
         /* Should never happen */
         DPRINTF("%s: invalid write state\n",  __func__);
-        goto reset_flash;
+        goto mode_read_array;
     }
     return;
 
@@ -638,11 +646,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                   "(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value 0x%x)"
                   "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
 
- reset_flash:
-    trace_pflash_reset();
-    memory_region_rom_device_set_romd(&pfl->mem, true);
-    pfl->wcycle = 0;
-    pfl->cmd = 0xff;
+ mode_read_array:
+    pflash_mode_read_array(pfl);
 }
 
 
@@ -757,8 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->wcycle = 0;
-    pfl->cmd = 0xff;
+    pflash_mode_read_array(pfl);
     pfl->status = 0;
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 13d1b21dd4..91a8a106c0 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -7,6 +7,7 @@ fdc_ioport_write(uint8_t reg, uint8_t value) "write reg 0x%02x val 0x%02x"
 # pflash_cfi02.c
 # pflash_cfi01.c
 pflash_reset(void) "reset"
+pflash_mode_read_array(void) "mode: read array"
 pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
 pflash_io_read(uint64_t offset, int width, int fmt_width, uint32_t value, uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x cmd:0x%02x wcycle:%u"
 pflash_io_write(uint64_t offset, int width, int fmt_width, uint32_t value, uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x wcycle:%u"
-- 
2.20.1

[Qemu-devel] [PATCH v3 4/9] hw/block/pflash_cfi01: Start state machine as READY to accept commands

[Philippe Mathieu-Daudé]

When the state machine is ready to accept command, the bit 7 of
the status register (SR) is set to 1.
The guest polls the status register and check this bit before
writting command to the internal 'Write State Machine' (WSM).

Set SR.7 bit to 1 when the device is created.

There is no migration impact by this change.

Reference: Read Array Flowchart
  "Common Flash Interface (CFI) and Command Sets"
   (Intel Application Note 646)
   Appendix B "Basic Command Set"

Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v3: Added migration comment.
---
 hw/block/pflash_cfi01.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 81fbdbde7f..200bfd0ab8 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -763,7 +763,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     pflash_mode_read_array(pfl);
-    pfl->status = 0;
+    pfl->status = 0x80; /* WSM ready */
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

[Qemu-devel] [PATCH v3 5/9] hw/block/pflash_cfi01: Add the DeviceReset() handler

[Philippe Mathieu-Daudé]

A "system reset" sets the device state machine in READ_ARRAY mode
and, after some delay, set the SR.7 READY bit.

We do not model timings, so we set the SR.7 bit directly.

The TYPE_DEVICE interface provides a DeviceReset handler.
This pflash device is a subclass of TYPE_SYS_BUS_DEVICE (which
is a subclass of TYPE_DEVICE).
SYS_BUS devices are automatically plugged into the 'main system
bus', which is the root of the qbus tree.
Devices in the qbus tree are guaranteed to have their reset()
handler called after realize() and before we try to run the guest.

To avoid incoherent states when the machine resets (see but report
below), factor out the reset code into pflash_cfi01_system_reset,
and register the method as a device reset callback.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Reported-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v3: reword description
---
 hw/block/pflash_cfi01.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 200bfd0ab8..c32c67d01d 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -762,8 +762,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pflash_mode_read_array(pfl);
-    pfl->status = 0x80; /* WSM ready */
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
@@ -851,6 +849,18 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
 }
 
+static void pflash_cfi01_system_reset(DeviceState *dev)
+{
+    PFlashCFI01 *pfl = PFLASH_CFI01(dev);
+
+    pflash_mode_read_array(pfl);
+    /*
+     * The WSM ready timer occurs at most 150ns after system reset.
+     * This model deliberately ignores this delay.
+     */
+    pfl->status = 0x80;
+}
+
 static Property pflash_cfi01_properties[] = {
     DEFINE_PROP_DRIVE("drive", PFlashCFI01, blk),
     /* num-blocks is the number of blocks actually visible to the guest,
@@ -895,6 +905,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = pflash_cfi01_system_reset;
     dc->realize = pflash_cfi01_realize;
     dc->props = pflash_cfi01_properties;
     dc->vmsd = &vmstate_pflash;
-- 
2.20.1

[Qemu-devel] [PATCH v3 6/9] hw/block/pflash_cfi01: Simplify CFI_QUERY processing

[Philippe Mathieu-Daudé]

The current code does:

if (write_cycle == 0)
  if (command == CFI_QUERY)
    break
  write_cycle += 1
  last_command = command

if (write_cycle == 1)
  if (last_command == CFI_QUERY)
    if (command == READ_ARRAY
      write_cycle = 0
      last_command = READ_ARRAY

Simplify by not increasing the write_cycle on CFI_QUERY,
the next command are processed as normal wcycle=0.

This matches the hardware datasheet (we do not enter the WRITE
state machine, thus no write cycle involved).

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index c32c67d01d..e097d9260d 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -491,7 +491,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             return;
         case 0x98: /* CFI query */
             DPRINTF("%s: CFI query\n", __func__);
-            break;
+            pfl->cmd = cmd;
+            return;
         case 0xe8: /* Write to buffer */
             DPRINTF("%s: Write to buffer\n", __func__);
             /* FIXME should save @offset, @width for case 1+ */
@@ -565,13 +566,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                 goto mode_read_array;
             }
             break;
-        case 0x98:
-            if (cmd == 0xff) {
-                goto mode_read_array;
-            } else {
-                DPRINTF("%s: leaving query mode\n", __func__);
-            }
-            break;
         default:
             goto error_flash;
         }
-- 
2.20.1

[Qemu-devel] [PATCH v3 7/9] hw/block/pflash_cfi01: Improve command comments

[Philippe Mathieu-Daudé]

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index e097d9260d..ba00e52923 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -208,11 +208,11 @@ static uint32_t pflash_devid_query(PFlashCFI01 *pfl, hwaddr offset)
      * Offsets 2/3 are block lock status, is not emulated.
      */
     switch (boff & 0xFF) {
-    case 0:
+    case 0: /* Manufacturer ID */
         resp = pfl->ident0;
         trace_pflash_manufacturer_id(resp);
         break;
-    case 1:
+    case 1: /* Device ID */
         resp = pfl->ident1;
         trace_pflash_device_id(resp);
         break;
@@ -455,11 +455,11 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     case 0:
         /* read mode */
         switch (cmd) {
-        case 0x10: /* Single Byte Program */
-        case 0x40: /* Single Byte Program */
+        case 0x10: /* Single Byte Program (setup) */
+        case 0x40: /* Single Byte Program (setup) [Intel] */
             DPRINTF("%s: Single Byte Program\n", __func__);
             break;
-        case 0x20: /* Block erase */
+        case 0x20: /* Block erase (setup) */
             p = pfl->storage;
             offset &= ~(pfl->sector_len - 1);
 
@@ -515,8 +515,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
         break;
     case 1:
         switch (pfl->cmd) {
-        case 0x10: /* Single Byte Program */
-        case 0x40: /* Single Byte Program */
+        case 0x10: /* Single Byte Program (confirm) */
+        case 0x40: /* Single Byte Program (confirm) [Intel] */
             DPRINTF("%s: Single Byte Program\n", __func__);
             if (!pfl->ro) {
                 pflash_data_write(pfl, offset, value, width, be);
@@ -527,7 +527,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             pfl->status |= 0x80; /* Ready! */
             pfl->wcycle = 0;
         break;
-        case 0x20: /* Block erase */
+        case 0x20: /* Block erase (confirm) */
         case 0x28:
             if (cmd == 0xd0) { /* confirm */
                 pfl->wcycle = 0;
-- 
2.20.1

[Qemu-devel] [PATCH v3 8/9] hw/block/pflash_cfi01: Replace DPRINTF by qemu_log_mask(GUEST_ERROR)

[Philippe Mathieu-Daudé]

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index ba00e52923..ab72af22a7 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -283,7 +283,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
     switch (pfl->cmd) {
     default:
         /* This should never happen : reset state & treat it as a read */
-        DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid state, "
+                      "wcycle %d cmd 0x02%x\n",
+                      __func__, pfl->wcycle, pfl->cmd);
         pfl->wcycle = 0;
         pfl->cmd = 0xff;
         /* fall through to read code */
@@ -630,7 +632,9 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
         break;
     default:
         /* Should never happen */
-        DPRINTF("%s: invalid write state\n",  __func__);
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid state, "
+                      "wcycle %d cmd (0x02%x -> value 0x02%x)\n",
+                      __func__, pfl->wcycle, pfl->cmd, value);
         goto mode_read_array;
     }
     return;
-- 
2.20.1

[Qemu-devel] [PATCH v3 9/9] hw/block/pflash_cfi01: Hold the PRI table offset in a variable

[Philippe Mathieu-Daudé]

Manufacturers are allowed to move the PRI table, this is why the
offset is queryable via fixed offsets 0x15/0x16.
Add a variable to hold the offset, so it will be easier to later
move the PRI table.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index ab72af22a7..67e714b32d 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -761,6 +761,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     /* Hardcoded CFI table */
+    const uint16_t pri_ofs = 0x31;
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
     pfl->cfi_table[0x11] = 'R';
@@ -769,14 +770,17 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x13] = 0x01;
     pfl->cfi_table[0x14] = 0x00;
     /* Primary extended table address (none) */
-    pfl->cfi_table[0x15] = 0x31;
-    pfl->cfi_table[0x16] = 0x00;
+    pfl->cfi_table[0x15] = pri_ofs;
+    pfl->cfi_table[0x16] = pri_ofs >> 8;
     /* Alternate command set (none) */
     pfl->cfi_table[0x17] = 0x00;
     pfl->cfi_table[0x18] = 0x00;
     /* Alternate extended table (none) */
     pfl->cfi_table[0x19] = 0x00;
     pfl->cfi_table[0x1A] = 0x00;
+
+    /* CFI: System Interface Information */
+
     /* Vcc min */
     pfl->cfi_table[0x1B] = 0x45;
     /* Vcc max */
@@ -801,6 +805,9 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x25] = 0x04;
     /* Max timeout for chip erase */
     pfl->cfi_table[0x26] = 0x00;
+
+    /* CFI: Device Geometry Definition */
+
     /* Device size */
     pfl->cfi_table[0x27] = ctz32(device_len); /* + 1; */
     /* Flash device interface (8 & 16 bits) */
@@ -825,26 +832,30 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x2E] = (blocks_per_device - 1) >> 8;
     pfl->cfi_table[0x2F] = sector_len_per_device >> 8;
     pfl->cfi_table[0x30] = sector_len_per_device >> 16;
+    assert(0x30 < pri_ofs);
+
+    /* CFI: Primary-Vendor Specific */
 
     /* Extended */
-    pfl->cfi_table[0x31] = 'P';
-    pfl->cfi_table[0x32] = 'R';
-    pfl->cfi_table[0x33] = 'I';
+    pfl->cfi_table[0x00 + pri_ofs] = 'P';
+    pfl->cfi_table[0x01 + pri_ofs] = 'R';
+    pfl->cfi_table[0x02 + pri_ofs] = 'I';
 
-    pfl->cfi_table[0x34] = '1';
-    pfl->cfi_table[0x35] = '0';
+    pfl->cfi_table[0x03 + pri_ofs] = '1';
+    pfl->cfi_table[0x04 + pri_ofs] = '0';
 
-    pfl->cfi_table[0x36] = 0x00;
-    pfl->cfi_table[0x37] = 0x00;
-    pfl->cfi_table[0x38] = 0x00;
-    pfl->cfi_table[0x39] = 0x00;
+    pfl->cfi_table[0x05 + pri_ofs] = 0x00; /* Optional features */
+    pfl->cfi_table[0x06 + pri_ofs] = 0x00;
+    pfl->cfi_table[0x07 + pri_ofs] = 0x00;
+    pfl->cfi_table[0x08 + pri_ofs] = 0x00;
 
-    pfl->cfi_table[0x3a] = 0x00;
+    pfl->cfi_table[0x09 + pri_ofs] = 0x00; /* Func. supported after suspend */
 
-    pfl->cfi_table[0x3b] = 0x00;
-    pfl->cfi_table[0x3c] = 0x00;
+    pfl->cfi_table[0x0a + pri_ofs] = 0x00; /* Block status register mask */
+    pfl->cfi_table[0x0b + pri_ofs] = 0x00;
 
-    pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
+    pfl->cfi_table[0x0e + pri_ofs] = 0x01; /* Number of protection fields */
+    assert(0x0e + pri_ofs < ARRAY_SIZE(pfl->cfi_table));
 }
 
 static void pflash_cfi01_system_reset(DeviceState *dev)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Laszlo Ersek]

On 07/05/19 17:46, Philippe Mathieu-Daudé wrote:
> In the "Read Array Flowchart" the command has a value of 0xFF.
> 
> In the document [*] the "Read Array Flowchart", the READ_ARRAY
> command has a value of 0xff.
> 
> Use the correct value in the pflash model.
> 
> There is no change of behavior in the guest, because:
> - when the guest were sending 0xFF, the reset_flash label
>   was setting the command value as 0x00
> - 0x00 was used internally for READ_ARRAY
> 
> To keep migration behaving correctly, we have to increase
> the VMState version. When migrating from an older version,
> we use the correct command value.
> 
> [*] "Common Flash Interface (CFI) and Command Sets"
>     (Intel Application Note 646)
>     Appendix B "Basic Command Set"
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v3: Handle migrating the 'cmd' field.
> 
> Since Laszlo stated he did not test migration [*], I'm keeping his
> test tag, because the change with v2 has no impact in the tests
> he ran.

My thinking exactly. Thanks!
Laszlo

> 
> Likewise I'm keeping John and Alistair tags, but I'd like an extra
> review for the migration change, thanks!
> 
> [*] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00679.html
> ---
>  hw/block/pflash_cfi01.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9e34fd4e82..58cbef0588 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -100,7 +100,7 @@ static int pflash_post_load(void *opaque, int version_id);
>  
>  static const VMStateDescription vmstate_pflash = {
>      .name = "pflash_cfi01",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .post_load = pflash_post_load,
>      .fields = (VMStateField[]) {
> @@ -277,10 +277,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>          pfl->wcycle = 0;
> -        pfl->cmd = 0;
> +        pfl->cmd = 0xff;
>          /* fall through to read code */
> -    case 0x00:
> -        /* Flash area read */
> +    case 0xff: /* Read Array */
>          ret = pflash_data_read(pfl, offset, width, be);
>          break;
>      case 0x10: /* Single byte program */
> @@ -448,8 +447,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      case 0:
>          /* read mode */
>          switch (cmd) {
> -        case 0x00: /* ??? */
> -            goto reset_flash;
>          case 0x10: /* Single Byte Program */
>          case 0x40: /* Single Byte Program */
>              DPRINTF("%s: Single Byte Program\n", __func__);
> @@ -526,7 +523,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              if (cmd == 0xd0) { /* confirm */
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) { /* read array mode */
> +            } else if (cmd == 0xff) { /* Read Array */
>                  goto reset_flash;
>              } else
>                  goto error_flash;
> @@ -553,7 +550,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              } else if (cmd == 0x01) {
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) {
> +            } else if (cmd == 0xff) { /* read array mode */
>                  goto reset_flash;
>              } else {
>                  DPRINTF("%s: Unknown (un)locking command\n", __func__);
> @@ -645,7 +642,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      trace_pflash_reset();
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>  }
>  
>  
> @@ -761,7 +758,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>  
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>      pfl->status = 0;
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> @@ -1001,5 +998,11 @@ static int pflash_post_load(void *opaque, int version_id)
>          pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
>                                                          pfl);
>      }
> +    if (version_id < 2) {
> +        /* v1 used incorrect value of 0x00 for the READ_ARRAY command. */
> +        if (pfl->cmd == 0x00) {
> +            pfl->cmd =  0xff;
> +        }
> +    }
>      return 0;
>  }
> 

Re: [Qemu-devel] [PATCH v3 5/9] hw/block/pflash_cfi01: Add the DeviceReset() handler

[Alistair Francis]

On Fri, Jul 5, 2019 at 8:51 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> A "system reset" sets the device state machine in READ_ARRAY mode
> and, after some delay, set the SR.7 READY bit.
>
> We do not model timings, so we set the SR.7 bit directly.
>
> The TYPE_DEVICE interface provides a DeviceReset handler.
> This pflash device is a subclass of TYPE_SYS_BUS_DEVICE (which
> is a subclass of TYPE_DEVICE).
> SYS_BUS devices are automatically plugged into the 'main system
> bus', which is the root of the qbus tree.
> Devices in the qbus tree are guaranteed to have their reset()
> handler called after realize() and before we try to run the guest.
>
> To avoid incoherent states when the machine resets (see but report
> below), factor out the reset code into pflash_cfi01_system_reset,
> and register the method as a device reset callback.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> v3: reword description
> ---
>  hw/block/pflash_cfi01.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 200bfd0ab8..c32c67d01d 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -762,8 +762,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>
> -    pflash_mode_read_array(pfl);
> -    pfl->status = 0x80; /* WSM ready */
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> @@ -851,6 +849,18 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
>  }
>
> +static void pflash_cfi01_system_reset(DeviceState *dev)
> +{
> +    PFlashCFI01 *pfl = PFLASH_CFI01(dev);
> +
> +    pflash_mode_read_array(pfl);
> +    /*
> +     * The WSM ready timer occurs at most 150ns after system reset.
> +     * This model deliberately ignores this delay.
> +     */
> +    pfl->status = 0x80;
> +}
> +
>  static Property pflash_cfi01_properties[] = {
>      DEFINE_PROP_DRIVE("drive", PFlashCFI01, blk),
>      /* num-blocks is the number of blocks actually visible to the guest,
> @@ -895,6 +905,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
> +    dc->reset = pflash_cfi01_system_reset;
>      dc->realize = pflash_cfi01_realize;
>      dc->props = pflash_cfi01_properties;
>      dc->vmsd = &vmstate_pflash;
> --
> 2.20.1
>
>

Re: [Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> In the "Read Array Flowchart" the command has a value of 0xFF.
> 
> In the document [*] the "Read Array Flowchart", the READ_ARRAY
> command has a value of 0xff.
> 
> Use the correct value in the pflash model.
> 
> There is no change of behavior in the guest, because:
> - when the guest were sending 0xFF, the reset_flash label
>   was setting the command value as 0x00
> - 0x00 was used internally for READ_ARRAY
> 
> To keep migration behaving correctly, we have to increase
> the VMState version. When migrating from an older version,
> we use the correct command value.

The problem is that incrementing the version will break backwards
compatibility; so you won't be able to migrate this back to an older
QEMU version; so for example a q35/uefi with this won't be able
to migrate backwards to a 4.0.0 or older qemu.

So instead of bumping the version_id you probably need to wire
the behaviour to a machine type and then on your new type
wire a subsection containing a flag; the reception of that subsection
tells you to use the new/correct semantics.


Dave


> [*] "Common Flash Interface (CFI) and Command Sets"
>     (Intel Application Note 646)
>     Appendix B "Basic Command Set"
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v3: Handle migrating the 'cmd' field.
> 
> Since Laszlo stated he did not test migration [*], I'm keeping his
> test tag, because the change with v2 has no impact in the tests
> he ran.
> 
> Likewise I'm keeping John and Alistair tags, but I'd like an extra
> review for the migration change, thanks!
> 
> [*] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00679.html
> ---
>  hw/block/pflash_cfi01.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9e34fd4e82..58cbef0588 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -100,7 +100,7 @@ static int pflash_post_load(void *opaque, int version_id);
>  
>  static const VMStateDescription vmstate_pflash = {
>      .name = "pflash_cfi01",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .post_load = pflash_post_load,
>      .fields = (VMStateField[]) {
> @@ -277,10 +277,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>          pfl->wcycle = 0;
> -        pfl->cmd = 0;
> +        pfl->cmd = 0xff;
>          /* fall through to read code */
> -    case 0x00:
> -        /* Flash area read */
> +    case 0xff: /* Read Array */
>          ret = pflash_data_read(pfl, offset, width, be);
>          break;
>      case 0x10: /* Single byte program */
> @@ -448,8 +447,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      case 0:
>          /* read mode */
>          switch (cmd) {
> -        case 0x00: /* ??? */
> -            goto reset_flash;
>          case 0x10: /* Single Byte Program */
>          case 0x40: /* Single Byte Program */
>              DPRINTF("%s: Single Byte Program\n", __func__);
> @@ -526,7 +523,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              if (cmd == 0xd0) { /* confirm */
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) { /* read array mode */
> +            } else if (cmd == 0xff) { /* Read Array */
>                  goto reset_flash;
>              } else
>                  goto error_flash;
> @@ -553,7 +550,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              } else if (cmd == 0x01) {
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) {
> +            } else if (cmd == 0xff) { /* read array mode */
>                  goto reset_flash;
>              } else {
>                  DPRINTF("%s: Unknown (un)locking command\n", __func__);
> @@ -645,7 +642,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      trace_pflash_reset();
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>  }
>  
>  
> @@ -761,7 +758,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>  
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>      pfl->status = 0;
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> @@ -1001,5 +998,11 @@ static int pflash_post_load(void *opaque, int version_id)
>          pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
>                                                          pfl);
>      }
> +    if (version_id < 2) {
> +        /* v1 used incorrect value of 0x00 for the READ_ARRAY command. */
> +        if (pfl->cmd == 0x00) {
> +            pfl->cmd =  0xff;
> +        }
> +    }
>      return 0;
>  }
> -- 
> 2.20.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Philippe Mathieu-Daudé]

Hi David,

On 7/9/19 12:30 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> In the "Read Array Flowchart" the command has a value of 0xFF.
>>
>> In the document [*] the "Read Array Flowchart", the READ_ARRAY
>> command has a value of 0xff.
>>
>> Use the correct value in the pflash model.
>>
>> There is no change of behavior in the guest, because:
>> - when the guest were sending 0xFF, the reset_flash label
>>   was setting the command value as 0x00
>> - 0x00 was used internally for READ_ARRAY
>>
>> To keep migration behaving correctly, we have to increase
>> the VMState version. When migrating from an older version,
>> we use the correct command value.
> 
> The problem is that incrementing the version will break backwards
> compatibility; so you won't be able to migrate this back to an older
> QEMU version; so for example a q35/uefi with this won't be able
> to migrate backwards to a 4.0.0 or older qemu.
> 
> So instead of bumping the version_id you probably need to wire
> the behaviour to a machine type and then on your new type
> wire a subsection containing a flag; the reception of that subsection
> tells you to use the new/correct semantics.

I'm starting to understand VMState subsections, but it might be overkill
for this change...

  Subsections
  -----------

  The most common structure change is adding new data, e.g. when adding
  a newer form of device, or adding that state that you previously
  forgot to migrate.  This is best solved using a subsection.

This is not the case here, the field is already present and migrated.

It seems I can use a simple pre_save hook, always migrating the
READ_ARRAY using the incorrect value:

-- >8 --
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -97,12 +97,29 @@ struct PFlashCFI01 {
     bool incorrect_read_array_command;
 };

+static int pflash_pre_save(void *opaque)
+{
+    PFlashCFI01 *s = opaque;
+
+    /*
+     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
+     * READ_ARRAY command. To preserve migrating to these older version,
+     * always migrate the READ_ARRAY command as 0x00.
+     */
+    if (s->cmd == 0xff) {
+        s->cmd = 0x00;
+    }
+
+    return 0;
+}
+
 static int pflash_post_load(void *opaque, int version_id);

 static const VMStateDescription vmstate_pflash = {
     .name = "pflash_cfi01",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_save = pflash_pre_save,
     .post_load = pflash_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(wcycle, PFlashCFI01),
@@ -1001,5 +1018,14 @@ static int pflash_post_load(void *opaque, int
version_id)
         pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
                                                         pfl);
     }
+
+    /*
+     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
+     * READ_ARRAY command.
+     */
+    if (pfl->cmd == 0x00) {
+        pfl->cmd = 0xff;
+    }
+
     return 0;
 }
---

Being simpler and less intrusive (no new property in hw/core/machine.c),
is this acceptable?

Thanks,

Phil.

[...]

Re: [Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Hi David,
> 
> On 7/9/19 12:30 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> >> In the "Read Array Flowchart" the command has a value of 0xFF.
> >>
> >> In the document [*] the "Read Array Flowchart", the READ_ARRAY
> >> command has a value of 0xff.
> >>
> >> Use the correct value in the pflash model.
> >>
> >> There is no change of behavior in the guest, because:
> >> - when the guest were sending 0xFF, the reset_flash label
> >>   was setting the command value as 0x00
> >> - 0x00 was used internally for READ_ARRAY
> >>
> >> To keep migration behaving correctly, we have to increase
> >> the VMState version. When migrating from an older version,
> >> we use the correct command value.
> > 
> > The problem is that incrementing the version will break backwards
> > compatibility; so you won't be able to migrate this back to an older
> > QEMU version; so for example a q35/uefi with this won't be able
> > to migrate backwards to a 4.0.0 or older qemu.
> > 
> > So instead of bumping the version_id you probably need to wire
> > the behaviour to a machine type and then on your new type
> > wire a subsection containing a flag; the reception of that subsection
> > tells you to use the new/correct semantics.
> 
> I'm starting to understand VMState subsections, but it might be overkill
> for this change...
> 
>   Subsections
>   -----------
> 
>   The most common structure change is adding new data, e.g. when adding
>   a newer form of device, or adding that state that you previously
>   forgot to migrate.  This is best solved using a subsection.
> 
> This is not the case here, the field is already present and migrated.
> 
> It seems I can use a simple pre_save hook, always migrating the
> READ_ARRAY using the incorrect value:
> 
> -- >8 --
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -97,12 +97,29 @@ struct PFlashCFI01 {
>      bool incorrect_read_array_command;
>  };
> 
> +static int pflash_pre_save(void *opaque)
> +{
> +    PFlashCFI01 *s = opaque;
> +
> +    /*
> +     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
> +     * READ_ARRAY command. To preserve migrating to these older version,
> +     * always migrate the READ_ARRAY command as 0x00.
> +     */
> +    if (s->cmd == 0xff) {
> +        s->cmd = 0x00;
> +    }
> +
> +    return 0;
> +}

Be careful what happens if migration fails and you continue on the
source - is that OK - or are you going to have to flip that back somehow
(in a post_save).

Another way to do the same is to have a dummy field; tmp_cmd, and the
tmp_cmd is the thing that's actually migrated and filled by pre_save
(or use VMSTATE_WITH_TMP )


>  static int pflash_post_load(void *opaque, int version_id);
> 
>  static const VMStateDescription vmstate_pflash = {
>      .name = "pflash_cfi01",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .pre_save = pflash_pre_save,
>      .post_load = pflash_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(wcycle, PFlashCFI01),
> @@ -1001,5 +1018,14 @@ static int pflash_post_load(void *opaque, int
> version_id)
>          pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
>                                                          pfl);
>      }
> +
> +    /*
> +     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
> +     * READ_ARRAY command.
> +     */
> +    if (pfl->cmd == 0x00) {
> +        pfl->cmd = 0xff;
> +    }
> +
>      return 0;
>  }
> ---
> 
> Being simpler and less intrusive (no new property in hw/core/machine.c),
> is this acceptable?

From the migration point of view yes; I don't know enough about pflash
to say if it makes sense;  for example could there ever be a 00 command
really used and then you'd have to distinguish that somehow?

> Thanks,
> 
> Phil.
> 
> [...]
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Philippe Mathieu-Daudé]

On 7/9/19 7:10 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> Hi David,
>>
>> On 7/9/19 12:30 PM, Dr. David Alan Gilbert wrote:
>>> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>>>> In the "Read Array Flowchart" the command has a value of 0xFF.
>>>>
>>>> In the document [*] the "Read Array Flowchart", the READ_ARRAY
>>>> command has a value of 0xff.
>>>>
>>>> Use the correct value in the pflash model.
>>>>
>>>> There is no change of behavior in the guest, because:
>>>> - when the guest were sending 0xFF, the reset_flash label
>>>>   was setting the command value as 0x00
>>>> - 0x00 was used internally for READ_ARRAY
>>>>
>>>> To keep migration behaving correctly, we have to increase
>>>> the VMState version. When migrating from an older version,
>>>> we use the correct command value.
>>>
>>> The problem is that incrementing the version will break backwards
>>> compatibility; so you won't be able to migrate this back to an older
>>> QEMU version; so for example a q35/uefi with this won't be able
>>> to migrate backwards to a 4.0.0 or older qemu.
>>>
>>> So instead of bumping the version_id you probably need to wire
>>> the behaviour to a machine type and then on your new type
>>> wire a subsection containing a flag; the reception of that subsection
>>> tells you to use the new/correct semantics.
>>
>> I'm starting to understand VMState subsections, but it might be overkill
>> for this change...
>>
>>   Subsections
>>   -----------
>>
>>   The most common structure change is adding new data, e.g. when adding
>>   a newer form of device, or adding that state that you previously
>>   forgot to migrate.  This is best solved using a subsection.
>>
>> This is not the case here, the field is already present and migrated.
>>
>> It seems I can use a simple pre_save hook, always migrating the
>> READ_ARRAY using the incorrect value:
>>
>> -- >8 --
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -97,12 +97,29 @@ struct PFlashCFI01 {
>>      bool incorrect_read_array_command;
>>  };
>>
>> +static int pflash_pre_save(void *opaque)
>> +{
>> +    PFlashCFI01 *s = opaque;
>> +
>> +    /*
>> +     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
>> +     * READ_ARRAY command. To preserve migrating to these older version,
>> +     * always migrate the READ_ARRAY command as 0x00.
>> +     */
>> +    if (s->cmd == 0xff) {
>> +        s->cmd = 0x00;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> Be careful what happens if migration fails and you continue on the
> source - is that OK - or are you going to have to flip that back somehow
> (in a post_save).

Hmm OK...

> 
> Another way to do the same is to have a dummy field; tmp_cmd, and the
> tmp_cmd is the thing that's actually migrated and filled by pre_save
> (or use VMSTATE_WITH_TMP )
> 
> 
>>  static int pflash_post_load(void *opaque, int version_id);
>>
>>  static const VMStateDescription vmstate_pflash = {
>>      .name = "pflash_cfi01",
>>      .version_id = 1,
>>      .minimum_version_id = 1,
>> +    .pre_save = pflash_pre_save,
>>      .post_load = pflash_post_load,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT8(wcycle, PFlashCFI01),
>> @@ -1001,5 +1018,14 @@ static int pflash_post_load(void *opaque, int
>> version_id)
>>          pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
>>                                                          pfl);
>>      }
>> +
>> +    /*
>> +     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
>> +     * READ_ARRAY command.
>> +     */
>> +    if (pfl->cmd == 0x00) {
>> +        pfl->cmd = 0xff;
>> +    }
>> +
>>      return 0;
>>  }
>> ---
>>
>> Being simpler and less intrusive (no new property in hw/core/machine.c),
>> is this acceptable?
> 
> From the migration point of view yes; I don't know enough about pflash
> to say if it makes sense;  for example could there ever be a 00 command
> really used and then you'd have to distinguish that somehow?

Well, I think this change is simpler than it looks.

Currently the code is (what will run on older guest):

static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
                            int width, int be)
{
    switch (pfl->cmd) {
    default:
        /* This should never happen : reset state & treat it as a read*/
        DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
        pfl->wcycle = 0;
        pfl->cmd = 0;
        /* fall through to read code */
    case 0x00:
        /* Flash area read */
        ret = pflash_data_read(pfl, offset, width, be);
        break;

and:

static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                         uint32_t value, int width, int be)
{
    switch (pfl->wcycle) {
    case 0:
        /* read mode */
        switch (cmd) {
        case 0x00: /* ??? */
            goto reset_flash;
        case 0xff: /* Read array mode */
            DPRINTF("%s: Read array mode\n", __func__);
            goto reset_flash;
        default:
            goto error_flash;
        }

So current (incorrect) 0x00 will be then 0xff, and will be backprocessed
correctly.

What I want is to get ride of this 0x00 processing that is confusing,
the spec and the guests use 0xff for READ_ARRAY.

So if I increase version I can not back-migrate, but luckily it seems I
can simply update 0x00 -> 0xff without even increasing the version :)

(I'm reluctant to skip this patch because I'd rather avoid Laszlo to
re-run his tests).

Regards,

Phil.