Re: [Qemu-devel] [PATCH v2 02/15] crypto: introduce crypto queue handler

2016-09-13 Thread Paolo Bonzini 


On 13/09/2016 11:20, Daniel P. Berrange wrote:
>> > +typedef struct CryptoPacket CryptoPacket;
>> > +typedef struct CryptoQueue CryptoQueue;
>> > +typedef struct CryptoPacketBuf CryptoPacketBuf;
>> > +
>> > +typedef void (CryptoPacketSent) (CryptoClientState *, int);
> As previously, I'd expect naming of
> 
>  QCryptoCryptodevPacket
>  QCryptoCryptodevPacketBuf
>  QCryptoCryptodevQueue
> 

Gonglei,

you are copying a lot of code from network backends.

I am not sure why you would need a queue for virtio-crypto rather than a
direct connection between frontend and backend (and the backend would be
QEMU crypto APIs, like Daniel suggested).

Paolo

Re: [Qemu-devel] [PATCH v2 02/15] crypto: introduce crypto queue handler

2016-09-13 Thread Gonglei (Arei) 
Hi,

All comments are accepted :)


Regards,
-Gonglei


> -Original Message-
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Tuesday, September 13, 2016 5:21 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org; Huangpeng
> (Peter); Luonengjun; m...@redhat.com; stefa...@redhat.com;
> pbonz...@redhat.com; Huangweidong (C); mike.cara...@nxp.com;
> ag...@suse.de; xin.z...@intel.com; Claudio Fontana; nmo...@kalray.eu;
> vincent.jar...@6wind.com
> Subject: Re: [PATCH v2 02/15] crypto: introduce crypto queue handler
> 
> On Tue, Sep 13, 2016 at 11:52:08AM +0800, Gonglei wrote:
> > crypto queue is a gallery used for executing crypto
> > operation, which supports both synchronization and
> > asynchronization. The thoughts stolen from net/queue.c
> >
> > Signed-off-by: Gonglei 
> > ---
> >  crypto/Makefile.objs  |   1 +
> >  crypto/crypto-queue.c | 206
> ++
> >  crypto/crypto.c   |  28 ++
> >  include/crypto/crypto-queue.h |  69 ++
> >  include/crypto/crypto.h   |  12 +++
> >  5 files changed, 316 insertions(+)
> >  create mode 100644 crypto/crypto-queue.c
> >  create mode 100644 include/crypto/crypto-queue.h
> >
> 
> > diff --git a/include/crypto/crypto-queue.h b/include/crypto/crypto-queue.h
> > new file mode 100644
> > index 000..6fba64d
> > --- /dev/null
> > +++ b/include/crypto/crypto-queue.h
> > @@ -0,0 +1,69 @@
> > +/*
> > + * Copyright (c) 2003-2008 Fabrice Bellard
> > + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> > + *
> > + * Authors:
> > + *Gonglei 
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> > + * of this software and associated documentation files (the "Software"), to
> deal
> > + * in the Software without restriction, including without limitation the 
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> 
> Again, wrong license header.
> 
> > +#ifndef QEMU_CRYPTO_QUEUE_H
> > +#define QEMU_CRYPTO_QUEUE_H
> > +
> > +#include "qemu-common.h"
> > +
> > +typedef struct CryptoPacket CryptoPacket;
> > +typedef struct CryptoQueue CryptoQueue;
> > +typedef struct CryptoPacketBuf CryptoPacketBuf;
> > +
> > +typedef void (CryptoPacketSent) (CryptoClientState *, int);
> 
> As previously, I'd expect naming of
> 
>  QCryptoCryptodevPacket
>  QCryptoCryptodevPacketBuf
>  QCryptoCryptodevQueue
> 
> > +
> > +
> > +/* Returns:
> > + *   >0 - success
> > + *0 - queue packet for future redelivery
> > + *   <0 - failure (discard packet)
> > + */
> > +typedef int (CryptoQueueDeliverFunc)(CryptoClientState *sender,
> > + unsigned flags,
> > + void *header_opaque,
> > + void *opaque);
> > +
> > +CryptoQueue *
> > +qemu_new_crypto_queue(CryptoQueueDeliverFunc *deliver, void *opaque);
> > +
> > +void qemu_crypto_queue_cache(CryptoQueue *queue,
> > +   unsigned flags,
> > +   CryptoClientState *sender,
> > +   void *opaque,
> > +   CryptoPacketSent *sent_cb);
> > +
> > +void qemu_del_crypto_queue(CryptoQueue *queue);
> > +
> > +int qemu_crypto_queue_send(CryptoQueue *queue,
> > +unsigned flags,
> > +CryptoClientState *sender,
> > +void *opaque,
> > +CryptoPacketSent *sent_cb);
> > +
> > +void qemu_crypto_queue_purge(CryptoQueue *queue, CryptoClientState
> *from);
> > +bool qemu_crypto_queue_flush(CryptoQueue *queue);
> 
> And naming of
> 
>   qcrypto_cryptodev_queue_purge
>   qcrypto_cryptodev_queue_flush
>   etc.
> 
> Also missing docs for this file
> 
> > +#endif /* QEMU_CRYPTO_QUEUE_H */
> > diff --git a/include/crypto/crypto.h b/include/crypto/crypto.h
> > index f93f6f9..46b3b9e 100644
> > --- a/include/crypto/crypto.h
> > +++

Re: [Qemu-devel] [PATCH v2 02/15] crypto: introduce crypto queue handler

2016-09-13 Thread Daniel P. Berrange 
On Tue, Sep 13, 2016 at 11:52:08AM +0800, Gonglei wrote:
> crypto queue is a gallery used for executing crypto
> operation, which supports both synchronization and
> asynchronization. The thoughts stolen from net/queue.c
> 
> Signed-off-by: Gonglei 
> ---
>  crypto/Makefile.objs  |   1 +
>  crypto/crypto-queue.c | 206 
> ++
>  crypto/crypto.c   |  28 ++
>  include/crypto/crypto-queue.h |  69 ++
>  include/crypto/crypto.h   |  12 +++
>  5 files changed, 316 insertions(+)
>  create mode 100644 crypto/crypto-queue.c
>  create mode 100644 include/crypto/crypto-queue.h
> 

> diff --git a/include/crypto/crypto-queue.h b/include/crypto/crypto-queue.h
> new file mode 100644
> index 000..6fba64d
> --- /dev/null
> +++ b/include/crypto/crypto-queue.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> + *
> + * Authors:
> + *Gonglei 
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */

Again, wrong license header.

> +#ifndef QEMU_CRYPTO_QUEUE_H
> +#define QEMU_CRYPTO_QUEUE_H
> +
> +#include "qemu-common.h"
> +
> +typedef struct CryptoPacket CryptoPacket;
> +typedef struct CryptoQueue CryptoQueue;
> +typedef struct CryptoPacketBuf CryptoPacketBuf;
> +
> +typedef void (CryptoPacketSent) (CryptoClientState *, int);

As previously, I'd expect naming of

 QCryptoCryptodevPacket
 QCryptoCryptodevPacketBuf
 QCryptoCryptodevQueue

> +
> +
> +/* Returns:
> + *   >0 - success
> + *0 - queue packet for future redelivery
> + *   <0 - failure (discard packet)
> + */
> +typedef int (CryptoQueueDeliverFunc)(CryptoClientState *sender,
> + unsigned flags,
> + void *header_opaque,
> + void *opaque);
> +
> +CryptoQueue *
> +qemu_new_crypto_queue(CryptoQueueDeliverFunc *deliver, void *opaque);
> +
> +void qemu_crypto_queue_cache(CryptoQueue *queue,
> +   unsigned flags,
> +   CryptoClientState *sender,
> +   void *opaque,
> +   CryptoPacketSent *sent_cb);
> +
> +void qemu_del_crypto_queue(CryptoQueue *queue);
> +
> +int qemu_crypto_queue_send(CryptoQueue *queue,
> +unsigned flags,
> +CryptoClientState *sender,
> +void *opaque,
> +CryptoPacketSent *sent_cb);
> +
> +void qemu_crypto_queue_purge(CryptoQueue *queue, CryptoClientState *from);
> +bool qemu_crypto_queue_flush(CryptoQueue *queue);

And naming of

  qcrypto_cryptodev_queue_purge
  qcrypto_cryptodev_queue_flush
  etc.

Also missing docs for this file

> +#endif /* QEMU_CRYPTO_QUEUE_H */
> diff --git a/include/crypto/crypto.h b/include/crypto/crypto.h
> index f93f6f9..46b3b9e 100644
> --- a/include/crypto/crypto.h
> +++ b/include/crypto/crypto.h
> @@ -29,6 +29,8 @@
>  
>  #include "qemu/queue.h"
>  #include "qapi-types.h"
> +#include "crypto/crypto-queue.h"
> +
>  
>  typedef void (CryptoPoll)(CryptoClientState *, bool);
>  typedef void (CryptoCleanup) (CryptoClientState *);
> @@ -52,6 +54,8 @@ struct CryptoClientState {
>  char *model;
>  char *name;
>  char info_str[256];
> +CryptoQueue *incoming_queue;
> +unsigned int queue_index;
>  CryptoClientDestructor *destructor;
>  };
>  
> @@ -62,5 +66,13 @@ CryptoClientState *new_crypto_client(CryptoClientInfo 
> *info,
>  CryptoClientState *peer,
>  const char *model,
>  const char *name);
> +int qemu_deliver_crypto_packet(CryptoClientState *sender,
> +

[Qemu-devel] [PATCH v2 02/15] crypto: introduce crypto queue handler

2016-09-12 Thread Gonglei 
crypto queue is a gallery used for executing crypto
operation, which supports both synchronization and
asynchronization. The thoughts stolen from net/queue.c

Signed-off-by: Gonglei 
---
 crypto/Makefile.objs  |   1 +
 crypto/crypto-queue.c | 206 ++
 crypto/crypto.c   |  28 ++
 include/crypto/crypto-queue.h |  69 ++
 include/crypto/crypto.h   |  12 +++
 5 files changed, 316 insertions(+)
 create mode 100644 crypto/crypto-queue.c
 create mode 100644 include/crypto/crypto-queue.h

diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index 2a63cb8..652b429 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -27,6 +27,7 @@ crypto-obj-y += block.o
 crypto-obj-y += block-qcow.o
 crypto-obj-y += block-luks.o
 crypto-obj-y += crypto.o
+crypto-obj-y += crypto-queue.o
 
 # Let the userspace emulators avoid linking gnutls/etc
 crypto-aes-obj-y = aes.o
diff --git a/crypto/crypto-queue.c b/crypto/crypto-queue.c
new file mode 100644
index 000..3e91be9
--- /dev/null
+++ b/crypto/crypto-queue.c
@@ -0,0 +1,206 @@
+/*
+ * Queue management for crypto device (based on net/qeueu.c)
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ * Authors:
+ *Gonglei 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "crypto/crypto-queue.h"
+#include "crypto/crypto.h"
+#include "qemu/queue.h"
+
+
+/* The delivery handler may only return zero if it will call
+ * qemu_crypto_queue_flush() when it determines that it is once again able
+ * to deliver packets. It must also call qemu_crypto_queue_purge() in its
+ * cleanup path.
+ *
+ * If a sent callback is provided to send(), the caller must handle a
+ * zero return from the delivery handler by not sending any more packets
+ * until we have invoked the callback. Only in that case will we queue
+ * the packet.
+ *
+ * If a sent callback isn't provided, we just drop the packet to avoid
+ * unbounded queueing.
+ */
+
+struct CryptoPacket {
+QTAILQ_ENTRY(CryptoPacket) entry;
+CryptoClientState *sender;
+unsigned flags; /* algorithms' type etc. */
+CryptoPacketSent *sent_cb; /* callback after packet sent */
+void *opaque; /* header struct pointer of operation */
+uint8_t data[0];
+};
+
+struct CryptoQueue {
+void *opaque;
+uint32_t nq_maxlen;
+uint32_t nq_count;
+CryptoQueueDeliverFunc *deliver;
+
+QTAILQ_HEAD(packets, CryptoPacket) packets;
+
+unsigned delivering:1;
+};
+
+CryptoQueue *
+qemu_new_crypto_queue(CryptoQueueDeliverFunc *deliver, void *opaque)
+{
+CryptoQueue *queue;
+
+queue = g_new0(CryptoQueue, 1);
+
+queue->opaque = opaque;
+queue->nq_maxlen = 1;
+queue->nq_count = 0;
+queue->deliver = deliver;
+
+QTAILQ_INIT(>packets);
+
+queue->delivering = 0;
+
+return queue;
+}
+
+void qemu_del_crypto_queue(CryptoQueue *queue)
+{
+CryptoPacket *packet, *next;
+
+QTAILQ_FOREACH_SAFE(packet, >packets, entry, next) {
+QTAILQ_REMOVE(>packets, packet, entry);
+g_free(packet->opaque);
+g_free(packet);
+}
+
+g_free(queue);
+}
+
+void qemu_crypto_queue_cache(CryptoQueue *queue,
+   unsigned flags,
+   CryptoClientState *sender,
+   void *opaque,
+   CryptoPacketSent *sent_cb)
+{
+CryptoPacket *packet;
+
+if (queue->nq_count >= queue->nq_maxlen && !sent_cb) {
+return; /* drop if queue full and no callback */
+}
+
+packet = g_malloc(sizeof(CryptoPacket));
+packet->sender = sender;
+packet->sent_cb = sent_cb;
+packet->flags = flags,
+packet->opaque = opaque;
+
+queue->nq_count++;
+QTAILQ_INSERT_TAIL(>packets, packet, entry);
+}
+
+static