Re: [PATCH v1 05/22] vfio/common: Extract out vfio_kvm_device_[add/del]_fd

[Alex Williamson]

On Wed, 30 Aug 2023 18:37:37 +0800
Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:

> ...which will be used by both legacy and iommufd backend.

+1 to Eric's comments regarding complete sentences in the commit log
and suggested description.

> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c              | 44 +++++++++++++++++++++++------------
>  include/hw/vfio/vfio-common.h |  3 +++
>  2 files changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 67150e4575..949ad6714a 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1759,17 +1759,17 @@ void vfio_reset_handler(void *opaque)
>      }
>  }
>  
> -static void vfio_kvm_device_add_group(VFIOGroup *group)
> +int vfio_kvm_device_add_fd(int fd)

Returning int vs void looks gratuitous, nothing uses the return value
in this series.

>  {
>  #ifdef CONFIG_KVM
>      struct kvm_device_attr attr = {
> -        .group = KVM_DEV_VFIO_GROUP,
> -        .attr = KVM_DEV_VFIO_GROUP_ADD,
> -        .addr = (uint64_t)(unsigned long)&group->fd,
> +        .group = KVM_DEV_VFIO_FILE,
> +        .attr = KVM_DEV_VFIO_FILE_ADD,
> +        .addr = (uint64_t)(unsigned long)&fd,
>      };
>  
>      if (!kvm_enabled()) {
> -        return;
> +        return 0;
>      }
>  
>      if (vfio_kvm_device_fd < 0) {
> @@ -1779,37 +1779,51 @@ static void vfio_kvm_device_add_group(VFIOGroup 
> *group)
>  
>          if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {
>              error_report("Failed to create KVM VFIO device: %m");
> -            return;
> +            return -ENODEV;
>          }
>  
>          vfio_kvm_device_fd = cd.fd;
>      }
>  
>      if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) {
> -        error_report("Failed to add group %d to KVM VFIO device: %m",
> -                     group->groupid);
> +        error_report("Failed to add fd %d to KVM VFIO device: %m",
> +                     fd);

It's not nearly as useful to report an fd# in the error log vs the
group#.  Thanks,

Alex

> +        return -errno;
>      }
>  #endif
> +    return 0;
>  }
>  
> -static void vfio_kvm_device_del_group(VFIOGroup *group)
> +static void vfio_kvm_device_add_group(VFIOGroup *group)
> +{
> +    vfio_kvm_device_add_fd(group->fd);
> +}
> +
> +int vfio_kvm_device_del_fd(int fd)
>  {
>  #ifdef CONFIG_KVM
>      struct kvm_device_attr attr = {
> -        .group = KVM_DEV_VFIO_GROUP,
> -        .attr = KVM_DEV_VFIO_GROUP_DEL,
> -        .addr = (uint64_t)(unsigned long)&group->fd,
> +        .group = KVM_DEV_VFIO_FILE,
> +        .attr = KVM_DEV_VFIO_FILE_DEL,
> +        .addr = (uint64_t)(unsigned long)&fd,
>      };
>  
>      if (vfio_kvm_device_fd < 0) {
> -        return;
> +        return -EINVAL;
>      }
>  
>      if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) {
> -        error_report("Failed to remove group %d from KVM VFIO device: %m",
> -                     group->groupid);
> +        error_report("Failed to remove fd %d from KVM VFIO device: %m",
> +                     fd);
> +        return -EBADF;
>      }
>  #endif
> +    return 0;
> +}
> +
> +static void vfio_kvm_device_del_group(VFIOGroup *group)
> +{
> +    vfio_kvm_device_del_fd(group->fd);
>  }
>  
>  static VFIOAddressSpace *vfio_get_address_space(AddressSpace *as)
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 5e376c436e..598c3ce079 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -220,6 +220,9 @@ struct vfio_device_info *vfio_get_device_info(int fd);
>  int vfio_get_device(VFIOGroup *group, const char *name,
>                      VFIODevice *vbasedev, Error **errp);
>  
> +int vfio_kvm_device_add_fd(int fd);
> +int vfio_kvm_device_del_fd(int fd);
> +
>  extern const MemoryRegionOps vfio_region_ops;
>  typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
>  extern VFIOGroupList vfio_group_list;