[PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Peter Xu]

I'm doing some machine type checks to make sure nothing breaks for
7.2<->8.0.  Along the way I found one false negative report on e1000e using
the static checker, turns out to be an issue in the checker itself.

The problem is the checker doesn't take VMS_ARRAY into account when
comparing with UNUSED, hence the total size is wrongly calculated.

Fix that first in qemu by start dumping size of array as "num", then teach
the checker for that.

NOTE: the patchset will change both behaviors for either -dump-vmstate on
QEMU or the checker, however both patches will be compatible even with old
QEMU dumps or even old vmstate-checker script.  That's not extremely
important, IMHO, but still worth mentioning.

Thanks,

Peter Xu (2):
  migration/vmstate-dump: Dump array size too as "num"
  vmstate-static-checker: Recognize "num" field

 migration/savevm.c                |  3 +++
 scripts/vmstate-static-checker.py | 13 ++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.39.1

[PATCH 1/2] migration/vmstate-dump: Dump array size too as "num"

[Peter Xu]

For VMS_ARRAY typed vmsd fields, also dump the number of entries in the
array in -vmstate-dump.

Without such information, vmstate static checker can report false negatives
of incompatible vmsd on VMS_ARRAY typed fields, when the src/dst do not
have the same type of array defined.  It's because in the checker we only
check against size of fields within a VMSD field.

One example: e1000e used to have a field defined as a boolean array with 5
entries, then removed it and replaced it with UNUSED (in 31e3f318c8b535):

-        VMSTATE_BOOL_ARRAY(core.eitr_intr_pending, E1000EState,
-                           E1000E_MSIX_VEC_NUM),
+        VMSTATE_UNUSED(E1000E_MSIX_VEC_NUM),

It's a legal replacement but vmstate static checker is not happy with it,
because it checks only against the "size" field between the two
fields (here one is BOOL_ARRAY, the other is UNUSED):

For BOOL_ARRAY:

      {
        "field": "core.eitr_intr_pending",
        "version_id": 0,
        "field_exists": false,
        "size": 1
      },

For UNUSED:

      {
        "field": "unused",
        "version_id": 0,
        "field_exists": false,
        "size": 5
      },

It's not the script to blame because there's just not enough information
dumped to show the total size of the entry for an array.  Add it.

Note that this will not break old vmstate checker because the field will
just be ignored.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 9671211339..550c38f0ef 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -536,6 +536,9 @@ static void dump_vmstate_vmsf(FILE *out_file, const VMStateField *field,
             field->version_id);
     fprintf(out_file, "%*s\"field_exists\": %s,\n", indent, "",
             field->field_exists ? "true" : "false");
+    if (field->flags & VMS_ARRAY) {
+        fprintf(out_file, "%*s\"num\": %d,\n", indent, "", field->num);
+    }
     fprintf(out_file, "%*s\"size\": %zu", indent, "", field->size);
     if (field->vmsd != NULL) {
         fprintf(out_file, ",\n");
-- 
2.39.1

Re: [PATCH 1/2] migration/vmstate-dump: Dump array size too as "num"

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> For VMS_ARRAY typed vmsd fields, also dump the number of entries in the
> array in -vmstate-dump.
>
> Without such information, vmstate static checker can report false negatives
> of incompatible vmsd on VMS_ARRAY typed fields, when the src/dst do not
> have the same type of array defined.  It's because in the checker we only
> check against size of fields within a VMSD field.
>
> One example: e1000e used to have a field defined as a boolean array with 5
> entries, then removed it and replaced it with UNUSED (in 31e3f318c8b535):
>
> -        VMSTATE_BOOL_ARRAY(core.eitr_intr_pending, E1000EState,
> -                           E1000E_MSIX_VEC_NUM),
> +        VMSTATE_UNUSED(E1000E_MSIX_VEC_NUM),
>
> It's a legal replacement but vmstate static checker is not happy with it,
> because it checks only against the "size" field between the two
> fields (here one is BOOL_ARRAY, the other is UNUSED):
>
> For BOOL_ARRAY:
>
>       {
>         "field": "core.eitr_intr_pending",
>         "version_id": 0,
>         "field_exists": false,
>         "size": 1
>       },
>
> For UNUSED:
>
>       {
>         "field": "unused",
>         "version_id": 0,
>         "field_exists": false,
>         "size": 5
>       },
>
> It's not the script to blame because there's just not enough information
> dumped to show the total size of the entry for an array.  Add it.
>
> Note that this will not break old vmstate checker because the field will
> just be ignored.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

queued.

[PATCH 2/2] vmstate-static-checker: Recognize "num" field

[Peter Xu]

Recognize this field for VMS_ARRAY typed vmsd fields, then we can do proper
size matching with previous patch.

Note that this is compatible with old -dump-vmstate output, because when
"num" is not there we'll still use the old "size" only.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 scripts/vmstate-static-checker.py | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/scripts/vmstate-static-checker.py b/scripts/vmstate-static-checker.py
index dfeee8231a..9c0e6b81f2 100755
--- a/scripts/vmstate-static-checker.py
+++ b/scripts/vmstate-static-checker.py
@@ -134,6 +134,11 @@ def exists_in_substruct(fields, item):
     return check_fields_match(fields["Description"]["name"],
                               substruct_fields[0]["field"], item)
 
+def size_total(entry):
+    size = entry["size"]
+    if "num" not in entry:
+        return size
+    return size * entry["num"]
 
 def check_fields(src_fields, dest_fields, desc, sec):
     # This function checks for all the fields in a section.  If some
@@ -249,17 +254,19 @@ def check_fields(src_fields, dest_fields, desc, sec):
                 continue
 
             if s_item["field"] == "unused" or d_item["field"] == "unused":
-                if s_item["size"] == d_item["size"]:
+                s_size = size_total(s_item)
+                d_size = size_total(d_item)
+                if s_size == d_size:
                     continue
 
                 if d_item["field"] == "unused":
                     advance_dest = False
-                    unused_count = d_item["size"] - s_item["size"]
+                    unused_count = d_size - s_size;
                     continue
 
                 if s_item["field"] == "unused":
                     advance_src = False
-                    unused_count = s_item["size"] - d_item["size"]
+                    unused_count = s_size - d_size
                     continue
 
             print("Section \"" + sec + "\",", end=' ')
-- 
2.39.1

Re: [PATCH 2/2] vmstate-static-checker: Recognize "num" field

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Recognize this field for VMS_ARRAY typed vmsd fields, then we can do proper
> size matching with previous patch.
>
> Note that this is compatible with old -dump-vmstate output, because when
> "num" is not there we'll still use the old "size" only.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

queued.

Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> I'm doing some machine type checks to make sure nothing breaks for
> 7.2<->8.0.  Along the way I found one false negative report on e1000e using
> the static checker, turns out to be an issue in the checker itself.
>
> The problem is the checker doesn't take VMS_ARRAY into account when
> comparing with UNUSED, hence the total size is wrongly calculated.
>
> Fix that first in qemu by start dumping size of array as "num", then teach
> the checker for that.
>
> NOTE: the patchset will change both behaviors for either -dump-vmstate on
> QEMU or the checker, however both patches will be compatible even with old
> QEMU dumps or even old vmstate-checker script.  That's not extremely
> important, IMHO, but still worth mentioning.
>
> Thanks,
>
> Peter Xu (2):
>   migration/vmstate-dump: Dump array size too as "num"
>   vmstate-static-checker: Recognize "num" field
>
>  migration/savevm.c                |  3 +++
>  scripts/vmstate-static-checker.py | 13 ++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)

Hi

once that you are working with the static checker.

Could we just run two checks in make check:

- qemu-<whatever> -M <previous-version> against the one from previous
  version, and see that they match.
- qemu-<whatever> -M <latests> against the one from previous version
  And we save the diffs each time that we add something incompatible and
  fix it on source.

I will start with x86_64.  And once that we have it running, the other
architectures that care about version compatibility can add to it.

What do you think?

Later, Juan.

Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Peter Xu]

On Wed, Apr 26, 2023 at 06:36:00PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > I'm doing some machine type checks to make sure nothing breaks for
> > 7.2<->8.0.  Along the way I found one false negative report on e1000e using
> > the static checker, turns out to be an issue in the checker itself.
> >
> > The problem is the checker doesn't take VMS_ARRAY into account when
> > comparing with UNUSED, hence the total size is wrongly calculated.
> >
> > Fix that first in qemu by start dumping size of array as "num", then teach
> > the checker for that.
> >
> > NOTE: the patchset will change both behaviors for either -dump-vmstate on
> > QEMU or the checker, however both patches will be compatible even with old
> > QEMU dumps or even old vmstate-checker script.  That's not extremely
> > important, IMHO, but still worth mentioning.
> >
> > Thanks,
> >
> > Peter Xu (2):
> >   migration/vmstate-dump: Dump array size too as "num"
> >   vmstate-static-checker: Recognize "num" field
> >
> >  migration/savevm.c                |  3 +++
> >  scripts/vmstate-static-checker.py | 13 ++++++++++---
> >  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> Hi
> 
> once that you are working with the static checker.
> 
> Could we just run two checks in make check:
> 
> - qemu-<whatever> -M <previous-version> against the one from previous
>   version, and see that they match.
> - qemu-<whatever> -M <latests> against the one from previous version
>   And we save the diffs each time that we add something incompatible and
>   fix it on source.

Normally we don't have "latest machine" but only "previous"?  Checking
"previous" would be enough, right?  E.g. currently we're at 8.1 dev window,
so we check against 8.0 with whatever new thing coming.

> 
> I will start with x86_64.  And once that we have it running, the other
> architectures that care about version compatibility can add to it.
> 
> What do you think?

It sounds a good idea to have some way to check compat bits in unit tests.
I'm just not sure whether it's easy to integrate to make check: the
comparision requires building two qemu binaries; one normally with an old
tag that I built manually.

For the static checker itself, it normally also needs some intervention
from human, e.g., it doesn't understand things like field_exists() so it
can report "warnings" only which can be false negative even with ARRAY
issue fixed.

But ideally e.g. in a CI env we can always keep an old version qemu binary
ready for each arch to be tested, then verify forward+backward migration
with that old machine type with whatever patch applied on top.  One trick
here is we need to make sure the test cmdline contains the device/anything
that got changed by the patch.  It may not always be the case.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Apr 26, 2023 at 06:36:00PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > I'm doing some machine type checks to make sure nothing breaks for
>> > 7.2<->8.0.  Along the way I found one false negative report on e1000e using
>> > the static checker, turns out to be an issue in the checker itself.
>> >
>> > The problem is the checker doesn't take VMS_ARRAY into account when
>> > comparing with UNUSED, hence the total size is wrongly calculated.
>> >
>> > Fix that first in qemu by start dumping size of array as "num", then teach
>> > the checker for that.
>> >
>> > NOTE: the patchset will change both behaviors for either -dump-vmstate on
>> > QEMU or the checker, however both patches will be compatible even with old
>> > QEMU dumps or even old vmstate-checker script.  That's not extremely
>> > important, IMHO, but still worth mentioning.
>> >
>> > Thanks,
>> >
>> > Peter Xu (2):
>> >   migration/vmstate-dump: Dump array size too as "num"
>> >   vmstate-static-checker: Recognize "num" field
>> >
>> >  migration/savevm.c                |  3 +++
>> >  scripts/vmstate-static-checker.py | 13 ++++++++++---
>> >  2 files changed, 13 insertions(+), 3 deletions(-)
>> 
>> Hi
>> 
>> once that you are working with the static checker.
>> 
>> Could we just run two checks in make check:
>> 
>> - qemu-<whatever> -M <previous-version> against the one from previous
>>   version, and see that they match.
>> - qemu-<whatever> -M <latests> against the one from previous version
>>   And we save the diffs each time that we add something incompatible and
>>   fix it on source.
>
> Normally we don't have "latest machine" but only "previous"?  Checking
> "previous" would be enough, right?  E.g. currently we're at 8.1 dev window,
> so we check against 8.0 with whatever new thing coming.

$ qemu-8.0.0 -M pc-q35-8.0.0 > dump-8.0.0-q35

We generate that dump-8.0-q35 on the tree.
We will change that once that we release 8.1, until then that is latests.

This qemu upstream is whatever is in HEAD

$ qemu-upstream -M pc-q35-8.0.0 > dump-8.0.0-upstream-q35

diff dump-8.0.0-q35 dump-8.0.0-upstream-q35

And it should be empty.

$ qemu-upstream -M pc-q35-8.1.0 > dump-8.1.0-upstream-q35

diff dump-8.0.0-q35 dump-8.1.0-upstream-q35

Each time that we find a difference, we know that we have to create a
property for that to make pc-q35-8.0.0 working.  We save that "hunk"
somehow.

Where I have put diff, we can have something more intelligent that is
able to compare json output and have into account that differences that
we already know that exist.

>> I will start with x86_64.  And once that we have it running, the other
>> architectures that care about version compatibility can add to it.
>> 
>> What do you think?
>
> It sounds a good idea to have some way to check compat bits in unit tests.
> I'm just not sure whether it's easy to integrate to make check: the
> comparision requires building two qemu binaries; one normally with an old
> tag that I built manually.

No. Single binary.  For the old binary we just have saved its output on
the tree.

> For the static checker itself, it normally also needs some intervention
> from human, e.g., it doesn't understand things like field_exists() so it
> can report "warnings" only which can be false negative even with ARRAY
> issue fixed.

This is why I mean that I want the "diff" to be a bit more intelligent
and "record" the things that we tell them that are correct.

> But ideally e.g. in a CI env we can always keep an old version qemu binary
> ready for each arch to be tested, then verify forward+backward migration
> with that old machine type with whatever patch applied on top.  One trick
> here is we need to make sure the test cmdline contains the device/anything
> that got changed by the patch.  It may not always be the case.

I will start with the default machine devices.
Once the mechanism is done, we can wonder with more configurations.

I will start small and then go from there.

Later, Juan.

Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Peter Xu]

On Wed, Apr 26, 2023 at 08:54:02PM +0200, Juan Quintela wrote:
> This is why I mean that I want the "diff" to be a bit more intelligent
> and "record" the things that we tell them that are correct.

I think I see what you meant. :)

> I will start with the default machine devices.
> Once the mechanism is done, we can wonder with more configurations.
> 
> I will start small and then go from there.

Sounds good!

-- 
Peter Xu