diff mbox

[v2,05/12] vfio/pci: Pass an error object to vfio_add_capabilities

Message ID 1474404352-28958-6-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Eric Auger Sept. 20, 2016, 8:45 p.m. UTC 
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.auger@redhat.com>
---
 hw/vfio/pci.c | 66 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 30 deletions(-)

Comments

Markus Armbruster Sept. 22, 2016, 4:52 p.m. UTC | #1 
Eric Auger <eric.auger@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.auger@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;
>      }
Eric Auger Sept. 30, 2016, 1:33 p.m. UTC | #2 
Hi Markus,

On 22/09/2016 18:52, Markus Armbruster wrote:
> Eric Auger <eric.auger@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.auger@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.
Sure
> 
>> @@ -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
OK thanks for reminding me of this guideline.
> 
>    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.
Yes. So I dared to keep it as is and added a comment in the commit message.

> 
>>  
>> -    return 0;
>> +    return ret < 0 ? ret : 0;
>> +
> 
> Spurious blank line.
OK
> 
>>  }
>>  
>> -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;
>        }
Agreed

Thanks

Eric
> 
>>  
>> -    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;
>>      }
>
diff mbox

Patch

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;
@@ -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);
     }
 
-    return 0;
+    return ret < 0 ? ret : 0;
+
 }
 
-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;
     }
 
-    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;
     }