qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 01/12] vfio/pci: Use local error object in vf


From: Auger Eric
Subject: Re: [Qemu-devel] [PATCH v2 01/12] vfio/pci: Use local error object in vfio_initfn
Date: Fri, 30 Sep 2016 15:35:49 +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 17:42, Markus Armbruster wrote:
> Eric Auger <address@hidden> writes:
> 
>> To prepare for migration to realize, let's use a local error
>> object in vfio_initfn. Also let's use the same error prefix for all
>> error messages.
>>
>> On top of the 1-1 conversion, we start using a common error prefix for
>> all error messages. We also introduce a similar warning prefix which will
>> be used later on.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>> ---
>>
>> v2: creation
>> ---
>>  hw/vfio/pci.c                 | 73 
>> +++++++++++++++++++++++++------------------
>>  include/hw/vfio/vfio-common.h |  3 ++
>>  2 files changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index a5a620a..ca0293d 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2493,6 +2493,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>      VFIODevice *vbasedev_iter;
>>      VFIOGroup *group;
>>      char *tmp, group_path[PATH_MAX], *group_name;
>> +    Error *err = NULL;
>>      ssize_t len;
>>      struct stat st;
>>      int groupid;
>> @@ -2506,9 +2507,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>      }
>>  
>>      if (stat(vdev->vbasedev.sysfsdev, &st) < 0) {
>> -        error_report("vfio: error: no such host device: %s",
>> -                     vdev->vbasedev.sysfsdev);
>> -        return -errno;
>> +        error_setg(&err, "no such host device");
> 
> Any particular reason you don't use error_setg_errno() here?
I hesitated and eventually removed into thinking it was not bringing
much value. But not strong opinion here, I will restore it.
> 
>> +        ret = -errno;
>> +        goto error;
>>      }
>>  
>>      vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev));
>> @@ -2520,40 +2521,44 @@ static int vfio_initfn(PCIDevice *pdev)
>>      g_free(tmp);
>>  
>>      if (len <= 0 || len >= sizeof(group_path)) {
>> -        error_report("vfio: error no iommu_group for device");
>> -        return len < 0 ? -errno : -ENAMETOOLONG;
>> +        error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
>> +        ret = len < 0 ? -errno : -ENAMETOOLONG;
> 
> Suggest
> 
>            ret = len < 0 ? -errno : -ENAMETOOLONG;
>            error_setg_errno(&err, -ret, "no iommu_group found");
Sure. Eventually ret will disappear so we will come back to
error_setg_errno(&err, len < 0 ? errno : ENAMETOOLONG,
>> +                         "no iommu_group found");
> 
>> +        goto error;
>>      }
>>  
>>      group_path[len] = 0;
>>  
>>      group_name = basename(group_path);
>>      if (sscanf(group_name, "%d", &groupid) != 1) {
>> -        error_report("vfio: error reading %s: %m", group_path);
>> -        return -errno;
>> +        error_setg_errno(&err, errno, "failed to read %s", group_path);
>> +        ret = -errno;
>> +        goto error;
>>      }
>>  
>>      trace_vfio_initfn(vdev->vbasedev.name, groupid);
>>  
>>      group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
>>      if (!group) {
>> -        error_report("vfio: failed to get group %d", groupid);
>> -        return -ENOENT;
>> +        error_setg(&err, "failed to get group %d", groupid);
>> +        ret = -ENOENT;
>> +        goto error;
>>      }
>>  
>>      QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>>          if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
>> -            error_report("vfio: error: device %s is already attached",
>> -                         vdev->vbasedev.name);
>> +            error_setg(&err, "device is already attached");
>>              vfio_put_group(group);
>> -            return -EBUSY;
>> +            ret = -EBUSY;
>> +            goto error;
>>          }
>>      }
>>  
>>      ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev);
>>      if (ret) {
>> -        error_report("vfio: failed to get device %s", vdev->vbasedev.name);
>> +        error_setg_errno(&err, -ret, "failed to get device");
>>          vfio_put_group(group);
>> -        return ret;
>> +        goto error;
>>      }
>>  
>>      ret = vfio_populate_device(vdev);
>> @@ -2567,8 +2572,8 @@ static int vfio_initfn(PCIDevice *pdev)
>>                  vdev->config_offset);
>>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>>          ret = ret < 0 ? -errno : -EFAULT;
>> -        error_report("vfio: Failed to read device config space");
>> -        return ret;
>> +        error_setg_errno(&err, -ret, "failed to read device config space");
>> +        goto error;
>>      }
>>  
>>      /* vfio emulates a lot for us, but some bits need extra love */
>> @@ -2584,8 +2589,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>       */
>>      if (vdev->vendor_id != PCI_ANY_ID) {
>>          if (vdev->vendor_id >= 0xffff) {
>> -            error_report("vfio: Invalid PCI vendor ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI vendor ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0);
>>          trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, 
>> vdev->vendor_id);
>> @@ -2595,8 +2601,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->device_id != PCI_ANY_ID) {
>>          if (vdev->device_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI device ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI device ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0);
>>          trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, 
>> vdev->device_id);
>> @@ -2606,8 +2613,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_vendor_id != PCI_ANY_ID) {
>>          if (vdev->sub_vendor_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI subsystem vendor ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI subsystem vendor ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID,
>>                                 vdev->sub_vendor_id, ~0);
>> @@ -2617,8 +2625,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>  
>>      if (vdev->sub_device_id != PCI_ANY_ID) {
>>          if (vdev->sub_device_id > 0xffff) {
>> -            error_report("vfio: Invalid PCI subsystem device ID provided");
>> -            return -EINVAL;
>> +            error_setg(&err, "invalid PCI subsystem device ID provided");
>> +            ret = -EINVAL;
>> +            goto error;
>>          }
>>          vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, 
>> ~0);
>>          trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name,
>> @@ -2671,8 +2680,9 @@ static int vfio_initfn(PCIDevice *pdev)
>>          struct vfio_region_info *opregion;
>>  
>>          if (vdev->pdev.qdev.hotplugged) {
>> -            error_report("Cannot support IGD OpRegion feature on hotplugged 
>> "
>> -                         "device %s", vdev->vbasedev.name);
>> +            error_setg(&err,
>> +                       "cannot support IGD OpRegion feature on hotplugged "
>> +                       "device");
>>              ret = -EINVAL;
>>              goto out_teardown;
>>          }
>> @@ -2681,16 +2691,15 @@ static int vfio_initfn(PCIDevice *pdev)
>>                          VFIO_REGION_TYPE_PCI_VENDOR_TYPE | 
>> PCI_VENDOR_ID_INTEL,
>>                          VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>>          if (ret) {
>> -            error_report("Device %s does not support requested IGD OpRegion 
>> "
>> -                         "feature", vdev->vbasedev.name);
>> +            error_setg_errno(&err, -ret,
>> +                             "does not support requested IGD OpRegion 
>> feature");
>>              goto out_teardown;
>>          }
>>  
>>          ret = vfio_pci_igd_opregion_init(vdev, opregion);
>>          g_free(opregion);
>>          if (ret) {
>> -            error_report("Device %s IGD OpRegion initialization failed",
>> -                         vdev->vbasedev.name);
>> +            error_setg_errno(&err, -ret, "IGD OpRegion initialization 
>> failed");
>>              goto out_teardown;
>>          }
>>      }
>> @@ -2726,6 +2735,10 @@ out_teardown:
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>      vfio_teardown_msi(vdev);
>>      vfio_bars_exit(vdev);
>> +error:
>> +    if (err) {
>> +        error_reportf_err(err, ERR_PREFIX, vdev->vbasedev.name);
>> +    }
>>      return ret;
>>  }
> 
> The value assigned to ret doesn't matter as long as it's negative, which
> makes me wonder why we bother with different values here.  Peeking
> ahead, I see you simplify this already as part of your conversion to
> realize().  Okay, that works for me.
At that stage it was a 1-1 conversion. Indeed this eventually disappears.

> 
>>  
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 94dfae3..fd19880 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -30,6 +30,9 @@
>>  #include <linux/vfio.h>
>>  #endif
>>  
>> +#define ERR_PREFIX "vfio error: %s: "
>> +#define WARN_PREFIX "vfio warning: %s: "
>> +
>>  /*#define DEBUG_VFIO*/
>>  #ifdef DEBUG_VFIO
>>  #define DPRINTF(fmt, ...) \
> 
> We already discussed error message prefixes and errno strings.  I got my
> taste, you got yours.  Since this is your code, yours wins :)
> 

Here is a sample output. Using qmp interactively you easily get to know
which device BDF produced the error. Also qemu prints the -device option
which lead to the error.

(QEMU) device_add driver=vfio-pci host=0000:01:10.0 bus=head.2 addr=0x14
{"error": {"class": "GenericError", "desc": "vfio error: 0000:01:10.0:
error opening /dev/vfio/6: No such file or directory"}}

qemu-system-aarch64: -device vfio-pci,host=0000:01:10.0: vfio error:
0000:01:10.0: error opening /dev/vfio/6: No such file or directory

However in case of more complex usage (qmp scripts?), might be helpful
to get the BDF directly in the error output?

Thanks

Eric




reply via email to

[Prev in Thread] Current Thread [Next in Thread]