Re: [Qemu-devel] [PATCH 23/35] acpi:piix4: make plug/unlug callbacks generic

[Igor Mammedov]

On Mon, 7 Apr 2014 16:25:30 +0300
"Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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" <address@hidden> 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 <address@hidden>
> > > > > 
> > > > > 
> > > > > 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?

 * What if there would be more handlers that could or should handle event
   for device?

 * What if only some of compatible handler should handle event

 * What if handler should conditionally handle event and only 
   caller knows about condition and have access to them?

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

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