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

[Gonglei (Arei)]

> 
> 
> 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:
> >> <address@hidden>)
> >>>>
> >>>>
> >>> 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