Re: [qemu-s390x] [PATCH v1 2/8] qdev: introduce ResourceHandler as a first-stage hotplug handler

[David Hildenbrand]

On 03.05.2018 17:49, David Hildenbrand wrote:
> Hotplug handlers usually do 3 things:
> 1. Allocate some resources for a new device
> 2. Make the new device visible for the guest
> 3. Notify the guest about the new device
> 
> While 2. and 3. are only ever performed once for a device, there
> might be various resources to allocate. E.g. a MemoryDevice could be
> exposed via Virtio. The proxy device hotplug handler (e.g. virtio-pci,
> virtio-ccw) takes care of exposing the device to the guest. However,
> we also have to allocate system resources, like a portion in guest
> physical address space, which is to be handled by the machine.
> 
> One key difference between a hotplug handler and a resource handler is
> that resource handlers have no "unplug_request". There is no
> communication with the guest (this is done by a "hotplug" handler). So
> conceptually, they are different.
> 
> For now we live with the assumption, that - in additon to the existing
> hotplug handler which can do some resource asignment - at most one
> resource handler is necessary.
> 
> We might even later decide to have multiple resource handlers for a
> specific device (e.g. one for each interface they implement). For now,
> this design is sufficient.
> 
> Resource handlers have to run before the hotplug handler runs (esp
> before exposing the device to the guest).
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hw/core/Makefile.objs         |  1 +
>  hw/core/qdev.c                | 41 ++++++++++++++++++++++++++++++-
>  hw/core/resource-handler.c    | 57 
> +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h           |  9 +++++++
>  include/hw/resource-handler.h | 46 ++++++++++++++++++++++++++++++++++
>  5 files changed, 153 insertions(+), 1 deletion(-)
>  create mode 100644 hw/core/resource-handler.c
>  create mode 100644 include/hw/resource-handler.h
> 
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index eb88ca979e..7474387fcd 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
>  common-obj-y += hotplug.o
> +common-obj-y += resource-handler.o
>  common-obj-$(CONFIG_SOFTMMU) += nmi.o
>  
>  common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index f6f92473b8..042e275884 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -35,6 +35,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
>  #include "hw/hotplug.h"
> +#include "hw/resource-handler.h"
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
>  
> @@ -271,6 +272,17 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState 
> *dev)
>      return hotplug_ctrl;
>  }
>  
> +static ResourceHandler *qdev_get_resource_handler(DeviceState *dev)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    if (mc->get_resource_handler) {
> +            return mc->get_resource_handler(ms, dev);
> +    }
> +    return NULL;
> +}
> +
>  static int qdev_reset_one(DeviceState *dev, void *opaque)
>  {
>      device_reset(dev);
> @@ -814,6 +826,7 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> +    ResourceHandler *resource_ctrl;
>      HotplugHandler *hotplug_ctrl;
>      BusState *bus;
>      Error *local_err = NULL;
> @@ -825,6 +838,8 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>          return;
>      }
>  
> +    resource_ctrl = qdev_get_resource_handler(dev);
> +
>      if (value && !dev->realized) {
>          if (!check_only_migratable(obj, &local_err)) {
>              goto fail;
> @@ -840,6 +855,13 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>              g_free(name);
>          }
>  
> +        if (resource_ctrl) {
> +            resource_handler_pre_assign(resource_ctrl, dev, &local_err);
> +            if (local_err != NULL) {
> +                goto fail;
> +            }
> +        }
> +
>          hotplug_ctrl = qdev_get_hotplug_handler(dev);
>          if (hotplug_ctrl) {
>              hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
> @@ -858,12 +880,19 @@ static void device_set_realized(Object *obj, bool 
> value, Error **errp)
>  
>          DEVICE_LISTENER_CALL(realize, Forward, dev);
>  
> +        if (resource_ctrl) {
> +            resource_handler_assign(resource_ctrl, dev, &local_err);
> +            if (local_err != NULL) {
> +                goto post_realize_fail;
> +            }
> +        }
> +
>          if (hotplug_ctrl) {
>              hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
>          }
>  
>          if (local_err != NULL) {
> -            goto post_realize_fail;
> +            goto post_assign_fail;
>          }
>  
>          /*
> @@ -903,6 +932,11 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>          if (qdev_get_vmsd(dev)) {
>              vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>          }
> +
> +        if (resource_ctrl) {
> +            resource_handler_unassign(resource_ctrl, dev);
> +        }
> +
>          if (dc->unrealize) {
>              local_errp = local_err ? NULL : &local_err;
>              dc->unrealize(dev, local_errp);
> @@ -928,6 +962,11 @@ child_realize_fail:
>          vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
>      }
>  
> +post_assign_fail:
> +    if (resource_ctrl) {
> +        resource_handler_unassign(resource_ctrl, dev);
> +    }
> +
>  post_realize_fail:
>      g_free(dev->canonical_path);
>      dev->canonical_path = NULL;
> diff --git a/hw/core/resource-handler.c b/hw/core/resource-handler.c
> new file mode 100644
> index 0000000000..0a1ff0e66a
> --- /dev/null
> +++ b/hw/core/resource-handler.c
> @@ -0,0 +1,57 @@
> +/*
> + * Resource handler interface.
> + *
> + * Copyright (c) 2018 Red Hat Inc.
> + *
> + * Authors:
> + *  David Hildenbrand <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/resource-handler.h"
> +#include "qemu/module.h"
> +
> +void resource_handler_pre_assign(ResourceHandler *rh,
> +                                 const DeviceState *dev, Error **errp)
> +{
> +    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
> +
> +    if (rhc->pre_assign) {
> +        rhc->pre_assign(rh, dev, errp);
> +    }
> +}
> +
> +void resource_handler_assign(ResourceHandler *rh, DeviceState *dev,
> +                             Error **errp)
> +{
> +    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
> +
> +    if (rhc->assign) {
> +        rhc->assign(rh, dev, errp);
> +    }
> +}
> +
> +void resource_handler_unassign(ResourceHandler *rh, DeviceState *dev)
> +{
> +    ResourceHandlerClass *rhc = RESOURCE_HANDLER_GET_CLASS(rh);
> +
> +    if (rhc->unassign) {
> +        rhc->unassign(rh, dev);
> +    }
> +}
> +
> +static const TypeInfo resource_handler_info = {
> +    .name          = TYPE_RESOURCE_HANDLER,
> +    .parent        = TYPE_INTERFACE,
> +    .class_size = sizeof(ResourceHandlerClass),
> +};
> +
> +static void resource_handler_register_types(void)
> +{
> +    type_register_static(&resource_handler_info);
> +}
> +
> +type_init(resource_handler_register_types)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 8748964e6f..ff5142d7c2 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -8,6 +8,7 @@
>  #include "hw/qdev.h"
>  #include "qom/object.h"
>  #include "qom/cpu.h"
> +#include "hw/resource-handler.h"
>  
>  /**
>   * memory_region_allocate_system_memory - Allocate a board's main memory
> @@ -115,6 +116,12 @@ typedef struct {
>   *    of HotplugHandler object, which handles hotplug operation
>   *    for a given @dev. It may return NULL if @dev doesn't require
>   *    any actions to be performed by hotplug handler.
> + * @get_resource_handler: this function is called when a new device is
> + *                        about to be hotplugged. If defined, it returns 
> pointer
> + *                        to an instance of ResourceHandler object, which
> + *                        handles resource asignment for a given @dev. It
> + *                        may return NULL if @dev doesn't require any actions
> + *                        to be performed by a resource handler.
>   * @cpu_index_to_instance_props:
>   *    used to provide @cpu_index to socket/core/thread number mapping, 
> allowing
>   *    legacy code to perform maping from cpu_index to topology properties
> @@ -208,6 +215,8 @@ struct MachineClass {
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> +    ResourceHandler *(*get_resource_handler)(MachineState *machine,
> +                                             const DeviceState *dev);
>      CpuInstanceProperties (*cpu_index_to_instance_props)(MachineState 
> *machine,
>                                                           unsigned cpu_index);
>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> diff --git a/include/hw/resource-handler.h b/include/hw/resource-handler.h
> new file mode 100644
> index 0000000000..3eda1bec5c
> --- /dev/null
> +++ b/include/hw/resource-handler.h
> @@ -0,0 +1,46 @@
> +/*
> + * Resource handler interface.
> + *
> + * Copyright (c) 2018 Red Hat Inc.
> + *
> + * Authors:
> + *  David Hildenbrand <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef RESOURCE_HANDLER_H
> +#define RESOURCE_HANDLER_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_RESOURCE_HANDLER "resource-handler"
> +
> +#define RESOURCE_HANDLER_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(ResourceHandlerClass, (klass), TYPE_RESOURCE_HANDLER)
> +#define RESOURCE_HANDLER_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(ResourceHandlerClass, (obj), TYPE_RESOURCE_HANDLER)
> +#define RESOURCE_HANDLER(obj) \
> +    INTERFACE_CHECK(ResourceHandler, (obj), TYPE_RESOURCE_HANDLER)
> +
> +typedef struct ResourceHandler {
> +    Object Parent;
> +} ResourceHandler;
> +
> +typedef struct ResourceHandlerClass {
> +    InterfaceClass parent;
> +
> +    void (*pre_assign)(ResourceHandler *rh, const DeviceState *dev,
> +                       Error **errp);
> +    void (*assign)(ResourceHandler *rh, DeviceState *dev, Error **errp);
> +    void (*unassign)(ResourceHandler *rh, DeviceState *dev);
> +} ResourceHandlerClass;
> +
> +void resource_handler_pre_assign(ResourceHandler *rh, const DeviceState *dev,
> +                                 Error **errp);
> +void resource_handler_assign(ResourceHandler *rh, DeviceState *dev,
> +                             Error **errp);
> +void resource_handler_unassign(ResourceHandler *rh, DeviceState *dev);
> +
> +#endif /* RESOURCE_HANDLER_H */
> 

I did the following changes to make it pass "make check".

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 042e275884..efa59b9d1e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -274,12 +274,18 @@ HotplugHandler
*qdev_get_hotplug_handler(DeviceState *dev)

 static ResourceHandler *qdev_get_resource_handler(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_machine());
-    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    MachineState *ms;
+    MachineClass *mc;
+    Object *m_obj = qdev_get_machine();

-    if (mc->get_resource_handler) {
-            return mc->get_resource_handler(ms, dev);
+    if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
+        ms = MACHINE(m_obj);
+        mc = MACHINE_GET_CLASS(ms);
+        if (mc->get_resource_handler) {
+                return mc->get_resource_handler(ms, dev);
+        }
     }
+
     return NULL;
 }

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2..45c64bee64 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -645,6 +645,7 @@ tests/atomic_add-bench$(EXESUF):
tests/atomic_add-bench.o $(test-util-obj-y)

 tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
        hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
+       hw/core/resource-handler.o \
        hw/core/bus.o \
        hw/core/irq.o \
        hw/core/fw-path-provider.o \


-- 

Thanks,

David / dhildenb