Re: [PATCH 00/18] hw: Mark the device with no migratable fields

[Philippe Mathieu-Daudé]

Hi Laurent,

On 1/18/21 8:33 AM, Laurent Vivier wrote:
> Le 14/01/2021 à 16:49, Philippe Mathieu-Daudé a écrit :
>> On 7/9/20 9:19 PM, Peter Maydell wrote:
>>> On Fri, 3 Jul 2020 at 21:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> This is a proof-of-concept after chatting with Peter Maydell
>>>> on IRC earlier.
>>>>
>>>> Introduce the vmstate_no_state_to_migrate structure, and
>>>> a reference to it: vmstate_qdev_no_state_to_migrate.
>>>> Use this reference in devices with no fields to migrate.
>>>>
>>>> This is useful to catch devices missing vmstate, such:
>>>> - ads7846
>>>> - mcf-uart
>>>> - mcf-fec
>>>> - versatile_i2c
>>>> - ...
>>>>
>>>> I am not sure about:
>>>> - gpex-pcihost
>>>
>>> I think it's correct that this has no internal state:
>>> the only interesting state is in the GPEXRootState, which
>>> is a TYPE_GPEX_ROOT_DEVICE which migrates itself.
>>>
>>> I made some comments on the "meaty" bits of the patchset,
>>> and reviewed one or two of the "mark this device as
>>> having no migration state" patches, but it doesn't seem
>>> worth reviewing all of them until the migration submaintainers
>>> have a chance to weigh in on whether they like the concept
>>> (I expect they're busy right now with freeze-related stuff :-))
>>
>> Now that we are far from freeze-date is a good time to ping
>> again on this concept :)
>>
>> Most of the devices are ARM except:
>> - cpu-cluster (Eduardo/Marcel)
>> - hcd-ohci (Gerd)
>> - mac-nubus-bridge (Laurent)
>> - generic QOM (Daniel, Paolo)
>>
>> Is someone against this proposal?
> 
> I'm not against the proposal, but I don't understand why we need this.

IIRC the IRC discussion followed this thread:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg554453.html

Quoting Peter:

> I think we should care about migration on all architectures
> and devices, in the sense that we want savevm/loadvm to work.
> This is a really useful debugging and user tool, and when
> I'm reviewing devices it's the minimum bar I think new
> devices should clear. You then get migration "for free" but
> I don't particularly expect it to be used compared to
> snapshot save/restore. (Of course some of our existing code
> doesn't support this, and we don't have a good way of testing
> so bugs creep in easily, but as a principle I think it's
> good.)

Currently there is no automatic way to catch missing vmstate,
we rely on code review (mostly from Peter...).

To be able to add a code check to catch the future device added,
we need to first clean the (old) devices missing VMState, justifying
why each doesn't have any field to migrate.

Also IMO it is simpler to have an unified API, rather than explaining
each experienced and new contributor why "old style qdev" are allowed
to do things than "new introduced qdev" can't do anymore.

Regards,

Phil.