Re: CVS commit: src/sys/miscfs/kernfs

2021-07-18 Thread David Holland
On Mon, Jul 19, 2021 at 01:33:53AM +, David A. Holland wrote:
 > Part 3; cvs randomly didn't commit all the files the first time, still
 > hunting down the files it skipped.

ok, I'm pretty sure that's the lot.

-- 
David A. Holland
dholl...@netbsd.org


Re: CVS commit: src/sys/dev/usb

2021-07-15 Thread Nick Hudson

On 15/07/2021 04:25, Tohru Nishimura wrote:

Module Name:src
Committed By:   nisimura
Date:   Thu Jul 15 03:25:50 UTC 2021

Modified Files:
src/sys/dev/usb: if_mue.c uchcom.c

Log Message:
explanation typo


To generate a diff of this commit:
cvs rdiff -u -r1.60 -r1.61 src/sys/dev/usb/if_mue.c
cvs rdiff -u -r1.38 -r1.39 src/sys/dev/usb/uchcom.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Modified files:

Index: src/sys/dev/usb/if_mue.c
diff -u src/sys/dev/usb/if_mue.c:1.60 src/sys/dev/usb/if_mue.c:1.61
--- src/sys/dev/usb/if_mue.c:1.60   Sat Jun 27 13:33:26 2020
+++ src/sys/dev/usb/if_mue.cThu Jul 15 03:25:50 2021

[snip]

did you mean to commit the if_mue.c change? Certainly the commit message
doesn't match.

Thanks,
Nick


Re: CVS commit: src/sys/kern

2021-06-30 Thread David Holland
On Thu, Jul 01, 2021 at 12:25:51AM -0400, Christos Zoulas wrote:
 > Modified Files:
 >  src/sys/kern: vfs_vnops.c
 > 
 > Log Message:
 > don't clear the error before we use it to determine if we are moving or 
 > duping.

oh ffs...

*goes to soak head*

-- 
David A. Holland
dholl...@netbsd.org


Re: CVS commit: src/sys/external/bsd/compiler_rt/dist/lib/builtins/arm

2021-06-30 Thread Nick Hudson

On 30/06/2021 15:13, Joerg Sonnenberger wrote:

On Tue, Jun 29, 2021 at 11:26:00PM +, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Tue Jun 29 23:26:00 UTC 2021

Modified Files:
src/sys/external/bsd/compiler_rt/dist/lib/builtins/arm: aeabi_cfcmp.S
divmodsi4.S divsi3.S modsi3.S

Log Message:
Align sp to 8-byte boundary as required by EABI.


Why not just push/pop another register? That's how it is normally done
for generated code.


https://mail-index.netbsd.org/port-arm/2021/06/30/msg007341.html

Nick


Re: CVS commit: src/sys/external/bsd/compiler_rt/dist/lib/builtins/arm

2021-06-30 Thread Joerg Sonnenberger
On Tue, Jun 29, 2021 at 11:26:00PM +, Rin Okuyama wrote:
> Module Name:  src
> Committed By: rin
> Date: Tue Jun 29 23:26:00 UTC 2021
> 
> Modified Files:
>   src/sys/external/bsd/compiler_rt/dist/lib/builtins/arm: aeabi_cfcmp.S
>   divmodsi4.S divsi3.S modsi3.S
> 
> Log Message:
> Align sp to 8-byte boundary as required by EABI.

Why not just push/pop another register? That's how it is normally done
for generated code.

Joerg


Re: CVS commit: src/sys

2021-06-29 Thread Paul Goyette

On Tue, 29 Jun 2021, David A. Holland wrote:


Module Name:src
Committed By:   dholland
Date:   Tue Jun 29 22:39:21 UTC 2021

Modified Files:
src/sys/fs/puffs: puffs_vnops.c
src/sys/fs/union: union_vnops.c
src/sys/fs/unionfs: unionfs_subr.c
src/sys/kern: vfs_getcwd.c vfs_lookup.c
src/sys/sys: namei.src

Log Message:
Now remove cn_consume from struct componentname.

This change requires a kernel bump.


Note that we're riding the 9.99.86 bump from earlier today.


Note though that I'm not going to version the VOP_LOOKUP args
structure (or any other args structure) as code that doesn't touch
cn_consume doesn't need attention and code that does will fail on it
without further intervention.


To generate a diff of this commit:
cvs rdiff -u -r1.218 -r1.219 src/sys/fs/puffs/puffs_vnops.c
cvs rdiff -u -r1.76 -r1.77 src/sys/fs/union/union_vnops.c
cvs rdiff -u -r1.14 -r1.15 src/sys/fs/unionfs/unionfs_subr.c
cvs rdiff -u -r1.60 -r1.61 src/sys/kern/vfs_getcwd.c
cvs rdiff -u -r1.228 -r1.229 src/sys/kern/vfs_lookup.c
cvs rdiff -u -r1.59 -r1.60 src/sys/sys/namei.src

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


!DSPAM:60dba34c228001125714949!




++--+--+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses:|
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com|
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org  |
||  | pgoyett...@gmail.com |
++--+--+


Re: CVS commit: src/sys/dev/pad

2021-06-16 Thread Rin Okuyama

On 2021/06/16 19:23, Rin Okuyama wrote:

On 2021/06/16 18:15, Taylor R Campbell wrote:

Date: Wed, 16 Jun 2021 17:38:26 +0900
From: Rin Okuyama 

KASSERT added to pad_attach() by this commit fires on macppc with mixerctl(1):
[...] panic: kernel diagnostic assertion "KERNEL_LOCKED_P()" failed: file 
"/usr/src/sys/dev/pad/pad.c", line 214


Can you share `ident netbsd | grep -e pad.c -e subr_autoconf.c'
output?

config_attach_pseudo takes the kernel lock, so this assertion should
definitely not fire.


I don't understand why this happens on macppc, while does not on
majority of other machines. Can this behavior depend on underlying
audio(4) driver? If so, what should I do to fix?


I don't think so -- this happens before we even reach audio_attach_mi.


Thanks for hints!

Seems like kernel module bug for powerpc. pad(4) is not in GENERIC.
If it is in kernel, panic does not take place. On the other hand,
if it is supplied as module, even if its pad.c (and subr_autoconf.c
in main kernel) is up to date, panic occurs.

Another problem is kernel modules for powerpc is:

http://mail-index.netbsd.org/port-powerpc/2020/07/07/msg003590.html

I still haven't figured out why...


This turned out to be an MI bug between UP kernel v.s. modules:

http://mail-index.netbsd.org/source-changes/2021/06/16/msg130239.html

Should be fixed now :).

Thanks,
rin


Re: CVS commit: src/sys/dev/pad

2021-06-16 Thread Rin Okuyama

On 2021/06/16 18:15, Taylor R Campbell wrote:

Date: Wed, 16 Jun 2021 17:38:26 +0900
From: Rin Okuyama 

KASSERT added to pad_attach() by this commit fires on macppc with mixerctl(1):
[...] panic: kernel diagnostic assertion "KERNEL_LOCKED_P()" failed: file 
"/usr/src/sys/dev/pad/pad.c", line 214


Can you share `ident netbsd | grep -e pad.c -e subr_autoconf.c'
output?

config_attach_pseudo takes the kernel lock, so this assertion should
definitely not fire.


I don't understand why this happens on macppc, while does not on
majority of other machines. Can this behavior depend on underlying
audio(4) driver? If so, what should I do to fix?


I don't think so -- this happens before we even reach audio_attach_mi.


Thanks for hints!

Seems like kernel module bug for powerpc. pad(4) is not in GENERIC.
If it is in kernel, panic does not take place. On the other hand,
if it is supplied as module, even if its pad.c (and subr_autoconf.c
in main kernel) is up to date, panic occurs.

Another problem is kernel modules for powerpc is:

http://mail-index.netbsd.org/port-powerpc/2020/07/07/msg003590.html

I still haven't figured out why...

Thanks,
rin


Re: CVS commit: src/sys/dev/pad

2021-06-16 Thread Taylor R Campbell
> Date: Wed, 16 Jun 2021 17:38:26 +0900
> From: Rin Okuyama 
> 
> KASSERT added to pad_attach() by this commit fires on macppc with mixerctl(1):
> [...] panic: kernel diagnostic assertion "KERNEL_LOCKED_P()" failed: file 
> "/usr/src/sys/dev/pad/pad.c", line 214

Can you share `ident netbsd | grep -e pad.c -e subr_autoconf.c'
output?

config_attach_pseudo takes the kernel lock, so this assertion should
definitely not fire.

> I don't understand why this happens on macppc, while does not on
> majority of other machines. Can this behavior depend on underlying
> audio(4) driver? If so, what should I do to fix?

I don't think so -- this happens before we even reach audio_attach_mi.


Re: CVS commit: src/sys/dev/pad

2021-06-16 Thread Rin Okuyama

Hi,

KASSERT added to pad_attach() by this commit fires on macppc with mixerctl(1):


# cd /usr/tests/usr.bin/mixerctl && atf-run
...
tc-start: ..., nflag
[...] panic: kernel diagnostic assertion "KERNEL_LOCKED_P()" failed: file 
"/usr/src/sys/dev/pad/pad.c", line 214
[...] cpu0: Begin traceback...
[...] ... vpanic ...
[...] ... kern_assert ...
[...] ... pad_attach ...
[...] ... config_attach_pseudo ...
[...] ... pad_open ...
[...] ... spec_open ...
[...] ... VOP_OPEN ...
[...] ... vn_open ...
[...] ... do_open ...
[...] ... do_sys_openat ...
[...] ... sys_open ...
[...] ... syscall ...
[...] user SC trap #5 by ...


(copy from framebuffer console by hands)

I don't understand why this happens on macppc, while does not on majority of
other machines. Can this behavior depend on underlying audio(4) driver? If so,
what should I do to fix?

Thanks,
rin

On 2021/06/14 19:14, Taylor R Campbell wrote:

Module Name:src
Committed By:   riastradh
Date:   Mon Jun 14 10:14:58 UTC 2021

Modified Files:
src/sys/dev/pad: pad.c

Log Message:
pad(4): Make this exclusively a cloning device.

padN numbering never corresponded with audioM numbering except by
accident, so the non-cloning device never worked reliably for
scripting.  This simplifies the logic substantially.

While here, fix drvctl detach race.


To generate a diff of this commit:
cvs rdiff -u -r1.70 -r1.71 src/sys/dev/pad/pad.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Re: CVS commit: src/sys/dev

2021-06-09 Thread Paul Goyette

On Wed, 9 Jun 2021, Paul Goyette wrote:


Module Name:src
Committed By:   pgoyette
Date:   Wed Jun  9 23:22:51 UTC 2021

Modified Files:
src/sys/dev: dev_verbose.h

Log Message:
Use the localcount(9)-based module_hook mechanism to prevent the verbose
modules' code and data being unloaded while in use.  Should prevent some
crashes reported by Riastradh@ to occur during suspend/resume operation.


FYI, this commit "rides the bump" introduced a few hours ago by martin@


++--+-+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses:   |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com   |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
++--+-+


Re: CVS commit: src/sys/dev/audio

2021-06-08 Thread Rin Okuyama

On 2021/06/08 16:09, nia wrote:

On Tue, Jun 01, 2021 at 09:12:24PM +, Taylor R Campbell wrote:

audio(4): Set AUMODE_PLAY/RECORD only if asked _and_ supported.

If one is requested and _not_ supported, fail; otherwise we might
enter audio_write with a null play track and crash on KASSERT.


It looks like this is an incompatible change.

Sun says:

- Attempts to open a device with FREAD set fail if the device is not
   capable of recording.  (Likewise for FWRITE and playback.)
https://github.com/illumos/illumos-gate/blob/9ecd05bdc59e4a1091c51ce68cce2028d5ba6fd1/usr/src/uts/common/io/audio/impl/audio_sun.c#L70

But in NetBSD 7...

audio_open() does not return a clear failure:
https://github.com/NetBSD/src/blob/netbsd-7/sys/dev/audio.c#L1652

EINVAL is returned if !audio_can_playback in audiostartp()
https://github.com/NetBSD/src/blob/netbsd-7/sys/dev/audio.c#L2801
... which is called from audio_write()

So it looks to me like open() should succeed but write() should fail.

This is important if you want to open an audio device just to test
a few properties (i.e. AUDIO_GETPROPS, AUDIO_GETDEV...).



Also, FYI, some tests for audio(4) have started to fail somewhen
between audio.c revs 1.96 (this commit) to 1.102:

https://releng.netbsd.org/b5reports/i386/commits-2021.06.html#2021.06.02.08.46.16

Thanks,
rin


Re: CVS commit: src/sys/dev/audio

2021-06-08 Thread nia
On Tue, Jun 01, 2021 at 09:12:24PM +, Taylor R Campbell wrote:
> audio(4): Set AUMODE_PLAY/RECORD only if asked _and_ supported.
> 
> If one is requested and _not_ supported, fail; otherwise we might
> enter audio_write with a null play track and crash on KASSERT.

It looks like this is an incompatible change.

Sun says:

- Attempts to open a device with FREAD set fail if the device is not
  capable of recording.  (Likewise for FWRITE and playback.)
https://github.com/illumos/illumos-gate/blob/9ecd05bdc59e4a1091c51ce68cce2028d5ba6fd1/usr/src/uts/common/io/audio/impl/audio_sun.c#L70

But in NetBSD 7...

audio_open() does not return a clear failure:
https://github.com/NetBSD/src/blob/netbsd-7/sys/dev/audio.c#L1652

EINVAL is returned if !audio_can_playback in audiostartp()
https://github.com/NetBSD/src/blob/netbsd-7/sys/dev/audio.c#L2801
... which is called from audio_write()

So it looks to me like open() should succeed but write() should fail.

This is important if you want to open an audio device just to test
a few properties (i.e. AUDIO_GETPROPS, AUDIO_GETDEV...).


Re: CVS commit: src/sys/kern

2021-06-02 Thread Rin Okuyama

Hmm, misleading commit log (code itself is correct). More precisely:

s/the last element of ksyms_symtabs/ksyms_last_snapshot/

Anyway, does this help you to reintroduce
"ksyms(4): Don't skip symbol tables that are soon to be freed."?

That KASSERT does not fire anymore for aarch64, as far as I can see.

Thanks,
rin

On 2021/06/03 0:43, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Wed Jun  2 15:43:33 UTC 2021

Modified Files:
src/sys/kern: kern_ksyms.c

Log Message:
Fix regression introduced in rev 1.90:

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/kern_ksyms.c#rev1.90

in which the last element of ksyms_symtabs is skipped by mistake.


To generate a diff of this commit:
cvs rdiff -u -r1.93 -r1.94 src/sys/kern/kern_ksyms.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Modified files:

Index: src/sys/kern/kern_ksyms.c
diff -u src/sys/kern/kern_ksyms.c:1.93 src/sys/kern/kern_ksyms.c:1.94
--- src/sys/kern/kern_ksyms.c:1.93  Wed Jun  2 08:46:16 2021
+++ src/sys/kern/kern_ksyms.c   Wed Jun  2 15:43:33 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: kern_ksyms.c,v 1.93 2021/06/02 08:46:16 riastradh Exp $
*/
+/* $NetBSD: kern_ksyms.c,v 1.94 2021/06/02 15:43:33 rin Exp $  */
  
  /*-

   * Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -73,7 +73,7 @@
   */
  
  #include 

-__KERNEL_RCSID(0, "$NetBSD: kern_ksyms.c,v 1.93 2021/06/02 08:46:16 riastradh Exp 
$");
+__KERNEL_RCSID(0, "$NetBSD: kern_ksyms.c,v 1.94 2021/06/02 15:43:33 rin Exp 
$");
  
  #if defined(_KERNEL) && defined(_KERNEL_OPT)

  #include "opt_copy_symtab.h"
@@ -1087,7 +1087,7 @@ ksymsread(dev_t dev, struct uio *uio, in
 */
filepos = sizeof(struct ksyms_hdr);
for (st = TAILQ_FIRST(_symtabs);
-st != ksyms_last_snapshot;
+st != TAILQ_NEXT(ksyms_last_snapshot, sd_queue);
 st = TAILQ_NEXT(st, sd_queue)) {
if (__predict_false(st->sd_gone))
continue;
@@ -1109,7 +1109,7 @@ ksymsread(dev_t dev, struct uio *uio, in
KASSERT(filepos <= sizeof(struct ksyms_hdr) +
ksyms_hdr.kh_shdr[SYMTAB].sh_size);
for (st = TAILQ_FIRST(_symtabs);
-st != ksyms_last_snapshot;
+st != TAILQ_NEXT(ksyms_last_snapshot, sd_queue);
 st = TAILQ_NEXT(st, sd_queue)) {
if (__predict_false(st->sd_gone))
continue;



Re: CVS commit: src/sys/kern

2021-06-02 Thread Taylor R Campbell
> Date: Wed, 2 Jun 2021 17:33:39 +0900
> From: Rin Okuyama 
> 
> On 2021/06/02 6:11, Taylor R Campbell wrote:
> > -   KASSERT(filepos <= sizeof(struct ksyms_hdr) +
> > +   KASSERT(filepos == sizeof(struct ksyms_hdr) +
> > ksyms_hdr.kh_shdr[SYMTAB].sh_size);
> ...
> 
> This KASSERT fires at least for arm and aarch64, with savecore or ``nm 
> /dev/ksyms'':

Oops.  I reverted that change for now, will investigate later.


Re: CVS commit: src/sys/kern

2021-06-02 Thread Rin Okuyama

Hi,

On 2021/06/02 6:11, Taylor R Campbell wrote:

Module Name:src
Committed By:   riastradh
Date:   Tue Jun  1 21:11:52 UTC 2021

Modified Files:
src/sys/kern: kern_ksyms.c

Log Message:
ksyms(4): Don't skip symbol tables that are soon to be freed.

They will not actually be freed until /dev/ksyms is closed, so
continued access to them remains kosher.


To generate a diff of this commit:
cvs rdiff -u -r1.91 -r1.92 src/sys/kern/kern_ksyms.c

...

Index: src/sys/kern/kern_ksyms.c
diff -u src/sys/kern/kern_ksyms.c:1.91 src/sys/kern/kern_ksyms.c:1.92
--- src/sys/kern/kern_ksyms.c:1.91  Tue Jun  1 21:11:07 2021
+++ src/sys/kern/kern_ksyms.c   Tue Jun  1 21:11:52 2021

...

-   KASSERT(filepos <= sizeof(struct ksyms_hdr) +
+   KASSERT(filepos == sizeof(struct ksyms_hdr) +
ksyms_hdr.kh_shdr[SYMTAB].sh_size);

...

This KASSERT fires at least for arm and aarch64, with savecore or ``nm 
/dev/ksyms'':

$ nm /dev/ksyms
[ 1501.3538912] panic: kernel diagnostic assertion "filepos == sizeof(struct ksyms_hdr) + 
ksyms_hdr.kh_shdr[SYMTAB].sh_size" failed: file "../../../../kern/kern_ksyms.c", 
line 1107
[ 1501.3701109] cpu0: Begin traceback...
[ 1501.3701109] 0xc9c2ecfc: netbsd:db_panic+0xc
[ 1501.3806387] 0xc9c2ed14: netbsd:vpanic+0xc4
[ 1501.3806387] 0xc9c2ed2c: netbsd:kern_assert+0x3c
[ 1501.3906766] 0xc9c2ed6c: netbsd:ksymsread+0x208
[ 1501.4006964] 0xc9c2ee1c: netbsd:spec_read+0xbc
[ 1501.4112302] 0xc9c2ee44: netbsd:VOP_READ+0x58
[ 1501.4112302] 0xc9c2ee6c: netbsd:vn_read+0x78
[ 1501.4215160] 0xc9c2eebc: netbsd:dofileread+0x80
[ 1501.4315439] 0xc9c2eeec: netbsd:sys_read+0x50
[ 1501.4426847] 0xc9c2efac: netbsd:syscall+0x188
[ 1501.4426847] cpu0: End traceback...
Stopped in pid 11825.11825 (nm) at  netbsd:cpu_Debugger+0x4:bx
r14
db>

Can you please take a look?

Thanks,
rin


Re: CVS commit: src/sys/lib/libunwind

2021-05-31 Thread Rin Okuyama

Correction:

On 2021/06/01 7:10, Rin Okuyama wrote:

On 2021/05/31 23:35, Joerg Sonnenberger wrote:

On Mon, May 31, 2021 at 12:12:24PM +, Rin Okuyama wrote:

Bump LAST_REGISTER and LAST_RESTORE_REG to REGNO_ARM32_S31 for arm.


This is not desirable as it significantly increases the memory use.
The goal here is to actually alias the single and double register in the
space. That's what the existing was doing.


Existing code did not.

With LAST_REGISTER = REGNO_ARM32_D31, libunwind gave up unwinding frames
when it encounters s0-s31; see parseInstructions():

https://nxr.netbsd.org/xref/src/sys/lib/libunwind/DwarfParser.hpp#parseInstructions


This paragraph:


And with LAST_RESTORE_REG = REGNO_ARM32_D31, it did not copy-back contents
of s0-s31 at all; see stepWithDwarf():

https://nxr.netbsd.org/xref/src/sys/lib/libunwind/DwarfInstructions.hpp#136


is only applied to the case of LAST_REGISTER = REGNO_ARM32_S31, and
not to existing code.

However, LAST_RESTORE_REG itself has nothing to do with memory usage.
And moreover, the conclusion below is not changed at all.

Thanks,
rin


IMO, in order to support VFP-register aliasing, we need to heavily modify
MI codes. This adds complexities to the code, and only ARM would receive
benefits from them.

Existing code was not acceptable for me; for example, GDB aborted *every
time* exceptions were raised. I won't revert this change at the moment.
Please provide working alternative if you don't like mine.

Thanks,
rin



Joerg



Re: CVS commit: src/sys/lib/libunwind

2021-05-31 Thread Rin Okuyama

On 2021/05/31 23:35, Joerg Sonnenberger wrote:

On Mon, May 31, 2021 at 12:12:24PM +, Rin Okuyama wrote:

Bump LAST_REGISTER and LAST_RESTORE_REG to REGNO_ARM32_S31 for arm.


This is not desirable as it significantly increases the memory use.
The goal here is to actually alias the single and double register in the
space. That's what the existing was doing.


Existing code did not.

With LAST_REGISTER = REGNO_ARM32_D31, libunwind gave up unwinding frames
when it encounters s0-s31; see parseInstructions():

https://nxr.netbsd.org/xref/src/sys/lib/libunwind/DwarfParser.hpp#parseInstructions

And with LAST_RESTORE_REG = REGNO_ARM32_D31, it did not copy-back contents
of s0-s31 at all; see stepWithDwarf():

https://nxr.netbsd.org/xref/src/sys/lib/libunwind/DwarfInstructions.hpp#136

IMO, in order to support VFP-register aliasing, we need to heavily modify
MI codes. This adds complexities to the code, and only ARM would receive
benefits from them.

Existing code was not acceptable for me; for example, GDB aborted *every
time* exceptions were raised. I won't revert this change at the moment.
Please provide working alternative if you don't like mine.

Thanks,
rin



Joerg



Re: CVS commit: src/sys/lib/libunwind

2021-05-31 Thread Rin Okuyama

On 2021/05/31 23:32, Joerg Sonnenberger wrote:

On Mon, May 31, 2021 at 12:12:24PM +, Rin Okuyama wrote:

There are two numbering schemes for VFPv2 registers: s0-s31 and d0-d15.
The former is used by GCC, and the latter is by LLVM. Since libunwind was
derived from LLVM, it has never supported the former. This results in
crashes for GCC-compiled binaries in exception handler of C++, if it
encounters VFPv2 registers when unwinding frames.


This is only half correct. GCC actually switched at some point.


I don't think so.

At least, when they supported VFPv3 back to 2009:

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=854b8a40570fee8d8c22acf21355a8ab88f17557

and when they introduced arm_dbx_register_number(), which converts b/w
internal/DWARF regnum, back to 2005:

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=2fa330b29a650365d4d88e4407fdbc2934dcb1b4

they used s0-s31 numbering for d0-d15.

More accurately, I *imagine*, GCC actually switched at some point to use
VFP registers for general purposes (mainly as cache for stack variables
for -O2, as far as I can see). Before this switch, only applications
using floating-point arithmetic were affected. But, after that, most
C++ applications became affected, and the problem got visible to us.

Thanks,
rin



Joerg



Re: CVS commit: src/sys/lib/libunwind

2021-05-31 Thread Rin Okuyama

On 2021/05/31 23:30, Joerg Sonnenberger wrote:

On Mon, May 31, 2021 at 11:41:22AM +, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Mon May 31 11:41:22 UTC 2021

Modified Files:
src/sys/lib/libunwind: Registers.hpp unwind_registers.S

Log Message:
...

- Introduce enum for flags.


Please undo this part. enums should *not* be used for flags.


Done. Thanks for pointing it out.

rin



Joerg



Re: CVS commit: src/sys/lib/libunwind

2021-05-31 Thread Joerg Sonnenberger
On Mon, May 31, 2021 at 12:12:24PM +, Rin Okuyama wrote:
> Bump LAST_REGISTER and LAST_RESTORE_REG to REGNO_ARM32_S31 for arm.

This is not desirable as it significantly increases the memory use.
The goal here is to actually alias the single and double register in the
space. That's what the existing was doing.

Joerg


Re: CVS commit: src/sys/lib/libunwind

2021-05-31 Thread Joerg Sonnenberger
On Mon, May 31, 2021 at 12:12:24PM +, Rin Okuyama wrote:
> There are two numbering schemes for VFPv2 registers: s0-s31 and d0-d15.
> The former is used by GCC, and the latter is by LLVM. Since libunwind was
> derived from LLVM, it has never supported the former. This results in
> crashes for GCC-compiled binaries in exception handler of C++, if it
> encounters VFPv2 registers when unwinding frames.

This is only half correct. GCC actually switched at some point.

Joerg


Re: CVS commit: src/sys/lib/libunwind

2021-05-31 Thread Joerg Sonnenberger
On Mon, May 31, 2021 at 11:41:22AM +, Rin Okuyama wrote:
> Module Name:  src
> Committed By: rin
> Date: Mon May 31 11:41:22 UTC 2021
> 
> Modified Files:
>   src/sys/lib/libunwind: Registers.hpp unwind_registers.S
> 
> Log Message:
> ...
> 
> - Introduce enum for flags.

Please undo this part. enums should *not* be used for flags.

Joerg


Re: CVS commit: src/sys/arch/arm/include/arm32

2021-05-30 Thread Simon Burge
"Rin Okuyama" wrote:

> Module Name:  src
> Committed By: rin
> Date: Sun May 30 07:20:00 UTC 2021
>
> Modified Files:
>
>   src/sys/arch/arm/include/arm32: param.h
>
> Log Message:
>
> Include opt_param.h for MSGBUFSIZE ifdef _KERNEL_OPT.

Thanks Rin!  I thought I had checked all the ways MSGBUFSIZE
was pulled in.

Cheers,
Simon.


Re: CVS commit: src/sys/net

2021-05-06 Thread Christos Zoulas
In article <20210506061816.94bb1f...@cvs.netbsd.org>,
Shoichi YAMAGUCHI  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  yamaguchi
>Date:  Thu May  6 06:18:16 UTC 2021
>
>Modified Files:
>   src/sys/net: if_spppsubr.c
>
>Log Message:
>do not clear destination address if there is no saved address
>and add initialization of saved_hisaddr for safety

You don't need to byte-swap in order to compare with 0... :-)

christos



Re: CVS commit: src/sys

2021-04-30 Thread Taylor R Campbell
> Module Name:src
> Committed By:   thorpej
> Date:   Sat Apr 24 23:37:01 UTC 2021
> 
> Log Message:
> Merge thorpej-cfargs branch:
> 
> Simplify and make extensible the config_search() / config_found() /
> config_attach() interfaces: rather than having different variants for
> which arguments you want pass along, just have a single call that
> takes a variadic list of tag-value arguments.

I hate to do this for such a large merge, and I appreciate what you've
done to work on fixing fallout promptly, but... I have to ask that
this be reverted.

I didn't realize that your message to tech-kern meant `I am going to
merge this imminently', and although my response on tech-kern didn't
go out until after the merge (I'm behind on source-changes, didn't
notice you'd already merged a few hours before I replied), I had
registered the same objections privately before the merge:

This creates a new class of stack garbage bugs that the compiler will
do nothing to help detect (mismatched tag/value pairs, missing EOL),
and, worse, it affects many code paths that are essentially guaranteed
never to be exercised by automatic tests because they are all
scattered throughout device drivers for hardware which is mostly not
virtualized.

Even if the config_found_ia_ia_cthulhu_fhtagn API may have too many
variant functions, at least they have types the compiler can check
(aside from the attach args passed as void * -- which is a design
mistake we should fix!).  I'm still a little unclear on what the
benefits of the change are, but losing type safety here is a serious
regression.

We can discuss better ways to do the config_found* API, but that
discussion needs to happen first (and concerns addressed, not brushed
off) before investment in widespread changes.  Let's please take
advantage of C's type system to avoid creating new bug classes,
especially new bug classes that can't be automatically tested -- I
suggested a couple of ways to do that already, and I think we could do
even better with some tweaks to config(8) to enable typed interface
attributes.


re: CVS commit: src/sys/net

2021-04-29 Thread matthew green
> (I haven't investigated exactly what's going on leading to ~5 KB-byte
> stack frames, but this shuts gcc up, at least, and the hypotheses
> sound plausible to me!)

this matches what i saw when looking at it.  i was going to
test the exact same idea (noinline).


.mrg.


Re: CVS commit: src/sys/dev

2021-04-23 Thread Paul Goyette

On Sat, 24 Apr 2021, Robert Elz wrote:


   Date:Sat, 24 Apr 2021 00:15:37 +
   From:"Michael Lorenz" 
   Message-ID:  <20210424001537.c5c83f...@cvs.netbsd.org>

 | add an ioctl() to get a list of fonts currently available via wsfont

It seems to me it would be useful for that ioctl to copyout()
the fi_numentries field of the struct (if addr != NULL) from
wsdisplayio_listfonts() just before the ENOMEM check (so it is
updated, even if ENOMEM is returned).   (Does it make any sense
for addr to be NULL, or should that be an error?  EINVAL or something.)

Otherwise, there doesn't seem to be any easy way for the user of
the ioctl to know how many fonts were returned (checking which elements
of the array were modified is not "easy") or how big the buffer would
need to be to fetch all of them in the ENOMEM case.


In several other places, we return "total space needed" separately,
regardless of how much data was actually copied.  The general paradigm
is (more or less)

buff = NULL;
size = 0;
err = func(..., buff, size, );
while (err == 0) {
if (need > size) {
free(buff);
buff = malloc(need);
if (buff == NULL)
err = ENOMEM;

}
}

For a real-life example look at the modctl(2) code for MODULE_STAT



++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/dev

2021-04-23 Thread Robert Elz
Oh, I see from your code change to wsfontload.c that you intended
for the fi_numentries field to get copied out, it just doesn't seem
to happen.

I also see that the addr==NULL case happens if malloc() (in wsfontload.c)
failed - going ahead with the ioctl() in that case seems like a mistake,
optimising away the error checking that way looks fragile.   (And the magic
4096 even moreso).

Just check the malloc() result, and then in the ioctl code, test for it
as well, and return an error from there, rather than doing all the work
for no benefit in that case.Alternatively, you could define that case
to be a "fetch the count" variant of the ioctl, where all that happens in
that case is that the fi_numentries field of the struct is filled in and
returned.

kre

kre



Re: CVS commit: src/sys/dev

2021-04-23 Thread Robert Elz
Date:Sat, 24 Apr 2021 00:15:37 +
From:"Michael Lorenz" 
Message-ID:  <20210424001537.c5c83f...@cvs.netbsd.org>

  | add an ioctl() to get a list of fonts currently available via wsfont

It seems to me it would be useful for that ioctl to copyout()
the fi_numentries field of the struct (if addr != NULL) from
wsdisplayio_listfonts() just before the ENOMEM check (so it is
updated, even if ENOMEM is returned).   (Does it make any sense
for addr to be NULL, or should that be an error?  EINVAL or something.)

Otherwise, there doesn't seem to be any easy way for the user of
the ioctl to know how many fonts were returned (checking which elements
of the array were modified is not "easy") or how big the buffer would
need to be to fetch all of them in the ENOMEM case.

It might also be worth allowing fi_numentries to also be an input
parameter, to indicate where in the set of fonts the fetch should
start, to provide a mechanism to cope should the list size ever grow
so big that it ends up bigger than a single ioctl can handle (that is,
skip the first fi_numentries fonts, and then continue from there).
That would mean a slightly more complex piece of internal code though.

And this last bit is just style, but I'd change the struct wsdisplayio_fontdesc
to be
uint32_t fd_len;
uint16_t fd_height;
unit16_t fd_width;
char fd_name[];
and then make the entries returned be (rounded up) just big enough
for the actual font names.   But that's just me, and not important.

kre



Re: CVS commit: src/sys/arch/powerpc/include/booke

2021-04-21 Thread Rin Okuyama

On 2021/04/18 0:38, Joerg Sonnenberger wrote:

On Sat, Apr 17, 2021 at 01:25:57PM +, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Sat Apr 17 13:25:57 UTC 2021

Modified Files:
src/sys/arch/powerpc/include/booke: vmparam.h

Log Message:
Sync MAXfoo params with oea:

   MAXTSIZ: 512MB -> 128MB
   MAXDSIZ: 3.25GB -> 1GB

There should be no particular reasons for having different values.


Is there an architectural reason for MAXTSIZ to be 128MB? Because we
have seen binaries larger than that.

Ah, I just forgot recent discussion on MAXTSIZ.

There's no limitation due to architecture. I've confirmed binary just
works fine, whose text is larger than 256MB segment on oea. It works
on booke and ibm4xx also.

Maybe we can leave MAXTSIZ undefined for powerpc.

Thanks,
rin


Re: CVS commit: src/sys/arch/powerpc/include/booke

2021-04-17 Thread Joerg Sonnenberger
On Sat, Apr 17, 2021 at 01:25:57PM +, Rin Okuyama wrote:
> Module Name:  src
> Committed By: rin
> Date: Sat Apr 17 13:25:57 UTC 2021
> 
> Modified Files:
>   src/sys/arch/powerpc/include/booke: vmparam.h
> 
> Log Message:
> Sync MAXfoo params with oea:
> 
>   MAXTSIZ: 512MB -> 128MB
>   MAXDSIZ: 3.25GB -> 1GB
> 
> There should be no particular reasons for having different values.

Is there an architectural reason for MAXTSIZ to be 128MB? Because we
have seen binaries larger than that.

Joerg


Re: CVS commit: src/sys/conf

2021-04-05 Thread Simon Burge
"Christos Zoulas" wrote:

> Module Name:  src
> Committed By: christos
> Date: Mon Apr  5 22:52:03 UTC 2021
>
> Modified Files:
>
>   src/sys/conf: Makefile.kern.inc
>
> Log Message:
>
> Don't use /usr/bin/time (it is not portable)

Oops, that bit wasn't meant to sneak in.  Thanks for noticing and
fixing.

Cheers,
Simon.


Re: CVS commit: src/sys/arch

2021-04-02 Thread Simon Burge
"Rin Okuyama" wrote:

> Module Name:  src
> Committed By: rin
> Date: Fri Apr  2 12:11:42 UTC 2021
>
> Log Message:
>
> For ports with __HAVE_LEGACY_INTRCNT, turn intrcnt[] and derived
> variables into u_int, to match with kern/subr_evcnt.c.

Thanks Rin!

Cheers,
Simon.


Re: CVS commit: src/sys/arch/powerpc/oea

2021-04-01 Thread Jason Thorpe
Ugh.  Can we please stop making these hacky one-offs in "shared by all PowerPC 
platforms" code?  This actually points to a deeper problem in the pmap code 
that needs to be addressed correctly.

> On Apr 1, 2021, at 3:02 PM, Michael Lorenz  wrote:
> 
> Module Name:  src
> Committed By: macallan
> Date: Thu Apr  1 22:02:20 UTC 2021
> 
> Modified Files:
>   src/sys/arch/powerpc/oea: ofwoea_machdep.c
> 
> Log Message:
> avoid mapping 0xf000 - my beige G3 DSIs on it
> with this my the machine boots again
> tested on a variety of G4 and G5 models with no problems
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.59 -r1.60 src/sys/arch/powerpc/oea/ofwoea_machdep.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

-- thorpej



Re: CVS commit: src/sys/arch/evbmips/stand/sbmips

2021-03-16 Thread Christos Zoulas
Yes, the binary is mips64-n32. There is no n32 for mips32.
I moved the CPUFLAGS=-n32 assignment to the 64 bit
portion of the Makefile.

christos

> On Mar 16, 2021, at 12:14 AM, matthew green  wrote:
> 
> "Christos Zoulas" writes:
>> Module Name: src
>> Committed By:christos
>> Date:Mon Mar 15 18:13:54 UTC 2021
>> 
>> Modified Files:
>>  src/sys/arch/evbmips/stand/sbmips: Makefile.bootprogs
>> 
>> Log Message:
>> - 32 bit mips uses oabi, don't force it to n32.
>> - compile assembly code with soft-float to kill linker warnings
> 
> this doesn't make sense to me.  it should be compiled
> as if for 64 bit CPU (it is.)  it even has this:
> 
> CFLAGS+=   -mips64
> CFLAGS+=   -Werror ${CWARNFLAGS}
> -CPUFLAGS+= -mabi=n32
> 
> two lines above the removed -mabi=n32.
> 
> what was the real problem here?  (the soft-float part
> seems reasonable.)
> 
> 
> .mrg.



signature.asc
Description: Message signed with OpenPGP


re: CVS commit: src/sys/arch/evbmips/stand/sbmips

2021-03-15 Thread matthew green
"Christos Zoulas" writes:
> Module Name:  src
> Committed By: christos
> Date: Mon Mar 15 18:13:54 UTC 2021
>
> Modified Files:
>   src/sys/arch/evbmips/stand/sbmips: Makefile.bootprogs
>
> Log Message:
> - 32 bit mips uses oabi, don't force it to n32.
> - compile assembly code with soft-float to kill linker warnings

this doesn't make sense to me.  it should be compiled
as if for 64 bit CPU (it is.)  it even has this:

 CFLAGS+=   -mips64
 CFLAGS+=   -Werror ${CWARNFLAGS}
-CPUFLAGS+= -mabi=n32

two lines above the removed -mabi=n32.

what was the real problem here?  (the soft-float part
seems reasonable.)


.mrg.


Re: CVS commit: src/sys/arch/amd64/conf

2021-03-05 Thread Greg Troxel

matthew green  writes:

> could this be done with include and "no foo" statement?
> eg, like sys/arch/sparc/conf/INSTALL does.

Maybe, but I'm not sure it will end up working.  Right now we don't know
if any of the missing things will be trouble, and even if we do move to
include/no I'd like to do that via an intermediate step of a config with
small differences.

Also I think we should also consider extracting lots of things into
includable files, similar to how

  include "dev/usb/usbdevices.config"

is used now in GENERIC.   That will raise interesting cross-arch issues
about value vs kernel memory usage probably.

These include files would allow a simplification of XEN3_DOMU which as I
see it should have ~no drivers but all the rest of the options.


signature.asc
Description: PGP signature


re: CVS commit: src/sys/arch/amd64/conf

2021-03-05 Thread matthew green
"Greg Troxel" writes:
> Module Name:  src
> Committed By: gdt
> Date: Fri Mar  5 20:30:56 UTC 2021
>
> Modified Files:
>   src/sys/arch/amd64/conf: XEN3_DOM0
>
> Log Message:
> XEN3_DOM0: Approach GENERIC
>
> When processed to remove comments, blank lines, normalize whitespace,
> and sort/uniq (one line was previously duplicated), this file is
> identical to the previous version.  It has been reorganized to reduce
> diffs to GENERIC, and many missing lines from GENERIC have been added
> but commented out.

could this be done with include and "no foo" statement?
eg, like sys/arch/sparc/conf/INSTALL does.


.mrg.


Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-23 Thread Jared McNeill

On Mon, 22 Feb 2021, Ryo Shimizu wrote:


I think this condition is not necessary since cpu_idle() is just called from 
idle_loop(),
and ci_intr_depth is always zero at this time.


Ah yes, my mistake! Please feel free to revert this commit as part of 
your proposed change.


Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Jason Thorpe


> On Feb 22, 2021, at 11:49 AM, Ryo Shimizu  wrote:
> 
> Ah, You are quite right!
> idle/# lwp is provided and assigned for each CPU, so curcpu() obtained from
> idle lwp was always the same.
> So, there's no need to move curcpu() to after DISABLE_INTERRUPT.

Please make sure to add a comment explaining why!

-- thorpej



Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Ryo Shimizu


>> In addition, because of the possibility of kpreemption (but aarch64 has =
>no KPREEMPT yet),
>> the acquisition of curcpu() is moved to after DISABLE_INTERRUPT and got =
>the following.
>>
>[snip]
>
>
>>
>> Is this ok?
>>
>
>Looks good - I wonder if the fact that curcpu is an invariant for the
>idlelwp helps here too?

Ah, You are quite right!
idle/# lwp is provided and assigned for each CPU, so curcpu() obtained from
idle lwp was always the same.
So, there's no need to move curcpu() to after DISABLE_INTERRUPT.

Thanks
-- 
ryo shimizu


Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Nick Hudson

On 22/02/2021 10:40, Ryo Shimizu wrote:



Module Name:src
Committed By:   jmcneill
Date:   Sun Feb 21 23:37:10 UTC 2021

Modified Files:
src/sys/arch/aarch64/aarch64: idle_machdep.S

Log Message:
When waking from cpu_idle(), only call dosoftints if ci_intr_depth == 0


To generate a diff of this commit:
cvs rdiff -u -r1.7 -r1.8 src/sys/arch/aarch64/aarch64/idle_machdep.S


I think this condition is not necessary since cpu_idle() is just called from 
idle_loop(),
and ci_intr_depth is always zero at this time.

After thinking about it, I realized that there is no need to even increment 
intr_depth.

   curcpu()->ci_ntr_depth = 1;
   ARM_IRQ_HANDLER();
   curcpu()->ci_ntr_depth = 0;


In addition, because of the possibility of kpreemption (but aarch64 has no 
KPREEMPT yet),
the acquisition of curcpu() is moved to after DISABLE_INTERRUPT and got the 
following.


[snip]




Is this ok?



Looks good - I wonder if the fact that curcpu is an invariant for the
idlelwp helps here too?

Nick


Re: CVS commit: src/sys/arch/aarch64/aarch64

2021-02-22 Thread Ryo Shimizu


>Module Name:   src
>Committed By:  jmcneill
>Date:  Sun Feb 21 23:37:10 UTC 2021
>
>Modified Files:
>   src/sys/arch/aarch64/aarch64: idle_machdep.S
>
>Log Message:
>When waking from cpu_idle(), only call dosoftints if ci_intr_depth == 0
>
>
>To generate a diff of this commit:
>cvs rdiff -u -r1.7 -r1.8 src/sys/arch/aarch64/aarch64/idle_machdep.S

I think this condition is not necessary since cpu_idle() is just called from 
idle_loop(),
and ci_intr_depth is always zero at this time.

After thinking about it, I realized that there is no need to even increment 
intr_depth.

  curcpu()->ci_ntr_depth = 1;
  ARM_IRQ_HANDLER();
  curcpu()->ci_ntr_depth = 0;


In addition, because of the possibility of kpreemption (but aarch64 has no 
KPREEMPT yet),
the acquisition of curcpu() is moved to after DISABLE_INTERRUPT and got the 
following.

cvs -q diff -aU10 -a -p idle_machdep.S
Index: idle_machdep.S
===
RCS file: /src/cvs/cvsroot-netbsd/src/sys/arch/aarch64/aarch64/idle_machdep.S,v
retrieving revision 1.8
diff -a -U 10 -a -p -r1.8 idle_machdep.S
--- idle_machdep.S  21 Feb 2021 23:37:09 -  1.8
+++ idle_machdep.S  22 Feb 2021 10:16:25 -
@@ -67,40 +67,36 @@ ENTRY(cpu_idle)
stp x29, x30, [sp, #TF_X29] /* save x29,x30 */
 #ifdef DDB
add x29, sp, #TF_X29/* link frame for backtrace */
 #endif
 
/* fill the minimum required trapframe */
mov x2, #SPSR_M_EL1H/* what our spsr should be */
str x2, [sp, #TF_SPSR]
adr x0, 1f
str x0, [sp, #TF_PC]/* CLKF_PC refer to tf_pc */
-
-   mrs x1, tpidr_el1   /* get curlwp */
-   ldr x1, [x1, #L_CPU]/* get curcpu */
-   ldr w28, [x1, #CI_INTR_DEPTH]   /* w28 = ci->ci_intr_depth */
-   add w2, w28, #1 /* w2 = intr_depth + 1 */
-
mov x0, sp  /* get pointer to trapframe */
+   mrs x1, tpidr_el1   /* get curlwp */
 
DISABLE_INTERRUPT
-   wfi
+   ldr x1, [x1, #L_CPU]/* get curcpu */
+   mov w2, #1
+   str w2, [x1, #CI_INTR_DEPTH]/* ci->ci_intr_depth = 1 */
 
-   str w2, [x1, #CI_INTR_DEPTH]/* ci->ci_intr_depth++ */
+   wfi
bl  ARM_IRQ_HANDLER /* irqhandler(trapframe) */
 1:
mrs x1, tpidr_el1   /* get curlwp */
ldr x1, [x1, #L_CPU]/* get curcpu */
-   str w28, [x1, #CI_INTR_DEPTH]   /* ci->ci_intr_depth = old */
+   str wzr, [x1, #CI_INTR_DEPTH]   /* ci->ci_intr_depth = 0 */
 
 #if defined(__HAVE_FAST_SOFTINTS) && !defined(__HAVE_PIC_FAST_SOFTINTS)
-   cbnzw28, 1f /* Skip if intr_depth > 0 */
ldr w3, [x1, #CI_SOFTINTS]  /* Get pending softint mask */
/* CPL should be 0 */
ldr w2, [x1, #CI_CPL]   /* Get current priority level */
lsr w3, w3, w2  /* shift mask by cpl */
cbz w3, 1f
bl  _C_LABEL(dosoftints)/* dosoftints() */
 1:
 #endif /* __HAVE_FAST_SOFTINTS && !__HAVE_PIC_FAST_SOFTINTS */
 
ldr x28, [sp, #TF_X28]  /* restore x28 */


Is this ok?
-- 
ryo shimizu


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Christos Zoulas
In article <5912ca9e-b4e7-423d-a45d-f4693d1c9...@zoulas.com>,
Christos Zoulas   wrote:
>-=-=-=-=-=-

Here's the final changes

- Make ALIGNED_POINTER use __alignof(t) instead of sizeof(t). This is more
  correct because it works with non-primitive types and provides the ABI
  alignment for the type the compiler will use.
- Remove all the *_HDR_ALIGNMENT macros and asserts
- Replace POINTER_ALIGNED_P with ACCESSIBLE_POINTER which is identical to
  ALIGNED_POINTER, but returns that the pointer is always aligned if the
  CPU supports unaligned accesses.

Index: sys/mbuf.h
===
RCS file: /cvsroot/src/sys/sys/mbuf.h,v
retrieving revision 1.231
diff -u -u -r1.231 mbuf.h
--- sys/mbuf.h  17 Feb 2021 22:32:04 -  1.231
+++ sys/mbuf.h  17 Feb 2021 23:54:31 -
@@ -843,15 +843,21 @@
m->m_pkthdr.rcvif_index = n->m_pkthdr.rcvif_index;
 }
 
+#define M_GET_ALIGNED_HDR(m, type, linkhdr) \
+m_get_aligned_hdr((m), __alignof(type) - 1, sizeof(type), (linkhdr))
+
 static __inline int
-m_get_aligned_hdr(struct mbuf **m, int align, size_t hlen, bool linkhdr)
+m_get_aligned_hdr(struct mbuf **m, int mask, size_t hlen, bool linkhdr)
 {
-   if (POINTER_ALIGNED_P(mtod(*m, void *), align) == 0) {
-   --align;// Turn into mask
+#ifndef __NO_STRICT_ALIGNMENT
+   if (((uintptr_t)mtod(*m, void *) & mask) != 0)
*m = m_copyup(*m, hlen, 
- linkhdr ? (max_linkhdr + align) & ~align : 0);
-   } else if (__predict_false((size_t)(*m)->m_len < hlen))
+ linkhdr ? (max_linkhdr + mask) & ~mask : 0);
+   else
+#endif
+   if (__predict_false((size_t)(*m)->m_len < hlen))
*m = m_pullup(*m, hlen);
+
return *m == NULL;
 }
 
Index: sys/param.h
===
RCS file: /cvsroot/src/sys/sys/param.h,v
retrieving revision 1.689
diff -u -u -r1.689 param.h
--- sys/param.h 17 Feb 2021 22:32:04 -  1.689
+++ sys/param.h 17 Feb 2021 23:54:31 -
@@ -281,16 +281,24 @@
 #defineALIGN(p)(((uintptr_t)(p) + ALIGNBYTES) & 
~ALIGNBYTES)
 #endif
 #ifndef ALIGNED_POINTER
-#defineALIGNED_POINTER(p,t)uintptr_t)(p)) & (sizeof(t) - 1)) 
== 0)
+#defineALIGNED_POINTER(p,t)uintptr_t)(p)) & (__alignof(t) - 
1)) == 0)
 #endif
 #ifndef ALIGNED_POINTER_LOAD
 #defineALIGNED_POINTER_LOAD(q,p,t) (*(q) = *((const t *)(p)))
 #endif
 
+/*
+ * Return if the pointer p is accessible for type t. For primitive types
+ * this means that the pointer itself can be dereferenced; for structures
+ * and unions this means that any field can be dereferenced. On CPUs
+ * that allow unaligned pointer access, we always return that the pointer
+ * is accessible to prevent unnecessary copies, although this might not be
+ * necessarily faster.
+ */
 #ifdef __NO_STRICT_ALIGNMENT
-#definePOINTER_ALIGNED_P(p, a) 1
+#defineACCESSIBLE_POINTER(p, t)1
 #else
-#definePOINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) 
== 0)
+#defineACCESSIBLE_POINTER(p, t)ALIGNED_POINTER(p, t)
 #endif
 
 /*
Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.42
diff -u -u -r1.42 if_arp.h
--- net/if_arp.h17 Feb 2021 22:32:04 -  1.42
+++ net/if_arp.h17 Feb 2021 23:54:31 -
@@ -72,8 +72,6 @@
uint8_t  ar_tpa[];  /* target protocol address */
 #endif
 };
-#defineARP_HDR_ALIGNMENT   __alignof(struct arphdr)
-__CTASSERT(ARP_HDR_ALIGNMENT == 2);
 
 static __inline uint8_t *
 ar_data(struct arphdr *ap)
Index: net/if_bridge.c
===
RCS file: /cvsroot/src/sys/net/if_bridge.c,v
retrieving revision 1.178
diff -u -u -r1.178 if_bridge.c
--- net/if_bridge.c 14 Feb 2021 20:58:34 -  1.178
+++ net/if_bridge.c 17 Feb 2021 23:54:31 -
@@ -2806,7 +2806,7 @@
if (*mp == NULL)
return -1;
 
-   if (m_get_aligned_hdr(, IP_HDR_ALIGNMENT, sizeof(*ip), true) != 0) {
+   if (M_GET_ALIGNED_HDR(, struct ip, true) != 0) {
/* XXXJRT new stat, please */
ip_statinc(IP_STAT_TOOSMALL);
goto bad;
@@ -2900,7 +2900,7 @@
 * it.  Otherwise, if it is aligned, make sure the entire base
 * IPv6 header is in the first mbuf of the chain.
 */
-   if (m_get_aligned_hdr(, IP6_HDR_ALIGNMENT, sizeof(*ip6), true) != 0) {
+   if (M_GET_ALIGNED_HDR(, struct ip6_hdr, true) != 0) {
struct ifnet *inifp = m_get_rcvif_NOMPSAFE(m);
/* XXXJRT new stat, please */
ip6_statinc(IP6_STAT_TOOSMALL);
Index: netinet/if_arp.c
===
RCS file: 

Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Christos Zoulas


> On Feb 17, 2021, at 4:20 PM, Valery Ushakov  wrote:
> 
> On Wed, Feb 17, 2021 at 17:49:15 -, Christos Zoulas wrote:
> 
> This incrementally improves wrong things b/c you still have the "is
> aligned" and "ought to be aligned" conflated in one.

The incremental patch improves things and does not make things worse.
I will address the __NO_STRICT_ALIGNMENT issue separately. I am planning
to propose something in tech-kern once I have it working to my liking. This
patch helps me because it aligns the semantics of the two macros better.

christos


signature.asc
Description: Message signed with OpenPGP


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Valery Ushakov
On Wed, Feb 17, 2021 at 17:49:15 -, Christos Zoulas wrote:

> In article ,
> Valery Ushakov   wrote:
> 
> >But to get back to my main point, PLEASE, can we stop making random
> >aimless changes without prior discussion?
> 
> Here's the change I'd like to make:
> - pass the alignment instead of the mask (as Roy asked and to match the
>   other macro)
> - use alignof to determine that alignment and CTASSERT what we expect
> - remove unused macros
> 
> This incrementally improves things.
[...]
> diff -u -p -u -r1.688 param.h
> --- sys/param.h   15 Feb 2021 19:46:53 -  1.688
> +++ sys/param.h   17 Feb 2021 17:45:55 -
> @@ -290,7 +290,7 @@
>  #ifdef __NO_STRICT_ALIGNMENT
>  #define  POINTER_ALIGNED_P(p, a) 1
>  #else
> -#define  POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & (a)) == 0)
> +#define  POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) 
> == 0)
>  #endif
>  
>  /*

This incrementally improves wrong things b/c you still have the "is
aligned" and "ought to be aligned" conflated in one.


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-17 Thread Christos Zoulas
In article ,
Valery Ushakov   wrote:

>But to get back to my main point, PLEASE, can we stop making random
>aimless changes without prior discussion?

Here's the change I'd like to make:
- pass the alignment instead of the mask (as Roy asked and to match the
  other macro)
- use alignof to determine that alignment and CTASSERT what we expect
- remove unused macros

This incrementally improves things.

christos

Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.41
diff -u -p -u -r1.41 if_arp.h
--- net/if_arp.h16 Feb 2021 10:20:56 -  1.41
+++ net/if_arp.h17 Feb 2021 17:45:55 -
@@ -72,7 +72,8 @@ structarphdr {
uint8_t  ar_tpa[];  /* target protocol address */
 #endif
 };
-#defineARP_HDR_ALIGNMENT   1
+#defineARP_HDR_ALIGNMENT   __alignof(struct arphdr)
+__CTASSERT(ARP_HDR_ALIGNMENT == 2);
 
 static __inline uint8_t *
 ar_data(struct arphdr *ap)
Index: netinet/icmp_private.h
===
RCS file: /cvsroot/src/sys/netinet/icmp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 icmp_private.h
--- netinet/icmp_private.h  14 Feb 2021 20:58:35 -  1.4
+++ netinet/icmp_private.h  17 Feb 2021 17:45:55 -
@@ -44,7 +44,6 @@ extern percpu_t *icmpstat_percpu;
 
 #defineICMP_STATINC(x) _NET_STATINC(icmpstat_percpu, x)
 
-#defineICMP_HDR_ALIGNMENT  3
 #endif /* _KERNEL_ */
 
 #endif /* !_NETINET_ICMP_PRIVATE_H_ */
Index: netinet/igmp_var.h
===
RCS file: /cvsroot/src/sys/netinet/igmp_var.h,v
retrieving revision 1.26
diff -u -p -u -r1.26 igmp_var.h
--- netinet/igmp_var.h  14 Feb 2021 20:58:35 -  1.26
+++ netinet/igmp_var.h  17 Feb 2021 17:45:55 -
@@ -105,8 +105,6 @@
  */
 #defineIGMP_RANDOM_DELAY(X)(cprng_fast32() % (X) + 1)
 
-#defineIGMP_HDR_ALIGNMENT  3
-
 void   igmp_init(void);
 void   igmp_input(struct mbuf *, int, int);
 intigmp_joingroup(struct in_multi *);
Index: netinet/ip_private.h
===
RCS file: /cvsroot/src/sys/netinet/ip_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 ip_private.h
--- netinet/ip_private.h14 Feb 2021 20:58:35 -  1.4
+++ netinet/ip_private.h17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *ipstat_percpu;
 #defineIP_STATINC(x)   _NET_STATINC(ipstat_percpu, x)
 #defineIP_STATDEC(x)   _NET_STATDEC(ipstat_percpu, x)
 
-#defineIP_HDR_ALIGNMENT3
+#defineIP_HDR_ALIGNMENT__alignof(struct ip)
+__CTASSERT(IP_HDR_ALIGNMENT == 4);
 #endif /* _KERNEL */
 
 #endif /* !_NETINET_IP_PRIVATE_H_ */
Index: netinet/tcp_private.h
===
RCS file: /cvsroot/src/sys/netinet/tcp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 tcp_private.h
--- netinet/tcp_private.h   14 Feb 2021 20:58:35 -  1.4
+++ netinet/tcp_private.h   17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *tcpstat_percpu;
 #defineTCP_STATINC(x)  _NET_STATINC(tcpstat_percpu, x)
 #defineTCP_STATADD(x, v)   _NET_STATADD(tcpstat_percpu, x, v)
 
-#defineTCP_HDR_ALIGNMENT   3
+#defineTCP_HDR_ALIGNMENT   __alignof(struct tcphdr)
+__CTASSERT(TCP_HDR_ALIGNMENT == 4);
 #endif /* _KERNEL */
 
 #endif /* !_NETINET_TCP_PRIVATE_H_ */
Index: netinet/udp_private.h
===
RCS file: /cvsroot/src/sys/netinet/udp_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 udp_private.h
--- netinet/udp_private.h   14 Feb 2021 20:58:35 -  1.4
+++ netinet/udp_private.h   17 Feb 2021 17:45:55 -
@@ -39,7 +39,8 @@ externpercpu_t *udpstat_percpu;
 
 #defineUDP_STATINC(x)  _NET_STATINC(udpstat_percpu, x)
 
-#defineUDP_HDR_ALIGNMENT   3
+#defineUDP_HDR_ALIGNMENT   __alignof(struct udphdr)
+__CTASSERT(UDP_HDR_ALIGNMENT == 2);
 #endif /* _KERNEL */
 
 #endif /* !_NETINET_UDP_PRIVATE_H_ */
Index: netinet6/ip6_private.h
===
RCS file: /cvsroot/src/sys/netinet6/ip6_private.h,v
retrieving revision 1.4
diff -u -p -u -r1.4 ip6_private.h
--- netinet6/ip6_private.h  14 Feb 2021 20:58:35 -  1.4
+++ netinet6/ip6_private.h  17 Feb 2021 17:45:55 -
@@ -43,7 +43,8 @@ externpercpu_t *ip6stat_percpu;
 #defineIP6_STATINC(x)  _NET_STATINC(ip6stat_percpu, x)
 #defineIP6_STATDEC(x)  _NET_STATDEC(ip6stat_percpu, x)
 
-#defineIP6_HDR_ALIGNMENT   3
+#defineIP6_HDR_ALIGNMENT   __alignof(struct ip6_hdr)
+__CTASSERT(IP6_HDR_ALIGNMENT == 4);

Re: CVS commit: src/sys

2021-02-17 Thread J. Hannken-Illjes
Jared,

please test if the attached diff is sufficient.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig



uvm_swap.c.diff
Description: Binary data

> On 17. Feb 2021, at 13:07, Jared McNeill  wrote:
> 
> I noticed this on reboot since this change:
> 
> [ 3636.2891122] turning off swap...stopping swap on /swap failed with error 16
> [ 3636.2991109] stopping swap on /swap2 failed with error 16
> 
> Can you have a look?
> 
> Thanks!
> Jared
> 
> 
> On Tue, 16 Feb 2021, Juergen Hannken-Illjes wrote:
> 
>> Module Name: src
>> Committed By:hannken
>> Date:Tue Feb 16 09:56:32 UTC 2021
>> 
>> Modified Files:
>>  src/sys/kern: vfs_mount.c
>>  src/sys/uvm: uvm_swap.c
>> 
>> Log Message:
>> Reorganize uvm_swap_shutdown() a bit, make sure the vnode gets
>> locked and referenced across the call to swap_off() and finally
>> use it from vfs_unmountall1() to remove swap after unmounting
>> the last file system.
>> 
>> Adresses PR kern/54969 (Disk cache is no longer flushed on shutdown)
>> 
>> 
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.85 -r1.86 src/sys/kern/vfs_mount.c
>> cvs rdiff -u -r1.200 -r1.201 src/sys/uvm/uvm_swap.c
>> 
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>> 
>> 



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys

2021-02-17 Thread Jared McNeill

I noticed this on reboot since this change:

 [ 3636.2891122] turning off swap...stopping swap on /swap failed with error 16
 [ 3636.2991109] stopping swap on /swap2 failed with error 16

Can you have a look?

Thanks!
Jared


On Tue, 16 Feb 2021, Juergen Hannken-Illjes wrote:


Module Name:src
Committed By:   hannken
Date:   Tue Feb 16 09:56:32 UTC 2021

Modified Files:
src/sys/kern: vfs_mount.c
src/sys/uvm: uvm_swap.c

Log Message:
Reorganize uvm_swap_shutdown() a bit, make sure the vnode gets
locked and referenced across the call to swap_off() and finally
use it from vfs_unmountall1() to remove swap after unmounting
the last file system.

Adresses PR kern/54969 (Disk cache is no longer flushed on shutdown)


To generate a diff of this commit:
cvs rdiff -u -r1.85 -r1.86 src/sys/kern/vfs_mount.c
cvs rdiff -u -r1.200 -r1.201 src/sys/uvm/uvm_swap.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.




Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 18:51:43 -0500, Christos Zoulas wrote:

> On Feb 17,  2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote:
> -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
> 
> | Well, it was you who did the definion of ALIGNED_POINTER centralized
> | and overridable :)
> | 
> |   revision 1.400
> |   date: 2012-01-25 00:03:36 +0400;  author: christos;  state: Exp;  lines: 
> +26 -1;
> |   Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
> |   and avoid definining them in 10 different places if not needed.
> 
> If you look a bit deeper into that change, it merged many MD copies
> of this macro into one, and I needed the override for the archs that
> were different.
> 
> | ALIGNED_POINTER is overriden on x86 to be always true.  Surprisingly
> | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
> | That is most likely an oversight, but that will probably require some
> | cvs archaeology to confirm.  Some uses of ALIGNED_POINTER are inside
> | an __NO_STRICT_ALIGNMENT #ifdef.
> 
> This is a mess as you can see. The problem is that in each case we
> need to determine if this macro is used to test alignment to determine
> access restrictions on certain architectures (most cases), or it
> is done for performance/protocol requirements.
> 
> I hope that nothing falls into the second category and then we can
> use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
> and __alignof() which my guess is that it did not exist at the time
> the macro was invented, and thus it used sizeof() and limited it to
> integral types.
> 
> | We don't even seem to be sure about its semantics, as far as I can
> | tell (see bus space comments in my mail).
> | 
> | That's even more of a reason to stop doing aimless random changes
> | without getting some kind of understanding first.  The last thing we
> | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
> | different semantics both of which are counter-intuitive to begin with
> | (and riastradh@ even had to add a verbose comment for that).
> 
> This change was not aimless. I wanted to remove the multiple copies of
> the m_copyup/m_pullup code into one function. To do that I needed the
> alignment as a value, not a predicate macro (that was again copied in
> ~10 places). When I tried to centralize it, I wanted to do the minimal
> change so I would not screw it up (and I did end up screwing it up).
> 
> This is a good opportunity now to clean this up even more and provide
> sane alignment macros.

We should start by at least separating the concerns.  The test for
"aligned at power of two boundary" and the actual intent/purpose of
that test ("stuff needs to be aligned for us to safely do $FOO").
Then we can test the alignment check without jumping through
ridiculous hoops.  That should have been done earlier for the
ALIGNED_POINTER change, but yeah, I can see how in the heat of the
moment that change was just "preserve the status quo" and that's
absolutely fine.  But doing the same thing now with the alignment test
and the purpose of that alignment test conflacted once again under a
different but confusignly similar name is negligent (if anything we
will run out of half way decent names for the just-the-alignment-test
macro, b/c all of them will be loaded with some additional accidental
semantic).


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Wed, Feb 17, 2021 at 00:51:03 +0100, Roland Illig wrote:

> 17.02.2021 00:25:10 Valery Ushakov :
> > On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:
> >> Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
> >> test is for.
> >>
> >> I intentionally placed it between  (which defines that
> >> macro on x86 and some other platforms) and  (which uses the
> >> macro to switch between the boring "everything is correctly aligned" and
> >> the more interesting formula suggested here.
> >
> > This is wrong on so many levels.  What is even the point of a test
> > that doesn't test the thing as it is actually defined and used in the
> > code?
> 
> The point of the test is to verify that the "complicated" formula
> produces correct results.  That's what several commits tried to
> fix.  If this test had been there from the beginning, none of the
> wrong formulas would have passed it.  That's the whole point.
> 
> The point of the test was intentionally not to test the actual
> behavior on each platform but to test the same formula, independent
> from the platform, and to do this, I somehow needed access to that
> formula.  Testing the actually used formula per platform could be
> added as another test.  I just wanted to avoid the obviously wrong
> formulas to go unnoticed in the code.  That's the point of the test,
> and that's exactly what it achieves.  Therefore I don't see anything
> wrong with it.

The very fact that you need to undefine an unspecified macro at an
unspecified time to get to the "formula" points to a problem.  We
shouldn't be pretending that it's not, and provide the false decorum
of "oh, but it's covered with a test, so it's ok".

-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Christos Zoulas
On Feb 17,  2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote:
-- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

| Well, it was you who did the definion of ALIGNED_POINTER centralized
| and overridable :)
| 
|   revision 1.400
|   date: 2012-01-25 00:03:36 +0400;  author: christos;  state: Exp;  lines: 
+26 -1;
|   Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
|   and avoid definining them in 10 different places if not needed.

If you look a bit deeper into that change, it merged many MD copies
of this macro into one, and I needed the override for the archs that
were different.

| ALIGNED_POINTER is overriden on x86 to be always true.  Surprisingly
| it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
| That is most likely an oversight, but that will probably require some
| cvs archaeology to confirm.  Some uses of ALIGNED_POINTER are inside
| an __NO_STRICT_ALIGNMENT #ifdef.

This is a mess as you can see. The problem is that in each case we
need to determine if this macro is used to test alignment to determine
access restrictions on certain architectures (most cases), or it
is done for performance/protocol requirements.

I hope that nothing falls into the second category and then we can
use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
and __alignof() which my guess is that it did not exist at the time
the macro was invented, and thus it used sizeof() and limited it to
integral types.

| We don't even seem to be sure about its semantics, as far as I can
| tell (see bus space comments in my mail).
| 
| That's even more of a reason to stop doing aimless random changes
| without getting some kind of understanding first.  The last thing we
| need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
| different semantics both of which are counter-intuitive to begin with
| (and riastradh@ even had to add a verbose comment for that).

This change was not aimless. I wanted to remove the multiple copies of
the m_copyup/m_pullup code into one function. To do that I needed the
alignment as a value, not a predicate macro (that was again copied in
~10 places). When I tried to centralize it, I wanted to do the minimal
change so I would not screw it up (and I did end up screwing it up).

This is a good opportunity now to clean this up even more and provide
sane alignment macros.

christos


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig
17.02.2021 00:25:10 Valery Ushakov :
> On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:
>> Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
>> test is for.
>>
>> I intentionally placed it between  (which defines that
>> macro on x86 and some other platforms) and  (which uses the
>> macro to switch between the boring "everything is correctly aligned" and
>> the more interesting formula suggested here.
>
> This is wrong on so many levels.  What is even the point of a test
> that doesn't test the thing as it is actually defined and used in the
> code?

The point of the test is to verify that the "complicated" formula produces 
correct results.  That's what several commits tried to fix.  If this test had 
been there from the beginning, none of the wrong formulas would have passed it. 
 That's the whole point.

The point of the test was intentionally not to test the actual behavior on each 
platform but to test the same formula, independent from the platform, and to do 
this, I somehow needed access to that formula.  Testing the actually used 
formula per platform could be added as another test.  I just wanted to avoid 
the obviously wrong formulas to go unnoticed in the code.  That's the point of 
the test, and that's exactly what it achieves.  Therefore I don't see anything 
wrong with it.

Roland


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig

On 16.02.2021 23:32, Jason Thorpe wrote:



On Feb 16, 2021, at 1:56 PM, Roland Illig  wrote:

A downside of this test is that the macro from  gets only
evaluated at compile time of the test.  The test therefore cannot cover
future updates to  without being reinstalled itself.  But
at least for the release builds, it ensures a correct definition.


You can make this get evaluated at run-time by passing in a non-constant 
argument.


I wanted to make the test even more flexible: react to the then-current
installed version of the macro in .  To do that, the test
would need to be compiled at runtime, requiring a C compiler on the
target machine, and I'm not sure that's a valid assumption.

And maybe that's something that shouldn't be done at all, updating the
system header without the corresponding test suite.  So it's probably
fine as it is now, notwithstanding the discussion about whether the
macro is needed at all in this prominent place.

It surprised me though that neither Google nor GitHub found the macro
name POINTER_ALIGNED_P anywhere outside the NetBSD repository.  I would
have expected this name to be already used by some projects, given that
it is so straight-forward.

Roland


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 17:53:09 -0500, Christos Zoulas wrote:

> In this case "type" is a struct and we have __alignof() to handle
> it, but does this give the right answer?
> 
> Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT and
> can be overriden (the opposite goes for POINTER_ALIGNED_P) I am all
> for having one macro, but how can we satisfy all the different
> semantics?

Well, it was you who did the definion of ALIGNED_POINTER centralized
and overridable :)

  revision 1.400
  date: 2012-01-25 00:03:36 +0400;  author: christos;  state: Exp;  lines: +26 
-1;
  Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
  and avoid definining them in 10 different places if not needed.

ALIGNED_POINTER is overriden on x86 to be always true.  Surprisingly
it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
That is most likely an oversight, but that will probably require some
cvs archaeology to confirm.  Some uses of ALIGNED_POINTER are inside
an __NO_STRICT_ALIGNMENT #ifdef.

We don't even seem to be sure about its semantics, as far as I can
tell (see bus space comments in my mail).

That's even more of a reason to stop doing aimless random changes
without getting some kind of understanding first.  The last thing we
need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
different semantics both of which are counter-intuitive to begin with
(and riastradh@ even had to add a verbose comment for that).


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:

> On 16.02.2021 23:40, Valery Ushakov wrote:
> > On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> > 
> > > On 16.02.2021 19:46, Roy Marples wrote:
> > > > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > > > want rather than the bitwise test we supply, like so:
> > > > 
> > > > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
> > > 
> > > To make sure that this macro doesn't get broken again, I have written
> > > the attached tests.  I will commit them after making sure I got the
> > > changes to distrib/sets/lists/tests/mi correct.  All the rest I already
> > > tested successfully.
> > 
> > I don't see any proposal to change the meaning of this macro to
> > actually require the alignment even for arches without strict
> > alignment.  Does the attached test really passes on e.g. x86 where the
> > macro is always true?  E.g. this one:
> > 
> > > + if (POINTER_ALIGNED_P(p + 1, 2))
> > > + atf_tc_fail("p + 1 must not be aligned to 2");
> 
> Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
> test is for.
> 
> I intentionally placed it between  (which defines that
> macro on x86 and some other platforms) and  (which uses the
> macro to switch between the boring "everything is correctly aligned" and
> the more interesting formula suggested here.

This is wrong on so many levels.  What is even the point of a test
that doesn't test the thing as it is actually defined and used in the
code?

-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig

On 16.02.2021 23:40, Valery Ushakov wrote:

On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:


On 16.02.2021 19:46, Roy Marples wrote:

I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:

#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))


To make sure that this macro doesn't get broken again, I have written
the attached tests.  I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct.  All the rest I already
tested successfully.


I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment.  Does the attached test really passes on e.g. x86 where the
macro is always true?  E.g. this one:


+   if (POINTER_ALIGNED_P(p + 1, 2))
+   atf_tc_fail("p + 1 must not be aligned to 2");


Yes, it does.  That's what the "#undef __NO_STRICT_ALIGNMENT" in the
test is for.

I intentionally placed it between  (which defines that
macro on x86 and some other platforms) and  (which uses the
macro to switch between the boring "everything is correctly aligned" and
the more interesting formula suggested here.


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Christos Zoulas
In this case "type" is a struct and we have __alignof() to handle it, but does 
this give the
right answer? Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT
and can be overriden (the opposite goes for POINTER_ALIGNED_P)  I am all for 
having one
macro, but how can we satisfy all the different semantics?

christos


signature.asc
Description: Message signed with OpenPGP


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Valery Ushakov
On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:

> On 16.02.2021 19:46, Roy Marples wrote:
> > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > want rather than the bitwise test we supply, like so:
> > 
> > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
> 
> That's a good definition, clear, simple, obvious, without any surprises.

Now, replace the value "a" with the type "t" and change (a) to
sizeof(t) and you will get ALIGNED_POINTER from just above.


> To make sure that this macro doesn't get broken again, I have written
> the attached tests.  I will commit them after making sure I got the
> changes to distrib/sets/lists/tests/mi correct.  All the rest I already
> tested successfully.

I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment.  Does the attached test really passes on e.g. x86 where the
macro is always true?  E.g. this one:

> + if (POINTER_ALIGNED_P(p + 1, 2))
> + atf_tc_fail("p + 1 must not be aligned to 2");


> Is my assumption correct that on each platform supported by NetBSD, a
> variable of type double gets aligned to a multiple of 8, even on the
> stack?

No.  E.g. on sh3 doubles are 8 bytes but are 4 bytes aligned.  I'm
almost sure some other ABI has that kind less strict alignment too,
but I don't remember.


> I wanted to keep the test as simple as possible, therefore I
> didn't want to call malloc just to get an aligned pointer.

You can use an ordinary array that is large enough and manually
find/construct a suitably aligned pointer value inside that array.


BUT, can we, PLEASE, stop making random breaking changes and at least
articulate first what is that that we want here?

POINTER_ALIGNED_P should have never been brought into existence in the
first place.

ALIGNED_POINTER seems to be used to define BUS_SPACE_ALIGNED_POINTER -
the real fun here is sys/arch/x86/include/bus_defs.h that defines
BUS_SPACE_ALIGNED_POINTER to be "really" aligned for BUS_SPACE_DEBUG,
which seems kinda suspicious to me.  BTW, can we really conclude from
the CPU's alignment requirements to some bus alignment requirements?

But to get back to my main point, PLEASE, can we stop making random
aimless changes without prior discussion?

To quote Vonnegut, "If I had of been a observer, I would of said we
was comical."


-uwe


Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Jason Thorpe


> On Feb 16, 2021, at 1:56 PM, Roland Illig  wrote:
> 
> A downside of this test is that the macro from  gets only
> evaluated at compile time of the test.  The test therefore cannot cover
> future updates to  without being reinstalled itself.  But
> at least for the release builds, it ensures a correct definition.

You can make this get evaluated at run-time by passing in a non-constant 
argument.

-- thorpej



Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

2021-02-16 Thread Roland Illig

On 16.02.2021 19:46, Roy Marples wrote:

I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:

#define    POINTER_ALIGNED_P(p, a)    (((uintptr_t)(p) & ((a) - 1))


That's a good definition, clear, simple, obvious, without any surprises.

To make sure that this macro doesn't get broken again, I have written
the attached tests.  I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct.  All the rest I already
tested successfully.

Is my assumption correct that on each platform supported by NetBSD, a
variable of type double gets aligned to a multiple of 8, even on the
stack?  I wanted to keep the test as simple as possible, therefore I
didn't want to call malloc just to get an aligned pointer.

A downside of this test is that the macro from  gets only
evaluated at compile time of the test.  The test therefore cannot cover
future updates to  without being reinstalled itself.  But
at least for the release builds, it ensures a correct definition.

Roland
Index: distrib/sets/lists/tests/mi
===
RCS file: /cvsroot/src/distrib/sets/lists/tests/mi,v
retrieving revision 1.1019
diff -u -p -r1.1019 mi
--- distrib/sets/lists/tests/mi 16 Feb 2021 09:46:24 -  1.1019
+++ distrib/sets/lists/tests/mi 16 Feb 2021 21:45:40 -
@@ -4396,6 +4396,10 @@
 ./usr/tests/sys/rc/h_args  tests-sys-tests 
compattestfile,atf
 ./usr/tests/sys/rc/h_simpletests-sys-tests 
compattestfile,atf
 ./usr/tests/sys/rc/t_rc_d_cli  tests-sys-tests 
compattestfile,atf
+./usr/tests/sys/systests-sys-tests 
compattestfile,atf
+./usr/tests/sys/sys/Atffiletests-sys-tests 
compattestfile,atf
+./usr/tests/sys/sys/Kyuafile   tests-sys-tests 
compattestfile,atf,kyua
+./usr/tests/sys/sys/t_pointer_aligned_ptests-sys-tests 
compattestfile,atf
 ./usr/tests/sys/x86tests-sys-tests 
compattestfile,atf
 ./usr/tests/syscalltests-obsolete  
obsolete
 ./usr/tests/syscall/Atffiletests-obsolete  
obsolete
Index: tests/sys/Makefile
===
RCS file: /cvsroot/src/tests/sys/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- tests/sys/Makefile  15 Oct 2020 17:44:44 -  1.5
+++ tests/sys/Makefile  16 Feb 2021 21:45:40 -
@@ -10,6 +10,7 @@ TESTS_SUBDIRS+=   netatalk
 TESTS_SUBDIRS+=netinet
 TESTS_SUBDIRS+=netinet6
 TESTS_SUBDIRS+=rc
+TESTS_SUBDIRS+=sys
 .if ${MACHINE} == amd64 || ${MACHINE} == i386
 TESTS_SUBDIRS+=x86
 .endif
Index: tests/sys/sys/Makefile
===
RCS file: tests/sys/sys/Makefile
diff -N tests/sys/sys/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ tests/sys/sys/Makefile  16 Feb 2021 21:45:40 -
@@ -0,0 +1,11 @@
+# $NetBSD$
+
+TESTSDIR=  ${TESTSBASE}/sys/sys
+WARNS= 6
+
+TESTS_C=   t_pointer_aligned_p
+
+.PATH: ${NETBSDSRCDIR}/sys
+CPPFLAGS+= -I${NETBSDSRCDIR}/sys
+
+.include 
Index: tests/sys/sys/t_pointer_aligned_p.c
===
RCS file: tests/sys/sys/t_pointer_aligned_p.c
diff -N tests/sys/sys/t_pointer_aligned_p.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ tests/sys/sys/t_pointer_aligned_p.c 16 Feb 2021 21:45:40 -
@@ -0,0 +1,77 @@
+/* $NetBSD$*/
+
+/*-
+ * Copyright (c) 2021 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND 

Re: CVS commit: src/sys

2021-02-16 Thread Christos Zoulas
Yes, I agree. I am going to change that.

christos

> On Feb 16, 2021, at 1:46 PM, Roy Marples  wrote:
> 
> Hi Christos
> 
> On 14/02/2021 20:58, Christos Zoulas wrote:
>> Module Name: src
>> Committed By:christos
>> Date:Sun Feb 14 20:58:35 UTC 2021
>> Modified Files:
>>  src/sys/net: if_arp.h if_bridge.c
>>  src/sys/netinet: icmp_private.h if_arp.c igmp_var.h in_l2tp.c ip_flow.c
>>  ip_input.c ip_private.h tcp_input.c tcp_private.h udp_private.h
>>  udp_usrreq.c
>>  src/sys/netinet6: icmp6.c in6_l2tp.c ip6_flow.c ip6_input.c
>>  ip6_private.h udp6_usrreq.c
>>  src/sys/sys: mbuf.h param.h
>> Log Message:
>> - centralize header align and pullup into a single inline function
>> - use a single macro to align pointers and expose the alignment, instead
>>   of hard-coding 3 in 1/2 the macros.
>> - fix an issue in the ipv6 lt2p where it was aligning for ipv4 and pulling
>>   for ipv6.
> 
> -#ifdef __NO_STRICT_ALIGNMENT
> -#define  IP_HDR_ALIGNED_P(ip)1
> -#else
> -#define  IP_HDR_ALIGNED_P(ip)vaddr_t) (ip)) & 3) == 0)
> -#endif
> +#define  IP_HDR_ALIGNMENT3
> #endif /* _KERNEL */
> 
> While this is a like for like change, I feel that the meaning of 
> IP_HDR_ALIGNMENT is no longer clear as 3 without context makes no sense at 
> all.
> We know it should be aligned to 4 bytes.
> 
> I suggest we change POINTER_ALIGNED_P to accept the alignment value we want 
> rather than the bitwise test we supply, like so:
> 
> #define   POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) 
> == 0)
> 
> Roy



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys

2021-02-16 Thread Roy Marples

Hi Christos

On 14/02/2021 20:58, Christos Zoulas wrote:

Module Name:src
Committed By:   christos
Date:   Sun Feb 14 20:58:35 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_bridge.c
src/sys/netinet: icmp_private.h if_arp.c igmp_var.h in_l2tp.c ip_flow.c
ip_input.c ip_private.h tcp_input.c tcp_private.h udp_private.h
udp_usrreq.c
src/sys/netinet6: icmp6.c in6_l2tp.c ip6_flow.c ip6_input.c
ip6_private.h udp6_usrreq.c
src/sys/sys: mbuf.h param.h

Log Message:
- centralize header align and pullup into a single inline function
- use a single macro to align pointers and expose the alignment, instead
   of hard-coding 3 in 1/2 the macros.
- fix an issue in the ipv6 lt2p where it was aligning for ipv4 and pulling
   for ipv6.


-#ifdef __NO_STRICT_ALIGNMENT
-#defineIP_HDR_ALIGNED_P(ip)1
-#else
-#defineIP_HDR_ALIGNED_P(ip)vaddr_t) (ip)) & 3) == 0)
-#endif
+#defineIP_HDR_ALIGNMENT3
 #endif /* _KERNEL */

While this is a like for like change, I feel that the meaning of 
IP_HDR_ALIGNMENT is no longer clear as 3 without context makes no sense at all.

We know it should be aligned to 4 bytes.

I suggest we change POINTER_ALIGNED_P to accept the alignment value we want 
rather than the bitwise test we supply, like so:


#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) == 0)

Roy


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Martin Husemann
On Tue, Feb 16, 2021 at 09:29:15AM +, Roy Marples wrote:
> In my testing on aarch64 and octeon (both of which I think are strict
> alignment) neither need pullups nor copyups as the mbuf already has enough
> and arphrd is aligned correctly already.

Ah, we asserted too much alignment - indeed, this variant works and I commited
it after testing on 32bit arm and sparc64.

Martin


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Roy Marples

On 16/02/2021 09:20, Martin Husemann wrote:

On Tue, Feb 16, 2021 at 08:26:40AM +, Roy Marples wrote:

Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment?


The KASSERT a few lines below triggerd, we need to be consistent.


For the purposes of using just the header we define I'm pretty sure we can
use 2 byte alignment and set ARP_HDR_ALIGNMENT to 1.


I can test (I have an alignment critical machine with non-ETHER_ALIGN'ing
network driver). Send me a patch, I lost track in the ongoing overhaul.


ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well.


Not sure I understand what you mean here.



Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.40
diff -u -p -r1.40 if_arp.h
--- net/if_arp.h14 Feb 2021 20:58:34 -  1.40
+++ net/if_arp.h16 Feb 2021 09:26:23 -
@@ -72,7 +72,7 @@ structarphdr {
uint8_t  ar_tpa[];  /* target protocol address */
 #endif
 };
-#defineARP_HDR_ALIGNMENT   3
+#defineARP_HDR_ALIGNMENT   1

 static __inline uint8_t *
 ar_data(struct arphdr *ap)
Index: netinet/if_arp.c
===
RCS file: /cvsroot/src/sys/netinet/if_arp.c,v
retrieving revision 1.305
diff -u -p -r1.305 if_arp.c
--- netinet/if_arp.c16 Feb 2021 05:44:13 -  1.305
+++ netinet/if_arp.c16 Feb 2021 09:26:23 -
@@ -133,12 +133,6 @@ __KERNEL_RCSID(0, "$NetBSD: if_arp.c,v 1
  */
 #define ETHERTYPE_IPTRAILERS ETHERTYPE_TRAIL

-#ifdef __NO_STRICT_ALIGNMENT
-#defineARP_HDR_ALIGNED_P(ar)   1
-#else
-#defineARP_HDR_ALIGNED_P(ar)   vaddr_t) (ar)) & 1) == 0)
-#endif
-
 /* timers */
 static int arp_reachable = REACHABLE_TIME;
 static int arp_retrans = RETRANS_TIMER;


In my testing on aarch64 and octeon (both of which I think are strict alignment) 
neither need pullups nor copyups as the mbuf already has enough and arphrd is 
aligned correctly already.


Roy


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Martin Husemann
On Tue, Feb 16, 2021 at 08:26:40AM +, Roy Marples wrote:
> Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment?

The KASSERT a few lines below triggerd, we need to be consistent.

> For the purposes of using just the header we define I'm pretty sure we can
> use 2 byte alignment and set ARP_HDR_ALIGNMENT to 1.

I can test (I have an alignment critical machine with non-ETHER_ALIGN'ing
network driver). Send me a patch, I lost track in the ongoing overhaul.

> ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well.

Not sure I understand what you mean here.

Martin


Re: CVS commit: src/sys/netinet

2021-02-16 Thread Roy Marples

On 16/02/2021 05:44, Martin Husemann wrote:

Module Name:src
Committed By:   martin
Date:   Tue Feb 16 05:44:14 UTC 2021

Modified Files:
src/sys/netinet: if_arp.c

Log Message:
Undo previous backout: alignment is needed here.
The reason for the previous backout was a misunderstanding (POINTER_ALIGNED_P
was broken, but the assertion fired even after it got fixed).


Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment?
For the purposes of using just the header we define I'm pretty sure we can use 2 
byte alignment and set ARP_HDR_ALIGNMENT to 1.


ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well.

Roy


Re: CVS commit: src/sys

2021-02-15 Thread David Young
On Sun, Feb 14, 2021 at 07:46:38PM +, Roy Marples wrote:
> On 13/02/2021 21:34, David Young wrote:
> > On Tue, Feb 09, 2021 at 07:02:32AM +, Roy Marples wrote:
> > > Hi David
> > > 
> > > On 03/02/2021 21:45, David Young wrote:
> > > > 
> > > > This change looks a little hasty to me.
> > > > 
> > > > It looks to me like some of these structs were __packed so that
> > > > they could be read/written directly from/to any offset in a packet
> > > > chain using mtod(), which does not pay any mind to the alignment
> > > > of `*t`:
> > > > 
> > > > #define mtod(m, t)  ((t)((m)->m_data))
> > > > 
> > > > I see gre_h is accessed in that way, just for one example.
> > > 
> > > Looking at the other places where this is handled, does this patch to 
> > > gre_h
> > > address your concerns?
> > > I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and 
> > > it
> > > passes the KASSERT.
> > 
> > It is possible to simplify your patch a lot.  The GRE header is only 4
> > bytes long.  On the receive side, just perform the m_pullup like the
> > old code did and then memcpy to a `struct gre_h` on the stack.  On the
> > send side, construct the header on the stack and then memcpy it into the
> > mbuf.
> > 
> > The same general approach, of copying headers between mbufs the stack,
> > is probably plenty fast for virtually any size of header used in the
> > network stack.

Thank you.  I know you probably did not expect to spend so much time on
this.  I appreciate your efforts.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981


Re: CVS commit: src/sys

2021-02-14 Thread Roy Marples

On 13/02/2021 21:34, David Young wrote:

On Tue, Feb 09, 2021 at 07:02:32AM +, Roy Marples wrote:

Hi David

On 03/02/2021 21:45, David Young wrote:


This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.


Looking at the other places where this is handled, does this patch to gre_h
address your concerns?
I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it
passes the KASSERT.


It is possible to simplify your patch a lot.  The GRE header is only 4
bytes long.  On the receive side, just perform the m_pullup like the
old code did and then memcpy to a `struct gre_h` on the stack.  On the
send side, construct the header on the stack and then memcpy it into the
mbuf.

The same general approach, of copying headers between mbufs the stack,
is probably plenty fast for virtually any size of header used in the
network stack.


Done


Re: CVS commit: src/sys/net

2021-02-14 Thread Roy Marples

On 13/02/2021 14:19, Jonathan A. Kollasch wrote:

On Sat, Feb 13, 2021 at 07:28:05AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Sat Feb 13 07:28:05 UTC 2021

Modified Files:
src/sys/net: if_ether.h if_ethersubr.c

Log Message:
if_ether: Ensure that ether_header is aligned


To generate a diff of this commit:
cvs rdiff -u -r1.84 -r1.85 src/sys/net/if_ether.h
cvs rdiff -u -r1.289 -r1.290 src/sys/net/if_ethersubr.c


This appears to ensure the Ethernet header is naturally aligned on a
32-bit boundary.  The 16-bit ether_type field is the only thing in
ether_header that is wider than a uint8_t.

Many drivers will place the start of the receive buffer at an
ETHER_ALIGN (which is 2) offset to ensure the layer three header
is 32-bit aligned after the 14-byte Ethernet header.  Thus this
will result in always calling m_copyup() in ether_input() on strict
alignment platforms.


Reverted


Re: CVS commit: src/sys

2021-02-13 Thread David Young
On Tue, Feb 09, 2021 at 07:02:32AM +, Roy Marples wrote:
> Hi David
> 
> On 03/02/2021 21:45, David Young wrote:
> >
> > This change looks a little hasty to me.
> >
> > It looks to me like some of these structs were __packed so that
> > they could be read/written directly from/to any offset in a packet
> > chain using mtod(), which does not pay any mind to the alignment
> > of `*t`:
> >
> > #define mtod(m, t)  ((t)((m)->m_data))
> >
> > I see gre_h is accessed in that way, just for one example.
> 
> Looking at the other places where this is handled, does this patch to gre_h
> address your concerns?
> I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it
> passes the KASSERT.

It is possible to simplify your patch a lot.  The GRE header is only 4
bytes long.  On the receive side, just perform the m_pullup like the
old code did and then memcpy to a `struct gre_h` on the stack.  On the
send side, construct the header on the stack and then memcpy it into the
mbuf.

The same general approach, of copying headers between mbufs the stack,
is probably plenty fast for virtually any size of header used in the
network stack.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981


Re: CVS commit: src/sys/net

2021-02-13 Thread Jonathan A. Kollasch
On Sat, Feb 13, 2021 at 07:28:05AM +, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Sat Feb 13 07:28:05 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_ether.h if_ethersubr.c
> 
> Log Message:
> if_ether: Ensure that ether_header is aligned
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.84 -r1.85 src/sys/net/if_ether.h
> cvs rdiff -u -r1.289 -r1.290 src/sys/net/if_ethersubr.c

This appears to ensure the Ethernet header is naturally aligned on a
32-bit boundary.  The 16-bit ether_type field is the only thing in
ether_header that is wider than a uint8_t.

Many drivers will place the start of the receive buffer at an
ETHER_ALIGN (which is 2) offset to ensure the layer three header
is 32-bit aligned after the 14-byte Ethernet header.  Thus this
will result in always calling m_copyup() in ether_input() on strict
alignment platforms.


Re: CVS commit: src/sys

2021-02-08 Thread Roy Marples

Hi David

On 03/02/2021 21:45, David Young wrote:
>
> This change looks a little hasty to me.
>
> It looks to me like some of these structs were __packed so that
> they could be read/written directly from/to any offset in a packet
> chain using mtod(), which does not pay any mind to the alignment
> of `*t`:
>
> #define mtod(m, t)  ((t)((m)->m_data))
>
> I see gre_h is accessed in that way, just for one example.

Looking at the other places where this is handled, does this patch to gre_h 
address your concerns?
I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it 
passes the KASSERT.


Roy

diff -r e3c82b1d9c2e sys/net/if_gre.c
--- a/sys/net/if_gre.c  Mon Feb 08 01:00:49 2021 +
+++ b/sys/net/if_gre.c  Tue Feb 09 06:55:44 2021 +
@@ -395,10 +395,26 @@
sc->sc_error_ev.ev_count++;
return;
}
-   if (m->m_len < sizeof(*gh) && (m = m_pullup(m, sizeof(*gh))) == NULL) {
-   GRE_DPRINTF(sc, "m_pullup failed\n");
-   sc->sc_pullup_ev.ev_count++;
-   return;
+
+   /* If the GRE header is not aligned, slurp it up into a new
+* mbuf with space for link headers, in the event we forward
+* it.  Otherwise, if it is aligned, make sure the entire
+* base GRE header is in the first mbuf of the chain.
+*/
+   if (GRE_HDR_ALIGNED_P(mtod(m, void *)) == 0) {
+   if ((m = m_copyup(m, sizeof(struct gre_h),
+   (max_linkhdr + 3) & ~3)) == NULL) {
+   /* XXXJRT new stat, please */
+   GRE_DPRINTF(sc, "m_copyup failed\n");
+   sc->sc_pullup_ev.ev_count++;
+   return;
+   }
+   } else if (__predict_false(m->m_len < sizeof(struct gre_h))) {
+   if ((m = m_pullup(m, sizeof(struct gre_h))) == NULL) {
+   GRE_DPRINTF(sc, "m_pullup failed\n");
+   sc->sc_pullup_ev.ev_count++;
+   return;
+   }
}
gh = mtod(m, const struct gre_h *);

@@ -940,7 +956,6 @@
 #endif

M_PREPEND(m, sizeof(*gh), M_DONTWAIT);
-
if (m == NULL) {
IF_DROP(>if_snd);
error = ENOBUFS;
@@ -948,6 +963,7 @@
}

gh = mtod(m, struct gre_h *);
+   KASSERT(GRE_HDR_ALIGNED_P(gh));
gh->flags = 0;
gh->ptype = etype;
/* XXX Need to handle IP ToS.  Look at how I handle IP TTL. */
diff -r e3c82b1d9c2e sys/net/if_gre.h
--- a/sys/net/if_gre.h  Mon Feb 08 01:00:49 2021 +
+++ b/sys/net/if_gre.h  Tue Feb 09 06:55:44 2021 +
@@ -131,6 +131,11 @@
Present if (rt_pres == 1)
  */
 };
+#ifdef __NO_STRICT_ALIGNMENT
+#defineGRE_HDR_ALIGNED_P(gh)   1
+#else
+#defineGRE_HDR_ALIGNED_P(gh)   vaddr_t) (gh)) & 3) == 0)
+#endif
 #ifdef __CTASSERT
 __CTASSERT(sizeof(struct gre_h) == 4);
 #endif


Re: CVS commit: src/sys

2021-02-07 Thread Roy Marples

On 04/02/2021 20:18, matthew green wrote:

Roy Marples writes:

On 03/02/2021 21:45, David Young wrote:

On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.


This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.


ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both ends
seems to work fine. I also tested an amd64 endpoint.

Not that I disagree with your assessment that the code can always be improved.


i looked at removing __packed from these when GCC 9 came
around and really started complaining about them.  however,
i was not able to convince myself that all the users were
actually safe if __packed was removed.

in particular, 'struct ip' has 4-byte objects at offset
14, 18, 22, and 26.  code accessing data directly from the
network may fail, and eg, mtod() makes it virtually
impossible to check for this at compile time.  sanitizers
could check at run time.


Isn't that already solved here?
http://anonhg.netbsd.org/src/rev/9f66cecd950e

And if I'm not wrong, just ignoring the warning causes alignment failures as it 
stands? So it's just swapping one bad thing with another? If so, then there is 
no right answer other than doing similar alignment checks for our mbufs.


Roy


Re: CVS commit: src/sys

2021-02-04 Thread Joerg Sonnenberger
On Thu, Feb 04, 2021 at 09:07:06PM +, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Thu Feb  4 21:07:06 UTC 2021
> 
> Modified Files:
>   src/sys/kern: vfs_init.c vfs_subr.c
>   src/sys/sys: mount.h
> 
> Log Message:
> introduce vfs.generic.timestamp_precision sysctl to control precision
> of the timer used for vfs_timestamp(); default stays the same
> to use nanotime(9), but option is there to use the faster, albeit
> less precise methods

Most of this seems to be misguided at best. I mean it makes sense to
have a variant of getnanotime vs nanotime to avoid touching the
timecounter hardware. But the rest is just begging for hard to debug bug
reports for little to no gain. Especially limiting it to microseconds
doesn't buy anything on this level really.

Joerg


re: CVS commit: src/sys

2021-02-04 Thread matthew green
Roy Marples writes:
> On 03/02/2021 21:45, David Young wrote:
> > On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:
> >> Module Name:   src
> >> Committed By:  roy
> >> Date:  Wed Feb  3 05:51:40 UTC 2021
> >>
> >> Modified Files:
> >>src/sys/net: if_arp.h if_ether.h if_gre.h
> >>src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
> >>ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> >>
> >> Log Message:
> >> Remove __packed from various network structures
> >>
> >> They are already network aligned and adding the __packed attribute
> >> just causes needless compiler warnings about accssing members of packed
> >> objects.
> > 
> > This change looks a little hasty to me.
> > 
> > It looks to me like some of these structs were __packed so that
> > they could be read/written directly from/to any offset in a packet
> > chain using mtod(), which does not pay any mind to the alignment
> > of `*t`:
> > 
> > #define mtod(m, t)  ((t)((m)->m_data))
> > 
> > I see gre_h is accessed in that way, just for one example.  I don't
> > see any reason in principle that every gre_h accessed through mtod()
> > should be 16-bit aligned in its buffer, but that is the alignment
> > the compiler will expect if gre_h is not __packed.  If the actual
> > alignment ever differs from compiler's expectation, then there
> > could be a bus error or an unwanted byte rotation/shift.
> > 
> > It looks to me like there's a bit of cleanup to do elsewhere before
> > removing __packed from network structures.
>
> ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both 
> ends 
> seems to work fine. I also tested an amd64 endpoint.
>
> Not that I disagree with your assessment that the code can always be improved.

i looked at removing __packed from these when GCC 9 came
around and really started complaining about them.  however,
i was not able to convince myself that all the users were
actually safe if __packed was removed.

in particular, 'struct ip' has 4-byte objects at offset
14, 18, 22, and 26.  code accessing data directly from the
network may fail, and eg, mtod() makes it virtually
impossible to check for this at compile time.  sanitizers
could check at run time.

i really support the _idea_ of this change, but i'm pretty
far from convinced we're ready to make it for all these
types, and we won't know unles the code actually exercises
these paths (ie, we may find a crash in years in some error
path that uses misaligned accesses.)

removing __packed as much as possible, replacing it with
specific __aligned(n) where necessary, are both goals that
i'm strongly in favour of.  eg, we could perhaps avoid the
'struct ip' issue mentioned above if we mark both the
ip_src and ip_dst members as __aligned(2).

.. but i remain unsure and worried about this change
without significant testing or code reading.


.mrg.


Re: CVS commit: src/sys/arch/hppa/gsc

2021-02-04 Thread Michael
Hello,

On Fri, 05 Feb 2021 00:12:35 +0900
Tetsuya Isaki  wrote:

> At Wed, 3 Feb 2021 13:37:24 -0500,
> Michael wrote:
> > > Committed By: isaki
> > > Date: Wed Feb  3 15:13:49 UTC 2021
> > > 
> > > Modified Files:
> > >   src/sys/arch/hppa/gsc: harmony.c
> > > 
> > > Log Message:
> > > Fix locking against myself.
> > > trigger_output will be called with sc_intr_lock held.
> > > From source code review, not tested.  
> > 
> > I just tested this on my C200 - playback works.  
> 
> Thank you for quick response!
> 
> > The sample rate seems off - everything plays too slow, but that's
> > probably completely unrelated.  
> 
> Oops, this was my mistake.
> I hope that harmony.c,v 1.9 will fix this problem.

Works perfectly now, thank you!

have fun
Michael



Re: CVS commit: src/sys

2021-02-04 Thread Roy Marples

On 03/02/2021 21:45, David Young wrote:

On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.


This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.


ssh over a gre tunnel using erlite (mips64) and pinebook (aarch64) as both ends 
seems to work fine. I also tested an amd64 endpoint.


Not that I disagree with your assessment that the code can always be improved.

Roy


Re: CVS commit: src/sys/arch/hppa/gsc

2021-02-04 Thread Tetsuya Isaki
Hello,

At Wed, 3 Feb 2021 13:37:24 -0500,
Michael wrote:
> > Committed By:   isaki
> > Date:   Wed Feb  3 15:13:49 UTC 2021
> > 
> > Modified Files:
> > src/sys/arch/hppa/gsc: harmony.c
> > 
> > Log Message:
> > Fix locking against myself.
> > trigger_output will be called with sc_intr_lock held.
> > From source code review, not tested.
> 
> I just tested this on my C200 - playback works.

Thank you for quick response!

> The sample rate seems off - everything plays too slow, but that's
> probably completely unrelated.

Oops, this was my mistake.
I hope that harmony.c,v 1.9 will fix this problem.
Sorry for breaking it.
---
Tetsuya Isaki 



Re: CVS commit: src/sys

2021-02-03 Thread David Young
On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Wed Feb  3 05:51:40 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_arp.h if_ether.h if_gre.h
>   src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
>   ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> 
> Log Message:
> Remove __packed from various network structures
> 
> They are already network aligned and adding the __packed attribute
> just causes needless compiler warnings about accssing members of packed
> objects.

This change looks a little hasty to me.

It looks to me like some of these structs were __packed so that
they could be read/written directly from/to any offset in a packet
chain using mtod(), which does not pay any mind to the alignment 
of `*t`:

#define mtod(m, t)  ((t)((m)->m_data))

I see gre_h is accessed in that way, just for one example.  I don't
see any reason in principle that every gre_h accessed through mtod()
should be 16-bit aligned in its buffer, but that is the alignment
the compiler will expect if gre_h is not __packed.  If the actual
alignment ever differs from compiler's expectation, then there
could be a bus error or an unwanted byte rotation/shift.

It looks to me like there's a bit of cleanup to do elsewhere before
removing __packed from network structures.

Dave

-- 
David Young
dyo...@pobox.comUrbana, IL(217) 721-9981


Re: CVS commit: src/sys

2021-02-03 Thread Kamil Rytarowski
On 03.02.2021 16:52, Roy Marples wrote:
> But you *have* to interogate the headers in order to work this out.

I noted how this could break. I'm right now not affected by this myself.
Please monitor gnats for reports.

I defer any decisions and discussions (if any) now to others.


Re: CVS commit: src/sys/arch/hppa/gsc

2021-02-03 Thread Michael
Hello,

On Wed, 3 Feb 2021 15:13:49 +
"Tetsuya Isaki"  wrote:

> Module Name:  src
> Committed By: isaki
> Date: Wed Feb  3 15:13:49 UTC 2021
> 
> Modified Files:
>   src/sys/arch/hppa/gsc: harmony.c
> 
> Log Message:
> Fix locking against myself.
> trigger_output will be called with sc_intr_lock held.
> From source code review, not tested.

I just tested this on my C200 - playback works.
The sample rate seems off - everything plays too slow, but that's
probably completely unrelated.

have fun
Michael


Re: CVS commit: src/sys

2021-02-03 Thread Roy Marples

On 03/02/2021 12:54, Kamil Rytarowski wrote:

This is still a valid usage and ABI breakage for userland. You cannot
blame a user for using system structures and headers that stop working
after an upgrade, at least before at least libc version bump.

For the record, I broke ABI here (it was the reverse situation, addition
of __packed).

https://github.com/NetBSD/src/commit/a833bd5cfdba983ecb5560512a3547f46f35f11e

I vote to revert and handling these structures with appropriate
functions that are aware of potentially misaligned data operations.

If we you or the project resist and insists on ABI breakage, it should
be boldly documented.


I don't buy this argument as we frequently do this:

r1.1
struct aaa {
uint16 a;
};


r1.2
struct oaaa {
uint16 a;
};

struct aaa {
uint16 a;
uint8 b;
};

Now if you choose to put your own stuff before and after in another struct, 
frankly you are really on your own.


Here's another example:

struct ehteripudp {
struct etherhdr;
struct iphdr;
struct udphdr;
};

This still works!
However, it's potentially broken as you are making the assumption there are no 
options after the ip header.


Infact this is how all the network headers have been designed.
From the start of the structure you can chain header structures together until 
you either reach options or data.


But you *have* to interogate the headers in order to work this out.

Roy


Re: CVS commit: src/sys

2021-02-03 Thread Joerg Sonnenberger
On Wed, Feb 03, 2021 at 11:03:25AM +0100, Kamil Rytarowski wrote:
> On 03.02.2021 06:51, Roy Marples wrote:
> > Module Name:src
> > Committed By:   roy
> > Date:   Wed Feb  3 05:51:40 UTC 2021
> > 
> > Modified Files:
> > src/sys/net: if_arp.h if_ether.h if_gre.h
> > src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
> > ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> > 
> > Log Message:
> > Remove __packed from various network structures
> > 
> > They are already network aligned and adding the __packed attribute
> > just causes needless compiler warnings about accssing members of packed
> > objects.
> > 
> 
> This changes the ABI for userland programs. With __packed, these
> structures, whenever contain at least 2-byte data, can be placed in a
> different location inside another structure now.

It doesn't break any of *our* ABI contracts. I also refuse us to be taken
hostage here for any (hypothetical) user program exposing internals of
the network stack on ABI boundaries. There is a very real price for the
__packed and *any* fix for the misuse has the same ABI impacts.

Joerg


Re: CVS commit: src/sys

2021-02-03 Thread Kamil Rytarowski
On 03.02.2021 13:39, Roy Marples wrote:
> On 03/02/2021 10:03, Kamil Rytarowski wrote:
>> On 03.02.2021 06:51, Roy Marples wrote:
>>> Module Name:    src
>>> Committed By:    roy
>>> Date:    Wed Feb  3 05:51:40 UTC 2021
>>>
>>> Modified Files:
>>> src/sys/net: if_arp.h if_ether.h if_gre.h
>>> src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h
>>> ip_icmp.h
>>>     ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
>>>
>>> Log Message:
>>> Remove __packed from various network structures
>>>
>>> They are already network aligned and adding the __packed attribute
>>> just causes needless compiler warnings about accssing members of packed
>>> objects.
>>>
>>
>> This changes the ABI for userland programs. With __packed, these
>> structures, whenever contain at least 2-byte data, can be placed in a
>> different location inside another structure now.
>>
>> #include 
>> #include 
>> #include 
>> #include 
>>
>> struct aaa {
>>  uint16_t a;
>>  uint8_t b;
>>  uint8_t c;
>> } __packed;
>>
>> struct aaa2 {
>>  uint8_t x;
>>  struct aaa y;
>> };
>>
>> struct bbb {
>>  uint16_t a;
>>  uint8_t b;
>>  uint8_t c;
>> };
>>
>> struct bbb2 {
>>  uint8_t x;
>>  struct bbb y;
>> };
> 
> Assuming that struct aaa and bbb are from NetBSD headers and aaa2 and
> bbb2 are your own constructs then you just have yourself to blame.
> 

It is a valid code to contain packed data in a structure without the
packed attribute.

> struct bbb2 {
>   uint8_t x;
>   struct bbb y;
> } __packed;
> 
> Makes bbb2 the same as aaa2.
> 
>> Before I saw your commit, I wanted to ask to revert the following
>> changes:
>>
>> icmp6: Remove __packed attribute from icmp6 structures
>>
>> https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462
>>
>>
>> ip6: Remove __packed attribute from ip6 structures
>>
>> https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f
>>
>>
>> The fallout can be subtle and hard to debug. Once, I removed __packed
>> from one innocent networking structure myself, qemu networking stopped
>> working at all.
> 
> How you use the structure is up to you.
> For the record, we were the only BSD to ever apply __packed to these
> structures and thanks to modern compilers emitting these wonderful
> warnings it proved to be a bad move.
> 

This is still a valid usage and ABI breakage for userland. You cannot
blame a user for using system structures and headers that stop working
after an upgrade, at least before at least libc version bump.

For the record, I broke ABI here (it was the reverse situation, addition
of __packed).

https://github.com/NetBSD/src/commit/a833bd5cfdba983ecb5560512a3547f46f35f11e

I vote to revert and handling these structures with appropriate
functions that are aware of potentially misaligned data operations.

If we you or the project resist and insists on ABI breakage, it should
be boldly documented.

> Roy



Re: CVS commit: src/sys

2021-02-03 Thread Roy Marples

On 03/02/2021 10:03, Kamil Rytarowski wrote:

On 03.02.2021 06:51, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.



This changes the ABI for userland programs. With __packed, these
structures, whenever contain at least 2-byte data, can be placed in a
different location inside another structure now.

#include 
#include 
#include 
#include 

struct aaa {
 uint16_t a;
 uint8_t b;
 uint8_t c;
} __packed;

struct aaa2 {
 uint8_t x;
 struct aaa y;
};

struct bbb {
 uint16_t a;
 uint8_t b;
 uint8_t c;
};

struct bbb2 {
 uint8_t x;
 struct bbb y;
};


Assuming that struct aaa and bbb are from NetBSD headers and aaa2 and bbb2 are 
your own constructs then you just have yourself to blame.


struct bbb2 {
  uint8_t x;
  struct bbb y;
} __packed;

Makes bbb2 the same as aaa2.


Before I saw your commit, I wanted to ask to revert the following changes:

icmp6: Remove __packed attribute from icmp6 structures

https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462

ip6: Remove __packed attribute from ip6 structures

https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f

The fallout can be subtle and hard to debug. Once, I removed __packed
from one innocent networking structure myself, qemu networking stopped
working at all.


How you use the structure is up to you.
For the record, we were the only BSD to ever apply __packed to these structures 
and thanks to modern compilers emitting these wonderful warnings it proved to be 
a bad move.


Roy


Re: CVS commit: src/sys

2021-02-03 Thread Roy Marples

On 03/02/2021 08:34, Joerg Sonnenberger wrote:

On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:

Module Name:src
Committed By:   roy
Date:   Wed Feb  3 05:51:40 UTC 2021

Modified Files:
src/sys/net: if_arp.h if_ether.h if_gre.h
src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h

Log Message:
Remove __packed from various network structures

They are already network aligned and adding the __packed attribute
just causes needless compiler warnings about accssing members of packed
objects.


Please add a compile-time assert at least for _KERNEL that the size
matches the expectation in that case.


Done


Re: CVS commit: src/sys

2021-02-03 Thread Kamil Rytarowski
On 03.02.2021 06:51, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Wed Feb  3 05:51:40 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_arp.h if_ether.h if_gre.h
>   src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
>   ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> 
> Log Message:
> Remove __packed from various network structures
> 
> They are already network aligned and adding the __packed attribute
> just causes needless compiler warnings about accssing members of packed
> objects.
> 

This changes the ABI for userland programs. With __packed, these
structures, whenever contain at least 2-byte data, can be placed in a
different location inside another structure now.

#include 
#include 
#include 
#include 

struct aaa {
uint16_t a;
uint8_t b;
uint8_t c;
} __packed;

struct aaa2 {
uint8_t x;
struct aaa y;
};

struct bbb {
uint16_t a;
uint8_t b;
uint8_t c;
};

struct bbb2 {
uint8_t x;
struct bbb y;
};

int
main()
{
printf("sizeof(aaa2) = %zu, alignof(aaa2) = %zu\n",
sizeof(struct aaa2), alignof(struct aaa2));
printf("sizeof(bbb2) = %zu, alignof(bbb2) = %zu\n",
sizeof(struct bbb2), alignof(struct bbb2));
}


$ ./a.out
sizeof(aaa) = 4, alignof(aaa) = 1
sizeof(bbb) = 4, alignof(bbb) = 2
sizeof(aaa2) = 5, alignof(aaa2) = 1
sizeof(bbb2) = 6, alignof(bbb2) = 2
Please try t access these members of the structs with memcpy(3),
memcmp(3), memset(3) etc rather than directly with * or [].

Before I saw your commit, I wanted to ask to revert the following changes:

icmp6: Remove __packed attribute from icmp6 structures

https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462

ip6: Remove __packed attribute from ip6 structures

https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f

The fallout can be subtle and hard to debug. Once, I removed __packed
from one innocent networking structure myself, qemu networking stopped
working at all.


Re: CVS commit: src/sys

2021-02-03 Thread Joerg Sonnenberger
On Wed, Feb 03, 2021 at 05:51:40AM +, Roy Marples wrote:
> Module Name:  src
> Committed By: roy
> Date: Wed Feb  3 05:51:40 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_arp.h if_ether.h if_gre.h
>   src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
>   ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
> 
> Log Message:
> Remove __packed from various network structures
> 
> They are already network aligned and adding the __packed attribute
> just causes needless compiler warnings about accssing members of packed
> objects.

Please add a compile-time assert at least for _KERNEL that the size
matches the expectation in that case.

Joerg


Re: CVS commit: src/sys/dev/pci

2021-02-01 Thread J. Hannken-Illjes
> On 12. Jul 2020, at 21:05, Jaromir Dolecek  wrote:
> 
> Module Name:  src
> Committed By: jdolecek
> Date: Sun Jul 12 19:05:32 UTC 2020
> 
> Modified Files:
>   src/sys/dev/pci: if_bnx.c if_bnxvar.h
> 
> Log Message:
> enable MSI/MSI-X if supported by adapter
> 
> tested MSI-X with Broadcom NetXtreme II BCM5709 1000Base-T
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.95 -r1.96 src/sys/dev/pci/if_bnx.c
> cvs rdiff -u -r1.12 -r1.13 src/sys/dev/pci/if_bnxvar.h
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.

With this change my Dell PowerEdge 2850 spits watchdog resets:

[  68.1828359] bnx0: Watchdog timeout -- resetting!
[  88.6042909] bnx0: Watchdog timeout -- resetting!
[ 119.0265230] bnx0: Watchdog timeout -- resetting!
[ 145.4484562] bnx0: Watchdog timeout -- resetting!

Dmesg before is:

[ 1.017306] pci4 at ppb3 bus 7
[ 1.017306] pci4: i/o space, memory space enabled, rd/line, wr/inv ok
[ 1.017306] bnx0 at pci4 dev 0 function 0: Broadcom NetXtreme II BCM5708 
1000Base-T
[ 1.017306] bnx0: Ethernet address 00:24:e8:67:4b:db
[ 1.017306] bnx0: ASIC BCM5708 B2 (0x57081020)
[ 1.017306] bnx0: PCI-X 64bit 133MHz
[ 1.017306] bnx0: B/C (4.6.0); Bufs (RX:2;TX:2); Flags (MFW); MFW (ipms 
1.6.0)
[ 1.017306] bnx0: Coal (RX:6,6,18,18; TX:20,20,80,80)
[ 1.017306] brgphy0 at bnx0 phy 1: BCM5708C 1000BASE-T media interface, 
rev. 6
[ 1.017306] brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 
1000baseT, 1000baseT-FDX, auto
[ 1.017306] bnx0: interrupting at ioapic0 pin 16

while dmesg after is:

[ 1.017262] pci4 at ppb3 bus 7
[ 1.017262] pci4: i/o space, memory space enabled, rd/line, wr/inv ok
[ 1.017262] bnx0 at pci4 dev 0 function 0: Broadcom NetXtreme II BCM5708 
1000Base-T
[ 1.017262] bnx0: Ethernet address 00:24:e8:67:4b:db
[ 1.017262] bnx0: ASIC BCM5708 B2 (0x57081020)
[ 1.017262] bnx0: PCI-X 64bit 133MHz
[ 1.017262] bnx0: B/C (4.6.0); Bufs (RX:2;TX:2); Flags (MFW); MFW (ipms 
1.6.0)
[ 1.017262] bnx0: Coal (RX:6,6,18,18; TX:20,20,80,80)
[ 1.017262] brgphy0 at bnx0 phy 1: BCM5708C 1000BASE-T media interface, 
rev. 6
[ 1.017262] brgphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 
1000baseT, 1000baseT-FDX, auto
[ 1.017262] bnx0: interrupting at msi0 vec 0

Pcictl dump gives (in the MSI-X case):

  PCI Message Signaled Interrupt
Message Control register: 0x0081
  MSI Enabled: on
  Multiple Message Capable: no (1 vector)
  Multiple Message Enabled: off (1 vector)
  64 Bit Address Capable: on
  Per-Vector Masking Capable: off
  Extended Message Data Capable: off
  Extended Message Data Enable: off
Message Address (lower) register: 0xfee0
Message Address (upper) register: 0x
Message Data register: 0x0064

Any ideas how to fix this issue?

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)


signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/net

2021-01-31 Thread Kengo NAKAHARA

Hi,

I have one question about the following commit.

Why stoeplitz_hash_ip4() and stoeplitz_hash_ip4port() argument types
are different between toeplitz.c and toeplitz.h?  They have in_addr_t
and in_port_t argument types in toeplitz.c, howerver uint32_t and
uint16_t in toeplitz.h.

On 2021/01/31 6:23, Jared D. McNeill wrote:

Module Name:src
Committed By:   jmcneill
Date:   Sat Jan 30 21:23:08 UTC 2021

Modified Files:
src/sys/net: files.net
Added Files:
src/sys/net: toeplitz.c toeplitz.h

Log Message:
Add symmetric toeplitz implementation with integration for NICs, from OpenBSD.


To generate a diff of this commit:
cvs rdiff -u -r1.29 -r1.30 src/sys/net/files.net
cvs rdiff -u -r0 -r1.1 src/sys/net/toeplitz.c src/sys/net/toeplitz.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Thanks,


--
//
Internet Initiative Japan Inc.

Device Engineering Section,
Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA 


Re: CVS commit: src/sys/sys

2021-01-26 Thread Jason Thorpe


> On Jan 26, 2021, at 6:49 PM, Valery Ushakov  wrote:
> 
> It's hardly fair to characterize three people politely inquiring about
> that choice and pointing out a more standard way to spell what you
> want to express (that is perfectly in rhyme with the preceding table
> and is only one character longer too) as "angry hordes objecting".

Sorry, should have added the "/s" at the end.  Was kind of a long day, and this 
was a misguided attempt at decompression humor.

-- thorpej



Re: CVS commit: src/sys/sys

2021-01-26 Thread Valery Ushakov
On Wed, Jan 27, 2021 at 01:00:05 +, Jason R Thorpe wrote:

> Module Name:  src
> Committed By: thorpej
> Date: Wed Jan 27 01:00:05 UTC 2021
> 
> Modified Files:
>   src/sys/sys: device.h
> 
> Log Message:
> Define a macro to terminate the common usage of device_compatible_entry
> arrays, in order to placate the angry hordes objecting to use of a GCC
> extension.

It's hardly fair to characterize three people politely inquiring about
that choice and pointing out a more standard way to spell what you
want to express (that is perfectly in rhyme with the preceding table
and is only one character longer too) as "angry hordes objecting".

The point is moot anyway, since anonymous unions themselves only
officially appeared in C11.

-uwe


Re: CVS commit: src/sys/dev/pci

2021-01-26 Thread Reinoud Zandijk
On Tue, Jan 26, 2021 at 05:51:42PM +0900, Rin Okuyama wrote:
> Hi,

> This seems not correct for me. Is the attached patch OK with you?

Well you spotted a bug indeed int he freeing section. I'll fix and commit it.
Thanks for reporting.

Reinoud


signature.asc
Description: PGP signature


Re: CVS commit: src/sys/dev/pci

2021-01-26 Thread Rin Okuyama

Hi,

On 2021/01/25 0:33, Reinoud Zandijk wrote:

Module Name:src
Committed By:   reinoud
Date:   Sun Jan 24 15:33:02 UTC 2021

Modified Files:
src/sys/dev/pci: virtio_pci.c

Log Message:
On error unmap the pci_mapreg_map()d regions using bus_space_unmap() as
suggested by jak@


To generate a diff of this commit:
cvs rdiff -u -r1.22 -r1.23 src/sys/dev/pci/virtio_pci.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


This seems not correct for me. Is the attached patch OK with you?

Thanks,
rin
Index: sys/dev/pci/virtio_pci.c
===
RCS file: /home/netbsd/src/sys/dev/pci/virtio_pci.c,v
retrieving revision 1.25
diff -p -u -r1.25 virtio_pci.c
--- sys/dev/pci/virtio_pci.c24 Jan 2021 15:59:35 -  1.25
+++ sys/dev/pci/virtio_pci.c26 Jan 2021 08:47:23 -
@@ -444,7 +444,7 @@ virtio_pci_attach_10(device_t self, void
bus_size_t bars[NMAPREG] = { 0 };
int bars_idx[NMAPREG] = { 0 };
struct virtio_pci_cap *caps[] = { , , ,  };
-   int i, j = 0, ret = 0;
+   int i, j, ret = 0;
 
if (virtio_pci_find_cap(psc, VIRTIO_PCI_CAP_COMMON_CFG,
, sizeof(common)))
@@ -471,7 +471,7 @@ virtio_pci_attach_10(device_t self, void
bars[bar] = len;
}
 
-   for (i = 0; i < __arraycount(bars); i++) {
+   for (i = j = 0; i < __arraycount(bars); i++) {
int reg;
pcireg_t type;
if (bars[i] == 0)
@@ -550,11 +550,12 @@ virtio_pci_attach_10(device_t self, void
 
 err:
/* undo our pci_mapreg_map()s */ 
-   for (i = 0; i < __arraycount(bars); i++) {
+   for (i = j = 0; i < __arraycount(bars); i++) {
if (bars[i] == 0)
continue;
bus_space_unmap(psc->sc_bars_iot[j], psc->sc_bars_ioh[j],
psc->sc_bars_iosize[j]);
+   j++;
}
return ret;
 }


Re: CVS commit: src/sys/arch

2021-01-25 Thread Jason Thorpe


> On Jan 25, 2021, at 9:45 AM, Robert Elz  wrote:
> 
>Date:Mon, 25 Jan 2021 08:19:44 -0800
>From:Jason Thorpe 
>Message-ID:  
> 
>  | Using { 0 } makes an assumption about the first member of the
>  | structure which is not guaranteed to remain true.
> 
> That's right, but you could explicitly init a named field, most likely
> the one that is tested to determine that this is the sentinel, eg: from
> one of the recent updates ...
> 
> static const struct device_compatible_entry compat_data[] = {
>{ .compat = "pnpPNP,401" },
> -   { 0 }
> +   { }
> };
> 
> that could instead be changed to
>   { .compat = NULL }
> 
> (or something similar to that).

I noticed this because of a different local change in my tree that makes the 
first field another anonymous union.

Anyhow, I'll go ahead and define a standard sentinel macro that can be used for 
the common { .compat = XXX } case, and fire up sed to fix up the tree.

-- thorpej



Re: CVS commit: src/sys/arch

2021-01-25 Thread Kamil Rytarowski
On 25.01.2021 17:19, Jason Thorpe wrote:
> 
>> On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski  wrote:
>>
>> I have no problem with this change but I am curious why should we use "{
>> }"? It's a C GNU extension and C++ syntax.
> 
> Using { 0 } makes an assumption about the first member of the structure which 
> is not guaranteed to remain true.
> 

Both versions should work in the same way, except that {0} is the
standard variation and {} an extension.

I have got no preference for either.

> -- thorpej
> 




signature.asc
Description: OpenPGP digital signature


Re: CVS commit: src/sys/arch

2021-01-25 Thread Valery Ushakov
On Mon, Jan 25, 2021 at 20:28:52 +0300, Valery Ushakov wrote:

> On Mon, Jan 25, 2021 at 08:19:44 -0800, Jason Thorpe wrote:
> 
> > > On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski  wrote:
> > > 
> > > I have no problem with this change but I am curious why should we use "{
> > > }"? It's a C GNU extension and C++ syntax.
> > 
> > Using { 0 } makes an assumption about the first member of the
> > structure which is not guaranteed to remain true.
> 
> The commit message says:
> 
> | Since we're using designated initialisers for compat data, we should
> | use a completely empty initializer for the sentinel. 
> 
> but that "should" is not true.  The code that checks that sentinel
> uses some particular member to access it, so, pedantically speaking,
> the initialization must designate that member in the sentinel
> initialization.  Yes, this is verbose and doesn't look as pretty, but
> that is what "should" happen here.  Using non-standard {} extension
> makes it look nicer, but is not a "should" kind of necessity.

PS: Forgot to add that C++ doesn't have designated initializers (or at
least it didn't last time I looked), so they are in a different
situation here and need an empty initializer list.  In C the only
difference it makes is, as far as I can tell, exactly this kind of an
array with a sentinel at the end and the difference is cosmetic.

-uwe


Re: CVS commit: src/sys/arch

2021-01-25 Thread Robert Elz
Date:Mon, 25 Jan 2021 08:19:44 -0800
From:Jason Thorpe 
Message-ID:  

  | Using { 0 } makes an assumption about the first member of the
  | structure which is not guaranteed to remain true.

That's right, but you could explicitly init a named field, most likely
the one that is tested to determine that this is the sentinel, eg: from
one of the recent updates ...

 static const struct device_compatible_entry compat_data[] = {
{ .compat = "pnpPNP,401" },
-   { 0 }
+   { }
 };

that could instead be changed to
{ .compat = NULL }

(or something similar to that).

kre



  1   2   3   4   5   6   7   8   9   10   >