Re: [Qemu-devel] [PATCH v16 01/14] vfio: extract vfio_get_hot_reset_info as a single function

[Marcel Apfelbaum]

On 01/12/2016 04:43 AM, Cao jin wrote:
> From: Chen Fan <address@hidden>
>
> the function is used to get affected devices by bus reset.
> so here extract it, and can used for aer soon.
>
> Signed-off-by: Chen Fan <address@hidden>
> ---
>   hw/vfio/pci.c | 66 +++++++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 48 insertions(+), 18 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 1fb868c..efcd3cd 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1654,6 +1654,51 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, 
> uint8_t pos)
>       }
>   }
>
> +/*
> + * return negative with errno, return 0 on success.
> + * if success, the point of ret_info fill with the affected device reset info.
> + *
> + */
> +static int vfio_get_hot_reset_info(VFIOPCIDevice *vdev,
> +                                   struct vfio_pci_hot_reset_info **ret_info)
> +{
> +    struct vfio_pci_hot_reset_info *info;
> +    int ret, count;
> +
> +    *ret_info = NULL;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->argsz = sizeof(*info);
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> +    if (ret && errno != ENOSPC) {
> +        ret = -errno;
> +        goto error;
> +    }
> +
> +    count = info->count;
> +
> +    info = g_realloc(info, sizeof(*info) +
> +                     (count * sizeof(struct vfio_pci_dependent_device)));
> +    info->argsz = sizeof(*info) +
> +                  (count * sizeof(struct vfio_pci_dependent_device));
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> +    if (ret) {
> +        ret = -errno;
> +        error_report("vfio: hot reset info failed: %m");
> +        goto error;
> +    }
> +
> +    *ret_info = info;
> +    info = NULL;
> +
> +    return 0;
> +error:
> +    g_free(info);
> +    return ret;
> +}
> +
>   static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos)
>   {
>       PCIDevice *pdev = &vdev->pdev;
> @@ -1793,7 +1838,7 @@ static bool vfio_pci_host_match(PCIHostDeviceAddress 
> *host1,
>   static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
>   {
>       VFIOGroup *group;
> -    struct vfio_pci_hot_reset_info *info;
> +    struct vfio_pci_hot_reset_info *info = NULL;
>       struct vfio_pci_dependent_device *devices;
>       struct vfio_pci_hot_reset *reset;
>       int32_t *fds;
> @@ -1805,12 +1850,8 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool 
> single)
>       vfio_pci_pre_reset(vdev);
>       vdev->vbasedev.needs_reset = false;
>
> -    info = g_malloc0(sizeof(*info));
> -    info->argsz = sizeof(*info);
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> -    if (ret && errno != ENOSPC) {
> -        ret = -errno;
> +    ret = vfio_get_hot_reset_info(vdev, &info);
> +    if (ret) {
>           if (!vdev->has_pm_reset) {
>               error_report("vfio: Cannot reset device %04x:%02x:%02x.%x, "
>                            "no available reset mechanism.", vdev->host.domain,


Hi,

I don't know how important this is, however if the second call to ioctl
fails (int the new vfio_get_hot_reset_info function) we will get both error
messages, the last one "no available reset mechanism"  being 
unnecessary/(wrong?).

You may want to move the error message to the new function (again, not sure
if it worth doing it)

Thanks,
Marcel

> @@ -1819,18 +1860,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool 
> single)
>           goto out_single;
>       }
>
> -    count = info->count;
> -    info = g_realloc(info, sizeof(*info) + (count * sizeof(*devices)));
> -    info->argsz = sizeof(*info) + (count * sizeof(*devices));
>       devices = &info->devices[0];
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
> -    if (ret) {
> -        ret = -errno;
> -        error_report("vfio: hot reset info failed: %m");
> -        goto out_single;
> -    }
> -
>       trace_vfio_pci_hot_reset_has_dep_devices(vdev->vbasedev.name);
>
>       /* Verify that we have all the groups required */