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

[Philippe Mathieu-Daudé]

Hi David,

On 7/9/19 12:30 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (address@hidden) 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.

[...]