RE: [PATCH v4 5/5] vfio/migration: Refactor and fix print of "Migration disabled"

[Duan, Zhenzhong]

>-----Original Message-----
>From: Alex Williamson <alex.williamson@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.martins@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