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

[Stephen Checkoway]

> On Apr 18, 2019, at 00:47, Thomas Huth <address@hidden> 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 <address@hidden>
>> ---
>> 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 0000000000..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 <err.h>
> 
> We generally don't use err.h in QEMU ... could you please use the
> standard functions from glib instead?

Done.

> +#include <unistd.h>
> 
> 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 (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');
>> +    /* Num erase regions. */
>> +    g_assert_cmpint(flash_read(FLASH_WIDTH * 0x2C), >=, 1);
>> +    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);
> 
> g_printerr() ?
> 
> or maybe simply use a g_assert_cmpint(ret, !=, -1) like we do it in a
> couple of other places already.

I went with g_printerr() + exit(EXIT_FAILURE) similar to what's just below.

>> +    }
>> +    if (ftruncate(fd, 8 * 1024 * 1024) < 0) {
>> +        int error_code = errno;
>> +        close(fd);
>> +        unlink(image_path);
>> +        g_printerr("Failed to truncate file %s to 8 MB: %s\n", image_path,
>> +                   strerror(error_code));
>> +        exit(EXIT_FAILURE);
>> +    }
>> +    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: */
> 
> IMHO the vim line should not be necessary (and we hardly do that in any
> of the other files).

Done.

I also fixed the nit you mentioned in patch 6 as well as an identical 
occurrence earlier.

Thank you for the fast review comments!

I'm new to contributing to QEMU and I have a two procedural questions:

1. When should I send version 4 of this patch series? I don't want to bombard 
everyone with another 11 emails after fixing every review comment, but I also 
don't want to let the patch linger without addressing comments.

2. I'm not familiar with the "Acked-by" convention. I read 
<https://www.kernel.org/doc/html/v4.16/process/submitting-patches.html#when-to-use-acked-by-and-cc>.
 Am I supposed to add those to the commit messages or do those get added by 
whomever actually includes the patch in the main QEMU repo?

-- 
Stephen Checkoway