Re: [Qemu-devel] [PATCH 1/2] Add param Error** to msi_init() & modify the callers

[Cao jin <caoj.fnst@cn.fujitsu.com>]

Hi Markus,
     I have to say, you really did a amazing review for this "trivial 
"patch, thanks a lot & really appreciate it:)

On 12/07/2015 05:59 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> msi_init() is a supporting function in PCI device initialization, in order to
>> convert .init() to .realize(), it should be modified first. Also modify the
>> callers
>>
>> Bonus: add more comment for msi_init().
>
> Incomplete.  See notes on impact inline.
>
>> 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/pci/msi.c                       | 17 +++++++++++++----
>>   hw/scsi/megasas.c                  | 12 +++++++++---
>>   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, 63 insertions(+), 19 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) {
>
> Please use scripts/checkpatch.pl to check your patches.  It's
> occasionally wrong, so use your judgement.
>

Thanks for the tips, seems I got dizzy looking because many trivial 
place need to be modified...

>> +            return;
>
> This returns with the device in a half-realized state.  Do we have to
> undo prior side effects to put it back into unrealized state?  See also
> ioh3420_initfn() below.
>
> Before: msi_init() failure is ignored.  After: it makes device
> realization fail.  To assess impact, we need to understand how
> msi_init() can fail.
>

It seems I missed the reality: devices are default to be hot-pluggable & 
most devices are hot-pluggable:-[ Because when cold plugged, process 
will exit on device-init failing, so, the half-realized state doesn`t 
matter in this condition.
Will rework it later.

>> +        }
>>       }
>>
>>       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);
>
> Do we have to put the device back into unrealized state on failure?
>
>>   }
>>
>>   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;
>
> The error is neither propagated nor handled, and the error object leaks.
>
> Since this function can't handle it, it needs to propagate it.  Requires
> adding an Error ** parameter.
>

[*]Actually, here is my consideration: a device-realize function(take 
the following ioh3420 for example) will call many supporting functions 
like msi_init(), so I am planning, every supporting function goes into a 
patch first, then every "device convert to realize()" goes into a patch, 
otherwise, it may will be a big patch for the reviewer. That`s why I 
didn`t add Error ** param, and propagate it, and plan to do it in 
"convert to realize()" patch. But for now, I think this patch should at 
least be successfully compiled & won`t impact the existed things.

Yes, it seems may have leaks when error happens, but will be fixed when 
the "convert to realize()" patch comes out.

I am not sure whether the plan is ok, So, How do you think?

>> 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;
>
> The error is neither propagated nor handled, and the error object leaks.
>
> Since this is an init method, it should report errors with error_report
> and return -1.  The latter is there, but the former is missing.  You
> need to error_report_err(local_err).
>

Refer previous comment[*]: Was planning propagate it in "convert to 
realize()" patch later.
>>       }
>> +
>>       rc = pcie_cap_init(d, IOH_EP_EXP_OFFSET, PCI_EXP_TYPE_ROOT_PORT, p->port);
>
> Aside, not for this patch: this function (and many more) will have to be
> converted to Error as well before we can convert to realize.
>

Yes, I am planning every supporting function goes into a patch. Because 
msi & msix are too similar, so I put them together and send out first...

>>       if (rc < 0) {
>>           goto err_msi;
>
> Further down:
>
>     err:
>         pcie_chassis_del_slot(s);
>     err_pcie_cap:
>         pcie_cap_exit(d);
>     err_msi:
>         msi_uninit(d);
>     err_bridge:
>         pci_bridge_exitfn(d);
>         return rc;
>     }
>
> Note that we carefully undo side effects on failure.
>
>> 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;
>
> The error is neither propagated nor handled, and the error object leaks.
>
> Again, you need error_report_err(local_err).
>

Refer the previous comment[*]: was planning propagate it in "convert to 
realize()" patch later.

>>           }
>> @@ -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;
>
> Likewise.
>
>>       }
>> +
>>       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;
>
> Likewise.
>
>>       }
>> +
>>       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/pci/msi.c b/hw/pci/msi.c
>> index c1dd531..961f114 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)
>
> Let's see how this function can fail.
>
>>   {
>>       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 is not supported by interrupt controller");
>>           return -ENOTSUP;
>
> Attempt to use MSI when !msi_supported.
>
> Before: silently return -ENOTSUP.
>
> After: additionally pass a suitable error.
>
>>       }
>>
>> @@ -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;
>
> Can't set PCI_CAP_ID_MSI.
>
> Before: report with error_report_err() and return -errno.
>
> After: pass a suitable error and return -errno.
>
>>       }
>> @@ -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;
>>   }
>
> To assess the patch's impact, you need to compare before / after for
> both failure modes and each caller.  I suspect the result will promote
> your patch from "prepare for realize" to "fix to catch and report
> errors".
>

Thanks a lot for the direction:) but I still have a question: if I start 
off by per-device, then will modify every supporting function, and 
common supporting function will impact other device, so will need to 
convert other device together, and this will result in a huge patch 
contains converting of many devices and supporting functions, what do 
you think of it?

>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index d7dc667..4759fb5 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -2346,9 +2346,15 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>>       memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s,
>>                             "megasas-queue", 0x40000);
>>
>> -    if (megasas_use_msi(s) &&
>> -        msi_init(dev, 0x50, 1, true, false)) {
>> -        s->flags &= ~MEGASAS_MASK_USE_MSI;
>> +    if (megasas_use_msi(s)) {
>> +        int ret;
>> +
>> +        ret = msi_init(dev, 0x50, 1, true, false, errp);
>> +        if(ret > 0) {
>> +            s->flags &= ~MEGASAS_MASK_USE_MSI;
>> +        } else {
>> +            return;
>
> Do we have to undo side effects?

will assess it.

>
>> +        }
>>       }
>>       if (megasas_use_msix(s) &&
>>           msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
>> 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;
>
> Once again, error_report_err(local_err) needed.
>
>> 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;
>
> Do we have to undo side effects?
>

will assess it.

>> +        }
>>       }
>>       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;
>
> error_report_err(local_err) now, but for conversion to realize with
> require conversion to Error.
>

Refer previous comment[*]. Was plannning to add Error ** param to every 
function in the call chain when convert the vfio device to realize().

>> 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);
>
> I guess your PATCH 2 has similar issues; I'll skip reviewing it.
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin