MemoryDevice: use multi stage hotplug handlers

PATCH Superseded David Hildenbrand 6 participants 29 messages May 11, 2018 – May 15, 2018

Share & tools

David Hildenbrand

author

2018-05-11 13:20 UTC #0 lore raw

v2 cover MemoryDevice: use multi stage hotplug handlers

This is an alternative implementation of "MemoryDevice: introduce and use ResourceHandler". It is based on the proposal of Igor, to have multi stage hotplug handlers. This seems to work just fine for DIMMs and virtio-mem.

This approach results in slightly more LOC (each machine has to implement support for it), however the end result looks cleaner to me.

----

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.

Now, we can route other devices through the machine hotplug handler, to properly assign/unassign resources - like a portion in guest physical address space.

Main changes to other approach:
- Use multi stage hotplug handler instead of resource handler
- MemoryDevices only compiled if necessary (CONFIG_MEM_HOTPLUG)
- Prepare PC/SPAPR machines properly for multi stage hotplug handlers
- Route SPAPR unplug code via the hotunplug handler
- Directly include s390x support :) But there are no usable memory devices yet :( (well, only my virtio-mem prototype)
- Included "memory-device: drop assert related to align and start of address space"

David Hildenbrand (16):
memory-device: drop assert related to align and start of address space
pc: prepare for multi stage hotplug handlers
pc: route all memory devices through the machine hotplug handler
spapr: prepare for multi stage hotplug handlers
spapr: route all memory devices through the machine hotplug handler
spapr: handle pc-dimm unplug via hotplug handler chain
spapr: handle cpu core unplug via hotplug handler chain
memory-device: new functions to handle plug/unplug
pc-dimm: implement new memory device functions
memory-device: factor out pre-plug into hotplug handler
memory-device: factor out unplug into hotplug handler
memory-device: factor out plug into hotplug handler s390x/sclp: make sure ram_size and maxram_size stay in sync
s390x: prepare for multi stage hotplug handlers
s390x: initialize memory region for memory devices
s390x: support memory devices

Igor Mammedov (1):
qdev: let machine hotplug handler to override bus hotplug handler

 default-configs/s390x-softmmu.mak |   1 +
 hw/core/qdev.c                    |   6 +-
 hw/i386/pc.c                      | 107 +++++++++++++++++++++++-------
 hw/mem/memory-device.c            | 129 ++++++++++++++++++++++--------------
 hw/mem/pc-dimm.c                  |  48 +++++---------
 hw/mem/trace-events               |   4 +-
 hw/ppc/spapr.c                    | 135 +++++++++++++++++++++++++++++++++-----
 hw/s390x/s390-virtio-ccw.c        | 102 +++++++++++++++++++++++++++-
 hw/s390x/sclp.c                   |  18 ++++-
 include/hw/mem/memory-device.h    |  21 ++++--
 include/hw/mem/pc-dimm.h          |   3 +-
 include/hw/qdev-core.h            |  11 ++++
 12 files changed, 446 insertions(+), 139 deletions(-)

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #1 lore raw

PATCH v2 1/17 memory-device: drop assert related to align and start of address space

The start of the address space does not have to be aligned for the search. Handly this case explicitly when starting the search for a new address.

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

---
 hw/mem/memory-device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/mem/memory-device.c +1 -2

index 3e04f3954e..361d38bfc5 100644
@@ -116,7 +116,6 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
     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);

@@ -149,7 +148,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        new_addr = QEMU_ALIGN_UP(address_space_start, align);
     }
 
     /* find address range that will fit new memory device */

Signature · 1 line

2.14.3

Michael S. Tsirkin

2018-05-14 06:03 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v2 01/17] memory-device: drop assert related to align and start of address space

Quoted 3 lines · David Hildenbrand

The start of the address space does not have to be aligned for the search. Handly this case explicitly when starting the search for a new address.

Handly -> Handle?

Quoted 29 lines

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

---
 hw/mem/memory-device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/mem/memory-device.c +1 -2

index 3e04f3954e..361d38bfc5 100644
@@ -116,7 +116,6 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
     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);

@@ -149,7 +148,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        new_addr = QEMU_ALIGN_UP(address_space_start, align);
     }
 
     /* find address range that will fit new memory device */

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #3 lore raw

PATCH v2 2/17 qdev: let machine hotplug handler to override bus hotplug handler

In reply to David Hildenbrand

From: Igor Mammedov <[email protected]>

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

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

---
 hw/core/qdev.c         |  6 ++----
 include/hw/qdev-core.h | 11 +++++++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

hw/core/qdev.c +2 -4

index f6f92473b8..885286f579 100644
@@ -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;
 }

include/hw/qdev-core.h +11 -0

index 9453588160..e6a8eca558 100644
@@ -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,

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #4 lore raw

PATCH v2 3/17 pc: prepare for multi stage hotplug handlers

In reply to David Hildenbrand

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

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

---
 hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

hw/i386/pc.c +31 -8

index b2374febbb..39153fa083 100644
@@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,

@@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         pc_dimm_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
-    } else {
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
+    } else if (!dev->parent_bus) {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }

@@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
-    } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
+    } else if (!dev->parent_bus) {
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #5 lore raw

PATCH v2 4/17 pc: route all memory devices through the machine hotplug handler

In reply to David Hildenbrand

Necessary to hotplug them cleanly later. We can only support memory devices that are not DIMMs if we have a parent bus. Otherwise we might miss "Device '%s' can not be hotplugged on this machine" cases.

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

---
 hw/i386/pc.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/i386/pc.c +7 -0

index 39153fa083..7a5558ebea 100644
@@ -74,6 +74,7 @@
 #include "hw/nmi.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
+#include "hw/mem/memory-device.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ

@@ -2079,6 +2080,12 @@ static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
         return HOTPLUG_HANDLER(machine);
     }
 
+    if (dev->parent_bus) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            return HOTPLUG_HANDLER(machine);
+        }
+    }
+
     return NULL;
 }

Signature · 1 line

2.14.3

Paolo Bonzini

2018-05-12 14:47 UTC #6 lore raw

Re: [Qemu-devel] [PATCH v2 04/17] pc: route all memory devices through the machine hotplug handler

hw/i386/pc.c static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, · line 2088

Quoted 6 lines · David Hildenbrand

hw/i386/pc.c +6 -0

+    if (dev->parent_bus) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            return HOTPLUG_HANDLER(machine);
+        }
+    }
+

How do you get here with a MemoryDevice that has !dev->parent_bus?

Thanks,

Paolo

David Hildenbrand

author

2018-05-12 16:45 UTC #7 lore raw

Re: [Qemu-devel] [PATCH v2 04/17] pc: route all memory devices through the machine hotplug handler

Quoted 10 lines · Paolo Bonzini

hw/i386/pc.c static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, · line 2088
Quoted 6 lines · David Hildenbrand
hw/i386/pc.c +6 -0
+    if (dev->parent_bus) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            return HOTPLUG_HANDLER(machine);
+        }
+    }
+
How do you get here with a MemoryDevice that has !dev->parent_bus?

Excellent question :)

This is for now (for pc and spapr) a theoretical case, but I included it to make all hotplug handler look alike and also show for other potential device (interfaces) how it should be handled.

I'll give you the s390x example I had in mind:

s390x cannot hotplug dimms. dimms, however are busless devices that implement the MemoryDevice interface.

If we would simply always indicate this way that we have a hotplug handler, e.g. the check in qdev_device_add() would not trigger:

...

if (bus) {
    qdev_set_parent_bus(dev, bus);
} else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
    /* No bus, no machine hotplug handler --> device is not hotpluggable */
    error_setg(&err, "Device '%s' can not be hotplugged on this machine",
               driver);
    goto err_del_dev;
}

...

So the rational is "if its a busless device and I (the machine) am not able to fully plug it, I must also not partially plug it."

However, right now I am not sure (due to qdev_hotplug) if this is enough.

Quoted 4 lines

Thanks,
Paolo

Signature · 3 lines

Thanks,

David / dhildenb

David Hildenbrand

author

2018-05-14 09:13 UTC #8 lore raw

Re: [Qemu-devel] [PATCH v2 04/17] pc: route all memory devices through the machine hotplug handler

In reply to Paolo Bonzini

Quoted 10 lines · Paolo Bonzini

hw/i386/pc.c static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, · line 2088
Quoted 6 lines · David Hildenbrand
hw/i386/pc.c +6 -0
+    if (dev->parent_bus) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            return HOTPLUG_HANDLER(machine);
+        }
+    }
+
How do you get here with a MemoryDevice that has !dev->parent_bus?

Thinking about it, I'll drop this check and instead split up CONFIG_MEM_HOTPLUG into CONFIG_MEM_DEVICE and CONFIG_DIMM

Thanks!

Quoted 4 lines

Thanks,
Paolo

Signature · 3 lines

Thanks,

David / dhildenb

David Hildenbrand

author

2018-05-11 13:20 UTC #9 lore raw

PATCH v2 5/17 spapr: prepare for multi stage hotplug handlers

In reply to David Hildenbrand

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

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

---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

hw/ppc/spapr.c +43 ~~-11~~

index a1abcba6ad..db09b626cc 100644
@@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 {
     MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
+    Error *local_err = NULL;
 
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;
 
         if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
+            error_setg(&local_err,
+                       "Memory hotplug not supported for this machine");
+            goto out;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {
-            return;
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                        &local_err);
+        if (local_err) {
+            goto out;
         }
         if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
+            error_setg(&local_err, "Invaild node %d", node);
+            goto out;
         }
 
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, node, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
+    }
+out:
+    error_propagate(errp, local_err);
+}
+
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

@@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
             return;
         }
         spapr_core_unplug_request(hotplug_dev, dev, errp);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
     }
 }
 
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_pre_plug(hotplug_dev, dev, errp);
+        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_pre_plug(hotplug_dev, dev, errp);
+        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

@@ -3987,6 +4018,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #10 lore raw

PATCH v2 6/17 spapr: route all memory devices through the machine hotplug handler

In reply to David Hildenbrand

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

---
 hw/ppc/spapr.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/ppc/spapr.c +7 -0

index db09b626cc..a7c84ec34c 100644
@@ -3669,6 +3669,13 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
         object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         return HOTPLUG_HANDLER(machine);
     }
+
+    if (dev->parent_bus) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            return HOTPLUG_HANDLER(machine);
+        }
+    }
+
     return NULL;
 }

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #11 lore raw

PATCH v2 7/17 spapr: handle pc-dimm unplug via hotplug handler chain

In reply to David Hildenbrand

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

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

---
 hw/ppc/spapr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

hw/ppc/spapr.c +19 -4

index a7c84ec34c..7881565d41 100644
@@ -3291,7 +3291,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3309,9 +3310,21 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                Error **errp)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    g_assert(ds);
+    g_assert(!ds->nr_lmbs);
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3608,7 +3621,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
 
     /* final stage hotplug handler */
-    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
                                &local_err);
     }

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #12 lore raw

PATCH v2 8/17 spapr: handle cpu core unplug via hotplug handler chain

In reply to David Hildenbrand

Let's handle it via hotplug_handler_unplug().

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

---
 hw/ppc/spapr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

hw/ppc/spapr.c +12 -1

index 7881565d41..2df15f9db6 100644
@@ -3412,7 +3412,16 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    /* Call the unplug handler chain. This can never fail. */
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              Error **errp)
+{
+    MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);

@@ -3623,6 +3632,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_unplug(hotplug_dev, dev, &local_err);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_unplug(hotplug_dev, dev, &local_err);
     } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
                                &local_err);

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #13 lore raw

PATCH v2 9/17 memory-device: new functions to handle plug/unplug

In reply to 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 plug/unplug memory devices.

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

---
 include/hw/mem/memory-device.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

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

index 2853b084b5..62d906be50 100644
@@ -29,14 +29,24 @@ typedef struct MemoryDeviceState {
     Object parent_obj;
 } MemoryDeviceState;
 
+/*
+ * MemoryDeviceClass functions should only be called on realized
+ * MemoryDevice instances.
+ */
 typedef struct MemoryDeviceClass {
     InterfaceClass parent_class;
 
+    /* required functions that have to be implemented */
     uint64_t (*get_addr)(const 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,
                              MemoryDeviceInfo *info);
+
+    /* optional functions that can be implemented */
+    uint64_t (*get_align)(const MemoryDeviceState *md);
 } MemoryDeviceClass;
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:20 UTC #14 lore raw

PATCH v2 10/17 pc-dimm: implement new memory device functions

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

---
 hw/mem/pc-dimm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

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

index 12da89d562..5e2e3263ab 100644
@@ -244,6 +244,21 @@ 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)
+{
+    const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
+    PCDIMMDevice *dimm = PC_DIMM(md);
+
+    return ddc->get_memory_region(dimm, &error_abort);
+}
+
 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 +319,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;

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:21 UTC #15 lore raw

PATCH v2 11/17 memory-device: factor out pre-plug into hotplug handler

In reply to David Hildenbrand

Let's move all pre-plug checks we can do without the device being realized into the applicable hotplug handler for pc and spapr.

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

---
 hw/i386/pc.c                   | 11 +++++++
 hw/mem/memory-device.c         | 72 +++++++++++++++++++-----------------------
 hw/ppc/spapr.c                 | 11 +++++++
 include/hw/mem/memory-device.h |  2 ++
 4 files changed, 57 insertions(+), 39 deletions(-)

hw/i386/pc.c +11 -0

index 7a5558ebea..49e590ef16 100644
@@ -2010,6 +2010,16 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
+                               &local_err);
+    }
+
+    if (local_err) {
+        goto out;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_pre_plug(hotplug_dev, dev, &local_err);

@@ -2017,6 +2027,7 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
                                  &local_err);
     }
+out:
     error_propagate(errp, local_err);
 }

hw/mem/memory-device.c +33 ~~-39~~

97 lines not on this page · raw message

hw/ppc/spapr.c +11 -0

index 2df15f9db6..870eafae28 100644
@@ -3676,6 +3676,16 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
+                               &local_err);
+    }
+
+    if (local_err) {
+        goto out;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_pre_plug(hotplug_dev, dev, &local_err);

@@ -3685,6 +3695,7 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
         hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
                                  &local_err);
     }
+out:
     error_propagate(errp, local_err);
 }

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

index 62d906be50..3a4e9edc92 100644
@@ -51,6 +51,8 @@ typedef struct MemoryDeviceClass {
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
+void memory_device_pre_plug(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);

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:21 UTC #16 lore raw

PATCH v2 12/17 memory-device: factor out unplug into hotplug handler

In reply to David Hildenbrand

Let's move the unplug logic into the applicable hotplug handler for pc and spapr.

We'll move the plug logic next, then this will look more symmetrical in the hotplug handlers.

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

---
 hw/i386/pc.c                   | 17 ++++++++++++++++-
 hw/mem/memory-device.c         | 14 ++++++++++++--
 hw/mem/pc-dimm.c               |  2 --
 hw/mem/trace-events            |  2 ++
 hw/ppc/spapr.c                 | 16 +++++++++++++++-
 include/hw/mem/memory-device.h |  2 +-
 6 files changed, 46 insertions(+), 7 deletions(-)

hw/i386/pc.c +16 -1

index 49e590ef16..b90cbd5495 100644
@@ -2044,6 +2044,12 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
     } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+
+    if (local_err) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
+        }
+    }
     error_propagate(errp, local_err);
 }

@@ -2080,7 +2086,16 @@ static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
         error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
-    error_propagate(errp, local_err);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
+    }
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,

hw/mem/memory-device.c +12 -2

index d22c91993f..8f10d613ea 100644
@@ -17,6 +17,7 @@
 #include "qemu/range.h"
 #include "hw/virtio/vhost.h"
 #include "sysemu/kvm.h"
+#include "trace.h"
 
 static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
 {

@@ -246,12 +247,21 @@ void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
                                 addr - ms->device_memory->base, mr);
 }
 
-void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr)
+void memory_device_unplug(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_plug */
     g_assert(ms->device_memory);
 
+    if (!memory_region_is_mapped(mr)) {
+        return;
+    }
+
     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 = {

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

index 5e2e3263ab..d487bb513b 100644
@@ -94,9 +94,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);
 }

hw/mem/trace-events +2 -0

index e150dcc497..a661ee49a3 100644
@@ -3,3 +3,5 @@
 # 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_unassign_address(uint64_t addr) "0x%"PRIx64

hw/ppc/spapr.c +15 -1

index 870eafae28..4b5e4de7da 100644
@@ -3621,6 +3621,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
         hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
 out:
+    if (local_err) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
+        }
+    }
     error_propagate(errp, local_err);
 }

@@ -3638,7 +3643,16 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
                                &local_err);
     }
-    error_propagate(errp, local_err);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
+    }
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

include/hw/mem/memory-device.h +1 -1

index 3a4e9edc92..b8365959e7 100644
@@ -58,6 +58,6 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
                                      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_unplug(MachineState *ms, MemoryDeviceState *md);
 
 #endif

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:21 UTC #17 lore raw

PATCH v2 13/17 memory-device: factor out plug into hotplug handler

In reply to David Hildenbrand

Let's move the plug logic into the applicable hotplug handler for pc and spapr.

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

---
 hw/i386/pc.c                   | 35 ++++++++++++++++++++---------------
 hw/mem/memory-device.c         | 40 ++++++++++++++++++++++++++++++++++------
 hw/mem/pc-dimm.c               | 29 +----------------------------
 hw/mem/trace-events            |  2 +-
 hw/ppc/spapr.c                 | 15 ++++++++++++---
 include/hw/mem/memory-device.h |  7 ++-----
 include/hw/mem/pc-dimm.h       |  3 +--
 7 files changed, 71 insertions(+), 60 deletions(-)

hw/i386/pc.c +20 ~~-15~~

index b90cbd5495..8bf0de1ef2 100644
@@ -1682,22 +1682,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

@@ -1715,7 +1701,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;
     }

@@ -2036,6 +2022,25 @@ static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(hotplug_dev);
+        uint64_t align = 0;
+
+        /* compat handling: force to TARGET_PAGE_SIZE */
+        if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
+            !pcmc->enforce_aligned_dimm) {
+            align = TARGET_PAGE_SIZE;
+        }
+        memory_device_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
+                           align ? &align : NULL, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         pc_dimm_plug(hotplug_dev, dev, &local_err);

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

index 8f10d613ea..04bdb30f22 100644
@@ -69,9 +69,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,11 +238,38 @@ void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
     }
 }
 
-void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
-                               uint64_t addr)
+void memory_device_plug(MachineState *ms, MemoryDeviceState *md,
+                        uint64_t *enforced_align, Error **errp)
 {
-    /* we expect a previous call to memory_device_get_free_addr() */
+    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;
+
+    /* we expect a previous call to memory_device_pre_plug */
     g_assert(ms->device_memory);
+    g_assert(mr && !memory_region_is_mapped(mr));
+
+    /* compat handling, some alignment has to be enforced for DIMMs */
+    if (enforced_align) {
+        align = *enforced_align;
+    } else {
+        align = memory_region_get_alignment(mr);
+    }
+
+    /* our device might have stronger alignment requirements */
+    if (mdc->get_align) {
+        align = MAX(align, mdc->get_align(md));
+    }
+
+    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);

hw/mem/pc-dimm.c +1 ~~-28~~

index d487bb513b..8b1dcb3260 100644
@@ -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:

hw/mem/trace-events +1 -1

index a661ee49a3..930b6aa6ea 100644
@@ -2,6 +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

hw/ppc/spapr.c +12 -3

index 4b5e4de7da..6bb677c95d 100644
@@ -3144,16 +3144,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;
     }

@@ -3595,6 +3594,16 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_plug(ms, MEMORY_DEVICE(dev), NULL, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;

include/hw/mem/memory-device.h +2 -5

index b8365959e7..a7408597fd 100644
@@ -53,11 +53,8 @@ MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
 void memory_device_pre_plug(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_plug(MachineState *ms, MemoryDeviceState *md,
+                        uint64_t *enforced_align, Error **errp);
 void memory_device_unplug(MachineState *ms, MemoryDeviceState *md);
 
 #endif

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

index 627c8601d9..006c80fb2e 100644
@@ -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

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:21 UTC #18 lore raw

PATCH v2 14/17 s390x/sclp: make sure ram_size and maxram_size stay in sync

In reply to David Hildenbrand

On s390x, we sometimes have to shrink ram_size in order to be able to correctly indicate the size to the guest.

In case maxmem is not set, ram_size and maxram_size should always be kept equal. Make sure to also fixup maxram_size if necessary.

In case maxmem is set, we really want to bail out in case we have to round down, as it basically can screw up the size of the device memory area later on.

Pleas note that this fixup ususally does not happen with sane values for the ram size.

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

---
 hw/s390x/sclp.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

hw/s390x/sclp.c +15 -3

index 047d577313..0757914374 100644
@@ -21,6 +21,8 @@
 #include "hw/s390x/event-facility.h"
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/ipl.h"
+#include "qemu/error-report.h"
+
 
 static inline SCLPDevice *get_sclp_device(void)
 {

@@ -318,9 +320,19 @@ static void sclp_memory_init(SCLPDevice *sclp)
      * down to align with the nearest increment boundary. */
     initial_mem = initial_mem >> increment_size << increment_size;
 
-    machine->ram_size = initial_mem;
-    /* let's propagate the changed ram size into the global variable. */
-    ram_size = initial_mem;
+    /* also shrink maxram_size in case we don't have maxmem configured */
+    if (initial_mem != machine->ram_size) {
+        if (machine->ram_size < machine->maxram_size) {
+            error_report("Ram size ('" RAM_ADDR_FMT "') had to be rounded "
+                         "down to ('" RAM_ADDR_FMT "'), maxmem not supported",
+                         machine->ram_size, initial_mem);
+            exit(1);
+        }
+        /* propagate the changed ram size into the different places */
+        machine->ram_size = initial_mem;
+        ram_size = initial_mem;
+        machine->maxram_size = initial_mem;
+    }
 }
 
 static void sclp_init(Object *obj)

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:21 UTC #19 lore raw

PATCH v2 15/17 s390x: prepare for multi stage hotplug handlers

In reply to David Hildenbrand

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

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

---
 hw/s390x/s390-virtio-ccw.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

hw/s390x/s390-virtio-ccw.c +39 -2

index 100dfdc96d..ee0a2b124f 100644
@@ -394,21 +394,56 @@ static void s390_machine_reset(void)
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
 }
 
+static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp)
+{
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
+    }
+    error_propagate(errp, local_err);
+}
+
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
+    }
+}
+
+static void s390_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
+{
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,

@@ -496,8 +531,10 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
     /* it is overridden with 'host' cpu *in kvm_arch_init* */
     mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
+    hc->pre_plug = s390_machine_device_pre_plug;
     hc->plug = s390_machine_device_plug;
     hc->unplug_request = s390_machine_device_unplug_request;
+    hc->unplug = s390_machine_device_unplug;
     nc->nmi_monitor_handler = s390_nmi;
 }

Signature · 1 line

2.14.3

David Hildenbrand

author

2018-05-11 13:21 UTC #20 lore raw

PATCH v2 16/17 s390x: initialize memory region for memory devices

In reply to David Hildenbrand

While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.

So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).

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

---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

hw/s390x/s390-virtio-ccw.c +26 -2

index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;

@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
+
+    /* initialize device memory address space */
+    if (machine->ram_size < machine->maxram_size) {
+        ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size;
+
+        if (QEMU_ALIGN_UP(machine->maxram_size, TARGET_PAGE_SIZE) !=
+            machine->maxram_size) {
+            error_report("maximum memory size must be aligned to multiple of "
+                         "%d bytes", TARGET_PAGE_SIZE);
+            exit(EXIT_FAILURE);
+        }
+
+        machine->device_memory->base = machine->ram_size;
+        memory_region_init(&machine->device_memory->mr, OBJECT(ms),
+                           "device-memory", device_mem_size);
+        memory_region_add_subregion(sysmem, machine->device_memory->base,
+                                    &machine->device_memory->mr);
+
+    }
+
     /* Initialize storage key device */
     s390_skeys_init();
     /* Initialize storage attributes device */

@@ -304,7 +328,7 @@ static void ccw_init(MachineState *machine)
     DeviceState *dev;
 
     s390_sclp_init();
-    s390_memory_init(machine->ram_size);
+    s390_memory_init(machine);
 
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);

Signature · 1 line

2.14.3

Murilo Opsfelder Araujo

2018-05-11 18:34 UTC #21 lore raw

Re: [Qemu-devel] [PATCH v2 16/17] s390x: initialize memory region for memory devices

Quoted 30 lines · David Hildenbrand

While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.
So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
hw/s390x/s390-virtio-ccw.c +3 -1
index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;

hw/s390x/s390-virtio-ccw.c static void s390_memory_init(ram_addr_t mem_size) · line 187

Quoted 5 lines

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

@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));

Is there any QEMU guideline/preference/recommendation in using g_new0 vs. g_malloc0?

I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:

http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html

Quoted 36 lines

hw/s390x/s390-virtio-ccw.c +21 -1

+
+    /* initialize device memory address space */
+    if (machine->ram_size < machine->maxram_size) {
+        ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size;
+
+        if (QEMU_ALIGN_UP(machine->maxram_size, TARGET_PAGE_SIZE) !=
+            machine->maxram_size) {
+            error_report("maximum memory size must be aligned to multiple of "
+                         "%d bytes", TARGET_PAGE_SIZE);
+            exit(EXIT_FAILURE);
+        }
+
+        machine->device_memory->base = machine->ram_size;
+        memory_region_init(&machine->device_memory->mr, OBJECT(ms),
+                           "device-memory", device_mem_size);
+        memory_region_add_subregion(sysmem, machine->device_memory->base,
+                                    &machine->device_memory->mr);
+
+    }
+
     /* Initialize storage key device */
     s390_skeys_init();
     /* Initialize storage attributes device */

@@ -304,7 +328,7 @@ static void ccw_init(MachineState *machine)
     DeviceState *dev;
 
     s390_sclp_init();
-    s390_memory_init(machine->ram_size);
+    s390_memory_init(machine);
 
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);

Signature · 1 line

2.14.3

Signature · 1 line

Murilo

Eduardo Habkost

2018-05-11 18:44 UTC #22 lore raw

Re: [Qemu-devel] [PATCH v2 16/17] s390x: initialize memory region for memory devices

Quoted 40 lines · Murilo Opsfelder Araujo

Quoted 30 lines · David Hildenbrand
While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.
So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
hw/s390x/s390-virtio-ccw.c +3 -1
index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;
hw/s390x/s390-virtio-ccw.c static void s390_memory_init(ram_addr_t mem_size) · line 187
Quoted 5 lines
hw/s390x/s390-virtio-ccw.c +2 -0
@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
Is there any QEMU guideline/preference/recommendation in using g_new0 vs. g_malloc0?

Quoted 3 lines

I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html

I don't see any reason to not use g_new0() instead of g_malloc0(sizeof(...)), as it's more readable.

But I don't think it's a problem that should block the patch from being merged. We have hundreds of g_malloc*(sizeof(...)) calls in the tree.

Signature · 1 line

Eduardo

David Hildenbrand

author

2018-05-12 07:54 UTC #23 lore raw

Re: [Qemu-devel] [PATCH v2 16/17] s390x: initialize memory region for memory devices

Quoted 41 lines · Eduardo Habkost

Quoted 40 lines · Murilo Opsfelder Araujo
Quoted 30 lines · David Hildenbrand
While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.
So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
hw/s390x/s390-virtio-ccw.c +3 -1
index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;
hw/s390x/s390-virtio-ccw.c static void s390_memory_init(ram_addr_t mem_size) · line 187
Quoted 5 lines
hw/s390x/s390-virtio-ccw.c +2 -0
@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
Is there any QEMU guideline/preference/recommendation in using g_new0 vs. g_malloc0?

Quoted 4 lines

Quoted 3 lines
I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html

This patch comes unmodified from my same queue, therefore the code looks identical :)

Quoted 2 lines

I don't see any reason to not use g_new0() instead of g_malloc0(sizeof(...)), as it's more readable.

I clearly favor g_malloc over g_new (except for arrays) for two simple reasons

1. No need to specify the type. Impossible to specify the wrong type. Easy to rename types.

2. Every C developer should be able to understand what g_malloc() does. This is not true for g_new. Especially as it might look strange for C++ developers (new vs. new[] - why don't we have g_new() vs. g_new_array())

I am a simple man, I prefer functions with one parameter if only one parameter is needed :)

Quoted 4 lines

But I don't think it's a problem that should block the patch from being merged. We have hundreds of g_malloc*(sizeof(...)) calls in the tree.

I assume there are a lot of hard feelings about this. I will continue using g_malloc() for scalars until the last user is removed from the QEMU source code. Or there is a coding style statement about it (haven't found one) ... or people start to curse me when I send patches :)

Signature · 3 lines

Thanks,

David / dhildenb

Murilo Opsfelder Araujo

2018-05-14 23:04 UTC #24 lore raw

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 16/17] s390x: initialize memory region for memory devices

Quoted 67 lines · David Hildenbrand

Quoted 41 lines · Eduardo Habkost
Quoted 40 lines · Murilo Opsfelder Araujo
Quoted 30 lines · David Hildenbrand
While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.
So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
hw/s390x/s390-virtio-ccw.c +3 -1
index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;
hw/s390x/s390-virtio-ccw.c static void s390_memory_init(ram_addr_t mem_size) · line 187
Quoted 5 lines
hw/s390x/s390-virtio-ccw.c +2 -0
@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
Is there any QEMU guideline/preference/recommendation in using g_new0 vs. g_malloc0?
Quoted 4 lines
Quoted 3 lines
I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html
This patch comes unmodified from my same queue, therefore the code looks identical :)
Quoted 2 lines
I don't see any reason to not use g_new0() instead of g_malloc0(sizeof(...)), as it's more readable.
I clearly favor g_malloc over g_new (except for arrays) for two simple reasons
1. No need to specify the type. Impossible to specify the wrong type. Easy to rename types.
2. Every C developer should be able to understand what g_malloc() does. This is not true for g_new. Especially as it might look strange for C++ developers (new vs. new[] - why don't we have g_new() vs. g_new_array())
I am a simple man, I prefer functions with one parameter if only one parameter is needed :)
Quoted 1 line

Quoted 8 lines

Quoted 3 lines
But I don't think it's a problem that should block the patch from being merged. We have hundreds of g_malloc*(sizeof(...)) calls in the tree.
I assume there are a lot of hard feelings about this. I will continue using g_malloc() for scalars until the last user is removed from the QEMU source code. Or there is a coding style statement about it (haven't found one) ... or people start to curse me when I send patches :)

Having g_malloc() for scalars and g_new() for arrays makes sense.

I understand and agree that using g_malloc() should not be a blocker for a patch, as Eduardo stated.

Looking at the history, there are quite a few patches replacing g_malloc*() by g_new*() because "is safer against overflow" (see commit 071d4054770205ddb8a58a9e2735069d8fe52af1 as an example):

    git log --oneline --grep=g_new

Perhaps we just need to update "3. Low level memory management" section in HACKING file describing the situations where g_new() is preferred vs. g_malloc() and vice-versa; and use the agreed criteria to ack/nack patches.

Signature · 1 line

Murilo

Markus Armbruster

2018-05-15 05:58 UTC #25 lore raw

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 16/17] s390x: initialize memory region for memory devices

Quoted 37 lines · Murilo Opsfelder Araujo

Quoted 36 lines · David Hildenbrand
Quoted 35 lines · Eduardo Habkost
Quoted 34 lines · Murilo Opsfelder Araujo
Quoted 33 lines · David Hildenbrand
While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.
So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
hw/s390x/s390-virtio-ccw.c +3 -1
index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;
@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
hw/s390x/s390-virtio-ccw.c static void s390_memory_init(ram_addr_t mem_size) · line 187
Quoted 2 lines
hw/s390x/s390-virtio-ccw.c +2 -0
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
Is there any QEMU guideline/preference/recommendation in using g_new0 vs. g_malloc0?

Yes, there is: we prefer g_new(T, n) over g_malloc(sizeof(T) * n) even when n==1. Commit b45c03f585e explains:

g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors.

'One' doesn't apply when n==1. 'Two' does.

We're okay with things like T *v = g_malloc(sizeof(*v)). Yes, 'two' applies here as well, but screwups are relatively unlikely.

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
Quoted 3 lines
I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html
This patch comes unmodified from my same queue, therefore the code looks identical :)

Quoted 7 lines

Quoted 7 lines
Quoted 2 lines
I don't see any reason to not use g_new0() instead of g_malloc0(sizeof(...)), as it's more readable.
I clearly favor g_malloc over g_new (except for arrays) for two simple reasons
1. No need to specify the type. Impossible to specify the wrong type.

Quite possible to specify the wrong size in other ways, and the type checker can't save you then (that's 'two'), although Coverity might.

Quoted 1 line

Quoted 1 line
Easy to rename types.

Renaming a type is exactly as easy as renaming a variable or any other
identifer: you have to update all occurences.

Quoted 3 lines

Quoted 3 lines
2. Every C developer should be able to understand what g_malloc() does. This is not true for g_new. Especially as it might look strange for C++ developers (new vs. new[] - why don't we have g_new() vs. g_new_array())

I'm sympathetic of this argument in general, but not here. g_malloc() already differs from malloc(), and for good reasons. Moreover, we use g_new() all over the place; there's simply no way to avoid understanding it.

Quoted 2 lines

Quoted 2 lines
I am a simple man, I prefer functions with one parameter if only one parameter is needed :)

The second parameter is moderately ugly in the non-array case. But then GLib is full of ugly. We'll live.

Quoted 10 lines

Quoted 8 lines
Quoted 3 lines
But I don't think it's a problem that should block the patch from being merged. We have hundreds of g_malloc*(sizeof(...)) calls in the tree.
I assume there are a lot of hard feelings about this. I will continue using g_malloc() for scalars until the last user is removed from the QEMU source code. Or there is a coding style statement about it (haven't found one) ... or people start to curse me when I send patches :)
Having g_malloc() for scalars and g_new() for arrays makes sense.

No. Use g_new() with types, g_malloc() / g_malloc_n() with other expressions. When both are practical, consider preferring g_new() for extra type checking. Also consider readability.

Quoted 2 lines

I understand and agree that using g_malloc() should not be a blocker for a patch, as Eduardo stated.

The assignment that triggered this sub-thread

    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));

is a matter of taste. I'd prefer

    machine->device_memory = g_new0(DeviceMemoryState, 1);

myself, because I find it easier to read. Giving the type checker the actual type to work with is a nice bonus.

Quoted 10 lines

Looking at the history, there are quite a few patches replacing g_malloc*() by g_new*() because "is safer against overflow" (see commit 071d4054770205ddb8a58a9e2735069d8fe52af1 as an example):
    git log --oneline --grep=g_new
Perhaps we just need to update "3. Low level memory management" section in HACKING file describing the situations where g_new() is preferred vs. g_malloc() and vice-versa; and use the agreed criteria to ack/nack patches.

We tend to update HACKING when we find ourselves debating the same things over and over. Perhaps this is such a case.

David Hildenbrand

author

2018-05-15 07:58 UTC #26 lore raw

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 16/17] s390x: initialize memory region for memory devices

Quoted 69 lines · Markus Armbruster

Quoted 37 lines · Murilo Opsfelder Araujo
Quoted 36 lines · David Hildenbrand
Quoted 35 lines · Eduardo Habkost
Quoted 34 lines · Murilo Opsfelder Araujo
Quoted 33 lines · David Hildenbrand
While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.
So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
hw/s390x/s390-virtio-ccw.c +3 -1
index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;
@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
hw/s390x/s390-virtio-ccw.c static void s390_memory_init(ram_addr_t mem_size) · line 187
Quoted 2 lines
hw/s390x/s390-virtio-ccw.c +2 -0
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
Is there any QEMU guideline/preference/recommendation in using g_new0 vs. g_malloc0?
Yes, there is: we prefer g_new(T, n) over g_malloc(sizeof(T) * n) even when n==1. Commit b45c03f585e explains:
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors.
'One' doesn't apply when n==1. 'Two' does.
We're okay with things like T *v = g_malloc(sizeof(*v)). Yes, 'two' applies here as well, but screwups are relatively unlikely.
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines
I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html
This patch comes unmodified from my same queue, therefore the code looks identical :)
Quoted 2 lines
I don't see any reason to not use g_new0() instead of g_malloc0(sizeof(...)), as it's more readable.

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
I clearly favor g_malloc over g_new (except for arrays) for two simple reasons
1. No need to specify the type. Impossible to specify the wrong type.
Quite possible to specify the wrong size in other ways, and the type checker can't save you then (that's 'two'), although Coverity might.

Good point about the type checker!

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
Easy to rename types.
Renaming a type is exactly as easy as renaming a variable or any other
identifer: you have to update all occurences.

And that means touching more lines.

Quoted 6 lines

Quoted 6 lines
Looking at the history, there are quite a few patches replacing g_malloc*() by g_new*() because "is safer against overflow" (see commit 071d4054770205ddb8a58a9e2735069d8fe52af1 as an example):
    git log --oneline --grep=g_new

Quoted 8 lines

Quoted 4 lines
Perhaps we just need to update "3. Low level memory management" section in HACKING file describing the situations where g_new() is preferred vs. g_malloc() and vice-versa; and use the agreed criteria to ack/nack patches.
We tend to update HACKING when we find ourselves debating the same things over and over. Perhaps this is such a case.

I don't want to get too involved in this discussion. (I have other problems to solve :) )

If we make this a rule, I want somebody to convert all applicable cases to the desired format. (we won't be able to convert all cases, e.g. structs with variable sized member arrays.)

Thanks!

Signature · 3 lines

Thanks,

David / dhildenb

Murilo Opsfelder Araujo

2018-05-15 14:02 UTC #27 lore raw

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 16/17] s390x: initialize memory region for memory devices

Quoted 104 lines · David Hildenbrand

Quoted 69 lines · Markus Armbruster
Quoted 37 lines · Murilo Opsfelder Araujo
Quoted 36 lines · David Hildenbrand
Quoted 35 lines · Eduardo Habkost
Quoted 34 lines · Murilo Opsfelder Araujo
Quoted 33 lines · David Hildenbrand
While s390x has no real interface for communicating devices mapped into the physical address space of the guest, paravirtualized devices can easily expose the applicable address range themselves.
So let's use the difference between maxram_size and ram_size as the size for our hotplug memory area (just as on other architectures).
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
hw/s390x/s390-virtio-ccw.c +3 -1
index ee0a2b124f..09b755282b 100644
@@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
 #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
 #define SEG_MSK (~0xfffffULL)
 #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & SEG_MSK)
-static void s390_memory_init(ram_addr_t mem_size)
+static void s390_memory_init(MachineState *machine)
 {
+    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
     MemoryRegion *sysmem = get_system_memory();
+    ram_addr_t mem_size = machine->ram_size;
     ram_addr_t chunk, offset = 0;
     unsigned int number = 0;
     gchar *name;
@@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
     }
     g_free(name);
 
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
hw/s390x/s390-virtio-ccw.c static void s390_memory_init(ram_addr_t mem_size) · line 187
Quoted 2 lines
hw/s390x/s390-virtio-ccw.c +2 -0
+    /* always allocate the device memory information */
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
Is there any QEMU guideline/preference/recommendation in using g_new0 vs. g_malloc0?
Yes, there is: we prefer g_new(T, n) over g_malloc(sizeof(T) * n) even when n==1. Commit b45c03f585e explains:
g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors.
'One' doesn't apply when n==1. 'Two' does.
We're okay with things like T *v = g_malloc(sizeof(*v)). Yes, 'two' applies here as well, but screwups are relatively unlikely.
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines
I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:
http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html
This patch comes unmodified from my same queue, therefore the code looks identical :)
Quoted 2 lines
I don't see any reason to not use g_new0() instead of g_malloc0(sizeof(...)), as it's more readable.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
I clearly favor g_malloc over g_new (except for arrays) for two simple reasons
1. No need to specify the type. Impossible to specify the wrong type.
Quite possible to specify the wrong size in other ways, and the type checker can't save you then (that's 'two'), although Coverity might.
Good point about the type checker!
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Easy to rename types.
Renaming a type is exactly as easy as renaming a variable or any other
identifer: you have to update all occurences.
And that means touching more lines.
Quoted 6 lines
Quoted 6 lines
Looking at the history, there are quite a few patches replacing g_malloc*() by g_new*() because "is safer against overflow" (see commit 071d4054770205ddb8a58a9e2735069d8fe52af1 as an example):
    git log --oneline --grep=g_new
Quoted 8 lines
Quoted 4 lines
Perhaps we just need to update "3. Low level memory management" section in HACKING file describing the situations where g_new() is preferred vs. g_malloc() and vice-versa; and use the agreed criteria to ack/nack patches.
We tend to update HACKING when we find ourselves debating the same things over and over. Perhaps this is such a case.

Quoted 6 lines

I don't want to get too involved in this discussion. (I have other problems to solve :) )
If we make this a rule, I want somebody to convert all applicable cases to the desired format. (we won't be able to convert all cases, e.g. structs with variable sized member arrays.)

I'll try to create a semantic patch to cover most of the cases.

For now, I only sent an update to the HACKING file:

http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg03362.html

Signature · 1 line

Murilo

David Hildenbrand

author

2018-05-11 13:21 UTC #28 lore raw

PATCH v2 17/17 s390x: support memory devices

In reply to David Hildenbrand

Let's route all memory devices we can hotplug through the machine hotplug handler, just like on pc and spapr.

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

---
 default-configs/s390x-softmmu.mak |  1 +
 hw/s390x/s390-virtio-ccw.c        | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

default-configs/s390x-softmmu.mak +1 -0

index 2f4bfe73b4..6b017b65fc 100644
@@ -9,3 +9,4 @@ CONFIG_S390_FLIC=y
 CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
 CONFIG_VFIO_CCW=$(CONFIG_LINUX)
 CONFIG_WDT_DIAG288=y
+CONFIG_MEM_HOTPLUG=y

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

index 09b755282b..91687ca327 100644
@@ -35,6 +35,7 @@
 #include "migration/register.h"
 #include "cpu_models.h"
 #include "hw/nmi.h"
+#include "hw/mem/memory-device.h"
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {

@@ -436,12 +437,29 @@ static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev), NULL,
+                           &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         s390_cpu_plug(hotplug_dev, dev, &local_err);
     } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+
+    if (local_err) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
+        }
+    }
     error_propagate(errp, local_err);
 }

@@ -468,6 +486,16 @@ static void s390_machine_device_unplug(HotplugHandler *hotplug_dev,
                                &local_err);
     }
     error_propagate(errp, local_err);
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_unplug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev));
+    }
 }
 
 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,

@@ -509,6 +537,13 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         return HOTPLUG_HANDLER(machine);
     }
+
+    if (dev->parent_bus) {
+        if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+            return HOTPLUG_HANDLER(machine);
+        }
+    }
+
     return NULL;
 }

Signature · 1 line

2.14.3