diff mbox series

[v3,7/8] virtio-mem: Migrate immutable properties early

Message ID 20230112164403.105085-8-david@redhat.com (mailing list archive)
State New, archived
Headers show
Series virtio-mem: Handle preallocation with migration | expand

Commit Message

David Hildenbrand Jan. 12, 2023, 4:44 p.m. UTC 
The bitmap and the size are immutable while migration is active: see
virtio_mem_is_busy(). We can migrate this information early, before
migrating any actual RAM content. Further, all information we need for
sanity checks is immutable as well.

Having this information in place early will, for example, allow for
properly preallocating memory before touching these memory locations
during RAM migration: this way, we can make sure that all memory was
actually preallocated and that any user errors (e.g., insufficient
hugetlb pages) can be handled gracefully.

In contrast, usable_region_size and requested_size can theoretically
still be modified on the source while the VM is running. Keep migrating
these properties the usual, late, way.

Use a new device property to keep behavior of compat machines
unmodified.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/machine.c              |  4 ++-
 hw/virtio/virtio-mem.c         | 51 ++++++++++++++++++++++++++++++++--
 include/hw/virtio/virtio-mem.h |  8 ++++++
 3 files changed, 60 insertions(+), 3 deletions(-)

Comments

Dr. David Alan Gilbert Jan. 12, 2023, 7:44 p.m. UTC | #1 
* David Hildenbrand (david@redhat.com) wrote:
> The bitmap and the size are immutable while migration is active: see
> virtio_mem_is_busy(). We can migrate this information early, before
> migrating any actual RAM content. Further, all information we need for
> sanity checks is immutable as well.
> 
> Having this information in place early will, for example, allow for
> properly preallocating memory before touching these memory locations
> during RAM migration: this way, we can make sure that all memory was
> actually preallocated and that any user errors (e.g., insufficient
> hugetlb pages) can be handled gracefully.
> 
> In contrast, usable_region_size and requested_size can theoretically
> still be modified on the source while the VM is running. Keep migrating
> these properties the usual, late, way.
> 
> Use a new device property to keep behavior of compat machines
> unmodified.

Can you get me a migration file from this? I want to try and understand
what happens when you have the vmstate_register together with the ->vmsd -
I'm not quite sure what ends up in the output.  Preferably for a VM with
two virtio-mem's.

Dave


> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/core/machine.c              |  4 ++-
>  hw/virtio/virtio-mem.c         | 51 ++++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-mem.h |  8 ++++++
>  3 files changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 616f3a207c..29b57f6448 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,7 +41,9 @@
>  #include "hw/virtio/virtio-pci.h"
>  #include "qom/object_interfaces.h"
>  
> -GlobalProperty hw_compat_7_2[] = {};
> +GlobalProperty hw_compat_7_2[] = {
> +    { "virtio-mem", "x-early-migration", "false" },
> +};
>  const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
>  
>  GlobalProperty hw_compat_7_1[] = {
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 02f7b5469a..51666baa01 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -31,6 +31,8 @@
>  #include CONFIG_DEVICES
>  #include "trace.h"
>  
> +static const VMStateDescription vmstate_virtio_mem_device_early;
> +
>  /*
>   * We only had legacy x86 guests that did not support
>   * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE. Other targets don't have legacy guests.
> @@ -878,6 +880,10 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
>  
>      host_memory_backend_set_mapped(vmem->memdev, true);
>      vmstate_register_ram(&vmem->memdev->mr, DEVICE(vmem));
> +    if (vmem->early_migration) {
> +        vmstate_register(VMSTATE_IF(vmem), VMSTATE_INSTANCE_ID_ANY,
> +                         &vmstate_virtio_mem_device_early, vmem);
> +    }
>      qemu_register_reset(virtio_mem_system_reset, vmem);
>  
>      /*
> @@ -899,6 +905,10 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
>       */
>      memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
>      qemu_unregister_reset(virtio_mem_system_reset, vmem);
> +    if (vmem->early_migration) {
> +        vmstate_unregister(VMSTATE_IF(vmem), &vmstate_virtio_mem_device_early,
> +                           vmem);
> +    }
>      vmstate_unregister_ram(&vmem->memdev->mr, DEVICE(vmem));
>      host_memory_backend_set_mapped(vmem->memdev, false);
>      virtio_del_queue(vdev, 0);
> @@ -1015,18 +1025,53 @@ static const VMStateDescription vmstate_virtio_mem_sanity_checks = {
>      },
>  };
>  
> +static bool virtio_mem_vmstate_field_exists(void *opaque, int version_id)
> +{
> +    const VirtIOMEM *vmem = VIRTIO_MEM(opaque);
> +
> +    /* With early migration, these fields were already migrated. */
> +    return !vmem->early_migration;
> +}
> +
>  static const VMStateDescription vmstate_virtio_mem_device = {
>      .name = "virtio-mem-device",
>      .minimum_version_id = 1,
>      .version_id = 1,
>      .priority = MIG_PRI_VIRTIO_MEM,
>      .post_load = virtio_mem_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_WITH_TMP_TEST(VirtIOMEM, virtio_mem_vmstate_field_exists,
> +                              VirtIOMEMMigSanityChecks,
> +                              vmstate_virtio_mem_sanity_checks),
> +        VMSTATE_UINT64(usable_region_size, VirtIOMEM),
> +        VMSTATE_UINT64_TEST(size, VirtIOMEM, virtio_mem_vmstate_field_exists),
> +        VMSTATE_UINT64(requested_size, VirtIOMEM),
> +        VMSTATE_BITMAP_TEST(bitmap, VirtIOMEM, virtio_mem_vmstate_field_exists,
> +                            0, bitmap_size),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +/*
> + * Transfer properties that are immutable while migration is active early,
> + * such that we have have this information around before migrating any RAM
> + * content.
> + *
> + * Note that virtio_mem_is_busy() makes sure these properties can no longer
> + * change on the migration source until migration completed.
> + *
> + * With QEMU compat machines, we transmit these properties later, via
> + * vmstate_virtio_mem_device instead -- see virtio_mem_vmstate_field_exists().
> + */
> +static const VMStateDescription vmstate_virtio_mem_device_early = {
> +    .name = "virtio-mem-device-early",
> +    .minimum_version_id = 1,
> +    .version_id = 1,
> +    .immutable = 1,
>      .fields = (VMStateField[]) {
>          VMSTATE_WITH_TMP(VirtIOMEM, VirtIOMEMMigSanityChecks,
>                           vmstate_virtio_mem_sanity_checks),
> -        VMSTATE_UINT64(usable_region_size, VirtIOMEM),
>          VMSTATE_UINT64(size, VirtIOMEM),
> -        VMSTATE_UINT64(requested_size, VirtIOMEM),
>          VMSTATE_BITMAP(bitmap, VirtIOMEM, 0, bitmap_size),
>          VMSTATE_END_OF_LIST()
>      },
> @@ -1211,6 +1256,8 @@ static Property virtio_mem_properties[] = {
>      DEFINE_PROP_ON_OFF_AUTO(VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP, VirtIOMEM,
>                              unplugged_inaccessible, ON_OFF_AUTO_AUTO),
>  #endif
> +    DEFINE_PROP_BOOL(VIRTIO_MEM_EARLY_MIGRATION_PROP, VirtIOMEM,
> +                     early_migration, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/virtio/virtio-mem.h b/include/hw/virtio/virtio-mem.h
> index 7745cfc1a3..f15e561785 100644
> --- a/include/hw/virtio/virtio-mem.h
> +++ b/include/hw/virtio/virtio-mem.h
> @@ -31,6 +31,7 @@ OBJECT_DECLARE_TYPE(VirtIOMEM, VirtIOMEMClass,
>  #define VIRTIO_MEM_BLOCK_SIZE_PROP "block-size"
>  #define VIRTIO_MEM_ADDR_PROP "memaddr"
>  #define VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP "unplugged-inaccessible"
> +#define VIRTIO_MEM_EARLY_MIGRATION_PROP "x-early-migration"
>  #define VIRTIO_MEM_PREALLOC_PROP "prealloc"
>  
>  struct VirtIOMEM {
> @@ -74,6 +75,13 @@ struct VirtIOMEM {
>      /* whether to prealloc memory when plugging new blocks */
>      bool prealloc;
>  
> +    /*
> +     * Whether we migrate properties that are immutable while migration is
> +     * active early, before state of other devices and especially, before
> +     * migrating any RAM content.
> +     */
> +    bool early_migration;
> +
>      /* notifiers to notify when "size" changes */
>      NotifierList size_change_notifiers;
>  
> -- 
> 2.39.0
>
David Hildenbrand Jan. 13, 2023, 1:59 p.m. UTC | #2 
On 12.01.23 20:44, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> The bitmap and the size are immutable while migration is active: see
>> virtio_mem_is_busy(). We can migrate this information early, before
>> migrating any actual RAM content. Further, all information we need for
>> sanity checks is immutable as well.
>>
>> Having this information in place early will, for example, allow for
>> properly preallocating memory before touching these memory locations
>> during RAM migration: this way, we can make sure that all memory was
>> actually preallocated and that any user errors (e.g., insufficient
>> hugetlb pages) can be handled gracefully.
>>
>> In contrast, usable_region_size and requested_size can theoretically
>> still be modified on the source while the VM is running. Keep migrating
>> these properties the usual, late, way.
>>
>> Use a new device property to keep behavior of compat machines
>> unmodified.
> 
> Can you get me a migration file from this? I want to try and understand
> what happens when you have the vmstate_register together with the ->vmsd -
> I'm not quite sure what ends up in the output.  Preferably for a VM with
> two virtio-mem's.

Sure, here is the stripped output from analyze-migration.py:

     "ram (2)": {
         "section sizes": {
             "0000:00:03.0/mem0": "0x0000000780000000",
             "0000:00:04.0/mem1": "0x0000000780000000",
             "pc.ram": "0x0000000100000000",
             "/rom@etc/acpi/tables": "0x0000000000020000",
             "pc.bios": "0x0000000000040000",
             "0000:00:02.0/e1000.rom": "0x0000000000040000",
             "pc.rom": "0x0000000000020000",
             "/rom@etc/table-loader": "0x0000000000001000",
             "/rom@etc/acpi/rsdp": "0x0000000000001000"
         }
     },
     "0000:00:03.0/virtio-mem-device-early (51)": {
         "tmp": "00 00 00 01 40 00 00 00 00 00 00 07 80 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00",
         "size": "0x0000000040000000",
         "bitmap": "ff ff ff ff [...] "
     },
     "0000:00:04.0/virtio-mem-device-early (53)": {
         "tmp": "00 00 00 08 c0 00 00 00 00 00 00 07 80 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00",
         "size": "0x00000001fa400000",
         "bitmap": "ff ff ff ff [...] "
     },
     "timer (0)": {
         "cpu_ticks_offset": "0x00000073f5ba3d28",
         "unused": "00 00 00 00 00 00 00 00",
         "cpu_clock_offset": "0x00000026b744e29c"
     },
[...]
     "serial (50)": {
         "state": {
             "divider": "0x0001",
             "rbr": "0x00",
             "ier": "0x05",
             "iir": "0xc1",
             "lcr": "0x13",
             "mcr": "0x0b",
             "lsr": "0x60",
             "msr": "0xb0",
             "scr": "0x00",
             "fcr_vmstate": "0x81"
         }
     },
     "0000:00:03.0/virtio-mem (52)": {
         "virtio": "00 00 00 02 f4 1a 58 10 07 01 10 00 01 00 ff [...]"
     "0000:00:04.0/virtio-mem (54)": {
         "virtio": "00 00 00 02 f4 1a 58 10 07 01 10 00 01 00 ff [...]"

The data of both "virtio" blobs is extremely large, a lot 0x00 -- no idea what virtio
core stores in there.

Note that vmstate_virtio_mem_device ("virtio-mem-device") will be included by virtio core in the
"virtio" blob.

I can send you a full savevm file privately, just ping me.
diff mbox series

Patch

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 616f3a207c..29b57f6448 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,7 +41,9 @@ 
 #include "hw/virtio/virtio-pci.h"
 #include "qom/object_interfaces.h"
 
-GlobalProperty hw_compat_7_2[] = {};
+GlobalProperty hw_compat_7_2[] = {
+    { "virtio-mem", "x-early-migration", "false" },
+};
 const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
 
 GlobalProperty hw_compat_7_1[] = {
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 02f7b5469a..51666baa01 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -31,6 +31,8 @@ 
 #include CONFIG_DEVICES
 #include "trace.h"
 
+static const VMStateDescription vmstate_virtio_mem_device_early;
+
 /*
  * We only had legacy x86 guests that did not support
  * VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE. Other targets don't have legacy guests.
@@ -878,6 +880,10 @@  static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
 
     host_memory_backend_set_mapped(vmem->memdev, true);
     vmstate_register_ram(&vmem->memdev->mr, DEVICE(vmem));
+    if (vmem->early_migration) {
+        vmstate_register(VMSTATE_IF(vmem), VMSTATE_INSTANCE_ID_ANY,
+                         &vmstate_virtio_mem_device_early, vmem);
+    }
     qemu_register_reset(virtio_mem_system_reset, vmem);
 
     /*
@@ -899,6 +905,10 @@  static void virtio_mem_device_unrealize(DeviceState *dev)
      */
     memory_region_set_ram_discard_manager(&vmem->memdev->mr, NULL);
     qemu_unregister_reset(virtio_mem_system_reset, vmem);
+    if (vmem->early_migration) {
+        vmstate_unregister(VMSTATE_IF(vmem), &vmstate_virtio_mem_device_early,
+                           vmem);
+    }
     vmstate_unregister_ram(&vmem->memdev->mr, DEVICE(vmem));
     host_memory_backend_set_mapped(vmem->memdev, false);
     virtio_del_queue(vdev, 0);
@@ -1015,18 +1025,53 @@  static const VMStateDescription vmstate_virtio_mem_sanity_checks = {
     },
 };
 
+static bool virtio_mem_vmstate_field_exists(void *opaque, int version_id)
+{
+    const VirtIOMEM *vmem = VIRTIO_MEM(opaque);
+
+    /* With early migration, these fields were already migrated. */
+    return !vmem->early_migration;
+}
+
 static const VMStateDescription vmstate_virtio_mem_device = {
     .name = "virtio-mem-device",
     .minimum_version_id = 1,
     .version_id = 1,
     .priority = MIG_PRI_VIRTIO_MEM,
     .post_load = virtio_mem_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_WITH_TMP_TEST(VirtIOMEM, virtio_mem_vmstate_field_exists,
+                              VirtIOMEMMigSanityChecks,
+                              vmstate_virtio_mem_sanity_checks),
+        VMSTATE_UINT64(usable_region_size, VirtIOMEM),
+        VMSTATE_UINT64_TEST(size, VirtIOMEM, virtio_mem_vmstate_field_exists),
+        VMSTATE_UINT64(requested_size, VirtIOMEM),
+        VMSTATE_BITMAP_TEST(bitmap, VirtIOMEM, virtio_mem_vmstate_field_exists,
+                            0, bitmap_size),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+/*
+ * Transfer properties that are immutable while migration is active early,
+ * such that we have have this information around before migrating any RAM
+ * content.
+ *
+ * Note that virtio_mem_is_busy() makes sure these properties can no longer
+ * change on the migration source until migration completed.
+ *
+ * With QEMU compat machines, we transmit these properties later, via
+ * vmstate_virtio_mem_device instead -- see virtio_mem_vmstate_field_exists().
+ */
+static const VMStateDescription vmstate_virtio_mem_device_early = {
+    .name = "virtio-mem-device-early",
+    .minimum_version_id = 1,
+    .version_id = 1,
+    .immutable = 1,
     .fields = (VMStateField[]) {
         VMSTATE_WITH_TMP(VirtIOMEM, VirtIOMEMMigSanityChecks,
                          vmstate_virtio_mem_sanity_checks),
-        VMSTATE_UINT64(usable_region_size, VirtIOMEM),
         VMSTATE_UINT64(size, VirtIOMEM),
-        VMSTATE_UINT64(requested_size, VirtIOMEM),
         VMSTATE_BITMAP(bitmap, VirtIOMEM, 0, bitmap_size),
         VMSTATE_END_OF_LIST()
     },
@@ -1211,6 +1256,8 @@  static Property virtio_mem_properties[] = {
     DEFINE_PROP_ON_OFF_AUTO(VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP, VirtIOMEM,
                             unplugged_inaccessible, ON_OFF_AUTO_AUTO),
 #endif
+    DEFINE_PROP_BOOL(VIRTIO_MEM_EARLY_MIGRATION_PROP, VirtIOMEM,
+                     early_migration, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-mem.h b/include/hw/virtio/virtio-mem.h
index 7745cfc1a3..f15e561785 100644
--- a/include/hw/virtio/virtio-mem.h
+++ b/include/hw/virtio/virtio-mem.h
@@ -31,6 +31,7 @@  OBJECT_DECLARE_TYPE(VirtIOMEM, VirtIOMEMClass,
 #define VIRTIO_MEM_BLOCK_SIZE_PROP "block-size"
 #define VIRTIO_MEM_ADDR_PROP "memaddr"
 #define VIRTIO_MEM_UNPLUGGED_INACCESSIBLE_PROP "unplugged-inaccessible"
+#define VIRTIO_MEM_EARLY_MIGRATION_PROP "x-early-migration"
 #define VIRTIO_MEM_PREALLOC_PROP "prealloc"
 
 struct VirtIOMEM {
@@ -74,6 +75,13 @@  struct VirtIOMEM {
     /* whether to prealloc memory when plugging new blocks */
     bool prealloc;
 
+    /*
+     * Whether we migrate properties that are immutable while migration is
+     * active early, before state of other devices and especially, before
+     * migrating any RAM content.
+     */
+    bool early_migration;
+
     /* notifiers to notify when "size" changes */
     NotifierList size_change_notifiers;