diff mbox series

[V1,07/26] migration: VMStateId

Message ID 1714406135-451286-8-git-send-email-steven.sistare@oracle.com (mailing list archive)
State New, archived
Headers show
Series Live update: cpr-exec | expand

Commit Message

Steven Sistare April 29, 2024, 3:55 p.m. UTC
Define a type for the 256 byte id string to guarantee the same length is
used and enforced everywhere.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 include/exec/ramblock.h     | 3 ++-
 include/migration/vmstate.h | 2 ++
 migration/savevm.c          | 8 ++++----
 migration/vmstate.c         | 3 ++-
 4 files changed, 10 insertions(+), 6 deletions(-)

Comments

Fabiano Rosas May 7, 2024, 9:03 p.m. UTC | #1
Steve Sistare <steven.sistare@oracle.com> writes:

> Define a type for the 256 byte id string to guarantee the same length is
> used and enforced everywhere.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Peter Xu May 27, 2024, 6:20 p.m. UTC | #2
On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote:
> Define a type for the 256 byte id string to guarantee the same length is
> used and enforced everywhere.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  include/exec/ramblock.h     | 3 ++-
>  include/migration/vmstate.h | 2 ++
>  migration/savevm.c          | 8 ++++----
>  migration/vmstate.c         | 3 ++-
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> index 0babd10..61deefe 100644
> --- a/include/exec/ramblock.h
> +++ b/include/exec/ramblock.h
> @@ -23,6 +23,7 @@
>  #include "cpu-common.h"
>  #include "qemu/rcu.h"
>  #include "exec/ramlist.h"
> +#include "migration/vmstate.h"
>  
>  struct RAMBlock {
>      struct rcu_head rcu;
> @@ -35,7 +36,7 @@ struct RAMBlock {
>      void (*resized)(const char*, uint64_t length, void *host);
>      uint32_t flags;
>      /* Protected by the BQL.  */
> -    char idstr[256];
> +    VMStateId idstr;
>      /* RCU-enabled, writes protected by the ramlist lock */
>      QLIST_ENTRY(RAMBlock) next;
>      QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;

Hmm.. Don't look like a good idea to include a migration header in
ramblock.h?  Is this ramblock change needed for this work?
Steven Sistare May 28, 2024, 3:10 p.m. UTC | #3
On 5/27/2024 2:20 PM, Peter Xu wrote:
> On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote:
>> Define a type for the 256 byte id string to guarantee the same length is
>> used and enforced everywhere.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   include/exec/ramblock.h     | 3 ++-
>>   include/migration/vmstate.h | 2 ++
>>   migration/savevm.c          | 8 ++++----
>>   migration/vmstate.c         | 3 ++-
>>   4 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
>> index 0babd10..61deefe 100644
>> --- a/include/exec/ramblock.h
>> +++ b/include/exec/ramblock.h
>> @@ -23,6 +23,7 @@
>>   #include "cpu-common.h"
>>   #include "qemu/rcu.h"
>>   #include "exec/ramlist.h"
>> +#include "migration/vmstate.h"
>>   
>>   struct RAMBlock {
>>       struct rcu_head rcu;
>> @@ -35,7 +36,7 @@ struct RAMBlock {
>>       void (*resized)(const char*, uint64_t length, void *host);
>>       uint32_t flags;
>>       /* Protected by the BQL.  */
>> -    char idstr[256];
>> +    VMStateId idstr;
>>       /* RCU-enabled, writes protected by the ramlist lock */
>>       QLIST_ENTRY(RAMBlock) next;
>>       QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
> 
> Hmm.. Don't look like a good idea to include a migration header in
> ramblock.h?  Is this ramblock change needed for this work?

Well, entities that are migrated include migration headers, and now that
includes RAMBlock.  There is precedent:

0 include/exec/ramblock.h   26 #include "migration/vmstate.h"
1 include/hw/acpi/ich9_tco. 14 #include "migration/vmstate.h"
2 include/hw/display/ramfb.  4 #include "migration/vmstate.h"
3 include/hw/hyperv/vmbus.h 16 #include "migration/vmstate.h"
4 include/hw/input/pl050.h  14 #include "migration/vmstate.h"
5 include/hw/pci/shpc.h      7 #include "migration/vmstate.h"
6 include/hw/virtio/virtio. 20 #include "migration/vmstate.h"
7 include/migration/cpu.h    8 #include "migration/vmstate.h"

Granted, only some of the C files that include ramblock.h need all of vmstate.h.
I could define VMStateId in a smaller file such as migration/misc.h, or a
new file migration/vmstateid.h, and include that in ramblock.h.
Any preference?

- Steve
Peter Xu May 28, 2024, 5:44 p.m. UTC | #4
On Tue, May 28, 2024 at 11:10:03AM -0400, Steven Sistare via wrote:
> On 5/27/2024 2:20 PM, Peter Xu wrote:
> > On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote:
> > > Define a type for the 256 byte id string to guarantee the same length is
> > > used and enforced everywhere.
> > > 
> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > > ---
> > >   include/exec/ramblock.h     | 3 ++-
> > >   include/migration/vmstate.h | 2 ++
> > >   migration/savevm.c          | 8 ++++----
> > >   migration/vmstate.c         | 3 ++-
> > >   4 files changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> > > index 0babd10..61deefe 100644
> > > --- a/include/exec/ramblock.h
> > > +++ b/include/exec/ramblock.h
> > > @@ -23,6 +23,7 @@
> > >   #include "cpu-common.h"
> > >   #include "qemu/rcu.h"
> > >   #include "exec/ramlist.h"
> > > +#include "migration/vmstate.h"
> > >   struct RAMBlock {
> > >       struct rcu_head rcu;
> > > @@ -35,7 +36,7 @@ struct RAMBlock {
> > >       void (*resized)(const char*, uint64_t length, void *host);
> > >       uint32_t flags;
> > >       /* Protected by the BQL.  */
> > > -    char idstr[256];
> > > +    VMStateId idstr;
> > >       /* RCU-enabled, writes protected by the ramlist lock */
> > >       QLIST_ENTRY(RAMBlock) next;
> > >       QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
> > 
> > Hmm.. Don't look like a good idea to include a migration header in
> > ramblock.h?  Is this ramblock change needed for this work?
> 
> Well, entities that are migrated include migration headers, and now that
> includes RAMBlock.  There is precedent:
> 
> 0 include/exec/ramblock.h   26 #include "migration/vmstate.h"
> 1 include/hw/acpi/ich9_tco. 14 #include "migration/vmstate.h"
> 2 include/hw/display/ramfb.  4 #include "migration/vmstate.h"
> 3 include/hw/hyperv/vmbus.h 16 #include "migration/vmstate.h"
> 4 include/hw/input/pl050.h  14 #include "migration/vmstate.h"
> 5 include/hw/pci/shpc.h      7 #include "migration/vmstate.h"
> 6 include/hw/virtio/virtio. 20 #include "migration/vmstate.h"
> 7 include/migration/cpu.h    8 #include "migration/vmstate.h"
> 
> Granted, only some of the C files that include ramblock.h need all of vmstate.h.
> I could define VMStateId in a smaller file such as migration/misc.h, or a
> new file migration/vmstateid.h, and include that in ramblock.h.
> Any preference?

One issue here is currently the idstr[] of ramblock is a verbose name of
the ramblock, and logically it doesn't need to have anything to do with
vmstate.

I'll continue to read to see why we need VMStateID here for a ramblock.  So
if it is necessary, maybe the name VMStateID to be used here is misleading?
It'll also be nice to separate the changes, as ramblock.h doesn't belong to
migration subsystem but memory api.

Thanks,
Steven Sistare May 29, 2024, 5:30 p.m. UTC | #5
On 5/28/2024 1:44 PM, Peter Xu wrote:
> On Tue, May 28, 2024 at 11:10:03AM -0400, Steven Sistare via wrote:
>> On 5/27/2024 2:20 PM, Peter Xu wrote:
>>> On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote:
>>>> Define a type for the 256 byte id string to guarantee the same length is
>>>> used and enforced everywhere.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>> ---
>>>>    include/exec/ramblock.h     | 3 ++-
>>>>    include/migration/vmstate.h | 2 ++
>>>>    migration/savevm.c          | 8 ++++----
>>>>    migration/vmstate.c         | 3 ++-
>>>>    4 files changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
>>>> index 0babd10..61deefe 100644
>>>> --- a/include/exec/ramblock.h
>>>> +++ b/include/exec/ramblock.h
>>>> @@ -23,6 +23,7 @@
>>>>    #include "cpu-common.h"
>>>>    #include "qemu/rcu.h"
>>>>    #include "exec/ramlist.h"
>>>> +#include "migration/vmstate.h"
>>>>    struct RAMBlock {
>>>>        struct rcu_head rcu;
>>>> @@ -35,7 +36,7 @@ struct RAMBlock {
>>>>        void (*resized)(const char*, uint64_t length, void *host);
>>>>        uint32_t flags;
>>>>        /* Protected by the BQL.  */
>>>> -    char idstr[256];
>>>> +    VMStateId idstr;
>>>>        /* RCU-enabled, writes protected by the ramlist lock */
>>>>        QLIST_ENTRY(RAMBlock) next;
>>>>        QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
>>>
>>> Hmm.. Don't look like a good idea to include a migration header in
>>> ramblock.h?  Is this ramblock change needed for this work?
>>
>> Well, entities that are migrated include migration headers, and now that
>> includes RAMBlock.  There is precedent:
>>
>> 0 include/exec/ramblock.h   26 #include "migration/vmstate.h"
>> 1 include/hw/acpi/ich9_tco. 14 #include "migration/vmstate.h"
>> 2 include/hw/display/ramfb.  4 #include "migration/vmstate.h"
>> 3 include/hw/hyperv/vmbus.h 16 #include "migration/vmstate.h"
>> 4 include/hw/input/pl050.h  14 #include "migration/vmstate.h"
>> 5 include/hw/pci/shpc.h      7 #include "migration/vmstate.h"
>> 6 include/hw/virtio/virtio. 20 #include "migration/vmstate.h"
>> 7 include/migration/cpu.h    8 #include "migration/vmstate.h"
>>
>> Granted, only some of the C files that include ramblock.h need all of vmstate.h.
>> I could define VMStateId in a smaller file such as migration/misc.h, or a
>> new file migration/vmstateid.h, and include that in ramblock.h.
>> Any preference?
> 
> One issue here is currently the idstr[] of ramblock is a verbose name of
> the ramblock, and logically it doesn't need to have anything to do with
> vmstate.
> 
> I'll continue to read to see why we need VMStateID here for a ramblock.  So
> if it is necessary, maybe the name VMStateID to be used here is misleading?
> It'll also be nice to separate the changes, as ramblock.h doesn't belong to
> migration subsystem but memory api.

cpr requires migrating vmstate for ramblock.  See the physmem patches for
why. vmstate requires a unique id, and ramblock already defines a unique
id which is used to identify the block and dirty bitmap in the migration
stream.

How about a more general name for the type:

migration/misc.h
     typedef char (MigrationId)[256];

exec/ramblock.h
     struct RAMBlock {
         MigrationId idstr;

migration/savevm.c
     typedef struct CompatEntry {
         MigrationId idstr;

     typedef struct SaveStateEntry {
         MigrationId idstr;


- Steve
Peter Xu May 29, 2024, 6:53 p.m. UTC | #6
On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote:
> How about a more general name for the type:
> 
> migration/misc.h
>     typedef char (MigrationId)[256];

How about qemu/typedefs.h?  Not sure whether it's applicable. Markus (in
the loop) may have a better idea.

Meanwhile, s/MigrationID/IDString/?

> 
> exec/ramblock.h
>     struct RAMBlock {
>         MigrationId idstr;
> 
> migration/savevm.c
>     typedef struct CompatEntry {
>         MigrationId idstr;
> 
>     typedef struct SaveStateEntry {
>         MigrationId idstr;
> 
> 
> - Steve
>
Steven Sistare May 30, 2024, 5:11 p.m. UTC | #7
On 5/29/2024 2:53 PM, Peter Xu wrote:
> On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote:
>> How about a more general name for the type:
>>
>> migration/misc.h
>>      typedef char (MigrationId)[256];
> 
> How about qemu/typedefs.h?  Not sure whether it's applicable. Markus (in
> the loop) may have a better idea.
> 
> Meanwhile, s/MigrationID/IDString/?

typedefs.h has a different purpose; giving short names to types
defined in internal include files.

This id is specific to migration, so I still think its name should reflect
migration and it belongs in some include/migration/*.h file.

ramblocks and migration are already closely related.  There is nothing wrong
with including a migration header in ramblock.h so it can use a migration type.
We already have:
   include/hw/acpi/ich9_tco.h:#include "migration/vmstate.h"
   include/hw/display/ramfb.h:#include "migration/vmstate.h"
   include/hw/input/pl050.h:#include "migration/vmstate.h"
   include/hw/pci/shpc.h:#include "migration/vmstate.h"
   include/hw/virtio/virtio.h:#include "migration/vmstate.h"
   include/hw/hyperv/vmbus.h:#include "migration/vmstate.h"

The 256 byte magic length already appears in too many places, and my code
would add more occurrences, so I really think that abstracting this type
would be cleaner.

- Steve

>> exec/ramblock.h
>>      struct RAMBlock {
>>          MigrationId idstr;
>>
>> migration/savevm.c
>>      typedef struct CompatEntry {
>>          MigrationId idstr;
>>
>>      typedef struct SaveStateEntry {
>>          MigrationId idstr;
>>
>>
>> - Steve
>>
>
Peter Xu May 30, 2024, 6:03 p.m. UTC | #8
On Thu, May 30, 2024 at 01:11:26PM -0400, Steven Sistare wrote:
> On 5/29/2024 2:53 PM, Peter Xu wrote:
> > On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote:
> > > How about a more general name for the type:
> > > 
> > > migration/misc.h
> > >      typedef char (MigrationId)[256];
> > 
> > How about qemu/typedefs.h?  Not sure whether it's applicable. Markus (in
> > the loop) may have a better idea.
> > 
> > Meanwhile, s/MigrationID/IDString/?
> 
> typedefs.h has a different purpose; giving short names to types
> defined in internal include files.
> 
> This id is specific to migration, so I still think its name should reflect
> migration and it belongs in some include/migration/*.h file.
> 
> ramblocks and migration are already closely related.  There is nothing wrong
> with including a migration header in ramblock.h so it can use a migration type.
> We already have:
>   include/hw/acpi/ich9_tco.h:#include "migration/vmstate.h"
>   include/hw/display/ramfb.h:#include "migration/vmstate.h"
>   include/hw/input/pl050.h:#include "migration/vmstate.h"
>   include/hw/pci/shpc.h:#include "migration/vmstate.h"
>   include/hw/virtio/virtio.h:#include "migration/vmstate.h"
>   include/hw/hyperv/vmbus.h:#include "migration/vmstate.h"
> 
> The 256 byte magic length already appears in too many places, and my code
> would add more occurrences, so I really think that abstracting this type
> would be cleaner.

I agree having a typedef is nicer, but I don't understand why it must be
migration related.  It can be the type QEMU uses to represent any string
based ID, and that's a generic concept to me.

Migration can have a wrapper to process that type, but then migration will
include the generic header in that case, it feels more natural that way?
diff mbox series

Patch

diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
index 0babd10..61deefe 100644
--- a/include/exec/ramblock.h
+++ b/include/exec/ramblock.h
@@ -23,6 +23,7 @@ 
 #include "cpu-common.h"
 #include "qemu/rcu.h"
 #include "exec/ramlist.h"
+#include "migration/vmstate.h"
 
 struct RAMBlock {
     struct rcu_head rcu;
@@ -35,7 +36,7 @@  struct RAMBlock {
     void (*resized)(const char*, uint64_t length, void *host);
     uint32_t flags;
     /* Protected by the BQL.  */
-    char idstr[256];
+    VMStateId idstr;
     /* RCU-enabled, writes protected by the ramlist lock */
     QLIST_ENTRY(RAMBlock) next;
     QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 4691334..a39c0e6 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1210,6 +1210,8 @@  int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 
 bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque);
 
+typedef char (VMStateId)[256];
+
 #define  VMSTATE_INSTANCE_ID_ANY  -1
 
 /* Returns: 0 on success, -1 on failure */
diff --git a/migration/savevm.c b/migration/savevm.c
index a30bcd9..9b1a335 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -197,13 +197,13 @@  const VMStateInfo vmstate_info_timer = {
 
 
 typedef struct CompatEntry {
-    char idstr[256];
+    VMStateId idstr;
     int instance_id;
 } CompatEntry;
 
 typedef struct SaveStateEntry {
     QTAILQ_ENTRY(SaveStateEntry) entry;
-    char idstr[256];
+    VMStateId idstr;
     uint32_t instance_id;
     int alias_id;
     int version_id;
@@ -814,7 +814,7 @@  int register_savevm_live(const char *idstr,
 void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque)
 {
     SaveStateEntry *se, *new_se;
-    char id[256] = "";
+    VMStateId id = "";
 
     if (obj) {
         char *oid = vmstate_if_get_id(obj);
@@ -2650,7 +2650,7 @@  qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
     uint32_t instance_id, version_id, section_id;
     int64_t start_ts, end_ts;
     SaveStateEntry *se;
-    char idstr[256];
+    VMStateId idstr;
     int ret;
 
     /* Read section start */
diff --git a/migration/vmstate.c b/migration/vmstate.c
index ef26f26..437f156 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -471,7 +471,8 @@  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
     trace_vmstate_subsection_load(vmsd->name);
 
     while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
-        char idstr[256], *idstr_ret;
+        VMStateId idstr;
+        char *idstr_ret;
         int ret;
         uint8_t version_id, len, size;
         const VMStateDescription *sub_vmsd;