Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device
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
Signed-off-by: Yi Liu
Signed-off-by: Zhenzhong Duan
---
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.
>>
Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device
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
>>> Signed-off-by: Yi Liu
>>> Signed-off-by: Zhenzhong Duan
>>>
>>> ---
>>>
>>> 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
Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device
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
>> Signed-off-by: Yi Liu
>> Signed-off-by: Zhenzhong Duan
>>
>> ---
>>
>> 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
>
> Thanks,
>
> C.
>
>
>
>> static void vfio_ap_unrealize(DeviceState *dev)
>>
Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device
Hi Matthew,
On 10/4/23 01:08, Matthew Rosato wrote:
> 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
>>> Signed-off-by: Yi Liu
>>> Signed-off-by: Zhenzhong Duan
>>>
>>> ---
>>>
>>> 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.
yes you totally right. Just need to
Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device
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
>> Signed-off-by: Yi Liu
>> Signed-off-by: Zhenzhong Duan
>>
>> ---
>>
>> 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
> failu
Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device
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
Signed-off-by: Yi Liu
Signed-off-by: Zhenzhong Duan
---
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.
This is minor, so :
Reviewed-by: Cédric Le Goater
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[] = {
[PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device
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
Signed-off-by: Yi Liu
Signed-off-by: Zhenzhong Duan
---
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);
}
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[] = {
--
2.41.0
