Hi,
>
>
> On 02/08/2017 07:24 AM, Gonglei (Arei) wrote:
> > Hi Halil,
> >
> > Thanks for your comments firstly.
> >
>
> You are welcome :). Sorry it took so long -- I'm currently
> quite busy.
>
It's fine.
> >>
> >> On 01/18/2017 09:22 AM, Gonglei wrote:
> >>> The virtio crypto device is a virtual crypto device (ie. hardware
> >>> crypto accelerator card). Currently, the virtio crypto device provides
> >>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
> >>>
> >>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
> >>>
> >>> VIRTIO-153
> >>>
> >>> Signed-off-by: Gonglei <arei.gong...@huawei.com>
> >>> CC: Michael S. Tsirkin <m...@redhat.com>
> >>> CC: Cornelia Huck <cornelia.h...@de.ibm.com>
> >>> CC: Stefan Hajnoczi <stefa...@redhat.com>
> >>> CC: Lingli Deng <denglin...@chinamobile.com>
> >>> CC: Jani Kokkonen <jani.kokko...@huawei.com>
> >>> CC: Ola Liljedahl <ola.liljed...@arm.com>
> >>> CC: Varun Sethi <varun.se...@freescale.com>
> >>> CC: Zeng Xin <xin.z...@intel.com>
> >>> CC: Keating Brian <brian.a.keat...@intel.com>
> >>> CC: Ma Liang J <liang.j...@intel.com>
> >>> CC: Griffin John <john.grif...@intel.com>
> >>> CC: Mihai Claudiu Caraman <mike.cara...@nxp.com>
> >>> ---
> >>> content.tex | 2 +
> >>> virtio-crypto.tex | 1245
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> 2 files changed, 1247 insertions(+)
> >>> create mode 100644 virtio-crypto.tex
> >>>
> >>> diff --git a/content.tex b/content.tex
> >>> index 4b45678..ab75f78 100644
> >>> --- a/content.tex
> >>> +++ b/content.tex
> >>> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len},
> >> \field{residual},
> >>> \field{status_qualifier}, \field{status}, \field{response} and
> >>> \field{sense} fields.
> >>>
> >>> +\input{virtio-crypto.tex}
> >>> +
> >>> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>>
> >>> Currently there are three device-independent feature bits defined:
> >>> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> >>> new file mode 100644
> >>> index 0000000..732cd30
> >>> --- /dev/null
> >>> +++ b/virtio-crypto.tex
> >>> @@ -0,0 +1,1245 @@
> >>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> >>> +
> >>> +The virtio crypto device is a virtual cryptography device as well as a
> >>> kind of
> >>> +virtual hardware accelerator for virtual machines. The encryption and
> >>> +decryption requests are placed in any of the data active queues and are
> >> ultimately handled by the
> >>
> >> Am I the only one having a problem with 'data active queues'?
> >
> > Maybe 'data queues' here is enough?
> >
>
> I guess 'active' came from virtio-net. For virtio-crypto you do not define
> active and passive queues, and my guess is there is no intention/reason
> to do this in the future either. Thus yes 'data queues' is fine. And
> 'active data queues' would be grammatically correct, but substantially
> confusing and wrong.
>
OK.
> >> I have objected on this before.
> >>
> >>> +backend crypto accelerators. The second kind of queue is the control
> queue
> >> used to create
> >>> +or destroy sessions for symmetric algorithms and will control some
> >> advanced
> >>> +features in the future. The virtio crypto device provides the following
> crypto
> >>> +services: CIPHER, MAC, HASH, and AEAD.
> >>> +
> >>> +
> >>> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device
> ID}
> >>> +
> >>> +20
> >>> +
> >>> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device /
> >> Virtqueues}
> >>> +
> >>> +\begin{description}
> >>> +\item[0] dataq1
> >>> +\item[\ldots]
> >>> +\item[N-1] dataqN
> >>> +\item[N] controlq
> >>> +\end{description}
> >>> +
> >>> +N is set by \field{max_dataqueues}.
> >>> +
> >>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device /
> Feature
> >> bits}
> >>> +
> >>> +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is
> available.
> >>> +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode
> is
> >> available for CIPHER service.
> >>> +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is
> >> available for HASH service.
> >>> +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is
> >> available for MAC service.
> >>> +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is
> >> available for AEAD service.
> >>> +
> >>
> >> I'm not sure that "non-session" entirely correct grammatically. I would
> >> prefer sessionless as alternatively proposed by Stefan, or even stateless.
> >> I think stateless is the phrase most frequently used to describe what
> >> we want to introduce -- that is basically response = f(request) and
> >> not response = f(request, server_state) where the server_state is
> >> a is determined by a series of previous interactions between the server
> >> and the client).
> >>
> > Makes sense. I discussed with Xin about this on call meeting two weeks ago.
> > I decide to use stateless mode instead of non-session mode here.
> >
> >>> +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto
> >> Device / Feature bits}
> >>> +
> >>> +Some crypto feature bits require other crypto feature bits
> >>> +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature
> Bits}):
> >>> +
> >>> +\begin{description}
> >>> +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires
> >> VIRTIO_CRYPTO_F_NON_SESSION_MODE.
> >>> +\end{description}
> >>> +
> >>> +\subsection{Device configuration layout}\label{sec:Device Types / Crypto
> >> Device / Device configuration layout}
> >>> +
> >>> +The following driver-read-only configuration fields are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +struct virtio_crypto_config {
> >>> + le32 status;
> >>> + le32 max_dataqueues;
> >>> + le32 crypto_services;
> >>> + /* Detailed algorithms mask */
> >>> + le32 cipher_algo_l;
> >>> + le32 cipher_algo_h;
> >>> + le32 hash_algo;
> >>> + le32 mac_algo_l;
> >>> + le32 mac_algo_h;
> >>> + le32 aead_algo;
> >>> + /* Maximum length of cipher key */
> >>> + le32 max_cipher_key_len;
> >>> + /* Maximum length of authenticated key */
> >>> + le32 max_auth_key_len;
> >>> + le32 reserved;
> >>> + /* Maximum size of each crypto request's content */
> >>> + le64 max_size;
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> >> ~VIRTIO_CRYPTO_S_HW_READY.
> >>
> >> Not entirely happy with this. What you want to say is reserved
> >> for future use, or? Would it make sense to have a general note
> >> -- in a similar fashion like for 'sizes are in bytes' -- for
> >> reserved for future use?
> >>
> >> One possible formulation would be:
> >>
> >> "In this specification, unless explicitly stated otherwise,
> >> fields and bits reserved for future use shall be zeroed out.
> >> Both the a device or a driver device and the driver should
> >> detect violations of this rule, and deny the requested
> >> operation in an appropriate way if possible."
> >>
> >>
> > Cornelia also provided a good suggestion about this. :)
> >
>
> Yeah.
>
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_S_HW_READY (1 << 0)
> >>> +\end{lstlisting}
> >>> +
> >>> +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the
> >> hardware is ready to work or not.
> >>
> >> I do not like hardware here.
> >>
> > Sorry about that, but it has existed in both Qemu and Linux driver. :(
> >
>
> This is a spec, that's an implementation. You could theoretically use
> different names, but I would not go that far. I would keep the constant
> name but reword the sentence stating the semantic.
>
> You seem to use "hardware", "accelerator", "backend", "hardware-backend",
> "backend accelerator", "backend crypto accelerator(s)" interchangeably -- as
> if these had the same meaning. Moreover I do not remember seeing a (precise)
> definition for any of these phrases. Or am I wrong?
>
> AFAIK, while avoiding word repetition (by using synonyms for example) is
> considered good style in general, technical writing and especially
> specifications are a prominent exception. In specifications consistent wording
> should be preferred over avoiding repetition, and IMHO this is
> especially important that the concepts of the domain under specification
> are referred to in a consistent way (e.g. called always the same).
>
> Do you agree? If you do, could we consolidate the virtio crypto spec
> in this sense?
>
Yes, I do. I'll try my best to perfect the spec.
> >>> +
> >>> +The following driver-read-only fields include \field{max_dataqueues},
> which
> >> specifies the
> >>
> >> Why following?
> >>
> > Er, I used 'following' at here and there... It's just an auxiliary word.
> >
>
> Well, I wanted to say 'following' and 'include' together make this
> sentence look very strange to me.
>
OK. Will fix them.
> AFAIU your answer 'following' is meant to be kind of ornamental here.
> I also think we should drop any elements (words, sentences) having
> purely ornamental function.
>
> >>> +maximum number of data virtqueues (dataq1\ldots dataqN), and
> >> \field{crypto_services},
> >>> +which indicates the crypto services the virtio crypto supports.
> >>> +
> >>> +The following services are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +/* CIPHER service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
> >>> +/* HASH service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_HASH 1
> >>> +/* MAC (Message Authentication Codes) service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_MAC 2
> >>> +/* AEAD (Authenticated Encryption with Associated Data) service */
> >>> +#define VIRTIO_CRYPTO_SERVICE_AEAD 3
> >>> +\end{lstlisting}
> >>> +
> >>> +The last driver-read-only fields specify detailed algorithms masks
> >>> +the device offers for corresponding services. The following CIPHER
> >> algorithms
> >>> +are defined:
> >>
> >> You do not establish an explicit relationship between the fields and the
> >> macros for the flags. These are flags or? It seems quite common in the
> >> spec to use _F_ in flag names. Would it be appropriate here?
> >>
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_CIPHER 0
> >>> +#define VIRTIO_CRYPTO_CIPHER_ARC4 1
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB 2
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC 3
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR 4
> >>> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB 5
> >>> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC 6
> >>> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB 7
> >>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC 8
> >>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR 9
> >>> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8 10
> >>> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2 11
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_F8 12
> >>> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS 13
> >>> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3 14
> >>> +\end{lstlisting}
> >>> +
> >>> +The following HASH algorithms are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_HASH 0
> >>> +#define VIRTIO_CRYPTO_HASH_MD5 1
> >>> +#define VIRTIO_CRYPTO_HASH_SHA1 2
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_224 3
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_256 4
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_384 5
> >>> +#define VIRTIO_CRYPTO_HASH_SHA_512 6
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_224 7
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_256 8
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_384 9
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_512 10
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128 11
> >>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256 12
> >>> +\end{lstlisting}
> >>> +
> >>> +The following MAC algorithms are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_MAC 0
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5 1
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1 2
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224 3
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256 4
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384 5
> >>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512 6
> >>> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES 25
> >>> +#define VIRTIO_CRYPTO_MAC_CMAC_AES 26
> >>> +#define VIRTIO_CRYPTO_MAC_KASUMI_F9 27
> >>> +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2 28
> >>> +#define VIRTIO_CRYPTO_MAC_GMAC_AES 41
> >>> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH 42
> >>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES 49
> >>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9 50
> >>> +#define VIRTIO_CRYPTO_MAC_XCBC_AES 53
> >>> +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3 54
> >>> +\end{lstlisting}
> >>> +
> >>> +The following AEAD algorithms are defined:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_NO_AEAD 0
> >>> +#define VIRTIO_CRYPTO_AEAD_GCM 1
> >>> +#define VIRTIO_CRYPTO_AEAD_CCM 2
> >>> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305 3
> >>> +\end{lstlisting}
> >>> +
> >>
> >> Would it make sense to interleave the flag definition
> >> with the struct virtio_crypto_config?
> >>
> >>> +\begin{note}
> >>> +Any other values are reserved for future use.
> >>
> >> Are these flags or values? I do not think values is appropriate here.
> >>
> > Please refer to my reply in another thread.
> >
>
> Will answer there.
>
> >>> +\end{note}
> >>> +
> >>
> >> Some fields are missing here. This is inconsistent. Either
> >> you should describe all or none (here).
> >>
> > Ok, will add other fields.
> >
>
> Thx.
>
> >>> +\devicenormative{\subsubsection}{Device configuration layout}{Device
> Types
> >> / Crypto Device / Device configuration layout}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The device MUST set \field{max_dataqueues} to between 1 and
> 65535
> >> inclusive.
> >>> +\item The device MUST set \field{status} based on the status of the
> >> hardware-backed implementation.
> >>
> >> What is a hardware-backend?
> >>
> > I meant the cryptodev backend, cryptodev-builtin, cryptodev-vhost-user for
> example.
> >
>
> See above. This was a rhetorical question. Many of my question
> are rhetorical, and are meant to be a gentle indicator of
> concern -- in opposition to saying something is straight-out
> incomprehensible or wrong.
>
Sorry for misunderstanding. :(
> I understood what hardware-backend means because I'm familiar with your
> Linux/QEMU implementation. Please keep in mind, this specification
> should make clean-room implementations possible (I mean, with
> no looking at the linux kernel, or qemu code or whatherver).
>
You are right.
> >>> +\item The device MUST accept and handle requests after \field{status} is
> set
> >> to VIRTIO_CRYPTO_S_HW_READY.
> >>
> >> Shouldn't this be the other way around (if VIRTIO_CRYPTO_S_HW_READY
>
> Sorry, of course I have meant if ~VIRTIO_CRYPTO_S_HW_READY then reject!
>
> >> then reject). Is a not well formed request or a backend failure considered?
> >> What does handle mean? What should happen if requests are submitted
> >> before VIRTIO_CRYPTO_S_HW_READY is set?
> >>
> > The requests MUST not be transmitted before
> VIRTIO_CRYPTO_S_HW_READY is set.
> > Which is stated by the driver requirements. Otherwise the requests will be
> dropped.
>
> This ('Otherwise the requests will be dropped.') ain't specified or?
> Should not we specify this? How exactly are they 'dropped'?
>
Hmm, we should add those explanation.
> >
> > The handle means execute crypto operations.
> >
>
> But things can fail, you have a status
> (VIRTIO_CRYPT_(ERR|NOTSUPP|BADMSG))
> for that. If that happens, does the device violate this point?
>
> Do (all) other virtio devices have a similar conformance statement on when
> they 'MUST accept and handle' requests or packets? If not what do you think,
> should they?
>
Take virtio-net as an example, there is only one requirement about
VIRTIO_NET_S_LINK_UP:
Section 5.1.5
6. If the VIRTIO_NET_F_STATUS feature bit is negotiated, the link status comes
from the bottom bit of
status. Otherwise, the driver assumes it's active.
It didn't tell us what will be happen if the link isn't active. And what will
be happen if the driver still
transmit packets to the device if the link isn't active.
So I think the potential mean is the driver MUST not transmit packets if the
link isn't active.
> >>> +\item The device MUST set \field{crypto_services} based on the crypto
> >> services the device offers.
> >>> +\item The device MUST set detailed algorithms masks based on the
> >> \field{crypto_services} field.
> >>> +\item The device MUST set \field{max_size} to show the maximum size of
> >> crypto request the device supports.
> >>> +\item The device MUST set \field{max_cipher_key_len} to show the
> >> maximum length of cipher key if the device supports CIPHER service.
> >>> +\item The device MUST set \field{max_auth_key_len} to show the
> maximum
> >> length of authenticated key if the device supports MAC service.
> >>> +\end{itemize*}
> >>> +
> >>> +\drivernormative{\subsubsection}{Device configuration layout}{Device
> Types
> >> / Crypto Device / Device configuration layout}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The driver MUST read the ready \field{status} from the bottom bit
> of
> >> status to check whether the hardware-backed
> >>> + implementation is ready or not, and the driver MUST reread it
> after
> >> the device reset.
> >>> +\item The driver MUST NOT transmit any packets to the device if the
> ready
> >> \field{status} is not set.
> >>> +\item The driver MUST read \field{max_dataqueues} field to discover the
> >> number of data queues the device supports.
> >>> +\item The driver MUST read \field{crypto_services} field to discover
> >>> which
> >> services the device is able to offer.
> >>> +\item The driver MUST read the detailed algorithms fields based on
> >> \field{crypto_services} field.
> >>> +\item The driver SHOULD read \field{max_size} to discover the maximum
> >> size of crypto request the device supports.
> >>> +\item The driver SHOULD read \field{max_cipher_key_len} to discover the
> >> maximum length of cipher key the device supports.
> >>> +\item The driver SHOULD read \field{max_auth_key_len} to discover the
> >> maximum length of authenticated key the device supports.
> >>> +\end{itemize*}
> >>> +
> >>> +\subsection{Device Initialization}\label{sec:Device Types / Crypto
> >>> Device /
> >> Device Initialization}
> >>> +
> >>> +\drivernormative{\subsubsection}{Device Initialization}{Device Types /
> >> Crypto Device / Device Initialization}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The driver MUST identify and initialize the control virtqueue.
> >>
> >> But does not have to identify and initialize any data virtqueues?
> >>
> > Good catch. Both controlq and dataq.
> >
> >>> +\item The driver MUST read the supported crypto services from bits of
> >> \field{crypto_services}.
> >>> +\item The driver MUST read the supported algorithms based on
> >> \field{crypto_services} field.
> >>> +\end{itemize*}
> >>> +
> >>> +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> >> Crypto Device / Device Initialization}
> >>> +
> >>> +\begin{itemize*}
> >>> +\item The device MUST be configured with at least one accelerator which
> >> executes backend crypto operations.
> >>
> >> What does configured mean here? Is this initialization requirement.
> >>
> > It means the virtio crypto device MUST attach cryptodev backend.
> > Yes, it's a requirement.
>
> What happens if an implementation fails to fulfill this requirement?
> What is the benefit of having this requirement?
>
Let me drop it. We only care about the 'status' field. if an implementation
fails to fulfill this requirement the VIRTIO_CRYPTO_S_HW_READY bit doesn't be
set.
> >
> >>> +\item The device MUST write the \field{crypto_services} field based on
> >>> the
> >> capacities of the backend accelerator.
> >>> +\end{itemize*}
> >>> +
> >>
> >> How do 'Initialization' and 'Configuration Layout' requirements relate to
> >> each-other.
> >>
> > Basically they do the same things. But 'configuration layout' can't state
> > the
> > specific time.
> >
> >>> +\subsection{Device Operation}\label{sec:Device Types / Crypto Device /
> >> Device Operation}
> >>> +
> >>> +Packets can be transmitted by placing them in both the controlq and
> dataq.
> >>> +Packets consist of a general header and a service-specific request.
> >>
> >> Are packets and requests synonyms?
> >>
> > Yes, they are in virtio crypto spec.
> >
>
> See above regarding consistent naming.
>
> >>> +Where 'general header' is for all crypto requests, and 'service specific
> >> requests'
> >>
> >> From below it seems you have two types of 'general header', but up until
> this
> >> point it seems there is a single definition. Of course, this does not
> >> really matter.
> >>
> > I distinguish it based on controlq and dataq in following statement.
> >
>
> How about 'request header' or 'queue header'? Or something like
> 'Packets consist of a queue-type specific header specifying among
> others the operation, and an operation specific payload.'
>
Sounds good.
> I do not like 'general' here.
>
> >>> +are composed of operation parameter + output data + input data in
> general.
> >>> +Operation parameters are algorithm-specific parameters, output data is
> the
> >>> +data that should be utilized in operations, and input data is equal to
> >>> +"operation result + result data".
> >>> +
> >>> +The device can support both session mode (See \ref{sec:Device Types /
> >> Crypto Device / Device Operation / Control Virtqueue / Session operation})
> and
> >> non-session mode, for example,
> >>> +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is
> >> negotiated, the driver can use non-session mode for CIPHER service,
> otherwise
> >> it can only use session mode.
> >>
> >> Grammar: Does not seem right to me. 'As' seems off and this could be two
> >> sentences.
> >>
> > OK. Let me s/As/If the/ here.
> >
>
> Capital won't do due to the 'for example, ' in the previous line.
> Otherwise OK.
>
Yes.
> >>
> >>> +
> >>> +\begin{note}
> >>> +The basic unit of all data length the byte.
> >>> +\end{note}
> >>> +
> >>> +The general header for controlq is as follows:
> >>> +
> >>> +\begin{lstlisting}
> >>> +#define VIRTIO_CRYPTO_OPCODE(service, op) (((service) << 8) | (op))
> >>> +
> >>> +struct virtio_crypto_ctrl_header {
> >>> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> >> 0x02)
> >>> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> >> 0x03)
> >>> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH,
> 0x02)
> >>> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH,
> 0x03)
> >>> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC,
> 0x02)
> >>> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC,
> 0x03)
> >>> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD,
> 0x02)
> >>> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD,
> 0x03)
> >>> + le32 opcode;
> >>> + le32 algo;
> >>> + le32 flag;
> >>> + /* data virtqueue id */
> >>> + le32 queue_id;
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +The general header of dataq:
> >>> +
> >>> +\begin{lstlisting}
> >>> +struct virtio_crypto_op_header {
> >>> +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
> >>> +#define VIRTIO_CRYPTO_CIPHER_DECRYPT \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
> >>> +#define VIRTIO_CRYPTO_HASH \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
> >>> +#define VIRTIO_CRYPTO_MAC \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
> >>> +#define VIRTIO_CRYPTO_AEAD_ENCRYPT \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> >>> +#define VIRTIO_CRYPTO_AEAD_DECRYPT \
> >>> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> >>> + le32 opcode;
> >>> + /* algo should be service-specific algorithms */
> >>> + le32 algo;
> >>> + /* session_id should be service-specific algorithms */
> >>> + le64 session_id;
> >>> +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1
> >>> +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2
> >>
> >> Use _F_ istead of _FLAG_?
> >>
> > I'm afraid they are confused with the feature bits.
> >
>
> Have a look at the desc flags (for example VIRTQ_DESC_F_INDIRECT).
>
But in virtio crypto spec, we have the following feature bits:
VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available.
VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is available for
CIPHER service.
VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is available for
HASH service.
VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is available for MAC
service.
VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is available for
AEAD service.
They are too similar.
> It is your call in the end.
>
> >>> + /* control flag to control the request */
> >>> + le32 flag;
> >>> + le32 padding;
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +The device can set the operation status as follows: VIRTIO_CRYPTO_OK:
> >> success;
> >>> +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP:
> >> not supported;
> >>> +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto
> >> operations.
> >>> +
> >>> +\begin{lstlisting}
> >>> +enum VIRITO_CRYPTO_STATUS {
> >>> + VIRTIO_CRYPTO_OK = 0,
> >>> + VIRTIO_CRYPTO_ERR = 1,
> >>> + VIRTIO_CRYPTO_BADMSG = 2,
> >>> + VIRTIO_CRYPTO_NOTSUPP = 3,
> >>> + VIRTIO_CRYPTO_INVSESS = 4,
> >>> + VIRTIO_CRYPTO_MAX
> >>> +};
> >>> +\end{lstlisting}
> >>> +
> >>> +\subsubsection{Control Virtqueue}\label{sec:Device Types / Crypto Device
> /
> >> Device Operation / Control Virtqueue}
> >>> +
> >>> +The driver uses the control virtqueue to send control commands to the
> >>> +device which handles the non-data plane operations, such as session
> >>
> >> What is a 'non-data plane'?
> >>
> > I named controlling plane as the non-data plane. Maybe it's superfluous.
> >
>
> Again consisten naming. I would prefer avoiding introducing this extra
> name.
>
Yes. Let me drop it.
> >>> +operations (See \ref{sec:Device Types / Crypto Device / Device Operation
> /
> >> Control Virtqueue / Session operation}).
> >>
> >> Reviewed up until here. Depending on how things evolve will try to
> >> review the rest too in the following days.
> >>
> > I knew it's a hard and time-consuming work to review such long spec.
> >
> > Great thanks for your reviewing. :)
> >
>
> I know, its annoying to deal with my comments ;). IMHO as spec
> without a certain quantity of rigor is not really a spec. I think
> it's very hard to write a good spec, so be patient.
>
Yes, I agree. Please join me when you have a leisure if possible, haha...
I invited you too in other thread. ;)
> >> A question and a remark as a closing word:
> >> * Are there already some kernel and qemu patches for this 'non-session'
> stuff?
> >
> > THB currently I have a patch for virtio_crypto.h.
> >
> > Attaching it for better review.
> >
>
> I would prefer having an implementation of the new features on the list,
> before voting on the spec itself.
>
As we reach a general consensus, I'll start coding.
Thanks,
-Gonglei
> >> * I think some proofreading (and eventually also touch-up) by a native
> speaker
> >> would really benefit us. Sadly my grammar skills are very questionable, so
> >> I can't help much. Nevertheless since it is a spec, I think we sould strive
> >> for high standards in language usage too.
> >>
> > I agree and I actually asked a native speaker in my company to review it
> > several months ago. When all comments are handled, I'll do it again.
>
> Thats cool.
>
> Regards,
> Halil
>
> >
> > Thanks,
> > -Gonglei
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
