Re: [Qemu-devel] [PATCH REPOST v19 1/2] virtio-crypto: Add virtio crypto device specification

[Gonglei (Arei)]

>
> > +
> > +The information required by AEAD session creation is stored in the
> virtio_crypto_aead_create_session_req
> > +structure, including the aead parameters stored in \field{para} and the
> cipher key in \field{key}.
> > +\field{input} stores the result of this operation.
> > +
> > +\drivernormative{\subparagraph}{Session operation: create session}{Device
> Types / Crypto Device / Device Operation / Control Virtqueue / Session
> operation / Session operation: create session}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST set the control general header and the
> corresponding algorithm-specific structure.
> > +    See \ref{sec:Device Types / Crypto Device / Device Operation / Control
> Virtqueue}.
> > +\item The driver MUST set the \field{opcode} field based on service type:
> CIPHER, HASH, MAC, or AEAD.
> > +\item The driver MUST set the \field{queue_id} field to show used dataq.
> 
> I've failed to figure out the semantic behind queue_id. This could mean,
> that sessions and session_id's are queue scoped. But then we should be
> (IMHO) more explicit on this -- my guess is that session_id's aren't
> dataqueue scoped.
> 
> > +\end{itemize*}
> > +
> > +\devicenormative{\subparagraph}{Session operation: create session}{Device
> Types / Crypto Device / Device
> > +Operation / Control Virtqueue / Session operation / Session operation:
> create session}
> > +
> > +\begin{itemize*}
> > +\item The device MUST use the corresponding algorithm-specific structure
> according to the
> > +    \field{opcode} in the control general header.
> > +\item The device MUST set the \field{status} field to one of the following
> values of enum
> > +    VIRTIO_CRYPTO_STATUS after finish a session creation:
> > +\begin{itemize*}
> > +\item VIRTIO_CRYPTO_OK if a session is created successfully.
> > +\item VIRTIO_CRYPTO_NOTSUPP if the requested algorithm or operation is
> unsupported.
> > +\item VIRTIO_CRYPTO_NOSPC if no free session ID (only when the
> VIRTIO_CRYPTO_F_MUX_MODE
> > +    feature bit is negotiated).
> > +\item VIRTIO_CRYPTO_ERR if failure not mentioned above occurs.
> 
> I guess an invalid queue_id would be among these.
> 
> > +\end{itemize*}
> > +\item The device MUST set the \field{session_id} field to a unique session
> identifieronly
> s/identifieronly/identifier only
> > +    if the status is set to VIRTIO_CRYPTO_OK.
> > +\end{itemize*}
> > +
> > +\drivernormative{\subparagraph}{Session operation: destroy
> session}{Device Types / Crypto Device / Device
> > +Operation / Control Virtqueue / Session operation / Session operation:
> destroy session}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST set the \field{opcode} field based on service type:
> CIPHER, HASH, MAC, or AEAD.
> > +\item The driver MUST set the \field{session_id} to a valid value assigned 
> > by
> the device
> > +    when the session was created.
> 
> Destroy does not need to specify queue_id. That means session_id's aren't
> queue scoped from namespace perspective. The question remains what is
> queue_id good for, and whether a session type op request should be
> rejected if the the session id originates from a session creation
> request specifying a different dataqueue (not the dataqueue containing
> the given request)?
> 

My original idea about the queue_id is using the queue_id to specify which
datequeue of the following data requests will be used. But after deep thinking,
I find that the queue_id is superfluous, and the current code in QEMU also
don't use the queue_id value as well. That's because the we can use session_id
to find the pervious session information and get the current dataqueue id
from the used virtqueue .

So maybe we should drop the queue_id this time.


> > +\end{itemize*}
> > +
> > +\devicenormative{\subparagraph}{Session operation: destroy
> session}{Device Types / Crypto Device / Device
> > +Operation / Control Virtqueue / Session operation / Session operation:
> destroy session}
> > +
> > +\begin{itemize*}
> > +\item The device MUST set the \field{status} field to one of the following
> values of enum VIRTIO_CRYPTO_STATUS.
> > +\begin{itemize*}
> > +\item VIRTIO_CRYPTO_OK if a session is created successfully.
> > +\item VIRTIO_CRYPTO_ERR if any failure occurs.
> > +\end{itemize*}
> > +\end{itemize*}
> > +
> > +\subsubsection{Data Virtqueue}\label{sec:Device Types / Crypto Device /
> Device Operation / Data Virtqueue}
> >
> 
> [..]
> 
> I've run through the rest. It some stuff seems very repetitive. I wonder if
> we can do better.
> 
> I also dislike this 5.7.7.5.3 Steps of Operation part. I also don't
> understand why is symmetric special in this respect (HASH, MAC and
> AEAD don't have a 'Steps of Operation' section.
> 
Hmm.. I just took a example as the symmetric is the most complicated operation. 
;)

> I would like to try some things out with the reference implementation.
> Depending on how that goes I may or may not end up providing a detailed
> review for the rest before discussing what I've already addressed.
> 
Halil, Thank you for your work, pls go ahead :)

> Regards,
> Halil

Thanks,
-Gonglei