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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Alex Williamson <alex.williamson@redhat.com>
>Sent: Thursday, September 21, 2023 5:40 AM
>Subject: Re: [PATCH v1 05/22] vfio/common: Extract out
>vfio_kvm_device_[add/del]_fd
>
>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.

Will fix.

>
>>
>> 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.

Will return void.

>
>>  {
>>  #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,

What about checking the return value of vfio_kvm_device_add_fd and
error_report in vfio_kvm_device_add_group. But that will be duplicate
error report. Is that acceptable?

Thanks
Zhenzhong