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

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

On 6/11/2024 4:48 PM, Parav Pandit wrote:
>
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Tuesday, June 11, 2024 1:57 PM
>>
>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
>>> Hi Lingshan, David,
>>>
>>>> From: Cornelia Huck <cohuck@redhat.com>
>>>> Sent: Monday, June 10, 2024 9:34 PM
>>>>
>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>>> ---
>>>>>  content.tex | 69
>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>>> Can you please add a changelog? Especially as some of the previous
>>>> discussion has been lost due to the broken old mailing lists...
>>>>
>>>> (...)
>>>>
>>>>> +\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 not, the device does not support the SUSPEND feature.
>>>> That sentence is a bit weird: I'd expect the device to not offer
>>>> VIRTIO_F_SUSPEND in the first place in that case... could this rather
>>>> happen if the device is not able to handle the request at a specific point in
>> time?
>>>>> +\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
>>>>> +notify
>>>> any virtqueues.
>>>>
>>>> "send notifications for any virtqueues"?
>>>>
>>>>> +\item The driver MUST NOT access Device Configuration Space.
>>>> ...except for the status field, if it is part of the config 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, access any virtqueues, or
>>>>> +modify any fields in its configuration space while suspended.
>>>>> +
>>>>> +If SUSPEND is set in \field{device status}, when the driver clears
>>>>> +SUSPEND,
>>>> "subsequently 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
>>>> SUSPEND bit in the \field{device status}:
>>>>> +
>>>>> +\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}
>>>> So, is there any opportunity for the device to fail setting SUSPEND?
>>>> I mean, if the driver is supposed to look whether it sticks, there
>>>> should be conditions for when the device might clear it again...
>>>>
>>> Additionally, a suspend operation usually involves saving things to a slow
>> memory (or media).
>>> This is because the device implementation wouldn’t know when exactly the
>> device will be resumed.
>>> Few examples, are:
>>> a. A gpu device with 128MB of video RAM when suspended, QEMU needs
>> to store this into a (for example) rotating hard disk as 1msec IO latency.
>>> b. a NIC may need to store its RSS, queues, flow filters configuration for
>> several tens of KBs to some slow memory.
>>> c. A block device may prefer to complete some IOs to a threshold level
>> instead of maintaining large list of outstanding IOs in some suspended
>> memory.
>>> d. May be more in future.
>>>
>>> Additionally, suspend operation needs to be synchronized with certain data
>> path hardware engines to suspend DMA operations (read and write both)
>> which are inflight.
>>> (without causing DMA errors). Same would apply to the sw backend
>> implementations too.
>>> Therefore, the suspend operation that is initiated by the driver, should get
>> the acknowledgement back from the device that it has been suspend.
>>> Some of the good examples if you prefer to follow, a driver<->device
>> interface needs a suspend register which should behave like below queue
>> reset register.
>>> Spec snippet:
>>> "The device MUST reset the queue when 1 is written to queue_reset. The
>>> device MUST continue to present
>>> 1 in queue_reset as long as the queue reset is ongoing. The device
>>> MUST present 0 in both queue_reset and queue_enable when queue reset
>> has completed."
>>> At minimum, we need, something implementable like,
>>>
>>> The device MUST suspend the device when 1 is written to the
>> device_suspend_ctlr. The device MUST continue to present 1 in
>> device_suspend_ctrl as the suspend operation is ongoing in the device.
>>> The device MUST present 0 in the device_suspend_ctlr register when
>> device has completely suspended the device.
>>> The device MUST resume the device when 2 is written to the
>> device_suspend_ctrl. The device MUST continue to present 2 in the
>> device_suspend_ctrl as the resume operation may not have yet completed.
>>> The device MUST present 0 in the device_suspend_ctrl register when the
>> device has completely resumed the device.
>>> At this point, the driver may resume notifying the device and accessing the
>> configuration space.
>>> Can you please enhance this part?
>> Hi Parav
>>
>> Yes they are necessary contents and we will address them in the following
>> patches.
>>
> The register polarity is critical.
> In your reply to Cornelia, you descried that suspend bit cleared in the device after driver sets it (until the device is suspended).
> This is ambiguous behavior of the register. Queue_reset register was like that before it was fixed in 1.2.
once suspended, the driver should not access the configuration space except for the device status, so it should not reset any vqs through
configuration space.
>
>> This patch focus on the basic facility virito suspending, not transport or
>> device types, this is to make this chageset focused and small.
>>
> If suspend bit is part of the device_status, it is already covered by the PCI transport.
> So it is fine, but its polarity is an issue, that we need to fix.
Oh, I see your confusion.
All transport have their own implementation of configuration space, this is not PCI specific.
PCI config space != virtio configuration space. And this patch focuses on virtio layer.
>
> i.e.
> One driver writes 1, it needs to stay 1, while the suspend is ongoing.
> When suspension is completed in the device, the device to turn it to zero.
>  
> If the device_status register does not fullfil this, a new register is worthwhile to add in this patch.
The driver writes SUSPEND, once the device is suspened, it should present SUSPEND in its device status.

Thanks
Zhu Lingshan
>