Hi Zhenzhong,
On 9/26/23 13:32, Zhenzhong Duan wrote:
> From: Eric Auger <eric.au...@redhat.com>
>
> Let the vfio-ap device use vfio_attach_device() and
> vfio_detach_device(), hence hiding the details of the used
> IOMMU backend.
>
> Signed-off-by: Eric Auger <eric.au...@redhat.com>
> Signed-off-by: Yi Liu <yi.l....@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
> ---
> hw/vfio/ap.c | 68 +++++++++-------------------------------------------
> 1 file changed, 11 insertions(+), 57 deletions(-)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 6e21d1da5a..16ea7fb3c2 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = {
> .vfio_compute_needs_reset = vfio_ap_compute_needs_reset,
> };
>
> -static void vfio_ap_put_device(VFIOAPDevice *vapdev)
> -{
> - g_free(vapdev->vdev.name);
> - vfio_put_base_device(&vapdev->vdev);
> -}
> -
> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
> -{
> - GError *gerror = NULL;
> - char *symlink, *group_path;
> - int groupid;
> -
> - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev);
> - group_path = g_file_read_link(symlink, &gerror);
> - g_free(symlink);
> -
> - if (!group_path) {
> - error_setg(errp, "%s: no iommu_group found for %s: %s",
> - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev,
> gerror->message);
> - g_error_free(gerror);
> - return NULL;
> - }
> -
> - if (sscanf(basename(group_path), "%d", &groupid) != 1) {
> - error_setg(errp, "vfio: failed to read %s", group_path);
> - g_free(group_path);
> - return NULL;
> - }
> -
> - g_free(group_path);
> -
> - return vfio_get_group(groupid, &address_space_memory, errp);
> -}
> -
> static void vfio_ap_req_notifier_handler(void *opaque)
> {
> VFIOAPDevice *vapdev = opaque;
> @@ -189,22 +155,15 @@ static void
> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
> static void vfio_ap_realize(DeviceState *dev, Error **errp)
> {
> int ret;
> - char *mdevid;
> Error *err = NULL;
> - VFIOGroup *vfio_group;
> APDevice *apdev = AP_DEVICE(dev);
> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> + VFIODevice *vbasedev = &vapdev->vdev;
>
> - vfio_group = vfio_ap_get_group(vapdev, errp);
> - if (!vfio_group) {
> - return;
> - }
> -
> - vapdev->vdev.ops = &vfio_ap_ops;
> - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP;
> - mdevid = basename(vapdev->vdev.sysfsdev);
> - vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> - vapdev->vdev.dev = dev;
> + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev);
I think we shall document in the commit msg the fact we use
g_path_get_basename instead of basename here to match other device init
see 3e015d815b use g_path_get_basename instead of basename
also leak of vbasedev->name
Eric
> + vbasedev->ops = &vfio_ap_ops;
> + vbasedev->type = VFIO_DEVICE_TYPE_AP;
> + vbasedev->dev = dev;
>
> /*
> * vfio-ap devices operate in a way compatible with discarding of
> @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, Error
> **errp)
> */
> vapdev->vdev.ram_block_discard_allowed = true;
>
> - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp);
> + ret = vfio_attach_device(vbasedev->name, vbasedev,
> + &address_space_memory, errp);
> if (ret) {
> - goto out_get_dev_err;
> + g_free(vbasedev->name);
> + return;
> }
>
> vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err);
> @@ -227,23 +188,16 @@ static void vfio_ap_realize(DeviceState *dev, Error
> **errp)
> */
> error_report_err(err);
> }
> -
> - return;
> -
> -out_get_dev_err:
> - vfio_ap_put_device(vapdev);
> - vfio_put_group(vfio_group);
> }
>
> static void vfio_ap_unrealize(DeviceState *dev)
> {
> APDevice *apdev = AP_DEVICE(dev);
> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> - VFIOGroup *group = vapdev->vdev.group;
>
> vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX);
> - vfio_ap_put_device(vapdev);
> - vfio_put_group(group);
> + vfio_detach_device(&vapdev->vdev);
> + g_free(vapdev->vdev.name);
> }
>
> static Property vfio_ap_properties[] = {
