| Message ID | 20240506092053.388578-11-clg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Improve error reporting (part 2) | expand |
On 06/05/2024 12:20, Cédric Le Goater wrote: > External email: Use caution opening links or attachments > > > vfio_set_migration_error() sets the 'return' error on the migration > stream if a migration is in progress. To improve error reporting, add > a new Error* argument to also set the Error object on the migration > stream, if a migration is progress. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > > Changes in v5: > > - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error") > > hw/vfio/common.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -147,10 +147,10 @@ bool vfio_viommu_preset(VFIODevice *vbasedev) > return vbasedev->bcontainer->space->as != &address_space_memory; > } > > -static void vfio_set_migration_error(int err) > +static void vfio_set_migration_error(int ret, Error *err) > { > if (migration_is_setup_or_active()) { > - migration_file_set_error(err, NULL); > + migration_file_set_error(ret, err); > } > } > > @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > iova, iova + iotlb->addr_mask); > > if (iotlb->target_as != &address_space_memory) { > - error_report("Wrong target AS \"%s\", only system memory is allowed", > - iotlb->target_as->name ? iotlb->target_as->name : "none"); > - vfio_set_migration_error(-EINVAL); > + error_setg(&local_err, > + "Wrong target AS \"%s\", only system memory is allowed", > + iotlb->target_as->name ? iotlb->target_as->name : "none"); > + vfio_set_migration_error(-EINVAL, local_err); > return; > } > > @@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > ret = vfio_container_dma_unmap(bcontainer, iova, > iotlb->addr_mask + 1, iotlb); > if (ret) { > - error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " > - "0x%"HWADDR_PRIx") = %d (%s)", > - bcontainer, iova, > - iotlb->addr_mask + 1, ret, strerror(-ret)); > - vfio_set_migration_error(ret); > + error_setg(&local_err, > + "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " > + "0x%"HWADDR_PRIx") = %d (%s)", > + bcontainer, iova, > + iotlb->addr_mask + 1, ret, strerror(-ret)); Use error_setg_errno()? > + vfio_set_migration_error(ret, local_err); Now dma unmap errors (and also the error before it) are not reported if they happen not during migration. This makes me think, maybe vfio_set_migration_error() is redundant and can be replaced by migration_file_set_error()? Thanks. > } > } > out: > @@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener) > if (ret) { > error_prepend(&local_err, > "vfio: Could not stop dirty page tracking - "); > - error_report_err(local_err); > - vfio_set_migration_error(ret); > + vfio_set_migration_error(ret, local_err); > } > } > > @@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); > > if (iotlb->target_as != &address_space_memory) { > - error_report("Wrong target AS \"%s\", only system memory is allowed", > - iotlb->target_as->name ? iotlb->target_as->name : "none"); > + error_setg(&local_err, > + "Wrong target AS \"%s\", only system memory is allowed", > + iotlb->target_as->name ? iotlb->target_as->name : "none"); > goto out; > } > > rcu_read_lock(); > if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { > - error_report_err(local_err); > goto out_lock; > } > > @@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " > "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, > iotlb->addr_mask + 1); > - error_report_err(local_err); > } > > out_lock: > @@ -1252,7 +1252,7 @@ out_lock: > > out: > if (ret) { > - vfio_set_migration_error(ret); > + vfio_set_migration_error(ret, local_err); > } > } > > @@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener, > if (vfio_devices_all_dirty_tracking(bcontainer)) { > ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); > if (ret) { > - error_report_err(local_err); > - vfio_set_migration_error(ret); > + vfio_set_migration_error(ret, local_err); > } > } > } > -- > 2.45.0 >
On 5/13/24 16:26, Avihai Horon wrote: > > On 06/05/2024 12:20, Cédric Le Goater wrote: >> External email: Use caution opening links or attachments >> >> >> vfio_set_migration_error() sets the 'return' error on the migration >> stream if a migration is in progress. To improve error reporting, add >> a new Error* argument to also set the Error object on the migration >> stream, if a migration is progress. >> >> Signed-off-by: Cédric Le Goater <clg@redhat.com> >> --- >> >> Changes in v5: >> >> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error") >> >> hw/vfio/common.c | 37 ++++++++++++++++++------------------- >> 1 file changed, 18 insertions(+), 19 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -147,10 +147,10 @@ bool vfio_viommu_preset(VFIODevice *vbasedev) >> return vbasedev->bcontainer->space->as != &address_space_memory; >> } >> >> -static void vfio_set_migration_error(int err) >> +static void vfio_set_migration_error(int ret, Error *err) >> { >> if (migration_is_setup_or_active()) { >> - migration_file_set_error(err, NULL); >> + migration_file_set_error(ret, err); >> } >> } >> >> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> iova, iova + iotlb->addr_mask); >> >> if (iotlb->target_as != &address_space_memory) { >> - error_report("Wrong target AS \"%s\", only system memory is allowed", >> - iotlb->target_as->name ? iotlb->target_as->name : "none"); >> - vfio_set_migration_error(-EINVAL); >> + error_setg(&local_err, >> + "Wrong target AS \"%s\", only system memory is allowed", >> + iotlb->target_as->name ? iotlb->target_as->name : "none"); >> + vfio_set_migration_error(-EINVAL, local_err); >> return; >> } >> >> @@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> ret = vfio_container_dma_unmap(bcontainer, iova, >> iotlb->addr_mask + 1, iotlb); >> if (ret) { >> - error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " >> - "0x%"HWADDR_PRIx") = %d (%s)", >> - bcontainer, iova, >> - iotlb->addr_mask + 1, ret, strerror(-ret)); >> - vfio_set_migration_error(ret); >> + error_setg(&local_err, >> + "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " >> + "0x%"HWADDR_PRIx") = %d (%s)", >> + bcontainer, iova, >> + iotlb->addr_mask + 1, ret, strerror(-ret)); > > Use error_setg_errno()? sure. > >> + vfio_set_migration_error(ret, local_err); > > Now dma unmap errors (and also the error before it) are not reported if they happen not during migration. > > This makes me think, maybe vfio_set_migration_error() is redundant and can be replaced by migration_file_set_error()? yes. Good suggestion. I would like to get rid of vfio_set_migration_error(), so that's a good start. Thanks, C. > Thanks. > >> } >> } >> out: >> @@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener) >> if (ret) { >> error_prepend(&local_err, >> "vfio: Could not stop dirty page tracking - "); >> - error_report_err(local_err); >> - vfio_set_migration_error(ret); >> + vfio_set_migration_error(ret, local_err); >> } >> } >> >> @@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); >> >> if (iotlb->target_as != &address_space_memory) { >> - error_report("Wrong target AS \"%s\", only system memory is allowed", >> - iotlb->target_as->name ? iotlb->target_as->name : "none"); >> + error_setg(&local_err, >> + "Wrong target AS \"%s\", only system memory is allowed", >> + iotlb->target_as->name ? iotlb->target_as->name : "none"); >> goto out; >> } >> >> rcu_read_lock(); >> if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { >> - error_report_err(local_err); >> goto out_lock; >> } >> >> @@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >> "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " >> "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, >> iotlb->addr_mask + 1); >> - error_report_err(local_err); >> } >> >> out_lock: >> @@ -1252,7 +1252,7 @@ out_lock: >> >> out: >> if (ret) { >> - vfio_set_migration_error(ret); >> + vfio_set_migration_error(ret, local_err); >> } >> } >> >> @@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener, >> if (vfio_devices_all_dirty_tracking(bcontainer)) { >> ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); >> if (ret) { >> - error_report_err(local_err); >> - vfio_set_migration_error(ret); >> + vfio_set_migration_error(ret, local_err); >> } >> } >> } >> -- >> 2.45.0 >> >
On 5/14/24 13:20, Cédric Le Goater wrote: > On 5/13/24 16:26, Avihai Horon wrote: >> >> On 06/05/2024 12:20, Cédric Le Goater wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> vfio_set_migration_error() sets the 'return' error on the migration >>> stream if a migration is in progress. To improve error reporting, add >>> a new Error* argument to also set the Error object on the migration >>> stream, if a migration is progress. >>> >>> Signed-off-by: Cédric Le Goater <clg@redhat.com> >>> --- >>> >>> Changes in v5: >>> >>> - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error") >>> >>> hw/vfio/common.c | 37 ++++++++++++++++++------------------- >>> 1 file changed, 18 insertions(+), 19 deletions(-) >>> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -147,10 +147,10 @@ bool vfio_viommu_preset(VFIODevice *vbasedev) >>> return vbasedev->bcontainer->space->as != &address_space_memory; >>> } >>> >>> -static void vfio_set_migration_error(int err) >>> +static void vfio_set_migration_error(int ret, Error *err) >>> { >>> if (migration_is_setup_or_active()) { >>> - migration_file_set_error(err, NULL); >>> + migration_file_set_error(ret, err); >>> } >>> } >>> >>> @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >>> iova, iova + iotlb->addr_mask); >>> >>> if (iotlb->target_as != &address_space_memory) { >>> - error_report("Wrong target AS \"%s\", only system memory is allowed", >>> - iotlb->target_as->name ? iotlb->target_as->name : "none"); >>> - vfio_set_migration_error(-EINVAL); >>> + error_setg(&local_err, >>> + "Wrong target AS \"%s\", only system memory is allowed", >>> + iotlb->target_as->name ? iotlb->target_as->name : "none"); >>> + vfio_set_migration_error(-EINVAL, local_err); >>> return; >>> } >>> >>> @@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >>> ret = vfio_container_dma_unmap(bcontainer, iova, >>> iotlb->addr_mask + 1, iotlb); >>> if (ret) { >>> - error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " >>> - "0x%"HWADDR_PRIx") = %d (%s)", >>> - bcontainer, iova, >>> - iotlb->addr_mask + 1, ret, strerror(-ret)); >>> - vfio_set_migration_error(ret); >>> + error_setg(&local_err, >>> + "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " >>> + "0x%"HWADDR_PRIx") = %d (%s)", >>> + bcontainer, iova, >>> + iotlb->addr_mask + 1, ret, strerror(-ret)); >> >> Use error_setg_errno()? > > sure. > >> >>> + vfio_set_migration_error(ret, local_err); >> >> Now dma unmap errors (and also the error before it) are not reported if they happen not during migration. >> >> This makes me think, maybe vfio_set_migration_error() is redundant and can be replaced by migration_file_set_error()? > > > yes. Good suggestion. I would like to get rid of vfio_set_migration_error(), > so that's a good start. After taking a closer look, I will drop this patch from v6 because it needs further splitting and some rework. Thanks, C.
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index c3d82a9d6e434e33f361e4b96157bf912d5c3a2f..4cf3e13a8439bd1b9a032e9d4e75df676eba457b 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -147,10 +147,10 @@ bool vfio_viommu_preset(VFIODevice *vbasedev) return vbasedev->bcontainer->space->as != &address_space_memory; } -static void vfio_set_migration_error(int err) +static void vfio_set_migration_error(int ret, Error *err) { if (migration_is_setup_or_active()) { - migration_file_set_error(err, NULL); + migration_file_set_error(ret, err); } } @@ -295,9 +295,10 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) iova, iova + iotlb->addr_mask); if (iotlb->target_as != &address_space_memory) { - error_report("Wrong target AS \"%s\", only system memory is allowed", - iotlb->target_as->name ? iotlb->target_as->name : "none"); - vfio_set_migration_error(-EINVAL); + error_setg(&local_err, + "Wrong target AS \"%s\", only system memory is allowed", + iotlb->target_as->name ? iotlb->target_as->name : "none"); + vfio_set_migration_error(-EINVAL, local_err); return; } @@ -330,11 +331,12 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) ret = vfio_container_dma_unmap(bcontainer, iova, iotlb->addr_mask + 1, iotlb); if (ret) { - error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " - "0x%"HWADDR_PRIx") = %d (%s)", - bcontainer, iova, - iotlb->addr_mask + 1, ret, strerror(-ret)); - vfio_set_migration_error(ret); + error_setg(&local_err, + "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", " + "0x%"HWADDR_PRIx") = %d (%s)", + bcontainer, iova, + iotlb->addr_mask + 1, ret, strerror(-ret)); + vfio_set_migration_error(ret, local_err); } } out: @@ -1108,8 +1110,7 @@ static void vfio_listener_log_global_stop(MemoryListener *listener) if (ret) { error_prepend(&local_err, "vfio: Could not stop dirty page tracking - "); - error_report_err(local_err); - vfio_set_migration_error(ret); + vfio_set_migration_error(ret, local_err); } } @@ -1226,14 +1227,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask); if (iotlb->target_as != &address_space_memory) { - error_report("Wrong target AS \"%s\", only system memory is allowed", - iotlb->target_as->name ? iotlb->target_as->name : "none"); + error_setg(&local_err, + "Wrong target AS \"%s\", only system memory is allowed", + iotlb->target_as->name ? iotlb->target_as->name : "none"); goto out; } rcu_read_lock(); if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err)) { - error_report_err(local_err); goto out_lock; } @@ -1244,7 +1245,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " "0x%"HWADDR_PRIx") failed - ", bcontainer, iova, iotlb->addr_mask + 1); - error_report_err(local_err); } out_lock: @@ -1252,7 +1252,7 @@ out_lock: out: if (ret) { - vfio_set_migration_error(ret); + vfio_set_migration_error(ret, local_err); } } @@ -1372,8 +1372,7 @@ static void vfio_listener_log_sync(MemoryListener *listener, if (vfio_devices_all_dirty_tracking(bcontainer)) { ret = vfio_sync_dirty_bitmap(bcontainer, section, &local_err); if (ret) { - error_report_err(local_err); - vfio_set_migration_error(ret); + vfio_set_migration_error(ret, local_err); } } }
vfio_set_migration_error() sets the 'return' error on the migration stream if a migration is in progress. To improve error reporting, add a new Error* argument to also set the Error object on the migration stream, if a migration is progress. Signed-off-by: Cédric Le Goater <clg@redhat.com> --- Changes in v5: - Rebased on 20c64c8a51a4 ("migration: migration_file_set_error") hw/vfio/common.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-)