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

[Roger Pau Monné <royger@xxxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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/include/xen-sys/FreeBSD/gntdev.h 
> b/tools/include/xen-sys/FreeBSD/gntdev.h
> new file mode 100644
> index 0000000..1d09c5d
> --- /dev/null
> +++ b/tools/include/xen-sys/FreeBSD/gntdev.h
> @@ -0,0 +1,118 @@
> +/*-
> + * Copyright (c) 2016 Akshay Jaggi <jaggi@xxxxxxxxxxx>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + *
> + * gntdev.h
> + * 
> + * Interface to /dev/xen/gntdev.
> + * 
> + */
> +
> +#ifndef __XEN_GNTDEV_H__
> +#define __XEN_GNTDEV_H__
> +
> +#include <sys/types.h>
> +
> +#define IOCTL_GNTDEV_SET_UNMAP_NOTIFY                                        
> \
> +     _IOW('E', 0, struct ioctl_gntdev_unmap_notify)
> +struct ioctl_gntdev_unmap_notify {
> +    /* IN parameters */
> +    uint64_t index;
> +    uint32_t action;
> +    uint32_t event_channel_port;
> +};
> +
> +#define UNMAP_NOTIFY_CLEAR_BYTE 0x1
> +#define UNMAP_NOTIFY_SEND_EVENT 0x2
> +
> +/*-------------------- Grant Creation IOCTLs  
> --------------------------------*/
> +
> +#define IOCTL_GNTDEV_ALLOC_GREF                                              
> \
> +     _IOWR('E', 1, struct ioctl_gntdev_alloc_gref)
> +struct ioctl_gntdev_alloc_gref {
> +    /* IN parameters */
> +    uint16_t domid;
> +    uint16_t flags;
> +    uint32_t count;
> +    /* OUT parameters */
> +    uint64_t index;
> +    /* Variable OUT parameter */
> +    uint32_t gref_ids[1];
> +};
> +
> +#define GNTDEV_ALLOC_FLAG_WRITABLE 1
> +
> +#define IOCTL_GNTDEV_DEALLOC_GREF                                    \
> +     _IOW('E', 2, struct ioctl_gntdev_dealloc_gref)
> +struct ioctl_gntdev_dealloc_gref {
> +    /* IN parameters */
> +    uint64_t index;
> +    uint32_t count;
> +};
> +
> +/*-------------------- Grant Accessing IOCTLs  
> -------------------------------*/
                                ^ I would say "Mapping" here instead.

> +
> +struct ioctl_gntdev_grant_ref {
> +    uint32_t domid;
> +    uint32_t ref;
> +};
> +
> +#define IOCTL_GNTDEV_MAP_GRANT_REF                                   \
> +     _IOWR('E', 3, struct ioctl_gntdev_map_grant_ref)
> +struct ioctl_gntdev_map_grant_ref {
> +    /* IN parameters */
> +    uint32_t count;
> +    uint32_t pad0;
> +    /* OUT parameters */
> +    uint64_t index;
> +    /* Variable IN parameter */
> +    struct ioctl_gntdev_grant_ref refs[1];
> +};
> +
> +#define IOCTL_GNTDEV_UNMAP_GRANT_REF                                 \
> +     _IOW('E', 4, struct ioctl_gntdev_unmap_grant_ref)
> +struct ioctl_gntdev_unmap_grant_ref {
> +    /* IN parameters */
> +    uint64_t index;
> +    uint32_t count;
> +};
> +
> +#define IOCTL_GNTDEV_GET_OFFSET_FOR_VADDR                            \
> +     _IOWR('E', 5, struct ioctl_gntdev_get_offset_for_vaddr)
> +struct ioctl_gntdev_get_offset_for_vaddr {
> +    /* IN parameters */
> +    uint64_t vaddr;
> +    /* OUT parameters */
> +    uint64_t offset;
> +    uint32_t count;
> +};
> +
> +#define IOCTL_GNTDEV_SET_MAX_GRANTS                                  \
> +     _IOW('E', 6, struct ioctl_gntdev_set_max_grants)
> +struct ioctl_gntdev_set_max_grants {
> +    /* IN parameters */
> +    uint32_t count;

This seems useless, I don't see any reason to introduce an IOCTL that is 
deprecated, and that will never be implemented in FreeBSD.

> +};
> +
> +#endif /* __XEN_GNTDEV_H__ */
> diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
> index af64542..69bb207 100644
> --- a/tools/libs/gnttab/Makefile
> +++ b/tools/libs/gnttab/Makefile
> @@ -14,7 +14,7 @@ SRCS-GNTSHR            += gntshr_core.c
>  
>  SRCS-$(CONFIG_Linux)   += $(SRCS-GNTTAB) $(SRCS-GNTSHR) linux.c
>  SRCS-$(CONFIG_MiniOS)  += $(SRCS-GNTTAB) gntshr_unimp.c minios.c
> -SRCS-$(CONFIG_FreeBSD) += gnttab_unimp.c gntshr_unimp.c
> +SRCS-$(CONFIG_FreeBSD) += $(SRCS-GNTTAB) $(SRCS-GNTSHR) freebsd.c
>  SRCS-$(CONFIG_SunOS)   += gnttab_unimp.c gntshr_unimp.c
>  SRCS-$(CONFIG_NetBSD)  += gnttab_unimp.c gntshr_unimp.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@xxxxxxxxxxxx>
> + * Copyright (c) 2016-2017, Akshay Jaggi <jaggi@xxxxxxxxxxx>
> + *
> + * 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/"

This should be /dev/xen/gntdev, FreeBSD only has one gnt device, there's no 
reason to not have the full path here.

> +#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))
>
> +#ifndef O_CLOEXEC
> +#define O_CLOEXEC 0

No need for this, the version of FreeBSD that support Xen all have O_CLOEXEC 
defined.

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

This should just return 0, the ioctl is not implemented and will never be.

> +{
> +    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) {

Coding style, the bracket should be on a new line and you need spaces inside 
the conditional.

> +        /*
> +         * FreeBSD kernel doesn't implement this IOCTL,
> +         * so ignore the resulting specific failure, if any.
> +         */
> +        if (errno == ENOTTY)

Same here, missing spaces inside of the conditional (I'm not going to 
comment on all of those).

> +            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));
> +    else
> +    {
> +        map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
> +                   MAP_PRIVATE | MAP_ANON, -1, 0);
> +        if ( map == MAP_FAILED )
> +        {
> +            GTERROR(xgt->logger, "mmap of map failed");
> +            return NULL;
> +        }
> +    }
> +
> +    for ( i = 0; i < count; i++ )
> +    {
> +        map->refs[i].domid = domids[i * domids_stride];
> +        map->refs[i].ref = refs[i];
> +    }
> +
> +    map->count = count;
> +
> +    if ( ioctl(fd, IOCTL_GNTDEV_MAP_GRANT_REF, map) ) {
> +        GTERROR(xgt->logger, "ioctl MAP_GRANT_REF failed");
> +        goto out;
> +    }
> +
> + retry:
> +    addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
> +                map->index);
> +
> +    if (addr == MAP_FAILED && errno == EAGAIN)
> +    {
> +        /*
> +         * The grant hypercall can return EAGAIN if the granted page
> +         * is swapped out. Since the paging daemon may be in the same
> +         * domain, the hypercall cannot block without causing a
> +         * deadlock.
> +         *
> +         * Because there are no notifications when the page is swapped
> +         * in, wait a bit before retrying, and hope that the page will
> +         * arrive eventually.
> +         */
> +        usleep(1000);
> +        goto retry;
> +    }
> +
> +    if (addr != MAP_FAILED)
> +    {
> +        int rv = 0;
> +        struct ioctl_gntdev_unmap_notify notify;

New line between variable declaration and code.

> +        notify.index = map->index;
> +        notify.action = 0;
> +        if (notify_offset < PAGE_SIZE * count) {
> +            notify.index += notify_offset;
> +            notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
> +        }
> +        if (notify_port != -1) {
> +            notify.event_channel_port = notify_port;
> +            notify.action |= UNMAP_NOTIFY_SEND_EVENT;
> +        }
> +        if (notify.action)
> +            rv = ioctl(fd, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, &notify);
> +        if (rv) {
> +            GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
> +            munmap(addr, count * PAGE_SIZE);
> +            addr = MAP_FAILED;
> +        }
> +    }
> +
> +    if (addr == MAP_FAILED)
> +    {
> +        int saved_errno = errno;
> +        struct ioctl_gntdev_unmap_grant_ref unmap_grant;
> +
> +        /* Unmap the driver slots used to store the grant information. */
> +        GTERROR(xgt->logger, "mmap failed");
> +        unmap_grant.index = map->index;
> +        unmap_grant.count = count;
> +        ioctl(fd, IOCTL_GNTDEV_UNMAP_GRANT_REF, &unmap_grant);
> +        errno = saved_errno;
> +        addr = NULL;
> +    }
> +
> + out:
> +    if ( map_size > PAGE_SIZE )
> +        munmap(map, map_size);
> +
> +    return addr;
> +}
> +
> +int osdep_gnttab_unmap(xengnttab_handle *xgt,
> +                       void *start_address,
> +                       uint32_t count)
> +{
> +    int fd = xgt->fd;
> +    struct ioctl_gntdev_get_offset_for_vaddr get_offset;
> +    struct ioctl_gntdev_unmap_grant_ref unmap_grant;
> +    int rc;
> +
> +    if ( start_address == NULL )
> +    {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    /* First, it is necessary to get the offset which was initially used to
> +     * mmap() the pages.
> +     */

Comment style, please leave the initial line empty in a multi line comment 
(like you have done above).

> +    get_offset.vaddr = (unsigned long)start_address;
> +    if ( (rc = ioctl(fd, IOCTL_GNTDEV_GET_OFFSET_FOR_VADDR,
> +                     &get_offset)) )
> +        return rc;
> +
> +    if ( get_offset.count != count )
> +    {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    /* Next, unmap the memory. */
> +    if ( (rc = munmap(start_address, count * PAGE_SIZE)) )
> +        return rc;
> +
> +    /* Finally, unmap the driver slots used to store the grant information. 
> */
> +    unmap_grant.index = get_offset.offset;
> +    unmap_grant.count = count;
> +    if ( (rc = ioctl(fd, IOCTL_GNTDEV_UNMAP_GRANT_REF, &unmap_grant)) )
> +        return rc;
> +
> +    return 0;
> +}
> +
> +int osdep_gntshr_open(xengntshr_handle *xgs)
> +{
> +    int fd = open(DEVXEN "gntdev", O_RDWR);

New line.

> +    if ( fd == -1 )
> +        return -1;
> +    xgs->fd = fd;

New line.

> +    return 0;
> +}
> +
> +int osdep_gntshr_close(xengntshr_handle *xgs)
> +{
> +    if ( xgs->fd == -1 )
> +        return 0;
> +
> +    return close(xgs->fd);
> +}
> +
> +void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
> +                               uint32_t domid, int count,
> +                               uint32_t *refs, int writable,
> +                               uint32_t notify_offset,
> +                               evtchn_port_t notify_port)
> +{
> +    struct ioctl_gntdev_alloc_gref *gref_info = NULL;
> +    struct ioctl_gntdev_unmap_notify notify;
> +    struct ioctl_gntdev_dealloc_gref gref_drop;
> +    int fd = xgs->fd;
> +    int err;
> +    void *area = NULL;
> +    gref_info = malloc(sizeof(*gref_info) + count * sizeof(uint32_t));

New line.

Roger.

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