[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/9] hw/block/pflash_cfi01: Use the correct READ_ARRAY value |
Date: |
Tue, 9 Jul 2019 20:36:59 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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.
[Qemu-block] [PATCH v3 3/9] hw/block/pflash_cfi01: Extract pflash_mode_read_array(), Philippe Mathieu-Daudé, 2019/07/05
[Qemu-block] [PATCH v3 4/9] hw/block/pflash_cfi01: Start state machine as READY to accept commands, Philippe Mathieu-Daudé, 2019/07/05
[Qemu-block] [PATCH v3 5/9] hw/block/pflash_cfi01: Add the DeviceReset() handler, Philippe Mathieu-Daudé, 2019/07/05
[Qemu-block] [PATCH v3 6/9] hw/block/pflash_cfi01: Simplify CFI_QUERY processing, Philippe Mathieu-Daudé, 2019/07/05
[Qemu-block] [PATCH v3 7/9] hw/block/pflash_cfi01: Improve command comments, Philippe Mathieu-Daudé, 2019/07/05
[Qemu-block] [PATCH v3 8/9] hw/block/pflash_cfi01: Replace DPRINTF by qemu_log_mask(GUEST_ERROR), Philippe Mathieu-Daudé, 2019/07/05
[Qemu-block] [PATCH v3 9/9] hw/block/pflash_cfi01: Hold the PRI table offset in a variable, Philippe Mathieu-Daudé, 2019/07/05