[Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[David Hildenbrand]

Hotplug handlers usually have the following tasks:
1. Allocate some resources for a new device
2. Make the new device visible for the guest
3. Notify the guest about the new device

Hotplug handlers have right now one limitation: They handle their own
context and only care about resources they manage.

We can have devices that need certain other resources that are e.g.
system resources managed by the machine. We need a clean way to assign
these resources (without violating layers as brought up by Igor).

One example is virtio-mem/virtio-pmem. Both device types need to be
assigned some region in guest physical address space. This device memory
belongs to the machine and is managed by it. However, virito devices are
hotplugged using the hotplug handler their proxy device implements. So we
could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
hotplug handler for virtio-ccw. But definetly not the machine.

So let's generalize the task of "assigning" resources and use it directly
for memory devices. We now have a clean way to support any kind of memory
device - independent of the underlying device type. Right now, only one
resource handler per device can be supported (in addition to the existing
hotplug handler).

You can find more details in patch nr 2.

This work is based on the already queued patch series
    "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice"

David Hildenbrand (8):
  memory-device: always compile support for memory devices for SOFTMMU
  qdev: introduce ResourceHandler as a first-stage hotplug handler
  machine: provide default resource handler
  memory-device: new functions to handle resource assignment
  pc-dimm: implement new memory device functions
  machine: introduce enforce_memory_device_align() and add it for pc
  memory-device: factor out pre-assign into default resource handler
  memory-device: factor out (un)assign into default resource handler

 hw/Makefile.objs               |   2 +-
 hw/core/Makefile.objs          |   1 +
 hw/core/machine.c              |  70 +++++++++++++++++++++++
 hw/core/qdev.c                 |  41 +++++++++++++-
 hw/core/resource-handler.c     |  57 +++++++++++++++++++
 hw/i386/pc.c                   |  31 ++++++-----
 hw/mem/Makefile.objs           |   2 +-
 hw/mem/memory-device.c         | 122 +++++++++++++++++++++++++----------------
 hw/mem/pc-dimm.c               |  53 ++++++++----------
 hw/mem/trace-events            |   4 +-
 hw/ppc/spapr.c                 |   5 +-
 include/hw/boards.h            |  17 ++++++
 include/hw/mem/memory-device.h |  17 ++++--
 include/hw/mem/pc-dimm.h       |   3 +-
 include/hw/resource-handler.h  |  46 ++++++++++++++++
 stubs/Makefile.objs            |   1 -
 stubs/qmp_memory_device.c      |  13 -----
 17 files changed, 364 insertions(+), 121 deletions(-)
 create mode 100644 hw/core/resource-handler.c
 create mode 100644 include/hw/resource-handler.h
 delete mode 100644 stubs/qmp_memory_device.c

-- 
2.14.3

[Qemu-devel] [PATCH v1 1/8] memory-device: always compile support for memory devices for SOFTMMU

[David Hildenbrand]

Let's always compile support memory devices. MEM_HOTPLUG is very DIMM
specific.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/Makefile.objs          |  2 +-
 hw/mem/Makefile.objs      |  2 +-
 stubs/Makefile.objs       |  1 -
 stubs/qmp_memory_device.c | 13 -------------
 4 files changed, 2 insertions(+), 16 deletions(-)
 delete mode 100644 stubs/qmp_memory_device.c

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 6a0ffe0afd..3d7d086930 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_SOFTMMU) += 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..c5648fb2c5 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-y += memory-device.o
 common-obj-$(CONFIG_NVDIMM) += nvdimm.o
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 53d3f32cb2..76ab2a9dbb 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -34,7 +34,6 @@ stub-obj-y += uuid.o
 stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
-stub-obj-y += qmp_memory_device.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += pc_madt_cpu_entry.o
diff --git a/stubs/qmp_memory_device.c b/stubs/qmp_memory_device.c
deleted file mode 100644
index 85ff8f2d7e..0000000000
--- a/stubs/qmp_memory_device.c
+++ /dev/null
@@ -1,13 +0,0 @@
-#include "qemu/osdep.h"
-#include "qom/object.h"
-#include "hw/mem/memory-device.h"
-
-MemoryDeviceInfoList *qmp_memory_device_list(void)
-{
-   return NULL;
-}
-
-uint64_t get_plugged_memory_size(void)
-{
-    return (uint64_t)-1;
-}
-- 
2.14.3

[Qemu-devel] [PATCH v1 2/8] qdev: introduce ResourceHandler as a first-stage hotplug handler

[David Hildenbrand]

Hotplug handlers usually do 3 things:
1. Allocate some resources for a new device
2. Make the new device visible for the guest
3. Notify the guest about the new device

While 2. and 3. are only ever performed once for a device, there
might be various resources to allocate. E.g. a MemoryDevice could be
exposed via Virtio. The proxy device hotplug handler (e.g. virtio-pci,
virtio-ccw) takes care of exposing the device to the guest. However,
we also have to allocate system resources, like a portion in guest
physical address space, which is to be handled by the machine.

One key difference between a hotplug handler and a resource handler is
that resource handlers have no "unplug_request". There is no
communication with the guest (this is done by a "hotplug" handler). So
conceptually, they are different.

For now we live with the assumption, that - in additon to the existing
hotplug handler which can do some resource asignment - at most one
resource handler is necessary.

We might even later decide to have multiple resource handlers for a
specific device (e.g. one for each interface they implement). For now,
this design is sufficient.

Resource handlers have to run before the hotplug handler runs (esp
before exposing the device to the guest).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/Makefile.objs         |  1 +
 hw/core/qdev.c                | 41 ++++++++++++++++++++++++++++++-
 hw/core/resource-handler.c    | 57 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h           |  9 +++++++
 include/hw/resource-handler.h | 46 ++++++++++++++++++++++++++++++++++
 5 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 hw/core/resource-handler.c
 create mode 100644 include/hw/resource-handler.h

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index eb88ca979e..7474387fcd 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
 common-obj-y += hotplug.o
+common-obj-y += resource-handler.o
 common-obj-$(CONFIG_SOFTMMU) += nmi.o
 
 common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f6f92473b8..042e275884 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -35,6 +35,7 @@
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "hw/hotplug.h"
+#include "hw/resource-handler.h"
 #include "hw/boards.h"
 #include "hw/sysbus.h"
 
@@ -271,6 +272,17 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
     return hotplug_ctrl;
 }
 
+static ResourceHandler *qdev_get_resource_handler(DeviceState *dev)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    if (mc->get_resource_handler) {
+            return mc->get_resource_handler(ms, dev);
+    }
+    return NULL;
+}
+
 static int qdev_reset_one(DeviceState *dev, void *opaque)
 {
     device_reset(dev);
@@ -814,6 +826,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    ResourceHandler *resource_ctrl;
     HotplugHandler *hotplug_ctrl;
     BusState *bus;
     Error *local_err = NULL;
@@ -825,6 +838,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         return;
     }
 
+    resource_ctrl = qdev_get_resource_handler(dev);
+
     if (value && !dev->realized) {
         if (!check_only_migratable(obj, &local_err)) {
             goto fail;
@@ -840,6 +855,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             g_free(name);
         }
 
+        if (resource_ctrl) {
+            resource_handler_pre_assign(resource_ctrl, dev, &local_err);
+            if (local_err != NULL) {
+                goto fail;
+            }
+        }
+
         hotplug_ctrl = qdev_get_hotplug_handler(dev);
         if (hotplug_ctrl) {
             hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
@@ -858,12 +880,19 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 
         DEVICE_LISTENER_CALL(realize, Forward, dev);
 
+        if (resource_ctrl) {
+            resource_handler_assign(resource_ctrl, dev, &local_err);
+            if (local_err != NULL) {
+                goto post_realize_fail;
+            }
+        }
+
         if (hotplug_ctrl) {
             hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
         }
 
         if (local_err != NULL) {
-            goto post_realize_fail;
+            goto post_assign_fail;
         }
 
         /*
@@ -903,6 +932,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
+
+        if (resource_ctrl) {
+            resource_handler_unassign(resource_ctrl, dev);
+        }
+
         if (dc->unrealize) {
             local_errp = local_err ? NULL : &local_err;
             dc->unrealize(dev, local_errp);
@@ -928,6 +962,11 @@ child_realize_fail:
         vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
     }
 
+post_assign_fail:
+    if (resource_ctrl) {
+        resource_handler_unassign(resource_ctrl, dev);
+    }
+
 post_realize_fail:
     g_free(dev->canonical_path);
     dev->canonical_path = NULL;
diff --git a/hw/core/resource-handler.c b/hw/core/resource-handler.c
new file mode 100644
index 0000000000..0a1ff0e66a
--- /dev/null
+++ b/hw/core/resource-handler.c
@@ -0,0 +1,57 @@
+/*
+ * Resource handler interface.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Authors:
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/resource-handler.h"
+#include "qemu/module.h"
+
+void resource_handler_pre_assign(ResourceHandler *rh,
+                                 const DeviceState *dev, Error **errp)
+{
+    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
+
+    if (rhc->pre_assign) {
+        rhc->pre_assign(rh, dev, errp);
+    }
+}
+
+void resource_handler_assign(ResourceHandler *rh, DeviceState *dev,
+                             Error **errp)
+{
+    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
+
+    if (rhc->assign) {
+        rhc->assign(rh, dev, errp);
+    }
+}
+
+void resource_handler_unassign(ResourceHandler *rh, DeviceState *dev)
+{
+    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
+
+    if (rhc->unassign) {
+        rhc->unassign(rh, dev);
+    }
+}
+
+static const TypeInfo resource_handler_info = {
+    .name          = TYPE_RESOURCE_HANDLER,
+    .parent        = TYPE_INTERFACE,
+    .class_size = sizeof(ResourceHandlerClass),
+};
+
+static void resource_handler_register_types(void)
+{
+    type_register_static(&resource_handler_info);
+}
+
+type_init(resource_handler_register_types)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 8748964e6f..ff5142d7c2 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -8,6 +8,7 @@
 #include "hw/qdev.h"
 #include "qom/object.h"
 #include "qom/cpu.h"
+#include "hw/resource-handler.h"
 
 /**
  * memory_region_allocate_system_memory - Allocate a board's main memory
@@ -115,6 +116,12 @@ typedef struct {
  *    of HotplugHandler object, which handles hotplug operation
  *    for a given @dev. It may return NULL if @dev doesn't require
  *    any actions to be performed by hotplug handler.
+ * @get_resource_handler: this function is called when a new device is
+ *                        about to be hotplugged. If defined, it returns pointer
+ *                        to an instance of ResourceHandler object, which
+ *                        handles resource asignment for a given @dev. It
+ *                        may return NULL if @dev doesn't require any actions
+ *                        to be performed by a resource handler.
  * @cpu_index_to_instance_props:
  *    used to provide @cpu_index to socket/core/thread number mapping, allowing
  *    legacy code to perform maping from cpu_index to topology properties
@@ -208,6 +215,8 @@ struct MachineClass {
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
+    ResourceHandler *(*get_resource_handler)(MachineState *machine,
+                                             const DeviceState *dev);
     CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
                                                          unsigned cpu_index);
     const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
diff --git a/include/hw/resource-handler.h b/include/hw/resource-handler.h
new file mode 100644
index 0000000000..3eda1bec5c
--- /dev/null
+++ b/include/hw/resource-handler.h
@@ -0,0 +1,46 @@
+/*
+ * Resource handler interface.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Authors:
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef RESOURCE_HANDLER_H
+#define RESOURCE_HANDLER_H
+
+#include "qom/object.h"
+
+#define TYPE_RESOURCE_HANDLER "resource-handler"
+
+#define RESOURCE_HANDLER_CLASS(klass) \
+    OBJECT_CLASS_CHECK(ResourceHandlerClass, (klass), TYPE_RESOURCE_HANDLER)
+#define RESOURCE_HANDLER_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(ResourceHandlerClass, (obj), TYPE_RESOURCE_HANDLER)
+#define RESOURCE_HANDLER(obj) \
+    INTERFACE_CHECK(ResourceHandler, (obj), TYPE_RESOURCE_HANDLER)
+
+typedef struct ResourceHandler {
+    Object Parent;
+} ResourceHandler;
+
+typedef struct ResourceHandlerClass {
+    InterfaceClass parent;
+
+    void (*pre_assign)(ResourceHandler *rh, const DeviceState *dev,
+                       Error **errp);
+    void (*assign)(ResourceHandler *rh, DeviceState *dev, Error **errp);
+    void (*unassign)(ResourceHandler *rh, DeviceState *dev);
+} ResourceHandlerClass;
+
+void resource_handler_pre_assign(ResourceHandler *rh, const DeviceState *dev,
+                                 Error **errp);
+void resource_handler_assign(ResourceHandler *rh, DeviceState *dev,
+                             Error **errp);
+void resource_handler_unassign(ResourceHandler *rh, DeviceState *dev);
+
+#endif /* RESOURCE_HANDLER_H */
-- 
2.14.3

[Qemu-devel] [PATCH v1 3/8] machine: provide default resource handler

[David Hildenbrand]

Most resources that can't be handled via the hotplug handler will have
to assign "system wide" resource. This is e.g. the case for memory
devices. This will allow for a quite clean implementation for such
handling.

The new functions will soon be filled with life.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/machine.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2040177664..a41068410c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -517,8 +517,31 @@ void machine_set_cpu_numa_node(MachineState *machine,
     }
 }
 
+static ResourceHandler *machine_get_resource_handler(MachineState *machine,
+                                                     const DeviceState *dev)
+{
+    return NULL;
+}
+
+static void machine_resource_handler_pre_assign(ResourceHandler *rh,
+                                                const DeviceState *dev,
+                                                Error **errp)
+{
+}
+
+static void machine_resource_handler_assign(ResourceHandler *rh,
+                                            DeviceState *dev, Error **errp)
+{
+}
+
+static void machine_resource_handler_unassign(ResourceHandler *rh,
+                                              DeviceState *dev)
+{
+}
+
 static void machine_class_init(ObjectClass *oc, void *data)
 {
+    ResourceHandlerClass *rhc = RESOURCE_HANDLER_CLASS(oc);
     MachineClass *mc = MACHINE_CLASS(oc);
 
     /* Default 128 MB as guest ram size */
@@ -531,6 +554,12 @@ static void machine_class_init(ObjectClass *oc, void *data)
     mc->numa_mem_align_shift = 23;
     mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
 
+    /* default resource handler */
+    mc->get_resource_handler = machine_get_resource_handler;
+    rhc->pre_assign = machine_resource_handler_pre_assign;
+    rhc->assign = machine_resource_handler_assign;
+    rhc->unassign = machine_resource_handler_unassign;
+
     object_class_property_add_str(oc, "accel",
         machine_get_accel, machine_set_accel, &error_abort);
     object_class_property_set_description(oc, "accel",
@@ -868,6 +897,10 @@ static const TypeInfo machine_info = {
     .instance_size = sizeof(MachineState),
     .instance_init = machine_initfn,
     .instance_finalize = machine_finalize,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_RESOURCE_HANDLER },
+        { }
+    },
 };
 
 static void machine_register_types(void)
-- 
2.14.3

[Qemu-devel] [PATCH v1 4/8] memory-device: new functions to handle resource assignment

[David Hildenbrand]

We will need a handful of new functions:
- set_addr(): To set the calculated address
- get_memory_region(): To add it to the memory region container
- get_addr(): If the device has any specific alignment requirements

Using these and the existing functions, we can properly assign resources
to memory devices.

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

diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 2853b084b5..e43ce1c8d3 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -32,11 +32,17 @@ typedef struct MemoryDeviceState {
 typedef struct MemoryDeviceClass {
     InterfaceClass parent_class;
 
+    /* required functions that have to be implemented */
     uint64_t (*get_addr)(const MemoryDeviceState *md);
+    uint64_t (*set_addr)(MemoryDeviceState *md);
+    MemoryRegion * (*get_memory_region)(MemoryDeviceState *md);
     uint64_t (*get_plugged_size)(const MemoryDeviceState *md);
     uint64_t (*get_region_size)(const MemoryDeviceState *md);
     void (*fill_device_info)(const MemoryDeviceState *md,
                              MemoryDeviceInfo *info);
+
+    /* optional functions that can be implemented */
+    uint64_t (*get_align)(const MemoryDeviceState *md);
 } MemoryDeviceClass;
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);
-- 
2.14.3

[Qemu-devel] [PATCH v1 5/8] pc-dimm: implement new memory device functions

[David Hildenbrand]

Implement the new functions, we don't have to care about alignment for
these DIMMs right now, so leave that function unimplemented.

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

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 12da89d562..7ab9e77747 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -244,6 +244,26 @@ 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)
+{
+    PCDIMMDevice *dimm = PC_DIMM(md);
+
+    dimm->addr = addr;
+}
+
+static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md)
+{
+    PCDIMMDevice *dimm = PC_DIMM(md);
+
+    const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
+    MemoryRegion *mr;
+
+    mr = ddc->get_memory_region(dimm, &error_abort);
+    g_assert(mr);
+
+    return mr;
+}
+
 static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md)
 {
     /* dropping const here is fine as we don't touch the memory region */
@@ -304,6 +324,8 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
     ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
 
     mdc->get_addr = pc_dimm_md_get_addr;
+    mdc->set_addr = pc_dimm_md_set_addr;
+    mdc->get_memory_region = pc_dimm_md_get_memory_region;
     /* 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;
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index e43ce1c8d3..e10f2e854a 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -34,8 +34,8 @@ typedef struct MemoryDeviceClass {
 
     /* required functions that have to be implemented */
     uint64_t (*get_addr)(const MemoryDeviceState *md);
-    uint64_t (*set_addr)(MemoryDeviceState *md);
-    MemoryRegion * (*get_memory_region)(MemoryDeviceState *md);
+    void (*set_addr)(MemoryDeviceState *md, uint64_t addr);
+    MemoryRegion *(*get_memory_region)(MemoryDeviceState *md);
     uint64_t (*get_plugged_size)(const MemoryDeviceState *md);
     uint64_t (*get_region_size)(const MemoryDeviceState *md);
     void (*fill_device_info)(const MemoryDeviceState *md,
-- 
2.14.3

[Qemu-devel] [PATCH v1 6/8] machine: introduce enforce_memory_device_align() and add it for pc

[David Hildenbrand]

For compat handling, we'll have to ask the machine class if a certain
memory device has alignment requirements when assigning resource to
a memory device. This will only be required for PC for now, PPC is fine
(and will be handled via the alignment of the memory region).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c        | 15 +++++++++++++++
 include/hw/boards.h |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ffcd7b85d9..9405e28cfa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2332,6 +2332,20 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+static void pc_machine_enforce_memory_device_align(const MachineClass *mc,
+                                                   const MemoryDeviceState *md,
+                                                   uint64_t *align)
+{
+    const PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
+
+    if (object_dynamic_cast(OBJECT(md), TYPE_PC_DIMM)) {
+        /* compat handling: force to TARGET_PAGE_SIZE */
+        if (!pcmc->enforce_aligned_dimm) {
+            *align = TARGET_PAGE_SIZE;
+        }
+    }
+}
+
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -2371,6 +2385,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = pc_machine_device_unplug_cb;
     nc->nmi_monitor_handler = x86_nmi;
     mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
+    mc->enforce_memory_device_align = pc_machine_enforce_memory_device_align;
 
     object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
         pc_machine_get_device_memory_region_size, NULL,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index ff5142d7c2..db66f2e2d9 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -9,6 +9,7 @@
 #include "qom/object.h"
 #include "qom/cpu.h"
 #include "hw/resource-handler.h"
+#include "hw/mem/memory-device.h"
 
 /**
  * memory_region_allocate_system_memory - Allocate a board's main memory
@@ -163,6 +164,10 @@ typedef struct {
  *    should instead use "unimplemented-device" for all memory ranges where
  *    the guest will attempt to probe for a device that QEMU doesn't
  *    implement and a stub device is required.
+ * @enforce_memory_device_align:
+ *    For some memory devices (e.g. DIMMs), alignment has to be enforced for
+ *    compat handling by the machine. This function will only modify the
+ *    asignment if it needs to be enforced.
  */
 struct MachineClass {
     /*< private >*/
@@ -221,6 +226,9 @@ struct MachineClass {
                                                          unsigned cpu_index);
     const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
     int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
+    void (*enforce_memory_device_align)(const MachineClass *mc,
+                                        const MemoryDeviceState *md,
+                                        uint64_t *align);
 };
 
 /**
-- 
2.14.3

[Qemu-devel] [PATCH v1 7/8] memory-device: factor out pre-assign into default resource handler

[David Hildenbrand]

Let's move all pre-assign checks we can do without the device being
realized into the default resource handler.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/machine.c              | 17 ++++++++++
 hw/mem/memory-device.c         | 72 +++++++++++++++++++-----------------------
 include/hw/mem/memory-device.h |  2 ++
 3 files changed, 52 insertions(+), 39 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a41068410c..e763501c66 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -21,6 +21,7 @@
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
 #include "sysemu/qtest.h"
+#include "hw/mem/memory-device.h"
 
 static char *machine_get_accel(Object *obj, Error **errp)
 {
@@ -520,6 +521,9 @@ void machine_set_cpu_numa_node(MachineState *machine,
 static ResourceHandler *machine_get_resource_handler(MachineState *machine,
                                                      const DeviceState *dev)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        return RESOURCE_HANDLER(machine);
+    }
     return NULL;
 }
 
@@ -527,6 +531,19 @@ static void machine_resource_handler_pre_assign(ResourceHandler *rh,
                                                 const DeviceState *dev,
                                                 Error **errp)
 {
+    MachineState *ms = MACHINE(rh);
+    Error *local_err = NULL;
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        const MemoryDeviceState *md = MEMORY_DEVICE(dev);
+
+        memory_device_pre_assign(ms, md, &local_err);
+        if (local_err) {
+            goto out;
+        }
+    }
+out:
+    error_propagate(errp, local_err);
 }
 
 static void machine_resource_handler_assign(ResourceHandler *rh,
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 3e04f3954e..aa04e0c962 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -68,59 +68,27 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
     return 0;
 }
 
-static void memory_device_check_addable(MachineState *ms, uint64_t size,
-                                        Error **errp)
-{
-    uint64_t used_region_size = 0;
-
-    /* we will need a new memory slot for kvm and vhost */
-    if (kvm_enabled() && !kvm_has_free_slot(ms)) {
-        error_setg(errp, "hypervisor has no free memory slots left");
-        return;
-    }
-    if (!vhost_has_free_slot()) {
-        error_setg(errp, "a used vhost backend has no free memory slots left");
-        return;
-    }
-
-    /* will we exceed the total amount of memory specified */
-    memory_device_used_region_size(OBJECT(ms), &used_region_size);
-    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
-        error_setg(errp, "not enough space, currently 0x%" PRIx64
-                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
-                   used_region_size, ms->maxram_size - ms->ram_size);
-        return;
-    }
-
-}
-
 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;
+    uint64_t used_region_size = 0;
     GSList *list = NULL, *item;
     uint64_t new_addr = 0;
 
-    if (!ms->device_memory) {
-        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
-                         "supported by the machine");
-        return 0;
-    }
-
-    if (!memory_region_size(&ms->device_memory->mr)) {
-        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
-                         "enabled, please specify the maxmem option");
-        return 0;
-    }
     address_space_start = ms->device_memory->base;
     address_space_end = address_space_start +
                         memory_region_size(&ms->device_memory->mr);
     g_assert(QEMU_ALIGN_UP(address_space_start, align) == address_space_start);
     g_assert(address_space_end >= address_space_start);
 
-    memory_device_check_addable(ms, size, errp);
-    if (*errp) {
+    /* will we exceed the total amount of memory specified */
+    memory_device_used_region_size(OBJECT(ms), &used_region_size);
+    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
+        error_setg(errp, "not enough space, currently 0x%" PRIx64
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   used_region_size, ms->maxram_size - ms->ram_size);
         return 0;
     }
 
@@ -243,6 +211,32 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+void memory_device_pre_assign(MachineState *ms,
+                              const MemoryDeviceState *md, Error **errp)
+{
+    if (!ms->device_memory) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "supported by the machine");
+        return;
+    }
+
+    if (!memory_region_size(&ms->device_memory->mr)) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "enabled, please specify the maxmem option");
+        return;
+    }
+
+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(ms)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }
+}
+
 void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
                                uint64_t addr)
 {
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index e10f2e854a..edbd964ac7 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -47,6 +47,8 @@ typedef struct MemoryDeviceClass {
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
+void memory_device_pre_assign(MachineState *ms,
+                              const MemoryDeviceState *md, Error **errp);
 uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
                                      uint64_t align, uint64_t size,
                                      Error **errp);
-- 
2.14.3

[Qemu-devel] [PATCH v1 8/8] memory-device: factor out (un)assign into default resource handler

[David Hildenbrand]

This moves the remaining bits into the default resource handler. The
pc-dimm remainings are limited to slot and migration handling.

Compatibility to old machines on x86 should be guaranteed by using the
newly introduced machine class function for enforcing alignment of
DIMMs.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/machine.c              | 20 +++++++++++++++++
 hw/i386/pc.c                   | 16 +-------------
 hw/mem/memory-device.c         | 50 +++++++++++++++++++++++++++++++++++-------
 hw/mem/pc-dimm.c               | 31 +-------------------------
 hw/mem/trace-events            |  4 +++-
 hw/ppc/spapr.c                 |  5 ++---
 include/hw/mem/memory-device.h |  9 +++-----
 include/hw/mem/pc-dimm.h       |  3 +--
 8 files changed, 73 insertions(+), 65 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index e763501c66..7fc1a6bc5e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -549,11 +549,31 @@ out:
 static void machine_resource_handler_assign(ResourceHandler *rh,
                                             DeviceState *dev, Error **errp)
 {
+    MachineState *ms = MACHINE(rh);
+    Error *local_err = NULL;
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        MemoryDeviceState *md = MEMORY_DEVICE(dev);
+
+        memory_device_assign(ms, md, &local_err);
+        if (local_err) {
+            goto out;
+        }
+    }
+out:
+    error_propagate(errp, local_err);
 }
 
 static void machine_resource_handler_unassign(ResourceHandler *rh,
                                               DeviceState *dev)
 {
+    MachineState *ms = MACHINE(rh);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        MemoryDeviceState *md = MEMORY_DEVICE(dev);
+
+        memory_device_unassign(ms, md);
+    }
 }
 
 static void machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9405e28cfa..385f18bb53 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1681,22 +1681,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-    PCDIMMDevice *dimm = PC_DIMM(dev);
-    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-    MemoryRegion *mr;
-    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);
-    }
-
     /*
      * When -no-acpi is used with Q35 machine type, no ACPI is built,
      * but pcms->acpi_dev is still created. Check !acpi_enabled in
@@ -1714,7 +1700,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
         goto out;
     }
 
-    pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
+    pc_dimm_memory_plug(dev, MACHINE(pcms), &local_err);
     if (local_err) {
         goto out;
     }
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index aa04e0c962..f7f19f6ed2 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -17,6 +17,8 @@
 #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)
 {
@@ -68,9 +70,10 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
     return 0;
 }
 
-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;
     uint64_t used_region_size = 0;
@@ -237,22 +240,53 @@ void memory_device_pre_assign(MachineState *ms,
     }
 }
 
-void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
-                               uint64_t addr)
+void memory_device_assign(MachineState *ms, MemoryDeviceState *md,
+                          Error **errp)
 {
-    /* we expect a previous call to memory_device_get_free_addr() */
+    const MachineClass *mc = MACHINE_GET_CLASS(ms);
+    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+    const uint64_t size = mdc->get_region_size(md);
+    MemoryRegion *mr = mdc->get_memory_region(md);
+    uint64_t addr = mdc->get_addr(md);
+    uint64_t align = memory_region_get_alignment(mr);
+
+    /* we expect a previous call to memory_device_pre_assign */
     g_assert(ms->device_memory);
 
+    /* our device might have stronger alignment requirements */
+    if (mdc->get_align) {
+        align = MAX(align, mdc->get_align(md));
+    }
+
+    /* for compat handling, some alignment has to be enforced for DIMMs */
+    if (mc->enforce_memory_device_align) {
+        /* align unchanged if nothing is to be enforced */
+        mc->enforce_memory_device_align(mc, md, &align);
+    }
+
+    addr = memory_device_get_free_addr(ms, !addr ? NULL : &addr, align,
+                                       size, errp);
+    if (*errp) {
+        return;
+    }
+    trace_memory_device_assign_address(addr);
+    mdc->set_addr(md, addr);
+
     memory_region_add_subregion(&ms->device_memory->mr,
                                 addr - ms->device_memory->base, mr);
 }
 
-void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr)
+void memory_device_unassign(MachineState *ms, MemoryDeviceState *md)
 {
-    /* we expect a previous call to memory_device_get_free_addr() */
+    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+    MemoryRegion *mr = mdc->get_memory_region(md);
+
+    /* we expect a previous call to memory_device_pre_assign */
     g_assert(ms->device_memory);
 
     memory_region_del_subregion(&ms->device_memory->mr, mr);
+    trace_memory_device_unassign_address(mdc->get_addr(md));
+    mdc->set_addr(md, 0);
 }
 
 static const TypeInfo memory_device_info = {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 7ab9e77747..9c5eb1fb16 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -32,39 +32,13 @@ typedef struct pc_dimms_capacity {
      Error    **errp;
 } pc_dimms_capacity;
 
-void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
-                         uint64_t align, Error **errp)
+void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, Error **errp)
 {
     int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
     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) {
-        goto out;
-    }
-
-    addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
-                                       memory_region_size(mr), &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    trace_mhp_pc_dimm_assigned_address(addr);
 
     slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
     if (local_err) {
@@ -82,7 +56,6 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
     }
     trace_mhp_pc_dimm_assigned_slot(slot);
 
-    memory_device_plug_region(machine, mr, addr);
     vmstate_register_ram(vmstate_mr, dev);
 
 out:
@@ -94,9 +67,7 @@ void pc_dimm_memory_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 *mr = ddc->get_memory_region(dimm, &error_abort);
 
-    memory_device_unplug_region(machine, mr);
     vmstate_unregister_ram(vmstate_mr, dev);
 }
 
diff --git a/hw/mem/trace-events b/hw/mem/trace-events
index e150dcc497..930b6aa6ea 100644
--- a/hw/mem/trace-events
+++ b/hw/mem/trace-events
@@ -2,4 +2,6 @@
 
 # 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_assign_address(uint64_t addr) "0x%"PRIx64
+memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 428f6c79c5..818e7232d8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3162,16 +3162,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
-    uint64_t align, size, addr;
+    uint64_t size, addr;
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {
         goto out;
     }
-    align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
-    pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
+    pc_dimm_memory_plug(dev, MACHINE(ms), &local_err);
     if (local_err) {
         goto out;
     }
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index edbd964ac7..6cedbaf747 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -49,11 +49,8 @@ MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
 void memory_device_pre_assign(MachineState *ms,
                               const MemoryDeviceState *md, Error **errp);
-uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
-                                     uint64_t align, uint64_t size,
-                                     Error **errp);
-void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
-                               uint64_t addr);
-void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
+void memory_device_assign(MachineState *ms, MemoryDeviceState *md,
+                          Error **errp);
+void memory_device_unassign(MachineState *ms, MemoryDeviceState *md);
 
 #endif
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 627c8601d9..006c80fb2e 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -78,7 +78,6 @@ 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_plug(DeviceState *dev, MachineState *machine, Error **errp);
 void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
 #endif
-- 
2.14.3

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[Igor Mammedov]

On Thu,  3 May 2018 17:49:28 +0200
David Hildenbrand <david@redhat.com> wrote:

> Hotplug handlers usually have the following tasks:
> 1. Allocate some resources for a new device
> 2. Make the new device visible for the guest
> 3. Notify the guest about the new device
> 
> Hotplug handlers have right now one limitation: They handle their own
> context and only care about resources they manage.
> 
> We can have devices that need certain other resources that are e.g.
> system resources managed by the machine. We need a clean way to assign
> these resources (without violating layers as brought up by Igor).
> 
> One example is virtio-mem/virtio-pmem. Both device types need to be
> assigned some region in guest physical address space. This device memory
> belongs to the machine and is managed by it. However, virito devices are
> hotplugged using the hotplug handler their proxy device implements. So we
> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
> hotplug handler for virtio-ccw. But definetly not the machine.
> 
> So let's generalize the task of "assigning" resources and use it directly
> for memory devices. We now have a clean way to support any kind of memory
> device - independent of the underlying device type. Right now, only one
> resource handler per device can be supported (in addition to the existing
> hotplug handler).
So far I've just skimmed through series and still don't get clear
picture if new interface is really needed.
I'd like very much to see patches for how virtio-[p]mem in CCW and PCI
case would utilize this.


> You can find more details in patch nr 2.
> 
> This work is based on the already queued patch series
>     "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice"
> 
> David Hildenbrand (8):
>   memory-device: always compile support for memory devices for SOFTMMU
>   qdev: introduce ResourceHandler as a first-stage hotplug handler
>   machine: provide default resource handler
>   memory-device: new functions to handle resource assignment
>   pc-dimm: implement new memory device functions
>   machine: introduce enforce_memory_device_align() and add it for pc
>   memory-device: factor out pre-assign into default resource handler
>   memory-device: factor out (un)assign into default resource handler
> 
>  hw/Makefile.objs               |   2 +-
>  hw/core/Makefile.objs          |   1 +
>  hw/core/machine.c              |  70 +++++++++++++++++++++++
>  hw/core/qdev.c                 |  41 +++++++++++++-
>  hw/core/resource-handler.c     |  57 +++++++++++++++++++
>  hw/i386/pc.c                   |  31 ++++++-----
>  hw/mem/Makefile.objs           |   2 +-
>  hw/mem/memory-device.c         | 122 +++++++++++++++++++++++++----------------
>  hw/mem/pc-dimm.c               |  53 ++++++++----------
>  hw/mem/trace-events            |   4 +-
>  hw/ppc/spapr.c                 |   5 +-
>  include/hw/boards.h            |  17 ++++++
>  include/hw/mem/memory-device.h |  17 ++++--
>  include/hw/mem/pc-dimm.h       |   3 +-
>  include/hw/resource-handler.h  |  46 ++++++++++++++++
>  stubs/Makefile.objs            |   1 -
>  stubs/qmp_memory_device.c      |  13 -----
>  17 files changed, 364 insertions(+), 121 deletions(-)
>  create mode 100644 hw/core/resource-handler.c
>  create mode 100644 include/hw/resource-handler.h
>  delete mode 100644 stubs/qmp_memory_device.c
> 

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[David Hildenbrand]

On 04.05.2018 10:49, Igor Mammedov wrote:
> On Thu,  3 May 2018 17:49:28 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Hotplug handlers usually have the following tasks:
>> 1. Allocate some resources for a new device
>> 2. Make the new device visible for the guest
>> 3. Notify the guest about the new device
>>
>> Hotplug handlers have right now one limitation: They handle their own
>> context and only care about resources they manage.
>>
>> We can have devices that need certain other resources that are e.g.
>> system resources managed by the machine. We need a clean way to assign
>> these resources (without violating layers as brought up by Igor).
>>
>> One example is virtio-mem/virtio-pmem. Both device types need to be
>> assigned some region in guest physical address space. This device memory
>> belongs to the machine and is managed by it. However, virito devices are
>> hotplugged using the hotplug handler their proxy device implements. So we
>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
>> hotplug handler for virtio-ccw. But definetly not the machine.
>>
>> So let's generalize the task of "assigning" resources and use it directly
>> for memory devices. We now have a clean way to support any kind of memory
>> device - independent of the underlying device type. Right now, only one
>> resource handler per device can be supported (in addition to the existing
>> hotplug handler).
> So far I've just skimmed through series and still don't get clear
> picture if new interface is really needed.
> I'd like very much to see patches for how virtio-[p]mem in CCW and PCI
> case would utilize this.

Sure, I think you've seen the problematic parts in the latest
virtio-pmem prototype
(https://www.spinics.net/lists/linux-mm/msg151081.html).

There, we do the assignment from the realize function, which you
described as layer violation. I will ask Pankaj to rebase his work on
this series.

Until then, I can point you at the current QEMU side prototype of
virtio-mem. I won't be posting these as patches as there are still many
things to sort out and clean up. The prototype currently works on x86
and s390x.

You can find the latest prototype on github, including patches of this
series at https://github.com/davidhildenbrand/qemu/tree/virtio-mem

Interesting patches are:
- "s390x: inititalize memory region for memory devices"
-- Provide a region for memory devices just like x86
- "virtio-mem: paravirtualized memory"
-- Prototype of virtio-mem.
- "virtio-ccw: add proxy for virtio-mem"
-- CCW proxy device for virtio-mem (using the CSS/CCW hotplug handler)
- "virtio-mem: paravirtualized memory"
-- PCI proxy device for virtio-mem (using PCI hotplug handler)

Note how virtio-mem implements the MemoryDevice interface and how
resource assignments happens without any layer violations (and no
modifications to any hotplug handler).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[Igor Mammedov]

On Fri, 4 May 2018 11:19:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 04.05.2018 10:49, Igor Mammedov wrote:
> > On Thu,  3 May 2018 17:49:28 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Hotplug handlers usually have the following tasks:
> >> 1. Allocate some resources for a new device
> >> 2. Make the new device visible for the guest
> >> 3. Notify the guest about the new device
> >>
> >> Hotplug handlers have right now one limitation: They handle their own
> >> context and only care about resources they manage.
> >>
> >> We can have devices that need certain other resources that are e.g.
> >> system resources managed by the machine. We need a clean way to assign
> >> these resources (without violating layers as brought up by Igor).
> >>
> >> One example is virtio-mem/virtio-pmem. Both device types need to be
> >> assigned some region in guest physical address space. This device memory
> >> belongs to the machine and is managed by it. However, virito devices are
> >> hotplugged using the hotplug handler their proxy device implements. So we
> >> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
> >> hotplug handler for virtio-ccw. But definetly not the machine.
> >>
> >> So let's generalize the task of "assigning" resources and use it directly
> >> for memory devices. We now have a clean way to support any kind of memory
> >> device - independent of the underlying device type. Right now, only one
> >> resource handler per device can be supported (in addition to the existing
> >> hotplug handler).  
> > So far I've just skimmed through series and still don't get clear
> > picture if new interface is really needed.
> > I'd like very much to see patches for how virtio-[p]mem in CCW and PCI
> > case would utilize this.  
> 
> Sure, I think you've seen the problematic parts in the latest
> virtio-pmem prototype
> (https://www.spinics.net/lists/linux-mm/msg151081.html).
> 
> There, we do the assignment from the realize function, which you
> described as layer violation. I will ask Pankaj to rebase his work on
> this series.
That's too premature at the moment.

> Until then, I can point you at the current QEMU side prototype of
> virtio-mem. I won't be posting these as patches as there are still many
> things to sort out and clean up. The prototype currently works on x86
> and s390x.
> 
> You can find the latest prototype on github, including patches of this
> series at https://github.com/davidhildenbrand/qemu/tree/virtio-mem
> 
> Interesting patches are:
> - "s390x: inititalize memory region for memory devices"
> -- Provide a region for memory devices just like x86
> - "virtio-mem: paravirtualized memory"
> -- Prototype of virtio-mem.
> - "virtio-ccw: add proxy for virtio-mem"
> -- CCW proxy device for virtio-mem (using the CSS/CCW hotplug handler)
> - "virtio-mem: paravirtualized memory"
> -- PCI proxy device for virtio-mem (using PCI hotplug handler)
> 
> Note how virtio-mem implements the MemoryDevice interface and how
> resource assignments happens without any layer violations (and no
> modifications to any hotplug handler).
Thanks, that should be sufficient to get idea.
I'll look into it and come back here with concrete comments or
suggesting alternative.

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[David Hildenbrand]

>> Note how virtio-mem implements the MemoryDevice interface and how
>> resource assignments happens without any layer violations (and no
>> modifications to any hotplug handler).
> Thanks, that should be sufficient to get idea.
> I'll look into it and come back here with concrete comments or
> suggesting alternative.
> 

Thanks, looking forward to your reply. If there are any questions in the
meantime, please let me know!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/8] qdev: introduce ResourceHandler as a first-stage hotplug handler

[David Hildenbrand]

On 03.05.2018 17:49, David Hildenbrand wrote:
> Hotplug handlers usually do 3 things:
> 1. Allocate some resources for a new device
> 2. Make the new device visible for the guest
> 3. Notify the guest about the new device
> 
> While 2. and 3. are only ever performed once for a device, there
> might be various resources to allocate. E.g. a MemoryDevice could be
> exposed via Virtio. The proxy device hotplug handler (e.g. virtio-pci,
> virtio-ccw) takes care of exposing the device to the guest. However,
> we also have to allocate system resources, like a portion in guest
> physical address space, which is to be handled by the machine.
> 
> One key difference between a hotplug handler and a resource handler is
> that resource handlers have no "unplug_request". There is no
> communication with the guest (this is done by a "hotplug" handler). So
> conceptually, they are different.
> 
> For now we live with the assumption, that - in additon to the existing
> hotplug handler which can do some resource asignment - at most one
> resource handler is necessary.
> 
> We might even later decide to have multiple resource handlers for a
> specific device (e.g. one for each interface they implement). For now,
> this design is sufficient.
> 
> Resource handlers have to run before the hotplug handler runs (esp
> before exposing the device to the guest).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/core/Makefile.objs         |  1 +
>  hw/core/qdev.c                | 41 ++++++++++++++++++++++++++++++-
>  hw/core/resource-handler.c    | 57 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h           |  9 +++++++
>  include/hw/resource-handler.h | 46 ++++++++++++++++++++++++++++++++++
>  5 files changed, 153 insertions(+), 1 deletion(-)
>  create mode 100644 hw/core/resource-handler.c
>  create mode 100644 include/hw/resource-handler.h
> 
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index eb88ca979e..7474387fcd 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
>  common-obj-y += hotplug.o
> +common-obj-y += resource-handler.o
>  common-obj-$(CONFIG_SOFTMMU) += nmi.o
>  
>  common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f6f92473b8..042e275884 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -35,6 +35,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
>  #include "hw/hotplug.h"
> +#include "hw/resource-handler.h"
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
>  
> @@ -271,6 +272,17 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>      return hotplug_ctrl;
>  }
>  
> +static ResourceHandler *qdev_get_resource_handler(DeviceState *dev)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    if (mc->get_resource_handler) {
> +            return mc->get_resource_handler(ms, dev);
> +    }
> +    return NULL;
> +}
> +
>  static int qdev_reset_one(DeviceState *dev, void *opaque)
>  {
>      device_reset(dev);
> @@ -814,6 +826,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    ResourceHandler *resource_ctrl;
>      HotplugHandler *hotplug_ctrl;
>      BusState *bus;
>      Error *local_err = NULL;
> @@ -825,6 +838,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          return;
>      }
>  
> +    resource_ctrl = qdev_get_resource_handler(dev);
> +
>      if (value && !dev->realized) {
>          if (!check_only_migratable(obj, &local_err)) {
>              goto fail;
> @@ -840,6 +855,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              g_free(name);
>          }
>  
> +        if (resource_ctrl) {
> +            resource_handler_pre_assign(resource_ctrl, dev, &local_err);
> +            if (local_err != NULL) {
> +                goto fail;
> +            }
> +        }
> +
>          hotplug_ctrl = qdev_get_hotplug_handler(dev);
>          if (hotplug_ctrl) {
>              hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
> @@ -858,12 +880,19 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>  
>          DEVICE_LISTENER_CALL(realize, Forward, dev);
>  
> +        if (resource_ctrl) {
> +            resource_handler_assign(resource_ctrl, dev, &local_err);
> +            if (local_err != NULL) {
> +                goto post_realize_fail;
> +            }
> +        }
> +
>          if (hotplug_ctrl) {
>              hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
>          }
>  
>          if (local_err != NULL) {
> -            goto post_realize_fail;
> +            goto post_assign_fail;
>          }
>  
>          /*
> @@ -903,6 +932,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }
> +
> +        if (resource_ctrl) {
> +            resource_handler_unassign(resource_ctrl, dev);
> +        }
> +
>          if (dc->unrealize) {
>              local_errp = local_err ? NULL : &local_err;
>              dc->unrealize(dev, local_errp);
> @@ -928,6 +962,11 @@ child_realize_fail:
>          vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>      }
>  
> +post_assign_fail:
> +    if (resource_ctrl) {
> +        resource_handler_unassign(resource_ctrl, dev);
> +    }
> +
>  post_realize_fail:
>      g_free(dev->canonical_path);
>      dev->canonical_path = NULL;
> diff --git a/hw/core/resource-handler.c b/hw/core/resource-handler.c
> new file mode 100644
> index 0000000000..0a1ff0e66a
> --- /dev/null
> +++ b/hw/core/resource-handler.c
> @@ -0,0 +1,57 @@
> +/*
> + * Resource handler interface.
> + *
> + * Copyright (c) 2018 Red Hat Inc.
> + *
> + * Authors:
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/resource-handler.h"
> +#include "qemu/module.h"
> +
> +void resource_handler_pre_assign(ResourceHandler *rh,
> +                                 const DeviceState *dev, Error **errp)
> +{
> +    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
> +
> +    if (rhc->pre_assign) {
> +        rhc->pre_assign(rh, dev, errp);
> +    }
> +}
> +
> +void resource_handler_assign(ResourceHandler *rh, DeviceState *dev,
> +                             Error **errp)
> +{
> +    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
> +
> +    if (rhc->assign) {
> +        rhc->assign(rh, dev, errp);
> +    }
> +}
> +
> +void resource_handler_unassign(ResourceHandler *rh, DeviceState *dev)
> +{
> +    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
> +
> +    if (rhc->unassign) {
> +        rhc->unassign(rh, dev);
> +    }
> +}
> +
> +static const TypeInfo resource_handler_info = {
> +    .name          = TYPE_RESOURCE_HANDLER,
> +    .parent        = TYPE_INTERFACE,
> +    .class_size = sizeof(ResourceHandlerClass),
> +};
> +
> +static void resource_handler_register_types(void)
> +{
> +    type_register_static(&resource_handler_info);
> +}
> +
> +type_init(resource_handler_register_types)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 8748964e6f..ff5142d7c2 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -8,6 +8,7 @@
>  #include "hw/qdev.h"
>  #include "qom/object.h"
>  #include "qom/cpu.h"
> +#include "hw/resource-handler.h"
>  
>  /**
>   * memory_region_allocate_system_memory - Allocate a board's main memory
> @@ -115,6 +116,12 @@ typedef struct {
>   *    of HotplugHandler object, which handles hotplug operation
>   *    for a given @dev. It may return NULL if @dev doesn't require
>   *    any actions to be performed by hotplug handler.
> + * @get_resource_handler: this function is called when a new device is
> + *                        about to be hotplugged. If defined, it returns pointer
> + *                        to an instance of ResourceHandler object, which
> + *                        handles resource asignment for a given @dev. It
> + *                        may return NULL if @dev doesn't require any actions
> + *                        to be performed by a resource handler.
>   * @cpu_index_to_instance_props:
>   *    used to provide @cpu_index to socket/core/thread number mapping, allowing
>   *    legacy code to perform maping from cpu_index to topology properties
> @@ -208,6 +215,8 @@ struct MachineClass {
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> +    ResourceHandler *(*get_resource_handler)(MachineState *machine,
> +                                             const DeviceState *dev);
>      CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState *machine,
>                                                           unsigned cpu_index);
>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> diff --git a/include/hw/resource-handler.h b/include/hw/resource-handler.h
> new file mode 100644
> index 0000000000..3eda1bec5c
> --- /dev/null
> +++ b/include/hw/resource-handler.h
> @@ -0,0 +1,46 @@
> +/*
> + * Resource handler interface.
> + *
> + * Copyright (c) 2018 Red Hat Inc.
> + *
> + * Authors:
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef RESOURCE_HANDLER_H
> +#define RESOURCE_HANDLER_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_RESOURCE_HANDLER "resource-handler"
> +
> +#define RESOURCE_HANDLER_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(ResourceHandlerClass, (klass), TYPE_RESOURCE_HANDLER)
> +#define RESOURCE_HANDLER_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ResourceHandlerClass, (obj), TYPE_RESOURCE_HANDLER)
> +#define RESOURCE_HANDLER(obj) \
> +    INTERFACE_CHECK(ResourceHandler, (obj), TYPE_RESOURCE_HANDLER)
> +
> +typedef struct ResourceHandler {
> +    Object Parent;
> +} ResourceHandler;
> +
> +typedef struct ResourceHandlerClass {
> +    InterfaceClass parent;
> +
> +    void (*pre_assign)(ResourceHandler *rh, const DeviceState *dev,
> +                       Error **errp);
> +    void (*assign)(ResourceHandler *rh, DeviceState *dev, Error **errp);
> +    void (*unassign)(ResourceHandler *rh, DeviceState *dev);
> +} ResourceHandlerClass;
> +
> +void resource_handler_pre_assign(ResourceHandler *rh, const DeviceState *dev,
> +                                 Error **errp);
> +void resource_handler_assign(ResourceHandler *rh, DeviceState *dev,
> +                             Error **errp);
> +void resource_handler_unassign(ResourceHandler *rh, DeviceState *dev);
> +
> +#endif /* RESOURCE_HANDLER_H */
> 

I did the following changes to make it pass "make check".

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 042e275884..efa59b9d1e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -274,12 +274,18 @@ HotplugHandler
*qdev_get_hotplug_handler(DeviceState *dev)

 static ResourceHandler *qdev_get_resource_handler(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
-    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    MachineState *ms;
+    MachineClass *mc;
+    Object *m_obj = qdev_get_machine();

-    if (mc->get_resource_handler) {
-            return mc->get_resource_handler(ms, dev);
+    if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
+        ms = MACHINE(m_obj);
+        mc = MACHINE_GET_CLASS(ms);
+        if (mc->get_resource_handler) {
+                return mc->get_resource_handler(ms, dev);
+        }
     }
+
     return NULL;
 }

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2..45c64bee64 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -645,6 +645,7 @@ tests/atomic_add-bench$(EXESUF):
tests/atomic_add-bench.o $(test-util-obj-y)

 tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
        hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
+       hw/core/resource-handler.o \
        hw/core/bus.o \
        hw/core/irq.o \
        hw/core/fw-path-provider.o \


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[David Hildenbrand]

On 03.05.2018 17:49, David Hildenbrand wrote:
> Hotplug handlers usually have the following tasks:
> 1. Allocate some resources for a new device
> 2. Make the new device visible for the guest
> 3. Notify the guest about the new device
> 
> Hotplug handlers have right now one limitation: They handle their own
> context and only care about resources they manage.
> 
> We can have devices that need certain other resources that are e.g.
> system resources managed by the machine. We need a clean way to assign
> these resources (without violating layers as brought up by Igor).
> 
> One example is virtio-mem/virtio-pmem. Both device types need to be
> assigned some region in guest physical address space. This device memory
> belongs to the machine and is managed by it. However, virito devices are
> hotplugged using the hotplug handler their proxy device implements. So we
> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
> hotplug handler for virtio-ccw. But definetly not the machine.
> 
> So let's generalize the task of "assigning" resources and use it directly
> for memory devices. We now have a clean way to support any kind of memory
> device - independent of the underlying device type. Right now, only one
> resource handler per device can be supported (in addition to the existing
> hotplug handler).
> 
> You can find more details in patch nr 2.
> 
> This work is based on the already queued patch series
>     "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice"
> 
> David Hildenbrand (8):
>   memory-device: always compile support for memory devices for SOFTMMU
>   qdev: introduce ResourceHandler as a first-stage hotplug handler
>   machine: provide default resource handler
>   memory-device: new functions to handle resource assignment
>   pc-dimm: implement new memory device functions
>   machine: introduce enforce_memory_device_align() and add it for pc
>   memory-device: factor out pre-assign into default resource handler
>   memory-device: factor out (un)assign into default resource handler
> 
>  hw/Makefile.objs               |   2 +-
>  hw/core/Makefile.objs          |   1 +
>  hw/core/machine.c              |  70 +++++++++++++++++++++++
>  hw/core/qdev.c                 |  41 +++++++++++++-
>  hw/core/resource-handler.c     |  57 +++++++++++++++++++
>  hw/i386/pc.c                   |  31 ++++++-----
>  hw/mem/Makefile.objs           |   2 +-
>  hw/mem/memory-device.c         | 122 +++++++++++++++++++++++++----------------
>  hw/mem/pc-dimm.c               |  53 ++++++++----------
>  hw/mem/trace-events            |   4 +-
>  hw/ppc/spapr.c                 |   5 +-
>  include/hw/boards.h            |  17 ++++++
>  include/hw/mem/memory-device.h |  17 ++++--
>  include/hw/mem/pc-dimm.h       |   3 +-
>  include/hw/resource-handler.h  |  46 ++++++++++++++++
>  stubs/Makefile.objs            |   1 -
>  stubs/qmp_memory_device.c      |  13 -----
>  17 files changed, 364 insertions(+), 121 deletions(-)
>  create mode 100644 hw/core/resource-handler.c
>  create mode 100644 include/hw/resource-handler.h
>  delete mode 100644 stubs/qmp_memory_device.c
> 

If there are no further comments, I'll send a v2 by the end of this
week. Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[Igor Mammedov]

On Wed, 9 May 2018 16:13:14 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 03.05.2018 17:49, David Hildenbrand wrote:
> > Hotplug handlers usually have the following tasks:
> > 1. Allocate some resources for a new device
> > 2. Make the new device visible for the guest
> > 3. Notify the guest about the new device
> > 
> > Hotplug handlers have right now one limitation: They handle their own
> > context and only care about resources they manage.
> > 
> > We can have devices that need certain other resources that are e.g.
> > system resources managed by the machine. We need a clean way to assign
> > these resources (without violating layers as brought up by Igor).
> > 
> > One example is virtio-mem/virtio-pmem. Both device types need to be
> > assigned some region in guest physical address space. This device memory
> > belongs to the machine and is managed by it. However, virito devices are
> > hotplugged using the hotplug handler their proxy device implements. So we
> > could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
> > hotplug handler for virtio-ccw. But definetly not the machine.
> > 
> > So let's generalize the task of "assigning" resources and use it directly
> > for memory devices. We now have a clean way to support any kind of memory
> > device - independent of the underlying device type. Right now, only one
> > resource handler per device can be supported (in addition to the existing
> > hotplug handler).
> > 
> > You can find more details in patch nr 2.
> > 
> > This work is based on the already queued patch series
> >     "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice"
> > 
> > David Hildenbrand (8):
> >   memory-device: always compile support for memory devices for SOFTMMU
> >   qdev: introduce ResourceHandler as a first-stage hotplug handler
> >   machine: provide default resource handler
> >   memory-device: new functions to handle resource assignment
> >   pc-dimm: implement new memory device functions
> >   machine: introduce enforce_memory_device_align() and add it for pc
> >   memory-device: factor out pre-assign into default resource handler
> >   memory-device: factor out (un)assign into default resource handler
> > 
> >  hw/Makefile.objs               |   2 +-
> >  hw/core/Makefile.objs          |   1 +
> >  hw/core/machine.c              |  70 +++++++++++++++++++++++
> >  hw/core/qdev.c                 |  41 +++++++++++++-
> >  hw/core/resource-handler.c     |  57 +++++++++++++++++++
> >  hw/i386/pc.c                   |  31 ++++++-----
> >  hw/mem/Makefile.objs           |   2 +-
> >  hw/mem/memory-device.c         | 122 +++++++++++++++++++++++++----------------
> >  hw/mem/pc-dimm.c               |  53 ++++++++----------
> >  hw/mem/trace-events            |   4 +-
> >  hw/ppc/spapr.c                 |   5 +-
> >  include/hw/boards.h            |  17 ++++++
> >  include/hw/mem/memory-device.h |  17 ++++--
> >  include/hw/mem/pc-dimm.h       |   3 +-
> >  include/hw/resource-handler.h  |  46 ++++++++++++++++
> >  stubs/Makefile.objs            |   1 -
> >  stubs/qmp_memory_device.c      |  13 -----
> >  17 files changed, 364 insertions(+), 121 deletions(-)
> >  create mode 100644 hw/core/resource-handler.c
> >  create mode 100644 include/hw/resource-handler.h
> >  delete mode 100644 stubs/qmp_memory_device.c
> >   
> 
> If there are no further comments, I'll send a v2 by the end of this
> week. Thanks!
I couldn't convince myself that ResourceHandler is really necessary.
My main gripe with it, is that it imposes specific ordering wrt hotplug
handler that resources will be touched. Other issue is that it looks
a bit over-engineered with a lot of code fragmentation. Hence,

I'd suggest use simple hotplug handler chaining instead,
which should take care of wiring up virtio-mem/virtio-pmem,
keeping code compact at the same time.

Could you try something along these lines (I'll post a patch to
override default hotplug handler as reply here for you to pick up,
that should make following work):

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 100dfdc..c400c0c 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -393,12 +393,30 @@ static void s390_machine_reset(void)
     s390_ipl_prepare_cpu(ipl_cpu);
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
 }
+static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
+        /* do checks && set default values if it weren't set by user */
+        /* possibly pass to device's bus pre_plug handler if need */
+    }
+}
 
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         s390_cpu_plug(hotplug_dev, dev, errp);
+    } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
+        HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler;
+
+        /* do machine specific wiring, i.e.
+         * assign resources specified by user or by foo_pre_plug(),
+         * like map region into machine address space at specified address */
+        if (OK) {
+            /* pass control down to bus specific hotplug chain */
+            hotplug_handler_plug(hotplug_ctrl, dev, errp);
+        }
     }
 }
 
@@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         return HOTPLUG_HANDLER(machine);
+    } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) {
+        return HOTPLUG_HANDLER(machine);
     }
     return NULL;
 }

[Qemu-devel] [PATCH] qdev: let machine hotplug handler to override bus hotplug handler

[Igor Mammedov]

it will allow to return another hotplug handler than the default
one for a specific bus based device type. Which is needed to handle
non trivial plug/unplug sequences that need the access to resources
configured outside of bus where device is attached.

That will allow for returned hotplug handler to orchestrate wiring
in arbitrary order, by chaining other hotplug handlers when
it's needed.

PS:
It could be used for hybrid virtio-mem and virtio-pmem devices
where it will return machine as hotplug handler which will do
necessary wiring at machine level and then pass control down
the chain to bus specific hotplug handler.

Example of top level hotplug handler override and custom plug sequence:

  some_machine_get_hotplug_handler(machine){
      if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
          return HOTPLUG_HANDLER(machine);
      }
      return NULL;
  }

  some_machine_device_plug(hotplug_dev, dev) {
      if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
          /* do machine specific initialization */
          some_machine_init_special_device(dev)

          /* pass control to bus specific handler */
          hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev)
      }
  }

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/qdev-core.h | 11 +++++++++++
 hw/core/qdev.c         |  6 ++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 9453588..e6a8eca 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -286,6 +286,17 @@ void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
+/**
+ * qdev_get_hotplug_handler: Get handler responsible for device wiring
+ *
+ * Find HOTPLUG_HANDLER for @dev that provides [pre|un]plug callbacks for it.
+ *
+ * Note: in case @dev has a parent bus, it will be returned as handler unless
+ * machine handler overrides it.
+ *
+ * Returns: pointer to object that implements TYPE_HOTPLUG_HANDLER interface
+ *          or NULL if there aren't any.
+ */
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
 void qdev_unplug(DeviceState *dev, Error **errp);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f6f9247..885286f 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -261,12 +261,10 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
 
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 {
-    HotplugHandler *hotplug_ctrl;
+    HotplugHandler *hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
 
-    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+    if (hotplug_ctrl == NULL && dev->parent_bus) {
         hotplug_ctrl = dev->parent_bus->hotplug_handler;
-    } else {
-        hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
     }
     return hotplug_ctrl;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[David Hildenbrand]

On 10.05.2018 15:02, Igor Mammedov wrote:
> On Wed, 9 May 2018 16:13:14 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 03.05.2018 17:49, David Hildenbrand wrote:
>>> Hotplug handlers usually have the following tasks:
>>> 1. Allocate some resources for a new device
>>> 2. Make the new device visible for the guest
>>> 3. Notify the guest about the new device
>>>
>>> Hotplug handlers have right now one limitation: They handle their own
>>> context and only care about resources they manage.
>>>
>>> We can have devices that need certain other resources that are e.g.
>>> system resources managed by the machine. We need a clean way to assign
>>> these resources (without violating layers as brought up by Igor).
>>>
>>> One example is virtio-mem/virtio-pmem. Both device types need to be
>>> assigned some region in guest physical address space. This device memory
>>> belongs to the machine and is managed by it. However, virito devices are
>>> hotplugged using the hotplug handler their proxy device implements. So we
>>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
>>> hotplug handler for virtio-ccw. But definetly not the machine.
>>>
>>> So let's generalize the task of "assigning" resources and use it directly
>>> for memory devices. We now have a clean way to support any kind of memory
>>> device - independent of the underlying device type. Right now, only one
>>> resource handler per device can be supported (in addition to the existing
>>> hotplug handler).
>>>
>>> You can find more details in patch nr 2.
>>>
>>> This work is based on the already queued patch series
>>>     "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice"
>>>
>>> David Hildenbrand (8):
>>>   memory-device: always compile support for memory devices for SOFTMMU
>>>   qdev: introduce ResourceHandler as a first-stage hotplug handler
>>>   machine: provide default resource handler
>>>   memory-device: new functions to handle resource assignment
>>>   pc-dimm: implement new memory device functions
>>>   machine: introduce enforce_memory_device_align() and add it for pc
>>>   memory-device: factor out pre-assign into default resource handler
>>>   memory-device: factor out (un)assign into default resource handler
>>>
>>>  hw/Makefile.objs               |   2 +-
>>>  hw/core/Makefile.objs          |   1 +
>>>  hw/core/machine.c              |  70 +++++++++++++++++++++++
>>>  hw/core/qdev.c                 |  41 +++++++++++++-
>>>  hw/core/resource-handler.c     |  57 +++++++++++++++++++
>>>  hw/i386/pc.c                   |  31 ++++++-----
>>>  hw/mem/Makefile.objs           |   2 +-
>>>  hw/mem/memory-device.c         | 122 +++++++++++++++++++++++++----------------
>>>  hw/mem/pc-dimm.c               |  53 ++++++++----------
>>>  hw/mem/trace-events            |   4 +-
>>>  hw/ppc/spapr.c                 |   5 +-
>>>  include/hw/boards.h            |  17 ++++++
>>>  include/hw/mem/memory-device.h |  17 ++++--
>>>  include/hw/mem/pc-dimm.h       |   3 +-
>>>  include/hw/resource-handler.h  |  46 ++++++++++++++++
>>>  stubs/Makefile.objs            |   1 -
>>>  stubs/qmp_memory_device.c      |  13 -----
>>>  17 files changed, 364 insertions(+), 121 deletions(-)
>>>  create mode 100644 hw/core/resource-handler.c
>>>  create mode 100644 include/hw/resource-handler.h
>>>  delete mode 100644 stubs/qmp_memory_device.c
>>>   
>>
>> If there are no further comments, I'll send a v2 by the end of this
>> week. Thanks!
> I couldn't convince myself that ResourceHandler is really necessary.
> My main gripe with it, is that it imposes specific ordering wrt hotplug
> handler that resources will be touched. Other issue is that it looks
> a bit over-engineered with a lot of code fragmentation. Hence,
> 
> I'd suggest use simple hotplug handler chaining instead,
> which should take care of wiring up virtio-mem/virtio-pmem,
> keeping code compact at the same time.

I'll have a look tomorrow or next friday if that could work - not sure
yet about unplug vs. unplug requests. Unplug requests might be tricky.
Would be nice if it would work. Thanks!

> 
> Could you try something along these lines (I'll post a patch to
> override default hotplug handler as reply here for you to pick up,
> that should make following work):
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 100dfdc..c400c0c 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -393,12 +393,30 @@ static void s390_machine_reset(void)
>      s390_ipl_prepare_cpu(ipl_cpu);
>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
>  }
> +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> +                                         DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
> +        /* do checks && set default values if it weren't set by user */
> +        /* possibly pass to device's bus pre_plug handler if need */
> +    }
> +}
>  
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>                                       DeviceState *dev, Error **errp)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          s390_cpu_plug(hotplug_dev, dev, errp);
> +    } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
> +        HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler;
> +
> +        /* do machine specific wiring, i.e.
> +         * assign resources specified by user or by foo_pre_plug(),
> +         * like map region into machine address space at specified address */
> +        if (OK) {
> +            /* pass control down to bus specific hotplug chain */
> +            hotplug_handler_plug(hotplug_ctrl, dev, errp);
> +        }
>      }
>  }
>  
> @@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          return HOTPLUG_HANDLER(machine);
> +    } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) {
> +        return HOTPLUG_HANDLER(machine);
>      }
>      return NULL;
>  }
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[Igor Mammedov]

On Thu, 10 May 2018 15:20:55 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 10.05.2018 15:02, Igor Mammedov wrote:
> > On Wed, 9 May 2018 16:13:14 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 03.05.2018 17:49, David Hildenbrand wrote:  
> >>> Hotplug handlers usually have the following tasks:
> >>> 1. Allocate some resources for a new device
> >>> 2. Make the new device visible for the guest
> >>> 3. Notify the guest about the new device
> >>>
> >>> Hotplug handlers have right now one limitation: They handle their own
> >>> context and only care about resources they manage.
> >>>
> >>> We can have devices that need certain other resources that are e.g.
> >>> system resources managed by the machine. We need a clean way to assign
> >>> these resources (without violating layers as brought up by Igor).
> >>>
> >>> One example is virtio-mem/virtio-pmem. Both device types need to be
> >>> assigned some region in guest physical address space. This device memory
> >>> belongs to the machine and is managed by it. However, virito devices are
> >>> hotplugged using the hotplug handler their proxy device implements. So we
> >>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
> >>> hotplug handler for virtio-ccw. But definetly not the machine.
> >>>
> >>> So let's generalize the task of "assigning" resources and use it directly
> >>> for memory devices. We now have a clean way to support any kind of memory
> >>> device - independent of the underlying device type. Right now, only one
> >>> resource handler per device can be supported (in addition to the existing
> >>> hotplug handler).
> >>>
> >>> You can find more details in patch nr 2.
> >>>
> >>> This work is based on the already queued patch series
> >>>     "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice"
> >>>
> >>> David Hildenbrand (8):
> >>>   memory-device: always compile support for memory devices for SOFTMMU
> >>>   qdev: introduce ResourceHandler as a first-stage hotplug handler
> >>>   machine: provide default resource handler
> >>>   memory-device: new functions to handle resource assignment
> >>>   pc-dimm: implement new memory device functions
> >>>   machine: introduce enforce_memory_device_align() and add it for pc
> >>>   memory-device: factor out pre-assign into default resource handler
> >>>   memory-device: factor out (un)assign into default resource handler
> >>>
> >>>  hw/Makefile.objs               |   2 +-
> >>>  hw/core/Makefile.objs          |   1 +
> >>>  hw/core/machine.c              |  70 +++++++++++++++++++++++
> >>>  hw/core/qdev.c                 |  41 +++++++++++++-
> >>>  hw/core/resource-handler.c     |  57 +++++++++++++++++++
> >>>  hw/i386/pc.c                   |  31 ++++++-----
> >>>  hw/mem/Makefile.objs           |   2 +-
> >>>  hw/mem/memory-device.c         | 122 +++++++++++++++++++++++++----------------
> >>>  hw/mem/pc-dimm.c               |  53 ++++++++----------
> >>>  hw/mem/trace-events            |   4 +-
> >>>  hw/ppc/spapr.c                 |   5 +-
> >>>  include/hw/boards.h            |  17 ++++++
> >>>  include/hw/mem/memory-device.h |  17 ++++--
> >>>  include/hw/mem/pc-dimm.h       |   3 +-
> >>>  include/hw/resource-handler.h  |  46 ++++++++++++++++
> >>>  stubs/Makefile.objs            |   1 -
> >>>  stubs/qmp_memory_device.c      |  13 -----
> >>>  17 files changed, 364 insertions(+), 121 deletions(-)
> >>>  create mode 100644 hw/core/resource-handler.c
> >>>  create mode 100644 include/hw/resource-handler.h
> >>>  delete mode 100644 stubs/qmp_memory_device.c
> >>>     
> >>
> >> If there are no further comments, I'll send a v2 by the end of this
> >> week. Thanks!  
> > I couldn't convince myself that ResourceHandler is really necessary.
> > My main gripe with it, is that it imposes specific ordering wrt hotplug
> > handler that resources will be touched. Other issue is that it looks
> > a bit over-engineered with a lot of code fragmentation. Hence,
> > 
> > I'd suggest use simple hotplug handler chaining instead,
> > which should take care of wiring up virtio-mem/virtio-pmem,
> > keeping code compact at the same time.  
> 
> I'll have a look tomorrow or next friday if that could work - not sure
> yet about unplug vs. unplug requests. Unplug requests might be tricky.
> Would be nice if it would work. Thanks!
If you have issues with it, ping me, Maybe we'd figure out how to make it
work together.

> 
> > 
> > Could you try something along these lines (I'll post a patch to
> > override default hotplug handler as reply here for you to pick up,
> > that should make following work):
> > 
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 100dfdc..c400c0c 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -393,12 +393,30 @@ static void s390_machine_reset(void)
> >      s390_ipl_prepare_cpu(ipl_cpu);
> >      s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
> >  }
> > +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > +                                         DeviceState *dev, Error **errp)
> > +{
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
> > +        /* do checks && set default values if it weren't set by user */
> > +        /* possibly pass to device's bus pre_plug handler if need */
> > +    }
> > +}
> >  
> >  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> >                                       DeviceState *dev, Error **errp)
> >  {
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >          s390_cpu_plug(hotplug_dev, dev, errp);
> > +    } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
> > +        HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler;
> > +
> > +        /* do machine specific wiring, i.e.
> > +         * assign resources specified by user or by foo_pre_plug(),
> > +         * like map region into machine address space at specified address */
> > +        if (OK) {
> > +            /* pass control down to bus specific hotplug chain */
> > +            hotplug_handler_plug(hotplug_ctrl, dev, errp);
> > +        }
> >      }
> >  }
> >  
> > @@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
> >  {
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >          return HOTPLUG_HANDLER(machine);
> > +    } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) {
> > +        return HOTPLUG_HANDLER(machine);
> >      }
> >      return NULL;
> >  }
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 0/8] MemoryDevice: introduce and use ResourceHandler

[David Hildenbrand]

On 10.05.2018 15:32, Igor Mammedov wrote:
> On Thu, 10 May 2018 15:20:55 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 10.05.2018 15:02, Igor Mammedov wrote:
>>> On Wed, 9 May 2018 16:13:14 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 03.05.2018 17:49, David Hildenbrand wrote:  
>>>>> Hotplug handlers usually have the following tasks:
>>>>> 1. Allocate some resources for a new device
>>>>> 2. Make the new device visible for the guest
>>>>> 3. Notify the guest about the new device
>>>>>
>>>>> Hotplug handlers have right now one limitation: They handle their own
>>>>> context and only care about resources they manage.
>>>>>
>>>>> We can have devices that need certain other resources that are e.g.
>>>>> system resources managed by the machine. We need a clean way to assign
>>>>> these resources (without violating layers as brought up by Igor).
>>>>>
>>>>> One example is virtio-mem/virtio-pmem. Both device types need to be
>>>>> assigned some region in guest physical address space. This device memory
>>>>> belongs to the machine and is managed by it. However, virito devices are
>>>>> hotplugged using the hotplug handler their proxy device implements. So we
>>>>> could trigger e.g. a PCI hotplug handler for virtio-pci or a CSS/CCW
>>>>> hotplug handler for virtio-ccw. But definetly not the machine.
>>>>>
>>>>> So let's generalize the task of "assigning" resources and use it directly
>>>>> for memory devices. We now have a clean way to support any kind of memory
>>>>> device - independent of the underlying device type. Right now, only one
>>>>> resource handler per device can be supported (in addition to the existing
>>>>> hotplug handler).
>>>>>
>>>>> You can find more details in patch nr 2.
>>>>>
>>>>> This work is based on the already queued patch series
>>>>>     "[PATCH v4 00/11] pc-dimm: factor out MemoryDevice"
>>>>>
>>>>> David Hildenbrand (8):
>>>>>   memory-device: always compile support for memory devices for SOFTMMU
>>>>>   qdev: introduce ResourceHandler as a first-stage hotplug handler
>>>>>   machine: provide default resource handler
>>>>>   memory-device: new functions to handle resource assignment
>>>>>   pc-dimm: implement new memory device functions
>>>>>   machine: introduce enforce_memory_device_align() and add it for pc
>>>>>   memory-device: factor out pre-assign into default resource handler
>>>>>   memory-device: factor out (un)assign into default resource handler
>>>>>
>>>>>  hw/Makefile.objs               |   2 +-
>>>>>  hw/core/Makefile.objs          |   1 +
>>>>>  hw/core/machine.c              |  70 +++++++++++++++++++++++
>>>>>  hw/core/qdev.c                 |  41 +++++++++++++-
>>>>>  hw/core/resource-handler.c     |  57 +++++++++++++++++++
>>>>>  hw/i386/pc.c                   |  31 ++++++-----
>>>>>  hw/mem/Makefile.objs           |   2 +-
>>>>>  hw/mem/memory-device.c         | 122 +++++++++++++++++++++++++----------------
>>>>>  hw/mem/pc-dimm.c               |  53 ++++++++----------
>>>>>  hw/mem/trace-events            |   4 +-
>>>>>  hw/ppc/spapr.c                 |   5 +-
>>>>>  include/hw/boards.h            |  17 ++++++
>>>>>  include/hw/mem/memory-device.h |  17 ++++--
>>>>>  include/hw/mem/pc-dimm.h       |   3 +-
>>>>>  include/hw/resource-handler.h  |  46 ++++++++++++++++
>>>>>  stubs/Makefile.objs            |   1 -
>>>>>  stubs/qmp_memory_device.c      |  13 -----
>>>>>  17 files changed, 364 insertions(+), 121 deletions(-)
>>>>>  create mode 100644 hw/core/resource-handler.c
>>>>>  create mode 100644 include/hw/resource-handler.h
>>>>>  delete mode 100644 stubs/qmp_memory_device.c
>>>>>     
>>>>
>>>> If there are no further comments, I'll send a v2 by the end of this
>>>> week. Thanks!  
>>> I couldn't convince myself that ResourceHandler is really necessary.
>>> My main gripe with it, is that it imposes specific ordering wrt hotplug
>>> handler that resources will be touched. Other issue is that it looks
>>> a bit over-engineered with a lot of code fragmentation. Hence,
>>>
>>> I'd suggest use simple hotplug handler chaining instead,
>>> which should take care of wiring up virtio-mem/virtio-pmem,
>>> keeping code compact at the same time.  
>>
>> I'll have a look tomorrow or next friday if that could work - not sure
>> yet about unplug vs. unplug requests. Unplug requests might be tricky.
>> Would be nice if it would work. Thanks!
> If you have issues with it, ping me, Maybe we'd figure out how to make it
> work together.

Looks promising :) Will post something based on your qdev patch as soon
as I have it cleaned up.

Thanks for your help!

> 
>>
>>>
>>> Could you try something along these lines (I'll post a patch to
>>> override default hotplug handler as reply here for you to pick up,
>>> that should make following work):
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 100dfdc..c400c0c 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -393,12 +393,30 @@ static void s390_machine_reset(void)
>>>      s390_ipl_prepare_cpu(ipl_cpu);
>>>      s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
>>>  }
>>> +static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>>> +                                         DeviceState *dev, Error **errp)
>>> +{
>>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
>>> +        /* do checks && set default values if it weren't set by user */
>>> +        /* possibly pass to device's bus pre_plug handler if need */
>>> +    }
>>> +}
>>>  
>>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>>                                       DeviceState *dev, Error **errp)
>>>  {
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>          s390_cpu_plug(hotplug_dev, dev, errp);
>>> +    } if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW)) {
>>> +        HotplugHandler *hotplug_ctrl = dev->parent_bus->hotplug_handler;
>>> +
>>> +        /* do machine specific wiring, i.e.
>>> +         * assign resources specified by user or by foo_pre_plug(),
>>> +         * like map region into machine address space at specified address */
>>> +        if (OK) {
>>> +            /* pass control down to bus specific hotplug chain */
>>> +            hotplug_handler_plug(hotplug_ctrl, dev, errp);
>>> +        }
>>>      }
>>>  }
>>>  
>>> @@ -449,6 +467,8 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>>>  {
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>          return HOTPLUG_HANDLER(machine);
>>> +    } (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM)) {
>>> +        return HOTPLUG_HANDLER(machine);
>>>      }
>>>      return NULL;
>>>  }
>>>   
>>
>>
> 


-- 

Thanks,

David / dhildenb