Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Gonglei (Arei)]

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