RE: [virtio-dev] Re: [PATCH v1 11/14] virtio-crypto: add control queue handler

[Gonglei (Arei)]

> -Original Message-
> From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-open.org]
> On Behalf Of Michael S. Tsirkin
> Sent: Thursday, September 08, 2016 12:30 PM
> To: Gonglei (Arei)
> Subject: [virtio-dev] Re: [PATCH v1 11/14] virtio-crypto: add control queue
> handler
> 
> On Thu, Sep 08, 2016 at 11:42:33AM +0800, Gonglei wrote:
> > +info->cipher_alg = virtio_ldl_p(vdev, _para->algo);
> > +info->key_len = virtio_ldl_p(vdev, _para->keylen);
> > +info->direction = virtio_ldl_p(vdev, _para->op);
> 
> There is no reason to use the virtio mixed endian-ness for
> modern devices. Just use little-endian everywhere.
> 
Okay, thanks :)


Regards,
-Gonglei


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

[virtio-dev] [PATCH v1 04/14] crypto: add symetric algorithms support

[Gonglei]

This patch include three parts, the first is define the
session structure and the second is the opertion structure,
whose properties are needed to finish the symetric algorithms.
The third part defines some function pointers.

Signed-off-by: Gonglei 
---
 crypto/crypto.c | 16 +-
 include/crypto/crypto.h | 85 +
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/crypto/crypto.c b/crypto/crypto.c
index 3f760fd..958a959 100644
--- a/crypto/crypto.c
+++ b/crypto/crypto.c
@@ -177,7 +177,21 @@ int qemu_deliver_crypto_packet(CryptoClientState *sender,
   void *header_opqaue,
   void *opaque)
 {
-return 0;
+CryptoClientState *cc = opaque;
+int ret = -1;
+
+if (!cc->ready) {
+return 1;
+}
+
+if (flags == QEMU_CRYPTO_PACKET_FLAG_SYM) {
+CryptoSymOpInfo *op_info = header_opqaue;
+if (cc->info->do_sym_op) {
+ret = cc->info->do_sym_op(cc, op_info);
+}
+}
+
+return ret;
 }
 
 int qemu_send_crypto_packet_async(CryptoClientState *sender,
diff --git a/include/crypto/crypto.h b/include/crypto/crypto.h
index 0bb7d73..5971c1b 100644
--- a/include/crypto/crypto.h
+++ b/include/crypto/crypto.h
@@ -34,10 +34,50 @@
 
 #define MAX_CRYPTO_QUEUE_NUM  64
 
+#define  QEMU_CRYPTO_PACKET_FLAG_NONE (0)
+#define  QEMU_CRYPTO_PACKET_FLAG_SYM (1 << 0)
+
+typedef struct CryptoSymSessionInfo {
+uint8_t op_code;
+uint8_t op_type;
+uint8_t direction;
+uint32_t cipher_alg;
+uint32_t key_len;
+uint8_t *cipher_key;
+
+uint32_t hash_alg;
+uint8_t hash_mode;
+uint32_t hash_result_len;
+uint8_t alg_chain_order;
+uint32_t auth_key_len;
+uint32_t add_len;
+uint8_t *auth_key;
+} CryptoSymSessionInfo;
+
+typedef struct CryptoSymOpInfo {
+uint64_t session_id;
+uint8_t op_type; /* cipher or algo chainning */
+uint8_t *src;
+uint8_t *dst;
+uint8_t *iv;
+uint8_t *aad_data; /* additional auth data */
+uint32_t aad_len;
+uint32_t iv_len;
+uint32_t src_len;
+/* the dst_len is equal to src_len + hash_result_len
+ * if hash alg configured */
+uint32_t dst_len;
+uint8_t data[0];
+} CryptoSymOpInfo;
+
 typedef void (CryptoPoll)(CryptoClientState *, bool);
 typedef void (CryptoCleanup) (CryptoClientState *);
 typedef void (CryptoClientDestructor)(CryptoClientState *);
 typedef void (CryptoHWStatusChanged)(CryptoClientState *);
+typedef int (CryptoCreateSymSession)(CryptoClientState *,
+  CryptoSymSessionInfo *, uint64_t *);
+typedef int (CryptoCloseSession)(CryptoClientState *, uint64_t);
+typedef int (CryptoDoSymOp)(CryptoClientState *, CryptoSymOpInfo *);
 
 typedef struct CryptoClientInfo {
 CryptoClientOptionsKind type;
@@ -46,6 +86,9 @@ typedef struct CryptoClientInfo {
 CryptoCleanup *cleanup;
 CryptoPoll *poll;
 CryptoHWStatusChanged *hw_status_changed;
+CryptoCreateSymSession *create_session;
+CryptoCloseSession *close_session;
+CryptoDoSymOp *do_sym_op;
 } CryptoClientInfo;
 
 struct CryptoClientState {
@@ -58,6 +101,21 @@ struct CryptoClientState {
 char info_str[256];
 CryptoQueue *incoming_queue;
 unsigned int queue_index;
+
+/* Supported service mask */
+uint32_t crypto_services;
+
+/* Detailed algorithms mask */
+uint32_t cipher_algo_l;
+uint32_t cipher_algo_h;
+uint32_t hash_algo;
+uint32_t mac_algo_l;
+uint32_t mac_algo_h;
+uint32_t asym_algo;
+uint32_t kdf_algo;
+uint32_t aead_algo;
+uint32_t primitive_algo;
+
 CryptoClientDestructor *destructor;
 };
 
@@ -70,6 +128,20 @@ typedef struct CryptoLegacyHWPeers {
 
 typedef struct CryptoLegacyHWConf {
 CryptoLegacyHWPeers peers;
+
+/* Supported service mask */
+uint32_t crypto_services;
+
+/* Detailed algorithms mask */
+uint32_t cipher_algo_l;
+uint32_t cipher_algo_h;
+uint32_t hash_algo;
+uint32_t mac_algo_l;
+uint32_t mac_algo_h;
+uint32_t asym_algo;
+uint32_t kdf_algo;
+uint32_t aead_algo;
+uint32_t primitive_algo;
 } CryptoLegacyHWConf;
 
 typedef struct CryptoLegacyHWState {
@@ -105,4 +177,17 @@ void qemu_del_crypto_legacy_hw(CryptoLegacyHWState 
*crypto);
 CryptoClientState *
 qemu_get_crypto_subqueue(CryptoLegacyHWState *crypto, int queue_index);
 
+CryptoLegacyHWState *qemu_get_crypto_legacy_hw(CryptoClientState *cc);
+
+void *qemu_get_crypto_legacy_hw_opaque(CryptoClientState *cc);
+
+int qemu_find_crypto_clients_except(const char *id, CryptoClientState **ccs,
+ CryptoClientOptionsKind type, int max);
+
+int qemu_crypto_create_session(CryptoClientState *cc,
+   CryptoSymSessionInfo *info,
+   uint64_t *session_id);
+int qemu_crypto_close_session(CryptoClientState *cc,
+   

[virtio-dev] [PATCH v1 02/14] crypto: introduce crypto queue handler

[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 | 205 ++
 crypto/crypto.c   |  28 ++
 include/crypto/crypto-queue.h |  69 ++
 include/crypto/crypto.h   |  12 +++
 5 files changed, 315 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..0d5be02
--- /dev/null
+++ b/crypto/crypto-queue.c
@@ -0,0 +1,205 @@
+/*
+ * 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);
+}
+
+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 ssize_t 

[virtio-dev] [PATCH v1 09/14] virtio-crypto: add virtio crypto realization modle

[Gonglei]

Introduce the virtio crypto realization modle, I'll
finish the core code in the following patches. The
thoughts came from virtio net realization.

Signed-off-by: Gonglei 
---
 hw/core/qdev-properties-system.c |  86 ++
 hw/virtio/Makefile.objs  |   1 +
 hw/virtio/virtio-crypto.c| 250 +++
 include/hw/qdev-properties.h |   3 +
 4 files changed, 340 insertions(+)
 create mode 100644 hw/virtio/virtio-crypto.c

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index e55afe6..93afd64 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -22,6 +22,8 @@
 #include "qapi/visitor.h"
 #include "sysemu/char.h"
 #include "sysemu/iothread.h"
+#include "crypto/crypto.h"
+
 
 static void get_pointer(Object *obj, Visitor *v, Property *prop,
 char *(*print)(void *ptr),
@@ -430,3 +432,87 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
 }
 nd->instantiated = 1;
 }
+
+/* --- cryptodev device --- */
+static void get_cryptodev(Object *obj, Visitor *v, const char *name,
+  void *opaque, Error **errp)
+{
+DeviceState *dev = DEVICE(obj);
+Property *prop = opaque;
+CryptoLegacyHWPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
+char *p = g_strdup(peers_ptr->ccs[0] ? peers_ptr->ccs[0]->name : "");
+visit_type_str(v, name, , errp);
+g_free(p);
+}
+
+static void set_cryptodev(Object *obj, Visitor *v, const char *name,
+  void *opaque, Error **errp)
+{
+DeviceState *dev = DEVICE(obj);
+Property *prop = opaque;
+CryptoLegacyHWPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
+CryptoClientState **ccs = peers_ptr->ccs;
+CryptoClientState *peers[MAX_CRYPTO_QUEUE_NUM];
+Error *local_err = NULL;
+int queues, err = 0, i = 0;
+char *str;
+
+if (dev->realized) {
+qdev_prop_set_after_realize(dev, name, errp);
+return;
+}
+
+visit_type_str(v, name, , _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+
+queues = qemu_find_crypto_clients_except(str, peers,
+  CRYPTO_CLIENT_OPTIONS_KIND_LEGACY_HW,
+  MAX_CRYPTO_QUEUE_NUM);
+if (queues == 0) {
+err = -ENOENT;
+goto out;
+}
+
+if (queues > MAX_CRYPTO_QUEUE_NUM) {
+error_setg(errp,
+  "queues of backend '%s'(%d) exceeds QEMU limitation(%d)",
+   str, queues, MAX_CRYPTO_QUEUE_NUM);
+goto out;
+}
+
+for (i = 0; i < queues; i++) {
+if (peers[i] == NULL) {
+err = -ENOENT;
+goto out;
+}
+
+if (peers[i]->peer) {
+err = -EEXIST;
+goto out;
+}
+
+if (ccs[i]) {
+err = -EINVAL;
+goto out;
+}
+
+ccs[i] = peers[i];
+ccs[i]->queue_index = i;
+}
+
+peers_ptr->queues = queues;
+
+out:
+error_set_from_qdev_prop_error(errp, err, dev, prop, str);
+g_free(str);
+}
+
+PropertyInfo qdev_prop_cryptodev = {
+.name  = "str",
+.description = "ID of a cryptodev to use as a backend",
+.get   = get_cryptodev,
+.set   = set_cryptodev,
+};
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 678281b..e2a90d2 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -6,3 +6,4 @@ common-obj-y += virtio-mmio.o
 obj-y += virtio.o virtio-balloon.o 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
 obj-$(CONFIG_VIRTIO_PCI) += virtio-crypto-pci.o
+obj-y += virtio-crypto.o
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
new file mode 100644
index 000..23c5041
--- /dev/null
+++ b/hw/virtio/virtio-crypto.c
@@ -0,0 +1,250 @@
+/*
+ * Virtio crypto Support
+ *
+ * Based on virtio-net.c
+ *
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ * Authors:
+ *Gonglei 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "qemu/iov.h"
+#include "hw/qdev.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-crypto.h"
+#include "hw/virtio/virtio-access.h"
+
+static void virtio_crypto_process(VirtIOCrypto *vcrypto)
+{
+}
+
+static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+{
+}
+
+static void virtio_crypto_handle_dataq_bh(VirtIODevice *vdev, VirtQueue *vq)
+{
+}
+
+static void virtio_crypto_dataq_bh(void *opaque)
+{
+}
+
+static void virtio_crypto_add_queue(VirtIOCrypto *vcrypto, int index)
+{
+VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
+
+vcrypto->vqs[index].dataq = virtio_add_queue(vdev, 1024,
+   

[virtio-dev] [PATCH v1 08/14] virtio-crypto-pci: add virtio crypto pci support

[Gonglei]

This patch adds virtio-crypto-pci, which is the pci proxy for the virtio
crypto device.

Signed-off-by: Gonglei 
---
 hw/virtio/Makefile.objs   |  1 +
 hw/virtio/virtio-crypto-pci.c | 71 +++
 hw/virtio/virtio-pci.h| 16 ++
 3 files changed, 88 insertions(+)
 create mode 100644 hw/virtio/virtio-crypto-pci.c

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 3e2b175..678281b 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -5,3 +5,4 @@ common-obj-y += virtio-mmio.o
 
 obj-y += virtio.o virtio-balloon.o 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
+obj-$(CONFIG_VIRTIO_PCI) += virtio-crypto-pci.o
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
new file mode 100644
index 000..4436955
--- /dev/null
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -0,0 +1,71 @@
+/*
+ * Virtio crypto device
+ *
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ * Authors:
+ *Gonglei 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ *
+ */
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-pci.h"
+#include "hw/virtio/virtio-crypto.h"
+
+static Property virtio_crypto_pci_properties[] = {
+DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
+VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
+DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_crypto_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIOCryptoPCI *vcrypto = VIRTIO_CRYPTO_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(>vdev);
+
+qdev_set_parent_bus(vdev, BUS(_dev->bus));
+virtio_pci_force_virtio_1(vpci_dev);
+object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void virtio_crypto_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+
+k->realize = virtio_crypto_pci_realize;
+set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+dc->props = virtio_crypto_pci_properties;
+
+pcidev_k->class_id = PCI_CLASS_OTHERS;
+}
+
+static void virtio_crypto_initfn(Object *obj)
+{
+VirtIOCryptoPCI *dev = VIRTIO_CRYPTO_PCI(obj);
+
+virtio_instance_init_common(obj, >vdev, sizeof(dev->vdev),
+TYPE_VIRTIO_CRYPTO);
+}
+
+static const TypeInfo virtio_crypto_pci_info = {
+.name  = TYPE_VIRTIO_CRYPTO_PCI,
+.parent= TYPE_VIRTIO_PCI,
+.instance_size = sizeof(VirtIOCryptoPCI),
+.instance_init = virtio_crypto_initfn,
+.class_init= virtio_crypto_pci_class_init,
+};
+
+static void virtio_crypto_pci_register_types(void)
+{
+type_register_static(_crypto_pci_info);
+}
+type_init(virtio_crypto_pci_register_types)
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 25fbf8a..1d8f622 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -25,6 +25,8 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-input.h"
 #include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-crypto.h"
+
 #ifdef CONFIG_VIRTFS
 #include "hw/9pfs/virtio-9p.h"
 #endif
@@ -44,6 +46,8 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
 typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
 typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
 typedef struct VirtIOGPUPCI VirtIOGPUPCI;
+typedef struct VirtIOCryptoPCI VirtIOCryptoPCI;
+
 
 /* virtio-pci-bus */
 
@@ -324,6 +328,18 @@ struct VirtIOGPUPCI {
 VirtIOGPU vdev;
 };
 
+/*
+ * virtio-crypto-pci: This extends VirtioPCIProxy.
+ */
+#define TYPE_VIRTIO_CRYPTO_PCI "virtio-crypto-pci"
+#define VIRTIO_CRYPTO_PCI(obj) \
+OBJECT_CHECK(VirtIOCryptoPCI, (obj), TYPE_VIRTIO_CRYPTO_PCI)
+
+struct VirtIOCryptoPCI {
+VirtIOPCIProxy parent_obj;
+VirtIOCrypto vdev;
+};
+
 /* Virtio ABI version, if we increment this, we break the guest driver. */
 #define VIRTIO_PCI_ABI_VERSION  0
 
-- 
1.7.12.4



-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

[virtio-dev] [PATCH v1 14/14] virtio-crypto: add data virtqueue processing handler

[Gonglei]

At present, we only support cipher and algorithm chainning.

Signed-off-by: Gonglei 
---
 hw/virtio/virtio-crypto.c | 364 ++
 1 file changed, 364 insertions(+)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 55ce330..6d4a13b 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -22,6 +22,8 @@
 #include "hw/virtio/virtio-crypto.h"
 #include "hw/virtio/virtio-access.h"
 
+static int32_t virtio_crypto_flush_dataq(VirtIOCryptoQueue *q);
+
 static void virtio_crypto_process(VirtIOCrypto *vcrypto)
 {
 }
@@ -31,6 +33,14 @@ static inline int virtio_crypto_vq2q(int queue_index)
 return queue_index;
 }
 
+static VirtIOCryptoQueue *
+virtio_crypto_get_subqueue(CryptoClientState *cc)
+{
+VirtIOCrypto *vcrypto = qemu_get_crypto_legacy_hw_opaque(cc);
+
+return >vqs[cc->queue_index];
+}
+
 static void
 virtio_crypto_cipher_session_helper(VirtIODevice *vdev,
CryptoSymSessionInfo *info,
@@ -258,12 +268,366 @@ static void virtio_crypto_handle_ctrl(VirtIODevice 
*vdev, VirtQueue *vq)
 }
 }
 
+static CryptoSymOpInfo *
+virtio_crypto_cipher_op_helper(VirtIODevice *vdev,
+   struct virtio_crypto_cipher_para *para,
+   struct virtio_crypto_cipher_output *out,
+   uint32_t aad_len,
+   uint64_t aad_data_addr)
+{
+CryptoSymOpInfo *op_info;
+uint32_t src_len, dst_len;
+uint32_t iv_len;
+size_t max_len, curr_size = 0;
+hwaddr iv_gpa, src_gpa;
+void *iv_hva, *src_hva, *aad_hva;
+hwaddr len;
+
+iv_len = virtio_ldl_p(vdev, >iv_len);
+src_len = virtio_ldl_p(vdev, >src_data_len);
+dst_len = virtio_ldl_p(vdev, >dst_data_len);
+
+max_len = iv_len + aad_len + src_len + dst_len;
+op_info = g_malloc0(sizeof(CryptoSymOpInfo) + max_len);
+op_info->iv_len = iv_len;
+op_info->src_len = src_len;
+op_info->dst_len = dst_len;
+op_info->aad_len = aad_len;
+/* handle the initilization vector */
+if (op_info->iv_len > 0) {
+len = op_info->iv_len;
+DPRINTF("iv_len=%" PRIu32 "\n", len);
+op_info->iv = op_info->data + curr_size;
+
+iv_gpa = virtio_ldq_p(vdev, >iv_addr);
+iv_hva = cpu_physical_memory_map(iv_gpa, , false);
+memcpy(op_info->iv, iv_hva, len);
+cpu_physical_memory_unmap(iv_hva, len, false, len);
+curr_size += len;
+}
+
+/* handle additional authentication data if exist */
+if (op_info->aad_len > 0) {
+len = op_info->aad_len;
+DPRINTF("aad_len=%" PRIu32 "\n", len);
+op_info->aad_data = op_info->data + curr_size;
+
+aad_hva = cpu_physical_memory_map(aad_data_addr, , false);
+memcpy(op_info->aad_data, aad_hva, len);
+cpu_physical_memory_unmap(aad_hva, len, false, len);
+curr_size += len;
+}
+
+/* handle the source data */
+if (op_info->src_len > 0) {
+len = op_info->src_len;
+DPRINTF("src_len=%" PRIu32 "\n", len);
+op_info->src = op_info->data + curr_size;
+
+src_gpa = virtio_ldq_p(vdev, >src_data_addr);
+src_hva = cpu_physical_memory_map(src_gpa, , false);
+memcpy(op_info->src, src_hva, len);
+cpu_physical_memory_unmap(src_hva, len, false, len);
+curr_size += len;
+}
+op_info->dst = op_info->data + curr_size;
+DPRINTF("dst_len=%" PRIu32 "\n", op_info->dst_len);
+
+return op_info;
+}
+
+static void
+virtio_crypto_sym_input_data_helper(VirtIODevice *vdev,
+void *idata_hva,
+uint32_t status,
+CryptoSymOpInfo *sym_op_info)
+{
+struct virtio_crypto_sym_input *idata = idata_hva;
+hwaddr dst_gpa, len;
+void *dst_hva;
+
+virtio_stl_p(vdev, >status, status);
+if (status != VIRTIO_CRYPTO_OP_OK) {
+return;
+}
+
+/* save the cipher result */
+dst_gpa = virtio_ldq_p(vdev, >dst_data_addr);
+/* Note: length of dest_data is equal to length of src_data for cipher */
+len = sym_op_info->src_len;
+dst_hva = cpu_physical_memory_map(dst_gpa, , true);
+memcpy(dst_hva, sym_op_info->dst, len);
+cpu_physical_memory_unmap(dst_hva, len, true, len);
+
+if (sym_op_info->op_type ==
+  VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
+hwaddr digest_gpa;
+void *digest_hva;
+
+/* save the digest result */
+digest_gpa = virtio_ldq_p(vdev, >digest_result_addr);
+len = sym_op_info->dst_len - sym_op_info->src_len;
+digest_hva = cpu_physical_memory_map(dst_gpa, , true);
+memcpy(digest_hva, sym_op_info->dst + sym_op_info->src_len, len);
+cpu_physical_memory_unmap(digest_hva, len, true, len);
+}
+}
+
+static void virtio_crypto_tx_complete(CryptoClientState *cc,
+  int ret)
+{
+VirtIOCrypto *vcrypto = qemu_get_crypto_legacy_hw_opaque(cc);
+VirtIOCryptoQueue *q = 

[virtio-dev] [PATCH v1 00/14] virtio-crypto: introduce framework and device emulation

[Gonglei]

This patch series realize the framework and emulation of a new
virtio crypto device, which is similar with virtio net device.
 
 - I introduce the cryptodev backend as the client of virtio crypto device
   which can be realized by different methods, such as cryptodev-linux in my 
series,
   vhost-crypto kernel module, vhost-user etc.
 - The patch set abides by the virtio crypto speccification v9.
 - The virtio crypto support symmetric algorithms (including CIPHER and 
algorithm chainning)
   at present, except HASH, MAC and AEAD services.
 - unsupport hot plug/unplug cryptodev client at this moment.

Cryptodev-linux is a device that allows access to Linux kernel cryptographic 
drivers;
thus allowing of userspace applications to take advantage of hardware 
accelerators.
It can be found here:

 http://cryptodev-linux.org/

(please use the latest version)

To use the cryptodev-linux as the client, the cryptodev.ko should be insert on 
the host.

 # enter cryptodev-linux module root directory, then
 make;make install

then configuring QEMU shows:

 [...]
 jemalloc support  no
 avx2 optimization no
 cryptodev-linux support yes

QEMU can then be started using the following parameters:

qemu-system-x86_64 \
[...] \
-cryptodev type=cryptodev-linux,id=cryptodev0 \
-device virtio-crypto-pci,id=crypto0,cryptodev=cryptodev0 \
[...]

The front-end linux kernel driver (Experimental at present) is publicly 
accessible from:
 
   https://github.com/gongleiarei/virtio-crypto-linux-driver.git

After insmod virtio-crypto.ko, you also can use cryptodev-linux test the crypto 
function
in the guest. For example:

linux-guest:/home/gonglei/cryptodev-linux/tests # ./cipher -
requested cipher CRYPTO_AES_CBC, got cbc(aes) with driver virtio_crypto_aes_cbc
AES Test passed
requested cipher CRYPTO_AES_CBC, got cbc(aes) with driver virtio_crypto_aes_cbc
requested cipher CRYPTO_AES_CBC, got cbc(aes) with driver virtio_crypto_aes_cbc
Test passed

Please review, thanks!

QEMU code also can be accessible from:

 https://github.com/gongleiarei/qemu.git 

 branch virtio-crypto

Regards,

Gonglei (14):
  crypto: introduce cryptodev backend and crypto legacy hardware
  crypto: introduce crypto queue handler
  crypto: add cryptoLegacyHW stuff
  crypto: add symetric algorithms support
  crypto: add cryptodev-linux as a cryptodev backend
  crypto: add internal handle logic layer
  virtio-crypto: introduce virtio-crypto.h
  virtio-crypto-pci: add virtio crypto pci support
  virtio-crypto: add virtio crypto realization modle
  virtio-crypto: set capacity of crypto legacy hardware
  virtio-crypto: add control queue handler
  virtio-crypto: add destroy session logic
  virtio-crypto: get correct input data address for each request
  virtio-crypto: add data virtqueue processing handler

 configure  |  16 +
 crypto/Makefile.objs   |   3 +
 crypto/crypto-queue.c  | 205 ++
 crypto/crypto.c| 378 +++
 crypto/cryptodev-linux.c   | 419 
 hw/core/qdev-properties-system.c   |  86 +++
 hw/virtio/Makefile.objs|   2 +
 hw/virtio/virtio-crypto-pci.c  |  71 ++
 hw/virtio/virtio-crypto.c  | 868 +
 hw/virtio/virtio-pci.h |  16 +
 include/crypto/crypto-clients.h|  39 ++
 include/crypto/crypto-queue.h  |  69 ++
 include/crypto/crypto.h| 193 ++
 include/hw/qdev-properties.h   |   3 +
 include/hw/virtio/virtio-crypto.h  |  84 +++
 include/qemu/typedefs.h|   1 +
 include/standard-headers/linux/virtio_crypto.h | 448 +
 include/sysemu/sysemu.h|   1 +
 qapi-schema.json   |  61 ++
 qemu-options.hx|  19 +
 vl.c   |  13 +
 21 files changed, 2995 insertions(+)
 create mode 100644 crypto/crypto-queue.c
 create mode 100644 crypto/crypto.c
 create mode 100644 crypto/cryptodev-linux.c
 create mode 100644 hw/virtio/virtio-crypto-pci.c
 create mode 100644 hw/virtio/virtio-crypto.c
 create mode 100644 include/crypto/crypto-clients.h
 create mode 100644 include/crypto/crypto-queue.h
 create mode 100644 include/crypto/crypto.h
 create mode 100644 include/hw/virtio/virtio-crypto.h
 create mode 100644 include/standard-headers/linux/virtio_crypto.h

-- 
1.7.12.4



-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

[virtio-dev] [PATCH v1 07/14] virtio-crypto: introduce virtio-crypto.h

[Gonglei]

This patch introduces the header of virtio crypto emulation.

Signed-off-by: Gonglei 
---
 include/hw/virtio/virtio-crypto.h  | 84 ++
 include/standard-headers/linux/virtio_crypto.h | 34 +--
 2 files changed, 101 insertions(+), 17 deletions(-)
 create mode 100644 include/hw/virtio/virtio-crypto.h

diff --git a/include/hw/virtio/virtio-crypto.h 
b/include/hw/virtio/virtio-crypto.h
new file mode 100644
index 000..fa97e97
--- /dev/null
+++ b/include/hw/virtio/virtio-crypto.h
@@ -0,0 +1,84 @@
+/*
+ * Virtio crypto Support
+ *
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ * Authors:
+ *Gonglei 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#ifndef _QEMU_VIRTIO_CRYPTO_H
+#define _QEMU_VIRTIO_CRYPTO_H
+
+#include "standard-headers/linux/virtio_crypto.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/iothread.h"
+#include "crypto/crypto.h"
+
+#define VIRTIO_ID_CRYPTO 20
+
+/* #define DEBUG_VIRTIO_CRYPTO */
+
+#ifdef DEBUG_VIRTIO_CRYPTO
+#define DPRINTF(fmt, ...) \
+do { printf("virtio_crypto: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do { } while (0)
+#endif
+
+#define TYPE_VIRTIO_CRYPTO "virtio-crypto-device"
+#define VIRTIO_CRYPTO(obj) \
+OBJECT_CHECK(VirtIOCrypto, (obj), TYPE_VIRTIO_CRYPTO)
+#define VIRTIO_CRYPTO_GET_PARENT_CLASS(obj) \
+OBJECT_GET_PARENT_CLASS(obj, TYPE_VIRTIO_CRYPTO)
+
+
+/* Limit the number of packets that can be sent via a single flush
+ * of the TX queue.  This gives us a guaranteed exit condition and
+ * ensures fairness in the io path.  256 conveniently matches the
+ * length of the TX queue and shows a good balance of performance
+ * and latency. */
+#define VIRTIO_CRYPTO_TX_BURST 256
+
+typedef struct VirtIOCryptoConf {
+int32_t txburst;
+} VirtIOCryptoConf;
+
+struct VirtIOCrypto;
+
+typedef struct VirtIOCryptoQueue {
+VirtQueue *dataq;
+QEMUBH *tx_bh;
+int tx_waiting;
+struct {
+VirtQueueElement *elem;
+uint32_t flags;
+CryptoSymOpInfo *op_info;
+void *idata_hva;
+} async_tx;
+struct VirtIOCrypto *vcrypto;
+} VirtIOCryptoQueue;
+
+typedef struct VirtIOCrypto {
+VirtIODevice parent_obj;
+
+VirtIOCryptoQueue *vqs;
+VirtQueue *ctrl_vq;
+CryptoLegacyHWState *crypto;
+CryptoLegacyHWConf legacy_conf;
+
+VirtIOCryptoConf conf;
+int32_t tx_burst;
+uint32_t max_queues;
+uint32_t status;
+
+int multiqueue;
+uint32_t curr_queues;
+size_t config_size;
+} VirtIOCrypto;
+
+#endif /* _QEMU_VIRTIO_CRYPTO_H */
diff --git a/include/standard-headers/linux/virtio_crypto.h 
b/include/standard-headers/linux/virtio_crypto.h
index 443b2a8..5d8b37e 100644
--- a/include/standard-headers/linux/virtio_crypto.h
+++ b/include/standard-headers/linux/virtio_crypto.h
@@ -13,7 +13,7 @@
 
 #define VIRTIO_CRYPTO_OPCODE(service, op)   ((service << 8) | (op))
 
-struct virtio_crypto_ctrl_header{
+struct virtio_crypto_ctrl_header {
 #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
 #define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
@@ -65,7 +65,7 @@ struct virtio_crypto_cipher_session_para {
 };
 
 struct virtio_crypto_session_input {
-// Device-writable part
+/* Device-writable part */
 __virtio64 session_id;
 __virtio32 status;
 __virtio32 padding;
@@ -205,7 +205,7 @@ struct virtio_crypto_sym_create_session_req {
 struct virtio_crypto_alg_chain_session_req chain;
 } u;
 
-// Device-readable part
+/* Device-readable part */
 
 /* No operation */
 #define VIRTIO_CRYPTO_SYM_OP_NONE  0
@@ -219,9 +219,9 @@ struct virtio_crypto_sym_create_session_req {
 };
 
 struct virtio_crypto_destroy_session_req {
-// Device-readable part
+/* Device-readable part */
 __virtio64  session_id;
-// Device-writable part
+/* Device-writable part */
 __virtio32  status;
 __virtio32  padding;
 };
@@ -331,24 +331,24 @@ struct virtio_crypto_aead_output {
 };
 
 struct virtio_crypto_cipher_data_req {
-// Device-readable part
+/* Device-readable part */
 struct virtio_crypto_cipher_para para;
 struct virtio_crypto_cipher_output odata;
-// Device-writable part
+/* Device-writable part */
 struct virtio_crypto_cipher_input idata;
 };
 
 struct virtio_crypto_hash_data_req {
-// Device-readable part
+/* Device-readable part */
 struct virtio_crypto_hash_output odata;
-// Device-writable part
+/* Device-writable part */
 struct virtio_crypto_hash_input idata;
 };
 
 struct virtio_crypto_mac_data_req {
-// Device-readable part
+/* Device-readable part */
 struct virtio_crypto_mac_output odata;
-// Device-writable part
+/* Device-writable part */
 struct 

[virtio-dev] [PATCH v1 05/14] crypto: add cryptodev-linux as a cryptodev backend

[Gonglei]

Cryptodev-linux is a device that allows access to Linux
kernel cryptographic drivers; thus allowing of userspace
applications to take advantage of hardware accelerators.
Cryptodev-linux is implemented as a standalone module
that requires no dependencies other than a stock linux kernel.

The Cryptodev-linux project website is:
  http://cryptodev-linux.org/

Meanwile, I introdue the virtio_crypto.h which follows
virtio-crypto specification bacause cryptodev-linux.c include it.

Signed-off-by: Gonglei 
---
 configure  |  16 +
 crypto/Makefile.objs   |   1 +
 crypto/crypto.c|   8 +-
 crypto/cryptodev-linux.c   | 419 +++
 include/crypto/crypto-clients.h|  39 +++
 include/standard-headers/linux/virtio_crypto.h | 448 +
 qapi-schema.json   |  17 +-
 qemu-options.hx|  18 +-
 8 files changed, 963 insertions(+), 3 deletions(-)
 create mode 100644 crypto/cryptodev-linux.c
 create mode 100644 include/crypto/crypto-clients.h
 create mode 100644 include/standard-headers/linux/virtio_crypto.h

diff --git a/configure b/configure
index 5a9bda1..0fbead7 100755
--- a/configure
+++ b/configure
@@ -320,6 +320,7 @@ vhdx=""
 numa=""
 tcmalloc="no"
 jemalloc="no"
+cryptodev_linux="no"
 
 # parse CC options first
 for opt do
@@ -3616,6 +3617,16 @@ EOF
 fi
 
 ##
+# cryptodev-linux header probe
+cat > $TMPC << EOF
+#include 
+int main(void) { return 0; }
+EOF
+if compile_prog "" "" ; then
+cryptodev_linux="yes"
+fi
+
+##
 # signalfd probe
 signalfd="no"
 cat > $TMPC << EOF
@@ -4920,6 +4931,7 @@ echo "NUMA host support $numa"
 echo "tcmalloc support  $tcmalloc"
 echo "jemalloc support  $jemalloc"
 echo "avx2 optimization $avx2_opt"
+echo "cryptodev-linux support $cryptodev_linux"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -5321,6 +5333,10 @@ if test "$avx2_opt" = "yes" ; then
   echo "CONFIG_AVX2_OPT=y" >> $config_host_mak
 fi
 
+if test "$cryptodev_linux" = "yes" ; then
+  echo "CONFIG_CRYPTODEV_LINUX=y" >> $config_host_mak
+fi
+
 if test "$lzo" = "yes" ; then
   echo "CONFIG_LZO=y" >> $config_host_mak
 fi
diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
index 652b429..1f2ec24 100644
--- a/crypto/Makefile.objs
+++ b/crypto/Makefile.objs
@@ -28,6 +28,7 @@ crypto-obj-y += block-qcow.o
 crypto-obj-y += block-luks.o
 crypto-obj-y += crypto.o
 crypto-obj-y += crypto-queue.o
+crypto-obj-$(CONFIG_CRYPTODEV_LINUX) += cryptodev-linux.o
 
 # Let the userspace emulators avoid linking gnutls/etc
 crypto-aes-obj-y = aes.o
diff --git a/crypto/crypto.c b/crypto/crypto.c
index 958a959..1d3d1d3 100644
--- a/crypto/crypto.c
+++ b/crypto/crypto.c
@@ -34,6 +34,7 @@
 #include "crypto/crypto.h"
 #include "qemu/config-file.h"
 #include "monitor/monitor.h"
+#include "crypto/crypto-clients.h"
 
 
 static QTAILQ_HEAD(, CryptoClientState) crypto_clients;
@@ -81,7 +82,12 @@ int crypto_init_clients(void)
 static int (* const crypto_client_init_fun[CRYPTO_CLIENT_OPTIONS_KIND__MAX])(
 const CryptoClientOptions *opts,
 const char *name,
-CryptoClientState *peer, Error **errp);
+CryptoClientState *peer, Error **errp) = {
+#ifdef CONFIG_CRYPTODEV_LINUX
+[CRYPTO_CLIENT_OPTIONS_KIND_CRYPTODEV_LINUX] =
+ crypto_init_cryptodev_linux,
+#endif
+};
 
 static int crypto_client_init1(const void *object, Error **errp)
 {
diff --git a/crypto/cryptodev-linux.c b/crypto/cryptodev-linux.c
new file mode 100644
index 000..ae2dcdb
--- /dev/null
+++ b/crypto/cryptodev-linux.c
@@ -0,0 +1,419 @@
+/*
+ * cryptodev-linux backend support
+ *
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ *
+ * Authors:
+ *Gonglei 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include 
+#include 
+#include 
+
+#include "qemu/osdep.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+#include "qemu/iov.h"
+#include "qapi-visit.h"
+#include "qapi/opts-visitor.h"
+#include "qemu/error-report.h"
+
+#include "crypto/crypto.h"
+#include "crypto/crypto-clients.h"
+#include "standard-headers/linux/virtio_crypto.h"
+
+
+#define CRYPTO_CHARDEV_PATH "/dev/crypto"
+
+
+typedef struct CryptodevLinuxSession {
+struct session_op *sess;
+uint8_t direction; /* encryption or decryption */
+uint8_t type; /* cipher? hash? aead? */
+QTAILQ_ENTRY(CryptodevLinuxSession) next;
+} CryptodevLinuxSession;
+
+typedef struct CryptodevLinuxState {
+CryptoClientState cc;
+int fd;
+bool read_poll;
+bool write_poll;
+bool enabled;
+

[virtio-dev] Re: [Qemu-devel] [virtio-dev][RFC v3] virtio-sdm: new device specification

[Christian Pinto]

On 07/09/2016 09:51, Edgar E. Iglesias wrote:

On Wed, Sep 07, 2016 at 09:24:39AM +0200, Christian Pinto wrote:

Hello Edgar,

thanks for your comments.


Thanks for the clarification, I have a few follow-up questions/comments.
  

On 06/09/2016 23:43, Edgar E. Iglesias wrote:

Hi,

Sorry for the delay. I have a few questions.

I don't fully understand the purpose of this. Could you elaborate a little on 
that?
You need a real IPI signal to drive this I guess, so it's a little bit of a
chicken and egg problem.

Usually in AMP-like systems CPUs signal each other with IPIs by exploiting
a dedicated hardware peripheral, like a hardware mailbox. So I see the
concept of the SDM (master, slave and communication channels) as the
hardware device to actually implement and deliver the IPIs.
In this case the SDM is designed in a way to be able to deliver multiple
signals (e.g., BOOT, RESET) and not just a single interrupt.


A useful feature I can see is multiplexing a single underlying IPI (driving
an sdm channel) into multiple "virtual" IPI signals. Is that the main
use-case?

In the SDM multiple slaves can connect to the same SDM channel (e.g.,
socket)
and the master uses the channel to deliver the signal (e.g., IRQ) to the
destination slave device. The other way around is also possible, slaves
signaling
the master.
I guess this is the multiplexing features you were talking about.

Right, but for this to be more useful, I think you should pass on some
payload with the IRQ signal. That will allow this channel to be used
to enable notification mechanisms for many other channels over a single
HW irq (e.g the payload would be like the irq vector or irq line


All signals carry a 64 bits payload that for the time being is ignored
for the IRQ signal. I now see your point, thanks for the clarification.
The payload could definitely be used as an IRQ vector, so that SW
can register to a specific IRQ line of the SDM HW interrupt.


Another question:
For the reverese path, I don't see how it will be reasonably implemented
in the remote cpus various sw stacks sharing a single virtio queue.
It would seem like you would need a reverese channel per remoteproc
to avoid locking (very messy in AMP systems) to safely update
the reverese gh_vq queue?

Or how do you see multiple AMP systems safely updating a single virtio
queue in parallel?


The AMP systems are not all interacting with the same virtio queues.
As described in the specification the device can be a master or a slave.
The master processor will interact with a master SDM instance,
while remote processors with slave SDM instances.
Master/slave instances should be seen as multiple ports of the same
SDM.
Master and slave SDM instances communicate using the SDM Channel.
So, no need for locking or to handle concurrent updates from
multiple processors to the same virtio queue.

Christian



Best regards,
Edgar



However it is worth to notice that one SDM channel is not dedicated to one
specific signal, but rather it is used to deliver all the signals
implemented
by the device.


Regarding the boot and reset. Typically there's a reset signal you release and
the remote CPU starts running. You can ofcourse reset the CPU and restart
at anytime. Not sure if you need an additional BOOT virtual signal.

There may also be additional mechanisms to control the startup address of the 
remote CPU.
Any thoughts on that?

The purpose of the BOOT signal is also to pass the remote CPU its startup
address. So the idea is to use the payload of the signal to send the slave
processor
its startup address over the SDM channel and SDM slave device. Our
implementation
for QEMU uses the BOOT signal for this purpose.

This part of the semantic of the BOOT signal is not described in the
documentation
since I believe it is a choice of the actual implementation of the SDM
whether to
pass the startup address or not (i.e. there might be a platform where the
boot
address of slave processors is somehow hard-coded).
I can add the explanation to the device specification to make it more clear.


Thanks,

Christian


Best regards,
Edgar


On Tue, Aug 30, 2016 at 01:22:26PM +0200, Christian Pinto wrote:

Hello,

are there any comments?


Christian


On 09/08/2016 09:37, Christian Pinto wrote:

This patch adds the specification of the Signal Dristribution Module virtio
device to the current virtio specification document.

Signed-off-by: Christian Pinto 

Signed-off-by: Baptiste Reynal 



---
v2 -> v3:
- Removed master field from configuration and replaced with device_id
- Added new RESET signal
- Added signals definition into device specs
- Added feature bits associated to signals

v1 -> v2:
- Fixed some typos
- Removed dependencies from QEMU
- Added explanation on how SDM can be used in AMP systems
- Explained semantics of payload field in SDMSignalData struct
---
  

Re: [virtio-dev] Re: [PATCH v9] virtio-net: add Max MTU configuration field

[Aaron Conole]

"Michael S. Tsirkin"  writes:

> On Tue, Sep 06, 2016 at 10:31:01AM -0400, Aaron Conole wrote:
>> It is helpful for a host to indicate it's MTU to be set on guest NICs
>> other than the assumed 1500 byte value.  This helps in situations where
>> the host network is using Jumbo Frames, or aiding in PMTU discovery by
>> configuring a homogenous network.  It is also helpful for sizing receive
>> buffers correctly.
>> 
>> The change adds a new field to configuration area of network
>> devices.  It will be used to pass a maximum MTU from the device to
>> the driver.  This will be used by the driver as a maximum value for
>> packet sizes during transmission, without segmentation offloading.
>> 
>> In addition, in order to support backward and forward compatibility,
>> we introduce a new feature bit called VIRTIO_NET_F_MTU.
>> 
>> VIRTIO-152
>> Signed-off-by: Aaron Conole 
>> Cc: Victor Kaplansky 
>> Acked-by: Michael S. Tsirkin 
>> Acked-by: Maxime Coquelin 
>
> One technicality - please Cc virtualizat...@lists.linux-foundation.org
> on anything that affects the drivers, also since that is archived more
> widely.

Will do, thanks.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

[virtio-dev] Re: [PATCH v9] virtio-net: add Max MTU configuration field

[Michael S. Tsirkin]

On Tue, Sep 06, 2016 at 10:31:01AM -0400, Aaron Conole wrote:
> It is helpful for a host to indicate it's MTU to be set on guest NICs
> other than the assumed 1500 byte value.  This helps in situations where
> the host network is using Jumbo Frames, or aiding in PMTU discovery by
> configuring a homogenous network.  It is also helpful for sizing receive
> buffers correctly.
> 
> The change adds a new field to configuration area of network
> devices.  It will be used to pass a maximum MTU from the device to
> the driver.  This will be used by the driver as a maximum value for
> packet sizes during transmission, without segmentation offloading.
> 
> In addition, in order to support backward and forward compatibility,
> we introduce a new feature bit called VIRTIO_NET_F_MTU.
> 
> VIRTIO-152
> Signed-off-by: Aaron Conole 
> Cc: Victor Kaplansky 
> Acked-by: Michael S. Tsirkin 
> Acked-by: Maxime Coquelin 

One technicality - please Cc virtualizat...@lists.linux-foundation.org
on anything that affects the drivers, also since that is archived more
widely.


> ---
> v1:
> This is an attempt at continuing the work done by Victor Kaplansky on
> mtu negiotiation for virtio-net devices. It attempts to pick up from
> https://lists.oasis-open.org/archives/virtio-dev/201508/msg7.html
> and is just a minor blurb from the first patch along with the 2nd patch
> from the series, and some of the feedback integrated.
> 
> v2:
> Rephrase and provide a mechanism for guest->host and host->guest
> communication through a driver read-only and driver write-only field.
> 
> v3:
> Converted to just support initial MTU. Guest->host and Host->guest MTU
> changes are outside the scope of this change.
> 
> v4:
> Removed references to 'initial', since that condition cannot be tested.
> Simply state that if the driver will use the mtu field, it must
> negotiate the feature bit, and if not, it must not.
> 
> v5:
> After feedback from Michael S. Tsirkin
> 
> v6:
> Bit has to change to bit 25 due to an undocumented bit 24 being taken.
> 
> v7:
> Partial rewrite with feedback from MST.  Additional conformance statements
> added.
> 
> v8:
> Clarified the new conformance statements.
> 
> v9:
> Updated the commit log for correctness.  Added ACKs from
> Michael S. Tsirkin, and Maxime Coquelin.  Included the virtio
> issue id.
> 
>  Note: should this proposal be accepted and approved, one or more
>claims disclosed to the TC admin and listed on the Virtio TC
>IPR page https://www.oasis-open.org/committees/virtio/ipr.php
>might become Essential Claims.
> 
> 
>  content.tex | 34 ++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 4b45678..b90cbad 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3049,6 +3049,14 @@ features.
>  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
>  reconfiguration support.
>  
> +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
> +offered by the device, device advises driver about the value of
> +MTU to be used. If negotiated, the driver uses \field{mtu} as
> +the maximum MTU value supplied to the operating system.
> +
> +Note: many operating systems override the MTU value provided by the
> +driver.
> +
>  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
>  
>  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues 
> (receiveq1\ldots receiveqN
>  and transmitq1\ldots transmitqN respectively) that can be configured once 
> VIRTIO_NET_F_MQ
>  is negotiated.
>  
> +The following driver-read-only field, \field{mtu} only exists if
> +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver 
> to
> +use.
> +
>  \begin{lstlisting}
>  struct virtio_net_config {
>  u8 mac[6];
>  le16 status;
>  le16 max_virtqueue_pairs;
> +le16 mtu;
>  };
>  \end{lstlisting}
>  
> @@ -3153,6 +3166,18 @@ struct virtio_net_config {
>  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 
> inclusive,
>  if it offers VIRTIO_NET_F_MQ.
>  
> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
> +if it offers VIRTIO_NET_F_MTU.
> +
> +The device MUST NOT modify \field{mtu} once it has been set.
> +
> +The device MUST NOT pass received packets that exceed \field{mtu} size
> +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
> +
> +The device MUST forward transmitted packets of up to MTU size with
> +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
> +offers VIRTIO_NET_F_MTU.
> +
>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / 
> Network Device / Device configuration layout}
>  
>  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -3165,6 +3190,15 @@ 

Re: [virtio-dev] [PATCH v9] virtio-net: add Max MTU configuration field

[Michael S. Tsirkin]

On Wed, Sep 07, 2016 at 10:20:24AM -0400, Aaron Conole wrote:
> > Again, sorry for being late with comments, but I'll vote with no for
> > now. I can still change the vote if you can convince me, though.
> 
> No problem, thanks so much for the review.  I'm hoping you would be
> convinced to change your vote to yes with the adjusted wording - I'll
> publish a v10 next week (to give folks enough time to respond).
> 
> -Aaron

In that case I'll withdraw the ballot for now. Please let me know
when you want me to restart it.

-- 
MST

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

Re: [virtio-dev] [PATCH v9] virtio-net: add Max MTU configuration field

[Aaron Conole]

"Michael S. Tsirkin"  writes:

> On Wed, Sep 07, 2016 at 11:18:22AM +0200, Cornelia Huck wrote:
>> On Tue,  6 Sep 2016 10:31:01 -0400
>> Aaron Conole  wrote:
>> 
>> > this as this is up for vote>
>> 
>> > diff --git a/content.tex b/content.tex
>> > index 4b45678..b90cbad 100644
>> > --- a/content.tex
>> > +++ b/content.tex
>> > @@ -3049,6 +3049,14 @@ features.
>> >  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
>> >  reconfiguration support.
>> > 
>> > +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
>> > +offered by the device, device advises driver about the value of
>> > +MTU to be used. If negotiated, the driver uses \field{mtu} as
>> > +the maximum MTU value supplied to the operating system.
>> > +
>> > +Note: many operating systems override the MTU value provided by the
>> > +driver.
>> 
>> I'm wondering: Do we need to distinguish between what the _driver_ does
>> and what the _guest_ does, generally speaking?
>> 
>> > +
>> >  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
>> > 
>> >  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
>> > @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues 
>> > (receiveq1\ldots receiveqN
>> >  and transmitq1\ldots transmitqN respectively) that can be configured once 
>> > VIRTIO_NET_F_MQ
>> >  is negotiated.
>> > 
>> > +The following driver-read-only field, \field{mtu} only exists if
>> > +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the 
>> > driver to
>> > +use.
>> > +
>> >  \begin{lstlisting}
>> >  struct virtio_net_config {
>> >  u8 mac[6];
>> >  le16 status;
>> >  le16 max_virtqueue_pairs;
>> > +le16 mtu;
>> >  };
>> >  \end{lstlisting}
>> > 
>> > @@ -3153,6 +3166,18 @@ struct virtio_net_config {
>> >  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 
>> > inclusive,
>> >  if it offers VIRTIO_NET_F_MQ.
>> > 
>> > +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
>> > +if it offers VIRTIO_NET_F_MTU.
>> > +
>> > +The device MUST NOT modify \field{mtu} once it has been set.
>> 
>> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
>> successfully negotiated"? I don't think the driver should assume
>> anything until after feature negotiation is complete.
>
> I don't think this matters much, but generally it's best
> if the config space can be read at any time.
> For example, this can allow validating MTU and not negotiating
> it if it's e.g. too small.
> Do you see value in tweaking the MTU after negotiation?
>
>
>> > +
>> > +The device MUST NOT pass received packets that exceed \field{mtu} size
>> > +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
>> 
>> I don't think it should be the offer that is the deciding factor, but
>> rather the final negotiation.
>
> Makes sense.
>
>> But maybe we can make this "SHOULD NOT"?
>
> It's important to make this a MUST because otherwise device
> needs to do something if it gets a bigger packet -
> e.g. fragment it, or drop it.
> With this wording, it does not have to.
>
>
>> > +
>> > +The device MUST forward transmitted packets of up to MTU size with
>> > +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
>> > +offers VIRTIO_NET_F_MTU.
>> > +
>> >  \drivernormative{\subsubsection}{Device configuration
>> > layout}{Device Types / Network Device / Device configuration
>> > layout}
>> > 
>> >  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
>> > @@ -3165,6 +3190,15 @@ If the driver does not negotiate the
>> > VIRTIO_NET_F_STATUS feature, it SHOULD
>> >  assume the link is active, otherwise it SHOULD read the link status from
>> >  the bottom bit of \field{status}.
>> > 
>> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
>> > +buffers of size \field{mtu} to be able to receive at least one receive
>> > +packet with \field{gso_type} NONE or ECN.
>> 
>> Again, this should be the final negotiation instead of the offer,
>> especially as...
>
> Agree here - we also can't declare all existing drivers nonconforming.
>
>
>> > +
>> > +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
>> 
>> ...this is SHOULD and not MUST. The driver should be able to fail
>> negotiating the feature if it cannot fulfill the condition above.
>
> For that to work, MTU must not change before negotiation though.
>
>> > +
>> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit
>> > packets of
>> > +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
>> > +
>> >  \subsubsection{Legacy Interface: Device configuration
>> > layout}\label{sec:Device Types / Network Device / Device
>> > configuration layout / Legacy Interface: Device configuration
>> > layout}
>> >  \label{sec:Device Types / Block Device / Feature bits / Device
>> > configuration layout / Legacy Interface: Device 

Re: [virtio-dev] [PATCH v9] virtio-net: add Max MTU configuration field

[Aaron Conole]

Cornelia Huck  writes:

> On Tue,  6 Sep 2016 10:31:01 -0400
> Aaron Conole  wrote:
>
>  this as this is up for vote>
>
>> diff --git a/content.tex b/content.tex
>> index 4b45678..b90cbad 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -3049,6 +3049,14 @@ features.
>>  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
>>  reconfiguration support.
>> 
>> +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
>> +offered by the device, device advises driver about the value of
>> +MTU to be used. If negotiated, the driver uses \field{mtu} as
>> +the maximum MTU value supplied to the operating system.
>> +
>> +Note: many operating systems override the MTU value provided by the
>> +driver.
>
> I'm wondering: Do we need to distinguish between what the _driver_ does
> and what the _guest_ does, generally speaking?
>
>> +
>>  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
>> 
>>  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
>> @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues 
>> (receiveq1\ldots receiveqN
>>  and transmitq1\ldots transmitqN respectively) that can be configured once 
>> VIRTIO_NET_F_MQ
>>  is negotiated.
>> 
>> +The following driver-read-only field, \field{mtu} only exists if
>> +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the 
>> driver to
>> +use.
>> +
>>  \begin{lstlisting}
>>  struct virtio_net_config {
>>  u8 mac[6];
>>  le16 status;
>>  le16 max_virtqueue_pairs;
>> +le16 mtu;
>>  };
>>  \end{lstlisting}
>> 
>> @@ -3153,6 +3166,18 @@ struct virtio_net_config {
>>  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 
>> inclusive,
>>  if it offers VIRTIO_NET_F_MQ.
>> 
>> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
>> +if it offers VIRTIO_NET_F_MTU.
>> +
>> +The device MUST NOT modify \field{mtu} once it has been set.
>
> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
> successfully negotiated"? I don't think the driver should assume
> anything until after feature negotiation is complete.

I agree; that was always the intent, but the wording does not convey
that.  Apologies.

>> +
>> +The device MUST NOT pass received packets that exceed \field{mtu} size
>> +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
>
> I don't think it should be the offer that is the deciding factor, but
> rather the final negotiation. But maybe we can make this "SHOULD NOT"?

It really needs to be MUST NOT, but as you note above this needs to be
based on the successful negotiation of the VIRTIO_NET_F_MTU feature,
rather than just the offer.

>> +
>> +The device MUST forward transmitted packets of up to MTU size with
>> +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
>> +offers VIRTIO_NET_F_MTU.
>> +
>>  \drivernormative{\subsubsection}{Device configuration
>> layout}{Device Types / Network Device / Device configuration layout}
>> 
>>  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
>> @@ -3165,6 +3190,15 @@ If the driver does not negotiate the
>> VIRTIO_NET_F_STATUS feature, it SHOULD
>>  assume the link is active, otherwise it SHOULD read the link status from
>>  the bottom bit of \field{status}.
>> 
>> +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
>> +buffers of size \field{mtu} to be able to receive at least one receive
>> +packet with \field{gso_type} NONE or ECN.
>
> Again, this should be the final negotiation instead of the offer,
> especially as...
>
>> +
>> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
>
> ...this is SHOULD and not MUST. The driver should be able to fail
> negotiating the feature if it cannot fulfill the condition above.

Agreed with both points.

>> +
>> +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets of
>> +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
>> +
>>  \subsubsection{Legacy Interface: Device configuration
>> layout}\label{sec:Device Types / Network Device / Device
>> configuration layout / Legacy Interface: Device configuration
>> layout}
>>  \label{sec:Device Types / Block Device / Feature bits / Device
>> configuration layout / Legacy Interface: Device configuration
>> layout}
>>  When using the legacy interface, transitional devices and drivers
>
> Again, sorry for being late with comments, but I'll vote with no for
> now. I can still change the vote if you can convince me, though.

No problem, thanks so much for the review.  I'm hoping you would be
convinced to change your vote to yes with the adjusted wording - I'll
publish a v10 next week (to give folks enough time to respond).

-Aaron

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional 

Re: [virtio-dev] [PATCH v9] virtio-net: add Max MTU configuration field

[Michael S. Tsirkin]

On Wed, Sep 07, 2016 at 11:18:22AM +0200, Cornelia Huck wrote:
> On Tue,  6 Sep 2016 10:31:01 -0400
> Aaron Conole  wrote:
> 
>  this as this is up for vote>
> 
> > diff --git a/content.tex b/content.tex
> > index 4b45678..b90cbad 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -3049,6 +3049,14 @@ features.
> >  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
> >  reconfiguration support.
> > 
> > +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
> > +offered by the device, device advises driver about the value of
> > +MTU to be used. If negotiated, the driver uses \field{mtu} as
> > +the maximum MTU value supplied to the operating system.
> > +
> > +Note: many operating systems override the MTU value provided by the
> > +driver.
> 
> I'm wondering: Do we need to distinguish between what the _driver_ does
> and what the _guest_ does, generally speaking?
> 
> > +
> >  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
> > 
> >  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> > @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues 
> > (receiveq1\ldots receiveqN
> >  and transmitq1\ldots transmitqN respectively) that can be configured once 
> > VIRTIO_NET_F_MQ
> >  is negotiated.
> > 
> > +The following driver-read-only field, \field{mtu} only exists if
> > +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the 
> > driver to
> > +use.
> > +
> >  \begin{lstlisting}
> >  struct virtio_net_config {
> >  u8 mac[6];
> >  le16 status;
> >  le16 max_virtqueue_pairs;
> > +le16 mtu;
> >  };
> >  \end{lstlisting}
> > 
> > @@ -3153,6 +3166,18 @@ struct virtio_net_config {
> >  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 
> > inclusive,
> >  if it offers VIRTIO_NET_F_MQ.
> > 
> > +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
> > +if it offers VIRTIO_NET_F_MTU.
> > +
> > +The device MUST NOT modify \field{mtu} once it has been set.
> 
> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
> successfully negotiated"? I don't think the driver should assume
> anything until after feature negotiation is complete.

I don't think this matters much, but generally it's best
if the config space can be read at any time.
For example, this can allow validating MTU and not negotiating
it if it's e.g. too small.
Do you see value in tweaking the MTU after negotiation?


> > +
> > +The device MUST NOT pass received packets that exceed \field{mtu} size
> > +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
> 
> I don't think it should be the offer that is the deciding factor, but
> rather the final negotiation.

Makes sense.

> But maybe we can make this "SHOULD NOT"?

It's important to make this a MUST because otherwise device
needs to do something if it gets a bigger packet -
e.g. fragment it, or drop it.
With this wording, it does not have to.


> > +
> > +The device MUST forward transmitted packets of up to MTU size with
> > +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
> > +offers VIRTIO_NET_F_MTU.
> > +
> >  \drivernormative{\subsubsection}{Device configuration layout}{Device Types 
> > / Network Device / Device configuration layout}
> > 
> >  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> > @@ -3165,6 +3190,15 @@ If the driver does not negotiate the 
> > VIRTIO_NET_F_STATUS feature, it SHOULD
> >  assume the link is active, otherwise it SHOULD read the link status from
> >  the bottom bit of \field{status}.
> > 
> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
> > +buffers of size \field{mtu} to be able to receive at least one receive
> > +packet with \field{gso_type} NONE or ECN.
> 
> Again, this should be the final negotiation instead of the offer,
> especially as...

Agree here - we also can't declare all existing drivers nonconforming.


> > +
> > +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
> 
> ...this is SHOULD and not MUST. The driver should be able to fail
> negotiating the feature if it cannot fulfill the condition above.

For that to work, MTU must not change before negotiation though.

> > +
> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets 
> > of
> > +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
> > +
> >  \subsubsection{Legacy Interface: Device configuration 
> > layout}\label{sec:Device Types / Network Device / Device configuration 
> > layout / Legacy Interface: Device configuration layout}
> >  \label{sec:Device Types / Block Device / Feature bits / Device 
> > configuration layout / Legacy Interface: Device configuration layout}
> >  When using the legacy interface, transitional devices and drivers
> 
> Again, sorry for being late with comments, but I'll vote with no for
> now. I can still change the 

Re: [virtio-dev] [PATCH v9] virtio-net: add Max MTU configuration field

[Cornelia Huck]

On Tue,  6 Sep 2016 10:31:01 -0400
Aaron Conole  wrote:



> diff --git a/content.tex b/content.tex
> index 4b45678..b90cbad 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3049,6 +3049,14 @@ features.
>  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
>  reconfiguration support.
> 
> +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
> +offered by the device, device advises driver about the value of
> +MTU to be used. If negotiated, the driver uses \field{mtu} as
> +the maximum MTU value supplied to the operating system.
> +
> +Note: many operating systems override the MTU value provided by the
> +driver.

I'm wondering: Do we need to distinguish between what the _driver_ does
and what the _guest_ does, generally speaking?

> +
>  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
> 
>  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues 
> (receiveq1\ldots receiveqN
>  and transmitq1\ldots transmitqN respectively) that can be configured once 
> VIRTIO_NET_F_MQ
>  is negotiated.
> 
> +The following driver-read-only field, \field{mtu} only exists if
> +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver 
> to
> +use.
> +
>  \begin{lstlisting}
>  struct virtio_net_config {
>  u8 mac[6];
>  le16 status;
>  le16 max_virtqueue_pairs;
> +le16 mtu;
>  };
>  \end{lstlisting}
> 
> @@ -3153,6 +3166,18 @@ struct virtio_net_config {
>  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 
> inclusive,
>  if it offers VIRTIO_NET_F_MQ.
> 
> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
> +if it offers VIRTIO_NET_F_MTU.
> +
> +The device MUST NOT modify \field{mtu} once it has been set.

Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
successfully negotiated"? I don't think the driver should assume
anything until after feature negotiation is complete.

> +
> +The device MUST NOT pass received packets that exceed \field{mtu} size
> +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.

I don't think it should be the offer that is the deciding factor, but
rather the final negotiation. But maybe we can make this "SHOULD NOT"?

> +
> +The device MUST forward transmitted packets of up to MTU size with
> +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
> +offers VIRTIO_NET_F_MTU.
> +
>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / 
> Network Device / Device configuration layout}
> 
>  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -3165,6 +3190,15 @@ If the driver does not negotiate the 
> VIRTIO_NET_F_STATUS feature, it SHOULD
>  assume the link is active, otherwise it SHOULD read the link status from
>  the bottom bit of \field{status}.
> 
> +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
> +buffers of size \field{mtu} to be able to receive at least one receive
> +packet with \field{gso_type} NONE or ECN.

Again, this should be the final negotiation instead of the offer,
especially as...

> +
> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.

...this is SHOULD and not MUST. The driver should be able to fail
negotiating the feature if it cannot fulfill the condition above.

> +
> +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets of
> +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
> +
>  \subsubsection{Legacy Interface: Device configuration 
> layout}\label{sec:Device Types / Network Device / Device configuration layout 
> / Legacy Interface: Device configuration layout}
>  \label{sec:Device Types / Block Device / Feature bits / Device configuration 
> layout / Legacy Interface: Device configuration layout}
>  When using the legacy interface, transitional devices and drivers

Again, sorry for being late with comments, but I'll vote with no for
now. I can still change the vote if you can convince me, though.


-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

[virtio-dev] Re: [Qemu-devel] [virtio-dev][RFC v3] virtio-sdm: new device specification

[Christian Pinto]

Hello Edgar,

thanks for your comments.


On 06/09/2016 23:43, Edgar E. Iglesias wrote:

Hi,

Sorry for the delay. I have a few questions.

I don't fully understand the purpose of this. Could you elaborate a little on 
that?
You need a real IPI signal to drive this I guess, so it's a little bit of a
chicken and egg problem.


Usually in AMP-like systems CPUs signal each other with IPIs by exploiting
a dedicated hardware peripheral, like a hardware mailbox. So I see the
concept of the SDM (master, slave and communication channels) as the
hardware device to actually implement and deliver the IPIs.
In this case the SDM is designed in a way to be able to deliver multiple
signals (e.g., BOOT, RESET) and not just a single interrupt.


A useful feature I can see is multiplexing a single underlying IPI (driving
an sdm channel) into multiple "virtual" IPI signals. Is that the main
use-case?


In the SDM multiple slaves can connect to the same SDM channel (e.g., 
socket)

and the master uses the channel to deliver the signal (e.g., IRQ) to the
destination slave device. The other way around is also possible, slaves 
signaling

the master.
I guess this is the multiplexing features you were talking about.

However it is worth to notice that one SDM channel is not dedicated to one
specific signal, but rather it is used to deliver all the signals 
implemented

by the device.



Regarding the boot and reset. Typically there's a reset signal you release and
the remote CPU starts running. You can ofcourse reset the CPU and restart
at anytime. Not sure if you need an additional BOOT virtual signal.

There may also be additional mechanisms to control the startup address of the 
remote CPU.
Any thoughts on that?


The purpose of the BOOT signal is also to pass the remote CPU its startup
address. So the idea is to use the payload of the signal to send the 
slave processor
its startup address over the SDM channel and SDM slave device. Our 
implementation

for QEMU uses the BOOT signal for this purpose.

This part of the semantic of the BOOT signal is not described in the 
documentation
since I believe it is a choice of the actual implementation of the SDM 
whether to
pass the startup address or not (i.e. there might be a platform where 
the boot

address of slave processors is somehow hard-coded).
I can add the explanation to the device specification to make it more clear.


Thanks,

Christian



Best regards,
Edgar


On Tue, Aug 30, 2016 at 01:22:26PM +0200, Christian Pinto wrote:

Hello,

are there any comments?


Christian


On 09/08/2016 09:37, Christian Pinto wrote:

This patch adds the specification of the Signal Dristribution Module virtio
device to the current virtio specification document.

Signed-off-by: Christian Pinto 
Signed-off-by: Baptiste Reynal 

---
v2 -> v3:
- Removed master field from configuration and replaced with device_id
- Added new RESET signal
- Added signals definition into device specs
- Added feature bits associated to signals

v1 -> v2:
- Fixed some typos
- Removed dependencies from QEMU
- Added explanation on how SDM can be used in AMP systems
- Explained semantics of payload field in SDMSignalData struct
---
  content.tex|   2 +
  virtio-sdm.tex | 166 +
  2 files changed, 168 insertions(+)
  create mode 100644 virtio-sdm.tex

diff --git a/content.tex b/content.tex
index 3317916..7fcf779 100644
--- a/content.tex
+++ b/content.tex
@@ -5643,6 +5643,8 @@ descriptor for the \field{sense_len}, \field{residual},
  \field{status_qualifier}, \field{status}, \field{response} and
  \field{sense} fields.
+\input{virtio-sdm.tex}
+
  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
  Currently there are three device-independent feature bits defined:
diff --git a/virtio-sdm.tex b/virtio-sdm.tex
new file mode 100644
index 000..ee43e23
--- /dev/null
+++ b/virtio-sdm.tex
@@ -0,0 +1,166 @@
+\section{Signal Distribution Module}\label{sec:Device Types / SDM Device}
+
+The virtio Signal Distribution Module is meant to enable Inter Processor signal
+exchange.
+An example are Inter Processor Interrupts used in AMP systems, for the
+processors involved to notify the presence of new data in the communication
+queues.
+In AMP systems signals are used for various purposes, an example are remoteproc
+or RPMSG. In the former signals are used by a master processor to trigger
+the boot of a slave processor. While the latter, RPMSG, uses virtio queues as a
+message exchange medium between processors. In this case the SDM can be used to
+generate the interrupt associated to the kick of a virtio queue.
+
+There are three signal types supported by the device, namely the
+\textit{IRQ} signal, \textit{BOOT} signal and \textit{RESET} signal. Such set 
of
+signals covers most of the needs of an AMP system, where a master processor can
+trigger the boot or reset of slave processors, and