On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
>> From: Halil Pasic [mailto:pa...@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: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
>>> 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.
