[Qemu-devel] [PATCH v1 00/11] memory-device: complete refactoring

[Qemu-devel] [PATCH v1 00/11] memory-device: complete refactoring

[David Hildenbrand]

This is another part of the original series
    [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
And is based on
    [PATCH v3 0/4] pc-dimm: pre_plug "slot" and "addr" assignment

This series completes refactoring of pre_plug, plug and unplug logic of
memory devices. With this as a basis, one can easily have e.g. virtio
based memory devices (virtio-mem, virtio-pmem, virtio-fs?) with minor
modifications on e.g. x86 and s390x.

Unfortunately, the "addr" property is already used for virtio devices, so
we will have to deal with device specific properties. So
set_addr() for memory devices is introduced to handle that (we already
have get_addr()).

The only way I see to avoid that would be for virtio based devices to
introduce an indirection:

E.g. right now for my virtio-mem prototype:
... -object memory-backend-ram,id=mem0,size=8G \
    -device virtio-mem-pci,id=vm0,memdev=mem0,node=0,phys-addr=0x12345
To something like:
... -object memory-backend-ram,id=mem0,size=8G \
    -object virtio-mem-backend,id=vmb0,memdev=mem0,addr=0x12345 \
    -device virtio-mem-pci,id=vm0,vmem=vmb0 \
Or something like (that might be interesting for virtio-pmem):
... -device virtio-mem-pci \ /* a virtio-mem bus */
    -object memory-backend-ram,id=mem0,size=8G \
    -device virtio-mem-backend,memdev=mem0,addr=0x12345 \

But both alternatives have their pros and cons.

Opinions?

David Hildenbrand (11):
  memory-device: fix error message when hinted address is too small
  memory-device: introduce separate config option
  memory-device: get_region_size()/get_plugged_size() might fail
  memory-device: convert get_region_size() to get_memory_region()
  memory-device: document MemoryDeviceClass
  memory-device: add device class function set_addr()
  pc-dimm: implement memory device class function set_addr()
  memory-device: complete factoring out pre_plug handling
  memory-device: complete factoring out plug handling
  memory-device: complete factoring out unplug handling
  memory-device: trace when pre_assigning/assigning/unassigning
    addresses

 default-configs/i386-softmmu.mak   |  3 +-
 default-configs/ppc64-softmmu.mak  |  3 +-
 default-configs/x86_64-softmmu.mak |  3 +-
 hw/Makefile.objs                   |  2 +-
 hw/mem/Makefile.objs               |  4 +-
 hw/mem/memory-device.c             | 60 +++++++++++++++++++-----
 hw/mem/pc-dimm.c                   | 75 +++++++++++++++++-------------
 hw/mem/trace-events                |  5 +-
 include/hw/mem/memory-device.h     | 30 ++++++++----
 qapi/misc.json                     |  2 +-
 10 files changed, 126 insertions(+), 61 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v1 01/11] memory-device: fix error message when hinted address is too small

[David Hildenbrand]

The "at" should actually be a "before".
    if (new_addr < address_space_start)
     -> "can't add memory ... before... $address_space_start"

So it looks similar to the other check
    } else if ((new_addr + size) > address_space_end)
     -> "can't add memory ... beyond..."

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

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 6de4f70bb4..efacbc2a7d 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *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);
+                       "] before 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
-- 
2.17.1

[Qemu-devel] [PATCH v1 02/11] memory-device: introduce separate config option

[David Hildenbrand]

Some architectures might support memory devices, while they don't
support DIMM/NVDIMM. So let's
- Rename CONFIG_MEM_HOTPLUG to CONFIG_MEM_DEVICE
- Introduce CONFIG_DIMM and use it similarly to CONFIG NVDIMM

CONFIG_DIMM and CONFIG_NVDIMM require CONFIG_MEM_DEVICE.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 default-configs/i386-softmmu.mak   | 3 ++-
 default-configs/ppc64-softmmu.mak  | 3 ++-
 default-configs/x86_64-softmmu.mak | 3 ++-
 hw/Makefile.objs                   | 2 +-
 hw/mem/Makefile.objs               | 4 ++--
 qapi/misc.json                     | 2 +-
 6 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 8c7d4a0fa0..4c1637338b 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -50,7 +50,8 @@ CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
 CONFIG_PVPANIC=y
-CONFIG_MEM_HOTPLUG=y
+CONFIG_MEM_DEVICE=y
+CONFIG_DIMM=y
 CONFIG_NVDIMM=y
 CONFIG_ACPI_NVDIMM=y
 CONFIG_PCIE_PORT=y
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index b94af6c7c6..f550573782 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -16,4 +16,5 @@ CONFIG_VIRTIO_VGA=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
-CONFIG_MEM_HOTPLUG=y
+CONFIG_MEM_DEVICE=y
+CONFIG_DIMM=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 0390b4303c..7785351414 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -50,7 +50,8 @@ CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
 CONFIG_PVPANIC=y
-CONFIG_MEM_HOTPLUG=y
+CONFIG_MEM_DEVICE=y
+CONFIG_DIMM=y
 CONFIG_NVDIMM=y
 CONFIG_ACPI_NVDIMM=y
 CONFIG_PCIE_PORT=y
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index a19c1417ed..58872e27e0 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -33,7 +33,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += vfio/
 devices-dirs-$(CONFIG_SOFTMMU) += virtio/
 devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
 devices-dirs-$(CONFIG_SOFTMMU) += xen/
-devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/
+devices-dirs-$(CONFIG_MEM_DEVICE) += mem/
 devices-dirs-$(CONFIG_SOFTMMU) += smbios/
 devices-dirs-y += core/
 common-obj-y += $(devices-dirs-y)
diff --git a/hw/mem/Makefile.objs b/hw/mem/Makefile.objs
index 10be4df2a2..3e2f7c5ca2 100644
--- a/hw/mem/Makefile.objs
+++ b/hw/mem/Makefile.objs
@@ -1,3 +1,3 @@
-common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o
-common-obj-$(CONFIG_MEM_HOTPLUG) += memory-device.o
+common-obj-$(CONFIG_DIMM) += pc-dimm.o
+common-obj-$(CONFIG_MEM_DEVICE) += memory-device.o
 common-obj-$(CONFIG_NVDIMM) += nvdimm.o
diff --git a/qapi/misc.json b/qapi/misc.json
index 29da7856e3..9c4aaab823 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -2061,7 +2061,7 @@
 #
 # @plugged-memory: size of memory that can be hot-unplugged. This field
 #                  is omitted if target doesn't support memory hotplug
-#                  (i.e. CONFIG_MEM_HOTPLUG not defined on build time).
+#                  (i.e. CONFIG_MEM_DEVICE not defined at build time).
 #
 # Since: 2.11.0
 ##
-- 
2.17.1

[Qemu-devel] [PATCH v1 03/11] memory-device: get_region_size()/get_plugged_size() might fail

[David Hildenbrand]

Let's properly forward the error, so in case somebody calls
get_region_size() / get_plugged_size(), he can react on the error.

Users right now call both functions after the device has been realized,
which is guaranteed to no fail (we'll document this behavior in a
follow-up patch).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c         | 6 +++---
 hw/mem/pc-dimm.c               | 5 +++--
 include/hw/mem/memory-device.h | 4 ++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index efacbc2a7d..3ac0d5e505 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -60,7 +60,7 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
 
         if (dev->realized) {
-            *size += mdc->get_region_size(md);
+            *size += mdc->get_region_size(md, &error_abort);
         }
     }
 
@@ -167,7 +167,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
         uint64_t md_size, md_addr;
 
         md_addr = mdc->get_addr(md);
-        md_size = mdc->get_region_size(md);
+        md_size = mdc->get_region_size(md, &error_abort);
         if (*errp) {
             goto out;
         }
@@ -233,7 +233,7 @@ static int memory_device_plugged_size(Object *obj, void *opaque)
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
 
         if (dev->realized) {
-            *size += mdc->get_plugged_size(md);
+            *size += mdc->get_plugged_size(md, &error_abort);
         }
     }
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index fb6bcaedc4..4bf1a0acc9 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -236,14 +236,15 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
     return dimm->addr;
 }
 
-static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md)
+static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,
+                                           Error **errp)
 {
     /* dropping const here is fine as we don't touch the memory region */
     PCDIMMDevice *dimm = PC_DIMM(md);
     const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
     MemoryRegion *mr;
 
-    mr = ddc->get_memory_region(dimm, &error_abort);
+    mr = ddc->get_memory_region(dimm, errp);
     if (!mr) {
         return 0;
     }
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 2853b084b5..f02b229837 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -33,8 +33,8 @@ typedef struct MemoryDeviceClass {
     InterfaceClass parent_class;
 
     uint64_t (*get_addr)(const MemoryDeviceState *md);
-    uint64_t (*get_plugged_size)(const MemoryDeviceState *md);
-    uint64_t (*get_region_size)(const MemoryDeviceState *md);
+    uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
+    uint64_t (*get_region_size)(const MemoryDeviceState *md, Error **errp);
     void (*fill_device_info)(const MemoryDeviceState *md,
                              MemoryDeviceInfo *info);
 } MemoryDeviceClass;
-- 
2.17.1

[Qemu-devel] [PATCH v1 04/11] memory-device: convert get_region_size() to get_memory_region()

[David Hildenbrand]

To factor out plugging and unplugging of memory device we need access to
the memory region. So let's replace get_region_size() by
get_memory_region().

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

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 3ac0d5e505..a69ae343f8 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -56,11 +56,13 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
 
     if (object_dynamic_cast(obj, TYPE_MEMORY_DEVICE)) {
         const DeviceState *dev = DEVICE(obj);
-        const MemoryDeviceState *md = MEMORY_DEVICE(obj);
+        MemoryDeviceState *md = MEMORY_DEVICE(obj);
         const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(obj);
 
         if (dev->realized) {
-            *size += mdc->get_region_size(md, &error_abort);
+            MemoryRegion *mr = mdc->get_memory_region(md, &error_abort);
+
+            *size += memory_region_size(mr);
         }
     }
 
@@ -162,12 +164,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
     /* find address range that will fit new memory device */
     object_child_foreach(OBJECT(ms), memory_device_build_list, &list);
     for (item = list; item; item = g_slist_next(item)) {
-        const MemoryDeviceState *md = item->data;
+        MemoryDeviceState *md = item->data;
         const 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, &error_abort);
+        md_size = memory_region_size(mdc->get_memory_region(md, &error_abort));
         if (*errp) {
             goto out;
         }
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 4bf1a0acc9..a208b17c64 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -236,8 +236,8 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
     return dimm->addr;
 }
 
-static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,
-                                           Error **errp)
+static uint64_t pc_dimm_md_get_plugged_size(const MemoryDeviceState *md,
+                                            Error **errp)
 {
     /* dropping const here is fine as we don't touch the memory region */
     PCDIMMDevice *dimm = PC_DIMM(md);
@@ -249,9 +249,19 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,
         return 0;
     }
 
+    /* for a dimm plugged_size == region_size */
     return memory_region_size(mr);
 }
 
+static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
+                                                  Error **errp)
+{
+    PCDIMMDevice *dimm = PC_DIMM(md);
+    const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
+
+    return ddc->get_memory_region(dimm, errp);
+}
+
 static void pc_dimm_md_fill_device_info(const MemoryDeviceState *md,
                                         MemoryDeviceInfo *info)
 {
@@ -297,9 +307,8 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
     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 */
-    mdc->get_plugged_size = pc_dimm_md_get_region_size;
-    mdc->get_region_size = pc_dimm_md_get_region_size;
+    mdc->get_plugged_size = pc_dimm_md_get_plugged_size;
+    mdc->get_memory_region = pc_dimm_md_get_memory_region;
     mdc->fill_device_info = pc_dimm_md_fill_device_info;
 }
 
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index f02b229837..852fd8f98a 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -34,7 +34,7 @@ typedef struct MemoryDeviceClass {
 
     uint64_t (*get_addr)(const MemoryDeviceState *md);
     uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
-    uint64_t (*get_region_size)(const MemoryDeviceState *md, Error **errp);
+    MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
     void (*fill_device_info)(const MemoryDeviceState *md,
                              MemoryDeviceInfo *info);
 } MemoryDeviceClass;
-- 
2.17.1

[Qemu-devel] [PATCH v1 05/11] memory-device: document MemoryDeviceClass

[David Hildenbrand]

Document the functions and when to not expect errors.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/hw/mem/memory-device.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 852fd8f98a..0c1fd66b68 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -29,9 +29,23 @@ typedef struct MemoryDeviceState {
     Object parent_obj;
 } MemoryDeviceState;
 
+/**
+ * MemoryDeviceClass:
+ * @get_addr: The address of the @md in guest physical memory. "0" means that
+ * no address has been specified by the user and that no address has been
+ * assigned yet.
+ * @get_plugged_size: The the amount of memory provided by this @md
+ * currently usable ("plugged") by the guest. Will not fail after the device
+ * was realized.
+ * @get_memory_region: The memory region of the @md to mapped in guest
+ * physical memory at @get_addr. Will not fail after the device was realized.
+ * @fill_device_info: Fill out #MemoryDeviceInfo with @md specific information.
+ */
 typedef struct MemoryDeviceClass {
+    /* private */
     InterfaceClass parent_class;
 
+    /* public */
     uint64_t (*get_addr)(const MemoryDeviceState *md);
     uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
     MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
-- 
2.17.1

[Qemu-devel] [PATCH v1 06/11] memory-device: add device class function set_addr()

[David Hildenbrand]

To be able to factor out address asignment of memory devices, we will
have to read (get_addr()) and write (set_addr()) the address. We cannot
use properties for this purpose, as properties are device specific. E.g.
while the address property for a DIMM is called "addr", it might be
called differently (e.g. "phys_addr") for other devices. E.g. virtio
based memory devices cannot use "addr" as that is already reserved and
used for the virtio device address on the bus.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/hw/mem/memory-device.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 0c1fd66b68..cf5d182ac2 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -34,6 +34,7 @@ typedef struct MemoryDeviceState {
  * @get_addr: The address of the @md in guest physical memory. "0" means that
  * no address has been specified by the user and that no address has been
  * assigned yet.
+ * @set_addr: Set the address of the @md in guest physical memory.
  * @get_plugged_size: The the amount of memory provided by this @md
  * currently usable ("plugged") by the guest. Will not fail after the device
  * was realized.
@@ -47,6 +48,7 @@ typedef struct MemoryDeviceClass {
 
     /* public */
     uint64_t (*get_addr)(const MemoryDeviceState *md);
+    void (*set_addr)(MemoryDeviceState *md, uint64_t addr, Error **errp);
     uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
     MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
     void (*fill_device_info)(const MemoryDeviceState *md,
-- 
2.17.1

[Qemu-devel] [PATCH v1 07/11] pc-dimm: implement memory device class function set_addr()

[David Hildenbrand]

We can change the address as long as the device has not been plugged
(== memory region mapped) yet.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/pc-dimm.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index a208b17c64..bb8e10903f 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -236,6 +236,28 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
     return dimm->addr;
 }
 
+static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
+                                Error **errp)
+{
+    PCDIMMDevice *dimm = PC_DIMM(md);
+    const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
+    Error *local_err = NULL;
+    MemoryRegion *mr;
+
+    mr = ddc->get_memory_region(dimm, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (memory_region_is_mapped(mr)) {
+        error_setg(errp, "Can't change address, memory region already mapped.");
+        return;
+    }
+
+    dimm->addr = addr;
+}
+
 static uint64_t pc_dimm_md_get_plugged_size(const MemoryDeviceState *md,
                                             Error **errp)
 {
@@ -307,6 +329,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
     ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
 
     mdc->get_addr = pc_dimm_md_get_addr;
+    mdc->set_addr = pc_dimm_md_set_addr;
     mdc->get_plugged_size = pc_dimm_md_get_plugged_size;
     mdc->get_memory_region = pc_dimm_md_get_memory_region;
     mdc->fill_device_info = pc_dimm_md_fill_device_info;
-- 
2.17.1

[Qemu-devel] [PATCH v1 08/11] memory-device: complete factoring out pre_plug handling

[David Hildenbrand]

With all required memory device class functions in place, we can factor
out pre_plug handling of memory devices. Take proper care of errors. We
still have to carry along legacy_align required for pc compatibility
handling.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c         | 29 ++++++++++++++++++++++++++---
 hw/mem/pc-dimm.c               | 16 +++-------------
 include/hw/mem/memory-device.h |  5 ++---
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index a69ae343f8..3ed067debb 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -96,9 +96,10 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,
 
 }
 
-uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
-                                     uint64_t align, uint64_t size,
-                                     Error **errp)
+static uint64_t memory_device_get_free_addr(MachineState *ms,
+                                            const uint64_t *hint,
+                                            uint64_t align, uint64_t size,
+                                            Error **errp)
 {
     uint64_t address_space_start, address_space_end;
     GSList *list = NULL, *item;
@@ -252,6 +253,28 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
+                            const uint64_t *legacy_align, Error **errp)
+{
+    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+    Error *local_err = NULL;
+    uint64_t addr, align;
+    MemoryRegion *mr;
+
+    mr = mdc->get_memory_region(md, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    align = legacy_align ? *legacy_align : memory_region_get_alignment(mr);
+    addr = mdc->get_addr(md);
+    addr = memory_device_get_free_addr(ms, !addr ? NULL : &addr, align,
+                                       memory_region_size(mr), &local_err);
+    mdc->set_addr(md, addr, &local_err);
+out:
+    error_propagate(errp, local_err);
+}
+
 void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
                                uint64_t addr)
 {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index bb8e10903f..33ab0621cc 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -32,11 +32,8 @@ static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
                       const uint64_t *legacy_align, Error **errp)
 {
-    PCDIMMDevice *dimm = PC_DIMM(dev);
-    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     Error *local_err = NULL;
-    MemoryRegion *mr;
-    uint64_t addr, align;
+    uint64_t addr;
     int slot;
 
     slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
@@ -49,22 +46,15 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
     object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort);
     trace_mhp_pc_dimm_assigned_slot(slot);
 
-    mr = ddc->get_memory_region(dimm, &local_err);
+    memory_device_pre_plug(MEMORY_DEVICE(dev), machine, legacy_align,
+                           &local_err);
     if (local_err) {
         goto out;
     }
 
-    align = legacy_align ? *legacy_align : memory_region_get_alignment(mr);
     addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
                                     &error_abort);
-    addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
-                                       memory_region_size(mr), &local_err);
-    if (local_err) {
-        goto out;
-    }
     trace_mhp_pc_dimm_assigned_address(addr);
-    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
-                             &error_abort);
 out:
     error_propagate(errp, local_err);
 }
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index cf5d182ac2..fb920b9f44 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -57,9 +57,8 @@ typedef struct MemoryDeviceClass {
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
-uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
-                                     uint64_t align, uint64_t size,
-                                     Error **errp);
+void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
+                            const uint64_t *legacy_align, Error **errp);
 void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
                                uint64_t addr);
 void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
-- 
2.17.1

[Qemu-devel] [PATCH v1 09/11] memory-device: complete factoring out plug handling

[David Hildenbrand]

With the new memory device functions in place, we can factor out
plugging of memory devices completely.

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

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 3ed067debb..ec81133edf 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -275,9 +275,12 @@ out:
     error_propagate(errp, local_err);
 }
 
-void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
-                               uint64_t addr)
+void memory_device_plug(MemoryDeviceState *md, MachineState *ms)
 {
+    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+    const uint64_t addr = mdc->get_addr(md);
+    MemoryRegion *mr = mdc->get_memory_region(md, &error_abort);
+
     /* we expect a previous call to memory_device_get_free_addr() */
     g_assert(ms->device_memory);
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 33ab0621cc..4760b17062 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -65,13 +65,8 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp)
     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);
-    uint64_t addr;
-
-    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
-                                    &error_abort);
 
-    memory_device_plug_region(machine, mr, addr);
+    memory_device_plug(MEMORY_DEVICE(dev), machine);
     vmstate_register_ram(vmstate_mr, dev);
 }
 
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index fb920b9f44..ea9331a778 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -59,8 +59,7 @@ MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
 void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
                             const uint64_t *legacy_align, Error **errp);
-void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
-                               uint64_t addr);
+void memory_device_plug(MemoryDeviceState *md, MachineState *ms);
 void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
 
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v1 10/11] memory-device: complete factoring out unplug handling

[David Hildenbrand]

With the new memory device functions in place, we can factor out
unplugging of memory devices completely.

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

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index ec81133edf..b52a0972d0 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -288,8 +288,11 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms)
                                 addr - ms->device_memory->base, mr);
 }
 
-void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr)
+void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
 {
+    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+    MemoryRegion *mr = mdc->get_memory_region(md, &error_abort);
+
     /* we expect a previous call to memory_device_get_free_addr() */
     g_assert(ms->device_memory);
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 4760b17062..70c46d2a73 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -76,9 +76,8 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
     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);
 
-    memory_device_unplug_region(machine, mr);
+    memory_device_unplug(MEMORY_DEVICE(dev), machine);
     vmstate_unregister_ram(vmstate_mr, dev);
 }
 
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index ea9331a778..d06b944ee7 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -60,6 +60,6 @@ uint64_t get_plugged_memory_size(void);
 void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
                             const uint64_t *legacy_align, Error **errp);
 void memory_device_plug(MemoryDeviceState *md, MachineState *ms);
-void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
+void memory_device_unplug(MemoryDeviceState *md, MachineState *ms);
 
 #endif
-- 
2.17.1

[Qemu-devel] [PATCH v1 11/11] memory-device: trace when pre_assigning/assigning/unassigning addresses

[David Hildenbrand]

Let's trace the address when pre_pluggin/plugging/unplugging a memory device.

Trace it when pre_plugging as well as when plugging, so we really know
when a specific address is actually used.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 4 ++++
 hw/mem/pc-dimm.c       | 8 --------
 hw/mem/trace-events    | 5 ++++-
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index b52a0972d0..b78b631212 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -17,6 +17,7 @@
 #include "qemu/range.h"
 #include "hw/virtio/vhost.h"
 #include "sysemu/kvm.h"
+#include "trace.h"
 
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {
@@ -271,6 +272,7 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
     addr = memory_device_get_free_addr(ms, !addr ? NULL : &addr, align,
                                        memory_region_size(mr), &local_err);
     mdc->set_addr(md, addr, &local_err);
+    trace_memory_device_pre_assign_address(addr);
 out:
     error_propagate(errp, local_err);
 }
@@ -286,6 +288,7 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms)
 
     memory_region_add_subregion(&ms->device_memory->mr,
                                 addr - ms->device_memory->base, mr);
+    trace_memory_device_assign_address(addr);
 }
 
 void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
@@ -297,6 +300,7 @@ void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
     g_assert(ms->device_memory);
 
     memory_region_del_subregion(&ms->device_memory->mr, mr);
+    trace_memory_device_unassign_address(mdc->get_addr(md));
 }
 
 static const TypeInfo memory_device_info = {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 70c46d2a73..e41a0d535a 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -33,7 +33,6 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
                       const uint64_t *legacy_align, Error **errp)
 {
     Error *local_err = NULL;
-    uint64_t addr;
     int slot;
 
     slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
@@ -48,13 +47,6 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
 
     memory_device_pre_plug(MEMORY_DEVICE(dev), machine, legacy_align,
                            &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
-                                    &error_abort);
-    trace_mhp_pc_dimm_assigned_address(addr);
 out:
     error_propagate(errp, local_err);
 }
diff --git a/hw/mem/trace-events b/hw/mem/trace-events
index e150dcc497..b40482077e 100644
--- a/hw/mem/trace-events
+++ b/hw/mem/trace-events
@@ -2,4 +2,7 @@
 
 # hw/mem/pc-dimm.c
 mhp_pc_dimm_assigned_slot(int slot) "%d"
-mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
+# hw/mem/memory-device.c
+memory_device_pre_assign_address(uint64_t addr) "0x%"PRIx64
+memory_device_assign_address(uint64_t addr) "0x%"PRIx64
+memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 00/11] memory-device: complete refactoring

[Igor Mammedov]

On Thu,  5 Jul 2018 13:59:32 +0200
David Hildenbrand <david@redhat.com> wrote:

> This is another part of the original series
>     [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
> And is based on
>     [PATCH v3 0/4] pc-dimm: pre_plug "slot" and "addr" assignment
> 
> This series completes refactoring of pre_plug, plug and unplug logic of
> memory devices. With this as a basis, one can easily have e.g. virtio
> based memory devices (virtio-mem, virtio-pmem, virtio-fs?) with minor
> modifications on e.g. x86 and s390x.
> 
> Unfortunately, the "addr" property is already used for virtio devices, so
> we will have to deal with device specific properties. So
> set_addr() for memory devices is introduced to handle that (we already
> have get_addr()).
> 
> The only way I see to avoid that would be for virtio based devices to
> introduce an indirection:
> 
> E.g. right now for my virtio-mem prototype:
> ... -object memory-backend-ram,id=mem0,size=8G \
>     -device virtio-mem-pci,id=vm0,memdev=mem0,node=0,phys-addr=0x12345
Though it seems inconvenient to have different property name,
it is not dimm device (even though it shares GPA resource)
so it is fine for it to have it's own set of properties.
(might make error reporting a bit ugly)

Also what about usecase of mmio based virtio (ARM)?


> To something like:
> ... -object memory-backend-ram,id=mem0,size=8G \
>     -object virtio-mem-backend,id=vmb0,memdev=mem0,addr=0x12345 \
>     -device virtio-mem-pci,id=vm0,vmem=vmb0 \
it's broken conceptually,
backends should deal only with host resource allocation while
'addr' is device model property and belongs to a frontend.

> Or something like (that might be interesting for virtio-pmem):
> ... -device virtio-mem-pci \ /* a virtio-mem bus */
>     -object memory-backend-ram,id=mem0,size=8G \
>     -device virtio-mem-backend,memdev=mem0,addr=0x12345 \
did you mean  ^^^^ virtio-pmem device?

Is it a separate controller and memory resource design
connected together somehow?


> But both alternatives have their pros and cons.
> 
> Opinions?
> 
> David Hildenbrand (11):
>   memory-device: fix error message when hinted address is too small
>   memory-device: introduce separate config option
>   memory-device: get_region_size()/get_plugged_size() might fail
>   memory-device: convert get_region_size() to get_memory_region()
>   memory-device: document MemoryDeviceClass
>   memory-device: add device class function set_addr()
>   pc-dimm: implement memory device class function set_addr()
>   memory-device: complete factoring out pre_plug handling
>   memory-device: complete factoring out plug handling
>   memory-device: complete factoring out unplug handling
>   memory-device: trace when pre_assigning/assigning/unassigning
>     addresses
> 
>  default-configs/i386-softmmu.mak   |  3 +-
>  default-configs/ppc64-softmmu.mak  |  3 +-
>  default-configs/x86_64-softmmu.mak |  3 +-
>  hw/Makefile.objs                   |  2 +-
>  hw/mem/Makefile.objs               |  4 +-
>  hw/mem/memory-device.c             | 60 +++++++++++++++++++-----
>  hw/mem/pc-dimm.c                   | 75 +++++++++++++++++-------------
>  hw/mem/trace-events                |  5 +-
>  include/hw/mem/memory-device.h     | 30 ++++++++----
>  qapi/misc.json                     |  2 +-
>  10 files changed, 126 insertions(+), 61 deletions(-)
> 

Re: [Qemu-devel] [PATCH v1 00/11] memory-device: complete refactoring

[David Hildenbrand]

On 23.07.2018 16:23, Igor Mammedov wrote:
> On Thu,  5 Jul 2018 13:59:32 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> This is another part of the original series
>>     [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
>> And is based on
>>     [PATCH v3 0/4] pc-dimm: pre_plug "slot" and "addr" assignment
>>
>> This series completes refactoring of pre_plug, plug and unplug logic of
>> memory devices. With this as a basis, one can easily have e.g. virtio
>> based memory devices (virtio-mem, virtio-pmem, virtio-fs?) with minor
>> modifications on e.g. x86 and s390x.
>>
>> Unfortunately, the "addr" property is already used for virtio devices, so
>> we will have to deal with device specific properties. So
>> set_addr() for memory devices is introduced to handle that (we already
>> have get_addr()).
>>
>> The only way I see to avoid that would be for virtio based devices to
>> introduce an indirection:
>>
>> E.g. right now for my virtio-mem prototype:
>> ... -object memory-backend-ram,id=mem0,size=8G \
>>     -device virtio-mem-pci,id=vm0,memdev=mem0,node=0,phys-addr=0x12345
> Though it seems inconvenient to have different property name,
> it is not dimm device (even though it shares GPA resource)
> so it is fine for it to have it's own set of properties.
> (might make error reporting a bit ugly)

Same opinion here, that's why I prefer this approach.

> 
> Also what about usecase of mmio based virtio (ARM)?

Can you elaborate?

> 
> 
>> To something like:
>> ... -object memory-backend-ram,id=mem0,size=8G \
>>     -object virtio-mem-backend,id=vmb0,memdev=mem0,addr=0x12345 \
>>     -device virtio-mem-pci,id=vm0,vmem=vmb0 \
> it's broken conceptually,
> backends should deal only with host resource allocation while
> 'addr' is device model property and belongs to a frontend.

Agreed.

> 
>> Or something like (that might be interesting for virtio-pmem):
>> ... -device virtio-mem-pci \ /* a virtio-mem bus */
>>     -object memory-backend-ram,id=mem0,size=8G \
>>     -device virtio-mem-backend,memdev=mem0,addr=0x12345 \
> did you mean  ^^^^ virtio-pmem device?
> 
> Is it a separate controller and memory resource design
> connected together somehow?

Yes, for this example s/mem/pmem/ of course (copy paste). virtio-mem and
virtio-pmem are two separate device types with their own set of
controllers (if any).

I favor the original approach (above).


-- 

Thanks,

David / dhildenb