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));
+ 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);
}
}
}
--
2.45.0
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.
© 2016 - 2026 Red Hat, Inc.