[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device
From: |
Duan, Zhenzhong |
Subject: |
RE: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device |
Date: |
Wed, 27 Sep 2023 12:30:04 +0000 |
>-----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