RAM migration overhaul

No reason not to call qemu_ram_map() once we have the allocation and remove duplicate code.

Signed-off-by Alex Williamson <[email protected]>

extern int phys_ram_fd;

cpu-all.h +4 -2

-extern uint8_t *phys_ram_dirty;
 extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+    uint8_t *host;
+    ram_addr_t offset;

void *qemu_get_ram_ptr(ram_addr_t addr)
{

exec.c +0 -21

-    RAMBlock *prev;
-    RAMBlock **prevp;
     RAMBlock *block;
 
-    prev = NULL;
-    prevp = &ram_blocks;
-    block = ram_blocks;
-    while (block && (block->offset > addr
-                     || block->offset + block->length <= addr)) {
-        if (prev)
-          prevp = &prev->next;
-        prev = block;
-        block = block->next;
-    }
-    if (!block) {
-        fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
-        abort();
-    }
-    /* Move this entry to to start of the list.  */
-    if (prev) {
-        prev->next = block->next;
-        block->next = *prevp;
-        *prevp = block;

Quoted 12 lines · * Alex Williamson ([email protected]) wrote:

extern int phys_ram_fd;

cpu-all.h +8 -2

-extern uint8_t *phys_ram_dirty;
 extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+    uint8_t *host;
+    ram_addr_t offset;
+    ram_addr_t length;
+    QLIST_ENTRY(RAMBlock) next;
+} RAMBlock;
+

cpu-all.h target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr); · line 876

Quoted 6 lines

cpu-all.h +6 -0

+typedef struct RAMList {
+    uint8_t *phys_dirty;
+    ram_addr_t last_offset;
+    QLIST_HEAD(ram, RAMBlock) blocks;
+} RAMList;
+extern RAMList ram;

such a generic name for global namespace

Quoted 9 lines

exec.c +0 -9

-    if (!block) {
-        fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
-        abort();
-    }
-    /* Move this entry to to start of the list.  */
-    if (prev) {
-        prev->next = block->next;
-        block->next = *prevp;
-        *prevp = block;

exec.c void qemu_ram_free(ram_addr_t addr) · line 2864

Quoted 10 lines

exec.c +8 -1

+    QLIST_FOREACH(block, &ram.blocks, next) {
+        if (addr - block->offset < block->length) {
+            QLIST_REMOVE(block, next);
+            QLIST_INSERT_HEAD(&ram.blocks, block, next);
+            return block->host + (addr - block->offset);
+        }
     }
-    return block->host + (addr - block->offset);
+
+    return NULL;

Why not preserve the error message and abort()? In error cases this would now just segfault.

Quoted 13 lines · Chris Wright

Quoted 12 lines · * Alex Williamson ([email protected]) wrote:

extern int phys_ram_fd;

cpu-all.h +8 -2

-extern uint8_t *phys_ram_dirty;
 extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+    uint8_t *host;
+    ram_addr_t offset;
+    ram_addr_t length;
+    QLIST_ENTRY(RAMBlock) next;
+} RAMBlock;
+

Quoted 8 lines

cpu-all.h target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr); · line 876

Quoted 6 lines

cpu-all.h +6 -0

+typedef struct RAMList {
+    uint8_t *phys_dirty;
+    ram_addr_t last_offset;
+    QLIST_HEAD(ram, RAMBlock) blocks;
+} RAMList;
+extern RAMList ram;

such a generic name for global namespace

Well it is _the_ ram, but yea... ;) Suggestions?

Quoted 13 lines · Chris Wright

Quoted 12 lines · * Alex Williamson ([email protected]) wrote:

extern int phys_ram_fd;

cpu-all.h +8 -2

-extern uint8_t *phys_ram_dirty;
 extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+    uint8_t *host;
+    ram_addr_t offset;
+    ram_addr_t length;
+    QLIST_ENTRY(RAMBlock) next;
+} RAMBlock;
+

Quoted 8 lines

cpu-all.h target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr); · line 876

Quoted 6 lines

cpu-all.h +6 -0

+typedef struct RAMList {
+    uint8_t *phys_dirty;
+    ram_addr_t last_offset;
+    QLIST_HEAD(ram, RAMBlock) blocks;
+} RAMList;
+extern RAMList ram;

such a generic name for global namespace

Well it is _the_ ram, but yea... ;) Suggestions?

This makes the RAM block list easier to manipulate.  Also incorporate relevant variables into the RAMList struct.

Signed-off-by Alex Williamson <[email protected]>

---

 arch_init.c |   14 ++++++-----
 cpu-all.h   |   28 ++++++++++++++++-------
 exec.c      |   72 ++++++++++++++++++-----------------------------------------

 3 files changed, 49 insertions(+), 65 deletions(-)

arch_init.c +7 -7

index 8e849a8..43e42ba 100644
@@ -110,7 +110,7 @@ static int ram_save_block(QEMUFile *f)
     ram_addr_t addr = 0;
     int bytes_sent = 0;
 
-    while (addr < last_ram_offset) {
+    while (addr < ram.last_offset) {
         if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
             uint8_t *p;
 

@@ -133,7 +133,7 @@ static int ram_save_block(QEMUFile *f)
             break;
         }
         addr += TARGET_PAGE_SIZE;
-        current_addr = (saved_addr + addr) % last_ram_offset;
+        current_addr = (saved_addr + addr) % ram.last_offset;
     }
 
     return bytes_sent;

@@ -146,7 +146,7 @@ static ram_addr_t ram_save_remaining(void)
     ram_addr_t addr;
     ram_addr_t count = 0;
 
-    for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
+    for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
         if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
             count++;
         }

@@ -167,7 +167,7 @@ uint64_t ram_bytes_transferred(void)
 
  uint64_t ram_bytes_total(void)
  {
-    return last_ram_offset;
+    return ram.last_offset;
  }
 
  int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)

@@ -191,7 +191,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         bytes_transferred = 0;
 
         /* Make sure all dirty bits are set */
-        for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
+        for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
             if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
                 cpu_physical_memory_set_dirty(addr);
             }

@@ -200,7 +200,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         /* Enable dirty memory tracking */
         cpu_physical_memory_set_dirty_tracking(1);
 
-        qemu_put_be64(f, last_ram_offset | RAM_SAVE_FLAG_MEM_SIZE);
+        qemu_put_be64(f, ram.last_offset | RAM_SAVE_FLAG_MEM_SIZE);
     }
 
     bytes_transferred_last = bytes_transferred;

@@ -259,7 +259,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
         addr &= TARGET_PAGE_MASK;
 
         if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
-            if (addr != last_ram_offset) {
+            if (addr != ram.last_offset) {
                 return -EINVAL;
             }
         }

cpu-all.h +0 -0

index 77eaf85..458cb4b 100644
@@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
  /* memory API */
 
  extern int phys_ram_fd;

cpu-all.h +13 -6

+
+typedef struct RAMList {
+    uint8_t *phys_dirty;
+    ram_addr_t last_offset;
+    QLIST_HEAD(ram, RAMBlock) blocks;
+} RAMList;
+extern RAMList ram;
 
  extern const char *mem_path;
  extern int mem_prealloc;

@@ -891,29 +903,29 @@ extern int mem_prealloc;
  /* read dirty bit (return 0 or 1) */
  static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
  {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
+    return ram.phys_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
  }
 
  static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
  {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS];
+    return ram.phys_dirty[addr >> TARGET_PAGE_BITS];
  }
 
  static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
                                                 int dirty_flags)
  {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
+    return ram.phys_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
  }
 
  static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
  {
-    phys_ram_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
+    ram.phys_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
  }
 
  static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
                                                       int dirty_flags)
  {
-    return phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
+    return ram.phys_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
  }
 
  static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,

@@ -925,7 +937,7 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
 
     len = length >> TARGET_PAGE_BITS;
     mask = ~dirty_flags;
-    p = phys_ram_dirty + (start >> TARGET_PAGE_BITS);
+    p = ram.phys_dirty + (start >> TARGET_PAGE_BITS);
     for (i = 0; i < len; i++) {
         p[i] &= mask;
     }

exec.c +22 -50

index c60f9e7..d785de3 100644
@@ -116,21 +116,9 @@ uint8_t *code_gen_ptr;
 
  #if !defined(CONFIG_USER_ONLY)
  int phys_ram_fd;
-uint8_t *phys_ram_dirty;
  static int in_migration;
 
-typedef struct RAMBlock {
-    uint8_t *host;
-    ram_addr_t offset;
-    ram_addr_t length;
-    struct RAMBlock *next;
-} RAMBlock;
-
-static RAMBlock *ram_blocks;
-/* TODO: When we implement (and use) ram deallocation (e.g. for hotplug)
-   then we can no longer assume contiguous ram offsets, and external uses
-   of this variable will break.  */
-ram_addr_t last_ram_offset;
+RAMList ram = { .blocks = QLIST_HEAD_INITIALIZER(ram) };
  #endif
 
  CPUState *first_cpu;

@@ -2795,18 +2783,17 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
 
     new_block->host = host;
 
-    new_block->offset = last_ram_offset;
+    new_block->offset = ram.last_offset;
     new_block->length = size;
 
-    new_block->next = ram_blocks;
-    ram_blocks = new_block;
+    QLIST_INSERT_HEAD(&ram.blocks, new_block, next);
 
-    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
-        (last_ram_offset + size) >> TARGET_PAGE_BITS);
-    memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
+    ram.phys_dirty = qemu_realloc(ram.phys_dirty,
+        (ram.last_offset + size) >> TARGET_PAGE_BITS);
+    memset(ram.phys_dirty + (ram.last_offset >> TARGET_PAGE_BITS),
            0xff, size >> TARGET_PAGE_BITS);
 
-    last_ram_offset += size;
+    ram.last_offset += size;
 
     if (kvm_enabled())
         kvm_setup_guest_memory(new_block->host, size);

@@ -2864,31 +2851,17 @@ void qemu_ram_free(ram_addr_t addr)
  */
  void *qemu_get_ram_ptr(ram_addr_t addr)
  {
-    RAMBlock *prev;
-    RAMBlock **prevp;
     RAMBlock *block;
 
-    prev = NULL;
-    prevp = &ram_blocks;
-    block = ram_blocks;
-    while (block && (block->offset > addr
-                     || block->offset + block->length <= addr)) {
-        if (prev)
-          prevp = &prev->next;
-        prev = block;
-        block = block->next;
-    }
-    if (!block) {
-        fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
-        abort();
-    }
-    /* Move this entry to to start of the list.  */
-    if (prev) {
-        prev->next = block->next;
-        block->next = *prevp;
-        *prevp = block;
+    QLIST_FOREACH(block, &ram.blocks, next) {
+        if (addr - block->offset < block->length) {
+            QLIST_REMOVE(block, next);
+            QLIST_INSERT_HEAD(&ram.blocks, block, next);
+            return block->host + (addr - block->offset);
+        }
     }
-    return block->host + (addr - block->offset);
+
+    return NULL;
  }
 
  int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)

@@ -2896,15 +2869,14 @@ int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
     RAMBlock *block;
     uint8_t *host = ptr;
 
-    block = ram_blocks;
-    while (block && (block->host > host
-                     || block->host + block->length <= host)) {
-        block = block->next;
+    QLIST_FOREACH(block, &ram.blocks, next) {
+        if (host - block->host < block->length) {
+            *ram_addr = block->offset + (host - block->host);
+            return 0;
+        }
     }
-    if (!block)
-        return -1;
-    *ram_addr = block->offset + (host - block->host);
-    return 0;
+
+    return -1;
  }
 
  /* Some of the softmmu routines need to translate from a host pointer

On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson

Quoted 7 lines

cpu-all.h +0 -0

index 77eaf85..458cb4b 100644
@@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
 /* memory API */
 

cpu-all.h target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr); · line 869

Quoted 11 lines

cpu-all.h +7 -2

 extern int phys_ram_fd;
-extern uint8_t *phys_ram_dirty;
 extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+    uint8_t *host;
+    ram_addr_t offset;
+    ram_addr_t length;
+    QLIST_ENTRY(RAMBlock) next;
+} RAMBlock;

For my shared memory device I need a way to mark device memory as not to be migrated. Can a flag to be added to this struct to accomplish this?

Quoted 8 lines

On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson

Quoted 7 lines

cpu-all.h +0 -0

index 77eaf85..458cb4b 100644
@@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
  /* memory API */
 

Quoted 15 lines

cpu-all.h target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr); · line 869

Quoted 11 lines

cpu-all.h +7 -2

  extern int phys_ram_fd;
-extern uint8_t *phys_ram_dirty;
  extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+    uint8_t *host;
+    ram_addr_t offset;
+    ram_addr_t length;
+    QLIST_ENTRY(RAMBlock) next;
+} RAMBlock;

For my shared memory device I need a way to mark device memory as not to be migrated.  Can a flag to be added to this struct to accomplish this?

Yep, it should be easy to skip blocks during migration based on a flag here.  I guess that probably means you'd also want a flag when you alloc the block, so maybe it should be:

qemu_ram_alloc(DeviceState *dev, const char *name, ram_addr_t size, int flags)

Best to make that change now, then we can add it to the RAMBlock as we need.

Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The offset given to a block created via qemu_ram_alloc/map() is arbitrary, let the caller specify a name so we can make a positive match.

Note, this only addresses the qemu-kvm callers so far.

Signed-off-by Alex Williamson<[email protected]>

---

  cpu-all.h              |    1 +
  cpu-common.h           |    4 ++--
  exec.c                 |    8 +++++---
  hw/device-assignment.c |    8 ++++++--
  hw/pc.c                |    8 ++++----
  hw/pci.c               |    5 ++++-
  hw/vga.c               |    2 +-
  hw/vmware_vga.c        |    2 +-
  8 files changed, 24 insertions(+), 14 deletions(-)

cpu-all.h +1 -0

index 458cb4b..a5b886a 100644
@@ -865,6 +865,7 @@ typedef struct RAMBlock {
      uint8_t *host;
      ram_addr_t offset;
      ram_addr_t length;
+    char name[64];
      QLIST_ENTRY(RAMBlock) next;
  } RAMBlock;
 

cpu-common.h +2 -2

index 4b0ba60..5b00544 100644
@@ -40,8 +40,8 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
  }
 
  ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
-ram_addr_t qemu_ram_alloc(ram_addr_t);
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host);
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t);
  void qemu_ram_free(ram_addr_t addr);
  /* This should only be used for ram local to a device.  */
  void *qemu_get_ram_ptr(ram_addr_t addr);

exec.c +0 -1

index d785de3..dd57515 100644
@@ -2774,13 +2774,15 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
  }
  #endif
 
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host)

exec.c +0 -0

      new_block->host = host;
 

      new_block->offset = ram.last_offset;

exec.c +2 -2

@@ -2801,7 +2803,7 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
      return new_block->offset;
  }
 
-ram_addr_t qemu_ram_alloc(ram_addr_t size)
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t size)
  {
      void *host;
 

@@ -2833,7 +2835,7 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
  #endif
      }
 
-    return qemu_ram_map(size, host);
+    return qemu_ram_map(name, size, host);
  }
 
  void qemu_ram_free(ram_addr_t addr)

hw/device-assignment.c +6 -2

index 2b963b5..1d631f6 100644
@@ -569,9 +569,13 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
 
              if (!slow_map) {
                  void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
+                char name[14];
 
-                pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
-                                                                virtbase);
+                snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
+                         PCI_SLOT(pci_dev->dev.devfn),
+                         PCI_FUNC(pci_dev->dev.devfn), i);
+                pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
+                                                  cur_region->size, virtbase);
              } else
                  pci_dev->v_addrs[i].memory_index = 0;
 

hw/pc.c +4 -4

index eae0db4..76be151 100644
@@ -834,7 +834,7 @@ void pc_memory_init(ram_addr_t ram_size,
      linux_boot = (kernel_filename != NULL);
 
      /* allocate RAM */
-    ram_addr = qemu_ram_alloc(below_4g_mem_size);
+    ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
      cpu_register_physical_memory(0, 0xa0000, ram_addr);
      cpu_register_physical_memory(0x100000,
                   below_4g_mem_size - 0x100000,

@@ -845,7 +845,7 @@ void pc_memory_init(ram_addr_t ram_size,
  #if TARGET_PHYS_ADDR_BITS == 32
          hw_error("To much RAM for 32-bit physical address");
  #else
-        ram_addr = qemu_ram_alloc(above_4g_mem_size);
+        ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
          cpu_register_physical_memory(0x100000000ULL,
                                       above_4g_mem_size,
                                       ram_addr);

@@ -866,7 +866,7 @@ void pc_memory_init(ram_addr_t ram_size,
          (bios_size % 65536) != 0) {
          goto bios_error;
      }
-    bios_offset = qemu_ram_alloc(bios_size);
+    bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
      ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
      if (ret != 0) {
      bios_error:

@@ -892,7 +892,7 @@ void pc_memory_init(ram_addr_t ram_size,
      }
      option_rom[nb_option_roms++] = qemu_strdup(VAPIC_FILENAME);
 
-    option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
+    option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
      cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
 
      /* map all the bios at the top of memory */

hw/pci.c +4 -1

index f0b6790..b2632a8 100644
@@ -1889,6 +1889,7 @@ static int pci_add_option_rom(PCIDevice *pdev)
      int size;
      char *path;
      void *ptr;
+    char name[13];
 
      if (!pdev->romfile)
          return 0;

@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
          size = 1<<  qemu_fls(size);
      }
 
-    pdev->rom_offset = qemu_ram_alloc(size);
+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);
 
      ptr = qemu_get_ram_ptr(pdev->rom_offset);
      load_image(path, ptr);

hw/vga.c +1 -1

index 0a535ae..47b800f 100644
@@ -2292,7 +2292,7 @@ void vga_common_init(VGACommonState *s, int vga_ram_size)
  #else
      s->is_vbe_vmstate = 0;
  #endif
-    s->vram_offset = qemu_ram_alloc(vga_ram_size);
+    s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
      s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
      s->vram_size = vga_ram_size;
      s->get_bpp = vga_get_bpp;

hw/vmware_vga.c +0 -0

index bf2a699..9df8c08 100644

Quoted 10 lines · Alex Williamson

The offset given to a block created via qemu_ram_alloc/map() is arbitrary, let the caller specify a name so we can make a positive match.

Note, this only addresses the qemu-kvm callers so far.

exec.c +1 -1

-ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
  {
      RAMBlock *new_block;

exec.c static void *file_ram_alloc(ram_addr_t memory, const char *path) · line 2785

Quoted 6 lines

      size = TARGET_PAGE_ALIGN(size);
      new_block = qemu_malloc(sizeof(*new_block));

exec.c +2 -0

+    // XXX check duplicates
+    snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));

That strdup() is probably unintentional.

+ // XXX check duplicates

Quoted 1 line · * Alex Williamson ([email protected]) wrote:

+ // XXX check duplicates

Yes, definitely.

You created a notion of a hierarchical namespace, can this be formalized any more? Currently scattered...

hw/device-assignment.c static int assigned_dev_register_regions(PCIRegion *io_regions,

Quoted 11 lines

hw/device-assignment.c +11 -0

+                char name[14];
+                snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
+                         PCI_SLOT(pci_dev->dev.devfn),
+                         PCI_FUNC(pci_dev->dev.devfn), i);
+                pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
+                                                  cur_region->size, virtbase);
+    ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
+        ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
+    bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
+    option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
+    char name[13];

(13, 14...good to be consistent, maybe w/ helper like, pci_ram_alloc_{bar,rom})

hw/pci.c static int pci_add_option_rom(PCIDevice *pdev)

Quoted 5 lines

hw/pci.c +5 -0

+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);
+    s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
+    s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);

So far we have:
ram.

     pc.
        lowmem
        highmem
        bios
        rom
     vga.
         vram
     vmsvga.
            fifo

pci.

      D.F. (B:D.F?)
          bar
          rom

pci.

       D.F. (B:D.F?)

bar
rom

Quoted 1 line

+ // XXX check duplicates

Yes, definitely. You created a notion of a hierarchical namespace, can this be formalized any more?

Quoted 3 lines

The offset given to a block created via qemu_ram_alloc/map() is arbitrary, let the caller specify a name so we can make a positive match.

hw/pci.c static int pci_add_option_rom(PCIDevice *pdev) · line 1928

Quoted 5 lines

hw/pci.c +3 -0

@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);
 

This looks pretty bogus. It should be associated with the device, rather than incorrectly trying to generate a globally unique name.

Paul

Quoted 5 lines

Quoted 5 lines

The offset given to a block created via qemu_ram_alloc/map() is arbitrary, let the caller specify a name so we can make a positive match.

Quoted 7 lines

hw/pci.c static int pci_add_option_rom(PCIDevice *pdev) · line 1927

Quoted 4 lines

hw/pci.c +3 -0

@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);

This looks pretty bogus. It should be associated with the device, rather than incorrectly trying to generate a globally unique name.

Not all ram is associated with a device.

Quoted 14 lines · Paul Brook

Quoted 12 lines

hw/pci.c static int pci_add_option_rom(PCIDevice *pdev) · line 1927

Quoted 9 lines

The offset given to a block created via qemu_ram_alloc/map() is arbitrary, let the caller specify a name so we can make a positive match.

hw/pci.c +3 -0

@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);

This looks pretty bogus. It should be associated with the device, rather than incorrectly trying to generate a globally unique name.

Quoted 1 line

Not all ram is associated with a device.

Maybe not, but where it is we should be using that information. Absolute minimum we should be using the existing qdev address rather than inventing a new one. Duplicating this logic inside every device seems like a bad idea so I suggest identifying ram blocks by a (name, device) pair. For now we can allow a NULL device for system memory.

Quoted 7 lines

Quoted 1 line

Not all ram is associated with a device.

Maybe not, but where it is we should be using that information. Absolute minimum we should be using the existing qdev address rather than inventing a new one. Duplicating this logic inside every device seems like a bad idea so I suggest identifying ram blocks by a (name, device) pair. For now we can allow a NULL device for system memory.

I'm not sure if this is what you're after, but what if we change it to something like:

(quoted diff) +3 -0

+    snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
+             pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
+             PCI_FUNC(pdev->devfn), pdev->qdev.info->name);

This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we can expand on the host details, such as:
..
Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?

Quoted 15 lines

Quoted 7 lines

Quoted 1 line

Not all ram is associated with a device.

Maybe not, but where it is we should be using that information. Absolute minimum we should be using the existing qdev address rather than inventing a new one. Duplicating this logic inside every device seems like a bad idea so I suggest identifying ram blocks by a (name, device) pair. For now we can allow a NULL device for system memory.

I'm not sure if this is what you're after, but what if we change it to something like:

(quoted diff) +3 -0

+    snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
+             pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
+             PCI_FUNC(pdev->devfn), pdev->qdev.info->name);

Quoted 4 lines

This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we can expand on the host details, such as:
..
Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?

Not really. This identifier is device and bus independent, which is why I suggested passing the device to qemu_ram_alloc. This can then figure out how to the identify the device. It should probably do this the same way that we identify the saved state for the device. Currently I think this is an arbitrary vmstate name/id, but I expect this to change to a qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").

Quoted 6 lines

Not really. This identifier is device and bus independent, which is why I suggested passing the device to qemu_ram_alloc. This can then figure out how to the identify the device. It should probably do this the same way that we identify the saved state for the device. Currently I think this is an arbitrary vmstate name/id, but I expect this to change to a qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").

Ok, that seems fairly reasonable, so from a device pointer we can get something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add something like ":rom" or ":bar.0" to it via an extra string.

qemu_ram_alloc(DeviceState *dev, const char *info, size)

Does a function already exist to print out a qdev address path?

I may have been a bit misleading here. What we really want to do is use the same matching algorithm as is used by the rest of the device state. Currently this is a vmstate name and [arbitrary] numeric id. I don't remember whether there's a convenient link from a device to its associated vmstate - if there isn't there probably should be.

Quoted 5 lines

I may have been a bit misleading here. What we really want to do is use the same matching algorithm as is used by the rest of the device state. Currently this is a vmstate name and [arbitrary] numeric id. I don't remember whether there's a convenient link from a device to its associated vmstate - if there isn't there probably should be.

DeviceState->info->vmsd->name for the name.
Dunno about the numeric id, I think savevm.c doesn't export it.

pci_add addr=4 nic model=e1000
pci_add addr=3 nic model=e1000

Quoted 6 lines

Quoted 5 lines

I may have been a bit misleading here. What we really want to do is use the same matching algorithm as is used by the rest of the device state. Currently this is a vmstate name and [arbitrary] numeric id. I don't remember whether there's a convenient link from a device to its associated vmstate - if there isn't there probably should be.

Quoted 2 lines

DeviceState->info->vmsd->name for the name.
Dunno about the numeric id, I think savevm.c doesn't export it.

Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>. It seems like this highlights a deficiency in the vmstate matching

Then we start the target, ordering the nics sequentially, are we going to store the vmstate into the opposite nics?

Quoted 7 lines · Gerd Hoffmann

Quoted 6 lines

Quoted 5 lines

I may have been a bit misleading here. What we really want to do is use the same matching algorithm as is used by the rest of the device state. Currently this is a vmstate name and [arbitrary] numeric id. I don't remember whether there's a convenient link from a device to its associated vmstate - if there isn't there probably should be.

Quoted 5 lines

Quoted 2 lines

DeviceState->info->vmsd->name for the name.
Dunno about the numeric id, I think savevm.c doesn't export it.

Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>. It seems like this highlights a deficiency in the vmstate matching

Why are you forcing this to be a string?

Quoted 4 lines · Paul Brook

Quoted 2 lines

Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>. It seems like this highlights a deficiency in the vmstate matching

Why are you forcing this to be a string?

It seemed like a good way to send an identifier. What do you suggest?

Quoted 7 lines · Alex Williamson

Quoted 4 lines · Paul Brook

Quoted 2 lines

Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>. It seems like this highlights a deficiency in the vmstate matching

Why are you forcing this to be a string?

It seemed like a good way to send an identifier. What do you suggest?

Do it the same way savevm handles device state? I think it simply puts a u32 for the instance id.

The trouble I'm running into is that the SaveStateEntry.instance_id is effectively private, and there's no easy way to associate a SaveStateEntry to a device since it passes an opaque pointer, which could be whatever the driver decides it wants. I'm wondering if we should pass the DeviceState pointer in when registering the vmstate so that we can stuff the instance_id into the DeviceInfo. Or maybe DeviceInfo should include a pointer to the SaveStateEntry. It all seems pretty messy. Any thoughts?

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

I may have been a bit misleading here. What we really want to do is use the same matching algorithm as is used by the rest of the device state. Currently this is a vmstate name and [arbitrary] numeric id. I don't remember whether there's a convenient link from a device to its associated vmstate - if there isn't there probably should be.

DeviceState->info->vmsd->name for the name.
Dunno about the numeric id, I think savevm.c doesn't export it.

Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>. It seems like this highlights a deficiency in the vmstate matching though. If on the source we do:

Quoted 2 lines

pci_add addr=4 nic model=e1000
pci_add addr=3 nic model=e1000

Then we start the target, ordering the nics sequentially, are we going to store the vmstate into the opposite nics?

Quoted 7 lines

Quoted 6 lines

Not really. This identifier is device and bus independent, which is why I suggested passing the device to qemu_ram_alloc. This can then figure out how to the identify the device. It should probably do this the same way that we identify the saved state for the device. Currently I think this is an arbitrary vmstate name/id, but I expect this to change to a qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").

Quoted 5 lines

Ok, that seems fairly reasonable, so from a device pointer we can get something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add something like ":rom" or ":bar.0" to it via an extra string.

qemu_ram_alloc(DeviceState *dev, const char *info, size)

Exactly - though personally I wouldn't call the second argument "info".

Quoted 4 lines · Paul Brook

to the identify the device. It should probably do this the same way that we identify the saved state for the device. Currently I think this is an arbitrary vmstate name/id, but I expect this to change to a qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").

Ok, that seems fairly reasonable, so from a device pointer we can get something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add something like ":rom" or ":bar.0" to it via an extra string.

Quoted 8 lines

Quoted 4 lines

to the identify the device. It should probably do this the same way that we identify the saved state for the device. Currently I think this is an arbitrary vmstate name/id, but I expect this to change to a qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").

Ok, that seems fairly reasonable, so from a device pointer we can get something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add something like ":rom" or ":bar.0" to it via an extra string.

In the fun game of what ifs...

The cmdline starts w/ device A placed at pci bus addr 00:04.0 (so matched on source and target). The source does hotunplug of 04.0 and replaces it w/ new device. I think we need something that is more uniquely identifying the block. Not sure that device name is correct or a generation ID.

Quoted 15 lines · Paul Brook

Quoted 14 lines

hw/pci.c static int pci_add_option_rom(PCIDevice *pdev) · line 1928

Quoted 11 lines

The offset given to a block created via qemu_ram_alloc/map() is arbitrary, let the caller specify a name so we can make a positive match.

hw/pci.c +3 -0

@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);
 

This looks pretty bogus. It should be associated with the device, rather than incorrectly trying to generate a globally unique name.

Quoted 2 lines

Not all ram is associated with a device.

Maybe not, but where it is we should be using that information. Absolute minimum we should be using the existing qdev address rather than inventing a new one. Duplicating this logic inside every device seems like a bad idea so I suggest identifying ram blocks by a (name, device) pair. For now we can allow a NULL device for system memory.

Quoted 13 lines · Paul Brook

Quoted 13 lines · Paul Brook

Quoted 11 lines

hw/pci.c static int pci_add_option_rom(PCIDevice *pdev) · line 1927

Quoted 9 lines

The offset given to a block created via qemu_ram_alloc/map() is arbitrary, let the caller specify a name so we can make a positive match.

hw/pci.c +3 -0

@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
+    snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    pdev->rom_offset = qemu_ram_alloc(name, size);

This looks pretty bogus. It should be associated with the device, rather than incorrectly trying to generate a globally unique name.

Not all ram is associated with a device.

Quoted 6 lines

Maybe not, but where it is we should be using that information. Absolute minimum we should be using the existing qdev address rather than inventing a new one. Duplicating this logic inside every device seems like a bad idea so I suggest identifying ram blocks by a (name, device) pair. For now we can allow a NULL device for system memory.

I agree, this is duplicating the qdev tree in a string. Devices/BARs should have ram qdev fields and so ram can be enumerated completely via qdev.

System memory can be part of a system device.

Keep in mind, this has to be a stable string across versions of qemu since this is savevm/migration. Are we absolutely confident that the full qdev path isn't going to change? I'm more confident that a unique device name is going to be static across qemu versions.

Quoted 10 lines · Paul Brook

hw/pci.c static int pci_add_option_rom(PCIDevice *pdev) · line 1927

Quoted 7 lines

The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
let the caller specify a name so we can make a positive match.

hw/pci.c +3 -0

@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
+ snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+ PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+ pdev->rom_offset = qemu_ram_alloc(name, size);

This looks pretty bogus. It should be associated with the device, rather than
incorrectly trying to generate a globally unique name.

Not all ram is associated with a device.

Synchronize RAM blocks with the target and migrate using name/offset pairs. This ensures both source and target have the same view of RAM and that we get the right bits into the right slot.

Signed-off-by Alex Williamson<[email protected]>

---

  arch_init.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
  vl.c        |    2 +
  2 files changed, 93 insertions(+), 12 deletions(-)

arch_init.c +5 -1

index 6103461..8ba92ec 100644
@@ -113,20 +113,29 @@ static int ram_save_block(QEMUFile *f)
 
      while (addr<  total_ram) {
          if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
+            RAMBlock *block;
+            ram_addr_t offset;
              uint8_t *p;
 
              cpu_physical_memory_reset_dirty(current_addr,
                                              current_addr + TARGET_PAGE_SIZE,
                                              MIGRATION_DIRTY_FLAG);
 
-            p = qemu_get_ram_ptr(current_addr);
+            QLIST_FOREACH(block,&ram.blocks, next) {
+                if (current_addr - block->offset<  block->length)
+                    break;

arch_init.c +2 -1

                  bytes_sent = 1;
              } else {
-                qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_PAGE);
+                qemu_put_be64(f, offset | RAM_SAVE_FLAG_PAGE);
+                qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
                  qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
                  bytes_sent = TARGET_PAGE_SIZE;

              }

arch_init.c +80 -8

@@ -196,6 +205,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
      }
 
      if (stage == 1) {
+        RAMBlock *block;
          uint64_t total_ram = ram_bytes_total();
          bytes_transferred = 0;
 

@@ -210,6 +220,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
          cpu_physical_memory_set_dirty_tracking(1);
 
          qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
+
+        QLIST_FOREACH(block,&ram.blocks, next) {
+            qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
+            qemu_put_be64(f, block->length);
+        }
      }
 
      bytes_transferred_last = bytes_transferred;

@@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
      ram_addr_t addr;
      int flags;
 
-    if (version_id != 3) {
+    if (version_id<  3) {
          return -EINVAL;
      }
 

@@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
          addr&= TARGET_PAGE_MASK;
 
          if (flags&  RAM_SAVE_FLAG_MEM_SIZE) {
-            if (addr != ram_bytes_total()) {
-                return -EINVAL;
+            if (version_id == 3) {
+                if (addr != ram_bytes_total()) {
+                    return -EINVAL;
+                }
+            } else {
+                /* Synchronize RAM block list */
+                char name[64];
+                ram_addr_t length;
+                ram_addr_t total_ram_bytes = addr;
+
+                while (total_ram_bytes) {
+                    RAMBlock *block;
+                    qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+                    length = qemu_get_be64(f);
+
+                    QLIST_FOREACH(block,&ram.blocks, next) {
+                        if (!strncmp(name, block->name, sizeof(name))) {
+                            if (block->length != length)
+                                return -EINVAL;
+                            break;
+                        }
+                    }
+
+                    if (!block) {
+                        if (!qemu_ram_alloc(name, length))
+                            return -ENOMEM;
+                    }
+
+                    total_ram_bytes -= length;
+                }
              }
          }
 
          if (flags&  RAM_SAVE_FLAG_COMPRESS) {
-            uint8_t ch = qemu_get_byte(f);
-            memset(qemu_get_ram_ptr(addr), ch, TARGET_PAGE_SIZE);
+            void *host;
+            uint8_t ch;
+
+            if (version_id == 3) {
+                host = qemu_get_ram_ptr(addr);
+            } else {
+                RAMBlock *block;
+                char name[64];
+
+                qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+
+                QLIST_FOREACH(block,&ram.blocks, next) {
+                    if (!strncmp(name, block->name, sizeof(name)))
+                        break;
+                }
+                if (!block)
+                    return -EINVAL;
+
+                host = block->host + addr;
+            }
+            ch = qemu_get_byte(f);
+            memset(host, ch, TARGET_PAGE_SIZE);
  #ifndef _WIN32
              if (ch == 0&&
                  (!kvm_enabled() || kvm_has_sync_mmu())) {
-                madvise(qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE,
-                        MADV_DONTNEED);
+                madvise(host, TARGET_PAGE_SIZE, MADV_DONTNEED);
              }
  #endif
          } else if (flags&  RAM_SAVE_FLAG_PAGE) {
-            qemu_get_buffer(f, qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE);
+            void *host;
+
+            if (version_id == 3) {
+                host = qemu_get_ram_ptr(addr);
+            } else {
+                RAMBlock *block;
+                char name[64];
+
+                qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+
+                QLIST_FOREACH(block,&ram.blocks, next) {
+                    if (!strncmp(name, block->name, sizeof(name)))
+                        break;
+                }
+                if (!block)
+                    return -EINVAL;
+
+                host = block->host + addr;
+            }
+            qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
          }
          if (qemu_file_has_error(f)) {
              return -EIO;

vl.c +1 -1

index 9e9c176..a871895 100644
@@ -3731,7 +3731,7 @@ int main(int argc, char **argv, char **envp)
      if (qemu_opts_foreach(&qemu_drive_opts, drive_init_func, machine, 1) != 0)
          exit(1);
 
-    register_savevm_live("ram", 0, 3, NULL, ram_save_live, NULL,
+    register_savevm_live("ram", 0, 4, NULL, ram_save_live, NULL,
                           ram_load, NULL);
 
      if (nb_numa_nodes>  0) {

arch_init.c static int ram_save_block(QEMUFile *f) · line 134

Quoted 6 lines · Alex Williamson

              if (is_dup_page(p, *p)) {

arch_init.c +2 -1

-                qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
+                qemu_put_be64(f, offset | RAM_SAVE_FLAG_COMPRESS);
+                qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
                  qemu_put_byte(f, *p);

I think we could use some trickery like use another flag in current_address to determine whether this was a different section from the previous section and then only encode the section name if that's true.

arch_init.c +16 -2

-            if (addr != ram_bytes_total()) {
-                return -EINVAL;
+            if (version_id == 3) {
+                if (addr != ram_bytes_total()) {
+                    return -EINVAL;
+                }
+            } else {
+                /* Synchronize RAM block list */
+                char name[64];
+                ram_addr_t length;
+                ram_addr_t total_ram_bytes = addr;
+
+                while (total_ram_bytes) {
+                    RAMBlock *block;
+                    qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+                    length = qemu_get_be64(f);
+
+                    QLIST_FOREACH(block, &ram.blocks, next) {

arch_init.c int ram_load(QEMUFile *f, void *opaque, int version_id) · line 276

Quoted 7 lines · * Alex Williamson ([email protected]) wrote:

arch_init.c +1 -1

@@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
     ram_addr_t addr;
     int flags;
 
-    if (version_id != 3) {
+    if (version_id < 3) {
         return -EINVAL;

Should we clamp to 3 and 4?

arch_init.c int ram_load(QEMUFile *f, void *opaque, int version_id) · line 285

Quoted 6 lines

arch_init.c +0 -0

     }
 

@@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
         addr &= TARGET_PAGE_MASK;
 
         if (flags & RAM_SAVE_FLAG_MEM_SIZE) {

Does it simplify anything to simply add a new flag?

arch_init.c int ram_load(QEMUFile *f, void *opaque, int version_id) · line 311

Quoted 11 lines

arch_init.c +11 -0

+                    QLIST_FOREACH(block, &ram.blocks, next) {
+                        if (!strncmp(name, block->name, sizeof(name))) {
+                            if (block->length != length)
+                                return -EINVAL;
+                            break;
+                        }
+                    }
+
+                    if (!block) {
+                        if (!qemu_ram_alloc(name, length))
+                            return -ENOMEM;

Is there any use to finding blocks in stream such that we simply allocate them all dynamically?