| Message ID | 20240306133441.2351700-19-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Improve error reporting | expand |
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? 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);
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); >
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); + } } 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);