Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan <lingshan.zhu@amd.com>]

On 7/4/2024 4:55 PM, Michael S. Tsirkin wrote:

> On Thu, Jul 04, 2024 at 05:39:32PM +0900, David Stevens wrote:
>> On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
>>>> This commit allows the driver to suspend the device by
>>>> introducing a new status bit SUSPEND in device_status.
>>>>
>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>> which indicating whether the device support SUSPEND.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>> Your signature would normally be last.
>>>
>>>
>>>> ---
>>>>  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>  1 file changed, 61 insertions(+), 10 deletions(-)
>>> At a high level, I think this is somewhat overspecified.
>>> Generally e.g. if driver is forbidden from accessing
>>> a field, then we do not also add specific requirements for
>>> devices - with no driver doing this, such functionality will
>>> remain untested and unused, and when we finally decide we need
>>> it it's not going to be there.
>>> Similarly for when device is not touching a field - no
>>> reason for driver not to touch it.
>>>
>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 0a62dce..8c974d3 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -36,19 +36,22 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>      this bit.  For example, under Linux, drivers can be loadable modules.
>>>>    \end{note}
>>>>
>>>> -\item[FAILED (128)] Indicates that something went wrong in the guest,
>>>> -  and it has given up on the device. This could be an internal
>>>> -  error, or the driver didn't like the device for some reason, or
>>>> -  even a fatal error during device operation.
>>>> +\item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>>>> +  drive the device.
>>>>
>>>>  \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
>>>>    features it understands, and feature negotiation is complete.
>>>>
>>>> -\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.
>>>> +
>>>> +\item[FAILED (128)] Indicates that something went wrong in the guest,
>>>> +  and it has given up on the device. This could be an internal
>>>> +  error, or the driver didn't like the device for some reason, or
>>>> +  even a fatal error during device operation.
>>>>  \end{description}
>>>>
>>>>  The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>> @@ -60,8 +63,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>  initialization sequence specified in
>>>>  \ref{sec:General Initialization And Device Operation / Device
>>>>  Initialization}.
>>>> -The driver MUST NOT clear a
>>>> -\field{device status} bit.  If the driver sets the FAILED bit,
>>>> +The driver MUST NOT clear a \field{device status} bit other than SUSPEND
>>>> +except when setting \field{device status} to 0 as a transport-specific way to
>>>> +initiate a reset. If the driver sets the FAILED bit,
>>>>  the driver MUST later reset the device before attempting to re-initialize.
>>>>
>>>>  The driver SHOULD NOT rely on completion of operations of a
>>>> @@ -99,10 +103,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
>>>>  \begin{description}
>>>>  \item[0 to 23, and 50 to 127] Feature bits for the specific device type
>>>>
>>>> -\item[24 to 41] Feature bits reserved for extensions to the queue and
>>>> +\item[24 to 42] Feature bits reserved for extensions to the queue and
>>>>    feature negotiation mechanisms
>>>>
>>>> -\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
>>>> +\item[43 to 49, and 128 and above] Feature bits reserved for future extensions.
>>>>  \end{description}
>>>>
>>>>  \begin{note}
>>>> @@ -629,6 +633,49 @@ \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{Device Suspend}\label{sec:General Initialization And Device Operation / Device Suspend}
>>>> +
>>>> +When VIRTIO_F_SUSPEND is negotiated, the driver can set the
>>>> +SUSPEND bit in \field{device status} to suspend a device, and can
>>>> +clear the SUSPEND bit to resume a suspended device.
>>>> +
>>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>>> +
>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>>>> +
>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>>> +\begin{itemize}
>>>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
>>> If it's not set, then what? Device error, have to reset?
>>>
>>>
>>>> +\item The driver MUST NOT make any more buffers available to the device.
>>>> +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
>>> what are these "fields"? the area in memory where the vq lives? why is
>>> doing that a problem - you want device to be able to write something
>>> there?
>>>
>>>> +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
>>>> +if it is implemented in the Config Space.
>>> what is "the Config Space"? device status is never in
>>> a device configuration space, it is part of the common configuration.
>>> so what exactly are you forbidding here?
>> IIUC, referring to the common configuration here wouldn't make sense.
>> The common configuration is part of the PCI transport specification,
>> so in this part of the specification it should only be referred to via
>> the "transport specific" phrasing.
> Good point.  We can easily fix this for MMIO though.
>
>  
> MMIO virtio devices provide a set of memory mapped control
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>
> ->
>
> MMIO virtio devices provide memory mapped control
> including a set of common configuration
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
IMHO this may be a bit vague, PCI as a section about common configuration and a struct "struct virtio_pci_common_cfg "
listed all contents. But what does CCW/MMIO common configuration contains? Shall we add new sections for them?
>
>
>
> Less sure about CCW.
> Maybe:
>
> In addition to the basic channel commands, virtio-ccw defines a
> set of channel commands related to configuration and operation of
> virtio:
>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> ->
>
>
> In addition to the basic channel commands, virtio-ccw defines
> channel commands related to configuration and operation of
> virtio - a set of commands to access common configuration of
> the device:
>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> and additionally, two commands to access the device
> specification configuration space:
>
> \begin{lstlisting}
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> \end{lstlisting}
>
>
> And now we have common configuration defined for all transports.
> Cornelia, WDYT?
>
>
>> This wording appears to be taken
>> from Cornelia's feedback on v5. Specifically:
>>
>>>> +\item The driver MUST NOT access Device Configuration Space.
>>> ...except for the status field, if it is part of the config space?
>> I asked for clarification [1], because Cornelia's feedback seems to
>> contradict your feedback here and on the (unfortunately unarchived) v3
>> patch. What is the definitive statement agreed upon by both editors of
>> the virtio specification as to whether or not the device status field
>> is part of the device configuration space?
>>
>> [1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/
>>
>> -David
> I don't know what did Cornelia mean. Cornelia?
>