Re: [PATCH V7 v7] virtio: introduce SUSPEND bit in device status

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

On 8/13/2024 4:01 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 01, 2024 at 07:35:16PM +0800, Zhu Lingshan wrote:
>> +\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.
> Actually, it has no effect before DRIVER_OK, no? I would forbid that
> then.
There can be two cases:
1) debugging the device before DRIVER_OK
2) VM migration right after boot, and before DRIVER_OK. So it needs to migrate the device before DRIVER_OK
>
>> +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.
>
> This is still vague, I commented on this several times.
> I think what you mean is that until it reads status with
> SUSPEND as 1, it does not consider SUSPEND set.
>
>
> But, what happens if driver clears SUSPEND before it reads it as set?
> It would seem that should cancel suspend (which is a useful thing to
> support)? But this creates a problem as it breaks read/modify/write that
> some hypervisors assumed to be safe. I guess we need an extra
> SUSPEND_IN_PROGRESS bit then? A little too much for status, at this
> stage - maybe we need an extra register for this.
if the driver reads SUSPEND bit == 0, with the knowledge of its own operations,
it knows the device is either in a) normal operation 2). suspending in progress

Cancelling a SUSPEND right after setting SUSPEND is actually a RESUME.
The ideal case is the driver waits until SUSPEND == 1, then clear it.

But there are various drivers, not all drivers follow our expectation,
so there can be a corner case that how the driver know whether
the device is resumed after clearing the SUSPEND bit or not finish suspending.

I think adding a new resister can surely fix this issue, however as you
said, add a new status bit is overkill and where to place this new register
is a new question.

So, to make things easier, how about suspend & resume the device sequentially:

If the driver has suspended the device by setting the SUSPEND bit to 1, it MUST NOT
clear the SUSPEND bit before the device presenting the SUSPEND bit set as 1.
>
>
>> +\item The driver MUST NOT make any more buffers available to the device.
>> +\item The driver MUST NOT access any virtqueues or send notifications for any virtqueues.
>> +\item The driver MUST NOT access Device Configuration Space.
>> +\end{itemize}
>> +
>> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>> +
>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>> +
>> +The device MUST ignore all access to its Configuration Space while
>> +suspended, except for \field{device status} if it is part of the Configuration Space.
>> +
>> +A device MUST NOT send any notifications for any virtqeuues,
>> +access any virtqueues, or modify any fields in
>> +its Configuration Space while suspended.
>> +
>> +If changes occur in the Configuration Space while the SUSPEND bit is set,
>> +the device MUST NOT send any configuration change notifications.
>> +Instead, the device MUST send the notification after the SUSPEND bit has been cleared.
>> +
>> +When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend.
>> +
>> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
>> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
>> +
>> +When the driver sets SUSPEND,
>> +the device SHOULD perform the following actions before presenting that the SUSPEND bit is set to 1 in the \field{device status}:
> what does "before presenting" mean? does it return SUSPEND as 0
> after driver wrote 1 there and before it completed these
> actions?
Yes, it should return 0 if the suspending  operation is still in progress,
It should not change SUSPEND bit value before finish.
Do you suggest we add:
The device MUST present SUSPEND bit set to 1 in \field{device status} once it has been suspended

It says: presenting that the SUSPEND bit is set to 1.
Not a native speaker, but this looks clear to me.
>
>> +
>> +\begin{itemize}
>> +\item Stop processing more buffers of any virtqueues
>> +\item Wait until all buffers that are being processed have been used.
>> +\item Send used buffer notifications to the driver.
>> +\end{itemize}
>> +
>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>  
>>  Virtio can use various different buses, thus the standard is split
>> @@ -872,6 +923,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(42)] This feature indicates that the driver can
>> +   trigger suspending the device via the SUSPEND flag
>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>> +
>>  \end{description}
>>  
>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> -- 
>> 2.45.2