Re: [PATCH V1 07/26] migration: VMStateId

[Steven Sistare via <qemu-devel@nongnu.org>]

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