[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4] xenbus_client: extend interface to suppurt multi-page ring



On Sat, Nov 9, 2013 at 4:30 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> @@ -357,17 +365,39 @@ static void xenbus_switch_fatal(struct xenbus_device 
> *dev, int depth, int err,
>  /**
>   * xenbus_grant_ring
>   * @dev: xenbus device
> - * @ring_mfn: mfn of ring to grant
> -
> - * Grant access to the given @ring_mfn to the peer of the given device.  
> Return
> - * 0 on success, or -errno on error.  On error, the device will switch to
> + * @vaddr: starting virtual address of the ring
> + * @nr_pages: number of pages to be granted
> + * @grefs: grant reference array to be filled in
> + *
> + * Grant access to the given @vaddr to the peer of the given device.
> + * Then fill in @grefs with grant references.  Return 0 on success, or
> + * -errno on error.  On error, the device will switch to
>   * XenbusStateClosing, and the error will be saved in the store.
>   */
> -int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn)
> +int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
> +                     unsigned int nr_pages, grant_ref_t *grefs)
>  {
> -       int err = gnttab_grant_foreign_access(dev->otherend_id, ring_mfn, 0);
> -       if (err < 0)
> -               xenbus_dev_fatal(dev, err, "granting access to ring page");
> +       int err;
> +       int i;
> +
> +       for (i = 0; i < nr_pages; i++) {
> +               unsigned long addr = (unsigned long)vaddr +
> +                       (PAGE_SIZE * i);
> +               err = gnttab_grant_foreign_access(dev->otherend_id,
> +                                                 virt_to_mfn(addr), 0);
> +               if (err < 0) {
> +                       xenbus_dev_fatal(dev, err,
> +                                        "granting access to ring page");
> +                       goto fail;
> +               }
> +               grefs[i] = err;
> +       }
> +
> +       return 0;
> +
> +fail:
> +       for (; i >= 0; i--)

There's an off-by-one here, causing gnttab_end_foreign_access_ref to
be called on the first uninitialized (at least in this function) gref.

> +               gnttab_end_foreign_access_ref(grefs[i], 0);
>         return err;
>  }
>  EXPORT_SYMBOL_GPL(xenbus_grant_ring);
> @@ -448,62 +478,131 @@ EXPORT_SYMBOL_GPL(xenbus_free_evtchn);
>  /**
>   * xenbus_map_ring_valloc
>   * @dev: xenbus device
> - * @gnt_ref: grant reference
> + * @gnt_refs: grant reference array
> + * @nr_grefs: number of grant references
>   * @vaddr: pointer to address to be filled out by mapping
>   *
> - * Based on Rusty Russell's skeleton driver's map_page.
> - * Map a page of memory into this domain from another domain's grant table.
> - * xenbus_map_ring_valloc allocates a page of virtual address space, maps the
> - * page to that address, and sets *vaddr to that address.
> - * Returns 0 on success, and GNTST_* (see 
> xen/include/interface/grant_table.h)
> - * or -ENOMEM on error. If an error is returned, device will switch to
> + * Map @nr_grefs pages of memory into this domain from another
> + * domain's grant table.  xenbus_map_ring_valloc allocates @nr_grefs
> + * pages of virtual address space, maps the pages to that address, and
> + * sets *vaddr to that address.  Returns 0 on success, and GNTST_*
> + * (see xen/include/interface/grant_table.h) or -ENOMEM / -EINVAL on
> + * error. If an error is returned, device will switch to
>   * XenbusStateClosing and the error message will be saved in XenStore.
>   */
> -int xenbus_map_ring_valloc(struct xenbus_device *dev, int gnt_ref, void 
> **vaddr)
> +int xenbus_map_ring_valloc(struct xenbus_device *dev, grant_ref_t *gnt_refs,
> +                          unsigned int nr_grefs, void **vaddr)
>  {
> -       return ring_ops->map(dev, gnt_ref, vaddr);
> +       return ring_ops->map(dev, gnt_refs, nr_grefs, vaddr);
>  }
>  EXPORT_SYMBOL_GPL(xenbus_map_ring_valloc);
>
> +/* N.B. sizeof(phys_addr_t) doesn't always equal to sizeof(unsigned
> + * long), e.g. 32-on-64.  Caller is responsible for preparing the
> + * right array to feed into this function */
> +static int __xenbus_map_ring(struct xenbus_device *dev,
> +                            grant_ref_t *gnt_refs,
> +                            unsigned int nr_grefs,
> +                            grant_handle_t *handles,
> +                            phys_addr_t *addrs,
> +                            unsigned int flags,
> +                            bool *leaked)
> +{
> +       struct gnttab_map_grant_ref map[XENBUS_MAX_RING_PAGES];
> +       struct gnttab_unmap_grant_ref unmap[XENBUS_MAX_RING_PAGES];
> +       int i, j;
> +       int err = GNTST_okay;
> +
> +       if (nr_grefs > XENBUS_MAX_RING_PAGES)
> +               return -EINVAL;
> +
> +       for (i = 0; i < nr_grefs; i++) {
> +               memset(&map[i], 0, sizeof(map[i]));
> +               gnttab_set_map_op(&map[i], addrs[i], flags, gnt_refs[i],
> +                                 dev->otherend_id);
> +               handles[i] = INVALID_GRANT_HANDLE;
> +       }
> +
> +       gnttab_batch_map(map, i);
> +
> +       for (i = 0; i < nr_grefs; i++) {
> +               if (map[i].status != GNTST_okay) {
> +                       err = map[i].status;
> +                       xenbus_dev_fatal(dev, map[i].status,
> +                                        "mapping in shared page %d from 
> domain %d",
> +                                        gnt_refs[i], dev->otherend_id);
> +                       goto fail;
> +               } else
> +                       handles[i] = map[i].handle;
> +       }
> +
> +       return GNTST_okay;
> +
> + fail:
> +       for (i = j = 0; i < nr_grefs; i++) {
> +               memset(&unmap[i], 0, sizeof(unmap[i]));

I think you mean unmap[j], not unmap[i] (both times)? If so, you might
as well only do this in the following if block, not out here
unconditionally.

> +               if (handles[i] != INVALID_GRANT_HANDLE) {
> +                       gnttab_set_unmap_op(&unmap[j], (phys_addr_t)addrs[i],
> +                                           GNTMAP_host_map, handles[i]);
> +                       j++;
> +               }
> +       }
> +
> +       if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, unmap, j))
> +               BUG();
> +
> +       *leaked = false;
> +       for (i = 0; i < j; i++) {
> +               if (unmap[i].status != GNTST_okay) {
> +                       *leaked = true;
> +                       break;
> +               }
> +       }
> +
> +       return err;
> +

Extra blank line?

- Matthew

> +}
> +

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.