Re: Add reset option to boot command of ddb(4)

[Martin Pieuchot]

On 14/12/17(Thu) 17:19, Artturi Alm wrote:
> On Thu, Dec 14, 2017 at 02:05:36PM +0100, Martin Pieuchot wrote:
> > On 14/12/17(Thu) 13:37, Florian Riehm wrote:
> > > [...]
> > > If the command could lead to DMA issues, I agree that we should not
> > > introduce it.
> > 
> > The problem is not only DMA issues.  The problem is that ddb(4) will
> > never get fixed.  If we give people a 'reset' button then how long
> > until the core dump code is going to rot?
> > 
> 
> implying the core dump code is not rotting around already?
> it won't take long to count the zer0 replies i got[0],
> for suggesting it's broken on armv7 at bugs@ :(

Please fix it.  That would be a useful task.  I'd suggest fixing arm64
first.

Re: Add reset option to boot command of ddb(4)

[Artturi Alm]

On Thu, Dec 14, 2017 at 02:05:36PM +0100, Martin Pieuchot wrote:
> On 14/12/17(Thu) 13:37, Florian Riehm wrote:
> > [...]
> > If the command could lead to DMA issues, I agree that we should not
> > introduce it.
> 
> The problem is not only DMA issues.  The problem is that ddb(4) will
> never get fixed.  If we give people a 'reset' button then how long
> until the core dump code is going to rot?
> 

implying the core dump code is not rotting around already?
it won't take long to count the zer0 replies i got[0],
for suggesting it's broken on armv7 at bugs@ :(

> As the kernel grows in complexity, because of MP or anything else,
> ddb(4) has to grow with it.
> 
> > I also agree that we should try to make boot reboot as reliable as possible
> > and it works fine in most situations.
> > Anyway I have seen it hanging in some cases.
> 
> Please send a bug report next time you see such case :)
> 

-Artturi

[0] https://marc.info/?l=openbsd-bugs=150125176824983=2

Re: Add reset option to boot command of ddb(4)

[Martin Pieuchot]

On 14/12/17(Thu) 13:37, Florian Riehm wrote:
> [...]
> If the command could lead to DMA issues, I agree that we should not
> introduce it.

The problem is not only DMA issues.  The problem is that ddb(4) will
never get fixed.  If we give people a 'reset' button then how long
until the core dump code is going to rot?

As the kernel grows in complexity, because of MP or anything else,
ddb(4) has to grow with it.

> I also agree that we should try to make boot reboot as reliable as possible
> and it works fine in most situations.
> Anyway I have seen it hanging in some cases.

Please send a bug report next time you see such case :)

Re: Add reset option to boot command of ddb(4)

[Florian Riehm]

On 12/14/17 12:20, Mark Kettenis wrote:

Date: Thu, 14 Dec 2017 11:49:18 +0100
From: Martin Pieuchot 

On 14/12/17(Thu) 11:30, Mark Kettenis wrote:

X-Originating-IP: 88.153.7.170
Date: Thu, 14 Dec 2017 10:30:21 +0100
From: Martin Pieuchot 

On 13/12/17(Wed) 19:09, Florian Riehm wrote:

Hi,

This patch follows bluhm's attempt for a ddb command 'boot reset'.
My first attempt was not architecture aware.

Tested on i386 by bluhm@ and on amd64 by me.


I don't understand why we need to add "boot reset"?  To not fix ddb(4)
and keep a broken "boot reboot"?  If we cannot fix our own code...


Funny you say that given the discussion about if_downall() on icb ;).


There's nothing funny.  There's people not reporting bugs with traceback
to bugs@ and coming around with workaround like that.


IIRC "boot reset" is all about avoiding the if_downall() call.  And we
really don't want to skip if_downall() in the "boot reboot".  We added
that call since not stopping the DMA engines of the network cards had
some very interesting effects when the machine rebooted...


If if_downall() is a problem, then please show me a traceback where
that's the case.  I'd be delighted to fix it :)


True.  Given the DMA issue, "boot reset" would be a rather dangerous
command.



If the command could lead to DMA issues, I agree that we should not
introduce it.
I also agree that we should try to make boot reboot as reliable as possible
and it works fine in most situations.
Anyway I have seen it hanging in some cases. This is a problem for me,
since I don't have physical access to my machines all the time. In such
cases I preferred the cpu_reset hammer. I have never noticed any side effects
like DMA issues, so I thought it would be a good idea, to provide that way
as a regular ddb feature. But I am also fine without the command.

Re: Add reset option to boot command of ddb(4)

[Mark Kettenis]

> Date: Thu, 14 Dec 2017 11:49:18 +0100
> From: Martin Pieuchot 
> 
> On 14/12/17(Thu) 11:30, Mark Kettenis wrote:
> > > X-Originating-IP: 88.153.7.170
> > > Date: Thu, 14 Dec 2017 10:30:21 +0100
> > > From: Martin Pieuchot 
> > > 
> > > On 13/12/17(Wed) 19:09, Florian Riehm wrote:
> > > > Hi,
> > > > 
> > > > This patch follows bluhm's attempt for a ddb command 'boot reset'.
> > > > My first attempt was not architecture aware.
> > > > 
> > > > Tested on i386 by bluhm@ and on amd64 by me.
> > > 
> > > I don't understand why we need to add "boot reset"?  To not fix ddb(4)
> > > and keep a broken "boot reboot"?  If we cannot fix our own code...
> > 
> > Funny you say that given the discussion about if_downall() on icb ;).
> 
> There's nothing funny.  There's people not reporting bugs with traceback
> to bugs@ and coming around with workaround like that.
> 
> > IIRC "boot reset" is all about avoiding the if_downall() call.  And we
> > really don't want to skip if_downall() in the "boot reboot".  We added
> > that call since not stopping the DMA engines of the network cards had
> > some very interesting effects when the machine rebooted...
> 
> If if_downall() is a problem, then please show me a traceback where
> that's the case.  I'd be delighted to fix it :)

True.  Given the DMA issue, "boot reset" would be a rather dangerous
command.

Re: Add reset option to boot command of ddb(4)

[Peter Hessler]

On 2017 Dec 14 (Thu) at 11:49:18 +0100 (+0100), Martin Pieuchot wrote:
:On 14/12/17(Thu) 11:30, Mark Kettenis wrote:
:> > X-Originating-IP: 88.153.7.170
:> > Date: Thu, 14 Dec 2017 10:30:21 +0100
:> > From: Martin Pieuchot 
:> > 
:> > On 13/12/17(Wed) 19:09, Florian Riehm wrote:
:> > > Hi,
:> > > 
:> > > This patch follows bluhm's attempt for a ddb command 'boot reset'.
:> > > My first attempt was not architecture aware.
:> > > 
:> > > Tested on i386 by bluhm@ and on amd64 by me.
:> > 
:> > I don't understand why we need to add "boot reset"?  To not fix ddb(4)
:> > and keep a broken "boot reboot"?  If we cannot fix our own code...
:> 
:> Funny you say that given the discussion about if_downall() on icb ;).
:
:There's nothing funny.  There's people not reporting bugs with traceback
:to bugs@ and coming around with workaround like that.
:
:> IIRC "boot reset" is all about avoiding the if_downall() call.  And we
:> really don't want to skip if_downall() in the "boot reboot".  We added
:> that call since not stopping the DMA engines of the network cards had
:> some very interesting effects when the machine rebooted...
:
:If if_downall() is a problem, then please show me a traceback where
:that's the case.  I'd be delighted to fix it :)
:

Trace is on bugs, Subject: arm64 panic uvm_fault failed: ff80002619b4 with 
bonus panic: netlock: lock not held

-- 
A truly wise man never plays leapfrog with a unicorn.

Re: Add reset option to boot command of ddb(4)

[Martin Pieuchot]

On 14/12/17(Thu) 11:30, Mark Kettenis wrote:
> > X-Originating-IP: 88.153.7.170
> > Date: Thu, 14 Dec 2017 10:30:21 +0100
> > From: Martin Pieuchot 
> > 
> > On 13/12/17(Wed) 19:09, Florian Riehm wrote:
> > > Hi,
> > > 
> > > This patch follows bluhm's attempt for a ddb command 'boot reset'.
> > > My first attempt was not architecture aware.
> > > 
> > > Tested on i386 by bluhm@ and on amd64 by me.
> > 
> > I don't understand why we need to add "boot reset"?  To not fix ddb(4)
> > and keep a broken "boot reboot"?  If we cannot fix our own code...
> 
> Funny you say that given the discussion about if_downall() on icb ;).

There's nothing funny.  There's people not reporting bugs with traceback
to bugs@ and coming around with workaround like that.

> IIRC "boot reset" is all about avoiding the if_downall() call.  And we
> really don't want to skip if_downall() in the "boot reboot".  We added
> that call since not stopping the DMA engines of the network cards had
> some very interesting effects when the machine rebooted...

If if_downall() is a problem, then please show me a traceback where
that's the case.  I'd be delighted to fix it :)

Re: Add reset option to boot command of ddb(4)

[Mark Kettenis]

> X-Originating-IP: 88.153.7.170
> Date: Thu, 14 Dec 2017 10:30:21 +0100
> From: Martin Pieuchot 
> 
> On 13/12/17(Wed) 19:09, Florian Riehm wrote:
> > Hi,
> > 
> > This patch follows bluhm's attempt for a ddb command 'boot reset'.
> > My first attempt was not architecture aware.
> > 
> > Tested on i386 by bluhm@ and on amd64 by me.
> 
> I don't understand why we need to add "boot reset"?  To not fix ddb(4)
> and keep a broken "boot reboot"?  If we cannot fix our own code...

Funny you say that given the discussion about if_downall() on icb ;).

IIRC "boot reset" is all about avoiding the if_downall() call.  And we
really don't want to skip if_downall() in the "boot reboot".  We added
that call since not stopping the DMA engines of the network cards had
some very interesting effects when the machine rebooted...

> > Index: share/man/man4/ddb.4
> > ===
> > RCS file: /openbsd/src/share/man/man4/ddb.4,v
> > retrieving revision 1.92
> > diff -u -p -r1.92 ddb.4
> > --- share/man/man4/ddb.429 Nov 2017 07:28:21 -  1.92
> > +++ share/man/man4/ddb.412 Dec 2017 06:35:44 -
> > @@ -381,6 +381,15 @@ Just halt.
> >  Just reboot.
> >  .It Ic boot poweroff
> >  Power down the machine whenever possible; if it fails, just halt.
> > +.It Ic boot reset
> > +Restart the machine by resetting the CPU on i386 and amd64
> > +architectures.
> > +Useful in situations were
> > +.Ic boot reboot
> > +does not work anymore, i.e. due to locking issues.
> > +On other platforms it is equivalent to the
> > +.Ic boot reboot
> > +command.
> >  .El
> >  .\" 
> >  .It Xo
> > Index: sys/arch/amd64/amd64/machdep.c
> > ===
> > RCS file: /openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> > retrieving revision 1.236
> > diff -u -p -r1.236 machdep.c
> > --- sys/arch/amd64/amd64/machdep.c  11 Dec 2017 05:27:40 -  1.236
> > +++ sys/arch/amd64/amd64/machdep.c  12 Dec 2017 06:35:44 -
> > @@ -713,6 +713,9 @@ struct pcb dumppcb;
> >  __dead void
> >  boot(int howto)
> >  {
> > +   if ((howto & RB_RESET) != 0)
> > +   goto reset;
> > +
> > if ((howto & RB_POWERDOWN) != 0)
> > lid_action = 0;
> >  
> > @@ -770,6 +773,7 @@ haltsys:
> > printf("rebooting...\n");
> > if (cpureset_delay > 0)
> > delay(cpureset_delay * 1000);
> > +reset:
> > cpu_reset();
> > for (;;)
> > continue;
> > Index: sys/arch/i386/i386/machdep.c
> > ===
> > RCS file: /openbsd/src/sys/arch/i386/i386/machdep.c,v
> > retrieving revision 1.607
> > diff -u -p -r1.607 machdep.c
> > --- sys/arch/i386/i386/machdep.c11 Dec 2017 05:27:40 -  1.607
> > +++ sys/arch/i386/i386/machdep.c12 Dec 2017 06:35:44 -
> > @@ -2629,6 +2629,9 @@ struct pcb dumppcb;
> >  __dead void
> >  boot(int howto)
> >  {
> > +   if ((howto & RB_RESET) != 0)
> > +   goto reset;
> > +
> > if ((howto & RB_POWERDOWN) != 0)
> > lid_action = 0;
> >  
> > @@ -2709,6 +2712,7 @@ haltsys:
> > }
> >  
> > printf("rebooting...\n");
> > +reset:
> > cpu_reset();
> > for (;;)
> > continue;
> > Index: sys/ddb/db_command.c
> > ===
> > RCS file: /openbsd/src/sys/ddb/db_command.c,v
> > retrieving revision 1.81
> > diff -u -p -r1.81 db_command.c
> > --- sys/ddb/db_command.c11 Dec 2017 05:27:40 -  1.81
> > +++ sys/ddb/db_command.c12 Dec 2017 06:35:44 -
> > @@ -105,6 +105,7 @@ voiddb_boot_dump_cmd(db_expr_t, int, db
> >  void   db_boot_halt_cmd(db_expr_t, int, db_expr_t, char *);
> >  void   db_boot_reboot_cmd(db_expr_t, int, db_expr_t, char *);
> >  void   db_boot_poweroff_cmd(db_expr_t, int, db_expr_t, char *);
> > +void   db_boot_reset_cmd(db_expr_t, int, db_expr_t, char *);
> >  void   db_stack_trace_cmd(db_expr_t, int, db_expr_t, char *);
> >  void   db_dmesg_cmd(db_expr_t, int, db_expr_t, char *);
> >  void   db_show_panic_cmd(db_expr_t, int, db_expr_t, char *);
> > @@ -597,6 +598,7 @@ struct db_command db_boot_cmds[] = {
> > { "halt",   db_boot_halt_cmd,   0,  0 },
> > { "reboot", db_boot_reboot_cmd, 0,  0 },
> > { "poweroff",   db_boot_poweroff_cmd,   0,  0 },
> > +   { "reset",  db_boot_reset_cmd,  0,  0 },
> > { NULL, }
> >  };
> >  
> > @@ -812,6 +814,12 @@ void
> >  db_boot_poweroff_cmd(db_expr_t addr, int haddr, db_expr_t count, char 
> > *modif)
> >  {
> > db_reboot(RB_NOSYNC | RB_HALT | RB_POWERDOWN | RB_TIMEBAD | RB_USERREQ);
> > +}
> > +
> > +void
> > +db_boot_reset_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
> > +{
> > +   db_reboot(RB_RESET | RB_AUTOBOOT | RB_NOSYNC | RB_TIMEBAD | RB_USERREQ);
> >  }
> >  
> >  void
> > Index: sys/sys/reboot.h
> > 

Re: Add reset option to boot command of ddb(4)

[Martin Pieuchot]

On 13/12/17(Wed) 19:09, Florian Riehm wrote:
> Hi,
> 
> This patch follows bluhm's attempt for a ddb command 'boot reset'.
> My first attempt was not architecture aware.
> 
> Tested on i386 by bluhm@ and on amd64 by me.

I don't understand why we need to add "boot reset"?  To not fix ddb(4)
and keep a broken "boot reboot"?  If we cannot fix our own code...

> Index: share/man/man4/ddb.4
> ===
> RCS file: /openbsd/src/share/man/man4/ddb.4,v
> retrieving revision 1.92
> diff -u -p -r1.92 ddb.4
> --- share/man/man4/ddb.4  29 Nov 2017 07:28:21 -  1.92
> +++ share/man/man4/ddb.4  12 Dec 2017 06:35:44 -
> @@ -381,6 +381,15 @@ Just halt.
>  Just reboot.
>  .It Ic boot poweroff
>  Power down the machine whenever possible; if it fails, just halt.
> +.It Ic boot reset
> +Restart the machine by resetting the CPU on i386 and amd64
> +architectures.
> +Useful in situations were
> +.Ic boot reboot
> +does not work anymore, i.e. due to locking issues.
> +On other platforms it is equivalent to the
> +.Ic boot reboot
> +command.
>  .El
>  .\" 
>  .It Xo
> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.236
> diff -u -p -r1.236 machdep.c
> --- sys/arch/amd64/amd64/machdep.c11 Dec 2017 05:27:40 -  1.236
> +++ sys/arch/amd64/amd64/machdep.c12 Dec 2017 06:35:44 -
> @@ -713,6 +713,9 @@ struct pcb dumppcb;
>  __dead void
>  boot(int howto)
>  {
> + if ((howto & RB_RESET) != 0)
> + goto reset;
> +
>   if ((howto & RB_POWERDOWN) != 0)
>   lid_action = 0;
>  
> @@ -770,6 +773,7 @@ haltsys:
>   printf("rebooting...\n");
>   if (cpureset_delay > 0)
>   delay(cpureset_delay * 1000);
> +reset:
>   cpu_reset();
>   for (;;)
>   continue;
> Index: sys/arch/i386/i386/machdep.c
> ===
> RCS file: /openbsd/src/sys/arch/i386/i386/machdep.c,v
> retrieving revision 1.607
> diff -u -p -r1.607 machdep.c
> --- sys/arch/i386/i386/machdep.c  11 Dec 2017 05:27:40 -  1.607
> +++ sys/arch/i386/i386/machdep.c  12 Dec 2017 06:35:44 -
> @@ -2629,6 +2629,9 @@ struct pcb dumppcb;
>  __dead void
>  boot(int howto)
>  {
> + if ((howto & RB_RESET) != 0)
> + goto reset;
> +
>   if ((howto & RB_POWERDOWN) != 0)
>   lid_action = 0;
>  
> @@ -2709,6 +2712,7 @@ haltsys:
>   }
>  
>   printf("rebooting...\n");
> +reset:
>   cpu_reset();
>   for (;;)
>   continue;
> Index: sys/ddb/db_command.c
> ===
> RCS file: /openbsd/src/sys/ddb/db_command.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 db_command.c
> --- sys/ddb/db_command.c  11 Dec 2017 05:27:40 -  1.81
> +++ sys/ddb/db_command.c  12 Dec 2017 06:35:44 -
> @@ -105,6 +105,7 @@ void  db_boot_dump_cmd(db_expr_t, int, db
>  void db_boot_halt_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_reboot_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_poweroff_cmd(db_expr_t, int, db_expr_t, char *);
> +void db_boot_reset_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_stack_trace_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_dmesg_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_show_panic_cmd(db_expr_t, int, db_expr_t, char *);
> @@ -597,6 +598,7 @@ struct db_command db_boot_cmds[] = {
>   { "halt",   db_boot_halt_cmd,   0,  0 },
>   { "reboot", db_boot_reboot_cmd, 0,  0 },
>   { "poweroff",   db_boot_poweroff_cmd,   0,  0 },
> + { "reset",  db_boot_reset_cmd,  0,  0 },
>   { NULL, }
>  };
>  
> @@ -812,6 +814,12 @@ void
>  db_boot_poweroff_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
>  {
>   db_reboot(RB_NOSYNC | RB_HALT | RB_POWERDOWN | RB_TIMEBAD | RB_USERREQ);
> +}
> +
> +void
> +db_boot_reset_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
> +{
> + db_reboot(RB_RESET | RB_AUTOBOOT | RB_NOSYNC | RB_TIMEBAD | RB_USERREQ);
>  }
>  
>  void
> Index: sys/sys/reboot.h
> ===
> RCS file: /openbsd/src/sys/sys/reboot.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 reboot.h
> --- sys/sys/reboot.h  11 Jul 2014 14:36:44 -  1.17
> +++ sys/sys/reboot.h  12 Dec 2017 06:35:45 -
> @@ -56,6 +56,7 @@
>  #define  RB_POWERDOWN0x1000  /* attempt to power down machine */
>  #define  RB_SERCONS  0x2000  /* use serial console if available */
>  #define  RB_USERREQ  0x4000  /* boot() called at user request (e.g. 
> ddb) */
> +#define  RB_RESET0x8000  /* do not try to cleanup, only for ddb 
> */
>  
>  /*
>   * Constants for converting boot-style 

Re: Add reset option to boot command of ddb(4)

[Florian Riehm]

I will prepare a new diff including the other architecures and
try to find people who can test it.
I have had such a diff already but then I decided to remove
the untested parts because I didn't want to submit untested
code.

friehm

On 12/13/17 21:59, Theo de Raadt wrote:

As it is, this diff will not go in.

Your 2nd attempt is not architecture aware either.  There are more
than 2 architectures.  If you add a MI feature, you must attempt to
add support for it to all the MD versions.  And the process of mailing
it out to the community gives people an opportunity to help test
those.

Seeing as this is only a goto and a label: on each architecture, what
is the purpose of not even trying to write such a diff??  You are
leaving the work, hoping someone eventually does it??

That isn't the way we work.


This patch follows bluhm's attempt for a ddb command 'boot reset'.
My first attempt was not architecture aware.

Tested on i386 by bluhm@ and on amd64 by me.

ok?

friehm

Index: share/man/man4/ddb.4
===
RCS file: /openbsd/src/share/man/man4/ddb.4,v
retrieving revision 1.92
diff -u -p -r1.92 ddb.4
--- share/man/man4/ddb.429 Nov 2017 07:28:21 -  1.92
+++ share/man/man4/ddb.412 Dec 2017 06:35:44 -
@@ -381,6 +381,15 @@ Just halt.
  Just reboot.
  .It Ic boot poweroff
  Power down the machine whenever possible; if it fails, just halt.
+.It Ic boot reset
+Restart the machine by resetting the CPU on i386 and amd64
+architectures.
+Useful in situations were
+.Ic boot reboot
+does not work anymore, i.e. due to locking issues.
+On other platforms it is equivalent to the
+.Ic boot reboot
+command.
  .El
  .\" 
  .It Xo
Index: sys/arch/amd64/amd64/machdep.c
===
RCS file: /openbsd/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.236
diff -u -p -r1.236 machdep.c
--- sys/arch/amd64/amd64/machdep.c  11 Dec 2017 05:27:40 -  1.236
+++ sys/arch/amd64/amd64/machdep.c  12 Dec 2017 06:35:44 -
@@ -713,6 +713,9 @@ struct pcb dumppcb;
  __dead void
  boot(int howto)
  {
+   if ((howto & RB_RESET) != 0)
+   goto reset;
+
if ((howto & RB_POWERDOWN) != 0)
lid_action = 0;
  
@@ -770,6 +773,7 @@ haltsys:

printf("rebooting...\n");
if (cpureset_delay > 0)
delay(cpureset_delay * 1000);
+reset:
cpu_reset();
for (;;)
continue;
Index: sys/arch/i386/i386/machdep.c
===
RCS file: /openbsd/src/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.607
diff -u -p -r1.607 machdep.c
--- sys/arch/i386/i386/machdep.c11 Dec 2017 05:27:40 -  1.607
+++ sys/arch/i386/i386/machdep.c12 Dec 2017 06:35:44 -
@@ -2629,6 +2629,9 @@ struct pcb dumppcb;
  __dead void
  boot(int howto)
  {
+   if ((howto & RB_RESET) != 0)
+   goto reset;
+
if ((howto & RB_POWERDOWN) != 0)
lid_action = 0;
  
@@ -2709,6 +2712,7 @@ haltsys:

}
  
  	printf("rebooting...\n");

+reset:
cpu_reset();
for (;;)
continue;
Index: sys/ddb/db_command.c
===
RCS file: /openbsd/src/sys/ddb/db_command.c,v
retrieving revision 1.81
diff -u -p -r1.81 db_command.c
--- sys/ddb/db_command.c11 Dec 2017 05:27:40 -  1.81
+++ sys/ddb/db_command.c12 Dec 2017 06:35:44 -
@@ -105,6 +105,7 @@ voiddb_boot_dump_cmd(db_expr_t, int, db
  void  db_boot_halt_cmd(db_expr_t, int, db_expr_t, char *);
  void  db_boot_reboot_cmd(db_expr_t, int, db_expr_t, char *);
  void  db_boot_poweroff_cmd(db_expr_t, int, db_expr_t, char *);
+void   db_boot_reset_cmd(db_expr_t, int, db_expr_t, char *);
  void  db_stack_trace_cmd(db_expr_t, int, db_expr_t, char *);
  void  db_dmesg_cmd(db_expr_t, int, db_expr_t, char *);
  void  db_show_panic_cmd(db_expr_t, int, db_expr_t, char *);
@@ -597,6 +598,7 @@ struct db_command db_boot_cmds[] = {
{ "halt", db_boot_halt_cmd,   0,  0 },
{ "reboot",   db_boot_reboot_cmd, 0,  0 },
{ "poweroff", db_boot_poweroff_cmd,   0,  0 },
+   { "reset",db_boot_reset_cmd,  0,  0 },
{ NULL, }
  };
  
@@ -812,6 +814,12 @@ void

  db_boot_poweroff_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
  {
db_reboot(RB_NOSYNC | RB_HALT | RB_POWERDOWN | RB_TIMEBAD | RB_USERREQ);
+}
+
+void
+db_boot_reset_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
+{
+   db_reboot(RB_RESET | RB_AUTOBOOT | RB_NOSYNC | RB_TIMEBAD | RB_USERREQ);
  }
  
  void

Index: sys/sys/reboot.h
===
RCS file: /openbsd/src/sys/sys/reboot.h,v
retrieving revision 1.17
diff -u -p -r1.17 reboot.h
--- sys/sys/reboot.h  

Re: Add reset option to boot command of ddb(4)

[Raf Czlonka]

On Wed, Dec 13, 2017 at 06:09:14PM GMT, Florian Riehm wrote:
> Hi,
> 
> This patch follows bluhm's attempt for a ddb command 'boot reset'.
> My first attempt was not architecture aware.
> 
> Tested on i386 by bluhm@ and on amd64 by me.
> 
> ok?
> 
> friehm
> 
> Index: share/man/man4/ddb.4
> ===
> RCS file: /openbsd/src/share/man/man4/ddb.4,v
> retrieving revision 1.92
> diff -u -p -r1.92 ddb.4
> --- share/man/man4/ddb.4  29 Nov 2017 07:28:21 -  1.92
> +++ share/man/man4/ddb.4  12 Dec 2017 06:35:44 -
> @@ -381,6 +381,15 @@ Just halt.
>  Just reboot.
>  .It Ic boot poweroff
>  Power down the machine whenever possible; if it fails, just halt.
> +.It Ic boot reset
> +Restart the machine by resetting the CPU on i386 and amd64
> +architectures.
> +Useful in situations were

Shouldn't this read "where"?

Raf

> +.Ic boot reboot
> +does not work anymore, i.e. due to locking issues.
> +On other platforms it is equivalent to the
> +.Ic boot reboot
> +command.
>  .El
>  .\" 
>  .It Xo
> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.236
> diff -u -p -r1.236 machdep.c
> --- sys/arch/amd64/amd64/machdep.c11 Dec 2017 05:27:40 -  1.236
> +++ sys/arch/amd64/amd64/machdep.c12 Dec 2017 06:35:44 -
> @@ -713,6 +713,9 @@ struct pcb dumppcb;
>  __dead void
>  boot(int howto)
>  {
> + if ((howto & RB_RESET) != 0)
> + goto reset;
> +
>   if ((howto & RB_POWERDOWN) != 0)
>   lid_action = 0;
>  
> @@ -770,6 +773,7 @@ haltsys:
>   printf("rebooting...\n");
>   if (cpureset_delay > 0)
>   delay(cpureset_delay * 1000);
> +reset:
>   cpu_reset();
>   for (;;)
>   continue;
> Index: sys/arch/i386/i386/machdep.c
> ===
> RCS file: /openbsd/src/sys/arch/i386/i386/machdep.c,v
> retrieving revision 1.607
> diff -u -p -r1.607 machdep.c
> --- sys/arch/i386/i386/machdep.c  11 Dec 2017 05:27:40 -  1.607
> +++ sys/arch/i386/i386/machdep.c  12 Dec 2017 06:35:44 -
> @@ -2629,6 +2629,9 @@ struct pcb dumppcb;
>  __dead void
>  boot(int howto)
>  {
> + if ((howto & RB_RESET) != 0)
> + goto reset;
> +
>   if ((howto & RB_POWERDOWN) != 0)
>   lid_action = 0;
>  
> @@ -2709,6 +2712,7 @@ haltsys:
>   }
>  
>   printf("rebooting...\n");
> +reset:
>   cpu_reset();
>   for (;;)
>   continue;
> Index: sys/ddb/db_command.c
> ===
> RCS file: /openbsd/src/sys/ddb/db_command.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 db_command.c
> --- sys/ddb/db_command.c  11 Dec 2017 05:27:40 -  1.81
> +++ sys/ddb/db_command.c  12 Dec 2017 06:35:44 -
> @@ -105,6 +105,7 @@ void  db_boot_dump_cmd(db_expr_t, int, db
>  void db_boot_halt_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_reboot_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_poweroff_cmd(db_expr_t, int, db_expr_t, char *);
> +void db_boot_reset_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_stack_trace_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_dmesg_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_show_panic_cmd(db_expr_t, int, db_expr_t, char *);
> @@ -597,6 +598,7 @@ struct db_command db_boot_cmds[] = {
>   { "halt",   db_boot_halt_cmd,   0,  0 },
>   { "reboot", db_boot_reboot_cmd, 0,  0 },
>   { "poweroff",   db_boot_poweroff_cmd,   0,  0 },
> + { "reset",  db_boot_reset_cmd,  0,  0 },
>   { NULL, }
>  };
>  
> @@ -812,6 +814,12 @@ void
>  db_boot_poweroff_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
>  {
>   db_reboot(RB_NOSYNC | RB_HALT | RB_POWERDOWN | RB_TIMEBAD | RB_USERREQ);
> +}
> +
> +void
> +db_boot_reset_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
> +{
> + db_reboot(RB_RESET | RB_AUTOBOOT | RB_NOSYNC | RB_TIMEBAD | RB_USERREQ);
>  }
>  
>  void
> Index: sys/sys/reboot.h
> ===
> RCS file: /openbsd/src/sys/sys/reboot.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 reboot.h
> --- sys/sys/reboot.h  11 Jul 2014 14:36:44 -  1.17
> +++ sys/sys/reboot.h  12 Dec 2017 06:35:45 -
> @@ -56,6 +56,7 @@
>  #define  RB_POWERDOWN0x1000  /* attempt to power down machine */
>  #define  RB_SERCONS  0x2000  /* use serial console if available */
>  #define  RB_USERREQ  0x4000  /* boot() called at user request (e.g. 
> ddb) */
> +#define  RB_RESET0x8000  /* do not try to cleanup, only for ddb 
> */
>  
>  /*
>   * Constants for converting boot-style device number to type,
> 

Re: Add reset option to boot command of ddb(4)

[Theo de Raadt]

As it is, this diff will not go in.

Your 2nd attempt is not architecture aware either.  There are more
than 2 architectures.  If you add a MI feature, you must attempt to
add support for it to all the MD versions.  And the process of mailing
it out to the community gives people an opportunity to help test
those.

Seeing as this is only a goto and a label: on each architecture, what
is the purpose of not even trying to write such a diff??  You are
leaving the work, hoping someone eventually does it??

That isn't the way we work.

> This patch follows bluhm's attempt for a ddb command 'boot reset'.
> My first attempt was not architecture aware.
> 
> Tested on i386 by bluhm@ and on amd64 by me.
> 
> ok?
> 
> friehm
> 
> Index: share/man/man4/ddb.4
> ===
> RCS file: /openbsd/src/share/man/man4/ddb.4,v
> retrieving revision 1.92
> diff -u -p -r1.92 ddb.4
> --- share/man/man4/ddb.4  29 Nov 2017 07:28:21 -  1.92
> +++ share/man/man4/ddb.4  12 Dec 2017 06:35:44 -
> @@ -381,6 +381,15 @@ Just halt.
>  Just reboot.
>  .It Ic boot poweroff
>  Power down the machine whenever possible; if it fails, just halt.
> +.It Ic boot reset
> +Restart the machine by resetting the CPU on i386 and amd64
> +architectures.
> +Useful in situations were
> +.Ic boot reboot
> +does not work anymore, i.e. due to locking issues.
> +On other platforms it is equivalent to the
> +.Ic boot reboot
> +command.
>  .El
>  .\" 
>  .It Xo
> Index: sys/arch/amd64/amd64/machdep.c
> ===
> RCS file: /openbsd/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.236
> diff -u -p -r1.236 machdep.c
> --- sys/arch/amd64/amd64/machdep.c11 Dec 2017 05:27:40 -  1.236
> +++ sys/arch/amd64/amd64/machdep.c12 Dec 2017 06:35:44 -
> @@ -713,6 +713,9 @@ struct pcb dumppcb;
>  __dead void
>  boot(int howto)
>  {
> + if ((howto & RB_RESET) != 0)
> + goto reset;
> +
>   if ((howto & RB_POWERDOWN) != 0)
>   lid_action = 0;
>  
> @@ -770,6 +773,7 @@ haltsys:
>   printf("rebooting...\n");
>   if (cpureset_delay > 0)
>   delay(cpureset_delay * 1000);
> +reset:
>   cpu_reset();
>   for (;;)
>   continue;
> Index: sys/arch/i386/i386/machdep.c
> ===
> RCS file: /openbsd/src/sys/arch/i386/i386/machdep.c,v
> retrieving revision 1.607
> diff -u -p -r1.607 machdep.c
> --- sys/arch/i386/i386/machdep.c  11 Dec 2017 05:27:40 -  1.607
> +++ sys/arch/i386/i386/machdep.c  12 Dec 2017 06:35:44 -
> @@ -2629,6 +2629,9 @@ struct pcb dumppcb;
>  __dead void
>  boot(int howto)
>  {
> + if ((howto & RB_RESET) != 0)
> + goto reset;
> +
>   if ((howto & RB_POWERDOWN) != 0)
>   lid_action = 0;
>  
> @@ -2709,6 +2712,7 @@ haltsys:
>   }
>  
>   printf("rebooting...\n");
> +reset:
>   cpu_reset();
>   for (;;)
>   continue;
> Index: sys/ddb/db_command.c
> ===
> RCS file: /openbsd/src/sys/ddb/db_command.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 db_command.c
> --- sys/ddb/db_command.c  11 Dec 2017 05:27:40 -  1.81
> +++ sys/ddb/db_command.c  12 Dec 2017 06:35:44 -
> @@ -105,6 +105,7 @@ void  db_boot_dump_cmd(db_expr_t, int, db
>  void db_boot_halt_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_reboot_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_poweroff_cmd(db_expr_t, int, db_expr_t, char *);
> +void db_boot_reset_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_stack_trace_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_dmesg_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_show_panic_cmd(db_expr_t, int, db_expr_t, char *);
> @@ -597,6 +598,7 @@ struct db_command db_boot_cmds[] = {
>   { "halt",   db_boot_halt_cmd,   0,  0 },
>   { "reboot", db_boot_reboot_cmd, 0,  0 },
>   { "poweroff",   db_boot_poweroff_cmd,   0,  0 },
> + { "reset",  db_boot_reset_cmd,  0,  0 },
>   { NULL, }
>  };
>  
> @@ -812,6 +814,12 @@ void
>  db_boot_poweroff_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
>  {
>   db_reboot(RB_NOSYNC | RB_HALT | RB_POWERDOWN | RB_TIMEBAD | RB_USERREQ);
> +}
> +
> +void
> +db_boot_reset_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
> +{
> + db_reboot(RB_RESET | RB_AUTOBOOT | RB_NOSYNC | RB_TIMEBAD | RB_USERREQ);
>  }
>  
>  void
> Index: sys/sys/reboot.h
> ===
> RCS file: /openbsd/src/sys/sys/reboot.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 reboot.h
> --- sys/sys/reboot.h  11 Jul 2014 14:36:44 -  1.17
> +++ sys/sys/reboot.h  12 Dec 2017 06:35:45 -
> @@ -56,6 +56,7 @@
>  #define  

Re: Add reset option to boot command of ddb(4)

[Theo de Raadt]

This is a better plan.  All the architectures can adapt to this,
even those that have a tricky ROM-related dance.

> On Thu, Oct 26, 2017 at 10:32:42PM +1100, Jonathan Gray wrote:
> > What specifically?  Skip if_downall() if rebooting from ddb?
> > That could perhaps even be done for RB_NOSYNC.
> 
> I thought of someting like a big hammer.  Skip everything except
> the final call in boot() that causes the machine to reset.  The
> command is only reachable form ddb and useful if nothing else can
> reboot the machine.
> 
> Here is an example implementation for amd64.
> 
> bluhm
> 
> Index: arch/amd64/amd64/machdep.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/machdep.c,v
> retrieving revision 1.234
> diff -u -p -r1.234 machdep.c
> --- arch/amd64/amd64/machdep.c23 Oct 2017 15:41:29 -  1.234
> +++ arch/amd64/amd64/machdep.c26 Oct 2017 11:43:55 -
> @@ -713,6 +713,9 @@ struct pcb dumppcb;
>  __dead void
>  boot(int howto)
>  {
> + if ((howto & RB_RESET) != 0)
> + goto reset;
> +
>   if ((howto & RB_POWERDOWN) != 0)
>   lid_action = 0;
>  
> @@ -770,6 +773,7 @@ haltsys:
>   printf("rebooting...\n");
>   if (cpureset_delay > 0)
>   delay(cpureset_delay * 1000);
> +reset:
>   cpu_reset();
>   for (;;)
>   continue;
> Index: sys/reboot.h
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/reboot.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 reboot.h
> --- sys/reboot.h  11 Jul 2014 14:36:44 -  1.17
> +++ sys/reboot.h  26 Oct 2017 11:41:56 -
> @@ -56,6 +56,7 @@
>  #define  RB_POWERDOWN0x1000  /* attempt to power down machine */
>  #define  RB_SERCONS  0x2000  /* use serial console if available */
>  #define  RB_USERREQ  0x4000  /* boot() called at user request (e.g. 
> ddb) */
> +#define  RB_RESET0x8000  /* do not try to cleanup, only for ddb 
> */
>  
>  /*
>   * Constants for converting boot-style device number to type,
> 

Re: Add reset option to boot command of ddb(4)

[Alexander Bluhm]

On Thu, Oct 26, 2017 at 10:32:42PM +1100, Jonathan Gray wrote:
> What specifically?  Skip if_downall() if rebooting from ddb?
> That could perhaps even be done for RB_NOSYNC.

I thought of someting like a big hammer.  Skip everything except
the final call in boot() that causes the machine to reset.  The
command is only reachable form ddb and useful if nothing else can
reboot the machine.

Here is an example implementation for amd64.

bluhm

Index: arch/amd64/amd64/machdep.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.234
diff -u -p -r1.234 machdep.c
--- arch/amd64/amd64/machdep.c  23 Oct 2017 15:41:29 -  1.234
+++ arch/amd64/amd64/machdep.c  26 Oct 2017 11:43:55 -
@@ -713,6 +713,9 @@ struct pcb dumppcb;
 __dead void
 boot(int howto)
 {
+   if ((howto & RB_RESET) != 0)
+   goto reset;
+
if ((howto & RB_POWERDOWN) != 0)
lid_action = 0;
 
@@ -770,6 +773,7 @@ haltsys:
printf("rebooting...\n");
if (cpureset_delay > 0)
delay(cpureset_delay * 1000);
+reset:
cpu_reset();
for (;;)
continue;
Index: sys/reboot.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/reboot.h,v
retrieving revision 1.17
diff -u -p -r1.17 reboot.h
--- sys/reboot.h11 Jul 2014 14:36:44 -  1.17
+++ sys/reboot.h26 Oct 2017 11:41:56 -
@@ -56,6 +56,7 @@
 #defineRB_POWERDOWN0x1000  /* attempt to power down machine */
 #defineRB_SERCONS  0x2000  /* use serial console if available */
 #defineRB_USERREQ  0x4000  /* boot() called at user request (e.g. 
ddb) */
+#defineRB_RESET0x8000  /* do not try to cleanup, only for ddb 
*/
 
 /*
  * Constants for converting boot-style device number to type,

Re: Add reset option to boot command of ddb(4)

[Jonathan Gray]

On Thu, Oct 26, 2017 at 01:12:53PM +0200, Alexander Bluhm wrote:
> On Thu, Oct 26, 2017 at 08:08:35PM +1100, Jonathan Gray wrote:
> > No, cpu_reset() is MD this will break ddb on all non x86 archs besides
> > landisk.
> 
> Would it make sense to implement a boot(RB_RESET) that works
> everywhere?
> 
> Problem is that when adding MP locks to the kernel, ddb boot reboot
> does not work reliably.  We need something that does not run code
> that may allocate locks.
> 
> bluhm

What specifically?  Skip if_downall() if rebooting from ddb?
That could perhaps even be done for RB_NOSYNC.

Re: Add reset option to boot command of ddb(4)

[Alexander Bluhm]

On Thu, Oct 26, 2017 at 08:08:35PM +1100, Jonathan Gray wrote:
> No, cpu_reset() is MD this will break ddb on all non x86 archs besides
> landisk.

Would it make sense to implement a boot(RB_RESET) that works
everywhere?

Problem is that when adding MP locks to the kernel, ddb boot reboot
does not work reliably.  We need something that does not run code
that may allocate locks.

bluhm

Re: Add reset option to boot command of ddb(4)

[Stuart Henderson]

On 2017/10/26 10:42, Florian Riehm wrote:
> Hi,
> 
> Sometimes I see systems hanging in ddb(4) after panic(9) and the "boot reboot"
> command doesn't work anymore, i.e. of filesystem or locking issues.
> Bluhm@ suggested to me to use "call cpu_reset" in such situations.
> 
> I would like to introduce a command 'boot reset' to do this.

cpu_reset is MD.

Re: Add reset option to boot command of ddb(4)

[Jonathan Gray]

On Thu, Oct 26, 2017 at 10:42:17AM +0200, Florian Riehm wrote:
> Hi,
> 
> Sometimes I see systems hanging in ddb(4) after panic(9) and the "boot reboot"
> command doesn't work anymore, i.e. of filesystem or locking issues.
> Bluhm@ suggested to me to use "call cpu_reset" in such situations.
> 
> I would like to introduce a command 'boot reset' to do this.
> 
> ok?

No, cpu_reset() is MD this will break ddb on all non x86 archs besides
landisk.

> 
> friehm
> 
> 
> Index: share/man/man4//ddb.4
> ===
> RCS file: /cvs/src/share/man/man4/ddb.4,v
> retrieving revision 1.91
> diff -u -p -r1.91 ddb.4
> --- share/man/man4//ddb.4 29 Sep 2017 09:36:04 -  1.91
> +++ share/man/man4//ddb.4 26 Oct 2017 08:18:44 -
> @@ -379,6 +379,10 @@ Just halt.
>  Just reboot.
>  .It Ic boot poweroff
>  Power down the machine whenever possible; if it fails, just halt.
> +.It Ic boot reset
> +Restart the machine by resetting the CPU. Useful in situations were
> +.Ic boot reboot
> +does not work anymore.
>  .El
>  .\" 
>  .It Xo
> Index: sys/ddb/db_command.c
> ===
> RCS file: /cvs/src/sys/ddb/db_command.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 db_command.c
> --- sys/ddb/db_command.c  19 Oct 2017 16:58:05 -  1.79
> +++ sys/ddb/db_command.c  26 Oct 2017 08:18:55 -
> @@ -105,6 +105,7 @@ void  db_boot_dump_cmd(db_expr_t, int, db
>  void db_boot_halt_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_reboot_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_boot_poweroff_cmd(db_expr_t, int, db_expr_t, char *);
> +void db_boot_reset_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_stack_trace_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_dmesg_cmd(db_expr_t, int, db_expr_t, char *);
>  void db_show_panic_cmd(db_expr_t, int, db_expr_t, char *);
> @@ -606,6 +607,7 @@ struct db_command db_boot_cmds[] = {
>   { "halt",   db_boot_halt_cmd,   0,  0 },
>   { "reboot", db_boot_reboot_cmd, 0,  0 },
>   { "poweroff",   db_boot_poweroff_cmd,   0,  0 },
> + { "reset",  db_boot_reset_cmd,  0,  0 },
>   { NULL, }
>  };
> @@ -812,6 +814,12 @@ void
>  db_boot_poweroff_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
>  {
>   reboot(RB_NOSYNC | RB_HALT | RB_POWERDOWN | RB_TIMEBAD | RB_USERREQ);
> +}
> +
> +void
> +db_boot_reset_cmd(db_expr_t addr, int haddr, db_expr_t count, char *modif)
> +{
> + cpu_reset();
>  }
>  void
>