[v6] RISCV device tree mapping

[PATCH v6 1/9] xen/riscv: prevent recursion when ASSERT(), BUG*(), or panic() are called

Posted by Oleksii Kurochko 2 weeks, 2 days ago

Implement machine_restart() using printk() to prevent recursion that
occurs when ASSERT(), BUG*(), or panic() are invoked.
All these macros (except panic() which could be called directly)
eventually call panic(), which then calls machine_restart(),
leading to a recursive loop.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v6:
 - new patch.
---
 xen/arch/riscv/stubs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 3285d18899..144f1250e1 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -53,7 +53,7 @@ void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
 
 void machine_restart(unsigned int delay_millisecs)
 {
-    BUG_ON("unimplemented");
+    printk("%s: unimplemented\n", __func__);
 }
 
 void machine_halt(void)
-- 
2.46.0

Re: [PATCH v6 1/9] xen/riscv: prevent recursion when ASSERT(), BUG*(), or panic() are called

Posted by Jan Beulich 1 week, 1 day ago

On 02.09.2024 19:01, Oleksii Kurochko wrote:
> Implement machine_restart() using printk() to prevent recursion that
> occurs when ASSERT(), BUG*(), or panic() are invoked.
> All these macros (except panic() which could be called directly)
> eventually call panic(), which then calls machine_restart(),
> leading to a recursive loop.

Right, that pretty likely was an oversight. Yet then ...

> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -53,7 +53,7 @@ void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
>  
>  void machine_restart(unsigned int delay_millisecs)
>  {
> -    BUG_ON("unimplemented");
> +    printk("%s: unimplemented\n", __func__);
>  }

... you still want to halt execution here, by (re?)adding a for() loop
of the kind you at least had in a few places earlier on. The function
is declared noreturn after all, which you're now violating. I'm
actually surprised the compiler didn't complain to you.

The same is also going to be needed for machine_halt(), btw: As soon
as you get far enough to parse the command line, "noreboot" on the
command line would have crashes end up there, not here.

Jan

Re: [PATCH v6 1/9] xen/riscv: prevent recursion when ASSERT(), BUG*(), or panic() are called

Posted by oleksii.kurochko@gmail.com 1 week, 1 day ago

On Tue, 2024-09-10 at 11:42 +0200, Jan Beulich wrote:
> On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > Implement machine_restart() using printk() to prevent recursion
> > that
> > occurs when ASSERT(), BUG*(), or panic() are invoked.
> > All these macros (except panic() which could be called directly)
> > eventually call panic(), which then calls machine_restart(),
> > leading to a recursive loop.
> 
> Right, that pretty likely was an oversight. Yet then ...
> 
> > --- a/xen/arch/riscv/stubs.c
> > +++ b/xen/arch/riscv/stubs.c
> > @@ -53,7 +53,7 @@ void domain_set_time_offset(struct domain *d,
> > int64_t time_offset_seconds)
> >  
> >  void machine_restart(unsigned int delay_millisecs)
> >  {
> > -    BUG_ON("unimplemented");
> > +    printk("%s: unimplemented\n", __func__);
> >  }
> 
> ... you still want to halt execution here, by (re?)adding a for()
> loop
> of the kind you at least had in a few places earlier on. The function
> is declared noreturn after all, which you're now violating. I'm
> actually surprised the compiler didn't complain to you.
> 
> The same is also going to be needed for machine_halt(), btw: As soon
> as you get far enough to parse the command line, "noreboot" on the
> command line would have crashes end up there, not here.

I will drop this patch in the next version as Andrew C. provides the
patch:
https://gitlab.com/xen-project/people/olkur/xen/-/commit/ea6d5a148970a7f8066e51e64fe67a9bd51e3084


~ Oleksii

[PATCH] RISCV/shutdown: Implement machine_{halt,restart}()

Posted by Andrew Cooper 2 weeks, 1 day ago

SBI has an API for shutdown so wire it up.  However, the spec does allow the
call not to be implemented, so we have to cope with the call return returning.

There is a reboot-capable SBI extention, but in the short term route route
machine_restart() into machine_halt().

Then, use use machine_halt() rather than an infinite loop at the end of
start_xen().  This avoids the Qemu smoke test needing to wait for the full
timeout in order to succeed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>

As per commit e44f33ccddc2 ("ppc/shutdown: Implement
machine_{halt,restart}()")

Simply replacing BUG() with a printk() is just swapping one problem for
another.
---
 xen/arch/riscv/Makefile          |  1 +
 xen/arch/riscv/include/asm/sbi.h |  3 +++
 xen/arch/riscv/sbi.c             |  5 +++++
 xen/arch/riscv/setup.c           |  6 ++----
 xen/arch/riscv/shutdown.c        | 25 +++++++++++++++++++++++++
 xen/arch/riscv/stubs.c           | 12 ------------
 6 files changed, 36 insertions(+), 16 deletions(-)
 create mode 100644 xen/arch/riscv/shutdown.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 81b77b13d652..d192be7b552a 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -4,6 +4,7 @@ obj-y += mm.o
 obj-$(CONFIG_RISCV_64) += riscv64/
 obj-y += sbi.o
 obj-y += setup.o
+obj-y += shutdown.o
 obj-y += stubs.o
 obj-y += traps.o
 obj-y += vm_event.o
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index 0e6820a4eda3..4d72a2295e72 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -13,6 +13,7 @@
 #define __ASM_RISCV_SBI_H__
 
 #define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
+#define SBI_EXT_0_1_SHUTDOWN			0x8
 
 struct sbiret {
     long error;
@@ -31,4 +32,6 @@ struct sbiret sbi_ecall(unsigned long ext, unsigned long fid,
  */
 void sbi_console_putchar(int ch);
 
+void sbi_shutdown(void);
+
 #endif /* __ASM_RISCV_SBI_H__ */
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
index 0ae166c8610e..c7984344bc6b 100644
--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -42,3 +42,8 @@ void sbi_console_putchar(int ch)
 {
     sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
 }
+
+void sbi_shutdown(void)
+{
+    sbi_ecall(SBI_EXT_0_1_SHUTDOWN, 0, 0, 0, 0, 0, 0, 0);
+}
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index a6a29a150869..bf9078f36aff 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -4,6 +4,7 @@
 #include <xen/compile.h>
 #include <xen/init.h>
 #include <xen/mm.h>
+#include <xen/shutdown.h>
 
 #include <public/version.h>
 
@@ -28,8 +29,5 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
 
     printk("All set up\n");
 
-    for ( ;; )
-        asm volatile ("wfi");
-
-    unreachable();
+    machine_halt();
 }
diff --git a/xen/arch/riscv/shutdown.c b/xen/arch/riscv/shutdown.c
new file mode 100644
index 000000000000..270bb26b68a6
--- /dev/null
+++ b/xen/arch/riscv/shutdown.c
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <xen/shutdown.h>
+
+#include <asm/sbi.h>
+
+void machine_halt(void)
+{
+    sbi_shutdown();
+
+    for ( ;; )
+        asm volatile ("wfi");
+
+    unreachable();
+}
+
+void machine_restart(unsigned int delay_millisecs)
+{
+    /*
+     * TODO: mdelay(delay_millisecs)
+     * TODO: Probe for #SRST support, where sbi_system_reset() has a
+     *       shutdown/reboot parameter.
+     */
+
+    machine_halt();
+}
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 3285d1889940..2aa245f272b5 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -49,18 +49,6 @@ void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
     BUG_ON("unimplemented");
 }
 
-/* shutdown.c */
-
-void machine_restart(unsigned int delay_millisecs)
-{
-    BUG_ON("unimplemented");
-}
-
-void machine_halt(void)
-{
-    BUG_ON("unimplemented");
-}
-
 /* domctl.c */
 
 long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,

base-commit: 1e6bb29b03680a9d0e12f14c4d406a0d67317ea7
-- 
2.39.2