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.
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -148,16 +148,18 @@ 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)
{
MigrationState *ms = migrate_get_current();
if (migration_is_setup_or_active(ms->state)) {
WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
if (ms->to_dst_file) {
- qemu_file_set_error(ms->to_dst_file, err);
+ qemu_file_set_error_obj(ms->to_dst_file, ret, err);
}
}
+ } else {
+ error_report_err(err);
}
}
@@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
VFIOContainerBase *bcontainer = giommu->bcontainer;
hwaddr iova = iotlb->iova + giommu->iommu_offset;
void *vaddr;
+ Error *local_err = NULL;
int ret;
trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
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;
}
@@ -336,11 +340,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:
@@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
VFIOContainerBase *bcontainer = giommu->bcontainer;
hwaddr iova = iotlb->iova + giommu->iommu_offset;
ram_addr_t translated_addr;
+ Error *local_err = NULL;
int ret = -EINVAL;
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;
}
@@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
translated_addr);
if (ret) {
- error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova, iotlb->addr_mask + 1, ret,
- strerror(-ret));
+ error_setg(&local_err,
+ "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") = %d (%s)",
+ bcontainer, iova, iotlb->addr_mask + 1, ret,
+ strerror(-ret));
}
}
rcu_read_unlock();
out:
if (ret) {
- vfio_set_migration_error(ret);
+ vfio_set_migration_error(ret, local_err);
}
}
@@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
+ Error *local_err = NULL;
int ret;
if (vfio_listener_skipped_section(section)) {
@@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
if (vfio_devices_all_dirty_tracking(bcontainer)) {
ret = vfio_sync_dirty_bitmap(bcontainer, section);
if (ret) {
- error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
- strerror(-ret));
- vfio_set_migration_error(ret);
+ error_setg(&local_err,
+ "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
+ strerror(-ret));
+ vfio_set_migration_error(ret, local_err);
}
}
}
--
2.43.0
Hi Cedric,
On 07/02/2024 15:33, 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.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
> 1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -148,16 +148,18 @@ 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)
> {
> MigrationState *ms = migrate_get_current();
>
> if (migration_is_setup_or_active(ms->state)) {
> WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
> if (ms->to_dst_file) {
> - qemu_file_set_error(ms->to_dst_file, err);
> + qemu_file_set_error_obj(ms->to_dst_file, ret, err);
> }
> }
> + } else {
> + error_report_err(err);
> }
> }
>
> @@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> VFIOContainerBase *bcontainer = giommu->bcontainer;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> void *vaddr;
> + Error *local_err = NULL;
> int ret;
>
> trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
> 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;
> }
>
> @@ -336,11 +340,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:
> @@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> VFIOContainerBase *bcontainer = giommu->bcontainer;
> hwaddr iova = iotlb->iova + giommu->iommu_offset;
> ram_addr_t translated_addr;
> + Error *local_err = NULL;
> int ret = -EINVAL;
>
> 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;
> }
>
> @@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
> translated_addr);
If vfio_get_xlat_addr() above (it's not shown here) returns false, we
will pass a NULL local_err to vfio_set_migration_error() and it may
de-reference NULL ptr in error_report_err().
Should we refactor vfio_get_xlat_addr() to get errp, or add an else
branch below, setting -EINVAL (and removing the default -EINVAL from the
top of the function)?
Thanks.
> if (ret) {
> - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> - "0x%"HWADDR_PRIx") = %d (%s)",
> - bcontainer, iova, iotlb->addr_mask + 1, ret,
> - strerror(-ret));
> + error_setg(&local_err,
> + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
> + "0x%"HWADDR_PRIx") = %d (%s)",
> + bcontainer, iova, iotlb->addr_mask + 1, ret,
> + strerror(-ret));
> }
> }
> rcu_read_unlock();
>
> out:
> if (ret) {
> - vfio_set_migration_error(ret);
> + vfio_set_migration_error(ret, local_err);
> }
> }
>
> @@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
> {
> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
> listener);
> + Error *local_err = NULL;
> int ret;
>
> if (vfio_listener_skipped_section(section)) {
> @@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
> if (vfio_devices_all_dirty_tracking(bcontainer)) {
> ret = vfio_sync_dirty_bitmap(bcontainer, section);
> if (ret) {
> - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> - strerror(-ret));
> - vfio_set_migration_error(ret);
> + error_setg(&local_err,
> + "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
> + strerror(-ret));
> + vfio_set_migration_error(ret, local_err);
> }
> }
> }
> --
> 2.43.0
>
>
Hello Avihai,
On 2/12/24 10:35, Avihai Horon wrote:
> Hi Cedric,
>
> On 07/02/2024 15:33, 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.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>> hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
>> 1 file changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 82173b039c47150f5edd05d329192c5b9c8a9a0f..afe8b6bd294fd5904f394a5db48aae3fd718b14e 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -148,16 +148,18 @@ 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)
>> {
>> MigrationState *ms = migrate_get_current();
>>
>> if (migration_is_setup_or_active(ms->state)) {
>> WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>> if (ms->to_dst_file) {
>> - qemu_file_set_error(ms->to_dst_file, err);
>> + qemu_file_set_error_obj(ms->to_dst_file, ret, err);
>> }
>> }
>> + } else {
>> + error_report_err(err);
>> }
>> }
>>
>> @@ -296,15 +298,17 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> VFIOContainerBase *bcontainer = giommu->bcontainer;
>> hwaddr iova = iotlb->iova + giommu->iommu_offset;
>> void *vaddr;
>> + Error *local_err = NULL;
>> int ret;
>>
>> trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
>> 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;
>> }
>>
>> @@ -336,11 +340,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:
>> @@ -1224,13 +1229,15 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> VFIOContainerBase *bcontainer = giommu->bcontainer;
>> hwaddr iova = iotlb->iova + giommu->iommu_offset;
>> ram_addr_t translated_addr;
>> + Error *local_err = NULL;
>> int ret = -EINVAL;
>>
>> 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;
>> }
>>
>> @@ -1239,17 +1246,18 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
>> translated_addr);
>
> If vfio_get_xlat_addr() above (it's not shown here) returns false, we will pass a NULL local_err to vfio_set_migration_error() and it may de-reference NULL ptr in error_report_err().
Ah yes. Thanks for spotting this.
>
> Should we refactor vfio_get_xlat_addr() to get errp,
I think we should add an Error** parameter to vfio_get_xlat_addr() and
memory_get_xlat_addr(). It shouldn't be too much change and would be
cleaner.
Thanks,
C.
> or add an else branch below, setting -EINVAL (and removing the default -EINVAL from the top of the function)?
>
> Thanks.
>
>> if (ret) {
>> - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> - "0x%"HWADDR_PRIx") = %d (%s)",
>> - bcontainer, iova, iotlb->addr_mask + 1, ret,
>> - strerror(-ret));
>> + error_setg(&local_err,
>> + "vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
>> + "0x%"HWADDR_PRIx") = %d (%s)",
>> + bcontainer, iova, iotlb->addr_mask + 1, ret,
>> + strerror(-ret));
>> }
>> }
>> rcu_read_unlock();
>>
>> out:
>> if (ret) {
>> - vfio_set_migration_error(ret);
>> + vfio_set_migration_error(ret, local_err);
>> }
>> }
>>
>> @@ -1345,6 +1353,7 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>> {
>> VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>> listener);
>> + Error *local_err = NULL;
>> int ret;
>>
>> if (vfio_listener_skipped_section(section)) {
>> @@ -1354,9 +1363,10 @@ static void vfio_listener_log_sync(MemoryListener *listener,
>> if (vfio_devices_all_dirty_tracking(bcontainer)) {
>> ret = vfio_sync_dirty_bitmap(bcontainer, section);
>> if (ret) {
>> - error_report("vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
>> - strerror(-ret));
>> - vfio_set_migration_error(ret);
>> + error_setg(&local_err,
>> + "vfio: Failed to sync dirty bitmap, err: %d (%s)", ret,
>> + strerror(-ret));
>> + vfio_set_migration_error(ret, local_err);
>> }
>> }
>> }
>> --
>> 2.43.0
>>
>>
>
On 7/2/24 14:33, Cédric Le Goater wrote:
> 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.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> hw/vfio/common.c | 50 +++++++++++++++++++++++++++++-------------------
> 1 file changed, 30 insertions(+), 20 deletions(-)
> -static void vfio_set_migration_error(int err)
> +static void vfio_set_migration_error(int ret, Error *err)
Maybe rename vfio_report_migration_error() for clarity?
> {
> MigrationState *ms = migrate_get_current();
>
> if (migration_is_setup_or_active(ms->state)) {
> WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
> if (ms->to_dst_file) {
> - qemu_file_set_error(ms->to_dst_file, err);
> + qemu_file_set_error_obj(ms->to_dst_file, ret, err);
> }
> }
> + } else {
> + error_report_err(err);
> }
> }
© 2016 - 2024 Red Hat, Inc.