[PATCH v1] migration/savevm: Allow stub VMSDs

[PATCH v1] migration/savevm: Allow stub VMSDs

[Fabiano Rosas]

After the referenced commit, empty VMStateDescription objects declared
as part of stubs have started reaching the vmstate code.

A valid VMStateDescription must at minimum have a name and either the
.fields or .unmigratable fields set. Stubs, being empty, have
none. Code that assumes a non-NULL name field will now cause a
crash. E.g.

$ ./build/mips/qemu-system-mipsel -nographic -drive if=none,format=qcow2,file=dummy.qcow2
    [Type "C-a c" to get the "(qemu)" monitor prompt)]
    (qemu) savevm foo

    Backtrace from doing this under gdb:

      #0  0x0000555555df7d4d in vmsd_can_compress (field=0x5555564f78a0
      <__compound_literal.3>) at ../../migration/vmstate.c:339
      #1  0x0000555555df7dbb in vmsd_desc_field_start
          (vmsd=0x555556431ba0 <vmstate_cpuhp_state>, vmdesc=0x555556918690,
      field=0x5555564f78a0 <__compound_literal.3>, i=0, max=1) at
      ../../migration/vmstate.c:362
      #2  0x0000555555df85a7 in vmstate_save_state_v
          (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
      opaque=0x555556c9aac0, vmdesc=0x555556918690, version_id=1,
      errp=0x7fffffffc948) at ../../migration/vmstate.c:528
      #3  0x0000555555df8032 in vmstate_save_state
          (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
      opaque=0x555556c9aac0, vmdesc_id=0x555556918690, errp=0x7fffffffc948)
      at ../../migration/vmstate.c:427
      #4  0x0000555555df8f83 in vmstate_subsection_save
          (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
      opaque=0x555556c9aac0, vmdesc=0x555556918690, errp=0x7fffffffc948)
          at ../../migration/vmstate.c:695

Due to their very nature, it's better to allow stubs to be
completely empty instead of forcing any rules. Teach the code to skip
them.

Fixes: 7aa563630b ("pc: Start with modern CPU hotplug interface by default")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/savevm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index dd58f2a705..e8d3360877 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -861,6 +861,10 @@ static void vmstate_check(const VMStateDescription *vmsd)
     const VMStateField *field = vmsd->fields;
     const VMStateDescription * const *subsection = vmsd->subsections;
 
+    if (!vmsd->name) {
+        return;
+    }
+
     if (field) {
         while (field->name) {
             if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
@@ -897,6 +901,11 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
 {
     SaveStateEntry *se;
 
+    if (!vmsd->name) {
+        /* assume it's a stub and ignore */
+        return 0;
+    }
+
     /* If this triggers, alias support can be dropped for the vmsd. */
     assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
 
-- 
2.51.0

Re: [PATCH v1] migration/savevm: Allow stub VMSDs

[Peter Maydell]

On Mon, 16 Mar 2026 at 19:48, Fabiano Rosas <farosas@suse.de> wrote:
>
> After the referenced commit, empty VMStateDescription objects declared
> as part of stubs have started reaching the vmstate code.
>
> A valid VMStateDescription must at minimum have a name and either the
> .fields or .unmigratable fields set. Stubs, being empty, have
> none. Code that assumes a non-NULL name field will now cause a
> crash. E.g.
>
> $ ./build/mips/qemu-system-mipsel -nographic -drive if=none,format=qcow2,file=dummy.qcow2
>     [Type "C-a c" to get the "(qemu)" monitor prompt)]
>     (qemu) savevm foo
>
>     Backtrace from doing this under gdb:
>
>       #0  0x0000555555df7d4d in vmsd_can_compress (field=0x5555564f78a0
>       <__compound_literal.3>) at ../../migration/vmstate.c:339
>       #1  0x0000555555df7dbb in vmsd_desc_field_start
>           (vmsd=0x555556431ba0 <vmstate_cpuhp_state>, vmdesc=0x555556918690,
>       field=0x5555564f78a0 <__compound_literal.3>, i=0, max=1) at
>       ../../migration/vmstate.c:362
>       #2  0x0000555555df85a7 in vmstate_save_state_v
>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>       opaque=0x555556c9aac0, vmdesc=0x555556918690, version_id=1,
>       errp=0x7fffffffc948) at ../../migration/vmstate.c:528
>       #3  0x0000555555df8032 in vmstate_save_state
>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>       opaque=0x555556c9aac0, vmdesc_id=0x555556918690, errp=0x7fffffffc948)
>       at ../../migration/vmstate.c:427
>       #4  0x0000555555df8f83 in vmstate_subsection_save
>           (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
>       opaque=0x555556c9aac0, vmdesc=0x555556918690, errp=0x7fffffffc948)
>           at ../../migration/vmstate.c:695
>
> Due to their very nature, it's better to allow stubs to be
> completely empty instead of forcing any rules. Teach the code to skip
> them.

I'm not sure about this. I think if we're hitting a stub
then something has gone wrong -- presumably the code author
intended something to get migrated but it has not been.
I think that any stub should ideally never be registered,
or failing that at least should never get used (i.e. it
should be guarded by a "needed" function that returns false).
Allowing stubs to silently "work" seems to me like it's
going to be an easy way for bugs to get in where some
builds of QEMU happen to pull in the stub but we never notice
it because the "all targets" build happens to compile the
non-stub version of the code (in which case either we
fail to migrate data we should, or the migration data on
the wire has different formats depending on the build config
of the QEMU binary, which is going to be a pain to debug).

For commit 7aa563630b I feel like the problem is "author of
the code didn't realise that there was the MIPS malta case
where this code can be used but CONFIG_ACPI_CPU_HOTPLUG
is not selected", and the right answer is "look at that
case and update the piix4 code so it's handled right".

thanks
-- PMM

Re: [PATCH v1] migration/savevm: Allow stub VMSDs

[Peter Xu]

On Mon, Mar 16, 2026 at 04:47:59PM -0300, Fabiano Rosas wrote:
> After the referenced commit, empty VMStateDescription objects declared
> as part of stubs have started reaching the vmstate code.
> 
> A valid VMStateDescription must at minimum have a name and either the
> .fields or .unmigratable fields set. Stubs, being empty, have
> none. Code that assumes a non-NULL name field will now cause a
> crash. E.g.
> 
> $ ./build/mips/qemu-system-mipsel -nographic -drive if=none,format=qcow2,file=dummy.qcow2
>     [Type "C-a c" to get the "(qemu)" monitor prompt)]
>     (qemu) savevm foo
> 
>     Backtrace from doing this under gdb:
> 
>       #0  0x0000555555df7d4d in vmsd_can_compress (field=0x5555564f78a0
>       <__compound_literal.3>) at ../../migration/vmstate.c:339
>       #1  0x0000555555df7dbb in vmsd_desc_field_start
>           (vmsd=0x555556431ba0 <vmstate_cpuhp_state>, vmdesc=0x555556918690,
>       field=0x5555564f78a0 <__compound_literal.3>, i=0, max=1) at
>       ../../migration/vmstate.c:362
>       #2  0x0000555555df85a7 in vmstate_save_state_v
>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>       opaque=0x555556c9aac0, vmdesc=0x555556918690, version_id=1,
>       errp=0x7fffffffc948) at ../../migration/vmstate.c:528
>       #3  0x0000555555df8032 in vmstate_save_state
>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>       opaque=0x555556c9aac0, vmdesc_id=0x555556918690, errp=0x7fffffffc948)
>       at ../../migration/vmstate.c:427
>       #4  0x0000555555df8f83 in vmstate_subsection_save
>           (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
>       opaque=0x555556c9aac0, vmdesc=0x555556918690, errp=0x7fffffffc948)
>           at ../../migration/vmstate.c:695
> 
> Due to their very nature, it's better to allow stubs to be
> completely empty instead of forcing any rules. Teach the code to skip
> them.
> 
> Fixes: 7aa563630b ("pc: Start with modern CPU hotplug interface by default")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/savevm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index dd58f2a705..e8d3360877 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -861,6 +861,10 @@ static void vmstate_check(const VMStateDescription *vmsd)
>      const VMStateField *field = vmsd->fields;
>      const VMStateDescription * const *subsection = vmsd->subsections;
>  
> +    if (!vmsd->name) {
> +        return;
> +    }

IIUC this is fine but isn't needed for stubs' use case, because "field" is
checked right below..

> +
>      if (field) {

.. here.

>          while (field->name) {
>              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> @@ -897,6 +901,11 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>  {
>      SaveStateEntry *se;
>  
> +    if (!vmsd->name) {
> +        /* assume it's a stub and ignore */
> +        return 0;
> +    }

Two questions on this one..

(1) when someone adds some new VMSD to device and forgot to attach a name,
    it'll silently ignore that new VMSD.. of course anyone testing this
    will find something strange, but silently ignoring an VMSD when not
    having name does sound like unexpected..

(2) would this fix the above crash?  The problem is the commit got fixed
    used VMSTATE_CPU_HOTPLUG which is an internal vmsd, this one only
    checks the top VMSD (which should have a name here..).  So I'm not sure
    if it'll work..

Is that MIPS board the only one that is broken?  Do we want to make sure
nothing is missed? We can definitely decide to break the migration, but we
can also choose to just revert the needed() back to normal.  It just that
it looks like we don't strongly need to break it.. as I don't see what we
get from breaking it yet.. (say, it's not like we can get rid of some weird
API or legacy function, we need to support needed() anyway..)

> +
>      /* If this triggers, alias support can be dropped for the vmsd. */
>      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
>  
> -- 
> 2.51.0
> 

-- 
Peter Xu

Re: [PATCH v1] migration/savevm: Allow stub VMSDs

[Peter Maydell]

On Mon, 16 Mar 2026 at 21:13, Peter Xu <peterx@redhat.com> wrote:
> Is that MIPS board the only one that is broken?

I believe that the MIPS malta and the i440fx PC machines are
the only ones that do "select PIIX" in their Kconfig, so
on the assumption that x86 was tested, the malta is likely
the only problem here.

(For the PC machines, I440FX, which is what does "select PIIX",
also does "select PC_ACPI", which does "select  ACPI_CPU_HOTPLUG".
But MIPS doesn't use ACPI, so no ACPI_CPU_HOTPLUG.)

-- PMM

Re: [PATCH v1] migration/savevm: Allow stub VMSDs

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Mar 16, 2026 at 04:47:59PM -0300, Fabiano Rosas wrote:
>> After the referenced commit, empty VMStateDescription objects declared
>> as part of stubs have started reaching the vmstate code.
>> 
>> A valid VMStateDescription must at minimum have a name and either the
>> .fields or .unmigratable fields set. Stubs, being empty, have
>> none. Code that assumes a non-NULL name field will now cause a
>> crash. E.g.
>> 
>> $ ./build/mips/qemu-system-mipsel -nographic -drive if=none,format=qcow2,file=dummy.qcow2
>>     [Type "C-a c" to get the "(qemu)" monitor prompt)]
>>     (qemu) savevm foo
>> 
>>     Backtrace from doing this under gdb:
>> 
>>       #0  0x0000555555df7d4d in vmsd_can_compress (field=0x5555564f78a0
>>       <__compound_literal.3>) at ../../migration/vmstate.c:339
>>       #1  0x0000555555df7dbb in vmsd_desc_field_start
>>           (vmsd=0x555556431ba0 <vmstate_cpuhp_state>, vmdesc=0x555556918690,
>>       field=0x5555564f78a0 <__compound_literal.3>, i=0, max=1) at
>>       ../../migration/vmstate.c:362
>>       #2  0x0000555555df85a7 in vmstate_save_state_v
>>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>>       opaque=0x555556c9aac0, vmdesc=0x555556918690, version_id=1,
>>       errp=0x7fffffffc948) at ../../migration/vmstate.c:528
>>       #3  0x0000555555df8032 in vmstate_save_state
>>           (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
>>       opaque=0x555556c9aac0, vmdesc_id=0x555556918690, errp=0x7fffffffc948)
>>       at ../../migration/vmstate.c:427
>>       #4  0x0000555555df8f83 in vmstate_subsection_save
>>           (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
>>       opaque=0x555556c9aac0, vmdesc=0x555556918690, errp=0x7fffffffc948)
>>           at ../../migration/vmstate.c:695
>> 
>> Due to their very nature, it's better to allow stubs to be
>> completely empty instead of forcing any rules. Teach the code to skip
>> them.
>> 
>> Fixes: 7aa563630b ("pc: Start with modern CPU hotplug interface by default")
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/savevm.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index dd58f2a705..e8d3360877 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -861,6 +861,10 @@ static void vmstate_check(const VMStateDescription *vmsd)
>>      const VMStateField *field = vmsd->fields;
>>      const VMStateDescription * const *subsection = vmsd->subsections;
>>  
>> +    if (!vmsd->name) {
>> +        return;
>> +    }
>
> IIUC this is fine but isn't needed for stubs' use case, because "field" is
> checked right below..
>
>> +
>>      if (field) {
>
> .. here.
>
>>          while (field->name) {
>>              if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
>> @@ -897,6 +901,11 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>>  {
>>      SaveStateEntry *se;
>>  
>> +    if (!vmsd->name) {
>> +        /* assume it's a stub and ignore */
>> +        return 0;
>> +    }
>
> Two questions on this one..
>
> (1) when someone adds some new VMSD to device and forgot to attach a name,
>     it'll silently ignore that new VMSD.. of course anyone testing this
>     will find something strange, but silently ignoring an VMSD when not
>     having name does sound like unexpected..
>
> (2) would this fix the above crash?  The problem is the commit got fixed
>     used VMSTATE_CPU_HOTPLUG which is an internal vmsd, this one only
>     checks the top VMSD (which should have a name here..).  So I'm not sure
>     if it'll work..
>

Ouch, you're right. I checked the CI, but the issue caught by the CI was
the other one. My brain is fried, I'll get back to you guys
tomorrow. Sorry for the noise.

> Is that MIPS board the only one that is broken?  Do we want to make sure
> nothing is missed? We can definitely decide to break the migration, but we
> can also choose to just revert the needed() back to normal.  It just that
> it looks like we don't strongly need to break it.. as I don't see what we
> get from breaking it yet.. (say, it's not like we can get rid of some weird
> API or legacy function, we need to support needed() anyway..)
>
>> +
>>      /* If this triggers, alias support can be dropped for the vmsd. */
>>      assert(alias_id == -1 || required_for_version >= vmsd->minimum_version_id);
>>  
>> -- 
>> 2.51.0
>>