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 02:10:46PM +0800, Jason Wang wrote:
> On Thu, Apr 28, 2022 at 12:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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.
> 
> It's unclear in the spec that whether or not we had such side effects.

...and that makes it pretty useless without a new feature bit

> > I think ideally there should be text in MMIO stating it does not
> > support the reset feature at the moment,
> 
> So do you mean the state is held after writing 0 to queue_ready?

I said nothing about QueueReady. I just said that we did not
add the reset register to MMIO yet, MMIO drivers should be
careful not to negotiate the reset feature bit until we
do and they know the reset register offset.

> What
> happen if driver do
> 
> write 0 to queue_enable,
> avail_idx = 0,
> write 1 to queue_enable,
> 
> ?
> 
> > 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).
> 
> I'm not sure I understand this, but it looks too late to do that? (We
> don't have a feature for this).
> 
> Or we can simply forbid writing 0 to queue_ready until we refine this
> behaviour with a feature.
> 
> > 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
> 
> Does the reset also mean stop?

After a queue has been reset by the driver, the device MUST NOT execute
any requests from that virtqueue, or notify the driver for it.

so apparently yes


> > - disable the queue
> 
> What's the benefit of separating the functions here?

I am not separating anything *here*, just listing what happens.
Below I list some ideas for why disable without reset might be useful.

> >
> >
> > It seems possible to split the functionality: queue reset would just
> > disable counters, queue enable would disable queue.
> 
> This requires hardware to synchronize the queue_reset with datapath
> internally which looks a little bit complicated.

this is already the case.

> > 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.
> 
> Does this mean we only support queue_reset after disabling the queue?
> If yes, this basically go back to using a single queue_enable
> register.

I do not think queue enable resets internal registers such as wrap
count.

> >
> >    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.
> 
> Yes, after we enable a queue.
> 
> >
> > 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.
> 
> It looks to me it's better to have a single feature for disable/rest.
> And it's self-contained if:
> 
> 1) it holds a virtualenv state (indices)
> 2) allows the driver to modify the indices after it has been stopped
> (with another feature).
> 
> This seems to cover all the use cases.
> 
> Thanks

I am not 100% sure what you advocate for here. Dropping RESET
feature from spec completely for now, and replacing in 1.3 with
a feature that allows driver to modify internal device state?
Or something else?



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