On Tue, Jun 06, 2017 at 07:26:30PM +0800, 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: pbonz...@redhat.com
> Cc: r...@twiddle.net
> Cc: ehabk...@redhat.com
> Cc: m...@redhat.com
> CC: dmi...@daynix.com
> Cc: jasow...@redhat.com
> Cc: mar...@redhat.com
> Cc: alex.william...@redhat.com
> Cc: arm...@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.f...@cn.fujitsu.com>
> ---
> hw/i386/amd_iommu.c | 24 +++++++++++++++++-------
> hw/net/e1000e.c | 7 ++++++-
> hw/net/eepro100.c | 20 +++++++++++++++-----
> 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 | 3 ++-
> hw/virtio/virtio-pci.c | 19 ++++++++++++++-----
> include/hw/pci/pci.h | 3 ++-
> 12 files changed, 85 insertions(+), 31 deletions(-)
There are multiple places below that checks for errors like this:
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
}
Sometimes it even includes another branch for the error path:
function(...);
if (function succeeded) {
/* non-error code path here */
foo = bar;
} else {
/* error path here */
return ret;
}
I suggest doing this instead, for readability:
function(...)
if (function failed) {
return ...; /* or: "goto out" */
}
/* non-error code path here */
foo = bar;
>
> 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;
Maybe adding a local_err variable is preferred instead of
checking for (pos < 0), but I'm not sure it's necessary.
Markus, what's the recommendation on those cases? Should we use
the negative return value to avoid adding an extra local_err
variable, or should we add local_err anyway to match the existing
style elsewhere?
(The same applies to other functions below [*])
> + }
> + 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..41430766 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,7 +373,9 @@ 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,
> @@ -386,6 +389,8 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset,
> uint16_t pmc)
>
> pci_set_word(pdev->w1cmask + offset + PCI_PM_CTRL,
> PCI_PM_CTRL_PME_STATUS);
> + } else {
> + error_report_err(local_err);
> }
I suggest this instead:
int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset,
PCI_PM_SIZEOF, &local_err);
if (local_err) {
error_report_err(local_err);
return ret;
}
pci_set_word(...);
pci_set_word(...);
pci_set_word(...);
return ret;
>
> return ret;
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 62e989c..0625839 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,9 +571,13 @@ 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);
> - pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> + cfg_offset, PCI_PM_SIZEOF,
> + errp);
> + if (r > 0) {
> + pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> + } else {
> + return;
> + }
I suggest this instead:
int r = pci_add_capability(..., errp);
if (r < 0) { [*]
return;
}
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
...
Or, even better, as this function is very long:
Error *local_err = NULL;
pci_add_capability(..., &local_err);
if (local_err) {
goto out;
}
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
...
out:
error_propagate(errp, local_err);
return;
> #if 0 /* TODO: replace dummy code for power management emulation. */
> /* TODO: Power Management Control / Status. */
> pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
> @@ -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;
> }
pci_add_capability() and pci_add_capability2() now do exactly the
same, why are both being kept? I suggest replacing
pci_add_capability2() with pci_add_capability() everywhere (on a
separate patch).
>
> 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..85cfe38 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1743,7 +1743,8 @@ 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);
> + pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
> + errp);
> if (pos > 0) {
> vdev->pdev.exp.exp_cap = pos;
> }
I would this instead:
pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
errp);
if (pos < 0) { [*]
return pos;
}
vdev->pdev.exp.exp_cap = pos;
...
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..cca5276 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1161,9 +1161,14 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy
> *proxy,
> {
> PCIDevice *dev = &proxy->pci_dev;
> int offset;
> + Error *local_err = NULL;
>
> - 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, &local_err);
> + if (offset < 0) {
> + error_report_err(local_err);
> + abort();
> + }
>
This can be simplified to:
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,9 +1815,13 @@ 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);
> - pci_dev->exp.pm_cap = pos;
> + pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0,
> + PCI_PM_SIZEOF, errp);
> + if (pos > 0) {
> + pci_dev->exp.pm_cap = pos;
> + } else {
> + return;
> + }
>
I suggest:
pos = pci_add_capability(...)
if (pos < 0) {
return;
}
pci_dev->exp.pm_cap = pos;
> /*
> * Indicates that this function complies with revision 1.2 of the
> 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
>
>
>
--
Eduardo
