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

Parav Pandit
2024-06-15T04:33:35Z

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Thursday, June 13, 2024 3:29 PM
>
> 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.
>
Yes, this will be good idea.
The TRANSITION bit which must turn very quickly to 1 on an MMIO write requires very special circuitry.
It needs to turn 1 just before the next read arrives from the guest driver.
This is roughly 10nsec of time (write followed by a read).

To simplify this aspect, a better approach can be,
1. device_status to have SUSPENDED bit, which starts with 0.
2. a new ctrl_register is added. When cmd is written there, its status reflects in device_status.

3. So even if the device takes 30nsec or 30usec to respond, things don’t break.

For example,
a. driver writes device_ctrl = 0x1 (cmd=suspend)
b. driver reads device_status = (suspended=0) -> not yet suspended
c. driver loops on #b few more nsecs.
d. finally device turns it suspended. So #b returns supended=1.

And we can extend it more to better restructure the other operations too such as features_ok, driver_ok etc.
And can also make the current "reset" from SHOULD to MUST.

Its one time price to pay for the extra register but it is far easier to implement for the devices (and works fine for sw backend too).