* [PATCH 2/7] xen: introduce the header file for the Xen 9pfs transport protocol
2017-03-06 20:01 ` [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini
@ 2017-03-06 20:01 ` Stefano Stabellini
2017-03-06 21:22 ` Boris Ostrovsky
2017-03-06 20:01 ` [PATCH 3/7] xen/9pfs: introduce Xen 9pfs transport driver Stefano Stabellini
` (5 subsequent siblings)
6 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-06 20:01 UTC (permalink / raw)
To: xen-devel
Cc: jgross, sstabellini, linux-kernel, Stefano Stabellini,
boris.ostrovsky
It uses the new ring.h macros to declare rings and interfaces.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: konrad.wilk@oracle.com
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
---
include/xen/interface/io/9pfs.h | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 include/xen/interface/io/9pfs.h
diff --git a/include/xen/interface/io/9pfs.h b/include/xen/interface/io/9pfs.h
new file mode 100644
index 0000000..276eda4
--- /dev/null
+++ b/include/xen/interface/io/9pfs.h
@@ -0,0 +1,40 @@
+/*
+ * 9pfs.h -- Xen 9PFS transport
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (C) 2017 Stefano Stabellini <stefano@aporeto.com>
+ */
+
+#ifndef __XEN_PUBLIC_IO_9PFS_H__
+#define __XEN_PUBLIC_IO_9PFS_H__
+
+#include "xen/interface/io/ring.h"
+
+struct xen_9pfs_header {
+ uint32_t size;
+ uint8_t id;
+ uint16_t tag;
+} __attribute__((packed));
+
+#define XEN_9PFS_RING_ORDER 6
+#define XEN_9PFS_RING_SIZE XEN_FLEX_RING_SIZE(XEN_9PFS_RING_ORDER)
+DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
+
+#endif
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 3/7] xen/9pfs: introduce Xen 9pfs transport driver
2017-03-06 20:01 ` [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini
2017-03-06 20:01 ` [PATCH 2/7] xen: introduce the header file for the Xen 9pfs transport protocol Stefano Stabellini
@ 2017-03-06 20:01 ` Stefano Stabellini
2017-03-06 20:01 ` [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini
` (4 subsequent siblings)
6 siblings, 0 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-06 20:01 UTC (permalink / raw)
To: xen-devel
Cc: jgross, Latchesar Ionkov, sstabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
boris.ostrovsky
Introduce the Xen 9pfs transport driver: add struct xenbus_driver to
register as a xenbus driver and add struct p9_trans_module to register
as v9fs driver.
All functions are empty stubs for now.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
CC: Eric Van Hensbergen <ericvh@gmail.com>
CC: Ron Minnich <rminnich@sandia.gov>
CC: Latchesar Ionkov <lucho@ionkov.net>
CC: v9fs-developer@lists.sourceforge.net
---
net/9p/trans_xen.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 101 insertions(+)
create mode 100644 net/9p/trans_xen.c
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
new file mode 100644
index 0000000..877dfd0
--- /dev/null
+++ b/net/9p/trans_xen.c
@@ -0,0 +1,101 @@
+/*
+ * linux/fs/9p/trans_xen
+ *
+ * Xen transport layer.
+ *
+ * Copyright (C) 2017 by Stefano Stabellini <stefano@aporeto.com>
+ */
+
+#include <xen/events.h>
+#include <xen/grant_table.h>
+#include <xen/xen.h>
+#include <xen/xenbus.h>
+#include <xen/interface/io/9pfs.h>
+
+#include <linux/module.h>
+#include <net/9p/9p.h>
+#include <net/9p/client.h>
+#include <net/9p/transport.h>
+
+static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
+{
+ return 0;
+}
+
+static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
+{
+ return 0;
+}
+
+static void p9_xen_close(struct p9_client *client)
+{
+}
+
+static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
+{
+ return 0;
+}
+
+static struct p9_trans_module p9_xen_trans = {
+ .name = "xen",
+ .maxsize = (1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT)),
+ .def = 1,
+ .create = p9_xen_create,
+ .close = p9_xen_close,
+ .request = p9_xen_request,
+ .cancel = p9_xen_cancel,
+ .owner = THIS_MODULE,
+};
+
+static const struct xenbus_device_id xen_9pfs_front_ids[] = {
+ { "9pfs" },
+ { "" }
+};
+
+static int xen_9pfs_front_remove(struct xenbus_device *dev)
+{
+ return 0;
+}
+
+static int xen_9pfs_front_probe(struct xenbus_device *dev,
+ const struct xenbus_device_id *id)
+{
+ return 0;
+}
+
+static int xen_9pfs_front_resume(struct xenbus_device *dev)
+{
+ return 0;
+}
+
+static void xen_9pfs_front_changed(struct xenbus_device *dev,
+ enum xenbus_state backend_state)
+{
+}
+
+static struct xenbus_driver xen_9pfs_front_driver = {
+ .ids = xen_9pfs_front_ids,
+ .probe = xen_9pfs_front_probe,
+ .remove = xen_9pfs_front_remove,
+ .resume = xen_9pfs_front_resume,
+ .otherend_changed = xen_9pfs_front_changed,
+};
+
+int p9_trans_xen_init(void)
+{
+ if (!xen_domain())
+ return -ENODEV;
+
+ pr_info("Initialising Xen transport for 9pfs\n");
+
+ v9fs_register_trans(&p9_xen_trans);
+ return xenbus_register_frontend(&xen_9pfs_front_driver);
+}
+module_init(p9_trans_xen_init);
+
+void p9_trans_xen_exit(void)
+{
+ v9fs_unregister_trans(&p9_xen_trans);
+ return xenbus_unregister_driver(&xen_9pfs_front_driver);
+}
+module_exit(p9_trans_xen_exit);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-06 20:01 ` [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini
2017-03-06 20:01 ` [PATCH 2/7] xen: introduce the header file for the Xen 9pfs transport protocol Stefano Stabellini
2017-03-06 20:01 ` [PATCH 3/7] xen/9pfs: introduce Xen 9pfs transport driver Stefano Stabellini
@ 2017-03-06 20:01 ` Stefano Stabellini
2017-03-07 15:03 ` Boris Ostrovsky
2017-03-07 17:37 ` Julien Grall
2017-03-06 20:01 ` [PATCH 5/7] xen/9pfs: send requests " Stefano Stabellini
` (3 subsequent siblings)
6 siblings, 2 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-06 20:01 UTC (permalink / raw)
To: xen-devel
Cc: jgross, Latchesar Ionkov, sstabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
boris.ostrovsky
Implement functions to handle the xenbus handshake. Upon connection,
allocate the rings according to the protocol specification.
Initialize a work_struct and a wait_queue. The work_struct will be used
to schedule work upon receiving an event channel notification from the
backend. The wait_queue will be used to wait when the ring is full and
we need to send a new request.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
CC: Eric Van Hensbergen <ericvh@gmail.com>
CC: Ron Minnich <rminnich@sandia.gov>
CC: Latchesar Ionkov <lucho@ionkov.net>
CC: v9fs-developer@lists.sourceforge.net
---
net/9p/trans_xen.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 227 insertions(+)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 877dfd0..9f6cf8d 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -17,6 +17,36 @@
#include <net/9p/client.h>
#include <net/9p/transport.h>
+#define XEN_9PFS_NUM_RINGS 2
+
+/* One per ring, more than one per 9pfs share */
+struct xen_9pfs_dataring {
+ struct xen_9pfs_front_priv *priv;
+
+ struct xen_9pfs_data_intf *intf;
+ grant_ref_t ref;
+ int evtchn;
+ int irq;
+ spinlock_t lock;
+
+ void *bytes;
+ struct xen_9pfs_data ring;
+ wait_queue_head_t wq;
+ struct work_struct work;
+};
+
+/* One per 9pfs share */
+struct xen_9pfs_front_priv {
+ struct list_head list;
+ struct xenbus_device *dev;
+ char *tag;
+ struct p9_client *client;
+
+ int num_rings;
+ struct xen_9pfs_dataring *rings;
+};
+static LIST_HEAD(xen_9pfs_devs);
+
static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
{
return 0;
@@ -36,6 +66,21 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
return 0;
}
+static void p9_xen_response(struct work_struct *work)
+{
+}
+
+static irqreturn_t xen_9pfs_front_event_handler(int irq, void *r)
+{
+ struct xen_9pfs_dataring *ring = r;
+ BUG_ON(!ring || !ring->priv->client);
+
+ wake_up_interruptible(&ring->wq);
+ schedule_work(&ring->work);
+
+ return IRQ_HANDLED;
+}
+
static struct p9_trans_module p9_xen_trans = {
.name = "xen",
.maxsize = (1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT)),
@@ -52,25 +97,207 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
{ "" }
};
+static int xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
+{
+ int i, j;
+
+ list_del(&priv->list);
+
+ for (i = 0; i < priv->num_rings; i++) {
+ if (priv->rings[i].intf == NULL)
+ break;
+ if (priv->rings[i].irq > 0)
+ unbind_from_irqhandler(priv->rings[i].irq, priv->dev);
+ if (priv->rings[i].bytes != NULL) {
+ for (j = 0; j < (1 << XEN_9PFS_RING_ORDER); j++)
+ gnttab_end_foreign_access(priv->rings[i].intf->ref[j], 0, 0);
+ free_pages((unsigned long)priv->rings[i].bytes, XEN_9PFS_RING_ORDER);
+ }
+ gnttab_end_foreign_access(priv->rings[i].ref, 0, 0);
+ free_page((unsigned long)priv->rings[i].intf);
+ }
+ kfree(priv->rings);
+ kfree(priv);
+
+ return 0;
+}
+
static int xen_9pfs_front_remove(struct xenbus_device *dev)
{
+ int ret;
+ struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
+
+ dev_set_drvdata(&dev->dev, NULL);
+ ret = xen_9pfs_front_free(priv);
+ return ret;
+}
+
+static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
+ struct xen_9pfs_dataring *ring)
+{
+ int i;
+ int ret = -ENOMEM;
+
+ init_waitqueue_head(&ring->wq);
+ spin_lock_init(&ring->lock);
+ INIT_WORK(&ring->work, p9_xen_response);
+
+ ring->intf = (struct xen_9pfs_data_intf *) __get_free_page(GFP_KERNEL | __GFP_ZERO);
+ if (!ring->intf)
+ goto error;
+ memset(ring->intf, 0, XEN_PAGE_SIZE);
+ ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, XEN_9PFS_RING_ORDER);
+ if (ring->bytes == NULL)
+ goto error;
+ for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
+ ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);
+ ring->ref = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
+ ring->ring.in = ring->bytes;
+ ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
+
+ ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
+ if (ret)
+ goto error;
+ ring->irq = bind_evtchn_to_irqhandler(ring->evtchn, xen_9pfs_front_event_handler,
+ 0, "xen_9pfs-frontend", ring);
+ if (ring->irq < 0) {
+ xenbus_free_evtchn(dev, ring->evtchn);
+ ret = ring->irq;
+ goto error;
+ }
return 0;
+
+error:
+ if (ring->intf != NULL)
+ kfree(ring->intf);
+ if (ring->bytes != NULL)
+ kfree(ring->bytes);
+ return ret;
}
static int xen_9pfs_front_probe(struct xenbus_device *dev,
const struct xenbus_device_id *id)
{
+ int ret = -EFAULT, i;
+ struct xenbus_transaction xbt;
+ struct xen_9pfs_front_priv *priv = NULL;
+ char *versions;
+ unsigned int max_rings, max_ring_order, len;
+
+ versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
+ if (!len || strcmp(versions, "1"))
+ return -EINVAL;
+ kfree(versions);
+ ret = xenbus_scanf(XBT_NIL, dev->otherend, "max-rings", "%u", &max_rings);
+ if (ret < 0 || max_rings < XEN_9PFS_NUM_RINGS)
+ return -EINVAL;
+ ret = xenbus_scanf(XBT_NIL, dev->otherend, "max-ring-page-order", "%u", &max_ring_order);
+ if (ret < 0|| max_ring_order < XEN_9PFS_RING_ORDER)
+ return -EINVAL;
+
+
+ priv = kzalloc(sizeof(struct xen_9pfs_front_priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = dev;
+ priv->num_rings = XEN_9PFS_NUM_RINGS;
+ priv->rings = kzalloc(sizeof(struct xen_9pfs_dataring) * priv->num_rings,
+ GFP_KERNEL);
+ if (!priv->rings) {
+ kfree(priv);
+ return -ENOMEM;
+ }
+
+ again:
+ ret = xenbus_transaction_start(&xbt);
+ if (ret) {
+ xenbus_dev_fatal(dev, ret, "starting transaction");
+ goto error;
+ }
+ ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
+ if (ret)
+ goto error_xenbus;
+ ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", priv->num_rings);
+ if (ret)
+ goto error_xenbus;
+ for (i = 0; i < priv->num_rings; i++) {
+ char str[16];
+
+ priv->rings[i].priv = priv;
+ ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i]);
+ if (ret < 0)
+ goto error_xenbus;
+
+ sprintf(str, "ring-ref%u", i);
+ ret = xenbus_printf(xbt, dev->nodename, str, "%d", priv->rings[i].ref);
+ if (ret)
+ goto error_xenbus;
+
+ sprintf(str, "event-channel-%u", i);
+ ret = xenbus_printf(xbt, dev->nodename, str, "%u", priv->rings[i].evtchn);
+ if (ret)
+ goto error_xenbus;
+ }
+ priv->tag = xenbus_read(xbt, dev->nodename, "tag", NULL);
+ if (ret)
+ goto error_xenbus;
+ ret = xenbus_transaction_end(xbt, 0);
+ if (ret) {
+ if (ret == -EAGAIN)
+ goto again;
+ xenbus_dev_fatal(dev, ret, "completing transaction");
+ goto error;
+ }
+
+
+ list_add_tail(&priv->list, &xen_9pfs_devs);
+ dev_set_drvdata(&dev->dev, priv);
+ xenbus_switch_state(dev, XenbusStateInitialised);
+
return 0;
+
+ error_xenbus:
+ xenbus_transaction_end(xbt, 1);
+ xenbus_dev_fatal(dev, ret, "writing xenstore");
+ error:
+ dev_set_drvdata(&dev->dev, NULL);
+ xen_9pfs_front_free(priv);
+ return ret;
}
static int xen_9pfs_front_resume(struct xenbus_device *dev)
{
+ dev_warn(&dev->dev, "suspsend/resume unsupported\n");
return 0;
}
static void xen_9pfs_front_changed(struct xenbus_device *dev,
enum xenbus_state backend_state)
{
+ switch (backend_state) {
+ case XenbusStateReconfiguring:
+ case XenbusStateReconfigured:
+ case XenbusStateInitialising:
+ case XenbusStateInitialised:
+ case XenbusStateUnknown:
+ break;
+
+ case XenbusStateInitWait:
+ break;
+
+ case XenbusStateConnected:
+ xenbus_switch_state(dev, XenbusStateConnected);
+ break;
+
+ case XenbusStateClosed:
+ if (dev->state == XenbusStateClosed)
+ break;
+ /* Missed the backend's CLOSING state -- fallthrough */
+ case XenbusStateClosing:
+ xenbus_frontend_closed(dev);
+ break;
+ }
}
static struct xenbus_driver xen_9pfs_front_driver = {
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-06 20:01 ` [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini
@ 2017-03-07 15:03 ` Boris Ostrovsky
2017-03-08 0:11 ` Stefano Stabellini
2017-03-07 17:37 ` Julien Grall
1 sibling, 1 reply; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-07 15:03 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich
>
> +static int xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
> +{
> + int i, j;
> +
> + list_del(&priv->list);
> +
> + for (i = 0; i < priv->num_rings; i++) {
> + if (priv->rings[i].intf == NULL)
> + break;
Are we guaranteed that all subsequent entries are not allocated (i.e.
this shouldn't be 'continue')?
> + if (priv->rings[i].irq > 0)
> + unbind_from_irqhandler(priv->rings[i].irq, priv->dev);
> + if (priv->rings[i].bytes != NULL) {
> + for (j = 0; j < (1 << XEN_9PFS_RING_ORDER); j++)
> + gnttab_end_foreign_access(priv->rings[i].intf->ref[j], 0, 0);
> + free_pages((unsigned long)priv->rings[i].bytes, XEN_9PFS_RING_ORDER);
> + }
> + gnttab_end_foreign_access(priv->rings[i].ref, 0, 0);
> + free_page((unsigned long)priv->rings[i].intf);
> + }
> + kfree(priv->rings);
> + kfree(priv);
> +
> + return 0;
> +}
> +
> static int xen_9pfs_front_remove(struct xenbus_device *dev)
> {
> + int ret;
> + struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
> +
> + dev_set_drvdata(&dev->dev, NULL);
> + ret = xen_9pfs_front_free(priv);
> + return ret;
> +}
> +
> +static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> + struct xen_9pfs_dataring *ring)
> +{
> + int i;
> + int ret = -ENOMEM;
> +
> + init_waitqueue_head(&ring->wq);
> + spin_lock_init(&ring->lock);
> + INIT_WORK(&ring->work, p9_xen_response);
> +
> + ring->intf = (struct xen_9pfs_data_intf *) __get_free_page(GFP_KERNEL | __GFP_ZERO);
> + if (!ring->intf)
> + goto error;
> + memset(ring->intf, 0, XEN_PAGE_SIZE);
get_zeroed_page()? (especially given that __get_free_page() returns
PAGE_SIZE, not XEN_PAGE_SIZE)
> + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, XEN_9PFS_RING_ORDER);
> + if (ring->bytes == NULL)
> + goto error;
> + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> + ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);
> + ring->ref = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
> + ring->ring.in = ring->bytes;
> + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
> +
> + ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
> + if (ret)
> + goto error;
> + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn, xen_9pfs_front_event_handler,
> + 0, "xen_9pfs-frontend", ring);
> + if (ring->irq < 0) {
> + xenbus_free_evtchn(dev, ring->evtchn);
> + ret = ring->irq;
> + goto error;
> + }
> return 0;
> +
> +error:
You may need to gnttab_end_foreign_access().
> + if (ring->intf != NULL)
> + kfree(ring->intf);
> + if (ring->bytes != NULL)
> + kfree(ring->bytes);
> + return ret;
> }
>
> static int xen_9pfs_front_probe(struct xenbus_device *dev,
> const struct xenbus_device_id *id)
> {
> + int ret = -EFAULT, i;
Unnecessary initialization.
> + struct xenbus_transaction xbt;
> + struct xen_9pfs_front_priv *priv = NULL;
> + char *versions;
> + unsigned int max_rings, max_ring_order, len;
> +
> + versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
> + if (!len || strcmp(versions, "1"))
> + return -EINVAL;
> + kfree(versions);
> + ret = xenbus_scanf(XBT_NIL, dev->otherend, "max-rings", "%u", &max_rings);
> + if (ret < 0 || max_rings < XEN_9PFS_NUM_RINGS)
> + return -EINVAL;
> + ret = xenbus_scanf(XBT_NIL, dev->otherend, "max-ring-page-order", "%u", &max_ring_order);
> + if (ret < 0|| max_ring_order < XEN_9PFS_RING_ORDER)
> + return -EINVAL;
> +
> +
> + priv = kzalloc(sizeof(struct xen_9pfs_front_priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = dev;
> + priv->num_rings = XEN_9PFS_NUM_RINGS;
> + priv->rings = kzalloc(sizeof(struct xen_9pfs_dataring) * priv->num_rings,
> + GFP_KERNEL);
> + if (!priv->rings) {
> + kfree(priv);
> + return -ENOMEM;
> + }
> +
> + again:
> + ret = xenbus_transaction_start(&xbt);
> + if (ret) {
> + xenbus_dev_fatal(dev, ret, "starting transaction");
> + goto error;
> + }
> + ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
> + if (ret)
> + goto error_xenbus;
> + ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", priv->num_rings);
> + if (ret)
> + goto error_xenbus;
> + for (i = 0; i < priv->num_rings; i++) {
> + char str[16];
> +
> + priv->rings[i].priv = priv;
> + ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i]);
Not for -EAGAIN, I think.
-boris
> + if (ret < 0)
> + goto error_xenbus;
> +
> + sprintf(str, "ring-ref%u", i);
> + ret = xenbus_printf(xbt, dev->nodename, str, "%d", priv->rings[i].ref);
> + if (ret)
> + goto error_xenbus;
> +
> + sprintf(str, "event-channel-%u", i);
> + ret = xenbus_printf(xbt, dev->nodename, str, "%u", priv->rings[i].evtchn);
> + if (ret)
> + goto error_xenbus;
> + }
> + priv->tag = xenbus_read(xbt, dev->nodename, "tag", NULL);
> + if (ret)
> + goto error_xenbus;
> + ret = xenbus_transaction_end(xbt, 0);
> + if (ret) {
> + if (ret == -EAGAIN)
> + goto again;
> + xenbus_dev_fatal(dev, ret, "completing transaction");
> + goto error;
> + }
> +
> +
> + list_add_tail(&priv->list, &xen_9pfs_devs);
> + dev_set_drvdata(&dev->dev, priv);
> + xenbus_switch_state(dev, XenbusStateInitialised);
> +
> return 0;
> +
> + error_xenbus:
> + xenbus_transaction_end(xbt, 1);
> + xenbus_dev_fatal(dev, ret, "writing xenstore");
> + error:
> + dev_set_drvdata(&dev->dev, NULL);
> + xen_9pfs_front_free(priv);
> + return ret;
> }
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-07 15:03 ` Boris Ostrovsky
@ 2017-03-08 0:11 ` Stefano Stabellini
2017-03-08 13:47 ` Boris Ostrovsky
0 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 0:11 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
xen-devel
On Tue, 7 Mar 2017, Boris Ostrovsky wrote:
> > +static int xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
> > +{
> > + int i, j;
> > +
> > + list_del(&priv->list);
> > +
> > + for (i = 0; i < priv->num_rings; i++) {
> > + if (priv->rings[i].intf == NULL)
> > + break;
>
> Are we guaranteed that all subsequent entries are not allocated (i.e.
> this shouldn't be 'continue')?
Yes, we are guaranteed that all subsequent entries are NULL because they
are allocated in order until an error occurs.
> > + if (priv->rings[i].irq > 0)
> > + unbind_from_irqhandler(priv->rings[i].irq, priv->dev);
> > + if (priv->rings[i].bytes != NULL) {
> > + for (j = 0; j < (1 << XEN_9PFS_RING_ORDER); j++)
> > + gnttab_end_foreign_access(priv->rings[i].intf->ref[j], 0, 0);
> > + free_pages((unsigned long)priv->rings[i].bytes, XEN_9PFS_RING_ORDER);
> > + }
> > + gnttab_end_foreign_access(priv->rings[i].ref, 0, 0);
> > + free_page((unsigned long)priv->rings[i].intf);
> > + }
> > + kfree(priv->rings);
> > + kfree(priv);
> > +
> > + return 0;
> > +}
> > +
> > static int xen_9pfs_front_remove(struct xenbus_device *dev)
> > {
> > + int ret;
> > + struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
> > +
> > + dev_set_drvdata(&dev->dev, NULL);
> > + ret = xen_9pfs_front_free(priv);
> > + return ret;
> > +}
> > +
> > +static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> > + struct xen_9pfs_dataring *ring)
> > +{
> > + int i;
> > + int ret = -ENOMEM;
> > +
> > + init_waitqueue_head(&ring->wq);
> > + spin_lock_init(&ring->lock);
> > + INIT_WORK(&ring->work, p9_xen_response);
> > +
> > + ring->intf = (struct xen_9pfs_data_intf *) __get_free_page(GFP_KERNEL | __GFP_ZERO);
> > + if (!ring->intf)
> > + goto error;
> > + memset(ring->intf, 0, XEN_PAGE_SIZE);
>
> get_zeroed_page()? (especially given that __get_free_page() returns
> PAGE_SIZE, not XEN_PAGE_SIZE)
Yes, good point.
> > + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, XEN_9PFS_RING_ORDER);
> > + if (ring->bytes == NULL)
> > + goto error;
> > + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> > + ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);
> > + ring->ref = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
> > + ring->ring.in = ring->bytes;
> > + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
> > +
> > + ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
> > + if (ret)
> > + goto error;
> > + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn, xen_9pfs_front_event_handler,
> > + 0, "xen_9pfs-frontend", ring);
> > + if (ring->irq < 0) {
> > + xenbus_free_evtchn(dev, ring->evtchn);
> > + ret = ring->irq;
> > + goto error;
> > + }
> > return 0;
> > +
> > +error:
>
> You may need to gnttab_end_foreign_access().
Actually this error path is unnecessary because it will be handled by
xen_9pfs_front_probe, that calls xen_9pfs_front_free on errors. I'll
remove it.
> > + if (ring->intf != NULL)
> > + kfree(ring->intf);
> > + if (ring->bytes != NULL)
> > + kfree(ring->bytes);
> > + return ret;
> > }
> >
> > static int xen_9pfs_front_probe(struct xenbus_device *dev,
> > const struct xenbus_device_id *id)
> > {
> > + int ret = -EFAULT, i;
>
> Unnecessary initialization.
I'll remove it.
> > + struct xenbus_transaction xbt;
> > + struct xen_9pfs_front_priv *priv = NULL;
> > + char *versions;
> > + unsigned int max_rings, max_ring_order, len;
> > +
> > + versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
> > + if (!len || strcmp(versions, "1"))
> > + return -EINVAL;
> > + kfree(versions);
> > + ret = xenbus_scanf(XBT_NIL, dev->otherend, "max-rings", "%u", &max_rings);
> > + if (ret < 0 || max_rings < XEN_9PFS_NUM_RINGS)
> > + return -EINVAL;
> > + ret = xenbus_scanf(XBT_NIL, dev->otherend, "max-ring-page-order", "%u", &max_ring_order);
> > + if (ret < 0|| max_ring_order < XEN_9PFS_RING_ORDER)
> > + return -EINVAL;
> > +
> > +
> > + priv = kzalloc(sizeof(struct xen_9pfs_front_priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->dev = dev;
> > + priv->num_rings = XEN_9PFS_NUM_RINGS;
> > + priv->rings = kzalloc(sizeof(struct xen_9pfs_dataring) * priv->num_rings,
> > + GFP_KERNEL);
> > + if (!priv->rings) {
> > + kfree(priv);
> > + return -ENOMEM;
> > + }
> > +
> > + again:
> > + ret = xenbus_transaction_start(&xbt);
> > + if (ret) {
> > + xenbus_dev_fatal(dev, ret, "starting transaction");
> > + goto error;
> > + }
> > + ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
> > + if (ret)
> > + goto error_xenbus;
> > + ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", priv->num_rings);
> > + if (ret)
> > + goto error_xenbus;
> > + for (i = 0; i < priv->num_rings; i++) {
> > + char str[16];
> > +
> > + priv->rings[i].priv = priv;
> > + ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i]);
>
> Not for -EAGAIN, I think.
I don't think xen_9pfs_front_alloc_dataring can return EAGAIN. EAGAIN
can only come from xenbus_transaction_end, the case we handle below.
> > + if (ret < 0)
> > + goto error_xenbus;
> > +
> > + sprintf(str, "ring-ref%u", i);
> > + ret = xenbus_printf(xbt, dev->nodename, str, "%d", priv->rings[i].ref);
> > + if (ret)
> > + goto error_xenbus;
> > +
> > + sprintf(str, "event-channel-%u", i);
> > + ret = xenbus_printf(xbt, dev->nodename, str, "%u", priv->rings[i].evtchn);
> > + if (ret)
> > + goto error_xenbus;
> > + }
> > + priv->tag = xenbus_read(xbt, dev->nodename, "tag", NULL);
> > + if (ret)
> > + goto error_xenbus;
> > + ret = xenbus_transaction_end(xbt, 0);
> > + if (ret) {
> > + if (ret == -EAGAIN)
> > + goto again;
> > + xenbus_dev_fatal(dev, ret, "completing transaction");
> > + goto error;
> > + }
> > +
> > +
> > + list_add_tail(&priv->list, &xen_9pfs_devs);
> > + dev_set_drvdata(&dev->dev, priv);
> > + xenbus_switch_state(dev, XenbusStateInitialised);
> > +
> > return 0;
> > +
> > + error_xenbus:
> > + xenbus_transaction_end(xbt, 1);
> > + xenbus_dev_fatal(dev, ret, "writing xenstore");
> > + error:
> > + dev_set_drvdata(&dev->dev, NULL);
> > + xen_9pfs_front_free(priv);
> > + return ret;
> > }
> >
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-08 0:11 ` Stefano Stabellini
@ 2017-03-08 13:47 ` Boris Ostrovsky
2017-03-08 19:22 ` Stefano Stabellini
0 siblings, 1 reply; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-08 13:47 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich, xen-devel
>
>>> + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, XEN_9PFS_RING_ORDER);
>>> + if (ring->bytes == NULL)
>>> + goto error;
>>> + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
>>> + ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);
>>> + ring->ref = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
>>> + ring->ring.in = ring->bytes;
>>> + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
>>> +
>>> + ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
>>> + if (ret)
>>> + goto error;
>>> + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn, xen_9pfs_front_event_handler,
>>> + 0, "xen_9pfs-frontend", ring);
>>> + if (ring->irq < 0) {
>>> + xenbus_free_evtchn(dev, ring->evtchn);
>>> + ret = ring->irq;
>>> + goto error;
>>> + }
>>> return 0;
>>> +
>>> +error:
>> You may need to gnttab_end_foreign_access().
> Actually this error path is unnecessary because it will be handled by
> xen_9pfs_front_probe, that calls xen_9pfs_front_free on errors. I'll
> remove it.
(It's a matter of personal preference but I think a routine should clean
up its own mess whenever it can.)
>
>
> +
> + again:
> + ret = xenbus_transaction_start(&xbt);
> + if (ret) {
> + xenbus_dev_fatal(dev, ret, "starting transaction");
> + goto error;
> + }
> + ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
> + if (ret)
> + goto error_xenbus;
> + ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", priv->num_rings);
> + if (ret)
> + goto error_xenbus;
> + for (i = 0; i < priv->num_rings; i++) {
> + char str[16];
> +
> + priv->rings[i].priv = priv;
> + ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i]);
>> Not for -EAGAIN, I think.
> I don't think xen_9pfs_front_alloc_dataring can return EAGAIN. EAGAIN
> can only come from xenbus_transaction_end, the case we handle below.
I didn't mean that xen_9pfs_front_alloc_dataring() can return -EAGAIN. I
was referring to...
>
>
>>> + if (ret < 0)
>>> + goto error_xenbus;
>>> +
>>> + sprintf(str, "ring-ref%u", i);
>>> + ret = xenbus_printf(xbt, dev->nodename, str, "%d", priv->rings[i].ref);
>>> + if (ret)
>>> + goto error_xenbus;
>>> +
>>> + sprintf(str, "event-channel-%u", i);
>>> + ret = xenbus_printf(xbt, dev->nodename, str, "%u", priv->rings[i].evtchn);
>>> + if (ret)
>>> + goto error_xenbus;
>>> + }
>>> + priv->tag = xenbus_read(xbt, dev->nodename, "tag", NULL);
>>> + if (ret)
>>> + goto error_xenbus;
>>> + ret = xenbus_transaction_end(xbt, 0);
>>> + if (ret) {
>>> + if (ret == -EAGAIN)
>>> + goto again;
... this.
Sorry for not being clear.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-08 13:47 ` Boris Ostrovsky
@ 2017-03-08 19:22 ` Stefano Stabellini
0 siblings, 0 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 19:22 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
xen-devel
On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>> + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, XEN_9PFS_RING_ORDER);
> >>> + if (ring->bytes == NULL)
> >>> + goto error;
> >>> + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> >>> + ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);
> >>> + ring->ref = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
> >>> + ring->ring.in = ring->bytes;
> >>> + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
> >>> +
> >>> + ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
> >>> + if (ret)
> >>> + goto error;
> >>> + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn, xen_9pfs_front_event_handler,
> >>> + 0, "xen_9pfs-frontend", ring);
> >>> + if (ring->irq < 0) {
> >>> + xenbus_free_evtchn(dev, ring->evtchn);
> >>> + ret = ring->irq;
> >>> + goto error;
> >>> + }
> >>> return 0;
> >>> +
> >>> +error:
> >> You may need to gnttab_end_foreign_access().
> > Actually this error path is unnecessary because it will be handled by
> > xen_9pfs_front_probe, that calls xen_9pfs_front_free on errors. I'll
> > remove it.
>
>
> (It's a matter of personal preference but I think a routine should clean
> up its own mess whenever it can.)
>
> >
> >
> > +
> > + again:
> > + ret = xenbus_transaction_start(&xbt);
> > + if (ret) {
> > + xenbus_dev_fatal(dev, ret, "starting transaction");
> > + goto error;
> > + }
> > + ret = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
> > + if (ret)
> > + goto error_xenbus;
> > + ret = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", priv->num_rings);
> > + if (ret)
> > + goto error_xenbus;
> > + for (i = 0; i < priv->num_rings; i++) {
> > + char str[16];
> > +
> > + priv->rings[i].priv = priv;
> > + ret = xen_9pfs_front_alloc_dataring(dev, &priv->rings[i]);
> >> Not for -EAGAIN, I think.
> > I don't think xen_9pfs_front_alloc_dataring can return EAGAIN. EAGAIN
> > can only come from xenbus_transaction_end, the case we handle below.
>
> I didn't mean that xen_9pfs_front_alloc_dataring() can return -EAGAIN. I
> was referring to...
>
> >
> >
> >>> + if (ret < 0)
> >>> + goto error_xenbus;
> >>> +
> >>> + sprintf(str, "ring-ref%u", i);
> >>> + ret = xenbus_printf(xbt, dev->nodename, str, "%d", priv->rings[i].ref);
> >>> + if (ret)
> >>> + goto error_xenbus;
> >>> +
> >>> + sprintf(str, "event-channel-%u", i);
> >>> + ret = xenbus_printf(xbt, dev->nodename, str, "%u", priv->rings[i].evtchn);
> >>> + if (ret)
> >>> + goto error_xenbus;
> >>> + }
> >>> + priv->tag = xenbus_read(xbt, dev->nodename, "tag", NULL);
> >>> + if (ret)
> >>> + goto error_xenbus;
> >>> + ret = xenbus_transaction_end(xbt, 0);
> >>> + if (ret) {
> >>> + if (ret == -EAGAIN)
> >>> + goto again;
>
> ... this.
>
> Sorry for not being clear.
You are right! I'll move the call to xen_9pfs_front_alloc_dataring out
of the xenbus loop.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-06 20:01 ` [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini
2017-03-07 15:03 ` Boris Ostrovsky
@ 2017-03-07 17:37 ` Julien Grall
2017-03-08 0:49 ` Stefano Stabellini
1 sibling, 1 reply; 38+ messages in thread
From: Julien Grall @ 2017-03-07 17:37 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, Ron Minnich, v9fs-developer, boris.ostrovsky
Hi Stefano,
On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
> +static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> + struct xen_9pfs_dataring *ring)
> +{
> + int i;
> + int ret = -ENOMEM;
> +
> + init_waitqueue_head(&ring->wq);
> + spin_lock_init(&ring->lock);
> + INIT_WORK(&ring->work, p9_xen_response);
> +
> + ring->intf = (struct xen_9pfs_data_intf *) __get_free_page(GFP_KERNEL | __GFP_ZERO);
> + if (!ring->intf)
> + goto error;
> + memset(ring->intf, 0, XEN_PAGE_SIZE);
> + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO, XEN_9PFS_RING_ORDER);
The ring order will be in term of Xen page size and not Linux. So you
are going to allocate much more memory than expected on 64KB kernel.
> + if (ring->bytes == NULL)
> + goto error;
> + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> + ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);.
Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
Also, this is not going to work on 64K kernel because you will grant
access to noncontiguous memory (e.g 0-4K, 64K-68K,...).
We have various helper to break-down the page for you, see
gnttab_for_one_grant, gnttab_foreach_grant, gnttab_count_grant,
xen_for_each_gfn (though this one it is internal to xlate_mmu.c so far)
Please use them to avoid any further.
> + ring->ref = gnttab_grant_foreign_access(dev->otherend_id, pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
> + ring->ring.in = ring->bytes;
> + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
> +
> + ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
> + if (ret)
> + goto error;
> + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn, xen_9pfs_front_event_handler,
> + 0, "xen_9pfs-frontend", ring);
> + if (ring->irq < 0) {
> + xenbus_free_evtchn(dev, ring->evtchn);
> + ret = ring->irq;
> + goto error;
> + }
> return 0;
> +
> +error:
> + if (ring->intf != NULL)
> + kfree(ring->intf);
> + if (ring->bytes != NULL)
> + kfree(ring->bytes);
> + return ret;
> }
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-07 17:37 ` Julien Grall
@ 2017-03-08 0:49 ` Stefano Stabellini
2017-03-08 12:25 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 0:49 UTC (permalink / raw)
To: Julien Grall
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
Ron Minnich, linux-kernel, Stefano Stabellini, v9fs-developer,
xen-devel, boris.ostrovsky
On Tue, 7 Mar 2017, Julien Grall wrote:
> Hi Stefano,
>
> On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
> > +static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> > + struct xen_9pfs_dataring *ring)
> > +{
> > + int i;
> > + int ret = -ENOMEM;
> > +
> > + init_waitqueue_head(&ring->wq);
> > + spin_lock_init(&ring->lock);
> > + INIT_WORK(&ring->work, p9_xen_response);
> > +
> > + ring->intf = (struct xen_9pfs_data_intf *) __get_free_page(GFP_KERNEL
> > | __GFP_ZERO);
> > + if (!ring->intf)
> > + goto error;
> > + memset(ring->intf, 0, XEN_PAGE_SIZE);
> > + ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> > XEN_9PFS_RING_ORDER);
>
> The ring order will be in term of Xen page size and not Linux. So you are
> going to allocate much more memory than expected on 64KB kernel.
I'll fix.
> > + if (ring->bytes == NULL)
> > + goto error;
> > + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> > + ring->intf->ref[i] =
> > gnttab_grant_foreign_access(dev->otherend_id,
> > pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);.
>
> Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
OK
> Also, this is not going to work on 64K kernel because you will grant access to
> noncontiguous memory (e.g 0-4K, 64K-68K,...).
By using virt_to_gfn like you suggested, the loop will correctly iterate
on a 4K by 4K basis, even on a 64K kernel:
ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
XEN_9PFS_RING_ORDER - (PAGE_SHIFT - XEN_PAGE_SHIFT));
for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, virt_to_gfn((void*)ring->bytes) + i, 0);
where XEN_9PFS_RING_ORDER specifies the order at 4K granularity. Am I
missing something?
> We have various helper to break-down the page for you, see
> gnttab_for_one_grant, gnttab_foreach_grant, gnttab_count_grant,
> xen_for_each_gfn (though this one it is internal to xlate_mmu.c so far)
>
> Please use them to avoid any further.
>
> > + ring->ref = gnttab_grant_foreign_access(dev->otherend_id,
> > pfn_to_gfn(virt_to_pfn((void*)ring->intf)), 0);
>
> Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
Sure
> > + ring->ring.in = ring->bytes;
> > + ring->ring.out = ring->bytes + XEN_9PFS_RING_SIZE;
> > +
> > + ret = xenbus_alloc_evtchn(dev, &ring->evtchn);
> > + if (ret)
> > + goto error;
> > + ring->irq = bind_evtchn_to_irqhandler(ring->evtchn,
> > xen_9pfs_front_event_handler,
> > + 0, "xen_9pfs-frontend", ring);
> > + if (ring->irq < 0) {
> > + xenbus_free_evtchn(dev, ring->evtchn);
> > + ret = ring->irq;
> > + goto error;
> > + }
> > return 0;
> > +
> > +error:
> > + if (ring->intf != NULL)
> > + kfree(ring->intf);
> > + if (ring->bytes != NULL)
> > + kfree(ring->bytes);
> > + return ret;
> > }
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-08 0:49 ` Stefano Stabellini
@ 2017-03-08 12:25 ` Julien Grall
2017-03-08 19:08 ` Stefano Stabellini
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2017-03-08 12:25 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, Ron Minnich, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, xen-devel,
boris.ostrovsky
Hi Stefano,
On 08/03/17 00:49, Stefano Stabellini wrote:
> On Tue, 7 Mar 2017, Julien Grall wrote:
>> On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
>>> + if (ring->bytes == NULL)
>>> + goto error;
>>> + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
>>> + ring->intf->ref[i] =
>>> gnttab_grant_foreign_access(dev->otherend_id,
>>> pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);.
>>
>> Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
>
> OK
>
>
>> Also, this is not going to work on 64K kernel because you will grant access to
>> noncontiguous memory (e.g 0-4K, 64K-68K,...).
>
> By using virt_to_gfn like you suggested, the loop will correctly iterate
> on a 4K by 4K basis, even on a 64K kernel:
>
> ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> XEN_9PFS_RING_ORDER - (PAGE_SHIFT - XEN_PAGE_SHIFT));
> for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id, virt_to_gfn((void*)ring->bytes) + i, 0);
BTW, the cast (void *) is not necessary.
>
> where XEN_9PFS_RING_ORDER specifies the order at 4K granularity. Am I
> missing something?
I think it is fine. You could move virt_to_gfn(...) outside and avoid to
do the translation everytime.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/7] xen/9pfs: connect to the backend
2017-03-08 12:25 ` Julien Grall
@ 2017-03-08 19:08 ` Stefano Stabellini
0 siblings, 0 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 19:08 UTC (permalink / raw)
To: Julien Grall
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
Ron Minnich, linux-kernel, Stefano Stabellini, v9fs-developer,
xen-devel, boris.ostrovsky
On Wed, 8 Mar 2017, Julien Grall wrote:
> Hi Stefano,
>
> On 08/03/17 00:49, Stefano Stabellini wrote:
> > On Tue, 7 Mar 2017, Julien Grall wrote:
> > > On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
> > > > + if (ring->bytes == NULL)
> > > > + goto error;
> > > > + for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> > > > + ring->intf->ref[i] =
> > > > gnttab_grant_foreign_access(dev->otherend_id,
> > > > pfn_to_gfn(virt_to_pfn((void*)ring->bytes) + i), 0);.
> > >
> > > Please use virt_to_gfn rather than pfn_to_gfn(virt_to_pfn).
> >
> > OK
> >
> >
> > > Also, this is not going to work on 64K kernel because you will grant
> > > access to
> > > noncontiguous memory (e.g 0-4K, 64K-68K,...).
> >
> > By using virt_to_gfn like you suggested, the loop will correctly iterate
> > on a 4K by 4K basis, even on a 64K kernel:
> >
> > ring->bytes = (void*)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> > XEN_9PFS_RING_ORDER - (PAGE_SHIFT - XEN_PAGE_SHIFT));
> > for (i = 0; i < (1 << XEN_9PFS_RING_ORDER); i++)
> > ring->intf->ref[i] = gnttab_grant_foreign_access(dev->otherend_id,
> > virt_to_gfn((void*)ring->bytes) + i, 0);
>
> BTW, the cast (void *) is not necessary.
>
> >
> > where XEN_9PFS_RING_ORDER specifies the order at 4K granularity. Am I
> > missing something?
>
> I think it is fine. You could move virt_to_gfn(...) outside and avoid to do
> the translation everytime.
All right, thanks.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 5/7] xen/9pfs: send requests to the backend
2017-03-06 20:01 ` [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini
` (2 preceding siblings ...)
2017-03-06 20:01 ` [PATCH 4/7] xen/9pfs: connect to the backend Stefano Stabellini
@ 2017-03-06 20:01 ` Stefano Stabellini
2017-03-07 15:27 ` Boris Ostrovsky
2017-03-06 20:01 ` [PATCH 6/7] xen/9pfs: receive responses Stefano Stabellini
` (2 subsequent siblings)
6 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-06 20:01 UTC (permalink / raw)
To: xen-devel
Cc: jgross, Latchesar Ionkov, sstabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
boris.ostrovsky
Implement struct p9_trans_module create and close functions by looking
at the available Xen 9pfs frontend-backend connections. We don't expect
many frontend-backend connections, thus walking a list is OK.
Send requests to the backend by copying each request to one of the
available rings (each frontend-backend connection comes with multiple
rings). Handle the ring and notifications following the 9pfs
specification. If there are not enough free bytes on the ring for the
request, wait on the wait_queue: the backend will send a notification
after consuming more requests.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
CC: Eric Van Hensbergen <ericvh@gmail.com>
CC: Ron Minnich <rminnich@sandia.gov>
CC: Latchesar Ionkov <lucho@ionkov.net>
CC: v9fs-developer@lists.sourceforge.net
---
net/9p/trans_xen.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 82 insertions(+), 1 deletion(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 9f6cf8d..4e26556 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -47,22 +47,103 @@ struct xen_9pfs_front_priv {
};
static LIST_HEAD(xen_9pfs_devs);
+/* We don't currently allow canceling of requests */
static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
{
- return 0;
+ return 1;
}
static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
{
+ struct xen_9pfs_front_priv *priv = NULL;
+
+ list_for_each_entry(priv, &xen_9pfs_devs, list) {
+ if (!strcmp(priv->tag, addr))
+ break;
+ }
+ if (!priv || strcmp(priv->tag, addr))
+ return -EINVAL;
+
+ priv->client = client;
return 0;
}
static void p9_xen_close(struct p9_client *client)
{
+ struct xen_9pfs_front_priv *priv = NULL;
+
+ list_for_each_entry(priv, &xen_9pfs_devs, list) {
+ if (priv->client == client)
+ break;
+ }
+ if (!priv || priv->client != client)
+ return;
+
+ priv->client = NULL;
+ return;
+}
+
+static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
+{
+ RING_IDX cons, prod;
+
+ cons = ring->intf->out_cons;
+ prod = ring->intf->out_prod;
+ mb();
+
+ if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
+ return 1;
+ else
+ return 0;
}
static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
{
+ struct xen_9pfs_front_priv *priv = NULL;
+ RING_IDX cons, prod, masked_cons, masked_prod;
+ unsigned long flags;
+ uint32_t size = p9_req->tc->size;
+ struct xen_9pfs_dataring *ring;
+ int num;
+
+ list_for_each_entry(priv, &xen_9pfs_devs, list) {
+ if (priv->client == client)
+ break;
+ }
+ if (priv == NULL || priv->client != client)
+ return -EINVAL;
+
+ num = p9_req->tc->tag % priv->num_rings;
+ ring = &priv->rings[num];
+
+again:
+ while (wait_event_interruptible(ring->wq,
+ p9_xen_write_todo(ring, size) > 0) != 0);
+
+ spin_lock_irqsave(&ring->lock, flags);
+ cons = ring->intf->out_cons;
+ prod = ring->intf->out_prod;
+ mb();
+
+ if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
+ spin_unlock_irqrestore(&ring->lock, flags);
+ goto again;
+ }
+
+ masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+
+ xen_9pfs_write_packet(ring->ring.out,
+ &masked_prod, masked_cons,
+ XEN_9PFS_RING_SIZE, p9_req->tc->sdata, size);
+
+ p9_req->status = REQ_STATUS_SENT;
+ wmb(); /* write ring before updating pointer */
+ prod += size;
+ ring->intf->out_prod = prod;
+ spin_unlock_irqrestore(&ring->lock, flags);
+ notify_remote_via_irq(ring->irq);
+
return 0;
}
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 5/7] xen/9pfs: send requests to the backend
2017-03-06 20:01 ` [PATCH 5/7] xen/9pfs: send requests " Stefano Stabellini
@ 2017-03-07 15:27 ` Boris Ostrovsky
2017-03-08 0:55 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1703071650320.17906@sstabellini-ThinkPad-X260>
0 siblings, 2 replies; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-07 15:27 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich
On 03/06/2017 03:01 PM, Stefano Stabellini wrote:
> Implement struct p9_trans_module create and close functions by looking
> at the available Xen 9pfs frontend-backend connections. We don't expect
> many frontend-backend connections, thus walking a list is OK.
>
> Send requests to the backend by copying each request to one of the
> available rings (each frontend-backend connection comes with multiple
> rings). Handle the ring and notifications following the 9pfs
> specification. If there are not enough free bytes on the ring for the
> request, wait on the wait_queue: the backend will send a notification
> after consuming more requests.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> CC: Eric Van Hensbergen <ericvh@gmail.com>
> CC: Ron Minnich <rminnich@sandia.gov>
> CC: Latchesar Ionkov <lucho@ionkov.net>
> CC: v9fs-developer@lists.sourceforge.net
> ---
> net/9p/trans_xen.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index 9f6cf8d..4e26556 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -47,22 +47,103 @@ struct xen_9pfs_front_priv {
> };
> static LIST_HEAD(xen_9pfs_devs);
>
> +/* We don't currently allow canceling of requests */
> static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
> {
> - return 0;
> + return 1;
> }
>
> static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
> {
> + struct xen_9pfs_front_priv *priv = NULL;
> +
> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> + if (!strcmp(priv->tag, addr))
> + break;
> + }
You could simplify this (and p9_xen_close()) but assigning client and
returning from inside the 'if' statement.
I am also not sure you need to initialize priv.
> + if (!priv || strcmp(priv->tag, addr))
> + return -EINVAL;
> +
> + priv->client = client;
> return 0;
> }
>
> static void p9_xen_close(struct p9_client *client)
> {
> + struct xen_9pfs_front_priv *priv = NULL;
> +
> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> + if (priv->client == client)
> + break;
> + }
> + if (!priv || priv->client != client)
> + return;
> +
> + priv->client = NULL;
> + return;
> +}
> +
> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> +{
> + RING_IDX cons, prod;
> +
> + cons = ring->intf->out_cons;
> + prod = ring->intf->out_prod;
> + mb();
> +
> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> + return 1;
> + else
> + return 0;
> }
>
> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> {
> + struct xen_9pfs_front_priv *priv = NULL;
> + RING_IDX cons, prod, masked_cons, masked_prod;
> + unsigned long flags;
> + uint32_t size = p9_req->tc->size;
> + struct xen_9pfs_dataring *ring;
> + int num;
> +
> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> + if (priv->client == client)
> + break;
> + }
> + if (priv == NULL || priv->client != client)
> + return -EINVAL;
> +
> + num = p9_req->tc->tag % priv->num_rings;
> + ring = &priv->rings[num];
> +
> +again:
> + while (wait_event_interruptible(ring->wq,
> + p9_xen_write_todo(ring, size) > 0) != 0);
> +
> + spin_lock_irqsave(&ring->lock, flags);
> + cons = ring->intf->out_cons;
> + prod = ring->intf->out_prod;
> + mb();
> +
> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
This looks like p9_xen_write_todo(). BTW, where is xen_9pfs_queued()
defined? I couldn't find it. Same for xen_9pfs_mask() and
xen_9pfs_write_packet().
-boris
> + spin_unlock_irqrestore(&ring->lock, flags);
> + goto again;
> + }
> +
> + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> +
> + xen_9pfs_write_packet(ring->ring.out,
> + &masked_prod, masked_cons,
> + XEN_9PFS_RING_SIZE, p9_req->tc->sdata, size);
> +
> + p9_req->status = REQ_STATUS_SENT;
> + wmb(); /* write ring before updating pointer */
> + prod += size;
> + ring->intf->out_prod = prod;
> + spin_unlock_irqrestore(&ring->lock, flags);
> + notify_remote_via_irq(ring->irq);
> +
> return 0;
> }
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/7] xen/9pfs: send requests to the backend
2017-03-07 15:27 ` Boris Ostrovsky
@ 2017-03-08 0:55 ` Stefano Stabellini
[not found] ` <alpine.DEB.2.10.1703071650320.17906@sstabellini-ThinkPad-X260>
1 sibling, 0 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 0:55 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
xen-devel
On Tue, 7 Mar 2017, Boris Ostrovsky wrote:
> On 03/06/2017 03:01 PM, Stefano Stabellini wrote:
> > Implement struct p9_trans_module create and close functions by looking
> > at the available Xen 9pfs frontend-backend connections. We don't expect
> > many frontend-backend connections, thus walking a list is OK.
> >
> > Send requests to the backend by copying each request to one of the
> > available rings (each frontend-backend connection comes with multiple
> > rings). Handle the ring and notifications following the 9pfs
> > specification. If there are not enough free bytes on the ring for the
> > request, wait on the wait_queue: the backend will send a notification
> > after consuming more requests.
> >
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > CC: Eric Van Hensbergen <ericvh@gmail.com>
> > CC: Ron Minnich <rminnich@sandia.gov>
> > CC: Latchesar Ionkov <lucho@ionkov.net>
> > CC: v9fs-developer@lists.sourceforge.net
> > ---
> > net/9p/trans_xen.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 82 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> > index 9f6cf8d..4e26556 100644
> > --- a/net/9p/trans_xen.c
> > +++ b/net/9p/trans_xen.c
> > @@ -47,22 +47,103 @@ struct xen_9pfs_front_priv {
> > };
> > static LIST_HEAD(xen_9pfs_devs);
> >
> > +/* We don't currently allow canceling of requests */
> > static int p9_xen_cancel(struct p9_client *client, struct p9_req_t *req)
> > {
> > - return 0;
> > + return 1;
> > }
> >
> > static int p9_xen_create(struct p9_client *client, const char *addr, char *args)
> > {
> > + struct xen_9pfs_front_priv *priv = NULL;
> > +
> > + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> > + if (!strcmp(priv->tag, addr))
> > + break;
> > + }
>
>
> You could simplify this (and p9_xen_close()) but assigning client and
> returning from inside the 'if' statement.
I'll do that.
> I am also not sure you need to initialize priv.
With the new changes, I won't need to.
> > + if (!priv || strcmp(priv->tag, addr))
> > + return -EINVAL;
> > +
> > + priv->client = client;
> > return 0;
> > }
> >
> > static void p9_xen_close(struct p9_client *client)
> > {
> > + struct xen_9pfs_front_priv *priv = NULL;
> > +
> > + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> > + if (priv->client == client)
> > + break;
> > + }
> > + if (!priv || priv->client != client)
> > + return;
> > +
> > + priv->client = NULL;
> > + return;
> > +}
> > +
> > +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> > +{
> > + RING_IDX cons, prod;
> > +
> > + cons = ring->intf->out_cons;
> > + prod = ring->intf->out_prod;
> > + mb();
> > +
> > + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> > + return 1;
> > + else
> > + return 0;
> > }
> >
> > static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> > {
> > + struct xen_9pfs_front_priv *priv = NULL;
> > + RING_IDX cons, prod, masked_cons, masked_prod;
> > + unsigned long flags;
> > + uint32_t size = p9_req->tc->size;
> > + struct xen_9pfs_dataring *ring;
> > + int num;
> > +
> > + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> > + if (priv->client == client)
> > + break;
> > + }
> > + if (priv == NULL || priv->client != client)
> > + return -EINVAL;
> > +
> > + num = p9_req->tc->tag % priv->num_rings;
> > + ring = &priv->rings[num];
> > +
> > +again:
> > + while (wait_event_interruptible(ring->wq,
> > + p9_xen_write_todo(ring, size) > 0) != 0);
> > +
> > + spin_lock_irqsave(&ring->lock, flags);
> > + cons = ring->intf->out_cons;
> > + prod = ring->intf->out_prod;
> > + mb();
> > +
> > + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
>
>
> This looks like p9_xen_write_todo().
p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
a return value that works well with wait_event_interruptible.
I would prefer not to call p9_xen_write_todo here, because it's simpler
if we don't read prod and cons twice.
> BTW, where is xen_9pfs_queued()
> defined? I couldn't find it. Same for xen_9pfs_mask() and
> xen_9pfs_write_packet().
They are provided by the new ring macros, see
include/xen/interface/io/ring.h (the first patch).
> > + spin_unlock_irqrestore(&ring->lock, flags);
> > + goto again;
> > + }
> > +
> > + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> > + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> > +
> > + xen_9pfs_write_packet(ring->ring.out,
> > + &masked_prod, masked_cons,
> > + XEN_9PFS_RING_SIZE, p9_req->tc->sdata, size);
> > +
> > + p9_req->status = REQ_STATUS_SENT;
> > + wmb(); /* write ring before updating pointer */
> > + prod += size;
> > + ring->intf->out_prod = prod;
> > + spin_unlock_irqrestore(&ring->lock, flags);
> > + notify_remote_via_irq(ring->irq);
> > +
> > return 0;
> > }
> >
> >
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread[parent not found: <alpine.DEB.2.10.1703071650320.17906@sstabellini-ThinkPad-X260>]
* Re: [PATCH 5/7] xen/9pfs: send requests to the backend
[not found] ` <alpine.DEB.2.10.1703071650320.17906@sstabellini-ThinkPad-X260>
@ 2017-03-08 13:58 ` Boris Ostrovsky
2017-03-08 19:33 ` Stefano Stabellini
0 siblings, 1 reply; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-08 13:58 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich, xen-devel
>>> +}
>>> +
>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
>>> +{
>>> + RING_IDX cons, prod;
>>> +
>>> + cons = ring->intf->out_cons;
>>> + prod = ring->intf->out_prod;
>>> + mb();
>>> +
>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
>>> + return 1;
>>> + else
>>> + return 0;
>>> }
>>>
>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>>> {
>>> + struct xen_9pfs_front_priv *priv = NULL;
>>> + RING_IDX cons, prod, masked_cons, masked_prod;
>>> + unsigned long flags;
>>> + uint32_t size = p9_req->tc->size;
>>> + struct xen_9pfs_dataring *ring;
>>> + int num;
>>> +
>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
>>> + if (priv->client == client)
>>> + break;
>>> + }
>>> + if (priv == NULL || priv->client != client)
>>> + return -EINVAL;
>>> +
>>> + num = p9_req->tc->tag % priv->num_rings;
>>> + ring = &priv->rings[num];
>>> +
>>> +again:
>>> + while (wait_event_interruptible(ring->wq,
>>> + p9_xen_write_todo(ring, size) > 0) != 0);
>>> +
>>> + spin_lock_irqsave(&ring->lock, flags);
>>> + cons = ring->intf->out_cons;
>>> + prod = ring->intf->out_prod;
>>> + mb();
>>> +
>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
>>
>> This looks like p9_xen_write_todo().
> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> a return value that works well with wait_event_interruptible.
>
> I would prefer not to call p9_xen_write_todo here, because it's simpler
> if we don't read prod and cons twice.
I was referring to the whole code fragment after spin_lock_irqsave(),
not just the last line. Isn't it exactly !p9_xen_write_todo()?
>
>
>> BTW, where is xen_9pfs_queued()
>> defined? I couldn't find it. Same for xen_9pfs_mask() and
>> xen_9pfs_write_packet().
> They are provided by the new ring macros, see
> include/xen/interface/io/ring.h (the first patch).
Oh, right. I was searching for the string literally.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/7] xen/9pfs: send requests to the backend
2017-03-08 13:58 ` Boris Ostrovsky
@ 2017-03-08 19:33 ` Stefano Stabellini
2017-03-08 20:02 ` Boris Ostrovsky
0 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 19:33 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
xen-devel
On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>> +}
> >>> +
> >>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> >>> +{
> >>> + RING_IDX cons, prod;
> >>> +
> >>> + cons = ring->intf->out_cons;
> >>> + prod = ring->intf->out_prod;
> >>> + mb();
> >>> +
> >>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> >>> + return 1;
> >>> + else
> >>> + return 0;
> >>> }
> >>>
> >>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >>> {
> >>> + struct xen_9pfs_front_priv *priv = NULL;
> >>> + RING_IDX cons, prod, masked_cons, masked_prod;
> >>> + unsigned long flags;
> >>> + uint32_t size = p9_req->tc->size;
> >>> + struct xen_9pfs_dataring *ring;
> >>> + int num;
> >>> +
> >>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> >>> + if (priv->client == client)
> >>> + break;
> >>> + }
> >>> + if (priv == NULL || priv->client != client)
> >>> + return -EINVAL;
> >>> +
> >>> + num = p9_req->tc->tag % priv->num_rings;
> >>> + ring = &priv->rings[num];
> >>> +
> >>> +again:
> >>> + while (wait_event_interruptible(ring->wq,
> >>> + p9_xen_write_todo(ring, size) > 0) != 0);
> >>> +
> >>> + spin_lock_irqsave(&ring->lock, flags);
> >>> + cons = ring->intf->out_cons;
> >>> + prod = ring->intf->out_prod;
> >>> + mb();
> >>> +
> >>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
> >>
> >> This looks like p9_xen_write_todo().
> > p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> > a return value that works well with wait_event_interruptible.
> >
> > I would prefer not to call p9_xen_write_todo here, because it's simpler
> > if we don't read prod and cons twice.
>
> I was referring to the whole code fragment after spin_lock_irqsave(),
> not just the last line. Isn't it exactly !p9_xen_write_todo()?
Yes, it is true they are almost the same. The difference, and the reason
for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
the wait_event_interruptible loop, as such it needs to read prod and
cons every time. On the other end, here we want to read them once. Does
it make sense?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/7] xen/9pfs: send requests to the backend
2017-03-08 19:33 ` Stefano Stabellini
@ 2017-03-08 20:02 ` Boris Ostrovsky
2017-03-08 20:56 ` Stefano Stabellini
0 siblings, 1 reply; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-08 20:02 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich, xen-devel
On 03/08/2017 02:33 PM, Stefano Stabellini wrote:
> On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
>>>>> +}
>>>>> +
>>>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
>>>>> +{
>>>>> + RING_IDX cons, prod;
>>>>> +
>>>>> + cons = ring->intf->out_cons;
>>>>> + prod = ring->intf->out_prod;
>>>>> + mb();
>>>>> +
>>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
>>>>> + return 1;
>>>>> + else
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>>>>> {
>>>>> + struct xen_9pfs_front_priv *priv = NULL;
>>>>> + RING_IDX cons, prod, masked_cons, masked_prod;
>>>>> + unsigned long flags;
>>>>> + uint32_t size = p9_req->tc->size;
>>>>> + struct xen_9pfs_dataring *ring;
>>>>> + int num;
>>>>> +
>>>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
>>>>> + if (priv->client == client)
>>>>> + break;
>>>>> + }
>>>>> + if (priv == NULL || priv->client != client)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + num = p9_req->tc->tag % priv->num_rings;
>>>>> + ring = &priv->rings[num];
>>>>> +
>>>>> +again:
>>>>> + while (wait_event_interruptible(ring->wq,
>>>>> + p9_xen_write_todo(ring, size) > 0) != 0);
>>>>> +
>>>>> + spin_lock_irqsave(&ring->lock, flags);
>>>>> + cons = ring->intf->out_cons;
>>>>> + prod = ring->intf->out_prod;
>>>>> + mb();
>>>>> +
>>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
>>>> This looks like p9_xen_write_todo().
>>> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
>>> a return value that works well with wait_event_interruptible.
>>>
>>> I would prefer not to call p9_xen_write_todo here, because it's simpler
>>> if we don't read prod and cons twice.
>> I was referring to the whole code fragment after spin_lock_irqsave(),
>> not just the last line. Isn't it exactly !p9_xen_write_todo()?
> Yes, it is true they are almost the same. The difference, and the reason
> for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
> the wait_event_interruptible loop, as such it needs to read prod and
> cons every time. On the other end, here we want to read them once. Does
> it make sense?
I am clearly being particularly dense here but what I was thinking was:
again:
while (wait_event_interruptible(ring->wq,
p9_xen_write_todo(ring, size) > 0) != 0);
spin_lock_irqsave(&ring->lock, flags);
if (!p9_xen_write_todo(ring, size)) {
spin_unlock_irqrestore(&ring->lock, flags);
goto again;
}
There is no extra read of prod/cons.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/7] xen/9pfs: send requests to the backend
2017-03-08 20:02 ` Boris Ostrovsky
@ 2017-03-08 20:56 ` Stefano Stabellini
2017-03-08 21:01 ` Boris Ostrovsky
0 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 20:56 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
xen-devel
On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> On 03/08/2017 02:33 PM, Stefano Stabellini wrote:
> > On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>>>> +}
> >>>>> +
> >>>>> +static int p9_xen_write_todo(struct xen_9pfs_dataring *ring, RING_IDX size)
> >>>>> +{
> >>>>> + RING_IDX cons, prod;
> >>>>> +
> >>>>> + cons = ring->intf->out_cons;
> >>>>> + prod = ring->intf->out_prod;
> >>>>> + mb();
> >>>>> +
> >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size)
> >>>>> + return 1;
> >>>>> + else
> >>>>> + return 0;
> >>>>> }
> >>>>>
> >>>>> static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >>>>> {
> >>>>> + struct xen_9pfs_front_priv *priv = NULL;
> >>>>> + RING_IDX cons, prod, masked_cons, masked_prod;
> >>>>> + unsigned long flags;
> >>>>> + uint32_t size = p9_req->tc->size;
> >>>>> + struct xen_9pfs_dataring *ring;
> >>>>> + int num;
> >>>>> +
> >>>>> + list_for_each_entry(priv, &xen_9pfs_devs, list) {
> >>>>> + if (priv->client == client)
> >>>>> + break;
> >>>>> + }
> >>>>> + if (priv == NULL || priv->client != client)
> >>>>> + return -EINVAL;
> >>>>> +
> >>>>> + num = p9_req->tc->tag % priv->num_rings;
> >>>>> + ring = &priv->rings[num];
> >>>>> +
> >>>>> +again:
> >>>>> + while (wait_event_interruptible(ring->wq,
> >>>>> + p9_xen_write_todo(ring, size) > 0) != 0);
> >>>>> +
> >>>>> + spin_lock_irqsave(&ring->lock, flags);
> >>>>> + cons = ring->intf->out_cons;
> >>>>> + prod = ring->intf->out_prod;
> >>>>> + mb();
> >>>>> +
> >>>>> + if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < size) {
> >>>> This looks like p9_xen_write_todo().
> >>> p9_xen_write_todo is just a wrapper around xen_9pfs_queued to provide
> >>> a return value that works well with wait_event_interruptible.
> >>>
> >>> I would prefer not to call p9_xen_write_todo here, because it's simpler
> >>> if we don't read prod and cons twice.
> >> I was referring to the whole code fragment after spin_lock_irqsave(),
> >> not just the last line. Isn't it exactly !p9_xen_write_todo()?
> > Yes, it is true they are almost the same. The difference, and the reason
> > for p9_xen_write_todo to exist, is that p9_xen_write_todo is called in
> > the wait_event_interruptible loop, as such it needs to read prod and
> > cons every time. On the other end, here we want to read them once. Does
> > it make sense?
>
>
> I am clearly being particularly dense here but what I was thinking was:
>
> again:
> while (wait_event_interruptible(ring->wq,
> p9_xen_write_todo(ring, size) > 0) != 0);
>
> spin_lock_irqsave(&ring->lock, flags);
> if (!p9_xen_write_todo(ring, size)) {
> spin_unlock_irqrestore(&ring->lock, flags);
> goto again;
> }
>
> There is no extra read of prod/cons.
Yes, there are: just after this if statement we would have to read them
again to calculate masked_prod and masked_cons.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/7] xen/9pfs: send requests to the backend
2017-03-08 20:56 ` Stefano Stabellini
@ 2017-03-08 21:01 ` Boris Ostrovsky
0 siblings, 0 replies; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-08 21:01 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich, xen-devel
>> There is no extra read of prod/cons.
> Yes, there are: just after this if statement we would have to read them
> again to calculate masked_prod and masked_cons.
Ah, of course. Thanks.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 6/7] xen/9pfs: receive responses
2017-03-06 20:01 ` [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini
` (3 preceding siblings ...)
2017-03-06 20:01 ` [PATCH 5/7] xen/9pfs: send requests " Stefano Stabellini
@ 2017-03-06 20:01 ` Stefano Stabellini
2017-03-07 15:49 ` Boris Ostrovsky
2017-03-06 20:01 ` [PATCH 7/7] xen/9pfs: build 9pfs Xen transport driver Stefano Stabellini
2017-03-07 17:15 ` [PATCH 1/7] xen: import new ring macros in ring.h Julien Grall
6 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-06 20:01 UTC (permalink / raw)
To: xen-devel
Cc: jgross, Latchesar Ionkov, sstabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
boris.ostrovsky
Upon receiving a notification from the backend, schedule the
p9_xen_response work_struct. p9_xen_response checks if any responses are
available, if so, it reads them one by one, calling p9_client_cb to send
them up to the 9p layer (p9_client_cb completes the request). Handle the
ring following the Xen 9pfs specification.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
CC: Eric Van Hensbergen <ericvh@gmail.com>
CC: Ron Minnich <rminnich@sandia.gov>
CC: Latchesar Ionkov <lucho@ionkov.net>
CC: v9fs-developer@lists.sourceforge.net
---
net/9p/trans_xen.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 4e26556..1ca9246 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -149,6 +149,59 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
static void p9_xen_response(struct work_struct *work)
{
+ struct xen_9pfs_front_priv *priv;
+ struct xen_9pfs_dataring *ring;
+ RING_IDX cons, prod, masked_cons, masked_prod;
+ struct xen_9pfs_header h;
+ struct p9_req_t *req;
+ int status = REQ_STATUS_ERROR;
+
+ ring = container_of(work, struct xen_9pfs_dataring, work);
+ priv = ring->priv;
+
+ while (1) {
+ cons = ring->intf->in_cons;
+ prod = ring->intf->in_prod;
+ rmb();
+
+ if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < sizeof(h)) {
+ notify_remote_via_irq(ring->irq);
+ return;
+ }
+
+ masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+
+ xen_9pfs_read_packet(ring->ring.in,
+ masked_prod, &masked_cons,
+ XEN_9PFS_RING_SIZE, &h, sizeof(h));
+
+ req = p9_tag_lookup(priv->client, h.tag);
+ if (!req || req->status != REQ_STATUS_SENT) {
+ dev_warn(&priv->dev->dev, "Wrong req tag=%x\n", h.tag);
+ cons += h.size;
+ mb();
+ ring->intf->in_cons = cons;
+ continue;
+ }
+
+ memcpy(req->rc, &h, sizeof(h));
+ req->rc->offset = 0;
+
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+ xen_9pfs_read_packet(ring->ring.in,
+ masked_prod, &masked_cons,
+ XEN_9PFS_RING_SIZE, req->rc->sdata, h.size);
+
+ mb();
+ cons += h.size;
+ ring->intf->in_cons = cons;
+
+ if (req->status != REQ_STATUS_ERROR)
+ status = REQ_STATUS_RCVD;
+
+ p9_client_cb(priv->client, req, status);
+ }
}
static irqreturn_t xen_9pfs_front_event_handler(int irq, void *r)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 6/7] xen/9pfs: receive responses
2017-03-06 20:01 ` [PATCH 6/7] xen/9pfs: receive responses Stefano Stabellini
@ 2017-03-07 15:49 ` Boris Ostrovsky
2017-03-08 1:06 ` Stefano Stabellini
0 siblings, 1 reply; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-07 15:49 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich
On 03/06/2017 03:01 PM, Stefano Stabellini wrote:
> Upon receiving a notification from the backend, schedule the
> p9_xen_response work_struct. p9_xen_response checks if any responses are
> available, if so, it reads them one by one, calling p9_client_cb to send
> them up to the 9p layer (p9_client_cb completes the request). Handle the
> ring following the Xen 9pfs specification.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
> CC: Eric Van Hensbergen <ericvh@gmail.com>
> CC: Ron Minnich <rminnich@sandia.gov>
> CC: Latchesar Ionkov <lucho@ionkov.net>
> CC: v9fs-developer@lists.sourceforge.net
> ---
> net/9p/trans_xen.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index 4e26556..1ca9246 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -149,6 +149,59 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
>
> static void p9_xen_response(struct work_struct *work)
> {
> + struct xen_9pfs_front_priv *priv;
> + struct xen_9pfs_dataring *ring;
> + RING_IDX cons, prod, masked_cons, masked_prod;
> + struct xen_9pfs_header h;
> + struct p9_req_t *req;
> + int status = REQ_STATUS_ERROR;
Doesn't this need to go inside the loop?
> +
> + ring = container_of(work, struct xen_9pfs_dataring, work);
> + priv = ring->priv;
> +
> + while (1) {
> + cons = ring->intf->in_cons;
> + prod = ring->intf->in_prod;
> + rmb();
Is this rmb() or mb()? (Or, in fact, virt_XXX()?) You used mb() in the
previous patch.
> +
> + if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < sizeof(h)) {
> + notify_remote_via_irq(ring->irq);
> + return;
> + }
> +
> + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> +
> + xen_9pfs_read_packet(ring->ring.in,
> + masked_prod, &masked_cons,
> + XEN_9PFS_RING_SIZE, &h, sizeof(h));
> +
> + req = p9_tag_lookup(priv->client, h.tag);
> + if (!req || req->status != REQ_STATUS_SENT) {
> + dev_warn(&priv->dev->dev, "Wrong req tag=%x\n", h.tag);
> + cons += h.size;
> + mb();
> + ring->intf->in_cons = cons;
> + continue;
I don't know what xen_9pfs_read_packet() does so perhaps it's done there
but shouldn't the pointers be updated regardless of the 'if' condition?
-boris
> + }
> +
> + memcpy(req->rc, &h, sizeof(h));
> + req->rc->offset = 0;
> +
> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> + xen_9pfs_read_packet(ring->ring.in,
> + masked_prod, &masked_cons,
> + XEN_9PFS_RING_SIZE, req->rc->sdata, h.size);
> +
> + mb();
> + cons += h.size;
> + ring->intf->in_cons = cons;
> +
> + if (req->status != REQ_STATUS_ERROR)
> + status = REQ_STATUS_RCVD;
> +
> + p9_client_cb(priv->client, req, status);
> + }
> }
>
> static irqreturn_t xen_9pfs_front_event_handler(int irq, void *r)
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/7] xen/9pfs: receive responses
2017-03-07 15:49 ` Boris Ostrovsky
@ 2017-03-08 1:06 ` Stefano Stabellini
2017-03-08 1:13 ` Stefano Stabellini
2017-03-08 14:33 ` Boris Ostrovsky
0 siblings, 2 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 1:06 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
xen-devel
On Tue, 7 Mar 2017, Boris Ostrovsky wrote:
> On 03/06/2017 03:01 PM, Stefano Stabellini wrote:
> > Upon receiving a notification from the backend, schedule the
> > p9_xen_response work_struct. p9_xen_response checks if any responses are
> > available, if so, it reads them one by one, calling p9_client_cb to send
> > them up to the 9p layer (p9_client_cb completes the request). Handle the
> > ring following the Xen 9pfs specification.
> >
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> > CC: Eric Van Hensbergen <ericvh@gmail.com>
> > CC: Ron Minnich <rminnich@sandia.gov>
> > CC: Latchesar Ionkov <lucho@ionkov.net>
> > CC: v9fs-developer@lists.sourceforge.net
> > ---
> > net/9p/trans_xen.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 53 insertions(+)
> >
> > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> > index 4e26556..1ca9246 100644
> > --- a/net/9p/trans_xen.c
> > +++ b/net/9p/trans_xen.c
> > @@ -149,6 +149,59 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
> >
> > static void p9_xen_response(struct work_struct *work)
> > {
> > + struct xen_9pfs_front_priv *priv;
> > + struct xen_9pfs_dataring *ring;
> > + RING_IDX cons, prod, masked_cons, masked_prod;
> > + struct xen_9pfs_header h;
> > + struct p9_req_t *req;
> > + int status = REQ_STATUS_ERROR;
>
>
> Doesn't this need to go inside the loop?
Yes, thank you!
> > +
> > + ring = container_of(work, struct xen_9pfs_dataring, work);
> > + priv = ring->priv;
> > +
> > + while (1) {
> > + cons = ring->intf->in_cons;
> > + prod = ring->intf->in_prod;
> > + rmb();
>
>
> Is this rmb() or mb()? (Or, in fact, virt_XXX()?) You used mb() in the
> previous patch.
I think they should all be virt_XXX, thanks.
> > +
> > + if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < sizeof(h)) {
> > + notify_remote_via_irq(ring->irq);
> > + return;
> > + }
> > +
> > + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> > + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> > +
> > + xen_9pfs_read_packet(ring->ring.in,
> > + masked_prod, &masked_cons,
> > + XEN_9PFS_RING_SIZE, &h, sizeof(h));
> > +
> > + req = p9_tag_lookup(priv->client, h.tag);
> > + if (!req || req->status != REQ_STATUS_SENT) {
> > + dev_warn(&priv->dev->dev, "Wrong req tag=%x\n", h.tag);
> > + cons += h.size;
> > + mb();
> > + ring->intf->in_cons = cons;
> > + continue;
>
>
> I don't know what xen_9pfs_read_packet() does so perhaps it's done there
> but shouldn't the pointers be updated regardless of the 'if' condition?
This is the error path - the index is increased immediately. In the
non-error case, we do that right after the next read_packet call, few
lines below.
> > + }
> > +
> > + memcpy(req->rc, &h, sizeof(h));
> > + req->rc->offset = 0;
> > +
> > + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> > + xen_9pfs_read_packet(ring->ring.in,
> > + masked_prod, &masked_cons,
> > + XEN_9PFS_RING_SIZE, req->rc->sdata, h.size);
> > +
> > + mb();
> > + cons += h.size;
> > + ring->intf->in_cons = cons;
Here ^
> > + if (req->status != REQ_STATUS_ERROR)
> > + status = REQ_STATUS_RCVD;
> > +
> > + p9_client_cb(priv->client, req, status);
> > + }
> > }
> >
> > static irqreturn_t xen_9pfs_front_event_handler(int irq, void *r)
> >
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/7] xen/9pfs: receive responses
2017-03-08 1:06 ` Stefano Stabellini
@ 2017-03-08 1:13 ` Stefano Stabellini
2017-03-08 20:11 ` Konrad Rzeszutek Wilk
2017-03-08 14:33 ` Boris Ostrovsky
1 sibling, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 1:13 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich, xen-devel,
Boris Ostrovsky
On Tue, 7 Mar 2017, Stefano Stabellini wrote:
> > > +
> > > + ring = container_of(work, struct xen_9pfs_dataring, work);
> > > + priv = ring->priv;
> > > +
> > > + while (1) {
> > > + cons = ring->intf->in_cons;
> > > + prod = ring->intf->in_prod;
> > > + rmb();
> >
> >
> > Is this rmb() or mb()? (Or, in fact, virt_XXX()?) You used mb() in the
> > previous patch.
>
> I think they should all be virt_XXX, thanks.
regarding mb() vs. rmb(), give a look at the workflow at the end of
docs/misc/9pfs.markdown, under "Ring Usage".
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/7] xen/9pfs: receive responses
2017-03-08 1:13 ` Stefano Stabellini
@ 2017-03-08 20:11 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 38+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-03-08 20:11 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, xen-devel, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, Ron Minnich, v9fs-developer,
Boris Ostrovsky
On Tue, Mar 07, 2017 at 05:13:59PM -0800, Stefano Stabellini wrote:
> On Tue, 7 Mar 2017, Stefano Stabellini wrote:
> > > > +
> > > > + ring = container_of(work, struct xen_9pfs_dataring, work);
> > > > + priv = ring->priv;
> > > > +
> > > > + while (1) {
> > > > + cons = ring->intf->in_cons;
> > > > + prod = ring->intf->in_prod;
> > > > + rmb();
> > >
> > >
> > > Is this rmb() or mb()? (Or, in fact, virt_XXX()?) You used mb() in the
> > > previous patch.
> >
> > I think they should all be virt_XXX, thanks.
>
> regarding mb() vs. rmb(), give a look at the workflow at the end of
> docs/misc/9pfs.markdown, under "Ring Usage".
That is not what Boris meant. He meant that you should use the
virt_ variants instead of the rmb() or wmb().
The reason that on UP kernels the rmb() and wmb() can be converted
to NOPs. While that is OK for a UP kernel it is not good in virtualization
as we need those barriers regardless of the flavor of the kernel.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 6/7] xen/9pfs: receive responses
2017-03-08 1:06 ` Stefano Stabellini
2017-03-08 1:13 ` Stefano Stabellini
@ 2017-03-08 14:33 ` Boris Ostrovsky
2017-03-08 19:26 ` Stefano Stabellini
1 sibling, 1 reply; 38+ messages in thread
From: Boris Ostrovsky @ 2017-03-08 14:33 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, Latchesar Ionkov, Eric Van Hensbergen, linux-kernel,
Stefano Stabellini, v9fs-developer, Ron Minnich, xen-devel
>
>>> +
>>> + if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < sizeof(h)) {
>>> + notify_remote_via_irq(ring->irq);
>>> + return;
>>> + }
>>> +
>>> + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
>>> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
>>> +
>>> + xen_9pfs_read_packet(ring->ring.in,
>>> + masked_prod, &masked_cons,
>>> + XEN_9PFS_RING_SIZE, &h, sizeof(h));
>>> +
>>> + req = p9_tag_lookup(priv->client, h.tag);
>>> + if (!req || req->status != REQ_STATUS_SENT) {
>>> + dev_warn(&priv->dev->dev, "Wrong req tag=%x\n", h.tag);
>>> + cons += h.size;
>>> + mb();
>>> + ring->intf->in_cons = cons;
>>> + continue;
>>
>> I don't know what xen_9pfs_read_packet() does so perhaps it's done there
>> but shouldn't the pointers be updated regardless of the 'if' condition?
> This is the error path - the index is increased immediately. In the
> non-error case, we do that right after the next read_packet call, few
> lines below.
>
>
>>> + }
>>> +
>>> + memcpy(req->rc, &h, sizeof(h));
>>> + req->rc->offset = 0;
>>> +
>>> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
>>> + xen_9pfs_read_packet(ring->ring.in,
>>> + masked_prod, &masked_cons,
>>> + XEN_9PFS_RING_SIZE, req->rc->sdata, h.size);
>>> +
>>> + mb();
>>> + cons += h.size;
>>> + ring->intf->in_cons = cons;
> Here ^
>
So the second read is reading again from the same pointer in the ring,
but this time it gets the whole packet, including the header. The first
read was just poking at the header. Right?
If that's correct, can you add a comment somewhere? (unless this is
obvious to everyone else but me.)
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/7] xen/9pfs: receive responses
2017-03-08 14:33 ` Boris Ostrovsky
@ 2017-03-08 19:26 ` Stefano Stabellini
0 siblings, 0 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 19:26 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: jgross, Latchesar Ionkov, Stefano Stabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
xen-devel
On Wed, 8 Mar 2017, Boris Ostrovsky wrote:
> >>> +
> >>> + if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) < sizeof(h)) {
> >>> + notify_remote_via_irq(ring->irq);
> >>> + return;
> >>> + }
> >>> +
> >>> + masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
> >>> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> >>> +
> >>> + xen_9pfs_read_packet(ring->ring.in,
> >>> + masked_prod, &masked_cons,
> >>> + XEN_9PFS_RING_SIZE, &h, sizeof(h));
> >>> +
> >>> + req = p9_tag_lookup(priv->client, h.tag);
> >>> + if (!req || req->status != REQ_STATUS_SENT) {
> >>> + dev_warn(&priv->dev->dev, "Wrong req tag=%x\n", h.tag);
> >>> + cons += h.size;
> >>> + mb();
> >>> + ring->intf->in_cons = cons;
> >>> + continue;
> >>
> >> I don't know what xen_9pfs_read_packet() does so perhaps it's done there
> >> but shouldn't the pointers be updated regardless of the 'if' condition?
> > This is the error path - the index is increased immediately. In the
> > non-error case, we do that right after the next read_packet call, few
> > lines below.
> >
> >
> >>> + }
> >>> +
> >>> + memcpy(req->rc, &h, sizeof(h));
> >>> + req->rc->offset = 0;
> >>> +
> >>> + masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
> >>> + xen_9pfs_read_packet(ring->ring.in,
> >>> + masked_prod, &masked_cons,
> >>> + XEN_9PFS_RING_SIZE, req->rc->sdata, h.size);
> >>> +
> >>> + mb();
> >>> + cons += h.size;
> >>> + ring->intf->in_cons = cons;
> > Here ^
> >
>
>
> So the second read is reading again from the same pointer in the ring,
> but this time it gets the whole packet, including the header. The first
> read was just poking at the header. Right?
That's right. First we read the header, to know how much data to read.
> If that's correct, can you add a comment somewhere? (unless this is
> obvious to everyone else but me.)
Sure.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 7/7] xen/9pfs: build 9pfs Xen transport driver
2017-03-06 20:01 ` [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini
` (4 preceding siblings ...)
2017-03-06 20:01 ` [PATCH 6/7] xen/9pfs: receive responses Stefano Stabellini
@ 2017-03-06 20:01 ` Stefano Stabellini
2017-03-07 17:15 ` [PATCH 1/7] xen: import new ring macros in ring.h Julien Grall
6 siblings, 0 replies; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-06 20:01 UTC (permalink / raw)
To: xen-devel
Cc: jgross, Latchesar Ionkov, sstabellini, Eric Van Hensbergen,
linux-kernel, Stefano Stabellini, v9fs-developer, Ron Minnich,
boris.ostrovsky
This patch adds a Kconfig option and Makefile support for building the
9pfs Xen driver.
Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: boris.ostrovsky@oracle.com
CC: jgross@suse.com
CC: Eric Van Hensbergen <ericvh@gmail.com>
CC: Ron Minnich <rminnich@sandia.gov>
CC: Latchesar Ionkov <lucho@ionkov.net>
CC: v9fs-developer@lists.sourceforge.net
---
net/9p/Kconfig | 8 ++++++++
net/9p/Makefile | 4 ++++
2 files changed, 12 insertions(+)
diff --git a/net/9p/Kconfig b/net/9p/Kconfig
index a75174a..5c5649b 100644
--- a/net/9p/Kconfig
+++ b/net/9p/Kconfig
@@ -22,6 +22,14 @@ config NET_9P_VIRTIO
This builds support for a transports between
guest partitions and a host partition.
+config NET_9P_XEN
+ depends on XEN
+ tristate "9P Xen Transport"
+ help
+ This builds support for a transport between
+ two Xen domains.
+
+
config NET_9P_RDMA
depends on INET && INFINIBAND && INFINIBAND_ADDR_TRANS
tristate "9P RDMA Transport (Experimental)"
diff --git a/net/9p/Makefile b/net/9p/Makefile
index a0874cc..697ea7c 100644
--- a/net/9p/Makefile
+++ b/net/9p/Makefile
@@ -1,4 +1,5 @@
obj-$(CONFIG_NET_9P) := 9pnet.o
+obj-$(CONFIG_NET_9P_XEN) += 9pnet_xen.o
obj-$(CONFIG_NET_9P_VIRTIO) += 9pnet_virtio.o
obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o
@@ -14,5 +15,8 @@ obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o
9pnet_virtio-objs := \
trans_virtio.o \
+9pnet_xen-objs := \
+ trans_xen.o \
+
9pnet_rdma-objs := \
trans_rdma.o \
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 1/7] xen: import new ring macros in ring.h
2017-03-06 20:01 ` [PATCH 1/7] xen: import new ring macros in ring.h Stefano Stabellini
` (5 preceding siblings ...)
2017-03-06 20:01 ` [PATCH 7/7] xen/9pfs: build 9pfs Xen transport driver Stefano Stabellini
@ 2017-03-07 17:15 ` Julien Grall
2017-03-08 0:12 ` Stefano Stabellini
6 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2017-03-07 17:15 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: jgross, Stefano Stabellini, boris.ostrovsky, linux-kernel
Hi Stefano,
On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
> Sync the ring.h file with upstream Xen, to introduce the new ring macros.
> They will be used by the Xen transport for 9pfs.
>
> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> CC: konrad.wilk@oracle.com
> CC: boris.ostrovsky@oracle.com
> CC: jgross@suse.com
>
> ---
> NB: The new macros have not been committed to Xen yet. Do not apply this
> patch until they do.
> ---
> ---
> include/xen/interface/io/ring.h | 131 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 131 insertions(+)
>
> diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> index 21f4fbd..e16aa92 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
[...]
> +#define XEN_FLEX_RING_SIZE(order) \
> + (1UL << (order + PAGE_SHIFT - 1))
This will need to be XEN_PAGE_SHIFT in order to works with 64K kernel.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/7] xen: import new ring macros in ring.h
2017-03-07 17:15 ` [PATCH 1/7] xen: import new ring macros in ring.h Julien Grall
@ 2017-03-08 0:12 ` Stefano Stabellini
2017-03-08 11:14 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2017-03-08 0:12 UTC (permalink / raw)
To: Julien Grall
Cc: jgross, Stefano Stabellini, linux-kernel, Stefano Stabellini,
xen-devel, boris.ostrovsky
On Tue, 7 Mar 2017, Julien Grall wrote:
> Hi Stefano,
>
> On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
> > Sync the ring.h file with upstream Xen, to introduce the new ring macros.
> > They will be used by the Xen transport for 9pfs.
> >
> > Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
> > CC: konrad.wilk@oracle.com
> > CC: boris.ostrovsky@oracle.com
> > CC: jgross@suse.com
> >
> > ---
> > NB: The new macros have not been committed to Xen yet. Do not apply this
> > patch until they do.
> > ---
> > ---
> > include/xen/interface/io/ring.h | 131
> > ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 131 insertions(+)
> >
> > diff --git a/include/xen/interface/io/ring.h
> > b/include/xen/interface/io/ring.h
> > index 21f4fbd..e16aa92 100644
> > --- a/include/xen/interface/io/ring.h
> > +++ b/include/xen/interface/io/ring.h
>
> [...]
>
> > +#define XEN_FLEX_RING_SIZE(order)
> > \
> > + (1UL << (order + PAGE_SHIFT - 1))
>
> This will need to be XEN_PAGE_SHIFT in order to works with 64K kernel.
Good point! I had it right at the beginning but I lost the change in one
of the updates from xen.git.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/7] xen: import new ring macros in ring.h
2017-03-08 0:12 ` Stefano Stabellini
@ 2017-03-08 11:14 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2017-03-08 11:14 UTC (permalink / raw)
To: Stefano Stabellini
Cc: jgross, xen-devel, Stefano Stabellini, linux-kernel,
boris.ostrovsky
Hi Stefano,
On 08/03/17 00:12, Stefano Stabellini wrote:
> On Tue, 7 Mar 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 03/06/2017 08:01 PM, Stefano Stabellini wrote:
>>> Sync the ring.h file with upstream Xen, to introduce the new ring macros.
>>> They will be used by the Xen transport for 9pfs.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
>>> CC: konrad.wilk@oracle.com
>>> CC: boris.ostrovsky@oracle.com
>>> CC: jgross@suse.com
>>>
>>> ---
>>> NB: The new macros have not been committed to Xen yet. Do not apply this
>>> patch until they do.
>>> ---
>>> ---
>>> include/xen/interface/io/ring.h | 131
>>> ++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 131 insertions(+)
>>>
>>> diff --git a/include/xen/interface/io/ring.h
>>> b/include/xen/interface/io/ring.h
>>> index 21f4fbd..e16aa92 100644
>>> --- a/include/xen/interface/io/ring.h
>>> +++ b/include/xen/interface/io/ring.h
>>
>> [...]
>>
>>> +#define XEN_FLEX_RING_SIZE(order)
>>> \
>>> + (1UL << (order + PAGE_SHIFT - 1))
>>
>> This will need to be XEN_PAGE_SHIFT in order to works with 64K kernel.
>
> Good point! I had it right at the beginning but I lost the change in one
> of the updates from xen.git.
It is probably worth to consider introducing XEN_PAGE_SIZE in the
hypervisor code because we are likely to miss the change when the
headers will be re-sync in the future.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread