Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642026 > unrolled thread

[PATCH 01/18] xen: introduce the pvcalls interface header

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 01/18] xen: introduce the pvcalls interface header Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 14/18] xen/pvcalls: disconnect and module_exit Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 02/18] xen/pvcalls: introduce the pvcalls xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 15/18] xen/pvcalls: introduce the ioworker Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 09/18] xen/pvcalls: implement bind command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 03/18] xen/pvcalls: initialize the module and register the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the  xenbus backend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 03:50 +0200
        Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:10 +0200
          Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:30 +0200
      Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the  xenbus backend Juergen Gross <jgross@suse.com> - 2017-05-16 08:50 +0200
        Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:00 +0200
          Re: [PATCH 03/18] xen/pvcalls: initialize the module and register the  xenbus backend Juergen Gross <jgross@suse.com> - 2017-05-17 07:30 +0200
            Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-18 23:20 +0200
              Re: [PATCH 03/18] xen/pvcalls: initialize the module and register  the xenbus backend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-20 00:40 +0200
    [PATCH 10/18] xen/pvcalls: implement listen command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 13/18] xen/pvcalls: implement release command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 06/18] xen/pvcalls: handle commands from the frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 06/18] xen/pvcalls: handle commands from the frontend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:20 +0200
        Re: [PATCH 06/18] xen/pvcalls: handle commands from the frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 23:00 +0200
    [PATCH 07/18] xen/pvcalls: implement socket command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 07/18] xen/pvcalls: implement socket command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:30 +0200
        Re: [PATCH 07/18] xen/pvcalls: implement socket command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:50 +0200
    [PATCH 12/18] xen/pvcalls: implement poll command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 16/18] xen/pvcalls: implement read Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 08/18] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
      Re: [PATCH 08/18] xen/pvcalls: implement connect command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:50 +0200
        Re: [PATCH 08/18] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 23:10 +0200
          Re: [PATCH 08/18] xen/pvcalls: implement connect command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-17 00:00 +0200
            Re: [PATCH 08/18] xen/pvcalls: implement connect command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-18 21:20 +0200
              Re: [PATCH 08/18] xen/pvcalls: implement connect command Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-18 22:30 +0200
    [PATCH 11/18] xen/pvcalls: implement accept command Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:40 +0200
    [PATCH 05/18] xen/pvcalls: connect to a frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:50 +0200
      Re: [PATCH 05/18] xen/pvcalls: connect to a frontend Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 04:10 +0200
        Re: [PATCH 05/18] xen/pvcalls: connect to a frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:30 +0200
          Re: [PATCH 05/18] xen/pvcalls: connect to a frontend Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:40 +0200
    [PATCH 04/18] xen/pvcalls: xenbus state handling Stefano Stabellini <sstabellini@kernel.org> - 2017-05-15 22:50 +0200
      Re: [PATCH 04/18] xen/pvcalls: xenbus state handling Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-05-16 03:50 +0200
        Re: [PATCH 04/18] xen/pvcalls: xenbus state handling Stefano Stabellini <sstabellini@kernel.org> - 2017-05-16 22:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1642186 — Re: [PATCH 07/18] xen/pvcalls: implement socket command

On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> Just reply with success to the other end for now. Delay the allocation
> of the actual socket to bind and/or connect.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-back.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index 2b2a49a..2eae096 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -12,12 +12,17 @@
>   * GNU General Public License for more details.
>   */
>
> +#include <linux/inet.h>
>  #include <linux/kthread.h>
>  #include <linux/list.h>
>  #include <linux/radix-tree.h>
>  #include <linux/module.h>
>  #include <linux/rwsem.h>
>  #include <linux/wait.h>
> +#include <net/sock.h>
> +#include <net/inet_common.h>
> +#include <net/inet_connection_sock.h>
> +#include <net/request_sock.h>
>
>  #include <xen/events.h>
>  #include <xen/grant_table.h>
> @@ -65,7 +70,31 @@ static void pvcalls_back_ioworker(struct work_struct *work)
>  static int pvcalls_back_socket(struct xenbus_device *dev,
>  		struct xen_pvcalls_request *req)
>  {
> -	return 0;
> +	struct pvcalls_back_priv *priv;
> +	int ret;
> +	struct xen_pvcalls_response *rsp;
> +
> +	if (dev == NULL)
> +		return 0;
> +	priv = dev_get_drvdata(&dev->dev);

This is inconsistent with pvcalls_back_event() tests, where you check 
both for NULL. OTOH, I am not sure a check is needed at all since you've 
just tested these in pvcalls_back_event().


-boris

> +
> +	if (req->u.socket.domain != AF_INET ||
> +	    req->u.socket.type != SOCK_STREAM ||
> +	    (req->u.socket.protocol != 0 &&
> +	     req->u.socket.protocol != AF_INET))
> +		ret = -EAFNOSUPPORT;
> +	else
> +		ret = 0;
> +
> +	/* leave the actual socket allocation for later */
> +
> +	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
> +	rsp->req_id = req->req_id;
> +	rsp->cmd = req->cmd;
> +	rsp->u.socket.id = req->u.socket.id;
> +	rsp->ret = ret;
> +
> +	return 1;
>  }
>
>  static int pvcalls_back_connect(struct xenbus_device *dev,
>

[toc] | [prev] | [next] | [standalone]

#1642821 — Re: [PATCH 07/18] xen/pvcalls: implement socket command

On Mon, 15 May 2017, Boris Ostrovsky wrote:
> On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> > Just reply with success to the other end for now. Delay the allocation
> > of the actual socket to bind and/or connect.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >  drivers/xen/pvcalls-back.c | 31 ++++++++++++++++++++++++++++++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > index 2b2a49a..2eae096 100644
> > --- a/drivers/xen/pvcalls-back.c
> > +++ b/drivers/xen/pvcalls-back.c
> > @@ -12,12 +12,17 @@
> >   * GNU General Public License for more details.
> >   */
> > 
> > +#include <linux/inet.h>
> >  #include <linux/kthread.h>
> >  #include <linux/list.h>
> >  #include <linux/radix-tree.h>
> >  #include <linux/module.h>
> >  #include <linux/rwsem.h>
> >  #include <linux/wait.h>
> > +#include <net/sock.h>
> > +#include <net/inet_common.h>
> > +#include <net/inet_connection_sock.h>
> > +#include <net/request_sock.h>
> > 
> >  #include <xen/events.h>
> >  #include <xen/grant_table.h>
> > @@ -65,7 +70,31 @@ static void pvcalls_back_ioworker(struct work_struct
> > *work)
> >  static int pvcalls_back_socket(struct xenbus_device *dev,
> >  		struct xen_pvcalls_request *req)
> >  {
> > -	return 0;
> > +	struct pvcalls_back_priv *priv;
> > +	int ret;
> > +	struct xen_pvcalls_response *rsp;
> > +
> > +	if (dev == NULL)
> > +		return 0;
> > +	priv = dev_get_drvdata(&dev->dev);
> 
> This is inconsistent with pvcalls_back_event() tests, where you check both for
> NULL. OTOH, I am not sure a check is needed at all since you've just tested
> these in pvcalls_back_event().

That's because priv cannot be NULL at this stage: it was allocated
before queuing any work to priv->wq. While in pvcalls_back_event I have
been more careful in case of spurious (erroneous) notifications.

I agree that I could remove this dev == NULL check here, and in the
other command handlers. I'll do that.


> > +
> > +	if (req->u.socket.domain != AF_INET ||
> > +	    req->u.socket.type != SOCK_STREAM ||
> > +	    (req->u.socket.protocol != 0 &&
> > +	     req->u.socket.protocol != AF_INET))
> > +		ret = -EAFNOSUPPORT;
> > +	else
> > +		ret = 0;
> > +
> > +	/* leave the actual socket allocation for later */
> > +
> > +	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
> > +	rsp->req_id = req->req_id;
> > +	rsp->cmd = req->cmd;
> > +	rsp->u.socket.id = req->u.socket.id;
> > +	rsp->ret = ret;
> > +
> > +	return 1;
> >  }
> > 
> >  static int pvcalls_back_connect(struct xenbus_device *dev,
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1642039 — [PATCH 12/18] xen/pvcalls: implement poll command

Implement poll on passive sockets by requesting a delayed response with
mappass->reqcopy, and reply back when there is data on the passive
socket.

Poll on active socket is unimplemented as by the spec, as the frontend
should just wait for events and check the indexes on the indexes page.

Only support one outstanding poll (or accept) request for every passive
socket at any given time.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index d8e0a60..d5b7412 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -381,11 +381,30 @@ static void __pvcalls_back_accept(struct work_struct *work)
 static void pvcalls_pass_sk_data_ready(struct sock *sock)
 {
 	struct sockpass_mapping *mappass = sock->sk_user_data;
+	struct pvcalls_back_priv *priv;
+	struct xen_pvcalls_response *rsp;
+	unsigned long flags;
+	int notify;
 
 	if (mappass == NULL)
 		return;
 
-	queue_work(mappass->wq, &mappass->register_work);
+	priv = mappass->priv;
+	spin_lock_irqsave(&mappass->copy_lock, flags);
+	if (mappass->reqcopy.cmd == PVCALLS_POLL) {
+		rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+		rsp->req_id = mappass->reqcopy.req_id;
+		rsp->u.poll.id = mappass->reqcopy.u.poll.id;
+		rsp->cmd = mappass->reqcopy.cmd;
+		rsp->ret = 0;
+
+		mappass->reqcopy.cmd = 0;
+		RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&priv->ring, notify);
+		if (notify)
+			notify_remote_via_irq(mappass->priv->irq);
+	} else
+		queue_work(mappass->wq, &mappass->register_work);
+	spin_unlock_irqrestore(&mappass->copy_lock, flags);
 }
 
 static int pvcalls_back_bind(struct xenbus_device *dev,
@@ -534,6 +553,55 @@ static int pvcalls_back_accept(struct xenbus_device *dev,
 static int pvcalls_back_poll(struct xenbus_device *dev,
 			     struct xen_pvcalls_request *req)
 {
+	struct pvcalls_back_priv *priv;
+	struct sockpass_mapping *mappass;
+	struct xen_pvcalls_response *rsp;
+	struct inet_connection_sock *icsk;
+	struct request_sock_queue *queue;
+	unsigned long flags;
+	int ret;
+	bool data;
+
+	if (dev == NULL)
+		return 0;
+	priv = dev_get_drvdata(&dev->dev);
+
+	mappass = radix_tree_lookup(&priv->socketpass_mappings, req->u.poll.id);
+	if (mappass == NULL)
+		return 0;
+
+	/*
+	 * Limitation of the current implementation: only support one
+	 * concurrent accept or poll call on one socket.
+	 */
+	spin_lock_irqsave(&mappass->copy_lock, flags);
+	if (mappass->reqcopy.cmd != 0) {
+		ret = -EINTR;
+		goto out;
+	}
+
+	mappass->reqcopy = *req;
+	lock_sock(mappass->sock->sk);
+	icsk = inet_csk(mappass->sock->sk);
+	queue = &icsk->icsk_accept_queue;
+	data = queue->rskq_accept_head != NULL;
+	release_sock(mappass->sock->sk);
+	if (data) {
+		mappass->reqcopy.cmd = 0;
+		ret = 0;
+		goto out;
+	}
+
+	return 0;
+
+out:
+	spin_unlock_irqrestore(&mappass->copy_lock, flags);
+
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->cmd = req->cmd;
+	rsp->u.poll.id = req->u.poll.id;
+	rsp->ret = ret;
 	return 0;
 }
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642040 — [PATCH 16/18] xen/pvcalls: implement read

When an active socket has data available, add the relative sock_mapping
to the ioworker list, increment the io and read counters, and schedule
the ioworker.

Implement the read function by reading from the socket, writing the data
to the data ring.

Set in_error on error.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index db3e02c..0f715a8 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -102,6 +102,79 @@ static int pvcalls_back_release_active(struct xenbus_device *dev,
 
 static void pvcalls_conn_back_read(unsigned long opaque)
 {
+	struct sock_mapping *map = (struct sock_mapping *)opaque;
+	struct msghdr msg;
+	struct kvec vec[2];
+	RING_IDX cons, prod, size, wanted, array_size, masked_prod, masked_cons;
+	int32_t error;
+	struct pvcalls_data_intf *intf = map->ring;
+	struct pvcalls_data *data = &map->data;
+	int ret;
+
+	array_size = XEN_FLEX_RING_SIZE(map->ring_order);
+	cons = intf->in_cons;
+	prod = intf->in_prod;
+	error = intf->in_error;
+	/* read the indexes first, then deal with the data */
+	virt_mb();
+
+	if (error)
+		return;
+
+	size = pvcalls_queued(prod, cons, array_size);
+	if (size >= array_size)
+		return;
+	lock_sock(map->sock->sk);
+	if (skb_queue_empty(&map->sock->sk->sk_receive_queue)) {
+		atomic_set(&map->read, 0);
+		release_sock(map->sock->sk);
+		return;
+	}
+	release_sock(map->sock->sk);
+	wanted = array_size - size;
+	masked_prod = pvcalls_mask(prod, array_size);
+	masked_cons = pvcalls_mask(cons, array_size);
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg_iter.type = ITER_KVEC|WRITE;
+	msg.msg_iter.count = wanted;
+	if (masked_prod < masked_cons) {
+		vec[0].iov_base = data->in + masked_prod;
+		vec[0].iov_len = wanted;
+		msg.msg_iter.kvec = vec;
+		msg.msg_iter.nr_segs = 1;
+	} else {
+		vec[0].iov_base = data->in + masked_prod;
+		vec[0].iov_len = array_size - masked_prod;
+		vec[1].iov_base = data->in;
+		vec[1].iov_len = wanted - vec[0].iov_len;
+		msg.msg_iter.kvec = vec;
+		msg.msg_iter.nr_segs = 2;
+	}
+
+	atomic_set(&map->read, 0);
+	ret = inet_recvmsg(map->sock, &msg, wanted, MSG_DONTWAIT);
+	WARN_ON(ret > 0 && ret > wanted);
+	if (ret == -EAGAIN) /* shouldn't happen */
+		return;
+	if (!ret)
+		ret = -ENOTCONN;
+	lock_sock(map->sock->sk);
+	if (ret > 0 && !skb_queue_empty(&map->sock->sk->sk_receive_queue))
+		atomic_inc(&map->read);
+	release_sock(map->sock->sk);
+
+	/* write the data, then modify the indexes */
+	virt_wmb();
+	if (ret < 0)
+		intf->in_error = ret;
+	else
+		intf->in_prod = prod + ret;
+	/* update the indexes, then notify the other end */
+	virt_wmb();
+	notify_remote_via_irq(map->irq);
+
+	return;
 }
 
 static int pvcalls_conn_back_write(struct sock_mapping *map)
@@ -192,6 +265,22 @@ static void pvcalls_sk_state_change(struct sock *sock)
 
 static void pvcalls_sk_data_ready(struct sock *sock)
 {
+	struct sock_mapping *map = sock->sk_user_data;
+	struct pvcalls_ioworker *iow;
+	unsigned long flags;
+
+	if (map == NULL)
+		return;
+
+	iow = &pvcalls_back_global.ioworkers[map->data_worker];
+	spin_lock_irqsave(&iow->lock, flags);
+	atomic_inc(&map->read);
+	if (list_empty(&map->queue))
+		list_add_tail(&map->queue, &iow->wqs);
+	spin_unlock_irqrestore(&iow->lock, flags);
+	atomic_inc(&iow->io);
+	queue_work_on(map->data_worker, pvcalls_back_global.wq,
+		&iow->register_work);
 }
 
 static int pvcalls_back_connect(struct xenbus_device *dev,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642041 — [PATCH 08/18] xen/pvcalls: implement connect command

Allocate a socket. Keep track of socket <-> ring mappings with a new data
structure, called sock_mapping. Implement the connect command by calling
inet_stream_connect, and mapping the new indexes page and data ring.
Associate the socket to an ioworker randomly.

When an active socket is closed (sk_state_change), set in_error to
-ENOTCONN and notify the other end, as specified by the protocol.

sk_data_ready will be implemented later.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 145 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 2eae096..9ac1cf2 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -63,6 +63,29 @@ struct pvcalls_back_priv {
 	struct work_struct register_work;
 };
 
+struct sock_mapping {
+	struct list_head list;
+	struct list_head queue;
+	struct pvcalls_back_priv *priv;
+	struct socket *sock;
+	int data_worker;
+	uint64_t id;
+	grant_ref_t ref;
+	struct pvcalls_data_intf *ring;
+	void *bytes;
+	struct pvcalls_data data;
+	uint32_t ring_order;
+	int irq;
+	atomic_t read;
+	atomic_t write;
+	atomic_t release;
+	void (*saved_data_ready)(struct sock *sk);
+};
+
+static irqreturn_t pvcalls_back_conn_event(int irq, void *sock_map);
+static int pvcalls_back_release_active(struct xenbus_device *dev,
+				       struct pvcalls_back_priv *priv,
+				       struct sock_mapping *map);
 static void pvcalls_back_ioworker(struct work_struct *work)
 {
 }
@@ -97,9 +120,126 @@ static int pvcalls_back_socket(struct xenbus_device *dev,
 	return 1;
 }
 
+static void pvcalls_sk_state_change(struct sock *sock)
+{
+	struct sock_mapping *map = sock->sk_user_data;
+	struct pvcalls_data_intf *intf;
+
+	if (map == NULL)
+		return;
+
+	intf = map->ring;
+	intf->in_error = -ENOTCONN;
+	notify_remote_via_irq(map->irq);
+}
+
+static void pvcalls_sk_data_ready(struct sock *sock)
+{
+}
+
 static int pvcalls_back_connect(struct xenbus_device *dev,
 				struct xen_pvcalls_request *req)
 {
+	struct pvcalls_back_priv *priv;
+	int ret;
+	struct socket *sock;
+	struct sock_mapping *map = NULL;
+	void *page;
+	struct xen_pvcalls_response *rsp;
+
+	if (dev == NULL)
+		return 0;
+	priv = dev_get_drvdata(&dev->dev);
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (map == NULL) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	ret = sock_create(AF_INET, SOCK_STREAM, 0, &sock);
+	if (ret < 0) {
+		kfree(map);
+		goto out;
+	}
+	INIT_LIST_HEAD(&map->queue);
+	map->data_worker = get_random_int() % pvcalls_back_global.nr_ioworkers;
+
+	map->priv = priv;
+	map->sock = sock;
+	map->id = req->u.connect.id;
+	map->ref = req->u.connect.ref;
+
+	ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page);
+	if (ret < 0) {
+		sock_release(map->sock);
+		kfree(map);
+		goto out;
+	}
+	map->ring = page;
+	map->ring_order = map->ring->ring_order;
+	/* first read the order, then map the data ring */
+	virt_rmb();
+	if (map->ring_order > MAX_RING_ORDER) {
+		ret = -EFAULT;
+		goto out;
+	}
+	ret = xenbus_map_ring_valloc(dev, map->ring->ref,
+				     (1 << map->ring_order), &page);
+	if (ret < 0) {
+		sock_release(map->sock);
+		xenbus_unmap_ring_vfree(dev, map->ring);
+		kfree(map);
+		goto out;
+	}
+	map->bytes = page;
+
+	ret = bind_interdomain_evtchn_to_irqhandler(priv->dev->otherend_id,
+						    req->u.connect.evtchn,
+						    pvcalls_back_conn_event,
+						    0,
+						    "pvcalls-backend",
+						    map);
+	if (ret < 0) {
+		sock_release(map->sock);
+		kfree(map);
+		goto out;
+	}
+	map->irq = ret;
+
+	map->data.in = map->bytes;
+	map->data.out = map->bytes + XEN_FLEX_RING_SIZE(map->ring_order);
+
+	down_write(&priv->pvcallss_lock);
+	list_add_tail(&map->list, &priv->socket_mappings);
+	up_write(&priv->pvcallss_lock);
+
+	ret = inet_stream_connect(sock, (struct sockaddr *)&req->u.connect.addr,
+				  req->u.connect.len, req->u.connect.flags);
+	if (ret < 0) {
+		pvcalls_back_release_active(dev, priv, map);
+	} else {
+		lock_sock(sock->sk);
+		map->saved_data_ready = sock->sk->sk_data_ready;
+		sock->sk->sk_user_data = map;
+		sock->sk->sk_data_ready = pvcalls_sk_data_ready;
+		sock->sk->sk_state_change = pvcalls_sk_state_change;
+		release_sock(sock->sk);
+	}
+
+out:
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->cmd = req->cmd;
+	rsp->u.connect.id = req->u.connect.id;
+	rsp->ret = ret;
+
+	return 1;
+}
+
+static int pvcalls_back_release_active(struct xenbus_device *dev,
+				       struct pvcalls_back_priv *priv,
+				       struct sock_mapping *map)
+{
 	return 0;
 }
 
@@ -215,6 +355,11 @@ static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t pvcalls_back_conn_event(int irq, void *sock_map)
+{
+	return IRQ_HANDLED;
+}
+
 static int backend_connect(struct xenbus_device *dev)
 {
 	int err, evtchn;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642192 — Re: [PATCH 08/18] xen/pvcalls: implement connect command

On 05/15/2017 04:36 PM, Stefano Stabellini wrote:
> Allocate a socket. Keep track of socket <-> ring mappings with a new data
> structure, called sock_mapping. Implement the connect command by calling
> inet_stream_connect, and mapping the new indexes page and data ring.
> Associate the socket to an ioworker randomly.
>
> When an active socket is closed (sk_state_change), set in_error to
> -ENOTCONN and notify the other end, as specified by the protocol.
>
> sk_data_ready will be implemented later.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-back.c | 145 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index 2eae096..9ac1cf2 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -63,6 +63,29 @@ struct pvcalls_back_priv {
>  	struct work_struct register_work;
>  };
>
> +struct sock_mapping {
> +	struct list_head list;
> +	struct list_head queue;

Since you have two lists it would be helpful if names were a bit more 
descriptive.

(and comments for at least some fields would be welcome too)

> +	struct pvcalls_back_priv *priv;
> +	struct socket *sock;
> +	int data_worker;
> +	uint64_t id;
> +	grant_ref_t ref;
> +	struct pvcalls_data_intf *ring;
> +	void *bytes;
> +	struct pvcalls_data data;
> +	uint32_t ring_order;
> +	int irq;
> +	atomic_t read;
> +	atomic_t write;
> +	atomic_t release;
> +	void (*saved_data_ready)(struct sock *sk);
> +};
> +
> +static irqreturn_t pvcalls_back_conn_event(int irq, void *sock_map);
> +static int pvcalls_back_release_active(struct xenbus_device *dev,
> +				       struct pvcalls_back_priv *priv,
> +				       struct sock_mapping *map);
>  static void pvcalls_back_ioworker(struct work_struct *work)
>  {
>  }
> @@ -97,9 +120,126 @@ static int pvcalls_back_socket(struct xenbus_device *dev,
>  	return 1;
>  }
>
> +static void pvcalls_sk_state_change(struct sock *sock)
> +{
> +	struct sock_mapping *map = sock->sk_user_data;
> +	struct pvcalls_data_intf *intf;
> +
> +	if (map == NULL)
> +		return;
> +
> +	intf = map->ring;
> +	intf->in_error = -ENOTCONN;
> +	notify_remote_via_irq(map->irq);
> +}
> +
> +static void pvcalls_sk_data_ready(struct sock *sock)
> +{
> +}
> +
>  static int pvcalls_back_connect(struct xenbus_device *dev,
>  				struct xen_pvcalls_request *req)
>  {
> +	struct pvcalls_back_priv *priv;
> +	int ret;
> +	struct socket *sock;
> +	struct sock_mapping *map = NULL;
> +	void *page;
> +	struct xen_pvcalls_response *rsp;
> +
> +	if (dev == NULL)
> +		return 0;
> +	priv = dev_get_drvdata(&dev->dev);
> +
> +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> +	if (map == NULL) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	ret = sock_create(AF_INET, SOCK_STREAM, 0, &sock);
> +	if (ret < 0) {
> +		kfree(map);
> +		goto out;
> +	}
> +	INIT_LIST_HEAD(&map->queue);
> +	map->data_worker = get_random_int() % pvcalls_back_global.nr_ioworkers;
> +
> +	map->priv = priv;
> +	map->sock = sock;
> +	map->id = req->u.connect.id;
> +	map->ref = req->u.connect.ref;
> +
> +	ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page);
> +	if (ret < 0) {
> +		sock_release(map->sock);
> +		kfree(map);
> +		goto out;
> +	}
> +	map->ring = page;
> +	map->ring_order = map->ring->ring_order;
> +	/* first read the order, then map the data ring */
> +	virt_rmb();


Not sure I understand what the barrier is for here. I don't think 
compiler will reorder ring_order access with the call.


> +	if (map->ring_order > MAX_RING_ORDER) {
> +		ret = -EFAULT;
> +		goto out;
> +	}

If the barrier is indeed needed this check belongs before it.

-boris


> +	ret = xenbus_map_ring_valloc(dev, map->ring->ref,
> +				     (1 << map->ring_order), &page);
> +	if (ret < 0) {
> +		sock_release(map->sock);
> +		xenbus_unmap_ring_vfree(dev, map->ring);
> +		kfree(map);
> +		goto out;
> +	}
> +	map->bytes = page;
> +
> +	ret = bind_interdomain_evtchn_to_irqhandler(priv->dev->otherend_id,
> +						    req->u.connect.evtchn,
> +						    pvcalls_back_conn_event,
> +						    0,
> +						    "pvcalls-backend",
> +						    map);
> +	if (ret < 0) {
> +		sock_release(map->sock);
> +		kfree(map);
> +		goto out;
> +	}
> +	map->irq = ret;
> +
> +	map->data.in = map->bytes;
> +	map->data.out = map->bytes + XEN_FLEX_RING_SIZE(map->ring_order);
> +
> +	down_write(&priv->pvcallss_lock);
> +	list_add_tail(&map->list, &priv->socket_mappings);
> +	up_write(&priv->pvcallss_lock);
> +
> +	ret = inet_stream_connect(sock, (struct sockaddr *)&req->u.connect.addr,
> +				  req->u.connect.len, req->u.connect.flags);
> +	if (ret < 0) {
> +		pvcalls_back_release_active(dev, priv, map);
> +	} else {
> +		lock_sock(sock->sk);
> +		map->saved_data_ready = sock->sk->sk_data_ready;
> +		sock->sk->sk_user_data = map;
> +		sock->sk->sk_data_ready = pvcalls_sk_data_ready;
> +		sock->sk->sk_state_change = pvcalls_sk_state_change;
> +		release_sock(sock->sk);
> +	}
> +
> +out:
> +	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
> +	rsp->req_id = req->req_id;
> +	rsp->cmd = req->cmd;
> +	rsp->u.connect.id = req->u.connect.id;
> +	rsp->ret = ret;
> +
> +	return 1;
> +}
> +
> +static int pvcalls_back_release_active(struct xenbus_device *dev,
> +				       struct pvcalls_back_priv *priv,
> +				       struct sock_mapping *map)
> +{
>  	return 0;
>  }
>
> @@ -215,6 +355,11 @@ static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>
> +static irqreturn_t pvcalls_back_conn_event(int irq, void *sock_map)
> +{
> +	return IRQ_HANDLED;
> +}
> +
>  static int backend_connect(struct xenbus_device *dev)
>  {
>  	int err, evtchn;
>

[toc] | [prev] | [next] | [standalone]

#1642826 — Re: [PATCH 08/18] xen/pvcalls: implement connect command

On Mon, 15 May 2017, Boris Ostrovsky wrote:
> On 05/15/2017 04:36 PM, Stefano Stabellini wrote:
> > Allocate a socket. Keep track of socket <-> ring mappings with a new data
> > structure, called sock_mapping. Implement the connect command by calling
> > inet_stream_connect, and mapping the new indexes page and data ring.
> > Associate the socket to an ioworker randomly.
> > 
> > When an active socket is closed (sk_state_change), set in_error to
> > -ENOTCONN and notify the other end, as specified by the protocol.
> > 
> > sk_data_ready will be implemented later.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >  drivers/xen/pvcalls-back.c | 145
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 145 insertions(+)
> > 
> > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > index 2eae096..9ac1cf2 100644
> > --- a/drivers/xen/pvcalls-back.c
> > +++ b/drivers/xen/pvcalls-back.c
> > @@ -63,6 +63,29 @@ struct pvcalls_back_priv {
> >  	struct work_struct register_work;
> >  };
> > 
> > +struct sock_mapping {
> > +	struct list_head list;
> > +	struct list_head queue;
> 
> Since you have two lists it would be helpful if names were a bit more
> descriptive.
> 
> (and comments for at least some fields would be welcome too)

Yeah, you are right. list is used to add sock_mapping to
priv->socket_mappings, the per frontend list of active sockets. queue is
used to add sock_mapping to the ioworker list. I'll add a comment.


> > +	struct pvcalls_back_priv *priv;
> > +	struct socket *sock;
> > +	int data_worker;
> > +	uint64_t id;
> > +	grant_ref_t ref;
> > +	struct pvcalls_data_intf *ring;
> > +	void *bytes;
> > +	struct pvcalls_data data;
> > +	uint32_t ring_order;
> > +	int irq;
> > +	atomic_t read;
> > +	atomic_t write;
> > +	atomic_t release;
> > +	void (*saved_data_ready)(struct sock *sk);
> > +};
> > +
> > +static irqreturn_t pvcalls_back_conn_event(int irq, void *sock_map);
> > +static int pvcalls_back_release_active(struct xenbus_device *dev,
> > +				       struct pvcalls_back_priv *priv,
> > +				       struct sock_mapping *map);
> >  static void pvcalls_back_ioworker(struct work_struct *work)
> >  {
> >  }
> > @@ -97,9 +120,126 @@ static int pvcalls_back_socket(struct xenbus_device
> > *dev,
> >  	return 1;
> >  }
> > 
> > +static void pvcalls_sk_state_change(struct sock *sock)
> > +{
> > +	struct sock_mapping *map = sock->sk_user_data;
> > +	struct pvcalls_data_intf *intf;
> > +
> > +	if (map == NULL)
> > +		return;
> > +
> > +	intf = map->ring;
> > +	intf->in_error = -ENOTCONN;
> > +	notify_remote_via_irq(map->irq);
> > +}
> > +
> > +static void pvcalls_sk_data_ready(struct sock *sock)
> > +{
> > +}
> > +
> >  static int pvcalls_back_connect(struct xenbus_device *dev,
> >  				struct xen_pvcalls_request *req)
> >  {
> > +	struct pvcalls_back_priv *priv;
> > +	int ret;
> > +	struct socket *sock;
> > +	struct sock_mapping *map = NULL;
> > +	void *page;
> > +	struct xen_pvcalls_response *rsp;
> > +
> > +	if (dev == NULL)
> > +		return 0;
> > +	priv = dev_get_drvdata(&dev->dev);
> > +
> > +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> > +	if (map == NULL) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +	ret = sock_create(AF_INET, SOCK_STREAM, 0, &sock);
> > +	if (ret < 0) {
> > +		kfree(map);
> > +		goto out;
> > +	}
> > +	INIT_LIST_HEAD(&map->queue);
> > +	map->data_worker = get_random_int() %
> > pvcalls_back_global.nr_ioworkers;
> > +
> > +	map->priv = priv;
> > +	map->sock = sock;
> > +	map->id = req->u.connect.id;
> > +	map->ref = req->u.connect.ref;
> > +
> > +	ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page);
> > +	if (ret < 0) {
> > +		sock_release(map->sock);
> > +		kfree(map);
> > +		goto out;
> > +	}
> > +	map->ring = page;
> > +	map->ring_order = map->ring->ring_order;
> > +	/* first read the order, then map the data ring */
> > +	virt_rmb();
> 
> 
> Not sure I understand what the barrier is for here. I don't think compiler
> will reorder ring_order access with the call.

It's to avoid using the live version of ring_order to map the data ring
pages (the other end could be changing that value at any time). We want
to be sure that the compiler doesn't optimize out map->ring_order and
use map->ring->ring_order instead.


> > +	if (map->ring_order > MAX_RING_ORDER) {
> > +		ret = -EFAULT;
> > +		goto out;
> > +	}
> 
> If the barrier is indeed needed this check belongs before it.

I don't think so, see above.


> 
> 
> > +	ret = xenbus_map_ring_valloc(dev, map->ring->ref,
> > +				     (1 << map->ring_order), &page);
> > +	if (ret < 0) {
> > +		sock_release(map->sock);
> > +		xenbus_unmap_ring_vfree(dev, map->ring);
> > +		kfree(map);
> > +		goto out;
> > +	}
> > +	map->bytes = page;
> > +
> > +	ret = bind_interdomain_evtchn_to_irqhandler(priv->dev->otherend_id,
> > +						    req->u.connect.evtchn,
> > +						    pvcalls_back_conn_event,
> > +						    0,
> > +						    "pvcalls-backend",
> > +						    map);
> > +	if (ret < 0) {
> > +		sock_release(map->sock);
> > +		kfree(map);
> > +		goto out;
> > +	}
> > +	map->irq = ret;
> > +
> > +	map->data.in = map->bytes;
> > +	map->data.out = map->bytes + XEN_FLEX_RING_SIZE(map->ring_order);
> > +
> > +	down_write(&priv->pvcallss_lock);
> > +	list_add_tail(&map->list, &priv->socket_mappings);
> > +	up_write(&priv->pvcallss_lock);
> > +
> > +	ret = inet_stream_connect(sock, (struct sockaddr
> > *)&req->u.connect.addr,
> > +				  req->u.connect.len, req->u.connect.flags);
> > +	if (ret < 0) {
> > +		pvcalls_back_release_active(dev, priv, map);
> > +	} else {
> > +		lock_sock(sock->sk);
> > +		map->saved_data_ready = sock->sk->sk_data_ready;
> > +		sock->sk->sk_user_data = map;
> > +		sock->sk->sk_data_ready = pvcalls_sk_data_ready;
> > +		sock->sk->sk_state_change = pvcalls_sk_state_change;
> > +		release_sock(sock->sk);
> > +	}
> > +
> > +out:
> > +	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
> > +	rsp->req_id = req->req_id;
> > +	rsp->cmd = req->cmd;
> > +	rsp->u.connect.id = req->u.connect.id;
> > +	rsp->ret = ret;
> > +
> > +	return 1;
> > +}
> > +
> > +static int pvcalls_back_release_active(struct xenbus_device *dev,
> > +				       struct pvcalls_back_priv *priv,
> > +				       struct sock_mapping *map)
> > +{
> >  	return 0;
> >  }
> > 
> > @@ -215,6 +355,11 @@ static irqreturn_t pvcalls_back_event(int irq, void
> > *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> > 
> > +static irqreturn_t pvcalls_back_conn_event(int irq, void *sock_map)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int backend_connect(struct xenbus_device *dev)
> >  {
> >  	int err, evtchn;
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1642854 — Re: [PATCH 08/18] xen/pvcalls: implement connect command

>>> +	ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page);
>>> +	if (ret < 0) {
>>> +		sock_release(map->sock);
>>> +		kfree(map);
>>> +		goto out;
>>> +	}
>>> +	map->ring = page;
>>> +	map->ring_order = map->ring->ring_order;
>>> +	/* first read the order, then map the data ring */
>>> +	virt_rmb();
>>
>> Not sure I understand what the barrier is for here. I don't think compiler
>> will reorder ring_order access with the call.
> It's to avoid using the live version of ring_order to map the data ring
> pages (the other end could be changing that value at any time). We want
> to be sure that the compiler doesn't optimize out map->ring_order and
> use map->ring->ring_order instead.

Wouldn't WRITE_ONCE(map->ring_order, map->ring->ring_order) be the right
primitive then?

And also: if the other side changes ring size, what are we mapping then?
It's obsolete by now.

-boris

>
>
>>> +	if (map->ring_order > MAX_RING_ORDER) {
>>> +		ret = -EFAULT;
>>> +		goto out;
>>> +	}
>> If the barrier is indeed needed this check belongs before it.
> I don't think so, see above.
>
>
>>
>>> +	ret = xenbus_map_ring_valloc(dev, map->ring->ref,
>>> +				     (1 << map->ring_order), &page);
>>> +	if (ret < 0) {
>>> +		sock_release(map->sock);
>>> +		xenbus_unmap_ring_vfree(dev, map->ring);
>>> +		kfree(map);
>>> +		goto out;
>>> +	}
>>> +	map->bytes = page;
>>>

[toc] | [prev] | [next] | [standalone]

#1644850 — Re: [PATCH 08/18] xen/pvcalls: implement connect command

On Tue, 16 May 2017, Boris Ostrovsky wrote:
> >>> +	ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page);
> >>> +	if (ret < 0) {
> >>> +		sock_release(map->sock);
> >>> +		kfree(map);
> >>> +		goto out;
> >>> +	}
> >>> +	map->ring = page;
> >>> +	map->ring_order = map->ring->ring_order;
> >>> +	/* first read the order, then map the data ring */
> >>> +	virt_rmb();
> >>
> >> Not sure I understand what the barrier is for here. I don't think compiler
> >> will reorder ring_order access with the call.
> > It's to avoid using the live version of ring_order to map the data ring
> > pages (the other end could be changing that value at any time). We want
> > to be sure that the compiler doesn't optimize out map->ring_order and
> > use map->ring->ring_order instead.
> 
> Wouldn't WRITE_ONCE(map->ring_order, map->ring->ring_order) be the right
> primitive then?

It doesn't have to be atomic, because right after the assignment we
check if map->ring_order is an appropriate value (see below).


> And also: if the other side changes ring size, what are we mapping then?
> It's obsolete by now.

If the grants are wrong, the mapping hypercalls will fail, the same way
they do with any of the other PV frontends/backends today. That is not
the problem we are trying to address with the barrier.

The issue is here is that by runtime changes to map->ring->ring_order,
the frontend could issue a denial of service by getting the backend into
a busyloop. You can imagine that:

  for (i = 0; i < map->ring->ring_order; i++) {

might not work as the backend expects if map->ring->ring_order can
change at any time.

One could say that the code is already written this way:

  for (i = 0; i < map->ring_order; i++) {

So what's the problem? We have seen instances in the past of the
compiler "optimizing" things in a way that actually the assembly did:

  for (i = 0; i < map->ring->ring_order; i++) {

This is why I put a barrier there, to avoid such compiler
"optimizations". Does it make sense?


> >>> +	if (map->ring_order > MAX_RING_ORDER) {
> >>> +		ret = -EFAULT;
> >>> +		goto out;
> >>> +	}
> >> If the barrier is indeed needed this check belongs before it.
> > I don't think so, see above.
> >
> >
> >>
> >>> +	ret = xenbus_map_ring_valloc(dev, map->ring->ref,
> >>> +				     (1 << map->ring_order), &page);
> >>> +	if (ret < 0) {
> >>> +		sock_release(map->sock);
> >>> +		xenbus_unmap_ring_vfree(dev, map->ring);
> >>> +		kfree(map);
> >>> +		goto out;
> >>> +	}
> >>> +	map->bytes = page;
> >>>
> 

[toc] | [prev] | [next] | [standalone]

#1644909 — Re: [PATCH 08/18] xen/pvcalls: implement connect command

On 05/18/2017 03:10 PM, Stefano Stabellini wrote:
> On Tue, 16 May 2017, Boris Ostrovsky wrote:
>>>>> +	ret = xenbus_map_ring_valloc(dev, &req->u.connect.ref, 1, &page);
>>>>> +	if (ret < 0) {
>>>>> +		sock_release(map->sock);
>>>>> +		kfree(map);
>>>>> +		goto out;
>>>>> +	}
>>>>> +	map->ring = page;
>>>>> +	map->ring_order = map->ring->ring_order;
>>>>> +	/* first read the order, then map the data ring */
>>>>> +	virt_rmb();
>>>> Not sure I understand what the barrier is for here. I don't think compiler
>>>> will reorder ring_order access with the call.
>>> It's to avoid using the live version of ring_order to map the data ring
>>> pages (the other end could be changing that value at any time). We want
>>> to be sure that the compiler doesn't optimize out map->ring_order and
>>> use map->ring->ring_order instead.
>> Wouldn't WRITE_ONCE(map->ring_order, map->ring->ring_order) be the right
>> primitive then?
> It doesn't have to be atomic, because right after the assignment we
> check if map->ring_order is an appropriate value (see below).

WRITE_ONCE() is not about atomicity, it's about not allowing compilers
get too aggressive.

>
>
>> And also: if the other side changes ring size, what are we mapping then?
>> It's obsolete by now.
> If the grants are wrong, the mapping hypercalls will fail, the same way
> they do with any of the other PV frontends/backends today. That is not
> the problem we are trying to address with the barrier.
>
> The issue is here is that by runtime changes to map->ring->ring_order,
> the frontend could issue a denial of service by getting the backend into
> a busyloop. You can imagine that:
>
>   for (i = 0; i < map->ring->ring_order; i++) {
>
> might not work as the backend expects if map->ring->ring_order can
> change at any time.
>
> One could say that the code is already written this way:
>
>   for (i = 0; i < map->ring_order; i++) {
>
> So what's the problem? We have seen instances in the past of the
> compiler "optimizing" things in a way that actually the assembly did:
>
>   for (i = 0; i < map->ring->ring_order; i++) {
>
> This is why I put a barrier there, to avoid such compiler
> "optimizations". Does it make sense?

Right, I understand all this. I thought you meant that changing
ring_order was part of normal operation (i.e. somewhat expected) and I
couldn't see how that would work.

Thanks for taking time to write this down.

-boris

>
>
>>>>> +	if (map->ring_order > MAX_RING_ORDER) {
>>>>> +		ret = -EFAULT;
>>>>> +		goto out;
>>>>> +	}
>>>> If the barrier is indeed needed this check belongs before it.
>>> I don't think so, see above.
>>>
>>>
>>>>> +	ret = xenbus_map_ring_valloc(dev, map->ring->ref,
>>>>> +				     (1 << map->ring_order), &page);
>>>>> +	if (ret < 0) {
>>>>> +		sock_release(map->sock);
>>>>> +		xenbus_unmap_ring_vfree(dev, map->ring);
>>>>> +		kfree(map);
>>>>> +		goto out;
>>>>> +	}
>>>>> +	map->bytes = page;
>>>>>

[toc] | [prev] | [next] | [standalone]

#1642042 — [PATCH 11/18] xen/pvcalls: implement accept command

Implement the accept command by calling inet_accept. To avoid blocking
in the kernel, call inet_accept(O_NONBLOCK) from a workqueue, which get
scheduled on sk_data_ready (for a passive socket, it means that there
are connections to accept).

Use the reqcopy field to store the request. Accept the new socket from
the delayed work function, create a new sock_mapping for it, map
the indexes page and data ring, and reply to the other end. Choose an
ioworker for the socket randomly.

Only support one outstanding blocking accept request for every socket at
any time.

Add a field to sock_mapping to remember the passive socket from which an
active socket was created.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 156 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index a762877..d8e0a60 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -67,6 +67,7 @@ struct sock_mapping {
 	struct list_head list;
 	struct list_head queue;
 	struct pvcalls_back_priv *priv;
+	struct sockpass_mapping *sockpass;
 	struct socket *sock;
 	int data_worker;
 	uint64_t id;
@@ -263,10 +264,128 @@ static int pvcalls_back_release(struct xenbus_device *dev,
 
 static void __pvcalls_back_accept(struct work_struct *work)
 {
+	struct sockpass_mapping *mappass = container_of(
+		work, struct sockpass_mapping, register_work);
+	struct sock_mapping *map;
+	struct pvcalls_ioworker *iow;
+	struct pvcalls_back_priv *priv;
+	struct xen_pvcalls_response *rsp;
+	struct xen_pvcalls_request *req;
+	void *page = NULL;
+	int notify;
+	int ret = -EINVAL;
+	unsigned long flags;
+
+	priv = mappass->priv;
+	/* We only need to check the value of "cmd" atomically on read. */
+	spin_lock_irqsave(&mappass->copy_lock, flags);
+	req = &mappass->reqcopy;
+	if (req->cmd != PVCALLS_ACCEPT) {
+		spin_unlock_irqrestore(&mappass->copy_lock, flags);
+		return;
+	}
+	spin_unlock_irqrestore(&mappass->copy_lock, flags);
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (map == NULL) {
+		ret = -ENOMEM;
+		goto out_error;
+	}
+
+	map->sock = sock_alloc();
+	if (!map->sock)
+		goto out_error;
+
+	INIT_LIST_HEAD(&map->queue);
+	map->data_worker = get_random_int() % pvcalls_back_global.nr_ioworkers;
+	map->ref = req->u.accept.ref;
+
+	map->priv = priv;
+	map->sockpass = mappass;
+	map->sock->type = mappass->sock->type;
+	map->sock->ops = mappass->sock->ops;
+	map->id = req->u.accept.id_new;
+
+	ret = xenbus_map_ring_valloc(priv->dev, &req->u.accept.ref, 1, &page);
+	if (ret < 0)
+		goto out_error;
+	map->ring = page;
+	map->ring_order = map->ring->ring_order;
+	/* first read the order, then map the data ring */
+	virt_rmb();
+	if (map->ring_order > MAX_RING_ORDER) {
+		ret = -EFAULT;
+		goto out_error;
+	}
+	ret = xenbus_map_ring_valloc(priv->dev, map->ring->ref,
+				     (1 << map->ring_order), &page);
+	if (ret < 0)
+		goto out_error;
+	map->bytes = page;
+
+	ret = bind_interdomain_evtchn_to_irqhandler(priv->dev->otherend_id,
+						    req->u.accept.evtchn,
+						    pvcalls_back_conn_event,
+						    0,
+						    "pvcalls-backend",
+						    map);
+	if (ret < 0)
+		goto out_error;
+	map->irq = ret;
+
+	map->data.in = map->bytes;
+	map->data.out = map->bytes + XEN_FLEX_RING_SIZE(map->ring_order);
+
+	down_write(&priv->pvcallss_lock);
+	list_add_tail(&map->list, &priv->socket_mappings);
+	up_write(&priv->pvcallss_lock);
+
+	ret = inet_accept(mappass->sock, map->sock, O_NONBLOCK, true);
+	if (ret == -EAGAIN)
+		goto out_error;
+
+	lock_sock(map->sock->sk);
+	map->saved_data_ready = map->sock->sk->sk_data_ready;
+	map->sock->sk->sk_user_data = map;
+	map->sock->sk->sk_data_ready = pvcalls_sk_data_ready;
+	map->sock->sk->sk_state_change = pvcalls_sk_state_change;
+	release_sock(map->sock->sk);
+
+	iow = &pvcalls_back_global.ioworkers[map->data_worker];
+	spin_lock_irqsave(&iow->lock, flags);
+	atomic_inc(&map->read);
+	if (list_empty(&map->queue))
+		list_add_tail(&map->queue, &iow->wqs);
+	spin_unlock_irqrestore( &iow->lock, flags);
+	atomic_inc(&iow->io);
+	queue_work_on(map->data_worker, pvcalls_back_global.wq, &iow->register_work);
+
+out_error:
+	if (ret < 0)
+		pvcalls_back_release_active(priv->dev, priv, map);
+
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->cmd = req->cmd;
+	rsp->u.accept.id = req->u.accept.id;
+	rsp->ret = ret;
+	RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&priv->ring, notify);
+	if (notify)
+		notify_remote_via_irq(priv->irq);
+
+	spin_lock_irqsave(&mappass->copy_lock, flags);
+	mappass->reqcopy.cmd = 0;
+	spin_unlock_irqrestore(&mappass->copy_lock, flags);
 }
 
 static void pvcalls_pass_sk_data_ready(struct sock *sock)
 {
+	struct sockpass_mapping *mappass = sock->sk_user_data;
+
+	if (mappass == NULL)
+		return;
+
+	queue_work(mappass->wq, &mappass->register_work);
 }
 
 static int pvcalls_back_bind(struct xenbus_device *dev,
@@ -372,7 +491,44 @@ static int pvcalls_back_listen(struct xenbus_device *dev,
 static int pvcalls_back_accept(struct xenbus_device *dev,
 			       struct xen_pvcalls_request *req)
 {
+	struct pvcalls_back_priv *priv;
+	struct sockpass_mapping *mappass;
+	int ret = -EINVAL;
+	struct xen_pvcalls_response *rsp;
+	unsigned long flags;
+
+	if (dev == NULL)
+		return 0;
+	priv = dev_get_drvdata(&dev->dev);
+
+	mappass = radix_tree_lookup(&priv->socketpass_mappings,
+		req->u.accept.id);
+	if (mappass == NULL)
+		goto out_error;
+
+	/* 
+	 * Limitation of the current implementation: only support one
+	 * concurrent accept or poll call on one socket.
+	 */
+	spin_lock_irqsave(&mappass->copy_lock, flags);
+	if (mappass->reqcopy.cmd != 0) {
+		spin_unlock_irqrestore(&mappass->copy_lock, flags);
+		ret = -EINTR;
+		goto out_error;
+	}
+
+	mappass->reqcopy = *req;
+	spin_unlock_irqrestore(&mappass->copy_lock, flags);
+	queue_work(mappass->wq, &mappass->register_work);
 	return 0;
+
+out_error:
+	rsp = RING_GET_RESPONSE(&priv->ring, priv->ring.rsp_prod_pvt++);
+	rsp->req_id = req->req_id;
+	rsp->cmd = req->cmd;
+	rsp->u.accept.id = req->u.accept.id;
+	rsp->ret = ret;
+	return 1;
 }
 
 static int pvcalls_back_poll(struct xenbus_device *dev,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642044 — [PATCH 05/18] xen/pvcalls: connect to a frontend

Introduce a per-frontend data structure named pvcalls_back_priv. It
contains pointers to the command ring, its event channel, a list of
active sockets and a tree of passive sockets (passing sockets need to be
looked up from the id on listen, accept and poll commands, while active
sockets only on release).

It also has an unbound workqueue to schedule the work of parsing and
executing commands on the command ring. pvcallss_lock protects the two
lists. In pvcalls_back_global, keep a list of connected frontends.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 86eca19..876e577 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -44,13 +44,100 @@ struct pvcalls_back_global {
 	struct rw_semaphore privs_lock;
 } pvcalls_back_global;
 
+struct pvcalls_back_priv {
+	struct list_head list;
+	struct xenbus_device *dev;
+	struct xen_pvcalls_sring *sring;
+	struct xen_pvcalls_back_ring ring;
+	int irq;
+	struct list_head socket_mappings;
+	struct radix_tree_root socketpass_mappings;
+	struct rw_semaphore pvcallss_lock;
+	atomic_t work;
+	struct workqueue_struct *wq;
+	struct work_struct register_work;
+};
+
 static void pvcalls_back_ioworker(struct work_struct *work)
 {
 }
 
+static void pvcalls_back_work(struct work_struct *work)
+{
+}
+
+static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+
 static int backend_connect(struct xenbus_device *dev)
 {
+	int err, evtchn;
+	grant_ref_t ring_ref;
+	void *addr = NULL;
+	struct pvcalls_back_priv *priv = NULL;
+
+	priv = kzalloc(sizeof(struct pvcalls_back_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	err = xenbus_scanf(XBT_NIL, dev->otherend, "port", "%u",
+			   &evtchn);
+	if (err != 1) {
+		err = -EINVAL;
+		xenbus_dev_fatal(dev, err, "reading %s/event-channel",
+				 dev->otherend);
+		goto error;
+	}
+
+	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-ref", "%u", &ring_ref);
+	if (err != 1) {
+		err = -EINVAL;
+		xenbus_dev_fatal(dev, err, "reading %s/ring-ref",
+				 dev->otherend);
+		goto error;
+	}
+
+	err = xenbus_map_ring_valloc(dev, &ring_ref, 1, &addr);
+	if (err < 0)
+		goto error;
+
+	err = bind_interdomain_evtchn_to_irqhandler(dev->otherend_id, evtchn,
+						    pvcalls_back_event, 0,
+						    "pvcalls-backend", dev);
+	if (err < 0)
+		goto error;
+
+	priv->wq = alloc_workqueue("pvcalls_back_wq", WQ_UNBOUND, 1);
+	if (!priv->wq) {
+		err = -ENOMEM;
+		goto error;
+	}
+	INIT_WORK(&priv->register_work, pvcalls_back_work);
+	priv->dev = dev;
+	priv->sring = addr;
+	BACK_RING_INIT(&priv->ring, priv->sring, XEN_PAGE_SIZE * 1);
+	priv->irq = err;
+	INIT_LIST_HEAD(&priv->socket_mappings);
+	INIT_RADIX_TREE(&priv->socketpass_mappings, GFP_KERNEL);
+	init_rwsem(&priv->pvcallss_lock);
+	dev_set_drvdata(&dev->dev, priv);
+	down_write(&pvcalls_back_global.privs_lock);
+	list_add_tail(&priv->list, &pvcalls_back_global.privs);
+	up_write(&pvcalls_back_global.privs_lock);
+	queue_work(priv->wq, &priv->register_work);
+
 	return 0;
+
+ error:
+	if (addr != NULL)
+		xenbus_unmap_ring_vfree(dev, addr);
+	if (priv->wq)
+		destroy_workqueue(priv->wq);
+	unbind_from_irqhandler(priv->irq, dev);
+	kfree(priv);
+	return err;
 }
 
 static int backend_disconnect(struct xenbus_device *dev)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642183 — Re: [PATCH 05/18] xen/pvcalls: connect to a frontend

On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> Introduce a per-frontend data structure named pvcalls_back_priv. It
> contains pointers to the command ring, its event channel, a list of
> active sockets and a tree of passive sockets (passing sockets need to be
> looked up from the id on listen, accept and poll commands, while active
> sockets only on release).

It would be useful to put this into a comment in pvcalls_back_priv 
definition.

>
> It also has an unbound workqueue to schedule the work of parsing and
> executing commands on the command ring. pvcallss_lock protects the two
> lists. In pvcalls_back_global, keep a list of connected frontends.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-back.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index 86eca19..876e577 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -44,13 +44,100 @@ struct pvcalls_back_global {
>  	struct rw_semaphore privs_lock;
>  } pvcalls_back_global;
>
> +struct pvcalls_back_priv {
> +	struct list_head list;
> +	struct xenbus_device *dev;
> +	struct xen_pvcalls_sring *sring;
> +	struct xen_pvcalls_back_ring ring;
> +	int irq;
> +	struct list_head socket_mappings;
> +	struct radix_tree_root socketpass_mappings;
> +	struct rw_semaphore pvcallss_lock;

Same question as before regarding using rw semaphore --- I only see 
down/up_writes.

And what does the name (pvcallss) stand for?


> +	atomic_t work;
> +	struct workqueue_struct *wq;
> +	struct work_struct register_work;
> +};
> +
>  static void pvcalls_back_ioworker(struct work_struct *work)
>  {
>  }
>
> +static void pvcalls_back_work(struct work_struct *work)
> +{
> +}
> +
> +static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
> +{
> +	return IRQ_HANDLED;
> +}
> +
>  static int backend_connect(struct xenbus_device *dev)
>  {
> +	int err, evtchn;
> +	grant_ref_t ring_ref;
> +	void *addr = NULL;
> +	struct pvcalls_back_priv *priv = NULL;
> +
> +	priv = kzalloc(sizeof(struct pvcalls_back_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	err = xenbus_scanf(XBT_NIL, dev->otherend, "port", "%u",
> +			   &evtchn);
> +	if (err != 1) {
> +		err = -EINVAL;
> +		xenbus_dev_fatal(dev, err, "reading %s/event-channel",
> +				 dev->otherend);
> +		goto error;
> +	}
> +
> +	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-ref", "%u", &ring_ref);
> +	if (err != 1) {
> +		err = -EINVAL;
> +		xenbus_dev_fatal(dev, err, "reading %s/ring-ref",
> +				 dev->otherend);
> +		goto error;
> +	}
> +
> +	err = xenbus_map_ring_valloc(dev, &ring_ref, 1, &addr);
> +	if (err < 0)
> +		goto error;


I'd move this closer to first use, below.

-boris

> +
> +	err = bind_interdomain_evtchn_to_irqhandler(dev->otherend_id, evtchn,
> +						    pvcalls_back_event, 0,
> +						    "pvcalls-backend", dev);
> +	if (err < 0)
> +		goto error;
> +
> +	priv->wq = alloc_workqueue("pvcalls_back_wq", WQ_UNBOUND, 1);
> +	if (!priv->wq) {
> +		err = -ENOMEM;
> +		goto error;
> +	}
> +	INIT_WORK(&priv->register_work, pvcalls_back_work);
> +	priv->dev = dev;
> +	priv->sring = addr;
> +	BACK_RING_INIT(&priv->ring, priv->sring, XEN_PAGE_SIZE * 1);
> +	priv->irq = err;
> +	INIT_LIST_HEAD(&priv->socket_mappings);
> +	INIT_RADIX_TREE(&priv->socketpass_mappings, GFP_KERNEL);
> +	init_rwsem(&priv->pvcallss_lock);
> +	dev_set_drvdata(&dev->dev, priv);
> +	down_write(&pvcalls_back_global.privs_lock);
> +	list_add_tail(&priv->list, &pvcalls_back_global.privs);
> +	up_write(&pvcalls_back_global.privs_lock);
> +	queue_work(priv->wq, &priv->register_work);
> +
>  	return 0;
> +
> + error:
> +	if (addr != NULL)
> +		xenbus_unmap_ring_vfree(dev, addr);
> +	if (priv->wq)
> +		destroy_workqueue(priv->wq);
> +	unbind_from_irqhandler(priv->irq, dev);
> +	kfree(priv);
> +	return err;
>  }
>
>  static int backend_disconnect(struct xenbus_device *dev)
>

[toc] | [prev] | [next] | [standalone]

#1642807 — Re: [PATCH 05/18] xen/pvcalls: connect to a frontend

On Mon, 15 May 2017, Boris Ostrovsky wrote:
> On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> > Introduce a per-frontend data structure named pvcalls_back_priv. It
> > contains pointers to the command ring, its event channel, a list of
> > active sockets and a tree of passive sockets (passing sockets need to be
> > looked up from the id on listen, accept and poll commands, while active
> > sockets only on release).
> 
> It would be useful to put this into a comment in pvcalls_back_priv definition.

I'll do that.


> > It also has an unbound workqueue to schedule the work of parsing and
> > executing commands on the command ring. pvcallss_lock protects the two
> > lists. In pvcalls_back_global, keep a list of connected frontends.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >  drivers/xen/pvcalls-back.c | 87
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> > 
> > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > index 86eca19..876e577 100644
> > --- a/drivers/xen/pvcalls-back.c
> > +++ b/drivers/xen/pvcalls-back.c
> > @@ -44,13 +44,100 @@ struct pvcalls_back_global {
> >  	struct rw_semaphore privs_lock;
> >  } pvcalls_back_global;
> > 
> > +struct pvcalls_back_priv {
> > +	struct list_head list;
> > +	struct xenbus_device *dev;
> > +	struct xen_pvcalls_sring *sring;
> > +	struct xen_pvcalls_back_ring ring;
> > +	int irq;
> > +	struct list_head socket_mappings;
> > +	struct radix_tree_root socketpass_mappings;
> > +	struct rw_semaphore pvcallss_lock;
> 
> Same question as before regarding using rw semaphore --- I only see
> down/up_writes.

And again, you are right. I'll switch it to a regular semaphore.


> And what does the name (pvcallss) stand for?
> 
> 
> > +	atomic_t work;
> > +	struct workqueue_struct *wq;
> > +	struct work_struct register_work;
> > +};
> > +
> >  static void pvcalls_back_ioworker(struct work_struct *work)
> >  {
> >  }
> > 
> > +static void pvcalls_back_work(struct work_struct *work)
> > +{
> > +}
> > +
> > +static irqreturn_t pvcalls_back_event(int irq, void *dev_id)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int backend_connect(struct xenbus_device *dev)
> >  {
> > +	int err, evtchn;
> > +	grant_ref_t ring_ref;
> > +	void *addr = NULL;
> > +	struct pvcalls_back_priv *priv = NULL;
> > +
> > +	priv = kzalloc(sizeof(struct pvcalls_back_priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	err = xenbus_scanf(XBT_NIL, dev->otherend, "port", "%u",
> > +			   &evtchn);
> > +	if (err != 1) {
> > +		err = -EINVAL;
> > +		xenbus_dev_fatal(dev, err, "reading %s/event-channel",
> > +				 dev->otherend);
> > +		goto error;
> > +	}
> > +
> > +	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-ref", "%u",
> > &ring_ref);
> > +	if (err != 1) {
> > +		err = -EINVAL;
> > +		xenbus_dev_fatal(dev, err, "reading %s/ring-ref",
> > +				 dev->otherend);
> > +		goto error;
> > +	}
> > +
> > +	err = xenbus_map_ring_valloc(dev, &ring_ref, 1, &addr);
> > +	if (err < 0)
> > +		goto error;
> 
> 
> I'd move this closer to first use, below.

Sure


> > +
> > +	err = bind_interdomain_evtchn_to_irqhandler(dev->otherend_id, evtchn,
> > +						    pvcalls_back_event, 0,
> > +						    "pvcalls-backend", dev);
> > +	if (err < 0)
> > +		goto error;
> > +
> > +	priv->wq = alloc_workqueue("pvcalls_back_wq", WQ_UNBOUND, 1);
> > +	if (!priv->wq) {
> > +		err = -ENOMEM;
> > +		goto error;
> > +	}
> > +	INIT_WORK(&priv->register_work, pvcalls_back_work);
> > +	priv->dev = dev;
> > +	priv->sring = addr;
> > +	BACK_RING_INIT(&priv->ring, priv->sring, XEN_PAGE_SIZE * 1);
> > +	priv->irq = err;
> > +	INIT_LIST_HEAD(&priv->socket_mappings);
> > +	INIT_RADIX_TREE(&priv->socketpass_mappings, GFP_KERNEL);
> > +	init_rwsem(&priv->pvcallss_lock);
> > +	dev_set_drvdata(&dev->dev, priv);
> > +	down_write(&pvcalls_back_global.privs_lock);
> > +	list_add_tail(&priv->list, &pvcalls_back_global.privs);
> > +	up_write(&pvcalls_back_global.privs_lock);
> > +	queue_work(priv->wq, &priv->register_work);
> > +
> >  	return 0;
> > +
> > + error:
> > +	if (addr != NULL)
> > +		xenbus_unmap_ring_vfree(dev, addr);
> > +	if (priv->wq)
> > +		destroy_workqueue(priv->wq);
> > +	unbind_from_irqhandler(priv->irq, dev);
> > +	kfree(priv);
> > +	return err;
> >  }
> > 
> >  static int backend_disconnect(struct xenbus_device *dev)
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1642812 — Re: [PATCH 05/18] xen/pvcalls: connect to a frontend

On Tue, 16 May 2017, Stefano Stabellini wrote:
> > > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > > index 86eca19..876e577 100644
> > > --- a/drivers/xen/pvcalls-back.c
> > > +++ b/drivers/xen/pvcalls-back.c
> > > @@ -44,13 +44,100 @@ struct pvcalls_back_global {
> > >  	struct rw_semaphore privs_lock;
> > >  } pvcalls_back_global;
> > > 
> > > +struct pvcalls_back_priv {
> > > +	struct list_head list;
> > > +	struct xenbus_device *dev;
> > > +	struct xen_pvcalls_sring *sring;
> > > +	struct xen_pvcalls_back_ring ring;
> > > +	int irq;
> > > +	struct list_head socket_mappings;
> > > +	struct radix_tree_root socketpass_mappings;
> > > +	struct rw_semaphore pvcallss_lock;
> > 
> > Same question as before regarding using rw semaphore --- I only see
> > down/up_writes.
> 
> And again, you are right. I'll switch it to a regular semaphore.
> 
> 
> > And what does the name (pvcallss) stand for?

It stands for socket lock. I'll rename it to socket_lock.


> > 
> > > +	atomic_t work;
> > > +	struct workqueue_struct *wq;
> > > +	struct work_struct register_work;
> > > +};

[toc] | [prev] | [next] | [standalone]

#1642051 — [PATCH 04/18] xen/pvcalls: xenbus state handling

Introduce the code to handle xenbus state changes.

Implement the probe function for the pvcalls backend. Write the
supported versions, max-page-order and function-calls nodes to xenstore,
as required by the protocol.

Introduce stub functions for disconnecting/connecting to a frontend.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
 drivers/xen/pvcalls-back.c | 133 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index 46a889a..86eca19 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -25,6 +25,9 @@
 #include <xen/xenbus.h>
 #include <xen/interface/io/pvcalls.h>
 
+#define PVCALLS_VERSIONS "1"
+#define MAX_RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
+
 struct pvcalls_ioworker {
 	struct work_struct register_work;
 	atomic_t io;
@@ -45,15 +48,145 @@ static void pvcalls_back_ioworker(struct work_struct *work)
 {
 }
 
+static int backend_connect(struct xenbus_device *dev)
+{
+	return 0;
+}
+
+static int backend_disconnect(struct xenbus_device *dev)
+{
+	return 0;
+}
+
 static int pvcalls_back_probe(struct xenbus_device *dev,
 			      const struct xenbus_device_id *id)
 {
+	int err;
+
+	err = xenbus_printf(XBT_NIL, dev->nodename, "versions", "%s",
+			    PVCALLS_VERSIONS);
+	if (err) {
+		pr_warn("%s write out 'version' failed\n", __func__);
+		return -EINVAL;
+	}
+
+	err = xenbus_printf(XBT_NIL, dev->nodename, "max-page-order", "%u",
+			    MAX_RING_ORDER);
+	if (err) {
+		pr_warn("%s write out 'max-page-order' failed\n", __func__);
+		return -EINVAL;
+	}
+
+	/* "1" means socket, connect, release, bind, listen, accept and poll*/
+	err = xenbus_printf(XBT_NIL, dev->nodename, "function-calls", "1");
+	if (err) {
+		pr_warn("%s write out 'function-calls' failed\n", __func__);
+		return -EINVAL;
+	}
+
+	err = xenbus_switch_state(dev, XenbusStateInitWait);
+	if (err)
+		return err;
+
 	return 0;
 }
 
+static void set_backend_state(struct xenbus_device *dev,
+			      enum xenbus_state state)
+{
+	while (dev->state != state) {
+		switch (dev->state) {
+		case XenbusStateClosed:
+			switch (state) {
+			case XenbusStateInitWait:
+			case XenbusStateConnected:
+				xenbus_switch_state(dev, XenbusStateInitWait);
+				break;
+			case XenbusStateClosing:
+				xenbus_switch_state(dev, XenbusStateClosing);
+				break;
+			default:
+				__WARN();
+			}
+			break;
+		case XenbusStateInitWait:
+		case XenbusStateInitialised:
+			switch (state) {
+			case XenbusStateConnected:
+				backend_connect(dev);
+				xenbus_switch_state(dev, XenbusStateConnected);
+				break;
+			case XenbusStateClosing:
+			case XenbusStateClosed:
+				xenbus_switch_state(dev, XenbusStateClosing);
+				break;
+			default:
+				__WARN();
+			}
+			break;
+		case XenbusStateConnected:
+			switch (state) {
+			case XenbusStateInitWait:
+			case XenbusStateClosing:
+			case XenbusStateClosed:
+				down_write(&pvcalls_back_global.privs_lock);
+				backend_disconnect(dev);
+				up_write(&pvcalls_back_global.privs_lock);
+				xenbus_switch_state(dev, XenbusStateClosing);
+				break;
+			default:
+				__WARN();
+			}
+			break;
+		case XenbusStateClosing:
+			switch (state) {
+			case XenbusStateInitWait:
+			case XenbusStateConnected:
+			case XenbusStateClosed:
+				xenbus_switch_state(dev, XenbusStateClosed);
+				break;
+			default:
+				__WARN();
+			}
+			break;
+		default:
+			__WARN();
+		}
+	}
+}
+
 static void pvcalls_back_changed(struct xenbus_device *dev,
 				 enum xenbus_state frontend_state)
 {
+	switch (frontend_state) {
+	case XenbusStateInitialising:
+		set_backend_state(dev, XenbusStateInitWait);
+		break;
+
+	case XenbusStateInitialised:
+	case XenbusStateConnected:
+		set_backend_state(dev, XenbusStateConnected);
+		break;
+
+	case XenbusStateClosing:
+		set_backend_state(dev, XenbusStateClosing);
+		break;
+
+	case XenbusStateClosed:
+		set_backend_state(dev, XenbusStateClosed);
+		if (xenbus_dev_is_online(dev))
+			break;
+		/* fall through if not online */
+	case XenbusStateUnknown:
+		set_backend_state(dev, XenbusStateClosed);
+		device_unregister(&dev->dev);
+		break;
+
+	default:
+		xenbus_dev_fatal(dev, -EINVAL, "saw state %d at frontend",
+				 frontend_state);
+		break;
+	}
 }
 
 static int pvcalls_back_remove(struct xenbus_device *dev)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1642175 — Re: [PATCH 04/18] xen/pvcalls: xenbus state handling

On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> Introduce the code to handle xenbus state changes.
>
> Implement the probe function for the pvcalls backend. Write the
> supported versions, max-page-order and function-calls nodes to xenstore,
> as required by the protocol.
>
> Introduce stub functions for disconnecting/connecting to a frontend.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> ---
>  drivers/xen/pvcalls-back.c | 133 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index 46a889a..86eca19 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -25,6 +25,9 @@
>  #include <xen/xenbus.h>
>  #include <xen/interface/io/pvcalls.h>
>
> +#define PVCALLS_VERSIONS "1"
> +#define MAX_RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
> +
>  struct pvcalls_ioworker {
>  	struct work_struct register_work;
>  	atomic_t io;
> @@ -45,15 +48,145 @@ static void pvcalls_back_ioworker(struct work_struct *work)
>  {
>  }
>
> +static int backend_connect(struct xenbus_device *dev)
> +{
> +	return 0;
> +}
> +
> +static int backend_disconnect(struct xenbus_device *dev)
> +{
> +	return 0;
> +}
> +
>  static int pvcalls_back_probe(struct xenbus_device *dev,
>  			      const struct xenbus_device_id *id)
>  {
> +	int err;
> +
> +	err = xenbus_printf(XBT_NIL, dev->nodename, "versions", "%s",
> +			    PVCALLS_VERSIONS);
> +	if (err) {
> +		pr_warn("%s write out 'version' failed\n", __func__);
> +		return -EINVAL;

Why not return err? (below too)


> +	}
> +
> +	err = xenbus_printf(XBT_NIL, dev->nodename, "max-page-order", "%u",
> +			    MAX_RING_ORDER);
> +	if (err) {
> +		pr_warn("%s write out 'max-page-order' failed\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* "1" means socket, connect, release, bind, listen, accept and poll*/
> +	err = xenbus_printf(XBT_NIL, dev->nodename, "function-calls", "1");


Should "1" be defined in the (public) header file?


> +	if (err) {
> +		pr_warn("%s write out 'function-calls' failed\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	err = xenbus_switch_state(dev, XenbusStateInitWait);
> +	if (err)
> +		return err;
> +
>  	return 0;
>  }
>
> +static void set_backend_state(struct xenbus_device *dev,
> +			      enum xenbus_state state)
> +{
> +	while (dev->state != state) {
> +		switch (dev->state) {
> +		case XenbusStateClosed:
> +			switch (state) {
> +			case XenbusStateInitWait:
> +			case XenbusStateConnected:
> +				xenbus_switch_state(dev, XenbusStateInitWait);
> +				break;
> +			case XenbusStateClosing:
> +				xenbus_switch_state(dev, XenbusStateClosing);
> +				break;
> +			default:
> +				__WARN();
> +			}
> +			break;
> +		case XenbusStateInitWait:
> +		case XenbusStateInitialised:
> +			switch (state) {
> +			case XenbusStateConnected:
> +				backend_connect(dev);
> +				xenbus_switch_state(dev, XenbusStateConnected);
> +				break;
> +			case XenbusStateClosing:
> +			case XenbusStateClosed:
> +				xenbus_switch_state(dev, XenbusStateClosing);
> +				break;
> +			default:
> +				__WARN();
> +			}
> +			break;
> +		case XenbusStateConnected:
> +			switch (state) {
> +			case XenbusStateInitWait:
> +			case XenbusStateClosing:
> +			case XenbusStateClosed:
> +				down_write(&pvcalls_back_global.privs_lock);
> +				backend_disconnect(dev);
> +				up_write(&pvcalls_back_global.privs_lock);


Unless you plan to have more stuff under the semaphore, I'd consider 
putting them in backend_disconnect().


> +				xenbus_switch_state(dev, XenbusStateClosing);
> +				break;
> +			default:
> +				__WARN();
> +			}
> +			break;
> +		case XenbusStateClosing:
> +			switch (state) {
> +			case XenbusStateInitWait:
> +			case XenbusStateConnected:
> +			case XenbusStateClosed:
> +				xenbus_switch_state(dev, XenbusStateClosed);
> +				break;
> +			default:
> +				__WARN();
> +			}
> +			break;
> +		default:
> +			__WARN();
> +		}
> +	}
> +}
> +
>  static void pvcalls_back_changed(struct xenbus_device *dev,
>  				 enum xenbus_state frontend_state)
>  {
> +	switch (frontend_state) {
> +	case XenbusStateInitialising:
> +		set_backend_state(dev, XenbusStateInitWait);
> +		break;
> +
> +	case XenbusStateInitialised:
> +	case XenbusStateConnected:
> +		set_backend_state(dev, XenbusStateConnected);
> +		break;
> +
> +	case XenbusStateClosing:
> +		set_backend_state(dev, XenbusStateClosing);
> +		break;
> +
> +	case XenbusStateClosed:
> +		set_backend_state(dev, XenbusStateClosed);
> +		if (xenbus_dev_is_online(dev))
> +			break;
> +		/* fall through if not online */
> +	case XenbusStateUnknown:
> +		set_backend_state(dev, XenbusStateClosed);


You are setting XenbusStateClosed twice in case of fallthrough.

-boris


> +		device_unregister(&dev->dev);
> +		break;
> +
> +	default:
> +		xenbus_dev_fatal(dev, -EINVAL, "saw state %d at frontend",
> +				 frontend_state);
> +		break;
> +	}
>  }
>
>  static int pvcalls_back_remove(struct xenbus_device *dev)
>

[toc] | [prev] | [next] | [standalone]

#1642800 — Re: [PATCH 04/18] xen/pvcalls: xenbus state handling

On Mon, 15 May 2017, Boris Ostrovsky wrote:
> On 05/15/2017 04:35 PM, Stefano Stabellini wrote:
> > Introduce the code to handle xenbus state changes.
> > 
> > Implement the probe function for the pvcalls backend. Write the
> > supported versions, max-page-order and function-calls nodes to xenstore,
> > as required by the protocol.
> > 
> > Introduce stub functions for disconnecting/connecting to a frontend.
> > 
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > ---
> >  drivers/xen/pvcalls-back.c | 133
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 133 insertions(+)
> > 
> > diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> > index 46a889a..86eca19 100644
> > --- a/drivers/xen/pvcalls-back.c
> > +++ b/drivers/xen/pvcalls-back.c
> > @@ -25,6 +25,9 @@
> >  #include <xen/xenbus.h>
> >  #include <xen/interface/io/pvcalls.h>
> > 
> > +#define PVCALLS_VERSIONS "1"
> > +#define MAX_RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
> > +
> >  struct pvcalls_ioworker {
> >  	struct work_struct register_work;
> >  	atomic_t io;
> > @@ -45,15 +48,145 @@ static void pvcalls_back_ioworker(struct work_struct
> > *work)
> >  {
> >  }
> > 
> > +static int backend_connect(struct xenbus_device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int backend_disconnect(struct xenbus_device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> >  static int pvcalls_back_probe(struct xenbus_device *dev,
> >  			      const struct xenbus_device_id *id)
> >  {
> > +	int err;
> > +
> > +	err = xenbus_printf(XBT_NIL, dev->nodename, "versions", "%s",
> > +			    PVCALLS_VERSIONS);
> > +	if (err) {
> > +		pr_warn("%s write out 'version' failed\n", __func__);
> > +		return -EINVAL;
> 
> Why not return err? (below too)

Yeah, I'll make the change.


> > +	}
> > +
> > +	err = xenbus_printf(XBT_NIL, dev->nodename, "max-page-order", "%u",
> > +			    MAX_RING_ORDER);
> > +	if (err) {
> > +		pr_warn("%s write out 'max-page-order' failed\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* "1" means socket, connect, release, bind, listen, accept and poll*/
> > +	err = xenbus_printf(XBT_NIL, dev->nodename, "function-calls", "1");
> 
> 
> Should "1" be defined in the (public) header file?

Fair enough, it makes sense.

 
> > +	if (err) {
> > +		pr_warn("%s write out 'function-calls' failed\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = xenbus_switch_state(dev, XenbusStateInitWait);
> > +	if (err)
> > +		return err;
> > +
> >  	return 0;
> >  }
> > 
> > +static void set_backend_state(struct xenbus_device *dev,
> > +			      enum xenbus_state state)
> > +{
> > +	while (dev->state != state) {
> > +		switch (dev->state) {
> > +		case XenbusStateClosed:
> > +			switch (state) {
> > +			case XenbusStateInitWait:
> > +			case XenbusStateConnected:
> > +				xenbus_switch_state(dev, XenbusStateInitWait);
> > +				break;
> > +			case XenbusStateClosing:
> > +				xenbus_switch_state(dev, XenbusStateClosing);
> > +				break;
> > +			default:
> > +				__WARN();
> > +			}
> > +			break;
> > +		case XenbusStateInitWait:
> > +		case XenbusStateInitialised:
> > +			switch (state) {
> > +			case XenbusStateConnected:
> > +				backend_connect(dev);
> > +				xenbus_switch_state(dev,
> > XenbusStateConnected);
> > +				break;
> > +			case XenbusStateClosing:
> > +			case XenbusStateClosed:
> > +				xenbus_switch_state(dev, XenbusStateClosing);
> > +				break;
> > +			default:
> > +				__WARN();
> > +			}
> > +			break;
> > +		case XenbusStateConnected:
> > +			switch (state) {
> > +			case XenbusStateInitWait:
> > +			case XenbusStateClosing:
> > +			case XenbusStateClosed:
> > +				down_write(&pvcalls_back_global.privs_lock);
> > +				backend_disconnect(dev);
> > +				up_write(&pvcalls_back_global.privs_lock);
> 
> 
> Unless you plan to have more stuff under the semaphore, I'd consider putting
> them in backend_disconnect().

Yes, there will be more things in pvcalls_back_fin (the function that
implements module_exit in patch #14).


> > +				xenbus_switch_state(dev, XenbusStateClosing);
> > +				break;
> > +			default:
> > +				__WARN();
> > +			}
> > +			break;
> > +		case XenbusStateClosing:
> > +			switch (state) {
> > +			case XenbusStateInitWait:
> > +			case XenbusStateConnected:
> > +			case XenbusStateClosed:
> > +				xenbus_switch_state(dev, XenbusStateClosed);
> > +				break;
> > +			default:
> > +				__WARN();
> > +			}
> > +			break;
> > +		default:
> > +			__WARN();
> > +		}
> > +	}
> > +}
> > +
> >  static void pvcalls_back_changed(struct xenbus_device *dev,
> >  				 enum xenbus_state frontend_state)
> >  {
> > +	switch (frontend_state) {
> > +	case XenbusStateInitialising:
> > +		set_backend_state(dev, XenbusStateInitWait);
> > +		break;
> > +
> > +	case XenbusStateInitialised:
> > +	case XenbusStateConnected:
> > +		set_backend_state(dev, XenbusStateConnected);
> > +		break;
> > +
> > +	case XenbusStateClosing:
> > +		set_backend_state(dev, XenbusStateClosing);
> > +		break;
> > +
> > +	case XenbusStateClosed:
> > +		set_backend_state(dev, XenbusStateClosed);
> > +		if (xenbus_dev_is_online(dev))
> > +			break;
> > +		/* fall through if not online */
> > +	case XenbusStateUnknown:
> > +		set_backend_state(dev, XenbusStateClosed);
> 
> 
> You are setting XenbusStateClosed twice in case of fallthrough.

I'll fix it.


> > +		device_unregister(&dev->dev);
> > +		break;
> > +
> > +	default:
> > +		xenbus_dev_fatal(dev, -EINVAL, "saw state %d at frontend",
> > +				 frontend_state);
> > +		break;
> > +	}
> >  }
> > 
> >  static int pvcalls_back_remove(struct xenbus_device *dev)
> > 
> 

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web