Re: [PATCH v5 10/10] vfio: Extend vfio_set_migration_error() with Error* argument

[Cédric Le Goater]

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.