On 10/4/23 5:58 AM, Eric Auger wrote:
> 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
Just to clarify, there is still a little error handling needed for the error
case on vfio_attach_device(), in that case you are passing errp into
vfio_attach_device and that should have an error propogated when ret!=0,
meaning we won't get the dc->unrealize() later. Device wasn't attached, but we
did allocate memory for vbasedev->name already so we need to undo that part
ourselves. That could be inline (as you do already in this patch) or, if you
choose to add the VFIO_MSG_PREFIX it might make sense to put it in an error:
label as Cedric suggested for additional later use. FWIW, I'm fine with either
but here's a snippet of what I mean for the sake of clarity:
Approach 1 (w/ new prepend):
ret = vfio_attach_device(vbasedev->name, vbasedev,
&address_space_memory, errp);
if (ret) {
goto error;
}
vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err);
if (err) {
/*
* Report this error, but do not make it a failing condition.
* Lack of this IRQ in the host does not prevent normal operation.
*/
error_report_err(err);
}
return;
error:
error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name);
g_free(vbasedev->name);
Approach 2 (no prepend):
ret = vfio_attach_device(vbasedev->name, vbasedev,
&address_space_memory, errp);
if (ret) {
g_free(vbasedev->name);
return;
}
vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err);
if (err) {
/*
* Report this error, but do not make it a failing condition.
* Lack of this IRQ in the host does not prevent normal operation.
*/
error_report_err(err);
}
return;
With either of these approaches:
Reviewed-by: Matthew Rosato <mjros...@linux.ibm.com>
Thanks,
Matt
>
> 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[] = {
>>
>
