Hi Cédric,
On 10/3/23 17:25, Cédric Le Goater wrote:
> On 10/3/23 12:14, Eric Auger wrote:
>> Let the vfio-ap device use vfio_attach_device() and
>> vfio_detach_device(), hence hiding the details of the used
>> IOMMU backend.
>>
>> We take the opportunity to use g_path_get_basename() which
>> is prefered, as suggested by
>> 3e015d815b ("use g_path_get_basename instead of basename")
>>
>> 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>
>>
>> ---
>>
>> v2 -> v3:
>> - Mention g_path_get_basename in commit message and properly free
>> vbasedev->name, call vfio_detach_device
>> ---
>> hw/vfio/ap.c | 70 ++++++++++------------------------------------------
>> 1 file changed, 13 insertions(+), 57 deletions(-)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 6e21d1da5a..d0b587b3b1 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);
>> @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev,
>> Error **errp)
>> * Report this error, but do not make it a failing condition.
>> * Lack of this IRQ in the host does not prevent normal
>> operation.
>> */
>> + vfio_detach_device(vbasedev);
>> error_report_err(err);
>> + g_free(vbasedev->name);
>> }
>> -
>> - return;
>> -
>> -out_get_dev_err:
>> - vfio_ap_put_device(vapdev);
>> - vfio_put_group(vfio_group);
>> }
>
>
> To be consistent with vfio_(pci)_realize(), I would introduce the same
> failure path at the end the routine :
>
> out_detach:
> vfio_detach_device(vbasedev);
> error:
> error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name);
> g_free(vbasedev->name);
>
>
> and add the VFIO_MSG_PREFIX while we are at it.
following Matthew's comment we do not have any need for error handling
in vfio_ap_realize() anymore.
so just removing the improper additions:
+ vfio_detach_device(vbasedev);
+ g_free(vbasedev->name);
Thanks
Eric
>
> This is minor, so :
>
> Reviewed-by: Cédric Le Goater <c...@redhat.com>
>
> Thanks,
>
> C.
>
>
>
>> 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[] = {
>
