[v2] amd64: simplify TSC sync testing
Hi,
Here is a second draft patch for changing our approach to TSC
synchronization.
With this patch, instead of trying to fix desync with a handshake we
test for desync with a (more) foolproof loop and then don't attempt to
correct for desync if we detect it.
The motivation for a more foolproof loop is to eliminate the false
positive results seen on multisocket CPUs using the current handshake.
The handshake seems to interpret NUMA lag as desync and disables the
TSC on some multisocket systems. Ideally this should not happen.
The motivation for not attempting to fix desync in the kernel with a
per-CPU skew value is, well, I think it's too error-prone. I think
reliably correcting TSC desync in software without the IA32_TSC_ADJUST
register is basically impossible given the speed of the TSC. If it
were a slower clock it would be more feasible, but this is not the
case.
One thing that doesn't work correctly yet is resetting
IA32_TSC_ADJUST. The relevant feature flag is missing during the
first boot. Could we move CPU identification up to an earlier point
in cpu_hatch() so the flag is set when we do the sync test?
--
I asked for review on the actual meat of the previous patch and got
nothing. So I have dumbed the code down to weed out confounding
factors.
I am relatively confident that if this test detects desync that
something is off with your TSC. Not totally confident, because I
haven't gotten any review yet, but I'm getting more confident.
When I forcibly desync the TSC on my APs during boot by 150 cycles
using the IA32_TSC_ADJUST register I get output like this:
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu1: sync test round 1/2 failed
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu1: cpu1: 947 lags 32 cycles
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu2: sync test round 1/2 failed
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu2: cpu2: 1215 lags 30 cycles
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu3: sync test round 1/2 failed
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu3: cpu3: 1085 lags 28 cycles
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu4: sync test round 1/2 failed
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu4: cpu4: 18667 lags 94 cycles
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu5: sync test round 1/2 failed
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu5: cpu5: 771 lags 34 cycles
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu6: sync test round 1/2 failed
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu6: cpu6: 842 lags 30 cycles
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu7: sync test round 1/2 failed
Feb 21 10:05:14 jetsam /bsd: tsc: cpu0/cpu7: cpu7: 895 lags 30 cycles
The CPUs are not running at full speed during boot, so we only measure
~30 cycles of lag when the true lag is 150 cycles. We get ~94 cycles
on CPU4 because it is an SMT twin with CPU0... for some reason this
reduces the margin of error. In general, the margin of error shrinks
with higher clock rates.
Please test! In particular:
- I'd love retests on systems that failed the test using the previous
patch. Almost all of these were AMD Ryzen CPUs. It's hard to say
what the issue is there. My vague guess is a firmware bug.
One would hope that AMD's QA would catch an issue with the #RESET
signal, which is supposed to start all TSCs on all CPUs from zero
simultaneously. I am unsure how you would diagnose that it was,
in fact, a firmware bug though.
- Multisocket systems
- Multiprocessor VMs
Please include your dmesg.
Thanks!
-Scott
Index: amd64/tsc.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/tsc.c,v
retrieving revision 1.24
diff -u -p -r1.24 tsc.c
--- amd64/tsc.c 31 Aug 2021 15:11:54 - 1.24
+++ amd64/tsc.c 23 Feb 2022 02:23:24 -
@@ -36,13 +36,6 @@ int tsc_recalibrate;
uint64_t tsc_frequency;
inttsc_is_invariant;
-#defineTSC_DRIFT_MAX 250
-#define TSC_SKEW_MAX 100
-int64_ttsc_drift_observed;
-
-volatile int64_t tsc_sync_val;
-volatile struct cpu_info *tsc_sync_cpu;
-
u_int tsc_get_timecount(struct timecounter *tc);
void tsc_delay(int usecs);
@@ -236,22 +229,12 @@ cpu_recalibrate_tsc(struct timecounter *
u_int
tsc_get_timecount(struct timecounter *tc)
{
- return rdtsc_lfence() + curcpu()->ci_tsc_skew;
+ return rdtsc_lfence();
}
void
tsc_timecounter_init(struct cpu_info *ci, uint64_t cpufreq)
{
-#ifdef TSC_DEBUG
- printf("%s: TSC skew=%lld observed drift=%lld\n", ci->ci_dev->dv_xname,
- (long long)ci->ci_tsc_skew, (long long)tsc_drift_observed);
-#endif
- if (ci->ci_tsc_skew < -TSC_SKEW_MAX || ci->ci_tsc_skew > TSC_SKEW_MAX) {
- printf("%s: disabling user TSC (skew=%lld)\n",
- ci->ci_dev->dv_xname, (long long)ci->ci_tsc_skew);
- tsc_timecounter.tc_user = 0;
- }
-
if (!(ci->ci_flags & CPUF_PRIMARY) ||
!(ci->ci_flags &
Re: seq: check for asprintf failure
On Wed, 23 Feb 2022 00:04:07 +0100, Theo Buehler wrote: > Hard to hit. Nevertheless it seems better to error out than to > crash in strcmp() Sure. OK millert@ - todd
seq: check for asprintf failure
Hard to hit. Nevertheless it seems better to error out than to
crash in strcmp()
Index: seq.c
===
RCS file: /cvs/src/usr.bin/seq/seq.c,v
retrieving revision 1.3
diff -u -p -r1.3 seq.c
--- seq.c 22 Feb 2022 16:14:38 - 1.3
+++ seq.c 22 Feb 2022 16:23:08 -
@@ -195,8 +195,9 @@ main(int argc, char *argv[])
* loop held true due to a rounding error and we still need to print
* 'last'.
*/
- asprintf(_print, fmt, cur);
- asprintf(_print, fmt, last);
+ if (asprintf(_print, fmt, cur) == -1 ||
+ asprintf(_print, fmt, last) == -1)
+ err(1, "asprintf");
if (strcmp(cur_print, last_print) == 0 && cur != last_shown_value) {
if (cur != first)
fputs(sep, stdout);
Re: dhcpd(8) man page correction : synchronisation
On Tue, Feb 22, 2022 at 08:54:02PM +, Stuart Henderson wrote: > On 2022/02/22 17:45, Jason McIntyre wrote: > > On Tue, Feb 22, 2022 at 04:49:08PM +0100, Matthieu Herrb wrote: > > > Hi, > > > > > > It looks to me that the -Y option is used to specify the destination > > > to which to send synchronisation messages (not where to receive them, > > > this is -y). > > > > > > > hi. > > > > i've never used -Yy so may be wrong, but i think the text is trying to > > say that synctarget receives the messages (so the messages are sent to > > it). i don;t think it's wrong. but, well, i may be wrong ;) > > > > jmc > > > > > Index: dhcpd.8 > > > === > > > RCS file: /cvs/OpenBSD/src/usr.sbin/dhcpd/dhcpd.8,v > > > retrieving revision 1.29 > > > diff -u -p -u -r1.29 dhcpd.8 > > > --- dhcpd.8 29 Aug 2017 08:20:18 - 1.29 > > > +++ dhcpd.8 22 Feb 2022 15:45:20 - > > > @@ -252,7 +252,7 @@ the limited broadcast address (255.255.2 > > > .It Fl Y Ar synctarget > > > Add target > > > .Ar synctarget > > > -to receive synchronisation messages. > > > +to send synchronisation messages. > > > .Ar synctarget > > > can be either an IPv4 address for unicast messages > > > or a network interface name followed optionally by a colon and a numeric > > > TTL > > I understand the existing wording but it is a little awkward. > I don't think the direct s/receive/send/ change helps though. > Maybe this or something like it instead? > > Send synchronisation messages to > .Ar synctarget . > This can be either a destination IPv4 address for unicast messages > or a network interface name followed optionally by a colon and a numeric TTL > [...] > yeah, that probably is clearer. jmc
Re: dhcpd(8) man page correction : synchronisation
On 2022/02/22 17:45, Jason McIntyre wrote: > On Tue, Feb 22, 2022 at 04:49:08PM +0100, Matthieu Herrb wrote: > > Hi, > > > > It looks to me that the -Y option is used to specify the destination > > to which to send synchronisation messages (not where to receive them, > > this is -y). > > > > hi. > > i've never used -Yy so may be wrong, but i think the text is trying to > say that synctarget receives the messages (so the messages are sent to > it). i don;t think it's wrong. but, well, i may be wrong ;) > > jmc > > > Index: dhcpd.8 > > === > > RCS file: /cvs/OpenBSD/src/usr.sbin/dhcpd/dhcpd.8,v > > retrieving revision 1.29 > > diff -u -p -u -r1.29 dhcpd.8 > > --- dhcpd.8 29 Aug 2017 08:20:18 - 1.29 > > +++ dhcpd.8 22 Feb 2022 15:45:20 - > > @@ -252,7 +252,7 @@ the limited broadcast address (255.255.2 > > .It Fl Y Ar synctarget > > Add target > > .Ar synctarget > > -to receive synchronisation messages. > > +to send synchronisation messages. > > .Ar synctarget > > can be either an IPv4 address for unicast messages > > or a network interface name followed optionally by a colon and a numeric > > TTL I understand the existing wording but it is a little awkward. I don't think the direct s/receive/send/ change helps though. Maybe this or something like it instead? Send synchronisation messages to .Ar synctarget . This can be either a destination IPv4 address for unicast messages or a network interface name followed optionally by a colon and a numeric TTL [...]
Re: Fix kernel stack alignment on riscv64
On Tue, Feb 22, 2022 at 07:04:23PM +0100, Mark Kettenis wrote:
> > Date: Tue, 22 Feb 2022 17:45:26 +
> > From: Visa Hankala
> >
> > On Tue, Feb 22, 2022 at 06:13:54PM +0100, Mark Kettenis wrote:
> > > > Date: Tue, 22 Feb 2022 16:59:24 +
> > > > From: Visa Hankala
> > > >
> > > > On Tue, Feb 22, 2022 at 05:31:31PM +0100, Mark Kettenis wrote:
> > > > > > Date: Tue, 22 Feb 2022 15:58:31 +
> > > > > > From: Visa Hankala
> > > > > >
> > > > > > The standard RISC-V calling convention says that the stack pointer
> > > > > > should be 16-byte aligned.
> > > > > >
> > > > > > The patch below corrects the alignment of the kernel stack in
> > > > > > context
> > > > > > switching and exception handling.
> > > > > >
> > > > > > OK?
> > > > >
> > > > > Is there a reason why we don't simply extend struct switchframe to
> > > > > include another register_t such that it is a multiple of 16 bytes?
> > > > >
> > > > > (and then add a compile-time assertion somewhere to make sure we don't
> > > > > mess that up again)
> > > >
> > > > Well, that is another way to do it.
> > > >
> > > > > > The diff reveals that curcpu is stored in an unnamed spot near the
> > > > > > upper
> > > > > > end of u-area when running in user mode. Should struct trapframe
> > > > > > contain
> > > > > > a field where curcpu is stored, so that the usage would become more
> > > > > > apparent? I guess the pointer could also be stored in one of the
> > > > > > existing fields with an appropriate comment in the struct
> > > > > > definition.
> > > > >
> > > > > Sorry, but I don't understand you; curpcu is kept in the tp
> > > > > register...
> > > >
> > > > In kernel, tp holds curcpu. In userland, tp holds TCB pointer. The
> > > > exception entry and exit code has to switch the register's content.
> > > > curcpu is stored in kernel stack when the CPU runs userland.
> > >
> > > Ah, right. So RISC-V doesn't have a special register to hold this. I
> > > don't think adding a field to struct trapframe would make this easier
> > > to understand.
> >
> > Here is a version that shows how a curcpu field would look like,
> > now that a new field is being added anyway.
> >
> > Still not tested...
>
> The weird thing about that is that curcpu doesn't actually live in the
> trapframe. And it would only live at that location for a userland ->
> kernel trapframe. So I think this would make things more confusing...
All right, trapframe is not the right place for it. The existence of
the "variable" should be visible more clearly nevertheless.
So far the variable has occupied the padding slot after the trapframe.
With the new padding field, the slot is gone and the variable would
reside in the final 16 bytes of u-area.
Index: arch/riscv64/include/frame.h
===
RCS file: src/sys/arch/riscv64/include/frame.h,v
retrieving revision 1.2
diff -u -p -r1.2 frame.h
--- arch/riscv64/include/frame.h12 May 2021 01:20:52 - 1.2
+++ arch/riscv64/include/frame.h22 Feb 2022 18:20:46 -
@@ -64,6 +64,7 @@ typedef struct trapframe {
register_t tf_sstatus;
register_t tf_stval;
register_t tf_scause;
+ register_t tf_pad;
} trapframe_t;
/*
@@ -85,6 +86,7 @@ struct sigframe {
struct switchframe {
register_t sf_s[12];
register_t sf_ra;
+ register_t sf_pad;
};
struct callframe {
Index: arch/riscv64/riscv64/vm_machdep.c
===
RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v
retrieving revision 1.8
diff -u -p -r1.8 vm_machdep.c
--- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 - 1.8
+++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 18:20:46 -
@@ -62,6 +62,10 @@ cpu_fork(struct proc *p1, struct proc *p
struct trapframe *tf;
struct switchframe *sf;
+ /* Ensure proper stack alignment. */
+ CTASSERT((sizeof(struct trapframe) & STACKALIGNBYTES) == 0);
+ CTASSERT((sizeof(struct switchframe) & STACKALIGNBYTES) == 0);
+
/* Save FPU state to PCB if necessary. */
if (pcb1->pcb_flags & PCB_FPU)
fpu_save(p1, pcb1->pcb_tf);
@@ -74,6 +78,7 @@ cpu_fork(struct proc *p1, struct proc *p
tf = (struct trapframe *)((u_long)p2->p_addr
+ USPACE
- sizeof(struct trapframe)
+ - sizeof(register_t)/* for holding curcpu */
- 0x10);
tf = (struct trapframe *)STACKALIGN(tf);
Re: Fix kernel stack alignment on riscv64
> Date: Tue, 22 Feb 2022 17:45:26 +
> From: Visa Hankala
>
> On Tue, Feb 22, 2022 at 06:13:54PM +0100, Mark Kettenis wrote:
> > > Date: Tue, 22 Feb 2022 16:59:24 +
> > > From: Visa Hankala
> > >
> > > On Tue, Feb 22, 2022 at 05:31:31PM +0100, Mark Kettenis wrote:
> > > > > Date: Tue, 22 Feb 2022 15:58:31 +
> > > > > From: Visa Hankala
> > > > >
> > > > > The standard RISC-V calling convention says that the stack pointer
> > > > > should be 16-byte aligned.
> > > > >
> > > > > The patch below corrects the alignment of the kernel stack in context
> > > > > switching and exception handling.
> > > > >
> > > > > OK?
> > > >
> > > > Is there a reason why we don't simply extend struct switchframe to
> > > > include another register_t such that it is a multiple of 16 bytes?
> > > >
> > > > (and then add a compile-time assertion somewhere to make sure we don't
> > > > mess that up again)
> > >
> > > Well, that is another way to do it.
> > >
> > > > > The diff reveals that curcpu is stored in an unnamed spot near the
> > > > > upper
> > > > > end of u-area when running in user mode. Should struct trapframe
> > > > > contain
> > > > > a field where curcpu is stored, so that the usage would become more
> > > > > apparent? I guess the pointer could also be stored in one of the
> > > > > existing fields with an appropriate comment in the struct definition.
> > > >
> > > > Sorry, but I don't understand you; curpcu is kept in the tp register...
> > >
> > > In kernel, tp holds curcpu. In userland, tp holds TCB pointer. The
> > > exception entry and exit code has to switch the register's content.
> > > curcpu is stored in kernel stack when the CPU runs userland.
> >
> > Ah, right. So RISC-V doesn't have a special register to hold this. I
> > don't think adding a field to struct trapframe would make this easier
> > to understand.
>
> Here is a version that shows how a curcpu field would look like,
> now that a new field is being added anyway.
>
> Still not tested...
The weird thing about that is that curcpu doesn't actually live in the
trapframe. And it would only live at that location for a userland ->
kernel trapframe. So I think this would make things more confusing...
> Index: arch/riscv64/include/frame.h
> ===
> RCS file: src/sys/arch/riscv64/include/frame.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 frame.h
> --- arch/riscv64/include/frame.h 12 May 2021 01:20:52 - 1.2
> +++ arch/riscv64/include/frame.h 22 Feb 2022 17:40:43 -
> @@ -64,6 +64,8 @@ typedef struct trapframe {
> register_t tf_sstatus;
> register_t tf_stval;
> register_t tf_scause;
> + /* Holds curcpu when the CPU runs in user mode. */
> + register_t tf_curcpu;
> } trapframe_t;
>
> /*
> @@ -85,6 +87,7 @@ struct sigframe {
> struct switchframe {
> register_t sf_s[12];
> register_t sf_ra;
> + register_t sf_pad;
> };
>
> struct callframe {
> Index: arch/riscv64/riscv64/exception.S
> ===
> RCS file: src/sys/arch/riscv64/riscv64/exception.S,v
> retrieving revision 1.5
> diff -u -p -r1.5 exception.S
> --- arch/riscv64/riscv64/exception.S 20 May 2021 04:22:33 - 1.5
> +++ arch/riscv64/riscv64/exception.S 22 Feb 2022 17:40:43 -
> @@ -54,7 +54,7 @@
>
> /* Load our pcpu */
> sd tp, (TF_TP)(sp)
> - ld tp, (TRAPFRAME_SIZEOF)(sp)
> + ld tp, (TF_CURCPU)(sp)
> .endif
>
> sd t0, (TF_T + 0 * 8)(sp)
> @@ -142,7 +142,7 @@
> csrwsscratch, t0
>
> /* Store our pcpu */
> - sd tp, (TRAPFRAME_SIZEOF)(sp)
> + sd tp, (TF_CURCPU)(sp)
> ld tp, (TF_TP)(sp)
>
> /* And restore the user's global pointer */
> Index: arch/riscv64/riscv64/genassym.cf
> ===
> RCS file: src/sys/arch/riscv64/riscv64/genassym.cf,v
> retrieving revision 1.6
> diff -u -p -r1.6 genassym.cf
> --- arch/riscv64/riscv64/genassym.cf 2 Jul 2021 10:42:22 - 1.6
> +++ arch/riscv64/riscv64/genassym.cf 22 Feb 2022 17:40:43 -
> @@ -36,6 +36,7 @@ member tf_sepc
> member tf_sstatus
> member tf_stval
> member tf_scause
> +member tf_curcpu
>
> struct switchframe
> member sf_s
> Index: arch/riscv64/riscv64/vm_machdep.c
> ===
> RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 vm_machdep.c
> --- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 - 1.8
> +++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 17:40:43 -
> @@ -62,6 +62,10 @@ cpu_fork(struct proc *p1, struct proc *p
> struct trapframe *tf;
> struct switchframe *sf;
>
> + /* Ensure proper stack alignment. */
> + CTASSERT((sizeof(struct trapframe)
Re: Fix kernel stack alignment on riscv64
On Tue, Feb 22, 2022 at 06:13:54PM +0100, Mark Kettenis wrote:
> > Date: Tue, 22 Feb 2022 16:59:24 +
> > From: Visa Hankala
> >
> > On Tue, Feb 22, 2022 at 05:31:31PM +0100, Mark Kettenis wrote:
> > > > Date: Tue, 22 Feb 2022 15:58:31 +
> > > > From: Visa Hankala
> > > >
> > > > The standard RISC-V calling convention says that the stack pointer
> > > > should be 16-byte aligned.
> > > >
> > > > The patch below corrects the alignment of the kernel stack in context
> > > > switching and exception handling.
> > > >
> > > > OK?
> > >
> > > Is there a reason why we don't simply extend struct switchframe to
> > > include another register_t such that it is a multiple of 16 bytes?
> > >
> > > (and then add a compile-time assertion somewhere to make sure we don't
> > > mess that up again)
> >
> > Well, that is another way to do it.
> >
> > > > The diff reveals that curcpu is stored in an unnamed spot near the upper
> > > > end of u-area when running in user mode. Should struct trapframe contain
> > > > a field where curcpu is stored, so that the usage would become more
> > > > apparent? I guess the pointer could also be stored in one of the
> > > > existing fields with an appropriate comment in the struct definition.
> > >
> > > Sorry, but I don't understand you; curpcu is kept in the tp register...
> >
> > In kernel, tp holds curcpu. In userland, tp holds TCB pointer. The
> > exception entry and exit code has to switch the register's content.
> > curcpu is stored in kernel stack when the CPU runs userland.
>
> Ah, right. So RISC-V doesn't have a special register to hold this. I
> don't think adding a field to struct trapframe would make this easier
> to understand.
Here is a version that shows how a curcpu field would look like,
now that a new field is being added anyway.
Still not tested...
Index: arch/riscv64/include/frame.h
===
RCS file: src/sys/arch/riscv64/include/frame.h,v
retrieving revision 1.2
diff -u -p -r1.2 frame.h
--- arch/riscv64/include/frame.h12 May 2021 01:20:52 - 1.2
+++ arch/riscv64/include/frame.h22 Feb 2022 17:40:43 -
@@ -64,6 +64,8 @@ typedef struct trapframe {
register_t tf_sstatus;
register_t tf_stval;
register_t tf_scause;
+ /* Holds curcpu when the CPU runs in user mode. */
+ register_t tf_curcpu;
} trapframe_t;
/*
@@ -85,6 +87,7 @@ struct sigframe {
struct switchframe {
register_t sf_s[12];
register_t sf_ra;
+ register_t sf_pad;
};
struct callframe {
Index: arch/riscv64/riscv64/exception.S
===
RCS file: src/sys/arch/riscv64/riscv64/exception.S,v
retrieving revision 1.5
diff -u -p -r1.5 exception.S
--- arch/riscv64/riscv64/exception.S20 May 2021 04:22:33 - 1.5
+++ arch/riscv64/riscv64/exception.S22 Feb 2022 17:40:43 -
@@ -54,7 +54,7 @@
/* Load our pcpu */
sd tp, (TF_TP)(sp)
- ld tp, (TRAPFRAME_SIZEOF)(sp)
+ ld tp, (TF_CURCPU)(sp)
.endif
sd t0, (TF_T + 0 * 8)(sp)
@@ -142,7 +142,7 @@
csrwsscratch, t0
/* Store our pcpu */
- sd tp, (TRAPFRAME_SIZEOF)(sp)
+ sd tp, (TF_CURCPU)(sp)
ld tp, (TF_TP)(sp)
/* And restore the user's global pointer */
Index: arch/riscv64/riscv64/genassym.cf
===
RCS file: src/sys/arch/riscv64/riscv64/genassym.cf,v
retrieving revision 1.6
diff -u -p -r1.6 genassym.cf
--- arch/riscv64/riscv64/genassym.cf2 Jul 2021 10:42:22 - 1.6
+++ arch/riscv64/riscv64/genassym.cf22 Feb 2022 17:40:43 -
@@ -36,6 +36,7 @@ membertf_sepc
member tf_sstatus
member tf_stval
member tf_scause
+member tf_curcpu
struct switchframe
member sf_s
Index: arch/riscv64/riscv64/vm_machdep.c
===
RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v
retrieving revision 1.8
diff -u -p -r1.8 vm_machdep.c
--- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 - 1.8
+++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 17:40:43 -
@@ -62,6 +62,10 @@ cpu_fork(struct proc *p1, struct proc *p
struct trapframe *tf;
struct switchframe *sf;
+ /* Ensure proper stack alignment. */
+ CTASSERT((sizeof(struct trapframe) & STACKALIGNBYTES) == 0);
+ CTASSERT((sizeof(struct switchframe) & STACKALIGNBYTES) == 0);
+
/* Save FPU state to PCB if necessary. */
if (pcb1->pcb_flags & PCB_FPU)
fpu_save(p1, pcb1->pcb_tf);
Re: dhcpd(8) man page correction : synchronisation
On Tue, Feb 22, 2022 at 04:49:08PM +0100, Matthieu Herrb wrote: > Hi, > > It looks to me that the -Y option is used to specify the destination > to which to send synchronisation messages (not where to receive them, > this is -y). > hi. i've never used -Yy so may be wrong, but i think the text is trying to say that synctarget receives the messages (so the messages are sent to it). i don;t think it's wrong. but, well, i may be wrong ;) jmc > Index: dhcpd.8 > === > RCS file: /cvs/OpenBSD/src/usr.sbin/dhcpd/dhcpd.8,v > retrieving revision 1.29 > diff -u -p -u -r1.29 dhcpd.8 > --- dhcpd.8 29 Aug 2017 08:20:18 - 1.29 > +++ dhcpd.8 22 Feb 2022 15:45:20 - > @@ -252,7 +252,7 @@ the limited broadcast address (255.255.2 > .It Fl Y Ar synctarget > Add target > .Ar synctarget > -to receive synchronisation messages. > +to send synchronisation messages. > .Ar synctarget > can be either an IPv4 address for unicast messages > or a network interface name followed optionally by a colon and a numeric TTL > > -- > Matthieu Herrb >
Re: bgpd convert parse.y to uintXY_t
On Tue, Feb 22, 2022 at 06:00:34PM +0100, Claudio Jeker wrote:
> In the big conversion I forgot to include parse.y in the files.
> This diff fixes that.
Seems your sed missed a 'g' after the pattern :)
I believe all the variables you missed are visible below (mentioned
inline), with those fixed
ok tb
>
> --
> :wq Claudio
>
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.420
> diff -u -p -r1.420 parse.y
> --- parse.y 15 Oct 2021 15:01:27 - 1.420
> +++ parse.y 22 Feb 2022 16:57:03 -
> @@ -161,10 +161,10 @@ int parsecommunity(struct community *,
> int parseextcommunity(struct community *, char *,
> char *);
> static intnew_as_set(char *);
> -static void add_as_set(u_int32_t);
> +static void add_as_set(uint32_t);
> static void done_as_set(void);
> static struct prefixset *new_prefix_set(char *, int);
> -static void add_roa_set(struct prefixset_item *, u_int32_t, u_int8_t,
> +static void add_roa_set(struct prefixset_item *, uint32_t, uint8_t,
> time_t);
> static struct rtr_config *get_rtr(struct bgpd_addr *);
> static intinsert_rtr(struct rtr_config *);
> @@ -174,7 +174,7 @@ typedef struct {
> long longnumber;
> char*string;
> struct bgpd_addr addr;
> - u_int8_t u8;
> + uint8_t u8;
> struct filter_rib_l *filter_rib;
> struct filter_peers_l *filter_peers;
> struct filter_match_lfilter_match;
> @@ -185,14 +185,14 @@ typedef struct {
> struct filter_set_head *filter_set_head;
> struct {
> struct bgpd_addrprefix;
> - u_int8_tlen;
> + uint8_t len;
> } prefix;
> struct filter_prefixlen prefixlen;
> struct prefixset_item *prefixset_item;
> struct {
> - u_int8_tenc_alg;
> + uint8_t enc_alg;
> + uint8_t enc_key_len;
> charenc_key[IPSEC_ENC_KEY_LEN];
> - u_int8_tenc_key_len;
> } encspec;
> } v;
> int lineno;
> @@ -283,7 +283,7 @@ asnumber : NUMBER{
> as4number: STRING{
> const char *errstr;
> char*dot;
> - u_int32_tuvalh = 0, uval;
> + uint32_t uvalh = 0, uval;
>
> if ((dot = strchr($1,'.')) != NULL) {
> *dot++ = '\0';
> @@ -315,7 +315,7 @@ as4number : STRING{
> | asnumber {
> if ($1 == AS_TRANS || $1 == 0) {
> yyerror("AS %u is reserved and may not be used",
> - (u_int32_t)$1);
> + (uint32_t)$1);
> YYERROR;
> }
> $$ = $1;
> @@ -325,7 +325,7 @@ as4number : STRING{
> as4number_any: STRING{
> const char *errstr;
> char*dot;
> - u_int32_tuvalh = 0, uval;
> + uint32_t uvalh = 0, uval;
>
> if ((dot = strchr($1,'.')) != NULL) {
> *dot++ = '\0';
> @@ -1063,7 +1063,7 @@ restricted : RESTRICTED{ $$ = 1; }
> ;
>
> address : STRING{
> - u_int8_tlen;
> + uint8_t len;
>
> if (!host($1, &$$, )) {
> yyerror("could not parse address spec \"%s\"",
> @@ -1439,8 +1439,8 @@ peeropts: REMOTEAS as4number{
> curpeer->conf.min_holdtime = $3;
> }
> | ANNOUNCE family safi {
> - u_int8_taid, safi;
> - u_int16_t afi;
> + uint8_t aid, safi;
> + uint16_tafi;
>
> if ($3 == SAFI_NONE) {
> for (aid = 0; aid < AID_MAX; aid++) {
> @@ -1474,7 +1474,7 @@ peeropts: REMOTEAS as4number{
> }
> | ANNOUNCE ADDPATH RECV yesno {
> int8_t *ap = curpeer->conf.capabilities.add_path;
> - u_int8_t
Re: Fix kernel stack alignment on riscv64
> Date: Tue, 22 Feb 2022 16:59:24 +
> From: Visa Hankala
>
> On Tue, Feb 22, 2022 at 05:31:31PM +0100, Mark Kettenis wrote:
> > > Date: Tue, 22 Feb 2022 15:58:31 +
> > > From: Visa Hankala
> > >
> > > The standard RISC-V calling convention says that the stack pointer
> > > should be 16-byte aligned.
> > >
> > > The patch below corrects the alignment of the kernel stack in context
> > > switching and exception handling.
> > >
> > > OK?
> >
> > Is there a reason why we don't simply extend struct switchframe to
> > include another register_t such that it is a multiple of 16 bytes?
> >
> > (and then add a compile-time assertion somewhere to make sure we don't
> > mess that up again)
>
> Well, that is another way to do it.
>
> > > The diff reveals that curcpu is stored in an unnamed spot near the upper
> > > end of u-area when running in user mode. Should struct trapframe contain
> > > a field where curcpu is stored, so that the usage would become more
> > > apparent? I guess the pointer could also be stored in one of the
> > > existing fields with an appropriate comment in the struct definition.
> >
> > Sorry, but I don't understand you; curpcu is kept in the tp register...
>
> In kernel, tp holds curcpu. In userland, tp holds TCB pointer. The
> exception entry and exit code has to switch the register's content.
> curcpu is stored in kernel stack when the CPU runs userland.
Ah, right. So RISC-V doesn't have a special register to hold this. I
don't think adding a field to struct trapframe would make this easier
to understand.
>
> I have not tested the following patch yet. It takes a while
> to compile...
I think this looks much better.
> Index: arch/riscv64/include/frame.h
> ===
> RCS file: src/sys/arch/riscv64/include/frame.h,v
> retrieving revision 1.2
> diff -u -p -r1.2 frame.h
> --- arch/riscv64/include/frame.h 12 May 2021 01:20:52 - 1.2
> +++ arch/riscv64/include/frame.h 22 Feb 2022 16:50:53 -
> @@ -64,6 +64,7 @@ typedef struct trapframe {
> register_t tf_sstatus;
> register_t tf_stval;
> register_t tf_scause;
> + register_t tf_pad;
> } trapframe_t;
>
> /*
> @@ -85,6 +86,7 @@ struct sigframe {
> struct switchframe {
> register_t sf_s[12];
> register_t sf_ra;
> + register_t sf_pad;
> };
>
> struct callframe {
> Index: arch/riscv64/riscv64/vm_machdep.c
> ===
> RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 vm_machdep.c
> --- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 - 1.8
> +++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 16:50:53 -
> @@ -62,6 +62,10 @@ cpu_fork(struct proc *p1, struct proc *p
> struct trapframe *tf;
> struct switchframe *sf;
>
> + /* Ensure proper stack alignment. */
> + CTASSERT((sizeof(struct trapframe) & STACKALIGNBYTES) == 0);
> + CTASSERT((sizeof(struct switchframe) & STACKALIGNBYTES) == 0);
> +
> /* Save FPU state to PCB if necessary. */
> if (pcb1->pcb_flags & PCB_FPU)
> fpu_save(p1, pcb1->pcb_tf);
>
bgpd convert parse.y to uintXY_t
In the big conversion I forgot to include parse.y in the files.
This diff fixes that.
--
:wq Claudio
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.420
diff -u -p -r1.420 parse.y
--- parse.y 15 Oct 2021 15:01:27 - 1.420
+++ parse.y 22 Feb 2022 16:57:03 -
@@ -161,10 +161,10 @@ intparsecommunity(struct community *,
int parseextcommunity(struct community *, char *,
char *);
static int new_as_set(char *);
-static void add_as_set(u_int32_t);
+static void add_as_set(uint32_t);
static void done_as_set(void);
static struct prefixset*new_prefix_set(char *, int);
-static void add_roa_set(struct prefixset_item *, u_int32_t, u_int8_t,
+static void add_roa_set(struct prefixset_item *, uint32_t, uint8_t,
time_t);
static struct rtr_config *get_rtr(struct bgpd_addr *);
static int insert_rtr(struct rtr_config *);
@@ -174,7 +174,7 @@ typedef struct {
long longnumber;
char*string;
struct bgpd_addr addr;
- u_int8_t u8;
+ uint8_t u8;
struct filter_rib_l *filter_rib;
struct filter_peers_l *filter_peers;
struct filter_match_lfilter_match;
@@ -185,14 +185,14 @@ typedef struct {
struct filter_set_head *filter_set_head;
struct {
struct bgpd_addrprefix;
- u_int8_tlen;
+ uint8_t len;
} prefix;
struct filter_prefixlen prefixlen;
struct prefixset_item *prefixset_item;
struct {
- u_int8_tenc_alg;
+ uint8_t enc_alg;
+ uint8_t enc_key_len;
charenc_key[IPSEC_ENC_KEY_LEN];
- u_int8_tenc_key_len;
} encspec;
} v;
int lineno;
@@ -283,7 +283,7 @@ asnumber: NUMBER{
as4number : STRING{
const char *errstr;
char*dot;
- u_int32_tuvalh = 0, uval;
+ uint32_t uvalh = 0, uval;
if ((dot = strchr($1,'.')) != NULL) {
*dot++ = '\0';
@@ -315,7 +315,7 @@ as4number : STRING{
| asnumber {
if ($1 == AS_TRANS || $1 == 0) {
yyerror("AS %u is reserved and may not be used",
- (u_int32_t)$1);
+ (uint32_t)$1);
YYERROR;
}
$$ = $1;
@@ -325,7 +325,7 @@ as4number : STRING{
as4number_any : STRING{
const char *errstr;
char*dot;
- u_int32_tuvalh = 0, uval;
+ uint32_t uvalh = 0, uval;
if ((dot = strchr($1,'.')) != NULL) {
*dot++ = '\0';
@@ -1063,7 +1063,7 @@ restricted: RESTRICTED{ $$ = 1; }
;
address: STRING{
- u_int8_tlen;
+ uint8_t len;
if (!host($1, &$$, )) {
yyerror("could not parse address spec \"%s\"",
@@ -1439,8 +1439,8 @@ peeropts : REMOTEAS as4number{
curpeer->conf.min_holdtime = $3;
}
| ANNOUNCE family safi {
- u_int8_taid, safi;
- u_int16_t afi;
+ uint8_t aid, safi;
+ uint16_tafi;
if ($3 == SAFI_NONE) {
for (aid = 0; aid < AID_MAX; aid++) {
@@ -1474,7 +1474,7 @@ peeropts : REMOTEAS as4number{
}
| ANNOUNCE ADDPATH RECV yesno {
int8_t *ap = curpeer->conf.capabilities.add_path;
- u_int8_t i;
+ uint8_t i;
for (i = 0; i < AID_MAX; i++)
if ($4)
@@ -1576,8 +1576,8 @@ peeropts : REMOTEAS as4number{
curpeer->conf.auth.method = AUTH_IPSEC_IKE_AH;
Re: Fix kernel stack alignment on riscv64
On Tue, Feb 22, 2022 at 05:31:31PM +0100, Mark Kettenis wrote:
> > Date: Tue, 22 Feb 2022 15:58:31 +
> > From: Visa Hankala
> >
> > The standard RISC-V calling convention says that the stack pointer
> > should be 16-byte aligned.
> >
> > The patch below corrects the alignment of the kernel stack in context
> > switching and exception handling.
> >
> > OK?
>
> Is there a reason why we don't simply extend struct switchframe to
> include another register_t such that it is a multiple of 16 bytes?
>
> (and then add a compile-time assertion somewhere to make sure we don't
> mess that up again)
Well, that is another way to do it.
> > The diff reveals that curcpu is stored in an unnamed spot near the upper
> > end of u-area when running in user mode. Should struct trapframe contain
> > a field where curcpu is stored, so that the usage would become more
> > apparent? I guess the pointer could also be stored in one of the
> > existing fields with an appropriate comment in the struct definition.
>
> Sorry, but I don't understand you; curpcu is kept in the tp register...
In kernel, tp holds curcpu. In userland, tp holds TCB pointer. The
exception entry and exit code has to switch the register's content.
curcpu is stored in kernel stack when the CPU runs userland.
I have not tested the following patch yet. It takes a while
to compile...
Index: arch/riscv64/include/frame.h
===
RCS file: src/sys/arch/riscv64/include/frame.h,v
retrieving revision 1.2
diff -u -p -r1.2 frame.h
--- arch/riscv64/include/frame.h12 May 2021 01:20:52 - 1.2
+++ arch/riscv64/include/frame.h22 Feb 2022 16:50:53 -
@@ -64,6 +64,7 @@ typedef struct trapframe {
register_t tf_sstatus;
register_t tf_stval;
register_t tf_scause;
+ register_t tf_pad;
} trapframe_t;
/*
@@ -85,6 +86,7 @@ struct sigframe {
struct switchframe {
register_t sf_s[12];
register_t sf_ra;
+ register_t sf_pad;
};
struct callframe {
Index: arch/riscv64/riscv64/vm_machdep.c
===
RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v
retrieving revision 1.8
diff -u -p -r1.8 vm_machdep.c
--- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 - 1.8
+++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 16:50:53 -
@@ -62,6 +62,10 @@ cpu_fork(struct proc *p1, struct proc *p
struct trapframe *tf;
struct switchframe *sf;
+ /* Ensure proper stack alignment. */
+ CTASSERT((sizeof(struct trapframe) & STACKALIGNBYTES) == 0);
+ CTASSERT((sizeof(struct switchframe) & STACKALIGNBYTES) == 0);
+
/* Save FPU state to PCB if necessary. */
if (pcb1->pcb_flags & PCB_FPU)
fpu_save(p1, pcb1->pcb_tf);
allow bgpd to listen and connect to non common ports
Sometimes (mainly for tests) it can be useful to run bgpd on something
different than port 179. The following diff does mostly that. It allows
to define a port with 'listen on' and makes it possible to set the port
on a neighbor like it is done for rtr sessions.
The only thing not working are IPsec flows. Those assume that the default
port is used and changing that is way to complex for the limited usecase.
--
:wq Claudio
Index: bgpd.conf.5
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
retrieving revision 1.216
diff -u -p -r1.216 bgpd.conf.5
--- bgpd.conf.5 22 Feb 2022 12:08:22 - 1.216
+++ bgpd.conf.5 22 Feb 2022 16:30:59 -
@@ -237,8 +237,8 @@ The default is 90 seconds.
The minimum acceptable holdtime in seconds.
This value must be at least 3.
.Pp
-.It Ic listen on Ar address
-Specify the local IP address for
+.It Ic listen on Ar address Op Ic port Ar port
+Specify the local IP address and optional port for
.Xr bgpd 8
to listen on.
The default is to listen on all local addresses on the current default
@@ -1078,6 +1078,9 @@ aes-128-cbc
.Pp
Keys must be given in hexadecimal format.
After changing settings a session needs to be reset to use the new keys.
+The
+.Ic ipsec
+flows only work with session using the default port 179.
.Pp
.It Xo
.Ic ipsec
@@ -1113,6 +1116,9 @@ and
.Xr bgpd 8
daemons on both sides, the session should be established.
After changing settings a session needs to be reset to use the new keys.
+The
+.Ic ipsec
+flows only work with session using the default port 179.
.Pp
.It Ic local-address Ar address
.It Ic no local-address
@@ -1183,6 +1189,11 @@ statement defines the maximum hops the n
.Pp
.It Ic passive
Do not attempt to actively open a TCP connection to the neighbor system.
+.Pp
+.It Ic port Ar port
+Connect to the peer using
+.Ar port
+instead of the default BGP port 179.
.Pp
.It Xo
.Ic reject Ic as-set
Index: bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.419
diff -u -p -r1.419 bgpd.h
--- bgpd.h 6 Feb 2022 09:51:19 - 1.419
+++ bgpd.h 22 Feb 2022 15:11:13 -
@@ -36,6 +36,7 @@
#defineBGP_VERSION 4
#defineBGP_PORT179
+#defineRTR_PORT323
#defineCONFFILE"/etc/bgpd.conf"
#defineBGPD_USER "_bgpd"
#definePEER_DESCR_LEN 32
@@ -402,6 +403,7 @@ struct peer_config {
uint16_t holdtime;
uint16_t min_holdtime;
uint16_t local_short_as;
+ uint16_t remote_port;
uint8_t template;
uint8_t remote_masklen;
uint8_t ebgp; /* 0 = ibgp else ebgp */
Index: config.c
===
RCS file: /cvs/src/usr.sbin/bgpd/config.c,v
retrieving revision 1.100
diff -u -p -r1.100 config.c
--- config.c6 Feb 2022 09:51:19 - 1.100
+++ config.c22 Feb 2022 11:00:43 -
@@ -447,30 +447,6 @@ prepare_listeners(struct bgpd_config *co
int opt = 1;
int r = 0;
- if (TAILQ_EMPTY(conf->listen_addrs)) {
- if ((la = calloc(1, sizeof(struct listen_addr))) == NULL)
- fatal("setup_listeners calloc");
- la->fd = -1;
- la->flags = DEFAULT_LISTENER;
- la->reconf = RECONF_REINIT;
- la->sa_len = sizeof(struct sockaddr_in);
- ((struct sockaddr_in *)>sa)->sin_family = AF_INET;
- ((struct sockaddr_in *)>sa)->sin_addr.s_addr =
- htonl(INADDR_ANY);
- ((struct sockaddr_in *)>sa)->sin_port = htons(BGP_PORT);
- TAILQ_INSERT_TAIL(conf->listen_addrs, la, entry);
-
- if ((la = calloc(1, sizeof(struct listen_addr))) == NULL)
- fatal("setup_listeners calloc");
- la->fd = -1;
- la->flags = DEFAULT_LISTENER;
- la->reconf = RECONF_REINIT;
- la->sa_len = sizeof(struct sockaddr_in6);
- ((struct sockaddr_in6 *)>sa)->sin6_family = AF_INET6;
- ((struct sockaddr_in6 *)>sa)->sin6_port = htons(BGP_PORT);
- TAILQ_INSERT_TAIL(conf->listen_addrs, la, entry);
- }
-
for (la = TAILQ_FIRST(conf->listen_addrs); la != NULL; la = next) {
next = TAILQ_NEXT(la, entry);
if (la->reconf != RECONF_REINIT)
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.420
diff -u -p -r1.420 parse.y
--- parse.y 15 Oct 2021 15:01:27 -
Re: ifconfig(8): always print the mtu, don't hide it on "bridges"
On Mon, Feb 21, 2022 at 9:47 PM David Gwynne wrote: > this lets ifconfig show the MTU on interfaces like nvgre, vxlan, etc. > they currently don't show it because they also implement a bridge ioctl, > so ifconfig thinks they're a bridge. > > why ifconfig hides the mtu on bridges looks to be a hold over from when > brconfig was merged into ifconfig. if we dont want bridge(4) to report > an mtu, then i can make bridge(4) itself hide the mtu or stop setting > the mtu. > > found by jason tubnor. > > ok? Ah, this test explains one of the three times ioctl(SIOCBRDGRTS) is used on each interface. This will drop it to twice per interface ;) ok guenther@
Re: Fix kernel stack alignment on riscv64
> Date: Tue, 22 Feb 2022 15:58:31 + > From: Visa Hankala > > The standard RISC-V calling convention says that the stack pointer > should be 16-byte aligned. > > The patch below corrects the alignment of the kernel stack in context > switching and exception handling. > > OK? Is there a reason why we don't simply extend struct switchframe to include another register_t such that it is a multiple of 16 bytes? (and then add a compile-time assertion somewhere to make sure we don't mess that up again) > The diff reveals that curcpu is stored in an unnamed spot near the upper > end of u-area when running in user mode. Should struct trapframe contain > a field where curcpu is stored, so that the usage would become more > apparent? I guess the pointer could also be stored in one of the > existing fields with an appropriate comment in the struct definition. Sorry, but I don't understand you; curpcu is kept in the tp register... > Index: arch/riscv64/include/param.h > === > RCS file: src/sys/arch/riscv64/include/param.h,v > retrieving revision 1.4 > diff -u -p -r1.4 param.h > --- arch/riscv64/include/param.h 16 Jun 2021 12:00:15 - 1.4 > +++ arch/riscv64/include/param.h 22 Feb 2022 15:29:05 - > @@ -75,6 +75,7 @@ > > #define STACKALIGNBYTES (16 - 1) > #define STACKALIGN(p) ((u_long)(p) &~ STACKALIGNBYTES) > +#define FRAMESZ(sz) (((sz) + STACKALIGNBYTES) & > ~STACKALIGNBYTES) > > #define __HAVE_FDT > > Index: arch/riscv64/riscv64/cpuswitch.S > === > RCS file: src/sys/arch/riscv64/riscv64/cpuswitch.S,v > retrieving revision 1.6 > diff -u -p -r1.6 cpuswitch.S > --- arch/riscv64/riscv64/cpuswitch.S 22 Feb 2022 07:47:46 - 1.6 > +++ arch/riscv64/riscv64/cpuswitch.S 22 Feb 2022 15:29:05 - > @@ -17,8 +17,9 @@ > * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > */ > > -#include "machine/asm.h" > #include "assym.h" > +#include > +#include > > /* > * cpu_switchto(struct proc *oldproc, struct proc *newproc) > @@ -30,7 +31,7 @@ ENTRY(cpu_switchto_asm) > beqza0, 1f > > // create switchframe > - addisp, sp, -SWITCHFRAME_SIZEOF > + addisp, sp, -FRAMESZ(SWITCHFRAME_SIZEOF) > sd s0, (SF_S + 0 * 8)(sp) > sd s1, (SF_S + 1 * 8)(sp) > sd s2, (SF_S + 2 * 8)(sp) > @@ -86,7 +87,7 @@ ENTRY(cpu_switchto_asm) > ld ra, SF_RA(sp) > > RETGUARD_CALC_COOKIE(a7) > - addisp, sp, SWITCHFRAME_SIZEOF > + addisp, sp, FRAMESZ(SWITCHFRAME_SIZEOF) > RETGUARD_CHECK(cpu_switchto, a7) > ret > END(cpu_switch_asm) > Index: arch/riscv64/riscv64/exception.S > === > RCS file: src/sys/arch/riscv64/riscv64/exception.S,v > retrieving revision 1.5 > diff -u -p -r1.5 exception.S > --- arch/riscv64/riscv64/exception.S 20 May 2021 04:22:33 - 1.5 > +++ arch/riscv64/riscv64/exception.S 22 Feb 2022 15:29:05 - > @@ -36,11 +36,12 @@ > > #include "assym.h" > #include > +#include > #include > #include > > .macro save_registers mode > - addisp, sp, -(TRAPFRAME_SIZEOF) > + addisp, sp, -FRAMESZ(TRAPFRAME_SIZEOF) > > sd ra, (TF_RA)(sp) > > @@ -54,7 +55,7 @@ > > /* Load our pcpu */ > sd tp, (TF_TP)(sp) > - ld tp, (TRAPFRAME_SIZEOF)(sp) > + ld tp, (FRAMESZ(TRAPFRAME_SIZEOF))(sp) > .endif > > sd t0, (TF_T + 0 * 8)(sp) > @@ -89,7 +90,7 @@ > > .if \mode == 1 > /* Store kernel sp */ > - li t1, TRAPFRAME_SIZEOF > + li t1, FRAMESZ(TRAPFRAME_SIZEOF) > add t0, sp, t1 > sd t0, (TF_SP)(sp) > .else > @@ -142,7 +143,7 @@ > csrwsscratch, t0 > > /* Store our pcpu */ > - sd tp, (TRAPFRAME_SIZEOF)(sp) > + sd tp, (FRAMESZ(TRAPFRAME_SIZEOF))(sp) > ld tp, (TF_TP)(sp) > > /* And restore the user's global pointer */ > @@ -181,7 +182,7 @@ > ld a6, (TF_A + 6 * 8)(sp) > ld a7, (TF_A + 7 * 8)(sp) > > - addisp, sp, (TRAPFRAME_SIZEOF) > + addisp, sp, FRAMESZ(TRAPFRAME_SIZEOF) > .endm > > .macro do_ast > Index: arch/riscv64/riscv64/vm_machdep.c > === > RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v > retrieving revision 1.8 > diff -u -p -r1.8 vm_machdep.c > --- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 - 1.8 > +++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 15:29:05 - > @@ -91,7 +91,8 @@ cpu_fork(struct proc *p1, struct proc *p > tf->tf_sstatus |= (SSTATUS_SPIE); /* Enable interrupts. */ > tf->tf_sstatus &= ~(SSTATUS_SPP); /* Enter user mode. */ > > - sf = (struct switchframe *)tf - 1; > + sf = (struct
Fix kernel stack alignment on riscv64
The standard RISC-V calling convention says that the stack pointer should be 16-byte aligned. The patch below corrects the alignment of the kernel stack in context switching and exception handling. OK? The diff reveals that curcpu is stored in an unnamed spot near the upper end of u-area when running in user mode. Should struct trapframe contain a field where curcpu is stored, so that the usage would become more apparent? I guess the pointer could also be stored in one of the existing fields with an appropriate comment in the struct definition. Index: arch/riscv64/include/param.h === RCS file: src/sys/arch/riscv64/include/param.h,v retrieving revision 1.4 diff -u -p -r1.4 param.h --- arch/riscv64/include/param.h16 Jun 2021 12:00:15 - 1.4 +++ arch/riscv64/include/param.h22 Feb 2022 15:29:05 - @@ -75,6 +75,7 @@ #defineSTACKALIGNBYTES (16 - 1) #defineSTACKALIGN(p) ((u_long)(p) &~ STACKALIGNBYTES) +#defineFRAMESZ(sz) (((sz) + STACKALIGNBYTES) & ~STACKALIGNBYTES) #define __HAVE_FDT Index: arch/riscv64/riscv64/cpuswitch.S === RCS file: src/sys/arch/riscv64/riscv64/cpuswitch.S,v retrieving revision 1.6 diff -u -p -r1.6 cpuswitch.S --- arch/riscv64/riscv64/cpuswitch.S22 Feb 2022 07:47:46 - 1.6 +++ arch/riscv64/riscv64/cpuswitch.S22 Feb 2022 15:29:05 - @@ -17,8 +17,9 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include "machine/asm.h" #include "assym.h" +#include +#include /* * cpu_switchto(struct proc *oldproc, struct proc *newproc) @@ -30,7 +31,7 @@ ENTRY(cpu_switchto_asm) beqza0, 1f // create switchframe - addisp, sp, -SWITCHFRAME_SIZEOF + addisp, sp, -FRAMESZ(SWITCHFRAME_SIZEOF) sd s0, (SF_S + 0 * 8)(sp) sd s1, (SF_S + 1 * 8)(sp) sd s2, (SF_S + 2 * 8)(sp) @@ -86,7 +87,7 @@ ENTRY(cpu_switchto_asm) ld ra, SF_RA(sp) RETGUARD_CALC_COOKIE(a7) - addisp, sp, SWITCHFRAME_SIZEOF + addisp, sp, FRAMESZ(SWITCHFRAME_SIZEOF) RETGUARD_CHECK(cpu_switchto, a7) ret END(cpu_switch_asm) Index: arch/riscv64/riscv64/exception.S === RCS file: src/sys/arch/riscv64/riscv64/exception.S,v retrieving revision 1.5 diff -u -p -r1.5 exception.S --- arch/riscv64/riscv64/exception.S20 May 2021 04:22:33 - 1.5 +++ arch/riscv64/riscv64/exception.S22 Feb 2022 15:29:05 - @@ -36,11 +36,12 @@ #include "assym.h" #include +#include #include #include .macro save_registers mode - addisp, sp, -(TRAPFRAME_SIZEOF) + addisp, sp, -FRAMESZ(TRAPFRAME_SIZEOF) sd ra, (TF_RA)(sp) @@ -54,7 +55,7 @@ /* Load our pcpu */ sd tp, (TF_TP)(sp) - ld tp, (TRAPFRAME_SIZEOF)(sp) + ld tp, (FRAMESZ(TRAPFRAME_SIZEOF))(sp) .endif sd t0, (TF_T + 0 * 8)(sp) @@ -89,7 +90,7 @@ .if \mode == 1 /* Store kernel sp */ - li t1, TRAPFRAME_SIZEOF + li t1, FRAMESZ(TRAPFRAME_SIZEOF) add t0, sp, t1 sd t0, (TF_SP)(sp) .else @@ -142,7 +143,7 @@ csrwsscratch, t0 /* Store our pcpu */ - sd tp, (TRAPFRAME_SIZEOF)(sp) + sd tp, (FRAMESZ(TRAPFRAME_SIZEOF))(sp) ld tp, (TF_TP)(sp) /* And restore the user's global pointer */ @@ -181,7 +182,7 @@ ld a6, (TF_A + 6 * 8)(sp) ld a7, (TF_A + 7 * 8)(sp) - addisp, sp, (TRAPFRAME_SIZEOF) + addisp, sp, FRAMESZ(TRAPFRAME_SIZEOF) .endm .macro do_ast Index: arch/riscv64/riscv64/vm_machdep.c === RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v retrieving revision 1.8 diff -u -p -r1.8 vm_machdep.c --- arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 07:47:46 - 1.8 +++ arch/riscv64/riscv64/vm_machdep.c 22 Feb 2022 15:29:05 - @@ -91,7 +91,8 @@ cpu_fork(struct proc *p1, struct proc *p tf->tf_sstatus |= (SSTATUS_SPIE); /* Enable interrupts. */ tf->tf_sstatus &= ~(SSTATUS_SPP); /* Enter user mode. */ - sf = (struct switchframe *)tf - 1; + sf = (struct switchframe *)STACKALIGN((u_long)tf + - sizeof(struct switchframe)); sf->sf_s[0] = 0;/* Terminate chain of call frames. */ sf->sf_s[1] = (uint64_t)func; sf->sf_s[2] = (uint64_t)arg;
dhcpd(8) man page correction : synchronisation
Hi, It looks to me that the -Y option is used to specify the destination to which to send synchronisation messages (not where to receive them, this is -y). Index: dhcpd.8 === RCS file: /cvs/OpenBSD/src/usr.sbin/dhcpd/dhcpd.8,v retrieving revision 1.29 diff -u -p -u -r1.29 dhcpd.8 --- dhcpd.8 29 Aug 2017 08:20:18 - 1.29 +++ dhcpd.8 22 Feb 2022 15:45:20 - @@ -252,7 +252,7 @@ the limited broadcast address (255.255.2 .It Fl Y Ar synctarget Add target .Ar synctarget -to receive synchronisation messages. +to send synchronisation messages. .Ar synctarget can be either an IPv4 address for unicast messages or a network interface name followed optionally by a colon and a numeric TTL -- Matthieu Herrb
Re: acme-client: plug leak in ec_key_create()
On Tue, Feb 22, 2022 at 02:01:26PM +0100, Theo Buehler wrote: > EVP_PKEY_set1_EC_KEY() bumps eckey's refcount (that's what "set1" means), > so eckey isn't freed when pkey is freed at the end of keyproc() or > acctproc() (which means that secret data isn't wiped). Moving the > freeing of eckey to the end of ec_key_create() decrements the refcount > again which should fix this. > > I don't currently have an easy way to test this, so I would appreciate > if someone could try this. I agree with the diff and tested this with a ecdsa domain key. No problem found. OK claudio@ > Index: key.c > === > RCS file: /cvs/src/usr.sbin/acme-client/key.c,v > retrieving revision 1.5 > diff -u -p -r1.5 key.c > --- key.c 22 Feb 2022 12:38:30 - 1.5 > +++ key.c 22 Feb 2022 12:51:32 - > @@ -116,10 +116,10 @@ ec_key_create(FILE *f, const char *fname > goto out; > > err: > - EC_KEY_free(eckey); > EVP_PKEY_free(pkey); > pkey = NULL; > out: > + EC_KEY_free(eckey); > return pkey; > } > > -- :wq Claudio
acme-client: plug leak in ec_key_create()
EVP_PKEY_set1_EC_KEY() bumps eckey's refcount (that's what "set1" means), so eckey isn't freed when pkey is freed at the end of keyproc() or acctproc() (which means that secret data isn't wiped). Moving the freeing of eckey to the end of ec_key_create() decrements the refcount again which should fix this. I don't currently have an easy way to test this, so I would appreciate if someone could try this. Index: key.c === RCS file: /cvs/src/usr.sbin/acme-client/key.c,v retrieving revision 1.5 diff -u -p -r1.5 key.c --- key.c 22 Feb 2022 12:38:30 - 1.5 +++ key.c 22 Feb 2022 12:51:32 - @@ -116,10 +116,10 @@ ec_key_create(FILE *f, const char *fname goto out; err: - EC_KEY_free(eckey); EVP_PKEY_free(pkey); pkey = NULL; out: + EC_KEY_free(eckey); return pkey; }
[Wolf] [PATCH] Move warnx into correct place
OK florian
Start of forwarded message
From: Wolf
To: m...@openbsd.org
Cc: Wolf
Subject: [PATCH] Move warnx into correct place
Date: Sun, 20 Feb 2022 15:10:16 +0100
Original location caused the line to be printed every time for ec keys.
I suspect copy error from rsa_key_create.
---
usr.sbin/acme-client/key.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/usr.sbin/acme-client/key.c b/usr.sbin/acme-client/key.c
index 6604751caef..64b00a36c2f 100644
--- a/usr.sbin/acme-client/key.c
+++ b/usr.sbin/acme-client/key.c
@@ -98,7 +98,7 @@ ec_key_create(FILE *f, const char *fname)
/* Serialise the key to the disc in EC format */
if (!PEM_write_ECPrivateKey(f, eckey, NULL, NULL, 0, NULL, NULL)) {
- warnx("PEM_write_ECPrivateKey");
+ warnx("%s: PEM_write_ECPrivateKey", fname);
goto err;
}
@@ -113,8 +113,6 @@ ec_key_create(FILE *f, const char *fname)
goto err;
}
- warnx("%s: PEM_write_ECPrivateKey", fname);
-
goto out;
err:
--
2.34.1
End of forwarded message
--
I'm not entirely sure you are real.
Re: ifconfig(8): always print the mtu, don't hide it on "bridges"
On Tue, Feb 22, 2022 at 03:46:05PM +1000, David Gwynne wrote: > this lets ifconfig show the MTU on interfaces like nvgre, vxlan, etc. > they currently don't show it because they also implement a bridge ioctl, > so ifconfig thinks they're a bridge. > > why ifconfig hides the mtu on bridges looks to be a hold over from when > brconfig was merged into ifconfig. if we dont want bridge(4) to report > an mtu, then i can make bridge(4) itself hide the mtu or stop setting > the mtu. > > found by jason tubnor. > > ok? > > Index: ifconfig.c > === > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v > retrieving revision 1.451 > diff -u -p -r1.451 ifconfig.c > --- ifconfig.c23 Nov 2021 19:13:45 - 1.451 > +++ ifconfig.c22 Feb 2022 05:38:48 - > @@ -1027,11 +1027,7 @@ getinfo(struct ifreq *ifr, int create) > metric = 0; > else > metric = ifr->ifr_metric; > -#ifdef SMALL > if (ioctl(sock, SIOCGIFMTU, (caddr_t)ifr) == -1) > -#else > - if (is_bridge() || ioctl(sock, SIOCGIFMTU, (caddr_t)ifr) == -1) > -#endif > mtu = 0; > else > mtu = ifr->ifr_mtu; > OK claudio@ -- :wq Claudio
