[PATCH v2 0/3] Add support for NetBSD gntdev

[PATCH v2 0/3] Add support for NetBSD gntdev

[Roger Pau Monne]

The following series adds support for NetBSD gntdev to libxc, and 
makes libxl use Qemu as a disk backend if an image file stored on a 
remote filesystem is used.

Patch 2/3 is just a fix for the comment in function 
libxl__try_phy_backend.

[PATCH v2 1/3] libxc: add suport for NetBSD gntdev

[Roger Pau Monne]

Add OS specific handlers for NetBSD gntdev. The main difference is
that NetBSD passes the VA where the grant should be set inside the
IOCTL_GNTDEV_MAP_GRANT_REF ioctl, instead of using mmap (this is due
to OS constraints).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/include/xen-sys/NetBSD/gntdev.h |  151 ++++++++++++++++++++++++++
 tools/libxc/xc_netbsd.c               |  188 +++++++++++++++++++++++++++++++++
 2 files changed, 339 insertions(+), 0 deletions(-)
 create mode 100644 tools/include/xen-sys/NetBSD/gntdev.h

diff --git a/tools/include/xen-sys/NetBSD/gntdev.h b/tools/include/xen-sys/NetBSD/gntdev.h
new file mode 100644
index 0000000..a25133a
--- /dev/null
+++ b/tools/include/xen-sys/NetBSD/gntdev.h
@@ -0,0 +1,151 @@
+/******************************************************************************
+ * gntdev.h
+ * 
+ * Interface to /dev/xen/gntdev.
+ * 
+ * Copyright (c) 2007, D G Murray
+ * 
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ * 
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ * 
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ * 
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef __NetBSD_PUBLIC_GNTDEV_H__
+#define __NetBSD_PUBLIC_GNTDEV_H__
+
+struct ioctl_gntdev_grant_ref {
+	/* The domain ID of the grant to be mapped. */
+	uint32_t domid;
+	/* The grant reference of the grant to be mapped. */
+	uint32_t ref;
+};
+
+/*
+ * Inserts the grant references into the mapping table of an instance
+ * of gntdev. N.B. This does not perform the mapping, which is deferred
+ * until mmap() is called with @index as the offset.
+ */
+#define IOCTL_GNTDEV_MAP_GRANT_REF \
+	_IOWR('G', 0, struct ioctl_gntdev_map_grant_ref)
+struct ioctl_gntdev_map_grant_ref {
+	/* IN parameters */
+	/* The number of grants to be mapped. */
+	uint32_t count;
+	uint32_t pad;
+	uint64_t vaddr;
+	/* OUT parameters */
+	/* The offset to be used on a subsequent call to mmap(). */
+	uint64_t index;
+	/* Variable IN parameter. */
+	/* Array of grant references, of size @count. */
+	struct ioctl_gntdev_grant_ref *refs;
+};
+
+/*
+ * Removes the grant references from the mapping table of an instance of
+ * of gntdev. N.B. munmap() must be called on the relevant virtual address(es)
+ * before this ioctl is called, or an error will result.
+ */
+#define IOCTL_GNTDEV_UNMAP_GRANT_REF \
+	_IOW('G', 1, struct ioctl_gntdev_unmap_grant_ref)
+struct ioctl_gntdev_unmap_grant_ref {
+	/* IN parameters */
+	/* The offset was returned by the corresponding map operation. */
+	uint64_t index;
+	/* The number of pages to be unmapped. */
+	uint32_t count;
+	uint32_t pad;
+};
+
+/*
+ * Returns the offset in the driver's address space that corresponds
+ * to @vaddr. This can be used to perform a munmap(), followed by an
+ * UNMAP_GRANT_REF ioctl, where no state about the offset is retained by
+ * the caller. The number of pages that were allocated at the same time as
+ * @vaddr is returned in @count.
+ *
+ * N.B. Where more than one page has been mapped into a contiguous range, the
+ *      supplied @vaddr must correspond to the start of the range; otherwise
+ *      an error will result. It is only possible to munmap() the entire
+ *      contiguously-allocated range at once, and not any subrange thereof.
+ */
+#define IOCTL_GNTDEV_GET_OFFSET_FOR_VADDR \
+	_IOWR('G', 2, struct ioctl_gntdev_get_offset_for_vaddr)
+struct ioctl_gntdev_get_offset_for_vaddr {
+	/* IN parameters */
+	/* The virtual address of the first mapped page in a range. */
+	uint64_t vaddr;
+	/* OUT parameters */
+	/* The offset that was used in the initial mmap() operation. */
+	uint64_t offset;
+	/* The number of pages mapped in the VM area that begins at @vaddr. */
+	uint32_t count;
+	uint32_t pad;
+};
+
+/*
+ * Sets the maximum number of grants that may mapped at once by this gntdev
+ * instance.
+ *
+ * N.B. This must be called before any other ioctl is performed on the device.
+ */
+#define IOCTL_GNTDEV_SET_MAX_GRANTS \
+	_IOW('G', 3, struct ioctl_gntdev_set_max_grants)
+struct ioctl_gntdev_set_max_grants {
+	/* IN parameter */
+	/* The maximum number of grants that may be mapped at once. */
+	uint32_t count;
+};
+
+/*
+ * Sets up an unmap notification within the page, so that the other side can do
+ * cleanup if this side crashes. Required to implement cross-domain robust
+ * mutexes or close notification on communication channels.
+ *
+ * Each mapped page only supports one notification; multiple calls referring to
+ * the same page overwrite the previous notification. You must clear the
+ * notification prior to the IOCTL_GNTALLOC_DEALLOC_GREF if you do not want it
+ * to occur.
+ */
+#define IOCTL_GNTDEV_SET_UNMAP_NOTIFY \
+	_IOW('G', 7, struct ioctl_gntdev_unmap_notify)
+struct ioctl_gntdev_unmap_notify {
+	/* IN parameters */
+	/* Offset in the file descriptor for a byte within the page. This offset
+	 * is the result of the IOCTL_GNTDEV_MAP_GRANT_REF and is the same as
+	 * is used with mmap(). If using UNMAP_NOTIFY_CLEAR_BYTE, this is the byte
+	 * within the page to be cleared.
+	 */
+	uint64_t index;
+	/* Action(s) to take on unmap */
+	uint32_t action;
+	/* Event channel to notify */
+	uint32_t event_channel_port;
+};
+
+/* Clear (set to zero) the byte specified by index */
+#define UNMAP_NOTIFY_CLEAR_BYTE 0x1
+/* Send an interrupt on the indicated event channel */
+#define UNMAP_NOTIFY_SEND_EVENT 0x2
+
+#endif /* __NetBSD_PUBLIC_GNTDEV_H__ */
diff --git a/tools/libxc/xc_netbsd.c b/tools/libxc/xc_netbsd.c
index dbcb640..1ee12db 100644
--- a/tools/libxc/xc_netbsd.c
+++ b/tools/libxc/xc_netbsd.c
@@ -21,11 +21,15 @@
 #include "xc_private.h"
 
 #include <xen/sys/evtchn.h>
+#include <xen/sys/gntdev.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <malloc.h>
 #include <sys/mman.h>
 
+#define ROUNDUP(_x,_w) \
+    (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
+
 static xc_osdep_handle netbsd_privcmd_open(xc_interface *xch)
 {
     int flags, saved_errno;
@@ -390,6 +394,188 @@ void *xc_memalign(xc_interface *xch, size_t alignment, size_t size)
     return valloc(size);
 }
 
+#define GNT_DEV_NAME  "/dev/gntdev"
+
+static xc_osdep_handle netbsd_gnttab_open(xc_gnttab *xcg)
+{
+    int fd = open(GNT_DEV_NAME, O_RDWR);
+
+    if ( fd == -1 )
+        return XC_OSDEP_OPEN_ERROR;
+
+    return (xc_osdep_handle)fd;
+}
+
+static int netbsd_gnttab_close(xc_gnttab *xcg, xc_osdep_handle h)
+{
+    int fd = (int)h;
+    return close(fd);
+}
+
+static int netbsd_gnttab_set_max_grants(xc_gnttab *xch, xc_osdep_handle h,
+                                       uint32_t count)
+{
+    /* NetBSD doesn't implement this feature */
+    return 0;
+}
+
+static void *netbsd_gnttab_grant_map(xc_gnttab *xch, xc_osdep_handle h,
+                                    uint32_t count, int flags, int prot,
+                                    uint32_t *domids, uint32_t *refs,
+                                    uint32_t notify_offset,
+                                    evtchn_port_t notify_port)
+{
+    int fd = (int)h;
+    struct ioctl_gntdev_map_grant_ref map;
+    struct ioctl_gntdev_grant_ref *mrefs;
+    unsigned int map_size = ROUNDUP(count *
+                                    sizeof(struct ioctl_gntdev_map_grant_ref),
+                                    XC_PAGE_SHIFT);
+    void *addr = MAP_FAILED;
+    int domids_stride = 1;
+    int rv = 0;
+    struct ioctl_gntdev_unmap_notify notify;
+    int i;
+
+    if ( flags & XC_GRANT_MAP_SINGLE_DOMAIN )
+        domids_stride = 0;
+
+    if ( map_size <= XC_PAGE_SIZE )
+        mrefs = alloca(count * sizeof(struct ioctl_gntdev_map_grant_ref));
+    else
+    {
+        mrefs = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
+                   MAP_PRIVATE | MAP_ANON, -1, 0);
+        if ( mrefs == MAP_FAILED )
+        {
+            PERROR("netbsd_gnttab_grant_map: mmap of refs failed");
+            return NULL;
+        }
+    }
+
+    map.refs = mrefs;
+
+    for ( i = 0; i < count; i++ )
+    {
+        map.refs[i].domid = domids[i * domids_stride];
+        map.refs[i].ref = refs[i];
+    }
+
+    map.count = count;
+
+    addr = mmap(NULL, XC_PAGE_SIZE * count, prot,
+                MAP_ANON | MAP_SHARED, -1, 0);
+    if ( addr == MAP_FAILED )
+    {
+        PERROR("netbsd_gnttab_grant_map: mmap of grants failed");
+        goto out;
+    }
+
+    map.vaddr = (uint64_t) addr;
+
+    if ( ioctl(fd, IOCTL_GNTDEV_MAP_GRANT_REF, &map) )
+    {
+        PERROR("netbsd_gnttab_grant_map: ioctl MAP_GRANT_REF failed");
+        munmap(addr, XC_PAGE_SIZE * count);
+        goto out;
+    }
+
+    notify.index = map.index;
+    notify.action = 0;
+    if ( notify_offset < (XC_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 )
+    {
+        PERROR("netbsd_gnttab_grant_map: ioctl SET_UNMAP_NOTIFY failed");
+        munmap(addr, count * XC_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. */
+        PERROR("xc_gnttab_map_grant_refs: 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 > XC_PAGE_SIZE )
+        munmap(mrefs, map_size);
+
+    return addr;
+}
+
+
+
+static int netbsd_gnttab_munmap(xc_gnttab *xcg, xc_osdep_handle h,
+                               void *start_address, uint32_t count)
+{
+    int fd = (int)h;
+    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.
+     */
+    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 * getpagesize())) )
+        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;
+}
+
+static struct xc_osdep_ops netbsd_gnttab_ops = {
+    .open = &netbsd_gnttab_open,
+    .close = &netbsd_gnttab_close,
+
+    .u.gnttab = {
+        .set_max_grants = &netbsd_gnttab_set_max_grants,
+        .grant_map = &netbsd_gnttab_grant_map,
+        .munmap = &netbsd_gnttab_munmap,
+    },
+};
+
 static struct xc_osdep_ops *netbsd_osdep_init(xc_interface *xch, enum xc_osdep_type type)
 {
     switch ( type )
@@ -398,6 +584,8 @@ static struct xc_osdep_ops *netbsd_osdep_init(xc_interface *xch, enum xc_osdep_t
         return &netbsd_privcmd_ops;
     case XC_OSDEP_EVTCHN:
         return &netbsd_evtchn_ops;
+    case XC_OSDEP_GNTTAB:
+        return &netbsd_gnttab_ops;
     default:
         return NULL;
     }
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH v2 2/3] libxl: fix wrong comment

[Roger Pau Monne]

The comment in function libxl__try_phy_backend is wrong, 1 is returned
if the backend should be handled as "phy", while 0 is returned if not.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl_internal.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index cba3616..0b38e3e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1041,7 +1041,7 @@ static inline void libxl__domaindeathcheck_stop(libxl__gc *gc,
  * type of file using the PHY backend
  * st_mode: mode_t of the file, as returned by stat function
  *
- * Returns 0 on success, and < 0 on error.
+ * Returns 1 on success, and 0 if not suitable for phy backend.
  */
 _hidden int libxl__try_phy_backend(mode_t st_mode);
 
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH v2 3/3] libxl: switch NetBSD file backend to Qemu if filesystem is not local

[Roger Pau Monne]

This forces libxl to use Qemu when a raw image file hosted on a
remote filesystem is used as a disk backend. NetBSD currently only
supports qemu-traditional, so
device_model_version="qemu-xen-traditional" has to be added to the
config file when using image files.

To detect if the filesystem is local the statvfs(2) function is used,
as recommended by Greg Troxel.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl_device.c   |    8 +++++++-
 tools/libxl/libxl_internal.h |    4 +++-
 tools/libxl/libxl_linux.c    |    2 +-
 tools/libxl/libxl_netbsd.c   |   12 ++++++++++--
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 51dd06e..eca2ec2 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -144,6 +144,7 @@ typedef struct {
     libxl__gc *gc;
     libxl_device_disk *disk;
     struct stat stab;
+    struct statvfs stvfs;
 } disk_try_backend_args;
 
 static int disk_try_backend(disk_try_backend_args *a,
@@ -167,7 +168,7 @@ static int disk_try_backend(disk_try_backend_args *a,
             return backend;
         }
 
-        if (libxl__try_phy_backend(a->stab.st_mode))
+        if (libxl__try_phy_backend(a->stab.st_mode, a->stvfs.f_flag))
             return backend;
 
         LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
@@ -250,6 +251,11 @@ int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
                              disk->vdev, disk->pdev_path);
             return ERROR_INVAL;
         }
+        if (statvfs(disk->pdev_path, &a.stvfs)) {
+            LOGE(ERROR, "Disk vdev=%s failed to stat: %s",
+                 disk->vdev, disk->pdev_path);
+            return ERROR_INVAL;
+        }
     }
 
     if (disk->backend != LIBXL_DISK_BACKEND_UNKNOWN) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0b38e3e..98666bd 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -43,6 +43,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <sys/socket.h>
+#include <sys/statvfs.h>
 
 #include <xenstore.h>
 #include <xenctrl.h>
@@ -1040,10 +1041,11 @@ static inline void libxl__domaindeathcheck_stop(libxl__gc *gc,
  * libxl__try_phy_backend - Check if there's support for the passed
  * type of file using the PHY backend
  * st_mode: mode_t of the file, as returned by stat function
+ * f_flag: flag as returned by statvfs(2) function
  *
  * Returns 1 on success, and 0 if not suitable for phy backend.
  */
-_hidden int libxl__try_phy_backend(mode_t st_mode);
+_hidden int libxl__try_phy_backend(mode_t st_mode, unsigned long f_flag);
 
 
 _hidden char *libxl__devid_to_localdev(libxl__gc *gc, int devid);
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 1fed3cd..4e11eeb 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -17,7 +17,7 @@
 
 #include "libxl_internal.h"
  
-int libxl__try_phy_backend(mode_t st_mode)
+int libxl__try_phy_backend(mode_t st_mode, unsigned long f_flag)
 {
     if (!S_ISBLK(st_mode)) {
         return 0;
diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
index 9587833..aa7e0a9 100644
--- a/tools/libxl/libxl_netbsd.c
+++ b/tools/libxl/libxl_netbsd.c
@@ -17,9 +17,17 @@
 
 #include "libxl_internal.h"
 
-int libxl__try_phy_backend(mode_t st_mode)
+int libxl__try_phy_backend(mode_t st_mode, unsigned long f_flag)
 {
-    if (S_ISREG(st_mode) || S_ISBLK(st_mode))
+    if (S_ISBLK(st_mode))
+        /* File is a block device, always handled by xbdback */
+        return 1;
+
+    if (S_ISREG(st_mode) && (ST_LOCAL & f_flag))
+        /*
+         * File is a regular file, and the filesystem is local,
+         * attach using vnd(4).
+         */
         return 1;
 
     return 0;
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Manuel Bouyer]

On Thu, Nov 29, 2012 at 06:31:45PM +0100, Roger Pau Monne wrote:
> The following series adds support for NetBSD gntdev to libxc, and 
> makes libxl use Qemu as a disk backend if an image file stored on a 
> remote filesystem is used.


Can't you let the administrator decide this in the domU's config file ?
Right now vnd on nfs has problems, but nothing unfixable.
So one day you'll want to use the kernel driver for nfs too.
But maybe other local filesystems will have the problem.
So it's not for the tool do decide in the back of the admin.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Roger Pau Monné]

On 29/11/12 19:56, Manuel Bouyer wrote:
> On Thu, Nov 29, 2012 at 06:31:45PM +0100, Roger Pau Monne wrote:
>> The following series adds support for NetBSD gntdev to libxc, and 
>> makes libxl use Qemu as a disk backend if an image file stored on a 
>> remote filesystem is used.
> 
> 
> Can't you let the administrator decide this in the domU's config file ?
> Right now vnd on nfs has problems, but nothing unfixable.
> So one day you'll want to use the kernel driver for nfs too.
> But maybe other local filesystems will have the problem.
> So it's not for the tool do decide in the back of the admin.

We can always add a configure check later when this is fixed to decide
if it is needed or not. I don't like the idea of having to set a
configuration option if you want to use disk images on NFS, because if
you don't know about this specific issue your whole Dom0 will crash
which is really not desired or user friendly.

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Roger Pau Monné]

On 30/11/12 09:52, Manuel Bouyer wrote:
> On Fri, Nov 30, 2012 at 09:42:52AM +0100, Roger Pau Monné wrote:
>> On 29/11/12 19:56, Manuel Bouyer wrote:
>>> On Thu, Nov 29, 2012 at 06:31:45PM +0100, Roger Pau Monne wrote:
>>>> The following series adds support for NetBSD gntdev to libxc, and 
>>>> makes libxl use Qemu as a disk backend if an image file stored on a 
>>>> remote filesystem is used.
>>>
>>>
>>> Can't you let the administrator decide this in the domU's config file ?
>>> Right now vnd on nfs has problems, but nothing unfixable.
>>> So one day you'll want to use the kernel driver for nfs too.
>>> But maybe other local filesystems will have the problem.
>>> So it's not for the tool do decide in the back of the admin.
>>
>> We can always add a configure check later when this is fixed to decide
>> if it is needed or not. I don't like the idea of having to set a
>> configuration option if you want to use disk images on NFS, because if
>> you don't know about this specific issue your whole Dom0 will crash
>> which is really not desired or user friendly.
> 
> And I don't like the idea of software doing things in my back.
> And, beside this, I don't think local vs remote is the right criteria.
> There are remote filesystems which may play nice with vnd. There
> are local filesystems that may not play nice with vnd.

I would agree with you if this was a DomU crash, but in this case the
crash happens in the Dom0, and every DomU that the system might be
running crashes completely. This is not acceptable in any way from my
point of view.

I think we should not expect the user to be aware of this kind of
problems. If we cannot guarantee that the vnd driver is functional for
all filesystems, we should not use it. From my point of view reliability
should always come before performance.

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Manuel Bouyer]

On Fri, Nov 30, 2012 at 10:57:57AM +0100, Roger Pau Monné wrote:
> There are also other tools that build on top of libxl, like libvirt, are
> we going to modify those high level tools to add a new option to the
> config file if the disk of a DomU is in NFS and the Dom0 is NetBSD? I

No if. the admin should be able to choose what backend to use, regardless
of the storage used, and probably of the dom0 OS (you have several backend
available in linux as well, isn't it) ?

> don't think we should take that road, I think libxl should take care of
> all those quicks, and provide an uniform layer that can be trusted
> independently of the environment, so the same configuration file can be
> used in all supported Dom0 OSes.

Ho, that won't work anyway. The options to setup the network are different,
for example.

> 
> Not fixing this in libxl just moves the problem a layer upper, where
> there's a lot more of options, and of course a lot more of work to track
> and fix them all.

Then, as a compromise, could the default be specified in xend-config.sxp
and be overridable in the domU's config file ?
this way, the admin can set a sane defaut for his setup, and eventually
decide what to use on a per-domU basis.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Ian Campbell]

On Fri, 2012-11-30 at 09:57 +0000, Roger Pau Monné wrote:
> On 30/11/12 10:41, Manuel Bouyer wrote:
> > On Fri, Nov 30, 2012 at 10:21:02AM +0100, Roger Pau Monné wrote:
> >>> And I don't like the idea of software doing things in my back.
> >>> And, beside this, I don't think local vs remote is the right criteria.
> >>> There are remote filesystems which may play nice with vnd. There
> >>> are local filesystems that may not play nice with vnd.
> >>
> >> I would agree with you if this was a DomU crash, but in this case the
> >> crash happens in the Dom0, and every DomU that the system might be
> >> running crashes completely. This is not acceptable in any way from my
> >> point of view.
> >>
> >> I think we should not expect the user to be aware of this kind of
> >> problems. If we cannot guarantee that the vnd driver is functional for
> >> all filesystems, we should not use it. From my point of view reliability
> >> should always come before performance.
> > 
> > In my POV, the admin show know what he's doing. This includes be aware of the
> > limitations of the software he uses. A 50% performances loss just "in case"
> > is not accetpable. Or at last last there should be a way for the admin
> > to revert to an acceptable configuration, performance wise, without
> > hacking and rebuilding from sources.
> 
> There are also other tools that build on top of libxl, like libvirt, are
> we going to modify those high level tools to add a new option to the
> config file if the disk of a DomU is in NFS and the Dom0 is NetBSD? I
> don't think we should take that road, I think libxl should take care of
> all those quicks, and provide an uniform layer that can be trusted
> independently of the environment, so the same configuration file can be
> used in all supported Dom0 OSes.
> 
> Not fixing this in libxl just moves the problem a layer upper, where
> there's a lot more of options, and of course a lot more of work to track
> and fix them all.

libxl only selects the backend itself if the caller doesn't provide one.
If the caller sets the backend field !=  UNKNOWN then libxl will (try)
and use it. This field is exposed by xl via the backendtype= key in the
disk configuration
http://xenbits.xen.org/docs/unstable/misc/xl-disk-configuration.txt

AIUI libvirt reuses xl's syntax and so would inherit this for free. In
any case any upper layer building on libxl is likely to want to provide
the option to manually select a specific backend. I think this satisfies
Manuel's "there should be a way for the admin..." requirement.

I agree that it doesn't seem right to expect upper layers to
automatically set this manual override, IYSWIM, though.

Ian.


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

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Manuel Bouyer]

On Fri, Nov 30, 2012 at 10:32:32AM +0000, Ian Campbell wrote:
> libxl only selects the backend itself if the caller doesn't provide one.
> If the caller sets the backend field !=  UNKNOWN then libxl will (try)
> and use it. This field is exposed by xl via the backendtype= key in the
> disk configuration
> http://xenbits.xen.org/docs/unstable/misc/xl-disk-configuration.txt

thanks for pointing this out.
I guess qdisk is the qemu backend, and tap would be the in-kernel backend ?

Is there a way to specify in a config file which default xl should use ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Toby Karyadi]

On 11/30/12 5:50 AM, Manuel Bouyer wrote:
> On Fri, Nov 30, 2012 at 10:43:21AM +0000, Ian Campbell wrote:
>> On Fri, 2012-11-30 at 10:38 +0000, Manuel Bouyer wrote:
>>> On Fri, Nov 30, 2012 at 10:32:32AM +0000, Ian Campbell wrote:
>>>> libxl only selects the backend itself if the caller doesn't provide one.
>>>> If the caller sets the backend field !=  UNKNOWN then libxl will (try)
>>>> and use it. This field is exposed by xl via the backendtype= key in the
>>>> disk configuration
>>>> http://xenbits.xen.org/docs/unstable/misc/xl-disk-configuration.txt
>>> thanks for pointing this out.
>>> I guess qdisk is the qemu backend, and tap would be the in-kernel backend ?
>> qdisk == qemu, tap == blktap, phy == in kernel.
> OK; but then, how does the script called by xenbackendd know what setup
> is should do ? With xm, it would get a string in the form
> phy:/dev/wd0e
> or
> file:/domains/foo.img
>
> but from what I've understant, this syntax is deprecated now ?

Hi Manuel, thanks for all of your work on xen.

So, the short answer is that the /usr/pkg/etc/xen/scripts/block 
executable (which is just a shell script) needs to fish the type of the 
backend and other extra info out of xenstore. In the case of netbsd's 
block script, it uses the xenstore-read utility. When the block script 
is called, $1 ($xpath) will be an entry like so 
/local/domain/0/backend/vbd/2/768; so this is vbd for domU instance #2 
with disk instance id 768. $2 ($xstatus) is the reason the block script 
is called, 2 for startup, and 6 for tear down.

If you look at the block script you can also see that the block script 
needs to fish out the type of the backend that is located at $xpath/type 
in xenstore. The block script utilizes the xenstore-read and 
xenstore-write to read/modify xenstore, if you notice.

So, under $xpath here are the pertinent entries:
- $xpath/type
- $xpath/params
- $xpath/physical-disk

With xm+xend, for disk config file:/var/xen/domu/server001/disk.img, 
prior to the block script getting called:
$xpath/type = 'file'
$xpath/params = '/var/xen/domu/server001/disk.img'
$xpath/physical-disk = <unspecified>

Then when the block script is called, it detects that the type is 'file' 
and so it would call vnconfig using the value $xpath/params as the 
actual file using an unused vnd device, say vnd2. Then it would modify 
$xpath/physical-disk using the xenstore-write utility and set the value 
of $xpath/physical-disk to /dev/vnd2. After that, the handling, 
everything else is handled as if the backend is an actual physical 
device, which is true.

If the value of $xpath/type is 'phy', nothing needs to be done, since 
$xpath/physical-disk has been setup properly.

Now, the above behavior is how xm+xend+xenbackendd works. There was a 
bug in libxl/xl that I fixed as described here 
http://mail-index.netbsd.org/port-xen/2012/05/29/msg007252.html so that 
libxl/xl would behave the same way. You might also notice, I made small 
changes to allow custom backend type. I don't mean to keep tooting my 
own horn, but I just don't want the fix to get lost, although I just 
learned that I probably should do a bug report with the patch, but I 
don't have much time right now.

Hopefully I don't bore you to death if you read this far, but the way 
the libxl/xl is going seems somewhat ridiculous, where they are trying 
to insert more and more policy vs functionality, as evidenced by Roger's 
effort to outrightly to just decide (hence a policy) that 'well, vnd 
can't work with file on NFS, so be damn with it and just route 
everything via qemu-dm'. Additionally, they are planning to retire 
xenbackendd and again this is a trend that I don't like where they clump 
everything into one giant ball called the libxl/xl, as opposed to a 
bunch of little executables doing specific things. Another illustration: 
you might ask who's managing the domU in the absence of xend; well what 
happens is that when you do 'xl create ...' xl would then daemonize 
itself to watch xenstore for that specific domU that it just created. I 
guess that's cool, but to me it's got the feel to put everything into 
one giant, monolithic, unflexible entity (shudder).

So, I'm just writing to give you some more information about this whole 
libxl/xl stuff, and hopefully it'll give you some arsenal so that some 
of the xen folks don't try to force policies that is claimed to protect 
'end-user' or very linux specific.

Cheers,
Toby

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Roger Pau Monné]

On 03/12/12 20:47, Toby Karyadi wrote:
> On 11/30/12 5:50 AM, Manuel Bouyer wrote:
>> On Fri, Nov 30, 2012 at 10:43:21AM +0000, Ian Campbell wrote:
>>> On Fri, 2012-11-30 at 10:38 +0000, Manuel Bouyer wrote:
>>>> On Fri, Nov 30, 2012 at 10:32:32AM +0000, Ian Campbell wrote:
>>>>> libxl only selects the backend itself if the caller doesn't provide one.
>>>>> If the caller sets the backend field !=  UNKNOWN then libxl will (try)
>>>>> and use it. This field is exposed by xl via the backendtype= key in the
>>>>> disk configuration
>>>>> http://xenbits.xen.org/docs/unstable/misc/xl-disk-configuration.txt
>>>> thanks for pointing this out.
>>>> I guess qdisk is the qemu backend, and tap would be the in-kernel backend ?
>>> qdisk == qemu, tap == blktap, phy == in kernel.
>> OK; but then, how does the script called by xenbackendd know what setup
>> is should do ? With xm, it would get a string in the form
>> phy:/dev/wd0e
>> or
>> file:/domains/foo.img
>>
>> but from what I've understant, this syntax is deprecated now ?
> 
> Hi Manuel, thanks for all of your work on xen.
> 
> So, the short answer is that the /usr/pkg/etc/xen/scripts/block 
> executable (which is just a shell script) needs to fish the type of the 
> backend and other extra info out of xenstore. In the case of netbsd's 
> block script, it uses the xenstore-read utility. When the block script 
> is called, $1 ($xpath) will be an entry like so 
> /local/domain/0/backend/vbd/2/768; so this is vbd for domU instance #2 
> with disk instance id 768. $2 ($xstatus) is the reason the block script 
> is called, 2 for startup, and 6 for tear down.
> 
> If you look at the block script you can also see that the block script 
> needs to fish out the type of the backend that is located at $xpath/type 
> in xenstore. The block script utilizes the xenstore-read and 
> xenstore-write to read/modify xenstore, if you notice.
> 
> So, under $xpath here are the pertinent entries:
> - $xpath/type
> - $xpath/params
> - $xpath/physical-disk
> 
> With xm+xend, for disk config file:/var/xen/domu/server001/disk.img, 
> prior to the block script getting called:
> $xpath/type = 'file'
> $xpath/params = '/var/xen/domu/server001/disk.img'
> $xpath/physical-disk = <unspecified>
> 
> Then when the block script is called, it detects that the type is 'file' 
> and so it would call vnconfig using the value $xpath/params as the 
> actual file using an unused vnd device, say vnd2. Then it would modify 
> $xpath/physical-disk using the xenstore-write utility and set the value 
> of $xpath/physical-disk to /dev/vnd2. After that, the handling, 
> everything else is handled as if the backend is an actual physical 
> device, which is true.
> 
> If the value of $xpath/type is 'phy', nothing needs to be done, since 
> $xpath/physical-disk has been setup properly.
> 
> Now, the above behavior is how xm+xend+xenbackendd works. There was a 
> bug in libxl/xl that I fixed as described here 
> http://mail-index.netbsd.org/port-xen/2012/05/29/msg007252.html so that 
> libxl/xl would behave the same way. You might also notice, I made small 
> changes to allow custom backend type. I don't mean to keep tooting my 
> own horn, but I just don't want the fix to get lost, although I just 
> learned that I probably should do a bug report with the patch, but I 
> don't have much time right now.
> 
> Hopefully I don't bore you to death if you read this far, but the way 
> the libxl/xl is going seems somewhat ridiculous, where they are trying 
> to insert more and more policy vs functionality, as evidenced by Roger's 
> effort to outrightly to just decide (hence a policy) that 'well, vnd 

I don't understand this whole argument about "policy vs functionality",
from my point of view what we have now is also a policy, every raw disk
file is attached using the vnd device.

And getting a gntdev isn't certainly a policy, on the other hand this is
going to get us much more functionality (like the ability to run
backends in userspace, for example support for blktap3 when it is released).

> can't work with file on NFS, so be damn with it and just route 
> everything via qemu-dm'. 

Again, as discused with Manuel, a better solution has to be found for
this issue, and I'm sure we can reach a consensus.

> Additionally, they are planning to retire 
> xenbackendd and again this is a trend that I don't like where they clump 
> everything into one giant ball called the libxl/xl, as opposed to a 

The retirement of xenbackendd is done for a good reason, calling hotplug
scripts from libxl allows for better control of when hotplug scripts are
called, and also allows for better error handling if these scripts fail.
Same scripts are called, so functionality stays the same.

This was done for both NetBSD and Linux, in the past Linux used to call
hotplug scripts from udev, and now they are called from libxl too. I'm
not able to see how having a more unified hotplug script interface can
be a bad thing.

> bunch of little executables doing specific things. Another illustration: 
> you might ask who's managing the domU in the absence of xend; well what 
> happens is that when you do 'xl create ...' xl would then daemonize 
> itself to watch xenstore for that specific domU that it just created. I 
> guess that's cool, but to me it's got the feel to put everything into 
> one giant, monolithic, unflexible entity (shudder).

Again, I'm not able to see how this is a problem, in the past you had
xend, which was a gigantic piece of python code acting as a central
arbiter, now you have a small C daemon for each running domain. libxl is
generally considered a better piece of code, and is much more easier to
maintain than xend.

Do you have any concrete technical reason to belive that xend was better
than libxl?

> So, I'm just writing to give you some more information about this whole 
> libxl/xl stuff, and hopefully it'll give you some arsenal so that some 
> of the xen folks don't try to force policies that is claimed to protect 
> 'end-user' or very linux specific.

libxl has even split OS specific routines in separate files, both for
Linux and NetBSD and frankly, I'm not able to see how that is worse that
the amount of patches we had in pkgsrc to have xend working. Now libxl
works out-of-the-box on NetBSD, and many efforts have been put into that.

We should focus our efforts on getting Xen to work on NetBSD without a
ton of NetBSD specific out of the tree patches, and right now my biggest
concern is getting Qemu upstream working.

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Toby Karyadi]

Whoops, this was meant to be a correspondence just for Manuel. But since 
the cat is out of the bag...

On 12/3/12 4:34 PM, Roger Pau Monné wrote:
> I don't understand this whole argument about "policy vs 
> functionality", from my point of view what we have now is also a 
> policy, every raw disk file is attached using the vnd device. And 
> getting a gntdev isn't certainly a policy, on the other hand this is 
> going to get us much more functionality (like the ability to run 
> backends in userspace, for example support for blktap3 when it is 
> released). 
Yes, on netbsd file: disk config will use vnd, but only because the way 
the block script is written. The functionality of the block script is 
well defined, and therefore is much easier to override, then, say 
patching libxl. Therefore, it's not a 'policy', but more of a default 
functionality, since anyone who has some knowledge about shell script 
can modify it to do something else, e.g. run the vnd under rump (is that 
possible?) to avoid dom0 blowing up.

But if it is decided that disk config that begins with file: to always 
use qemu-dm just because on the off chance that using vnd over NFS can 
blow up the dom0, and if there is no way to override it, or really 
difficult (which is relative, I know), then it becomes a  'policy'. Can 
I setup iscsi through hotplug for example?

  To be honest, I haven't look into how the hotplug script works, so 
hopefully it can provide equivalent functionality and flexibility.
>> Again, as discused with Manuel, a better solution has to be found for
>> this issue, and I'm sure we can reach a consensus.
That's what I'm hoping.
> The retirement of xenbackendd is done for a good reason, calling 
> hotplug scripts from libxl allows for better control of when hotplug 
> scripts are called, and also allows for better error handling if these 
> scripts fail. Same scripts are called, so functionality stays the 
> same. This was done for both NetBSD and Linux, in the past Linux used 
> to call hotplug scripts from udev, and now they are called from libxl 
> too. I'm not able to see how having a more unified hotplug script 
> interface can be a bad thing. Again, I'm not able to see how this is a 
> problem, in the past you had xend, which was a gigantic piece of 
> python code acting as a central arbiter, now you have a small C daemon 
> for each running domain. libxl is generally considered a better piece 
> of code, and is much more easier to maintain than xend. Do you have 
> any concrete technical reason to belive that xend was better than libxl? 
Like I said, I probably look into the hotplug script framework before I 
yammer any further. If it allows for overriding file: disk configs with 
vnd or whatever, then I'm happy. I don't think xend is better than 
libxl, on the contrary, I find libxl source to be more understandable, 
and much smaller. I couldn't make out head from tail with xend source 
code and I'm a python programmer most of the time. But by the same token 
I know I've got this blob called xend that does these specific things, 
and xenbackendd that does these specific things. There is really nothing 
that prevents from say creating another program called domu_watcher (or 
whatever) that is linked to libxl for functionality sharing and gets 
launched by xl during xl create; but again, this is more nitpicks and a 
matter of preference.
> libxl has even split OS specific routines in separate files, both for 
> Linux and NetBSD and frankly, I'm not able to see how that is worse 
> that the amount of patches we had in pkgsrc to have xend working. Now 
> libxl works out-of-the-box on NetBSD, and many efforts have been put 
> into that. We should focus our efforts on getting Xen to work on 
> NetBSD without a ton of NetBSD specific out of the tree patches, and 
> right now my biggest concern is getting Qemu upstream working. 
I noticed that even in 4.1.2, which I thought was a good start, however 
those compile-time 'plugins' were not useful back then since it was only 
for whether blktap is supported, to which under netbsd compilation the 
answer is 'nah!'. I haven't looked into 4.2 libxl src either, so maybe 
the plugin points are much extensive now. I know enough that creating a 
system with plugin support, whether compile-time or during runtime, is 
not easy, because to a certain degree you have to expect the unexpected. 
If I sound I'm just griping all of the time, it's just that I had more 
time right now to jump in, but I don't. So, I do a lot of hand waving 
;-). I understand that you gotta get qemu working, but that's orthogonal 
to letting us shoot ourselves in the foot ;-) I mean, have you used 
netbsd's disklabel?

Anyways...

Cheers,
Toby

Re: [PATCH v2 1/3] libxc: add suport for NetBSD gntdev

[Ian Campbell]

On Thu, 2012-11-29 at 17:31 +0000, Roger Pau Monne wrote:
> Add OS specific handlers for NetBSD gntdev. The main difference is
> that NetBSD passes the VA where the grant should be set inside the
> IOCTL_GNTDEV_MAP_GRANT_REF ioctl, instead of using mmap (this is due
> to OS constraints).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Is anyone from the NetBSD camp likely to review this code?

Otherwise I'm minded to just throw it in, given that it looks OK to
me ;-)

> +static int netbsd_gnttab_set_max_grants(xc_gnttab *xch, xc_osdep_handle h,
> +                                       uint32_t count)
> +{
> +    /* NetBSD doesn't implement this feature */
> +    return 0;
> +}

Is -ENOSYS or "errno=ENOSYS; return -1" more appropriate then?

Ian.


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

Re: [PATCH v2 1/3] libxc: add suport for NetBSD gntdev

[Roger Pau Monné]

On 04/12/12 15:45, Ian Campbell wrote:
> On Thu, 2012-11-29 at 17:31 +0000, Roger Pau Monne wrote:
>> Add OS specific handlers for NetBSD gntdev. The main difference is
>> that NetBSD passes the VA where the grant should be set inside the
>> IOCTL_GNTDEV_MAP_GRANT_REF ioctl, instead of using mmap (this is due
>> to OS constraints).
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Is anyone from the NetBSD camp likely to review this code?
> 
> Otherwise I'm minded to just throw it in, given that it looks OK to
> me ;-)
> 
>> +static int netbsd_gnttab_set_max_grants(xc_gnttab *xch, xc_osdep_handle h,
>> +                                       uint32_t count)
>> +{
>> +    /* NetBSD doesn't implement this feature */
>> +    return 0;
>> +}
> 
> Is -ENOSYS or "errno=ENOSYS; return -1" more appropriate then?

Thanks for the review, I've just copied Linux behaviour and returned 0
when the ioctl is not implemented, I'm afraid that if I return some kind
of error legancy applications (which expect this call to work) may abort
the whole process (this is from
xc_linux_osdep.c:linux_gnttab_set_max_grants).


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

Re: [PATCH v2 2/3] libxl: fix wrong comment

[Ian Campbell]

On Thu, 2012-11-29 at 17:31 +0000, Roger Pau Monne wrote:
> The comment in function libxl__try_phy_backend is wrong, 1 is returned
> if the backend should be handled as "phy", while 0 is returned if not.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked + applied, thanks.

> ---
>  tools/libxl/libxl_internal.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index cba3616..0b38e3e 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1041,7 +1041,7 @@ static inline void libxl__domaindeathcheck_stop(libxl__gc *gc,
>   * type of file using the PHY backend
>   * st_mode: mode_t of the file, as returned by stat function
>   *
> - * Returns 0 on success, and < 0 on error.
> + * Returns 1 on success, and 0 if not suitable for phy backend.
>   */
>  _hidden int libxl__try_phy_backend(mode_t st_mode);
>  



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

Re: [PATCH v2 0/3] Add support for NetBSD gntdev

[Ian Jackson]

Toby Karyadi writes ("Re: [Xen-devel] [PATCH v2 0/3] Add support for NetBSD gntdev"):
> Hopefully I don't bore you to death if you read this far, but the way 
> the libxl/xl is going seems somewhat ridiculous, where they are trying 
> to insert more and more policy vs functionality,

The proposal here AIUI is just about what the default should be.

We have taken the view that the admin should not /need/ to specify
explicitly which software components to assemble together to get the
block devices to work.

That doesn't mean that the admin won't be enabled to specify exactly
what they want (and to keep all the pieces if it doesn't work).

Ian.