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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Hi David,
> 
> On 7/9/19 12:30 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> >> In the "Read Array Flowchart" the command has a value of 0xFF.
> >>
> >> 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
> >>
> >> To keep migration behaving correctly, we have to increase
> >> the VMState version. When migrating from an older version,
> >> we use the correct command value.
> > 
> > The problem is that incrementing the version will break backwards
> > compatibility; so you won't be able to migrate this back to an older
> > QEMU version; so for example a q35/uefi with this won't be able
> > to migrate backwards to a 4.0.0 or older qemu.
> > 
> > So instead of bumping the version_id you probably need to wire
> > the behaviour to a machine type and then on your new type
> > wire a subsection containing a flag; the reception of that subsection
> > tells you to use the new/correct semantics.
> 
> I'm starting to understand VMState subsections, but it might be overkill
> for this change...
> 
>   Subsections
>   -----------
> 
>   The most common structure change is adding new data, e.g. when adding
>   a newer form of device, or adding that state that you previously
>   forgot to migrate.  This is best solved using a subsection.
> 
> This is not the case here, the field is already present and migrated.
> 
> It seems I can use a simple pre_save hook, always migrating the
> READ_ARRAY using the incorrect value:
> 
> -- >8 --
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -97,12 +97,29 @@ struct PFlashCFI01 {
>      bool incorrect_read_array_command;
>  };
> 
> +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;
> +}

Be careful what happens if migration fails and you continue on the
source - is that OK - or are you going to have to flip that back somehow
(in a post_save).

Another way to do the same is to have a dummy field; tmp_cmd, and the
tmp_cmd is the thing that's actually migrated and filled by pre_save
(or use VMSTATE_WITH_TMP )


>  static int pflash_post_load(void *opaque, int version_id);
> 
>  static const VMStateDescription vmstate_pflash = {
>      .name = "pflash_cfi01",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .pre_save = pflash_pre_save,
>      .post_load = pflash_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(wcycle, PFlashCFI01),
> @@ -1001,5 +1018,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;
>  }
> ---
> 
> Being simpler and less intrusive (no new property in hw/core/machine.c),
> is this acceptable?

From the migration point of view yes; I don't know enough about pflash
to say if it makes sense;  for example could there ever be a 00 command
really used and then you'd have to distinguish that somehow?

> Thanks,
> 
> Phil.
> 
> [...]
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK