Re: [PATCH] gnttab: Add gntdev device mappings for FreeBSD

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Aug 04, 2016 at 06:23:51PM +0530, Akshay Jaggi wrote:
> Add grant table userspace device mappings for
> FreeBSD (enables support for qdisk backend
> on FreeBSD Dom0).
> 
> Signed-off-by: Akshay Jaggi <akshay1994.leo@gmail.com>
> ---
>  tools/include/xen-sys/FreeBSD/gntdev.h | 118 ++++++++++++
>  tools/libs/gnttab/Makefile             |   2 +-
>  tools/libs/gnttab/freebsd.c            | 335 +++++++++++++++++++++++++++++++++
>  3 files changed, 454 insertions(+), 1 deletion(-)
>  create mode 100644 tools/include/xen-sys/FreeBSD/gntdev.h
>  create mode 100644 tools/libs/gnttab/freebsd.c
[...]
> diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
> new file mode 100644
> index 0000000..eef0238
> --- /dev/null
> +++ b/tools/libs/gnttab/freebsd.c
> @@ -0,0 +1,335 @@
> +/*
> + * Copyright (c) 2007-2008, D G Murray <Derek.Murray@cl.cam.ac.uk>
> + * Copyright (c) 2016-2017, Akshay Jaggi <jaggi@FreeBSD.org>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Split out from linux.c
> + */
> +
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +
> +#include <xen/sys/gntdev.h>
> +
> +#include "private.h"
> +
> +#define DEVXEN "/dev/xen/"
> +
> +#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
> +
> +#define GTERROR(_l, _f...) xtl_log(_l, XTL_ERROR, errno, "gnttab", _f)
> +#define GSERROR(_l, _f...) xtl_log(_l, XTL_ERROR, errno, "gntshr", _f)
> +
> +#define PAGE_SHIFT           12
> +#define PAGE_SIZE            (1UL << PAGE_SHIFT)
> +#define PAGE_MASK            (~(PAGE_SIZE-1))
> +

This is repetitive.  Please consider moving all the common bits to
private.h with appropriate ifdef's around the code -- presumably you
only want them for FreeBSD and Linux.

> +#ifndef O_CLOEXEC
> +#define O_CLOEXEC 0
> +#endif
> +
> +int osdep_gnttab_open(xengnttab_handle *xgt)
> +{
> +    int fd = open(DEVXEN "gntdev", O_RDWR|O_CLOEXEC);
> +    if ( fd == -1 )
> +        return -1;
> +    xgt->fd = fd;
> +    return 0;
> +}
> +
> +int osdep_gnttab_close(xengnttab_handle *xgt)
> +{
> +    if ( xgt->fd == -1 )
> +        return 0;
> +
> +    return close(xgt->fd);
> +}
> +
> +int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
> +{
> +    int fd = xgt->fd, rc;
> +    struct ioctl_gntdev_set_max_grants max_grants = { .count = count };
> +
> +    rc = ioctl(fd, IOCTL_GNTDEV_SET_MAX_GRANTS, &max_grants);
> +    if (rc) {
> +        /*
> +         * FreeBSD kernel doesn't implement this IOCTL,
> +         * so ignore the resulting specific failure, if any.
> +         */
> +        if (errno == ENOTTY)
> +            rc = 0;
> +        else
> +            GTERROR(xgt->logger, "ioctl SET_MAX_GRANTS failed");
> +    }
> +
> +    return rc;
> +}
> +
> +void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
> +                             uint32_t count, int flags, int prot,
> +                             uint32_t *domids, uint32_t *refs,
> +                             uint32_t notify_offset,
> +                             evtchn_port_t notify_port)
> +{
> +    int fd = xgt->fd;
> +    struct ioctl_gntdev_map_grant_ref *map;
> +    unsigned int map_size = ROUNDUP((sizeof(*map) + (count - 1) *
> +                                    sizeof(struct ioctl_gntdev_map_grant_ref)),
> +                                    PAGE_SHIFT);
> +    void *addr = NULL;
> +    int domids_stride = 1;
> +    int i;
> +
> +    if (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN)
> +        domids_stride = 0;
> +
> +    if ( map_size <= PAGE_SIZE )
> +        map = alloca(sizeof(*map) +
> +                     (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));

Please avoid using alloca. Yes, I know you copy this from linux.c, but I
don't think this is a good idea because the error semantics is horrible
-- if stack overflows program behaviour is undefined (!).

Wei.

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