Re: [virtio-dev] queue_reset register polarity to improve

[Stefan Hajnoczi <stefanha@redhat.com>]

On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote:
> On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit <parav@nvidia.com> wrote:
> > Hi,
> >
> > A recently defined queue_reset register has a little weird definition that we should improve.
> > When driver initiate queue reset, it writes queue_reset = 1.
> > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0.
> > Once queue reset is completed it is expected to return queue_reset = 1.
> > (Polarity changed twice to same value as what was driver set). See more below.
> >
> > So state wise,
> > # q_enable, q_reset represents :
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled and working
> > c) 1,1 -> vq is enabled, driver initiated reset
> > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b )
> > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above )
> >
> > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later).
> >
> > q_enable, q_reset
> > A) 0, 0 -> default, device init time
> > B) 1, 0 -> driver has enabled vq
> > C) 1, 1 -> driver started q reset
> > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq (communicating that its working on resetting, consistent with #C)
> > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A)
> 
> Seems to me to be two different designs, I don't see any particular value in the
> conflict mentioned here, and under what circumstances would it cause any
> trouble?
> 
> The design here is similar to many scenarios, such as device reset.
> 
> So if you want to change to your design, I want to know if there are other
> reasons.

A benefit of Parav's suggested definition is that reading the value of
q_enable and q_reset from the device tells you the state.

The current definition relies on the driver maintaining internal state
(although this could just be assumptions in the code rather than
variables stored in memory) because it cannot determine the state by
reading q_enable and q_reset.

Introspection may not be important for regular drivers because they know
which previous operations they performed, but in some cases like driver
crash recovery or live migration introspection can be useful. That said,
they can reset the entire device to return to a known state, if
necessary.

Stefan

Attachment: signature.asc
Description: PGP signature