On Thu, 17 May 2018 10:15:26 +0200
David Hildenbrand <da...@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 <da...@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
