Re: [Qemu-devel] [PATCH 08/35] qdev: hotplug for buss-less devices

[Peter Crosthwaite]

On Fri, Apr 4, 2014 at 11:36 PM, Igor Mammedov <address@hidden> wrote:
> Adds get_hotplug_handler() method to machine, and
> makes bus-less device to use it during hotplug
> as a means to discover hotplug handler controller.
> Returned controller is used to permorm a hotplug
> action.
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/core/qdev.c      | 13 +++++++++++++
>  include/hw/boards.h |  8 ++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 60f9df1..50bb8f5 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -34,6 +34,7 @@
>  #include "qapi/qmp/qjson.h"
>  #include "monitor/monitor.h"
>  #include "hw/hotplug.h"
> +#include "hw/boards.h"
>
>  int qdev_hotplug = 0;
>  static bool qdev_hot_added = false;
> @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, 
> Error **err)
>              local_err == NULL) {
>              hotplug_handler_plug(dev->parent_bus->hotplug_handler,
>                                   dev, &local_err);
> +        } else if (local_err == NULL &&
> +                   object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {

This doesn't look right - you are relying on global state to implement
hotplug. Later in the series you then need to do RTTI at the machine
level to properly determine if hotplug is really appropriate then do
some machine specific attachment. This idea of "we dont need a bus
just hotplug to the machine itself" doesnt seem generic at all. If you
need to define hotplug socket-side functionality, then that seems to
me to be a bus level problem - and you should use a bus - probably
SYSBUS or an extension thereof. Then your hotplug work can be
generalized to sysbus and a range of devices rather than us having
independent competing "embedded vs PC" hotplug implementations.

How do you implement hotplugging to multiple completely independent
DIMM slots? (i.e. two slots at completely different places in the bus
heir-achy).

Regarding DIMM, I think it is a bus. I'm not sure if it actually needs
its own class yet (TBH I haven't gone line-line on this series looking
for DIMMisms). It is ultimately a memory mapped bus if anything I
think it should be:

.name = TYPE_DIMM_DEVICE,
.parent = TYPE_SYSBUS_DEVICE,

.name = TYPE_DIMM_SLOT,
.parent = TYPE_SYSTEM_BUS,

It is true that we still need to remove the global stateness from
sysbus itself (there are a few threads on list on the issue) before
this can really be nice.

Regards,
Peter

> +            HotplugHandler *hotplug_ctrl;
> +            MachineState *machine = MACHINE(qdev_get_machine());
> +            MachineClass *mc = MACHINE_GET_CLASS(machine);
> +
> +            if (mc->get_hotplug_handler) {
> +                hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> +                if (hotplug_ctrl) {
> +                    hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
> +                }
> +            }
>          }
>
>          if (qdev_get_vmsd(dev) && local_err == NULL) {
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3567190..67750b5 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -73,6 +73,11 @@ extern MachineState *current_machine;
>  /**
>   * MachineClass:
>   * @qemu_machine: #QEMUMachine
> + * @get_hotplug_handler: this function is called during bus-less
> + *    device hotplug. If defined it returns pointer to an instance
> + *    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.
>   */
>  struct MachineClass {
>      /*< private >*/
> @@ -80,6 +85,9 @@ struct MachineClass {
>      /*< public >*/
>
>      QEMUMachine *qemu_machine;
> +
> +    HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> +                                           DeviceState *dev);
>  };
>
>  /**
> --
> 1.9.0
>
>