Re: [PATCH V2 2/2] virtio: introduce STOP status bit

[Jason Wang <jasowang@redhat.com>]

在 2021/7/6 下午8:50, Cornelia Huck 写道:
> On Tue, Jul 06 2021, Jason Wang <jasowang@redhat.com> 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 <jasowang@redhat.com>
>> ---
>>   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.
> Is this more that the driver needs to re-read the status until STOP is
> set to make sure that the stop process has finished?


Yes.


>   If the device has
> offered the feature and the driver accepted it, I'd assume that the
> device will eventually finish with the procedure, or sets NEEDS_RESET if
> something goes wrong?


As stated below, the device must either:

1) finish all pending requests

or

2) provide a device specific way for the driver to save and restore 
pending requests

before setting STOP.

Otherwise the device can't offer this feature.

Using NEEDS_RESET seems more complicated than this.


>
>> +
>>   \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.
> Maybe use "If VIRTIO_F_STOP has been negotiated:" and put the next three
> points in a list? Might read a bit better.


That's fine.


>
>> +
>> +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.
> What happens if the driver writes STOP in when DRIVER_OK has not been
> set?


I think we need a device normative like:

If VIRTIO_F_STOP has been negotiated, the driver SHOULD ignore the STOP 
status bit if DRIVER_OK is not set.


>   Should the device set NEEDS_RESET, as suggested above? Same, if
> saving the states somehow goes wrong?


I try hard to avoid NEEDS_RESET, so the driver is required to only read 
the state during DRIVER_OK & STOP, and set the state during FEATURES_OK 
& !DRIVER_OK. This is described in the driver normative in patch 1 and 
below.


>
>> +
>>   \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.
> Is that device type specific, or something generic? The last patch
> suggests that it may vary by device type.


Both virtqueue state (avail/used state) and the STOP status bit is generic.

But the device is free to have its own specific:

1) extra virtqueue states (pending requests)
2) device states

And 2) is out of the scope of this series.

Thanks


>
>> +
>> +\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.
>> +\end{enumerate}
>> +
>> +The driver MAY perform device specific steps to save device specific sate.
>> +
>> +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}
>> +
>>   \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}