>-----Original Message----- >From: Alex Williamson <alex.william...@redhat.com> >Subject: Re: [PATCH v4 5/5] vfio/migration: Refactor and fix print of >"Migration >disabled" > >On Thu, 29 Jun 2023 16:42:23 +0100 >Joao Martins <joao.m.mart...@oracle.com> wrote: > >> On 29/06/2023 16:20, Avihai Horon wrote: >> > On 29/06/2023 15:44, Joao Martins wrote: >> >> On 29/06/2023 09:40, Zhenzhong Duan wrote: ... >> >>> @@ -403,9 +402,15 @@ int >> >>> vfio_block_multiple_devices_migration(VFIODevice >> >>> *vbasedev, Error **errp) >> >>> if (ret < 0) { >> >>> error_free(multiple_devices_migration_blocker); >> >>> multiple_devices_migration_blocker = NULL; >> >>> + } else { >> >>> + /* >> >>> + * Only ON_OFF_AUTO_AUTO case, ON_OFF_AUTO_OFF is checked >> >>> + * in vfio_migration_realize(). >> >>> + */ >> >>> + warn_report("Migration disabled, not support multiple >> >>> +VFIO devices"); >> >>> } >> >>> >> >> Perhaps you could stash the previous error message and use it in >> >> the warn_report_error to consolidate the error messages e.g. >> >> >> >> bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, >> >> Error **errp) { >> >> Error *err = NULL; >> >> >> >> if (multiple_devices_migration_blocker || >> >> vfio_migratable_device_num() <= 1) { >> >> return true; >> >> } >> >> >> >> error_setg(&err, "%s: Migration is currently not supported with >multiple " >> >> "VFIO devices", vbasedev->name); >> >> >> >> if (vbasedev->enable_migration == ON_OFF_AUTO_ON) { >> >> error_propagate(errp, err); >> >> return -EINVAL; >> >> } >> >> >> >> ... >> >> if (ret < 0) { >> >> } else { >> >> /* Warns only on ON_OFF_AUTO_AUTO case */ >> >> warn_report_err(err); >> > >> > I'm not sure this warning is needed. >> > If I remember correctly, I think Alex didn't want migration >> > error/warning messages to be logged in the AUTO case. > >Correct. > >> Hmm, ok, I missed this from the previous discussions. >> >> So today there are migration warnings in the current code. (even in >> the AUTO case). So if we want them removed, then this patch would then >> just remove the "Migration disabled" all together (in the two places we >commented). >> >> The rest of the cases already propagate the error I think. And the >> AUTO case will always be blocked migration and see the same printed >messages elsewhere. > >I tested this with Avihai's series and saw the correct logging, at least for a >device that does not support migration. > >In AUTO mode, we should only ever see errors or warnings if the device >supports migration and an error or incompatibility occurs while further >probing or configuring it. Lack of support for migration should only ever >generate an error or warning when using enable_migration=on or the global - >only-migratable flag. Will remove the two places of "Migration disabled" print.
> >As I understood Avihai's patch, we're populating the Error pointer, but we >only ever propagate that error in the above cases. Thanks, > >Alex > ... >> >>> +818,11 @@ static int vfio_block_migration(VFIODevice *vbasedev, >> >>> Error *err, Error **errp) >> >>> if (ret < 0) { >> >>> error_free(vbasedev->migration_blocker); >> >>> vbasedev->migration_blocker = NULL; >> >>> + } else if (vbasedev->enable_migration != ON_OFF_AUTO_OFF) { >> >>> + warn_report("%s: Migration disabled", vbasedev->name); >> >>> } >> >>> >> >> Perhaps you can use the the local error to expand on why migration >> >> was disabled e.g. >> >> >> >> warn_report_err(err); >> > >> > Same here. >> > >> > Thanks. >> > >> >> >> >>> - return ret; >> >>> + return !ret; >> >>> } >> >>> >> >>> /* >> >>> ------------------------------------------------------------------ >> >>> ---- */ @@ -835,7 +837,12 @@ void >> >>> vfio_reset_bytes_transferred(void) >> >>> bytes_transferred = 0; >> >>> } >> >>> >> >>> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) >> >>> +/* >> >>> + * Return true when either migration initialized or blocker registered. >> >>> + * Currently only return false when adding blocker fails which >> >>> +will >> >>> + * de-register vfio device. >> >>> + */ >> >>> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp) >> >>> { >> >>> Error *err = NULL; >> >>> int ret; >> >>> @@ -873,18 +880,17 @@ int vfio_migration_realize(VFIODevice >> >>> *vbasedev, Error >> >>> **errp) >> >>> vbasedev->name); >> >>> } >> >>> >> >>> - ret = vfio_block_multiple_devices_migration(vbasedev, errp); >> >>> - if (ret) { >> >>> - return ret; >> >>> + if (!vfio_block_multiple_devices_migration(vbasedev, errp)) { >> >>> + return false; >> >>> } >> >>> >> >>> - ret = vfio_block_giommu_migration(vbasedev, errp); >> >>> - if (ret) { >> >>> - return ret; >> >>> + if (vfio_viommu_preset(vbasedev)) { >> >> The /* Block migration with a vIOMMU */ >> >> >> >> Would go above, but I don't think we need it anymore ... Will remove it. >> >> >> >>> + error_setg(&err, "%s: Migration is currently not supported " >> >>> + "with vIOMMU enabled", vbasedev->name); >> >>> + return vfio_block_migration(vbasedev, err, errp); >> >> ... as the error message when placed here makes it obvious. So the >> >> comment I suggested won't add much. Unless others disagree. >> >> >> >>> } >> >>> >> >>> - trace_vfio_migration_realize(vbasedev->name); >> >>> - return 0; >> >>> + return true; >> >>> } >> >>> >> >> I think somewhere in function we should have vfio_migration_exit() >> >> being called behind a label or elsewhere from >> >> vfio_migration_realize (...) >> >> >> >>> void vfio_migration_exit(VFIODevice *vbasedev) diff --git >> >>> a/hw/vfio/pci.c b/hw/vfio/pci.c index dc69d3031b24..184d08568154 >> >>> 100644 >> >>> --- a/hw/vfio/pci.c >> >>> +++ b/hw/vfio/pci.c >> >>> @@ -3209,7 +3209,8 @@ static void vfio_realize(PCIDevice *pdev, >> >>> Error **errp) >> >>> if (!pdev->failover_pair_id) { >> >>> ret = vfio_migration_realize(vbasedev, errp); >> >>> if (ret) { >> >>> - error_report("%s: Migration disabled", >> >>> vbasedev->name); >> >>> + trace_vfio_migration_realize(vbasedev->name); >> >>> + } else { >> >>> goto out_vfio_migration; >> >>> } >> >>> } >> >> (...) Which then void the need for this change. Perhaps your >> >> previous patch >> >> (4/5) could come after this refactor patch instead ... where you >> >> would fix the unwinding error path inside the >> >> vfio_migration_realize() as opposed to vfio_realize(). Sure, will fix. Thanks Zhenzhong