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

[Cédric Le Goater]

On 9/20/23 13:49, Eric Auger wrote:
> 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.

Ah yes. I didn't understand why the 's/GROUP/FILE/' change in the
VFIO KVM device ioctls. Thanks for clarifying.

What about pre-6.6 kernels without KVM_DEV_VFIO_FILE support ?

C.


> vfio_kvm_device_add/del_group then call those new helpers.
>
>
>
> > 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)
> >   {
> >   #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?
> >           }
> >   
> >           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.
> > +}
> > +
> > +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.
> >   {
> >   #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?
> >       }
> >   #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;
> Thanks
>
> Eric