Message ID | 1396618620-27823-24-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
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 <imammedo@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. > --- > 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
On Mon, 7 Apr 2014 14:32:41 +0300 "Michael S. Tsirkin" <mst@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 <imammedo@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. I think hotplug handler should return error if unsupported device passed in rather than ignore it. It makes catching wiring errors easier. Dropping error so that we could not care which hotplug handler should be notified, looks like a wrong direction and makes system more fragile. 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. > > > > --- > > 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
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" <mst@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 <imammedo@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; 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
On Mon, 7 Apr 2014 15:07:15 +0300 "Michael S. Tsirkin" <mst@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" <mst@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 <imammedo@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. And there isn't any routing here, it just blindly broadcast to every handler, regardless whether it's right or not. 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. 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
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" <mst@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" <mst@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 <imammedo@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. > 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. > 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. > > 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
On Mon, 7 Apr 2014 16:25:30 +0300 "Michael S. Tsirkin" <mst@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" <mst@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" <mst@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 <imammedo@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? * 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 >
On Mon, Apr 07, 2014 at 04:25:30PM +0300, Michael S. Tsirkin 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" <mst@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" <mst@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 <imammedo@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. > > > 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. > > > 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. > > > > > Andreas, > > since you care about QDEV > > do you have an opinion on ^^^ discussion? Actually, there's one useful case that this would help address. At the moment ACPI code has to poke at SHPC bridges and modify their hotplug callbacks in order to get notified of hotplug events. This means we can't cleanly implement an option for guest to disable ACPI and switch to native if supported, like the ACPI spec allows. If we change hotplug code to walk the tree top down and invoke all callbacks, then it will be fixed in a cleaner way: bridges would just do: if (dev->bus != self) { set_error return; } and suddently pci host can trap callbacks and redirect to acpi if it wants to. > > > > > > > > > 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
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" <mst@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" <mst@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" <mst@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 <imammedo@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 > >
On Mon, 7 Apr 2014 18:36:34 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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" <mst@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" <mst@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" <mst@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 <imammedo@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". That would start special casing some error codes. With explicit "routing table" it won't be necessary. > > > * 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. That would imply specific ordering in event propagation, which in case of broadcast is not guarantied. And without explicit "routing table" it could easily break if propagation order changes. > > > * What if only some of compatible handler should handle event > > Each one can check whether it's applicable. handler might need to access some device internals to do so, it means that it might to pull in target dependent headers in its implementation. > > > * What if handler should conditionally handle event and only > > caller knows about condition and have access to them? > > Doesn't sound possible. If it's not handler who decides whether to handle/receive event or not, it's quite 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. To sum-up above said, I'm not convinced that implementing generic hotplug event broadcast is what is needed. You are trying to fix with it a very specific use-case, which might not need it at all. Instead of broadcast, if there is a need the hardcoded event routing should be replaced with data driven approach (like VMSD) with explicit routing tables which describes where and when to forward hotplug event. That would make sure that event won't be mis-routed accidentally and no need to special case routing decisions on error code. > > > > > > > > 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 > > > >
Il 07/04/2014 11:19, Michael S. Tsirkin ha scritto: > This means we can't cleanly implement an option for guest to > disable ACPI and switch to native if supported, > like the ACPI spec allows. > > If we change hotplug code to walk the tree top down > and invoke all callbacks, then it will be fixed > in a cleaner way: bridges would just do: > > if (dev->bus != self) { > set_error > return; > } > > and suddently pci host can trap callbacks and redirect > to acpi if it wants to. I think this should be handled by making the q35 PCI host bridge implement HotplugHandler itself, possibly overriding the parent's implementation. Paolo
On Tue, Apr 29, 2014 at 10:12:34AM +0200, Paolo Bonzini wrote: > Il 07/04/2014 17:36, Michael S. Tsirkin ha scritto: > >>> * 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. > > > > Be careful about "top to bottom". Does top-to-bottom mean by bus or > by composition? This has been a huge can of worms in the > discussions about recursive realization. > > One thing we could do is to always go through /machine even before > invoking the bus handler. Then /machine can do machine-specific > checks to interpose the PCI host bridge and/or the ACPI device's > hotplug handlers. > > But this can be done on top of this series, it has nothing to do > with memory hotplug and this one is already big enough! > > Paolo Yes, new functionality does not need to be added in this patchset. But to repeat what I was saying, this check (that object passed in is a PCI DEVICE) belongs in acpi_pcihp_device_plug_cb and should not be piix specific, I don't want to duplicate this logic in q35 later. Similar checks should be added in shpc_device_hotplug_cb pcie_cap_slot_hotplug_cb for consistency.
On Fri, Apr 11, 2014 at 09:40:19PM -0400, Paolo Bonzini wrote: > Il 07/04/2014 11:19, Michael S. Tsirkin ha scritto: > >This means we can't cleanly implement an option for guest to > >disable ACPI and switch to native if supported, > >like the ACPI spec allows. > > > >If we change hotplug code to walk the tree top down > >and invoke all callbacks, then it will be fixed > >in a cleaner way: bridges would just do: > > > > if (dev->bus != self) { > > set_error > > return; > > } > > > >and suddently pci host can trap callbacks and redirect > >to acpi if it wants to. > > I think this should be handled by making the q35 PCI host bridge > implement HotplugHandler itself, possibly overriding the parent's > implementation. > > Paolo Yes but it needs to be dynamic, i.e. q35 should be able to say at runtime "I can't handle this hotplug, pass it up to parent" rather than just checking callback is NULL.
Il 07/04/2014 17:36, Michael S. Tsirkin ha scritto: >> > * 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. > Be careful about "top to bottom". Does top-to-bottom mean by bus or by composition? This has been a huge can of worms in the discussions about recursive realization. One thing we could do is to always go through /machine even before invoking the bus handler. Then /machine can do machine-specific checks to interpose the PCI host bridge and/or the ACPI device's hotplug handlers. But this can be done on top of this series, it has nothing to do with memory hotplug and this one is already big enough! Paolo
Il 29/04/2014 09:25, Michael S. Tsirkin ha scritto: > But to repeat what I was saying, this check (that object passed in > is a PCI DEVICE) belongs in acpi_pcihp_device_plug_cb and should not be > piix specific, I don't want to duplicate this logic in q35 later. > Similar checks should be added in > shpc_device_hotplug_cb pcie_cap_slot_hotplug_cb for consistency. I disagree. In fact, I think the opposite is true: the three functions you mention should take a PCIDevice*, and when this is done acpi_memory_plug_cb should be changed to take a DIMMDevice* too. Paolo
On Tue, Apr 29, 2014 at 10:49:16AM +0200, Paolo Bonzini wrote: > Il 29/04/2014 09:25, Michael S. Tsirkin ha scritto: > >But to repeat what I was saying, this check (that object passed in > >is a PCI DEVICE) belongs in acpi_pcihp_device_plug_cb and should not be > >piix specific, I don't want to duplicate this logic in q35 later. > >Similar checks should be added in > >shpc_device_hotplug_cb pcie_cap_slot_hotplug_cb for consistency. > > I disagree. In fact, I think the opposite is true: the three > functions you mention should take a PCIDevice*, and when this is > done acpi_memory_plug_cb should be changed to take a DIMMDevice* > too. > > Paolo Well this just means we'll need another chipset-independent wrapper to detect the correct hotplug type?
Il 29/04/2014 11:44, Michael S. Tsirkin ha scritto: > > I disagree. In fact, I think the opposite is true: the three > > functions you mention should take a PCIDevice*, and when this is > > done acpi_memory_plug_cb should be changed to take a DIMMDevice* > > too. > > Well this just means we'll need another chipset-independent wrapper > to detect the correct hotplug type? Yes, we could do that. Calling the wrapper is equivalent to the "broadcast" concept you mentioned, though of course the implementations differ. Paolo
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 = {
... and report error if plugged in device is not supported. Later generic callbacks will be used by memory hotplug. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/acpi/piix4.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)