Re: [virtio-dev] [RFC PATCH] docs/interop: define STANDALONE protocol feature for vhost-user

[Stefano Garzarella]

On Tue, Jul 04, 2023 at 04:02:42PM +0100, Alex Bennée wrote:
> Stefano Garzarella <sgarzare@redhat.com> writes:
>
> > On Tue, Jul 04, 2023 at 01:36:00PM +0100, Alex Bennée wrote:
> > > Currently QEMU has to know some details about the back-end to be able
> > > to setup the guest. While various parts of the setup can be delegated
> > > to the backend (for example config handling) this is a very piecemeal
> > > approach.
> > >
> > > This patch suggests a new feature flag (VHOST_USER_PROTOCOL_F_STANDALONE)
> > > which the back-end can advertise which allows a probe message to be
> > > sent to get all the details QEMU needs to know in one message.
> > >
> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > >
> > > ---
> > > Initial RFC for discussion. I intend to prototype this work with QEMU
> > > and one of the rust-vmm vhost-user daemons.
> >
> > Thanks for starting this discussion!
> >
> > I'm comparing with vhost-vdpa IOCTLs, so my questions may be
> > superficial, but they help me understand the differences.
>
> I did have a quick read-through to get a handle on vhost-vdpa but the
> docs are fairly empty. However I see there is more detail in the linux
> commit so after looking at that I do wonder:
>
> * The kernel commit defines a subset of VIRTIO_F feature flags. Should
>   we do the same for this interface?

Sorry, I didn't get this.

Do you mean that the kernel is filtering some features?
Or are you talking about backend features?

> * The VDPA GET/SET STATUS and GET/SET CONFIG ioctls are already covered
>   by the equivalent VHOST_USER messages?

Yep, I think so.

> > > ---
> > > docs/interop/vhost-user.rst | 37 +++++++++++++++++++++++++++++++++++++
> > > hw/virtio/vhost-user.c      |  8 ++++++++
> > > 2 files changed, 45 insertions(+)
> > >
> > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> > > index 5a070adbc1..85b1b1583a 100644
> > > --- a/docs/interop/vhost-user.rst
> > > +++ b/docs/interop/vhost-user.rst
> > > @@ -275,6 +275,21 @@ Inflight description
> > >
> > > :queue size: a 16-bit size of virtqueues
> > >
> > > +Backend specifications
> > > +^^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > ++-----------+-------------+------------+------------+
> > > +| device id | config size |   min_vqs  |   max_vqs  |
> > > ++-----------+-------------+------------+------------+
> > > +
> > > +:device id: a 32-bit value holding the VirtIO device ID
> > > +
> > > +:config size: a 32-bit value holding the config size (see 
> > > ``VHOST_USER_GET_CONFIG``)
> > > +
> > > +:min_vqs: a 32-bit value holding the minimum number of vqs supported
> >
> > Why do we need the minimum?
>
> We need to know the minimum number because some devices have fixed VQs
> that must be present.

But does QEMU need to know this?

Or is it okay that the driver will then fail in the guest if there
are not the right number of queues?

> > > +
> > > +:max_vqs: a 32-bit value holding the maximum number of vqs supported, must be 
> > > >= min_vqs
> >
> > Is this overlap with VHOST_USER_GET_QUEUE_NUM?
>
> Yes it does and I considered implementing a bunch of messages to fill in
> around what we already have. However that seemed like it would add a
> bunch of complexity to the interface when we could get all the initial
> data in one go.

Yes I understand, though if we need to add new things to return in the
future how do we do it? Do we need to provide features for this
structure?

> > > +
> > > C structure
> > > -----------
> > >
> > > @@ -296,6 +311,7 @@ In QEMU the vhost-user message is implemented with the 
> > > following struct:
> > >           VhostUserConfig config;
> > >           VhostUserVringArea area;
> > >           VhostUserInflight inflight;
> > > +          VhostUserBackendSpecs specs;
> > >       };
> > >   } QEMU_PACKED VhostUserMsg;
> > >
> > > @@ -316,6 +332,7 @@ replies. Here is a list of the ones that do:
> > > * ``VHOST_USER_GET_VRING_BASE``
> > > * ``VHOST_USER_SET_LOG_BASE`` (if ``VHOST_USER_PROTOCOL_F_LOG_SHMFD``)
> > > * ``VHOST_USER_GET_INFLIGHT_FD`` (if ``VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD``)
> > > +* ``VHOST_USER_GET_BACKEND_SPECS`` (if ``VHOST_USER_PROTOCOL_F_STANDALONE``)
> > >
> > > .. seealso::
> > >
> > > @@ -885,6 +902,13 @@ Protocol features
> > >   #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS  15
> > >   #define VHOST_USER_PROTOCOL_F_STATUS               16
> > >   #define VHOST_USER_PROTOCOL_F_XEN_MMAP             17
> > > +  #define VHOST_USER_PROTOCOL_F_STANDALONE           18
> > > +
> > > +Some features are only valid in the presence of other supporting
> > > +features. In the case of ``VHOST_USER_PROTOCOL_F_STANDALONE`` the
> > > +backend must also support ``VHOST_USER_PROTOCOL_F_CONFIG`` and
> > > +``VHOST_USER_PROTOCOL_F_STATUS``.
> > > +
> >
> > What about adding a new section where we will describe what we mean
> > with "standalone" devices?
> >
> > For example that the entire virtio device is emulated in the backend,
> > etc.
> >
> > By the way, I was thinking more about F_FULL_DEVICE, but I'm not good
> > with names, so I'll just throw out an idea :-)
>
> Naming things is hard ;-)

I know :-)

> I could add a new section although AIUI there is nothing really in
> existing daemons which is split between the front-end and back-end. The
> stubs are basically boilerplate and ensure DT/PCIe hubs make the device
> appear so once things are discovered QEMU never really gets involved
> aside from being a dumb relay.

For the backend I don't think there is much to say, but for the frontend
it changes a lot in my opinion if this new feature is supported, since
we basically offlaod the whole virtio device to the external process.

Thanks,
Stefano