Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Dec 06 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 05-12-23, 14:18, Cornelia Huck wrote:
>> On Tue, Dec 05 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > +\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) ?

Yes, I think if we are just spelling out things explicitly in one place
that have been required anyway (by existing transports etc.), we are
fine using MUST; everything where someone could have chosen a different
implementation needs to be SHOULD.

(...)

>> > +\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)."

Thanks for clarifying where this came from! See my suggestions in the
other fork of this thread.

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

I think we can't assume transports to use r/o and w/o memory areas in
all cases -- for example, ccw is largely based on passing buffers
around. Of course, if you look at the actual structures, there are
always fields that are to be used for reading or writing only.

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

Should we do s/events/notifications/, or do we also cover transactions
initiated by the device explicitly (I'd assume that the device would
notify the driver im- or explicitly if it has to do something?)

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

In that case, I'm fine for the first two; however, I'm not sure what the
third one adds here -- I don't think we ever said anything about when
the driver can initiate a reset, it can basically be at any time, and it
overrides whatever the device did if we reword the statement above.

>
>> 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 a driver to discover a
device" ?

>
> - The transport must provide a mechanism for the driver to identify
>   the device type.

Ack.

>
> - The transport MUST provide a mechanism for the driver to share
>   virtqueue configurations with the device.

"a mechanism for communicating virtqueue configurations between the
device and the driver" ?

That gives us more flexibility.

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

Isn't that a MUST, depending on the device type?

>
> - The transport MUST provide shared memory regions between the device
>   and the driver for the implementation of virtqueues.

We probably shouldn't talk about providing shared memory regions, but
instead mandate that the transport provides a mechanism that the device
and the driver use to access memory for implementing 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.

Probably a mechanism for the device to notify the driver, and a
mechanism for the driver to notify the device? They can be different, as
long both of them are present.

>
> - The transport SHOULD provide a mechanism for the driver to initiate
>   a reset of the virtqueues and device.

Can we mandate a mechanism for a device reset? Reset of virtqueues needs
to be optional, I'm not sure whether a SHOULD would be appropriate for
that (or maybe we should finally come up with something for ccw ;)

Other things that probably should be mandatory:

- A way for the driver to change the device status. Reading it would
  probably be a strong SHOULD.
- A way to implement config space. (For example, channel devices don't
  have a config space or anything similar enough to repurpose, so I had
  to invent a mechanism for ccw... maybe future transports will be in
  the same boat.)