Re: [Qemu-devel] [PATCH v5 2/5] hw/block/pflash_cfi01: Use the correct READ_ARRAY value

[Markus Armbruster]

Philippe asked me to have a look at this one, so here goes.

Philippe Mathieu-Daudé <address@hidden> writes:

> In the document [*] the "Read Array Flowchart", the READ_ARRAY
> command has a value of 0xff.
>
> Use the correct value in the pflash model.
>
> There is no change of behavior in the guest, because:
> - when the guest were sending 0xFF, the reset_flash label
>   was setting the command value as 0x00
> - 0x00 was used internally for READ_ARRAY

*Groan*

Is this cleanup, or does it fix an observable bug?

> To keep migration with older versions behaving correctly, we
> decide to always migrate the READ_ARRAY as 0x00.
>
> If the CFI open standard decide to assign a new command of value
> 0x00, this model is flawed because it uses this value internally.
> If a guest eventually requires this new CFI feature, a different
> model will be required (or this same model but breaking backward
> migration). So it is safe to keep migrating READ_ARRAY as 0x00.

We could perhaps keep migration working for "benign" device states, with
judicious use of subsections.  We'll cross that bridge when we get to
it.

> [*] "Common Flash Interface (CFI) and Command Sets"
>     (Intel Application Note 646)
>     Appendix B "Basic Command Set"
>
> Reviewed-by: John Snow <address@hidden>
> Reviewed-by: Alistair Francis <address@hidden>
> Regression-tested-by: Laszlo Ersek <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> v3: Handle migrating the 'cmd' field.
> v4: Handle migrating to older QEMU (Dave)
> v5: Add a paragraph about why this model is flawed due to
>     historically using READ_ARRAY as 0x00 (Dave, Peter).
>
> Since Laszlo stated he did not test migration [*], I'm keeping his
> test tag, because the change with v2 has no impact in the tests
> he ran.
>
> Likewise I'm keeping John and Alistair tags, but I'd like an extra
> review for the migration change, thanks!
>
> [*] https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00679.html
> ---
>  hw/block/pflash_cfi01.c | 57 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9e34fd4e82..85bb2132c0 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -96,6 +96,37 @@ struct PFlashCFI01 {
>      bool old_multiple_chip_handling;
>  };
>  
> +static int pflash_pre_save(void *opaque)
> +{
> +    PFlashCFI01 *s = opaque;
> +
> +    /*
> +     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
> +     * READ_ARRAY command. To preserve migrating to these older version,
> +     * always migrate the READ_ARRAY command as 0x00.
> +     */
> +    if (s->cmd == 0xff) {
> +        s->cmd = 0x00;
> +    }
> +
> +    return 0;
> +}
> +
> +static int pflash_post_save(void *opaque)
> +{
> +    PFlashCFI01 *s = opaque;
> +
> +    /*
> +     * If migration failed, the guest will continue to run.
> +     * Restore the correct READ_ARRAY value.
> +     */
> +    if (s->cmd == 0x00) {
> +        s->cmd = 0xff;
> +    }
> +
> +    return 0;
> +}

Uh, this gives me a queasy feeling.  Perhaps David can assuage it.

I figure the intent is to migrate PFlashCFI01 member @cmd value 0xFF as
0x00, for migration compatibility to and from older versions.

You do this by monkey-patching it to 0x00 before migration, and to 0xFF
afterwards.  On the incoming side, you replace 0x00 by 0xFF, in
pflash_post_load() below.

Questions:

* Can anything but the code that sends @cmd see the temporary 0x00 value
  between pflash_pre_save() and pflash_post_save()

* Consider the matrix source \in { old, new } x dest \in { old, new } x
  @cmd on source in { 0x00, 0xFF }.  What does migration put into @cmd
  on dest?  Eight cases:

  source @cmd  ->  wire  ->  dest @cmd
    old  0x00  ->  0x00  ->  old  0x00  (1)
                             new  0xFF  (2)
    old  0xFF  ->  0xFF  ->  old  0xFF  (3)
                             new  0xFF  (4)
    new  0x00  ->  0x00  ->  old  0x00  (5)
                             new  0xFF  (6)
    new  0xFF  ->  0x00  ->  old  0x00  (7)
                             new  0xFF  (8)

  Old -> old (cases 1 and 3) is unaffected by this patch.

  New -> new leaves 0xFF unchanged (8).  It changes 0x00 to 0xFF (6).
  Uh-oh.  Can this happen?  Rephrasing the question: can @cmd ever be
  0x00 with this patch applied?

  Old -> new leaves 0xFF unchanged (4).  It changes 0x00 to 0xFF (2),
  which I think is intentional.

  New -> old leaves 0x00 unchanged (5).  It changes 0xFF to 0x00 (7),
  which I think is intentional.

  Old -> new -> old leaves 0x00 unchanged.  Good.  It changes 0xFF to
  0x00.  Uh-oh.  Can @cmd ever be 0xFF before this patch?

  New -> old -> new leaves 0xFF unchanged.  Good.  It changes 0x00 to
  0xFF.  Same uh-oh as for new -> new.

> +
>  static int pflash_post_load(void *opaque, int version_id);
>  
>  static const VMStateDescription vmstate_pflash = {
> @@ -103,6 +134,8 @@ static const VMStateDescription vmstate_pflash = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .post_load = pflash_post_load,
> +    .pre_save = pflash_pre_save,
> +    .post_save = pflash_post_save,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(wcycle, PFlashCFI01),
>          VMSTATE_UINT8(cmd, PFlashCFI01),
> @@ -277,10 +310,9 @@ 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;
> +        pfl->cmd = 0xff;
>          /* fall through to read code */
> -    case 0x00:
> -        /* Flash area read */
> +    case 0xff: /* Read Array */
>          ret = pflash_data_read(pfl, offset, width, be);
>          break;

On 0xFF, we no longer zap pfl->wcycle and pfl->cmd.

On 0x00, we do.

We zap pfl->cmd to 0xFF instead of 0x00.  Same below after label
error_flash and reset_flash.  Related: initialization to 0xFF instead of
0x00 in pflash_cfi01_realize().  I *guess* these changes together ensure
pfl->cmd can't become 0x00.  Correct?

>      case 0x10: /* Single byte program */
> @@ -448,8 +480,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>      case 0:
>          /* read mode */
>          switch (cmd) {
> -        case 0x00: /* ??? */
> -            goto reset_flash;

On 0x00, we now use default: goto error_flash.  Can this happen?

>          case 0x10: /* Single Byte Program */
>          case 0x40: /* Single Byte Program */
>              DPRINTF("%s: Single Byte Program\n", __func__);
> @@ -526,7 +556,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              if (cmd == 0xd0) { /* confirm */
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) { /* read array mode */
> +            } else if (cmd == 0xff) { /* Read Array */
>                  goto reset_flash;
>              } else
>                  goto error_flash;
> @@ -553,7 +583,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>              } else if (cmd == 0x01) {
>                  pfl->wcycle = 0;
>                  pfl->status |= 0x80;
> -            } else if (cmd == 0xff) {
> +            } else if (cmd == 0xff) { /* read array mode */

Your new comment is phrased the way you corrected in the previous hunk.
Intentional?

>                  goto reset_flash;
>              } else {
>                  DPRINTF("%s: Unknown (un)locking command\n", __func__);
> @@ -645,7 +675,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
    error_flash:
       qemu_log_mask(LOG_UNIMP, "%s: Unimplemented flash cmd sequence "
                     "(offset " TARGET_FMT_plx ", wcycle 0x%x cmd 0x%x value 
0x%x)"
                     "\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 = 0;
> +    pfl->cmd = 0xff;
>  }
>  
>  
> @@ -761,7 +791,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
> **errp)
>      }
>  
>      pfl->wcycle = 0;
> -    pfl->cmd = 0;
> +    pfl->cmd = 0xff;
>      pfl->status = 0;
>      /* Hardcoded CFI table */
>      /* Standard "QRY" string */
> @@ -1001,5 +1031,14 @@ static int pflash_post_load(void *opaque, int 
> version_id)
>          pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb,
>                                                          pfl);
>      }
> +
> +    /*
> +     * Previous to QEMU v4.1 an incorrect value of 0x00 was used for the
> +     * READ_ARRAY command.
> +     */
> +    if (pfl->cmd == 0x00) {
> +        pfl->cmd = 0xff;
> +    }
> +
>      return 0;
>  }