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

Parav Pandit
2024-06-17T03:00:59Z

> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, June 17, 2024 7:52 AM
>
> 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.
>
Yes, the real difference is: simplified device implementation.
The device does not have the time constraint to reply (transition=1) to the incoming read in next 10nsec.