Re: [PATCH] virtio-transport: Clarify requirements

[Viresh Kumar <viresh.kumar@linaro.org>]

Ping.

On 06-12-23, 15:13, Viresh Kumar wrote:
> On 05-12-23, 14:18, Cornelia Huck wrote:
> > On Tue, Dec 05 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > diff --git a/content.tex b/content.tex
> 
> > 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."
> 
> Looks good.
> 
> > > +\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?
> 
> Fair point. I think it would be okay to use MUST (NOT) if we are sure
> that the existing implementations are fulfilling the requirements.
> Else maybe we can use SHOULD (NOT) ?
> 
> > > +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.
> 
> I liked Alex's suggestion, how about that ?
> 
> "A device initiated transaction can isn't considered complete until
> acknowledged by the driver. As such data MUST remain visible to the
> driver until the transaction is complete"
> 
> > > +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?
> 
> Yes, some of this is picked from those places only. The purpose of
> this patch is to get it all at a single place, which would be easier
> for people to go through. Especially while introducing new transports
> or device specifications.
> 
> > > +\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.
> 
> Hmm, you are right about the guest/host terminology. This comes mostly
> from the MMIO transport section, where a guest presents 0x100 bytes of
> memory, followed by configuration space (whose length is device/driver
> dependent). The MMIO section has this:
> 
> "The driver MUST NOT access memory locations not described in the
> table \ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}
> (or, in case of the configuration space, described in the device specification),
> MUST NOT write to the read-only registers (direction R) and
> MUST NOT read from the write-only registers (direction W)."
> 
> > > +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?
> 
> Yes, based again on the above MMIO section, but perhaps apply to other
> transports as well..
> 
> > > +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.
> 
> What about:
> 
> The driver SHOULD acknowledge events presented by the device, as
> mandated by the transport, unless a new event from the device
> overrides the previous one or 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
> 
> Right, but having it all at a single place makes it more convenient,
> instead of looking at implementations.
> 
> > 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.)
> 
> Hmm, yeah sounds good. How about these ?
> 
> - The transport MUST provide a mechanism for device discovery at the
>   driver's end.
> 
> - The transport must provide a mechanism for the driver to identify
>   the device type.
> 
> - The transport MUST provide a mechanism for the driver to share
>   virtqueue configurations with the device.
> 
> - The transport SHOULD 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 shared memory regions between the device
>   and the driver for the implementation of virtqueues.
> 
> - The transport MUST provide a mechanism for the device and the driver
>   to notify each other, for example about availability of a buffer on
>   the virtqueue.
> 
> - The transport SHOULD provide a mechanism for the driver to initiate
>   a reset of the virtqueues and device.

-- 
viresh