On Tue, Jul 16, 2019 at 3:16 PM Philippe Mathieu-Daudé <phi...@redhat.com> 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é <phi...@redhat.com>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> 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 > >