> From: Halil Pasic [mailto:pa...@linux.vnet.ibm.com]
> Sent: Friday, May 12, 2017 7:02 PM
>
>
> On 05/08/2017 01:38 PM, Gonglei wrote:
> > According to the new spec, we should use different
> > requst structure to store the data request based
> > on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
> > negotiated or not.
> >
> > In this patch, we havn't supported stateless mode
> > yet. The device reportes an error if both
> > VIRTIO_CRYPTO_F_MUX_MODE and
> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
> > are negotiated, meanwhile the header.flag doesn't set
> > to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
> >
> > Let's handle this scenario in the following patches.
> >
> > Signed-off-by: Gonglei <arei.gong...@huawei.com>
> > ---
> > hw/virtio/virtio-crypto.c | 83
> ++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 71 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> > index 0353eb6..c4b8a2c 100644
> > --- a/hw/virtio/virtio-crypto.c
> > +++ b/hw/virtio/virtio-crypto.c
> > @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> *request)
> > VirtQueueElement *elem = &request->elem;
> > int queue_index =
> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> > struct virtio_crypto_op_data_req req;
> > + struct virtio_crypto_op_data_req_mux req_mux;
> > int ret;
> > struct iovec *in_iov;
> > struct iovec *out_iov;
> > @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> *request)
> > uint64_t session_id;
> > CryptoDevBackendSymOpInfo *sym_op_info = NULL;
> > Error *local_err = NULL;
> > + bool mux_mode_is_negotiated;
> > + struct virtio_crypto_op_header *header;
> > + bool is_stateless_req = false;
> >
> > if (elem->out_num < 1 || elem->in_num < 1) {
> > virtio_error(vdev, "virtio-crypto dataq missing headers");
> > @@ -597,12 +601,28 @@ virtio_crypto_handle_request(VirtIOCryptoReq
> *request)
> > out_iov = elem->out_sg;
> > in_num = elem->in_num;
> > in_iov = elem->in_sg;
> > - if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> > - != sizeof(req))) {
> > - virtio_error(vdev, "virtio-crypto request outhdr too short");
> > - return -1;
> > +
> > + mux_mode_is_negotiated =
> > + virtio_vdev_has_feature(vdev, VIRTIO_CRYPTO_F_MUX_MODE);
> > + if (!mux_mode_is_negotiated) {
> > + if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
> > + != sizeof(req))) {
> > + virtio_error(vdev, "virtio-crypto request outhdr too short");
> > + return -1;
> > + }
> > + iov_discard_front(&out_iov, &out_num, sizeof(req));
> > +
> > + header = &req.header;
> > + } else {
> > + if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
> > + sizeof(req_mux)) != sizeof(req_mux))) {
> > + virtio_error(vdev, "virtio-crypto request outhdr too short");
> > + return -1;
> > + }
> > + iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
> > +
> > + header = &req_mux.header;
>
> I wonder if this request length checking logic is conform to the
> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
> virtio crypto device specification").
>
Sure. Please see below normative formulation:
'''
\drivernormative{\paragraph}{Symmetric algorithms Operation}{Device Types /
Crypto Device / Device Operation / Symmetric algorithms Operation}
...
\item If the VIRTIO_CRYPTO_F_MUX_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.
...
'''
> AFAIU here you allow only requests of two sizes: one fixed size
> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
> means that some requests need quite some padding between what
> you call the 'request' and the actual data on which the crypto
> operation dictated by the 'request' needs to be performed.
Yes, that's true.
> What are the benefits of this approach?
>
We could unify the request for all algorithms, both symmetric algos and
asymmetric algos,
which is very convenient for handling tens of hundreds of different algorithm
requests.
Thanks,
-Gonglei
