All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Stephen Checkoway <stephen.checkoway@oberlin.edu>, qemu-devel@nongnu.org
Cc: "Magnus Damm" <magnus.damm@gmail.com>,
	qemu-block@nongnu.org, "Kevin Wolf" <kwolf@redhat.com>,
	"Antony Pavlov" <antonynpavlov@gmail.com>,
	"Max Reitz" <mreitz@redhat.com>,
	qemu-ppc@nongnu.org, "Thomas Huth" <thuth@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	qemu-arm@nongnu.org, "Laurent Vivier" <lvivier@redhat.com>,
	"Jan Kiszka" <jan.kiszka@web.de>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	"Michael Walle" <michael@walle.cc>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"John Snow" <jsnow@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"David Gibson" <david@gibson.dropbear.id.au>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Alistair Francis" <alistair@alistair23.me>,
	"Alex Bennée" <alex.bennee@linaro.org>
Subject: Re: [PATCH v5 18/28] hw/block/pflash_cfi02: Implement nonuniform sector sizes
Date: Fri, 28 Jun 2019 19:28:26 +0200	[thread overview]
Message-ID: <241eaafa-e5fa-1653-e789-2ef9ac5ae592@redhat.com> (raw)
In-Reply-To: <20190627202719.17739-19-philmd@redhat.com>

On 6/27/19 10:27 PM, Philippe Mathieu-Daudé wrote:
> From: Stephen Checkoway <stephen.checkoway@oberlin.edu>
> 
> 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 <stephen.checkoway@oberlin.edu>
> Acked-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20190426162624.55977-6-stephen.checkoway@oberlin.edu>
> [PMD: Rebased, add assert() on pri_offset]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c   | 141 +++++++++++++++++++++++++++-------
>  tests/pflash-cfi02-test.c | 155 ++++++++++++++++++++++++++++----------
>  2 files changed, 231 insertions(+), 65 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 01d9c5d75a..1f096ec185 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -29,7 +29,6 @@
>   * - 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
>   * It does not implement multiple sectors erase
> @@ -57,6 +56,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 cycles for CFI queries. */
>  enum {
>      WCYCLE_CFI              = 7,
> @@ -68,8 +74,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];

I think we can drop the 'uniform' variables, and only use the indexed ones.

>      uint32_t chip_len;
>      uint8_t mappings;
>      uint8_t width;
> @@ -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
> @@ -177,6 +185,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)

Hmm I'd rather have a more generic pflash_sector_index(pfl, offset).

> +{
> +    assert(offset < pfl->chip_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;
> @@ -191,10 +218,11 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
>      }
>      offset &= pfl->chip_len - 1;
>      boff = offset & 0xFF;
> -    if (pfl->width == 2)
> +    if (pfl->width == 2) {
>          boff = boff >> 1;
> -    else if (pfl->width == 4)
> +    } else if (pfl->width == 4) {
>          boff = boff >> 2;
> +    }

Simple style, ok.

>      switch (pfl->cmd) {
>      default:
>          /* This should never happen : reset state & treat it as a read*/
> @@ -273,6 +301,7 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
>      hwaddr boff;
>      uint8_t *p;
>      uint8_t cmd;
> +    uint32_t sector_len;
>  
>      trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
>      cmd = value;
> @@ -282,10 +311,11 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
>      offset &= pfl->chip_len - 1;
>  
>      boff = offset;
> -    if (pfl->width == 2)
> +    if (pfl->width == 2) {
>          boff = boff >> 1;
> -    else if (pfl->width == 4)
> +    } else if (pfl->width == 4) {
>          boff = boff >> 2;
> +    }

Style again.

>      /* Only the least-significant 11 bits are used in most cases. */
>      boff &= 0x7FF;
>      switch (pfl->wcycle) {
> @@ -420,12 +450,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->width * 2, offset,
> +                    pfl->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 done */
> @@ -493,11 +525,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      int ret;
>      Error *local_err = NULL;
>  
> -    if (pfl->sector_len == 0) {
> +    if (pfl->uniform_sector_len == 0 && pfl->sector_len[0] == 0) {
>          error_setg(errp, "attribute \"sector-length\" not specified or zero.");
>          return;
>      }
> -    if (pfl->nb_blocs == 0) {
> +    if (pfl->uniform_nb_blocs == 0 && pfl->nb_blocs[0] == 0) {
>          error_setg(errp, "attribute \"num-blocks\" not specified or zero.");
>          return;
>      }
> @@ -506,7 +538,51 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    pfl->chip_len = pfl->sector_len * pfl->nb_blocs;
> +    int nb_regions;
> +    pfl->chip_len = 0;
> +    for (nb_regions = 0; nb_regions < PFLASH_MAX_ERASE_REGIONS; ++nb_regions) {
> +        if (pfl->nb_blocs[nb_regions] == 0) {
> +            break;
> +        }
> +        uint64_t sector_len_per_device = pfl->sector_len[nb_regions];
> +
> +        /*
> +         * The size of each flash sector must be a power of 2 and it must be
> +         * aligned at the same power of 2.
> +         */
> +        if (sector_len_per_device & 0xff ||
> +            sector_len_per_device >= (1 << 24) ||
> +            !is_power_of_2(sector_len_per_device))
> +        {
> +            error_setg(errp, "unsupported configuration: "
> +                       "sector length[%d] per device = %" PRIx64 ".",
> +                       nb_regions, sector_len_per_device);
> +            return;
> +        }
> +        if (pfl->chip_len & (sector_len_per_device - 1)) {
> +            error_setg(errp, "unsupported configuration: "
> +                       "flash region %d not correctly aligned.",
> +                       nb_regions);
> +            return;
> +        }
> +
> +        pfl->chip_len += (uint64_t)pfl->sector_len[nb_regions] *
> +                          pfl->nb_blocs[nb_regions];
> +    }
> +
> +    uint64_t uniform_len = (uint64_t)pfl->uniform_nb_blocs *
> +                           pfl->uniform_sector_len;
> +    if (nb_regions == 0) {
> +        nb_regions = 1;
> +        pfl->nb_blocs[0] = pfl->uniform_nb_blocs;
> +        pfl->sector_len[0] = pfl->uniform_sector_len;
> +        pfl->chip_len = uniform_len;
> +    } else if (uniform_len != 0 && uniform_len != pfl->chip_len) {
> +        error_setg(errp, "\"num-blocks\"*\"sector-length\" "
> +                   "different from \"num-blocks0\"*\'sector-length0\" + ... + "
> +                   "\"num-blocks3\"*\"sector-length3\"");
> +        return;
> +    }
>  
>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
>                                    &pflash_cfi02_ops, pfl, pfl->name,
> @@ -552,7 +628,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->status = 0;
>  
>      /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
> -    const uint16_t pri_ofs = 0x31;
> +    const uint16_t pri_ofs = 0x40;
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
>      pfl->cfi_table[0x11] = 'R';
> @@ -603,14 +679,17 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      //    pfl->cfi_table[0x2A] = 0x05;
>      pfl->cfi_table[0x2A] = 0x00;
>      pfl->cfi_table[0x2B] = 0x00;
> -    /* Number of erase block regions (uniform) */
> -    pfl->cfi_table[0x2C] = 0x01;
> -    /* Erase block region 1 */
> -    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
> -    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
> -    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
> -    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
> -    assert(0x30 < pri_ofs);
> +    /* Number of erase block regions */
> +    pfl->cfi_table[0x2c] = nb_regions;
> +    /* Erase block regions */
> +    for (int i = 0; i < nb_regions; ++i) {
> +        uint32_t sector_len_per_device = pfl->sector_len[i];
> +        pfl->cfi_table[0x2d + 4 * i] = pfl->nb_blocs[i] - 1;
> +        pfl->cfi_table[0x2e + 4 * i] = (pfl->nb_blocs[i] - 1) >> 8;
> +        pfl->cfi_table[0x2f + 4 * i] = sector_len_per_device >> 8;
> +        pfl->cfi_table[0x30 + 4 * i] = sector_len_per_device >> 16;
> +    }

OK.

> +    assert(0x2c + 4 * nb_regions < pri_ofs);
>  
>      /* Extended */
>      pfl->cfi_table[0x00 + pri_ofs] = 'P';
> @@ -644,8 +723,16 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>  
>  static Property pflash_cfi02_properties[] = {
>      DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
> -    DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
> -    DEFINE_PROP_UINT32("sector-length", PFlashCFI02, sector_len, 0),
> +    DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, uniform_nb_blocs, 0),
> +    DEFINE_PROP_UINT32("sector-length", PFlashCFI02, uniform_sector_len, 0),
> +    DEFINE_PROP_UINT32("num-blocks0", PFlashCFI02, nb_blocs[0], 0),
> +    DEFINE_PROP_UINT32("sector-length0", PFlashCFI02, sector_len[0], 0),
> +    DEFINE_PROP_UINT32("num-blocks1", PFlashCFI02, nb_blocs[1], 0),
> +    DEFINE_PROP_UINT32("sector-length1", PFlashCFI02, sector_len[1], 0),
> +    DEFINE_PROP_UINT32("num-blocks2", PFlashCFI02, nb_blocs[2], 0),
> +    DEFINE_PROP_UINT32("sector-length2", PFlashCFI02, sector_len[2], 0),
> +    DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0),
> +    DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0),
>      DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0),
>      DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0),
>      DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0),
> diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
> index b00f5ca2e7..1659eaebce 100644
> --- a/tests/pflash-cfi02-test.c
> +++ b/tests/pflash-cfi02-test.c
> @@ -17,9 +17,11 @@
>   */
>  
>  #define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
> -#define FLASH_SIZE (8 * 1024 * 1024)
>  #define BASE_ADDR (0x100000000ULL - MP_FLASH_SIZE_MAX)
>  
> +#define UNIFORM_FLASH_SIZE (8 * 1024 * 1024)
> +#define UNIFORM_FLASH_SECTOR_SIZE (64 * 1024)
> +
>  /* Use a newtype to keep flash addresses separate from byte addresses. */
>  typedef struct {
>      uint64_t addr;
> @@ -44,6 +46,10 @@ typedef struct {
>  typedef struct {
>      int bank_width;
>  
> +    /* Nonuniform block size. */
> +    int nb_blocs[4];
> +    int sector_len[4];
> +
>      QTestState *qtest;
>  } FlashConfig;
>  
> @@ -62,6 +68,10 @@ static FlashConfig expand_config_defaults(const FlashConfig *c)
>      if (ret.bank_width == 0) {
>          ret.bank_width = 2;
>      }
> +    if (ret.nb_blocs[0] == 0 && ret.sector_len[0] == 0) {
> +        ret.sector_len[0] = UNIFORM_FLASH_SECTOR_SIZE;
> +        ret.nb_blocs[0] = UNIFORM_FLASH_SIZE / UNIFORM_FLASH_SECTOR_SIZE;

See, here you use index=0 for the uniform config.

> +    }
>  
>      /* XXX: Limitations of test harness. */
>      assert(ret.bank_width == 2);
> @@ -230,13 +240,41 @@ static void chip_erase(const FlashConfig *c)
>      flash_cmd(c, UNLOCK0_ADDR, CHIP_ERASE_CMD);
>  }
>  
> -static void test_flash(const void *opaque)
> +/*
> + * Test flash commands with a variety of device geometry.
> + */
> +static void test_geometry(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);
> +                        " -drive if=pflash,file=%s,format=raw,copy-on-read"
> +                        /* Device geometry properties. */
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks0,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length0,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks1,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length1,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks2,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length2,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks3,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length3,value=%d",
> +                        image_path,
> +                        config->nb_blocs[0],
> +                        config->sector_len[0],
> +                        config->nb_blocs[1],
> +                        config->sector_len[1],
> +                        config->nb_blocs[2],
> +                        config->sector_len[2],
> +                        config->nb_blocs[3],
> +                        config->sector_len[3]);
>      FlashConfig explicit_config = expand_config_defaults(config);
>      explicit_config.qtest = qtest;
>      const FlashConfig *c = &explicit_config;
> @@ -264,39 +302,56 @@ static void test_flash(const void *opaque)
>      g_assert_cmphex(flash_query(c, FLASH_ADDR(0x12)), ==, replicate(c, 'Y'));
>  
>      /* Num erase regions. */
> -    g_assert_cmphex(flash_query_1(c, FLASH_ADDR(0x2C)), >=, 1);
> +    int nb_erase_regions = flash_query_1(c, FLASH_ADDR(0x2C));
> +    g_assert_cmphex(nb_erase_regions, ==,
> +                    !!c->nb_blocs[0] + !!c->nb_blocs[1] + !!c->nb_blocs[2] +
> +                    !!c->nb_blocs[3]);
> +
> +    /* Check device length. */
> +    uint32_t device_len = 1 << flash_query_1(c, FLASH_ADDR(0x27));
> +    g_assert_cmphex(device_len, ==, UNIFORM_FLASH_SIZE);
>  
> -    uint32_t nb_sectors = flash_query_1(c, FLASH_ADDR(0x2D)) +
> -                          (flash_query_1(c, FLASH_ADDR(0x2E)) << 8) + 1;
> -    uint32_t sector_len = (flash_query_1(c, FLASH_ADDR(0x2F)) << 8) +
> -                          (flash_query_1(c, FLASH_ADDR(0x30)) << 16);
>      reset(c);
>  
>      const uint64_t dq7 = replicate(c, 0x80);
>      const uint64_t dq6 = replicate(c, 0x40);
> -    /* Erase and program sector. */
> -    for (uint32_t i = 0; i < nb_sectors; ++i) {
> -        uint64_t byte_addr = i * sector_len;
> -        sector_erase(c, byte_addr);
> -        /* Read toggle. */
> -        uint64_t status0 = flash_read(c, byte_addr);
> -        /* DQ7 is 0 during an erase. */
> -        g_assert_cmphex(status0 & dq7, ==, 0);
> -        uint64_t status1 = flash_read(c, byte_addr);
> -        /* DQ6 toggles during an erase. */
> -        g_assert_cmphex(status0 & dq6, ==, ~status1 & dq6);
> -        /* Wait for erase to complete. */
> -        qtest_clock_step_next(c->qtest);
> -        /* Ensure DQ6 has stopped toggling. */
> -        g_assert_cmphex(flash_read(c, byte_addr), ==, flash_read(c, byte_addr));
> -        /* Now the data should be valid. */
> -        g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +    uint64_t byte_addr = 0;
> +    for (int region = 0; region < nb_erase_regions; ++region) {
> +        uint64_t base = 0x2D + 4 * region;
> +        flash_cmd(c, CFI_ADDR, CFI_CMD);
> +        uint32_t nb_sectors = flash_query_1(c, FLASH_ADDR(base + 0)) +
> +                              (flash_query_1(c, FLASH_ADDR(base + 1)) << 8) + 1;
> +        uint32_t sector_len = (flash_query_1(c, FLASH_ADDR(base + 2)) << 8) +
> +                              (flash_query_1(c, FLASH_ADDR(base + 3)) << 16);
> +        g_assert_cmphex(nb_sectors, ==, c->nb_blocs[region]);
> +        g_assert_cmphex(sector_len, ==, c->sector_len[region]);
> +        reset(c);
>  
> -        /* Program a bit pattern. */
> -        program(c, byte_addr, 0x55);
> -        g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x55);
> -        program(c, byte_addr, 0xA5);
> -        g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x05);
> +        /* Erase and program sector. */
> +        for (uint32_t i = 0; i < nb_sectors; ++i) {
> +            sector_erase(c, byte_addr);
> +            /* Read toggle. */
> +            uint64_t status0 = flash_read(c, byte_addr);
> +            /* DQ7 is 0 during an erase. */
> +            g_assert_cmphex(status0 & dq7, ==, 0);
> +            uint64_t status1 = flash_read(c, byte_addr);
> +            /* DQ6 toggles during an erase. */
> +            g_assert_cmphex(status0 & dq6, ==, ~status1 & dq6);
> +            /* Wait for erase to complete. */
> +            qtest_clock_step_next(c->qtest);
> +            /* Ensure DQ6 has stopped toggling. */
> +            g_assert_cmphex(flash_read(c, byte_addr), ==,
> +                            flash_read(c, byte_addr));
> +            /* Now the data should be valid. */
> +            g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +
> +            /* Program a bit pattern. */
> +            program(c, byte_addr, 0x55);
> +            g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x55);
> +            program(c, byte_addr, 0xA5);
> +            g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x05);
> +            byte_addr += sector_len;
> +        }
>      }
>  
>      /* Erase the chip. */
> @@ -314,9 +369,11 @@ static void test_flash(const void *opaque)
>      g_assert_cmphex(flash_read(c, 0), ==, flash_read(c, 0));
>      /* Now the data should be valid. */
>  
> -    for (uint32_t i = 0; i < nb_sectors; ++i) {
> -        uint64_t byte_addr = i * sector_len;
> -        g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +    for (int region = 0; region < nb_erase_regions; ++region) {
> +        for (uint32_t i = 0; i < c->nb_blocs[region]; ++i) {
> +            uint64_t byte_addr = i * c->sector_len[region];
> +            g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +        }
>      }
>  
>      /* Unlock bypass */
> @@ -364,6 +421,18 @@ static const FlashConfig configuration[] = {
>      {
>          .bank_width = 2,
>      },
> +    /* Nonuniform sectors (top boot). */
> +    {
> +        .bank_width = 2,
> +        .nb_blocs = { 127, 1, 2, 1 },
> +        .sector_len = { 0x10000, 0x08000, 0x02000, 0x04000 },
> +    },
> +    /* Nonuniform sectors (bottom boot). */
> +    {
> +        .bank_width = 2,
> +        .nb_blocs = { 1, 2, 1, 127 },
> +        .sector_len = { 0x04000, 0x02000, 0x08000, 0x10000 },
> +    },
>  };
>  
>  int main(int argc, char **argv)
> @@ -374,12 +443,12 @@ int main(int argc, char **argv)
>                     strerror(errno));
>          exit(EXIT_FAILURE);
>      }
> -    if (ftruncate(fd, FLASH_SIZE) < 0) {
> +    if (ftruncate(fd, UNIFORM_FLASH_SIZE) < 0) {
>          int error_code = errno;
>          close(fd);
>          unlink(image_path);
>          g_printerr("Failed to truncate file %s to %u MB: %s\n", image_path,
> -                   FLASH_SIZE, strerror(error_code));
> +                   UNIFORM_FLASH_SIZE, strerror(error_code));
>          exit(EXIT_FAILURE);
>      }
>      close(fd);
> @@ -390,9 +459,19 @@ int main(int argc, char **argv)
>      size_t nb_configurations = sizeof configuration / sizeof configuration[0];
>      for (size_t i = 0; i < nb_configurations; ++i) {
>          const FlashConfig *config = &configuration[i];
> -        char *path = g_strdup_printf("pflash-cfi02/%d",
> +        char *path = g_strdup_printf("pflash-cfi02"
> +                                     "/geometry/%dx%x-%dx%x-%dx%x-%dx%x"
> +                                     "/%d",
> +                                     config->nb_blocs[0],
> +                                     config->sector_len[0],
> +                                     config->nb_blocs[1],
> +                                     config->sector_len[1],
> +                                     config->nb_blocs[2],
> +                                     config->sector_len[2],
> +                                     config->nb_blocs[3],
> +                                     config->sector_len[3],
>                                       config->bank_width);
> -        qtest_add_data_func(path, config, test_flash);
> +        qtest_add_data_func(path, config, test_geometry);
>          g_free(path);
>      }
>      int result = g_test_run();
> 

WARNING: multiple messages have this Message-ID (diff)
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Stephen Checkoway <stephen.checkoway@oberlin.edu>, qemu-devel@nongnu.org
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Laurent Vivier" <lvivier@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	qemu-block@nongnu.org, "Peter Maydell" <peter.maydell@linaro.org>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Alistair Francis" <alistair@alistair23.me>,
	"Magnus Damm" <magnus.damm@gmail.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Max Reitz" <mreitz@redhat.com>,
	"Michael Walle" <michael@walle.cc>,
	qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
	"Antony Pavlov" <antonynpavlov@gmail.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	"Jan Kiszka" <jan.kiszka@web.de>, "John Snow" <jsnow@redhat.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [Qemu-devel] [PATCH v5 18/28] hw/block/pflash_cfi02: Implement nonuniform sector sizes
Date: Fri, 28 Jun 2019 19:28:26 +0200	[thread overview]
Message-ID: <241eaafa-e5fa-1653-e789-2ef9ac5ae592@redhat.com> (raw)
In-Reply-To: <20190627202719.17739-19-philmd@redhat.com>

On 6/27/19 10:27 PM, Philippe Mathieu-Daudé wrote:
> From: Stephen Checkoway <stephen.checkoway@oberlin.edu>
> 
> 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 <stephen.checkoway@oberlin.edu>
> Acked-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20190426162624.55977-6-stephen.checkoway@oberlin.edu>
> [PMD: Rebased, add assert() on pri_offset]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi02.c   | 141 +++++++++++++++++++++++++++-------
>  tests/pflash-cfi02-test.c | 155 ++++++++++++++++++++++++++++----------
>  2 files changed, 231 insertions(+), 65 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 01d9c5d75a..1f096ec185 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -29,7 +29,6 @@
>   * - 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
>   * It does not implement multiple sectors erase
> @@ -57,6 +56,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 cycles for CFI queries. */
>  enum {
>      WCYCLE_CFI              = 7,
> @@ -68,8 +74,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];

I think we can drop the 'uniform' variables, and only use the indexed ones.

>      uint32_t chip_len;
>      uint8_t mappings;
>      uint8_t width;
> @@ -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
> @@ -177,6 +185,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)

Hmm I'd rather have a more generic pflash_sector_index(pfl, offset).

> +{
> +    assert(offset < pfl->chip_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;
> @@ -191,10 +218,11 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
>      }
>      offset &= pfl->chip_len - 1;
>      boff = offset & 0xFF;
> -    if (pfl->width == 2)
> +    if (pfl->width == 2) {
>          boff = boff >> 1;
> -    else if (pfl->width == 4)
> +    } else if (pfl->width == 4) {
>          boff = boff >> 2;
> +    }

Simple style, ok.

>      switch (pfl->cmd) {
>      default:
>          /* This should never happen : reset state & treat it as a read*/
> @@ -273,6 +301,7 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
>      hwaddr boff;
>      uint8_t *p;
>      uint8_t cmd;
> +    uint32_t sector_len;
>  
>      trace_pflash_io_write(offset, width, width << 1, value, pfl->wcycle);
>      cmd = value;
> @@ -282,10 +311,11 @@ static void pflash_write(void *opaque, hwaddr offset, uint64_t value,
>      offset &= pfl->chip_len - 1;
>  
>      boff = offset;
> -    if (pfl->width == 2)
> +    if (pfl->width == 2) {
>          boff = boff >> 1;
> -    else if (pfl->width == 4)
> +    } else if (pfl->width == 4) {
>          boff = boff >> 2;
> +    }

Style again.

>      /* Only the least-significant 11 bits are used in most cases. */
>      boff &= 0x7FF;
>      switch (pfl->wcycle) {
> @@ -420,12 +450,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->width * 2, offset,
> +                    pfl->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 done */
> @@ -493,11 +525,11 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      int ret;
>      Error *local_err = NULL;
>  
> -    if (pfl->sector_len == 0) {
> +    if (pfl->uniform_sector_len == 0 && pfl->sector_len[0] == 0) {
>          error_setg(errp, "attribute \"sector-length\" not specified or zero.");
>          return;
>      }
> -    if (pfl->nb_blocs == 0) {
> +    if (pfl->uniform_nb_blocs == 0 && pfl->nb_blocs[0] == 0) {
>          error_setg(errp, "attribute \"num-blocks\" not specified or zero.");
>          return;
>      }
> @@ -506,7 +538,51 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    pfl->chip_len = pfl->sector_len * pfl->nb_blocs;
> +    int nb_regions;
> +    pfl->chip_len = 0;
> +    for (nb_regions = 0; nb_regions < PFLASH_MAX_ERASE_REGIONS; ++nb_regions) {
> +        if (pfl->nb_blocs[nb_regions] == 0) {
> +            break;
> +        }
> +        uint64_t sector_len_per_device = pfl->sector_len[nb_regions];
> +
> +        /*
> +         * The size of each flash sector must be a power of 2 and it must be
> +         * aligned at the same power of 2.
> +         */
> +        if (sector_len_per_device & 0xff ||
> +            sector_len_per_device >= (1 << 24) ||
> +            !is_power_of_2(sector_len_per_device))
> +        {
> +            error_setg(errp, "unsupported configuration: "
> +                       "sector length[%d] per device = %" PRIx64 ".",
> +                       nb_regions, sector_len_per_device);
> +            return;
> +        }
> +        if (pfl->chip_len & (sector_len_per_device - 1)) {
> +            error_setg(errp, "unsupported configuration: "
> +                       "flash region %d not correctly aligned.",
> +                       nb_regions);
> +            return;
> +        }
> +
> +        pfl->chip_len += (uint64_t)pfl->sector_len[nb_regions] *
> +                          pfl->nb_blocs[nb_regions];
> +    }
> +
> +    uint64_t uniform_len = (uint64_t)pfl->uniform_nb_blocs *
> +                           pfl->uniform_sector_len;
> +    if (nb_regions == 0) {
> +        nb_regions = 1;
> +        pfl->nb_blocs[0] = pfl->uniform_nb_blocs;
> +        pfl->sector_len[0] = pfl->uniform_sector_len;
> +        pfl->chip_len = uniform_len;
> +    } else if (uniform_len != 0 && uniform_len != pfl->chip_len) {
> +        error_setg(errp, "\"num-blocks\"*\"sector-length\" "
> +                   "different from \"num-blocks0\"*\'sector-length0\" + ... + "
> +                   "\"num-blocks3\"*\"sector-length3\"");
> +        return;
> +    }
>  
>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
>                                    &pflash_cfi02_ops, pfl, pfl->name,
> @@ -552,7 +628,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->status = 0;
>  
>      /* Hardcoded CFI table (mostly from SG29 Spansion flash) */
> -    const uint16_t pri_ofs = 0x31;
> +    const uint16_t pri_ofs = 0x40;
>      /* Standard "QRY" string */
>      pfl->cfi_table[0x10] = 'Q';
>      pfl->cfi_table[0x11] = 'R';
> @@ -603,14 +679,17 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      //    pfl->cfi_table[0x2A] = 0x05;
>      pfl->cfi_table[0x2A] = 0x00;
>      pfl->cfi_table[0x2B] = 0x00;
> -    /* Number of erase block regions (uniform) */
> -    pfl->cfi_table[0x2C] = 0x01;
> -    /* Erase block region 1 */
> -    pfl->cfi_table[0x2D] = pfl->nb_blocs - 1;
> -    pfl->cfi_table[0x2E] = (pfl->nb_blocs - 1) >> 8;
> -    pfl->cfi_table[0x2F] = pfl->sector_len >> 8;
> -    pfl->cfi_table[0x30] = pfl->sector_len >> 16;
> -    assert(0x30 < pri_ofs);
> +    /* Number of erase block regions */
> +    pfl->cfi_table[0x2c] = nb_regions;
> +    /* Erase block regions */
> +    for (int i = 0; i < nb_regions; ++i) {
> +        uint32_t sector_len_per_device = pfl->sector_len[i];
> +        pfl->cfi_table[0x2d + 4 * i] = pfl->nb_blocs[i] - 1;
> +        pfl->cfi_table[0x2e + 4 * i] = (pfl->nb_blocs[i] - 1) >> 8;
> +        pfl->cfi_table[0x2f + 4 * i] = sector_len_per_device >> 8;
> +        pfl->cfi_table[0x30 + 4 * i] = sector_len_per_device >> 16;
> +    }

OK.

> +    assert(0x2c + 4 * nb_regions < pri_ofs);
>  
>      /* Extended */
>      pfl->cfi_table[0x00 + pri_ofs] = 'P';
> @@ -644,8 +723,16 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>  
>  static Property pflash_cfi02_properties[] = {
>      DEFINE_PROP_DRIVE("drive", PFlashCFI02, blk),
> -    DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, nb_blocs, 0),
> -    DEFINE_PROP_UINT32("sector-length", PFlashCFI02, sector_len, 0),
> +    DEFINE_PROP_UINT32("num-blocks", PFlashCFI02, uniform_nb_blocs, 0),
> +    DEFINE_PROP_UINT32("sector-length", PFlashCFI02, uniform_sector_len, 0),
> +    DEFINE_PROP_UINT32("num-blocks0", PFlashCFI02, nb_blocs[0], 0),
> +    DEFINE_PROP_UINT32("sector-length0", PFlashCFI02, sector_len[0], 0),
> +    DEFINE_PROP_UINT32("num-blocks1", PFlashCFI02, nb_blocs[1], 0),
> +    DEFINE_PROP_UINT32("sector-length1", PFlashCFI02, sector_len[1], 0),
> +    DEFINE_PROP_UINT32("num-blocks2", PFlashCFI02, nb_blocs[2], 0),
> +    DEFINE_PROP_UINT32("sector-length2", PFlashCFI02, sector_len[2], 0),
> +    DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0),
> +    DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0),
>      DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0),
>      DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0),
>      DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0),
> diff --git a/tests/pflash-cfi02-test.c b/tests/pflash-cfi02-test.c
> index b00f5ca2e7..1659eaebce 100644
> --- a/tests/pflash-cfi02-test.c
> +++ b/tests/pflash-cfi02-test.c
> @@ -17,9 +17,11 @@
>   */
>  
>  #define MP_FLASH_SIZE_MAX (32 * 1024 * 1024)
> -#define FLASH_SIZE (8 * 1024 * 1024)
>  #define BASE_ADDR (0x100000000ULL - MP_FLASH_SIZE_MAX)
>  
> +#define UNIFORM_FLASH_SIZE (8 * 1024 * 1024)
> +#define UNIFORM_FLASH_SECTOR_SIZE (64 * 1024)
> +
>  /* Use a newtype to keep flash addresses separate from byte addresses. */
>  typedef struct {
>      uint64_t addr;
> @@ -44,6 +46,10 @@ typedef struct {
>  typedef struct {
>      int bank_width;
>  
> +    /* Nonuniform block size. */
> +    int nb_blocs[4];
> +    int sector_len[4];
> +
>      QTestState *qtest;
>  } FlashConfig;
>  
> @@ -62,6 +68,10 @@ static FlashConfig expand_config_defaults(const FlashConfig *c)
>      if (ret.bank_width == 0) {
>          ret.bank_width = 2;
>      }
> +    if (ret.nb_blocs[0] == 0 && ret.sector_len[0] == 0) {
> +        ret.sector_len[0] = UNIFORM_FLASH_SECTOR_SIZE;
> +        ret.nb_blocs[0] = UNIFORM_FLASH_SIZE / UNIFORM_FLASH_SECTOR_SIZE;

See, here you use index=0 for the uniform config.

> +    }
>  
>      /* XXX: Limitations of test harness. */
>      assert(ret.bank_width == 2);
> @@ -230,13 +240,41 @@ static void chip_erase(const FlashConfig *c)
>      flash_cmd(c, UNLOCK0_ADDR, CHIP_ERASE_CMD);
>  }
>  
> -static void test_flash(const void *opaque)
> +/*
> + * Test flash commands with a variety of device geometry.
> + */
> +static void test_geometry(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);
> +                        " -drive if=pflash,file=%s,format=raw,copy-on-read"
> +                        /* Device geometry properties. */
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks0,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length0,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks1,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length1,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks2,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length2,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=num-blocks3,value=%d"
> +                        " -global driver=cfi.pflash02,"
> +                        "property=sector-length3,value=%d",
> +                        image_path,
> +                        config->nb_blocs[0],
> +                        config->sector_len[0],
> +                        config->nb_blocs[1],
> +                        config->sector_len[1],
> +                        config->nb_blocs[2],
> +                        config->sector_len[2],
> +                        config->nb_blocs[3],
> +                        config->sector_len[3]);
>      FlashConfig explicit_config = expand_config_defaults(config);
>      explicit_config.qtest = qtest;
>      const FlashConfig *c = &explicit_config;
> @@ -264,39 +302,56 @@ static void test_flash(const void *opaque)
>      g_assert_cmphex(flash_query(c, FLASH_ADDR(0x12)), ==, replicate(c, 'Y'));
>  
>      /* Num erase regions. */
> -    g_assert_cmphex(flash_query_1(c, FLASH_ADDR(0x2C)), >=, 1);
> +    int nb_erase_regions = flash_query_1(c, FLASH_ADDR(0x2C));
> +    g_assert_cmphex(nb_erase_regions, ==,
> +                    !!c->nb_blocs[0] + !!c->nb_blocs[1] + !!c->nb_blocs[2] +
> +                    !!c->nb_blocs[3]);
> +
> +    /* Check device length. */
> +    uint32_t device_len = 1 << flash_query_1(c, FLASH_ADDR(0x27));
> +    g_assert_cmphex(device_len, ==, UNIFORM_FLASH_SIZE);
>  
> -    uint32_t nb_sectors = flash_query_1(c, FLASH_ADDR(0x2D)) +
> -                          (flash_query_1(c, FLASH_ADDR(0x2E)) << 8) + 1;
> -    uint32_t sector_len = (flash_query_1(c, FLASH_ADDR(0x2F)) << 8) +
> -                          (flash_query_1(c, FLASH_ADDR(0x30)) << 16);
>      reset(c);
>  
>      const uint64_t dq7 = replicate(c, 0x80);
>      const uint64_t dq6 = replicate(c, 0x40);
> -    /* Erase and program sector. */
> -    for (uint32_t i = 0; i < nb_sectors; ++i) {
> -        uint64_t byte_addr = i * sector_len;
> -        sector_erase(c, byte_addr);
> -        /* Read toggle. */
> -        uint64_t status0 = flash_read(c, byte_addr);
> -        /* DQ7 is 0 during an erase. */
> -        g_assert_cmphex(status0 & dq7, ==, 0);
> -        uint64_t status1 = flash_read(c, byte_addr);
> -        /* DQ6 toggles during an erase. */
> -        g_assert_cmphex(status0 & dq6, ==, ~status1 & dq6);
> -        /* Wait for erase to complete. */
> -        qtest_clock_step_next(c->qtest);
> -        /* Ensure DQ6 has stopped toggling. */
> -        g_assert_cmphex(flash_read(c, byte_addr), ==, flash_read(c, byte_addr));
> -        /* Now the data should be valid. */
> -        g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +    uint64_t byte_addr = 0;
> +    for (int region = 0; region < nb_erase_regions; ++region) {
> +        uint64_t base = 0x2D + 4 * region;
> +        flash_cmd(c, CFI_ADDR, CFI_CMD);
> +        uint32_t nb_sectors = flash_query_1(c, FLASH_ADDR(base + 0)) +
> +                              (flash_query_1(c, FLASH_ADDR(base + 1)) << 8) + 1;
> +        uint32_t sector_len = (flash_query_1(c, FLASH_ADDR(base + 2)) << 8) +
> +                              (flash_query_1(c, FLASH_ADDR(base + 3)) << 16);
> +        g_assert_cmphex(nb_sectors, ==, c->nb_blocs[region]);
> +        g_assert_cmphex(sector_len, ==, c->sector_len[region]);
> +        reset(c);
>  
> -        /* Program a bit pattern. */
> -        program(c, byte_addr, 0x55);
> -        g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x55);
> -        program(c, byte_addr, 0xA5);
> -        g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x05);
> +        /* Erase and program sector. */
> +        for (uint32_t i = 0; i < nb_sectors; ++i) {
> +            sector_erase(c, byte_addr);
> +            /* Read toggle. */
> +            uint64_t status0 = flash_read(c, byte_addr);
> +            /* DQ7 is 0 during an erase. */
> +            g_assert_cmphex(status0 & dq7, ==, 0);
> +            uint64_t status1 = flash_read(c, byte_addr);
> +            /* DQ6 toggles during an erase. */
> +            g_assert_cmphex(status0 & dq6, ==, ~status1 & dq6);
> +            /* Wait for erase to complete. */
> +            qtest_clock_step_next(c->qtest);
> +            /* Ensure DQ6 has stopped toggling. */
> +            g_assert_cmphex(flash_read(c, byte_addr), ==,
> +                            flash_read(c, byte_addr));
> +            /* Now the data should be valid. */
> +            g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +
> +            /* Program a bit pattern. */
> +            program(c, byte_addr, 0x55);
> +            g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x55);
> +            program(c, byte_addr, 0xA5);
> +            g_assert_cmphex(flash_read(c, byte_addr) & 0xFF, ==, 0x05);
> +            byte_addr += sector_len;
> +        }
>      }
>  
>      /* Erase the chip. */
> @@ -314,9 +369,11 @@ static void test_flash(const void *opaque)
>      g_assert_cmphex(flash_read(c, 0), ==, flash_read(c, 0));
>      /* Now the data should be valid. */
>  
> -    for (uint32_t i = 0; i < nb_sectors; ++i) {
> -        uint64_t byte_addr = i * sector_len;
> -        g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +    for (int region = 0; region < nb_erase_regions; ++region) {
> +        for (uint32_t i = 0; i < c->nb_blocs[region]; ++i) {
> +            uint64_t byte_addr = i * c->sector_len[region];
> +            g_assert_cmphex(flash_read(c, byte_addr), ==, bank_mask(c));
> +        }
>      }
>  
>      /* Unlock bypass */
> @@ -364,6 +421,18 @@ static const FlashConfig configuration[] = {
>      {
>          .bank_width = 2,
>      },
> +    /* Nonuniform sectors (top boot). */
> +    {
> +        .bank_width = 2,
> +        .nb_blocs = { 127, 1, 2, 1 },
> +        .sector_len = { 0x10000, 0x08000, 0x02000, 0x04000 },
> +    },
> +    /* Nonuniform sectors (bottom boot). */
> +    {
> +        .bank_width = 2,
> +        .nb_blocs = { 1, 2, 1, 127 },
> +        .sector_len = { 0x04000, 0x02000, 0x08000, 0x10000 },
> +    },
>  };
>  
>  int main(int argc, char **argv)
> @@ -374,12 +443,12 @@ int main(int argc, char **argv)
>                     strerror(errno));
>          exit(EXIT_FAILURE);
>      }
> -    if (ftruncate(fd, FLASH_SIZE) < 0) {
> +    if (ftruncate(fd, UNIFORM_FLASH_SIZE) < 0) {
>          int error_code = errno;
>          close(fd);
>          unlink(image_path);
>          g_printerr("Failed to truncate file %s to %u MB: %s\n", image_path,
> -                   FLASH_SIZE, strerror(error_code));
> +                   UNIFORM_FLASH_SIZE, strerror(error_code));
>          exit(EXIT_FAILURE);
>      }
>      close(fd);
> @@ -390,9 +459,19 @@ int main(int argc, char **argv)
>      size_t nb_configurations = sizeof configuration / sizeof configuration[0];
>      for (size_t i = 0; i < nb_configurations; ++i) {
>          const FlashConfig *config = &configuration[i];
> -        char *path = g_strdup_printf("pflash-cfi02/%d",
> +        char *path = g_strdup_printf("pflash-cfi02"
> +                                     "/geometry/%dx%x-%dx%x-%dx%x-%dx%x"
> +                                     "/%d",
> +                                     config->nb_blocs[0],
> +                                     config->sector_len[0],
> +                                     config->nb_blocs[1],
> +                                     config->sector_len[1],
> +                                     config->nb_blocs[2],
> +                                     config->sector_len[2],
> +                                     config->nb_blocs[3],
> +                                     config->sector_len[3],
>                                       config->bank_width);
> -        qtest_add_data_func(path, config, test_flash);
> +        qtest_add_data_func(path, config, test_geometry);
>          g_free(path);
>      }
>      int result = g_test_run();
> 


  reply	other threads:[~2019-06-28 17:28 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-27 20:26 [PATCH v5 00/28] block/pflash_cfi02: Implement missing AMD pflash functionality Philippe Mathieu-Daudé
2019-06-27 20:26 ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-27 20:26 ` [PATCH v5 01/28] tests/pflash-cfi02: Add test for supported CFI commands Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-27 20:26 ` [PATCH v5 02/28] hw/block/pflash: Simplify trace_pflash_io_read/write() Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-27 21:49   ` Stephen Checkoway
2019-06-27 21:49     ` [Qemu-devel] " Stephen Checkoway
2019-06-28 15:53     ` Alistair Francis
2019-06-28 15:53       ` Alistair Francis
2019-06-28 15:52   ` Alistair Francis
2019-06-28 15:52     ` Alistair Francis
2019-06-27 20:26 ` [PATCH v5 03/28] hw/block/pflash: Simplify trace_pflash_data_read/write() Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 15:54   ` Alistair Francis
2019-06-28 15:54     ` Alistair Francis
2019-06-27 20:26 ` [PATCH v5 04/28] hw/block/pflash_cfi02: Fix debug format string Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 15:55   ` Alistair Francis
2019-06-28 15:55     ` Alistair Francis
2019-06-27 20:26 ` [PATCH v5 05/28] hw/block/pflash_cfi02: Add an enum to define the write cycles Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 15:56   ` Alistair Francis
2019-06-28 15:56     ` Alistair Francis
2019-06-27 20:26 ` [PATCH v5 06/28] hw/block/pflash_cfi02: Add helpers to manipulate the status bits Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:00   ` Alistair Francis
2019-06-28 16:00     ` Alistair Francis
2019-06-27 20:26 ` [PATCH v5 07/28] hw/block/pflash_cfi02: Simplify a statement using fall through Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:01   ` Alistair Francis
2019-06-28 16:01     ` Alistair Francis
2019-06-27 20:26 ` [PATCH v5 08/28] hw/block/pflash_cfi02: Use the ldst API in pflash_write() Philippe Mathieu-Daudé
2019-06-27 20:26   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:03   ` Alistair Francis
2019-06-28 16:03     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 09/28] hw/block/pflash_cfi02: Use the ldst API in pflash_read() Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:03   ` Alistair Francis
2019-06-28 16:03     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 10/28] hw/block/pflash_cfi02: Extract the pflash_data_read() function Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:17   ` Alistair Francis
2019-06-28 16:17     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 11/28] hw/block/pflash_cfi02: Unify the MemoryRegionOps Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:25   ` Alistair Francis
2019-06-28 16:25     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 12/28] hw/block/pflash_cfi02: Fix command address comparison Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:28   ` Alistair Francis
2019-06-28 16:28     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 13/28] tests/pflash-cfi02: Refactor to support testing multiple configurations Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:41   ` Alistair Francis
2019-06-28 16:41     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 14/28] hw/block/pflash_cfi02: Remove pointless local variable Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:32   ` Alistair Francis
2019-06-28 16:32     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 15/28] hw/block/pflash_cfi02: Document the current CFI values Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:33   ` Alistair Francis
2019-06-28 16:33     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 16/28] hw/block/pflash_cfi02: Hold the PRI table offset in a variable Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:34   ` Alistair Francis
2019-06-28 16:34     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 17/28] hw/block/pflash_cfi02: Document 'Page Mode' operations are not supported Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:35   ` Alistair Francis
2019-06-28 16:35     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 18/28] hw/block/pflash_cfi02: Implement nonuniform sector sizes Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 17:28   ` Philippe Mathieu-Daudé [this message]
2019-06-28 17:28     ` Philippe Mathieu-Daudé
2019-06-27 20:27 ` [PATCH v5 19/28] hw/block/pflash_cfi02: Extract pflash_regions_count() Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 16:43   ` Alistair Francis
2019-06-28 16:43     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 20/28] hw/block/pflash_cfi02: Split if() condition Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 23:06   ` Alistair Francis
2019-06-28 23:06     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 21/28] hw/block/pflash_cfi02: Fix CFI in autoselect mode Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 23:08   ` Alistair Francis
2019-06-28 23:08     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 22/28] hw/block/pflash_cfi02: Fix reset command not ignored during erase Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 23:11   ` Alistair Francis
2019-06-28 23:11     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 23/28] hw/block/pflash_cfi02: Implement multi-sector erase Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-27 20:27 ` [PATCH v5 24/28] hw/block/pflash_cfi02: Implement erase suspend/resume Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-27 20:27 ` [PATCH v5 25/28] hw/block/pflash_cfi02: Use chip erase time specified in the CFI table Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 23:12   ` Alistair Francis
2019-06-28 23:12     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 26/28] hw/block/pflash_cfi02: Reduce single byte/word write delay Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-27 20:27 ` [PATCH v5 27/28] hw/block/pflash_cfi02: Document commands Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 23:13   ` Alistair Francis
2019-06-28 23:13     ` Alistair Francis
2019-06-27 20:27 ` [PATCH v5 28/28] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit Philippe Mathieu-Daudé
2019-06-27 20:27   ` [Qemu-devel] " Philippe Mathieu-Daudé
2019-06-28 23:13   ` Alistair Francis
2019-06-28 23:13     ` Alistair Francis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=241eaafa-e5fa-1653-e789-2ef9ac5ae592@redhat.com \
    --to=philmd@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=alistair@alistair23.me \
    --cc=antonynpavlov@gmail.com \
    --cc=armbru@redhat.com \
    --cc=aurelien@aurel32.net \
    --cc=david@gibson.dropbear.id.au \
    --cc=edgar.iglesias@gmail.com \
    --cc=jan.kiszka@web.de \
    --cc=jsnow@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=lvivier@redhat.com \
    --cc=magnus.damm@gmail.com \
    --cc=michael@walle.cc \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=stephen.checkoway@oberlin.edu \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.