Re: [patch] correct return type in pcap_open_live.3

2020-05-28 Thread Jason McIntyre
On Tue, May 26, 2020 at 05:10:33PM -0500, Edgar Pettijohn wrote:
> Please see attached diff.

fixed, thanks.
jmc

> Index: pcap_open_live.3
> ===
> RCS file: /cvs/src/lib/libpcap/pcap_open_live.3,v
> retrieving revision 1.3
> diff -u -p -u -r1.3 pcap_open_live.3
> --- pcap_open_live.3  25 Sep 2019 17:02:00 -  1.3
> +++ pcap_open_live.3  26 May 2020 22:09:24 -
> @@ -95,7 +95,7 @@
>  .Fn pcap_dump_fopen "pcap_t *p" "FILE *f"
>  .Ft "char *"
>  .Fn pcap_lookupdev "char *errbuf"
> -.Ft uint
> +.Ft int
>  .Fn pcap_lookupnet "const char *device" "bpf_u_int32 *netp" "bpf_u_int32 
> *maskp" "char *errbuf"
>  .Ft int
>  .Fn pcap_dispatch "pcap_t *p" "int cnt" "pcap_handler callback" "u_char 
> *user"



symmetric toeplitz hashing

2020-05-28 Thread David Gwynne
This is another bit of the puzzle for supporting multiple rx rings
and receive side scaling (RSS) on nics. It borrows heavily from
DragonflyBSD, but I've made some tweaks on the way.

For background on the dfly side, I recommend having a look at
https://leaf.dragonflybsd.org/~sephe/AsiaBSDCon%20-%20Dfly.pdf.

>From my point of view, the interesting thing is that they came up
with a way to use Toeplitz hashing so the kernel AND network
interfaces hash packets so packets in both directions onto the same
bucket. The other interesting thing is that the optimised the hash
calculation by building a cache of all the intermediate results
possible for each input byte. Their hash calculation is simply
xoring these intermediate reults together.

I've made some tweaks compared to dfly for how the caching is
calculated and used, so it's not an exactly 1:1 port of the dfly
code. If anyone is interested in the tweaks, let me know.

So this diff adds an API for the kernel to use for calculating a
hash for ip addresses and ports, and adds a function for network
drivers to call that gives them a key to use with RSS. If all drivers
use the same key, then the same flows should be steered to the same
place when they enter the network stack regardless of which hardware
they came in on.

I've tested it with vmx(4) and some quick and dirty hacks to the
network stack (and with some magical observability), and can see
things like tcpbench push packets onto the same numbered ifq/txring
that the "nic" picks for the rxring and therefore ifiq into the
stack. We're going to try it on some more drivers soon.

The way this is set up now, if a nic driver wants to do RSS, you
add stoeplitz as a dependency in the kernel config file, which
causes this code to be included in the build.

There's some discussion to be had about the best way to integrate
this on the IP stack side, but that is about where this API is
called from, not the internals of it per se.

Thoughts? ok?

Index: share/man/man9/stoeplitz_to_key.9
===
RCS file: share/man/man9/stoeplitz_to_key.9
diff -N share/man/man9/stoeplitz_to_key.9
--- /dev/null   1 Jan 1970 00:00:00 -
+++ share/man/man9/stoeplitz_to_key.9   29 May 2020 04:01:26 -
@@ -0,0 +1,126 @@
+.\" $OpenBSD$
+.\"
+.\" Copyright (c) 2020 David Gwynne 
+.\"
+.\" Permission to use, copy, modify, and distribute this software for any
+.\" purpose with or without fee is hereby granted, provided that the above
+.\" copyright notice and this permission notice appear in all copies.
+.\"
+.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+.\"
+.Dd $Mdocdate: May 29 2020 $
+.Dt STOEPLITZ_TO_KEY 9
+.Os
+.Sh NAME
+.Nm stoeplitz_to_key ,
+.Nm stoeplitz_hash_ip4 ,
+.Nm stoeplitz_hash_ip4port ,
+.Nm stoeplitz_hash_ip6 ,
+.Nm stoeplitz_hash_ip4port
+.Nd Symmetric Toeplitz Hash API
+.Sh SYNOPSIS
+.In net/toeplitz.h
+.Ft void
+.Fn stoeplitz_to_key "uint8_t *key" "size_t keylen"
+.Ft uint16_t
+.Fo stoeplitz_hash_ip4
+.Fa "uint32_t srcaddr"
+.Fa "uint32_t dstaddr"
+.Fc
+.Ft uint16_t
+.Fo stoeplitz_hash_ip4port
+.Fa "uint32_t srcaddr"
+.Fa "uint32_t dstaddr"
+.Fa "uint16_t srcport"
+.Fa "uint16_t dstport"
+.Fc
+.Ft uint16_t
+.Fo stoeplitz_hash_ip6
+.Fa "const struct in6_addr *srcaddr"
+.Fa "const struct in6_addr *dstaddr"
+.Fc
+.Ft uint16_t
+.Fo stoeplitz_hash_ip6port
+.Fa "const struct in6_addr *srcaddr"
+.Fa "const struct in6_addr *dstaddr"
+.Fa "uint16_t srcport"
+.Fa "uint16_t dstport"
+.Fc
+.Sh DESCRIPTION
+The Toeplitz hash algorithm is commonly used by network interface
+controllers to to generate a short hash based on the value of fields
+in network packet headers.
+.\" mention RSS?
+The resulting hash value can be used as a flow identifier, which
+in turn can be used to consistently select a context for processing
+packets using those fields.
+Traditionally, the Toeplitz hash produces different results depending
+on the order of inputs, ie, adding port 80 then 1234 as inputs would
+produce a different result to hashing port 1234 then 80.
+.Pp
+The symmetric Toeplitz API uses a key selected to generate the same
+hash result regardless of the order the inputs were added.
+The API also supports producing Toeplitz hash keys for use by
+network interface controllers that provide the same symmetric
+property.
+.Pp
+The
+.Fn stoeplitz_to_key
+function generates a Toeplitz key for use by a network interface
+controller based on the systems symmetric Toeplitz key.
+A Toeplitz key of
+.Fa keylen
+bytes will 

Re: [PATCH] from.c: stricter pledge(2)

2020-05-28 Thread Ricardo Mestre
this is ok mestre@ may I commit it?

On 21:18 Thu 28 May , Martin Vahlensieck wrote:
> Hey!
> 
> This pledge was added with the use of unveil(2), but doesn't require the
> getpw promise anymore (it is only needed in mail_spool to get the
> username).
> 
> This patch makes it stricter.
> 
> Best,
> 
> Martin
> 
> Index: from.c
> ===
> RCS file: /cvs/src/usr.bin/from/from.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 from.c
> --- from.c8 Aug 2018 17:52:46 -   1.26
> +++ from.c24 May 2020 12:01:06 -
> @@ -81,7 +81,7 @@ main(int argc, char *argv[])
>  
>   if (unveil(file, "r") == -1)
>   err(1, "unveil");
> - if (pledge("stdio rpath getpw", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>  
>   if ((fp = fopen(file, "r")) == NULL) {
> 



Re: Enable building wsmoused on arm64 and armv7

2020-05-28 Thread Frederic Cambus
On Thu, May 28, 2020 at 10:52:44AM -0600, Theo de Raadt wrote:
> -MANSUBDIR= i386 amd64 alpha
> +MANSUBDIR= i386 amd64 arm64 armv7 alpha
> 
> Actually, I suggest making this a MI man page.  Delete that line, and see
> where the files land.  I'll adjust sets.

Yes, makes sense. Just commited the change, MANSUBDIR is now gone.

The man page ends up in /usr/share/man/man8 as expected.



Re: Avoid offline cpu in automatic frequency scheduling

2020-05-28 Thread Ted Unangst
On 2020-05-28, Solene Rapenne wrote:
> > > In the current case, if you have offline cpu (because of hw.smt=0),
> > > the total will still sum all the idle cpu and it's unlikely that
> > > the total threshold is ever reached before the per cpu one.
> > > 
> > > so, this diff skip offline cpus in the loop (robert@ gave me the big
> > > clue to use cpu_is_online in the loop)  

This looks good.

> 
> It's a lot of magic numbers without explanation here. I'm curious if
> this could be improved. I guess that changing them would tip the
> balance between responsiveness and battery saving.

Did I do that? You have it about right, although there was never much science
behind the algorithm or numbers. It was something that seemed to work okay
without much complexity.



Re: userland clock_gettime proof of concept

2020-05-28 Thread Paul Irofti
On Thu, May 28, 2020 at 10:27:03PM +0300, Paul Irofti wrote:
> > 5. What if the TSC is not available as a usable timecounter?  In that
> >case libc should fall back on the system call.  But we need a way
> >to communicate what the timecounter is and detect when we switch
> >timecounters.  Maybe adding a timecounter ID to the page will help
> >here.  But then MD code in libc will have to check the ID and
> >dispatch to the right timecounter read function.
> 
> I fixed 1--4 and 6, but with 5 the solutions I found are a bit
> convoluted and involve string passing and parsing if we are to pass this
> information to libc.
> 
> Would it be acceptable to add a memember to struct timecounter that
> states whether the clock is libc ready or not? This means that when you
> add support for a new clock in libc you also have to touch the kernel to
> set that bit...
> 
> On the other hand the code would be clean and safe:

if (timekeep == NULL || !timekeep->tc_supported)
clock_gettime();

that's what I meant, of course...

The tc_supported bit would be set in the kernel when the timecounter is
changed. I have those bits inside tc_update_timekeep() already for the
tc_counter_mask.



Re: userland clock_gettime proof of concept

2020-05-28 Thread Paul Irofti
> 5. What if the TSC is not available as a usable timecounter?  In that
>case libc should fall back on the system call.  But we need a way
>to communicate what the timecounter is and detect when we switch
>timecounters.  Maybe adding a timecounter ID to the page will help
>here.  But then MD code in libc will have to check the ID and
>dispatch to the right timecounter read function.

I fixed 1--4 and 6, but with 5 the solutions I found are a bit
convoluted and involve string passing and parsing if we are to pass this
information to libc.

Would it be acceptable to add a memember to struct timecounter that
states whether the clock is libc ready or not? This means that when you
add support for a new clock in libc you also have to touch the kernel to
set that bit...

On the other hand the code would be clean and safe:

if (timekeep == NULL || timekeep->tc_supported)
clock_gettime();

/* rest of wrapper function */

What do you think?



[PATCH] from.c: stricter pledge(2)

2020-05-28 Thread Martin Vahlensieck
Hey!

This pledge was added with the use of unveil(2), but doesn't require the
getpw promise anymore (it is only needed in mail_spool to get the
username).

This patch makes it stricter.

Best,

Martin

Index: from.c
===
RCS file: /cvs/src/usr.bin/from/from.c,v
retrieving revision 1.26
diff -u -p -r1.26 from.c
--- from.c  8 Aug 2018 17:52:46 -   1.26
+++ from.c  24 May 2020 12:01:06 -
@@ -81,7 +81,7 @@ main(int argc, char *argv[])
 
if (unveil(file, "r") == -1)
err(1, "unveil");
-   if (pledge("stdio rpath getpw", NULL) == -1)
+   if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
 
if ((fp = fopen(file, "r")) == NULL) {



Re: filesystem code integer and many inodes

2020-05-28 Thread Todd C . Miller
On Thu, 28 May 2020 20:53:07 +0200, Otto Moerbeek wrote:

> Here's the separate diff for the prefcg loops. From FreeBSD.

OK millert@

 - todd



Re: filesystem code integer and many inodes

2020-05-28 Thread Otto Moerbeek
On Tue, May 26, 2020 at 04:11:50PM +0200, Otto Moerbeek wrote:

> On Tue, May 26, 2020 at 03:54:15PM +0200, Otto Moerbeek wrote:
> 
> > On Tue, May 26, 2020 at 07:51:28AM -0600, Todd C. Miller wrote:
> > 
> > > On Tue, 26 May 2020 12:07:21 +0200, Otto Moerbeek wrote:
> > > 
> > > > Apart from the noting the strange Subject: I also like to mention one
> > > > change in the way cylinder groups are scanned. The current code scans
> > > > forward and backward, which causes an uneven distribution of full cgs
> > > > (the upper end of the cgs will get full first). Fix that by always
> > > > scanning forward, wrapping to cg 0 if needed.
> > > 
> > > Should that be a separate commit?  I can't find any problems
> > > with the diff but I haven't tried running with it yet.
> > > 
> > >  - todd
> > 
> > Yeah, I can do that. Note that it must be comitted first, since the
> > loop condition is always true if I change the loop var to unsigned.
> > 
> > -Otto
> > 
> 
> And a new diff. I accidentally capitalized a letter just before sending.
> Thanks to naddy for spotting that.

Here's the separate diff for the prefcg loops. From FreeBSD.

OK?

-Otto


Index: ffs_alloc.c
===
RCS file: /cvs/src/sys/ufs/ffs/ffs_alloc.c,v
retrieving revision 1.111
diff -u -p -r1.111 ffs_alloc.c
--- ffs_alloc.c 28 May 2020 15:48:29 -  1.111
+++ ffs_alloc.c 28 May 2020 18:47:53 -
@@ -515,7 +518,7 @@ ffs_dirpref(struct inode *pip)
maxcontigdirs = 1;
 
/*
-* Limit number of dirs in one cg and reserve space for 
+* Limit number of dirs in one cg and reserve space for
 * regular files, but only if we have no deficit in
 * inodes or space.
 *
@@ -524,16 +527,17 @@ ffs_dirpref(struct inode *pip)
 * We scan from our preferred cylinder group forward looking
 * for a cylinder group that meets our criterion. If we get
 * to the final cylinder group and do not find anything,
-* we start scanning backwards from our preferred cylinder
-* group. The ideal would be to alternate looking forward
-* and backward, but tha tis just too complex to code for
-* the gain it would get. The most likely place where the
-* backward scan would take effect is when we start near
-* the end of the filesystem and do not find anything from
-* where we are to the end. In that case, scanning backward
-* will likely find us a suitable cylinder group much closer
-* to our desired location than if we were to start scanning
-* forward from the beginning for the filesystem.
+* we start scanning forwards from the beginning of the
+* filesystem. While it might seem sensible to start scanning
+* backwards or even to alternate looking forward and backward,
+* this approach fails badly when the filesystem is nearly full.
+* Specifically, we first search all the areas that have no space
+* and finally try the one preceding that. We repeat this on
+* every request and in the case of the final block end up
+* searching the entire filesystem. By jumping to the front
+* of the filesystem, our future forward searches always look
+* in new cylinder groups so finds every possible block after
+* one pass over the filesystem.
 */
for (cg = prefcg; cg < fs->fs_ncg; cg++)
if (fs->fs_cs(fs, cg).cs_ndir < maxndir &&
@@ -542,7 +546,7 @@ ffs_dirpref(struct inode *pip)
if (fs->fs_contigdirs[cg] < maxcontigdirs)
goto end;
}
-   for (cg = prefcg - 1; cg >= 0; cg--)
+   for (cg = 0; cg < prefcg; cg++)
if (fs->fs_cs(fs, cg).cs_ndir < maxndir &&
fs->fs_cs(fs, cg).cs_nifree >= minifree &&
fs->fs_cs(fs, cg).cs_nbfree >= minbfree) {
@@ -555,7 +559,7 @@ ffs_dirpref(struct inode *pip)
for (cg = prefcg; cg < fs->fs_ncg; cg++)
if (fs->fs_cs(fs, cg).cs_nifree >= avgifree)
goto end;
-   for (cg = prefcg - 1; cg >= 0; cg--)
+   for (cg = 0; cg < prefcg; cg++)
if (fs->fs_cs(fs, cg).cs_nifree >= avgifree)
goto end;
 end:



Re: [RFC] pppd: add pipex(4) L2TP control support

2020-05-28 Thread Jason McIntyre
On Wed, May 27, 2020 at 08:43:47AM +0200, Martin Pieuchot wrote:
> On 26/05/20(Tue) 10:31, Claudio Jeker wrote:
> > [...] 
> > npppd(8) is server only it can not establish a connection. pppd(8) on the
> > other hand is more client side (but I think it can do both).
> 
> Could someone knowledgable indicate that in the man pages?

well, i don;t qualify as knowledgable about ppp, but i'll bite anyway:

> Currently there is:
> 
>   npppd ??? new Point-to-Point Protocol daemon
>   pppd ??? Point-to-Point Protocol daemon
> 
> Confusing...
> 

yes. at the time, i think npppd's description made sense - it was
a newer version of the ppp server. i think it was pppd that had
the confusing description, but there was a reason for that too.
wasn;t there an alternative way to do dialup, and pppd was the
kernel method?

i guess i'm going to be burned for that vague text. anyway:

> The DESCRIPTIONs aren't much more helpful :)
> 

to be honest, i think they're clear enough. i do dislike the
descriptions: "new" always goes out of date. i personally don;t
have any suggestions though. if we make them the same, they could cause
more confusion (or just as much). we could make pppd client/daemon, and
remove "new" i suppose:

npppd - Point-to-Point Protocol daemon
pppd - Point-to-Point Protocol client/daemon

jmc



Re: userland clock_gettime proof of concept

2020-05-28 Thread Mark Kettenis
> Date: Thu, 28 May 2020 17:44:31 +0300
> From: Paul Irofti 
> 
> Hi,
> 
> Here is a new iteration of the diff which includes support for MD high
> resolution clocks. Currently only implements TSC on amd64. If the
> MD function is not defined, it fallsback to the syscall.
> 
> There is the question of the skew fix, but that will be addressed in a
> separate kernel diff that will not affect the current diff at all.
> 
> I could not find a way to find on which processor the process is running
> on from userland without going through a syscall. If there is one please
> let me know. It would make things easier.
> 
> In the meantime I have also gotten positive feedback from various
> testers that run this on their main machine.
> 
> Anyway, I think we can decide on the struct name and the auxiliary
> vector ID and consider this done.
> 
> Thoughts?

This is getting us somewhere.

Still some issues though (besides the skew thing you already mention).

1. The synchronization mechanism is broken.  The seq member needs to
   be set to 0 while updating the struct and only set to the "next"
   value after completing the update of the full struct.  You need to
   be careful to avoid 0, otherwise the application will spin for a
   full timeslice while seq overflows into 0.

   However, since you now export the timehands generation, I'd really
   drop seq and use the timehands generation for synchronization.  It
   makes no sense to have both.

2. Since tc_update_timekeep() is called from tc_windup() it doesn't
   need to do the synchronization dance.

3. Like tc_windup, tc_update_timekeep() needs to have some
membar_procer() calls in it instead of membar_consumer() calls.

4. There is no need to update th_counter_mask on every update.

5. What if the TSC is not available as a usable timecounter?  In that
   case libc should fall back on the system call.  But we need a way
   to communicate what the timecounter is and detect when we switch
   timecounters.  Maybe adding a timecounter ID to the page will help
   here.  But then MD code in libc will have to check the ID and
   dispatch to the right timecounter read function.

6. The major and minor fields probably should bbe uint32_t or maybe
uint16_t.  You're not saving any space by making them uint8_t.

> 
> Paul 
> 
> diff --git lib/libc/arch/amd64/gen/Makefile.inc 
> lib/libc/arch/amd64/gen/Makefile.inc
> index e995309ed71..caa4452a3d9 100644
> --- lib/libc/arch/amd64/gen/Makefile.inc
> +++ lib/libc/arch/amd64/gen/Makefile.inc
> @@ -2,6 +2,6 @@
>  
>  SRCS+=   _setjmp.S fabs.S infinity.c ldexp.c modf.S nan.c setjmp.S \
>   sigsetjmp.S
> -SRCS+=   fpclassifyl.c isfinitel.c isinfl.c isnanl.c isnormall.c 
> signbitl.c
> +SRCS+=   fpclassifyl.c rdtsc.c isfinitel.c isinfl.c isnanl.c isnormall.c 
> signbitl.c
>  SRCS+=   flt_rounds.S fpgetmask.S fpgetround.S fpgetsticky.S fpsetmask.S 
> \
>   fpsetround.S fpsetsticky.S
> diff --git lib/libc/arch/amd64/gen/rdtsc.c lib/libc/arch/amd64/gen/rdtsc.c
> new file mode 100644
> index 000..b14c862c61a
> --- /dev/null
> +++ lib/libc/arch/amd64/gen/rdtsc.c
> @@ -0,0 +1,26 @@
> +/*   $OpenBSD$ */
> +/*
> + * Copyright (c) 2020 Paul Irofti 
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include 
> +
> +uint64_t
> +tc_get_timecount_md(void)
> +{
> + uint32_t hi, lo;
> + asm volatile("rdtsc" : "=a"(lo), "=d"(hi));
> + return ((uint64_t)lo)|(((uint64_t)hi)<<32);
> +}
> diff --git lib/libc/asr/asr.c lib/libc/asr/asr.c
> index cd056c85719..2b25d49f32a 100644
> --- lib/libc/asr/asr.c
> +++ lib/libc/asr/asr.c
> @@ -196,11 +196,11 @@ poll_intrsafe(struct pollfd *fds, nfds_t nfds, int 
> timeout)
>   struct timespec pollstart, pollend, elapsed;
>   int r;
>  
> - if (clock_gettime(CLOCK_MONOTONIC, ))
> + if (WRAP(clock_gettime)(CLOCK_MONOTONIC, ))
>   return -1;
>  
>   while ((r = poll(fds, 1, timeout)) == -1 && errno == EINTR) {
> - if (clock_gettime(CLOCK_MONOTONIC, ))
> + if (WRAP(clock_gettime)(CLOCK_MONOTONIC, ))
>   return -1;
>   timespecsub(, , );
>   timeout -= elapsed.tv_sec * 1000 + elapsed.tv_nsec / 100;
> @@ -418,7 +418,7 @@ asr_check_reload(struct asr *asr)
> 

Re: Enable building wsmoused on arm64 and armv7

2020-05-28 Thread Mark Kettenis
> Date: Thu, 28 May 2020 18:45:52 +0200
> From: Frederic Cambus 
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> Hi tech@,
> 
> Here is a diff to enable building wsmoused on arm64 and armv7.
> 
> Builds and works correctly on my CubieBoard2.
> 
> Comments? OK?

ok kettenis@

> Index: usr.sbin/wsmoused/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/wsmoused/Makefile,v
> retrieving revision 1.7
> diff -u -p -r1.7 Makefile
> --- usr.sbin/wsmoused/Makefile16 Jul 2014 20:07:03 -  1.7
> +++ usr.sbin/wsmoused/Makefile28 May 2020 16:35:14 -
> @@ -1,6 +1,7 @@
>  #$OpenBSD: Makefile,v 1.7 2014/07/16 20:07:03 okan Exp $ 
>  
> -.if ${MACHINE} == "i386" || ${MACHINE} == "amd64" ||\
> +.if ${MACHINE} == "i386" || ${MACHINE} == "amd64" || \
> +${MACHINE} == "arm64" || ${MACHINE} == "armv7" || \
>  ${MACHINE} == "alpha"
>  
>  PROG=wsmoused
> @@ -13,6 +14,6 @@ NOPROG=yes
>  .endif
>  
>  MAN= wsmoused.8 
> -MANSUBDIR=   i386 amd64 alpha
> +MANSUBDIR=   i386 amd64 arm64 armv7 alpha
>  
>  .include 
> 
> 



Re: Enable building wsfontload on arm64 and armv7

2020-05-28 Thread Mark Kettenis
> Date: Thu, 28 May 2020 18:44:43 +0200
> From: Frederic Cambus 
> 
> Hi tech@,
> 
> Here is a diff to enable building wsfontload on arm64 and armv7.
> 
> Builds and works correctly on my CubieBoard2.
> 
> Comments? OK?

ok kettenis@

> Index: usr.sbin/wsfontload/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/wsfontload/Makefile,v
> retrieving revision 1.16
> diff -u -p -r1.16 Makefile
> --- usr.sbin/wsfontload/Makefile  19 Jan 2017 08:06:27 -  1.16
> +++ usr.sbin/wsfontload/Makefile  28 May 2020 16:34:58 -
> @@ -2,6 +2,7 @@
>  
>  .if ${MACHINE} == "i386" || ${MACHINE} == "amd64" || \
>  ${MACHINE} == "alpha" || ${MACHINE} == "hppa" || \
> +${MACHINE} == "arm64" || ${MACHINE} == "armv7" || \
>  ${MACHINE} == "loongson"
>  
>  PROG=wsfontload
> 
> 



Re: userland clock_gettime proof of concept

2020-05-28 Thread Paul Irofti

Is the bump actually needed? Symbols.list is untouched so there's no change to
the exported symbols.


I am not sure if WRAP does not require it. Probably not. Otherwise as 
the diff stands now (always falling back to the syscall if timekeep is 
missing), I tend to agree with your statement :)




Re: userland clock_gettime proof of concept

2020-05-28 Thread Stuart Henderson
I'm running it here.

On 2020/05/28 17:44, Paul Irofti wrote:
> diff --git lib/libc/shlib_version lib/libc/shlib_version
> index 06f98b01084..5fb0770494f 100644
> --- lib/libc/shlib_version
> +++ lib/libc/shlib_version
> @@ -1,4 +1,4 @@
>  major=96
> -minor=0
> +minor=1
>  # note: If changes were made to include/thread_private.h or if system calls
>  # were added/changed then librthread/shlib_version must also be updated.

Is the bump actually needed? Symbols.list is untouched so there's no change to
the exported symbols.



Re: Enable building wsmoused on arm64 and armv7

2020-05-28 Thread Theo de Raadt
-MANSUBDIR= i386 amd64 alpha
+MANSUBDIR= i386 amd64 arm64 armv7 alpha

Actually, I suggest making this a MI man page.  Delete that line, and see
where the files land.  I'll adjust sets.



Enable building wsmoused on arm64 and armv7

2020-05-28 Thread Frederic Cambus
Hi tech@,

Here is a diff to enable building wsmoused on arm64 and armv7.

Builds and works correctly on my CubieBoard2.

Comments? OK?

Index: usr.sbin/wsmoused/Makefile
===
RCS file: /cvs/src/usr.sbin/wsmoused/Makefile,v
retrieving revision 1.7
diff -u -p -r1.7 Makefile
--- usr.sbin/wsmoused/Makefile  16 Jul 2014 20:07:03 -  1.7
+++ usr.sbin/wsmoused/Makefile  28 May 2020 16:35:14 -
@@ -1,6 +1,7 @@
 #  $OpenBSD: Makefile,v 1.7 2014/07/16 20:07:03 okan Exp $ 
 
-.if ${MACHINE} == "i386" || ${MACHINE} == "amd64" ||\
+.if ${MACHINE} == "i386" || ${MACHINE} == "amd64" || \
+${MACHINE} == "arm64" || ${MACHINE} == "armv7" || \
 ${MACHINE} == "alpha"
 
 PROG=  wsmoused
@@ -13,6 +14,6 @@ NOPROG=yes
 .endif
 
 MAN=   wsmoused.8 
-MANSUBDIR= i386 amd64 alpha
+MANSUBDIR= i386 amd64 arm64 armv7 alpha
 
 .include 



Enable building wsfontload on arm64 and armv7

2020-05-28 Thread Frederic Cambus
Hi tech@,

Here is a diff to enable building wsfontload on arm64 and armv7.

Builds and works correctly on my CubieBoard2.

Comments? OK?

Index: usr.sbin/wsfontload/Makefile
===
RCS file: /cvs/src/usr.sbin/wsfontload/Makefile,v
retrieving revision 1.16
diff -u -p -r1.16 Makefile
--- usr.sbin/wsfontload/Makefile19 Jan 2017 08:06:27 -  1.16
+++ usr.sbin/wsfontload/Makefile28 May 2020 16:34:58 -
@@ -2,6 +2,7 @@
 
 .if ${MACHINE} == "i386" || ${MACHINE} == "amd64" || \
 ${MACHINE} == "alpha" || ${MACHINE} == "hppa" || \
+${MACHINE} == "arm64" || ${MACHINE} == "armv7" || \
 ${MACHINE} == "loongson"
 
 PROG=  wsfontload



iked(8): AES-GCM in default proposals

2020-05-28 Thread Tobias Heider
Hi,

now that we finally have AES-GCM in IKE and ESP I would like to add them as
defaults.

This is a bit more complicated than one might think because AEADs and non-AEADs
can not be sent in the same proposal.  Instead we must send one proposal
for each and adjust the config parser a bit.

Test feedback welcome. ok?

diff --git a/sbin/iked/parse.y b/sbin/iked/parse.y
index 2cb00c29833..b92a4306e5d 100644
--- a/sbin/iked/parse.y
+++ b/sbin/iked/parse.y
@@ -159,6 +159,28 @@ struct iked_transform ikev2_default_ike_transforms[] = {
 size_t ikev2_default_nike_transforms = ((sizeof(ikev2_default_ike_transforms) /
 sizeof(ikev2_default_ike_transforms[0])) - 1);
 
+struct iked_transform ikev2_default_ike_transforms_noauth[] = {
+   { IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_GCM_16, 256 },
+   { IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_GCM_16, 128 },
+   { IKEV2_XFORMTYPE_PRF,  IKEV2_XFORMPRF_HMAC_SHA2_512 },
+   { IKEV2_XFORMTYPE_PRF,  IKEV2_XFORMPRF_HMAC_SHA2_384 },
+   { IKEV2_XFORMTYPE_PRF,  IKEV2_XFORMPRF_HMAC_SHA2_256 },
+   { IKEV2_XFORMTYPE_PRF,  IKEV2_XFORMPRF_HMAC_SHA1 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_CURVE25519 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_ECP_521 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_ECP_384 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_ECP_256 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_MODP_4096 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_MODP_3072 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_MODP_2048 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_MODP_1536 },
+   { IKEV2_XFORMTYPE_DH,   IKEV2_XFORMDH_MODP_1024 },
+   { 0 }
+};
+size_t ikev2_default_nike_transforms_noauth =
+((sizeof(ikev2_default_ike_transforms_noauth) /
+sizeof(ikev2_default_ike_transforms_noauth[0])) - 1);
+
 struct iked_transform ikev2_default_esp_transforms[] = {
{ IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_CBC, 256 },
{ IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_CBC, 192 },
@@ -172,6 +194,17 @@ struct iked_transform ikev2_default_esp_transforms[] = {
 size_t ikev2_default_nesp_transforms = ((sizeof(ikev2_default_esp_transforms) /
 sizeof(ikev2_default_esp_transforms[0])) - 1);
 
+struct iked_transform ikev2_default_esp_transforms_noauth[] = {
+   { IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_GCM_16, 256 },
+   { IKEV2_XFORMTYPE_ENCR, IKEV2_XFORMENCR_AES_GCM_16, 128 },
+   { IKEV2_XFORMTYPE_ESN,  IKEV2_XFORMESN_ESN },
+   { IKEV2_XFORMTYPE_ESN,  IKEV2_XFORMESN_NONE },
+   { 0 }
+};
+size_t ikev2_default_nesp_transforms_noauth =
+((sizeof(ikev2_default_esp_transforms_noauth) /
+sizeof(ikev2_default_esp_transforms_noauth[0])) - 1);
+
 const struct ipsec_xf authxfs[] = {
{ "hmac-md5",   IKEV2_XFORMAUTH_HMAC_MD5_96,16 },
{ "hmac-sha1",  IKEV2_XFORMAUTH_HMAC_SHA1_96,   20 },
@@ -2727,7 +2760,7 @@ create_ike(char *name, int af, uint8_t ipproto,
struct iked_policy   pol;
struct iked_proposal*p, *ptmp;
struct iked_transform   *xf;
-   unsigned int i, j, xfi, noauth;
+   unsigned int i, j, xfi, noauth, auth;
unsigned int ikepropid = 1, ipsecpropid = 1;
struct iked_flow*flow, *ftmp;
static unsigned int  policy_id = 0;
@@ -2857,6 +2890,17 @@ create_ike(char *name, int af, uint8_t ipproto,
RB_INIT(_flows);
 
if (ike_sa == NULL || ike_sa->nxfs == 0) {
+   /* AES-GCM proposal */
+   if ((p = calloc(1, sizeof(*p))) == NULL)
+   err(1, "%s", __func__);
+   p->prop_id = ikepropid++;
+   p->prop_protoid = IKEV2_SAPROTO_IKE;
+   p->prop_nxforms = ikev2_default_nike_transforms_noauth;
+   p->prop_xforms = ikev2_default_ike_transforms_noauth;
+   TAILQ_INSERT_TAIL(_proposals, p, prop_entry);
+   pol.pol_nproposals++;
+
+   /* Non GCM proposal */
if ((p = calloc(1, sizeof(*p))) == NULL)
err(1, "%s", __func__);
p->prop_id = ikepropid++;
@@ -2867,11 +2911,16 @@ create_ike(char *name, int af, uint8_t ipproto,
pol.pol_nproposals++;
} else {
for (i = 0; i < ike_sa->nxfs; i++) {
-   noauth = 0;
+   noauth = auth = 0;
for (j = 0; j < ike_sa->xfs[i]->nencxf; j++) {
if (ike_sa->xfs[i]->encxf[j]->noauth)
noauth++;
+   else
+   auth++;
}
+   if (ike_sa->xfs[i]->nauthxf)
+   auth++;
+
if (ike_sa->xfs[i]->nesnxf) {
yyerror("cannot use ESN with ikesa.");
goto 

Re: pipex(4): kill NET_LOCK() in pipex_ioctl()

2020-05-28 Thread Vitaliy Makkoveev
On Thu, May 28, 2020 at 12:26:39PM +0200, Martin Pieuchot wrote:
> On 27/05/20(Wed) 11:54, Vitaliy Makkoveev wrote:
> > pipex(4) is simultaneously protected by KERNEL_LOCK() and NET_LOCK() and
> > the last is not required. I guess to start remove NET_LOCK(). Diff below
> > drops NET_LOCK() in pipex_ioctl() and underlaying routines. At least
> > this helps to kill unlock/lock mess in pppx_add_session() and
> > pppx_if_destroy().
> 
> Getting rid of the NET_LOCK() might indeed help to untangle the current
> locking mess.  However we should aim towards removing the KERNEL_LOCK()
> from, at least, the packet processing path starting with pipexintr().

I guess we can made `pipexoutq' processing NET_LOCK() free. Also
`pipexinq' processing requires a little amount of places where NET_LOCK()
is required. So we can implement special locks for pipex(4).

> 
> ok mpi@
> 
> > Index: sys/net/pipex.c
> > ===
> > RCS file: /cvs/src/sys/net/pipex.c,v
> > retrieving revision 1.113
> > diff -u -p -r1.113 pipex.c
> > --- sys/net/pipex.c 7 Apr 2020 07:11:22 -   1.113
> > +++ sys/net/pipex.c 27 May 2020 08:46:32 -
> > @@ -205,7 +205,8 @@ pipex_ioctl(struct pipex_iface_context *
> >  {
> > int pipexmode, ret = 0;
> >  
> > -   NET_LOCK();
> > +   KERNEL_ASSERT_LOCKED();
> > +
> > switch (cmd) {
> > case PIPEXSMODE:
> > pipexmode = *(int *)data;
> > @@ -250,7 +251,6 @@ pipex_ioctl(struct pipex_iface_context *
> > ret = ENOTTY;
> > break;
> > }
> > -   NET_UNLOCK();
> >  
> > return (ret);
> >  }
> > @@ -269,6 +269,8 @@ pipex_add_session(struct pipex_session_r
> > struct ifnet *over_ifp = NULL;
> >  #endif
> >  
> > +   KERNEL_ASSERT_LOCKED();
> > +
> > /* Checks requeted parameters.  */
> > if (!iface->pipexmode)
> > return (ENXIO);
> > @@ -423,7 +425,6 @@ pipex_add_session(struct pipex_session_r
> > }
> >  #endif
> >  
> > -   NET_ASSERT_LOCKED();
> > /* commit the session */
> > if (!in_nullhost(session->ip_address.sin_addr)) {
> > if (pipex_lookup_by_ip_address(session->ip_address.sin_addr)
> > @@ -473,7 +474,7 @@ pipex_add_session(struct pipex_session_r
> >  int
> >  pipex_notify_close_session(struct pipex_session *session)
> >  {
> > -   NET_ASSERT_LOCKED();
> > +   KERNEL_ASSERT_LOCKED();
> > session->state = PIPEX_STATE_CLOSE_WAIT;
> > session->stat.idle_time = 0;
> > 
> 



Re: pppx(4): prevent access to `pxi' being destroyed

2020-05-28 Thread Vitaliy Makkoveev
On Thu, May 28, 2020 at 12:14:43PM +0200, Martin Pieuchot wrote:
> On 26/05/20(Tue) 16:12, Vitaliy Makkoveev wrote:
> > `pppx_if' has `pxi_ready' field used to prevent access to incomplete
> > `pxi'. But we don't prevent access to `pxi' which we destroy.
> > pppx_if_destroy() can sleep so we can grab `pxi' which we already
> > destroying by concurrent thread and cause use-after-free issue. I guess
> > to use `pxi_ready' to prevent access to `pxi' at destroy stage too.
> 
> What about setting this field as first step in pppx_if_destroy()?

This time it's makes sences.

Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.86
diff -u -p -r1.86 if_pppx.c
--- sys/net/if_pppx.c   26 May 2020 08:02:54 -  1.86
+++ sys/net/if_pppx.c   28 May 2020 12:06:41 -
@@ -1004,6 +1004,7 @@ pppx_if_destroy(struct pppx_dev *pxd, st
struct pipex_session *session;
 
NET_ASSERT_LOCKED();
+   pxi->pxi_ready = 0;
session = >pxi_session;
ifp = >pxi_if;
 



Re: Avoid offline cpu in automatic frequency scheduling

2020-05-28 Thread Solene Rapenne
On Thu, 28 May 2020 11:43:07 -0400
Bryan Steele :

> On Thu, May 28, 2020 at 04:29:19PM +0200, Solene Rapenne wrote:
> > the macro CPU_INFO_FOREACH loop over every CPU but the frequency
> > algorithm will raise frequency if one cpu usage goes over a
> > threshold but also if the sum of cpu usage goes over another
> > threshold.
> > 
> > In the current case, if you have offline cpu (because of hw.smt=0),
> > the total will still sum all the idle cpu and it's unlikely that
> > the total threshold is ever reached before the per cpu one.
> > 
> > so, this diff skip offline cpus in the loop (robert@ gave me the big
> > clue to use cpu_is_online in the loop)  
> 
> Thanks for finding this solene! Nice work.
> 
> I've been aware of some issues on AMD with the automatic frequency
> scaling and haven't been able to narrow them down. This may not solve
> all of them, but does appear to improve things on Ryzen CPUs which
> have many cores/threads and no BIOS knob to disable SMT.
> 
> Assuming others agree with the approach..
> 
> ok brynet@

I think this should help CPU with a lot of cores. If I got the
algorithm right this is doing this:

if 1 cpu has idle < 25% then scale up
OR
if sum cpu idle < 33% then scale up

when it scales up, it will bump a ticker to 5 and set perf to 100% and
check again in the next 100ms. Everytime the loop doesn't need to scale
up (but is still at 100% freq) it decrement the ticket. Once the ticket
is 0 or less, then the frequency goes to minimum.

with the current state, if you have offline cpu, you can never go under
50% of the total idle. So the only way to scale up is to have 1 CPU
with less than 25% idle.

It's a lot of magic numbers without explanation here. I'm curious if
this could be improved. I guess that changing them would tip the
balance between responsiveness and battery saving.



unlock PF_UNIX sockets

2020-05-28 Thread Vitaliy Makkoveev
socket(2) layer is already protected by solock(). It grabs NET_LOCK()
for inet{,6}(4) sockets, but all other sockets are still under
KERNEL_LOCK().

I guess solock is already placed everythere it's required. Also `struct
file' is already mp-safe. 

Diff below introduces rwlock `unp_lock'. It's like NET_LOCK()'s
`netlock' but for PF_UNIX sockets. This lock protects the whole PF_UNIX
layer. Vnode layer accessed in "connect" "bind" and "desconnect" cases
is still under KERNEL_LOCK().

I'am experimenting with his diff since 19.04.2020 and my test systems,
include Gnome workstaion are stable, without any issue.


Index: sys/kern/uipc_socket2.c
===
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
retrieving revision 1.104
diff -u -p -r1.104 uipc_socket2.c
--- sys/kern/uipc_socket2.c 11 Apr 2020 14:07:06 -  1.104
+++ sys/kern/uipc_socket2.c 1 May 2020 13:47:11 -
@@ -53,6 +53,8 @@ u_longsb_max = SB_MAX;/* patchable */
 extern struct pool mclpools[];
 extern struct pool mbpool;
 
+extern struct rwlock unp_lock;
+
 /*
  * Procedures to manipulate state flags of socket
  * and do appropriate wakeups.  Normal sequence from the
@@ -282,6 +284,8 @@ solock(struct socket *so)
NET_LOCK();
break;
case PF_UNIX:
+   rw_enter_write(_lock);
+   break;
case PF_ROUTE:
case PF_KEY:
default:
@@ -306,6 +310,8 @@ sounlock(struct socket *so, int s)
NET_UNLOCK();
break;
case PF_UNIX:
+   rw_exit_write(_lock);
+   break;
case PF_ROUTE:
case PF_KEY:
default:
@@ -323,6 +329,8 @@ soassertlocked(struct socket *so)
NET_ASSERT_LOCKED();
break;
case PF_UNIX:
+   rw_assert_wrlock(_lock);
+   break;
case PF_ROUTE:
case PF_KEY:
default:
@@ -335,12 +343,24 @@ int
 sosleep_nsec(struct socket *so, void *ident, int prio, const char *wmesg,
 uint64_t nsecs)
 {
-   if ((so->so_proto->pr_domain->dom_family != PF_UNIX) &&
-   (so->so_proto->pr_domain->dom_family != PF_ROUTE) &&
-   (so->so_proto->pr_domain->dom_family != PF_KEY)) {
-   return rwsleep_nsec(ident, , prio, wmesg, nsecs);
-   } else
-   return tsleep_nsec(ident, prio, wmesg, nsecs);
+   int ret;
+
+   switch (so->so_proto->pr_domain->dom_family) {
+   case PF_INET:
+   case PF_INET6:
+   ret = rwsleep_nsec(ident, , prio, wmesg, nsecs);
+   break;
+   case PF_UNIX:
+   ret = rwsleep_nsec(ident, _lock, prio, wmesg, nsecs);
+   break;
+   case PF_ROUTE:
+   case PF_KEY:
+   default:
+   ret = tsleep_nsec(ident, prio, wmesg, nsecs);
+   break;
+   }
+
+   return ret;
 }
 
 /*
Index: sys/kern/uipc_usrreq.c
===
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.142
diff -u -p -r1.142 uipc_usrreq.c
--- sys/kern/uipc_usrreq.c  16 Jul 2019 21:41:37 -  1.142
+++ sys/kern/uipc_usrreq.c  1 May 2020 13:47:11 -
@@ -51,10 +51,17 @@
 #include 
 #include 
 #include 
+#include 
 
+/*
+ * Locks used to protect global data and struct members:
+ *  I   immutable after creation
+ *  U   unp_lock
+ */
+struct rwlock unp_lock = RWLOCK_INITIALIZER("unplock");
 
 void   uipc_setaddr(const struct unpcb *, struct mbuf *);
 
-/* list of all UNIX domain sockets, for unp_gc() */
+/* [U] list of all UNIX domain sockets, for unp_gc() */
 LIST_HEAD(unp_head, unpcb) unp_head = LIST_HEAD_INITIALIZER(unp_head);
 
 /*
@@ -62,10 +69,10 @@ LIST_HEAD(unp_head, unpcb) unp_head = LI
  * not received and need to be closed.
  */
 struct unp_deferral {
-   SLIST_ENTRY(unp_deferral)   ud_link;
-   int ud_n;
+   SLIST_ENTRY(unp_deferral)   ud_link;/* [U] */
+   int ud_n;   /* [I] */
/* followed by ud_n struct fdpass */
-   struct fdpass ud_fp[];
+   struct fdpass   ud_fp[];/* [I] */
 };
 
 void   unp_discard(struct fdpass *, int);
@@ -74,7 +81,7 @@ void  unp_scan(struct mbuf *, void (*)(st
 intunp_nam2sun(struct mbuf *, struct sockaddr_un **, size_t *);
 
 struct pool unpcb_pool;
-/* list of sets of files that were sent over sockets that are now closed */
+/* [U] list of sets of files that were sent over sockets that are now closed */
 SLIST_HEAD(,unp_deferral) unp_deferred = SLIST_HEAD_INITIALIZER(unp_deferred);
 
 struct task unp_gc_task = TASK_INITIALIZER(unp_gc, NULL);
@@ -89,7 +96,7 @@ struct task unp_gc_task = TASK_INITIALIZ
  * need a proper out-of-band
  */
 struct sockaddr sun_noname = { 

Re: locale thread support

2020-05-28 Thread Marc Espie
On Tue, May 26, 2020 at 12:13:02AM +0200, Ingo Schwarze wrote:
> Hi,
> 
> my impression is there are two totally independent topics in this
> thread.
> 
>  1. Whether and how to make things safer, ideally terminating the
> program in a controlled manner, if it uses functions that are
> not thread-safe in an invalid manner.  I'm not addressing that
> topic in this mail.
> 
>  2. Whether we want additional non-standard xlocale.h functions
> in our libc.
> 
> Regarding topic 2, let me say up front that i do not feel strongly
> either way.  It seems to me this is mostly a question for porters.
> If some functions are widely used - even if non-standard - and help
> avoid porting hassle, then sure, let's have them unless they cause
> excessive fuss.  But the latter does not seem likely here.
> 
> There are a number of functions that seem likely useful to me off
> the top of my head, for example: querylocale, nl_langinfo_l,
> MB_CUR_MAX_L, wcwidth_l
> 
> In general, i must say i like the whole xlocale.h business not all
> that much: in a nutshell, it is doubling the number of half the
> functions under the sun, which usually indicates poor design in the
> first place, and then the vast majority of these additional functions
> are almost useless on any operating system and totally useless on
> OpenBSD.  Like, i mean, btowc_l(3)?  Or even the isdigit_l(3) which
> we already have?  You must be choking, Mr. Chisnall!  I don't think
> stuff should be added because it is out there, but only if there is
> at least some porting benefit.
> 
> Regarding the FreeBSD headers, i like them even less.  There are both
> terrible design choices and terrible implementation choices.  Regarding
> design, it appears that you *must* #include  after other
> headers - if you include it before, it won't work.  Regarding
> implementation, there is quite repulsive macro abuse in xlocale/_ctype.h;
> but probably that can be unrolled in LibreSSL style.  Either way, none
> of that hinders providing them if doing so yields benefit.
> 
> See below for a list of functions that i drafted extremely quickly,
> so beware of errors and omissions.  The point isn't to present a
> definite plan.  The point is merely to demonstrate the sheer fatness
> of the animal and to give a first impression of the degree to which
> it stinks and grunts from most of its ends.
> 
> If any of this is needed, do porters already know which functions
> are in particularly high demand?  Do you have any idea how to find
> out which ones are actually useful for porting purposes?

Serendipity

Today, I looked at an older library  for audit purposes.

Library is called udns.

If you look at the code, there's a lot of apparently useless reinvention,
like the code explicitly checks for digits using c >= '0' && c <= '9' patterns
(or isprint for that matter).

Thinking some more, I realized why: this is library code, so it can't assume
it's running under any kind of sensible locale unless it has asserted it 
itself.

In that specific context, isdigit_l and friends make a lot of sense.
Either that, or systematic uselocale wrappers for any function that can be
called from outside.

(yes, I now that isdigit/isspace are not a problem on OpenBSD, but they can
be elsewhere!)



Re: Avoid offline cpu in automatic frequency scheduling

2020-05-28 Thread Bryan Steele
On Thu, May 28, 2020 at 04:29:19PM +0200, Solene Rapenne wrote:
> the macro CPU_INFO_FOREACH loop over every CPU but the frequency
> algorithm will raise frequency if one cpu usage goes over a threshold
> but also if the sum of cpu usage goes over another threshold.
> 
> In the current case, if you have offline cpu (because of hw.smt=0), the
> total will still sum all the idle cpu and it's unlikely that the total
> threshold is ever reached before the per cpu one.
> 
> so, this diff skip offline cpus in the loop (robert@ gave me the big
> clue to use cpu_is_online in the loop)

Thanks for finding this solene! Nice work.

I've been aware of some issues on AMD with the automatic frequency
scaling and haven't been able to narrow them down. This may not solve
all of them, but does appear to improve things on Ryzen CPUs which have
many cores/threads and no BIOS knob to disable SMT.

Assuming others agree with the approach..

ok brynet@

> Index: sched_bsd.c
> ===
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 sched_bsd.c
> --- sched_bsd.c   30 Jan 2020 08:51:27 -  1.62
> +++ sched_bsd.c   28 May 2020 14:21:25 -
> @@ -576,6 +576,8 @@ setperf_auto(void *v)
>   j = 0;
>   speedup = 0;
>   CPU_INFO_FOREACH(cii, ci) {
> + if (!cpu_is_online(ci))
> + continue;
>   total = 0;
>   for (i = 0; i < CPUSTATES; i++) {
>   total += ci->ci_schedstate.spc_cp_time[i];
> 
> 



Re: userland clock_gettime proof of concept

2020-05-28 Thread Paul Irofti
Hi,

Here is a new iteration of the diff which includes support for MD high
resolution clocks. Currently only implements TSC on amd64. If the
MD function is not defined, it fallsback to the syscall.

There is the question of the skew fix, but that will be addressed in a
separate kernel diff that will not affect the current diff at all.

I could not find a way to find on which processor the process is running
on from userland without going through a syscall. If there is one please
let me know. It would make things easier.

In the meantime I have also gotten positive feedback from various
testers that run this on their main machine.

Anyway, I think we can decide on the struct name and the auxiliary
vector ID and consider this done.

Thoughts?

Paul 

diff --git lib/libc/arch/amd64/gen/Makefile.inc 
lib/libc/arch/amd64/gen/Makefile.inc
index e995309ed71..caa4452a3d9 100644
--- lib/libc/arch/amd64/gen/Makefile.inc
+++ lib/libc/arch/amd64/gen/Makefile.inc
@@ -2,6 +2,6 @@
 
 SRCS+= _setjmp.S fabs.S infinity.c ldexp.c modf.S nan.c setjmp.S \
sigsetjmp.S
-SRCS+= fpclassifyl.c isfinitel.c isinfl.c isnanl.c isnormall.c signbitl.c
+SRCS+= fpclassifyl.c rdtsc.c isfinitel.c isinfl.c isnanl.c isnormall.c 
signbitl.c
 SRCS+= flt_rounds.S fpgetmask.S fpgetround.S fpgetsticky.S fpsetmask.S \
fpsetround.S fpsetsticky.S
diff --git lib/libc/arch/amd64/gen/rdtsc.c lib/libc/arch/amd64/gen/rdtsc.c
new file mode 100644
index 000..b14c862c61a
--- /dev/null
+++ lib/libc/arch/amd64/gen/rdtsc.c
@@ -0,0 +1,26 @@
+/* $OpenBSD$ */
+/*
+ * Copyright (c) 2020 Paul Irofti 
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+
+uint64_t
+tc_get_timecount_md(void)
+{
+   uint32_t hi, lo;
+   asm volatile("rdtsc" : "=a"(lo), "=d"(hi));
+   return ((uint64_t)lo)|(((uint64_t)hi)<<32);
+}
diff --git lib/libc/asr/asr.c lib/libc/asr/asr.c
index cd056c85719..2b25d49f32a 100644
--- lib/libc/asr/asr.c
+++ lib/libc/asr/asr.c
@@ -196,11 +196,11 @@ poll_intrsafe(struct pollfd *fds, nfds_t nfds, int 
timeout)
struct timespec pollstart, pollend, elapsed;
int r;
 
-   if (clock_gettime(CLOCK_MONOTONIC, ))
+   if (WRAP(clock_gettime)(CLOCK_MONOTONIC, ))
return -1;
 
while ((r = poll(fds, 1, timeout)) == -1 && errno == EINTR) {
-   if (clock_gettime(CLOCK_MONOTONIC, ))
+   if (WRAP(clock_gettime)(CLOCK_MONOTONIC, ))
return -1;
timespecsub(, , );
timeout -= elapsed.tv_sec * 1000 + elapsed.tv_nsec / 100;
@@ -418,7 +418,7 @@ asr_check_reload(struct asr *asr)
asr->a_rtime = 0;
}
 
-   if (clock_gettime(CLOCK_MONOTONIC, ) == -1)
+   if (WRAP(clock_gettime)(CLOCK_MONOTONIC, ) == -1)
return;
 
if ((ts.tv_sec - asr->a_rtime) < RELOAD_DELAY && asr->a_rtime != 0)
diff --git lib/libc/crypt/bcrypt.c lib/libc/crypt/bcrypt.c
index 82de8fa33b7..02fd3013cc1 100644
--- lib/libc/crypt/bcrypt.c
+++ lib/libc/crypt/bcrypt.c
@@ -248,9 +248,9 @@ _bcrypt_autorounds(void)
char buf[_PASSWORD_LEN];
int duration;
 
-   clock_gettime(CLOCK_THREAD_CPUTIME_ID, );
+   WRAP(clock_gettime)(CLOCK_THREAD_CPUTIME_ID, );
bcrypt_newhash("testpassword", r, buf, sizeof(buf));
-   clock_gettime(CLOCK_THREAD_CPUTIME_ID, );
+   WRAP(clock_gettime)(CLOCK_THREAD_CPUTIME_ID, );
 
duration = after.tv_sec - before.tv_sec;
duration *= 100;
diff --git lib/libc/dlfcn/init.c lib/libc/dlfcn/init.c
index 270f54aada5..c5921851203 100644
--- lib/libc/dlfcn/init.c
+++ lib/libc/dlfcn/init.c
@@ -30,6 +30,7 @@
 #include 
 #include /* atexit */
 #include 
+#include   /* timekeep */
 #include 
 
 #include "init.h"
@@ -45,8 +46,9 @@
 /* XXX should be in an include file shared with csu */
 char   ***_csu_finish(char **_argv, char **_envp, void (*_cleanup)(void));
 
-/* provide definition for this */
+/* provide definition for these */
 int_pagesize = 0;
+void   *_timekeep = NULL;
 
 /*
  * In dynamicly linked binaries environ and __progname are overriden by
@@ -68,6 +70,12 @@ extern Elf_Ehdr __executable_start[] __attribute__((weak));
 
 /* provide definitions for these */
 const dl_cb *_dl_cb __relro = NULL;
+#if 

Re: Avoid offline cpu in automatic frequency scheduling

2020-05-28 Thread Solene Rapenne
On Thu, 28 May 2020 16:29:19 +0200
Solene Rapenne :

> so, this diff skip offline cpus in the loop (robert@ gave me the big
> clue to use cpu_is_online in the loop)

apologies, it was claudio@ but robert@ helped too.



Avoid offline cpu in automatic frequency scheduling

2020-05-28 Thread Solene Rapenne
the macro CPU_INFO_FOREACH loop over every CPU but the frequency
algorithm will raise frequency if one cpu usage goes over a threshold
but also if the sum of cpu usage goes over another threshold.

In the current case, if you have offline cpu (because of hw.smt=0), the
total will still sum all the idle cpu and it's unlikely that the total
threshold is ever reached before the per cpu one.

so, this diff skip offline cpus in the loop (robert@ gave me the big
clue to use cpu_is_online in the loop)

Index: sched_bsd.c
===
RCS file: /cvs/src/sys/kern/sched_bsd.c,v
retrieving revision 1.62
diff -u -p -r1.62 sched_bsd.c
--- sched_bsd.c 30 Jan 2020 08:51:27 -  1.62
+++ sched_bsd.c 28 May 2020 14:21:25 -
@@ -576,6 +576,8 @@ setperf_auto(void *v)
j = 0;
speedup = 0;
CPU_INFO_FOREACH(cii, ci) {
+   if (!cpu_is_online(ci))
+   continue;
total = 0;
for (i = 0; i < CPUSTATES; i++) {
total += ci->ci_schedstate.spc_cp_time[i];



Re: clear margins when remapping efifb

2020-05-28 Thread Jonathan Gray
On Thu, May 28, 2020 at 12:23:26PM +0200, Frederic Cambus wrote:
> On Wed, May 27, 2020 at 12:25:00PM +0200, Mark Kettenis wrote:
> > > Date: Wed, 27 May 2020 19:39:07 +1000
> > > From: Jonathan Gray 
> > > 
> > > When testing the row and column increase for efifb on a 1920x1080
> > > display I noticed the top part of the screen continues to contain part
> > > of a white on blue line from earlier in the dmesg even after the machine
> > > has finished booting.
> > > 
> > > RI_CENTER changes the ri_bits offset, doing RI_CLEARMARGINS in cnremap
> > > clears the fragment of a line caused by using RI_CENTER.
> > 
> > ok kettenis@
> 
> I had a different fix for this issue, which prevents the margins to
> appear in the first place.
> 
> This is why the margins appear:
> 
> In rasops(9), if 'ri_emuwidth' is larger than 'ri_width', it is set to
> 'ri_width', and similarly for 'ri_emuheight' relative to 'ri_height'.
> 
> In efifb_cnattach_common(), we call rasops_init() with EFIFB_HEIGHT
> and EFIFB_WIDTH as parameters, so on smaller screens or with larger
> fonts, or when increasing the EFIFB_HEIGHT and EFIFB_WIDTH values,
> 'ri_emu{width,height}' becomes equals to 'ri_{width,height}' and no
> centering happens.
> 
> Then in efifb_cnremap() and efifb_attach(), efifb_std_descr.nrows and
> efifb_std_descr.ncols are used instead, and in this case 'ri_emuwidth'
> and 'ri_emuheight' can now be a few pixels smaller than 'ri_width' and
> 'ri_height' and the text area is centered where it previously wasn't,
> causing the content previously displayed in what are now margins to
> remain there.
> 
> Here is a rebased version of the diff:
> 
> Index: sys/arch/amd64/amd64/efifb.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/efifb.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 efifb.c
> --- sys/arch/amd64/amd64/efifb.c  27 May 2020 07:48:02 -  1.31
> +++ sys/arch/amd64/amd64/efifb.c  28 May 2020 07:17:49 -
> @@ -222,7 +222,7 @@ efifb_attach(struct device *parent, stru
>   ri->ri_flg &= ~RI_CLEAR;
>   ri->ri_flg |= RI_VCONS | RI_WRONLY;
>  
> - rasops_init(ri, efifb_std_descr.nrows, efifb_std_descr.ncols);
> + rasops_init(ri, EFIFB_HEIGHT, EFIFB_WIDTH);
>  
>   ri->ri_ops.pack_attr(ri->ri_active, 0, 0, 0, );
>   wsdisplay_cnattach(_std_descr, ri->ri_active, ccol, crow,
> @@ -480,7 +480,7 @@ efifb_cnremap(void)
>   ri->ri_flg &= ~RI_CLEAR;
>   ri->ri_flg |= RI_CENTER | RI_WRONLY;
>  
> - rasops_init(ri, efifb_std_descr.nrows, efifb_std_descr.ncols);
> + rasops_init(ri, EFIFB_HEIGHT, EFIFB_WIDTH);
>  
>   efifb_early_cleanup();
>  }
> 
> Because the efifb resolution doesn't change, this ensures 'ri_emuwidth'
> and 'ri_emuheight' will always get the same value when we remap and
> later when we attach, so the text area is always displayed at the same
> position.
> 
> I verified it still works, and it solves the issue for me on a 1920x1080
> screen.
> 
> It was commited [1] and reverted [2] at the time we tried to increase
> EFIFB_HEIGHT / EFIFB_WIDTH. I reverted it to be on the safe side as we
> were nearing a release and it wasn't useful on its own after reverting
> the other changes.
> 
> [1] 
> https://cvsweb.openbsd.org/src/sys/arch/amd64/amd64/efifb.c?rev=1.20=text/x-cvsweb-markup
> [2] 
> https://cvsweb.openbsd.org/src/sys/arch/amd64/amd64/efifb.c?rev=1.23=text/x-cvsweb-markup

Thanks I missed that commit in the log.

The result of your diff is the area the text starts at is higher on the
panel matching amdgpu and there are no glitches when booting via efi
with amdgpu disabled.

ok jsg@



Re: diff: init efifb even if VGA is probed.

2020-05-28 Thread Jonathan Gray
On Thu, May 28, 2020 at 02:06:28PM +0200, Mark Kettenis wrote:
> > Date: Thu, 28 May 2020 20:49:18 +0900 (JST)
> > From: YASUOKA Masahiko 
> > 
> > On Thu, 28 May 2020 12:31:31 +0200 (CEST)
> > Mark Kettenis  wrote:
> > >> Date: Thu, 28 May 2020 17:01:48 +0900 (JST)
> > >> From: YASUOKA Masahiko 
> > >> 
> > >> Hi,
> > >> 
> > >> I'd like to conclude this issue.
> > >> 
> > >> On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
> > >> YASUOKA Masahiko  wrote:
> > >> > I am testing a new hardware, HPE DL20 Gen10.
> > >> > 
> > >> > When efiboot starts the kernel, the video display becomes distorted
> > >> > and never recovered until CPU reset.
> > >> > 
> > >> > Our kernel tries to initialized console twice, first trial is done
> > >> > before getting boot info and second trial is done after getting boot
> > >> > info.  Since EFI framebuffer needs "boot info", it is initialized on
> > >> > second trial.
> > >> > 
> > >> > On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
> > >> > selects vga for the console, but actually it is broken.  On usual
> > >> > machines which boot with EFI, the problem doesn't happen since they
> > >> > have no vga.
> > >> 
> > >> If we have a way to detect whether the machine has VGA.  ACPI
> > >> FADT_NO_VGA was a candidate.  But that bit is cleard both on my "HPE
> > >> DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230.  Also the
> > >> problem newly posted at misc@ (*) might be the same problem.
> > >> 
> > >>  (*) https://marc.info/?l=openbsd-misc=159064773219779=2
> > >> 
> > >> I think having the diff folowing is the best for this momemnt.
> > >> The diff does:
> > >> 
> > >>   - move cninit() after parsing bootinfo
> > >>   - give up the debug message which can be enabled if BOOTINFO_DEBUG is 
> > >> defined
> > >> 
> > >> ok?
> > > 
> > > I suspect we have to accept that there is too much broken hardware out
> > > there.
> > 
> > Finally we might have no way other than having a configuration in
> > boot.conf...
> > 
> > > There is no real reason to drop the debug messages.
> > 
> > OK, the debug messages are reverted.
> > 
> > > I'd prefer to call cninit() directly from init_x86_64, so I'd just
> > > move the call immediately after the block that calls getbootinfo().
> > > And emove the call from getbootinfo() of course.
> > 
> > I think the last diff already satisfied these things.
> > 
> > >> @@ -1395,11 +1395,6 @@ init_x86_64(paddr_t first_avail)
> > >>  i8254_startclock();
> > >>  
> > >>  /*
> > >> - * Attach the glass console early in case we need to display a 
> > >> panic.
> > >> - */
> > >> -cninit();
> > >> -
> > >> -/*
> > >>   * Initialize PAGE_SIZE-dependent variables.
> > >>   */
> > >>  uvm_setpagesize();
> > >> @@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
> > >>  } else
> > >>  panic("invalid /boot");
> > >>  
> > >> +cninit();
> > >> +
> > >>  /*
> > >>   * Memory on the AMD64 port is described by three different things.
> > >>   *
> > 
> > A hidden line which calls getbootinfo() is at just before second
> > chunk.  The updated diff was created with "-U 4" to clarify this.
> > 
> > Alternatively, are you suggesting
> > 
> > getbootinfo(bootinfo, bootinfo_size);
> > +   cninit();
> > } else
> > panic("invalid /boot");
> > 
> > ?
> > 
> > Both is OK for me.
> > 
> > How about this?
> 
> The attached diff looks good to me.
> 
> ok kettenis@

ok jsg@ on this one as well

> 
> > Index: sys/arch/amd64/amd64/machdep.c
> > ===
> > RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> > retrieving revision 1.264
> > diff -u -p -U4 -r1.264 machdep.c
> > --- sys/arch/amd64/amd64/machdep.c  16 May 2020 14:44:44 -  1.264
> > +++ sys/arch/amd64/amd64/machdep.c  28 May 2020 11:34:39 -
> > @@ -1394,13 +1394,8 @@ init_x86_64(paddr_t first_avail)
> >  
> > i8254_startclock();
> >  
> > /*
> > -* Attach the glass console early in case we need to display a panic.
> > -*/
> > -   cninit();
> > -
> > -   /*
> >  * Initialize PAGE_SIZE-dependent variables.
> >  */
> > uvm_setpagesize();
> >  
> > @@ -1420,8 +1415,10 @@ init_x86_64(paddr_t first_avail)
> > getbootinfo(bootinfo, bootinfo_size);
> > } else
> > panic("invalid /boot");
> >  
> > +   cninit();
> > +
> >  /*
> >   * Memory on the AMD64 port is described by three different things.
> >   *
> >   * 1. biosbasemem - This is outdated, and should really only be used to
> > @@ -1926,10 +1923,8 @@ getbootinfo(char *bootinfo, int bootinfo
> > bootarg32_t *q;
> > bios_ddb_t *bios_ddb;
> > bios_bootduid_t *bios_bootduid;
> > bios_bootsr_t *bios_bootsr;
> > -   int docninit = 0;
> > -
> >  #undef BOOTINFO_DEBUG
> >  #ifdef BOOTINFO_DEBUG
> > printf("bootargv:");
> >  #endif
> > @@ -1982,11 +1977,8 @@ getbootinfo(char 

Re: diff: init efifb even if VGA is probed.

2020-05-28 Thread Mark Kettenis
> Date: Thu, 28 May 2020 20:49:18 +0900 (JST)
> From: YASUOKA Masahiko 
> 
> On Thu, 28 May 2020 12:31:31 +0200 (CEST)
> Mark Kettenis  wrote:
> >> Date: Thu, 28 May 2020 17:01:48 +0900 (JST)
> >> From: YASUOKA Masahiko 
> >> 
> >> Hi,
> >> 
> >> I'd like to conclude this issue.
> >> 
> >> On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
> >> YASUOKA Masahiko  wrote:
> >> > I am testing a new hardware, HPE DL20 Gen10.
> >> > 
> >> > When efiboot starts the kernel, the video display becomes distorted
> >> > and never recovered until CPU reset.
> >> > 
> >> > Our kernel tries to initialized console twice, first trial is done
> >> > before getting boot info and second trial is done after getting boot
> >> > info.  Since EFI framebuffer needs "boot info", it is initialized on
> >> > second trial.
> >> > 
> >> > On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
> >> > selects vga for the console, but actually it is broken.  On usual
> >> > machines which boot with EFI, the problem doesn't happen since they
> >> > have no vga.
> >> 
> >> If we have a way to detect whether the machine has VGA.  ACPI
> >> FADT_NO_VGA was a candidate.  But that bit is cleard both on my "HPE
> >> DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230.  Also the
> >> problem newly posted at misc@ (*) might be the same problem.
> >> 
> >>  (*) https://marc.info/?l=openbsd-misc=159064773219779=2
> >> 
> >> I think having the diff folowing is the best for this momemnt.
> >> The diff does:
> >> 
> >>   - move cninit() after parsing bootinfo
> >>   - give up the debug message which can be enabled if BOOTINFO_DEBUG is 
> >> defined
> >> 
> >> ok?
> > 
> > I suspect we have to accept that there is too much broken hardware out
> > there.
> 
> Finally we might have no way other than having a configuration in
> boot.conf...
> 
> > There is no real reason to drop the debug messages.
> 
> OK, the debug messages are reverted.
> 
> > I'd prefer to call cninit() directly from init_x86_64, so I'd just
> > move the call immediately after the block that calls getbootinfo().
> > And emove the call from getbootinfo() of course.
> 
> I think the last diff already satisfied these things.
> 
> >> @@ -1395,11 +1395,6 @@ init_x86_64(paddr_t first_avail)
> >>i8254_startclock();
> >>  
> >>/*
> >> -   * Attach the glass console early in case we need to display a panic.
> >> -   */
> >> -  cninit();
> >> -
> >> -  /*
> >> * Initialize PAGE_SIZE-dependent variables.
> >> */
> >>uvm_setpagesize();
> >> @@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
> >>} else
> >>panic("invalid /boot");
> >>  
> >> +  cninit();
> >> +
> >>  /*
> >>   * Memory on the AMD64 port is described by three different things.
> >>   *
> 
> A hidden line which calls getbootinfo() is at just before second
> chunk.  The updated diff was created with "-U 4" to clarify this.
> 
> Alternatively, are you suggesting
> 
>   getbootinfo(bootinfo, bootinfo_size);
> + cninit();
>   } else
>   panic("invalid /boot");
> 
> ?
> 
> Both is OK for me.
> 
> How about this?

The attached diff looks good to me.

ok kettenis@

> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.264
> diff -u -p -U4 -r1.264 machdep.c
> --- sys/arch/amd64/amd64/machdep.c16 May 2020 14:44:44 -  1.264
> +++ sys/arch/amd64/amd64/machdep.c28 May 2020 11:34:39 -
> @@ -1394,13 +1394,8 @@ init_x86_64(paddr_t first_avail)
>  
>   i8254_startclock();
>  
>   /*
> -  * Attach the glass console early in case we need to display a panic.
> -  */
> - cninit();
> -
> - /*
>* Initialize PAGE_SIZE-dependent variables.
>*/
>   uvm_setpagesize();
>  
> @@ -1420,8 +1415,10 @@ init_x86_64(paddr_t first_avail)
>   getbootinfo(bootinfo, bootinfo_size);
>   } else
>   panic("invalid /boot");
>  
> + cninit();
> +
>  /*
>   * Memory on the AMD64 port is described by three different things.
>   *
>   * 1. biosbasemem - This is outdated, and should really only be used to
> @@ -1926,10 +1923,8 @@ getbootinfo(char *bootinfo, int bootinfo
>   bootarg32_t *q;
>   bios_ddb_t *bios_ddb;
>   bios_bootduid_t *bios_bootduid;
>   bios_bootsr_t *bios_bootsr;
> - int docninit = 0;
> -
>  #undef BOOTINFO_DEBUG
>  #ifdef BOOTINFO_DEBUG
>   printf("bootargv:");
>  #endif
> @@ -1982,11 +1977,8 @@ getbootinfo(char *bootinfo, int bootinfo
>   comconsunit = unit;
>   comconsaddr = consaddr;
>   comconsrate = cdp->conspeed;
>   comconsiot = X86_BUS_SPACE_IO;
> -
> - /* Probe the serial port this time. */
> - 

Re: Enable scrollback in simplefb(4)

2020-05-28 Thread Mark Kettenis
> Date: Thu, 28 May 2020 12:55:32 +0200
> From: Frederic Cambus 
> 
> Hi tech@,
> 
> Here is a diff to enable scrollback in simplefb(4).
> 
> Tested on OpenBSD/armv7 on my CubieBoard2. The only arm64 device I own
> is headless so I cannot test on this platform.
> 
> Comments? OK?

ok kettenis@

> Index: sys/dev/fdt/simplefb.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/simplefb.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 simplefb.c
> --- sys/dev/fdt/simplefb.c25 May 2020 09:55:48 -  1.10
> +++ sys/dev/fdt/simplefb.c28 May 2020 10:52:42 -
> @@ -103,6 +103,7 @@ struct wsdisplay_accessops simplefb_acce
>   .getchar = rasops_getchar,
>   .load_font = rasops_load_font,
>   .list_font = rasops_list_font,
> + .scrollback = rasops_scrollback
>  };
>  
>  int
> 
> 



Re: WireGuard patchset for OpenBSD, rev. 2

2020-05-28 Thread Claudio Jeker
On Thu, May 28, 2020 at 01:07:40PM +0200, Martin Pieuchot wrote:
> On 27/05/20(Wed) 20:18, Matt Dunwoodie wrote:
> > On Wed, 27 May 2020 09:34:53 +0200
> > Martin Pieuchot  wrote:
> > > Regarding the kernel, I'd suggest you use "#if NWG > 0" like it is
> > > done for other pseudo-drives with 'needs-flag'.  
> > 
> > For the most part there is no significant changes to other parts of the
> > network stack, so I don't believe this should be necessary. If there is
> > anything in particular that you think should be flagged like that then
> > please do say.
> 
> I'm thinking of the `inp_upcall' abstraction and the in6_ifattach()
> chunk.  Is it possible to do without adding a function pointer to
> "struct inpcb" and guard the logic with #if NWG > 0" instead?  I'm not
> saying that such abstraction is not wanted, but I would prefer we think
> it through rather than add it for one particular driver/subsystem.
> 
> Have you seen the "#if NVXLAN" chunk in udp_input()?  Maybe this hook
> could also be considered for the abstraction you're suggesting.

Why is this code not just using the existing kernel socket API?
Why is there a need for yet another upcall interface?

I would not like to have more #if XYZ in udp_input() I already feel that
other codepaths in there should be reworked because this spaghetti coding
in the input path needs to stop.
 
-- 
:wq Claudio



Re: diff: init efifb even if VGA is probed.

2020-05-28 Thread YASUOKA Masahiko
On Thu, 28 May 2020 12:31:31 +0200 (CEST)
Mark Kettenis  wrote:
>> Date: Thu, 28 May 2020 17:01:48 +0900 (JST)
>> From: YASUOKA Masahiko 
>> 
>> Hi,
>> 
>> I'd like to conclude this issue.
>> 
>> On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
>> YASUOKA Masahiko  wrote:
>> > I am testing a new hardware, HPE DL20 Gen10.
>> > 
>> > When efiboot starts the kernel, the video display becomes distorted
>> > and never recovered until CPU reset.
>> > 
>> > Our kernel tries to initialized console twice, first trial is done
>> > before getting boot info and second trial is done after getting boot
>> > info.  Since EFI framebuffer needs "boot info", it is initialized on
>> > second trial.
>> > 
>> > On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
>> > selects vga for the console, but actually it is broken.  On usual
>> > machines which boot with EFI, the problem doesn't happen since they
>> > have no vga.
>> 
>> If we have a way to detect whether the machine has VGA.  ACPI
>> FADT_NO_VGA was a candidate.  But that bit is cleard both on my "HPE
>> DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230.  Also the
>> problem newly posted at misc@ (*) might be the same problem.
>> 
>>  (*) https://marc.info/?l=openbsd-misc=159064773219779=2
>> 
>> I think having the diff folowing is the best for this momemnt.
>> The diff does:
>> 
>>   - move cninit() after parsing bootinfo
>>   - give up the debug message which can be enabled if BOOTINFO_DEBUG is 
>> defined
>> 
>> ok?
> 
> I suspect we have to accept that there is too much broken hardware out
> there.

Finally we might have no way other than having a configuration in
boot.conf...

> There is no real reason to drop the debug messages.

OK, the debug messages are reverted.

> I'd prefer to call cninit() directly from init_x86_64, so I'd just
> move the call immediately after the block that calls getbootinfo().
> And emove the call from getbootinfo() of course.

I think the last diff already satisfied these things.

>> @@ -1395,11 +1395,6 @@ init_x86_64(paddr_t first_avail)
>>  i8254_startclock();
>>  
>>  /*
>> - * Attach the glass console early in case we need to display a panic.
>> - */
>> -cninit();
>> -
>> -/*
>>   * Initialize PAGE_SIZE-dependent variables.
>>   */
>>  uvm_setpagesize();
>> @@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
>>  } else
>>  panic("invalid /boot");
>>  
>> +cninit();
>> +
>>  /*
>>   * Memory on the AMD64 port is described by three different things.
>>   *

A hidden line which calls getbootinfo() is at just before second
chunk.  The updated diff was created with "-U 4" to clarify this.

Alternatively, are you suggesting

getbootinfo(bootinfo, bootinfo_size);
+   cninit();
} else
panic("invalid /boot");

?

Both is OK for me.

How about this?

Index: sys/arch/amd64/amd64/machdep.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.264
diff -u -p -U4 -r1.264 machdep.c
--- sys/arch/amd64/amd64/machdep.c  16 May 2020 14:44:44 -  1.264
+++ sys/arch/amd64/amd64/machdep.c  28 May 2020 11:34:39 -
@@ -1394,13 +1394,8 @@ init_x86_64(paddr_t first_avail)
 
i8254_startclock();
 
/*
-* Attach the glass console early in case we need to display a panic.
-*/
-   cninit();
-
-   /*
 * Initialize PAGE_SIZE-dependent variables.
 */
uvm_setpagesize();
 
@@ -1420,8 +1415,10 @@ init_x86_64(paddr_t first_avail)
getbootinfo(bootinfo, bootinfo_size);
} else
panic("invalid /boot");
 
+   cninit();
+
 /*
  * Memory on the AMD64 port is described by three different things.
  *
  * 1. biosbasemem - This is outdated, and should really only be used to
@@ -1926,10 +1923,8 @@ getbootinfo(char *bootinfo, int bootinfo
bootarg32_t *q;
bios_ddb_t *bios_ddb;
bios_bootduid_t *bios_bootduid;
bios_bootsr_t *bios_bootsr;
-   int docninit = 0;
-
 #undef BOOTINFO_DEBUG
 #ifdef BOOTINFO_DEBUG
printf("bootargv:");
 #endif
@@ -1982,11 +1977,8 @@ getbootinfo(char *bootinfo, int bootinfo
comconsunit = unit;
comconsaddr = consaddr;
comconsrate = cdp->conspeed;
comconsiot = X86_BUS_SPACE_IO;
-
-   /* Probe the serial port this time. */
-   docninit++;
}
 #endif
 #ifdef BOOTINFO_DEBUG
printf(" console 0x%x:%d",
@@ -2022,10 +2014,8 @@ getbootinfo(char *bootinfo, int bootinfo
break;
 
case BOOTARG_EFIINFO:
bios_efiinfo = (bios_efiinfo_t *)q->ba_arg;

Re: WireGuard patchset for OpenBSD, rev. 2

2020-05-28 Thread Martin Pieuchot
On 27/05/20(Wed) 20:18, Matt Dunwoodie wrote:
> On Wed, 27 May 2020 09:34:53 +0200
> Martin Pieuchot  wrote:
> > Regarding the kernel, I'd suggest you use "#if NWG > 0" like it is
> > done for other pseudo-drives with 'needs-flag'.  
> 
> For the most part there is no significant changes to other parts of the
> network stack, so I don't believe this should be necessary. If there is
> anything in particular that you think should be flagged like that then
> please do say.

I'm thinking of the `inp_upcall' abstraction and the in6_ifattach()
chunk.  Is it possible to do without adding a function pointer to
"struct inpcb" and guard the logic with #if NWG > 0" instead?  I'm not
saying that such abstraction is not wanted, but I would prefer we think
it through rather than add it for one particular driver/subsystem.

Have you seen the "#if NVXLAN" chunk in udp_input()?  Maybe this hook
could also be considered for the abstraction you're suggesting.

> > Aren't wg_noise.c and wg_cookie.c meant to be used by if_wg.c only?
> > Or would other parts of the kernel benefit from them?  If wg(4) is
> > the only user I'm questionnings whether putting them in crypto/ is
> > the best choice.  
> 
> Yes, I'm open to moving them somewhere, the were originally in crypto/
> as they handled specifically the crypto side of WireGuard. The end
> result though, I don't think they should be merged into if_wg.c as they
> are designed to be separate to help auditing/review. Would it make
> sense to have them in sys/net/?

In my opinion putting them in sys/net/ is better.

> > Why are you using rwlock to protect cookie_* and noise_* states?  Is
> > any sleeping involved?  Did you consider a mutex with IPL_NONE? rwlock
> > introduce sleep point that might be surprising.  Did you try the
> > option WITNESS of the kernel to check your locking?  
> 
> Both the cookie_* and noise_* are expected to run concurrently, and I
> wouldn't want one thread holding up another with a mutex while
> performing (relatively) expensive crypto operations. Where possible and
> correct, we use a read lock.

As long as rwlock are used in wg(4)'s taskqs that makes sense.  However
in the processing paths of the network stack (wg_start, wg_output,
wg_input and if you introduce it wg_enqueue) they should be avoided.
The rational is that they introduce sleeping points which we still try
to avoid. 

At least wg_input() calls wg_index_get() that tries to grab a read lock.
It would be great if those small iterations could be protected by a
mutex.  I haven't done a complete audit so they might be others ;)

I'd also suggest you look at vlan_enqueue() and see if it makes sense to
implement an if_enqueue() routine.  That should allow you to avoid
useless queueing involving locking, if HFSQ is not used on top of wg(4),
and maybe help for the double error accounting ;)

> > Is mq_push() required?  I'm always surprise to see mbuf APIs grow and
> > grow over years.  Anyway, that could be a separate change.  
> 
> To be explicit, the behaviour of mq_push is required, that is: we want
> to add a packet to the queue; if the queue is full we want to drop the
> oldest packet in the queue.
> 
> I'm aware of APIs growing though, so understand the concern. While it
> would be possible to emulate this behaviour with mq_full, and then some
> combination of mq_dequeue and mq_enqueue the result would be racey and
> may have unintended side effects. The end goal is that we want to
> achieve the behaviour above atomically.

Sure, please make sure dlg@ can review this change then :)



Enable scrollback in simplefb(4)

2020-05-28 Thread Frederic Cambus
Hi tech@,

Here is a diff to enable scrollback in simplefb(4).

Tested on OpenBSD/armv7 on my CubieBoard2. The only arm64 device I own
is headless so I cannot test on this platform.

Comments? OK?

Index: sys/dev/fdt/simplefb.c
===
RCS file: /cvs/src/sys/dev/fdt/simplefb.c,v
retrieving revision 1.10
diff -u -p -r1.10 simplefb.c
--- sys/dev/fdt/simplefb.c  25 May 2020 09:55:48 -  1.10
+++ sys/dev/fdt/simplefb.c  28 May 2020 10:52:42 -
@@ -103,6 +103,7 @@ struct wsdisplay_accessops simplefb_acce
.getchar = rasops_getchar,
.load_font = rasops_load_font,
.list_font = rasops_list_font,
+   .scrollback = rasops_scrollback
 };
 
 int



mcx(4) vlan offload

2020-05-28 Thread Jonathan Matthew
This implements vlan offload in mcx(4).  vlan stripping is fairly
straightforward, as the nic just removes the tag and populates a field in
the completion queue entry.  vlan insertion is a bit funny, as the nic doesn't
do any of the work here at all.  The driver has to copy at least the L2 headers
of the packet into the send queue entry, so it can insert a tag into a
previously untagged packet while it's doing that, and somewhat lower cost
than shuffling the packet data around in an mbuf.

I've tested that this doesn't break tcp or udp checksums on vlan-tagged
packets (including udp fragments).

ok?

Index: if_mcx.c
===
RCS file: /cvs/src/sys/dev/pci/if_mcx.c,v
retrieving revision 1.48
diff -u -p -r1.48 if_mcx.c
--- if_mcx.c27 May 2020 04:03:20 -  1.48
+++ if_mcx.c28 May 2020 09:30:31 -
@@ -18,6 +18,7 @@
  */
 
 #include "bpfilter.h"
+#include "vlan.h"
 
 #include 
 #include 
@@ -92,6 +93,7 @@
((1 << MCX_LOG_FLOW_TABLE_SIZE) - MCX_NUM_STATIC_FLOWS)
 
 #define MCX_SQ_INLINE_SIZE  18
+CTASSERT(ETHER_HDR_LEN + ETHER_VLAN_ENCAP_LEN == MCX_SQ_INLINE_SIZE);
 
 /* doorbell offsets */
 #define MCX_CQ_DOORBELL_OFFSET  0
@@ -1258,6 +1260,8 @@ struct mcx_cq_entry {
 #define MCX_CQ_ENTRY_FLAGS_L4_OK   (1 << 26)
 #define MCX_CQ_ENTRY_FLAGS_L3_OK   (1 << 25)
 #define MCX_CQ_ENTRY_FLAGS_L2_OK   (1 << 24)
+#define MCX_CQ_ENTRY_FLAGS_CV  (1 << 16)
+#define MCX_CQ_ENTRY_FLAGS_VLAN_MASK   (0x)
 
uint32_tcq_lro_srqn;
uint32_t__reserved__[2];
@@ -2363,6 +2367,9 @@ mcx_attach(struct device *parent, struct
ifp->if_capabilities = IFCAP_VLAN_MTU | IFCAP_CSUM_IPv4 |
IFCAP_CSUM_UDPv4 | IFCAP_CSUM_UDPv6 | IFCAP_CSUM_TCPv4 |
IFCAP_CSUM_TCPv6;
+#if NVLAN > 0
+   ifp->if_capabilities |= IFCAP_VLAN_HWTAGGING;
+#endif
IFQ_SET_MAXLEN(>if_snd, 1024);
 
ifmedia_init(>sc_media, IFM_IMASK, mcx_media_change,
@@ -4013,6 +4020,7 @@ mcx_create_rq(struct mcx_softc *sc, int 
struct mcx_rq_ctx *mbin;
int error;
uint64_t *pas;
+   uint32_t rq_flags;
uint8_t *doorbell;
int insize, npages, paslen, token;
 
@@ -4044,7 +4052,11 @@ mcx_create_rq(struct mcx_softc *sc, int 
goto free;
}
mbin = (struct mcx_rq_ctx *)(((char 
*)mcx_cq_mbox_data(mcx_cq_mbox(, 0))) + 0x10);
-   mbin->rq_flags = htobe32(MCX_RQ_CTX_RLKEY | MCX_RQ_CTX_VLAN_STRIP_DIS);
+   rq_flags = MCX_RQ_CTX_RLKEY;
+#if NVLAN == 0
+   rq_flags |= MCX_RQ_CTX_VLAN_STRIP_DIS;
+#endif
+   mbin->rq_flags = htobe32(rq_flags);
mbin->rq_cqn = htobe32(cqn);
mbin->rq_wq.wq_type = MCX_WQ_CTX_TYPE_CYCLIC;
mbin->rq_wq.wq_pd = htobe32(sc->sc_pd);
@@ -5697,6 +5709,13 @@ mcx_process_rx(struct mcx_softc *sc, str
if (flags & MCX_CQ_ENTRY_FLAGS_L4_OK)
m->m_pkthdr.csum_flags |= M_TCP_CSUM_IN_OK |
M_UDP_CSUM_IN_OK;
+#if NVLAN > 0
+   if (flags & MCX_CQ_ENTRY_FLAGS_CV) {
+   m->m_pkthdr.ether_vtag = (flags &
+   MCX_CQ_ENTRY_FLAGS_VLAN_MASK);
+   m->m_flags |= M_VLANTAG;
+   }
+#endif
 
if (c->c_tdiff) {
uint64_t t = bemtoh64(>cq_timestamp) - c->c_timestamp;
@@ -6369,9 +6388,26 @@ mcx_start(struct ifqueue *ifq)
csum |= MCX_SQE_L4_CSUM;
sqe->sqe_mss_csum = htobe32(csum);
sqe->sqe_inline_header_size = htobe16(MCX_SQ_INLINE_SIZE);
-   m_copydata(m, 0, MCX_SQ_INLINE_SIZE,
-   (caddr_t)sqe->sqe_inline_headers);
-   m_adj(m, MCX_SQ_INLINE_SIZE);
+#if NVLAN > 0
+   if (m->m_flags & M_VLANTAG) {
+   struct ether_vlan_header *evh;
+   evh = (struct ether_vlan_header *)
+   >sqe_inline_headers;
+
+   /* slightly cheaper vlan_inject() */
+   m_copydata(m, 0, ETHER_HDR_LEN, (caddr_t)evh);
+   evh->evl_proto = evh->evl_encap_proto;
+   evh->evl_encap_proto = htons(ETHERTYPE_VLAN);
+   evh->evl_tag = htons(m->m_pkthdr.ether_vtag);
+
+   m_adj(m, ETHER_HDR_LEN);
+   } else
+#endif
+   {
+   m_copydata(m, 0, MCX_SQ_INLINE_SIZE,
+   (caddr_t)sqe->sqe_inline_headers);
+   m_adj(m, MCX_SQ_INLINE_SIZE);
+   }
 
if (mcx_load_mbuf(sc, ms, m) != 0) {
m_freem(m);



Re: diff: init efifb even if VGA is probed.

2020-05-28 Thread Mark Kettenis
> Date: Thu, 28 May 2020 17:01:48 +0900 (JST)
> From: YASUOKA Masahiko 
> 
> Hi,
> 
> I'd like to conclude this issue.
> 
> On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
> YASUOKA Masahiko  wrote:
> > I am testing a new hardware, HPE DL20 Gen10.
> > 
> > When efiboot starts the kernel, the video display becomes distorted
> > and never recovered until CPU reset.
> > 
> > Our kernel tries to initialized console twice, first trial is done
> > before getting boot info and second trial is done after getting boot
> > info.  Since EFI framebuffer needs "boot info", it is initialized on
> > second trial.
> > 
> > On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
> > selects vga for the console, but actually it is broken.  On usual
> > machines which boot with EFI, the problem doesn't happen since they
> > have no vga.
> 
> If we have a way to detect whether the machine has VGA.  ACPI
> FADT_NO_VGA was a candidate.  But that bit is cleard both on my "HPE
> DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230.  Also the
> problem newly posted at misc@ (*) might be the same problem.
> 
>  (*) https://marc.info/?l=openbsd-misc=159064773219779=2
> 
> I think having the diff folowing is the best for this momemnt.
> The diff does:
> 
>   - move cninit() after parsing bootinfo
>   - give up the debug message which can be enabled if BOOTINFO_DEBUG is 
> defined
> 
> ok?

I suspect we have to accept that there is too much broken hardware out
there.

There is no real reason to drop the debug messages.

I'd prefer to call cninit() directly from init_x86_64, so I'd just
move the call immediately after the block that calls getbootinfo().
And emove the call from getbootinfo() of course.

> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.264
> diff -u -p -r1.264 machdep.c
> --- sys/arch/amd64/amd64/machdep.c16 May 2020 14:44:44 -  1.264
> +++ sys/arch/amd64/amd64/machdep.c28 May 2020 07:40:14 -
> @@ -1395,11 +1395,6 @@ init_x86_64(paddr_t first_avail)
>   i8254_startclock();
>  
>   /*
> -  * Attach the glass console early in case we need to display a panic.
> -  */
> - cninit();
> -
> - /*
>* Initialize PAGE_SIZE-dependent variables.
>*/
>   uvm_setpagesize();
> @@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
>   } else
>   panic("invalid /boot");
>  
> + cninit();
> +
>  /*
>   * Memory on the AMD64 port is described by three different things.
>   *
> @@ -1927,12 +1924,6 @@ getbootinfo(char *bootinfo, int bootinfo
>   bios_ddb_t *bios_ddb;
>   bios_bootduid_t *bios_bootduid;
>   bios_bootsr_t *bios_bootsr;
> - int docninit = 0;
> -
> -#undef BOOTINFO_DEBUG
> -#ifdef BOOTINFO_DEBUG
> - printf("bootargv:");
> -#endif
>  
>   for (q = (bootarg32_t *)bootinfo;
>   (q->ba_type != BOOTARG_END) &&
> @@ -1942,24 +1933,15 @@ getbootinfo(char *bootinfo, int bootinfo
>   switch (q->ba_type) {
>   case BOOTARG_MEMMAP:
>   bios_memmap = (bios_memmap_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" memmap %p", bios_memmap);
> -#endif
>   break;
>   case BOOTARG_DISKINFO:
>   bios_diskinfo = (bios_diskinfo_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" diskinfo %p", bios_diskinfo);
> -#endif
>   break;
>   case BOOTARG_APMINFO:
>   /* generated by i386 boot loader */
>   break;
>   case BOOTARG_CKSUMLEN:
>   bios_cksumlen = *(u_int32_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" cksumlen %d", bios_cksumlen);
> -#endif
>   break;
>   case BOOTARG_PCIINFO:
>   /* generated by i386 boot loader */
> @@ -1983,15 +1965,8 @@ getbootinfo(char *bootinfo, int bootinfo
>   comconsaddr = consaddr;
>   comconsrate = cdp->conspeed;
>   comconsiot = X86_BUS_SPACE_IO;
> -
> - /* Probe the serial port this time. */
> - docninit++;
>   }
>  #endif
> -#ifdef BOOTINFO_DEBUG
> - printf(" console 0x%x:%d",
> - cdp->consdev, cdp->conspeed);
> -#endif
>   }
>   break;
>   case BOOTARG_BOOTMAC:
> @@ -2023,8 +1998,6 @@ getbootinfo(char *bootinfo, int bootinfo
>  
>   case BOOTARG_EFIINFO:
>   bios_efiinfo = (bios_efiinfo_t *)q->ba_arg;
> - if (bios_efiinfo->fb_addr != 0)
> -   

Re: pipex(4): kill NET_LOCK() in pipex_ioctl()

2020-05-28 Thread Martin Pieuchot
On 27/05/20(Wed) 11:54, Vitaliy Makkoveev wrote:
> pipex(4) is simultaneously protected by KERNEL_LOCK() and NET_LOCK() and
> the last is not required. I guess to start remove NET_LOCK(). Diff below
> drops NET_LOCK() in pipex_ioctl() and underlaying routines. At least
> this helps to kill unlock/lock mess in pppx_add_session() and
> pppx_if_destroy().

Getting rid of the NET_LOCK() might indeed help to untangle the current
locking mess.  However we should aim towards removing the KERNEL_LOCK()
from, at least, the packet processing path starting with pipexintr().

ok mpi@

> Index: sys/net/pipex.c
> ===
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.113
> diff -u -p -r1.113 pipex.c
> --- sys/net/pipex.c   7 Apr 2020 07:11:22 -   1.113
> +++ sys/net/pipex.c   27 May 2020 08:46:32 -
> @@ -205,7 +205,8 @@ pipex_ioctl(struct pipex_iface_context *
>  {
>   int pipexmode, ret = 0;
>  
> - NET_LOCK();
> + KERNEL_ASSERT_LOCKED();
> +
>   switch (cmd) {
>   case PIPEXSMODE:
>   pipexmode = *(int *)data;
> @@ -250,7 +251,6 @@ pipex_ioctl(struct pipex_iface_context *
>   ret = ENOTTY;
>   break;
>   }
> - NET_UNLOCK();
>  
>   return (ret);
>  }
> @@ -269,6 +269,8 @@ pipex_add_session(struct pipex_session_r
>   struct ifnet *over_ifp = NULL;
>  #endif
>  
> + KERNEL_ASSERT_LOCKED();
> +
>   /* Checks requeted parameters.  */
>   if (!iface->pipexmode)
>   return (ENXIO);
> @@ -423,7 +425,6 @@ pipex_add_session(struct pipex_session_r
>   }
>  #endif
>  
> - NET_ASSERT_LOCKED();
>   /* commit the session */
>   if (!in_nullhost(session->ip_address.sin_addr)) {
>   if (pipex_lookup_by_ip_address(session->ip_address.sin_addr)
> @@ -473,7 +474,7 @@ pipex_add_session(struct pipex_session_r
>  int
>  pipex_notify_close_session(struct pipex_session *session)
>  {
> - NET_ASSERT_LOCKED();
> + KERNEL_ASSERT_LOCKED();
>   session->state = PIPEX_STATE_CLOSE_WAIT;
>   session->stat.idle_time = 0;
> 



Re: Initialize v4l2_requestbuffers struct to avoid invalid mmap

2020-05-28 Thread Martin Pieuchot
On 26/05/20(Tue) 11:30, Ingo Feinerer wrote:
> video(1) supports reading frames from a webcam via mmap(). To inform the
> V4L2 device about the number of desired buffers containing the frames to
> be memory-mapped, a VIDIOC_REQBUFS ioctl call is used.
> 
> At the moment the v4l2_requestbuffers struct used for the VIDIOC_REQBUFS ioctl
> is only partially initialized which leads to an invalid mmap for my webcam:

The kernel driver, uvideo(4) or utvfu(4) is responsible for the rest of
the initialization.

> video: mmap: Invalid argument
> 
> With the following diff applied my camera works (only on EHCI ports with USB
> 3.0 disabled in BIOS but that is a different story).

I'm afraid this might mask a real bug.  Did you try building a kernel
with UVIDEO_DEBUG and run it to see what the error is?  The function
you're interested in is uvideo_reqbufs() which correspond to the
VIDIOC_REQBUFS ioctl(2).

> Index: video.c
> ===
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 video.c
> --- video.c   6 Nov 2019 05:46:51 -   1.29
> +++ video.c   26 May 2020 09:14:39 -
> @@ -1354,6 +1354,8 @@ mmap_init(struct video *vid)
>  
>   /* request buffers */
>   rb.count = MMAP_NUM_BUFS;
> + rb.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + rb.memory = V4L2_MEMORY_MMAP;
>   r = ioctl(vid->dev.fd, VIDIOC_REQBUFS, );
>   if (r == -1) {
>   warn("ioctl VIDIOC_REQBUFS");
> 



Re: clear margins when remapping efifb

2020-05-28 Thread Frederic Cambus
On Wed, May 27, 2020 at 12:25:00PM +0200, Mark Kettenis wrote:
> > Date: Wed, 27 May 2020 19:39:07 +1000
> > From: Jonathan Gray 
> > 
> > When testing the row and column increase for efifb on a 1920x1080
> > display I noticed the top part of the screen continues to contain part
> > of a white on blue line from earlier in the dmesg even after the machine
> > has finished booting.
> > 
> > RI_CENTER changes the ri_bits offset, doing RI_CLEARMARGINS in cnremap
> > clears the fragment of a line caused by using RI_CENTER.
> 
> ok kettenis@

I had a different fix for this issue, which prevents the margins to
appear in the first place.

This is why the margins appear:

In rasops(9), if 'ri_emuwidth' is larger than 'ri_width', it is set to
'ri_width', and similarly for 'ri_emuheight' relative to 'ri_height'.

In efifb_cnattach_common(), we call rasops_init() with EFIFB_HEIGHT
and EFIFB_WIDTH as parameters, so on smaller screens or with larger
fonts, or when increasing the EFIFB_HEIGHT and EFIFB_WIDTH values,
'ri_emu{width,height}' becomes equals to 'ri_{width,height}' and no
centering happens.

Then in efifb_cnremap() and efifb_attach(), efifb_std_descr.nrows and
efifb_std_descr.ncols are used instead, and in this case 'ri_emuwidth'
and 'ri_emuheight' can now be a few pixels smaller than 'ri_width' and
'ri_height' and the text area is centered where it previously wasn't,
causing the content previously displayed in what are now margins to
remain there.

Here is a rebased version of the diff:

Index: sys/arch/amd64/amd64/efifb.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/efifb.c,v
retrieving revision 1.31
diff -u -p -r1.31 efifb.c
--- sys/arch/amd64/amd64/efifb.c27 May 2020 07:48:02 -  1.31
+++ sys/arch/amd64/amd64/efifb.c28 May 2020 07:17:49 -
@@ -222,7 +222,7 @@ efifb_attach(struct device *parent, stru
ri->ri_flg &= ~RI_CLEAR;
ri->ri_flg |= RI_VCONS | RI_WRONLY;
 
-   rasops_init(ri, efifb_std_descr.nrows, efifb_std_descr.ncols);
+   rasops_init(ri, EFIFB_HEIGHT, EFIFB_WIDTH);
 
ri->ri_ops.pack_attr(ri->ri_active, 0, 0, 0, );
wsdisplay_cnattach(_std_descr, ri->ri_active, ccol, crow,
@@ -480,7 +480,7 @@ efifb_cnremap(void)
ri->ri_flg &= ~RI_CLEAR;
ri->ri_flg |= RI_CENTER | RI_WRONLY;
 
-   rasops_init(ri, efifb_std_descr.nrows, efifb_std_descr.ncols);
+   rasops_init(ri, EFIFB_HEIGHT, EFIFB_WIDTH);
 
efifb_early_cleanup();
 }

Because the efifb resolution doesn't change, this ensures 'ri_emuwidth'
and 'ri_emuheight' will always get the same value when we remap and
later when we attach, so the text area is always displayed at the same
position.

I verified it still works, and it solves the issue for me on a 1920x1080
screen.

It was commited [1] and reverted [2] at the time we tried to increase
EFIFB_HEIGHT / EFIFB_WIDTH. I reverted it to be on the safe side as we
were nearing a release and it wasn't useful on its own after reverting
the other changes.

[1] 
https://cvsweb.openbsd.org/src/sys/arch/amd64/amd64/efifb.c?rev=1.20=text/x-cvsweb-markup
[2] 
https://cvsweb.openbsd.org/src/sys/arch/amd64/amd64/efifb.c?rev=1.23=text/x-cvsweb-markup



Re: pppx(4): prevent access to `pxi' being destroyed

2020-05-28 Thread Martin Pieuchot
On 26/05/20(Tue) 16:12, Vitaliy Makkoveev wrote:
> `pppx_if' has `pxi_ready' field used to prevent access to incomplete
> `pxi'. But we don't prevent access to `pxi' which we destroy.
> pppx_if_destroy() can sleep so we can grab `pxi' which we already
> destroying by concurrent thread and cause use-after-free issue. I guess
> to use `pxi_ready' to prevent access to `pxi' at destroy stage too.

What about setting this field as first step in pppx_if_destroy()?
 
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 if_pppx.c
> --- sys/net/if_pppx.c 18 Apr 2020 04:03:56 -  1.84
> +++ sys/net/if_pppx.c 26 May 2020 12:59:56 -
> @@ -594,8 +594,10 @@ pppxclose(dev_t dev, int flags, int mode
>  
>   /* XXX */
>   NET_LOCK();
> - while ((pxi = LIST_FIRST(>pxd_pxis)))
> + while ((pxi = LIST_FIRST(>pxd_pxis))) {
> + pxi->pxi_ready = 0;
>   pppx_if_destroy(pxd, pxi);
> + }
>   NET_UNLOCK();
>  
>   LIST_REMOVE(pxd, pxd_entry);
> @@ -951,6 +953,7 @@ pppx_del_session(struct pppx_dev *pxd, s
>   pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol);
>   if (pxi == NULL)
>   return (EINVAL);
> + pxi->pxi_ready = 0;
>  
>   req->pcr_stat = pxi->pxi_session.stat;
>  
> 



Kill NFS-only kqueue poller thread

2020-05-28 Thread Martin Pieuchot
When it comes to kqueue filters NFS is special.  A custom thread is
created when the first event is registered.  Its purpose is to poll
for changes every 2.5sec.  This logic has been inherited from NetBSD
and is not present in FreeBSD.

Since filt_nfsread() only check `n_size' of a given nfsnode having a
different context to emulate stat(2) behavior in kernel is questionable.
So the diff below gets rid of this logic.  The rationals are KISS,
coherency with nfs_poll() and match what other FS kqueue filters do.

While here add a missing write filter.

Matching the nfs_poll() logic and the write filter are necessary to keep
the current behavior of poll(2) & select(2) once they start querying
kqfilter handlers.

ok?

Index: nfs/nfs_kq.c
===
RCS file: /cvs/src/sys/nfs/nfs_kq.c,v
retrieving revision 1.30
diff -u -p -r1.30 nfs_kq.c
--- nfs/nfs_kq.c7 Apr 2020 13:27:52 -   1.30
+++ nfs/nfs_kq.c28 May 2020 09:52:52 -
@@ -32,183 +32,26 @@
 
 #include 
 #include 
-#include 
-#include 
 #include 
-#include 
 #include 
-#include 
 #include 
-#include 
-#include 
-#include 
 
-#include 
 #include 
 #include 
 #include 
 #include 
 
-void   nfs_kqpoll(void *);
-
 void   filt_nfsdetach(struct knote *);
 intfilt_nfsread(struct knote *, long);
+intfilt_nfswrite(struct knote *, long);
 intfilt_nfsvnode(struct knote *, long);
 
-struct kevq {
-   SLIST_ENTRY(kevq)   kev_link;
-   struct vnode*vp;
-   u_int   usecount;
-   u_int   flags;
-#define KEVQ_BUSY  0x01/* currently being processed */
-#define KEVQ_WANT  0x02/* want to change this entry */
-   struct timespec omtime; /* old modification time */
-   struct timespec octime; /* old change time */
-   nlink_t onlink; /* old number of references to file */
-};
-SLIST_HEAD(kevqlist, kevq);
-
-struct rwlock nfskevq_lock = RWLOCK_INITIALIZER("nfskqlk");
-struct proc *pnfskq;
-struct kevqlist kevlist = SLIST_HEAD_INITIALIZER(kevlist);
-
-/*
- * This quite simplistic routine periodically checks for server changes
- * of any of the watched files every NFS_MINATTRTIMO/2 seconds.
- * Only changes in size, modification time, change time and nlinks
- * are being checked, everything else is ignored.
- * The routine only calls VOP_GETATTR() when it's likely it would get
- * some new data, i.e. when the vnode expires from attrcache. This
- * should give same result as periodically running stat(2) from userland,
- * while keeping CPU/network usage low, and still provide proper kevent
- * semantics.
- * The poller thread is created when first vnode is added to watch list,
- * and exits when the watch list is empty. The overhead of thread creation
- * isn't really important, neither speed of attach and detach of knote.
- */
-/* ARGSUSED */
-void
-nfs_kqpoll(void *arg)
-{
-   struct kevq *ke;
-   struct vattr attr;
-   struct proc *p = pnfskq;
-   u_quad_t osize;
-   int error;
-
-   for(;;) {
-   rw_enter_write(_lock);
-   SLIST_FOREACH(ke, , kev_link) {
-   struct nfsnode *np = VTONFS(ke->vp);
-
-#ifdef DEBUG
-   printf("nfs_kqpoll on: ");
-   VOP_PRINT(ke->vp);
-#endif
-   /* skip if still in attrcache */
-   if (nfs_getattrcache(ke->vp, ) != ENOENT)
-   continue;
-
-   /*
-* Mark entry busy, release lock and check
-* for changes.
-*/
-   ke->flags |= KEVQ_BUSY;
-   rw_exit_write(_lock);
-
-   /* save v_size, nfs_getattr() updates it */
-   osize = np->n_size;
-
-   error = VOP_GETATTR(ke->vp, , p->p_ucred, p);
-   if (error == ESTALE) {
-   NFS_INVALIDATE_ATTRCACHE(np);
-   VN_KNOTE(ke->vp, NOTE_DELETE);
-   goto next;
-   }
-
-   /* following is a bit fragile, but about best
-* we can get */
-   if (attr.va_size != osize) {
-   int flags = NOTE_WRITE;
-
-   if (attr.va_size > osize)
-   flags |= NOTE_EXTEND;
-   else
-   flags |= NOTE_TRUNCATE;
-
-   VN_KNOTE(ke->vp, flags);
-   ke->omtime = attr.va_mtime;
-   } else if (attr.va_mtime.tv_sec != ke->omtime.tv_sec
-   || attr.va_mtime.tv_nsec != ke->omtime.tv_nsec) {
-   VN_KNOTE(ke->vp, 

Re: diff: init efifb even if VGA is probed.

2020-05-28 Thread Jonathan Gray
On Thu, May 28, 2020 at 05:19:19PM +0900, YASUOKA Masahiko wrote:
> On Thu, 28 May 2020 17:01:48 +0900 (JST)
> YASUOKA Masahiko  wrote:
> > Hi,
> > 
> > I'd like to conclude this issue.
> > 
> > On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
> > YASUOKA Masahiko  wrote:
> >> I am testing a new hardware, HPE DL20 Gen10.
> >> 
> >> When efiboot starts the kernel, the video display becomes distorted
> >> and never recovered until CPU reset.
> >> 
> >> Our kernel tries to initialized console twice, first trial is done
> >> before getting boot info and second trial is done after getting boot
> >> info.  Since EFI framebuffer needs "boot info", it is initialized on
> >> second trial.
> >> 
> >> On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
> >> selects vga for the console, but actually it is broken.  On usual
> >> machines which boot with EFI, the problem doesn't happen since they
> >> have no vga.
> > 
> > If we have a way to detect whether the machine has VGA.  ACPI
> > FADT_NO_VGA was a candidate.  But that bit is cleard both on my "HPE
> > DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230.  Also the
> > problem newly posted at misc@ (*) might be the same problem.
> 
> Above paragraph may be unclear.  Let me update it by the following
> paragraph.
> 
> If we have a way to detect whether the machine has VGA, we thought we
> can select VGA or EFI framebuffer safely.  ACPI FADT_NO_VGA was a
> candidate.  But the bit is cleared both on my "HPE DL20 Gen10" and
> Andrew Daugherity's Dell PowerEdge R230.  Also the problem newly
> posted at misc@ (*) might be the same problem.

I agree this approach of waiting for bootargs before probing for a
console is the best way to handle this at the moment unless someone
can come up with a way to stop vga matching.

I have tested this without problems on
bios boot serial console
bios boot vga/inteldrm console
efi boot efifb/amdgpu console

ok jsg@

> 
> >  (*) https://marc.info/?l=openbsd-misc=159064773219779=2
> > 
> > I think having the diff folowing is the best for this momemnt.
> > The diff does:
> > 
> >   - move cninit() after parsing bootinfo
> >   - give up the debug message which can be enabled if BOOTINFO_DEBUG is 
> > defined
> > 
> > ok?
> > 
> > Index: sys/arch/amd64/amd64/machdep.c
> > ===
> > RCS file: /disk/cvs/openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> > retrieving revision 1.264
> > diff -u -p -r1.264 machdep.c
> > --- sys/arch/amd64/amd64/machdep.c  16 May 2020 14:44:44 -  1.264
> > +++ sys/arch/amd64/amd64/machdep.c  28 May 2020 07:40:14 -
> > @@ -1395,11 +1395,6 @@ init_x86_64(paddr_t first_avail)
> > i8254_startclock();
> >  
> > /*
> > -* Attach the glass console early in case we need to display a panic.
> > -*/
> > -   cninit();
> > -
> > -   /*
> >  * Initialize PAGE_SIZE-dependent variables.
> >  */
> > uvm_setpagesize();
> > @@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
> > } else
> > panic("invalid /boot");
> >  
> > +   cninit();
> > +
> >  /*
> >   * Memory on the AMD64 port is described by three different things.
> >   *
> > @@ -1927,12 +1924,6 @@ getbootinfo(char *bootinfo, int bootinfo
> > bios_ddb_t *bios_ddb;
> > bios_bootduid_t *bios_bootduid;
> > bios_bootsr_t *bios_bootsr;
> > -   int docninit = 0;
> > -
> > -#undef BOOTINFO_DEBUG
> > -#ifdef BOOTINFO_DEBUG
> > -   printf("bootargv:");
> > -#endif
> >  
> > for (q = (bootarg32_t *)bootinfo;
> > (q->ba_type != BOOTARG_END) &&
> > @@ -1942,24 +1933,15 @@ getbootinfo(char *bootinfo, int bootinfo
> > switch (q->ba_type) {
> > case BOOTARG_MEMMAP:
> > bios_memmap = (bios_memmap_t *)q->ba_arg;
> > -#ifdef BOOTINFO_DEBUG
> > -   printf(" memmap %p", bios_memmap);
> > -#endif
> > break;
> > case BOOTARG_DISKINFO:
> > bios_diskinfo = (bios_diskinfo_t *)q->ba_arg;
> > -#ifdef BOOTINFO_DEBUG
> > -   printf(" diskinfo %p", bios_diskinfo);
> > -#endif
> > break;
> > case BOOTARG_APMINFO:
> > /* generated by i386 boot loader */
> > break;
> > case BOOTARG_CKSUMLEN:
> > bios_cksumlen = *(u_int32_t *)q->ba_arg;
> > -#ifdef BOOTINFO_DEBUG
> > -   printf(" cksumlen %d", bios_cksumlen);
> > -#endif
> > break;
> > case BOOTARG_PCIINFO:
> > /* generated by i386 boot loader */
> > @@ -1983,15 +1965,8 @@ getbootinfo(char *bootinfo, int bootinfo
> > comconsaddr = consaddr;
> > comconsrate = cdp->conspeed;
> > comconsiot = X86_BUS_SPACE_IO;
> > -
> > -   /* Probe the serial port this time. */
> > -   

Re: diff: init efifb even if VGA is probed.

2020-05-28 Thread YASUOKA Masahiko
On Thu, 28 May 2020 17:01:48 +0900 (JST)
YASUOKA Masahiko  wrote:
> Hi,
> 
> I'd like to conclude this issue.
> 
> On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
> YASUOKA Masahiko  wrote:
>> I am testing a new hardware, HPE DL20 Gen10.
>> 
>> When efiboot starts the kernel, the video display becomes distorted
>> and never recovered until CPU reset.
>> 
>> Our kernel tries to initialized console twice, first trial is done
>> before getting boot info and second trial is done after getting boot
>> info.  Since EFI framebuffer needs "boot info", it is initialized on
>> second trial.
>> 
>> On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
>> selects vga for the console, but actually it is broken.  On usual
>> machines which boot with EFI, the problem doesn't happen since they
>> have no vga.
> 
> If we have a way to detect whether the machine has VGA.  ACPI
> FADT_NO_VGA was a candidate.  But that bit is cleard both on my "HPE
> DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230.  Also the
> problem newly posted at misc@ (*) might be the same problem.

Above paragraph may be unclear.  Let me update it by the following
paragraph.

If we have a way to detect whether the machine has VGA, we thought we
can select VGA or EFI framebuffer safely.  ACPI FADT_NO_VGA was a
candidate.  But the bit is cleared both on my "HPE DL20 Gen10" and
Andrew Daugherity's Dell PowerEdge R230.  Also the problem newly
posted at misc@ (*) might be the same problem.

>  (*) https://marc.info/?l=openbsd-misc=159064773219779=2
> 
> I think having the diff folowing is the best for this momemnt.
> The diff does:
> 
>   - move cninit() after parsing bootinfo
>   - give up the debug message which can be enabled if BOOTINFO_DEBUG is 
> defined
> 
> ok?
> 
> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.264
> diff -u -p -r1.264 machdep.c
> --- sys/arch/amd64/amd64/machdep.c16 May 2020 14:44:44 -  1.264
> +++ sys/arch/amd64/amd64/machdep.c28 May 2020 07:40:14 -
> @@ -1395,11 +1395,6 @@ init_x86_64(paddr_t first_avail)
>   i8254_startclock();
>  
>   /*
> -  * Attach the glass console early in case we need to display a panic.
> -  */
> - cninit();
> -
> - /*
>* Initialize PAGE_SIZE-dependent variables.
>*/
>   uvm_setpagesize();
> @@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
>   } else
>   panic("invalid /boot");
>  
> + cninit();
> +
>  /*
>   * Memory on the AMD64 port is described by three different things.
>   *
> @@ -1927,12 +1924,6 @@ getbootinfo(char *bootinfo, int bootinfo
>   bios_ddb_t *bios_ddb;
>   bios_bootduid_t *bios_bootduid;
>   bios_bootsr_t *bios_bootsr;
> - int docninit = 0;
> -
> -#undef BOOTINFO_DEBUG
> -#ifdef BOOTINFO_DEBUG
> - printf("bootargv:");
> -#endif
>  
>   for (q = (bootarg32_t *)bootinfo;
>   (q->ba_type != BOOTARG_END) &&
> @@ -1942,24 +1933,15 @@ getbootinfo(char *bootinfo, int bootinfo
>   switch (q->ba_type) {
>   case BOOTARG_MEMMAP:
>   bios_memmap = (bios_memmap_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" memmap %p", bios_memmap);
> -#endif
>   break;
>   case BOOTARG_DISKINFO:
>   bios_diskinfo = (bios_diskinfo_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" diskinfo %p", bios_diskinfo);
> -#endif
>   break;
>   case BOOTARG_APMINFO:
>   /* generated by i386 boot loader */
>   break;
>   case BOOTARG_CKSUMLEN:
>   bios_cksumlen = *(u_int32_t *)q->ba_arg;
> -#ifdef BOOTINFO_DEBUG
> - printf(" cksumlen %d", bios_cksumlen);
> -#endif
>   break;
>   case BOOTARG_PCIINFO:
>   /* generated by i386 boot loader */
> @@ -1983,15 +1965,8 @@ getbootinfo(char *bootinfo, int bootinfo
>   comconsaddr = consaddr;
>   comconsrate = cdp->conspeed;
>   comconsiot = X86_BUS_SPACE_IO;
> -
> - /* Probe the serial port this time. */
> - docninit++;
>   }
>  #endif
> -#ifdef BOOTINFO_DEBUG
> - printf(" console 0x%x:%d",
> - cdp->consdev, cdp->conspeed);
> -#endif
>   }
>   break;
>   case BOOTARG_BOOTMAC:
> @@ -2023,8 +1998,6 @@ getbootinfo(char *bootinfo, int bootinfo
>  
>   case BOOTARG_EFIINFO:
>   bios_efiinfo = (bios_efiinfo_t *)q->ba_arg;
> -

Re: WireGuard patchset for OpenBSD, rev. 2

2020-05-28 Thread Otto Moerbeek
On Thu, May 28, 2020 at 01:21:21AM -0600, Jason A. Donenfeld wrote:

> On Thu, May 28, 2020 at 1:19 AM Otto Moerbeek  wrote:
> > Of course.., I was running it from a !wxallowed mount. BTW, qemu is in
> > packages, no need to build it yourself.
> 
> Sure, but now I've been somewhat nerd sniped and am playing with this
> fcode forth implementation in qemu :-P. I wonder if there's something
> missing in the 64-bit extensions to IEEE 1275, in table.fs...

OK, can reproduce. I'll see if I can find out something.

-Otto



Re: diff: init efifb even if VGA is probed.

2020-05-28 Thread YASUOKA Masahiko
Hi,

I'd like to conclude this issue.

On Fri, 21 Feb 2020 14:09:07 +0900 (JST)
YASUOKA Masahiko  wrote:
> I am testing a new hardware, HPE DL20 Gen10.
> 
> When efiboot starts the kernel, the video display becomes distorted
> and never recovered until CPU reset.
> 
> Our kernel tries to initialized console twice, first trial is done
> before getting boot info and second trial is done after getting boot
> info.  Since EFI framebuffer needs "boot info", it is initialized on
> second trial.
> 
> On HPE DL20 Gen10, probing vga is succeeded on first trial, the kernel
> selects vga for the console, but actually it is broken.  On usual
> machines which boot with EFI, the problem doesn't happen since they
> have no vga.

If we have a way to detect whether the machine has VGA.  ACPI
FADT_NO_VGA was a candidate.  But that bit is cleard both on my "HPE
DL20 Gen10" and Andrew Daugherity's Dell PowerEdge R230.  Also the
problem newly posted at misc@ (*) might be the same problem.

 (*) https://marc.info/?l=openbsd-misc=159064773219779=2

I think having the diff folowing is the best for this momemnt.
The diff does:

  - move cninit() after parsing bootinfo
  - give up the debug message which can be enabled if BOOTINFO_DEBUG is defined

ok?

Index: sys/arch/amd64/amd64/machdep.c
===
RCS file: /disk/cvs/openbsd/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.264
diff -u -p -r1.264 machdep.c
--- sys/arch/amd64/amd64/machdep.c  16 May 2020 14:44:44 -  1.264
+++ sys/arch/amd64/amd64/machdep.c  28 May 2020 07:40:14 -
@@ -1395,11 +1395,6 @@ init_x86_64(paddr_t first_avail)
i8254_startclock();
 
/*
-* Attach the glass console early in case we need to display a panic.
-*/
-   cninit();
-
-   /*
 * Initialize PAGE_SIZE-dependent variables.
 */
uvm_setpagesize();
@@ -1421,6 +1416,8 @@ init_x86_64(paddr_t first_avail)
} else
panic("invalid /boot");
 
+   cninit();
+
 /*
  * Memory on the AMD64 port is described by three different things.
  *
@@ -1927,12 +1924,6 @@ getbootinfo(char *bootinfo, int bootinfo
bios_ddb_t *bios_ddb;
bios_bootduid_t *bios_bootduid;
bios_bootsr_t *bios_bootsr;
-   int docninit = 0;
-
-#undef BOOTINFO_DEBUG
-#ifdef BOOTINFO_DEBUG
-   printf("bootargv:");
-#endif
 
for (q = (bootarg32_t *)bootinfo;
(q->ba_type != BOOTARG_END) &&
@@ -1942,24 +1933,15 @@ getbootinfo(char *bootinfo, int bootinfo
switch (q->ba_type) {
case BOOTARG_MEMMAP:
bios_memmap = (bios_memmap_t *)q->ba_arg;
-#ifdef BOOTINFO_DEBUG
-   printf(" memmap %p", bios_memmap);
-#endif
break;
case BOOTARG_DISKINFO:
bios_diskinfo = (bios_diskinfo_t *)q->ba_arg;
-#ifdef BOOTINFO_DEBUG
-   printf(" diskinfo %p", bios_diskinfo);
-#endif
break;
case BOOTARG_APMINFO:
/* generated by i386 boot loader */
break;
case BOOTARG_CKSUMLEN:
bios_cksumlen = *(u_int32_t *)q->ba_arg;
-#ifdef BOOTINFO_DEBUG
-   printf(" cksumlen %d", bios_cksumlen);
-#endif
break;
case BOOTARG_PCIINFO:
/* generated by i386 boot loader */
@@ -1983,15 +1965,8 @@ getbootinfo(char *bootinfo, int bootinfo
comconsaddr = consaddr;
comconsrate = cdp->conspeed;
comconsiot = X86_BUS_SPACE_IO;
-
-   /* Probe the serial port this time. */
-   docninit++;
}
 #endif
-#ifdef BOOTINFO_DEBUG
-   printf(" console 0x%x:%d",
-   cdp->consdev, cdp->conspeed);
-#endif
}
break;
case BOOTARG_BOOTMAC:
@@ -2023,8 +1998,6 @@ getbootinfo(char *bootinfo, int bootinfo
 
case BOOTARG_EFIINFO:
bios_efiinfo = (bios_efiinfo_t *)q->ba_arg;
-   if (bios_efiinfo->fb_addr != 0)
-   docninit++;
break;
 
case BOOTARG_UCODE:
@@ -2032,18 +2005,9 @@ getbootinfo(char *bootinfo, int bootinfo
break;
 
default:
-#ifdef BOOTINFO_DEBUG
-   printf(" unsupported arg (%d) %p", q->ba_type,
-   q->ba_arg);
-#endif
break;
}
}
-   if (docninit > 0)
-   cninit();
-#ifdef BOOTINFO_DEBUG
-   printf("\n");
-#endif
 }
 
 int



Re: WireGuard patchset for OpenBSD, rev. 2

2020-05-28 Thread Jason A. Donenfeld
On Thu, May 28, 2020 at 1:19 AM Otto Moerbeek  wrote:
> Of course.., I was running it from a !wxallowed mount. BTW, qemu is in
> packages, no need to build it yourself.

Sure, but now I've been somewhat nerd sniped and am playing with this
fcode forth implementation in qemu :-P. I wonder if there's something
missing in the 64-bit extensions to IEEE 1275, in table.fs...



Re: WireGuard patchset for OpenBSD, rev. 2

2020-05-28 Thread Otto Moerbeek
On Thu, May 28, 2020 at 01:05:59AM -0600, Jason A. Donenfeld wrote:

> On Thu, May 28, 2020 at 12:15 AM Otto Moerbeek  wrote:
> >
> > On Wed, May 27, 2020 at 11:28:09PM -0600, Jason A. Donenfeld wrote:
> >
> > > Hi Otto,
> > >
> > > On Wed, May 27, 2020 at 4:07 AM Otto Moerbeek  wrote:
> > > > Although I'm not terribly interested in bugs that are only seen (s0
> > > > far) using emulation, please send me the details on how you set up
> > > > qemu.
> > >
> > > Right, it could very well be a TCG bug. But maybe not. Here's how to
> > > get things [not-]working:
> > >
> > > $ qemu-system-sparc64 --version
> > > QEMU emulator version 5.0.0
> > > $ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2
> > > $ qemu-img resize disk.qcow2 20G
> > > $ qemu-system-sparc64 \
> > >         -machine sun4u \
> > >         -m 1024 \
> > >         -drive file=disk.qcow2,if=ide \
> > >         -net nic,model=ne2k_pci -net user \
> > >         -nographic -serial stdio -monitor none \
> > >         -boot c
> > >
> > > OpenBIOS for Sparc64
> > > [...]
> > > Loading FCode image...
> > > Loaded 5840 bytes
> > > entry point is 0x4000
> > > Evaluating FCode...
> > > OpenBSD IEEE 1275 Bootblock 1.4
> > > ..>> OpenBSD BOOT 1.14
> > > Trying bsd...
> > > [...]
> > > OpenBSD 6.6 (RAMDISK) #84: Sat Oct 12 10:42:12 MDT 2019
> > >    dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/RAMDISK
> > > [...]
> > > Welcome to the OpenBSD/sparc64 6.6 installation program.
> > > (I)nstall, (U)pgrade, (A)utoinstall or (S)hell?
> > >
> > > If you swap out miniroot66.fs for miniroot67.fs, you'll get the error
> > > I sent prior.
> > >
> > > Jason
> > >
> >
> > Does not work for me, error message on OpenBSD/amd64:
> >
> > Could not allocate dynamic translator buffer
> >
> > ktrace snippet:
> >
> > 74960 qemu-system-spar CALL  
> > mmap(0,0x4000,0x7 > EC>,0x1002,-1,0)
> > 74960 qemu-system-spar RET   mmap -1 errno 91 Not supported
> >
> > It's doing a RWX mapping, won't fly on OpenBSD.
> >
> >         -Otto
> 
> This sequence worked fine on my OpenBSD box for reproducing the maybe-bug. 
> (See: mount option.) YMMV:
> 
> bart ~ # pkg_add git gmake glib2 bison sdl2 gsed bash xz
> [...]
> bart ~ # ftp -o - https://download.qemu.org/qemu-5.0.0.tar.xz | unxz | tar xf 
> -
> bart ~ # cd qemu-5.0.0/
> bart ~/qemu-5.0.0 # mkdir build && cd build
> bart ~/qemu-5.0.0/build # ../configure && gmake -j$(sysctl -n hw.ncpu)
> [...]
> bart ~/qemu-5.0.0/build # ftp 
> https://cdn.openbsd.org/pub/OpenBSD/6.7/sparc64/miniroot67.fs
> [...]
> bart ~/qemu-5.0.0/build # ./qemu-img convert -O qcow2 miniroot67.fs disk.qcow2
> bart ~/qemu-5.0.0/build # ./qemu-img resize disk.qcow2 20G
> Image resized.
> bart ~/qemu-5.0.0/build # mount
> /dev/sd0a on / type ffs (local, wxallowed)
> bart ~/qemu-5.0.0/build # ./sparc64-softmmu/qemu-system-sparc64 -machine 
> sun4u -m 1024 -drive file=disk.qcow2,if=ide -net nic,model=ne2k_pci -net user 
> -nographic -serial stdio -monitor none -boot c
> OpenBIOS for Sparc64
> Configuration device id QEMU version 1 machine id 0
> kernel cmdline 
> CPUs: 1 x SUNW,UltraSPARC-IIi
> UUID: ----
> Welcome to OpenBIOS v1.1 built on Oct 28 2019 17:08
>   Type 'help' for detailed information
> Trying disk:a...
> Not a bootable ELF image
> Not a bootable a.out image
> Loading FCode image...
> Loaded 6882 bytes
> entry point is 0x4000
> Evaluating FCode...
> OpenBSD IEEE 1275 Bootblock 2.0
> ..reserved fcode word.
> Unhandled Exception 0x0030
> PC = 0xffd0f3ac NPC = 0xffd0f3b0
> Stopping execution
> 
> Jason
> 

Of course.., I was running it from a !wxallowed mount. BTW, qemu is in
packages, no need to build it yourself.

-Otto



Re: WireGuard patchset for OpenBSD, rev. 2

2020-05-28 Thread Jason A. Donenfeld
On Thu, May 28, 2020 at 12:15 AM Otto Moerbeek  wrote:
>
> On Wed, May 27, 2020 at 11:28:09PM -0600, Jason A. Donenfeld wrote:
>
> > Hi Otto,
> >
> > On Wed, May 27, 2020 at 4:07 AM Otto Moerbeek  wrote:
> > > Although I'm not terribly interested in bugs that are only seen (s0
> > > far) using emulation, please send me the details on how you set up
> > > qemu.
> >
> > Right, it could very well be a TCG bug. But maybe not. Here's how to
> > get things [not-]working:
> >
> > $ qemu-system-sparc64 --version
> > QEMU emulator version 5.0.0
> > $ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2
> > $ qemu-img resize disk.qcow2 20G
> > $ qemu-system-sparc64 \
> >         -machine sun4u \
> >         -m 1024 \
> >         -drive file=disk.qcow2,if=ide \
> >         -net nic,model=ne2k_pci -net user \
> >         -nographic -serial stdio -monitor none \
> >         -boot c
> >
> > OpenBIOS for Sparc64
> > [...]
> > Loading FCode image...
> > Loaded 5840 bytes
> > entry point is 0x4000
> > Evaluating FCode...
> > OpenBSD IEEE 1275 Bootblock 1.4
> > ..>> OpenBSD BOOT 1.14
> > Trying bsd...
> > [...]
> > OpenBSD 6.6 (RAMDISK) #84: Sat Oct 12 10:42:12 MDT 2019
> >    dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/RAMDISK
> > [...]
> > Welcome to the OpenBSD/sparc64 6.6 installation program.
> > (I)nstall, (U)pgrade, (A)utoinstall or (S)hell?
> >
> > If you swap out miniroot66.fs for miniroot67.fs, you'll get the error
> > I sent prior.
> >
> > Jason
> >
>
> Does not work for me, error message on OpenBSD/amd64:
>
> Could not allocate dynamic translator buffer
>
> ktrace snippet:
>
> 74960 qemu-system-spar CALL  
> mmap(0,0x4000,0x7 EC>,0x1002,-1,0)
> 74960 qemu-system-spar RET   mmap -1 errno 91 Not supported
>
> It's doing a RWX mapping, won't fly on OpenBSD.
>
>         -Otto

This sequence worked fine on my OpenBSD box for reproducing the maybe-bug. 
(See: mount option.) YMMV:

bart ~ # pkg_add git gmake glib2 bison sdl2 gsed bash xz
[...]
bart ~ # ftp -o - https://download.qemu.org/qemu-5.0.0.tar.xz | unxz | tar xf -
bart ~ # cd qemu-5.0.0/
bart ~/qemu-5.0.0 # mkdir build && cd build
bart ~/qemu-5.0.0/build # ../configure && gmake -j$(sysctl -n hw.ncpu)
[...]
bart ~/qemu-5.0.0/build # ftp 
https://cdn.openbsd.org/pub/OpenBSD/6.7/sparc64/miniroot67.fs
[...]
bart ~/qemu-5.0.0/build # ./qemu-img convert -O qcow2 miniroot67.fs disk.qcow2
bart ~/qemu-5.0.0/build # ./qemu-img resize disk.qcow2 20G
Image resized.
bart ~/qemu-5.0.0/build # mount
/dev/sd0a on / type ffs (local, wxallowed)
bart ~/qemu-5.0.0/build # ./sparc64-softmmu/qemu-system-sparc64 -machine sun4u 
-m 1024 -drive file=disk.qcow2,if=ide -net nic,model=ne2k_pci -net user 
-nographic -serial stdio -monitor none -boot c
OpenBIOS for Sparc64
Configuration device id QEMU version 1 machine id 0
kernel cmdline 
CPUs: 1 x SUNW,UltraSPARC-IIi
UUID: ----
Welcome to OpenBIOS v1.1 built on Oct 28 2019 17:08
  Type 'help' for detailed information
Trying disk:a...
Not a bootable ELF image
Not a bootable a.out image
Loading FCode image...
Loaded 6882 bytes
entry point is 0x4000
Evaluating FCode...
OpenBSD IEEE 1275 Bootblock 2.0
..reserved fcode word.
Unhandled Exception 0x0030
PC = 0xffd0f3ac NPC = 0xffd0f3b0
Stopping execution

Jason



Re: WireGuard patchset for OpenBSD, rev. 2

2020-05-28 Thread Otto Moerbeek
On Wed, May 27, 2020 at 11:28:09PM -0600, Jason A. Donenfeld wrote:

> Hi Otto,
> 
> On Wed, May 27, 2020 at 4:07 AM Otto Moerbeek  wrote:
> > Although I'm not terribly interested in bugs that are only seen (s0
> > far) using emulation, please send me the details on how you set up
> > qemu.
> 
> Right, it could very well be a TCG bug. But maybe not. Here's how to
> get things [not-]working:
> 
> $ qemu-system-sparc64 --version
> QEMU emulator version 5.0.0
> $ qemu-img convert -O qcow2 miniroot66.fs disk.qcow2
> $ qemu-img resize disk.qcow2 20G
> $ qemu-system-sparc64 \
> -machine sun4u \
> -m 1024 \
> -drive file=disk.qcow2,if=ide \
> -net nic,model=ne2k_pci -net user \
> -nographic -serial stdio -monitor none \
> -boot c
> 
> OpenBIOS for Sparc64
> [...]
> Loading FCode image...
> Loaded 5840 bytes
> entry point is 0x4000
> Evaluating FCode...
> OpenBSD IEEE 1275 Bootblock 1.4
> ..>> OpenBSD BOOT 1.14
> Trying bsd...
> [...]
> OpenBSD 6.6 (RAMDISK) #84: Sat Oct 12 10:42:12 MDT 2019
>dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/RAMDISK
> [...]
> Welcome to the OpenBSD/sparc64 6.6 installation program.
> (I)nstall, (U)pgrade, (A)utoinstall or (S)hell?
> 
> If you swap out miniroot66.fs for miniroot67.fs, you'll get the error
> I sent prior.
> 
> Jason
> 

Does not work for me, error message on OpenBSD/amd64:

Could not allocate dynamic translator buffer

ktrace snippet:

74960 qemu-system-spar CALL  mmap(0,0x4000,0x7,0x1002,-1,0)
74960 qemu-system-spar RET   mmap -1 errno 91 Not supported

It's doing a RWX mapping, won't fly on OpenBSD.

-Otto