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 <david@redhat.com>
---
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(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 61f1537e14..426fb534c2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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,
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index d22c91993f..8f10d613ea 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -17,6 +17,7 @@
#include "qemu/range.h"
#include "hw/virtio/vhost.h"
#include "sysemu/kvm.h"
+#include "trace.h"
static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
{
@@ -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 = {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 5e2e3263ab..d487bb513b 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -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);
}
diff --git a/hw/mem/trace-events b/hw/mem/trace-events
index e150dcc497..a661ee49a3 100644
--- a/hw/mem/trace-events
+++ b/hw/mem/trace-events
@@ -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
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 562712def2..abdd38a6b5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -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,
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 3a4e9edc92..b8365959e7 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -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
--
2.14.3
On Thu, 17 May 2018 10:15:26 +0200
David Hildenbrand <david@redhat.com> wrote:
> 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 <david@redhat.com>
> ---
> 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(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 61f1537e14..426fb534c2 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -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,
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index d22c91993f..8f10d613ea 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -17,6 +17,7 @@
> #include "qemu/range.h"
> #include "hw/virtio/vhost.h"
> #include "sysemu/kvm.h"
> +#include "trace.h"
>
> static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
> {
> @@ -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 = {
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 5e2e3263ab..d487bb513b 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -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);
> }
>
> diff --git a/hw/mem/trace-events b/hw/mem/trace-events
> index e150dcc497..a661ee49a3 100644
> --- a/hw/mem/trace-events
> +++ b/hw/mem/trace-events
> @@ -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
maybe split out tracing into a separate patch?
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 562712def2..abdd38a6b5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -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));
> + }
> + }
we shouldn't call unplug here.
I'd suggest spapr_memory_plug() and/or memory_device_plug() to do
error handling and rollback on it's own, it shouldn't complicate generic
machines' hotplug handlers.
> 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) {
this check probably not needed, error_propagate()
bails out of local_err is NULL
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + /* first stage hotplug handler */
I'd drop this comment, it looks not necessary and even a bit confusing to me.
> + 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,
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index 3a4e9edc92..b8365959e7 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -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
>> # 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
> maybe split out tracing into a separate patch?
Can do, although I think this is overkill.
>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 562712def2..abdd38a6b5 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -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));
>> + }
>> + }
> we shouldn't call unplug here.
> I'd suggest spapr_memory_plug() and/or memory_device_plug() to do
> error handling and rollback on it's own, it shouldn't complicate generic
> machines' hotplug handlers.
Please note that this is the generic way to handle multi stage hotplug
handlers. (see the more detailed comment in reply to patch 4 if I
remember correctly)
From my point of view, this is the right thing to do.
>
>> 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) {
> this check probably not needed, error_propagate()
> bails out of local_err is NULL
With the current code (returning), this is needed.
>
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> + /* first stage hotplug handler */
> I'd drop this comment, it looks not necessary and even a bit confusing to me.
I'll drop all the comments of this form.
>
>> + 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,
>> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
>> index 3a4e9edc92..b8365959e7 100644
>> --- a/include/hw/mem/memory-device.h
>> +++ b/include/hw/mem/memory-device.h
>> @@ -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
>
--
Thanks,
David / dhildenb
© 2016 - 2025 Red Hat, Inc.