[PATCH v1 0/8] Basic early_printk and smoke test implementation

[PATCH v1 0/8] Basic early_printk and smoke test implementation

[Oleksii Kurochko]

The patch series introduces the following:
- the minimal set of headers and changes inside them.
- SBI (RISC-V Supervisor Binary Interface) things necessary for basic
  early_printk implementation.
- things needed to set up the stack.
- early_printk() function to print only strings.
- RISC-V smoke test which checks if  "Hello from C env" message is
  present in serial.tmp

Oleksii Kurochko (8):
  xen/riscv: introduce dummy asm/init.h
  xen/riscv: introduce asm/types.h header file
  xen/riscv: introduce stack stuff
  xen/riscv: introduce sbi call to putchar to console
  xen/include: include <asm/types.h> in <xen/early_printk.h>
  xen/riscv: introduce early_printk basic stuff
  xen/riscv: print hello message from C env
  automation: add RISC-V smoke test

 automation/build/archlinux/riscv64.dockerfile |  3 +-
 automation/scripts/qemu-smoke-riscv64.sh      | 20 +++++
 xen/arch/riscv/Kconfig.debug                  |  7 ++
 xen/arch/riscv/Makefile                       |  3 +
 xen/arch/riscv/early_printk.c                 | 27 +++++++
 xen/arch/riscv/include/asm/early_printk.h     | 12 +++
 xen/arch/riscv/include/asm/init.h             | 12 +++
 xen/arch/riscv/include/asm/sbi.h              | 34 +++++++++
 xen/arch/riscv/include/asm/types.h            | 73 +++++++++++++++++++
 xen/arch/riscv/riscv64/head.S                 |  6 +-
 xen/arch/riscv/sbi.c                          | 44 +++++++++++
 xen/arch/riscv/setup.c                        | 18 +++++
 xen/include/xen/early_printk.h                |  2 +
 13 files changed, 259 insertions(+), 2 deletions(-)
 create mode 100755 automation/scripts/qemu-smoke-riscv64.sh
 create mode 100644 xen/arch/riscv/early_printk.c
 create mode 100644 xen/arch/riscv/include/asm/early_printk.h
 create mode 100644 xen/arch/riscv/include/asm/init.h
 create mode 100644 xen/arch/riscv/include/asm/sbi.h
 create mode 100644 xen/arch/riscv/include/asm/types.h
 create mode 100644 xen/arch/riscv/sbi.c
 create mode 100644 xen/arch/riscv/setup.c

-- 
2.38.1

[PATCH v1 1/8] xen/riscv: introduce dummy asm/init.h

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/init.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/init.h

diff --git a/xen/arch/riscv/include/asm/init.h b/xen/arch/riscv/include/asm/init.h
new file mode 100644
index 0000000000..237ec25e4e
--- /dev/null
+++ b/xen/arch/riscv/include/asm/init.h
@@ -0,0 +1,12 @@
+#ifndef _XEN_ASM_INIT_H
+#define _XEN_ASM_INIT_H
+
+#endif /* _XEN_ASM_INIT_H */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.38.1

Re: [PATCH v1 1/8] xen/riscv: introduce dummy asm/init.h

[Julien Grall]

On 06/01/2023 13:14, Oleksii Kurochko wrote:

I am guessing this is necessary because you will use <xen/init.h> will 
be used later on.

If so, then I think it would be useful for mention it in the commit message.

> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v1 1/8] xen/riscv: introduce dummy asm/init.h

[Oleksii]

Thanks for the comments. I'll update commit message in the next
patch series.

~Oleksii
On Fri, 2023-01-06 at 13:42 +0000, Julien Grall wrote:
> On 06/01/2023 13:14, Oleksii Kurochko wrote:
> 
> I am guessing this is necessary because you will use <xen/init.h>
> will 
> be used later on.
> 
> If so, then I think it would be useful for mention it in the commit
> message.
> 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 

[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/types.h | 73 ++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/types.h

diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h
new file mode 100644
index 0000000000..48f27f97ba
--- /dev/null
+++ b/xen/arch/riscv/include/asm/types.h
@@ -0,0 +1,73 @@
+#ifndef __RISCV_TYPES_H__
+#define __RISCV_TYPES_H__
+
+#ifndef __ASSEMBLY__
+
+typedef __signed__ char __s8;
+typedef unsigned char __u8;
+
+typedef __signed__ short __s16;
+typedef unsigned short __u16;
+
+typedef __signed__ int __s32;
+typedef unsigned int __u32;
+
+#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
+#if defined(CONFIG_RISCV_32)
+typedef __signed__ long long __s64;
+typedef unsigned long long __u64;
+#elif defined (CONFIG_RISCV_64)
+typedef __signed__ long __s64;
+typedef unsigned long __u64;
+#endif
+#endif
+
+typedef signed char s8;
+typedef unsigned char u8;
+
+typedef signed short s16;
+typedef unsigned short u16;
+
+typedef signed int s32;
+typedef unsigned int u32;
+
+#if defined(CONFIG_RISCV_32)
+typedef signed long long s64;
+typedef unsigned long long u64;
+typedef u32 vaddr_t;
+#define PRIvaddr PRIx32
+typedef u64 paddr_t;
+#define INVALID_PADDR (~0ULL)
+#define PRIpaddr "016llx"
+typedef u32 register_t;
+#define PRIregister "x"
+#elif defined (CONFIG_RISCV_64)
+typedef signed long s64;
+typedef unsigned long u64;
+typedef u64 vaddr_t;
+#define PRIvaddr PRIx64
+typedef u64 paddr_t;
+#define INVALID_PADDR (~0UL)
+#define PRIpaddr "016lx"
+typedef u64 register_t;
+#define PRIregister "lx"
+#endif
+
+#if defined(__SIZE_TYPE__)
+typedef __SIZE_TYPE__ size_t;
+#else
+typedef unsigned long size_t;
+#endif
+typedef signed long ssize_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __RISCV_TYPES_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.38.1

Re: [PATCH v1 2/8] xen/riscv: introduce asm/types.h header file

[Jan Beulich]

On 06.01.2023 14:14, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/include/asm/types.h | 73 ++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/types.h
> 
> diff --git a/xen/arch/riscv/include/asm/types.h b/xen/arch/riscv/include/asm/types.h
> new file mode 100644
> index 0000000000..48f27f97ba
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/types.h
> @@ -0,0 +1,73 @@
> +#ifndef __RISCV_TYPES_H__
> +#define __RISCV_TYPES_H__
> +
> +#ifndef __ASSEMBLY__
> +
> +typedef __signed__ char __s8;
> +typedef unsigned char __u8;
> +
> +typedef __signed__ short __s16;
> +typedef unsigned short __u16;
> +
> +typedef __signed__ int __s32;
> +typedef unsigned int __u32;
> +
> +#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> +#if defined(CONFIG_RISCV_32)
> +typedef __signed__ long long __s64;
> +typedef unsigned long long __u64;
> +#elif defined (CONFIG_RISCV_64)
> +typedef __signed__ long __s64;
> +typedef unsigned long __u64;
> +#endif
> +#endif

Of these, only the ones actually needed should be introduced. We're
in the process of phasing out especially the above, but also ...

> +typedef signed char s8;
> +typedef unsigned char u8;
> +
> +typedef signed short s16;
> +typedef unsigned short u16;
> +
> +typedef signed int s32;
> +typedef unsigned int u32;
> +
> +#if defined(CONFIG_RISCV_32)
> +typedef signed long long s64;
> +typedef unsigned long long u64;

... all of these.

> +typedef u32 vaddr_t;

(New) consumers of such types should therefore use {u,}int<N>_t instead.

Jan

Re: [PATCH v1 2/8] xen/riscv: introduce asm/types.h header file

[Oleksii]

On Fri, 2023-01-06 at 15:12 +0100, Jan Beulich wrote:
> On 06.01.2023 14:14, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >  xen/arch/riscv/include/asm/types.h | 73
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644 xen/arch/riscv/include/asm/types.h
> > 
> > diff --git a/xen/arch/riscv/include/asm/types.h
> > b/xen/arch/riscv/include/asm/types.h
> > new file mode 100644
> > index 0000000000..48f27f97ba
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/types.h
> > @@ -0,0 +1,73 @@
> > +#ifndef __RISCV_TYPES_H__
> > +#define __RISCV_TYPES_H__
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +typedef __signed__ char __s8;
> > +typedef unsigned char __u8;
> > +
> > +typedef __signed__ short __s16;
> > +typedef unsigned short __u16;
> > +
> > +typedef __signed__ int __s32;
> > +typedef unsigned int __u32;
> > +
> > +#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
> > +#if defined(CONFIG_RISCV_32)
> > +typedef __signed__ long long __s64;
> > +typedef unsigned long long __u64;
> > +#elif defined (CONFIG_RISCV_64)
> > +typedef __signed__ long __s64;
> > +typedef unsigned long __u64;
> > +#endif
> > +#endif
> 
> Of these, only the ones actually needed should be introduced. We're
> in the process of phasing out especially the above, but also ...
> 
Got it. I will take it into account when the next version of patch
series will be ready.
> > +typedef signed char s8;
> > +typedef unsigned char u8;
> > +
> > +typedef signed short s16;
> > +typedef unsigned short u16;
> > +
> > +typedef signed int s32;
> > +typedef unsigned int u32;
> > +
> > +#if defined(CONFIG_RISCV_32)
> > +typedef signed long long s64;
> > +typedef unsigned long long u64;
> 
> ... all of these.
> 
> > +typedef u32 vaddr_t;
> 
> (New) consumers of such types should therefore use {u,}int<N>_t
> instead.
> 
> Jan

[PATCH v1 3/8] xen/riscv: introduce stack stuff

[Oleksii Kurochko]

The patch introduces and sets up a stack in order to go to C environment

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Makefile       | 1 +
 xen/arch/riscv/riscv64/head.S | 8 +++++++-
 xen/arch/riscv/setup.c        | 6 ++++++
 3 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/riscv/setup.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 248f2cbb3e..5a67a3f493 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_RISCV_64) += riscv64/
+obj-y += setup.o
 
 $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 990edb70a0..ddc7104701 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,4 +1,10 @@
         .section .text.header, "ax", %progbits
 
 ENTRY(start)
-        j  start
+        la      sp, cpu0_boot_stack
+        li      t0, PAGE_SIZE
+        add     sp, sp, t0
+
+_start_hang:
+        wfi
+        j  _start_hang
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
new file mode 100644
index 0000000000..2c7dca1daa
--- /dev/null
+++ b/xen/arch/riscv/setup.c
@@ -0,0 +1,6 @@
+#include <xen/init.h>
+#include <xen/compile.h>
+
+/* Xen stack for bringing up the first CPU. */
+unsigned char __initdata cpu0_boot_stack[PAGE_SIZE]
+    __aligned(PAGE_SIZE);
-- 
2.38.1

Re: [PATCH v1 3/8] xen/riscv: introduce stack stuff

[Julien Grall]

Hi,

On 06/01/2023 13:14, Oleksii Kurochko wrote:
> The patch introduces and sets up a stack in order to go to C environment
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   xen/arch/riscv/Makefile       | 1 +
>   xen/arch/riscv/riscv64/head.S | 8 +++++++-
>   xen/arch/riscv/setup.c        | 6 ++++++
>   3 files changed, 14 insertions(+), 1 deletion(-)
>   create mode 100644 xen/arch/riscv/setup.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 248f2cbb3e..5a67a3f493 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,4 +1,5 @@
>   obj-$(CONFIG_RISCV_64) += riscv64/
> +obj-y += setup.o
>   
>   $(TARGET): $(TARGET)-syms
>   	$(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 990edb70a0..ddc7104701 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,4 +1,10 @@
>           .section .text.header, "ax", %progbits
>   
>   ENTRY(start)
> -        j  start
> +        la      sp, cpu0_boot_stack
> +        li      t0, PAGE_SIZE

I would recommend to a define STACK_SIZE. So you don't make assumption 
on the size in the code and it is easier to update.

> +        add     sp, sp, t0
> +
> +_start_hang:
> +        wfi
> +        j  _start_hang
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> new file mode 100644
> index 0000000000..2c7dca1daa
> --- /dev/null
> +++ b/xen/arch/riscv/setup.c
> @@ -0,0 +1,6 @@
> +#include <xen/init.h>
> +#include <xen/compile.h>
> +
> +/* Xen stack for bringing up the first CPU. */
> +unsigned char __initdata cpu0_boot_stack[PAGE_SIZE]
> +    __aligned(PAGE_SIZE);

Cheers,

-- 
Julien Grall

Re: [PATCH v1 3/8] xen/riscv: introduce stack stuff

[Oleksii]

Hi,

On Fri, 2023-01-06 at 13:54 +0000, Julien Grall wrote:
> Hi,
> 
> On 06/01/2023 13:14, Oleksii Kurochko wrote:
> > The patch introduces and sets up a stack in order to go to C
> > environment
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   xen/arch/riscv/Makefile       | 1 +
> >   xen/arch/riscv/riscv64/head.S | 8 +++++++-
> >   xen/arch/riscv/setup.c        | 6 ++++++
> >   3 files changed, 14 insertions(+), 1 deletion(-)
> >   create mode 100644 xen/arch/riscv/setup.c
> > 
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 248f2cbb3e..5a67a3f493 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,4 +1,5 @@
> >   obj-$(CONFIG_RISCV_64) += riscv64/
> > +obj-y += setup.o
> >   
> >   $(TARGET): $(TARGET)-syms
> >         $(OBJCOPY) -O binary -S $< $@
> > diff --git a/xen/arch/riscv/riscv64/head.S
> > b/xen/arch/riscv/riscv64/head.S
> > index 990edb70a0..ddc7104701 100644
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -1,4 +1,10 @@
> >           .section .text.header, "ax", %progbits
> >   
> >   ENTRY(start)
> > -        j  start
> > +        la      sp, cpu0_boot_stack
> > +        li      t0, PAGE_SIZE
> 
> I would recommend to a define STACK_SIZE. So you don't make
> assumption 
> on the size in the code and it is easier to update.
> 
Thanks. I decied to not define STACK_SIZE because it is used now only
once and the STACK_SIZE that was introduced in Bobby's patch series was
too big at least for current purpose.

Any way it probably makes sensne to intrdouce STACK_SIZE from the start
so will take it into account during a work at new patch series.
> > +        add     sp, sp, t0
> > +
> > +_start_hang:
> > +        wfi
> > +        j  _start_hang
> > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > new file mode 100644
> > index 0000000000..2c7dca1daa
> > --- /dev/null
> > +++ b/xen/arch/riscv/setup.c
> > @@ -0,0 +1,6 @@
> > +#include <xen/init.h>
> > +#include <xen/compile.h>
> > +
> > +/* Xen stack for bringing up the first CPU. */
> > +unsigned char __initdata cpu0_boot_stack[PAGE_SIZE]
> > +    __aligned(PAGE_SIZE);
> 
> Cheers,
> 

Re: [PATCH v1 3/8] xen/riscv: introduce stack stuff

[Jan Beulich]

On 06.01.2023 14:14, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,4 +1,10 @@
>          .section .text.header, "ax", %progbits
>  
>  ENTRY(start)
> -        j  start
> +        la      sp, cpu0_boot_stack
> +        li      t0, PAGE_SIZE
> +        add     sp, sp, t0
> +
> +_start_hang:
> +        wfi
> +        j  _start_hang

Nit: I think it would be nice if in a new port assembly code used
consistent padding between insn mnemonic and operand(s).

Jan

Re: [PATCH v1 3/8] xen/riscv: introduce stack stuff

[Oleksii]

On Fri, 2023-01-06 at 15:15 +0100, Jan Beulich wrote:
> On 06.01.2023 14:14, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -1,4 +1,10 @@
> >          .section .text.header, "ax", %progbits
> >  
> >  ENTRY(start)
> > -        j  start
> > +        la      sp, cpu0_boot_stack
> > +        li      t0, PAGE_SIZE
> > +        add     sp, sp, t0
> > +
> > +_start_hang:
> > +        wfi
> > +        j  _start_hang
> 
> Nit: I think it would be nice if in a new port assembly code used
> consistent padding between insn mnemonic and operand(s).
> 
Missed it.
Actually it will be "fixed" in the later patches of the patch series
but i'll fix this while working on a new version of the patch series.

~Oleksii
> Jan

Re: [PATCH v1 3/8] xen/riscv: introduce stack stuff

[Andrew Cooper]

On 06/01/2023 1:14 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 990edb70a0..ddc7104701 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,4 +1,10 @@
>          .section .text.header, "ax", %progbits
>  
>  ENTRY(start)
> -        j  start
> +        la      sp, cpu0_boot_stack
> +        li      t0, PAGE_SIZE
> +        add     sp, sp, t0
> +
> +_start_hang:
> +        wfi
> +        j  _start_hang
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> new file mode 100644
> index 0000000000..2c7dca1daa
> --- /dev/null
> +++ b/xen/arch/riscv/setup.c
> @@ -0,0 +1,6 @@
> +#include <xen/init.h>
> +#include <xen/compile.h>
> +
> +/* Xen stack for bringing up the first CPU. */
> +unsigned char __initdata cpu0_boot_stack[PAGE_SIZE]
> +    __aligned(PAGE_SIZE);

Ah, I didn't spot his when looking at the unified delta of the series.

You want most of patch 7 merged into this, so we end up with

+void __init noreturn start_xen(void)
+{
+    for ( ;; )
+        asm volatile ("wfi");
+
+    unreachable();
+}

in setup.c too.  That means we don't transiently add _start_hang just to
delete it 3 patches later.  (And it will fix Jan's observation about the
misaligned operand.)

Adding the early_printk("Hello from C env\n"); line wants merging into
patch 6.

~Andrew

[PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Oleksii Kurochko]

The patch introduce sbi_putchar() SBI call which is necessary
to implement initial early_printk

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Makefile          |  1 +
 xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
 xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/sbi.h
 create mode 100644 xen/arch/riscv/sbi.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 5a67a3f493..60db415654 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_RISCV_64) += riscv64/
 obj-y += setup.o
+obj-y += sbi.o
 
 $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
new file mode 100644
index 0000000000..34b53f8eaf
--- /dev/null
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: (GPL-2.0-or-later) */
+/*
+ * Copyright (c) 2021 Vates SAS.
+ *
+ * Taken from xvisor, modified by Bobby Eshleman (bobby.eshleman@gmail.com).
+ *
+ * Taken/modified from Xvisor project with the following copyright:
+ *
+ * Copyright (c) 2019 Western Digital Corporation or its affiliates.
+ */
+
+#ifndef __CPU_SBI_H__
+#define __CPU_SBI_H__
+
+#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
+
+struct sbiret {
+    long error;
+    long value;
+};
+
+struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
+        unsigned long arg1, unsigned long arg2,
+        unsigned long arg3, unsigned long arg4,
+        unsigned long arg5);
+
+/**
+ * Writes given character to the console device.
+ *
+ * @param ch The data to be written to the console.
+ */
+void sbi_console_putchar(int ch);
+
+#endif // __CPU_SBI_H__
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
new file mode 100644
index 0000000000..67cf5dd982
--- /dev/null
+++ b/xen/arch/riscv/sbi.c
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Taken and modified from the xvisor project with the copyright Copyright (c)
+ * 2019 Western Digital Corporation or its affiliates and author Anup Patel
+ * (anup.patel@wdc.com).
+ *
+ * Modified by Bobby Eshleman (bobby.eshleman@gmail.com).
+ *
+ * Copyright (c) 2019 Western Digital Corporation or its affiliates.
+ * Copyright (c) 2021 Vates SAS.
+ */
+
+#include <xen/errno.h>
+#include <asm/sbi.h>
+
+struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
+            unsigned long arg1, unsigned long arg2,
+            unsigned long arg3, unsigned long arg4,
+            unsigned long arg5)
+{
+    struct sbiret ret;
+    register unsigned long a0 asm ("a0") = arg0;
+    register unsigned long a1 asm ("a1") = arg1;
+    register unsigned long a2 asm ("a2") = arg2;
+    register unsigned long a3 asm ("a3") = arg3;
+    register unsigned long a4 asm ("a4") = arg4;
+    register unsigned long a5 asm ("a5") = arg5;
+    register unsigned long a6 asm ("a6") = fid;
+    register unsigned long a7 asm ("a7") = ext;
+
+    asm volatile ("ecall"
+              : "+r" (a0), "+r" (a1)
+              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
+              : "memory");
+    ret.error = a0;
+    ret.value = a1;
+
+    return ret;
+}
+
+void sbi_console_putchar(int ch)
+{
+    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
+}
-- 
2.38.1

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Bobby Eshleman]

On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
> The patch introduce sbi_putchar() SBI call which is necessary
> to implement initial early_printk
> 

I think that it might be wise to start off with an alternative to
sbi_putchar() since it is already planned for deprecation. I realize
that this will require rework, but it is almost guaranteed that
early_printk() will break on future SBI implementations if using this
SBI call. IIRC, Xen/ARM's early printk looked like a reasonable analogy
for how it could work on RISC-V, IMHO.

> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/Makefile          |  1 +
>  xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>  xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/sbi.h
>  create mode 100644 xen/arch/riscv/sbi.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 5a67a3f493..60db415654 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_RISCV_64) += riscv64/
>  obj-y += setup.o
> +obj-y += sbi.o
>  
>  $(TARGET): $(TARGET)-syms
>  	$(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
> new file mode 100644
> index 0000000000..34b53f8eaf
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later) */
> +/*
> + * Copyright (c) 2021 Vates SAS.
> + *
> + * Taken from xvisor, modified by Bobby Eshleman (bobby.eshleman@gmail.com).
> + *
> + * Taken/modified from Xvisor project with the following copyright:
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + */
> +
> +#ifndef __CPU_SBI_H__
> +#define __CPU_SBI_H__
> +
> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
> +
> +struct sbiret {
> +    long error;
> +    long value;
> +};
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +        unsigned long arg1, unsigned long arg2,
> +        unsigned long arg3, unsigned long arg4,
> +        unsigned long arg5);
> +
> +/**
> + * Writes given character to the console device.
> + *
> + * @param ch The data to be written to the console.
> + */
> +void sbi_console_putchar(int ch);
> +
> +#endif // __CPU_SBI_H__
> diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
> new file mode 100644
> index 0000000000..67cf5dd982
> --- /dev/null
> +++ b/xen/arch/riscv/sbi.c
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Taken and modified from the xvisor project with the copyright Copyright (c)
> + * 2019 Western Digital Corporation or its affiliates and author Anup Patel
> + * (anup.patel@wdc.com).
> + *
> + * Modified by Bobby Eshleman (bobby.eshleman@gmail.com).
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + * Copyright (c) 2021 Vates SAS.
> + */
> +
> +#include <xen/errno.h>
> +#include <asm/sbi.h>
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +            unsigned long arg1, unsigned long arg2,
> +            unsigned long arg3, unsigned long arg4,
> +            unsigned long arg5)
> +{
> +    struct sbiret ret;
> +    register unsigned long a0 asm ("a0") = arg0;
> +    register unsigned long a1 asm ("a1") = arg1;
> +    register unsigned long a2 asm ("a2") = arg2;
> +    register unsigned long a3 asm ("a3") = arg3;
> +    register unsigned long a4 asm ("a4") = arg4;
> +    register unsigned long a5 asm ("a5") = arg5;
> +    register unsigned long a6 asm ("a6") = fid;
> +    register unsigned long a7 asm ("a7") = ext;
> +
> +    asm volatile ("ecall"
> +              : "+r" (a0), "+r" (a1)
> +              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
> +              : "memory");
> +    ret.error = a0;
> +    ret.value = a1;
> +
> +    return ret;
> +}
> +
> +void sbi_console_putchar(int ch)
> +{
> +    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
> +}
> -- 
> 2.38.1
> 
> 

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Andrew Cooper]

On 20/12/2022 6:23 am, Bobby Eshleman wrote:
> On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
>> The patch introduce sbi_putchar() SBI call which is necessary
>> to implement initial early_printk
>>
> I think that it might be wise to start off with an alternative to
> sbi_putchar() since it is already planned for deprecation. I realize
> that this will require rework, but it is almost guaranteed that
> early_printk() will break on future SBI implementations if using this
> SBI call. IIRC, Xen/ARM's early printk looked like a reasonable analogy
> for how it could work on RISC-V, IMHO.

Hmm, we're using it as a stopgap right now in CI so we can break
"upstream RISC-V support" into manageable chunks.

So perhaps we want to forgo plumbing sbi_putchar() into early_printk() 
(to avoid giving the impression that this will stay around in the
future) and use sbi_putchar() directly for the hello world print.

Next, we focus on getting the real uart driver working along with real
printk (rather than focusing on exceptions which was going to be the
next task), and we can drop sbi_putchar() entirely.

Thoughts?

~Andrew

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Bobby Eshleman]

On Fri, Jan 06, 2023 at 05:16:31PM +0000, Andrew Cooper wrote:
> On 20/12/2022 6:23 am, Bobby Eshleman wrote:
> > On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
> >> The patch introduce sbi_putchar() SBI call which is necessary
> >> to implement initial early_printk
> >>
> > I think that it might be wise to start off with an alternative to
> > sbi_putchar() since it is already planned for deprecation. I realize
> > that this will require rework, but it is almost guaranteed that
> > early_printk() will break on future SBI implementations if using this
> > SBI call. IIRC, Xen/ARM's early printk looked like a reasonable analogy
> > for how it could work on RISC-V, IMHO.
> 
> Hmm, we're using it as a stopgap right now in CI so we can break
> "upstream RISC-V support" into manageable chunks.
> 
> So perhaps we want to forgo plumbing sbi_putchar() into early_printk() 
> (to avoid giving the impression that this will stay around in the
> future) and use sbi_putchar() directly for the hello world print.
> 
> Next, we focus on getting the real uart driver working along with real
> printk (rather than focusing on exceptions which was going to be the
> next task), and we can drop sbi_putchar() entirely.
> 
> Thoughts?
> 
> ~Andrew

That sounds like a reasonable approach to me.

Best,
Bobby

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Oleksii]

On Fri, 2023-01-06 at 17:16 +0000, Andrew Cooper wrote:
> On 20/12/2022 6:23 am, Bobby Eshleman wrote:
> > On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
> > > The patch introduce sbi_putchar() SBI call which is necessary
> > > to implement initial early_printk
> > > 
> > I think that it might be wise to start off with an alternative to
> > sbi_putchar() since it is already planned for deprecation. I
> > realize
> > that this will require rework, but it is almost guaranteed that
> > early_printk() will break on future SBI implementations if using
> > this
> > SBI call. IIRC, Xen/ARM's early printk looked like a reasonable
> > analogy
> > for how it could work on RISC-V, IMHO.
> 
> Hmm, we're using it as a stopgap right now in CI so we can break
> "upstream RISC-V support" into manageable chunks.
> 
> So perhaps we want to forgo plumbing sbi_putchar() into
> early_printk() 
> (to avoid giving the impression that this will stay around in the
> future) and use sbi_putchar() directly for the hello world print.
> 
> Next, we focus on getting the real uart driver working along with
> real
> printk (rather than focusing on exceptions which was going to be the
> next task), and we can drop sbi_putchar() entirely.
> 
> Thoughts?
> 
I think it would be better to remain it as it is done now and add TODO
comment "TODO: rewrite early_printk() to use uart directly as
sbi_putchar() is already planned for deprecation" and update the commit
message too.
After real uart will be ready it will be easy to remove sbi_putchar()
and do something similar to ARM early printk implementation.
> ~Andrew

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Julien Grall]

Hi,

On 06/01/2023 13:14, Oleksii Kurochko wrote:
> The patch introduce sbi_putchar() SBI call which is necessary
> to implement initial early_printk
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   xen/arch/riscv/Makefile          |  1 +
>   xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>   xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++

IMHO, it would be better to implement sbi.c in assembly so you can use 
print in the console before you jump to C world.

>   3 files changed, 79 insertions(+)
>   create mode 100644 xen/arch/riscv/include/asm/sbi.h
>   create mode 100644 xen/arch/riscv/sbi.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 5a67a3f493..60db415654 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>   obj-$(CONFIG_RISCV_64) += riscv64/
>   obj-y += setup.o
> +obj-y += sbi.o

Please order the filename alphabetically.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Andrew Cooper]

On 06/01/2023 1:40 pm, Julien Grall wrote:
> Hi,
>
> On 06/01/2023 13:14, Oleksii Kurochko wrote:
>> The patch introduce sbi_putchar() SBI call which is necessary
>> to implement initial early_printk
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>>   xen/arch/riscv/Makefile          |  1 +
>>   xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>>   xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>
> IMHO, it would be better to implement sbi.c in assembly so you can use
> print in the console before you jump to C world.

That was already requested not to happen.  Frankly, if I was an arm
maintainer I'd object to the how it's used there too, but RISCV is
massively more simple still.

Not even the pagetable setup, or the physical relocation (if even
necessary) needs doing in ASM.

I'm not completely ruling it out in the future, but someone is going to
have to come up with a very convincing argument for why they can't do
this piece of critical setup in C.

~Andrew

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Julien Grall]

Hi Andrew,

On 06/01/2023 15:19, Andrew Cooper wrote:
> On 06/01/2023 1:40 pm, Julien Grall wrote:
>> Hi,
>>
>> On 06/01/2023 13:14, Oleksii Kurochko wrote:
>>> The patch introduce sbi_putchar() SBI call which is necessary
>>> to implement initial early_printk
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>>    xen/arch/riscv/Makefile          |  1 +
>>>    xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>>>    xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>>
>> IMHO, it would be better to implement sbi.c in assembly so you can use
>> print in the console before you jump to C world.
> 
> That was already requested not to happen.  Frankly, if I was an arm
> maintainer I'd object to the how it's used there too, but RISCV is
> massively more simple still.

There are a few reasons:
   1) Xen is not fully position independent. Even if -fpic is enabled, 
you would still need apply some relocation in order if you want to use 
it before running in the virtual address space.
   2) Doing any memory access before the MMU is setup requires some 
careful though (see [1]). With function implemented in C, you can't 
really control which memory accesses are done.

> 
> Not even the pagetable setup, or the physical relocation (if even
> necessary) needs doing in ASM.
> 
> I'm not completely ruling it out in the future, but someone is going to
> have to come up with a very convincing argument for why they can't do
> this piece of critical setup in C.

That's great if RISC-V doesn't have the issues I mentioned above. On 
Arm, I don't really think we can get away. But feel free to explain how 
this could be done...

Cheers,

[1] 
https://events.static.linuxfound.org/sites/events/files/slides/slides_10.pdf

-- 
Julien Grall

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Oleksii]

Hi,

On Fri, 2023-01-06 at 13:40 +0000, Julien Grall wrote:
> Hi,
> 
> On 06/01/2023 13:14, Oleksii Kurochko wrote:
> > The patch introduce sbi_putchar() SBI call which is necessary
> > to implement initial early_printk
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   xen/arch/riscv/Makefile          |  1 +
> >   xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
> >   xen/arch/riscv/sbi.c             | 44
> > ++++++++++++++++++++++++++++++++
> 
> IMHO, it would be better to implement sbi.c in assembly so you can
> use 
> print in the console before you jump to C world.
> 
I thought that we can live with C version as we set up stack from the
start and then we can call early_printk() from assembly code too.
Is it bad approach?

> >   3 files changed, 79 insertions(+)
> >   create mode 100644 xen/arch/riscv/include/asm/sbi.h
> >   create mode 100644 xen/arch/riscv/sbi.c
> > 
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 5a67a3f493..60db415654 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,5 +1,6 @@
> >   obj-$(CONFIG_RISCV_64) += riscv64/
> >   obj-y += setup.o
> > +obj-y += sbi.o
> 
> Please order the filename alphabetically.
> 
> Cheers,
> 

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Julien Grall]

Hi Oleksii,

On 09/01/2023 09:04, Oleksii wrote:
> On Fri, 2023-01-06 at 13:40 +0000, Julien Grall wrote:
>> Hi,
>>
>> On 06/01/2023 13:14, Oleksii Kurochko wrote:
>>> The patch introduce sbi_putchar() SBI call which is necessary
>>> to implement initial early_printk
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>>    xen/arch/riscv/Makefile          |  1 +
>>>    xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>>>    xen/arch/riscv/sbi.c             | 44
>>> ++++++++++++++++++++++++++++++++
>>
>> IMHO, it would be better to implement sbi.c in assembly so you can
>> use
>> print in the console before you jump to C world.
>>
> I thought that we can live with C version as we set up stack from the
> start and then we can call early_printk() from assembly code too.
> Is it bad approach?

It depends on how early you want to call it. For Arm, we chose to use 
assembly because the C code may not be PIE (and even with PIE it may 
need some relocation work).

Andrew suggested that this may not be a problem with RISC-V. I have 
looked a bit more around and notice that the kernel is also calling some 
C function very early (like setup_vm()). But they ensure that the code 
is built with -mcmodel=medany.

It looks like you are already building Xen with this option. So all 
looks good for RISC-V. That said, I would suggest to check that 
__riscv_cmodel_medany is defined in files where you implement C function 
called from early assembly code.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Michal Orzel]

Hi Oleksii,

On 06/01/2023 14:14, Oleksii Kurochko wrote:
> 
> 
> The patch introduce sbi_putchar() SBI call which is necessary
> to implement initial early_printk
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/Makefile          |  1 +
>  xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>  xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/sbi.h
>  create mode 100644 xen/arch/riscv/sbi.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 5a67a3f493..60db415654 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_RISCV_64) += riscv64/
>  obj-y += setup.o
> +obj-y += sbi.o
> 
>  $(TARGET): $(TARGET)-syms
>         $(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
> new file mode 100644
> index 0000000000..34b53f8eaf
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later) */
> +/*
> + * Copyright (c) 2021 Vates SAS.
> + *
> + * Taken from xvisor, modified by Bobby Eshleman (bobby.eshleman@gmail.com).
> + *
> + * Taken/modified from Xvisor project with the following copyright:
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + */
> +
> +#ifndef __CPU_SBI_H__
> +#define __CPU_SBI_H__
I wonder where does CPU come from. Shouldn't this be called __ASM_RISCV_SBI_H__ ?

> +
> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR            0x1
> +
> +struct sbiret {
> +    long error;
> +    long value;
> +};
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +        unsigned long arg1, unsigned long arg2,
> +        unsigned long arg3, unsigned long arg4,
> +        unsigned long arg5);
The arguments needs to be aligned.

> +
> +/**
> + * Writes given character to the console device.
> + *
> + * @param ch The data to be written to the console.
> + */
> +void sbi_console_putchar(int ch);
> +
> +#endif // __CPU_SBI_H__
// should be replaced with /* */

> diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
> new file mode 100644
> index 0000000000..67cf5dd982
> --- /dev/null
> +++ b/xen/arch/riscv/sbi.c
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Taken and modified from the xvisor project with the copyright Copyright (c)
> + * 2019 Western Digital Corporation or its affiliates and author Anup Patel
> + * (anup.patel@wdc.com).
> + *
> + * Modified by Bobby Eshleman (bobby.eshleman@gmail.com).
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + * Copyright (c) 2021 Vates SAS.
> + */
> +
> +#include <xen/errno.h>
> +#include <asm/sbi.h>
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +            unsigned long arg1, unsigned long arg2,
> +            unsigned long arg3, unsigned long arg4,
> +            unsigned long arg5)
The arguments needs to be aligned.

> +{
> +    struct sbiret ret;
Could you please add an empty line here.

> +    register unsigned long a0 asm ("a0") = arg0;
> +    register unsigned long a1 asm ("a1") = arg1;
> +    register unsigned long a2 asm ("a2") = arg2;
> +    register unsigned long a3 asm ("a3") = arg3;
> +    register unsigned long a4 asm ("a4") = arg4;
> +    register unsigned long a5 asm ("a5") = arg5;
> +    register unsigned long a6 asm ("a6") = fid;
> +    register unsigned long a7 asm ("a7") = ext;
> +
> +    asm volatile ("ecall"
> +              : "+r" (a0), "+r" (a1)
> +              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
> +              : "memory");
> +    ret.error = a0;
> +    ret.value = a1;
> +
> +    return ret;
> +}
> +
> +void sbi_console_putchar(int ch)
> +{
> +    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
> +}
> --
> 2.38.1
> 
> 

~Michal

Re: [PATCH v1 4/8] xen/riscv: introduce sbi call to putchar to console

[Oleksii]

Hi Michal,

On Fri, 2023-01-06 at 16:19 +0100, Michal Orzel wrote:
> Hi Oleksii,
> 
> On 06/01/2023 14:14, Oleksii Kurochko wrote:
> > 
> > 
> > The patch introduce sbi_putchar() SBI call which is necessary
> > to implement initial early_printk
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >  xen/arch/riscv/Makefile          |  1 +
> >  xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
> >  xen/arch/riscv/sbi.c             | 44
> > ++++++++++++++++++++++++++++++++
> >  3 files changed, 79 insertions(+)
> >  create mode 100644 xen/arch/riscv/include/asm/sbi.h
> >  create mode 100644 xen/arch/riscv/sbi.c
> > 
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 5a67a3f493..60db415654 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,5 +1,6 @@
> >  obj-$(CONFIG_RISCV_64) += riscv64/
> >  obj-y += setup.o
> > +obj-y += sbi.o
> > 
> >  $(TARGET): $(TARGET)-syms
> >         $(OBJCOPY) -O binary -S $< $@
> > diff --git a/xen/arch/riscv/include/asm/sbi.h
> > b/xen/arch/riscv/include/asm/sbi.h
> > new file mode 100644
> > index 0000000000..34b53f8eaf
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/sbi.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-or-later) */
> > +/*
> > + * Copyright (c) 2021 Vates SAS.
> > + *
> > + * Taken from xvisor, modified by Bobby Eshleman
> > (bobby.eshleman@gmail.com).
> > + *
> > + * Taken/modified from Xvisor project with the following
> > copyright:
> > + *
> > + * Copyright (c) 2019 Western Digital Corporation or its
> > affiliates.
> > + */
> > +
> > +#ifndef __CPU_SBI_H__
> > +#define __CPU_SBI_H__
> I wonder where does CPU come from. Shouldn't this be called
> __ASM_RISCV_SBI_H__ ?
> 
I missed that when get this files from Bobby's patch series.
It makes sense to rename the define.
Thanks.
> > +
> > +#define SBI_EXT_0_1_CONSOLE_PUTCHAR            0x1
> > +
> > +struct sbiret {
> > +    long error;
> > +    long value;
> > +};
> > +
> > +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid,
> > unsigned long arg0,
> > +        unsigned long arg1, unsigned long arg2,
> > +        unsigned long arg3, unsigned long arg4,
> > +        unsigned long arg5);
> The arguments needs to be aligned.
> 
> > +
> > +/**
> > + * Writes given character to the console device.
> > + *
> > + * @param ch The data to be written to the console.
> > + */
> > +void sbi_console_putchar(int ch);
> > +
> > +#endif // __CPU_SBI_H__
> // should be replaced with /* */
> 
> > diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
> > new file mode 100644
> > index 0000000000..67cf5dd982
> > --- /dev/null
> > +++ b/xen/arch/riscv/sbi.c
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Taken and modified from the xvisor project with the copyright
> > Copyright (c)
> > + * 2019 Western Digital Corporation or its affiliates and author
> > Anup Patel
> > + * (anup.patel@wdc.com).
> > + *
> > + * Modified by Bobby Eshleman (bobby.eshleman@gmail.com).
> > + *
> > + * Copyright (c) 2019 Western Digital Corporation or its
> > affiliates.
> > + * Copyright (c) 2021 Vates SAS.
> > + */
> > +
> > +#include <xen/errno.h>
> > +#include <asm/sbi.h>
> > +
> > +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid,
> > unsigned long arg0,
> > +            unsigned long arg1, unsigned long arg2,
> > +            unsigned long arg3, unsigned long arg4,
> > +            unsigned long arg5)
> The arguments needs to be aligned.
> 
It looks that I mixed tabs with space or vice versa.
Will double check.
Thanks.
> > +{
> > +    struct sbiret ret;
> Could you please add an empty line here.
> 
> > +    register unsigned long a0 asm ("a0") = arg0;
> > +    register unsigned long a1 asm ("a1") = arg1;
> > +    register unsigned long a2 asm ("a2") = arg2;
> > +    register unsigned long a3 asm ("a3") = arg3;
> > +    register unsigned long a4 asm ("a4") = arg4;
> > +    register unsigned long a5 asm ("a5") = arg5;
> > +    register unsigned long a6 asm ("a6") = fid;
> > +    register unsigned long a7 asm ("a7") = ext;
> > +
> > +    asm volatile ("ecall"
> > +              : "+r" (a0), "+r" (a1)
> > +              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6),
> > "r" (a7)
> > +              : "memory");
> > +    ret.error = a0;
> > +    ret.value = a1;
> > +
> > +    return ret;
> > +}
> > +
> > +void sbi_console_putchar(int ch)
> > +{
> > +    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
> > +}
> > --
> > 2.38.1
> > 
> > 
> 
> ~Michal
~Oleksii

[PATCH v1 5/8] xen/include: include <asm/types.h> in <xen/early_printk.h>

[Oleksii Kurochko]

<asm/types.h> should be included because second argument of
early_puts has type 'size_t' which is defined in <asm/types.h>

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/include/xen/early_printk.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/xen/early_printk.h b/xen/include/xen/early_printk.h
index 0f76c3a74f..abb34687da 100644
--- a/xen/include/xen/early_printk.h
+++ b/xen/include/xen/early_printk.h
@@ -4,6 +4,8 @@
 #ifndef __XEN_EARLY_PRINTK_H__
 #define __XEN_EARLY_PRINTK_H__
 
+#include <asm/types.h>
+
 #ifdef CONFIG_EARLY_PRINTK
 void early_puts(const char *s, size_t nr);
 #else
-- 
2.38.1

Re: [PATCH v1 5/8] xen/include: include <asm/types.h> in <xen/early_printk.h>

[Julien Grall]

On 06/01/2023 13:14, Oleksii Kurochko wrote:
> <asm/types.h> should be included because second argument of
> early_puts has type 'size_t' which is defined in <asm/types.h>
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
>   xen/include/xen/early_printk.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/xen/include/xen/early_printk.h b/xen/include/xen/early_printk.h
> index 0f76c3a74f..abb34687da 100644
> --- a/xen/include/xen/early_printk.h
> +++ b/xen/include/xen/early_printk.h
> @@ -4,6 +4,8 @@
>   #ifndef __XEN_EARLY_PRINTK_H__
>   #define __XEN_EARLY_PRINTK_H__
>   
> +#include <asm/types.h>
> +
>   #ifdef CONFIG_EARLY_PRINTK
>   void early_puts(const char *s, size_t nr);
>   #else

-- 
Julien Grall

[PATCH v1 6/8] xen/riscv: introduce early_printk basic stuff

[Oleksii Kurochko]

The patch introduces a basic stuff of early_printk functionality
which will be enough to print 'hello from C environment"

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Kconfig.debug              |  7 ++++++
 xen/arch/riscv/Makefile                   |  1 +
 xen/arch/riscv/early_printk.c             | 27 +++++++++++++++++++++++
 xen/arch/riscv/include/asm/early_printk.h | 12 ++++++++++
 4 files changed, 47 insertions(+)
 create mode 100644 xen/arch/riscv/early_printk.c
 create mode 100644 xen/arch/riscv/include/asm/early_printk.h

diff --git a/xen/arch/riscv/Kconfig.debug b/xen/arch/riscv/Kconfig.debug
index e69de29bb2..940630fd62 100644
--- a/xen/arch/riscv/Kconfig.debug
+++ b/xen/arch/riscv/Kconfig.debug
@@ -0,0 +1,7 @@
+config EARLY_PRINTK
+    bool "Enable early printk config"
+    default DEBUG
+    depends on RISCV_64
+    help
+
+      Enables early printk debug messages
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 60db415654..e8630fe68d 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_RISCV_64) += riscv64/
 obj-y += setup.o
 obj-y += sbi.o
+obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 
 $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
diff --git a/xen/arch/riscv/early_printk.c b/xen/arch/riscv/early_printk.c
new file mode 100644
index 0000000000..f357f3220b
--- /dev/null
+++ b/xen/arch/riscv/early_printk.c
@@ -0,0 +1,27 @@
+/*
+ * RISC-V early printk using SBI
+ *
+ * Copyright (C) 2021 Bobby Eshleman <bobbyeshleman@gmail.com>
+ */
+#include <asm/sbi.h>
+#include <asm/early_printk.h>
+
+void early_puts(const char *s, size_t nr)
+{
+    while ( nr-- > 0 )
+    {
+        if (*s == '\n')
+            sbi_console_putchar('\r');
+        sbi_console_putchar(*s);
+        s++;
+    }
+}
+
+void early_printk(const char *str)
+{
+    while (*str)
+    {
+        early_puts(str, 1);
+        str++;
+    }
+}
diff --git a/xen/arch/riscv/include/asm/early_printk.h b/xen/arch/riscv/include/asm/early_printk.h
new file mode 100644
index 0000000000..05106e160d
--- /dev/null
+++ b/xen/arch/riscv/include/asm/early_printk.h
@@ -0,0 +1,12 @@
+#ifndef __EARLY_PRINTK_H__
+#define __EARLY_PRINTK_H__
+
+#include <xen/early_printk.h>
+
+#ifdef CONFIG_EARLY_PRINTK
+void early_printk(const char *str);
+#else
+static inline void early_printk(const char *s) {};
+#endif
+
+#endif /* __EARLY_PRINTK_H__ */
-- 
2.38.1

Re: [PATCH v1 6/8] xen/riscv: introduce early_printk basic stuff

[Julien Grall]

Hi,

On 06/01/2023 13:14, Oleksii Kurochko wrote:
> The patch introduces a basic stuff of early_printk functionality
> which will be enough to print 'hello from C environment"
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   xen/arch/riscv/Kconfig.debug              |  7 ++++++
>   xen/arch/riscv/Makefile                   |  1 +
>   xen/arch/riscv/early_printk.c             | 27 +++++++++++++++++++++++
>   xen/arch/riscv/include/asm/early_printk.h | 12 ++++++++++
>   4 files changed, 47 insertions(+)
>   create mode 100644 xen/arch/riscv/early_printk.c
>   create mode 100644 xen/arch/riscv/include/asm/early_printk.h
> 
> diff --git a/xen/arch/riscv/Kconfig.debug b/xen/arch/riscv/Kconfig.debug
> index e69de29bb2..940630fd62 100644
> --- a/xen/arch/riscv/Kconfig.debug
> +++ b/xen/arch/riscv/Kconfig.debug
> @@ -0,0 +1,7 @@
> +config EARLY_PRINTK
> +    bool "Enable early printk config"
> +    default DEBUG
> +    depends on RISCV_64

OOI, why can't this be used for RISCV_32?

> +    help
> +
> +      Enables early printk debug messages
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 60db415654..e8630fe68d 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,6 +1,7 @@
>   obj-$(CONFIG_RISCV_64) += riscv64/
>   obj-y += setup.o
>   obj-y += sbi.o
> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o

Please order the files alphabetically.

>   
>   $(TARGET): $(TARGET)-syms
>   	$(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/early_printk.c b/xen/arch/riscv/early_printk.c
> new file mode 100644
> index 0000000000..f357f3220b
> --- /dev/null
> +++ b/xen/arch/riscv/early_printk.c
> @@ -0,0 +1,27 @@

Please add an SPDX license (the default for Xen is GPLv2).

> +/*
> + * RISC-V early printk using SBI
> + *
> + * Copyright (C) 2021 Bobby Eshleman <bobbyeshleman@gmail.com>

So the copyright here is from Bobby. But I don't see any mention in the 
commit message. Where is this coming from?

> + */
> +#include <asm/sbi.h>
> +#include <asm/early_printk.h>

Please order the files alphabetically.

> +
> +void early_puts(const char *s, size_t nr)
> +{
> +    while ( nr-- > 0 )
> +    {
> +        if (*s == '\n')
> +            sbi_console_putchar('\r');
> +        sbi_console_putchar(*s);
> +        s++;
> +    }
> +}
> +
> +void early_printk(const char *str)
> +{
> +    while (*str)
> +    {
> +        early_puts(str, 1);
> +        str++;
> +    }
> +}
> diff --git a/xen/arch/riscv/include/asm/early_printk.h b/xen/arch/riscv/include/asm/early_printk.h
> new file mode 100644
> index 0000000000..05106e160d
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/early_printk.h
> @@ -0,0 +1,12 @@
> +#ifndef __EARLY_PRINTK_H__
> +#define __EARLY_PRINTK_H__
> +
> +#include <xen/early_printk.h>
> +
> +#ifdef CONFIG_EARLY_PRINTK
> +void early_printk(const char *str);
> +#else
> +static inline void early_printk(const char *s) {};
> +#endif
> +
> +#endif /* __EARLY_PRINTK_H__ */

-- 
Julien Grall

Re: [PATCH v1 6/8] xen/riscv: introduce early_printk basic stuff

[Oleksii]

Hi,

On Fri, 2023-01-06 at 13:51 +0000, Julien Grall wrote:
> Hi,
> 
> On 06/01/2023 13:14, Oleksii Kurochko wrote:
> > The patch introduces a basic stuff of early_printk functionality
> > which will be enough to print 'hello from C environment"
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   xen/arch/riscv/Kconfig.debug              |  7 ++++++
> >   xen/arch/riscv/Makefile                   |  1 +
> >   xen/arch/riscv/early_printk.c             | 27
> > +++++++++++++++++++++++
> >   xen/arch/riscv/include/asm/early_printk.h | 12 ++++++++++
> >   4 files changed, 47 insertions(+)
> >   create mode 100644 xen/arch/riscv/early_printk.c
> >   create mode 100644 xen/arch/riscv/include/asm/early_printk.h
> > 
> > diff --git a/xen/arch/riscv/Kconfig.debug
> > b/xen/arch/riscv/Kconfig.debug
> > index e69de29bb2..940630fd62 100644
> > --- a/xen/arch/riscv/Kconfig.debug
> > +++ b/xen/arch/riscv/Kconfig.debug
> > @@ -0,0 +1,7 @@
> > +config EARLY_PRINTK
> > +    bool "Enable early printk config"
> > +    default DEBUG
> > +    depends on RISCV_64
> 
> OOI, why can't this be used for RISCV_32?
> 
We can. It's my fault we wanted to start from RISCV_64 support so I
totally forgot about RISCV_32 =)
> > +    help
> > +
> > +      Enables early printk debug messages
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 60db415654..e8630fe68d 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,6 +1,7 @@
> >   obj-$(CONFIG_RISCV_64) += riscv64/
> >   obj-y += setup.o
> >   obj-y += sbi.o
> > +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> 
> Please order the files alphabetically.
> 
> >   
> >   $(TARGET): $(TARGET)-syms
> >         $(OBJCOPY) -O binary -S $< $@
> > diff --git a/xen/arch/riscv/early_printk.c
> > b/xen/arch/riscv/early_printk.c
> > new file mode 100644
> > index 0000000000..f357f3220b
> > --- /dev/null
> > +++ b/xen/arch/riscv/early_printk.c
> > @@ -0,0 +1,27 @@
> 
> Please add an SPDX license (the default for Xen is GPLv2).
> 
> > +/*
> > + * RISC-V early printk using SBI
> > + *
> > + * Copyright (C) 2021 Bobby Eshleman <bobbyeshleman@gmail.com>
> 
> So the copyright here is from Bobby. But I don't see any mention in
> the 
> commit message. Where is this coming from?
> 
Could you please share with me an example how it should be look like?
Signed-off: ... ? Or "this file was taken from patch series ..."?
> > + */
> > +#include <asm/sbi.h>
> > +#include <asm/early_printk.h>
> 
> Please order the files alphabetically.
> 
> > +
> > +void early_puts(const char *s, size_t nr)
> > +{
> > +    while ( nr-- > 0 )
> > +    {
> > +        if (*s == '\n')
> > +            sbi_console_putchar('\r');
> > +        sbi_console_putchar(*s);
> > +        s++;
> > +    }
> > +}
> > +
> > +void early_printk(const char *str)
> > +{
> > +    while (*str)
> > +    {
> > +        early_puts(str, 1);
> > +        str++;
> > +    }
> > +}
> > diff --git a/xen/arch/riscv/include/asm/early_printk.h
> > b/xen/arch/riscv/include/asm/early_printk.h
> > new file mode 100644
> > index 0000000000..05106e160d
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/early_printk.h
> > @@ -0,0 +1,12 @@
> > +#ifndef __EARLY_PRINTK_H__
> > +#define __EARLY_PRINTK_H__
> > +
> > +#include <xen/early_printk.h>
> > +
> > +#ifdef CONFIG_EARLY_PRINTK
> > +void early_printk(const char *str);
> > +#else
> > +static inline void early_printk(const char *s) {};
> > +#endif
> > +
> > +#endif /* __EARLY_PRINTK_H__ */
> 

Re: [PATCH v1 6/8] xen/riscv: introduce early_printk basic stuff

[Julien Grall]

Hi,

On 09/01/2023 09:10, Oleksii wrote:
> On Fri, 2023-01-06 at 13:51 +0000, Julien Grall wrote:
>> Hi,
>>
>> On 06/01/2023 13:14, Oleksii Kurochko wrote:
>>> The patch introduces a basic stuff of early_printk functionality
>>> which will be enough to print 'hello from C environment"
>>>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>>    xen/arch/riscv/Kconfig.debug              |  7 ++++++
>>>    xen/arch/riscv/Makefile                   |  1 +
>>>    xen/arch/riscv/early_printk.c             | 27
>>> +++++++++++++++++++++++
>>>    xen/arch/riscv/include/asm/early_printk.h | 12 ++++++++++
>>>    4 files changed, 47 insertions(+)
>>>    create mode 100644 xen/arch/riscv/early_printk.c
>>>    create mode 100644 xen/arch/riscv/include/asm/early_printk.h
>>>
>>> diff --git a/xen/arch/riscv/Kconfig.debug
>>> b/xen/arch/riscv/Kconfig.debug
>>> index e69de29bb2..940630fd62 100644
>>> --- a/xen/arch/riscv/Kconfig.debug
>>> +++ b/xen/arch/riscv/Kconfig.debug
>>> @@ -0,0 +1,7 @@
>>> +config EARLY_PRINTK
>>> +    bool "Enable early printk config"
>>> +    default DEBUG
>>> +    depends on RISCV_64
>>
>> OOI, why can't this be used for RISCV_32?
>>
> We can. It's my fault we wanted to start from RISCV_64 support so I
> totally forgot about RISCV_32 =)
>>> +    help
>>> +
>>> +      Enables early printk debug messages
>>> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
>>> index 60db415654..e8630fe68d 100644
>>> --- a/xen/arch/riscv/Makefile
>>> +++ b/xen/arch/riscv/Makefile
>>> @@ -1,6 +1,7 @@
>>>    obj-$(CONFIG_RISCV_64) += riscv64/
>>>    obj-y += setup.o
>>>    obj-y += sbi.o
>>> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>>
>> Please order the files alphabetically.
>>
>>>    
>>>    $(TARGET): $(TARGET)-syms
>>>          $(OBJCOPY) -O binary -S $< $@
>>> diff --git a/xen/arch/riscv/early_printk.c
>>> b/xen/arch/riscv/early_printk.c
>>> new file mode 100644
>>> index 0000000000..f357f3220b
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/early_printk.c
>>> @@ -0,0 +1,27 @@
>>
>> Please add an SPDX license (the default for Xen is GPLv2).
>>
>>> +/*
>>> + * RISC-V early printk using SBI
>>> + *
>>> + * Copyright (C) 2021 Bobby Eshleman <bobbyeshleman@gmail.com>
>>
>> So the copyright here is from Bobby. But I don't see any mention in
>> the
>> commit message. Where is this coming from?
>>
> Could you please share with me an example how it should be look like?
> Signed-off: ... ? Or "this file was taken from patch series ..."?

This depends on the context. Do you have a pointer to the original work?

If you are taking the patch mostly as-is, then the author should be 
Bobby. The first signed-off-by would be Bobby and you will be the second 
one.

Otherwise, you could credit him with "Based on the original work from 
...". A link could be added in the commit message or after ---.

Cheers,

-- 
Julien Grall

[PATCH v1 7/8] xen/riscv: print hello message from C env

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/riscv64/head.S |  4 +---
 xen/arch/riscv/setup.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index ddc7104701..4e399016e9 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -5,6 +5,4 @@ ENTRY(start)
         li      t0, PAGE_SIZE
         add     sp, sp, t0
 
-_start_hang:
-        wfi
-        j  _start_hang
+        tail    start_xen
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 2c7dca1daa..785566103b 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,6 +1,18 @@
 #include <xen/init.h>
 #include <xen/compile.h>
 
+#include <asm/early_printk.h>
+
 /* Xen stack for bringing up the first CPU. */
 unsigned char __initdata cpu0_boot_stack[PAGE_SIZE]
     __aligned(PAGE_SIZE);
+
+void __init noreturn start_xen(void)
+{
+    early_printk("Hello from C env\n");
+
+    for ( ;; )
+        asm volatile ("wfi");
+
+    unreachable();
+}
-- 
2.38.1

[PATCH v1 8/8] automation: add RISC-V smoke test

[Oleksii Kurochko]

Add check if there is a message 'Hello from C env' presents
in log file to be sure that stack is set and C part of early printk
is working.

Also qemu-system-riscv was added to riscv64.dockerfile as it is
required for RISC-V smoke test.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 automation/build/archlinux/riscv64.dockerfile |  3 ++-
 automation/scripts/qemu-smoke-riscv64.sh      | 20 +++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100755 automation/scripts/qemu-smoke-riscv64.sh

diff --git a/automation/build/archlinux/riscv64.dockerfile b/automation/build/archlinux/riscv64.dockerfile
index ff8b2b955d..375c78ecd5 100644
--- a/automation/build/archlinux/riscv64.dockerfile
+++ b/automation/build/archlinux/riscv64.dockerfile
@@ -9,7 +9,8 @@ RUN pacman --noconfirm --needed -Syu \
     inetutils \
     riscv64-linux-gnu-binutils \
     riscv64-linux-gnu-gcc \
-    riscv64-linux-gnu-glibc
+    riscv64-linux-gnu-glibc \
+    qemu-system-riscv
 
 # Add compiler path
 ENV CROSS_COMPILE=riscv64-linux-gnu-
diff --git a/automation/scripts/qemu-smoke-riscv64.sh b/automation/scripts/qemu-smoke-riscv64.sh
new file mode 100755
index 0000000000..e0f06360bc
--- /dev/null
+++ b/automation/scripts/qemu-smoke-riscv64.sh
@@ -0,0 +1,20 @@
+#!/bin/bash
+
+set -ex
+
+# Run the test
+rm -f smoke.serial
+set +e
+
+timeout -k 1 2 \
+qemu-system-riscv64 \
+    -M virt \
+    -smp 1 \
+    -nographic \
+    -m 2g \
+    -kernel binaries/xen \
+    |& tee smoke.serial
+
+set -e
+(grep -q "Hello from C env" smoke.serial) || exit 1
+exit 0
-- 
2.38.1

Re: [PATCH v1 8/8] automation: add RISC-V smoke test

[Andrew Cooper]

On 06/01/2023 1:14 pm, Oleksii Kurochko wrote:
> Add check if there is a message 'Hello from C env' presents
> in log file to be sure that stack is set and C part of early printk
> is working.
>
> Also qemu-system-riscv was added to riscv64.dockerfile as it is
> required for RISC-V smoke test.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  automation/build/archlinux/riscv64.dockerfile |  3 ++-
>  automation/scripts/qemu-smoke-riscv64.sh      | 20 +++++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>  create mode 100755 automation/scripts/qemu-smoke-riscv64.sh

Looking through the entire series, aren't we missing a hunk to test.yml
to wire up the smoke test?

It wants to live in this patch along with the introduction of
qemu-smoke-riscv64.sh

However, the modification to the dockerfile want breaking out and
submitted separately.  It will involve rebuilding and redeploying the
container, which is a) fine to do separately, and b) a necessary
prerequisite for anyone else to take this series and test it.

~Andrew

Re: [PATCH v1 8/8] automation: add RISC-V smoke test

[Oleksii]

On Fri, 2023-01-06 at 15:05 +0000, Andrew Cooper wrote:
> On 06/01/2023 1:14 pm, Oleksii Kurochko wrote:
> > Add check if there is a message 'Hello from C env' presents
> > in log file to be sure that stack is set and C part of early printk
> > is working.
> > 
> > Also qemu-system-riscv was added to riscv64.dockerfile as it is
> > required for RISC-V smoke test.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >  automation/build/archlinux/riscv64.dockerfile |  3 ++-
> >  automation/scripts/qemu-smoke-riscv64.sh      | 20
> > +++++++++++++++++++
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >  create mode 100755 automation/scripts/qemu-smoke-riscv64.sh
> 
> Looking through the entire series, aren't we missing a hunk to
> test.yml
> to wire up the smoke test?
> 
Missed that. Will update test.yml in the next patch series.
> It wants to live in this patch along with the introduction of
> qemu-smoke-riscv64.sh
> 
> However, the modification to the dockerfile want breaking out and
> submitted separately.  It will involve rebuilding and redeploying the
> container, which is a) fine to do separately, and b) a necessary
> prerequisite for anyone else to take this series and test it.
> 
I am going to send a patch with the dockerfle modification today.
Thanks.

> ~Andrew
~ Oleksii

Re: [PATCH v1 0/8] Basic early_printk and smoke test implementation

[Andrew Cooper]

On 06/01/2023 1:14 pm, Oleksii Kurochko wrote:
> The patch series introduces the following:
> - the minimal set of headers and changes inside them.
> - SBI (RISC-V Supervisor Binary Interface) things necessary for basic
>   early_printk implementation.
> - things needed to set up the stack.
> - early_printk() function to print only strings.
> - RISC-V smoke test which checks if  "Hello from C env" message is
>   present in serial.tmp
>
> Oleksii Kurochko (8):
>   xen/riscv: introduce dummy asm/init.h
>   xen/riscv: introduce asm/types.h header file
>   xen/riscv: introduce stack stuff
>   xen/riscv: introduce sbi call to putchar to console
>   xen/include: include <asm/types.h> in <xen/early_printk.h>
>   xen/riscv: introduce early_printk basic stuff
>   xen/riscv: print hello message from C env
>   automation: add RISC-V smoke test

Thanks.  This highlights several areas where I think we want some rework
to the current common/arch split.

First, it really shouldn't be necessary for architectures to create
emtpy stub files.  There are two options - first drop some empty files
in xen/include/arch-fallback/asm and put a suitable -I at the end of
CFLAGS.  The other option, which is nicer IMO, is to use __has_include()
although that would require us finally deciding to bump the minimum GCC
version to 5 for x86 (which we need to do for may other reasons too).

Second, the asm/types is absurd.  That should be one common header,
because there's nothing arch specific about making those types appear.

Third, the early printk infrastructure is partially broken (the common
header can't be cleanly included), and unsatisfactory with how it plumbs
into the default console steal function.  With a bit of cleanup, most of
it can be not duplicated per arch.

~Andrew