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

Zhu Lingshan
2024-06-11T10:12:25Z

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.
Differs from vq reset, in this patch there is a section describing how the device should behave suspending. So once the driver sets SUSPEND,
the device should perform the required actions and suspend itself, during this process, SUSPEND bit is still 0, and the drivers know that, like
how we handle FEATURES_OK. Once the device suspended itself, it present SUSPEND == 1 in the device status.

1 bit controls whether SUSPEND-ed.
>
> Hence, it needs to work like how the queue_reset works.
> i.e. if suspend is ongoing, one should be able to read device_status without involving driver and understand that it is ongoing.
there are other users than the driver?
>
>>>> This patch focus on the basic facility virito suspending, not
>>>> transport or device types, this is to make this chageset focused and small.
>>>>
>>> If suspend bit is part of the device_status, it is already covered by the PCI
>> transport.
>>> So it is fine, but its polarity is an issue, that we need to fix.
>> Oh, I see your confusion.
>> All transport have their own implementation of configuration space, this is not
>> PCI specific.
>> PCI config space != virtio configuration space. And this patch focuses on virtio
>> layer.
>>> i.e.
>>> One driver writes 1, it needs to stay 1, while the suspend is ongoing.
>>> When suspension is completed in the device, the device to turn it to zero.
>>>
>>> If the device_status register does not fullfil this, a new register is worthwhile
>> to add in this patch.
>> The driver writes SUSPEND, once the device is suspened, it should present
>> SUSPEND in its device status.
> Yes, I understood what you are saying.
> It is ambiguous like how I explained above.
>
> The suspend bit needs to work like queue_reset register. Please go through the queue reset specification text.
>
> I explained this in context of VQ reset, at [1].
> Suspend bit needs to avoid similar ambiguity.
>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/139
replied above

Thanks
Zhu Lingshan