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

[Avihai Horon]

On 06/06/2023 16:40, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Avihai
>
> On 6/6/23 13:59, Avihai Horon wrote:
> > 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.
>
> No need to resend if there are no other changes.

I think that just the last paragraph of the commit message in patch #9 
should be removed.

Thanks.

> > > > > 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),