Eric Auger <eric.au...@redhat.com> writes:
> Pass an error object to prepare for migration to VFIO-PCI realize.
> The error is cascaded downto vfio_add_std_cap and then vfio_msi(x)_setup,
> vfio_setup_pcie_cap.
>
> vfio_add_ext_cap does not return anything else than 0 so let's transform
> it into a void function.
>
> Also use pci_add_capability2 which takes an error object.
>
> Signed-off-by: Eric Auger <eric.au...@redhat.com>
> ---
> hw/vfio/pci.c | 66
> ++++++++++++++++++++++++++++++++---------------------------
> 1 file changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index f67eec4..07a44f5 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1178,7 +1178,7 @@ static void vfio_disable_interrupts(VFIOPCIDevice *vdev)
> }
> }
>
> -static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
> +static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> {
> uint16_t ctrl;
> bool msi_64bit, msi_maskbit;
int ret, entries;
Error *err = NULL;
if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
return -errno;
}
Fails without setting error here.
> @@ -1202,8 +1202,7 @@ static int vfio_msi_setup(VFIOPCIDevice *vdev, int pos)
> if (ret == -ENOTSUP) {
> return 0;
> }
> - error_prepend(&err, "vfio: msi_init failed: ");
> - error_report_err(err);
> + error_propagate(errp, err);
> return ret;
> }
> vdev->msi_cap_size = 0xa + (msi_maskbit ? 0xa : 0) + (msi_64bit ? 0x4 :
> 0);
> @@ -1361,7 +1360,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev,
> Error **errp)
> return 0;
> }
>
> -static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
> +static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> {
> int ret;
>
> @@ -1376,7 +1375,7 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos)
> if (ret == -ENOTSUP) {
> return 0;
> }
> - error_report("vfio: msix_init failed");
> + error_setg(errp, "msix_init failed");
> return ret;
> }
>
> @@ -1561,7 +1560,8 @@ static void vfio_add_emulated_long(VFIOPCIDevice *vdev,
> int pos,
> vfio_set_long_bits(vdev->emulated_config_bits + pos, mask, mask);
> }
>
> -static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size)
> +static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
> + Error **errp)
> {
> uint16_t flags;
> uint8_t type;
> @@ -1573,8 +1573,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int
> pos, uint8_t size)
> type != PCI_EXP_TYPE_LEG_END &&
> type != PCI_EXP_TYPE_RC_END) {
>
> - error_report("vfio: Assignment of PCIe type 0x%x "
> - "devices is not currently supported", type);
> + error_setg(errp, "assignment of PCIe type 0x%x "
> + "devices is not currently supported", type);
> return -EINVAL;
> }
>
> @@ -1708,10 +1708,11 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev,
> uint8_t pos)
> }
> }
>
> -static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
> +static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
> {
> PCIDevice *pdev = &vdev->pdev;
> uint8_t cap_id, next, size;
> + Error *err = NULL;
> int ret;
>
> cap_id = pdev->config[pos];
> @@ -1733,9 +1734,9 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev,
> uint8_t pos)
> * will be changed as we unwind the stack.
> */
> if (next) {
> - ret = vfio_add_std_cap(vdev, next);
> + ret = vfio_add_std_cap(vdev, next, &err);
> if (ret) {
> - return ret;
> + goto out;
> }
> } else {
> /* Begin the rebuild, use QEMU emulated list bits */
> @@ -1749,40 +1750,42 @@ static int vfio_add_std_cap(VFIOPCIDevice *vdev,
> uint8_t pos)
>
> switch (cap_id) {
> case PCI_CAP_ID_MSI:
> - ret = vfio_msi_setup(vdev, pos);
> + ret = vfio_msi_setup(vdev, pos, &err);
> break;
> case PCI_CAP_ID_EXP:
> vfio_check_pcie_flr(vdev, pos);
> - ret = vfio_setup_pcie_cap(vdev, pos, size);
> + ret = vfio_setup_pcie_cap(vdev, pos, size, &err);
> break;
> case PCI_CAP_ID_MSIX:
> - ret = vfio_msix_setup(vdev, pos);
> + ret = vfio_msix_setup(vdev, pos, &err);
> break;
> case PCI_CAP_ID_PM:
> vfio_check_pm_reset(vdev, pos);
> vdev->pm_cap = pos;
> - ret = pci_add_capability(pdev, cap_id, pos, size);
> + ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
> break;
> case PCI_CAP_ID_AF:
> vfio_check_af_flr(vdev, pos);
> - ret = pci_add_capability(pdev, cap_id, pos, size);
> + ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
> break;
> default:
> - ret = pci_add_capability(pdev, cap_id, pos, size);
> + ret = pci_add_capability2(pdev, cap_id, pos, size, &err);
> break;
> }
>
> - if (ret < 0) {
> - error_report("vfio: %s Error adding PCI capability "
> - "0x%x[0x%x]@0x%x: %d", vdev->vbasedev.name,
> - cap_id, size, pos, ret);
> - return ret;
> +out:
> + error_propagate(errp, err);
> + if (*errp) {
> + error_prepend(errp,
> + "failed to add PCI capability 0x%x[0x%x]@0x%x: ",
> + cap_id, size, pos);
> }
Less verbose, and therefore recommended by the big comment in error.h is
using errp instead of &err, then
out:
if (ret < 0) {
error_prepend(errp,
"failed to add PCI capability 0x%x[0x%x]@0x%x: ",
cap_id, size, pos);
}
Adding the error_propagate() now anyway can make sense when you get rid
of ret later in the series.
>
> - return 0;
> + return ret < 0 ? ret : 0;
> +
Spurious blank line.
> }
>
> -static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> +static void vfio_add_ext_cap(VFIOPCIDevice *vdev)
> {
> PCIDevice *pdev = &vdev->pdev;
> uint32_t header;
> @@ -1793,7 +1796,7 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> /* Only add extended caps if we have them and the guest can see them */
> if (!pci_is_express(pdev) || !pci_bus_is_express(pdev->bus) ||
> !pci_get_long(pdev->config + PCI_CONFIG_SPACE_SIZE)) {
> - return 0;
> + return;
> }
>
> /*
> @@ -1858,12 +1861,13 @@ static int vfio_add_ext_cap(VFIOPCIDevice *vdev)
> }
>
> g_free(config);
> - return 0;
> + return;
> }
>
> -static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> +static int vfio_add_capabilities(VFIOPCIDevice *vdev, Error **errp)
> {
> PCIDevice *pdev = &vdev->pdev;
> + Error *err = NULL;
> int ret;
>
> if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST) ||
> @@ -1871,12 +1875,14 @@ static int vfio_add_capabilities(VFIOPCIDevice *vdev)
> return 0; /* Nothing to add */
> }
>
> - ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST]);
> + ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], &err);
> if (ret) {
> + error_propagate(errp, err);
> return ret;
> }
Likewise, as long as you have (and intend to keep) ret, do
ret = vfio_add_std_cap(vdev, pdev->config[PCI_CAPABILITY_LIST], errp);
if (ret) {
return ret;
}
>
> - return vfio_add_ext_cap(vdev);
> + vfio_add_ext_cap(vdev);
> + return 0;
> }
>
> static void vfio_pci_pre_reset(VFIOPCIDevice *vdev)
> @@ -2680,7 +2686,7 @@ static int vfio_initfn(PCIDevice *pdev)
>
> vfio_bars_setup(vdev);
>
> - ret = vfio_add_capabilities(vdev);
> + ret = vfio_add_capabilities(vdev, &err);
> if (ret) {
> goto out_teardown;
> }
