| Message ID | 20230830103754.36461-6-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Adopt iommufd | expand |
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. > > 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
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;
>-----Original Message----- >From: Eric Auger <eric.auger@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.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? 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
>-----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
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 >
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Thursday, September 21, 2023 4:42 PM >Subject: Re: [PATCH v1 05/22] vfio/common: Extract out >vfio_kvm_device_[add/del]_fd > >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 ? They are purely alias. See below commit: commit da3c22c74a3c6cbd26df40b2f6798a2d41be80ac Author: Thomas Huth <thuth@redhat.com> Date: Tue Sep 12 11:24:40 2023 +0200 linux-headers: Update to Linux v6.6-rc1 This update contains the required header changes for the "target/s390x: AP-passthrough for PV guests" patch from Steffen Eiden. Message-ID: <20230912093432.180041-1-thuth@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h index 1f3f3333a4..0d74ee999a 100644 --- a/linux-headers/linux/kvm.h +++ b/linux-headers/linux/kvm.h @@ -1414,9 +1414,16 @@ struct kvm_device_attr { __u64 addr; /* userspace address of attr data */ }; -#define KVM_DEV_VFIO_GROUP 1 -#define KVM_DEV_VFIO_GROUP_ADD 1 -#define KVM_DEV_VFIO_GROUP_DEL 2 +#define KVM_DEV_VFIO_FILE 1 + +#define KVM_DEV_VFIO_FILE_ADD 1 +#define KVM_DEV_VFIO_FILE_DEL 2 + +/* KVM_DEV_VFIO_GROUP aliases are for compile time uapi compatibility */ +#define KVM_DEV_VFIO_GROUP KVM_DEV_VFIO_FILE + +#define KVM_DEV_VFIO_GROUP_ADD KVM_DEV_VFIO_FILE_ADD +#define KVM_DEV_VFIO_GROUP_DEL KVM_DEV_VFIO_FILE_DEL #define KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE 3 Pre-6.6 kernel not supporting KVM_DEV_VFIO_FILE also not support IOMMUFD. So I think that's fine. Thanks Zhenzhong
On 9/21/23 12:22, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Sent: Thursday, September 21, 2023 4:42 PM >> Subject: Re: [PATCH v1 05/22] vfio/common: Extract out >> vfio_kvm_device_[add/del]_fd >> >> 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 ? > They are purely alias. See below commit: Ah. I missed that. thanks again. C.
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; } 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); +} + +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;