Re: [PATCH v4 20/33] vfio/migration: Add x-migration-load-config-after-iter VFIO property

["Cédric Le Goater" <clg@redhat.com>]

On 2/11/25 16:57, Maciej S. Szmigiero wrote:
> On 11.02.2025 16:00, Cédric Le Goater wrote:
>> On 2/11/25 15:37, Maciej S. Szmigiero wrote:
>>> On 10.02.2025 18:24, Cédric Le Goater wrote:
>>>> On 1/30/25 11:08, Maciej S. Szmigiero wrote:
>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>
>>>>> This property allows configuring whether to start the config load only
>>>>> after all iterables were loaded.
>>>>> Such interlocking is required for ARM64 due to this platform VFIO
>>>>> dependency on interrupt controller being loaded first.
>>>>>
>>>>> The property defaults to AUTO, which means ON for ARM, OFF for other
>>>>> platforms.>
>>>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>> ---
>>>>>   hw/vfio/migration.c           | 25 +++++++++++++++++++++++++
>>>>>   hw/vfio/pci.c                 |  3 +++
>>>>>   include/hw/vfio/vfio-common.h |  1 +
>>>>>   3 files changed, 29 insertions(+)
>>>>>
>>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>>> index adfa752db527..d801c861d202 100644
>>>>> --- a/hw/vfio/migration.c
>>>>> +++ b/hw/vfio/migration.c
>>>>> @@ -254,6 +254,31 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
>>>>>       return ret;
>>>>>   }
>>>>> +static bool vfio_load_config_after_iter(VFIODevice *vbasedev)
>>>>> +{
>>>>> +    if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_ON) {
>>>>> +        return true;
>>>>> +    } else if (vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_OFF) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>>> +    assert(vbasedev->migration_load_config_after_iter == ON_OFF_AUTO_AUTO);
>>>>> +
>>>>> +    /*
>>>>> +     * Starting the config load only after all iterables were loaded is required
>>>>> +     * for ARM64 due to this platform VFIO dependency on interrupt controller
>>>>> +     * being loaded first.
>>>>> +     *
>>>>> +     * See commit d329f5032e17 ("vfio: Move the saving of the config space to
>>>>> +     * the right place in VFIO migration").
>>>>> +     */
>>>>> +#if defined(TARGET_ARM)
>>>>> +    return true;
>>>>> +#else
>>>>> +    return false;
>>>>> +#endif
>>>>
>>>> I would rather deactivate support on ARM and avoid workarounds.
>>>>
>>>> This can be done in routine vfio_multifd_transfer_supported() I believe,
>>>> at the end of this series. A warning can be added to inform the user.
>>>
>>> The reason why this interlocking support (x-migration-load-config-after-iter)
>>> was added because you said during the review of the previous version of
>>> this patch set that "regarding ARM64, it would be unfortunate to deactivate
>>> the feature since migration works correctly today [..] and this series should
>>> improve also downtime":
>>> https://lore.kernel.org/qemu-devel/59897119-25d7-4a8b-9616-f8ab54e03f65@redhat.com/
>>
>> So much happened since ... my bad. I think this patch is not well
>> placed in the series, it should be at the end.
>>
>> The series should present first the feature in a perfect world
>> and introduce at the end the toggles to handle the corner cases.
>> It helps the reader to focus on the good side of the proposal
>> and better understand the more unpleasant/ugly part.
>>
>>> My point is that after spending time developing and testing that feature> (or "workaround") it would be a shame to throw it away (with all the benefits
>>> it brings) and completely disable multifd VFIO device state transfer on ARM.
>>
>> Well, if you take the approach described above, this patch would
>> be proposed after merge as a fix/workaround for ARM or we would
>> fix the ARM platform.
> 
> Looks like there should be no problems moving this x-migration-load-config-after-iter
> feature to a separate patch near the end of the series - I will try
> to do this.

Thanks, The patch is broken anyhow, since vfio_load_config_after_iter
is unused.
  
>>> Or am I misunderstanding you right now and you only mean here to make
>>> x-migration-load-config-after-iter forcefully enabled on ARM?
>>
>> If we only need this toggle for ARM, and this seems to be the case,
>> let's take a more direct path and avoid a property.
> 
> The reason why we likely want a some kind of switch even on
> a non-ARM platform is ability to test this functionality there.
> 
> Most VFIO setups are probably x86 so people working on this code
> will benefit from easy ability to check if they haven't accidentally
> broke this interlocking.

it's a valid request. We might want to find a better name for the
property.
  
>> I haven't read all your series and the comments yet.
> 
> I keep the series updated with received review comments and re-based
> on top of the latest Fabiano's TLS session termination patches here:
> https://gitlab.com/maciejsszmigiero/qemu/-/commits/multifd-device-state-transfer-vfio
> 
> The changes up to this point has been mostly limited to migration
> core code but it would be nice to get review comments for the VFIO
> parts soon too so I can post a complete new version.

yes. Please introduce vfio/migration-multifd.{c,h} files. I would like
the interface to be clear and avoid bits and pieces of multifd support
slipping into the VFIO migration component.

Also, please provide documentation on :

  - the design principles and its limitations (the mlx5 kernel driver
    has some outcomes)
  - a guide on how to use VFIO migration with multifd
  - how the properties (migration and VFIO) can be set to improve
    performance.

It's not trivial, we will need some guidance for the people wanting
to use it.

I have asked our QE to rerun benchmarks on RHEL. I think we could
run it on ARM too if needed.

Thanks,

C.