[Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[Cao jin]

Hi,
    As you know, there are many PCI devices still using .init() as its
    initialization function, I am planning to do the "convert to realize()"
    work, and PCI bridge devices are chosen first.
    The supporting functions should be modified first. msi_init() a supporting
    function for PCI devices.

    Maybe it should be put in 2.6, as title indicated

Cao jin (2):
  Add param Error** to msi_init()
  Modify callers of msi_init()

 hw/audio/intel-hda.c               |  7 ++++++-
 hw/ide/ich.c                       |  2 +-
 hw/net/vmxnet3.c                   |  3 ++-
 hw/pci-bridge/ioh3420.c            |  6 +++++-
 hw/pci-bridge/pci_bridge_dev.c     |  6 +++++-
 hw/pci-bridge/xio3130_downstream.c |  7 ++++++-
 hw/pci-bridge/xio3130_upstream.c   |  7 ++++++-
 hw/pci/msi.c                       | 17 +++++++++++++----
 hw/scsi/megasas.c                  |  2 +-
 hw/scsi/vmw_pvscsi.c               |  3 ++-
 hw/usb/hcd-xhci.c                  |  5 ++++-
 hw/vfio/pci.c                      |  3 ++-
 include/hw/pci/msi.h               |  4 ++--
 13 files changed, 55 insertions(+), 17 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH for-2.6 1/2] Add param Error** to msi_init()

[Cao jin]

msi_init() is a supporting function in PCI device initialization, in order to
convert .init() to .realize(), it should be modified first.

Bonus: add more comment.
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/msi.c         | 17 +++++++++++++----
 include/hw/pci/msi.h |  4 ++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index c1dd531..1f5d9e8 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -150,15 +150,23 @@ bool msi_enabled(const PCIDevice *dev)
          PCI_MSI_FLAGS_ENABLE);
 }
 
-int msi_init(struct PCIDevice *dev, uint8_t offset,
-             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
+/*
+ * @nr_vectors: Multiple Message Capable field of Message Control register
+ * @msi64bit: support 64-bit message address or not
+ * @msi_per_vector_mask: support per-vector masking or not
+ *
+ * return: MSI capability offset in config space
+ */
+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
+             bool msi64bit, bool msi_per_vector_mask, Error **errp)
 {
     unsigned int vectors_order;
-    uint16_t flags;
+    uint16_t flags; /* Message Control register value */
     uint8_t cap_size;
     int config_offset;
 
     if (!msi_supported) {
+        error_setg(errp, "MSI/MSI-X is not supported by interrupt controller");
         return -ENOTSUP;
     }
 
@@ -182,7 +190,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
     }
 
     cap_size = msi_cap_sizeof(flags);
-    config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size);
+    config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset, cap_size, errp);
     if (config_offset < 0) {
         return config_offset;
     }
@@ -205,6 +213,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
         pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
                      0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
     }
+
     return config_offset;
 }
 
diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
index 50e452b..da1dc1a 100644
--- a/include/hw/pci/msi.h
+++ b/include/hw/pci/msi.h
@@ -34,8 +34,8 @@ extern bool msi_supported;
 void msi_set_message(PCIDevice *dev, MSIMessage msg);
 MSIMessage msi_get_message(PCIDevice *dev, unsigned int vector);
 bool msi_enabled(const PCIDevice *dev);
-int msi_init(struct PCIDevice *dev, uint8_t offset,
-             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);
+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
+             bool msi64bit, bool msi_per_vector_mask, Error **errp);
 void msi_uninit(struct PCIDevice *dev);
 void msi_reset(PCIDevice *dev);
 void msi_notify(PCIDevice *dev, unsigned int vector);
-- 
2.1.0

[Qemu-devel] [PATCH for-2.6 2/2] Modify callers of msi_init()

[Cao jin]

Because definition of msi_init() has been changed for the purpose: convert
to realize(). There are 11 callers for now.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/audio/intel-hda.c               | 7 ++++++-
 hw/ide/ich.c                       | 2 +-
 hw/net/vmxnet3.c                   | 3 ++-
 hw/pci-bridge/ioh3420.c            | 6 +++++-
 hw/pci-bridge/pci_bridge_dev.c     | 6 +++++-
 hw/pci-bridge/xio3130_downstream.c | 7 ++++++-
 hw/pci-bridge/xio3130_upstream.c   | 7 ++++++-
 hw/scsi/megasas.c                  | 2 +-
 hw/scsi/vmw_pvscsi.c               | 3 ++-
 hw/usb/hcd-xhci.c                  | 5 ++++-
 hw/vfio/pci.c                      | 3 ++-
 11 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 433463e..9d733da 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1130,6 +1130,7 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
 {
     IntelHDAState *d = INTEL_HDA(pci);
     uint8_t *conf = d->pci.config;
+    int ret;
 
     d->name = object_get_typename(OBJECT(d));
 
@@ -1142,7 +1143,11 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
                           "intel-hda", 0x4000);
     pci_register_bar(&d->pci, 0, 0, &d->mmio);
     if (d->msi) {
-        msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
+        ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true,
+                false, errp);
+        if(ret < 0) {
+            return;
+        }
     }
 
     hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 16925fa..94b1809 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -145,7 +145,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     /* Although the AHCI 1.3 specification states that the first capability
      * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9
      * AHCI device puts the MSI capability first, pointing to 0x80. */
-    msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false);
+    msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp);
 }
 
 static void pci_ich9_uninit(PCIDevice *dev)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5e3a233..4269141 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2140,9 +2140,10 @@ vmxnet3_init_msi(VMXNET3State *s)
 {
     PCIDevice *d = PCI_DEVICE(s);
     int res;
+    Error *local_err = NULL;
 
     res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS,
-                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
+                   VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, &local_err);
     if (0 > res) {
         VMW_WRPRN("Failed to initialize MSI, error %d", res);
         s->msi_used = false;
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index eead195..05b004a 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -96,6 +96,7 @@ static int ioh3420_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
+    Error *local_err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
@@ -105,12 +106,15 @@ static int ioh3420_initfn(PCIDevice *d)
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = msi_init(d, IOH_EP_MSI_OFFSET, IOH_EP_MSI_NR_VECTOR,
                   IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+                  IOH_EP_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+                  &local_err);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port);
     if (rc < 0) {
         goto err_msi;
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index bc3e1b7..13e8583 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -51,6 +51,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
     PCIBridge *br = PCI_BRIDGE(dev);
     PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
     int err;
+    Error *local_err = NULL;
 
     pci_bridge_initfn(dev, TYPE_PCI_BUS);
 
@@ -66,13 +67,15 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
         /* MSI is not applicable without SHPC */
         bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
     }
+
     err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
     if (err) {
         goto slotid_error;
     }
+
     if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
         msi_supported) {
-        err = msi_init(dev, 0, 1, true, true);
+        err = msi_init(dev, 0, 1, true, true, &local_err);
         if (err < 0) {
             goto msi_error;
         }
@@ -84,6 +87,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
                          PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
     }
     return 0;
+
 msi_error:
     slotid_cap_cleanup(dev);
 slotid_error:
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index b4dd25f..8e91034 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -59,21 +59,25 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
+    Error *local_err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
                   XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+                  &local_err);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
                                XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
                        p->port);
     if (rc < 0) {
@@ -103,6 +107,7 @@ err_msi:
     msi_uninit(d);
 err_bridge:
     pci_bridge_exitfn(d);
+
     return rc;
 }
 
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 434c8fd..bae49f6 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -55,26 +55,31 @@ static int xio3130_upstream_initfn(PCIDevice *d)
 {
     PCIEPort *p = PCIE_PORT(d);
     int rc;
+    Error *local_err = NULL;
 
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
                   XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_64BIT,
-                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT);
+                  XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+                  &local_err);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
                                XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
     if (rc < 0) {
         goto err_bridge;
     }
+
     rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
                        p->port);
     if (rc < 0) {
         goto err_msi;
     }
+
     pcie_cap_flr_init(d);
     pcie_cap_deverr_init(d);
     rc = pcie_aer_init(d, XIO3130_AER_OFFSET);
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index d7dc667..3290a9f 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2347,7 +2347,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
                           "megasas-queue", 0x40000);
 
     if (megasas_use_msi(s) &&
-        msi_init(dev, 0x50, 1, true, false)) {
+        msi_init(dev, 0x50, 1, true, false, errp)) {
         s->flags &= ~MEGASAS_MASK_USE_MSI;
     }
     if (megasas_use_msix(s) &&
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 9c71f31..b2ff293 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1018,9 +1018,10 @@ pvscsi_init_msi(PVSCSIState *s)
 {
     int res;
     PCIDevice *d = PCI_DEVICE(s);
+    Error *local_err = NULL;
 
     res = msi_init(d, PVSCSI_MSI_OFFSET, PVSCSI_MSIX_NUM_VECTORS,
-                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
+                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &local_err);
     if (res < 0) {
         trace_pvscsi_init_msi_fail(res);
         s->msi_used = false;
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 268ab36..b452c52 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3643,7 +3643,10 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     }
 
     if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
-        msi_init(dev, 0x70, xhci->numintrs, true, false);
+        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, errp);
+        if (ret < 0) {
+            return;
+        }
     }
     if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
         msix_init(dev, xhci->numintrs,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1fb868c..8559950 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1144,6 +1144,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
     uint16_t ctrl;
     bool msi_64bit, msi_maskbit;
     int ret, entries;
+    Error *local_err = NULL;
 
     if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
               vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
@@ -1157,7 +1158,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
 
     trace_vfio_msi_setup(vdev->vbasedev.name, pos);
 
-    ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit);
+    ret = msi_init(&vdev->pdev, pos, entries, msi_64bit, msi_maskbit, &local_err);
     if (ret < 0) {
         if (ret == -ENOTSUP) {
             return 0;
-- 
2.1.0

Re: [Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[John Snow]

On 12/04/2015 02:47 AM, Cao jin wrote:
> Hi,
>     As you know, there are many PCI devices still using .init() as its
>     initialization function, I am planning to do the "convert to realize()"
>     work, and PCI bridge devices are chosen first.
>     The supporting functions should be modified first. msi_init() a supporting
>     function for PCI devices.
> 
>     Maybe it should be put in 2.6, as title indicated
> 
> Cao jin (2):
>   Add param Error** to msi_init()
>   Modify callers of msi_init()
> 
>  hw/audio/intel-hda.c               |  7 ++++++-
>  hw/ide/ich.c                       |  2 +-
>  hw/net/vmxnet3.c                   |  3 ++-
>  hw/pci-bridge/ioh3420.c            |  6 +++++-
>  hw/pci-bridge/pci_bridge_dev.c     |  6 +++++-
>  hw/pci-bridge/xio3130_downstream.c |  7 ++++++-
>  hw/pci-bridge/xio3130_upstream.c   |  7 ++++++-
>  hw/pci/msi.c                       | 17 +++++++++++++----
>  hw/scsi/megasas.c                  |  2 +-
>  hw/scsi/vmw_pvscsi.c               |  3 ++-
>  hw/usb/hcd-xhci.c                  |  5 ++++-
>  hw/vfio/pci.c                      |  3 ++-
>  include/hw/pci/msi.h               |  4 ++--
>  13 files changed, 55 insertions(+), 17 deletions(-)
> 

You'll need to squash these patches as the first patch will break git
bisect.

--js

Re: [Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[Cao jin]

Hi John

On 12/05/2015 01:55 AM, John Snow wrote:
>
>
> On 12/04/2015 02:47 AM, Cao jin wrote:
>> Hi,
>>      As you know, there are many PCI devices still using .init() as its
>>      initialization function, I am planning to do the "convert to realize()"
>>      work, and PCI bridge devices are chosen first.
>>      The supporting functions should be modified first. msi_init() a supporting
>>      function for PCI devices.
>>
>>      Maybe it should be put in 2.6, as title indicated
>>
>> Cao jin (2):
>>    Add param Error** to msi_init()
>>    Modify callers of msi_init()
>>
>>   hw/audio/intel-hda.c               |  7 ++++++-
>>   hw/ide/ich.c                       |  2 +-
>>   hw/net/vmxnet3.c                   |  3 ++-
>>   hw/pci-bridge/ioh3420.c            |  6 +++++-
>>   hw/pci-bridge/pci_bridge_dev.c     |  6 +++++-
>>   hw/pci-bridge/xio3130_downstream.c |  7 ++++++-
>>   hw/pci-bridge/xio3130_upstream.c   |  7 ++++++-
>>   hw/pci/msi.c                       | 17 +++++++++++++----
>>   hw/scsi/megasas.c                  |  2 +-
>>   hw/scsi/vmw_pvscsi.c               |  3 ++-
>>   hw/usb/hcd-xhci.c                  |  5 ++++-
>>   hw/vfio/pci.c                      |  3 ++-
>>   include/hw/pci/msi.h               |  4 ++--
>>   13 files changed, 55 insertions(+), 17 deletions(-)
>>
>
> You'll need to squash these patches as the first patch will break git
> bisect.
>

Ok, will squash it. And I have another question： what`s the benefit of 
converting to realize? Because AFAICT, doing this make the error 
reporting machanism seems clean & clear, all device-init errors are 
passed above along the call chain. I mean, besides, are there any other 
benefits?

> --js
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> Hi John
>
> On 12/05/2015 01:55 AM, John Snow wrote:
>>
>>
>> On 12/04/2015 02:47 AM, Cao jin wrote:
>>> Hi,
>>>      As you know, there are many PCI devices still using .init() as its
>>>      initialization function, I am planning to do the "convert to realize()"
>>>      work, and PCI bridge devices are chosen first.
>>>      The supporting functions should be modified first. msi_init() a supporting
>>>      function for PCI devices.
>>>
>>>      Maybe it should be put in 2.6, as title indicated
>>>
>>> Cao jin (2):
>>>    Add param Error** to msi_init()
>>>    Modify callers of msi_init()
>>>
>>>   hw/audio/intel-hda.c               |  7 ++++++-
>>>   hw/ide/ich.c                       |  2 +-
>>>   hw/net/vmxnet3.c                   |  3 ++-
>>>   hw/pci-bridge/ioh3420.c            |  6 +++++-
>>>   hw/pci-bridge/pci_bridge_dev.c     |  6 +++++-
>>>   hw/pci-bridge/xio3130_downstream.c |  7 ++++++-
>>>   hw/pci-bridge/xio3130_upstream.c   |  7 ++++++-
>>>   hw/pci/msi.c                       | 17 +++++++++++++----
>>>   hw/scsi/megasas.c                  |  2 +-
>>>   hw/scsi/vmw_pvscsi.c               |  3 ++-
>>>   hw/usb/hcd-xhci.c                  |  5 ++++-
>>>   hw/vfio/pci.c                      |  3 ++-
>>>   include/hw/pci/msi.h               |  4 ++--
>>>   13 files changed, 55 insertions(+), 17 deletions(-)
>>>
>>
>> You'll need to squash these patches as the first patch will break git
>> bisect.
>>
>
> Ok, will squash it. And I have another question： what`s the benefit
> of converting to realize? Because AFAICT, doing this make the error
> reporting machanism seems clean & clear, all device-init errors are
> passed above along the call chain. I mean, besides, are there any
> other benefits?

Let's compare behavior on error:

* realize methods pass an error object to their caller

  The caller decides how to handle the error.

  Many callers simply pass it on.  Typically, errors from realize are
  passed up the call chain through qdev_device_add().

  Eventually, the caller that handles the error is reached.  If the
  error needs to be reported to the user, it decides how.  Examples:

  - device_init_func(), which does the actual work for -device, reports
    the human-readable message to stderr.

  - hmp_device_add(), which does the work for HMP device_add, reports
    the human-readable message to to the appropriate monitor.

  - handle_qmp_command(), which handles all QMP commands including QMP
    device_add, sends a QMP error reply down the QMP connection.

  - petalogix_ml605_init(), which creates the "petalogix-ml605" board,
    aborts when realizing the CPU fails.

* init methods report to stderr and return -1

  Fine when the error should be reported to stderr.  This is fine for
  some of the above examples, and wrong on others.  In particular, a
  device needs to provide a realize method to be fully work with
  device_add, unless its init method cannot fail.

Re: [Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> Hi John
>>
>> On 12/05/2015 01:55 AM, John Snow wrote:
>>>
>>>
>>> On 12/04/2015 02:47 AM, Cao jin wrote:
>>>> Hi,
>>>>      As you know, there are many PCI devices still using .init() as its
>>>>      initialization function, I am planning to do the "convert to realize()"
>>>>      work, and PCI bridge devices are chosen first.
>>>>      The supporting functions should be modified first. msi_init() a supporting
>>>>      function for PCI devices.
>>>>
>>>>      Maybe it should be put in 2.6, as title indicated
>>>>
>>>> Cao jin (2):
>>>>    Add param Error** to msi_init()
>>>>    Modify callers of msi_init()
>>>>
>>>>   hw/audio/intel-hda.c               |  7 ++++++-
>>>>   hw/ide/ich.c                       |  2 +-
>>>>   hw/net/vmxnet3.c                   |  3 ++-
>>>>   hw/pci-bridge/ioh3420.c            |  6 +++++-
>>>>   hw/pci-bridge/pci_bridge_dev.c     |  6 +++++-
>>>>   hw/pci-bridge/xio3130_downstream.c |  7 ++++++-
>>>>   hw/pci-bridge/xio3130_upstream.c   |  7 ++++++-
>>>>   hw/pci/msi.c                       | 17 +++++++++++++----
>>>>   hw/scsi/megasas.c                  |  2 +-
>>>>   hw/scsi/vmw_pvscsi.c               |  3 ++-
>>>>   hw/usb/hcd-xhci.c                  |  5 ++++-
>>>>   hw/vfio/pci.c                      |  3 ++-
>>>>   include/hw/pci/msi.h               |  4 ++--
>>>>   13 files changed, 55 insertions(+), 17 deletions(-)
>>>>
>>>
>>> You'll need to squash these patches as the first patch will break git
>>> bisect.
>>>
>>
>> Ok, will squash it. And I have another question： what`s the benefit
>> of converting to realize? Because AFAICT, doing this make the error
>> reporting machanism seems clean & clear, all device-init errors are
>> passed above along the call chain. I mean, besides, are there any
>> other benefits?
>
> Let's compare behavior on error:
>
> * realize methods pass an error object to their caller
>
>   The caller decides how to handle the error.
>
>   Many callers simply pass it on.  Typically, errors from realize are
>   passed up the call chain through qdev_device_add().
>
>   Eventually, the caller that handles the error is reached.  If the
>   error needs to be reported to the user, it decides how.  Examples:
>
>   - device_init_func(), which does the actual work for -device, reports
>     the human-readable message to stderr.
>
>   - hmp_device_add(), which does the work for HMP device_add, reports
>     the human-readable message to to the appropriate monitor.
>
>   - handle_qmp_command(), which handles all QMP commands including QMP
>     device_add, sends a QMP error reply down the QMP connection.
>
>   - petalogix_ml605_init(), which creates the "petalogix-ml605" board,
>     aborts when realizing the CPU fails.
>
> * init methods report to stderr and return -1
>
>   Fine when the error should be reported to stderr.

Correction: init methods should use error_report(), which reports either
to stderr or the current monitor.

>                                                      This is fine for
>   some of the above examples, and wrong on others.  In particular, a
>   device needs to provide a realize method to be fully work with
>   device_add, unless its init method cannot fail.

This is the case all the same.