Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 10/3/23 11:25 AM, 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.auger@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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);

This patch overall looks good to me and passes basic tests with vfio-ap devices.  But I note that this addition of detach+free here runs counter to what the comment block above it states and prior behavior (where we did not goto out_get_dev_err for this case and expect the realize to complete successfully despite this error).  

In this error case, we only report the local 'err' contents and nothing is propagated into 'errp' -- which means that to the caller dc->realize() should be viewed as successful (errp is NULL) and so we should be able to assume a subsequent dc->unrealize() will do this g_free+detach later. 

>>       }
>> -
>> -    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);

So based on my comment above, I think you'd only need the 'error:' case now, but otherwise adding this error_prepend seems reasonable to me too. 

Thanks,
Matt

> 
> 
> and add the VFIO_MSG_PREFIX while we are at it.
> 
> This is minor, so :
> 
> Reviewed-by: Cédric Le Goater <clg@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[] = {
>