Re: [PATCH V7] virtio-transport: Add a new section to clarify transport requirements

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 11, 2024 at 10:24:16AM +0200, Cornelia Huck wrote:
> On Thu, Jul 11 2024, 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 under a new Appendix section.
> >
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > V6->V7:
> > - Remove parts that talk about accessing content of the virtqueue.
> >
> > V5->V6:
> > - Move the changes to a new appendix section.
> > - Clarify the requirements a bit more based on review comments.
> >
> > V4->V5:
> > - s/The transport/A transport/
> > - s/MUST provide/provides/
> > - Added some text for transport requirements.
> >
> > V3->V4:
> > - Remove the normative sections and use direct speech.
> > - Change wording at few places.
> >
> > 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.
> >
> >  main.tex         |  2 ++
> >  newtransport.tex | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> >  create mode 100644 newtransport.tex
> >
> > diff --git a/main.tex b/main.tex
> > index b1913d65e964..6d337217a3d1 100644
> > --- a/main.tex
> > +++ b/main.tex
> > @@ -42,6 +42,8 @@
> >  
> >  \input{newdevice.tex}
> >  
> > +\input{newtransport.tex}
> > +
> >  % acknowledgements
> >  \input{acknowledgements.tex} 
> >  
> > diff --git a/newtransport.tex b/newtransport.tex
> > new file mode 100644
> > index 000000000000..2abd76e5b037
> > --- /dev/null
> > +++ b/newtransport.tex
> > @@ -0,0 +1,72 @@
> > +\chapter{Creating New Transports}\label{sec:Creating New Transports}
> > +
> > +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.
> > +
> > +There are some mechanisms that any transport is required to implement,
> > +and some requirements that devices and drivers are required to follow.
> > +
> > +\section{Transport Requirements}\label{sec:Creating New Transports / Transport Requirements}
> > +
> > +A transport provides a mechanism for the driver to discover the device.
> > +
> > +A transport provides a mechanism for the driver to identify the device
> > +type.
> > +
> > +A transport provides a mechanism for communicating virtqueue
> > +configurations between the device and the driver.
> > +
> > +A transport allows multiple virtqueues per device. The number of
> > +virtqueues for a pair of device-driver are governed by the individual
> > +device protocol.
> 
> Maybe "for a given device/driver pair"?
> 
> > +
> > +A transport provides a mechanism that the device and the driver use to
> > +access memory for implementing virtqueues.

Hmm not really.
At the moment virtqueues are all in driver memory, what transport
provides is a mechanism for device to locate this memory.


> > +
> > +A transport provides 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.
> > +
> > +A transport provides a mechanism for the driver to initiate a reset of
> > +the device.
> > +
> > +A transport provides a mechanism to reset an individual virtqueue.
> 
> Is that more of a requirement or a strong suggestion, given that not all
> existing transports provide this?

I think this should be "a transport can provide" in all instances where
the feature is optional. Given it's usually tied to a feature bit,
also mention that.

> > +
> > +A transport provides a mechanism for the driver to read the device
> > +status. A transport provides a mechanism for the driver to change the
> > +device status.
> > +
> > +A transport provides a mechanism to implement configuration space
> > +between the device and the driver.

so far so good.

> > +\section{Device Requirements}\label{sec:Creating New Transports / Device Requirements}
> > +
> > +The device keeps any data associated with a device-initiated transaction
> > +accessible to the driver until the driver acknowledges the transaction
> > +to be complete.

what are transactions? how does driver acknowledge the transaction to
be complete? Sounds like something Currently spec mentions transactions in the
context of i2c (and in passing, iommu) - maybe put this inside
that device description?

> > +The device resets itself if requested by the driver, in a transport
> > +defined way, if the transport provides such a method.
> > +
> > +The device resets an individual virtqueue if requested by the driver,
> > +in a transport defined way, if the transport provides such a method.
> > +
> > +\section{Driver Requirements}\label{sec:Creating New Transports / Driver Requirements}
> > +
> > +The driver acknowledges device notifications, as mandated by the
> > +transport.
> > +
> > +The driver accesses queued buffers after the device has processed them
> > +and notified the driver of their availability. This mechanism is
> > +transport defined.
> > +
> > +The driver asks the device to reset itself if, for example, the driver
> > +times out waiting for a notification from the device for a previously
> > +queued request.
> > +
> > +The driver asks the device to reset an individual virtqueue.

These two sections are weird.
What is said here that isn't already said in other sections?


> 
> This sentence is a bit weird, like the driver would randomly decide to
> reset a virtqueue :) Can we qualify this a bit?