Re: [PATCH] VMD: Remove switch on reload prior to re-creating

2017-10-10 Thread Mike Larkin 
On Wed, Oct 11, 2017 at 06:46:22AM +0200, Claudio Jeker wrote:
> On Tue, Oct 10, 2017 at 03:57:25PM -0700, Carlos Cardenas wrote:
> > Destroy switch on `vmctl reload` to allow SIOCBRDGADD to succeed
> > when creating new bridge and attaching interfaces to it.
> > 
> > Comments? Ok?
> 
> I don't think it is a good idea to destroy and recreate bridge interfaces
> on every reload. This changes the underlying network and may result in
> broken connections and network hickups. I would suggest that vmctl is
> instead checking which interfaces are on the bridge and calls SIOCBRDDEL
> if needed and skips the SIOCBRDGADD if not needed.
> 

you mean vmd, right?

vmctl has no special privilege to do this.

>  
> > diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
> > index ef42549d105..0190c049837 100644
> > --- usr.sbin/vmd/priv.c
> > +++ usr.sbin/vmd/priv.c
> > @@ -88,6 +88,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
> > struct imsg *imsg)
> > switch (imsg->hdr.type) {
> > case IMSG_VMDOP_PRIV_IFDESCR:
> > case IMSG_VMDOP_PRIV_IFCREATE:
> > +   case IMSG_VMDOP_PRIV_IFDESTROY:
> > case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > case IMSG_VMDOP_PRIV_IFADD:
> > case IMSG_VMDOP_PRIV_IFUP:
> > @@ -125,6 +126,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
> > struct imsg *imsg)
> > errno != EEXIST)
> > log_warn("SIOCIFCREATE");
> > break;
> > +   case IMSG_VMDOP_PRIV_IFDESTROY:
> > +   /* Destroy the bridge */
> > +   strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > +   if (ioctl(env->vmd_fd, SIOCIFDESTROY, ) < 0 &&
> > +   errno != EEXIST)
> > +   log_warn("SIOCIFDESTROY");
> > +   break;
> > case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > ifr.ifr_rdomainid = vfr.vfr_id;
> > @@ -444,6 +452,23 @@ vm_priv_brconfig(struct privsep *ps, struct vmd_switch 
> > *vsw)
> > return (0);
> >  }
> >  
> > +int
> > +vm_priv_brdestroy(struct privsep *ps, struct vmd_switch *vsw)
> > +{
> > +   struct vmop_ifreqvfr;
> > +
> > +   memset(, 0, sizeof(vfr));
> > +
> > +   if (strlcpy(vfr.vfr_name, vsw->sw_ifname,
> > +   sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
> > +   return (-1);
> > +
> > +   proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFDESTROY,
> > +   , sizeof(vfr));
> > +
> > +   return (0);
> > +}
> > +
> >  uint32_t
> >  vm_priv_addr(struct address *h, uint32_t vmid, int idx, int isvm)
> >  {
> > diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> > index f1abc54d9a3..834654a76de 100644
> > --- usr.sbin/vmd/vmd.c
> > +++ usr.sbin/vmd/vmd.c
> > @@ -852,7 +852,7 @@ int
> >  vmd_reload(unsigned int reset, const char *filename)
> >  {
> > struct vmd_vm   *vm, *next_vm;
> > -   struct vmd_switch   *vsw;
> > +   struct vmd_switch   *vsw, *next_vsw;
> > int  reload = 0;
> >  
> > /* Switch back to the default config file */
> > @@ -885,6 +885,19 @@ vmd_reload(unsigned int reset, const char *filename)
> > }
> > }
> >  
> > +   TAILQ_FOREACH_SAFE(vsw, env->vmd_switches,
> > +   sw_entry, next_vsw) {
> > +   log_debug("%s: removing %s",
> > +   __func__, vsw->sw_ifname);
> > +   if (vm_priv_brdestroy(>vmd_ps,
> > +   vsw) == -1 ) {
> > +   log_warn("%s: failed to destroy switch",
> > +   __func__);
> > +   }
> > +   TAILQ_REMOVE(env->vmd_switches, vsw, sw_entry);
> > +   free(vsw);
> > +   }
> > +
> > /* Update shared global configuration in all children */
> > if (config_setconfig(env) == -1)
> > return (-1);
> > diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> > index 4b7b5f70495..fce5fcaaa60 100644
> > --- usr.sbin/vmd/vmd.h
> > +++ usr.sbin/vmd/vmd.h
> > @@ -100,6 +100,7 @@ enum imsg_type {
> > IMSG_VMDOP_PRIV_IFGROUP,
> > IMSG_VMDOP_PRIV_IFADDR,
> > IMSG_VMDOP_PRIV_IFRDOMAIN,
> > +   IMSG_VMDOP_PRIV_IFDESTROY,
> > IMSG_VMDOP_VM_SHUTDOWN,
> > IMSG_VMDOP_VM_REBOOT,
> > IMSG_VMDOP_CONFIG
> > @@ -323,6 +324,7 @@ int  priv_findname(const char *, const char **);
> >  int priv_validgroup(const char *);
> >  int vm_priv_ifconfig(struct privsep *, struct vmd_vm *);
> >  int vm_priv_brconfig(struct privsep *, struct vmd_switch *);
> > +int vm_priv_brdestroy(struct privsep *, struct vmd_switch *);
> >  uint32_t vm_priv_addr(struct address *, uint32_t, int, int);
> >  
> >  /* vmm.c */
> > -- 
> > 2.14.2
> > 
> 
> -- 
> :wq Claudio
>

Re: [PATCH] VMD: Remove switch on reload prior to re-creating

2017-10-10 Thread Claudio Jeker 
On Tue, Oct 10, 2017 at 03:57:25PM -0700, Carlos Cardenas wrote:
> Destroy switch on `vmctl reload` to allow SIOCBRDGADD to succeed
> when creating new bridge and attaching interfaces to it.
> 
> Comments? Ok?

I don't think it is a good idea to destroy and recreate bridge interfaces
on every reload. This changes the underlying network and may result in
broken connections and network hickups. I would suggest that vmctl is
instead checking which interfaces are on the bridge and calls SIOCBRDDEL
if needed and skips the SIOCBRDGADD if not needed.

 
> diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
> index ef42549d105..0190c049837 100644
> --- usr.sbin/vmd/priv.c
> +++ usr.sbin/vmd/priv.c
> @@ -88,6 +88,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, struct 
> imsg *imsg)
>   switch (imsg->hdr.type) {
>   case IMSG_VMDOP_PRIV_IFDESCR:
>   case IMSG_VMDOP_PRIV_IFCREATE:
> + case IMSG_VMDOP_PRIV_IFDESTROY:
>   case IMSG_VMDOP_PRIV_IFRDOMAIN:
>   case IMSG_VMDOP_PRIV_IFADD:
>   case IMSG_VMDOP_PRIV_IFUP:
> @@ -125,6 +126,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
> struct imsg *imsg)
>   errno != EEXIST)
>   log_warn("SIOCIFCREATE");
>   break;
> + case IMSG_VMDOP_PRIV_IFDESTROY:
> + /* Destroy the bridge */
> + strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> + if (ioctl(env->vmd_fd, SIOCIFDESTROY, ) < 0 &&
> + errno != EEXIST)
> + log_warn("SIOCIFDESTROY");
> + break;
>   case IMSG_VMDOP_PRIV_IFRDOMAIN:
>   strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
>   ifr.ifr_rdomainid = vfr.vfr_id;
> @@ -444,6 +452,23 @@ vm_priv_brconfig(struct privsep *ps, struct vmd_switch 
> *vsw)
>   return (0);
>  }
>  
> +int
> +vm_priv_brdestroy(struct privsep *ps, struct vmd_switch *vsw)
> +{
> + struct vmop_ifreqvfr;
> +
> + memset(, 0, sizeof(vfr));
> +
> + if (strlcpy(vfr.vfr_name, vsw->sw_ifname,
> + sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
> + return (-1);
> +
> + proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFDESTROY,
> + , sizeof(vfr));
> +
> + return (0);
> +}
> +
>  uint32_t
>  vm_priv_addr(struct address *h, uint32_t vmid, int idx, int isvm)
>  {
> diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> index f1abc54d9a3..834654a76de 100644
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -852,7 +852,7 @@ int
>  vmd_reload(unsigned int reset, const char *filename)
>  {
>   struct vmd_vm   *vm, *next_vm;
> - struct vmd_switch   *vsw;
> + struct vmd_switch   *vsw, *next_vsw;
>   int  reload = 0;
>  
>   /* Switch back to the default config file */
> @@ -885,6 +885,19 @@ vmd_reload(unsigned int reset, const char *filename)
>   }
>   }
>  
> + TAILQ_FOREACH_SAFE(vsw, env->vmd_switches,
> + sw_entry, next_vsw) {
> + log_debug("%s: removing %s",
> + __func__, vsw->sw_ifname);
> + if (vm_priv_brdestroy(>vmd_ps,
> + vsw) == -1 ) {
> + log_warn("%s: failed to destroy switch",
> + __func__);
> + }
> + TAILQ_REMOVE(env->vmd_switches, vsw, sw_entry);
> + free(vsw);
> + }
> +
>   /* Update shared global configuration in all children */
>   if (config_setconfig(env) == -1)
>   return (-1);
> diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> index 4b7b5f70495..fce5fcaaa60 100644
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -100,6 +100,7 @@ enum imsg_type {
>   IMSG_VMDOP_PRIV_IFGROUP,
>   IMSG_VMDOP_PRIV_IFADDR,
>   IMSG_VMDOP_PRIV_IFRDOMAIN,
> + IMSG_VMDOP_PRIV_IFDESTROY,
>   IMSG_VMDOP_VM_SHUTDOWN,
>   IMSG_VMDOP_VM_REBOOT,
>   IMSG_VMDOP_CONFIG
> @@ -323,6 +324,7 @@ intpriv_findname(const char *, const char **);
>  int   priv_validgroup(const char *);
>  int   vm_priv_ifconfig(struct privsep *, struct vmd_vm *);
>  int   vm_priv_brconfig(struct privsep *, struct vmd_switch *);
> +int   vm_priv_brdestroy(struct privsep *, struct vmd_switch *);
>  uint32_t vm_priv_addr(struct address *, uint32_t, int, int);
>  
>  /* vmm.c */
> -- 
> 2.14.2
> 

-- 
:wq Claudio

Httpd support for internal redirects.

2017-10-10 Thread Ori Bernstein 
My website generator is a little stupid at times. It generates
files with .html suffixes, but urls without them.

I worked around this with some redirects, but it never felt
quite right doing an extra round trip. Therefore, I added
internal redirects, processing the rewrite before responding to
an http request.

This introduces new syntax to the config file, allowing you to
do:

location match "^(/foo/bar/[%w]+)$" {
rewrite-to "/baz/%1.html"
}

Because we don't know what the paths should be relative
to, all paths rewritten must be absolute.

It seems like someone else may find it useful[1], so
I'm submitting it. I've been running a slightly older
version of this on https://myrlang.org for the last
day or two, and it's been uneventful. The difference
is that the syntax used to piggy back off the "block"
action => 'block internal return 302 "path"'.

This doesn't currently support chained rewrites. I think
that it wouldn't be hard to add if it's needed.

Ok?

[1] https://github.com/reyk/httpd/issues/27
==


diff --git usr.sbin/httpd/config.c usr.sbin/httpd/config.c
index 3c31c3d4cd3..7d2982af7b9 100644
--- usr.sbin/httpd/config.c
+++ usr.sbin/httpd/config.c
@@ -448,7 +448,7 @@ config_getserver_config(struct httpd *env, struct server 
*srv,
sizeof(srv_conf->errorlog));
}
 
-   f = SRVFLAG_BLOCK|SRVFLAG_NO_BLOCK;
+   f = SRVFLAG_BLOCK|SRVFLAG_NO_BLOCK|SRVFLAG_REWRITE;
if ((srv_conf->flags & f) == 0) {
free(srv_conf->return_uri);
srv_conf->flags |= parent->flags & f;
diff --git usr.sbin/httpd/httpd.conf.5 usr.sbin/httpd/httpd.conf.5
index a3c97629de3..3a00a750537 100644
--- usr.sbin/httpd/httpd.conf.5
+++ usr.sbin/httpd/httpd.conf.5
@@ -454,6 +454,14 @@ instead of the log files.
 Disable any previous
 .Ic block
 in a location.
+.It Ic rewrite-to Ar path
+The current request path is rewritten to
+.Ar  path .
+using the same macro expansions as
+.Cm block return
+rules. After macros are substituted, the resulting paths must be
+absolute, beginning with a slash.  Rewriting is not done
+recursively.
 .It Ic root Ar option
 Configure the document root and options for the request path.
 Valid options are:
diff --git usr.sbin/httpd/httpd.h usr.sbin/httpd/httpd.h
index 05cbb8e3550..477115ec92d 100644
--- usr.sbin/httpd/httpd.h
+++ usr.sbin/httpd/httpd.h
@@ -394,6 +394,7 @@ SPLAY_HEAD(client_tree, client);
 #define SRVFLAG_SERVER_MATCH   0x0020
 #define SRVFLAG_SERVER_HSTS0x0040
 #define SRVFLAG_DEFAULT_TYPE   0x0080
+#define SRVFLAG_REWRITE0x0100
 
 #define SRVFLAG_BITS   \
"\10\01INDEX\02NO_INDEX\03AUTO_INDEX\04NO_AUTO_INDEX"   \
diff --git usr.sbin/httpd/parse.y usr.sbin/httpd/parse.y
index fcf1938c42d..4072ee5b532 100644
--- usr.sbin/httpd/parse.y
+++ usr.sbin/httpd/parse.y
@@ -134,7 +134,7 @@ typedef struct {
 %token LISTEN LOCATION LOG LOGDIR MATCH MAXIMUM NO NODELAY OCSP ON PORT PREFORK
 %token PROTOCOLS REQUESTS ROOT SACK SERVER SOCKET STRIP STYLE SYSLOG TCP TICKET
 %token TIMEOUT TLS TYPE TYPES HSTS MAXAGE SUBDOMAINS DEFAULT PRELOAD REQUEST
-%token ERROR INCLUDE AUTHENTICATE WITH BLOCK DROP RETURN PASS
+%token ERROR INCLUDE AUTHENTICATE WITH BLOCK DROP RETURN REWRITE PASS
 %token   STRING
 %token   NUMBER
 %type  port
@@ -986,6 +986,11 @@ filter : block RETURN NUMBER optstring {
srv_conf->return_uri_len = strlen($4) + 1;
}
}
+   | REWRITE STRING {
+   srv_conf->flags |= SRVFLAG_REWRITE;
+   srv_conf->return_uri = $2;
+   srv_conf->return_uri_len = strlen($2) + 1;
+   }
| block DROP{
/* No return code, silently drop the connection */
srv_conf->return_code = 0;
@@ -1255,6 +1260,7 @@ lookup(char *s)
{ "request",REQUEST },
{ "requests",   REQUESTS },
{ "return", RETURN },
+   { "rewrite-to", REWRITE },
{ "root",   ROOT },
{ "sack",   SACK },
{ "server", SERVER },
diff --git usr.sbin/httpd/server_http.c usr.sbin/httpd/server_http.c
index e64de0d2f9c..c9ea4771037 100644
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -1162,10 +1162,34 @@ server_expand_http(struct client *clt, const char *val, 
char *buf,
return (buf);
 }
 
+static int
+server_set_path(struct http_descriptor *desc, char *input)
+{
+   char path[PATH_MAX];
+
+   if (input == NULL || url_decode(input) == NULL)
+   return -1;
+   if

[PATCH] tests for vmd config parsing

2017-10-10 Thread Carlos Cardenas 
This patch adds a set of tests for vmd config parsing.

Comments? Ok?


diff --git regress/usr.sbin/Makefile regress/usr.sbin/Makefile
index 3912e794d4d..f19a656d45e 100644
--- regress/usr.sbin/Makefile
+++ regress/usr.sbin/Makefile
@@ -12,6 +12,7 @@ SUBDIR += relayd
 SUBDIR += snmpd
 SUBDIR += switchd
 SUBDIR += syslogd
+SUBDIR += vmd
 
 .if defined(REGRESS_FULL) || make(clean) || make(cleandir) || make(obj)
 SUBDIR += pkg_add
diff --git regress/usr.sbin/vmd/Makefile regress/usr.sbin/vmd/Makefile
new file mode 100644
index 000..6c6671ada3f
--- /dev/null
+++ regress/usr.sbin/vmd/Makefile
@@ -0,0 +1,5 @@
+#  $OpenBSD$
+
+SUBDIR += config
+
+.include 
diff --git regress/usr.sbin/vmd/config/Makefile 
regress/usr.sbin/vmd/config/Makefile
new file mode 100644
index 000..f5f58658af6
--- /dev/null
+++ regress/usr.sbin/vmd/config/Makefile
@@ -0,0 +1,32 @@
+#  $OpenBSD$
+
+VMD ?= /usr/sbin/vmd
+
+VMD_PASS=boot-keyword memory-round memory-just-enough
+VMD_FAIL=kernel-keyword too-few-ram vm-name-too-long too-many-ifs \
+boot-name-too-long disk-path-too-long too-many-disks
+
+REGRESS_TARGETS=
+
+.for n in ${VMD_PASS}
+REGRESS_TARGETS += vmd-pass-${n}
+
+vmd-pass-${n}:
+   @echo ' $@ '
+   ${VMD} -n -f ${.CURDIR}/vmd-pass-${n}.conf 2>&1 | \
+   diff -u ${.CURDIR}/vmd-pass-${n}.ok /dev/stdin
+.endfor
+
+.for n in ${VMD_FAIL}
+REGRESS_TARGETS += vmd-fail-${n}
+
+vmd-fail-${n}:
+   @echo ' $@ '
+   ${VMD} -n -f ${.CURDIR}/vmd-fail-${n}.conf 2>&1 | \
+   cut -d : -f 2,3,4 | \
+   diff -u ${.CURDIR}/vmd-fail-${n}.ok /dev/stdin
+.endfor
+
+.PHONY: ${REGRESS_TARGETS}
+
+.include 
diff --git regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.conf 
regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.conf
new file mode 100644
index 000..bc569a9119e
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.conf
@@ -0,0 +1,6 @@
+#  $OpenBSD$
+# Fail on boot path (> 128)
+ramdisk="/some/absolutepath/somewhere/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/bsd.rd"
+vm "x" {
+boot $ramdisk
+}
diff --git regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.ok 
regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.ok
new file mode 100644
index 000..56cb73b98cf
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.ok
@@ -0,0 +1 @@
+5: kernel name too long
diff --git regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.conf 
regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.conf
new file mode 100644
index 000..b70c3acf507
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.conf
@@ -0,0 +1,6 @@
+#  $OpenBSD$
+# Fail on disk path (> 128)
+rdisk="/some/absolutepath/somewhere/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/bsd.img"
+vm "x" {
+disk $rdisk
+}
diff --git regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.ok 
regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.ok
new file mode 100644
index 000..a384c812362
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.ok
@@ -0,0 +1,2 @@
+disk path too long
+5: failed to parse disks: 
/some/absolutepath/somewhere/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/bsd.img
diff --git regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.conf 
regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.conf
new file mode 100644
index 000..3505def581b
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.conf
@@ -0,0 +1,12 @@
+#  $OpenBSD$
+# Fail on kernel keyword; has been replaced by boot.
+ramdisk="/bsd.rd"
+switch "sw" {
+add vether0
+}
+vm "x" {
+kernel $ramdisk
+memory 1G
+disable
+interface { switch "sw" }
+}
diff --git regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.ok 
regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.ok
new file mode 100644
index 000..348817b1477
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.ok
@@ -0,0 +1 @@
+8: syntax error
diff --git regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.conf 
regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.conf
new file mode 100644
index 000..f8b27056dea
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.conf
@@ -0,0 +1,5 @@
+#  $OpenBSD$
+# Fail on memory (less than 1MB)
+vm "x" {
+memory 1048575
+}
diff --git regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.ok 
regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.ok
new file mode 100644
index 000..0cf48a97eaf
--- /dev/null
+++ regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.ok
@@ -0,0 +1,2 @@
+size must be at least one megabyte
+4: failed to parse size: 1048575
diff --git

files.macppc: fix recursive dependency, parallel build race

2017-10-10 Thread Scott Cheloha 
Hi,

The attached patch eliminates a recursive dependency for
locore.o in the resultant kernel Makefile.

Without the patch you get this in the Makefile:

locore.o: $S/arch/macppc/macppc/locore.o

and these dependencies:

# make -p | egrep '^locore.o' | sed 's/.*://' | tr ' ' '\n'

/usr/src/sys/arch/macppc/macppc/locore.S
assym.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/assym.h
/usr/src/sys/sys/syscall.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/asm.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/asm.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/param.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/param.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/pmap.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/pmap.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/pte.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/pte.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/psl.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/psl.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/intr.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/intr.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/trap.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/trap.h
/usr/src/sys/arch/macppc/macppc/locore.o

With the patch you get this in the Makefile:

locore.o: $S/arch/macppc/macppc/locore.S

and the recursive dependency goes away:

# make -p | egrep '^locore.o' | sed 's/.*://' | tr ' ' '\n'

/usr/src/sys/arch/macppc/macppc/locore.S
assym.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/assym.h
/usr/src/sys/sys/syscall.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/asm.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/asm.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/param.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/param.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/pmap.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/pmap.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/pte.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/pte.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/psl.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/psl.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/intr.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/intr.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/machine/trap.h
/usr/src/sys/arch/macppc/compile/GENERIC.MP/obj/powerpc/trap.h

The recursive dependency also explains the error I've been
getting sporadically when building the kernel in parallel on
macppc for the last few months:

/usr/src/sys/arch/macppc/macppc/locore.S:35:19: error: assym.h: No such file or 
directory
*** Error 1 in target '/usr/src/sys/arch/macppc/macppc/locore.o'
*** Error 1 in /usr/src/sys/arch/macppc/compile/GENERIC.MP (Makefile:820 
'/usr/src/sys/arch/macppc/macppc/locore.o')

Thoughts/Feedback?

--
Scott Cheloha

Index: sys/arch/macppc/conf/files.macppc
===
RCS file: /cvs/src/sys/arch/macppc/conf/files.macppc,v
retrieving revision 1.88
diff -u -p -r1.88 files.macppc
--- sys/arch/macppc/conf/files.macppc   13 Jun 2017 01:44:27 -  1.88
+++ sys/arch/macppc/conf/files.macppc   11 Oct 2017 01:41:27 -
@@ -17,7 +17,7 @@ file  arch/macppc/macppc/mem.c
 file   arch/macppc/macppc/ofw_machdep.c
 file   arch/macppc/macppc/openfirm.c
 file   arch/macppc/macppc/openprom.c
-file   arch/macppc/macppc/locore.o
+file   arch/macppc/macppc/locore.S
 file   dev/cninit.c
 file   arch/macppc/macppc/ofwreal.S

[PATCH] innovations.html - use singular nop instead of plural nops

2017-10-10 Thread Raf Czlonka 
Hi all,

As per the subject - nops sequences -> nop sequences.

Regards,

Raf

Index: innovations.html
===
RCS file: /cvs/www/innovations.html,v
retrieving revision 1.53
diff -u -p -r1.53 innovations.html
--- innovations.html9 Oct 2017 16:02:38 -   1.53
+++ innovations.html11 Oct 2017 00:18:59 -
@@ -466,7 +466,7 @@ explained in detail in our

[PATCH] VMD: Remove switch on reload prior to re-creating

2017-10-10 Thread Carlos Cardenas 
Destroy switch on `vmctl reload` to allow SIOCBRDGADD to succeed
when creating new bridge and attaching interfaces to it.

Comments? Ok?

diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
index ef42549d105..0190c049837 100644
--- usr.sbin/vmd/priv.c
+++ usr.sbin/vmd/priv.c
@@ -88,6 +88,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, struct 
imsg *imsg)
switch (imsg->hdr.type) {
case IMSG_VMDOP_PRIV_IFDESCR:
case IMSG_VMDOP_PRIV_IFCREATE:
+   case IMSG_VMDOP_PRIV_IFDESTROY:
case IMSG_VMDOP_PRIV_IFRDOMAIN:
case IMSG_VMDOP_PRIV_IFADD:
case IMSG_VMDOP_PRIV_IFUP:
@@ -125,6 +126,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
struct imsg *imsg)
errno != EEXIST)
log_warn("SIOCIFCREATE");
break;
+   case IMSG_VMDOP_PRIV_IFDESTROY:
+   /* Destroy the bridge */
+   strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
+   if (ioctl(env->vmd_fd, SIOCIFDESTROY, ) < 0 &&
+   errno != EEXIST)
+   log_warn("SIOCIFDESTROY");
+   break;
case IMSG_VMDOP_PRIV_IFRDOMAIN:
strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
ifr.ifr_rdomainid = vfr.vfr_id;
@@ -444,6 +452,23 @@ vm_priv_brconfig(struct privsep *ps, struct vmd_switch 
*vsw)
return (0);
 }
 
+int
+vm_priv_brdestroy(struct privsep *ps, struct vmd_switch *vsw)
+{
+   struct vmop_ifreqvfr;
+
+   memset(, 0, sizeof(vfr));
+
+   if (strlcpy(vfr.vfr_name, vsw->sw_ifname,
+   sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
+   return (-1);
+
+   proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFDESTROY,
+   , sizeof(vfr));
+
+   return (0);
+}
+
 uint32_t
 vm_priv_addr(struct address *h, uint32_t vmid, int idx, int isvm)
 {
diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
index f1abc54d9a3..834654a76de 100644
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -852,7 +852,7 @@ int
 vmd_reload(unsigned int reset, const char *filename)
 {
struct vmd_vm   *vm, *next_vm;
-   struct vmd_switch   *vsw;
+   struct vmd_switch   *vsw, *next_vsw;
int  reload = 0;
 
/* Switch back to the default config file */
@@ -885,6 +885,19 @@ vmd_reload(unsigned int reset, const char *filename)
}
}
 
+   TAILQ_FOREACH_SAFE(vsw, env->vmd_switches,
+   sw_entry, next_vsw) {
+   log_debug("%s: removing %s",
+   __func__, vsw->sw_ifname);
+   if (vm_priv_brdestroy(>vmd_ps,
+   vsw) == -1 ) {
+   log_warn("%s: failed to destroy switch",
+   __func__);
+   }
+   TAILQ_REMOVE(env->vmd_switches, vsw, sw_entry);
+   free(vsw);
+   }
+
/* Update shared global configuration in all children */
if (config_setconfig(env) == -1)
return (-1);
diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
index 4b7b5f70495..fce5fcaaa60 100644
--- usr.sbin/vmd/vmd.h
+++ usr.sbin/vmd/vmd.h
@@ -100,6 +100,7 @@ enum imsg_type {
IMSG_VMDOP_PRIV_IFGROUP,
IMSG_VMDOP_PRIV_IFADDR,
IMSG_VMDOP_PRIV_IFRDOMAIN,
+   IMSG_VMDOP_PRIV_IFDESTROY,
IMSG_VMDOP_VM_SHUTDOWN,
IMSG_VMDOP_VM_REBOOT,
IMSG_VMDOP_CONFIG
@@ -323,6 +324,7 @@ int  priv_findname(const char *, const char **);
 int priv_validgroup(const char *);
 int vm_priv_ifconfig(struct privsep *, struct vmd_vm *);
 int vm_priv_brconfig(struct privsep *, struct vmd_switch *);
+int vm_priv_brdestroy(struct privsep *, struct vmd_switch *);
 uint32_t vm_priv_addr(struct address *, uint32_t, int, int);
 
 /* vmm.c */
-- 
2.14.2

[PATCH 0/2] VMD: handle VM termination error conditions better

2017-10-10 Thread Carlos Cardenas 
This patch series handles two error conditions when
terminating a VM (via vmctl or from within the VM).

Comments? Ok?

-- 
2.14.2

[PATCH 1/2] VMD: Prevent Disappearing VM from vmctl status

2017-10-10 Thread Carlos Cardenas 
Remove terminate_vm/vm_remove logic from vmm_dispatch_parent. This
logic is present in vmm_sighdlr when a VM process has signaled
SIGCHLD for proper cleanup.

diff --git usr.sbin/vmd/vmm.c usr.sbin/vmd/vmm.c
index ccd7680b479..8cc1c15157a 100644
--- usr.sbin/vmd/vmm.c
+++ usr.sbin/vmd/vmm.c
@@ -170,15 +170,13 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, 
struct imsg *imsg)
else
res = 0;
} else {
-   /* in the process of shutting down... */
-   log_debug("%s: performing a forced shutdown",
-   __func__);
+   /*
+* VM is currently being shutdown.
+* Check to see if the VM process is still
+* active.  If not, return VMD_VM_STOP_INVALID.
+*/
vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm);
-   /* ensure vm_id isn't 0 */
-   if (vtp.vtp_vm_id != 0) {
-   res = terminate_vm();
-   vm_remove(vm);
-   } else {
+   if (vtp.vtp_vm_id == 0) {
log_debug("%s: no vm running anymore",
__func__);
res = VMD_VM_STOP_INVALID;
-- 
2.14.2

[PATCH 2/2] VMD: Handle vm-induced powerdown better

2017-10-10 Thread Carlos Cardenas 
The VMD parent process didn't handle the case of a VM exiting
with a non 0 return properly (i.e. EIO).

diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
index f1abc54d9a3..dcff6de0c0e 100644
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -394,11 +394,14 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struct 
imsg *imsg)
case IMSG_VMDOP_TERMINATE_VM_EVENT:
IMSG_SIZE_CHECK(imsg, );
memcpy(, imsg->data, sizeof(vmr));
-   log_debug("%s: handling TERMINATE_EVENT for vm id %d",
-   __func__, vmr.vmr_id);
-   if ((vm = vm_getbyvmid(vmr.vmr_id)) == NULL)
+   log_debug("%s: handling TERMINATE_EVENT for vm id %d ret %d",
+   __func__, vmr.vmr_id, vmr.vmr_result);
+   if ((vm = vm_getbyvmid(vmr.vmr_id)) == NULL) {
+   log_debug("%s: vm %d is no longer available",
+   __func__, vmr.vmr_id);
break;
-   if (vmr.vmr_result == 0) {
+   }
+   if (vmr.vmr_result != EAGAIN) {
if (vm->vm_from_config) {
log_debug("%s: about to stop vm id %d",
__func__, vm->vm_vmid);
@@ -408,7 +411,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struct 
imsg *imsg)
__func__, vm->vm_vmid);
vm_remove(vm);
}
-   } else if (vmr.vmr_result == EAGAIN) {
+   } else {
/* Stop VM instance but keep the tty open */
log_debug("%s: about to stop vm id %d with tty open",
__func__, vm->vm_vmid);
-- 
2.14.2

[Patch] man umb(4) added Sierra Wireless EM7345

2017-10-10 Thread Christoph R. Murauer 
Hello !

Sorry if something is missing (to keep things shorter I did not add a
dmesg) or wrong, but I am not a coder.

Index: umb.4
===
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.7
diff -u -p -r1.7 umb.4
--- umb.4   25 Nov 2016 21:01:17 -  1.7
+++ umb.4   10 Oct 2017 22:03:02 -
@@ -46,6 +46,7 @@ The following devices should work:
 .Bl -tag -width Ds -offset indent -compact
 .It Ericsson H5321gw and N5321gw
 .It Medion Mobile S4222 (MediaTek OEM)
+.It Sierra Wireless EM7345
 .It Sierra Wireless EM7455
 .It Sierra Wireless EM8805
 .It Sierra Wireless MC8305


I upgraded today a Ericsson N5321gw to a Sierra Wireless EM7345 4G LTE
and can confirm, that booth cards work. The N5321gw in 3G (is 3G only)
and the EM7345 in LTE mode (here in Europe).

$ usbdevs
addr 1: xHCI root hub, Intel
addr 2: EMV Smartcard Reader, Generic
addr 3: VFS5011 Fingerprint Reader, Validity Sensors
addr 4: Sierra Wireless EM7345 4G LTE, Sierra Wireless Inc.
addr 5: product 0x07dc, Intel
addr 6: Integrated Camera, SunplusIT INC.
addr 7: product 0x5010, vendor 0x0765
addr 1: EHCI root hub, Intel
addr 2: Rate Matching Hub, Intel
addr 1: EHCI root hub, Intel
addr 2: Rate Matching Hub, Intel

$ ifconfig umb0
umb0: flags=8851 mtu 1500
  index 5 priority 0 llprio 3
  roaming disabled registration home network
  state up cell-class LTE rssi -107dBm speed 47.7Mps up 95.4Mps down
  SIM initialized PIN valid (3 attempts left)
  subscriber-id SECRET ICC-id SECRET provider T-Mobile A
  device XMM7160_V1.2_MBIM_GNSS_NAND_RE IMEI SECRET firmware
FIH7160_V1.2_WW_01.1415.07
  phone# +43650SECRET
  dns 213.162.69.1 213.162.69.169
  groups: egress
  status: active
  inet 10.9.122.124 --> 10.9.122.1 netmask 0xff00

OT : Would it people help to add in the UMB section of man ifconfig(8)
the information, that the APN username and password which some
provider gave to their customers are not need with umb(4) ?

Regards,

Christoph

inteldrm: reduce i2c busy-wait to 100us

2017-10-10 Thread joshua stein 
On my Kaby Lake laptop, running xbacklight or xrandr causes the
audio and mouse cursor to pause briefly.  This is because those
codepaths do DRM operations that have to probe all of the available
connectors, even disconnected ones.  For HDMI, it does i2c
operations that have to timeout.

Reducing the DELAY() calls to 100us avoids this, while still making
HDMI output work when it's actually connected.

intel_gmbus_exec appears to be an OpenBSD-specific function.


Index: sys/dev/pci/drm/i915/intel_i2c.c
===
RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_i2c.c,v
retrieving revision 1.12
diff -u -p -u -p -r1.12 intel_i2c.c
--- sys/dev/pci/drm/i915/intel_i2c.c30 Sep 2017 07:36:56 -  1.12
+++ sys/dev/pci/drm/i915/intel_i2c.c10 Oct 2017 20:32:59 -
@@ -231,7 +231,7 @@ intel_gmbus_exec(void *cookie, i2c_op_t 
st = I915_READ(GMBUS2);
if (st & (GMBUS_SATOER | GMBUS_HW_RDY))
break;
-   DELAY(1000);
+   DELAY(100);
}
if (st & GMBUS_SATOER) {
bus_err = 1;
@@ -254,7 +254,7 @@ intel_gmbus_exec(void *cookie, i2c_op_t 
st = I915_READ(GMBUS2);
if (st & (GMBUS_SATOER | GMBUS_HW_RDY))
break;
-   DELAY(1000);
+   DELAY(100);
}
if (st & GMBUS_SATOER) {
bus_err = 1;
@@ -279,7 +279,7 @@ out:
bus_err = 1;
if ((st & GMBUS_ACTIVE) == 0)
break;
-   DELAY(1000);
+   DELAY(100);
}
if (st & GMBUS_ACTIVE)
return (ETIMEDOUT);

Re: rw locks vs memory barriers and adaptive spinning

2017-10-10 Thread Mateusz Guzik 
On Tue, Oct 10, 2017 at 10:15:48AM +0200, Martin Pieuchot wrote:
> Hello Mateusz,
>
> On 09/10/17(Mon) 21:43, Mateusz Guzik wrote:
> > I was looking at rw lock code out of curiosity and noticed you always do
> > membar_enter which on MP-enabled amd64 kernel translates to mfence.
> > This makes the entire business a little bit slower.
> >
> > Interestingly you already have relevant macros for amd64:
> > #define membar_enter_after_atomic() __membar("")
> > #define membar_exit_before_atomic() __membar("")
>
> If you look at the history, you'll see that theses macro didn't exist
> when membar_* where added to rw locks.
>

I know. Addition of such a macro sounds like an accelent opportunity to
examine existing users. I figured rw locks were ommitted by accident
as the original posting explicitly mentions mutexes.

https://marc.info/?l=openbsd-tech=149555411827749=2

> > And there is even a default variant for archs which don't provide one.
> > I guess the switch should be easy.
>
> Then please do it :)  At lot of stuff is easy on OpenBSD but we are not
> enough.
>
> Do it, test it, explain it, get oks and commit it 8)
>

As noted earlier the patch is trivial. I have no easy means to even
compile-test it though as I'm not using OpenBSD. Only had a look out of
curiosity.

Nonetheless, here is the diff which can be split in 2. One part deals
with barriers, another one cleans up rw_status.

This should not result in binary change on any arch apart from i386 and
amd64. On these 2 if there is a breakage, it's a braino of some sort.
The change in principle is definitely fine.

So:

Utilize membar_*{before,after}_atomic in rw locks.

On architectures whose atomic operations provide relevant barriers there
is no need for additional membar. In particular this is the case on
amd64 and i386.

Read from the lock only once in rw_status. The lock word is marked as
volatile which forces re-read on each access. There is no correctness
issue here, but the assembly is worse than it needs to be and in case
of contention this adds avoidable cache traffic.

diff --git a/sys/kern/kern_rwlock.c b/sys/kern/kern_rwlock.c
index e2c3cae..5551835 100644
--- a/sys/kern/kern_rwlock.c
+++ b/sys/kern/kern_rwlock.c
@@ -96,7 +96,7 @@ _rw_enter_read(struct rwlock *rwl LOCK_FL_VARS)
rw_cas(>rwl_owner, owner, owner + RWLOCK_READ_INCR)))
_rw_enter(rwl, RW_READ LOCK_FL_ARGS);
else {
-   membar_enter();
+   membar_enter_after_atomic();
WITNESS_CHECKORDER(>rwl_lock_obj, LOP_NEWORDER, file,
line,
NULL);
WITNESS_LOCK(>rwl_lock_obj, 0, file, line);
@@ -112,7 +112,7 @@ _rw_enter_write(struct rwlock *rwl LOCK_FL_VARS)
RW_PROC(p) | RWLOCK_WRLOCK)))
_rw_enter(rwl, RW_WRITE LOCK_FL_ARGS);
else {
-   membar_enter();
+   membar_enter_after_atomic();
WITNESS_CHECKORDER(>rwl_lock_obj,
LOP_EXCLUSIVE | LOP_NEWORDER, file, line, NULL);
WITNESS_LOCK(>rwl_lock_obj, LOP_EXCLUSIVE, file, line);
@@ -126,7 +126,7 @@ _rw_exit_read(struct rwlock *rwl LOCK_FL_VARS)

rw_assert_rdlock(rwl);

-   membar_exit();
+   membar_exit_before_atomic();
if (__predict_false((owner & RWLOCK_WAIT) ||
rw_cas(>rwl_owner, owner, owner - RWLOCK_READ_INCR)))
_rw_exit(rwl LOCK_FL_ARGS);
@@ -141,7 +141,7 @@ _rw_exit_write(struct rwlock *rwl LOCK_FL_VARS)

rw_assert_wrlock(rwl);

-   membar_exit();
+   membar_exit_before_atomic();
if (__predict_false((owner & RWLOCK_WAIT) ||
rw_cas(>rwl_owner, owner, 0)))
_rw_exit(rwl LOCK_FL_ARGS);
@@ -261,7 +261,7 @@ retry:

if (__predict_false(rw_cas(>rwl_owner, o, o + inc)))
goto retry;
-   membar_enter();
+   membar_enter_after_atomic();

/*
 * If old lock had RWLOCK_WAIT and RWLOCK_WRLOCK set, it means we
@@ -295,7 +295,7 @@ _rw_exit(struct rwlock *rwl LOCK_FL_VARS)
WITNESS_UNLOCK(>rwl_lock_obj, wrlock ? LOP_EXCLUSIVE : 0,
file, line);

-   membar_exit();
+   membar_exit_before_atomic();
do {
owner = rwl->rwl_owner;
if (wrlock)
@@ -312,13 +312,15 @@ _rw_exit(struct rwlock *rwl LOCK_FL_VARS)
 int
 rw_status(struct rwlock *rwl)
 {
-   if (rwl->rwl_owner & RWLOCK_WRLOCK) {
-   if (RW_PROC(curproc) == RW_PROC(rwl->rwl_owner))
+   unsigned long owner = rwl->rwl_owner;
+
+   if (owner & RWLOCK_WRLOCK) {
+   if (RW_PROC(curproc) == RW_PROC(owner))
return RW_WRITE;
else
return RW_WRITE_OTHER;
}
-   if (rwl->rwl_owner)
+   if (owner)
return RW_READ;
return (0);
 }

-- 
Mateusz Guzik

Re: Move 'struct kevent' storage to the stack

2017-10-10 Thread Alexander Bluhm 
On Mon, Oct 09, 2017 at 11:48:56AM +0200, Martin Pieuchot wrote:
> The diff below move 'struct kevent' storage to the stack.  This is
> the simplest way to make it mp-safe.
> 
> This has been tested as part of a larger diff by many.

I am currently running the regression tests with it.

> Ok?

OK bluhm@

> 
> Index: kern/kern_event.c
> ===
> RCS file: /cvs/src/sys/kern/kern_event.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 kern_event.c
> --- kern/kern_event.c 31 May 2017 14:52:05 -  1.79
> +++ kern/kern_event.c 9 Oct 2017 09:02:48 -
> @@ -476,6 +476,7 @@ sys_kevent(struct proc *p, void *v, regi
>   struct file *fp;
>   struct timespec ts;
>   int i, n, nerrors, error;
> + struct kevent kev[KQ_NEVENTS];
>  
>   if ((fp = fd_getfile(fdp, SCARG(uap, fd))) == NULL ||
>   (fp->f_type != DTYPE_KQUEUE))
> @@ -500,16 +501,16 @@ sys_kevent(struct proc *p, void *v, regi
>   while (SCARG(uap, nchanges) > 0) {
>   n = SCARG(uap, nchanges) > KQ_NEVENTS ?
>   KQ_NEVENTS : SCARG(uap, nchanges);
> - error = copyin(SCARG(uap, changelist), kq->kq_kev,
> + error = copyin(SCARG(uap, changelist), kev,
>   n * sizeof(struct kevent));
>   if (error)
>   goto done;
>  #ifdef KTRACE
>   if (KTRPOINT(p, KTR_STRUCT))
> - ktrevent(p, kq->kq_kev, n);
> + ktrevent(p, kev, n);
>  #endif
>   for (i = 0; i < n; i++) {
> - kevp = >kq_kev[i];
> + kevp = [i];
>   kevp->flags &= ~EV_SYSFLAGS;
>   error = kqueue_register(kq, kevp, p);
>   if (error || (kevp->flags & EV_RECEIPT)) {
> @@ -691,6 +692,7 @@ kqueue_scan(struct kqueue *kq, int maxev
>   struct timeval atv, rtv, ttv;
>   struct knote *kn, marker;
>   int s, count, timeout, nkev = 0, error = 0;
> + struct kevent kev[KQ_NEVENTS];
>  
>   count = maxevents;
>   if (count == 0)
> @@ -737,7 +739,7 @@ start:
>   goto done;
>   }
>  
> - kevp = kq->kq_kev;
> + kevp = [0];
>   s = splhigh();
>   if (kq->kq_count == 0) {
>   if (timeout < 0) {
> @@ -805,13 +807,13 @@ start:
>   splx(s);
>  #ifdef KTRACE
>   if (KTRPOINT(p, KTR_STRUCT))
> - ktrevent(p, kq->kq_kev, nkev);
> + ktrevent(p, kev, nkev);
>  #endif
> - error = copyout(kq->kq_kev, ulistp,
> + error = copyout(kev, ulistp,
>   sizeof(struct kevent) * nkev);
>   ulistp += nkev;
>   nkev = 0;
> - kevp = kq->kq_kev;
> + kevp = [0];
>   s = splhigh();
>   if (error)
>   break;
> @@ -823,9 +825,9 @@ done:
>   if (nkev != 0) {
>  #ifdef KTRACE
>   if (KTRPOINT(p, KTR_STRUCT))
> - ktrevent(p, kq->kq_kev, nkev);
> + ktrevent(p, kev, nkev);
>  #endif
> - error = copyout(kq->kq_kev, ulistp,
> + error = copyout(kev, ulistp,
>   sizeof(struct kevent) * nkev);
>   }
>   *retval = maxevents - count;
> Index: sys/eventvar.h
> ===
> RCS file: /cvs/src/sys/sys/eventvar.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 eventvar.h
> --- sys/eventvar.h25 Mar 2012 20:33:52 -  1.3
> +++ sys/eventvar.h9 Oct 2017 09:02:48 -
> @@ -44,7 +44,6 @@ struct kqueue {
>  #define KQ_SEL   0x01
>  #define KQ_SLEEP 0x02
>  #define KQ_DYING 0x04
> - struct  kevent kq_kev[KQ_NEVENTS];
>  };
>  
>  #endif /* !_SYS_EVENTVAR_H_ */

Re: kq_count earlier!

2017-10-10 Thread Alexander Bluhm 
On Mon, Oct 09, 2017 at 12:15:46PM +0200, Martin Pieuchot wrote:
> To prevent an infinite loop, threads looking for events inside
> kqueue_scan(), insert a `marker' in the list.  Such markers are
> not accounted and they are removed from the list as soon as the
> thread is finished or goes to sleep.
> 
> Diff below is a small cleanup to keep the accounting of events in
> sync with the number of events on the list.  This is a noop for the
> moment, but it's small & easy part to review of my upcoming diff.
> 
> ok?

OK bluhm@

> 
> Index: kern/kern_event.c
> ===
> RCS file: /cvs/src/sys/kern/kern_event.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 kern_event.c
> --- kern/kern_event.c 31 May 2017 14:52:05 -  1.79
> +++ kern/kern_event.c 9 Oct 2017 09:57:54 -
> @@ -760,22 +760,24 @@ start:
>   TAILQ_INSERT_TAIL(>kq_head, , kn_tqe);
>   while (count) {
>   kn = TAILQ_FIRST(>kq_head);
> - TAILQ_REMOVE(>kq_head, kn, kn_tqe);
>   if (kn == ) {
> + TAILQ_REMOVE(>kq_head, kn, kn_tqe);
>   splx(s);
>   if (count == maxevents)
>   goto retry;
>   goto done;
>   }
> +
> + TAILQ_REMOVE(>kq_head, kn, kn_tqe);
> + kq->kq_count--;
> +
>   if (kn->kn_status & KN_DISABLED) {
>   kn->kn_status &= ~KN_QUEUED;
> - kq->kq_count--;
>   continue;
>   }
>   if ((kn->kn_flags & EV_ONESHOT) == 0 &&
>   kn->kn_fop->f_event(kn, 0) == 0) {
>   kn->kn_status &= ~(KN_QUEUED | KN_ACTIVE);
> - kq->kq_count--;
>   continue;
>   }
>   *kevp = kn->kn_kevent;
> @@ -783,7 +785,6 @@ start:
>   nkev++;
>   if (kn->kn_flags & EV_ONESHOT) {
>   kn->kn_status &= ~KN_QUEUED;
> - kq->kq_count--;
>   splx(s);
>   kn->kn_fop->f_detach(kn);
>   knote_drop(kn, p, p->p_fd);
> @@ -796,9 +797,9 @@ start:
>   if (kn->kn_flags & EV_DISPATCH)
>   kn->kn_status |= KN_DISABLED;
>   kn->kn_status &= ~(KN_QUEUED | KN_ACTIVE);
> - kq->kq_count--;
>   } else {
>   TAILQ_INSERT_TAIL(>kq_head, kn, kn_tqe);
> + kq->kq_count++;
>   }
>   count--;
>   if (nkev == KQ_NEVENTS) {

Re: ifioctl() cleanup

2017-10-10 Thread Alexander Bluhm 
On Tue, Oct 10, 2017 at 03:55:30PM +0200, Martin Pieuchot wrote:
> Similar diff without factorizing privilege checks, deraadt@ pointed out
> it is too fragile.
> 
> ok?

OK bluhm@

> 
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.513
> diff -u -p -r1.513 if.c
> --- net/if.c  9 Oct 2017 08:35:38 -   1.513
> +++ net/if.c  10 Oct 2017 13:46:43 -
> @@ -1816,10 +1816,9 @@ int
>  ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
>  {
>   struct ifnet *ifp;
> - struct ifreq *ifr;
> - struct sockaddr_dl *sdl;
> + struct ifreq *ifr = (struct ifreq *)data;
>   struct ifgroupreq *ifgr;
> - struct if_afreq *ifar;
> + struct if_afreq *ifar = (struct if_afreq *)data;
>   char ifdescrbuf[IFDESCRSIZE];
>   char ifrtlabelbuf[RTLABEL_LEN];
>   int s, error = 0, oif_xflags;
> @@ -1828,13 +1827,8 @@ ifioctl(struct socket *so, u_long cmd, c
>   const char *label;
>  
>   switch (cmd) {
> -
>   case SIOCGIFCONF:
>   return (ifconf(cmd, data));
> - }
> - ifr = (struct ifreq *)data;
> -
> - switch (cmd) {
>   case SIOCIFCREATE:
>   case SIOCIFDESTROY:
>   if ((error = suser(p, 0)) != 0)
> @@ -1852,15 +1846,19 @@ ifioctl(struct socket *so, u_long cmd, c
>   if ((error = suser(p, 0)) != 0)
>   return (error);
>   return (if_setgroupattribs(data));
> + }
> +
> + ifp = ifunit(ifr->ifr_name);
> + if (ifp == NULL)
> + return (ENXIO);
> + oif_flags = ifp->if_flags;
> + oif_xflags = ifp->if_xflags;
> +
> + switch (cmd) {
>   case SIOCIFAFATTACH:
>   case SIOCIFAFDETACH:
>   if ((error = suser(p, 0)) != 0)
>   return (error);
> - ifar = (struct if_afreq *)data;
> - if ((ifp = ifunit(ifar->ifar_name)) == NULL)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
> - oif_xflags = ifp->if_xflags;
>   switch (ifar->ifar_af) {
>   case AF_INET:
>   /* attach is a noop for AF_INET */
> @@ -1878,16 +1876,7 @@ ifioctl(struct socket *so, u_long cmd, c
>   default:
>   return (EAFNOSUPPORT);
>   }
> - if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
> - rtm_ifchg(ifp);
> - return (error);
> - }
> -
> - ifp = ifunit(ifr->ifr_name);
> - if (ifp == 0)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
> - switch (cmd) {
> + break;
>  
>   case SIOCGIFFLAGS:
>   ifr->ifr_flags = ifp->if_flags;
> @@ -2002,7 +1991,6 @@ ifioctl(struct socket *so, u_long cmd, c
>  
>   ifp->if_xflags = (ifp->if_xflags & IFXF_CANTCHANGE) |
>   (ifr->ifr_flags & ~IFXF_CANTCHANGE);
> - rtm_ifchg(ifp);
>   break;
>  
>   case SIOCSIFMETRIC:
> @@ -2140,8 +2128,7 @@ ifioctl(struct socket *so, u_long cmd, c
>   case SIOCSIFLLADDR:
>   if ((error = suser(p, 0)))
>   return (error);
> - sdl = ifp->if_sadl;
> - if (sdl == NULL)
> + if (ifp->if_sadl == NULL)
>   return (EINVAL);
>   if (ifr->ifr_addr.sa_len != ETHER_ADDR_LEN)
>   return (EINVAL);
> @@ -2182,6 +2169,9 @@ ifioctl(struct socket *so, u_long cmd, c
>   (struct mbuf *) ifp, p));
>   break;
>   }
> +
> + if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
> + rtm_ifchg(ifp);
>  
>   if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0)
>   getmicrotime(>if_lastchange);

[patch] fix sys/dev/video.c debug printf formats

2017-10-10 Thread Dave Voutila 
Hi tech,

I was debugging some webcam issues and received compiler errors when
building with the VIDEO_DEBUG option due to printf patterns in the expanded 
DPRINTF macro.

See below patch for updating the formats in the DPRINTF usage. I've tested on 
i386 and amd64.

Index: sys/dev/video.c
===
RCS file: /cvs/src/sys/dev/video.c,v
retrieving revision 1.40
diff -u -p -u -p -r1.40 video.c
--- sys/dev/video.c 3 Jul 2016 20:05:44 -   1.40
+++ sys/dev/video.c 10 Oct 2017 15:25:39 -
@@ -178,7 +178,7 @@ videoread(dev_t dev, struct uio *uio, in
sc->sc_vidmode = VIDMODE_READ;
}
  
-   DPRINTF(("resid=%d\n", uio->uio_resid));
+   DPRINTF(("resid=%zu\n", uio->uio_resid));
 
if (sc->sc_frames_ready < 1) {
/* block userland read until a frame is ready */
@@ -212,8 +212,8 @@ videoioctl(dev_t dev, u_long cmd, caddr_
(sc = video_cd.cd_devs[unit]) == NULL || sc->hw_if == NULL)
return (ENXIO);
 
-   DPRINTF(("video_ioctl(%d, '%c', %d)\n",
-   IOCPARM_LEN(cmd), IOCGROUP(cmd), cmd & 0xff));
+   DPRINTF(("video_ioctl(%zu, '%c', %zu)\n",
+   IOCPARM_LEN(cmd), (int) IOCGROUP(cmd), cmd & 0xff));
 
error = EOPNOTSUPP;
switch (cmd) {
@@ -389,7 +389,7 @@ videommap(dev_t dev, off_t off, int prot
caddr_t p;
paddr_t pa;
 
-   DPRINTF(("%s: off=%d, prot=%d\n", __func__, off, prot));
+   DPRINTF(("%s: off=%lld, prot=%d\n", __func__, off, prot));
 
unit = VIDEOUNIT(dev);
if (unit >= video_cd.cd_ndevs ||

Re: pow() returns a negative result on loongson

2017-10-10 Thread Juan Francisco Cantero Hurtado 
On Tue, Oct 10, 2017 at 12:08:06PM +, Visa Hankala wrote:
> On Mon, Oct 09, 2017 at 10:39:47PM +0200, Juan Francisco Cantero Hurtado 
> wrote:
> > Marc Feeley (Gambit Scheme) has been helping me with a bug on Gambit on
> > Loongson. Apparently the bug is on our side.
> > 
> > I've created this little test based on his code:
> > 
> > #include 
> > #include 
> > 
> > int main()
> > {
> > double x = 0.5;
> > double y = 1074.0;
> > printf("x=%.20g y=%.20g pow(x,y)=%.20g\n",x,y,pow(x,y));
> > return 0;
> > }
> > 
> > On OpenBSD/amd64, Linux/amd64, Linux/aarch64 and Linux/mips64:
> > x=0.5 y=1074 pow(x,y)=4.9406564584124654418e-324
> > 
> > On OpenBSD/loongson:
> > x=0.5 y=1074 pow(x,y)=-4.9406564584124654418e-324
> > 
> > Is the negative result expected?
> 
> No. On mips64, ldexp(3) seems to use a garbage value when figuring out
> the sign of a denormal value. This applies to infinity values as well.
> 
> The patch below should fix the bug. The t3 register contains the binary
> representation of the floating-point `x' parameter. I can also make
> a regression test for the patch.
> 
> OK?

Works for me. Thanks for the quick fix.

> 
> Index: arch/mips64/gen/ldexp.S
> ===
> RCS file: src/lib/libc/arch/mips64/gen/ldexp.S,v
> retrieving revision 1.7
> diff -u -p -r1.7 ldexp.S
> --- arch/mips64/gen/ldexp.S   27 Oct 2015 05:54:49 -  1.7
> +++ arch/mips64/gen/ldexp.S   10 Oct 2017 11:52:56 -
> @@ -143,20 +143,20 @@ LEAF(ldexp, 0)
>   xorit2, 1
>  1:
>   dmtc1   t2, $f0 # save denormalized result (LSW)
> - bge v1, zero, 1f# should result be negative?
> + bge t3, zero, 1f# should result be negative?
>   neg.d   $f0, $f0# negate result
>  1:
>   j   ra
>  7:
>   dmtc1   zero, $f0   # result is zero
> - beq t0, zero, 1f# is result positive?
> + bge t3, zero, 1f# is result positive?
>   neg.d   $f0, $f0# negate result
>  1:
>   j   ra
>  8:
>   dli t1, 0x7ff0  # result is infinity (MSW)
>   dmtc1   t1, $f0 
> - bge v1, zero, 1f# should result be negative infinity?
> + bge t3, zero, 1f# should result be negative infinity?
>   neg.d   $f0, $f0# result is negative infinity
>  1:
>   add.d   $f0, $f0# cause overflow faults if enabled
> 

-- 
Juan Francisco Cantero Hurtado http://juanfra.info

Re: ifioctl() cleanup

2017-10-10 Thread Martin Pieuchot 
On 09/10/17(Mon) 16:34, Martin Pieuchot wrote:
> Here's a small cleanup to make it easy to push the NET_LOCK() further
> down.
> 
> Diff below factorize all ioctl requests need root privileges in one
> switch () block.
> 
> It moves SIOCIFAFATTACH/SIOCIFAFDETACH where it belongs, to the block
> that keeps track of the timestamps of the last change.
> 
> It gets rid of unnecessary `sdl' variable.

Similar diff without factorizing privilege checks, deraadt@ pointed out
it is too fragile.

ok?

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.513
diff -u -p -r1.513 if.c
--- net/if.c9 Oct 2017 08:35:38 -   1.513
+++ net/if.c10 Oct 2017 13:46:43 -
@@ -1816,10 +1816,9 @@ int
 ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
 {
struct ifnet *ifp;
-   struct ifreq *ifr;
-   struct sockaddr_dl *sdl;
+   struct ifreq *ifr = (struct ifreq *)data;
struct ifgroupreq *ifgr;
-   struct if_afreq *ifar;
+   struct if_afreq *ifar = (struct if_afreq *)data;
char ifdescrbuf[IFDESCRSIZE];
char ifrtlabelbuf[RTLABEL_LEN];
int s, error = 0, oif_xflags;
@@ -1828,13 +1827,8 @@ ifioctl(struct socket *so, u_long cmd, c
const char *label;
 
switch (cmd) {
-
case SIOCGIFCONF:
return (ifconf(cmd, data));
-   }
-   ifr = (struct ifreq *)data;
-
-   switch (cmd) {
case SIOCIFCREATE:
case SIOCIFDESTROY:
if ((error = suser(p, 0)) != 0)
@@ -1852,15 +1846,19 @@ ifioctl(struct socket *so, u_long cmd, c
if ((error = suser(p, 0)) != 0)
return (error);
return (if_setgroupattribs(data));
+   }
+
+   ifp = ifunit(ifr->ifr_name);
+   if (ifp == NULL)
+   return (ENXIO);
+   oif_flags = ifp->if_flags;
+   oif_xflags = ifp->if_xflags;
+
+   switch (cmd) {
case SIOCIFAFATTACH:
case SIOCIFAFDETACH:
if ((error = suser(p, 0)) != 0)
return (error);
-   ifar = (struct if_afreq *)data;
-   if ((ifp = ifunit(ifar->ifar_name)) == NULL)
-   return (ENXIO);
-   oif_flags = ifp->if_flags;
-   oif_xflags = ifp->if_xflags;
switch (ifar->ifar_af) {
case AF_INET:
/* attach is a noop for AF_INET */
@@ -1878,16 +1876,7 @@ ifioctl(struct socket *so, u_long cmd, c
default:
return (EAFNOSUPPORT);
}
-   if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
-   rtm_ifchg(ifp);
-   return (error);
-   }
-
-   ifp = ifunit(ifr->ifr_name);
-   if (ifp == 0)
-   return (ENXIO);
-   oif_flags = ifp->if_flags;
-   switch (cmd) {
+   break;
 
case SIOCGIFFLAGS:
ifr->ifr_flags = ifp->if_flags;
@@ -2002,7 +1991,6 @@ ifioctl(struct socket *so, u_long cmd, c
 
ifp->if_xflags = (ifp->if_xflags & IFXF_CANTCHANGE) |
(ifr->ifr_flags & ~IFXF_CANTCHANGE);
-   rtm_ifchg(ifp);
break;
 
case SIOCSIFMETRIC:
@@ -2140,8 +2128,7 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCSIFLLADDR:
if ((error = suser(p, 0)))
return (error);
-   sdl = ifp->if_sadl;
-   if (sdl == NULL)
+   if (ifp->if_sadl == NULL)
return (EINVAL);
if (ifr->ifr_addr.sa_len != ETHER_ADDR_LEN)
return (EINVAL);
@@ -2182,6 +2169,9 @@ ifioctl(struct socket *so, u_long cmd, c
(struct mbuf *) ifp, p));
break;
}
+
+   if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
+   rtm_ifchg(ifp);
 
if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0)
getmicrotime(>if_lastchange);

Re: pow() returns a negative result on loongson

2017-10-10 Thread Visa Hankala 
On Mon, Oct 09, 2017 at 10:39:47PM +0200, Juan Francisco Cantero Hurtado wrote:
> Marc Feeley (Gambit Scheme) has been helping me with a bug on Gambit on
> Loongson. Apparently the bug is on our side.
> 
> I've created this little test based on his code:
> 
> #include 
> #include 
> 
> int main()
> {
> double x = 0.5;
> double y = 1074.0;
> printf("x=%.20g y=%.20g pow(x,y)=%.20g\n",x,y,pow(x,y));
> return 0;
> }
> 
> On OpenBSD/amd64, Linux/amd64, Linux/aarch64 and Linux/mips64:
> x=0.5 y=1074 pow(x,y)=4.9406564584124654418e-324
> 
> On OpenBSD/loongson:
> x=0.5 y=1074 pow(x,y)=-4.9406564584124654418e-324
> 
> Is the negative result expected?

No. On mips64, ldexp(3) seems to use a garbage value when figuring out
the sign of a denormal value. This applies to infinity values as well.

The patch below should fix the bug. The t3 register contains the binary
representation of the floating-point `x' parameter. I can also make
a regression test for the patch.

OK?

Index: arch/mips64/gen/ldexp.S
===
RCS file: src/lib/libc/arch/mips64/gen/ldexp.S,v
retrieving revision 1.7
diff -u -p -r1.7 ldexp.S
--- arch/mips64/gen/ldexp.S 27 Oct 2015 05:54:49 -  1.7
+++ arch/mips64/gen/ldexp.S 10 Oct 2017 11:52:56 -
@@ -143,20 +143,20 @@ LEAF(ldexp, 0)
xorit2, 1
 1:
dmtc1   t2, $f0 # save denormalized result (LSW)
-   bge v1, zero, 1f# should result be negative?
+   bge t3, zero, 1f# should result be negative?
neg.d   $f0, $f0# negate result
 1:
j   ra
 7:
dmtc1   zero, $f0   # result is zero
-   beq t0, zero, 1f# is result positive?
+   bge t3, zero, 1f# is result positive?
neg.d   $f0, $f0# negate result
 1:
j   ra
 8:
dli t1, 0x7ff0  # result is infinity (MSW)
dmtc1   t1, $f0 
-   bge v1, zero, 1f# should result be negative infinity?
+   bge t3, zero, 1f# should result be negative infinity?
neg.d   $f0, $f0# result is negative infinity
 1:
add.d   $f0, $f0# cause overflow faults if enabled

Re: ifioctl() cleanup

2017-10-10 Thread Michele Curti 
On Mon, Oct 09, 2017 at 04:34:38PM +0200, Martin Pieuchot wrote:
> Here's a small cleanup to make it easy to push the NET_LOCK() further
> down.
> 
> Diff below factorize all ioctl requests need root privileges in one
> switch () block.
> 
> It moves SIOCIFAFATTACH/SIOCIFAFDETACH where it belongs, to the block
> that keeps track of the timestamps of the last change.
> 
> It gets rid of unnecessary `sdl' variable.
> 
> ok?

Before the cmds
SIOCAIFGROUP
SIOCDIFGROUP
SIOCSIFLLADDR
SIOCSIFLLPRIO
returned EPERM, with this patch 1.. Is this a problem?

Regards,
Michele

> 
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.513
> diff -u -p -r1.513 if.c
> --- net/if.c  9 Oct 2017 08:35:38 -   1.513
> +++ net/if.c  9 Oct 2017 14:27:35 -
> @@ -1816,10 +1816,9 @@ int
>  ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
>  {
>   struct ifnet *ifp;
> - struct ifreq *ifr;
> - struct sockaddr_dl *sdl;
> + struct ifreq *ifr = (struct ifreq *)data;
>   struct ifgroupreq *ifgr;
> - struct if_afreq *ifar;
> + struct if_afreq *ifar = (struct if_afreq *)data;
>   char ifdescrbuf[IFDESCRSIZE];
>   char ifrtlabelbuf[RTLABEL_LEN];
>   int s, error = 0, oif_xflags;
> @@ -1828,17 +1827,49 @@ ifioctl(struct socket *so, u_long cmd, c
>   const char *label;
>  
>   switch (cmd) {
> -
> - case SIOCGIFCONF:
> - return (ifconf(cmd, data));
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + case SIOCSIFGATTR:
> + case SIOCIFAFATTACH:
> + case SIOCIFAFDETACH:
> + case SIOCSIFFLAGS:
> + case SIOCSIFXFLAGS:
> + case SIOCSIFMETRIC:
> + case SIOCSIFMTU:
> + case SIOCSIFPHYADDR:
> + case SIOCDIFPHYADDR:
> +#ifdef INET6
> + case SIOCSIFPHYADDR_IN6:
> +#endif
> + case SIOCSLIFPHYADDR:
> + case SIOCSLIFPHYRTABLE:
> + case SIOCSLIFPHYTTL:
> + case SIOCADDMULTI:
> + case SIOCDELMULTI:
> + case SIOCSIFMEDIA:
> + case SIOCSVNETID:
> + case SIOCSIFPAIR:
> + case SIOCSIFPARENT:
> + case SIOCDIFPARENT:
> + case SIOCSIFDESCR:
> + case SIOCSIFRTLABEL:
> + case SIOCSIFPRIORITY:
> + case SIOCSIFRDOMAIN:
> + case SIOCAIFGROUP:
> + case SIOCDIFGROUP:
> + case SIOCSIFLLADDR:
> + case SIOCSIFLLPRIO:
> + if ((error = suser(p, 0)) != 0)
> + return (error);
> + default:
> + break;
>   }
> - ifr = (struct ifreq *)data;
>  
>   switch (cmd) {
> + case SIOCGIFCONF:
> + return (ifconf(cmd, data));
>   case SIOCIFCREATE:
>   case SIOCIFDESTROY:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
>   return ((cmd == SIOCIFCREATE) ?
>   if_clone_create(ifr->ifr_name, 0) :
>   if_clone_destroy(ifr->ifr_name));
> @@ -1849,17 +1880,17 @@ ifioctl(struct socket *so, u_long cmd, c
>   case SIOCGIFGATTR:
>   return (if_getgroupattribs(data));
>   case SIOCSIFGATTR:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
>   return (if_setgroupattribs(data));
> + }
> +
> + ifp = ifunit(ifr->ifr_name);
> + if (ifp == NULL)
> + return (ENXIO);
> + oif_flags = ifp->if_flags;
> +
> + switch (cmd) {
>   case SIOCIFAFATTACH:
>   case SIOCIFAFDETACH:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
> - ifar = (struct if_afreq *)data;
> - if ((ifp = ifunit(ifar->ifar_name)) == NULL)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
>   oif_xflags = ifp->if_xflags;
>   switch (ifar->ifar_af) {
>   case AF_INET:
> @@ -1880,15 +1911,7 @@ ifioctl(struct socket *so, u_long cmd, c
>   }
>   if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
>   rtm_ifchg(ifp);
> - return (error);
> - }
> -
> - ifp = ifunit(ifr->ifr_name);
> - if (ifp == 0)
> - return (ENXIO);
> - oif_flags = ifp->if_flags;
> - switch (cmd) {
> -
> + break;
>   case SIOCGIFFLAGS:
>   ifr->ifr_flags = ifp->if_flags;
>   if (ifq_is_oactive(>if_snd))
> @@ -1919,9 +1942,6 @@ ifioctl(struct socket *so, u_long cmd, c
>   }
>  
>   case SIOCSIFFLAGS:
> - if ((error = suser(p, 0)) != 0)
> - return (error);
> -
>   ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) |
>   (ifr->ifr_flags & ~IFF_CANTCHANGE);
>  
> @@ -1944,9 +1964,6 @@ ifioctl(struct socket *so, u_long cmd, c
>   break;
>  
>   case SIOCSIFXFLAGS:
> - if ((error = suser(p, 0)) != 0)
> -

Re: Additional media options for ix(4) [again]

2017-10-10 Thread Martin Pieuchot 
On 16/08/17(Wed) 15:55, Mike Belopuhov wrote:
> Hi,
> 
> I haven't gotten any feedback on the following diff
> but I think there's still hope.  Please test.
> 
> Original mail:
> 
> I won't mind some broad testing of the following diff
> which adds some additional media options to ix(4) from
> FreeBSD and includes a fix for changing media from
> Masanobu SAITOH.
> 
> The fix makes sure that when the media operation speed
> is selected manually, the device doesn't additionally
> advertise other (slower) modes.

Time to get this broadly tested by committing it?

> diff --git sys/dev/pci/if_ix.c sys/dev/pci/if_ix.c
> index 339ba2bc4f1..8fca8742f7f 100644
> --- sys/dev/pci/if_ix.c
> +++ sys/dev/pci/if_ix.c
> @@ -1028,62 +1028,115 @@ ixgbe_intr(void *arg)
>   *  This routine is called whenever the user queries the status of
>   *  the interface using ifconfig.
>   *
>   **/
>  void
> -ixgbe_media_status(struct ifnet * ifp, struct ifmediareq *ifmr)
> +ixgbe_media_status(struct ifnet *ifp, struct ifmediareq *ifmr)
>  {
>   struct ix_softc *sc = ifp->if_softc;
> + int layer;
> +
> + layer = sc->hw.mac.ops.get_supported_physical_layer(>hw);
>  
>   ifmr->ifm_active = IFM_ETHER;
>   ifmr->ifm_status = IFM_AVALID;
>  
>   INIT_DEBUGOUT("ixgbe_media_status: begin");
>   ixgbe_update_link_status(sc);
>  
> - if (LINK_STATE_IS_UP(ifp->if_link_state)) {
> - ifmr->ifm_status |= IFM_ACTIVE;
> + if (!LINK_STATE_IS_UP(ifp->if_link_state))
> + return;
> +
> + ifmr->ifm_status |= IFM_ACTIVE;
>  
> + if (layer & IXGBE_PHYSICAL_LAYER_10GBASE_T ||
> + layer & IXGBE_PHYSICAL_LAYER_1000BASE_T ||
> + layer & IXGBE_PHYSICAL_LAYER_100BASE_TX)
>   switch (sc->link_speed) {
> + case IXGBE_LINK_SPEED_10GB_FULL:
> + ifmr->ifm_active |= IFM_10G_T | IFM_FDX;
> + break;
> + case IXGBE_LINK_SPEED_1GB_FULL:
> + ifmr->ifm_active |= IFM_1000_T | IFM_FDX;
> + break;
>   case IXGBE_LINK_SPEED_100_FULL:
>   ifmr->ifm_active |= IFM_100_TX | IFM_FDX;
>   break;
> + }
> + if (layer & IXGBE_PHYSICAL_LAYER_SFP_PLUS_CU ||
> + layer & IXGBE_PHYSICAL_LAYER_SFP_ACTIVE_DA)
> + switch (sc->link_speed) {
> + case IXGBE_LINK_SPEED_10GB_FULL:
> + ifmr->ifm_active |= IFM_10G_SFP_CU | IFM_FDX;
> + break;
> + }
> + if (layer & IXGBE_PHYSICAL_LAYER_10GBASE_LR)
> + switch (sc->link_speed) {
> + case IXGBE_LINK_SPEED_10GB_FULL:
> + ifmr->ifm_active |= IFM_10G_LR | IFM_FDX;
> + break;
>   case IXGBE_LINK_SPEED_1GB_FULL:
> - switch (sc->optics) {
> - case IFM_10G_SR: /* multi-speed fiber */
> - ifmr->ifm_active |= IFM_1000_SX | IFM_FDX;
> - break;
> - case IFM_10G_LR: /* multi-speed fiber */
> - ifmr->ifm_active |= IFM_1000_LX | IFM_FDX;
> - break;
> - default:
> - ifmr->ifm_active |= sc->optics | IFM_FDX;
> - break;
> - }
> + ifmr->ifm_active |= IFM_1000_LX | IFM_FDX;
>   break;
> + }
> + if (layer & IXGBE_PHYSICAL_LAYER_10GBASE_LRM)
> + switch (sc->link_speed) {
>   case IXGBE_LINK_SPEED_10GB_FULL:
> - ifmr->ifm_active |= sc->optics | IFM_FDX;
> + ifmr->ifm_active |= IFM_10G_LRM | IFM_FDX;
> + break;
> + case IXGBE_LINK_SPEED_1GB_FULL:
> + ifmr->ifm_active |= IFM_1000_LX | IFM_FDX;
>   break;
>   }
> -
> - switch (sc->hw.fc.current_mode) {
> - case ixgbe_fc_tx_pause:
> - ifmr->ifm_active |= IFM_FLOW | IFM_ETH_TXPAUSE;
> + if (layer & IXGBE_PHYSICAL_LAYER_10GBASE_SR ||
> + layer & IXGBE_PHYSICAL_LAYER_1000BASE_SX)
> + switch (sc->link_speed) {
> + case IXGBE_LINK_SPEED_10GB_FULL:
> + ifmr->ifm_active |= IFM_10G_SR | IFM_FDX;
> + break;
> + case IXGBE_LINK_SPEED_1GB_FULL:
> + ifmr->ifm_active |= IFM_1000_SX | IFM_FDX;
>   break;
> - case ixgbe_fc_rx_pause:
> - ifmr->ifm_active |= IFM_FLOW | IFM_ETH_RXPAUSE;
> + }
> + if (layer & IXGBE_PHYSICAL_LAYER_10GBASE_CX4)
> + switch (sc->link_speed) {
> + case IXGBE_LINK_SPEED_10GB_FULL:
> +

Re: rw locks vs memory barriers and adaptive spinning

2017-10-10 Thread Martin Pieuchot 
Hello Mateusz,

On 09/10/17(Mon) 21:43, Mateusz Guzik wrote:
> I was looking at rw lock code out of curiosity and noticed you always do
> membar_enter which on MP-enabled amd64 kernel translates to mfence.
> This makes the entire business a little bit slower.
> 
> Interestingly you already have relevant macros for amd64:
> #define membar_enter_after_atomic() __membar("")
> #define membar_exit_before_atomic() __membar("")

If you look at the history, you'll see that theses macro didn't exist
when membar_* where added to rw locks.

> And there is even a default variant for archs which don't provide one.
> I guess the switch should be easy.

Then please do it :)  At lot of stuff is easy on OpenBSD but we are not
enough.

Do it, test it, explain it, get oks and commit it 8)

Re: pow() returns a negative result on loongson

2017-10-10 Thread Miod Vallat 
> My first thought would on Loongson would be softfloat bug.
> But I am not 100% sure it (still) uses softfloat.

Loongson has never used softfloat. But all mips ports embed the
softfloat code in order to perform tricky computations the FPU won't
perform (mostly edge cases involving denormals and infinities).

So in this case, either the computation gets entirely done in hardware,
but the hardware yields a wrong results, and there won't likely be a fix
for this, or it asks the kernel for assistance, and there might be a
sign bug in there.

Re: ctags(1): missing space between tag and line number

2017-10-10 Thread Anton Lindqvist 
Ping

On Thu, Oct 05, 2017 at 10:41:09AM +0200, Anton Lindqvist wrote:
> Hi,
> Running `ctags -x` on a file including a tag which satisfies strlen(tag)
> >= 16 and line number >= 1000 corrupts the output since there's no space
> between the tag and line number. Therefore, add a space between them
> just like ectags and uctags in ports does.
> 
>   $ ctags -x /sys/dev/usb/umass.c | grep dump # before
>   umass_bbb_dump_cbw1830 /sys/dev/usb/umass.c umass_bbb_dump_cbw(struct 
> umass_softc *sc, struct umass_bbb_cbw *cbw)
>   umass_bbb_dump_csw1850 /sys/dev/usb/umass.c umass_bbb_dump_csw(struct 
> umass_softc *sc, struct umass_bbb_csw *csw)
>   umass_dump_buffer1867 /sys/dev/usb/umass.c umass_dump_buffer(struct 
> umass_softc *sc, u_int8_t *buffer, int buflen,
>   $ ./obj/ctags -x /sys/dev/usb/umass.c | grep dump # after
>   umass_bbb_dump_cbw 1830 /sys/dev/usb/umass.c umass_bbb_dump_cbw(struct 
> umass_softc *sc, struct umass_bbb_cbw *cbw)
>   umass_bbb_dump_csw 1850 /sys/dev/usb/umass.c umass_bbb_dump_csw(struct 
> umass_softc *sc, struct umass_bbb_csw *csw)
>   umass_dump_buffer 1867 /sys/dev/usb/umass.c umass_dump_buffer(struct 
> umass_softc *sc, u_int8_t *buffer, int buflen,
> 
> Comments? OK?
> 
> Index: print.c
> ===
> RCS file: /cvs/src/usr.bin/ctags/print.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 print.c
> --- print.c   4 Mar 2012 04:05:15 -   1.7
> +++ print.c   5 Oct 2017 08:39:54 -
> @@ -99,7 +99,7 @@ put_entries(NODE *node)
>   printf("%s %s %d\n",
>   node->entry, node->file, (node->lno + 63) / 64);
>   else if (xflag)
> - printf("%-16s%4d %-16s %s\n",
> + printf("%-16s %4d %-16s %s\n",
>   node->entry, node->lno, node->file, node->pat);
>   else
>   fprintf(outf, "%s\t%s\t%c^%s%c\n",

Error compiling kernel with SR_DEBUG

2017-10-10 Thread Zé Loff 

Dear all

While compiling amd64 -current with "option SR_DEBUG" the error below
comes up.  Steps leading to this were exactly those presented on
config(8)'s first example.  The only change from GENERIC is adding
SR_DEBUG.

Apologies in advance for any PEBKAC, it's the first time I'm
compiling bsd myself.  Also, lines below aren't wrapped as per the
mailing list rules, but I didn't think it'd make sense to do so.  And I
sure as hell don't know enough to come up with a diff, sorry for that
too.

Cheers
Zé


cc -g -Werror -Wall -Wimplicit-function-declaration  -Wno-uninitialized 
-Wno-pointer-sign  -Wno-address-of-packed-member -Wno-constant-conversion  
-Wframe-larger-than=2047 -mcmodel=kernel -mno-red-zone -mno-sse2 -mno-sse 
-mno-3dnow  -mno-mmx -msoft-float -fno-omit-frame-pointer -ffreestanding 
-fno-pie -O2 -pipe -nostdinc -I/usr/src/sys -I/home/zeloff/build/bsd 
-I/usr/src/sys/arch -DDDB -DDIAGNOSTIC -DKTRACE -DACCOUNTING -DKMEMSTATS 
-DPTRACE -DPOOL_DEBUG -DCRYPTO -DSYSVMSG -DSYSVSEM -DSYSVSHM -DUVM_SWAP_ENCRYPT 
-DFFS -DFFS2 -DFFS_SOFTUPDATES -DUFS_DIRHASH -DQUOTA -DEXT2FS -DMFS -DNFSCLIENT 
-DNFSSERVER -DCD9660 -DUDF -DMSDOSFS -DFIFO -DFUSE -DSOCKET_SPLICE -DTCP_SACK 
-DTCP_ECN -DTCP_SIGNATURE -DINET6 -DIPSEC -DPPP_BSDCOMP -DPPP_DEFLATE -DPIPEX 
-DMROUTING -DMPLS -DBOOT_CONFIG -DUSER_PCICONF -DAPERTURE -DMTRR -DNTFS 
-DHIBERNATE -DSR_DEBUG -DPCIVERBOSE -DUSBVERBOSE -DWSDISPLAY_COMPAT_USL 
-DWSDISPLAY_COMPAT_RAWKBD -DWSDISPLAY_DEFAULTSCREENS="6" -DX86EMU 
-DONEWIREVERBOSE -DMAXUSERS=80 -D_KERNEL -MD -MP  -c /usr/src/sys/dev/softraid.c
/usr/src/sys/dev/softraid.c:1537:23: error: format specifies type 'unsigned 
short' but the argument has type 'u_char' (aka 'unsigned char') 
[-Werror,-Wformat]
DEVNAME(sc), rootduid[0],
 ^~~
/usr/src/sys/dev/softraidvar.h:317:56: note: expanded from macro 'DNPRINTF'
#define DNPRINTF(n,x...)do { if (sr_debug & n) printf(x); } while(0)
  ^
/usr/src/sys/dev/softraid.c:1538:10: error: format specifies type 'unsigned 
short' but the argument has type 'u_char' (aka 'unsigned char') 
[-Werror,-Wformat]
rootduid[1], rootduid[2],
^~~
/usr/src/sys/dev/softraidvar.h:317:56: note: expanded from macro 'DNPRINTF'
#define DNPRINTF(n,x...)do { if (sr_debug & n) printf(x); } while(0)
  ^
/usr/src/sys/dev/softraid.c:1538:23: error: format specifies type 'unsigned 
short' but the argument has type 'u_char' (aka 'unsigned char') 
[-Werror,-Wformat]
rootduid[1], rootduid[2],
 ^~~
/usr/src/sys/dev/softraidvar.h:317:56: note: expanded from macro 'DNPRINTF'
#define DNPRINTF(n,x...)do { if (sr_debug & n) printf(x); } while(0)
  ^
/usr/src/sys/dev/softraid.c:1539:10: error: format specifies type 'unsigned 
short' but the argument has type 'u_char' (aka 'unsigned char') 
[-Werror,-Wformat]
rootduid[3], rootduid[4],
^~~
/usr/src/sys/dev/softraidvar.h:317:56: note: expanded from macro 'DNPRINTF'
#define DNPRINTF(n,x...)do { if (sr_debug & n) printf(x); } while(0)
  ^
/usr/src/sys/dev/softraid.c:1539:23: error: format specifies type 'unsigned 
short' but the argument has type 'u_char' (aka 'unsigned char') 
[-Werror,-Wformat]
rootduid[3], rootduid[4],
 ^~~
/usr/src/sys/dev/softraidvar.h:317:56: note: expanded from macro 'DNPRINTF'
#define DNPRINTF(n,x...)do { if (sr_debug & n) printf(x); } while(0)
  ^
/usr/src/sys/dev/softraid.c:1540:10: error: format specifies type 'unsigned 
short' but the argument has type 'u_char' (aka 'unsigned char') 
[-Werror,-Wformat]
rootduid[5], rootduid[6],
^~~
/usr/src/sys/dev/softraidvar.h:317:56: note: expanded from macro 'DNPRINTF'
#define DNPRINTF(n,x...)do { if (sr_debug & n) printf(x); } while(0)
  ^
/usr/src/sys/dev/softraid.c:1540:23: error: format specifies type 'unsigned 
short' but the argument has type 'u_char' (aka 'unsigned char') 
[-Werror,-Wformat]
rootduid[5], rootduid[6],
 ^~~
/usr/src/sys/dev/softraidvar.h:317:56: note: expanded from macro

Re: armv7/panda usb eth addr wondering

2017-10-10 Thread Mark Kettenis 
> Date: Tue, 10 Oct 2017 06:32:58 +0300
> From: Artturi Alm 
> 
> Hi,
> 
> i've been bothered by this:
> smsc0 at uhub1 port 1 configuration 1 interface 0 "Standard Microsystems 
> SMSC9512/14" rev 2.00/2.00 addr 3
> smsc0: address ff:ff:ff:ff:ff:ff
> 
> that does happen no matter if i run dhcp-command in u-boot shell or not.
> u-boot does store the addr in env, but i don't want to learn how to
> access it from efi, i think it wouldn't be pretty.
> 
> iirc. the DT is mapped kernel writable, so would anything like below be
> accepted for generalizing the kind of quirk there already is in if_smsc.c
> for rPI (the smsc_enaddr_OF())?
> 
> diff --git a/sys/arch/armv7/stand/efiboot/efiboot.c 
> b/sys/arch/armv7/stand/efiboot/efiboot.c
> index dbc434da7bf..b8f17fc5921 100644
> --- a/sys/arch/armv7/stand/efiboot/efiboot.c
> +++ b/sys/arch/armv7/stand/efiboot/efiboot.c
> @@ -328,6 +328,12 @@ efi_makebootargs(char *bootargs, uint32_t *board_id)
>   fdt_node_add_property(node, "openbsd,uefi-mmap-desc-size", zero, 4);
>   fdt_node_add_property(node, "openbsd,uefi-mmap-desc-ver", zero, 4);
>  
> + /*
> +  * For boards with integrated USB ethernet, to be filled later
> +  * during autoconf, from efuses/eeprom or such.
> +  */
> + fdt_node_add_property(node, "openbsd,uea", zero, 6);
> +
>   fdt_finalize();
>  
>   node = fdt_find_node("/");
> 
> 
> or, if hack like above is totally unaccepted for this, some clue how to
> solve my issue w/o giving up running GENERIC on panda would be appreciated.

See dev/usb/if_smsc.c:smsc_enaddr_OF().

Re: efiboot: Restore GOP mode on SetMode() failure

2017-10-10 Thread Klemens Nanni 
On Tue, Oct 10, 2017 at 02:56:26AM +, YASUOKA Masahiko wrote:
> On Sat, 7 Oct 2017 09:24:20 +0200
> Klemens Nanni  wrote:
> > On Sat, Oct 07, 2017 at 01:15:40AM +, YASUOKA Masahiko wrote:
> >> > See my updated diff for reusing the gopi struct, please.
> >> 
> >> ok, but the diff seems to be against wrong revision.  You seems to
> >> have other diffs, moving gop and gopi to global at least.
> >> 
> >> Can you send it entirely?
> > My bad, those were applied on top of other (unsubmitted) diffs such as
> > https://marc.info/?l=openbsd-tech=150437080106164
> 
> Ah, sorry I missed that mail.
> 
> On Fri, 6 Oct 2017 20:11:24 +0200
> Klemens Nanni  wrote:
> > Declaring the gop and gopi strucutures globally makes things easier in
> > preparation for the next commit.
> > 
> > Both changes also improve consistency with regard to other structures
> > like ei, di and conout as well.
> 
> As for gopi, I'd like to keep it be a local since it is used to refer
> various different modes.
That's fine.

> also s/efi_gopmode/gopmode/
I adapted efi_gopmode from your previous patch.

> comments?
> 
> Index: sys/arch/amd64/stand/efiboot/efiboot.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 efiboot.c
> --- sys/arch/amd64/stand/efiboot/efiboot.c6 Oct 2017 04:52:22 -   
> 1.24
> +++ sys/arch/amd64/stand/efiboot/efiboot.c10 Oct 2017 02:52:46 -
> @@ -60,7 +60,7 @@ static void  efi_memprobe_internal(void)
>  static void   efi_video_init(void);
>  static void   efi_video_reset(void);
>  static EFI_STATUS
> -  efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode);
> +  efi_gop_setmode(int mode);
>  EFI_STATUSefi_main(EFI_HANDLE, EFI_SYSTEM_TABLE *);
>  
>  void (*run_i386)(u_long, u_long, int, int, int, int, int, int, int, int)
> @@ -696,13 +696,15 @@ efi_com_putc(dev_t dev, int c)
>   * {EFI_,}_ACPI_20_TABLE_GUID or EFI_ACPI_TABLE_GUID means
>   * ACPI 2.0 or above.
>   */
> -static EFI_GUID acpi_guid = ACPI_20_TABLE_GUID;
> -static EFI_GUID smbios_guid = SMBIOS_TABLE_GUID;
> +static EFI_GUID   acpi_guid = ACPI_20_TABLE_GUID;
> +static EFI_GUID   smbios_guid = SMBIOS_TABLE_GUID;
> +static EFI_GRAPHICS_OUTPUT   *gop;
> +static intgopmode = -1;
>  
>  #define  efi_guidcmp(_a, _b) memcmp((_a), (_b), sizeof(EFI_GUID))
>  
>  static EFI_STATUS
> -efi_gop_setmode(EFI_GRAPHICS_OUTPUT *gop, int mode)
> +efi_gop_setmode(int mode)
>  {
>   EFI_STATUS  status;
>  
> @@ -718,14 +720,11 @@ efi_makebootargs(void)
>  {
>   int  i;
>   EFI_STATUS   status;
> - EFI_GRAPHICS_OUTPUT *gop;
>   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
>   *gopi;
>   bios_efiinfo_t   ei;
> - int  curmode, bestmode = -1;
> + int  curmode;
>   UINTNsz, gopsiz, bestsiz = 0;
> - EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
> - *info;
>  
>   memset(, 0, sizeof(ei));
>   /*
> @@ -748,21 +747,24 @@ efi_makebootargs(void)
>   status = EFI_CALL(BS->LocateProtocol, _guid, NULL,
>   (void **));
>   if (!EFI_ERROR(status)) {
> - for (i = 0; i < gop->Mode->MaxMode; i++) {
> - status = EFI_CALL(gop->QueryMode, gop, i, , );
> - if (EFI_ERROR(status))
> - continue;
> - gopsiz = info->HorizontalResolution *
> - info->VerticalResolution;
> - if (gopsiz > bestsiz) {
> - bestmode = i;
> - bestsiz = gopsiz;
> + if (gopmode < 0) {
> + for (i = 0; i < gop->Mode->MaxMode; i++) {
> + status = EFI_CALL(gop->QueryMode, gop,
> + i, , );
> + if (EFI_ERROR(status))
> + continue;
> + gopsiz = gopi->HorizontalResolution *
> + gopi->VerticalResolution;
> + if (gopsiz > bestsiz) {
> + gopmode = i;
> + bestsiz = gopsiz;
> + }
>   }
>   }
> - if (bestmode >= 0) {
> + if (gopmode >= 0 && gopmode != gop->Mode->Mode) {
>   curmode = gop->Mode->Mode;
> - if (efi_gop_setmode(gop, bestmode) != EFI_SUCCESS)
> - (void)efi_gop_setmode(gop, curmode);
> + if (efi_gop_setmode(gopmode) != EFI_SUCCESS)
> +