* [Qemu-devel] [PATCH v8 01/17] pci core: assert ENOSPC when add capability
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 02/17] fix some coding style problems Cao jin
` (17 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum, Markus Armbruster
ENOSPC is programming error, assert it for debugging.
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/pci/pci.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 06d3117..26f387c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2152,10 +2152,8 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
if (!offset) {
offset = pci_find_space(pdev, size);
- if (!offset) {
- error_setg(errp, "out of PCI config space");
- return -ENOSPC;
- }
+ /* out of PCI config space is programming error */
+ assert(offset);
} else {
/* Verify that capabilities don't overlap. Note: device assignment
* depends on this check to verify that the device is not broken.
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 02/17] fix some coding style problems
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 01/17] pci core: assert ENOSPC when add capability Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 03/17] change pvscsi_init_msi() type to void Cao jin
` (16 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Markus Armbruster, Marcel Apfelbaum,
Dmitry Fleytman, Jason Wang
It has:
1. More newlines make the code block well separated.
2. Add more comments for msi_init.
3. Fix a indentation in vmxnet3.c.
4. ioh3420 & xio3130_downstream: put PCI Express capability init function
together, make it more readable.
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/net/vmxnet3.c | 2 +-
hw/pci-bridge/ioh3420.c | 7 ++++++-
hw/pci-bridge/pci_bridge_dev.c | 4 ++++
hw/pci-bridge/xio3130_downstream.c | 6 +++++-
hw/pci-bridge/xio3130_upstream.c | 3 +++
hw/pci/msi.c | 16 ++++++++++++++++
6 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 16645e6..d978976 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)
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 0937fa3..b4a7806 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -106,12 +106,14 @@ 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);
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;
@@ -120,18 +122,21 @@ static int ioh3420_initfn(PCIDevice *d)
pcie_cap_arifwd_init(d);
pcie_cap_deverr_init(d);
pcie_cap_slot_init(d, s->slot);
+ pcie_cap_root_init(d);
+
pcie_chassis_create(s->chassis);
rc = pcie_chassis_add_slot(s);
if (rc < 0) {
goto err_pcie_cap;
}
- pcie_cap_root_init(d);
+
rc = pcie_aer_init(d, IOH_EP_AER_OFFSET, PCI_ERR_SIZEOF);
if (rc < 0) {
goto err;
}
pcie_aer_root_init(d);
ioh3420_aer_vector_update(d);
+
return 0;
err:
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 7b582e9..41ca47b 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -68,10 +68,12 @@ 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);
@@ -79,6 +81,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
goto msi_error;
}
}
+
if (shpc_present(dev)) {
/* TODO: spec recommends using 64 bit prefetcheable BAR.
* Check whether that works well. */
@@ -86,6 +89,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..e6d653d 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -70,11 +70,13 @@ static int xio3130_downstream_initfn(PCIDevice *d)
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) {
@@ -83,12 +85,14 @@ static int xio3130_downstream_initfn(PCIDevice *d)
pcie_cap_flr_init(d);
pcie_cap_deverr_init(d);
pcie_cap_slot_init(d, s->slot);
+ pcie_cap_arifwd_init(d);
+
pcie_chassis_create(s->chassis);
rc = pcie_chassis_add_slot(s);
if (rc < 0) {
goto err_pcie_cap;
}
- pcie_cap_arifwd_init(d);
+
rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
if (rc < 0) {
goto err;
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 164ef58..d976844 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -66,11 +66,13 @@ static int xio3130_upstream_initfn(PCIDevice *d)
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) {
@@ -78,6 +80,7 @@ static int xio3130_upstream_initfn(PCIDevice *d)
}
pcie_cap_flr_init(d);
pcie_cap_deverr_init(d);
+
rc = pcie_aer_init(d, XIO3130_AER_OFFSET, PCI_ERR_SIZEOF);
if (rc < 0) {
goto err;
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87ef4d..359058e 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -165,6 +165,22 @@ 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.
+ * Return the offset of capability MSI in config space on success,
+ * return -errno on error.
+ *
+ * -ENOTSUP means lacking msi support for a msi-capable platform.
+ * -EINVAL means capability overlap, happens when @offset is non-zero,
+ * also means a programming error, except device assignment, which can check
+ * if a real HW is broken.
+ */
int msi_init(struct PCIDevice *dev, uint8_t offset,
unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 03/17] change pvscsi_init_msi() type to void
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 01/17] pci core: assert ENOSPC when add capability Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 02/17] fix some coding style problems Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 04/17] megasas: Fix check for msi_init() failure Cao jin
` (15 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Markus Armbruster, Marcel Apfelbaum
Nobody use its return value, so change the type to void.
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Dmitry Fleytman <dmitry@daynix.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/vmw_pvscsi.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 2d7528d..e035fce 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1056,7 +1056,7 @@ pvscsi_io_read(void *opaque, hwaddr addr, unsigned size)
}
-static bool
+static void
pvscsi_init_msi(PVSCSIState *s)
{
int res;
@@ -1070,8 +1070,6 @@ pvscsi_init_msi(PVSCSIState *s)
} else {
s->msi_used = true;
}
-
- return s->msi_used;
}
static void
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 04/17] megasas: Fix check for msi_init() failure
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (2 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 03/17] change pvscsi_init_msi() type to void Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 05/17] mptsas: change .realize function name Cao jin
` (14 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Marcel Apfelbaum, Markus Armbruster
msi_init returns non-zero value on both failure and success.
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/megasas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index a9ffc32..561e372 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2351,7 +2351,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) < 0) {
s->flags &= ~MEGASAS_MASK_USE_MSI;
}
if (megasas_use_msix(s) &&
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 05/17] mptsas: change .realize function name
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (3 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 04/17] megasas: Fix check for msi_init() failure Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 06/17] usb xhci: change msi/msix property type Cao jin
` (13 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Markus Armbruster
All the other devices` .realize function name are xxx_realize, except this one.
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/mptsas.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index be88e16..ae8514e 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1269,7 +1269,7 @@ static const struct SCSIBusInfo mptsas_scsi_info = {
.load_request = mptsas_load_request,
};
-static void mptsas_scsi_init(PCIDevice *dev, Error **errp)
+static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
{
DeviceState *d = DEVICE(dev);
MPTSASState *s = MPT_SAS(dev);
@@ -1412,7 +1412,7 @@ static void mptsas1068_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
- pc->realize = mptsas_scsi_init;
+ pc->realize = mptsas_scsi_realize;
pc->exit = mptsas_scsi_uninit;
pc->romfile = 0;
pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 06/17] usb xhci: change msi/msix property type
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (4 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 05/17] mptsas: change .realize function name Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 07/17] intel-hda: change msi " Cao jin
` (12 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Gerd Hoffmann, Michael S. Tsirkin, Markus Armbruster,
Marcel Apfelbaum
>From bit to enum OnOffAuto
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/usb/hcd-xhci.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 43ba615..0a5510f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -461,6 +461,8 @@ struct XHCIState {
uint32_t numslots;
uint32_t flags;
uint32_t max_pstreams_mask;
+ OnOffAuto msi;
+ OnOffAuto msix;
/* Operational Registers */
uint32_t usbcmd;
@@ -498,9 +500,7 @@ typedef struct XHCIEvRingSeg {
} XHCIEvRingSeg;
enum xhci_flags {
- XHCI_FLAG_USE_MSI = 1,
- XHCI_FLAG_USE_MSI_X,
- XHCI_FLAG_SS_FIRST,
+ XHCI_FLAG_SS_FIRST = 1,
XHCI_FLAG_FORCE_PCIE_ENDCAP,
XHCI_FLAG_ENABLE_STREAMS,
};
@@ -3648,10 +3648,12 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
assert(ret >= 0);
}
- if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI)) {
+ if (xhci->msi != ON_OFF_AUTO_OFF) {
+ /* TODO check for errors */
msi_init(dev, 0x70, xhci->numintrs, true, false);
}
- if (xhci_get_flag(xhci, XHCI_FLAG_USE_MSI_X)) {
+ if (xhci->msix != ON_OFF_AUTO_OFF) {
+ /* TODO check for errors */
msix_init(dev, xhci->numintrs,
&xhci->mem, 0, OFF_MSIX_TABLE,
&xhci->mem, 0, OFF_MSIX_PBA,
@@ -3872,8 +3874,8 @@ static const VMStateDescription vmstate_xhci = {
};
static Property xhci_properties[] = {
- DEFINE_PROP_BIT("msi", XHCIState, flags, XHCI_FLAG_USE_MSI, true),
- DEFINE_PROP_BIT("msix", XHCIState, flags, XHCI_FLAG_USE_MSI_X, true),
+ DEFINE_PROP_ON_OFF_AUTO("msi", XHCIState, msi, ON_OFF_AUTO_AUTO),
+ DEFINE_PROP_ON_OFF_AUTO("msix", XHCIState, msix, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT("superspeed-ports-first",
XHCIState, flags, XHCI_FLAG_SS_FIRST, true),
DEFINE_PROP_BIT("force-pcie-endcap", XHCIState, flags,
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 07/17] intel-hda: change msi property type
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (5 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 06/17] usb xhci: change msi/msix property type Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 08/17] mptsas: " Cao jin
` (11 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Gerd Hoffmann, Michael S. Tsirkin, Markus Armbruster,
Marcel Apfelbaum
>From uint32 to enum OnOffAuto. msi_enabled() should be enough to check device
msi state, and change local variant type from int to bool.
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/audio/intel-hda.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 93d7669..6b4dda0 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -191,7 +191,7 @@ struct IntelHDAState {
/* properties */
uint32_t debug;
- uint32_t msi;
+ OnOffAuto msi;
bool old_msi_addr;
};
@@ -259,7 +259,7 @@ static void intel_hda_update_int_sts(IntelHDAState *d)
static void intel_hda_update_irq(IntelHDAState *d)
{
- int msi = d->msi && msi_enabled(&d->pci);
+ bool msi = msi_enabled(&d->pci);
int level;
intel_hda_update_int_sts(d);
@@ -1146,7 +1146,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
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) {
+ if (d->msi != ON_OFF_AUTO_OFF) {
+ /* TODO check for errors */
msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60, 1, true, false);
}
@@ -1238,7 +1239,7 @@ static const VMStateDescription vmstate_intel_hda = {
static Property intel_hda_properties[] = {
DEFINE_PROP_UINT32("debug", IntelHDAState, debug, 0),
- DEFINE_PROP_UINT32("msi", IntelHDAState, msi, 1),
+ DEFINE_PROP_ON_OFF_AUTO("msi", IntelHDAState, msi, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BOOL("old_msi_addr", IntelHDAState, old_msi_addr, false),
DEFINE_PROP_END_OF_LIST(),
};
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 08/17] mptsas: change msi property type
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (6 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 07/17] intel-hda: change msi " Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 09/17] megasas: change msi/msix " Cao jin
` (10 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S. Tsirkin, Markus Armbruster,
Marcel Apfelbaum
>From uint32 to enum OnOffAuto, and give it a shorter name.
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/mptsas.c | 5 +++--
hw/scsi/mptsas.h | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index ae8514e..dfbc0c4 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1284,8 +1284,9 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
"mptsas-diag", 0x10000);
- if (s->msi_available &&
+ if (s->msi != ON_OFF_AUTO_OFF &&
msi_init(dev, 0, 1, true, false) >= 0) {
+ /* TODO check for errors */
s->msi_in_use = true;
}
@@ -1403,7 +1404,7 @@ static const VMStateDescription vmstate_mptsas = {
static Property mptsas_properties[] = {
DEFINE_PROP_UINT64("sas_address", MPTSASState, sas_addr, 0),
/* TODO: test MSI support under Windows */
- DEFINE_PROP_BIT("msi", MPTSASState, msi_available, 0, true),
+ DEFINE_PROP_ON_OFF_AUTO("msi", MPTSASState, msi, ON_OFF_AUTO_AUTO),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/scsi/mptsas.h b/hw/scsi/mptsas.h
index 595f81f..0436a33 100644
--- a/hw/scsi/mptsas.h
+++ b/hw/scsi/mptsas.h
@@ -27,7 +27,8 @@ struct MPTSASState {
MemoryRegion diag_io;
QEMUBH *request_bh;
- uint32_t msi_available;
+ /* properties */
+ OnOffAuto msi;
uint64_t sas_addr;
bool msi_in_use;
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 09/17] megasas: change msi/msix property type
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (7 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 08/17] mptsas: " Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 10/17] pci bridge dev: change msi " Cao jin
` (9 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Hannes Reinecke, Paolo Bonzini, Michael S. Tsirkin,
Markus Armbruster, Marcel Apfelbaum
>From bit to enum OnOffAuto.
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/megasas.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 561e372..2ede192 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -48,11 +48,7 @@
#define MEGASAS_FLAG_USE_JBOD 0
#define MEGASAS_MASK_USE_JBOD (1 << MEGASAS_FLAG_USE_JBOD)
-#define MEGASAS_FLAG_USE_MSI 1
-#define MEGASAS_MASK_USE_MSI (1 << MEGASAS_FLAG_USE_MSI)
-#define MEGASAS_FLAG_USE_MSIX 2
-#define MEGASAS_MASK_USE_MSIX (1 << MEGASAS_FLAG_USE_MSIX)
-#define MEGASAS_FLAG_USE_QUEUE64 3
+#define MEGASAS_FLAG_USE_QUEUE64 1
#define MEGASAS_MASK_USE_QUEUE64 (1 << MEGASAS_FLAG_USE_QUEUE64)
static const char *mfi_frame_desc[] = {
@@ -96,6 +92,8 @@ typedef struct MegasasState {
int busy;
int diag;
int adp_reset;
+ OnOffAuto msi;
+ OnOffAuto msix;
MegasasCmd *event_cmd;
int event_locale;
@@ -159,12 +157,12 @@ static bool megasas_use_queue64(MegasasState *s)
static bool megasas_use_msi(MegasasState *s)
{
- return s->flags & MEGASAS_MASK_USE_MSI;
+ return s->msi != ON_OFF_AUTO_OFF;
}
static bool megasas_use_msix(MegasasState *s)
{
- return s->flags & MEGASAS_MASK_USE_MSIX;
+ return s->msix != ON_OFF_AUTO_OFF;
}
static bool megasas_is_jbod(MegasasState *s)
@@ -2352,12 +2350,12 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
if (megasas_use_msi(s) &&
msi_init(dev, 0x50, 1, true, false) < 0) {
- s->flags &= ~MEGASAS_MASK_USE_MSI;
+ s->msi = ON_OFF_AUTO_OFF;
}
if (megasas_use_msix(s) &&
msix_init(dev, 15, &s->mmio_io, b->mmio_bar, 0x2000,
&s->mmio_io, b->mmio_bar, 0x3800, 0x68)) {
- s->flags &= ~MEGASAS_MASK_USE_MSIX;
+ s->msix = ON_OFF_AUTO_OFF;
}
if (pci_is_express(dev)) {
pcie_endpoint_cap_init(dev, 0xa0);
@@ -2425,10 +2423,8 @@ static Property megasas_properties_gen1[] = {
MEGASAS_DEFAULT_FRAMES),
DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
- DEFINE_PROP_BIT("use_msi", MegasasState, flags,
- MEGASAS_FLAG_USE_MSI, false),
- DEFINE_PROP_BIT("use_msix", MegasasState, flags,
- MEGASAS_FLAG_USE_MSIX, false),
+ DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
+ DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
MEGASAS_FLAG_USE_JBOD, false),
DEFINE_PROP_END_OF_LIST(),
@@ -2441,10 +2437,8 @@ static Property megasas_properties_gen2[] = {
MEGASAS_GEN2_DEFAULT_FRAMES),
DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial),
DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0),
- DEFINE_PROP_BIT("use_msi", MegasasState, flags,
- MEGASAS_FLAG_USE_MSI, true),
- DEFINE_PROP_BIT("use_msix", MegasasState, flags,
- MEGASAS_FLAG_USE_MSIX, true),
+ DEFINE_PROP_ON_OFF_AUTO("msi", MegasasState, msi, ON_OFF_AUTO_AUTO),
+ DEFINE_PROP_ON_OFF_AUTO("msix", MegasasState, msix, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT("use_jbod", MegasasState, flags,
MEGASAS_FLAG_USE_JBOD, false),
DEFINE_PROP_END_OF_LIST(),
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 10/17] pci bridge dev: change msi property type
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (8 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 09/17] megasas: change msi/msix " Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-13 9:35 ` Marcel Apfelbaum
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 11/17] msi_init: change return value to 0 on success Cao jin
` (8 subsequent siblings)
18 siblings, 1 reply; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Markus Armbruster, Marcel Apfelbaum
>From bit to enum OnOffAuto.
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/pci-bridge/pci_bridge_dev.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 41ca47b..0fbecc4 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -42,9 +42,10 @@ struct PCIBridgeDev {
MemoryRegion bar;
uint8_t chassis_nr;
-#define PCI_BRIDGE_DEV_F_MSI_REQ 0
-#define PCI_BRIDGE_DEV_F_SHPC_REQ 1
+#define PCI_BRIDGE_DEV_F_SHPC_REQ 0
uint32_t flags;
+
+ OnOffAuto msi;
};
typedef struct PCIBridgeDev PCIBridgeDev;
@@ -66,7 +67,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
}
} else {
/* MSI is not applicable without SHPC */
- bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
+ bridge_dev->msi = ON_OFF_AUTO_OFF;
}
err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
@@ -74,7 +75,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
goto slotid_error;
}
- if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
+ if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
msi_nonbroken) {
err = msi_init(dev, 0, 1, true, true);
if (err < 0) {
@@ -147,8 +148,8 @@ static Property pci_bridge_dev_properties[] = {
/* Note: 0 is not a legal chassis number. */
DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
0),
- DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
- PCI_BRIDGE_DEV_F_MSI_REQ, true),
+ DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
+ ON_OFF_AUTO_AUTO),
DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
PCI_BRIDGE_DEV_F_SHPC_REQ, true),
DEFINE_PROP_END_OF_LIST(),
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 10/17] pci bridge dev: change msi property type
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 10/17] pci bridge dev: change msi " Cao jin
@ 2016-06-13 9:35 ` Marcel Apfelbaum
0 siblings, 0 replies; 35+ messages in thread
From: Marcel Apfelbaum @ 2016-06-13 9:35 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: Michael S. Tsirkin, Markus Armbruster
On 06/10/2016 12:54 PM, Cao jin wrote:
> From bit to enum OnOffAuto.
>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/pci-bridge/pci_bridge_dev.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 41ca47b..0fbecc4 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -42,9 +42,10 @@ struct PCIBridgeDev {
>
> MemoryRegion bar;
> uint8_t chassis_nr;
> -#define PCI_BRIDGE_DEV_F_MSI_REQ 0
> -#define PCI_BRIDGE_DEV_F_SHPC_REQ 1
> +#define PCI_BRIDGE_DEV_F_SHPC_REQ 0
> uint32_t flags;
> +
> + OnOffAuto msi;
> };
> typedef struct PCIBridgeDev PCIBridgeDev;
>
> @@ -66,7 +67,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> }
> } else {
> /* MSI is not applicable without SHPC */
> - bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
> + bridge_dev->msi = ON_OFF_AUTO_OFF;
> }
>
> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
> @@ -74,7 +75,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> goto slotid_error;
> }
>
> - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> + if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
> msi_nonbroken) {
> err = msi_init(dev, 0, 1, true, true);
> if (err < 0) {
> @@ -147,8 +148,8 @@ static Property pci_bridge_dev_properties[] = {
> /* Note: 0 is not a legal chassis number. */
> DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
> 0),
> - DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
> - PCI_BRIDGE_DEV_F_MSI_REQ, true),
> + DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
> + ON_OFF_AUTO_AUTO),
> DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
> PCI_BRIDGE_DEV_F_SHPC_REQ, true),
> DEFINE_PROP_END_OF_LIST(),
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v8 11/17] msi_init: change return value to 0 on success
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (9 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 10/17] pci bridge dev: change msi " Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-13 9:38 ` Marcel Apfelbaum
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it Cao jin
` (7 subsequent siblings)
18 siblings, 1 reply; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Paolo Bonzini, Hannes Reinecke,
Markus Armbruster, Marcel Apfelbaum
No caller use its return value as msi capability offset, also in order
to make its return behaviour consistent with msix_init().
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Hannes Reinecke <hare@suse.de>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Acked-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/pci/msi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Hi Hannes,
This version changed, If is ok with you, I will let your A-b still here.
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 359058e..ed79225 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -173,8 +173,7 @@ bool msi_enabled(const PCIDevice *dev)
* 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.
- * Return the offset of capability MSI in config space on success,
- * return -errno on error.
+ * Return 0 on success, return -errno on error.
*
* -ENOTSUP means lacking msi support for a msi-capable platform.
* -EINVAL means capability overlap, happens when @offset is non-zero,
@@ -236,7 +235,8 @@ 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;
+
+ return 0;
}
void msi_uninit(struct PCIDevice *dev)
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 11/17] msi_init: change return value to 0 on success
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 11/17] msi_init: change return value to 0 on success Cao jin
@ 2016-06-13 9:38 ` Marcel Apfelbaum
0 siblings, 0 replies; 35+ messages in thread
From: Marcel Apfelbaum @ 2016-06-13 9:38 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: Michael S. Tsirkin, Paolo Bonzini, Hannes Reinecke,
Markus Armbruster
On 06/10/2016 12:54 PM, Cao jin wrote:
> No caller use its return value as msi capability offset, also in order
> to make its return behaviour consistent with msix_init().
>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Acked-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/pci/msi.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> Hi Hannes,
> This version changed, If is ok with you, I will let your A-b still here.
>
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index 359058e..ed79225 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -173,8 +173,7 @@ bool msi_enabled(const PCIDevice *dev)
> * 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.
> - * Return the offset of capability MSI in config space on success,
> - * return -errno on error.
> + * Return 0 on success, return -errno on error.
> *
> * -ENOTSUP means lacking msi support for a msi-capable platform.
> * -EINVAL means capability overlap, happens when @offset is non-zero,
> @@ -236,7 +235,8 @@ 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;
> +
> + return 0;
> }
>
> void msi_uninit(struct PCIDevice *dev)
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (10 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 11/17] msi_init: change return value to 0 on success Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-13 10:16 ` Marcel Apfelbaum
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 13/17] megasas: remove unnecessary megasas_use_msi() Cao jin
` (6 subsequent siblings)
18 siblings, 1 reply; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster, Marcel Apfelbaum
msi_init() reports errors with error_report(), which is wrong
when it's used in realize().
Fix by converting it to Error.
Fix its callers to handle failure instead of ignoring it.
For those callers who don't handle the failure, it might happen:
when user want msi on, but he doesn't get what he want because of
msi_init fails silently.
cc: Gerd Hoffmann <kraxel@redhat.com>
cc: John Snow <jsnow@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Alex Williamson <alex.williamson@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/audio/intel-hda.c | 24 ++++++++++++++++++++----
hw/ide/ich.c | 15 +++++++++------
hw/net/e1000e.c | 8 ++------
hw/net/vmxnet3.c | 37 ++++++++++++-------------------------
hw/pci-bridge/ioh3420.c | 6 +++++-
hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++----
hw/pci-bridge/xio3130_downstream.c | 6 +++++-
hw/pci-bridge/xio3130_upstream.c | 6 +++++-
hw/pci/msi.c | 11 ++++++++---
hw/scsi/megasas.c | 26 +++++++++++++++++++++-----
hw/scsi/mptsas.c | 31 ++++++++++++++++++++++++-------
hw/scsi/vmw_pvscsi.c | 2 +-
hw/usb/hcd-xhci.c | 23 +++++++++++++++++++----
hw/vfio/pci.c | 7 +++++--
include/hw/pci/msi.h | 3 ++-
15 files changed, 154 insertions(+), 71 deletions(-)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 6b4dda0..82101f8 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
{
IntelHDAState *d = INTEL_HDA(pci);
uint8_t *conf = d->pci.config;
+ Error *err = NULL;
+ int ret;
d->name = object_get_typename(OBJECT(d));
@@ -1143,13 +1145,27 @@ 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 != ON_OFF_AUTO_OFF) {
+ ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
+ 1, true, false, &err);
+ /* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error */
+ assert(!ret || ret == -ENOTSUP);
+ if (ret && d->msi == ON_OFF_AUTO_ON) {
+ /* Can't satisfy user's explicit msi=on request, fail */
+ error_append_hint(&err, "You have to use msi=auto (default) or "
+ "msi=off with this machine type.\n");
+ error_propagate(errp, err);
+ return;
+ }
+ assert(!err || d->msi == ON_OFF_AUTO_AUTO);
+ /* With msi=auto, we fall back to MSI off silently */
+ error_free(err);
+ }
+
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 != ON_OFF_AUTO_OFF) {
- /* TODO check for errors */
- 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..084bef8 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -68,7 +68,6 @@
#include <hw/isa/isa.h>
#include "sysemu/block-backend.h"
#include "sysemu/dma.h"
-
#include <hw/ide/pci.h>
#include <hw/ide/ahci.h>
@@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
int sata_cap_offset;
uint8_t *sata_cap;
d = ICH_AHCI(dev);
+ int ret;
+
+ /* 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. */
+ ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
+ /* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error. Fall back to INTx silently on -ENOTSUP */
+ assert(!ret || ret == -ENOTSUP);
ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
@@ -142,11 +150,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
pci_set_long(sata_cap + SATA_CAP_BAR,
(ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4));
d->ahci.idp_offset = ICH9_IDP_INDEX;
-
- /* 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);
}
static void pci_ich9_uninit(PCIDevice *dev)
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 692283f..a06d184 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
{
int res;
- res = msi_init(PCI_DEVICE(s),
- 0xD0, /* MSI capability offset */
- 1, /* MAC MSI interrupts */
- true, /* 64-bit message addresses supported */
- false); /* Per vector mask supported */
+ res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
- if (res > 0) {
+ if (!res) {
s->intr_state |= E1000E_USE_MSI;
} else {
trace_e1000e_msi_init_fail(res);
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index d978976..63f8904 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2197,27 +2197,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)
{
@@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s)
return dsn_payload;
}
+
+#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);
VMXNET3State *s = VMXNET3(pci_dev);
+ int ret;
VMW_CBPRN("Starting init...");
@@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
/* Interrupt pin A */
pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
+ ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
+ VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, NULL);
+ /* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error. Fall back to INTx silently on -ENOTSUP */
+ assert(!ret || ret == -ENOTSUP);
+ s->msi_used = !ret;
+
if (!vmxnet3_init_msix(s)) {
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 b4a7806..93c6f0b 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -25,6 +25,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/pcie.h"
#include "ioh3420.h"
+#include "qapi/error.h"
#define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */
#define PCI_DEVICE_ID_IOH_REV 0x2
@@ -97,6 +98,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);
@@ -109,8 +111,10 @@ static int ioh3420_initfn(PCIDevice *d)
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) {
+ assert(rc == -ENOTSUP);
+ error_report_err(err);
goto err_bridge;
}
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 0fbecc4..c4d2c0b 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -54,6 +54,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);
@@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
goto slotid_error;
}
- if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
- msi_nonbroken) {
- err = msi_init(dev, 0, 1, true, true);
- if (err < 0) {
+ if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
+ /* it means SHPC exists, because SHPC is required for MSI */
+
+ err = msi_init(dev, 0, 1, true, true, &local_err);
+ /* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error */
+ assert(!err || err == -ENOTSUP);
+ if (err && bridge_dev->msi == ON_OFF_AUTO_ON) {
+ /* Can't satisfy user's explicit msi=on request, fail */
+ error_append_hint(&local_err, "You have to use msi=auto (default) "
+ "or msi=off with this machine type.\n");
+ error_report_err(local_err);
goto msi_error;
}
+ assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO);
+ /* With msi=auto, we fall back to MSI off silently */
+ error_free(local_err);
}
if (shpc_present(dev)) {
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index e6d653d..f6149a3 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -24,6 +24,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/pcie.h"
#include "xio3130_downstream.h"
+#include "qapi/error.h"
#define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */
#define XIO3130_REVISION 0x1
@@ -60,14 +61,17 @@ 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) {
+ assert(rc == -ENOTSUP);
+ error_report_err(err);
goto err_bridge;
}
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index d976844..487edac 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -24,6 +24,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/pcie.h"
#include "xio3130_upstream.h"
+#include "qapi/error.h"
#define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */
#define XIO3130_REVISION 0x2
@@ -56,14 +57,17 @@ 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) {
+ assert(rc == -ENOTSUP);
+ error_report_err(err);
goto err_bridge;
}
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index ed79225..a87b227 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -22,6 +22,7 @@
#include "hw/pci/msi.h"
#include "hw/xen/xen.h"
#include "qemu/range.h"
+#include "qapi/error.h"
/* PCI_MSI_ADDRESS_LO */
#define PCI_MSI_ADDRESS_LO_MASK (~0x3)
@@ -173,7 +174,8 @@ bool msi_enabled(const PCIDevice *dev)
* 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.
- * Return 0 on success, return -errno on error.
+ * @errp is for returning errors.
+ * Return 0 on success; set @errp and return -errno on error.
*
* -ENOTSUP means lacking msi support for a msi-capable platform.
* -EINVAL means capability overlap, happens when @offset is non-zero,
@@ -181,7 +183,8 @@ bool msi_enabled(const PCIDevice *dev)
* if a real HW is broken.
*/
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;
@@ -189,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
int config_offset;
if (!msi_nonbroken) {
+ error_setg(errp, "MSI is not supported by interrupt controller");
return -ENOTSUP;
}
@@ -212,7 +216,8 @@ 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;
}
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 2ede192..345783d 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -29,7 +29,7 @@
#include "hw/scsi/scsi.h"
#include "block/scsi.h"
#include "trace.h"
-
+#include "qapi/error.h"
#include "mfi.h"
#define MEGASAS_VERSION_GEN1 "1.70"
@@ -2333,6 +2333,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
uint8_t *pci_conf;
int i, bar_type;
+ Error *err = NULL;
+ int ret;
pci_conf = dev->config;
@@ -2341,6 +2343,24 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
/* Interrupt pin 1 */
pci_conf[PCI_INTERRUPT_PIN] = 0x01;
+ if (megasas_use_msi(s)) {
+ ret = msi_init(dev, 0x50, 1, true, false, &err);
+ /* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error */
+ assert(!ret || ret == -ENOTSUP);
+ if (ret && s->msi == ON_OFF_AUTO_ON) {
+ /* Can't satisfy user's explicit msi=on request, fail */
+ error_append_hint(&err, "You have to use msi=auto (default) or "
+ "msi=off with this machine type.\n");
+ error_propagate(errp, err);
+ return;
+ } else if (ret) {
+ /* With msi=auto, we fall back to MSI off silently */
+ s->msi = ON_OFF_AUTO_OFF;
+ error_free(err);
+ }
+ }
+
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,
@@ -2348,10 +2368,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) < 0) {
- s->msi = ON_OFF_AUTO_OFF;
- }
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 dfbc0c4..698be42 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -32,7 +32,7 @@
#include "hw/scsi/scsi.h"
#include "block/scsi.h"
#include "trace.h"
-
+#include "qapi/error.h"
#include "mptsas.h"
#include "mpi.h"
@@ -1273,10 +1273,33 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
{
DeviceState *d = DEVICE(dev);
MPTSASState *s = MPT_SAS(dev);
+ Error *err = NULL;
+ int ret;
dev->config[PCI_LATENCY_TIMER] = 0;
dev->config[PCI_INTERRUPT_PIN] = 0x01;
+ if (s->msi != ON_OFF_AUTO_OFF) {
+ ret = msi_init(dev, 0, 1, true, false, &err);
+ /* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error */
+ assert(!ret || ret == -ENOTSUP);
+ if (ret && s->msi == ON_OFF_AUTO_ON) {
+ /* Can't satisfy user's explicit msi=on request, fail */
+ error_append_hint(&err, "You have to use msi=auto (default) or "
+ "msi=off with this machine type.\n");
+ error_propagate(errp, err);
+ s->msi_in_use = false;
+ return;
+ } else if (ret) {
+ /* With msi=auto, we fall back to MSI off silently */
+ error_free(err);
+ s->msi_in_use = false;
+ } else {
+ s->msi_in_use = true;
+ }
+ }
+
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,
@@ -1284,12 +1307,6 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
"mptsas-diag", 0x10000);
- if (s->msi != ON_OFF_AUTO_OFF &&
- msi_init(dev, 0, 1, true, false) >= 0) {
- /* TODO check for errors */
- 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 e035fce..ecd6077 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1063,7 +1063,7 @@ pvscsi_init_msi(PVSCSIState *s)
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, NULL);
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 0a5510f..1a3377f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -26,6 +26,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/msix.h"
#include "trace.h"
+#include "qapi/error.h"
//#define DEBUG_XHCI
//#define DEBUG_DATA
@@ -3581,6 +3582,7 @@ static void usb_xhci_init(XHCIState *xhci)
static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
{
int i, ret;
+ Error *err = NULL;
XHCIState *xhci = XHCI(dev);
@@ -3591,6 +3593,23 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
usb_xhci_init(xhci);
+ if (xhci->msi != ON_OFF_AUTO_OFF) {
+ ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
+ /* Any error other than -ENOTSUP(board's MSI support is broken)
+ * is a programming error */
+ assert(!ret || ret == -ENOTSUP);
+ if (ret && xhci->msi == ON_OFF_AUTO_ON) {
+ /* Can't satisfy user's explicit msi=on request, fail */
+ error_append_hint(&err, "You have to use msi=auto (default) or "
+ "msi=off with this machine type.\n");
+ error_propagate(errp, err);
+ return;
+ }
+ assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
+ /* With msi=auto, we fall back to MSI off silently */
+ error_free(err);
+ }
+
if (xhci->numintrs > MAXINTRS) {
xhci->numintrs = MAXINTRS;
}
@@ -3648,10 +3667,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
assert(ret >= 0);
}
- if (xhci->msi != ON_OFF_AUTO_OFF) {
- /* TODO check for errors */
- msi_init(dev, 0x70, xhci->numintrs, true, false);
- }
if (xhci->msix != ON_OFF_AUTO_OFF) {
/* TODO check for errors */
msix_init(dev, xhci->numintrs,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index deab0c6..dfbf8ba 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -32,6 +32,7 @@
#include "sysemu/sysemu.h"
#include "pci.h"
#include "trace.h"
+#include "qapi/error.h"
#define MSIX_CAP_LENGTH 12
@@ -1171,6 +1172,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 +1186,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);
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it Cao jin
@ 2016-06-13 10:16 ` Marcel Apfelbaum
2016-06-13 11:07 ` Markus Armbruster
2016-06-13 11:09 ` Cao jin
0 siblings, 2 replies; 35+ messages in thread
From: Marcel Apfelbaum @ 2016-06-13 10:16 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster
On 06/10/2016 12:54 PM, Cao jin wrote:
> msi_init() reports errors with error_report(), which is wrong
> when it's used in realize().
>
> Fix by converting it to Error.
>
> Fix its callers to handle failure instead of ignoring it.
>
> For those callers who don't handle the failure, it might happen:
> when user want msi on, but he doesn't get what he want because of
> msi_init fails silently.
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/audio/intel-hda.c | 24 ++++++++++++++++++++----
> hw/ide/ich.c | 15 +++++++++------
> hw/net/e1000e.c | 8 ++------
> hw/net/vmxnet3.c | 37 ++++++++++++-------------------------
> hw/pci-bridge/ioh3420.c | 6 +++++-
> hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++----
> hw/pci-bridge/xio3130_downstream.c | 6 +++++-
> hw/pci-bridge/xio3130_upstream.c | 6 +++++-
> hw/pci/msi.c | 11 ++++++++---
> hw/scsi/megasas.c | 26 +++++++++++++++++++++-----
> hw/scsi/mptsas.c | 31 ++++++++++++++++++++++++-------
> hw/scsi/vmw_pvscsi.c | 2 +-
> hw/usb/hcd-xhci.c | 23 +++++++++++++++++++----
> hw/vfio/pci.c | 7 +++++--
> include/hw/pci/msi.h | 3 ++-
> 15 files changed, 154 insertions(+), 71 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 6b4dda0..82101f8 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
> {
> IntelHDAState *d = INTEL_HDA(pci);
> uint8_t *conf = d->pci.config;
> + Error *err = NULL;
> + int ret;
>
> d->name = object_get_typename(OBJECT(d));
>
> @@ -1143,13 +1145,27 @@ 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 != ON_OFF_AUTO_OFF) {
> + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
> + 1, true, false, &err);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error */
> + assert(!ret || ret == -ENOTSUP);
> + if (ret && d->msi == ON_OFF_AUTO_ON) {
> + /* Can't satisfy user's explicit msi=on request, fail */
> + error_append_hint(&err, "You have to use msi=auto (default) or "
> + "msi=off with this machine type.\n");
> + error_propagate(errp, err);
> + return;
> + }
> + assert(!err || d->msi == ON_OFF_AUTO_AUTO);
> + /* With msi=auto, we fall back to MSI off silently */
> + error_free(err);
> + }
> +
> 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 != ON_OFF_AUTO_OFF) {
> - /* TODO check for errors */
> - 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..084bef8 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -68,7 +68,6 @@
> #include <hw/isa/isa.h>
> #include "sysemu/block-backend.h"
> #include "sysemu/dma.h"
> -
> #include <hw/ide/pci.h>
> #include <hw/ide/ahci.h>
>
> @@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
> int sata_cap_offset;
> uint8_t *sata_cap;
> d = ICH_AHCI(dev);
> + int ret;
> +
> + /* 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. */
> + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error. Fall back to INTx silently on -ENOTSUP */
> + assert(!ret || ret == -ENOTSUP);
>
> ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>
> @@ -142,11 +150,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
> pci_set_long(sata_cap + SATA_CAP_BAR,
> (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4));
> d->ahci.idp_offset = ICH9_IDP_INDEX;
> -
> - /* 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);
> }
>
> static void pci_ich9_uninit(PCIDevice *dev)
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 692283f..a06d184 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
> {
> int res;
>
> - res = msi_init(PCI_DEVICE(s),
> - 0xD0, /* MSI capability offset */
> - 1, /* MAC MSI interrupts */
> - true, /* 64-bit message addresses supported */
> - false); /* Per vector mask supported */
> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>
Why did you chose to remove author's comments?
> - if (res > 0) {
> + if (!res) {
> s->intr_state |= E1000E_USE_MSI;
> } else {
> trace_e1000e_msi_init_fail(res);
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index d978976..63f8904 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2197,27 +2197,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)
> {
> @@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s)
> return dsn_payload;
> }
>
> +
> +#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);
> VMXNET3State *s = VMXNET3(pci_dev);
> + int ret;
>
> VMW_CBPRN("Starting init...");
>
> @@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
> /* Interrupt pin A */
> pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
>
> + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, NULL);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error. Fall back to INTx silently on -ENOTSUP */
> + assert(!ret || ret == -ENOTSUP);
> + s->msi_used = !ret;
> +
> if (!vmxnet3_init_msix(s)) {
> 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 b4a7806..93c6f0b 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -25,6 +25,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/pcie.h"
> #include "ioh3420.h"
> +#include "qapi/error.h"
>
> #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */
> #define PCI_DEVICE_ID_IOH_REV 0x2
> @@ -97,6 +98,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);
> @@ -109,8 +111,10 @@ static int ioh3420_initfn(PCIDevice *d)
>
> 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) {
> + assert(rc == -ENOTSUP);
> + error_report_err(err);
> goto err_bridge;
> }
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 0fbecc4..c4d2c0b 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -54,6 +54,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);
>
> @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> goto slotid_error;
> }
>
> - if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
> - msi_nonbroken) {
> - err = msi_init(dev, 0, 1, true, true);
> - if (err < 0) {
> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
> + /* it means SHPC exists, because SHPC is required for MSI */
Is the other way around, MSI is needed by SHPC (but not compulsory)
> +
> + err = msi_init(dev, 0, 1, true, true, &local_err);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error */
> + assert(!err || err == -ENOTSUP);
> + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) {
> + /* Can't satisfy user's explicit msi=on request, fail */
> + error_append_hint(&local_err, "You have to use msi=auto (default) "
> + "or msi=off with this machine type.\n");
> + error_report_err(local_err);
> goto msi_error;
> }
> + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO);
> + /* With msi=auto, we fall back to MSI off silently */
> + error_free(local_err);
> }
>
> if (shpc_present(dev)) {
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index e6d653d..f6149a3 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -24,6 +24,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/pcie.h"
> #include "xio3130_downstream.h"
> +#include "qapi/error.h"
>
> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */
> #define XIO3130_REVISION 0x1
> @@ -60,14 +61,17 @@ 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) {
> + assert(rc == -ENOTSUP);
> + error_report_err(err);
> goto err_bridge;
> }
>
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index d976844..487edac 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -24,6 +24,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/pcie.h"
> #include "xio3130_upstream.h"
> +#include "qapi/error.h"
>
> #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */
> #define XIO3130_REVISION 0x2
> @@ -56,14 +57,17 @@ 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) {
> + assert(rc == -ENOTSUP);
> + error_report_err(err);
Hi Jin, Markus
It looks a little weird to me to check for negative error code
and then assert for a specific error as the only "valid error".
Maybe we should assert inside msi_init to leave the callers "clean"?
I am well aware a lot of time was spent for this series, but I just
spotted it and I want to be sure we are doing it right.
Thanks,
Marcel
> goto err_bridge;
> }
>
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index ed79225..a87b227 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -22,6 +22,7 @@
> #include "hw/pci/msi.h"
> #include "hw/xen/xen.h"
> #include "qemu/range.h"
> +#include "qapi/error.h"
>
> /* PCI_MSI_ADDRESS_LO */
> #define PCI_MSI_ADDRESS_LO_MASK (~0x3)
> @@ -173,7 +174,8 @@ bool msi_enabled(const PCIDevice *dev)
> * 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.
> - * Return 0 on success, return -errno on error.
> + * @errp is for returning errors.
> + * Return 0 on success; set @errp and return -errno on error.
> *
> * -ENOTSUP means lacking msi support for a msi-capable platform.
> * -EINVAL means capability overlap, happens when @offset is non-zero,
> @@ -181,7 +183,8 @@ bool msi_enabled(const PCIDevice *dev)
> * if a real HW is broken.
> */
> 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;
> @@ -189,6 +192,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
> int config_offset;
>
> if (!msi_nonbroken) {
> + error_setg(errp, "MSI is not supported by interrupt controller");
> return -ENOTSUP;
> }
>
> @@ -212,7 +216,8 @@ 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;
> }
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 2ede192..345783d 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -29,7 +29,7 @@
> #include "hw/scsi/scsi.h"
> #include "block/scsi.h"
> #include "trace.h"
> -
> +#include "qapi/error.h"
> #include "mfi.h"
>
> #define MEGASAS_VERSION_GEN1 "1.70"
> @@ -2333,6 +2333,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
> uint8_t *pci_conf;
> int i, bar_type;
> + Error *err = NULL;
> + int ret;
>
> pci_conf = dev->config;
>
> @@ -2341,6 +2343,24 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
> /* Interrupt pin 1 */
> pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>
> + if (megasas_use_msi(s)) {
> + ret = msi_init(dev, 0x50, 1, true, false, &err);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error */
> + assert(!ret || ret == -ENOTSUP);
> + if (ret && s->msi == ON_OFF_AUTO_ON) {
> + /* Can't satisfy user's explicit msi=on request, fail */
> + error_append_hint(&err, "You have to use msi=auto (default) or "
> + "msi=off with this machine type.\n");
> + error_propagate(errp, err);
> + return;
> + } else if (ret) {
> + /* With msi=auto, we fall back to MSI off silently */
> + s->msi = ON_OFF_AUTO_OFF;
> + error_free(err);
> + }
> + }
> +
> 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,
> @@ -2348,10 +2368,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) < 0) {
> - s->msi = ON_OFF_AUTO_OFF;
> - }
> 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 dfbc0c4..698be42 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -32,7 +32,7 @@
> #include "hw/scsi/scsi.h"
> #include "block/scsi.h"
> #include "trace.h"
> -
> +#include "qapi/error.h"
> #include "mptsas.h"
> #include "mpi.h"
>
> @@ -1273,10 +1273,33 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
> {
> DeviceState *d = DEVICE(dev);
> MPTSASState *s = MPT_SAS(dev);
> + Error *err = NULL;
> + int ret;
>
> dev->config[PCI_LATENCY_TIMER] = 0;
> dev->config[PCI_INTERRUPT_PIN] = 0x01;
>
> + if (s->msi != ON_OFF_AUTO_OFF) {
> + ret = msi_init(dev, 0, 1, true, false, &err);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error */
> + assert(!ret || ret == -ENOTSUP);
> + if (ret && s->msi == ON_OFF_AUTO_ON) {
> + /* Can't satisfy user's explicit msi=on request, fail */
> + error_append_hint(&err, "You have to use msi=auto (default) or "
> + "msi=off with this machine type.\n");
> + error_propagate(errp, err);
> + s->msi_in_use = false;
> + return;
> + } else if (ret) {
> + /* With msi=auto, we fall back to MSI off silently */
> + error_free(err);
> + s->msi_in_use = false;
> + } else {
> + s->msi_in_use = true;
> + }
> + }
> +
> 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,
> @@ -1284,12 +1307,6 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
> memory_region_init_io(&s->diag_io, OBJECT(s), &mptsas_diag_ops, s,
> "mptsas-diag", 0x10000);
>
> - if (s->msi != ON_OFF_AUTO_OFF &&
> - msi_init(dev, 0, 1, true, false) >= 0) {
> - /* TODO check for errors */
> - 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 e035fce..ecd6077 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1063,7 +1063,7 @@ pvscsi_init_msi(PVSCSIState *s)
> 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, NULL);
> 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 0a5510f..1a3377f 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -26,6 +26,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/msix.h"
> #include "trace.h"
> +#include "qapi/error.h"
>
> //#define DEBUG_XHCI
> //#define DEBUG_DATA
> @@ -3581,6 +3582,7 @@ static void usb_xhci_init(XHCIState *xhci)
> static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
> {
> int i, ret;
> + Error *err = NULL;
>
> XHCIState *xhci = XHCI(dev);
>
> @@ -3591,6 +3593,23 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
>
> usb_xhci_init(xhci);
>
> + if (xhci->msi != ON_OFF_AUTO_OFF) {
> + ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
> + /* Any error other than -ENOTSUP(board's MSI support is broken)
> + * is a programming error */
> + assert(!ret || ret == -ENOTSUP);
> + if (ret && xhci->msi == ON_OFF_AUTO_ON) {
> + /* Can't satisfy user's explicit msi=on request, fail */
> + error_append_hint(&err, "You have to use msi=auto (default) or "
> + "msi=off with this machine type.\n");
> + error_propagate(errp, err);
> + return;
> + }
> + assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
> + /* With msi=auto, we fall back to MSI off silently */
> + error_free(err);
> + }
> +
> if (xhci->numintrs > MAXINTRS) {
> xhci->numintrs = MAXINTRS;
> }
> @@ -3648,10 +3667,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
> assert(ret >= 0);
> }
>
> - if (xhci->msi != ON_OFF_AUTO_OFF) {
> - /* TODO check for errors */
> - msi_init(dev, 0x70, xhci->numintrs, true, false);
> - }
> if (xhci->msix != ON_OFF_AUTO_OFF) {
> /* TODO check for errors */
> msix_init(dev, xhci->numintrs,
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index deab0c6..dfbf8ba 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -32,6 +32,7 @@
> #include "sysemu/sysemu.h"
> #include "pci.h"
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define MSIX_CAP_LENGTH 12
>
> @@ -1171,6 +1172,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 +1186,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);
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it
2016-06-13 10:16 ` Marcel Apfelbaum
@ 2016-06-13 11:07 ` Markus Armbruster
2016-06-13 11:53 ` Marcel Apfelbaum
2016-06-13 11:09 ` Cao jin
1 sibling, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2016-06-13 11:07 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: Cao jin, qemu-devel, Michael S. Tsirkin, Jason Wang,
Alex Williamson, Hannes Reinecke, Dmitry Fleytman, Paolo Bonzini,
John Snow, Gerd Hoffmann
Marcel Apfelbaum <marcel@redhat.com> writes:
> On 06/10/2016 12:54 PM, Cao jin wrote:
>> msi_init() reports errors with error_report(), which is wrong
>> when it's used in realize().
>>
>> Fix by converting it to Error.
>>
>> Fix its callers to handle failure instead of ignoring it.
>>
>> For those callers who don't handle the failure, it might happen:
>> when user want msi on, but he doesn't get what he want because of
>> msi_init fails silently.
>>
>> cc: Gerd Hoffmann <kraxel@redhat.com>
>> cc: John Snow <jsnow@redhat.com>
>> cc: Dmitry Fleytman <dmitry@daynix.com>
>> cc: Jason Wang <jasowang@redhat.com>
>> cc: Michael S. Tsirkin <mst@redhat.com>
>> cc: Hannes Reinecke <hare@suse.de>
>> cc: Paolo Bonzini <pbonzini@redhat.com>
>> cc: Alex Williamson <alex.williamson@redhat.com>
>> cc: Markus Armbruster <armbru@redhat.com>
>> cc: Marcel Apfelbaum <marcel@redhat.com>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> hw/audio/intel-hda.c | 24 ++++++++++++++++++++----
>> hw/ide/ich.c | 15 +++++++++------
>> hw/net/e1000e.c | 8 ++------
>> hw/net/vmxnet3.c | 37 ++++++++++++-------------------------
>> hw/pci-bridge/ioh3420.c | 6 +++++-
>> hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++----
>> hw/pci-bridge/xio3130_downstream.c | 6 +++++-
>> hw/pci-bridge/xio3130_upstream.c | 6 +++++-
>> hw/pci/msi.c | 11 ++++++++---
>> hw/scsi/megasas.c | 26 +++++++++++++++++++++-----
>> hw/scsi/mptsas.c | 31 ++++++++++++++++++++++++-------
>> hw/scsi/vmw_pvscsi.c | 2 +-
>> hw/usb/hcd-xhci.c | 23 +++++++++++++++++++----
>> hw/vfio/pci.c | 7 +++++--
>> include/hw/pci/msi.h | 3 ++-
>> 15 files changed, 154 insertions(+), 71 deletions(-)
>>
>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
>> index 6b4dda0..82101f8 100644
>> --- a/hw/audio/intel-hda.c
>> +++ b/hw/audio/intel-hda.c
>> @@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>> {
>> IntelHDAState *d = INTEL_HDA(pci);
>> uint8_t *conf = d->pci.config;
>> + Error *err = NULL;
>> + int ret;
>>
>> d->name = object_get_typename(OBJECT(d));
>>
>> @@ -1143,13 +1145,27 @@ 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 != ON_OFF_AUTO_OFF) {
>> + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
>> + 1, true, false, &err);
>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>> + * is a programming error */
>> + assert(!ret || ret == -ENOTSUP);
>> + if (ret && d->msi == ON_OFF_AUTO_ON) {
>> + /* Can't satisfy user's explicit msi=on request, fail */
>> + error_append_hint(&err, "You have to use msi=auto (default) or "
>> + "msi=off with this machine type.\n");
>> + error_propagate(errp, err);
>> + return;
>> + }
>> + assert(!err || d->msi == ON_OFF_AUTO_AUTO);
>> + /* With msi=auto, we fall back to MSI off silently */
>> + error_free(err);
>> + }
>> +
>> 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 != ON_OFF_AUTO_OFF) {
>> - /* TODO check for errors */
>> - 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..084bef8 100644
>> --- a/hw/ide/ich.c
>> +++ b/hw/ide/ich.c
>> @@ -68,7 +68,6 @@
>> #include <hw/isa/isa.h>
>> #include "sysemu/block-backend.h"
>> #include "sysemu/dma.h"
>> -
>> #include <hw/ide/pci.h>
>> #include <hw/ide/ahci.h>
>>
>> @@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>> int sata_cap_offset;
>> uint8_t *sata_cap;
>> d = ICH_AHCI(dev);
>> + int ret;
>> +
>> + /* 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. */
>> + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>> + * is a programming error. Fall back to INTx silently on -ENOTSUP */
>> + assert(!ret || ret == -ENOTSUP);
>>
>> ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>>
>> @@ -142,11 +150,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>> pci_set_long(sata_cap + SATA_CAP_BAR,
>> (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4));
>> d->ahci.idp_offset = ICH9_IDP_INDEX;
>> -
>> - /* 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);
>> }
>>
>> static void pci_ich9_uninit(PCIDevice *dev)
>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>> index 692283f..a06d184 100644
>> --- a/hw/net/e1000e.c
>> +++ b/hw/net/e1000e.c
>> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
>> {
>> int res;
>>
>> - res = msi_init(PCI_DEVICE(s),
>> - 0xD0, /* MSI capability offset */
>> - 1, /* MAC MSI interrupts */
>> - true, /* 64-bit message addresses supported */
>> - false); /* Per vector mask supported */
>> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>>
>
> Why did you chose to remove author's comments?
>
>
>> - if (res > 0) {
>> + if (!res) {
>> s->intr_state |= E1000E_USE_MSI;
>> } else {
>> trace_e1000e_msi_init_fail(res);
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index d978976..63f8904 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -2197,27 +2197,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)
>> {
>> @@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s)
>> return dsn_payload;
>> }
>>
>> +
>> +#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);
>> VMXNET3State *s = VMXNET3(pci_dev);
>> + int ret;
>>
>> VMW_CBPRN("Starting init...");
>>
>> @@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>> /* Interrupt pin A */
>> pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
>>
>> + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
>> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, NULL);
>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>> + * is a programming error. Fall back to INTx silently on -ENOTSUP */
>> + assert(!ret || ret == -ENOTSUP);
>> + s->msi_used = !ret;
>> +
>> if (!vmxnet3_init_msix(s)) {
>> 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 b4a7806..93c6f0b 100644
>> --- a/hw/pci-bridge/ioh3420.c
>> +++ b/hw/pci-bridge/ioh3420.c
>> @@ -25,6 +25,7 @@
>> #include "hw/pci/msi.h"
>> #include "hw/pci/pcie.h"
>> #include "ioh3420.h"
>> +#include "qapi/error.h"
>>
>> #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */
>> #define PCI_DEVICE_ID_IOH_REV 0x2
>> @@ -97,6 +98,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);
>> @@ -109,8 +111,10 @@ static int ioh3420_initfn(PCIDevice *d)
>>
>> 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) {
>> + assert(rc == -ENOTSUP);
>> + error_report_err(err);
>> goto err_bridge;
>> }
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index 0fbecc4..c4d2c0b 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -54,6 +54,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);
>>
>> @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> goto slotid_error;
>> }
>>
>> - if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
>> - msi_nonbroken) {
>> - err = msi_init(dev, 0, 1, true, true);
>> - if (err < 0) {
>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>> + /* it means SHPC exists, because SHPC is required for MSI */
>
> Is the other way around, MSI is needed by SHPC (but not compulsory)
>
>> +
>> + err = msi_init(dev, 0, 1, true, true, &local_err);
>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>> + * is a programming error */
>> + assert(!err || err == -ENOTSUP);
>> + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) {
>> + /* Can't satisfy user's explicit msi=on request, fail */
>> + error_append_hint(&local_err, "You have to use msi=auto (default) "
>> + "or msi=off with this machine type.\n");
>> + error_report_err(local_err);
>> goto msi_error;
>> }
>> + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO);
>> + /* With msi=auto, we fall back to MSI off silently */
>> + error_free(local_err);
>> }
>>
>> if (shpc_present(dev)) {
>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>> index e6d653d..f6149a3 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -24,6 +24,7 @@
>> #include "hw/pci/msi.h"
>> #include "hw/pci/pcie.h"
>> #include "xio3130_downstream.h"
>> +#include "qapi/error.h"
>>
>> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */
>> #define XIO3130_REVISION 0x1
>> @@ -60,14 +61,17 @@ 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) {
>> + assert(rc == -ENOTSUP);
>> + error_report_err(err);
>> goto err_bridge;
>> }
>>
>> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
>> index d976844..487edac 100644
>> --- a/hw/pci-bridge/xio3130_upstream.c
>> +++ b/hw/pci-bridge/xio3130_upstream.c
>> @@ -24,6 +24,7 @@
>> #include "hw/pci/msi.h"
>> #include "hw/pci/pcie.h"
>> #include "xio3130_upstream.h"
>> +#include "qapi/error.h"
>>
>> #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */
>> #define XIO3130_REVISION 0x2
>> @@ -56,14 +57,17 @@ 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) {
>> + assert(rc == -ENOTSUP);
>> + error_report_err(err);
>
> Hi Jin, Markus
>
> It looks a little weird to me to check for negative error code
> and then assert for a specific error as the only "valid error".
> Maybe we should assert inside msi_init to leave the callers "clean"?
>
> I am well aware a lot of time was spent for this series, but I just
> spotted it and I want to be sure we are doing it right.
Only the callers know how to handle these errors. Let me explain using
the function comment:
* -ENOTSUP means lacking msi support for a msi-capable platform.
Our board emulation is broken. The device decides whether this is an
error (say because the user requested msi=on), or not. If it isn't,
devices generally fall back to a non-MSI variant.
* -EINVAL means capability overlap, happens when @offset is non-zero,
* also means a programming error, except device assignment, which can check
* if a real HW is broken.
For virtual devices, this is a programming error. Such callers should
therefore abort. But for device assignment, it's a physical device with
messed up capabilities --- not a programming error, aborting would be
inappropriate. See vfio_msi_setup() for an example.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it
2016-06-13 11:07 ` Markus Armbruster
@ 2016-06-13 11:53 ` Marcel Apfelbaum
0 siblings, 0 replies; 35+ messages in thread
From: Marcel Apfelbaum @ 2016-06-13 11:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: Cao jin, qemu-devel, Michael S. Tsirkin, Jason Wang,
Alex Williamson, Hannes Reinecke, Dmitry Fleytman, Paolo Bonzini,
John Snow, Gerd Hoffmann
On 06/13/2016 02:07 PM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> On 06/10/2016 12:54 PM, Cao jin wrote:
>>> msi_init() reports errors with error_report(), which is wrong
>>> when it's used in realize().
>>>
>>> Fix by converting it to Error.
>>>
>>> Fix its callers to handle failure instead of ignoring it.
>>>
>>> For those callers who don't handle the failure, it might happen:
>>> when user want msi on, but he doesn't get what he want because of
>>> msi_init fails silently.
>>>
>>> cc: Gerd Hoffmann <kraxel@redhat.com>
>>> cc: John Snow <jsnow@redhat.com>
>>> cc: Dmitry Fleytman <dmitry@daynix.com>
>>> cc: Jason Wang <jasowang@redhat.com>
>>> cc: Michael S. Tsirkin <mst@redhat.com>
>>> cc: Hannes Reinecke <hare@suse.de>
>>> cc: Paolo Bonzini <pbonzini@redhat.com>
>>> cc: Alex Williamson <alex.williamson@redhat.com>
>>> cc: Markus Armbruster <armbru@redhat.com>
>>> cc: Marcel Apfelbaum <marcel@redhat.com>
>>>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---
>>> hw/audio/intel-hda.c | 24 ++++++++++++++++++++----
>>> hw/ide/ich.c | 15 +++++++++------
>>> hw/net/e1000e.c | 8 ++------
>>> hw/net/vmxnet3.c | 37 ++++++++++++-------------------------
>>> hw/pci-bridge/ioh3420.c | 6 +++++-
>>> hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++----
>>> hw/pci-bridge/xio3130_downstream.c | 6 +++++-
>>> hw/pci-bridge/xio3130_upstream.c | 6 +++++-
>>> hw/pci/msi.c | 11 ++++++++---
>>> hw/scsi/megasas.c | 26 +++++++++++++++++++++-----
>>> hw/scsi/mptsas.c | 31 ++++++++++++++++++++++++-------
>>> hw/scsi/vmw_pvscsi.c | 2 +-
>>> hw/usb/hcd-xhci.c | 23 +++++++++++++++++++----
>>> hw/vfio/pci.c | 7 +++++--
>>> include/hw/pci/msi.h | 3 ++-
>>> 15 files changed, 154 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
>>> index 6b4dda0..82101f8 100644
>>> --- a/hw/audio/intel-hda.c
>>> +++ b/hw/audio/intel-hda.c
>>> @@ -1135,6 +1135,8 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
>>> {
>>> IntelHDAState *d = INTEL_HDA(pci);
>>> uint8_t *conf = d->pci.config;
>>> + Error *err = NULL;
>>> + int ret;
>>>
>>> d->name = object_get_typename(OBJECT(d));
>>>
>>> @@ -1143,13 +1145,27 @@ 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 != ON_OFF_AUTO_OFF) {
>>> + ret = msi_init(&d->pci, d->old_msi_addr ? 0x50 : 0x60,
>>> + 1, true, false, &err);
>>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>>> + * is a programming error */
>>> + assert(!ret || ret == -ENOTSUP);
>>> + if (ret && d->msi == ON_OFF_AUTO_ON) {
>>> + /* Can't satisfy user's explicit msi=on request, fail */
>>> + error_append_hint(&err, "You have to use msi=auto (default) or "
>>> + "msi=off with this machine type.\n");
>>> + error_propagate(errp, err);
>>> + return;
>>> + }
>>> + assert(!err || d->msi == ON_OFF_AUTO_AUTO);
>>> + /* With msi=auto, we fall back to MSI off silently */
>>> + error_free(err);
>>> + }
>>> +
>>> 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 != ON_OFF_AUTO_OFF) {
>>> - /* TODO check for errors */
>>> - 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..084bef8 100644
>>> --- a/hw/ide/ich.c
>>> +++ b/hw/ide/ich.c
>>> @@ -68,7 +68,6 @@
>>> #include <hw/isa/isa.h>
>>> #include "sysemu/block-backend.h"
>>> #include "sysemu/dma.h"
>>> -
>>> #include <hw/ide/pci.h>
>>> #include <hw/ide/ahci.h>
>>>
>>> @@ -111,6 +110,15 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>>> int sata_cap_offset;
>>> uint8_t *sata_cap;
>>> d = ICH_AHCI(dev);
>>> + int ret;
>>> +
>>> + /* 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. */
>>> + ret = msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, NULL);
>>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>>> + * is a programming error. Fall back to INTx silently on -ENOTSUP */
>>> + assert(!ret || ret == -ENOTSUP);
>>>
>>> ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
>>>
>>> @@ -142,11 +150,6 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
>>> pci_set_long(sata_cap + SATA_CAP_BAR,
>>> (ICH9_IDP_BAR + 0x4) | (ICH9_IDP_INDEX_LOG2 << 4));
>>> d->ahci.idp_offset = ICH9_IDP_INDEX;
>>> -
>>> - /* 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);
>>> }
>>>
>>> static void pci_ich9_uninit(PCIDevice *dev)
>>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>>> index 692283f..a06d184 100644
>>> --- a/hw/net/e1000e.c
>>> +++ b/hw/net/e1000e.c
>>> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
>>> {
>>> int res;
>>>
>>> - res = msi_init(PCI_DEVICE(s),
>>> - 0xD0, /* MSI capability offset */
>>> - 1, /* MAC MSI interrupts */
>>> - true, /* 64-bit message addresses supported */
>>> - false); /* Per vector mask supported */
>>> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>>>
>>
>> Why did you chose to remove author's comments?
>>
>>
>>> - if (res > 0) {
>>> + if (!res) {
>>> s->intr_state |= E1000E_USE_MSI;
>>> } else {
>>> trace_e1000e_msi_init_fail(res);
>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>> index d978976..63f8904 100644
>>> --- a/hw/net/vmxnet3.c
>>> +++ b/hw/net/vmxnet3.c
>>> @@ -2197,27 +2197,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)
>>> {
>>> @@ -2279,10 +2258,15 @@ static uint64_t vmxnet3_device_serial_num(VMXNET3State *s)
>>> return dsn_payload;
>>> }
>>>
>>> +
>>> +#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);
>>> VMXNET3State *s = VMXNET3(pci_dev);
>>> + int ret;
>>>
>>> VMW_CBPRN("Starting init...");
>>>
>>> @@ -2306,14 +2290,17 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
>>> /* Interrupt pin A */
>>> pci_dev->config[PCI_INTERRUPT_PIN] = 0x01;
>>>
>>> + ret = msi_init(pci_dev, VMXNET3_MSI_OFFSET(s), VMXNET3_MAX_NMSIX_INTRS,
>>> + VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK, NULL);
>>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>>> + * is a programming error. Fall back to INTx silently on -ENOTSUP */
>>> + assert(!ret || ret == -ENOTSUP);
>>> + s->msi_used = !ret;
>>> +
>>> if (!vmxnet3_init_msix(s)) {
>>> 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 b4a7806..93c6f0b 100644
>>> --- a/hw/pci-bridge/ioh3420.c
>>> +++ b/hw/pci-bridge/ioh3420.c
>>> @@ -25,6 +25,7 @@
>>> #include "hw/pci/msi.h"
>>> #include "hw/pci/pcie.h"
>>> #include "ioh3420.h"
>>> +#include "qapi/error.h"
>>>
>>> #define PCI_DEVICE_ID_IOH_EPORT 0x3420 /* D0:F0 express mode */
>>> #define PCI_DEVICE_ID_IOH_REV 0x2
>>> @@ -97,6 +98,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);
>>> @@ -109,8 +111,10 @@ static int ioh3420_initfn(PCIDevice *d)
>>>
>>> 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) {
>>> + assert(rc == -ENOTSUP);
>>> + error_report_err(err);
>>> goto err_bridge;
>>> }
>>>
>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>> index 0fbecc4..c4d2c0b 100644
>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>> @@ -54,6 +54,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);
>>>
>>> @@ -75,12 +76,23 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> goto slotid_error;
>>> }
>>>
>>> - if (bridge_dev->msi != ON_OFF_AUTO_OFF &&
>>> - msi_nonbroken) {
>>> - err = msi_init(dev, 0, 1, true, true);
>>> - if (err < 0) {
>>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>>> + /* it means SHPC exists, because SHPC is required for MSI */
>>
>> Is the other way around, MSI is needed by SHPC (but not compulsory)
>>
>>> +
>>> + err = msi_init(dev, 0, 1, true, true, &local_err);
>>> + /* Any error other than -ENOTSUP(board's MSI support is broken)
>>> + * is a programming error */
>>> + assert(!err || err == -ENOTSUP);
>>> + if (err && bridge_dev->msi == ON_OFF_AUTO_ON) {
>>> + /* Can't satisfy user's explicit msi=on request, fail */
>>> + error_append_hint(&local_err, "You have to use msi=auto (default) "
>>> + "or msi=off with this machine type.\n");
>>> + error_report_err(local_err);
>>> goto msi_error;
>>> }
>>> + assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO);
>>> + /* With msi=auto, we fall back to MSI off silently */
>>> + error_free(local_err);
>>> }
>>>
>>> if (shpc_present(dev)) {
>>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>>> index e6d653d..f6149a3 100644
>>> --- a/hw/pci-bridge/xio3130_downstream.c
>>> +++ b/hw/pci-bridge/xio3130_downstream.c
>>> @@ -24,6 +24,7 @@
>>> #include "hw/pci/msi.h"
>>> #include "hw/pci/pcie.h"
>>> #include "xio3130_downstream.h"
>>> +#include "qapi/error.h"
>>>
>>> #define PCI_DEVICE_ID_TI_XIO3130D 0x8233 /* downstream port */
>>> #define XIO3130_REVISION 0x1
>>> @@ -60,14 +61,17 @@ 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) {
>>> + assert(rc == -ENOTSUP);
>>> + error_report_err(err);
>>> goto err_bridge;
>>> }
>>>
>>> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
>>> index d976844..487edac 100644
>>> --- a/hw/pci-bridge/xio3130_upstream.c
>>> +++ b/hw/pci-bridge/xio3130_upstream.c
>>> @@ -24,6 +24,7 @@
>>> #include "hw/pci/msi.h"
>>> #include "hw/pci/pcie.h"
>>> #include "xio3130_upstream.h"
>>> +#include "qapi/error.h"
>>>
>>> #define PCI_DEVICE_ID_TI_XIO3130U 0x8232 /* upstream port */
>>> #define XIO3130_REVISION 0x2
>>> @@ -56,14 +57,17 @@ 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) {
>>> + assert(rc == -ENOTSUP);
>>> + error_report_err(err);
>>
>> Hi Jin, Markus
>>
>> It looks a little weird to me to check for negative error code
>> and then assert for a specific error as the only "valid error".
>> Maybe we should assert inside msi_init to leave the callers "clean"?
>>
>> I am well aware a lot of time was spent for this series, but I just
>> spotted it and I want to be sure we are doing it right.
>
> Only the callers know how to handle these errors. Let me explain using
> the function comment:
>
> * -ENOTSUP means lacking msi support for a msi-capable platform.
>
> Our board emulation is broken. The device decides whether this is an
> error (say because the user requested msi=on), or not. If it isn't,
> devices generally fall back to a non-MSI variant.
>
> * -EINVAL means capability overlap, happens when @offset is non-zero,
> * also means a programming error, except device assignment, which can check
> * if a real HW is broken.
>
> For virtual devices, this is a programming error. Such callers should
> therefore abort. But for device assignment, it's a physical device with
> messed up capabilities --- not a programming error, aborting would be
> inappropriate. See vfio_msi_setup() for an example.
>
I missed the vfio scenario.
Now I got it.
Thanks!
Marcel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it
2016-06-13 10:16 ` Marcel Apfelbaum
2016-06-13 11:07 ` Markus Armbruster
@ 2016-06-13 11:09 ` Cao jin
2016-06-13 11:55 ` Marcel Apfelbaum
1 sibling, 1 reply; 35+ messages in thread
From: Cao jin @ 2016-06-13 11:09 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster
On 06/13/2016 06:16 PM, Marcel Apfelbaum wrote:
> On 06/10/2016 12:54 PM, Cao jin wrote:
>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>> index 692283f..a06d184 100644
>> --- a/hw/net/e1000e.c
>> +++ b/hw/net/e1000e.c
>> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
>> {
>> int res;
>>
>> - res = msi_init(PCI_DEVICE(s),
>> - 0xD0, /* MSI capability offset */
>> - 1, /* MAC MSI interrupts */
>> - true, /* 64-bit message addresses supported */
>> - false); /* Per vector mask supported */
>> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>>
>
> Why did you chose to remove author's comments?
>
Because msi_init() has its function comments now, which is the same is
the author`s comments, I guess author add these comments because
function has no comment before, remove comments also is to save screen
space:)
some macros of some devices is also unnecessary I think, because we have
comments of msi_init().
>> +
>> +#define VMXNET3_USE_64BIT (true)
>> +#define VMXNET3_PER_VECTOR_MASK (false)
>> +
like these macros, but it does't take too much space as above one I
feel, so I didn't touch them.
>> @@ -56,14 +57,17 @@ 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) {
>> + assert(rc == -ENOTSUP);
>> + error_report_err(err);
>
> Hi Jin, Markus
>
> It looks a little weird to me to check for negative error code
> and then assert for a specific error as the only "valid error".
> Maybe we should assert inside msi_init to leave the callers "clean"?
>
Hi Marcel,
I think it is because: except assigned device(vfio), the emulated pci
devices won`t have config space overlapped(-EINVAL), unless programming
error.
If implemented as you said, I guess there need a judgement (if..else..)
to check if current device is assigned in msi_init(), or else assert the
error
> I am well aware a lot of time was spent for this series, but I just
> spotted it and I want to be sure we are doing it right.
>
> Thanks,
> Marcel
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it
2016-06-13 11:09 ` Cao jin
@ 2016-06-13 11:55 ` Marcel Apfelbaum
0 siblings, 0 replies; 35+ messages in thread
From: Marcel Apfelbaum @ 2016-06-13 11:55 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Michael S. Tsirkin, Hannes Reinecke, Paolo Bonzini,
Alex Williamson, Markus Armbruster
On 06/13/2016 02:09 PM, Cao jin wrote:
>
>
> On 06/13/2016 06:16 PM, Marcel Apfelbaum wrote:
>> On 06/10/2016 12:54 PM, Cao jin wrote:
>
>>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>>> index 692283f..a06d184 100644
>>> --- a/hw/net/e1000e.c
>>> +++ b/hw/net/e1000e.c
>>> @@ -268,13 +268,9 @@ e1000e_init_msi(E1000EState *s)
>>> {
>>> int res;
>>>
>>> - res = msi_init(PCI_DEVICE(s),
>>> - 0xD0, /* MSI capability offset */
>>> - 1, /* MAC MSI interrupts */
>>> - true, /* 64-bit message addresses supported */
>>> - false); /* Per vector mask supported */
>>> + res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
>>>
>>
>> Why did you chose to remove author's comments?
>>
>
> Because msi_init() has its function comments now, which is the same is the author`s comments, I guess author add these comments because function has no comment before, remove comments also is to save
> screen space:)
>
> some macros of some devices is also unnecessary I think, because we have comments of msi_init().
>
Right.
>>> +
>>> +#define VMXNET3_USE_64BIT (true)
>>> +#define VMXNET3_PER_VECTOR_MASK (false)
>>> +
>
> like these macros, but it does't take too much space as above one I feel, so I didn't touch them.
>
>>> @@ -56,14 +57,17 @@ 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) {
>>> + assert(rc == -ENOTSUP);
>>> + error_report_err(err);
>>
>> Hi Jin, Markus
>>
>> It looks a little weird to me to check for negative error code
>> and then assert for a specific error as the only "valid error".
>> Maybe we should assert inside msi_init to leave the callers "clean"?
>>
> Hi Marcel,
>
> I think it is because: except assigned device(vfio), the emulated pci devices won`t have config space overlapped(-EINVAL), unless programming error.
>
Understood, thanks for the explanation.
For the PCI part:
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
> If implemented as you said, I guess there need a judgement (if..else..) to check if current device is assigned in msi_init(), or else assert the error
>
>> I am well aware a lot of time was spent for this series, but I just
>> spotted it and I want to be sure we are doing it right.
>>
>> Thanks,
>> Marcel
>>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v8 13/17] megasas: remove unnecessary megasas_use_msi()
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (11 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 12/17] pci: Convert msi_init() to Error and fix callers to check it Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 14/17] mptsas: remove unnecessary internal msi state flag Cao jin
` (5 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Hannes Reinecke, Paolo Bonzini, Markus Armbruster,
Marcel Apfelbaum, Michael S. Tsirkin
megasas overwrites user configuration when msi_init fail to flag internal msi
state, which is unsuitable. megasa_use_msi() is unnecessary, we can call
msi_uninit() directly when unrealize, even no need to call msi_enabled() first.
cc: Hannes Reinecke <hare@suse.de>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/megasas.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 345783d..6a3f860 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -155,11 +155,6 @@ static bool megasas_use_queue64(MegasasState *s)
return s->flags & MEGASAS_MASK_USE_QUEUE64;
}
-static bool megasas_use_msi(MegasasState *s)
-{
- return s->msi != ON_OFF_AUTO_OFF;
-}
-
static bool megasas_use_msix(MegasasState *s)
{
return s->msix != ON_OFF_AUTO_OFF;
@@ -2310,9 +2305,7 @@ static void megasas_scsi_uninit(PCIDevice *d)
if (megasas_use_msix(s)) {
msix_uninit(d, &s->mmio_io, &s->mmio_io);
}
- if (megasas_use_msi(s)) {
- msi_uninit(d);
- }
+ msi_uninit(d);
}
static const struct SCSIBusInfo megasas_scsi_info = {
@@ -2343,7 +2336,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
/* Interrupt pin 1 */
pci_conf[PCI_INTERRUPT_PIN] = 0x01;
- if (megasas_use_msi(s)) {
+ if (s->msi != ON_OFF_AUTO_OFF) {
ret = msi_init(dev, 0x50, 1, true, false, &err);
/* Any error other than -ENOTSUP(board's MSI support is broken)
* is a programming error */
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 14/17] mptsas: remove unnecessary internal msi state flag
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (12 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 13/17] megasas: remove unnecessary megasas_use_msi() Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 15/17] vmw_pvscsi: " Cao jin
` (4 subsequent siblings)
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Marcel Apfelbaum, Paolo Bonzini,
Michael S. Tsirkin
Internal flag msi_in_use in unnecessary, msi_uninit() could be called
directly, and msi_enabled() is enough to check device msi state.
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/mptsas.c | 18 ++++++------------
hw/scsi/mptsas.h | 2 --
2 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 698be42..c1a0649 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -63,7 +63,7 @@ static void mptsas_update_interrupt(MPTSASState *s)
PCIDevice *pci = (PCIDevice *) s;
uint32_t state = s->intr_status & ~(s->intr_mask | MPI_HIS_IOP_DOORBELL_STATUS);
- if (s->msi_in_use && msi_enabled(pci)) {
+ if (msi_enabled(pci)) {
if (state) {
trace_mptsas_irq_msi(s);
msi_notify(pci, 0);
@@ -1289,15 +1289,12 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
error_append_hint(&err, "You have to use msi=auto (default) or "
"msi=off with this machine type.\n");
error_propagate(errp, err);
- s->msi_in_use = false;
return;
- } else if (ret) {
- /* With msi=auto, we fall back to MSI off silently */
- error_free(err);
- s->msi_in_use = false;
- } else {
- s->msi_in_use = true;
}
+ assert(!err || s->msi == ON_OFF_AUTO_AUTO);
+ /* With msi=auto, we fall back to MSI off silently */
+ error_free(err);
+
}
memory_region_init_io(&s->mmio_io, OBJECT(s), &mptsas_mmio_ops, s,
@@ -1337,9 +1334,7 @@ static void mptsas_scsi_uninit(PCIDevice *dev)
MPTSASState *s = MPT_SAS(dev);
qemu_bh_delete(s->request_bh);
- if (s->msi_in_use) {
- msi_uninit(dev);
- }
+ msi_uninit(dev);
}
static void mptsas_reset(DeviceState *dev)
@@ -1375,7 +1370,6 @@ static const VMStateDescription vmstate_mptsas = {
.post_load = mptsas_post_load,
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(dev, MPTSASState),
- VMSTATE_BOOL(msi_in_use, MPTSASState),
VMSTATE_UINT32(state, MPTSASState),
VMSTATE_UINT8(who_init, MPTSASState),
diff --git a/hw/scsi/mptsas.h b/hw/scsi/mptsas.h
index 0436a33..da014a3 100644
--- a/hw/scsi/mptsas.h
+++ b/hw/scsi/mptsas.h
@@ -31,8 +31,6 @@ struct MPTSASState {
OnOffAuto msi;
uint64_t sas_addr;
- bool msi_in_use;
-
/* Doorbell register */
uint32_t state;
uint8_t who_init;
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 15/17] vmw_pvscsi: remove unnecessary internal msi state flag
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (13 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 14/17] mptsas: remove unnecessary internal msi state flag Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-13 19:42 ` Michael S. Tsirkin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 16/17] vmxnet3: " Cao jin
` (3 subsequent siblings)
18 siblings, 1 reply; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Dmitry Fleytman, Markus Armbruster,
Marcel Apfelbaum, Michael S. Tsirkin
Internal flag msi_used is uncesessary, msi_uninit() could be called
directly, msi_enabled() is enough to check device msi state.
cc: Paolo Bonzini <pbonzini@redhat.com>
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/scsi/vmw_pvscsi.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index ecd6077..363a7ed 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -121,8 +121,6 @@ typedef struct {
uint8_t msg_ring_info_valid; /* Whether message ring initialized */
uint8_t use_msg; /* Whether to use message ring */
- uint8_t msi_used; /* Whether MSI support was installed successfully */
-
PVSCSIRingInfo rings; /* Data transfer rings manager */
uint32_t resetting; /* Reset in progress */
@@ -362,7 +360,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled,
s->reg_interrupt_status);
- if (s->msi_used && msi_enabled(d)) {
+ if (msi_enabled(d)) {
if (should_raise) {
trace_pvscsi_update_irq_msi();
msi_notify(d, PVSCSI_VECTOR_COMPLETION);
@@ -1066,9 +1064,6 @@ pvscsi_init_msi(PVSCSIState *s)
PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, NULL);
if (res < 0) {
trace_pvscsi_init_msi_fail(res);
- s->msi_used = false;
- } else {
- s->msi_used = true;
}
}
@@ -1077,9 +1072,7 @@ pvscsi_cleanup_msi(PVSCSIState *s)
{
PCIDevice *d = PCI_DEVICE(s);
- if (s->msi_used) {
- msi_uninit(d);
- }
+ msi_uninit(d);
}
static const MemoryRegionOps pvscsi_ops = {
@@ -1222,7 +1215,6 @@ static const VMStateDescription vmstate_pvscsi = {
VMSTATE_STRUCT_TEST(parent_obj, PVSCSIState,
pvscsi_vmstate_test_pci_device, 0,
vmstate_pci_device, PCIDevice),
- VMSTATE_UINT8(msi_used, PVSCSIState),
VMSTATE_UINT32(resetting, PVSCSIState),
VMSTATE_UINT64(reg_interrupt_status, PVSCSIState),
VMSTATE_UINT64(reg_interrupt_enabled, PVSCSIState),
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/17] vmw_pvscsi: remove unnecessary internal msi state flag
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 15/17] vmw_pvscsi: " Cao jin
@ 2016-06-13 19:42 ` Michael S. Tsirkin
2016-06-14 8:12 ` Markus Armbruster
0 siblings, 1 reply; 35+ messages in thread
From: Michael S. Tsirkin @ 2016-06-13 19:42 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Paolo Bonzini, Dmitry Fleytman, Markus Armbruster,
Marcel Apfelbaum
On Fri, Jun 10, 2016 at 05:54:36PM +0800, Cao jin wrote:
> Internal flag msi_used is uncesessary, msi_uninit() could be called
> directly, msi_enabled() is enough to check device msi state.
>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/scsi/vmw_pvscsi.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index ecd6077..363a7ed 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -121,8 +121,6 @@ typedef struct {
> uint8_t msg_ring_info_valid; /* Whether message ring initialized */
> uint8_t use_msg; /* Whether to use message ring */
>
> - uint8_t msi_used; /* Whether MSI support was installed successfully */
> -
> PVSCSIRingInfo rings; /* Data transfer rings manager */
> uint32_t resetting; /* Reset in progress */
>
> @@ -362,7 +360,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
> trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled,
> s->reg_interrupt_status);
>
> - if (s->msi_used && msi_enabled(d)) {
> + if (msi_enabled(d)) {
> if (should_raise) {
> trace_pvscsi_update_irq_msi();
> msi_notify(d, PVSCSI_VECTOR_COMPLETION);
> @@ -1066,9 +1064,6 @@ pvscsi_init_msi(PVSCSIState *s)
> PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, NULL);
> if (res < 0) {
> trace_pvscsi_init_msi_fail(res);
> - s->msi_used = false;
> - } else {
> - s->msi_used = true;
> }
> }
>
> @@ -1077,9 +1072,7 @@ pvscsi_cleanup_msi(PVSCSIState *s)
> {
> PCIDevice *d = PCI_DEVICE(s);
>
> - if (s->msi_used) {
> - msi_uninit(d);
> - }
> + msi_uninit(d);
> }
>
> static const MemoryRegionOps pvscsi_ops = {
> @@ -1222,7 +1215,6 @@ static const VMStateDescription vmstate_pvscsi = {
> VMSTATE_STRUCT_TEST(parent_obj, PVSCSIState,
> pvscsi_vmstate_test_pci_device, 0,
> vmstate_pci_device, PCIDevice),
> - VMSTATE_UINT8(msi_used, PVSCSIState),
> VMSTATE_UINT32(resetting, PVSCSIState),
> VMSTATE_UINT64(reg_interrupt_status, PVSCSIState),
> VMSTATE_UINT64(reg_interrupt_enabled, PVSCSIState),
This change will break cross-version migration, we can't make it.
> --
> 2.1.0
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/17] vmw_pvscsi: remove unnecessary internal msi state flag
2016-06-13 19:42 ` Michael S. Tsirkin
@ 2016-06-14 8:12 ` Markus Armbruster
2016-06-14 9:31 ` Cao jin
0 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2016-06-14 8:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Cao jin, Dmitry Fleytman, Paolo Bonzini, qemu-devel,
Marcel Apfelbaum
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Jun 10, 2016 at 05:54:36PM +0800, Cao jin wrote:
>> Internal flag msi_used is uncesessary, msi_uninit() could be called
>> directly, msi_enabled() is enough to check device msi state.
>>
>> cc: Paolo Bonzini <pbonzini@redhat.com>
>> cc: Dmitry Fleytman <dmitry@daynix.com>
>> cc: Markus Armbruster <armbru@redhat.com>
>> cc: Marcel Apfelbaum <marcel@redhat.com>
>> cc: Michael S. Tsirkin <mst@redhat.com>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> hw/scsi/vmw_pvscsi.c | 12 ++----------
>> 1 file changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
>> index ecd6077..363a7ed 100644
>> --- a/hw/scsi/vmw_pvscsi.c
>> +++ b/hw/scsi/vmw_pvscsi.c
>> @@ -121,8 +121,6 @@ typedef struct {
>> uint8_t msg_ring_info_valid; /* Whether message ring initialized */
>> uint8_t use_msg; /* Whether to use message ring */
>>
>> - uint8_t msi_used; /* Whether MSI support was installed successfully */
>> -
>> PVSCSIRingInfo rings; /* Data transfer rings manager */
>> uint32_t resetting; /* Reset in progress */
>>
>> @@ -362,7 +360,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
>> trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled,
>> s->reg_interrupt_status);
>>
>> - if (s->msi_used && msi_enabled(d)) {
>> + if (msi_enabled(d)) {
>> if (should_raise) {
>> trace_pvscsi_update_irq_msi();
>> msi_notify(d, PVSCSI_VECTOR_COMPLETION);
>> @@ -1066,9 +1064,6 @@ pvscsi_init_msi(PVSCSIState *s)
>> PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK, NULL);
>> if (res < 0) {
>> trace_pvscsi_init_msi_fail(res);
>> - s->msi_used = false;
>> - } else {
>> - s->msi_used = true;
>> }
>> }
>>
>> @@ -1077,9 +1072,7 @@ pvscsi_cleanup_msi(PVSCSIState *s)
>> {
>> PCIDevice *d = PCI_DEVICE(s);
>>
>> - if (s->msi_used) {
>> - msi_uninit(d);
>> - }
>> + msi_uninit(d);
>> }
>>
>> static const MemoryRegionOps pvscsi_ops = {
>> @@ -1222,7 +1215,6 @@ static const VMStateDescription vmstate_pvscsi = {
>> VMSTATE_STRUCT_TEST(parent_obj, PVSCSIState,
>> pvscsi_vmstate_test_pci_device, 0,
>> vmstate_pci_device, PCIDevice),
>> - VMSTATE_UINT8(msi_used, PVSCSIState),
>> VMSTATE_UINT32(resetting, PVSCSIState),
>> VMSTATE_UINT64(reg_interrupt_status, PVSCSIState),
>> VMSTATE_UINT64(reg_interrupt_enabled, PVSCSIState),
>
>
> This change will break cross-version migration, we can't make it.
Oww, sorry I missed this!
The value shouldn't have been put into the migration stream, but now
it's there, we need to keep it there. Suggest:
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index ecd6077..edd91ec 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -121,7 +121,7 @@ typedef struct {
uint8_t msg_ring_info_valid; /* Whether message ring initialized */
uint8_t use_msg; /* Whether to use message ring */
- uint8_t msi_used; /* Whether MSI support was installed successfully */
+ uint8_t msi_used; /* For migration compatibility */
PVSCSIRingInfo rings; /* Data transfer rings manager */
uint32_t resetting; /* Reset in progress */
@@ -362,7 +362,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled,
s->reg_interrupt_status);
- if (s->msi_used && msi_enabled(d)) {
+ if (msi_enabled(d)) {
if (should_raise) {
trace_pvscsi_update_irq_msi();
msi_notify(d, PVSCSI_VECTOR_COMPLETION);
@@ -1077,9 +1077,7 @@ pvscsi_cleanup_msi(PVSCSIState *s)
{
PCIDevice *d = PCI_DEVICE(s);
- if (s->msi_used) {
- msi_uninit(d);
- }
+ msi_uninit(d);
}
static const MemoryRegionOps pvscsi_ops = {
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/17] vmw_pvscsi: remove unnecessary internal msi state flag
2016-06-14 8:12 ` Markus Armbruster
@ 2016-06-14 9:31 ` Cao jin
0 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-14 9:31 UTC (permalink / raw)
To: Markus Armbruster, Michael S. Tsirkin
Cc: Dmitry Fleytman, Paolo Bonzini, qemu-devel, Marcel Apfelbaum
On 06/14/2016 04:12 PM, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>>> @@ -1222,7 +1215,6 @@ static const VMStateDescription vmstate_pvscsi = {
>>> VMSTATE_STRUCT_TEST(parent_obj, PVSCSIState,
>>> pvscsi_vmstate_test_pci_device, 0,
>>> vmstate_pci_device, PCIDevice),
>>> - VMSTATE_UINT8(msi_used, PVSCSIState),
>>> VMSTATE_UINT32(resetting, PVSCSIState),
>>> VMSTATE_UINT64(reg_interrupt_status, PVSCSIState),
>>> VMSTATE_UINT64(reg_interrupt_enabled, PVSCSIState),
>>
>>
>> This change will break cross-version migration, we can't make it.
>
> Oww, sorry I missed this!
>
> The value shouldn't have been put into the migration stream, but now
> it's there, we need to keep it there. Suggest:
>
Thanks, will collect these in next version.
>
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index ecd6077..edd91ec 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -121,7 +121,7 @@ typedef struct {
> uint8_t msg_ring_info_valid; /* Whether message ring initialized */
> uint8_t use_msg; /* Whether to use message ring */
>
> - uint8_t msi_used; /* Whether MSI support was installed successfully */
> + uint8_t msi_used; /* For migration compatibility */
>
> PVSCSIRingInfo rings; /* Data transfer rings manager */
> uint32_t resetting; /* Reset in progress */
> @@ -362,7 +362,7 @@ pvscsi_update_irq_status(PVSCSIState *s)
> trace_pvscsi_update_irq_level(should_raise, s->reg_interrupt_enabled,
> s->reg_interrupt_status);
>
> - if (s->msi_used && msi_enabled(d)) {
> + if (msi_enabled(d)) {
> if (should_raise) {
> trace_pvscsi_update_irq_msi();
> msi_notify(d, PVSCSI_VECTOR_COMPLETION);
> @@ -1077,9 +1077,7 @@ pvscsi_cleanup_msi(PVSCSIState *s)
> {
> PCIDevice *d = PCI_DEVICE(s);
>
> - if (s->msi_used) {
> - msi_uninit(d);
> - }
> + msi_uninit(d);
> }
>
> static const MemoryRegionOps pvscsi_ops = {
>
>
>
> .
>
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v8 16/17] vmxnet3: remove unnecessary internal msi state flag
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (14 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 15/17] vmw_pvscsi: " Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-13 8:47 ` Markus Armbruster
2016-06-13 9:31 ` Cao jin
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 17/17] e1000e: " Cao jin
` (2 subsequent siblings)
18 siblings, 2 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Dmitry Fleytman, Jason Wang, Markus Armbruster, Marcel Apfelbaum,
Michael S. Tsirkin
Internal flag msi_used is unnecessary, it has the same effect as msi_enabled().
msi_uninit() could be called directly without risk.
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/net/vmxnet3.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 63f8904..3ed4335 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -280,8 +280,6 @@ typedef struct {
/* Whether MSI-X support was installed successfully */
bool msix_used;
- /* Whether MSI support was installed successfully */
- bool msi_used;
hwaddr drv_shmem;
hwaddr temp_shared_guest_driver_memory;
@@ -363,7 +361,7 @@ static bool _vmxnet3_assert_interrupt_line(VMXNET3State *s, uint32_t int_idx)
msix_notify(d, int_idx);
return false;
}
- if (s->msi_used && msi_enabled(d)) {
+ if (msi_enabled(d)) {
VMW_IRPRN("Sending MSI notification for vector %u", int_idx);
msi_notify(d, int_idx);
return false;
@@ -387,7 +385,7 @@ static void _vmxnet3_deassert_interrupt_line(VMXNET3State *s, int lidx)
* This function should never be called for MSI(X) interrupts
* because deassertion never required for message interrupts
*/
- assert(!s->msi_used || !msi_enabled(d));
+ assert(!msi_enabled(d));
VMW_IRPRN("Deasserting line for interrupt %u", lidx);
pci_irq_deassert(d);
@@ -424,7 +422,7 @@ static void vmxnet3_trigger_interrupt(VMXNET3State *s, int lidx)
goto do_automask;
}
- if (s->msi_used && msi_enabled(d) && s->auto_int_masking) {
+ if (msi_enabled(d) && s->auto_int_masking) {
goto do_automask;
}
@@ -1409,7 +1407,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
{
- return s->msix_used || s->msi_used || (intx ==
+ return s->msix_used || msi_enabled(PCI_DEVICE(s)) || (intx ==
(pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
}
@@ -2202,9 +2200,7 @@ vmxnet3_cleanup_msi(VMXNET3State *s)
{
PCIDevice *d = PCI_DEVICE(s);
- if (s->msi_used) {
- msi_uninit(d);
- }
+ msi_uninit(d);
}
static void
@@ -2295,7 +2291,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
/* Any error other than -ENOTSUP(board's MSI support is broken)
* is a programming error. Fall back to INTx silently on -ENOTSUP */
assert(!ret || ret == -ENOTSUP);
- s->msi_used = !ret;
if (!vmxnet3_init_msix(s)) {
VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 16/17] vmxnet3: remove unnecessary internal msi state flag
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 16/17] vmxnet3: " Cao jin
@ 2016-06-13 8:47 ` Markus Armbruster
2016-06-13 9:31 ` Cao jin
1 sibling, 0 replies; 35+ messages in thread
From: Markus Armbruster @ 2016-06-13 8:47 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Dmitry Fleytman, Marcel Apfelbaum, Jason Wang,
Michael S. Tsirkin
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> Internal flag msi_used is unnecessary, it has the same effect as msi_enabled().
> msi_uninit() could be called directly without risk.
>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> hw/net/vmxnet3.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 63f8904..3ed4335 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -280,8 +280,6 @@ typedef struct {
>
> /* Whether MSI-X support was installed successfully */
> bool msix_used;
> - /* Whether MSI support was installed successfully */
> - bool msi_used;
> hwaddr drv_shmem;
> hwaddr temp_shared_guest_driver_memory;
>
> @@ -363,7 +361,7 @@ static bool _vmxnet3_assert_interrupt_line(VMXNET3State *s, uint32_t int_idx)
> msix_notify(d, int_idx);
> return false;
> }
> - if (s->msi_used && msi_enabled(d)) {
> + if (msi_enabled(d)) {
> VMW_IRPRN("Sending MSI notification for vector %u", int_idx);
> msi_notify(d, int_idx);
> return false;
> @@ -387,7 +385,7 @@ static void _vmxnet3_deassert_interrupt_line(VMXNET3State *s, int lidx)
> * This function should never be called for MSI(X) interrupts
> * because deassertion never required for message interrupts
> */
> - assert(!s->msi_used || !msi_enabled(d));
> + assert(!msi_enabled(d));
>
> VMW_IRPRN("Deasserting line for interrupt %u", lidx);
> pci_irq_deassert(d);
> @@ -424,7 +422,7 @@ static void vmxnet3_trigger_interrupt(VMXNET3State *s, int lidx)
> goto do_automask;
> }
>
> - if (s->msi_used && msi_enabled(d) && s->auto_int_masking) {
> + if (msi_enabled(d) && s->auto_int_masking) {
> goto do_automask;
> }
>
> @@ -1409,7 +1407,7 @@ static void vmxnet3_update_features(VMXNET3State *s)
>
> static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
> {
> - return s->msix_used || s->msi_used || (intx ==
> + return s->msix_used || msi_enabled(PCI_DEVICE(s)) || (intx ==
> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
If you need to respin for some other reason, you could clean up the
distasteful line break here, and drop the superfluous parenthesis:
return s->msix_used || msi_enabled(PCI_DEVICE(s))
|| intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1;
> }
>
> @@ -2202,9 +2200,7 @@ vmxnet3_cleanup_msi(VMXNET3State *s)
> {
> PCIDevice *d = PCI_DEVICE(s);
>
> - if (s->msi_used) {
> - msi_uninit(d);
> - }
> + msi_uninit(d);
> }
>
> static void
> @@ -2295,7 +2291,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
> /* Any error other than -ENOTSUP(board's MSI support is broken)
> * is a programming error. Fall back to INTx silently on -ENOTSUP */
> assert(!ret || ret == -ENOTSUP);
> - s->msi_used = !ret;
>
> if (!vmxnet3_init_msix(s)) {
> VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
^ permalink raw reply [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH v8 16/17] vmxnet3: remove unnecessary internal msi state flag
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 16/17] vmxnet3: " Cao jin
2016-06-13 8:47 ` Markus Armbruster
@ 2016-06-13 9:31 ` Cao jin
1 sibling, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-13 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: Dmitry Fleytman, Jason Wang, Markus Armbruster, Marcel Apfelbaum,
Michael S. Tsirkin
Internal flag msi_used is unnecessary, it has the same effect as msi_enabled().
msi_uninit() could be called directly without risk.
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
changelog: fix stylistic issue.
hw/net/vmxnet3.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 63f8904..9d439db 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -280,8 +280,6 @@ typedef struct {
/* Whether MSI-X support was installed successfully */
bool msix_used;
- /* Whether MSI support was installed successfully */
- bool msi_used;
hwaddr drv_shmem;
hwaddr temp_shared_guest_driver_memory;
@@ -363,7 +361,7 @@ static bool _vmxnet3_assert_interrupt_line(VMXNET3State *s, uint32_t int_idx)
msix_notify(d, int_idx);
return false;
}
- if (s->msi_used && msi_enabled(d)) {
+ if (msi_enabled(d)) {
VMW_IRPRN("Sending MSI notification for vector %u", int_idx);
msi_notify(d, int_idx);
return false;
@@ -387,7 +385,7 @@ static void _vmxnet3_deassert_interrupt_line(VMXNET3State *s, int lidx)
* This function should never be called for MSI(X) interrupts
* because deassertion never required for message interrupts
*/
- assert(!s->msi_used || !msi_enabled(d));
+ assert(!msi_enabled(d));
VMW_IRPRN("Deasserting line for interrupt %u", lidx);
pci_irq_deassert(d);
@@ -424,7 +422,7 @@ static void vmxnet3_trigger_interrupt(VMXNET3State *s, int lidx)
goto do_automask;
}
- if (s->msi_used && msi_enabled(d) && s->auto_int_masking) {
+ if (msi_enabled(d) && s->auto_int_masking) {
goto do_automask;
}
@@ -1409,8 +1407,8 @@ static void vmxnet3_update_features(VMXNET3State *s)
static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
{
- return s->msix_used || s->msi_used || (intx ==
- (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1));
+ return s->msix_used || msi_enabled(PCI_DEVICE(s))
+ || intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1;
}
static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
@@ -2202,9 +2200,7 @@ vmxnet3_cleanup_msi(VMXNET3State *s)
{
PCIDevice *d = PCI_DEVICE(s);
- if (s->msi_used) {
- msi_uninit(d);
- }
+ msi_uninit(d);
}
static void
@@ -2295,7 +2291,6 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp)
/* Any error other than -ENOTSUP(board's MSI support is broken)
* is a programming error. Fall back to INTx silently on -ENOTSUP */
assert(!ret || ret == -ENOTSUP);
- s->msi_used = !ret;
if (!vmxnet3_init_msix(s)) {
VMW_WRPRN("Failed to initialize MSI-X, configuration is inconsistent.");
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v8 17/17] e1000e: remove unnecessary internal msi state flag
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (15 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 16/17] vmxnet3: " Cao jin
@ 2016-06-10 9:54 ` Cao jin
2016-06-13 8:48 ` [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Markus Armbruster
2016-06-13 19:45 ` Michael S. Tsirkin
18 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-10 9:54 UTC (permalink / raw)
To: qemu-devel
Cc: Dmitry Fleytman, Jason Wang, Markus Armbruster, Marcel Apfelbaum,
Michael S. Tsirkin
Internal big flag E1000E_USE_MSI is unnecessary, also is the helper
function: e1000e_init_msi(), e1000e_cleanup_msi(), so, remove them all.
cc: Dmitry Fleytman <dmitry@daynix.com>
cc: Jason Wang <jasowang@redhat.com>
cc: Markus Armbruster <armbru@redhat.com>
cc: Marcel Apfelbaum <marcel@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
hw/net/e1000e.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index a06d184..c7d33ee 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -89,8 +89,7 @@ typedef struct E1000EState {
#define E1000E_MSIX_TABLE (0x0000)
#define E1000E_MSIX_PBA (0x2000)
-#define E1000E_USE_MSI BIT(0)
-#define E1000E_USE_MSIX BIT(1)
+#define E1000E_USE_MSIX BIT(0)
static uint64_t
e1000e_mmio_read(void *opaque, hwaddr addr, unsigned size)
@@ -264,28 +263,6 @@ static void e1000e_core_realize(E1000EState *s)
}
static void
-e1000e_init_msi(E1000EState *s)
-{
- int res;
-
- res = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
-
- if (!res) {
- s->intr_state |= E1000E_USE_MSI;
- } else {
- trace_e1000e_msi_init_fail(res);
- }
-}
-
-static void
-e1000e_cleanup_msi(E1000EState *s)
-{
- if (s->intr_state & E1000E_USE_MSI) {
- msi_uninit(PCI_DEVICE(s));
- }
-}
-
-static void
e1000e_unuse_msix_vectors(E1000EState *s, int num_vectors)
{
int i;
@@ -440,6 +417,7 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
static const uint16_t e1000e_dsn_offset = 0x140;
E1000EState *s = E1000E(pci_dev);
uint8_t *macaddr;
+ int ret;
trace_e1000e_cb_pci_realize();
@@ -489,7 +467,10 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
hw_error("Failed to initialize PCIe capability");
}
- e1000e_init_msi(s);
+ ret = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
+ if (ret) {
+ trace_e1000e_msi_init_fail(ret);
+ }
if (e1000e_add_pm_capability(pci_dev, e1000e_pmrb_offset,
PCI_PM_CAP_DSI) < 0) {
@@ -528,7 +509,7 @@ static void e1000e_pci_uninit(PCIDevice *pci_dev)
qemu_del_nic(s->nic);
e1000e_cleanup_msix(s);
- e1000e_cleanup_msi(s);
+ msi_uninit(pci_dev);
}
static void e1000e_qdev_reset(DeviceState *dev)
--
2.1.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init()
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (16 preceding siblings ...)
2016-06-10 9:54 ` [Qemu-devel] [PATCH v8 17/17] e1000e: " Cao jin
@ 2016-06-13 8:48 ` Markus Armbruster
2016-06-13 9:15 ` Cao jin
2016-06-13 19:45 ` Michael S. Tsirkin
18 siblings, 1 reply; 35+ messages in thread
From: Markus Armbruster @ 2016-06-13 8:48 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Michael S. Tsirkin, Jason Wang, Marcel Apfelbaum,
Alex Williamson, Hannes Reinecke, Dmitry Fleytman, Paolo Bonzini,
John Snow, Gerd Hoffmann
Cao jin <caoj.fnst@cn.fujitsu.com> writes:
> v8 changelog:
> 1. address all stylistic remarks (Markus)
> 2. add two new patches(16&17) which I missed in last round, shoot the unnecessary
> internal msi flag.
> 3. rebase on the upstream, fix trivial conflict in vmxnet3:
> -#include "vmxnet_tx_pkt.h"
> -#include "vmxnet_rx_pkt.h"
> +#include "net_tx_pkt.h"
> +#include "net_rx_pkt.h"
> 4. There is a new device "e1000e" added, need to cover it in patch 12.
>
> Hi Markus, I add your R-b in first 15 patches, maybe you want to take a glance
> at e1000e part in patch 12.
>
> The last two new patches need some review.
> Will take care of msix flag cleanup when tackle msix_init().
Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init()
2016-06-13 8:48 ` [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Markus Armbruster
@ 2016-06-13 9:15 ` Cao jin
0 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-13 9:15 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Michael S. Tsirkin, Jason Wang, Marcel Apfelbaum,
Alex Williamson, Hannes Reinecke, Dmitry Fleytman, Paolo Bonzini,
John Snow, Gerd Hoffmann
On 06/13/2016 04:48 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> v8 changelog:
>> 1. address all stylistic remarks (Markus)
>> 2. add two new patches(16&17) which I missed in last round, shoot the unnecessary
>> internal msi flag.
>> 3. rebase on the upstream, fix trivial conflict in vmxnet3:
>> -#include "vmxnet_tx_pkt.h"
>> -#include "vmxnet_rx_pkt.h"
>> +#include "net_tx_pkt.h"
>> +#include "net_rx_pkt.h"
>> 4. There is a new device "e1000e" added, need to cover it in patch 12.
>>
>> Hi Markus, I add your R-b in first 15 patches, maybe you want to take a glance
>> at e1000e part in patch 12.
>>
>> The last two new patches need some review.
>> Will take care of msix flag cleanup when tackle msix_init().
>
> Series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
Thanks very much. I will send a single patch 16 in-reply-to the current
in the series, to fix the stylistic issue.
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init()
2016-06-10 9:54 [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Cao jin
` (17 preceding siblings ...)
2016-06-13 8:48 ` [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init() Markus Armbruster
@ 2016-06-13 19:45 ` Michael S. Tsirkin
2016-06-13 20:31 ` Michael S. Tsirkin
18 siblings, 1 reply; 35+ messages in thread
From: Michael S. Tsirkin @ 2016-06-13 19:45 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Hannes Reinecke, Paolo Bonzini, Alex Williamson,
Markus Armbruster, Marcel Apfelbaum
Thanks!
I applied 1-12.
One of the cleanups breaks cross-version migration, so
I decided to defer them all. Once 1st part is merged,
please rebase and repost.
On Fri, Jun 10, 2016 at 05:54:21PM +0800, Cao jin wrote:
> v8 changelog:
> 1. address all stylistic remarks (Markus)
> 2. add two new patches(16&17) which I missed in last round, shoot the unnecessary
> internal msi flag.
> 3. rebase on the upstream, fix trivial conflict in vmxnet3:
> -#include "vmxnet_tx_pkt.h"
> -#include "vmxnet_rx_pkt.h"
> +#include "net_tx_pkt.h"
> +#include "net_rx_pkt.h"
> 4. There is a new device "e1000e" added, need to cover it in patch 12.
>
> Hi Markus, I add your R-b in first 15 patches, maybe you want to take a glance
> at e1000e part in patch 12.
>
> The last two new patches need some review.
> Will take care of msix flag cleanup when tackle msix_init().
>
> cc: Gerd Hoffmann <kraxel@redhat.com>
> cc: John Snow <jsnow@redhat.com>
> cc: Dmitry Fleytman <dmitry@daynix.com>
> cc: Jason Wang <jasowang@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Hannes Reinecke <hare@suse.de>
> cc: Paolo Bonzini <pbonzini@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
>
> Cao jin (17):
> pci core: assert ENOSPC when add capability
> fix some coding style problems
> change pvscsi_init_msi() type to void
> megasas: Fix check for msi_init() failure
> mptsas: change .realize function name
> usb xhci: change msi/msix property type
> intel-hda: change msi property type
> mptsas: change msi property type
> megasas: change msi/msix property type
> pci bridge dev: change msi property type
> msi_init: change return value to 0 on success
> pci: Convert msi_init() to Error and fix callers to check it
> megasas: remove unnecessary megasas_use_msi()
> mptsas: remove unnecessary internal msi state flag
> vmw_pvscsi: remove unnecessary internal msi state flag
> vmxnet3: remove unnecessary internal msi state flag
> e1000e: remove unnecessary internal msi state flag
>
> hw/audio/intel-hda.c | 29 +++++++++++++++----
> hw/ide/ich.c | 15 ++++++----
> hw/net/e1000e.c | 37 +++++-------------------
> hw/net/vmxnet3.c | 52 +++++++++++----------------------
> hw/pci-bridge/ioh3420.c | 13 +++++++--
> hw/pci-bridge/pci_bridge_dev.c | 35 ++++++++++++++++------
> hw/pci-bridge/xio3130_downstream.c | 12 ++++++--
> hw/pci-bridge/xio3130_upstream.c | 9 +++++-
> hw/pci/msi.c | 27 +++++++++++++++--
> hw/pci/pci.c | 6 ++--
> hw/scsi/megasas.c | 59 ++++++++++++++++++++------------------
> hw/scsi/mptsas.c | 40 +++++++++++++++++---------
> hw/scsi/mptsas.h | 5 ++--
> hw/scsi/vmw_pvscsi.c | 18 +++---------
> hw/usb/hcd-xhci.c | 35 ++++++++++++++++------
> hw/vfio/pci.c | 7 +++--
> include/hw/pci/msi.h | 3 +-
> 17 files changed, 233 insertions(+), 169 deletions(-)
>
> --
> 2.1.0
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init()
2016-06-13 19:45 ` Michael S. Tsirkin
@ 2016-06-13 20:31 ` Michael S. Tsirkin
2016-06-14 9:35 ` Cao jin
0 siblings, 1 reply; 35+ messages in thread
From: Michael S. Tsirkin @ 2016-06-13 20:31 UTC (permalink / raw)
To: Cao jin
Cc: qemu-devel, Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Hannes Reinecke, Paolo Bonzini, Alex Williamson,
Markus Armbruster, Marcel Apfelbaum
On Mon, Jun 13, 2016 at 10:45:18PM +0300, Michael S. Tsirkin wrote:
> Thanks!
> I applied 1-12.
And reverted most of them except 1-2: make check fails.
Pls rebase, retest and repost.
Thanks!
> One of the cleanups breaks cross-version migration, so
> I decided to defer them all. Once 1st part is merged,
> please rebase and repost.
>
> On Fri, Jun 10, 2016 at 05:54:21PM +0800, Cao jin wrote:
> > v8 changelog:
> > 1. address all stylistic remarks (Markus)
> > 2. add two new patches(16&17) which I missed in last round, shoot the unnecessary
> > internal msi flag.
> > 3. rebase on the upstream, fix trivial conflict in vmxnet3:
> > -#include "vmxnet_tx_pkt.h"
> > -#include "vmxnet_rx_pkt.h"
> > +#include "net_tx_pkt.h"
> > +#include "net_rx_pkt.h"
> > 4. There is a new device "e1000e" added, need to cover it in patch 12.
> >
> > Hi Markus, I add your R-b in first 15 patches, maybe you want to take a glance
> > at e1000e part in patch 12.
> >
> > The last two new patches need some review.
> > Will take care of msix flag cleanup when tackle msix_init().
> >
> > cc: Gerd Hoffmann <kraxel@redhat.com>
> > cc: John Snow <jsnow@redhat.com>
> > cc: Dmitry Fleytman <dmitry@daynix.com>
> > cc: Jason Wang <jasowang@redhat.com>
> > cc: Michael S. Tsirkin <mst@redhat.com>
> > cc: Hannes Reinecke <hare@suse.de>
> > cc: Paolo Bonzini <pbonzini@redhat.com>
> > cc: Alex Williamson <alex.williamson@redhat.com>
> > cc: Markus Armbruster <armbru@redhat.com>
> > cc: Marcel Apfelbaum <marcel@redhat.com>
> >
> > Cao jin (17):
> > pci core: assert ENOSPC when add capability
> > fix some coding style problems
> > change pvscsi_init_msi() type to void
> > megasas: Fix check for msi_init() failure
> > mptsas: change .realize function name
> > usb xhci: change msi/msix property type
> > intel-hda: change msi property type
> > mptsas: change msi property type
> > megasas: change msi/msix property type
> > pci bridge dev: change msi property type
> > msi_init: change return value to 0 on success
> > pci: Convert msi_init() to Error and fix callers to check it
> > megasas: remove unnecessary megasas_use_msi()
> > mptsas: remove unnecessary internal msi state flag
> > vmw_pvscsi: remove unnecessary internal msi state flag
> > vmxnet3: remove unnecessary internal msi state flag
> > e1000e: remove unnecessary internal msi state flag
> >
> > hw/audio/intel-hda.c | 29 +++++++++++++++----
> > hw/ide/ich.c | 15 ++++++----
> > hw/net/e1000e.c | 37 +++++-------------------
> > hw/net/vmxnet3.c | 52 +++++++++++----------------------
> > hw/pci-bridge/ioh3420.c | 13 +++++++--
> > hw/pci-bridge/pci_bridge_dev.c | 35 ++++++++++++++++------
> > hw/pci-bridge/xio3130_downstream.c | 12 ++++++--
> > hw/pci-bridge/xio3130_upstream.c | 9 +++++-
> > hw/pci/msi.c | 27 +++++++++++++++--
> > hw/pci/pci.c | 6 ++--
> > hw/scsi/megasas.c | 59 ++++++++++++++++++++------------------
> > hw/scsi/mptsas.c | 40 +++++++++++++++++---------
> > hw/scsi/mptsas.h | 5 ++--
> > hw/scsi/vmw_pvscsi.c | 18 +++---------
> > hw/usb/hcd-xhci.c | 35 ++++++++++++++++------
> > hw/vfio/pci.c | 7 +++--
> > include/hw/pci/msi.h | 3 +-
> > 17 files changed, 233 insertions(+), 169 deletions(-)
> >
> > --
> > 2.1.0
> >
> >
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v8 00/17] Add param Error ** for msi_init()
2016-06-13 20:31 ` Michael S. Tsirkin
@ 2016-06-14 9:35 ` Cao jin
0 siblings, 0 replies; 35+ messages in thread
From: Cao jin @ 2016-06-14 9:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Gerd Hoffmann, John Snow, Dmitry Fleytman, Jason Wang,
Hannes Reinecke, Paolo Bonzini, Alex Williamson,
Markus Armbruster, Marcel Apfelbaum
On 06/14/2016 04:31 AM, Michael S. Tsirkin wrote:
> On Mon, Jun 13, 2016 at 10:45:18PM +0300, Michael S. Tsirkin wrote:
>> Thanks!
>> I applied 1-12.
>
> And reverted most of them except 1-2: make check fails.
Sorry for that. It is actually not a bug, I changed the order of adding
pci capability in ich9-ahci, but test case "/ahci/hba_spec" always think
the 1st capability pointed by Capabilities pointer is MSI. will fix make
check when repost.
> Pls rebase, retest and repost.
>
> Thanks!
>
>
>> One of the cleanups breaks cross-version migration, so
>> I decided to defer them all. Once 1st part is merged,
>> please rebase and repost.
>>
>> On Fri, Jun 10, 2016 at 05:54:21PM +0800, Cao jin wrote:
--
Yours Sincerely,
Cao jin
^ permalink raw reply [flat|nested] 35+ messages in thread