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

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

On 6/12/2024 7:26 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Wednesday, June 12, 2024 4:00 PM
>>
>> On 6/12/2024 6:07 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Sent: Wednesday, June 12, 2024 2:50 PM
>>>>
>>>> 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.
>>> Device migration and debug channel [1] plans to access the device from the
>> hypervisor.
>>> I don't see any disadvantage of separate control and status register.
>>> Do you see any?
>>>
>>> [1]
>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
>>> .kernel.org%2Fvirtio-comment%2F20240601145042.2074739-1-
>> parav%40nvidia
>> .com%2FT%2F%23m14b80fd996e8d2522723967d8af844b70e131a90&dat
>> a=05%7C02%7
>> Cparav%40nvidia.com%7C06c09157b2d74af8b7f608dc8acaab0a%7C43083
>> d1572734
>> 0c1b7db39efd9ccc17a%7C0%7C0%7C638537850271999791%7CUnknown
>> %7CTWFpbGZsb
>> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
>> 0%3D%
>> 7C0%7C%7C%7C&sdata=e%2BBDJMpuFzmj47CKqnWU1jXcRZUXATmqhhMZ
>> 7WC8zj8%3D&re
>>> served=0
>> is the admin vq governed by the driver? There should be only one owner of
>> the device, or you need locks.
>>
> No locks are needed. Hypervisor is accessing the admin vq on the PF.
> VF is accessing the suspend ctrl and status register of its own from its own MMIO space.
> If for some reason, this registers is trapped and accessed in the hypervisor too, it is upto the hypervisor on lock/no-lock etc.
Even access device status from PF through admin vq, it is still trying to RW the unique vf->dev_status,
so there are too individuals accessing the same copy of resource, there need a lock.
>
>> I am not sure we want to repeat the discussion on the admin vq for live
>> migration last year, you know it breaks nested.
> I am showing you the use case where ctrl+status is useful without any negatives.
> You are diverging the discussion to a point of no return.
> I wish you can be more cooperative by seeing larger picture. :)
>
> You didn't answer my question, do you see any disadvantage of ctrl+status register?
> If not, lets converge and progress as it enables suspend functionality + device migration both using non-ambiguous register pair.
This is not about SUSPEND or ctrl + status. If we take the transition period into consideration, we need a total solution for all status transitions.
For example DRIVER_OK and FEATURES_OK, or the device setting NEEDS_RESET,
they can also take longer time than expect to take effects, depends on the device implementation.

Maybe we need a new bit in the device stauts: TRANSITION.