[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [RFC PATCH v2] docs/interop: define PROBE feature for vhost-user VirtIO devices
On Fri, Sep 01, 2023 at 12:00:18PM +0100, Alex Bennée wrote:
> Currently QEMU has to know some details about the VirtIO device
> supported by a vhost-user daemon to be able to setup the guest. This
> makes it hard for QEMU to add support for additional vhost-user
> daemons without adding specific stubs for each additional VirtIO
> device.
>
> This patch suggests a new feature flag (VHOST_USER_PROTOCOL_F_PROBE)
> 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.
>
> Together with the existing features VHOST_USER_PROTOCOL_F_STATUS and
> VHOST_USER_PROTOCOL_F_CONFIG we can create "standalone" vhost-user
> daemons which are capable of handling all aspects of the VirtIO
> transactions with only a generic stub on the QEMU side. These daemons
> can also be used without QEMU in situations where there isn't a full
> VMM managing their setup.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
I think the mindset for this change should be "vhost-user is becoming a
VIRTIO Transport". VIRTIO Transports have a reasonably well-defined
feature set in the VIRTIO specification. The goal should be to cover
every VIRTIO Transport operation via vhost-user protocol messages so
that the VIRTIO device model can be fully conveyed over vhost-user.
Anything less is yet another ad-hoc protocol extension that will lead to
more bugs and hacks when it turns out some VIRTIO devices cannot be
expressed due to limitations in the protocol.
This requires going through the VIRTIO spec to find a correspondence
between virtio-pci/virtio-mmio/virtio-ccw's interfaces and vhost-user
protocol messages. In most cases vhost-user already offers messages and
your patch adds more of what is missing. I think this effort is already
very close but missing the final check that it really matches the VIRTIO
spec.
Please do the comparison against the VIRTIO Transports and then adjust
this patch to make it clear that the back-end is becoming a full-fledged
VIRTIO Transport:
- The name of the patch series should reflect that.
- The vhost-user protocol feature should be named F_TRANSPORT.
- The messages added in this patch should have a 1:1 correspondence with
the VIRTIO spec including using the same terminology for consistency.
Sorry for the hassle, but I think this is a really crucial point where
we have the chance to make vhost-user work smoothly in the future...but
only if we can faithfully expose VIRTIO Transport semantics.
>
> ---
> v2
> - dropped F_STANDALONE in favour of F_PROBE
> - split probe details across several messages
> - probe messages don't automatically imply a standalone daemon
> - add wording where probe details interact (F_MQ/F_CONFIG)
> - define VMM and make clear QEMU is only one of many potential VMMs
> - reword commit message
> ---
> docs/interop/vhost-user.rst | 90 ++++++++++++++++++++++++++++++++-----
> hw/virtio/vhost-user.c | 8 ++++
> 2 files changed, 88 insertions(+), 10 deletions(-)
>
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 5a070adbc1..ba3b5e07b7 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -7,6 +7,7 @@ Vhost-user Protocol
> ..
> Copyright 2014 Virtual Open Systems Sarl.
> Copyright 2019 Intel Corporation
> + Copyright 2023 Linaro Ltd
> Licence: This work is licensed under the terms of the GNU GPL,
> version 2 or later. See the COPYING file in the top-level
> directory.
> @@ -27,17 +28,31 @@ The protocol defines 2 sides of the communication, *front-end* and
> *back-end*. The *front-end* is the application that shares its virtqueues, in
> our case QEMU. The *back-end* is the consumer of the virtqueues.
>
> -In the current implementation QEMU is the *front-end*, and the *back-end*
> -is the external process consuming the virtio queues, for example a
> -software Ethernet switch running in user space, such as Snabbswitch,
> -or a block device back-end processing read & write to a virtual
> -disk. In order to facilitate interoperability between various back-end
> -implementations, it is recommended to follow the :ref:`Backend program
> -conventions <backend_conventions>`.
> +In the current implementation a Virtual Machine Manager (VMM) such as
> +QEMU is the *front-end*, and the *back-end* is the external process
> +consuming the virtio queues, for example a software Ethernet switch
> +running in user space, such as Snabbswitch, or a block device back-end
> +processing read & write to a virtual disk. In order to facilitate
> +interoperability between various back-end implementations, it is
> +recommended to follow the :ref:`Backend program conventions
> +<backend_conventions>`.
>
> The *front-end* and *back-end* can be either a client (i.e. connecting) or
> server (listening) in the socket communication.
>
> +Probing device details
> +----------------------
> +
> +Traditionally the vhost-user daemon *back-end* shares configuration
> +responsibilities with the VMM *front-end* which needs to know certain
> +key bits of information about the device. This means the VMM needs to
> +define at least a minimal stub for each VirtIO device it wants to
> +support. If the daemon supports the right set of protocol features the
> +VMM can probe the daemon for the information it needs to setup the
> +device.
"... without a per-device stub in the VMM"
This makes it clear that this sentence is describing an alternative
to the per-device stub in the VMM.
> See :ref:`Probing features for standalone daemons
> +<probing_features>` for more details.
The current section is named "Probing device details" and one being
reference is called "Probing features for standalone daemons". Are
"features" or "device details" two terms for the same thing? Why
"daemons" and not "back-end"?
I suggest calling this section "Standalone back-ends" and the other
section "Probing standalone back-ends" to keep the terminology
consistent.
> +
> +
> Support for platforms other than Linux
> --------------------------------------
>
> @@ -316,6 +331,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``)
F_STANDALONE vs F_PROBE
"SPECS" vs "features" vs "details".
Please be consistent.
>
> .. seealso::
>
> @@ -396,9 +412,10 @@ must support changing some configuration aspects on the fly.
> Multiple queue support
> ----------------------
>
> -Many devices have a fixed number of virtqueues. In this case the front-end
> -already knows the number of available virtqueues without communicating with the
> -back-end.
> +Many devices have a fixed number of virtqueues. In this case the
> +*front-end* usually already knows the number of available virtqueues
> +without communicating with the back-end. For standalone daemons this
"Usually" is vague. It's possible to be precise:
In this case a front-end that is aware of the device type already
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
knows the number of available virtqueues without communicating with
the back-end.
> +number can be can be probed with the ``VHOST_USER_GET_MIN_VQ`` message.
Then this sentence can be adjusted to:
When the front-end is not aware of the device type, the number can be
probed with the ``VHOST_USER_GET_MIN_VQ`` message.
>
> Some devices do not have a fixed number of virtqueues. Instead the maximum
> number of virtqueues is chosen by the back-end. The number can depend on host
> @@ -885,6 +902,23 @@ 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_PROBE 18
> +
> +.. _probing_features:
> +
> +Probing features for standalone daemons
> +---------------------------------------
> +
> +The protocol feature ``VHOST_USER_PROTOCOL_F_PROBE`` enables a number
> +of additional messages which allow the *front-end* to probe details
> +about the VirtIO device from the *back-end*. However for a *back-end*
> +to be described as standalone it must also support:
> +
> + * ``VHOST_USER_PROTOCOL_F_STATUS``
> + * ``VHOST_USER_PROTOCOL_F_CONFIG`` (if there is a config space)
> +
> +which are required to ensure the *back-end* daemon can operate
> +without the *front-end* managing some aspects of its configuration.
>
> Front-end message types
> -----------------------
> @@ -1440,6 +1474,42 @@ Front-end message types
> query the back-end for its device status as defined in the Virtio
> specification.
>
> +``VHOST_USER_GET_DEVICE_ID``
> + :id: 41
> + :request payload: N/A
> + :reply payload: ``u32``
> +
> + When the ``VHOST_USER_PROTOCOL_F_PROBE`` protocol feature has been
> + successfully negotiated, this message is submitted by the front-end
> + to query what VirtIO device the back-end support. This is intended
> + to remove the need for the front-end to know ahead of time what the
> + VirtIO device the backend emulates is.
"... VIRTIO device type that the backend emulates is."
"Device type" is the name used in the VIRTIO spec.
> +
> +``VHOST_USER_GET_CONFIG_SIZE``
> + :id: 42
> + :request payload: N/A
> + :reply payload: ``u32``
> +
> + When the ``VHOST_USER_PROTOCOL_F_PROBE`` protocol feature has been
> + successfully negotiated, this message is submitted by the front-end
> + to query the size of the VirtIO device's config space. This is
> + intended to remove the need for the front-end to know ahead of time
> + what the size is. Replying with 0 when
> + ``VHOST_USER_PROTOCOL_F_CONFIG`` has been negotiated would indicate
> + an bug.
"a bug"
What is the harm in returning 0 when the device has an empty
Configuration Space like the Entropy device, the I2C Adapter, the SCMI
device, etc?
> +
> +``VHOST_USER_GET_MIN_VQ``
> + :id: 43
> + :request payload: N/A
> + :reply payload: ``u32``
> +
> + When the ``VHOST_USER_PROTOCOL_F_PROBE`` protocol feature has been
> + successfully negotiated, this message is submitted by the front-end to
> + query minimum number of VQ's required to support the device. A
> + device may support more than this number of VQ's if it advertises
> + the ``VHOST_USER_PROTOCOL_F_MQ`` protocol feature. Reporting a
> + number greater than the result of ``VHOST_USER_GET_QUEUE_NUM`` would
> + indicate a bug.
What is the purpose of this message? I don't see an equivalent in the
VIRTIO specification.
>
> Back-end message types
> ----------------------
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 8dcf049d42..4d433cdf2b 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -202,6 +202,13 @@ typedef struct VhostUserInflight {
> uint16_t queue_size;
> } VhostUserInflight;
>
> +typedef struct VhostUserBackendSpecs {
> + uint32_t device_id;
> + uint32_t config_size;
> + uint32_t min_vqs;
> + uint32_t max_vqs;
> +} VhostUserBackendSpecs;
This message is undocumented? I think it may be outdated and you split
it up into individual messages.
> +
> typedef struct {
> VhostUserRequest request;
>
> @@ -226,6 +233,7 @@ typedef union {
> VhostUserCryptoSession session;
> VhostUserVringArea area;
> VhostUserInflight inflight;
> + VhostUserBackendSpecs specs;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
> --
> 2.39.2
>
Attachment:
signature.asc
Description: PGP signature
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]