QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PULL 0/4] pflash-next patches for 2020-05-22
@ 2020-05-22 17:45 Philippe Mathieu-Daudé
  2020-05-22 17:45 ` [PULL 1/4] hw/block/pflash_cfi01: Removed an unused timer Philippe Mathieu-Daudé
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-22 17:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Philippe Mathieu-Daudé, qemu-block, Max Reitz

The following changes since commit d19f1ab0de8b763159513e3eaa12c5bc68122361:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-2020052=
1-1' into staging (2020-05-21 22:06:56 +0100)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/pflash-next-20200522

for you to fetch changes up to 1857b9db49770590483be44eb90993c42b2a5a99:

  hw/block/pflash: Check return value of blk_pwrite() (2020-05-22 19:38:14 +0=
200)

----------------------------------------------------------------

- Remove unused timer in CFI01 flash,
- Clean up code documentation,
- Silent a long-standing Coverity warning (2016-07-15).

----------------------------------------------------------------

Mansour Ahmadi (1):
  hw/block/pflash: Check return value of blk_pwrite()

Philippe Mathieu-Daud=C3=A9 (3):
  hw/block/pflash_cfi01: Removed an unused timer
  hw/block/pflash_cfi01: Document use of non-CFI compliant command
    '0x00'
  hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array'

 hw/block/pflash_cfi01.c | 71 ++++++++++++++++++++---------------------
 hw/block/pflash_cfi02.c |  8 ++++-
 2 files changed, 42 insertions(+), 37 deletions(-)

--=20
2.21.3



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PULL 1/4] hw/block/pflash_cfi01: Removed an unused timer
  2020-05-22 17:45 [PULL 0/4] pflash-next patches for 2020-05-22 Philippe Mathieu-Daudé
@ 2020-05-22 17:45 ` Philippe Mathieu-Daudé
  2020-05-22 17:45 ` [PULL 2/4] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00' Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-22 17:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Philippe Mathieu-Daudé,
	Max Reitz, Alistair Francis, Wei Yang, Laszlo Ersek

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>
Tested-by: Laszlo Ersek <lersek@redhat.com>
[Laszlo Ersek: Regression tested EDK2 OVMF IA32X64, ArmVirtQemu Aarch64
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg04373.html]
Message-Id: <20190716221555.11145-2-philmd@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 8e8887253d..d67f84d655 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -42,7 +42,6 @@
 #include "hw/qdev-properties.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"
@@ -91,7 +90,6 @@ struct PFlashCFI01 {
     uint8_t cfi_table[0x52];
     uint64_t counter;
     unsigned int writeblock_size;
-    QEMUTimer *timer;
     MemoryRegion mem;
     char *name;
     void *storage;
@@ -115,18 +113,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.
@@ -775,7 +761,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.21.3



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PULL 2/4] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00'
  2020-05-22 17:45 [PULL 0/4] pflash-next patches for 2020-05-22 Philippe Mathieu-Daudé
  2020-05-22 17:45 ` [PULL 1/4] hw/block/pflash_cfi01: Removed an unused timer Philippe Mathieu-Daudé
@ 2020-05-22 17:45 ` Philippe Mathieu-Daudé
  2020-05-22 17:45 ` [PULL 3/4] hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array' Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-22 17:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Alistair Francis, Philippe Mathieu-Daudé,
	qemu-block, Max Reitz

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.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20190716221555.11145-3-philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 d67f84d655..3cd483d26a 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -278,9 +278,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;
@@ -449,7 +453,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 */
@@ -646,7 +650,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) */
 }
 
 
@@ -762,7 +766,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.21.3



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PULL 3/4] hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array'
  2020-05-22 17:45 [PULL 0/4] pflash-next patches for 2020-05-22 Philippe Mathieu-Daudé
  2020-05-22 17:45 ` [PULL 1/4] hw/block/pflash_cfi01: Removed an unused timer Philippe Mathieu-Daudé
  2020-05-22 17:45 ` [PULL 2/4] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00' Philippe Mathieu-Daudé
@ 2020-05-22 17:45 ` Philippe Mathieu-Daudé
  2020-05-22 17:45 ` [PULL 4/4] hw/block/pflash: Check return value of blk_pwrite() Philippe Mathieu-Daudé
  2020-05-24 13:42 ` [PULL 0/4] pflash-next patches for 2020-05-22 Peter Maydell
  4 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-22 17:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Philippe Mathieu-Daudé,
	Max Reitz, Alistair Francis, John Snow

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>
Message-Id: <20190716221555.11145-5-philmd@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 3cd483d26a..2ca173aa46 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -454,7 +454,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__);
@@ -477,7 +477,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
         case 0x50: /* Clear status bits */
             DPRINTF("%s: Clear status bits\n", __func__);
             pfl->status = 0x0;
-            goto reset_flash;
+            goto mode_read_array;
         case 0x60: /* Block (un)lock */
             DPRINTF("%s: Block unlock\n", __func__);
             break;
@@ -502,10 +502,10 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             break;
         case 0xf0: /* Probe for AMD flash */
             DPRINTF("%s: Probe for AMD flash\n", __func__);
-            goto reset_flash;
-        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;
         }
@@ -531,8 +531,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;
 
@@ -558,16 +558,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__);
             }
@@ -627,7 +627,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                     " the data is already written to storage!\n"
                     "Flash device reset into READ mode.\n",
                     __func__);
-                goto reset_flash;
+                goto mode_read_array;
             }
             break;
         default:
@@ -637,7 +637,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
     default:
         /* Should never happen */
         DPRINTF("%s: invalid write state\n",  __func__);
-        goto reset_flash;
+        goto mode_read_array;
     }
     return;
 
@@ -646,7 +646,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:
     trace_pflash_reset();
     memory_region_rom_device_set_romd(&pfl->mem, true);
     pfl->wcycle = 0;
-- 
2.21.3



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PULL 4/4] hw/block/pflash: Check return value of blk_pwrite()
  2020-05-22 17:45 [PULL 0/4] pflash-next patches for 2020-05-22 Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2020-05-22 17:45 ` [PULL 3/4] hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array' Philippe Mathieu-Daudé
@ 2020-05-22 17:45 ` Philippe Mathieu-Daudé
  2020-05-24 13:42 ` [PULL 0/4] pflash-next patches for 2020-05-22 Peter Maydell
  4 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-22 17:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Mansour Ahmadi, Philippe Mathieu-Daudé,
	qemu-block, Max Reitz

From: Mansour Ahmadi <mansourweb@gmail.com>

When updating the PFLASH file contents, we should check for a
possible failure of blk_pwrite(). Similar to commit 3a688294e.

Reported-by: Coverity (CID 1357678 CHECKED_RETURN)
Signed-off-by: Mansour Ahmadi <mansourweb@gmail.com>
Message-Id: <20200408003552.58095-1-mansourweb@gmail.com>
[PMD: Add missing "qemu/error-report.h" include and TODO comment]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 8 +++++++-
 hw/block/pflash_cfi02.c | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 2ca173aa46..11922c0f96 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -42,6 +42,7 @@
 #include "hw/qdev-properties.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
 #include "qemu/host-utils.h"
@@ -389,13 +390,18 @@ static void pflash_update(PFlashCFI01 *pfl, int offset,
                           int size)
 {
     int offset_end;
+    int ret;
     if (pfl->blk) {
         offset_end = offset + size;
         /* widen to sector boundaries */
         offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
         offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
-        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
+        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
                    offset_end - offset, 0);
+        if (ret < 0) {
+            /* TODO set error bit in status */
+            error_report("Could not update PFLASH: %s", strerror(-ret));
+        }
     }
 }
 
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index c277b0309d..ac7e34ecbf 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -37,6 +37,7 @@
 #include "hw/block/flash.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/bitmap.h"
 #include "qemu/timer.h"
 #include "sysemu/block-backend.h"
@@ -393,13 +394,18 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 static void pflash_update(PFlashCFI02 *pfl, int offset, int size)
 {
     int offset_end;
+    int ret;
     if (pfl->blk) {
         offset_end = offset + size;
         /* widen to sector boundaries */
         offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
         offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
-        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
+        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
                    offset_end - offset, 0);
+        if (ret < 0) {
+            /* TODO set error bit in status */
+            error_report("Could not update PFLASH: %s", strerror(-ret));
+        }
     }
 }
 
-- 
2.21.3



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PULL 0/4] pflash-next patches for 2020-05-22
  2020-05-22 17:45 [PULL 0/4] pflash-next patches for 2020-05-22 Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2020-05-22 17:45 ` [PULL 4/4] hw/block/pflash: Check return value of blk_pwrite() Philippe Mathieu-Daudé
@ 2020-05-24 13:42 ` Peter Maydell
  4 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2020-05-24 13:42 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, QEMU Developers, Qemu-block, Max Reitz

On Fri, 22 May 2020 at 18:47, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> The following changes since commit d19f1ab0de8b763159513e3eaa12c5bc68122361:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-2020052=
> 1-1' into staging (2020-05-21 22:06:56 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/pflash-next-20200522
>
> for you to fetch changes up to 1857b9db49770590483be44eb90993c42b2a5a99:
>
>   hw/block/pflash: Check return value of blk_pwrite() (2020-05-22 19:38:14 +0=
> 200)
>
> ----------------------------------------------------------------
>
> - Remove unused timer in CFI01 flash,
> - Clean up code documentation,
> - Silent a long-standing Coverity warning (2016-07-15).


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-22 17:45 [PULL 0/4] pflash-next patches for 2020-05-22 Philippe Mathieu-Daudé
2020-05-22 17:45 ` [PULL 1/4] hw/block/pflash_cfi01: Removed an unused timer Philippe Mathieu-Daudé
2020-05-22 17:45 ` [PULL 2/4] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00' Philippe Mathieu-Daudé
2020-05-22 17:45 ` [PULL 3/4] hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array' Philippe Mathieu-Daudé
2020-05-22 17:45 ` [PULL 4/4] hw/block/pflash: Check return value of blk_pwrite() Philippe Mathieu-Daudé
2020-05-24 13:42 ` [PULL 0/4] pflash-next patches for 2020-05-22 Peter Maydell

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git