[Qemu-devel] [PATCH v3 00/13] pc-dimm: next bunch of cleanups

[Qemu-devel] [PATCH v3 00/13] pc-dimm: next bunch of cleanups

[David Hildenbrand]

This is another set of cleanups as the result from
    [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
And is based on
    [PATCH v1 0/2] memory: fix alignment checks/asserts

These cleanup are the last step before
- 1. moving pc-dimm address and slot assignment into pc_dimm_pre_plug
- 2. factoring out pre_plug, plug and unplug logic of memory devices
     completely

v2 -> v3:
- "nvdimm: no need to overwrite get_vmstate_memory_region()"
-- reshuffeld to avoid breaking compilation in between patches
- "nvdimm: convert "label-size" into a static property"
-- replace by "nvdimm: convert nvdimm_mr into a pointer"
-- and "nvdimm: allow setting the label-size to 0"

v1 -> v2:
- deferring "pc-dimm: assign and verify the "slot" property during pre_plug"
- deferring "pc-dimm: introduce and use pc_dimm_memory_pre_plug()"
- dropped "pc-dimm: get_memory_region() will never return a NULL pointer"
- dropped "pc-dimm: don't allow to access "size" before the device was realized"
- dropped "spapr: move memory hotplug size check into plug code"
- dropped "pc-dimm: get_memory_region() can never fail"
-- replaces by "pc-dimm: get_memory_region() will not fail after realize"
- added "pc: rename pc_dimm_(plug|unplug|...)* into ..."
- added "pc-dimm: rename pc_dimm_memory_* to pc_dimm_*"
- added "pc-dimm: remove pc_dimm_get_free_slot() from header"
- added "pc-dimm: merge get_(vmstate_)memory_region()"
- added "nvdimm: convert "unarmed" into a static property"
- added "nvdimm: convert "label-size" into a static property"
- added "nvdimm: make get_memory_region() perform checks and initialization"

David Hildenbrand (13):
  pc-dimm: remove leftover "struct pc_dimms_capacity"
  pc: rename pc_dimm_(plug|unplug|...)* into
    pc_memory_(plug|unplug|...)*
  pc-dimm: rename pc_dimm_memory_* to pc_dimm_*
  pc-dimm: remove pc_dimm_get_free_slot() from header
  pc: factor out pc specific dimm checks into pc_memory_pre_plug()
  nvdimm: no need to overwrite get_vmstate_memory_region()
  hostmem: drop error variable from host_memory_backend_get_memory()
  pc-dimm: merge get_(vmstate_)memory_region()
  nvdimm: convert "unarmed" into a static property
  nvdimm: convert nvdimm_mr into a pointer
  nvdimm: allow setting the label-size to 0
  nvdimm: make get_memory_region() perform checks and initialization
  pc-dimm: get_memory_region() will not fail after realize

 backends/hostmem.c       |  3 +-
 hw/i386/pc.c             | 73 +++++++++++++++---------------
 hw/mem/nvdimm.c          | 95 +++++++++++++++++++++-------------------
 hw/mem/pc-dimm.c         | 35 +++++----------
 hw/misc/ivshmem.c        |  3 +-
 hw/ppc/spapr.c           | 18 +++-----
 include/hw/mem/nvdimm.h  |  2 +-
 include/hw/mem/pc-dimm.h | 17 +++----
 include/sysemu/hostmem.h |  3 +-
 numa.c                   |  3 +-
 10 files changed, 119 insertions(+), 133 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v3 01/13] pc-dimm: remove leftover "struct pc_dimms_capacity"

[David Hildenbrand]

Not needed anymore, let's drop it.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/pc-dimm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 12da89d562..62b34a992e 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,11 +27,6 @@
 #include "sysemu/numa.h"
 #include "trace.h"
 
-typedef struct pc_dimms_capacity {
-     uint64_t size;
-     Error    **errp;
-} pc_dimms_capacity;
-
 void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
                          uint64_t align, Error **errp)
 {
-- 
2.17.1

[Qemu-devel] [PATCH v3 02/13] pc: rename pc_dimm_(plug|unplug|...)* into pc_memory_(plug|unplug|...)*

[David Hildenbrand]

Use a similar naming scheme as spapr. This way, we can go ahead and
rename e.g. pc_dimm_memory_plug to pc_dimm_plug, which avoids
confusion.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 622e49d6bc..f9250ffae7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1674,8 +1674,8 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
     }
 }
 
-static void pc_dimm_plug(HotplugHandler *hotplug_dev,
-                         DeviceState *dev, Error **errp)
+static void pc_memory_plug(HotplugHandler *hotplug_dev,
+                           DeviceState *dev, Error **errp)
 {
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
@@ -1728,8 +1728,8 @@ out:
     error_propagate(errp, local_err);
 }
 
-static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
-                                   DeviceState *dev, Error **errp)
+static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
 {
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
@@ -1759,8 +1759,8 @@ out:
     error_propagate(errp, local_err);
 }
 
-static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
-                           DeviceState *dev, Error **errp)
+static void pc_memory_unplug(HotplugHandler *hotplug_dev,
+                             DeviceState *dev, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
     HotplugHandlerClass *hhc;
@@ -2015,7 +2015,7 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_plug(hotplug_dev, dev, errp);
     }
@@ -2025,7 +2025,7 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug_request(hotplug_dev, dev, errp);
+        pc_memory_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
     } else {
@@ -2038,7 +2038,7 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_memory_unplug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_cb(hotplug_dev, dev, errp);
     } else {
-- 
2.17.1

[Qemu-devel] [PATCH v3 03/13] pc-dimm: rename pc_dimm_memory_* to pc_dimm_*

[David Hildenbrand]

Let's rename it to make it look more consistent.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c             | 4 ++--
 hw/mem/pc-dimm.c         | 6 +++---
 hw/ppc/spapr.c           | 6 +++---
 include/hw/mem/pc-dimm.h | 6 +++---
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f9250ffae7..f23133facc 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1713,7 +1713,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
+    pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
     if (local_err) {
         goto out;
     }
@@ -1773,7 +1773,7 @@ static void pc_memory_unplug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_unplug(dev, MACHINE(pcms));
+    pc_dimm_unplug(dev, MACHINE(pcms));
     object_unparent(OBJECT(dev));
 
  out:
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 62b34a992e..9e0c83e415 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,8 +27,8 @@
 #include "sysemu/numa.h"
 #include "trace.h"
 
-void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
-                         uint64_t align, Error **errp)
+void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
+                  Error **errp)
 {
     int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
@@ -84,7 +84,7 @@ out:
     error_propagate(errp, local_err);
 }
 
-void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine)
+void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f59999daac..3e5320020f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3153,7 +3153,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, MACHINE(ms), align, &local_err);
+    pc_dimm_plug(dev, MACHINE(ms), align, &local_err);
     if (local_err) {
         goto out;
     }
@@ -3176,7 +3176,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     return;
 
 out_unplug:
-    pc_dimm_memory_unplug(dev, MACHINE(ms));
+    pc_dimm_unplug(dev, MACHINE(ms));
 out:
     error_propagate(errp, local_err);
 }
@@ -3328,7 +3328,7 @@ static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
     sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
-    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
+    pc_dimm_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 627c8601d9..860343d64f 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -78,7 +78,7 @@ typedef struct PCDIMMDeviceClass {
 
 int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
-void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
-                         uint64_t align, Error **errp);
-void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
+void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
+                  Error **errp);
+void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v3 04/13] pc-dimm: remove pc_dimm_get_free_slot() from header

[David Hildenbrand]

Not used outside of pc-dimm.c and there shouldn't be other users. If
other devices (e.g. memory devices) ever have to also use slots, then we
will have to factor this out.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/pc-dimm.c         | 4 +++-
 include/hw/mem/pc-dimm.h | 2 --
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9e0c83e415..7387963cf1 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,6 +27,8 @@
 #include "sysemu/numa.h"
 #include "trace.h"
 
+static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
+
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
                   Error **errp)
 {
@@ -111,7 +113,7 @@ static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
     return 0;
 }
 
-int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
+static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
 {
     unsigned long *bitmap;
     int slot = 0;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 860343d64f..cf71247630 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -76,8 +76,6 @@ typedef struct PCDIMMDeviceClass {
     MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;
 
-int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
-
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
                   Error **errp);
 void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
-- 
2.17.1

[Qemu-devel] [PATCH v3 05/13] pc: factor out pc specific dimm checks into pc_memory_pre_plug()

[David Hildenbrand]

We can perform these checks before the device is actually realized.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f23133facc..2db032a6df 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1674,6 +1674,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
     }
 }
 
+static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp)
+{
+    const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+
+    /*
+     * When -no-acpi is used with Q35 machine type, no ACPI is built,
+     * but pcms->acpi_dev is still created. Check !acpi_enabled in
+     * addition to cover this case.
+     */
+    if (!pcms->acpi_dev || !acpi_enabled) {
+        error_setg(errp,
+                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
+        return;
+    }
+
+    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
+        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+        return;
+    }
+}
+
 static void pc_memory_plug(HotplugHandler *hotplug_dev,
                            DeviceState *dev, Error **errp)
 {
@@ -1696,23 +1719,6 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
         align = memory_region_get_alignment(mr);
     }
 
-    /*
-     * When -no-acpi is used with Q35 machine type, no ACPI is built,
-     * but pcms->acpi_dev is still created. Check !acpi_enabled in
-     * addition to cover this case.
-     */
-    if (!pcms->acpi_dev || !acpi_enabled) {
-        error_setg(&local_err,
-                   "memory hotplug is not enabled: missing acpi device or acpi disabled");
-        goto out;
-    }
-
-    if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
-        error_setg(&local_err,
-                   "nvdimm is not enabled: missing 'nvdimm' in '-M'");
-        goto out;
-    }
-
     pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
     if (local_err) {
         goto out;
@@ -2006,7 +2012,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
-    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        pc_memory_pre_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_pre_plug(hotplug_dev, dev, errp);
     }
 }
-- 
2.17.1

[Qemu-devel] [PATCH v3 06/13] nvdimm: no need to overwrite get_vmstate_memory_region()

[David Hildenbrand]

Our parent class (PC_DIMM) provides exactly the same function.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/nvdimm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 4087aca25e..f974accbdd 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -166,11 +166,6 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
     memory_region_set_dirty(mr, backend_offset, size);
 }
 
-static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
-{
-    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
-}
-
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
@@ -178,7 +173,6 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
 
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
-    ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
 
     nvc->read_label_data = nvdimm_read_label_data;
     nvc->write_label_data = nvdimm_write_label_data;
-- 
2.17.1

[Qemu-devel] [PATCH v3 07/13] hostmem: drop error variable from host_memory_backend_get_memory()

[David Hildenbrand]

Unused, so let's remove it.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 backends/hostmem.c       | 3 +--
 hw/mem/nvdimm.c          | 4 ++--
 hw/mem/pc-dimm.c         | 4 ++--
 hw/misc/ivshmem.c        | 3 +--
 include/sysemu/hostmem.h | 3 +--
 numa.c                   | 3 +--
 6 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 3627e61584..4908946cd3 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -247,8 +247,7 @@ bool host_memory_backend_mr_inited(HostMemoryBackend *backend)
     return memory_region_size(&backend->mr) != 0;
 }
 
-MemoryRegion *
-host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
+MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend)
 {
     return host_memory_backend_mr_inited(backend) ? &backend->mr : NULL;
 }
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index f974accbdd..df9716231f 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -105,7 +105,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
 
 static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
 {
-    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem, errp);
+    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
     NVDIMMDevice *nvdimm = NVDIMM(dimm);
     uint64_t align, pmem_size, size = memory_region_size(mr);
 
@@ -161,7 +161,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
 
     memcpy(nvdimm->label_data + offset, buf, size);
 
-    mr = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+    mr = host_memory_backend_get_memory(dimm->hostmem);
     backend_offset = memory_region_size(mr) - nvdimm->label_size + offset;
     memory_region_set_dirty(mr, backend_offset, size);
 }
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 7387963cf1..73f0eee4c7 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -226,12 +226,12 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
         return NULL;
     }
 
-    return host_memory_backend_get_memory(dimm->hostmem, errp);
+    return host_memory_backend_get_memory(dimm->hostmem);
 }
 
 static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
 {
-    return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+    return host_memory_backend_get_memory(dimm->hostmem);
 }
 
 static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 16f03701b7..ee01c5e66b 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -909,8 +909,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
     if (s->hostmem != NULL) {
         IVSHMEM_DPRINTF("using hostmem\n");
 
-        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem,
-                                                         &error_abort);
+        s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
     } else {
         Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
         assert(chr);
diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 5beb0ef8ab..6e6bd2c1cb 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -62,8 +62,7 @@ struct HostMemoryBackend {
 };
 
 bool host_memory_backend_mr_inited(HostMemoryBackend *backend);
-MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
-                                             Error **errp);
+MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend);
 
 void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped);
 bool host_memory_backend_is_mapped(HostMemoryBackend *backend);
diff --git a/numa.c b/numa.c
index 33572bfa74..94f758c757 100644
--- a/numa.c
+++ b/numa.c
@@ -523,8 +523,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
         if (!backend) {
             continue;
         }
-        MemoryRegion *seg = host_memory_backend_get_memory(backend,
-                                                           &error_fatal);
+        MemoryRegion *seg = host_memory_backend_get_memory(backend);
 
         if (memory_region_is_mapped(seg)) {
             char *path = object_get_canonical_path_component(OBJECT(backend));
-- 
2.17.1

[Qemu-devel] [PATCH v3 08/13] pc-dimm: merge get_(vmstate_)memory_region()

[David Hildenbrand]

Importantly, get_vmstate_memory_region() should also fail with a proper
error if called before the device is realized. For a PCDIMM, both functions
are to return the same thing, so share the implementation.

All current users are called after the device has been realized, so we
can expect the calls to succeed.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/pc-dimm.c         | 13 +++++--------
 include/hw/mem/pc-dimm.h |  3 ++-
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 73f0eee4c7..4ff39b59ef 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -35,7 +35,8 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
     int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
+    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
+                                                              &error_abort);
     Error *local_err = NULL;
     MemoryRegion *mr;
     uint64_t addr;
@@ -90,7 +91,8 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
+    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
+                                                              &error_abort);
     MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
 
     memory_device_unplug_region(machine, mr);
@@ -229,11 +231,6 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
     return host_memory_backend_get_memory(dimm->hostmem);
 }
 
-static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
-{
-    return host_memory_backend_get_memory(dimm->hostmem);
-}
-
 static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
 {
     const PCDIMMDevice *dimm = PC_DIMM(md);
@@ -298,7 +295,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
     dc->desc = "DIMM memory module";
 
     ddc->get_memory_region = pc_dimm_get_memory_region;
-    ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
+    ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
 
     mdc->get_addr = pc_dimm_md_get_addr;
     /* for a dimm plugged_size == region_size */
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index cf71247630..5679a80465 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -73,7 +73,8 @@ typedef struct PCDIMMDeviceClass {
     /* public */
     void (*realize)(PCDIMMDevice *dimm, Error **errp);
     MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
-    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
+    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm,
+                                               Error **errp);
 } PCDIMMDeviceClass;
 
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
-- 
2.17.1

[Qemu-devel] [PATCH v3 09/13] nvdimm: convert "unarmed" into a static property

[David Hildenbrand]

We don't allow to modify it after realization. So we can simply turn
it into a static property.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/nvdimm.c | 32 +++++++-------------------------
 1 file changed, 7 insertions(+), 25 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index df9716231f..7260c9c6b1 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -64,36 +64,11 @@ out:
     error_propagate(errp, local_err);
 }
 
-static bool nvdimm_get_unarmed(Object *obj, Error **errp)
-{
-    NVDIMMDevice *nvdimm = NVDIMM(obj);
-
-    return nvdimm->unarmed;
-}
-
-static void nvdimm_set_unarmed(Object *obj, bool value, Error **errp)
-{
-    NVDIMMDevice *nvdimm = NVDIMM(obj);
-    Error *local_err = NULL;
-
-    if (memory_region_size(&nvdimm->nvdimm_mr)) {
-        error_setg(&local_err, "cannot change property value");
-        goto out;
-    }
-
-    nvdimm->unarmed = value;
-
- out:
-    error_propagate(errp, local_err);
-}
-
 static void nvdimm_init(Object *obj)
 {
     object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
                         nvdimm_get_label_size, nvdimm_set_label_size, NULL,
                         NULL, NULL);
-    object_property_add_bool(obj, NVDIMM_UNARMED_PROP,
-                             nvdimm_get_unarmed, nvdimm_set_unarmed, NULL);
 }
 
 static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
@@ -166,13 +141,20 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
     memory_region_set_dirty(mr, backend_offset, size);
 }
 
+static Property nvdimm_properties[] = {
+    DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
     PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
     NVDIMMClass *nvc = NVDIMM_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
     ddc->realize = nvdimm_realize;
     ddc->get_memory_region = nvdimm_get_memory_region;
+    dc->props = nvdimm_properties;
 
     nvc->read_label_data = nvdimm_read_label_data;
     nvc->write_label_data = nvdimm_write_label_data;
-- 
2.17.1

[Qemu-devel] [PATCH v3 10/13] nvdimm: convert nvdimm_mr into a pointer

[David Hildenbrand]

This way we can easily check if the region has already been inititalized
without having to rely on the size of an uninitialized region being 0.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/nvdimm.c         | 9 +++++----
 include/hw/mem/nvdimm.h | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7260c9c6b1..db7d8c3050 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -43,7 +43,7 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
     Error *local_err = NULL;
     uint64_t value;
 
-    if (memory_region_size(&nvdimm->nvdimm_mr)) {
+    if (nvdimm->nvdimm_mr) {
         error_setg(&local_err, "cannot change property value");
         goto out;
     }
@@ -75,7 +75,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
 {
     NVDIMMDevice *nvdimm = NVDIMM(dimm);
 
-    return &nvdimm->nvdimm_mr;
+    return nvdimm->nvdimm_mr;
 }
 
 static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
@@ -102,9 +102,10 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
         return;
     }
 
-    memory_region_init_alias(&nvdimm->nvdimm_mr, OBJECT(dimm),
+    nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
+    memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
                              "nvdimm-memory", mr, 0, pmem_size);
-    nvdimm->nvdimm_mr.align = align;
+    nvdimm->nvdimm_mr->align = align;
 }
 
 /*
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 9340631cfc..c5c9b3c7f8 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -74,7 +74,7 @@ struct NVDIMMDevice {
      * it's the PMEM region in NVDIMM device, which is presented to
      * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
      */
-    MemoryRegion nvdimm_mr;
+    MemoryRegion *nvdimm_mr;
 
     /*
      * The 'on' value results in the unarmed flag set in ACPI NFIT,
-- 
2.17.1

[Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0

[David Hildenbrand]

It is inititally 0, so setting it to 0 should be allowed, too.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/nvdimm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index db7d8c3050..df7646488b 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -52,9 +52,9 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
     if (local_err) {
         goto out;
     }
-    if (value < MIN_NAMESPACE_LABEL_SIZE) {
+    if (value && value < MIN_NAMESPACE_LABEL_SIZE) {
         error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
-                   " at least 0x%lx", object_get_typename(obj),
+                   " either 0 or at least 0x%lx", object_get_typename(obj),
                    name, value, MIN_NAMESPACE_LABEL_SIZE);
         goto out;
     }
-- 
2.17.1

[Qemu-devel] [PATCH v3 12/13] nvdimm: make get_memory_region() perform checks and initialization

[David Hildenbrand]

We might get a call to get_memory_region() before the device has been
realized. We should return a consistent value, as the return value
will e.g. later on be used in the pre_plug handler.

To avoid duplicating too much code, factor the initialization and checks
out into a helper function.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/nvdimm.c | 46 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index df7646488b..deba5719bc 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -71,20 +71,24 @@ static void nvdimm_init(Object *obj)
                         NULL, NULL);
 }
 
-static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
+static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
 {
-    NVDIMMDevice *nvdimm = NVDIMM(dimm);
+    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
+    uint64_t align, pmem_size, size;
+    MemoryRegion *mr;
 
-    return nvdimm->nvdimm_mr;
-}
+    if (nvdimm->nvdimm_mr) {
+        return;
+    }
 
-static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
-{
-    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
-    NVDIMMDevice *nvdimm = NVDIMM(dimm);
-    uint64_t align, pmem_size, size = memory_region_size(mr);
+    if (!dimm->hostmem) {
+        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
+        return;
+    }
 
+    mr = host_memory_backend_get_memory(dimm->hostmem);
     align = memory_region_get_alignment(mr);
+    size = memory_region_size(mr);
 
     pmem_size = size - nvdimm->label_size;
     nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
@@ -108,6 +112,30 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
     nvdimm->nvdimm_mr->align = align;
 }
 
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(dimm);
+    Error *local_err = NULL;
+
+    if (!nvdimm->nvdimm_mr) {
+        nvdimm_prepare_memory_region(nvdimm, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return NULL;
+        }
+    }
+    return nvdimm->nvdimm_mr;
+}
+
+static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(dimm);
+
+    if (!nvdimm->nvdimm_mr) {
+        nvdimm_prepare_memory_region(nvdimm, errp);
+    }
+}
+
 /*
  * the caller should check the input parameters before calling
  * label read/write functions.
-- 
2.17.1

[Qemu-devel] [PATCH v3 13/13] pc-dimm: get_memory_region() will not fail after realize

[David Hildenbrand]

Let's try to reduce error handling a bit. In the plug/unplug case, the
device was realized and therefore we can assume that getting access to
the memory region will not fail.

For get_vmstate_memory_region() this is already handled that way.
Document both cases.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c             |  7 +------
 hw/mem/pc-dimm.c         |  7 +------
 hw/ppc/spapr.c           | 12 ++----------
 include/hw/mem/pc-dimm.h |  6 ++++--
 4 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2db032a6df..f310040351 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1706,15 +1706,10 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     uint64_t align = TARGET_PAGE_SIZE;
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
     if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
         align = memory_region_get_alignment(mr);
     }
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 4ff39b59ef..65843bc52a 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -37,15 +37,10 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
                                                               &error_abort);
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     Error *local_err = NULL;
-    MemoryRegion *mr;
     uint64_t addr;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
     addr = object_property_get_uint(OBJECT(dimm),
                                     PC_DIMM_ADDR_PROP, &local_err);
     if (local_err) {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3e5320020f..6934abc21e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     uint64_t align, size, addr;
     uint32_t node;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
     align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
@@ -3340,16 +3336,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
+    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
     uint32_t nr_lmbs;
     uint64_t size, addr_start, addr;
     int i;
     sPAPRDRConnector *drc;
 
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
     size = memory_region_size(mr);
     nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
 
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 5679a80465..26ebb7d5e9 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -62,9 +62,11 @@ typedef struct PCDIMMDevice {
  * @realize: called after common dimm is realized so that the dimm based
  * devices get the chance to do specified operations.
  * @get_memory_region: returns #MemoryRegion associated with @dimm which
- * is directly mapped into the physical address space of guest.
+ * is directly mapped into the physical address space of guest. Will not
+ * fail after the device was realized.
  * @get_vmstate_memory_region: returns #MemoryRegion which indicates the
- * memory of @dimm should be kept during live migration.
+ * memory of @dimm should be kept during live migration. Will not fail
+ * after the device was realized.
  */
 typedef struct PCDIMMDeviceClass {
     /* private */
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0

[Haozhong Zhang]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

On 06/15/18 16:04, David Hildenbrand wrote:
> It is inititally 0, so setting it to 0 should be allowed, too.

I'm fine with this change and believe nothing is broken in practice,
but what is expected by the user who sets a zero label size?

Look at nvdimm_dsm_device() which enables label DSMs only if the label
size is not smaller than 128 KB. If a user sets a zero label size
explicitly, does he/she expect those label DSMs are available in
guest?  (according to Intel spec, the minimal label size is 128
KBytes)

I think if it's allowed to set a zero label-size, it would be better
to document its difference from other non-zero values in docs/nvdimm.txt.

Thanks,
Haozhong

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/nvdimm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index db7d8c3050..df7646488b 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -52,9 +52,9 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>      if (local_err) {
>          goto out;
>      }
> -    if (value < MIN_NAMESPACE_LABEL_SIZE) {
> +    if (value && value < MIN_NAMESPACE_LABEL_SIZE) {
>          error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
> -                   " at least 0x%lx", object_get_typename(obj),
> +                   " either 0 or at least 0x%lx", object_get_typename(obj),
>                     name, value, MIN_NAMESPACE_LABEL_SIZE);
>          goto out;
>      }
> -- 
> 2.17.1
> 
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 02/13] pc: rename pc_dimm_(plug|unplug|...)* into pc_memory_(plug|unplug|...)*

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3572 bytes --]

On Fri, Jun 15, 2018 at 04:04:37PM +0200, David Hildenbrand wrote:
> Use a similar naming scheme as spapr. This way, we can go ahead and
> rename e.g. pc_dimm_memory_plug to pc_dimm_plug, which avoids
> confusion.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/pc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 622e49d6bc..f9250ffae7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1674,8 +1674,8 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>      }
>  }
>  
> -static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> -                         DeviceState *dev, Error **errp)
> +static void pc_memory_plug(HotplugHandler *hotplug_dev,
> +                           DeviceState *dev, Error **errp)
>  {
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
> @@ -1728,8 +1728,8 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static void pc_dimm_unplug_request(HotplugHandler *hotplug_dev,
> -                                   DeviceState *dev, Error **errp)
> +static void pc_memory_unplug_request(HotplugHandler *hotplug_dev,
> +                                     DeviceState *dev, Error **errp)
>  {
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
> @@ -1759,8 +1759,8 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
> -                           DeviceState *dev, Error **errp)
> +static void pc_memory_unplug(HotplugHandler *hotplug_dev,
> +                             DeviceState *dev, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>      HotplugHandlerClass *hhc;
> @@ -2015,7 +2015,7 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_plug(hotplug_dev, dev, errp);
> +        pc_memory_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_plug(hotplug_dev, dev, errp);
>      }
> @@ -2025,7 +2025,7 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                                  DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_unplug_request(hotplug_dev, dev, errp);
> +        pc_memory_unplug_request(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
>      } else {
> @@ -2038,7 +2038,7 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                          DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_unplug(hotplug_dev, dev, errp);
> +        pc_memory_unplug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          pc_cpu_unplug_cb(hotplug_dev, dev, errp);
>      } else {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 03/13] pc-dimm: rename pc_dimm_memory_* to pc_dimm_*

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4310 bytes --]

On Fri, Jun 15, 2018 at 04:04:38PM +0200, David Hildenbrand wrote:
> Let's rename it to make it look more consistent.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

ppc parts

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/pc.c             | 4 ++--
>  hw/mem/pc-dimm.c         | 6 +++---
>  hw/ppc/spapr.c           | 6 +++---
>  include/hw/mem/pc-dimm.h | 6 +++---
>  4 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f9250ffae7..f23133facc 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1713,7 +1713,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
> +    pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -1773,7 +1773,7 @@ static void pc_memory_unplug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    pc_dimm_memory_unplug(dev, MACHINE(pcms));
> +    pc_dimm_unplug(dev, MACHINE(pcms));
>      object_unparent(OBJECT(dev));
>  
>   out:
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 62b34a992e..9e0c83e415 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,8 +27,8 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> -void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
> -                         uint64_t align, Error **errp)
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
> +                  Error **errp)
>  {
>      int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
> @@ -84,7 +84,7 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine)
> +void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..3e5320020f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3153,7 +3153,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, MACHINE(ms), align, &local_err);
> +    pc_dimm_plug(dev, MACHINE(ms), align, &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -3176,7 +3176,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      return;
>  
>  out_unplug:
> -    pc_dimm_memory_unplug(dev, MACHINE(ms));
> +    pc_dimm_unplug(dev, MACHINE(ms));
>  out:
>      error_propagate(errp, local_err);
>  }
> @@ -3328,7 +3328,7 @@ static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
>      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
>  
> -    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
> +    pc_dimm_unplug(dev, MACHINE(hotplug_dev));
>      object_unparent(OBJECT(dev));
>      spapr_pending_dimm_unplugs_remove(spapr, ds);
>  }
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 627c8601d9..860343d64f 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -78,7 +78,7 @@ typedef struct PCDIMMDeviceClass {
>  
>  int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> -void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
> -                         uint64_t align, Error **errp);
> -void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
> +                  Error **errp);
> +void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
>  #endif

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 04/13] pc-dimm: remove pc_dimm_get_free_slot() from header

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

On Fri, Jun 15, 2018 at 04:04:39PM +0200, David Hildenbrand wrote:
> Not used outside of pc-dimm.c and there shouldn't be other users. If
> other devices (e.g. memory devices) ever have to also use slots, then we
> will have to factor this out.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mem/pc-dimm.c         | 4 +++-
>  include/hw/mem/pc-dimm.h | 2 --
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 9e0c83e415..7387963cf1 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -27,6 +27,8 @@
>  #include "sysemu/numa.h"
>  #include "trace.h"
>  
> +static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
> +
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp)
>  {
> @@ -111,7 +113,7 @@ static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
>      return 0;
>  }
>  
> -int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
> +static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp)
>  {
>      unsigned long *bitmap;
>      int slot = 0;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 860343d64f..cf71247630 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -76,8 +76,6 @@ typedef struct PCDIMMDeviceClass {
>      MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
>  } PCDIMMDeviceClass;
>  
> -int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
> -
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>                    Error **errp);
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 08/13] pc-dimm: merge get_(vmstate_)memory_region()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3673 bytes --]

On Fri, Jun 15, 2018 at 04:04:43PM +0200, David Hildenbrand wrote:
> Importantly, get_vmstate_memory_region() should also fail with a proper
> error if called before the device is realized. For a PCDIMM, both functions
> are to return the same thing, so share the implementation.
> 
> All current users are called after the device has been realized, so we
> can expect the calls to succeed.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mem/pc-dimm.c         | 13 +++++--------
>  include/hw/mem/pc-dimm.h |  3 ++-
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 73f0eee4c7..4ff39b59ef 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -35,7 +35,8 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>      int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
> +    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> +                                                              &error_abort);
>      Error *local_err = NULL;
>      MemoryRegion *mr;
>      uint64_t addr;
> @@ -90,7 +91,8 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
> +    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> +                                                              &error_abort);
>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>  
>      memory_device_unplug_region(machine, mr);
> @@ -229,11 +231,6 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>      return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
> -static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> -{
> -    return host_memory_backend_get_memory(dimm->hostmem);
> -}
> -
>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>  {
>      const PCDIMMDevice *dimm = PC_DIMM(md);
> @@ -298,7 +295,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      dc->desc = "DIMM memory module";
>  
>      ddc->get_memory_region = pc_dimm_get_memory_region;
> -    ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
> +    ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>  
>      mdc->get_addr = pc_dimm_md_get_addr;
>      /* for a dimm plugged_size == region_size */
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index cf71247630..5679a80465 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -73,7 +73,8 @@ typedef struct PCDIMMDeviceClass {
>      /* public */
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
>      MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> -    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> +    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm,
> +                                               Error **errp);
>  } PCDIMMDeviceClass;
>  
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 09/13] nvdimm: convert "unarmed" into a static property

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

On Fri, Jun 15, 2018 at 04:04:44PM +0200, David Hildenbrand wrote:
> We don't allow to modify it after realization. So we can simply turn
> it into a static property.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mem/nvdimm.c | 32 +++++++-------------------------
>  1 file changed, 7 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index df9716231f..7260c9c6b1 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -64,36 +64,11 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static bool nvdimm_get_unarmed(Object *obj, Error **errp)
> -{
> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
> -
> -    return nvdimm->unarmed;
> -}
> -
> -static void nvdimm_set_unarmed(Object *obj, bool value, Error **errp)
> -{
> -    NVDIMMDevice *nvdimm = NVDIMM(obj);
> -    Error *local_err = NULL;
> -
> -    if (memory_region_size(&nvdimm->nvdimm_mr)) {
> -        error_setg(&local_err, "cannot change property value");
> -        goto out;
> -    }
> -
> -    nvdimm->unarmed = value;
> -
> - out:
> -    error_propagate(errp, local_err);
> -}
> -
>  static void nvdimm_init(Object *obj)
>  {
>      object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
>                          nvdimm_get_label_size, nvdimm_set_label_size, NULL,
>                          NULL, NULL);
> -    object_property_add_bool(obj, NVDIMM_UNARMED_PROP,
> -                             nvdimm_get_unarmed, nvdimm_set_unarmed, NULL);
>  }
>  
>  static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> @@ -166,13 +141,20 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>      memory_region_set_dirty(mr, backend_offset, size);
>  }
>  
> +static Property nvdimm_properties[] = {
> +    DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void nvdimm_class_init(ObjectClass *oc, void *data)
>  {
>      PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
>      NVDIMMClass *nvc = NVDIMM_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      ddc->realize = nvdimm_realize;
>      ddc->get_memory_region = nvdimm_get_memory_region;
> +    dc->props = nvdimm_properties;
>  
>      nvc->read_label_data = nvdimm_read_label_data;
>      nvc->write_label_data = nvdimm_write_label_data;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 10/13] nvdimm: convert nvdimm_mr into a pointer

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2600 bytes --]

On Fri, Jun 15, 2018 at 04:04:45PM +0200, David Hildenbrand wrote:
> This way we can easily check if the region has already been inititalized
> without having to rely on the size of an uninitialized region being 0.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

I'm not terribly convinced that this is a worthwhile change, but in
the sense that the patch appears to be technically correct:

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mem/nvdimm.c         | 9 +++++----
>  include/hw/mem/nvdimm.h | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7260c9c6b1..db7d8c3050 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -43,7 +43,7 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>      Error *local_err = NULL;
>      uint64_t value;
>  
> -    if (memory_region_size(&nvdimm->nvdimm_mr)) {
> +    if (nvdimm->nvdimm_mr) {
>          error_setg(&local_err, "cannot change property value");
>          goto out;
>      }
> @@ -75,7 +75,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>  
> -    return &nvdimm->nvdimm_mr;
> +    return nvdimm->nvdimm_mr;
>  }
>  
>  static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
> @@ -102,9 +102,10 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>          return;
>      }
>  
> -    memory_region_init_alias(&nvdimm->nvdimm_mr, OBJECT(dimm),
> +    nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
> +    memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
>                               "nvdimm-memory", mr, 0, pmem_size);
> -    nvdimm->nvdimm_mr.align = align;
> +    nvdimm->nvdimm_mr->align = align;
>  }
>  
>  /*
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 9340631cfc..c5c9b3c7f8 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -74,7 +74,7 @@ struct NVDIMMDevice {
>       * it's the PMEM region in NVDIMM device, which is presented to
>       * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
>       */
> -    MemoryRegion nvdimm_mr;
> +    MemoryRegion *nvdimm_mr;
>  
>      /*
>       * The 'on' value results in the unarmed flag set in ACPI NFIT,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 13/13] pc-dimm: get_memory_region() will not fail after realize

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5113 bytes --]

On Fri, Jun 15, 2018 at 04:04:48PM +0200, David Hildenbrand wrote:
> Let's try to reduce error handling a bit. In the plug/unplug case, the
> device was realized and therefore we can assume that getting access to
> the memory region will not fail.
> 
> For get_vmstate_memory_region() this is already handled that way.
> Document both cases.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

and ppc parts

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/pc.c             |  7 +------
>  hw/mem/pc-dimm.c         |  7 +------
>  hw/ppc/spapr.c           | 12 ++----------
>  include/hw/mem/pc-dimm.h |  6 ++++--
>  4 files changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2db032a6df..f310040351 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1706,15 +1706,10 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
>      if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 4ff39b59ef..65843bc52a 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -37,15 +37,10 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
>                                                                &error_abort);
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      Error *local_err = NULL;
> -    MemoryRegion *mr;
>      uint64_t addr;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
>      addr = object_property_get_uint(OBJECT(dimm),
>                                      PC_DIMM_ADDR_PROP, &local_err);
>      if (local_err) {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3e5320020f..6934abc21e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      uint64_t align, size, addr;
>      uint32_t node;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>      align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> @@ -3340,16 +3336,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
>      Error *local_err = NULL;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> +    MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
>      int i;
>      sPAPRDRConnector *drc;
>  
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>      size = memory_region_size(mr);
>      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>  
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 5679a80465..26ebb7d5e9 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -62,9 +62,11 @@ typedef struct PCDIMMDevice {
>   * @realize: called after common dimm is realized so that the dimm based
>   * devices get the chance to do specified operations.
>   * @get_memory_region: returns #MemoryRegion associated with @dimm which
> - * is directly mapped into the physical address space of guest.
> + * is directly mapped into the physical address space of guest. Will not
> + * fail after the device was realized.
>   * @get_vmstate_memory_region: returns #MemoryRegion which indicates the
> - * memory of @dimm should be kept during live migration.
> + * memory of @dimm should be kept during live migration. Will not fail
> + * after the device was realized.
>   */
>  typedef struct PCDIMMDeviceClass {
>      /* private */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0

[David Hildenbrand]

On 16.06.2018 04:05, Haozhong Zhang wrote:
> On 06/15/18 16:04, David Hildenbrand wrote:
>> It is inititally 0, so setting it to 0 should be allowed, too.
> 
> I'm fine with this change and believe nothing is broken in practice,
> but what is expected by the user who sets a zero label size?

I'd say exactly the same as if not specifying a label size, because the
default is initially 0.

I remember that the user should be able to spell everything out on the
cmdline. Relying only on default values is usually not what we want.

> 
> Look at nvdimm_dsm_device() which enables label DSMs only if the label
> size is not smaller than 128 KB. If a user sets a zero label size
> explicitly, does he/she expect those label DSMs are available in
> guest?  (according to Intel spec, the minimal label size is 128
> KBytes)
> 
> I think if it's allowed to set a zero label-size, it would be better
> to document its difference from other non-zero values in docs/nvdimm.txt.

We can fixup the the documentation to to explicitly state "default is
label-size=0" and "With label-size=0 label support is disabled.".

But this will be a separate patch.

Thanks!

> 
> Thanks,
> Haozhong
> 
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/mem/nvdimm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
>> index db7d8c3050..df7646488b 100644
>> --- a/hw/mem/nvdimm.c
>> +++ b/hw/mem/nvdimm.c
>> @@ -52,9 +52,9 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>>      if (local_err) {
>>          goto out;
>>      }
>> -    if (value < MIN_NAMESPACE_LABEL_SIZE) {
>> +    if (value && value < MIN_NAMESPACE_LABEL_SIZE) {
>>          error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
>> -                   " at least 0x%lx", object_get_typename(obj),
>> +                   " either 0 or at least 0x%lx", object_get_typename(obj),
>>                     name, value, MIN_NAMESPACE_LABEL_SIZE);
>>          goto out;
>>      }
>> -- 
>> 2.17.1
>>
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 10/13] nvdimm: convert nvdimm_mr into a pointer

[David Hildenbrand]

On 18.06.2018 02:49, David Gibson wrote:
> On Fri, Jun 15, 2018 at 04:04:45PM +0200, David Hildenbrand wrote:
>> This way we can easily check if the region has already been inititalized
>> without having to rely on the size of an uninitialized region being 0.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> I'm not terribly convinced that this is a worthwhile change, but in
> the sense that the patch appears to be technically correct:
> 

Igor requested this, and as I had no strong feelings about it, I
included it. At least we are not able to hand out "uninitialized
regions" this way anymore.

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 08/13] pc-dimm: merge get_(vmstate_)memory_region()

[Igor Mammedov]

On Fri, 15 Jun 2018 16:04:43 +0200
David Hildenbrand <david@redhat.com> wrote:

> Importantly, get_vmstate_memory_region() should also fail with a proper
> error if called before the device is realized. For a PCDIMM, both functions
> are to return the same thing, so share the implementation.
> 
> All current users are called after the device has been realized, so we
> can expect the calls to succeed.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/mem/pc-dimm.c         | 13 +++++--------
>  include/hw/mem/pc-dimm.h |  3 ++-
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 73f0eee4c7..4ff39b59ef 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -35,7 +35,8 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
>      int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
> +    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> +                                                              &error_abort);
>      Error *local_err = NULL;
>      MemoryRegion *mr;
>      uint64_t addr;
> @@ -90,7 +91,8 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
> +    MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> +                                                              &error_abort);
>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>  
>      memory_device_unplug_region(machine, mr);
> @@ -229,11 +231,6 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>      return host_memory_backend_get_memory(dimm->hostmem);
>  }
>  
> -static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
> -{
> -    return host_memory_backend_get_memory(dimm->hostmem);
> -}
> -
>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>  {
>      const PCDIMMDevice *dimm = PC_DIMM(md);
> @@ -298,7 +295,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      dc->desc = "DIMM memory module";
>  
>      ddc->get_memory_region = pc_dimm_get_memory_region;
> -    ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
> +    ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>  
>      mdc->get_addr = pc_dimm_md_get_addr;
>      /* for a dimm plugged_size == region_size */
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index cf71247630..5679a80465 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -73,7 +73,8 @@ typedef struct PCDIMMDeviceClass {
>      /* public */
>      void (*realize)(PCDIMMDevice *dimm, Error **errp);
>      MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> -    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm);
> +    MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm,
> +                                               Error **errp);
>  } PCDIMMDeviceClass;
>  
>  void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,

Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0

[Igor Mammedov]

On Fri, 15 Jun 2018 16:04:46 +0200
David Hildenbrand <david@redhat.com> wrote:

> It is inititally 0, so setting it to 0 should be allowed, too.
I'm not sure if we need to permit it.
By default labels are disabled (label-size=0) and user are supposed to provide
this option if labels should be enabled with a valid size. 

it could be confusing for user when asking for label and not getting it.

I suggest to drop this patch, it's not really related to this series
nor required for your future work.

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/mem/nvdimm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index db7d8c3050..df7646488b 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -52,9 +52,9 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>      if (local_err) {
>          goto out;
>      }
> -    if (value < MIN_NAMESPACE_LABEL_SIZE) {
> +    if (value && value < MIN_NAMESPACE_LABEL_SIZE) {
>          error_setg(&local_err, "Property '%s.%s' (0x%" PRIx64 ") is required"
> -                   " at least 0x%lx", object_get_typename(obj),
> +                   " either 0 or at least 0x%lx", object_get_typename(obj),
>                     name, value, MIN_NAMESPACE_LABEL_SIZE);
>          goto out;
>      }

Re: [Qemu-devel] [PATCH v3 11/13] nvdimm: allow setting the label-size to 0

[David Hildenbrand]

On 18.06.2018 14:03, Igor Mammedov wrote:
> On Fri, 15 Jun 2018 16:04:46 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> It is inititally 0, so setting it to 0 should be allowed, too.
> I'm not sure if we need to permit it.
> By default labels are disabled (label-size=0) and user are supposed to provide
> this option if labels should be enabled with a valid size. 
> 
> it could be confusing for user when asking for label and not getting it.
> 
> I suggest to drop this patch, it's not really related to this series
> nor required for your future work.
> 

As I just want to finally get this stuff off my table, I agree to
whatever you say.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 00/13] pc-dimm: next bunch of cleanups

[David Hildenbrand]

On 15.06.2018 16:04, David Hildenbrand wrote:
> This is another set of cleanups as the result from
>     [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
> And is based on
>     [PATCH v1 0/2] memory: fix alignment checks/asserts
> 
> These cleanup are the last step before
> - 1. moving pc-dimm address and slot assignment into pc_dimm_pre_plug
> - 2. factoring out pre_plug, plug and unplug logic of memory devices
>      completely
> 
> v2 -> v3:
> - "nvdimm: no need to overwrite get_vmstate_memory_region()"
> -- reshuffeld to avoid breaking compilation in between patches
> - "nvdimm: convert "label-size" into a static property"
> -- replace by "nvdimm: convert nvdimm_mr into a pointer"
> -- and "nvdimm: allow setting the label-size to 0"
> 
> v1 -> v2:
> - deferring "pc-dimm: assign and verify the "slot" property during pre_plug"
> - deferring "pc-dimm: introduce and use pc_dimm_memory_pre_plug()"
> - dropped "pc-dimm: get_memory_region() will never return a NULL pointer"
> - dropped "pc-dimm: don't allow to access "size" before the device was realized"
> - dropped "spapr: move memory hotplug size check into plug code"
> - dropped "pc-dimm: get_memory_region() can never fail"
> -- replaces by "pc-dimm: get_memory_region() will not fail after realize"
> - added "pc: rename pc_dimm_(plug|unplug|...)* into ..."
> - added "pc-dimm: rename pc_dimm_memory_* to pc_dimm_*"
> - added "pc-dimm: remove pc_dimm_get_free_slot() from header"
> - added "pc-dimm: merge get_(vmstate_)memory_region()"
> - added "nvdimm: convert "unarmed" into a static property"
> - added "nvdimm: convert "label-size" into a static property"
> - added "nvdimm: make get_memory_region() perform checks and initialization"
> 
> David Hildenbrand (13):
>   pc-dimm: remove leftover "struct pc_dimms_capacity"
>   pc: rename pc_dimm_(plug|unplug|...)* into
>     pc_memory_(plug|unplug|...)*
>   pc-dimm: rename pc_dimm_memory_* to pc_dimm_*
>   pc-dimm: remove pc_dimm_get_free_slot() from header
>   pc: factor out pc specific dimm checks into pc_memory_pre_plug()
>   nvdimm: no need to overwrite get_vmstate_memory_region()
>   hostmem: drop error variable from host_memory_backend_get_memory()
>   pc-dimm: merge get_(vmstate_)memory_region()
>   nvdimm: convert "unarmed" into a static property
>   nvdimm: convert nvdimm_mr into a pointer
>   nvdimm: allow setting the label-size to 0
>   nvdimm: make get_memory_region() perform checks and initialization
>   pc-dimm: get_memory_region() will not fail after realize
> 
>  backends/hostmem.c       |  3 +-
>  hw/i386/pc.c             | 73 +++++++++++++++---------------
>  hw/mem/nvdimm.c          | 95 +++++++++++++++++++++-------------------
>  hw/mem/pc-dimm.c         | 35 +++++----------
>  hw/misc/ivshmem.c        |  3 +-
>  hw/ppc/spapr.c           | 18 +++-----
>  include/hw/mem/nvdimm.h  |  2 +-
>  include/hw/mem/pc-dimm.h | 17 +++----
>  include/sysemu/hostmem.h |  3 +-
>  numa.c                   |  3 +-
>  10 files changed, 119 insertions(+), 133 deletions(-)
> 

If there are no more comments I'll send a v4 without patch #11.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 10/13] nvdimm: convert nvdimm_mr into a pointer

[Igor Mammedov]

On Fri, 15 Jun 2018 16:04:45 +0200
David Hildenbrand <david@redhat.com> wrote:

> This way we can easily check if the region has already been inititalized
> without having to rely on the size of an uninitialized region being 0.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/mem/nvdimm.c         | 9 +++++----
>  include/hw/mem/nvdimm.h | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7260c9c6b1..db7d8c3050 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -43,7 +43,7 @@ static void nvdimm_set_label_size(Object *obj, Visitor *v, const char *name,
>      Error *local_err = NULL;
>      uint64_t value;
>  
> -    if (memory_region_size(&nvdimm->nvdimm_mr)) {
> +    if (nvdimm->nvdimm_mr) {
>          error_setg(&local_err, "cannot change property value");
>          goto out;
>      }
> @@ -75,7 +75,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  {
>      NVDIMMDevice *nvdimm = NVDIMM(dimm);
>  
> -    return &nvdimm->nvdimm_mr;
> +    return nvdimm->nvdimm_mr;
>  }
>  
>  static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
> @@ -102,9 +102,10 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>          return;
>      }
>  
> -    memory_region_init_alias(&nvdimm->nvdimm_mr, OBJECT(dimm),
> +    nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
> +    memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
>                               "nvdimm-memory", mr, 0, pmem_size);
> -    nvdimm->nvdimm_mr.align = align;
> +    nvdimm->nvdimm_mr->align = align;
>  }
>  
>  /*
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 9340631cfc..c5c9b3c7f8 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -74,7 +74,7 @@ struct NVDIMMDevice {
>       * it's the PMEM region in NVDIMM device, which is presented to
>       * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
>       */
> -    MemoryRegion nvdimm_mr;
> +    MemoryRegion *nvdimm_mr;
>  
>      /*
>       * The 'on' value results in the unarmed flag set in ACPI NFIT,

Re: [Qemu-devel] [PATCH v3 12/13] nvdimm: make get_memory_region() perform checks and initialization

[Igor Mammedov]

On Fri, 15 Jun 2018 16:04:47 +0200
David Hildenbrand <david@redhat.com> wrote:

> We might get a call to get_memory_region() before the device has been
> realized. We should return a consistent value, as the return value
> will e.g. later on be used in the pre_plug handler.
> 
> To avoid duplicating too much code, factor the initialization and checks
> out into a helper function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/mem/nvdimm.c | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index df7646488b..deba5719bc 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -71,20 +71,24 @@ static void nvdimm_init(Object *obj)
>                          NULL, NULL);
>  }
>  
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
>  {
> -    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> +    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
> +    uint64_t align, pmem_size, size;
> +    MemoryRegion *mr;
>  
> -    return nvdimm->nvdimm_mr;
> -}
> +    if (nvdimm->nvdimm_mr) {
> +        return;
> +    }
>  
> -static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
> -{
> -    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
> -    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> -    uint64_t align, pmem_size, size = memory_region_size(mr);
> +    if (!dimm->hostmem) {
> +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> +        return;
> +    }
>  
> +    mr = host_memory_backend_get_memory(dimm->hostmem);
>      align = memory_region_get_alignment(mr);
> +    size = memory_region_size(mr);
>  
>      pmem_size = size - nvdimm->label_size;
>      nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
> @@ -108,6 +112,30 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
>      nvdimm->nvdimm_mr->align = align;
>  }
>  
> +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> +    Error *local_err = NULL;
> +
> +    if (!nvdimm->nvdimm_mr) {
> +        nvdimm_prepare_memory_region(nvdimm, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return NULL;
> +        }
> +    }
> +    return nvdimm->nvdimm_mr;
> +}
> +
> +static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
> +{
> +    NVDIMMDevice *nvdimm = NVDIMM(dimm);
> +
> +    if (!nvdimm->nvdimm_mr) {
> +        nvdimm_prepare_memory_region(nvdimm, errp);
> +    }
> +}
> +
>  /*
>   * the caller should check the input parameters before calling
>   * label read/write functions.