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

[Peter Crosthwaite]

On Thu, Apr 17, 2014 at 7:40 PM, Igor Mammedov <address@hidden> wrote:
> 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.
>

If different buses behave different on hotplug then they should be
different busses.

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

Lets divide and conquer this - I agree Sysbus as a bus
(TYPE_SYSTEM_BUS) sucks as we are trying to get rid of busses in favor
of linkages etc.. So long term, SYSTEM_BUS should go away (along with
TYPE_BUS itself).

But TYPE_SYS_BUS_DEVICE should live on. Its a highly useful
abstraction which allows you to define devices with any number of
memory mapped regions. And this device level API doesn't define any
real busisms so it should be possible to convert SYS_BUS_DEVICE to
this "bussless" regime you are proposing anyway. Infact I do wonder
exactly how hard it would be to patch one of your handlers to just
call into the sysbus API and grab the memory regions and attach as you
already do for DIMMs. Then you work works for all of sysbus and we can
continue doing our embedded thing which some decent code sharing.

Regards,
Peter

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