Message ID | 1511859789-43680-4-git-send-email-arei.gonglei@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 28/11/2017 10:03, Gonglei wrote: > Introduce two vhost-user meassges: VHOST_USER_CREATE_CRYPTO_SESSION > and VHOST_USER_CLOSE_CRYPTO_SESSION. At this point, the QEMU side > support crypto operation in cryptodev host-user backend. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > Signed-off-by: Zhoujian <jianjay.zhou@huawei.com> > --- > backends/cryptodev-vhost-user.c | 50 +++++++++++++++++----- > docs/interop/vhost-user.txt | 19 +++++++++ > hw/virtio/vhost-user.c | 89 +++++++++++++++++++++++++++++++++++++++ > include/hw/virtio/vhost-backend.h | 8 ++++ > 4 files changed, 155 insertions(+), 11 deletions(-) As far as I understand, VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is called as a result of sending a message on the control virtqueue. Why can't vhost-user also process create/destroy session messages on the control virtqueue, instead of having device-specific messages in the protocol? Thanks, Paolo
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Tuesday, November 28, 2017 6:02 PM > To: Gonglei (Arei); qemu-devel@nongnu.org > Cc: mst@redhat.com; Huangweidong (C); stefanha@redhat.com; Zhoujian > (jay); pasic@linux.vnet.ibm.com; longpeng; xin.zeng@intel.com; > roy.fan.zhang@intel.com > Subject: Re: [PATCH 3/4] cryptodev-vhost-user: add crypto session handler > > On 28/11/2017 10:03, Gonglei wrote: > > Introduce two vhost-user meassges: > VHOST_USER_CREATE_CRYPTO_SESSION > > and VHOST_USER_CLOSE_CRYPTO_SESSION. At this point, the QEMU side > > support crypto operation in cryptodev host-user backend. > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > > Signed-off-by: Zhoujian <jianjay.zhou@huawei.com> > > --- > > backends/cryptodev-vhost-user.c | 50 +++++++++++++++++----- > > docs/interop/vhost-user.txt | 19 +++++++++ > > hw/virtio/vhost-user.c | 89 > +++++++++++++++++++++++++++++++++++++++ > > include/hw/virtio/vhost-backend.h | 8 ++++ > > 4 files changed, 155 insertions(+), 11 deletions(-) > > As far as I understand, VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is called as > a result of sending a message on the control virtqueue. > VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is a message type of control queue, Means creating a session for next crypto requests. > Why can't vhost-user also process create/destroy session messages on the > control virtqueue, instead of having device-specific messages in the > protocol? > You mean we can share control virtqueue to DPDK as well? Like data queues? Maybe I don't get your point. :( Thanks, -Gonglei
On 28/11/2017 11:43, Gonglei (Arei) wrote: >> As far as I understand, VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is called as >> a result of sending a message on the control virtqueue. > > VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is a message type of control queue, > Means creating a session for next crypto requests. Ok, so the message does have the same meaning as the control queue message. Thanks for confirming. >> Why can't vhost-user also process create/destroy session messages on the >> control virtqueue, instead of having device-specific messages in the >> protocol? > > You mean we can share control virtqueue to DPDK as well? Like data queues? I don't know :) but why not? Thanks, Paolo
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Tuesday, November 28, 2017 6:46 PM > To: Gonglei (Arei); qemu-devel@nongnu.org > Cc: mst@redhat.com; Huangweidong (C); stefanha@redhat.com; Zhoujian > (jay); pasic@linux.vnet.ibm.com; longpeng; xin.zeng@intel.com; > roy.fan.zhang@intel.com > Subject: Re: [PATCH 3/4] cryptodev-vhost-user: add crypto session handler > > On 28/11/2017 11:43, Gonglei (Arei) wrote: > >> As far as I understand, VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is called > as > >> a result of sending a message on the control virtqueue. > > > > VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is a message type of control > queue, > > Means creating a session for next crypto requests. > > Ok, so the message does have the same meaning as the control queue > message. Thanks for confirming. > > >> Why can't vhost-user also process create/destroy session messages on the > >> control virtqueue, instead of having device-specific messages in the > >> protocol? > > > > You mean we can share control virtqueue to DPDK as well? Like data queues? > > I don't know :) but why not? > Current there are two main reasons for this design: 1) we should use another cpu to polling the control virtqueue, which is expensive. 2) we should copy the logic of parsing control message to DPDK, which break current layered architecture . I'm not sure if there are any other hidden issues for future scalability, such as using Qemu to manage some control messages, avoiding D-Dos attack etc. Thanks, -Gonglei
On 28/11/2017 12:06, Gonglei (Arei) wrote: >>> You mean we can share control virtqueue to DPDK as well? Like data queues? >> I don't know :) but why not? >> > Current there are two main reasons for this design: > > 1) we should use another cpu to polling the control virtqueue, which is expensive. IIRC DPDK also supports interrupt mode, doesn't it? Is it possible to do interrupt mode for some virtqueues and poll mode for others? > 2) we should copy the logic of parsing control message to DPDK, which break > current layered architecture . But isn't it already a layering violation that you're adding *some* control messages to the vhost-user protocol? I am not sure why only these two are necessary. Paolo > I'm not sure if there are any other hidden issues for future scalability, such as > using Qemu to manage some control messages, avoiding D-Dos attack etc. > > Thanks, > -Gonglei
On Tue, Nov 28, 2017 at 11:45:31AM +0100, Paolo Bonzini wrote: > On 28/11/2017 11:43, Gonglei (Arei) wrote: > >> As far as I understand, VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is called as > >> a result of sending a message on the control virtqueue. > > > > VIRTIO_CRYPTO_CIPHER_CREATE_SESSION is a message type of control queue, > > Means creating a session for next crypto requests. > > Ok, so the message does have the same meaning as the control queue > message. Thanks for confirming. > > >> Why can't vhost-user also process create/destroy session messages on the > >> control virtqueue, instead of having device-specific messages in the > >> protocol? > > > > You mean we can share control virtqueue to DPDK as well? Like data queues? > > I don't know :) but why not? > > Thanks, > > Paolo There are advantages to parsing the control queue in QEMU, e.g. it then has visibility into which sessions exist. It's a question of what works and what are the driver's expectations. E.g. for network guests tend to poll the control queue so handling that queue in QEMU helps us to keep guest going even if backend disconnects.
> -----Original Message----- > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > Sent: Tuesday, November 28, 2017 7:20 PM > To: Gonglei (Arei); qemu-devel@nongnu.org > Cc: mst@redhat.com; Huangweidong (C); stefanha@redhat.com; Zhoujian > (jay); pasic@linux.vnet.ibm.com; longpeng; xin.zeng@intel.com; > roy.fan.zhang@intel.com > Subject: Re: [PATCH 3/4] cryptodev-vhost-user: add crypto session handler > > On 28/11/2017 12:06, Gonglei (Arei) wrote: > >>> You mean we can share control virtqueue to DPDK as well? Like data > queues? > >> I don't know :) but why not? > >> > > Current there are two main reasons for this design: > > > > 1) we should use another cpu to polling the control virtqueue, which is > expensive. > > IIRC DPDK also supports interrupt mode, doesn't it? Is it possible to > do interrupt mode for some virtqueues and poll mode for others? > The intel guy Tan (Ccing) said to me: " Interrupt mode for vhost-user is still not supported in current implementation. But we are evaluating the necessity now. And yes, the mode (polling or interrupt) can be different for different queues." > > 2) we should copy the logic of parsing control message to DPDK, which break > > current layered architecture . > > But isn't it already a layering violation that you're adding *some* > control messages to the vhost-user protocol? I am not sure why only > these two are necessary. > Sorry, but I don't think this is layering violation, just like "vhost_net_set_mtu" for vhost-net and "vhost_vsock_set_guest_cid_op" for vhost_vsock. They're all device-specific messages. Aren't they? Thanks, -Gonglei > Paolo > > > I'm not sure if there are any other hidden issues for future scalability, such as > > using Qemu to manage some control messages, avoiding D-Dos attack etc. > > > > Thanks, > > -Gonglei
On Wed, Nov 29, 2017 at 02:33:59AM +0000, Gonglei (Arei) wrote: > > > -----Original Message----- > > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > > Sent: Tuesday, November 28, 2017 7:20 PM > > To: Gonglei (Arei); qemu-devel@nongnu.org > > Cc: mst@redhat.com; Huangweidong (C); stefanha@redhat.com; Zhoujian > > (jay); pasic@linux.vnet.ibm.com; longpeng; xin.zeng@intel.com; > > roy.fan.zhang@intel.com > > Subject: Re: [PATCH 3/4] cryptodev-vhost-user: add crypto session handler > > > > On 28/11/2017 12:06, Gonglei (Arei) wrote: > > >>> You mean we can share control virtqueue to DPDK as well? Like data > > queues? > > >> I don't know :) but why not? > > >> > > > Current there are two main reasons for this design: > > > > > > 1) we should use another cpu to polling the control virtqueue, which is > > expensive. > > > > IIRC DPDK also supports interrupt mode, doesn't it? Is it possible to > > do interrupt mode for some virtqueues and poll mode for others? > > > > The intel guy Tan (Ccing) said to me: > > " Interrupt mode for vhost-user is still not supported in current > implementation. But we are evaluating the necessity now. That's more or less a spec violation. Guest must get interrupts if it does not disable them. And it must notify host if host does not disable notifications. > And yes, the mode (polling or interrupt) can be different for different > queues." > > > > 2) we should copy the logic of parsing control message to DPDK, which break > > > current layered architecture . > > > > But isn't it already a layering violation that you're adding *some* > > control messages to the vhost-user protocol? I am not sure why only > > these two are necessary. > > > Sorry, but I don't think this is layering violation, just like "vhost_net_set_mtu" > for vhost-net and "vhost_vsock_set_guest_cid_op" for vhost_vsock. They're all > device-specific messages. Aren't they? > > Thanks, > -Gonglei > > > Paolo > > > > > I'm not sure if there are any other hidden issues for future scalability, such as > > > using Qemu to manage some control messages, avoiding D-Dos attack etc. > > > > > > Thanks, > > > -Gonglei >
On 29/11/2017 05:10, Michael S. Tsirkin wrote: >> " Interrupt mode for vhost-user is still not supported in current >> implementation. But we are evaluating the necessity now. > > That's more or less a spec violation. Guest must get interrupts > if it does not disable them. And it must notify host > if host does not disable notifications. I understood that as "the vhost-user server is always using poll mode for the virtqueues" and cannot use e.g. ioeventfds. Thanks, Paolo
On 11/29/2017 5:11 PM, Paolo Bonzini wrote: > On 29/11/2017 05:10, Michael S. Tsirkin wrote: >>> " Interrupt mode for vhost-user is still not supported in current >>> implementation. But we are evaluating the necessity now. >> That's more or less a spec violation. Guest must get interrupts >> if it does not disable them. And it must notify host >> if host does not disable notifications. By that, I mean the transmission from virtio to vhost, if virtio is not actively sending packets, at vhost side, we can stop polling from that vhost port (i.e., virtio); if VM wants to send packets, besides putting packets on virtqueue, it also kicks the backend through pio/mmio. So no need to modify spec. > I understood that as "the vhost-user server is always using poll mode > for the virtqueues" and cannot use e.g. ioeventfds. We can change to work like this: stop polling, and wait for the eventfd, and then get back to polling. Thanks, Jianfeng > > Thanks, > > Paolo
diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c index 0b1f049..061c0e5 100644 --- a/backends/cryptodev-vhost-user.c +++ b/backends/cryptodev-vhost-user.c @@ -233,7 +233,26 @@ static int64_t cryptodev_vhost_user_sym_create_session( CryptoDevBackendSymSessionInfo *sess_info, uint32_t queue_index, Error **errp) { - return 0; + CryptoDevBackendClient *cc = + backend->conf.peers.ccs[queue_index]; + CryptoDevBackendVhost *vhost_crypto; + uint64_t session_id = 0; + int ret; + + vhost_crypto = cryptodev_vhost_user_get_vhost(cc, + backend, queue_index); + if (vhost_crypto) { + struct vhost_dev *dev = &(vhost_crypto->dev); + ret = dev->vhost_ops->vhost_crypto_create_session(dev, + sess_info, + &session_id); + if (ret < 0) { + return -1; + } else { + return session_id; + } + } + return -1; } static int cryptodev_vhost_user_sym_close_session( @@ -241,15 +260,24 @@ static int cryptodev_vhost_user_sym_close_session( uint64_t session_id, uint32_t queue_index, Error **errp) { - return 0; -} - -static int cryptodev_vhost_user_sym_operation( - CryptoDevBackend *backend, - CryptoDevBackendSymOpInfo *op_info, - uint32_t queue_index, Error **errp) -{ - return VIRTIO_CRYPTO_OK; + CryptoDevBackendClient *cc = + backend->conf.peers.ccs[queue_index]; + CryptoDevBackendVhost *vhost_crypto; + int ret; + + vhost_crypto = cryptodev_vhost_user_get_vhost(cc, + backend, queue_index); + if (vhost_crypto) { + struct vhost_dev *dev = &(vhost_crypto->dev); + ret = dev->vhost_ops->vhost_crypto_close_session(dev, + session_id); + if (ret < 0) { + return -1; + } else { + return 0; + } + } + return -1; } static void cryptodev_vhost_user_cleanup( @@ -328,7 +356,7 @@ cryptodev_vhost_user_class_init(ObjectClass *oc, void *data) bc->cleanup = cryptodev_vhost_user_cleanup; bc->create_session = cryptodev_vhost_user_sym_create_session; bc->close_session = cryptodev_vhost_user_sym_close_session; - bc->do_sym_op = cryptodev_vhost_user_sym_operation; + bc->do_sym_op = NULL; } static const TypeInfo cryptodev_vhost_user_info = { diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt index 954771d..f43c63d 100644 --- a/docs/interop/vhost-user.txt +++ b/docs/interop/vhost-user.txt @@ -596,6 +596,25 @@ Master message types and expect this message once (per VQ) during device configuration (ie. before the master starts the VQ). + * VHOST_USER_CREATE_CRYPTO_SESSION + + Id: 23 + Equivalent ioctl: N/A + Master payload: crypto session description + Slave payload: crypto session description + + Create a session for crypto operation. The server side must return the + session id, 0 or positive for success, negative for failure. + + * VHOST_USER_CLOSE_CRYPTO_SESSION + + Id: 24 + Equivalent ioctl: N/A + Master payload: u64 + + Close a session for crypto operation which was previously + created by VHOST_USER_CREATE_CRYPTO_SESSION. + Slave message types ------------------- diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 093675e..7865c6d 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -17,6 +17,7 @@ #include "sysemu/kvm.h" #include "qemu/error-report.h" #include "qemu/sockets.h" +#include "sysemu/cryptodev.h" #include <sys/ioctl.h> #include <sys/socket.h> @@ -65,6 +66,8 @@ typedef enum VhostUserRequest { VHOST_USER_SET_SLAVE_REQ_FD = 21, VHOST_USER_IOTLB_MSG = 22, VHOST_USER_SET_VRING_ENDIAN = 23, + VHOST_USER_CREATE_CRYPTO_SESSION = 24, + VHOST_USER_CLOSE_CRYPTO_SESSION = 25, VHOST_USER_MAX } VhostUserRequest; @@ -92,6 +95,17 @@ typedef struct VhostUserLog { uint64_t mmap_offset; } VhostUserLog; +#define VHOST_CRYPTO_SYM_HMAC_MAX_KEY_LEN 512 +#define VHOST_CRYPTO_SYM_CIPHER_MAX_KEY_LEN 64 + +typedef struct VhostUserCryptoSession { + /* session id for success, -1 on errors */ + int64_t session_id; + CryptoDevBackendSymSessionInfo session_setup_data; + uint8_t key[VHOST_CRYPTO_SYM_CIPHER_MAX_KEY_LEN]; + uint8_t auth_key[VHOST_CRYPTO_SYM_HMAC_MAX_KEY_LEN]; +} VhostUserCryptoSession; + typedef struct VhostUserMsg { VhostUserRequest request; @@ -109,6 +123,7 @@ typedef struct VhostUserMsg { VhostUserMemory memory; VhostUserLog log; struct vhost_iotlb_msg iotlb; + VhostUserCryptoSession session; } payload; } QEMU_PACKED VhostUserMsg; @@ -922,6 +937,78 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) /* No-op as the receive channel is not dedicated to IOTLB messages. */ } +static int vhost_user_crypto_create_session(struct vhost_dev *dev, + void *session_info, + uint64_t *session_id) +{ + CryptoDevBackendSymSessionInfo *sess_info = session_info; + VhostUserMsg msg = { + .request = VHOST_USER_CREATE_CRYPTO_SESSION, + .flags = VHOST_USER_VERSION, + .size = sizeof(msg.payload.session), + }; + + assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER); + + memcpy(&msg.payload.session.session_setup_data, sess_info, + sizeof(CryptoDevBackendSymSessionInfo)); + if (sess_info->key_len) { + memcpy(&msg.payload.session.key, sess_info->cipher_key, + sess_info->key_len); + } + if (sess_info->auth_key_len > 0) { + memcpy(&msg.payload.session.auth_key, sess_info->auth_key, + sess_info->auth_key_len); + } + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + error_report("vhost_user_write() return -1, create session failed"); + return -1; + } + + if (vhost_user_read(dev, &msg) < 0) { + error_report("vhost_user_read() return -1, create session failed"); + return -1; + } + + if (msg.request != VHOST_USER_CREATE_CRYPTO_SESSION) { + error_report("Received unexpected msg type. Expected %d received %d", + VHOST_USER_CREATE_CRYPTO_SESSION, msg.request); + return -1; + } + + if (msg.size != sizeof(msg.payload.session)) { + error_report("Received bad msg size."); + return -1; + } + + if (msg.payload.session.session_id < 0) { + error_report("Bad session id: %" PRId64 "", + msg.payload.session.session_id); + return -1; + } + *session_id = msg.payload.session.session_id; + + return 0; +} + +static int +vhost_user_crypto_close_session(struct vhost_dev *dev, uint64_t session_id) +{ + VhostUserMsg msg = { + .request = VHOST_USER_CLOSE_CRYPTO_SESSION, + .flags = VHOST_USER_VERSION, + .size = sizeof(msg.payload.u64), + }; + msg.payload.u64 = session_id; + + if (vhost_user_write(dev, &msg, NULL, 0) < 0) { + error_report("vhost_user_write() return -1, close session failed"); + return -1; + } + + return 0; +} + const VhostOps user_ops = { .backend_type = VHOST_BACKEND_TYPE_USER, .vhost_backend_init = vhost_user_init, @@ -948,4 +1035,6 @@ const VhostOps user_ops = { .vhost_net_set_mtu = vhost_user_net_set_mtu, .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback, .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg, + .vhost_crypto_create_session = vhost_user_crypto_create_session, + .vhost_crypto_close_session = vhost_user_crypto_close_session, }; diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index a7a5f22..21d9479 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -85,6 +85,12 @@ typedef void (*vhost_set_iotlb_callback_op)(struct vhost_dev *dev, typedef int (*vhost_send_device_iotlb_msg_op)(struct vhost_dev *dev, struct vhost_iotlb_msg *imsg); +typedef int (*vhost_crypto_create_session_op)(struct vhost_dev *dev, + void *session_info, + uint64_t *session_id); +typedef int (*vhost_crypto_close_session_op)(struct vhost_dev *dev, + uint64_t session_id); + typedef struct VhostOps { VhostBackendType backend_type; vhost_backend_init vhost_backend_init; @@ -118,6 +124,8 @@ typedef struct VhostOps { vhost_vsock_set_running_op vhost_vsock_set_running; vhost_set_iotlb_callback_op vhost_set_iotlb_callback; vhost_send_device_iotlb_msg_op vhost_send_device_iotlb_msg; + vhost_crypto_create_session_op vhost_crypto_create_session; + vhost_crypto_close_session_op vhost_crypto_close_session; } VhostOps; extern const VhostOps user_ops;