RE: [virtio-dev] RE: [Qemu-devel] [PATCH v18 1/2] virtio-crypto: Add virtio crypto device specification

["Gonglei (Arei)" <arei.gonglei@huawei.com>]

>
> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> Sent: Friday, May 05, 2017 9:52 PM
> 
> On 05/05/2017 05:39 AM, Gonglei (Arei) wrote:
> >>
> >>
> >> On 05/04/2017 04:13 PM, Gonglei (Arei) wrote:
> >>>>
> >>>>
> >>>> On 05/04/2017 03:33 PM, Gonglei (Arei) wrote:
> >>>>>>> +\begin{description}
> >>>>>>> +\item[VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE] Requires
> >>>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>>>> +\item[VIRTIO_CRYPTO_F_HASH_STATELESS_MODE] Requires
> >>>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>>>> +\item[VIRTIO_CRYPTO_F_MAC_STATELESS_MODE] Requires
> >>>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>>>> +\item[VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE] Requires
> >>>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
> >>>>>>> +\end{description}
> >>>>>>
> >>>>>> I find feature bit 0 redundant and bit confusing. We had a discussion
> >>>>>> in v15 and v16.
> >>>>>>
> >>>>>> Could you answer:
> >>>>>>
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03214.html
> >>>>>> (Message-ID:
> >>>> <1fbe30cc-87ec-32bc-4c57-85f9b03b3034@linux.vnet.ibm.com>)
> >>>>>>
> >>>>>>
> >>>>> Please see my reply:
> >>>>> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03186.html
> >>>>>
> >>>>> The main reason is we should keep compatibility to pre-existing driver
> and
> >>>>> support some function that different services have different modes.
> >>>>> We have only one unique crypto request named structure
> >>>> virtio_crypto_op_data_req_mux.
> >>>>> Please continue to see the sepc, you'll find the truth.
> >>>>>
> >>>>
> >>>> Sorry, I still do not understand why can't we drop
> >>>> VIRTIO_CRYPTO_F_STATELESS_MODE
> >>>> and just keep the four service specific modes.
> >>>>
> >>>> Can you point me to the (published) code where
> >>>> VIRTIO_CRYPTO_F_STATELESS_MODE is
> >>>> used (that's what I'm missing -- preferably state the repository, the
> commit
> >>>> a file and a line using VIRTIO_CRYPTO_F_STATELESS_MODE)?
> >>>>
> >>> No no no, there isn't existing code use
> VIRTIO_CRYPTO_F_STATELESS_MODE
> >> yet,
> >>> It's just for future use.
> >>>
> >>
> >> Thanks, that's a crucial piece of information. In the thread I referenced
> >> this was not cleared (at least for me). It would be really nice to have
> >> some working code before doing the spec, because I find it very easy to miss
> >> a detail which renders the whole thing useless if one works solely on
> >> theoretical level and does not try to create at least a working prototype.
> >>
> > I see.
> >
> >>> Please the blow description:
> >>> """
> >>> \drivernormative{\paragraph}{HASH Service Operation}{Device Types /
> Crypto
> >> Device / Device Operation / HASH Service Operation}
> >>>
> >>> \begin{itemize*}
> >>> \item If the driver uses the session mode, then the driver MUST set
> >> \field{session_id} in struct virtio_crypto_op_header
> >>>       to a valid value assigned by the device when the session was
> created.
> >>> \item If the VIRTIO_CRYPTO_F_STATELESS_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.
> >>> \item If the VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is
> >> negotiated, 1) if the driver uses the stateless mode, then the driver MUST
> set
> >> the \field{flag} field in struct virtio_crypto_op_header
> >>>       to VIRTIO_CRYPTO_FLAG_STATELESS_MODE and MUST set the
> fields
> >> in struct virtio_crypto_hash_para_statelession.sess_para, 2) if the driver
> uses
> >> the session mode, then the driver MUST set the \field{flag} field in struct
> >> virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_STATE_MODE.
> >>> \item The driver MUST set \field{opcode} in struct virtio_crypto_op_header
> to
> >> VIRTIO_CRYPTO_HASH.
> >>> \end{itemize*}
> >>> """
> >>>
> >>> If we drop the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit, and the
> >>> VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is not negotiated,
> >>> then the driver doesn't to know use which structure to store the crypto
> >> request:
> >>> is struct virtio_crypto_op_data_req_mux ? or struct
> >> virtio_crypto_op_data_req.
> >>
> >> Thanks for the detailed explanation.
> >>
> >> Let's clarify some things first:
> >> 1) struct virtio_crypto_op_data_req_mux IS NOT a C language struct but
> >> a somewhat informal desciption using C-like syntax. If you don't follow
> >> try to reason about the size of struct virtio_crypto_op_data_req_mux.
> >> For instance look at:
> >> +struct virtio_crypto_cipher_data_req_stateless {
> >> +    /* Device-readable part */
> >> +    struct virtio_crypto_cipher_para_stateless para;
> >> +    /* The cipher key */
> >> +    u8 cipher_key[keylen];
> >> +
> >> +    /* Initialization Vector or Counter data. */
> >> +    u8 iv[iv_len];
> >> +    /* Source data */
> >> +    u8 src_data[src_data_len];           <==
> >>
> >> The  isrc_data_len is not a compile time constant!!
> >>
> >> +
> >> +    /* Device-writable part */
> >> +    /* Destination data */
> >> +    u8 dst_data[dst_data_len];
> >> +    struct virtio_crypto_inhdr inhdr;
> >> +};
> >>
> >> Using something like BNF to describe the requests would be
> >> less ambiguous but likely more difficult to read and reason about
> >> for most of us (and also inconsistent with the rest of the spec).
> >>
> > I used to use src_data_gpa/dst_data_gpa to express the data,
> > But you thought it's not clear, so I changed to use u8[len] like
> > virtio-scsi device in the virtio spec as your suggested.
> >
> [..]
> 
> That's right, I suggested being consistent there, I'm a bit confused
> by this *_gpa stuff though. I guess gpa stands for guest physical
> address, so I guess that would mean transmitting only a 'pointer'
> to the src_data via the virtq as a part of the request instead of
> transmitting a buffer holding the src_data. So IFAIU that would
> be a different protocol and not an alternative description of
> the same protocol. AFAIK your prototype implementation was
> transmitting the data buffer via the virtqueue and not only a pointer
> to it. If that's not the case my proposal was utterly wrong!
> 
Yes, they express a data buffer.

> >> 2) Each struct virtio_crypto_op_data_req is also a struct
> >> virtio_crypto_op_data_req_mux
> >> in a sense of 1).
> >>
> >> 3) The header struct virtio_crypto_op_header is a common prefix for
> >> struct virtio_crypto_op_data_req and a struct
> >> virtio_crypto_op_data_req_mux.
> >>
> > Yes.
> >
> >>
> >>>
> >>> We assume the driver uses struct virtio_crypto_op_data_req, what about
> the
> >> device?
> >>> The device doesn't know how to parse the request in the
> >> virtio_crypto_handle_request()
> >>> of virito-crypto.c in Qemu.
> >>>
> >>> static int
> >>> virtio_crypto_handle_request(VirtIOCryptoReq *request)
> >>> {
> >>>     VirtIOCrypto *vcrypto = request->vcrypto;
> >>>     VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> >>>     VirtQueueElement *elem = &request->elem;
> >>>     int queue_index =
> >> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> >>>     struct virtio_crypto_op_data_req req; ???
> >>>
> >>>     ///or struct virtio_crypto_op_data_req_mux req; ???
> >>>
> >>
> >> Because of 3) and because the service associated with the request can be
> >> inferred
> >> form virtio_crypto_op_header one can answer the question you as, and
> decide
> >> if what comes after the header is a stateless or a session variant based
> >> on virtio_crypto_op_header.flag -- maybe it does not even need opcode
> >> to do that.
> >>
> > Yes, you are right. We can get the common header:
> >
> > static int
> > virtio_crypto_handle_request(VirtIOCryptoReq *request)
> > {
> > ...
> > struct virtio_crypto_op_header header;
> >
> > iov_to_buf(out_iov, out_num, 0, &header, sizeof(header));
> >
> > ...
> >
> > Then we check the header.flag:
> >
> > If (header.flag == VIRTIO_CRYPTO_FLAG_STATELESS_MOD) {
> >   //Use struct virtio_crypto_op_data_req_mux req;
> > } else if (header.flag == VIRTIO_CRYPTO_FLAG_SESSION_MOD) {
> >   //Use struct virtio_crypto_op_data_req_mux req;
> > } else {
> >   //Use struct virtio_crypto_op_data_req req;
> > }
> >
> > But for existing code we don't use the header.flag to check the request,
> > Will it break the pre-existing code? Because we don't assume header.flag is
> > zero before.
> >
> 
> That's a very good question, and it relates well to our discussion about
> the algorithm bits. You say, the problem is that the published QEMU
> implementation does not reject 'unknown' flags (that is requests having
> unknown flags).
> 
> In pure theory, that ain't really a problem, because we can first look at
> the op_code and only check the MOD flags if stateless feature bit was set
> for that op_code (service). That way a (IMHO buggy) driver could continue
> happily writing garbage into flag for session requests for any service if
> it does not negotiate the stateless bit for that service.
> 
> Thus I think omitting VIRTIO_CRYPTO_F_HASH_STATELESS_MODE (using 4
> feature bits for stateless) is theoretically possible. Do you agree?
> 
Yes, I agree.

> But let's approach the problem from the header.flag perspective.
> 
> I see two ways for handling the problem you describe.
> 1) The device MUST ignore header.flag bit's not explicitly activated
> by the presence of some condition involving feature bits, and MUST
> honor activated ones.

It's true.

> 2) We guard the header.flag (and possibly other stuff we forgot to
> handle) must not have unknown bits with a feature bit. That is
> the device MUST reject unknown bits. But even if we do this we still
> need a feature bit based mechanism for adding new known bits. And
> since we have a version out there with no flags defined we would
> need to guard (that is 'activate' from 1)) at least the flags
> introduced together.
> 
> Option 2) from above is probably an over-complication, so we should
> probably go with 1).
> 
> I do not think 1) is correctly described in the spec because we miss MUST
> ignore. This MUST ignore is analogous to the MUST ignore in 'The device
> MUST ignore the used_event value' from 2.4.7.2 Device Requirements:
> Virtqueue Interrupt Suppression. Maybe replacing this MUST with two
> SHOULDs
> is viable too (the driver SHOULD not send garbage and the device SHOULD
> ignore garbage), but I think we need something. Do you agree?
> 
Yes, I agree.

> Even if using just 4 feature bits for stateless is possible it does not
> mean we that's the best way, but VIRTIO_CRYPTO_F_STATELESS_MODE is a
> slightly confusing name given its semantic. If we examine how
> VIRTIO_CRYPTO_F_STATELESS_MODE negotiated affects the protocol, we have
> to conclude that it changes the request format on the data queues: if
> negotiated you have to use _mux if not you must not use _mux. If you use
> _mux you MUST set VIRTIO_CRYPTO_F_STATELESS_MODE for session requests,
> if you do not use _mux you do not (here you == the driver). So it impacts
> session requests too. 

Sorry? A typo? 
s/ VIRTIO_CRYPTO_F_STATELESS_MODE/ VIRTIO_CRYPTO_FLAG_SESSION_MODE/ ?

> My problem other problem with the name is that
> VIRTIO_CRYPTO_F_STATELESS_MODE does not mean we can use stateless
> mode, and that's counter-intuitive for me.
> 
You're right, I agree with you too. So I think we should change the name
to VIRTIO_CRYPTO_F_MUX_MODE, what's your opinion?

> Honestly I have to think about the how formulations impact our ability to
> extend the protocol some more myself. So sorry if I'm generating too much
> noise.
> 
You comments are very useful, so I should say thank you sincerely. :) 

Thanks,
-Gonglei

> Regards,
> Halil
> 
> >
> >> Thus (IMHO) dropping VIRTIO_CRYPTO_F_STATELESS_MODE is possible.
> >>
> >> Or is there a flaw in my reasoning?
> >>
> >
> > Thanks,
> > -Gonglei
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >