[PATCH v5 0/8] OpenSBI debug console support

[PATCH v5 0/8] OpenSBI debug console support

[Anup Patel]

The latest proposal of SBI debug console extension can be found at:
https://lists.riscv.org/g/tech-prs/message/96

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

To test these patches corresponding Linux patches can be found in
riscv_sbi_dbcn_v1 branch at: https://github.com/avpatel/linux.git

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

Changes since v4:
 - Rebased on latest OpenSBI sources
 - Updated Reviewed-by tags
 - Added new PATCH8 to speed-up sbi_printf() using nputs()

Changes since v3:
 - Rebased on OpenSBI v1.2 release
 - Updated Reviewed-by tags
 - Added new PATCH3 for sbi_ngets() function
 - Updated SBI debug console implementation in PATCH5 as-per latest
   proposal having both console write() and read() functions.

Changes since v2:
 - Reworked sbi_domain_check_addr_range() in PATCH3 to make it work
   for overlapping regions.

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 (8):
  include: Add defines for SBI debug console extension
  lib: sbi: Add sbi_nputs() function
  lib: sbi: Add sbi_ngets() 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
  lib: sbi: Speed-up sbi_printf() and friends using nputs()

 include/sbi/sbi_console.h         |  7 +++
 include/sbi/sbi_domain.h          | 15 +++++++
 include/sbi/sbi_ecall_interface.h |  6 +++
 lib/sbi/Kconfig                   |  4 ++
 lib/sbi/objects.mk                |  3 ++
 lib/sbi/sbi_console.c             | 64 ++++++++++++++++++++++++---
 lib/sbi/sbi_domain.c              | 69 +++++++++++++++++++++++++++++
 lib/sbi/sbi_ecall_dbcn.c          | 72 +++++++++++++++++++++++++++++++
 lib/utils/serial/semihosting.c    | 33 ++++++++++++++
 9 files changed, 266 insertions(+), 7 deletions(-)
 create mode 100644 lib/sbi/sbi_ecall_dbcn.c

-- 
2.34.1

[PATCH v5 1/8] include: Add defines for SBI debug console extension

[Anup Patel]

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

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Xiang W <wxjstz@126.com>
---
 include/sbi/sbi_ecall_interface.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/sbi/sbi_ecall_interface.h b/include/sbi/sbi_ecall_interface.h
index a3f2bf4..9d6f474 100644
--- a/include/sbi/sbi_ecall_interface.h
+++ b/include/sbi/sbi_ecall_interface.h
@@ -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,11 @@ 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_WRITE		0x0
+#define SBI_EXT_DBCN_CONSOLE_READ		0x1
+#define SBI_EXT_DBCN_CONSOLE_WRITE_BYTE		0x2
+
 /* SBI base specification related macros */
 #define SBI_SPEC_VERSION_MAJOR_OFFSET		24
 #define SBI_SPEC_VERSION_MAJOR_MASK		0x7f
-- 
2.34.1

[PATCH v5 2/8] lib: sbi: Add sbi_nputs() function

[Anup Patel]

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

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Xiang W <wxjstz@126.com>
---
 include/sbi/sbi_console.h |  2 ++
 lib/sbi/sbi_console.c     | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
index e15b55d..1bdeeb9 100644
--- a/include/sbi/sbi_console.h
+++ b/include/sbi/sbi_console.h
@@ -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, ...);
diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
index bb6e1ef..946c61d 100644
--- a/lib/sbi/sbi_console.c
+++ b/lib/sbi/sbi_console.c
@@ -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;
-- 
2.34.1

[PATCH v5 3/8] lib: sbi: Add sbi_ngets() function

[Anup Patel]

We add new sbi_ngets() which help us read characters into a
physical memory location.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
---
 include/sbi/sbi_console.h |  2 ++
 lib/sbi/sbi_console.c     | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
index 1bdeeb9..660c239 100644
--- a/include/sbi/sbi_console.h
+++ b/include/sbi/sbi_console.h
@@ -37,6 +37,8 @@ void sbi_nputs(const char *str, unsigned long len);
 
 void sbi_gets(char *s, int maxwidth, char endchar);
 
+unsigned long sbi_ngets(char *str, unsigned long len);
+
 int __printf(2, 3) sbi_sprintf(char *out, const char *format, ...);
 
 int __printf(3, 4) sbi_snprintf(char *out, u32 out_sz, const char *format, ...);
diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
index 946c61d..89d6a49 100644
--- a/lib/sbi/sbi_console.c
+++ b/lib/sbi/sbi_console.c
@@ -74,6 +74,21 @@ void sbi_gets(char *s, int maxwidth, char endchar)
 	*retval = '\0';
 }
 
+unsigned long sbi_ngets(char *str, unsigned long len)
+{
+	int ch;
+	unsigned long i;
+
+	for (i = 0; i < len; i++) {
+		ch = sbi_getc();
+		if (ch < 0)
+			break;
+		str[i] = ch;
+	}
+
+	return i;
+}
+
 #define PAD_RIGHT 1
 #define PAD_ZERO 2
 #define PAD_ALTERNATE 4
-- 
2.34.1

[PATCH v5 4/8] lib: sbi: Add sbi_domain_check_addr_range() function

[Anup Patel]

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

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Xiang W <wxjstz@126.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 include/sbi/sbi_domain.h | 15 +++++++++
 lib/sbi/sbi_domain.c     | 69 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index bbb3eff..ab1a944 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -196,6 +196,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);
 
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 60fda01..683b6cf 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -208,6 +208,44 @@ 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 const struct sbi_domain_memregion *find_next_subset_region(
+				const struct sbi_domain *dom,
+				const struct sbi_domain_memregion *reg,
+				unsigned long addr)
+{
+	struct sbi_domain_memregion *sreg, *ret = NULL;
+
+	sbi_domain_for_each_memregion(dom, sreg) {
+		if (sreg == reg || (sreg->base <= addr) ||
+		    !is_region_subset(sreg, reg))
+			continue;
+
+		if (!ret || (sreg->base < ret->base) ||
+		    ((sreg->base == ret->base) && (sreg->order < ret->order)))
+			ret = sreg;
+	}
+
+	return ret;
+}
+
 static int sanitize_domain(const struct sbi_platform *plat,
 			   struct sbi_domain *dom)
 {
@@ -316,6 +354,37 @@ 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 max = addr + size;
+	const struct sbi_domain_memregion *reg, *sreg;
+
+	if (!dom)
+		return false;
+
+	while (addr < max) {
+		reg = find_region(dom, addr);
+		if (!reg)
+			return false;
+
+		if (!sbi_domain_check_addr(dom, addr, mode, access_flags))
+			return false;
+
+		sreg = find_next_subset_region(dom, reg, addr);
+		if (sreg)
+			addr = sreg->base;
+		else if (reg->order < __riscv_xlen)
+			addr = reg->base + (1UL << reg->order);
+		else
+			break;
+	}
+
+	return true;
+}
+
 void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix)
 {
 	u32 i, k;
-- 
2.34.1

[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension

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

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Xiang W <wxjstz@126.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 lib/sbi/Kconfig          |  4 +++
 lib/sbi/objects.mk       |  3 ++
 lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 lib/sbi/sbi_ecall_dbcn.c

diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index df74bba..ef6728b 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -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
diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index c774ebb..319f38d 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -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
 
diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
new file mode 100644
index 0000000..bfa40e9
--- /dev/null
+++ b/lib/sbi/sbi_ecall_dbcn.c
@@ -0,0 +1,72 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2022 Ventana Micro Systems Inc.
+ *
+ * Authors:
+ *   Anup Patel <apatel@ventanamicro.com>
+ */
+
+#include <sbi/sbi_console.h>
+#include <sbi/sbi_domain.h>
+#include <sbi/sbi_error.h>
+#include <sbi/sbi_ecall.h>
+#include <sbi/sbi_ecall_interface.h>
+#include <sbi/sbi_trap.h>
+#include <sbi/riscv_asm.h>
+
+static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
+				  const struct sbi_trap_regs *regs,
+				  unsigned long *out_val,
+				  struct sbi_trap_info *out_trap)
+{
+	ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
+			MSTATUS_MPP_SHIFT;
+
+	switch (funcid) {
+	case SBI_EXT_DBCN_CONSOLE_WRITE:
+	case SBI_EXT_DBCN_CONSOLE_READ:
+		/*
+		 * On RV32, the M-mode can only access the first 4GB of
+		 * the physical address space because M-mode does not have
+		 * MMU to access full 34-bit physical address space.
+		 *
+		 * Based on above, we simply fail if the upper 32bits of
+		 * the physical address (i.e. a2 register) is non-zero on
+		 * RV32.
+		 */
+#if __riscv_xlen == 32
+		if (regs->a2)
+			return SBI_ERR_FAILED;
+#endif
+		if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
+					regs->a1, regs->a0, smode,
+					SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
+			return SBI_EINVALID_ADDR;
+		if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
+			sbi_nputs((const char *)regs->a1, regs->a0);
+		else
+			*out_val = sbi_ngets((char *)regs->a1, regs->a0);
+		return 0;
+	case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
+		sbi_putc(regs->a0);
+		return 0;
+	default:
+		break;
+	}
+
+	return SBI_ENOTSUPP;
+}
+
+static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
+{
+	*out_val = (sbi_console_get_device()) ? 1 : 0;
+	return 0;
+}
+
+struct sbi_ecall_extension ecall_dbcn = {
+	.extid_start = SBI_EXT_DBCN,
+	.extid_end = SBI_EXT_DBCN,
+	.handle = sbi_ecall_dbcn_handler,
+	.probe = sbi_ecall_dbcn_probe,
+};
-- 
2.34.1

[PATCH v5 6/8] lib: sbi: Add console_puts() callback in the console device

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

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Xiang W <wxjstz@126.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 include/sbi/sbi_console.h |  3 +++
 lib/sbi/sbi_console.c     | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
index 660c239..8e95b9d 100644
--- a/include/sbi/sbi_console.h
+++ b/include/sbi/sbi_console.h
@@ -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);
 };
diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
index 89d6a49..c1b9f73 100644
--- a/lib/sbi/sbi_console.c
+++ b/lib/sbi/sbi_console.c
@@ -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);
 }
 
-- 
2.34.1

[PATCH v5 7/8] lib: utils/serial: Implement console_puts() for semihosting

[Anup Patel]

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

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Xiang W <wxjstz@126.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 lib/utils/serial/semihosting.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
index 86fa296..773b75e 100644
--- a/lib/utils/serial/semihosting.c
+++ b/lib/utils/serial/semihosting.c
@@ -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);
 
-- 
2.34.1

[PATCH v5 8/8] lib: sbi: Speed-up sbi_printf() and friends using nputs()

[Anup Patel]

The sbi_printf() is slow for semihosting because it prints one
character at a time. To speed-up sbi_printf() for semihosting,
we use a temporary buffer and nputs().

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 lib/sbi/sbi_console.c | 52 ++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
index c1b9f73..41881d7 100644
--- a/lib/sbi/sbi_console.c
+++ b/lib/sbi/sbi_console.c
@@ -42,31 +42,29 @@ void sbi_putc(char ch)
 	}
 }
 
-void sbi_puts(const char *str)
+static void nputs(const char *str, unsigned long len)
 {
-	spin_lock(&console_out_lock);
+	unsigned long i;
+
 	if (console_dev && console_dev->console_puts) {
-		console_dev->console_puts(str, sbi_strlen(str));
+		console_dev->console_puts(str, len);
 	} else {
-		while (*str) {
-			sbi_putc(*str);
-			str++;
-		}
+		for (i = 0; i < len; i++)
+			sbi_putc(str[i]);
 	}
+}
+
+void sbi_puts(const char *str)
+{
+	spin_lock(&console_out_lock);
+	nputs(str, sbi_strlen(str));
 	spin_unlock(&console_out_lock);
 }
 
 void sbi_nputs(const char *str, unsigned long len)
 {
-	unsigned long i;
-
 	spin_lock(&console_out_lock);
-	if (console_dev && console_dev->console_puts) {
-		console_dev->console_puts(str, len);
-	} else {
-		for (i = 0; i < len; i++)
-			sbi_putc(str[i]);
-	}
+	nputs(str, len);
 	spin_unlock(&console_out_lock);
 }
 
@@ -102,6 +100,7 @@ unsigned long sbi_ngets(char *str, unsigned long len)
 #define PAD_ZERO 2
 #define PAD_ALTERNATE 4
 #define PRINT_BUF_LEN 64
+#define PRINT_TBUF_MAX 128
 
 #define va_start(v, l) __builtin_va_start((v), l)
 #define va_end __builtin_va_end
@@ -217,12 +216,26 @@ static int printi(char **out, u32 *out_len, long long i, int b, int sg,
 
 static int print(char **out, u32 *out_len, const char *format, va_list args)
 {
-	int width, flags;
-	int pc = 0;
-	char scr[2];
+	u32 tbuf_len;
+	int width, flags, pc = 0;
+	char scr[2], *tout, tbuf[PRINT_TBUF_MAX];
+	bool use_tbuf = (!out) ? true : false;
 	unsigned long long tmp;
 
+	if (use_tbuf) {
+		tbuf_len = PRINT_TBUF_MAX;
+		tout = tbuf;
+		out = &tout;
+		out_len = &tbuf_len;
+	}
+
 	for (; *format != 0; ++format) {
+		if (use_tbuf && !tbuf_len) {
+			nputs(tbuf, PRINT_TBUF_MAX);
+			tbuf_len = PRINT_TBUF_MAX;
+			tout = tbuf;
+		}
+
 		if (*format == '%') {
 			++format;
 			width = flags = 0;
@@ -348,6 +361,9 @@ literal:
 		}
 	}
 
+	if (use_tbuf && tbuf_len < PRINT_TBUF_MAX)
+		nputs(tbuf, PRINT_TBUF_MAX - tbuf_len);
+
 	return pc;
 }
 
-- 
2.34.1

[PATCH v5 1/8] include: Add defines for SBI debug console extension

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:03PM +0530, Anup Patel wrote:
> We add SBI debug console extension related defines/enum to the

Not adding any enums.

> SBI ecall interface header.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Xiang W <wxjstz@126.com>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

> ---
>  include/sbi/sbi_ecall_interface.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/sbi/sbi_ecall_interface.h b/include/sbi/sbi_ecall_interface.h
> index a3f2bf4..9d6f474 100644
> --- a/include/sbi/sbi_ecall_interface.h
> +++ b/include/sbi/sbi_ecall_interface.h
> @@ -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,11 @@ 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_WRITE		0x0
> +#define SBI_EXT_DBCN_CONSOLE_READ		0x1
> +#define SBI_EXT_DBCN_CONSOLE_WRITE_BYTE		0x2
> +
>  /* SBI base specification related macros */
>  #define SBI_SPEC_VERSION_MAJOR_OFFSET		24
>  #define SBI_SPEC_VERSION_MAJOR_MASK		0x7f
> -- 
> 2.34.1
> 
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v5 2/8] lib: sbi: Add sbi_nputs() function

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:04PM +0530, Anup Patel wrote:
> We add new sbi_nputs() which help us print a fixed number of characters
> from a physical memory location.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Xiang W <wxjstz@126.com>
> ---
>  include/sbi/sbi_console.h |  2 ++
>  lib/sbi/sbi_console.c     | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
> index e15b55d..1bdeeb9 100644
> --- a/include/sbi/sbi_console.h
> +++ b/include/sbi/sbi_console.h
> @@ -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, ...);
> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> index bb6e1ef..946c61d 100644
> --- a/lib/sbi/sbi_console.c
> +++ b/lib/sbi/sbi_console.c
> @@ -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]);

sbi_putc() adds '\r' after '\n'. So, sbi_nputs("\n\n\n\n", 4)
actually outputs 8 chars, but I imagine the new debug extension
will still report it only wrote 4. Is that a problem?

Thanks,
drew


> +	spin_unlock(&console_out_lock);
> +}
> +
>  void sbi_gets(char *s, int maxwidth, char endchar)
>  {
>  	int ch;
> -- 
> 2.34.1
> 
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v5 3/8] lib: sbi: Add sbi_ngets() function

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:05PM +0530, Anup Patel wrote:
> We add new sbi_ngets() which help us read characters into a
> physical memory location.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> ---
>  include/sbi/sbi_console.h |  2 ++
>  lib/sbi/sbi_console.c     | 15 +++++++++++++++
>  2 files changed, 17 insertions(+)


Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH v5 4/8] lib: sbi: Add sbi_domain_check_addr_range() function

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:06PM +0530, Anup Patel wrote:
> We add sbi_domain_check_addr_range() helper function to check
> whether a given address range is accessible under a particular
> domain.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Xiang W <wxjstz@126.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  include/sbi/sbi_domain.h | 15 +++++++++
>  lib/sbi/sbi_domain.c     | 69 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index bbb3eff..ab1a944 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -196,6 +196,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);
>  
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 60fda01..683b6cf 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -208,6 +208,44 @@ 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 const struct sbi_domain_memregion *find_next_subset_region(
> +				const struct sbi_domain *dom,
> +				const struct sbi_domain_memregion *reg,
> +				unsigned long addr)
> +{
> +	struct sbi_domain_memregion *sreg, *ret = NULL;
> +
> +	sbi_domain_for_each_memregion(dom, sreg) {
> +		if (sreg == reg || (sreg->base <= addr) ||
> +		    !is_region_subset(sreg, reg))
> +			continue;
> +
> +		if (!ret || (sreg->base < ret->base) ||
> +		    ((sreg->base == ret->base) && (sreg->order < ret->order)))
> +			ret = sreg;
> +	}
> +
> +	return ret;
> +}
> +
>  static int sanitize_domain(const struct sbi_platform *plat,
>  			   struct sbi_domain *dom)
>  {
> @@ -316,6 +354,37 @@ 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 max = addr + size;
> +	const struct sbi_domain_memregion *reg, *sreg;
> +
> +	if (!dom)
> +		return false;
> +
> +	while (addr < max) {
> +		reg = find_region(dom, addr);
> +		if (!reg)
> +			return false;
> +
> +		if (!sbi_domain_check_addr(dom, addr, mode, access_flags))
> +			return false;
> +
> +		sreg = find_next_subset_region(dom, reg, addr);
> +		if (sreg)
> +			addr = sreg->base;
> +		else if (reg->order < __riscv_xlen)
> +			addr = reg->base + (1UL << reg->order);
> +		else
> +			break;
> +	}
> +
> +	return true;
> +}
> +

It seems like there should be a way to optimize this check / these
searches, but I suppose we can do that later.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:07PM +0530, Anup Patel wrote:
> 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.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Xiang W <wxjstz@126.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  lib/sbi/Kconfig          |  4 +++
>  lib/sbi/objects.mk       |  3 ++
>  lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 lib/sbi/sbi_ecall_dbcn.c
> 
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index df74bba..ef6728b 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -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
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index c774ebb..319f38d 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -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
>  
> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> new file mode 100644
> index 0000000..bfa40e9
> --- /dev/null
> +++ b/lib/sbi/sbi_ecall_dbcn.c
> @@ -0,0 +1,72 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2022 Ventana Micro Systems Inc.
> + *
> + * Authors:
> + *   Anup Patel <apatel@ventanamicro.com>
> + */
> +
> +#include <sbi/sbi_console.h>
> +#include <sbi/sbi_domain.h>
> +#include <sbi/sbi_error.h>
> +#include <sbi/sbi_ecall.h>
> +#include <sbi/sbi_ecall_interface.h>
> +#include <sbi/sbi_trap.h>
> +#include <sbi/riscv_asm.h>
> +
> +static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
> +				  const struct sbi_trap_regs *regs,
> +				  unsigned long *out_val,
> +				  struct sbi_trap_info *out_trap)
> +{
> +	ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
> +			MSTATUS_MPP_SHIFT;
> +
> +	switch (funcid) {
> +	case SBI_EXT_DBCN_CONSOLE_WRITE:
> +	case SBI_EXT_DBCN_CONSOLE_READ:
> +		/*
> +		 * On RV32, the M-mode can only access the first 4GB of
> +		 * the physical address space because M-mode does not have
> +		 * MMU to access full 34-bit physical address space.
> +		 *
> +		 * Based on above, we simply fail if the upper 32bits of
> +		 * the physical address (i.e. a2 register) is non-zero on
> +		 * RV32.
> +		 */
> +#if __riscv_xlen == 32
> +		if (regs->a2)
> +			return SBI_ERR_FAILED;

The spec drafts says SBI_ERR_FAILED is only for I/O errors. I think
we should extend the spec draft to point this situation out and return
SBI_ERR_INVALID_ADDRESS.

> +#endif
> +		if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
> +					regs->a1, regs->a0, smode,
> +					SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
> +			return SBI_EINVALID_ADDR;

The spec draft says this should be SBI_ERR_INVALID_PARAM

> +		if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
> +			sbi_nputs((const char *)regs->a1, regs->a0);
> +		else
> +			*out_val = sbi_ngets((char *)regs->a1, regs->a0);
> +		return 0;
> +	case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> +		sbi_putc(regs->a0);
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	return SBI_ENOTSUPP;
> +}
> +
> +static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
> +{
> +	*out_val = (sbi_console_get_device()) ? 1 : 0;

nit: Unnecessary ()

> +	return 0;
> +}
> +
> +struct sbi_ecall_extension ecall_dbcn = {
> +	.extid_start = SBI_EXT_DBCN,
> +	.extid_end = SBI_EXT_DBCN,
> +	.handle = sbi_ecall_dbcn_handler,
> +	.probe = sbi_ecall_dbcn_probe,
> +};
> -- 
> 2.34.1
> 
>

Thanks,
drew

[PATCH v5 6/8] lib: sbi: Add console_puts() callback in the console device

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:08PM +0530, Anup Patel wrote:
> 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.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Xiang W <wxjstz@126.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  include/sbi/sbi_console.h |  3 +++
>  lib/sbi/sbi_console.c     | 19 ++++++++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
> index 660c239..8e95b9d 100644
> --- a/include/sbi/sbi_console.h
> +++ b/include/sbi/sbi_console.h
> @@ -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);
>  };
> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> index 89d6a49..c1b9f73 100644
> --- a/lib/sbi/sbi_console.c
> +++ b/lib/sbi/sbi_console.c
> @@ -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);
>  }
>  
> -- 
> 2.34.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH v5 7/8] lib: utils/serial: Implement console_puts() for semihosting

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:09PM +0530, Anup Patel wrote:
> We implement console_puts() for semihosting serial driver to speed-up
> semihosting based prints.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Xiang W <wxjstz@126.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  lib/utils/serial/semihosting.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/lib/utils/serial/semihosting.c b/lib/utils/serial/semihosting.c
> index 86fa296..773b75e 100644
> --- a/lib/utils/serial/semihosting.c
> +++ b/lib/utils/serial/semihosting.c
> @@ -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);
>  
> -- 
> 2.34.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

[PATCH v5 8/8] lib: sbi: Speed-up sbi_printf() and friends using nputs()

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:10PM +0530, Anup Patel wrote:
> The sbi_printf() is slow for semihosting because it prints one
> character at a time. To speed-up sbi_printf() for semihosting,
> we use a temporary buffer and nputs().
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  lib/sbi/sbi_console.c | 52 ++++++++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> index c1b9f73..41881d7 100644
> --- a/lib/sbi/sbi_console.c
> +++ b/lib/sbi/sbi_console.c
> @@ -42,31 +42,29 @@ void sbi_putc(char ch)
>  	}
>  }
>  
> -void sbi_puts(const char *str)
> +static void nputs(const char *str, unsigned long len)
>  {
> -	spin_lock(&console_out_lock);
> +	unsigned long i;
> +
>  	if (console_dev && console_dev->console_puts) {
> -		console_dev->console_puts(str, sbi_strlen(str));
> +		console_dev->console_puts(str, len);
>  	} else {
> -		while (*str) {
> -			sbi_putc(*str);
> -			str++;
> -		}
> +		for (i = 0; i < len; i++)
> +			sbi_putc(str[i]);
>  	}
> +}
> +
> +void sbi_puts(const char *str)
> +{
> +	spin_lock(&console_out_lock);
> +	nputs(str, sbi_strlen(str));
>  	spin_unlock(&console_out_lock);
>  }
>  
>  void sbi_nputs(const char *str, unsigned long len)
>  {
> -	unsigned long i;
> -
>  	spin_lock(&console_out_lock);
> -	if (console_dev && console_dev->console_puts) {
> -		console_dev->console_puts(str, len);
> -	} else {
> -		for (i = 0; i < len; i++)
> -			sbi_putc(str[i]);
> -	}
> +	nputs(str, len);
>  	spin_unlock(&console_out_lock);
>  }
>  
> @@ -102,6 +100,7 @@ unsigned long sbi_ngets(char *str, unsigned long len)
>  #define PAD_ZERO 2
>  #define PAD_ALTERNATE 4
>  #define PRINT_BUF_LEN 64
> +#define PRINT_TBUF_MAX 128
>  
>  #define va_start(v, l) __builtin_va_start((v), l)
>  #define va_end __builtin_va_end
> @@ -217,12 +216,26 @@ static int printi(char **out, u32 *out_len, long long i, int b, int sg,
>  
>  static int print(char **out, u32 *out_len, const char *format, va_list args)
>  {
> -	int width, flags;
> -	int pc = 0;
> -	char scr[2];
> +	u32 tbuf_len;
> +	int width, flags, pc = 0;
> +	char scr[2], *tout, tbuf[PRINT_TBUF_MAX];
> +	bool use_tbuf = (!out) ? true : false;

print() is only called with a non-null 'out' parameter or under
console_out_lock, so we can use global variable for the buffer
to avoid putting it on the stack, and it can be larger.

>  	unsigned long long tmp;
>  
> +	if (use_tbuf) {
> +		tbuf_len = PRINT_TBUF_MAX;
> +		tout = tbuf;
> +		out = &tout;
> +		out_len = &tbuf_len;
> +	}
> +
>  	for (; *format != 0; ++format) {
> +		if (use_tbuf && !tbuf_len) {
> +			nputs(tbuf, PRINT_TBUF_MAX);
> +			tbuf_len = PRINT_TBUF_MAX;
> +			tout = tbuf;
> +		}
> +
>  		if (*format == '%') {
>  			++format;
>  			width = flags = 0;
> @@ -348,6 +361,9 @@ literal:
>  		}
>  	}
>  
> +	if (use_tbuf && tbuf_len < PRINT_TBUF_MAX)
> +		nputs(tbuf, PRINT_TBUF_MAX - tbuf_len);
> +
>  	return pc;
>  }
>  
> -- 
> 2.34.1
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

[PATCH v5 6/8] lib: sbi: Add console_puts() callback in the console device

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:08PM +0530, Anup Patel wrote:
> 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.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Xiang W <wxjstz@126.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  include/sbi/sbi_console.h |  3 +++
>  lib/sbi/sbi_console.c     | 19 ++++++++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
> index 660c239..8e95b9d 100644
> --- a/include/sbi/sbi_console.h
> +++ b/include/sbi/sbi_console.h
> @@ -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);

The spec draft says the write command is non-blocking and returns the
number of bytes written. So this interface should at least return the
number of bytes written. And, maybe it should take a flags parameter
where a NON_BLOCKING flag can be set? But, if we can't enforce the
console to be non-blocking at the SBI implementation level, then
maybe the spec should state that the calls may block?

Thanks,
drew

> +
>  	/** Read a character from the console input */
>  	int (*console_getc)(void);
>  };
> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> index 89d6a49..c1b9f73 100644
> --- a/lib/sbi/sbi_console.c
> +++ b/lib/sbi/sbi_console.c
> @@ -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);
>  }
>  
> -- 
> 2.34.1
> 
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension

[Andrew Jones]

On Fri, Jan 13, 2023 at 05:11:07PM +0530, Anup Patel wrote:
> 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.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Xiang W <wxjstz@126.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  lib/sbi/Kconfig          |  4 +++
>  lib/sbi/objects.mk       |  3 ++
>  lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 lib/sbi/sbi_ecall_dbcn.c
> 
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index df74bba..ef6728b 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -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
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index c774ebb..319f38d 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -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
>  
> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> new file mode 100644
> index 0000000..bfa40e9
> --- /dev/null
> +++ b/lib/sbi/sbi_ecall_dbcn.c
> @@ -0,0 +1,72 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (c) 2022 Ventana Micro Systems Inc.
> + *
> + * Authors:
> + *   Anup Patel <apatel@ventanamicro.com>
> + */
> +
> +#include <sbi/sbi_console.h>
> +#include <sbi/sbi_domain.h>
> +#include <sbi/sbi_error.h>
> +#include <sbi/sbi_ecall.h>
> +#include <sbi/sbi_ecall_interface.h>
> +#include <sbi/sbi_trap.h>
> +#include <sbi/riscv_asm.h>
> +
> +static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
> +				  const struct sbi_trap_regs *regs,
> +				  unsigned long *out_val,
> +				  struct sbi_trap_info *out_trap)
> +{
> +	ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
> +			MSTATUS_MPP_SHIFT;
> +
> +	switch (funcid) {
> +	case SBI_EXT_DBCN_CONSOLE_WRITE:
> +	case SBI_EXT_DBCN_CONSOLE_READ:
> +		/*
> +		 * On RV32, the M-mode can only access the first 4GB of
> +		 * the physical address space because M-mode does not have
> +		 * MMU to access full 34-bit physical address space.
> +		 *
> +		 * Based on above, we simply fail if the upper 32bits of
> +		 * the physical address (i.e. a2 register) is non-zero on
> +		 * RV32.
> +		 */
> +#if __riscv_xlen == 32
> +		if (regs->a2)
> +			return SBI_ERR_FAILED;
> +#endif
> +		if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
> +					regs->a1, regs->a0, smode,
> +					SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
> +			return SBI_EINVALID_ADDR;
> +		if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
> +			sbi_nputs((const char *)regs->a1, regs->a0);

The spec draft says we return the number of bytes written, so we need
sbi_nputs to return that. If we never intend to support partial writes,
then I guess we at least need '*out_val = regs->a0'.

Thanks,
drew

> +		else
> +			*out_val = sbi_ngets((char *)regs->a1, regs->a0);
> +		return 0;
> +	case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> +		sbi_putc(regs->a0);
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	return SBI_ENOTSUPP;
> +}
> +
> +static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
> +{
> +	*out_val = (sbi_console_get_device()) ? 1 : 0;
> +	return 0;
> +}
> +
> +struct sbi_ecall_extension ecall_dbcn = {
> +	.extid_start = SBI_EXT_DBCN,
> +	.extid_end = SBI_EXT_DBCN,
> +	.handle = sbi_ecall_dbcn_handler,
> +	.probe = sbi_ecall_dbcn_probe,
> +};
> -- 
> 2.34.1
> 
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v5 6/8] lib: sbi: Add console_puts() callback in the console device

[Andrew Jones]

On Wed, Feb 01, 2023 at 10:30:13AM +0100, Andrew Jones wrote:
> On Fri, Jan 13, 2023 at 05:11:08PM +0530, Anup Patel wrote:
> > 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.
> > 
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > Reviewed-by: Xiang W <wxjstz@126.com>
> > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > ---
> >  include/sbi/sbi_console.h |  3 +++
> >  lib/sbi/sbi_console.c     | 19 ++++++++++++++-----
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
> > index 660c239..8e95b9d 100644
> > --- a/include/sbi/sbi_console.h
> > +++ b/include/sbi/sbi_console.h
> > @@ -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);
> 
> The spec draft says the write command is non-blocking and returns the
> number of bytes written. So this interface should at least return the
> number of bytes written. And, maybe it should take a flags parameter
> where a NON_BLOCKING flag can be set? But, if we can't enforce the
> console to be non-blocking at the SBI implementation level, then
> maybe the spec should state that the calls may block?
>

Also, if the console fails it'd be good if it returned an error,
allowing SBI_ERR_FAILED to be returned to the SBI caller as the
spec draft states. It'd be good if console_putc() could also
return an error for the same reason.

Thanks,
drew

[PATCH v5 6/8] lib: sbi: Add console_puts() callback in the console device

[Xiang W]

? 2023-02-01???? 10:00 +0100?Andrew Jones???
> On Fri, Jan 13, 2023 at 05:11:08PM +0530, Anup Patel wrote:
> > 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.
> > 
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > Reviewed-by: Xiang W <wxjstz@126.com>
> > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > ---
> > ?include/sbi/sbi_console.h |? 3 +++
> > ?lib/sbi/sbi_console.c???? | 19 ++++++++++++++-----
> > ?2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
> > index 660c239..8e95b9d 100644
> > --- a/include/sbi/sbi_console.h
> > +++ b/include/sbi/sbi_console.h
> > @@ -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);
We should add a comment here that \r\n needs to be output when \n is output.
Otherwise, the output effect of sbi_nputs will be inconsistent.

Regards,
Xiang W
> > +
> > ????????/** Read a character from the console input */
> > ????????int (*console_getc)(void);
> > ?};
> > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> > index 89d6a49..c1b9f73 100644
> > --- a/lib/sbi/sbi_console.c
> > +++ b/lib/sbi/sbi_console.c
> > @@ -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);
> > ?}
> > ?
> > -- 
> > 2.34.1
> > 
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH v5 1/8] include: Add defines for SBI debug console extension

[Anup Patel]

On Tue, Jan 31, 2023 at 10:56 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jan 13, 2023 at 05:11:03PM +0530, Anup Patel wrote:
> > We add SBI debug console extension related defines/enum to the
>
> Not adding any enums.

Okay, I will update.

>
> > SBI ecall interface header.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > Reviewed-by: Xiang W <wxjstz@126.com>
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> > ---
> >  include/sbi/sbi_ecall_interface.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/sbi/sbi_ecall_interface.h b/include/sbi/sbi_ecall_interface.h
> > index a3f2bf4..9d6f474 100644
> > --- a/include/sbi/sbi_ecall_interface.h
> > +++ b/include/sbi/sbi_ecall_interface.h
> > @@ -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,11 @@ 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_WRITE           0x0
> > +#define SBI_EXT_DBCN_CONSOLE_READ            0x1
> > +#define SBI_EXT_DBCN_CONSOLE_WRITE_BYTE              0x2
> > +
> >  /* SBI base specification related macros */
> >  #define SBI_SPEC_VERSION_MAJOR_OFFSET                24
> >  #define SBI_SPEC_VERSION_MAJOR_MASK          0x7f
> > --
> > 2.34.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi

Regards,
Anup

[PATCH v5 2/8] lib: sbi: Add sbi_nputs() function

[Anup Patel]

On Tue, Jan 31, 2023 at 11:02 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jan 13, 2023 at 05:11:04PM +0530, Anup Patel wrote:
> > We add new sbi_nputs() which help us print a fixed number of characters
> > from a physical memory location.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > Reviewed-by: Xiang W <wxjstz@126.com>
> > ---
> >  include/sbi/sbi_console.h |  2 ++
> >  lib/sbi/sbi_console.c     | 10 ++++++++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
> > index e15b55d..1bdeeb9 100644
> > --- a/include/sbi/sbi_console.h
> > +++ b/include/sbi/sbi_console.h
> > @@ -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, ...);
> > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> > index bb6e1ef..946c61d 100644
> > --- a/lib/sbi/sbi_console.c
> > +++ b/lib/sbi/sbi_console.c
> > @@ -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]);
>
> sbi_putc() adds '\r' after '\n'. So, sbi_nputs("\n\n\n\n", 4)
> actually outputs 8 chars, but I imagine the new debug extension
> will still report it only wrote 4. Is that a problem?

I don't see this as a problem because the additional '\r' is only
for correctly rederring the effect of `\n` on console.

In other words, the nputs() function is still printing all the
characters it got as parameters and additionally it is printing
few more characters.

Regards,
Anup

[PATCH v5 2/8] lib: sbi: Add sbi_nputs() function

[Jessica Clarke]

On 9 Feb 2023, at 16:59, Anup Patel <anup@brainfault.org> wrote:
> 
> On Tue, Jan 31, 2023 at 11:02 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>> 
>> On Fri, Jan 13, 2023 at 05:11:04PM +0530, Anup Patel wrote:
>>> We add new sbi_nputs() which help us print a fixed number of characters
>>> from a physical memory location.
>>> 
>>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>>> Reviewed-by: Atish Patra <atishp@rivosinc.com>
>>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>>> Reviewed-by: Xiang W <wxjstz@126.com>
>>> ---
>>> include/sbi/sbi_console.h |  2 ++
>>> lib/sbi/sbi_console.c     | 10 ++++++++++
>>> 2 files changed, 12 insertions(+)
>>> 
>>> diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
>>> index e15b55d..1bdeeb9 100644
>>> --- a/include/sbi/sbi_console.h
>>> +++ b/include/sbi/sbi_console.h
>>> @@ -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, ...);
>>> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
>>> index bb6e1ef..946c61d 100644
>>> --- a/lib/sbi/sbi_console.c
>>> +++ b/lib/sbi/sbi_console.c
>>> @@ -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]);
>> 
>> sbi_putc() adds '\r' after '\n'. So, sbi_nputs("\n\n\n\n", 4)
>> actually outputs 8 chars, but I imagine the new debug extension
>> will still report it only wrote 4. Is that a problem?
> 
> I don't see this as a problem because the additional '\r' is only
> for correctly rederring the effect of `\n` on console.
> 
> In other words, the nputs() function is still printing all the
> characters it got as parameters and additionally it is printing
> few more characters.

Which is the same as the Unix tty layer when onlcr is set.

Jess

[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension

[Anup Patel]

On Wed, Feb 1, 2023 at 2:28 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jan 13, 2023 at 05:11:07PM +0530, Anup Patel wrote:
> > 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.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > Reviewed-by: Xiang W <wxjstz@126.com>
> > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > ---
> >  lib/sbi/Kconfig          |  4 +++
> >  lib/sbi/objects.mk       |  3 ++
> >  lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 79 insertions(+)
> >  create mode 100644 lib/sbi/sbi_ecall_dbcn.c
> >
> > diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> > index df74bba..ef6728b 100644
> > --- a/lib/sbi/Kconfig
> > +++ b/lib/sbi/Kconfig
> > @@ -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
> > diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> > index c774ebb..319f38d 100644
> > --- a/lib/sbi/objects.mk
> > +++ b/lib/sbi/objects.mk
> > @@ -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
> >
> > diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> > new file mode 100644
> > index 0000000..bfa40e9
> > --- /dev/null
> > +++ b/lib/sbi/sbi_ecall_dbcn.c
> > @@ -0,0 +1,72 @@
> > +/*
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright (c) 2022 Ventana Micro Systems Inc.
> > + *
> > + * Authors:
> > + *   Anup Patel <apatel@ventanamicro.com>
> > + */
> > +
> > +#include <sbi/sbi_console.h>
> > +#include <sbi/sbi_domain.h>
> > +#include <sbi/sbi_error.h>
> > +#include <sbi/sbi_ecall.h>
> > +#include <sbi/sbi_ecall_interface.h>
> > +#include <sbi/sbi_trap.h>
> > +#include <sbi/riscv_asm.h>
> > +
> > +static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
> > +                               const struct sbi_trap_regs *regs,
> > +                               unsigned long *out_val,
> > +                               struct sbi_trap_info *out_trap)
> > +{
> > +     ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
> > +                     MSTATUS_MPP_SHIFT;
> > +
> > +     switch (funcid) {
> > +     case SBI_EXT_DBCN_CONSOLE_WRITE:
> > +     case SBI_EXT_DBCN_CONSOLE_READ:
> > +             /*
> > +              * On RV32, the M-mode can only access the first 4GB of
> > +              * the physical address space because M-mode does not have
> > +              * MMU to access full 34-bit physical address space.
> > +              *
> > +              * Based on above, we simply fail if the upper 32bits of
> > +              * the physical address (i.e. a2 register) is non-zero on
> > +              * RV32.
> > +              */
> > +#if __riscv_xlen == 32
> > +             if (regs->a2)
> > +                     return SBI_ERR_FAILED;
>
> The spec drafts says SBI_ERR_FAILED is only for I/O errors. I think
> we should extend the spec draft to point this situation out and return
> SBI_ERR_INVALID_ADDRESS.

This is a limitation for OpenSBI RV32 because to support a 34bit
physical address for RV32, we will need special infrastructure
using a temporary page table. Due to this reason, twe reat this
limitation as I/O error on OpenSBI RV32 side.

>
> > +#endif
> > +             if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
> > +                                     regs->a1, regs->a0, smode,
> > +                                     SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
> > +                     return SBI_EINVALID_ADDR;
>
> The spec draft says this should be SBI_ERR_INVALID_PARAM

Okay, I will update.

>
> > +             if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
> > +                     sbi_nputs((const char *)regs->a1, regs->a0);
> > +             else
> > +                     *out_val = sbi_ngets((char *)regs->a1, regs->a0);
> > +             return 0;
> > +     case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> > +             sbi_putc(regs->a0);
> > +             return 0;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return SBI_ENOTSUPP;
> > +}
> > +
> > +static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
> > +{
> > +     *out_val = (sbi_console_get_device()) ? 1 : 0;
>
> nit: Unnecessary ()

Okay, I will update.

>
> > +     return 0;
> > +}
> > +
> > +struct sbi_ecall_extension ecall_dbcn = {
> > +     .extid_start = SBI_EXT_DBCN,
> > +     .extid_end = SBI_EXT_DBCN,
> > +     .handle = sbi_ecall_dbcn_handler,
> > +     .probe = sbi_ecall_dbcn_probe,
> > +};
> > --
> > 2.34.1
> >
> >
>
> Thanks,
> drew

Regards,
Anup

[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension

[Anup Patel]

On Wed, Feb 1, 2023 at 3:06 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jan 13, 2023 at 05:11:07PM +0530, Anup Patel wrote:
> > 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.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > Reviewed-by: Xiang W <wxjstz@126.com>
> > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > ---
> >  lib/sbi/Kconfig          |  4 +++
> >  lib/sbi/objects.mk       |  3 ++
> >  lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 79 insertions(+)
> >  create mode 100644 lib/sbi/sbi_ecall_dbcn.c
> >
> > diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> > index df74bba..ef6728b 100644
> > --- a/lib/sbi/Kconfig
> > +++ b/lib/sbi/Kconfig
> > @@ -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
> > diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> > index c774ebb..319f38d 100644
> > --- a/lib/sbi/objects.mk
> > +++ b/lib/sbi/objects.mk
> > @@ -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
> >
> > diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> > new file mode 100644
> > index 0000000..bfa40e9
> > --- /dev/null
> > +++ b/lib/sbi/sbi_ecall_dbcn.c
> > @@ -0,0 +1,72 @@
> > +/*
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright (c) 2022 Ventana Micro Systems Inc.
> > + *
> > + * Authors:
> > + *   Anup Patel <apatel@ventanamicro.com>
> > + */
> > +
> > +#include <sbi/sbi_console.h>
> > +#include <sbi/sbi_domain.h>
> > +#include <sbi/sbi_error.h>
> > +#include <sbi/sbi_ecall.h>
> > +#include <sbi/sbi_ecall_interface.h>
> > +#include <sbi/sbi_trap.h>
> > +#include <sbi/riscv_asm.h>
> > +
> > +static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
> > +                               const struct sbi_trap_regs *regs,
> > +                               unsigned long *out_val,
> > +                               struct sbi_trap_info *out_trap)
> > +{
> > +     ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
> > +                     MSTATUS_MPP_SHIFT;
> > +
> > +     switch (funcid) {
> > +     case SBI_EXT_DBCN_CONSOLE_WRITE:
> > +     case SBI_EXT_DBCN_CONSOLE_READ:
> > +             /*
> > +              * On RV32, the M-mode can only access the first 4GB of
> > +              * the physical address space because M-mode does not have
> > +              * MMU to access full 34-bit physical address space.
> > +              *
> > +              * Based on above, we simply fail if the upper 32bits of
> > +              * the physical address (i.e. a2 register) is non-zero on
> > +              * RV32.
> > +              */
> > +#if __riscv_xlen == 32
> > +             if (regs->a2)
> > +                     return SBI_ERR_FAILED;
> > +#endif
> > +             if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
> > +                                     regs->a1, regs->a0, smode,
> > +                                     SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
> > +                     return SBI_EINVALID_ADDR;
> > +             if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
> > +                     sbi_nputs((const char *)regs->a1, regs->a0);
>
> The spec draft says we return the number of bytes written, so we need
> sbi_nputs to return that. If we never intend to support partial writes,
> then I guess we at least need '*out_val = regs->a0'.

I had already updated the implementation based on the latest draft so
this is already taken care in v6.

Regards,
Anup

>
> Thanks,
> drew
>
> > +             else
> > +                     *out_val = sbi_ngets((char *)regs->a1, regs->a0);
> > +             return 0;
> > +     case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> > +             sbi_putc(regs->a0);
> > +             return 0;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return SBI_ENOTSUPP;
> > +}
> > +
> > +static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
> > +{
> > +     *out_val = (sbi_console_get_device()) ? 1 : 0;
> > +     return 0;
> > +}
> > +
> > +struct sbi_ecall_extension ecall_dbcn = {
> > +     .extid_start = SBI_EXT_DBCN,
> > +     .extid_end = SBI_EXT_DBCN,
> > +     .handle = sbi_ecall_dbcn_handler,
> > +     .probe = sbi_ecall_dbcn_probe,
> > +};
> > --
> > 2.34.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension

[Jessica Clarke]

On 10 Feb 2023, at 04:23, Anup Patel <anup@brainfault.org> wrote:
> 
> On Wed, Feb 1, 2023 at 2:28 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>> 
>> On Fri, Jan 13, 2023 at 05:11:07PM +0530, Anup Patel wrote:
>>> 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.
>>> 
>>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>>> Reviewed-by: Atish Patra <atishp@rivosinc.com>
>>> Reviewed-by: Xiang W <wxjstz@126.com>
>>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>>> ---
>>> lib/sbi/Kconfig          |  4 +++
>>> lib/sbi/objects.mk       |  3 ++
>>> lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 79 insertions(+)
>>> create mode 100644 lib/sbi/sbi_ecall_dbcn.c
>>> 
>>> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
>>> index df74bba..ef6728b 100644
>>> --- a/lib/sbi/Kconfig
>>> +++ b/lib/sbi/Kconfig
>>> @@ -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
>>> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
>>> index c774ebb..319f38d 100644
>>> --- a/lib/sbi/objects.mk
>>> +++ b/lib/sbi/objects.mk
>>> @@ -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
>>> 
>>> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
>>> new file mode 100644
>>> index 0000000..bfa40e9
>>> --- /dev/null
>>> +++ b/lib/sbi/sbi_ecall_dbcn.c
>>> @@ -0,0 +1,72 @@
>>> +/*
>>> + * SPDX-License-Identifier: BSD-2-Clause
>>> + *
>>> + * Copyright (c) 2022 Ventana Micro Systems Inc.
>>> + *
>>> + * Authors:
>>> + *   Anup Patel <apatel@ventanamicro.com>
>>> + */
>>> +
>>> +#include <sbi/sbi_console.h>
>>> +#include <sbi/sbi_domain.h>
>>> +#include <sbi/sbi_error.h>
>>> +#include <sbi/sbi_ecall.h>
>>> +#include <sbi/sbi_ecall_interface.h>
>>> +#include <sbi/sbi_trap.h>
>>> +#include <sbi/riscv_asm.h>
>>> +
>>> +static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
>>> +                               const struct sbi_trap_regs *regs,
>>> +                               unsigned long *out_val,
>>> +                               struct sbi_trap_info *out_trap)
>>> +{
>>> +     ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
>>> +                     MSTATUS_MPP_SHIFT;
>>> +
>>> +     switch (funcid) {
>>> +     case SBI_EXT_DBCN_CONSOLE_WRITE:
>>> +     case SBI_EXT_DBCN_CONSOLE_READ:
>>> +             /*
>>> +              * On RV32, the M-mode can only access the first 4GB of
>>> +              * the physical address space because M-mode does not have
>>> +              * MMU to access full 34-bit physical address space.
>>> +              *
>>> +              * Based on above, we simply fail if the upper 32bits of
>>> +              * the physical address (i.e. a2 register) is non-zero on
>>> +              * RV32.
>>> +              */
>>> +#if __riscv_xlen == 32
>>> +             if (regs->a2)
>>> +                     return SBI_ERR_FAILED;
>> 
>> The spec drafts says SBI_ERR_FAILED is only for I/O errors. I think
>> we should extend the spec draft to point this situation out and return
>> SBI_ERR_INVALID_ADDRESS.
> 
> This is a limitation for OpenSBI RV32 because to support a 34bit
> physical address for RV32, we will need special infrastructure
> using a temporary page table. Due to this reason, twe reat this
> limitation as I/O error on OpenSBI RV32 side.

Were the interface to take a virtual address (or, really, pointer) this
would not be an issue as the supervisor would already have handled the
mapping for you. Which is what I?ve kept advocating for.

Jess

>> 
>>> +#endif
>>> +             if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
>>> +                                     regs->a1, regs->a0, smode,
>>> +                                     SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
>>> +                     return SBI_EINVALID_ADDR;
>> 
>> The spec draft says this should be SBI_ERR_INVALID_PARAM
> 
> Okay, I will update.
> 
>> 
>>> +             if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
>>> +                     sbi_nputs((const char *)regs->a1, regs->a0);
>>> +             else
>>> +                     *out_val = sbi_ngets((char *)regs->a1, regs->a0);
>>> +             return 0;
>>> +     case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
>>> +             sbi_putc(regs->a0);
>>> +             return 0;
>>> +     default:
>>> +             break;
>>> +     }
>>> +
>>> +     return SBI_ENOTSUPP;
>>> +}
>>> +
>>> +static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
>>> +{
>>> +     *out_val = (sbi_console_get_device()) ? 1 : 0;
>> 
>> nit: Unnecessary ()
> 
> Okay, I will update.
> 
>> 
>>> +     return 0;
>>> +}
>>> +
>>> +struct sbi_ecall_extension ecall_dbcn = {
>>> +     .extid_start = SBI_EXT_DBCN,
>>> +     .extid_end = SBI_EXT_DBCN,
>>> +     .handle = sbi_ecall_dbcn_handler,
>>> +     .probe = sbi_ecall_dbcn_probe,
>>> +};
>>> --
>>> 2.34.1
>>> 
>>> 
>> 
>> Thanks,
>> drew
> 
> Regards,
> Anup
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension

[Anup Patel]

On Fri, Feb 10, 2023 at 10:50 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 10 Feb 2023, at 04:23, Anup Patel <anup@brainfault.org> wrote:
> >
> > On Wed, Feb 1, 2023 at 2:28 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >>
> >> On Fri, Jan 13, 2023 at 05:11:07PM +0530, Anup Patel wrote:
> >>> 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.
> >>>
> >>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >>> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> >>> Reviewed-by: Xiang W <wxjstz@126.com>
> >>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> >>> ---
> >>> lib/sbi/Kconfig          |  4 +++
> >>> lib/sbi/objects.mk       |  3 ++
> >>> lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 79 insertions(+)
> >>> create mode 100644 lib/sbi/sbi_ecall_dbcn.c
> >>>
> >>> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> >>> index df74bba..ef6728b 100644
> >>> --- a/lib/sbi/Kconfig
> >>> +++ b/lib/sbi/Kconfig
> >>> @@ -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
> >>> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> >>> index c774ebb..319f38d 100644
> >>> --- a/lib/sbi/objects.mk
> >>> +++ b/lib/sbi/objects.mk
> >>> @@ -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
> >>>
> >>> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> >>> new file mode 100644
> >>> index 0000000..bfa40e9
> >>> --- /dev/null
> >>> +++ b/lib/sbi/sbi_ecall_dbcn.c
> >>> @@ -0,0 +1,72 @@
> >>> +/*
> >>> + * SPDX-License-Identifier: BSD-2-Clause
> >>> + *
> >>> + * Copyright (c) 2022 Ventana Micro Systems Inc.
> >>> + *
> >>> + * Authors:
> >>> + *   Anup Patel <apatel@ventanamicro.com>
> >>> + */
> >>> +
> >>> +#include <sbi/sbi_console.h>
> >>> +#include <sbi/sbi_domain.h>
> >>> +#include <sbi/sbi_error.h>
> >>> +#include <sbi/sbi_ecall.h>
> >>> +#include <sbi/sbi_ecall_interface.h>
> >>> +#include <sbi/sbi_trap.h>
> >>> +#include <sbi/riscv_asm.h>
> >>> +
> >>> +static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
> >>> +                               const struct sbi_trap_regs *regs,
> >>> +                               unsigned long *out_val,
> >>> +                               struct sbi_trap_info *out_trap)
> >>> +{
> >>> +     ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
> >>> +                     MSTATUS_MPP_SHIFT;
> >>> +
> >>> +     switch (funcid) {
> >>> +     case SBI_EXT_DBCN_CONSOLE_WRITE:
> >>> +     case SBI_EXT_DBCN_CONSOLE_READ:
> >>> +             /*
> >>> +              * On RV32, the M-mode can only access the first 4GB of
> >>> +              * the physical address space because M-mode does not have
> >>> +              * MMU to access full 34-bit physical address space.
> >>> +              *
> >>> +              * Based on above, we simply fail if the upper 32bits of
> >>> +              * the physical address (i.e. a2 register) is non-zero on
> >>> +              * RV32.
> >>> +              */
> >>> +#if __riscv_xlen == 32
> >>> +             if (regs->a2)
> >>> +                     return SBI_ERR_FAILED;
> >>
> >> The spec drafts says SBI_ERR_FAILED is only for I/O errors. I think
> >> we should extend the spec draft to point this situation out and return
> >> SBI_ERR_INVALID_ADDRESS.
> >
> > This is a limitation for OpenSBI RV32 because to support a 34bit
> > physical address for RV32, we will need special infrastructure
> > using a temporary page table. Due to this reason, twe reat this
> > limitation as I/O error on OpenSBI RV32 side.
>
> Were the interface to take a virtual address (or, really, pointer) this
> would not be an issue as the supervisor would already have handled the
> mapping for you. Which is what I?ve kept advocating for.

There are two major issues in passing virtual address:
1) The virtual address could point to anything so SBI implementation
    (Both M-mode firmware and hypervisors) have to translate the
    virtual address into physical address and then check sanity of
    the virtual address. This will involve doing software page table walks
    which are very expensive.
2) SBI implementation will have to use unpriv load/stores for accessing
   data pointed by virtual address which can potentially trap if the page
   table mapping is changed by OS while the SBI implementation was
   accessing it.

Regards,
Anup

>
> Jess
>
> >>
> >>> +#endif
> >>> +             if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
> >>> +                                     regs->a1, regs->a0, smode,
> >>> +                                     SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
> >>> +                     return SBI_EINVALID_ADDR;
> >>
> >> The spec draft says this should be SBI_ERR_INVALID_PARAM
> >
> > Okay, I will update.
> >
> >>
> >>> +             if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
> >>> +                     sbi_nputs((const char *)regs->a1, regs->a0);
> >>> +             else
> >>> +                     *out_val = sbi_ngets((char *)regs->a1, regs->a0);
> >>> +             return 0;
> >>> +     case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> >>> +             sbi_putc(regs->a0);
> >>> +             return 0;
> >>> +     default:
> >>> +             break;
> >>> +     }
> >>> +
> >>> +     return SBI_ENOTSUPP;
> >>> +}
> >>> +
> >>> +static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
> >>> +{
> >>> +     *out_val = (sbi_console_get_device()) ? 1 : 0;
> >>
> >> nit: Unnecessary ()
> >
> > Okay, I will update.
> >
> >>
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +struct sbi_ecall_extension ecall_dbcn = {
> >>> +     .extid_start = SBI_EXT_DBCN,
> >>> +     .extid_end = SBI_EXT_DBCN,
> >>> +     .handle = sbi_ecall_dbcn_handler,
> >>> +     .probe = sbi_ecall_dbcn_probe,
> >>> +};
> >>> --
> >>> 2.34.1
> >>>
> >>>
> >>
> >> Thanks,
> >> drew
> >
> > Regards,
> > Anup
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>

[PATCH v5 8/8] lib: sbi: Speed-up sbi_printf() and friends using nputs()

[Anup Patel]

On Wed, Feb 1, 2023 at 2:52 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jan 13, 2023 at 05:11:10PM +0530, Anup Patel wrote:
> > The sbi_printf() is slow for semihosting because it prints one
> > character at a time. To speed-up sbi_printf() for semihosting,
> > we use a temporary buffer and nputs().
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  lib/sbi/sbi_console.c | 52 ++++++++++++++++++++++++++++---------------
> >  1 file changed, 34 insertions(+), 18 deletions(-)
> >
> > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> > index c1b9f73..41881d7 100644
> > --- a/lib/sbi/sbi_console.c
> > +++ b/lib/sbi/sbi_console.c
> > @@ -42,31 +42,29 @@ void sbi_putc(char ch)
> >       }
> >  }
> >
> > -void sbi_puts(const char *str)
> > +static void nputs(const char *str, unsigned long len)
> >  {
> > -     spin_lock(&console_out_lock);
> > +     unsigned long i;
> > +
> >       if (console_dev && console_dev->console_puts) {
> > -             console_dev->console_puts(str, sbi_strlen(str));
> > +             console_dev->console_puts(str, len);
> >       } else {
> > -             while (*str) {
> > -                     sbi_putc(*str);
> > -                     str++;
> > -             }
> > +             for (i = 0; i < len; i++)
> > +                     sbi_putc(str[i]);
> >       }
> > +}
> > +
> > +void sbi_puts(const char *str)
> > +{
> > +     spin_lock(&console_out_lock);
> > +     nputs(str, sbi_strlen(str));
> >       spin_unlock(&console_out_lock);
> >  }
> >
> >  void sbi_nputs(const char *str, unsigned long len)
> >  {
> > -     unsigned long i;
> > -
> >       spin_lock(&console_out_lock);
> > -     if (console_dev && console_dev->console_puts) {
> > -             console_dev->console_puts(str, len);
> > -     } else {
> > -             for (i = 0; i < len; i++)
> > -                     sbi_putc(str[i]);
> > -     }
> > +     nputs(str, len);
> >       spin_unlock(&console_out_lock);
> >  }
> >
> > @@ -102,6 +100,7 @@ unsigned long sbi_ngets(char *str, unsigned long len)
> >  #define PAD_ZERO 2
> >  #define PAD_ALTERNATE 4
> >  #define PRINT_BUF_LEN 64
> > +#define PRINT_TBUF_MAX 128
> >
> >  #define va_start(v, l) __builtin_va_start((v), l)
> >  #define va_end __builtin_va_end
> > @@ -217,12 +216,26 @@ static int printi(char **out, u32 *out_len, long long i, int b, int sg,
> >
> >  static int print(char **out, u32 *out_len, const char *format, va_list args)
> >  {
> > -     int width, flags;
> > -     int pc = 0;
> > -     char scr[2];
> > +     u32 tbuf_len;
> > +     int width, flags, pc = 0;
> > +     char scr[2], *tout, tbuf[PRINT_TBUF_MAX];
> > +     bool use_tbuf = (!out) ? true : false;
>
> print() is only called with a non-null 'out' parameter or under
> console_out_lock, so we can use global variable for the buffer
> to avoid putting it on the stack, and it can be larger.

Okay, I will update.

>
> >       unsigned long long tmp;
> >
> > +     if (use_tbuf) {
> > +             tbuf_len = PRINT_TBUF_MAX;
> > +             tout = tbuf;
> > +             out = &tout;
> > +             out_len = &tbuf_len;
> > +     }
> > +
> >       for (; *format != 0; ++format) {
> > +             if (use_tbuf && !tbuf_len) {
> > +                     nputs(tbuf, PRINT_TBUF_MAX);
> > +                     tbuf_len = PRINT_TBUF_MAX;
> > +                     tout = tbuf;
> > +             }
> > +
> >               if (*format == '%') {
> >                       ++format;
> >                       width = flags = 0;
> > @@ -348,6 +361,9 @@ literal:
> >               }
> >       }
> >
> > +     if (use_tbuf && tbuf_len < PRINT_TBUF_MAX)
> > +             nputs(tbuf, PRINT_TBUF_MAX - tbuf_len);
> > +
> >       return pc;
> >  }
> >
> > --
> > 2.34.1
> >
>
> Otherwise,
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> Thanks,
> drew

Regards,
Anup