From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39764) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq1Pb-0001X1-Py for qemu-devel@nongnu.org; Mon, 09 Dec 2013 09:02:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vq1PV-0000X7-On for qemu-devel@nongnu.org; Mon, 09 Dec 2013 09:02:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:12542) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq1PV-0000X3-G8 for qemu-devel@nongnu.org; Mon, 09 Dec 2013 09:02:09 -0500 Message-ID: <1386597714.10879.23.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 09 Dec 2013 16:01:54 +0200 In-Reply-To: <20131209145525.48f3a575@nial.usersys.redhat.com> References: <1386349395-5710-1-git-send-email-imammedo@redhat.com> <1386349395-5710-8-git-send-email-imammedo@redhat.com> <1386580142.10879.14.camel@localhost.localdomain> <20131209145525.48f3a575@nial.usersys.redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: peter.maydell@linaro.org, ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, alex.williamson@redhat.com, anthony@codemonkey.ws, pbonzini@redhat.com, afaerber@suse.de On Mon, 2013-12-09 at 14:55 +0100, Igor Mammedov wrote: > On Mon, 09 Dec 2013 11:09:02 +0200 > Marcel Apfelbaum wrote: > > > On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote: > > > Signed-off-by: Igor Mammedov > > > --- > > > 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; > > > > > > >