[virtio-dev] Re: [virtio-comment] [RFC PATCH 0/5] virtio: introduce SUSPEND bit and vq state

[virtio-dev] Re: [virtio-comment] [RFC PATCH 0/5] virtio: introduce SUSPEND bit and vq state

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 03:28:59AM +0800, Zhu Lingshan wrote:
> This seires introduces
> 1)a new SUSPEND bit in the device status
> Which is used to suspend the device, so that the device states
> and virtqueue states are stablized.

Hanna, you might be interested in this because it has overlap with your
vhost-user device state migration work.

Stefan

> 
> 2)virtqueue state and accessors, to get and set last_avail_idx
> and last_used_idx of virtqueues.
> 
> The main usecase of these new facilities is Live Migration.
> 
> Furture work: dirty page tracking and in-flight descriptors.
> 
> This is RFC, this series carries on Jason and Eugenio's pervious work.
> 
> Any comments are welcome.
> 
> Zhu Lingshan (5):
>   virtio: introduce SUSPEND bit in device status
>   virtio: introduce vq state as basic facility
>   virtio: The actions by the device upon SUSPEND
>   virtqueue: constraints for virtqueue state
>   virtio-pci: implement VIRTIO_F_QUEUE_STATE
> 
>  content.tex       | 120 ++++++++++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex |  15 ++++++
>  2 files changed, 135 insertions(+)
> 
> -- 
> 2.35.3
> 
> 
> 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/
> 

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

[virtio-dev] Re: [virtio-comment] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 03:29:00AM +0800, Zhu Lingshan wrote:
> This patch introudces a new status bit in the device status: SUSPEND.
> 
> This SUSPEND bit can be used by the driver to suspend a device,
> in order to stablize the device states and virtqueue states.
> 
> Its main use case is live migration.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>

There is an character encoding issue in Eugenio's surname.

> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  content.tex | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

This patch hints at the asynchronous nature of the SUSPEND bit (the
driver must re-read the Device Status Field) but doesn't explain the
rationale or any limits.

For example, is there a timeout or should the driver re-read the Device
Status Field forever?

Does the driver need to re-read the Device Status Field after clearing
the SUSPEND bit?

> 
> diff --git a/content.tex b/content.tex
> index 0a62dce..1bb4401 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>    drive the device.
>  
> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> +  device has been suspended by the driver.
> +
>  \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>    an error from which it can't recover.
>  \end{description}
> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  recover by issuing a reset.
>  \end{note}
>  
> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> +
> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.

"When setting SUSPEND, ..." would be grammatically correct. Another
option is "After setting the SUSPEND bit, ...".

> +
>  \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>  
>  The device MUST NOT consume buffers or send any used buffer
> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>  MUST send a device configuration change notification to the driver.
>  
> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> +
> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> +
> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> +and resumes operation upon DRIVER_OK.

I can't parse this sentence. If the driver writes SUSPEND | DRIVER_OK |
... to the Device Status Field, then the device accepts DRIVER_OK and
clears SUSPEND?

Why?

> +
>  \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>  
>  Each virtio device offers all the features it understands.  During
> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>  	handling features reserved for future use.
>  
> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
> +   SUSPEND the device.
> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> +
>  \end{description}
>  
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> -- 
> 2.35.3
> 
> 
> 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/
> 

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

Re: [virtio-dev] [RFC PATCH 2/5] virtio: introduce vq state as basic facility

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 03:29:01AM +0800, Zhu Lingshan wrote:
> This patch adds new device facility to save and restore virtqueue
> state. The virtqueue state is split into two parts:
> 
> - The available state: The state that is used for read the next
>   available buffer.
> - The used state: The state that is used for make buffer used.
> 
> This will simply the transport specific method implemention. E.g two
> le16 could be used instead of a single le32). For split virtqueue, we
> only need the available state since the used state is implemented in
> the virtqueue itself (the used index). For packed virtqueue, we need
> both the available state and the used state.
> 
> Those states are required to implement live migration support for
> virtio device.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  content.tex | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 1bb4401..074f43e 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -516,6 +516,68 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
>  types. It is RECOMMENDED that devices generate version 4
>  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>  
> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State}
> +
> +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and
> +get the device internal virtqueue state through the following
> +fields. The implementation of the interfaces is transport specific.
> +
> +\subsection{\field{Available State} Field}
> +
> +The available state field is two bytes virtqueue state that is used by

"two bytes of virtqueue state"

> +the device to read the next available buffer.
> +
> +When VIRTIO_RING_F_PACKED is not negotiated, it contains:
> +
> +\begin{lstlisting}
> +le16 last_avail_idx;
> +\end{lstlisting}
> +
> +The \field{last_avail_idx} field is the free-running available ring
> +index where the device will read the next available head of a
> +descriptor chain.
> +
> +See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Available Ring}.
> +
> +When VIRTIO_RING_F_PACKED is negotiated, it contains:
> +
> +\begin{lstlisting}
> +le16 {
> +  last_avail_idx : 15;
> +  last_avail_wrap_counter : 1;
> +};
> +\end{lstlisting}
> +
> +The \field{last_avail_idx} field is the free-running location
> +where the device read the next descriptor from the virtqueue descriptor ring.
> +
> +The \field{last_avail_wrap_counter} field is the last driver ring wrap
> +counter that was observed by the device.
> +
> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> +
> +\subsection{\field{Used State} Field}
> +
> +The used state field is two bytes of virtqueue state that is used by
> +the device when marking a buffer used.

Please specify the non-packed case:

"When VIRTIO_RING_F_PACKED is not negotiated, the 16-bit value is always
0."

> +
> +When VIRTIO_RING_F_PACKED is negotiated, the used state contains:
> +
> +\begin{lstlisting}
> +le16 {
> +  used_idx : 15;
> +  used_wrap_counter : 1;
> +};
> +\end{lstlisting}
> +
> +The \field{used_idx} field is the free-running location where the device write next

"device writes the next"

> +used descriptor to the descriptor ring.
> +
> +The \field{used_wrap_counter} field is the wrap counter that is used
> +by the device.
> +
> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> +
>  \input{admin.tex}
>  
>  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> -- 
> 2.35.3
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

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

[virtio-dev] Re: [virtio-comment] [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 03:29:02AM +0800, Zhu Lingshan wrote:
> This commit specifies the actions to be taken by the device upon
> SUSPEND.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  content.tex | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 074f43e..43bd5de 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>  and resumes operation upon DRIVER_OK.
>  
> +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:

"when SUSPEND is set" is ambigious. It could mean when the driver writes
the Device Status Field, when the device transitions to SUSPEND, or
something else. Maybe "before the device reports the SUSPEND bit set in
the Device Status Field"?

> +\begin{itemize}
> +\item Stop comsuming any descriptors

"consuming"

It may be more consistent to talk about virtqueue buffers (i.e.
requests) rather than descriptors here.

> +\item Mark all finished descriptors as used and send used buffer notification to the driver

The device has to complete everything that is in flight, just completing
"finished" stuff is not enough. Does "finished descriptors" really mean
"in-flight virtqueue buffers"?

"send a used buffer notification" or "send used buffer notifications"

> +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}

Does "Record" mean that Virtqueue State fields can only be accessed by
the driver while device is paused (they may be outdated or invalid while
the device is unpaused)?

> +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}

What does "preserve" mean? Does it mean the Device Configuration Space
is not allowed to change during SUSPEND?

> +\item Present SUSPEND in \field{device status}
> +\end{itemize}
> +
>  \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>  
>  Each virtio device offers all the features it understands.  During
> -- 
> 2.35.3
> 
> 
> 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/
> 

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

Re: [virtio-dev] [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 03:29:03AM +0800, Zhu Lingshan wrote:
> This commit specifies the constraints of the virtqueue state,
> and the actions should be taken by the device when SUSPEND
> and DRIVER_OK is set
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  content.tex | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 43bd5de..f6ac581 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>  
>  See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>  
> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> +
> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
> +first before getting or setting Virtqueue State of any virtqueues.
> +
> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,

"negotiated"

Please run a spell checker.

> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
> +used index in the used ring.
> +
> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> +
> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.

"accesses against" is not commonly used in English. "accesses to" is more natural.

> +
> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
> +the device MUST ignore any accesses against \field{Used State}.

Same here.

> +
> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
> +the Virtqueue State of every virtqueue upon a reset.

I'm not sure if this statement is necessary since Virtqueue State
becomes inaccessible when the device is reset. By definition, Virtqueue
State is the device-internal state of the virtqueue, so it follows
logically that next time the driver can access the Virtqueue State after
reset, it contains the current state and not previous state from before
reset.

> +
> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
> +the device MUST record the Virtqueue State of every enabled virtqueue
> +in \field{Available State} and \field{Used State} respectively,
> +and correspondingly restore the Virtqueue State of every enabled virtqueue
> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.

"Available State"

> +
> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
> +and restore the available state of every enabled virtqueue from \field{Avaiable State}

"Available State"

> +when DRIVER_OK is set.
> +
>  \input{admin.tex}
>  
>  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> -- 
> 2.35.3
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

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

Re: [virtio-dev] [RFC PATCH 5/5] virtio-pci: implement VIRTIO_F_QUEUE_STATE

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 03:29:04AM +0800, Zhu Lingshan wrote:
> This patch adds two new le16 fields to common configruation structure
> to support VIRTIO_F_QUEUE_STATE in PCI tranport layer.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  transport-pci.tex | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/transport-pci.tex b/transport-pci.tex
> index a5c6719..d9bccb0 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -321,6 +321,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          le64 queue_device;              /* read-write */
>          le16 queue_notif_config_data;   /* read-only for driver */
>          le16 queue_reset;               /* read-write */
> +        le16 queue_avail_state;         /* read-write */
> +        le16 queue_used_state;          /* read-write */
>  
>          /* About the administration virtqueue. */
>          le16 admin_queue_index;         /* read-only for driver */

This is an incompatible change to the register layout. Please add new
registers at the end of struct virtio_pci_common_cfg so field offsets
don't change for existing drivers and devices.

> @@ -415,6 +417,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          This field exists only if VIRTIO_F_RING_RESET has been
>          negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
>  
> +\item[\field{queue_avail_state}]
> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been

"valid"

> +        negotiated. The driver sets and gets the available state of
> +        the virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
> +
> +\item[\field{queue_used_state}]
> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been

"valid"

> +        negotiated. The driver sets and gets the used state of the
> +        virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
> +
>  \item[\field{admin_queue_index}]
>          The device uses this to report the index of the first administration virtqueue.
>          This field is valid only if VIRTIO_F_ADMIN_VQ has been negotiated.
> @@ -488,6 +500,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  present either a value of 0 or a power of 2 in
>  \field{queue_size}.
>  
> +If VIRTIO_F_QUEUE_STATE has not been negotiated, the device MUST ignore
> +any accesses against \field{queue_avail_state} and \field{queue_used_state}.

"accesses to"

> +
>  If VIRTIO_F_ADMIN_VQ has been negotiated, the value
>  \field{admin_queue_index} MUST be equal to, or bigger than
>  \field{num_queues}; also, \field{admin_queue_num} MUST be
> -- 
> 2.35.3
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

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

[virtio-dev] Re: [virtio-comment] [RFC PATCH 0/5] virtio: introduce SUSPEND bit and vq state

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 03:28:59AM +0800, Zhu Lingshan wrote:
> This seires introduces
> 1)a new SUSPEND bit in the device status
> Which is used to suspend the device, so that the device states
> and virtqueue states are stablized.
> 
> 2)virtqueue state and accessors, to get and set last_avail_idx
> and last_used_idx of virtqueues.
> 
> The main usecase of these new facilities is Live Migration.
> 
> Furture work: dirty page tracking and in-flight descriptors.
> 
> This is RFC, this series carries on Jason and Eugenio's pervious work.
> 
> Any comments are welcome.

In order to support stateful devices like virtiofs, virtio-gpu, or
virtio-crypto it would be necessary to add device state save/load
functionality too.

Even virtio-net needs device state save/load functionality if the VMM is
not intercepting the stateful controlq.

In-flight descriptors can be included in the device state.

In fact, the only reason why I think VIRTIO_F_QUEUE_STATE makes sense is
for easy integration into existing vhost software stacks. Otherwise we
should just define a device state save/load interface along with
standard device state serialization for devices where migration
interoperability is possible. VIRTIO_F_QUEUE_STATE is a small subset of
device state save/load.

I think the convenience of integrating into existing vhost software
stacks is worth the duplication, but I wanted to mention that this
interface will not be enough to live migrate all device types and we'll
need something more in the future.

Stefan

> 
> Zhu Lingshan (5):
>   virtio: introduce SUSPEND bit in device status
>   virtio: introduce vq state as basic facility
>   virtio: The actions by the device upon SUSPEND
>   virtqueue: constraints for virtqueue state
>   virtio-pci: implement VIRTIO_F_QUEUE_STATE
> 
>  content.tex       | 120 ++++++++++++++++++++++++++++++++++++++++++++++
>  transport-pci.tex |  15 ++++++
>  2 files changed, 135 insertions(+)
> 
> -- 
> 2.35.3
> 
> 
> 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/
> 

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

[virtio-dev] [RFC PATCH 0/5] virtio: introduce SUSPEND bit and vq state

[Zhu Lingshan]

This seires introduces
1)a new SUSPEND bit in the device status
Which is used to suspend the device, so that the device states
and virtqueue states are stablized.

2)virtqueue state and accessors, to get and set last_avail_idx
and last_used_idx of virtqueues.

The main usecase of these new facilities is Live Migration.

Furture work: dirty page tracking and in-flight descriptors.

This is RFC, this series carries on Jason and Eugenio's pervious work.

Any comments are welcome.

Zhu Lingshan (5):
  virtio: introduce SUSPEND bit in device status
  virtio: introduce vq state as basic facility
  virtio: The actions by the device upon SUSPEND
  virtqueue: constraints for virtqueue state
  virtio-pci: implement VIRTIO_F_QUEUE_STATE

 content.tex       | 120 ++++++++++++++++++++++++++++++++++++++++++++++
 transport-pci.tex |  15 ++++++
 2 files changed, 135 insertions(+)

-- 
2.35.3


---------------------------------------------------------------------
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] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan]

This patch introudces a new status bit in the device status: SUSPEND.

This SUSPEND bit can be used by the driver to suspend a device,
in order to stablize the device states and virtqueue states.

Its main use case is live migration.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 content.tex | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/content.tex b/content.tex
index 0a62dce..1bb4401 100644
--- a/content.tex
+++ b/content.tex
@@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
   drive the device.
 
+\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
+  device has been suspended by the driver.
+
 \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
   an error from which it can't recover.
 \end{description}
@@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 recover by issuing a reset.
 \end{note}
 
+The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
+
+When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
+
 \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
 
 The device MUST NOT consume buffers or send any used buffer
@@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
 MUST send a device configuration change notification to the driver.
 
+The device MUST ignore SUSPEND if FEATURES_OK is not set.
+
+The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
+
+If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
+and resumes operation upon DRIVER_OK.
+
 \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
 
 Each virtio device offers all the features it understands.  During
@@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
 	handling features reserved for future use.
 
+  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
+   SUSPEND the device.
+   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
2.35.3


---------------------------------------------------------------------
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] [RFC PATCH 2/5] virtio: introduce vq state as basic facility

[Zhu Lingshan]

This patch adds new device facility to save and restore virtqueue
state. The virtqueue state is split into two parts:

- The available state: The state that is used for read the next
  available buffer.
- The used state: The state that is used for make buffer used.

This will simply the transport specific method implemention. E.g two
le16 could be used instead of a single le32). For split virtqueue, we
only need the available state since the used state is implemented in
the virtqueue itself (the used index). For packed virtqueue, we need
both the available state and the used state.

Those states are required to implement live migration support for
virtio device.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 content.tex | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/content.tex b/content.tex
index 1bb4401..074f43e 100644
--- a/content.tex
+++ b/content.tex
@@ -516,6 +516,68 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
 types. It is RECOMMENDED that devices generate version 4
 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
 
+\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State}
+
+When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and
+get the device internal virtqueue state through the following
+fields. The implementation of the interfaces is transport specific.
+
+\subsection{\field{Available State} Field}
+
+The available state field is two bytes virtqueue state that is used by
+the device to read the next available buffer.
+
+When VIRTIO_RING_F_PACKED is not negotiated, it contains:
+
+\begin{lstlisting}
+le16 last_avail_idx;
+\end{lstlisting}
+
+The \field{last_avail_idx} field is the free-running available ring
+index where the device will read the next available head of a
+descriptor chain.
+
+See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Available Ring}.
+
+When VIRTIO_RING_F_PACKED is negotiated, it contains:
+
+\begin{lstlisting}
+le16 {
+  last_avail_idx : 15;
+  last_avail_wrap_counter : 1;
+};
+\end{lstlisting}
+
+The \field{last_avail_idx} field is the free-running location
+where the device read the next descriptor from the virtqueue descriptor ring.
+
+The \field{last_avail_wrap_counter} field is the last driver ring wrap
+counter that was observed by the device.
+
+See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
+
+\subsection{\field{Used State} Field}
+
+The used state field is two bytes of virtqueue state that is used by
+the device when marking a buffer used.
+
+When VIRTIO_RING_F_PACKED is negotiated, the used state contains:
+
+\begin{lstlisting}
+le16 {
+  used_idx : 15;
+  used_wrap_counter : 1;
+};
+\end{lstlisting}
+
+The \field{used_idx} field is the free-running location where the device write next
+used descriptor to the descriptor ring.
+
+The \field{used_wrap_counter} field is the wrap counter that is used
+by the device.
+
+See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
+
 \input{admin.tex}
 
 \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
-- 
2.35.3


---------------------------------------------------------------------
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] [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Zhu Lingshan]

This commit specifies the actions to be taken by the device upon
SUSPEND.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 content.tex | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/content.tex b/content.tex
index 074f43e..43bd5de 100644
--- a/content.tex
+++ b/content.tex
@@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
 and resumes operation upon DRIVER_OK.
 
+If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
+\begin{itemize}
+\item Stop comsuming any descriptors
+\item Mark all finished descriptors as used and send used buffer notification to the driver
+\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
+\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
+\item Present SUSPEND in \field{device status}
+\end{itemize}
+
 \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
 
 Each virtio device offers all the features it understands.  During
-- 
2.35.3


---------------------------------------------------------------------
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] [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu Lingshan]

This commit specifies the constraints of the virtqueue state,
and the actions should be taken by the device when SUSPEND
and DRIVER_OK is set

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 content.tex | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/content.tex b/content.tex
index 43bd5de..f6ac581 100644
--- a/content.tex
+++ b/content.tex
@@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
 
 See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
 
+\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
+
+If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
+first before getting or setting Virtqueue State of any virtqueues.
+
+If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
+the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
+used index in the used ring.
+
+\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
+
+If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
+the device MUST ignore any accesses against Virtqueue State of any virtqueues.
+
+When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
+the device MUST ignore any accesses against \field{Used State}.
+
+If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
+the Virtqueue State of every virtqueue upon a reset.
+
+If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
+the device MUST record the Virtqueue State of every enabled virtqueue
+in \field{Available State} and \field{Used State} respectively,
+and correspondingly restore the Virtqueue State of every enabled virtqueue
+from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
+
+If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
+the device MUST record the available state of every enabled virtqueue in \field{Available State},
+and restore the available state of every enabled virtqueue from \field{Avaiable State}
+when DRIVER_OK is set.
+
 \input{admin.tex}
 
 \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
-- 
2.35.3


---------------------------------------------------------------------
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] [RFC PATCH 5/5] virtio-pci: implement VIRTIO_F_QUEUE_STATE

[Zhu Lingshan]

This patch adds two new le16 fields to common configruation structure
to support VIRTIO_F_QUEUE_STATE in PCI tranport layer.

Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 transport-pci.tex | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/transport-pci.tex b/transport-pci.tex
index a5c6719..d9bccb0 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -321,6 +321,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         le64 queue_device;              /* read-write */
         le16 queue_notif_config_data;   /* read-only for driver */
         le16 queue_reset;               /* read-write */
+        le16 queue_avail_state;         /* read-write */
+        le16 queue_used_state;          /* read-write */
 
         /* About the administration virtqueue. */
         le16 admin_queue_index;         /* read-only for driver */
@@ -415,6 +417,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         This field exists only if VIRTIO_F_RING_RESET has been
         negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 
+\item[\field{queue_avail_state}]
+        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
+        negotiated. The driver sets and gets the available state of
+        the virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
+
+\item[\field{queue_used_state}]
+        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
+        negotiated. The driver sets and gets the used state of the
+        virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
+
 \item[\field{admin_queue_index}]
         The device uses this to report the index of the first administration virtqueue.
         This field is valid only if VIRTIO_F_ADMIN_VQ has been negotiated.
@@ -488,6 +500,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 present either a value of 0 or a power of 2 in
 \field{queue_size}.
 
+If VIRTIO_F_QUEUE_STATE has not been negotiated, the device MUST ignore
+any accesses against \field{queue_avail_state} and \field{queue_used_state}.
+
 If VIRTIO_F_ADMIN_VQ has been negotiated, the value
 \field{admin_queue_index} MUST be equal to, or bigger than
 \field{num_queues}; also, \field{admin_queue_num} MUST be
-- 
2.35.3


---------------------------------------------------------------------
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: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Jason Wang]

On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This patch introudces a new status bit in the device status: SUSPEND.
>
> This SUSPEND bit can be used by the driver to suspend a device,
> in order to stablize the device states and virtqueue states.
>
> Its main use case is live migration.

I think patch 3 needs to be squashed into this one.

>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  content.tex | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 0a62dce..1bb4401 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>    drive the device.
>
> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> +  device has been suspended by the driver.
> +
>  \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>    an error from which it can't recover.
>  \end{description}
> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  recover by issuing a reset.
>  \end{note}
>
> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> +
> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> +
>  \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>
>  The device MUST NOT consume buffers or send any used buffer
> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>  MUST send a device configuration change notification to the driver.
>
> +The device MUST ignore SUSPEND if FEATURES_OK is not set.

Is there any value to allow SUSPEND to be set without DRIVER_OK?

> +
> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.

typo.

Thanks

> +
> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> +and resumes operation upon DRIVER_OK.
> +
>  \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>
>  Each virtio device offers all the features it understands.  During
> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>         \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>         handling features reserved for future use.
>
> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
> +   SUSPEND the device.
> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> +
>  \end{description}
>
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> --
> 2.35.3
>


---------------------------------------------------------------------
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: [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Jason Wang]

On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit specifies the actions to be taken by the device upon
> SUSPEND.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  content.tex | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 074f43e..43bd5de 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>  and resumes operation upon DRIVER_OK.
>
> +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
> +\begin{itemize}
> +\item Stop comsuming any descriptors

Typo.

> +\item Mark all finished descriptors as used and send used buffer notification to the driver

What happens to the unfinished descriptors?

> +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}

This basically means those states are only available after suspending
or not? It would be still useful for debugging if we allow it without
suspending.

> +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}

We probably need to define the "pause" here (e.g what happens to the
inflight descriptors).

Thanks

> +\item Present SUSPEND in \field{device status}
> +\end{itemize}
> +
>  \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>
>  Each virtio device offers all the features it understands.  During
> --
> 2.35.3
>


---------------------------------------------------------------------
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: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Jason Wang]

On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit specifies the constraints of the virtqueue state,
> and the actions should be taken by the device when SUSPEND
> and DRIVER_OK is set
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  content.tex | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 43bd5de..f6ac581 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>
>  See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>
> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> +
> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
> +first before getting or setting Virtqueue State of any virtqueues.

I don't get why this is a must. It could be useful for debugging.

> +
> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,

typo

> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
> +used index in the used ring.
> +
> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> +
> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.

Btw, do we need to clarify the behavior of ring reset after suspending?

> +
> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
> +the device MUST ignore any accesses against \field{Used State}.
> +
> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
> +the Virtqueue State of every virtqueue upon a reset.

Need to define the meaning of "reset" this is important for packed virtqueue.

> +
> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
> +the device MUST record the Virtqueue State of every enabled virtqueue
> +in \field{Available State} and \field{Used State} respectively,
> +and correspondingly restore the Virtqueue State of every enabled virtqueue
> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.

We can just let the device report those states in any case then we
don't need to care about those details, or did you see any blockers?

Thanks

> +
> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
> +when DRIVER_OK is set.
> +
>  \input{admin.tex}
>
>  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> --
> 2.35.3
>


---------------------------------------------------------------------
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: [RFC PATCH 5/5] virtio-pci: implement VIRTIO_F_QUEUE_STATE

[Jason Wang]

On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This patch adds two new le16 fields to common configruation structure
> to support VIRTIO_F_QUEUE_STATE in PCI tranport layer.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  transport-pci.tex | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/transport-pci.tex b/transport-pci.tex
> index a5c6719..d9bccb0 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -321,6 +321,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          le64 queue_device;              /* read-write */
>          le16 queue_notif_config_data;   /* read-only for driver */
>          le16 queue_reset;               /* read-write */
> +        le16 queue_avail_state;         /* read-write */
> +        le16 queue_used_state;          /* read-write */

Those need to be placed at the end of this structure at least or we
need a dedicated new capability.

>
>          /* About the administration virtqueue. */
>          le16 admin_queue_index;         /* read-only for driver */
> @@ -415,6 +417,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          This field exists only if VIRTIO_F_RING_RESET has been
>          negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
>
> +\item[\field{queue_avail_state}]
> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
> +        negotiated. The driver sets and gets the available state of
> +        the virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
> +
> +\item[\field{queue_used_state}]
> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
> +        negotiated. The driver sets and gets the used state of the
> +        virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
> +
>  \item[\field{admin_queue_index}]
>          The device uses this to report the index of the first administration virtqueue.
>          This field is valid only if VIRTIO_F_ADMIN_VQ has been negotiated.
> @@ -488,6 +500,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  present either a value of 0 or a power of 2 in
>  \field{queue_size}.
>
> +If VIRTIO_F_QUEUE_STATE has not been negotiated, the device MUST ignore
> +any accesses against \field{queue_avail_state} and \field{queue_used_state}.
> +
>  If VIRTIO_F_ADMIN_VQ has been negotiated, the value
>  \field{admin_queue_index} MUST be equal to, or bigger than
>  \field{num_queues}; also, \field{admin_queue_num} MUST be
> --
> 2.35.3
>


---------------------------------------------------------------------
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: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Jason Wang]

On Tue, Aug 15, 2023 at 8:26 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >
> > This patch introudces a new status bit in the device status: SUSPEND.
> >
> > This SUSPEND bit can be used by the driver to suspend a device,
> > in order to stablize the device states and virtqueue states.
> >
> > Its main use case is live migration.
>
> I think patch 3 needs to be squashed into this one.
>
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > ---
> >  content.tex | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 0a62dce..1bb4401 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >  \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
> >    drive the device.
> >
> > +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > +  device has been suspended by the driver.
> > +
> >  \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> >    an error from which it can't recover.
> >  \end{description}
> > @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >  recover by issuing a reset.
> >  \end{note}
> >
> > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> > +
> > +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> > +
> >  \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> >
> >  The device MUST NOT consume buffers or send any used buffer
> > @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >  that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
> >  MUST send a device configuration change notification to the driver.
> >
> > +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>
> Is there any value to allow SUSPEND to be set without DRIVER_OK?
>
> > +
> > +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>
> typo.
>
> Thanks

Btw, it's not clear to me if driver is allowed to clear this bit.

Thanks

>
> > +
> > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > +and resumes operation upon DRIVER_OK.
> > +
> >  \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> >
> >  Each virtio device offers all the features it understands.  During
> > @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >         \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
> >         handling features reserved for future use.
> >
> > +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
> > +   SUSPEND the device.
> > +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> > +
> >  \end{description}
> >
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > --
> > 2.35.3
> >


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 0/5] virtio: introduce SUSPEND bit and vq state

[Jason Wang]

On Tue, Aug 15, 2023 at 12:28 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Aug 15, 2023 at 03:28:59AM +0800, Zhu Lingshan wrote:
> > This seires introduces
> > 1)a new SUSPEND bit in the device status
> > Which is used to suspend the device, so that the device states
> > and virtqueue states are stablized.
> >
> > 2)virtqueue state and accessors, to get and set last_avail_idx
> > and last_used_idx of virtqueues.
> >
> > The main usecase of these new facilities is Live Migration.
> >
> > Furture work: dirty page tracking and in-flight descriptors.
> >
> > This is RFC, this series carries on Jason and Eugenio's pervious work.
> >
> > Any comments are welcome.
>
> In order to support stateful devices like virtiofs, virtio-gpu, or
> virtio-crypto it would be necessary to add device state save/load
> functionality too.

+1 and before save/load, we need to define those states first.

E.g there are patches that try to fix the GPU suspend/hibernation
which probably requires those facilities as well.

>
> Even virtio-net needs device state save/load functionality if the VMM is
> not intercepting the stateful controlq.

Yes, this could be done via trap and emulation.

>
> In-flight descriptors can be included in the device state.
>

Probably.

> In fact, the only reason why I think VIRTIO_F_QUEUE_STATE makes sense is
> for easy integration into existing vhost software stacks. Otherwise we
> should just define a device state save/load interface along with
> standard device state serialization for devices where migration
> interoperability is possible. VIRTIO_F_QUEUE_STATE is a small subset of
> device state save/load.

For simple devices like virtio-net, queue state should be sufficient
for hardware.

>
> I think the convenience of integrating into existing vhost software
> stacks is worth the duplication, but I wanted to mention that this
> interface will not be enough to live migrate all device types and we'll
> need something more in the future.

Yes, the patch is a start for simple stateless devices.

Thanks

>
> Stefan
>
> >
> > Zhu Lingshan (5):
> >   virtio: introduce SUSPEND bit in device status
> >   virtio: introduce vq state as basic facility
> >   virtio: The actions by the device upon SUSPEND
> >   virtqueue: constraints for virtqueue state
> >   virtio-pci: implement VIRTIO_F_QUEUE_STATE
> >
> >  content.tex       | 120 ++++++++++++++++++++++++++++++++++++++++++++++
> >  transport-pci.tex |  15 ++++++
> >  2 files changed, 135 insertions(+)
> >
> > --
> > 2.35.3
> >
> >
> > 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/
> >


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 0/5] virtio: introduce SUSPEND bit and vq state

[Zhu, Lingshan]

On 8/15/2023 9:38 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 12:28 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> On Tue, Aug 15, 2023 at 03:28:59AM +0800, Zhu Lingshan wrote:
>>> This seires introduces
>>> 1)a new SUSPEND bit in the device status
>>> Which is used to suspend the device, so that the device states
>>> and virtqueue states are stablized.
>>>
>>> 2)virtqueue state and accessors, to get and set last_avail_idx
>>> and last_used_idx of virtqueues.
>>>
>>> The main usecase of these new facilities is Live Migration.
>>>
>>> Furture work: dirty page tracking and in-flight descriptors.
>>>
>>> This is RFC, this series carries on Jason and Eugenio's pervious work.
>>>
>>> Any comments are welcome.
>> In order to support stateful devices like virtiofs, virtio-gpu, or
>> virtio-crypto it would be necessary to add device state save/load
>> functionality too.
> +1 and before save/load, we need to define those states first.
>
> E.g there are patches that try to fix the GPU suspend/hibernation
> which probably requires those facilities as well.
I agree, we should define the states of the device types first, like
virtio-fs. For virtio-net, one of the states if in-flight descriptors,
and this is planned work.

>
>> Even virtio-net needs device state save/load functionality if the VMM is
>> not intercepting the stateful controlq.
> Yes, this could be done via trap and emulation.
I agree
>
>> In-flight descriptors can be included in the device state.
>>
> Probably.
Yes, in-flight descriptors and dirty page tracking are in the plan. This 
series only
implements SUSPEND and vq state accessors as the first RFC try to 
collect comments.
>
>> In fact, the only reason why I think VIRTIO_F_QUEUE_STATE makes sense is
>> for easy integration into existing vhost software stacks. Otherwise we
>> should just define a device state save/load interface along with
>> standard device state serialization for devices where migration
>> interoperability is possible. VIRTIO_F_QUEUE_STATE is a small subset of
>> device state save/load.
> For simple devices like virtio-net, queue state should be sufficient
> for hardware.
>
>> I think the convenience of integrating into existing vhost software
>> stacks is worth the duplication, but I wanted to mention that this
>> interface will not be enough to live migrate all device types and we'll
>> need something more in the future.
> Yes, the patch is a start for simple stateless devices.
Yes!

Thanks!
>
> Thanks
>
>> Stefan
>>
>>> Zhu Lingshan (5):
>>>    virtio: introduce SUSPEND bit in device status
>>>    virtio: introduce vq state as basic facility
>>>    virtio: The actions by the device upon SUSPEND
>>>    virtqueue: constraints for virtqueue state
>>>    virtio-pci: implement VIRTIO_F_QUEUE_STATE
>>>
>>>   content.tex       | 120 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   transport-pci.tex |  15 ++++++
>>>   2 files changed, 135 insertions(+)
>>>
>>> --
>>> 2.35.3
>>>
>>>
>>> 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/
>>>


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Zhu, Lingshan]

On 8/14/2023 10:30 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 15, 2023 at 03:29:00AM +0800, Zhu Lingshan wrote:
>> This patch introudces a new status bit in the device status: SUSPEND.
>>
>> This SUSPEND bit can be used by the driver to suspend a device,
>> in order to stablize the device states and virtqueue states.
>>
>> Its main use case is live migration.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> There is an character encoding issue in Eugenio's surname.
Oh, I copied his SOB form his email, I will copy from git log to fix this,
thanks for point out it.
>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   content.tex | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
> This patch hints at the asynchronous nature of the SUSPEND bit (the
> driver must re-read the Device Status Field) but doesn't explain the
> rationale or any limits.
>
> For example, is there a timeout or should the driver re-read the Device
> Status Field forever?
It depends on the driver, normally we expect this operation can be done 
successfully
like how the driver/device handles FEATURES_OK.

Once failed due to:
1) driver timeout, the driver can reset the device
2) device failure, the device can set NEEDS_RESET.
>
> Does the driver need to re-read the Device Status Field after clearing
> the SUSPEND bit?
I think the driver should re-read, I will add this in the next version.
>
>> diff --git a/content.tex b/content.tex
>> index 0a62dce..1bb4401 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>>     drive the device.
>>   
>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>> +  device has been suspended by the driver.
>> +
>>   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>     an error from which it can't recover.
>>   \end{description}
>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>   recover by issuing a reset.
>>   \end{note}
>>   
>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
>> +
>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> "When setting SUSPEND, ..." would be grammatically correct. Another
> option is "After setting the SUSPEND bit, ...".
Will fix in the next version.
>
>> +
>>   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>   
>>   The device MUST NOT consume buffers or send any used buffer
>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>>   MUST send a device configuration change notification to the driver.
>>   
>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>> +
>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>> +
>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>> +and resumes operation upon DRIVER_OK.
> I can't parse this sentence. If the driver writes SUSPEND | DRIVER_OK |
> ... to the Device Status Field, then the device accepts DRIVER_OK and
> clears SUSPEND?
>
> Why?
I expect DRIVER_OK can clear SUSPEND, so that the device can resume running
in case of a failed live migration.

Maybe I should say: DRIVER_OK clears SUSPEND, and if DRIVER_OK is set to
a suspended device, the device should resume operation

Thanks
>
>> +
>>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>   
>>   Each virtio device offers all the features it understands.  During
>> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>   	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>>   	handling features reserved for future use.
>>   
>> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
>> +   SUSPEND the device.
>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>> +
>>   \end{description}
>>   
>>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> -- 
>> 2.35.3
>>
>>
>> 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/
>>


---------------------------------------------------------------------
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: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Zhu, Lingshan]

On 8/15/2023 8:37 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 8:26 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>> This patch introudces a new status bit in the device status: SUSPEND.
>>>
>>> This SUSPEND bit can be used by the driver to suspend a device,
>>> in order to stablize the device states and virtqueue states.
>>>
>>> Its main use case is live migration.
>> I think patch 3 needs to be squashed into this one.
Patch 3 mentions Virtqueue State that is introduced in patch 2,
so I think patch 3 should placed after patch 2.

Thanks
>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   content.tex | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/content.tex b/content.tex
>>> index 0a62dce..1bb4401 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>>>     drive the device.
>>>
>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>> +  device has been suspended by the driver.
>>> +
>>>   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>     an error from which it can't recover.
>>>   \end{description}
>>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>   recover by issuing a reset.
>>>   \end{note}
>>>
>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
>>> +
>>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
>>> +
>>>   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>>
>>>   The device MUST NOT consume buffers or send any used buffer
>>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>>>   MUST send a device configuration change notification to the driver.
>>>
>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>> Is there any value to allow SUSPEND to be set without DRIVER_OK?
If live migration happens when the guest initializing the device,
maybe the guest driver has already configured some virtqueues or
modified the config space. At that point, We should allow to suspend
the device, because the hypervisor should not reset the device at
that moment.

SUSPEND requires FEATURES_OK because VIRTIO_F_SUSPEND has to be negotiated.
>>
>>> +
>>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>> typo.
Yes
>>
>> Thanks
> Btw, it's not clear to me if driver is allowed to clear this bit.
Patch 1 mentions that DRIVER_OK clears SUSPEND, so that the driver
can resume the device if LM fails.

Thanks
>
> Thanks
>
>>> +
>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>> +and resumes operation upon DRIVER_OK.
>>> +
>>>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>
>>>   Each virtio device offers all the features it understands.  During
>>> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>>          \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>>>          handling features reserved for future use.
>>>
>>> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
>>> +   SUSPEND the device.
>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>>> +
>>>   \end{description}
>>>
>>>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>>> --
>>> 2.35.3
>>>


---------------------------------------------------------------------
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: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Zhu, Lingshan]

On 8/15/2023 8:37 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 8:26 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>> This patch introudces a new status bit in the device status: SUSPEND.
>>>
>>> This SUSPEND bit can be used by the driver to suspend a device,
>>> in order to stablize the device states and virtqueue states.
>>>
>>> Its main use case is live migration.
>> I think patch 3 needs to be squashed into this one.
>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   content.tex | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/content.tex b/content.tex
>>> index 0a62dce..1bb4401 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>>>     drive the device.
>>>
>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>> +  device has been suspended by the driver.
>>> +
>>>   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>     an error from which it can't recover.
>>>   \end{description}
>>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>   recover by issuing a reset.
>>>   \end{note}
>>>
>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
>>> +
>>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
>>> +
>>>   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>>
>>>   The device MUST NOT consume buffers or send any used buffer
>>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>>>   MUST send a device configuration change notification to the driver.
>>>
>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>> Is there any value to allow SUSPEND to be set without DRIVER_OK?
>>
>>> +
>>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>> typo.
>>
>> Thanks
> Btw, it's not clear to me if driver is allowed to clear this bit.
To allow terminate a live migration process or resume from a failed live 
migration,
we allow DRIVER_OK to clear SUSPEND, in patch 1.

Thanks
>
> Thanks
>
>>> +
>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>> +and resumes operation upon DRIVER_OK.
>>> +
>>>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>
>>>   Each virtio device offers all the features it understands.  During
>>> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>>          \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>>>          handling features reserved for future use.
>>>
>>> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
>>> +   SUSPEND the device.
>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>>> +
>>>   \end{description}
>>>
>>>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>>> --
>>> 2.35.3
>>>


---------------------------------------------------------------------
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] [RFC PATCH 2/5] virtio: introduce vq state as basic facility

[Zhu, Lingshan]

On 8/14/2023 10:49 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 15, 2023 at 03:29:01AM +0800, Zhu Lingshan wrote:
>> This patch adds new device facility to save and restore virtqueue
>> state. The virtqueue state is split into two parts:
>>
>> - The available state: The state that is used for read the next
>>    available buffer.
>> - The used state: The state that is used for make buffer used.
>>
>> This will simply the transport specific method implemention. E.g two
>> le16 could be used instead of a single le32). For split virtqueue, we
>> only need the available state since the used state is implemented in
>> the virtqueue itself (the used index). For packed virtqueue, we need
>> both the available state and the used state.
>>
>> Those states are required to implement live migration support for
>> virtio device.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   content.tex | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 62 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index 1bb4401..074f43e 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -516,6 +516,68 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
>>   types. It is RECOMMENDED that devices generate version 4
>>   UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>>   
>> +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State}
>> +
>> +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and
>> +get the device internal virtqueue state through the following
>> +fields. The implementation of the interfaces is transport specific.
>> +
>> +\subsection{\field{Available State} Field}
>> +
>> +The available state field is two bytes virtqueue state that is used by
> "two bytes of virtqueue state"
Yes, will fix
>
>> +the device to read the next available buffer.
>> +
>> +When VIRTIO_RING_F_PACKED is not negotiated, it contains:
>> +
>> +\begin{lstlisting}
>> +le16 last_avail_idx;
>> +\end{lstlisting}
>> +
>> +The \field{last_avail_idx} field is the free-running available ring
>> +index where the device will read the next available head of a
>> +descriptor chain.
>> +
>> +See also \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Available Ring}.
>> +
>> +When VIRTIO_RING_F_PACKED is negotiated, it contains:
>> +
>> +\begin{lstlisting}
>> +le16 {
>> +  last_avail_idx : 15;
>> +  last_avail_wrap_counter : 1;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{last_avail_idx} field is the free-running location
>> +where the device read the next descriptor from the virtqueue descriptor ring.
>> +
>> +The \field{last_avail_wrap_counter} field is the last driver ring wrap
>> +counter that was observed by the device.
>> +
>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>> +
>> +\subsection{\field{Used State} Field}
>> +
>> +The used state field is two bytes of virtqueue state that is used by
>> +the device when marking a buffer used.
> Please specify the non-packed case:
>
> "When VIRTIO_RING_F_PACKED is not negotiated, the 16-bit value is always
> 0."
we did not define the used_state for non-packed vqs in this series.
I will define this in the next version. and Say it is always 0 if 
VIRTIO_F_PACKED is not negotiated.
>
>> +
>> +When VIRTIO_RING_F_PACKED is negotiated, the used state contains:
>> +
>> +\begin{lstlisting}
>> +le16 {
>> +  used_idx : 15;
>> +  used_wrap_counter : 1;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{used_idx} field is the free-running location where the device write next
> "device writes the next"
Yes, will fix

Thanks
>
>> +used descriptor to the descriptor ring.
>> +
>> +The \field{used_wrap_counter} field is the wrap counter that is used
>> +by the device.
>> +
>> +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>> +
>>   \input{admin.tex}
>>   
>>   \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>> -- 
>> 2.35.3
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Zhu, Lingshan]

On 8/14/2023 11:00 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 15, 2023 at 03:29:02AM +0800, Zhu Lingshan wrote:
>> This commit specifies the actions to be taken by the device upon
>> SUSPEND.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   content.tex | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index 074f43e..43bd5de 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>   If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>   and resumes operation upon DRIVER_OK.
>>   
>> +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
> "when SUSPEND is set" is ambigious. It could mean when the driver writes
> the Device Status Field, when the device transitions to SUSPEND, or
> something else. Maybe "before the device reports the SUSPEND bit set in
> the Device Status Field"?
Nice catch! will fix: when the driver sets SUSPEND, before the device 
reports the
SUSPEND bit set in the \field{device status}, the device MUST perform the
following actions.
>
>> +\begin{itemize}
>> +\item Stop comsuming any descriptors
> "consuming"
yes
>
> It may be more consistent to talk about virtqueue buffers (i.e.
> requests) rather than descriptors here.
yes
>
>> +\item Mark all finished descriptors as used and send used buffer notification to the driver
> The device has to complete everything that is in flight, just completing
> "finished" stuff is not enough. Does "finished descriptors" really mean
> "in-flight virtqueue buffers"?
A patch of tracking in-flight descriptors is planned. Here I expect
"finished" mean "done" buffers, transmitted and received ACK.
>
> "send a used buffer notification" or "send used buffer notifications"
Yes will fix
>
>> +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
> Does "Record" mean that Virtqueue State fields can only be accessed by
> the driver while device is paused (they may be outdated or invalid while
> the device is unpaused)?
Yes, to avoid race with the device and make device implementation 
easier, this is also mentioned in Patch 4.
>
>> +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> What does "preserve" mean? Does it mean the Device Configuration Space
> is not allowed to change during SUSPEND?
Yes, once the device presents SUSPEND in its device status, it should 
not change any
fields in its Device Configuration Space.

Thanks for your review
>
>> +\item Present SUSPEND in \field{device status}
>> +\end{itemize}
>> +
>>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>   
>>   Each virtio device offers all the features it understands.  During
>> -- 
>> 2.35.3
>>
>>
>> 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/
>>


---------------------------------------------------------------------
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: [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Zhu, Lingshan]

On 8/15/2023 8:29 AM, Jason Wang wrote:
> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> This commit specifies the actions to be taken by the device upon
>> SUSPEND.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   content.tex | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index 074f43e..43bd5de 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>   If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>   and resumes operation upon DRIVER_OK.
>>
>> +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
>> +\begin{itemize}
>> +\item Stop comsuming any descriptors
> Typo.
yes will fix
>
>> +\item Mark all finished descriptors as used and send used buffer notification to the driver
> What happens to the unfinished descriptors?
still in the descriptors table or considered as in-flight, we will post 
a patch tracking in-flight descriptors.
>
>> +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
> This basically means those states are only available after suspending
> or not? It would be still useful for debugging if we allow it without
> suspending.
Yes, for now only allow to read/write the virtqueue state after setting
SUSPEND, this is to avoid race conditions with the device.

Still can suspend then collect the idx for debugging?
>
>> +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> We probably need to define the "pause" here (e.g what happens to the
> inflight descriptors).
Shall we say: freeze both its data-plane and control-plane?
>
> Thanks
>
>> +\item Present SUSPEND in \field{device status}
>> +\end{itemize}
>> +
>>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>
>>   Each virtio device offers all the features it understands.  During
>> --
>> 2.35.3
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>


---------------------------------------------------------------------
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] [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 8/14/2023 11:15 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 15, 2023 at 03:29:03AM +0800, Zhu Lingshan wrote:
>> This commit specifies the constraints of the virtqueue state,
>> and the actions should be taken by the device when SUSPEND
>> and DRIVER_OK is set
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   content.tex | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index 43bd5de..f6ac581 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>   
>>   See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>>   
>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
>> +first before getting or setting Virtqueue State of any virtqueues.
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
> "negotiated"
>
> Please run a spell checker.
Yes
>
>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
>> +used index in the used ring.
>> +
>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
> "accesses against" is not commonly used in English. "accesses to" is more natural.
Yes
>
>> +
>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
>> +the device MUST ignore any accesses against \field{Used State}.
> Same here.
OK
>
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
>> +the Virtqueue State of every virtqueue upon a reset.
> I'm not sure if this statement is necessary since Virtqueue State
> becomes inaccessible when the device is reset. By definition, Virtqueue
> State is the device-internal state of the virtqueue, so it follows
> logically that next time the driver can access the Virtqueue State after
> reset, it contains the current state and not previous state from before
> reset.
Yes, will remove this.
>
>> +
>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
>> +the device MUST record the Virtqueue State of every enabled virtqueue
>> +in \field{Available State} and \field{Used State} respectively,
>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
> "Available State"
Yes
>
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
> "Available State"
Yes
>
>> +when DRIVER_OK is set.
>> +
>>   \input{admin.tex}
>>   
>>   \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>> -- 
>> 2.35.3
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>


---------------------------------------------------------------------
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: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 8/15/2023 8:34 AM, Jason Wang wrote:
> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> This commit specifies the constraints of the virtqueue state,
>> and the actions should be taken by the device when SUSPEND
>> and DRIVER_OK is set
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   content.tex | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index 43bd5de..f6ac581 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>
>>   See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>>
>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
>> +first before getting or setting Virtqueue State of any virtqueues.
> I don't get why this is a must. It could be useful for debugging.
To avoid race conditions with the device and make the device 
implementation easier
>
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
> typo
yes
>
>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
>> +used index in the used ring.
>> +
>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
> Btw, do we need to clarify the behavior of ring reset after suspending?
I think once suspended, the device should ignore resetting a queue
>
>> +
>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
>> +the device MUST ignore any accesses against \field{Used State}.
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
>> +the Virtqueue State of every virtqueue upon a reset.
> Need to define the meaning of "reset" this is important for packed virtqueue.
I will remove this as Stefan suggested.
>
>> +
>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
>> +the device MUST record the Virtqueue State of every enabled virtqueue
>> +in \field{Available State} and \field{Used State} respectively,
>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
> We can just let the device report those states in any case then we
> don't need to care about those details, or did you see any blockers?
Agree, I will add the definition of used_state of splitted vq in the 
next version

Thanks
>
> Thanks
>
>> +
>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
>> +when DRIVER_OK is set.
>> +
>>   \input{admin.tex}
>>
>>   \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>> --
>> 2.35.3
>>


---------------------------------------------------------------------
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: [virtio-comment] Re: [virtio-dev] [RFC PATCH 5/5] virtio-pci: implement VIRTIO_F_QUEUE_STATE

[Zhu, Lingshan]

On 8/14/2023 11:18 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 15, 2023 at 03:29:04AM +0800, Zhu Lingshan wrote:
>> This patch adds two new le16 fields to common configruation structure
>> to support VIRTIO_F_QUEUE_STATE in PCI tranport layer.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   transport-pci.tex | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/transport-pci.tex b/transport-pci.tex
>> index a5c6719..d9bccb0 100644
>> --- a/transport-pci.tex
>> +++ b/transport-pci.tex
>> @@ -321,6 +321,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>           le64 queue_device;              /* read-write */
>>           le16 queue_notif_config_data;   /* read-only for driver */
>>           le16 queue_reset;               /* read-write */
>> +        le16 queue_avail_state;         /* read-write */
>> +        le16 queue_used_state;          /* read-write */
>>   
>>           /* About the administration virtqueue. */
>>           le16 admin_queue_index;         /* read-only for driver */
> This is an incompatible change to the register layout. Please add new
> registers at the end of struct virtio_pci_common_cfg so field offsets
> don't change for existing drivers and devices.
Oh yes, I missed that, thanks
>> @@ -415,6 +417,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>           This field exists only if VIRTIO_F_RING_RESET has been
>>           negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
>>   
>> +\item[\field{queue_avail_state}]
>> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
> "valid"
yes
>
>> +        negotiated. The driver sets and gets the available state of
>> +        the virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
>> +
>> +\item[\field{queue_used_state}]
>> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
> "valid"
yes
>
>> +        negotiated. The driver sets and gets the used state of the
>> +        virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
>> +
>>   \item[\field{admin_queue_index}]
>>           The device uses this to report the index of the first administration virtqueue.
>>           This field is valid only if VIRTIO_F_ADMIN_VQ has been negotiated.
>> @@ -488,6 +500,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>   present either a value of 0 or a power of 2 in
>>   \field{queue_size}.
>>   
>> +If VIRTIO_F_QUEUE_STATE has not been negotiated, the device MUST ignore
>> +any accesses against \field{queue_avail_state} and \field{queue_used_state}.
> "accesses to"
Yes, thanks for your review
>
>> +
>>   If VIRTIO_F_ADMIN_VQ has been negotiated, the value
>>   \field{admin_queue_index} MUST be equal to, or bigger than
>>   \field{num_queues}; also, \field{admin_queue_num} MUST be
>> -- 
>> 2.35.3
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>


---------------------------------------------------------------------
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: [RFC PATCH 5/5] virtio-pci: implement VIRTIO_F_QUEUE_STATE

[Zhu, Lingshan]

On 8/15/2023 8:35 AM, Jason Wang wrote:
> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> This patch adds two new le16 fields to common configruation structure
>> to support VIRTIO_F_QUEUE_STATE in PCI tranport layer.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   transport-pci.tex | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/transport-pci.tex b/transport-pci.tex
>> index a5c6719..d9bccb0 100644
>> --- a/transport-pci.tex
>> +++ b/transport-pci.tex
>> @@ -321,6 +321,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>           le64 queue_device;              /* read-write */
>>           le16 queue_notif_config_data;   /* read-only for driver */
>>           le16 queue_reset;               /* read-write */
>> +        le16 queue_avail_state;         /* read-write */
>> +        le16 queue_used_state;          /* read-write */
> Those need to be placed at the end of this structure at least or we
> need a dedicated new capability.
Yes!
>
>>           /* About the administration virtqueue. */
>>           le16 admin_queue_index;         /* read-only for driver */
>> @@ -415,6 +417,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>           This field exists only if VIRTIO_F_RING_RESET has been
>>           negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
>>
>> +\item[\field{queue_avail_state}]
>> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
>> +        negotiated. The driver sets and gets the available state of
>> +        the virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
>> +
>> +\item[\field{queue_used_state}]
>> +        This field is vaild only if VIRTIO_F_QUEUE_STATE has been
>> +        negotiated. The driver sets and gets the used state of the
>> +        virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
>> +
>>   \item[\field{admin_queue_index}]
>>           The device uses this to report the index of the first administration virtqueue.
>>           This field is valid only if VIRTIO_F_ADMIN_VQ has been negotiated.
>> @@ -488,6 +500,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>   present either a value of 0 or a power of 2 in
>>   \field{queue_size}.
>>
>> +If VIRTIO_F_QUEUE_STATE has not been negotiated, the device MUST ignore
>> +any accesses against \field{queue_avail_state} and \field{queue_used_state}.
>> +
>>   If VIRTIO_F_ADMIN_VQ has been negotiated, the value
>>   \field{admin_queue_index} MUST be equal to, or bigger than
>>   \field{num_queues}; also, \field{admin_queue_num} MUST be
>> --
>> 2.35.3
>>


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 06:31:23PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 8/14/2023 10:30 PM, Stefan Hajnoczi wrote:
> > On Tue, Aug 15, 2023 at 03:29:00AM +0800, Zhu Lingshan wrote:
> > > This patch introudces a new status bit in the device status: SUSPEND.
> > > 
> > > This SUSPEND bit can be used by the driver to suspend a device,
> > > in order to stablize the device states and virtqueue states.
> > > 
> > > Its main use case is live migration.
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > There is an character encoding issue in Eugenio's surname.
> Oh, I copied his SOB form his email, I will copy from git log to fix this,
> thanks for point out it.
> > 
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > ---
> > >   content.tex | 18 ++++++++++++++++++
> > >   1 file changed, 18 insertions(+)
> > This patch hints at the asynchronous nature of the SUSPEND bit (the
> > driver must re-read the Device Status Field) but doesn't explain the
> > rationale or any limits.
> > 
> > For example, is there a timeout or should the driver re-read the Device
> > Status Field forever?
> It depends on the driver, normally we expect this operation can be done
> successfully
> like how the driver/device handles FEATURES_OK.
> 
> Once failed due to:
> 1) driver timeout, the driver can reset the device
> 2) device failure, the device can set NEEDS_RESET.

I mention this because SUSPEND involves quiescing the device so that no
requests are in flight and that can take an unbounded amount of time on
a virtio-blk, virtio-scsi, or virtiofs device. If the driver is busy
waiting for the device to report the SUSPEND bit, then that could take a
long time/forever.

Imagine a virtiofs PCI device implemented in hardware that forwards I/O
to a distributed storage system. If the distributed storage system has a
request in flight then SUSPEND needs to wait for it to complete. The
device has no control over how long that will take, but if it does not
then corruption could occur later on.

There are two issues with long SUSPEND times:
1. Busy wait CPU consumption. Since there is no interrupt that signals
   when the bit is set, the best a driver can do is to back off
   gradually and use timers to avoid hogging the CPU.
2. Synchronous blocking. If the call stack that led the driver to set
   SUSPEND is blocked until the device reports the SUSPEND bit, then
   other parts of the system could experience blocking. For example, the
   VMM might be blocked in a vhost ioctl() call, which makes the guest
   unresponsive.

Making SUSPEND asynchronous is more complicated but would allow long
SUSPEND times to be handled gracefully.

> > 
> > Does the driver need to re-read the Device Status Field after clearing
> > the SUSPEND bit?
> I think the driver should re-read, I will add this in the next version.
> > 
> > > diff --git a/content.tex b/content.tex
> > > index 0a62dce..1bb4401 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
> > >     drive the device.
> > > +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > > +  device has been suspended by the driver.
> > > +
> > >   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > >     an error from which it can't recover.
> > >   \end{description}
> > > @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   recover by issuing a reset.
> > >   \end{note}
> > > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> > > +
> > > +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> > "When setting SUSPEND, ..." would be grammatically correct. Another
> > option is "After setting the SUSPEND bit, ...".
> Will fix in the next version.
> > 
> > > +
> > >   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> > >   The device MUST NOT consume buffers or send any used buffer
> > > @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
> > >   MUST send a device configuration change notification to the driver.
> > > +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> > > +
> > > +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.

I noticed a typo:
"device MUST"

> > > +
> > > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > > +and resumes operation upon DRIVER_OK.
> > I can't parse this sentence. If the driver writes SUSPEND | DRIVER_OK |
> > ... to the Device Status Field, then the device accepts DRIVER_OK and
> > clears SUSPEND?
> > 
> > Why?
> I expect DRIVER_OK can clear SUSPEND, so that the device can resume running
> in case of a failed live migration.
> 
> Maybe I should say: DRIVER_OK clears SUSPEND, and if DRIVER_OK is set to
> a suspended device, the device should resume operation

It's confusing because there are other Device Status Field bits aside
from DRIVER_OK. I wasn't sure what you meant.

I think this is really saying that devices must support the SUSPEND ->
!SUSPEND transition. It's not really about DRIVER_OK because that bit
will be set the entire time (!SUSPEND -> SUSPEND -> !SUSPEND).

Can you rephrase it? For example:

  If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST
  resume operation when the driver clears the SUSPEND bit.

Stefan

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

[virtio-dev] Re: [virtio-comment] [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Stefan Hajnoczi]

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

On Tue, Aug 15, 2023 at 07:07:42PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 8/14/2023 11:00 PM, Stefan Hajnoczi wrote:
> > On Tue, Aug 15, 2023 at 03:29:02AM +0800, Zhu Lingshan wrote:
> > > This commit specifies the actions to be taken by the device upon
> > > SUSPEND.
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > ---
> > >   content.tex | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 074f43e..43bd5de 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > >   and resumes operation upon DRIVER_OK.
> > > +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
> > "when SUSPEND is set" is ambigious. It could mean when the driver writes
> > the Device Status Field, when the device transitions to SUSPEND, or
> > something else. Maybe "before the device reports the SUSPEND bit set in
> > the Device Status Field"?
> Nice catch! will fix: when the driver sets SUSPEND, before the device
> reports the
> SUSPEND bit set in the \field{device status}, the device MUST perform the
> following actions.
> > 
> > > +\begin{itemize}
> > > +\item Stop comsuming any descriptors
> > "consuming"
> yes
> > 
> > It may be more consistent to talk about virtqueue buffers (i.e.
> > requests) rather than descriptors here.
> yes
> > 
> > > +\item Mark all finished descriptors as used and send used buffer notification to the driver
> > The device has to complete everything that is in flight, just completing
> > "finished" stuff is not enough. Does "finished descriptors" really mean
> > "in-flight virtqueue buffers"?
> A patch of tracking in-flight descriptors is planned. Here I expect
> "finished" mean "done" buffers, transmitted and received ACK.

Yes, I don't mean tracking in-flight virtqueue buffers, I mean waiting
until they are marked as used. I think "finished" is unclear and the
text should explain that the device waits for in-flight virtqueue
buffers (in the future this could be relaxed with in-flight tracking
functionality).

> > 
> > "send a used buffer notification" or "send used buffer notifications"
> Yes will fix
> > 
> > > +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
> > Does "Record" mean that Virtqueue State fields can only be accessed by
> > the driver while device is paused (they may be outdated or invalid while
> > the device is unpaused)?
> Yes, to avoid race with the device and make device implementation easier,
> this is also mentioned in Patch 4.
> > 
> > > +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > What does "preserve" mean? Does it mean the Device Configuration Space
> > is not allowed to change during SUSPEND?
> Yes, once the device presents SUSPEND in its device status, it should not
> change any
> fields in its Device Configuration Space.
> 
> Thanks for your review
> > 
> > > +\item Present SUSPEND in \field{device status}
> > > +\end{itemize}
> > > +
> > >   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> > >   Each virtio device offers all the features it understands.  During
> > > -- 
> > > 2.35.3
> > > 
> > > 
> > > 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/
> > > 
> 
> 
> 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/
> 

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

[virtio-dev] Re: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Jason Wang]

On Tue, Aug 15, 2023 at 6:48 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/15/2023 8:37 AM, Jason Wang wrote:
> > On Tue, Aug 15, 2023 at 8:26 AM Jason Wang <jasowang@redhat.com> wrote:
> >> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>> This patch introudces a new status bit in the device status: SUSPEND.
> >>>
> >>> This SUSPEND bit can be used by the driver to suspend a device,
> >>> in order to stablize the device states and virtqueue states.
> >>>
> >>> Its main use case is live migration.
> >> I think patch 3 needs to be squashed into this one.
> Patch 3 mentions Virtqueue State that is introduced in patch 2,

Ok, so let's reorder the let patch 2 come first.

Thanks

> so I think patch 3 should placed after patch 2.
>
> Thanks
> >>
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>> ---
> >>>   content.tex | 18 ++++++++++++++++++
> >>>   1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/content.tex b/content.tex
> >>> index 0a62dce..1bb4401 100644
> >>> --- a/content.tex
> >>> +++ b/content.tex
> >>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
> >>>     drive the device.
> >>>
> >>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> >>> +  device has been suspended by the driver.
> >>> +
> >>>   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> >>>     an error from which it can't recover.
> >>>   \end{description}
> >>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>   recover by issuing a reset.
> >>>   \end{note}
> >>>
> >>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> >>> +
> >>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> >>> +
> >>>   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> >>>
> >>>   The device MUST NOT consume buffers or send any used buffer
> >>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
> >>>   MUST send a device configuration change notification to the driver.
> >>>
> >>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> >> Is there any value to allow SUSPEND to be set without DRIVER_OK?
> If live migration happens when the guest initializing the device,
> maybe the guest driver has already configured some virtqueues or
> modified the config space. At that point, We should allow to suspend
> the device, because the hypervisor should not reset the device at
> that moment.
>
> SUSPEND requires FEATURES_OK because VIRTIO_F_SUSPEND has to be negotiated.
> >>
> >>> +
> >>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> >> typo.
> Yes
> >>
> >> Thanks
> > Btw, it's not clear to me if driver is allowed to clear this bit.
> Patch 1 mentions that DRIVER_OK clears SUSPEND, so that the driver
> can resume the device if LM fails.
>
> Thanks
> >
> > Thanks
> >
> >>> +
> >>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> >>> +and resumes operation upon DRIVER_OK.
> >>> +
> >>>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> >>>
> >>>   Each virtio device offers all the features it understands.  During
> >>> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>>          \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
> >>>          handling features reserved for future use.
> >>>
> >>> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
> >>> +   SUSPEND the device.
> >>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> >>> +
> >>>   \end{description}
> >>>
> >>>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> >>> --
> >>> 2.35.3
> >>>
>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Jason Wang]

On Tue, Aug 15, 2023 at 6:50 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/15/2023 8:37 AM, Jason Wang wrote:
> > On Tue, Aug 15, 2023 at 8:26 AM Jason Wang <jasowang@redhat.com> wrote:
> >> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>> This patch introudces a new status bit in the device status: SUSPEND.
> >>>
> >>> This SUSPEND bit can be used by the driver to suspend a device,
> >>> in order to stablize the device states and virtqueue states.
> >>>
> >>> Its main use case is live migration.
> >> I think patch 3 needs to be squashed into this one.
> >>
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>> ---
> >>>   content.tex | 18 ++++++++++++++++++
> >>>   1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/content.tex b/content.tex
> >>> index 0a62dce..1bb4401 100644
> >>> --- a/content.tex
> >>> +++ b/content.tex
> >>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
> >>>     drive the device.
> >>>
> >>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> >>> +  device has been suspended by the driver.
> >>> +
> >>>   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> >>>     an error from which it can't recover.
> >>>   \end{description}
> >>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>   recover by issuing a reset.
> >>>   \end{note}
> >>>
> >>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> >>> +
> >>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> >>> +
> >>>   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> >>>
> >>>   The device MUST NOT consume buffers or send any used buffer
> >>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
> >>>   MUST send a device configuration change notification to the driver.
> >>>
> >>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> >> Is there any value to allow SUSPEND to be set without DRIVER_OK?
> >>
> >>> +
> >>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> >> typo.
> >>
> >> Thanks
> > Btw, it's not clear to me if driver is allowed to clear this bit.
> To allow terminate a live migration process or resume from a failed live
> migration,
> we allow DRIVER_OK to clear SUSPEND, in patch 1.

Does this imply DRIVER_OK is cleared during SUSPEND? This seems more
complicated than simply allowing clearing this bit?

Thanks

>
> Thanks
> >
> > Thanks
> >
> >>> +
> >>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> >>> +and resumes operation upon DRIVER_OK.
> >>> +
> >>>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> >>>
> >>>   Each virtio device offers all the features it understands.  During
> >>> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>>          \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
> >>>          handling features reserved for future use.
> >>>
> >>> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
> >>> +   SUSPEND the device.
> >>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> >>> +
> >>>   \end{description}
> >>>
> >>>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> >>> --
> >>> 2.35.3
> >>>
>
>
> 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/
>


---------------------------------------------------------------------
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: [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Jason Wang]

On Tue, Aug 15, 2023 at 7:17 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/15/2023 8:29 AM, Jason Wang wrote:
> > On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit specifies the actions to be taken by the device upon
> >> SUSPEND.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   content.tex | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/content.tex b/content.tex
> >> index 074f43e..43bd5de 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>   If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> >>   and resumes operation upon DRIVER_OK.
> >>
> >> +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
> >> +\begin{itemize}
> >> +\item Stop comsuming any descriptors
> > Typo.
> yes will fix
> >
> >> +\item Mark all finished descriptors as used and send used buffer notification to the driver
> > What happens to the unfinished descriptors?
> still in the descriptors table or considered as in-flight, we will post
> a patch tracking in-flight descriptors.

So I think we should either

1) add in-flight descriptors in this series

or

2) force a flush

To make sure the new proposed function is complete.

> >
> >> +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
> > This basically means those states are only available after suspending
> > or not? It would be still useful for debugging if we allow it without
> > suspending.
> Yes, for now only allow to read/write the virtqueue state after setting
> SUSPEND, this is to avoid race conditions with the device.

For debugging, we don't need to care about those races. A lot of
hardware allow to expose those via e.g ethtool.

>
> Still can suspend then collect the idx for debugging?

I mean for runtime debugging.

> >
> >> +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > We probably need to define the "pause" here (e.g what happens to the
> > inflight descriptors).
> Shall we say: freeze both its data-plane and control-plane?

I mean if we've defined "suspend" we can simply use "suspend" instead
of "pause"?

Thanks

> >
> > Thanks
> >
> >> +\item Present SUSPEND in \field{device status}
> >> +\end{itemize}
> >> +
> >>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> >>
> >>   Each virtio device offers all the features it understands.  During
> >> --
> >> 2.35.3
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
>


---------------------------------------------------------------------
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: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Jason Wang]

On Tue, Aug 15, 2023 at 7:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/15/2023 8:34 AM, Jason Wang wrote:
> > On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit specifies the constraints of the virtqueue state,
> >> and the actions should be taken by the device when SUSPEND
> >> and DRIVER_OK is set
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   content.tex | 31 +++++++++++++++++++++++++++++++
> >>   1 file changed, 31 insertions(+)
> >>
> >> diff --git a/content.tex b/content.tex
> >> index 43bd5de..f6ac581 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
> >>
> >>   See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> >>
> >> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
> >> +first before getting or setting Virtqueue State of any virtqueues.
> > I don't get why this is a must. It could be useful for debugging.
> To avoid race conditions with the device and make the device
> implementation easier

replied in another thread.

> >
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
> > typo
> yes
> >
> >> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
> >> +used index in the used ring.
> >> +
> >> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
> >> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
> > Btw, do we need to clarify the behavior of ring reset after suspending?
> I think once suspended, the device should ignore resetting a queue

This needs to be clarified.

Thanks


---------------------------------------------------------------------
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: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Zhu, Lingshan]

On 8/16/2023 9:58 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 6:48 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 8/15/2023 8:37 AM, Jason Wang wrote:
>>> On Tue, Aug 15, 2023 at 8:26 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>>> This patch introudces a new status bit in the device status: SUSPEND.
>>>>>
>>>>> This SUSPEND bit can be used by the driver to suspend a device,
>>>>> in order to stablize the device states and virtqueue states.
>>>>>
>>>>> Its main use case is live migration.
>>>> I think patch 3 needs to be squashed into this one.
>> Patch 3 mentions Virtqueue State that is introduced in patch 2,
> Ok, so let's reorder the let patch 2 come first.
OK, I can work on that

Thanks
>
> Thanks
>
>> so I think patch 3 should placed after patch 2.
>>
>> Thanks
>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---
>>>>>    content.tex | 18 ++++++++++++++++++
>>>>>    1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/content.tex b/content.tex
>>>>> index 0a62dce..1bb4401 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>    \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>>>>>      drive the device.
>>>>>
>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>> +  device has been suspended by the driver.
>>>>> +
>>>>>    \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>      an error from which it can't recover.
>>>>>    \end{description}
>>>>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>    recover by issuing a reset.
>>>>>    \end{note}
>>>>>
>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
>>>>> +
>>>>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
>>>>> +
>>>>>    \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>>>>
>>>>>    The device MUST NOT consume buffers or send any used buffer
>>>>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>    that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>>>>>    MUST send a device configuration change notification to the driver.
>>>>>
>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>>>> Is there any value to allow SUSPEND to be set without DRIVER_OK?
>> If live migration happens when the guest initializing the device,
>> maybe the guest driver has already configured some virtqueues or
>> modified the config space. At that point, We should allow to suspend
>> the device, because the hypervisor should not reset the device at
>> that moment.
>>
>> SUSPEND requires FEATURES_OK because VIRTIO_F_SUSPEND has to be negotiated.
>>>>> +
>>>>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>> typo.
>> Yes
>>>> Thanks
>>> Btw, it's not clear to me if driver is allowed to clear this bit.
>> Patch 1 mentions that DRIVER_OK clears SUSPEND, so that the driver
>> can resume the device if LM fails.
>>
>> Thanks
>>> Thanks
>>>
>>>>> +
>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>>>> +and resumes operation upon DRIVER_OK.
>>>>> +
>>>>>    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>>>
>>>>>    Each virtio device offers all the features it understands.  During
>>>>> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>>>>           \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>>>>>           handling features reserved for future use.
>>>>>
>>>>> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
>>>>> +   SUSPEND the device.
>>>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>>>>> +
>>>>>    \end{description}
>>>>>
>>>>>    \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>>>>> --
>>>>> 2.35.3
>>>>>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Zhu, Lingshan]

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



On 8/16/2023 10:05 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 6:50 PM Zhu, Lingshan<lingshan.zhu@intel.com>  wrote:
>>
>>
>> On 8/15/2023 8:37 AM, Jason Wang wrote:
>>> On Tue, Aug 15, 2023 at 8:26 AM Jason Wang<jasowang@redhat.com>  wrote:
>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan<lingshan.zhu@intel.com>  wrote:
>>>>> This patch introudces a new status bit in the device status: SUSPEND.
>>>>>
>>>>> This SUSPEND bit can be used by the driver to suspend a device,
>>>>> in order to stablize the device states and virtqueue states.
>>>>>
>>>>> Its main use case is live migration.
>>>> I think patch 3 needs to be squashed into this one.
>>>>
>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>> Signed-off-by: Eugenio PÃrez<eperezma@redhat.com>
>>>>> Signed-off-by: Zhu Lingshan<lingshan.zhu@intel.com>
>>>>> ---
>>>>>    content.tex | 18 ++++++++++++++++++
>>>>>    1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/content.tex b/content.tex
>>>>> index 0a62dce..1bb4401 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>    \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>>>>>      drive the device.
>>>>>
>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>> +  device has been suspended by the driver.
>>>>> +
>>>>>    \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>      an error from which it can't recover.
>>>>>    \end{description}
>>>>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>    recover by issuing a reset.
>>>>>    \end{note}
>>>>>
>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
>>>>> +
>>>>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
>>>>> +
>>>>>    \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>>>>
>>>>>    The device MUST NOT consume buffers or send any used buffer
>>>>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>    that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>>>>>    MUST send a device configuration change notification to the driver.
>>>>>
>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>>>> Is there any value to allow SUSPEND to be set without DRIVER_OK?
>>>>
>>>>> +
>>>>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>> typo.
>>>>
>>>> Thanks
>>> Btw, it's not clear to me if driver is allowed to clear this bit.
>> To allow terminate a live migration process or resume from a failed live
>> migration,
>> we allow DRIVER_OK to clear SUSPEND, in patch 1.
> Does this imply DRIVER_OK is cleared during SUSPEND? This seems more
> complicated than simply allowing clearing this bit?
No, SUSPEND does not clears DRIVER_OK, only DRIVER_OK clears SUSPEND.
The spec says: The driver MUST NOT clear a device status bit.
So I think maybe it's better let the device clears SUSPEND.

Thanks
>
> Thanks
>
>> Thanks
>>> Thanks
>>>
>>>>> +
>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>>>> +and resumes operation upon DRIVER_OK.
>>>>> +
>>>>>    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>>>
>>>>>    Each virtio device offers all the features it understands.  During
>>>>> @@ -872,6 +886,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>>>>           \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>>>>>           handling features reserved for future use.
>>>>>
>>>>> +  \item[VIRTIO_F_SUSPEND(41)] This feature indicates that the driver can
>>>>> +   SUSPEND the device.
>>>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>>>>> +
>>>>>    \end{description}
>>>>>
>>>>>    \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>>>>> --
>>>>> 2.35.3
>>>>>
>>
>> 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/
>>

[-- Attachment #2: Type: text/html, Size: 8355 bytes --]

[virtio-dev] Re: [virtio-comment] [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Zhu, Lingshan]

On 8/15/2023 8:33 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 15, 2023 at 07:07:42PM +0800, Zhu, Lingshan wrote:
>>
>> On 8/14/2023 11:00 PM, Stefan Hajnoczi wrote:
>>> On Tue, Aug 15, 2023 at 03:29:02AM +0800, Zhu Lingshan wrote:
>>>> This commit specifies the actions to be taken by the device upon
>>>> SUSPEND.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    content.tex | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 074f43e..43bd5de 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>    If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>>>    and resumes operation upon DRIVER_OK.
>>>> +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
>>> "when SUSPEND is set" is ambigious. It could mean when the driver writes
>>> the Device Status Field, when the device transitions to SUSPEND, or
>>> something else. Maybe "before the device reports the SUSPEND bit set in
>>> the Device Status Field"?
>> Nice catch! will fix: when the driver sets SUSPEND, before the device
>> reports the
>> SUSPEND bit set in the \field{device status}, the device MUST perform the
>> following actions.
>>>> +\begin{itemize}
>>>> +\item Stop comsuming any descriptors
>>> "consuming"
>> yes
>>> It may be more consistent to talk about virtqueue buffers (i.e.
>>> requests) rather than descriptors here.
>> yes
>>>> +\item Mark all finished descriptors as used and send used buffer notification to the driver
>>> The device has to complete everything that is in flight, just completing
>>> "finished" stuff is not enough. Does "finished descriptors" really mean
>>> "in-flight virtqueue buffers"?
>> A patch of tracking in-flight descriptors is planned. Here I expect
>> "finished" mean "done" buffers, transmitted and received ACK.
> Yes, I don't mean tracking in-flight virtqueue buffers, I mean waiting
> until they are marked as used. I think "finished" is unclear and the
> text should explain that the device waits for in-flight virtqueue
> buffers (in the future this could be relaxed with in-flight tracking
> functionality).
Sure, I can add this in the next version: the device MUST wait until all 
descriptors that being processed to finish and mark them as used

And we can replace it with tracking in-flight descriptors in the future

Thanks
>
>>> "send a used buffer notification" or "send used buffer notifications"
>> Yes will fix
>>>> +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
>>> Does "Record" mean that Virtqueue State fields can only be accessed by
>>> the driver while device is paused (they may be outdated or invalid while
>>> the device is unpaused)?
>> Yes, to avoid race with the device and make device implementation easier,
>> this is also mentioned in Patch 4.
>>>> +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>> What does "preserve" mean? Does it mean the Device Configuration Space
>>> is not allowed to change during SUSPEND?
>> Yes, once the device presents SUSPEND in its device status, it should not
>> change any
>> fields in its Device Configuration Space.
>>
>> Thanks for your review
>>>> +\item Present SUSPEND in \field{device status}
>>>> +\end{itemize}
>>>> +
>>>>    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>>    Each virtio device offers all the features it understands.  During
>>>> -- 
>>>> 2.35.3
>>>>
>>>>
>>>> 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/
>>>>
>>
>> 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/
>>


---------------------------------------------------------------------
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: [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Zhu, Lingshan]

On 8/16/2023 10:10 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 7:17 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 8/15/2023 8:29 AM, Jason Wang wrote:
>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> This commit specifies the actions to be taken by the device upon
>>>> SUSPEND.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    content.tex | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 074f43e..43bd5de 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>    If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>>>    and resumes operation upon DRIVER_OK.
>>>>
>>>> +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
>>>> +\begin{itemize}
>>>> +\item Stop comsuming any descriptors
>>> Typo.
>> yes will fix
>>>> +\item Mark all finished descriptors as used and send used buffer notification to the driver
>>> What happens to the unfinished descriptors?
>> still in the descriptors table or considered as in-flight, we will post
>> a patch tracking in-flight descriptors.
> So I think we should either
>
> 1) add in-flight descriptors in this series
>
> or
>
> 2) force a flush
>
> To make sure the new proposed function is complete.
Yes, will fix in the next version: The device must wait for the 
descriptors and flush the buffers.
>
>>>> +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
>>> This basically means those states are only available after suspending
>>> or not? It would be still useful for debugging if we allow it without
>>> suspending.
>> Yes, for now only allow to read/write the virtqueue state after setting
>> SUSPEND, this is to avoid race conditions with the device.
> For debugging, we don't need to care about those races. A lot of
> hardware allow to expose those via e.g ethtool.
How about we allow read vq state without SUSPEND and forbid write vq state?
>
>> Still can suspend then collect the idx for debugging?
> I mean for runtime debugging.
>
>>>> +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>> We probably need to define the "pause" here (e.g what happens to the
>>> inflight descriptors).
>> Shall we say: freeze both its data-plane and control-plane?
> I mean if we've defined "suspend" we can simply use "suspend" instead
> of "pause"?
This section defines SUSPEND behaviors, so actually SUSPEND is defined here.
so if we use suspend in the inner text, looks like a circular reasoning.

Thanks
>
> Thanks
>
>>> Thanks
>>>
>>>> +\item Present SUSPEND in \field{device status}
>>>> +\end{itemize}
>>>> +
>>>>    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>>
>>>>    Each virtio device offers all the features it understands.  During
>>>> --
>>>> 2.35.3
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>


---------------------------------------------------------------------
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: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 8/16/2023 10:11 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 7:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> This commit specifies the constraints of the virtqueue state,
>>>> and the actions should be taken by the device when SUSPEND
>>>> and DRIVER_OK is set
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    content.tex | 31 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 43bd5de..f6ac581 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>
>>>>    See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>>>>
>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
>>>> +first before getting or setting Virtqueue State of any virtqueues.
>>> I don't get why this is a must. It could be useful for debugging.
>> To avoid race conditions with the device and make the device
>> implementation easier
> replied in another thread.
>
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
>>> typo
>> yes
>>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
>>>> +used index in the used ring.
>>>> +
>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
>>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
>>> Btw, do we need to clarify the behavior of ring reset after suspending?
>> I think once suspended, the device should ignore resetting a queue
> This needs to be clarified.
OK, I will clarify this in the resting queue section.

This reminds me I should add: When SUSPEND is set, the \field{device 
status} is still operational for
both the device and the driver. Because after SUSPEND, the driver may 
set DRIVER_OK and the device may
set NEEDS_RESET.

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


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Stefan Hajnoczi]

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

On Wed, Aug 16, 2023 at 12:25:39PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 8/15/2023 8:33 PM, Stefan Hajnoczi wrote:
> > On Tue, Aug 15, 2023 at 07:07:42PM +0800, Zhu, Lingshan wrote:
> > > 
> > > On 8/14/2023 11:00 PM, Stefan Hajnoczi wrote:
> > > > On Tue, Aug 15, 2023 at 03:29:02AM +0800, Zhu Lingshan wrote:
> > > > > This commit specifies the actions to be taken by the device upon
> > > > > SUSPEND.
> > > > > 
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > > ---
> > > > >    content.tex | 9 +++++++++
> > > > >    1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/content.tex b/content.tex
> > > > > index 074f43e..43bd5de 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > > >    If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > > > >    and resumes operation upon DRIVER_OK.
> > > > > +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
> > > > "when SUSPEND is set" is ambigious. It could mean when the driver writes
> > > > the Device Status Field, when the device transitions to SUSPEND, or
> > > > something else. Maybe "before the device reports the SUSPEND bit set in
> > > > the Device Status Field"?
> > > Nice catch! will fix: when the driver sets SUSPEND, before the device
> > > reports the
> > > SUSPEND bit set in the \field{device status}, the device MUST perform the
> > > following actions.
> > > > > +\begin{itemize}
> > > > > +\item Stop comsuming any descriptors
> > > > "consuming"
> > > yes
> > > > It may be more consistent to talk about virtqueue buffers (i.e.
> > > > requests) rather than descriptors here.
> > > yes
> > > > > +\item Mark all finished descriptors as used and send used buffer notification to the driver
> > > > The device has to complete everything that is in flight, just completing
> > > > "finished" stuff is not enough. Does "finished descriptors" really mean
> > > > "in-flight virtqueue buffers"?
> > > A patch of tracking in-flight descriptors is planned. Here I expect
> > > "finished" mean "done" buffers, transmitted and received ACK.
> > Yes, I don't mean tracking in-flight virtqueue buffers, I mean waiting
> > until they are marked as used. I think "finished" is unclear and the
> > text should explain that the device waits for in-flight virtqueue
> > buffers (in the future this could be relaxed with in-flight tracking
> > functionality).
> Sure, I can add this in the next version: the device MUST wait until all
> descriptors that being processed to finish and mark them as used

Sounds good.

> And we can replace it with tracking in-flight descriptors in the future
> 
> Thanks
> > 
> > > > "send a used buffer notification" or "send used buffer notifications"
> > > Yes will fix
> > > > > +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
> > > > Does "Record" mean that Virtqueue State fields can only be accessed by
> > > > the driver while device is paused (they may be outdated or invalid while
> > > > the device is unpaused)?
> > > Yes, to avoid race with the device and make device implementation easier,
> > > this is also mentioned in Patch 4.
> > > > > +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > > > What does "preserve" mean? Does it mean the Device Configuration Space
> > > > is not allowed to change during SUSPEND?
> > > Yes, once the device presents SUSPEND in its device status, it should not
> > > change any
> > > fields in its Device Configuration Space.
> > > 
> > > Thanks for your review
> > > > > +\item Present SUSPEND in \field{device status}
> > > > > +\end{itemize}
> > > > > +
> > > > >    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> > > > >    Each virtio device offers all the features it understands.  During
> > > > > -- 
> > > > > 2.35.3
> > > > > 
> > > > > 
> > > > > 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/
> > > > > 
> > > 
> > > 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/
> > > 
> 
> 
> 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/
> 

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

[virtio-dev] Re: [RFC PATCH 0/5] virtio: introduce SUSPEND bit and vq state

[Zhu, Lingshan]

Hi @Parav

I remember you are interested in live migration, any comments?

Thanks
Zhu Lingshan

On 8/15/2023 3:28 AM, Zhu Lingshan wrote:
> This seires introduces
> 1)a new SUSPEND bit in the device status
> Which is used to suspend the device, so that the device states
> and virtqueue states are stablized.
>
> 2)virtqueue state and accessors, to get and set last_avail_idx
> and last_used_idx of virtqueues.
>
> The main usecase of these new facilities is Live Migration.
>
> Furture work: dirty page tracking and in-flight descriptors.
>
> This is RFC, this series carries on Jason and Eugenio's pervious work.
>
> Any comments are welcome.
>
> Zhu Lingshan (5):
>    virtio: introduce SUSPEND bit in device status
>    virtio: introduce vq state as basic facility
>    virtio: The actions by the device upon SUSPEND
>    virtqueue: constraints for virtqueue state
>    virtio-pci: implement VIRTIO_F_QUEUE_STATE
>
>   content.tex       | 120 ++++++++++++++++++++++++++++++++++++++++++++++
>   transport-pci.tex |  15 ++++++
>   2 files changed, 135 insertions(+)
>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 8/17/2023 4:31 PM, Uminski, Piotr wrote:
>
>> -----Original Message-----
>> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
>> open.org> On Behalf Of Zhu, Lingshan
>> Sent: Tuesday, August 15, 2023 1:30 PM
>> To: Jason Wang <jasowang@redhat.com>
>> Cc: mst@redhat.com; eperezma@redhat.com; cohuck@redhat.com; virtio-
>> comment@lists.oasis-open.org; virtio-dev@lists.oasis-open.org
>> Subject: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for
>> virtqueue state
>>
>>
>>
>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com>
>> wrote:
>>>> This commit specifies the constraints of the virtqueue state,
>>>> and the actions should be taken by the device when SUSPEND
>>>> and DRIVER_OK is set
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    content.tex | 31 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 43bd5de..f6ac581 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>
>>>>    See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap
>> Counters}.
>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio
>> Device / Virtqueue State}
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set
>> SUSPEND in \field{device status}
>>>> +first before getting or setting Virtqueue State of any virtqueues.
>>> I don't get why this is a must. It could be useful for debugging.
>> To avoid race conditions with the device and make the device
>> implementation easier
> It should be possible to set Virtqueue State in two states:
>   - Before DRIVER_OK - as a part of the queue creation at target system
> -  After SUSPEND - when the device was activated and then suspended
> Setting state on active device can produce random results and should be forbidden.
> We may allow to get Virtqueue State on active device, without SUSPEND. However, we should not mandate the device
> to report correct value - on some implementations it may be impossible without suspend.
I agree
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but
>> VIRTIO_RING_F_PACKED not been negotiated,
>>> typo
>> yes
>>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should
>> use the
>>>> +used index in the used ring.
>>>> +
>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio
>> Device / Virtqueue State}
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in
>> \field{device status},
>>>> +the device MUST ignore any accesses against Virtqueue State of any
>> virtqueues.
>>> Btw, do we need to clarify the behavior of ring reset after suspending?
>> I think once suspended, the device should ignore resetting a queue
>>>> +
>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but
>> VIRTIO_RING_F_PACKED is not,
>>>> +the device MUST ignore any accesses against \field{Used State}.
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
>>>> +the Virtqueue State of every virtqueue upon a reset.
>>> Need to define the meaning of "reset" this is important for packed virtqueue.
>> I will remove this as Stefan suggested.
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been
>> negotiaged, when SUSPEND is set,
>>>> +the device MUST record the Virtqueue State of every enabled virtqueue
>>>> +in \field{Available State} and \field{Used State} respectively,
>>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
>>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
>>> We can just let the device report those states in any case then we
>>> don't need to care about those details, or did you see any blockers?
>> Agree, I will add the definition of used_state of splitted vq in the
>> next version
>>
>> Thanks
>>> Thanks
>>>
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but
>> VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>>>> +the device MUST record the available state of every enabled virtqueue in
>> \field{Available State},
>>>> +and restore the available state of every enabled virtqueue from
>> \field{Avaiable State}
>>>> +when DRIVER_OK is set.
>>>> +
>>>>    \input{admin.tex}
>>>>
>>>>    \chapter{General Initialization And Device Operation}\label{sec:General
>> Initialization And Device Operation}
>>>> --
>>>> 2.35.3
>>>>
>>
>> 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/


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Eugenio Perez Martin]

On Tue, Aug 15, 2023 at 2:29 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Aug 15, 2023 at 06:31:23PM +0800, Zhu, Lingshan wrote:
> >
> >
> > On 8/14/2023 10:30 PM, Stefan Hajnoczi wrote:
> > > On Tue, Aug 15, 2023 at 03:29:00AM +0800, Zhu Lingshan wrote:
> > > > This patch introudces a new status bit in the device status: SUSPEND.
> > > >
> > > > This SUSPEND bit can be used by the driver to suspend a device,
> > > > in order to stablize the device states and virtqueue states.
> > > >
> > > > Its main use case is live migration.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > There is an character encoding issue in Eugenio's surname.
> > Oh, I copied his SOB form his email, I will copy from git log to fix this,
> > thanks for point out it.
> > >
> > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > ---
> > > >   content.tex | 18 ++++++++++++++++++
> > > >   1 file changed, 18 insertions(+)
> > > This patch hints at the asynchronous nature of the SUSPEND bit (the
> > > driver must re-read the Device Status Field) but doesn't explain the
> > > rationale or any limits.
> > >
> > > For example, is there a timeout or should the driver re-read the Device
> > > Status Field forever?
> > It depends on the driver, normally we expect this operation can be done
> > successfully
> > like how the driver/device handles FEATURES_OK.
> >
> > Once failed due to:
> > 1) driver timeout, the driver can reset the device
> > 2) device failure, the device can set NEEDS_RESET.
>
> I mention this because SUSPEND involves quiescing the device so that no
> requests are in flight and that can take an unbounded amount of time on
> a virtio-blk, virtio-scsi, or virtiofs device. If the driver is busy
> waiting for the device to report the SUSPEND bit, then that could take a
> long time/forever.
>
> Imagine a virtiofs PCI device implemented in hardware that forwards I/O
> to a distributed storage system. If the distributed storage system has a
> request in flight then SUSPEND needs to wait for it to complete. The
> device has no control over how long that will take, but if it does not
> then corruption could occur later on.
>
> There are two issues with long SUSPEND times:
> 1. Busy wait CPU consumption. Since there is no interrupt that signals
>    when the bit is set, the best a driver can do is to back off
>    gradually and use timers to avoid hogging the CPU.
> 2. Synchronous blocking. If the call stack that led the driver to set
>    SUSPEND is blocked until the device reports the SUSPEND bit, then
>    other parts of the system could experience blocking. For example, the
>    VMM might be blocked in a vhost ioctl() call, which makes the guest
>    unresponsive.
>

I think all of this already happens with ring reset or even a plain
device reset, doesn't it?

In my opinion the best thing the device can do here is to fail the
request after a certain time, the same way it would fail if the
backend distributed storage system gets disconnected or latency gets
out of bounds.

> Making SUSPEND asynchronous is more complicated but would allow long
> SUSPEND times to be handled gracefully.
>

Maybe that should be the direction of the transport vq, so transport
commands are asynchronous and we get rid of all the similar problems
in one shot?

Thanks!

> > >
> > > Does the driver need to re-read the Device Status Field after clearing
> > > the SUSPEND bit?
> > I think the driver should re-read, I will add this in the next version.
> > >
> > > > diff --git a/content.tex b/content.tex
> > > > index 0a62dce..1bb4401 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > >   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
> > > >     drive the device.
> > > > +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > > > +  device has been suspended by the driver.
> > > > +
> > > >   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > > >     an error from which it can't recover.
> > > >   \end{description}
> > > > @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > >   recover by issuing a reset.
> > > >   \end{note}
> > > > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> > > > +
> > > > +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> > > "When setting SUSPEND, ..." would be grammatically correct. Another
> > > option is "After setting the SUSPEND bit, ...".
> > Will fix in the next version.
> > >
> > > > +
> > > >   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> > > >   The device MUST NOT consume buffers or send any used buffer
> > > > @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > >   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
> > > >   MUST send a device configuration change notification to the driver.
> > > > +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> > > > +
> > > > +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>
> I noticed a typo:
> "device MUST"
>
> > > > +
> > > > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > > > +and resumes operation upon DRIVER_OK.
> > > I can't parse this sentence. If the driver writes SUSPEND | DRIVER_OK |
> > > ... to the Device Status Field, then the device accepts DRIVER_OK and
> > > clears SUSPEND?
> > >
> > > Why?
> > I expect DRIVER_OK can clear SUSPEND, so that the device can resume running
> > in case of a failed live migration.
> >
> > Maybe I should say: DRIVER_OK clears SUSPEND, and if DRIVER_OK is set to
> > a suspended device, the device should resume operation
>
> It's confusing because there are other Device Status Field bits aside
> from DRIVER_OK. I wasn't sure what you meant.
>
> I think this is really saying that devices must support the SUSPEND ->
> !SUSPEND transition. It's not really about DRIVER_OK because that bit
> will be set the entire time (!SUSPEND -> SUSPEND -> !SUSPEND).
>
> Can you rephrase it? For example:
>
>   If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST
>   resume operation when the driver clears the SUSPEND bit.
>
> Stefan


---------------------------------------------------------------------
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: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Eugenio Perez Martin]

On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/15/2023 8:34 AM, Jason Wang wrote:
> > On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit specifies the constraints of the virtqueue state,
> >> and the actions should be taken by the device when SUSPEND
> >> and DRIVER_OK is set
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   content.tex | 31 +++++++++++++++++++++++++++++++
> >>   1 file changed, 31 insertions(+)
> >>
> >> diff --git a/content.tex b/content.tex
> >> index 43bd5de..f6ac581 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
> >>
> >>   See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> >>
> >> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
> >> +first before getting or setting Virtqueue State of any virtqueues.
> > I don't get why this is a must. It could be useful for debugging.
> To avoid race conditions with the device and make the device
> implementation easier
> >
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
> > typo
> yes
> >
> >> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
> >> +used index in the used ring.
> >> +
> >> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
> >> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
> > Btw, do we need to clarify the behavior of ring reset after suspending?
> I think once suspended, the device should ignore resetting a queue

Actually shadow virtqueue could benefit from the ability to change vq
properties (addresses) while the device is suspended, and then just
resume it. I've been told that ring reset is overkill for that.

But probably it is better to address it on top, with another feature flag.

> >
> >> +
> >> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
> >> +the device MUST ignore any accesses against \field{Used State}.
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
> >> +the Virtqueue State of every virtqueue upon a reset.
> > Need to define the meaning of "reset" this is important for packed virtqueue.
> I will remove this as Stefan suggested.
> >
> >> +
> >> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
> >> +the device MUST record the Virtqueue State of every enabled virtqueue
> >> +in \field{Available State} and \field{Used State} respectively,
> >> +and correspondingly restore the Virtqueue State of every enabled virtqueue
> >> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
> > We can just let the device report those states in any case then we
> > don't need to care about those details, or did you see any blockers?
> Agree, I will add the definition of used_state of splitted vq in the
> next version
>
> Thanks
> >
> > Thanks
> >
> >> +
> >> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
> >> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
> >> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
> >> +when DRIVER_OK is set.
> >> +
> >>   \input{admin.tex}
> >>
> >>   \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> >> --
> >> 2.35.3
> >>
>


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Stefan Hajnoczi]

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

On Thu, Aug 17, 2023 at 05:15:16PM +0200, Eugenio Perez Martin wrote:
> On Tue, Aug 15, 2023 at 2:29 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Tue, Aug 15, 2023 at 06:31:23PM +0800, Zhu, Lingshan wrote:
> > >
> > >
> > > On 8/14/2023 10:30 PM, Stefan Hajnoczi wrote:
> > > > On Tue, Aug 15, 2023 at 03:29:00AM +0800, Zhu Lingshan wrote:
> > > > > This patch introudces a new status bit in the device status: SUSPEND.
> > > > >
> > > > > This SUSPEND bit can be used by the driver to suspend a device,
> > > > > in order to stablize the device states and virtqueue states.
> > > > >
> > > > > Its main use case is live migration.
> > > > >
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > > There is an character encoding issue in Eugenio's surname.
> > > Oh, I copied his SOB form his email, I will copy from git log to fix this,
> > > thanks for point out it.
> > > >
> > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > > ---
> > > > >   content.tex | 18 ++++++++++++++++++
> > > > >   1 file changed, 18 insertions(+)
> > > > This patch hints at the asynchronous nature of the SUSPEND bit (the
> > > > driver must re-read the Device Status Field) but doesn't explain the
> > > > rationale or any limits.
> > > >
> > > > For example, is there a timeout or should the driver re-read the Device
> > > > Status Field forever?
> > > It depends on the driver, normally we expect this operation can be done
> > > successfully
> > > like how the driver/device handles FEATURES_OK.
> > >
> > > Once failed due to:
> > > 1) driver timeout, the driver can reset the device
> > > 2) device failure, the device can set NEEDS_RESET.
> >
> > I mention this because SUSPEND involves quiescing the device so that no
> > requests are in flight and that can take an unbounded amount of time on
> > a virtio-blk, virtio-scsi, or virtiofs device. If the driver is busy
> > waiting for the device to report the SUSPEND bit, then that could take a
> > long time/forever.
> >
> > Imagine a virtiofs PCI device implemented in hardware that forwards I/O
> > to a distributed storage system. If the distributed storage system has a
> > request in flight then SUSPEND needs to wait for it to complete. The
> > device has no control over how long that will take, but if it does not
> > then corruption could occur later on.
> >
> > There are two issues with long SUSPEND times:
> > 1. Busy wait CPU consumption. Since there is no interrupt that signals
> >    when the bit is set, the best a driver can do is to back off
> >    gradually and use timers to avoid hogging the CPU.
> > 2. Synchronous blocking. If the call stack that led the driver to set
> >    SUSPEND is blocked until the device reports the SUSPEND bit, then
> >    other parts of the system could experience blocking. For example, the
> >    VMM might be blocked in a vhost ioctl() call, which makes the guest
> >    unresponsive.
> >
> 
> I think all of this already happens with ring reset or even a plain
> device reset, doesn't it?

Yes, but keep in mind that the driver has typically already drained
requests when it invokes reset. If SUSPEND is used transparently during
live migration then there really will be requests in flight because the
guest driver is unaware.

> 
> In my opinion the best thing the device can do here is to fail the
> request after a certain time, the same way it would fail if the
> backend distributed storage system gets disconnected or latency gets
> out of bounds.

In order to prevent corruption there needs to be a fence in addition to
a timeout. In other words, the storage backend needs to guarantee that
any requests sent before the fence will be ignored if they are still
encountered.

> > Making SUSPEND asynchronous is more complicated but would allow long
> > SUSPEND times to be handled gracefully.
> >
> 
> Maybe that should be the direction of the transport vq, so transport
> commands are asynchronous and we get rid of all the similar problems
> in one shot?

Yes, a transport virtqueue would make this operation asynchronous.

Stefan

> 
> Thanks!
> 
> > > >
> > > > Does the driver need to re-read the Device Status Field after clearing
> > > > the SUSPEND bit?
> > > I think the driver should re-read, I will add this in the next version.
> > > >
> > > > > diff --git a/content.tex b/content.tex
> > > > > index 0a62dce..1bb4401 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > > >   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
> > > > >     drive the device.
> > > > > +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > > > > +  device has been suspended by the driver.
> > > > > +
> > > > >   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > > > >     an error from which it can't recover.
> > > > >   \end{description}
> > > > > @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > > >   recover by issuing a reset.
> > > > >   \end{note}
> > > > > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> > > > > +
> > > > > +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> > > > "When setting SUSPEND, ..." would be grammatically correct. Another
> > > > option is "After setting the SUSPEND bit, ...".
> > > Will fix in the next version.
> > > >
> > > > > +
> > > > >   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> > > > >   The device MUST NOT consume buffers or send any used buffer
> > > > > @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > > >   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
> > > > >   MUST send a device configuration change notification to the driver.
> > > > > +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> > > > > +
> > > > > +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> >
> > I noticed a typo:
> > "device MUST"
> >
> > > > > +
> > > > > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > > > > +and resumes operation upon DRIVER_OK.
> > > > I can't parse this sentence. If the driver writes SUSPEND | DRIVER_OK |
> > > > ... to the Device Status Field, then the device accepts DRIVER_OK and
> > > > clears SUSPEND?
> > > >
> > > > Why?
> > > I expect DRIVER_OK can clear SUSPEND, so that the device can resume running
> > > in case of a failed live migration.
> > >
> > > Maybe I should say: DRIVER_OK clears SUSPEND, and if DRIVER_OK is set to
> > > a suspended device, the device should resume operation
> >
> > It's confusing because there are other Device Status Field bits aside
> > from DRIVER_OK. I wasn't sure what you meant.
> >
> > I think this is really saying that devices must support the SUSPEND ->
> > !SUSPEND transition. It's not really about DRIVER_OK because that bit
> > will be set the entire time (!SUSPEND -> SUSPEND -> !SUSPEND).
> >
> > Can you rephrase it? For example:
> >
> >   If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST
> >   resume operation when the driver clears the SUSPEND bit.
> >
> > Stefan
> 

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

[virtio-dev] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
> On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> This commit specifies the constraints of the virtqueue state,
>>>> and the actions should be taken by the device when SUSPEND
>>>> and DRIVER_OK is set
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    content.tex | 31 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 43bd5de..f6ac581 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>
>>>>    See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>>>>
>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
>>>> +first before getting or setting Virtqueue State of any virtqueues.
>>> I don't get why this is a must. It could be useful for debugging.
>> To avoid race conditions with the device and make the device
>> implementation easier
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
>>> typo
>> yes
>>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
>>>> +used index in the used ring.
>>>> +
>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
>>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
>>> Btw, do we need to clarify the behavior of ring reset after suspending?
>> I think once suspended, the device should ignore resetting a queue
> Actually shadow virtqueue could benefit from the ability to change vq
> properties (addresses) while the device is suspended, and then just
> resume it. I've been told that ring reset is overkill for that.
If ring reset is overkill, is SUSPEND even more overkill?
>
> But probably it is better to address it on top, with another feature flag.
I think if we want to changing the vq properties, there must be a 
mechanism to
stop the queue then resume the queue.

How about allow setting queue_enable = 0 to stop it and =1 to resume and
force it reinitialize?

Thanks
Zhu Lingshan
>
>>>> +
>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
>>>> +the device MUST ignore any accesses against \field{Used State}.
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
>>>> +the Virtqueue State of every virtqueue upon a reset.
>>> Need to define the meaning of "reset" this is important for packed virtqueue.
>> I will remove this as Stefan suggested.
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
>>>> +the device MUST record the Virtqueue State of every enabled virtqueue
>>>> +in \field{Available State} and \field{Used State} respectively,
>>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
>>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
>>> We can just let the device report those states in any case then we
>>> don't need to care about those details, or did you see any blockers?
>> Agree, I will add the definition of used_state of splitted vq in the
>> next version
>>
>> Thanks
>>> Thanks
>>>
>>>> +
>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>>>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
>>>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
>>>> +when DRIVER_OK is set.
>>>> +
>>>>    \input{admin.tex}
>>>>
>>>>    \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>>>> --
>>>> 2.35.3
>>>>


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Zhu, Lingshan]

On 8/18/2023 12:04 AM, Stefan Hajnoczi wrote:
> On Thu, Aug 17, 2023 at 05:15:16PM +0200, Eugenio Perez Martin wrote:
>> On Tue, Aug 15, 2023 at 2:29 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> On Tue, Aug 15, 2023 at 06:31:23PM +0800, Zhu, Lingshan wrote:
>>>>
>>>> On 8/14/2023 10:30 PM, Stefan Hajnoczi wrote:
>>>>> On Tue, Aug 15, 2023 at 03:29:00AM +0800, Zhu Lingshan wrote:
>>>>>> This patch introudces a new status bit in the device status: SUSPEND.
>>>>>>
>>>>>> This SUSPEND bit can be used by the driver to suspend a device,
>>>>>> in order to stablize the device states and virtqueue states.
>>>>>>
>>>>>> Its main use case is live migration.
>>>>>>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>>>>> There is an character encoding issue in Eugenio's surname.
>>>> Oh, I copied his SOB form his email, I will copy from git log to fix this,
>>>> thanks for point out it.
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>>>>>>    content.tex | 18 ++++++++++++++++++
>>>>>>    1 file changed, 18 insertions(+)
>>>>> This patch hints at the asynchronous nature of the SUSPEND bit (the
>>>>> driver must re-read the Device Status Field) but doesn't explain the
>>>>> rationale or any limits.
>>>>>
>>>>> For example, is there a timeout or should the driver re-read the Device
>>>>> Status Field forever?
>>>> It depends on the driver, normally we expect this operation can be done
>>>> successfully
>>>> like how the driver/device handles FEATURES_OK.
>>>>
>>>> Once failed due to:
>>>> 1) driver timeout, the driver can reset the device
>>>> 2) device failure, the device can set NEEDS_RESET.
>>> I mention this because SUSPEND involves quiescing the device so that no
>>> requests are in flight and that can take an unbounded amount of time on
>>> a virtio-blk, virtio-scsi, or virtiofs device. If the driver is busy
>>> waiting for the device to report the SUSPEND bit, then that could take a
>>> long time/forever.
>>>
>>> Imagine a virtiofs PCI device implemented in hardware that forwards I/O
>>> to a distributed storage system. If the distributed storage system has a
>>> request in flight then SUSPEND needs to wait for it to complete. The
>>> device has no control over how long that will take, but if it does not
>>> then corruption could occur later on.
>>>
>>> There are two issues with long SUSPEND times:
>>> 1. Busy wait CPU consumption. Since there is no interrupt that signals
>>>     when the bit is set, the best a driver can do is to back off
>>>     gradually and use timers to avoid hogging the CPU.
>>> 2. Synchronous blocking. If the call stack that led the driver to set
>>>     SUSPEND is blocked until the device reports the SUSPEND bit, then
>>>     other parts of the system could experience blocking. For example, the
>>>     VMM might be blocked in a vhost ioctl() call, which makes the guest
>>>     unresponsive.
>>>
>> I think all of this already happens with ring reset or even a plain
>> device reset, doesn't it?
> Yes, but keep in mind that the driver has typically already drained
> requests when it invokes reset. If SUSPEND is used transparently during
> live migration then there really will be requests in flight because the
> guest driver is unaware.
I agree, and as discussed before, I think in this series we should say:
the device MUST wait untilall descriptors that being processed to finish 
and mark them as used.

Then in the following series, we should implement in-flight IO tracker.
>
>> In my opinion the best thing the device can do here is to fail the
>> request after a certain time, the same way it would fail if the
>> backend distributed storage system gets disconnected or latency gets
>> out of bounds.
> In order to prevent corruption there needs to be a fence in addition to
> a timeout. In other words, the storage backend needs to guarantee that
> any requests sent before the fence will be ignored if they are still
> encountered.
Please correct me if I misunderstand anything.

I am not sure a remote target is aware of SUSPEND, but the device can
fail SUSPEND by setting NEEDS_RESET for sure. IN this series,
maybe it is best to flush and wait for the IO requests.
>
>>> Making SUSPEND asynchronous is more complicated but would allow long
>>> SUSPEND times to be handled gracefully.
>>>
>> Maybe that should be the direction of the transport vq, so transport
>> commands are asynchronous and we get rid of all the similar problems
>> in one shot?
> Yes, a transport virtqueue would make this operation asynchronous.
I agree

Thanks
Zhu Lingshan
>
> Stefan
>
>> Thanks!
>>
>>>>> Does the driver need to re-read the Device Status Field after clearing
>>>>> the SUSPEND bit?
>>>> I think the driver should re-read, I will add this in the next version.
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 0a62dce..1bb4401 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>    \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>>>>>>      drive the device.
>>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>>> +  device has been suspended by the driver.
>>>>>> +
>>>>>>    \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>>      an error from which it can't recover.
>>>>>>    \end{description}
>>>>>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>    recover by issuing a reset.
>>>>>>    \end{note}
>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
>>>>>> +
>>>>>> +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
>>>>> "When setting SUSPEND, ..." would be grammatically correct. Another
>>>>> option is "After setting the SUSPEND bit, ...".
>>>> Will fix in the next version.
>>>>>> +
>>>>>>    \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>>>>>    The device MUST NOT consume buffers or send any used buffer
>>>>>> @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>    that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>>>>>>    MUST send a device configuration change notification to the driver.
>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>>>>>> +
>>>>>> +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>> I noticed a typo:
>>> "device MUST"
>>>
>>>>>> +
>>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
>>>>>> +and resumes operation upon DRIVER_OK.
>>>>> I can't parse this sentence. If the driver writes SUSPEND | DRIVER_OK |
>>>>> ... to the Device Status Field, then the device accepts DRIVER_OK and
>>>>> clears SUSPEND?
>>>>>
>>>>> Why?
>>>> I expect DRIVER_OK can clear SUSPEND, so that the device can resume running
>>>> in case of a failed live migration.
>>>>
>>>> Maybe I should say: DRIVER_OK clears SUSPEND, and if DRIVER_OK is set to
>>>> a suspended device, the device should resume operation
>>> It's confusing because there are other Device Status Field bits aside
>>> from DRIVER_OK. I wasn't sure what you meant.
>>>
>>> I think this is really saying that devices must support the SUSPEND ->
>>> !SUSPEND transition. It's not really about DRIVER_OK because that bit
>>> will be set the entire time (!SUSPEND -> SUSPEND -> !SUSPEND).
>>>
>>> Can you rephrase it? For example:
>>>
>>>    If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST
>>>    resume operation when the driver clears the SUSPEND bit.
>>>
>>> Stefan


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Jason Wang]

On Thu, Aug 17, 2023 at 4:43 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/17/2023 4:31 PM, Uminski, Piotr wrote:
> >
> >> -----Original Message-----
> >> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> >> open.org> On Behalf Of Zhu, Lingshan
> >> Sent: Tuesday, August 15, 2023 1:30 PM
> >> To: Jason Wang <jasowang@redhat.com>
> >> Cc: mst@redhat.com; eperezma@redhat.com; cohuck@redhat.com; virtio-
> >> comment@lists.oasis-open.org; virtio-dev@lists.oasis-open.org
> >> Subject: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for
> >> virtqueue state
> >>
> >>
> >>
> >> On 8/15/2023 8:34 AM, Jason Wang wrote:
> >>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com>
> >> wrote:
> >>>> This commit specifies the constraints of the virtqueue state,
> >>>> and the actions should be taken by the device when SUSPEND
> >>>> and DRIVER_OK is set
> >>>>
> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> ---
> >>>>    content.tex | 31 +++++++++++++++++++++++++++++++
> >>>>    1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/content.tex b/content.tex
> >>>> index 43bd5de..f6ac581 100644
> >>>> --- a/content.tex
> >>>> +++ b/content.tex
> >>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
> >>>>
> >>>>    See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap
> >> Counters}.
> >>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio
> >> Device / Virtqueue State}
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set
> >> SUSPEND in \field{device status}
> >>>> +first before getting or setting Virtqueue State of any virtqueues.
> >>> I don't get why this is a must. It could be useful for debugging.
> >> To avoid race conditions with the device and make the device
> >> implementation easier
> > It should be possible to set Virtqueue State in two states:
> >   - Before DRIVER_OK - as a part of the queue creation at target system
> > -  After SUSPEND - when the device was activated and then suspended
> > Setting state on active device can produce random results and should be forbidden.
> > We may allow to get Virtqueue State on active device, without SUSPEND. However, we should not mandate the device
> > to report correct value - on some implementations it may be impossible without suspend.

Exactly.

> I agree

Great.

Thanks

> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but
> >> VIRTIO_RING_F_PACKED not been negotiated,
> >>> typo
> >> yes
> >>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should
> >> use the
> >>>> +used index in the used ring.
> >>>> +
> >>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio
> >> Device / Virtqueue State}
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in
> >> \field{device status},
> >>>> +the device MUST ignore any accesses against Virtqueue State of any
> >> virtqueues.
> >>> Btw, do we need to clarify the behavior of ring reset after suspending?
> >> I think once suspended, the device should ignore resetting a queue
> >>>> +
> >>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but
> >> VIRTIO_RING_F_PACKED is not,
> >>>> +the device MUST ignore any accesses against \field{Used State}.
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
> >>>> +the Virtqueue State of every virtqueue upon a reset.
> >>> Need to define the meaning of "reset" this is important for packed virtqueue.
> >> I will remove this as Stefan suggested.
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been
> >> negotiaged, when SUSPEND is set,
> >>>> +the device MUST record the Virtqueue State of every enabled virtqueue
> >>>> +in \field{Available State} and \field{Used State} respectively,
> >>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
> >>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
> >>> We can just let the device report those states in any case then we
> >>> don't need to care about those details, or did you see any blockers?
> >> Agree, I will add the definition of used_state of splitted vq in the
> >> next version
> >>
> >> Thanks
> >>> Thanks
> >>>
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but
> >> VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
> >>>> +the device MUST record the available state of every enabled virtqueue in
> >> \field{Available State},
> >>>> +and restore the available state of every enabled virtqueue from
> >> \field{Avaiable State}
> >>>> +when DRIVER_OK is set.
> >>>> +
> >>>>    \input{admin.tex}
> >>>>
> >>>>    \chapter{General Initialization And Device Operation}\label{sec:General
> >> Initialization And Device Operation}
> >>>> --
> >>>> 2.35.3
> >>>>
> >>
> >> 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/
>


---------------------------------------------------------------------
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: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Eugenio Perez Martin]

On Fri, Aug 18, 2023 at 11:44 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
> > On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> On 8/15/2023 8:34 AM, Jason Wang wrote:
> >>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> This commit specifies the constraints of the virtqueue state,
> >>>> and the actions should be taken by the device when SUSPEND
> >>>> and DRIVER_OK is set
> >>>>
> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> ---
> >>>>    content.tex | 31 +++++++++++++++++++++++++++++++
> >>>>    1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/content.tex b/content.tex
> >>>> index 43bd5de..f6ac581 100644
> >>>> --- a/content.tex
> >>>> +++ b/content.tex
> >>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
> >>>>
> >>>>    See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> >>>>
> >>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
> >>>> +first before getting or setting Virtqueue State of any virtqueues.
> >>> I don't get why this is a must. It could be useful for debugging.
> >> To avoid race conditions with the device and make the device
> >> implementation easier
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
> >>> typo
> >> yes
> >>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
> >>>> +used index in the used ring.
> >>>> +
> >>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
> >>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
> >>> Btw, do we need to clarify the behavior of ring reset after suspending?
> >> I think once suspended, the device should ignore resetting a queue
> > Actually shadow virtqueue could benefit from the ability to change vq
> > properties (addresses) while the device is suspended, and then just
> > resume it. I've been told that ring reset is overkill for that.
> If ring reset is overkill, is SUSPEND even more overkill?

It depends on the cost of recreating the vq in the device I think. But
it has more to do with *what* is changed in the vq, as it seems some
parameters (vq size) has more impact than others like vq address. The
way to stop the device does not affect, but ring reset offers the
possibility of change all of the parameters already.

Adding Si-Wei and Dragos here, as they pointed it out in the
virtio-networking upstream meeting.

> >
> > But probably it is better to address it on top, with another feature flag.
> I think if we want to changing the vq properties, there must be a
> mechanism to
> stop the queue then resume the queue.
>
> How about allow setting queue_enable = 0 to stop it and =1 to resume and
> force it reinitialize?
>

Yes, I think that is better suited. But maybe this is better to be
added on top, so we maintain this series small.

Thanks!

> Thanks
> Zhu Lingshan
> >
> >>>> +
> >>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
> >>>> +the device MUST ignore any accesses against \field{Used State}.
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
> >>>> +the Virtqueue State of every virtqueue upon a reset.
> >>> Need to define the meaning of "reset" this is important for packed virtqueue.
> >> I will remove this as Stefan suggested.
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
> >>>> +the device MUST record the Virtqueue State of every enabled virtqueue
> >>>> +in \field{Available State} and \field{Used State} respectively,
> >>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
> >>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
> >>> We can just let the device report those states in any case then we
> >>> don't need to care about those details, or did you see any blockers?
> >> Agree, I will add the definition of used_state of splitted vq in the
> >> next version
> >>
> >> Thanks
> >>> Thanks
> >>>
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
> >>>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
> >>>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
> >>>> +when DRIVER_OK is set.
> >>>> +
> >>>>    \input{admin.tex}
> >>>>
> >>>>    \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> >>>> --
> >>>> 2.35.3
> >>>>
>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 8/21/2023 5:26 PM, Eugenio Perez Martin wrote:
> On Fri, Aug 18, 2023 at 11:44 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
>>> On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>>
>>>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>>>> This commit specifies the constraints of the virtqueue state,
>>>>>> and the actions should be taken by the device when SUSPEND
>>>>>> and DRIVER_OK is set
>>>>>>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>>>>>>     content.tex | 31 +++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 43bd5de..f6ac581 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>>>
>>>>>>     See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>>>>>>
>>>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
>>>>>> +first before getting or setting Virtqueue State of any virtqueues.
>>>>> I don't get why this is a must. It could be useful for debugging.
>>>> To avoid race conditions with the device and make the device
>>>> implementation easier
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
>>>>> typo
>>>> yes
>>>>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
>>>>>> +used index in the used ring.
>>>>>> +
>>>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
>>>>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
>>>>> Btw, do we need to clarify the behavior of ring reset after suspending?
>>>> I think once suspended, the device should ignore resetting a queue
>>> Actually shadow virtqueue could benefit from the ability to change vq
>>> properties (addresses) while the device is suspended, and then just
>>> resume it. I've been told that ring reset is overkill for that.
>> If ring reset is overkill, is SUSPEND even more overkill?
> It depends on the cost of recreating the vq in the device I think. But
> it has more to do with *what* is changed in the vq, as it seems some
> parameters (vq size) has more impact than others like vq address. The
> way to stop the device does not affect, but ring reset offers the
> possibility of change all of the parameters already.
>
> Adding Si-Wei and Dragos here, as they pointed it out in the
> virtio-networking upstream meeting.
>
>>> But probably it is better to address it on top, with another feature flag.
>> I think if we want to changing the vq properties, there must be a
>> mechanism to
>> stop the queue then resume the queue.
>>
>> How about allow setting queue_enable = 0 to stop it and =1 to resume and
>> force it reinitialize?
>>
> Yes, I think that is better suited. But maybe this is better to be
> added on top, so we maintain this series small.
OK, I can work out a patch in this series implementing it.
And as discussed in another thread with Jason in this series, it
is better to forbid resetting a vq after SUSPEND.

Thanks
>
> Thanks!
>
>> Thanks
>> Zhu Lingshan
>>>>>> +
>>>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
>>>>>> +the device MUST ignore any accesses against \field{Used State}.
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
>>>>>> +the Virtqueue State of every virtqueue upon a reset.
>>>>> Need to define the meaning of "reset" this is important for packed virtqueue.
>>>> I will remove this as Stefan suggested.
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
>>>>>> +the device MUST record the Virtqueue State of every enabled virtqueue
>>>>>> +in \field{Available State} and \field{Used State} respectively,
>>>>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
>>>>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
>>>>> We can just let the device report those states in any case then we
>>>>> don't need to care about those details, or did you see any blockers?
>>>> Agree, I will add the definition of used_state of splitted vq in the
>>>> next version
>>>>
>>>> Thanks
>>>>> Thanks
>>>>>
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>>>>>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
>>>>>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
>>>>>> +when DRIVER_OK is set.
>>>>>> +
>>>>>>     \input{admin.tex}
>>>>>>
>>>>>>     \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>>>>>> --
>>>>>> 2.35.3
>>>>>>
>
> 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/
>


---------------------------------------------------------------------
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: [virtio-comment] [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

[Stefan Hajnoczi]

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

On Fri, Aug 18, 2023 at 05:55:42PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 8/18/2023 12:04 AM, Stefan Hajnoczi wrote:
> > On Thu, Aug 17, 2023 at 05:15:16PM +0200, Eugenio Perez Martin wrote:
> > > On Tue, Aug 15, 2023 at 2:29 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > > On Tue, Aug 15, 2023 at 06:31:23PM +0800, Zhu, Lingshan wrote:
> > > > > 
> > > > > On 8/14/2023 10:30 PM, Stefan Hajnoczi wrote:
> > > > > > On Tue, Aug 15, 2023 at 03:29:00AM +0800, Zhu Lingshan wrote:
> > > > > > > This patch introudces a new status bit in the device status: SUSPEND.
> > > > > > > 
> > > > > > > This SUSPEND bit can be used by the driver to suspend a device,
> > > > > > > in order to stablize the device states and virtqueue states.
> > > > > > > 
> > > > > > > Its main use case is live migration.
> > > > > > > 
> > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > > > > There is an character encoding issue in Eugenio's surname.
> > > > > Oh, I copied his SOB form his email, I will copy from git log to fix this,
> > > > > thanks for point out it.
> > > > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > > > > ---
> > > > > > >    content.tex | 18 ++++++++++++++++++
> > > > > > >    1 file changed, 18 insertions(+)
> > > > > > This patch hints at the asynchronous nature of the SUSPEND bit (the
> > > > > > driver must re-read the Device Status Field) but doesn't explain the
> > > > > > rationale or any limits.
> > > > > > 
> > > > > > For example, is there a timeout or should the driver re-read the Device
> > > > > > Status Field forever?
> > > > > It depends on the driver, normally we expect this operation can be done
> > > > > successfully
> > > > > like how the driver/device handles FEATURES_OK.
> > > > > 
> > > > > Once failed due to:
> > > > > 1) driver timeout, the driver can reset the device
> > > > > 2) device failure, the device can set NEEDS_RESET.
> > > > I mention this because SUSPEND involves quiescing the device so that no
> > > > requests are in flight and that can take an unbounded amount of time on
> > > > a virtio-blk, virtio-scsi, or virtiofs device. If the driver is busy
> > > > waiting for the device to report the SUSPEND bit, then that could take a
> > > > long time/forever.
> > > > 
> > > > Imagine a virtiofs PCI device implemented in hardware that forwards I/O
> > > > to a distributed storage system. If the distributed storage system has a
> > > > request in flight then SUSPEND needs to wait for it to complete. The
> > > > device has no control over how long that will take, but if it does not
> > > > then corruption could occur later on.
> > > > 
> > > > There are two issues with long SUSPEND times:
> > > > 1. Busy wait CPU consumption. Since there is no interrupt that signals
> > > >     when the bit is set, the best a driver can do is to back off
> > > >     gradually and use timers to avoid hogging the CPU.
> > > > 2. Synchronous blocking. If the call stack that led the driver to set
> > > >     SUSPEND is blocked until the device reports the SUSPEND bit, then
> > > >     other parts of the system could experience blocking. For example, the
> > > >     VMM might be blocked in a vhost ioctl() call, which makes the guest
> > > >     unresponsive.
> > > > 
> > > I think all of this already happens with ring reset or even a plain
> > > device reset, doesn't it?
> > Yes, but keep in mind that the driver has typically already drained
> > requests when it invokes reset. If SUSPEND is used transparently during
> > live migration then there really will be requests in flight because the
> > guest driver is unaware.
> I agree, and as discussed before, I think in this series we should say:
> the device MUST wait untilall descriptors that being processed to finish and
> mark them as used.

Sounds good.

> Then in the following series, we should implement in-flight IO tracker.
> > 
> > > In my opinion the best thing the device can do here is to fail the
> > > request after a certain time, the same way it would fail if the
> > > backend distributed storage system gets disconnected or latency gets
> > > out of bounds.
> > In order to prevent corruption there needs to be a fence in addition to
> > a timeout. In other words, the storage backend needs to guarantee that
> > any requests sent before the fence will be ignored if they are still
> > encountered.
> Please correct me if I misunderstand anything.
> 
> I am not sure a remote target is aware of SUSPEND, but the device can
> fail SUSPEND by setting NEEDS_RESET for sure. IN this series,
> maybe it is best to flush and wait for the IO requests.

Yes, it is necessary to wait for pending I/O.

I was pointing out that timeouts implemented inside the storage
initiator (client) are unsafe. The storage target (server) needs to
participate in stopping in-flight I/O one way or another (timeout,
cancellation, fence, etc).

Stefan

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

[virtio-dev] Re: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 8/21/2023 5:26 PM, Eugenio Perez Martin wrote:
> On Fri, Aug 18, 2023 at 11:44 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
>>> On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>>
>>>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>>>> This commit specifies the constraints of the virtqueue state,
>>>>>> and the actions should be taken by the device when SUSPEND
>>>>>> and DRIVER_OK is set
>>>>>>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>>>>>>     content.tex | 31 +++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 43bd5de..f6ac581 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>>>
>>>>>>     See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>>>>>>
>>>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
>>>>>> +first before getting or setting Virtqueue State of any virtqueues.
>>>>> I don't get why this is a must. It could be useful for debugging.
>>>> To avoid race conditions with the device and make the device
>>>> implementation easier
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
>>>>> typo
>>>> yes
>>>>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
>>>>>> +used index in the used ring.
>>>>>> +
>>>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
>>>>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
>>>>> Btw, do we need to clarify the behavior of ring reset after suspending?
>>>> I think once suspended, the device should ignore resetting a queue
>>> Actually shadow virtqueue could benefit from the ability to change vq
>>> properties (addresses) while the device is suspended, and then just
>>> resume it. I've been told that ring reset is overkill for that.
>> If ring reset is overkill, is SUSPEND even more overkill?
> It depends on the cost of recreating the vq in the device I think. But
> it has more to do with *what* is changed in the vq, as it seems some
> parameters (vq size) has more impact than others like vq address. The
> way to stop the device does not affect, but ring reset offers the
> possibility of change all of the parameters already.
>
> Adding Si-Wei and Dragos here, as they pointed it out in the
> virtio-networking upstream meeting.
>
>>> But probably it is better to address it on top, with another feature flag.
>> I think if we want to changing the vq properties, there must be a
>> mechanism to
>> stop the queue then resume the queue.
>>
>> How about allow setting queue_enable = 0 to stop it and =1 to resume and
>> force it reinitialize?
>>
> Yes, I think that is better suited. But maybe this is better to be
> added on top, so we maintain this series small.
Hi Eugenio,

I have a second thought while implementing above queue_enable = 0,
it doesn't provide more advantages over queue_reset:

1) queue_reset can help to stop a queue and the vq properties can be
reconfigured during queue_reset --> queue_enable.

2) once the driver sees SUSPEND presented by the device, it assume the
device states and vq states are stable, at that point the driver can
read reliable device configurations. So vq reset should be ignored
once SUSPEND is present and if we implement queue stop, it should be
ignored too when SUSPEND.

3) the device should only accept resetting a queue when !SUSPEND and
the driver can flush the queue buffers before resetting it to avoid 
losing buffers,
and we will have tracker for in-flight descriptors later.

Any thoughts?

Thanks
>
> Thanks!
>
>> Thanks
>> Zhu Lingshan
>>>>>> +
>>>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
>>>>>> +the device MUST ignore any accesses against \field{Used State}.
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
>>>>>> +the Virtqueue State of every virtqueue upon a reset.
>>>>> Need to define the meaning of "reset" this is important for packed virtqueue.
>>>> I will remove this as Stefan suggested.
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
>>>>>> +the device MUST record the Virtqueue State of every enabled virtqueue
>>>>>> +in \field{Available State} and \field{Used State} respectively,
>>>>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
>>>>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
>>>>> We can just let the device report those states in any case then we
>>>>> don't need to care about those details, or did you see any blockers?
>>>> Agree, I will add the definition of used_state of splitted vq in the
>>>> next version
>>>>
>>>> Thanks
>>>>> Thanks
>>>>>
>>>>>> +
>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>>>>>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
>>>>>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
>>>>>> +when DRIVER_OK is set.
>>>>>> +
>>>>>>     \input{admin.tex}
>>>>>>
>>>>>>     \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>>>>>> --
>>>>>> 2.35.3
>>>>>>
>
> 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/
>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Eugenio Perez Martin]

On Tue, Sep 5, 2023 at 11:08 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/21/2023 5:26 PM, Eugenio Perez Martin wrote:
> > On Fri, Aug 18, 2023 at 11:44 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
> >>> On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>>>
> >>>> On 8/15/2023 8:34 AM, Jason Wang wrote:
> >>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>>>>> This commit specifies the constraints of the virtqueue state,
> >>>>>> and the actions should be taken by the device when SUSPEND
> >>>>>> and DRIVER_OK is set
> >>>>>>
> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>>> ---
> >>>>>>     content.tex | 31 +++++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 31 insertions(+)
> >>>>>>
> >>>>>> diff --git a/content.tex b/content.tex
> >>>>>> index 43bd5de..f6ac581 100644
> >>>>>> --- a/content.tex
> >>>>>> +++ b/content.tex
> >>>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
> >>>>>>
> >>>>>>     See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> >>>>>>
> >>>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >>>>>> +
> >>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
> >>>>>> +first before getting or setting Virtqueue State of any virtqueues.
> >>>>> I don't get why this is a must. It could be useful for debugging.
> >>>> To avoid race conditions with the device and make the device
> >>>> implementation easier
> >>>>>> +
> >>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
> >>>>> typo
> >>>> yes
> >>>>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
> >>>>>> +used index in the used ring.
> >>>>>> +
> >>>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >>>>>> +
> >>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
> >>>>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
> >>>>> Btw, do we need to clarify the behavior of ring reset after suspending?
> >>>> I think once suspended, the device should ignore resetting a queue
> >>> Actually shadow virtqueue could benefit from the ability to change vq
> >>> properties (addresses) while the device is suspended, and then just
> >>> resume it. I've been told that ring reset is overkill for that.
> >> If ring reset is overkill, is SUSPEND even more overkill?
> > It depends on the cost of recreating the vq in the device I think. But
> > it has more to do with *what* is changed in the vq, as it seems some
> > parameters (vq size) has more impact than others like vq address. The
> > way to stop the device does not affect, but ring reset offers the
> > possibility of change all of the parameters already.
> >
> > Adding Si-Wei and Dragos here, as they pointed it out in the
> > virtio-networking upstream meeting.
> >
> >>> But probably it is better to address it on top, with another feature flag.
> >> I think if we want to changing the vq properties, there must be a
> >> mechanism to
> >> stop the queue then resume the queue.
> >>
> >> How about allow setting queue_enable = 0 to stop it and =1 to resume and
> >> force it reinitialize?
> >>
> > Yes, I think that is better suited. But maybe this is better to be
> > added on top, so we maintain this series small.
> Hi Eugenio,
>
> I have a second thought while implementing above queue_enable = 0,
> it doesn't provide more advantages over queue_reset:
>
> 1) queue_reset can help to stop a queue and the vq properties can be
> reconfigured during queue_reset --> queue_enable.
>
> 2) once the driver sees SUSPEND presented by the device, it assume the
> device states and vq states are stable, at that point the driver can
> read reliable device configurations. So vq reset should be ignored
> once SUSPEND is present and if we implement queue stop, it should be
> ignored too when SUSPEND.
>

The relation between SUSPEND and ring_reset needs to be described in
this series, yes. This is a good start, but I'm not sure if this one
meets all the requirements for SW assisted live migration.

We can always add new feature flags to define a different interaction
in the future, like for devices that can support the change of vq
attributes in the suspend. To not steal the merit, this idea was
proposed by Si-Wei in a recent virtio-networking meeting.

> 3) the device should only accept resetting a queue when !SUSPEND and
> the driver can flush the queue buffers before resetting it to avoid
> losing buffers,
> and we will have tracker for in-flight descriptors later.
>
> Any thoughts?
>
> Thanks
> >
> > Thanks!
> >
> >> Thanks
> >> Zhu Lingshan
> >>>>>> +
> >>>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
> >>>>>> +the device MUST ignore any accesses against \field{Used State}.
> >>>>>> +
> >>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
> >>>>>> +the Virtqueue State of every virtqueue upon a reset.
> >>>>> Need to define the meaning of "reset" this is important for packed virtqueue.
> >>>> I will remove this as Stefan suggested.
> >>>>>> +
> >>>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
> >>>>>> +the device MUST record the Virtqueue State of every enabled virtqueue
> >>>>>> +in \field{Available State} and \field{Used State} respectively,
> >>>>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
> >>>>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
> >>>>> We can just let the device report those states in any case then we
> >>>>> don't need to care about those details, or did you see any blockers?
> >>>> Agree, I will add the definition of used_state of splitted vq in the
> >>>> next version
> >>>>
> >>>> Thanks
> >>>>> Thanks
> >>>>>
> >>>>>> +
> >>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
> >>>>>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
> >>>>>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
> >>>>>> +when DRIVER_OK is set.
> >>>>>> +
> >>>>>>     \input{admin.tex}
> >>>>>>
> >>>>>>     \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> >>>>>> --
> >>>>>> 2.35.3
> >>>>>>
> >
> > 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/
> >
>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 9/7/2023 4:09 PM, Eugenio Perez Martin wrote:
> On Tue, Sep 5, 2023 at 11:08 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 8/21/2023 5:26 PM, Eugenio Perez Martin wrote:
>>> On Fri, Aug 18, 2023 at 11:44 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>>
>>>> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
>>>>> On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>>>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>>>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>>>>>> This commit specifies the constraints of the virtqueue state,
>>>>>>>> and the actions should be taken by the device when SUSPEND
>>>>>>>> and DRIVER_OK is set
>>>>>>>>
>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>>> ---
>>>>>>>>      content.tex | 31 +++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 31 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>>> index 43bd5de..f6ac581 100644
>>>>>>>> --- a/content.tex
>>>>>>>> +++ b/content.tex
>>>>>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>>>>>
>>>>>>>>      See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
>>>>>>>>
>>>>>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>>>>>> +
>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
>>>>>>>> +first before getting or setting Virtqueue State of any virtqueues.
>>>>>>> I don't get why this is a must. It could be useful for debugging.
>>>>>> To avoid race conditions with the device and make the device
>>>>>> implementation easier
>>>>>>>> +
>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
>>>>>>> typo
>>>>>> yes
>>>>>>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
>>>>>>>> +used index in the used ring.
>>>>>>>> +
>>>>>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
>>>>>>>> +
>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
>>>>>>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
>>>>>>> Btw, do we need to clarify the behavior of ring reset after suspending?
>>>>>> I think once suspended, the device should ignore resetting a queue
>>>>> Actually shadow virtqueue could benefit from the ability to change vq
>>>>> properties (addresses) while the device is suspended, and then just
>>>>> resume it. I've been told that ring reset is overkill for that.
>>>> If ring reset is overkill, is SUSPEND even more overkill?
>>> It depends on the cost of recreating the vq in the device I think. But
>>> it has more to do with *what* is changed in the vq, as it seems some
>>> parameters (vq size) has more impact than others like vq address. The
>>> way to stop the device does not affect, but ring reset offers the
>>> possibility of change all of the parameters already.
>>>
>>> Adding Si-Wei and Dragos here, as they pointed it out in the
>>> virtio-networking upstream meeting.
>>>
>>>>> But probably it is better to address it on top, with another feature flag.
>>>> I think if we want to changing the vq properties, there must be a
>>>> mechanism to
>>>> stop the queue then resume the queue.
>>>>
>>>> How about allow setting queue_enable = 0 to stop it and =1 to resume and
>>>> force it reinitialize?
>>>>
>>> Yes, I think that is better suited. But maybe this is better to be
>>> added on top, so we maintain this series small.
>> Hi Eugenio,
>>
>> I have a second thought while implementing above queue_enable = 0,
>> it doesn't provide more advantages over queue_reset:
>>
>> 1) queue_reset can help to stop a queue and the vq properties can be
>> reconfigured during queue_reset --> queue_enable.
>>
>> 2) once the driver sees SUSPEND presented by the device, it assume the
>> device states and vq states are stable, at that point the driver can
>> read reliable device configurations. So vq reset should be ignored
>> once SUSPEND is present and if we implement queue stop, it should be
>> ignored too when SUSPEND.
>>
> The relation between SUSPEND and ring_reset needs to be described in
> this series, yes. This is a good start, but I'm not sure if this one
> meets all the requirements for SW assisted live migration.
>
> We can always add new feature flags to define a different interaction
> in the future, like for devices that can support the change of vq
> attributes in the suspend. To not steal the merit, this idea was
> proposed by Si-Wei in a recent virtio-networking meeting.
If so, we even don't need a new feature bit. We can just allow
resetting vqs after the device presenting SUSPEND.

The device presenting SUSPEND indicates that the device config space
is stabilized at that moment, ready for the driver to fetch fields data 
there.

Then the driver is allowed to reset, re-config and re-enable the vqs.

The only requirement is: The driver is responsible for maintain
the integrity and validity of the config space fields, because
the device is ready-only to the config space at that moment(SUSPEND-ed)
and the driver should be responsible for its actions, perform proper
synchronizations, e.g., re-read.

Does this work for you?

Thanks
>
>> 3) the device should only accept resetting a queue when !SUSPEND and
>> the driver can flush the queue buffers before resetting it to avoid
>> losing buffers,
>> and we will have tracker for in-flight descriptors later.
>>
>> Any thoughts?
>>
>> Thanks
>>> Thanks!
>>>
>>>> Thanks
>>>> Zhu Lingshan
>>>>>>>> +
>>>>>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
>>>>>>>> +the device MUST ignore any accesses against \field{Used State}.
>>>>>>>> +
>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
>>>>>>>> +the Virtqueue State of every virtqueue upon a reset.
>>>>>>> Need to define the meaning of "reset" this is important for packed virtqueue.
>>>>>> I will remove this as Stefan suggested.
>>>>>>>> +
>>>>>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
>>>>>>>> +the device MUST record the Virtqueue State of every enabled virtqueue
>>>>>>>> +in \field{Available State} and \field{Used State} respectively,
>>>>>>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
>>>>>>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
>>>>>>> We can just let the device report those states in any case then we
>>>>>>> don't need to care about those details, or did you see any blockers?
>>>>>> Agree, I will add the definition of used_state of splitted vq in the
>>>>>> next version
>>>>>>
>>>>>> Thanks
>>>>>>> Thanks
>>>>>>>
>>>>>>>> +
>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>>>>>>>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
>>>>>>>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
>>>>>>>> +when DRIVER_OK is set.
>>>>>>>> +
>>>>>>>>      \input{admin.tex}
>>>>>>>>
>>>>>>>>      \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>>>>>>>> --
>>>>>>>> 2.35.3
>>>>>>>>
>>> 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/
>>>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Si-Wei Liu]

On 9/7/2023 2:34 AM, Zhu, Lingshan wrote:
>
>
> On 9/7/2023 4:09 PM, Eugenio Perez Martin wrote:
>> On Tue, Sep 5, 2023 at 11:08 AM Zhu, Lingshan 
>> <lingshan.zhu@intel.com> wrote:
>>>
>>>
>>> On 8/21/2023 5:26 PM, Eugenio Perez Martin wrote:
>>>> On Fri, Aug 18, 2023 at 11:44 AM Zhu, Lingshan 
>>>> <lingshan.zhu@intel.com> wrote:
>>>>>
>>>>> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
>>>>>> On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan 
>>>>>> <lingshan.zhu@intel.com> wrote:
>>>>>>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>>>>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan 
>>>>>>>> <lingshan.zhu@intel.com> wrote:
>>>>>>>>> This commit specifies the constraints of the virtqueue state,
>>>>>>>>> and the actions should be taken by the device when SUSPEND
>>>>>>>>> and DRIVER_OK is set
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>>>> ---
>>>>>>>>>      content.tex | 31 +++++++++++++++++++++++++++++++
>>>>>>>>>      1 file changed, 31 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>>>> index 43bd5de..f6ac581 100644
>>>>>>>>> --- a/content.tex
>>>>>>>>> +++ b/content.tex
>>>>>>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>>>>>>
>>>>>>>>>      See also \ref{sec:Packed Virtqueues / Driver and Device 
>>>>>>>>> Ring Wrap Counters}.
>>>>>>>>>
>>>>>>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic 
>>>>>>>>> Facilities of a Virtio Device / Virtqueue State}
>>>>>>>>> +
>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST 
>>>>>>>>> set SUSPEND in \field{device status}
>>>>>>>>> +first before getting or setting Virtqueue State of any 
>>>>>>>>> virtqueues.
>>>>>>>> I don't get why this is a must. It could be useful for debugging.
>>>>>>> To avoid race conditions with the device and make the device
>>>>>>> implementation easier
>>>>>>>>> +
>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but 
>>>>>>>>> VIRTIO_RING_F_PACKED not been negotiated,
>>>>>>>> typo
>>>>>>> yes
>>>>>>>>> +the driver MUST NOT access \field{Used State} of any 
>>>>>>>>> virtqueues, it should use the
>>>>>>>>> +used index in the used ring.
>>>>>>>>> +
>>>>>>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic 
>>>>>>>>> Facilities of a Virtio Device / Virtqueue State}
>>>>>>>>> +
>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is 
>>>>>>>>> not set in \field{device status},
>>>>>>>>> +the device MUST ignore any accesses against Virtqueue State 
>>>>>>>>> of any virtqueues.
>>>>>>>> Btw, do we need to clarify the behavior of ring reset after 
>>>>>>>> suspending?
>>>>>>> I think once suspended, the device should ignore resetting a queue
>>>>>> Actually shadow virtqueue could benefit from the ability to 
>>>>>> change vq
>>>>>> properties (addresses) while the device is suspended, and then just
>>>>>> resume it. I've been told that ring reset is overkill for that.
>>>>> If ring reset is overkill, is SUSPEND even more overkill?
>>>> It depends on the cost of recreating the vq in the device I think. But
>>>> it has more to do with *what* is changed in the vq, as it seems some
>>>> parameters (vq size) has more impact than others like vq address. The
>>>> way to stop the device does not affect, but ring reset offers the
>>>> possibility of change all of the parameters already.
>>>>
>>>> Adding Si-Wei and Dragos here, as they pointed it out in the
>>>> virtio-networking upstream meeting.
>>>>
>>>>>> But probably it is better to address it on top, with another 
>>>>>> feature flag.
>>>>> I think if we want to changing the vq properties, there must be a
>>>>> mechanism to
>>>>> stop the queue then resume the queue.
>>>>>
>>>>> How about allow setting queue_enable = 0 to stop it and =1 to 
>>>>> resume and
>>>>> force it reinitialize?
>>>>>
>>>> Yes, I think that is better suited. But maybe this is better to be
>>>> added on top, so we maintain this series small.
>>> Hi Eugenio,
>>>
>>> I have a second thought while implementing above queue_enable = 0,
>>> it doesn't provide more advantages over queue_reset:
>>>
>>> 1) queue_reset can help to stop a queue and the vq properties can be
>>> reconfigured during queue_reset --> queue_enable.
>>>
>>> 2) once the driver sees SUSPEND presented by the device, it assume the
>>> device states and vq states are stable, at that point the driver can
>>> read reliable device configurations. So vq reset should be ignored
>>> once SUSPEND is present and if we implement queue stop, it should be
>>> ignored too when SUSPEND.
>>>
>> The relation between SUSPEND and ring_reset needs to be described in
>> this series, yes. This is a good start, but I'm not sure if this one
>> meets all the requirements for SW assisted live migration.
>>
>> We can always add new feature flags to define a different interaction
>> in the future, like for devices that can support the change of vq
>> attributes in the suspend. To not steal the merit, this idea was
>> proposed by Si-Wei in a recent virtio-networking meeting.
> If so, we even don't need a new feature bit. We can just allow
> resetting vqs after the device presenting SUSPEND.
For the single bit of feature interaction with queue_reset this looks 
fine, but queue_reset is perhaps not the only feature that needs to 
interact with SUSPEND. While on the other hand I suspect it's probably 
not easy to converge on everything all at once for the moment. Just to 
avoid the lure of hijacking this thread for other things, it'd be easier 
I feel to define a pristine SUSPEND method starting with the most 
restrictive mandates, describing every possible means to prohibiting 
*any* change to the config space for device in suspension. This not just 
keeps the (backward) compatibility on the table which is consistent with 
the assumption of various SUSPEND implementations available today, but 
would make it possible to customize different flavors of interactions 
guarded by different feature flag in the future. For instance, today 
queue_reset may mostly work the best on software device implementation 
where one can introduce a specific SUSPEND_RING_RESET_ALLOWED feature 
flag to unlock/override part of the restriction from the pristine 
SUSPEND feature when both are negotiated and used together. In future, 
if there's any need to revisit this part for e.g. hardware device 
implementation of queue_reset might not be able to meet certain desired 
performance (downtime) goal, then a new feature might have to be 
introduced to define another hardware-biased means of interaction with 
suspended device.

>
> The device presenting SUSPEND indicates that the device config space
> is stabilized at that moment, ready for the driver to fetch fields 
> data there.
>
> Then the driver is allowed to reset, re-config and re-enable the vqs.
Maybe not for this case, but for completeness I found a very relevant 
question is, as your patch defines SUSPEND in the context of live 
migration, how do you envision to resume/restart the device immediately 
in place on the source host (say migration is cancelled after all 
devices are suspended, or migration failed at the last minute for some 
reason)? Reset the device and start to recover everything from scratch? 
Or do queue_reset then queue_enable on every virtqueue while keeping the 
other device states (those already populated through ctrl vq) around? Or 
suppose right now we have a symmetric RESUME feature that keeps every 
device state including the queue state in place. Which option a hardware 
vendor would like to pick if user/customer would like to have the 
best/least downtime? Does the hardware's choice matter much for software 
device implementation?

As can be seen amongst these options, there's perhaps no single best 
solution between software and hardware devices, or even between 
different hardware vendors. So instead of ruling out possibility for 
future extension to flavor other implementations, be it hardware or 
software, I feel it's probably not the best thing for now to get SUSPEND 
hard wired to queue_reset or RESUME. Device reset is the base case that 
every device has to implement, that I feel might be the only failsafe 
method to get the device out of the suspension state with pristine SUSPEND.

>
> The only requirement is: The driver is responsible for maintain
> the integrity and validity of the config space fields, because
> the device is ready-only to the config space at that moment(SUSPEND-ed)
> and the driver should be responsible for its actions, perform proper
> synchronizations, e.g., re-read.
It looks fine, though as stated above, please leave it to a different 
feature flag with another patch to define the queue_reset interaction 
with SUSPEND.

Thanks,
-Siwei

>
> Does this work for you?
>
> Thanks
>>
>>> 3) the device should only accept resetting a queue when !SUSPEND and
>>> the driver can flush the queue buffers before resetting it to avoid
>>> losing buffers,
>>> and we will have tracker for in-flight descriptors later.
>>>
>>> Any thoughts?
>>>
>>> Thanks
>>>> Thanks!
>>>>
>>>>> Thanks
>>>>> Zhu Lingshan
>>>>>>>>> +
>>>>>>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but 
>>>>>>>>> VIRTIO_RING_F_PACKED is not,
>>>>>>>>> +the device MUST ignore any accesses against \field{Used State}.
>>>>>>>>> +
>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST 
>>>>>>>>> reset
>>>>>>>>> +the Virtqueue State of every virtqueue upon a reset.
>>>>>>>> Need to define the meaning of "reset" this is important for 
>>>>>>>> packed virtqueue.
>>>>>>> I will remove this as Stefan suggested.
>>>>>>>>> +
>>>>>>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been 
>>>>>>>>> negotiaged, when SUSPEND is set,
>>>>>>>>> +the device MUST record the Virtqueue State of every enabled 
>>>>>>>>> virtqueue
>>>>>>>>> +in \field{Available State} and \field{Used State} respectively,
>>>>>>>>> +and correspondingly restore the Virtqueue State of every 
>>>>>>>>> enabled virtqueue
>>>>>>>>> +from \field{Avaiable State} and \field{Used State} when 
>>>>>>>>> DRIVER_OK is set.
>>>>>>>> We can just let the device report those states in any case then we
>>>>>>>> don't need to care about those details, or did you see any 
>>>>>>>> blockers?
>>>>>>> Agree, I will add the definition of used_state of splitted vq in 
>>>>>>> the
>>>>>>> next version
>>>>>>>
>>>>>>> Thanks
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but 
>>>>>>>>> VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>>>>>>>>> +the device MUST record the available state of every enabled 
>>>>>>>>> virtqueue in \field{Available State},
>>>>>>>>> +and restore the available state of every enabled virtqueue 
>>>>>>>>> from \field{Avaiable State}
>>>>>>>>> +when DRIVER_OK is set.
>>>>>>>>> +
>>>>>>>>>      \input{admin.tex}
>>>>>>>>>
>>>>>>>>>      \chapter{General Initialization And Device 
>>>>>>>>> Operation}\label{sec:General Initialization And Device Operation}
>>>>>>>>> -- 
>>>>>>>>> 2.35.3
>>>>>>>>>
>>>> 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/
>>>>
>


---------------------------------------------------------------------
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: [virtio-comment] Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Zhu, Lingshan]

On 9/8/2023 2:23 PM, Si-Wei Liu wrote:
>
>
> On 9/7/2023 2:34 AM, Zhu, Lingshan wrote:
>>
>>
>> On 9/7/2023 4:09 PM, Eugenio Perez Martin wrote:
>>> On Tue, Sep 5, 2023 at 11:08 AM Zhu, Lingshan 
>>> <lingshan.zhu@intel.com> wrote:
>>>>
>>>>
>>>> On 8/21/2023 5:26 PM, Eugenio Perez Martin wrote:
>>>>> On Fri, Aug 18, 2023 at 11:44 AM Zhu, Lingshan 
>>>>> <lingshan.zhu@intel.com> wrote:
>>>>>>
>>>>>> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
>>>>>>> On Tue, Aug 15, 2023 at 1:30 PM Zhu, Lingshan 
>>>>>>> <lingshan.zhu@intel.com> wrote:
>>>>>>>> On 8/15/2023 8:34 AM, Jason Wang wrote:
>>>>>>>>> On Mon, Aug 14, 2023 at 7:29 PM Zhu Lingshan 
>>>>>>>>> <lingshan.zhu@intel.com> wrote:
>>>>>>>>>> This commit specifies the constraints of the virtqueue state,
>>>>>>>>>> and the actions should be taken by the device when SUSPEND
>>>>>>>>>> and DRIVER_OK is set
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>      content.tex | 31 +++++++++++++++++++++++++++++++
>>>>>>>>>>      1 file changed, 31 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>>>>> index 43bd5de..f6ac581 100644
>>>>>>>>>> --- a/content.tex
>>>>>>>>>> +++ b/content.tex
>>>>>>>>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
>>>>>>>>>>
>>>>>>>>>>      See also \ref{sec:Packed Virtqueues / Driver and Device 
>>>>>>>>>> Ring Wrap Counters}.
>>>>>>>>>>
>>>>>>>>>> +\drivernormative{\subsection}{Virtqueue State}{Basic 
>>>>>>>>>> Facilities of a Virtio Device / Virtqueue State}
>>>>>>>>>> +
>>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST 
>>>>>>>>>> set SUSPEND in \field{device status}
>>>>>>>>>> +first before getting or setting Virtqueue State of any 
>>>>>>>>>> virtqueues.
>>>>>>>>> I don't get why this is a must. It could be useful for debugging.
>>>>>>>> To avoid race conditions with the device and make the device
>>>>>>>> implementation easier
>>>>>>>>>> +
>>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but 
>>>>>>>>>> VIRTIO_RING_F_PACKED not been negotiated,
>>>>>>>>> typo
>>>>>>>> yes
>>>>>>>>>> +the driver MUST NOT access \field{Used State} of any 
>>>>>>>>>> virtqueues, it should use the
>>>>>>>>>> +used index in the used ring.
>>>>>>>>>> +
>>>>>>>>>> +\devicenormative{\subsection}{Virtqueue State}{Basic 
>>>>>>>>>> Facilities of a Virtio Device / Virtqueue State}
>>>>>>>>>> +
>>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is 
>>>>>>>>>> not set in \field{device status},
>>>>>>>>>> +the device MUST ignore any accesses against Virtqueue State 
>>>>>>>>>> of any virtqueues.
>>>>>>>>> Btw, do we need to clarify the behavior of ring reset after 
>>>>>>>>> suspending?
>>>>>>>> I think once suspended, the device should ignore resetting a queue
>>>>>>> Actually shadow virtqueue could benefit from the ability to 
>>>>>>> change vq
>>>>>>> properties (addresses) while the device is suspended, and then just
>>>>>>> resume it. I've been told that ring reset is overkill for that.
>>>>>> If ring reset is overkill, is SUSPEND even more overkill?
>>>>> It depends on the cost of recreating the vq in the device I think. 
>>>>> But
>>>>> it has more to do with *what* is changed in the vq, as it seems some
>>>>> parameters (vq size) has more impact than others like vq address. The
>>>>> way to stop the device does not affect, but ring reset offers the
>>>>> possibility of change all of the parameters already.
>>>>>
>>>>> Adding Si-Wei and Dragos here, as they pointed it out in the
>>>>> virtio-networking upstream meeting.
>>>>>
>>>>>>> But probably it is better to address it on top, with another 
>>>>>>> feature flag.
>>>>>> I think if we want to changing the vq properties, there must be a
>>>>>> mechanism to
>>>>>> stop the queue then resume the queue.
>>>>>>
>>>>>> How about allow setting queue_enable = 0 to stop it and =1 to 
>>>>>> resume and
>>>>>> force it reinitialize?
>>>>>>
>>>>> Yes, I think that is better suited. But maybe this is better to be
>>>>> added on top, so we maintain this series small.
>>>> Hi Eugenio,
>>>>
>>>> I have a second thought while implementing above queue_enable = 0,
>>>> it doesn't provide more advantages over queue_reset:
>>>>
>>>> 1) queue_reset can help to stop a queue and the vq properties can be
>>>> reconfigured during queue_reset --> queue_enable.
>>>>
>>>> 2) once the driver sees SUSPEND presented by the device, it assume the
>>>> device states and vq states are stable, at that point the driver can
>>>> read reliable device configurations. So vq reset should be ignored
>>>> once SUSPEND is present and if we implement queue stop, it should be
>>>> ignored too when SUSPEND.
>>>>
>>> The relation between SUSPEND and ring_reset needs to be described in
>>> this series, yes. This is a good start, but I'm not sure if this one
>>> meets all the requirements for SW assisted live migration.
>>>
>>> We can always add new feature flags to define a different interaction
>>> in the future, like for devices that can support the change of vq
>>> attributes in the suspend. To not steal the merit, this idea was
>>> proposed by Si-Wei in a recent virtio-networking meeting.
>> If so, we even don't need a new feature bit. We can just allow
>> resetting vqs after the device presenting SUSPEND.
> For the single bit of feature interaction with queue_reset this looks 
> fine, but queue_reset is perhaps not the only feature that needs to 
> interact with SUSPEND. While on the other hand I suspect it's probably 
> not easy to converge on everything all at once for the moment. Just to 
> avoid the lure of hijacking this thread for other things, it'd be 
> easier I feel to define a pristine SUSPEND method starting with the 
> most restrictive mandates, describing every possible means to 
> prohibiting *any* change to the config space for device in suspension. 
> This not just keeps the (backward) compatibility on the table which is 
> consistent with the assumption of various SUSPEND implementations 
> available today, but would make it possible to customize different 
> flavors of interactions guarded by different feature flag in the 
> future. For instance, today queue_reset may mostly work the best on 
> software device implementation where one can introduce a specific 
> SUSPEND_RING_RESET_ALLOWED feature flag to unlock/override part of the 
> restriction from the pristine SUSPEND feature when both are negotiated 
> and used together. In future, if there's any need to revisit this part 
> for e.g. hardware device implementation of queue_reset might not be 
> able to meet certain desired performance (downtime) goal, then a new 
> feature might have to be introduced to define another hardware-biased 
> means of interaction with suspended device.
Hi Siwei

OK, I got it, there can be a new feature bit for resetting a queue after 
SUSPEND, and other interactions can follow the same way, more flexible.
>
>>
>> The device presenting SUSPEND indicates that the device config space
>> is stabilized at that moment, ready for the driver to fetch fields 
>> data there.
>>
>> Then the driver is allowed to reset, re-config and re-enable the vqs.
> Maybe not for this case, but for completeness I found a very relevant 
> question is, as your patch defines SUSPEND in the context of live 
> migration, how do you envision to resume/restart the device 
> immediately in place on the source host (say migration is cancelled 
> after all devices are suspended, or migration failed at the last 
> minute for some reason)? Reset the device and start to recover 
> everything from scratch? Or do queue_reset then queue_enable on every 
> virtqueue while keeping the other device states (those already 
> populated through ctrl vq) around? Or suppose right now we have a 
> symmetric RESUME feature that keeps every device state including the 
> queue state in place. Which option a hardware vendor would like to 
> pick if user/customer would like to have the best/least downtime? Does 
> the hardware's choice matter much for software device implementation?
>
> As can be seen amongst these options, there's perhaps no single best 
> solution between software and hardware devices, or even between 
> different hardware vendors. So instead of ruling out possibility for 
> future extension to flavor other implementations, be it hardware or 
> software, I feel it's probably not the best thing for now to get 
> SUSPEND hard wired to queue_reset or RESUME. Device reset is the base 
> case that every device has to implement, that I feel might be the only 
> failsafe method to get the device out of the suspension state with 
> pristine SUSPEND.
In case of failed or cancelled Live Migration, the driver can reset the 
re-config the device to resume it for sure.

In this series, we also say:
If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD 
clear SUSPEND and resumes operation upon DRIVER_OK.

>
>>
>> The only requirement is: The driver is responsible for maintain
>> the integrity and validity of the config space fields, because
>> the device is ready-only to the config space at that moment(SUSPEND-ed)
>> and the driver should be responsible for its actions, perform proper
>> synchronizations, e.g., re-read.
> It looks fine, though as stated above, please leave it to a different 
> feature flag with another patch to define the queue_reset interaction 
> with SUSPEND.
Sure, we will introduce a new feature bit for resetting vq.

Thanks for your advice
Zhu Lingshan
>
> Thanks,
> -Siwei
>
>>
>> Does this work for you?
>>
>> Thanks
>>>
>>>> 3) the device should only accept resetting a queue when !SUSPEND and
>>>> the driver can flush the queue buffers before resetting it to avoid
>>>> losing buffers,
>>>> and we will have tracker for in-flight descriptors later.
>>>>
>>>> Any thoughts?
>>>>
>>>> Thanks
>>>>> Thanks!
>>>>>
>>>>>> Thanks
>>>>>> Zhu Lingshan
>>>>>>>>>> +
>>>>>>>>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but 
>>>>>>>>>> VIRTIO_RING_F_PACKED is not,
>>>>>>>>>> +the device MUST ignore any accesses against \field{Used State}.
>>>>>>>>>> +
>>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST 
>>>>>>>>>> reset
>>>>>>>>>> +the Virtqueue State of every virtqueue upon a reset.
>>>>>>>>> Need to define the meaning of "reset" this is important for 
>>>>>>>>> packed virtqueue.
>>>>>>>> I will remove this as Stefan suggested.
>>>>>>>>>> +
>>>>>>>>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been 
>>>>>>>>>> negotiaged, when SUSPEND is set,
>>>>>>>>>> +the device MUST record the Virtqueue State of every enabled 
>>>>>>>>>> virtqueue
>>>>>>>>>> +in \field{Available State} and \field{Used State} respectively,
>>>>>>>>>> +and correspondingly restore the Virtqueue State of every 
>>>>>>>>>> enabled virtqueue
>>>>>>>>>> +from \field{Avaiable State} and \field{Used State} when 
>>>>>>>>>> DRIVER_OK is set.
>>>>>>>>> We can just let the device report those states in any case 
>>>>>>>>> then we
>>>>>>>>> don't need to care about those details, or did you see any 
>>>>>>>>> blockers?
>>>>>>>> Agree, I will add the definition of used_state of splitted vq 
>>>>>>>> in the
>>>>>>>> next version
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but 
>>>>>>>>>> VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
>>>>>>>>>> +the device MUST record the available state of every enabled 
>>>>>>>>>> virtqueue in \field{Available State},
>>>>>>>>>> +and restore the available state of every enabled virtqueue 
>>>>>>>>>> from \field{Avaiable State}
>>>>>>>>>> +when DRIVER_OK is set.
>>>>>>>>>> +
>>>>>>>>>>      \input{admin.tex}
>>>>>>>>>>
>>>>>>>>>>      \chapter{General Initialization And Device 
>>>>>>>>>> Operation}\label{sec:General Initialization And Device 
>>>>>>>>>> Operation}
>>>>>>>>>> -- 
>>>>>>>>>> 2.35.3
>>>>>>>>>>
>>>>> 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/
>>>>>
>>
>


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