diff mbox series

[v2,08/12] vfio/ap: Use vfio_[attach/detach]_device

Message ID 20230926113255.1177834-9-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series Prerequisite change for IOMMUFD support | expand

Commit Message

Duan, Zhenzhong Sept. 26, 2023, 11:32 a.m. UTC 
From: Eric Auger <eric.auger@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.auger@redhat.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/ap.c | 68 +++++++++-------------------------------------------
 1 file changed, 11 insertions(+), 57 deletions(-)

Comments

Eric Auger Sept. 27, 2023, 9:16 a.m. UTC | #1 
Hi Zhenzhong,

On 9/26/23 13:32, Zhenzhong Duan wrote:
> From: Eric Auger <eric.auger@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.auger@redhat.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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[] = {
Duan, Zhenzhong Sept. 27, 2023, 11:52 a.m. UTC | #2 
>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Wednesday, September 27, 2023 5:16 PM
>Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device
>
>Hi Zhenzhong,
>
>On 9/26/23 13:32, Zhenzhong Duan wrote:
>> From: Eric Auger <eric.auger@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.auger@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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

Yes, will do.

>
>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
I free it in vfio_ap_unrealize().

Thanks
Zhenzhong
Eric Auger Sept. 27, 2023, 12:24 p.m. UTC | #3 
On 9/27/23 13:52, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: Wednesday, September 27, 2023 5:16 PM
>> Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device
>>
>> Hi Zhenzhong,
>>
>> On 9/26/23 13:32, Zhenzhong Duan wrote:
>>> From: Eric Auger <eric.auger@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.auger@redhat.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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
> Yes, will do.
>
>> 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
> I free it in vfio_ap_unrealize().
is it called if realize fails?
>
> Thanks
> Zhenzhong
Duan, Zhenzhong Sept. 27, 2023, 12:30 p.m. UTC | #4 
>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device
>
>
>
>On 9/27/23 13:52, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.auger@redhat.com>
>>> Sent: Wednesday, September 27, 2023 5:16 PM
>>> Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device
>>>
>>> Hi Zhenzhong,
>>>
>>> On 9/26/23 13:32, Zhenzhong Duan wrote:
>>>> From: Eric Auger <eric.auger@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.auger@redhat.com>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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
>> Yes, will do.
>>
>>> 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
>> I free it in vfio_ap_unrealize().
>is it called if realize fails?

My understanding is:
if realize fails, err path in realize() take this responsibility, if succeed, unrealize() should do that. So as you can see, I have done it in err path.

    if (ret) {
        g_free(vbasedev->name);
        return;
    }

Zhenzhong
Eric Auger Sept. 27, 2023, 12:37 p.m. UTC | #5 
On 9/27/23 14:30, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device
>>
>>
>>
>> On 9/27/23 13:52, Duan, Zhenzhong wrote:
>>>> -----Original Message-----
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>> Sent: Wednesday, September 27, 2023 5:16 PM
>>>> Subject: Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device
>>>>
>>>> Hi Zhenzhong,
>>>>
>>>> On 9/26/23 13:32, Zhenzhong Duan wrote:
>>>>> From: Eric Auger <eric.auger@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.auger@redhat.com>
>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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
>>> Yes, will do.
>>>
>>>> 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
>>> I free it in vfio_ap_unrealize().
>> is it called if realize fails?
> My understanding is:
> if realize fails, err path in realize() take this responsibility, if succeed, unrealize() should do that. So as you can see, I have done it in err path.
>
>     if (ret) {
>         g_free(vbasedev->name);
oh you're right, this is done here sorry. And to me that's what is
missing in some other devices like pci)

Eric
>         return;
>     }
>
> Zhenzhong
diff mbox series

Patch

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);
+    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[] = {