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

[Laurent Vivier <lvivier@redhat.com>]

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."

> 
> 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."
"Any other value of cmd MUST be ignored by the device."

But where? In this section?

Thanks,
Laurent