From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH V2 2/2] virtio: introduce STOP status bit References: <20210706043334.36359-1-jasowang@redhat.com> <20210706043334.36359-3-jasowang@redhat.com> From: Jason Wang Message-ID: Date: Mon, 12 Jul 2021 12:06:33 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US To: Eugenio Perez Martin Cc: Michael Tsirkin , virtio-comment@lists.oasis-open.org, Virtio-Dev , Stefan Hajnoczi , Max Gurtovoy , Cornelia Huck , Oren Duer , Shahaf Shuler , Parav Pandit , Bodong Wang , Alexander Mikheev , Halil Pasic List-ID: 在 2021/7/10 上午1:35, Eugenio Perez Martin 写道: > On Tue, Jul 6, 2021 at 6:34 AM Jason Wang wrote: >> This patch introduces a new status bit STOP. This will be >> used by the driver to stop the device in order to safely fetch the >> device state (virtqueue state) from the device. >> >> This is a must for supporting migration. >> >> Signed-off-by: Jason Wang >> --- >> content.tex | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 66 insertions(+), 3 deletions(-) >> >> diff --git a/content.tex b/content.tex >> index 8877b6f..284ead0 100644 >> --- a/content.tex >> +++ b/content.tex >> @@ -47,6 +47,10 @@ \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[STOP (32)] When VIRTIO_F_STOP is negotiated, indicates that the >> + device has been stopped by the driver. This status bit is different >> + from the reset since the device state is preserved. >> + >> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced >> an error from which it can't recover. >> \end{description} >> @@ -70,12 +74,38 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev >> recover by issuing a reset. >> \end{note} >> >> +If VIRTIO_F_STOP has been negotiated, the driver MUST NOT set STOP if >> +DRIVER_OK is not set. >> + >> +If VIRTIO_F_STOP has been negotiated, to stop a device, after setting >> +STOP, the driver MUST re-read the device status to ensure the STOP bit >> +is set to synchronize with the device. >> + >> \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field} >> The device MUST initialize \field{device status} to 0 upon reset. >> >> The device MUST NOT consume buffers or send any used buffer >> notifications to the driver before DRIVER_OK. >> >> +If VIRTIO_F_STOP has not been negotiated, the device MUST ignore the >> +write of STOP. >> + >> +If VIRTIO_F_STOP has been negotiated, after the driver writes STOP, >> +the device MUST finish any pending operations like in flight requests >> +or have its device specific way for driver to save the pending >> +operations like in flight requests before setting the STOP status bit. >> + >> +If VIRTIO_F_STOP has been negotiated, the device MUST NOT consume >> +buffers or send any used buffer notifications to the driver after >> +STOP. The device MUST keep the configuration space unchanged and MUST >> +NOT send configuration space change notification to the driver after >> +STOP. >> + >> +If VIRTIO_F_STOP has been negotiated, after STOP, the device MUST >> +preserve all the necessary state (the virtqueue states with the >> +possible device specific states) that is required for restoring in the >> +future. >> + >> \label{sec:Basic Facilities of a Virtio Device / Device Status Field / DEVICENEEDSRESET}The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state >> 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. >> @@ -474,8 +504,8 @@ \subsection{\field{Used State} Field} >> \begin{itemize} >> \item A driver MUST NOT set the virtqueue state before setting the >> FEATURE_OK status bit. >> -\item A driver MUST NOT set the virtqueue state after setting the >> - DRIVER_OK status bit. >> +\item A driver MUST NOT set the virtqueue state if DRIVER_OK status >> + bit is set without STOP status bit. >> \end{itemize} >> >> \devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State} >> @@ -488,7 +518,7 @@ \subsection{\field{Used State} Field} >> \item A device SHOULD ignore the write to the virtqueue state if the >> FEATURE_OK status bit is not set. >> \item A device SHOULD ignore the write to the virtqueue state if the >> -DRIVER_OK status bit is set. >> +DRIVER_OK status bit is set without STOP status bit. >> \end{itemize} >> >> If VIRTIO_F_RING_STATE has been negotiated, a device MAY has its >> @@ -623,6 +653,36 @@ \section{Device Cleanup}\label{sec:General Initialization And Device Operation / >> >> Thus a driver MUST ensure a virtqueue isn't live (by device reset) before removing exposed buffers. >> >> +\section{Virtqueue State Saving} >> + >> +If both VIRTIO_F_RING_STATE and VIRTIO_F_STOP have been negotiated. A >> +driver MAY save the internal virtqueue state. >> + >> +\drivernormative{\subsection}{Virtqueue State Saving}{General Initialization And Device Operation / Virtqueue State Saving} >> + >> +Assuming the device is 'live'. The driver MUST follow this sequence to >> +stop the device and save the virtqueue state: >> + >> +\begin{enumerate} >> +\item Set the STOP status bit. >> + >> +\item Re-read \field{device status} until the STOP bit is set to >> + synchronize with the device. >> + >> +\item Read \field{available state} and save it. >> + >> +\item Read \field{used state} and save it. >> + >> +\item Read device specific virtqueue states if needed. >> + >> +\item Reset the device. > Maybe I'm being too nitpicky here, Nope, any comments are welcomed. > but the next user of the device > must reset anyway to start using it (as "driver initialization" > states). Maybe it's better to remove this "reset the device" from the > enumeration and ... [1] That's fine since the first step for "driver initialization" is also a reset. Let me have more think about this. > >> +\end{enumerate} >> + >> +The driver MAY perform device specific steps to save device specific sate. > s/sate/state/ Will fix. > >> + >> +The driver MAY 'resume' the device by redoing the device initialization >> +with the saved virtqueue state. See ref\{sec:General Initialization and Device Operation / Device Initialization} > [1] just let this aclaration? If we let the reset in the enumeration, > it seems that the reset step is a MUST to save the virtqueue state. Yes. Thanks > >> + >> \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options} >> >> Virtio can use various different buses, thus the standard is split >> @@ -6713,6 +6773,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} >> can set and get the device internal virtqueue state. >> See \ref{sec:Virtqueues / Virtqueue State}~\nameref{sec:Virtqueues / Virtqueue State}. >> >> + \item[VIRTIO_F_STOP(41)] This feature indicates that the driver can >> + stop 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.25.1 >>