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

Philippe Mathieu-Daudé posted 18 patches 3 years, 8 months ago
Test FreeBSD passed
Test docker-quick@centos7 failed
Test checkpatch passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200703201911.26573-1-f4bug@amsat.org
There is a newer version of this series
configure                    |  9 +++++++++
hw/usb/hcd-ohci.h            |  2 ++
include/hw/arm/bcm2836.h     |  7 ++++---
include/hw/arm/msf2-soc.h    | 11 ++++++-----
include/hw/cpu/a9mpcore.h    |  3 ++-
include/hw/qdev-core.h       |  2 ++
include/migration/vmstate.h  |  3 ++-
hw/arm/armv7m.c              |  1 +
hw/arm/aspeed_soc.c          |  1 +
hw/arm/bcm2835_peripherals.c |  1 +
hw/arm/bcm2836.c             |  1 +
hw/arm/msf2-soc.c            |  1 +
hw/core/qdev.c               |  8 ++++++++
hw/core/split-irq.c          |  1 +
hw/cpu/a9mpcore.c            |  1 +
hw/cpu/cluster.c             |  1 +
hw/intc/arm_gicv2m.c         |  2 ++
hw/misc/armsse-cpuid.c       |  1 +
hw/misc/iotkit-sysinfo.c     |  1 +
hw/misc/unimp.c              |  1 +
hw/nubus/mac-nubus-bridge.c  |  1 +
hw/sparc64/sun4u.c           |  7 ++++++-
hw/usb/hcd-ohci.c            |  1 +
migration/vmstate.c          |  7 +++++++
24 files changed, 63 insertions(+), 11 deletions(-)
[PATCH 00/18] hw: Mark the device with no migratable fields
Posted by Philippe Mathieu-Daudé 3 years, 8 months ago
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

Philippe Mathieu-Daudé (18):
  migration/vmstate: Document vmstate_dummy
  migration/vmstate: Introduce vmstate_no_state_to_migrate
  hw/core/qdev: Add vmstate_qdev_no_state_to_migrate
  hw/arm/armv7m: Mark the device with no migratable fields
  hw/arm/aspeed_soc: Mark the device with no migratable fields
  hw/arm/bcm283x: Mark devices with no migratable fields
  hw/arm/msf2-soc: Mark the device with no migratable fields
  hw/core/split-irq: Mark the device with no migratable fields
  hw/cpu/a9mpcore: Mark the device with no migratable fields
  hw/cpu/cluster: Mark the device with no migratable fields
  hw/usb/hcd-ohci: Mark the device with no migratable fields
  hw/intc/arm_gicv2m: Mark the device with no migratable fields
  hw/misc/armsse-cpuid: Mark the device with no migratable fields
  hw/misc/iotkit-sysinfo: Mark the device with no migratable fields
  hw/misc/unimp: Mark the device with no migratable fields
  hw/nubus/mac-nubus-bridge: Mark the device with no migratable fields
  hw/sparc64/sun4u: Mark devices with no migratable fields
  hw/core/qdev: Display warning for devices missing migration state

 configure                    |  9 +++++++++
 hw/usb/hcd-ohci.h            |  2 ++
 include/hw/arm/bcm2836.h     |  7 ++++---
 include/hw/arm/msf2-soc.h    | 11 ++++++-----
 include/hw/cpu/a9mpcore.h    |  3 ++-
 include/hw/qdev-core.h       |  2 ++
 include/migration/vmstate.h  |  3 ++-
 hw/arm/armv7m.c              |  1 +
 hw/arm/aspeed_soc.c          |  1 +
 hw/arm/bcm2835_peripherals.c |  1 +
 hw/arm/bcm2836.c             |  1 +
 hw/arm/msf2-soc.c            |  1 +
 hw/core/qdev.c               |  8 ++++++++
 hw/core/split-irq.c          |  1 +
 hw/cpu/a9mpcore.c            |  1 +
 hw/cpu/cluster.c             |  1 +
 hw/intc/arm_gicv2m.c         |  2 ++
 hw/misc/armsse-cpuid.c       |  1 +
 hw/misc/iotkit-sysinfo.c     |  1 +
 hw/misc/unimp.c              |  1 +
 hw/nubus/mac-nubus-bridge.c  |  1 +
 hw/sparc64/sun4u.c           |  7 ++++++-
 hw/usb/hcd-ohci.c            |  1 +
 migration/vmstate.c          |  7 +++++++
 24 files changed, 63 insertions(+), 11 deletions(-)

-- 
2.21.3


Re: [PATCH 00/18] hw: Mark the device with no migratable fields
Posted by no-reply@patchew.org 3 years, 8 months ago
Patchew URL: https://patchew.org/QEMU/20200703201911.26573-1-f4bug@amsat.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  LINK    tests/test-io-task
  LINK    tests/test-io-channel-socket
hw/core/qdev.o:(.data.rel+0x0): undefined reference to `vmstate_no_state_to_migrate'
collect2: error: ld returned 1 exit status
make: *** [tests/test-qdev-global-props] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 002
  TEST    iotest-qcow2: 003
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=ba983f0b8bc94d39a0a649a449a91f08', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-mg_r93vd/src/docker-src.2020-07-03-16.46.29.22759:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=ba983f0b8bc94d39a0a649a449a91f08
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-mg_r93vd/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    16m8.994s
user    0m8.902s


The full log is available at
http://patchew.org/logs/20200703201911.26573-1-f4bug@amsat.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH 00/18] hw: Mark the device with no migratable fields
Posted by Peter Maydell 3 years, 8 months ago
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 :-))

thanks
-- PMM

Re: [PATCH 00/18] hw: Mark the device with no migratable fields
Posted by Philippe Mathieu-Daudé 3 years, 2 months ago
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?

Regards,

Phil.

Re: [PATCH 00/18] hw: Mark the device with no migratable fields
Posted by Laurent Vivier 3 years, 2 months ago
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.

Thanks,
Laurent

Re: [PATCH 00/18] hw: Mark the device with no migratable fields
Posted by Philippe Mathieu-Daudé 3 years, 2 months ago
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.