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

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

On Mon, Apr 25, 2022 at 11:45:51PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, April 25, 2022 7:11 PM
> > 
> [..]
> 
> > That's a weird way to describe this.  I guess the logic in the spec is simply a
> > register bit that is set to != 1 when reset starts and set to 1 when reset
> > completes.
> > You propose reversing its polarity. Fair enough.
> > 
> I will drop this part from the commit message as example clarifies it enough without device side code.
> 
> > 
> > I do think it's inelegant that the value is different when reset is through vq
> > reset (1) and through device reset (0).
> 
> > >  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1
> > > to -\field{queue_reset} to reset the queue, it MUST verify that the
> > > queue -has been reset by reading back \field{queue_reset} and ensuring
> > > that it -is 1. The driver MAY re-enable the queue by writing a 1 to
> > > +\field{queue_reset} to reset the queue, driver MUST verify that the
> > > +queue has been reset by reading back \field{queue_reset} until device
> > returns a value of 0.
> > > +Device continue to report value of 1 for the \field{queue_reset}
> > > +until device finishes
> > 
> > this MUST should move to conformance chapter BTW.
> 
> In this proposal it is covered in the section 4.1.4.3.2 " Driver Requirements: Common configuration structure layout".
> This section is listed in 7.2 " Clause 2: PCI Driver Conformance".
> 
> To me it appears that this is covered in the conformance.
> What am I missing?

My bad. git diff did something I did not expect here showing \subsection
instead of \drivernormative. I'll have to learn how it decides which
line to show as function name.


> > 
> > > +the queue reset request. When the device completes the queue reset,
> > > +\field{queue_reset} and \field{queue_enable} set to zero,
> > 
> > set-> "are set"
> > 
> Will fix.
> 
> > > indicating
> > > +that specified queue is ready to be enabled again. The driver MAY
> > > +re-enable the queue by writing a 1 to
> > 
> > a 1 -> 1
> Will fix.
> 
> > 
> > >  \field{queue_enable} after ensuring that the other virtqueue fields
> > > have
> > 
> > the other -> other (not your fault)
> > 
> Should this be pre-patch?
> Or small enough editorial change to be part of this?

ideally a separate patch and issue.

> > >  been set up correctly. The driver MAY set driver-writeable queue
> > > configuration  values to different values than those that were used before
> > the queue reset.
> > >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue
> > Reset}).
> > >
> > > +
> > 
> > drop this pls.
> My bad. Will fix it.
> 
> > 
> > ccw will have to be changed too.
> My weak area. Didn't find the vq reset section in below 3 commits.
> 
> a4ce81a virtio: mmio support virtqueue reset
> 12998e7 virtio: pci support virtqueue reset
> 3b5378d virtio: introduce virtqueue reset as basic facility
> 
> In a previous email by Cornelia, she replied " For ccw, we have not yet added queue reset;".
> So, can we please do this later in a some a non_bug_fix patch?

Oh right, I was confused. maybe mention in commit log so others are not
confused.

> I will revise v1 for the comments that I can fix.
> Based on v1 review, we can split patches and improve further.