| Message ID | 1459161888-32566-1-git-send-email-caoj.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/28/2016 01:44 PM, Cao jin wrote: > Add param Error **errp, and change pci_add_capability() to > pci_add_capability2(), because pci_add_capability() report error, and > msi_init() is widely used in realize(), so it is not suitable for realize(). > Hi, The patch is looking good in my opinion, a few comments below: > Also fix all the callers who should deal with the msi_init() failure but > actually not. The affected devices are as following: > > 1. intel hd audio: move msi_init() above, save the strength to free > the MemoryRegion when it fails. > 2. usb-xhci: move msi_init() above, save the strength to free the > MemoryRegion when it fails. > 3. ich9 ahci: it is a on-board device created during machine > initialization, when it fail, qemu will exit, so, no need to free > resource manually. > 4. megasas_scsi: move msi_init() above, save the strength to free > the MemoryRegion when it fails . Also fix a bug: when user enable > msi, and msi_init success(return positive offset value), the code > disable msi in the flags. But it seems this bug doesn`t do anything > bad. > 5. mptsas: when msi_init() fail, it will use INTx. So, catch the > error & report it right there, don`t propagate it. Move msi_init() > above, save the strength to free the MemoryRegion when it fails. > 6. pci_bridge_dev/ioh3420/xio3130_downstream/xio3130_upstream: catch > error & report it right there. > 7. vmxnet3: move msi_init() above, save the strength to free the > MemoryRegion when it fails. Remove the unecessary vmxnet3_init_msi(). > When msi_init() fail, it will use INTx, so msi_init()`s failure should not > break the realize. Just catch & report msi_init()`s failure message. > 8. pvscsi: when msi_init fail, it will use INTx. so msi_init failure > should not break the realize. Report the error when msi_init fail. > Nobody use the return value of pvscsi_init_msi(), so change its type > to void. > 9. vfio-pci: it ignores the config space corruption error, so, > catch & report it right there. I suggest to move 1-9 points to after --- so it will not be part of the commit message, only reference for the reviewers. > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > The patch has been compiled. No further test. > > hw/audio/intel-hda.c | 11 +++++++--- > hw/ide/ich.c | 2 +- > hw/net/vmxnet3.c | 41 ++++++++++++++------------------------ > hw/pci-bridge/ioh3420.c | 6 +++++- > hw/pci-bridge/pci_bridge_dev.c | 8 +++++++- > hw/pci-bridge/xio3130_downstream.c | 7 ++++++- > hw/pci-bridge/xio3130_upstream.c | 7 ++++++- > hw/pci/msi.c | 23 +++++++++++++++++++-- > hw/scsi/megasas.c | 12 +++++++---- > hw/scsi/mptsas.c | 17 +++++++++++----- > hw/scsi/vmw_pvscsi.c | 10 ++++++---- > hw/usb/hcd-xhci.c | 10 +++++++--- > hw/vfio/pci.c | 6 ++++-- > include/hw/pci/msi.h | 3 ++- > 14 files changed, 108 insertions(+), 55 deletions(-) > > diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c > index d372d4a..c3856cc 100644 > --- a/hw/audio/intel-hda.c > +++ b/hw/audio/intel-hda.c > @@ -1139,12 +1139,17 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) > /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */ > conf[0x40] = 0x01; > > + if (d->msi) { > + msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, > + true, false, errp); > + if (*errp) { > + return; > + } > + } > + > memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d, > "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); > - } > > hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), > intel_hda_response, intel_hda_xfer); > diff --git a/hw/ide/ich.c b/hw/ide/ich.c > index 0a13334..db4fdb5 100644 > --- a/hw/ide/ich.c > +++ b/hw/ide/ich.c > @@ -146,7 +146,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 093a71e..f3614f2 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -348,7 +348,7 @@ typedef struct { > /* Interrupt management */ > > /* > - *This function returns sign whether interrupt line is in asserted state > + * This function returns sign whether interrupt line is in asserted state > * This depends on the type of interrupt used. For INTX interrupt line will > * be asserted until explicit deassertion, for MSI(X) interrupt line will > * be deasserted automatically due to notification semantics of the MSI(X) > @@ -2189,27 +2189,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s) > } > } > > -#define VMXNET3_USE_64BIT (true) > -#define VMXNET3_PER_VECTOR_MASK (false) > - > -static bool > -vmxnet3_init_msi(VMXNET3State *s) > -{ > - PCIDevice *d = PCI_DEVICE(s); > - int res; > - > - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, > - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); > - if (0 > res) { > - VMW_WRPRN("Failed to initialize MSI, error %d", res); > - s->msi_used = false; > - } else { > - s->msi_used = true; > - } > - > - return s->msi_used; > -} > - > static void > vmxnet3_cleanup_msi(VMXNET3State *s) > { > @@ -2271,6 +2250,10 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s) > return dsnp; > } > > + > +#define VMXNET3_USE_64BIT (true) > +#define VMXNET3_PER_VECTOR_MASK (false) > + > static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) > { > DeviceState *dev = DEVICE(pci_dev); > @@ -2278,6 +2261,16 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) > > VMW_CBPRN("Starting init..."); > > + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, > + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp); > + if (*errp) { > + error_report_err(*errp); > + *errp = NULL; You suppress prev debug message: VMW_WRPRN("Failed to initialize MSI, error %d", res), but you replace with one of yourself. If the vmxnet maintainers have nothing against, I suppose is OK. Then, you don't propagate the error because you don't want realize to fail, so you report it and NULL it. I suppose we should have a "warning only" error type so the reporting party can decide to issue a warning or to fail, but is out of the scope of this patch, of course. > - s->msi_used = false; > + s->msi_used = false; > + } else { > + s->msi_used = true; > + } > + > memory_region_init_io(&s->bar0, OBJECT(s), &b0_ops, s, > "vmxnet3-b0", VMXNET3_PT_REG_SIZE); > pci_register_bar(pci_dev, VMXNET3_BAR0_IDX, > @@ -2302,10 +2295,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) > VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); > } > > - if (!vmxnet3_init_msi(s)) { > - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); Here again you remove an existent debug warning, maybe you should keep it. > - } > - > vmxnet3_net_init(s); > > if (pci_is_express(pci_dev)) { > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c > index 0937fa3..159a10c 100644 > --- a/hw/pci-bridge/ioh3420.c > +++ b/hw/pci-bridge/ioh3420.c > @@ -97,6 +97,7 @@ static int ioh3420_initfn(PCIDevice *d) > PCIEPort *p = PCIE_PORT(d); > PCIESlot *s = PCIE_SLOT(d); > int rc; > + Error *err = NULL; > > pci_bridge_initfn(d, TYPE_PCIE_BUS); > pcie_port_init_reg(d); > @@ -106,12 +107,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, &err); > if (rc < 0) { > + error_report_err(err); > 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 862a236..07c7bf8 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -52,6 +52,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; You use both local_err and err for local error names. It doesn't really matter the name, but please be consistent. I personally like local_error but is up to you, of course. > > pci_bridge_initfn(dev, TYPE_PCI_BUS); > > @@ -67,17 +68,21 @@ 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; > } > + You have several new lines (before and after this comment) that have nothing to do with the patch. I suggest putting them into a trivial separate patch. > if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && > msi_nonbroken) { > - err = msi_init(dev, 0, 1, true, true); > + err = msi_init(dev, 0, 1, true, true, &local_err); > if (err < 0) { > + error_report_err(local_err); > goto msi_error; > } > } > + > if (shpc_present(dev)) { > /* TODO: spec recommends using 64 bit prefetcheable BAR. > * Check whether that works well. */ > @@ -85,6 +90,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 cf1ee63..7428923 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -60,26 +60,31 @@ static int xio3130_downstream_initfn(PCIDevice *d) > PCIEPort *p = PCIE_PORT(d); > PCIESlot *s = PCIE_SLOT(d); > int rc; > + Error *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, &err); > if (rc < 0) { > + error_report_err(err); > 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) { > goto err_msi; > } > + > pcie_cap_flr_init(d); > pcie_cap_deverr_init(d); > pcie_cap_slot_init(d, s->slot); > diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c > index 164ef58..a7f7987 100644 > --- a/hw/pci-bridge/xio3130_upstream.c > +++ b/hw/pci-bridge/xio3130_upstream.c > @@ -56,26 +56,31 @@ static int xio3130_upstream_initfn(PCIDevice *d) > { > PCIEPort *p = PCIE_PORT(d); > int rc; > + Error *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, &err); > if (rc < 0) { > + error_report_err(err); > 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, PCI_ERR_SIZEOF); > diff --git a/hw/pci/msi.c b/hw/pci/msi.c > index e0e64c2..b1c8f8f 100644 > --- a/hw/pci/msi.c > +++ b/hw/pci/msi.c > @@ -165,15 +165,32 @@ bool msi_enabled(const PCIDevice *dev) > PCI_MSI_FLAGS_ENABLE); > } > > +/* > + * Make PCI device @dev MSI-capable. > + * Non-zero @offset puts capability MSI at that offset in PCI config > + * space. > + * @nr_vectors is the number of MSI vectors (1, 2, 4, 8, 16 or 32). > + * If @msi64bit, make the device capable of sending a 64-bit message > + * address. > + * If @msi_per_vector_mask, make the device support per-vector masking. > + * @errp is for returning errors. > + * Return the offset of capability MSI in config space on success, > + * set @errp and return -errno on error. > + * -ENOTSUP means lacking msi support for a msi-capable platform. > + */ > int msi_init(struct PCIDevice *dev, uint8_t offset, > - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask) > + unsigned int nr_vectors, bool msi64bit, > + bool msi_per_vector_mask, Error **errp) > { > unsigned int vectors_order; > uint16_t flags; > uint8_t cap_size; > int config_offset; > + Error *err = NULL; > > if (!msi_nonbroken) { > + error_setg(&err, "MSI is not supported by interrupt controller"); > + error_propagate(errp, err); I suppose error_setg(errp... is enough and you don't have to use also error_propagate. > return -ENOTSUP; > } > > @@ -197,8 +214,10 @@ 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, &err); > if (config_offset < 0) { > + error_propagate(errp, err); > return config_offset; > } > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index a63a581..0aaf3af 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -2340,6 +2340,14 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) > /* Interrupt pin 1 */ > pci_conf[PCI_INTERRUPT_PIN] = 0x01; > > + if (megasas_use_msi(s)) { > + msi_init(dev, 0x50, 1, true, false, errp); > + if (*errp) { > + s->flags &= ~MEGASAS_MASK_USE_MSI; > + return; > + } > + } > + > memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s, > "megasas-mmio", 0x4000); > memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s, > @@ -2347,10 +2355,6 @@ 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)) { This was a bug. msi_init returns non-zero value on both failure and success. Your code fixes the bug by checking the errp. I think you should fix the bug in a separate patch and only then apply this patch. > - s->flags &= ~MEGASAS_MASK_USE_MSI; > - } > if (megasas_use_msix(s) && > msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, > &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { > diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c > index 499c146..27350b7 100644 > --- a/hw/scsi/mptsas.c > +++ b/hw/scsi/mptsas.c > @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) > { > DeviceState *d = DEVICE(dev); > MPTSASState *s = MPT_SAS(dev); > + Error *err = NULL; > > dev->config[PCI_LATENCY_TIMER] = 0; > dev->config[PCI_INTERRUPT_PIN] = 0x01; > > + if (s->msi_available) { > + if (msi_init(dev, 0, 1, true, false, &err) >= 0) { > + s->msi_in_use = true; > + } > + else { > + error_append_hint(&err, "MSI init fail, will use INTx instead"); The hint should end with a new line: "\n". > + error_report_err(err); You should not report the error if the fucntion has an **errp parameter. > + } > + } > + > + > memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s, > "mptsas-mmio", 0x4000); > memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s, > @@ -1285,11 +1297,6 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) > memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s, > "mptsas-diag", 0x10000); > > - if (s->msi_available && > - msi_init(dev, 0, 1, true, false) >= 0) { > - s->msi_in_use = true; > - } > - > pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io); > pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY | > PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io); > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c > index 9abc086..88fb611 100644 > --- a/hw/scsi/vmw_pvscsi.c > +++ b/hw/scsi/vmw_pvscsi.c > @@ -1039,22 +1039,24 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size) > } > > > -static bool > +static void > pvscsi_init_msi(PVSCSIState *s) > { > int res; > + Error *err = NULL; > PCIDevice *d = PCI_DEVICE(s); > > res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS, > - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK); > + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err); > if (res < 0) { > trace_pvscsi_init_msi_fail(res); > + error_append_hint(&err, "MSI capability fail to init," > + " will use INTx intead"); The same for this hint, should end with "\n" > + error_report_err(err); > s->msi_used = false; > } else { > s->msi_used = true; > } > - > - return s->msi_used; Refactoring the function to return void instead of bool is out of the scope of this patch. You can take this out to a simple patch. > } > > static void > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index bcde8a2..f132a57 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -3588,6 +3588,13 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > > usb_xhci_init(xhci); > > + if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) { > + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, errp); > + if (ret < 0) { > + return; > + } > + } > + > if (xhci->numintrs > MAXINTRS) { > xhci->numintrs = MAXINTRS; > } > @@ -3645,9 +3652,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) > assert(ret >= 0); > } > > - if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) { > - msi_init(dev, 0x70, xhci->numintrs, true, false); > - } > if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) { > msix_init(dev, xhci->numintrs, > &xhci->mem, 0, OFF_MSIX_TABLE, > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index d091d8c..55ceb67 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1171,6 +1171,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) > uint16_t ctrl; > bool msi_64bit, msi_maskbit; > int ret, entries; > + Error *err = NULL; > > if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), > vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { > @@ -1184,12 +1185,13 @@ 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, &err); > if (ret < 0) { > if (ret == -ENOTSUP) { > return 0; > } > - error_report("vfio: msi_init failed"); > + error_prepend(&err, "vfio: msi_init failed: "); > + error_report_err(err); > return ret; > } > vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0); > diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h > index 8124908..4837bcf 100644 > --- a/include/hw/pci/msi.h > +++ b/include/hw/pci/msi.h > @@ -35,7 +35,8 @@ 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); > + 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); > Thanks, Marcel
Hi Marcel,
Thanks for your quick review for this big fat patch:)
please see my comments inline.
On 03/30/2016 04:56 AM, Marcel Apfelbaum wrote:
> On 03/28/2016 01:44 PM, Cao jin wrote:
>>
>> -#define VMXNET3_USE_64BIT (true)
>> -#define VMXNET3_PER_VECTOR_MASK (false)
>> -
>> -static bool
>> -vmxnet3_init_msi(VMXNET3State *s)
>> -{
>> - PCIDevice *d = PCI_DEVICE(s);
>> - int res;
>> -
>> - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
>> - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK);
>> - if (0 > res) {
>> - VMW_WRPRN("Failed to initialize MSI, error %d", res);
>> - s->msi_used = false;
>> - } else {
>> - s->msi_used = true;
>> - }
>> -
>> - return s->msi_used;
>> -}
>> -
>> static void
>> vmxnet3_cleanup_msi(VMXNET3State *s)
>> {
>> @@ -2271,6 +2250,10 @@ static uint8_t
>> *vmxnet3_device_serial_num(VMXNET3State *s)
>> return dsnp;
>> }
>>
>> +
>> +#define VMXNET3_USE_64BIT (true)
>> +#define VMXNET3_PER_VECTOR_MASK (false)
>> +
>> static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>> {
>> DeviceState *dev = DEVICE(pci_dev);
>> @@ -2278,6 +2261,16 @@ static void vmxnet3_pci_realize(PCIDevice
>> *pci_dev, Error **errp)
>>
>> VMW_CBPRN("Starting init...");
>>
>> + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
>> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp);
>> + if (*errp) {
>> + error_report_err(*errp);
>> + *errp = NULL;
>
> You suppress prev debug message: VMW_WRPRN("Failed to initialize MSI,
> error %d", res),
> but you replace with one of yourself. If the vmxnet maintainers have
> nothing against,
> I suppose is OK.
>
> Then, you don't propagate the error because you don't want realize
> to fail, so you report it and NULL it. I suppose we should have
> a "warning only" error type so the reporting party can decide to issue a
> warning
> or to fail, but is out of the scope of this patch, of course.
>
Yes, I should add more hint message. I don`t quite understand about:
/have a "warning only" error type so the reporting party can decide to
issue a warning or to fail/
Do you mean still using VMW_WRPRN or using error_append_hint? It seems
VMW_WRPRN should only work in debug time? and if user should see this
error message, should use error_report_err ?
>> - if (!vmxnet3_init_msi(s)) {
>> - VMW_WRPRN("Failed to initialize MSI, configuration is
>> inconsistent.");
>
> Here again you remove an existent debug warning, maybe you should keep it.
>
>> - }
>> -
>> vmxnet3_net_init(s);
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>> b/hw/pci-bridge/pci_bridge_dev.c
>> index 862a236..07c7bf8 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -52,6 +52,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;
>
> You use both local_err and err for local error names. It doesn't really
> matter
> the name, but please be consistent. I personally like local_error but is
> up to you, of course.
>
Yes, I prefer consistent way, but here, you see there is a "int err", so
I thought two different variants using same name maybe not so good for
reading code? what would you choose?(I like local_err at first because
it is easy to understand for newbie, but when I get familiar with error
handling, I turn to like err, just because it is shorter:p)
>>
>> pci_bridge_initfn(dev, TYPE_PCI_BUS);
>>
>> @@ -67,17 +68,21 @@ 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;
>> }
>> +
>
> You have several new lines (before and after this comment) that have
> nothing
> to do with the patch. I suggest putting them into a trivial separate patch.
>
see what I was told before:
http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html
http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html
Actually I am ok with both sides. I just stop sending coding style patch
since then:)
I don`t know, coding style & indentation patch really matters or is just
a personal taste thing?
>> +++ b/hw/scsi/mptsas.c
>> @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(PCIDevice *dev,
>> Error **errp)
>> {
>> DeviceState *d = DEVICE(dev);
>> MPTSASState *s = MPT_SAS(dev);
>> + Error *err = NULL;
>>
>> dev->config[PCI_LATENCY_TIMER] = 0;
>> dev->config[PCI_INTERRUPT_PIN] = 0x01;
>>
>> + if (s->msi_available) {
>> + if (msi_init(dev, 0, 1, true, false, &err) >= 0) {
>> + s->msi_in_use = true;
>> + }
>> + else {
>> + error_append_hint(&err, "MSI init fail, will use INTx
>> instead");
>
> The hint should end with a new line: "\n".
>
>> + error_report_err(err);
>
> You should not report the error if the fucntion has an **errp parameter.
>
it will use INTx even if msi_init fail, so it should not break realize.
But I think user should know it if something wrong happened there?so I
use a local *err* to report error message, while don`t touch **errp. Is
there any better way?
>
For all the other comments, will fix them in next version.
On 03/30/2016 07:10 AM, Cao jin wrote: > Hi Marcel, > Thanks for your quick review for this big fat patch:) > please see my comments inline. > > On 03/30/2016 04:56 AM, Marcel Apfelbaum wrote: >> On 03/28/2016 01:44 PM, Cao jin wrote: > >>> >>> -#define VMXNET3_USE_64BIT (true) >>> -#define VMXNET3_PER_VECTOR_MASK (false) >>> - >>> -static bool >>> -vmxnet3_init_msi(VMXNET3State *s) >>> -{ >>> - PCIDevice *d = PCI_DEVICE(s); >>> - int res; >>> - >>> - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, >>> - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); >>> - if (0 > res) { >>> - VMW_WRPRN("Failed to initialize MSI, error %d", res); >>> - s->msi_used = false; >>> - } else { >>> - s->msi_used = true; >>> - } >>> - >>> - return s->msi_used; >>> -} >>> - >>> static void >>> vmxnet3_cleanup_msi(VMXNET3State *s) >>> { >>> @@ -2271,6 +2250,10 @@ static uint8_t >>> *vmxnet3_device_serial_num(VMXNET3State *s) >>> return dsnp; >>> } >>> >>> + >>> +#define VMXNET3_USE_64BIT (true) >>> +#define VMXNET3_PER_VECTOR_MASK (false) >>> + >>> static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) >>> { >>> DeviceState *dev = DEVICE(pci_dev); >>> @@ -2278,6 +2261,16 @@ static void vmxnet3_pci_realize(PCIDevice >>> *pci_dev, Error **errp) >>> >>> VMW_CBPRN("Starting init..."); >>> >>> + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, >>> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp); >>> + if (*errp) { >>> + error_report_err(*errp); >>> + *errp = NULL; >> >> You suppress prev debug message: VMW_WRPRN("Failed to initialize MSI, >> error %d", res), >> but you replace with one of yourself. If the vmxnet maintainers have >> nothing against, >> I suppose is OK. >> >> Then, you don't propagate the error because you don't want realize >> to fail, so you report it and NULL it. I suppose we should have >> a "warning only" error type so the reporting party can decide to issue a >> warning >> or to fail, but is out of the scope of this patch, of course. >> Hi, > > Yes, I should add more hint message. I don`t quite understand about: > > /have a "warning only" error type so the reporting party can decide to issue a warning or to fail/ > > Do you mean still using VMW_WRPRN or using error_append_hint? It seems VMW_WRPRN should only work in debug time? and if user should see this error message, should use error_report_err ? No, it is not related to VMW_WRPRN. On this subject, those are debugging warnings and we should keep them the same as before. About the "warning only" error type: if an error is returned, the caller assumes that the initialization failed and it will return with failure. That means that you cannot return a non-null error if you want the process to finish OK. This is why you had to: - report the error (even if this function should not be a reporter because it receives an Error parameter) - empty the error: so why use error at all, right? My point is if the caller had a way to check what kind of error this is and act accordingly, it would be nicer. But again, this is out of the scope of this patch, only some thoughts. > >>> - if (!vmxnet3_init_msi(s)) { >>> - VMW_WRPRN("Failed to initialize MSI, configuration is >>> inconsistent."); >> >> Here again you remove an existent debug warning, maybe you should keep it. >> >>> - } >>> - >>> vmxnet3_net_init(s); >>> > >>> diff --git a/hw/pci-bridge/pci_bridge_dev.c >>> b/hw/pci-bridge/pci_bridge_dev.c >>> index 862a236..07c7bf8 100644 >>> --- a/hw/pci-bridge/pci_bridge_dev.c >>> +++ b/hw/pci-bridge/pci_bridge_dev.c >>> @@ -52,6 +52,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; >> >> You use both local_err and err for local error names. It doesn't really >> matter >> the name, but please be consistent. I personally like local_error but is >> up to you, of course. >> > > Yes, I prefer consistent way, but here, you see there is a "int err", so I thought two different variants using same name maybe not so good for reading code? what would you choose?(I like local_err > at first because it is easy to understand for newbie, but when I get familiar with error handling, I turn to like err, just because it is shorter:p) Indeed is a little confusing, this is why I prefer "local_error" because it is used widely and became a familiar pattern. > >>> >>> pci_bridge_initfn(dev, TYPE_PCI_BUS); >>> >>> @@ -67,17 +68,21 @@ 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; >>> } >>> + >> >> You have several new lines (before and after this comment) that have >> nothing >> to do with the patch. I suggest putting them into a trivial separate patch. >> > > see what I was told before: > http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html > http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html > > Actually I am ok with both sides. I just stop sending coding style patch since then:) Oh, I hate when it happens to me, tho different opinions, how can you deal with that, right ? :) > > I don`t know, coding style & indentation patch really matters or is just a personal taste thing? Coding style and indentation *are important* IMHO. For this case, what I would do is put the new lines and comments in a separate patch,\ but send it with *the same* series, not through trivial list, saying something like: "fixed some code style problems while resolving the ... problem". > >>> +++ b/hw/scsi/mptsas.c >>> @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(PCIDevice *dev, >>> Error **errp) >>> { >>> DeviceState *d = DEVICE(dev); >>> MPTSASState *s = MPT_SAS(dev); >>> + Error *err = NULL; >>> >>> dev->config[PCI_LATENCY_TIMER] = 0; >>> dev->config[PCI_INTERRUPT_PIN] = 0x01; >>> >>> + if (s->msi_available) { >>> + if (msi_init(dev, 0, 1, true, false, &err) >= 0) { >>> + s->msi_in_use = true; >>> + } >>> + else { >>> + error_append_hint(&err, "MSI init fail, will use INTx >>> instead"); >> >> The hint should end with a new line: "\n". >> >>> + error_report_err(err); >> >> You should not report the error if the fucntion has an **errp parameter. >> > > it will use INTx even if msi_init fail, so it should not break realize. But I think user should know it if something wrong happened there?so I use a local *err* to report error message, while don`t > touch **errp. Is there any better way? Same problem as discussed before, Markus do you have any idea how to solve this elegantly? CC: Markus Armbruster <armbru@redhat.com> Thanks, Marcel > >> > > For all the other comments, will fix them in next version.
On 03/30/2016 05:00 PM, Marcel Apfelbaum wrote: > On 03/30/2016 07:10 AM, Cao jin wrote: > > Hi, > >> >> Yes, I should add more hint message. I don`t quite understand about: >> >> /have a "warning only" error type so the reporting party can decide to >> issue a warning or to fail/ >> >> Do you mean still using VMW_WRPRN or using error_append_hint? It seems >> VMW_WRPRN should only work in debug time? and if user should see this >> error message, should use error_report_err ? > > No, it is not related to VMW_WRPRN. On this subject, those are debugging > warnings > and we should keep them the same as before. > ok > About the "warning only" error type: if an error is returned, the caller > assumes that the initialization failed and it will return with failure. > That means > that you cannot return a non-null error if you want the process to > finish OK. > > This is why you had to: > - report the error (even if this function should not be a reporter > because it receives an Error parameter) > - empty the error: so why use error at all, right? > > My point is if the caller had a way to check what kind of error this is > and act accordingly, it would be nicer. But again, this is out of the > scope of this patch, only some thoughts. > I see, and agree. >> >>> >> >> see what I was told before: >> http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00116.html >> http://lists.nongnu.org/archive/html/qemu-trivial/2015-10/msg00123.html >> >> Actually I am ok with both sides. I just stop sending coding style >> patch since then:) > > Oh, I hate when it happens to me, tho different opinions, how can you > deal with that, right ? :) > >> >> I don`t know, coding style & indentation patch really matters or is >> just a personal taste thing? > > Coding style and indentation *are important* IMHO. > Totally, absolutely agree > For this case, what I would do is put the new lines and comments in a > separate patch,\ > but send it with *the same* series, not through trivial list, saying > something like: > "fixed some code style problems while resolving the ... problem". > OK
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c index d372d4a..c3856cc 100644 --- a/hw/audio/intel-hda.c +++ b/hw/audio/intel-hda.c @@ -1139,12 +1139,17 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp) /* HDCTL off 0x40 bit 0 selects signaling mode (1-HDA, 0 - Ac97) 18.1.19 */ conf[0x40] = 0x01; + if (d->msi) { + msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, + true, false, errp); + if (*errp) { + return; + } + } + memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d, "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); - } hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs), intel_hda_response, intel_hda_xfer); diff --git a/hw/ide/ich.c b/hw/ide/ich.c index 0a13334..db4fdb5 100644 --- a/hw/ide/ich.c +++ b/hw/ide/ich.c @@ -146,7 +146,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 093a71e..f3614f2 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -348,7 +348,7 @@ typedef struct { /* Interrupt management */ /* - *This function returns sign whether interrupt line is in asserted state + * This function returns sign whether interrupt line is in asserted state * This depends on the type of interrupt used. For INTX interrupt line will * be asserted until explicit deassertion, for MSI(X) interrupt line will * be deasserted automatically due to notification semantics of the MSI(X) @@ -2189,27 +2189,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s) } } -#define VMXNET3_USE_64BIT (true) -#define VMXNET3_PER_VECTOR_MASK (false) - -static bool -vmxnet3_init_msi(VMXNET3State *s) -{ - PCIDevice *d = PCI_DEVICE(s); - int res; - - res = msi_init(d, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, - VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); - if (0 > res) { - VMW_WRPRN("Failed to initialize MSI, error %d", res); - s->msi_used = false; - } else { - s->msi_used = true; - } - - return s->msi_used; -} - static void vmxnet3_cleanup_msi(VMXNET3State *s) { @@ -2271,6 +2250,10 @@ static uint8_t *vmxnet3_device_serial_num(VMXNET3State *s) return dsnp; } + +#define VMXNET3_USE_64BIT (true) +#define VMXNET3_PER_VECTOR_MASK (false) + static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) { DeviceState *dev = DEVICE(pci_dev); @@ -2278,6 +2261,16 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) VMW_CBPRN("Starting init..."); + msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS, + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, errp); + if (*errp) { + error_report_err(*errp); + *errp = NULL; + s->msi_used = false; + } else { + s->msi_used = true; + } + memory_region_init_io(&s->bar0, OBJECT(s), &b0_ops, s, "vmxnet3-b0", VMXNET3_PT_REG_SIZE); pci_register_bar(pci_dev, VMXNET3_BAR0_IDX, @@ -2302,10 +2295,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent."); } - if (!vmxnet3_init_msi(s)) { - VMW_WRPRN("Failed to initialize MSI, configuration is inconsistent."); - } - vmxnet3_net_init(s); if (pci_is_express(pci_dev)) { diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c index 0937fa3..159a10c 100644 --- a/hw/pci-bridge/ioh3420.c +++ b/hw/pci-bridge/ioh3420.c @@ -97,6 +97,7 @@ static int ioh3420_initfn(PCIDevice *d) PCIEPort *p = PCIE_PORT(d); PCIESlot *s = PCIE_SLOT(d); int rc; + Error *err = NULL; pci_bridge_initfn(d, TYPE_PCIE_BUS); pcie_port_init_reg(d); @@ -106,12 +107,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, &err); if (rc < 0) { + error_report_err(err); 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 862a236..07c7bf8 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -52,6 +52,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); @@ -67,17 +68,21 @@ 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_nonbroken) { - err = msi_init(dev, 0, 1, true, true); + err = msi_init(dev, 0, 1, true, true, &local_err); if (err < 0) { + error_report_err(local_err); goto msi_error; } } + if (shpc_present(dev)) { /* TODO: spec recommends using 64 bit prefetcheable BAR. * Check whether that works well. */ @@ -85,6 +90,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 cf1ee63..7428923 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -60,26 +60,31 @@ static int xio3130_downstream_initfn(PCIDevice *d) PCIEPort *p = PCIE_PORT(d); PCIESlot *s = PCIE_SLOT(d); int rc; + Error *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, &err); if (rc < 0) { + error_report_err(err); 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) { goto err_msi; } + pcie_cap_flr_init(d); pcie_cap_deverr_init(d); pcie_cap_slot_init(d, s->slot); diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c index 164ef58..a7f7987 100644 --- a/hw/pci-bridge/xio3130_upstream.c +++ b/hw/pci-bridge/xio3130_upstream.c @@ -56,26 +56,31 @@ static int xio3130_upstream_initfn(PCIDevice *d) { PCIEPort *p = PCIE_PORT(d); int rc; + Error *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, &err); if (rc < 0) { + error_report_err(err); 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, PCI_ERR_SIZEOF); diff --git a/hw/pci/msi.c b/hw/pci/msi.c index e0e64c2..b1c8f8f 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -165,15 +165,32 @@ bool msi_enabled(const PCIDevice *dev) PCI_MSI_FLAGS_ENABLE); } +/* + * Make PCI device @dev MSI-capable. + * Non-zero @offset puts capability MSI at that offset in PCI config + * space. + * @nr_vectors is the number of MSI vectors (1, 2, 4, 8, 16 or 32). + * If @msi64bit, make the device capable of sending a 64-bit message + * address. + * If @msi_per_vector_mask, make the device support per-vector masking. + * @errp is for returning errors. + * Return the offset of capability MSI in config space on success, + * set @errp and return -errno on error. + * -ENOTSUP means lacking msi support for a msi-capable platform. + */ int msi_init(struct PCIDevice *dev, uint8_t offset, - unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask) + unsigned int nr_vectors, bool msi64bit, + bool msi_per_vector_mask, Error **errp) { unsigned int vectors_order; uint16_t flags; uint8_t cap_size; int config_offset; + Error *err = NULL; if (!msi_nonbroken) { + error_setg(&err, "MSI is not supported by interrupt controller"); + error_propagate(errp, err); return -ENOTSUP; } @@ -197,8 +214,10 @@ 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, &err); if (config_offset < 0) { + error_propagate(errp, err); return config_offset; } diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index a63a581..0aaf3af 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2340,6 +2340,14 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp) /* Interrupt pin 1 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; + if (megasas_use_msi(s)) { + msi_init(dev, 0x50, 1, true, false, errp); + if (*errp) { + s->flags &= ~MEGASAS_MASK_USE_MSI; + return; + } + } + memory_region_init_io(&s->mmio_io, OBJECT(s), &megasas_mmio_ops, s, "megasas-mmio", 0x4000); memory_region_init_io(&s->port_io, OBJECT(s), &megasas_port_ops, s, @@ -2347,10 +2355,6 @@ 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_msix(s) && msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000, &s->mmio_io, b->mmio_bar, 0x3800, 0x68)) { diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index 499c146..27350b7 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1274,10 +1274,22 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) { DeviceState *d = DEVICE(dev); MPTSASState *s = MPT_SAS(dev); + Error *err = NULL; dev->config[PCI_LATENCY_TIMER] = 0; dev->config[PCI_INTERRUPT_PIN] = 0x01; + if (s->msi_available) { + if (msi_init(dev, 0, 1, true, false, &err) >= 0) { + s->msi_in_use = true; + } + else { + error_append_hint(&err, "MSI init fail, will use INTx instead"); + error_report_err(err); + } + } + + memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s, "mptsas-mmio", 0x4000); memory_region_init_io(&s->port_io, OBJECT(s), &mptsas_port_ops, s, @@ -1285,11 +1297,6 @@ static void mptsas_scsi_init(PCIDevice *dev, Error **errp) memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s, "mptsas-diag", 0x10000); - if (s->msi_available && - msi_init(dev, 0, 1, true, false) >= 0) { - s->msi_in_use = true; - } - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io); pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32, &s->mmio_io); diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c index 9abc086..88fb611 100644 --- a/hw/scsi/vmw_pvscsi.c +++ b/hw/scsi/vmw_pvscsi.c @@ -1039,22 +1039,24 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size) } -static bool +static void pvscsi_init_msi(PVSCSIState *s) { int res; + Error *err = NULL; PCIDevice *d = PCI_DEVICE(s); res = msi_init(d, PVSCSI_MSI_OFFSET(s), PVSCSI_MSIX_NUM_VECTORS, - PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK); + PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, &err); if (res < 0) { trace_pvscsi_init_msi_fail(res); + error_append_hint(&err, "MSI capability fail to init," + " will use INTx intead"); + error_report_err(err); s->msi_used = false; } else { s->msi_used = true; } - - return s->msi_used; } static void diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index bcde8a2..f132a57 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3588,6 +3588,13 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) usb_xhci_init(xhci); + if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) { + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, errp); + if (ret < 0) { + return; + } + } + if (xhci->numintrs > MAXINTRS) { xhci->numintrs = MAXINTRS; } @@ -3645,9 +3652,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp) assert(ret >= 0); } - if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) { - msi_init(dev, 0x70, xhci->numintrs, true, false); - } if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) { msix_init(dev, xhci->numintrs, &xhci->mem, 0, OFF_MSIX_TABLE, diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index d091d8c..55ceb67 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1171,6 +1171,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos) uint16_t ctrl; bool msi_64bit, msi_maskbit; int ret, entries; + Error *err = NULL; if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) { @@ -1184,12 +1185,13 @@ 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, &err); if (ret < 0) { if (ret == -ENOTSUP) { return 0; } - error_report("vfio: msi_init failed"); + error_prepend(&err, "vfio: msi_init failed: "); + error_report_err(err); return ret; } vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 : 0); diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h index 8124908..4837bcf 100644 --- a/include/hw/pci/msi.h +++ b/include/hw/pci/msi.h @@ -35,7 +35,8 @@ 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); + 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);
Add param Error **errp, and change pci_add_capability() to pci_add_capability2(), because pci_add_capability() report error, and msi_init() is widely used in realize(), so it is not suitable for realize(). Also fix all the callers who should deal with the msi_init() failure but actually not. The affected devices are as following: 1. intel hd audio: move msi_init() above, save the strength to free the MemoryRegion when it fails. 2. usb-xhci: move msi_init() above, save the strength to free the MemoryRegion when it fails. 3. ich9 ahci: it is a on-board device created during machine initialization, when it fail, qemu will exit, so, no need to free resource manually. 4. megasas_scsi: move msi_init() above, save the strength to free the MemoryRegion when it fails . Also fix a bug: when user enable msi, and msi_init success(return positive offset value), the code disable msi in the flags. But it seems this bug doesn`t do anything bad. 5. mptsas: when msi_init() fail, it will use INTx. So, catch the error & report it right there, don`t propagate it. Move msi_init() above, save the strength to free the MemoryRegion when it fails. 6. pci_bridge_dev/ioh3420/xio3130_downstream/xio3130_upstream: catch error & report it right there. 7. vmxnet3: move msi_init() above, save the strength to free the MemoryRegion when it fails. Remove the unecessary vmxnet3_init_msi(). When msi_init() fail, it will use INTx, so msi_init()`s failure should not break the realize. Just catch & report msi_init()`s failure message. 8. pvscsi: when msi_init fail, it will use INTx. so msi_init failure should not break the realize. Report the error when msi_init fail. Nobody use the return value of pvscsi_init_msi(), so change its type to void. 9. vfio-pci: it ignores the config space corruption error, so, catch & report it right there. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- The patch has been compiled. No further test. hw/audio/intel-hda.c | 11 +++++++--- hw/ide/ich.c | 2 +- hw/net/vmxnet3.c | 41 ++++++++++++++------------------------ hw/pci-bridge/ioh3420.c | 6 +++++- hw/pci-bridge/pci_bridge_dev.c | 8 +++++++- hw/pci-bridge/xio3130_downstream.c | 7 ++++++- hw/pci-bridge/xio3130_upstream.c | 7 ++++++- hw/pci/msi.c | 23 +++++++++++++++++++-- hw/scsi/megasas.c | 12 +++++++---- hw/scsi/mptsas.c | 17 +++++++++++----- hw/scsi/vmw_pvscsi.c | 10 ++++++---- hw/usb/hcd-xhci.c | 10 +++++++--- hw/vfio/pci.c | 6 ++++-- include/hw/pci/msi.h | 3 ++- 14 files changed, 108 insertions(+), 55 deletions(-)