pc-dimm: factor out MemoryDevice

On the qmp level, we already have the concept of memory devices:
"query-memory-devices"
Right now, we only support NVDIMM and PCDIMM.

We want to map other devices later into the address space of the guest. Such device could e.g. be virtio devices. These devices will have a guest memory range assigned but won't be exposed via e.g. ACPI. We want to make them look like memory device, but not glued to pc-dimm.

Especially, it will not always be possible to have TYPE_PC_DIMM as a parent class (e.g. virtio devices). Let's use an interface instead. As a first part, convert handling of
- qmp_pc_dimm_device_list
- get_plugged_memory_size
to our new model. plug/unplug stuff etc. will follow later.

A memory device will have to provide the following functions:
- get_addr(): Necessary, as the property "addr" can e.g. not be used for virtio devices (already defined).
- get_plugged_size(): The amount this device offers to the guest as of now.
- get_region_size(): Because this can later on be bigger than the plugged size.
- fill_device_info(): Fill MemoryDeviceInfo, e.g. for qmp.

Signed-off-by David Hildenbrand <[email protected]>

hw/mem/memory-device.c new file +4 -0

+    MemoryDeviceClass *mdc_a = MEMORY_DEVICE_GET_CLASS(a);
+    MemoryDeviceClass *mdc_b = MEMORY_DEVICE_GET_CLASS(b);
+    const uint64_t addr_a = mdc_a->get_addr(md_a);
+    const uint64_t addr_b = mdc_b->get_addr(md_b);

Quoted 25 lines · David Hildenbrand

On the qmp level, we already have the concept of memory devices:
"query-memory-devices"
Right now, we only support NVDIMM and PCDIMM.

We want to map other devices later into the address space of the guest. Such device could e.g. be virtio devices. These devices will have a guest memory range assigned but won't be exposed via e.g. ACPI. We want to make them look like memory device, but not glued to pc-dimm.

Especially, it will not always be possible to have TYPE_PC_DIMM as a parent class (e.g. virtio devices). Let's use an interface instead. As a first part, convert handling of
- qmp_pc_dimm_device_list
- get_plugged_memory_size
to our new model. plug/unplug stuff etc. will follow later.

A memory device will have to provide the following functions:
- get_addr(): Necessary, as the property "addr" can e.g. not be used for virtio devices (already defined).
- get_plugged_size(): The amount this device offers to the guest as of now.
- get_region_size(): Because this can later on be bigger than the plugged size.
- fill_device_info(): Fill MemoryDeviceInfo, e.g. for qmp.

Quoted 1 line

Signed-off-by David Hildenbrand <[email protected]>

Reviewed-by David Gibson <[email protected]>

with the exception of some tiny nits..

[snip]

hw/mem/memory-device.c line 22

Quoted 4 lines

hw/mem/memory-device.c new file +4 -0

+static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
+{
+    MemoryDeviceState *md_a = MEMORY_DEVICE(a);
+    MemoryDeviceState *md_b = MEMORY_DEVICE(b);

These probably should be const MemoryDeviceState *.

Quoted 5 lines

hw/mem/memory-device.c new file +4 -0

+    MemoryDeviceClass *mdc_a = MEMORY_DEVICE_GET_CLASS(a);
+    MemoryDeviceClass *mdc_b = MEMORY_DEVICE_GET_CLASS(b);
+    const uint64_t addr_a = mdc_a->get_addr(md_a);
+    const uint64_t addr_b = mdc_b->get_addr(md_b);

hw/mem/memory-device.c line 36

Quoted 10 lines

hw/mem/memory-device.c new file +10 -0

+
+    if (addr_a > addr_b) {
+        return 1;
+    } else if (addr_a < addr_b) {
+        return -1;
+    }
+    return 0;
+}
+
+static int memory_device_built_list(Object *obj, void *opaque)

s/built/build/ will read a bit more clearly I think.

Quoted 32 lines · David Gibson

Quoted 25 lines · David Hildenbrand

On the qmp level, we already have the concept of memory devices:
"query-memory-devices"
Right now, we only support NVDIMM and PCDIMM.

We want to map other devices later into the address space of the guest. Such device could e.g. be virtio devices. These devices will have a guest memory range assigned but won't be exposed via e.g. ACPI. We want to make them look like memory device, but not glued to pc-dimm.

Especially, it will not always be possible to have TYPE_PC_DIMM as a parent class (e.g. virtio devices). Let's use an interface instead. As a first part, convert handling of
- qmp_pc_dimm_device_list
- get_plugged_memory_size
to our new model. plug/unplug stuff etc. will follow later.

A memory device will have to provide the following functions:
- get_addr(): Necessary, as the property "addr" can e.g. not be used for virtio devices (already defined).
- get_plugged_size(): The amount this device offers to the guest as of now.
- get_region_size(): Because this can later on be bigger than the plugged size.
- fill_device_info(): Fill MemoryDeviceInfo, e.g. for qmp.

Quoted 1 line

Signed-off-by David Hildenbrand <[email protected]>

Reviewed-by David Gibson <[email protected]>

with the exception of some tiny nits..

Quoted 7 lines

[snip]

hw/mem/memory-device.c line 22

Quoted 4 lines

hw/mem/memory-device.c new file +4 -0

+static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
+{
+    MemoryDeviceState *md_a = MEMORY_DEVICE(a);
+    MemoryDeviceState *md_b = MEMORY_DEVICE(b);

These probably should be const MemoryDeviceState *.

Yes, that could work (depends on what the functions getting called internally expect). Will look into it

Quoted 8 lines

Quoted 8 lines

hw/mem/memory-device.c new file +8 -0

+    MemoryDeviceClass *mdc_a = MEMORY_DEVICE_GET_CLASS(a);
+    MemoryDeviceClass *mdc_b = MEMORY_DEVICE_GET_CLASS(b);
+    const uint64_t addr_a = mdc_a->get_addr(md_a);
+    const uint64_t addr_b = mdc_b->get_addr(md_b);
+
+    if (addr_a > addr_b) {
+        return 1;
+    } else if (addr_a < addr_b) {

Quoted 9 lines

hw/mem/memory-device.c line 36

Quoted 6 lines

hw/mem/memory-device.c new file +6 -0

+        return -1;
+    }
+    return 0;
+}
+
+static int memory_device_built_list(Object *obj, void *opaque)

s/built/build/ will read a bit more clearly I think.

Indeed, thanks!

Quoted 41 lines · David Hildenbrand

Quoted 39 lines · David Gibson

Quoted 25 lines · David Hildenbrand

On the qmp level, we already have the concept of memory devices:
"query-memory-devices"
Right now, we only support NVDIMM and PCDIMM.

We want to map other devices later into the address space of the guest. Such device could e.g. be virtio devices. These devices will have a guest memory range assigned but won't be exposed via e.g. ACPI. We want to make them look like memory device, but not glued to pc-dimm.

Especially, it will not always be possible to have TYPE_PC_DIMM as a parent class (e.g. virtio devices). Let's use an interface instead. As a first part, convert handling of
- qmp_pc_dimm_device_list
- get_plugged_memory_size
to our new model. plug/unplug stuff etc. will follow later.

A memory device will have to provide the following functions:
- get_addr(): Necessary, as the property "addr" can e.g. not be used for virtio devices (already defined).
- get_plugged_size(): The amount this device offers to the guest as of now.
- get_region_size(): Because this can later on be bigger than the plugged size.
- fill_device_info(): Fill MemoryDeviceInfo, e.g. for qmp.

Quoted 1 line

Signed-off-by David Hildenbrand <[email protected]>

Reviewed-by David Gibson <[email protected]>

with the exception of some tiny nits..

[snip]

hw/mem/memory-device.c line 22

Quoted 4 lines

hw/mem/memory-device.c new file +4 -0

+static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
+{
+    MemoryDeviceState *md_a = MEMORY_DEVICE(a);
+    MemoryDeviceState *md_b = MEMORY_DEVICE(b);

These probably should be const MemoryDeviceState *.

Quoted 2 lines

Yes, that could work (depends on what the functions getting called internally expect). Will look into it

Well, the only thing being called here is the ->get_addr() hook, which you can redefine if necessary. Discarding the const in the passed in pointers seems like bad idea, even though it won't explicitly warn due to the existing MEMORY_DEVICE() cast.

Quoted 21 lines

Quoted 18 lines

Quoted 5 lines

hw/mem/memory-device.c new file +4 -0

+    MemoryDeviceClass *mdc_a = MEMORY_DEVICE_GET_CLASS(a);
+    MemoryDeviceClass *mdc_b = MEMORY_DEVICE_GET_CLASS(b);
+    const uint64_t addr_a = mdc_a->get_addr(md_a);
+    const uint64_t addr_b = mdc_b->get_addr(md_b);

hw/mem/memory-device.c line 36

Quoted 10 lines

hw/mem/memory-device.c new file +10 -0

+
+    if (addr_a > addr_b) {
+        return 1;
+    } else if (addr_a < addr_b) {
+        return -1;
+    }
+    return 0;
+}
+
+static int memory_device_built_list(Object *obj, void *opaque)

s/built/build/ will read a bit more clearly I think.

Indeed, thanks!

On the qmp level, we already have the concept of memory devices:
"query-memory-devices"
Right now, we only support NVDIMM and PCDIMM.

We want to map other devices later into the address space of the guest. Such device could e.g. be virtio devices. These devices will have a guest memory range assigned but won't be exposed via e.g. ACPI. We want to make them look like memory device, but not glued to pc-dimm.

Especially, it will not always be possible to have TYPE_PC_DIMM as a parent class (e.g. virtio devices). Let's use an interface instead. As a first part, convert handling of
- qmp_pc_dimm_device_list
- get_plugged_memory_size
to our new model. plug/unplug stuff etc. will follow later.

A memory device will have to provide the following functions:
- get_addr(): Necessary, as the property "addr" can e.g. not be used for virtio devices (already defined).
- get_plugged_size(): The amount this device offers to the guest as of now.
- get_region_size(): Because this can later on be bigger than the plugged size.
- fill_device_info(): Fill MemoryDeviceInfo, e.g. for qmp.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/acpi-build.c                         |   3 +-
 hw/mem/Makefile.objs                         |   1 +
 hw/mem/memory-device.c                       | 120
 +++++++++++++++++++++++++++
 hw/mem/pc-dimm.c                             | 119
 +++++++++++++-------------
 hw/ppc/spapr.c                               |   3 +-
 hw/ppc/spapr_hcall.c                         |   1 +
 include/hw/mem/memory-device.h               |  44 ++++++++++
 include/hw/mem/pc-dimm.h                     |   2 -
 numa.c                                       |   3 +-
 qmp.c                                        |   4 +-
 stubs/Makefile.objs                          |   2 +-
 stubs/{qmp_pc_dimm.c => qmp_memory_device.c} |   4 +-
 12 files changed, 240 insertions(+), 66 deletions(-)
 create mode 100644 hw/mem/memory-device.c
 create mode 100644 include/hw/mem/memory-device.h
 rename stubs/{qmp_pc_dimm.c => qmp_memory_device.c} (61%)

hw/i386/acpi-build.c +2 -1

index 3cf2a1679c..ca3645d57b 100644
@@ -46,6 +46,7 @@
 #include "hw/acpi/vmgenid.h"
 #include "sysemu/tpm_backend.h"
 #include "hw/timer/mc146818rtc_regs.h"
+#include "hw/mem/memory-device.h"
 #include "sysemu/numa.h"
 
 /* Supported chipsets: */

@@ -2253,7 +2254,7 @@ build_tpm2(GArray *table_data, BIOSLinker *linker,
 GArray *tcpalog)
 static void build_srat_hotpluggable_memory(GArray *table_data, uint64_t
 base,
                                            uint64_t len, int default_node)
 {
-    MemoryDeviceInfoList *info_list = qmp_pc_dimm_device_list();
+    MemoryDeviceInfoList *info_list = qmp_memory_device_list();
     MemoryDeviceInfoList *info;

     MemoryDeviceInfo *mi;
     PCDIMMDeviceInfo *di;

hw/mem/Makefile.objs +1 -0

index f12f8b97a2..10be4df2a2 100644
@@ -1,2 +1,3 @@
 common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o
+common-obj-$(CONFIG_MEM_HOTPLUG) += memory-device.o
 common-obj-$(CONFIG_NVDIMM) += nvdimm.o

hw/mem/memory-device.c new file +43 -0

new file mode 100644
index 0000000000..b860c9c582
@@ -0,0 +1,120 @@
+/*
+ * Memory Device Interface
+ *
+ * Copyright ProfitBricks GmbH 2012
+ * Copyright (C) 2014 Red Hat Inc
+ * Copyright (c) 2018 Red Hat Inc
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/mem/memory-device.h"
+#include "hw/qdev.h"
+#include "qapi/error.h"
+#include "hw/boards.h"
+#include "qemu/range.h"
+
+static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
+{
+    MemoryDeviceState *md_a = MEMORY_DEVICE(a);
+    MemoryDeviceState *md_b = MEMORY_DEVICE(b);
+    MemoryDeviceClass *mdc_a = MEMORY_DEVICE_GET_CLASS(a);
+    MemoryDeviceClass *mdc_b = MEMORY_DEVICE_GET_CLASS(b);
+    const uint64_t addr_a = mdc_a->get_addr(md_a);
+    const uint64_t addr_b = mdc_b->get_addr(md_b);
+
+    if (addr_a > addr_b) {
+        return 1;
+    } else if (addr_a < addr_b) {
+        return -1;
+    }
+    return 0;
+}
+
+static int memory_device_built_list(Object *obj, void *opaque)
+{
+    GSList **list = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
+        DeviceState *dev = DEVICE(obj);
+        if (dev->realized) { /* only realized memory devices matter */
+            *list = g_slist_insert_sorted(*list, dev,

memory_device_addr_sort);

hw/mem/memory-device.c new file +77 -0

+        }
+    }
+
+    object_child_foreach(obj, memory_device_built_list, opaque);
+    return 0;
+}
+
+MemoryDeviceInfoList *qmp_memory_device_list(void)
+{
+    GSList *devices = NULL, *item;
+    MemoryDeviceInfoList *list = NULL, *prev = NULL;
+
+    object_child_foreach(qdev_get_machine(), memory_device_built_list,
+                         &devices);
+
+    for (item = devices; item; item = g_slist_next(item)) {
+        MemoryDeviceState *md = MEMORY_DEVICE(item->data);
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(item->data);
+        MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
+        MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
+
+        mdc->fill_device_info(md, info);
+
+        elem->value = info;
+        elem->next = NULL;
+        if (prev) {
+            prev->next = elem;
+        } else {
+            list = elem;
+        }
+        prev = elem;
+    }
+
+    g_slist_free(devices);
+
+    return list;
+}
+
+static int memory_device_plugged_size(Object *obj, void *opaque)
+{
+    uint64_t *size = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
+        DeviceState *dev = DEVICE(obj);
+        MemoryDeviceState *md = MEMORY_DEVICE(obj);
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
+
+        if (dev->realized) {
+            *size += mdc->get_plugged_size(md, &error_abort);
+        }
+    }
+
+    object_child_foreach(obj, memory_device_plugged_size, opaque);
+    return 0;
+}
+
+uint64_t get_plugged_memory_size(void)
+{
+    uint64_t size = 0;
+
+    memory_device_plugged_size(qdev_get_machine(), &size);
+
+    return size;
+}
+
+static const TypeInfo memory_device_info = {
+    .name          = TYPE_MEMORY_DEVICE,
+    .parent        = TYPE_INTERFACE,
+    .class_size = sizeof(MemoryDeviceClass),
+};
+
+static void memory_device_register_types(void)
+{
+    type_register_static(&memory_device_info);
+}
+
+type_init(memory_device_register_types)

hw/mem/pc-dimm.c +1 -56

index 51350d9c2d..1dbf699e02 100644
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
+#include "hw/mem/memory-device.h"
 #include "qapi/error.h"
 #include "qemu/config-file.h"
 #include "qapi/visitor.h"

@@ -158,11 +159,6 @@ uint64_t pc_existing_dimms_capacity(Error **errp)
     return cap.size;
 }
 
-uint64_t get_plugged_memory_size(void)
-{
-    return pc_existing_dimms_capacity(&error_abort);
-}
-
 static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
 {
     unsigned long *bitmap = opaque;

@@ -238,57 +234,6 @@ static int pc_dimm_built_list(Object *obj, void *opaque)
     return 0;
 }
 
-MemoryDeviceInfoList *qmp_pc_dimm_device_list(void)
-{
-    GSList *dimms = NULL, *item;
-    MemoryDeviceInfoList *list = NULL, *prev = NULL;
-
-    object_child_foreach(qdev_get_machine(), pc_dimm_built_list, &dimms);
-
-    for (item = dimms; item; item = g_slist_next(item)) {
-        PCDIMMDevice *dimm = PC_DIMM(item->data);
-        Object *obj = OBJECT(dimm);
-        MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1);
-        MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1);
-        PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1);
-        bool is_nvdimm = object_dynamic_cast(obj, TYPE_NVDIMM);
-        DeviceClass *dc = DEVICE_GET_CLASS(obj);
-        DeviceState *dev = DEVICE(obj);
-
-        if (dev->id) {
-            di->has_id = true;
-            di->id = g_strdup(dev->id);
-        }
-        di->hotplugged = dev->hotplugged;
-        di->hotpluggable = dc->hotpluggable;
-        di->addr = dimm->addr;
-        di->slot = dimm->slot;
-        di->node = dimm->node;
-        di->size = object_property_get_uint(obj, PC_DIMM_SIZE_PROP, NULL);
-        di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
-
-        if (!is_nvdimm) {
-            info->u.dimm.data = di;
-            info->type = MEMORY_DEVICE_INFO_KIND_DIMM;
-        } else {
-            info->u.nvdimm.data = di;
-            info->type = MEMORY_DEVICE_INFO_KIND_NVDIMM;
-        }
-        elem->value = info;
-        elem->next = NULL;
-        if (prev) {
-            prev->next = elem;
-        } else {
-            list = elem;
-        }
-        prev = elem;
-    }
-
-    g_slist_free(dimms);
-
-    return list;
-}
-
 uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
                                uint64_t address_space_size,
                                uint64_t *hint, uint64_t align, uint64_t

size,

hw/mem/pc-dimm.c +52 -0

@@ -445,10 +390,62 @@ static MemoryRegion
 *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
     return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
 }
 
+static uint64_t pc_dimm_md_get_addr(MemoryDeviceState *md)
+{
+    PCDIMMDevice *dimm = PC_DIMM(md);
+
+    return dimm->addr;
+}
+
+static uint64_t pc_dimm_md_get_region_size(MemoryDeviceState *md, Error
 **errp)
+{
+    PCDIMMDevice *dimm = PC_DIMM(md);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
+    MemoryRegion *mr;
+
+    mr = ddc->get_memory_region(dimm, errp);
+    if (!mr) {
+        return 0;
+    }
+
+    return memory_region_size(mr);
+}
+
+static void pc_dimm_md_fill_device_info(MemoryDeviceState *md,
+                                        MemoryDeviceInfo *info)
+{
+    PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1);
+    DeviceClass *dc = DEVICE_GET_CLASS(md);
+    PCDIMMDevice *dimm = PC_DIMM(md);
+    DeviceState *dev = DEVICE(md);
+
+    if (dev->id) {
+        di->has_id = true;
+        di->id = g_strdup(dev->id);
+    }
+    di->hotplugged = dev->hotplugged;
+    di->hotpluggable = dc->hotpluggable;
+    di->addr = dimm->addr;
+    di->slot = dimm->slot;
+    di->node = dimm->node;
+    di->size = object_property_get_uint(OBJECT(dimm), PC_DIMM_SIZE_PROP,
+                                        NULL);
+    di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+        info->u.nvdimm.data = di;
+        info->type = MEMORY_DEVICE_INFO_KIND_NVDIMM;
+    } else {
+        info->u.dimm.data = di;
+        info->type = MEMORY_DEVICE_INFO_KIND_DIMM;
+    }
+}
+
 static void pc_dimm_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
+    MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc);
 

     dc->realize = pc_dimm_realize;
     dc->unrealize = pc_dimm_unrealize;

hw/mem/pc-dimm.c +6 -0

@@ -457,6 +454,12 @@ static void pc_dimm_class_init(ObjectClass *oc, void
 *data)
 
     ddc->get_memory_region = pc_dimm_get_memory_region;
     ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
+
+    mdc->get_addr = pc_dimm_md_get_addr;
+    /* for a dimm plugged_size == region_size */
+    mdc->get_plugged_size = pc_dimm_md_get_region_size;
+    mdc->get_region_size = pc_dimm_md_get_region_size;
+    mdc->fill_device_info = pc_dimm_md_fill_device_info;
 }
 

static TypeInfo pc_dimm_info = {

hw/mem/pc-dimm.c +4 -0

@@ -466,6 +469,10 @@ static TypeInfo pc_dimm_info = {
     .instance_init = pc_dimm_init,
     .class_init    = pc_dimm_class_init,
     .class_size    = sizeof(PCDIMMDeviceClass),
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_MEMORY_DEVICE },
+        { }
+    },
 };
 
 static void pc_dimm_register_types(void)

hw/ppc/spapr.c +2 -1

index a81570e7c8..a7428f7da7 100644
@@ -74,6 +74,7 @@
 #include "hw/compat.h"
 #include "qemu/cutils.h"
 #include "hw/ppc/spapr_cpu_core.h"
+#include "hw/mem/memory-device.h"
 
 #include <libfdt.h>
 

@@ -722,7 +723,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState
 *spapr, void *fdt)
     }
 
     if (hotplug_lmb_start) {
-        dimms = qmp_pc_dimm_device_list();
+        dimms = qmp_memory_device_list();
     }
 

     /* ibm,dynamic-memory */

hw/ppc/spapr_hcall.c +1 -0

index 16bccdd5c0..4cdae3ca3a 100644
@@ -14,6 +14,7 @@
 #include "kvm_ppc.h"
 #include "hw/ppc/spapr_ovec.h"
 #include "mmu-book3s-v3.h"
+#include "hw/mem/memory-device.h"
 
 struct SPRSyncState {
     int spr;

include/hw/mem/memory-device.h new file +27 -0

new file mode 100644
index 0000000000..3e498b2e61
@@ -0,0 +1,44 @@
+/*
+ * Memory Device Interface
+ *
+ * Copyright (c) 2018 Red Hat, Inc.
+ *
+ * Authors:
+ *  David Hildenbrand <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef MEMORY_DEVICE_H
+#define MEMORY_DEVICE_H
+
+#include "qom/object.h"
+#include "hw/qdev.h"
+
+#define TYPE_MEMORY_DEVICE "memory-device"
+
+#define MEMORY_DEVICE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(MemoryDeviceClass, (klass), TYPE_MEMORY_DEVICE)
+#define MEMORY_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MemoryDeviceClass, (obj), TYPE_MEMORY_DEVICE)
+#define MEMORY_DEVICE(obj) \
+     INTERFACE_CHECK(MemoryDeviceState, (obj), TYPE_MEMORY_DEVICE)
+

include/hw/mem/memory-device.h new file +7 -0

+    void (*fill_device_info)(MemoryDeviceState *md, MemoryDeviceInfo *info);
+} MemoryDeviceClass;
+
+MemoryDeviceInfoList *qmp_memory_device_list(void);
+uint64_t get_plugged_memory_size(void);
+
+#endif

include/hw/mem/pc-dimm.h +0 -2

index 1fc479281c..e88073321f 100644
@@ -93,9 +93,7 @@ uint64_t pc_dimm_get_free_addr(uint64_t
 address_space_start,
 
 int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
-MemoryDeviceInfoList *qmp_pc_dimm_device_list(void);
 uint64_t pc_existing_dimms_capacity(Error **errp);
-uint64_t get_plugged_memory_size(void);
 void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
                          MemoryRegion *mr, uint64_t align, Error **errp);

void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,

numa.c +2 -1

index 1116c90af9..6a0eaebc01 100644
@@ -36,6 +36,7 @@
 #include "hw/boards.h"
 #include "sysemu/hostmem.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/memory-device.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qemu/cutils.h"

@@ -520,7 +521,7 @@ void memory_region_allocate_system_memory(MemoryRegion
 *mr, Object *owner,
 
 static void numa_stat_memory_devices(NumaNodeMem node_mem[])
 {
-    MemoryDeviceInfoList *info_list = qmp_pc_dimm_device_list();
+    MemoryDeviceInfoList *info_list = qmp_memory_device_list();
     MemoryDeviceInfoList *info;
     PCDIMMDeviceInfo     *pcdimm_info;

qmp.c +2 -2

index f72261667f..3de029946a 100644
@@ -39,7 +39,7 @@
 #include "qapi/qobject-input-visitor.h"
 #include "hw/boards.h"
 #include "qom/object_interfaces.h"
-#include "hw/mem/pc-dimm.h"
+#include "hw/mem/memory-device.h"
 #include "hw/acpi/acpi_dev_interface.h"
 
 NameInfo *qmp_query_name(Error **errp)

@@ -731,7 +731,7 @@ void qmp_object_del(const char *id, Error **errp)
 
 MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
 {
-    return qmp_pc_dimm_device_list();
+    return qmp_memory_device_list();
 }
 
 ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error **errp)

stubs/Makefile.objs +1 -1

index 2d59d84091..53d3f32cb2 100644
@@ -34,7 +34,7 @@ stub-obj-y += uuid.o
 stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
-stub-obj-y += qmp_pc_dimm.o
+stub-obj-y += qmp_memory_device.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += pc_madt_cpu_entry.o

stubs/qmp_memory_device.c renamed from stubs/qmp_pc_dimm.c +2 -3

similarity index 61%
rename from stubs/qmp_pc_dimm.c
rename to stubs/qmp_memory_device.c
index b6b2cca89e..85ff8f2d7e 100644
@@ -1,8 +1,8 @@
 #include "qemu/osdep.h"
 #include "qom/object.h"
-#include "hw/mem/pc-dimm.h"
+#include "hw/mem/memory-device.h"
 
-MemoryDeviceInfoList *qmp_pc_dimm_device_list(void)
+MemoryDeviceInfoList *qmp_memory_device_list(void)
 {
    return NULL;
 }
--

2.14.3

include/hw/mem/memory-device.h line 37

Quoted 6 lines

include/hw/mem/memory-device.h new file +6 -0

+typedef struct MemoryDeviceClass {
+    InterfaceClass parent_class;
+
+    uint64_t (*get_addr)(MemoryDeviceState *md);
+    uint64_t (*get_plugged_size)(MemoryDeviceState *md, Error **errp);
+    uint64_t (*get_region_size)(MemoryDeviceState *md, Error **errp);

Just not sure if we need second argument 'Error **errp'? Or all functions declarations should have this?

Let's allow to query the MemoryHotplugState from the machine.

This allows us to generically detect if a certain machine has support for memory devices, and to generically manage it (find free address range, plug/unplug a memory region).

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c             | 12 ++++++++++++
 hw/ppc/spapr.c           | 12 ++++++++++++
 include/hw/boards.h      | 16 ++++++++++++++++
 include/hw/mem/pc-dimm.h | 12 +-----------
 4 files changed, 41 insertions(+), 11 deletions(-)

hw/i386/pc.c +12 -0

index d36bac8c89..fa8862af33 100644
@@ -2337,6 +2337,17 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+static MemoryHotplugState *pc_machine_get_memory_hotplug_state(MachineState *ms)
+{
+    PCMachineState *pcms = PC_MACHINE(ms);
+
+    if (!pcms->hotplug_memory.base) {
+        /* hotplug not supported */
+        return NULL;
+    }
+    return &pcms->hotplug_memory;
+}
+
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);

@@ -2376,6 +2387,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = pc_machine_device_unplug_cb;
     nc->nmi_monitor_handler = x86_nmi;
     mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
+    mc->get_memory_hotplug_state = pc_machine_get_memory_hotplug_state;
 
     object_class_property_add(oc, PC_MACHINE_MEMHP_REGION_SIZE, "int",
         pc_machine_get_hotplug_memory_region_size, NULL,

hw/ppc/spapr.c +12 -0

index a7428f7da7..7ccdb705b3 100644
@@ -3868,6 +3868,17 @@ PowerPCCPU *spapr_find_cpu(int vcpu_id)
     return NULL;
 }
 
+static MemoryHotplugState *spapr_get_memory_hotplug_state(MachineState *ms)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
+
+    if (!spapr->hotplug_memory.base) {
+        /* hotplug not supported */
+        return NULL;
+    }
+    return &spapr->hotplug_memory;
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);

@@ -3927,6 +3938,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
      * in which LMBs are represented and hot-added
      */
     mc->numa_mem_align_shift = 28;
+    mc->get_memory_hotplug_state = spapr_get_memory_hotplug_state;
 
     smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
     smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;

include/hw/boards.h +16 -0

index a609239112..447bdc7219 100644
@@ -105,6 +105,17 @@ typedef struct {
     CPUArchId cpus[0];
 } CPUArchIdList;
 
+/**
+ * MemoryHotplugState:
+ * @base: address in guest physical address space where hotplug memory
+ * address space begins.
+ * @mr: hotplug memory address space container
+ */
+typedef struct MemoryHotplugState {
+    hwaddr base;
+    MemoryRegion mr;
+} MemoryHotplugState;
+
 /**
  * MachineClass:
  * @max_cpus: maximum number of CPUs supported. Default: 1

@@ -156,6 +167,10 @@ typedef struct {
  *    should instead use "unimplemented-device" for all memory ranges where
  *    the guest will attempt to probe for a device that QEMU doesn't
  *    implement and a stub device is required.
+ * @get_memory_hotplug_state:
+ *    If a machine support memory hotplug, the returned data ontains
+ *    information about the portion of guest physical address space
+ *    where memory devices can be mapped to (e.g. to hotplug a pc-dimm).
  */
 struct MachineClass {
     /*< private >*/

@@ -212,6 +227,7 @@ struct MachineClass {
                                                          unsigned cpu_index);
     const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
     int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
+    MemoryHotplugState *(*get_memory_hotplug_state)(MachineState *ms);
 };
 
 /**

include/hw/mem/pc-dimm.h +1 -11

index e88073321f..8bda37adab 100644
@@ -19,6 +19,7 @@
 #include "exec/memory.h"
 #include "sysemu/hostmem.h"
 #include "hw/qdev.h"
+#include "hw/boards.h"
 
 #define TYPE_PC_DIMM "pc-dimm"
 #define PC_DIMM(obj) \

@@ -75,17 +76,6 @@ typedef struct PCDIMMDeviceClass {
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;
 
-/**
- * MemoryHotplugState:
- * @base: address in guest physical address space where hotplug memory
- * address space begins.
- * @mr: hotplug memory address space container
- */
-typedef struct MemoryHotplugState {
-    hwaddr base;
-    MemoryRegion mr;
-} MemoryHotplugState;
-
 uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
                                uint64_t address_space_size,
                                uint64_t *hint, uint64_t align, uint64_t size,

Quoted 6 lines · David Hildenbrand

Let's allow to query the MemoryHotplugState from the machine.

This allows us to generically detect if a certain machine has support for memory devices, and to generically manage it (find free address range, plug/unplug a memory region).

Quoted 1 line

Signed-off-by David Hildenbrand <[email protected]>

So, we're creating a hook where it seems very likely that the only implementationss will be simply to retrieve the right field from the machine specific structure.

So.. should we instead just move the hotplug_memory structure to the based MachineState type?

Quoted 7 lines · David Gibson

Quoted 6 lines · David Hildenbrand

Let's allow to query the MemoryHotplugState from the machine.

This allows us to generically detect if a certain machine has support for memory devices, and to generically manage it (find free address range, plug/unplug a memory region).

Quoted 8 lines

Quoted 1 line

Signed-off-by David Hildenbrand <[email protected]>

So, we're creating a hook where it seems very likely that the only implementationss will be simply to retrieve the right field from the machine specific structure.

So.. should we instead just move the hotplug_memory structure to the based MachineState type?

It allows us in patch nr. 3 to report different error messages.

"Not supported" vs. "Not enabled (maxmem)".

We could also handle that via a simple boolean flag inside of the struct. What do you think?

Quoted 19 lines · David Hildenbrand

Quoted 7 lines · David Gibson

Quoted 6 lines · David Hildenbrand

Let's allow to query the MemoryHotplugState from the machine.

This allows us to generically detect if a certain machine has support for memory devices, and to generically manage it (find free address range, plug/unplug a memory region).

Quoted 8 lines

Quoted 1 line

Signed-off-by David Hildenbrand <[email protected]>

So, we're creating a hook where it seems very likely that the only implementationss will be simply to retrieve the right field from the machine specific structure.

So.. should we instead just move the hotplug_memory structure to the based MachineState type?

It allows us in patch nr. 3 to report different error messages.

Quoted 4 lines

"Not supported" vs. "Not enabled (maxmem)".

We could also handle that via a simple boolean flag inside of the struct. What do you think?

A third option would be to make it a pointer, rather than directly embedded in the MachineState. That would also avoid the extra allocation for machines that don't use it.

To be able to reuse MemoryDevice logic from other devices besides pc-dimm, factor the relevant stuff out into the MemoryDevice code.

As we don't care about slots for memory devices that are not pc-dimm, don't factor that part out.

Most of this patch just moves checks and logic around. While at it, make the code properly detect certain error conditions better (e.g. fragmented memory).

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c                   |  12 +--
 hw/mem/memory-device.c         | 162 ++++++++++++++++++++++++++++++++++++
 hw/mem/pc-dimm.c               | 185 +++--------------------------------------
 hw/ppc/spapr.c                 |   9 +-
 include/hw/mem/memory-device.h |   4 +
 include/hw/mem/pc-dimm.h       |  14 +---
 6 files changed, 185 insertions(+), 201 deletions(-)

hw/i386/pc.c +0 -0

index fa8862af33..1c25546a0c 100644

hw/i386/pc.c +0 -8

     if (local_err) {
         goto out;
     }

@@ -1761,17 +1761,9 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
                            DeviceState *dev, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    PCDIMMDevice *dimm = PC_DIMM(dev);
-    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 

hw/i386/pc.c +0 -0

     object_unparent(OBJECT(dev));
 
  out:

hw/mem/memory-device.c +21 -0

index b860c9c582..b96efa3bf4 100644
@@ -15,6 +15,8 @@
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "qemu/range.h"
+#include "hw/virtio/vhost.h"
+#include "sysemu/kvm.h"
 
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {

@@ -106,6 +108,166 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+static int memory_device_used_region_size_internal(Object *obj, void *opaque)
+{
+    uint64_t *size = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
+        DeviceState *dev = DEVICE(obj);
+        MemoryDeviceState *md = MEMORY_DEVICE(obj);
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
+
+        if (dev->realized) {
+            *size += mdc->get_region_size(md, &error_abort);
+        }
+    }
+
+    object_child_foreach(obj, memory_device_used_region_size_internal, opaque);
+    return 0;
+}
+
+static uint64_t memory_device_used_region_size(void)

hw/mem/memory-device.c +11 -0

+{
+    const uint64_t used_region_size = memory_device_used_region_size();
+    uint64_t address_space_start, address_space_end;
+    MachineState *machine = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    MemoryHotplugState *hpms;
+    GSList *list = NULL, *item;
+    uint64_t new_addr = 0;
+
+    if (!mc->get_memory_hotplug_state) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "

hw/mem/memory-device.c +74 -0

+    address_space_start = hpms->base;
+    address_space_end = address_space_start + memory_region_size(&hpms->mr);
+    g_assert(address_space_end >= address_space_start);
+
+    if (used_region_size + size > machine->maxram_size - machine->ram_size) {
+        error_setg(errp, "not enough space, currently 0x%" PRIx64
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   used_region_size, machine->maxram_size - machine->ram_size);
+        return 0;
+    }
+
+    if (hint && QEMU_ALIGN_UP(*hint, align) != *hint) {
+        error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
+                   align);
+        return 0;
+    }
+
+    if (QEMU_ALIGN_UP(size, align) != size) {
+        error_setg(errp, "backend memory size must be multiple of 0x%"
+                   PRIx64, align);
+        return 0;
+    }
+
+    if (hint) {
+        new_addr = *hint;
+        if (new_addr < address_space_start) {
+            error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
+                       "] at 0x%" PRIx64, new_addr, size, address_space_start);
+            return 0;
+        } else if ((new_addr + size) > address_space_end) {
+            error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
+                       "] beyond 0x%" PRIx64, new_addr, size,
+                       address_space_end);
+            return 0;
+        }
+    } else {
+        new_addr = address_space_start;
+    }
+
+    /* find address range that will fit new memory device */
+    object_child_foreach(qdev_get_machine(), memory_device_built_list, &list);
+    for (item = list; item; item = g_slist_next(item)) {
+        MemoryDeviceState *md = item->data;
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
+        uint64_t md_size, md_addr;
+
+        md_addr = mdc->get_addr(md);
+        md_size = mdc->get_region_size(md, errp);
+        if (*errp) {
+            goto out;
+        }
+
+        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
+            if (hint) {
+                DeviceState *d = DEVICE(md);
+                error_setg(errp, "address range conflicts with '%s'", d->id);
+                goto out;
+            }
+            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+        }
+    }
+
+    if (new_addr + size > address_space_end) {
+        error_setg(errp, "could not find position in guest address space for "
+                   "memory device - memory fragmented due to alignments");
+        goto out;
+    }
+out:
+    g_slist_free(list);
+    return new_addr;
+}
+
+void memory_device_plug_region(MemoryRegion *mr, uint64_t addr, Error **errp)
+{

hw/mem/memory-device.c +16 -0

+}
+
+void memory_device_unplug_region(MemoryRegion *mr)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    MemoryHotplugState *hpms;
+
+    /* we expect a previous call to memory_device_get_free_addr() */
+    g_assert(mc->get_memory_hotplug_state);
+    hpms = mc->get_memory_hotplug_state(machine);
+    g_assert(hpms);
+
+    memory_region_del_subregion(&hpms->mr, mr);
+}
+
 static const TypeInfo memory_device_info = {
     .name          = TYPE_MEMORY_DEVICE,
     .parent        = TYPE_INTERFACE,

hw/mem/pc-dimm.c +12 -173

index 1dbf699e02..cf23ab5d76 100644
@@ -25,19 +25,10 @@
 #include "qapi/error.h"
 #include "qemu/config-file.h"
 #include "qapi/visitor.h"
-#include "qemu/range.h"
 #include "sysemu/numa.h"
-#include "sysemu/kvm.h"
 #include "trace.h"
-#include "hw/virtio/vhost.h"
 
-typedef struct pc_dimms_capacity {
-     uint64_t size;
-     Error    **errp;
-} pc_dimms_capacity;
-
-void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
-                         MemoryRegion *mr, uint64_t align, Error **errp)
+void pc_dimm_memory_plug(DeviceState *dev, uint64_t align, Error **errp)
 {
     int slot;
     MachineState *machine = MACHINE(qdev_get_machine());

@@ -45,37 +36,26 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
     Error *local_err = NULL;
-    uint64_t existing_dimms_capacity = 0;
+    MemoryRegion *mr;
     uint64_t addr;
 
-    addr = object_property_get_uint(OBJECT(dimm),
-                                    PC_DIMM_ADDR_PROP, &local_err);
+    mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {
         goto out;
     }
 
-    addr = pc_dimm_get_free_addr(hpms->base,
-                                 memory_region_size(&hpms->mr),
-                                 !addr ? NULL : &addr, align,
-                                 memory_region_size(mr), &local_err);
+    addr = object_property_get_uint(OBJECT(dimm),
+                                    PC_DIMM_ADDR_PROP, &local_err);
     if (local_err) {
         goto out;
     }
 
-    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
+    addr = memory_device_get_free_addr(!addr ? NULL : &addr, align,
+                                       memory_region_size(mr), &local_err);
     if (local_err) {
         goto out;
     }
 
-    if (existing_dimms_capacity + memory_region_size(mr) >
-        machine->maxram_size - machine->ram_size) {
-        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
-                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
-                   existing_dimms_capacity,
-                   machine->maxram_size - machine->ram_size);
-        goto out;
-    }
-
     object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
     if (local_err) {
         goto out;

@@ -98,67 +78,27 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
     }
     trace_mhp_pc_dimm_assigned_slot(slot);
 
-    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
-        error_setg(&local_err, "hypervisor has no free memory slots left");
-        goto out;
-    }
-
-    if (!vhost_has_free_slot()) {
-        error_setg(&local_err, "a used vhost backend has no free"
-                               " memory slots left");
+    memory_device_plug_region(mr, addr, &local_err);
+    if (local_err) {
         goto out;
     }
-
-    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
     vmstate_register_ram(vmstate_mr, dev);
 
 out:
     error_propagate(errp, local_err);
 }
 
-void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
-                           MemoryRegion *mr)
+void pc_dimm_memory_unplug(DeviceState *dev)
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
 
-    memory_region_del_subregion(&hpms->mr, mr);
+    memory_device_unplug_region(mr);
     vmstate_unregister_ram(vmstate_mr, dev);
 }
 
-static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
-{
-    pc_dimms_capacity *cap = opaque;
-    uint64_t *size = &cap->size;
-
-    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
-        DeviceState *dev = DEVICE(obj);
-
-        if (dev->realized) {
-            (*size) += object_property_get_uint(obj, PC_DIMM_SIZE_PROP,
-                cap->errp);
-        }
-
-        if (cap->errp && *cap->errp) {
-            return 1;
-        }
-    }
-    object_child_foreach(obj, pc_existing_dimms_capacity_internal, opaque);
-    return 0;
-}
-
-uint64_t pc_existing_dimms_capacity(Error **errp)
-{
-    pc_dimms_capacity cap;
-
-    cap.size = 0;
-    cap.errp = errp;
-
-    pc_existing_dimms_capacity_internal(qdev_get_machine(), &cap);
-    return cap.size;
-}
-
 static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
 {
     unsigned long *bitmap = opaque;

@@ -205,107 +145,6 @@ out:
     return slot;
 }
 
-static gint pc_dimm_addr_sort(gconstpointer a, gconstpointer b)
-{
-    PCDIMMDevice *x = PC_DIMM(a);
-    PCDIMMDevice *y = PC_DIMM(b);
-    Int128 diff = int128_sub(int128_make64(x->addr), int128_make64(y->addr));
-
-    if (int128_lt(diff, int128_zero())) {
-        return -1;
-    } else if (int128_gt(diff, int128_zero())) {
-        return 1;
-    }
-    return 0;
-}
-
-static int pc_dimm_built_list(Object *obj, void *opaque)
-{
-    GSList **list = opaque;
-
-    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
-        DeviceState *dev = DEVICE(obj);
-        if (dev->realized) { /* only realized DIMMs matter */
-            *list = g_slist_insert_sorted(*list, dev, pc_dimm_addr_sort);
-        }
-    }
-
-    object_child_foreach(obj, pc_dimm_built_list, opaque);
-    return 0;
-}
-
-uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
-                               uint64_t address_space_size,
-                               uint64_t *hint, uint64_t align, uint64_t size,
-                               Error **errp)
-{
-    GSList *list = NULL, *item;
-    uint64_t new_addr, ret = 0;
-    uint64_t address_space_end = address_space_start + address_space_size;
-
-    g_assert(QEMU_ALIGN_UP(address_space_start, align) == address_space_start);
-
-    if (!address_space_size) {
-        error_setg(errp, "memory hotplug is not enabled, "
-                         "please add maxmem option");
-        goto out;
-    }
-
-    if (hint && QEMU_ALIGN_UP(*hint, align) != *hint) {
-        error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
-                   align);
-        goto out;
-    }
-
-    if (QEMU_ALIGN_UP(size, align) != size) {
-        error_setg(errp, "backend memory size must be multiple of 0x%"
-                   PRIx64, align);
-        goto out;
-    }
-
-    assert(address_space_end > address_space_start);
-    object_child_foreach(qdev_get_machine(), pc_dimm_built_list, &list);
-
-    if (hint) {
-        new_addr = *hint;
-    } else {
-        new_addr = address_space_start;
-    }
-
-    /* find address range that will fit new DIMM */
-    for (item = list; item; item = g_slist_next(item)) {
-        PCDIMMDevice *dimm = item->data;
-        uint64_t dimm_size = object_property_get_uint(OBJECT(dimm),
-                                                      PC_DIMM_SIZE_PROP,
-                                                      errp);
-        if (errp && *errp) {
-            goto out;
-        }
-
-        if (ranges_overlap(dimm->addr, dimm_size, new_addr, size)) {
-            if (hint) {
-                DeviceState *d = DEVICE(dimm);
-                error_setg(errp, "address range conflicts with '%s'", d->id);
-                goto out;
-            }
-            new_addr = QEMU_ALIGN_UP(dimm->addr + dimm_size, align);
-        }
-    }
-    ret = new_addr;
-
-    if (new_addr < address_space_start) {
-        error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
-                   "] at 0x%" PRIx64, new_addr, size, address_space_start);
-    } else if ((new_addr + size) > address_space_end) {
-        error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
-                   "] beyond 0x%" PRIx64, new_addr, size, address_space_end);
-    }
-
-out:
-    g_slist_free(list);
-    return ret;
-}
-
 static Property pc_dimm_properties[] = {
     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
     DEFINE_PROP_UINT32(PC_DIMM_NODE_PROP, PCDIMMDevice, node, 0),

hw/ppc/spapr.c +3 -6

index 7ccdb705b3..7757a49335 100644
@@ -3041,7 +3041,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
-    pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
+    pc_dimm_memory_plug(dev, align, &local_err);
     if (local_err) {
         goto out;
     }

@@ -3062,7 +3062,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     return;
 
 out_unplug:
-    pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr);
+    pc_dimm_memory_unplug(dev);
 out:
     error_propagate(errp, local_err);
 }

@@ -3180,9 +3180,6 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 void spapr_lmb_release(DeviceState *dev)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
-    PCDIMMDevice *dimm = PC_DIMM(dev);
-    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3202,7 +3199,7 @@ void spapr_lmb_release(DeviceState *dev)
      * Now that all the LMBs have been removed by the guest, call the
      * pc-dimm unplug handler to cleanup up the pc-dimm device.
      */
-    pc_dimm_memory_unplug(dev, &spapr->hotplug_memory, mr);
+    pc_dimm_memory_unplug(dev);
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

include/hw/mem/memory-device.h +4 -0

index 3e498b2e61..722620da24 100644
@@ -40,5 +40,9 @@ typedef struct MemoryDeviceClass {
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
+uint64_t memory_device_get_free_addr(uint64_t *hint, uint64_t align,
+                                     uint64_t size, Error **errp);
+void memory_device_plug_region(MemoryRegion *mr, uint64_t addr, Error **errp);
+void memory_device_unplug_region(MemoryRegion *mr);
 
 #endif

include/hw/mem/pc-dimm.h +2 -12

index 8bda37adab..2e7c2abe35 100644
@@ -19,7 +19,6 @@
 #include "exec/memory.h"
 #include "sysemu/hostmem.h"
 #include "hw/qdev.h"
-#include "hw/boards.h"
 
 #define TYPE_PC_DIMM "pc-dimm"
 #define PC_DIMM(obj) \

@@ -76,16 +75,7 @@ typedef struct PCDIMMDeviceClass {
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;
 
-uint64_t pc_dimm_get_free_addr(uint64_t address_space_start,
-                               uint64_t address_space_size,
-                               uint64_t *hint, uint64_t align, uint64_t size,
-                               Error **errp);
-
 int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
-
-uint64_t pc_existing_dimms_capacity(Error **errp);
-void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
-                         MemoryRegion *mr, uint64_t align, Error **errp);
-void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
-                           MemoryRegion *mr);
+void pc_dimm_memory_plug(DeviceState *dev, uint64_t align, Error **errp);
+void pc_dimm_memory_unplug(DeviceState *dev);
 #endif

Quoted 5 lines · On Fri, 20 Apr 2018 14:34:56 +0200 David Hildenbrand <[email protected]> wrote:

To be able to reuse MemoryDevice logic from other devices besides pc-dimm, factor the relevant stuff out into the MemoryDevice code.

As we don't care about slots for memory devices that are not pc-dimm, don't factor that part out.

that's not really true, you still consume kvm and vhost slots (whatever it is) whenever you map it into address space as ram memory region.

Also ram_slots currently are (ab)used as flag that user enabled memory hotplug via CLI.

Quoted 4 lines

Most of this patch just moves checks and logic around. While at it, make the code properly detect certain error conditions better (e.g. fragmented memory).

I'd suggest splitting patch in several smaller ones if possible, especially parts that do anything more than just moving code around.

Quoted 20 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c                   |  12 +--
 hw/mem/memory-device.c         | 162 ++++++++++++++++++++++++++++++++++++
 hw/mem/pc-dimm.c               | 185 +++--------------------------------------
 hw/ppc/spapr.c                 |   9 +-
 include/hw/mem/memory-device.h |   4 +
 include/hw/mem/pc-dimm.h       |  14 +---
 6 files changed, 185 insertions(+), 201 deletions(-)

hw/i386/pc.c +0 -0

index fa8862af33..1c25546a0c 100644
@@ -1711,7 +1711,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 

hw/i386/pc.c static void pc_dimm_plug(HotplugHandler *hotplug_dev, · line 1714

Quoted 2 lines

hw/i386/pc.c +1 -1

-    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
+    pc_dimm_memory_plug(dev, align, &local_err);

Is there a reason why you are dropping pcms->hotplug_memory argument and fall back to qdev_get_machine()?

I'd rather see it going other direction,
i.e. move hotplug_memory from PC
machine to MachineState and then pass it down as argument whenever it's needed.

Quoted 22 lines

hw/i386/pc.c +0 -8

     if (local_err) {
         goto out;
     }

@@ -1761,17 +1761,9 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
                            DeviceState *dev, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    PCDIMMDevice *dimm = PC_DIMM(dev);
-    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
 

hw/i386/pc.c static void pc_dimm_unplug(HotplugHandler *hotplug_dev, · line 1774

Quoted 6 lines

hw/i386/pc.c +1 -1

@@ -1779,7 +1771,7 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
+    pc_dimm_memory_unplug(dev);

ditto

Quoted 46 lines

hw/i386/pc.c +0 -0

     object_unparent(OBJECT(dev));
 
  out:

hw/mem/memory-device.c +27 -0

index b860c9c582..b96efa3bf4 100644
@@ -15,6 +15,8 @@
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "qemu/range.h"
+#include "hw/virtio/vhost.h"
+#include "sysemu/kvm.h"
 
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {

@@ -106,6 +108,166 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+static int memory_device_used_region_size_internal(Object *obj, void *opaque)
+{
+    uint64_t *size = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
+        DeviceState *dev = DEVICE(obj);
+        MemoryDeviceState *md = MEMORY_DEVICE(obj);
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
+
+        if (dev->realized) {
+            *size += mdc->get_region_size(md, &error_abort);
+        }
+    }
+
+    object_child_foreach(obj, memory_device_used_region_size_internal, opaque);
+    return 0;
+}
+
+static uint64_t memory_device_used_region_size(void)
+{
+    uint64_t size = 0;
+
+    memory_device_used_region_size_internal(qdev_get_machine(), &size);
+
+    return size;

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 139

Quoted 4 lines

hw/mem/memory-device.c +4 -0

+}
+
+uint64_t memory_device_get_free_addr(uint64_t *hint, uint64_t align,
+                                     uint64_t size, Error **errp)

I'd suggest to pc_dimm_memory_plug/pc_dimm_get_free_addr first, namely most of the stuff it does like checks and assigning default values should go to pre_plug (pre realize) handler and then only actual mapping is left for plug (after realize) handler to deal with:

    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);               
    vmstate_register_ram(vmstate_mr, dev); 

Quoted 15 lines

hw/mem/memory-device.c +15 -0

+{
+    const uint64_t used_region_size = memory_device_used_region_size();
+    uint64_t address_space_start, address_space_end;
+    MachineState *machine = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    MemoryHotplugState *hpms;
+    GSList *list = NULL, *item;
+    uint64_t new_addr = 0;
+
+    if (!mc->get_memory_hotplug_state) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "supported by the machine");
+        return 0;
+    }
+

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 160

Quoted 6 lines

hw/mem/memory-device.c +6 -0

+    hpms = mc->get_memory_hotplug_state(machine);
+    if (!hpms || !memory_region_size(&hpms->mr)) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "enabled, please specify the maxmem option");
+        return 0;
+    }

above 2 checks are repeated multiple times separate helper to do it would be better.

PS:
there is no need to check for it every time device is plugged, doing this check once (at machine_init time) is sufficient.

Quoted 75 lines

hw/mem/memory-device.c +74 -0

+    address_space_start = hpms->base;
+    address_space_end = address_space_start + memory_region_size(&hpms->mr);
+    g_assert(address_space_end >= address_space_start);
+
+    if (used_region_size + size > machine->maxram_size - machine->ram_size) {
+        error_setg(errp, "not enough space, currently 0x%" PRIx64
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   used_region_size, machine->maxram_size - machine->ram_size);
+        return 0;
+    }
+
+    if (hint && QEMU_ALIGN_UP(*hint, align) != *hint) {
+        error_setg(errp, "address must be aligned to 0x%" PRIx64 " bytes",
+                   align);
+        return 0;
+    }
+
+    if (QEMU_ALIGN_UP(size, align) != size) {
+        error_setg(errp, "backend memory size must be multiple of 0x%"
+                   PRIx64, align);
+        return 0;
+    }
+
+    if (hint) {
+        new_addr = *hint;
+        if (new_addr < address_space_start) {
+            error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
+                       "] at 0x%" PRIx64, new_addr, size, address_space_start);
+            return 0;
+        } else if ((new_addr + size) > address_space_end) {
+            error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
+                       "] beyond 0x%" PRIx64, new_addr, size,
+                       address_space_end);
+            return 0;
+        }
+    } else {
+        new_addr = address_space_start;
+    }
+
+    /* find address range that will fit new memory device */
+    object_child_foreach(qdev_get_machine(), memory_device_built_list, &list);
+    for (item = list; item; item = g_slist_next(item)) {
+        MemoryDeviceState *md = item->data;
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md));
+        uint64_t md_size, md_addr;
+
+        md_addr = mdc->get_addr(md);
+        md_size = mdc->get_region_size(md, errp);
+        if (*errp) {
+            goto out;
+        }
+
+        if (ranges_overlap(md_addr, md_size, new_addr, size)) {
+            if (hint) {
+                DeviceState *d = DEVICE(md);
+                error_setg(errp, "address range conflicts with '%s'", d->id);
+                goto out;
+            }
+            new_addr = QEMU_ALIGN_UP(md_addr + md_size, align);
+        }
+    }
+
+    if (new_addr + size > address_space_end) {
+        error_setg(errp, "could not find position in guest address space for "
+                   "memory device - memory fragmented due to alignments");
+        goto out;
+    }
+out:
+    g_slist_free(list);
+    return new_addr;
+}
+
+void memory_device_plug_region(MemoryRegion *mr, uint64_t addr, Error **errp)
+{

[...]

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 252

Quoted 9 lines

hw/mem/memory-device.c +9 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }

move these checks to pre_plug time

Quoted 2 lines · Igor Mammedov

On Fri, 20 Apr 2018 14:34:56 +0200
David Hildenbrand <[email protected]> wrote:

Quoted 42 lines

Quoted 42 lines

hw/mem/memory-device.c +27 -0

index b860c9c582..b96efa3bf4 100644
@@ -15,6 +15,8 @@
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "qemu/range.h"
+#include "hw/virtio/vhost.h"
+#include "sysemu/kvm.h"
 
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {

@@ -106,6 +108,166 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+static int memory_device_used_region_size_internal(Object *obj, void *opaque)
+{
+    uint64_t *size = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
+        DeviceState *dev = DEVICE(obj);
+        MemoryDeviceState *md = MEMORY_DEVICE(obj);
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
+
+        if (dev->realized) {
+            *size += mdc->get_region_size(md, &error_abort);
+        }
+    }
+
+    object_child_foreach(obj, memory_device_used_region_size_internal, opaque);
+    return 0;
+}
+
+static uint64_t memory_device_used_region_size(void)
+{
+    uint64_t size = 0;
+
+    memory_device_used_region_size_internal(qdev_get_machine(), &size);
+
+    return size;

Quoted 9 lines

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 139

Quoted 4 lines

hw/mem/memory-device.c +4 -0

+}
+
+uint64_t memory_device_get_free_addr(uint64_t *hint, uint64_t align,
+                                     uint64_t size, Error **errp)  

I'd suggest to pc_dimm_memory_plug/pc_dimm_get_free_addr first, namely most of the stuff it does like checks and assigning default values should go to pre_plug (pre realize) handler and then only actual mapping is left for plug (after realize) handler to deal with:

Can you elaborate what you mean by pre-plug? If this is about pre plug handler of the (machine) hotplug handler, it might be problematic for virtio devices.

Quoted 3 lines · On Mon, 23 Apr 2018 14:44:34 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 2 lines · Igor Mammedov

On Fri, 20 Apr 2018 14:34:56 +0200
David Hildenbrand <[email protected]> wrote:

considering v4 queued, I'm dropping mostly nor relevant points at this point. wrt, virtio I'll elaborate more in reply to Pankaj

[...]

Quoted 52 lines

Quoted 51 lines

Quoted 36 lines

hw/mem/memory-device.c +21 -0

index b860c9c582..b96efa3bf4 100644
@@ -15,6 +15,8 @@
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "qemu/range.h"
+#include "hw/virtio/vhost.h"
+#include "sysemu/kvm.h"
 
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {

@@ -106,6 +108,166 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+static int memory_device_used_region_size_internal(Object *obj, void *opaque)
+{
+    uint64_t *size = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
+        DeviceState *dev = DEVICE(obj);
+        MemoryDeviceState *md = MEMORY_DEVICE(obj);
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
+
+        if (dev->realized) {
+            *size += mdc->get_region_size(md, &error_abort);
+        }
+    }
+
+    object_child_foreach(obj, memory_device_used_region_size_internal, opaque);
+    return 0;
+}
+
+static uint64_t memory_device_used_region_size(void)

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 139

Quoted 10 lines

hw/mem/memory-device.c +10 -0

+{
+    uint64_t size = 0;
+
+    memory_device_used_region_size_internal(qdev_get_machine(), &size);
+
+    return size;
+}
+
+uint64_t memory_device_get_free_addr(uint64_t *hint, uint64_t align,
+                                     uint64_t size, Error **errp)  

I'd suggest to pc_dimm_memory_plug/pc_dimm_get_free_addr first, namely most of the stuff it does like checks and assigning default values should go to pre_plug (pre realize) handler and then only actual mapping is left for plug (after realize) handler to deal with:

Quoted 3 lines

Can you elaborate what you mean by pre-plug? If this is about pre plug handler of the (machine) hotplug handler, it might be problematic for virtio devices.

yes, something along these lines: c871bc70b

Quoted 66 lines · Igor Mammedov

Quoted 3 lines · On Mon, 23 Apr 2018 14:44:34 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 2 lines · Igor Mammedov

On Fri, 20 Apr 2018 14:34:56 +0200
David Hildenbrand <[email protected]> wrote:

considering v4 queued, I'm dropping mostly nor relevant points at this point. wrt, virtio I'll elaborate more in reply to Pankaj

[...]

Quoted 55 lines

Quoted 42 lines

Quoted 42 lines

hw/mem/memory-device.c +27 -0

index b860c9c582..b96efa3bf4 100644
@@ -15,6 +15,8 @@
 #include "qapi/error.h"
 #include "hw/boards.h"
 #include "qemu/range.h"
+#include "hw/virtio/vhost.h"
+#include "sysemu/kvm.h"
 
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {

@@ -106,6 +108,166 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+static int memory_device_used_region_size_internal(Object *obj, void *opaque)
+{
+    uint64_t *size = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
+        DeviceState *dev = DEVICE(obj);
+        MemoryDeviceState *md = MEMORY_DEVICE(obj);
+        MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
+
+        if (dev->realized) {
+            *size += mdc->get_region_size(md, &error_abort);
+        }
+    }
+
+    object_child_foreach(obj, memory_device_used_region_size_internal, opaque);
+    return 0;
+}
+
+static uint64_t memory_device_used_region_size(void)
+{
+    uint64_t size = 0;
+
+    memory_device_used_region_size_internal(qdev_get_machine(), &size);
+
+    return size;

Quoted 9 lines

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 139

Quoted 4 lines

hw/mem/memory-device.c +4 -0

+}
+
+uint64_t memory_device_get_free_addr(uint64_t *hint, uint64_t align,
+                                     uint64_t size, Error **errp)    

I'd suggest to pc_dimm_memory_plug/pc_dimm_get_free_addr first, namely most of the stuff it does like checks and assigning default values should go to pre_plug (pre realize) handler and then only actual mapping is left for plug (after realize) handler to deal with:

Can you elaborate what you mean by pre-plug? If this is about pre plug handler of the (machine) hotplug handler, it might be problematic for virtio devices.

Quoted 3 lines

yes, something along these lines: c871bc70b

Yes, we can factor that out (at least) for pc-dimm later on easily. Seems to be just about moving a couple of calls.

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 252

Quoted 11 lines

hw/mem/memory-device.c +9 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }

move these checks to pre_plug time

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);

missing vmstate registration?

Quoted 10 lines

Quoted 10 lines

hw/mem/memory-device.c +9 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }  

Quoted 5 lines

move these checks to pre_plug time

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);  

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

Thanks!

Quoted 14 lines · On Mon, 23 Apr 2018 14:52:37 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 14 lines

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 252

Quoted 10 lines

hw/mem/memory-device.c +9 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }  

move these checks to pre_plug time

Quoted 2 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);  

Quoted 4 lines

Quoted 1 line

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 17 lines · Igor Mammedov

Quoted 14 lines · On Mon, 23 Apr 2018 14:52:37 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 14 lines

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 252

Quoted 10 lines

hw/mem/memory-device.c +9 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }    

move these checks to pre_plug time

Quoted 2 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);    

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Yes, if we realize that all MemoryDevices need this call, we can move it to that place, too.

Wonder if we might want to make this configurable for virtio-pmem later on (via a flag or sth like that).

Quoted 26 lines · On Tue, 24 Apr 2018 15:41:23 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 24 lines · Igor Mammedov

Quoted 10 lines · On Mon, 23 Apr 2018 14:52:37 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 10 lines

Quoted 10 lines

hw/mem/memory-device.c +9 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }    

Quoted 8 lines

Quoted 5 lines

move these checks to pre_plug time

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);    

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 5 lines

Yes, if we realize that all MemoryDevices need this call, we can move it to that place, too.

Wonder if we might want to make this configurable for virtio-pmem later on (via a flag or sth like that).

I don't see any reason why we wouldn't like it to be migrated, it's the same as nvdimm but with another qemu:guest ABI and async flush instead of sync one we have with nvdimm.

Quoted 17 lines

Quoted 17 lines

Review

Quoted 13 lines

hw/mem/memory-device.c +7 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots

left");

(quoted diff) +2 -0

+        return;
+    }

move these checks to pre_plug time

Quoted 2 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);

Quoted 4 lines

Quoted 1 line

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 20 lines

Quoted 20 lines

Quoted 12 lines

Review

Quoted 12 lines

hw/mem/memory-device.c +7 -0

+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots

left");

(quoted diff) +2 -0

+        return;
+    }  

Quoted 5 lines

move these checks to pre_plug time

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);  

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

Quoted 3 lines

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Main difference between RAM and pmem it acts like combination of RAM and disk. Saying this, in normal use-case size would be 100 GB's - few TB's range. I am not sure we really want to migrate it for non-shared storage use-case.

One reason why nvdimm added vmstate info could be: still there would be transient writes in memory with fake DAX and there is no way(till now) to flush the guest writes. But with virtio-pmem we can flush such writes before migration and automatically at destination host with shared disk we will have updated data.

Thanks,
Pankaj

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 4 lines

Main difference between RAM and pmem it acts like combination of RAM and disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range. I am not sure we really want to migrate it for non-shared storage use-case.

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

Quoted 8 lines

One reason why nvdimm added vmstate info could be: still there would be transient
writes in memory with fake DAX and there is no way(till now) to flush the guest
writes. But with virtio-pmem we can flush such writes before migration and automatically
at destination host with shared disk we will have updated data.

nvdimm has concept of flush address hint (may be not implemented in qemu yet) but it can flush. The only reason I'm buying into virtio-mem idea is that would allow async flush queues which would reduce number of vmexits.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);  

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 8 lines

Quoted 4 lines

Main difference between RAM and pmem it acts like combination of RAM and disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range. I am not sure we really want to migrate it for non-shared storage use-case.

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated first. But if we migrate entire fake DAX RAMstate it will populate destination host page cache including pages while were idle in source host. This would unnecessarily create entropy in destination host.

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

Quoted 2 lines

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't need emulation at all.

Quoted 5 lines

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

not sure I agree.

Quoted 11 lines

Quoted 7 lines

One reason why nvdimm added vmstate info could be: still there would be transient
writes in memory with fake DAX and there is no way(till now) to flush the guest
writes. But with virtio-pmem we can flush such writes before migration and automatically
at destination host with shared disk we will have updated data.

nvdimm has concept of flush address hint (may be not implemented in qemu yet) but it can flush. The only reason I'm buying into virtio-mem idea is that would allow async flush queues which would reduce number of vmexits.

Thats correct.

Thanks,
Pankaj

Quoted 35 lines

Quoted 20 lines

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base,

mr);

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for
virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 7 lines

Main difference between RAM and pmem it acts like combination of RAM and
disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range.
I am not sure we really want to migrate it for non-shared storage use-case.

Quoted 4 lines

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated first.
But if we migrate entire fake DAX RAMstate it will populate destination host page
cache including pages while were idle in source host. This would unnecessarily
create entropy in destination host.

Quoted 2 lines

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

What would live migration handover flow look like in case of guest constantly dirting memory provided by virtio-pmem and and sometimes issuing async flush req along with it?

Quoted 8 lines

Quoted 4 lines

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't need
emulation at all.

depends on if file is on DAX filesystem, but your argument about migrating huge 100Gb- TB's range applies in this case as well.

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines

Quoted 3 lines

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

not sure I agree.

So would you inhibit migration in case of non shared backend storage, to avoid loosing data since they aren't migrated?

Quoted 23 lines

Quoted 11 lines

Quoted 11 lines

One reason why nvdimm added vmstate info could be: still there would be transient
writes in memory with fake DAX and there is no way(till now) to flush the
guest
writes. But with virtio-pmem we can flush such writes before migration and
automatically
at destination host with shared disk we will have updated data.

Quoted 5 lines

nvdimm has concept of flush address hint (may be not implemented in qemu yet)
but it can flush. The only reason I'm buying into virtio-mem idea is that would allow async flush queues which would reduce number of vmexits.

Thats correct.

Thanks,
Pankaj

Quoted 34 lines

Quoted 34 lines

Quoted 19 lines

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base,

mr);

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for
virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 7 lines

Main difference between RAM and pmem it acts like combination of RAM and
disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range.
I am not sure we really want to migrate it for non-shared storage use-case.

Quoted 4 lines

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated first.
But if we migrate entire fake DAX RAMstate it will populate destination host page
cache including pages while were idle in source host. This would unnecessarily
create entropy in destination host.

Quoted 5 lines

Quoted 2 lines

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

What would live migration handover flow look like in case of guest constantly dirting memory provided by virtio-pmem and and sometimes issuing async flush req along with it?

Dirty entire pmem (disk) at once not a usual scenario. Some part of disk/pmem would get dirty and we need to handle that. I just want to say moving entire pmem (disk) is not efficient solution because we are using this solution to manage guest memory efficiently. Otherwise it will be like any block device copy with non-shared storage.

Quoted 16 lines

Quoted 6 lines

Quoted 2 lines

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't need
emulation at all.

depends on if file is on DAX filesystem, but your argument about migrating huge 100Gb- TB's range applies in this case as well.

Quoted 7 lines

Quoted 5 lines

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

not sure I agree.

Quoted 2 lines

So would you inhibit migration in case of non shared backend storage, to avoid loosing data since they aren't migrated?

I am just thinking what features we want to support with pmem. And live migration with shared storage is the one which comes to my mind.

If live migration with non-shared storage is what we want to support (I don't know yet) we can add this? Even with shared storage it would copy entire pmem state?

Thanks,
Pankaj

Quoted 26 lines

Quoted 23 lines

Quoted 11 lines

Quoted 11 lines

One reason why nvdimm added vmstate info could be: still there would be transient
writes in memory with fake DAX and there is no way(till now) to flush the
guest
writes. But with virtio-pmem we can flush such writes before migration and
automatically
at destination host with shared disk we will have updated data.

Quoted 5 lines

nvdimm has concept of flush address hint (may be not implemented in qemu yet)
but it can flush. The only reason I'm buying into virtio-mem idea is that would allow async flush queues which would reduce number of vmexits.

Thats correct.

Thanks,
Pankaj

On Thu, 26 Apr 2018 03:37:51 -0400 (EDT)
Pankaj Gupta <[email protected]> wrote:

trimming CC list to keep people that might be interested in the topic and renaming thread to reflect it.

Quoted 40 lines

Quoted 34 lines

Quoted 34 lines

Quoted 19 lines

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base,

mr);

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for
virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 7 lines

Main difference between RAM and pmem it acts like combination of RAM and
disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range.
I am not sure we really want to migrate it for non-shared storage use-case.

Quoted 4 lines

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated first.
But if we migrate entire fake DAX RAMstate it will populate destination host page
cache including pages while were idle in source host. This would unnecessarily
create entropy in destination host.

Quoted 5 lines

Quoted 2 lines

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

What would live migration handover flow look like in case of guest constantly dirting memory provided by virtio-pmem and and sometimes issuing async flush req along with it?

Quoted 5 lines

Dirty entire pmem (disk) at once not a usual scenario. Some part of disk/pmem would get dirty and we need to handle that. I just want to say moving entire pmem (disk) is not efficient solution because we are using this solution to manage guest memory efficiently. Otherwise it will be like any block device copy with non-shared storage.

not sure if we can use block layer analogy here.

Quoted 16 lines

Quoted 16 lines

Quoted 6 lines

Quoted 2 lines

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't need
emulation at all.

depends on if file is on DAX filesystem, but your argument about migrating huge 100Gb- TB's range applies in this case as well.

Quoted 7 lines

Quoted 5 lines

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

not sure I agree.

Quoted 8 lines

Quoted 2 lines

So would you inhibit migration in case of non shared backend storage, to avoid loosing data since they aren't migrated?

I am just thinking what features we want to support with pmem. And live migration with shared storage is the one which comes to my mind.

If live migration with non-shared storage is what we want to support (I don't know yet) we can add this? Even with shared storage it would copy entire pmem state?

Perhaps we should register vmstate like for normal ram and use something similar to
http://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00003.html this
to skip shared memory on migration.
In this case we could use this for pc-dimms as well.

David,
what's your take on it?

Quoted 4 lines

Thanks,
Pankaj

Quoted 84 lines

Quoted 45 lines

Quoted 38 lines

Quoted 38 lines

Quoted 22 lines

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base,

mr);

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for
virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 7 lines

Main difference between RAM and pmem it acts like combination of RAM and
disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range.
I am not sure we really want to migrate it for non-shared storage use-case.

Quoted 4 lines

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated
first.
But if we migrate entire fake DAX RAMstate it will populate destination host page
cache including pages while were idle in source host. This would unnecessarily
create entropy in destination host.

Quoted 6 lines

Quoted 3 lines

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

What would live migration handover flow look like in case of guest constantly dirting memory provided by virtio-pmem and and sometimes issuing async flush req along with it?

Quoted 9 lines

Dirty entire pmem (disk) at once not a usual scenario. Some part of disk/pmem
would get dirty and we need to handle that. I just want to say moving entire
pmem (disk) is not efficient solution because we are using this solution to
manage guest memory efficiently. Otherwise it will be like any block device copy
with non-shared storage.

not sure if we can use block layer analogy here.

Quoted 20 lines

Quoted 12 lines

Quoted 7 lines

Quoted 2 lines

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't
need
emulation at all.

depends on if file is on DAX filesystem, but your argument about migrating huge 100Gb- TB's range applies in this case as well.

Quoted 2 lines

Quoted 2 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

not sure I agree.

So would you inhibit migration in case of non shared backend storage, to avoid loosing data since they aren't migrated?

Quoted 8 lines

I am just thinking what features we want to support with pmem. And live migration
with shared storage is the one which comes to my mind.

If live migration with non-shared storage is what we want to support (I don't know
yet) we can add this? Even with shared storage it would copy entire pmem state?

Quoted 8 lines

Perhaps we should register vmstate like for normal ram and use something similar to
http://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00003.html this
to skip shared memory on migration.
In this case we could use this for pc-dimms as well.

David,
what's your take on it?

(I assume you were talking to David Gilbert, but still my take on it :))

"shared RAM" use in that context is rather "shared between processes", no? What would be the benefit of enabling migration for a ramblock but then again blocking it off?

Anyhow, I think this detail discussion is way to early right now. Pankaj has some other problems to solve before moving on to migration (yes, migration is important to keep in mind but not the top priority right now). And I would consider migration in this context as the next step once we have the basics sorted out (flushing, threads ...)

On Thu, 26 Apr 2018 03:37:51 -0400 (EDT)
Pankaj Gupta <[email protected]> wrote:

trimming CC list to keep people that might be interested in the topic and renaming thread to reflect it.

Quoted 40 lines

Quoted 34 lines

Quoted 34 lines

Quoted 19 lines

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base,

mr);

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for
virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 7 lines

Main difference between RAM and pmem it acts like combination of RAM and
disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range.
I am not sure we really want to migrate it for non-shared storage use-case.

Quoted 4 lines

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated first.
But if we migrate entire fake DAX RAMstate it will populate destination host page
cache including pages while were idle in source host. This would unnecessarily
create entropy in destination host.

Quoted 5 lines

Quoted 2 lines

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

What would live migration handover flow look like in case of guest constantly dirting memory provided by virtio-pmem and and sometimes issuing async flush req along with it?

Quoted 5 lines

Dirty entire pmem (disk) at once not a usual scenario. Some part of disk/pmem would get dirty and we need to handle that. I just want to say moving entire pmem (disk) is not efficient solution because we are using this solution to manage guest memory efficiently. Otherwise it will be like any block device copy with non-shared storage.

not sure if we can use block layer analogy here.

Quoted 16 lines

Quoted 16 lines

Quoted 6 lines

Quoted 2 lines

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't need
emulation at all.

depends on if file is on DAX filesystem, but your argument about migrating huge 100Gb- TB's range applies in this case as well.

Quoted 7 lines

Quoted 5 lines

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

not sure I agree.

Quoted 8 lines

Quoted 2 lines

So would you inhibit migration in case of non shared backend storage, to avoid loosing data since they aren't migrated?

I am just thinking what features we want to support with pmem. And live migration with shared storage is the one which comes to my mind.

If live migration with non-shared storage is what we want to support (I don't know yet) we can add this? Even with shared storage it would copy entire pmem state?

Perhaps we should register vmstate like for normal ram and use something similar to
http://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00003.html this
to skip shared memory on migration.
In this case we could use this for pc-dimms as well.

David,
what's your take on it?

Quoted 22 lines

Thanks,
Pankaj

Quoted 19 lines

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

One reason why nvdimm added vmstate info could be: still there would be transient
writes in memory with fake DAX and there is no way(till now) to flush the
guest
writes. But with virtio-pmem we can flush such writes before migration and
automatically
at destination host with shared disk we will have updated data.

Quoted 5 lines

nvdimm has concept of flush address hint (may be not implemented in qemu yet)
but it can flush. The only reason I'm buying into virtio-mem idea is that would allow async flush queues which would reduce number of vmexits.

Thats correct.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Thanks,
Pankaj

Quoted 82 lines · * Igor Mammedov ([email protected]) wrote:

On Thu, 26 Apr 2018 03:37:51 -0400 (EDT)
Pankaj Gupta <[email protected]> wrote:

trimming CC list to keep people that might be interested in the topic and renaming thread to reflect it.

Quoted 40 lines

Quoted 34 lines

Quoted 34 lines

Quoted 19 lines

Quoted 12 lines

Quoted 11 lines

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base,

mr);

missing vmstate registration?

Missed this one: To be called by the caller. Important because e.g. for
virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 7 lines

Main difference between RAM and pmem it acts like combination of RAM and
disk.
Saying this, in normal use-case size would be 100 GB's - few TB's range.
I am not sure we really want to migrate it for non-shared storage use-case.

Quoted 4 lines

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly from target host. That probably won't work right out of box and would need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated first.
But if we migrate entire fake DAX RAMstate it will populate destination host page
cache including pages while were idle in source host. This would unnecessarily
create entropy in destination host.

Quoted 5 lines

Quoted 2 lines

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

What would live migration handover flow look like in case of guest constantly dirting memory provided by virtio-pmem and and sometimes issuing async flush req along with it?

Quoted 5 lines

Dirty entire pmem (disk) at once not a usual scenario. Some part of disk/pmem would get dirty and we need to handle that. I just want to say moving entire pmem (disk) is not efficient solution because we are using this solution to manage guest memory efficiently. Otherwise it will be like any block device copy with non-shared storage.

not sure if we can use block layer analogy here.

Quoted 16 lines

Quoted 16 lines

Quoted 6 lines

Quoted 2 lines

The same applies to nv/pc-dimm as well, as backend file easily could be on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't need
emulation at all.

depends on if file is on DAX filesystem, but your argument about migrating huge 100Gb- TB's range applies in this case as well.

Quoted 7 lines

Quoted 5 lines

Maybe for now we should migrate everything so it would work in case of non shared NVDIMM on host. And then later add migration-less capability to all of them.

not sure I agree.

Quoted 8 lines

Quoted 2 lines

So would you inhibit migration in case of non shared backend storage, to avoid loosing data since they aren't migrated?

I am just thinking what features we want to support with pmem. And live migration with shared storage is the one which comes to my mind.

If live migration with non-shared storage is what we want to support (I don't know yet) we can add this? Even with shared storage it would copy entire pmem state?

Perhaps we should register vmstate like for normal ram and use something similar to
http://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00003.html this
to skip shared memory on migration.
In this case we could use this for pc-dimms as well.

Quoted 2 lines

David,
what's your take on it?

My feel is that something is going to have to migrate it, I'm just not sure how.
So let me just check I understand:
a) It's potentially huge

b) It's a RAMBlock

c) It's backed by ????
c1) Something machine local - i.e. a physical lump of flash in a
socket rather than something sharable by machines?

d) It can potentially be rapidly changing as the guest writes to it?

Dave

Quoted 22 lines

Quoted 22 lines

Thanks,
Pankaj

Quoted 19 lines

Quoted 19 lines

Quoted 11 lines

Quoted 11 lines

One reason why nvdimm added vmstate info could be: still there would be transient
writes in memory with fake DAX and there is no way(till now) to flush the
guest
writes. But with virtio-pmem we can flush such writes before migration and
automatically
at destination host with shared disk we will have updated data.

Quoted 5 lines

nvdimm has concept of flush address hint (may be not implemented in qemu yet)
but it can flush. The only reason I'm buying into virtio-mem idea is that would allow async flush queues which would reduce number of vmexits.

Thats correct.

Quoted 9 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Thanks,
Pankaj

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 106 lines · On Fri, 4 May 2018 13:26:51 +0100 "Dr. David Alan Gilbert"…

Quoted 105 lines · * Igor Mammedov ([email protected]) wrote:

On Thu, 26 Apr 2018 03:37:51 -0400 (EDT)
Pankaj Gupta <[email protected]> wrote:

trimming CC list to keep people that might be interested in the topic and renaming thread to reflect it.

Quoted 51 lines

Quoted 44 lines

Quoted 44 lines

Quoted 25 lines

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 4 lines

(quoted diff) +2 -0

+
+    memory_region_add_subregion(&hpms->mr, addr -

hpms->base,
mr);

missing vmstate registration?

Quoted 8 lines

Quoted 4 lines

Missed this one: To be called by the caller. Important because e.g.
for
virtio-pmem we don't want this (I assume :) ).

if pmem isn't on shared storage, then We'd probably want to migrate
it as well, otherwise target would experience data loss. Anyways, I'd just reat it as normal RAM in migration case

Quoted 10 lines

Main difference between RAM and pmem it acts like combination of RAM
and
disk.
Saying this, in normal use-case size would be 100 GB's - few TB's
range.
I am not sure we really want to migrate it for non-shared storage
use-case.

Quoted 6 lines

with non shared storage you'd have to migrate it target host but with shared storage it might be possible to flush it and use directly
from target host. That probably won't work right out of box and would
need some sort of synchronization between src/dst hosts.

Shared storage should work out of the box.
Only thing is data in destination
host will be cache cold and existing pages in cache should be invalidated
first.
But if we migrate entire fake DAX RAMstate it will populate destination
host page
cache including pages while were idle in source host. This would unnecessarily
create entropy in destination host.

Quoted 6 lines

Quoted 3 lines

To me this feature don't make much sense. Problem which we are solving is:
Efficiently use guest RAM.

What would live migration handover flow look like in case of guest constantly dirting memory provided by virtio-pmem and and sometimes issuing async flush req along with it?

Quoted 9 lines

Dirty entire pmem (disk) at once not a usual scenario. Some part of disk/pmem
would get dirty and we need to handle that. I just want to say moving entire
pmem (disk) is not efficient solution because we are using this solution to
manage guest memory efficiently. Otherwise it will be like any block device copy
with non-shared storage.

not sure if we can use block layer analogy here.

Quoted 23 lines

Quoted 19 lines

Quoted 8 lines

Quoted 3 lines

The same applies to nv/pc-dimm as well, as backend file easily could be
on pmem storage as well.

Are you saying backing file is in actual actual nvdimm hardware? we don't
need
emulation at all.

depends on if file is on DAX filesystem, but your argument about migrating huge 100Gb- TB's range applies in this case as well.

Quoted 8 lines

Quoted 7 lines

Maybe for now we should migrate everything so it would work in case of
non shared NVDIMM on host. And then later add migration-less capability
to all of them.

Quoted 3 lines

Quoted 1 line

not sure I agree.

So would you inhibit migration in case of non shared backend storage, to avoid loosing data since they aren't migrated?

Quoted 8 lines

I am just thinking what features we want to support with pmem. And live migration
with shared storage is the one which comes to my mind.

If live migration with non-shared storage is what we want to support (I don't know
yet) we can add this? Even with shared storage it would copy entire pmem state?

Perhaps we should register vmstate like for normal ram and use something similar to
http://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00003.html this
to skip shared memory on migration.
In this case we could use this for pc-dimms as well.

Quoted 7 lines

Quoted 2 lines

David,
what's your take on it?

My feel is that something is going to have to migrate it, I'm just not sure how.
So let me just check I understand:
a) It's potentially huge

yep, assume it could be in storage quarantines (100s of Gb)

Quoted 1 line

b) It's a RAMBlock

it is

Quoted 3 lines

c) It's backed by ????
c1) Something machine local - i.e. a physical lump of flash in a
socket rather than something sharable by machines?

it's backed by memory-backend-foo, so it could be really anything (RAM, file on local or shared storage, file descriptor)

Quoted 2 lines

d) It can potentially be rapidly changing as the guest writes to it?

it's sort of like NVDIMM but without NVDIMM interface, it uses virtio to to force flushing instead. Otherwise it's directly mapped into guest address space, so guest can do anything with it including fast dirtying.

Quoted 39 lines

Dave

Quoted 26 lines

Quoted 26 lines

Thanks,
Pankaj

Quoted 23 lines

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

One reason why nvdimm added vmstate info could be: still there would be
transient
writes in memory with fake DAX and there is no way(till now) to flush
the
guest
writes. But with virtio-pmem we can flush such writes before migration
and
automatically
at destination host with shared disk we will have updated data.

Quoted 6 lines

nvdimm has concept of flush address hint (may be not implemented in qemu
yet)
but it can flush. The only reason I'm buying into virtio-mem idea is that would allow async flush queues which would reduce number of vmexits.

Thats correct.

Quoted 9 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

Thanks,
Pankaj

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK