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

Jason Wang
2024-06-17T02:22:36Z

On Sat, Jun 15, 2024 at 12:33 PM Parav Pandit <parav@nvidia.com> wrote:
>
> > 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.

Is there any real difference? Currently, transitions could be spotted
by the driver easily: read after write doesn't match.

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

Well, I don't know why this needs to be addressed in this series as
it's not specific to suspend.

Thanks

>
>
>
>