Re: [virtio-comment] [PATCH] virtio-rng: add a control queue

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 3 Aug 2020 17:00:34 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 03/08/2020 15:57, Cornelia Huck wrote:
> > On Mon,  3 Aug 2020 15:24:31 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> A control queue is needed to send command to ask the device to
> >> release the request buffers if it cannot provide data.
> >>
> >> Changes:
> >>
> >>   5.4.2 Virtqueues
> >>   0 requestq
> >>   1 controlq
> >>   controlq only exists if VIRTIO_RNG_F_CTRL_VQ set.
> >>
> >>   5.4.3 Feature bits
> >>   VIRTIO_RNG_F_CTRL_VQ (0)
> >>   Device has a control queue
> >>
> >>   5.4.5 Device Initialization
> >>   Identify and initialize the request virtqueue.
> >>   If the VIRTIO_RNG_F_CTRL_VQ feature bit is negotiated,
> >>   identify the control virtqueue.
> >>
> >>   5.4.6.3 Control Virtqueue
> >>   The driver uses the control virtqueue (if VIRTIO_RNG_F_CTRL_VQ is negotiated)
> >>   to send commands to control the device.
> >>   All commands are of the following form:
> >>   struct virtio_rng_ctrl_hdr {
> >>          u8 cmd;
> >>   };
> >>
> >>   5.4.6.3.1 Request Virtqueue Flushing
> >>   When the device is not able to provide enough entropy to the driver,
> >>   the driver can be stuck. And for instance, it cannot be removed from the
> >>   system to switch to another one. The flush command allows the driver to ask
> >>   the device to release all the buffers sent to the device and to unlock the
> >>   driver.
> >>   #define VIRTIO_RNG_CMD_FLUSH 0
> >>
> >> Bug-Link: https://github.com/oasis-tcs/virtio-spec/issues/83
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>  content.tex | 38 ++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 36 insertions(+), 2 deletions(-)  
> > 
> > (...)
> >   
> >> +\paragraph{Request Virtqueue Flushing}\label{sec:Device Types / Entropy Device / Device Operation / Control Virtqueue / Request Virtqueue Flushing}
> >> +
> >> +When the device is not able to provide enough entropy to the driver, the driver can be stuck.
> >> +And for instance, it cannot be removed from the system to switch to another one.  
> > 
> > Maybe
> > 
> > "...the driver may be stuck waiting for the device returning buffers,
> > and cannot be removed..." ?  
> 
> ok
> 
> >   
> >> +The flush command allows the driver to ask the device to release all the buffers sent to the
> >> +device and to unlock the driver.  
> > 
> > What does "unlock the driver" mean?  
> 
> In fact, this is very implementation specific: in current linux
> implementation the driver is waiting on the data with read mutex locked.
> And with this mutex locked you can't do anything else in the driver...
> 
> I will replace this by:
> "This allows the driver to release resources allocated to receive data
> from the device."

Sounds good.

> 
> > 
> > You'll probably also want a device normative statement for that, something like
> > 
> > "When the driver sends a flush request, the device MUST release all
> > outstanding buffers."
> >   
> >> +
> >> +\begin{lstlisting}
> >> +#define VIRTIO_RNG_CMD_FLUSH 0
> >> +\end{lstlisting}  
> > 
> > As we (currently) have only that one command, I think we need to
> > specify that (a) negotiation of the CTRL_VQ feature implies that the
> > flush command is available, and that (b) any other value of cmd should
> > (must?) be ignored by the device. (I'd assume that new commands would
> > require new feature bits.)  
> 
> I will add:
> 
> "The negociation of the CTRL_VQ feature implies that the flush command
> is available."

"If VIRTIO_RNG_F_CTRL_VQ is negotiated, the device MUST implement the
flush command." ?

> "Any other value of cmd MUST be ignored by the device."
> 
> But where? In this section?

I think you want a devicenormative paragraph in the control vq
subsection (also for the MUST statement above.) This needs to be
referenced by the conformance part of the spec.

> 
> Thanks,
> Laurent
>