Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Apr 28, 2022 at 11:43:16AM +0800, Jason Wang wrote:
> 
> å 2022/4/28 11:24, Parav Pandit åé:
> > 
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, April 27, 2022 11:15 PM
> > > 
> > > On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > 
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > > > 
> > > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > > > 
> > > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> > > wrote:
> > > > > > > > example flow:
> > > > > > > > a) 0,0 -> device init time value
> > > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > > > clear behavior that is different from the device status?
> > > > > > > 
> > > > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > > > 0 to reset the virtqueue.
> > > > > > > 
> > > > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > > > the same, then we can simply reuse queue_enable.
> > > > > > > 
> > > > > > Yes, I know we can make this work using new feature bit + single
> > > > > queue_enable register.
> > > > > > I replied that in v0 to Michael.
> > > > > A bigger question in my eyes is that down the road we might want to
> > > > > be able to stop the ring without having it lose state.
> > > > > The natural interface for that seems to be writing 0 to queue enable.
> > > > Why queue_enable and not queue_reset?
> > > > 
> > > > to me this interface is unlikely performant and useful for such case.
> > > > When we want to pause/stop the VQ and query the state we need
> > > performant scheme, that can even work in a batch for all the VQs.
> > > > At that point programming 64 registers to pause/stop VQ without losing
> > > state and querying its indices etc won't be scalable with register interface.
> > > 
> > > The register interface to sync indices has already been implemented in real
> > > hardware for years.
> > > 
> > Sure. I meant to that when we want to pause a VQ and restart later use-case will require more plumbing than just enable/disable register.
> > And to do that, a register interface won't be performant/scalable. So for the wider use case this may not be the good choice.
> > 
> > And I explained the other reason that we lose the state information with this busy-wait register in the other reply to V2 and summary in the github issue too on Michael's request.
> 
> 
> I've replied to the thread.
> 
> 
> > 
> > > > I imagine a AQ (likely) or some other interface.
> > > So did the queue_enable registers, we need to write 1 to queue_enable for
> > > each virtqueue before DRIVER_OK.
> > > 
> > > Where to allow writing 0 to queue_enable is orthogonal to scalability.
> > Sure, lets sync after you get chance to go through my other reply to Michael about why with single busy-wait register we lose the state.
> > And hence, queue_reset register (with the fix) is better.
> 
> 
> Note that the MMIO transport allows writing 0 to queue_ready.
> 
> Thanks

... without the side effect of resetting the queue state.
I think ideally there should be text in MMIO stating it does not
support the reset feature at the moment, so devices should not expose
this feature and drivers should not acknowledge this if exposed by device
but should not fail if it's exposed (for future expansion).
This is an existing spec issue, should be a separate patch.

Regarding MMIO it is not exactly clear but I think the assumption
always was you don't disable queues after DRIVER_OK.

Just thinking aloud, I am wondering:


So in the proposal, the new queue reset register does two things:
- reset the internal queue state
- disable the queue


It seems possible to split the functionality: queue reset would just
disable counters, queue enable would disable queue. If we do
that:
1- We need to specify what happens if we reset counters while ring is enabled.
   I guess queue then has to go until ring is empty, then stop?
   Draining queue like this has value for e.g. snapshots.
   An alternative is to prohibit this transition.

   By the way current spec does not make it clear what happens if
   driver keeps adding buffers to the queue after writing to
   reset. It's probably a good idea to specify that device
   should not wait to drain the queue, it should just reset,
   but again this is an existing defect so maybe a separate patch.

2- what happens if queue is disabled but
   without reset, and queue size changes? We do not specify.
   Maybe that has to be prohibited.

I conclude that at some point, there is a chance we will want
to add VIRTIO_RING_F_DISABLE making queue enable writeable for
all transports after DRIVER_OK.
If we do, we might want the reset register behaviour to change
if VIRTIO_RING_F_DISABLE has been negotiated.
Parav from hardware point of view, would you say such a register where
behaviour changes depending on feature bit be too messy?


Overall, this makes me feel existing two register interface is
fine, reusing queue enable would make future expansion harder
while making queue enable writeable would be a new feature
and needs its own feature bit.


-- 
MST