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

[Eric Auger]

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