[PATCH v3 0/6] Basic early_printk and smoke test implementation

[PATCH v3 0/6] 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

---
Changes in V3:
    - Most of "[PATCH v2 7/8] xen/riscv: print hello message from C env"
      was merged with [PATCH v2 3/6] xen/riscv: introduce stack stuff.
    - "[PATCH v2 7/8] xen/riscv: print hello message from C env" was
      merged with "[PATCH v2 6/8] xen/riscv: introduce early_printk basic
      stuff".
    - "[PATCH v2 5/8] xen/include: include <asm/types.h> in
      <xen/early_printk.h>" was removed as it has been already merged to
      mainline staging.
    - code style fixes.
---
Changes in V2:
    - update commit patches commit messages according to the mailing
      list comments
    - Remove unneeded types in <asm/types.h>
    - Introduce definition of STACK_SIZE
    - order the files alphabetically in Makefile
    - Add license to early_printk.c
    - Add RISCV_32 dependecy to config EARLY_PRINTK in Kconfig.debug
    - Move dockerfile changes to separate config and sent them as
      separate patch to mailing list.
    - Update test.yaml to wire up smoke test
---

Bobby Eshleman (2):
  xen/riscv: introduce sbi call to putchar to console
  xen/riscv: introduce early_printk basic stuff

Oleksii Kurochko (4):
  xen/riscv: introduce dummy asm/init.h
  xen/riscv: introduce asm/types.h header file
  xen/riscv: introduce stack stuff
  automation: add RISC-V smoke test

 automation/gitlab-ci/test.yaml            | 20 ++++++++++
 automation/scripts/qemu-smoke-riscv64.sh  | 20 ++++++++++
 xen/arch/riscv/Kconfig.debug              |  7 ++++
 xen/arch/riscv/Makefile                   |  3 ++
 xen/arch/riscv/early_printk.c             | 33 +++++++++++++++++
 xen/arch/riscv/include/asm/config.h       |  2 +
 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        | 22 +++++++++++
 xen/arch/riscv/riscv64/head.S             |  6 ++-
 xen/arch/riscv/sbi.c                      | 45 +++++++++++++++++++++++
 xen/arch/riscv/setup.c                    | 18 +++++++++
 13 files changed, 233 insertions(+), 1 deletion(-)
 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 v3 1/6] xen/riscv: introduce dummy asm/init.h

[Oleksii Kurochko]

The following patches require <xen/init.h> which includes
<asm/init.h>

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
    - Nothing changed
---
Changes in V2:
    - Add explanation comment to the comment message
---
 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

[PATCH v3 2/6] xen/riscv: introduce asm/types.h header file

[Oleksii Kurochko]

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
    - Nothing changed
---
Changes in V2:
    - Remove unneeded now types from <asm/types.h>
---
 xen/arch/riscv/include/asm/types.h | 22 ++++++++++++++++++++++
 1 file changed, 22 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..fbe352ef20
--- /dev/null
+++ b/xen/arch/riscv/include/asm/types.h
@@ -0,0 +1,22 @@
+#ifndef __RISCV_TYPES_H__
+#define __RISCV_TYPES_H__
+
+#ifndef __ASSEMBLY__
+
+#if defined(__SIZE_TYPE__)
+typedef __SIZE_TYPE__ size_t;
+#else
+typedef unsigned long size_t;
+#endif
+
+#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

[PATCH v3 3/6] 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>
---
Changes in V3:
    - reorder headers in alphabetical order
    - merge changes related to start_xen() function from "[PATCH v2 7/8]
      xen/riscv: print hello message from C env" to this patch
    - remove unneeded parentheses in definition of STACK_SIZE
---
Changes in V2:
    - introduce STACK_SIZE define.
    - use consistent padding between instruction mnemonic and operand(s)
---
 xen/arch/riscv/Makefile             |  1 +
 xen/arch/riscv/include/asm/config.h |  2 ++
 xen/arch/riscv/riscv64/head.S       |  6 +++++-
 xen/arch/riscv/setup.c              | 14 ++++++++++++++
 4 files changed, 22 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/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index 0370f865f3..763a922a04 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -43,6 +43,8 @@
 
 #define SMP_CACHE_BYTES (1 << 6)
 
+#define STACK_SIZE PAGE_SIZE
+
 #endif /* __RISCV_CONFIG_H__ */
 /*
  * Local variables:
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 990edb70a0..d444dd8aad 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -1,4 +1,8 @@
         .section .text.header, "ax", %progbits
 
 ENTRY(start)
-        j  start
+        la      sp, cpu0_boot_stack
+        li      t0, STACK_SIZE
+        add     sp, sp, t0
+
+        tail    start_xen
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
new file mode 100644
index 0000000000..13e24e2fe1
--- /dev/null
+++ b/xen/arch/riscv/setup.c
@@ -0,0 +1,14 @@
+#include <xen/compile.h>
+#include <xen/init.h>
+
+/* Xen stack for bringing up the first CPU. */
+unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
+    __aligned(STACK_SIZE);
+
+void __init noreturn start_xen(void)
+{
+    for ( ;; )
+        asm volatile ("wfi");
+
+    unreachable();
+}
-- 
2.38.1

[PATCH v3 4/6] xen/riscv: introduce sbi call to putchar to console

[Oleksii Kurochko]

From: Bobby Eshleman <bobby.eshleman@gmail.com>

Originally SBI implementation for Xen was introduced by
Bobby Eshleman <bobby.eshleman@gmail.com> but it was removed
all the stuff for simplicity  except SBI call for putting
character to console.

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

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
    - update copyright's year
    - rename define __CPU_SBI_H__ to __ASM_RISCV_SBI_H__
    - fix identations
    - change an author of the commit
---
Changes in V2:
    - add an explanatory comment about sbi_console_putchar() function.
    - order the files alphabetically in Makefile
---
 xen/arch/riscv/Makefile          |  1 +
 xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
 xen/arch/riscv/sbi.c             | 45 ++++++++++++++++++++++++++++++++
 3 files changed, 80 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..fd916e1004 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_RISCV_64) += riscv64/
+obj-y += sbi.o
 obj-y += setup.o
 
 $(TARGET): $(TARGET)-syms
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
new file mode 100644
index 0000000000..0e6820a4ed
--- /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-2023 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 __ASM_RISCV_SBI_H__
+#define __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);
+
+/**
+ * Writes given character to the console device.
+ *
+ * @param ch The data to be written to the console.
+ */
+void sbi_console_putchar(int ch);
+
+#endif /* __ASM_RISCV_SBI_H__ */
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
new file mode 100644
index 0000000000..dc0eb44bc6
--- /dev/null
+++ b/xen/arch/riscv/sbi.c
@@ -0,0 +1,45 @@
+/* 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-2023 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

[PATCH v3 5/6] xen/riscv: introduce early_printk basic stuff

[Oleksii Kurochko]

From: Bobby Eshleman <bobby.eshleman@gmail.com>

The patch introduces a basic stuff of early_printk functionality
which will be enough to print 'hello from C environment".
early_printk() function was changed in comparison with original as
common isn't being built now so there is no vscnprintf.

Because printk() relies on a serial driver (like the ns16550 driver)
and drivers require working virtual memory (ioremap()) there is not
print functionality early in Xen boot.

This commit adds early printk implementation built on the putc SBI call.

As sbi_console_putchar() is being already planned for deprecation
it is used temporary now and will be removed or reworked after
real uart will be ready.

Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
    - reorder "include <header>" in erly_printk.c
    - change an author of the commit
---
Changes in V2:
    - add license to early_printk.c
    - add signed-off-by Bobby
    - add RISCV_32 to Kconfig.debug to EARLY_PRINTK config
    - update commit message
    - order the files alphabetically in Makefile
---
 xen/arch/riscv/Kconfig.debug              |  7 +++++
 xen/arch/riscv/Makefile                   |  1 +
 xen/arch/riscv/early_printk.c             | 33 +++++++++++++++++++++++
 xen/arch/riscv/include/asm/early_printk.h | 12 +++++++++
 xen/arch/riscv/setup.c                    |  4 +++
 5 files changed, 57 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..e21649781d 100644
--- a/xen/arch/riscv/Kconfig.debug
+++ b/xen/arch/riscv/Kconfig.debug
@@ -0,0 +1,7 @@
+config EARLY_PRINTK
+    bool "Enable early printk"
+    default DEBUG
+    depends on RISCV_64 || RISCV_32
+    help
+
+      Enables early printk debug messages
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index fd916e1004..1a4f1a6015 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-$(CONFIG_RISCV_64) += riscv64/
 obj-y += sbi.o
 obj-y += setup.o
diff --git a/xen/arch/riscv/early_printk.c b/xen/arch/riscv/early_printk.c
new file mode 100644
index 0000000000..348c21bdaa
--- /dev/null
+++ b/xen/arch/riscv/early_printk.c
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * RISC-V early printk using SBI
+ *
+ * Copyright (C) 2021 Bobby Eshleman <bobbyeshleman@gmail.com>
+ */
+#include <asm/early_printk.h>
+#include <asm/sbi.h>
+
+/*
+ * TODO:
+ *   sbi_console_putchar is already planned for deprecation
+ *   so it should be reworked to use UART directly.
+*/
+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__ */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 13e24e2fe1..d09ffe1454 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,12 +1,16 @@
 #include <xen/compile.h>
 #include <xen/init.h>
 
+#include <asm/early_printk.h>
+
 /* Xen stack for bringing up the first CPU. */
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
 void __init noreturn start_xen(void)
 {
+    early_printk("Hello from C env\n");
+
     for ( ;; )
         asm volatile ("wfi");
 
-- 
2.38.1

[PATCH v3 6/6] 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.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in V3:
    - Nothing changed
    - All mentioned comments by Stefano in Xen mailing list will be
      fixed as a separate patch out of this patch series.
---
Changes in V2:
    - Move changes in the dockerfile to separate patch and  send it to
      mailing list separately:
        [PATCH] automation: add qemu-system-riscv to riscv64.dockerfile
    - Update test.yaml to wire up smoke test
---
 automation/gitlab-ci/test.yaml           | 20 ++++++++++++++++++++
 automation/scripts/qemu-smoke-riscv64.sh | 20 ++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100755 automation/scripts/qemu-smoke-riscv64.sh

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index afd80adfe1..64f47a0ab9 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -54,6 +54,19 @@
   tags:
     - x86_64
 
+.qemu-riscv64:
+  extends: .test-jobs-common
+  variables:
+    CONTAINER: archlinux:riscv64
+    LOGFILE: qemu-smoke-riscv64.log
+  artifacts:
+    paths:
+      - smoke.serial
+      - '*.log'
+    when: always
+  tags:
+    - x86_64
+
 .yocto-test:
   extends: .test-jobs-common
   script:
@@ -234,6 +247,13 @@ qemu-smoke-x86-64-clang-pvh:
   needs:
     - debian-unstable-clang-debug
 
+qemu-smoke-riscv64-gcc:
+  extends: .qemu-riscv64
+  script:
+    - ./automation/scripts/qemu-smoke-riscv64.sh 2>&1 | tee ${LOGFILE}
+  needs:
+    - riscv64-cross-gcc
+
 # Yocto test jobs
 yocto-qemuarm64:
   extends: .yocto-test-arm64
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 v3 5/6] xen/riscv: introduce early_printk basic stuff

[Jan Beulich]

On 10.01.2023 16:17, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/Kconfig.debug
> +++ b/xen/arch/riscv/Kconfig.debug
> @@ -0,0 +1,7 @@
> +config EARLY_PRINTK
> +    bool "Enable early printk"
> +    default DEBUG
> +    depends on RISCV_64 || RISCV_32

You're in a RISC-V-specific Kconfig - do you really need this line?

> --- /dev/null
> +++ b/xen/arch/riscv/early_printk.c
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * RISC-V early printk using SBI
> + *
> + * Copyright (C) 2021 Bobby Eshleman <bobbyeshleman@gmail.com>
> + */
> +#include <asm/early_printk.h>
> +#include <asm/sbi.h>
> +
> +/*
> + * TODO:
> + *   sbi_console_putchar is already planned for deprecation
> + *   so it should be reworked to use UART directly.
> +*/
> +void early_puts(const char *s, size_t nr)
> +{
> +    while ( nr-- > 0 )
> +    {
> +        if (*s == '\n')

Nit (style): Missing blanks.

> +            sbi_console_putchar('\r');
> +        sbi_console_putchar(*s);
> +        s++;
> +    }
> +}
> +
> +void early_printk(const char *str)
> +{
> +    while (*str)

Again.

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,12 +1,16 @@
>  #include <xen/compile.h>
>  #include <xen/init.h>
>  
> +#include <asm/early_printk.h>
> +
>  /* Xen stack for bringing up the first CPU. */
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
>  void __init noreturn start_xen(void)
>  {
> +    early_printk("Hello from C env\n");
> +
>      for ( ;; )
>          asm volatile ("wfi");

While this is only context here, it affects an earlier patch in the
series; this wants to be

    for ( ; ; )
        asm volatile ( "wfi" );

Jan

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

[Jan Beulich]

On 10.01.2023 16:17, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V3:
>     - Nothing changed
> ---
> Changes in V2:
>     - Remove unneeded now types from <asm/types.h>

I guess you went a little too far: Types used in common code, even if you
do not build that yet, will want declaring right away imo. Of course I
should finally try and get rid of at least some of the being-phased-out
ones (s8 and s16 look to be relatively low hanging fruit, for example,
and of these only s16 looks to be used in common code) ...

Jan

Re: [PATCH v3 1/6] xen/riscv: introduce dummy asm/init.h

[Jan Beulich]

Arm maintainers,

On 10.01.2023 16:17, Oleksii Kurochko wrote:
> --- /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 */

instead of having RISC-V introduce an empty stub matching what x86 has,
what would it take to empty Arm's as well, allowing both present ones to
go away and no new one to be introduced? The only thing you have in there
is struct init_info, which doesn't feel like it's properly placed in this
header (x86 has such stuff in setup.h) ...

Jan

Re: [PATCH v3 1/6] xen/riscv: introduce dummy asm/init.h

[Oleksii]

Hello Jan,

Sorry for breaking into the conversation.

On Tue, 2023-01-10 at 18:02 +0100, Jan Beulich wrote:
> Arm maintainers,
> 
> On 10.01.2023 16:17, Oleksii Kurochko wrote:
> > --- /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 */
> 
> instead of having RISC-V introduce an empty stub matching what x86
> has,
Have you had a chance to look at the answer (Re: [PATCH v1 0/8] Basic
early_printk and smoke test implementation) of Andrew:
https://lore.kernel.org/xen-devel/299d913c-8095-ad90-ea3b-d46ef74d4fdc@citrix.com/#t

I agree with his point regarding the usage of __has_include() to not
produce empty headers stubs for RISCV and for future architectures too.

> If maintainers are OK I can start to use __has_include() in the
> correspondent <xen/*.h> files.what would it take to empty Arm's as
> well, allowing both present ones to
> go away and no new one to be introduced? The only thing you have in
> there
> is struct init_info, which doesn't feel like it's properly placed in
> this
> header (x86 has such stuff in setup.h) ...
> 
> Jan
~Oleksii

Re: [PATCH v3 3/6] xen/riscv: introduce stack stuff

[Alistair Francis]

On Wed, Jan 11, 2023 at 1:18 AM Oleksii Kurochko
<oleksii.kurochko@gmail.com> 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Changes in V3:
>     - reorder headers in alphabetical order
>     - merge changes related to start_xen() function from "[PATCH v2 7/8]
>       xen/riscv: print hello message from C env" to this patch
>     - remove unneeded parentheses in definition of STACK_SIZE
> ---
> Changes in V2:
>     - introduce STACK_SIZE define.
>     - use consistent padding between instruction mnemonic and operand(s)
> ---
>  xen/arch/riscv/Makefile             |  1 +
>  xen/arch/riscv/include/asm/config.h |  2 ++
>  xen/arch/riscv/riscv64/head.S       |  6 +++++-
>  xen/arch/riscv/setup.c              | 14 ++++++++++++++
>  4 files changed, 22 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/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
> index 0370f865f3..763a922a04 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -43,6 +43,8 @@
>
>  #define SMP_CACHE_BYTES (1 << 6)
>
> +#define STACK_SIZE PAGE_SIZE
> +
>  #endif /* __RISCV_CONFIG_H__ */
>  /*
>   * Local variables:
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 990edb70a0..d444dd8aad 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -1,4 +1,8 @@
>          .section .text.header, "ax", %progbits
>
>  ENTRY(start)
> -        j  start
> +        la      sp, cpu0_boot_stack
> +        li      t0, STACK_SIZE
> +        add     sp, sp, t0
> +
> +        tail    start_xen
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> new file mode 100644
> index 0000000000..13e24e2fe1
> --- /dev/null
> +++ b/xen/arch/riscv/setup.c
> @@ -0,0 +1,14 @@
> +#include <xen/compile.h>
> +#include <xen/init.h>
> +
> +/* Xen stack for bringing up the first CPU. */
> +unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
> +    __aligned(STACK_SIZE);
> +
> +void __init noreturn start_xen(void)
> +{
> +    for ( ;; )
> +        asm volatile ("wfi");
> +
> +    unreachable();
> +}
> --
> 2.38.1
>
>

Re: [PATCH v3 1/6] xen/riscv: introduce dummy asm/init.h

[Julien Grall]

On 10/01/2023 17:02, Jan Beulich wrote:
> Arm maintainers,

Hi Jan,

> On 10.01.2023 16:17, Oleksii Kurochko wrote:
>> --- /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 */
> 
> instead of having RISC-V introduce an empty stub matching what x86 has,
> what would it take to empty Arm's as well, allowing both present ones to
> go away and no new one to be introduced? The only thing you have in there
> is struct init_info, which doesn't feel like it's properly placed in this
> header (x86 has such stuff in setup.h) ...

Yes. setup.h should be a good place even though the header is getting 
quite crowded.

I am happy to send a patch for it.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/6] xen/riscv: introduce dummy asm/init.h

[Jan Beulich]

On 10.01.2023 20:16, Oleksii wrote:
> Sorry for breaking into the conversation.

That's perfectly fine; no need to be sorry.

> On Tue, 2023-01-10 at 18:02 +0100, Jan Beulich wrote:
>> Arm maintainers,
>>
>> On 10.01.2023 16:17, Oleksii Kurochko wrote:
>>> --- /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 */
>>
>> instead of having RISC-V introduce an empty stub matching what x86
>> has,
> Have you had a chance to look at the answer (Re: [PATCH v1 0/8] Basic
> early_printk and smoke test implementation) of Andrew:
> https://lore.kernel.org/xen-devel/299d913c-8095-ad90-ea3b-d46ef74d4fdc@citrix.com/#t
> 
> I agree with his point regarding the usage of __has_include() to not
> produce empty headers stubs for RISCV and for future architectures too.

Sure, but as he said, that requires settling on a new toolchain baseline,
which is something that we've failed to come to any results for, for a
considerable number of years. Plus if we could get rid of this (then
optional) arch header altogether, it would imo be even better.

Jan

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

[Oleksii]

On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote:
> On 10.01.2023 16:17, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V3:
> >     - Nothing changed
> > ---
> > Changes in V2:
> >     - Remove unneeded now types from <asm/types.h>
> 
> I guess you went a little too far: Types used in common code, even if
> you
It looks then I didn't understand which one of types are needed.

In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all
types were introduced as most of them are used in <xen/types.h>:
__{u|s}{8|16|32|64}. Thereby it looks like the following types in
<asm/types.h> should be present from the start:
  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

 Then it turns out that there is no any sense in:
  typedef signed char s8;
  typedef unsigned char u8;

  typedef signed short s16;
  typedef unsigned short u16;

  typedef signed int s32;
  typedef unsigned int u32;

  typedef signed long long s64;
  typedef unsigned long long u64;
    OR
  typedef signed long s64;
  typedef unsigned long u64;
As I understand instead of them should be used: {u|s}int<N>_t.

All other types such as {v,p}addr_t, register_t and definitions
PRIvaddr, INVALID_PADDR, PRIpaadr, PRIregister should be present in
<asm/types.h> from the start.

Am I right?
> do not build that yet, will want declaring right away imo. Of course
> I
> should finally try and get rid of at least some of the being-phased-
> out
> ones (s8 and s16 look to be relatively low hanging fruit, for
> example,
> and of these only s16 looks to be used in common code) ...
> 
> Jan
~Oleksii

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

[Jan Beulich]

On 11.01.2023 09:47, Oleksii wrote:
> On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote:
>> On 10.01.2023 16:17, Oleksii Kurochko wrote:
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> ---
>>> Changes in V3:
>>>     - Nothing changed
>>> ---
>>> Changes in V2:
>>>     - Remove unneeded now types from <asm/types.h>
>>
>> I guess you went a little too far: Types used in common code, even if
>> you
> It looks then I didn't understand which one of types are needed.
> 
> In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all
> types were introduced as most of them are used in <xen/types.h>:
> __{u|s}{8|16|32|64}. Thereby it looks like the following types in
> <asm/types.h> should be present from the start:
>   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
> 
>  Then it turns out that there is no any sense in:
>   typedef signed char s8;
>   typedef unsigned char u8;
> 
>   typedef signed short s16;
>   typedef unsigned short u16;
> 
>   typedef signed int s32;
>   typedef unsigned int u32;
> 
>   typedef signed long long s64;
>   typedef unsigned long long u64;
>     OR
>   typedef signed long s64;
>   typedef unsigned long u64;
> As I understand instead of them should be used: {u|s}int<N>_t.

Hmm, the situation is worse than I thought (recalled) it was: You're
right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis-
guiding you; we'll need to do more cleanup first for asm/types.h to
become smaller.

Jan

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

[Xenia Ragiadakou]

On 1/11/23 11:08, Jan Beulich wrote:
> On 11.01.2023 09:47, Oleksii wrote:
>> On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote:
>>> On 10.01.2023 16:17, Oleksii Kurochko wrote:
>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>> ---
>>>> Changes in V3:
>>>>      - Nothing changed
>>>> ---
>>>> Changes in V2:
>>>>      - Remove unneeded now types from <asm/types.h>
>>>
>>> I guess you went a little too far: Types used in common code, even if
>>> you
>> It looks then I didn't understand which one of types are needed.
>>
>> In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all
>> types were introduced as most of them are used in <xen/types.h>:
>> __{u|s}{8|16|32|64}. Thereby it looks like the following types in
>> <asm/types.h> should be present from the start:
>>    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
>>
>>   Then it turns out that there is no any sense in:
>>    typedef signed char s8;
>>    typedef unsigned char u8;
>>
>>    typedef signed short s16;
>>    typedef unsigned short u16;
>>
>>    typedef signed int s32;
>>    typedef unsigned int u32;
>>
>>    typedef signed long long s64;
>>    typedef unsigned long long u64;
>>      OR
>>    typedef signed long s64;
>>    typedef unsigned long u64;
>> As I understand instead of them should be used: {u|s}int<N>_t.
> 
> Hmm, the situation is worse than I thought (recalled) it was: You're
> right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis-
> guiding you; we'll need to do more cleanup first for asm/types.h to
> become smaller.

What is the reason for not declaring __{u,s}<N> directly in xen/types.h 
as type alias to {u,s}<N>?

IIUC __{u,s}<N> and {u,s}<N> are needed by code ported from Linux while 
Xen code should use {u|s}int<N>_t instead, right?

-- 
Xenia

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

[Jan Beulich]

On 11.01.2023 11:22, Xenia Ragiadakou wrote:
> 
> On 1/11/23 11:08, Jan Beulich wrote:
>> On 11.01.2023 09:47, Oleksii wrote:
>>> On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote:
>>>> On 10.01.2023 16:17, Oleksii Kurochko wrote:
>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>> ---
>>>>> Changes in V3:
>>>>>      - Nothing changed
>>>>> ---
>>>>> Changes in V2:
>>>>>      - Remove unneeded now types from <asm/types.h>
>>>>
>>>> I guess you went a little too far: Types used in common code, even if
>>>> you
>>> It looks then I didn't understand which one of types are needed.
>>>
>>> In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all
>>> types were introduced as most of them are used in <xen/types.h>:
>>> __{u|s}{8|16|32|64}. Thereby it looks like the following types in
>>> <asm/types.h> should be present from the start:
>>>    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
>>>
>>>   Then it turns out that there is no any sense in:
>>>    typedef signed char s8;
>>>    typedef unsigned char u8;
>>>
>>>    typedef signed short s16;
>>>    typedef unsigned short u16;
>>>
>>>    typedef signed int s32;
>>>    typedef unsigned int u32;
>>>
>>>    typedef signed long long s64;
>>>    typedef unsigned long long u64;
>>>      OR
>>>    typedef signed long s64;
>>>    typedef unsigned long u64;
>>> As I understand instead of them should be used: {u|s}int<N>_t.
>>
>> Hmm, the situation is worse than I thought (recalled) it was: You're
>> right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis-
>> guiding you; we'll need to do more cleanup first for asm/types.h to
>> become smaller.
> 
> What is the reason for not declaring __{u,s}<N> directly in xen/types.h 
> as type alias to {u,s}<N>?
> 
> IIUC __{u,s}<N> and {u,s}<N> are needed by code ported from Linux while 
> Xen code should use {u|s}int<N>_t instead, right?

Yes. Hence in the long run only Linux files should get to see __{u,s}<N>
and {u,s}<N>, perhaps from a new linux-types.h.

Jan

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

[Xenia Ragiadakou]

On 1/11/23 13:38, Jan Beulich wrote:
> On 11.01.2023 11:22, Xenia Ragiadakou wrote:
>>
>> On 1/11/23 11:08, Jan Beulich wrote:
>>> On 11.01.2023 09:47, Oleksii wrote:
>>>> On Tue, 2023-01-10 at 17:58 +0100, Jan Beulich wrote:
>>>>> On 10.01.2023 16:17, Oleksii Kurochko wrote:
>>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>>> ---
>>>>>> Changes in V3:
>>>>>>       - Nothing changed
>>>>>> ---
>>>>>> Changes in V2:
>>>>>>       - Remove unneeded now types from <asm/types.h>
>>>>>
>>>>> I guess you went a little too far: Types used in common code, even if
>>>>> you
>>>> It looks then I didn't understand which one of types are needed.
>>>>
>>>> In "[PATCH v1 2/8] xen/riscv: introduce asm/types.h header file" all
>>>> types were introduced as most of them are used in <xen/types.h>:
>>>> __{u|s}{8|16|32|64}. Thereby it looks like the following types in
>>>> <asm/types.h> should be present from the start:
>>>>     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
>>>>
>>>>    Then it turns out that there is no any sense in:
>>>>     typedef signed char s8;
>>>>     typedef unsigned char u8;
>>>>
>>>>     typedef signed short s16;
>>>>     typedef unsigned short u16;
>>>>
>>>>     typedef signed int s32;
>>>>     typedef unsigned int u32;
>>>>
>>>>     typedef signed long long s64;
>>>>     typedef unsigned long long u64;
>>>>       OR
>>>>     typedef signed long s64;
>>>>     typedef unsigned long u64;
>>>> As I understand instead of them should be used: {u|s}int<N>_t.
>>>
>>> Hmm, the situation is worse than I thought (recalled) it was: You're
>>> right, xen/types.h actually uses __{u,s}<N>. So I'm sorry for mis-
>>> guiding you; we'll need to do more cleanup first for asm/types.h to
>>> become smaller.
>>
>> What is the reason for not declaring __{u,s}<N> directly in xen/types.h
>> as type alias to {u,s}<N>?
>>
>> IIUC __{u,s}<N> and {u,s}<N> are needed by code ported from Linux while
>> Xen code should use {u|s}int<N>_t instead, right?
> 
> Yes. Hence in the long run only Linux files should get to see __{u,s}<N>
> and {u,s}<N>, perhaps from a new linux-types.h.

Thanks for the clarification. Could you please help me understand also 
why __signed__ keyword is required when declaring __{u,s}<N>?
I mean why __{u,s}<N> cannot be declared using the signed type 
specifier, just like {u,s}<N>?

-- 
Xenia

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

[Jan Beulich]

On 11.01.2023 13:30, Xenia Ragiadakou wrote:
> Could you please help me understand also 
> why __signed__ keyword is required when declaring __{u,s}<N>?
> I mean why __{u,s}<N> cannot be declared using the signed type 
> specifier, just like {u,s}<N>?

I'm afraid I can't, as this looks to have been (blindly?) imported
from Linux very, very long ago. Having put time in going through
our own history, I'm afraid I don't want to go further and see
whether I could spot the reason for you by going through Linux'es.

Jan

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

[Xenia Ragiadakou]

On 1/11/23 15:17, Jan Beulich wrote:
> On 11.01.2023 13:30, Xenia Ragiadakou wrote:
>> Could you please help me understand also
>> why __signed__ keyword is required when declaring __{u,s}<N>?
>> I mean why __{u,s}<N> cannot be declared using the signed type
>> specifier, just like {u,s}<N>?
> 
> I'm afraid I can't, as this looks to have been (blindly?) imported
> from Linux very, very long ago. Having put time in going through
> our own history, I'm afraid I don't want to go further and see
> whether I could spot the reason for you by going through Linux'es.

Sorry, I was not aiming to drag you (or anyone) into spotting why Linux 
uses __signed__ when declaring __s<N>. AfAIU these types are exported 
and used in userspace and maybe the reason is to support building with 
pre-standard C compilers.
I am just wondering why Xen, that is compiled with std=c99, uses 
__signed__. If there is no reason, I think this difference between the 
declarations of __{u,s}<N> and {u,s}<N> is misleading and confusing (to 
me at least).

-- 
Xenia