Re: [PATCH] virtio-transport: Clarify requirements

[Alex BennÃe <alex.bennee@linaro.org>]

Cornelia Huck <cohuck@redhat.com> writes:

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

Maybe:

"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?

I think the recent example is the virtio-sound driver continuing to feed
data into buffers after those buffers where submitted into the
virtqueue. We should be explicit that the only time both sides of a
VirtIO implementation can access things at the same time is with
explicitly shared memory (and you need some sort of mechanism to mediate
that to avoid chaos).

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

I agree guest memory isn't the right terminology here. However there are
discussions about how to implement secure buffers for VirtIO - so for
example a buffer mediated by some sort of secure layer. In those cases
the driver may not have access to it outside of the transactions. 

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

Yes I think so. The s390x channel transport gets referenced because it
has a nice enumerated list of operations. It would be good to codify
which operations are mandatory for all transports and which are
optional.

-- 
Alex BennÃe
Virtualisation Tech Lead @ Linaro