Re: [PATCH v5 8/9] vfio/migration: Add x-allow-pre-copy VFIO device property

[Avihai Horon <avihaih@nvidia.com>]

On 05/06/2023 17:56, Alex Williamson wrote:
> External email: Use caution opening links or attachments
>
>
> On Sun, 4 Jun 2023 12:33:43 +0300
> Avihai Horon <avihaih@nvidia.com> wrote:
>
>> On 01/06/2023 23:22, Alex Williamson wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, 30 May 2023 17:48:20 +0300
>>> Avihai Horon <avihaih@nvidia.com> wrote:
>>>
>>>> Add a new VFIO device property x-allow-pre-copy to keep migration
>>>> compatibility to/from older QEMU versions that don't have VFIO pre-copy
>>>> support.
>>> This doesn't make sense to me, vfio migration is not currently
>>> supported, it can only be enabled via an experimental flag.  AFAIK we
>>> have no obligation to maintain migration compatibility against
>>> experimental features.  Is there any other reason we need a flag to
>>> disable pre-copy?
>> This could give flexibility to do migration between hosts without
>> matching VFIO device kernel drivers. E.g., source driver doesn't have
>> precopy support and dest driver has or vice versa.
> If these are valid scenarios, the protocol should support negotiation
> without requiring an experimental flag to do so.

Thinking again, the two intree drivers we have support precopy and as 
you said, this is still experimental, so I assume that we can drop this 
patch in v6.

>
>>> OTOH, should this series finally remove the experimental migration
>>> flag?  Do we require Joao's vIOMMU support to finally make it
>>> supportable?  Is there something else?
>> I think that after precopy is accepted we can remove the experimental
>> flag, as we'll have the major parts of VFIO migration upstream.
>> After that we will still need to add Joao's vIOMMU support and P2P support.
>> Do you want me to add a patch to this series that makes VFIO migration
>> non-experimental?
> I'd keep it as a separate patch with a clearly described dependency on
> this series so that we can discuss it separately.

Sure, so I will post it separately.

Thanks.

>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>>>> ---
>>>>    include/hw/vfio/vfio-common.h | 1 +
>>>>    hw/core/machine.c             | 1 +
>>>>    hw/vfio/migration.c           | 3 ++-
>>>>    hw/vfio/pci.c                 | 2 ++
>>>>    4 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>>> index 1db901c194..a53ecbe2e0 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -146,6 +146,7 @@ typedef struct VFIODevice {
>>>>        VFIOMigration *migration;
>>>>        Error *migration_blocker;
>>>>        OnOffAuto pre_copy_dirty_page_tracking;
>>>> +    bool allow_pre_copy;
>>>>        bool dirty_pages_supported;
>>>>        bool dirty_tracking;
>>>>    } VFIODevice;
>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>> index 1000406211..64ac3fe38e 100644
>>>> --- a/hw/core/machine.c
>>>> +++ b/hw/core/machine.c
>>>> @@ -41,6 +41,7 @@
>>>>
>>>>    GlobalProperty hw_compat_8_0[] = {
>>>>        { "migration", "multifd-flush-after-each-section", "on"},
>>>> +    { "vfio-pci", "x-allow-pre-copy", "false" },
>>>>    };
>>>>    const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
>>>>
>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>> index d8f6a22ae1..cb6923ed3f 100644
>>>> --- a/hw/vfio/migration.c
>>>> +++ b/hw/vfio/migration.c
>>>> @@ -323,7 +323,8 @@ static bool vfio_precopy_supported(VFIODevice *vbasedev)
>>>>    {
>>>>        VFIOMigration *migration = vbasedev->migration;
>>>>
>>>> -    return migration->mig_flags & VFIO_MIGRATION_PRE_COPY;
>>>> +    return vbasedev->allow_pre_copy &&
>>>> +           migration->mig_flags & VFIO_MIGRATION_PRE_COPY;
>>>>    }
>>>>
>>>>    /* ---------------------------------------------------------------------- */
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index 73874a94de..c69813af7f 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -3335,6 +3335,8 @@ static Property vfio_pci_dev_properties[] = {
>>>>        DEFINE_PROP_ON_OFF_AUTO("x-pre-copy-dirty-page-tracking", VFIOPCIDevice,
>>>>                                vbasedev.pre_copy_dirty_page_tracking,
>>>>                                ON_OFF_AUTO_ON),
>>>> +    DEFINE_PROP_BOOL("x-allow-pre-copy", VFIOPCIDevice,
>>>> +                     vbasedev.allow_pre_copy, true),
>>>>        DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
>>>>                                display, ON_OFF_AUTO_OFF),
>>>>        DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),