All of lore.kernel.org
 help / color / mirror / Atom feed
From: Cornelia Huck <cohuck@redhat.com>
To: Viresh Kumar <viresh.kumar@linaro.org>,
	virtio-dev@lists.oasis-open.org,
	"Michael S. Tsirkin" <mst@redhat.com>
Cc: "Viresh Kumar" <viresh.kumar@linaro.org>,
	"Vincent Guittot" <vincent.guittot@linaro.org>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	stratos-dev@op-lists.linaro.org,
	"Erik Schilling" <erik.schilling@linaro.org>,
	"Manos Pitsidianakis" <manos.pitsidianakis@linaro.org>,
	"Mathieu Poirier" <mathieu.poirier@linaro.org>
Subject: [virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements
Date: Tue, 05 Dec 2023 14:18:02 +0100	[thread overview]
Message-ID: <875y1ciy9h.fsf@redhat.com> (raw)
In-Reply-To: <3fbb010e96124cfbffd70709d9ce7a2a458322c8.1701771424.git.viresh.kumar@linaro.org>

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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


  reply	other threads:[~2023-12-05 13:18 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-05 10:20 [virtio-dev] [PATCH] virtio-transport: Clarify requirements Viresh Kumar
2023-12-05 13:18 ` Cornelia Huck [this message]
2023-12-05 13:54   ` [virtio-dev] " Alex Bennée
2023-12-18 13:12     ` Cornelia Huck
2023-12-18 14:09       ` Alex Bennée
2023-12-20 12:43         ` Cornelia Huck
2023-12-06  9:43   ` Viresh Kumar
2023-12-18  7:00     ` Viresh Kumar
2023-12-18 14:02     ` Cornelia Huck
2023-12-18 14:19       ` Alex Bennée
     [not found]         ` <8b278f33-4702-4a7c-bb80-e11c316234c4@linaro.org>
2023-12-20 13:50           ` Cornelia Huck
2024-01-29 10:35       ` Viresh Kumar
2024-01-29 16:22         ` Cornelia Huck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875y1ciy9h.fsf@redhat.com \
    --to=cohuck@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=erik.schilling@linaro.org \
    --cc=manos.pitsidianakis@linaro.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=mst@redhat.com \
    --cc=stratos-dev@op-lists.linaro.org \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=virtio-dev@lists.oasis-open.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.