[virtio-dev] [PATCH] virtio-transport: Clarify requirements

[virtio-dev] [PATCH] virtio-transport: Clarify requirements

[Viresh Kumar]

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.

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. 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.
+
+\section{Virtio Transport Requirements}\label{sec:Virtio Transport Options / Virtio Transport Requirements}
+
+\devicenormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}
+
+The device MUST present each event, in a transport defined way, from the
+moment it takes place until the driver acknowledges the event.
+
+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.
+
+\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.
+
+The driver MUST NOT write to the read-only memory area and MUST NOT read
+from the write-only memory area.
+
+The driver MUST acknowledge events presented by the device, as mandated
+by the transport.
+
+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.
 
 \input{transport-pci.tex}
 \input{transport-mmio.tex}
-- 
2.31.1.272.g89b43f80a514


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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck]

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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Alex Bennée]

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

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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Viresh Kumar]

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

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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Viresh Kumar]

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

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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck]

On Tue, Dec 05 2023, Alex Bennée <alex.bennee@linaro.org> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
>
>> On Tue, Dec 05 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> +
>>> +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"?

Transaction is good, what about:

"Any data associated with a device-initiated transaction MUST remain
accessible to the driver until the driver acknowledges the transaction
to be 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).

Fair enough, let's make it explicit if people already stumbled
here. Some rewording suggestions:

"The device MUST NOT access the contents of a virtqueue before the
driver notifies, in a transport defined way, the device that the
virtqueue is ready to be accessed.

The device MUST NOT access or modify buffers on a virtqueue after it has
notified the driver about their availability.

The device MUST reset the virtqueues if requested, in a transport
defined way, by the driver."

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

Yes, I think we need to limit the scope of "guest memory" here. I think
we are basically wanting to deal with any memory used by virtio (device
type including memory access controlled by it, transport, and the
protocol itself). We would be talking about memory made available to the
device by the driver for explicit usage to implement the virtio spec. I
think this would cover mediation by a secure layer as well (with the
driver calling into that secure layer?) Or does the (host) device end up
donating memory to the (guest) driver, and we need to make sure it
doesn't scribble over it?

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

The problem with the ccw transport is that while it has a nice list of
operations, (a) it only covers guest-initiated actions, (b) probably not
all of them shold be mandatory (and some of them are more of an artifact
of how channel I/O works), and (c) it only implements a subset of the
defined operations (which makes the not-implemented ones de facto
optional, of course :) But yes, we could use it as a starting point.


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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck]

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


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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Alex Bennée]

Cornelia Huck <cohuck@redhat.com> writes:

> On Tue, Dec 05 2023, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>> Cornelia Huck <cohuck@redhat.com> writes:
>>
>>> On Tue, Dec 05 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>> +
>>>> +The device MUST present each event, in a transport defined way, from the
>>>> +moment it takes place until the driver acknowledges the event.
>>>
<snip>
>>>> +
>>>> +\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. 
>
> Yes, I think we need to limit the scope of "guest memory" here. I think
> we are basically wanting to deal with any memory used by virtio (device
> type including memory access controlled by it, transport, and the
> protocol itself). We would be talking about memory made available to the
> device by the driver for explicit usage to implement the virtio spec. I
> think this would cover mediation by a secure layer as well (with the
> driver calling into that secure layer?) Or does the (host) device end up
> donating memory to the (guest) driver, and we need to make sure it
> doesn't scribble over it?

I'm not sure if we have example of the host donating memory apart from
the sort of static partitioning we see with guests on start-up where a
region is defined as shared. The Xen grant model leaves the guest to
grant access to its own pages to the backend.

I guess for firmware mediated sharing this would still be driven by the
guest rather than the host?

>
>>>> +
>>>> +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.
>
> The problem with the ccw transport is that while it has a nice list of
> operations, (a) it only covers guest-initiated actions,

What examples of host initiated actions are there (aside from an IPI
indicating a receive VirtQueue has buffers waiting)?

> (b) probably not
> all of them shold be mandatory (and some of them are more of an artifact
> of how channel I/O works),

These ones?

  #define CCW_CMD_SET_IND 0x43
  #define CCW_CMD_SET_CONF_IND 0x53
  #define CCW_CMD_SET_IND_ADAPTER 0x73

> and (c) it only implements a subset of the
> defined operations (which makes the not-implemented ones de facto
> optional, of course :) But yes, we could use it as a starting point.

Got to start somewhere :-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Alex Bennée]

Cornelia Huck <cohuck@redhat.com> writes:

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

I think Bill has run into problems with config space on OpenAMP setups
where there can be no specific trapping between domains making it hard
to manage a config space where both sides might want to make updates. I
guess the original MMIO transport gets away with it because config space
is always in the trap-and-emulate address space in a normal VM/emulation
situation.

Bill,

Is the plan to introduce a new transport that manages config in a
different way?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

Re: [virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck]

On Mon, Dec 18 2023, Alex Bennée <alex.bennee@linaro.org> wrote:

> Cornelia Huck <cohuck@redhat.com> writes:
>
>> On Tue, Dec 05 2023, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>> Cornelia Huck <cohuck@redhat.com> writes:
>>>
>>>> On Tue, Dec 05 2023, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>>> +
>>>>> +The device MUST present each event, in a transport defined way, from the
>>>>> +moment it takes place until the driver acknowledges the event.
>>>>
> <snip>
>>>>> +
>>>>> +\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. 
>>
>> Yes, I think we need to limit the scope of "guest memory" here. I think
>> we are basically wanting to deal with any memory used by virtio (device
>> type including memory access controlled by it, transport, and the
>> protocol itself). We would be talking about memory made available to the
>> device by the driver for explicit usage to implement the virtio spec. I
>> think this would cover mediation by a secure layer as well (with the
>> driver calling into that secure layer?) Or does the (host) device end up
>> donating memory to the (guest) driver, and we need to make sure it
>> doesn't scribble over it?
>
> I'm not sure if we have example of the host donating memory apart from
> the sort of static partitioning we see with guests on start-up where a
> region is defined as shared. The Xen grant model leaves the guest to
> grant access to its own pages to the backend.
>
> I guess for firmware mediated sharing this would still be driven by the
> guest rather than the host?

Yes, I think the guest telling the firmware which parts of its memory
the host may access is the usual pattern, or at least what I've seen so
far.

>>
>>>>> +
>>>>> +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.
>>
>> The problem with the ccw transport is that while it has a nice list of
>> operations, (a) it only covers guest-initiated actions,
>
> What examples of host initiated actions are there (aside from an IPI
> indicating a receive VirtQueue has buffers waiting)?

Also notifications for configuration changes; but I think we can put all
of this under the device->driver notification header. (Hmm, we probably
should change all those host/guest references for ccw some day...)

>
>> (b) probably not
>> all of them shold be mandatory (and some of them are more of an artifact
>> of how channel I/O works),
>
> These ones?
>
>   #define CCW_CMD_SET_IND 0x43
>   #define CCW_CMD_SET_CONF_IND 0x53
>   #define CCW_CMD_SET_IND_ADAPTER 0x73

Yes, and also CCW_CMD_SET_VIRTIO_REV (I think there used to be some
interest to implement something like that for mmio, but I don't think
anything ever ended up being specified?)

>
>> and (c) it only implements a subset of the
>> defined operations (which makes the not-implemented ones de facto
>> optional, of course :) But yes, we could use it as a starting point.
>
> Got to start somewhere :-)

Indeed :)


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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck]

On Mon, Dec 18 2023, Bill Mills <bill.mills@linaro.org> wrote:

> On 12/18/23 9:19 AM, Alex Bennée wrote:
>> Cornelia Huck <cohuck@redhat.com> writes:
>> 
>>> 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:
>> <snip>
>>>>
>>>> - 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.)
>> 
>> I think Bill has run into problems with config space on OpenAMP setups
>> where there can be no specific trapping between domains making it hard
>> to manage a config space where both sides might want to make updates. I
>> guess the original MMIO transport gets away with it because config space
>> is always in the trap-and-emulate address space in a normal VM/emulation
>> situation.
>> 
>> Bill,
>> 
>> Is the plan to introduce a new transport that manages config in a
>> different way?
>> 
>
> I/We had a couple of ideas.  This is a thorny issue if you want 
> something that 100% matches the implications of the virtio spec.  If you 
> want something that works with existing virtio protocols it is not as 
> bad but still tricky.
>
> If you read the virtio spec it implies you could have a device with zero 
> virtqueues and just does things with the config space.  To me this 
> should be a SHOULD NOT.  Something like "The config space SHOULD NOT be 
> used as a mechanism for frequent changes."
>
> To me the config space is best used as a device to driver info space 
> with infrequent updates from both sides.  I don't know how that should 
> be expressed in the spec.

We do have some text suggesting that this should work like that, i.e. in
"Config Space" we say "Device configuration space is generally used for
rarely-changing or initialization-time parameters." and in Appendix B we
say "Device configuration space should only be used for
initialization-time parameters. It is a limited resource with no
synchronization between field written by the driver, so for most uses it
is better to use a virtqueue to update configuration information". I'm
not sure where to add a normative statement, given that this is
something that device types need to take care of.

>
> It is true that virtio-pci and virtio-mmio can signal changes in config 
> space from the driver to the device.  However the device driver can read 
> the config space asynchronously whenever it wants.  The generation 
> counter was added to fix non-atomic updates from the device.
>
> The generation counter in virtio-mmio and virtio-pci transport config 
> spaces only work because the hypervisor can see each read attempt to the 
> config space.  The virtio-pci suggests not incrementing the generation 
> counter unless the guest is doing a read.  (Because the generation 
> counter on virtio-pci is small.)
>
> In addition, config updates from the driver at not formally signaled and 
> it again relies on the hypervisor intersecting writes to the config space.

The configuration space for ccw is really different conceptually: both
reading from and writing to it are initiated by the driver sending
commands, and the architecture makes sure that reads and writes are
processed by the device strictly in the order that the driver sent
them. IOW, unless the device messes up, the driver will get consistent
information.

>
> Anyway I have thought of two solutions for the config space that do not 
> rely on hypervisor magic.
>
> 1) Config generation counter odd == update in progress
>
> In OpenAMP we are working on a transport that looks like virtio-mmio but 
> has modifications to allow it to be used w/o hypervisor magic.  A dumb 
> shared memory and bi-directional notifications should be enough.
>
> In this model we are adding multiple events in each direction.  One 
> event in each direction is a config space update.  This helps solve the 
> driver -> device config space change notification.  Assuming the driver 
> does not send back to back overlapping changes this should be enough 
> (but is not perfect.)
>
> To allow asynchronous config space reads by the driver, we could define 
> a 32 bit generation counter where odd numbers meant update in progress. 
> The config space read operation would look similar to the current model 
> => if the two values don't match >>> or are odd <<<, then read again.
>
> Not great but may be workable.
>
> 2) Message based config space
>
> The other transport option I am considering is something that is still 
> vring/virtqueue based but replaces the transport level structure with a 
> simple message passing facility.
>
> This would apply to all transport level ops (virtqueue discover and 
> config, feature negotiation, etc) but the config space would work like 
> below.
>
> In this model there would be no shared config space.  Each side would 
> have its own copy of the config space and would send messages to make 
> updates.  The driver side would start empty and would send a message to 
> discover the size of the config space and read its initial value. 
> Multiple reads may be needed to read the whole initial value if the 
> config space was large (>128 bytes?).  After the initial sync either 
> side can make updates and sends the offset, size and value of a region 
> that contains the change.

If all messages were sent by the driver, this would be very similar to
the ccw implementation. You would still have the device view of the
config space, and the driver could either have its own copy and update
if needed, or just obtain/change information when it actually needs it.
Plus a notification mechanism for device->driver "something changed, you
might want to re-read" (instead of sending a message with the change).

>
> Sorry for the info dump.  I was working on a doc that described the 
> problems and the AMP virtio-mmio option but it got stalled.  I will try 
> to get it going again.

This is actually very interesting :)


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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Viresh Kumar]

On 18-12-23, 15:02, Cornelia Huck wrote:
> 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.)

How about these changes now, before I post them again:

diff --git a/content.tex b/content.tex
index 0a62dce5f65f..2a86b1041747 100644
--- a/content.tex
+++ b/content.tex
@@ -631,8 +631,81 @@ \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.
+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}
+
+The transport MUST provide a mechanism for the driver to discover the
+device.
+
+The transport must provide a mechanism for the driver to identify the
+device type.
+
+The transport MUST provide a mechanism for communicating virtqueue
+configurations between the device and the driver.
+
+The transport MUST 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 a mechanism that the device and the driver
+use to access memory for implementing virtqueues.
+
+The transport MUST provide 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.
+
+The transport MAY provide a mechanism for the driver to initiate a
+reset of the virtqueues and device.
+
+The transport MUST provide a mechanism for the driver to read the
+device status. The transport SHOULD provide a mechanism for the driver to
+change the device status.
+
+The transport MUST provide a mechanism to implement config space between
+the device and the driver.
+
+\devicenormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}
+
+Any data associated with a device-initiated transaction MUST remain
+accessible to the driver until the driver acknowledges the transaction
+to be complete.
+
+The device MUST NOT access the contents of a virtqueue before the
+driver notifies, in a transport defined way, the device that the
+virtqueue is ready to be accessed.
+
+The device MUST NOT access or modify buffers on a virtqueue after it has
+notified the driver about their availability.
+
+The device MUST reset the virtqueues if requested, in a transport
+defined way, by the driver.
+
+\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}
+
+The driver MUST acknowledge device notifications, as mandated by the
+transport.
+
+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.
 
 \input{transport-pci.tex}
 \input{transport-mmio.tex}

-- 
viresh

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

[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements

[Cornelia Huck]

On Mon, Jan 29 2024, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 18-12-23, 15:02, Cornelia Huck wrote:
>> 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.)
>
> How about these changes now, before I post them again:
>
> diff --git a/content.tex b/content.tex
> index 0a62dce5f65f..2a86b1041747 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -631,8 +631,81 @@ \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.
> +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}
> +
> +The transport MUST provide a mechanism for the driver to discover the
> +device.
> +
> +The transport must provide a mechanism for the driver to identify the

s/must/MUST/

> +device type.
> +
> +The transport MUST provide a mechanism for communicating virtqueue
> +configurations between the device and the driver.
> +
> +The transport MUST 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 a mechanism that the device and the driver
> +use to access memory for implementing virtqueues.
> +
> +The transport MUST provide 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.
> +
> +The transport MAY provide a mechanism for the driver to initiate a
> +reset of the virtqueues and device.
> +
> +The transport MUST provide a mechanism for the driver to read the
> +device status. The transport SHOULD provide a mechanism for the driver to
> +change the device status.

I think the other way around? CCW only gained support for reading the
status with revision 2.

> +
> +The transport MUST provide a mechanism to implement config space between
> +the device and the driver.
> +
> +\devicenormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}
> +
> +Any data associated with a device-initiated transaction MUST remain
> +accessible to the driver until the driver acknowledges the transaction
> +to be complete.

Maybe better "The device MUST keep any data (...) accessible to the
driver..." ?

> +
> +The device MUST NOT access the contents of a virtqueue before the
> +driver notifies, in a transport defined way, the device that the
> +virtqueue is ready to be accessed.
> +
> +The device MUST NOT access or modify buffers on a virtqueue after it has
> +notified the driver about their availability.
> +
> +The device MUST reset the virtqueues if requested, in a transport
> +defined way, by the driver.

"in a transport defined way, if the transport provides such a method" ?

> +
> +\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio Transport Options}
> +
> +The driver MUST acknowledge device notifications, as mandated by the
> +transport.
> +
> +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.
>  
>  \input{transport-pci.tex}
>  \input{transport-mmio.tex}
>

These statements need to be hooked up in comformance.tex -- we also need
a way to formulate "transport normative" statements, I think?


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