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

[Zhu Lingshan <lingshan.zhu@amd.com>]

On 6/13/2024 1:58 PM, David Stevens wrote:
> On Wed, Jun 12, 2024 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Wed, Jun 12, 2024 at 11:26:39AM +0000, Parav Pandit wrote:
>>> No locks are needed. Hypervisor is accessing the admin vq on the PF.
>> I'm not even going to try and get through a jumble of this thread
>> heavily corrupted by outlook and whatnot, but from the glimpses I got
>> this discussion looks like just two people talking past each other.
>> Lingshan, Parav's not the only one bothered by how the device seemingly
>> has to block a transaction until it completes suspend. Parav, I feel how
>> to address this is up to the author to resolve really, leave it at that.
> IIUC, Prava's concern here is that with the current proposal, the
> device's state can't be determined unambiguously by reading the device
> status field. If the SUSPEND bit is 0, it's ambiguous as to whether
> the device is running normally or if it's in the process of
> suspending, and there is a corresponding issue for suspended vs
> resuming. The device and driver can know based on their internal
> information what state the device is in. However, some hypothetical
> admin tool in guest userspace wouldn't be able to tell simply by
> reading the values in the PCI BAR.
>
> Assuming that's actually a problem that we want to solve, then I think
> the most straightforward way to address this would be to use another
> bit in the status field as a SUSPEND_TRANSITION bit, with something
> like:
>
> "After the driver writes or clears the SUSPEND bit, the device MUST
> present the SUSPEND_TRANSITION bit until it completes the requested
> suspend or resume transition. If the device is unable to successfully
> complete the transition, it MUST present DEVICE_NEEDS_RESET."
>
> Doing that would give distinct values for each of the
> running/suspending/suspended/resuming states. That said, although the
> device status field doesn't have a defined width, both the PCI and
> channel transports assume that it's a single byte, so we'd be using
> the last remaining bits those transports have available.
Current spec does not handle status transitions well, not only for SUSPEND and resuming.
other status transitions may also take longer time than expect depends on how
the device is implemented, it can be sluggish if implemented by SW during high traffic,
even when handling FEATURES_OK or present NEEDS_RESET.

This patch wants to copy the mechanism of handling FEATURES_OK, but if we want to
resolve the problem of device status transition, I agree to add a new status bit: TRANSITION.

The device should present TRANSITION until it complete the requested status transition.
Combining the knowledge of the device, the driver can tell the exact device status.

Any thoughts?

Thanks
Zhu Lingshan
>
> -David