[PATCH] virtio-pci: implement VIRTIO_F_RING_STATE

[PATCH] virtio-pci: implement VIRTIO_F_RING_STATE

[Jason Wang]

This patch is a follow up for the virtqueue state synchronization
series to implement VIRTIO_F_RING_STATE via a dedicated capability for
PCI transport.

With the help of the STOP status bit, the virtqueue state
synchronization for PCI should be self contained.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 content.tex | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/content.tex b/content.tex
index 284ead0..f1848f7 100644
--- a/content.tex
+++ b/content.tex
@@ -878,6 +878,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
 #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
 /* Vendor-specific data */
 #define VIRTIO_PCI_CAP_VENDOR_CFG        9
+/* General Virtqueue State */
+#define VIRTIO_PCI_CAP_RING_STATE        10
 \end{lstlisting}
 
         Any other value is reserved for future use.
@@ -1434,6 +1436,36 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
 specified by some other Virtio Structure PCI Capability
 of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
 
+\subsubsection{General Virtqueue State}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / General Virtqueue State}
+
+The VITIO_PCI_CAP_RING_STATE capability is for the driver to configure
+the virtqueue state(see \ref{sec:Virtqueues / Virtqueue State}):
+
+The capability is immediately followed by an additional field like so:
+
+\begin{lstlisting}
+struct virtio_pci_cfg_cap {
+        struct virtio_pci_cap cap;
+        le16 queue_select; /* read-write */
+        le16 avail_state;  /* read-write */
+        le16 used_state;   /* read-write */
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{queue_select}]
+        Queue Select. The driver selects which virtqueue the following
+        fields refer to.
+\item[\field{avail_state}]
+        The driver writes and reads available state here.
+\item[\field{used_state}]
+        The driver writes and reads used state here.
+\end{description}
+
+\devicenormative{\paragraph}{General Virtqueue State}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / General Virtqueue State}
+
+The device MUST present at least one VIRTIO_PCI_CAP_RING_STATE capability if VITIO_F_RING_STATE is negotiated.
+
 \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
 
 Transitional devices MUST present part of configuration
-- 
2.25.1

[virtio-comment] Re: [PATCH] virtio-pci: implement VIRTIO_F_RING_STATE

[Cornelia Huck]

On Wed, Jul 07 2021, Jason Wang <jasowang@redhat.com> wrote:

> This patch is a follow up for the virtqueue state synchronization
> series to implement VIRTIO_F_RING_STATE via a dedicated capability for
> PCI transport.
>
> With the help of the STOP status bit, the virtqueue state
> synchronization for PCI should be self contained.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  content.tex | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

(...)

> +\devicenormative{\paragraph}{General Virtqueue State}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / General Virtqueue State}
> +
> +The device MUST present at least one VIRTIO_PCI_CAP_RING_STATE
> capability if VITIO_F_RING_STATE is negotiated.

s/VITIO/VIRTIO/

Are multiple capabilities inteded to enable parallel access to multiple
virtqueues?

> +
>  \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
>  
>  Transitional devices MUST present part of configuration


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [PATCH] virtio-pci: implement VIRTIO_F_RING_STATE

[Jason Wang]

在 2021/7/9 下午7:06, Cornelia Huck 写道:
> On Wed, Jul 07 2021, Jason Wang <jasowang@redhat.com> wrote:
>
>> This patch is a follow up for the virtqueue state synchronization
>> series to implement VIRTIO_F_RING_STATE via a dedicated capability for
>> PCI transport.
>>
>> With the help of the STOP status bit, the virtqueue state
>> synchronization for PCI should be self contained.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   content.tex | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
> (...)
>
>> +\devicenormative{\paragraph}{General Virtqueue State}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / General Virtqueue State}
>> +
>> +The device MUST present at least one VIRTIO_PCI_CAP_RING_STATE
>> capability if VITIO_F_RING_STATE is negotiated.
> s/VITIO/VIRTIO/


Will fix.


>
> Are multiple capabilities inteded to enable parallel access to multiple
> virtqueues?


I'm not sure, it just not forbid the device to have multiple of such 
capabilies. We've already had normative like this for common 
configuration structure:

"The device MUST present at least one common configuration capability."

Thanks


>
>> +
>>   \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
>>   
>>   Transitional devices MUST present part of configuration

Re: [PATCH] virtio-pci: implement VIRTIO_F_RING_STATE

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

On Wed, Jul 07, 2021 at 12:34:41PM +0800, Jason Wang wrote:
> This patch is a follow up for the virtqueue state synchronization
> series to implement VIRTIO_F_RING_STATE via a dedicated capability for
> PCI transport.
> 
> With the help of the STOP status bit, the virtqueue state
> synchronization for PCI should be self contained.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  content.tex | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

This may not be critical, but the interface reminds me of the legacy
fw_cfg interface that was slow. It was replaced by a DMA interface that
reduced the number of vmexits. fw_cfg often transferred far more data
than this interface, so the inefficiency was a bigger problem.

If there was an admin virtqueue then the state of all virtqueues could
be saved/loaded in a single request instead of requiring 2 accesses per
virtqueue.

Assuming a register access takes 1 microsecond, a device with 64
virtqueues needs 128 microseconds to save/load virtqueue state. The
actual register access time will vary depending on how the device is
implemented (VFIO, vDPA, userspace). The total time is probably going to
be acceptable, however, if we need an admin virtqueue anyway, then maybe
do this as an admin virtqueue request instead of a transport-specific
method?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] virtio-pci: implement VIRTIO_F_RING_STATE

[Jason Wang]

在 2021/7/12 下午6:29, Stefan Hajnoczi 写道:
> On Wed, Jul 07, 2021 at 12:34:41PM +0800, Jason Wang wrote:
>> This patch is a follow up for the virtqueue state synchronization
>> series to implement VIRTIO_F_RING_STATE via a dedicated capability for
>> PCI transport.
>>
>> With the help of the STOP status bit, the virtqueue state
>> synchronization for PCI should be self contained.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   content.tex | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
> This may not be critical, but the interface reminds me of the legacy
> fw_cfg interface that was slow. It was replaced by a DMA interface that
> reduced the number of vmexits. fw_cfg often transferred far more data
> than this interface, so the inefficiency was a bigger problem.
>
> If there was an admin virtqueue then the state of all virtqueues could
> be saved/loaded in a single request instead of requiring 2 accesses per
> virtqueue.


So this is just one of the possible implementation. It could be done via 
other ways for sure:

1) new control command which pack both avail and used state
2) 32bit registers
3) shared memory (DMA)


>
> Assuming a register access takes 1 microsecond, a device with 64
> virtqueues needs 128 microseconds to save/load virtqueue state. The
> actual register access time will vary depending on how the device is
> implemented (VFIO, vDPA, userspace). The total time is probably going to
> be acceptable, however, if we need an admin virtqueue anyway,


It can only help if we invent a control command that can be used to set 
or get a brunch of states (e.g 128 states).


>   then maybe
> do this as an admin virtqueue request instead of a transport-specific
> method?


As discussed in another thread, admin virtqueue is one of the possible 
solution since the virtqueue state is one of the basic facility.

We can allow the state to be carried via admin virtqueue after it was 
introduced in the spec.

But admin virtqueue should not be the only method.

Thanks


>
> Stefan