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

[Peter Xu <peterx@redhat.com>]

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,

-- 
Peter Xu