Re: [Xen-devel] [PATCH v2 4/5] ALSA: xen-front: Implement handling of shared buffers

[Juergen Gross <jgross@xxxxxxxx>]

On 16/04/18 08:24, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Implement shared buffer handling according to the
> para-virtualized sound device protocol at xen/interface/io/sndif.h:
>   - manage buffer memory
>   - handle granted references
>   - handle page directories
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
>  sound/xen/Makefile              |   3 +-
>  sound/xen/xen_snd_front.c       |   8 ++
>  sound/xen/xen_snd_front_shbuf.c | 193 
> ++++++++++++++++++++++++++++++++++++++++
>  sound/xen/xen_snd_front_shbuf.h |  36 ++++++++
>  4 files changed, 239 insertions(+), 1 deletion(-)
>  create mode 100644 sound/xen/xen_snd_front_shbuf.c
>  create mode 100644 sound/xen/xen_snd_front_shbuf.h
> 
> diff --git a/sound/xen/Makefile b/sound/xen/Makefile
> index 03c669984000..f028bc30af5d 100644
> --- a/sound/xen/Makefile
> +++ b/sound/xen/Makefile
> @@ -2,6 +2,7 @@
>  
>  snd_xen_front-objs := xen_snd_front.o \
>                     xen_snd_front_cfg.o \
> -                   xen_snd_front_evtchnl.o
> +                   xen_snd_front_evtchnl.o \
> +                   xen_snd_front_shbuf.o
>  
>  obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o
> diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c
> index eb46bf4070f9..0569c6c596a3 100644
> --- a/sound/xen/xen_snd_front.c
> +++ b/sound/xen/xen_snd_front.c
> @@ -11,6 +11,7 @@
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  
> +#include <xen/page.h>
>  #include <xen/platform_pci.h>
>  #include <xen/xen.h>
>  #include <xen/xenbus.h>
> @@ -186,6 +187,13 @@ static struct xenbus_driver xen_driver = {
>  
>  static int __init xen_drv_init(void)
>  {
> +     /* At the moment we only support case with XEN_PAGE_SIZE == PAGE_SIZE */
> +     if (XEN_PAGE_SIZE != PAGE_SIZE) {
> +             pr_err(XENSND_DRIVER_NAME ": different kernel and Xen page 
> sizes are not supported: XEN_PAGE_SIZE (%lu) != PAGE_SIZE (%lu)\n",
> +                    XEN_PAGE_SIZE, PAGE_SIZE);
> +             return -ENODEV;
> +     }

Do you really want to print that error message on bare metal?

> +
>       if (!xen_domain())
>               return -ENODEV;
>  
> diff --git a/sound/xen/xen_snd_front_shbuf.c b/sound/xen/xen_snd_front_shbuf.c
> new file mode 100644
> index 000000000000..6845dbc7fdf5
> --- /dev/null
> +++ b/sound/xen/xen_snd_front_shbuf.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +
> +/*
> + * Xen para-virtual sound device
> + *
> + * Copyright (C) 2016-2018 EPAM Systems Inc.
> + *
> + * Author: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> + */
> +
> +#include <xen/xen.h>
> +#include <xen/xenbus.h>
> +
> +#include "xen_snd_front_shbuf.h"
> +
> +grant_ref_t xen_snd_front_shbuf_get_dir_start(struct xen_snd_front_shbuf 
> *buf)
> +{
> +     if (!buf->grefs)
> +             return GRANT_INVALID_REF;
> +
> +     return buf->grefs[0];
> +}
> +
> +void xen_snd_front_shbuf_clear(struct xen_snd_front_shbuf *buf)
> +{
> +     memset(buf, 0, sizeof(*buf));
> +}
> +
> +void xen_snd_front_shbuf_free(struct xen_snd_front_shbuf *buf)
> +{
> +     int i;
> +
> +     if (buf->grefs) {
> +             for (i = 0; i < buf->num_grefs; i++)
> +                     if (buf->grefs[i] != GRANT_INVALID_REF)
> +                             gnttab_end_foreign_access(buf->grefs[i],
> +                                                       0, 0UL);
> +             kfree(buf->grefs);
> +     }
> +     kfree(buf->directory);
> +     free_pages_exact(buf->buffer, buf->buffer_sz);
> +     xen_snd_front_shbuf_clear(buf);
> +}
> +
> +/*
> + * number of grant references a page can hold with respect to the
> + * xensnd_page_directory header
> + */
> +#define XENSND_NUM_GREFS_PER_PAGE ((XEN_PAGE_SIZE - \
> +             offsetof(struct xensnd_page_directory, gref)) / \
> +             sizeof(grant_ref_t))
> +
> +static void fill_page_dir(struct xen_snd_front_shbuf *buf,
> +                       int num_pages_dir)
> +{
> +     struct xensnd_page_directory *page_dir;
> +     unsigned char *ptr;
> +     int i, cur_gref, grefs_left, to_copy;
> +
> +     ptr = buf->directory;
> +     grefs_left = buf->num_grefs - num_pages_dir;
> +     /*
> +      * skip grant references at the beginning, they are for pages granted
> +      * for the page directory itself
> +      */
> +     cur_gref = num_pages_dir;
> +     for (i = 0; i < num_pages_dir; i++) {
> +             page_dir = (struct xensnd_page_directory *)ptr;
> +             if (grefs_left <= XENSND_NUM_GREFS_PER_PAGE) {
> +                     to_copy = grefs_left;
> +                     page_dir->gref_dir_next_page = GRANT_INVALID_REF;
> +             } else {
> +                     to_copy = XENSND_NUM_GREFS_PER_PAGE;
> +                     page_dir->gref_dir_next_page = buf->grefs[i + 1];
> +             }
> +
> +             memcpy(&page_dir->gref, &buf->grefs[cur_gref],
> +                    to_copy * sizeof(grant_ref_t));
> +
> +             ptr += XEN_PAGE_SIZE;
> +             grefs_left -= to_copy;
> +             cur_gref += to_copy;
> +     }
> +}
> +
> +static int grant_references(struct xenbus_device *xb_dev,
> +                         struct xen_snd_front_shbuf *buf,
> +                         int num_pages_dir, int num_pages_buffer,
> +                         int num_grefs)
> +{
> +     grant_ref_t priv_gref_head;
> +     unsigned long frame;
> +     int ret, i, j, cur_ref;
> +     int otherend_id;
> +
> +     ret = gnttab_alloc_grant_references(num_grefs, &priv_gref_head);
> +     if (ret)
> +             return ret;
> +
> +     buf->num_grefs = num_grefs;
> +     otherend_id = xb_dev->otherend_id;
> +     j = 0;
> +
> +     for (i = 0; i < num_pages_dir; i++) {
> +             cur_ref = gnttab_claim_grant_reference(&priv_gref_head);
> +             if (cur_ref < 0) {
> +                     ret = cur_ref;
> +                     goto fail;
> +             }
> +
> +             frame = xen_page_to_gfn(virt_to_page(buf->directory +
> +                                                  XEN_PAGE_SIZE * i));
> +             gnttab_grant_foreign_access_ref(cur_ref, otherend_id, frame, 0);
> +             buf->grefs[j++] = cur_ref;
> +     }
> +
> +     for (i = 0; i < num_pages_buffer; i++) {
> +             cur_ref = gnttab_claim_grant_reference(&priv_gref_head);
> +             if (cur_ref < 0) {
> +                     ret = cur_ref;
> +                     goto fail;
> +             }
> +
> +             frame = xen_page_to_gfn(virt_to_page(buf->buffer +
> +                                                  XEN_PAGE_SIZE * i));
> +             gnttab_grant_foreign_access_ref(cur_ref, otherend_id, frame, 0);
> +             buf->grefs[j++] = cur_ref;
> +     }
> +
> +     gnttab_free_grant_references(priv_gref_head);
> +     fill_page_dir(buf, num_pages_dir);
> +     return 0;
> +
> +fail:
> +     gnttab_free_grant_references(priv_gref_head);
> +     return ret;
> +}
> +
> +static int alloc_int_buffers(struct xen_snd_front_shbuf *buf,
> +                          int num_pages_dir, int num_pages_buffer,
> +                          int num_grefs)
> +{
> +     buf->grefs = kcalloc(num_grefs, sizeof(*buf->grefs), GFP_KERNEL);
> +     if (!buf->grefs)
> +             return -ENOMEM;
> +
> +     buf->directory = kcalloc(num_pages_dir, XEN_PAGE_SIZE, GFP_KERNEL);
> +     if (!buf->directory)
> +             goto fail;
> +
> +     buf->buffer_sz = num_pages_buffer * XEN_PAGE_SIZE;
> +     buf->buffer = alloc_pages_exact(buf->buffer_sz, GFP_KERNEL);
> +     if (!buf->buffer)
> +             goto fail;
> +
> +     return 0;
> +
> +fail:
> +     kfree(buf->grefs);
> +     buf->grefs = NULL;
> +     kfree(buf->directory);

Why do you need to free those here? Shouldn't that be done via
xen_snd_front_shbuf_free() in case of an error?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel