Re: [PATCH v3 1/4] VSOCK: Introduce virtio-vsock-common.ko
On Thu, Dec 10, 2015 at 10:17:07AM +, Alex Bennée wrote:
> Stefan Hajnoczi writes:
>
> > From: Asias He
> >
> > This module contains the common code and header files for the following
> > virtio-vsock and virtio-vhost kernel modules.
>
> General comment checkpatch has a bunch of warnings about 80 character
> limits, extra braces and BUG_ON usage.
Will fix in the next verison.
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > new file mode 100644
> > index 000..e54eb45
> > --- /dev/null
> > +++ b/include/linux/virtio_vsock.h
> > @@ -0,0 +1,203 @@
> > +/*
> > + * This header, excluding the #ifdef __KERNEL__ part, is BSD licensed so
> > + * anyone can use the definitions to implement compatible
> > drivers/servers:
>
> Is anything in here actually exposed to userspace or the guest? The
> #ifdef __KERNEL__ statement seems redundant for this file at least.
You are right. I think the header was copied from a uapi file.
I'll compare against other virtio code and apply an appropriate header.
> > +void virtio_vsock_dumppkt(const char *func, const struct virtio_vsock_pkt
> > *pkt)
> > +{
> > + pr_debug("%s: pkt=%p, op=%d, len=%d, %d:%d---%d:%d, len=%d\n",
> > +func, pkt,
> > +le16_to_cpu(pkt->hdr.op),
> > +le32_to_cpu(pkt->hdr.len),
> > +le32_to_cpu(pkt->hdr.src_cid),
> > +le32_to_cpu(pkt->hdr.src_port),
> > +le32_to_cpu(pkt->hdr.dst_cid),
> > +le32_to_cpu(pkt->hdr.dst_port),
> > +pkt->len);
> > +}
> > +EXPORT_SYMBOL_GPL(virtio_vsock_dumppkt);
>
> Why export this at all? The only users are in this file so you could
> make it static.
I'll make it static.
> > +u32 virtio_transport_get_credit(struct virtio_transport *trans, u32 credit)
> > +{
> > + u32 ret;
> > +
> > + mutex_lock(&trans->tx_lock);
> > + ret = trans->peer_buf_alloc - (trans->tx_cnt - trans->peer_fwd_cnt);
> > + if (ret > credit)
> > + ret = credit;
> > + trans->tx_cnt += ret;
> > + mutex_unlock(&trans->tx_lock);
> > +
> > + pr_debug("%s: ret=%d, buf_alloc=%d, peer_buf_alloc=%d,"
> > +"tx_cnt=%d, fwd_cnt=%d, peer_fwd_cnt=%d\n", __func__,
>
> I think __func__ is superfluous here as the dynamic print code already
> has it and can print it when required. Having said that there seems to
> be plenty of code already in the kernel that uses __func__ :-/
I'll convert most printks to tracepoints in the next revision.
> > +u64 virtio_transport_get_max_buffer_size(struct vsock_sock *vsk)
> > +{
> > + struct virtio_transport *trans = vsk->trans;
> > +
> > + return trans->buf_size_max;
> > +}
> > +EXPORT_SYMBOL_GPL(virtio_transport_get_max_buffer_size);
>
> All these accesses functions seem pretty simple. Maybe they should be
> inline header functions or even #define macros?
They are used as struct vsock_transport function pointers. What is the
advantage to inlining them?
> > +int virtio_transport_notify_send_post_enqueue(struct vsock_sock *vsk,
> > + ssize_t written, struct vsock_transport_send_notify_data *data)
> > +{
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(virtio_transport_notify_send_post_enqueue);
>
> This makes me wonder if the calling code should be having
> if(transport->fn) checks rather than filling stuff out will null
> implementations but I guess that's a question better aimed at the
> maintainers.
I've considered it too. I'll try to streamline this in the next
revision.
> > +/* We are under the virtio-vsock's vsock->rx_lock or
> > + * vhost-vsock's vq->mutex lock */
> > +void virtio_transport_recv_pkt(struct virtio_vsock_pkt *pkt)
> > +{
> > + struct virtio_transport *trans;
> > + struct sockaddr_vm src, dst;
> > + struct vsock_sock *vsk;
> > + struct sock *sk;
> > +
> > + vsock_addr_init(&src, le32_to_cpu(pkt->hdr.src_cid),
> > le32_to_cpu(pkt->hdr.src_port));
> > + vsock_addr_init(&dst, le32_to_cpu(pkt->hdr.dst_cid),
> > le32_to_cpu(pkt->hdr.dst_port));
> > +
> > + virtio_vsock_dumppkt(__func__, pkt);
> > +
> > + if (le16_to_cpu(pkt->hdr.type) != VIRTIO_VSOCK_TYPE_STREAM) {
> > + /* TODO send RST */
>
> TODO's shouldn't make it into final submissions.
>
> > + goto free_pkt;
> > + }
> > +
> > + /* The socket must be in connected or bound table
> > +* otherwise send reset back
> > +*/
> > + sk = vsock_find_connected_socket(&src, &dst);
> > + if (!sk) {
> > + sk = vsock_find_bound_socket(&dst);
> > + if (!sk) {
> > + pr_debug("%s: can not find bound_socket\n", __func__);
> > + virtio_vsock_dumppkt(__func__, pkt);
> > + /* Ignore this pkt instead of sending reset back */
> > + /* TODO send a RST unless this packet is a RST
> > (to avoid infinite loops) */
>
> Ditto.
Thanks, I'll complete the RST code in the next revision.
signature.asc
Description: PGP signature
Re: [PATCH v3 1/4] VSOCK: Introduce virtio-vsock-common.ko
Stefan Hajnoczi writes:
> From: Asias He
>
> This module contains the common code and header files for the following
> virtio-vsock and virtio-vhost kernel modules.
General comment checkpatch has a bunch of warnings about 80 character
limits, extra braces and BUG_ON usage.
>
> Signed-off-by: Asias He
> Signed-off-by: Stefan Hajnoczi
> ---
> v3:
> * Remove unnecessary 3-way handshake, just do REQUEST/RESPONSE instead
>of REQUEST/RESPONSE/ACK
> * Remove SOCK_DGRAM support and focus on SOCK_STREAM first
> * Only allow host->guest connections (same security model as latest
>VMware)
> v2:
> * Fix peer_buf_alloc inheritance on child socket
> * Notify other side of SOCK_STREAM disconnect (fixes shutdown
>semantics)
> * Avoid recursive mutex_lock(tx_lock) for write_space (fixes deadlock)
> * Define VIRTIO_VSOCK_TYPE_STREAM/DGRAM hardware interface constants
> * Define VIRTIO_VSOCK_SHUTDOWN_RCV/SEND hardware interface constants
> ---
> include/linux/virtio_vsock.h| 203
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_vsock.h | 87
> net/vmw_vsock/virtio_transport_common.c | 854
>
> 4 files changed, 1145 insertions(+)
> create mode 100644 include/linux/virtio_vsock.h
> create mode 100644 include/uapi/linux/virtio_vsock.h
> create mode 100644 net/vmw_vsock/virtio_transport_common.c
>
> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> new file mode 100644
> index 000..e54eb45
> --- /dev/null
> +++ b/include/linux/virtio_vsock.h
> @@ -0,0 +1,203 @@
> +/*
> + * This header, excluding the #ifdef __KERNEL__ part, is BSD licensed so
> + * anyone can use the definitions to implement compatible
> drivers/servers:
Is anything in here actually exposed to userspace or the guest? The
#ifdef __KERNEL__ statement seems redundant for this file at least.
> + *
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *notice, this list of conditions and the following disclaimer in the
> + *documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors
> + *may be used to endorse or promote products derived from this software
> + *without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS
> IS''
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + *
> + * Copyright (C) Red Hat, Inc., 2013-2015
> + * Copyright (C) Asias He , 2013
> + * Copyright (C) Stefan Hajnoczi , 2015
> + */
> +
> +#ifndef _LINUX_VIRTIO_VSOCK_H
> +#define _LINUX_VIRTIO_VSOCK_H
> +
> +#include
> +#include
> +#include
> +
> +#define VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE128
> +#define VIRTIO_VSOCK_DEFAULT_BUF_SIZE(1024 * 256)
> +#define VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE(1024 * 256)
> +#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
> +#define VIRTIO_VSOCK_MAX_BUF_SIZE0xUL
> +#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE(1024 * 64)
> +#define VIRTIO_VSOCK_MAX_TX_BUF_SIZE (1024 * 1024 * 16)
> +#define VIRTIO_VSOCK_MAX_DGRAM_SIZE (1024 * 64)
> +
> +struct vsock_transport_recv_notify_data;
> +struct vsock_transport_send_notify_data;
> +struct sockaddr_vm;
> +struct vsock_sock;
> +
> +enum {
> + VSOCK_VQ_CTRL = 0,
> + VSOCK_VQ_RX = 1, /* for host to guest data */
> + VSOCK_VQ_TX = 2, /* for guest to host data */
> + VSOCK_VQ_MAX= 3,
> +};
> +
> +/* virtio transport socket state */
> +struct virtio_transport {
> + struct virtio_transport_pkt_ops *ops;
> + struct vsock_sock *vsk;
> +
> + u32 buf_size;
> + u32 buf_size_min;
> + u32 buf_size_max;
> +
> + struct mutex tx_lock;
> + struct mutex rx_lock;
> +
> + struct list_head rx_queue;
> + u32 rx_bytes;
> +
> + /* Protected by trans->tx_lock */
> + u32 tx_cnt;
> + u32 buf_alloc;
[PATCH v3 1/4] VSOCK: Introduce virtio-vsock-common.ko
From: Asias He
This module contains the common code and header files for the following
virtio-vsock and virtio-vhost kernel modules.
Signed-off-by: Asias He
Signed-off-by: Stefan Hajnoczi
---
v3:
* Remove unnecessary 3-way handshake, just do REQUEST/RESPONSE instead
of REQUEST/RESPONSE/ACK
* Remove SOCK_DGRAM support and focus on SOCK_STREAM first
* Only allow host->guest connections (same security model as latest
VMware)
v2:
* Fix peer_buf_alloc inheritance on child socket
* Notify other side of SOCK_STREAM disconnect (fixes shutdown
semantics)
* Avoid recursive mutex_lock(tx_lock) for write_space (fixes deadlock)
* Define VIRTIO_VSOCK_TYPE_STREAM/DGRAM hardware interface constants
* Define VIRTIO_VSOCK_SHUTDOWN_RCV/SEND hardware interface constants
---
include/linux/virtio_vsock.h| 203
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_vsock.h | 87
net/vmw_vsock/virtio_transport_common.c | 854
4 files changed, 1145 insertions(+)
create mode 100644 include/linux/virtio_vsock.h
create mode 100644 include/uapi/linux/virtio_vsock.h
create mode 100644 net/vmw_vsock/virtio_transport_common.c
diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
new file mode 100644
index 000..e54eb45
--- /dev/null
+++ b/include/linux/virtio_vsock.h
@@ -0,0 +1,203 @@
+/*
+ * This header, excluding the #ifdef __KERNEL__ part, is BSD licensed so
+ * anyone can use the definitions to implement compatible drivers/servers:
+ *
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of IBM nor the names of its contributors
+ *may be used to endorse or promote products derived from this software
+ *without specific prior written permission.
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS
IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ *
+ * Copyright (C) Red Hat, Inc., 2013-2015
+ * Copyright (C) Asias He , 2013
+ * Copyright (C) Stefan Hajnoczi , 2015
+ */
+
+#ifndef _LINUX_VIRTIO_VSOCK_H
+#define _LINUX_VIRTIO_VSOCK_H
+
+#include
+#include
+#include
+
+#define VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE 128
+#define VIRTIO_VSOCK_DEFAULT_BUF_SIZE (1024 * 256)
+#define VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE (1024 * 256)
+#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
+#define VIRTIO_VSOCK_MAX_BUF_SIZE 0xUL
+#define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64)
+#define VIRTIO_VSOCK_MAX_TX_BUF_SIZE (1024 * 1024 * 16)
+#define VIRTIO_VSOCK_MAX_DGRAM_SIZE(1024 * 64)
+
+struct vsock_transport_recv_notify_data;
+struct vsock_transport_send_notify_data;
+struct sockaddr_vm;
+struct vsock_sock;
+
+enum {
+ VSOCK_VQ_CTRL = 0,
+ VSOCK_VQ_RX = 1, /* for host to guest data */
+ VSOCK_VQ_TX = 2, /* for guest to host data */
+ VSOCK_VQ_MAX= 3,
+};
+
+/* virtio transport socket state */
+struct virtio_transport {
+ struct virtio_transport_pkt_ops *ops;
+ struct vsock_sock *vsk;
+
+ u32 buf_size;
+ u32 buf_size_min;
+ u32 buf_size_max;
+
+ struct mutex tx_lock;
+ struct mutex rx_lock;
+
+ struct list_head rx_queue;
+ u32 rx_bytes;
+
+ /* Protected by trans->tx_lock */
+ u32 tx_cnt;
+ u32 buf_alloc;
+ u32 peer_fwd_cnt;
+ u32 peer_buf_alloc;
+ /* Protected by trans->rx_lock */
+ u32 fwd_cnt;
+};
+
+struct virtio_vsock_pkt {
+ struct virtio_vsock_hdr hdr;
+ struct virtio_transport *trans;
+ struct work_struct work;
+ struct list_head list;
+ void *buf;
+ u32 len;
+ u32 off;
+};
+
+struct virtio_vsock_pkt_info {
+ u32 remote_cid, remote_port;
+ struct msghdr *msg;
+ u32 pkt_len;
+ u16
