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

Zhu Lingshan
2024-06-12T10:30:24Z

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://lore.kernel.org/virtio-comment/20240601145042.2074739-1-parav@nvidia.com/T/#m14b80fd996e8d2522723967d8af844b70e131a90
is the admin vq governed by the driver? There should be only one owner of the device, or you need locks.

I am not sure we want to repeat the discussion on the admin vq for live migration last year, you know it breaks nested.