[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v1 05/22] vfio/common: Extract out vfio_kvm_device_[add/del]_
From: |
Duan, Zhenzhong |
Subject: |
RE: [PATCH v1 05/22] vfio/common: Extract out vfio_kvm_device_[add/del]_fd |
Date: |
Thu, 21 Sep 2023 02:04:14 +0000 |
>-----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