Re: [PATCH v2 01/18] aspeed/smc: Fix write incorrect data into flash in user mode

["Cédric Le Goater" <clg@kaod.org>]

On 10/22/24 17:23, Kevin Wolf wrote:
> Am 22.10.2024 um 15:40 hat Cédric Le Goater geschrieben:
>>>>    static const VMStateDescription vmstate_aspeed_smc = {
>>>>        .name = "aspeed.smc",
>>>> -    .version_id = 2,
>>>> +    .version_id = 3,
>>>>        .minimum_version_id = 2,
>>>>        .fields = (const VMStateField[]) {
>>>>            VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX),
>>>>            VMSTATE_UINT8(snoop_index, AspeedSMCState),
>>>>            VMSTATE_UINT8(snoop_dummies, AspeedSMCState),
>>>> +        VMSTATE_BOOL(unselect, AspeedSMCState),
>>>>            VMSTATE_END_OF_LIST()
>>>>        }
>>>>    };
>>>
>>> I think this will break migration compatibility. In order to enable
>>> at least forward migration, it should be:
>>>
>>>       VMSTATE_BOOL_V(unselect, AspeedSMCState, 3),
>>
>> This is correct. I will fix the patch.
>>
>> Some background,
>>
>> The aspeed machines are fully emulated and the Aspeed SoC models are not
>> part of any virt* machines (yet). So migration support is a bit of a
>> theory. We have done our best to maintain some support, compatibility
>> not being a priority. IOW, it's not perfectly tuned as on virt machines.
>>
>> Also, on ARM, migration of the CPU secure mode (I think this is the reason,
>> Peter please correct me !) is not supported and if migration is initiated
>> after Linux has started, the machine will hang.
> 
> That's a good reason not to implement backwards migration for now, it
> would only complicate things. But as long as we claim to be migratable
> by having VMStateDescriptions and even increasing version_id, we should
> at least try to keep that part correct.

Sure. Thanks for pointing that out.

>> However, if one day, an aspeed model becomes part of a virt machine, we
>> should be more careful. I would start by resetting all vmstate versions
>> to 1!
> 
> Why would you reset it? Keeping 3 (or whatever it will be by then) as
> the first serious supported version shouldn't hurt and probably avoids
> some confusion.
  OK . We'll see when we get to that point. Who knows. The Aspeed machines
have gotten much bigger than we expected !

C.