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

[Igor Mammedov]

On Thu, 17 Apr 2014 09:46:28 +1000
Peter Crosthwaite <address@hidden> wrote:

> 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
It's rather "allow to define at board level" what should be hotplug-handler
than using hardcoded in bus implementation one.

The issue here is to discover which hotplug handler should be used for
a bus-less device. Which MachineClass->get_hotplug_handler(machine, dev);
does. QEMU so far is using bus to solve it, but it by no means is
generic (i.e. applicable only bus-devices).
Proposed bus-less hotplug is a simplified solution that is result of
https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04184.html
discussion.

> 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
Socket object + attached bus is rather workaround due to the lack of
bus-less hotplug than a generic solution.

> generalized to sysbus and a range of devices rather than us having
> independent competing "embedded vs PC" hotplug implementations.
SYSBUS are definitely is no go for hotplug, there were numerous
attempts to make it hotpluggable during past years, which were immediately
rejected. Reasoning in gist was "Sysbus is legacy which shouldn't be used
for anything new". That's one of the reasons why x86 APIC is not
Sysbus device anymore and is attached to ICC bus.

> How do you implement hotplugging to multiple completely independent
> DIMM slots? (i.e. two slots at completely different places in the bus
> heir-achy).
I probably do not understand what is a problem here.
Why plugging bus-less DIMM, one would need to care about buses?

> 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:
v7 of this series was using DIMMBus + DIMMDevice, but afaerber was
pushing towards making DIMM bus-less device as a more generic solution
so that it could be easily reused by other targets.
And it's more flexible in case of PC, considering plan to convert
initial memory to DIMMs where it would involve low and high memory
ranges, proper alignment and placement. It's all very machine specific
and using machine level hotplug-handlers allows to do placement/
checks/mapping cleanly on board level where it belongs and having
completely usable device by the time device becomes "realized".

It's not so with SYSBUS device, I'll comment on your sysbus-memory
series so it would be easy to compare approaches.

> 
> .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
> >
> >
>