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

[Philippe Mathieu-Daudé]

On 7/12/19 5:15 PM, Peter Maydell wrote:
> On Thu, 11 Jul 2019 at 16:58, 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 with older versions behaving correctly, we
>> decide to always migrate the READ_ARRAY as 0x00.
>>
>> [*] "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>
>> ---
> 
> These changes look correct as far as they go, but are
> we sure that command value 0x00 will never be a valid
> command in some future version? If it ever does, then we
> have a problem because we can't distinguish "0xff with
> a silly encoding" from "really 0x00" in the incoming
> migration data stream.
> 
> If we're 100% confident that there will never be a true
> command 0x00 then this approach is OK.

I am not confident, the industry can surprise us.

If a CFI command of value 0x00 is ever published, then this device will
be in troubles because it can not support it (due to back-migration).
Neither in its current state, neither after this patch.

So if this ever happens, this device will never be able to announce it
supports features with a such command. And if guests require we model
this feature, then we'll increase the migration version and the device
won't be backward-migratable.

I'll try to explain that in the commit description.