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

[Akshay Jaggi <akshay1994.leo@xxxxxxxxx>]

On 11 August 2016 at 21:34, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:

Please send a separate patch for linux.c as we probably want to backport
that.

It needs changes to "private.h" and other files too, so I will have to send another patch anyway. If you see in gntshr_core.c, you will notice we are passing -1 to an unsigned argument. This is also the reason why this bug doesn't have unintended consequences (data corruption on disk backends, etc.)

On 11 August 2016 at 21:44, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:

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.

I'll do this with my other patch that touches "private.h" and "linux.c". I'm using this patch to only introduce freebsd gnttab support.

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 (!).

Noted. And Done.

Regards,

Akshay

On 11 August 2016 at 21:44, Wei Liu <wei.liu2@xxxxxxxxxx> 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/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/"
> +
> +#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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel