Re: [Qemu-devel] [PATCH v2 0/9] hw/block/pflash_cfi01: Add DeviceReset() handler

[Philippe Mathieu-Daudé]

On 7/2/19 1:52 PM, Laszlo Ersek wrote:
> Hi Phil,
> 
> On 07/02/19 02:12, Philippe Mathieu-Daudé wrote:
>> The pflash device lacks a reset() function.
>> When a machine is resetted, the flash might be in an
>> inconsistent state, leading to unexpected behavior:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>> Resolve this issue by adding a DeviceReset() handler.
>>
>> Fix also two minor issues, and clean a bit the codebase.
>>
>> Since v1: https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg00962.html
>> - addressed Laszlo review comments
>>
>> Maintainers spam list from:
>> ./scripts/get_maintainer.pl -f $(git grep -El 
>> '(pflash_cfi01_register|TYPE_PFLASH_CFI01)')
>>
>> Regards,
>>
>> Phil.
>>
>> Philippe Mathieu-Daudé (9):
>>   hw/block/pflash_cfi01: Removed an unused timer
>>   hw/block/pflash_cfi01: Use the correct READ_ARRAY value
>>   hw/block/pflash_cfi01: Extract pflash_mode_read_array()
>>   hw/block/pflash_cfi01: Start state machine as READY to accept commands
>>   hw/block/pflash_cfi01: Add the DeviceReset() handler
>>   hw/block/pflash_cfi01: Simplify CFI_QUERY processing
>>   hw/block/pflash_cfi01: Improve command comments
>>   hw/block/pflash_cfi01: Replace DPRINTF by qemu_log_mask(GUEST_ERROR)
>>   hw/block/pflash_cfi01: Hold the PRI table offset in a variable
>>
>>  hw/block/pflash_cfi01.c | 140 +++++++++++++++++++++-------------------
>>  hw/block/trace-events   |   1 +
>>  2 files changed, 74 insertions(+), 67 deletions(-)
>>
> 
> I'll do some regression-tests with this, using OVMF and ArmVirtQemu.

My local tree is larger with many tracing improvments, but I stripped
that part to keep this series short and bugfix oriented.

I used the trace logs to analyse the guest accesses.

My test setup is not yet automatized but I'm working on passing all the
one working with TCG as Avocado tests.

Tests machines (firmwares):

- ARM Verdex with (U-Boot)
- MIPS Malta with (Linux)
- LM32 Milkymist One (RTEMS)
- X86 (EDK2 OVMF)

Each guest has different driver, thus use this device quite differently.

What I want to add to this list is:

- AARCH64 Virt (EDK2 ArmVirtQemu)

Because it uses another driver (not the OVMF one).

> I don't think I can usefully review the patches without getting lost in
> the related spec(s), and I don't have capacity for that.
> 
> Until I have regression test results, one question: are the changes to
> the device model transparent with regard to migration? (You are not
> introducing any compat properties.)

Good point.

Two fields updated are migrated:

static const VMStateDescription vmstate_pflash = {
    .name = "pflash_cfi01",
    .version_id = 1,
    .minimum_version_id = 1,
    .post_load = pflash_post_load,
    .fields = (VMStateField[]) {
        ...
        VMSTATE_UINT8(cmd, PFlashCFI01),
        VMSTATE_UINT8(status, PFlashCFI01),
        ...
        VMSTATE_END_OF_LIST()
    }
};

- status: should not break anything

- cmd:

  If migrated during cmd == READ_ARRAY state:

  Guest is saved with older QEMU having cmd=READ_ARRAY=0x00

  Guest restored to newer QEMU expecting READ_ARRAY=0xff

  - If the guest do a READ access, we are [currently] fine:
  the 0x00 command hits the default case and falls through
  0xff.

    ('currently' because I have a local patch that would
     clean up this impossible case, but I made it possible).

  - If guest does WRITE access, again the 0x00 is processed
  by the default case where we jump to the error_flash label.
  There we log a warning and call pflash_mode_read_array().

  So this is not clean, but safe.

  To be clean I should document 0x00 was previously used and
  even not matching the specs, we have to live with handling
  it forever.



  A cleaner way to keep this safe and the code simple is to
  increment vmstate_pflash.version_id, and add a new case in
  the post_load handler:

static int pflash_post_load(void *opaque, int version_id)
{
    PFlashCFI01 *pfl = opaque;

    ... // current handler code

    if (version_id < 2) {
        /* version_id used 0x00 for READ_ARRAY */
        if (s->cmd == 0x00) {
            s->cmd =  0xff;
        }
    }
    return 0;
}

Since I'm new with migration, I Cc'd Dave to check :)

Thanks!

Phil.