Skip to contentSkip to conversation

OpenSBI debug console support

PATCH Superseded Anup Patel 5 participants 29 messages Jan 13, 2023 – Feb 10, 2023

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Anup Patel

author
#0 lore raw

v5 cover OpenSBI debug console support

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
Signature · 1 line
2.34.1
author
#1 lore raw

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

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

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Bin Meng <[email protected]>
Reviewed-by Xiang W <[email protected]>
---
 include/sbi/sbi_ecall_interface.h | 6 ++++++
 1 file changed, 6 insertions(+)
include/sbi/sbi_ecall_interface.h +6 -0
index a3f2bf4..9d6f474 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,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
Signature · 1 line
2.34.1
#2 lore raw

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

Quoted 1 line · Anup Patel

We add SBI debug console extension related defines/enum to the

Not adding any enums.

Quoted 6 lines

SBI ecall interface header.

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Bin Meng <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Quoted 36 lines
---
 include/sbi/sbi_ecall_interface.h | 6 ++++++
 1 file changed, 6 insertions(+)
include/sbi/sbi_ecall_interface.h +6 -0
index a3f2bf4..9d6f474 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,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
Signature · 7 lines
2.34.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
#3 lore raw

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

Quoted 5 lines · Andrew Jones
Quoted 1 line · Anup Patel

We add SBI debug console extension related defines/enum to the

Not adding any enums.

Okay, I will update.

Quoted 42 lines
Quoted 7 lines

SBI ecall interface header.

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Bin Meng <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Andrew Jones <[email protected]>
Quoted 32 lines
---
 include/sbi/sbi_ecall_interface.h | 6 ++++++
 1 file changed, 6 insertions(+)
include/sbi/sbi_ecall_interface.h +6 -1
index a3f2bf4..9d6f474 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,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

Quoted 4 lines
Quoted 4 lines

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

Regards,
Anup

author
#4 lore raw

PATCH v5 2/8 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.

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Bin Meng <[email protected]>
Reviewed-by Xiang W <[email protected]>
---
 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 bb6e1ef..946c61d 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
#5 lore raw

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

Quoted 30 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Bin Meng <[email protected]>
Reviewed-by Xiang W <[email protected]>
---
 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 +0 -0
index bb6e1ef..946c61d 100644

lib/sbi/sbi_console.c void sbi_puts(const char *str) · line 60

Quoted 10 lines
lib/sbi/sbi_console.c +7 -0
@@ -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

Quoted 14 lines
lib/sbi/sbi_console.c +3 -0
+	spin_unlock(&console_out_lock);
+}
+
 void sbi_gets(char *s, int maxwidth, char endchar)
 {
 	int ch;
Signature · 7 lines
2.34.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
#6 lore raw

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

Quoted 35 lines · Andrew Jones
Quoted 33 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Bin Meng <[email protected]>
Reviewed-by Xiang W <[email protected]>
---
 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 +0 -0
index bb6e1ef..946c61d 100644
@@ -51,6 +51,16 @@ void sbi_puts(const char *str)
      spin_unlock(&console_out_lock);
 }
Quoted 11 lines

lib/sbi/sbi_console.c void sbi_puts(const char *str) · line 60

Quoted 7 lines
lib/sbi/sbi_console.c +7 -0
+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

#7 lore raw

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

Quoted 49 lines · Anup Patel
Quoted 35 lines · Andrew Jones
Quoted 33 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Bin Meng <[email protected]>
Reviewed-by Xiang W <[email protected]>

---
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 +0 -0
index bb6e1ef..946c61d 100644
@@ -51,6 +51,16 @@ void sbi_puts(const char *str)
     spin_unlock(&console_out_lock);
 }
Quoted 11 lines

lib/sbi/sbi_console.c void sbi_puts(const char *str) · line 60

Quoted 7 lines
lib/sbi/sbi_console.c +7 -0
+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?

Quoted 6 lines

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

author
#8 lore raw

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

In reply to Anup Patel

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

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
---
 include/sbi/sbi_console.h |  2 ++
 lib/sbi/sbi_console.c     | 15 +++++++++++++++
 2 files changed, 17 insertions(+)
include/sbi/sbi_console.h +2 -0
index 1bdeeb9..660c239 100644
@@ -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, ...);
lib/sbi/sbi_console.c +15 -0
index 946c61d..89d6a49 100644
@@ -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
Signature · 1 line
2.34.1
author
#10 lore raw

PATCH v5 4/8 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.

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 include/sbi/sbi_domain.h | 15 +++++++++
 lib/sbi/sbi_domain.c     | 69 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)
include/sbi/sbi_domain.h +15 -0
index bbb3eff..ab1a944 100644
@@ -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);
lib/sbi/sbi_domain.c +69 -0

81 lines not on this page · raw message

Signature · 1 line
2.34.1
#11 lore raw

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

Quoted 113 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 include/sbi/sbi_domain.h | 15 +++++++++
 lib/sbi/sbi_domain.c     | 69 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)
include/sbi/sbi_domain.h +15 -0
index bbb3eff..ab1a944 100644
@@ -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);
lib/sbi/sbi_domain.c +59 -0
index 60fda01..683b6cf 100644
@@ -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)

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

Quoted 10 lines
lib/sbi/sbi_domain.c +10 -0
+			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 <[email protected]>

Thanks,
drew

author
#12 lore raw

PATCH v5 5/8 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.

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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
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 +72 -0

72 lines not on this page · raw message

Signature · 1 line
2.34.1
#13 lore raw

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

Quoted 80 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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
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 +30 -0
new file mode 100644
index 0000000..bfa40e9
@@ -0,0 +1,72 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2022 Ventana Micro Systems Inc.
+ *
+ * Authors:
+ *   Anup Patel <[email protected]>
+ */
+
+#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

lib/sbi/sbi_ecall_dbcn.c line 40

Quoted 10 lines
lib/sbi/sbi_ecall_dbcn.c new file +10 -0
+		 * 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.

lib/sbi/sbi_ecall_dbcn.c line 45

Quoted 5 lines
lib/sbi/sbi_ecall_dbcn.c new file +5 -0
+#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

Quoted 8 lines
lib/sbi/sbi_ecall_dbcn.c new file +8 -0
+		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;

lib/sbi/sbi_ecall_dbcn.c line 63

Quoted 10 lines
lib/sbi/sbi_ecall_dbcn.c new file +10 -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 ()

Quoted 13 lines
lib/sbi/sbi_ecall_dbcn.c new file +9 -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,
+};
Signature · 1 line
2.34.1

Thanks,
drew

#14 lore raw

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

Quoted 88 lines · Andrew Jones
Quoted 86 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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
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 +36 -0
new file mode 100644
index 0000000..bfa40e9
@@ -0,0 +1,72 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2022 Ventana Micro Systems Inc.
+ *
+ * Authors:
+ *   Anup Patel <[email protected]>
+ */
+
+#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.
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 40

Quoted 4 lines
lib/sbi/sbi_ecall_dbcn.c new file +4 -0
+              */
+#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.

Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 45

Quoted 6 lines
lib/sbi/sbi_ecall_dbcn.c new file +5 -0
+#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.

Quoted 13 lines
Quoted 13 lines
lib/sbi/sbi_ecall_dbcn.c new file +12 -0
+             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;
+     }
+
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 63

Quoted 6 lines
lib/sbi/sbi_ecall_dbcn.c new file +6 -0
+     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.

Quoted 17 lines
Quoted 11 lines
lib/sbi/sbi_ecall_dbcn.c new file +9 -1
+     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,
+};
--
Quoted 3 lines

2.34.1

Thanks,
drew

Regards,
Anup

#15 lore raw

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

Quoted 99 lines · Anup Patel
Quoted 88 lines · Andrew Jones
Quoted 86 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>

---

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

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 +0 -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

lib/sbi/objects.mk +3 -0
+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 +36 -0
new file mode 100644
index 0000000..bfa40e9
@@ -0,0 +1,72 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2022 Ventana Micro Systems Inc.
+ *
+ * Authors:
+ *   Anup Patel <[email protected]>
+ */
+
+#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.
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 40

Quoted 4 lines
lib/sbi/sbi_ecall_dbcn.c new file +4 -0
+              */
+#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.

Quoted 4 lines

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

Quoted 60 lines
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 45

Quoted 6 lines
lib/sbi/sbi_ecall_dbcn.c new file +5 -0
+#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.

Quoted 13 lines
Quoted 13 lines
lib/sbi/sbi_ecall_dbcn.c new file +12 -0
+             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;
+     }
+
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 63

Quoted 6 lines
lib/sbi/sbi_ecall_dbcn.c new file +6 -0
+     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.

Quoted 17 lines
Quoted 11 lines
lib/sbi/sbi_ecall_dbcn.c new file +9 -1
+     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,
+};
--
Quoted 3 lines

2.34.1

Thanks,
drew

Regards,
Anup

Signature · 3 lines
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
author
#16 lore raw

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

Quoted 101 lines · Jessica Clarke
Quoted 99 lines · Anup Patel
Quoted 96 lines · Andrew Jones
Quoted 80 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>

---

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

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 +0 -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

lib/sbi/objects.mk +3 -0
+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 +30 -0
new file mode 100644
index 0000000..bfa40e9
@@ -0,0 +1,72 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2022 Ventana Micro Systems Inc.
+ *
+ * Authors:
+ *   Anup Patel <[email protected]>
+ */
+
+#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

lib/sbi/sbi_ecall_dbcn.c line 40

Quoted 10 lines
lib/sbi/sbi_ecall_dbcn.c new file +10 -0
+              * 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.

Quoted 8 lines
Quoted 4 lines

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

Quoted 64 lines

Jess

Quoted 60 lines
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 45

Quoted 6 lines
lib/sbi/sbi_ecall_dbcn.c new file +5 -0
+#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.

Quoted 13 lines
Quoted 13 lines
lib/sbi/sbi_ecall_dbcn.c new file +12 -0
+             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;
+     }
+
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 63

Quoted 6 lines
lib/sbi/sbi_ecall_dbcn.c new file +6 -0
+     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.

Quoted 17 lines
Quoted 11 lines
lib/sbi/sbi_ecall_dbcn.c new file +9 -1
+     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,
+};
--
Quoted 3 lines

2.34.1

Thanks,
drew

Regards,
Anup

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

#17 lore raw

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

In reply to Anup Patel
Quoted 87 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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
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 +37 -0
new file mode 100644
index 0000000..bfa40e9
@@ -0,0 +1,72 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2022 Ventana Micro Systems Inc.
+ *
+ * Authors:
+ *   Anup Patel <[email protected]>
+ */
+
+#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.
+		 */

lib/sbi/sbi_ecall_dbcn.c line 47

Quoted 10 lines
lib/sbi/sbi_ecall_dbcn.c new file +10 -0
+#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

Quoted 33 lines
lib/sbi/sbi_ecall_dbcn.c new file +25 -0
+		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,
+};
Signature · 7 lines
2.34.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
#18 lore raw

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

Quoted 95 lines · Andrew Jones
Quoted 93 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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
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 +43 -0
new file mode 100644
index 0000000..bfa40e9
@@ -0,0 +1,72 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2022 Ventana Micro Systems Inc.
+ *
+ * Authors:
+ *   Anup Patel <[email protected]>
+ */
+
+#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,
Quoted 8 lines

lib/sbi/sbi_ecall_dbcn.c line 47

Quoted 4 lines
lib/sbi/sbi_ecall_dbcn.c new file +4 -0
+                                     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

Quoted 37 lines

Thanks,
drew

Quoted 33 lines
lib/sbi/sbi_ecall_dbcn.c new file +25 -1
+             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

author
#19 lore raw

PATCH v5 6/8 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.

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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 660c239..8e95b9d 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 89d6a49..c1b9f73 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
#20 lore raw

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

Quoted 74 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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 660c239..8e95b9d 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 89d6a49..c1b9f73 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
Reviewed-by Andrew Jones <[email protected]>
#21 lore raw

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

? 2023-02-01???? 10:00 +0100?Andrew Jones???

Quoted 14 lines · Anup Patel
Quoted 13 lines

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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>

---
?include/sbi/sbi_console.h |? 3 +++ ?lib/sbi/sbi_console.c???? | 19 ++++++++++++++----- ?2 files changed, 17 insertions(+), 5 deletions(-)

Quoted 10 lines
Quoted 10 lines
include/sbi/sbi_console.h +0 -0
index 660c239..8e95b9d 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);

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

Quoted 53 lines
Quoted 40 lines

+
????????/** Read a character from the console input */ ????????int (*console_getc)(void);
?};

lib/sbi/sbi_console.c +0 -0
index 89d6a49..c1b9f73 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;

lib/sbi/sbi_console.c +7 -3
@@ -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) {

Quoted 11 lines

+???????????????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
Reviewed-by Andrew Jones <[email protected]>
#22 lore raw

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

In reply to Anup Patel
Quoted 18 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 include/sbi/sbi_console.h |  3 +++
 lib/sbi/sbi_console.c     | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)
include/sbi/sbi_console.h +0 -0
index 660c239..8e95b9d 100644

include/sbi/sbi_console.h struct sbi_console_device { · line 23

Quoted 5 lines
include/sbi/sbi_console.h +2 -0
@@ -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

Quoted 56 lines
include/sbi/sbi_console.h +1 -0
+
 	/** Read a character from the console input */
 	int (*console_getc)(void);
 };
lib/sbi/sbi_console.c +14 -5
index 89d6a49..c1b9f73 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 · 7 lines
2.34.1


-- 
opensbi mailing list
opensbi at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
#23 lore raw

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

Quoted 22 lines · Andrew Jones
Quoted 21 lines · 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 <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 include/sbi/sbi_console.h |  3 +++
 lib/sbi/sbi_console.c     | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)
include/sbi/sbi_console.h +0 -0
index 660c239..8e95b9d 100644
@@ -19,6 +19,9 @@ struct sbi_console_device {
 	/** Write a character to the console output */
 	void (*console_putc)(char ch);
Quoted 10 lines

include/sbi/sbi_console.h struct sbi_console_device { · line 23

Quoted 2 lines
include/sbi/sbi_console.h +2 -0
+	/** 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

author
#24 lore raw

PATCH v5 7/8 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.

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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
#25 lore raw

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

Quoted 92 lines · Anup Patel

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

Signed-off-by Anup Patel <[email protected]>
Reviewed-by Atish Patra <[email protected]>
Reviewed-by Xiang W <[email protected]>
Reviewed-by Bin Meng <[email protected]>
---
 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
Reviewed-by Andrew Jones <[email protected]>

Thanks,
drew

author
#26 lore raw

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

In reply to 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 <[email protected]>
---
 lib/sbi/sbi_console.c | 52 ++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 18 deletions(-)
lib/sbi/sbi_console.c +34 -18
index c1b9f73..41881d7 100644
@@ -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;
 }
Signature · 1 line
2.34.1
#27 lore raw

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

Quoted 67 lines · 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 <[email protected]>
---
 lib/sbi/sbi_console.c | 52 ++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 18 deletions(-)
lib/sbi/sbi_console.c +14 -15
index c1b9f73..41881d7 100644
@@ -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

lib/sbi/sbi_console.c static int printi(char **out, u32 *out_len, long long i, int b, int sg, · line 222

Quoted 10 lines
lib/sbi/sbi_console.c +4 -3
@@ -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.

Quoted 32 lines
lib/sbi/sbi_console.c +16 -0
 	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;
 }
Signature · 1 line
2.34.1

Otherwise,

Reviewed-by Andrew Jones <[email protected]>

Thanks,
drew

#28 lore raw

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

Quoted 70 lines · Andrew Jones
Quoted 68 lines · 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 <[email protected]>
---
 lib/sbi/sbi_console.c | 52 ++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 18 deletions(-)
lib/sbi/sbi_console.c +14 -15
index c1b9f73..41881d7 100644
@@ -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,
Quoted 13 lines

lib/sbi/sbi_console.c static int printi(char **out, u32 *out_len, long long i, int b, int sg, · line 222

Quoted 9 lines
lib/sbi/sbi_console.c +4 -3
 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.

Quoted 36 lines
Quoted 31 lines
lib/sbi/sbi_console.c +16 -1
      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;
 }
 
--
Quoted 2 lines

2.34.1

Otherwise,

Quoted 4 lines
Reviewed-by Andrew Jones <[email protected]>

Thanks,
drew

Regards,
Anup