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

[Philippe Mathieu-Daudé]

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

Hmm OK...

> 
> 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?

Well, I think this change is simpler than it looks.

Currently the code is (what will run on older guest):

static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
                            int width, int be)
{
    switch (pfl->cmd) {
    default:
        /* This should never happen : reset state & treat it as a read*/
        DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd);
        pfl->wcycle = 0;
        pfl->cmd = 0;
        /* fall through to read code */
    case 0x00:
        /* Flash area read */
        ret = pflash_data_read(pfl, offset, width, be);
        break;

and:

static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                         uint32_t value, int width, int be)
{
    switch (pfl->wcycle) {
    case 0:
        /* read mode */
        switch (cmd) {
        case 0x00: /* ??? */
            goto reset_flash;
        case 0xff: /* Read array mode */
            DPRINTF("%s: Read array mode\n", __func__);
            goto reset_flash;
        default:
            goto error_flash;
        }

So current (incorrect) 0x00 will be then 0xff, and will be backprocessed
correctly.

What I want is to get ride of this 0x00 processing that is confusing,
the spec and the guests use 0xff for READ_ARRAY.

So if I increase version I can not back-migrate, but luckily it seems I
can simply update 0x00 -> 0xff without even increasing the version :)

(I'm reluctant to skip this patch because I'd rather avoid Laszlo to
re-run his tests).

Regards,

Phil.