Re: CVS commit: src/sys/arch/powerpc/include/booke
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
"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
"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
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
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
"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
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
"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
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
> 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
>> 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
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
>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)
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)
> 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)
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)
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
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
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
> 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)
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
> 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
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
> 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
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
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
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
> 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
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
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
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
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
> 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
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
> 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
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
> 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
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
> 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
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
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
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
> 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
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
"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
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
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)
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)
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
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
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
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
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
> +#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
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)
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)
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
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
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
> 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
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
"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.
