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

Parav Pandit
2024-06-12T11:26:43Z

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

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