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

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

Posted by Andrew Cooper 2 months, 2 weeks 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

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

Posted by oleksii.kurochko@gmail.com 2 months, 2 weeks ago

On Tue, 2024-09-03 at 15:19 +0100, Andrew Cooper wrote:
> 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>
LGTM:
 Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks for the patch.

~ Oleksii

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

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

Posted by Jan Beulich 2 months, 2 weeks ago

On 03.09.2024 16:19, Andrew Cooper wrote:
> 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>

Acked-by: Jan Beulich <jbeulich@suse.com>
ideally with ...

> --- /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");

... the missing blanks added here, as you move that loop around.

Jan

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

Posted by Andrew Cooper 2 months, 2 weeks ago

On 03/09/2024 3:26 pm, Jan Beulich wrote:
> On 03.09.2024 16:19, Andrew Cooper wrote:
>> 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>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> ideally with ...
>
>> --- /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");
> ... the missing blanks added here, as you move that loop around.

Ah yes.  Will do.

~Andrew

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

Posted by Andrew Cooper 2 months, 2 weeks ago

On 03/09/2024 3:19 pm, Andrew Cooper wrote:
> 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.

Sorry, this is supposed to read "... cope with sbi_shutdown() returning."

~Andrew

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

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

Posted by Jan Beulich 2 months, 2 weeks ago

On 03.09.2024 16:23, Andrew Cooper wrote:
> On 03/09/2024 3:19 pm, Andrew Cooper wrote:
>> 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.
> 
> Sorry, this is supposed to read "... cope with sbi_shutdown() returning."

And then perhaps also ...

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

... one "route" dropped from this sentence?

Jan