Re: [Qemu-devel] [PATCH-for-4.1 v6 2/5] hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00'

[Alistair Francis]

On Tue, Jul 16, 2019 at 3:16 PM Philippe Mathieu-Daudé
<address@hidden> wrote:
>
> The command 0x00 is used by this model since its origin (commit
> 05ee37ebf630). In this commit the command is described with a
> amusing '/* ??? */' comment, probably meaning 'FIXME'.
>
>         switch (cmd) {
>         case 0x00: /* ??? */
>             ...
>
> This comment survived 12 years because the 0x00 value is indeed
> not specified by the CFI open standard (as of this commit).
>
> The 'cmd' field is transfered during migration. To keep the
> migration feature working with older QEMU version, we have to
> take a lot of care with migrated field. We figured out it is
> too late to remove a non-specified value from this model
> (this would make migration review very complex). It is however
> not too late to improve the documentation.
>
> Add few comments to remember this is a special value related
> to QEMU, and we won't find information about it on the CFI
> spec.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>

Reviewed-by: Alistair Francis <address@hidden>

Alistair

> ---
> v6: new patch
> ---
>  hw/block/pflash_cfi01.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index a9529957f8..6838e8a1ab 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -277,9 +277,13 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr 
> offset,
>          /* This should never happen : reset state & treat it as a read */
>          DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
>          pfl->wcycle = 0;
> -        pfl->cmd = 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;
>          /* fall through to read code */
> -    case 0x00:
> +    case 0x00: /* This model reset value for READ_ARRAY (not CFI compliant) 
> */
>          /* Flash area read */
>          ret = pflash_data_read(pfl, offset, width, be);
>          break;
> @@ -448,7 +452,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      case 0:
>          /* read mode */
>          switch (cmd) {
> -        case 0x00: /* ??? */
> +        case 0x00: /* This model reset value for READ_ARRAY (not CFI) */
>              goto reset_flash;
>          case 0x10: /* Single Byte Program */
>          case 0x40: /* Single Byte Program */
> @@ -645,7 +649,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      trace_pflash_reset();
>      memory_region_rom_device_set_romd(&pfl->mem, true);
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0x00; /* This model reset value for READ_ARRAY (not CFI) */
>  }
>
>
> @@ -761,7 +765,11 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
> **errp)
>      }
>
>      pfl->wcycle = 0;
> -    pfl->cmd = 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;
>      pfl->status = 0x80; /* WSM ready */
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> --
> 2.20.1
>
>