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

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> 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.

See the previous 4 versions of discussion....

> 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()

It is the same pflash data structure; but all CPUs are stopped and we're
just walking the list of devices serialising them; so no nothing should
be seeing that value.
(There is another way to do this, which is to produce a temporary
structure at this point, populate the temporary structure and migrate
that)

Dave

> * 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;
> >  }
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK