Re: [Xen-devel] [PATCH v2 02/13] xen/pvcalls: connect to the backend

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 26 Jul 2017, Boris Ostrovsky wrote:
> On 7/25/2017 5:21 PM, 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 <stefano@xxxxxxxxxxx>
> > CC: boris.ostrovsky@xxxxxxxxxx
> > CC: jgross@xxxxxxxx
> > ---
> >   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 a8d38c2..5e0b265 100644
> > --- a/drivers/xen/pvcalls-front.c
> > +++ b/drivers/xen/pvcalls-front.c
> > @@ -20,6 +20,29 @@
> >   #include <xen/xenbus.h>
> >   #include <xen/interface/io/pvcalls.h>
> >   +#define PVCALLS_INVALID_ID (UINT_MAX)
> 
> Unnecessary parentheses

OK


> > +#define RING_ORDER XENBUS_MAX_RING_GRANT_ORDER
> 
> PVCALLS_RING_ORDER?

Sure


> > +#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

good point, I'll fix


> > +
> > +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,12 +56,142 @@ 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);
> 
> Is ref really needed?

No, I'll remove it


> > +   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;
> > +   ret = xenbus_printf(xbt, dev->nodename, "ring-ref", "%d", ref);
> > +   if (ret)
> > +           goto error_xenbus;
> > +   ret = xenbus_printf(xbt, dev->nodename, "port", "%u",
> > +                       evtchn);
> > +   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;
> > +   }
> > +
> > +   INIT_LIST_HEAD(&bedata->socket_mappings);
> > +   INIT_LIST_HEAD(&bedata->socketpass_mappings);
> > +   spin_lock_init(&bedata->pvcallss_lock);
> > +   dev_set_drvdata(&dev->dev, bedata);
> > +   pvcalls_front_dev = dev;
> > +   xenbus_switch_state(dev, XenbusStateInitialised);
> > +
> >     return 0;
> > +
> > + error_xenbus:
> > +   xenbus_transaction_end(xbt, 1);
> > +   xenbus_dev_fatal(dev, ret, "writing xenstore");
> > + error:
> > +   pvcalls_front_remove(dev);
> 
> I think patch 12 (where you implement cleanup) could be moved before this one.

I'll move the patch


> I also think you are leaking bedata on error paths.

bedata is freed by pvcalls_front_remove (kfree(bedata)), why do you say
so?


> > +   return ret;
> >   }
> >     static void pvcalls_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 pvcalls_front_driver = {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel