MemoryDevice: introduce and use ResourceHandler

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 <[email protected]>

---
 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

hw/core/Makefile.objs +1 -0

index eb88ca979e..7474387fcd 100644
@@ -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

hw/core/qdev.c +40 -1

index f6f92473b8..042e275884 100644
@@ -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;

hw/core/resource-handler.c new file +57 -0

new file mode 100644
index 0000000000..0a1ff0e66a
@@ -0,0 +1,57 @@
+/*
+ * Resource handler interface.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Authors:
+ *  David Hildenbrand <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#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)

include/hw/boards.h +9 -0

index 8748964e6f..ff5142d7c2 100644
@@ -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);

include/hw/resource-handler.h new file +36 -0

new file mode 100644
index 0000000000..3eda1bec5c
@@ -0,0 +1,46 @@
+/*
+ * Resource handler interface.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Authors:
+ *  David Hildenbrand <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef 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);

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

Quoted 19 lines · On Thu, 3 May 2018 17:49:28 +0200 David Hildenbrand <[email protected]> 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.

Quoted 5 lines

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.

Quoted 27 lines · Igor Mammedov

Quoted 19 lines · On Thu, 3 May 2018 17:49:28 +0200 David Hildenbrand <[email protected]> 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.

Quoted 5 lines

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).

Quoted 4 lines

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).

Quoted 4 lines

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.

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

Quoted 53 lines · 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 ++++--

Quoted 9 lines

 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!

Quoted 64 lines · On Wed, 9 May 2018 16:13:14 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 53 lines · 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 ++++--

Quoted 9 lines

 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

Quoted 2 lines

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):

hw/s390x/s390-virtio-ccw.c +20 -0

index 100dfdc..c400c0c 100644
@@ -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;
 }

Quoted 74 lines · Igor Mammedov

Quoted 64 lines · On Wed, 9 May 2018 16:13:14 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 53 lines · 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 ++++--

Quoted 9 lines

 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

Quoted 2 lines

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,

Quoted 3 lines

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!

Quoted 51 lines

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):

hw/s390x/s390-virtio-ccw.c +20 -0

index 100dfdc..c400c0c 100644
@@ -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;
 }

Quoted 79 lines · On Thu, 10 May 2018 15:20:55 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 77 lines · Igor Mammedov

Quoted 64 lines · On Wed, 9 May 2018 16:13:14 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 53 lines · 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 ++++--

Quoted 9 lines

 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

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 54 lines

Quoted 52 lines

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):

hw/s390x/s390-virtio-ccw.c +20 -0

index 100dfdc..c400c0c 100644
@@ -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;
 }