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

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

On 6/11/2024 6:37 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Tuesday, June 11, 2024 3:42 PM
>>
>> On 6/11/2024 5:43 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Sent: Tuesday, June 11, 2024 3:03 PM
>>>>
>>>> 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.
>>> I am not talking about VQ reset at all.
>>> I gave an example of how the VQ reset register polarity works (with was
>> broken like how the proposed suspend bit is broken).
>>> Suspend register needs to work the way the reset register works.
>>> Can you please go through my exact text I replied in the previous email?
>>>
>>> Basically, the bug is:
>>> When driver has initiated the suspend (writing 1), the device continue to
>> respond 0, while suspend is going on.
>>> At this point, just by looking at the device_status register one cannot figure
>> out what is going on in the device.
>>> Is driver initiated suspend is ongoing, or it was never started.
>>> It is ambiguous.
>> From the device perspective, there is only one user: the driver, and the driver
>> sets or clears SUSPEND, it knows the status of the device.
> It does not work elegantly when the hypervisor or other system is looking dealing with this register.
>
> In current proposal, anyone else than the driver looking at the device_status, it is ambiguous.
>
> For example,
> suspend_bit: 0x1 suspended.
> 	     0x0 not suspended or suspend is ongoing. (ambiguous for hypervisor debugging this device)
>
> For device migration, it requires yet another side bit in the device parts.
>
> An elegant and non-ambiguous interface can be,
>
> ctrl_register =	0x0 (a usual default)
> 		0x1, suspend the device
> 		0x2, resume the device.
> status_register =	0x0, (still) running.
> 			0x1, suspended.
>
> With this driver and hypervisor (both) has very clear view of what is going on and what is the status.
> No more ambiguity for anyone.
>
> Please consider pros and cons of both approaches.
The driver "owns" the device, other components access the device configurations through the driver, I am not sure there are other "owners" than the driver.

Thanks
Zhu Lingshan