[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 05/12] vfio/pci: Pass an error object to vfio
From: |
Auger Eric |
Subject: |
Re: [Qemu-devel] [PATCH v2 05/12] vfio/pci: Pass an error object to vfio_add_capabilities |
Date: |
Fri, 30 Sep 2016 15:33:12 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
Hi Markus,
On 22/09/2016 18:52, Markus Armbruster wrote:
> Eric Auger <address@hidden> 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 <address@hidden>
>> ---
>> 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 "
>> - "address@hidden: %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 address@hidden: ",
>> + 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 address@hidden: ",
> 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;
>> }
>
- Re: [Qemu-devel] [PATCH v2 01/12] vfio/pci: Use local error object in vfio_initfn, (continued)
- [Qemu-devel] [PATCH v2 02/12] vfio/pci: Pass an error object to vfio_populate_device, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 03/12] vfio/pci: Pass an error object to vfio_msix_early_setup, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 04/12] vfio/pci: Pass an error object to vfio_intx_enable, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 05/12] vfio/pci: Pass an error object to vfio_add_capabilities, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 06/12] vfio/pci: Pass an error object to vfio_pci_igd_opregion_init, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 07/12] vfio: Pass an error object to vfio_get_group, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 08/12] vfio: Pass an error object to vfio_get_device, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 09/12] vfio/pci: Conversion to realize, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 10/12] vfio/pci: Remove vfio_msix_early_setup returned value, Eric Auger, 2016/09/20
- [Qemu-devel] [PATCH v2 11/12] vfio/pci: Remove vfio_populate_device returned value, Eric Auger, 2016/09/20