On 11 August 2016 at 21:34, Wei Liu <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > > --- > > 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 <[email protected]> > > + * Copyright (c) 2016-2017, Akshay Jaggi <[email protected]> > > + * > > + * 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 [email protected] https://lists.xen.org/xen-devel
