| Message ID | 20230926113255.1177834-7-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prerequisite change for IOMMUFD support | expand |
Hi Zhenzhong, On 9/26/23 13:32, Zhenzhong Duan wrote: > From: Eric Auger <eric.auger@redhat.com> > > We want the VFIO devices to be able to use two different > IOMMU backends, the legacy VFIO one and the new iommufd one. > > Introduce vfio_[attach/detach]_device which aim at hiding the > underlying IOMMU backend (IOCTLs, datatypes, ...). > > Once vfio_attach_device completes, the device is attached > to a security context and its fd can be used. Conversely > When vfio_detach_device completes, the device has been > detached from the security context. > > At the moment only the implementation based on the legacy > container/group exists. Let's use it from the vfio-pci device. > Subsequent patches will handle other devices. you may add: no functional change intended > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 3 ++ > hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ > hw/vfio/pci.c | 50 +++----------------------- > hw/vfio/trace-events | 2 +- > 4 files changed, 77 insertions(+), 46 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index c4e7c3b4a7..12fbfbc37d 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); > 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_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp); > +void vfio_detach_device(VFIODevice *vbasedev); > > int vfio_kvm_device_add_fd(int fd, Error **errp); > int vfio_kvm_device_del_fd(int fd, Error **errp); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 959b1362bb..7f3798b152 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) > } > return vfio_eeh_container_op(container, op); > } > + > +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) > +{ > + char *tmp, group_path[PATH_MAX], *group_name; > + int ret, groupid; > + ssize_t len; > + > + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > + len = readlink(tmp, group_path, sizeof(group_path)); > + g_free(tmp); > + > + if (len <= 0 || len >= sizeof(group_path)) { > + ret = len < 0 ? -errno : -ENAMETOOLONG; > + error_setg_errno(errp, -ret, "no iommu_group found"); > + return ret; > + } > + > + group_path[len] = 0; > + > + group_name = basename(group_path); > + if (sscanf(group_name, "%d", &groupid) != 1) { > + error_setg_errno(errp, errno, "failed to read %s", group_path); > + return -errno; > + } > + return groupid; > +} > + > +int vfio_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp) > +{ > + int groupid = vfio_device_groupid(vbasedev, errp); > + VFIODevice *vbasedev_iter; > + VFIOGroup *group; > + int ret; > + > + if (groupid < 0) { > + return groupid; > + } > + > + trace_vfio_attach_device(vbasedev->name, groupid); hum looking at that again, I was confused by the fact we passed the name arg in vfio_attach_device() whereas vbasedev->name was already filled. Looking at pci vfio_realize() both are sometimes different if (!qemu_uuid_is_null(&vdev->vf_token)) { qemu_uuid_unparse(&vdev->vf_token, uuid); name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); } else { name = g_strdup(vbasedev->name); } This may be worth a doc comment. > + > + group = vfio_get_group(groupid, as, errp); > + if (!group) { > + return -ENOENT; > + } > + > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { > + error_setg(errp, "device is already attached"); > + vfio_put_group(group); > + return -EBUSY; > + } > + } > + ret = vfio_get_device(group, name, vbasedev, errp); > + if (ret) { > + vfio_put_group(group); > + } > + > + return ret; > +} > + > +void vfio_detach_device(VFIODevice *vbasedev) > +{ > + VFIOGroup *group = vbasedev->group; > + nit we could add a trace_vfio_detach_device tracepoint here for symetry sake > + vfio_put_base_device(vbasedev); > + vfio_put_group(group); > +} > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 3b2ca3c24c..fe56789893 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > > static void vfio_put_device(VFIOPCIDevice *vdev) > { > + vfio_detach_device(&vdev->vbasedev); > + > g_free(vdev->vbasedev.name); > g_free(vdev->msix); > - > - vfio_put_base_device(&vdev->vbasedev); > } > > static void vfio_err_notifier_handler(void *opaque) > @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > { > VFIOPCIDevice *vdev = VFIO_PCI(pdev); > VFIODevice *vbasedev = &vdev->vbasedev; > - VFIODevice *vbasedev_iter; > - VFIOGroup *group; > - char *tmp, *subsys, group_path[PATH_MAX], *group_name; > + char *tmp, *subsys; > Error *err = NULL; > - ssize_t len; > struct stat st; > - int groupid; > int i, ret; > bool is_mdev; > char uuid[UUID_FMT_LEN]; > @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > vbasedev->type = VFIO_DEVICE_TYPE_PCI; > vbasedev->dev = DEVICE(vdev); > > - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > - len = readlink(tmp, group_path, sizeof(group_path)); > - g_free(tmp); > - > - if (len <= 0 || len >= sizeof(group_path)) { > - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, > - "no iommu_group found"); > - goto error; > - } > - > - group_path[len] = 0; > - > - group_name = basename(group_path); > - if (sscanf(group_name, "%d", &groupid) != 1) { > - error_setg_errno(errp, errno, "failed to read %s", group_path); > - goto error; > - } > - > - trace_vfio_realize(vbasedev->name, groupid); > - > - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), errp); > - if (!group) { > - goto error; > - } > - > - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { > - error_setg(errp, "device is already attached"); > - vfio_put_group(group); > - goto error; > - } > - } > - > /* > * Mediated devices *might* operate compatibly with discarding of RAM, but > * we cannot know for certain, it depends on whether the mdev vendor driver > @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (vbasedev->ram_block_discard_allowed && !is_mdev) { > error_setg(errp, "x-balloon-allowed only potentially compatible " > "with mdev devices"); > - vfio_put_group(group); > goto error; > } > > @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > name = g_strdup(vbasedev->name); > } > > - ret = vfio_get_device(group, name, vbasedev, errp); > + ret = vfio_attach_device(name, vbasedev, > + pci_device_iommu_address_space(pdev), errp); > g_free(name); > if (ret) { > - vfio_put_group(group); > goto error; > } > > @@ -3304,7 +3266,6 @@ error: > static void vfio_instance_finalize(Object *obj) > { > VFIOPCIDevice *vdev = VFIO_PCI(obj); > - VFIOGroup *group = vdev->vbasedev.group; > > vfio_display_finalize(vdev); > vfio_bars_finalize(vdev); > @@ -3318,7 +3279,6 @@ static void vfio_instance_finalize(Object *obj) > * g_free(vdev->igd_opregion); > */ > vfio_put_device(vdev); > - vfio_put_group(group); > } > > static void vfio_exitfn(PCIDevice *pdev) > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index e64ca4a019..e710026a73 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -37,7 +37,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int > vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s" > vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n size: 0x%lx, offset: 0x%lx, flags: 0x%lx" > vfio_populate_device_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s" > -vfio_realize(const char *name, int group_id) " (%s) group %d" > +vfio_attach_device(const char *name, int group_id) " (%s) group %d" > vfio_mdev(const char *name, bool is_mdev) " (%s) is_mdev %d" > vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s 0x%x@0x%x" > vfio_pci_reset(const char *name) " (%s)" Thanks Eric
Hi Zhenzhong, On 9/26/23 13:32, Zhenzhong Duan wrote: > From: Eric Auger <eric.auger@redhat.com> > > We want the VFIO devices to be able to use two different > IOMMU backends, the legacy VFIO one and the new iommufd one. > > Introduce vfio_[attach/detach]_device which aim at hiding the > underlying IOMMU backend (IOCTLs, datatypes, ...). > > Once vfio_attach_device completes, the device is attached > to a security context and its fd can be used. Conversely > When vfio_detach_device completes, the device has been > detached from the security context. > > At the moment only the implementation based on the legacy > container/group exists. Let's use it from the vfio-pci device. > Subsequent patches will handle other devices. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 3 ++ > hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ > hw/vfio/pci.c | 50 +++----------------------- > hw/vfio/trace-events | 2 +- > 4 files changed, 77 insertions(+), 46 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index c4e7c3b4a7..12fbfbc37d 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); > 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_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp); > +void vfio_detach_device(VFIODevice *vbasedev); > > int vfio_kvm_device_add_fd(int fd, Error **errp); > int vfio_kvm_device_del_fd(int fd, Error **errp); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 959b1362bb..7f3798b152 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) > } > return vfio_eeh_container_op(container, op); > } > + > +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) > +{ > + char *tmp, group_path[PATH_MAX], *group_name; > + int ret, groupid; > + ssize_t len; > + > + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > + len = readlink(tmp, group_path, sizeof(group_path)); > + g_free(tmp); > + > + if (len <= 0 || len >= sizeof(group_path)) { > + ret = len < 0 ? -errno : -ENAMETOOLONG; > + error_setg_errno(errp, -ret, "no iommu_group found"); > + return ret; > + } > + > + group_path[len] = 0; > + > + group_name = basename(group_path); > + if (sscanf(group_name, "%d", &groupid) != 1) { > + error_setg_errno(errp, errno, "failed to read %s", group_path); > + return -errno; > + } > + return groupid; > +} > + > +int vfio_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp) > +{ > + int groupid = vfio_device_groupid(vbasedev, errp); > + VFIODevice *vbasedev_iter; > + VFIOGroup *group; > + int ret; > + > + if (groupid < 0) { > + return groupid; > + } > + > + trace_vfio_attach_device(vbasedev->name, groupid); > + > + group = vfio_get_group(groupid, as, errp); > + if (!group) { > + return -ENOENT; > + } > + > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { > + error_setg(errp, "device is already attached"); > + vfio_put_group(group); > + return -EBUSY; > + } > + } > + ret = vfio_get_device(group, name, vbasedev, errp); > + if (ret) { > + vfio_put_group(group); > + } > + > + return ret; > +} > + > +void vfio_detach_device(VFIODevice *vbasedev) > +{ > + VFIOGroup *group = vbasedev->group; > + > + vfio_put_base_device(vbasedev); > + vfio_put_group(group); > +} > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 3b2ca3c24c..fe56789893 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > > static void vfio_put_device(VFIOPCIDevice *vdev) > { > + vfio_detach_device(&vdev->vbasedev); > + > g_free(vdev->vbasedev.name); > g_free(vdev->msix); > - > - vfio_put_base_device(&vdev->vbasedev); > } > > static void vfio_err_notifier_handler(void *opaque) > @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > { > VFIOPCIDevice *vdev = VFIO_PCI(pdev); > VFIODevice *vbasedev = &vdev->vbasedev; > - VFIODevice *vbasedev_iter; > - VFIOGroup *group; > - char *tmp, *subsys, group_path[PATH_MAX], *group_name; > + char *tmp, *subsys; > Error *err = NULL; > - ssize_t len; > struct stat st; > - int groupid; > int i, ret; > bool is_mdev; > char uuid[UUID_FMT_LEN]; > @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > vbasedev->type = VFIO_DEVICE_TYPE_PCI; > vbasedev->dev = DEVICE(vdev); > > - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > - len = readlink(tmp, group_path, sizeof(group_path)); > - g_free(tmp); > - > - if (len <= 0 || len >= sizeof(group_path)) { > - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, > - "no iommu_group found"); > - goto error; > - } > - > - group_path[len] = 0; > - > - group_name = basename(group_path); > - if (sscanf(group_name, "%d", &groupid) != 1) { > - error_setg_errno(errp, errno, "failed to read %s", group_path); > - goto error; > - } > - > - trace_vfio_realize(vbasedev->name, groupid); > - > - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), errp); > - if (!group) { > - goto error; > - } > - > - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { > - error_setg(errp, "device is already attached"); > - vfio_put_group(group); > - goto error; > - } > - } > - > /* > * Mediated devices *might* operate compatibly with discarding of RAM, but > * we cannot know for certain, it depends on whether the mdev vendor driver > @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (vbasedev->ram_block_discard_allowed && !is_mdev) { > error_setg(errp, "x-balloon-allowed only potentially compatible " > "with mdev devices"); > - vfio_put_group(group); > goto error; > } > > @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > name = g_strdup(vbasedev->name); > } > > - ret = vfio_get_device(group, name, vbasedev, errp); > + ret = vfio_attach_device(name, vbasedev, > + pci_device_iommu_address_space(pdev), errp); > g_free(name); > if (ret) { > - vfio_put_group(group); independently on this patch, I think in case of error we leak vbasedev->name. Please have a look and if confirmed you can send a separate patch. Also I think if any subsequent action fail we shoudl properly detach the detach the device so introduce an extra error goto label, my bad sorry. Thanks Eric > goto error; > } > > @@ -3304,7 +3266,6 @@ error: > static void vfio_instance_finalize(Object *obj) > { > VFIOPCIDevice *vdev = VFIO_PCI(obj); > - VFIOGroup *group = vdev->vbasedev.group; > > vfio_display_finalize(vdev); > vfio_bars_finalize(vdev); > @@ -3318,7 +3279,6 @@ static void vfio_instance_finalize(Object *obj) > * g_free(vdev->igd_opregion); > */ > vfio_put_device(vdev); > - vfio_put_group(group); > } > > static void vfio_exitfn(PCIDevice *pdev) > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index e64ca4a019..e710026a73 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -37,7 +37,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int > vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s" > vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n size: 0x%lx, offset: 0x%lx, flags: 0x%lx" > vfio_populate_device_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s" > -vfio_realize(const char *name, int group_id) " (%s) group %d" > +vfio_attach_device(const char *name, int group_id) " (%s) group %d" > vfio_mdev(const char *name, bool is_mdev) " (%s) is_mdev %d" > vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s 0x%x@0x%x" > vfio_pci_reset(const char *name) " (%s)"
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Wednesday, September 27, 2023 3:36 PM >Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device > >Hi Zhenzhong, > >On 9/26/23 13:32, Zhenzhong Duan wrote: >> From: Eric Auger <eric.auger@redhat.com> >> >> We want the VFIO devices to be able to use two different >> IOMMU backends, the legacy VFIO one and the new iommufd one. >> >> Introduce vfio_[attach/detach]_device which aim at hiding the >> underlying IOMMU backend (IOCTLs, datatypes, ...). >> >> Once vfio_attach_device completes, the device is attached >> to a security context and its fd can be used. Conversely >> When vfio_detach_device completes, the device has been >> detached from the security context. >> >> At the moment only the implementation based on the legacy >> container/group exists. Let's use it from the vfio-pci device. >> Subsequent patches will handle other devices. > >you may add: no functional change intended Will do. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 3 ++ >> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >> hw/vfio/pci.c | 50 +++----------------------- >> hw/vfio/trace-events | 2 +- >> 4 files changed, 77 insertions(+), 46 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index c4e7c3b4a7..12fbfbc37d 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >> 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_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp); >> +void vfio_detach_device(VFIODevice *vbasedev); >> >> int vfio_kvm_device_add_fd(int fd, Error **errp); >> int vfio_kvm_device_del_fd(int fd, Error **errp); >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 959b1362bb..7f3798b152 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) >> } >> return vfio_eeh_container_op(container, op); >> } >> + >> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >> +{ >> + char *tmp, group_path[PATH_MAX], *group_name; >> + int ret, groupid; >> + ssize_t len; >> + >> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >> + len = readlink(tmp, group_path, sizeof(group_path)); >> + g_free(tmp); >> + >> + if (len <= 0 || len >= sizeof(group_path)) { >> + ret = len < 0 ? -errno : -ENAMETOOLONG; >> + error_setg_errno(errp, -ret, "no iommu_group found"); >> + return ret; >> + } >> + >> + group_path[len] = 0; >> + >> + group_name = basename(group_path); >> + if (sscanf(group_name, "%d", &groupid) != 1) { >> + error_setg_errno(errp, errno, "failed to read %s", group_path); >> + return -errno; >> + } >> + return groupid; >> +} >> + >> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp) >> +{ >> + int groupid = vfio_device_groupid(vbasedev, errp); >> + VFIODevice *vbasedev_iter; >> + VFIOGroup *group; >> + int ret; >> + >> + if (groupid < 0) { >> + return groupid; >> + } >> + >> + trace_vfio_attach_device(vbasedev->name, groupid); >hum looking at that again, I was confused by the fact we passed the name >arg in > >vfio_attach_device() whereas vbasedev->name was already filled. Looking at pci >vfio_realize() >both are sometimes different > > if (!qemu_uuid_is_null(&vdev->vf_token)) { > qemu_uuid_unparse(&vdev->vf_token, uuid); > name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); > } else { > name = g_strdup(vbasedev->name); > } >This may be worth a doc comment. Yes, agree this is confusing. Just want to ask about the doc comment? Should I create a vfio doc or just a small comment on call site of vfio_attach_device()? Thanks Zhenzhong
On 9/27/23 11:58, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Wednesday, September 27, 2023 3:36 PM >> Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device >> >> Hi Zhenzhong, >> >> On 9/26/23 13:32, Zhenzhong Duan wrote: >>> From: Eric Auger <eric.auger@redhat.com> >>> >>> We want the VFIO devices to be able to use two different >>> IOMMU backends, the legacy VFIO one and the new iommufd one. >>> >>> Introduce vfio_[attach/detach]_device which aim at hiding the >>> underlying IOMMU backend (IOCTLs, datatypes, ...). >>> >>> Once vfio_attach_device completes, the device is attached >>> to a security context and its fd can be used. Conversely >>> When vfio_detach_device completes, the device has been >>> detached from the security context. >>> >>> At the moment only the implementation based on the legacy >>> container/group exists. Let's use it from the vfio-pci device. >>> Subsequent patches will handle other devices. >> you may add: no functional change intended > Will do. > >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 3 ++ >>> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >>> hw/vfio/pci.c | 50 +++----------------------- >>> hw/vfio/trace-events | 2 +- >>> 4 files changed, 77 insertions(+), 46 deletions(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >>> index c4e7c3b4a7..12fbfbc37d 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >>> 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_attach_device(char *name, VFIODevice *vbasedev, >>> + AddressSpace *as, Error **errp); >>> +void vfio_detach_device(VFIODevice *vbasedev); >>> >>> int vfio_kvm_device_add_fd(int fd, Error **errp); >>> int vfio_kvm_device_del_fd(int fd, Error **errp); >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 959b1362bb..7f3798b152 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) >>> } >>> return vfio_eeh_container_op(container, op); >>> } >>> + >>> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >>> +{ >>> + char *tmp, group_path[PATH_MAX], *group_name; >>> + int ret, groupid; >>> + ssize_t len; >>> + >>> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>> + len = readlink(tmp, group_path, sizeof(group_path)); >>> + g_free(tmp); >>> + >>> + if (len <= 0 || len >= sizeof(group_path)) { >>> + ret = len < 0 ? -errno : -ENAMETOOLONG; >>> + error_setg_errno(errp, -ret, "no iommu_group found"); >>> + return ret; >>> + } >>> + >>> + group_path[len] = 0; >>> + >>> + group_name = basename(group_path); >>> + if (sscanf(group_name, "%d", &groupid) != 1) { >>> + error_setg_errno(errp, errno, "failed to read %s", group_path); >>> + return -errno; >>> + } >>> + return groupid; >>> +} >>> + >>> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >>> + AddressSpace *as, Error **errp) >>> +{ >>> + int groupid = vfio_device_groupid(vbasedev, errp); >>> + VFIODevice *vbasedev_iter; >>> + VFIOGroup *group; >>> + int ret; >>> + >>> + if (groupid < 0) { >>> + return groupid; >>> + } >>> + >>> + trace_vfio_attach_device(vbasedev->name, groupid); >> hum looking at that again, I was confused by the fact we passed the name >> arg in >> >> vfio_attach_device() whereas vbasedev->name was already filled. Looking at pci >> vfio_realize() >> both are sometimes different >> >> if (!qemu_uuid_is_null(&vdev->vf_token)) { >> qemu_uuid_unparse(&vdev->vf_token, uuid); >> name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid); >> } else { >> name = g_strdup(vbasedev->name); >> } >> This may be worth a doc comment. > Yes, agree this is confusing. Just want to ask about the doc comment? > Should I create a vfio doc or just a small comment on call site of vfio_attach_device()? I meant a comment associated to this vfio_attach_device helper indicating what is the purpose/semantic of name versus vbasedev->name See my last comment on the ccw conversion which is even more confusing now to me :-( Eric > > Thanks > Zhenzhong
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Wednesday, September 27, 2023 5:02 PM >Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device > >Hi Zhenzhong, > >On 9/26/23 13:32, Zhenzhong Duan wrote: >> From: Eric Auger <eric.auger@redhat.com> >> >> We want the VFIO devices to be able to use two different >> IOMMU backends, the legacy VFIO one and the new iommufd one. >> >> Introduce vfio_[attach/detach]_device which aim at hiding the >> underlying IOMMU backend (IOCTLs, datatypes, ...). >> >> Once vfio_attach_device completes, the device is attached >> to a security context and its fd can be used. Conversely >> When vfio_detach_device completes, the device has been >> detached from the security context. >> >> At the moment only the implementation based on the legacy >> container/group exists. Let's use it from the vfio-pci device. >> Subsequent patches will handle other devices. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 3 ++ >> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >> hw/vfio/pci.c | 50 +++----------------------- >> hw/vfio/trace-events | 2 +- >> 4 files changed, 77 insertions(+), 46 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index c4e7c3b4a7..12fbfbc37d 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >> 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_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp); >> +void vfio_detach_device(VFIODevice *vbasedev); >> >> int vfio_kvm_device_add_fd(int fd, Error **errp); >> int vfio_kvm_device_del_fd(int fd, Error **errp); >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 959b1362bb..7f3798b152 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) >> } >> return vfio_eeh_container_op(container, op); >> } >> + >> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >> +{ >> + char *tmp, group_path[PATH_MAX], *group_name; >> + int ret, groupid; >> + ssize_t len; >> + >> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >> + len = readlink(tmp, group_path, sizeof(group_path)); >> + g_free(tmp); >> + >> + if (len <= 0 || len >= sizeof(group_path)) { >> + ret = len < 0 ? -errno : -ENAMETOOLONG; >> + error_setg_errno(errp, -ret, "no iommu_group found"); >> + return ret; >> + } >> + >> + group_path[len] = 0; >> + >> + group_name = basename(group_path); >> + if (sscanf(group_name, "%d", &groupid) != 1) { >> + error_setg_errno(errp, errno, "failed to read %s", group_path); >> + return -errno; >> + } >> + return groupid; >> +} >> + >> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp) >> +{ >> + int groupid = vfio_device_groupid(vbasedev, errp); >> + VFIODevice *vbasedev_iter; >> + VFIOGroup *group; >> + int ret; >> + >> + if (groupid < 0) { >> + return groupid; >> + } >> + >> + trace_vfio_attach_device(vbasedev->name, groupid); >> + >> + group = vfio_get_group(groupid, as, errp); >> + if (!group) { >> + return -ENOENT; >> + } >> + >> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >> + error_setg(errp, "device is already attached"); >> + vfio_put_group(group); >> + return -EBUSY; >> + } >> + } >> + ret = vfio_get_device(group, name, vbasedev, errp); >> + if (ret) { >> + vfio_put_group(group); >> + } >> + >> + return ret; >> +} >> + >> +void vfio_detach_device(VFIODevice *vbasedev) >> +{ >> + VFIOGroup *group = vbasedev->group; >> + >> + vfio_put_base_device(vbasedev); >> + vfio_put_group(group); >> +} >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 3b2ca3c24c..fe56789893 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice >*vdev, Error **errp) >> >> static void vfio_put_device(VFIOPCIDevice *vdev) >> { >> + vfio_detach_device(&vdev->vbasedev); >> + >> g_free(vdev->vbasedev.name); >> g_free(vdev->msix); >> - >> - vfio_put_base_device(&vdev->vbasedev); >> } >> >> static void vfio_err_notifier_handler(void *opaque) >> @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> { >> VFIOPCIDevice *vdev = VFIO_PCI(pdev); >> VFIODevice *vbasedev = &vdev->vbasedev; >> - VFIODevice *vbasedev_iter; >> - VFIOGroup *group; >> - char *tmp, *subsys, group_path[PATH_MAX], *group_name; >> + char *tmp, *subsys; >> Error *err = NULL; >> - ssize_t len; >> struct stat st; >> - int groupid; >> int i, ret; >> bool is_mdev; >> char uuid[UUID_FMT_LEN]; >> @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> vbasedev->type = VFIO_DEVICE_TYPE_PCI; >> vbasedev->dev = DEVICE(vdev); >> >> - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >> - len = readlink(tmp, group_path, sizeof(group_path)); >> - g_free(tmp); >> - >> - if (len <= 0 || len >= sizeof(group_path)) { >> - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, >> - "no iommu_group found"); >> - goto error; >> - } >> - >> - group_path[len] = 0; >> - >> - group_name = basename(group_path); >> - if (sscanf(group_name, "%d", &groupid) != 1) { >> - error_setg_errno(errp, errno, "failed to read %s", group_path); >> - goto error; >> - } >> - >> - trace_vfio_realize(vbasedev->name, groupid); >> - >> - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), >errp); >> - if (!group) { >> - goto error; >> - } >> - >> - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >> - error_setg(errp, "device is already attached"); >> - vfio_put_group(group); >> - goto error; >> - } >> - } >> - >> /* >> * Mediated devices *might* operate compatibly with discarding of RAM, but >> * we cannot know for certain, it depends on whether the mdev vendor >driver >> @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> if (vbasedev->ram_block_discard_allowed && !is_mdev) { >> error_setg(errp, "x-balloon-allowed only potentially compatible " >> "with mdev devices"); >> - vfio_put_group(group); >> goto error; >> } >> >> @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> name = g_strdup(vbasedev->name); >> } >> >> - ret = vfio_get_device(group, name, vbasedev, errp); >> + ret = vfio_attach_device(name, vbasedev, >> + pci_device_iommu_address_space(pdev), errp); >> g_free(name); >> if (ret) { >> - vfio_put_group(group); >independently on this patch, I think in case of error we leak >vbasedev->name. Please have a look and if confirmed you can send a >separate patch. In case of error, vbasedev->name is freed in vfio_put_device(). > >Also I think if any subsequent action fail we shoudl properly detach the >detach the device so introduce an extra error goto label, my bad sorry. vfio_detach_device is called in vfio_instance_finalize(), this is just to follow the old code, group and container resource allocated in vfio_realize() are freed in vfio_instance_finalize(). I agree this is strange, but I guess there may be some reason I'm unclear. Thanks Zhenzhong
On 9/26/23 13:32, Zhenzhong Duan wrote: > From: Eric Auger <eric.auger@redhat.com> > > We want the VFIO devices to be able to use two different > IOMMU backends, the legacy VFIO one and the new iommufd one. > > Introduce vfio_[attach/detach]_device which aim at hiding the > underlying IOMMU backend (IOCTLs, datatypes, ...). > > Once vfio_attach_device completes, the device is attached > to a security context and its fd can be used. Conversely > When vfio_detach_device completes, the device has been > detached from the security context. > > At the moment only the implementation based on the legacy > container/group exists. Let's use it from the vfio-pci device. > Subsequent patches will handle other devices. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 3 ++ > hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ > hw/vfio/pci.c | 50 +++----------------------- > hw/vfio/trace-events | 2 +- > 4 files changed, 77 insertions(+), 46 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index c4e7c3b4a7..12fbfbc37d 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); > 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_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp); > +void vfio_detach_device(VFIODevice *vbasedev); > > int vfio_kvm_device_add_fd(int fd, Error **errp); > int vfio_kvm_device_del_fd(int fd, Error **errp); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 959b1362bb..7f3798b152 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) > } > return vfio_eeh_container_op(container, op); > } > + > +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) > +{ > + char *tmp, group_path[PATH_MAX], *group_name; > + int ret, groupid; > + ssize_t len; > + > + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > + len = readlink(tmp, group_path, sizeof(group_path)); > + g_free(tmp); > + > + if (len <= 0 || len >= sizeof(group_path)) { > + ret = len < 0 ? -errno : -ENAMETOOLONG; > + error_setg_errno(errp, -ret, "no iommu_group found"); > + return ret; > + } > + > + group_path[len] = 0; > + > + group_name = basename(group_path); > + if (sscanf(group_name, "%d", &groupid) != 1) { > + error_setg_errno(errp, errno, "failed to read %s", group_path); > + return -errno; > + } > + return groupid; > +} > + > +int vfio_attach_device(char *name, VFIODevice *vbasedev, > + AddressSpace *as, Error **errp) > +{ > + int groupid = vfio_device_groupid(vbasedev, errp); > + VFIODevice *vbasedev_iter; > + VFIOGroup *group; > + int ret; > + > + if (groupid < 0) { > + return groupid; > + } > + > + trace_vfio_attach_device(vbasedev->name, groupid); > + > + group = vfio_get_group(groupid, as, errp); > + if (!group) { > + return -ENOENT; > + } > + > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { > + error_setg(errp, "device is already attached"); > + vfio_put_group(group); > + return -EBUSY; > + } > + } > + ret = vfio_get_device(group, name, vbasedev, errp); > + if (ret) { > + vfio_put_group(group); > + } > + > + return ret; > +} > + > +void vfio_detach_device(VFIODevice *vbasedev) > +{ > + VFIOGroup *group = vbasedev->group; > + > + vfio_put_base_device(vbasedev); > + vfio_put_group(group); > +} > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 3b2ca3c24c..fe56789893 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > > static void vfio_put_device(VFIOPCIDevice *vdev) > { > + vfio_detach_device(&vdev->vbasedev); > + > g_free(vdev->vbasedev.name); > g_free(vdev->msix); > - > - vfio_put_base_device(&vdev->vbasedev); > } > > static void vfio_err_notifier_handler(void *opaque) > @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > { > VFIOPCIDevice *vdev = VFIO_PCI(pdev); > VFIODevice *vbasedev = &vdev->vbasedev; > - VFIODevice *vbasedev_iter; > - VFIOGroup *group; > - char *tmp, *subsys, group_path[PATH_MAX], *group_name; > + char *tmp, *subsys; > Error *err = NULL; > - ssize_t len; > struct stat st; > - int groupid; > int i, ret; > bool is_mdev; > char uuid[UUID_FMT_LEN]; > @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > vbasedev->type = VFIO_DEVICE_TYPE_PCI; > vbasedev->dev = DEVICE(vdev); > > - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > - len = readlink(tmp, group_path, sizeof(group_path)); > - g_free(tmp); > - > - if (len <= 0 || len >= sizeof(group_path)) { > - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, > - "no iommu_group found"); > - goto error; > - } > - > - group_path[len] = 0; > - > - group_name = basename(group_path); > - if (sscanf(group_name, "%d", &groupid) != 1) { > - error_setg_errno(errp, errno, "failed to read %s", group_path); > - goto error; > - } > - > - trace_vfio_realize(vbasedev->name, groupid); > - > - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), errp); > - if (!group) { > - goto error; > - } > - > - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { > - error_setg(errp, "device is already attached"); > - vfio_put_group(group); > - goto error; > - } > - } > - > /* > * Mediated devices *might* operate compatibly with discarding of RAM, but > * we cannot know for certain, it depends on whether the mdev vendor driver > @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > if (vbasedev->ram_block_discard_allowed && !is_mdev) { > error_setg(errp, "x-balloon-allowed only potentially compatible " > "with mdev devices"); > - vfio_put_group(group); > goto error; > } > > @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > name = g_strdup(vbasedev->name); > } > > - ret = vfio_get_device(group, name, vbasedev, errp); > + ret = vfio_attach_device(name, vbasedev, > + pci_device_iommu_address_space(pdev), errp); > g_free(name); > if (ret) { > - vfio_put_group(group); > goto error; > } > > @@ -3304,7 +3266,6 @@ error: > static void vfio_instance_finalize(Object *obj) > { > VFIOPCIDevice *vdev = VFIO_PCI(obj); > - VFIOGroup *group = vdev->vbasedev.group; > > vfio_display_finalize(vdev); > vfio_bars_finalize(vdev); > @@ -3318,7 +3279,6 @@ static void vfio_instance_finalize(Object *obj) > * g_free(vdev->igd_opregion); > */ > vfio_put_device(vdev); Please rebase v2 on vfio-next. I merged your patch since :) Thanks, C. > - vfio_put_group(group); > } > > static void vfio_exitfn(PCIDevice *pdev) > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index e64ca4a019..e710026a73 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -37,7 +37,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int > vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s" > vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n size: 0x%lx, offset: 0x%lx, flags: 0x%lx" > vfio_populate_device_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s" > -vfio_realize(const char *name, int group_id) " (%s) group %d" > +vfio_attach_device(const char *name, int group_id) " (%s) group %d" > vfio_mdev(const char *name, bool is_mdev) " (%s) is_mdev %d" > vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s 0x%x@0x%x" > vfio_pci_reset(const char *name) " (%s)"
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Wednesday, September 27, 2023 6:17 PM >Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device > >On 9/26/23 13:32, Zhenzhong Duan wrote: >> From: Eric Auger <eric.auger@redhat.com> >> >> We want the VFIO devices to be able to use two different >> IOMMU backends, the legacy VFIO one and the new iommufd one. >> >> Introduce vfio_[attach/detach]_device which aim at hiding the >> underlying IOMMU backend (IOCTLs, datatypes, ...). >> >> Once vfio_attach_device completes, the device is attached >> to a security context and its fd can be used. Conversely >> When vfio_detach_device completes, the device has been >> detached from the security context. >> >> At the moment only the implementation based on the legacy >> container/group exists. Let's use it from the vfio-pci device. >> Subsequent patches will handle other devices. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 3 ++ >> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >> hw/vfio/pci.c | 50 +++----------------------- >> hw/vfio/trace-events | 2 +- >> 4 files changed, 77 insertions(+), 46 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index c4e7c3b4a7..12fbfbc37d 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >> 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_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp); >> +void vfio_detach_device(VFIODevice *vbasedev); >> >> int vfio_kvm_device_add_fd(int fd, Error **errp); >> int vfio_kvm_device_del_fd(int fd, Error **errp); >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 959b1362bb..7f3798b152 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) >> } >> return vfio_eeh_container_op(container, op); >> } >> + >> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >> +{ >> + char *tmp, group_path[PATH_MAX], *group_name; >> + int ret, groupid; >> + ssize_t len; >> + >> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >> + len = readlink(tmp, group_path, sizeof(group_path)); >> + g_free(tmp); >> + >> + if (len <= 0 || len >= sizeof(group_path)) { >> + ret = len < 0 ? -errno : -ENAMETOOLONG; >> + error_setg_errno(errp, -ret, "no iommu_group found"); >> + return ret; >> + } >> + >> + group_path[len] = 0; >> + >> + group_name = basename(group_path); >> + if (sscanf(group_name, "%d", &groupid) != 1) { >> + error_setg_errno(errp, errno, "failed to read %s", group_path); >> + return -errno; >> + } >> + return groupid; >> +} >> + >> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >> + AddressSpace *as, Error **errp) >> +{ >> + int groupid = vfio_device_groupid(vbasedev, errp); >> + VFIODevice *vbasedev_iter; >> + VFIOGroup *group; >> + int ret; >> + >> + if (groupid < 0) { >> + return groupid; >> + } >> + >> + trace_vfio_attach_device(vbasedev->name, groupid); >> + >> + group = vfio_get_group(groupid, as, errp); >> + if (!group) { >> + return -ENOENT; >> + } >> + >> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >> + error_setg(errp, "device is already attached"); >> + vfio_put_group(group); >> + return -EBUSY; >> + } >> + } >> + ret = vfio_get_device(group, name, vbasedev, errp); >> + if (ret) { >> + vfio_put_group(group); >> + } >> + >> + return ret; >> +} >> + >> +void vfio_detach_device(VFIODevice *vbasedev) >> +{ >> + VFIOGroup *group = vbasedev->group; >> + >> + vfio_put_base_device(vbasedev); >> + vfio_put_group(group); >> +} >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 3b2ca3c24c..fe56789893 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice >*vdev, Error **errp) >> >> static void vfio_put_device(VFIOPCIDevice *vdev) >> { >> + vfio_detach_device(&vdev->vbasedev); >> + >> g_free(vdev->vbasedev.name); >> g_free(vdev->msix); >> - >> - vfio_put_base_device(&vdev->vbasedev); >> } >> >> static void vfio_err_notifier_handler(void *opaque) >> @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> { >> VFIOPCIDevice *vdev = VFIO_PCI(pdev); >> VFIODevice *vbasedev = &vdev->vbasedev; >> - VFIODevice *vbasedev_iter; >> - VFIOGroup *group; >> - char *tmp, *subsys, group_path[PATH_MAX], *group_name; >> + char *tmp, *subsys; >> Error *err = NULL; >> - ssize_t len; >> struct stat st; >> - int groupid; >> int i, ret; >> bool is_mdev; >> char uuid[UUID_FMT_LEN]; >> @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> vbasedev->type = VFIO_DEVICE_TYPE_PCI; >> vbasedev->dev = DEVICE(vdev); >> >> - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >> - len = readlink(tmp, group_path, sizeof(group_path)); >> - g_free(tmp); >> - >> - if (len <= 0 || len >= sizeof(group_path)) { >> - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, >> - "no iommu_group found"); >> - goto error; >> - } >> - >> - group_path[len] = 0; >> - >> - group_name = basename(group_path); >> - if (sscanf(group_name, "%d", &groupid) != 1) { >> - error_setg_errno(errp, errno, "failed to read %s", group_path); >> - goto error; >> - } >> - >> - trace_vfio_realize(vbasedev->name, groupid); >> - >> - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), >errp); >> - if (!group) { >> - goto error; >> - } >> - >> - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >> - error_setg(errp, "device is already attached"); >> - vfio_put_group(group); >> - goto error; >> - } >> - } >> - >> /* >> * Mediated devices *might* operate compatibly with discarding of RAM, >but >> * we cannot know for certain, it depends on whether the mdev vendor >driver >> @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> if (vbasedev->ram_block_discard_allowed && !is_mdev) { >> error_setg(errp, "x-balloon-allowed only potentially compatible " >> "with mdev devices"); >> - vfio_put_group(group); >> goto error; >> } >> >> @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> name = g_strdup(vbasedev->name); >> } >> >> - ret = vfio_get_device(group, name, vbasedev, errp); >> + ret = vfio_attach_device(name, vbasedev, >> + pci_device_iommu_address_space(pdev), errp); >> g_free(name); >> if (ret) { >> - vfio_put_group(group); >> goto error; >> } >> >> @@ -3304,7 +3266,6 @@ error: >> static void vfio_instance_finalize(Object *obj) >> { >> VFIOPCIDevice *vdev = VFIO_PCI(obj); >> - VFIOGroup *group = vdev->vbasedev.group; >> >> vfio_display_finalize(vdev); >> vfio_bars_finalize(vdev); >> @@ -3318,7 +3279,6 @@ static void vfio_instance_finalize(Object *obj) >> * g_free(vdev->igd_opregion); >> */ >> vfio_put_device(vdev); > >Please rebase v2 on vfio-next. I merged your patch since :) Sure, will do. Maybe will be a little late. We will have national holiday between Sep 29 and Oct 6. Thanks Zhenzhong
On 9/27/23 12:07, Duan, Zhenzhong wrote: > Hi Eric, > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Wednesday, September 27, 2023 5:02 PM >> Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device >> >> Hi Zhenzhong, >> >> On 9/26/23 13:32, Zhenzhong Duan wrote: >>> From: Eric Auger <eric.auger@redhat.com> >>> >>> We want the VFIO devices to be able to use two different >>> IOMMU backends, the legacy VFIO one and the new iommufd one. >>> >>> Introduce vfio_[attach/detach]_device which aim at hiding the >>> underlying IOMMU backend (IOCTLs, datatypes, ...). >>> >>> Once vfio_attach_device completes, the device is attached >>> to a security context and its fd can be used. Conversely >>> When vfio_detach_device completes, the device has been >>> detached from the security context. >>> >>> At the moment only the implementation based on the legacy >>> container/group exists. Let's use it from the vfio-pci device. >>> Subsequent patches will handle other devices. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 3 ++ >>> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >>> hw/vfio/pci.c | 50 +++----------------------- >>> hw/vfio/trace-events | 2 +- >>> 4 files changed, 77 insertions(+), 46 deletions(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >>> index c4e7c3b4a7..12fbfbc37d 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >>> 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_attach_device(char *name, VFIODevice *vbasedev, >>> + AddressSpace *as, Error **errp); >>> +void vfio_detach_device(VFIODevice *vbasedev); >>> >>> int vfio_kvm_device_add_fd(int fd, Error **errp); >>> int vfio_kvm_device_del_fd(int fd, Error **errp); >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 959b1362bb..7f3798b152 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) >>> } >>> return vfio_eeh_container_op(container, op); >>> } >>> + >>> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >>> +{ >>> + char *tmp, group_path[PATH_MAX], *group_name; >>> + int ret, groupid; >>> + ssize_t len; >>> + >>> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>> + len = readlink(tmp, group_path, sizeof(group_path)); >>> + g_free(tmp); >>> + >>> + if (len <= 0 || len >= sizeof(group_path)) { >>> + ret = len < 0 ? -errno : -ENAMETOOLONG; >>> + error_setg_errno(errp, -ret, "no iommu_group found"); >>> + return ret; >>> + } >>> + >>> + group_path[len] = 0; >>> + >>> + group_name = basename(group_path); >>> + if (sscanf(group_name, "%d", &groupid) != 1) { >>> + error_setg_errno(errp, errno, "failed to read %s", group_path); >>> + return -errno; >>> + } >>> + return groupid; >>> +} >>> + >>> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >>> + AddressSpace *as, Error **errp) >>> +{ >>> + int groupid = vfio_device_groupid(vbasedev, errp); >>> + VFIODevice *vbasedev_iter; >>> + VFIOGroup *group; >>> + int ret; >>> + >>> + if (groupid < 0) { >>> + return groupid; >>> + } >>> + >>> + trace_vfio_attach_device(vbasedev->name, groupid); >>> + >>> + group = vfio_get_group(groupid, as, errp); >>> + if (!group) { >>> + return -ENOENT; >>> + } >>> + >>> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>> + error_setg(errp, "device is already attached"); >>> + vfio_put_group(group); >>> + return -EBUSY; >>> + } >>> + } >>> + ret = vfio_get_device(group, name, vbasedev, errp); >>> + if (ret) { >>> + vfio_put_group(group); >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +void vfio_detach_device(VFIODevice *vbasedev) >>> +{ >>> + VFIOGroup *group = vbasedev->group; >>> + >>> + vfio_put_base_device(vbasedev); >>> + vfio_put_group(group); >>> +} >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index 3b2ca3c24c..fe56789893 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice >> *vdev, Error **errp) >>> static void vfio_put_device(VFIOPCIDevice *vdev) >>> { >>> + vfio_detach_device(&vdev->vbasedev); >>> + >>> g_free(vdev->vbasedev.name); >>> g_free(vdev->msix); >>> - >>> - vfio_put_base_device(&vdev->vbasedev); >>> } >>> >>> static void vfio_err_notifier_handler(void *opaque) >>> @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >>> { >>> VFIOPCIDevice *vdev = VFIO_PCI(pdev); >>> VFIODevice *vbasedev = &vdev->vbasedev; >>> - VFIODevice *vbasedev_iter; >>> - VFIOGroup *group; >>> - char *tmp, *subsys, group_path[PATH_MAX], *group_name; >>> + char *tmp, *subsys; >>> Error *err = NULL; >>> - ssize_t len; >>> struct stat st; >>> - int groupid; >>> int i, ret; >>> bool is_mdev; >>> char uuid[UUID_FMT_LEN]; >>> @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >>> vbasedev->type = VFIO_DEVICE_TYPE_PCI; >>> vbasedev->dev = DEVICE(vdev); >>> >>> - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>> - len = readlink(tmp, group_path, sizeof(group_path)); >>> - g_free(tmp); >>> - >>> - if (len <= 0 || len >= sizeof(group_path)) { >>> - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, >>> - "no iommu_group found"); >>> - goto error; >>> - } >>> - >>> - group_path[len] = 0; >>> - >>> - group_name = basename(group_path); >>> - if (sscanf(group_name, "%d", &groupid) != 1) { >>> - error_setg_errno(errp, errno, "failed to read %s", group_path); >>> - goto error; >>> - } >>> - >>> - trace_vfio_realize(vbasedev->name, groupid); >>> - >>> - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), >> errp); >>> - if (!group) { >>> - goto error; >>> - } >>> - >>> - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>> - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>> - error_setg(errp, "device is already attached"); >>> - vfio_put_group(group); >>> - goto error; >>> - } >>> - } >>> - >>> /* >>> * Mediated devices *might* operate compatibly with discarding of RAM, but >>> * we cannot know for certain, it depends on whether the mdev vendor >> driver >>> @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >>> if (vbasedev->ram_block_discard_allowed && !is_mdev) { >>> error_setg(errp, "x-balloon-allowed only potentially compatible " >>> "with mdev devices"); >>> - vfio_put_group(group); >>> goto error; >>> } >>> >>> @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >>> name = g_strdup(vbasedev->name); >>> } >>> >>> - ret = vfio_get_device(group, name, vbasedev, errp); >>> + ret = vfio_attach_device(name, vbasedev, >>> + pci_device_iommu_address_space(pdev), errp); >>> g_free(name); >>> if (ret) { >>> - vfio_put_group(group); >> independently on this patch, I think in case of error we leak >> vbasedev->name. Please have a look and if confirmed you can send a >> separate patch. > In case of error, vbasedev->name is freed in vfio_put_device(). called in vfio_instance_finalize only. See comment below > >> Also I think if any subsequent action fail we shoudl properly detach the >> detach the device so introduce an extra error goto label, my bad sorry. > vfio_detach_device is called in vfio_instance_finalize(), this is just to follow > the old code, group and container resource allocated in vfio_realize() are > freed in vfio_instance_finalize(). > > I agree this is strange, but I guess there may be some reason I'm unclear. Yeah but if vfio_realize does fail, I am not sure the vfio_instance_finalize gets called Eric > > Thanks > Zhenzhong >
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Wednesday, September 27, 2023 8:23 PM >Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device > > > >On 9/27/23 12:07, Duan, Zhenzhong wrote: >> Hi Eric, >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Sent: Wednesday, September 27, 2023 5:02 PM >>> Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device >>> >>> Hi Zhenzhong, >>> >>> On 9/26/23 13:32, Zhenzhong Duan wrote: >>>> From: Eric Auger <eric.auger@redhat.com> >>>> >>>> We want the VFIO devices to be able to use two different >>>> IOMMU backends, the legacy VFIO one and the new iommufd one. >>>> >>>> Introduce vfio_[attach/detach]_device which aim at hiding the >>>> underlying IOMMU backend (IOCTLs, datatypes, ...). >>>> >>>> Once vfio_attach_device completes, the device is attached >>>> to a security context and its fd can be used. Conversely >>>> When vfio_detach_device completes, the device has been >>>> detached from the security context. >>>> >>>> At the moment only the implementation based on the legacy >>>> container/group exists. Let's use it from the vfio-pci device. >>>> Subsequent patches will handle other devices. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 3 ++ >>>> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >>>> hw/vfio/pci.c | 50 +++----------------------- >>>> hw/vfio/trace-events | 2 +- >>>> 4 files changed, 77 insertions(+), 46 deletions(-) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >>>> index c4e7c3b4a7..12fbfbc37d 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >>>> 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_attach_device(char *name, VFIODevice *vbasedev, >>>> + AddressSpace *as, Error **errp); >>>> +void vfio_detach_device(VFIODevice *vbasedev); >>>> >>>> int vfio_kvm_device_add_fd(int fd, Error **errp); >>>> int vfio_kvm_device_del_fd(int fd, Error **errp); >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>> index 959b1362bb..7f3798b152 100644 >>>> --- a/hw/vfio/common.c >>>> +++ b/hw/vfio/common.c >>>> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t >op) >>>> } >>>> return vfio_eeh_container_op(container, op); >>>> } >>>> + >>>> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >>>> +{ >>>> + char *tmp, group_path[PATH_MAX], *group_name; >>>> + int ret, groupid; >>>> + ssize_t len; >>>> + >>>> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>>> + len = readlink(tmp, group_path, sizeof(group_path)); >>>> + g_free(tmp); >>>> + >>>> + if (len <= 0 || len >= sizeof(group_path)) { >>>> + ret = len < 0 ? -errno : -ENAMETOOLONG; >>>> + error_setg_errno(errp, -ret, "no iommu_group found"); >>>> + return ret; >>>> + } >>>> + >>>> + group_path[len] = 0; >>>> + >>>> + group_name = basename(group_path); >>>> + if (sscanf(group_name, "%d", &groupid) != 1) { >>>> + error_setg_errno(errp, errno, "failed to read %s", group_path); >>>> + return -errno; >>>> + } >>>> + return groupid; >>>> +} >>>> + >>>> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >>>> + AddressSpace *as, Error **errp) >>>> +{ >>>> + int groupid = vfio_device_groupid(vbasedev, errp); >>>> + VFIODevice *vbasedev_iter; >>>> + VFIOGroup *group; >>>> + int ret; >>>> + >>>> + if (groupid < 0) { >>>> + return groupid; >>>> + } >>>> + >>>> + trace_vfio_attach_device(vbasedev->name, groupid); >>>> + >>>> + group = vfio_get_group(groupid, as, errp); >>>> + if (!group) { >>>> + return -ENOENT; >>>> + } >>>> + >>>> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>>> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>>> + error_setg(errp, "device is already attached"); >>>> + vfio_put_group(group); >>>> + return -EBUSY; >>>> + } >>>> + } >>>> + ret = vfio_get_device(group, name, vbasedev, errp); >>>> + if (ret) { >>>> + vfio_put_group(group); >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +void vfio_detach_device(VFIODevice *vbasedev) >>>> +{ >>>> + VFIOGroup *group = vbasedev->group; >>>> + >>>> + vfio_put_base_device(vbasedev); >>>> + vfio_put_group(group); >>>> +} >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index 3b2ca3c24c..fe56789893 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice >>> *vdev, Error **errp) >>>> static void vfio_put_device(VFIOPCIDevice *vdev) >>>> { >>>> + vfio_detach_device(&vdev->vbasedev); >>>> + >>>> g_free(vdev->vbasedev.name); >>>> g_free(vdev->msix); >>>> - >>>> - vfio_put_base_device(&vdev->vbasedev); >>>> } >>>> >>>> static void vfio_err_notifier_handler(void *opaque) >>>> @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error >>> **errp) >>>> { >>>> VFIOPCIDevice *vdev = VFIO_PCI(pdev); >>>> VFIODevice *vbasedev = &vdev->vbasedev; >>>> - VFIODevice *vbasedev_iter; >>>> - VFIOGroup *group; >>>> - char *tmp, *subsys, group_path[PATH_MAX], *group_name; >>>> + char *tmp, *subsys; >>>> Error *err = NULL; >>>> - ssize_t len; >>>> struct stat st; >>>> - int groupid; >>>> int i, ret; >>>> bool is_mdev; >>>> char uuid[UUID_FMT_LEN]; >>>> @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error >>> **errp) >>>> vbasedev->type = VFIO_DEVICE_TYPE_PCI; >>>> vbasedev->dev = DEVICE(vdev); >>>> >>>> - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>>> - len = readlink(tmp, group_path, sizeof(group_path)); >>>> - g_free(tmp); >>>> - >>>> - if (len <= 0 || len >= sizeof(group_path)) { >>>> - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, >>>> - "no iommu_group found"); >>>> - goto error; >>>> - } >>>> - >>>> - group_path[len] = 0; >>>> - >>>> - group_name = basename(group_path); >>>> - if (sscanf(group_name, "%d", &groupid) != 1) { >>>> - error_setg_errno(errp, errno, "failed to read %s", group_path); >>>> - goto error; >>>> - } >>>> - >>>> - trace_vfio_realize(vbasedev->name, groupid); >>>> - >>>> - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), >>> errp); >>>> - if (!group) { >>>> - goto error; >>>> - } >>>> - >>>> - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>>> - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>>> - error_setg(errp, "device is already attached"); >>>> - vfio_put_group(group); >>>> - goto error; >>>> - } >>>> - } >>>> - >>>> /* >>>> * Mediated devices *might* operate compatibly with discarding of RAM, >but >>>> * we cannot know for certain, it depends on whether the mdev vendor >>> driver >>>> @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >>>> if (vbasedev->ram_block_discard_allowed && !is_mdev) { >>>> error_setg(errp, "x-balloon-allowed only potentially compatible " >>>> "with mdev devices"); >>>> - vfio_put_group(group); >>>> goto error; >>>> } >>>> >>>> @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error >>> **errp) >>>> name = g_strdup(vbasedev->name); >>>> } >>>> >>>> - ret = vfio_get_device(group, name, vbasedev, errp); >>>> + ret = vfio_attach_device(name, vbasedev, >>>> + pci_device_iommu_address_space(pdev), errp); >>>> g_free(name); >>>> if (ret) { >>>> - vfio_put_group(group); >>> independently on this patch, I think in case of error we leak >>> vbasedev->name. Please have a look and if confirmed you can send a >>> separate patch. >> In case of error, vbasedev->name is freed in vfio_put_device(). >called in vfio_instance_finalize only. See comment below >> >>> Also I think if any subsequent action fail we shoudl properly detach the >>> detach the device so introduce an extra error goto label, my bad sorry. >> vfio_detach_device is called in vfio_instance_finalize(), this is just to follow >> the old code, group and container resource allocated in vfio_realize() are >> freed in vfio_instance_finalize(). >> >> I agree this is strange, but I guess there may be some reason I'm unclear. >Yeah but if vfio_realize does fail, I am not sure the >vfio_instance_finalize gets called If vfio_realize fails, fio_exitfn() will not be called but vfio_instance_finalize will. Note vfio_instance_finalize() is called by object_unref(). Thanks Zhenzhong
On 9/27/23 14:32, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Wednesday, September 27, 2023 8:23 PM >> Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device >> >> >> >> On 9/27/23 12:07, Duan, Zhenzhong wrote: >>> Hi Eric, >>> >>>> -----Original Message----- >>>> From: Eric Auger <eric.auger@redhat.com> >>>> Sent: Wednesday, September 27, 2023 5:02 PM >>>> Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device >>>> >>>> Hi Zhenzhong, >>>> >>>> On 9/26/23 13:32, Zhenzhong Duan wrote: >>>>> From: Eric Auger <eric.auger@redhat.com> >>>>> >>>>> We want the VFIO devices to be able to use two different >>>>> IOMMU backends, the legacy VFIO one and the new iommufd one. >>>>> >>>>> Introduce vfio_[attach/detach]_device which aim at hiding the >>>>> underlying IOMMU backend (IOCTLs, datatypes, ...). >>>>> >>>>> Once vfio_attach_device completes, the device is attached >>>>> to a security context and its fd can be used. Conversely >>>>> When vfio_detach_device completes, the device has been >>>>> detached from the security context. >>>>> >>>>> At the moment only the implementation based on the legacy >>>>> container/group exists. Let's use it from the vfio-pci device. >>>>> Subsequent patches will handle other devices. >>>>> >>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> include/hw/vfio/vfio-common.h | 3 ++ >>>>> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >>>>> hw/vfio/pci.c | 50 +++----------------------- >>>>> hw/vfio/trace-events | 2 +- >>>>> 4 files changed, 77 insertions(+), 46 deletions(-) >>>>> >>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >> common.h >>>>> index c4e7c3b4a7..12fbfbc37d 100644 >>>>> --- a/include/hw/vfio/vfio-common.h >>>>> +++ b/include/hw/vfio/vfio-common.h >>>>> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >>>>> 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_attach_device(char *name, VFIODevice *vbasedev, >>>>> + AddressSpace *as, Error **errp); >>>>> +void vfio_detach_device(VFIODevice *vbasedev); >>>>> >>>>> int vfio_kvm_device_add_fd(int fd, Error **errp); >>>>> int vfio_kvm_device_del_fd(int fd, Error **errp); >>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>>> index 959b1362bb..7f3798b152 100644 >>>>> --- a/hw/vfio/common.c >>>>> +++ b/hw/vfio/common.c >>>>> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t >> op) >>>>> } >>>>> return vfio_eeh_container_op(container, op); >>>>> } >>>>> + >>>>> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >>>>> +{ >>>>> + char *tmp, group_path[PATH_MAX], *group_name; >>>>> + int ret, groupid; >>>>> + ssize_t len; >>>>> + >>>>> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>>>> + len = readlink(tmp, group_path, sizeof(group_path)); >>>>> + g_free(tmp); >>>>> + >>>>> + if (len <= 0 || len >= sizeof(group_path)) { >>>>> + ret = len < 0 ? -errno : -ENAMETOOLONG; >>>>> + error_setg_errno(errp, -ret, "no iommu_group found"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + group_path[len] = 0; >>>>> + >>>>> + group_name = basename(group_path); >>>>> + if (sscanf(group_name, "%d", &groupid) != 1) { >>>>> + error_setg_errno(errp, errno, "failed to read %s", group_path); >>>>> + return -errno; >>>>> + } >>>>> + return groupid; >>>>> +} >>>>> + >>>>> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >>>>> + AddressSpace *as, Error **errp) >>>>> +{ >>>>> + int groupid = vfio_device_groupid(vbasedev, errp); >>>>> + VFIODevice *vbasedev_iter; >>>>> + VFIOGroup *group; >>>>> + int ret; >>>>> + >>>>> + if (groupid < 0) { >>>>> + return groupid; >>>>> + } >>>>> + >>>>> + trace_vfio_attach_device(vbasedev->name, groupid); >>>>> + >>>>> + group = vfio_get_group(groupid, as, errp); >>>>> + if (!group) { >>>>> + return -ENOENT; >>>>> + } >>>>> + >>>>> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>>>> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>>>> + error_setg(errp, "device is already attached"); >>>>> + vfio_put_group(group); >>>>> + return -EBUSY; >>>>> + } >>>>> + } >>>>> + ret = vfio_get_device(group, name, vbasedev, errp); >>>>> + if (ret) { >>>>> + vfio_put_group(group); >>>>> + } >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +void vfio_detach_device(VFIODevice *vbasedev) >>>>> +{ >>>>> + VFIOGroup *group = vbasedev->group; >>>>> + >>>>> + vfio_put_base_device(vbasedev); >>>>> + vfio_put_group(group); >>>>> +} >>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>>> index 3b2ca3c24c..fe56789893 100644 >>>>> --- a/hw/vfio/pci.c >>>>> +++ b/hw/vfio/pci.c >>>>> @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice >>>> *vdev, Error **errp) >>>>> static void vfio_put_device(VFIOPCIDevice *vdev) >>>>> { >>>>> + vfio_detach_device(&vdev->vbasedev); >>>>> + >>>>> g_free(vdev->vbasedev.name); >>>>> g_free(vdev->msix); >>>>> - >>>>> - vfio_put_base_device(&vdev->vbasedev); >>>>> } >>>>> >>>>> static void vfio_err_notifier_handler(void *opaque) >>>>> @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error >>>> **errp) >>>>> { >>>>> VFIOPCIDevice *vdev = VFIO_PCI(pdev); >>>>> VFIODevice *vbasedev = &vdev->vbasedev; >>>>> - VFIODevice *vbasedev_iter; >>>>> - VFIOGroup *group; >>>>> - char *tmp, *subsys, group_path[PATH_MAX], *group_name; >>>>> + char *tmp, *subsys; >>>>> Error *err = NULL; >>>>> - ssize_t len; >>>>> struct stat st; >>>>> - int groupid; >>>>> int i, ret; >>>>> bool is_mdev; >>>>> char uuid[UUID_FMT_LEN]; >>>>> @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error >>>> **errp) >>>>> vbasedev->type = VFIO_DEVICE_TYPE_PCI; >>>>> vbasedev->dev = DEVICE(vdev); >>>>> >>>>> - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>>>> - len = readlink(tmp, group_path, sizeof(group_path)); >>>>> - g_free(tmp); >>>>> - >>>>> - if (len <= 0 || len >= sizeof(group_path)) { >>>>> - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, >>>>> - "no iommu_group found"); >>>>> - goto error; >>>>> - } >>>>> - >>>>> - group_path[len] = 0; >>>>> - >>>>> - group_name = basename(group_path); >>>>> - if (sscanf(group_name, "%d", &groupid) != 1) { >>>>> - error_setg_errno(errp, errno, "failed to read %s", group_path); >>>>> - goto error; >>>>> - } >>>>> - >>>>> - trace_vfio_realize(vbasedev->name, groupid); >>>>> - >>>>> - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), >>>> errp); >>>>> - if (!group) { >>>>> - goto error; >>>>> - } >>>>> - >>>>> - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>>>> - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>>>> - error_setg(errp, "device is already attached"); >>>>> - vfio_put_group(group); >>>>> - goto error; >>>>> - } >>>>> - } >>>>> - >>>>> /* >>>>> * Mediated devices *might* operate compatibly with discarding of RAM, >> but >>>>> * we cannot know for certain, it depends on whether the mdev vendor >>>> driver >>>>> @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >>>>> if (vbasedev->ram_block_discard_allowed && !is_mdev) { >>>>> error_setg(errp, "x-balloon-allowed only potentially compatible " >>>>> "with mdev devices"); >>>>> - vfio_put_group(group); >>>>> goto error; >>>>> } >>>>> >>>>> @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error >>>> **errp) >>>>> name = g_strdup(vbasedev->name); >>>>> } >>>>> >>>>> - ret = vfio_get_device(group, name, vbasedev, errp); >>>>> + ret = vfio_attach_device(name, vbasedev, >>>>> + pci_device_iommu_address_space(pdev), errp); >>>>> g_free(name); >>>>> if (ret) { >>>>> - vfio_put_group(group); >>>> independently on this patch, I think in case of error we leak >>>> vbasedev->name. Please have a look and if confirmed you can send a >>>> separate patch. >>> In case of error, vbasedev->name is freed in vfio_put_device(). >> called in vfio_instance_finalize only. See comment below >>>> Also I think if any subsequent action fail we shoudl properly detach the >>>> detach the device so introduce an extra error goto label, my bad sorry. >>> vfio_detach_device is called in vfio_instance_finalize(), this is just to follow >>> the old code, group and container resource allocated in vfio_realize() are >>> freed in vfio_instance_finalize(). >>> >>> I agree this is strange, but I guess there may be some reason I'm unclear. >> Yeah but if vfio_realize does fail, I am not sure the >> vfio_instance_finalize gets called > If vfio_realize fails, fio_exitfn() will not be called but vfio_instance_finalize will. > Note vfio_instance_finalize() is called by object_unref(). ah OK that's what I forgot then :-( Eric > > Thanks > Zhenzhong >
Hi Zhenzhong, On 9/27/23 14:32, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Wednesday, September 27, 2023 8:23 PM >> Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device >> >> >> >> On 9/27/23 12:07, Duan, Zhenzhong wrote: >>> Hi Eric, >>> >>>> -----Original Message----- >>>> From: Eric Auger <eric.auger@redhat.com> >>>> Sent: Wednesday, September 27, 2023 5:02 PM >>>> Subject: Re: [PATCH v2 06/12] vfio/pci: Introduce vfio_[attach/detach]_device >>>> >>>> Hi Zhenzhong, >>>> >>>> On 9/26/23 13:32, Zhenzhong Duan wrote: >>>>> From: Eric Auger <eric.auger@redhat.com> >>>>> >>>>> We want the VFIO devices to be able to use two different >>>>> IOMMU backends, the legacy VFIO one and the new iommufd one. >>>>> >>>>> Introduce vfio_[attach/detach]_device which aim at hiding the >>>>> underlying IOMMU backend (IOCTLs, datatypes, ...). >>>>> >>>>> Once vfio_attach_device completes, the device is attached >>>>> to a security context and its fd can be used. Conversely >>>>> When vfio_detach_device completes, the device has been >>>>> detached from the security context. >>>>> >>>>> At the moment only the implementation based on the legacy >>>>> container/group exists. Let's use it from the vfio-pci device. >>>>> Subsequent patches will handle other devices. >>>>> >>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> include/hw/vfio/vfio-common.h | 3 ++ >>>>> hw/vfio/common.c | 68 +++++++++++++++++++++++++++++++++++ >>>>> hw/vfio/pci.c | 50 +++----------------------- >>>>> hw/vfio/trace-events | 2 +- >>>>> 4 files changed, 77 insertions(+), 46 deletions(-) >>>>> >>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >> common.h >>>>> index c4e7c3b4a7..12fbfbc37d 100644 >>>>> --- a/include/hw/vfio/vfio-common.h >>>>> +++ b/include/hw/vfio/vfio-common.h >>>>> @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); >>>>> 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_attach_device(char *name, VFIODevice *vbasedev, >>>>> + AddressSpace *as, Error **errp); >>>>> +void vfio_detach_device(VFIODevice *vbasedev); >>>>> >>>>> int vfio_kvm_device_add_fd(int fd, Error **errp); >>>>> int vfio_kvm_device_del_fd(int fd, Error **errp); >>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>>> index 959b1362bb..7f3798b152 100644 >>>>> --- a/hw/vfio/common.c >>>>> +++ b/hw/vfio/common.c >>>>> @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t >> op) >>>>> } >>>>> return vfio_eeh_container_op(container, op); >>>>> } >>>>> + >>>>> +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) >>>>> +{ >>>>> + char *tmp, group_path[PATH_MAX], *group_name; >>>>> + int ret, groupid; >>>>> + ssize_t len; >>>>> + >>>>> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>>>> + len = readlink(tmp, group_path, sizeof(group_path)); >>>>> + g_free(tmp); >>>>> + >>>>> + if (len <= 0 || len >= sizeof(group_path)) { >>>>> + ret = len < 0 ? -errno : -ENAMETOOLONG; >>>>> + error_setg_errno(errp, -ret, "no iommu_group found"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + group_path[len] = 0; >>>>> + >>>>> + group_name = basename(group_path); >>>>> + if (sscanf(group_name, "%d", &groupid) != 1) { >>>>> + error_setg_errno(errp, errno, "failed to read %s", group_path); >>>>> + return -errno; >>>>> + } >>>>> + return groupid; >>>>> +} >>>>> + >>>>> +int vfio_attach_device(char *name, VFIODevice *vbasedev, >>>>> + AddressSpace *as, Error **errp) >>>>> +{ >>>>> + int groupid = vfio_device_groupid(vbasedev, errp); >>>>> + VFIODevice *vbasedev_iter; >>>>> + VFIOGroup *group; >>>>> + int ret; >>>>> + >>>>> + if (groupid < 0) { >>>>> + return groupid; >>>>> + } >>>>> + >>>>> + trace_vfio_attach_device(vbasedev->name, groupid); >>>>> + >>>>> + group = vfio_get_group(groupid, as, errp); >>>>> + if (!group) { >>>>> + return -ENOENT; >>>>> + } >>>>> + >>>>> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>>>> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>>>> + error_setg(errp, "device is already attached"); >>>>> + vfio_put_group(group); >>>>> + return -EBUSY; >>>>> + } >>>>> + } >>>>> + ret = vfio_get_device(group, name, vbasedev, errp); >>>>> + if (ret) { >>>>> + vfio_put_group(group); >>>>> + } >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +void vfio_detach_device(VFIODevice *vbasedev) >>>>> +{ >>>>> + VFIOGroup *group = vbasedev->group; >>>>> + >>>>> + vfio_put_base_device(vbasedev); >>>>> + vfio_put_group(group); >>>>> +} >>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>>> index 3b2ca3c24c..fe56789893 100644 >>>>> --- a/hw/vfio/pci.c >>>>> +++ b/hw/vfio/pci.c >>>>> @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice >>>> *vdev, Error **errp) >>>>> static void vfio_put_device(VFIOPCIDevice *vdev) >>>>> { >>>>> + vfio_detach_device(&vdev->vbasedev); >>>>> + >>>>> g_free(vdev->vbasedev.name); >>>>> g_free(vdev->msix); >>>>> - >>>>> - vfio_put_base_device(&vdev->vbasedev); >>>>> } >>>>> >>>>> static void vfio_err_notifier_handler(void *opaque) >>>>> @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error >>>> **errp) >>>>> { >>>>> VFIOPCIDevice *vdev = VFIO_PCI(pdev); >>>>> VFIODevice *vbasedev = &vdev->vbasedev; >>>>> - VFIODevice *vbasedev_iter; >>>>> - VFIOGroup *group; >>>>> - char *tmp, *subsys, group_path[PATH_MAX], *group_name; >>>>> + char *tmp, *subsys; >>>>> Error *err = NULL; >>>>> - ssize_t len; >>>>> struct stat st; >>>>> - int groupid; >>>>> int i, ret; >>>>> bool is_mdev; >>>>> char uuid[UUID_FMT_LEN]; >>>>> @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error >>>> **errp) >>>>> vbasedev->type = VFIO_DEVICE_TYPE_PCI; >>>>> vbasedev->dev = DEVICE(vdev); >>>>> >>>>> - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>>>> - len = readlink(tmp, group_path, sizeof(group_path)); >>>>> - g_free(tmp); >>>>> - >>>>> - if (len <= 0 || len >= sizeof(group_path)) { >>>>> - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, >>>>> - "no iommu_group found"); >>>>> - goto error; >>>>> - } >>>>> - >>>>> - group_path[len] = 0; >>>>> - >>>>> - group_name = basename(group_path); >>>>> - if (sscanf(group_name, "%d", &groupid) != 1) { >>>>> - error_setg_errno(errp, errno, "failed to read %s", group_path); >>>>> - goto error; >>>>> - } >>>>> - >>>>> - trace_vfio_realize(vbasedev->name, groupid); >>>>> - >>>>> - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), >>>> errp); >>>>> - if (!group) { >>>>> - goto error; >>>>> - } >>>>> - >>>>> - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>>>> - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>>>> - error_setg(errp, "device is already attached"); >>>>> - vfio_put_group(group); >>>>> - goto error; >>>>> - } >>>>> - } >>>>> - >>>>> /* >>>>> * Mediated devices *might* operate compatibly with discarding of RAM, >> but >>>>> * we cannot know for certain, it depends on whether the mdev vendor >>>> driver >>>>> @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >>>>> if (vbasedev->ram_block_discard_allowed && !is_mdev) { >>>>> error_setg(errp, "x-balloon-allowed only potentially compatible " >>>>> "with mdev devices"); >>>>> - vfio_put_group(group); >>>>> goto error; >>>>> } >>>>> >>>>> @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error >>>> **errp) >>>>> name = g_strdup(vbasedev->name); >>>>> } >>>>> >>>>> - ret = vfio_get_device(group, name, vbasedev, errp); >>>>> + ret = vfio_attach_device(name, vbasedev, >>>>> + pci_device_iommu_address_space(pdev), errp); >>>>> g_free(name); >>>>> if (ret) { >>>>> - vfio_put_group(group); >>>> independently on this patch, I think in case of error we leak >>>> vbasedev->name. Please have a look and if confirmed you can send a >>>> separate patch. >>> In case of error, vbasedev->name is freed in vfio_put_device(). >> called in vfio_instance_finalize only. See comment below >>>> Also I think if any subsequent action fail we shoudl properly detach the >>>> detach the device so introduce an extra error goto label, my bad sorry. >>> vfio_detach_device is called in vfio_instance_finalize(), this is just to follow >>> the old code, group and container resource allocated in vfio_realize() are >>> freed in vfio_instance_finalize(). >>> >>> I agree this is strange, but I guess there may be some reason I'm unclear. >> Yeah but if vfio_realize does fail, I am not sure the >> vfio_instance_finalize gets called > If vfio_realize fails, fio_exitfn() will not be called but vfio_instance_finalize will. > Note vfio_instance_finalize() is called by object_unref(). I think this holds if the object has been properly initialized. With gdb I don't see vfio_instance_finalize() gets called on vfio_realize() failure. Thanks Eric > > Thanks > Zhenzhong >
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index c4e7c3b4a7..12fbfbc37d 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -225,6 +225,9 @@ void vfio_put_group(VFIOGroup *group); 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_attach_device(char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp); +void vfio_detach_device(VFIODevice *vbasedev); int vfio_kvm_device_add_fd(int fd, Error **errp); int vfio_kvm_device_del_fd(int fd, Error **errp); diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 959b1362bb..7f3798b152 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -2611,3 +2611,71 @@ int vfio_eeh_as_op(AddressSpace *as, uint32_t op) } return vfio_eeh_container_op(container, op); } + +static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp) +{ + char *tmp, group_path[PATH_MAX], *group_name; + int ret, groupid; + ssize_t len; + + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); + len = readlink(tmp, group_path, sizeof(group_path)); + g_free(tmp); + + if (len <= 0 || len >= sizeof(group_path)) { + ret = len < 0 ? -errno : -ENAMETOOLONG; + error_setg_errno(errp, -ret, "no iommu_group found"); + return ret; + } + + group_path[len] = 0; + + group_name = basename(group_path); + if (sscanf(group_name, "%d", &groupid) != 1) { + error_setg_errno(errp, errno, "failed to read %s", group_path); + return -errno; + } + return groupid; +} + +int vfio_attach_device(char *name, VFIODevice *vbasedev, + AddressSpace *as, Error **errp) +{ + int groupid = vfio_device_groupid(vbasedev, errp); + VFIODevice *vbasedev_iter; + VFIOGroup *group; + int ret; + + if (groupid < 0) { + return groupid; + } + + trace_vfio_attach_device(vbasedev->name, groupid); + + group = vfio_get_group(groupid, as, errp); + if (!group) { + return -ENOENT; + } + + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { + error_setg(errp, "device is already attached"); + vfio_put_group(group); + return -EBUSY; + } + } + ret = vfio_get_device(group, name, vbasedev, errp); + if (ret) { + vfio_put_group(group); + } + + return ret; +} + +void vfio_detach_device(VFIODevice *vbasedev) +{ + VFIOGroup *group = vbasedev->group; + + vfio_put_base_device(vbasedev); + vfio_put_group(group); +} diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 3b2ca3c24c..fe56789893 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2828,10 +2828,10 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) static void vfio_put_device(VFIOPCIDevice *vdev) { + vfio_detach_device(&vdev->vbasedev); + g_free(vdev->vbasedev.name); g_free(vdev->msix); - - vfio_put_base_device(&vdev->vbasedev); } static void vfio_err_notifier_handler(void *opaque) @@ -2978,13 +2978,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) { VFIOPCIDevice *vdev = VFIO_PCI(pdev); VFIODevice *vbasedev = &vdev->vbasedev; - VFIODevice *vbasedev_iter; - VFIOGroup *group; - char *tmp, *subsys, group_path[PATH_MAX], *group_name; + char *tmp, *subsys; Error *err = NULL; - ssize_t len; struct stat st; - int groupid; int i, ret; bool is_mdev; char uuid[UUID_FMT_LEN]; @@ -3015,39 +3011,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) vbasedev->type = VFIO_DEVICE_TYPE_PCI; vbasedev->dev = DEVICE(vdev); - tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); - len = readlink(tmp, group_path, sizeof(group_path)); - g_free(tmp); - - if (len <= 0 || len >= sizeof(group_path)) { - error_setg_errno(errp, len < 0 ? errno : ENAMETOOLONG, - "no iommu_group found"); - goto error; - } - - group_path[len] = 0; - - group_name = basename(group_path); - if (sscanf(group_name, "%d", &groupid) != 1) { - error_setg_errno(errp, errno, "failed to read %s", group_path); - goto error; - } - - trace_vfio_realize(vbasedev->name, groupid); - - group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev), errp); - if (!group) { - goto error; - } - - QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { - if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { - error_setg(errp, "device is already attached"); - vfio_put_group(group); - goto error; - } - } - /* * Mediated devices *might* operate compatibly with discarding of RAM, but * we cannot know for certain, it depends on whether the mdev vendor driver @@ -3065,7 +3028,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) if (vbasedev->ram_block_discard_allowed && !is_mdev) { error_setg(errp, "x-balloon-allowed only potentially compatible " "with mdev devices"); - vfio_put_group(group); goto error; } @@ -3076,10 +3038,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) name = g_strdup(vbasedev->name); } - ret = vfio_get_device(group, name, vbasedev, errp); + ret = vfio_attach_device(name, vbasedev, + pci_device_iommu_address_space(pdev), errp); g_free(name); if (ret) { - vfio_put_group(group); goto error; } @@ -3304,7 +3266,6 @@ error: static void vfio_instance_finalize(Object *obj) { VFIOPCIDevice *vdev = VFIO_PCI(obj); - VFIOGroup *group = vdev->vbasedev.group; vfio_display_finalize(vdev); vfio_bars_finalize(vdev); @@ -3318,7 +3279,6 @@ static void vfio_instance_finalize(Object *obj) * g_free(vdev->igd_opregion); */ vfio_put_device(vdev); - vfio_put_group(group); } static void vfio_exitfn(PCIDevice *pdev) diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index e64ca4a019..e710026a73 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -37,7 +37,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s" vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n size: 0x%lx, offset: 0x%lx, flags: 0x%lx" vfio_populate_device_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s" -vfio_realize(const char *name, int group_id) " (%s) group %d" +vfio_attach_device(const char *name, int group_id) " (%s) group %d" vfio_mdev(const char *name, bool is_mdev) " (%s) is_mdev %d" vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s 0x%x@0x%x" vfio_pci_reset(const char *name) " (%s)"