Re: [Qemu-devel] [PATCH RFC 01/10] pci: Convert core to realize

[Marcel Apfelbaum]

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 <address@hidden>
> ---
>  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 <address@hidden>