from:"Stephen Checkoway"

The goal of this patch series implement the following AMD command-set parallel
flash functionality:
- flash interleaving;
- nonuniform sector sizes;
- erase suspend/resume commands; and
- multi-sector erase.

During refactoring and implementation, I discovered several bugs that are
fixed here as well:
- flash commands use only 11-bits of the address in most cases, but the
  current code uses all of them [1];
- entering CFI mode from autoselect mode and then exiting CFI mode should
  return the chip to autoselect mode, but the current code returns to read
  array mode; and
- reset command should be ignored during sector/chip erase, but the current
  code performs the reset.

The first patch in the series adds a test for the existing behavior. Tests for
additional behavior/bug fixes are added in the relevant patch.

1. I found firmware in the wild that relies on the 11-bit address behavior,
   probably due to a bug in the firmware itself.

Stephen Checkoway (10):
  block/pflash_cfi02: Add test for supported commands
  block/pflash_cfi02: Refactor, NFC intended
  block/pflash_cfi02: Fix command address comparison
  block/pflash_cfi02: Implement intereleaved flash devices
  block/pflash_cfi02: Implement nonuniform sector sizes
  block/pflash_cfi02: Fix CFI in autoselect mode
  block/pflash_cfi02: Fix reset command not ignored during erase
  block/pflash_cfi02: Implement multi-sector erase
  block/pflash_cfi02: Implement erase suspend/resume
  block/pflash_cfi02: Use the chip erase time specified in the CFI table

 hw/block/pflash_cfi02.c   | 843 +++---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 757 ++
 3 files changed, 1367 insertions(+), 235 deletions(-)
 create mode 100644 tests/pflash-cfi02-test.c

-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH 05/10] block/pflash_cfi02: Implement nonuniform sector sizes

Some flash chips support sectors of different sizes. For example, the
AMD AM29LV160DT has 31 64 kB sectors, one 32 kB sector, two 8 kB
sectors, and a 16 kB sector, in that order. The AM29LV160DB has those in
the reverse order.

The `num-blocks` and `sector-length` properties work exactly as they did
before: a flash device with uniform sector lengths. To get non-uniform
sector lengths for up to four regions, the following properties may be
set
- region 0. `num-blocks0` and `sector-length0`;
- region 1. `num-blocks1` and `sector-length1`;
- region 2. `num-blocks2` and `sector-length2`; and
- region 3. `num-blocks3` and `sector-length3`.

If the uniform and nonuniform properties are set, then both must specify
a flash device with the same total size. It would be better to disallow
both being set, or make `num-blocks0` and `sector-length0` alias
`num-blocks` and `sector-length`, but that would make testing currently
impossible.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 177 ++---
 tests/pflash-cfi02-test.c | 181 +-
 2 files changed, 266 insertions(+), 92 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index c7e568d6e0..7c94c3adef 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
  * It does not implement multiple sectors erase
@@ -55,6 +54,13 @@ do {   \
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/*
+ * The size of the cfi_table indirectly depends on this and the start of the
+ * PRI table directly depends on it. 4 is the maximum size (and also what
+ * seems common) without changing the PRT table address.
+ */
+#define PFLASH_MAX_ERASE_REGIONS 4
+
 /* Special write cycle for CFI queries. */
 #define WCYCLE_CFI 7
 
@@ -64,8 +70,10 @@ struct PFlashCFI02 {
 /*< public >*/
 
 BlockBackend *blk;
-uint32_t sector_len;
-uint32_t nb_blocs;
+uint32_t uniform_nb_blocs;
+uint32_t uniform_sector_len;
+uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
+uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
 uint64_t total_len;
 uint64_t interleave_multiplier;
 uint8_t mappings;
@@ -86,7 +94,7 @@ struct PFlashCFI02 {
 uint16_t ident3;
 uint16_t unlock_addr0;
 uint16_t unlock_addr1;
-uint8_t cfi_table[0x52];
+uint8_t cfi_table[0x4D];
 QEMUTimer timer;
 /* The device replicates the flash memory across its memory space.  Emulate
  * that by having a container (.mem) filled with an array of aliases
@@ -189,6 +197,25 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
+/*
+ * offset should be a byte offset of the QEMU device and _not_ a device
+ * offset.
+ */
+static uint32_t pflash_sector_len(PFlashCFI02 *pfl, hwaddr offset)
+{
+assert(offset < pfl->total_len);
+int nb_regions = pfl->cfi_table[0x2C];
+hwaddr addr = 0;
+for (int i = 0; i < nb_regions; ++i) {
+uint64_t region_size = (uint64_t)pfl->nb_blocs[i] * pfl->sector_len[i];
+if (addr <= offset && offset < addr + region_size) {
+return pfl->sector_len[i];
+}
+addr += region_size;
+}
+abort();
+}
+
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
@@ -285,6 +312,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
+uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -450,12 +478,14 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 case 0x30:
 /* Sector erase */
 p = pfl->storage;
-offset &= ~(pfl->sector_len - 1);
-DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
-offset);
+sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
 if (!pfl->ro) {
-memset(p + offset, 0xFF, pfl->sector_len);
-pflash_update(pfl, offset, pfl->sector_len);
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
 }
 set_dq7(pfl, 0x00);
 /* Let's wait 1/2 second before sector erase is do

[Qemu-devel] [PATCH 07/10] block/pflash_cfi02: Fix reset command not ignored during erase

When the flash device is performing a chip erase, all commands are
ignored. When it is performing a sector erase, only the erase suspend
command is valid, which is currently not supported.

In particular, the reset command should not cause the device to reset to
read array mode while programming is on going.
---
 hw/block/pflash_cfi02.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 32aba9a771..fa6929b9b6 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -325,7 +325,8 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pfl->bank_width * 2, value);
 }
 
-if (cmd == 0xF0) {
+/* Reset does nothing during chip erase and sector erase. */
+if (cmd == 0xF0 && pfl->cmd != 0x10 && pfl->cmd != 0x30) {
 #if 0
 DPRINTF("%s: flash reset asked (%02x %02x)\n",
 __func__, pfl->cmd, cmd);
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH 06/10] block/pflash_cfi02: Fix CFI in autoselect mode

After a flash device enters CFI mode from autoselect mode, the reset
command returns the device to autoselect mode. An additional reset
command is necessary to return to read array mode.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 21 +
 tests/pflash-cfi02-test.c | 36 
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 7c94c3adef..32aba9a771 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -61,8 +61,9 @@ do {   \
  */
 #define PFLASH_MAX_ERASE_REGIONS 4
 
-/* Special write cycle for CFI queries. */
+/* Special write cycles for CFI queries. */
 #define WCYCLE_CFI 7
+#define WCYCLE_AUTOSELECT_CFI 8
 
 struct PFlashCFI02 {
 /*< private >*/
@@ -329,6 +330,12 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 DPRINTF("%s: flash reset asked (%02x %02x)\n",
 __func__, pfl->cmd, cmd);
 #endif
+if (pfl->wcycle == WCYCLE_AUTOSELECT_CFI) {
+/* Return to autoselect mode. */
+pfl->wcycle = 3;
+pfl->cmd = 0x90;
+return;
+}
 goto reset_flash;
 }
 }
@@ -354,7 +361,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* We're in read mode */
 check_unlock0:
 if (masked_addr == 0x55 && cmd == 0x98) {
-enter_CFI_mode:
 /* Enter CFI query mode */
 pfl->wcycle = WCYCLE_CFI;
 pfl->cmd = 0x98;
@@ -431,9 +437,15 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* Unlock bypass reset */
 goto reset_flash;
 }
-/* We can enter CFI query mode from autoselect mode */
+/*
+ * We can enter CFI query mode from autoselect mode, but we must
+ * return to autoselect mode after a reset.
+ */
 if (masked_addr == 0x55 && cmd == 0x98) {
-goto enter_CFI_mode;
+/* Enter autoselect CFI query mode */
+pfl->wcycle = WCYCLE_AUTOSELECT_CFI;
+pfl->cmd = 0x98;
+return;
 }
 /* No break here */
 default:
@@ -514,6 +526,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 break;
 case WCYCLE_CFI: /* Special value for CFI queries */
+case WCYCLE_AUTOSELECT_CFI:
 DPRINTF("%s: invalid write in CFI query mode\n", __func__);
 goto reset_flash;
 default:
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index c16118e38b..c984295167 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -437,6 +437,39 @@ static void test_geometry(const void *opaque)
 qtest_quit(global_qtest);
 }
 
+/*
+ * Test that
+ * 1. enter autoselect mode;
+ * 2. enter CFI mode; and then
+ * 3. exit CFI mode
+ * leaves the flash device in autoselect mode.
+ */
+static void test_cfi_in_autoselect(const void *opaque)
+{
+const FlashConfig *c = opaque;
+global_qtest = qtest_initf("-M musicpal,accel=qtest"
+   " -drive if=pflash,file=%s,format=raw,"
+   "copy-on-read",
+   image_path);
+
+/* 1. Enter autoselect. */
+unlock(c);
+flash_cmd(c, UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+/* 2. Enter CFI. */
+flash_cmd(c, CFI_ADDR, CFI_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x10)), ==, replicate(c, 'Q'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x11)), ==, replicate(c, 'R'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x12)), ==, replicate(c, 'Y'));
+
+/* 3. Exit CFI. */
+reset(c);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+qtest_quit(global_qtest);
+}
+
 static void cleanup(void *opaque)
 {
 unlink(image_path);
@@ -560,6 +593,9 @@ int main(int argc, char **argv)
 qtest_add_data_func(path, config, test_geometry);
 g_free(path);
 }
+
+qtest_add_data_func("pflash-cfi02/cfi-in-autoselect", &configuration[0],
+test_cfi_in_autoselect);
 int result = g_test_run();
 cleanup(NULL);
 return result;
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands

From: Stephen Checkoway 

Test the AMD command set for parallel flash chips. This test uses an
ARM musicpal board with a pflash drive to test the following list of
currently-supported commands.
- Autoselect
- CFI
- Sector erase
- Chip erase
- Program
- Unlock bypass
- Reset

Signed-off-by: Stephen Checkoway 
---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 224 ++
 2 files changed, 226 insertions(+)
 create mode 100644 tests/pflash-cfi02-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6b904d7430..0a26eacce0 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -263,6 +263,7 @@ check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/hexloader-test$(EXESUF)
+check-qtest-arm-$(CONFIG_PFLASH_CFI02) += tests/pflash-cfi02-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
@@ -773,6 +774,7 @@ tests/device-introspect-test$(EXESUF): 
tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/hexloader-test$(EXESUF): tests/hexloader-test.o
+tests/pflash-cfi02$(EXESUF): tests/pflash-cfi02-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
 tests/rtas-test$(EXESUF): tests/rtas-test.o $(libqos-spapr-obj-y)
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
new file mode 100644
index 00..d349b2cc22
--- /dev/null
+++ b/tests/pflash-cfi02-test.c
@@ -0,0 +1,224 @@
+/*
+ * QTest testcase for parallel flash with AMD command set
+ *
+ * Copyright (c) 2018 Stephen Checkoway
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include 
+#include "libqtest.h"
+
+/*
+ * To test the pflash_cfi02 device, we run QEMU with the musicpal machine with
+ * a pflash drive. This enables us to test some flash configurations, but not
+ * all. In particular, we're limited to a 16-bit wide flash device.
+ */
+
+#define MP_FLASH_SIZE_MAX (32*1024*1024)
+#define BASE_ADDR (0x1ULL-MP_FLASH_SIZE_MAX)
+
+#define FLASH_WIDTH 2
+#define CFI_ADDR (FLASH_WIDTH*0x55)
+#define UNLOCK0_ADDR (FLASH_WIDTH*0x)
+#define UNLOCK1_ADDR (FLASH_WIDTH*0x2AAA)
+
+#define CFI_CMD 0x98
+#define UNLOCK0_CMD 0xAA
+#define UNLOCK1_CMD 0x55
+#define AUTOSELECT_CMD 0x90
+#define RESET_CMD 0xF0
+#define PROGRAM_CMD 0xA0
+#define SECTOR_ERASE_CMD 0x30
+#define CHIP_ERASE_CMD 0x10
+#define UNLOCK_BYPASS_CMD 0x20
+#define UNLOCK_BYPASS_RESET_CMD 0x00
+
+static char image_path[] = "/tmp/qtest.XX";
+
+static inline void flash_write(uint64_t byte_addr, uint16_t data)
+{
+qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
+}
+
+static inline uint16_t flash_read(uint64_t byte_addr)
+{
+return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
+}
+
+static void unlock(void)
+{
+flash_write(UNLOCK0_ADDR, UNLOCK0_CMD);
+flash_write(UNLOCK1_ADDR, UNLOCK1_CMD);
+}
+
+static void reset(void)
+{
+flash_write(0, RESET_CMD);
+}
+
+static void sector_erase(uint64_t byte_addr)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(byte_addr, SECTOR_ERASE_CMD);
+}
+
+static void wait_for_completion(uint64_t byte_addr)
+{
+/* If DQ6 is toggling, step the clock and ensure the toggle stops. */
+if ((flash_read(byte_addr) & 0x40) ^ (flash_read(byte_addr) & 0x40)) {
+/* Wait for erase or program to finish. */
+clock_step_next();
+/* Ensure that DQ6 has stopped toggling. */
+g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
+}
+}
+
+static void bypass_program(uint64_t byte_addr, uint16_t data)
+{
+flash_write(UNLOCK0_ADDR, PROGRAM_CMD);
+flash_write(byte_addr, data);
+/*
+ * Data isn't valid until DQ6 stops toggling. We don't model this as
+ * writes are immediate, but if this changes in the future, we can wait
+ * until the program is complete.
+ */
+wait_for_completion(byte_addr);
+}
+
+static void program(uint64_t byte_addr, uint16_t data)
+{
+unlock();
+bypass_program(byte_addr, data);
+}
+
+static void chip_erase(void)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(UNLOCK0_ADDR, SECTOR_ERASE_CMD);
+}
+
+static void test_flash(void)
+{
+global_qtest = qtest_initf("-M musicpal,accel=qtest "
+   "-drive 
if=pflash,file=%s,format=raw,copy-on-read",
+   image_path);
+/* Check the IDs. */
+unlock();
+flash_write(UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert

[Qemu-devel] [PATCH 04/10] block/pflash_cfi02: Implement intereleaved flash devices

It's common for multiple narrow flash chips to be hooked up in parallel
to support wider buses. For example, four 8-bit wide flash chips (x8)
may be combined in parallel to produce a 32-bit wide device. Similarly,
two 16-bit wide chips (x16) may be combined.

This commit introduces `device-width` and `max-device-width` properties,
similar to pflash_cfi01, with the following meanings:
- `width`: The width of the logical, qemu device (same as before);
- `device-width`: The width of an individual flash chip, defaulting to
  `width`; and
- `max-device-width`: The maximum width of an individual flash chip,
  defaulting to `device-width`.

Nothing needs to change to support reading such interleaved devices but
commands (e.g., erase and programming) must be sent to all devices at
the same time or else the various chips will be in different states.

For example, a 4-byte wide logical device can be composed of four x8/x16
devices in x8 mode. That is, each device supports both x8 or x16 and
they're being used in the byte, rather than word, mode. This
configuration would have `width=4`, `device-width=1`, and
`max-device-width=2`.

In addition to commands being sent to all devices, guest firmware
expects the status and CFI queries to be replicated for each device.
(The one exception to the response replication is that each device gets
to report its own status bit DQ7 while programming because its value
depends on the value being programmed which will usually differ for each
device.)

Testing is limited to 16-bit wide devices due to the current inability
to override the properties set by `pflash_cfi02_register`, but multiple
configurations are tested.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 270 +---
 tests/pflash-cfi02-test.c | 419 +-
 2 files changed, 526 insertions(+), 163 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index cb102739d6..c7e568d6e0 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not support flash interleaving.
  * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
@@ -67,15 +66,19 @@ struct PFlashCFI02 {
 BlockBackend *blk;
 uint32_t sector_len;
 uint32_t nb_blocs;
-uint32_t chip_len;
+uint64_t total_len;
+uint64_t interleave_multiplier;
 uint8_t mappings;
-uint8_t width;
+uint8_t bank_width; /* Width of the QEMU device in bytes. */
+uint8_t device_width; /* Width of individual pflash chip. */
+uint8_t max_device_width; /* Maximum width of individual pflash chip. */
 uint8_t be;
+int device_shift; /* Amount to shift an offset to get a device address. */
 int wcycle; /* if 0, the flash is read normally */
 int bypass;
 int ro;
 uint8_t cmd;
-uint8_t status;
+uint64_t status;
 /* FIXME: implement array device properties */
 uint16_t ident0;
 uint16_t ident1;
@@ -103,16 +106,17 @@ struct PFlashCFI02 {
  */
 static inline void toggle_dq7(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x80;
+pfl->status ^= pfl->interleave_multiplier * 0x80;
 }
 
 /*
  * Set status bit DQ7 to bit 7 of value.
  */
-static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+static inline void set_dq7(PFlashCFI02 *pfl, uint64_t value)
 {
-pfl->status &= 0x7F;
-pfl->status |= value & 0x80;
+uint64_t mask = pfl->interleave_multiplier * 0x80;
+pfl->status &= ~mask;
+pfl->status |= value & mask;
 }
 
 /*
@@ -120,7 +124,7 @@ static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
  */
 static inline void toggle_dq6(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x40;
+pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
 /*
@@ -188,7 +192,6 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
-hwaddr boff;
 uint64_t ret;
 
 ret = -1;
@@ -198,12 +201,10 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
unsigned int width)
 ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
 pflash_register_memory(pfl, 1);
 }
-offset &= pfl->chip_len - 1;
-boff = offset & 0xFF;
-if (pfl->width == 2)
-boff = boff >> 1;
-else if (pfl->width == 4)
-boff = boff >> 2;
+/* Mask by the total length of the chip to account for alias mappings. */
+offset &= pfl->total_len - 1;
+hwaddr device_addr = offset >> pfl->device_shift;
+
 switch (pfl->cmd) {
 default:
 /* This should never happen : reset state & treat it as a read*/
@@ -215,29 +216,32 @@ static uint64_t pf

[Qemu-devel] [PATCH 10/10] block/pflash_cfi02: Use the chip erase time specified in the CFI table

When erasing the chip, use the typical time specified in the CFI table
rather than arbitrarily selecting 5 seconds.

Since the currently unconfigurable value set in the table is 12, this
means a chip erase takes 4096 ms so this isn't a big change in behavior.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f8a7314dee..f256049375 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -637,9 +637,9 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pflash_update(pfl, 0, pfl->total_len);
 }
 set_dq7(pfl, 0x00);
-/* Let's wait 5 seconds before chip erase is done */
+/* Wait the time specified at CFI address 0x22. */
 timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-  (NANOSECONDS_PER_SECOND * 5));
+  (1ULL << pfl->cfi_table[0x22]) * SCALE_MS);
 break;
 case 0x30:
 /* Sector erase */
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH 03/10] block/pflash_cfi02: Fix command address comparison

From: Stephen Checkoway 

Most AMD commands only examine 11 bits of the address. This masks the
addresses used in the comparison to 11 bits. The exceptions are word or
sector addresses which use offset directly rather than the shifted
offset, boff.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   |  8 +++-
 tests/pflash-cfi02-test.c | 12 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 53db3fd09f..cb102739d6 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -296,11 +296,13 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 
 DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n", __func__,
 offset, value, width);
-boff = offset & (pfl->sector_len - 1);
+boff = offset;
 if (pfl->width == 2)
 boff = boff >> 1;
 else if (pfl->width == 4)
 boff = boff >> 2;
+/* Only the least-significant 11 bits are used in most cases. */
+boff &= 0x7FF;
 switch (pfl->wcycle) {
 case 0:
 /* Set the device in I/O access mode if required */
@@ -519,6 +521,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
+/* Only 11 bits are used in the comparison. */
+pfl->unlock_addr0 &= 0x7FF;
+pfl->unlock_addr1 &= 0x7FF;
+
 chip_len = pfl->sector_len * pfl->nb_blocs;
 
 memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index d349b2cc22..57484c1585 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -23,8 +23,8 @@
 
 #define FLASH_WIDTH 2
 #define CFI_ADDR (FLASH_WIDTH*0x55)
-#define UNLOCK0_ADDR (FLASH_WIDTH*0x)
-#define UNLOCK1_ADDR (FLASH_WIDTH*0x2AAA)
+#define UNLOCK0_ADDR (FLASH_WIDTH*0x555)
+#define UNLOCK1_ADDR (FLASH_WIDTH*0x2AA)
 
 #define CFI_CMD 0x98
 #define UNLOCK0_CMD 0xAA
@@ -191,6 +191,14 @@ static void test_flash(void)
 g_assert_cmpint(flash_read(6), ==, 0xCDEF);
 g_assert_cmpint(flash_read(8), ==, 0x);
 
+/* Test ignored high order bits of address. */
+flash_write(FLASH_WIDTH*0x, UNLOCK0_CMD);
+flash_write(FLASH_WIDTH*0x2AAA, UNLOCK1_CMD);
+flash_write(FLASH_WIDTH*0x, AUTOSELECT_CMD);
+g_assert_cmpint(flash_read(FLASH_WIDTH*0x), ==, 0x00BF);
+g_assert_cmpint(flash_read(FLASH_WIDTH*0x0001), ==, 0x236D);
+reset();
+
 qtest_quit(global_qtest);
 }
 
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH 09/10] block/pflash_cfi02: Implement erase suspend/resume

During a sector erase (but not a chip erase), the embeded erase program
can be suspended. Once suspended, the sectors not selected for erasure
may be read and programmed. Autoselect mode is allowed during erase
suspend mode. Presumably, CFI queries are similarly allowed so this
commit allows them as well.

Since guest firmware can use status bits DQ7, DQ6, DQ3, and DQ2 to
determine the current state of sector erasure, these bits are properly
implemented.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 153 ++
 tests/pflash-cfi02-test.c | 102 +
 2 files changed, 241 insertions(+), 14 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4f773f9229..f8a7314dee 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -29,7 +29,6 @@
  * - CFI queries
  *
  * It does not implement software data protection as found in many real chips
- * It does not implement erase suspend/resume commands
  */
 
 #include "qemu/osdep.h"
@@ -37,6 +36,7 @@
 #include "hw/block/block.h"
 #include "hw/block/flash.h"
 #include "qapi/error.h"
+#include "qemu/bitmap.h"
 #include "qemu/timer.h"
 #include "sysemu/block-backend.h"
 #include "qemu/host-utils.h"
@@ -72,6 +72,7 @@ struct PFlashCFI02 {
 BlockBackend *blk;
 uint32_t uniform_nb_blocs;
 uint32_t uniform_sector_len;
+uint32_t total_sectors;
 uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
 uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
 uint64_t total_len;
@@ -106,6 +107,8 @@ struct PFlashCFI02 {
 int rom_mode;
 int read_counter; /* used for lazy switch-back to rom mode */
 int sectors_to_erase;
+uint64_t erase_time_remaining;
+unsigned long *sector_erase_map;
 char *name;
 void *storage;
 };
@@ -152,6 +155,14 @@ static inline void reset_dq3(PFlashCFI02 *pfl)
 pfl->status &= ~(pfl->interleave_multiplier * 0x08);
 }
 
+/*
+ * Toggle status bit DQ2.
+ */
+static inline void toggle_dq2(PFlashCFI02 *pfl)
+{
+pfl->status ^= pfl->interleave_multiplier * 0x04;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -175,6 +186,29 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
+/*
+ * Returns the time it takes to erase the number of sectors scheduled for
+ * erasure based on CFI address 0x21 which is "Typical timeout per individual
+ * block erase 2^N ms."
+ */
+static uint64_t pflash_erase_time(PFlashCFI02 *pfl)
+{
+/*
+ * If there are no sectors to erase (which can happen if all of the sectors
+ * to be erased are protected), then erase takes 100 us. Protected sectors
+ * aren't supported so this should never happen.
+ */
+return ((1ULL << pfl->cfi_table[0x21]) * pfl->sectors_to_erase) * SCALE_US;
+}
+
+/*
+ * Returns true if the device is currently in erase suspend mode.
+ */
+static inline bool pflash_erase_suspend_mode(PFlashCFI02 *pfl)
+{
+return pfl->erase_time_remaining > 0;
+}
+
 static void pflash_timer(void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
@@ -189,12 +223,7 @@ static void pflash_timer(void *opaque)
  */
 if ((pfl->status & 0x08) == 0) {
 assert_dq3(pfl);
-/*
- * CFI address 0x21 is "Typical timeout per individual block erase
- * 2^N ms"
- */
-uint64_t timeout = ((1ULL << pfl->cfi_table[0x21]) *
-pfl->sectors_to_erase) * 100;
+uint64_t timeout = pflash_erase_time(pfl);
 timer_mod(&pfl->timer,
   qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
 DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
@@ -202,6 +231,7 @@ static void pflash_timer(void *opaque)
 return;
 }
 DPRINTF("%s: sector erase complete\n", __func__);
+bitmap_zero(pfl->sector_erase_map, pfl->total_sectors);
 pfl->sectors_to_erase = 0;
 reset_dq3(pfl);
 }
@@ -240,25 +270,45 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
+typedef struct {
+uint32_t len;
+uint32_t num;
+} SectorInfo;
+
 /*
  * offset should be a byte offset of the QEMU device and _not_ a device
  * offset.
  */
-static uint32_t pflash_sector_len(PFlashCFI02 *pfl, hwaddr offset)
+static SectorInfo pflash_sector_info(PFlashCFI02 *pfl, hwaddr offset)
 {
 assert(offset < pfl->total_len);
 int nb_regions = pfl->cfi_table[0x2C];
 hwaddr addr = 0;
+uint32_t sector_num = 0;
 for (int i = 0; i < nb_regions; ++i) {
 uint64_t region_size = (uint64_t)pfl->nb_blocs[i] * pfl->sector_len[i];
 if (addr <= offset &&a

[Qemu-devel] [PATCH 02/10] block/pflash_cfi02: Refactor, NFC intended

From: Stephen Checkoway 

Simplify and refactor for upcoming commits. In particular, pull out all
of the code to modify the status into simple helper functions. Status
handling becomes more complex once multiple chips are interleaved to
produce a single device.

No change in functionality is intended with this commit.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 221 +---
 1 file changed, 95 insertions(+), 126 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f81..53db3fd09f 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -46,18 +46,19 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 
-//#define PFLASH_DEBUG
-#ifdef PFLASH_DEBUG
+#define PFLASH_DEBUG false
 #define DPRINTF(fmt, ...)  \
 do {   \
-fprintf(stderr, "PFLASH: " fmt , ## __VA_ARGS__);   \
+if (PFLASH_DEBUG) {\
+fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__);   \
+}  \
 } while (0)
-#else
-#define DPRINTF(fmt, ...) do { } while (0)
-#endif
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/* Special write cycle for CFI queries. */
+#define WCYCLE_CFI 7
+
 struct PFlashCFI02 {
 /*< private >*/
 SysBusDevice parent_obj;
@@ -97,6 +98,31 @@ struct PFlashCFI02 {
 void *storage;
 };
 
+/*
+ * Toggle status bit DQ7.
+ */
+static inline void toggle_dq7(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x80;
+}
+
+/*
+ * Set status bit DQ7 to bit 7 of value.
+ */
+static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+{
+pfl->status &= 0x7F;
+pfl->status |= value & 0x80;
+}
+
+/*
+ * Toggle status bit DQ6.
+ */
+static inline void toggle_dq6(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x40;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -126,7 +152,7 @@ static void pflash_timer (void *opaque)
 
 trace_pflash_timer_expired(pfl->cmd);
 /* Reset flash */
-pfl->status ^= 0x80;
+toggle_dq7(pfl);
 if (pfl->bypass) {
 pfl->wcycle = 2;
 } else {
@@ -136,12 +162,34 @@ static void pflash_timer (void *opaque)
 pfl->cmd = 0;
 }
 
-static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
-int width, int be)
+/*
+ * Read data from flash.
+ */
+static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr offset,
+ unsigned int width)
 {
+uint8_t *p = (uint8_t *)pfl->storage + offset;
+uint64_t ret = pfl->be? ldn_be_p(p, width) : ldn_le_p(p, width);
+/* XXX: Need a trace_pflash_data_read(offset, ret, width) */
+switch (width) {
+case 1:
+trace_pflash_data_read8(offset, ret);
+break;
+case 2:
+trace_pflash_data_read16(offset, ret);
+break;
+case 4:
+trace_pflash_data_read32(offset, ret);
+break;
+}
+return ret;
+}
+
+static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
+{
+PFlashCFI02 *pfl = opaque;
 hwaddr boff;
-uint32_t ret;
-uint8_t *p;
+uint64_t ret;
 
 ret = -1;
 trace_pflash_read(offset, pfl->cmd, width, pfl->wcycle);
@@ -166,39 +214,8 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x80:
 /* We accept reads during second unlock sequence... */
 case 0x00:
-flash_read:
 /* Flash area read */
-p = pfl->storage;
-switch (width) {
-case 1:
-ret = p[offset];
-trace_pflash_data_read8(offset, ret);
-break;
-case 2:
-if (be) {
-ret = p[offset] << 8;
-ret |= p[offset + 1];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-}
-trace_pflash_data_read16(offset, ret);
-break;
-case 4:
-if (be) {
-ret = p[offset] << 24;
-ret |= p[offset + 1] << 16;
-ret |= p[offset + 2] << 8;
-ret |= p[offset + 3];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-ret |= p[offset + 2] << 16;
-ret |= p[offset + 3] << 24;
-}
-trace_pflash_data_read32(offset, ret);
-break;
-}
+ret = pflash_data_read(pfl, offset, width);
 break;
 case 0x90:
 /* flash ID read */
@@ -213,23 +230,23 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x0E:
 case 0x0F:
 ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
-if (ret == (uint8_t)-1) {
-go

[Qemu-devel] [PATCH 08/10] block/pflash_cfi02: Implement multi-sector erase

After two unlock cycles and a sector erase command, the AMD flash chips
start a 50 us erase time out. Any additional sector erase commands add a
sector to be erased and restart the 50 us timeout. During the timeout,
status bit DQ3 is cleared. After the time out, DQ3 is asserted during
erasure.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 94 +++
 tests/pflash-cfi02-test.c | 59 ++--
 2 files changed, 131 insertions(+), 22 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index fa6929b9b6..4f773f9229 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -30,7 +30,6 @@
  *
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
- * It does not implement multiple sectors erase
  */
 
 #include "qemu/osdep.h"
@@ -106,6 +105,7 @@ struct PFlashCFI02 {
 MemoryRegion orig_mem;
 int rom_mode;
 int read_counter; /* used for lazy switch-back to rom mode */
+int sectors_to_erase;
 char *name;
 void *storage;
 };
@@ -136,6 +136,22 @@ static inline void toggle_dq6(PFlashCFI02 *pfl)
 pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
+/*
+ * Turn on DQ3.
+ */
+static inline void assert_dq3(PFlashCFI02 *pfl)
+{
+pfl->status |= pfl->interleave_multiplier * 0x08;
+}
+
+/*
+ * Turn off DQ3.
+ */
+static inline void reset_dq3(PFlashCFI02 *pfl)
+{
+pfl->status &= ~(pfl->interleave_multiplier * 0x08);
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -159,11 +175,37 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
-static void pflash_timer (void *opaque)
+static void pflash_timer(void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
 
 trace_pflash_timer_expired(pfl->cmd);
+if (pfl->cmd == 0x30) {
+/*
+ * Sector erase. If DQ3 is 0 when the timer expires, then the 50
+ * us erase timeout has expired so we need to start the timer for the
+ * sector erase algorithm. Otherwise, the erase completed and we should
+ * go back to read array mode.
+ */
+if ((pfl->status & 0x08) == 0) {
+assert_dq3(pfl);
+/*
+ * CFI address 0x21 is "Typical timeout per individual block erase
+ * 2^N ms"
+ */
+uint64_t timeout = ((1ULL << pfl->cfi_table[0x21]) *
+pfl->sectors_to_erase) * 100;
+timer_mod(&pfl->timer,
+  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
+DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
+__func__, pfl->sectors_to_erase);
+return;
+}
+DPRINTF("%s: sector erase complete\n", __func__);
+pfl->sectors_to_erase = 0;
+reset_dq3(pfl);
+}
+
 /* Reset flash */
 toggle_dq7(pfl);
 if (pfl->bypass) {
@@ -307,13 +349,30 @@ static void pflash_update(PFlashCFI02 *pfl, int offset, 
int size)
 }
 }
 
+static void pflash_sector_erase(PFlashCFI02 *pfl, hwaddr offset)
+{
+uint64_t sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
+if (!pfl->ro) {
+uint8_t *p = pfl->storage;
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
+}
+set_dq7(pfl, 0x00);
+++pfl->sectors_to_erase;
+/* Set (or reset) the 50 us timer for additional erase commands.  */
+timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 5);
+}
+
 static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
  unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
-uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -490,20 +549,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 break;
 case 0x30:
 /* Sector erase */
-p = pfl->storage;
-sector_len = pflash_sector_len(pfl, offset);
-offset &= ~(sector_len - 1);
-DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
-__func__, pfl->bank_width * 2, offset,
-pfl->bank_width * 2, offset + sector_len - 1);
-if (!pfl->ro) {
-memset(p + offset, 0xFF, sector_len);
-pflash_update(pfl, offset, sector_len);
-}
-set_

[Qemu-devel] [PATCH v2 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality

The goal of this patch series implement the following AMD command-set parallel
flash functionality:
- flash interleaving;
- nonuniform sector sizes;
- erase suspend/resume commands; and
- multi-sector erase.

During refactoring and implementation, I discovered several bugs that are
fixed here as well:
- flash commands use only 11-bits of the address in most cases, but the
  current code uses all of them [1];
- entering CFI mode from autoselect mode and then exiting CFI mode should
  return the chip to autoselect mode, but the current code returns to read
  array mode; and
- reset command should be ignored during sector/chip erase, but the current
  code performs the reset.

The first patch in the series adds a test for the existing behavior. Tests for
additional behavior/bug fixes are added in the relevant patch.

1. I found firmware in the wild that relies on the 11-bit address behavior,
   probably due to a bug in the firmware itself.

Changes from v1:
- Fix missing spaces around *, -, and ?.
- Fix missing Signed-off-by line on patch 7
- Replace use of errc with g_printerr and exit

Stephen Checkoway (10):
  block/pflash_cfi02: Add test for supported commands
  block/pflash_cfi02: Refactor, NFC intended
  block/pflash_cfi02: Fix command address comparison
  block/pflash_cfi02: Implement intereleaved flash devices
  block/pflash_cfi02: Implement nonuniform sector sizes
  block/pflash_cfi02: Fix CFI in autoselect mode
  block/pflash_cfi02: Fix reset command not ignored during erase
  block/pflash_cfi02: Implement multi-sector erase
  block/pflash_cfi02: Implement erase suspend/resume
  block/pflash_cfi02: Use the chip erase time specified in the CFI table

 hw/block/pflash_cfi02.c   | 843 +++---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 759 ++
 3 files changed, 1367 insertions(+), 237 deletions(-)
 create mode 100644 tests/pflash-cfi02-test.c

-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v2 01/10] block/pflash_cfi02: Add test for supported commands

Test the AMD command set for parallel flash chips. This test uses an
ARM musicpal board with a pflash drive to test the following list of
currently-supported commands.
- Autoselect
- CFI
- Sector erase
- Chip erase
- Program
- Unlock bypass
- Reset

Signed-off-by: Stephen Checkoway 
---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 227 ++
 2 files changed, 229 insertions(+)
 create mode 100644 tests/pflash-cfi02-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6b904d7430..0a26eacce0 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -263,6 +263,7 @@ check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/hexloader-test$(EXESUF)
+check-qtest-arm-$(CONFIG_PFLASH_CFI02) += tests/pflash-cfi02-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
@@ -773,6 +774,7 @@ tests/device-introspect-test$(EXESUF): 
tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/hexloader-test$(EXESUF): tests/hexloader-test.o
+tests/pflash-cfi02$(EXESUF): tests/pflash-cfi02-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
 tests/rtas-test$(EXESUF): tests/rtas-test.o $(libqos-spapr-obj-y)
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
new file mode 100644
index 00..b113fca5af
--- /dev/null
+++ b/tests/pflash-cfi02-test.c
@@ -0,0 +1,227 @@
+/*
+ * QTest testcase for parallel flash with AMD command set
+ *
+ * Copyright (c) 2018 Stephen Checkoway
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include 
+#include "libqtest.h"
+
+/*
+ * To test the pflash_cfi02 device, we run QEMU with the musicpal machine with
+ * a pflash drive. This enables us to test some flash configurations, but not
+ * all. In particular, we're limited to a 16-bit wide flash device.
+ */
+
+#define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
+#define BASE_ADDR (0x1ULL - MP_FLASH_SIZE_MAX)
+
+#define FLASH_WIDTH 2
+#define CFI_ADDR (FLASH_WIDTH * 0x55)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+
+#define CFI_CMD 0x98
+#define UNLOCK0_CMD 0xAA
+#define UNLOCK1_CMD 0x55
+#define AUTOSELECT_CMD 0x90
+#define RESET_CMD 0xF0
+#define PROGRAM_CMD 0xA0
+#define SECTOR_ERASE_CMD 0x30
+#define CHIP_ERASE_CMD 0x10
+#define UNLOCK_BYPASS_CMD 0x20
+#define UNLOCK_BYPASS_RESET_CMD 0x00
+
+static char image_path[] = "/tmp/qtest.XX";
+
+static inline void flash_write(uint64_t byte_addr, uint16_t data)
+{
+qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
+}
+
+static inline uint16_t flash_read(uint64_t byte_addr)
+{
+return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
+}
+
+static void unlock(void)
+{
+flash_write(UNLOCK0_ADDR, UNLOCK0_CMD);
+flash_write(UNLOCK1_ADDR, UNLOCK1_CMD);
+}
+
+static void reset(void)
+{
+flash_write(0, RESET_CMD);
+}
+
+static void sector_erase(uint64_t byte_addr)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(byte_addr, SECTOR_ERASE_CMD);
+}
+
+static void wait_for_completion(uint64_t byte_addr)
+{
+/* If DQ6 is toggling, step the clock and ensure the toggle stops. */
+if ((flash_read(byte_addr) & 0x40) ^ (flash_read(byte_addr) & 0x40)) {
+/* Wait for erase or program to finish. */
+clock_step_next();
+/* Ensure that DQ6 has stopped toggling. */
+g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
+}
+}
+
+static void bypass_program(uint64_t byte_addr, uint16_t data)
+{
+flash_write(UNLOCK0_ADDR, PROGRAM_CMD);
+flash_write(byte_addr, data);
+/*
+ * Data isn't valid until DQ6 stops toggling. We don't model this as
+ * writes are immediate, but if this changes in the future, we can wait
+ * until the program is complete.
+ */
+wait_for_completion(byte_addr);
+}
+
+static void program(uint64_t byte_addr, uint16_t data)
+{
+unlock();
+bypass_program(byte_addr, data);
+}
+
+static void chip_erase(void)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(UNLOCK0_ADDR, SECTOR_ERASE_CMD);
+}
+
+static void test_flash(void)
+{
+global_qtest = qtest_initf("-M musicpal,accel=qtest "
+   "-drive 
if=pflash,file=%s,format=raw,copy-on-read",
+   image_path);
+/* Check the IDs. */
+unlock();
+flash_write(UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert_cmpint(flash

[Qemu-devel] [PATCH v2 03/10] block/pflash_cfi02: Fix command address comparison

Most AMD commands only examine 11 bits of the address. This masks the
addresses used in the comparison to 11 bits. The exceptions are word or
sector addresses which use offset directly rather than the shifted
offset, boff.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   |  8 +++-
 tests/pflash-cfi02-test.c | 12 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4b7af71806..e4bff0c8f8 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -296,11 +296,13 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 
 DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n", __func__,
 offset, value, width);
-boff = offset & (pfl->sector_len - 1);
+boff = offset;
 if (pfl->width == 2)
 boff = boff >> 1;
 else if (pfl->width == 4)
 boff = boff >> 2;
+/* Only the least-significant 11 bits are used in most cases. */
+boff &= 0x7FF;
 switch (pfl->wcycle) {
 case 0:
 /* Set the device in I/O access mode if required */
@@ -519,6 +521,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
+/* Only 11 bits are used in the comparison. */
+pfl->unlock_addr0 &= 0x7FF;
+pfl->unlock_addr1 &= 0x7FF;
+
 chip_len = pfl->sector_len * pfl->nb_blocs;
 
 memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index b113fca5af..b91bb66a79 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -23,8 +23,8 @@
 
 #define FLASH_WIDTH 2
 #define CFI_ADDR (FLASH_WIDTH * 0x55)
-#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
-#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x555)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AA)
 
 #define CFI_CMD 0x98
 #define UNLOCK0_CMD 0xAA
@@ -192,6 +192,14 @@ static void test_flash(void)
 g_assert_cmpint(flash_read(6), ==, 0xCDEF);
 g_assert_cmpint(flash_read(8), ==, 0x);
 
+/* Test ignored high order bits of address. */
+flash_write(FLASH_WIDTH * 0x, UNLOCK0_CMD);
+flash_write(FLASH_WIDTH * 0x2AAA, UNLOCK1_CMD);
+flash_write(FLASH_WIDTH * 0x, AUTOSELECT_CMD);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x), ==, 0x00BF);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x0001), ==, 0x236D);
+reset();
+
 qtest_quit(global_qtest);
 }
 
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v2 07/10] block/pflash_cfi02: Fix reset command not ignored during erase

When the flash device is performing a chip erase, all commands are
ignored. When it is performing a sector erase, only the erase suspend
command is valid, which is currently not supported.

In particular, the reset command should not cause the device to reset to
read array mode while programming is on going.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index be10036886..cb1160eb35 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -325,7 +325,8 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pfl->bank_width * 2, value);
 }
 
-if (cmd == 0xF0) {
+/* Reset does nothing during chip erase and sector erase. */
+if (cmd == 0xF0 && pfl->cmd != 0x10 && pfl->cmd != 0x30) {
 if (pfl->wcycle == WCYCLE_AUTOSELECT_CFI) {
 /* Return to autoselect mode. */
 pfl->wcycle = 3;
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v2 04/10] block/pflash_cfi02: Implement intereleaved flash devices

It's common for multiple narrow flash chips to be hooked up in parallel
to support wider buses. For example, four 8-bit wide flash chips (x8)
may be combined in parallel to produce a 32-bit wide device. Similarly,
two 16-bit wide chips (x16) may be combined.

This commit introduces `device-width` and `max-device-width` properties,
similar to pflash_cfi01, with the following meanings:
- `width`: The width of the logical, qemu device (same as before);
- `device-width`: The width of an individual flash chip, defaulting to
  `width`; and
- `max-device-width`: The maximum width of an individual flash chip,
  defaulting to `device-width`.

Nothing needs to change to support reading such interleaved devices but
commands (e.g., erase and programming) must be sent to all devices at
the same time or else the various chips will be in different states.

For example, a 4-byte wide logical device can be composed of four x8/x16
devices in x8 mode. That is, each device supports both x8 or x16 and
they're being used in the byte, rather than word, mode. This
configuration would have `width=4`, `device-width=1`, and
`max-device-width=2`.

In addition to commands being sent to all devices, guest firmware
expects the status and CFI queries to be replicated for each device.
(The one exception to the response replication is that each device gets
to report its own status bit DQ7 while programming because its value
depends on the value being programmed which will usually differ for each
device.)

Testing is limited to 16-bit wide devices due to the current inability
to override the properties set by `pflash_cfi02_register`, but multiple
configurations are tested.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 270 +---
 tests/pflash-cfi02-test.c | 418 +-
 2 files changed, 523 insertions(+), 165 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index e4bff0c8f8..101628b4ec 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not support flash interleaving.
  * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
@@ -67,15 +66,19 @@ struct PFlashCFI02 {
 BlockBackend *blk;
 uint32_t sector_len;
 uint32_t nb_blocs;
-uint32_t chip_len;
+uint64_t total_len;
+uint64_t interleave_multiplier;
 uint8_t mappings;
-uint8_t width;
+uint8_t bank_width; /* Width of the QEMU device in bytes. */
+uint8_t device_width; /* Width of individual pflash chip. */
+uint8_t max_device_width; /* Maximum width of individual pflash chip. */
 uint8_t be;
+int device_shift; /* Amount to shift an offset to get a device address. */
 int wcycle; /* if 0, the flash is read normally */
 int bypass;
 int ro;
 uint8_t cmd;
-uint8_t status;
+uint64_t status;
 /* FIXME: implement array device properties */
 uint16_t ident0;
 uint16_t ident1;
@@ -103,16 +106,17 @@ struct PFlashCFI02 {
  */
 static inline void toggle_dq7(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x80;
+pfl->status ^= pfl->interleave_multiplier * 0x80;
 }
 
 /*
  * Set status bit DQ7 to bit 7 of value.
  */
-static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+static inline void set_dq7(PFlashCFI02 *pfl, uint64_t value)
 {
-pfl->status &= 0x7F;
-pfl->status |= value & 0x80;
+uint64_t mask = pfl->interleave_multiplier * 0x80;
+pfl->status &= ~mask;
+pfl->status |= value & mask;
 }
 
 /*
@@ -120,7 +124,7 @@ static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
  */
 static inline void toggle_dq6(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x40;
+pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
 /*
@@ -188,7 +192,6 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
-hwaddr boff;
 uint64_t ret;
 
 ret = -1;
@@ -198,12 +201,10 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
unsigned int width)
 ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
 pflash_register_memory(pfl, 1);
 }
-offset &= pfl->chip_len - 1;
-boff = offset & 0xFF;
-if (pfl->width == 2)
-boff = boff >> 1;
-else if (pfl->width == 4)
-boff = boff >> 2;
+/* Mask by the total length of the chip to account for alias mappings. */
+offset &= pfl->total_len - 1;
+hwaddr device_addr = offset >> pfl->device_shift;
+
 switch (pfl->cmd) {
 default:
 /* This should never happen : reset state & treat it as a read*/
@@ -215,29 +216,32 @@ static uint64_t pf

[Qemu-devel] [PATCH v2 02/10] block/pflash_cfi02: Refactor, NFC intended

Simplify and refactor for upcoming commits. In particular, pull out all
of the code to modify the status into simple helper functions. Status
handling becomes more complex once multiple chips are interleaved to
produce a single device.

No change in functionality is intended with this commit.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 221 +---
 1 file changed, 95 insertions(+), 126 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f81..4b7af71806 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -46,18 +46,19 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 
-//#define PFLASH_DEBUG
-#ifdef PFLASH_DEBUG
+#define PFLASH_DEBUG false
 #define DPRINTF(fmt, ...)  \
 do {   \
-fprintf(stderr, "PFLASH: " fmt , ## __VA_ARGS__);   \
+if (PFLASH_DEBUG) {\
+fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__);   \
+}  \
 } while (0)
-#else
-#define DPRINTF(fmt, ...) do { } while (0)
-#endif
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/* Special write cycle for CFI queries. */
+#define WCYCLE_CFI 7
+
 struct PFlashCFI02 {
 /*< private >*/
 SysBusDevice parent_obj;
@@ -97,6 +98,31 @@ struct PFlashCFI02 {
 void *storage;
 };
 
+/*
+ * Toggle status bit DQ7.
+ */
+static inline void toggle_dq7(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x80;
+}
+
+/*
+ * Set status bit DQ7 to bit 7 of value.
+ */
+static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+{
+pfl->status &= 0x7F;
+pfl->status |= value & 0x80;
+}
+
+/*
+ * Toggle status bit DQ6.
+ */
+static inline void toggle_dq6(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x40;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -126,7 +152,7 @@ static void pflash_timer (void *opaque)
 
 trace_pflash_timer_expired(pfl->cmd);
 /* Reset flash */
-pfl->status ^= 0x80;
+toggle_dq7(pfl);
 if (pfl->bypass) {
 pfl->wcycle = 2;
 } else {
@@ -136,12 +162,34 @@ static void pflash_timer (void *opaque)
 pfl->cmd = 0;
 }
 
-static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
-int width, int be)
+/*
+ * Read data from flash.
+ */
+static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr offset,
+ unsigned int width)
 {
+uint8_t *p = (uint8_t *)pfl->storage + offset;
+uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
+/* XXX: Need a trace_pflash_data_read(offset, ret, width) */
+switch (width) {
+case 1:
+trace_pflash_data_read8(offset, ret);
+break;
+case 2:
+trace_pflash_data_read16(offset, ret);
+break;
+case 4:
+trace_pflash_data_read32(offset, ret);
+break;
+}
+return ret;
+}
+
+static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
+{
+PFlashCFI02 *pfl = opaque;
 hwaddr boff;
-uint32_t ret;
-uint8_t *p;
+uint64_t ret;
 
 ret = -1;
 trace_pflash_read(offset, pfl->cmd, width, pfl->wcycle);
@@ -166,39 +214,8 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x80:
 /* We accept reads during second unlock sequence... */
 case 0x00:
-flash_read:
 /* Flash area read */
-p = pfl->storage;
-switch (width) {
-case 1:
-ret = p[offset];
-trace_pflash_data_read8(offset, ret);
-break;
-case 2:
-if (be) {
-ret = p[offset] << 8;
-ret |= p[offset + 1];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-}
-trace_pflash_data_read16(offset, ret);
-break;
-case 4:
-if (be) {
-ret = p[offset] << 24;
-ret |= p[offset + 1] << 16;
-ret |= p[offset + 2] << 8;
-ret |= p[offset + 3];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-ret |= p[offset + 2] << 16;
-ret |= p[offset + 3] << 24;
-}
-trace_pflash_data_read32(offset, ret);
-break;
-}
+ret = pflash_data_read(pfl, offset, width);
 break;
 case 0x90:
 /* flash ID read */
@@ -213,23 +230,23 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x0E:
 case 0x0F:
 ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
-if (ret == (uint8_t)-1) {
-goto flash_read;
+

[Qemu-devel] [PATCH v2 08/10] block/pflash_cfi02: Implement multi-sector erase

After two unlock cycles and a sector erase command, the AMD flash chips
start a 50 us erase time out. Any additional sector erase commands add a
sector to be erased and restart the 50 us timeout. During the timeout,
status bit DQ3 is cleared. After the time out, DQ3 is asserted during
erasure.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 94 +++
 tests/pflash-cfi02-test.c | 59 ++--
 2 files changed, 131 insertions(+), 22 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index cb1160eb35..21ceb0823b 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -30,7 +30,6 @@
  *
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
- * It does not implement multiple sectors erase
  */
 
 #include "qemu/osdep.h"
@@ -106,6 +105,7 @@ struct PFlashCFI02 {
 MemoryRegion orig_mem;
 int rom_mode;
 int read_counter; /* used for lazy switch-back to rom mode */
+int sectors_to_erase;
 char *name;
 void *storage;
 };
@@ -136,6 +136,22 @@ static inline void toggle_dq6(PFlashCFI02 *pfl)
 pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
+/*
+ * Turn on DQ3.
+ */
+static inline void assert_dq3(PFlashCFI02 *pfl)
+{
+pfl->status |= pfl->interleave_multiplier * 0x08;
+}
+
+/*
+ * Turn off DQ3.
+ */
+static inline void reset_dq3(PFlashCFI02 *pfl)
+{
+pfl->status &= ~(pfl->interleave_multiplier * 0x08);
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -159,11 +175,37 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
-static void pflash_timer (void *opaque)
+static void pflash_timer(void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
 
 trace_pflash_timer_expired(pfl->cmd);
+if (pfl->cmd == 0x30) {
+/*
+ * Sector erase. If DQ3 is 0 when the timer expires, then the 50
+ * us erase timeout has expired so we need to start the timer for the
+ * sector erase algorithm. Otherwise, the erase completed and we should
+ * go back to read array mode.
+ */
+if ((pfl->status & 0x08) == 0) {
+assert_dq3(pfl);
+/*
+ * CFI address 0x21 is "Typical timeout per individual block erase
+ * 2^N ms"
+ */
+uint64_t timeout = ((1ULL << pfl->cfi_table[0x21]) *
+pfl->sectors_to_erase) * 100;
+timer_mod(&pfl->timer,
+  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
+DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
+__func__, pfl->sectors_to_erase);
+return;
+}
+DPRINTF("%s: sector erase complete\n", __func__);
+pfl->sectors_to_erase = 0;
+reset_dq3(pfl);
+}
+
 /* Reset flash */
 toggle_dq7(pfl);
 if (pfl->bypass) {
@@ -307,13 +349,30 @@ static void pflash_update(PFlashCFI02 *pfl, int offset, 
int size)
 }
 }
 
+static void pflash_sector_erase(PFlashCFI02 *pfl, hwaddr offset)
+{
+uint64_t sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
+if (!pfl->ro) {
+uint8_t *p = pfl->storage;
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
+}
+set_dq7(pfl, 0x00);
+++pfl->sectors_to_erase;
+/* Set (or reset) the 50 us timer for additional erase commands.  */
+timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 5);
+}
+
 static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
  unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
-uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -486,20 +545,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 break;
 case 0x30:
 /* Sector erase */
-p = pfl->storage;
-sector_len = pflash_sector_len(pfl, offset);
-offset &= ~(sector_len - 1);
-DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
-__func__, pfl->bank_width * 2, offset,
-pfl->bank_width * 2, offset + sector_len - 1);
-if (!pfl->ro) {
-memset(p + offset, 0xFF, sector_len);
-pflash_update(pfl, offset, sector_len);
-}
-set_

[Qemu-devel] [PATCH v2 06/10] block/pflash_cfi02: Fix CFI in autoselect mode

After a flash device enters CFI mode from autoselect mode, the reset
command returns the device to autoselect mode. An additional reset
command is necessary to return to read array mode.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 21 +
 tests/pflash-cfi02-test.c | 36 
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index c4efbe8cdf..be10036886 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -61,8 +61,9 @@ do {   \
  */
 #define PFLASH_MAX_ERASE_REGIONS 4
 
-/* Special write cycle for CFI queries. */
+/* Special write cycles for CFI queries. */
 #define WCYCLE_CFI 7
+#define WCYCLE_AUTOSELECT_CFI 8
 
 struct PFlashCFI02 {
 /*< private >*/
@@ -325,6 +326,12 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 
 if (cmd == 0xF0) {
+if (pfl->wcycle == WCYCLE_AUTOSELECT_CFI) {
+/* Return to autoselect mode. */
+pfl->wcycle = 3;
+pfl->cmd = 0x90;
+return;
+}
 goto reset_flash;
 }
 }
@@ -350,7 +357,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* We're in read mode */
 check_unlock0:
 if (masked_addr == 0x55 && cmd == 0x98) {
-enter_CFI_mode:
 /* Enter CFI query mode */
 pfl->wcycle = WCYCLE_CFI;
 pfl->cmd = 0x98;
@@ -427,9 +433,15 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* Unlock bypass reset */
 goto reset_flash;
 }
-/* We can enter CFI query mode from autoselect mode */
+/*
+ * We can enter CFI query mode from autoselect mode, but we must
+ * return to autoselect mode after a reset.
+ */
 if (masked_addr == 0x55 && cmd == 0x98) {
-goto enter_CFI_mode;
+/* Enter autoselect CFI query mode */
+pfl->wcycle = WCYCLE_AUTOSELECT_CFI;
+pfl->cmd = 0x98;
+return;
 }
 /* No break here */
 default:
@@ -510,6 +522,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 break;
 case WCYCLE_CFI: /* Special value for CFI queries */
+case WCYCLE_AUTOSELECT_CFI:
 DPRINTF("%s: invalid write in CFI query mode\n", __func__);
 goto reset_flash;
 default:
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index dc85783a6a..ae1cd4e54b 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -437,6 +437,39 @@ static void test_geometry(const void *opaque)
 qtest_quit(global_qtest);
 }
 
+/*
+ * Test that
+ * 1. enter autoselect mode;
+ * 2. enter CFI mode; and then
+ * 3. exit CFI mode
+ * leaves the flash device in autoselect mode.
+ */
+static void test_cfi_in_autoselect(const void *opaque)
+{
+const FlashConfig *c = opaque;
+global_qtest = qtest_initf("-M musicpal,accel=qtest"
+   " -drive if=pflash,file=%s,format=raw,"
+   "copy-on-read",
+   image_path);
+
+/* 1. Enter autoselect. */
+unlock(c);
+flash_cmd(c, UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+/* 2. Enter CFI. */
+flash_cmd(c, CFI_ADDR, CFI_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x10)), ==, replicate(c, 'Q'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x11)), ==, replicate(c, 'R'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x12)), ==, replicate(c, 'Y'));
+
+/* 3. Exit CFI. */
+reset(c);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+qtest_quit(global_qtest);
+}
+
 static void cleanup(void *opaque)
 {
 unlink(image_path);
@@ -562,6 +595,9 @@ int main(int argc, char **argv)
 qtest_add_data_func(path, config, test_geometry);
 g_free(path);
 }
+
+qtest_add_data_func("pflash-cfi02/cfi-in-autoselect", &configuration[0],
+test_cfi_in_autoselect);
 int result = g_test_run();
 cleanup(NULL);
 return result;
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v2 05/10] block/pflash_cfi02: Implement nonuniform sector sizes

Some flash chips support sectors of different sizes. For example, the
AMD AM29LV160DT has 31 64 kB sectors, one 32 kB sector, two 8 kB
sectors, and a 16 kB sector, in that order. The AM29LV160DB has those in
the reverse order.

The `num-blocks` and `sector-length` properties work exactly as they did
before: a flash device with uniform sector lengths. To get non-uniform
sector lengths for up to four regions, the following properties may be
set
- region 0. `num-blocks0` and `sector-length0`;
- region 1. `num-blocks1` and `sector-length1`;
- region 2. `num-blocks2` and `sector-length2`; and
- region 3. `num-blocks3` and `sector-length3`.

If the uniform and nonuniform properties are set, then both must specify
a flash device with the same total size. It would be better to disallow
both being set, or make `num-blocks0` and `sector-length0` alias
`num-blocks` and `sector-length`, but that would make testing currently
impossible.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 177 ++---
 tests/pflash-cfi02-test.c | 179 +-
 2 files changed, 265 insertions(+), 91 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 101628b4ec..c4efbe8cdf 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
  * It does not implement multiple sectors erase
@@ -55,6 +54,13 @@ do {   \
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/*
+ * The size of the cfi_table indirectly depends on this and the start of the
+ * PRI table directly depends on it. 4 is the maximum size (and also what
+ * seems common) without changing the PRT table address.
+ */
+#define PFLASH_MAX_ERASE_REGIONS 4
+
 /* Special write cycle for CFI queries. */
 #define WCYCLE_CFI 7
 
@@ -64,8 +70,10 @@ struct PFlashCFI02 {
 /*< public >*/
 
 BlockBackend *blk;
-uint32_t sector_len;
-uint32_t nb_blocs;
+uint32_t uniform_nb_blocs;
+uint32_t uniform_sector_len;
+uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
+uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
 uint64_t total_len;
 uint64_t interleave_multiplier;
 uint8_t mappings;
@@ -86,7 +94,7 @@ struct PFlashCFI02 {
 uint16_t ident3;
 uint16_t unlock_addr0;
 uint16_t unlock_addr1;
-uint8_t cfi_table[0x52];
+uint8_t cfi_table[0x4D];
 QEMUTimer timer;
 /* The device replicates the flash memory across its memory space.  Emulate
  * that by having a container (.mem) filled with an array of aliases
@@ -189,6 +197,25 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
+/*
+ * offset should be a byte offset of the QEMU device and _not_ a device
+ * offset.
+ */
+static uint32_t pflash_sector_len(PFlashCFI02 *pfl, hwaddr offset)
+{
+assert(offset < pfl->total_len);
+int nb_regions = pfl->cfi_table[0x2C];
+hwaddr addr = 0;
+for (int i = 0; i < nb_regions; ++i) {
+uint64_t region_size = (uint64_t)pfl->nb_blocs[i] * pfl->sector_len[i];
+if (addr <= offset && offset < addr + region_size) {
+return pfl->sector_len[i];
+}
+addr += region_size;
+}
+abort();
+}
+
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
@@ -285,6 +312,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
+uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -446,12 +474,14 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 case 0x30:
 /* Sector erase */
 p = pfl->storage;
-offset &= ~(pfl->sector_len - 1);
-DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
-offset);
+sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
 if (!pfl->ro) {
-memset(p + offset, 0xFF, pfl->sector_len);
-pflash_update(pfl, offset, pfl->sector_len);
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
 }
 set_dq7(pfl, 0x00);
 /* Let's wait 1/2 second before sector erase is do

[Qemu-devel] [PATCH v2 09/10] block/pflash_cfi02: Implement erase suspend/resume

During a sector erase (but not a chip erase), the embeded erase program
can be suspended. Once suspended, the sectors not selected for erasure
may be read and programmed. Autoselect mode is allowed during erase
suspend mode. Presumably, CFI queries are similarly allowed so this
commit allows them as well.

Since guest firmware can use status bits DQ7, DQ6, DQ3, and DQ2 to
determine the current state of sector erasure, these bits are properly
implemented.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 153 ++
 tests/pflash-cfi02-test.c | 102 +
 2 files changed, 241 insertions(+), 14 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 21ceb0823b..d9087cafff 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -29,7 +29,6 @@
  * - CFI queries
  *
  * It does not implement software data protection as found in many real chips
- * It does not implement erase suspend/resume commands
  */
 
 #include "qemu/osdep.h"
@@ -37,6 +36,7 @@
 #include "hw/block/block.h"
 #include "hw/block/flash.h"
 #include "qapi/error.h"
+#include "qemu/bitmap.h"
 #include "qemu/timer.h"
 #include "sysemu/block-backend.h"
 #include "qemu/host-utils.h"
@@ -72,6 +72,7 @@ struct PFlashCFI02 {
 BlockBackend *blk;
 uint32_t uniform_nb_blocs;
 uint32_t uniform_sector_len;
+uint32_t total_sectors;
 uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
 uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
 uint64_t total_len;
@@ -106,6 +107,8 @@ struct PFlashCFI02 {
 int rom_mode;
 int read_counter; /* used for lazy switch-back to rom mode */
 int sectors_to_erase;
+uint64_t erase_time_remaining;
+unsigned long *sector_erase_map;
 char *name;
 void *storage;
 };
@@ -152,6 +155,14 @@ static inline void reset_dq3(PFlashCFI02 *pfl)
 pfl->status &= ~(pfl->interleave_multiplier * 0x08);
 }
 
+/*
+ * Toggle status bit DQ2.
+ */
+static inline void toggle_dq2(PFlashCFI02 *pfl)
+{
+pfl->status ^= pfl->interleave_multiplier * 0x04;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -175,6 +186,29 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
+/*
+ * Returns the time it takes to erase the number of sectors scheduled for
+ * erasure based on CFI address 0x21 which is "Typical timeout per individual
+ * block erase 2^N ms."
+ */
+static uint64_t pflash_erase_time(PFlashCFI02 *pfl)
+{
+/*
+ * If there are no sectors to erase (which can happen if all of the sectors
+ * to be erased are protected), then erase takes 100 us. Protected sectors
+ * aren't supported so this should never happen.
+ */
+return ((1ULL << pfl->cfi_table[0x21]) * pfl->sectors_to_erase) * SCALE_US;
+}
+
+/*
+ * Returns true if the device is currently in erase suspend mode.
+ */
+static inline bool pflash_erase_suspend_mode(PFlashCFI02 *pfl)
+{
+return pfl->erase_time_remaining > 0;
+}
+
 static void pflash_timer(void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
@@ -189,12 +223,7 @@ static void pflash_timer(void *opaque)
  */
 if ((pfl->status & 0x08) == 0) {
 assert_dq3(pfl);
-/*
- * CFI address 0x21 is "Typical timeout per individual block erase
- * 2^N ms"
- */
-uint64_t timeout = ((1ULL << pfl->cfi_table[0x21]) *
-pfl->sectors_to_erase) * 100;
+uint64_t timeout = pflash_erase_time(pfl);
 timer_mod(&pfl->timer,
   qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
 DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
@@ -202,6 +231,7 @@ static void pflash_timer(void *opaque)
 return;
 }
 DPRINTF("%s: sector erase complete\n", __func__);
+bitmap_zero(pfl->sector_erase_map, pfl->total_sectors);
 pfl->sectors_to_erase = 0;
 reset_dq3(pfl);
 }
@@ -240,25 +270,45 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
+typedef struct {
+uint32_t len;
+uint32_t num;
+} SectorInfo;
+
 /*
  * offset should be a byte offset of the QEMU device and _not_ a device
  * offset.
  */
-static uint32_t pflash_sector_len(PFlashCFI02 *pfl, hwaddr offset)
+static SectorInfo pflash_sector_info(PFlashCFI02 *pfl, hwaddr offset)
 {
 assert(offset < pfl->total_len);
 int nb_regions = pfl->cfi_table[0x2C];
 hwaddr addr = 0;
+uint32_t sector_num = 0;
 for (int i = 0; i < nb_regions; ++i) {
 uint64_t region_size = (uint64_t)pfl->nb_blocs[i] * pfl->sector_len[i];
 if (addr <= offset &&a

[Qemu-devel] [PATCH v2 10/10] block/pflash_cfi02: Use the chip erase time specified in the CFI table

When erasing the chip, use the typical time specified in the CFI table
rather than arbitrarily selecting 5 seconds.

Since the currently unconfigurable value set in the table is 12, this
means a chip erase takes 4096 ms so this isn't a big change in behavior.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index d9087cafff..76c8af4365 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -633,9 +633,9 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pflash_update(pfl, 0, pfl->total_len);
 }
 set_dq7(pfl, 0x00);
-/* Let's wait 5 seconds before chip erase is done */
+/* Wait the time specified at CFI address 0x22. */
 timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-  (NANOSECONDS_PER_SECOND * 5));
+  (1ULL << pfl->cfi_table[0x22]) * SCALE_MS);
 break;
 case 0x30:
 /* Sector erase */
-- 
2.20.1 (Apple Git-117)

Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands

2019-04-09 Thread Stephen Checkoway

On Apr 9, 2019, at 02:13, Thomas Huth  wrote:

> We'd like to get rid of global_qtest in the long run (since it is
> causing trouble for tests that run multiple instances of QEMU in
> parallel, e.g. migration tests)... so if it is feasible, please don't
> use it in new code anymore. Try to use a local variable in the function
> that call qtest_initf() and pass the test state around via a parameter
> to the functions that need it.

One of the patches introduces a FlashConfig structure which gets passed around 
to all the functions. I could stuff the local qtest in there. The earlier 
patches would still use global_qtest. Would that be acceptable or would you 
prefer that global_qtest not appear in any of the patches?

Or rather than putting it in the FlashConfig struct, I could just pass a 
separate parameter around. Whatever you prefer.

-- 
Stephen Checkoway

Re: [Qemu-devel] [PATCH 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality

2019-04-09 Thread Stephen Checkoway

Hi Phil,

> On Apr 9, 2019, at 06:34, Philippe Mathieu-Daudé  wrote:
> 
> Hi Stephen,
> 
> [Cc'ing Markus and Laszlo, we have similar interest in pflash01 testing]
> 
> On 4/8/19 10:55 PM, Stephen Checkoway wrote:
>> The goal of this patch series implement the following AMD command-set 
>> parallel
>> flash functionality:
>> - flash interleaving;
>> - nonuniform sector sizes;
>> - erase suspend/resume commands; and
>> - multi-sector erase.
> 
> I am very glad you addressed these long overdue issues and very pleased
> by your patches :)
> I'll thoroughly review it during next week (this won't get merge for the
> current 4.0 cycle anyway).

Great!

> I started a similar cleanup (mostly pflash01 focused) and converted
> DPRINTF to trace events, added few constants. I'll see if I can rebase
> your work on top of mine. So far only your patch 2 (refactor) would be
> modified, simplified as the "pull out all of the code to modify the
> status into simple helper functions" part (which else I'd ask you to
> move in a separate patch).
> 
> We should think of more intereleaved tests, I'll prepare a table of
> current QEMU models using this device and how is their intereleave
> mapping. Hopefully it would be enough to simply add the existing
> machines to your current musicpal qtest.
> 
> Also I'd like to see some Top/Bottom block configuration qtests, your
> patch #5 doesn't seem tested.

I included four tests 
<https://patchew.org/QEMU/cover.1554774454.git.stephen.checko...@oberlin.edu/bbe417c354dfa908d5e1bc8f8ad7121ec9451682.1554774454.git.stephen.checko...@oberlin.edu/>.

Patchew makes it hard to link to particular lines. Here's the same patch on 
Github 
<https://github.com/qemu/qemu/commit/a851d21347121a91ba9a39c133a8fbee6e84e557#diff-d2ed797ca8898de80768bdfb390781dfR498>.

Are those sufficient or would you like more tests?

> 
>> During refactoring and implementation, I discovered several bugs that are
>> fixed here as well:
>> - flash commands use only 11-bits of the address in most cases, but the
>>  current code uses all of them [1];
>> - entering CFI mode from autoselect mode and then exiting CFI mode should
>>  return the chip to autoselect mode, but the current code returns to read
>>  array mode; and
>> - reset command should be ignored during sector/chip erase, but the current
>>  code performs the reset.
>> 
>> The first patch in the series adds a test for the existing behavior. Tests 
>> for
>> additional behavior/bug fixes are added in the relevant patch.
>> 
>> 1. I found firmware in the wild that relies on the 11-bit address behavior,
>>   probably due to a bug in the firmware itself.
> 
> Is it a musicpal firmware? Are you able to compare with real hardware?

No, it's not musicpal. I'm not even sure what musicpal is, it was just the most 
convenient choice for testing.  The real hardware is an embedded system using 
an old AMD processor (Am486) with three different AMD command set flash chips. 
The unlock addresses the firmware uses don't actually match the part sheets (in 
most cases). It took quite a while to realize that the flash hardware only uses 
11 bits of address. (It's clearly spelled out in the various part sheets, but I 
guess I missed it on the first 15 or so readings.)

> I vaguely remember some issue regarding address bus width when trying to
> implement the TopBlock small sectors, but I wasn't using the musicpal.
> I'll see if I can find my old notes and test with your series.

The one boot chip I'm using is the AM29F080B-90SI with top boot blocks. It has 
an 8-bit address bus.

-- 
Stephen Checkoway

Re: [Qemu-devel] [PATCH 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality

2019-04-09 Thread Stephen Checkoway

On Apr 9, 2019, at 12:15, Philippe Mathieu-Daudé  wrote:

> Since you did changes in the CFI table, I think we should add a tests
> verifying the table is correctly generated for all you FlashConfig entries.

That's a good idea. Some of the values are essentially arbitrary (e.g., how 
long an erase typically takes) but the CFI values that indicate support for 
erase/suspend and erase regions at least should be tested. I'll take a closer 
look at what all I changed (it has been a few months since I wrote the code). 
Are there any in particular you think I should be sure to test? Do you want me 
to add those tests to the appropriate patches in the series or would you like 
an additional patch that adds those tests? (The later is easier to do as 
modifying the earlier patches in the series are likely to cause conflicts with 
later patches that I'd need to resolve, but I can do either.)

> I suppose you can not share the firmware binary. Can you share these
> unlock addresses? Maybe once I reviewed carefully your series I might
> ask you the (pflash) trace event output.

I probably cannot share the binary but the unlock addresses are 0x1554 and 
0x2AA8. This is for two interleaved x8/x16 chips in x16 mode so shifting right 
by 2 (1 for the interleaving and 1 because it's an x16 chip) gives 0x555 and 
0xAAA. The appropriate (word) unlock addresses for the chips are 0x555 and 
0x2AA which is what you get if you restrict to 11 bits.

My guess is that the firmware authors took the byte unlock addresses (0xAAA and 
0x555), shifted left by 2, discovered that this didn't work, tried the unlock 
addresses in the opposite order, and found that that worked due to the chips 
only looking at 11 bits.

Looking at hw/arm/musicpal.c, I see that it is using unlock addresses 0x 
and 0x2AAA. My guess is this reflects a bug in some firmware and it should be 
using 0x555 and 0x2AA. I haven't seen any AMD pflash chips that used other 
unlock addresses, but I've only looked at about a dozen part sheets and I'm not 
sure what chips the musicpal is actually using.

-- 
Stephen Checkoway

Re: [Qemu-devel] [PATCH] hw/char/escc: Lower irq when transmit buffer is filled

2019-04-10 Thread Stephen Checkoway




On Apr 10, 2019, at 16:01, Philippe Mathieu-Daudé  wrote:

> On 3/6/19 12:01 PM, Paolo Bonzini wrote:
>> On 05/03/19 06:10, Stephen Checkoway wrote:
>>> The SCC/ESCC will briefly stop asserting an interrupt when the
>>> transmit FIFO is filled.
>>> 
>>> This code doesn't model the transmit FIFO/shift register so the
>>> pending transmit interrupt is never deasserted which means that an
>>> edge-triggered interrupt controller will never see the low-to-high
>>> transition it needs to raise another interrupt. The practical
>>> consequence of this is that guest firmware with an interrupt service
>>> routine for the ESCC that does not send all of the data it has
>>> immediately will stop sending data if the following sequence of
>>> events occurs:
>>> 1. Disable processor interrupts
>>> 2. Write a character to the ESCC
>>> 3. Add additional characters to a buffer which is drained by the ISR
>>> 4. Enable processor interrupts
>>> 
>>> In this case, the first character will be sent, the interrupt will
>>> fire and the ISR will output the second character. Since the pending
>>> transmit interrupt remains asserted, no additional interrupts will
>>> ever fire.
>>> 
>>> This fixes that situation by explicitly lowering the IRQ when a
>>> character is written to the buffer.
>>> 
>>> Signed-off-by: Stephen Checkoway 
>> 
>> Looks good but I would like Mark to give his ack as well.
>> 
>> Mark, could you also add hw/char/escc.c to both SPARC and Mac sections
>> of MAINTAINERS?
> 
> Cc'ing Artyom who also made some changes in this file, and Laurent.
> 
> 
> Stephen, which particular chipset are you using?

I'm away from the hardware, but my notes say Z80C30. I've included the image 
from the board if you want to try to decipher the rest of the part number. My 
best guess is it's a Z85C3008VEC. If it's important, I can ask someone who is a 
few thousand km closer to the hardware than I currently am to take a look.



> This file models various types. I had a talk with Mark or Laurent at
> last KVM forum about this device. IIRC, while the sun4m machines use a
> real chipset, the MacIO embeds an slighly modified IP core.
> 
> I can't find what you describe in the Z85C30 doc, however in the ESCC
> datasheet referenced in escc.c (which happens to be a 85MiB pdf!!!) I found:

(This chip is ridiculously configurable, I honestly found it much easier to 
write a driver by reverse engineering existing firmware than by reading the 
part sheet.)

> 
>  Transmit Interrupts and Transmit Buffer Empty Bit on the NMOS/CMOS
> 
>  The TxIP is reset either by writing data to the transmit buffer or
>  by issuing the Reset Tx Int command in WR0.
> 
> I understand the NMOS/CMOS desc. matches the Z85C30 (no FIFO used).

It has been a little while since I was last looking at this, but my 
recollection was the SCC has a 1-byte transmit buffer and the ESCC has a small 
(4 maybe?) byte transmit buffer. But in either case writing data to the 
transmit buffer should clear the interrupt.

> So your description and patch makes sens.
> What worries me is the controller could have other pending IRQs to
> deliver and you are clearing them. Shouldn't we only clear the
> INTR_TXINT bit, and call escc_update_irq() which should lower the IRQ if
> no bits are pending?
> 
> Maybe as:
> 
>s->wregs[W_INTR] &= ~INTR_TXINT;
>escc_update_irq(s);

Ah, I think I see. In this case, escc_update_irq() will lower the IRQ if no 
other interrupts are pending and the set_txint() will raise it again. 
Otherwise, it'll remain raised. I can give this a shot and see how it goes. (I 
think this should be R_INTR instead of W_INTR, but I'll double check once I 
have an opportunity to dig into this again.)

Thanks for looking at this!

Steve


-- 
Stephen Checkoway

Re: [Qemu-devel] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit

2019-07-03 Thread Stephen Checkoway




> On Jul 1, 2019, at 20:59, Philippe Mathieu-Daudé  wrote:
> 
> Parallel NOR flashes are limited to 16-bit bus accesses.

I don't think this is correct. The CFI spec defines an x32 interface for 
parallel NOR. CFI addresses 0x28 and 0x29 specify the interface and value 3 is 
x32 and value 5 is x16/x32.

Here's an example of an x32 device 
<https://www.mouser.com/datasheet/2/100/002-00948_29CD032J_S29CD016J_S29CL032J_S29CL016J_3-1316792.pdf>.

Cheers,

Steve

> Remove the 32-bit dead code.
> 
> Reviewed-by: Alistair Francis 
> Message-Id: <20190627202719.17739-29-phi...@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> hw/block/pflash_cfi02.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 83084b9d72..5392290c72 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -317,8 +317,6 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
> unsigned int width)
> boff = offset & 0xFF;
> if (pfl->width == 2) {
> boff = boff >> 1;
> -} else if (pfl->width == 4) {
> -boff = boff >> 2;
> }
> switch (pfl->cmd) {
> default:
> @@ -449,8 +447,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
> uint64_t value,
> boff = offset;
> if (pfl->width == 2) {
> boff = boff >> 1;
> -} else if (pfl->width == 4) {
> -boff = boff >> 2;
> }
> /* Only the least-significant 11 bits are used in most cases. */
> boff &= 0x7FF;
> @@ -710,6 +706,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
> uint64_t value,
> static const MemoryRegionOps pflash_cfi02_ops = {
> .read = pflash_read,
>     .write = pflash_write,
> +.impl.max_access_size = 2,
> .valid.min_access_size = 1,
> .valid.max_access_size = 4,
> .endianness = DEVICE_NATIVE_ENDIAN,
> -- 
> 2.20.1
> 

-- 
Stephen Checkoway

Re: [Qemu-devel] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit

2019-07-03 Thread Stephen Checkoway




> On Jul 3, 2019, at 12:02, Philippe Mathieu-Daudé  wrote:
> 
> On 7/3/19 5:52 PM, Stephen Checkoway wrote:
>> 
>> 
>>> On Jul 1, 2019, at 20:59, Philippe Mathieu-Daudé  wrote:
>>> 
>>> Parallel NOR flashes are limited to 16-bit bus accesses.
>> 
>> I don't think this is correct. The CFI spec defines an x32 interface for 
>> parallel NOR. CFI addresses 0x28 and 0x29 specify the interface and value 3 
>> is x32 and value 5 is x16/x32.
>> 
>> Here's an example of an x32 device 
>> <https://www.mouser.com/datasheet/2/100/002-00948_29CD032J_S29CD016J_S29CL032J_S29CL016J_3-1316792.pdf>.
> 
> OK, I was not aware of these.
> 
> QEMU never CFI-announced itself as x32 capable:
> 
>/* Flash device interface (8 & 16 bits) */
>pfl->cfi_table[0x28] = 0x02;
>pfl->cfi_table[0x29] = 0x00;
> 
> So while the commit description is incorrect, the code is safe with the
> current device model.
> 
> I am not comfortable keeping untested 32-bit mode.
> Were you using it?

I'm not using it. I did have some code to set these CFI values based on the 
parameters used to control the interleaving 
<https://github.com/stevecheckoway/qemu/commit/f9a79a6e18b2c7c5a05e344ff554a7d980a56042#diff-d33881bd0ef099e2f46ebd4797c653bcR599>.

In general, a better testing harness would be nice though.

Steve

> 
> Regards,
> 
> Phil.
> 
>> Cheers,
>> 
>> Steve
>> 
>>> Remove the 32-bit dead code.
>>> 
>>> Reviewed-by: Alistair Francis 
>>> Message-Id: <20190627202719.17739-29-phi...@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>> hw/block/pflash_cfi02.c | 5 +
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>> 
>>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>>> index 83084b9d72..5392290c72 100644
>>> --- a/hw/block/pflash_cfi02.c
>>> +++ b/hw/block/pflash_cfi02.c
>>> @@ -317,8 +317,6 @@ static uint64_t pflash_read(void *opaque, hwaddr 
>>> offset, unsigned int width)
>>>boff = offset & 0xFF;
>>>if (pfl->width == 2) {
>>>boff = boff >> 1;
>>> -} else if (pfl->width == 4) {
>>> -boff = boff >> 2;
>>>}
>>>switch (pfl->cmd) {
>>>default:
>>> @@ -449,8 +447,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
>>> uint64_t value,
>>>boff = offset;
>>>if (pfl->width == 2) {
>>>boff = boff >> 1;
>>> -} else if (pfl->width == 4) {
>>> -boff = boff >> 2;
>>>}
>>>/* Only the least-significant 11 bits are used in most cases. */
>>>boff &= 0x7FF;
>>> @@ -710,6 +706,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
>>> uint64_t value,
>>> static const MemoryRegionOps pflash_cfi02_ops = {
>>>.read = pflash_read,
>>>.write = pflash_write,
>>> +.impl.max_access_size = 2,
>>>.valid.min_access_size = 1,
>>>.valid.max_access_size = 4,
>>>.endianness = DEVICE_NATIVE_ENDIAN,
>>> -- 
>>> 2.20.1

-- 
Stephen Checkoway

Re: [Qemu-devel] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit

2019-07-08 Thread Stephen Checkoway




> On Jul 4, 2019, at 08:45, Philippe Mathieu-Daudé  wrote:
> 
> Cc'ing PPC/taihu_405ep and ARM/Digic4 maintainers.
> 
> On 7/3/19 6:36 PM, Philippe Mathieu-Daudé wrote:
>> On 7/3/19 6:20 PM, Stephen Checkoway wrote:
>>>> On Jul 3, 2019, at 12:02, Philippe Mathieu-Daudé  wrote:
>>>> On 7/3/19 5:52 PM, Stephen Checkoway wrote:
>>>>> 
>>>>> 
>>>>>> On Jul 1, 2019, at 20:59, Philippe Mathieu-Daudé  
>>>>>> wrote:
>>>>>> 
>>>>>> Parallel NOR flashes are limited to 16-bit bus accesses.
>>>>> 
>>>>> I don't think this is correct. The CFI spec defines an x32 interface for 
>>>>> parallel NOR. CFI addresses 0x28 and 0x29 specify the interface and value 
>>>>> 3 is x32 and value 5 is x16/x32.
>>>>> 
>>>>> Here's an example of an x32 device 
>>>>> <https://www.mouser.com/datasheet/2/100/002-00948_29CD032J_S29CD016J_S29CL032J_S29CL016J_3-1316792.pdf>.
>>>> 
>>>> OK, I was not aware of these.
>>>> 
>>>> QEMU never CFI-announced itself as x32 capable:
>>>> 
>>>>   /* Flash device interface (8 & 16 bits) */
>>>>   pfl->cfi_table[0x28] = 0x02;
>>>>   pfl->cfi_table[0x29] = 0x00;
>>>> 
>>>> So while the commit description is incorrect, the code is safe with the
>>>> current device model.
>>>> 
>>>> I am not comfortable keeping untested 32-bit mode.
>>>> Were you using it?
>>> 
>>> I'm not using it. I did have some code to set these CFI values based on the 
>>> parameters used to control the interleaving 
>>> <https://github.com/stevecheckoway/qemu/commit/f9a79a6e18b2c7c5a05e344ff554a7d980a56042#diff-d33881bd0ef099e2f46ebd4797c653bcR599>.
>>> 
>>> In general, a better testing harness would be nice though.
>> 
>> We can revert it if it helps you.
> 
> So after further analysis, there are not guest visible changes, because
> 32-bit accesses are still valid (.valid.max_access_size = 4) but now
> they are processed as two 16-bit accesses, shifted by
> access_with_adjusted_size().
> Well, I haven't checked (yet) when the guest and the flash are in
> different endianess, and I wish we don't use that.
> 
> Now I see 2 different guests "registering" the flash in 32-bit access:
> 
> - PPC/taihu_405ep
> 
>  The CFI id matches the S29AL008J that is a 1MB in x16, while the code
>  QEMU forces it to be 2MB, and checking Linux it expects a 4MB flash
>  there, Yay \o/
> 
> - ARM/Digic4
> 
>  While the comment says "Samsung K8P3215UQB 64M Bit (4Mx16)", this
>  flash is 32Mb (2MB). Also note the CFI id does not match the comment.
>  Again, Yay.
> 
> Anyway, better safe than sorry, so I'll revert.
> 
> Thanks for following and catching this,

Great, thanks!

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

2019-02-18 Thread Stephen Checkoway




On Feb 18, 2019, at 13:08, Markus Armbruster  wrote:

> Stephen Checkoway  writes:
> 
>> On Feb 18, 2019, at 08:43, Thomas Huth  wrote:
>> 
>>> On 18/02/2019 07.07, Stephen Checkoway wrote:
>>>> Hi all,
>>>> 
>>>> I've been working on some improvements to the pflash_cfi02 block device 
>>>> (interleaved flash devices similar to pflash_cfi01, multi-sector erase, 
>>>> nonuniform sector sizes, and some bug fixes and I'm planning on 
>>>> implementing sector erase suspend/resume commands in the near future).
> 
> Any chance you could do multiple region support, too?

Can you point me at the data sheet for a flash chip with multiple region 
support? For my purposes, I only need the features I mentioned, but if it's a 
simple change, I'll consider it.

>>> QTestState *qts;
>>> qts = qtest_initf(" qemu-system-arm -M musicpal,accel=qtest "
>>>   "-drive if=pflash,file=%s,format=raw", filename);
>> 
>> If I do that, will it be possible for the test to override the properties 
>> set by pflash_cfi02_register? It looks like I should be able to use -global 
>> to set properties that aren't set explicitly.
> 
> Yes.
> 
> Won't work for properties set by pflash_cfi02_register(), though.  To
> test the full range of values there, you'd have to make them
> configurable somehow.  We currently don't have a good way to do that.
> Please see
> 
>Subject: Re: Configuring pflash devices for OVMF firmware
>Message-ID: <87mun8gd2x@dusky.pond.sub.org>
>https://lists.nongnu.org/archive/html/qemu-devel/2019-02/msg01734.html

I see. That's too bad.

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

2019-02-19 Thread Stephen Checkoway

On Feb 19, 2019, at 01:09, Markus Armbruster  wrote:

> Stephen Checkoway  writes:
> 
>> On Feb 18, 2019, at 13:08, Markus Armbruster  wrote:
>> 
>>> Any chance you could do multiple region support, too?
>> 
>> Can you point me at the data sheet for a flash chip with multiple region 
>> support? For my purposes, I only need the features I mentioned, but if it's 
>> a simple change, I'll consider it.
> 
> I'm not familiar with CFI pflash, but I can operate a search engine.
> Have a look at page 27 and 56 of
> 
> https://media.digikey.com/pdf/Data%20Sheets/Intel%20PDFs/28F160C3,320C3,640C3,800C3%20(x16).pdf
> 
> and tell us whether it's helpful.

That's a flash chip that uses the Intel command set (pflash_cfi01.c in qemu), 
I'm only touching the AMD command set (pflash_cfi02.c). Also those pages seem 
to be about block locking, not multiple regions.

I'm not entirely sure what you meant by multiple regions. If you meant blocks* 
having different sizes (usually in the top or bottom of the flash as boot 
blocks), then I've implemented that (the nonuniform sectors* I mentioned in my 
first email).

* Some flash chips differentiate between sectors and blocks. E.g., the flash 
chip used by hw/arm/musicpal.c 
<http://ww1.microchip.com/downloads/en/DeviceDoc/SST39VF6401B-SST39VF6402B-64-Mbit-x16-Multi-Purpose-Flash-Plus-Data-Sheet-DS20005008C.pdf>
 has sectors and blocks with separate erase commands. Qemu treats these the 
same and does not support separate erase commands.

> I think a test would be quite welcome even if it only tests what's
> testable now with reasonable effort.

I used Thomas Huth's suggestion for testing the autoselect, CFI, sector erase, 
chip erase, programming, and reset commands. I'll see how much I can test.

-- 
Stephen Checkoway

[Qemu-devel] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free

2019-02-19 Thread Stephen Checkoway

Don't dynamically allocate the pflash's timer. But do use timer_del in
an unrealize function to make sure that the timer can't fire after the
pflash_t has been freed.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 0f8b7b8c7b..1588aeff5a 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -84,7 +84,7 @@ struct pflash_t {
 uint16_t unlock_addr0;
 uint16_t unlock_addr1;
 uint8_t cfi_table[0x52];
-QEMUTimer *timer;
+QEMUTimer timer;
 /* The device replicates the flash memory across its memory space.  Emulate
  * that by having a container (.mem) filled with an array of aliases
  * (.mem_mappings) pointing to the flash memory (.orig_mem).
@@ -429,7 +429,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
 }
 pfl->status = 0x00;
 /* Let's wait 5 seconds before chip erase is done */
-timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
   (NANOSECONDS_PER_SECOND * 5));
 break;
 case 0x30:
@@ -444,7 +444,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
 }
 pfl->status = 0x00;
 /* Let's wait 1/2 second before sector erase is done */
-timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
   (NANOSECONDS_PER_SECOND / 2));
 break;
 default:
@@ -596,7 +596,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 pfl->rom_mode = 1;
 sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
-pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
+timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
 pfl->wcycle = 0;
 pfl->cmd = 0;
 pfl->status = 0;
@@ -695,11 +695,18 @@ static Property pflash_cfi02_properties[] = {
 DEFINE_PROP_END_OF_LIST(),
 };
 
+static void pflash_cfi02_unrealize(DeviceState *dev, Error **errp)
+{
+pflash_t *pfl = CFI_PFLASH02(dev);
+timer_del(&pfl->timer);
+}
+
 static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
 dc->realize = pflash_cfi02_realize;
+dc->unrealize = pflash_cfi02_unrealize;
 dc->props = pflash_cfi02_properties;
 set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
-- 
2.17.2 (Apple Git-113)

Re: [Qemu-devel] Testing sysbus devices

2019-02-19 Thread Stephen Checkoway

> On Feb 19, 2019, at 10:28, Markus Armbruster  wrote:
> 
> My terminology might be confused...
> 
> Let me backtrack a bit an explain my use case.  On physical PCs, the
> single flash chip is commonly configured to have a read-only part and a
> read/write part.  The read-only part holds UEFI code, and the read-write
> part holds its persistent state.
> 
> Since our virtual flash chips lack this feature, our virtual PCs have
> *two* of them: one configured read-only, and one configured read/write.
> Cleaning that up would be nice.
> 
> The comment "It does not implement software data protection as found in
> many real chips" in both pflash_cfi0*.c might be referring to this
> missing feature.

I understand now, thank you for explaining. I noticed the comments about 
software data protection in the code, but I didn't investigate.

From a quick look at <https://www.cypress.com/file/195291/download> Table 27 on 
page 8, I see there are at least 4 different protection modes. I think the most 
common one (based on my reading of a handful of data sheets for flash chips) is 
the high voltage one. Essentially, there are sector groups that can be 
locked/unlocked using high voltage. It seems easy enough to model this by 
configuring sectors as locked and refusing to erase or program them.

Software command locking would probably involve implementing a few additional 
commands.

I'm not sure what the others are.

Which locking method do you need?

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

2019-02-21 Thread Stephen Checkoway

> On Feb 20, 2019, at 03:55, Laszlo Ersek wrote:
>
> I would strongly prefer if the guest-side view wouldn't change at all.

It sounds like sector protection isn't something you want and it's not
something I currently need so unless that changes, I probably won't do anything
with it.

My goal is merely to implement some missing flash functionality that I need to
emulate some hardware that I have. My plan for doing this is to not change any
defaults (except for a few bug fixes) while doing so. I'm happy for the qemu
community to take as much or as little as it finds useful.

I'll send a patch series for review in the normal fashion, but if anyone wants
to see my in-progress work, including tests, the diff is available here
<https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02>.

For my own edification, I'm curious how you're currently dealing with some
regions of flash that are protected. I believe Markus mentioned using multiple
flash devices. Are you overlapping the address ranges?

The current pflash_cfi02.c code assumes, but doesn't check that both the total
size of the chip as well as the size of each sector is a power of two. If you
wanted say 7 MB of read/write flash and 1 MB of read-only flash, qemu might be
willing to create a device with say 7 MB of storage, but it will definitely
misbehave. (I added a check for that here
<https://github.com/qemu/qemu/compare/master...stevecheckoway:pflash02#diff-d33881bd0ef099e2f46ebd4797c653bcR738>.)

Cheers,

Steve

--
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

2019-02-22 Thread Stephen Checkoway




> On Feb 22, 2019, at 02:55, Laszlo Ersek  wrote:
> 
> OVMF and the q35/i440fx boards use cfi01. The 4KB sector size is assumed
> by both QEMU board code and OVMF. The 4KB sector size is not assumed (to
> my knowledge) by cfi01.pflash device model code.
> 
> Regarding the full size of each cfi01.pflash chip, i440fx/q35 board code
> determines that dynamically from the file size (the only requirement is
> that the size be a multiple of the sector size). There is no assumption
> in the device model. In OVMF, the assumed/expected full size is
> hard-coded (build-time constant).

Gotcha, thanks for the info. It sounds like the code for the Intel command set 
chips (cfi01) and the AMD command set chips (cfi02) have different requirements 
for their use.

Regards,

Steve

-- 
Stephen Checkoway

Re: [Qemu-devel] Testing sysbus devices

2019-02-22 Thread Stephen Checkoway

> On Feb 22, 2019, at 02:42, Markus Armbruster  wrote:
> 
> Awesome.  The magic setup code in hw/i386/pc_sysfw.c will happily create
> any size that's a multiple of 4KiB.  The current sizes are 128KiB
> writable (power of two, good) and 2MiB - 128KiB for read-only (very much
> not a power of two, possibly bad).

As far as I can tell, the code I'm touching does not even compile by default 
for the i386 which only supports flash chips with the Intel command set 
hw/block/pflash_cfi01.c and not the flash chips with the AMD command set 
hw/block/pflash_cfi02.c.

> Can you tell us a bit more about what exactly can go wrong?

I don't know anything at all about the pflash_cfi01.c code. So if it's working 
now, then it's probably fine.

If you had been using pflash_cfi02.c, the problem would be that it has support 
for mapping the chip at multiple, consecutive locations (I'm not sure why 
that's part of the device itself and not something setup by code that's using 
the device). So device accesses mask offsets in the read and write functions by 
pfl->chip_len - 1 (here 
<https://github.com/qemu/qemu/blob/master/hw/block/pflash_cfi02.c#L154> and 
here <https://github.com/qemu/qemu/blob/master/hw/block/pflash_cfi02.c#L279>). 
If the chip size isn't a power of two, this breaks.

-- 
Stephen Checkoway

[Qemu-devel] x86 segment limits enforcement with TCG

2019-02-24 Thread Stephen Checkoway

6`tcg_cpu_exec(cpu=0x000105043c00) at cpus.c:1429
frame #10: 0x00010006b4b7 
qemu-system-i386`qemu_tcg_cpu_thread_fn(arg=0x000105043c00) at cpus.c:1733
frame #11: 0x0001006223ec 
qemu-system-i386`qemu_thread_start(args=0x000103d0e770) at 
qemu-thread-posix.c:502
frame #12: 0x7fff6b507305 libsystem_pthread.dylib`_pthread_body + 126
frame #13: 0x7fff6b50a26f libsystem_pthread.dylib`_pthread_start + 70
frame #14: 0x7fff6b506415 libsystem_pthread.dylib`thread_start + 13

diff --git a/target/i386/translate.c b/target/i386/translate.c
index 49cd298374..6f3682a268 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -76,6 +76,7 @@ static TCGv cpu_cc_dst, cpu_cc_src, cpu_cc_src2;
 static TCGv_i32 cpu_cc_op;
 static TCGv cpu_regs[CPU_NB_REGS];
 static TCGv cpu_seg_base[6];
+static TCGv cpu_seg_limit[6];
 static TCGv_i64 cpu_bndl[4];
 static TCGv_i64 cpu_bndu[4];

@@ -130,6 +131,7 @@ typedef struct DisasContext {
 /* TCG local temps */
 TCGv cc_srcT;
 TCGv A0;
+TCGv A1;
 TCGv T0;
 TCGv T1;

@@ -448,6 +450,8 @@ static inline void gen_jmp_im(DisasContext *s, target_ulong 
pc)
 gen_op_jmp_v(s->tmp0);
 }

+static void gen_exception(DisasContext *s, int trapno, target_ulong cur_eip);
+
 /* Compute SEG:REG into A0.  SEG is selected from the override segment
(OVR_SEG) and the default segment (DEF_SEG).  OVR_SEG may be -1 to
indicate no override.  */
@@ -491,6 +495,23 @@ static void gen_lea_v_seg(DisasContext *s, TCGMemOp aflag, 
TCGv a0,

 if (ovr_seg >= 0) {
 TCGv seg = cpu_seg_base[ovr_seg];
+TCGv limit = cpu_seg_limit[ovr_seg];
+
+if (s->pe && !CODE64(s)) {
+/*
+ * Check that the access is within the segment limit in protected
+ * mode.
+ */
+TCGLabel *label = gen_new_label();
+int access_limit = (1 << (aflag & MO_SIZE)) - 1;
+tcg_gen_mov_tl(s->A1, a0);
+if (access_limit > 0) {
+tcg_gen_addi_tl(s->A1, s->A1, access_limit);
+}
+tcg_gen_brcond_tl(TCG_COND_LE, s->A1, limit, label);
+gen_exception(s, EXCP0D_GPF, s->pc_start - s->cs_base);
+gen_set_label(label);
+}

 if (aflag == MO_64) {
 tcg_gen_add_tl(s->A0, a0, seg);
@@ -8382,6 +8403,14 @@ void tcg_x86_init(void)
 [R_GS] = "gs_base",
 [R_SS] = "ss_base",
 };
+static const char seg_limit_names[6][9] = {
+[R_CS] = "cs_limit",
+[R_DS] = "ds_limit",
+[R_ES] = "es_limit",
+[R_FS] = "fs_limit",
+[R_GS] = "gs_limit",
+[R_SS] = "ss_limit",
+};
 static const char bnd_regl_names[4][8] = {
 "bnd0_lb", "bnd1_lb", "bnd2_lb", "bnd3_lb"
 };
@@ -8410,6 +8439,10 @@ void tcg_x86_init(void)
 = tcg_global_mem_new(cpu_env,
  offsetof(CPUX86State, segs[i].base),
  seg_base_names[i]);
+cpu_seg_limit[i]
+= tcg_global_mem_new(cpu_env,
+ offsetof(CPUX86State, segs[i].limit),
+ seg_limit_names[i]);
 }

 for (i = 0; i < 4; ++i) {
@@ -8482,6 +8515,7 @@ static void i386_tr_init_disas_context(DisasContextBase 
*dcbase, CPUState *cpu)
 dc->T0 = tcg_temp_new();
 dc->T1 = tcg_temp_new();
 dc->A0 = tcg_temp_new();
+dc->A1 = tcg_temp_new();

 dc->tmp0 = tcg_temp_new();
 dc->tmp1_i64 = tcg_temp_new_i64();

-- 
Stephen Checkoway

Re: [Qemu-devel] x86 segment limits enforcement with TCG

2019-02-24 Thread Stephen Checkoway




> On Feb 24, 2019, at 14:46, Peter Maydell  wrote:
> 
> On Sun, 24 Feb 2019 at 19:37, Stephen Checkoway
>  wrote:
>> I think that something about adding the tcg_gen_brcond_tl is causing values 
>> to become dead and then qemu aborts.
> 
> Yep -- all "TCG temporaries" are dead at the end
> of a basic block, and brcond ends a basic block.
> Only globals and "local temporaries" stay live
> across brcond. This is documented in tcg/README,
> though it doesn't spell it out very explicitly.

Ah yes. I see that now. I missed it on my first read through.

> This makes brcond pretty painful to use and
> almost impossible to introduce into the middle
> of some existing sequence of generated code.
> I haven't looked at what the best way to do what
> you're trying to do here is, though.

Are there other examples of straight-line code being converted to a conditional 
I might be able to use as an example? I thought INTO would be a good example, 
but it merely calls a helper. Maybe I should do that? I assume it'll be slow, 
but speed isn't really my primary concern.

> By the way, don't do this:
> +dc->A1 = tcg_temp_new();
> 
> The current use of a small number of tcg temps
> in the i386 translate.c code is an antipattern
> that is a relic from a very old version of the
> code. It's much better to simply create new
> temporaries in the code at the point where you
> need them and then free them once you're done.

Great, thanks. I saw both the A0/T0/T1 and the creation of new temporaries and 
I wasn't sure which pattern I should follow.

-- 
Stephen Checkoway

Re: [Qemu-devel] x86 segment limits enforcement with TCG

2019-02-25 Thread Stephen Checkoway

FWIW, I figured out an approach to this. Essentially, I'm reusing the function 
which computes linear addresses to enforce not only segment limits (in 
everything but long mode), but also read/write access (in protected mode).

Unfortunately, that meant every call to the linear address computation has to 
be augmented with an access size and whether it's a store or not. The patch is 
pretty large so I won't include it here, but you can view it at 
<https://github.com/stevecheckoway/qemu/commit/ac58652efacedc53f3831301ea0326ac8f882b18>.

If this is something that qemu would like, then I think two additional things 
are definitely required:
1. Tests. make check passes and the firmware I have which necessitated the 
checks appears to work, but that change touches the almost every 
guest-memory-accessing x86 instruction.
2. This is going to slow down the common case—where segments have a 0 base 
address and a limit of 0x—and there's no real need to do that. It seems 
like something akin to addseg could be used to decide when to insert the 
checks. I don't really understand how that works and in my case, segments with 
nonzero bases and non-0x limits are the norm so I didn't investigate 
that. Something similar could probably be done to omit the writable segment 
checks.

Finally, there are some limitations. The amount of memory touched by xsave (and 
the related instructions) depends on edx:eax. I didn't bother checking that at 
all. Similarly, there are some MPX instructions that don't do any access checks 
when they really should. And finally, there's one place that checks for an 
access size of 8 bytes when, in some cases, it should be 16.

I'm happy to work to upstream this, if the approach is broadly acceptable and 
the functionality is desired.

If not, thank you Peter for answering my questions which enabled me to solve my 
problem.

Cheers,

Steve

> On Feb 24, 2019, at 15:21, Stephen Checkoway  
> wrote:
> 
> 
> 
>> On Feb 24, 2019, at 14:46, Peter Maydell  wrote:
>> 
>> On Sun, 24 Feb 2019 at 19:37, Stephen Checkoway
>>  wrote:
>>> I think that something about adding the tcg_gen_brcond_tl is causing values 
>>> to become dead and then qemu aborts.
>> 
>> Yep -- all "TCG temporaries" are dead at the end
>> of a basic block, and brcond ends a basic block.
>> Only globals and "local temporaries" stay live
>> across brcond. This is documented in tcg/README,
>> though it doesn't spell it out very explicitly.
> 
> Ah yes. I see that now. I missed it on my first read through.
> 
>> This makes brcond pretty painful to use and
>> almost impossible to introduce into the middle
>> of some existing sequence of generated code.
>> I haven't looked at what the best way to do what
>> you're trying to do here is, though.
> 
> Are there other examples of straight-line code being converted to a 
> conditional I might be able to use as an example? I thought INTO would be a 
> good example, but it merely calls a helper. Maybe I should do that? I assume 
> it'll be slow, but speed isn't really my primary concern.
> 
>> By the way, don't do this:
>> +dc->A1 = tcg_temp_new();
>> 
>> The current use of a small number of tcg temps
>> in the i386 translate.c code is an antipattern
>> that is a relic from a very old version of the
>> code. It's much better to simply create new
>> temporaries in the code at the point where you
>> need them and then free them once you're done.
> 
> Great, thanks. I saw both the A0/T0/T1 and the creation of new temporaries 
> and I wasn't sure which pattern I should follow.
> 
> -- 
> Stephen Checkoway
> 
> 
> 
> 
> 

-- 
Stephen Checkoway

[Qemu-devel] [PATCH v2] hw/char/escc: Lower irq when transmit buffer is filled

2019-04-16 Thread Stephen Checkoway

The SCC/ESCC will briefly stop asserting an interrupt when the
transmit FIFO is filled.

This code doesn't model the transmit FIFO/shift register so the
pending transmit interrupt is never deasserted which means that an
edge-triggered interrupt controller will never see the low-to-high
transition it needs to raise another interrupt. The practical
consequence of this is that guest firmware with an interrupt service
routine for the ESCC that does not send all of the data it has
immediately will stop sending data if the following sequence of
events occurs:
1. Disable processor interrupts
2. Write a character to the ESCC
3. Add additional characters to a buffer which is drained by the ISR
4. Enable processor interrupts

In this case, the first character will be sent, the interrupt will
fire and the ISR will output the second character. Since the pending
transmit interrupt remains asserted, no additional interrupts will
ever fire.

This fixes that situation by explicitly lowering the IRQ when a
character is written to the buffer and no other interrupts are currently
pending.

Signed-off-by: Stephen Checkoway 
---
 hw/char/escc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 628f5f81f7..c5b05a63f1 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -509,6 +509,13 @@ static void escc_mem_write(void *opaque, hwaddr addr,
 break;
 case SERIAL_DATA:
 trace_escc_mem_writeb_data(CHN_C(s), val);
+/*
+ * Lower the irq when data is written to the Tx buffer and no other
+ * interrupts are currently pending. The irq will be raised again once
+ * the Tx buffer becomes empty below.
+ */
+s->txint = 0;
+escc_update_irq(s);
 s->tx = val;
 if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
 if (qemu_chr_fe_backend_connected(&s->chr)) {
-- 
2.20.1 (Apple Git-117)

Re: [Qemu-devel] [PATCH] hw/char/escc: Lower irq when transmit buffer is filled

2019-04-16 Thread Stephen Checkoway




> On Apr 10, 2019, at 16:01, Philippe Mathieu-Daudé  wrote:
> 
> So your description and patch makes sens.
> What worries me is the controller could have other pending IRQs to
> deliver and you are clearing them. Shouldn't we only clear the
> INTR_TXINT bit, and call escc_update_irq() which should lower the IRQ if
> no bits are pending?
> 
> Maybe as:
> 
>s->wregs[W_INTR] &= ~INTR_TXINT;
>escc_update_irq(s);

I just sent a v2 patch (but it looks like I failed to include Philippe, sorry 
about that!)

I used

s->txint = 0;
escc_update_irq(s);

which appears to do the right thing in my tests.

Thank you,

Steve

-- 
Stephen Checkoway

[Qemu-devel] [PATCH v3 02/10] block/pflash_cfi02: Refactor, NFC intended

Simplify and refactor for upcoming commits. In particular, pull out all
of the code to modify the status into simple helper functions. Status
handling becomes more complex once multiple chips are interleaved to
produce a single device.

No change in functionality is intended with this commit.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 221 +---
 1 file changed, 95 insertions(+), 126 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f81..4b7af71806 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -46,18 +46,19 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 
-//#define PFLASH_DEBUG
-#ifdef PFLASH_DEBUG
+#define PFLASH_DEBUG false
 #define DPRINTF(fmt, ...)  \
 do {   \
-fprintf(stderr, "PFLASH: " fmt , ## __VA_ARGS__);   \
+if (PFLASH_DEBUG) {\
+fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__);   \
+}  \
 } while (0)
-#else
-#define DPRINTF(fmt, ...) do { } while (0)
-#endif
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/* Special write cycle for CFI queries. */
+#define WCYCLE_CFI 7
+
 struct PFlashCFI02 {
 /*< private >*/
 SysBusDevice parent_obj;
@@ -97,6 +98,31 @@ struct PFlashCFI02 {
 void *storage;
 };
 
+/*
+ * Toggle status bit DQ7.
+ */
+static inline void toggle_dq7(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x80;
+}
+
+/*
+ * Set status bit DQ7 to bit 7 of value.
+ */
+static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+{
+pfl->status &= 0x7F;
+pfl->status |= value & 0x80;
+}
+
+/*
+ * Toggle status bit DQ6.
+ */
+static inline void toggle_dq6(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x40;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -126,7 +152,7 @@ static void pflash_timer (void *opaque)
 
 trace_pflash_timer_expired(pfl->cmd);
 /* Reset flash */
-pfl->status ^= 0x80;
+toggle_dq7(pfl);
 if (pfl->bypass) {
 pfl->wcycle = 2;
 } else {
@@ -136,12 +162,34 @@ static void pflash_timer (void *opaque)
 pfl->cmd = 0;
 }
 
-static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
-int width, int be)
+/*
+ * Read data from flash.
+ */
+static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr offset,
+ unsigned int width)
 {
+uint8_t *p = (uint8_t *)pfl->storage + offset;
+uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
+/* XXX: Need a trace_pflash_data_read(offset, ret, width) */
+switch (width) {
+case 1:
+trace_pflash_data_read8(offset, ret);
+break;
+case 2:
+trace_pflash_data_read16(offset, ret);
+break;
+case 4:
+trace_pflash_data_read32(offset, ret);
+break;
+}
+return ret;
+}
+
+static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
+{
+PFlashCFI02 *pfl = opaque;
 hwaddr boff;
-uint32_t ret;
-uint8_t *p;
+uint64_t ret;
 
 ret = -1;
 trace_pflash_read(offset, pfl->cmd, width, pfl->wcycle);
@@ -166,39 +214,8 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x80:
 /* We accept reads during second unlock sequence... */
 case 0x00:
-flash_read:
 /* Flash area read */
-p = pfl->storage;
-switch (width) {
-case 1:
-ret = p[offset];
-trace_pflash_data_read8(offset, ret);
-break;
-case 2:
-if (be) {
-ret = p[offset] << 8;
-ret |= p[offset + 1];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-}
-trace_pflash_data_read16(offset, ret);
-break;
-case 4:
-if (be) {
-ret = p[offset] << 24;
-ret |= p[offset + 1] << 16;
-ret |= p[offset + 2] << 8;
-ret |= p[offset + 3];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-ret |= p[offset + 2] << 16;
-ret |= p[offset + 3] << 24;
-}
-trace_pflash_data_read32(offset, ret);
-break;
-}
+ret = pflash_data_read(pfl, offset, width);
 break;
 case 0x90:
 /* flash ID read */
@@ -213,23 +230,23 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x0E:
 case 0x0F:
 ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
-if (ret == (uint8_t)-1) {
-goto flash_read;
+

[Qemu-devel] [PATCH v3 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality

The goal of this patch series implement the following AMD command-set parallel
flash functionality:
- flash interleaving;
- nonuniform sector sizes;
- erase suspend/resume commands; and
- multi-sector erase.

During refactoring and implementation, I discovered several bugs that are
fixed here as well:
- flash commands use only 11-bits of the address in most cases, but the
  current code uses all of them [1];
- entering CFI mode from autoselect mode and then exiting CFI mode should
  return the chip to autoselect mode, but the current code returns to read
  array mode; and
- reset command should be ignored during sector/chip erase, but the current
  code performs the reset.

The first patch in the series adds a test for the existing behavior. Tests for
additional behavior/bug fixes are added in the relevant patch.

1. I found firmware in the wild that relies on the 11-bit address behavior,
   probably due to a bug in the firmware itself.

Changes from v1:
- Fix missing spaces around *, -, and ?;
- Fix missing Signed-off-by line on patch 7; and
- Replace use of errc with g_printerr and exit.

Changes from v2:
- Remove global_qtest from tests; and
- Test the CFI table changes.

Stephen Checkoway (10):
  block/pflash_cfi02: Add test for supported commands
  block/pflash_cfi02: Refactor, NFC intended
  block/pflash_cfi02: Fix command address comparison
  block/pflash_cfi02: Implement intereleaved flash devices
  block/pflash_cfi02: Implement nonuniform sector sizes
  block/pflash_cfi02: Fix CFI in autoselect mode
  block/pflash_cfi02: Fix reset command not ignored during erase
  block/pflash_cfi02: Implement multi-sector erase
  block/pflash_cfi02: Implement erase suspend/resume
  block/pflash_cfi02: Use the chip erase time specified in the CFI table

 hw/block/pflash_cfi02.c   | 843 +++---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 815 
 3 files changed, 1423 insertions(+), 237 deletions(-)
 create mode 100644 tests/pflash-cfi02-test.c

-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v3 06/10] block/pflash_cfi02: Fix CFI in autoselect mode

After a flash device enters CFI mode from autoselect mode, the reset
command returns the device to autoselect mode. An additional reset
command is necessary to return to read array mode.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 21 +
 tests/pflash-cfi02-test.c | 39 +++
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index c4efbe8cdf..be10036886 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -61,8 +61,9 @@ do {   \
  */
 #define PFLASH_MAX_ERASE_REGIONS 4
 
-/* Special write cycle for CFI queries. */
+/* Special write cycles for CFI queries. */
 #define WCYCLE_CFI 7
+#define WCYCLE_AUTOSELECT_CFI 8
 
 struct PFlashCFI02 {
 /*< private >*/
@@ -325,6 +326,12 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 
 if (cmd == 0xF0) {
+if (pfl->wcycle == WCYCLE_AUTOSELECT_CFI) {
+/* Return to autoselect mode. */
+pfl->wcycle = 3;
+pfl->cmd = 0x90;
+return;
+}
 goto reset_flash;
 }
 }
@@ -350,7 +357,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* We're in read mode */
 check_unlock0:
 if (masked_addr == 0x55 && cmd == 0x98) {
-enter_CFI_mode:
 /* Enter CFI query mode */
 pfl->wcycle = WCYCLE_CFI;
 pfl->cmd = 0x98;
@@ -427,9 +433,15 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* Unlock bypass reset */
 goto reset_flash;
 }
-/* We can enter CFI query mode from autoselect mode */
+/*
+ * We can enter CFI query mode from autoselect mode, but we must
+ * return to autoselect mode after a reset.
+ */
 if (masked_addr == 0x55 && cmd == 0x98) {
-goto enter_CFI_mode;
+/* Enter autoselect CFI query mode */
+pfl->wcycle = WCYCLE_AUTOSELECT_CFI;
+pfl->cmd = 0x98;
+return;
 }
 /* No break here */
 default:
@@ -510,6 +522,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 break;
 case WCYCLE_CFI: /* Special value for CFI queries */
+case WCYCLE_AUTOSELECT_CFI:
 DPRINTF("%s: invalid write in CFI query mode\n", __func__);
 goto reset_flash;
 default:
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index 82bc5695e1..4039647604 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -480,6 +480,42 @@ static void test_geometry(const void *opaque)
 qtest_quit(qtest);
 }
 
+/*
+ * Test that
+ * 1. enter autoselect mode;
+ * 2. enter CFI mode; and then
+ * 3. exit CFI mode
+ * leaves the flash device in autoselect mode.
+ */
+static void test_cfi_in_autoselect(const void *opaque)
+{
+const FlashConfig *config = opaque;
+QTestState *qtest = qtest_initf("-M musicpal,accel=qtest"
+" -drive if=pflash,file=%s,format=raw,"
+"copy-on-read",
+image_path);
+FlashConfig explicit_config = expand_config_defaults(config);
+explicit_config.qtest = qtest;
+const FlashConfig *c = &explicit_config;
+
+/* 1. Enter autoselect. */
+unlock(c);
+flash_cmd(c, UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+/* 2. Enter CFI. */
+flash_cmd(c, CFI_ADDR, CFI_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x10)), ==, replicate(c, 'Q'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x11)), ==, replicate(c, 'R'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x12)), ==, replicate(c, 'Y'));
+
+/* 3. Exit CFI. */
+reset(c);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+qtest_quit(qtest);
+}
+
 static void cleanup(void *opaque)
 {
 unlink(image_path);
@@ -605,6 +641,9 @@ int main(int argc, char **argv)
 qtest_add_data_func(path, config, test_geometry);
 g_free(path);
 }
+
+qtest_add_data_func("pflash-cfi02/cfi-in-autoselect", &configuration[0],
+test_cfi_in_autoselect);
 int result = g_test_run();
 cleanup(NULL);
 return result;
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v3 05/10] block/pflash_cfi02: Implement nonuniform sector sizes

Some flash chips support sectors of different sizes. For example, the
AMD AM29LV160DT has 31 64 kB sectors, one 32 kB sector, two 8 kB
sectors, and a 16 kB sector, in that order. The AM29LV160DB has those in
the reverse order.

The `num-blocks` and `sector-length` properties work exactly as they did
before: a flash device with uniform sector lengths. To get non-uniform
sector lengths for up to four regions, the following properties may be
set
- region 0. `num-blocks0` and `sector-length0`;
- region 1. `num-blocks1` and `sector-length1`;
- region 2. `num-blocks2` and `sector-length2`; and
- region 3. `num-blocks3` and `sector-length3`.

If the uniform and nonuniform properties are set, then both must specify
a flash device with the same total size. It would be better to disallow
both being set, or make `num-blocks0` and `sector-length0` alias
`num-blocks` and `sector-length`, but that would make testing currently
impossible.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 177 +---
 tests/pflash-cfi02-test.c | 185 --
 2 files changed, 265 insertions(+), 97 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 101628b4ec..c4efbe8cdf 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
  * It does not implement multiple sectors erase
@@ -55,6 +54,13 @@ do {   \
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/*
+ * The size of the cfi_table indirectly depends on this and the start of the
+ * PRI table directly depends on it. 4 is the maximum size (and also what
+ * seems common) without changing the PRT table address.
+ */
+#define PFLASH_MAX_ERASE_REGIONS 4
+
 /* Special write cycle for CFI queries. */
 #define WCYCLE_CFI 7
 
@@ -64,8 +70,10 @@ struct PFlashCFI02 {
 /*< public >*/
 
 BlockBackend *blk;
-uint32_t sector_len;
-uint32_t nb_blocs;
+uint32_t uniform_nb_blocs;
+uint32_t uniform_sector_len;
+uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
+uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
 uint64_t total_len;
 uint64_t interleave_multiplier;
 uint8_t mappings;
@@ -86,7 +94,7 @@ struct PFlashCFI02 {
 uint16_t ident3;
 uint16_t unlock_addr0;
 uint16_t unlock_addr1;
-uint8_t cfi_table[0x52];
+uint8_t cfi_table[0x4D];
 QEMUTimer timer;
 /* The device replicates the flash memory across its memory space.  Emulate
  * that by having a container (.mem) filled with an array of aliases
@@ -189,6 +197,25 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
+/*
+ * offset should be a byte offset of the QEMU device and _not_ a device
+ * offset.
+ */
+static uint32_t pflash_sector_len(PFlashCFI02 *pfl, hwaddr offset)
+{
+assert(offset < pfl->total_len);
+int nb_regions = pfl->cfi_table[0x2C];
+hwaddr addr = 0;
+for (int i = 0; i < nb_regions; ++i) {
+uint64_t region_size = (uint64_t)pfl->nb_blocs[i] * pfl->sector_len[i];
+if (addr <= offset && offset < addr + region_size) {
+return pfl->sector_len[i];
+}
+addr += region_size;
+}
+abort();
+}
+
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
@@ -285,6 +312,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
+uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -446,12 +474,14 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 case 0x30:
 /* Sector erase */
 p = pfl->storage;
-offset &= ~(pfl->sector_len - 1);
-DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
-offset);
+sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
 if (!pfl->ro) {
-memset(p + offset, 0xFF, pfl->sector_len);
-pflash_update(pfl, offset, pfl->sector_len);
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
 }
 set_dq7(pfl, 0x00);
 /* Let's wait 1/2 second before sector erase is do

[Qemu-devel] [PATCH v3 01/10] block/pflash_cfi02: Add test for supported commands

Test the AMD command set for parallel flash chips. This test uses an
ARM musicpal board with a pflash drive to test the following list of
currently-supported commands.
- Autoselect
- CFI
- Sector erase
- Chip erase
- Program
- Unlock bypass
- Reset

Signed-off-by: Stephen Checkoway 
---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 227 ++
 2 files changed, 229 insertions(+)
 create mode 100644 tests/pflash-cfi02-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 6b904d7430..0a26eacce0 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -263,6 +263,7 @@ check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/hexloader-test$(EXESUF)
+check-qtest-arm-$(CONFIG_PFLASH_CFI02) += tests/pflash-cfi02-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
@@ -773,6 +774,7 @@ tests/device-introspect-test$(EXESUF): 
tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/hexloader-test$(EXESUF): tests/hexloader-test.o
+tests/pflash-cfi02$(EXESUF): tests/pflash-cfi02-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
 tests/rtas-test$(EXESUF): tests/rtas-test.o $(libqos-spapr-obj-y)
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
new file mode 100644
index 00..b113fca5af
--- /dev/null
+++ b/tests/pflash-cfi02-test.c
@@ -0,0 +1,227 @@
+/*
+ * QTest testcase for parallel flash with AMD command set
+ *
+ * Copyright (c) 2018 Stephen Checkoway
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include 
+#include "libqtest.h"
+
+/*
+ * To test the pflash_cfi02 device, we run QEMU with the musicpal machine with
+ * a pflash drive. This enables us to test some flash configurations, but not
+ * all. In particular, we're limited to a 16-bit wide flash device.
+ */
+
+#define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
+#define BASE_ADDR (0x1ULL - MP_FLASH_SIZE_MAX)
+
+#define FLASH_WIDTH 2
+#define CFI_ADDR (FLASH_WIDTH * 0x55)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+
+#define CFI_CMD 0x98
+#define UNLOCK0_CMD 0xAA
+#define UNLOCK1_CMD 0x55
+#define AUTOSELECT_CMD 0x90
+#define RESET_CMD 0xF0
+#define PROGRAM_CMD 0xA0
+#define SECTOR_ERASE_CMD 0x30
+#define CHIP_ERASE_CMD 0x10
+#define UNLOCK_BYPASS_CMD 0x20
+#define UNLOCK_BYPASS_RESET_CMD 0x00
+
+static char image_path[] = "/tmp/qtest.XX";
+
+static inline void flash_write(uint64_t byte_addr, uint16_t data)
+{
+qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
+}
+
+static inline uint16_t flash_read(uint64_t byte_addr)
+{
+return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
+}
+
+static void unlock(void)
+{
+flash_write(UNLOCK0_ADDR, UNLOCK0_CMD);
+flash_write(UNLOCK1_ADDR, UNLOCK1_CMD);
+}
+
+static void reset(void)
+{
+flash_write(0, RESET_CMD);
+}
+
+static void sector_erase(uint64_t byte_addr)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(byte_addr, SECTOR_ERASE_CMD);
+}
+
+static void wait_for_completion(uint64_t byte_addr)
+{
+/* If DQ6 is toggling, step the clock and ensure the toggle stops. */
+if ((flash_read(byte_addr) & 0x40) ^ (flash_read(byte_addr) & 0x40)) {
+/* Wait for erase or program to finish. */
+clock_step_next();
+/* Ensure that DQ6 has stopped toggling. */
+g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
+}
+}
+
+static void bypass_program(uint64_t byte_addr, uint16_t data)
+{
+flash_write(UNLOCK0_ADDR, PROGRAM_CMD);
+flash_write(byte_addr, data);
+/*
+ * Data isn't valid until DQ6 stops toggling. We don't model this as
+ * writes are immediate, but if this changes in the future, we can wait
+ * until the program is complete.
+ */
+wait_for_completion(byte_addr);
+}
+
+static void program(uint64_t byte_addr, uint16_t data)
+{
+unlock();
+bypass_program(byte_addr, data);
+}
+
+static void chip_erase(void)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(UNLOCK0_ADDR, SECTOR_ERASE_CMD);
+}
+
+static void test_flash(void)
+{
+global_qtest = qtest_initf("-M musicpal,accel=qtest "
+   "-drive 
if=pflash,file=%s,format=raw,copy-on-read",
+   image_path);
+/* Check the IDs. */
+unlock();
+flash_write(UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert_cmpint(flash

[Qemu-devel] [PATCH v3 09/10] block/pflash_cfi02: Implement erase suspend/resume

During a sector erase (but not a chip erase), the embeded erase program
can be suspended. Once suspended, the sectors not selected for erasure
may be read and programmed. Autoselect mode is allowed during erase
suspend mode. Presumably, CFI queries are similarly allowed so this
commit allows them as well.

Since guest firmware can use status bits DQ7, DQ6, DQ3, and DQ2 to
determine the current state of sector erasure, these bits are properly
implemented.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 153 ++
 tests/pflash-cfi02-test.c | 112 
 2 files changed, 251 insertions(+), 14 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 21ceb0823b..d9087cafff 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -29,7 +29,6 @@
  * - CFI queries
  *
  * It does not implement software data protection as found in many real chips
- * It does not implement erase suspend/resume commands
  */
 
 #include "qemu/osdep.h"
@@ -37,6 +36,7 @@
 #include "hw/block/block.h"
 #include "hw/block/flash.h"
 #include "qapi/error.h"
+#include "qemu/bitmap.h"
 #include "qemu/timer.h"
 #include "sysemu/block-backend.h"
 #include "qemu/host-utils.h"
@@ -72,6 +72,7 @@ struct PFlashCFI02 {
 BlockBackend *blk;
 uint32_t uniform_nb_blocs;
 uint32_t uniform_sector_len;
+uint32_t total_sectors;
 uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
 uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
 uint64_t total_len;
@@ -106,6 +107,8 @@ struct PFlashCFI02 {
 int rom_mode;
 int read_counter; /* used for lazy switch-back to rom mode */
 int sectors_to_erase;
+uint64_t erase_time_remaining;
+unsigned long *sector_erase_map;
 char *name;
 void *storage;
 };
@@ -152,6 +155,14 @@ static inline void reset_dq3(PFlashCFI02 *pfl)
 pfl->status &= ~(pfl->interleave_multiplier * 0x08);
 }
 
+/*
+ * Toggle status bit DQ2.
+ */
+static inline void toggle_dq2(PFlashCFI02 *pfl)
+{
+pfl->status ^= pfl->interleave_multiplier * 0x04;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -175,6 +186,29 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
+/*
+ * Returns the time it takes to erase the number of sectors scheduled for
+ * erasure based on CFI address 0x21 which is "Typical timeout per individual
+ * block erase 2^N ms."
+ */
+static uint64_t pflash_erase_time(PFlashCFI02 *pfl)
+{
+/*
+ * If there are no sectors to erase (which can happen if all of the sectors
+ * to be erased are protected), then erase takes 100 us. Protected sectors
+ * aren't supported so this should never happen.
+ */
+return ((1ULL << pfl->cfi_table[0x21]) * pfl->sectors_to_erase) * SCALE_US;
+}
+
+/*
+ * Returns true if the device is currently in erase suspend mode.
+ */
+static inline bool pflash_erase_suspend_mode(PFlashCFI02 *pfl)
+{
+return pfl->erase_time_remaining > 0;
+}
+
 static void pflash_timer(void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
@@ -189,12 +223,7 @@ static void pflash_timer(void *opaque)
  */
 if ((pfl->status & 0x08) == 0) {
 assert_dq3(pfl);
-/*
- * CFI address 0x21 is "Typical timeout per individual block erase
- * 2^N ms"
- */
-uint64_t timeout = ((1ULL << pfl->cfi_table[0x21]) *
-pfl->sectors_to_erase) * 100;
+uint64_t timeout = pflash_erase_time(pfl);
 timer_mod(&pfl->timer,
   qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
 DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
@@ -202,6 +231,7 @@ static void pflash_timer(void *opaque)
 return;
 }
 DPRINTF("%s: sector erase complete\n", __func__);
+bitmap_zero(pfl->sector_erase_map, pfl->total_sectors);
 pfl->sectors_to_erase = 0;
 reset_dq3(pfl);
 }
@@ -240,25 +270,45 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
+typedef struct {
+uint32_t len;
+uint32_t num;
+} SectorInfo;
+
 /*
  * offset should be a byte offset of the QEMU device and _not_ a device
  * offset.
  */
-static uint32_t pflash_sector_len(PFlashCFI02 *pfl, hwaddr offset)
+static SectorInfo pflash_sector_info(PFlashCFI02 *pfl, hwaddr offset)
 {
 assert(offset < pfl->total_len);
 int nb_regions = pfl->cfi_table[0x2C];
 hwaddr addr = 0;
+uint32_t sector_num = 0;
 for (int i = 0; i < nb_regions; ++i) {
 uint64_t region_size = (uint64_t)pfl->nb_blocs[i] * pfl->sector_len[i];
 if (addr <= offset &&a

[Qemu-devel] [PATCH v3 10/10] block/pflash_cfi02: Use the chip erase time specified in the CFI table

When erasing the chip, use the typical time specified in the CFI table
rather than arbitrarily selecting 5 seconds.

Since the currently unconfigurable value set in the table is 12, this
means a chip erase takes 4096 ms so this isn't a big change in behavior.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index d9087cafff..76c8af4365 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -633,9 +633,9 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pflash_update(pfl, 0, pfl->total_len);
 }
 set_dq7(pfl, 0x00);
-/* Let's wait 5 seconds before chip erase is done */
+/* Wait the time specified at CFI address 0x22. */
 timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-  (NANOSECONDS_PER_SECOND * 5));
+  (1ULL << pfl->cfi_table[0x22]) * SCALE_MS);
 break;
 case 0x30:
 /* Sector erase */
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v3 08/10] block/pflash_cfi02: Implement multi-sector erase

After two unlock cycles and a sector erase command, the AMD flash chips
start a 50 us erase time out. Any additional sector erase commands add a
sector to be erased and restart the 50 us timeout. During the timeout,
status bit DQ3 is cleared. After the time out, DQ3 is asserted during
erasure.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 94 +++
 tests/pflash-cfi02-test.c | 59 ++--
 2 files changed, 131 insertions(+), 22 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index cb1160eb35..21ceb0823b 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -30,7 +30,6 @@
  *
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
- * It does not implement multiple sectors erase
  */
 
 #include "qemu/osdep.h"
@@ -106,6 +105,7 @@ struct PFlashCFI02 {
 MemoryRegion orig_mem;
 int rom_mode;
 int read_counter; /* used for lazy switch-back to rom mode */
+int sectors_to_erase;
 char *name;
 void *storage;
 };
@@ -136,6 +136,22 @@ static inline void toggle_dq6(PFlashCFI02 *pfl)
 pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
+/*
+ * Turn on DQ3.
+ */
+static inline void assert_dq3(PFlashCFI02 *pfl)
+{
+pfl->status |= pfl->interleave_multiplier * 0x08;
+}
+
+/*
+ * Turn off DQ3.
+ */
+static inline void reset_dq3(PFlashCFI02 *pfl)
+{
+pfl->status &= ~(pfl->interleave_multiplier * 0x08);
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -159,11 +175,37 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
-static void pflash_timer (void *opaque)
+static void pflash_timer(void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
 
 trace_pflash_timer_expired(pfl->cmd);
+if (pfl->cmd == 0x30) {
+/*
+ * Sector erase. If DQ3 is 0 when the timer expires, then the 50
+ * us erase timeout has expired so we need to start the timer for the
+ * sector erase algorithm. Otherwise, the erase completed and we should
+ * go back to read array mode.
+ */
+if ((pfl->status & 0x08) == 0) {
+assert_dq3(pfl);
+/*
+ * CFI address 0x21 is "Typical timeout per individual block erase
+ * 2^N ms"
+ */
+uint64_t timeout = ((1ULL << pfl->cfi_table[0x21]) *
+pfl->sectors_to_erase) * 100;
+timer_mod(&pfl->timer,
+  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
+DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
+__func__, pfl->sectors_to_erase);
+return;
+}
+DPRINTF("%s: sector erase complete\n", __func__);
+pfl->sectors_to_erase = 0;
+reset_dq3(pfl);
+}
+
 /* Reset flash */
 toggle_dq7(pfl);
 if (pfl->bypass) {
@@ -307,13 +349,30 @@ static void pflash_update(PFlashCFI02 *pfl, int offset, 
int size)
 }
 }
 
+static void pflash_sector_erase(PFlashCFI02 *pfl, hwaddr offset)
+{
+uint64_t sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
+if (!pfl->ro) {
+uint8_t *p = pfl->storage;
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
+}
+set_dq7(pfl, 0x00);
+++pfl->sectors_to_erase;
+/* Set (or reset) the 50 us timer for additional erase commands.  */
+timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 5);
+}
+
 static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
  unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
-uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -486,20 +545,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 break;
 case 0x30:
 /* Sector erase */
-p = pfl->storage;
-sector_len = pflash_sector_len(pfl, offset);
-offset &= ~(sector_len - 1);
-DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
-__func__, pfl->bank_width * 2, offset,
-pfl->bank_width * 2, offset + sector_len - 1);
-if (!pfl->ro) {
-memset(p + offset, 0xFF, sector_len);
-pflash_update(pfl, offset, sector_len);
-}
-set_

[Qemu-devel] [PATCH v3 03/10] block/pflash_cfi02: Fix command address comparison

Most AMD commands only examine 11 bits of the address. This masks the
addresses used in the comparison to 11 bits. The exceptions are word or
sector addresses which use offset directly rather than the shifted
offset, boff.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   |  8 +++-
 tests/pflash-cfi02-test.c | 12 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4b7af71806..e4bff0c8f8 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -296,11 +296,13 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 
 DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n", __func__,
 offset, value, width);
-boff = offset & (pfl->sector_len - 1);
+boff = offset;
 if (pfl->width == 2)
 boff = boff >> 1;
 else if (pfl->width == 4)
 boff = boff >> 2;
+/* Only the least-significant 11 bits are used in most cases. */
+boff &= 0x7FF;
 switch (pfl->wcycle) {
 case 0:
 /* Set the device in I/O access mode if required */
@@ -519,6 +521,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
+/* Only 11 bits are used in the comparison. */
+pfl->unlock_addr0 &= 0x7FF;
+pfl->unlock_addr1 &= 0x7FF;
+
 chip_len = pfl->sector_len * pfl->nb_blocs;
 
 memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index b113fca5af..b91bb66a79 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -23,8 +23,8 @@
 
 #define FLASH_WIDTH 2
 #define CFI_ADDR (FLASH_WIDTH * 0x55)
-#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
-#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x555)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AA)
 
 #define CFI_CMD 0x98
 #define UNLOCK0_CMD 0xAA
@@ -192,6 +192,14 @@ static void test_flash(void)
 g_assert_cmpint(flash_read(6), ==, 0xCDEF);
 g_assert_cmpint(flash_read(8), ==, 0x);
 
+/* Test ignored high order bits of address. */
+flash_write(FLASH_WIDTH * 0x, UNLOCK0_CMD);
+flash_write(FLASH_WIDTH * 0x2AAA, UNLOCK1_CMD);
+flash_write(FLASH_WIDTH * 0x, AUTOSELECT_CMD);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x), ==, 0x00BF);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x0001), ==, 0x236D);
+reset();
+
 qtest_quit(global_qtest);
 }
 
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v3 04/10] block/pflash_cfi02: Implement intereleaved flash devices

It's common for multiple narrow flash chips to be hooked up in parallel
to support wider buses. For example, four 8-bit wide flash chips (x8)
may be combined in parallel to produce a 32-bit wide device. Similarly,
two 16-bit wide chips (x16) may be combined.

This commit introduces `device-width` and `max-device-width` properties,
similar to pflash_cfi01, with the following meanings:
- `width`: The width of the logical, qemu device (same as before);
- `device-width`: The width of an individual flash chip, defaulting to
  `width`; and
- `max-device-width`: The maximum width of an individual flash chip,
  defaulting to `device-width`.

Nothing needs to change to support reading such interleaved devices but
commands (e.g., erase and programming) must be sent to all devices at
the same time or else the various chips will be in different states.

For example, a 4-byte wide logical device can be composed of four x8/x16
devices in x8 mode. That is, each device supports both x8 or x16 and
they're being used in the byte, rather than word, mode. This
configuration would have `width=4`, `device-width=1`, and
`max-device-width=2`.

In addition to commands being sent to all devices, guest firmware
expects the status and CFI queries to be replicated for each device.
(The one exception to the response replication is that each device gets
to report its own status bit DQ7 while programming because its value
depends on the value being programmed which will usually differ for each
device.)

Testing is limited to 16-bit wide devices due to the current inability
to override the properties set by `pflash_cfi02_register`, but multiple
configurations are tested.

Stop using global_qtest. Instead, package the qtest variable inside the
FlashConfig structure.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c   | 270 +++--
 tests/pflash-cfi02-test.c | 477 ++
 2 files changed, 577 insertions(+), 170 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index e4bff0c8f8..101628b4ec 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not support flash interleaving.
  * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
@@ -67,15 +66,19 @@ struct PFlashCFI02 {
 BlockBackend *blk;
 uint32_t sector_len;
 uint32_t nb_blocs;
-uint32_t chip_len;
+uint64_t total_len;
+uint64_t interleave_multiplier;
 uint8_t mappings;
-uint8_t width;
+uint8_t bank_width; /* Width of the QEMU device in bytes. */
+uint8_t device_width; /* Width of individual pflash chip. */
+uint8_t max_device_width; /* Maximum width of individual pflash chip. */
 uint8_t be;
+int device_shift; /* Amount to shift an offset to get a device address. */
 int wcycle; /* if 0, the flash is read normally */
 int bypass;
 int ro;
 uint8_t cmd;
-uint8_t status;
+uint64_t status;
 /* FIXME: implement array device properties */
 uint16_t ident0;
 uint16_t ident1;
@@ -103,16 +106,17 @@ struct PFlashCFI02 {
  */
 static inline void toggle_dq7(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x80;
+pfl->status ^= pfl->interleave_multiplier * 0x80;
 }
 
 /*
  * Set status bit DQ7 to bit 7 of value.
  */
-static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+static inline void set_dq7(PFlashCFI02 *pfl, uint64_t value)
 {
-pfl->status &= 0x7F;
-pfl->status |= value & 0x80;
+uint64_t mask = pfl->interleave_multiplier * 0x80;
+pfl->status &= ~mask;
+pfl->status |= value & mask;
 }
 
 /*
@@ -120,7 +124,7 @@ static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
  */
 static inline void toggle_dq6(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x40;
+pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
 /*
@@ -188,7 +192,6 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
-hwaddr boff;
 uint64_t ret;
 
 ret = -1;
@@ -198,12 +201,10 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
unsigned int width)
 ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
 pflash_register_memory(pfl, 1);
 }
-offset &= pfl->chip_len - 1;
-boff = offset & 0xFF;
-if (pfl->width == 2)
-boff = boff >> 1;
-else if (pfl->width == 4)
-boff = boff >> 2;
+/* Mask by the total length of the chip to account for alias mappings. */
+offset &= pfl->total_len - 1;
+hwaddr device_addr = offset >> pfl->device_shift;
+
 switch (pfl->cmd) {
 default:
 /* This should

[Qemu-devel] [PATCH v3 07/10] block/pflash_cfi02: Fix reset command not ignored during erase

When the flash device is performing a chip erase, all commands are
ignored. When it is performing a sector erase, only the erase suspend
command is valid, which is currently not supported.

In particular, the reset command should not cause the device to reset to
read array mode while programming is on going.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index be10036886..cb1160eb35 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -325,7 +325,8 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pfl->bank_width * 2, value);
 }
 
-if (cmd == 0xF0) {
+/* Reset does nothing during chip erase and sector erase. */
+if (cmd == 0xF0 && pfl->cmd != 0x10 && pfl->cmd != 0x30) {
 if (pfl->wcycle == WCYCLE_AUTOSELECT_CFI) {
 /* Return to autoselect mode. */
 pfl->wcycle = 3;
-- 
2.20.1 (Apple Git-117)

Re: [Qemu-devel] [PATCH v3 01/10] block/pflash_cfi02: Add test for supported commands

2019-04-18 Thread Stephen Checkoway




> On Apr 18, 2019, at 00:47, Thomas Huth  wrote:
> 
> On 18/04/2019 00.01, Stephen Checkoway wrote:
>> Test the AMD command set for parallel flash chips. This test uses an
>> ARM musicpal board with a pflash drive to test the following list of
>> currently-supported commands.
>> - Autoselect
>> - CFI
>> - Sector erase
>> - Chip erase
>> - Program
>> - Unlock bypass
>> - Reset
>> 
>> Signed-off-by: Stephen Checkoway 
>> ---
>> tests/Makefile.include|   2 +
>> tests/pflash-cfi02-test.c | 227 ++
>> 2 files changed, 229 insertions(+)
>> create mode 100644 tests/pflash-cfi02-test.c
> [...]
>> new file mode 100644
>> index 00..b113fca5af
>> --- /dev/null
>> +++ b/tests/pflash-cfi02-test.c
>> @@ -0,0 +1,227 @@
>> +/*
>> + * QTest testcase for parallel flash with AMD command set
>> + *
>> + * Copyright (c) 2018 Stephen Checkoway
> 
> Do you maybe want to update that to 2019?

Done.

> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include 
> 
> We generally don't use err.h in QEMU ... could you please use the
> standard functions from glib instead?

Done.

> +#include 
> 
> unistd.h is already provided by osdep.h, so you don't need to include it
> here again. (and the scripts/clean-includes script will barf at this
> later, so better fix it right from the start)

Done.

> +#include "libqtest.h"
>> +
>> +/*
>> + * To test the pflash_cfi02 device, we run QEMU with the musicpal machine 
>> with
>> + * a pflash drive. This enables us to test some flash configurations, but 
>> not
>> + * all. In particular, we're limited to a 16-bit wide flash device.
>> + */
>> +
>> +#define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
>> +#define BASE_ADDR (0x1ULL - MP_FLASH_SIZE_MAX)
>> +
>> +#define FLASH_WIDTH 2
>> +#define CFI_ADDR (FLASH_WIDTH * 0x55)
>> +#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
>> +#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
>> +
>> +#define CFI_CMD 0x98
>> +#define UNLOCK0_CMD 0xAA
>> +#define UNLOCK1_CMD 0x55
>> +#define AUTOSELECT_CMD 0x90
>> +#define RESET_CMD 0xF0
>> +#define PROGRAM_CMD 0xA0
>> +#define SECTOR_ERASE_CMD 0x30
>> +#define CHIP_ERASE_CMD 0x10
>> +#define UNLOCK_BYPASS_CMD 0x20
>> +#define UNLOCK_BYPASS_RESET_CMD 0x00
>> +
>> +static char image_path[] = "/tmp/qtest.XX";
>> +
>> +static inline void flash_write(uint64_t byte_addr, uint16_t data)
>> +{
>> +qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
>> +}
>> +
>> +static inline uint16_t flash_read(uint64_t byte_addr)
>> +{
>> +return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
>> +}
>> +
>> +static void unlock(void)
>> +{
>> +flash_write(UNLOCK0_ADDR, UNLOCK0_CMD);
>> +flash_write(UNLOCK1_ADDR, UNLOCK1_CMD);
>> +}
>> +
>> +static void reset(void)
>> +{
>> +flash_write(0, RESET_CMD);
>> +}
>> +
>> +static void sector_erase(uint64_t byte_addr)
>> +{
>> +unlock();
>> +flash_write(UNLOCK0_ADDR, 0x80);
>> +unlock();
>> +flash_write(byte_addr, SECTOR_ERASE_CMD);
>> +}
>> +
>> +static void wait_for_completion(uint64_t byte_addr)
>> +{
>> +/* If DQ6 is toggling, step the clock and ensure the toggle stops. */
>> +if ((flash_read(byte_addr) & 0x40) ^ (flash_read(byte_addr) & 0x40)) {
>> +/* Wait for erase or program to finish. */
>> +clock_step_next();
>> +/* Ensure that DQ6 has stopped toggling. */
>> +g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
>> +}
>> +}
>> +
>> +static void bypass_program(uint64_t byte_addr, uint16_t data)
>> +{
>> +flash_write(UNLOCK0_ADDR, PROGRAM_CMD);
>> +flash_write(byte_addr, data);
>> +/*
>> + * Data isn't valid until DQ6 stops toggling. We don't model this as
>> + * writes are immediate, but if this changes in the future, we can wait
>> + * until the program is complete.
>> + */
>> +wait_for_completion(byte_addr);
>> +}
>> +
>> +static void program(uint64_t byte_addr, uint16_t data)
>> +{
>> +unlock();
>> +bypass_program(byte_addr, data);
>> +}
>>

Re: [Qemu-devel] [PATCH 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality

2019-04-18 Thread Stephen Checkoway




On Apr 9, 2019, at 12:15, Philippe Mathieu-Daudé  wrote:

> Since you did changes in the CFI table, I think we should add a tests
> verifying the table is correctly generated for all you FlashConfig entries.

In v3 of the patch, I added tests for the CFI table values
- device length
- sector length and number of sectors
- erase suspend supported

-- 
Stephen Checkoway

[Qemu-devel] [PATCH v3] hw/char/escc: Lower irq when transmit buffer is filled

2019-04-19 Thread Stephen Checkoway

The SCC/ESCC will briefly stop asserting an interrupt when the
transmit FIFO is filled.

This code doesn't model the transmit FIFO/shift register so the
pending transmit interrupt is never deasserted which means that an
edge-triggered interrupt controller will never see the low-to-high
transition it needs to raise another interrupt. The practical
consequence of this is that guest firmware with an interrupt service
routine for the ESCC that does not send all of the data it has
immediately will stop sending data if the following sequence of
events occurs:
1. Disable processor interrupts
2. Write a character to the ESCC
3. Add additional characters to a buffer which is drained by the ISR
4. Enable processor interrupts

In this case, the first character will be sent, the interrupt will
fire and the ISR will output the second character. Since the pending
transmit interrupt remains asserted, no additional interrupts will
ever fire.

This behavior was triggered by firmware for an embedded system with a
Z85C30 which necessitated this patch.

This patch fixes that situation by explicitly lowering the IRQ when a
character is written to the buffer and no other interrupts are currently
pending.

Signed-off-by: Stephen Checkoway 
Reviewed-by: Philippe Mathieu-Daudé 
---

I added a sentence about the Z85C30 necessitating this to the commit message.

 hw/char/escc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 628f5f81f7..c5b05a63f1 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -509,6 +509,13 @@ static void escc_mem_write(void *opaque, hwaddr addr,
 break;
 case SERIAL_DATA:
 trace_escc_mem_writeb_data(CHN_C(s), val);
+/*
+ * Lower the irq when data is written to the Tx buffer and no other
+ * interrupts are currently pending. The irq will be raised again once
+ * the Tx buffer becomes empty below.
+ */
+s->txint = 0;
+escc_update_irq(s);
 s->tx = val;
 if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
 if (qemu_chr_fe_backend_connected(&s->chr)) {
-- 
2.20.1 (Apple Git-117)

Re: [Qemu-devel] [PATCH v2] hw/char/escc: Lower irq when transmit buffer is filled

2019-04-19 Thread Stephen Checkoway




> On Apr 18, 2019, at 08:13, Philippe Mathieu-Daudé  wrote:
> 
> On 4/17/19 2:50 AM, Stephen Checkoway wrote:
>> The SCC/ESCC will briefly stop asserting an interrupt when the
>> transmit FIFO is filled.
>> 
>> This code doesn't model the transmit FIFO/shift register so the
>> pending transmit interrupt is never deasserted which means that an
>> edge-triggered interrupt controller will never see the low-to-high
>> transition it needs to raise another interrupt. The practical
>> consequence of this is that guest firmware with an interrupt service
>> routine for the ESCC that does not send all of the data it has
>> immediately will stop sending data if the following sequence of
>> events occurs:
>> 1. Disable processor interrupts
>> 2. Write a character to the ESCC
>> 3. Add additional characters to a buffer which is drained by the ISR
>> 4. Enable processor interrupts
>> 
>> In this case, the first character will be sent, the interrupt will
>> fire and the ISR will output the second character. Since the pending
>> transmit interrupt remains asserted, no additional interrupts will
>> ever fire.
> 
> You might want to add a line expliciting the chipset model which forced
> you to do that patch (Z85C30).

Done.

Thanks for looking at this.

-- 
Stephen Checkoway

[Qemu-devel] [PATCH v4 00/10] block/pflash_cfi02: Implement missing AMD pflash functionality

The goal of this patch series implement the following AMD command-set parallel
flash functionality:
- flash interleaving;
- nonuniform sector sizes;
- erase suspend/resume commands; and
- multi-sector erase.

During refactoring and implementation, I discovered several bugs that are
fixed here as well:
- flash commands use only 11-bits of the address in most cases, but the
  current code uses all of them [1];
- entering CFI mode from autoselect mode and then exiting CFI mode should
  return the chip to autoselect mode, but the current code returns to read
  array mode; and
- reset command should be ignored during sector/chip erase, but the current
  code performs the reset.

The first patch in the series adds a test for the existing behavior. Tests for
additional behavior/bug fixes are added in the relevant patch.

1. I found firmware in the wild that relies on the 11-bit address behavior,
   probably due to a bug in the firmware itself.

Changes from v1:
- Fix missing spaces around *, -, and ?;
- Fix missing Signed-off-by line on patch 7; and
- Replace use of errc with g_printerr and exit.

Changes from v2:
- Remove global_qtest from tests; and
- Test the CFI table changes.

Changes from v3:
- Replace err.h/err() with glib functions; and
- Reformat qtest_initf lines.

Stephen Checkoway (10):
  block/pflash_cfi02: Add test for supported commands
  block/pflash_cfi02: Refactor, NFC intended
  block/pflash_cfi02: Fix command address comparison
  block/pflash_cfi02: Implement intereleaved flash devices
  block/pflash_cfi02: Implement nonuniform sector sizes
  block/pflash_cfi02: Fix CFI in autoselect mode
  block/pflash_cfi02: Fix reset command not ignored during erase
  block/pflash_cfi02: Implement multi-sector erase
  block/pflash_cfi02: Implement erase suspend/resume
  block/pflash_cfi02: Use the chip erase time specified in the CFI table

 hw/block/pflash_cfi02.c   | 843 +++---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 812 
 3 files changed, 1420 insertions(+), 237 deletions(-)
 create mode 100644 tests/pflash-cfi02-test.c

-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v4 01/10] block/pflash_cfi02: Add test for supported commands

Test the AMD command set for parallel flash chips. This test uses an
ARM musicpal board with a pflash drive to test the following list of
currently-supported commands.
- Autoselect
- CFI
- Sector erase
- Chip erase
- Program
- Unlock bypass
- Reset

Signed-off-by: Stephen Checkoway 
---
 tests/Makefile.include|   2 +
 tests/pflash-cfi02-test.c | 225 ++
 2 files changed, 227 insertions(+)
 create mode 100644 tests/pflash-cfi02-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 36fc73fef5..dbdb2c0082 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -263,6 +263,7 @@ check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/hexloader-test$(EXESUF)
+check-qtest-arm-$(CONFIG_PFLASH_CFI02) += tests/pflash-cfi02-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
@@ -773,6 +774,7 @@ tests/device-introspect-test$(EXESUF): 
tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/hexloader-test$(EXESUF): tests/hexloader-test.o
+tests/pflash-cfi02$(EXESUF): tests/pflash-cfi02-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
 tests/rtas-test$(EXESUF): tests/rtas-test.o $(libqos-spapr-obj-y)
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
new file mode 100644
index 00..40af1bb523
--- /dev/null
+++ b/tests/pflash-cfi02-test.c
@@ -0,0 +1,225 @@
+/*
+ * QTest testcase for parallel flash with AMD command set
+ *
+ * Copyright (c) 2019 Stephen Checkoway
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+/*
+ * To test the pflash_cfi02 device, we run QEMU with the musicpal machine with
+ * a pflash drive. This enables us to test some flash configurations, but not
+ * all. In particular, we're limited to a 16-bit wide flash device.
+ */
+
+#define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
+#define BASE_ADDR (0x1ULL - MP_FLASH_SIZE_MAX)
+
+#define FLASH_WIDTH 2
+#define CFI_ADDR (FLASH_WIDTH * 0x55)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+
+#define CFI_CMD 0x98
+#define UNLOCK0_CMD 0xAA
+#define UNLOCK1_CMD 0x55
+#define AUTOSELECT_CMD 0x90
+#define RESET_CMD 0xF0
+#define PROGRAM_CMD 0xA0
+#define SECTOR_ERASE_CMD 0x30
+#define CHIP_ERASE_CMD 0x10
+#define UNLOCK_BYPASS_CMD 0x20
+#define UNLOCK_BYPASS_RESET_CMD 0x00
+
+static char image_path[] = "/tmp/qtest.XX";
+
+static inline void flash_write(uint64_t byte_addr, uint16_t data)
+{
+qtest_writew(global_qtest, BASE_ADDR + byte_addr, data);
+}
+
+static inline uint16_t flash_read(uint64_t byte_addr)
+{
+return qtest_readw(global_qtest, BASE_ADDR + byte_addr);
+}
+
+static void unlock(void)
+{
+flash_write(UNLOCK0_ADDR, UNLOCK0_CMD);
+flash_write(UNLOCK1_ADDR, UNLOCK1_CMD);
+}
+
+static void reset(void)
+{
+flash_write(0, RESET_CMD);
+}
+
+static void sector_erase(uint64_t byte_addr)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(byte_addr, SECTOR_ERASE_CMD);
+}
+
+static void wait_for_completion(uint64_t byte_addr)
+{
+/* If DQ6 is toggling, step the clock and ensure the toggle stops. */
+if ((flash_read(byte_addr) & 0x40) ^ (flash_read(byte_addr) & 0x40)) {
+/* Wait for erase or program to finish. */
+clock_step_next();
+/* Ensure that DQ6 has stopped toggling. */
+g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
+}
+}
+
+static void bypass_program(uint64_t byte_addr, uint16_t data)
+{
+flash_write(UNLOCK0_ADDR, PROGRAM_CMD);
+flash_write(byte_addr, data);
+/*
+ * Data isn't valid until DQ6 stops toggling. We don't model this as
+ * writes are immediate, but if this changes in the future, we can wait
+ * until the program is complete.
+ */
+wait_for_completion(byte_addr);
+}
+
+static void program(uint64_t byte_addr, uint16_t data)
+{
+unlock();
+bypass_program(byte_addr, data);
+}
+
+static void chip_erase(void)
+{
+unlock();
+flash_write(UNLOCK0_ADDR, 0x80);
+unlock();
+flash_write(UNLOCK0_ADDR, SECTOR_ERASE_CMD);
+}
+
+static void test_flash(void)
+{
+global_qtest = qtest_initf("-M musicpal,accel=qtest "
+   "-drive 
if=pflash,file=%s,format=raw,copy-on-read",
+   image_path);
+/* Check the IDs. */
+unlock();
+flash_write(UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0

[Qemu-devel] [PATCH v4 07/10] block/pflash_cfi02: Fix reset command not ignored during erase

When the flash device is performing a chip erase, all commands are
ignored. When it is performing a sector erase, only the erase suspend
command is valid, which is currently not supported.

In particular, the reset command should not cause the device to reset to
read array mode while programming is on going.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index be10036886..cb1160eb35 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -325,7 +325,8 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pfl->bank_width * 2, value);
 }
 
-if (cmd == 0xF0) {
+/* Reset does nothing during chip erase and sector erase. */
+if (cmd == 0xF0 && pfl->cmd != 0x10 && pfl->cmd != 0x30) {
 if (pfl->wcycle == WCYCLE_AUTOSELECT_CFI) {
 /* Return to autoselect mode. */
 pfl->wcycle = 3;
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v4 03/10] block/pflash_cfi02: Fix command address comparison

Most AMD commands only examine 11 bits of the address. This masks the
addresses used in the comparison to 11 bits. The exceptions are word or
sector addresses which use offset directly rather than the shifted
offset, boff.

Signed-off-by: Stephen Checkoway 
Acked-by: Thomas Huth 
---
 hw/block/pflash_cfi02.c   |  8 +++-
 tests/pflash-cfi02-test.c | 12 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4b7af71806..e4bff0c8f8 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -296,11 +296,13 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 
 DPRINTF("%s: offset " TARGET_FMT_plx " %08" PRIx64 " %d\n", __func__,
 offset, value, width);
-boff = offset & (pfl->sector_len - 1);
+boff = offset;
 if (pfl->width == 2)
 boff = boff >> 1;
 else if (pfl->width == 4)
 boff = boff >> 2;
+/* Only the least-significant 11 bits are used in most cases. */
+boff &= 0x7FF;
 switch (pfl->wcycle) {
 case 0:
 /* Set the device in I/O access mode if required */
@@ -519,6 +521,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error 
**errp)
 return;
 }
 
+/* Only 11 bits are used in the comparison. */
+pfl->unlock_addr0 &= 0x7FF;
+pfl->unlock_addr1 &= 0x7FF;
+
 chip_len = pfl->sector_len * pfl->nb_blocs;
 
 memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index 40af1bb523..ea5f8b2648 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -21,8 +21,8 @@
 
 #define FLASH_WIDTH 2
 #define CFI_ADDR (FLASH_WIDTH * 0x55)
-#define UNLOCK0_ADDR (FLASH_WIDTH * 0x)
-#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AAA)
+#define UNLOCK0_ADDR (FLASH_WIDTH * 0x555)
+#define UNLOCK1_ADDR (FLASH_WIDTH * 0x2AA)
 
 #define CFI_CMD 0x98
 #define UNLOCK0_CMD 0xAA
@@ -190,6 +190,14 @@ static void test_flash(void)
 g_assert_cmpint(flash_read(6), ==, 0xCDEF);
 g_assert_cmpint(flash_read(8), ==, 0x);
 
+/* Test ignored high order bits of address. */
+flash_write(FLASH_WIDTH * 0x, UNLOCK0_CMD);
+flash_write(FLASH_WIDTH * 0x2AAA, UNLOCK1_CMD);
+flash_write(FLASH_WIDTH * 0x, AUTOSELECT_CMD);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x), ==, 0x00BF);
+g_assert_cmpint(flash_read(FLASH_WIDTH * 0x0001), ==, 0x236D);
+reset();
+
 qtest_quit(global_qtest);
 }
 
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v4 02/10] block/pflash_cfi02: Refactor, NFC intended

Simplify and refactor for upcoming commits. In particular, pull out all
of the code to modify the status into simple helper functions. Status
handling becomes more complex once multiple chips are interleaved to
produce a single device.

No change in functionality is intended with this commit.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 221 +---
 1 file changed, 95 insertions(+), 126 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index f2c6201f81..4b7af71806 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -46,18 +46,19 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 
-//#define PFLASH_DEBUG
-#ifdef PFLASH_DEBUG
+#define PFLASH_DEBUG false
 #define DPRINTF(fmt, ...)  \
 do {   \
-fprintf(stderr, "PFLASH: " fmt , ## __VA_ARGS__);   \
+if (PFLASH_DEBUG) {\
+fprintf(stderr, "PFLASH: " fmt, ## __VA_ARGS__);   \
+}  \
 } while (0)
-#else
-#define DPRINTF(fmt, ...) do { } while (0)
-#endif
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/* Special write cycle for CFI queries. */
+#define WCYCLE_CFI 7
+
 struct PFlashCFI02 {
 /*< private >*/
 SysBusDevice parent_obj;
@@ -97,6 +98,31 @@ struct PFlashCFI02 {
 void *storage;
 };
 
+/*
+ * Toggle status bit DQ7.
+ */
+static inline void toggle_dq7(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x80;
+}
+
+/*
+ * Set status bit DQ7 to bit 7 of value.
+ */
+static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+{
+pfl->status &= 0x7F;
+pfl->status |= value & 0x80;
+}
+
+/*
+ * Toggle status bit DQ6.
+ */
+static inline void toggle_dq6(PFlashCFI02 *pfl)
+{
+pfl->status ^= 0x40;
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -126,7 +152,7 @@ static void pflash_timer (void *opaque)
 
 trace_pflash_timer_expired(pfl->cmd);
 /* Reset flash */
-pfl->status ^= 0x80;
+toggle_dq7(pfl);
 if (pfl->bypass) {
 pfl->wcycle = 2;
 } else {
@@ -136,12 +162,34 @@ static void pflash_timer (void *opaque)
 pfl->cmd = 0;
 }
 
-static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr offset,
-int width, int be)
+/*
+ * Read data from flash.
+ */
+static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr offset,
+ unsigned int width)
 {
+uint8_t *p = (uint8_t *)pfl->storage + offset;
+uint64_t ret = pfl->be ? ldn_be_p(p, width) : ldn_le_p(p, width);
+/* XXX: Need a trace_pflash_data_read(offset, ret, width) */
+switch (width) {
+case 1:
+trace_pflash_data_read8(offset, ret);
+break;
+case 2:
+trace_pflash_data_read16(offset, ret);
+break;
+case 4:
+trace_pflash_data_read32(offset, ret);
+break;
+}
+return ret;
+}
+
+static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
+{
+PFlashCFI02 *pfl = opaque;
 hwaddr boff;
-uint32_t ret;
-uint8_t *p;
+uint64_t ret;
 
 ret = -1;
 trace_pflash_read(offset, pfl->cmd, width, pfl->wcycle);
@@ -166,39 +214,8 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x80:
 /* We accept reads during second unlock sequence... */
 case 0x00:
-flash_read:
 /* Flash area read */
-p = pfl->storage;
-switch (width) {
-case 1:
-ret = p[offset];
-trace_pflash_data_read8(offset, ret);
-break;
-case 2:
-if (be) {
-ret = p[offset] << 8;
-ret |= p[offset + 1];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-}
-trace_pflash_data_read16(offset, ret);
-break;
-case 4:
-if (be) {
-ret = p[offset] << 24;
-ret |= p[offset + 1] << 16;
-ret |= p[offset + 2] << 8;
-ret |= p[offset + 3];
-} else {
-ret = p[offset];
-ret |= p[offset + 1] << 8;
-ret |= p[offset + 2] << 16;
-ret |= p[offset + 3] << 24;
-}
-trace_pflash_data_read32(offset, ret);
-break;
-}
+ret = pflash_data_read(pfl, offset, width);
 break;
 case 0x90:
 /* flash ID read */
@@ -213,23 +230,23 @@ static uint32_t pflash_read(PFlashCFI02 *pfl, hwaddr 
offset,
 case 0x0E:
 case 0x0F:
 ret = boff & 0x01 ? pfl->ident3 : pfl->ident2;
-if (ret == (uint8_t)-1) {
-goto flash_read;
+

[Qemu-devel] [PATCH v4 06/10] block/pflash_cfi02: Fix CFI in autoselect mode

After a flash device enters CFI mode from autoselect mode, the reset
command returns the device to autoselect mode. An additional reset
command is necessary to return to read array mode.

Signed-off-by: Stephen Checkoway 
Acked-by: Thomas Huth 
---
 hw/block/pflash_cfi02.c   | 21 +
 tests/pflash-cfi02-test.c | 39 +++
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index c4efbe8cdf..be10036886 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -61,8 +61,9 @@ do {   \
  */
 #define PFLASH_MAX_ERASE_REGIONS 4
 
-/* Special write cycle for CFI queries. */
+/* Special write cycles for CFI queries. */
 #define WCYCLE_CFI 7
+#define WCYCLE_AUTOSELECT_CFI 8
 
 struct PFlashCFI02 {
 /*< private >*/
@@ -325,6 +326,12 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 
 if (cmd == 0xF0) {
+if (pfl->wcycle == WCYCLE_AUTOSELECT_CFI) {
+/* Return to autoselect mode. */
+pfl->wcycle = 3;
+pfl->cmd = 0x90;
+return;
+}
 goto reset_flash;
 }
 }
@@ -350,7 +357,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* We're in read mode */
 check_unlock0:
 if (masked_addr == 0x55 && cmd == 0x98) {
-enter_CFI_mode:
 /* Enter CFI query mode */
 pfl->wcycle = WCYCLE_CFI;
 pfl->cmd = 0x98;
@@ -427,9 +433,15 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 /* Unlock bypass reset */
 goto reset_flash;
 }
-/* We can enter CFI query mode from autoselect mode */
+/*
+ * We can enter CFI query mode from autoselect mode, but we must
+ * return to autoselect mode after a reset.
+ */
 if (masked_addr == 0x55 && cmd == 0x98) {
-goto enter_CFI_mode;
+/* Enter autoselect CFI query mode */
+pfl->wcycle = WCYCLE_AUTOSELECT_CFI;
+pfl->cmd = 0x98;
+return;
 }
 /* No break here */
 default:
@@ -510,6 +522,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 }
 break;
 case WCYCLE_CFI: /* Special value for CFI queries */
+case WCYCLE_AUTOSELECT_CFI:
 DPRINTF("%s: invalid write in CFI query mode\n", __func__);
 goto reset_flash;
 default:
diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
index 703f084c5d..c2798bbb36 100644
--- a/tests/pflash-cfi02-test.c
+++ b/tests/pflash-cfi02-test.c
@@ -477,6 +477,42 @@ static void test_geometry(const void *opaque)
 qtest_quit(qtest);
 }
 
+/*
+ * Test that
+ * 1. enter autoselect mode;
+ * 2. enter CFI mode; and then
+ * 3. exit CFI mode
+ * leaves the flash device in autoselect mode.
+ */
+static void test_cfi_in_autoselect(const void *opaque)
+{
+const FlashConfig *config = opaque;
+QTestState *qtest;
+qtest = qtest_initf("-M musicpal,accel=qtest"
+" -drive if=pflash,file=%s,format=raw,copy-on-read",
+image_path);
+FlashConfig explicit_config = expand_config_defaults(config);
+explicit_config.qtest = qtest;
+const FlashConfig *c = &explicit_config;
+
+/* 1. Enter autoselect. */
+unlock(c);
+flash_cmd(c, UNLOCK0_ADDR, AUTOSELECT_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+/* 2. Enter CFI. */
+flash_cmd(c, CFI_ADDR, CFI_CMD);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x10)), ==, replicate(c, 'Q'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x11)), ==, replicate(c, 'R'));
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0x12)), ==, replicate(c, 'Y'));
+
+/* 3. Exit CFI. */
+reset(c);
+g_assert_cmpint(flash_query(c, FLASH_ADDR(0)), ==, replicate(c, 0xBF));
+
+qtest_quit(qtest);
+}
+
 static void cleanup(void *opaque)
 {
 unlink(image_path);
@@ -604,6 +640,9 @@ int main(int argc, char **argv)
 qtest_add_data_func(path, config, test_geometry);
 g_free(path);
 }
+
+qtest_add_data_func("pflash-cfi02/cfi-in-autoselect", &configuration[0],
+test_cfi_in_autoselect);
 int result = g_test_run();
 cleanup(NULL);
 return result;
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v4 05/10] block/pflash_cfi02: Implement nonuniform sector sizes

Some flash chips support sectors of different sizes. For example, the
AMD AM29LV160DT has 31 64 kB sectors, one 32 kB sector, two 8 kB
sectors, and a 16 kB sector, in that order. The AM29LV160DB has those in
the reverse order.

The `num-blocks` and `sector-length` properties work exactly as they did
before: a flash device with uniform sector lengths. To get non-uniform
sector lengths for up to four regions, the following properties may be
set
- region 0. `num-blocks0` and `sector-length0`;
- region 1. `num-blocks1` and `sector-length1`;
- region 2. `num-blocks2` and `sector-length2`; and
- region 3. `num-blocks3` and `sector-length3`.

If the uniform and nonuniform properties are set, then both must specify
a flash device with the same total size. It would be better to disallow
both being set, or make `num-blocks0` and `sector-length0` alias
`num-blocks` and `sector-length`, but that would make testing currently
impossible.

Signed-off-by: Stephen Checkoway 
Acked-by: Thomas Huth 
---
 hw/block/pflash_cfi02.c   | 177 +---
 tests/pflash-cfi02-test.c | 185 --
 2 files changed, 265 insertions(+), 97 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 101628b4ec..c4efbe8cdf 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
  * It does not implement multiple sectors erase
@@ -55,6 +54,13 @@ do {   \
 
 #define PFLASH_LAZY_ROMD_THRESHOLD 42
 
+/*
+ * The size of the cfi_table indirectly depends on this and the start of the
+ * PRI table directly depends on it. 4 is the maximum size (and also what
+ * seems common) without changing the PRT table address.
+ */
+#define PFLASH_MAX_ERASE_REGIONS 4
+
 /* Special write cycle for CFI queries. */
 #define WCYCLE_CFI 7
 
@@ -64,8 +70,10 @@ struct PFlashCFI02 {
 /*< public >*/
 
 BlockBackend *blk;
-uint32_t sector_len;
-uint32_t nb_blocs;
+uint32_t uniform_nb_blocs;
+uint32_t uniform_sector_len;
+uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
+uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
 uint64_t total_len;
 uint64_t interleave_multiplier;
 uint8_t mappings;
@@ -86,7 +94,7 @@ struct PFlashCFI02 {
 uint16_t ident3;
 uint16_t unlock_addr0;
 uint16_t unlock_addr1;
-uint8_t cfi_table[0x52];
+uint8_t cfi_table[0x4D];
 QEMUTimer timer;
 /* The device replicates the flash memory across its memory space.  Emulate
  * that by having a container (.mem) filled with an array of aliases
@@ -189,6 +197,25 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 return ret;
 }
 
+/*
+ * offset should be a byte offset of the QEMU device and _not_ a device
+ * offset.
+ */
+static uint32_t pflash_sector_len(PFlashCFI02 *pfl, hwaddr offset)
+{
+assert(offset < pfl->total_len);
+int nb_regions = pfl->cfi_table[0x2C];
+hwaddr addr = 0;
+for (int i = 0; i < nb_regions; ++i) {
+uint64_t region_size = (uint64_t)pfl->nb_blocs[i] * pfl->sector_len[i];
+if (addr <= offset && offset < addr + region_size) {
+return pfl->sector_len[i];
+}
+addr += region_size;
+}
+abort();
+}
+
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
@@ -285,6 +312,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
+uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -446,12 +474,14 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 case 0x30:
 /* Sector erase */
 p = pfl->storage;
-offset &= ~(pfl->sector_len - 1);
-DPRINTF("%s: start sector erase at " TARGET_FMT_plx "\n", __func__,
-offset);
+sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
 if (!pfl->ro) {
-memset(p + offset, 0xFF, pfl->sector_len);
-pflash_update(pfl, offset, pfl->sector_len);
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
 }
 set_dq7(pfl, 0x00);
 /* Let's wai

[Qemu-devel] [PATCH v4 04/10] block/pflash_cfi02: Implement intereleaved flash devices

It's common for multiple narrow flash chips to be hooked up in parallel
to support wider buses. For example, four 8-bit wide flash chips (x8)
may be combined in parallel to produce a 32-bit wide device. Similarly,
two 16-bit wide chips (x16) may be combined.

This commit introduces `device-width` and `max-device-width` properties,
similar to pflash_cfi01, with the following meanings:
- `width`: The width of the logical, qemu device (same as before);
- `device-width`: The width of an individual flash chip, defaulting to
  `width`; and
- `max-device-width`: The maximum width of an individual flash chip,
  defaulting to `device-width`.

Nothing needs to change to support reading such interleaved devices but
commands (e.g., erase and programming) must be sent to all devices at
the same time or else the various chips will be in different states.

For example, a 4-byte wide logical device can be composed of four x8/x16
devices in x8 mode. That is, each device supports both x8 or x16 and
they're being used in the byte, rather than word, mode. This
configuration would have `width=4`, `device-width=1`, and
`max-device-width=2`.

In addition to commands being sent to all devices, guest firmware
expects the status and CFI queries to be replicated for each device.
(The one exception to the response replication is that each device gets
to report its own status bit DQ7 while programming because its value
depends on the value being programmed which will usually differ for each
device.)

Testing is limited to 16-bit wide devices due to the current inability
to override the properties set by `pflash_cfi02_register`, but multiple
configurations are tested.

Stop using global_qtest. Instead, package the qtest variable inside the
FlashConfig structure.

Signed-off-by: Stephen Checkoway 
Acked-by: Thomas Huth 
---
 hw/block/pflash_cfi02.c   | 270 +++--
 tests/pflash-cfi02-test.c | 476 ++
 2 files changed, 576 insertions(+), 170 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index e4bff0c8f8..101628b4ec 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -28,7 +28,6 @@
  * - unlock bypass command
  * - CFI queries
  *
- * It does not support flash interleaving.
  * It does not implement boot blocs with reduced size
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
@@ -67,15 +66,19 @@ struct PFlashCFI02 {
 BlockBackend *blk;
 uint32_t sector_len;
 uint32_t nb_blocs;
-uint32_t chip_len;
+uint64_t total_len;
+uint64_t interleave_multiplier;
 uint8_t mappings;
-uint8_t width;
+uint8_t bank_width; /* Width of the QEMU device in bytes. */
+uint8_t device_width; /* Width of individual pflash chip. */
+uint8_t max_device_width; /* Maximum width of individual pflash chip. */
 uint8_t be;
+int device_shift; /* Amount to shift an offset to get a device address. */
 int wcycle; /* if 0, the flash is read normally */
 int bypass;
 int ro;
 uint8_t cmd;
-uint8_t status;
+uint64_t status;
 /* FIXME: implement array device properties */
 uint16_t ident0;
 uint16_t ident1;
@@ -103,16 +106,17 @@ struct PFlashCFI02 {
  */
 static inline void toggle_dq7(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x80;
+pfl->status ^= pfl->interleave_multiplier * 0x80;
 }
 
 /*
  * Set status bit DQ7 to bit 7 of value.
  */
-static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
+static inline void set_dq7(PFlashCFI02 *pfl, uint64_t value)
 {
-pfl->status &= 0x7F;
-pfl->status |= value & 0x80;
+uint64_t mask = pfl->interleave_multiplier * 0x80;
+pfl->status &= ~mask;
+pfl->status |= value & mask;
 }
 
 /*
@@ -120,7 +124,7 @@ static inline void set_dq7(PFlashCFI02 *pfl, uint8_t value)
  */
 static inline void toggle_dq6(PFlashCFI02 *pfl)
 {
-pfl->status ^= 0x40;
+pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
 /*
@@ -188,7 +192,6 @@ static uint64_t pflash_data_read(PFlashCFI02 *pfl, hwaddr 
offset,
 static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
-hwaddr boff;
 uint64_t ret;
 
 ret = -1;
@@ -198,12 +201,10 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, 
unsigned int width)
 ++pfl->read_counter > PFLASH_LAZY_ROMD_THRESHOLD) {
 pflash_register_memory(pfl, 1);
 }
-offset &= pfl->chip_len - 1;
-boff = offset & 0xFF;
-if (pfl->width == 2)
-boff = boff >> 1;
-else if (pfl->width == 4)
-boff = boff >> 2;
+/* Mask by the total length of the chip to account for alias mappings. */
+offset &= pfl->total_len - 1;
+hwaddr device_addr = offset >> pfl->device_shift;
+
 switch (pfl->cmd) {
 default

[Qemu-devel] [PATCH v4 08/10] block/pflash_cfi02: Implement multi-sector erase

After two unlock cycles and a sector erase command, the AMD flash chips
start a 50 us erase time out. Any additional sector erase commands add a
sector to be erased and restart the 50 us timeout. During the timeout,
status bit DQ3 is cleared. After the time out, DQ3 is asserted during
erasure.

Signed-off-by: Stephen Checkoway 
Acked-by: Thomas Huth 
---
 hw/block/pflash_cfi02.c   | 94 +++
 tests/pflash-cfi02-test.c | 59 ++--
 2 files changed, 131 insertions(+), 22 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index cb1160eb35..21ceb0823b 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -30,7 +30,6 @@
  *
  * It does not implement software data protection as found in many real chips
  * It does not implement erase suspend/resume commands
- * It does not implement multiple sectors erase
  */
 
 #include "qemu/osdep.h"
@@ -106,6 +105,7 @@ struct PFlashCFI02 {
 MemoryRegion orig_mem;
 int rom_mode;
 int read_counter; /* used for lazy switch-back to rom mode */
+int sectors_to_erase;
 char *name;
 void *storage;
 };
@@ -136,6 +136,22 @@ static inline void toggle_dq6(PFlashCFI02 *pfl)
 pfl->status ^= pfl->interleave_multiplier * 0x40;
 }
 
+/*
+ * Turn on DQ3.
+ */
+static inline void assert_dq3(PFlashCFI02 *pfl)
+{
+pfl->status |= pfl->interleave_multiplier * 0x08;
+}
+
+/*
+ * Turn off DQ3.
+ */
+static inline void reset_dq3(PFlashCFI02 *pfl)
+{
+pfl->status &= ~(pfl->interleave_multiplier * 0x08);
+}
+
 /*
  * Set up replicated mappings of the same region.
  */
@@ -159,11 +175,37 @@ static void pflash_register_memory(PFlashCFI02 *pfl, int 
rom_mode)
 pfl->rom_mode = rom_mode;
 }
 
-static void pflash_timer (void *opaque)
+static void pflash_timer(void *opaque)
 {
 PFlashCFI02 *pfl = opaque;
 
 trace_pflash_timer_expired(pfl->cmd);
+if (pfl->cmd == 0x30) {
+/*
+ * Sector erase. If DQ3 is 0 when the timer expires, then the 50
+ * us erase timeout has expired so we need to start the timer for the
+ * sector erase algorithm. Otherwise, the erase completed and we should
+ * go back to read array mode.
+ */
+if ((pfl->status & 0x08) == 0) {
+assert_dq3(pfl);
+/*
+ * CFI address 0x21 is "Typical timeout per individual block erase
+ * 2^N ms"
+ */
+uint64_t timeout = ((1ULL << pfl->cfi_table[0x21]) *
+pfl->sectors_to_erase) * 100;
+timer_mod(&pfl->timer,
+  qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + timeout);
+DPRINTF("%s: erase timeout fired; erasing %d sectors\n",
+__func__, pfl->sectors_to_erase);
+return;
+}
+DPRINTF("%s: sector erase complete\n", __func__);
+pfl->sectors_to_erase = 0;
+reset_dq3(pfl);
+}
+
 /* Reset flash */
 toggle_dq7(pfl);
 if (pfl->bypass) {
@@ -307,13 +349,30 @@ static void pflash_update(PFlashCFI02 *pfl, int offset, 
int size)
 }
 }
 
+static void pflash_sector_erase(PFlashCFI02 *pfl, hwaddr offset)
+{
+uint64_t sector_len = pflash_sector_len(pfl, offset);
+offset &= ~(sector_len - 1);
+DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
+__func__, pfl->bank_width * 2, offset,
+pfl->bank_width * 2, offset + sector_len - 1);
+if (!pfl->ro) {
+uint8_t *p = pfl->storage;
+memset(p + offset, 0xFF, sector_len);
+pflash_update(pfl, offset, sector_len);
+}
+set_dq7(pfl, 0x00);
+++pfl->sectors_to_erase;
+/* Set (or reset) the 50 us timer for additional erase commands.  */
+timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 5);
+}
+
 static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
  unsigned int width)
 {
 PFlashCFI02 *pfl = opaque;
 uint8_t *p;
 uint8_t cmd;
-uint32_t sector_len;
 
 cmd = value;
 if (pfl->cmd != 0xA0) {
@@ -486,20 +545,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 break;
 case 0x30:
 /* Sector erase */
-p = pfl->storage;
-sector_len = pflash_sector_len(pfl, offset);
-offset &= ~(sector_len - 1);
-DPRINTF("%s: start sector erase at %0*" PRIx64 "-%0*" PRIx64 "\n",
-__func__, pfl->bank_width * 2, offset,
-pfl->bank_width * 2, offset + sector_len - 1);
-if (!pfl->ro) {
-memset(p + offset, 0xFF, sector_len);
-pflash_update(pfl,

[Qemu-devel] [PATCH v4 10/10] block/pflash_cfi02: Use the chip erase time specified in the CFI table

When erasing the chip, use the typical time specified in the CFI table
rather than arbitrarily selecting 5 seconds.

Since the currently unconfigurable value set in the table is 12, this
means a chip erase takes 4096 ms so this isn't a big change in behavior.

Signed-off-by: Stephen Checkoway 
---
 hw/block/pflash_cfi02.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index d9087cafff..76c8af4365 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -633,9 +633,9 @@ static void pflash_write(void *opaque, hwaddr offset, 
uint64_t value,
 pflash_update(pfl, 0, pfl->total_len);
 }
 set_dq7(pfl, 0x00);
-/* Let's wait 5 seconds before chip erase is done */
+/* Wait the time specified at CFI address 0x22. */
 timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-  (NANOSECONDS_PER_SECOND * 5));
+  (1ULL << pfl->cfi_table[0x22]) * SCALE_MS);
 break;
 case 0x30:
 /* Sector erase */
-- 
2.20.1 (Apple Git-117)

[Qemu-devel] [PATCH v4 09/10] block/pflash_cfi02: Implement erase suspend/resume