Introduce semihosting facilities and semihosting console driver #6700

gagachang · 2024-02-21T09:09:18Z

Hello,

This series implement semihosting facilities.
Please review it if this is feasible in OP-TEE system, thanks!

The new config option CFG_SEMIHOSTING is used to compile in-kernel semihosting functions.
Architectures who support semihosting should provide their implementation of __do_semihosting(), which consists of special instructions to trigger the semihosting.
In this series, we've implemented RISC-V version.

Currently we only implement semihosting open/read/write operations.
Based on this in-kernel semihosting, semihosting console driver is also introduced.
There are two paths to output the log:

host side console
host side file

The path is decided by file_path parameter provided by the caller of semihosting_console_init().
The new config option CFG_SEMIHOSTING_CONSOLE is used to compile semihosting console driver.

Example code to use semihosting console driver:

#include <drivers/semihosting_console.h>

static struct semihosting_console_data console_data __nex_bss;

void console_init(void)
{
          /* User must only choose one of the following two ways */
          /* 1. Output log to a file on semihosting host side system */
	  semihosting_console_init(&console_data, "semihosting.txt");
          /* 2. Output log to semihosting host side console */
	  semihosting_console_init(&console_data, NULL);

	  register_serial_console(&console_data.chip);
}

jforissier

For "libutils: Move _ansi.h to include folder":
Reviewed-by: Jerome Forissier <[email protected]>
For " libutils: Import part of sys/fcntl.h":
Please mention you are importing this from newlib in the commit description. One more comment below.
For "core: riscv: Add semihosting.S for semihosting instructions":
Acked-by: Jerome Forissier <[email protected]>
For "core: kernel: Add semihosting functions": see comments below
For "drivers: Implement semihosting based console driver for log":
Acked-by: Jerome Forissier <[email protected]>

It would be nice to be able to test this. Since the whole RISC-V QEMU environment is not ready yet, we should consider implementing __do_semihosting() for Arm. I may give it a shot ;)

lib/libutils/isoc/include/sys/_default_fcntl.h

core/kernel/semihosting.c

mk/config.mk

gagachang · 2024-02-21T16:58:20Z

It would be nice to be able to test this.

Sure, do you have any suggestion to test this feature ?

Since the whole RISC-V QEMU environment is not ready yet, we should consider implementing __do_semihosting() for Arm. I may give it a shot ;)

Yes please! You may want to try this PR on ARM QEMUv8.
I would like to add your implementation for ARM into this series.

core/arch/riscv/kernel/sub.mk

lib/libutils/isoc/include/sys/_default_fcntl.h

core/kernel/semihosting.c

core/drivers/semihosting_console.c

core/include/drivers/semihosting_console.h

core/include/kernel/semihosting.h

core/kernel/semihosting.c

lib/libutils/isoc/include/sys/_default_fcntl.h

gagachang · 2024-02-23T09:34:12Z

Hi @jforissier @etienne-lms

I have one question:
The semihosting console driver is compiled when CFG_SEMIHOSTING_CONSOLE is y.
Should I also put CFG_SEMIHOSTING_CONSOLE into mk/config.mk ?
Besides, it has dependency on CFG_SEMIHOSTING

etienne-lms · 2024-02-23T10:18:12Z

Should I also put CFG_SEMIHOSTING_CONSOLE into mk/config.mk ?
Besides, it has dependency on CFG_SEMIHOSTING

Yes, this is a good idea. This is a generic driver so the relate config switch deserve to be defined and documented in mk/config.mk.

gagachang · 2024-02-23T12:53:44Z

This is a generic driver so the relate config switch deserve to be defined and documented in mk/config.mk.

Thanks! I will add it.

gagachang · 2024-02-23T13:18:45Z

Add dedicated 4b71811 for new configurations: CFG_SEMIHOSTING and CFG_SEMIHOSTING_CONSOLE.

jforissier · 2024-02-23T13:33:18Z

Should I also put CFG_SEMIHOSTING_CONSOLE into mk/config.mk ?
Besides, it has dependency on CFG_SEMIHOSTING

Yes, this is a good idea. This is a generic driver so the relate config switch deserve to be defined and documented in mk/config.mk.

I agree, but while we're at it, defining CFG_SEMIHOSTING_CONSOLE=y should be enough. Currently some platform-specific code is needed (for example QEMUv8 below). We should be able to avoid that.

commit e66d6b8fa8b52a0eaa87de755fc6b0f434e07306 (HEAD -> wip/semihosting, jf/wip/semihosting)
Author: Jerome Forissier <[email protected]>
Date:   Thu Feb 22 16:28:25 2024 +0100

    qemuv8: use semihosting cosole driver when CFG_SEMIHOSTING_CONSOLE=y

    Implement semihosting console for Aarch64 QEMU.

    Signed-off-by: Jerome Forissier <[email protected]>

diff --git a/core/arch/arm/plat-vexpress/main.c b/core/arch/arm/plat-vexpress/main.c
index 42116c2f2..a02017bc6 100644
--- a/core/arch/arm/plat-vexpress/main.c
+++ b/core/arch/arm/plat-vexpress/main.c
@@ -10,6 +10,7 @@
 #include <drivers/gic.h>
 #include <drivers/hfic.h>
 #include <drivers/pl011.h>
+#include <drivers/semihosting_console.h>
 #include <drivers/tzc400.h>
 #include <initcall.h>
 #include <keep.h>
@@ -27,6 +28,7 @@
 #include <trace.h>

 static struct pl011_data console_data __nex_bss;
+static struct semihosting_console_data sh_console_data __nex_bss;

 register_phys_mem_pgdir(MEM_AREA_IO_SEC, CONSOLE_UART_BASE, PL011_REG_SIZE);
 #if defined(PLATFORM_FLAVOR_fvp)
@@ -82,9 +84,14 @@ void boot_primary_init_intc(void)

 void console_init(void)
 {
-       pl011_init(&console_data, CONSOLE_UART_BASE, CONSOLE_UART_CLK_IN_HZ,
-                  CONSOLE_BAUDRATE);
-       register_serial_console(&console_data.chip);
+       if (IS_ENABLED(CFG_SEMIHOSTING_CONSOLE)) {
+               semihosting_console_init(&sh_console_data, "optee.log");
+               register_serial_console(&sh_console_data.chip);
+       } else {
+               pl011_init(&console_data, CONSOLE_UART_BASE,
+                          CONSOLE_UART_CLK_IN_HZ, CONSOLE_BAUDRATE);
+               register_serial_console(&console_data.chip);
+       }
 }

 #if (defined(CFG_GIC) || defined(CFG_CORE_HAFNIUM_INTC)) && \

jforissier · 2024-02-23T13:37:05Z

@gagachang when adding fixup commits you should rather keep the subject of the original commit because it will help when it is time to rebase and squash the commits later at the end of the review. For example "drivers: Address comments for semihosting_console" could be "[Review] drivers: Implement semihosting based console driver for log".

core/include/drivers/semihosting_console.h

gagachang · 2024-02-23T14:03:51Z

@gagachang when adding fixup commits you should rather keep the subject of the original commit because it will help when it is time to rebase and squash the commits later at the end of the review. For example "drivers: Address comments for semihosting_console" could be "[Review] drivers: Implement semihosting based console driver for log".

I am sorry for missing that. Subjects will be fixed in next force push.

gagachang · 2024-02-23T14:28:58Z

Currently some platform-specific code is needed (for example QEMUv8 below). We should be able to avoid that.

Maybe we can change the code in entry_a64.S ?

#ifdef CFG_SEMIHOSTING_CONSOLE
        bl	semihosting_console_init_helper    // Helper prepares console data and file_path
#else
	bl	console_init
#endif

jforissier · 2024-02-23T14:36:43Z

Currently some platform-specific code is needed (for example QEMUv8 below). We should be able to avoid that.

Maybe we can change the code in entry_a64.S ?
#ifdef CFG_SEMIHOSTING_CONSOLE
        bl	semihosting_console_init_helper    // Helper prepares console data and file_path
#else
	bl	console_init
#endif

Or we could decide console_init() is common code in core/kernel/console.c. It would deal with semihosting (and possibly other things like CFG_CBMEM_CONSOLE), then the platform-specific console_init() functions would be renamed plat_console_init().

jforissier · 2024-02-23T14:37:51Z

I successfully enabled the semihosting console for Aarch64 🎉 see 0a49e1c...jforissier:optee_os:semihosting-aarch64. I will create a PR after this PR is reviewed and merged for simplicity.

gagachang · 2024-02-27T14:59:37Z

Comments on commit message for "mk: config.mk: Add semihosting related configurations": Can remove the enumeration 1., 2., 3.. Prfer replace the word 'kernel' with 'core'.

This commit is split and merged into previous two commits respectively, suggested by @jforissier .

jforissier

Minor: I suggest applying "core: riscv: Add semihosting.S for semihosting instructions" after "core: kernel: Add semihosting functions".
For "libutils: Import part of sys/fcntl.h":
Acked-by: Jerome Forissier <[email protected]>
For "core: kernel: Add semihosting functions": one comment, with that:
Reviewed-by: Jerome Forissier <[email protected]>
For "core: Refactor console_init() and introduce plat_console_init()":
Reviewed-by: Jerome Forissier <[email protected]>
For "drivers: Implement semihosting based console driver for log":
You may remove the paragraph about semihosting and keep only "This commit implements a simple console driver...". Add it to commit description for "core: kernel: Add semihosting functions".

core/kernel/semihosting.c

gagachang · 2024-02-27T15:40:34Z

Minor: I suggest applying "core: riscv: Add semihosting.S for semihosting instructions" after "core: kernel: Add semihosting functions".

Yes it makes sense.

You may remove the paragraph about semihosting and keep only "This commit implements a simple console driver...". Add it to commit description for "core: kernel: Add semihosting functions".

Done, the paragraph is moved to "core: kernel: Add semihosting functions".

jforissier · 2024-02-27T16:01:34Z

Tested on Aarch64 QEMU. I just had to add the __do_semihosting() implementation and a minor fix to the plat-vexpress console initialization code, see 1f6295e...jforissier:optee_os:semihosting-aarch64-2
Good work @gagachang 😉

jenswi-linaro

I'm not sure we want to do "libutils: Move _ansi.h to include folder"

core/kernel/console.c

core/kernel/semihosting.c

lib/libutils/isoc/include/sys/_default_fcntl.h

gagachang · 2024-02-28T10:43:40Z

I'm not sure we want to do "libutils: Move _ansi.h to include folder"

I remove this commit since we only copy necessary definitions into sys/fcntl.h

core/kernel/semihosting.c

core/include/drivers/semihosting_console.h

core/kernel/console.c

core/include/kernel/semihosting.h

jforissier · 2024-02-28T13:33:49Z

My review/ack tags still apply.

core/kernel/semihosting.c

mk/config.mk

jenswi-linaro · 2024-02-28T17:15:22Z

Reviewed-by: Jens Wiklander <[email protected]>

gagachang · 2024-02-28T17:23:00Z

Fix commit message of libutils: Import part of sys/fcntl.h and apply review tag and rebase to latest master.

etienne-lms

Acked-by: Etienne Carriere <[email protected]> for commit
"libutils: Import part of sys/fcntl.h".

Reviewed-by: Etienne Carriere <[email protected]> for commit
"core: Refactor console_init() and introduce plat_console_init()".

Comments for commit "drivers: Implement semihosting based console driver for log": we prefer have the commit message at the imperative mode:

-This commit implements a simple console driver which uses semihosting
+Implement a simple console driver which uses...

A equivalent (and late) comment for commit "core: riscv: Add semihosting.S for semihosting instructions":

 RISC-V architecture has defined the semihosting binary interface, which
 consists of a special trap instruction sequence, in:
 https://github.com/riscv-non-isa/riscv-semihosting
 
-This commit adds semihosting.S into RISC-V architecture folder and
-implements the trap instruction sequence.
+Add semihosting.S into RISC-V kernel folder to implement the trap
+instruction sequence.

mk/config.mk

Import part of sys/fcntl.h for necessary file flags, from newlib: - newlib/newlib/libc/include/sys/fcntl.h Signed-off-by: Alvin Chang <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

Semihosting is a mechanism that enables target to communicate and use I/O facilities on a host computer which is running a debugger, such as GDB. The I/O facilities include character {read|write} {from|to} the semihosting host side console or a file. In other words, OP-TEE OS can output log to the host side console or the host side file, if there is a semihosting host and OP-TEE OS requests the semihosting operations to that host. If CFG_SEMIHOSTING is enabled, some semihosting functions will be compiled into OP-TEE kernel, including: - semihosting_sys_readc() - semihosting_sys_writec() - semihosting_open() - semihosting_read() - semihosting_write() - semihosting_close() Note that the architectures which support semihosting should provide their implementation of __do_semihosting(), which performs semihosting instruction to raise the semihosting request. Signed-off-by: Alvin Chang <[email protected]> Reviewed-by: Etienne Carriere <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

RISC-V architecture has defined the semihosting binary interface, which consists of a special trap instruction sequence, in: https://github.com/riscv-non-isa/riscv-semihosting Add semihosting.S into RISC-V kernel folder to implement the trap instruction sequence. Signed-off-by: Alvin Chang <[email protected]> Acked-by: Jerome Forissier <[email protected]> Acked-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

Since there are some cross-platform console drivers, we let console_init() be common code to have a chance to initialize those console drivers (e.g., semihosting console). If the cross-platform console drivers are not configured to be compiled, plat_console_init() will be invoked to initialize platform-specific console driver. Signed-off-by: Alvin Chang <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

etienne-lms

Comment for commit

The commit message could be update accordingly:

 - If the caller of semihosting_console_init() provides the path of the
   file, the driver will try to open that file, and output the log to
   that host side file.
 - If the caller of semihosting_console_init() does not provide the path
+  of the file, the driver will try to output the log to host side
-  of the file, the driver will connect the console to the host debug
   console directly.

mk/config.mk

Implement a simple console driver which uses semihosting operations to read/write the trace messages. There are two paths to output the trace messages: - If the caller of semihosting_console_init() provides the path of the file, the driver will try to open that file, and output the log to that host side file. - If the caller of semihosting_console_init() does not provide the path of the file, the driver will connect the console to the host debug console directly. If CFG_SEMIHOSTING_CONSOLE is enabled, OP-TEE will try to initialize the semihosting console driver by given CFG_SEMIHOSTING_CONSOLE_FILE. Signed-off-by: Alvin Chang <[email protected]> Acked-by: Jerome Forissier <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

jforissier · 2024-03-04T09:05:21Z

@gagachang thanks!

gagachang force-pushed the dev-semihosting branch 2 times, most recently from cbfaa08 to ebef753 Compare February 21, 2024 09:18

jforissier reviewed Feb 21, 2024

View reviewed changes

lib/libutils/isoc/include/sys/_default_fcntl.h Outdated Show resolved Hide resolved

core/kernel/semihosting.c Show resolved Hide resolved

mk/config.mk Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch 2 times, most recently from 8201769 to 0a49e1c Compare February 21, 2024 16:52

jforissier reviewed Feb 21, 2024

View reviewed changes

core/arch/riscv/kernel/sub.mk Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch from 0a49e1c to 883e246 Compare February 22, 2024 01:23

etienne-lms reviewed Feb 22, 2024

View reviewed changes

core/include/drivers/semihosting_console.h Outdated Show resolved Hide resolved

core/include/drivers/semihosting_console.h Outdated Show resolved Hide resolved

core/include/kernel/semihosting.h Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch from 1723ab8 to c4e2b5c Compare February 22, 2024 13:48

etienne-lms reviewed Feb 23, 2024

View reviewed changes

gagachang force-pushed the dev-semihosting branch 2 times, most recently from 69a53ff to 1bd1a1d Compare February 23, 2024 09:30

gagachang force-pushed the dev-semihosting branch from 1bd1a1d to d21e9f7 Compare February 23, 2024 09:41

gagachang force-pushed the dev-semihosting branch 2 times, most recently from 56e2bdf to 0d902a0 Compare February 23, 2024 13:16

jforissier reviewed Feb 23, 2024

View reviewed changes

core/include/drivers/semihosting_console.h Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch 2 times, most recently from 3e7d68d to 6f83af5 Compare February 23, 2024 14:36

gagachang force-pushed the dev-semihosting branch from 49765c2 to f542f6c Compare February 27, 2024 14:54

jforissier reviewed Feb 27, 2024

View reviewed changes

core/kernel/semihosting.c Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch from f542f6c to 1f6295e Compare February 27, 2024 15:36

jenswi-linaro reviewed Feb 28, 2024

View reviewed changes

gagachang force-pushed the dev-semihosting branch from 1f6295e to 062625a Compare February 28, 2024 10:40

jenswi-linaro reviewed Feb 28, 2024

View reviewed changes

core/kernel/semihosting.c Outdated Show resolved Hide resolved

etienne-lms reviewed Feb 28, 2024

View reviewed changes

core/include/drivers/semihosting_console.h Show resolved Hide resolved

core/kernel/console.c Outdated Show resolved Hide resolved

core/include/kernel/semihosting.h Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch from 062625a to 24d0645 Compare February 28, 2024 13:23

etienne-lms reviewed Feb 28, 2024

View reviewed changes

core/kernel/semihosting.c Show resolved Hide resolved

core/kernel/semihosting.c Show resolved Hide resolved

mk/config.mk Show resolved Hide resolved

mk/config.mk Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch from 24d0645 to 35cb6b0 Compare February 28, 2024 17:13

gagachang force-pushed the dev-semihosting branch 2 times, most recently from 2234612 to f8a34cb Compare February 28, 2024 17:21

etienne-lms reviewed Feb 29, 2024

View reviewed changes

mk/config.mk Outdated Show resolved Hide resolved

mk/config.mk Outdated Show resolved Hide resolved

gagachang added 4 commits March 1, 2024 11:04

gagachang force-pushed the dev-semihosting branch from f8a34cb to 4779839 Compare March 1, 2024 03:11

etienne-lms reviewed Mar 1, 2024

View reviewed changes

mk/config.mk Outdated Show resolved Hide resolved

gagachang force-pushed the dev-semihosting branch from 4779839 to bd19a9b Compare March 1, 2024 08:52

jforissier merged commit 458ef44 into OP-TEE:master Mar 4, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce semihosting facilities and semihosting console driver #6700

Introduce semihosting facilities and semihosting console driver #6700

gagachang commented Feb 21, 2024 •

edited

Loading

jforissier left a comment

gagachang commented Feb 21, 2024

gagachang commented Feb 23, 2024

etienne-lms commented Feb 23, 2024

gagachang commented Feb 23, 2024

gagachang commented Feb 23, 2024

jforissier commented Feb 23, 2024

jforissier commented Feb 23, 2024

gagachang commented Feb 23, 2024

gagachang commented Feb 23, 2024

jforissier commented Feb 23, 2024

jforissier commented Feb 23, 2024

gagachang commented Feb 27, 2024 •

edited

Loading

jforissier left a comment

gagachang commented Feb 27, 2024

jforissier commented Feb 27, 2024

jenswi-linaro left a comment

gagachang commented Feb 28, 2024

jforissier commented Feb 28, 2024

jenswi-linaro commented Feb 28, 2024

gagachang commented Feb 28, 2024 •

edited

Loading

etienne-lms left a comment

etienne-lms left a comment

jforissier commented Mar 4, 2024

Introduce semihosting facilities and semihosting console driver #6700

Introduce semihosting facilities and semihosting console driver #6700

Conversation

gagachang commented Feb 21, 2024 • edited Loading

jforissier left a comment

Choose a reason for hiding this comment

gagachang commented Feb 21, 2024

gagachang commented Feb 23, 2024

etienne-lms commented Feb 23, 2024

gagachang commented Feb 23, 2024

gagachang commented Feb 23, 2024

jforissier commented Feb 23, 2024

jforissier commented Feb 23, 2024

gagachang commented Feb 23, 2024

gagachang commented Feb 23, 2024

jforissier commented Feb 23, 2024

jforissier commented Feb 23, 2024

gagachang commented Feb 27, 2024 • edited Loading

jforissier left a comment

Choose a reason for hiding this comment

gagachang commented Feb 27, 2024

jforissier commented Feb 27, 2024

jenswi-linaro left a comment

Choose a reason for hiding this comment

gagachang commented Feb 28, 2024

jforissier commented Feb 28, 2024

jenswi-linaro commented Feb 28, 2024

gagachang commented Feb 28, 2024 • edited Loading

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

jforissier commented Mar 4, 2024

gagachang commented Feb 21, 2024 •

edited

Loading

gagachang commented Feb 27, 2024 •

edited

Loading

gagachang commented Feb 28, 2024 •

edited

Loading