[Qemu-devel] [PATCH 0/5] hw/block/pflash: Add DeviceReset() handlers

[Qemu-devel] [PATCH 0/5] hw/block/pflash: Add DeviceReset() handlers

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

Both CFI01/CFI02 devices are fixed by this series.

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  hw/block/pflash_cfi01: Removed an unused timer
  hw/block/pflash_cfi01: Extract the pflash_reset() code
  hw/block/pflash_cfi01: Add the DeviceReset() handler
  hw/block/pflash_cfi02: Extract the pflash_reset() code
  hw/block/pflash_cfi02: Add the DeviceReset() handler

 hw/block/pflash_cfi01.c | 31 ++++++++++++-------------------
 hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 29 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 0/5] hw/block/pflash: Add DeviceReset() handlers

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

Both CFI01/CFI02 devices are fixed by this series.

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  hw/block/pflash_cfi01: Removed an unused timer
  hw/block/pflash_cfi01: Extract the pflash_reset() code
  hw/block/pflash_cfi01: Add the DeviceReset() handler
  hw/block/pflash_cfi02: Extract the pflash_reset() code
  hw/block/pflash_cfi02: Add the DeviceReset() handler

 hw/block/pflash_cfi01.c | 31 ++++++++++++-------------------
 hw/block/pflash_cfi02.c | 31 +++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 29 deletions(-)

-- 
2.20.1

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

[Philippe Mathieu-Daudé]

The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
timing modelled. One year later, the CFI02 model was introduced
(commit 05ee37ebf630) based on the CFI01 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>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Yes, I plan to model those timings later. Actually I have a series
working, but I'd rather first
1/ refactor common code between the both CFI implementations,
2/ discuss on list whether or not use timings for the Virt flash.
---
 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 16dfae14b80..6dc04f156a7 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/host-utils.h"
 #include "qemu/log.h"
@@ -86,7 +85,6 @@ struct PFlashCFI01 {
     uint8_t cfi_table[0x52];
     uint64_t counter;
     unsigned int writeblock_size;
-    QEMUTimer *timer;
     MemoryRegion mem;
     char *name;
     void *storage;
@@ -110,18 +108,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.
@@ -771,7 +757,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 1/5] hw/block/pflash_cfi01: Removed an unused timer

[Philippe Mathieu-Daudé]

The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
timing modelled. One year later, the CFI02 model was introduced
(commit 05ee37ebf630) based on the CFI01 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>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Yes, I plan to model those timings later. Actually I have a series
working, but I'd rather first
1/ refactor common code between the both CFI implementations,
2/ discuss on list whether or not use timings for the Virt flash.
---
 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 16dfae14b80..6dc04f156a7 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/host-utils.h"
 #include "qemu/log.h"
@@ -86,7 +85,6 @@ struct PFlashCFI01 {
     uint8_t cfi_table[0x52];
     uint64_t counter;
     unsigned int writeblock_size;
-    QEMUTimer *timer;
     MemoryRegion mem;
     char *name;
     void *storage;
@@ -110,18 +108,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.
@@ -771,7 +757,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 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

[Philippe Mathieu-Daudé]

The reset() code is used in various places, refactor it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6dc04f156a7..073cd14978f 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
+static void pflash_reset(PFlashCFI01 *pfl)
+{
+    trace_pflash_reset();
+    pfl->wcycle = 0;
+    pfl->cmd = 0;
+    pfl->status = 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.
@@ -275,8 +284,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;
-        pfl->cmd = 0;
+        pflash_reset(pfl);
         /* fall through to read code */
     case 0x00:
         /* Flash area read */
@@ -639,10 +647,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 = 0;
+    pflash_reset(pfl);
 }
 
 
@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
-    pfl->status = 0;
+    pflash_reset(pfl);
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

[Qemu-devel] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

[Philippe Mathieu-Daudé]

The reset() code is used in various places, refactor it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6dc04f156a7..073cd14978f 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
     }
 };
 
+static void pflash_reset(PFlashCFI01 *pfl)
+{
+    trace_pflash_reset();
+    pfl->wcycle = 0;
+    pfl->cmd = 0;
+    pfl->status = 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.
@@ -275,8 +284,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;
-        pfl->cmd = 0;
+        pflash_reset(pfl);
         /* fall through to read code */
     case 0x00:
         /* Flash area read */
@@ -639,10 +647,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 = 0;
+    pflash_reset(pfl);
 }
 
 
@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
-    pfl->status = 0;
+    pflash_reset(pfl);
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

The pflash device is a child of TYPE_DEVICE, so it can implement
the DeviceReset handler. Actually it has to implement it, else
on machine reset it might stay in an incoherent state, as it has
been reported in the buglink listed below.

Add the DeviceReset handler and remove its call from the realize()
function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 073cd14978f..639b05bc4d5 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -762,7 +762,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pflash_reset(pfl);
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
@@ -850,6 +849,11 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
 }
 
+static void pflash_cfi01_reset(DeviceState *dev)
+{
+    pflash_reset(PFLASH_CFI01(dev));
+}
+
 static Property pflash_cfi01_properties[] = {
     DEFINE_PROP_DRIVE("drive", PFlashCFI01, blk),
     /* num-blocks is the number of blocks actually visible to the guest,
@@ -894,6 +898,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = pflash_cfi01_reset;
     dc->realize = pflash_cfi01_realize;
     dc->props = pflash_cfi01_properties;
     dc->vmsd = &vmstate_pflash;
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

The pflash device is a child of TYPE_DEVICE, so it can implement
the DeviceReset handler. Actually it has to implement it, else
on machine reset it might stay in an incoherent state, as it has
been reported in the buglink listed below.

Add the DeviceReset handler and remove its call from the realize()
function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 073cd14978f..639b05bc4d5 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -762,7 +762,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
         pfl->max_device_width = pfl->device_width;
     }
 
-    pflash_reset(pfl);
     /* Hardcoded CFI table */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
@@ -850,6 +849,11 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
 }
 
+static void pflash_cfi01_reset(DeviceState *dev)
+{
+    pflash_reset(PFLASH_CFI01(dev));
+}
+
 static Property pflash_cfi01_properties[] = {
     DEFINE_PROP_DRIVE("drive", PFlashCFI01, blk),
     /* num-blocks is the number of blocks actually visible to the guest,
@@ -894,6 +898,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = pflash_cfi01_reset;
     dc->realize = pflash_cfi01_realize;
     dc->props = pflash_cfi01_properties;
     dc->vmsd = &vmstate_pflash;
-- 
2.20.1

[Qemu-devel] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code

[Philippe Mathieu-Daudé]

The reset() code is used in various places, refactor it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi02.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f813..f321b74433c 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
     pfl->rom_mode = rom_mode;
 }
 
+static void pflash_reset(PFlashCFI02 *pfl)
+{
+    trace_pflash_reset();
+    timer_del(&pfl->timer);
+    pfl->bypass = 0;
+    pfl->wcycle = 0;
+    pfl->cmd = 0;
+    pfl->status = 0;
+    pflash_register_memory(pfl, 1);
+}
+
 static void pflash_timer (void *opaque)
 {
     PFlashCFI02 *pfl = opaque;
@@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
     pfl->status ^= 0x80;
     if (pfl->bypass) {
         pfl->wcycle = 2;
+        pfl->cmd = 0;
     } else {
-        pflash_register_memory(pfl, 1);
-        pfl->wcycle = 0;
+        pflash_reset(pfl);
     }
-    pfl->cmd = 0;
 }
 
 static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
@@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 
     /* Reset flash */
  reset_flash:
-    trace_pflash_reset();
-    pfl->bypass = 0;
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pflash_reset(pfl);
     return;
 
  do_bypass:
@@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
     timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
-    pfl->status = 0;
+    pflash_reset(pfl);
     /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

[Qemu-devel] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code

[Philippe Mathieu-Daudé]

The reset() code is used in various places, refactor it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi02.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f813..f321b74433c 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
     pfl->rom_mode = rom_mode;
 }
 
+static void pflash_reset(PFlashCFI02 *pfl)
+{
+    trace_pflash_reset();
+    timer_del(&pfl->timer);
+    pfl->bypass = 0;
+    pfl->wcycle = 0;
+    pfl->cmd = 0;
+    pfl->status = 0;
+    pflash_register_memory(pfl, 1);
+}
+
 static void pflash_timer (void *opaque)
 {
     PFlashCFI02 *pfl = opaque;
@@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
     pfl->status ^= 0x80;
     if (pfl->bypass) {
         pfl->wcycle = 2;
+        pfl->cmd = 0;
     } else {
-        pflash_register_memory(pfl, 1);
-        pfl->wcycle = 0;
+        pflash_reset(pfl);
     }
-    pfl->cmd = 0;
 }
 
 static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
@@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
 
     /* Reset flash */
  reset_flash:
-    trace_pflash_reset();
-    pfl->bypass = 0;
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
+    pflash_reset(pfl);
     return;
 
  do_bypass:
@@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
     timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
-    pfl->wcycle = 0;
-    pfl->cmd = 0;
-    pfl->status = 0;
+    pflash_reset(pfl);
     /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
     /* Standard "QRY" string */
     pfl->cfi_table[0x10] = 'Q';
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

The pflash device is a child of TYPE_DEVICE, so it can implement
the DeviceReset handler. Actually it has to implement it, else
on machine reset it might stay in an incoherent state, as it has
been reported in the buglink listed below.

Add the DeviceReset handler and remove its call from the realize()
function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi02.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f321b74433c..5af367d1563 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -674,6 +674,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x3c] = 0x00;
 }
 
+static void pflash_cfi02_reset(DeviceState *dev)
+{
+    pflash_reset(PFLASH_CFI02(dev));
+}
+
 static Property pflash_cfi02_properties[] = {
     DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
     DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
@@ -701,6 +706,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = pflash_cfi02_reset;
     dc->realize = pflash_cfi02_realize;
     dc->unrealize = pflash_cfi02_unrealize;
     dc->props = pflash_cfi02_properties;
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

The pflash device is a child of TYPE_DEVICE, so it can implement
the DeviceReset handler. Actually it has to implement it, else
on machine reset it might stay in an incoherent state, as it has
been reported in the buglink listed below.

Add the DeviceReset handler and remove its call from the realize()
function.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi02.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f321b74433c..5af367d1563 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -674,6 +674,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->cfi_table[0x3c] = 0x00;
 }
 
+static void pflash_cfi02_reset(DeviceState *dev)
+{
+    pflash_reset(PFLASH_CFI02(dev));
+}
+
 static Property pflash_cfi02_properties[] = {
     DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
     DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
@@ -701,6 +706,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->reset = pflash_cfi02_reset;
     dc->realize = pflash_cfi02_realize;
     dc->unrealize = pflash_cfi02_unrealize;
     dc->props = pflash_cfi02_properties;
-- 
2.20.1

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

[Wei Yang]

On Sun, May 05, 2019 at 10:05:58PM +0200, Philippe Mathieu-Daudé wrote:
>The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
>timing modelled. One year later, the CFI02 model was introduced
>(commit 05ee37ebf630) based on the CFI01 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>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

>---
>Yes, I plan to model those timings later. Actually I have a series
>working, but I'd rather first
>1/ refactor common code between the both CFI implementations,
>2/ discuss on list whether or not use timings for the Virt flash.
>---
> 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 16dfae14b80..6dc04f156a7 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/host-utils.h"
> #include "qemu/log.h"
>@@ -86,7 +85,6 @@ struct PFlashCFI01 {
>     uint8_t cfi_table[0x52];
>     uint64_t counter;
>     unsigned int writeblock_size;
>-    QEMUTimer *timer;
>     MemoryRegion mem;
>     char *name;
>     void *storage;
>@@ -110,18 +108,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.
>@@ -771,7 +757,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

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

[Wei Yang]

On Sun, May 05, 2019 at 10:05:59PM +0200, Philippe Mathieu-Daudé wrote:
>The reset() code is used in various places, refactor it.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

>---
> hw/block/pflash_cfi01.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
>diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>index 6dc04f156a7..073cd14978f 100644
>--- a/hw/block/pflash_cfi01.c
>+++ b/hw/block/pflash_cfi01.c
>@@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
>     }
> };
> 
>+static void pflash_reset(PFlashCFI01 *pfl)
>+{
>+    trace_pflash_reset();
>+    pfl->wcycle = 0;
>+    pfl->cmd = 0;
>+    pfl->status = 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.
>@@ -275,8 +284,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;
>-        pfl->cmd = 0;
>+        pflash_reset(pfl);
>         /* fall through to read code */
>     case 0x00:
>         /* Flash area read */
>@@ -639,10 +647,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 = 0;
>+    pflash_reset(pfl);
> }
> 
> 
>@@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>         pfl->max_device_width = pfl->device_width;
>     }
> 
>-    pfl->wcycle = 0;
>-    pfl->cmd = 0;
>-    pfl->status = 0;
>+    pflash_reset(pfl);
>     /* Hardcoded CFI table */
>     /* Standard "QRY" string */
>     pfl->cfi_table[0x10] = 'Q';
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Sun, May 05, 2019 at 10:06:00PM +0200, Philippe Mathieu-Daudé wrote:
>The pflash device is a child of TYPE_DEVICE, so it can implement
>the DeviceReset handler. Actually it has to implement it, else
>on machine reset it might stay in an incoherent state, as it has
>been reported in the buglink listed below.
>
>Add the DeviceReset handler and remove its call from the realize()
>function.
>
>Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>Reported-by: Laszlo Ersek <lersek@redhat.com>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>


-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code

[Wei Yang]

On Sun, May 05, 2019 at 10:06:01PM +0200, Philippe Mathieu-Daudé wrote:
>The reset() code is used in various places, refactor it.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

>---
> hw/block/pflash_cfi02.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
>diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>index f2c6201f813..f321b74433c 100644
>--- a/hw/block/pflash_cfi02.c
>+++ b/hw/block/pflash_cfi02.c
>@@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
>     pfl->rom_mode = rom_mode;
> }
> 
>+static void pflash_reset(PFlashCFI02 *pfl)
>+{
>+    trace_pflash_reset();
>+    timer_del(&pfl->timer);
>+    pfl->bypass = 0;
>+    pfl->wcycle = 0;
>+    pfl->cmd = 0;
>+    pfl->status = 0;
>+    pflash_register_memory(pfl, 1);
>+}
>+
> static void pflash_timer (void *opaque)
> {
>     PFlashCFI02 *pfl = opaque;
>@@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
>     pfl->status ^= 0x80;
>     if (pfl->bypass) {
>         pfl->wcycle = 2;
>+        pfl->cmd = 0;
>     } else {
>-        pflash_register_memory(pfl, 1);
>-        pfl->wcycle = 0;
>+        pflash_reset(pfl);
>     }
>-    pfl->cmd = 0;
> }
> 
> static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
>@@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
> 
>     /* Reset flash */
>  reset_flash:
>-    trace_pflash_reset();
>-    pfl->bypass = 0;
>-    pfl->wcycle = 0;
>-    pfl->cmd = 0;
>+    pflash_reset(pfl);
>     return;
> 
>  do_bypass:
>@@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> 
>     timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>-    pfl->wcycle = 0;
>-    pfl->cmd = 0;
>-    pfl->status = 0;
>+    pflash_reset(pfl);
>     /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
>     /* Standard "QRY" string */
>     pfl->cfi_table[0x10] = 'Q';
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Sun, May 05, 2019 at 10:06:02PM +0200, Philippe Mathieu-Daudé wrote:
>The pflash device is a child of TYPE_DEVICE, so it can implement
>the DeviceReset handler. Actually it has to implement it, else
>on machine reset it might stay in an incoherent state, as it has
>been reported in the buglink listed below.
>
>Add the DeviceReset handler and remove its call from the realize()
>function.
>
>Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>Reported-by: Laszlo Ersek <lersek@redhat.com>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>


-- 
Wei Yang
Help you, Help me

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

[Laszlo Ersek]

On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
> The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
> timing modelled. One year later, the CFI02 model was introduced
> (commit 05ee37ebf630) based on the CFI01 model. As noted in the

You got those commit references backwards, I believe:

* Commit 29133e9a0fff ("AMD NOR flash device support (initial patch by
Jocelyn Mayer)", 2006-06-25) introduced "hw/pflash_cfi02.c".

* Commit 05ee37ebf630 ("Gumstix 'connex' board support by Thorsten
Zitterell.", 2007-11-17) introduced "hw/pflash_cfi01.c".

> 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>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Yes, I plan to model those timings later. Actually I have a series
> working, but I'd rather first
> 1/ refactor common code between the both CFI implementations,

Good idea.

> 2/ discuss on list whether or not use timings for the Virt flash.

What would the timer buy us (specifically wrt. cfi01 / OVMF / ArmVirt)?

Being faithful to actual hardware is always good... except when it runs
a significant risk of regressions. :) By that I don't mean "programming
errors"; I mean that guest code would now have to conform to various
timeouts, and that always makes me a bit concerned.


For this patch, with the commit references fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
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 16dfae14b80..6dc04f156a7 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/host-utils.h"
>  #include "qemu/log.h"
> @@ -86,7 +85,6 @@ struct PFlashCFI01 {
>      uint8_t cfi_table[0x52];
>      uint64_t counter;
>      unsigned int writeblock_size;
> -    QEMUTimer *timer;
>      MemoryRegion mem;
>      char *name;
>      void *storage;
> @@ -110,18 +108,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.
> @@ -771,7 +757,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;
> 

Re: [Qemu-devel] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

[Laszlo Ersek]

On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
> The reset() code is used in various places, refactor it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6dc04f156a7..073cd14978f 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> +static void pflash_reset(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_reset();
> +    pfl->wcycle = 0;
> +    pfl->cmd = 0;
> +    pfl->status = 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.
> @@ -275,8 +284,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;
> -        pfl->cmd = 0;
> +        pflash_reset(pfl);
>          /* fall through to read code */
>      case 0x00:
>          /* Flash area read */

This change introduces two new actions:
- zeroing "status"
- flipping the chip to romd mode (unless it's already in romd mode)

> @@ -639,10 +647,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 = 0;
> +    pflash_reset(pfl);
>  }

This change introduces a new action:
- zeroing "status"

>  
>  
> @@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> -    pfl->status = 0;
> +    pflash_reset(pfl);

This change introduces a new action:
- flipping the chip to romd mode (unless it's already in romd mode)

>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> 

Therefore, this patch is not *obviously* a refactoring patch. (IOW, it
may be a pure refactoring patch, but it's not very easy to see.) I suggest:

- either documenting the new actions in the commit message (and why they
are justified),

- or prepending a separate patch that first unifies the behavior in all
the separate reset locations, and then this patch could indeed become an
"obvious" refactoring / code exctraction.

(Clearly, I'm asking for this with an eye towards git-bisect.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 2/5] hw/block/pflash_cfi01: Extract the pflash_reset() code

[Laszlo Ersek]

On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
> The reset() code is used in various places, refactor it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

BTW: you didn't add an explicit "Cc: stable" tag here, so I almost
missed it -- but, mainly, is this really suitable material for stable?
We haven't really noticed (or at least pin-pointed) the lack of the
reset handlers in 12 years, correct?

Thanks
Laszlo

> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6dc04f156a7..073cd14978f 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -108,6 +108,15 @@ static const VMStateDescription vmstate_pflash = {
>      }
>  };
>  
> +static void pflash_reset(PFlashCFI01 *pfl)
> +{
> +    trace_pflash_reset();
> +    pfl->wcycle = 0;
> +    pfl->cmd = 0;
> +    pfl->status = 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.
> @@ -275,8 +284,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;
> -        pfl->cmd = 0;
> +        pflash_reset(pfl);
>          /* fall through to read code */
>      case 0x00:
>          /* Flash area read */
> @@ -639,10 +647,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 = 0;
> +    pflash_reset(pfl);
>  }
>  
>  
> @@ -757,9 +762,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> -    pfl->status = 0;
> +    pflash_reset(pfl);
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> 

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

[Laszlo Ersek]

On 05/05/19 22:06, Philippe Mathieu-Daudé wrote:
> The pflash device is a child of TYPE_DEVICE, so it can implement
> the DeviceReset handler. Actually it has to implement it, else
> on machine reset it might stay in an incoherent state, as it has
> been reported in the buglink listed below.
> 
> Add the DeviceReset handler and remove its call from the realize()
> function.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 073cd14978f..639b05bc4d5 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -762,7 +762,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>          pfl->max_device_width = pfl->device_width;
>      }
>  
> -    pflash_reset(pfl);
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> @@ -850,6 +849,11 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
>  }
>  
> +static void pflash_cfi01_reset(DeviceState *dev)
> +{
> +    pflash_reset(PFLASH_CFI01(dev));
> +}
> +
>  static Property pflash_cfi01_properties[] = {
>      DEFINE_PROP_DRIVE("drive", PFlashCFI01, blk),
>      /* num-blocks is the number of blocks actually visible to the guest,
> @@ -894,6 +898,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> +    dc->reset = pflash_cfi01_reset;
>      dc->realize = pflash_cfi01_realize;
>      dc->props = pflash_cfi01_properties;
>      dc->vmsd = &vmstate_pflash;
> 

(Hopefully I'm not missing anything obvious, while writing the below:)

Sometimes the object model is really nice.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code

[Laszlo Ersek]

On 05/05/19 22:06, Philippe Mathieu-Daudé wrote:
> The reset() code is used in various places, refactor it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index f2c6201f813..f321b74433c 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
>      pfl->rom_mode = rom_mode;
>  }
>  
> +static void pflash_reset(PFlashCFI02 *pfl)
> +{
> +    trace_pflash_reset();
> +    timer_del(&pfl->timer);
> +    pfl->bypass = 0;
> +    pfl->wcycle = 0;
> +    pfl->cmd = 0;
> +    pfl->status = 0;
> +    pflash_register_memory(pfl, 1);
> +}
> +
>  static void pflash_timer (void *opaque)
>  {
>      PFlashCFI02 *pfl = opaque;
> @@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
>      pfl->status ^= 0x80;
>      if (pfl->bypass) {
>          pfl->wcycle = 2;
> +        pfl->cmd = 0;
>      } else {
> -        pflash_register_memory(pfl, 1);
> -        pfl->wcycle = 0;
> +        pflash_reset(pfl);
>      }
> -    pfl->cmd = 0;
>  }
>  
>  static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
> @@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>  
>      /* Reset flash */
>   reset_flash:
> -    trace_pflash_reset();
> -    pfl->bypass = 0;
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pflash_reset(pfl);
>      return;
>  
>   do_bypass:
> @@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
>  
>      timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> -    pfl->wcycle = 0;
> -    pfl->cmd = 0;
> -    pfl->status = 0;
> +    pflash_reset(pfl);
>      /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
> 

I don't have a vested interest in the pflash_cfi02 device model, but I
guess my earlier (cfi01) comments would apply -- unify first, extract
second. (Or at least document why these changes are unobservable from
the behavior POV.)

Thanks
Laszlo

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

[Philippe Mathieu-Daudé]

On 5/6/19 4:39 PM, Laszlo Ersek wrote:
> On 05/05/19 22:05, Philippe Mathieu-Daudé wrote:
>> The 'CFI01' NOR flash was introduced in commit 29133e9a0fff, with
>> timing modelled. One year later, the CFI02 model was introduced
>> (commit 05ee37ebf630) based on the CFI01 model. As noted in the
> 
> You got those commit references backwards, I believe:
> 
> * Commit 29133e9a0fff ("AMD NOR flash device support (initial patch by
> Jocelyn Mayer)", 2006-06-25) introduced "hw/pflash_cfi02.c".
> 
> * Commit 05ee37ebf630 ("Gumstix 'connex' board support by Thorsten
> Zitterell.", 2007-11-17) introduced "hw/pflash_cfi01.c".

Argh yes, thank you!

>> 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>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Yes, I plan to model those timings later. Actually I have a series
>> working, but I'd rather first
>> 1/ refactor common code between the both CFI implementations,
> 
> Good idea.
> 
>> 2/ discuss on list whether or not use timings for the Virt flash.
> 
> What would the timer buy us (specifically wrt. cfi01 / OVMF / ArmVirt)?
> 
> Being faithful to actual hardware is always good... except when it runs
> a significant risk of regressions. :) By that I don't mean "programming
> errors"; I mean that guest code would now have to conform to various
> timeouts, and that always makes me a bit concerned.

I'm glat you feel concerned :)
My goal is to model enough of the device to be able to run 'Capsule
Based Firmware Updates' [*], but I haven't investigated much yet.
Embedded firmware usually care about such timings. Anyway if this is
implemented as a feature, it would be disabled by default for the Virt
flash (I name the Virt flash the one used by the Virt X86/Aarch64 machines).

> 
> For this patch, with the commit references fixed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Will do, thanks!

> 
> Thanks,
> 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 16dfae14b80..6dc04f156a7 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/host-utils.h"
>>  #include "qemu/log.h"
>> @@ -86,7 +85,6 @@ struct PFlashCFI01 {
>>      uint8_t cfi_table[0x52];
>>      uint64_t counter;
>>      unsigned int writeblock_size;
>> -    QEMUTimer *timer;
>>      MemoryRegion mem;
>>      char *name;
>>      void *storage;
>> @@ -110,18 +108,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.
>> @@ -771,7 +757,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;
>>
> 

Re: [Qemu-devel] [PATCH 4/5] hw/block/pflash_cfi02: Extract the pflash_reset() code

[Philippe Mathieu-Daudé]

On 5/6/19 4:57 PM, Laszlo Ersek wrote:
> On 05/05/19 22:06, Philippe Mathieu-Daudé wrote:
>> The reset() code is used in various places, refactor it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/pflash_cfi02.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index f2c6201f813..f321b74433c 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -120,6 +120,17 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int rom_mode)
>>      pfl->rom_mode = rom_mode;
>>  }
>>  
>> +static void pflash_reset(PFlashCFI02 *pfl)
>> +{
>> +    trace_pflash_reset();
>> +    timer_del(&pfl->timer);
>> +    pfl->bypass = 0;
>> +    pfl->wcycle = 0;
>> +    pfl->cmd = 0;
>> +    pfl->status = 0;
>> +    pflash_register_memory(pfl, 1);
>> +}
>> +
>>  static void pflash_timer (void *opaque)
>>  {
>>      PFlashCFI02 *pfl = opaque;
>> @@ -129,11 +140,10 @@ static void pflash_timer (void *opaque)
>>      pfl->status ^= 0x80;
>>      if (pfl->bypass) {
>>          pfl->wcycle = 2;
>> +        pfl->cmd = 0;
>>      } else {
>> -        pflash_register_memory(pfl, 1);
>> -        pfl->wcycle = 0;
>> +        pflash_reset(pfl);
>>      }
>> -    pfl->cmd = 0;
>>  }
>>  
>>  static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
>> @@ -481,10 +491,7 @@ static void pflash_write(PFlashCFI02 *pfl, hwaddr offset,
>>  
>>      /* Reset flash */
>>   reset_flash:
>> -    trace_pflash_reset();
>> -    pfl->bypass = 0;
>> -    pfl->wcycle = 0;
>> -    pfl->cmd = 0;
>> +    pflash_reset(pfl);
>>      return;
>>  
>>   do_bypass:
>> @@ -588,9 +595,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
>>  
>>      timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>> -    pfl->wcycle = 0;
>> -    pfl->cmd = 0;
>> -    pfl->status = 0;
>> +    pflash_reset(pfl);
>>      /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
>>      /* Standard "QRY" string */
>>      pfl->cfi_table[0x10] = 'Q';
>>
> 
> I don't have a vested interest in the pflash_cfi02 device model, but I
> guess my earlier (cfi01) comments would apply -- unify first, extract
> second. (Or at least document why these changes are unobservable from
> the behavior POV.)

Ah sorry I forgot to add "Laszlo, please only review patches 1-3" in the
cover :( I use git-publish that send all the patches of the series to
all the list. I might start to add 'Cc:' tags in my patches.

> Thanks
> Laszlo
> 

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

[Laszlo Ersek]

On 05/05/19 22:06, Philippe Mathieu-Daudé wrote:
> The pflash device is a child of TYPE_DEVICE, so it can implement
> the DeviceReset handler. Actually it has to implement it, else
> on machine reset it might stay in an incoherent state, as it has
> been reported in the buglink listed below.
> 
> Add the DeviceReset handler and remove its call from the realize()
> function.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
> Reported-by: Laszlo Ersek <lersek@redhat.com>

- IMO, the above two tags should be dropped from the commit message, as
they are specific to CFI01.

- Additionally, the commit message references the realize() function
(correctly), but the patch doesn't change that function. That is, the
patch doesn't remove the pflash_reset() call from pflash_cfi02_realize()
that was introduced in the last patch.

Thanks
Laszlo

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index f321b74433c..5af367d1563 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -674,6 +674,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->cfi_table[0x3c] = 0x00;
>  }
>  
> +static void pflash_cfi02_reset(DeviceState *dev)
> +{
> +    pflash_reset(PFLASH_CFI02(dev));
> +}
> +
>  static Property pflash_cfi02_properties[] = {
>      DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
>      DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
> @@ -701,6 +706,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> +    dc->reset = pflash_cfi02_reset;
>      dc->realize = pflash_cfi02_realize;
>      dc->unrealize = pflash_cfi02_unrealize;
>      dc->props = pflash_cfi02_properties;
> 

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

[Philippe Mathieu-Daudé]

On 5/6/19 5:17 PM, Laszlo Ersek wrote:
> On 05/05/19 22:06, Philippe Mathieu-Daudé wrote:
>> The pflash device is a child of TYPE_DEVICE, so it can implement
>> the DeviceReset handler. Actually it has to implement it, else
>> on machine reset it might stay in an incoherent state, as it has
>> been reported in the buglink listed below.
>>
>> Add the DeviceReset handler and remove its call from the realize()
>> function.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
> 
> - IMO, the above two tags should be dropped from the commit message, as
> they are specific to CFI01.

OK.

> - Additionally, the commit message references the realize() function
> (correctly), but the patch doesn't change that function. That is, the
> patch doesn't remove the pflash_reset() call from pflash_cfi02_realize()
> that was introduced in the last patch.

You found a bug, having your reviewing CFI02 is useful :)
I had it correct in a staged branch, then messed while rebasing.

Thanks for noticing this,

Phil.

> 
> Thanks
> Laszlo
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/pflash_cfi02.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>> index f321b74433c..5af367d1563 100644
>> --- a/hw/block/pflash_cfi02.c
>> +++ b/hw/block/pflash_cfi02.c
>> @@ -674,6 +674,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>>      pfl->cfi_table[0x3c] = 0x00;
>>  }
>>  
>> +static void pflash_cfi02_reset(DeviceState *dev)
>> +{
>> +    pflash_reset(PFLASH_CFI02(dev));
>> +}
>> +
>>  static Property pflash_cfi02_properties[] = {
>>      DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
>>      DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
>> @@ -701,6 +706,7 @@ static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>> +    dc->reset = pflash_cfi02_reset;
>>      dc->realize = pflash_cfi02_realize;
>>      dc->unrealize = pflash_cfi02_unrealize;
>>      dc->props = pflash_cfi02_properties;
>>
>