>
> 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...
>
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.
