Re: [PATCH 3/3] vfio/migration: Make VFIO migration non-experimental

[Joao Martins]

On 26/06/2023 16:26, Cédric Le Goater wrote:
> On 6/26/23 15:40, Joao Martins wrote:
>> On 26/06/2023 14:20, Cédric Le Goater wrote:
>>> On 6/26/23 10:23, Avihai Horon wrote:
>>>> +        error_setg(&vbasedev->migration_blocker,
>>>> +                   "%s: Migration couldn't be initialized for VFIO 
>>>> device, "
>>>> +                   "err: %d (%s)",
>>>> +                   vbasedev->name, ret, strerror(-ret));
>>>> +        goto add_blocker;
>>>> +    }
>>>> +
>>>> +    if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO &&
>>>> +        !vbasedev->dirty_pages_supported) {
>>>
>>> I don't agree with this test.
>>>
>>
>> The alternative right now is perceptual dirty tracking. How is that OK as a
>> default? It's not like we have even an option :(
>>
>> Maybe perhaps you refer to avoid strongly enforcing *always* it to allow 
>> testing
>> of the non dirty tracking parts? Maybe when you 'force' enabling with
>> enable-migration=on is when you ignore the dirty tracking which is what this 
>> is
>> doing.
> 
> 
> I see ON_OFF_AUTO_ON as a way to abort the machine startup while
> ON_OFF_AUTO_AUTO would let it run but block migration. We would
> need an extra property to relax the checks, else we are hijacking
> some pre-existing option to fit our need.
> 
OK

> Since dirty tracking is a must-have to implement migration support
> for any existing and future VFIO PCI variant driver, anything else
> would be experimental code and we are trying to remove the flag !
> Please correct me if I am wrong.
> 
My thinking was mainly requiring the default migration case without any relaxing
from the user to require dirty tracking by default. hisilicon driver is the
current vfio driver upstream that doesn't support dirty tracking (neither P2P).

> So, the case !vbasedev->dirty_pages_supported is just an extra
> information to report for why migration is not supported. Does
> that sound reasonable ?
> 
We can always piggy back on the x-pre-copy-dirty-page-tracking (which already
exists too) as means to relax this option, and that should cover this other
case. My comment was more targetted at your suggestion of making it optional *by
default* to not use the dirty tracking.

        Joao