On Mon, Apr 07, 2014 at 04:22:06PM +0200, Igor Mammedov wrote:
> On Mon, 7 Apr 2014 16:25:30 +0300
> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>
> > On Mon, Apr 07, 2014 at 03:12:11PM +0200, Igor Mammedov wrote:
> > > On Mon, 7 Apr 2014 15:07:15 +0300
> > > "Michael S. Tsirkin" <m...@redhat.com> wrote:
> > >
> > > > On Mon, Apr 07, 2014 at 02:00:37PM +0200, Igor Mammedov wrote:
> > > > > On Mon, 7 Apr 2014 14:32:41 +0300
> > > > > "Michael S. Tsirkin" <m...@redhat.com> wrote:
> > > > >
> > > > > > On Fri, Apr 04, 2014 at 03:36:48PM +0200, Igor Mammedov wrote:
> > > > > > > ... and report error if plugged in device is not supported.
> > > > > > > Later generic callbacks will be used by memory hotplug.
> > > > > > >
> > > > > > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > > > > >
> > > > > >
> > > > > > OK in that case, how about teaching all hotplug callbacks about
> > > > > > this?
> > > > > >
> > > > > > There are two ATM:
> > > > > > shpc_device_hotplug_cb
> > > > > > pcie_cap_slot_hotplug_cb
> > > > > >
> > > > > > Teach them both to fail gracefully if they get
> > > > > > an object that is not a pci device.
> > > > > >
> > > > > > Afterwards, simply iterate over all objects of type
> > > > > > TYPE_HOTPLUG_HANDLER
> > > > > > and look for one that will accept your object.
> > > > > Then you would never know if any hotplug handler has actually
> > > > > handled event.
> > > >
> > > > Why not? Check the error.
> > > > If no one accepts your object, return error to user.
> > > >
> > > > > I think hotplug handler should return error if unsupported
> > > > > device passed in rather than ignore it. It makes catching
> > > > > wiring errors easier.
> > > >
> > > > Absolutely.
> > > >
> > > > > Dropping error so that we could not care which hotplug handler
> > > > > should be notified, looks like a wrong direction and makes
> > > > > system more fragile.
> > > >
> > > > That's not what I was suggesting.
> > > >
> > > > > It shouldn't be up to consumer to determine that event should
> > > > > be routed to it, but rather by external routing that knows
> > > > > what and when should be notified.
> > > >
> > > > Yes. So
> > > >
> > > > for each hotplug handler (&err)
> > > > handler->plug(device, &err)
> > > > if (!err)
> > > > break;
> > > here it would break on the first handler that doesn't support device
> > > and tells so to caller.
> >
> > This is pseudo-code, I really mean !err == no error reported.
> * what if all handlers returned error, err might not reflect the actual
> error returned from handler that cares about device?
We can use a special error code to mean "hotplug not supported".
> * What if there would be more handlers that could or should handle event
> for device?
That's actually very useful. We could scan top to bottom
so e.g. acpi can intercept bridges.
> * What if only some of compatible handler should handle event
Each one can check whether it's applicable.
> * What if handler should conditionally handle event and only
> caller knows about condition and have access to them?
Doesn't sound possible.
> * What about ordering in which handlers should be called?
>
> Broadcast would be useful if it were impossible to know in advance
> which hotplug handler to use. Is there use case for this?
ACPI vs SHPC would be an example. For that one, we need to order
them top to bottom.
> >
> > > And there isn't any routing here, it just blindly broadcast to every
> > > handler, regardless whether it's right or not.
> >
> > Yes - handlers verify what they can support.
> Sure handlers could verify, there is no harm in extra checking, but
> handlers should not decide what to handle. That's what I'm against from.
> It should be upto caller to decide if handler is the right one and call it.
> There shouldn't be a chance for random/wrong handler to be called.
>
> >
> > > If broadcast should be ever done than it probably should be a part of
> > > DEVICE class and part of
> > > [Qemu-devel] [PATCH 08/35] qdev: hotplug for buss-less devices
> > > patch and be generic to all devices.
> >
> > Not sure what's suggested here.
> above looks like generic code that should be part of Device.realize()
> and should replace 08/35 patch if it's deemed as acceptable.
> I think implementing design like that requires much more though
> if viable and out of scope of this series.
>
> >
> > >
> > > Andreas,
> > > since you care about QDEV
> > > do you have an opinion on ^^^ discussion?
> > >
> > > >
> > > >
> > > > if (err)
> > > > hotplug failed - destroy device
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > > hw/acpi/piix4.c | 31 ++++++++++++++++++++++---------
> > > > > > > 1 file changed, 22 insertions(+), 9 deletions(-)
> > > > > > >
> > > > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > > > > index 67dc075..4341f82 100644
> > > > > > > --- a/hw/acpi/piix4.c
> > > > > > > +++ b/hw/acpi/piix4.c
> > > > > > > @@ -310,19 +310,32 @@ static void piix4_pm_powerdown_req(Notifier
> > > > > > > *n, void *opaque)
> > > > > > > acpi_pm1_evt_power_down(&s->ar);
> > > > > > > }
> > > > > > >
> > > > > > > -static void piix4_pci_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > > > > - DeviceState *dev, Error
> > > > > > > **errp)
> > > > > > > +static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > > > > + DeviceState *dev, Error **errp)
> > > > > > > {
> > > > > > > PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > > > > > > - acpi_pcihp_device_plug_cb(&s->ar, s->irq,
> > > > > > > &s->acpi_pci_hotplug, dev, errp);
> > > > > > > +
> > > > > > > + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > > > > > + acpi_pcihp_device_plug_cb(&s->ar, s->irq,
> > > > > > > &s->acpi_pci_hotplug, dev,
> > > > > > > + errp);
> > > > > > > + } else {
> > > > > > > + error_setg(errp, "acpi: device plug request for not
> > > > > > > supported device"
> > > > > > > + " type: %s",
> > > > > > > object_get_typename(OBJECT(dev)));
> > > > > > > + }
> > > > > > > }
> > > > > > >
> > > > > > > -static void piix4_pci_device_unplug_cb(HotplugHandler
> > > > > > > *hotplug_dev,
> > > > > > > - DeviceState *dev, Error
> > > > > > > **errp)
> > > > > > > +static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
> > > > > > > + DeviceState *dev, Error
> > > > > > > **errp)
> > > > > > > {
> > > > > > > PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > > > > > > - acpi_pcihp_device_unplug_cb(&s->ar, s->irq,
> > > > > > > &s->acpi_pci_hotplug, dev,
> > > > > > > - errp);
> > > > > > > +
> > > > > > > + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > > > > > + acpi_pcihp_device_unplug_cb(&s->ar, s->irq,
> > > > > > > &s->acpi_pci_hotplug, dev,
> > > > > > > + errp);
> > > > > > > + } else {
> > > > > > > + error_setg(errp, "acpi: device unplug request for not
> > > > > > > supported device"
> > > > > > > + " type: %s",
> > > > > > > object_get_typename(OBJECT(dev)));
> > > > > > > + }
> > > > > > > }
> > > > > > >
> > > > > > > static void piix4_update_bus_hotplug(PCIBus *pci_bus, void
> > > > > > > *opaque)
> > > > > > > @@ -553,8 +566,8 @@ static void piix4_pm_class_init(ObjectClass
> > > > > > > *klass, void *data)
> > > > > > > */
> > > > > > > dc->cannot_instantiate_with_device_add_yet = true;
> > > > > > > dc->hotpluggable = false;
> > > > > > > - hc->plug = piix4_pci_device_plug_cb;
> > > > > > > - hc->unplug = piix4_pci_device_unplug_cb;
> > > > > > > + hc->plug = piix4_device_plug_cb;
> > > > > > > + hc->unplug = piix4_device_unplug_cb;
> > > > > > > }
> > > > > > >
> > > > > > > static const TypeInfo piix4_pm_info = {
> > > > > > > --
> > > > > > > 1.9.0
> >
