On 05/13/2017 03:16 AM, Gonglei (Arei) wrote: > >> 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. > ... > ''' >
As far as I can remember, we have already agreed that in terms of the spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your code you have a substantially different struct virtio_crypto_op_data_req than in your spec! For instance in the spec virtio_crypto_op_data_req is the full request and contains the data buffers (src_data and the dest_data), while in your code it's effectively just a header and does not contain any data buffers. >> 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. > This implies that should we need more than 128 bytes for a request, we will need to introduce jet another request format and negotiate it via feature bits. By the way, I'm having a hard time understing how is the requirement form http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260004 (2.4.4 Message Framing) satisfied by this code. Could you explain this to me please? >> 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. > AFAIU the reason is ease of implementation. If everybody else is fine with this, I won't object either. > > Thanks, > -Gonglei >
