Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck <cohuck@redhat.com>]

On Tue, Dec 05 2023, Viresh Kumar <viresh.kumar@linaro.org> 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.

Thank you for tackling this, albeit the devil's in the details :)

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  content.tex | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 0a62dce5f65f..d4d5e7d7045b 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -631,8 +631,52 @@ \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.
> +The virtio devices are exposed to the guest as if they are physical
> +devices using a specific transport method, like PCI, MMIO or Channel
> +I/O.

I'm not sure we can talk about "exposed to the guest" here, except as an
example... maybe if we reword the whole paragraph (see my suggestion
below.)

> 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.. Virtio can use
> +various different buses, thus the standard is split into virtio general
> +and bus-specific sections.

I think we should concentrate on the transport being what links device
and driver together... what about (reusing parts of your writeup):

"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}
> +
> +\devicenormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}

I'm not sure we can introduce MUST (NOT) requirements for basic
functionality after the spec has been published for quite a time already
(although I'd assume every implementation is fulfilling the requirements
anyway)... thoughts?

> +
> +The device MUST present each event, in a transport defined way, from the
> +moment it takes place until the driver acknowledges the event.

I don't believe "event" is well-defined here.

> +
> +The device MUST NOT access virtqueue's contents before the driver
> +notifies that the queue is ready for access, in a transport defined way.
> +
> +The device MUST NOT access buffers on the virtqueue, after it has
> +modified them and notified the driver about their availability.
> +
> +The device MUST reset the virtqueues if requested by the driver, in a
> +transport defined way.

Isn't all of this already defined in one place of the spec or another?

> +
> +\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}
> +
> +The driver MUST NOT access guest memory locations outside what's made
> +available by the device to the driver.

I don't think that makes sense -- I'd assume most guest memory locations
do not have anything to do with virtio, and we should try to avoid
host/guest terminology.

> +
> +The driver MUST NOT write to the read-only memory area and MUST NOT read
> +from the write-only memory area.

Which memory areas does that refer to? Parts of the transport-specific
data structures?

> +
> +The driver MUST acknowledge events presented by the device, as mandated
> +by the transport.

I don't think this is quite correct in the absolute -- for example, it
should be fine to not acknowledge events if some overriding event comes
along, or if the driver initiates a reset.

> +
> +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.

Again, I believe this has already been covered in the generic
sections -- do we instead need to specify that a transport MUST provide
a method to do xy? (or SHOULD, MAY, as applicable -- it would be good to
list explicitly what is mandatory for a transport to implement, and what
is optional.)