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



Re: CVS commit: src/sys/arch

2021-01-25 Thread Valery Ushakov
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.

-uwe


Re: CVS commit: src/sys/arch

2021-01-25 Thread Jason Thorpe


> 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.

-- thorpej



Re: CVS commit: src/sys/arch

2021-01-25 Thread Kamil Rytarowski
On 25.01.2021 15:20, Jason R Thorpe wrote:
> Module Name:  src
> Committed By: thorpej
> Date: Mon Jan 25 14:20:39 UTC 2021
> 
> Modified Files:
>   src/sys/arch/arm/altera: cycv_clkmgr.c
>   src/sys/arch/arm/amlogic: meson_pinctrl.c meson_pwm.c meson_thermal.c
>   meson_usbctrl.c meson_usbphy.c mesong12_clkc.c mesongx_mmc.c
>   mesongxbb_clkc.c
>   src/sys/arch/arm/broadcom: bcm2835_emmc.c bcm2835_intr.c
>   src/sys/arch/arm/fdt: gicv3_fdt.c pcihost_fdt.c
>   src/sys/arch/arm/nvidia: tegra_ahcisata.c tegra_nouveau.c tegra_pcie.c
>   tegra_pinmux.c tegra_soctherm.c tegra_xusb.c
>   src/sys/arch/arm/nxp: if_enet_imx.c imx6_pcie.c imx6_spi.c
>   imx8mq_usbphy.c imx_sdhc.c
>   src/sys/arch/arm/rockchip: rk3328_iomux.c rk3399_iomux.c rk_gmac.c
>   rk_i2s.c rk_pwm.c rk_tsadc.c rk_usb.c rk_v1crypto.c rk_vop.c
>   src/sys/arch/arm/samsung: exynos_dwcmmc.c exynos_pinctrl.c
>   exynos_platform.c exynos_usbdrdphy.c exynos_usbphy.c
>   src/sys/arch/arm/sociox: if_ave.c
>   src/sys/arch/arm/sunxi: sun4i_a10_ccu.c sun4i_dma.c sun6i_dma.c
>   sun8i_crypto.c sunxi_can.c sunxi_codec.c sunxi_de2_ccu.c
>   sunxi_dep.c sunxi_gpio.c sunxi_hdmi.c sunxi_hdmiphy.c sunxi_i2s.c
>   sunxi_lcdc.c sunxi_lradc.c sunxi_mixer.c sunxi_mmc.c sunxi_musb.c
>   sunxi_nmi.c sunxi_pwm.c sunxi_rsb.c sunxi_rtc.c sunxi_sid.c
>   sunxi_sramc.c sunxi_tcon.c sunxi_thermal.c sunxi_ts.c sunxi_twi.c
>   sunxi_usb3phy.c sunxi_usbphy.c sunxi_wdt.c
>   src/sys/arch/arm/ti: ti_dpll_clock.c ti_gpio.c ti_iic.c ti_omapintc.c
>   ti_omaptimer.c ti_sdhc.c
>   src/sys/arch/macppc/dev: deq.c lmu.c psoc.c smusat.c
>   src/sys/arch/mips/cavium/dev: octeon_cib.c octeon_intc.c
>   src/sys/arch/sparc64/dev: pcf8591_envctrl.c
> 
> Log Message:
> Since we're using designated initialisers for compat data, we should
> use a completely empty initializer for the sentinel.
> 

>  static const struct device_compatible_entry compat_data[] = {
>   { .compat = "ecadc" },
> -
> - { 0 }
> + { }
>  };
>  
>  static int
> 

I have no problem with this change but I am curious why should we use "{
}"? It's a C GNU extension and C++ syntax.



signature.asc
Description: OpenPGP digital signature


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

2021-01-25 Thread Jason Thorpe


> On Jan 24, 2021, at 10:20 PM, Martin Husemann  wrote:
> 
>> I kept getting a ?static after non-static declaration? error when building 
>> for aarch64.
> 
> I guess that was with outdated arm/include/bus_funcs.h and
> sys/bus_proto.h (or where was the previous declaration)?

I did a “cvs update” just before, *shrug*.  In any case, the problem was easily 
avoidable, and now it is avoided.

-- thorpej



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

2021-01-24 Thread Martin Husemann
On Sun, Jan 24, 2021 at 09:45:14PM -0800, Jason Thorpe wrote:
> 
> > On Jan 24, 2021, at 9:37 PM, Martin Husemann  wrote:
> > 
> > While I don't care for names, I would like to understand fallout in
> > details before hiding it - what exactly did not compile correctly?
> > At least the affected arm kernels worked for me in the state directly
> > before your commit.
> 
> I kept getting a ?static after non-static declaration? error when building 
> for aarch64.

I guess that was with outdated arm/include/bus_funcs.h and
sys/bus_proto.h (or where was the previous declaration)?

Martin


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

2021-01-24 Thread Jason Thorpe


> On Jan 24, 2021, at 9:37 PM, Martin Husemann  wrote:
> 
> While I don't care for names, I would like to understand fallout in
> details before hiding it - what exactly did not compile correctly?
> At least the affected arm kernels worked for me in the state directly
> before your commit.

I kept getting a “static after non-static declaration” error when building for 
aarch64.

-- thorpej



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

2021-01-24 Thread Martin Husemann
On Sun, Jan 24, 2021 at 03:34:08PM +, Jason R Thorpe wrote:
> Module Name:  src
> Committed By: thorpej
> Date: Sun Jan 24 15:34:08 UTC 2021
> 
> Modified Files:
>   src/sys/dev/pci: virtio_pci.c
> 
> Log Message:
> Redefining bus_space functions in drivers is a bad idea, and we just
> should't be in the habit of doing so.  Besides, the previous "solutions"
> still did not compile correctly, and this does, so let's be done with
> this nonsense, shall we?

While I don't care for names, I would like to understand fallout in
details before hiding it - what exactly did not compile correctly?
At least the affected arm kernels worked for me in the state directly
before your commit.

Martin


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

2021-01-23 Thread Jason Thorpe


> On Jan 23, 2021, at 5:40 PM, Christos Zoulas  wrote:
> 
>> it's a bad example.  someone might copy it into another
>> driver that _doesn't_ work with this version, but may seem
>> to fix a build error.
>> 
>> that's why i wanted to wrap the usage to make it clear if
>> someone were to copy it elsewhere.
> 
> I will add a comment.

Yah, I was gonna say, “big fat comment in order!"

-- thorpej



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

2021-01-23 Thread Christos Zoulas
In article <17141.1611452...@splode.eterna.com.au>,
matthew green   wrote:
>Christos Zoulas writes:
>> In article <20210123230600.52be160...@jupiter.mumble.net>,
>> Taylor R Campbell   wrote:
>> >
>> >Conversely, how do you know whether this hacked-up implementation
>> >which tears the write into two will actually work?  Maybe it works for
>> >virtio but there are likely other devices out there for which it will
>> >fail or have weird side effects if the architecture doesn't have
>> >native 8-byte bus I/O.
>>
>> But it is a static function defined in virtio_pci.c. How will other
>> devices use it? I must be missing something.
>
>it's a bad example.  someone might copy it into another
>driver that _doesn't_ work with this version, but may seem
>to fix a build error.
>
>that's why i wanted to wrap the usage to make it clear if
>someone were to copy it elsewhere.

I will add a comment.

christos



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

2021-01-23 Thread matthew green
Christos Zoulas writes:
> In article <20210123230600.52be160...@jupiter.mumble.net>,
> Taylor R Campbell   wrote:
> >
> >Conversely, how do you know whether this hacked-up implementation
> >which tears the write into two will actually work?  Maybe it works for
> >virtio but there are likely other devices out there for which it will
> >fail or have weird side effects if the architecture doesn't have
> >native 8-byte bus I/O.
>
> But it is a static function defined in virtio_pci.c. How will other
> devices use it? I must be missing something.

it's a bad example.  someone might copy it into another
driver that _doesn't_ work with this version, but may seem
to fix a build error.

that's why i wanted to wrap the usage to make it clear if
someone were to copy it elsewhere.


.mrg.


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

2021-01-23 Thread Christos Zoulas
In article <20210123230600.52be160...@jupiter.mumble.net>,
Taylor R Campbell   wrote:
>
>Conversely, how do you know whether this hacked-up implementation
>which tears the write into two will actually work?  Maybe it works for
>virtio but there are likely other devices out there for which it will
>fail or have weird side effects if the architecture doesn't have
>native 8-byte bus I/O.

But it is a static function defined in virtio_pci.c. How will other
devices use it? I must be missing something.

christos



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

2021-01-23 Thread Taylor R Campbell
> Date: Sat, 23 Jan 2021 22:59:22 - (UTC)
> From: chris...@astron.com (Christos Zoulas)
> 
> In article <23974.1611441...@splode.eterna.com.au>,
> matthew green   wrote:
> >this seems dangerous to me.  we don't define it on
> >some platforms because we can't, so having it faked
> >out here seems like someone later will be confused
> >and the wrong thing will happen.
> >
> >i would rather have something like
> >
> >virtio_write8(...)
> >{
> >#ifdef __HAVE_BUS_SPACE_8
> > just use the real thing
> >#else
> > use the dual-_4 version that is ok _for this device_
> >#endif
> >}
> >
> >and then use this wrapper in the rest of the code.
> 
> This implementation is internal to virtio_pci and is guaranteed to work
> by the spec, how will someone else us it?

Conversely, how do you know whether this hacked-up implementation
which tears the write into two will actually work?  Maybe it works for
virtio but there are likely other devices out there for which it will
fail or have weird side effects if the architecture doesn't have
native 8-byte bus I/O.


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

2021-01-23 Thread Christos Zoulas
In article <23974.1611441...@splode.eterna.com.au>,
matthew green   wrote:
>"Christos Zoulas" writes:
>> Module Name: src
>> Committed By:christos
>> Date:Sat Jan 23 20:00:19 UTC 2021
>>
>> Modified Files:
>>  src/sys/dev/pci: virtio_pci.c
>>
>> Log Message:
>> Provide a generic bus_space_write_8 function that is bi-endian.
>
>this seems dangerous to me.  we don't define it on
>some platforms because we can't, so having it faked
>out here seems like someone later will be confused
>and the wrong thing will happen.
>
>i would rather have something like
>
>virtio_write8(...)
>{
>#ifdef __HAVE_BUS_SPACE_8
>   just use the real thing
>#else
>   use the dual-_4 version that is ok _for this device_
>#endif
>}
>
>and then use this wrapper in the rest of the code.

This implementation is internal to virtio_pci and is guaranteed to work
by the spec, how will someone else us it?


christos



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

2021-01-23 Thread matthew green
"Christos Zoulas" writes:
> Module Name:  src
> Committed By: christos
> Date: Sat Jan 23 20:00:19 UTC 2021
>
> Modified Files:
>   src/sys/dev/pci: virtio_pci.c
>
> Log Message:
> Provide a generic bus_space_write_8 function that is bi-endian.

this seems dangerous to me.  we don't define it on
some platforms because we can't, so having it faked
out here seems like someone later will be confused
and the wrong thing will happen.

i would rather have something like

virtio_write8(...)
{
#ifdef __HAVE_BUS_SPACE_8
just use the real thing
#else
use the dual-_4 version that is ok _for this device_
#endif
}

and then use this wrapper in the rest of the code.


.mrg.


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

2021-01-23 Thread Christos Zoulas
In article ,
Reinoud Zandijk   wrote:
>On Fri, Jan 22, 2021 at 04:54:51PM +1100, matthew green wrote:
>> > +#ifndef _LP64
>> 
>> _LP64 is a terrible way to make this choice.
>> 
>> heaps of our 32 bit platforms implement the _8 variants.
>
>Can't we then just make sure they have the 8 bit variant? and set a define if
>its atomic or not?
>
>This way drivers van use the _8 variant freely and for the few drivers that
>NEED the atomicity, they can check the define and deal with it the way they
>like.

Perhaps. But still for the ones that don't have it should use the central
implementation so that we don't duplicate code.

christos



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

2021-01-23 Thread Christos Zoulas
In article ,
Nick Hudson   wrote:
>On 22/01/2021 04:48, Christos Zoulas wrote:
>> +#if _QUAD_HIGHWORD
>> +bus_space_write_4(iot, ioh, offset, (uint32_t)(value & 0x));
>> +bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value >> 32));
>> +#else
>> +bus_space_write_4(iot, ioh, offset, (uint32_t)(value >> 32));
>> +bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value & 0x));
>> +#endif
>> +}
>> +#endif /* !_LP64 */
>
>
>BUS_ADDR_{HI,LO}32 are also available for your convenience.

Will use those thanks!

christos



Re: CVS commit: src/sys/kern (kern_event.c)

2021-01-22 Thread Paul Goyette

On Fri, 22 Jan 2021, Paul Goyette wrote:


On Thu, 21 Jan 2021, Paul Goyette wrote:


Ooopppsss ignore me - looks like this was already fixed and my update
missed it.

I'll retry.


OK, I built and installed a new kernel+userland.

Most everything works, and syslogd seems to work fine (at least, it
no longer panics during startup).

HOWEVER, firefox seems to be badly broken.  Attempting to open certain
pages results in never-ending-hang, and nothing ever gets rendered.  I
can use the Stop-Reloading "X" button, and the "oscillating dot" load
indicator stops oscillating, but nothing ever happens.  At that point,
the tab is hung and cannot load any other page, not even pages that
loaded successfully previously!  I _can_ delete the tab, and opening a
new tab works.

Some of the "failing" pages are:

airnow.gov
gmail.com
www.prudential.com/login
www.myaccountviewonline.com/AccountView/Logon


Slight correction:  above I said "nothing ever happens" but while I've
been composing this Email a couple of the above pages seem to have made
some progress (although none of them have finished and stopped the
"oscillating dot").  So "ever" is at least 5 minutes or longer ...  :)


I don't know if the kern_event.c changes are responsible, but I haven't
seen anything else recently.


I reverted kern_event.c to rev 1.110 and firefox behaves correctly.  So
it's pretty fair bet that the subsequent kern_event.c changes are the
reason for the breakage.

PR kern/55946 has been filed.


++--+---+
| 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/kern (kern_event.c)

2021-01-22 Thread Paul Goyette

On Thu, 21 Jan 2021, Paul Goyette wrote:


Ooopppsss ignore me - looks like this was already fixed and my update
missed it.

I'll retry.


OK, I built and installed a new kernel+userland.

Most everything works, and syslogd seems to work fine (at least, it
no longer panics during startup).

HOWEVER, firefox seems to be badly broken.  Attempting to open certain
pages results in never-ending-hang, and nothing ever gets rendered.  I
can use the Stop-Reloading "X" button, and the "oscillating dot" load
indicator stops oscillating, but nothing ever happens.  At that point,
the tab is hung and cannot load any other page, not even pages that
loaded successfully previously!  I _can_ delete the tab, and opening a
new tab works.

Some of the "failing" pages are:

airnow.gov
gmail.com
www.prudential.com/login
www.myaccountviewonline.com/AccountView/Logon


Slight correction:  above I said "nothing ever happens" but while I've
been composing this Email a couple of the above pages seem to have made
some progress (although none of them have finished and stopped the
"oscillating dot").  So "ever" is at least 5 minutes or longer ...  :)


I don't know if the kern_event.c changes are responsible, but I haven't
seen anything else recently.

FWIW, I'm running firefox 83.0 from pkgsrc, around 2020-12-08





On Thu, 21 Jan 2021, Paul Goyette wrote:


This change seems to break everything!  As soon as I try to start
syslogd I hit the panic() that you added

[  28.0253983] panic: kqueue_scan,1491: kq=0xdc13890bc4c0 
kq->kq_count(1) != count(0), nmarker=1


[  28.0253983] cpu0: Begin traceback...
[  28.0253983] vpanic() at netbsd:vpanic+0x156
[  28.0253983] snprintf() at netbsd:snprintf
[  28.0253983] kqueue_check() at netbsd:kqueue_check+0x183
[  28.0253983] kevent1() at netbsd:kevent1+0x49f
[  28.0253983] sys___kevent50() at netbsd:sys___kevent50+0x33
[  28.0253983] syscall() at netbsd:syscall+0x23e
[  28.0253983] --- syscall (number 435) ---
[  28.0253983] netbsd:syscall+0x23e:
[  28.0253983] cpu0: End traceback...
[  28.0253983] fatal breakpoint trap in supervisor mode
[  28.0253983] trap type 1 code 0 rip 0x8021f415 cs 0x8 rflags 
0x202 cr2 0x78742459e000 ilevel 0x8 rsp 0xa809281ebb50
[  28.0253983] curlwp 0xdc138aa46540 pid 1352.1352 lowest kstack 
0xa809281e72c0

Stopped in pid 1352.1352 (syslogd) at   netbsd:breakpoint+0x5:  leave

I have a full crash dump if you need any further info.


Module Name:src
Committed By:   jdolecek
Date:   Thu Jan 21 18:09:23 UTC 2021

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

Log Message:
adjust kq_check() (enabled with DEBUG) to new reality - it's now perfectly
normal to have kq_count bigger than number of the linked entries
on the kqueue

PR kern/50094, problem pointed out by Chuck Silvers


To generate a diff of this commit:
cvs rdiff -u -r1.111 -r1.112 src/sys/kern/kern_event.c

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


++--+---+
| 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   |
++--+---+



++--+---+
| 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   |
++--+---+



++--+---+
| 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/pci

2021-01-22 Thread Reinoud Zandijk
On Fri, Jan 22, 2021 at 04:54:51PM +1100, matthew green wrote:
> > +#ifndef _LP64
> 
> _LP64 is a terrible way to make this choice.
> 
> heaps of our 32 bit platforms implement the _8 variants.

Can't we then just make sure they have the 8 bit variant? and set a define if
its atomic or not?

This way drivers van use the _8 variant freely and for the few drivers that
NEED the atomicity, they can check the define and deal with it the way they
like.

Reinoud


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

2021-01-22 Thread Jared McNeill
I forgot that I added dma-ranges support back in Feb last year. All good, 
please ignore me :)



On Thu, 21 Jan 2021, Jared McNeill wrote:

This driver is not 64-bit safe and should not be enabled on aarch64 as-is. I 
think before turning it on we should restrict it to the lower 1GB of memory 
like the Pi3 models where we know it at least has a chance of working. You 
can do this easily by creating a restrictive tag using 
bus_dmatag_subregion(9).


Take care,
Jared

On Thu, 21 Jan 2021, Nia Alarie wrote:


Module Name:src
Committed By:   nia
Date:   Thu Jan 21 17:46:28 UTC 2021

Modified Files:
src/sys/arch/evbarm/conf: GENERIC64

Log Message:
add vcaudio (intentionally this time)

gives working audio output on rpi3 without needing to run a 32-bit image.


To generate a diff of this commit:
cvs rdiff -u -r1.173 -r1.174 src/sys/arch/evbarm/conf/GENERIC64

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/pci

2021-01-22 Thread Nick Hudson

On 22/01/2021 04:48, Christos Zoulas wrote:

+#if _QUAD_HIGHWORD
+   bus_space_write_4(iot, ioh, offset, (uint32_t)(value & 0x));
+   bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value >> 32));
+#else
+   bus_space_write_4(iot, ioh, offset, (uint32_t)(value >> 32));
+   bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value & 0x));
+#endif
+}
+#endif /* !_LP64 */



BUS_ADDR_{HI,LO}32 are also available for your convenience.

Nick


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

2021-01-21 Thread Christos Zoulas
In article <27744.1611294...@splode.eterna.com.au>,
matthew green   wrote:
>> +#ifndef _LP64
>
>_LP64 is a terrible way to make this choice.
>
>heaps of our 32 bit platforms implement the _8 variants.

Let's add a _HAVE_ variable then?

christos



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

2021-01-21 Thread matthew green
> +#ifndef _LP64

_LP64 is a terrible way to make this choice.

heaps of our 32 bit platforms implement the _8 variants.


.mrg.


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

2021-01-21 Thread Christos Zoulas
In article <20210121204833.9ebcff...@cvs.netbsd.org>,
Reinoud Zandijk  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  reinoud
>Date:  Thu Jan 21 20:48:33 UTC 2021
>
>Modified Files:
>   src/sys/dev/pci: virtio_pci.c
>
>Log Message:
>Remove dependency on bus_space_write_8() for i386 and instead implement it as
>two bus_space_write_4()'s as allowed in the spec.

Isn't it better to do it this way so it always works (not just for little
endian)? We could even provide this in the MI bus.h if others need it
and don't care about the non-atomic transactions. I have not even compile
tested it.

christos

Index: virtio_pci.c
===
RCS file: /cvsroot/src/sys/dev/pci/virtio_pci.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 virtio_pci.c
--- virtio_pci.c21 Jan 2021 20:48:33 -  1.18
+++ virtio_pci.c22 Jan 2021 04:46:24 -
@@ -34,6 +34,7 @@ __KERNEL_RCSID(0, "$NetBSD: virtio_pci.c
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -731,9 +732,20 @@ virtio_pci_read_queue_size_10(struct vir
  * By definition little endian only in v1.0 and 8 byters are allowed to be
  * written as two 4 byters
  */
-#define bus_space_write_le_8(iot, ioh, reg, val) \
-   bus_space_write_4(iot, ioh, reg, ((uint64_t) (val)) & 0x); \
-   bus_space_write_4(iot, ioh, reg + 4, ((uint64_t) (val)) >> 32);
+#ifndef _LP64
+static __inline void
+bus_space_write_8(bus_space_tag_t iot, bus_space_handle_t ioh,
+ bus_size_t offset, uint64_t value)
+{
+#if _QUAD_HIGHWORD
+   bus_space_write_4(iot, ioh, offset, (uint32_t)(value & 0x));
+   bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value >> 32));
+#else
+   bus_space_write_4(iot, ioh, offset, (uint32_t)(value >> 32));
+   bus_space_write_4(iot, ioh, offset + 4, (uint32_t)(value & 0x));
+#endif
+}
+#endif /* !_LP64 */
 
 static void
 virtio_pci_setup_queue_10(struct virtio_softc *sc, uint16_t idx, uint64_t addr)
@@ -747,15 +759,15 @@ virtio_pci_setup_queue_10(struct virtio_
bus_space_write_2(iot, ioh, VIRTIO_CONFIG1_QUEUE_SELECT, vq->vq_index);
if (addr == 0) {
bus_space_write_2(iot, ioh, VIRTIO_CONFIG1_QUEUE_ENABLE, 0);
-   bus_space_write_le_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_DESC,   0);
-   bus_space_write_le_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_AVAIL,  0);
-   bus_space_write_le_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_USED,   0);
+   bus_space_write_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_DESC,   0);
+   bus_space_write_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_AVAIL,  0);
+   bus_space_write_8(iot, ioh, VIRTIO_CONFIG1_QUEUE_USED,   0);
} else {
-   bus_space_write_le_8(iot, ioh,
+   bus_space_write_8(iot, ioh,
VIRTIO_CONFIG1_QUEUE_DESC, addr);
-   bus_space_write_le_8(iot, ioh,
+   bus_space_write_8(iot, ioh,
VIRTIO_CONFIG1_QUEUE_AVAIL, addr + vq->vq_availoffset);
-   bus_space_write_le_8(iot, ioh,
+   bus_space_write_8(iot, ioh,
VIRTIO_CONFIG1_QUEUE_USED, addr + vq->vq_usedoffset);
bus_space_write_2(iot, ioh,
VIRTIO_CONFIG1_QUEUE_ENABLE, 1);
@@ -771,7 +783,6 @@ virtio_pci_setup_queue_10(struct virtio_
VIRTIO_CONFIG1_QUEUE_MSIX_VECTOR, vec);
}
 }
-#undef bus_space_write_le_8
 
 static void
 virtio_pci_set_status_10(struct virtio_softc *sc, int status)




Re: CVS commit: src/sys/kern (kern_event.c)

2021-01-21 Thread Paul Goyette

Ooopppsss ignore me - looks like this was already fixed and my update
missed it.

I'll retry.


On Thu, 21 Jan 2021, Paul Goyette wrote:


This change seems to break everything!  As soon as I try to start
syslogd I hit the panic() that you added

[  28.0253983] panic: kqueue_scan,1491: kq=0xdc13890bc4c0 kq->kq_count(1) 
!= count(0), nmarker=1


[  28.0253983] cpu0: Begin traceback...
[  28.0253983] vpanic() at netbsd:vpanic+0x156
[  28.0253983] snprintf() at netbsd:snprintf
[  28.0253983] kqueue_check() at netbsd:kqueue_check+0x183
[  28.0253983] kevent1() at netbsd:kevent1+0x49f
[  28.0253983] sys___kevent50() at netbsd:sys___kevent50+0x33
[  28.0253983] syscall() at netbsd:syscall+0x23e
[  28.0253983] --- syscall (number 435) ---
[  28.0253983] netbsd:syscall+0x23e:
[  28.0253983] cpu0: End traceback...
[  28.0253983] fatal breakpoint trap in supervisor mode
[  28.0253983] trap type 1 code 0 rip 0x8021f415 cs 0x8 rflags 0x202 
cr2 0x78742459e000 ilevel 0x8 rsp 0xa809281ebb50
[  28.0253983] curlwp 0xdc138aa46540 pid 1352.1352 lowest kstack 
0xa809281e72c0

Stopped in pid 1352.1352 (syslogd) at   netbsd:breakpoint+0x5:  leave

I have a full crash dump if you need any further info.


Module Name:src
Committed By:   jdolecek
Date:   Thu Jan 21 18:09:23 UTC 2021

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

Log Message:
adjust kq_check() (enabled with DEBUG) to new reality - it's now perfectly
normal to have kq_count bigger than number of the linked entries
on the kqueue

PR kern/50094, problem pointed out by Chuck Silvers


To generate a diff of this commit:
cvs rdiff -u -r1.111 -r1.112 src/sys/kern/kern_event.c

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


++--+---+
| 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   |
++--+---+



++--+---+
| 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/kern (kern_event.c)

2021-01-21 Thread Paul Goyette

This change seems to break everything!  As soon as I try to start
syslogd I hit the panic() that you added

[  28.0253983] panic: kqueue_scan,1491: kq=0xdc13890bc4c0 kq->kq_count(1) 
!= count(0), nmarker=1

[  28.0253983] cpu0: Begin traceback...
[  28.0253983] vpanic() at netbsd:vpanic+0x156
[  28.0253983] snprintf() at netbsd:snprintf
[  28.0253983] kqueue_check() at netbsd:kqueue_check+0x183
[  28.0253983] kevent1() at netbsd:kevent1+0x49f
[  28.0253983] sys___kevent50() at netbsd:sys___kevent50+0x33
[  28.0253983] syscall() at netbsd:syscall+0x23e
[  28.0253983] --- syscall (number 435) ---
[  28.0253983] netbsd:syscall+0x23e:
[  28.0253983] cpu0: End traceback...
[  28.0253983] fatal breakpoint trap in supervisor mode
[  28.0253983] trap type 1 code 0 rip 0x8021f415 cs 0x8 rflags 0x202 
cr2 0x78742459e000 ilevel 0x8 rsp 0xa809281ebb50
[  28.0253983] curlwp 0xdc138aa46540 pid 1352.1352 lowest kstack 
0xa809281e72c0
Stopped in pid 1352.1352 (syslogd) at   netbsd:breakpoint+0x5:  leave

I have a full crash dump if you need any further info.


Module Name:src
Committed By:   jdolecek
Date:   Thu Jan 21 18:09:23 UTC 2021

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

Log Message:
adjust kq_check() (enabled with DEBUG) to new reality - it's now 
perfectly

normal to have kq_count bigger than number of the linked entries
on the kqueue

PR kern/50094, problem pointed out by Chuck Silvers


To generate a diff of this commit:
cvs rdiff -u -r1.111 -r1.112 src/sys/kern/kern_event.c

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


++--+---+
| 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/arch/evbarm/conf

2021-01-21 Thread Jared McNeill
This driver is not 64-bit safe and should not be enabled on aarch64 as-is. 
I think before turning it on we should restrict it to the lower 1GB of 
memory like the Pi3 models where we know it at least has a chance of 
working. You can do this easily by creating a restrictive tag using 
bus_dmatag_subregion(9).


Take care,
Jared

On Thu, 21 Jan 2021, Nia Alarie wrote:


Module Name:src
Committed By:   nia
Date:   Thu Jan 21 17:46:28 UTC 2021

Modified Files:
src/sys/arch/evbarm/conf: GENERIC64

Log Message:
add vcaudio (intentionally this time)

gives working audio output on rpi3 without needing to run a 32-bit image.


To generate a diff of this commit:
cvs rdiff -u -r1.173 -r1.174 src/sys/arch/evbarm/conf/GENERIC64

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




Re: CVS commit: src/sys/kern

2021-01-21 Thread Tom Spindler (moof)
I believe it's this change that's made my vbox image panic at the drop of
a hat; while it occasionally panics before it even hits single user, it
also consistently panics when starting syslogd (even from single-user):

panic: kqueue_scan,1491: kq=0xc779aeff6dc0 kq->kq_count(1) != count(0), 
nmarker=1

and the call chain is syscall -> sys___kevent50 -> kevent1 -> kqueue_check

it also causes a panic while trying to dump more than half the time,
but I have managed to get an image or two. ("panic: atastart: channel 0
busy, xfer not possible", if you're curious.)

On Wed, Jan 20, 2021 at 09:39:09PM +, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Wed Jan 20 21:39:09 UTC 2021
>
> Modified Files:
>   src/sys/kern: kern_event.c
>
> Log Message:
> fix a race in kqueue_scan() - when multiple threads check the same
> kqueue, it could happen other thread seen empty kqueue while kevent
> was being checked for re-firing and re-queued
>
> make sure to keep retrying if there are outstanding kevents even
> if no kevent is found on first pass through the queue, and only
> drop the KN_QUEUED flag and kq_count when actually completely done
> with the kevent
>
> change is inspired by the FreeBSD in-flux handling, but without
> introducing the reference counting
>
> PR kern/50094 by Christof Meerwald
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.110 -r1.111 src/sys/kern/kern_event.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-01-21 Thread Jason Thorpe


> On Jan 21, 2021, at 12:00 AM, Martin Husemann  
> wrote:
> 
> On Wed, Jan 20, 2021 at 07:46:48PM +, Reinoud Zandijk wrote:
>> Module Name: src
>> Committed By:reinoud
>> Date:Wed Jan 20 19:46:48 UTC 2021
>> 
> [..] 
>> Log Message:
>> Add VirtIO PCI v1.0 attachments and fix the drivers affected.
>> 
>> * virtio on sparc64 attaches but is it not functioning though not a
>>  regression.
> 
> While not a regression by this commit, it *did* work in netbsd-8,
> so overall a bad regression that we should fix.

This could be a bug in Qemu … it does not work on Alpha, either, and Jonathan 
Kollasch tracked down to Qemu 5’s Virtio subsystem not considering the DMA 
window on the Alpha platform.

-- thorpej



Re: CVS commit: src/sys/dev

2021-01-21 Thread Martin Husemann
On Wed, Jan 20, 2021 at 07:46:48PM +, Reinoud Zandijk wrote:
> Module Name:  src
> Committed By: reinoud
> Date: Wed Jan 20 19:46:48 UTC 2021
>
[..] 
> Log Message:
> Add VirtIO PCI v1.0 attachments and fix the drivers affected.
> 
> * virtio on sparc64 attaches but is it not functioning though not a
>   regression.

While not a regression by this commit, it *did* work in netbsd-8,
so overall a bad regression that we should fix.

Martin


re: CVS commit: src/sys/arch

2021-01-20 Thread matthew green
"Nia Alarie" writes:
> Module Name:  src
> Committed By: nia
> Date: Wed Jan 20 13:22:08 UTC 2021
>
> Modified Files:
>   src/sys/arch/amd64/conf: GENERIC MODULAR XEN3_DOM0 XEN3_DOMU
>   src/sys/arch/evbarm/conf: OPENBLOCKS_AX3
>   src/sys/arch/i386/conf: GENERIC MODULAR XEN3PAE_DOM0 XEN3PAE_DOMU
>   src/sys/arch/landisk/conf: GENERIC
>   src/sys/arch/usermode/conf: GENERIC.common
>   src/sys/arch/zaurus/conf: GENERIC
>
> Log Message:
> remove compat_ossaudio from kernel modules
>
> this is only useful with compat_linux and gets autoloaded when
> compat_linux is loaded, so there's no reason to bake it into kernels
> any more.

not everyone uses COMPAT_LINUX as a module.  this seems
reasonable to have as a comment anywhere COMAPT_LINUX is
commented.  at least the GENERIC*s should have it, IMO.

additionally, there are a few issues remaining.  eg, the
evbarm/conf/POGO kernel now has a useless "no options"
for COMPAT_OSSAUDIO, and there are at least a handful of
commented versions remaining.


.mrg.


  1   2   3   4   5   6   7   8   9   10   >