Re: [PATCH V3 Resend] virtio-transport: Clarify requirements

Hi Viresh,
I haven't followed v1 and v2, so apologies in advance if my comments 
have already been discussed.

On Mon, May 20, 2024 at 02:59:40PM GMT, Viresh Kumar wrote:
>The virtio documentation currently doesn't define any generic
>requirements that are applicable to all transports. They can be useful
>while adding support for a new transport.
>
>This commit tries to define the same.
>
>Reviewed-by: Alex Bennée <[email protected]>
>Signed-off-by: Viresh Kumar <[email protected]>
>---
>V2->V3:
>- Minor fixes.
>- Added Reviewed by from Alex.
>
>V1->V2:
>- Lot of changes after discussions with Alex and Cornelia.
>- Almost a rewrite of the first commit.
>- Add Transport normative sections.
>
> commands.tex    |  3 +-
> conformance.tex | 14 +++++++++
> content.tex     | 78 +++++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 92 insertions(+), 3 deletions(-)
>
>diff --git a/commands.tex b/commands.tex
>index 25ea8ee3bc78..b64b14424bd2 100644
>--- a/commands.tex
>+++ b/commands.tex
>@@ -7,7 +7,8 @@
> % How we format a field name
> \newcommand{\field}[1]{\emph{#1}}
>
>-% Mark a normative section (driver or device)
>+% Mark a normative section (driver or device, or transport)
>+\newcommand{\transportnormative}[3]{#1{Transport Requirements: #2}\label{transportnormative:#3}}
> \newcommand{\drivernormative}[3]{#1{Driver Requirements: #2}\label{drivernormative:#3}}
> \newcommand{\devicenormative}[3]{#1{Device Requirements: #2}\label{devicenormative:#3}}
> \newcounter{clausecounter}
>diff --git a/conformance.tex b/conformance.tex
>index dc00e84e75ae..4a873169ce63 100644
>--- a/conformance.tex
>+++ b/conformance.tex
>@@ -11,6 +11,10 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>
> Conformance targets:
> \begin{description}
>+\item[Transport] A transport MUST conform to one conformance clauses:
>+  \begin{itemize}
>+    \item Clause \ref{sec:Conformance / Transport Conformance}.
>+  \end{itemize}
> \item[Driver] A driver MUST conform to four conformance clauses:
>   \begin{itemize}
>     \item Clause \ref{sec:Conformance / Driver Conformance}.
>@@ -66,6 +70,14 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>   \end{itemize}
> \end{description}
>
>+\conformance{\section}{Transport Conformance}\label{sec:Conformance / Transport Conformance}
>+
>+A transport MUST conform to the following normative statements:
>+
>+\begin{itemize}
>+\item \ref{transportnormative:Virtio Transport Options / Virtio Transport Requirements}
>+\end{itemize}
>+
> \conformance{\section}{Driver Conformance}\label{sec:Conformance / 
> Driver Conformance}
>
> A driver MUST conform to the following normative statements:
>@@ -93,6 +105,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> \item \ref{drivernormative:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Sending Available Buffer Notifications}
> \item \ref{drivernormative:General Initialization And Device Operation / Device Initialization}
> \item \ref{drivernormative:General Initialization And Device Operation / Device Cleanup}
>+\item \ref{drivernormative:Virtio Transport Options / Virtio Transport Requirements}
> \item \ref{drivernormative:Reserved Feature Bits}
> \end{itemize}
>
>@@ -172,6 +185,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Table}
> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Packed Virtqueues / Scatter-Gather Support}
> \item \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory Regions}
>+\item \ref{devicenormative:Virtio Transport Options / Virtio Transport Requirements}
> \item \ref{devicenormative:Reserved Feature Bits}
> \end{itemize}
>
>diff --git a/content.tex b/content.tex
>index 0a62dce5f65f..a79993b5ed69 100644
>--- a/content.tex
>+++ b/content.tex
>@@ -631,8 +631,82 @@ \section{Device Cleanup}\label{sec:General Initialization And Device Operation /
>
> \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>
>-Virtio can use various different buses, thus the standard is split
>-into virtio general and bus-specific sections.
>+Devices and drivers can use different transport methods to enable
>+interaction, for example PCI, MMIO, or Channel I/O. The transport
>+methods define various aspects of the communication between the device
>+and the driver, like device discovery, exchanging capabilities,
>+interrupt handling, data transfer, etc. For example, in a host/guest
>+architecture, the host might expose a device to the guest on a PCI bus,
>+and the guest will use a PCI-specific driver to interact with it.
>+
>+The standard is split into sections describing general virtio
>+implementation and transport-specific sections.
>+
>+\section{Virtio Transport Requirements}\label{sec:Virtio Transport Options / Virtio Transport Requirements}
>+
>+\transportnormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options / Virtio Transport Requirements}
>+The transport MUST provide a mechanism for the driver to discover the
>+device.

This seems a little too stringent. For example, does MMIO offer a 
discovery mechanism? IIRC we have to pass the address via device tree or 
kernel command line.

>+
>+The transport MUST provide a mechanism for the driver to identify the
>+device type.
>+
>+The transport MUST provide a mechanism for communicating virtqueue
>+configurations between the device and the driver.
>+
>+The transport MUST allow multiple virtqueues per device. The number of
>+virtqueues for a pair of device-driver are governed by the individual
>+device protocol.
>+
>+The transport MUST provide a mechanism that the device and the driver
>+use to access memory for implementing virtqueues.
>+
>+The transport MUST provide a mechanism for the device to notify the
>+driver and a mechanism for the driver to notify the device, for example
>+regarding availability of a buffer on the virtqueue.
>+
>+The transport MAY provide a mechanism for the driver to initiate a
>+reset of the virtqueues and device.
>+
>+The transport SHOULD provide a mechanism for the driver to read the
>+device status. The transport MUST provide a mechanism for the driver to
>+change the device status.
>+
>+The transport MUST provide a mechanism to implement config space 

"configuration space"

>between
>+the device and the driver.
>+
>+\devicenormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options / Virtio Transport Requirements}
>+
>+The device MUST keep any data associated with a device-initiated
>+transaction accessible to the driver until the driver acknowledges the
>+transaction to be complete.
>+
>+The device MUST NOT access the contents of a virtqueue before the
>+driver notifies, in a transport defined way, the device that the
>+virtqueue is ready to be accessed.
>+
>+The device MUST NOT access or modify buffers on a virtqueue after it has
>+notified the driver about their availability.
>+
>+The device MUST reset the virtqueues if requested by the driver, in a

Should we also talk about the possibility of resetting the entire 
device?

Thanks,
Stefano

>+transport defined way, if the transport provides such a method.
>+
>+\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options / Virtio Transport Requirements}
>+
>+The driver MUST acknowledge device notifications, as mandated by the
>+transport.
>+
>+The driver MUST NOT access virtqueue contents before the device notifies
>+about the readiness of the same.
>+
>+The driver MUST NOT access buffers, after it has added them to the
>+virtqueue and notified the device about their availability. The driver
>+MAY access them after the device has processed them and notified the
>+driver of their availability, in a transport defined way.
>+
>+The driver MAY ask the device to reset the virtqueues if, for example,
>+the driver times out waiting for a notification from the device for a
>+previously queued request.
>
> \input{transport-pci.tex}
> \input{transport-mmio.tex}
>-- 
>2.31.1.272.g89b43f80a514
>
>