Re: [virtio-comment] RE: [PATCH v3 2/2] content: Support enabling virtqueue after DRIVER_OK stage

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

On Thu, Oct 26, 2023 at 03:30:56AM +0000, Parav Pandit wrote:
> 
> > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> > open.org> On Behalf Of Michael S. Tsirkin
> > Sent: Wednesday, October 25, 2023 3:54 PM
> > 
> > On Wed, Oct 25, 2023 at 09:55:20AM +0000, Parav Pandit wrote:
> > > Hi Cornelia,
> > >
> > > > From: Parav Pandit <parav@nvidia.com>
> > > > Sent: Monday, October 23, 2023 8:52 PM
> > > > To: Cornelia Huck <cohuck@redhat.com>; Michael S. Tsirkin
> > > > <mst@redhat.com>
> > > > Cc: virtio-comment@lists.oasis-open.org; hengqi@linux.alibaba.com;
> > > > xuanzhuo@linux.alibaba.com; Shahaf Shuler <shahafs@nvidia.com>
> > > > Subject: RE: [virtio-comment] RE: [PATCH v3 2/2] content: Support
> > > > enabling virtqueue after DRIVER_OK stage
> > > >
> > > >
> > > > > From: virtio-comment@lists.oasis-open.org
> > > > > <virtio-comment@lists.oasis- open.org> On Behalf Of Cornelia Huck
> > > > > Sent: Monday, October 23, 2023 8:31 PM
> > > > >
> > > > > On Mon, Oct 23 2023, Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > >> From: Cornelia Huck <cohuck@redhat.com>
> > > > > >> Sent: Monday, October 23, 2023 7:58 PM
> > > > > >
> > > > > >> On Mon, Oct 23 2023, Parav Pandit <parav@nvidia.com> wrote:
> > > > > >>
> > > > > >> >> From: virtio-comment@lists.oasis-open.org
> > > > > >> >> <virtio-comment@lists.oasis- open.org> On Behalf Of Cornelia
> > > > > >> >> Huck
> > > > > >> >> Sent: Monday, October 23, 2023 7:27 PM
> > > > > >> >
> > > > > >> >> Well, there are  still comments from me here that post-date
> > > > > >> >> v4 (please wait for a bit before posting another version!),
> > > > > >> >> so I'll continue waiting for them to be addressed first.
> > > > > >> > The last one was [1].
> > > > > >> > Which I replied few days ago at [2].
> > > > > >> >
> > > > > >> > [1]
> > > > > >> > https://lists.oasis-open.org/archives/virtio-comment/202310/m
> > > > > >> > sg00
> > > > > >> > 24
> > > > > >> > 0.h
> > > > > >> > tml [2]
> > > > > >> > https://lists.oasis-open.org/archives/virtio-comment/202310/m
> > > > > >> > sg00
> > > > > >> > 24
> > > > > >> > 2.h
> > > > > >> > tml
> > > > > >>
> > > > > >> I still have some open questions from
> > > > > >> <87sf68cjn0.fsf@redhat.com> (first and last paragraph.)
> > > > > >
> > > > > > Above link is not accessible to me. â
> > > > >
> > > > > That's not a link, but a message id... many mail clients support
> > > > > searching by it, and you can also get it from lore:
> > > > >
> > > > > 87sf68cjn0.fsf@redhat.com/">https://lore.kernel.org/all/87sf68cjn0.fsf@redhat.com/
> > > > >
> > > > > > I addressed your comments/questions in v4.
> > > > > > Can you please check v4 if they are addressed? I rewrote as you
> > > > > > suggested in
> > > > > v3.
> > > > >
> > > > > This was for points you did not agree with. (Again, it might be
> > > > > helpful to wait with posting a new version until discussion has
> > > > > somewhat
> > > > > concluded.)
> > > > Sure, will wait next time.
> > > >
> > > > Your comment was,
> > > > =====
> > > > This is not really clear to me just from this text, especially if
> > > > you just wrote above that enabling or re-enabling is something
> > > > different... my understanding would be:
> > > >
> > > > - if neither dynamic vqs nor queue reset are supported or negotiated,
> > > >   the only way to enable a vq is before DRIVER_OK, during setup
> > > > - both of these features rely on the transport supporting enabling
> > > >   individual queues (either a queue that has not been enabled before, or
> > > >   a queue that has been reset)
> > > > - the transport is supposed to use the same mechanism for either
> > > >
> > > > Did I get it right? If so, I think we should make it a bit more clear.
> > > > =====
> > > >
> > > > Above is clarified in below wording without complicating the queue_reset
> > here.
> > > >
> > > > Does that look ok to you?
> > > >
> > > > +When VIRTIO_F_RING_DYNAMIC is not negotiated, the driver enables
> > > > +the virtqueues during the device initialization sequence, i.e.
> > > > +after the device sets the FEATURES_OK status bit
> > 
> > all status bits are set by driver
> >
> Well device clears it if it does not like the features.
> 
> Yes. I will rewrite it as, 
> 
> i.e. after the driver has verified that FEATURES_OK bit is set and before the driver sets DRIVER_OK bit.

shorter: i.e. after FEATURES_OK is set and before DRIVER_OK is set.

> > > and before the driver setting the
> > 
> > 
> > setting -> sets
> > 
> > > > DRIVER_OK status bit.
> > > > +
> > > > +When VIRTIO_F_RING_DYNAMIC is negotiated, the driver is not
> > > > +required to
> > 
> > try to avoid "required" outside conformance sections though pls.
> > And when used, we upper-case it.
> > 
> Ok.
> How about a rewrite as,
> 
> When VIRTIO_F_RING_DYNAMIC is negotiated, the driver may not enable every virtqueue, which the driver wants to use before setting the DRIVER_OK...

This repetition does not buy us much - verbose and "when" is confusing too, you just mean "if". So simply:

If VIRTIO_F_RING_DYNAMIC is negotiated,
the driver can choose to enable some virtqueues
after DRIVER_OK is set.


> 
> > > > +enable every virtqueue it wants to use before setting the DRIVER_OK
> > > > +status bit; the driver can choose to enable a virtqueue even after
> > > > +the driver has set the DRIVER_OK status bit.




>> The virtqueue enable
> > > > +mechanism is
> > > > transport specific.




I still feel there is no real explanation in a transport agnostic
manner what does it mean to "enable" virtqueue. Do you maybe just mean
"configure virtqueue"?


Can we just avoid talking about "enabling"?
Is it true that you can basically change anything you want about the vq
not just the enable bit? If so talking about enabling is just
confusing I think.

-- 
MST