Re: [PATCH v1 02/13] xen/pvcalls: connect to the backend
On Mon, 24 Jul 2017, Juergen Gross wrote:
> On 22/07/17 02:11, Stefano Stabellini wrote:
> > Implement the probe function for the pvcalls frontend. Read the
> > supported versions, max-page-order and function-calls nodes from
> > xenstore.
> >
> > Introduce a data structure named pvcalls_bedata. It contains pointers to
> > the command ring, the event channel, a list of active sockets and a list
> > of passive sockets. Lists accesses are protected by a spin_lock.
> >
> > Introduce a waitqueue to allow waiting for a response on commands sent
> > to the backend.
> >
> > Introduce an array of struct xen_pvcalls_response to store commands
> > responses.
> >
> > Only one frontend<->backend connection is supported at any given time
> > for a guest. Store the active frontend device to a static pointer.
> >
> > Introduce a stub functions for the event handler.
> >
> > Signed-off-by: Stefano Stabellini
> > CC: boris.ostrov...@oracle.com
> > CC: jgr...@suse.com
> > ---
> > drivers/xen/pvcalls-front.c | 153
> >
> > 1 file changed, 153 insertions(+)
> >
> > diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> > index 173e204..fb08ebf 100644
> > --- a/drivers/xen/pvcalls-front.c
> > +++ b/drivers/xen/pvcalls-front.c
> > @@ -20,6 +20,29 @@
> > #include
> > #include
> >
> > +#define PVCALLS_INVALID_ID (UINT_MAX)
> > +#define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
> > +#define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls,
> > XEN_PAGE_SIZE)
> > +
> > +struct pvcalls_bedata {
> > + struct xen_pvcalls_front_ring ring;
> > + grant_ref_t ref;
> > + int irq;
> > +
> > + struct list_head socket_mappings;
> > + struct list_head socketpass_mappings;
> > + spinlock_t pvcallss_lock;
> > +
> > + wait_queue_head_t inflight_req;
> > + struct xen_pvcalls_response rsp[PVCALLS_NR_REQ_PER_RING];
> > +};
> > +struct xenbus_device *pvcalls_front_dev;
> > +
> > +static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
> > +{
> > + return IRQ_HANDLED;
> > +}
> > +
> > static const struct xenbus_device_id pvcalls_front_ids[] = {
> > { "pvcalls" },
> > { "" }
> > @@ -33,7 +56,114 @@ static int pvcalls_front_remove(struct xenbus_device
> > *dev)
> > static int pvcalls_front_probe(struct xenbus_device *dev,
> > const struct xenbus_device_id *id)
> > {
> > + int ret = -EFAULT, evtchn, ref = -1, i;
> > + unsigned int max_page_order, function_calls, len;
> > + char *versions;
> > + grant_ref_t gref_head = 0;
> > + struct xenbus_transaction xbt;
> > + struct pvcalls_bedata *bedata = NULL;
> > + struct xen_pvcalls_sring *sring;
> > +
> > + if (pvcalls_front_dev != NULL) {
> > + dev_err(&dev->dev, "only one PV Calls connection supported\n");
> > + return -EINVAL;
> > + }
> > +
> > + versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
> > + if (!len)
> > + return -EINVAL;
> > + if (strcmp(versions, "1")) {
> > + kfree(versions);
> > + return -EINVAL;
> > + }
> > + kfree(versions);
> > + ret = xenbus_scanf(XBT_NIL, dev->otherend,
> > + "max-page-order", "%u", &max_page_order);
>
> Use xenbus_read_unsigned() instead?
OK
> > + if (ret <= 0)
> > + return -ENODEV;
> > + if (max_page_order < RING_ORDER)
> > + return -ENODEV;
> > + ret = xenbus_scanf(XBT_NIL, dev->otherend,
> > + "function-calls", "%u", &function_calls);
>
> xenbus_read_unsigned() again?
OK
> > + if (ret <= 0 || function_calls != 1)
> > + return -ENODEV;
> > + pr_info("%s max-page-order is %u\n", __func__, max_page_order);
> > +
> > + bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
> > + if (!bedata)
> > + return -ENOMEM;
> > +
>
> You should call dev_set_drvdata() here already, otherwise entering the
> error path will dereference a NULL pointer instead of bedata.
OK
> > + init_waitqueue_head(&bedata->inflight_req);
> > + for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
> > + bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
> > +
> > + sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
> > +__GFP_ZERO);
> > + if (!sring)
> > + goto error;
>
> ret will be 1 here. Shouldn't you set it to -ENOMEM?
Yes, I'll do that
>
> > + SHARED_RING_INIT(sring);
> > + FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
> > +
> > + ret = xenbus_alloc_evtchn(dev, &evtchn);
> > + if (ret)
> > + goto error;
> > +
> > + bedata->irq = bind_evtchn_to_irqhandler(evtchn,
> > + pvcalls_front_event_handler,
> > + 0, "pvcalls-frontend", dev);
> > + if (bedata->irq < 0) {
> > + ret = bedata->irq;
> > + goto error;
> > + }
> > +
> > + re
Re: [PATCH v1 02/13] xen/pvcalls: connect to the backend
On 22/07/17 02:11, Stefano Stabellini wrote:
> Implement the probe function for the pvcalls frontend. Read the
> supported versions, max-page-order and function-calls nodes from
> xenstore.
>
> Introduce a data structure named pvcalls_bedata. It contains pointers to
> the command ring, the event channel, a list of active sockets and a list
> of passive sockets. Lists accesses are protected by a spin_lock.
>
> Introduce a waitqueue to allow waiting for a response on commands sent
> to the backend.
>
> Introduce an array of struct xen_pvcalls_response to store commands
> responses.
>
> Only one frontend<->backend connection is supported at any given time
> for a guest. Store the active frontend device to a static pointer.
>
> Introduce a stub functions for the event handler.
>
> Signed-off-by: Stefano Stabellini
> CC: boris.ostrov...@oracle.com
> CC: jgr...@suse.com
> ---
> drivers/xen/pvcalls-front.c | 153
>
> 1 file changed, 153 insertions(+)
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index 173e204..fb08ebf 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -20,6 +20,29 @@
> #include
> #include
>
> +#define PVCALLS_INVALID_ID (UINT_MAX)
> +#define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
> +#define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls, XEN_PAGE_SIZE)
> +
> +struct pvcalls_bedata {
> + struct xen_pvcalls_front_ring ring;
> + grant_ref_t ref;
> + int irq;
> +
> + struct list_head socket_mappings;
> + struct list_head socketpass_mappings;
> + spinlock_t pvcallss_lock;
> +
> + wait_queue_head_t inflight_req;
> + struct xen_pvcalls_response rsp[PVCALLS_NR_REQ_PER_RING];
> +};
> +struct xenbus_device *pvcalls_front_dev;
> +
> +static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
> +{
> + return IRQ_HANDLED;
> +}
> +
> static const struct xenbus_device_id pvcalls_front_ids[] = {
> { "pvcalls" },
> { "" }
> @@ -33,7 +56,114 @@ static int pvcalls_front_remove(struct xenbus_device *dev)
> static int pvcalls_front_probe(struct xenbus_device *dev,
> const struct xenbus_device_id *id)
> {
> + int ret = -EFAULT, evtchn, ref = -1, i;
> + unsigned int max_page_order, function_calls, len;
> + char *versions;
> + grant_ref_t gref_head = 0;
> + struct xenbus_transaction xbt;
> + struct pvcalls_bedata *bedata = NULL;
> + struct xen_pvcalls_sring *sring;
> +
> + if (pvcalls_front_dev != NULL) {
> + dev_err(&dev->dev, "only one PV Calls connection supported\n");
> + return -EINVAL;
> + }
> +
> + versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
> + if (!len)
> + return -EINVAL;
> + if (strcmp(versions, "1")) {
> + kfree(versions);
> + return -EINVAL;
> + }
> + kfree(versions);
> + ret = xenbus_scanf(XBT_NIL, dev->otherend,
> +"max-page-order", "%u", &max_page_order);
Use xenbus_read_unsigned() instead?
> + if (ret <= 0)
> + return -ENODEV;
> + if (max_page_order < RING_ORDER)
> + return -ENODEV;
> + ret = xenbus_scanf(XBT_NIL, dev->otherend,
> +"function-calls", "%u", &function_calls);
xenbus_read_unsigned() again?
> + if (ret <= 0 || function_calls != 1)
> + return -ENODEV;
> + pr_info("%s max-page-order is %u\n", __func__, max_page_order);
> +
> + bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
> + if (!bedata)
> + return -ENOMEM;
> +
You should call dev_set_drvdata() here already, otherwise entering the
error path will dereference a NULL pointer instead of bedata.
> + init_waitqueue_head(&bedata->inflight_req);
> + for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
> + bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
> +
> + sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
> + __GFP_ZERO);
> + if (!sring)
> + goto error;
ret will be 1 here. Shouldn't you set it to -ENOMEM?
> + SHARED_RING_INIT(sring);
> + FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
> +
> + ret = xenbus_alloc_evtchn(dev, &evtchn);
> + if (ret)
> + goto error;
> +
> + bedata->irq = bind_evtchn_to_irqhandler(evtchn,
> + pvcalls_front_event_handler,
> + 0, "pvcalls-frontend", dev);
> + if (bedata->irq < 0) {
> + ret = bedata->irq;
> + goto error;
> + }
> +
> + ret = gnttab_alloc_grant_references(1, &gref_head);
> + if (ret < 0)
> + goto error;
> + bedata->ref = ref = gnttab_claim_grant_reference(&gref_head);
> + if (ref < 0)
> + goto error;
Setting ret?
[PATCH v1 02/13] xen/pvcalls: connect to the backend
Implement the probe function for the pvcalls frontend. Read the
supported versions, max-page-order and function-calls nodes from
xenstore.
Introduce a data structure named pvcalls_bedata. It contains pointers to
the command ring, the event channel, a list of active sockets and a list
of passive sockets. Lists accesses are protected by a spin_lock.
Introduce a waitqueue to allow waiting for a response on commands sent
to the backend.
Introduce an array of struct xen_pvcalls_response to store commands
responses.
Only one frontend<->backend connection is supported at any given time
for a guest. Store the active frontend device to a static pointer.
Introduce a stub functions for the event handler.
Signed-off-by: Stefano Stabellini
CC: boris.ostrov...@oracle.com
CC: jgr...@suse.com
---
drivers/xen/pvcalls-front.c | 153
1 file changed, 153 insertions(+)
diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 173e204..fb08ebf 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -20,6 +20,29 @@
#include
#include
+#define PVCALLS_INVALID_ID (UINT_MAX)
+#define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
+#define PVCALLS_NR_REQ_PER_RING __CONST_RING_SIZE(xen_pvcalls, XEN_PAGE_SIZE)
+
+struct pvcalls_bedata {
+ struct xen_pvcalls_front_ring ring;
+ grant_ref_t ref;
+ int irq;
+
+ struct list_head socket_mappings;
+ struct list_head socketpass_mappings;
+ spinlock_t pvcallss_lock;
+
+ wait_queue_head_t inflight_req;
+ struct xen_pvcalls_response rsp[PVCALLS_NR_REQ_PER_RING];
+};
+struct xenbus_device *pvcalls_front_dev;
+
+static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
+{
+ return IRQ_HANDLED;
+}
+
static const struct xenbus_device_id pvcalls_front_ids[] = {
{ "pvcalls" },
{ "" }
@@ -33,7 +56,114 @@ static int pvcalls_front_remove(struct xenbus_device *dev)
static int pvcalls_front_probe(struct xenbus_device *dev,
const struct xenbus_device_id *id)
{
+ int ret = -EFAULT, evtchn, ref = -1, i;
+ unsigned int max_page_order, function_calls, len;
+ char *versions;
+ grant_ref_t gref_head = 0;
+ struct xenbus_transaction xbt;
+ struct pvcalls_bedata *bedata = NULL;
+ struct xen_pvcalls_sring *sring;
+
+ if (pvcalls_front_dev != NULL) {
+ dev_err(&dev->dev, "only one PV Calls connection supported\n");
+ return -EINVAL;
+ }
+
+ versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
+ if (!len)
+ return -EINVAL;
+ if (strcmp(versions, "1")) {
+ kfree(versions);
+ return -EINVAL;
+ }
+ kfree(versions);
+ ret = xenbus_scanf(XBT_NIL, dev->otherend,
+ "max-page-order", "%u", &max_page_order);
+ if (ret <= 0)
+ return -ENODEV;
+ if (max_page_order < RING_ORDER)
+ return -ENODEV;
+ ret = xenbus_scanf(XBT_NIL, dev->otherend,
+ "function-calls", "%u", &function_calls);
+ if (ret <= 0 || function_calls != 1)
+ return -ENODEV;
+ pr_info("%s max-page-order is %u\n", __func__, max_page_order);
+
+ bedata = kzalloc(sizeof(struct pvcalls_bedata), GFP_KERNEL);
+ if (!bedata)
+ return -ENOMEM;
+
+ init_waitqueue_head(&bedata->inflight_req);
+ for (i = 0; i < PVCALLS_NR_REQ_PER_RING; i++)
+ bedata->rsp[i].req_id = PVCALLS_INVALID_ID;
+
+ sring = (struct xen_pvcalls_sring *) __get_free_page(GFP_KERNEL |
+__GFP_ZERO);
+ if (!sring)
+ goto error;
+ SHARED_RING_INIT(sring);
+ FRONT_RING_INIT(&bedata->ring, sring, XEN_PAGE_SIZE);
+
+ ret = xenbus_alloc_evtchn(dev, &evtchn);
+ if (ret)
+ goto error;
+
+ bedata->irq = bind_evtchn_to_irqhandler(evtchn,
+ pvcalls_front_event_handler,
+ 0, "pvcalls-frontend", dev);
+ if (bedata->irq < 0) {
+ ret = bedata->irq;
+ goto error;
+ }
+
+ ret = gnttab_alloc_grant_references(1, &gref_head);
+ if (ret < 0)
+ goto error;
+ bedata->ref = ref = gnttab_claim_grant_reference(&gref_head);
+ if (ref < 0)
+ goto error;
+ gnttab_grant_foreign_access_ref(ref, dev->otherend_id,
+ virt_to_gfn((void *)sring), 0);
+
+ 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;
+
