* [PATCH] pci: Abort if pci_add_capability fails
@ 2022-08-29 8:44 小田喜陽彦
2022-08-30 11:37 ` Markus Armbruster
0 siblings, 1 reply; 5+ messages in thread
From: 小田喜陽彦 @ 2022-08-29 8:44 UTC (permalink / raw)
Cc: qemu-devel, qemu-block, qemu-arm, Michael S. Tsirkin,
Marcel Apfelbaum, Gerd Hoffmann, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, John Snow, Dmitry Fleytman, Jason Wang,
Stefan Weil, Keith Busch, Klaus Jensen, Peter Maydell,
Andrey Smirnov, Paul Burton, Aleksandar Rikalo, Alex Williamson,
Akihiko Odaki
From: Akihiko Odaki <akihiko.odaki@daynix.com>
pci_add_capability appears most PCI devices. The error handling required
lots of code, and led to inconsistent behaviors such as:
- passing error_abort
- passing error_fatal
- asserting the returned value
- propagating the error to the caller
- skipping the rest of the function
- just ignoring
pci_add_capability fails if the new capability overlaps with an existing
one. It happens only if the device implementation is wrong so
pci_add_capability can just abort instead of returning to the caller in
the case, fixing inconsistencies and removing extra code.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
docs/pcie_sriov.txt | 4 +--
hw/display/bochs-display.c | 4 +--
hw/i386/amd_iommu.c | 18 ++---------
hw/ide/ich.c | 8 ++---
hw/net/e1000e.c | 22 +++----------
hw/net/eepro100.c | 7 +---
hw/nvme/ctrl.c | 10 +-----
hw/pci-bridge/cxl_downstream.c | 9 ++----
hw/pci-bridge/cxl_upstream.c | 9 ++----
hw/pci-bridge/i82801b11.c | 15 ++-------
hw/pci-bridge/pci_bridge_dev.c | 2 +-
hw/pci-bridge/pcie_pci_bridge.c | 17 +++-------
hw/pci-bridge/pcie_root_port.c | 16 ++-------
hw/pci-bridge/xio3130_downstream.c | 15 ++-------
hw/pci-bridge/xio3130_upstream.c | 15 ++-------
hw/pci-host/designware.c | 3 +-
hw/pci-host/xilinx-pcie.c | 5 +--
hw/pci/msi.c | 9 +-----
hw/pci/msix.c | 8 ++---
hw/pci/pci.c | 14 +++-----
hw/pci/pci_bridge.c | 22 ++++---------
hw/pci/pcie.c | 52 ++++++++----------------------
hw/pci/shpc.c | 16 +++------
hw/pci/slotid_cap.c | 8 ++---
hw/usb/hcd-xhci-pci.c | 3 +-
hw/vfio/pci-quirks.c | 15 ++-------
hw/vfio/pci.c | 12 +++----
hw/virtio/virtio-pci.c | 22 ++++---------
include/hw/pci/pci.h | 5 ++-
include/hw/pci/pci_bridge.h | 5 ++-
include/hw/pci/pcie.h | 11 +++----
include/hw/pci/shpc.h | 5 ++-
include/hw/virtio/virtio-pci.h | 2 +-
33 files changed, 98 insertions(+), 290 deletions(-)
diff --git a/docs/pcie_sriov.txt b/docs/pcie_sriov.txt
index 11158dbf88..728a73ba7b 100644
--- a/docs/pcie_sriov.txt
+++ b/docs/pcie_sriov.txt
@@ -49,7 +49,7 @@ setting up a BAR for a VF.
pci_your_pf_dev_realize( ... )
{
...
- int ret = pcie_endpoint_cap_init(d, 0x70);
+ pcie_endpoint_cap_init(d, 0x70);
...
pcie_ari_init(d, 0x100, 1);
...
@@ -79,7 +79,7 @@ setting up a BAR for a VF.
pci_your_vf_dev_realize( ... )
{
...
- int ret = pcie_endpoint_cap_init(d, 0x60);
+ pcie_endpoint_cap_init(d, 0x60);
...
pcie_ari_init(d, 0x100, 1);
...
diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 8ed734b195..111cabcfb3 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -265,7 +265,6 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
{
BochsDisplayState *s = BOCHS_DISPLAY(dev);
Object *obj = OBJECT(dev);
- int ret;
if (s->vgamem < 4 * MiB) {
error_setg(errp, "bochs-display: video memory too small");
@@ -302,8 +301,7 @@ static void bochs_display_realize(PCIDevice *dev, Error **errp)
}
if (pci_bus_is_express(pci_get_bus(dev))) {
- ret = pcie_endpoint_cap_init(dev, 0x80);
- assert(ret > 0);
+ pcie_endpoint_cap_init(dev, 0x80);
} else {
dev->cap_present &= ~QEMU_PCI_CAP_EXPRESS;
}
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 725f69095b..256ecba1c3 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1553,23 +1553,11 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
if (!qdev_realize(DEVICE(&s->pci), &bus->qbus, errp)) {
return;
}
- ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
- AMDVI_CAPAB_SIZE, errp);
- if (ret < 0) {
- return;
- }
+ pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0, AMDVI_CAPAB_SIZE);
s->capab_offset = ret;
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
- AMDVI_CAPAB_REG_SIZE, errp);
- if (ret < 0) {
- return;
- }
- ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
- AMDVI_CAPAB_REG_SIZE, errp);
- if (ret < 0) {
- return;
- }
+ pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
+ pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
/* Pseudo address space under root PCI bus. */
x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 1007a51fcb..7349faa78f 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -106,7 +106,7 @@ static void pci_ich9_ahci_init(Object *obj)
static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
{
struct AHCIPCIState *d;
- int sata_cap_offset;
+ uint8_t sata_cap_offset;
uint8_t *sata_cap;
d = ICH9_AHCI(dev);
int ret;
@@ -130,11 +130,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
&d->ahci.mem);
sata_cap_offset = pci_add_capability(dev, PCI_CAP_ID_SATA,
- ICH9_SATA_CAP_OFFSET, SATA_CAP_SIZE,
- errp);
- if (sata_cap_offset < 0) {
- return;
- }
+ ICH9_SATA_CAP_OFFSET, SATA_CAP_SIZE);
sata_cap = dev->config + sata_cap_offset;
pci_set_word(sata_cap + SATA_CAP_REV, 0x10);
diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index ac96f7665a..aea4305c43 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -377,17 +377,10 @@ e1000e_gen_dsn(uint8_t *mac)
(uint64_t)(mac[0]) << 56;
}
-static int
+static void
e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc)
{
- Error *local_err = NULL;
- 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_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
pci_set_word(pdev->config + offset + PCI_PM_PMC,
PCI_PM_CAP_VER_1_1 |
@@ -400,8 +393,6 @@ 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);
-
- return ret;
}
static void e1000e_write_config(PCIDevice *pci_dev, uint32_t address,
@@ -471,19 +462,14 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
e1000e_init_msix(s);
- if (pcie_endpoint_cap_v1_init(pci_dev, e1000e_pcie_offset) < 0) {
- hw_error("Failed to initialize PCIe capability");
- }
+ pcie_endpoint_cap_v1_init(pci_dev, e1000e_pcie_offset);
ret = msi_init(PCI_DEVICE(s), 0xD0, 1, true, false, NULL);
if (ret) {
trace_e1000e_msi_init_fail(ret);
}
- if (e1000e_add_pm_capability(pci_dev, e1000e_pmrb_offset,
- PCI_PM_CAP_DSI) < 0) {
- hw_error("Failed to initialize PM capability");
- }
+ e1000e_add_pm_capability(pci_dev, e1000e_pmrb_offset, PCI_PM_CAP_DSI);
if (pcie_aer_init(pci_dev, PCI_ERR_VER, e1000e_aer_offset,
PCI_ERR_SIZEOF, NULL) < 0) {
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 679f52f80f..bf2ecdded9 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -549,12 +549,7 @@ static void e100_pci_reset(EEPRO100State *s, Error **errp)
if (info->power_management) {
/* Power Management Capabilities */
int cfg_offset = 0xdc;
- int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM,
- cfg_offset, PCI_PM_SIZEOF,
- errp);
- if (r < 0) {
- return;
- }
+ pci_add_capability(&s->dev, PCI_CAP_ID_PM, cfg_offset, PCI_PM_SIZEOF);
pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
#if 0 /* TODO: replace dummy code for power management emulation. */
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 87aeba0564..ee56aa7cba 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7327,15 +7327,7 @@ static void nvme_init_sriov(NvmeCtrl *n, PCIDevice *pci_dev, uint16_t offset)
static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
{
- Error *err = NULL;
- int ret;
-
- ret = pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset,
- PCI_PM_SIZEOF, &err);
- if (err) {
- error_report_err(err);
- return ret;
- }
+ pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF);
pci_set_word(pci_dev->config + offset + PCI_PM_PMC,
PCI_PM_CAP_VER_1_2);
diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
index a361e519d0..1980dd9c6c 100644
--- a/hw/pci-bridge/cxl_downstream.c
+++ b/hw/pci-bridge/cxl_downstream.c
@@ -155,12 +155,8 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
goto err_bridge;
}
- rc = pcie_cap_init(d, CXL_DOWNSTREAM_PORT_EXP_OFFSET,
- PCI_EXP_TYPE_DOWNSTREAM, p->port,
- errp);
- if (rc < 0) {
- goto err_msi;
- }
+ pcie_cap_init(d, CXL_DOWNSTREAM_PORT_EXP_OFFSET,
+ PCI_EXP_TYPE_DOWNSTREAM, p->port);
pcie_cap_flr_init(d);
pcie_cap_deverr_init(d);
@@ -195,7 +191,6 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
pcie_chassis_del_slot(s);
err_pcie_cap:
pcie_cap_exit(d);
- err_msi:
msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index a83a3e81e4..ca25b0c4d9 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -138,11 +138,8 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp)
goto err_bridge;
}
- rc = pcie_cap_init(d, CXL_UPSTREAM_PORT_PCIE_CAP_OFFSET,
- PCI_EXP_TYPE_UPSTREAM, p->port, errp);
- if (rc < 0) {
- goto err_msi;
- }
+ pcie_cap_init(d, CXL_UPSTREAM_PORT_PCIE_CAP_OFFSET,
+ PCI_EXP_TYPE_UPSTREAM, p->port);
pcie_cap_flr_init(d);
pcie_cap_deverr_init(d);
@@ -165,8 +162,6 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp)
err_cap:
pcie_cap_exit(d);
-err_msi:
- msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
}
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index f28181e210..ddeb55b8b6 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -61,21 +61,10 @@ typedef struct I82801b11Bridge {
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,
- errp);
- if (rc < 0) {
- goto err_bridge;
- }
+ pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
+ I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
pci_config_set_prog_interface(d->config, PCI_CLASS_BRIDGE_PCI_INF_SUB);
- return;
-
-err_bridge:
- pci_bridge_exitfn(d);
}
static const VMStateDescription i82801b11_bridge_dev_vmstate = {
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 657a06ddbe..4b6d1876eb 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -66,7 +66,7 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error **errp)
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, errp);
+ err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
if (err) {
goto shpc_error;
}
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index 1cd917a459..c50ba1fa80 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -35,27 +35,22 @@ static void pcie_pci_bridge_realize(PCIDevice *d, Error **errp)
{
PCIBridge *br = PCI_BRIDGE(d);
PCIEPCIBridge *pcie_br = PCIE_PCI_BRIDGE_DEV(d);
- int rc, pos;
+ int rc;
+ uint8_t pos;
pci_bridge_initfn(d, TYPE_PCI_BUS);
d->config[PCI_INTERRUPT_PIN] = 0x1;
memory_region_init(&pcie_br->shpc_bar, OBJECT(d), "shpc-bar",
shpc_bar_size(d));
- rc = shpc_init(d, &br->sec_bus, &pcie_br->shpc_bar, 0, errp);
+ rc = shpc_init(d, &br->sec_bus, &pcie_br->shpc_bar, 0);
if (rc) {
goto error;
}
- rc = pcie_cap_init(d, 0, PCI_EXP_TYPE_PCI_BRIDGE, 0, errp);
- if (rc < 0) {
- goto cap_error;
- }
+ pcie_cap_init(d, 0, PCI_EXP_TYPE_PCI_BRIDGE, 0);
- pos = pci_add_capability(d, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF, errp);
- if (pos < 0) {
- goto pm_error;
- }
+ pos = pci_add_capability(d, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
d->exp.pm_cap = pos;
pci_set_word(d->config + pos + PCI_PM_PMC, 0x3);
@@ -88,9 +83,7 @@ static void pcie_pci_bridge_realize(PCIDevice *d, Error **errp)
msi_error:
pcie_aer_exit(d);
aer_error:
-pm_error:
pcie_cap_exit(d);
-cap_error:
shpc_cleanup(d, &pcie_br->shpc_bar);
error:
pci_bridge_exitfn(d);
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 460e48269d..92cebc7cce 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -74,12 +74,7 @@ static void rp_realize(PCIDevice *d, Error **errp)
}
pcie_port_init_reg(d);
- rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
- rpc->ssid, errp);
- if (rc < 0) {
- error_append_hint(errp, "Can't init SSV ID, error %d\n", rc);
- goto err_bridge;
- }
+ pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id, rpc->ssid);
if (rpc->interrupts_init) {
rc = rpc->interrupts_init(d, errp);
@@ -88,13 +83,7 @@ static void rp_realize(PCIDevice *d, Error **errp)
}
}
- rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
- p->port, errp);
- if (rc < 0) {
- error_append_hint(errp, "Can't add Root Port capability, "
- "error %d\n", rc);
- goto err_int;
- }
+ pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT, p->port);
pcie_cap_arifwd_init(d);
pcie_cap_deverr_init(d);
@@ -125,7 +114,6 @@ err:
pcie_chassis_del_slot(s);
err_pcie_cap:
pcie_cap_exit(d);
-err_int:
if (rpc->interrupts_uninit) {
rpc->interrupts_uninit(d);
}
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 05e2b06c0c..37307c8c23 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -81,18 +81,10 @@ static void xio3130_downstream_realize(PCIDevice *d, Error **errp)
goto err_bridge;
}
- rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
- XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
- errp);
- if (rc < 0) {
- goto err_msi;
- }
+ pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
+ XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
- rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM,
- p->port, errp);
- if (rc < 0) {
- goto err_msi;
- }
+ pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_DOWNSTREAM, p->port);
pcie_cap_flr_init(d);
pcie_cap_deverr_init(d);
pcie_cap_slot_init(d, s);
@@ -117,7 +109,6 @@ err:
pcie_chassis_del_slot(s);
err_pcie_cap:
pcie_cap_exit(d);
-err_msi:
msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 5ff46ef050..546224d97c 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -71,18 +71,10 @@ static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
goto err_bridge;
}
- rc = pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
- XIO3130_SSVID_SVID, XIO3130_SSVID_SSID,
- errp);
- if (rc < 0) {
- goto err_msi;
- }
+ pci_bridge_ssvid_init(d, XIO3130_SSVID_OFFSET,
+ XIO3130_SSVID_SVID, XIO3130_SSVID_SSID);
- rc = pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM,
- p->port, errp);
- if (rc < 0) {
- goto err_msi;
- }
+ pcie_cap_init(d, XIO3130_EXP_OFFSET, PCI_EXP_TYPE_UPSTREAM, p->port);
pcie_cap_flr_init(d);
pcie_cap_deverr_init(d);
@@ -96,7 +88,6 @@ static void xio3130_upstream_realize(PCIDevice *d, Error **errp)
err:
pcie_cap_exit(d);
-err_msi:
msi_uninit(d);
err_bridge:
pci_bridge_exitfn(d);
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index bde3a343a2..3e4972ad76 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -414,8 +414,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
pcie_port_init_reg(dev);
- pcie_cap_init(dev, 0x70, PCI_EXP_TYPE_ROOT_PORT,
- 0, &error_fatal);
+ pcie_cap_init(dev, 0x70, PCI_EXP_TYPE_ROOT_PORT, 0);
msi_nonbroken = true;
msi_init(dev, 0x50, 32, true, true, &error_fatal);
diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 38d5901a45..4c6f1ab3d0 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -281,10 +281,7 @@ static void xilinx_pcie_root_realize(PCIDevice *pci_dev, Error **errp)
((s->mmio_base + s->mmio_size - 1) >> 16) & 0xfff0);
pci_bridge_initfn(pci_dev, TYPE_PCI_BUS);
-
- if (pcie_endpoint_cap_v1_init(pci_dev, 0x80) < 0) {
- error_setg(errp, "Failed to initialize PCIe capability");
- }
+ pcie_endpoint_cap_v1_init(pci_dev, 0x80);
}
static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index 058d1d1ef1..5283a08b5a 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -194,7 +194,6 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
unsigned int vectors_order;
uint16_t flags;
uint8_t cap_size;
- int config_offset;
if (!msi_nonbroken) {
error_setg(errp, "MSI is not supported by interrupt controller");
@@ -221,13 +220,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
}
cap_size = msi_cap_sizeof(flags);
- config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset,
- cap_size, errp);
- if (config_offset < 0) {
- return config_offset;
- }
-
- dev->msi_cap = config_offset;
+ dev->msi_cap = pci_add_capability(dev, PCI_CAP_ID_MSI, offset, cap_size);
dev->cap_present |= QEMU_PCI_CAP_MSI;
pci_set_word(dev->config + msi_flags_off(dev), flags);
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 1e381a9813..28af83403b 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -311,7 +311,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos,
Error **errp)
{
- int cap;
+ uint8_t cap;
unsigned table_size, pba_size;
uint8_t *config;
@@ -340,11 +340,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
return -EINVAL;
}
- cap = pci_add_capability(dev, PCI_CAP_ID_MSIX,
- cap_pos, MSIX_CAP_LENGTH, errp);
- if (cap < 0) {
- return cap;
- }
+ cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
dev->msix_cap = cap;
dev->cap_present |= QEMU_PCI_CAP_MSIX;
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2f450f6a72..5831dfc742 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2513,14 +2513,11 @@ static void pci_del_option_rom(PCIDevice *pdev)
}
/*
- * 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.
+ * pci_add_capability() returns a positive value that the offset of the pci
+ * capability.
*/
-int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
- uint8_t offset, uint8_t size,
- Error **errp)
+uint8_t pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
+ uint8_t offset, uint8_t size)
{
uint8_t *config;
int i, overlapping_cap;
@@ -2537,13 +2534,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
for (i = offset; i < offset + size; i++) {
overlapping_cap = pci_find_capability_at_offset(pdev, i);
if (overlapping_cap) {
- error_setg(errp, "%s:%02x:%02x.%x "
+ error_setg(&error_abort, "%s:%02x:%02x.%x "
"Attempt to add PCI capability %x at offset "
"%x overlaps existing capability %x at offset %x",
pci_root_bus_path(pdev), pci_dev_bus_num(pdev),
PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
cap_id, offset, overlapping_cap, i);
- return -EINVAL;
}
}
}
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index da34c8ebcd..ebc03e2bab 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -42,21 +42,14 @@
#define PCI_SSVID_SVID 4
#define PCI_SSVID_SSID 6
-int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
- uint16_t svid, uint16_t ssid,
- Error **errp)
+void 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, errp);
- if (pos < 0) {
- return pos;
- }
+ uint8_t pos;
+ pos = pci_add_capability(dev, PCI_CAP_ID_SSVID, offset, PCI_SSVID_SIZEOF);
pci_set_word(dev->config + pos + PCI_SSVID_SVID, svid);
pci_set_word(dev->config + pos + PCI_SSVID_SSID, ssid);
- return pos;
}
/* Accessor function to get parent bridge device from pci bus. */
@@ -455,11 +448,8 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
.mem_pref_64 = cpu_to_le64(res_reserve.mem_pref_64)
};
- int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
- cap_offset, cap_len, errp);
- if (offset < 0) {
- return offset;
- }
+ uint8_t offset = pci_add_capability(dev, PCI_CAP_ID_VNDR,
+ cap_offset, cap_len);
memcpy(dev->config + offset + PCI_CAP_FLAGS,
(char *)&cap + PCI_CAP_FLAGS,
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 68a62da0b5..923ad29c52 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -151,21 +151,15 @@ static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
}
}
-int pcie_cap_init(PCIDevice *dev, uint8_t offset,
- uint8_t type, uint8_t port,
- Error **errp)
+void pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port)
{
/* PCIe cap v2 init */
- int pos;
+ uint8_t pos;
uint8_t *exp_cap;
assert(pci_is_express(dev));
- pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
- PCI_EXP_VER2_SIZEOF, errp);
- if (pos < 0) {
- return pos;
- }
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF);
dev->exp.exp_cap = pos;
exp_cap = dev->config + pos;
@@ -185,38 +179,26 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
/* read-only to behave like a 'NULL' Extended Capability Header */
pci_set_long(dev->wmask + PCI_CONFIG_SPACE_SIZE, 0);
}
-
- return pos;
}
-int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
- uint8_t port)
+void pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type,
+ uint8_t port)
{
/* PCIe cap v1 init */
- int pos;
- Error *local_err = NULL;
+ uint8_t pos;
assert(pci_is_express(dev));
- 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;
- }
+ pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER1_SIZEOF);
dev->exp.exp_cap = pos;
pcie_cap_v1_fill(dev, port, type, PCI_EXP_FLAGS_VER1);
-
- return pos;
}
-static int
+static void
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;
- int ret;
/*
* Windows guests will report Code 10, device cannot start, if
@@ -229,26 +211,20 @@ pcie_endpoint_cap_common_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size)
}
if (cap_size == PCI_EXP_VER1_SIZEOF) {
- return pcie_cap_v1_init(dev, offset, type, 0);
+ pcie_cap_v1_init(dev, offset, type, 0);
} else {
- ret = pcie_cap_init(dev, offset, type, 0, &local_err);
-
- if (ret < 0) {
- error_report_err(local_err);
- }
-
- return ret;
+ pcie_cap_init(dev, offset, type, 0);
}
}
-int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
+void pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset)
{
- return pcie_endpoint_cap_common_init(dev, offset, PCI_EXP_VER2_SIZEOF);
+ pcie_endpoint_cap_common_init(dev, offset, PCI_EXP_VER2_SIZEOF);
}
-int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset)
+void pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset)
{
- return pcie_endpoint_cap_common_init(dev, offset, PCI_EXP_VER1_SIZEOF);
+ pcie_endpoint_cap_common_init(dev, offset, PCI_EXP_VER1_SIZEOF);
}
void pcie_cap_exit(PCIDevice *dev)
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index e71f3a7483..275644ab48 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -440,16 +440,11 @@ 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, Error **errp)
+static int shpc_cap_add_config(PCIDevice *d)
{
uint8_t *config;
- int config_offset;
- config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC,
- 0, SHPC_CAP_LENGTH,
- errp);
- if (config_offset < 0) {
- return config_offset;
- }
+ uint8_t config_offset;
+ config_offset = pci_add_capability(d, PCI_CAP_ID_SHPC, 0, SHPC_CAP_LENGTH);
config = d->config + config_offset;
pci_set_byte(config + SHPC_CAP_DWORD_SELECT, 0);
@@ -584,14 +579,13 @@ void shpc_device_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, Error **errp)
+int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
{
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, errp);
+ ret = shpc_cap_add_config(d);
if (ret) {
g_free(d->shpc);
return ret;
diff --git a/hw/pci/slotid_cap.c b/hw/pci/slotid_cap.c
index 36d021b4a6..5da8c82133 100644
--- a/hw/pci/slotid_cap.c
+++ b/hw/pci/slotid_cap.c
@@ -12,7 +12,7 @@ int slotid_cap_init(PCIDevice *d, int nslots,
unsigned offset,
Error **errp)
{
- int cap;
+ uint8_t cap;
if (!chassis) {
error_setg(errp, "Bridge chassis not specified. Each bridge is required"
@@ -24,11 +24,7 @@ int slotid_cap_init(PCIDevice *d, int nslots,
return -EINVAL;
}
- cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset,
- SLOTID_CAP_LENGTH, errp);
- if (cap < 0) {
- return cap;
- }
+ cap = pci_add_capability(d, PCI_CAP_ID_SLOTID, offset, SLOTID_CAP_LENGTH);
/* We make each chassis unique, this way each bridge is First in Chassis */
d->config[cap + PCI_SID_ESR] = PCI_SID_ESR_FIC |
(nslots << SLOTID_NSLOTS_SHIFT);
diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index e934b1a5b1..0eba2b36ae 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -150,8 +150,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
if (pci_bus_is_express(pci_get_bus(dev)) ||
xhci_get_flag(&s->xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
- ret = pcie_endpoint_cap_init(dev, 0xa0);
- assert(ret > 0);
+ pcie_endpoint_cap_init(dev, 0xa0);
}
if (s->msix != ON_OFF_AUTO_OFF) {
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index f0147a050a..e94fd273ea 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1530,7 +1530,7 @@ const PropertyInfo qdev_prop_nv_gpudirect_clique = {
static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
{
PCIDevice *pdev = &vdev->pdev;
- int ret, pos = 0xC8;
+ int pos = 0xC8;
if (vdev->nv_gpudirect_clique == 0xFF) {
return 0;
@@ -1547,11 +1547,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
return -EINVAL;
}
- ret = pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8, errp);
- if (ret < 0) {
- error_prepend(errp, "Failed to add NVIDIA GPUDirect cap: ");
- return ret;
- }
+ pci_add_capability(pdev, PCI_CAP_ID_VNDR, pos, 8);
memset(vdev->emulated_config_bits + pos, 0xFF, 8);
pos += PCI_CAP_FLAGS;
@@ -1718,12 +1714,7 @@ static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
return -EFAULT;
}
- ret = pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos,
- VMD_SHADOW_CAP_LEN, errp);
- if (ret < 0) {
- error_prepend(errp, "Failed to add VMD MEMBAR Shadow cap: ");
- return ret;
- }
+ pci_add_capability(&vdev->pdev, PCI_CAP_ID_VNDR, pos, VMD_SHADOW_CAP_LEN);
memset(vdev->emulated_config_bits + pos, 0xFF, VMD_SHADOW_CAP_LEN);
pos += PCI_CAP_FLAGS;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 939dcc3d4a..2f0038a73f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1943,11 +1943,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
1, PCI_EXP_FLAGS_VERS);
}
- pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size,
- errp);
- if (pos < 0) {
- return pos;
- }
+ pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size);
vdev->pdev.exp.exp_cap = pos;
@@ -2045,14 +2041,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_capability(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size);
break;
case PCI_CAP_ID_AF:
vfio_check_af_flr(vdev, pos);
- ret = pci_add_capability(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size);
break;
default:
- ret = pci_add_capability(pdev, cap_id, pos, size, errp);
+ ret = pci_add_capability(pdev, cap_id, pos, size);
break;
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 45327f0b31..19ef46169f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1148,14 +1148,13 @@ static bool virtio_pci_queue_enabled(DeviceState *d, int n)
return virtio_queue_enabled_legacy(vdev, n);
}
-static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
- struct virtio_pci_cap *cap)
+static uint8_t virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
+ struct virtio_pci_cap *cap)
{
PCIDevice *dev = &proxy->pci_dev;
- int offset;
+ uint8_t offset;
- offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0,
- cap->cap_len, &error_abort);
+ offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len);
assert(cap->cap_len >= sizeof *cap);
memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len,
@@ -1859,18 +1858,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
}
if (pcie_port && pci_is_express(pci_dev)) {
- int pos;
+ uint8_t pos;
uint16_t last_pcie_cap_offset = PCI_CONFIG_SPACE_SIZE;
- 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, errp);
- if (pos < 0) {
- return;
- }
-
+ pcie_endpoint_cap_init(pci_dev, 0);
+ pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
pci_dev->exp.pm_cap = pos;
/*
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b54b6ef88f..2a5d4b329f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -390,9 +390,8 @@ void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
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,
- Error **errp);
+uint8_t pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
+ uint8_t offset, uint8_t size);
void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index ba4bafac7c..e499482972 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -101,9 +101,8 @@ typedef struct PXBDev PXBDev;
DECLARE_INSTANCE_CHECKER(PXBDev, PXB_CXL_DEV,
TYPE_PXB_CXL_DEVICE)
-int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
- uint16_t svid, uint16_t ssid,
- Error **errp);
+void pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
+ uint16_t svid, uint16_t ssid);
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 798a262a0a..7a35851ae8 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -92,13 +92,12 @@ 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, 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);
+void pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port);
+void pcie_cap_v1_init(PCIDevice *dev, uint8_t offset,
+ uint8_t type, uint8_t port);
+void pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset);
void pcie_cap_exit(PCIDevice *dev);
-int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset);
+void pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset);
void pcie_cap_v1_exit(PCIDevice *dev);
uint8_t pcie_cap_get_type(const PCIDevice *dev);
void pcie_cap_flags_set_vector(PCIDevice *dev, uint8_t vector);
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index d5683b7399..b3be8a889f 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -8,7 +8,7 @@
struct SHPCDevice {
/* Capability offset in device's config space */
- int cap;
+ uint8_t cap;
/* # of hot-pluggable slots */
int nslots;
@@ -38,8 +38,7 @@ 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, Error **errp);
+int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
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/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index 2446dcd9ae..9f3736723c 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -141,7 +141,7 @@ struct VirtIOPCIProxy {
uint32_t msix_bar_idx;
uint32_t modern_io_bar_idx;
uint32_t modern_mem_bar_idx;
- int config_cap;
+ uint8_t config_cap;
uint32_t flags;
bool disable_modern;
bool ignore_backend_features;
--
2.37.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] pci: Abort if pci_add_capability fails
2022-08-29 8:44 [PATCH] pci: Abort if pci_add_capability fails 小田喜陽彦
@ 2022-08-30 11:37 ` Markus Armbruster
2022-08-30 18:00 ` Alex Williamson
0 siblings, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2022-08-30 11:37 UTC (permalink / raw)
To: 小田喜陽彦
Cc: qemu-devel, qemu-block, qemu-arm, Michael S. Tsirkin,
Marcel Apfelbaum, Gerd Hoffmann, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, John Snow, Dmitry Fleytman, Jason Wang,
Stefan Weil, Keith Busch, Klaus Jensen, Peter Maydell,
Andrey Smirnov, Paul Burton, Aleksandar Rikalo, Alex Williamson
Alex, got a question for you below.
小田喜陽彦 <akihiko.odaki@daynix.com> writes:
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> pci_add_capability appears most PCI devices. The error handling required
> lots of code, and led to inconsistent behaviors such as:
> - passing error_abort
> - passing error_fatal
> - asserting the returned value
> - propagating the error to the caller
> - skipping the rest of the function
> - just ignoring
>
> pci_add_capability fails if the new capability overlaps with an existing
> one. It happens only if the device implementation is wrong so
> pci_add_capability can just abort instead of returning to the caller in
> the case, fixing inconsistencies and removing extra code.
It already abort()s when passed a zero @offset and there is not enough
config space left.
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
[...]
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2f450f6a72..5831dfc742 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2513,14 +2513,11 @@ static void pci_del_option_rom(PCIDevice *pdev)
> }
>
> /*
> - * 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.
> + * pci_add_capability() returns a positive value that the offset of the pci
> + * capability.
Suggest "returns the (positive) offset of the PCI capability".
> */
> -int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> - uint8_t offset, uint8_t size,
> - Error **errp)
> +uint8_t pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> + uint8_t offset, uint8_t size)
> {
> uint8_t *config;
> int i, overlapping_cap;
> @@ -2537,13 +2534,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
if (!offset) {
offset = pci_find_space(pdev, size);
/* out of PCI config space is programming error */
assert(offset);
} else {
/* Verify that capabilities don't overlap. Note: device assignment
* depends on this check to verify that the device is not broken.
* Should never trigger for emulated devices, but it's helpful
* for debugging these. */
The comment makes me suspect that device assignment of a broken device
could trigger the error. It goes back to
commit c9abe111209abca1b910e35c6ca9888aced5f183
Author: Jan Kiszka <jan.kiszka@siemens.com>
Date: Wed Aug 24 14:29:30 2011 +0200
pci: Error on PCI capability collisions
Nothing good can happen when we overlap capabilities. This may happen
when plugging in assigned devices or when devices models contain bugs.
Detect the overlap and report it.
Based on qemu-kvm commit by Alex Williamson.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Don Dutile <ddutile@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
If this is still correct, then your patch is a regression: QEMU is no
longer able to gracefully handle assignment of a broken device. Does
this matter? Alex, maybe?
Even if we must avoid this regression, you still make a compelling
argument for *virtual* devices, where pci_add_capability() failure is
always a programming error. Simplest solution: change them all to pass
&error_abort.
Alternatively, add a wrapper that does, then call that. Not sure that's
worth the bother.
> for (i = offset; i < offset + size; i++) {
> overlapping_cap = pci_find_capability_at_offset(pdev, i);
> if (overlapping_cap) {
> - error_setg(errp, "%s:%02x:%02x.%x "
> + error_setg(&error_abort, "%s:%02x:%02x.%x "
> "Attempt to add PCI capability %x at offset "
> "%x overlaps existing capability %x at offset %x",
> pci_root_bus_path(pdev), pci_dev_bus_num(pdev),
> PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> cap_id, offset, overlapping_cap, i);
error.h advises:
* Please don't error_setg(&error_fatal, ...), use error_report() and
* exit(), because that's more obvious.
* Likewise, don't error_setg(&error_abort, ...), use assert().
Thus
assert(!overlapping_cap);
or even
assert(!pci_find_capability_at_offset(pdev, i);
> - return -EINVAL;
> }
> }
> }
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] pci: Abort if pci_add_capability fails
2022-08-30 11:37 ` Markus Armbruster
@ 2022-08-30 18:00 ` Alex Williamson
2022-08-31 8:18 ` Markus Armbruster
0 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2022-08-30 18:00 UTC (permalink / raw)
To: Markus Armbruster
Cc: 小田喜陽彦, qemu-devel, qemu-block,
qemu-arm, Michael S. Tsirkin, Marcel Apfelbaum, Gerd Hoffmann,
Paolo Bonzini, Richard Henderson, Eduardo Habkost, John Snow,
Dmitry Fleytman, Jason Wang, Stefan Weil, Keith Busch,
Klaus Jensen, Peter Maydell, Andrey Smirnov, Paul Burton,
Aleksandar Rikalo
On Tue, 30 Aug 2022 13:37:35 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> if (!offset) {
> offset = pci_find_space(pdev, size);
> /* out of PCI config space is programming error */
> assert(offset);
> } else {
> /* Verify that capabilities don't overlap. Note: device assignment
> * depends on this check to verify that the device is not broken.
> * Should never trigger for emulated devices, but it's helpful
> * for debugging these. */
>
> The comment makes me suspect that device assignment of a broken device
> could trigger the error. It goes back to
>
> commit c9abe111209abca1b910e35c6ca9888aced5f183
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date: Wed Aug 24 14:29:30 2011 +0200
>
> pci: Error on PCI capability collisions
>
> Nothing good can happen when we overlap capabilities. This may happen
> when plugging in assigned devices or when devices models contain bugs.
> Detect the overlap and report it.
>
> Based on qemu-kvm commit by Alex Williamson.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Acked-by: Don Dutile <ddutile@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> If this is still correct, then your patch is a regression: QEMU is no
> longer able to gracefully handle assignment of a broken device. Does
> this matter? Alex, maybe?
Ok, that was a long time ago. I have some vague memories of hitting
something like this with a Broadcom NIC, but a google search for the
error string doesn't turn up anything recently. So there's a fair
chance this wouldn't break anyone initially.
Even back when the above patch was proposed, there were some
suggestions to turn the error path into an abort, which I pushed back
on since clearly enumerating capabilities of a device can occur due to
a hot-plug and we don't necessarily have control of the device being
added. This is only more true with the possibility of soft-devices out
of tree, through things like vfio-user.
Personally I think the right approach is to support an error path such
that we can abort when triggered by a cold-plug device, while simply
rejecting a broken hot-plug device, but that seems to be the minority
opinion among QEMU developers afaict. Thanks,
Alex
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] pci: Abort if pci_add_capability fails
2022-08-30 18:00 ` Alex Williamson
@ 2022-08-31 8:18 ` Markus Armbruster
2022-08-31 12:32 ` Akihiko Odaki
0 siblings, 1 reply; 5+ messages in thread
From: Markus Armbruster @ 2022-08-31 8:18 UTC (permalink / raw)
To: Alex Williamson
Cc: Markus Armbruster, 小田喜陽彦,
qemu-devel, qemu-block, qemu-arm, Michael S. Tsirkin,
Marcel Apfelbaum, Gerd Hoffmann, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, John Snow, Dmitry Fleytman, Jason Wang,
Stefan Weil, Keith Busch, Klaus Jensen, Peter Maydell,
Andrey Smirnov, Paul Burton, Aleksandar Rikalo
Alex Williamson <alex.williamson@redhat.com> writes:
> On Tue, 30 Aug 2022 13:37:35 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>> if (!offset) {
>> offset = pci_find_space(pdev, size);
>> /* out of PCI config space is programming error */
>> assert(offset);
>> } else {
>> /* Verify that capabilities don't overlap. Note: device assignment
>> * depends on this check to verify that the device is not broken.
>> * Should never trigger for emulated devices, but it's helpful
>> * for debugging these. */
>>
>> The comment makes me suspect that device assignment of a broken device
>> could trigger the error. It goes back to
>>
>> commit c9abe111209abca1b910e35c6ca9888aced5f183
>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>> Date: Wed Aug 24 14:29:30 2011 +0200
>>
>> pci: Error on PCI capability collisions
>>
>> Nothing good can happen when we overlap capabilities. This may happen
>> when plugging in assigned devices or when devices models contain bugs.
>> Detect the overlap and report it.
>>
>> Based on qemu-kvm commit by Alex Williamson.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Acked-by: Don Dutile <ddutile@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> If this is still correct, then your patch is a regression: QEMU is no
>> longer able to gracefully handle assignment of a broken device. Does
>> this matter? Alex, maybe?
>
> Ok, that was a long time ago. I have some vague memories of hitting
Indeed!
> something like this with a Broadcom NIC, but a google search for the
> error string doesn't turn up anything recently. So there's a fair
> chance this wouldn't break anyone initially.
I like your careful phrasing.
> Even back when the above patch was proposed, there were some
> suggestions to turn the error path into an abort, which I pushed back
> on since clearly enumerating capabilities of a device can occur due to
> a hot-plug and we don't necessarily have control of the device being
> added. This is only more true with the possibility of soft-devices out
> of tree, through things like vfio-user.
Valid point.
When an compiled-in device model asks for overlapping PCI capabilities,
it's a programming error.
When an assigned device (be it physical or virtual) has overlapping PCI
capabilities, it's bad input.
Abort on programming error is okay. Abort on bad input isn't.
I think callers of the former sort should pass &error_abort to
pci_add_capability(), and callers of the latter sort should handle the
error.
> Personally I think the right approach is to support an error path such
> that we can abort when triggered by a cold-plug device, while simply
> rejecting a broken hot-plug device, but that seems to be the minority
> opinion among QEMU developers afaict. Thanks,
I'm in the "aborting on programming errors is okay" camp.
Recovery from certain programming errors is possible. However,
different programming errors can manifest themselves the same way, and
not all need permit safe recovery.
Say we detect overlapping PCI capabilities when hot-plugging a virtual
device model.
If this is because the programmer passed an incorrect literal offset, we
can recover safely by failing the hot plug.
But perhaps the offset comes from a table, and some other bug scribbled
over it. Continuing the process is *unsafe*, and may increase the
damage and/or obstruct the root cause.
The former kind of bug is unlikely to survive even cursory testing.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] pci: Abort if pci_add_capability fails
2022-08-31 8:18 ` Markus Armbruster
@ 2022-08-31 12:32 ` Akihiko Odaki
0 siblings, 0 replies; 5+ messages in thread
From: Akihiko Odaki @ 2022-08-31 12:32 UTC (permalink / raw)
To: Markus Armbruster
Cc: Alex Williamson, qemu-devel, qemu-block, qemu-arm,
Michael S. Tsirkin, Marcel Apfelbaum, Gerd Hoffmann,
Paolo Bonzini, Richard Henderson, Eduardo Habkost, John Snow,
Dmitry Fleytman, Jason Wang, Stefan Weil, Keith Busch,
Klaus Jensen, Peter Maydell, Andrey Smirnov, Paul Burton,
Aleksandar Rikalo
On Wed, Aug 31, 2022 at 5:18 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Alex Williamson <alex.williamson@redhat.com> writes:
>
> > On Tue, 30 Aug 2022 13:37:35 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >> if (!offset) {
> >> offset = pci_find_space(pdev, size);
> >> /* out of PCI config space is programming error */
> >> assert(offset);
> >> } else {
> >> /* Verify that capabilities don't overlap. Note: device assignment
> >> * depends on this check to verify that the device is not broken.
> >> * Should never trigger for emulated devices, but it's helpful
> >> * for debugging these. */
> >>
> >> The comment makes me suspect that device assignment of a broken device
> >> could trigger the error. It goes back to
> >>
> >> commit c9abe111209abca1b910e35c6ca9888aced5f183
> >> Author: Jan Kiszka <jan.kiszka@siemens.com>
> >> Date: Wed Aug 24 14:29:30 2011 +0200
> >>
> >> pci: Error on PCI capability collisions
> >>
> >> Nothing good can happen when we overlap capabilities. This may happen
> >> when plugging in assigned devices or when devices models contain bugs.
> >> Detect the overlap and report it.
> >>
> >> Based on qemu-kvm commit by Alex Williamson.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Acked-by: Don Dutile <ddutile@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >> If this is still correct, then your patch is a regression: QEMU is no
> >> longer able to gracefully handle assignment of a broken device. Does
> >> this matter? Alex, maybe?
> >
> > Ok, that was a long time ago. I have some vague memories of hitting
>
> Indeed!
>
> > something like this with a Broadcom NIC, but a google search for the
> > error string doesn't turn up anything recently. So there's a fair
> > chance this wouldn't break anyone initially.
>
> I like your careful phrasing.
>
> > Even back when the above patch was proposed, there were some
> > suggestions to turn the error path into an abort, which I pushed back
> > on since clearly enumerating capabilities of a device can occur due to
> > a hot-plug and we don't necessarily have control of the device being
> > added. This is only more true with the possibility of soft-devices out
> > of tree, through things like vfio-user.
>
> Valid point.
>
> When an compiled-in device model asks for overlapping PCI capabilities,
> it's a programming error.
>
> When an assigned device (be it physical or virtual) has overlapping PCI
> capabilities, it's bad input.
>
> Abort on programming error is okay. Abort on bad input isn't.
>
> I think callers of the former sort should pass &error_abort to
> pci_add_capability(), and callers of the latter sort should handle the
> error.
Looking at vfio's implementation, it seems it makes its own effort to
make capabilities not overlap. Taking account of that, I have sent a
new version titled "[PATCH v2] pci: Assert that capabilities never
overlap", which has an updated message and removes the comment in
pci_add_capability().
>
> > Personally I think the right approach is to support an error path such
> > that we can abort when triggered by a cold-plug device, while simply
> > rejecting a broken hot-plug device, but that seems to be the minority
> > opinion among QEMU developers afaict. Thanks,
>
> I'm in the "aborting on programming errors is okay" camp.
>
> Recovery from certain programming errors is possible. However,
> different programming errors can manifest themselves the same way, and
> not all need permit safe recovery.
>
> Say we detect overlapping PCI capabilities when hot-plugging a virtual
> device model.
>
> If this is because the programmer passed an incorrect literal offset, we
> can recover safely by failing the hot plug.
>
> But perhaps the offset comes from a table, and some other bug scribbled
> over it. Continuing the process is *unsafe*, and may increase the
> damage and/or obstruct the root cause.
>
> The former kind of bug is unlikely to survive even cursory testing.
>
Whatever kind of programming error it is, it is dangerous to attempt
to recover from it using an error handling code path untested for a
long period. I think it is better to have some common code to recover
from assertion failures. For example, we may have some option to print
"oops" just like Linux does when an assertion fails and let the user
handle it. That is quite out of scope of this change though.
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-31 12:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-29 8:44 [PATCH] pci: Abort if pci_add_capability fails 小田喜陽彦
2022-08-30 11:37 ` Markus Armbruster
2022-08-30 18:00 ` Alex Williamson
2022-08-31 8:18 ` Markus Armbruster
2022-08-31 12:32 ` Akihiko Odaki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).