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

Parav Pandit
2024-06-12T10:07:39Z

> 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