Xen project Mailing List

On 5 August 2016 at 22:15, Roger Pau Monné <royger@xxxxxxxxxxx> wrote:

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.

Done.

> +
> +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.

Ioctl Removed.

> +};
> +
> +#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.

Done.

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

Done.

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

Done.

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

Most code is from linux.c. I modified the code to use appropriate definitions from the interface we expose, but left all other things in place.

I went over the code, to fix the style issues, but found some other errors.

For instance, in function osdep_gntshr_share_pages(), notify_offset and notify_port are specified -1 when the user doesn't want these notify functions. But there is no such check for notify_offset, and it ends up having an unintended consequence on a previous grant (map->index-1 lies on a different grant).

I'll fix this in the freebsd file, and also the linux one and send by tonight. :)

> + /*
> + * 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 = ""> > + 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.

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