>-----Original Message----- >From: Eric Auger <eric.au...@redhat.com> >Sent: Wednesday, September 20, 2023 7:49 PM >Subject: Re: [PATCH v1 05/22] vfio/common: Extract out >vfio_kvm_device_[add/del]_fd > >Hi Zhenzhong, > >On 8/30/23 12:37, Zhenzhong Duan wrote: >> ...which will be used by both legacy and iommufd backend. >I prefer genuine sentences in the commit msg. Also you explain what you >do but not why. > >suggestion: Introduce two new helpers, vfio_kvm_device_[add/del]_fd >which take as input a file descriptor which can be either a group fd or >a cdev fd. This uses the new KVM_DEV_VFIO_FILE VFIO KVM device group, >which aliases to the legacy KVM_DEV_VFIO_GROUP. > >vfio_kvm_device_add/del_group then call those new helpers.
Thanks, will update in v2. > > > >> >> Signed-off-by: Yi Liu <yi.l....@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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) >> { >> #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; >can't you return -errno? Will fix. >> } >> >> 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); >> + 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); >Since vfio_kvm_device_add_fd now returns an error value, it's a pity not >to use it and propagate it. Also you could fill an errp with the error >msg and use it in vfio_connect_container(). But this is a new error >handling there. What about having vfio_kvm_device_add_fd return void as vfio_kvm_device_add_group. I just realize vfio_connect_container() doesn't get any failure of vfio_kvm_device_add_group, propagating err to vfio_connect_container() is just to print it out there which I have done in vfio_kvm_device_add_fd. >> +} >> + >> +int vfio_kvm_device_del_fd(int fd) >not sure we want this to return an error. But if we do, I think it would >be nicer to propagate the error up. Same question as above. >> { >> #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; >-errno? Sure. Thanks Zhenzhong