On Tue, 2014-10-28 at 08:35 +0100, Markus Armbruster wrote:
> Implement DeviceClass methods realize() and unrealize() instead of
> init() and exit(). The core's initialization errors now get
> propagated properly, and QMP sends them instead of an unspecific
> "Device initialization failed" error. Unrealize can't fail, so no
> change there.
>
> PCIDeviceClass is unchanged: it still provides init() and exit().
> Therefore, device models' errors are still not propagated.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> hw/pci/pci.c | 91
> +++++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 47 insertions(+), 44 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index cd7a403..aef95c3 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -114,7 +114,7 @@ static const TypeInfo pcie_bus_info = {
> static PCIBus *pci_find_bus_nr(PCIBus *bus, int bus_num);
> static void pci_update_mappings(PCIDevice *d);
> static void pci_irq_handler(void *opaque, int irq_num, int level);
> -static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom);
> +static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, Error
> **);
> static void pci_del_option_rom(PCIDevice *pdev);
>
> static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET;
> @@ -725,7 +725,7 @@ static void pci_init_mask_bridge(PCIDevice *d)
> PCI_PREF_RANGE_TYPE_MASK);
> }
>
> -static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> +static void pci_init_multifunction(PCIBus *bus, PCIDevice *dev, Error **errp)
> {
> uint8_t slot = PCI_SLOT(dev->devfn);
> uint8_t func;
> @@ -751,26 +751,25 @@ static int pci_init_multifunction(PCIBus *bus,
> PCIDevice *dev)
> PCIDevice *f0 = bus->devices[PCI_DEVFN(slot, 0)];
> if (f0 && !(f0->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)) {
> /* function 0 should set multifunction bit */
> - error_report("PCI: single function device can't be populated "
> - "in function %x.%x", slot, PCI_FUNC(dev->devfn));
> - return -1;
> + error_setg(errp, "PCI: single function device can't be populated
> "
> + "in function %x.%x", slot, PCI_FUNC(dev->devfn));
> + return;
> }
> - return 0;
> + return;
> }
>
> if (dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> - return 0;
> + return;
> }
> /* function 0 indicates single function, so function > 0 must be NULL */
> for (func = 1; func < PCI_FUNC_MAX; ++func) {
> if (bus->devices[PCI_DEVFN(slot, func)]) {
> - error_report("PCI: %x.0 indicates single function, "
> - "but %x.%x is already populated.",
> - slot, slot, func);
> - return -1;
> + error_setg(errp, "PCI: %x.0 indicates single function, "
> + "but %x.%x is already populated.",
> + slot, slot, func);
> + return;
> }
> }
> - return 0;
> }
>
> static void pci_config_alloc(PCIDevice *pci_dev)
> @@ -803,11 +802,13 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
>
> /* -1 for devfn means auto assign */
> static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> - const char *name, int devfn)
> + const char *name, int devfn,
> + Error **errp)
> {
> PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> PCIConfigReadFunc *config_read = pc->config_read;
> PCIConfigWriteFunc *config_write = pc->config_write;
> + Error *local_err = NULL;
> AddressSpace *dma_as;
>
> if (devfn < 0) {
> @@ -816,12 +817,15 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev, PCIBus *bus,
> if (!bus->devices[devfn])
> goto found;
> }
> - error_report("PCI: no slot/function available for %s, all in use",
> name);
> + error_setg(errp, "PCI: no slot/function available for %s, all in
> use",
> + name);
> return NULL;
> found: ;
> } else if (bus->devices[devfn]) {
> - error_report("PCI: slot %d function %d not available for %s, in use
> by %s",
> - PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> bus->devices[devfn]->name);
> + error_setg(errp, "PCI: slot %d function %d not available for %s,"
> + " in use by %s",
> + PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> + bus->devices[devfn]->name);
> return NULL;
> }
>
> @@ -865,7 +869,9 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev, PCIBus *bus,
> if (pc->is_bridge) {
> pci_init_mask_bridge(pci_dev);
> }
> - if (pci_init_multifunction(bus, pci_dev)) {
> + pci_init_multifunction(bus, pci_dev, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> do_pci_unregister_device(pci_dev);
> return NULL;
> }
> @@ -896,7 +902,7 @@ static void pci_unregister_io_regions(PCIDevice *pci_dev)
> pci_unregister_vga(pci_dev);
> }
>
> -static int pci_unregister_device(DeviceState *dev)
> +static void pci_qdev_unrealize(DeviceState *dev, Error **errp)
> {
> PCIDevice *pci_dev = PCI_DEVICE(dev);
> PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> @@ -909,7 +915,6 @@ static int pci_unregister_device(DeviceState *dev)
> }
>
> do_pci_unregister_device(pci_dev);
> - return 0;
> }
>
> void pci_register_bar(PCIDevice *pci_dev, int region_num,
> @@ -1742,10 +1747,11 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num,
> uint8_t devfn)
> return bus->devices[devfn];
> }
>
> -static int pci_qdev_init(DeviceState *qdev)
> +static void pci_qdev_realize(DeviceState *qdev, Error **errp)
> {
> PCIDevice *pci_dev = (PCIDevice *)qdev;
> PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> + Error *local_err = NULL;
> PCIBus *bus;
> int rc;
> bool is_default_rom;
> @@ -1758,15 +1764,16 @@ static int pci_qdev_init(DeviceState *qdev)
> bus = PCI_BUS(qdev_get_parent_bus(qdev));
> pci_dev = do_pci_register_device(pci_dev, bus,
> object_get_typename(OBJECT(qdev)),
> - pci_dev->devfn);
> + pci_dev->devfn, errp);
> if (pci_dev == NULL)
> - return -1;
> + return;
>
> if (pc->init) {
> rc = pc->init(pci_dev);
> if (rc != 0) {
> do_pci_unregister_device(pci_dev);
> - return rc;
> + error_setg(errp, "Device initialization failed");
> + return;
> }
> }
>
> @@ -1777,13 +1784,12 @@ static int pci_qdev_init(DeviceState *qdev)
> is_default_rom = true;
> }
>
> - rc = pci_add_option_rom(pci_dev, is_default_rom);
> - if (rc != 0) {
> - pci_unregister_device(DEVICE(pci_dev));
> - return rc;
> + pci_add_option_rom(pci_dev, is_default_rom, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + pci_qdev_unrealize(DEVICE(pci_dev), NULL);
> + return;
> }
> -
> - return 0;
> }
>
> PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool
> multifunction,
> @@ -1923,7 +1929,8 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t
> *ptr, int size)
> }
>
> /* Add an option rom for the device */
> -static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
> +static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
> + Error **errp)
> {
> int size;
> char *path;
> @@ -1932,9 +1939,9 @@ static int pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom)
> const VMStateDescription *vmsd;
>
> if (!pdev->romfile)
> - return 0;
> + return;
> if (strlen(pdev->romfile) == 0)
> - return 0;
> + return;
>
> if (!pdev->rom_bar) {
> /*
> @@ -1948,7 +1955,7 @@ static int pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom)
> * if the rom bar is disabled.
> */
> if (DEVICE(pdev)->hotplugged) {
> - return 0;
> + return;
> }
>
> if (class == 0x0300) {
> @@ -1956,7 +1963,7 @@ static int pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom)
> } else {
> rom_add_option(pdev->romfile, -1);
> }
> - return 0;
> + return;
> }
>
> path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> @@ -1966,15 +1973,13 @@ static int pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom)
>
> size = get_image_size(path);
> if (size < 0) {
> - error_report("%s: failed to find romfile \"%s\"",
> - __func__, pdev->romfile);
> + error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
> g_free(path);
> - return -1;
> + return;
> } else if (size == 0) {
> - error_report("%s: ignoring empty romfile \"%s\"",
> - __func__, pdev->romfile);
> + error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
> g_free(path);
> - return -1;
> + return;
> }
> if (size & (size - 1)) {
> size = 1 << qemu_fls(size);
> @@ -2000,8 +2005,6 @@ static int pci_add_option_rom(PCIDevice *pdev, bool
> is_default_rom)
> }
>
> pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom);
> -
> - return 0;
> }
>
> static void pci_del_option_rom(PCIDevice *pdev)
> @@ -2283,8 +2286,8 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev)
> static void pci_device_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *k = DEVICE_CLASS(klass);
> - k->init = pci_qdev_init;
> - k->exit = pci_unregister_device;
> + k->realize = pci_qdev_realize;
> + k->unrealize = pci_qdev_unrealize;
> k->bus_type = TYPE_PCI_BUS;
> k->props = pci_props;
> }
Code-wise looks clean to me. QOM-wise I still cannot definitely say
that I know all the pitfalls.
That being said,
Reviewed-by: Marcel Apfelbaum <marce...@redhat.com>
