| Message ID | 1494243504-127980-7-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
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.gonglei@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"). 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. What are the benefits of this approach? Regards, Halil
> From: Halil Pasic [mailto:pasic@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.gonglei@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
On 05/13/2017 03:16 AM, Gonglei (Arei) wrote: > >> From: Halil Pasic [mailto:pasic@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.gonglei@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 >
> > On 05/13/2017 03:16 AM, Gonglei (Arei) wrote: > > > >> From: Halil Pasic [mailto:pasic@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.gonglei@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 Sorry, I don't think so. :( > 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. > I said struct virtio_crypto_op_data_req in the spec is just a symbol. I didn't find a better way to express the src_data and dst_data etc. So I used u8[len] xxx_data to occupy a sit in the request. > > >> 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. > Why do we need other 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-260 > 004 > (2.4.4 Message Framing) satisfied by this code. Could you explain this > to me please? > Sorry for my bad English, I don't know which normative formulation the code violates? Thanks -Gonglei > > >> 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 > >
On 05/16/2017 04:52 AM, Gonglei (Arei) wrote: >> >> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote: >>> >>>> From: Halil Pasic [mailto:pasic@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.gonglei@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 > > Sorry, I don't think so. :( > >> 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. >> > I said struct virtio_crypto_op_data_req in the spec is just a symbol. > I didn't find a better way to express the src_data and dst_data etc. So > I used u8[len] xxx_data to occupy a sit in the request. > OK, tell me how is the reader/implementer of the spec supposed to figure out that a 124 byte padded "header" needs to be precede any "data"? Besides if you look at +Stateless mode HASH service requests are as follows: + +\begin{lstlisting} +struct virtio_crypto_hash_para_statelesss { + struct { + /* See VIRTIO_CRYPTO_HASH_* above */ + le32 algo; + } sess_para; + + /* length of source data */ + le32 src_data_len; + /* hash result length */ + le32 hash_result_len; + le32 reserved; +}; +struct virtio_crypto_hash_data_req_stateless { + /* Device-readable part */ + struct virtio_crypto_hash_para_stateless para; + /* Source data */ + u8 src_data[src_data_len]; + + /* Device-writable part */ + /* Hash result data */ + u8 hash_result[hash_result_len]; + struct virtio_crypto_inhdr inhdr; +}; +\end{lstlisting} + from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device specification". You need the padding to 128 bytes after virtio_crypto_hash_para_stateless para, but before src_data. But there is no indication in the definition of virtio_crypto_hash_data_req_stateless nor somewhere in the spec about that. On the contrary based on this one would expect para.reserved and src_data being pretty adjecent. Thus the normative statement you quoted is (IMHO) ill suited and insufficient to express what you have been trying to express. >> >>>> 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. >> > Why do we need other feature bits? Because assume you have something that needs more that 128 bytes for its request, how do you solve the problem without new format end new feature bit? > >> 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-260 >> 004 >> (2.4.4 Message Framing) satisfied by this code. Could you explain this >> to me please? >> > Sorry for my bad English, > I don't know which normative formulation the code violates? I'm not sure it's actually violated, but I'm concerned about the following normative statement: "The device MUST NOT make assumptions about the particular arrangement of descriptors. The device MAY have a reasonable limit of descriptors it will allow in a chain." Please also read the explanatory part I've linked, because the normative statement is pretty abstract. In my understanding, the spec says, that e.g. the virti-crypto device should not fail if a single request is split up into let's say two chunks and transmitted by the means of two top level descriptors. Do you agree with my reading of the spec? What does the virtio-crypto device do if it encounters such a situation? Regards, Halil
> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com] > Sent: Wednesday, May 17, 2017 6:18 AM > > > On 05/16/2017 04:52 AM, Gonglei (Arei) wrote: > >> > >> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote: > >>> > >>>> From: Halil Pasic [mailto:pasic@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.gonglei@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 > > > > Sorry, I don't think so. :( > > > >> 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. > >> > > I said struct virtio_crypto_op_data_req in the spec is just a symbol. > > I didn't find a better way to express the src_data and dst_data etc. So > > I used u8[len] xxx_data to occupy a sit in the request. > > > > OK, tell me how is the reader/implementer of the spec supposed to figure > out that a 124 byte padded "header" needs to be precede any "data"? > If those people use the asked request based on the spec, they don't need to worry about the pad IMHO. > Besides if you look at > > +Stateless mode HASH service requests are as follows: > + > +\begin{lstlisting} > +struct virtio_crypto_hash_para_statelesss { > + struct { > + /* See VIRTIO_CRYPTO_HASH_* above */ > + le32 algo; > + } sess_para; > + > + /* length of source data */ > + le32 src_data_len; > + /* hash result length */ > + le32 hash_result_len; > + le32 reserved; > +}; > +struct virtio_crypto_hash_data_req_stateless { > + /* Device-readable part */ > + struct virtio_crypto_hash_para_stateless para; > + /* Source data */ > + u8 src_data[src_data_len]; > + > + /* Device-writable part */ > + /* Hash result data */ > + u8 hash_result[hash_result_len]; > + struct virtio_crypto_inhdr inhdr; > +}; > +\end{lstlisting} > + > > from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device > specification". You need the padding to 128 bytes after > virtio_crypto_hash_para_stateless para, but before src_data. But there is > no indication in the definition of virtio_crypto_hash_data_req_stateless > nor somewhere in the spec about that. On the contrary based on this > one would expect para.reserved and src_data being pretty adjecent. > > Thus the normative statement you quoted is (IMHO) ill suited and > insufficient to express what you have been trying to express. > That's indeed a problem. I can't find a good way to express my thoughts in the spec. Make me sad.~ > >> > >>>> 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. > >> > > Why do we need other feature bits? > > Because assume you have something that needs more that 128 bytes for > its request, how do you solve the problem without new format end new > feature bit? > Oh, maybe I get your point now. You mean the future use for some algorithm requests use more than 128 bytes? If so, we have to introduce new feature bits. AFAICT the 128 bytes is enough for sym and asym algorithms currently. Xin Zeng? Any thoughts? > > > >> 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-260 > >> 004 > >> (2.4.4 Message Framing) satisfied by this code. Could you explain this > >> to me please? > >> > > Sorry for my bad English, > > I don't know which normative formulation the code violates? > > I'm not sure it's actually violated, but I'm concerned about the following > normative statement: "The device MUST NOT make assumptions about the > particular > arrangement of descriptors. The device MAY have a reasonable limit of > descriptors it will allow in a chain." > > Please also read the explanatory part I've linked, because the normative > statement is pretty abstract. > > In my understanding, the spec says, that e.g. the virti-crypto device > should not fail if a single request is split up into let's say two chunks > and transmitted by the means of two top level descriptors. > > Do you agree with my reading of the spec? > Yes, I agree. But what's the relationship about the request here? We don't assume the request have to use one desc entity, it can use scatter-gather list for one request header. The device can cover the situation in the QEMU. > What does the virtio-crypto device do if it encounters such a situation? > This isn't a problem. Pls see blow code segment: virtio_crypto_handle_request() {... 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)); ... Thanks, -Gonglei
On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: >>>> 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-260 >>>> 004 >>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this >>>> to me please? >>>> >>> Sorry for my bad English, >>> I don't know which normative formulation the code violates? >> I'm not sure it's actually violated, but I'm concerned about the following >> normative statement: "The device MUST NOT make assumptions about the >> particular >> arrangement of descriptors. The device MAY have a reasonable limit of >> descriptors it will allow in a chain." >> >> Please also read the explanatory part I've linked, because the normative >> statement is pretty abstract. >> >> In my understanding, the spec says, that e.g. the virti-crypto device >> should not fail if a single request is split up into let's say two chunks >> and transmitted by the means of two top level descriptors. >> >> Do you agree with my reading of the spec? >> > Yes, I agree. But what's the relationship about the request here? > We don't assume the request have to use one desc entity, it can > use scatter-gather list for one request header. > The device can cover the situation in the QEMU. > >> What does the virtio-crypto device do if it encounters such a situation? >> > This isn't a problem. Pls see blow code segment: > > virtio_crypto_handle_request() > {... > 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)); > ... > Thats exactly what worries me. I see a call to virtio_error there... void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...) { va_list ap; va_start(ap, fmt); error_vreport(fmt, ap); va_end(ap); vdev->broken = true; if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET); virtio_notify_config(vdev); } } This however seems to make the device 'broken'. Or am I missing something? Halil > > Thanks, > -Gonglei >
> > On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: > >>>> 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-260 > >>>> 004 > >>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this > >>>> to me please? > >>>> > >>> Sorry for my bad English, > >>> I don't know which normative formulation the code violates? > >> I'm not sure it's actually violated, but I'm concerned about the following > >> normative statement: "The device MUST NOT make assumptions about the > >> particular > >> arrangement of descriptors. The device MAY have a reasonable limit of > >> descriptors it will allow in a chain." > >> > >> Please also read the explanatory part I've linked, because the normative > >> statement is pretty abstract. > >> > >> In my understanding, the spec says, that e.g. the virti-crypto device > >> should not fail if a single request is split up into let's say two chunks > >> and transmitted by the means of two top level descriptors. > >> > >> Do you agree with my reading of the spec? > >> > > Yes, I agree. But what's the relationship about the request here? > > We don't assume the request have to use one desc entity, it can > > use scatter-gather list for one request header. > > The device can cover the situation in the QEMU. > > > >> What does the virtio-crypto device do if it encounters such a situation? > >> > > This isn't a problem. Pls see blow code segment: > > > > virtio_crypto_handle_request() > > {... > > 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)); > > ... > > > > Thats exactly what worries me. I see a call to virtio_error there... > > > void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...) > { > va_list ap; > > va_start(ap, fmt); > error_vreport(fmt, ap); > va_end(ap); > > vdev->broken = true; > > if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { > virtio_set_status(vdev, vdev->status | > VIRTIO_CONFIG_S_NEEDS_RESET); > virtio_notify_config(vdev); > } > } > > This however seems to make the device 'broken'. Or am I missing something? > Yes, if the parse process failed, the device will broke. But This is a exception scenario IMHO, which is the same situation with other virtio devices. Stefan introduced the virtio_error(). Thanks, -Gonglei
On 05/17/2017 12:13 PM, Gonglei (Arei) wrote: >> >> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: >>>>>> 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-260 >>>>>> 004 >>>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this >>>>>> to me please? >>>>>> >>>>> Sorry for my bad English, >>>>> I don't know which normative formulation the code violates? >>>> I'm not sure it's actually violated, but I'm concerned about the following >>>> normative statement: "The device MUST NOT make assumptions about the >>>> particular >>>> arrangement of descriptors. The device MAY have a reasonable limit of >>>> descriptors it will allow in a chain." >>>> >>>> Please also read the explanatory part I've linked, because the normative >>>> statement is pretty abstract. >>>> >>>> In my understanding, the spec says, that e.g. the virti-crypto device >>>> should not fail if a single request is split up into let's say two chunks >>>> and transmitted by the means of two top level descriptors. >>>> >>>> Do you agree with my reading of the spec? >>>> >>> Yes, I agree. But what's the relationship about the request here? >>> We don't assume the request have to use one desc entity, it can >>> use scatter-gather list for one request header. >>> The device can cover the situation in the QEMU. >>> >>>> What does the virtio-crypto device do if it encounters such a situation? >>>> >>> This isn't a problem. Pls see blow code segment: >>> >>> virtio_crypto_handle_request() >>> {... >>> 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)); >>> ... >>> >> >> Thats exactly what worries me. I see a call to virtio_error there... >> >> >> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...) >> { >> va_list ap; >> >> va_start(ap, fmt); >> error_vreport(fmt, ap); >> va_end(ap); >> >> vdev->broken = true; >> >> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { >> virtio_set_status(vdev, vdev->status | >> VIRTIO_CONFIG_S_NEEDS_RESET); >> virtio_notify_config(vdev); >> } >> } >> >> This however seems to make the device 'broken'. Or am I missing something? >> > Yes, if the parse process failed, the device will broke. > But This is a exception scenario IMHO, which is the same situation > with other virtio devices. I know that virtio-blk does the same. I'm not sure my reading of the spec is correct. Maybe Stefan, Michael or Connie can clarify this for us! By the way for virtio-blk the current handling was introduced by commit 20ea686a0 (by Greg Kurz), but before we were failing even harder. Regards, Halil > > Stefan introduced the virtio_error(). > > Thanks, > -Gonglei >
On Wed, 17 May 2017 12:33:20 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 05/17/2017 12:13 PM, Gonglei (Arei) wrote: > >> > >> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: > >>>>>> 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-260 > >>>>>> 004 > >>>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this > >>>>>> to me please? > >>>>>> > >>>>> Sorry for my bad English, > >>>>> I don't know which normative formulation the code violates? > >>>> I'm not sure it's actually violated, but I'm concerned about the following > >>>> normative statement: "The device MUST NOT make assumptions about the > >>>> particular > >>>> arrangement of descriptors. The device MAY have a reasonable limit of > >>>> descriptors it will allow in a chain." > >>>> > >>>> Please also read the explanatory part I've linked, because the normative > >>>> statement is pretty abstract. > >>>> > >>>> In my understanding, the spec says, that e.g. the virti-crypto device > >>>> should not fail if a single request is split up into let's say two chunks > >>>> and transmitted by the means of two top level descriptors. > >>>> > >>>> Do you agree with my reading of the spec? > >>>> > >>> Yes, I agree. But what's the relationship about the request here? > >>> We don't assume the request have to use one desc entity, it can > >>> use scatter-gather list for one request header. > >>> The device can cover the situation in the QEMU. > >>> > >>>> What does the virtio-crypto device do if it encounters such a situation? > >>>> > >>> This isn't a problem. Pls see blow code segment: > >>> > >>> virtio_crypto_handle_request() > >>> {... > >>> 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)); > >>> ... > >>> > >> > >> Thats exactly what worries me. I see a call to virtio_error there... > >> > >> > >> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...) > >> { > >> va_list ap; > >> > >> va_start(ap, fmt); > >> error_vreport(fmt, ap); > >> va_end(ap); > >> > >> vdev->broken = true; > >> > >> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { > >> virtio_set_status(vdev, vdev->status | > >> VIRTIO_CONFIG_S_NEEDS_RESET); > >> virtio_notify_config(vdev); > >> } > >> } > >> > >> This however seems to make the device 'broken'. Or am I missing something? > >> > > Yes, if the parse process failed, the device will broke. > > But This is a exception scenario IMHO, which is the same situation > > with other virtio devices. > > I know that virtio-blk does the same. I'm not sure my reading of the > spec is correct. Maybe Stefan, Michael or Connie can clarify this > for us! The spec says for NEEDS_RESET: "Indicates that the device has experienced an error from which it can't recover." For me, this means: - If this is something that might happen in the normal course of events and there's a good recovery path, just recover. - Else, use virtio_error() and require the driver to recover via reset. If the driver is sending requests in an unexpected format, I'd use virtio_error(). The format needs to be properly described, though. > > By the way for virtio-blk the current handling was introduced by > commit 20ea686a0 (by Greg Kurz), but before we were failing even > harder. > > Regards, > Halil > > > > > Stefan introduced the virtio_error(). > > > > Thanks, > > -Gonglei > >
On 05/17/2017 01:10 PM, Cornelia Huck wrote: > On Wed, 17 May 2017 12:33:20 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 05/17/2017 12:13 PM, Gonglei (Arei) wrote: >>>> >>>> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: >>>>>>>> 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-260 >>>>>>>> 004 >>>>>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this >>>>>>>> to me please? >>>>>>>> >>>>>>> Sorry for my bad English, >>>>>>> I don't know which normative formulation the code violates? >>>>>> I'm not sure it's actually violated, but I'm concerned about the following >>>>>> normative statement: "The device MUST NOT make assumptions about the >>>>>> particular >>>>>> arrangement of descriptors. The device MAY have a reasonable limit of >>>>>> descriptors it will allow in a chain." >>>>>> >>>>>> Please also read the explanatory part I've linked, because the normative >>>>>> statement is pretty abstract. >>>>>> >>>>>> In my understanding, the spec says, that e.g. the virti-crypto device >>>>>> should not fail if a single request is split up into let's say two chunks >>>>>> and transmitted by the means of two top level descriptors. >>>>>> >>>>>> Do you agree with my reading of the spec? >>>>>> >>>>> Yes, I agree. But what's the relationship about the request here? >>>>> We don't assume the request have to use one desc entity, it can >>>>> use scatter-gather list for one request header. >>>>> The device can cover the situation in the QEMU. >>>>> >>>>>> What does the virtio-crypto device do if it encounters such a situation? >>>>>> >>>>> This isn't a problem. Pls see blow code segment: >>>>> >>>>> virtio_crypto_handle_request() >>>>> {... >>>>> 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)); >>>>> ... >>>>> >>>> >>>> Thats exactly what worries me. I see a call to virtio_error there... >>>> >>>> >>>> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...) >>>> { >>>> va_list ap; >>>> >>>> va_start(ap, fmt); >>>> error_vreport(fmt, ap); >>>> va_end(ap); >>>> >>>> vdev->broken = true; >>>> >>>> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { >>>> virtio_set_status(vdev, vdev->status | >>>> VIRTIO_CONFIG_S_NEEDS_RESET); >>>> virtio_notify_config(vdev); >>>> } >>>> } >>>> >>>> This however seems to make the device 'broken'. Or am I missing something? >>>> >>> Yes, if the parse process failed, the device will broke. >>> But This is a exception scenario IMHO, which is the same situation >>> with other virtio devices. >> >> I know that virtio-blk does the same. I'm not sure my reading of the >> spec is correct. Maybe Stefan, Michael or Connie can clarify this >> for us! > > The spec says for NEEDS_RESET: > > "Indicates that the device has experienced an error from which it can't > recover." > > For me, this means: > - If this is something that might happen in the normal course of events > and there's a good recovery path, just recover. > - Else, use virtio_error() and require the driver to recover via reset. > > If the driver is sending requests in an unexpected format, I'd use > virtio_error(). The format needs to be properly described, though. > All-clear, my bad. After re-reading the spec and virtio_pop I have realized that virtio_pop pops full descriptor chains. These however correspond to single requests. Thus at the given point the full request has to be placed into the queue. Sorry! >> >> By the way for virtio-blk the current handling was introduced by >> commit 20ea686a0 (by Greg Kurz), but before we were failing even >> harder. >> >> Regards, >> Halil >> >>> >>> Stefan introduced the virtio_error(). >>> >>> Thanks, >>> -Gonglei >>> > >
On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: >> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com] >> Sent: Wednesday, May 17, 2017 6:18 AM >> >> >> On 05/16/2017 04:52 AM, Gonglei (Arei) wrote: >>>> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote: >>>>>> From: Halil Pasic [mailto:pasic@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.gonglei@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 >>> Sorry, I don't think so. :( >>> >>>> 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. >>>> >>> I said struct virtio_crypto_op_data_req in the spec is just a symbol. >>> I didn't find a better way to express the src_data and dst_data etc. So >>> I used u8[len] xxx_data to occupy a sit in the request. >>> >> OK, tell me how is the reader/implementer of the spec supposed to figure >> out that a 124 byte padded "header" needs to be precede any "data"? >> > If those people use the asked request based on the spec, > they don't need to worry about the pad IMHO. > Is this comment of yours outdated? I have described below why I think there are problems, and below you seem to agree... >> Besides if you look at >> >> +Stateless mode HASH service requests are as follows: >> + >> +\begin{lstlisting} >> +struct virtio_crypto_hash_para_statelesss { >> + struct { >> + /* See VIRTIO_CRYPTO_HASH_* above */ >> + le32 algo; >> + } sess_para; >> + >> + /* length of source data */ >> + le32 src_data_len; >> + /* hash result length */ >> + le32 hash_result_len; >> + le32 reserved; >> +}; >> +struct virtio_crypto_hash_data_req_stateless { >> + /* Device-readable part */ >> + struct virtio_crypto_hash_para_stateless para; >> + /* Source data */ >> + u8 src_data[src_data_len]; >> + >> + /* Device-writable part */ >> + /* Hash result data */ >> + u8 hash_result[hash_result_len]; >> + struct virtio_crypto_inhdr inhdr; >> +}; >> +\end{lstlisting} >> + >> >> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device >> specification". You need the padding to 128 bytes after >> virtio_crypto_hash_para_stateless para, but before src_data. But there is >> no indication in the definition of virtio_crypto_hash_data_req_stateless >> nor somewhere in the spec about that. On the contrary based on this >> one would expect para.reserved and src_data being pretty adjecent. >> >> Thus the normative statement you quoted is (IMHO) ill suited and >> insufficient to express what you have been trying to express. >> > That's indeed a problem. I can't find a good way to express my thoughts > in the spec. Make me sad.~ > Can't really tell if we are in an agreement based on your reply above. If we are not, please tell. If we are we have two paths: 1) Give up on the concept of same 'header' length. You could easily branch on the common header and do everything else algorithm specific. 2) Find a way to clean up the mess: * Bring to expression that the struct virtio_crypto_op_data_req from the code ain't the full request (e.g. by postfix-ing _header). Same for mux. * Update the description in the spec so that it's compatible with what your implementations are doing. >>>>>> 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. >>>> >>> Why do we need other feature bits? >> Because assume you have something that needs more that 128 bytes for >> its request, how do you solve the problem without new format end new >> feature bit? >> > Oh, maybe I get your point now. You mean the future use for some algorithm requests > use more than 128 bytes? If so, we have to introduce new feature bits. > AFAICT the 128 bytes is enough for sym and asym algorithms currently. Xin Zeng? Any thoughts? > That's what I was trying to say.
> > On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: > >> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com] > >> Sent: Wednesday, May 17, 2017 6:18 AM > >> > >> > >> On 05/16/2017 04:52 AM, Gonglei (Arei) wrote: > >>>> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote: > >>>>>> From: Halil Pasic [mailto:pasic@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.gonglei@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 > >>> Sorry, I don't think so. :( > >>> > >>>> 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. > >>>> > >>> I said struct virtio_crypto_op_data_req in the spec is just a symbol. > >>> I didn't find a better way to express the src_data and dst_data etc. So > >>> I used u8[len] xxx_data to occupy a sit in the request. > >>> > >> OK, tell me how is the reader/implementer of the spec supposed to figure > >> out that a 124 byte padded "header" needs to be precede any "data"? > >> > > If those people use the asked request based on the spec, > > they don't need to worry about the pad IMHO. > > > > Is this comment of yours outdated? I have described below why I think > there are problems, and below you seem to agree... > Yes. > >> Besides if you look at > >> > >> +Stateless mode HASH service requests are as follows: > >> + > >> +\begin{lstlisting} > >> +struct virtio_crypto_hash_para_statelesss { > >> + struct { > >> + /* See VIRTIO_CRYPTO_HASH_* above */ > >> + le32 algo; > >> + } sess_para; > >> + > >> + /* length of source data */ > >> + le32 src_data_len; > >> + /* hash result length */ > >> + le32 hash_result_len; > >> + le32 reserved; > >> +}; > >> +struct virtio_crypto_hash_data_req_stateless { > >> + /* Device-readable part */ > >> + struct virtio_crypto_hash_para_stateless para; > >> + /* Source data */ > >> + u8 src_data[src_data_len]; > >> + > >> + /* Device-writable part */ > >> + /* Hash result data */ > >> + u8 hash_result[hash_result_len]; > >> + struct virtio_crypto_inhdr inhdr; > >> +}; > >> +\end{lstlisting} > >> + > >> > >> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device > >> specification". You need the padding to 128 bytes after > >> virtio_crypto_hash_para_stateless para, but before src_data. But there is > >> no indication in the definition of virtio_crypto_hash_data_req_stateless > >> nor somewhere in the spec about that. On the contrary based on this > >> one would expect para.reserved and src_data being pretty adjecent. > >> > >> Thus the normative statement you quoted is (IMHO) ill suited and > >> insufficient to express what you have been trying to express. > >> > > That's indeed a problem. I can't find a good way to express my thoughts > > in the spec. Make me sad.~ > > > > Can't really tell if we are in an agreement based on your reply above. > If we are not, please tell. > > If we are we have two paths: > 1) Give up on the concept of same 'header' length. You could easily > branch on the common header and do everything else algorithm specific. > 2) Find a way to clean up the mess: > * Bring to expression that the struct virtio_crypto_op_data_req > from the code ain't the full request (e.g. by postfix-ing _header). > Same for mux. > * Update the description in the spec so that it's compatible with > what your implementations are doing. > Could you pls explain more about those two ways? Oh give me an example for each other. Which way do you like better? Thanks, -Gonglei > >>>>>> 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. > >>>> > >>> Why do we need other feature bits? > >> Because assume you have something that needs more that 128 bytes for > >> its request, how do you solve the problem without new format end new > >> feature bit? > >> > > Oh, maybe I get your point now. You mean the future use for some algorithm > requests > > use more than 128 bytes? If so, we have to introduce new feature bits. > > AFAICT the 128 bytes is enough for sym and asym algorithms currently. Xin > Zeng? Any thoughts? > > > > That's what I was trying to say.
On 05/18/2017 03:21 PM, Gonglei (Arei) wrote: >>>> Besides if you look at >>>> >>>> +Stateless mode HASH service requests are as follows: >>>> + >>>> +\begin{lstlisting} >>>> +struct virtio_crypto_hash_para_statelesss { >>>> + struct { >>>> + /* See VIRTIO_CRYPTO_HASH_* above */ >>>> + le32 algo; >>>> + } sess_para; >>>> + >>>> + /* length of source data */ >>>> + le32 src_data_len; >>>> + /* hash result length */ >>>> + le32 hash_result_len; >>>> + le32 reserved; >>>> +}; >>>> +struct virtio_crypto_hash_data_req_stateless { >>>> + /* Device-readable part */ >>>> + struct virtio_crypto_hash_para_stateless para; >>>> + /* Source data */ >>>> + u8 src_data[src_data_len]; >>>> + >>>> + /* Device-writable part */ >>>> + /* Hash result data */ >>>> + u8 hash_result[hash_result_len]; >>>> + struct virtio_crypto_inhdr inhdr; >>>> +}; >>>> +\end{lstlisting} >>>> + >>>> >>>> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device >>>> specification". You need the padding to 128 bytes after >>>> virtio_crypto_hash_para_stateless para, but before src_data. But there is >>>> no indication in the definition of virtio_crypto_hash_data_req_stateless >>>> nor somewhere in the spec about that. On the contrary based on this >>>> one would expect para.reserved and src_data being pretty adjecent. >>>> >>>> Thus the normative statement you quoted is (IMHO) ill suited and >>>> insufficient to express what you have been trying to express. >>>> >>> That's indeed a problem. I can't find a good way to express my thoughts >>> in the spec. Make me sad.~ >>> >> Can't really tell if we are in an agreement based on your reply above. >> If we are not, please tell. >> >> If we are we have two paths: >> 1) Give up on the concept of same 'header' length. You could easily >> branch on the common header and do everything else algorithm specific. >> 2) Find a way to clean up the mess: >> * Bring to expression that the struct virtio_crypto_op_data_req >> from the code ain't the full request (e.g. by postfix-ing _header). >> Same for mux. >> * Update the description in the spec so that it's compatible with >> what your implementations are doing. >> > Could you pls explain more about those two ways? Oh give me an example > for each other. Which way do you like better? Sorry, I'm quite busy at the time and it does not look like I will be able to find the time for providing a more detailed explanation. Regarding my preferences I think 1) is the more straight-forward approach at least conceptually. OTOH I would need to implement 1) to verify that... Regards, Halil
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; } - iov_discard_front(&out_iov, &out_num, sizeof(req)); if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_crypto_inhdr)) { @@ -623,16 +643,51 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request) request->in_num = in_num; request->in_iov = in_iov; - opcode = ldl_le_p(&req.header.opcode); - session_id = ldq_le_p(&req.header.session_id); + opcode = ldl_le_p(&header->opcode); switch (opcode) { case VIRTIO_CRYPTO_CIPHER_ENCRYPT: case VIRTIO_CRYPTO_CIPHER_DECRYPT: - ret = virtio_crypto_handle_sym_req(vcrypto, + if (!mux_mode_is_negotiated) { + ret = virtio_crypto_handle_sym_req(vcrypto, &req.u.sym_req, &sym_op_info, out_iov, out_num); + } else { + if (!virtio_vdev_has_feature(vdev, + VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE)) { + /* + * If the VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE is not + * negotiated, the driver MUST use the session mode + */ + ret = virtio_crypto_handle_sym_req(vcrypto, + &req_mux.u.sym_req.data, + &sym_op_info, + out_iov, out_num); + } else { + /* + * If the VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE is + * negotiated, the device MUST parse header.flag + * in order to decide which mode the driver uses. + */ + if (header->flag == VIRTIO_CRYPTO_FLAG_SESSION_MODE) { + ret = virtio_crypto_handle_sym_req(vcrypto, + &req_mux.u.sym_req.data, + &sym_op_info, + out_iov, out_num); + } else { + is_stateless_req = true; + /* + * Handle stateless mode, that is + * header->flag == VIRTIO_CRYPTO_FLAG_STATELESS_MODE + */ + virtio_error(vdev, + "virtio-crypto do not support stateless mode"); + return -1; + } + } + } + /* Serious errors, need to reset virtio crypto device */ if (ret == -EFAULT) { return -1; @@ -640,11 +695,15 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request) virtio_crypto_req_complete(request, VIRTIO_CRYPTO_NOTSUPP); virtio_crypto_free_request(request); } else { - sym_op_info->session_id = session_id; + if (!is_stateless_req) { + sym_op_info->op_code = opcode; + session_id = ldq_le_p(&header->session_id); + sym_op_info->session_id = session_id; + /* Set request's parameter */ + request->flags = CRYPTODEV_BACKEND_ALG_SYM; + request->u.sym_op_info = sym_op_info; + } - /* Set request's parameter */ - request->flags = CRYPTODEV_BACKEND_ALG_SYM; - request->u.sym_op_info = sym_op_info; ret = cryptodev_backend_crypto_operation(vcrypto->cryptodev, request, queue_index, &local_err); if (ret < 0) {
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.gonglei@huawei.com> --- hw/virtio/virtio-crypto.c | 83 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 71 insertions(+), 12 deletions(-)