[Qemu-devel] [PATCH-for-4.1 v6 0/5] hw/block/pflash_cfi01: Add DeviceReset() handler

[Qemu-devel] [PATCH-for-4.1 v6 0/5] hw/block/pflash_cfi01: Add DeviceReset() handler

[Philippe Mathieu-Daudé]

Hello it's me again, insisting with this series because there are at
least 2 different report of guests bricked on reset due to the bug
fixed by patch #5:
https://bugzilla.redhat.com/show_bug.cgi?id=1678713
https://bugzilla.redhat.com/show_bug.cgi?id=1704584

Patches missing review: 2 and 3

The pflash device lacks a reset() function.
When a machine is resetted, the flash might be in an
inconsistent state, leading to unexpected behavior.
Resolve this issue by adding a DeviceReset() handler.

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

Since v2: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00395.html
- consider migration (Laszlo, Peter)

Since v3: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01668.html
- more reliable migration (Dave)
- dropped patches 6-9 not required for next release

Since v4: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg02785.html
- document why using READ_ARRAY value 0x00 for migration is safe

Since v5: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03366.html
- avoid trying to be spec-compliant and messing with migration. KISS.
  review/test tags reset, sorry.

$ git backport-diff -u v5
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/5:[----] [-C] 'hw/block/pflash_cfi01: Removed an unused timer'
002/5:[down] 'hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00''
003/5:[0031] [FC] 'hw/block/pflash_cfi01: Extract pflash_mode_read_array()'
004/5:[down] 'hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array''
005/5:[----] [--] 'hw/block/pflash_cfi01: Add the DeviceReset() handler'

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  hw/block/pflash_cfi01: Removed an unused timer
  hw/block/pflash_cfi01: Document use of non-CFI compliant command
    '0x00'
  hw/block/pflash_cfi01: Extract pflash_mode_read_array()
  hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array'
  hw/block/pflash_cfi01: Add the DeviceReset() handler

 hw/block/pflash_cfi01.c | 77 +++++++++++++++++++++--------------------
 hw/block/trace-events   |  1 +
 2 files changed, 41 insertions(+), 37 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v6 1/5] 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>
---
 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 435be1e35c..a9529957f8 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 = 0x80; /* WSM ready */
-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v6 2/5] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00'

[Philippe Mathieu-Daudé]

The command 0x00 is used by this model since its origin (commit
05ee37ebf630). In this commit the command is described with a
amusing '/* ??? */' comment, probably meaning 'FIXME'.

        switch (cmd) {
        case 0x00: /* ??? */
            ...

This comment survived 12 years because the 0x00 value is indeed
not specified by the CFI open standard (as of this commit).

The 'cmd' field is transfered during migration. To keep the
migration feature working with older QEMU version, we have to
take a lot of care with migrated field. We figured out it is
too late to remove a non-specified value from this model
(this would make migration review very complex). It is however
not too late to improve the documentation.

Add few comments to remember this is a special value related
to QEMU, and we won't find information about it on the CFI
spec.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v6: new patch
---
 hw/block/pflash_cfi01.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index a9529957f8..6838e8a1ab 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -277,9 +277,13 @@ 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;
+        /*
+         * The command 0x00 is not assigned by the CFI open standard,
+         * but QEMU historically uses it for the READ_ARRAY command (0xff).
+         */
+        pfl->cmd = 0x00;
         /* fall through to read code */
-    case 0x00:
+    case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant) */
         /* Flash area read */
         ret = pflash_data_read(pfl, offset, width, be);
         break;
@@ -448,7 +452,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     case 0:
         /* read mode */
         switch (cmd) {
-        case 0x00: /* ??? */
+        case 0x00: /* This model reset value for READ_ARRAY (not CFI) */
             goto reset_flash;
         case 0x10: /* Single Byte Program */
         case 0x40: /* Single Byte Program */
@@ -645,7 +649,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 = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
 }
 
 
@@ -761,7 +765,11 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     pfl->wcycle = 0;
-    pfl->cmd = 0;
+    /*
+     * The command 0x00 is not assigned by the CFI open standard,
+     * but QEMU historically uses it for the READ_ARRAY command (0xff).
+     */
+    pfl->cmd = 0x00;
     pfl->status = 0x80; /* WSM ready */
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v6 3/5] 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 three times:

- On a read access (on invalid command).

  Note this default case is not reachable by the state machine
  updates in pflash_data_write(). However we might reach this
  case migrating from a future QEMU version that would implement
  newer commands, without incrementing the migration version.
  Since we never know, we keep this default case.

  Previous to this patch, an invalid read command would not reset
  the memory region in ROMD mode, so:

  . A further read access would keep going into I/O mode, calling
  the same switch in pflash_read(). Undefined behaviour, probably
  unexpected.
  . A further write access in I/O mode. Since the default case set
  (wcycle=0, cmd=0x00), we jump to reset_flash which set the flash
  in READ_ARRAY.

  After this patch, if we get an invalid read command we directly
  set (wcycle=0, cmd=0x00) and put the device in ROMD mode.
  Further I/O access are now properly handled.

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v6: split of the label rename to ease review
    the pattern is used 3 times (was 2 times previously)
    describe the 3rd time and reset the review tags :(
---
 hw/block/pflash_cfi01.c | 31 +++++++++++++++----------------
 hw/block/trace-events   |  1 +
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6838e8a1ab..a28d0f8cc7 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -112,6 +112,18 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
+static void pflash_mode_read_array(PFlashCFI01 *pfl)
+{
+    trace_pflash_mode_read_array();
+    /*
+     * The command 0x00 is not assigned by the CFI open standard,
+     * but QEMU historically uses it for the READ_ARRAY command (0xff).
+     */
+    pfl->cmd = 0x00;
+    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.
@@ -276,12 +288,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
     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;
-        /*
-         * The command 0x00 is not assigned by the CFI open standard,
-         * but QEMU historically uses it for the READ_ARRAY command (0xff).
-         */
-        pfl->cmd = 0x00;
+        pflash_mode_read_array(pfl);
         /* fall through to read code */
     case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant) */
         /* Flash area read */
@@ -646,10 +653,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                   "\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 = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
+    pflash_mode_read_array(pfl);
 }
 
 
@@ -764,12 +768,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->wcycle = 0;
-    /*
-     * The command 0x00 is not assigned by the CFI open standard,
-     * but QEMU historically uses it for the READ_ARRAY command (0xff).
-     */
-    pfl->cmd = 0x00;
+    pflash_mode_read_array(pfl);
     pfl->status = 0x80; /* WSM ready */
     /* 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-for-4.1 v6 4/5] hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array'

[Philippe Mathieu-Daudé]

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>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v6: split of from previous patch to ease review
    dropped Laszlo's Regression-tested-by tag
---
 hw/block/pflash_cfi01.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index a28d0f8cc7..65afdbf3a7 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -460,7 +460,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
         /* read mode */
         switch (cmd) {
         case 0x00: /* This model reset value for READ_ARRAY (not CFI) */
-            goto reset_flash;
+            goto mode_read_array;
         case 0x10: /* Single Byte Program */
         case 0x40: /* Single Byte Program */
             DPRINTF("%s: Single Byte Program\n", __func__);
@@ -483,7 +483,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;
@@ -508,10 +508,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;
-        case 0xff: /* Read array mode */
+            goto mode_read_array;
+        case 0xff: /* Read Array */
             DPRINTF("%s: Read array mode\n", __func__);
-            goto reset_flash;
+            goto mode_read_array;
         default:
             goto error_flash;
         }
@@ -537,8 +537,8 @@ 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 */
-                goto reset_flash;
+            } else if (cmd == 0xff) { /* Read Array */
+                goto mode_read_array;
             } else
                 goto error_flash;
 
@@ -564,16 +564,16 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             } else if (cmd == 0x01) {
                 pfl->wcycle = 0;
                 pfl->status |= 0x80;
-            } else if (cmd == 0xff) {
-                goto reset_flash;
+            } else if (cmd == 0xff) { /* Read Array */
+                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;
+            if (cmd == 0xff) { /* Read Array */
+                goto mode_read_array;
             } else {
                 DPRINTF("%s: leaving query mode\n", __func__);
             }
@@ -633,7 +633,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:
@@ -643,7 +643,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;
 
@@ -652,7 +652,7 @@ 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:
+ mode_read_array:
     pflash_mode_read_array(pfl);
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v6 5/5] 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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v6: dropped Laszlo's Regression-tested-by tag
---
 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 65afdbf3a7..ee0ed70242 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -768,8 +768,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';
@@ -857,6 +855,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,
@@ -901,6 +911,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-for-4.1 v6 2/5] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00'

[Alistair Francis]

On Tue, Jul 16, 2019 at 3:16 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> The command 0x00 is used by this model since its origin (commit
> 05ee37ebf630). In this commit the command is described with a
> amusing '/* ??? */' comment, probably meaning 'FIXME'.
>
>         switch (cmd) {
>         case 0x00: /* ??? */
>             ...
>
> This comment survived 12 years because the 0x00 value is indeed
> not specified by the CFI open standard (as of this commit).
>
> The 'cmd' field is transfered during migration. To keep the
> migration feature working with older QEMU version, we have to
> take a lot of care with migrated field. We figured out it is
> too late to remove a non-specified value from this model
> (this would make migration review very complex). It is however
> not too late to improve the documentation.
>
> Add few comments to remember this is a special value related
> to QEMU, and we won't find information about it on the CFI
> spec.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

Alistair

> ---
> v6: new patch
> ---
>  hw/block/pflash_cfi01.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index a9529957f8..6838e8a1ab 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -277,9 +277,13 @@ 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;
> +        /*
> +         * The command 0x00 is not assigned by the CFI open standard,
> +         * but QEMU historically uses it for the READ_ARRAY command (0xff).
> +         */
> +        pfl->cmd = 0x00;
>          /* fall through to read code */
> -    case 0x00:
> +    case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant) */
>          /* Flash area read */
>          ret = pflash_data_read(pfl, offset, width, be);
>          break;
> @@ -448,7 +452,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      case 0:
>          /* read mode */
>          switch (cmd) {
> -        case 0x00: /* ??? */
> +        case 0x00: /* This model reset value for READ_ARRAY (not CFI) */
>              goto reset_flash;
>          case 0x10: /* Single Byte Program */
>          case 0x40: /* Single Byte Program */
> @@ -645,7 +649,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 = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
>  }
>
>
> @@ -761,7 +765,11 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      }
>
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    /*
> +     * The command 0x00 is not assigned by the CFI open standard,
> +     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> +     */
> +    pfl->cmd = 0x00;
>      pfl->status = 0x80; /* WSM ready */
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> --
> 2.20.1
>
>

Re: [Qemu-devel] [PATCH-for-4.1 v6 3/5] hw/block/pflash_cfi01: Extract pflash_mode_read_array()

[Alistair Francis]

On Tue, Jul 16, 2019 at 3:16 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> 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 three times:
>
> - On a read access (on invalid command).
>
>   Note this default case is not reachable by the state machine
>   updates in pflash_data_write(). However we might reach this
>   case migrating from a future QEMU version that would implement
>   newer commands, without incrementing the migration version.
>   Since we never know, we keep this default case.
>
>   Previous to this patch, an invalid read command would not reset
>   the memory region in ROMD mode, so:
>
>   . A further read access would keep going into I/O mode, calling
>   the same switch in pflash_read(). Undefined behaviour, probably
>   unexpected.
>   . A further write access in I/O mode. Since the default case set
>   (wcycle=0, cmd=0x00), we jump to reset_flash which set the flash
>   in READ_ARRAY.
>
>   After this patch, if we get an invalid read command we directly
>   set (wcycle=0, cmd=0x00) and put the device in ROMD mode.
>   Further I/O access are now properly handled.
>
> - 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.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

Alistair

> ---
> v6: split of the label rename to ease review
>     the pattern is used 3 times (was 2 times previously)
>     describe the 3rd time and reset the review tags :(
> ---
>  hw/block/pflash_cfi01.c | 31 +++++++++++++++----------------
>  hw/block/trace-events   |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6838e8a1ab..a28d0f8cc7 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -112,6 +112,18 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>
> +static void pflash_mode_read_array(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_mode_read_array();
> +    /*
> +     * The command 0x00 is not assigned by the CFI open standard,
> +     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> +     */
> +    pfl->cmd = 0x00;
> +    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.
> @@ -276,12 +288,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>      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;
> -        /*
> -         * The command 0x00 is not assigned by the CFI open standard,
> -         * but QEMU historically uses it for the READ_ARRAY command (0xff).
> -         */
> -        pfl->cmd = 0x00;
> +        pflash_mode_read_array(pfl);
>          /* fall through to read code */
>      case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant) */
>          /* Flash area read */
> @@ -646,10 +653,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                    "\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 = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
> +    pflash_mode_read_array(pfl);
>  }
>
>
> @@ -764,12 +768,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>
> -    pfl->wcycle = 0;
> -    /*
> -     * The command 0x00 is not assigned by the CFI open standard,
> -     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> -     */
> -    pfl->cmd = 0x00;
> +    pflash_mode_read_array(pfl);
>      pfl->status = 0x80; /* WSM ready */
>      /* 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
>
>

Re: [Qemu-devel] [PATCH-for-4.1 v6 5/5] hw/block/pflash_cfi01: Add the DeviceReset() handler

[Philippe Mathieu-Daudé]

On 7/17/19 12:15 AM, Philippe Mathieu-Daudé 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

"bug 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>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v6: dropped Laszlo's Regression-tested-by tag
> ---
>  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 65afdbf3a7..ee0ed70242 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -768,8 +768,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';
> @@ -857,6 +855,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,
> @@ -901,6 +911,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;
> 

Re: [Qemu-devel] [PATCH-for-4.1 v6 0/5] hw/block/pflash_cfi01: Add DeviceReset() handler

[Laszlo Ersek]

Hi Phil,

On 07/17/19 00:15, Philippe Mathieu-Daudé wrote:
> Hello it's me again, insisting with this series because there are at
> least 2 different report of guests bricked on reset due to the bug
> fixed by patch #5:
> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
> https://bugzilla.redhat.com/show_bug.cgi?id=1704584
> 
> Patches missing review: 2 and 3
> 
> The pflash device lacks a reset() function.
> When a machine is resetted, the flash might be in an
> inconsistent state, leading to unexpected behavior.
> Resolve this issue by adding a DeviceReset() handler.
> 
> Since v1: https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg00962.html
> - addressed Laszlo review comments
> 
> Since v2: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00395.html
> - consider migration (Laszlo, Peter)
> 
> Since v3: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01668.html
> - more reliable migration (Dave)
> - dropped patches 6-9 not required for next release
> 
> Since v4: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg02785.html
> - document why using READ_ARRAY value 0x00 for migration is safe
> 
> Since v5: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03366.html
> - avoid trying to be spec-compliant and messing with migration. KISS.
>   review/test tags reset, sorry.

I have a number of questions / requests.


(1) The last I recall from the v5 discussion is Markus asking about some
risky cases (corner cases?) related to migration.

So what is the new avenue taken in v6? What does "continue ignoring spec
compliance" mean, with regard to migration?

My vague understanding is that we're not trying to answer Markus's
questions; instead, we're side-stepping them, by doing something else.
That works for me, but can we please summarize in a bit more detail?
Like, "in v6, we're not mapping 0x00 vs. 0xff across migration because..."

Yes, I could inter-diff v5 vs. v6, but I know way too little about
pflash. I'd miss how our *dropping* of the special 0xff mapping impacts
our conformance to the data sheet.

I'm not asking for commit message updates, just a bit more explanation
in this free-form discussion. (I looked for it under v5, and couldn't
find it.)


(2) Has someone regression-tested v6 for migration specifically?

Or, is v6 not "risky" wrt. migration any longer?


(3) I'm fine regression testing v6 too (without migration, again).
Please ping me separately once the reviews have converged and the series
is otherwise ready for merging.

Thanks!
Laszlo


> $ git backport-diff -u v5
> 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/5:[----] [-C] 'hw/block/pflash_cfi01: Removed an unused timer'
> 002/5:[down] 'hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00''
> 003/5:[0031] [FC] 'hw/block/pflash_cfi01: Extract pflash_mode_read_array()'
> 004/5:[down] 'hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array''
> 005/5:[----] [--] 'hw/block/pflash_cfi01: Add the DeviceReset() handler'
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (5):
>   hw/block/pflash_cfi01: Removed an unused timer
>   hw/block/pflash_cfi01: Document use of non-CFI compliant command
>     '0x00'
>   hw/block/pflash_cfi01: Extract pflash_mode_read_array()
>   hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array'
>   hw/block/pflash_cfi01: Add the DeviceReset() handler
> 
>  hw/block/pflash_cfi01.c | 77 +++++++++++++++++++++--------------------
>  hw/block/trace-events   |  1 +
>  2 files changed, 41 insertions(+), 37 deletions(-)
> 

Re: [Qemu-devel] [PATCH-for-4.1 v6 0/5] hw/block/pflash_cfi01: Add DeviceReset() handler

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 7/17/19 2:24 PM, Laszlo Ersek wrote:
> Hi Phil,
> 
> On 07/17/19 00:15, Philippe Mathieu-Daudé wrote:
>> Hello it's me again, insisting with this series because there are at
>> least 2 different report of guests bricked on reset due to the bug
>> fixed by patch #5:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>> https://bugzilla.redhat.com/show_bug.cgi?id=1704584
>>
>> Patches missing review: 2 and 3
>>
>> The pflash device lacks a reset() function.
>> When a machine is resetted, the flash might be in an
>> inconsistent state, leading to unexpected behavior.
>> Resolve this issue by adding a DeviceReset() handler.
>>
>> Since v1: https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg00962.html
>> - addressed Laszlo review comments
>>
>> Since v2: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00395.html
>> - consider migration (Laszlo, Peter)
>>
>> Since v3: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01668.html
>> - more reliable migration (Dave)
>> - dropped patches 6-9 not required for next release
>>
>> Since v4: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg02785.html
>> - document why using READ_ARRAY value 0x00 for migration is safe
>>
>> Since v5: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03366.html
>> - avoid trying to be spec-compliant and messing with migration. KISS.
>>   review/test tags reset, sorry.
> 
> I have a number of questions / requests.
> 
> 
> (1) The last I recall from the v5 discussion is Markus asking about some
> risky cases (corner cases?) related to migration.
> 
> So what is the new avenue taken in v6? What does "continue ignoring spec
> compliance" mean, with regard to migration?
> 
> My vague understanding is that we're not trying to answer Markus's
> questions; instead, we're side-stepping them, by doing something else.

I'd love to keep the v5 series and address Markus issues, but as shown
by the quantity of respins, I don't understand the migration feature
enough to fix it in time for the next release. I plan to address them
(and other issues reported by Markus in other reviews) during the next
dev cycle.

> That works for me, but can we please summarize in a bit more detail?
> Like, "in v6, we're not mapping 0x00 vs. 0xff across migration because..."

Since it is very late in the release process and this series intend to
fix a guest corruption bug worthwhile for release, the approch taken by
v6 is "try to change the strict minimum regarding to migration, do not
worry about spec issues". I even tried to make no migration change at
all, but as explained in patch 6 "Extract pflash_mode_read_array" there
is still one.

I could make no migration change, and in patch 6 not call
memory_region_rom_device_set_romd() in pflash_mode_read_array() [and
call it in other places instead], but then we still have the undefined
behavior described in the patch. We always lived with this UNDEF, so...
I could work on a simpler v7.

> Yes, I could inter-diff v5 vs. v6, but I know way too little about
> pflash. I'd miss how our *dropping* of the special 0xff mapping impacts
> our conformance to the data sheet.

To reassure you, it never worked well but nobody cared, I noticed while
converting DPRINTF to trace-events and adding more, then follow the
model state machine. While it's probably not worth fixing, it makes
debugging slighly harder when looking at the CFI spec workflow. Now I'm
used to it.

> I'm not asking for commit message updates, just a bit more explanation
> in this free-form discussion. (I looked for it under v5, and couldn't
> find it.)

I tried to be verbose in the patch description, so for reference:

0xff on v5:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03368.html

not 0xff on v6:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03931.html

migration impact on v6 [1]:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03932.html

> (2) Has someone regression-tested v6 for migration specifically?

No, neither were tested the previous versions.

> 
> Or, is v6 not "risky" wrt. migration any longer?

v6 should be way less risky than previous versions (still one issue
noted in [1]).

> (3) I'm fine regression testing v6 too (without migration, again).
> Please ping me separately once the reviews have converged and the series
> is otherwise ready for merging.

Yes, I know your testing takes very long, so let's wait first.

Thanks for having a look.

Phil.

> 
> Thanks!
> Laszlo
> 
> 
>> $ git backport-diff -u v5
>> 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/5:[----] [-C] 'hw/block/pflash_cfi01: Removed an unused timer'
>> 002/5:[down] 'hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00''
>> 003/5:[0031] [FC] 'hw/block/pflash_cfi01: Extract pflash_mode_read_array()'
>> 004/5:[down] 'hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array''
>> 005/5:[----] [--] 'hw/block/pflash_cfi01: Add the DeviceReset() handler'
>>
>> Regards,
>>
>> Phil.
>>
>> Philippe Mathieu-Daudé (5):
>>   hw/block/pflash_cfi01: Removed an unused timer
>>   hw/block/pflash_cfi01: Document use of non-CFI compliant command
>>     '0x00'
>>   hw/block/pflash_cfi01: Extract pflash_mode_read_array()
>>   hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array'
>>   hw/block/pflash_cfi01: Add the DeviceReset() handler
>>
>>  hw/block/pflash_cfi01.c | 77 +++++++++++++++++++++--------------------
>>  hw/block/trace-events   |  1 +
>>  2 files changed, 41 insertions(+), 37 deletions(-)
>>
>