* Philippe Mathieu-Daudé (phi...@redhat.com) wrote: > Hi David, > > On 7/9/19 12:30 PM, Dr. David Alan Gilbert wrote: > > * Philippe Mathieu-Daudé (phi...@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 / dgilb...@redhat.com / Manchester, UK