[PATCH v2 0/3] vmsd checks

[PATCH v2 0/3] vmsd checks

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Aftern the patch the other day where I added a missing END_OF_LIST,
Peter suggested adding something more robust.

Here I:
  add a check for a flag at the end of the list
  add a check that's performed in vmstate_register_with_alias_id
    only within qtest recursively for that canary and for
    subsection naming constraints.
  Fix a ppc issue that the vmstate naming constraint caught
    (Waiting for a reply from the PPC folk to check that).

The checks can't go in until I get the def into libslirp.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Dr. David Alan Gilbert (3):
  ppc: Fix vmstate_pbr403 subsection name
  migration: Add canary to VMSTATE_END_OF_LIST
  migration: Perform vmsd structure check during tests

 include/migration/vmstate.h |  7 ++++++-
 migration/savevm.c          | 40 +++++++++++++++++++++++++++++++++++++
 migration/vmstate.c         |  2 ++
 target/ppc/machine.c        |  2 +-
 4 files changed, 49 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] ppc: Fix vmstate_pbr403 subsection name

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The pbr403 subsection is part of the tlb6xx state, so I believe it's
name needs to be:

    .name = "cpu/tlb6xx/pbr403",

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 target/ppc/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 756d8de5d8..e535edb7c4 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -718,7 +718,7 @@ static bool pbr403_needed(void *opaque)
 }
 
 static const VMStateDescription vmstate_pbr403 = {
-    .name = "cpu/pbr403",
+    .name = "cpu/tlb6xx/pbr403",
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = pbr403_needed,
-- 
2.34.1

[PATCH v2 2/3] migration: Add canary to VMSTATE_END_OF_LIST

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

We fairly regularly forget VMSTATE_END_OF_LIST markers off descriptions;
given that the current check is only for ->name being NULL, sometimes
we get unlucky and the code apparently works and no one spots the error.

Explicitly add a flag, VMS_END that should be set, and assert it is
set during the traversal.

Note: This can't go in until we update the copy of vmstate.h in slirp.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/vmstate.h | 7 ++++++-
 migration/savevm.c          | 1 +
 migration/vmstate.c         | 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 017c03675c..b50708e57a 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -147,6 +147,9 @@ enum VMStateFlags {
      * VMStateField.struct_version_id to tell which version of the
      * structure we are referencing to use. */
     VMS_VSTRUCT           = 0x8000,
+
+    /* Marker for end of list */
+    VMS_END = 0x10000
 };
 
 typedef enum {
@@ -1163,7 +1166,9 @@ extern const VMStateInfo vmstate_info_qlist;
     VMSTATE_UNUSED_BUFFER(_test, 0, _size)
 
 #define VMSTATE_END_OF_LIST()                                         \
-    {}
+    {                     \
+        .flags = VMS_END, \
+    }
 
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id);
diff --git a/migration/savevm.c b/migration/savevm.c
index 0bef031acb..8077393d11 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -620,6 +620,7 @@ static void dump_vmstate_vmsd(FILE *out_file,
             field++;
             first = false;
         }
+        assert(field->flags == VMS_END);
         fprintf(out_file, "\n%*s]", indent, "");
     }
     if (vmsd->subsections != NULL) {
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 05f87cdddc..181ba08c7d 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -160,6 +160,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         }
         field++;
     }
+    assert(field->flags == VMS_END);
     ret = vmstate_subsection_load(f, vmsd, opaque);
     if (ret != 0) {
         return ret;
@@ -413,6 +414,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         }
         field++;
     }
+    assert(field->flags == VMS_END);
 
     if (vmdesc) {
         json_writer_end_array(vmdesc);
-- 
2.34.1

[PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Perform a check on vmsd structures during test runs in the hope
of catching any missing terminators and other simple screwups.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/savevm.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 8077393d11..97a4471220 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -66,6 +66,7 @@
 #include "net/announce.h"
 #include "qemu/yank.h"
 #include "yank_functions.h"
+#include "sysemu/qtest.h"
 
 const unsigned int postcopy_ram_discard_version;
 
@@ -839,6 +840,39 @@ void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque)
     }
 }
 
+/*
+ * Perform some basic checks on vmsd's at registration
+ * time.
+ */
+static void vmstate_check(const VMStateDescription *vmsd)
+{
+    const VMStateField *field = vmsd->fields;
+    const VMStateDescription **subsection = vmsd->subsections;
+
+    if (field) {
+        while (field->name) {
+            if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
+                /* Recurse to sub structures */
+                vmstate_check(field->vmsd);
+            }
+            /* Carry on */
+            field++;
+        }
+        /* Check for the end of field list canary */
+        assert(field->flags == VMS_END);
+    }
+
+    while (subsection && *subsection) {
+        /*
+         * The name of a subsection should start with the name of the
+         * current object.
+         */
+        assert(!strncmp(vmsd->name, (*subsection)->name, strlen(vmsd->name)));
+        vmstate_check(*subsection);
+        subsection++;
+    }
+}
+
 int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
@@ -884,6 +918,11 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
     } else {
         se->instance_id = instance_id;
     }
+
+    /* Perform a recursive sanity check during the test runs */
+    if (qtest_enabled()) {
+        vmstate_check(vmsd);
+    }
     assert(!se->compat || se->instance_id == 0);
     savevm_state_handler_insert(se);
     return 0;
-- 
2.34.1

Re: [PATCH v2 2/3] migration: Add canary to VMSTATE_END_OF_LIST

[Peter Maydell]

On Thu, 13 Jan 2022 at 19:45, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
>
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> We fairly regularly forget VMSTATE_END_OF_LIST markers off descriptions;
> given that the current check is only for ->name being NULL, sometimes
> we get unlucky and the code apparently works and no one spots the error.
>
> Explicitly add a flag, VMS_END that should be set, and assert it is
> set during the traversal.
>
> Note: This can't go in until we update the copy of vmstate.h in slirp.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Peter Maydell]

On Thu, 13 Jan 2022 at 19:45, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
>
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Perform a check on vmsd structures during test runs in the hope
> of catching any missing terminators and other simple screwups.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/savevm.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 1/3] ppc: Fix vmstate_pbr403 subsection name

[Peter Maydell]

On Thu, 13 Jan 2022 at 19:45, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
>
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The pbr403 subsection is part of the tlb6xx state, so I believe it's
> name needs to be:
>
>     .name = "cpu/tlb6xx/pbr403",
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  target/ppc/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 756d8de5d8..e535edb7c4 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -718,7 +718,7 @@ static bool pbr403_needed(void *opaque)
>  }
>
>  static const VMStateDescription vmstate_pbr403 = {
> -    .name = "cpu/pbr403",
> +    .name = "cpu/tlb6xx/pbr403",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = pbr403_needed,
> --
> 2.34.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 2/3] migration: Add canary to VMSTATE_END_OF_LIST

[Philippe Mathieu-Daudé via]

On 13/1/22 20:44, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> We fairly regularly forget VMSTATE_END_OF_LIST markers off descriptions;
> given that the current check is only for ->name being NULL, sometimes
> we get unlucky and the code apparently works and no one spots the error.
> 
> Explicitly add a flag, VMS_END that should be set, and assert it is
> set during the traversal.
> 
> Note: This can't go in until we update the copy of vmstate.h in slirp.

Do we need a libslirp buildsys version check to get this patch merged?

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   include/migration/vmstate.h | 7 ++++++-
>   migration/savevm.c          | 1 +
>   migration/vmstate.c         | 2 ++
>   3 files changed, 9 insertions(+), 1 deletion(-)

Re: [PATCH v2 2/3] migration: Add canary to VMSTATE_END_OF_LIST

[Philippe Mathieu-Daudé via]

On 14/1/22 12:32, Philippe Mathieu-Daudé wrote:
> On 13/1/22 20:44, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> We fairly regularly forget VMSTATE_END_OF_LIST markers off descriptions;
>> given that the current check is only for ->name being NULL, sometimes
>> we get unlucky and the code apparently works and no one spots the error.
>>
>> Explicitly add a flag, VMS_END that should be set, and assert it is
>> set during the traversal.
>>
>> Note: This can't go in until we update the copy of vmstate.h in slirp.
> 
> Do we need a libslirp buildsys version check to get this patch merged?

In that case we should use an intermediate function which would
eventually call assert() after checking SLIRP_MAJOR_VERSION/...
values.

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> ---
>>   include/migration/vmstate.h | 7 ++++++-
>>   migration/savevm.c          | 1 +
>>   migration/vmstate.c         | 2 ++
>>   3 files changed, 9 insertions(+), 1 deletion(-)

Re: [PATCH v2 1/3] ppc: Fix vmstate_pbr403 subsection name

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 13 Jan 2022 at 19:45, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> >
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > The pbr403 subsection is part of the tlb6xx state, so I believe it's
> > name needs to be:
> >
> >     .name = "cpu/tlb6xx/pbr403",
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  target/ppc/machine.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 756d8de5d8..e535edb7c4 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -718,7 +718,7 @@ static bool pbr403_needed(void *opaque)
> >  }
> >
> >  static const VMStateDescription vmstate_pbr403 = {
> > -    .name = "cpu/pbr403",
> > +    .name = "cpu/tlb6xx/pbr403",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .needed = pbr403_needed,
> > --
> > 2.34.1
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Actually, we'll drop this one; Cédric Le Goater has just published
'target/ppc: Finish removal of 401/403 CPUs' that kills this code off.

Dave

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 1/3] ppc: Fix vmstate_pbr403 subsection name

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The pbr403 subsection is part of the tlb6xx state, so I believe it's
> name needs to be:
>
>     .name = "cpu/tlb6xx/pbr403",
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dropped as code don't exist anymore.


> ---
>  target/ppc/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 756d8de5d8..e535edb7c4 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -718,7 +718,7 @@ static bool pbr403_needed(void *opaque)
>  }
>  
>  static const VMStateDescription vmstate_pbr403 = {
> -    .name = "cpu/pbr403",
> +    .name = "cpu/tlb6xx/pbr403",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = pbr403_needed,

Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Perform a check on vmsd structures during test runs in the hope
> of catching any missing terminators and other simple screwups.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.

Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Perform a check on vmsd structures during test runs in the hope
> > of catching any missing terminators and other simple screwups.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> queued.

Careful; I think that'll break with slirp until libslirp gets updated
first.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Perform a check on vmsd structures during test runs in the hope
>> > of catching any missing terminators and other simple screwups.
>> >
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> 
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> 
>> queued.
>
> Careful; I think that'll break with slirp until libslirp gets updated
> first.

As expected, it broke it.

I resend the PULL request wihtout that two patches.

Once that we are here, how it is that make check didn't catch this?

Later, Juan.

Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Perform a check on vmsd structures during test runs in the hope
> >> > of catching any missing terminators and other simple screwups.
> >> >
> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> 
> >> Reviewed-by: Juan Quintela <quintela@redhat.com>
> >> 
> >> queued.
> >
> > Careful; I think that'll break with slirp until libslirp gets updated
> > first.
> 
> As expected, it broke it.
> 
> I resend the PULL request wihtout that two patches.
> 
> Once that we are here, how it is that make check didn't catch this?

Because in my local world I did the changes to libslirp; I wanted to
make sure qemu people were happy with the changes before proposing them
to libslirp.

Which I've just done:

https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/112

Dave

> Later, Juan.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > * Juan Quintela (quintela@redhat.com) wrote:
>> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
>> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >> >
>> >> > Perform a check on vmsd structures during test runs in the hope
>> >> > of catching any missing terminators and other simple screwups.
>> >> >
>> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >> 
>> >> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> >> 
>> >> queued.
>> >
>> > Careful; I think that'll break with slirp until libslirp gets updated
>> > first.
>> 
>> As expected, it broke it.
>> 
>> I resend the PULL request wihtout that two patches.
>> 
>> Once that we are here, how it is that make check didn't catch this?
>
> Because in my local world I did the changes to libslirp; I wanted to
> make sure qemu people were happy with the changes before proposing them
> to libslirp.
>
> Which I've just done:
>
> https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/112

I mean make check.

It worked for me on my PULL request.  I would have assumed that it
checks slirp.

Later, Juan.

Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests

[Peter Maydell]

On Wed, 2 Feb 2022 at 11:32, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> Because in my local world I did the changes to libslirp; I wanted to
> make sure qemu people were happy with the changes before proposing them
> to libslirp.
>
> Which I've just done:
>
> https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/112

Does QEMU's own vmstate handling code see the libslirp vmstate
structures ? Looking at the code it seems to me like QEMU's
migration code only interacts with slirp via the
slirp_state_save() and slirp_state_load() functions.
Internally those work with some use of a vmstate structure,
but the code that iterates over field arrays in those is
all inside slirp itself (in src/slirp/vmstate.c if you're
looking at the in-tree copy).

So maybe I'm missing something but I'm not sure there really
is a dependency on the libslirp change here...

-- PMM