| Message ID | 1386349395-5710-8-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------ > include/hw/pci/pci.h | 10 ------ > include/hw/pci/pci_bus.h | 2 - > 3 files changed, 51 insertions(+), 31 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 49eca95..26229de 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -35,6 +35,7 @@ > #include "hw/pci/msi.h" > #include "hw/pci/msix.h" > #include "exec/address-spaces.h" > +#include "hw/hotplug.h" > > //#define DEBUG_PCI > #ifdef DEBUG_PCI > @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0])); > } > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev) > -{ > - bus->qbus.allow_hotplug = 1; > - bus->hotplug = hotplug; > - bus->hotplug_qdev = qdev; > -} > - > PCIBus *pci_register_bus(DeviceState *parent, const char *name, > pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > void *irq_opaque, > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev) > { > PCIDevice *pci_dev = (PCIDevice *)qdev; > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev); > + DeviceState *hotplug_dev; > + Error *local_err = NULL; > PCIBus *bus; > int rc; > bool is_default_rom; > @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev) > } > pci_add_option_rom(pci_dev, is_default_rom); > > - if (bus->hotplug) { > - /* Let buses differentiate between hotplug and when device is > - * enabled during qemu machine creation. */ > - rc = bus->hotplug(bus->hotplug_qdev, pci_dev, > - qdev->hotplugged ? PCI_HOTPLUG_ENABLED: > - PCI_COLDPLUG_ENABLED); > - if (rc != 0) { > - int r = pci_unregister_device(&pci_dev->qdev); > - assert(!r); > - return rc; > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > + &local_err)); > + if (error_is_set(&local_err)) { > + goto error_exit; > + } > + if (hotplug_dev) { It seems that object_property_get_link never returns NULL if no error, meaning that if you go for this link you *must* be sure that the link exists. If the above is the case, I think you can loose the "if" statement. Otherwise, if NULL is an option, you would abort the device init unnecessary. > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > + > + /* handler can differentiate between hotplug and when device is > + * enabled during qemu machine creation by inspecting > + * dev->hotplugged field. */ > + if (hdc->hotplug) { > + hdc->hotplug(hotplug_dev, qdev, &local_err); > + if (error_is_set(&local_err)) { > + int r = pci_unregister_device(&pci_dev->qdev); > + assert(!r); > + goto error_exit; > + } > } > } > return 0; > + > +error_exit: > + qerror_report_err(local_err); > + error_free(local_err); > + return -1; > } > > static int pci_unplug_device(DeviceState *qdev) > { > PCIDevice *dev = PCI_DEVICE(qdev); > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > + BusState *bus = qdev_get_parent_bus(qdev); > + DeviceState *hotplug_dev; > + Error *local_err = NULL; > > if (pc->no_hotplug) { > qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev))); > return -1; > } > - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, > - PCI_HOTPLUG_DISABLED); > + > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > + &local_err)); > + if (error_is_set(&local_err)) { > + goto error_exit; > + } > + if (hotplug_dev) Same as above, I hope it helped, Thanks, Marcel > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > + > + if (hdc->hot_unplug) { > + hdc->hot_unplug(hotplug_dev, qdev, &local_err); > + if (error_is_set(&local_err)) { > + goto error_exit; > + } > + } > + } > + return 0; > + > +error_exit: > + qerror_report_err(local_err); > + error_free(local_err); > + return -1; > } > > PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction, > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index b783e68..33dec40 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); > typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); > > -typedef enum { > - PCI_HOTPLUG_DISABLED, > - PCI_HOTPLUG_ENABLED, > - PCI_COLDPLUG_ENABLED, > -} PCIHotplugState; > - > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, > - PCIHotplugState state); > - > #define TYPE_PCI_BUS "PCI" > #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS) > #define TYPE_PCIE_BUS "PCIE" > @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name, > void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > void *irq_opaque, int nirq); > int pci_bus_get_irq_level(PCIBus *bus, int irq_num); > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev); > /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */ > int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin); > PCIBus *pci_register_bus(DeviceState *parent, const char *name, > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h > index 9df1788..fabaeee 100644 > --- a/include/hw/pci/pci_bus.h > +++ b/include/hw/pci/pci_bus.h > @@ -16,8 +16,6 @@ struct PCIBus { > pci_set_irq_fn set_irq; > pci_map_irq_fn map_irq; > pci_route_irq_fn route_intx_to_irq; > - pci_hotplug_fn hotplug; > - DeviceState *hotplug_qdev; > void *irq_opaque; > PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX]; > PCIDevice *parent_dev;
On Fri, 06 Dec 2013 18:29:58 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 06/12/2013 18:03, Igor Mammedov ha scritto: > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > > + &local_err)); > > + if (error_is_set(&local_err)) { > > + goto error_exit; > > + } > > + if (hotplug_dev) { > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > > + > > + /* handler can differentiate between hotplug and when device is > > + * enabled during qemu machine creation by inspecting > > + * dev->hotplugged field. */ > > + if (hdc->hotplug) { > > + hdc->hotplug(hotplug_dev, qdev, &local_err); > > + if (error_is_set(&local_err)) { > > + int r = pci_unregister_device(&pci_dev->qdev); > > + assert(!r); > > + goto error_exit; > > + } > > } > > } > > > + > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > > + &local_err)); > > + if (error_is_set(&local_err)) { > > + goto error_exit; > > + } > > + if (hotplug_dev) { > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > > + > > + if (hdc->hot_unplug) { > > + hdc->hot_unplug(hotplug_dev, qdev, &local_err); > > + if (error_is_set(&local_err)) { > > + goto error_exit; > > + } > > + } > > + } > > Please move the parts under the "if" to hotplug.c (something like > hotplug_handler_plug and hotplug_handler_unplug). Also, should this be sure. > moved up to generic code (e.g. bus_add_child/bus_remove_child)? Current usage hints that it's more realize() related part. If handler fails device might want to do same device specific failure handling before returning from realize() with failure. It will work in bus_add_child/bus_remove_child as well but it would basically put handlers to nofail category. > > A NULL link can be a no-op for coldplugged devices, an error for > hotplugged devices, and an assertion failure for hot-unplug. > > Paolo >
Il 09/12/2013 14:41, Igor Mammedov ha scritto: >> > Please move the parts under the "if" to hotplug.c (something like >> > hotplug_handler_plug and hotplug_handler_unplug). Also, should this be > sure. > >> > moved up to generic code (e.g. bus_add_child/bus_remove_child)? > Current usage hints that it's more realize() related part. If handler > fails device might want to do same device specific failure handling > before returning from realize() with failure. > It will work in bus_add_child/bus_remove_child as well but it would > basically put handlers to nofail category. Makes sense. realize() for the "plug" handler, and qdev_unplug for the unplug handler, I guess. Paolo
On Mon, 09 Dec 2013 11:09:02 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------ > > include/hw/pci/pci.h | 10 ------ > > include/hw/pci/pci_bus.h | 2 - > > 3 files changed, 51 insertions(+), 31 deletions(-) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 49eca95..26229de 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -35,6 +35,7 @@ > > #include "hw/pci/msi.h" > > #include "hw/pci/msix.h" > > #include "exec/address-spaces.h" > > +#include "hw/hotplug.h" > > > > //#define DEBUG_PCI > > #ifdef DEBUG_PCI > > @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0])); > > } > > > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev) > > -{ > > - bus->qbus.allow_hotplug = 1; > > - bus->hotplug = hotplug; > > - bus->hotplug_qdev = qdev; > > -} > > - > > PCIBus *pci_register_bus(DeviceState *parent, const char *name, > > pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > void *irq_opaque, > > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev) > > { > > PCIDevice *pci_dev = (PCIDevice *)qdev; > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev); > > + DeviceState *hotplug_dev; > > + Error *local_err = NULL; > > PCIBus *bus; > > int rc; > > bool is_default_rom; > > @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev) > > } > > pci_add_option_rom(pci_dev, is_default_rom); > > > > - if (bus->hotplug) { > > - /* Let buses differentiate between hotplug and when device is > > - * enabled during qemu machine creation. */ > > - rc = bus->hotplug(bus->hotplug_qdev, pci_dev, > > - qdev->hotplugged ? PCI_HOTPLUG_ENABLED: > > - PCI_COLDPLUG_ENABLED); > > - if (rc != 0) { > > - int r = pci_unregister_device(&pci_dev->qdev); > > - assert(!r); > > - return rc; > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > > + &local_err)); > > + if (error_is_set(&local_err)) { > > + goto error_exit; > > + } > > + if (hotplug_dev) { > It seems that object_property_get_link never returns NULL > if no error, meaning that if you go for this link > you *must* be sure that the link exists. > If the above is the case, I think you can loose the "if" statement. > Otherwise, if NULL is an option, you would abort the device init unnecessary. Link property might exists but it might be not set. in that case get_link returns NULL without error set. So if nobody bothered to set link, it would mean skip hotplug section (it's not fail). > > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > > + > > + /* handler can differentiate between hotplug and when device is > > + * enabled during qemu machine creation by inspecting > > + * dev->hotplugged field. */ > > + if (hdc->hotplug) { > > + hdc->hotplug(hotplug_dev, qdev, &local_err); > > + if (error_is_set(&local_err)) { > > + int r = pci_unregister_device(&pci_dev->qdev); > > + assert(!r); > > + goto error_exit; > > + } > > } > > } > > return 0; > > + > > +error_exit: > > + qerror_report_err(local_err); > > + error_free(local_err); > > + return -1; > > } > > > > static int pci_unplug_device(DeviceState *qdev) > > { > > PCIDevice *dev = PCI_DEVICE(qdev); > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > > + BusState *bus = qdev_get_parent_bus(qdev); > > + DeviceState *hotplug_dev; > > + Error *local_err = NULL; > > > > if (pc->no_hotplug) { > > qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev))); > > return -1; > > } > > - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, > > - PCI_HOTPLUG_DISABLED); > > + > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > > + &local_err)); > > + if (error_is_set(&local_err)) { > > + goto error_exit; > > + } > > + if (hotplug_dev) > Same as above, > > I hope it helped, > Thanks, > Marcel > > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > > + > > + if (hdc->hot_unplug) { > > + hdc->hot_unplug(hotplug_dev, qdev, &local_err); > > + if (error_is_set(&local_err)) { > > + goto error_exit; > > + } > > + } > > + } > > + return 0; > > + > > +error_exit: > > + qerror_report_err(local_err); > > + error_free(local_err); > > + return -1; > > } > > > > PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction, > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > > index b783e68..33dec40 100644 > > --- a/include/hw/pci/pci.h > > +++ b/include/hw/pci/pci.h > > @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); > > typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > > typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); > > > > -typedef enum { > > - PCI_HOTPLUG_DISABLED, > > - PCI_HOTPLUG_ENABLED, > > - PCI_COLDPLUG_ENABLED, > > -} PCIHotplugState; > > - > > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, > > - PCIHotplugState state); > > - > > #define TYPE_PCI_BUS "PCI" > > #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS) > > #define TYPE_PCIE_BUS "PCIE" > > @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name, > > void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > void *irq_opaque, int nirq); > > int pci_bus_get_irq_level(PCIBus *bus, int irq_num); > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev); > > /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */ > > int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin); > > PCIBus *pci_register_bus(DeviceState *parent, const char *name, > > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h > > index 9df1788..fabaeee 100644 > > --- a/include/hw/pci/pci_bus.h > > +++ b/include/hw/pci/pci_bus.h > > @@ -16,8 +16,6 @@ struct PCIBus { > > pci_set_irq_fn set_irq; > > pci_map_irq_fn map_irq; > > pci_route_irq_fn route_intx_to_irq; > > - pci_hotplug_fn hotplug; > > - DeviceState *hotplug_qdev; > > void *irq_opaque; > > PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX]; > > PCIDevice *parent_dev; > > >
On Mon, 2013-12-09 at 14:55 +0100, Igor Mammedov wrote: > On Mon, 09 Dec 2013 11:09:02 +0200 > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------ > > > include/hw/pci/pci.h | 10 ------ > > > include/hw/pci/pci_bus.h | 2 - > > > 3 files changed, 51 insertions(+), 31 deletions(-) > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > index 49eca95..26229de 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -35,6 +35,7 @@ > > > #include "hw/pci/msi.h" > > > #include "hw/pci/msix.h" > > > #include "exec/address-spaces.h" > > > +#include "hw/hotplug.h" > > > > > > //#define DEBUG_PCI > > > #ifdef DEBUG_PCI > > > @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > > bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0])); > > > } > > > > > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev) > > > -{ > > > - bus->qbus.allow_hotplug = 1; > > > - bus->hotplug = hotplug; > > > - bus->hotplug_qdev = qdev; > > > -} > > > - > > > PCIBus *pci_register_bus(DeviceState *parent, const char *name, > > > pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > > void *irq_opaque, > > > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev) > > > { > > > PCIDevice *pci_dev = (PCIDevice *)qdev; > > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev); > > > + DeviceState *hotplug_dev; > > > + Error *local_err = NULL; > > > PCIBus *bus; > > > int rc; > > > bool is_default_rom; > > > @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev) > > > } > > > pci_add_option_rom(pci_dev, is_default_rom); > > > > > > - if (bus->hotplug) { > > > - /* Let buses differentiate between hotplug and when device is > > > - * enabled during qemu machine creation. */ > > > - rc = bus->hotplug(bus->hotplug_qdev, pci_dev, > > > - qdev->hotplugged ? PCI_HOTPLUG_ENABLED: > > > - PCI_COLDPLUG_ENABLED); > > > - if (rc != 0) { > > > - int r = pci_unregister_device(&pci_dev->qdev); > > > - assert(!r); > > > - return rc; > > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > > > + &local_err)); > > > + if (error_is_set(&local_err)) { > > > + goto error_exit; > > > + } > > > + if (hotplug_dev) { > > It seems that object_property_get_link never returns NULL > > if no error, meaning that if you go for this link > > you *must* be sure that the link exists. > > If the above is the case, I think you can loose the "if" statement. > > Otherwise, if NULL is an option, you would abort the device init unnecessary. > Link property might exists but it might be not set. in that case get_link > returns NULL without error set. So if nobody bothered to set link, it would mean > skip hotplug section (it's not fail). Got it, thanks! Marcel > > > > > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > > > + > > > + /* handler can differentiate between hotplug and when device is > > > + * enabled during qemu machine creation by inspecting > > > + * dev->hotplugged field. */ > > > + if (hdc->hotplug) { > > > + hdc->hotplug(hotplug_dev, qdev, &local_err); > > > + if (error_is_set(&local_err)) { > > > + int r = pci_unregister_device(&pci_dev->qdev); > > > + assert(!r); > > > + goto error_exit; > > > + } > > > } > > > } > > > return 0; > > > + > > > +error_exit: > > > + qerror_report_err(local_err); > > > + error_free(local_err); > > > + return -1; > > > } > > > > > > static int pci_unplug_device(DeviceState *qdev) > > > { > > > PCIDevice *dev = PCI_DEVICE(qdev); > > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); > > > + BusState *bus = qdev_get_parent_bus(qdev); > > > + DeviceState *hotplug_dev; > > > + Error *local_err = NULL; > > > > > > if (pc->no_hotplug) { > > > qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev))); > > > return -1; > > > } > > > - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, > > > - PCI_HOTPLUG_DISABLED); > > > + > > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", > > > + &local_err)); > > > + if (error_is_set(&local_err)) { > > > + goto error_exit; > > > + } > > > + if (hotplug_dev) > > Same as above, > > > > I hope it helped, > > Thanks, > > Marcel > > > > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); > > > + > > > + if (hdc->hot_unplug) { > > > + hdc->hot_unplug(hotplug_dev, qdev, &local_err); > > > + if (error_is_set(&local_err)) { > > > + goto error_exit; > > > + } > > > + } > > > + } > > > + return 0; > > > + > > > +error_exit: > > > + qerror_report_err(local_err); > > > + error_free(local_err); > > > + return -1; > > > } > > > > > > PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction, > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > > > index b783e68..33dec40 100644 > > > --- a/include/hw/pci/pci.h > > > +++ b/include/hw/pci/pci.h > > > @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); > > > typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > > > typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); > > > > > > -typedef enum { > > > - PCI_HOTPLUG_DISABLED, > > > - PCI_HOTPLUG_ENABLED, > > > - PCI_COLDPLUG_ENABLED, > > > -} PCIHotplugState; > > > - > > > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, > > > - PCIHotplugState state); > > > - > > > #define TYPE_PCI_BUS "PCI" > > > #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS) > > > #define TYPE_PCIE_BUS "PCIE" > > > @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name, > > > void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, > > > void *irq_opaque, int nirq); > > > int pci_bus_get_irq_level(PCIBus *bus, int irq_num); > > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev); > > > /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */ > > > int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin); > > > PCIBus *pci_register_bus(DeviceState *parent, const char *name, > > > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h > > > index 9df1788..fabaeee 100644 > > > --- a/include/hw/pci/pci_bus.h > > > +++ b/include/hw/pci/pci_bus.h > > > @@ -16,8 +16,6 @@ struct PCIBus { > > > pci_set_irq_fn set_irq; > > > pci_map_irq_fn map_irq; > > > pci_route_irq_fn route_intx_to_irq; > > > - pci_hotplug_fn hotplug; > > > - DeviceState *hotplug_qdev; > > > void *irq_opaque; > > > PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX]; > > > PCIDevice *parent_dev; > > > > > > >
On Mon, 09 Dec 2013 14:47:00 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 09/12/2013 14:41, Igor Mammedov ha scritto: > >> > Please move the parts under the "if" to hotplug.c (something like > >> > hotplug_handler_plug and hotplug_handler_unplug). Also, should this be > > sure. > > > >> > moved up to generic code (e.g. bus_add_child/bus_remove_child)? > > Current usage hints that it's more realize() related part. If handler > > fails device might want to do same device specific failure handling > > before returning from realize() with failure. > > It will work in bus_add_child/bus_remove_child as well but it would > > basically put handlers to nofail category. > > Makes sense. realize() for the "plug" handler, and qdev_unplug for the > unplug handler, I guess. Just to be sure, I've meant not DEVICE.realize() but each device specific one. qdev_unplug() might work for now, but I haven't checked all devices that might use interface and if it would break anything. Ideally it should be in device's unrealize() complementing realize() part. I'd wait till all buses converted to new interface before attempting to generalize current plug/unplug call pathes though. > > Paolo >
Il 09/12/2013 15:14, Igor Mammedov ha scritto: >> > >> > Makes sense. realize() for the "plug" handler, and qdev_unplug for the >> > unplug handler, I guess. > Just to be sure, I've meant not DEVICE.realize() but each device specific > one. If it's each specific device, then why should the hotplug handler link be in DeviceState? I think it should be in device_set_realized. > qdev_unplug() might work for now, but I haven't checked all devices that > might use interface and if it would break anything. Ideally it should be > in device's unrealize() complementing realize() part. > > I'd wait till all buses converted to new interface before attempting to > generalize current plug/unplug call pathes though. I agree that adding a default behavior for no link probably requires conversion of all buses. However, looking for the link in the generic code can be done now. Paolo
On Mon, 09 Dec 2013 15:36:35 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 09/12/2013 15:14, Igor Mammedov ha scritto: > >> > > >> > Makes sense. realize() for the "plug" handler, and qdev_unplug for the > >> > unplug handler, I guess. > > Just to be sure, I've meant not DEVICE.realize() but each device specific > > one. > > If it's each specific device, then why should the hotplug handler link > be in DeviceState? The reason I've put it there is to eventually replace allow_hotplug field with it, it also reduces code duplication (i.e. we wont' have to add it in PCIDevice, DimmDevice ...) and potentially allows to use NULL for error in property lookup since each bus will have it. > > I think it should be in device_set_realized. if we dub it nofail then it's fine, otherwise failure path becomes more complicated. Calling handler in specific device realize() allows to gracefully abort realize() since that device knows what needs to be done to do so: For example: @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev) ... + hdc->hotplug(hotplug_dev, qdev, &local_err); + if (error_is_set(&local_err)) { + int r = pci_unregister_device(&pci_dev->qdev); Calling handler in realize will not allow to do it. It's just much more flexible to call handler from specific device since it knows when it's the best to call handler and how to deal with failure. > > > qdev_unplug() might work for now, but I haven't checked all devices that > > might use interface and if it would break anything. Ideally it should be > > in device's unrealize() complementing realize() part. > > > > I'd wait till all buses converted to new interface before attempting to > > generalize current plug/unplug call pathes though. > > I agree that adding a default behavior for no link probably requires > conversion of all buses. However, looking for the link in the generic > code can be done now. > > Paolo >
Il 09/12/2013 16:08, Igor Mammedov ha scritto: > On Mon, 09 Dec 2013 15:36:35 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Il 09/12/2013 15:14, Igor Mammedov ha scritto: >>>>> >>>>> Makes sense. realize() for the "plug" handler, and qdev_unplug for the >>>>> unplug handler, I guess. >>> Just to be sure, I've meant not DEVICE.realize() but each device specific >>> one. >> >> If it's each specific device, then why should the hotplug handler link >> be in DeviceState? > The reason I've put it there is to eventually replace allow_hotplug field with it, > it also reduces code duplication (i.e. we wont' have to add it in PCIDevice, > DimmDevice ...) and potentially allows to use NULL for error in > property lookup since each bus will have it. I agree that's the right thing to do. >> I think it should be in device_set_realized. > if we dub it nofail then it's fine, otherwise failure path becomes more complicated. > > Calling handler in specific device realize() allows to gracefully abort > realize() since that device knows what needs to be done to do so: > For example: > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev) > ... > + hdc->hotplug(hotplug_dev, qdev, &local_err); > + if (error_is_set(&local_err)) { > + int r = pci_unregister_device(&pci_dev->qdev); > > Calling handler in realize will not allow to do it. > It's just much more flexible to call handler from specific device since it knows > when it's the best to call handler and how to deal with failure. We could have separate check/plug methods. Only check can fail, it must be idempotent, and it can be invoked while the device is still unrealized. The reason I liked the interface, is because it removes the need for each bus to add its own plug/unplug handling. Paolo >>> qdev_unplug() might work for now, but I haven't checked all devices that >>> might use interface and if it would break anything. Ideally it should be >>> in device's unrealize() complementing realize() part. >>> >>> I'd wait till all buses converted to new interface before attempting to >>> generalize current plug/unplug call pathes though. >> >> I agree that adding a default behavior for no link probably requires >> conversion of all buses. However, looking for the link in the generic >> code can be done now. >> >> Paolo >> >
On Mon, 09 Dec 2013 16:16:57 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 09/12/2013 16:08, Igor Mammedov ha scritto: > > On Mon, 09 Dec 2013 15:36:35 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> Il 09/12/2013 15:14, Igor Mammedov ha scritto: > >>>>> > >>>>> Makes sense. realize() for the "plug" handler, and qdev_unplug for the > >>>>> unplug handler, I guess. > >>> Just to be sure, I've meant not DEVICE.realize() but each device specific > >>> one. > >> > >> If it's each specific device, then why should the hotplug handler link > >> be in DeviceState? > > The reason I've put it there is to eventually replace allow_hotplug field with it, > > it also reduces code duplication (i.e. we wont' have to add it in PCIDevice, > > DimmDevice ...) and potentially allows to use NULL for error in > > property lookup since each bus will have it. > > I agree that's the right thing to do. > > >> I think it should be in device_set_realized. > > if we dub it nofail then it's fine, otherwise failure path becomes more complicated. > > > > Calling handler in specific device realize() allows to gracefully abort > > realize() since that device knows what needs to be done to do so: > > For example: > > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev) > > ... > > + hdc->hotplug(hotplug_dev, qdev, &local_err); > > + if (error_is_set(&local_err)) { > > + int r = pci_unregister_device(&pci_dev->qdev); > > > > Calling handler in realize will not allow to do it. > > It's just much more flexible to call handler from specific device since it knows > > when it's the best to call handler and how to deal with failure. > > We could have separate check/plug methods. Only check can fail, it must > be idempotent, and it can be invoked while the device is still unrealized. Reasons I've stated before apply to 'check' as well, for only specific device knows when it's fine to call it. That would just replace "plug" with "check" in realize() for not much of benefit. > > The reason I liked the interface, is because it removes the need for > each bus to add its own plug/unplug handling. ^^^ Have you meant device instead of bus? In PCI code it's a PCIDevice who calls hotplug handling, PCI BUS serves only as a source for plug/upnlug handlers and hotplug_qdev. This series moves hotplug_qdev to BusState so it could be reused by other buses and removes from bus not belonging to it plug/unplug callbacks. It's still improvement over current PCI hotplug code and allows to simplify other buses as well by removing callbacks from them. The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if we make them all nofail, then it would be safe to call them in "realize" property setter. But we have at least 2 callbacks that can fail: pcie_cap_slot_hotplug() and shpc_device_hotplug() maybe more. I'm not an expert on both of them to tell for sure if they could be made nofail. We can try do it, but it could perfectly apply on top of this series and could be made later. Goal of this series was to add and demonstrate reusable hotplug interface as opposed to PCI specific or SCSI-bus specific ones, so it could be used for memory hotplug as well. It might not do everything we would like but it looks like a move the right direction. If it's wrong direction, I could drop the idea and fallback to an original less intrusive approach, taking in account your comment to move type definitions into separate header. > > Paolo > > >>> qdev_unplug() might work for now, but I haven't checked all devices that > >>> might use interface and if it would break anything. Ideally it should be > >>> in device's unrealize() complementing realize() part. > >>> > >>> I'd wait till all buses converted to new interface before attempting to > >>> generalize current plug/unplug call pathes though. > >> > >> I agree that adding a default behavior for no link probably requires > >> conversion of all buses. However, looking for the link in the generic > >> code can be done now. > >> > >> Paolo > >> > > >
Il 09/12/2013 17:48, Igor Mammedov ha scritto: >> > >> > We could have separate check/plug methods. Only check can fail, it must >> > be idempotent, and it can be invoked while the device is still unrealized. > Reasons I've stated before apply to 'check' as well, for only specific device knows > when it's fine to call it. That would just replace "plug" with "check" in realize() > for not much of benefit. Check is idempotent, and can be called before realize makes any change (it could also be called after the device is added to /machine/unattached, it's not a big difference). Plug is called after realize. >> > >> > The reason I liked the interface, is because it removes the need for >> > each bus to add its own plug/unplug handling. > ^^^ > Have you meant device instead of bus? I meant each bus-specific abstract class (PCIDevice, SCSIDevice, etc.). > It's still improvement over current PCI hotplug code and allows to simplify > other buses as well by removing callbacks from them. Exactly. But so far you don't get any benefit: no removal of PCI hotplug code, no removal of allow_hotunplug. What I'm proposing, I believe, has a small cost and already starts the transition (which I believe we can complete for 2.0). > The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if we make > them all nofail, then it would be safe to call them in "realize" property setter. > But we have at least 2 callbacks that can fail: > pcie_cap_slot_hotplug() and shpc_device_hotplug() Both of them can be handled by a "check" step in the handler. > Goal of this series was to add and demonstrate reusable hotplug interface as > opposed to PCI specific or SCSI-bus specific ones, so it could be used for memory > hotplug as well. It might not do everything we would like but it looks like a move > the right direction. > If it's wrong direction, I could drop the idea and fallback to an original > less intrusive approach, taking in account your comment to move type definitions > into separate header. No, absolutely. I think it's the right direction, I just think more aspects of it should be made generic. Paolo
On Mon, 09 Dec 2013 18:18:42 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 09/12/2013 17:48, Igor Mammedov ha scritto: > >> > > >> > We could have separate check/plug methods. Only check can fail, it must > >> > be idempotent, and it can be invoked while the device is still unrealized. > > Reasons I've stated before apply to 'check' as well, for only specific device knows > > when it's fine to call it. That would just replace "plug" with "check" in realize() > > for not much of benefit. > > Check is idempotent, and can be called before realize makes any change > (it could also be called after the device is added to > /machine/unattached, it's not a big difference). > > Plug is called after realize. PCIE case: "check" before realize will work since code that does check depends only on hotplug device (i.e. PCIE slot) and do not access not yet realized device at all. however SHPC case: check code access pci_slot that is derived from PCIDevice.devfn, which in turn could be initialized in realize() (see pci_qdev_init() devfn auto allocation). So it's not possible to call check before realize() it should be called from realize(). Perhaps other hotplug buses/devices have similar limitations, where it's not fine to access device state from outside before calling it's realize(), so it should be some post_realize() hook then to make it generic which leads to the following: if ->plug() called after realize() fails, all we need to do is to fail "realize" property setter. That should cause qdev_device_add() -> object_unparent() -> device_unparent() -> unrealize() doing all necessary cleanup. > > >> > > >> > The reason I liked the interface, is because it removes the need for > >> > each bus to add its own plug/unplug handling. > > ^^^ > > Have you meant device instead of bus? > > I meant each bus-specific abstract class (PCIDevice, SCSIDevice, etc.). > > > It's still improvement over current PCI hotplug code and allows to simplify > > other buses as well by removing callbacks from them. > > Exactly. But so far you don't get any benefit: no removal of PCI > hotplug code, no removal of allow_hotunplug. What I'm proposing, I > believe, has a small cost and already starts the transition (which I > believe we can complete for 2.0). > > > The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if we make > > them all nofail, then it would be safe to call them in "realize" property setter. > > But we have at least 2 callbacks that can fail: > > pcie_cap_slot_hotplug() and shpc_device_hotplug() > > Both of them can be handled by a "check" step in the handler. > > > Goal of this series was to add and demonstrate reusable hotplug interface as > > opposed to PCI specific or SCSI-bus specific ones, so it could be used for memory > > hotplug as well. It might not do everything we would like but it looks like a move > > the right direction. > > If it's wrong direction, I could drop the idea and fallback to an original > > less intrusive approach, taking in account your comment to move type definitions > > into separate header. > > No, absolutely. I think it's the right direction, I just think more > aspects of it should be made generic. > > Paolo >
Il 09/12/2013 22:15, Igor Mammedov ha scritto: >> > Check is idempotent, and can be called before realize makes any change >> > (it could also be called after the device is added to >> > /machine/unattached, it's not a big difference). >> > >> > Plug is called after realize. > PCIE case: "check" before realize will work since code that does check depends > only on hotplug device (i.e. PCIE slot) and do not access not yet realized > device at all. > > however > SHPC case: check code access pci_slot that is derived from PCIDevice.devfn, > which in turn could be initialized in realize() (see pci_qdev_init() devfn > auto allocation). So it's not possible to call check before realize() it > should be called from realize(). > > Perhaps other hotplug buses/devices have similar limitations, where it's not > fine to access device state from outside before calling it's realize(), so it > should be some post_realize() hook then to make it generic which leads to the > following: > if ->plug() called after realize() fails, all we need to do is to > fail "realize" property setter. That should cause > qdev_device_add() -> object_unparent() -> device_unparent() -> unrealize() > doing all necessary cleanup. If you can make it work, that'd be great. Otherwise, let's do it later, but please make a wiki page with a todo list. We already have too many items on the same critical path (hotplug, memdev, NUMA,...). Paolo
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 49eca95..26229de 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -35,6 +35,7 @@ #include "hw/pci/msi.h" #include "hw/pci/msix.h" #include "exec/address-spaces.h" +#include "hw/hotplug.h" //#define DEBUG_PCI #ifdef DEBUG_PCI @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0])); } -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev) -{ - bus->qbus.allow_hotplug = 1; - bus->hotplug = hotplug; - bus->hotplug_qdev = qdev; -} - PCIBus *pci_register_bus(DeviceState *parent, const char *name, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, void *irq_opaque, @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev) { PCIDevice *pci_dev = (PCIDevice *)qdev; PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev); + DeviceState *hotplug_dev; + Error *local_err = NULL; PCIBus *bus; int rc; bool is_default_rom; @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev) } pci_add_option_rom(pci_dev, is_default_rom); - if (bus->hotplug) { - /* Let buses differentiate between hotplug and when device is - * enabled during qemu machine creation. */ - rc = bus->hotplug(bus->hotplug_qdev, pci_dev, - qdev->hotplugged ? PCI_HOTPLUG_ENABLED: - PCI_COLDPLUG_ENABLED); - if (rc != 0) { - int r = pci_unregister_device(&pci_dev->qdev); - assert(!r); - return rc; + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", + &local_err)); + if (error_is_set(&local_err)) { + goto error_exit; + } + if (hotplug_dev) { + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); + + /* handler can differentiate between hotplug and when device is + * enabled during qemu machine creation by inspecting + * dev->hotplugged field. */ + if (hdc->hotplug) { + hdc->hotplug(hotplug_dev, qdev, &local_err); + if (error_is_set(&local_err)) { + int r = pci_unregister_device(&pci_dev->qdev); + assert(!r); + goto error_exit; + } } } return 0; + +error_exit: + qerror_report_err(local_err); + error_free(local_err); + return -1; } static int pci_unplug_device(DeviceState *qdev) { PCIDevice *dev = PCI_DEVICE(qdev); PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev); + BusState *bus = qdev_get_parent_bus(qdev); + DeviceState *hotplug_dev; + Error *local_err = NULL; if (pc->no_hotplug) { qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev))); return -1; } - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, - PCI_HOTPLUG_DISABLED); + + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device", + &local_err)); + if (error_is_set(&local_err)) { + goto error_exit; + } + if (hotplug_dev) { + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev); + + if (hdc->hot_unplug) { + hdc->hot_unplug(hotplug_dev, qdev, &local_err); + if (error_is_set(&local_err)) { + goto error_exit; + } + } + } + return 0; + +error_exit: + qerror_report_err(local_err); + error_free(local_err); + return -1; } PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction, diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index b783e68..33dec40 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin); -typedef enum { - PCI_HOTPLUG_DISABLED, - PCI_HOTPLUG_ENABLED, - PCI_COLDPLUG_ENABLED, -} PCIHotplugState; - -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, - PCIHotplugState state); - #define TYPE_PCI_BUS "PCI" #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS) #define TYPE_PCIE_BUS "PCIE" @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name, void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq, void *irq_opaque, int nirq); int pci_bus_get_irq_level(PCIBus *bus, int irq_num); -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev); /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */ int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin); PCIBus *pci_register_bus(DeviceState *parent, const char *name, diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h index 9df1788..fabaeee 100644 --- a/include/hw/pci/pci_bus.h +++ b/include/hw/pci/pci_bus.h @@ -16,8 +16,6 @@ struct PCIBus { pci_set_irq_fn set_irq; pci_map_irq_fn map_irq; pci_route_irq_fn route_intx_to_irq; - pci_hotplug_fn hotplug; - DeviceState *hotplug_qdev; void *irq_opaque; PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX]; PCIDevice *parent_dev;
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------ include/hw/pci/pci.h | 10 ------ include/hw/pci/pci_bus.h | 2 - 3 files changed, 51 insertions(+), 31 deletions(-)