On 3/7/24 09:53, Eric Auger wrote:
>
>
> On 3/6/24 14:34, Cédric Le Goater wrote:
>> This improves error reporting in the log_global_stop() VFIO handler.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>> Changes in v4:
>>
>> - Dropped log_global_stop() and log_global_sync() changes
>>
>> Changes in v3:
>>
>> - Use error_setg_errno() in vfio_devices_dma_logging_stop()
>>
>> hw/vfio/common.c | 19 ++++++++++++++-----
>> 1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index d6790557da2f2890398fa03dbbef18129cd2c1bb..5b2e6a179cdd5f8ca5be84b7097661e96b391456 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -938,12 +938,14 @@ static void vfio_dirty_tracking_init(VFIOContainerBase *bcontainer,
>> memory_listener_unregister(&dirty.listener);
>> }
>>
>> -static void vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer)
>> +static int vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer,
>> + Error **errp)
>> {
>> uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
>> sizeof(uint64_t))] = {};
>> struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
>> VFIODevice *vbasedev;
>> + int ret = 0;
>>
>> feature->argsz = sizeof(buf);
>> feature->flags = VFIO_DEVICE_FEATURE_SET |
>> @@ -955,11 +957,17 @@ static void vfio_devices_dma_logging_stop(VFIOContainerBase *bcontainer)
>> }
>>
>> if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
>> - warn_report("%s: Failed to stop DMA logging, err %d (%s)",
>> - vbasedev->name, -errno, strerror(errno));
>> + /* Keep first error */
>> + if (!ret) {
>> + ret = -errno;
>> + error_setg_errno(errp, errno, "%s: Failed to stop DMA logging",
>> + vbasedev->name);
> maybe you can keep the previous warn_report in case errp is NULL
> (rollback) or for subsequent failures?
Hmm, I wonder if we should keep this patch. It made sense when
vfio_listener_log_global_stop() had an Error ** parameter. We
dropped it in v4 for the sake of simplicity, so we might as
well keep the previous behavior and simply warn the user when
dirty tracking fails to stop. I will check for v5.
Thanks,
C.
>
> Eric
>> + }
>> }
>> vbasedev->dirty_tracking = false;
>> }
>> +
>> + return ret;
>> }
>>
>> static struct vfio_device_feature *
>> @@ -1068,7 +1076,8 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>>
>> out:
>> if (ret) {
>> - vfio_devices_dma_logging_stop(bcontainer);
>> + /* Ignore the potential errors when doing rollback */
>> + vfio_devices_dma_logging_stop(bcontainer, NULL);
>> }
>>
>> vfio_device_feature_dma_logging_start_destroy(feature);
>> @@ -1104,7 +1113,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>> int ret = 0;
>>
>> if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>> - vfio_devices_dma_logging_stop(bcontainer);
>> + ret = vfio_devices_dma_logging_stop(bcontainer, &local_err);
>> } else {
>> ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
>> &local_err);
>