* [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup
@ 2017-06-12 13:48 Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 1/9] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
` (8 more replies)
0 siblings, 9 replies; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel
Cc: mst, marcel, armbru, dmitry, jasowang, kraxel, alex.williamson,
pbonzini, rth, ehabkost
This series mainly implements the conversions of pci-bridge devices
i82801b11, io3130_upstream/downstream and so on to realize(). Naturally
part of error messages need to be converted to Error, then propagate
to its callers via the argument errp, bonus clean related minor flaw
up. In short, the former patches are prerequisites for latter ones.
v5:
* patch5: replace pci_add_capability2() with pci_add_capability(), because
it's confusing to have a function named pci_add_capability2() if
pci_add_capability() doesn't exist anymore. [Eduardo Habkost]
* patch8: a new patch that fix the return type of verify_irqchip_kernel().
* patch9: a new patch that use the errp directly instead of the local_err to
propagate the error messages.
v4:
* patch4: changed from patch 5 in v3. use a elegant way to check
the error, like
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
}
or
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
} else {
/* error path here */
return ret;
}
for readability, instead of this:
function(...)
if (function failed) {
return ...; /* or: "goto out" */
}
/* non-error code path here */
foo = bar; [Eduardo Habkost]
meanwhile, split previous patch4 out. [Michael S. Tsirkin]
* patch5: a new patch that replace pci_add_capability() with
pci_add_capability2(). [Eduardo Habkost]
v3:
* patch2: explain the specified means of the return value, also
improve the commit message. [Marcel Apfelbaum]
* patch3: simplify the subject and commit message, fix another
wrong assert. [Marcel Apfelbaum]
* patch4: adjust the subject.
* patch5: fix a wrong optimization for errp. [Eduardo Habkost]
* patch7: a new patch that converts shpc_init() to Error in order
to propagate the error better.
v2:
* patch1: subject and commit message was rewrited by markus.
* patch2: comment was added to pci_add_capability2().
* patch3: a new patch that fix the wrong return value judgment condition.
* patch4: a new patch that fix code style problems.
* patch5: add an errp argument for pci_add_capability to pass
error for its callers.
* patch6: convert part of pci-bridge device to realize.
v1:
* patch1: fix unreasonable return value check
Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Cc: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: kraxel@redhat.com
Cc: alex.williamson@redhat.com
Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Mao Zhongyi (9):
pci: Clean up error checking in pci_add_capability()
pci: Add comment for pci_add_capability2()
pci: Fix the return value checking
pci: Make errp the last parameter of pci_add_capability()
pci: Replace pci_add_capability2() with pci_add_capability()
pci: Convert to realize
pci: Convert shpc_init() to Error
i386/kvm/pci-assign: Fix return type of verify_irqchip_kernel()
i386/kvm/pci-assign: Use errp directly rather than local_err
hw/i386/amd_iommu.c | 24 ++++++++++++-----
hw/i386/kvm/pci-assign.c | 54 ++++++++++++++------------------------
hw/ide/ich.c | 2 +-
hw/net/e1000e.c | 30 ++++++++++++---------
hw/net/eepro100.c | 18 ++++++++++---
hw/pci-bridge/i82801b11.c | 12 ++++-----
hw/pci-bridge/pci_bridge_dev.c | 21 ++++++---------
hw/pci-bridge/pcie_root_port.c | 15 +++++------
hw/pci-bridge/xio3130_downstream.c | 20 +++++++-------
hw/pci-bridge/xio3130_upstream.c | 20 +++++++-------
hw/pci/msi.c | 2 +-
hw/pci/msix.c | 2 +-
hw/pci/pci.c | 24 +++--------------
hw/pci/pci_bridge.c | 8 ++++--
hw/pci/pcie.c | 15 ++++++++---
hw/pci/shpc.c | 10 ++++---
hw/pci/slotid_cap.c | 12 ++++++---
hw/usb/hcd-xhci.c | 2 +-
hw/vfio/pci.c | 15 ++++++-----
hw/virtio/virtio-pci.c | 12 ++++++---
include/hw/pci/pci.h | 2 --
include/hw/pci/pci_bridge.h | 3 ++-
include/hw/pci/pcie.h | 3 ++-
include/hw/pci/shpc.h | 3 ++-
include/hw/pci/slotid_cap.h | 3 ++-
25 files changed, 171 insertions(+), 161 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 1/9] pci: Clean up error checking in pci_add_capability()
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 2/9] pci: Add comment for pci_add_capability2() Mao Zhongyi
` (7 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel
On success, pci_add_capability2() returns a positive value. On
failure, it sets an error and return a negative value.
pci_add_capability() laboriously checks this behavior. No other
caller does. Drop the checks from pci_add_capability().
Cc: mst@redhat.com
Cc: marcel@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/pci/pci.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 98ccc27..53566b8 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2270,12 +2270,8 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
Error *local_err = NULL;
ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
- if (local_err) {
- assert(ret < 0);
+ if (ret < 0) {
error_report_err(local_err);
- } else {
- /* success implies a positive offset in config space */
- assert(ret > 0);
}
return ret;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 2/9] pci: Add comment for pci_add_capability2()
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 1/9] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-19 10:13 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 3/9] pci: Fix the return value checking Mao Zhongyi
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, armbru
Comments for pci_add_capability2() to explain the return
value. This may help to make a correct return value check
for its callers.
Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/pci/pci.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 53566b8..b73bfea 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2276,6 +2276,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
return ret;
}
+/*
+ * On success, pci_add_capability2() returns a positive value
+ * that the offset of the pci capability.
+ * On failure, it sets an error and returns a negative error
+ * code.
+ */
int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
Error **errp)
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 3/9] pci: Fix the return value checking
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 1/9] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 2/9] pci: Add comment for pci_add_capability2() Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 4/9] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
` (5 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: dmitry, jasowang, kraxel, alex.williamson, armbru
pci_add_capability returns a strictly positive value on success,
correct asserts.
Cc: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: kraxel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/net/e1000e.c | 2 +-
hw/net/eepro100.c | 2 +-
hw/usb/hcd-xhci.c | 2 +-
hw/vfio/pci.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 6e23493..8259d67 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -374,7 +374,7 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
{
int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
- if (ret >= 0) {
+ if (ret > 0) {
pci_set_word(pdev->config + offset + PCI_PM_PMC,
PCI_PM_CAP_VER_1_1 |
pmc);
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 4bf71f2..da36816 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -571,7 +571,7 @@ static void e100_pci_reset(EEPRO100State * s)
int cfg_offset = 0xdc;
int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
cfg_offset, PCI_PM_SIZEOF);
- assert(r >= 0);
+ assert(r > 0);
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
#if 0 /* TODO: replace dummy code for power management emulation. */
/* TODO: Power Management Control / Status. */
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index a0c7960..ab42f86 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3417,7 +3417,7 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
if (pci_bus_is_express(dev->bus) ||
xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
ret = pcie_endpoint_cap_init(dev, 0xa0);
- assert(ret >= 0);
+ assert(ret > 0);
}
if (xhci->msix != ON_OFF_AUTO_OFF) {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 32aca77..5881968 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1744,7 +1744,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
}
pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
- if (pos >= 0) {
+ if (pos > 0) {
vdev->pdev.exp.exp_cap = pos;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 4/9] pci: Make errp the last parameter of pci_add_capability()
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
` (2 preceding siblings ...)
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 3/9] pci: Fix the return value checking Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-19 11:17 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 5/9] pci: Replace pci_add_capability2() with pci_add_capability() Mao Zhongyi
` (4 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, rth, ehabkost, mst, jasowang, marcel, alex.williamson, armbru
Add Error argument for pci_add_capability() to leverage the errp
to pass info on errors. This way is helpful for its callers to
make a better error handling when moving to 'realize'.
Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: jasowang@redhat.com
Cc: marcel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
hw/net/e1000e.c | 30 ++++++++++++++++++------------
hw/net/eepro100.c | 18 ++++++++++++++----
hw/pci-bridge/i82801b11.c | 1 +
hw/pci/pci.c | 10 ++++------
hw/pci/pci_bridge.c | 7 ++++++-
hw/pci/pcie.c | 10 ++++++++--
hw/pci/shpc.c | 5 ++++-
hw/pci/slotid_cap.c | 7 ++++++-
hw/vfio/pci.c | 9 ++++++---
hw/virtio/virtio-pci.c | 12 ++++++++----
include/hw/pci/pci.h | 3 ++-
12 files changed, 94 insertions(+), 42 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 7b6d4ea..d93ffc2 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
x86_iommu->type = TYPE_AMD;
qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
- s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
- AMDVI_CAPAB_SIZE);
- assert(s->capab_offset > 0);
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
- assert(ret > 0);
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
- assert(ret > 0);
+ ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
+ AMDVI_CAPAB_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
+ s->capab_offset = ret;
+
+ ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
+ AMDVI_CAPAB_REG_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
+ ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
+ AMDVI_CAPAB_REG_SIZE, err);
+ if (ret < 0) {
+ return;
+ }
/* set up MMIO */
memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 8259d67..d1b1a97 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -47,6 +47,7 @@
#include "e1000e_core.h"
#include "trace.h"
+#include "qapi/error.h"
#define TYPE_E1000E "e1000e"
#define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
@@ -372,21 +373,26 @@ e1000e_gen_dsn(uint8_t *mac)
static int
e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
{
- int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
+ Error *local_err = NULL;
+ int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
+ PCI_PM_SIZEOF, &local_err);
- if (ret > 0) {
- pci_set_word(pdev->config + offset + PCI_PM_PMC,
- PCI_PM_CAP_VER_1_1 |
- pmc);
+ if (local_err) {
+ error_report_err(local_err);
+ return ret;
+ }
- pci_set_word(pdev->wmask + offset + PCI_PM_CTRL,
- PCI_PM_CTRL_STATE_MASK |
- PCI_PM_CTRL_PME_ENABLE |
- PCI_PM_CTRL_DATA_SEL_MASK);
+ pci_set_word(pdev->config + offset + PCI_PM_PMC,
+ PCI_PM_CAP_VER_1_1 |
+ pmc);
- pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
- PCI_PM_CTRL_PME_STATUS);
- }
+ pci_set_word(pdev->wmask + offset + PCI_PM_CTRL,
+ PCI_PM_CTRL_STATE_MASK |
+ PCI_PM_CTRL_PME_ENABLE |
+ PCI_PM_CTRL_DATA_SEL_MASK);
+
+ pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
+ PCI_PM_CTRL_PME_STATUS);
return ret;
}
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index da36816..5a4774a 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -48,6 +48,7 @@
#include "sysemu/sysemu.h"
#include "sysemu/dma.h"
#include "qemu/bitops.h"
+#include "qapi/error.h"
/* QEMU sends frames smaller than 60 bytes to ethernet nics.
* Such frames are rejected by real nics and their emulations.
@@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)
}
#endif
-static void e100_pci_reset(EEPRO100State * s)
+static void e100_pci_reset(EEPRO100State *s, Error **errp)
{
E100PCIDeviceInfo *info = eepro100_get_class(s);
uint32_t device = s->device;
@@ -570,8 +571,12 @@ static void e100_pci_reset(EEPRO100State * s)
/* Power Management Capabilities */
int cfg_offset = 0xdc;
int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
- cfg_offset, PCI_PM_SIZEOF);
- assert(r > 0);
+ cfg_offset, PCI_PM_SIZEOF,
+ errp);
+ if (r < 0) {
+ return;
+ }
+
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
#if 0 /* TODO: replace dummy code for power management emulation. */
/* TODO: Power Management Control / Status. */
@@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
{
EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
E100PCIDeviceInfo *info = eepro100_get_class(s);
+ Error *local_err = NULL;
TRACE(OTHER, logout("\n"));
s->device = info->device;
- e100_pci_reset(s);
+ e100_pci_reset(s, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
/* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
* i82559 and later support 64 or 256 word EEPROM. */
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2404e7e..2c065c3 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -44,6 +44,7 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
#include "hw/i386/ich9.h"
+#include "qapi/error.h"
/*****************************************************************************/
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b73bfea..2bba37a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
* in pci config space
*/
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size)
+ uint8_t offset, uint8_t size,
+ Error **errp)
{
int ret;
- Error *local_err = NULL;
- ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
- if (ret < 0) {
- error_report_err(local_err);
- }
+ ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
+
return ret;
}
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 5118ef4..bb0f3a3 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -33,6 +33,7 @@
#include "hw/pci/pci_bridge.h"
#include "hw/pci/pci_bus.h"
#include "qemu/range.h"
+#include "qapi/error.h"
/* PCI bridge subsystem vendor ID helper functions */
#define PCI_SSVID_SIZEOF 8
@@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
uint16_t svid, uint16_t ssid)
{
int pos;
- pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
+ Error *local_err = NULL;
+
+ pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
+ PCI_SSVID_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 18e634f..f187512 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
/* PCIe cap v2 init */
int pos;
uint8_t *exp_cap;
+ Error *local_err = NULL;
assert(pci_is_express(dev));
- pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+ PCI_EXP_VER2_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
@@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
{
/* PCIe cap v1 init */
int pos;
+ Error *local_err = NULL;
assert(pci_is_express(dev));
- pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
+ PCI_EXP_VER1_SIZEOF, &local_err);
if (pos < 0) {
+ error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 42fafac..d72d5e4 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
{
uint8_t *config;
int config_offset;
+ Error *local_err = NULL;
config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
- 0, SHPC_CAP_LENGTH);
+ 0, SHPC_CAP_LENGTH,
+ &local_err);
if (config_offset < 0) {
+ error_report_err(local_err);
return config_offset;
}
config = d->config + config_offset;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index aec1e91..bdca205 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -2,6 +2,7 @@
#include "hw/pci/slotid_cap.h"
#include "hw/pci/pci.h"
#include "qemu/error-report.h"
+#include "qapi/error.h"
#define SLOTID_CAP_LENGTH 4
#define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
@@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
unsigned offset)
{
int cap;
+ Error *local_err = NULL;
+
if (!chassis) {
error_report("Bridge chassis not specified. Each bridge is required "
"to be assigned a unique chassis id > 0.");
@@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
return -EINVAL;
}
- cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
+ cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
+ SLOTID_CAP_LENGTH, &local_err);
if (cap < 0) {
+ error_report_err(local_err);
return cap;
}
/* We make each chassis unique, this way each bridge is First in Chassis */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5881968..70bfb59 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1743,11 +1743,14 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
}
- pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
- if (pos > 0) {
- vdev->pdev.exp.exp_cap = pos;
+ pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
+ errp);
+ if (pos < 0) {
+ return pos;
}
+ vdev->pdev.exp.exp_cap = pos;
+
return pos;
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9b7244..1fc5059 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1162,8 +1162,8 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
PCIDevice *dev = &proxy->pci_dev;
int offset;
- offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
- assert(offset > 0);
+ offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
+ cap->cap_len, &error_abort);
assert(cap->cap_len >= sizeof *cap);
memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
@@ -1810,8 +1810,12 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
pos = pcie_endpoint_cap_init(pci_dev, 0);
assert(pos > 0);
- pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
- assert(pos > 0);
+ pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
+ PCI_PM_SIZEOF, errp);
+ if (pos < 0) {
+ return;
+ }
+
pci_dev->exp.pm_cap = pos;
/*
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index a37a2d5..fe52aa8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size);
+ uint8_t offset, uint8_t size,
+ Error **errp);
int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
Error **errp);
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 5/9] pci: Replace pci_add_capability2() with pci_add_capability()
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
` (3 preceding siblings ...)
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 4/9] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-19 11:20 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 6/9] pci: Convert to realize Mao Zhongyi
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel
Cc: pbonzini, rth, ehabkost, mst, dmitry, jasowang, marcel,
alex.williamson, armbru
After the patch 'Make errp the last parameter of pci_add_capability()',
pci_add_capability() and pci_add_capability2() now do exactly the same.
So drop the wrapper pci_add_capability() of pci_add_capability2(), then
replace the pci_add_capability2() with pci_add_capability() everywhere.
Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: dmitry@daynix.com
Cc: jasowang@redhat.com
Cc: marcel@redhat.com
Cc: alex.williamson@redhat.com
Cc: armbru@redhat.com
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/i386/kvm/pci-assign.c | 14 +++++++-------
hw/ide/ich.c | 2 +-
hw/pci/msi.c | 2 +-
hw/pci/msix.c | 2 +-
hw/pci/pci.c | 20 ++------------------
hw/vfio/pci.c | 6 +++---
include/hw/pci/pci.h | 3 ---
7 files changed, 15 insertions(+), 34 deletions(-)
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 87dcbdd..3d60455 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1254,7 +1254,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
dev->dev.cap_present |= QEMU_PCI_CAP_MSI;
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
/* Only 32-bit/no-mask currently supported */
- ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSI, pos, 10,
+ ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10,
&local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1288,7 +1288,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
}
dev->dev.cap_present |= QEMU_PCI_CAP_MSIX;
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
- ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
+ ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
&local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1318,7 +1318,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
if (pos) {
uint16_t pmc;
- ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
+ ret = pci_add_capability(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
&local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1386,7 +1386,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
return -EINVAL;
}
- ret = pci_add_capability2(pci_dev, PCI_CAP_ID_EXP, pos, size,
+ ret = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, pos, size,
&local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1462,7 +1462,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
uint32_t status;
/* Only expose the minimum, 8 byte capability */
- ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
+ ret = pci_add_capability(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
&local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1490,7 +1490,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VPD, 0);
if (pos) {
/* Direct R/W passthrough */
- ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VPD, pos, 8,
+ ret = pci_add_capability(pci_dev, PCI_CAP_ID_VPD, pos, 8,
&local_err);
if (ret < 0) {
error_propagate(errp, local_err);
@@ -1508,7 +1508,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
pos += PCI_CAP_LIST_NEXT) {
uint8_t len = pci_get_byte(pci_dev->config + pos + PCI_CAP_FLAGS);
/* Direct R/W passthrough */
- ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VNDR, pos, len,
+ ret = pci_add_capability(pci_dev, PCI_CAP_ID_VNDR, pos, len,
&local_err);
if (ret < 0) {
error_propagate(errp, local_err);
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 4599169..989fca5 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -130,7 +130,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
pci_register_bar(dev, ICH9_MEM_BAR, PCI_BASE_ADDRESS_SPACE_MEMORY,
&d->ahci.mem);
- sata_cap_offset = pci_add_capability2(dev, PCI_CAP_ID_SATA,
+ sata_cap_offset = pci_add_capability(dev, PCI_CAP_ID_SATA,
ICH9_SATA_CAP_OFFSET, SATA_CAP_SIZE,
errp);
if (sata_cap_offset < 0) {
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87b227..5e05ce5 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -216,7 +216,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
}
cap_size = msi_cap_sizeof(flags);
- config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
+ config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset,
cap_size, errp);
if (config_offset < 0) {
return config_offset;
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index bb54e8b..d634326 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -294,7 +294,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
return -EINVAL;
}
- cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
+ cap = pci_add_capability(dev, PCI_CAP_ID_MSIX,
cap_pos, MSIX_CAP_LENGTH, errp);
if (cap < 0) {
return cap;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2bba37a..283ca5e 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2259,28 +2259,12 @@ static void pci_del_option_rom(PCIDevice *pdev)
}
/*
- * if offset = 0,
- * Find and reserve space and add capability to the linked list
- * in pci config space
- */
-int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size,
- Error **errp)
-{
- int ret;
-
- ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
-
- return ret;
-}
-
-/*
- * On success, pci_add_capability2() returns a positive value
+ * On success, pci_add_capability() returns a positive value
* that the offset of the pci capability.
* On failure, it sets an error and returns a negative error
* code.
*/
-int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
+int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
Error **errp)
{
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 70bfb59..8de8272 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1837,14 +1837,14 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
case PCI_CAP_ID_PM:
vfio_check_pm_reset(vdev, pos);
vdev->pm_cap = pos;
- ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size, errp);
break;
case PCI_CAP_ID_AF:
vfio_check_af_flr(vdev, pos);
- ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size, errp);
break;
default:
- ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size, errp);
break;
}
out:
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index fe52aa8..e598b09 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -358,9 +358,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size,
Error **errp);
-int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size,
- Error **errp);
void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 6/9] pci: Convert to realize
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
` (4 preceding siblings ...)
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 5/9] pci: Replace pci_add_capability2() with pci_add_capability() Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-19 11:42 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error Mao Zhongyi
` (2 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, armbru
The pci-birdge device i82801b11 and io3130_upstream/downstream
still implements the old PCIDeviceClass .init() through *_init()
instead of the new .realize(). All devices need to be converted
to .realize(). So convert it and rename it to *_realize().
Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/pci-bridge/i82801b11.c | 11 +++++------
hw/pci-bridge/pcie_root_port.c | 15 ++++++---------
hw/pci-bridge/xio3130_downstream.c | 20 +++++++++-----------
hw/pci-bridge/xio3130_upstream.c | 20 +++++++++-----------
hw/pci/pci_bridge.c | 7 +++----
hw/pci/pcie.c | 11 ++++++-----
include/hw/pci/pci_bridge.h | 3 ++-
include/hw/pci/pcie.h | 3 ++-
8 files changed, 42 insertions(+), 48 deletions(-)
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 2c065c3..2c1b747 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -59,24 +59,23 @@ typedef struct I82801b11Bridge {
/*< public >*/
} I82801b11Bridge;
-static int i82801b11_bridge_initfn(PCIDevice *d)
+static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
{
int rc;
pci_bridge_initfn(d, TYPE_PCI_BUS);
rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
- I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
+ I82801ba_SSVID_SVID, I82801ba_SSVID_SSID,
+ errp);
if (rc < 0) {
goto err_bridge;
}
pci_config_set_prog_interface(d->config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
- return 0;
+ return;
err_bridge:
pci_bridge_exitfn(d);
-
- return rc;
}
static const VMStateDescription i82801b11_bridge_dev_vmstate = {
@@ -96,7 +95,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
k->vendor_id = PCI_VENDOR_ID_INTEL;
k->device_id = PCI_DEVICE_ID_INTEL_82801BA_11;
k->revision = ICH9_D2P_A2_REVISION;
- k->init = i82801b11_bridge_initfn;
+ k->realize = i82801b11_bridge_realize;
k->config_write = pci_bridge_write_config;
dc->vmsd = &i82801b11_bridge_dev_vmstate;
set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index cf36318..00f0b1f 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -59,29 +59,27 @@ static void rp_realize(PCIDevice *d, Error **errp)
PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
int rc;
- Error *local_err = NULL;
pci_config_set_interrupt_pin(d->config, 1);
pci_bridge_initfn(d, TYPE_PCIE_BUS);
pcie_port_init_reg(d);
- rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, rpc->ssid);
+ rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
+ rpc->ssid, errp);
if (rc < 0) {
- error_setg(errp, "Can't init SSV ID, error %d", rc);
goto err_bridge;
}
if (rpc->interrupts_init) {
- rc = rpc->interrupts_init(d, &local_err);
+ rc = rpc->interrupts_init(d, errp);
if (rc < 0) {
- error_propagate(errp, local_err);
goto err_bridge;
}
}
- rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT, p->port);
+ rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
+ p->port, errp);
if (rc < 0) {
- error_setg(errp, "Can't add Root Port capability, error %d", rc);
goto err_int;
}
@@ -98,9 +96,8 @@ static void rp_realize(PCIDevice *d, Error **errp)
}
rc = pcie_aer_init(d, PCI_ERR_VER, rpc->aer_offset,
- PCI_ERR_SIZEOF, &local_err);
+ PCI_ERR_SIZEOF, errp);
if (rc < 0) {
- error_propagate(errp, local_err);
goto err;
}
pcie_aer_root_init(d);
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index cfe8a36..e706f36 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
pci_bridge_reset(qdev);
}
-static int xio3130_downstream_initfn(PCIDevice *d)
+static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
{
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, &err);
+ XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+ errp);
if (rc < 0) {
assert(rc == -ENOTSUP);
- error_report_err(err);
goto err_bridge;
}
rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
- XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
+ XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
+ errp);
if (rc < 0) {
goto err_bridge;
}
rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
- p->port);
+ p->port, errp);
if (rc < 0) {
goto err_msi;
}
@@ -98,13 +98,12 @@ static int xio3130_downstream_initfn(PCIDevice *d)
}
rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
- PCI_ERR_SIZEOF, &err);
+ PCI_ERR_SIZEOF, errp);
if (rc < 0) {
- error_report_err(err);
goto err;
}
- return 0;
+ return;
err:
pcie_chassis_del_slot(s);
@@ -114,7 +113,6 @@ err_msi:
msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
- return rc;
}
static void xio3130_downstream_exitfn(PCIDevice *d)
@@ -181,7 +179,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
k->is_express = 1;
k->is_bridge = 1;
k->config_write = xio3130_downstream_write_config;
- k->init = xio3130_downstream_initfn;
+ k->realize = xio3130_downstream_realize;
k->exit = xio3130_downstream_exitfn;
k->vendor_id = PCI_VENDOR_ID_TI;
k->device_id = PCI_DEVICE_ID_TI_XIO3130D;
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 401c784..a052224 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -53,32 +53,32 @@ static void xio3130_upstream_reset(DeviceState *qdev)
pcie_cap_deverr_reset(d);
}
-static int xio3130_upstream_initfn(PCIDevice *d)
+static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
{
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, &err);
+ XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
+ errp);
if (rc < 0) {
assert(rc == -ENOTSUP);
- error_report_err(err);
goto err_bridge;
}
rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
- XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
+ XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
+ errp);
if (rc < 0) {
goto err_bridge;
}
rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
- p->port);
+ p->port, errp);
if (rc < 0) {
goto err_msi;
}
@@ -86,13 +86,12 @@ static int xio3130_upstream_initfn(PCIDevice *d)
pcie_cap_deverr_init(d);
rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
- PCI_ERR_SIZEOF, &err);
+ PCI_ERR_SIZEOF, errp);
if (rc < 0) {
- error_report_err(err);
goto err;
}
- return 0;
+ return;
err:
pcie_cap_exit(d);
@@ -100,7 +99,6 @@ err_msi:
msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
- return rc;
}
static void xio3130_upstream_exitfn(PCIDevice *d)
@@ -153,7 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass, void *data)
k->is_express = 1;
k->is_bridge = 1;
k->config_write = xio3130_upstream_write_config;
- k->init = xio3130_upstream_initfn;
+ k->realize = xio3130_upstream_realize;
k->exit = xio3130_upstream_exitfn;
k->vendor_id = PCI_VENDOR_ID_TI;
k->device_id = PCI_DEVICE_ID_TI_XIO3130U;
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index bb0f3a3..720119b 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -41,15 +41,14 @@
#define PCI_SSVID_SSID 6
int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
- uint16_t svid, uint16_t ssid)
+ uint16_t svid, uint16_t ssid,
+ Error **errp)
{
int pos;
- Error *local_err = NULL;
pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
- PCI_SSVID_SIZEOF, &local_err);
+ PCI_SSVID_SIZEOF, errp);
if (pos < 0) {
- error_report_err(local_err);
return pos;
}
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index f187512..05d091a 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -86,19 +86,19 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
pci_set_word(cmask + PCI_EXP_LNKSTA, 0);
}
-int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
+int pcie_cap_init(PCIDevice *dev, uint8_t offset,
+ uint8_t type, uint8_t port,
+ Error **errp)
{
/* PCIe cap v2 init */
int pos;
uint8_t *exp_cap;
- Error *local_err = NULL;
assert(pci_is_express(dev));
pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
- PCI_EXP_VER2_SIZEOF, &local_err);
+ PCI_EXP_VER2_SIZEOF, errp);
if (pos < 0) {
- error_report_err(local_err);
return pos;
}
dev->exp.exp_cap = pos;
@@ -147,6 +147,7 @@ static int
pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
{
uint8_t type = PCI_EXP_TYPE_ENDPOINT;
+ Error *local_err = NULL;
/*
* Windows guests will report Code 10, device cannot start, if
@@ -159,7 +160,7 @@ pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
return (cap_size == PCI_EXP_VER1_SIZEOF)
? pcie_cap_v1_init(dev, offset, type, 0)
- : pcie_cap_init(dev, offset, type, 0);
+ : pcie_cap_init(dev, offset, type, 0, &local_err);
}
int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index d5891cd..ff7cbaa 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -33,7 +33,8 @@
#define PCI_BRIDGE_DEV_PROP_SHPC "shpc"
int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
- uint16_t svid, uint16_t ssid);
+ uint16_t svid, uint16_t ssid,
+ Error **errp);
PCIDevice *pci_bridge_get_device(PCIBus *bus);
PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 3d8f24b..b71e369 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -84,7 +84,8 @@ struct PCIExpressDevice {
#define COMPAT_PROP_PCP "power_controller_present"
/* PCI express capability helper functions */
-int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
+int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
+ uint8_t port, Error **errp);
int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
uint8_t type, uint8_t port);
int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
` (5 preceding siblings ...)
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 6/9] pci: Convert to realize Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-19 12:05 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 8/9] i386/kvm/pci-assign: Fix return type of verify_irqchip_kernel() Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 9/9] i386/kvm/pci-assign: Use errp directly rather than local_err Mao Zhongyi
8 siblings, 1 reply; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, marcel, armbru
In order to propagate error message better, convert shpc_init() to
Error also convert the pci_bridge_dev_initfn() to realize.
Cc: mst@redhat.com
Cc: marcel@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
hw/pci/shpc.c | 11 +++++------
hw/pci/slotid_cap.c | 11 +++++------
include/hw/pci/shpc.h | 3 ++-
include/hw/pci/slotid_cap.h | 3 ++-
5 files changed, 22 insertions(+), 27 deletions(-)
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 5dbd933..30c4186 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -49,12 +49,11 @@ struct PCIBridgeDev {
};
typedef struct PCIBridgeDev PCIBridgeDev;
-static int pci_bridge_dev_initfn(PCIDevice *dev)
+static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
{
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);
@@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
dev->config[PCI_INTERRUPT_PIN] = 0x1;
memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
shpc_bar_size(dev));
- err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
+ err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
if (err) {
goto shpc_error;
}
@@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
bridge_dev->msi = ON_OFF_AUTO_OFF;
}
- err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
+ err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
if (err) {
goto slotid_error;
}
@@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
/* it means SHPC exists, because MSI is needed by SHPC */
- err = msi_init(dev, 0, 1, true, true, &local_err);
+ err = msi_init(dev, 0, 1, true, true, errp);
/* 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) "
+ error_append_hint(errp, "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);
+ assert(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)) {
@@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
}
- return 0;
+ return;
msi_error:
slotid_cap_cleanup(dev);
@@ -111,8 +108,6 @@ slotid_error:
}
shpc_error:
pci_bridge_exitfn(dev);
-
- return err;
}
static void pci_bridge_dev_exitfn(PCIDevice *dev)
@@ -216,7 +211,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
- k->init = pci_bridge_dev_initfn;
+ k->realize = pci_bridge_dev_realize;
k->exit = pci_bridge_dev_exitfn;
k->config_write = pci_bridge_dev_write_config;
k->vendor_id = PCI_VENDOR_ID_REDHAT;
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index d72d5e4..69fc14b 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
}
/* Add SHPC capability to the config space for the device. */
-static int shpc_cap_add_config(PCIDevice *d)
+static int shpc_cap_add_config(PCIDevice *d, Error **errp)
{
uint8_t *config;
int config_offset;
- Error *local_err = NULL;
config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
0, SHPC_CAP_LENGTH,
- &local_err);
+ errp);
if (config_offset < 0) {
- error_report_err(local_err);
return config_offset;
}
config = d->config + config_offset;
@@ -584,13 +582,14 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
}
/* Initialize the SHPC structure in bridge's BAR. */
-int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
+int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
+ unsigned offset, Error **errp)
{
int i, ret;
int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
shpc->sec_bus = sec_bus;
- ret = shpc_cap_add_config(d);
+ ret = shpc_cap_add_config(d, errp);
if (ret) {
g_free(d->shpc);
return ret;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index bdca205..36d021b 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -9,14 +9,14 @@
int slotid_cap_init(PCIDevice *d, int nslots,
uint8_t chassis,
- unsigned offset)
+ unsigned offset,
+ Error **errp)
{
int cap;
- Error *local_err = NULL;
if (!chassis) {
- error_report("Bridge chassis not specified. Each bridge is required "
- "to be assigned a unique chassis id > 0.");
+ error_setg(errp, "Bridge chassis not specified. Each bridge is required"
+ " to be assigned a unique chassis id > 0.");
return -EINVAL;
}
if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >> SLOTID_NSLOTS_SHIFT)) {
@@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
}
cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
- SLOTID_CAP_LENGTH, &local_err);
+ SLOTID_CAP_LENGTH, errp);
if (cap < 0) {
- error_report_err(local_err);
return cap;
}
/* We make each chassis unique, this way each bridge is First in Chassis */
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 71e836b..ee19fec 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -38,7 +38,8 @@ struct SHPCDevice {
void shpc_reset(PCIDevice *d);
int shpc_bar_size(PCIDevice *dev);
-int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
+int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
+ unsigned off, Error **errp);
void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
void shpc_free(PCIDevice *dev);
void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
index 70db047..a777ea0 100644
--- a/include/hw/pci/slotid_cap.h
+++ b/include/hw/pci/slotid_cap.h
@@ -5,7 +5,8 @@
int slotid_cap_init(PCIDevice *dev, int nslots,
uint8_t chassis,
- unsigned offset);
+ unsigned offset,
+ Error **errp);
void slotid_cap_cleanup(PCIDevice *dev);
#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 8/9] i386/kvm/pci-assign: Fix return type of verify_irqchip_kernel()
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
` (6 preceding siblings ...)
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 9/9] i386/kvm/pci-assign: Use errp directly rather than local_err Mao Zhongyi
8 siblings, 0 replies; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, rth, ehabkost, mst, armbru
When the function no success value to transmit, it usually make the
function return void. It has turned out not to be a success, because
it means that the extra local_err variable and error_propagate() will
be needed. It leads to cumbersome code, therefore, transmit success/
failure in the return value is worth. So fix the return type to avoid
it.
Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/i386/kvm/pci-assign.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 3d60455..b7fdb47 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -824,12 +824,13 @@ static void assign_device(AssignedDevice *dev, Error **errp)
}
}
-static void verify_irqchip_in_kernel(Error **errp)
+static int verify_irqchip_in_kernel(Error **errp)
{
if (kvm_irqchip_in_kernel()) {
- return;
+ return 0;
}
error_setg(errp, "pci-assign requires KVM with in-kernel irqchip enabled");
+ return -1;
}
static int assign_intx(AssignedDevice *dev, Error **errp)
@@ -838,7 +839,6 @@ static int assign_intx(AssignedDevice *dev, Error **errp)
PCIINTxRoute intx_route;
bool intx_host_msi;
int r;
- Error *local_err = NULL;
/* Interrupt PIN 0 means don't use INTx */
if (assigned_dev_pci_read_byte(&dev->dev, PCI_INTERRUPT_PIN) == 0) {
@@ -846,9 +846,7 @@ static int assign_intx(AssignedDevice *dev, Error **errp)
return 0;
}
- verify_irqchip_in_kernel(&local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (verify_irqchip_in_kernel(errp) < 0) {
return -ENOTSUP;
}
@@ -1246,9 +1244,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
* MSI capability is the 1st capability in capability config */
pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI, 0);
if (pos != 0 && kvm_check_extension(kvm_state, KVM_CAP_ASSIGN_DEV_IRQ)) {
- verify_irqchip_in_kernel(&local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (verify_irqchip_in_kernel(errp) < 0) {
return -ENOTSUP;
}
dev->dev.cap_present |= QEMU_PCI_CAP_MSI;
@@ -1281,9 +1277,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
uint32_t msix_table_entry;
uint16_t msix_max;
- verify_irqchip_in_kernel(&local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (verify_irqchip_in_kernel(errp) < 0) {
return -ENOTSUP;
}
dev->dev.cap_present |= QEMU_PCI_CAP_MSIX;
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH v5 9/9] i386/kvm/pci-assign: Use errp directly rather than local_err
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
` (7 preceding siblings ...)
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 8/9] i386/kvm/pci-assign: Fix return type of verify_irqchip_kernel() Mao Zhongyi
@ 2017-06-12 13:48 ` Mao Zhongyi
8 siblings, 0 replies; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-12 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, rth, ehabkost, mst, armbru
In assigned_device_pci_cap_init(), first, error messages are filled
to a local_err variable, then through error_propagate() pass to
the parameter of errp. It leads to cumbersome code. In order to
avoid the extra local_err and error_propagate(), drop it and use
errp instead.
Cc: pbonzini@redhat.com
Cc: rth@twiddle.net
Cc: ehabkost@redhat.com
Cc: mst@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/i386/kvm/pci-assign.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index b7fdb47..9f2615c 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1232,7 +1232,6 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
AssignedDevice *dev = PCI_ASSIGN(pci_dev);
PCIRegion *pci_region = dev->real_device.regions;
int ret, pos;
- Error *local_err = NULL;
/* Clear initial capabilities pointer and status copied from hw */
pci_set_byte(pci_dev->config + PCI_CAPABILITY_LIST, 0);
@@ -1251,9 +1250,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
/* Only 32-bit/no-mask currently supported */
ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10,
- &local_err);
+ errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
pci_dev->msi_cap = pos;
@@ -1283,9 +1281,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
dev->dev.cap_present |= QEMU_PCI_CAP_MSIX;
dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
- &local_err);
+ errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
pci_dev->msix_cap = pos;
@@ -1313,9 +1310,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
uint16_t pmc;
ret = pci_add_capability(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
- &local_err);
+ errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
@@ -1381,9 +1377,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
}
ret = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, pos, size,
- &local_err);
+ errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
@@ -1457,9 +1452,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
/* Only expose the minimum, 8 byte capability */
ret = pci_add_capability(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
- &local_err);
+ errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
@@ -1485,9 +1479,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
if (pos) {
/* Direct R/W passthrough */
ret = pci_add_capability(pci_dev, PCI_CAP_ID_VPD, pos, 8,
- &local_err);
+ errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
@@ -1503,9 +1496,8 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
uint8_t len = pci_get_byte(pci_dev->config + pos + PCI_CAP_FLAGS);
/* Direct R/W passthrough */
ret = pci_add_capability(pci_dev, PCI_CAP_ID_VNDR, pos, len,
- &local_err);
+ errp);
if (ret < 0) {
- error_propagate(errp, local_err);
return ret;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 2/9] pci: Add comment for pci_add_capability2()
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 2/9] pci: Add comment for pci_add_capability2() Mao Zhongyi
@ 2017-06-19 10:13 ` Marcel Apfelbaum
0 siblings, 0 replies; 21+ messages in thread
From: Marcel Apfelbaum @ 2017-06-19 10:13 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel; +Cc: mst, armbru
On 12/06/2017 16:48, Mao Zhongyi wrote:
> Comments for pci_add_capability2() to explain the return
> value. This may help to make a correct return value check
> for its callers.
>
> Cc: mst@redhat.com
> Cc: marcel@redhat.com
> Cc: armbru@redhat.com
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/pci/pci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 53566b8..b73bfea 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2276,6 +2276,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> return ret;
> }
>
> +/*
> + * On success, pci_add_capability2() returns a positive value
> + * that the offset of the pci capability.
> + * On failure, it sets an error and returns a negative error
> + * code.
> + */
> int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
> uint8_t offset, uint8_t size,
> Error **errp)
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/9] pci: Make errp the last parameter of pci_add_capability()
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 4/9] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
@ 2017-06-19 11:17 ` Marcel Apfelbaum
0 siblings, 0 replies; 21+ messages in thread
From: Marcel Apfelbaum @ 2017-06-19 11:17 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel
Cc: pbonzini, rth, ehabkost, mst, jasowang, alex.williamson, armbru
On 12/06/2017 16:48, Mao Zhongyi wrote:
> Add Error argument for pci_add_capability() to leverage the errp
> to pass info on errors. This way is helpful for its callers to
> make a better error handling when moving to 'realize'.
>
> Cc: pbonzini@redhat.com
> Cc: rth@twiddle.net
> Cc: ehabkost@redhat.com
> Cc: mst@redhat.com
> Cc: jasowang@redhat.com
> Cc: marcel@redhat.com
> Cc: alex.williamson@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
> hw/net/e1000e.c | 30 ++++++++++++++++++------------
> hw/net/eepro100.c | 18 ++++++++++++++----
> hw/pci-bridge/i82801b11.c | 1 +
> hw/pci/pci.c | 10 ++++------
> hw/pci/pci_bridge.c | 7 ++++++-
> hw/pci/pcie.c | 10 ++++++++--
> hw/pci/shpc.c | 5 ++++-
> hw/pci/slotid_cap.c | 7 ++++++-
> hw/vfio/pci.c | 9 ++++++---
> hw/virtio/virtio-pci.c | 12 ++++++++----
> include/hw/pci/pci.h | 3 ++-
> 12 files changed, 94 insertions(+), 42 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 7b6d4ea..d93ffc2 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1158,13 +1158,23 @@ static void amdvi_realize(DeviceState *dev, Error **err)
> x86_iommu->type = TYPE_AMD;
> qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
> object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
> - s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> - AMDVI_CAPAB_SIZE);
> - assert(s->capab_offset > 0);
> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
> - assert(ret > 0);
> - ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
> - assert(ret > 0);
> + ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> + AMDVI_CAPAB_SIZE, err);
> + if (ret < 0) {
> + return;
> + }
> + s->capab_offset = ret;
> +
> + ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
> + AMDVI_CAPAB_REG_SIZE, err);
> + if (ret < 0) {
> + return;
> + }
> + ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
> + AMDVI_CAPAB_REG_SIZE, err);
> + if (ret < 0) {
> + return;
> + }
>
> /* set up MMIO */
> memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 8259d67..d1b1a97 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -47,6 +47,7 @@
> #include "e1000e_core.h"
>
> #include "trace.h"
> +#include "qapi/error.h"
>
> #define TYPE_E1000E "e1000e"
> #define E1000E(obj) OBJECT_CHECK(E1000EState, (obj), TYPE_E1000E)
> @@ -372,21 +373,26 @@ e1000e_gen_dsn(uint8_t *mac)
> static int
> e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
> {
> - int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
> + Error *local_err = NULL;
> + int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
> + PCI_PM_SIZEOF, &local_err);
>
> - if (ret > 0) {
> - pci_set_word(pdev->config + offset + PCI_PM_PMC,
> - PCI_PM_CAP_VER_1_1 |
> - pmc);
> + if (local_err) {
> + error_report_err(local_err);
> + return ret;
> + }
>
> - pci_set_word(pdev->wmask + offset + PCI_PM_CTRL,
> - PCI_PM_CTRL_STATE_MASK |
> - PCI_PM_CTRL_PME_ENABLE |
> - PCI_PM_CTRL_DATA_SEL_MASK);
> + pci_set_word(pdev->config + offset + PCI_PM_PMC,
> + PCI_PM_CAP_VER_1_1 |
> + pmc);
>
> - pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
> - PCI_PM_CTRL_PME_STATUS);
> - }
> + pci_set_word(pdev->wmask + offset + PCI_PM_CTRL,
> + PCI_PM_CTRL_STATE_MASK |
> + PCI_PM_CTRL_PME_ENABLE |
> + PCI_PM_CTRL_DATA_SEL_MASK);
> +
> + pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
> + PCI_PM_CTRL_PME_STATUS);
>
> return ret;
> }
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index da36816..5a4774a 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -48,6 +48,7 @@
> #include "sysemu/sysemu.h"
> #include "sysemu/dma.h"
> #include "qemu/bitops.h"
> +#include "qapi/error.h"
>
> /* QEMU sends frames smaller than 60 bytes to ethernet nics.
> * Such frames are rejected by real nics and their emulations.
> @@ -494,7 +495,7 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)
> }
> #endif
>
> -static void e100_pci_reset(EEPRO100State * s)
> +static void e100_pci_reset(EEPRO100State *s, Error **errp)
> {
> E100PCIDeviceInfo *info = eepro100_get_class(s);
> uint32_t device = s->device;
> @@ -570,8 +571,12 @@ static void e100_pci_reset(EEPRO100State * s)
> /* Power Management Capabilities */
> int cfg_offset = 0xdc;
> int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
> - cfg_offset, PCI_PM_SIZEOF);
> - assert(r > 0);
> + cfg_offset, PCI_PM_SIZEOF,
> + errp);
> + if (r < 0
> + return;
> + }
> +
> pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> #if 0 /* TODO: replace dummy code for power management emulation. */
> /* TODO: Power Management Control / Status. */
> @@ -1858,12 +1863,17 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
> {
> EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
> E100PCIDeviceInfo *info = eepro100_get_class(s);
> + Error *local_err = NULL;
>
> TRACE(OTHER, logout("\n"));
>
> s->device = info->device;
>
> - e100_pci_reset(s);
> + e100_pci_reset(s, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
>
> /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
> * i82559 and later support 64 or 256 word EEPROM. */
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index 2404e7e..2c065c3 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -44,6 +44,7 @@
> #include "qemu/osdep.h"
> #include "hw/pci/pci.h"
> #include "hw/i386/ich9.h"
> +#include "qapi/error.h"
>
>
> /*****************************************************************************/
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b73bfea..2bba37a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2264,15 +2264,13 @@ static void pci_del_option_rom(PCIDevice *pdev)
> * in pci config space
> */
> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size)
> + uint8_t offset, uint8_t size,
> + Error **errp)
> {
> int ret;
> - Error *local_err = NULL;
>
> - ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
> - if (ret < 0) {
> - error_report_err(local_err);
> - }
> + ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
> +
> return ret;
> }
>
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 5118ef4..bb0f3a3 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -33,6 +33,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "hw/pci/pci_bus.h"
> #include "qemu/range.h"
> +#include "qapi/error.h"
>
> /* PCI bridge subsystem vendor ID helper functions */
> #define PCI_SSVID_SIZEOF 8
> @@ -43,8 +44,12 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
> uint16_t svid, uint16_t ssid)
> {
> int pos;
> - pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
> + Error *local_err = NULL;
> +
> + pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
> + PCI_SSVID_SIZEOF, &local_err);
> if (pos < 0) {
> + error_report_err(local_err);
> return pos;
> }
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 18e634f..f187512 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -91,11 +91,14 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
> /* PCIe cap v2 init */
> int pos;
> uint8_t *exp_cap;
> + Error *local_err = NULL;
>
> assert(pci_is_express(dev));
>
> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> + PCI_EXP_VER2_SIZEOF, &local_err);
> if (pos < 0) {
> + error_report_err(local_err);
> return pos;
> }
> dev->exp.exp_cap = pos;
> @@ -123,11 +126,14 @@ int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
> {
> /* PCIe cap v1 init */
> int pos;
> + Error *local_err = NULL;
>
> assert(pci_is_express(dev));
>
> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> + PCI_EXP_VER1_SIZEOF, &local_err);
> if (pos < 0) {
> + error_report_err(local_err);
> return pos;
> }
> dev->exp.exp_cap = pos;
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 42fafac..d72d5e4 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -450,9 +450,12 @@ static int shpc_cap_add_config(PCIDevice *d)
> {
> uint8_t *config;
> int config_offset;
> + Error *local_err = NULL;
> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
> - 0, SHPC_CAP_LENGTH);
> + 0, SHPC_CAP_LENGTH,
> + &local_err);
> if (config_offset < 0) {
> + error_report_err(local_err);
> return config_offset;
> }
> config = d->config + config_offset;
> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
> index aec1e91..bdca205 100644
> --- a/hw/pci/slotid_cap.c
> +++ b/hw/pci/slotid_cap.c
> @@ -2,6 +2,7 @@
> #include "hw/pci/slotid_cap.h"
> #include "hw/pci/pci.h"
> #include "qemu/error-report.h"
> +#include "qapi/error.h"
>
> #define SLOTID_CAP_LENGTH 4
> #define SLOTID_NSLOTS_SHIFT ctz32(PCI_SID_ESR_NSLOTS)
> @@ -11,6 +12,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
> unsigned offset)
> {
> int cap;
> + Error *local_err = NULL;
> +
> if (!chassis) {
> error_report("Bridge chassis not specified. Each bridge is required "
> "to be assigned a unique chassis id > 0.");
> @@ -21,8 +24,10 @@ int slotid_cap_init(PCIDevice *d, int nslots,
> return -EINVAL;
> }
>
> - cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
> + cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
> + SLOTID_CAP_LENGTH, &local_err);
> if (cap < 0) {
> + error_report_err(local_err);
> return cap;
> }
> /* We make each chassis unique, this way each bridge is First in Chassis */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5881968..70bfb59 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1743,11 +1743,14 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
> PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
> }
>
> - pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
> - if (pos > 0) {
> - vdev->pdev.exp.exp_cap = pos;
> + pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
> + errp);
> + if (pos < 0) {
> + return pos;
> }
>
> + vdev->pdev.exp.exp_cap = pos;
> +
> return pos;
> }
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..1fc5059 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1162,8 +1162,8 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
> PCIDevice *dev = &proxy->pci_dev;
> int offset;
>
> - offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
> - assert(offset > 0);
> + offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
> + cap->cap_len, &error_abort);
>
> assert(cap->cap_len >= sizeof *cap);
> memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
> @@ -1810,8 +1810,12 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
> pos = pcie_endpoint_cap_init(pci_dev, 0);
> assert(pos > 0);
>
> - pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
> - assert(pos > 0);
> + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
> + PCI_PM_SIZEOF, errp);
> + if (pos < 0) {
> + return;
> + }
> +
> pci_dev->exp.pm_cap = pos;
>
> /*
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a37a2d5..fe52aa8 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -356,7 +356,8 @@ void pci_unregister_vga(PCIDevice *pci_dev);
> pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
>
> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size);
> + uint8_t offset, uint8_t size,
> + Error **errp);
> int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
> uint8_t offset, uint8_t size,
> Error **errp);
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 5/9] pci: Replace pci_add_capability2() with pci_add_capability()
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 5/9] pci: Replace pci_add_capability2() with pci_add_capability() Mao Zhongyi
@ 2017-06-19 11:20 ` Marcel Apfelbaum
0 siblings, 0 replies; 21+ messages in thread
From: Marcel Apfelbaum @ 2017-06-19 11:20 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel
Cc: pbonzini, rth, ehabkost, mst, dmitry, jasowang, alex.williamson, armbru
On 12/06/2017 16:48, Mao Zhongyi wrote:
> After the patch 'Make errp the last parameter of pci_add_capability()',
> pci_add_capability() and pci_add_capability2() now do exactly the same.
> So drop the wrapper pci_add_capability() of pci_add_capability2(), then
> replace the pci_add_capability2() with pci_add_capability() everywhere.
>
> Cc: pbonzini@redhat.com
> Cc: rth@twiddle.net
> Cc: ehabkost@redhat.com
> Cc: mst@redhat.com
> Cc: dmitry@daynix.com
> Cc: jasowang@redhat.com
> Cc: marcel@redhat.com
> Cc: alex.williamson@redhat.com
> Cc: armbru@redhat.com
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/i386/kvm/pci-assign.c | 14 +++++++-------
> hw/ide/ich.c | 2 +-
> hw/pci/msi.c | 2 +-
> hw/pci/msix.c | 2 +-
> hw/pci/pci.c | 20 ++------------------
> hw/vfio/pci.c | 6 +++---
> include/hw/pci/pci.h | 3 ---
> 7 files changed, 15 insertions(+), 34 deletions(-)
>
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index 87dcbdd..3d60455 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -1254,7 +1254,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
> dev->dev.cap_present |= QEMU_PCI_CAP_MSI;
> dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
> /* Only 32-bit/no-mask currently supported */
> - ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSI, pos, 10,
> + ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10,
> &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1288,7 +1288,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
> }
> dev->dev.cap_present |= QEMU_PCI_CAP_MSIX;
> dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
> - ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
> + ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
> &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1318,7 +1318,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
> if (pos) {
> uint16_t pmc;
>
> - ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
> + ret = pci_add_capability(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
> &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1386,7 +1386,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
> return -EINVAL;
> }
>
> - ret = pci_add_capability2(pci_dev, PCI_CAP_ID_EXP, pos, size,
> + ret = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, pos, size,
> &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1462,7 +1462,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
> uint32_t status;
>
> /* Only expose the minimum, 8 byte capability */
> - ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
> + ret = pci_add_capability(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
> &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1490,7 +1490,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
> pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VPD, 0);
> if (pos) {
> /* Direct R/W passthrough */
> - ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VPD, pos, 8,
> + ret = pci_add_capability(pci_dev, PCI_CAP_ID_VPD, pos, 8,
> &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> @@ -1508,7 +1508,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
> pos += PCI_CAP_LIST_NEXT) {
> uint8_t len = pci_get_byte(pci_dev->config + pos + PCI_CAP_FLAGS);
> /* Direct R/W passthrough */
> - ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VNDR, pos, len,
> + ret = pci_add_capability(pci_dev, PCI_CAP_ID_VNDR, pos, len,
> &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 4599169..989fca5 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -130,7 +130,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
> pci_register_bar(dev, ICH9_MEM_BAR, PCI_BASE_ADDRESS_SPACE_MEMORY,
> &d->ahci.mem);
>
> - sata_cap_offset = pci_add_capability2(dev, PCI_CAP_ID_SATA,
> + sata_cap_offset = pci_add_capability(dev, PCI_CAP_ID_SATA,
> ICH9_SATA_CAP_OFFSET, SATA_CAP_SIZE,
> errp);
> if (sata_cap_offset < 0) {
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index a87b227..5e05ce5 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -216,7 +216,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
> }
>
> cap_size = msi_cap_sizeof(flags);
> - config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
> + config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset,
> cap_size, errp);
> if (config_offset < 0) {
> return config_offset;
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index bb54e8b..d634326 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -294,7 +294,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> return -EINVAL;
> }
>
> - cap = pci_add_capability2(dev, PCI_CAP_ID_MSIX,
> + cap = pci_add_capability(dev, PCI_CAP_ID_MSIX,
> cap_pos, MSIX_CAP_LENGTH, errp);
> if (cap < 0) {
> return cap;
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2bba37a..283ca5e 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2259,28 +2259,12 @@ static void pci_del_option_rom(PCIDevice *pdev)
> }
>
> /*
> - * if offset = 0,
> - * Find and reserve space and add capability to the linked list
> - * in pci config space
> - */
> -int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size,
> - Error **errp)
> -{
> - int ret;
> -
> - ret = pci_add_capability2(pdev, cap_id, offset, size, errp);
> -
> - return ret;
> -}
> -
> -/*
> - * On success, pci_add_capability2() returns a positive value
> + * On success, pci_add_capability() returns a positive value
> * that the offset of the pci capability.
> * On failure, it sets an error and returns a negative error
> * code.
> */
> -int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
> +int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> uint8_t offset, uint8_t size,
> Error **errp)
> {
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 70bfb59..8de8272 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1837,14 +1837,14 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
> case PCI_CAP_ID_PM:
> vfio_check_pm_reset(vdev, pos);
> vdev->pm_cap = pos;
> - ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
> + ret = pci_add_capability(pdev, cap_id, pos, size, errp);
> break;
> case PCI_CAP_ID_AF:
> vfio_check_af_flr(vdev, pos);
> - ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
> + ret = pci_add_capability(pdev, cap_id, pos, size, errp);
> break;
> default:
> - ret = pci_add_capability2(pdev, cap_id, pos, size, errp);
> + ret = pci_add_capability(pdev, cap_id, pos, size, errp);
> break;
> }
> out:
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index fe52aa8..e598b09 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -358,9 +358,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
> int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> uint8_t offset, uint8_t size,
> Error **errp);
> -int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size,
> - Error **errp);
>
> void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
>
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Thanks,
Marcel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 6/9] pci: Convert to realize
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 6/9] pci: Convert to realize Mao Zhongyi
@ 2017-06-19 11:42 ` Marcel Apfelbaum
2017-06-20 2:41 ` Mao Zhongyi
0 siblings, 1 reply; 21+ messages in thread
From: Marcel Apfelbaum @ 2017-06-19 11:42 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel; +Cc: mst, armbru
On 12/06/2017 16:48, Mao Zhongyi wrote:
> The pci-birdge device i82801b11
It is a dmi-to-pci brigde
> and io3130_upstream/downstream
You forgot to mention the pcie_root_port.
> still implements the old PCIDeviceClass .init() through *_init()
> instead of the new .realize(). All devices need to be converted
> to .realize(). So convert it and rename it to *_realize().
>
I would change the message to something more concise like:
"Convert i82801b11, xio3130_downstream, xio3130_upstream
and pcie_root_port devices to realize."
I am sure it worth a re-spin though.
> Cc: mst@redhat.com
> Cc: marcel@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/pci-bridge/i82801b11.c | 11 +++++------
> hw/pci-bridge/pcie_root_port.c | 15 ++++++---------
> hw/pci-bridge/xio3130_downstream.c | 20 +++++++++-----------
> hw/pci-bridge/xio3130_upstream.c | 20 +++++++++-----------
> hw/pci/pci_bridge.c | 7 +++----
> hw/pci/pcie.c | 11 ++++++-----
> include/hw/pci/pci_bridge.h | 3 ++-
> include/hw/pci/pcie.h | 3 ++-
> 8 files changed, 42 insertions(+), 48 deletions(-)
>
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index 2c065c3..2c1b747 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -59,24 +59,23 @@ typedef struct I82801b11Bridge {
> /*< public >*/
> } I82801b11Bridge;
>
> -static int i82801b11_bridge_initfn(PCIDevice *d)
> +static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
> {
> int rc;
>
> pci_bridge_initfn(d, TYPE_PCI_BUS);
>
> rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
> - I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
> + I82801ba_SSVID_SVID, I82801ba_SSVID_SSID,
> + errp);
> if (rc < 0) {
> goto err_bridge;
> }
> pci_config_set_prog_interface(d->config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
> - return 0;
> + return;
>
> err_bridge:
> pci_bridge_exitfn(d);
> -
> - return rc;
> }
>
> static const VMStateDescription i82801b11_bridge_dev_vmstate = {
> @@ -96,7 +95,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
> k->vendor_id = PCI_VENDOR_ID_INTEL;
> k->device_id = PCI_DEVICE_ID_INTEL_82801BA_11;
> k->revision = ICH9_D2P_A2_REVISION;
> - k->init = i82801b11_bridge_initfn;
> + k->realize = i82801b11_bridge_realize;
> k->config_write = pci_bridge_write_config;
> dc->vmsd = &i82801b11_bridge_dev_vmstate;
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index cf36318..00f0b1f 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -59,29 +59,27 @@ static void rp_realize(PCIDevice *d, Error **errp)
> PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
> PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
> int rc;
> - Error *local_err = NULL;
>
> pci_config_set_interrupt_pin(d->config, 1);
> pci_bridge_initfn(d, TYPE_PCIE_BUS);
> pcie_port_init_reg(d);
>
> - rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, rpc->ssid);
> + rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
> + rpc->ssid, errp);
> if (rc < 0) {
> - error_setg(errp, "Can't init SSV ID, error %d", rc);
I suggest using 'error_append_hint' instead of removing
the message; even if it does not add a lot of information,
maybe someone is expecting it to be in the logs.
> goto err_bridge;
> }
>
> if (rpc->interrupts_init) {
> - rc = rpc->interrupts_init(d, &local_err);
> + rc = rpc->interrupts_init(d, errp);
> if (rc < 0) {
> - error_propagate(errp, local_err);
> goto err_bridge;
> }
> }
>
> - rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT, p->port);
> + rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
> + p->port, errp);
> if (rc < 0) {
> - error_setg(errp, "Can't add Root Port capability, error %d", rc);
> goto err_int;
> }
>
> @@ -98,9 +96,8 @@ static void rp_realize(PCIDevice *d, Error **errp)
> }
>
> rc = pcie_aer_init(d, PCI_ERR_VER, rpc->aer_offset,
> - PCI_ERR_SIZEOF, &local_err);
> + PCI_ERR_SIZEOF, errp);
> if (rc < 0) {
> - error_propagate(errp, local_err);
> goto err;
> }
> pcie_aer_root_init(d);
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index cfe8a36..e706f36 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
> pci_bridge_reset(qdev);
> }
>
> -static int xio3130_downstream_initfn(PCIDevice *d)
> +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
> {
> 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, &err);
> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
> + errp);
> if (rc < 0) {
> assert(rc == -ENOTSUP);
> - error_report_err(err);
> goto err_bridge;
> }
>
> rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
> - XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
> + XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
> + errp);
> if (rc < 0) {
> goto err_bridge;
> }
>
> rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
> - p->port);
> + p->port, errp);
> if (rc < 0) {
> goto err_msi;
> }
> @@ -98,13 +98,12 @@ static int xio3130_downstream_initfn(PCIDevice *d)
> }
>
> rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> - PCI_ERR_SIZEOF, &err);
> + PCI_ERR_SIZEOF, errp);
> if (rc < 0) {
> - error_report_err(err);
> goto err;
> }
>
> - return 0;
> + return;
>
> err:
> pcie_chassis_del_slot(s);
> @@ -114,7 +113,6 @@ err_msi:
> msi_uninit(d);
> err_bridge:
> pci_bridge_exitfn(d);
> - return rc;
> }
>
> static void xio3130_downstream_exitfn(PCIDevice *d)
> @@ -181,7 +179,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
> k->is_express = 1;
> k->is_bridge = 1;
> k->config_write = xio3130_downstream_write_config;
> - k->init = xio3130_downstream_initfn;
> + k->realize = xio3130_downstream_realize;
> k->exit = xio3130_downstream_exitfn;
> k->vendor_id = PCI_VENDOR_ID_TI;
> k->device_id = PCI_DEVICE_ID_TI_XIO3130D;
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index 401c784..a052224 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -53,32 +53,32 @@ static void xio3130_upstream_reset(DeviceState *qdev)
> pcie_cap_deverr_reset(d);
> }
>
> -static int xio3130_upstream_initfn(PCIDevice *d)
> +static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
> {
> 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, &err);
> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
> + errp);
> if (rc < 0) {
> assert(rc == -ENOTSUP);
> - error_report_err(err);
> goto err_bridge;
> }
>
> rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
> - XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
> + XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
> + errp);
> if (rc < 0) {
> goto err_bridge;
> }
>
> rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
> - p->port);
> + p->port, errp);
> if (rc < 0) {
> goto err_msi;
> }
> @@ -86,13 +86,12 @@ static int xio3130_upstream_initfn(PCIDevice *d)
> pcie_cap_deverr_init(d);
>
> rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
> - PCI_ERR_SIZEOF, &err);
> + PCI_ERR_SIZEOF, errp);
> if (rc < 0) {
> - error_report_err(err);
> goto err;
> }
>
> - return 0;
> + return;
>
> err:
> pcie_cap_exit(d);
> @@ -100,7 +99,6 @@ err_msi:
> msi_uninit(d);
> err_bridge:
> pci_bridge_exitfn(d);
> - return rc;
> }
>
> static void xio3130_upstream_exitfn(PCIDevice *d)
> @@ -153,7 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass, void *data)
> k->is_express = 1;
> k->is_bridge = 1;
> k->config_write = xio3130_upstream_write_config;
> - k->init = xio3130_upstream_initfn;
> + k->realize = xio3130_upstream_realize;
> k->exit = xio3130_upstream_exitfn;
> k->vendor_id = PCI_VENDOR_ID_TI;
> k->device_id = PCI_DEVICE_ID_TI_XIO3130U;
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index bb0f3a3..720119b 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -41,15 +41,14 @@
> #define PCI_SSVID_SSID 6
>
> int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
> - uint16_t svid, uint16_t ssid)
> + uint16_t svid, uint16_t ssid,
> + Error **errp)
> {
> int pos;
> - Error *local_err = NULL;
>
> pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
> - PCI_SSVID_SIZEOF, &local_err);
> + PCI_SSVID_SIZEOF, errp);
> if (pos < 0) {
> - error_report_err(local_err);
> return pos;
> }
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index f187512..05d091a 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -86,19 +86,19 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
> pci_set_word(cmask + PCI_EXP_LNKSTA, 0);
> }
>
> -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
> +int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> + uint8_t type, uint8_t port,
> + Error **errp)
> {
> /* PCIe cap v2 init */
> int pos;
> uint8_t *exp_cap;
> - Error *local_err = NULL;
>
> assert(pci_is_express(dev));
>
> pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> - PCI_EXP_VER2_SIZEOF, &local_err);
> + PCI_EXP_VER2_SIZEOF, errp);
> if (pos < 0) {
> - error_report_err(local_err);
> return pos;
> }
> dev->exp.exp_cap = pos;
> @@ -147,6 +147,7 @@ static int
> pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
> {
> uint8_t type = PCI_EXP_TYPE_ENDPOINT;
> + Error *local_err = NULL;
>
> /*
> * Windows guests will report Code 10, device cannot start, if
> @@ -159,7 +160,7 @@ pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
>
> return (cap_size == PCI_EXP_VER1_SIZEOF)
> ? pcie_cap_v1_init(dev, offset, type, 0)
> - : pcie_cap_init(dev, offset, type, 0);
> + : pcie_cap_init(dev, offset, type, 0, &local_err);
You have some extra info on local_err,
I think you should print an err message or propagate it with an
extra parameter.
> }
>
> int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index d5891cd..ff7cbaa 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -33,7 +33,8 @@
> #define PCI_BRIDGE_DEV_PROP_SHPC "shpc"
>
> int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
> - uint16_t svid, uint16_t ssid);
> + uint16_t svid, uint16_t ssid,
> + Error **errp);
>
> PCIDevice *pci_bridge_get_device(PCIBus *bus);
> PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 3d8f24b..b71e369 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -84,7 +84,8 @@ struct PCIExpressDevice {
> #define COMPAT_PROP_PCP "power_controller_present"
>
> /* PCI express capability helper functions */
> -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
> +int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
> + uint8_t port, Error **errp);
> int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
> uint8_t type, uint8_t port);
> int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
>
Thanks for converting the devices to realize!
Other than some minor comments, the patch looks reaaly good.
Thanks,
Marcel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error Mao Zhongyi
@ 2017-06-19 12:05 ` Marcel Apfelbaum
2017-06-20 3:51 ` Mao Zhongyi
0 siblings, 1 reply; 21+ messages in thread
From: Marcel Apfelbaum @ 2017-06-19 12:05 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel; +Cc: mst, armbru
On 12/06/2017 16:48, Mao Zhongyi wrote:
> In order to propagate error message better, convert shpc_init() to
> Error also convert the pci_bridge_dev_initfn() to realize.
>
> Cc: mst@redhat.com
> Cc: marcel@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
> hw/pci/shpc.c | 11 +++++------
> hw/pci/slotid_cap.c | 11 +++++------
> include/hw/pci/shpc.h | 3 ++-
> include/hw/pci/slotid_cap.h | 3 ++-
> 5 files changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 5dbd933..30c4186 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -49,12 +49,11 @@ struct PCIBridgeDev {
> };
> typedef struct PCIBridgeDev PCIBridgeDev;
>
> -static int pci_bridge_dev_initfn(PCIDevice *dev)
> +static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
> {
> 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);
>
> @@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> dev->config[PCI_INTERRUPT_PIN] = 0x1;
> memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
> shpc_bar_size(dev));
> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
> + err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
> if (err) {
> goto shpc_error;
> }
> @@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> bridge_dev->msi = ON_OFF_AUTO_OFF;
> }
>
> - err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
> + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
> if (err) {
> goto slotid_error;
> }
> @@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
> /* it means SHPC exists, because MSI is needed by SHPC */
>
> - err = msi_init(dev, 0, 1, true, true, &local_err);
> + err = msi_init(dev, 0, 1, true, true, errp);
> /* 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) "
> + error_append_hint(errp, "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);
> + assert(bridge_dev->msi == ON_OFF_AUTO_AUTO);
I am not sure we can drop the !local_err assert.
We don't have a local_err anymore, but the error
is stored now in errp.
Other than that, the patch looks OK to me.
Thanks,
Marcel
> /* With msi=auto, we fall back to MSI off silently */
> - error_free(local_err);
> }
>
> if (shpc_present(dev)) {
> @@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
> PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
> }
> - return 0;
> + return;
>
> msi_error:
> slotid_cap_cleanup(dev);
> @@ -111,8 +108,6 @@ slotid_error:
> }
> shpc_error:
> pci_bridge_exitfn(dev);
> -
> - return err;
> }
>
> static void pci_bridge_dev_exitfn(PCIDevice *dev)
> @@ -216,7 +211,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>
> - k->init = pci_bridge_dev_initfn;
> + k->realize = pci_bridge_dev_realize;
> k->exit = pci_bridge_dev_exitfn;
> k->config_write = pci_bridge_dev_write_config;
> k->vendor_id = PCI_VENDOR_ID_REDHAT;
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index d72d5e4..69fc14b 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
> }
>
> /* Add SHPC capability to the config space for the device. */
> -static int shpc_cap_add_config(PCIDevice *d)
> +static int shpc_cap_add_config(PCIDevice *d, Error **errp)
> {
> uint8_t *config;
> int config_offset;
> - Error *local_err = NULL;
> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
> 0, SHPC_CAP_LENGTH,
> - &local_err);
> + errp);
> if (config_offset < 0) {
> - error_report_err(local_err);
> return config_offset;
> }
> config = d->config + config_offset;
> @@ -584,13 +582,14 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> }
>
> /* Initialize the SHPC structure in bridge's BAR. */
> -int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
> +int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
> + unsigned offset, Error **errp)
> {
> int i, ret;
> int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
> SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
> shpc->sec_bus = sec_bus;
> - ret = shpc_cap_add_config(d);
> + ret = shpc_cap_add_config(d, errp);
> if (ret) {
> g_free(d->shpc);
> return ret;
> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
> index bdca205..36d021b 100644
> --- a/hw/pci/slotid_cap.c
> +++ b/hw/pci/slotid_cap.c
> @@ -9,14 +9,14 @@
>
> int slotid_cap_init(PCIDevice *d, int nslots,
> uint8_t chassis,
> - unsigned offset)
> + unsigned offset,
> + Error **errp)
> {
> int cap;
> - Error *local_err = NULL;
>
> if (!chassis) {
> - error_report("Bridge chassis not specified. Each bridge is required "
> - "to be assigned a unique chassis id > 0.");
> + error_setg(errp, "Bridge chassis not specified. Each bridge is required"
> + " to be assigned a unique chassis id > 0.");
> return -EINVAL;
> }
> if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >> SLOTID_NSLOTS_SHIFT)) {
> @@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
> }
>
> cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
> - SLOTID_CAP_LENGTH, &local_err);
> + SLOTID_CAP_LENGTH, errp);
> if (cap < 0) {
> - error_report_err(local_err);
> return cap;
> }
> /* We make each chassis unique, this way each bridge is First in Chassis */
> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
> index 71e836b..ee19fec 100644
> --- a/include/hw/pci/shpc.h
> +++ b/include/hw/pci/shpc.h
> @@ -38,7 +38,8 @@ struct SHPCDevice {
>
> void shpc_reset(PCIDevice *d);
> int shpc_bar_size(PCIDevice *dev);
> -int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
> +int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
> + unsigned off, Error **errp);
> void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
> void shpc_free(PCIDevice *dev);
> void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
> diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
> index 70db047..a777ea0 100644
> --- a/include/hw/pci/slotid_cap.h
> +++ b/include/hw/pci/slotid_cap.h
> @@ -5,7 +5,8 @@
>
> int slotid_cap_init(PCIDevice *dev, int nslots,
> uint8_t chassis,
> - unsigned offset);
> + unsigned offset,
> + Error **errp);
> void slotid_cap_cleanup(PCIDevice *dev);
>
> #endif
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 6/9] pci: Convert to realize
2017-06-19 11:42 ` Marcel Apfelbaum
@ 2017-06-20 2:41 ` Mao Zhongyi
0 siblings, 0 replies; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-20 2:41 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: mst, armbru
Hi, Marcel
On 06/19/2017 07:42 PM, Marcel Apfelbaum wrote:
> On 12/06/2017 16:48, Mao Zhongyi wrote:
>> The pci-birdge device i82801b11
>
> It is a dmi-to-pci brigde
>
>> and io3130_upstream/downstream
>
> You forgot to mention the pcie_root_port.
>
>> still implements the old PCIDeviceClass .init() through *_init()
>> instead of the new .realize(). All devices need to be converted
>> to .realize(). So convert it and rename it to *_realize().
>>
>
>
> I would change the message to something more concise like:
>
> "Convert i82801b11, xio3130_downstream, xio3130_upstream
> and pcie_root_port devices to realize."
>
> I am sure it worth a re-spin though.
Thanks for your kind suggestion, I will. :)
>
>> Cc: mst@redhat.com
>> Cc: marcel@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> hw/pci-bridge/i82801b11.c | 11 +++++------
>> hw/pci-bridge/pcie_root_port.c | 15 ++++++---------
>> hw/pci-bridge/xio3130_downstream.c | 20 +++++++++-----------
>> hw/pci-bridge/xio3130_upstream.c | 20 +++++++++-----------
>> hw/pci/pci_bridge.c | 7 +++----
>> hw/pci/pcie.c | 11 ++++++-----
>> include/hw/pci/pci_bridge.h | 3 ++-
>> include/hw/pci/pcie.h | 3 ++-
>> 8 files changed, 42 insertions(+), 48 deletions(-)
>>
>> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>> index 2c065c3..2c1b747 100644
>> --- a/hw/pci-bridge/i82801b11.c
>> +++ b/hw/pci-bridge/i82801b11.c
>> @@ -59,24 +59,23 @@ typedef struct I82801b11Bridge {
>> /*< public >*/
>> } I82801b11Bridge;
>> -static int i82801b11_bridge_initfn(PCIDevice *d)
>> +static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
>> {
>> int rc;
>> pci_bridge_initfn(d, TYPE_PCI_BUS);
>> rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
>> - I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
>> + I82801ba_SSVID_SVID, I82801ba_SSVID_SSID,
>> + errp);
>> if (rc < 0) {
>> goto err_bridge;
>> }
>> pci_config_set_prog_interface(d->config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
>> - return 0;
>> + return;
>> err_bridge:
>> pci_bridge_exitfn(d);
>> -
>> - return rc;
>> }
>> static const VMStateDescription i82801b11_bridge_dev_vmstate = {
>> @@ -96,7 +95,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
>> k->vendor_id = PCI_VENDOR_ID_INTEL;
>> k->device_id = PCI_DEVICE_ID_INTEL_82801BA_11;
>> k->revision = ICH9_D2P_A2_REVISION;
>> - k->init = i82801b11_bridge_initfn;
>> + k->realize = i82801b11_bridge_realize;
>> k->config_write = pci_bridge_write_config;
>> dc->vmsd = &i82801b11_bridge_dev_vmstate;
>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
>> index cf36318..00f0b1f 100644
>> --- a/hw/pci-bridge/pcie_root_port.c
>> +++ b/hw/pci-bridge/pcie_root_port.c
>> @@ -59,29 +59,27 @@ static void rp_realize(PCIDevice *d, Error **errp)
>> PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
>> PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
>> int rc;
>> - Error *local_err = NULL;
>> pci_config_set_interrupt_pin(d->config, 1);
>> pci_bridge_initfn(d, TYPE_PCIE_BUS);
>> pcie_port_init_reg(d);
>> - rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, rpc->ssid);
>> + rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
>> + rpc->ssid, errp);
>> if (rc < 0) {
>> - error_setg(errp, "Can't init SSV ID, error %d", rc);
>
>
> I suggest using 'error_append_hint' instead of removing
> the message; even if it does not add a lot of information,
> maybe someone is expecting it to be in the logs.
OK, I see.
>
>> goto err_bridge;
>> }
>> if (rpc->interrupts_init) {
>> - rc = rpc->interrupts_init(d, &local_err);
>> + rc = rpc->interrupts_init(d, errp);
>> if (rc < 0) {
>> - error_propagate(errp, local_err);
>> goto err_bridge;
>> }
>> }
>> - rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT, p->port);
>> + rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
>> + p->port, errp);
>> if (rc < 0) {
>> - error_setg(errp, "Can't add Root Port capability, error %d", rc);
>> goto err_int;
>> }
>> @@ -98,9 +96,8 @@ static void rp_realize(PCIDevice *d, Error **errp)
>> }
>> rc = pcie_aer_init(d, PCI_ERR_VER, rpc->aer_offset,
>> - PCI_ERR_SIZEOF, &local_err);
>> + PCI_ERR_SIZEOF, errp);
>> if (rc < 0) {
>> - error_propagate(errp, local_err);
>> goto err;
>> }
>> pcie_aer_root_init(d);
>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>> index cfe8a36..e706f36 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -56,33 +56,33 @@ static void xio3130_downstream_reset(DeviceState *qdev)
>> pci_bridge_reset(qdev);
>> }
>> -static int xio3130_downstream_initfn(PCIDevice *d)
>> +static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
>> {
>> 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, &err);
>> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
>> + errp);
>> if (rc < 0) {
>> assert(rc == -ENOTSUP);
>> - error_report_err(err);
>> goto err_bridge;
>> }
>> rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
>> - XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
>> + XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
>> + errp);
>> if (rc < 0) {
>> goto err_bridge;
>> }
>> rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
>> - p->port);
>> + p->port, errp);
>> if (rc < 0) {
>> goto err_msi;
>> }
>> @@ -98,13 +98,12 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>> }
>> rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
>> - PCI_ERR_SIZEOF, &err);
>> + PCI_ERR_SIZEOF, errp);
>> if (rc < 0) {
>> - error_report_err(err);
>> goto err;
>> }
>> - return 0;
>> + return;
>> err:
>> pcie_chassis_del_slot(s);
>> @@ -114,7 +113,6 @@ err_msi:
>> msi_uninit(d);
>> err_bridge:
>> pci_bridge_exitfn(d);
>> - return rc;
>> }
>> static void xio3130_downstream_exitfn(PCIDevice *d)
>> @@ -181,7 +179,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
>> k->is_express = 1;
>> k->is_bridge = 1;
>> k->config_write = xio3130_downstream_write_config;
>> - k->init = xio3130_downstream_initfn;
>> + k->realize = xio3130_downstream_realize;
>> k->exit = xio3130_downstream_exitfn;
>> k->vendor_id = PCI_VENDOR_ID_TI;
>> k->device_id = PCI_DEVICE_ID_TI_XIO3130D;
>> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
>> index 401c784..a052224 100644
>> --- a/hw/pci-bridge/xio3130_upstream.c
>> +++ b/hw/pci-bridge/xio3130_upstream.c
>> @@ -53,32 +53,32 @@ static void xio3130_upstream_reset(DeviceState *qdev)
>> pcie_cap_deverr_reset(d);
>> }
>> -static int xio3130_upstream_initfn(PCIDevice *d)
>> +static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
>> {
>> 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, &err);
>> + XIO3130_MSI_SUPPORTED_FLAGS & PCI_MSI_FLAGS_MASKBIT,
>> + errp);
>> if (rc < 0) {
>> assert(rc == -ENOTSUP);
>> - error_report_err(err);
>> goto err_bridge;
>> }
>> rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
>> - XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
>> + XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
>> + errp);
>> if (rc < 0) {
>> goto err_bridge;
>> }
>> rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
>> - p->port);
>> + p->port, errp);
>> if (rc < 0) {
>> goto err_msi;
>> }
>> @@ -86,13 +86,12 @@ static int xio3130_upstream_initfn(PCIDevice *d)
>> pcie_cap_deverr_init(d);
>> rc = pcie_aer_init(d, PCI_ERR_VER, XIO3130_AER_OFFSET,
>> - PCI_ERR_SIZEOF, &err);
>> + PCI_ERR_SIZEOF, errp);
>> if (rc < 0) {
>> - error_report_err(err);
>> goto err;
>> }
>> - return 0;
>> + return;
>> err:
>> pcie_cap_exit(d);
>> @@ -100,7 +99,6 @@ err_msi:
>> msi_uninit(d);
>> err_bridge:
>> pci_bridge_exitfn(d);
>> - return rc;
>> }
>> static void xio3130_upstream_exitfn(PCIDevice *d)
>> @@ -153,7 +151,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass, void *data)
>> k->is_express = 1;
>> k->is_bridge = 1;
>> k->config_write = xio3130_upstream_write_config;
>> - k->init = xio3130_upstream_initfn;
>> + k->realize = xio3130_upstream_realize;
>> k->exit = xio3130_upstream_exitfn;
>> k->vendor_id = PCI_VENDOR_ID_TI;
>> k->device_id = PCI_DEVICE_ID_TI_XIO3130U;
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index bb0f3a3..720119b 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -41,15 +41,14 @@
>> #define PCI_SSVID_SSID 6
>> int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
>> - uint16_t svid, uint16_t ssid)
>> + uint16_t svid, uint16_t ssid,
>> + Error **errp)
>> {
>> int pos;
>> - Error *local_err = NULL;
>> pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset,
>> - PCI_SSVID_SIZEOF, &local_err);
>> + PCI_SSVID_SIZEOF, errp);
>> if (pos < 0) {
>> - error_report_err(local_err);
>> return pos;
>> }
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index f187512..05d091a 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -86,19 +86,19 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
>> pci_set_word(cmask + PCI_EXP_LNKSTA, 0);
>> }
>> -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
>> +int pcie_cap_init(PCIDevice *dev, uint8_t offset,
>> + uint8_t type, uint8_t port,
>> + Error **errp)
>> {
>> /* PCIe cap v2 init */
>> int pos;
>> uint8_t *exp_cap;
>> - Error *local_err = NULL;
>> assert(pci_is_express(dev));
>> pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
>> - PCI_EXP_VER2_SIZEOF, &local_err);
>> + PCI_EXP_VER2_SIZEOF, errp);
>> if (pos < 0) {
>> - error_report_err(local_err);
>> return pos;
>> }
>> dev->exp.exp_cap = pos;
>> @@ -147,6 +147,7 @@ static int
>> pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
>> {
>> uint8_t type = PCI_EXP_TYPE_ENDPOINT;
>> + Error *local_err = NULL;
>> /*
>> * Windows guests will report Code 10, device cannot start, if
>> @@ -159,7 +160,7 @@ pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
>> return (cap_size == PCI_EXP_VER1_SIZEOF)
>> ? pcie_cap_v1_init(dev, offset, type, 0)
>> - : pcie_cap_init(dev, offset, type, 0);
>> + : pcie_cap_init(dev, offset, type, 0, &local_err);
>
> You have some extra info on local_err,
> I think you should print an err message or propagate it with an
> extra parameter.
Yes, it's true to report the error message from local_err, will
fix it in the next.
>
>> }
>> int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
>> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
>> index d5891cd..ff7cbaa 100644
>> --- a/include/hw/pci/pci_bridge.h
>> +++ b/include/hw/pci/pci_bridge.h
>> @@ -33,7 +33,8 @@
>> #define PCI_BRIDGE_DEV_PROP_SHPC "shpc"
>> int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
>> - uint16_t svid, uint16_t ssid);
>> + uint16_t svid, uint16_t ssid,
>> + Error **errp);
>> PCIDevice *pci_bridge_get_device(PCIBus *bus);
>> PCIBus *pci_bridge_get_sec_bus(PCIBridge *br);
>> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
>> index 3d8f24b..b71e369 100644
>> --- a/include/hw/pci/pcie.h
>> +++ b/include/hw/pci/pcie.h
>> @@ -84,7 +84,8 @@ struct PCIExpressDevice {
>> #define COMPAT_PROP_PCP "power_controller_present"
>> /* PCI express capability helper functions */
>> -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
>> +int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
>> + uint8_t port, Error **errp);
>> int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
>> uint8_t type, uint8_t port);
>> int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
>>
>
>
> Thanks for converting the devices to realize!
> Other than some minor comments, the patch looks reaaly good.
Thanks :)
Mao
> Thanks,
> Marcel
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error
2017-06-19 12:05 ` Marcel Apfelbaum
@ 2017-06-20 3:51 ` Mao Zhongyi
2017-06-21 7:39 ` Marcel Apfelbaum
0 siblings, 1 reply; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-20 3:51 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: mst, armbru
Hi, Marcel
On 06/19/2017 08:05 PM, Marcel Apfelbaum wrote:
> On 12/06/2017 16:48, Mao Zhongyi wrote:
>> In order to propagate error message better, convert shpc_init() to
>> Error also convert the pci_bridge_dev_initfn() to realize.
>>
>> Cc: mst@redhat.com
>> Cc: marcel@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
>> hw/pci/shpc.c | 11 +++++------
>> hw/pci/slotid_cap.c | 11 +++++------
>> include/hw/pci/shpc.h | 3 ++-
>> include/hw/pci/slotid_cap.h | 3 ++-
>> 5 files changed, 22 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>> index 5dbd933..30c4186 100644
>> --- a/hw/pci-bridge/pci_bridge_dev.c
>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>> @@ -49,12 +49,11 @@ struct PCIBridgeDev {
>> };
>> typedef struct PCIBridgeDev PCIBridgeDev;
>> -static int pci_bridge_dev_initfn(PCIDevice *dev)
>> +static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>> {
>> 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);
>> @@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> dev->config[PCI_INTERRUPT_PIN] = 0x1;
>> memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>> shpc_bar_size(dev));
>> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>> + err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
>> if (err) {
>> goto shpc_error;
>> }
>> @@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> bridge_dev->msi = ON_OFF_AUTO_OFF;
>> }
>> - err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>> + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
>> if (err) {
>> goto slotid_error;
>> }
>> @@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>> /* it means SHPC exists, because MSI is needed by SHPC */
>> - err = msi_init(dev, 0, 1, true, true, &local_err);
>> + err = msi_init(dev, 0, 1, true, true, errp);
>> /* 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) "
>> + error_append_hint(errp, "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);
>> + assert(bridge_dev->msi == ON_OFF_AUTO_AUTO);
>
> I am not sure we can drop the !local_err assert.
> We don't have a local_err anymore, but the error
> is stored now in errp.
>
I think it's OK if we drop the !local_err assert; even if we don't store the error
message to errp.
Because the code can go to assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO)
only if the return value of msi_init is 0(success), then the local_err is NULL, no
error message in it. So the assertation of local_err is always superfluous.
Thanks
Mao
> Other than that, the patch looks OK to me.
>
> Thanks,
> Marcel
>
>> /* With msi=auto, we fall back to MSI off silently */
>> - error_free(local_err);
>> }
>> if (shpc_present(dev)) {
>> @@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
>> PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
>> }
>> - return 0;
>> + return;
>> msi_error:
>> slotid_cap_cleanup(dev);
>> @@ -111,8 +108,6 @@ slotid_error:
>> }
>> shpc_error:
>> pci_bridge_exitfn(dev);
>> -
>> - return err;
>> }
>> static void pci_bridge_dev_exitfn(PCIDevice *dev)
>> @@ -216,7 +211,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>> - k->init = pci_bridge_dev_initfn;
>> + k->realize = pci_bridge_dev_realize;
>> k->exit = pci_bridge_dev_exitfn;
>> k->config_write = pci_bridge_dev_write_config;
>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
>> index d72d5e4..69fc14b 100644
>> --- a/hw/pci/shpc.c
>> +++ b/hw/pci/shpc.c
>> @@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
>> }
>> /* Add SHPC capability to the config space for the device. */
>> -static int shpc_cap_add_config(PCIDevice *d)
>> +static int shpc_cap_add_config(PCIDevice *d, Error **errp)
>> {
>> uint8_t *config;
>> int config_offset;
>> - Error *local_err = NULL;
>> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
>> 0, SHPC_CAP_LENGTH,
>> - &local_err);
>> + errp);
>> if (config_offset < 0) {
>> - error_report_err(local_err);
>> return config_offset;
>> }
>> config = d->config + config_offset;
>> @@ -584,13 +582,14 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>> }
>> /* Initialize the SHPC structure in bridge's BAR. */
>> -int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
>> +int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
>> + unsigned offset, Error **errp)
>> {
>> int i, ret;
>> int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
>> SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
>> shpc->sec_bus = sec_bus;
>> - ret = shpc_cap_add_config(d);
>> + ret = shpc_cap_add_config(d, errp);
>> if (ret) {
>> g_free(d->shpc);
>> return ret;
>> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
>> index bdca205..36d021b 100644
>> --- a/hw/pci/slotid_cap.c
>> +++ b/hw/pci/slotid_cap.c
>> @@ -9,14 +9,14 @@
>> int slotid_cap_init(PCIDevice *d, int nslots,
>> uint8_t chassis,
>> - unsigned offset)
>> + unsigned offset,
>> + Error **errp)
>> {
>> int cap;
>> - Error *local_err = NULL;
>> if (!chassis) {
>> - error_report("Bridge chassis not specified. Each bridge is required "
>> - "to be assigned a unique chassis id > 0.");
>> + error_setg(errp, "Bridge chassis not specified. Each bridge is required"
>> + " to be assigned a unique chassis id > 0.");
>> return -EINVAL;
>> }
>> if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >> SLOTID_NSLOTS_SHIFT)) {
>> @@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
>> }
>> cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
>> - SLOTID_CAP_LENGTH, &local_err);
>> + SLOTID_CAP_LENGTH, errp);
>> if (cap < 0) {
>> - error_report_err(local_err);
>> return cap;
>> }
>> /* We make each chassis unique, this way each bridge is First in Chassis */
>> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
>> index 71e836b..ee19fec 100644
>> --- a/include/hw/pci/shpc.h
>> +++ b/include/hw/pci/shpc.h
>> @@ -38,7 +38,8 @@ struct SHPCDevice {
>> void shpc_reset(PCIDevice *d);
>> int shpc_bar_size(PCIDevice *dev);
>> -int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
>> +int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
>> + unsigned off, Error **errp);
>> void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
>> void shpc_free(PCIDevice *dev);
>> void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
>> diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
>> index 70db047..a777ea0 100644
>> --- a/include/hw/pci/slotid_cap.h
>> +++ b/include/hw/pci/slotid_cap.h
>> @@ -5,7 +5,8 @@
>> int slotid_cap_init(PCIDevice *dev, int nslots,
>> uint8_t chassis,
>> - unsigned offset);
>> + unsigned offset,
>> + Error **errp);
>> void slotid_cap_cleanup(PCIDevice *dev);
>> #endif
>>
>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error
2017-06-20 3:51 ` Mao Zhongyi
@ 2017-06-21 7:39 ` Marcel Apfelbaum
2017-06-22 3:47 ` Mao Zhongyi
0 siblings, 1 reply; 21+ messages in thread
From: Marcel Apfelbaum @ 2017-06-21 7:39 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel; +Cc: mst, armbru
On 20/06/2017 6:51, Mao Zhongyi wrote:
> Hi, Marcel
>
> On 06/19/2017 08:05 PM, Marcel Apfelbaum wrote:
>> On 12/06/2017 16:48, Mao Zhongyi wrote:
>>> In order to propagate error message better, convert shpc_init() to
>>> Error also convert the pci_bridge_dev_initfn() to realize.
>>>
>>> Cc: mst@redhat.com
>>> Cc: marcel@redhat.com
>>> Cc: armbru@redhat.com
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>> ---
>>> hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
>>> hw/pci/shpc.c | 11 +++++------
>>> hw/pci/slotid_cap.c | 11 +++++------
>>> include/hw/pci/shpc.h | 3 ++-
>>> include/hw/pci/slotid_cap.h | 3 ++-
>>> 5 files changed, 22 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>>> b/hw/pci-bridge/pci_bridge_dev.c
>>> index 5dbd933..30c4186 100644
>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>> @@ -49,12 +49,11 @@ struct PCIBridgeDev {
>>> };
>>> typedef struct PCIBridgeDev PCIBridgeDev;
>>> -static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> +static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>> {
>>> 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);
>>> @@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>> memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>>> shpc_bar_size(dev));
>>> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>> + err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
>>> if (err) {
>>> goto shpc_error;
>>> }
>>> @@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> bridge_dev->msi = ON_OFF_AUTO_OFF;
>>> }
>>> - err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>>> + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
>>> if (err) {
>>> goto slotid_error;
>>> }
>>> @@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>>> /* it means SHPC exists, because MSI is needed by SHPC */
>>> - err = msi_init(dev, 0, 1, true, true, &local_err);
>>> + err = msi_init(dev, 0, 1, true, true, errp);
>>> /* 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) "
>>> + error_append_hint(errp, "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);
>>> + assert(bridge_dev->msi == ON_OFF_AUTO_AUTO);
>>
>> I am not sure we can drop the !local_err assert.
>> We don't have a local_err anymore, but the error
>> is stored now in errp.
>>
>
> I think it's OK if we drop the !local_err assert; even if we don't store
> the error
> message to errp.
>
> Because the code can go to assert(!local_err || bridge_dev->msi ==
> ON_OFF_AUTO_AUTO)
> only if the return value of msi_init is 0(success), then the local_err
> is NULL,
I think I am missing something.
What if msi_init returns with -ENOTSUP and
bridge_dev->msi == ON_OFF_AUTO_AUTO ?
We do reach the assert with an err.
Previously it was a asserted "error allowed only on auto mode",
now we assert only "auto mode".
Thanks,
Marcel
no
> error message in it. So the assertation of local_err is always superfluous.
>
> Thanks
> Mao
>
>
>> Other than that, the patch looks OK to me.
>>
>> Thanks,
>> Marcel
>>
>>> /* With msi=auto, we fall back to MSI off silently */
>>> - error_free(local_err);
>>> }
>>> if (shpc_present(dev)) {
>>> @@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
>>> PCI_BASE_ADDRESS_MEM_TYPE_64,
>>> &bridge_dev->bar);
>>> }
>>> - return 0;
>>> + return;
>>> msi_error:
>>> slotid_cap_cleanup(dev);
>>> @@ -111,8 +108,6 @@ slotid_error:
>>> }
>>> shpc_error:
>>> pci_bridge_exitfn(dev);
>>> -
>>> - return err;
>>> }
>>> static void pci_bridge_dev_exitfn(PCIDevice *dev)
>>> @@ -216,7 +211,7 @@ static void pci_bridge_dev_class_init(ObjectClass
>>> *klass, void *data)
>>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>> - k->init = pci_bridge_dev_initfn;
>>> + k->realize = pci_bridge_dev_realize;
>>> k->exit = pci_bridge_dev_exitfn;
>>> k->config_write = pci_bridge_dev_write_config;
>>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
>>> index d72d5e4..69fc14b 100644
>>> --- a/hw/pci/shpc.c
>>> +++ b/hw/pci/shpc.c
>>> @@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
>>> }
>>> /* Add SHPC capability to the config space for the device. */
>>> -static int shpc_cap_add_config(PCIDevice *d)
>>> +static int shpc_cap_add_config(PCIDevice *d, Error **errp)
>>> {
>>> uint8_t *config;
>>> int config_offset;
>>> - Error *local_err = NULL;
>>> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
>>> 0, SHPC_CAP_LENGTH,
>>> - &local_err);
>>> + errp);
>>> if (config_offset < 0) {
>>> - error_report_err(local_err);
>>> return config_offset;
>>> }
>>> config = d->config + config_offset;
>>> @@ -584,13 +582,14 @@ void
>>> shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>> }
>>> /* Initialize the SHPC structure in bridge's BAR. */
>>> -int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
>>> unsigned offset)
>>> +int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
>>> + unsigned offset, Error **errp)
>>> {
>>> int i, ret;
>>> int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
>>> SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
>>> shpc->sec_bus = sec_bus;
>>> - ret = shpc_cap_add_config(d);
>>> + ret = shpc_cap_add_config(d, errp);
>>> if (ret) {
>>> g_free(d->shpc);
>>> return ret;
>>> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
>>> index bdca205..36d021b 100644
>>> --- a/hw/pci/slotid_cap.c
>>> +++ b/hw/pci/slotid_cap.c
>>> @@ -9,14 +9,14 @@
>>> int slotid_cap_init(PCIDevice *d, int nslots,
>>> uint8_t chassis,
>>> - unsigned offset)
>>> + unsigned offset,
>>> + Error **errp)
>>> {
>>> int cap;
>>> - Error *local_err = NULL;
>>> if (!chassis) {
>>> - error_report("Bridge chassis not specified. Each bridge is
>>> required "
>>> - "to be assigned a unique chassis id > 0.");
>>> + error_setg(errp, "Bridge chassis not specified. Each bridge
>>> is required"
>>> + " to be assigned a unique chassis id > 0.");
>>> return -EINVAL;
>>> }
>>> if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >>
>>> SLOTID_NSLOTS_SHIFT)) {
>>> @@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
>>> }
>>> cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
>>> - SLOTID_CAP_LENGTH, &local_err);
>>> + SLOTID_CAP_LENGTH, errp);
>>> if (cap < 0) {
>>> - error_report_err(local_err);
>>> return cap;
>>> }
>>> /* We make each chassis unique, this way each bridge is First
>>> in Chassis */
>>> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
>>> index 71e836b..ee19fec 100644
>>> --- a/include/hw/pci/shpc.h
>>> +++ b/include/hw/pci/shpc.h
>>> @@ -38,7 +38,8 @@ struct SHPCDevice {
>>> void shpc_reset(PCIDevice *d);
>>> int shpc_bar_size(PCIDevice *dev);
>>> -int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
>>> unsigned off);
>>> +int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
>>> + unsigned off, Error **errp);
>>> void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
>>> void shpc_free(PCIDevice *dev);
>>> void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t
>>> val, int len);
>>> diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
>>> index 70db047..a777ea0 100644
>>> --- a/include/hw/pci/slotid_cap.h
>>> +++ b/include/hw/pci/slotid_cap.h
>>> @@ -5,7 +5,8 @@
>>> int slotid_cap_init(PCIDevice *dev, int nslots,
>>> uint8_t chassis,
>>> - unsigned offset);
>>> + unsigned offset,
>>> + Error **errp);
>>> void slotid_cap_cleanup(PCIDevice *dev);
>>> #endif
>>>
>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error
2017-06-21 7:39 ` Marcel Apfelbaum
@ 2017-06-22 3:47 ` Mao Zhongyi
2017-06-22 6:29 ` Marcel Apfelbaum
0 siblings, 1 reply; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-22 3:47 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: mst, armbru
On 06/21/2017 03:39 PM, Marcel Apfelbaum wrote:
> On 20/06/2017 6:51, Mao Zhongyi wrote:
>> Hi, Marcel
>>
>> On 06/19/2017 08:05 PM, Marcel Apfelbaum wrote:
>>> On 12/06/2017 16:48, Mao Zhongyi wrote:
>>>> In order to propagate error message better, convert shpc_init() to
>>>> Error also convert the pci_bridge_dev_initfn() to realize.
>>>>
>>>> Cc: mst@redhat.com
>>>> Cc: marcel@redhat.com
>>>> Cc: armbru@redhat.com
>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>> ---
>>>> hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
>>>> hw/pci/shpc.c | 11 +++++------
>>>> hw/pci/slotid_cap.c | 11 +++++------
>>>> include/hw/pci/shpc.h | 3 ++-
>>>> include/hw/pci/slotid_cap.h | 3 ++-
>>>> 5 files changed, 22 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>>> index 5dbd933..30c4186 100644
>>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>>> @@ -49,12 +49,11 @@ struct PCIBridgeDev {
>>>> };
>>>> typedef struct PCIBridgeDev PCIBridgeDev;
>>>> -static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> +static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>>> {
>>>> 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);
>>>> @@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>>> memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>>>> shpc_bar_size(dev));
>>>> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>>> + err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
>>>> if (err) {
>>>> goto shpc_error;
>>>> }
>>>> @@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> bridge_dev->msi = ON_OFF_AUTO_OFF;
>>>> }
>>>> - err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>>>> + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
>>>> if (err) {
>>>> goto slotid_error;
>>>> }
>>>> @@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>>>> /* it means SHPC exists, because MSI is needed by SHPC */
>>>> - err = msi_init(dev, 0, 1, true, true, &local_err);
>>>> + err = msi_init(dev, 0, 1, true, true, errp);
>>>> /* 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) "
>>>> + error_append_hint(errp, "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);
>>>> + assert(bridge_dev->msi == ON_OFF_AUTO_AUTO);
>>>
>>> I am not sure we can drop the !local_err assert.
>>> We don't have a local_err anymore, but the error
>>> is stored now in errp.
>>>
>>
>> I think it's OK if we drop the !local_err assert; even if we don't store the error
>> message to errp.
>>
>> Because the code can go to assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO)
>> only if the return value of msi_init is 0(success), then the local_err is NULL,
>
> I think I am missing something.
> What if msi_init returns with -ENOTSUP and
> bridge_dev->msi == ON_OFF_AUTO_AUTO ?
> We do reach the assert with an err.
> Previously it was a asserted "error allowed only on auto mode",
> now we assert only "auto mode".
Hi, Marcel
Thanks for your explanation, you are right, I am missing something.
We can do reach the assert in the next three cases:
------------------------------------------------------------------------------------------------------------------------------------------------------------------
| | | previous | now | |
| case | condition |-------------------------------------------------------------|---------------------------------------------| result |
| | | assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | |
|------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
| | err == -ENOTSUP | local_err != NULL, is equivalent to: | | |
| 1 | bridge_dev->msi == ON_OFF_AUTO_AUTO | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | same |
|------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
| | err == 0 | local_err == NULL, is equivalent to: | | |
| 2 | bridge_dev->msi == ON_OFF_AUTO_AUTO | assert(!local_err) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | same |
|------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
| | err == 0 | local_err == NULL, is equivalent to: | | |
| 3 | bridge_dev->msi != ON_OFF_AUTO_AUTO | assert(!local_err) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) |different|
------------------------------------------------------------------------------------------------------------------------------------------------------------------
In the case 1 & 2, the code runs the same path as before,
but in case 3 does not, it will be abort after the patch,
So drop the !local_err assert is really not appropriate,
will revert it in the v7. :)
Thanks,
Mao
> Thanks,
> Marcel
>
>
>
>
>
> no
>> error message in it. So the assertation of local_err is always superfluous.
>>
>
>
>> Thanks
>> Mao
>>
>>
>>> Other than that, the patch looks OK to me.
>>>
>>> Thanks,
>>> Marcel
>>>
>>>> /* With msi=auto, we fall back to MSI off silently */
>>>> - error_free(local_err);
>>>> }
>>>> if (shpc_present(dev)) {
>>>> @@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
>>>> PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
>>>> }
>>>> - return 0;
>>>> + return;
>>>> msi_error:
>>>> slotid_cap_cleanup(dev);
>>>> @@ -111,8 +108,6 @@ slotid_error:
>>>> }
>>>> shpc_error:
>>>> pci_bridge_exitfn(dev);
>>>> -
>>>> - return err;
>>>> }
>>>> static void pci_bridge_dev_exitfn(PCIDevice *dev)
>>>> @@ -216,7 +211,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>>> - k->init = pci_bridge_dev_initfn;
>>>> + k->realize = pci_bridge_dev_realize;
>>>> k->exit = pci_bridge_dev_exitfn;
>>>> k->config_write = pci_bridge_dev_write_config;
>>>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
>>>> index d72d5e4..69fc14b 100644
>>>> --- a/hw/pci/shpc.c
>>>> +++ b/hw/pci/shpc.c
>>>> @@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
>>>> }
>>>> /* Add SHPC capability to the config space for the device. */
>>>> -static int shpc_cap_add_config(PCIDevice *d)
>>>> +static int shpc_cap_add_config(PCIDevice *d, Error **errp)
>>>> {
>>>> uint8_t *config;
>>>> int config_offset;
>>>> - Error *local_err = NULL;
>>>> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
>>>> 0, SHPC_CAP_LENGTH,
>>>> - &local_err);
>>>> + errp);
>>>> if (config_offset < 0) {
>>>> - error_report_err(local_err);
>>>> return config_offset;
>>>> }
>>>> config = d->config + config_offset;
>>>> @@ -584,13 +582,14 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>> }
>>>> /* Initialize the SHPC structure in bridge's BAR. */
>>>> -int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
>>>> +int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
>>>> + unsigned offset, Error **errp)
>>>> {
>>>> int i, ret;
>>>> int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
>>>> SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
>>>> shpc->sec_bus = sec_bus;
>>>> - ret = shpc_cap_add_config(d);
>>>> + ret = shpc_cap_add_config(d, errp);
>>>> if (ret) {
>>>> g_free(d->shpc);
>>>> return ret;
>>>> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
>>>> index bdca205..36d021b 100644
>>>> --- a/hw/pci/slotid_cap.c
>>>> +++ b/hw/pci/slotid_cap.c
>>>> @@ -9,14 +9,14 @@
>>>> int slotid_cap_init(PCIDevice *d, int nslots,
>>>> uint8_t chassis,
>>>> - unsigned offset)
>>>> + unsigned offset,
>>>> + Error **errp)
>>>> {
>>>> int cap;
>>>> - Error *local_err = NULL;
>>>> if (!chassis) {
>>>> - error_report("Bridge chassis not specified. Each bridge is required "
>>>> - "to be assigned a unique chassis id > 0.");
>>>> + error_setg(errp, "Bridge chassis not specified. Each bridge is required"
>>>> + " to be assigned a unique chassis id > 0.");
>>>> return -EINVAL;
>>>> }
>>>> if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >> SLOTID_NSLOTS_SHIFT)) {
>>>> @@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
>>>> }
>>>> cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
>>>> - SLOTID_CAP_LENGTH, &local_err);
>>>> + SLOTID_CAP_LENGTH, errp);
>>>> if (cap < 0) {
>>>> - error_report_err(local_err);
>>>> return cap;
>>>> }
>>>> /* We make each chassis unique, this way each bridge is First in Chassis */
>>>> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
>>>> index 71e836b..ee19fec 100644
>>>> --- a/include/hw/pci/shpc.h
>>>> +++ b/include/hw/pci/shpc.h
>>>> @@ -38,7 +38,8 @@ struct SHPCDevice {
>>>> void shpc_reset(PCIDevice *d);
>>>> int shpc_bar_size(PCIDevice *dev);
>>>> -int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
>>>> +int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
>>>> + unsigned off, Error **errp);
>>>> void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
>>>> void shpc_free(PCIDevice *dev);
>>>> void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
>>>> diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
>>>> index 70db047..a777ea0 100644
>>>> --- a/include/hw/pci/slotid_cap.h
>>>> +++ b/include/hw/pci/slotid_cap.h
>>>> @@ -5,7 +5,8 @@
>>>> int slotid_cap_init(PCIDevice *dev, int nslots,
>>>> uint8_t chassis,
>>>> - unsigned offset);
>>>> + unsigned offset,
>>>> + Error **errp);
>>>> void slotid_cap_cleanup(PCIDevice *dev);
>>>> #endif
>>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error
2017-06-22 3:47 ` Mao Zhongyi
@ 2017-06-22 6:29 ` Marcel Apfelbaum
2017-06-22 6:55 ` Mao Zhongyi
0 siblings, 1 reply; 21+ messages in thread
From: Marcel Apfelbaum @ 2017-06-22 6:29 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel; +Cc: mst, armbru
On 22/06/2017 6:47, Mao Zhongyi wrote:
>
>
> On 06/21/2017 03:39 PM, Marcel Apfelbaum wrote:
>> On 20/06/2017 6:51, Mao Zhongyi wrote:
>>> Hi, Marcel
>>>
>>> On 06/19/2017 08:05 PM, Marcel Apfelbaum wrote:
>>>> On 12/06/2017 16:48, Mao Zhongyi wrote:
>>>>> In order to propagate error message better, convert shpc_init() to
>>>>> Error also convert the pci_bridge_dev_initfn() to realize.
>>>>>
>>>>> Cc: mst@redhat.com
>>>>> Cc: marcel@redhat.com
>>>>> Cc: armbru@redhat.com
>>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>>> ---
>>>>> hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
>>>>> hw/pci/shpc.c | 11 +++++------
>>>>> hw/pci/slotid_cap.c | 11 +++++------
>>>>> include/hw/pci/shpc.h | 3 ++-
>>>>> include/hw/pci/slotid_cap.h | 3 ++-
>>>>> 5 files changed, 22 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c
>>>>> b/hw/pci-bridge/pci_bridge_dev.c
>>>>> index 5dbd933..30c4186 100644
>>>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>>>> @@ -49,12 +49,11 @@ struct PCIBridgeDev {
>>>>> };
>>>>> typedef struct PCIBridgeDev PCIBridgeDev;
>>>>> -static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> +static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>>>> {
>>>>> 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);
>>>>> @@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>>>> memory_region_init(&bridge_dev->bar, OBJECT(dev),
>>>>> "shpc-bar",
>>>>> shpc_bar_size(dev));
>>>>> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>>>> + err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0,
>>>>> errp);
>>>>> if (err) {
>>>>> goto shpc_error;
>>>>> }
>>>>> @@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> bridge_dev->msi = ON_OFF_AUTO_OFF;
>>>>> }
>>>>> - err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>>>>> + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
>>>>> if (err) {
>>>>> goto slotid_error;
>>>>> }
>>>>> @@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>>>>> /* it means SHPC exists, because MSI is needed by SHPC */
>>>>> - err = msi_init(dev, 0, 1, true, true, &local_err);
>>>>> + err = msi_init(dev, 0, 1, true, true, errp);
>>>>> /* 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) "
>>>>> + error_append_hint(errp, "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);
>>>>> + assert(bridge_dev->msi == ON_OFF_AUTO_AUTO);
>>>>
>>>> I am not sure we can drop the !local_err assert.
>>>> We don't have a local_err anymore, but the error
>>>> is stored now in errp.
>>>>
>>>
>>> I think it's OK if we drop the !local_err assert; even if we don't
>>> store the error
>>> message to errp.
>>>
>>> Because the code can go to assert(!local_err || bridge_dev->msi ==
>>> ON_OFF_AUTO_AUTO)
>>> only if the return value of msi_init is 0(success), then the
>>> local_err is NULL,
>>
>> I think I am missing something.
>> What if msi_init returns with -ENOTSUP and
>> bridge_dev->msi == ON_OFF_AUTO_AUTO ?
>> We do reach the assert with an err.
>> Previously it was a asserted "error allowed only on auto mode",
>> now we assert only "auto mode".
>
> Hi, Marcel
>
> Thanks for your explanation, you are right, I am missing something.
> We can do reach the assert in the next three cases:
>
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------
> | | |
> previous |
> now | |
> | case | condition
> |-------------------------------------------------------------|---------------------------------------------|
> result |
> | | | assert(!local_err ||
> bridge_dev->msi == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi ==
> ON_OFF_AUTO_AUTO) | |
> |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
>
> | | err == -ENOTSUP | local_err != NULL, is
> equivalent to:
> | | |
> | 1 | bridge_dev->msi == ON_OFF_AUTO_AUTO | assert(bridge_dev->msi
> == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi ==
> ON_OFF_AUTO_AUTO) | same |
> |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
>
> | | err == 0 | local_err == NULL, is
> equivalent to:
> | | |
> | 2 | bridge_dev->msi == ON_OFF_AUTO_AUTO |
> assert(!local_err) |
> assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | same |
> |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
>
> | | err == 0 | local_err == NULL, is
> equivalent to:
> | | |
> | 3 | bridge_dev->msi != ON_OFF_AUTO_AUTO |
> assert(!local_err) |
> assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) |different|
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> In the case 1 & 2, the code runs the same path as before,
> but in case 3 does not, it will be abort after the patch,
> So drop the !local_err assert is really not appropriate,
> will revert it in the v7. :)
Thanks for looking into it and sorry for the late review.
Marcel
>
> Thanks,
> Mao
>
>
>
>
>
>> Thanks,
>> Marcel
>>
>>
>>
>>
>>
>> no
>>> error message in it. So the assertation of local_err is always
>>> superfluous.
>>>
>>
>>
>>> Thanks
>>> Mao
>>>
>>>
>>>> Other than that, the patch looks OK to me.
>>>>
>>>> Thanks,
>>>> Marcel
>>>>
>>>>> /* With msi=auto, we fall back to MSI off silently */
>>>>> - error_free(local_err);
>>>>> }
>>>>> if (shpc_present(dev)) {
>>>>> @@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
>>>>> PCI_BASE_ADDRESS_MEM_TYPE_64,
>>>>> &bridge_dev->bar);
>>>>> }
>>>>> - return 0;
>>>>> + return;
>>>>> msi_error:
>>>>> slotid_cap_cleanup(dev);
>>>>> @@ -111,8 +108,6 @@ slotid_error:
>>>>> }
>>>>> shpc_error:
>>>>> pci_bridge_exitfn(dev);
>>>>> -
>>>>> - return err;
>>>>> }
>>>>> static void pci_bridge_dev_exitfn(PCIDevice *dev)
>>>>> @@ -216,7 +211,7 @@ static void
>>>>> pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>>>> - k->init = pci_bridge_dev_initfn;
>>>>> + k->realize = pci_bridge_dev_realize;
>>>>> k->exit = pci_bridge_dev_exitfn;
>>>>> k->config_write = pci_bridge_dev_write_config;
>>>>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
>>>>> index d72d5e4..69fc14b 100644
>>>>> --- a/hw/pci/shpc.c
>>>>> +++ b/hw/pci/shpc.c
>>>>> @@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
>>>>> }
>>>>> /* Add SHPC capability to the config space for the device. */
>>>>> -static int shpc_cap_add_config(PCIDevice *d)
>>>>> +static int shpc_cap_add_config(PCIDevice *d, Error **errp)
>>>>> {
>>>>> uint8_t *config;
>>>>> int config_offset;
>>>>> - Error *local_err = NULL;
>>>>> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
>>>>> 0, SHPC_CAP_LENGTH,
>>>>> - &local_err);
>>>>> + errp);
>>>>> if (config_offset < 0) {
>>>>> - error_report_err(local_err);
>>>>> return config_offset;
>>>>> }
>>>>> config = d->config + config_offset;
>>>>> @@ -584,13 +582,14 @@ void
>>>>> shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>>> }
>>>>> /* Initialize the SHPC structure in bridge's BAR. */
>>>>> -int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
>>>>> unsigned offset)
>>>>> +int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
>>>>> + unsigned offset, Error **errp)
>>>>> {
>>>>> int i, ret;
>>>>> int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
>>>>> SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
>>>>> shpc->sec_bus = sec_bus;
>>>>> - ret = shpc_cap_add_config(d);
>>>>> + ret = shpc_cap_add_config(d, errp);
>>>>> if (ret) {
>>>>> g_free(d->shpc);
>>>>> return ret;
>>>>> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
>>>>> index bdca205..36d021b 100644
>>>>> --- a/hw/pci/slotid_cap.c
>>>>> +++ b/hw/pci/slotid_cap.c
>>>>> @@ -9,14 +9,14 @@
>>>>> int slotid_cap_init(PCIDevice *d, int nslots,
>>>>> uint8_t chassis,
>>>>> - unsigned offset)
>>>>> + unsigned offset,
>>>>> + Error **errp)
>>>>> {
>>>>> int cap;
>>>>> - Error *local_err = NULL;
>>>>> if (!chassis) {
>>>>> - error_report("Bridge chassis not specified. Each bridge is
>>>>> required "
>>>>> - "to be assigned a unique chassis id > 0.");
>>>>> + error_setg(errp, "Bridge chassis not specified. Each
>>>>> bridge is required"
>>>>> + " to be assigned a unique chassis id > 0.");
>>>>> return -EINVAL;
>>>>> }
>>>>> if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >>
>>>>> SLOTID_NSLOTS_SHIFT)) {
>>>>> @@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
>>>>> }
>>>>> cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
>>>>> - SLOTID_CAP_LENGTH, &local_err);
>>>>> + SLOTID_CAP_LENGTH, errp);
>>>>> if (cap < 0) {
>>>>> - error_report_err(local_err);
>>>>> return cap;
>>>>> }
>>>>> /* We make each chassis unique, this way each bridge is First
>>>>> in Chassis */
>>>>> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
>>>>> index 71e836b..ee19fec 100644
>>>>> --- a/include/hw/pci/shpc.h
>>>>> +++ b/include/hw/pci/shpc.h
>>>>> @@ -38,7 +38,8 @@ struct SHPCDevice {
>>>>> void shpc_reset(PCIDevice *d);
>>>>> int shpc_bar_size(PCIDevice *dev);
>>>>> -int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
>>>>> unsigned off);
>>>>> +int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
>>>>> + unsigned off, Error **errp);
>>>>> void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
>>>>> void shpc_free(PCIDevice *dev);
>>>>> void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t
>>>>> val, int len);
>>>>> diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
>>>>> index 70db047..a777ea0 100644
>>>>> --- a/include/hw/pci/slotid_cap.h
>>>>> +++ b/include/hw/pci/slotid_cap.h
>>>>> @@ -5,7 +5,8 @@
>>>>> int slotid_cap_init(PCIDevice *dev, int nslots,
>>>>> uint8_t chassis,
>>>>> - unsigned offset);
>>>>> + unsigned offset,
>>>>> + Error **errp);
>>>>> void slotid_cap_cleanup(PCIDevice *dev);
>>>>> #endif
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error
2017-06-22 6:29 ` Marcel Apfelbaum
@ 2017-06-22 6:55 ` Mao Zhongyi
0 siblings, 0 replies; 21+ messages in thread
From: Mao Zhongyi @ 2017-06-22 6:55 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: mst, armbru
On 06/22/2017 02:29 PM, Marcel Apfelbaum wrote:
> On 22/06/2017 6:47, Mao Zhongyi wrote:
>>
>>
>> On 06/21/2017 03:39 PM, Marcel Apfelbaum wrote:
>>> On 20/06/2017 6:51, Mao Zhongyi wrote:
>>>> Hi, Marcel
>>>>
>>>> On 06/19/2017 08:05 PM, Marcel Apfelbaum wrote:
>>>>> On 12/06/2017 16:48, Mao Zhongyi wrote:
>>>>>> In order to propagate error message better, convert shpc_init() to
>>>>>> Error also convert the pci_bridge_dev_initfn() to realize.
>>>>>>
>>>>>> Cc: mst@redhat.com
>>>>>> Cc: marcel@redhat.com
>>>>>> Cc: armbru@redhat.com
>>>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>> hw/pci-bridge/pci_bridge_dev.c | 21 ++++++++-------------
>>>>>> hw/pci/shpc.c | 11 +++++------
>>>>>> hw/pci/slotid_cap.c | 11 +++++------
>>>>>> include/hw/pci/shpc.h | 3 ++-
>>>>>> include/hw/pci/slotid_cap.h | 3 ++-
>>>>>> 5 files changed, 22 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>>>>> index 5dbd933..30c4186 100644
>>>>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>>>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>>>>> @@ -49,12 +49,11 @@ struct PCIBridgeDev {
>>>>>> };
>>>>>> typedef struct PCIBridgeDev PCIBridgeDev;
>>>>>> -static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>> +static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
>>>>>> {
>>>>>> 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);
>>>>>> @@ -62,7 +61,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>> dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>>>>> memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>>>>>> shpc_bar_size(dev));
>>>>>> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>>>>> + err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0, errp);
>>>>>> if (err) {
>>>>>> goto shpc_error;
>>>>>> }
>>>>>> @@ -71,7 +70,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>> bridge_dev->msi = ON_OFF_AUTO_OFF;
>>>>>> }
>>>>>> - err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>>>>>> + err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp);
>>>>>> if (err) {
>>>>>> goto slotid_error;
>>>>>> }
>>>>>> @@ -79,20 +78,18 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>> if (bridge_dev->msi != ON_OFF_AUTO_OFF) {
>>>>>> /* it means SHPC exists, because MSI is needed by SHPC */
>>>>>> - err = msi_init(dev, 0, 1, true, true, &local_err);
>>>>>> + err = msi_init(dev, 0, 1, true, true, errp);
>>>>>> /* 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) "
>>>>>> + error_append_hint(errp, "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);
>>>>>> + assert(bridge_dev->msi == ON_OFF_AUTO_AUTO);
>>>>>
>>>>> I am not sure we can drop the !local_err assert.
>>>>> We don't have a local_err anymore, but the error
>>>>> is stored now in errp.
>>>>>
>>>>
>>>> I think it's OK if we drop the !local_err assert; even if we don't store the error
>>>> message to errp.
>>>>
>>>> Because the code can go to assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO)
>>>> only if the return value of msi_init is 0(success), then the local_err is NULL,
>>>
>>> I think I am missing something.
>>> What if msi_init returns with -ENOTSUP and
>>> bridge_dev->msi == ON_OFF_AUTO_AUTO ?
>>> We do reach the assert with an err.
>>> Previously it was a asserted "error allowed only on auto mode",
>>> now we assert only "auto mode".
>>
>> Hi, Marcel
>>
>> Thanks for your explanation, you are right, I am missing something.
>> We can do reach the assert in the next three cases:
>>
>> ------------------------------------------------------------------------------------------------------------------------------------------------------------------
>> | | | previous | now | |
>> | case | condition |-------------------------------------------------------------|---------------------------------------------| result |
>> | | | assert(!local_err || bridge_dev->msi == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | |
>> |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
>> | | err == -ENOTSUP | local_err != NULL, is equivalent to: | | |
>> | 1 | bridge_dev->msi == ON_OFF_AUTO_AUTO | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | same |
>> |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
>> | | err == 0 | local_err == NULL, is equivalent to: | | |
>> | 2 | bridge_dev->msi == ON_OFF_AUTO_AUTO | assert(!local_err) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) | same |
>> |------|-------------------------------------|-------------------------------------------------------------|---------------------------------------------|---------|
>> | | err == 0 | local_err == NULL, is equivalent to: | | |
>> | 3 | bridge_dev->msi != ON_OFF_AUTO_AUTO | assert(!local_err) | assert(bridge_dev->msi == ON_OFF_AUTO_AUTO) |different|
>> ------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>
>> In the case 1 & 2, the code runs the same path as before,
>> but in case 3 does not, it will be abort after the patch,
>> So drop the !local_err assert is really not appropriate,
>> will revert it in the v7. :)
>
>
> Thanks for looking into it and sorry for the late review.
> Marcel
That's all right. :)
Mao
>>> Thanks,
>>> Marcel
>>>
>>>
>>>
>>>
>>>
>>> no
>>>> error message in it. So the assertation of local_err is always superfluous.
>>>>
>>>
>>>
>>>> Thanks
>>>> Mao
>>>>
>>>>
>>>>> Other than that, the patch looks OK to me.
>>>>>
>>>>> Thanks,
>>>>> Marcel
>>>>>
>>>>>> /* With msi=auto, we fall back to MSI off silently */
>>>>>> - error_free(local_err);
>>>>>> }
>>>>>> if (shpc_present(dev)) {
>>>>>> @@ -101,7 +98,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>>> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
>>>>>> PCI_BASE_ADDRESS_MEM_TYPE_64, &bridge_dev->bar);
>>>>>> }
>>>>>> - return 0;
>>>>>> + return;
>>>>>> msi_error:
>>>>>> slotid_cap_cleanup(dev);
>>>>>> @@ -111,8 +108,6 @@ slotid_error:
>>>>>> }
>>>>>> shpc_error:
>>>>>> pci_bridge_exitfn(dev);
>>>>>> -
>>>>>> - return err;
>>>>>> }
>>>>>> static void pci_bridge_dev_exitfn(PCIDevice *dev)
>>>>>> @@ -216,7 +211,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>>> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>>>>>> - k->init = pci_bridge_dev_initfn;
>>>>>> + k->realize = pci_bridge_dev_realize;
>>>>>> k->exit = pci_bridge_dev_exitfn;
>>>>>> k->config_write = pci_bridge_dev_write_config;
>>>>>> k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>>>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
>>>>>> index d72d5e4..69fc14b 100644
>>>>>> --- a/hw/pci/shpc.c
>>>>>> +++ b/hw/pci/shpc.c
>>>>>> @@ -446,16 +446,14 @@ static void shpc_cap_update_dword(PCIDevice *d)
>>>>>> }
>>>>>> /* Add SHPC capability to the config space for the device. */
>>>>>> -static int shpc_cap_add_config(PCIDevice *d)
>>>>>> +static int shpc_cap_add_config(PCIDevice *d, Error **errp)
>>>>>> {
>>>>>> uint8_t *config;
>>>>>> int config_offset;
>>>>>> - Error *local_err = NULL;
>>>>>> config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
>>>>>> 0, SHPC_CAP_LENGTH,
>>>>>> - &local_err);
>>>>>> + errp);
>>>>>> if (config_offset < 0) {
>>>>>> - error_report_err(local_err);
>>>>>> return config_offset;
>>>>>> }
>>>>>> config = d->config + config_offset;
>>>>>> @@ -584,13 +582,14 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>>>> }
>>>>>> /* Initialize the SHPC structure in bridge's BAR. */
>>>>>> -int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
>>>>>> +int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar,
>>>>>> + unsigned offset, Error **errp)
>>>>>> {
>>>>>> int i, ret;
>>>>>> int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
>>>>>> SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
>>>>>> shpc->sec_bus = sec_bus;
>>>>>> - ret = shpc_cap_add_config(d);
>>>>>> + ret = shpc_cap_add_config(d, errp);
>>>>>> if (ret) {
>>>>>> g_free(d->shpc);
>>>>>> return ret;
>>>>>> diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
>>>>>> index bdca205..36d021b 100644
>>>>>> --- a/hw/pci/slotid_cap.c
>>>>>> +++ b/hw/pci/slotid_cap.c
>>>>>> @@ -9,14 +9,14 @@
>>>>>> int slotid_cap_init(PCIDevice *d, int nslots,
>>>>>> uint8_t chassis,
>>>>>> - unsigned offset)
>>>>>> + unsigned offset,
>>>>>> + Error **errp)
>>>>>> {
>>>>>> int cap;
>>>>>> - Error *local_err = NULL;
>>>>>> if (!chassis) {
>>>>>> - error_report("Bridge chassis not specified. Each bridge is required "
>>>>>> - "to be assigned a unique chassis id > 0.");
>>>>>> + error_setg(errp, "Bridge chassis not specified. Each bridge is required"
>>>>>> + " to be assigned a unique chassis id > 0.");
>>>>>> return -EINVAL;
>>>>>> }
>>>>>> if (nslots < 0 || nslots > (PCI_SID_ESR_NSLOTS >> SLOTID_NSLOTS_SHIFT)) {
>>>>>> @@ -25,9 +25,8 @@ int slotid_cap_init(PCIDevice *d, int nslots,
>>>>>> }
>>>>>> cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
>>>>>> - SLOTID_CAP_LENGTH, &local_err);
>>>>>> + SLOTID_CAP_LENGTH, errp);
>>>>>> if (cap < 0) {
>>>>>> - error_report_err(local_err);
>>>>>> return cap;
>>>>>> }
>>>>>> /* We make each chassis unique, this way each bridge is First in Chassis */
>>>>>> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
>>>>>> index 71e836b..ee19fec 100644
>>>>>> --- a/include/hw/pci/shpc.h
>>>>>> +++ b/include/hw/pci/shpc.h
>>>>>> @@ -38,7 +38,8 @@ struct SHPCDevice {
>>>>>> void shpc_reset(PCIDevice *d);
>>>>>> int shpc_bar_size(PCIDevice *dev);
>>>>>> -int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
>>>>>> +int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar,
>>>>>> + unsigned off, Error **errp);
>>>>>> void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
>>>>>> void shpc_free(PCIDevice *dev);
>>>>>> void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
>>>>>> diff --git a/include/hw/pci/slotid_cap.h b/include/hw/pci/slotid_cap.h
>>>>>> index 70db047..a777ea0 100644
>>>>>> --- a/include/hw/pci/slotid_cap.h
>>>>>> +++ b/include/hw/pci/slotid_cap.h
>>>>>> @@ -5,7 +5,8 @@
>>>>>> int slotid_cap_init(PCIDevice *dev, int nslots,
>>>>>> uint8_t chassis,
>>>>>> - unsigned offset);
>>>>>> + unsigned offset,
>>>>>> + Error **errp);
>>>>>> void slotid_cap_cleanup(PCIDevice *dev);
>>>>>> #endif
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-06-22 6:55 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-12 13:48 [Qemu-devel] [PATCH v5 0/9] Convert to realize and cleanup Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 1/9] pci: Clean up error checking in pci_add_capability() Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 2/9] pci: Add comment for pci_add_capability2() Mao Zhongyi
2017-06-19 10:13 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 3/9] pci: Fix the return value checking Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 4/9] pci: Make errp the last parameter of pci_add_capability() Mao Zhongyi
2017-06-19 11:17 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 5/9] pci: Replace pci_add_capability2() with pci_add_capability() Mao Zhongyi
2017-06-19 11:20 ` Marcel Apfelbaum
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 6/9] pci: Convert to realize Mao Zhongyi
2017-06-19 11:42 ` Marcel Apfelbaum
2017-06-20 2:41 ` Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 7/9] pci: Convert shpc_init() to Error Mao Zhongyi
2017-06-19 12:05 ` Marcel Apfelbaum
2017-06-20 3:51 ` Mao Zhongyi
2017-06-21 7:39 ` Marcel Apfelbaum
2017-06-22 3:47 ` Mao Zhongyi
2017-06-22 6:29 ` Marcel Apfelbaum
2017-06-22 6:55 ` Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 8/9] i386/kvm/pci-assign: Fix return type of verify_irqchip_kernel() Mao Zhongyi
2017-06-12 13:48 ` [Qemu-devel] [PATCH v5 9/9] i386/kvm/pci-assign: Use errp directly rather than local_err Mao Zhongyi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.