[PATCH v2] Fix error: array subscript has type 'char'

[PATCH v2] Fix error: array subscript has type 'char'

[Manuel Bouyer]

Use unsigned char variable, or cast to (unsigned char), for
tolower()/islower() and friends. Fix compiler error
array subscript has type 'char' [-Werror=char-subscripts]

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Release-Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libs/light/libxl_qmp.c | 2 +-
 tools/xentrace/xentrace.c    | 2 +-
 xen/tools/symbols.c          | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libs/light/libxl_qmp.c b/tools/libs/light/libxl_qmp.c
index c394000ea9..9b638e6f54 100644
--- a/tools/libs/light/libxl_qmp.c
+++ b/tools/libs/light/libxl_qmp.c
@@ -1249,7 +1249,7 @@ static int qmp_error_class_to_libxl_error_code(libxl__gc *gc,
                 se++;
                 continue;
             }
-            if (tolower(*s) != tolower(*se))
+            if (tolower((unsigned char)*s) != tolower((unsigned char)*se))
                 break;
             s++, se++;
         }
diff --git a/tools/xentrace/xentrace.c b/tools/xentrace/xentrace.c
index 4b50b8a53e..a8903ebf46 100644
--- a/tools/xentrace/xentrace.c
+++ b/tools/xentrace/xentrace.c
@@ -957,7 +957,7 @@ static int parse_cpumask_range(const char *mask_str, xc_cpumap_t map)
 {
     unsigned int a, b;
     int nmaskbits;
-    char c;
+    unsigned char c;
     int in_range;
     const char *s;
 
diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c
index 9f9e2c9900..0b12452616 100644
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -173,11 +173,11 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */
 	s->len = strlen(str) + 1;
-	if (islower(stype) && filename)
+	if (islower((unsigned char)stype) && filename)
 		s->len += strlen(filename) + 1;
 	s->sym = malloc(s->len + 1);
 	sym = SYMBOL_NAME(s);
-	if (islower(stype) && filename) {
+	if (islower((unsigned char)stype) && filename) {
 		sym = stpcpy(sym, filename);
 		*sym++ = '#';
 	}
-- 
2.29.2

[PATCH v2] NetBSD hotplug: Introduce locking functions

[Manuel Bouyer]

On NetBSD, some block device configuration requires serialisation.
Introcuce locking functions (derived from the Linux version), and use them
in the block script where appropriate.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/hotplug/NetBSD/Makefile   |   1 +
 tools/hotplug/NetBSD/block      |   5 +-
 tools/hotplug/NetBSD/locking.sh | 121 ++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+), 1 deletion(-)
 create mode 100644 tools/hotplug/NetBSD/locking.sh

diff --git a/tools/hotplug/NetBSD/Makefile b/tools/hotplug/NetBSD/Makefile
index 6926885ab8..114b223207 100644
--- a/tools/hotplug/NetBSD/Makefile
+++ b/tools/hotplug/NetBSD/Makefile
@@ -3,6 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 # Xen script dir and scripts to go there.
 XEN_SCRIPTS =
+XEN_SCRIPTS += locking.sh
 XEN_SCRIPTS += block
 XEN_SCRIPTS += vif-bridge
 XEN_SCRIPTS += vif-ip
diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block
index c8b31a7b2b..eb5e80d640 100644
--- a/tools/hotplug/NetBSD/block
+++ b/tools/hotplug/NetBSD/block
@@ -5,6 +5,7 @@
 
 DIR=$(dirname "$0")
 . "${DIR}/hotplugpath.sh"
+. "${DIR}/locking.sh"
 
 PATH=${bindir}:${sbindir}:${LIBEXEC_BIN}:/bin:/usr/bin:/sbin:/usr/sbin
 export PATH
@@ -52,6 +53,7 @@ case $xstatus in
 			available_disks="$available_disks $disk"
 			eval $disk=free
 		done
+		claim_lock block
 		# Mark the used vnd(4) devices as ``used''.
 		for disk in `sysctl hw.disknames`; do
 			case $disk in
@@ -67,6 +69,7 @@ case $xstatus in
 				break	
 			fi
 		done
+		release_lock block
 		if [ x$device = x ] ; then
 			error "no available vnd device"
 		fi
@@ -76,7 +79,7 @@ case $xstatus in
 		device=$xparams
 		;;
 	esac
-	physical_device=$(stat -f '%r' "$device")
+	physical_device=$(stat -L -f '%r' "$device")
 	xenstore-write $xpath/physical-device $physical_device
 	xenstore-write $xpath/hotplug-status connected
 	exit 0
diff --git a/tools/hotplug/NetBSD/locking.sh b/tools/hotplug/NetBSD/locking.sh
new file mode 100644
index 0000000000..2098d0e3ab
--- /dev/null
+++ b/tools/hotplug/NetBSD/locking.sh
@@ -0,0 +1,121 @@
+#
+# Copyright (c) 2005 XenSource Ltd.
+# Copyright (c) 2007 Red Hat
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of version 2.1 of the GNU Lesser General Public
+# License as published by the Free Software Foundation.
+#
+# 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/>.
+#
+
+#
+# Serialisation
+#
+
+LOCK_BASEDIR=$XEN_LOCK_DIR/xen-hotplug
+
+_setlockfd()
+{
+    _lockfd=9
+    _lockfile="$LOCK_BASEDIR/$1"
+}
+
+
+claim_lock()
+{
+    mkdir -p "$LOCK_BASEDIR"
+    _setlockfd $1
+    # The locking strategy is identical to that from with-lock-ex(1)
+    # from chiark-utils, except using flock.  It has the benefit of
+    # it being possible to safely remove the lockfile when done.
+    # See below for a correctness proof.
+    local stat
+    while true; do
+        eval "exec $_lockfd<> $_lockfile"
+	# we can't flock $_lockfd here, as the shell closes it on exec.
+	# Workaround by redirecting to 0 for the command, and flock 0 instead.
+        flock -v -x 0  0<& $_lockfd|| exit 1
+        local file_stat
+        local fd_stat
+        if fd_stat=$(stat -f '%d.%i' 0<&$_lockfd 2>/dev/null) && file_stat=$(stat -f '%d.%i' $_lockfile 2>/dev/null )
+        then
+            if [ "$fd_stat" = "$file_stat" ] ; then break; fi
+        fi
+        # Some versions of bash appear to be buggy if the same
+        # $_lockfile is opened repeatedly. Close the current fd here.
+        eval "exec $_lockfd<&-"
+    done
+}
+
+
+release_lock()
+{
+    _setlockfd $1
+    rm "$_lockfile"
+}
+
+# Protocol and correctness proof:
+#
+# * The lock is owned not by a process but by an open-file (informally
+#   an fd).  Any process with an fd onto this open-file is a
+#   lockholder and may perform the various operations; such a process
+#   should only do so when its co-lockholder processes expect.  Ie, we
+#   will treat all processes holding fds onto the open-file as acting
+#   in concert and not distinguish between them.
+#
+# * You are a lockholder if
+#     - You have an fd onto an open-file which
+#       currently holds an exclusive flock lock on its inum
+#     - and that inum is currently linked at the lockfile path
+#
+# * The rules are:
+#     - No-one but a lockholder may unlink the lockfile path
+#       (or otherwise cause it to stop referring to a file it
+#       refers to).
+#     - Anyone may open the lockfile with O_CREAT
+#
+# * The protocol for locking is:
+#     - Open the file (O_CREAT)
+#     - flock it
+#     - fstat the fd you have open
+#     - stat the lockfile path
+#     - if both are equal you have the lock, otherwise try again.
+#
+# * Informal proof of exclusivity:
+#     - No two open-files can hold an fcntl lock onto the same file
+#       at the same time
+#     - No two files can have the same name at the same time
+#
+# * Informal proof of correctness of locking protocol:
+#     - After you call flock successfully no-one other than you
+#       (someone with the same open-file) can stop you having
+#       that flock lock.
+#     - Obviously the inum you get from the fstat is fixed
+#     - At the point where you call stat there are two
+#       possibilities:
+#         (i) the lockfile path referred to some other inum
+#             in which case you have failed
+#         (ii) the lockfile path referred to the same file
+#             in which case at that point you were the
+#             lockholder (by definition).
+#
+# * Informal proof that no-one else can steal the lock:
+#     - After you call flock successfully no-one other than you
+#       can stop you having that flock lock
+#     - No-one other than the lockholder is permitted to stop
+#       the path referring to a particular inum.  So if you
+#       hold the lock then only you are allowed to stop the
+#       path referring to the file whose flock you hold; so
+#       it will continue to refer to that file.
+#   That's both the conditions for being the lockholder.
+#
+#   Thus once you hold the lock at any instant, you will
+#   continue to do so until you voluntarily stop doing so
+#   (eg by unlinking the lockfile or closing the fd).
-- 
2.29.2

[PATCH v2] NetBSD hotplug: fix block unconfigure on destroy

[Manuel Bouyer]

When a domain is destroyed, xparams may not be available any more when
the block script is called to unconfigure the vnd.
Check xparam only at configure time, and just unconfigure any vnd present
in the xenstore.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/hotplug/NetBSD/block | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/tools/hotplug/NetBSD/block b/tools/hotplug/NetBSD/block
index 2a0516f436..c8b31a7b2b 100644
--- a/tools/hotplug/NetBSD/block
+++ b/tools/hotplug/NetBSD/block
@@ -20,37 +20,28 @@ error() {
 xpath=$1
 xstatus=$2
 xparams=$(xenstore-read "$xpath/params")
-if [ -b "$xparams" ]; then
-	xtype="phy"
-elif [ -f "$xparams" ]; then
-	xtype="file"
-elif [ -z "$xparams" ]; then
-	error "$xpath/params is empty, unable to attach block device."
-else
-	error "$xparams is not a valid file type to use as block device." \
-	      "Only block and regular image files accepted."
-fi
 
 case $xstatus in
 6)
 	# device removed
-	case $xtype in
-	file)
-		vnd=$(xenstore-read "$xpath/vnd" || echo none)
-		if [ $vnd != none ]; then
-			vnconfig -u $vnd
-		fi
-		;;
-	phy)
-		;;
-	*)
-		echo "unknown type $xtype" >&2
-		;;
-	esac
+	vnd=$(xenstore-read "$xpath/vnd" || echo none)
+	if [ $vnd != none ]; then
+		vnconfig -u $vnd
+	fi
 	xenstore-rm $xpath
 	exit 0
 	;;
 2)
+	if [ -b "$xparams" ]; then
+		xtype="phy"
+	elif [ -f "$xparams" ]; then
+		xtype="file"
+	elif [ -z "$xparams" ]; then
+		error "$xpath/params is empty, unable to attach block device."
+	else
+		error "$xparams is not a valid file type to use as block device." \
+		      "Only block and regular image files accepted."
+	fi
 	case $xtype in
 	file)
 		# Store the list of available vnd(4) devices in
-- 
2.29.2

Re: [PATCH v2] NetBSD hotplug: fix block unconfigure on destroy

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:47:49PM +0100, Manuel Bouyer wrote:
> When a domain is destroyed, xparams may not be available any more when
> the block script is called to unconfigure the vnd.
> Check xparam only at configure time, and just unconfigure any vnd present
> in the xenstore.

Can you paste the output of `xl -vvv destroy ...` when this happens?
I'm puzzled as to why the nodes would be removed before hotplug script
execution. Could this somehow be done by NetBSD blkback?

Thanks, Roger.

Re: [PATCH v2] NetBSD hotplug: fix block unconfigure on destroy

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 12:45:09PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 11:47:49PM +0100, Manuel Bouyer wrote:
> > When a domain is destroyed, xparams may not be available any more when
> > the block script is called to unconfigure the vnd.
> > Check xparam only at configure time, and just unconfigure any vnd present
> > in the xenstore.
> 
> Can you paste the output of `xl -vvv destroy ...` when this happens?
> I'm puzzled as to why the nodes would be removed before hotplug script
> execution. Could this somehow be done by NetBSD blkback?

I'll try. I've seen it mostly while running automated tests, so I'm
not sure I can reliably reproduce it.

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

[PATCH v2] NetBSD: use system-provided headers

[Manuel Bouyer]

On NetBSD use the system-provided headers for ioctl and related definitions,
they are up to date and have more chances to match the kernel's idea of
the ioctls and structures.
Remove now-unused NetBSD/evtchn.h and NetBSD/privcmd.h.
Don't fail install if xen/sys/*.h are not present.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/debugger/gdbsx/xg/xg_main.c      |   4 +
 tools/include/Makefile                 |   2 +
 tools/include/xen-sys/NetBSD/evtchn.h  |  86 --------------------
 tools/include/xen-sys/NetBSD/privcmd.h | 106 -------------------------
 tools/libs/call/private.h              |   4 +
 tools/libs/ctrl/xc_private.h           |   4 +
 tools/libs/foreignmemory/private.h     |   6 ++
 7 files changed, 20 insertions(+), 192 deletions(-)
 delete mode 100644 tools/include/xen-sys/NetBSD/evtchn.h
 delete mode 100644 tools/include/xen-sys/NetBSD/privcmd.h

diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index 4576c762af..903d60baed 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -49,7 +49,11 @@
 #include "xg_public.h"
 #include <xen/version.h>
 #include <xen/domctl.h>
+#ifdef __NetBSD__
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 #include <xen/foreign/x86_32.h>
 #include <xen/foreign/x86_64.h>
 
diff --git a/tools/include/Makefile b/tools/include/Makefile
index 4d4ec5f974..04902397b7 100644
--- a/tools/include/Makefile
+++ b/tools/include/Makefile
@@ -68,7 +68,9 @@ install: all
 	$(INSTALL_DATA) xen/foreign/*.h $(DESTDIR)$(includedir)/xen/foreign
 	$(INSTALL_DATA) xen/hvm/*.h $(DESTDIR)$(includedir)/xen/hvm
 	$(INSTALL_DATA) xen/io/*.h $(DESTDIR)$(includedir)/xen/io
+ifeq ($(wildcard xen/sys/.),)
 	$(INSTALL_DATA) xen/sys/*.h $(DESTDIR)$(includedir)/xen/sys
+endif
 	$(INSTALL_DATA) xen/xsm/*.h $(DESTDIR)$(includedir)/xen/xsm
 
 .PHONY: uninstall
diff --git a/tools/include/xen-sys/NetBSD/evtchn.h b/tools/include/xen-sys/NetBSD/evtchn.h
deleted file mode 100644
index 2d8a1f9164..0000000000
--- a/tools/include/xen-sys/NetBSD/evtchn.h
+++ /dev/null
@@ -1,86 +0,0 @@
-/* $NetBSD: evtchn.h,v 1.1.1.1 2007/06/14 19:39:45 bouyer Exp $ */
-/******************************************************************************
- * evtchn.h
- * 
- * Interface to /dev/xen/evtchn.
- * 
- * Copyright (c) 2003-2005, K A Fraser
- * 
- * This file may be 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_EVTCHN_H__
-#define __NetBSD_EVTCHN_H__
-
-/*
- * Bind a fresh port to VIRQ @virq.
- */
-#define IOCTL_EVTCHN_BIND_VIRQ				\
-	_IOWR('E', 4, struct ioctl_evtchn_bind_virq)
-struct ioctl_evtchn_bind_virq {
-	unsigned int virq;
-	unsigned int port;
-};
-
-/*
- * Bind a fresh port to remote <@remote_domain, @remote_port>.
- */
-#define IOCTL_EVTCHN_BIND_INTERDOMAIN			\
-	_IOWR('E', 5, struct ioctl_evtchn_bind_interdomain)
-struct ioctl_evtchn_bind_interdomain {
-	unsigned int remote_domain, remote_port;
-	unsigned int port;
-};
-
-/*
- * Allocate a fresh port for binding to @remote_domain.
- */
-#define IOCTL_EVTCHN_BIND_UNBOUND_PORT			\
-	_IOWR('E', 6, struct ioctl_evtchn_bind_unbound_port)
-struct ioctl_evtchn_bind_unbound_port {
-	unsigned int remote_domain;
-	unsigned int port;
-};
-
-/*
- * Unbind previously allocated @port.
- */
-#define IOCTL_EVTCHN_UNBIND				\
-	_IOW('E', 7, struct ioctl_evtchn_unbind)
-struct ioctl_evtchn_unbind {
-	unsigned int port;
-};
-
-/*
- * Send event to previously allocated @port.
- */
-#define IOCTL_EVTCHN_NOTIFY				\
-	_IOW('E', 8, struct ioctl_evtchn_notify)
-struct ioctl_evtchn_notify {
-	unsigned int port;
-};
-
-/* Clear and reinitialise the event buffer. Clear error condition. */
-#define IOCTL_EVTCHN_RESET				\
-	_IO('E', 9)
-
-#endif /* __NetBSD_EVTCHN_H__ */
diff --git a/tools/include/xen-sys/NetBSD/privcmd.h b/tools/include/xen-sys/NetBSD/privcmd.h
deleted file mode 100644
index 555bad973e..0000000000
--- a/tools/include/xen-sys/NetBSD/privcmd.h
+++ /dev/null
@@ -1,106 +0,0 @@
-/*	NetBSD: xenio.h,v 1.3 2005/05/24 12:07:12 yamt Exp $	*/
-
-/******************************************************************************
- * privcmd.h
- * 
- * Copyright (c) 2003-2004, K A Fraser
- * 
- * This file may be 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_PRIVCMD_H__
-#define __NetBSD_PRIVCMD_H__
-
-/* Interface to /dev/xen/privcmd */
-
-typedef struct privcmd_hypercall
-{
-    unsigned long op;
-    unsigned long arg[5];
-    long retval;
-} privcmd_hypercall_t;
-
-typedef struct privcmd_mmap_entry {
-    unsigned long va;
-    unsigned long mfn;
-    unsigned long npages;
-} privcmd_mmap_entry_t; 
-
-typedef struct privcmd_mmap {
-    int num;
-    domid_t dom; /* target domain */
-    privcmd_mmap_entry_t *entry;
-} privcmd_mmap_t; 
-
-typedef struct privcmd_mmapbatch {
-    int num;     /* number of pages to populate */
-    domid_t dom; /* target domain */
-    unsigned long addr;  /* virtual address */
-    unsigned long *arr; /* array of mfns - top nibble set on err */
-} privcmd_mmapbatch_t; 
-
-typedef struct privcmd_blkmsg
-{
-    unsigned long op;
-    void         *buf;
-    int           buf_size;
-} privcmd_blkmsg_t;
-
-/*
- * @cmd: IOCTL_PRIVCMD_HYPERCALL
- * @arg: &privcmd_hypercall_t
- * Return: Value returned from execution of the specified hypercall.
- */
-#define IOCTL_PRIVCMD_HYPERCALL         \
-    _IOWR('P', 0, privcmd_hypercall_t)
-
-#if defined(_KERNEL)
-/* compat */
-#define IOCTL_PRIVCMD_INITDOMAIN_EVTCHN_OLD \
-    _IO('P', 1)
-#endif /* defined(_KERNEL) */
-    
-#define IOCTL_PRIVCMD_MMAP             \
-    _IOW('P', 2, privcmd_mmap_t)
-#define IOCTL_PRIVCMD_MMAPBATCH        \
-    _IOW('P', 3, privcmd_mmapbatch_t)
-#define IOCTL_PRIVCMD_GET_MACH2PHYS_START_MFN \
-    _IOR('P', 4, unsigned long)
-
-/*
- * @cmd: IOCTL_PRIVCMD_INITDOMAIN_EVTCHN
- * @arg: n/a
- * Return: Port associated with domain-controller end of control event channel
- *         for the initial domain.
- */
-#define IOCTL_PRIVCMD_INITDOMAIN_EVTCHN \
-    _IOR('P', 5, int)
-
-/* Interface to /dev/xenevt */
-/* EVTCHN_RESET: Clear and reinit the event buffer. Clear error condition. */
-#define EVTCHN_RESET  _IO('E', 1)
-/* EVTCHN_BIND: Bind to the specified event-channel port. */
-#define EVTCHN_BIND   _IOW('E', 2, unsigned long)
-/* EVTCHN_UNBIND: Unbind from the specified event-channel port. */
-#define EVTCHN_UNBIND _IOW('E', 3, unsigned long)
-
-#endif /* __NetBSD_PRIVCMD_H__ */
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 7944ac5baf..96922e03d5 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -7,7 +7,11 @@
 #include <xencall.h>
 
 #include <xen/xen.h>
+#ifdef __NetBSD__
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 
 #ifndef PAGE_SHIFT /* Mini-os, Yukk */
 #define PAGE_SHIFT           12
diff --git a/tools/libs/ctrl/xc_private.h b/tools/libs/ctrl/xc_private.h
index f0b5f83ac8..68e388f488 100644
--- a/tools/libs/ctrl/xc_private.h
+++ b/tools/libs/ctrl/xc_private.h
@@ -39,7 +39,11 @@
 #include <xenforeignmemory.h>
 #include <xendevicemodel.h>
 
+#ifdef __NetBSD__
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 
 #include <xen-tools/libs.h>
 
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 7e734ac61e..39f8e0b869 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -8,7 +8,13 @@
 #include <xentoolcore_internal.h>
 
 #include <xen/xen.h>
+
+#ifdef __NetBSD__
+#include <xen/xen.h>
+#include <xen/xenio.h>
+#else
 #include <xen/sys/privcmd.h>
+#endif
 
 #ifndef PAGE_SHIFT /* Mini-os, Yukk */
 #define PAGE_SHIFT           12
-- 
2.29.2

Re: [PATCH v2] NetBSD: use system-provided headers

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:47:50PM +0100, Manuel Bouyer wrote:
> On NetBSD use the system-provided headers for ioctl and related definitions,
> they are up to date and have more chances to match the kernel's idea of
> the ioctls and structures.
> Remove now-unused NetBSD/evtchn.h and NetBSD/privcmd.h.
> Don't fail install if xen/sys/*.h are not present.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> ---
>  tools/debugger/gdbsx/xg/xg_main.c      |   4 +
>  tools/include/Makefile                 |   2 +
>  tools/include/xen-sys/NetBSD/evtchn.h  |  86 --------------------
>  tools/include/xen-sys/NetBSD/privcmd.h | 106 -------------------------
>  tools/libs/call/private.h              |   4 +
>  tools/libs/ctrl/xc_private.h           |   4 +
>  tools/libs/foreignmemory/private.h     |   6 ++
>  7 files changed, 20 insertions(+), 192 deletions(-)
>  delete mode 100644 tools/include/xen-sys/NetBSD/evtchn.h
>  delete mode 100644 tools/include/xen-sys/NetBSD/privcmd.h
> 
> diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
> index 4576c762af..903d60baed 100644
> --- a/tools/debugger/gdbsx/xg/xg_main.c
> +++ b/tools/debugger/gdbsx/xg/xg_main.c
> @@ -49,7 +49,11 @@
>  #include "xg_public.h"
>  #include <xen/version.h>
>  #include <xen/domctl.h>
> +#ifdef __NetBSD__
> +#include <xen/xenio.h>
> +#else
>  #include <xen/sys/privcmd.h>
> +#endif
>  #include <xen/foreign/x86_32.h>
>  #include <xen/foreign/x86_64.h>
>  
> diff --git a/tools/include/Makefile b/tools/include/Makefile
> index 4d4ec5f974..04902397b7 100644
> --- a/tools/include/Makefile
> +++ b/tools/include/Makefile
> @@ -68,7 +68,9 @@ install: all
>  	$(INSTALL_DATA) xen/foreign/*.h $(DESTDIR)$(includedir)/xen/foreign
>  	$(INSTALL_DATA) xen/hvm/*.h $(DESTDIR)$(includedir)/xen/hvm
>  	$(INSTALL_DATA) xen/io/*.h $(DESTDIR)$(includedir)/xen/io
> +ifeq ($(wildcard xen/sys/.),)

Maybe I'm confused, but doesn't this need to be ifneq? So that if
wildcard returns any output the command is executed?

I would also use $(wildcard xen/sys/*.h) because that matches exactly
the pattern used below.

Thanks, Roger.

Re: [PATCH v2] NetBSD: use system-provided headers

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 12:21:33PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 11:47:50PM +0100, Manuel Bouyer wrote:
> > On NetBSD use the system-provided headers for ioctl and related definitions,
> > they are up to date and have more chances to match the kernel's idea of
> > the ioctls and structures.
> > Remove now-unused NetBSD/evtchn.h and NetBSD/privcmd.h.
> > Don't fail install if xen/sys/*.h are not present.
> > 
> > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > ---
> >  tools/debugger/gdbsx/xg/xg_main.c      |   4 +
> >  tools/include/Makefile                 |   2 +
> >  tools/include/xen-sys/NetBSD/evtchn.h  |  86 --------------------
> >  tools/include/xen-sys/NetBSD/privcmd.h | 106 -------------------------
> >  tools/libs/call/private.h              |   4 +
> >  tools/libs/ctrl/xc_private.h           |   4 +
> >  tools/libs/foreignmemory/private.h     |   6 ++
> >  7 files changed, 20 insertions(+), 192 deletions(-)
> >  delete mode 100644 tools/include/xen-sys/NetBSD/evtchn.h
> >  delete mode 100644 tools/include/xen-sys/NetBSD/privcmd.h
> > 
> > diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
> > index 4576c762af..903d60baed 100644
> > --- a/tools/debugger/gdbsx/xg/xg_main.c
> > +++ b/tools/debugger/gdbsx/xg/xg_main.c
> > @@ -49,7 +49,11 @@
> >  #include "xg_public.h"
> >  #include <xen/version.h>
> >  #include <xen/domctl.h>
> > +#ifdef __NetBSD__
> > +#include <xen/xenio.h>
> > +#else
> >  #include <xen/sys/privcmd.h>
> > +#endif
> >  #include <xen/foreign/x86_32.h>
> >  #include <xen/foreign/x86_64.h>
> >  
> > diff --git a/tools/include/Makefile b/tools/include/Makefile
> > index 4d4ec5f974..04902397b7 100644
> > --- a/tools/include/Makefile
> > +++ b/tools/include/Makefile
> > @@ -68,7 +68,9 @@ install: all
> >  	$(INSTALL_DATA) xen/foreign/*.h $(DESTDIR)$(includedir)/xen/foreign
> >  	$(INSTALL_DATA) xen/hvm/*.h $(DESTDIR)$(includedir)/xen/hvm
> >  	$(INSTALL_DATA) xen/io/*.h $(DESTDIR)$(includedir)/xen/io
> > +ifeq ($(wildcard xen/sys/.),)
> 
> Maybe I'm confused, but doesn't this need to be ifneq? So that if
> wildcard returns any output the command is executed?
> 
> I would also use $(wildcard xen/sys/*.h) because that matches exactly
> the pattern used below.

You're right. It's strange that I didn't get an error at install time.
Will send a fixed version

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

[PATCH v2] libs/call: fix build on NetBSD

[Manuel Bouyer]

Define PAGE_* if not already defined
Catch up with osdep interface change.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libs/call/netbsd.c  | 19 +++++++++++--------
 tools/libs/call/private.h |  4 +++-
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/tools/libs/call/netbsd.c b/tools/libs/call/netbsd.c
index a5502da377..4dcc2919ba 100644
--- a/tools/libs/call/netbsd.c
+++ b/tools/libs/call/netbsd.c
@@ -19,12 +19,15 @@
  * Split from xc_netbsd.c
  */
 
-#include "xc_private.h"
 
 #include <unistd.h>
 #include <fcntl.h>
 #include <malloc.h>
+#include <errno.h>
 #include <sys/mman.h>
+#include <sys/ioctl.h>
+
+#include "private.h"
 
 int osdep_xencall_open(xencall_handle *xcall)
 {
@@ -69,12 +72,13 @@ int osdep_xencall_close(xencall_handle *xcall)
     return close(fd);
 }
 
-void *osdep_alloc_hypercall_buffer(xencall_handle *xcall, size_t npages)
+void *osdep_alloc_pages(xencall_handle *xcall, size_t npages)
 {
-    size_t size = npages * XC_PAGE_SIZE;
+    size_t size = npages * PAGE_SIZE;
     void *p;
+    int ret;
 
-    ret = posix_memalign(&p, XC_PAGE_SIZE, size);
+    ret = posix_memalign(&p, PAGE_SIZE, size);
     if ( ret != 0 || !p )
         return NULL;
 
@@ -86,14 +90,13 @@ void *osdep_alloc_hypercall_buffer(xencall_handle *xcall, size_t npages)
     return p;
 }
 
-void osdep_free_hypercall_buffer(xencall_handle *xcall, void *ptr,
-                                 size_t npages)
+void osdep_free_pages(xencall_handle *xcall, void *ptr, size_t npages)
 {
-    (void) munlock(ptr, npages * XC_PAGE_SIZE);
+    munlock(ptr, npages * PAGE_SIZE);
     free(ptr);
 }
 
-int do_xen_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
+int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
 {
     int fd = xcall->fd;
     int error = ioctl(fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
index 21f992b37e..7944ac5baf 100644
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -12,8 +12,10 @@
 #ifndef PAGE_SHIFT /* Mini-os, Yukk */
 #define PAGE_SHIFT           12
 #endif
-#ifndef __MINIOS__ /* Yukk */
+#ifndef PAGE_SIZE
 #define PAGE_SIZE            (1UL << PAGE_SHIFT)
+#endif
+#ifndef PAGE_MASK
 #define PAGE_MASK            (~(PAGE_SIZE-1))
 #endif
 
-- 
2.29.2

[PATCH v2] libs/foreignmemory: Implement on NetBSD

[Manuel Bouyer]

Implement foreignmemory interface on NetBSD. The compat interface is now used
only on __sun__

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/foreignmemory/Makefile  |  2 +-
 tools/libs/foreignmemory/netbsd.c  | 66 +++++++++++++++++++++++++-----
 tools/libs/foreignmemory/private.h |  6 +--
 3 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
index 13850f7988..f191cdbed0 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -8,7 +8,7 @@ SRCS-y                 += core.c
 SRCS-$(CONFIG_Linux)   += linux.c
 SRCS-$(CONFIG_FreeBSD) += freebsd.c
 SRCS-$(CONFIG_SunOS)   += compat.c solaris.c
-SRCS-$(CONFIG_NetBSD)  += compat.c netbsd.c
+SRCS-$(CONFIG_NetBSD)  += netbsd.c
 SRCS-$(CONFIG_MiniOS)  += minios.c
 
 include $(XEN_ROOT)/tools/libs/libs.mk
diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
index 54a418ebd6..a7e1d72ffc 100644
--- a/tools/libs/foreignmemory/netbsd.c
+++ b/tools/libs/foreignmemory/netbsd.c
@@ -19,7 +19,9 @@
 
 #include <unistd.h>
 #include <fcntl.h>
+#include <errno.h>
 #include <sys/mman.h>
+#include <sys/ioctl.h>
 
 #include "private.h"
 
@@ -66,15 +68,17 @@ int osdep_xenforeignmemory_close(xenforeignmemory_handle *fmem)
     return close(fd);
 }
 
-void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
-                              void *addr, int prot, int flags,
-                              xen_pfn_t *arr, int num)
+void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
+                                 uint32_t dom, void *addr,
+                                 int prot, int flags, size_t num,
+                                 const xen_pfn_t arr[/*num*/], int err[/*num*/])
+
 {
     int fd = fmem->fd;
-    privcmd_mmapbatch_t ioctlx;
-    addr = mmap(addr, num*XC_PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
+    privcmd_mmapbatch_v2_t ioctlx;
+    addr = mmap(addr, num*PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
     if ( addr == MAP_FAILED ) {
-        PERROR("osdep_map_foreign_batch: mmap failed");
+        PERROR("osdep_xenforeignmemory_map: mmap failed");
         return NULL;
     }
 
@@ -82,11 +86,12 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
     ioctlx.dom=dom;
     ioctlx.addr=(unsigned long)addr;
     ioctlx.arr=arr;
-    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx) < 0 )
+    ioctlx.err=err;
+    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, &ioctlx) < 0 )
     {
         int saved_errno = errno;
-        PERROR("osdep_map_foreign_batch: ioctl failed");
-        (void)munmap(addr, num*XC_PAGE_SIZE);
+        PERROR("osdep_xenforeignmemory_map: ioctl failed");
+        (void)munmap(addr, num*PAGE_SIZE);
         errno = saved_errno;
         return NULL;
     }
@@ -97,7 +102,48 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num)
 {
-    return munmap(addr, num*XC_PAGE_SIZE);
+    return munmap(addr, num*PAGE_SIZE);
+}
+
+int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
+                                    domid_t domid)
+{
+    errno = EOPNOTSUPP;
+    return -1;
+}
+
+int osdep_xenforeignmemory_unmap_resource(
+    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+}
+
+int osdep_xenforeignmemory_map_resource(
+    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
+{
+    privcmd_mmap_resource_t mr = {
+        .dom = fres->domid,
+        .type = fres->type,
+        .id = fres->id,
+        .idx = fres->frame,
+        .num = fres->nr_frames,
+    };
+    int rc;
+
+    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+                      fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
+    if ( fres->addr == MAP_FAILED )
+        return -1;
+
+    mr.addr = (uintptr_t)fres->addr;
+
+    rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
+    if ( rc )
+    {
+        PERROR("ioctl failed");
+    }
+
+    return 0;
 }
 
 /*
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index ebd45c4785..7e734ac61e 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -36,9 +36,9 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
                                  void *addr, size_t num);
 
-#if defined(__NetBSD__) || defined(__sun__)
+#if defined(__sun__)
 /* Strictly compat for those two only only */
-void *compat_mapforeign_batch(xenforeignmem_handle *fmem, uint32_t dom,
+void *osdep_map_foreign_batch(xenforeignmemory_handle *fmem, uint32_t dom,
                               void *addr, int prot, int flags,
                               xen_pfn_t *arr, int num);
 #endif
@@ -54,7 +54,7 @@ struct xenforeignmemory_resource_handle {
     int flags;
 };
 
-#if !defined(__linux__) && !defined(__FreeBSD__)
+#ifdef __sun__
 static inline int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
                                                   domid_t domid)
 {
-- 
2.29.2

Re: [PATCH v2] libs/foreignmemory: Implement on NetBSD

[Andrew Cooper]

On 26/01/2021 22:47, Manuel Bouyer wrote:
> Implement foreignmemory interface on NetBSD. The compat interface is now used
> only on __sun__
>
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Just as a note.  I've also got a bugfix for this library (as well as
every other level of the stack), which will need an incremental change
in osdep_xenforeignmemory_map_resource().

See
https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=3768453802baece85140e579814af9c27f70d99a
for the details, but you may also need to bugfix the kernel side of the
IOCTL if you borrowed the FreeBSD copy to begin with.

What is probably easiest is for this patch to be committed first, then I
will rework the resource size fix() to make the same delta to NetBSD's
copy as Linux/FreeBSD.

~Andrew

Re: [PATCH v2] libs/foreignmemory: Implement on NetBSD

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:47:52PM +0100, Manuel Bouyer wrote:
> Implement foreignmemory interface on NetBSD. The compat interface is now used
> only on __sun__
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> ---
>  tools/libs/foreignmemory/Makefile  |  2 +-
>  tools/libs/foreignmemory/netbsd.c  | 66 +++++++++++++++++++++++++-----
>  tools/libs/foreignmemory/private.h |  6 +--
>  3 files changed, 60 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
> index 13850f7988..f191cdbed0 100644
> --- a/tools/libs/foreignmemory/Makefile
> +++ b/tools/libs/foreignmemory/Makefile
> @@ -8,7 +8,7 @@ SRCS-y                 += core.c
>  SRCS-$(CONFIG_Linux)   += linux.c
>  SRCS-$(CONFIG_FreeBSD) += freebsd.c
>  SRCS-$(CONFIG_SunOS)   += compat.c solaris.c
> -SRCS-$(CONFIG_NetBSD)  += compat.c netbsd.c
> +SRCS-$(CONFIG_NetBSD)  += netbsd.c
>  SRCS-$(CONFIG_MiniOS)  += minios.c
>  
>  include $(XEN_ROOT)/tools/libs/libs.mk
> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
> index 54a418ebd6..a7e1d72ffc 100644
> --- a/tools/libs/foreignmemory/netbsd.c
> +++ b/tools/libs/foreignmemory/netbsd.c
> @@ -19,7 +19,9 @@
>  
>  #include <unistd.h>
>  #include <fcntl.h>
> +#include <errno.h>
>  #include <sys/mman.h>
> +#include <sys/ioctl.h>
>  
>  #include "private.h"
>  
> @@ -66,15 +68,17 @@ int osdep_xenforeignmemory_close(xenforeignmemory_handle *fmem)
>      return close(fd);
>  }
>  
> -void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
> -                              void *addr, int prot, int flags,
> -                              xen_pfn_t *arr, int num)
> +void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
> +                                 uint32_t dom, void *addr,
> +                                 int prot, int flags, size_t num,
> +                                 const xen_pfn_t arr[/*num*/], int err[/*num*/])
> +
>  {
>      int fd = fmem->fd;
> -    privcmd_mmapbatch_t ioctlx;
> -    addr = mmap(addr, num*XC_PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
> +    privcmd_mmapbatch_v2_t ioctlx;
> +    addr = mmap(addr, num*PAGE_SIZE, prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
>      if ( addr == MAP_FAILED ) {
> -        PERROR("osdep_map_foreign_batch: mmap failed");
> +        PERROR("osdep_xenforeignmemory_map: mmap failed");
>          return NULL;
>      }
>  
> @@ -82,11 +86,12 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
>      ioctlx.dom=dom;
>      ioctlx.addr=(unsigned long)addr;
>      ioctlx.arr=arr;
> -    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, &ioctlx) < 0 )
> +    ioctlx.err=err;
> +    if ( ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, &ioctlx) < 0 )
>      {
>          int saved_errno = errno;
> -        PERROR("osdep_map_foreign_batch: ioctl failed");
> -        (void)munmap(addr, num*XC_PAGE_SIZE);
> +        PERROR("osdep_xenforeignmemory_map: ioctl failed");
> +        (void)munmap(addr, num*PAGE_SIZE);
>          errno = saved_errno;
>          return NULL;
>      }
> @@ -97,7 +102,48 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
>  int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
>                                   void *addr, size_t num)
>  {
> -    return munmap(addr, num*XC_PAGE_SIZE);
> +    return munmap(addr, num*PAGE_SIZE);
> +}
> +
> +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
> +                                    domid_t domid)
> +{
> +    errno = EOPNOTSUPP;
> +    return -1;
> +}
> +
> +int osdep_xenforeignmemory_unmap_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
> +}
> +
> +int osdep_xenforeignmemory_map_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    privcmd_mmap_resource_t mr = {
> +        .dom = fres->domid,
> +        .type = fres->type,
> +        .id = fres->id,
> +        .idx = fres->frame,
> +        .num = fres->nr_frames,
> +    };
> +    int rc;
> +
> +    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
> +                      fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
> +    if ( fres->addr == MAP_FAILED )
> +        return -1;
> +
> +    mr.addr = (uintptr_t)fres->addr;
> +
> +    rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
> +    if ( rc )
> +    {
> +        PERROR("ioctl failed");
> +    }
> +
> +    return 0;

You need to return rc here, or else the failure won't be propagated to
the caller.

If you agree I don't think I have further comments:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH v2] libs/foreignmemory: Implement on NetBSD

[Andrew Cooper]

On 26/01/2021 22:47, Manuel Bouyer wrote:
> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
> index 54a418ebd6..a7e1d72ffc 100644
> --- a/tools/libs/foreignmemory/netbsd.c
> +++ b/tools/libs/foreignmemory/netbsd.c
> @@ -97,7 +102,48 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
>  int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
>                                   void *addr, size_t num)
>  {
> -    return munmap(addr, num*XC_PAGE_SIZE);
> +    return munmap(addr, num*PAGE_SIZE);
> +}
> +
> +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
> +                                    domid_t domid)
> +{
> +    errno = EOPNOTSUPP;
> +    return -1;
> +}
> +
> +int osdep_xenforeignmemory_unmap_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
> +}
> +
> +int osdep_xenforeignmemory_map_resource(
> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
> +{
> +    privcmd_mmap_resource_t mr = {
> +        .dom = fres->domid,
> +        .type = fres->type,
> +        .id = fres->id,
> +        .idx = fres->frame,
> +        .num = fres->nr_frames,
> +    };
> +    int rc;
> +
> +    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
> +                      fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
> +    if ( fres->addr == MAP_FAILED )
> +        return -1;
> +
> +    mr.addr = (uintptr_t)fres->addr;
> +
> +    rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
> +    if ( rc )
> +    {
> +        PERROR("ioctl failed");

return -1;

I was rebasing my resource_size fix over this patch.

It would be easiest for me if I fix up and commit this patch, if
everyone is happy with that.

~Andrew

Re: [PATCH v2] libs/foreignmemory: Implement on NetBSD

[Andrew Cooper]

On 28/01/2021 10:52, Andrew Cooper wrote:
> On 26/01/2021 22:47, Manuel Bouyer wrote:
>> diff --git a/tools/libs/foreignmemory/netbsd.c b/tools/libs/foreignmemory/netbsd.c
>> index 54a418ebd6..a7e1d72ffc 100644
>> --- a/tools/libs/foreignmemory/netbsd.c
>> +++ b/tools/libs/foreignmemory/netbsd.c
>> @@ -97,7 +102,48 @@ void *osdep_map_foreign_batch(xenforeignmem_handle *fmem, uint32_t dom,
>>  int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
>>                                   void *addr, size_t num)
>>  {
>> -    return munmap(addr, num*XC_PAGE_SIZE);
>> +    return munmap(addr, num*PAGE_SIZE);
>> +}
>> +
>> +int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
>> +                                    domid_t domid)
>> +{
>> +    errno = EOPNOTSUPP;
>> +    return -1;
>> +}
>> +
>> +int osdep_xenforeignmemory_unmap_resource(
>> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
>> +{
>> +    return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
>> +}
>> +
>> +int osdep_xenforeignmemory_map_resource(
>> +    xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
>> +{
>> +    privcmd_mmap_resource_t mr = {
>> +        .dom = fres->domid,
>> +        .type = fres->type,
>> +        .id = fres->id,
>> +        .idx = fres->frame,
>> +        .num = fres->nr_frames,
>> +    };
>> +    int rc;
>> +
>> +    fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
>> +                      fres->prot, fres->flags | MAP_ANON | MAP_SHARED, -1, 0);
>> +    if ( fres->addr == MAP_FAILED )
>> +        return -1;
>> +
>> +    mr.addr = (uintptr_t)fres->addr;
>> +
>> +    rc = ioctl(fmem->fd, IOCTL_PRIVCMD_MMAP_RESOURCE, &mr);
>> +    if ( rc )
>> +    {
>> +        PERROR("ioctl failed");
> return -1;
>
> I was rebasing my resource_size fix over this patch.
>
> It would be easiest for me if I fix up and commit this patch, if
> everyone is happy with that.

FAOD I've committed a fixed up version of this patch as discussed.

~Andrew

Re: [PATCH v2] libs/foreignmemory: Implement on NetBSD

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 11:42:45AM +0000, Andrew Cooper wrote:
> FAOD I've committed a fixed up version of this patch as discussed.

thanks !

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

[PATCH v2] libs/gnttab: implement on NetBSD

[Manuel Bouyer]

Implement gnttab interface on NetBSD.
The kernel interface is different from FreeBSD so we can't use the FreeBSD
version

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/gnttab/Makefile |   2 +-
 tools/libs/gnttab/netbsd.c | 267 +++++++++++++++++++++++++++++++++++++
 2 files changed, 268 insertions(+), 1 deletion(-)
 create mode 100644 tools/libs/gnttab/netbsd.c

diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
index d86c49d243..ae390ce60f 100644
--- a/tools/libs/gnttab/Makefile
+++ b/tools/libs/gnttab/Makefile
@@ -10,7 +10,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) += $(SRCS-GNTTAB) $(SRCS-GNTSHR) freebsd.c
+SRCS-$(CONFIG_NetBSD)  += $(SRCS-GNTTAB) $(SRCS-GNTSHR) netbsd.c
 SRCS-$(CONFIG_SunOS)   += gnttab_unimp.c gntshr_unimp.c
-SRCS-$(CONFIG_NetBSD)  += gnttab_unimp.c gntshr_unimp.c
 
 include $(XEN_ROOT)/tools/libs/libs.mk
diff --git a/tools/libs/gnttab/netbsd.c b/tools/libs/gnttab/netbsd.c
new file mode 100644
index 0000000000..272cbd8961
--- /dev/null
+++ b/tools/libs/gnttab/netbsd.c
@@ -0,0 +1,267 @@
+/*
+ * Copyright (c) 2007-2008, D G Murray <Derek.Murray@cl.cam.ac.uk>
+ * Copyright (c) 2016-2017, Akshay Jaggi <jaggi@FreeBSD.org>
+ *
+ * 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/xen.h>
+#include <xen/xenio.h>
+
+#include "private.h"
+
+#define PAGE_SHIFT           12
+#define PAGE_SIZE            (1UL << PAGE_SHIFT)
+#define PAGE_MASK            (~(PAGE_SIZE-1))
+
+#define DEVXEN "/kern/xen/privcmd"
+
+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)
+{
+    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_mmap_grant_ref map;
+    void *addr = NULL;
+    int domids_stride;
+    unsigned int refs_size = count * sizeof(struct ioctl_gntdev_grant_ref);
+    int rv;
+
+    domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
+    map.refs = malloc(refs_size);
+
+    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, count * PAGE_SIZE,
+        prot, flags | MAP_ANON | MAP_SHARED, -1, 0);
+
+    if (map.va == MAP_FAILED) {
+        GTERROR(xgt->logger, "osdep_gnttab_grant_map: mmap failed");
+        munmap((void *)map.va, count * PAGE_SIZE);
+        addr = MAP_FAILED;
+    }
+    map.va = addr;
+
+    map.notify.offset = 0;
+    map.notify.action = 0;
+    if ( notify_offset < PAGE_SIZE * count )
+    {
+        map.notify.offset = notify_offset;
+        map.notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
+    }
+    if ( notify_port != -1 )
+    {
+       map.notify.event_channel_port = notify_port;
+       map.notify.action |= UNMAP_NOTIFY_SEND_EVENT;
+    }
+
+    rv = ioctl(fd, IOCTL_GNTDEV_MMAP_GRANT_REF, &map);
+    if ( rv )
+    {
+        GTERROR(xgt->logger,
+            "ioctl IOCTL_GNTDEV_MMAP_GRANT_REF failed: %d", rv);
+        munmap(addr, count * PAGE_SIZE);
+        addr = MAP_FAILED;
+    }
+    free(map.refs);
+    return addr;
+}
+
+int osdep_gnttab_unmap(xengnttab_handle *xgt,
+                       void *start_address,
+                       uint32_t count)
+{
+    int rc;
+    if ( start_address == NULL )
+    {
+        errno = EINVAL;
+        return -1;
+    }
+
+    /* Next, unmap the memory. */
+    rc = munmap(start_address, count * PAGE_SIZE);
+
+    return rc;
+}
+
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+                            uint32_t count,
+                            xengnttab_grant_copy_segment_t *segs)
+{
+    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)
+{
+    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_alloc_grant_ref alloc;
+
+    alloc.gref_ids = malloc(count * sizeof(uint32_t));
+    if ( alloc.gref_ids == NULL )
+        return NULL;
+    alloc.domid = domid;
+    alloc.flags = writable ? GNTDEV_ALLOC_FLAG_WRITABLE : 0;
+    alloc.count = count;
+    area = mmap(NULL, count * PAGE_SIZE,
+        PROT_READ | PROT_WRITE, MAP_ANON | MAP_SHARED, -1, 0);
+
+    if (area == MAP_FAILED) {
+        GTERROR(xgs->logger, "osdep_gnttab_grant_map: mmap failed");
+        area = MAP_FAILED;
+        goto out;
+    }
+    alloc.va = area;
+
+    alloc.notify.offset = 0;
+    alloc.notify.action = 0;
+    if ( notify_offset < PAGE_SIZE * count )
+    {
+        alloc.notify.offset = notify_offset;
+        alloc.notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
+    }
+    if ( notify_port != -1 )
+    {
+       alloc.notify.event_channel_port = notify_port;
+       alloc.notify.action |= UNMAP_NOTIFY_SEND_EVENT;
+    }
+    err = ioctl(fd, IOCTL_GNTDEV_ALLOC_GRANT_REF, &alloc);
+    if ( err )
+    {
+        GSERROR(xgs->logger, "IOCTL_GNTDEV_ALLOC_GRANT_REF failed");
+        munmap(area, count * PAGE_SIZE);
+        area = MAP_FAILED;
+        goto out;
+    }
+    memcpy(refs, alloc.gref_ids, count * sizeof(uint32_t));
+
+ out:
+    free(alloc.gref_ids);
+    return area;
+}
+
+int osdep_gntshr_unshare(xengntshr_handle *xgs,
+                         void *start_address, uint32_t count)
+{
+    return munmap(start_address, count * PAGE_SIZE);
+}
+
+/*
+ * The functions below are Linux-isms that will likely never be implemented
+ * on FreeBSD unless FreeBSD also implements something akin to Linux dmabuf.
+ */
+int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
+                                      uint32_t flags, uint32_t count,
+                                      const uint32_t *refs,
+                                      uint32_t *dmabuf_fd)
+{
+    abort();
+}
+
+int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
+                                          uint32_t fd, uint32_t wait_to_ms)
+{
+    abort();
+}
+
+int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
+                                    uint32_t fd, uint32_t count, uint32_t *refs)
+{
+    abort();
+}
+
+int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
+{
+    abort();
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.29.2

Re: [PATCH v2] libs/gnttab: implement on NetBSD

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:47:53PM +0100, Manuel Bouyer wrote:
> +int osdep_gntshr_unshare(xengntshr_handle *xgs,
> +                         void *start_address, uint32_t count)
> +{
> +    return munmap(start_address, count * PAGE_SIZE);
> +}
> +
> +/*
> + * The functions below are Linux-isms that will likely never be implemented
> + * on FreeBSD unless FreeBSD also implements something akin to Linux dmabuf.

You might want to s/FreeBSD/NetBSD/ in the line above.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger

[PATCH v2] libs/light: Switch NetBSD to QEMU_XEN

[Manuel Bouyer]

Switch NetBSD to QEMU_XEN.
All 3 versions of libxl__default_device_model() now return
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN, so remove it and just set
b_info->device_model_version to LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN in
libxl__domain_build_info_setdefault().

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libs/light/libxl_create.c   | 2 +-
 tools/libs/light/libxl_freebsd.c  | 5 -----
 tools/libs/light/libxl_internal.h | 2 --
 tools/libs/light/libxl_linux.c    | 5 -----
 tools/libs/light/libxl_netbsd.c   | 5 -----
 5 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 86f4a8369d..8616113e72 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -102,7 +102,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
                 b_info->device_model_version =
                     LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
             } else {
-                b_info->device_model_version = libxl__default_device_model(gc);
+                b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
             }
         } else {
             b_info->device_model_version =
diff --git a/tools/libs/light/libxl_freebsd.c b/tools/libs/light/libxl_freebsd.c
index f7ef4a8910..422c6b3b79 100644
--- a/tools/libs/light/libxl_freebsd.c
+++ b/tools/libs/light/libxl_freebsd.c
@@ -229,11 +229,6 @@ out:
     return rc;
 }
 
-libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
-{
-    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
-}
-
 int libxl__pci_numdevs(libxl__gc *gc)
 {
     return ERROR_NI;
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index c79523ba92..6c8b7d71a9 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2309,8 +2309,6 @@ _hidden char *libxl__json_object_to_json(libxl__gc *gc,
   /* Based on /local/domain/$domid/dm-version xenstore key
    * default is qemu xen traditional */
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
-  /* Return the system-wide default device model */
-_hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
 
 static inline
 bool libxl__stubdomain_is_linux_running(libxl__gc *gc, uint32_t domid)
diff --git a/tools/libs/light/libxl_linux.c b/tools/libs/light/libxl_linux.c
index 873b0271af..8d62dfd255 100644
--- a/tools/libs/light/libxl_linux.c
+++ b/tools/libs/light/libxl_linux.c
@@ -241,11 +241,6 @@ out:
     return rc;
 }
 
-libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
-{
-    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
-}
-
 int libxl__pci_numdevs(libxl__gc *gc)
 {
     DIR *dir;
diff --git a/tools/libs/light/libxl_netbsd.c b/tools/libs/light/libxl_netbsd.c
index e66a393d7f..6ad4ed34c2 100644
--- a/tools/libs/light/libxl_netbsd.c
+++ b/tools/libs/light/libxl_netbsd.c
@@ -108,11 +108,6 @@ out:
     return rc;
 }
 
-libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
-{
-    return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
-}
-
 int libxl__pci_numdevs(libxl__gc *gc)
 {
     return ERROR_NI;
-- 
2.29.2

[PATCH v2] libs/light: fix tv_sec printf format

[Manuel Bouyer]

Don't assume tv_sec is a unsigned long, it is 64 bits on NetBSD 32 bits.
Use %jd and cast to (intmax_t) instead

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libs/light/libxl_create.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 8616113e72..9848d65f36 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -496,7 +496,7 @@ int libxl__domain_build(libxl__gc *gc,
         vments[2] = "image/ostype";
         vments[3] = "hvm";
         vments[4] = "start_time";
-        vments[5] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[5] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
 
         localents = libxl__calloc(gc, 13, sizeof(char *));
         i = 0;
@@ -535,7 +535,7 @@ int libxl__domain_build(libxl__gc *gc,
         vments[i++] = "image/kernel";
         vments[i++] = (char *) state->pv_kernel.path;
         vments[i++] = "start_time";
-        vments[i++] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[i++] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
         if (state->pv_ramdisk.path) {
             vments[i++] = "image/ramdisk";
             vments[i++] = (char *) state->pv_ramdisk.path;
@@ -1502,7 +1502,7 @@ static void domcreate_stream_done(libxl__egc *egc,
         vments[2] = "image/ostype";
         vments[3] = "hvm";
         vments[4] = "start_time";
-        vments[5] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[5] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         vments = libxl__calloc(gc, 11, sizeof(char *));
@@ -1512,7 +1512,7 @@ static void domcreate_stream_done(libxl__egc *egc,
         vments[i++] = "image/kernel";
         vments[i++] = (char *) state->pv_kernel.path;
         vments[i++] = "start_time";
-        vments[i++] = GCSPRINTF("%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000);
+        vments[i++] = GCSPRINTF("%jd.%02d", (intmax_t)start_time.tv_sec,(int)start_time.tv_usec/10000);
         if (state->pv_ramdisk.path) {
             vments[i++] = "image/ramdisk";
             vments[i++] = (char *) state->pv_ramdisk.path;
-- 
2.29.2

[PATCH v2] libs/light: fix uuid on NetBSD

[Manuel Bouyer]

NetBSD uses the same uuid library as FreeBSD. As this is in a
__FreeBSD__ || __NetBSD__ block, just drop the #ifdef __FreeBSD__
and dead code.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libs/light/libxl_uuid.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/tools/libs/light/libxl_uuid.c b/tools/libs/light/libxl_uuid.c
index dadb79bad8..7b68270a33 100644
--- a/tools/libs/light/libxl_uuid.c
+++ b/tools/libs/light/libxl_uuid.c
@@ -82,7 +82,6 @@ void libxl_uuid_generate(libxl_uuid *uuid)
     uuid_enc_be(uuid->uuid, &nat_uuid);
 }
 
-#ifdef __FreeBSD__
 int libxl_uuid_from_string(libxl_uuid *uuid, const char *in)
 {
     uint32_t status;
@@ -95,19 +94,6 @@ int libxl_uuid_from_string(libxl_uuid *uuid, const char *in)
 
     return 0;
 }
-#else
-#define LIBXL__UUID_PTRS(uuid) &uuid[0], &uuid[1], &uuid[2], &uuid[3], \
-                               &uuid[4], &uuid[5], &uuid[6], &uuid[7], \
-                               &uuid[8], &uuid[9], &uuid[10],&uuid[11], \
-                               &uuid[12],&uuid[13],&uuid[14],&uuid[15]
-int libxl_uuid_from_string(libxl_uuid *uuid, const char *in)
-{
-    if ( sscanf(in, LIBXL_UUID_FMT, LIBXL__UUID_PTRS(uuid->uuid)) != sizeof(uuid->uuid) )
-        return -1;
-    return 0;
-}
-#undef LIBXL__UUID_PTRS
-#endif
 
 void libxl_uuid_copy(libxl_ctx *ctx_opt, libxl_uuid *dst,
                      const libxl_uuid *src)
@@ -120,7 +106,6 @@ void libxl_uuid_clear(libxl_uuid *uuid)
     memset(&uuid->uuid, 0, sizeof(uuid->uuid));
 }
 
-#ifdef __FreeBSD__
 int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
 {
     uuid_t nat_uuid1, nat_uuid2;
@@ -130,12 +115,6 @@ int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
 
     return uuid_compare(&nat_uuid1, &nat_uuid2, NULL);
 }
-#else
-int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
-{
-     return memcmp(uuid1->uuid, uuid2->uuid, sizeof(uuid1->uuid));
-}
-#endif
 
 const uint8_t *libxl_uuid_bytearray_const(const libxl_uuid *uuid)
 {
-- 
2.29.2

[PATCH v2] libs/light: make it build without setresuid()

[Manuel Bouyer]

NetBSD doesn't have setresuid(). introcuce libxl__setresuid(),
which on NetBSD assert() that it's never called (it should not be called when
dm restriction is off, and NetBSD doesn't support dm restriction at
this time).
On linux and FreeBSD it just calls setresuid().

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/light/Makefile          |  4 ++--
 tools/libs/light/libxl_dm.c        |  2 +-
 tools/libs/light/libxl_internal.h  |  3 +++
 tools/libs/light/libxl_netbsd.c    |  5 +++++
 tools/libs/light/libxl_setresuid.c | 23 +++++++++++++++++++++++
 5 files changed, 34 insertions(+), 3 deletions(-)
 create mode 100644 tools/libs/light/libxl_setresuid.c

diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index 68f6fa315f..d62ca6e477 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -64,8 +64,8 @@ SRCS-$(CONFIG_ARM) += libxl_arm_no_acpi.c
 endif
 
 SRCS-OS-$(CONFIG_NetBSD) = libxl_netbsd.c
-SRCS-OS-$(CONFIG_Linux) = libxl_linux.c
-SRCS-OS-$(CONFIG_FreeBSD) = libxl_freebsd.c
+SRCS-OS-$(CONFIG_Linux) = libxl_linux.c libxl_setresuid.c
+SRCS-OS-$(CONFIG_FreeBSD) = libxl_freebsd.c libxl_setresuid.c
 ifeq ($(SRCS-OS-y),)
 $(error Your Operating System is not supported by libxenlight, \
 please check libxl_linux.c and libxl_netbsd.c to see how to get it ported)
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 13f79ec471..291dee9b3f 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -3655,7 +3655,7 @@ static int kill_device_model_uid_child(libxl__destroy_devicemodel_state *ddms,
 
     LOGD(DEBUG, domid, "DM reaper: calling setresuid(%d, %d, 0)",
          reaper_uid, dm_kill_uid);
-    r = setresuid(reaper_uid, dm_kill_uid, 0);
+    r = libxl__setresuid(reaper_uid, dm_kill_uid, 0);
     if (r) {
         LOGED(ERROR, domid, "setresuid to (%d, %d, 0)",
               reaper_uid, dm_kill_uid);
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 6c8b7d71a9..028bc013d9 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -4845,6 +4845,9 @@ _hidden int libxl__domain_pvcontrol(libxl__egc *egc,
 /* Check whether a domid is recent */
 int libxl__is_domid_recent(libxl__gc *gc, uint32_t domid, bool *recent);
 
+/* os-specific implementation of setresuid() */
+int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid);
+
 #endif
 
 /*
diff --git a/tools/libs/light/libxl_netbsd.c b/tools/libs/light/libxl_netbsd.c
index 6ad4ed34c2..67caafab9e 100644
--- a/tools/libs/light/libxl_netbsd.c
+++ b/tools/libs/light/libxl_netbsd.c
@@ -124,3 +124,8 @@ int libxl__local_dm_preexec_restrict(libxl__gc *gc)
 {
     return 0;
 }
+
+int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid)
+{
+    assert(!"setresuid is not available on NetBSD, and dm restrction is not supported, so this code path should not have been reached");
+}
diff --git a/tools/libs/light/libxl_setresuid.c b/tools/libs/light/libxl_setresuid.c
new file mode 100644
index 0000000000..ac5cb5db53
--- /dev/null
+++ b/tools/libs/light/libxl_setresuid.c
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2021
+ * Author Manuel Bouyer <bouyer@netbsd.org>
+ *
+ * This program 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 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program 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.
+ */
+ 
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+
+int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid)
+{
+    setresuid(ruid, euid, suid);
+}
-- 
2.29.2

Re: [PATCH v2] libs/light: make it build without setresuid()

[Ian Jackson]

Manuel Bouyer writes ("[PATCH v2] libs/light: make it build without setresuid()"):
> NetBSD doesn't have setresuid(). introcuce libxl__setresuid(),
> which on NetBSD assert() that it's never called (it should not be called when
> dm restriction is off, and NetBSD doesn't support dm restriction at
> this time).
> On linux and FreeBSD it just calls setresuid().

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v2] libs/light: make it build without setresuid()

[Andrew Cooper]

On 26/01/2021 22:47, Manuel Bouyer wrote:
> diff --git a/tools/libs/light/libxl_setresuid.c b/tools/libs/light/libxl_setresuid.c
> new file mode 100644
> index 0000000000..ac5cb5db53
> --- /dev/null
> +++ b/tools/libs/light/libxl_setresuid.c
> @@ -0,0 +1,23 @@
> +/*
> + * Copyright (C) 2021
> + * Author Manuel Bouyer <bouyer@netbsd.org>
> + *
> + * This program 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 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program 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.
> + */
> + 
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +int libxl__setresuid(uid_t ruid, uid_t euid, uid_t suid)
> +{
> +    setresuid(ruid, euid, suid);
> +}

Given the freeze, and discussions on IRC, I have committed most of this
series.

This particular patch doesn't compile, but I fixed it up.

Still outstanding are "NetBSD: use system-provided headers", the
followon patches requested in "libs/light: pass some infos to qemu", and
"xenpmd.c: use dynamic allocation" which failed like this:

https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/996140268

This latter one I didn't feel confident fixing in a way which didn't
break NetBSD, particularly at this point when I'm also racing to get
other content ready as well.

~Andrew

Re: [PATCH v2] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Fri, Jan 29, 2021 at 10:51:14PM +0000, Andrew Cooper wrote:
> 
> Given the freeze, and discussions on IRC, I have committed most of this
> series.

thanks

> 
> This particular patch doesn't compile, but I fixed it up.
> 
> Still outstanding are "NetBSD: use system-provided headers", the

I just sent a v3 of this patch

> followon patches requested in "libs/light: pass some infos to qemu", and

will try to get at it tomorow

> "xenpmd.c: use dynamic allocation" which failed like this:
> 
> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/996140268

It looks like I don't have access to this page, could you share the
content ?

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

Re: [PATCH v2] libs/light: make it build without setresuid()

[Andrew Cooper]

On 29/01/2021 23:01, Manuel Bouyer wrote:
> On Fri, Jan 29, 2021 at 10:51:14PM +0000, Andrew Cooper wrote:
>> Given the freeze, and discussions on IRC, I have committed most of this
>> series.
> thanks
>
>> This particular patch doesn't compile, but I fixed it up.
>>
>> Still outstanding are "NetBSD: use system-provided headers", the
> I just sent a v3 of this patch

You accidentally labelled it v2, but I'm sure we can cope.

>
>> followon patches requested in "libs/light: pass some infos to qemu", and
> will try to get at it tomorow
>
>> "xenpmd.c: use dynamic allocation" which failed like this:
>>
>> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/996140268
> It looks like I don't have access to this page, could you share the
> content ?

urgh - have the permissions broken themselves again...

xenpmd.c:115:13: error: implicit declaration of function 'asprintf'
[-Werror=implicit-function-declaration]

It needs an include of stdio.h, and/or some form of #define _GNU_SOURCE.

~Andrew

Re: [PATCH v2] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Fri, Jan 29, 2021 at 11:05:24PM +0000, Andrew Cooper wrote:
> On 29/01/2021 23:01, Manuel Bouyer wrote:
> > On Fri, Jan 29, 2021 at 10:51:14PM +0000, Andrew Cooper wrote:
> >> Given the freeze, and discussions on IRC, I have committed most of this
> >> series.
> > thanks
> >
> >> This particular patch doesn't compile, but I fixed it up.
> >>
> >> Still outstanding are "NetBSD: use system-provided headers", the
> > I just sent a v3 of this patch
> 
> You accidentally labelled it v2, but I'm sure we can cope.
> 
> >
> >> followon patches requested in "libs/light: pass some infos to qemu", and
> > will try to get at it tomorow
> >
> >> "xenpmd.c: use dynamic allocation" which failed like this:
> >>
> >> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/996140268
> > It looks like I don't have access to this page, could you share the
> > content ?
> 
> urgh - have the permissions broken themselves again...
> 
> xenpmd.c:115:13: error: implicit declaration of function 'asprintf'
> [-Werror=implicit-function-declaration]
> 
> It needs an include of stdio.h, and/or some form of #define _GNU_SOURCE.

stdio.h is there.

I'll look at this closer tomorow.

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

Re: [PATCH v2] libs/light: make it build without setresuid()

[Manuel Bouyer]

On Fri, Jan 29, 2021 at 11:05:24PM +0000, Andrew Cooper wrote:
> On 29/01/2021 23:01, Manuel Bouyer wrote:
> > On Fri, Jan 29, 2021 at 10:51:14PM +0000, Andrew Cooper wrote:
> >> Given the freeze, and discussions on IRC, I have committed most of this
> >> series.
> > thanks
> >
> >> This particular patch doesn't compile, but I fixed it up.
> >>
> >> Still outstanding are "NetBSD: use system-provided headers", the
> > I just sent a v3 of this patch
> 
> You accidentally labelled it v2, but I'm sure we can cope.
> 
> >
> >> followon patches requested in "libs/light: pass some infos to qemu", and
> > will try to get at it tomorow
> >
> >> "xenpmd.c: use dynamic allocation" which failed like this:
> >>
> >> https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/996140268
> > It looks like I don't have access to this page, could you share the
> > content ?
> 
> urgh - have the permissions broken themselves again...
> 
> xenpmd.c:115:13: error: implicit declaration of function 'asprintf'
> [-Werror=implicit-function-declaration]
> 
> It needs an include of stdio.h, and/or some form of #define _GNU_SOURCE.

I added the #define, it builds on NetBSD and Linux.

I just sent a v3 (as 2 separate patches, sorry I don't know how
to easily make it otherwise with git) that should fix it.

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

[PATCH v2] libs/light: pass some infos to qemu

[Manuel Bouyer]

Pass bridge name to qemu as command line option
When starting qemu, set an environnement variable XEN_DOMAIN_ID,
to be used by qemu helper scripts
The only functional difference of using the br parameter is that the
bridge name gets passed to the QEMU script.
NetBSD doesn't have the ioctl to rename network interfaces implemented, and
thus cannot rename the interface from tapX to vifX.Y-emu. Only qemu knowns
the tap interface name, so we need to use the qemu script from qemu itself.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/light/libxl_dm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 3da83259c0..13f79ec471 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -761,6 +761,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
         int nr_set_cpus = 0;
         char *s;
 
+        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", domid));
+
         if (b_info->kernel) {
             LOGD(ERROR, domid, "HVM direct kernel boot is not supported by "
                  "qemu-xen-traditional");
@@ -1547,8 +1549,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 flexarray_append(dm_args, "-netdev");
                 flexarray_append(dm_args,
                                  GCSPRINTF("type=tap,id=net%d,ifname=%s,"
+                                           "br=%s,"
                                            "script=%s,downscript=%s",
                                            nics[i].devid, ifname,
+                                           nics[i].bridge,
                                            libxl_tapif_script(gc),
                                            libxl_tapif_script(gc)));
 
@@ -1825,6 +1829,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     flexarray_append(dm_args, GCSPRINTF("%"PRId64, ram_size));
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", guest_domid));
+
         if (b_info->u.hvm.hdtype == LIBXL_HDTYPE_AHCI)
             flexarray_append_pair(dm_args, "-device", "ahci,id=ahci0");
         for (i = 0; i < num_disks; i++) {
-- 
2.29.2

Re: [PATCH v2] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:47:58PM +0100, Manuel Bouyer wrote:
> Pass bridge name to qemu as command line option
> When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> to be used by qemu helper scripts
> The only functional difference of using the br parameter is that the
> bridge name gets passed to the QEMU script.
> NetBSD doesn't have the ioctl to rename network interfaces implemented, and
> thus cannot rename the interface from tapX to vifX.Y-emu. Only qemu knowns
> the tap interface name, so we need to use the qemu script from qemu itself.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

If you have a moment might be worth adding a note in
xl-network-configuration.5.pod that NetBSD in HVM mode requires
bridged networking I think?

Also, the qemu-ifup script doesn't seem to be part of the NetBSD
scripts that are upstream, is this something carried by the NetBSD
package?

I certainly don't mind adding those extra parameters/env variables,
but might be nice to clarify the expectations, that can be done in a
separate patch.

Thanks, Roger.

Re: [PATCH v2] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 11:47:58PM +0100, Manuel Bouyer wrote:
> > Pass bridge name to qemu as command line option
> > When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> > to be used by qemu helper scripts
> > The only functional difference of using the br parameter is that the
> > bridge name gets passed to the QEMU script.
> > NetBSD doesn't have the ioctl to rename network interfaces implemented, and
> > thus cannot rename the interface from tapX to vifX.Y-emu. Only qemu knowns
> > the tap interface name, so we need to use the qemu script from qemu itself.
> > 
> > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> If you have a moment might be worth adding a note in
> xl-network-configuration.5.pod that NetBSD in HVM mode requires
> bridged networking I think?

With the default qemu-ifup script only. As you can do whatever you want
in the script, you can support whatever network configuration you want.
It's quite easy to do IP routing for example.

> 
> Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> scripts that are upstream, is this something carried by the NetBSD
> package?

Ha maybe I overlooked this. I'll add it, but maybe it can be submitted in
a separate patch ?

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

Re: [PATCH v2] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Fri, Jan 29, 2021 at 11:46:53AM +0100, Manuel Bouyer wrote:
> On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 26, 2021 at 11:47:58PM +0100, Manuel Bouyer wrote:
> > > Pass bridge name to qemu as command line option
> > > When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> > > to be used by qemu helper scripts
> > > The only functional difference of using the br parameter is that the
> > > bridge name gets passed to the QEMU script.
> > > NetBSD doesn't have the ioctl to rename network interfaces implemented, and
> > > thus cannot rename the interface from tapX to vifX.Y-emu. Only qemu knowns
> > > the tap interface name, so we need to use the qemu script from qemu itself.
> > > 
> > > Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > If you have a moment might be worth adding a note in
> > xl-network-configuration.5.pod that NetBSD in HVM mode requires
> > bridged networking I think?
> 
> With the default qemu-ifup script only. As you can do whatever you want
> in the script, you can support whatever network configuration you want.
> It's quite easy to do IP routing for example.

Right, but the default script provided will do bridging mode only, and
even if you add 'script=vif-ip' to the network configuration line it
won't do what you expect. Instead it will try to add the tap network
interface to the default xenbr0 bridge.

I'm not opposed to having it this way right now, as it's better to
have this than no support at all, but we should have the shortcoming
documented somewhere. Can be done as a separate patch.

> > 
> > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > scripts that are upstream, is this something carried by the NetBSD
> > package?
> 
> Ha maybe I overlooked this. I'll add it, but maybe it can be submitted in
> a separate patch ?

Yes, that's fine, can be added as a separate patch.

Thanks, Roger.

Re: [PATCH v2] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Fri, Jan 29, 2021 at 03:52:14PM +0100, Roger Pau Monné wrote:
> 
> Right, but the default script provided will do bridging mode only, and
> even if you add 'script=vif-ip' to the network configuration line it
> won't do what you expect. Instead it will try to add the tap network
> interface to the default xenbr0 bridge.
> 
> I'm not opposed to having it this way right now, as it's better to
> have this than no support at all, but we should have the shortcoming
> documented somewhere. Can be done as a separate patch.

I just sent a v3, with a patch to xl-network-configuration.5.pod

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

Re: [PATCH v2] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> [...]
> Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> scripts that are upstream, is this something carried by the NetBSD
> package?

Actually, the script is part of qemu-xen-traditional:
tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD

and it's installed as part of 'make install'. The same script can be used
for both qemu-xen-traditional and qemu-xen as long as we support only
bridged mode by default.

qemu-xen-traditional did call the script with the bridge name.
This patch makes qemu-xen call the script with the same parameters,
and add the XEN_DOMAIN_ID environnement variable.

Is it OK to keep the script from qemu-xen-traditional (and installed as
part of qemu-xen-traditional) for now ?

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

Re: [PATCH v2] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Sat, Jan 30, 2021 at 12:50:13PM +0100, Manuel Bouyer wrote:
> On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > [...]
> > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > scripts that are upstream, is this something carried by the NetBSD
> > package?
> 
> Actually, the script is part of qemu-xen-traditional:
> tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD
> 
> and it's installed as part of 'make install'. The same script can be used
> for both qemu-xen-traditional and qemu-xen as long as we support only
> bridged mode by default.
> 
> qemu-xen-traditional did call the script with the bridge name.
> This patch makes qemu-xen call the script with the same parameters,
> and add the XEN_DOMAIN_ID environnement variable.
> 
> Is it OK to keep the script from qemu-xen-traditional (and installed as
> part of qemu-xen-traditional) for now ?

I think you want to move the script into hotplug/NetBSD/ because it
should be possible to install a system without qemu-xen-traditional
(--disable-qemu-traditional) and only qemu-upstream, and the script
will still be needed in that case.

Thanks, Roger.

Re: [PATCH v2] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Mon, Feb 01, 2021 at 09:06:13AM +0100, Roger Pau Monné wrote:
> On Sat, Jan 30, 2021 at 12:50:13PM +0100, Manuel Bouyer wrote:
> > On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > > [...]
> > > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > > scripts that are upstream, is this something carried by the NetBSD
> > > package?
> > 
> > Actually, the script is part of qemu-xen-traditional:
> > tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD
> > 
> > and it's installed as part of 'make install'. The same script can be used
> > for both qemu-xen-traditional and qemu-xen as long as we support only
> > bridged mode by default.
> > 
> > qemu-xen-traditional did call the script with the bridge name.
> > This patch makes qemu-xen call the script with the same parameters,
> > and add the XEN_DOMAIN_ID environnement variable.
> > 
> > Is it OK to keep the script from qemu-xen-traditional (and installed as
> > part of qemu-xen-traditional) for now ?
> 
> I think you want to move the script into hotplug/NetBSD/ because it
> should be possible to install a system without qemu-xen-traditional
> (--disable-qemu-traditional) and only qemu-upstream, and the script
> will still be needed in that case.

I can, but how do I get the ecript removed from qemu-traditional ?
It's a different repo, isn't it ?

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

Re: [PATCH v2] libs/light: pass some infos to qemu

[Roger Pau Monné]

On Mon, Feb 01, 2021 at 10:39:39AM +0100, Manuel Bouyer wrote:
> On Mon, Feb 01, 2021 at 09:06:13AM +0100, Roger Pau Monné wrote:
> > On Sat, Jan 30, 2021 at 12:50:13PM +0100, Manuel Bouyer wrote:
> > > On Thu, Jan 28, 2021 at 12:08:02PM +0100, Roger Pau Monné wrote:
> > > > [...]
> > > > Also, the qemu-ifup script doesn't seem to be part of the NetBSD
> > > > scripts that are upstream, is this something carried by the NetBSD
> > > > package?
> > > 
> > > Actually, the script is part of qemu-xen-traditional:
> > > tools/qemu-xen-traditional/i386-dm/qemu-ifup-NetBSD
> > > 
> > > and it's installed as part of 'make install'. The same script can be used
> > > for both qemu-xen-traditional and qemu-xen as long as we support only
> > > bridged mode by default.
> > > 
> > > qemu-xen-traditional did call the script with the bridge name.
> > > This patch makes qemu-xen call the script with the same parameters,
> > > and add the XEN_DOMAIN_ID environnement variable.
> > > 
> > > Is it OK to keep the script from qemu-xen-traditional (and installed as
> > > part of qemu-xen-traditional) for now ?
> > 
> > I think you want to move the script into hotplug/NetBSD/ because it
> > should be possible to install a system without qemu-xen-traditional
> > (--disable-qemu-traditional) and only qemu-upstream, and the script
> > will still be needed in that case.
> 
> I can, but how do I get the ecript removed from qemu-traditional ?
> It's a different repo, isn't it ?

Yes, it's:

http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=summary

I would remove it from qemu-trad and then only install from
hotplug/NetBSD if it's not already there? Or maybe just force-install
it from hotplug/NetBSD even if it's already present?

Thanks, Roger.

Re: [PATCH v2] libs/light: pass some infos to qemu

[Manuel Bouyer]

On Mon, Feb 01, 2021 at 11:54:35AM +0100, Roger Pau Monné wrote:
> > I can, but how do I get the ecript removed from qemu-traditional ?
> > It's a different repo, isn't it ?
> 
> Yes, it's:
> 
> http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=summary
> 
> I would remove it from qemu-trad and then only install from
> hotplug/NetBSD if it's not already there? Or maybe just force-install
> it from hotplug/NetBSD even if it's already present?

OK will try that

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

[PATCH v2] libs/store: make build without PTHREAD_STACK_MIN

[Manuel Bouyer]

On NetBSD, PTHREAD_STACK_MIN is not available.
If PTHREAD_STACK_MIN is not defined, define it to 0 so that we fallback to
DEFAULT_THREAD_STACKSIZE

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/libs/store/xs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 4ac73ec317..b6ecbd787e 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -811,6 +811,11 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
 
 #ifdef USE_PTHREAD
 #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
+/* NetBSD doesn't have PTHREAD_STACK_MIN. */
+#ifndef PTHREAD_STACK_MIN
+# define PTHREAD_STACK_MIN 0
+#endif
+
 #define READ_THREAD_STACKSIZE 					\
 	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
 	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
-- 
2.29.2

Re: [PATCH v2] libs/store: make build without PTHREAD_STACK_MIN

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:47:59PM +0100, Manuel Bouyer wrote:
> On NetBSD, PTHREAD_STACK_MIN is not available.
> If PTHREAD_STACK_MIN is not defined, define it to 0 so that we fallback to
> DEFAULT_THREAD_STACKSIZE
> 

I would add:

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>

> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
>  tools/libs/store/xs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 4ac73ec317..b6ecbd787e 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -811,6 +811,11 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
>  
>  #ifdef USE_PTHREAD
>  #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
> +/* NetBSD doesn't have PTHREAD_STACK_MIN. */
> +#ifndef PTHREAD_STACK_MIN
> +# define PTHREAD_STACK_MIN 0
> +#endif
> +
>  #define READ_THREAD_STACKSIZE 					\
>  	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
>  	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)

There was also a suggestion to use MAX(PTHREAD_STACK_MIN,
DEFAULT_THREAD_STACKSIZE). Is maybe MAX not defied here?

Thanks, Roger.

Re: [PATCH v2] libs/store: make build without PTHREAD_STACK_MIN

[Andrew Cooper]

On 28/01/2021 10:57, Roger Pau Monné wrote:
> On Tue, Jan 26, 2021 at 11:47:59PM +0100, Manuel Bouyer wrote:
>> On NetBSD, PTHREAD_STACK_MIN is not available.
>> If PTHREAD_STACK_MIN is not defined, define it to 0 so that we fallback to
>> DEFAULT_THREAD_STACKSIZE
>>
> I would add:
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
>> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> ---
>>  tools/libs/store/xs.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>> index 4ac73ec317..b6ecbd787e 100644
>> --- a/tools/libs/store/xs.c
>> +++ b/tools/libs/store/xs.c
>> @@ -811,6 +811,11 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
>>  
>>  #ifdef USE_PTHREAD
>>  #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
>> +/* NetBSD doesn't have PTHREAD_STACK_MIN. */
>> +#ifndef PTHREAD_STACK_MIN
>> +# define PTHREAD_STACK_MIN 0
>> +#endif
>> +
>>  #define READ_THREAD_STACKSIZE 					\
>>  	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
>>  	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
> There was also a suggestion to use MAX(PTHREAD_STACK_MIN,
> DEFAULT_THREAD_STACKSIZE). Is maybe MAX not defied here?

TBH, I was planning to submit an incremental cleanup doing this
separately.  It would be cleaner than putting cleanup into the "fix
NetBSD" patch.

~Andrew

Re: [PATCH v2] libs/store: make build without PTHREAD_STACK_MIN

[Manuel Bouyer]

On Thu, Jan 28, 2021 at 11:08:08AM +0000, Andrew Cooper wrote:
> On 28/01/2021 10:57, Roger Pau Monné wrote:
> > On Tue, Jan 26, 2021 at 11:47:59PM +0100, Manuel Bouyer wrote:
> >> On NetBSD, PTHREAD_STACK_MIN is not available.
> >> If PTHREAD_STACK_MIN is not defined, define it to 0 so that we fallback to
> >> DEFAULT_THREAD_STACKSIZE
> >>
> > I would add:
> >
> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> >> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> >> ---
> >>  tools/libs/store/xs.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> >> index 4ac73ec317..b6ecbd787e 100644
> >> --- a/tools/libs/store/xs.c
> >> +++ b/tools/libs/store/xs.c
> >> @@ -811,6 +811,11 @@ bool xs_watch(struct xs_handle *h, const char *path, const char *token)
> >>  
> >>  #ifdef USE_PTHREAD
> >>  #define DEFAULT_THREAD_STACKSIZE (16 * 1024)
> >> +/* NetBSD doesn't have PTHREAD_STACK_MIN. */
> >> +#ifndef PTHREAD_STACK_MIN
> >> +# define PTHREAD_STACK_MIN 0
> >> +#endif
> >> +
> >>  #define READ_THREAD_STACKSIZE 					\
> >>  	((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? 	\
> >>  	PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
> > There was also a suggestion to use MAX(PTHREAD_STACK_MIN,
> > DEFAULT_THREAD_STACKSIZE). Is maybe MAX not defied here?
> 
> TBH, I was planning to submit an incremental cleanup doing this
> separately.  It would be cleaner than putting cleanup into the "fix
> NetBSD" patch.

yes, that was my idea too

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

[PATCH v2] xenpmd.c: use dynamic allocation

[Manuel Bouyer]

On NetBSD, d_name is larger than 256, so file_name[284] may not be large
enough (and gcc emits a format-truncation error).
Use asprintf() instead of snprintf() on a static on-stack buffer.

Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
---
 tools/xenpmd/xenpmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/xenpmd/xenpmd.c b/tools/xenpmd/xenpmd.c
index 12b82cf43e..e432aad856 100644
--- a/tools/xenpmd/xenpmd.c
+++ b/tools/xenpmd/xenpmd.c
@@ -101,7 +101,7 @@ FILE *get_next_battery_file(DIR *battery_dir,
 {
     FILE *file = 0;
     struct dirent *dir_entries;
-    char file_name[284];
+    char *file_name;
     int ret;
     
     do 
@@ -112,16 +112,16 @@ FILE *get_next_battery_file(DIR *battery_dir,
         if ( strlen(dir_entries->d_name) < 4 )
             continue;
         if ( battery_info_type == BIF ) 
-            ret = snprintf(file_name, sizeof(file_name), BATTERY_INFO_FILE_PATH,
+            ret = asprintf(&file_name, BATTERY_INFO_FILE_PATH,
                      dir_entries->d_name);
         else 
-            ret = snprintf(file_name, sizeof(file_name), BATTERY_STATE_FILE_PATH,
+            ret = asprintf(&file_name, BATTERY_STATE_FILE_PATH,
                      dir_entries->d_name);
         /* This should not happen but is needed to pass gcc checks */
         if (ret < 0)
             continue;
-        file_name[sizeof(file_name) - 1] = '\0';
         file = fopen(file_name, "r");
+	free(file_name);
     } while ( !file );
 
     return file;
-- 
2.29.2

Re: [PATCH v2] xenpmd.c: use dynamic allocation

[Roger Pau Monné]

On Tue, Jan 26, 2021 at 11:48:00PM +0100, Manuel Bouyer wrote:
> On NetBSD, d_name is larger than 256, so file_name[284] may not be large
> enough (and gcc emits a format-truncation error).
> Use asprintf() instead of snprintf() on a static on-stack buffer.
> 
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
>  tools/xenpmd/xenpmd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/xenpmd/xenpmd.c b/tools/xenpmd/xenpmd.c
> index 12b82cf43e..e432aad856 100644
> --- a/tools/xenpmd/xenpmd.c
> +++ b/tools/xenpmd/xenpmd.c
> @@ -101,7 +101,7 @@ FILE *get_next_battery_file(DIR *battery_dir,
>  {
>      FILE *file = 0;
>      struct dirent *dir_entries;
> -    char file_name[284];
> +    char *file_name;
>      int ret;
>      
>      do 
> @@ -112,16 +112,16 @@ FILE *get_next_battery_file(DIR *battery_dir,
>          if ( strlen(dir_entries->d_name) < 4 )
>              continue;
>          if ( battery_info_type == BIF ) 
> -            ret = snprintf(file_name, sizeof(file_name), BATTERY_INFO_FILE_PATH,
> +            ret = asprintf(&file_name, BATTERY_INFO_FILE_PATH,
>                       dir_entries->d_name);
>          else 
> -            ret = snprintf(file_name, sizeof(file_name), BATTERY_STATE_FILE_PATH,
> +            ret = asprintf(&file_name, BATTERY_STATE_FILE_PATH,
>                       dir_entries->d_name);
>          /* This should not happen but is needed to pass gcc checks */
>          if (ret < 0)
>              continue;
> -        file_name[sizeof(file_name) - 1] = '\0';
>          file = fopen(file_name, "r");
> +	free(file_name);

Hard tab. May I ask whether this can be fixed on commit so that
there's no need to resend a new version?

Thanks, Roger.

Re: [PATCH v2] Fix error: array subscript has type 'char'

[Andrew Cooper]

On 26/01/2021 22:47, Manuel Bouyer wrote:
> Use unsigned char variable, or cast to (unsigned char), for
> tolower()/islower() and friends. Fix compiler error
> array subscript has type 'char' [-Werror=char-subscripts]
>
> Signed-off-by: Manuel Bouyer <bouyer@netbsd.org>
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Release-Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libs/light/libxl_qmp.c | 2 +-
>  tools/xentrace/xentrace.c    | 2 +-
>  xen/tools/symbols.c          | 4 ++--

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

This is totally ugly, but it is a well known totally ugly corner case of
C.  Non-freestanding bits have to play by C's rules to be compatible.

~Andrew