Re: [PATCH v1 1/1] vfio: Make migration support non experimental by default.

[Alex Williamson]

On Mon, 8 Mar 2021 16:36:34 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Mon, Mar 08, 2021 at 09:39:49PM +0530, Tarun Gupta wrote:
> > VFIO migration support in QEMU is experimental as of now, which was done to
> > provide soak time and resolve concerns regarding bit-stream.
> > But, with the patches discussed in
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg784931.html , we have
> > corrected ordering of saving PCI config space and bit-stream.
> > 
> > So, this patch proposes to make vfio migration support in QEMU to be enabled
> > by default. Tested by successfully migrating mdev device.
> > 
> > Signed-off-by: Tarun Gupta <targupta@nvidia.com>
> > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> > ---
> >  hw/vfio/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index f74be78209..15e26f460b 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -3199,7 +3199,7 @@ static Property vfio_pci_dev_properties[] = {
> >      DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
> >                      VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
> >      DEFINE_PROP_BOOL("x-enable-migration", VFIOPCIDevice,
> > -                     vbasedev.enable_migration, false),
> > +                     vbasedev.enable_migration, true),  
> 
> If it isn't experimental then why do we even need an experimental 'x-'
> property at all ?
> 
> IOW, rather than changing this to "true", shouldn't we just be deleting
> the x-enable-migration property entirely and have the code just do the
> right thing.

I don't think it's necessarily invalid to have experimental disables
for supported features.  We actually have quite a few of those already.
Most of them are generally aimed at debugging, for example disabling
direct mappings or acceleration paths that might mask an access when
trying to trace the behavior of a device.  We might want to consider
changing the polarity of the option to avoid user confusion, ie.
x-disable-migration.  I'm not fully convinced that there might not be
valid non-experimental use cases for such an option, but it seems like
that should be supported at a base device class level rather than per
driver, so best to be left experimental here.  Thanks,

Alex