Re: [Qemu-devel] [PATCH v4 13/14] memory-device: factor out unplug into hotplug handler
>> # 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
Re: [Qemu-devel] [PATCH v4 13/14] memory-device: factor out unplug into hotplug handler
On Thu, 17 May 2018 10:15:26 +0200
David Hildenbrand 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
> ---
> 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_propagat
[Qemu-devel] [PATCH v4 13/14] memory-device: factor out unplug into hotplug handler
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
---
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 voi
