Re: [Qemu-devel] [PATCH 1/7] define hotplug interface

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 06 Dec 2013 18:26:37 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> > Provide generic hotplug interface for devices.
> > Intended for replacing hotplug mechanism used by
> > PCI/PCIE/SHPC code.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > it's scsi-bus like interface, but abstracted from bus altogether
> > since all current users care about in hotplug handlers, it's
> > hotplug device and hotplugged device and bus only serves
> > as a means to get access to hotplug device and it's callbacks.
> > ---
> >  hw/core/Makefile.objs |    1 +
> >  hw/core/hotplug.c     |   25 ++++++++++++++++++++++++
> >  include/hw/hotplug.h  |   50 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 76 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/core/hotplug.c
> >  create mode 100644 include/hw/hotplug.h
> > 
> > diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> > index 950146c..47f6555 100644
> > --- a/hw/core/Makefile.objs
> > +++ b/hw/core/Makefile.objs
> > @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> >  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> >  common-obj-$(CONFIG_SOFTMMU) += loader.o
> >  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> > +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
> >  
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > new file mode 100644
> > index 0000000..3e84d9c
> > --- /dev/null
> > +++ b/hw/core/hotplug.c
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Hotplug device interface.
> > + *
> > + * Copyright (c) 2013 Red Hat Inc.
> > + *
> > + * Authors:
> > + *  Igor Mammedov <imammedo@redhat.com>,
> > + *
> > + * 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 "hw/hotplug.h"
> > +
> > +static const TypeInfo hotplug_device_info = {
> > +    .name          = TYPE_HOTPLUG_DEVICE,
> 
> Perhaps replace "device" with "handler" throughout?
Marcel and Li suggested s/device/interface/ what do you think about it?

> 
> > +    .parent        = TYPE_INTERFACE,
> > +    .class_size = sizeof(HotplugDeviceClass),
> > +};
> > +
> > +static void hotplug_device_register_types(void)
> > +{
> > +    type_register_static(&hotplug_device_info);
> > +}
> > +
> > +type_init(hotplug_device_register_types)
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > new file mode 100644
> > index 0000000..cfa79bb
> > --- /dev/null
> > +++ b/include/hw/hotplug.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Hotplug device interface.
> > + *
> > + * Copyright (c) 2013 Red Hat Inc.
> > + *
> > + * Authors:
> > + *  Igor Mammedov <imammedo@redhat.com>,
> > + *
> > + * 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 HOTPLUG_H
> > +#define HOTPLUG_H
> > +
> > +#include "hw/qdev-core.h"
> > +
> > +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> > +
> > +#define HOTPLUG_DEVICE_CLASS(klass) \
> > +     OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> > +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
> 
> Missing HOTPLUG_DEVICE macro, see include/hw/stream.h.
Since there were no users for it, I've dropped it. But I can sure add it.

> 
> > +/**
> > + * hotplug_fn:
> > + * @hotplug_dev: a device performing hotplug/uplug action
> > + * @hotplugged_dev: a device that has been hotplugged
> > + * @errp: returns an error if this function fails
> > + */
> > +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
> 
> First argument should be HotplugDevice (which is a struct containing a
> single Object field; again see include/hw/stream.h).
>
> > +                           DeviceState *hotplugged_dev, Error **errp);
> > +
> > +/**
> > + * HotplugDeviceClass:
> > + *
> > + * Interface to be implemented by a device performing
> > + * hardware hotplug/unplug functions.
> > + *
> > + * @parent: Opaque parent interface.
> > + * @hotplug: hotplug callback.
> > + * @hot_unplug: hot unplug callback.
> 
> Just plug/unplug.  IIRC it is used for cold-plug too.
> 
> Otherwise looks great.
> 
> Paolo
> 
> > + */
> > +typedef struct HotplugDeviceClass {
> > +    InterfaceClass parent;
> > +
> > +    hotplug_fn hotplug;
> > +    hotplug_fn hot_unplug;
> > +} HotplugDeviceClass;
> > +
> > +#endif
> > 
>