From: Stephen Checkoway <s@pahtak.org>

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 <stephen.checkoway@oberlin.edu>
---
 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 0000000000..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 <err.h>
+#include <unistd.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 (0x100000000ULL-MP_FLASH_SIZE_MAX)
+
+#define FLASH_WIDTH 2
+#define CFI_ADDR (FLASH_WIDTH*0x55)
+#define UNLOCK0_ADDR (FLASH_WIDTH*0x5555)
+#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.XXXXXX";
+
+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*0x0000), ==, 0x00BF);
+    g_assert_cmpint(flash_read(FLASH_WIDTH*0x0001), ==, 0x236D);
+    reset();
+
+    /* Check the erase blocks. */
+    flash_write(CFI_ADDR, CFI_CMD);
+    g_assert_cmpint(flash_read(FLASH_WIDTH*0x10), ==, 'Q');
+    g_assert_cmpint(flash_read(FLASH_WIDTH*0x11), ==, 'R');
+    g_assert_cmpint(flash_read(FLASH_WIDTH*0x12), ==, 'Y');
+    g_assert_cmpint(flash_read(FLASH_WIDTH*0x2C), >=, 1); /* Num erase regions. */
+    uint32_t nb_sectors = flash_read(FLASH_WIDTH*0x2D) +
+                          (flash_read(FLASH_WIDTH*0x2E) << 8) + 1;
+    uint32_t sector_len = (flash_read(FLASH_WIDTH*0x2F) << 8) +
+                          (flash_read(FLASH_WIDTH*0x30) << 16);
+    reset();
+
+    /* Erase and program sector. */
+    for (uint32_t i = 0; i < nb_sectors; ++i) {
+        uint64_t byte_addr = i * sector_len;
+        sector_erase(byte_addr);
+        /* Read toggle. */
+        uint16_t status0 = flash_read(byte_addr);
+        /* DQ7 is 0 during an erase. */
+        g_assert_cmpint(status0 & 0x80, ==, 0);
+        uint16_t status1 = flash_read(byte_addr);
+        /* DQ6 toggles during an erase. */
+        g_assert_cmpint(status0 & 0x40, !=, status1 & 0x40);
+        /* Wait for erase to complete. */
+        clock_step_next();
+        /* Ensure DQ6 has stopped toggling. */
+        g_assert_cmpint(flash_read(byte_addr), ==, flash_read(byte_addr));
+        /* Now the data should be valid. */
+        g_assert_cmpint(flash_read(byte_addr), ==, 0xFFFF);
+
+        /* Program a bit pattern. */
+        program(byte_addr, 0x5555);
+        g_assert_cmpint(flash_read(byte_addr), ==, 0x5555);
+        program(byte_addr, 0xAA55);
+        g_assert_cmpint(flash_read(byte_addr), ==, 0x0055);
+    }
+
+    /* Erase the chip. */
+    chip_erase();
+    /* Read toggle. */
+    uint16_t status0 = flash_read(0);
+    /* DQ7 is 0 during an erase. */
+    g_assert_cmpint(status0 & 0x80, ==, 0);
+    uint16_t status1 = flash_read(0);
+    /* DQ6 toggles during an erase. */
+    g_assert_cmpint(status0 & 0x40, !=, status1 & 0x40);
+    /* Wait for erase to complete. */
+    clock_step_next();
+    /* Ensure DQ6 has stopped toggling. */
+    g_assert_cmpint(flash_read(0), ==, flash_read(0));
+    /* Now the data should be valid. */
+    g_assert_cmpint(flash_read(0), ==, 0xFFFF);
+
+    /* Unlock bypass */
+    unlock();
+    flash_write(UNLOCK0_ADDR, UNLOCK_BYPASS_CMD);
+    bypass_program(0, 0x0123);
+    bypass_program(2, 0x4567);
+    bypass_program(4, 0x89AB);
+    /*
+     * Test that bypass programming, unlike normal programming can use any
+     * address for the PROGRAM_CMD.
+     */
+    flash_write(6, PROGRAM_CMD);
+    flash_write(6, 0xCDEF);
+    wait_for_completion(6);
+    flash_write(0, UNLOCK_BYPASS_RESET_CMD);
+    bypass_program(8, 0x55AA); /* Should fail. */
+    g_assert_cmpint(flash_read(0), ==, 0x0123);
+    g_assert_cmpint(flash_read(2), ==, 0x4567);
+    g_assert_cmpint(flash_read(4), ==, 0x89AB);
+    g_assert_cmpint(flash_read(6), ==, 0xCDEF);
+    g_assert_cmpint(flash_read(8), ==, 0xFFFF);
+
+    qtest_quit(global_qtest);
+}
+
+static void cleanup(void *opaque)
+{
+    unlink(image_path);
+}
+
+int main(int argc, char **argv)
+{
+    int fd = mkstemp(image_path);
+    if (fd == -1) {
+        err(1, "Failed to create temporary file %s", image_path);
+    }
+    if (ftruncate(fd, 8*1024*1024) < 0) {
+        int error_code = errno;
+        close(fd);
+        unlink(image_path);
+        errc(1, error_code, "Failed to trucate file %s to 8 MB", image_path);
+    }
+    close(fd);
+
+    qtest_add_abrt_handler(cleanup, NULL);
+    g_test_init(&argc, &argv, NULL);
+    qtest_add_func("pflash-cfi02", test_flash);
+    int result = g_test_run();
+    cleanup(NULL);
+    return result;
+}
+
+/* vim: set sw=4 sts=4 ts=8 et: */
-- 
2.20.1 (Apple Git-117)

On 08/04/2019 22.55, Stephen Checkoway wrote:
> From: Stephen Checkoway <s@pahtak.org>
> 
> 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 <stephen.checkoway@oberlin.edu>
> ---
>  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 0000000000..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 <err.h>
> +#include <unistd.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 (0x100000000ULL-MP_FLASH_SIZE_MAX)
> +
> +#define FLASH_WIDTH 2
> +#define CFI_ADDR (FLASH_WIDTH*0x55)
> +#define UNLOCK0_ADDR (FLASH_WIDTH*0x5555)
> +#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.XXXXXX";
> +
> +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);
> +}

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.

 Thanks,
  Thomas

On Apr 9, 2019, at 02:13, Thomas Huth <thuth@redhat.com> 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

On 09/04/2019 17.37, Stephen Checkoway wrote:
> 
> 
> On Apr 9, 2019, at 02:13, Thomas Huth <thuth@redhat.com> 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?

Using FlashConfig sounds fine to me. If you need to use global_qtest in
some of the earlier patches, that's ok, too (especially if these go away
again in the later patches ;-)).

 Thomas

Thomas Huth <thuth@redhat.com> writes:

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

Twenty tests still use @global_qtest

Either we're serious about getting rid of @global_qtest.  Then we should
just do it.

Or we're not.  Then we shouldn't ask contributors to do extra work.

On 09/04/2019 09.45, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> 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.
> 
> Twenty tests still use @global_qtest
> 
> Either we're serious about getting rid of @global_qtest.  Then we should
> just do it.

Ha ha, "just do it" ... that's quite a bit of work, actually. It's not
just about grep'ing for global_qtest, you also have to replace all the
writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc.

If you feel like this is a "just do it" quick task, you're welcome to help!

> Or we're not.  Then we shouldn't ask contributors to do extra work.

We are:

 git log --oneline -- tests | grep global_qtest

... it just takes time.

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 09/04/2019 09.45, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> 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.
>> 
>> Twenty tests still use @global_qtest
>> 
>> Either we're serious about getting rid of @global_qtest.  Then we should
>> just do it.
>
> Ha ha, "just do it" ... that's quite a bit of work, actually. It's not
> just about grep'ing for global_qtest, you also have to replace all the
> writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc.

And that's precisely why I'm reluctant to demand this work from
contributors.  Asking nicely is of course fair.

> If you feel like this is a "just do it" quick task, you're welcome to help!
>
>> Or we're not.  Then we shouldn't ask contributors to do extra work.
>
> We are:
>
>  git log --oneline -- tests | grep global_qtest
>
> ... it just takes time.
>
>  Thomas

On 09/04/2019 10.35, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 09/04/2019 09.45, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> 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.
>>>
>>> Twenty tests still use @global_qtest
>>>
>>> Either we're serious about getting rid of @global_qtest.  Then we should
>>> just do it.
>>
>> Ha ha, "just do it" ... that's quite a bit of work, actually. It's not
>> just about grep'ing for global_qtest, you also have to replace all the
>> writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc.
> 
> And that's precisely why I'm reluctant to demand this work from
> contributors.  Asking nicely is of course fair.

That's what I did, didn't I? I said "... so if it is feasible, please
don't use it in new code anymore". I did not say "you must not use this
in new code anymore". So where's your problem here, Markus?

Stephen, for the records, in case my mail was not clear: I'm fine if you
keep the code as it currently is. But I'd be really happy if you could
change it to avoid global_qtest.

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 09/04/2019 10.35, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 09/04/2019 09.45, Markus Armbruster wrote:
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>
>>>>> 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.
>>>>
>>>> Twenty tests still use @global_qtest
>>>>
>>>> Either we're serious about getting rid of @global_qtest.  Then we should
>>>> just do it.
>>>
>>> Ha ha, "just do it" ... that's quite a bit of work, actually. It's not
>>> just about grep'ing for global_qtest, you also have to replace all the
>>> writel(), readl() etc. functions with qtest_writel(), qtest_readl() etc.
>> 
>> And that's precisely why I'm reluctant to demand this work from
>> contributors.  Asking nicely is of course fair.
>
> That's what I did, didn't I? I said "... so if it is feasible, please
> don't use it in new code anymore". I did not say "you must not use this
> in new code anymore". So where's your problem here, Markus?

You did, I don't have a problem, I just wanted to make quite sure your
asking nicely wasn't misunderstood as a polite way to demand.

[...]