[PATCH v2 0/7] migration: Fix s390 regressions + migration script

[PATCH v2 0/7] migration: Fix s390 regressions + migration script

[Fabiano Rosas]

changes from v1:

- dropped comments patch
- new patch 4: rename the field to nullptr
- patch 6: add a sample JSON, fix the appending code

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1616920974

v1:
https://lore.kernel.org/r/20250107195025.9951-1-farosas@suse.de

Hi,

The situation that broke the last migration PR was:

1) emitting of JSON data by QEMU for
   VMSTATE_ARRAY_OF_POINTER_TO_STRUCT when NULL pointers are present
   has been broken for a while;

2) parsing of s390x migration stream by analyze-script.py has been
   broken for a while;

   (there's indications that it worked on s390x hosts, I'm assuming due
   to byte order coincidences)

3) s390x CSS migration has been broken for a while;

The s390x CSS migration uses VMSTATE_ARRAY_OF_POINTER_TO_STRUCT with
NULL pointers, triggering #1, but hidden due to #2 on TCG hosts and
due to #3 overall.

- patches 1: just to make rebase easier
- patches 2-3: cleanups
- patch 4: fixes #2
- patches 5-6: fix #1
- patch 7: fixes #3

Fabiano Rosas (6):
  migration: Add more error handling to analyze-migration.py
  migration: Remove unused argument in vmsd_desc_field_end
  migration: Fix parsing of s390 stream
  migration: Rename vmstate_info_nullptr
  migration: Fix arrays of pointers in JSON writer
  s390x: Fix CSS migration

Peter Xu (1):
  migration: Dump correct JSON format for nullptr replacement

 hw/s390x/s390-virtio-ccw.c   |   2 +-
 migration/vmstate-types.c    |   2 +-
 migration/vmstate.c          | 151 ++++++++++++++++++++++++++++-------
 scripts/analyze-migration.py | 144 ++++++++++++++++++++++++---------
 4 files changed, 230 insertions(+), 69 deletions(-)

-- 
2.35.3

[PATCH v2 1/7] migration: Add more error handling to analyze-migration.py

[Fabiano Rosas]

The analyze-migration script was seen failing in s390x in misterious
ways. It seems we're reaching the VMSDFieldStruct constructor without
any fields, which would indicate an empty .subsection entry, a
VMSTATE_STRUCT with no fields or a vmsd with no fields. We don't have
any of those, at least not without the unmigratable flag set, so this
should never happen.

Add some debug statements so that we can see what's going on the next
time the issue happens.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Message-Id: <20250103141305.8435-1-farosas@suse.de>
---
 scripts/analyze-migration.py | 75 +++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 8a254a5b6a..f2457b1dde 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -429,6 +429,9 @@ def __init__(self, desc, file):
         super(VMSDFieldStruct, self).__init__(desc, file)
         self.data = collections.OrderedDict()
 
+        if 'fields' not in self.desc['struct']:
+            raise Exception("No fields in struct. VMSD:\n%s" % self.desc)
+
         # When we see compressed array elements, unfold them here
         new_fields = []
         for field in self.desc['struct']['fields']:
@@ -477,6 +480,10 @@ def read(self):
                     raise Exception("Subsection %s not found at offset %x" % ( subsection['vmsd_name'], self.file.tell()))
                 name = self.file.readstr()
                 version_id = self.file.read32()
+
+                if not subsection:
+                    raise Exception("Empty description for subsection: %s" % name)
+
                 self.data[name] = VMSDSection(self.file, version_id, subsection, (name, 0))
                 self.data[name].read()
 
@@ -574,10 +581,13 @@ def __init__(self, filename):
         }
         self.filename = filename
         self.vmsd_desc = None
+        self.vmsd_json = ""
 
-    def read(self, desc_only = False, dump_memory = False, write_memory = False):
+    def read(self, desc_only = False, dump_memory = False,
+             write_memory = False):
         # Read in the whole file
         file = MigrationFile(self.filename)
+        self.vmsd_json = file.read_migration_debug_json()
 
         # File magic
         data = file.read32()
@@ -635,9 +645,11 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
         file.close()
 
     def load_vmsd_json(self, file):
-        vmsd_json = file.read_migration_debug_json()
-        self.vmsd_desc = json.loads(vmsd_json, object_pairs_hook=collections.OrderedDict)
+        self.vmsd_desc = json.loads(self.vmsd_json,
+                                    object_pairs_hook=collections.OrderedDict)
         for device in self.vmsd_desc['devices']:
+            if 'fields' not in device:
+                raise Exception("vmstate for device %s has no fields" % device['name'])
             key = (device['name'], device['instance_id'])
             value = ( VMSDSection, device )
             self.section_classes[key] = value
@@ -666,31 +678,34 @@ def default(self, o):
 
 jsonenc = JSONEncoder(indent=4, separators=(',', ': '))
 
-if args.extract:
-    dump = MigrationDump(args.file)
-
-    dump.read(desc_only = True)
-    print("desc.json")
-    f = open("desc.json", "w")
-    f.truncate()
-    f.write(jsonenc.encode(dump.vmsd_desc))
-    f.close()
-
-    dump.read(write_memory = True)
-    dict = dump.getDict()
-    print("state.json")
-    f = open("state.json", "w")
-    f.truncate()
-    f.write(jsonenc.encode(dict))
-    f.close()
-elif args.dump == "state":
-    dump = MigrationDump(args.file)
-    dump.read(dump_memory = args.memory)
-    dict = dump.getDict()
-    print(jsonenc.encode(dict))
-elif args.dump == "desc":
-    dump = MigrationDump(args.file)
-    dump.read(desc_only = True)
-    print(jsonenc.encode(dump.vmsd_desc))
-else:
+if not any([args.extract, args.dump == "state", args.dump == "desc"]):
     raise Exception("Please specify either -x, -d state or -d desc")
+
+try:
+    dump = MigrationDump(args.file)
+
+    if args.extract:
+        dump.read(desc_only = True)
+
+        print("desc.json")
+        f = open("desc.json", "w")
+        f.truncate()
+        f.write(jsonenc.encode(dump.vmsd_desc))
+        f.close()
+
+        dump.read(write_memory = True)
+        dict = dump.getDict()
+        print("state.json")
+        f = open("state.json", "w")
+        f.truncate()
+        f.write(jsonenc.encode(dict))
+        f.close()
+    elif args.dump == "state":
+        dump.read(dump_memory = args.memory)
+        dict = dump.getDict()
+        print(jsonenc.encode(dict))
+    elif args.dump == "desc":
+        dump.read(desc_only = True)
+        print(jsonenc.encode(dump.vmsd_desc))
+except Exception:
+    raise Exception("Full JSON dump:\n%s", dump.vmsd_json)
-- 
2.35.3

[PATCH v2 2/7] migration: Remove unused argument in vmsd_desc_field_end

[Fabiano Rosas]

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/vmstate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index fa002b24e8..aa2821dec6 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -311,7 +311,7 @@ static void vmsd_desc_field_start(const VMStateDescription *vmsd,
 
 static void vmsd_desc_field_end(const VMStateDescription *vmsd,
                                 JSONWriter *vmdesc,
-                                const VMStateField *field, size_t size, int i)
+                                const VMStateField *field, size_t size)
 {
     if (!vmdesc) {
         return;
@@ -420,7 +420,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 }
 
                 written_bytes = qemu_file_transferred(f) - old_offset;
-                vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
+                vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes);
 
                 /* Compressed arrays only care about the first element */
                 if (vmdesc_loop && vmsd_can_compress(field)) {
-- 
2.35.3

[PATCH v2 3/7] migration: Fix parsing of s390 stream

[Fabiano Rosas]

The parsing for the S390StorageAttributes section is currently leaving
an unconsumed token that is later interpreted by the generic code as
QEMU_VM_EOF, cutting the parsing short.

The migration will issue a STATTR_FLAG_DONE between iterations, which
the script consumes correctly, but there's a final STATTR_FLAG_EOS at
.save_complete that the script is ignoring. Since the EOS flag is a
u64 0x1ULL and the stream is big endian, on little endian hosts a byte
read from it will be 0x0, the same as QEMU_VM_EOF.

Fixes: 81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py for s390x")
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 scripts/analyze-migration.py | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index f2457b1dde..fcda11f31d 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -65,6 +65,9 @@ def readvar(self, size = None):
     def tell(self):
         return self.file.tell()
 
+    def seek(self, a, b):
+        return self.file.seek(a, b)
+
     # The VMSD description is at the end of the file, after EOF. Look for
     # the last NULL byte, then for the beginning brace of JSON.
     def read_migration_debug_json(self):
@@ -272,11 +275,24 @@ def __init__(self, file, version_id, device, section_key):
         self.section_key = section_key
 
     def read(self):
+        pos = 0
         while True:
             addr_flags = self.file.read64()
             flags = addr_flags & 0xfff
-            if (flags & (self.STATTR_FLAG_DONE | self.STATTR_FLAG_EOS)):
+
+            if flags & self.STATTR_FLAG_DONE:
+                pos = self.file.tell()
+                continue
+            elif flags & self.STATTR_FLAG_EOS:
                 return
+            else:
+                # No EOS came after DONE, that's OK, but rewind the
+                # stream because this is not our data.
+                if pos:
+                    self.file.seek(pos, os.SEEK_SET)
+                    return
+                raise Exception("Unknown flags %x", flags)
+
             if (flags & self.STATTR_FLAG_ERROR):
                 raise Exception("Error in migration stream")
             count = self.file.read64()
-- 
2.35.3

[PATCH v2 4/7] migration: Rename vmstate_info_nullptr

[Fabiano Rosas]

Rename vmstate_info_nullptr from "uint64_t" to "nullptr". This vmstate
actually reads and writes just a byte, so the proper name would be
uint8. However, since this is a marker for a NULL pointer, it's
convenient to have a more explicit name that can be identified by the
consumers of the JSON part of the stream.

Change the name to "nullptr" and add support for it in the
analyze-migration.py script. Arbitrarily use the name of the type as
the value of the field to avoid the script showing 0x30 or '0', which
could be confusing for readers.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/vmstate-types.c    |  2 +-
 scripts/analyze-migration.py | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index e83bfccb9e..d70d573dbd 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -338,7 +338,7 @@ static int put_nullptr(QEMUFile *f, void *pv, size_t size,
 }
 
 const VMStateInfo vmstate_info_nullptr = {
-    .name = "uint64",
+    .name = "nullptr",
     .get  = get_nullptr,
     .put  = put_nullptr,
 };
diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index fcda11f31d..134c25f20a 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -377,6 +377,8 @@ def read(self):
 
 
 class VMSDFieldInt(VMSDFieldGeneric):
+    NULL_PTR_MARKER = 0x30
+
     def __init__(self, desc, file):
         super(VMSDFieldInt, self).__init__(desc, file)
         self.size = int(desc['size'])
@@ -385,6 +387,16 @@ def __init__(self, desc, file):
         self.udtype = '>u%d' % self.size
 
     def __repr__(self):
+
+        # A NULL pointer is encoded in the stream as a '0' to
+        # disambiguate from a mere 0x0 value and avoid consumers
+        # trying to follow the NULL pointer. Displaying '0', 0x30 or
+        # 0x0 when analyzing the JSON debug stream could become
+        # confusing, so use an explicit term instead. The actual value
+        # in the stream was already validated by VMSDFieldNull.
+        if self.data == self.NULL_PTR_MARKER:
+            return "nullptr"
+
         if self.data < 0:
             return ('%s (%d)' % ((self.format % self.udata), self.data))
         else:
@@ -417,6 +429,15 @@ def __init__(self, desc, file):
         super(VMSDFieldIntLE, self).__init__(desc, file)
         self.dtype = '<i%d' % self.size
 
+class VMSDFieldNull(VMSDFieldUInt):
+    def __init__(self, desc, file):
+        super(VMSDFieldUInt, self).__init__(desc, file)
+
+    def read(self):
+        super(VMSDFieldUInt, self).read()
+        assert(self.data == self.NULL_PTR_MARKER)
+        return self.data
+
 class VMSDFieldBool(VMSDFieldGeneric):
     def __init__(self, desc, file):
         super(VMSDFieldBool, self).__init__(desc, file)
@@ -558,6 +579,7 @@ def getDict(self):
     "bitmap" : VMSDFieldGeneric,
     "struct" : VMSDFieldStruct,
     "capability": VMSDFieldCap,
+    "nullptr": VMSDFieldNull,
     "unknown" : VMSDFieldGeneric,
 }
 
-- 
2.35.3

Re: [PATCH v2 4/7] migration: Rename vmstate_info_nullptr

[Peter Xu]

On Thu, Jan 09, 2025 at 11:09:56AM -0300, Fabiano Rosas wrote:
> Rename vmstate_info_nullptr from "uint64_t" to "nullptr". This vmstate
> actually reads and writes just a byte, so the proper name would be
> uint8. However, since this is a marker for a NULL pointer, it's
> convenient to have a more explicit name that can be identified by the
> consumers of the JSON part of the stream.
> 
> Change the name to "nullptr" and add support for it in the
> analyze-migration.py script. Arbitrarily use the name of the type as
> the value of the field to avoid the script showing 0x30 or '0', which
> could be confusing for readers.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/vmstate-types.c    |  2 +-
>  scripts/analyze-migration.py | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index e83bfccb9e..d70d573dbd 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -338,7 +338,7 @@ static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>  }
>  
>  const VMStateInfo vmstate_info_nullptr = {
> -    .name = "uint64",
> +    .name = "nullptr",
>      .get  = get_nullptr,
>      .put  = put_nullptr,
>  };
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index fcda11f31d..134c25f20a 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -377,6 +377,8 @@ def read(self):
>  
>  
>  class VMSDFieldInt(VMSDFieldGeneric):
> +    NULL_PTR_MARKER = 0x30
> +
>      def __init__(self, desc, file):
>          super(VMSDFieldInt, self).__init__(desc, file)
>          self.size = int(desc['size'])
> @@ -385,6 +387,16 @@ def __init__(self, desc, file):
>          self.udtype = '>u%d' % self.size
>  
>      def __repr__(self):
> +
> +        # A NULL pointer is encoded in the stream as a '0' to
> +        # disambiguate from a mere 0x0 value and avoid consumers
> +        # trying to follow the NULL pointer. Displaying '0', 0x30 or
> +        # 0x0 when analyzing the JSON debug stream could become
> +        # confusing, so use an explicit term instead. The actual value
> +        # in the stream was already validated by VMSDFieldNull.
> +        if self.data == self.NULL_PTR_MARKER:
> +            return "nullptr"

What happens if a real int field has value 0x30 which is not a marker?
Would it be wrongly represented as "nullptr"?

> +
>          if self.data < 0:
>              return ('%s (%d)' % ((self.format % self.udata), self.data))
>          else:
> @@ -417,6 +429,15 @@ def __init__(self, desc, file):
>          super(VMSDFieldIntLE, self).__init__(desc, file)
>          self.dtype = '<i%d' % self.size
>  
> +class VMSDFieldNull(VMSDFieldUInt):
> +    def __init__(self, desc, file):
> +        super(VMSDFieldUInt, self).__init__(desc, file)
> +
> +    def read(self):
> +        super(VMSDFieldUInt, self).read()
> +        assert(self.data == self.NULL_PTR_MARKER)
> +        return self.data
> +
>  class VMSDFieldBool(VMSDFieldGeneric):
>      def __init__(self, desc, file):
>          super(VMSDFieldBool, self).__init__(desc, file)
> @@ -558,6 +579,7 @@ def getDict(self):
>      "bitmap" : VMSDFieldGeneric,
>      "struct" : VMSDFieldStruct,
>      "capability": VMSDFieldCap,
> +    "nullptr": VMSDFieldNull,
>      "unknown" : VMSDFieldGeneric,
>  }
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v2 4/7] migration: Rename vmstate_info_nullptr

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jan 09, 2025 at 11:09:56AM -0300, Fabiano Rosas wrote:
>> Rename vmstate_info_nullptr from "uint64_t" to "nullptr". This vmstate
>> actually reads and writes just a byte, so the proper name would be
>> uint8. However, since this is a marker for a NULL pointer, it's
>> convenient to have a more explicit name that can be identified by the
>> consumers of the JSON part of the stream.
>> 
>> Change the name to "nullptr" and add support for it in the
>> analyze-migration.py script. Arbitrarily use the name of the type as
>> the value of the field to avoid the script showing 0x30 or '0', which
>> could be confusing for readers.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/vmstate-types.c    |  2 +-
>>  scripts/analyze-migration.py | 22 ++++++++++++++++++++++
>>  2 files changed, 23 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
>> index e83bfccb9e..d70d573dbd 100644
>> --- a/migration/vmstate-types.c
>> +++ b/migration/vmstate-types.c
>> @@ -338,7 +338,7 @@ static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>>  }
>>  
>>  const VMStateInfo vmstate_info_nullptr = {
>> -    .name = "uint64",
>> +    .name = "nullptr",
>>      .get  = get_nullptr,
>>      .put  = put_nullptr,
>>  };
>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
>> index fcda11f31d..134c25f20a 100755
>> --- a/scripts/analyze-migration.py
>> +++ b/scripts/analyze-migration.py
>> @@ -377,6 +377,8 @@ def read(self):
>>  
>>  
>>  class VMSDFieldInt(VMSDFieldGeneric):
>> +    NULL_PTR_MARKER = 0x30
>> +
>>      def __init__(self, desc, file):
>>          super(VMSDFieldInt, self).__init__(desc, file)
>>          self.size = int(desc['size'])
>> @@ -385,6 +387,16 @@ def __init__(self, desc, file):
>>          self.udtype = '>u%d' % self.size
>>  
>>      def __repr__(self):
>> +
>> +        # A NULL pointer is encoded in the stream as a '0' to
>> +        # disambiguate from a mere 0x0 value and avoid consumers
>> +        # trying to follow the NULL pointer. Displaying '0', 0x30 or
>> +        # 0x0 when analyzing the JSON debug stream could become
>> +        # confusing, so use an explicit term instead. The actual value
>> +        # in the stream was already validated by VMSDFieldNull.
>> +        if self.data == self.NULL_PTR_MARKER:
>> +            return "nullptr"
>
> What happens if a real int field has value 0x30 which is not a marker?
> Would it be wrongly represented as "nullptr"?
>

Yes, better to not inherit from VMSDFieldInt then.

>> +
>>          if self.data < 0:
>>              return ('%s (%d)' % ((self.format % self.udata), self.data))
>>          else:
>> @@ -417,6 +429,15 @@ def __init__(self, desc, file):
>>          super(VMSDFieldIntLE, self).__init__(desc, file)
>>          self.dtype = '<i%d' % self.size
>>  
>> +class VMSDFieldNull(VMSDFieldUInt):
>> +    def __init__(self, desc, file):
>> +        super(VMSDFieldUInt, self).__init__(desc, file)
>> +
>> +    def read(self):
>> +        super(VMSDFieldUInt, self).read()
>> +        assert(self.data == self.NULL_PTR_MARKER)
>> +        return self.data
>> +
>>  class VMSDFieldBool(VMSDFieldGeneric):
>>      def __init__(self, desc, file):
>>          super(VMSDFieldBool, self).__init__(desc, file)
>> @@ -558,6 +579,7 @@ def getDict(self):
>>      "bitmap" : VMSDFieldGeneric,
>>      "struct" : VMSDFieldStruct,
>>      "capability": VMSDFieldCap,
>> +    "nullptr": VMSDFieldNull,
>>      "unknown" : VMSDFieldGeneric,
>>  }
>>  
>> -- 
>> 2.35.3
>> 

[PATCH v2 5/7] migration: Dump correct JSON format for nullptr replacement

[Fabiano Rosas]

From: Peter Xu <peterx@redhat.com>

QEMU plays a trick with null pointers inside an array of pointers in a VMSD
field.  See 07d4e69147 ("migration/vmstate: fix array of ptr with
nullptrs") for more details on why.  The idea makes sense in general, but
it may overlooked the JSON writer where it could write nothing in a
"struct" in the JSON hints section.

We hit some analyze-migration.py issues on s390 recently, showing that some
of the struct field contains nothing, like:

{"name": "css", "array_len": 256, "type": "struct", "struct": {}, "size": 1}

As described in details by Fabiano:

https://lore.kernel.org/r/87pll37cin.fsf@suse.de

It could be that we hit some null pointers there, and JSON was gone when
they're null pointers.

To fix it, instead of hacking around only at VMStateInfo level, do that
from VMStateField level, so that JSON writer can also be involved.  In this
case, JSON writer will replace the pointer array (which used to be a
"struct") to be the real representation of the nullptr field.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/vmstate.c | 118 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 91 insertions(+), 27 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index aa2821dec6..52704c822c 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -51,6 +51,36 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
     return result;
 }
 
+/*
+ * Create a fake nullptr field when there's a NULL pointer detected in the
+ * array of a VMS_ARRAY_OF_POINTER VMSD field.  It's needed because we
+ * can't dereference the NULL pointer.
+ */
+static const VMStateField *
+vmsd_create_fake_nullptr_field(const VMStateField *field)
+{
+    VMStateField *fake = g_new0(VMStateField, 1);
+
+    /* It can only happen on an array of pointers! */
+    assert(field->flags & VMS_ARRAY_OF_POINTER);
+
+    /* Some of fake's properties should match the original's */
+    fake->name = field->name;
+    fake->version_id = field->version_id;
+
+    /* Do not need "field_exists" check as it always exists (which is null) */
+    fake->field_exists = NULL;
+
+    /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
+    fake->size = 1;
+    fake->info = &vmstate_info_nullptr;
+    fake->flags = VMS_SINGLE;
+
+    /* All the rest fields shouldn't matter.. */
+
+    return (const VMStateField *)fake;
+}
+
 static int vmstate_n_elems(void *opaque, const VMStateField *field)
 {
     int n_elems = 1;
@@ -143,23 +173,39 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
             }
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
+                const VMStateField *inner_field;
 
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     curr_elem = *(void **)curr_elem;
                 }
+
                 if (!curr_elem && size) {
-                    /* if null pointer check placeholder and do not follow */
-                    assert(field->flags & VMS_ARRAY_OF_POINTER);
-                    ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
-                } else if (field->flags & VMS_STRUCT) {
-                    ret = vmstate_load_state(f, field->vmsd, curr_elem,
-                                             field->vmsd->version_id);
-                } else if (field->flags & VMS_VSTRUCT) {
-                    ret = vmstate_load_state(f, field->vmsd, curr_elem,
-                                             field->struct_version_id);
+                    /*
+                     * If null pointer found (which should only happen in
+                     * an array of pointers), use null placeholder and do
+                     * not follow.
+                     */
+                    inner_field = vmsd_create_fake_nullptr_field(field);
                 } else {
-                    ret = field->info->get(f, curr_elem, size, field);
+                    inner_field = field;
                 }
+
+                if (inner_field->flags & VMS_STRUCT) {
+                    ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
+                                             inner_field->vmsd->version_id);
+                } else if (inner_field->flags & VMS_VSTRUCT) {
+                    ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
+                                             inner_field->struct_version_id);
+                } else {
+                    ret = inner_field->info->get(f, curr_elem, size,
+                                                 inner_field);
+                }
+
+                /* If we used a fake temp field.. free it now */
+                if (inner_field != field) {
+                    g_clear_pointer((gpointer *)&inner_field, g_free);
+                }
+
                 if (ret >= 0) {
                     ret = qemu_file_get_error(f);
                 }
@@ -387,29 +433,50 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
             }
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
+                const VMStateField *inner_field;
 
-                vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
                 old_offset = qemu_file_transferred(f);
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
                 }
+
                 if (!curr_elem && size) {
-                    /* if null pointer write placeholder and do not follow */
-                    assert(field->flags & VMS_ARRAY_OF_POINTER);
-                    ret = vmstate_info_nullptr.put(f, curr_elem, size, NULL,
-                                                   NULL);
-                } else if (field->flags & VMS_STRUCT) {
-                    ret = vmstate_save_state(f, field->vmsd, curr_elem,
-                                             vmdesc_loop);
-                } else if (field->flags & VMS_VSTRUCT) {
-                    ret = vmstate_save_state_v(f, field->vmsd, curr_elem,
-                                               vmdesc_loop,
-                                               field->struct_version_id, errp);
+                    /*
+                     * If null pointer found (which should only happen in
+                     * an array of pointers), use null placeholder and do
+                     * not follow.
+                     */
+                    inner_field = vmsd_create_fake_nullptr_field(field);
                 } else {
-                    ret = field->info->put(f, curr_elem, size, field,
-                                     vmdesc_loop);
+                    inner_field = field;
                 }
+
+                vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
+                                      i, n_elems);
+
+                if (inner_field->flags & VMS_STRUCT) {
+                    ret = vmstate_save_state(f, inner_field->vmsd,
+                                             curr_elem, vmdesc_loop);
+                } else if (inner_field->flags & VMS_VSTRUCT) {
+                    ret = vmstate_save_state_v(f, inner_field->vmsd,
+                                               curr_elem, vmdesc_loop,
+                                               inner_field->struct_version_id,
+                                               errp);
+                } else {
+                    ret = inner_field->info->put(f, curr_elem, size,
+                                                 inner_field, vmdesc_loop);
+                }
+
+                written_bytes = qemu_file_transferred(f) - old_offset;
+                vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
+                                    written_bytes);
+
+                /* If we used a fake temp field.. free it now */
+                if (inner_field != field) {
+                    g_clear_pointer((gpointer *)&inner_field, g_free);
+                }
+
                 if (ret) {
                     error_setg(errp, "Save of field %s/%s failed",
                                 vmsd->name, field->name);
@@ -419,9 +486,6 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                     return ret;
                 }
 
-                written_bytes = qemu_file_transferred(f) - old_offset;
-                vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes);
-
                 /* Compressed arrays only care about the first element */
                 if (vmdesc_loop && vmsd_can_compress(field)) {
                     vmdesc_loop = NULL;
-- 
2.35.3

[PATCH v2 6/7] migration: Fix arrays of pointers in JSON writer

[Fabiano Rosas]

Currently, if an array of pointers contains a NULL pointer, that
pointer will be encoded as '0' in the stream. Since the JSON writer
doesn't define a "pointer" type, that '0' will now be an uint8, which
is different from the original type being pointed to, e.g. struct.

(we're further calling uint8 "nullptr", but that's irrelevant to the
issue)

That mixed-type array shouldn't be compressed, otherwise data is lost
as the code currently makes the whole array have the type of the first
element:

css = {NULL, NULL, ..., 0x5555568a7940, NULL};

{"name": "s390_css", "instance_id": 0, "vmsd_name": "s390_css",
 "version": 1, "fields": [
    ...,
    {"name": "css", "array_len": 256, "type": "nullptr", "size": 1},
    ...,
]}

In the above, the valid pointer at position 254 got lost among the
compressed array of nullptr.

While we could disable the array compression when a NULL pointer is
found, the JSON part of the stream still makes part of downtime, so we
should avoid writing unecessary bytes to it.

Keep the array compression in place, but if NULL and non-NULL pointers
are mixed break the array into several type-contiguous pieces :

css = {NULL, NULL, ..., 0x5555568a7940, NULL};

{"name": "s390_css", "instance_id": 0, "vmsd_name": "s390_css",
 "version": 1, "fields": [
     ...,
     {"name": "css", "array_len": 254, "type": "nullptr", "size": 1},
     {"name": "css", "type": "struct", "struct": {"vmsd_name": "s390_css_img", ... }, "size": 768},
     {"name": "css", "type": "nullptr", "size": 1},
     ...,
]}

Now each type-discontiguous region will become a new JSON entry. The
reader should interpret this as a concatenation of values, all part of
the same field.

Parsing the JSON with analyze-script.py now shows the proper data
being pointed to at the places where the pointer is valid and
"nullptr" where there's NULL:

"s390_css (14)": {
    ...
    "css": [
        "nullptr",
        "nullptr",
        ...
        "nullptr",
        {
            "chpids": [
            {
                "in_use": "0x00",
                "type": "0x00",
                "is_virtual": "0x00"
            },
            ...
            ]
        },
        "nullptr",
    }

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/vmstate.c          | 33 ++++++++++++++++++++++++++++++++-
 scripts/analyze-migration.py | 29 +++++++++++++++++++++--------
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 52704c822c..a79ccf3875 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -425,15 +425,19 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
             int size = vmstate_size(opaque, field);
             uint64_t old_offset, written_bytes;
             JSONWriter *vmdesc_loop = vmdesc;
+            bool is_prev_null = false;
 
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
                 assert(first_elem || !n_elems || !size);
             }
+
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
                 const VMStateField *inner_field;
+                bool is_null;
+                int max_elems = n_elems - i;
 
                 old_offset = qemu_file_transferred(f);
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
@@ -448,12 +452,39 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                      * not follow.
                      */
                     inner_field = vmsd_create_fake_nullptr_field(field);
+                    is_null = true;
                 } else {
                     inner_field = field;
+                    is_null = false;
+                }
+
+                /*
+                 * Due to the fake nullptr handling above, if there's mixed
+                 * null/non-null data, it doesn't make sense to emit a
+                 * compressed array representation spanning the entire array
+                 * because the field types will be different (e.g. struct
+                 * vs. uint64_t). Search ahead for the next null/non-null
+                 * element and start a new compressed array if found.
+                 */
+                if (field->flags & VMS_ARRAY_OF_POINTER &&
+                    is_null != is_prev_null) {
+
+                    is_prev_null = is_null;
+                    vmdesc_loop = vmdesc;
+
+                    for (int j = i + 1; j < n_elems; j++) {
+                        void *elem = *(void **)(first_elem + size * j);
+                        bool elem_is_null = !elem && size;
+
+                        if (is_null != elem_is_null) {
+                            max_elems = j - i;
+                            break;
+                        }
+                    }
                 }
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
-                                      i, n_elems);
+                                      i, max_elems);
 
                 if (inner_field->flags & VMS_STRUCT) {
                     ret = vmstate_save_state(f, inner_field->vmsd,
diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 134c25f20a..7f0797308d 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -501,15 +501,28 @@ def read(self):
             field['data'] = reader(field, self.file)
             field['data'].read()
 
-            if 'index' in field:
-                if field['name'] not in self.data:
-                    self.data[field['name']] = []
-                a = self.data[field['name']]
-                if len(a) != int(field['index']):
-                    raise Exception("internal index of data field unmatched (%d/%d)" % (len(a), int(field['index'])))
-                a.append(field['data'])
+            fname = field['name']
+            fdata = field['data']
+
+            # The field could be:
+            # i) a single data entry, e.g. uint64
+            # ii) an array, indicated by it containing the 'index' key
+            #
+            # However, the overall data after parsing the whole
+            # stream, could be a mix of arrays and single data fields,
+            # all sharing the same field name due to how QEMU breaks
+            # up arrays with NULL pointers into multiple compressed
+            # array segments.
+            if fname not in self.data:
+                if 'index' in field:
+                    self.data[fname] = []
+                else:
+                    self.data[fname] = fdata
+            elif type(self.data[fname]) == list:
+                self.data[fname].append(fdata)
             else:
-                self.data[field['name']] = field['data']
+                tmp = self.data[fname]
+                self.data[fname] = [tmp, fdata]
 
         if 'subsections' in self.desc['struct']:
             for subsection in self.desc['struct']['subsections']:
-- 
2.35.3

Re: [PATCH v2 6/7] migration: Fix arrays of pointers in JSON writer

[Peter Xu]

On Thu, Jan 09, 2025 at 11:09:58AM -0300, Fabiano Rosas wrote:
> Currently, if an array of pointers contains a NULL pointer, that
> pointer will be encoded as '0' in the stream. Since the JSON writer
> doesn't define a "pointer" type, that '0' will now be an uint8, which
> is different from the original type being pointed to, e.g. struct.
> 
> (we're further calling uint8 "nullptr", but that's irrelevant to the
> issue)
> 
> That mixed-type array shouldn't be compressed, otherwise data is lost
> as the code currently makes the whole array have the type of the first
> element:
> 
> css = {NULL, NULL, ..., 0x5555568a7940, NULL};
> 
> {"name": "s390_css", "instance_id": 0, "vmsd_name": "s390_css",
>  "version": 1, "fields": [
>     ...,
>     {"name": "css", "array_len": 256, "type": "nullptr", "size": 1},
>     ...,
> ]}
> 
> In the above, the valid pointer at position 254 got lost among the
> compressed array of nullptr.
> 
> While we could disable the array compression when a NULL pointer is
> found, the JSON part of the stream still makes part of downtime, so we
> should avoid writing unecessary bytes to it.
> 
> Keep the array compression in place, but if NULL and non-NULL pointers
> are mixed break the array into several type-contiguous pieces :
> 
> css = {NULL, NULL, ..., 0x5555568a7940, NULL};
> 
> {"name": "s390_css", "instance_id": 0, "vmsd_name": "s390_css",
>  "version": 1, "fields": [
>      ...,
>      {"name": "css", "array_len": 254, "type": "nullptr", "size": 1},
>      {"name": "css", "type": "struct", "struct": {"vmsd_name": "s390_css_img", ... }, "size": 768},
>      {"name": "css", "type": "nullptr", "size": 1},
>      ...,
> ]}
> 
> Now each type-discontiguous region will become a new JSON entry. The
> reader should interpret this as a concatenation of values, all part of
> the same field.
> 
> Parsing the JSON with analyze-script.py now shows the proper data
> being pointed to at the places where the pointer is valid and
> "nullptr" where there's NULL:
> 
> "s390_css (14)": {
>     ...
>     "css": [
>         "nullptr",
>         "nullptr",
>         ...
>         "nullptr",
>         {
>             "chpids": [
>             {
>                 "in_use": "0x00",
>                 "type": "0x00",
>                 "is_virtual": "0x00"
>             },
>             ...
>             ]
>         },
>         "nullptr",
>     }
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/vmstate.c          | 33 ++++++++++++++++++++++++++++++++-
>  scripts/analyze-migration.py | 29 +++++++++++++++++++++--------
>  2 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 52704c822c..a79ccf3875 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -425,15 +425,19 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>              int size = vmstate_size(opaque, field);
>              uint64_t old_offset, written_bytes;
>              JSONWriter *vmdesc_loop = vmdesc;
> +            bool is_prev_null = false;
>  
>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>              if (field->flags & VMS_POINTER) {
>                  first_elem = *(void **)first_elem;
>                  assert(first_elem || !n_elems || !size);
>              }
> +
>              for (i = 0; i < n_elems; i++) {
>                  void *curr_elem = first_elem + size * i;
>                  const VMStateField *inner_field;
> +                bool is_null;
> +                int max_elems = n_elems - i;
>  
>                  old_offset = qemu_file_transferred(f);
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
> @@ -448,12 +452,39 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                       * not follow.
>                       */
>                      inner_field = vmsd_create_fake_nullptr_field(field);
> +                    is_null = true;
>                  } else {
>                      inner_field = field;
> +                    is_null = false;
> +                }
> +
> +                /*
> +                 * Due to the fake nullptr handling above, if there's mixed
> +                 * null/non-null data, it doesn't make sense to emit a
> +                 * compressed array representation spanning the entire array
> +                 * because the field types will be different (e.g. struct
> +                 * vs. uint64_t). Search ahead for the next null/non-null

s/uint64_t/nullptr/

> +                 * element and start a new compressed array if found.
> +                 */
> +                if (field->flags & VMS_ARRAY_OF_POINTER &&
> +                    is_null != is_prev_null) {
> +
> +                    is_prev_null = is_null;
> +                    vmdesc_loop = vmdesc;
> +
> +                    for (int j = i + 1; j < n_elems; j++) {
> +                        void *elem = *(void **)(first_elem + size * j);
> +                        bool elem_is_null = !elem && size;
> +
> +                        if (is_null != elem_is_null) {
> +                            max_elems = j - i;
> +                            break;
> +                        }
> +                    }
>                  }

This definitely adds some slight more difficulty on reading this
code.. Let's have this first.

If it's proved that the JSON is a measurable overhead, I think we may
consider turn that off by default even for precopy (postcopy never used
vmdesc), then maybe we can simplify this chunk (and probably the whole
compression idea, because when turned on it is for debugging).

>  
>                  vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
> -                                      i, n_elems);
> +                                      i, max_elems);
>  
>                  if (inner_field->flags & VMS_STRUCT) {
>                      ret = vmstate_save_state(f, inner_field->vmsd,
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index 134c25f20a..7f0797308d 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -501,15 +501,28 @@ def read(self):
>              field['data'] = reader(field, self.file)
>              field['data'].read()
>  
> -            if 'index' in field:
> -                if field['name'] not in self.data:
> -                    self.data[field['name']] = []
> -                a = self.data[field['name']]
> -                if len(a) != int(field['index']):
> -                    raise Exception("internal index of data field unmatched (%d/%d)" % (len(a), int(field['index'])))
> -                a.append(field['data'])
> +            fname = field['name']
> +            fdata = field['data']
> +
> +            # The field could be:
> +            # i) a single data entry, e.g. uint64
> +            # ii) an array, indicated by it containing the 'index' key
> +            #
> +            # However, the overall data after parsing the whole
> +            # stream, could be a mix of arrays and single data fields,
> +            # all sharing the same field name due to how QEMU breaks
> +            # up arrays with NULL pointers into multiple compressed
> +            # array segments.
> +            if fname not in self.data:
> +                if 'index' in field:
> +                    self.data[fname] = []

This needs to be:

     self.data[fname] = [fdata]

?

Btw, since the new code will process it correctly with non-array below,
IIUC here we can make it simple:

  if 'index' in field:
      self.data[fname] = fdata

> +                else:
> +                    self.data[fname] = fdata
> +            elif type(self.data[fname]) == list:
> +                self.data[fname].append(fdata)
>              else:
> -                self.data[field['name']] = field['data']
> +                tmp = self.data[fname]
> +                self.data[fname] = [tmp, fdata]
>  
>          if 'subsections' in self.desc['struct']:
>              for subsection in self.desc['struct']['subsections']:
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH v2 6/7] migration: Fix arrays of pointers in JSON writer

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jan 09, 2025 at 11:09:58AM -0300, Fabiano Rosas wrote:
>> Currently, if an array of pointers contains a NULL pointer, that
>> pointer will be encoded as '0' in the stream. Since the JSON writer
>> doesn't define a "pointer" type, that '0' will now be an uint8, which
>> is different from the original type being pointed to, e.g. struct.
>> 
>> (we're further calling uint8 "nullptr", but that's irrelevant to the
>> issue)
>> 
>> That mixed-type array shouldn't be compressed, otherwise data is lost
>> as the code currently makes the whole array have the type of the first
>> element:
>> 
>> css = {NULL, NULL, ..., 0x5555568a7940, NULL};
>> 
>> {"name": "s390_css", "instance_id": 0, "vmsd_name": "s390_css",
>>  "version": 1, "fields": [
>>     ...,
>>     {"name": "css", "array_len": 256, "type": "nullptr", "size": 1},
>>     ...,
>> ]}
>> 
>> In the above, the valid pointer at position 254 got lost among the
>> compressed array of nullptr.
>> 
>> While we could disable the array compression when a NULL pointer is
>> found, the JSON part of the stream still makes part of downtime, so we
>> should avoid writing unecessary bytes to it.
>> 
>> Keep the array compression in place, but if NULL and non-NULL pointers
>> are mixed break the array into several type-contiguous pieces :
>> 
>> css = {NULL, NULL, ..., 0x5555568a7940, NULL};
>> 
>> {"name": "s390_css", "instance_id": 0, "vmsd_name": "s390_css",
>>  "version": 1, "fields": [
>>      ...,
>>      {"name": "css", "array_len": 254, "type": "nullptr", "size": 1},
>>      {"name": "css", "type": "struct", "struct": {"vmsd_name": "s390_css_img", ... }, "size": 768},
>>      {"name": "css", "type": "nullptr", "size": 1},
>>      ...,
>> ]}
>> 
>> Now each type-discontiguous region will become a new JSON entry. The
>> reader should interpret this as a concatenation of values, all part of
>> the same field.
>> 
>> Parsing the JSON with analyze-script.py now shows the proper data
>> being pointed to at the places where the pointer is valid and
>> "nullptr" where there's NULL:
>> 
>> "s390_css (14)": {
>>     ...
>>     "css": [
>>         "nullptr",
>>         "nullptr",
>>         ...
>>         "nullptr",
>>         {
>>             "chpids": [
>>             {
>>                 "in_use": "0x00",
>>                 "type": "0x00",
>>                 "is_virtual": "0x00"
>>             },
>>             ...
>>             ]
>>         },
>>         "nullptr",
>>     }
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/vmstate.c          | 33 ++++++++++++++++++++++++++++++++-
>>  scripts/analyze-migration.py | 29 +++++++++++++++++++++--------
>>  2 files changed, 53 insertions(+), 9 deletions(-)
>> 
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 52704c822c..a79ccf3875 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -425,15 +425,19 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>              int size = vmstate_size(opaque, field);
>>              uint64_t old_offset, written_bytes;
>>              JSONWriter *vmdesc_loop = vmdesc;
>> +            bool is_prev_null = false;
>>  
>>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>>              if (field->flags & VMS_POINTER) {
>>                  first_elem = *(void **)first_elem;
>>                  assert(first_elem || !n_elems || !size);
>>              }
>> +
>>              for (i = 0; i < n_elems; i++) {
>>                  void *curr_elem = first_elem + size * i;
>>                  const VMStateField *inner_field;
>> +                bool is_null;
>> +                int max_elems = n_elems - i;
>>  
>>                  old_offset = qemu_file_transferred(f);
>>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>> @@ -448,12 +452,39 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>                       * not follow.
>>                       */
>>                      inner_field = vmsd_create_fake_nullptr_field(field);
>> +                    is_null = true;
>>                  } else {
>>                      inner_field = field;
>> +                    is_null = false;
>> +                }
>> +
>> +                /*
>> +                 * Due to the fake nullptr handling above, if there's mixed
>> +                 * null/non-null data, it doesn't make sense to emit a
>> +                 * compressed array representation spanning the entire array
>> +                 * because the field types will be different (e.g. struct
>> +                 * vs. uint64_t). Search ahead for the next null/non-null
>
> s/uint64_t/nullptr/
>

ok

>> +                 * element and start a new compressed array if found.
>> +                 */
>> +                if (field->flags & VMS_ARRAY_OF_POINTER &&
>> +                    is_null != is_prev_null) {
>> +
>> +                    is_prev_null = is_null;
>> +                    vmdesc_loop = vmdesc;
>> +
>> +                    for (int j = i + 1; j < n_elems; j++) {
>> +                        void *elem = *(void **)(first_elem + size * j);
>> +                        bool elem_is_null = !elem && size;
>> +
>> +                        if (is_null != elem_is_null) {
>> +                            max_elems = j - i;
>> +                            break;
>> +                        }
>> +                    }
>>                  }
>
> This definitely adds some slight more difficulty on reading this
> code.. Let's have this first.
>
> If it's proved that the JSON is a measurable overhead, I think we may
> consider turn that off by default even for precopy (postcopy never used
> vmdesc), then maybe we can simplify this chunk (and probably the whole
> compression idea, because when turned on it is for debugging).
>
>>  
>>                  vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
>> -                                      i, n_elems);
>> +                                      i, max_elems);
>>  
>>                  if (inner_field->flags & VMS_STRUCT) {
>>                      ret = vmstate_save_state(f, inner_field->vmsd,
>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
>> index 134c25f20a..7f0797308d 100755
>> --- a/scripts/analyze-migration.py
>> +++ b/scripts/analyze-migration.py
>> @@ -501,15 +501,28 @@ def read(self):
>>              field['data'] = reader(field, self.file)
>>              field['data'].read()
>>  
>> -            if 'index' in field:
>> -                if field['name'] not in self.data:
>> -                    self.data[field['name']] = []
>> -                a = self.data[field['name']]
>> -                if len(a) != int(field['index']):
>> -                    raise Exception("internal index of data field unmatched (%d/%d)" % (len(a), int(field['index'])))
>> -                a.append(field['data'])
>> +            fname = field['name']
>> +            fdata = field['data']
>> +
>> +            # The field could be:
>> +            # i) a single data entry, e.g. uint64
>> +            # ii) an array, indicated by it containing the 'index' key
>> +            #
>> +            # However, the overall data after parsing the whole
>> +            # stream, could be a mix of arrays and single data fields,
>> +            # all sharing the same field name due to how QEMU breaks
>> +            # up arrays with NULL pointers into multiple compressed
>> +            # array segments.
>> +            if fname not in self.data:
>> +                if 'index' in field:
>> +                    self.data[fname] = []
>
> This needs to be:
>
>      self.data[fname] = [fdata]
>
> ?

Yes.

>
> Btw, since the new code will process it correctly with non-array below,
> IIUC here we can make it simple:
>
>   if 'index' in field:
>       self.data[fname] = fdata
>

Sorry, I don't understand what you mean here. I changed it now to:

    if fname not in self.data:
        if 'index' in field:
            self.data[fname] = [fdata]
        else:
            self.data[fname] = fdata
    elif type(self.data[fname]) == list:
        self.data[fname].append(fdata)
    else:
        tmp = self.data[fname]
        self.data[fname] = [tmp, fdata]

>> +                else:
>> +                    self.data[fname] = fdata
>> +            elif type(self.data[fname]) == list:
>> +                self.data[fname].append(fdata)
>>              else:
>> -                self.data[field['name']] = field['data']
>> +                tmp = self.data[fname]
>> +                self.data[fname] = [tmp, fdata]
>>  
>>          if 'subsections' in self.desc['struct']:
>>              for subsection in self.desc['struct']['subsections']:
>> -- 
>> 2.35.3
>> 

Re: [PATCH v2 6/7] migration: Fix arrays of pointers in JSON writer

[Peter Xu]

On Thu, Jan 09, 2025 at 01:16:37PM -0300, Fabiano Rosas wrote:
> > Btw, since the new code will process it correctly with non-array below,
> > IIUC here we can make it simple:
> >
> >   if 'index' in field:
> >       self.data[fname] = fdata
> >
> 
> Sorry, I don't understand what you mean here. I changed it now to:
> 
>     if fname not in self.data:
>         if 'index' in field:
>             self.data[fname] = [fdata]
>         else:
>             self.data[fname] = fdata
>     elif type(self.data[fname]) == list:
>         self.data[fname].append(fdata)
>     else:
>         tmp = self.data[fname]
>         self.data[fname] = [tmp, fdata]

I meant we could avoid checking "index" completely now with the new code
knowing how to expand, so IIUC it can be simplified to:

  if fname not in self.data:
      self.data[fname] = fdata
  elif type(self.data[fname]) == list:
      self.data[fname].append(fdata)
  else:
      tmp = self.data[fname]
      self.data[fname] = [tmp, fdata]

-- 
Peter Xu

Re: [PATCH v2 6/7] migration: Fix arrays of pointers in JSON writer

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jan 09, 2025 at 01:16:37PM -0300, Fabiano Rosas wrote:
>> > Btw, since the new code will process it correctly with non-array below,
>> > IIUC here we can make it simple:
>> >
>> >   if 'index' in field:
>> >       self.data[fname] = fdata
>> >
>> 
>> Sorry, I don't understand what you mean here. I changed it now to:
>> 
>>     if fname not in self.data:
>>         if 'index' in field:
>>             self.data[fname] = [fdata]
>>         else:
>>             self.data[fname] = fdata
>>     elif type(self.data[fname]) == list:
>>         self.data[fname].append(fdata)
>>     else:
>>         tmp = self.data[fname]
>>         self.data[fname] = [tmp, fdata]
>
> I meant we could avoid checking "index" completely now with the new code
> knowing how to expand, so IIUC it can be simplified to:
>
>   if fname not in self.data:
>       self.data[fname] = fdata
>   elif type(self.data[fname]) == list:
>       self.data[fname].append(fdata)
>   else:
>       tmp = self.data[fname]
>       self.data[fname] = [tmp, fdata]

Good point, I'll change that. Thanks

[PATCH v2 7/7] s390x: Fix CSS migration

[Fabiano Rosas]

Commit a55ae46683 ("s390: move css_migration_enabled from machine to
css.c") disabled CSS migration globally instead of doing it
per-instance.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: qemu-stable@nongnu.org #9.1
Fixes: a55ae46683 ("s390: move css_migration_enabled from machine to css.c")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2704
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Message-Id: <20241213160120.23880-3-farosas@suse.de>
---
 hw/s390x/s390-virtio-ccw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8a242cc1ec..38aeba14ee 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -1244,6 +1244,7 @@ static void ccw_machine_2_9_instance_options(MachineState *machine)
     s390_cpudef_featoff_greater(12, 1, S390_FEAT_ZPCI);
     s390_cpudef_featoff_greater(12, 1, S390_FEAT_ADAPTER_INT_SUPPRESSION);
     s390_cpudef_featoff_greater(12, 1, S390_FEAT_ADAPTER_EVENT_NOTIFICATION);
+    css_migration_enabled = false;
 }
 
 static void ccw_machine_2_9_class_options(MachineClass *mc)
@@ -1256,7 +1257,6 @@ static void ccw_machine_2_9_class_options(MachineClass *mc)
     ccw_machine_2_10_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_2_9, hw_compat_2_9_len);
     compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
-    css_migration_enabled = false;
 }
 DEFINE_CCW_MACHINE(2, 9);
 
-- 
2.35.3