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

Parav Pandit
2024-06-11T10:37:11Z

> 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.