>
>
> On 05/04/2017 04:13 PM, Gonglei (Arei) wrote:
> >>
> >>
> >> On 05/04/2017 03:33 PM, Gonglei (Arei) wrote:
> >>>>> +\begin{description}
> >>>>> +\item[VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE] Requires
> >>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>> +\item[VIRTIO_CRYPTO_F_HASH_STATELESS_MODE] Requires
> >>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>> +\item[VIRTIO_CRYPTO_F_MAC_STATELESS_MODE] Requires
> >>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>> +\item[VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE] Requires
> >>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>> +\end{description}
> >>>>
> >>>> I find feature bit 0 redundant and bit confusing. We had a discussion
> >>>> in v15 and v16.
> >>>>
> >>>> Could you answer:
> >>>> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03214.html
> >>>> (Message-ID:
> >> <1fbe30cc-87ec-32bc-4c57-85f9b03b3...@linux.vnet.ibm.com>)
> >>>>
> >>>>
> >>> Please see my reply:
> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03186.html
> >>>
> >>> The main reason is we should keep compatibility to pre-existing driver and
> >>> support some function that different services have different modes.
> >>> We have only one unique crypto request named structure
> >> virtio_crypto_op_data_req_mux.
> >>> Please continue to see the sepc, you'll find the truth.
> >>>
> >>
> >> Sorry, I still do not understand why can't we drop
> >> VIRTIO_CRYPTO_F_STATELESS_MODE
> >> and just keep the four service specific modes.
> >>
> >> Can you point me to the (published) code where
> >> VIRTIO_CRYPTO_F_STATELESS_MODE is
> >> used (that's what I'm missing -- preferably state the repository, the
> >> commit
> >> a file and a line using VIRTIO_CRYPTO_F_STATELESS_MODE)?
> >>
> > No no no, there isn't existing code use VIRTIO_CRYPTO_F_STATELESS_MODE
> yet,
> > It's just for future use.
> >
>
> Thanks, that's a crucial piece of information. In the thread I referenced
> this was not cleared (at least for me). It would be really nice to have
> some working code before doing the spec, because I find it very easy to miss
> a detail which renders the whole thing useless if one works solely on
> theoretical level and does not try to create at least a working prototype.
>
I see.
> > Please the blow description:
> > """
> > \drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto
> Device / Device Operation / HASH Service Operation}
> >
> > \begin{itemize*}
> > \item If the driver uses the session mode, then the driver MUST set
> \field{session_id} in struct virtio_crypto_op_header
> > to a valid value assigned by the device when the session was created.
> > \item If the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit is negotiated,
> the driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto
> requests. Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
> > \item If the VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is
> negotiated, 1) if the driver uses the stateless mode, then the driver MUST set
> the \field{flag} field in struct virtio_crypto_op_header
> > to VIRTIO_CRYPTO_FLAG_STATELESS_MODE and MUST set the fields
> in struct virtio_crypto_hash_para_statelession.sess_para, 2) if the driver
> uses
> the session mode, then the driver MUST set the \field{flag} field in struct
> virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_STATE_MODE.
> > \item The driver MUST set \field{opcode} in struct virtio_crypto_op_header
> > to
> VIRTIO_CRYPTO_HASH.
> > \end{itemize*}
> > """
> >
> > If we drop the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit, and the
> > VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is not negotiated,
> > then the driver doesn't to know use which structure to store the crypto
> request:
> > is struct virtio_crypto_op_data_req_mux ? or struct
> virtio_crypto_op_data_req.
>
> Thanks for the detailed explanation.
>
> Let's clarify some things first:
> 1) struct virtio_crypto_op_data_req_mux IS NOT a C language struct but
> a somewhat informal desciption using C-like syntax. If you don't follow
> try to reason about the size of struct virtio_crypto_op_data_req_mux.
> For instance look at:
> +struct virtio_crypto_cipher_data_req_stateless {
> + /* Device-readable part */
> + struct virtio_crypto_cipher_para_stateless para;
> + /* The cipher key */
> + u8 cipher_key[keylen];
> +
> + /* Initialization Vector or Counter data. */
> + u8 iv[iv_len];
> + /* Source data */
> + u8 src_data[src_data_len]; <==
>
> The isrc_data_len is not a compile time constant!!
>
> +
> + /* Device-writable part */
> + /* Destination data */
> + u8 dst_data[dst_data_len];
> + struct virtio_crypto_inhdr inhdr;
> +};
>
> Using something like BNF to describe the requests would be
> less ambiguous but likely more difficult to read and reason about
> for most of us (and also inconsistent with the rest of the spec).
>
I used to use src_data_gpa/dst_data_gpa to express the data,
But you thought it's not clear, so I changed to use u8[len] like
virtio-scsi device in the virtio spec as your suggested.
struct virtio_scsi_req_cmd {
// Device-readable part
u8 lun[8];
le64 id;
u8 task_attr;
u8 prio;
u8 crn;
u8 cdb[cdb_size];
// The next two fields are only present if VIRTIO_SCSI_F_T10_PI
// is negotiated.
le32 pi_bytesout;
le32 pi_bytesin;
u8 pi_out[pi_bytesout];
u8 dataout[];
// Device-writable part
le32 sense_len;
le32 residual;
le16 status_qualifier;
u8 status;
u8 response;
u8 sense[sense_size];
// The next two fields are only present if VIRTIO_SCSI_F_T10_PI
// is negotiated
u8 pi_in[pi_bytesin];
u8 datain[];
};
> 2) Each struct virtio_crypto_op_data_req is also a struct
> virtio_crypto_op_data_req_mux
> in a sense of 1).
>
> 3) The header struct virtio_crypto_op_header is a common prefix for
> struct virtio_crypto_op_data_req and a struct
> virtio_crypto_op_data_req_mux.
>
Yes.
>
> >
> > We assume the driver uses struct virtio_crypto_op_data_req, what about the
> device?
> > The device doesn't know how to parse the request in the
> virtio_crypto_handle_request()
> > of virito-crypto.c in Qemu.
> >
> > static int
> > virtio_crypto_handle_request(VirtIOCryptoReq *request)
> > {
> > VirtIOCrypto *vcrypto = request->vcrypto;
> > VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > VirtQueueElement *elem = &request->elem;
> > int queue_index =
> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> > struct virtio_crypto_op_data_req req; ???
> >
> > ///or struct virtio_crypto_op_data_req_mux req; ???
> >
>
> Because of 3) and because the service associated with the request can be
> inferred
> form virtio_crypto_op_header one can answer the question you as, and decide
> if what comes after the header is a stateless or a session variant based
> on virtio_crypto_op_header.flag -- maybe it does not even need opcode
> to do that.
>
Yes, you are right. We can get the common header:
static int
virtio_crypto_handle_request(VirtIOCryptoReq *request)
{
...
struct virtio_crypto_op_header header;
iov_to_buf(out_iov, out_num, 0, &header, sizeof(header));
...
Then we check the header.flag:
If (header.flag == VIRTIO_CRYPTO_FLAG_STATELESS_MOD) {
//Use struct virtio_crypto_op_data_req_mux req;
} else if (header.flag == VIRTIO_CRYPTO_FLAG_SESSION_MOD) {
//Use struct virtio_crypto_op_data_req_mux req;
} else {
//Use struct virtio_crypto_op_data_req req;
}
But for existing code we don't use the header.flag to check the request,
Will it break the pre-existing code? Because we don't assume header.flag is
zero before.
> Thus (IMHO) dropping VIRTIO_CRYPTO_F_STATELESS_MODE is possible.
>
> Or is there a flaw in my reasoning?
>
Thanks,
-Gonglei
