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;
+}
+
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?
Thanks,
Phil.
[...]
