Re: [Xen-devel] [PATCH] libs/gnttab: add FreeBSD handlers for the grant-table user-space device

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jan 26, 2017 at 03:35:46PM +0000, Roger Pau Monne wrote:
> This patch adds the headers and helpers for the FreeBSD gntdev, used in order
> to map grants from remote domains and to allocate grants on behalf of the
> current domain.
> 
> Current code has been tested with the QEMU/Qdisk backend.
> 
> Signed-off-by: Akshay Jaggi <akshay1994.leo@xxxxxxxxx>
> [ added dummy stub for osdep_gnttab_grant_copy ]
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I have mostly got some style nitpicking.

> ---
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
>  tools/include/xen-sys/FreeBSD/gntdev.h | 191 +++++++++++++++++++
>  tools/libs/gnttab/Makefile             |   2 +-
>  tools/libs/gnttab/freebsd.c            | 327 
> +++++++++++++++++++++++++++++++++
>  3 files changed, 519 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/Makefile b/tools/libs/gnttab/Makefile
> index 95c2cd8..6a77f21 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..46a0130
> --- /dev/null
> +++ b/tools/libs/gnttab/freebsd.c
> @@ -0,0 +1,327 @@
> +/*
> + * 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/gntdev"
> +
> +#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))
> +

I think at some point I asked to move these to a common header. There is
no reason to duplicate them.

> +int osdep_gnttab_open(xengnttab_handle *xgt)
> +{
> +    int fd = open(DEVXEN, O_RDWR|O_CLOEXEC);
> +
> +    if ( fd == -1 )
> +        return -1;
> +    xgt->fd = fd;
> +
> +    return 0;
> +}
> +
> +int osdep_gnttab_close(xengnttab_handle *xgt)
> +{
> +

Could you please remove blank lines like this please. There are quite a
few instances in this patch.

> +    if ( xgt->fd == -1 )
> +        return 0;
> +
> +    return close(xgt->fd);
> +}
> +
> +int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
> +{
> +
> +    return 0;
> +}
> +
> +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)
> +{
> +    uint32_t i;
> +    int fd = xgt->fd;
> +    struct ioctl_gntdev_map_grant_ref *map;
> +    void *addr = NULL;
> +    int domids_stride;
> +    unsigned int map_size = ROUNDUP((sizeof(*map) + (count - 1) *
> +                                    sizeof(struct 
> ioctl_gntdev_map_grant_ref)),
> +                                    PAGE_SHIFT);
> +
> +    domids_stride = ( flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN ) ? 0 : 1;

No space after '(' here.

> +    if ( map_size <= PAGE_SIZE )
> +        map = malloc(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, "anon 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;
> +    }
> +
> +    addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
> +                map->index);
> +    if ( addr != MAP_FAILED )
> +    {
> +        int rv = 0;
> +        struct ioctl_gntdev_unmap_notify notify;
> +
> +        notify.index = map->index;
> +        notify.action = 0;
> +        if ( notify_offset < PAGE_SIZE * count )
> +     {

Indentation.

> +            notify.index += notify_offset;
> +            notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
> +        }
> +        if ( notify_port != -1 ) {

Position of '{'.

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

Indentation.

> +            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);
> +    else
> +        free(map);
> +
> +    return addr;
> +}
> +
> +int osdep_gnttab_unmap(xengnttab_handle *xgt,
> +                       void *start_address,
> +                       uint32_t count)
> +{
> +    int rc;
> +    int fd = xgt->fd;
> +    struct ioctl_gntdev_unmap_grant_ref unmap_grant;
> +    struct ioctl_gntdev_get_offset_for_vaddr get_offset;
> +
> +    if ( start_address == NULL )
> +    {
> +        errno = EINVAL;
> +        return -1;
> +    }
> +
> +    /*
> +     * First, it is necessary to get the offset which was initially used to
> +     * mmap() the pages.
> +     */
> +    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_gnttab_grant_copy(xengnttab_handle *xgt,
> +                            uint32_t count,
> +                            xengnttab_grant_copy_segment_t *segs)
> +{
> +

Blank line.

> +    errno = ENOSYS;
> +    return -1;
> +}
> +
> +int osdep_gntshr_open(xengntshr_handle *xgs)
> +{
> +
> +    int fd = open(DEVXEN, O_RDWR);
> +    if ( fd == -1 )
> +        return -1;
> +    xgs->fd = fd;
> +
> +    return 0;
> +}
> +
> +int osdep_gntshr_close(xengntshr_handle *xgs)
> +{
> +

Blank line.

> +    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)
> +{
> +    int err;
> +    int fd = xgs->fd;
> +    void *area = NULL;
> +    struct ioctl_gntdev_unmap_notify notify;
> +    struct ioctl_gntdev_dealloc_gref gref_drop;
> +    struct ioctl_gntdev_alloc_gref *gref_info = NULL;
> +
> +    gref_info = malloc(sizeof(*gref_info) + count * sizeof(uint32_t));
> +    if ( gref_info == NULL )
> +        return NULL;
> +    gref_info->domid = domid;
> +    gref_info->flags = writable ? GNTDEV_ALLOC_FLAG_WRITABLE : 0;
> +    gref_info->count = count;
> +
> +    err = ioctl(fd, IOCTL_GNTDEV_ALLOC_GREF, gref_info);
> +    if ( err )
> +    {
> +        GSERROR(xgs->logger, "ioctl failed");
> +        goto out;
> +    }
> +
> +    area = mmap(NULL, count * PAGE_SIZE, PROT_READ | PROT_WRITE,
> +        MAP_SHARED, fd, gref_info->index);

Indentation.

> +
> +    if ( area == MAP_FAILED )
> +    {
> +        area = NULL;
> +        GSERROR(xgs->logger, "mmap failed");
> +        goto out_remove_fdmap;
> +    }
> +
> +    notify.index = gref_info->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 )
> +        err = ioctl(fd, IOCTL_GNTDEV_SET_UNMAP_NOTIFY, &notify);
> +    if ( err )
> +    {
> +        GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
> +        munmap(area, count * PAGE_SIZE);
> +        area = NULL;
> +    }
> +
> +    memcpy(refs, gref_info->gref_ids, count * sizeof(uint32_t));
> +
> + out_remove_fdmap:
> +    /*
> +     * Removing the mapping from the file descriptor does not cause the
> +     * pages to be deallocated until the mapping is removed.
> +     */
> +    gref_drop.index = gref_info->index;
> +    gref_drop.count = count;
> +    ioctl(fd, IOCTL_GNTDEV_DEALLOC_GREF, &gref_drop);
> + out:
> +    free(gref_info);
> +
> +    return area;
> +}
> +
> +int osdep_gntshr_unshare(xengntshr_handle *xgs,
> +                         void *start_address, uint32_t count)
> +{
> +

Blank line.

> +    return munmap(start_address, count * PAGE_SIZE);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.10.1 (Apple Git-78)
> 

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