Hi Matthew,
On 10/4/23 15:41, Matthew Rosato wrote:
> 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:
yes thank you for the notice
>
>
> 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);
So I took that one
>
>
> 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>
Thank you for your time!
Eric
>
> 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[] = {
