On Mon, Apr 25, 2022 at 11:45:51PM +0000, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin <[email protected]>
> > 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.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
