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

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

On Wed, Oct 18, 2023 at 12:25:23PM +0200, Cornelia Huck wrote:
> On Tue, Oct 17 2023, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Tuesday, October 17, 2023 5:55 PM
> >> 
> >> On Mon, Oct 02 2023, Parav Pandit <parav@nvidia.com> wrote:
> >> > @@ -440,6 +440,38 @@ \subsubsection{Virtqueue
> >> > Re-enable}\label{sec:Basic Facilities of a Virtio Devic  as during
> >> > initial virtqueue discovery, but optionally with different  parameters.
> >> >
> >> > +\subsection{Dynamic Virtqueues}\label{sec:Basic Facilities of a
> >> > +Virtio Device / Virtqueues / Dynamic Virtqueues}
> >> > +
> >> > +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 and before the driver setting the
> >> DRIVER_OK status bit.
> >> 
> >> _Or_ if a virtqueue has been reset and the driver wants to re-enable it, right?
> >> 
> > Well no. Because above text is for "enablement", and not re-enablement.
> > If the driver want to reset and re-enable, it must be enabled in first place before setting driver_ok.
> > VQ not enabled before driver ok, cannot be reset, and hence cannot be re-enabled.
> >
> > So I think above text is fine because it says about "enables" and not "re-enables".
> >
> >> > +
> >> > +When VIRTIO_F_RING_DYNAMIC is negotiated, the driver can avoid
> >> > +enabling the virtqueues before setting the DRIVER_OK status bit; the
> >> > +driver can enable the specific virtqueues after the driver has set the
> >> DRIVER_OK status bit.
> >> 
> >> "the driver is not required to enable every virtqueue it wants to use before
> >> setting the DRIVER_OK status bit; it can choose to enable a virtqueue even after
> >> it has set the DRIVER_OK status bit."
> >> 
> > Sounds good. Will change it.
> >
> >> > +The virtqueue enable mechanism is transport specific.
> >> 
> >> Would that be the same mechanism as for re-enabling a queue after a queue
> >> reset? I guess I'm missing the relationship here...
> >> 
> > Yes, it is same.
> > There is no change in enabling/re-enabling the virtqueue after/before DRIVER_OK with/without _dynamic bit.
> > So no extra text added here.
> 
> 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.
> 
> (...)
> 
> >> > +When VIRTIO_F_RING_DYNAMIC is not negotiated, the driver MUST enable
> >> > +the required number of virtqueues before setting the DRIVER_OK status bit.
> >> 
> >> What does "required" mean here? It just chooses to enable the queues it wants
> >> to use, right?
> > Right.
> > Required meaning, whatever number of queues that driver choose to enable, those must be enabled before driver_ok.
> > So it is "required by the driver".
> > Would that be ok?
> 
> I'd write it as "the driver MUST enable any virtqueue it plans to use"
> or something like that.
> 
> (...)

It would have to be SHOULD - we can't add new MUST requirements not
contingent on a feature bit, we can give recommendation based on
existing installed base.

> >> We currently have a device normative statement:
> >> 
> >> "The device MUST NOT consume buffers or send any used buffer notifications to
> >> the driver before DRIVER_OK."
> >> 
> >> I guess we need to extend that to not doing that for not-yet-enabled queues in
> >> the dynamic virtqueue case? There's a (transport-specific) point in time when
> >> the driver tells the device that the queue is ready, right?
> >
> > We donât need extend this text because device does not know anything about the disabled queue, so it cannot consume any buffer from it anyway.
> > Above line only applied to the enabled virtqueue.
> 
> My point is: DRIVER_OK signals that the driver is done with its setup
> and the device may start to interact with the queue. Depending on the
> mechanism the transport uses for enabling the queue, it may "know" about
> the queue before the driver is ready -- it might need to wait until the
> driver has completed enabling of the queue. Not sure if that is worth
> spelling out.