Re: [PATCH v2 3/6] virtio-msg: Add virtio-msg, a message based virtio transport layer

["David Hildenbrand (Arm)" <david@kernel.org>]

This is pretty impressive work.

In general, this looks very good to me. Mostly nits and some questions on
added/removed events.

> +\busnormative{\paragraph}{Bus Message Implementation}{Virtio Transport Options / Virtio Over Messages / Basic Concepts / Relationship between bus and transport / Bus Messages}
> +\begin{itemize}
> +  \item A bus implementation MAY support any subset of \busref{GET_DEVICES},
> +        \busref{PING}, and \busref{EVENT_DEVICE}.
> +  \item If a bus implementation supports one of these bus messages, it MUST
> +        conform to the normative definition for that message.
> +\end{itemize}
> +
> +\busnormative{\paragraph}{Transport Message Forwarding}{Virtio Transport Options / Virtio Over Messages / Basic Concepts / Relationship between bus and transport / Transport Message Forwarding}
> +\begin{itemize}
> +  \item A bus implementation MUST validate the device number in each transport
> +        message before forwarding it.
> +  \item A bus implementation MUST relay each transport message to the device
> +        number identified in the message header, regardless of how it
> +        discovered or enumerated that device.
> +  \item A bus implementation SHOULD treat transport messages as opaque apart
> +        from enforcing generic transport limits, such as the advertised maximum
> +        message size, and SHOULD NOT modify the transport payload.
> +  \item If a bus implementation cannot validate or route a transport request
> +        that expects a response, it MUST surface a transport-visible failure
> +        for that request.
> +  \item If a bus implementation cannot validate or route a one-way transport
> +        event, it MAY drop the event or handle it according to
> +        message-specific bus requirements.
> +\end{itemize}
> +
> +\subsubsection{Known Bus Implementation Examples}
> +\label{sec:Virtio Transport Options / Virtio Over Messages / Basic Concepts / Known Bus Implementation Examples}
> +
> +This subsubsection lists informative examples of bus implementations and related
> +work for virtio-msg.
> +
> +\begin{itemize}
> +  \item \textbf{Virtio Message Bus over FF-A}: specified by Arm in
> +        \hyperref[intro:FFA-BUS]{[FFA-BUS]}.
> +  \item \textbf{Virtio Message Bus over PCI for AMP}: under development; proof
> +        of concept:
> +        \url{https://github.com/wmamills/hvac-demo}.
> +  \item \textbf{Virtio Message Bus over Xen grant tables and events}: under
> +        discussion.
> +\end{itemize}

I don't know if it's common to include such PoC/discussions in the spec. I'd
have moved that to the patch description. In any case, it's very helpful + nice
to see how this will be used.


> +\paragraph{virtio-msg transport revisions}
> +
> +The following table lists the currently defined virtio-msg transport revisions
> +and their protocol-defined maximum message sizes. Transport feature bits are
> +defined separately in
> +\ref{sec:Virtio Transport Options / Virtio Over Messages / Basic Concepts / Transport Feature Bits}.
> +
> +\begin{tabular}{ |l|l|l| }
> +\hline
> +\field{revision} & \field{maximum size} & remarks \\
> +\hline \hline
> +1        & 52-65535    & Virtio Message Revision 1 \\
> +\hline
> +\end{tabular}

Not sure if that table really adds a lot of value, hard to tell what else should
be mentioned if there is no rev 2 yet. But maybe that's just the common way of
doing it.

> +The table reflects the protocol-defined maximum size; the recommended
> +advertised maximum remains 264 bytes for interoperability.
> +
> +Note that a change in the virtio standard does not necessarily
> +correspond to a change in the virtio-msg transport revision.
> +
> +The maximum message size is specified from the transport-layer point of view
> +and includes the 8-byte common header plus payload. Any extra encapsulation
> +imposed by the underlying bus (for example, a framing header) does not count
> +against this limit. Today the largest practical inline configuration payload is
> +244 bytes: messages such as GET_CONFIG and SET_CONFIG use 12 bytes of
> +message-specific fields, leaving up to 244 bytes for device configuration data
> +in one transfer when the recommended 264-byte maximum message size is used.
> +Larger configuration regions can be accessed through multiple exchanges without
> +requiring a larger per-message limit.
> +
> +\busnormative{\paragraph}{Message Size Bounds}{Virtio Transport Options / Virtio Over Messages / Basic Concepts / Revisions / Message Size}
> +\begin{itemize}
> +  \item A bus implementation MUST advertise a maximum message size of at least
> +        52 bytes.
> +  \item A bus implementation SHOULD NOT advertise a maximum message size that
> +        exceeds 264 bytes (256-byte payload plus the common header).

Ah, that points back at the recommendation mentioned above.

> +\end{itemize}
> +
> +\paragraph{Versioning and Forward Compatibility}
> +
> +A higher transport revision or additional transport feature bits extend the
> +protocol with new messages or capabilities. Implementations are expected to
> +remain interoperable across revisions: devices and drivers designed for a newer
> +revision still implement the mandatory messages and semantics defined in prior
> +revisions.

"are expected" -- do we want to make that stricter and turn it into a MUST, or
an informal "will", or what is the reason why this phrasing was chosen?

Because below, I read a lot of "MUST" :)

[...]

> +Each virtio-msg bus instance contains zero or more \emph{devices}, identified
> +by a 16-bit \textbf{device number}. The device number is a bus-assigned
> +identifier and is distinct from the virtio device ID (device type) returned by
> +\msgref{GET_DEVICE_INFO}. 

Good.

[...]
> +The 16-bit device-number space provides roughly 65k device slots per bus
> +instance. Deployments that require more devices can instantiate multiple bus
> +instances and distribute devices across those instances.

:)

[...]

> +
> +Some bus implementations may reject requests that violate their local policy
> +or that are not valid for the current device state. In such cases, the bus may
> +return a bus-specific error indication to the transport, which can then surface
> +that failure to the driver.
> +
> +\subsubsection{Endianness}
> +\label{sec:Virtio Transport Options / Virtio Over Messages / Basic Concepts / Endianness}
> +
> +Unless otherwise stated, all numeric fields defined for virtio-msg messages use
> +little-endian encoding.

Hurray :)

[...]

> +
> +\subsubsection{Message Ordering}
> +\label{sec:Virtio Transport Options / Virtio Over Messages / Basic Concepts / Ordering}
> +
> +Transport messages fall into two classes: requests (which expect responses) and
> +events (which are one-way). Drivers and devices do not interpret the
> +\field{token} field directly. A bus implementation MAY have multiple
> +outstanding requests for the same device number and MAY use the bus-owned
> +\field{token} field to correlate each response with its originating request.
> +The device copies the received request token into the matching response, and

yeah, that's a clear MUST. Is that covered somewhere and I missed it? (it's a
lot of text for a Friday evening ...)?

[...]


> +\msgdef{GET_SHM}
> +
> +\msgref{GET_SHM} returns the location and size of a device-owned shared memory
> +region. The request carries the shared-memory region identifier
> +\field{shmid}; the response echoes \field{shmid} and provides the base
> +physical \field{address} and \field{length}. A \field{length} of zero
> +indicates that the specified \field{shmid} is not supported by the device.
> +
> +\begin{lstlisting}
> +struct virtio_msg_get_shm_req {
> +        le32 shmid; /* shared memory region identifier */
> +};
> +
> +struct virtio_msg_get_shm_resp {
> +        le32 shmid;     /* echoed region identifier */
> +        le32 reserved;  /* must be zero */
> +        le64 length;    /* region length (0 if nonexistent) */
> +        le64 address;   /* region physical address */
> +};
> +\end{lstlisting}
> +
> +\devicenormative{\paragraph}{GET\_SHM}{Virtio Transport Options / Virtio Over Messages / Transport Messages / VIRTIO_MSG_GET_SHM / Device}
> +\begin{itemize}
> +  \item A device MUST return zero \field{length} if the requested
> +        \field{shmid} is not supported and MUST report accurate physical
> +        \field{address}/\field{length} information for regions it supports.
> +  \item A device MUST set the \field{reserved} field in \msgref{GET_SHM}
> +        responses to zero.
> +\end{itemize}

Don't we need some statements that the device MUST echo the shmid etc?

[...]

> +
> +\drivernormative{\paragraph}{EVENT\_AVAIL}{Virtio Transport Options / Virtio Over Messages / Transport Messages / VIRTIO_MSG_EVENT_AVAIL / Driver}
> +\begin{itemize}
> +  \item A driver MUST identify the target virtqueue in \field{vq_index}.
> +  \item If VIRTIO\_F\_NOTIFICATION\_DATA has not been negotiated, a driver MUST
> +        set \field{next_offset} to zero in \msgref{EVENT_AVAIL} messages. If it
> +        has been negotiated, the driver MUST encode core \field{next_off} in
> +        \field{next_offset}[30:0] and core \field{next_wrap} in
> +        \field{next_offset}[31], using split-queue or packed-queue derivation
> +        as defined by core virtio.
> +\end{itemize}
> +
> +\busnormative{\paragraph}{EVENT\_AVAIL}{Virtio Transport Options / Virtio Over Messages / Transport Messages / VIRTIO_MSG_EVENT_AVAIL / Bus}
> +\begin{itemize}
> +  \item If a bus implementation cannot preserve
> +        VIRTIO\_F\_NOTIFICATION\_DATA semantics for \msgref{EVENT_AVAIL}, it
> +        MUST prevent negotiation of VIRTIO\_F\_NOTIFICATION\_DATA.
> +\end{itemize}

Don't we need some statements about the device setting the virtiqueue in
\field{vq_index} etc?

[...]

> +
> +\busnormative{\paragraph}{GET\_DEVICES}{Virtio Transport Options / Virtio Over Messages / Bus Messages / GET_DEVICES / Bus}
> +\begin{itemize}
> +  \item The responding bus implementation MUST interpret \field{count} as a
> +        number of
> +        device-number slots and MUST return a \field{bitmap} of
> +        \textbf{(\field{count} + 7) / 8} bytes, packed least-significant-bit
> +        first.
> +  \item A request with \field{count}=0 is valid. In response to such a request,
> +        the responding bus implementation MUST return \field{count}=0 and an
> +        empty \field{bitmap}.
> +  \item The responding bus implementation MUST NOT return a \field{count} value
> +        greater
> +        than the \field{count} value from the request.

Is that early line break intentional? I assume not :) (latex won't care)

> +  \item If \field{count} is not a multiple of 8, the responder MUST set unused
> +        bits in the last \field{bitmap} byte (positions at or above
> +        \field{count}) to zero.
> +  \item For a terminal response, the responding bus implementation MUST set
> +        \field{next\_offset} to 0.
> +  \item For a nonterminal response, the responding bus implementation MUST set
> +        \field{next\_offset} to a value strictly greater than the request
> +        \field{offset}.
> +  \item Responders SHOULD return the requested \field{count} unless constrained
> +        (e.g., by maximum message size) or when trailing parts of the requested
> +        window contain no present device numbers; if a smaller \field{count} is
> +        returned, the \field{bitmap} MUST still represent the window defined by
> +        the echoed \field{offset} and \field{count}.
> +  \item If no devices are present in the requested window, a responder MAY
> +        return \field{count}=0 with an empty \field{bitmap}.
> +\end{itemize}
> +
> +\busdef{PING}
> +
> +\busref{PING} is a simple liveness check that can be sent by either side of the
> +bus; the response echoes the 32-bit data value from the request.

Why would one need a liveness check and when do we expect them to be used? How
frequently?

> +
> +\begin{lstlisting}
> +struct virtio_bus_msg_ping_req {
> +        le32 data; /* arbitrary value chosen by the sender */
> +};
> +
> +struct virtio_bus_msg_ping_resp {
> +        le32 data; /* echoed request data */
> +};
> +\end{lstlisting}
> +
> +Drivers or bus implementations can use \busref{PING} as a liveness check. If a
> +reply is missing or delayed beyond local policy, the initiator can verify
> +device status (for example, with \msgref{GET_DEVICE_STATUS}) and perform
> +recovery as needed.
> +
> +\busnormative{\paragraph}{PING}{Virtio Transport Options / Virtio Over Messages / Bus Messages / PING / Bus}
> +\begin{itemize}
> +  \item The responding bus implementation MUST echo the 32-bit data field from
> +        the request exactly in the \busref{PING} response.
> +\end{itemize}
> +
> +\busdef{EVENT_DEVICE}
> +
> +\busref{EVENT_DEVICE} signals device-level hotplug changes as a bus-originated
> +control-plane event. The bus instance on the driver side receives these
> +indications from the peer bus instance; the table below lists the defined
> +device bus states.
> +
> +\begin{lstlisting}
> +struct virtio_bus_msg_event_device {
> +        le16 device_number;
> +        le16 device_bus_state; /* see table below */
> +};
> +\end{lstlisting}
> +
> +\begin{tabular}{|l|l|p{7cm}|}
> +\hline
> +Constant & Value & Meaning \\
> +\hline
> +DEVICE\_BUS\_STATE\_ADDED    & 0x0001 & Device is present and ready. \\
> +DEVICE\_BUS\_STATE\_REMOVED  & 0x0002 & Device is no longer present. \\

Is there no "REMOVING" or "Request for removal" request mechanism?

Other unplug mechanisms (iirc ACPI) avoid this kind of surprise removal, as it's
not particularly nice to handle.

I guess for such surprise removal, any concurrent message to the device will
silently get lost? Or how is that supposed to work?


> +Reserved                    & 0x0003-0x7FFF & Reserved for standard use. \\
> +Implementation-defined      & 0x8000-0xFFFF & MAY be used by the bus implementation. \\
> +\hline
> +\end{tabular}
> +
> +Upon receiving DEVICE\_BUS\_STATE\_ADDED, drivers typically probe the device
> +with \msgref{GET_DEVICE_INFO} before continuing initialization. Upon receiving
> +DEVICE\_BUS\_STATE\_REMOVED, drivers typically stop queueing new work, quiesce
> +the device instance, tear down local driver state, and release resources.
> +Duplicate or out-of-order events can occur and can be handled with additional
> +bus-level monitoring, such as \busref{PING}.

What if we get a fast sequence of ADDED+REMOVE, REMOVING the device while still
probing the driver etc? Isn't that a concern if it's architected like that?

> +
> +\busnormative{\paragraph}{EVENT\_DEVICE}{Virtio Transport Options / Virtio Over Messages / Bus Messages / EVENT_DEVICE / Bus}
> +\begin{itemize}
> +  \item A bus implementation that sends \busref{EVENT_DEVICE} MUST set
> +        \field{device\_number} to a valid bus device number exposed to the
> +        driver side.
> +  \item A bus implementation that sends \busref{EVENT_DEVICE} with
> +        DEVICE\_BUS\_STATE\_ADDED MUST do so only when the referenced device is
> +        accessible for transport-message processing.

Right, but what prevents immediate removal?


-- 
Cheers,

David