Re: [Qemu-devel] [PATCH for-3.2 01/11] vhost-user: define conventions for vhost-user backends

[Marc-André Lureau]

Hi

On Thu, Dec 20, 2018 at 8:34 PM Michael S. Tsirkin <address@hidden> wrote:
>
> On Thu, Dec 20, 2018 at 04:40:55PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Dec 19, 2018 at 7:42 PM Michael S. Tsirkin <address@hidden> wrote:
> > >
> > > On Wed, Dec 19, 2018 at 12:01:59PM +0400, Marc-André Lureau wrote:
> > > > Hi
> > > >
> > > > On Wed, Dec 19, 2018 at 3:20 AM Michael S. Tsirkin <address@hidden> 
> > > > wrote:
> > > > >
> > > > > On Tue, Dec 18, 2018 at 10:35:05PM +0400, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > >
> > > > > > On Tue, Dec 11, 2018 at 10:56 PM Michael S. Tsirkin 
> > > > > > <address@hidden> wrote:
> > > > > > >
> > > > > > > On Tue, Dec 11, 2018 at 09:29:44AM +0000, Daniel P. Berrangé 
> > > > > > > wrote:
> > > > > > > > On Tue, Dec 11, 2018 at 08:42:41AM +0100, Hoffmann, Gerd wrote:
> > > > > > > > >   Hi,
> > > > > > > > >
> > > > > > > > > > Right. The main issue is that we need to make sure only
> > > > > > > > > > in-tree devices are supported.
> > > > > > > > >
> > > > > > > > > Well, that is under debate right now, see:
> > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg04912.html
> > > > > > > >
> > > > > > > > I've previously been against the idea of external plugins for 
> > > > > > > > QEMU,
> > > > > > > > however, that was when the plugin was something that would be 
> > > > > > > > dlopen'd
> > > > > > > > by QEMU. That would cause our internal ABI to be exposed to 3rd 
> > > > > > > > parties
> > > > > > > > which is highly undesirable, even if they were open source to 
> > > > > > > > comply
> > > > > > > > with the license needs.
> > > > > > > >
> > > > > > > > When the plugin is a completely isolated process communicating 
> > > > > > > > with a
> > > > > > > > well defined protocol, it is not placing a significant burden 
> > > > > > > > on the
> > > > > > > > QEMU developers' ongoing maintainence, nor has problems with 
> > > > > > > > license
> > > > > > > > compliance. The main problem would come from debugging the 
> > > > > > > > combined
> > > > > > > > system as the external process is essentially a black box from 
> > > > > > > > QEMU's
> > > > > > > > POV. Downstream OS vendors are free to place restrictions on 
> > > > > > > > which
> > > > > > > > backend processes they'd be willing to support with QEMU, and 
> > > > > > > > upstream
> > > > > > > > is under no obligation to debug stuff beyond the QEMU boundary.
> > > > > > > >
> > > > > > > > We have already accepted that tradeoff with networking by 
> > > > > > > > supporting
> > > > > > > > vhost-user and have externals impls like DPDK, so I don't see a
> > > > > > > > compelling reason to try to restrict it for other vhost-user 
> > > > > > > > backends.
> > > > > > >
> > > > > > > OK seems to be more or less a rough concensus then.
> > > > > > >
> > > > > > > I wonder what's the approach wrt migration though.
> > > > > >
> > > > > > The series doesn't take care of migration.
> > > > > >
> > > > > > >
> > > > > > > Even the compatibility story about vhost-user isn't
> > > > > > > great, I would like to see something solid before
> > > > > > > we allow that.
> > > > > >
> > > > > > To allow migration? vhost-user has partial support for migration
> > > > > > (dirty memory tracking), and there is also "[PATCH v2 for-4.0 0/7]
> > > > > > vhost-user-blk: Add support for backend reconnecting" - allowing the
> > > > > > backend to store some state, if I understand correctly, which could 
> > > > > > be
> > > > > > leveraged I guess...
> > > > > >
> > > > > > But I don't think we should block this series because migration 
> > > > > > isn't
> > > > > > tackled here.
> > > > > >
> > > > > > thanks
> > > > > >
> > > > > >
> > > > > > .
> > > > >
> > > > > How about blocking migration for now then?
> > > >
> > > > The device here (vhost-user-input) blocks migration (unmigratable = 1)
> > >
> > > Right. But that device is just an excersize, right?
> >
> > Mostly
> >
> > > It bothers me that next device might not remember and
> > > we will get a mess.
> >
> > The next device (the one I care most about) is vhost-user-gpu.
> >
> > > Could we make it somehow that if there is no vmsd
> > > then migration is blocked?
> >
> > Where would you do that? in DeviceClass? That might break other
> > devices, needs code review. In VirtIODevice? that would be probably
> > simpler.
>
> In vhost user core somehow?

I suppose something like that:

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d51d1087f6..5680dc5d8e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1470,12 +1470,16 @@ static int vhost_user_backend_init(struct
vhost_dev *dev, void *opaque)
         }
     }

-    if (dev->migration_blocker == NULL &&
-        !virtio_has_feature(dev->protocol_features,
-                            VHOST_USER_PROTOCOL_F_LOG_SHMFD)) {
-        error_setg(&dev->migration_blocker,
-                   "Migration disabled: vhost-user backend lacks "
-                   "VHOST_USER_PROTOCOL_F_LOG_SHMFD feature.");
+    if (dev->migration_blocker == NULL) {
+        if (!virtio_has_feature(dev->protocol_features,
+                                VHOST_USER_PROTOCOL_F_LOG_SHMFD)) {
+            error_setg(&dev->migration_blocker,
+                       "Migration disabled: vhost-user backend lacks "
+                       "VHOST_USER_PROTOCOL_F_LOG_SHMFD feature.");
+        } else if (!qdev_get_vmsd(DEVICE(dev->vdev))) {
+            error_setg(&dev->migration_blocker,
+                       "Migration disabled: vhost-user device lacks VMSD");
+        }
     }

Unfortunately, vdev is not set before vhost_dev_start().

We could add the migration blocker there somehow? There is no
dedicated backend callback for that (I don't think set_features(),
set_mem_table()  etc are good places..)

Looking for help here, this is currently blocking me on
vhost-user-input/vhost-user-gpu.

-- 
Marc-André Lureau