Skip to contentSkip to conversation

OpenSBI debug console support

PATCH Superseded Anup Patel 2 participants 9 messages Nov 23, 2022

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Anup Patel

author
#0 lore raw

v2 cover OpenSBI debug console support

The latest draft-v4 proposal of SBI debug console can be found here:
https://lists.riscv.org/g/tech-unixplatformspec/message/1815

This series implements SBI debug console support in OpenSBI as-per above draft proposal.

These patches can also be found in riscv_sbi_dbcn_v2 branch
at: https://github.com/avpatel/opensbi.git

Changes since v1:
- New PATCH3 to implement sbi_domain_check_addr_range() function
- Added checks in PATCH4 for location and size of string to print
- New PATCH5 to add console_puts() callback in the console device
- New PATCH6 to implement console_puts() for semihosting

Anup Patel (6):
include: Add defines for SBI debug console extension
lib: sbi: Add sbi_nputs() function
lib: sbi: Add sbi_domain_check_addr_range() function
lib: sbi: Implement SBI debug console extension
lib: sbi: Add console_puts() callback in the console device
lib: utils/serial: Implement console_puts() for semihosting

 include/sbi/sbi_console.h         |  5 +++
 include/sbi/sbi_domain.h          | 15 ++++++++
 include/sbi/sbi_ecall_interface.h |  4 ++
 lib/sbi/Kconfig                   |  4 ++
 lib/sbi/objects.mk                |  3 ++
 lib/sbi/sbi_console.c             | 25 +++++++++++--
 lib/sbi/sbi_domain.c              | 47 +++++++++++++++++++++++
 lib/sbi/sbi_ecall_dbcn.c          | 62 +++++++++++++++++++++++++++++++
 lib/utils/serial/semihosting.c    | 33 ++++++++++++++++
 9 files changed, 195 insertions(+), 3 deletions(-)
 create mode 100644 lib/sbi/sbi_ecall_dbcn.c
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v2 1/6 include: Add defines for SBI debug console extension

We add SBI debug console extension related defines/enum to the SBI ecall interface header.

---
 include/sbi/sbi_ecall_interface.h | 4 ++++
 1 file changed, 4 insertions(+)
include/sbi/sbi_ecall_interface.h +4 -0
index a3f2bf4..1dd3874 100644
@@ -29,6 +29,7 @@
 #define SBI_EXT_HSM				0x48534D
 #define SBI_EXT_SRST				0x53525354
 #define SBI_EXT_PMU				0x504D55
+#define SBI_EXT_DBCN				0x4442434E
 
 /* SBI function IDs for BASE extension*/
 #define SBI_EXT_BASE_GET_SPEC_VERSION		0x0
@@ -230,6 +231,9 @@ enum sbi_pmu_ctr_type {
 /* Flags defined for counter stop function */
 #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
 
+/* SBI function IDs for DBCN extension */
+#define SBI_EXT_DBCN_CONSOLE_PUTS		0x0
+
 /* SBI base specification related macros */
 #define SBI_SPEC_VERSION_MAJOR_OFFSET		24
 #define SBI_SPEC_VERSION_MAJOR_MASK		0x7f
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH v2 2/6 lib: sbi: Add sbi_nputs() function

In reply to Anup Patel

We add new sbi_nputs() which help us print a fixed number of characters from a physical memory location.

---
 include/sbi/sbi_console.h |  2 ++
 lib/sbi/sbi_console.c     | 10 ++++++++++
 2 files changed, 12 insertions(+)
include/sbi/sbi_console.h +2 -0
index e15b55d..1bdeeb9 100644
@@ -33,6 +33,8 @@ void sbi_putc(char ch);
 
 void sbi_puts(const char *str);
 
+void sbi_nputs(const char *str, unsigned long len);
+
 void sbi_gets(char *s, int maxwidth, char endchar);
 
 int __printf(2, 3) sbi_sprintf(char *out, const char *format, ...);
lib/sbi/sbi_console.c +10 -0
index cfef2f8..a975708 100644
@@ -51,6 +51,16 @@ void sbi_puts(const char *str)
 	spin_unlock(&console_out_lock);
 }
 
+void sbi_nputs(const char *str, unsigned long len)
+{
+	unsigned long i;
+
+	spin_lock(&console_out_lock);
+	for (i = 0; i < len; i++)
+		sbi_putc(str[i]);
+	spin_unlock(&console_out_lock);
+}
+
 void sbi_gets(char *s, int maxwidth, char endchar)
 {
 	int ch;
Signature · 1 line
2.34.1
author
#3 lore raw

PATCH v2 3/6 lib: sbi: Add sbi_domain_check_addr_range() function

In reply to Anup Patel

We add sbi_domain_check_addr_range() helper function to check whether a given address range is accessible under a particular domain.

---
 include/sbi/sbi_domain.h | 15 +++++++++++++
 lib/sbi/sbi_domain.c     | 47 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
include/sbi/sbi_domain.h +15 -0
index 5553d21..4afcc37 100644
@@ -154,6 +154,21 @@ bool sbi_domain_check_addr(const struct sbi_domain *dom,
 			   unsigned long addr, unsigned long mode,
 			   unsigned long access_flags);
 
+/**
+ * Check whether we can access specified address range for given mode and
+ * memory region flags under a domain
+ * @param dom pointer to domain
+ * @param addr the start of the address range to be checked
+ * @param size the size of the address range to be checked
+ * @param mode the privilege mode of access
+ * @param access_flags bitmask of domain access types (enum sbi_domain_access)
+ * @return TRUE if access allowed otherwise FALSE
+ */
+bool sbi_domain_check_addr_range(const struct sbi_domain *dom,
+				 unsigned long addr, unsigned long size,
+				 unsigned long mode,
+				 unsigned long access_flags);
+
 /** Dump domain details on the console */
 void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix);
 
lib/sbi/sbi_domain.c +47 -0

59 lines not on this page · raw message

Signature · 1 line
2.34.1
#4 lore raw

PATCH v2 3/6 lib: sbi: Add sbi_domain_check_addr_range() function

? 2022-11-23???? 16:27 +0530?Anup Patel???

Quoted 81 lines

We add sbi_domain_check_addr_range() helper function to check whether a given address range is accessible under a particular domain.

---
?include/sbi/sbi_domain.h | 15 +++++++++++++ ?lib/sbi/sbi_domain.c???? | 47 ++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 62 insertions(+)

include/sbi/sbi_domain.h +0 -0
index 5553d21..4afcc37 100644
@@ -154,6 +154,21 @@ bool sbi_domain_check_addr(const struct sbi_domain *dom,
 ?????????????????????????? unsigned long addr, unsigned long mode,
 ?????????????????????????? unsigned long access_flags);

?

include/sbi/sbi_domain.h +15 -0
+/**
+ * Check whether we can access specified address range for given mode and
+ * memory region flags under a domain
+ * @param dom pointer to domain
+ * @param addr the start of the address range to be checked
+ * @param size the size of the address range to be checked
+ * @param mode the privilege mode of access
+ * @param access_flags bitmask of domain access types (enum sbi_domain_access)
+ * @return TRUE if access allowed otherwise FALSE
+ */
+bool sbi_domain_check_addr_range(const struct sbi_domain *dom,
+??????????????????????????????? unsigned long addr, unsigned long size,
+??????????????????????????????? unsigned long mode,
+??????????????????????????????? unsigned long access_flags);
+

?/** Dump domain details on the console */
?void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix); ?

lib/sbi/sbi_domain.c +0 -0
index f24a8e5..6d19d2c 100644
@@ -195,6 +195,24 @@ static bool is_region_before(const struct sbi_domain_memregion *regA,
 ????????return FALSE;
 ?}

?

lib/sbi/sbi_domain.c +18 -0
+static const struct sbi_domain_memregion *find_region(
+???????????????????????????????????????????????const struct sbi_domain *dom,
+???????????????????????????????????????????????unsigned long addr)
+{
+???????unsigned long rstart, rend;
+???????struct sbi_domain_memregion *reg;
+
+???????sbi_domain_for_each_memregion(dom, reg) {
+???????????????rstart = reg->base;
+???????????????rend = (reg->order < __riscv_xlen) ?
+???????????????????????rstart + ((1UL << reg->order) - 1) : -1UL;
+???????????????if (rstart <= addr && addr <= rend)
+???????????????????????return reg;
+???????}
+
+???????return NULL;
+}
+

?static int sanitize_domain(const struct sbi_platform *plat, ?????????????????????????? struct sbi_domain *dom) ?{

lib/sbi/sbi_domain.c +0 -0
@@ -303,6 +321,35 @@ static int sanitize_domain(const struct sbi_platform *plat,
 ????????return 0;
 ?}

?

lib/sbi/sbi_domain.c +12 -0
+bool sbi_domain_check_addr_range(const struct sbi_domain *dom,
+??????????????????????????????? unsigned long addr, unsigned long size,
+??????????????????????????????? unsigned long mode,
+??????????????????????????????? unsigned long access_flags)
+{
+???????unsigned long pos = 0;
+???????const struct sbi_domain_memregion *reg;
+
+???????if (!dom)
+???????????????return FALSE;
+
+???????while (pos < size) {

lib/sbi/sbi_domain.c

Quoted 10 lines
lib/sbi/sbi_domain.c +10 -0
+???????????????reg = find_region(dom, addr + pos);
+???????????????if (!reg)
+???????????????????????return FALSE;
+
+???????????????if (!sbi_domain_check_addr(dom, addr + pos,
+????????????????????????????????????????? mode, access_flags))
+???????????????????????return FALSE;
+
+???????????????if (reg->order < __riscv_xlen)
+???????????????????????pos += 1UL << reg->order;

addr+pos not always point to the beginning of the region, so the size of the region cannot be added directly.

suggestion:

	pos = reg->base + (1 << reg->order) - addr

Regards,
Xiang W

Quoted 14 lines

+???????????????else
+???????????????????????break;
+???????}

lib/sbi/sbi_domain.c +4 -0
+
+???????return TRUE;
+}
+

?void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix) ?{
????????u32 i, k;

Signature · 1 line
2.34.1
author
#5 lore raw

PATCH v2 3/6 lib: sbi: Add sbi_domain_check_addr_range() function

Quoted 96 lines · Xiang W

? 2022-11-23???? 16:27 +0530?Anup Patel???

Quoted 81 lines

We add sbi_domain_check_addr_range() helper function to check whether a given address range is accessible under a particular domain.

---
 include/sbi/sbi_domain.h | 15 +++++++++++++
 lib/sbi/sbi_domain.c     | 47 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
include/sbi/sbi_domain.h +15 -0
index 5553d21..4afcc37 100644
@@ -154,6 +154,21 @@ bool sbi_domain_check_addr(const struct sbi_domain *dom,
                           unsigned long addr, unsigned long mode,
                           unsigned long access_flags);
 
+/**
+ * Check whether we can access specified address range for given mode and
+ * memory region flags under a domain
+ * @param dom pointer to domain
+ * @param addr the start of the address range to be checked
+ * @param size the size of the address range to be checked
+ * @param mode the privilege mode of access
+ * @param access_flags bitmask of domain access types (enum sbi_domain_access)
+ * @return TRUE if access allowed otherwise FALSE
+ */
+bool sbi_domain_check_addr_range(const struct sbi_domain *dom,
+                                unsigned long addr, unsigned long size,
+                                unsigned long mode,
+                                unsigned long access_flags);
+
 /** Dump domain details on the console */
 void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix);
 
lib/sbi/sbi_domain.c +30 -0
index f24a8e5..6d19d2c 100644
@@ -195,6 +195,24 @@ static bool is_region_before(const struct sbi_domain_memregion *regA,
        return FALSE;
 }
 
+static const struct sbi_domain_memregion *find_region(
+                                               const struct sbi_domain *dom,
+                                               unsigned long addr)
+{
+       unsigned long rstart, rend;
+       struct sbi_domain_memregion *reg;
+
+       sbi_domain_for_each_memregion(dom, reg) {
+               rstart = reg->base;
+               rend = (reg->order < __riscv_xlen) ?
+                       rstart + ((1UL << reg->order) - 1) : -1UL;
+               if (rstart <= addr && addr <= rend)
+                       return reg;
+       }
+
+       return NULL;
+}
+
 static int sanitize_domain(const struct sbi_platform *plat,
                           struct sbi_domain *dom)
 {
@@ -303,6 +321,35 @@ static int sanitize_domain(const struct sbi_platform *plat,
        return 0;
 }
 
+bool sbi_domain_check_addr_range(const struct sbi_domain *dom,
+                                unsigned long addr, unsigned long size,
+                                unsigned long mode,
+                                unsigned long access_flags)
+{
+       unsigned long pos = 0;
+       const struct sbi_domain_memregion *reg;
+
+       if (!dom)
+               return FALSE;
+
+       while (pos < size) {

lib/sbi/sbi_domain.c static int sanitize_domain(const struct sbi_platform *plat, · line 345

Quoted 10 lines
lib/sbi/sbi_domain.c +10 -0
+               reg = find_region(dom, addr + pos);
+               if (!reg)
+                       return FALSE;
+
+               if (!sbi_domain_check_addr(dom, addr + pos,
+                                          mode, access_flags))
+                       return FALSE;
+
+               if (reg->order < __riscv_xlen)
+                       pos += 1UL << reg->order;

addr+pos not always point to the beginning of the region, so the size of the region cannot be added directly.

Quoted 2 lines

suggestion:

        pos = reg->base + (1 << reg->order) - addr

This loop seems to be broken for overlapping regions.

What you are suggesting will also not work for overlapping regions.

Let me rethink.

Regards,
Anup

Quoted 24 lines

Regards,
Xiang W

Quoted 11 lines
lib/sbi/sbi_domain.c +7 -1
+               else
+                       break;
+       }
+
+       return TRUE;
+}
+
 void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix)
 {
        u32 i, k;
--
Quoted 3 lines

2.34.1

--
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

author
#6 lore raw

PATCH v2 4/6 lib: sbi: Implement SBI debug console extension

In reply to Anup Patel

We implement SBI debug console extension as one of the replacement SBI extensions. This extension is only available when OpenSBI platform provides a console device to generic library.

---
 lib/sbi/Kconfig          |  4 +++
 lib/sbi/objects.mk       |  3 ++
 lib/sbi/sbi_ecall_dbcn.c | 62 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+)
 create mode 100644 lib/sbi/sbi_ecall_dbcn.c
lib/sbi/Kconfig +4 -0
index df74bba..ef6728b 100644
@@ -26,6 +26,10 @@ config SBI_ECALL_PMU
 	bool "Performance Monitoring Unit extension"
 	default y
 
+config SBI_ECALL_DBCN
+	bool "Debug Console extension"
+	default y
+
 config SBI_ECALL_LEGACY
 	bool "SBI v0.1 legacy extensions"
 	default y
lib/sbi/objects.mk +3 -0
index c774ebb..319f38d 100644
@@ -37,6 +37,9 @@ libsbi-objs-$(CONFIG_SBI_ECALL_SRST) += sbi_ecall_srst.o
 carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_PMU) += ecall_pmu
 libsbi-objs-$(CONFIG_SBI_ECALL_PMU) += sbi_ecall_pmu.o
 
+carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_DBCN) += ecall_dbcn
+libsbi-objs-$(CONFIG_SBI_ECALL_DBCN) += sbi_ecall_dbcn.o
+
 carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_LEGACY) += ecall_legacy
 libsbi-objs-$(CONFIG_SBI_ECALL_LEGACY) += sbi_ecall_legacy.o
 
lib/sbi/sbi_ecall_dbcn.c new file +62 -0

62 lines not on this page · raw message

Signature · 1 line
2.34.1
author
#7 lore raw

PATCH v2 5/6 lib: sbi: Add console_puts() callback in the console device

In reply to Anup Patel

We add console_puts() callback in the console device which allows console drivers (such as semihosting) to implement a specialized way to output character string.

---
 include/sbi/sbi_console.h |  3 +++
 lib/sbi/sbi_console.c     | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)
include/sbi/sbi_console.h +3 -0
index 1bdeeb9..7e757a7 100644
@@ -19,6 +19,9 @@ struct sbi_console_device {
 	/** Write a character to the console output */
 	void (*console_putc)(char ch);
 
+	/** Write a character string to the console output */
+	void (*console_puts)(const char *str, unsigned long len);
+
 	/** Read a character from the console input */
 	int (*console_getc)(void);
 };
lib/sbi/sbi_console.c +14 -5
index a975708..3c98f4e 100644
@@ -12,6 +12,7 @@
 #include <sbi/sbi_hart.h>
 #include <sbi/sbi_platform.h>
 #include <sbi/sbi_scratch.h>
+#include <sbi/sbi_string.h>
 
 static const struct sbi_console_device *console_dev = NULL;
 static spinlock_t console_out_lock	       = SPIN_LOCK_INITIALIZER;
@@ -44,9 +45,13 @@ void sbi_putc(char ch)
 void sbi_puts(const char *str)
 {
 	spin_lock(&console_out_lock);
-	while (*str) {
-		sbi_putc(*str);
-		str++;
+	if (console_dev && console_dev->console_puts) {
+		console_dev->console_puts(str, sbi_strlen(str));
+	} else {
+		while (*str) {
+			sbi_putc(*str);
+			str++;
+		}
 	}
 	spin_unlock(&console_out_lock);
 }
@@ -56,8 +61,12 @@ void sbi_nputs(const char *str, unsigned long len)
 	unsigned long i;
 
 	spin_lock(&console_out_lock);
-	for (i = 0; i < len; i++)
-		sbi_putc(str[i]);
+	if (console_dev && console_dev->console_puts) {
+		console_dev->console_puts(str, len);
+	} else {
+		for (i = 0; i < len; i++)
+			sbi_putc(str[i]);
+	}
 	spin_unlock(&console_out_lock);
 }
 
Signature · 1 line
2.34.1
author
#8 lore raw

PATCH v2 6/6 lib: utils/serial: Implement console_puts() for semihosting

In reply to Anup Patel

We implement console_puts() for semihosting serial driver to speed-up semihosting based prints.

---
 lib/utils/serial/semihosting.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)
lib/utils/serial/semihosting.c +33 -0
index 86fa296..773b75e 100644
@@ -15,6 +15,7 @@
 
 #define SYSOPEN     0x01
 #define SYSWRITEC   0x03
+#define SYSWRITE    0x05
 #define SYSREAD     0x06
 #define SYSREADC    0x07
 #define SYSERRNO	0x13
@@ -93,6 +94,7 @@ static int semihosting_errno(void)
 }
 
 static int semihosting_infd = SBI_ENODEV;
+static int semihosting_outfd = SBI_ENODEV;
 
 static long semihosting_open(const char *fname, enum semihosting_open_mode mode)
 {
@@ -141,6 +143,21 @@ static long semihosting_read(long fd, void *memp, size_t len)
 	return len - ret;
 }
 
+static long semihosting_write(long fd, const void *memp, size_t len)
+{
+	long ret;
+	struct semihosting_rdwr_s write;
+
+	write.fd = fd;
+	write.memp = (void *)memp;
+	write.len = len;
+
+	ret = semihosting_trap(SYSWRITE, &write);
+	if (ret < 0)
+		return semihosting_errno();
+	return len - ret;
+}
+
 /* clang-format on */
 
 static void semihosting_putc(char ch)
@@ -148,6 +165,20 @@ static void semihosting_putc(char ch)
 	semihosting_trap(SYSWRITEC, &ch);
 }
 
+static void semihosting_puts(const char *str, unsigned long len)
+{
+	char ch;
+	unsigned long i;
+
+	if (semihosting_outfd < 0) {
+		for (i = 0; i < len; i++) {
+			ch = str[i];
+			semihosting_trap(SYSWRITEC, &ch);
+		}
+	} else
+		semihosting_write(semihosting_outfd, str, len);
+}
+
 static int semihosting_getc(void)
 {
 	char ch = 0;
@@ -165,12 +196,14 @@ static int semihosting_getc(void)
 static struct sbi_console_device semihosting_console = {
 	.name = "semihosting",
 	.console_putc = semihosting_putc,
+	.console_puts = semihosting_puts,
 	.console_getc = semihosting_getc
 };
 
 int semihosting_init(void)
 {
 	semihosting_infd = semihosting_open(":tt", MODE_READ);
+	semihosting_outfd = semihosting_open(":tt", MODE_WRITE);
 
 	sbi_console_set_device(&semihosting_console);
 
Signature · 1 line
2.34.1