[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH-for-4.1 v6 3/5] hw/block/pflash_cfi
From: |
Alistair Francis |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH-for-4.1 v6 3/5] hw/block/pflash_cfi01: Extract pflash_mode_read_array() |
Date: |
Tue, 16 Jul 2019 15:53:36 -0700 |
On Tue, Jul 16, 2019 at 3:16 PM Philippe Mathieu-Daudé
<address@hidden> wrote:
>
> The same pattern is used when setting the flash in READ_ARRAY mode:
> - Set the state machine command to READ_ARRAY
> - Reset the write_cycle counter
> - Reset the memory region in ROMD
>
> Refactor the current code by extracting this pattern.
> It is used three times:
>
> - On a read access (on invalid command).
>
> Note this default case is not reachable by the state machine
> updates in pflash_data_write(). However we might reach this
> case migrating from a future QEMU version that would implement
> newer commands, without incrementing the migration version.
> Since we never know, we keep this default case.
>
> Previous to this patch, an invalid read command would not reset
> the memory region in ROMD mode, so:
>
> . A further read access would keep going into I/O mode, calling
> the same switch in pflash_read(). Undefined behaviour, probably
> unexpected.
> . A further write access in I/O mode. Since the default case set
> (wcycle=0, cmd=0x00), we jump to reset_flash which set the flash
> in READ_ARRAY.
>
> After this patch, if we get an invalid read command we directly
> set (wcycle=0, cmd=0x00) and put the device in ROMD mode.
> Further I/O access are now properly handled.
>
> - On a write access (on command failure, error, or explicitly asked)
>
> - When the device is initialized. Here the ROMD mode is hidden
> by the memory_region_init_rom_device() call.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
Reviewed-by: Alistair Francis <address@hidden>
Alistair
> ---
> v6: split of the label rename to ease review
> the pattern is used 3 times (was 2 times previously)
> describe the 3rd time and reset the review tags :(
> ---
> hw/block/pflash_cfi01.c | 31 +++++++++++++++----------------
> hw/block/trace-events | 1 +
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 6838e8a1ab..a28d0f8cc7 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -112,6 +112,18 @@ static const VMStateDescription vmstate_pflash = {
> }
> };
>
> +static void pflash_mode_read_array(PFlashCFI01 *pfl)
> +{
> + trace_pflash_mode_read_array();
> + /*
> + * The command 0x00 is not assigned by the CFI open standard,
> + * but QEMU historically uses it for the READ_ARRAY command (0xff).
> + */
> + pfl->cmd = 0x00;
> + pfl->wcycle = 0;
> + memory_region_rom_device_set_romd(&pfl->mem, true);
> +}
> +
> /* Perform a CFI query based on the bank width of the flash.
> * If this code is called we know we have a device_width set for
> * this flash.
> @@ -276,12 +288,7 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr
> offset,
> default:
> /* This should never happen : reset state & treat it as a read */
> DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
> - pfl->wcycle = 0;
> - /*
> - * The command 0x00 is not assigned by the CFI open standard,
> - * but QEMU historically uses it for the READ_ARRAY command (0xff).
> - */
> - pfl->cmd = 0x00;
> + pflash_mode_read_array(pfl);
> /* fall through to read code */
> case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant)
> */
> /* Flash area read */
> @@ -646,10 +653,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> "\n", __func__, offset, pfl->wcycle, pfl->cmd, value);
>
> reset_flash:
> - trace_pflash_reset();
> - memory_region_rom_device_set_romd(&pfl->mem, true);
> - pfl->wcycle = 0;
> - pfl->cmd = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
> + pflash_mode_read_array(pfl);
> }
>
>
> @@ -764,12 +768,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
> **errp)
> pfl->max_device_width = pfl->device_width;
> }
>
> - pfl->wcycle = 0;
> - /*
> - * The command 0x00 is not assigned by the CFI open standard,
> - * but QEMU historically uses it for the READ_ARRAY command (0xff).
> - */
> - pfl->cmd = 0x00;
> + pflash_mode_read_array(pfl);
> pfl->status = 0x80; /* WSM ready */
> /* Hardcoded CFI table */
> /* Standard "QRY" string */
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 13d1b21dd4..91a8a106c0 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -7,6 +7,7 @@ fdc_ioport_write(uint8_t reg, uint8_t value) "write reg
> 0x%02x val 0x%02x"
> # pflash_cfi02.c
> # pflash_cfi01.c
> pflash_reset(void) "reset"
> +pflash_mode_read_array(void) "mode: read array"
> pflash_timer_expired(uint8_t cmd) "command 0x%02x done"
> pflash_io_read(uint64_t offset, int width, int fmt_width, uint32_t value,
> uint8_t cmd, uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x
> cmd:0x%02x wcycle:%u"
> pflash_io_write(uint64_t offset, int width, int fmt_width, uint32_t value,
> uint8_t wcycle) "offset:0x%04"PRIx64" width:%d value:0x%0*x wcycle:%u"
> --
> 2.20.1
>
>
- [Qemu-block] [PATCH-for-4.1 v6 0/5] hw/block/pflash_cfi01: Add DeviceReset() handler, Philippe Mathieu-Daudé, 2019/07/16
- [Qemu-block] [PATCH-for-4.1 v6 1/5] hw/block/pflash_cfi01: Removed an unused timer, Philippe Mathieu-Daudé, 2019/07/16
- [Qemu-block] [PATCH-for-4.1 v6 2/5] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00', Philippe Mathieu-Daudé, 2019/07/16
- [Qemu-block] [PATCH-for-4.1 v6 3/5] hw/block/pflash_cfi01: Extract pflash_mode_read_array(), Philippe Mathieu-Daudé, 2019/07/16
- Re: [Qemu-block] [Qemu-devel] [PATCH-for-4.1 v6 3/5] hw/block/pflash_cfi01: Extract pflash_mode_read_array(),
Alistair Francis <=
- [Qemu-block] [PATCH-for-4.1 v6 4/5] hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array', Philippe Mathieu-Daudé, 2019/07/16
- [Qemu-block] [PATCH-for-4.1 v6 5/5] hw/block/pflash_cfi01: Add the DeviceReset() handler, Philippe Mathieu-Daudé, 2019/07/16
- Re: [Qemu-block] [PATCH-for-4.1 v6 0/5] hw/block/pflash_cfi01: Add DeviceReset() handler, Laszlo Ersek, 2019/07/17