On Mon, 16 Mar 2026 at 14:22, Fabiano Rosas <[email protected]> wrote:
>
> Roman Kiryanov <[email protected]> writes:
>
> > The vmstate_save_state_v() function does not support
> > NULL in VMStateDescription::fields and will crash if
> > one is provided.
> >
> > This change prevents this situation from happening
> > by explicitly crashing earlier.
> >
> > Suggested-by: Fabiano Rosas <[email protected]>
> > Reviewed-by: Peter Xu <[email protected]>
> > Signed-off-by: Roman Kiryanov <[email protected]>
> > ---
> > v3:
> > - Also added assert to virtio.c to validate
> > VirtioDeviceClass instances which bypass
> > vmstate_register_with_alias_id.
> > - Updated the commit message to "Reviewed-by".
> > v2:
> > - Moved the assert from vmstate_save_state_v to the registration
> > path (vmstate_register_with_alias_id) and the qtest validation path
> > (vmstate_check) to catch missing fields earlier.
> > ---
> > hw/virtio/virtio.c | 6 ++++++
> > migration/savevm.c | 5 +++++
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 0ba734d0bc..e4543de335 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -4054,6 +4054,12 @@ static void virtio_device_realize(DeviceState *dev,
> > Error **errp)
> > /* Devices should either use vmsd or the load/save methods */
> > assert(!vdc->vmsd || !vdc->load);
> >
> > + /*
> > + * If a device has vmsd, it either MUST have valid fields
> > + * or marked unmigratable.
> > + */
> > + assert(!vdc->vmsd || (vdc->vmsd->unmigratable || vdc->vmsd->fields));
> > +
> > if (vdc->realize != NULL) {
> > vdc->realize(dev, &err);
> > if (err != NULL) {
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 197c89e0e6..20c2b99231 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -861,6 +861,8 @@ static void vmstate_check(const VMStateDescription
> > *vmsd)
> > const VMStateField *field = vmsd->fields;
> > const VMStateDescription * const *subsection = vmsd->subsections;
> >
> > + assert(field || vmsd->unmigratable);
> > +
> > if (field) {
> > while (field->name) {
> > if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> > @@ -900,6 +902,9 @@ int vmstate_register_with_alias_id(VMStateIf *obj,
> > uint32_t instance_id,
> > /* If this triggers, alias support can be dropped for the vmsd. */
> > assert(alias_id == -1 || required_for_version >=
> > vmsd->minimum_version_id);
> >
> > + /* If vmsd is migratable it MUST have valid fields. */
> > + assert(vmsd->fields || vmsd->unmigratable);
> > +
> > se = g_new0(SaveStateEntry, 1);
> > se->version_id = vmsd->version_id;
> > se->section_id = savevm_state.global_section_id++;
>
> Looks like we'll also need to fix some stubs that define empty vmsds:
>
> qemu-system-mips64: ../migration/savevm.c:864: void vmstate_check(const
> VMStateDescription *): Assertion `field || vmsd->unmigratable' failed.
>
> Maybe something like the following. Peter, what do you think?
Ah, I was about to report this -- commit 7aa563630b6b0 ("pc: Start
with modern CPU hotplug interface by default") broke the MIPS Malta
board migration, which now segfaults when you "savevm" because
the stub vmstate_cpu_hotplug struct has a NULL fields pointer.
This used to work because the relevant vmstate had a "needed"
function that meant we never used the stub vmstate structs, but
that commit dropped the "needed" functions.
If the change you suggest is the right way to fix this then it
ought to be its own patch with a Fixes: tag pointing at 7aa563630b6b0.
thanks
-- PMM
