[PATCH 0/6] RISC-V SBI debug console extension support

[PATCH 0/6] RISC-V SBI debug console extension support

[Anup Patel]

This series adds support for SBI debug console extension in KVM RISC-V
and Linux RISC-V.

To try these patches with KVM RISC-V, use KVMTOOL from riscv_sbi_dbcn_v1
branch at: https://github.com/avpatel/kvmtool.git

These patches can also be found in the riscv_sbi_dbcn_v1 branch at:
https://github.com/avpatel/linux.git

Anup Patel (5):
  RISC-V: Add defines for SBI debug console extension
  RISC-V: KVM: Change the SBI specification version to v2.0
  RISC-V: KVM: Forward SBI DBCN extension to user-space
  tty/serial: Add RISC-V SBI debug console based earlycon
  RISC-V: Enable SBI based earlycon support

Atish Patra (1):
  tty: Add SBI debug console support to HVC SBI driver

 arch/riscv/configs/defconfig            |  1 +
 arch/riscv/configs/rv32_defconfig       |  1 +
 arch/riscv/include/asm/kvm_vcpu_sbi.h   |  3 +-
 arch/riscv/include/asm/sbi.h            |  7 +++
 arch/riscv/include/uapi/asm/kvm.h       |  1 +
 arch/riscv/kvm/vcpu_sbi.c               |  4 ++
 arch/riscv/kvm/vcpu_sbi_replace.c       | 31 ++++++++++
 drivers/tty/hvc/Kconfig                 |  2 +-
 drivers/tty/hvc/hvc_riscv_sbi.c         | 80 ++++++++++++++++++++++---
 drivers/tty/serial/Kconfig              |  2 +-
 drivers/tty/serial/earlycon-riscv-sbi.c | 35 +++++++++--
 11 files changed, 153 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH 1/6] RISC-V: Add defines for SBI debug console extension

[Anup Patel]

We add SBI debug console extension related defines/enum to the
asm/sbi.h header.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arch/riscv/include/asm/sbi.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 5b4a1bf5f439..12dfda6bb924 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -30,6 +30,7 @@ enum sbi_ext_id {
 	SBI_EXT_HSM = 0x48534D,
 	SBI_EXT_SRST = 0x53525354,
 	SBI_EXT_PMU = 0x504D55,
+	SBI_EXT_DBCN = 0x4442434E,
 
 	/* Experimentals extensions must lie within this range */
 	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
@@ -236,6 +237,12 @@ enum sbi_pmu_ctr_type {
 /* Flags defined for counter stop function */
 #define SBI_PMU_STOP_FLAG_RESET (1 << 0)
 
+enum sbi_ext_dbcn_fid {
+	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
+	SBI_EXT_DBCN_CONSOLE_READ = 1,
+	SBI_EXT_DBCN_CONSOLE_WRITE_BYTE = 2,
+};
+
 #define SBI_SPEC_VERSION_DEFAULT	0x1
 #define SBI_SPEC_VERSION_MAJOR_SHIFT	24
 #define SBI_SPEC_VERSION_MAJOR_MASK	0x7f
-- 
2.34.1

[PATCH 2/6] RISC-V: KVM: Change the SBI specification version to v2.0

[Anup Patel]

We will be implementing SBI DBCN extension for KVM RISC-V so let
us change the KVM RISC-V SBI specification version to v2.0.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index cdcf0ff07be7..8d6d4dce8a5e 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -11,7 +11,7 @@
 
 #define KVM_SBI_IMPID 3
 
-#define KVM_SBI_VERSION_MAJOR 1
+#define KVM_SBI_VERSION_MAJOR 2
 #define KVM_SBI_VERSION_MINOR 0
 
 enum kvm_riscv_sbi_ext_status {
-- 
2.34.1

[PATCH 3/6] RISC-V: KVM: Forward SBI DBCN extension to user-space

[Anup Patel]

The SBI DBCN extension needs to be emulated in user-space so let
us forward console_puts() call to user-space.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_vcpu_sbi.h |  1 +
 arch/riscv/include/uapi/asm/kvm.h     |  1 +
 arch/riscv/kvm/vcpu_sbi.c             |  4 ++++
 arch/riscv/kvm/vcpu_sbi_replace.c     | 31 +++++++++++++++++++++++++++
 4 files changed, 37 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index 8d6d4dce8a5e..a85f95eb6e85 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -69,6 +69,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_ipi;
 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence;
 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
+extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
 extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
 
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index 917d8cc2489e..60d3b21dead7 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -156,6 +156,7 @@ enum KVM_RISCV_SBI_EXT_ID {
 	KVM_RISCV_SBI_EXT_PMU,
 	KVM_RISCV_SBI_EXT_EXPERIMENTAL,
 	KVM_RISCV_SBI_EXT_VENDOR,
+	KVM_RISCV_SBI_EXT_DBCN,
 	KVM_RISCV_SBI_EXT_MAX,
 };
 
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index 9cd97091c723..b54fe52c915a 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -66,6 +66,10 @@ static const struct kvm_riscv_sbi_extension_entry sbi_ext[] = {
 		.ext_idx = KVM_RISCV_SBI_EXT_PMU,
 		.ext_ptr = &vcpu_sbi_ext_pmu,
 	},
+	{
+		.ext_idx = KVM_RISCV_SBI_EXT_DBCN,
+		.ext_ptr = &vcpu_sbi_ext_dbcn,
+	},
 	{
 		.ext_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
 		.ext_ptr = &vcpu_sbi_ext_experimental,
diff --git a/arch/riscv/kvm/vcpu_sbi_replace.c b/arch/riscv/kvm/vcpu_sbi_replace.c
index 7c4d5d38a339..347c5856347e 100644
--- a/arch/riscv/kvm/vcpu_sbi_replace.c
+++ b/arch/riscv/kvm/vcpu_sbi_replace.c
@@ -175,3 +175,34 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst = {
 	.extid_end = SBI_EXT_SRST,
 	.handler = kvm_sbi_ext_srst_handler,
 };
+
+static int kvm_sbi_ext_dbcn_handler(struct kvm_vcpu *vcpu,
+				    struct kvm_run *run,
+				    struct kvm_vcpu_sbi_return *retdata)
+{
+	struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
+	unsigned long funcid = cp->a6;
+
+	switch (funcid) {
+	case SBI_EXT_DBCN_CONSOLE_WRITE:
+	case SBI_EXT_DBCN_CONSOLE_READ:
+	case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
+		/*
+		 * The SBI debug console functions are unconditionally
+		 * forwarded to the userspace.
+		 */
+		kvm_riscv_vcpu_sbi_forward(vcpu, run);
+		retdata->uexit = true;
+		break;
+	default:
+		retdata->err_val = SBI_ERR_NOT_SUPPORTED;
+	}
+
+	return 0;
+}
+
+const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn = {
+	.extid_start = SBI_EXT_DBCN,
+	.extid_end = SBI_EXT_DBCN,
+	.handler = kvm_sbi_ext_dbcn_handler,
+};
-- 
2.34.1

[PATCH 4/6] tty/serial: Add RISC-V SBI debug console based earlycon

[Anup Patel]

We extend the existing RISC-V SBI earlycon support to use the new
RISC-V SBI debug console extension.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 drivers/tty/serial/Kconfig              |  2 +-
 drivers/tty/serial/earlycon-riscv-sbi.c | 35 ++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index bdc568a4ab66..cec46091a716 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -87,7 +87,7 @@ config SERIAL_EARLYCON_SEMIHOST
 
 config SERIAL_EARLYCON_RISCV_SBI
 	bool "Early console using RISC-V SBI"
-	depends on RISCV_SBI_V01
+	depends on RISCV_SBI
 	select SERIAL_CORE
 	select SERIAL_CORE_CONSOLE
 	select SERIAL_EARLYCON
diff --git a/drivers/tty/serial/earlycon-riscv-sbi.c b/drivers/tty/serial/earlycon-riscv-sbi.c
index 27afb0b74ea7..b1da34e8d8cd 100644
--- a/drivers/tty/serial/earlycon-riscv-sbi.c
+++ b/drivers/tty/serial/earlycon-riscv-sbi.c
@@ -10,22 +10,49 @@
 #include <linux/serial_core.h>
 #include <asm/sbi.h>
 
+#ifdef CONFIG_RISCV_SBI_V01
 static void sbi_putc(struct uart_port *port, unsigned char c)
 {
 	sbi_console_putchar(c);
 }
 
-static void sbi_console_write(struct console *con,
-			      const char *s, unsigned n)
+static void sbi_0_1_console_write(struct console *con,
+				  const char *s, unsigned int n)
 {
 	struct earlycon_device *dev = con->data;
 	uart_console_write(&dev->port, s, n, sbi_putc);
 }
+#endif
+
+static void sbi_dbcn_console_write(struct console *con,
+				   const char *s, unsigned int n)
+{
+	phys_addr_t pa = __pa(s);
+
+	sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
+#ifdef CONFIG_32BIT
+		  n, pa, (u64)pa >> 32,
+#else
+		  n, pa, 0,
+#endif
+		  0, 0, 0);
+}
 
 static int __init early_sbi_setup(struct earlycon_device *device,
 				  const char *opt)
 {
-	device->con->write = sbi_console_write;
-	return 0;
+	int ret = 0;
+
+	if ((sbi_spec_version >= sbi_mk_version(2, 0)) &&
+	    (sbi_probe_extension(SBI_EXT_DBCN) > 0))
+		device->con->write = sbi_dbcn_console_write;
+	else
+#ifdef CONFIG_RISCV_SBI_V01
+		device->con->write = sbi_0_1_console_write;
+#else
+		ret = -ENODEV;
+#endif
+
+	return ret;
 }
 EARLYCON_DECLARE(sbi, early_sbi_setup);
-- 
2.34.1

[PATCH 5/6] tty: Add SBI debug console support to HVC SBI driver

[Anup Patel]

From: Atish Patra <atishp@rivosinc.com>

RISC-V SBI specification supports advanced debug console
support via SBI DBCN extension.

Extend the HVC SBI driver to support it.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 drivers/tty/hvc/Kconfig         |  2 +-
 drivers/tty/hvc/hvc_riscv_sbi.c | 80 ++++++++++++++++++++++++++++++---
 2 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
index 4f9264d005c0..6e05c5c7bca1 100644
--- a/drivers/tty/hvc/Kconfig
+++ b/drivers/tty/hvc/Kconfig
@@ -108,7 +108,7 @@ config HVC_DCC_SERIALIZE_SMP
 
 config HVC_RISCV_SBI
 	bool "RISC-V SBI console support"
-	depends on RISCV_SBI_V01
+	depends on RISCV_SBI
 	select HVC_DRIVER
 	help
 	  This enables support for console output via RISC-V SBI calls, which
diff --git a/drivers/tty/hvc/hvc_riscv_sbi.c b/drivers/tty/hvc/hvc_riscv_sbi.c
index 31f53fa77e4a..be8b7e351840 100644
--- a/drivers/tty/hvc/hvc_riscv_sbi.c
+++ b/drivers/tty/hvc/hvc_riscv_sbi.c
@@ -15,6 +15,7 @@
 
 #include "hvc_console.h"
 
+#ifdef CONFIG_RISCV_SBI_V01
 static int hvc_sbi_tty_put(uint32_t vtermno, const char *buf, int count)
 {
 	int i;
@@ -39,21 +40,86 @@ static int hvc_sbi_tty_get(uint32_t vtermno, char *buf, int count)
 	return i;
 }
 
-static const struct hv_ops hvc_sbi_ops = {
+static const struct hv_ops hvc_sbi_v01_ops = {
 	.get_chars = hvc_sbi_tty_get,
 	.put_chars = hvc_sbi_tty_put,
 };
+#endif
 
-static int __init hvc_sbi_init(void)
+static int hvc_sbi_dbcn_tty_put(uint32_t vtermno, const char *buf, int count)
 {
-	return PTR_ERR_OR_ZERO(hvc_alloc(0, 0, &hvc_sbi_ops, 16));
+	phys_addr_t pa;
+	struct sbiret ret;
+
+	if (is_vmalloc_addr(buf))
+		pa = page_to_phys(vmalloc_to_page(buf)) + offset_in_page(buf);
+	else
+		pa = __pa(buf);
+
+	ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
+#ifdef CONFIG_32BIT
+		  count, pa, (u64)pa >> 32,
+#else
+		  count, pa, 0,
+#endif
+		  0, 0, 0);
+
+	if (ret.error)
+		return 0;
+
+	return count;
 }
-device_initcall(hvc_sbi_init);
 
-static int __init hvc_sbi_console_init(void)
+static int hvc_sbi_dbcn_tty_get(uint32_t vtermno, char *buf, int count)
 {
-	hvc_instantiate(0, 0, &hvc_sbi_ops);
+	phys_addr_t pa;
+	struct sbiret ret;
+
+	if (is_vmalloc_addr(buf))
+		pa = page_to_phys(vmalloc_to_page(buf)) + offset_in_page(buf);
+	else
+		pa = __pa(buf);
+
+	ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_READ,
+#ifdef CONFIG_32BIT
+		  count, pa, (u64)pa >> 32,
+#else
+		  count, pa, 0,
+#endif
+		  0, 0, 0);
+
+	if (ret.error)
+		return 0;
+
+	return ret.value;
+}
+
+static const struct hv_ops hvc_sbi_dbcn_ops = {
+	.put_chars = hvc_sbi_dbcn_tty_put,
+	.get_chars = hvc_sbi_dbcn_tty_get,
+};
+
+static int __init hvc_sbi_init(void)
+{
+	int err;
+
+	if ((sbi_spec_version >= sbi_mk_version(2, 0)) &&
+	    (sbi_probe_extension(SBI_EXT_DBCN) > 0)) {
+		err = PTR_ERR_OR_ZERO(hvc_alloc(0, 0, &hvc_sbi_dbcn_ops, 16));
+		if (err)
+			return err;
+		hvc_instantiate(0, 0, &hvc_sbi_dbcn_ops);
+	} else {
+#ifdef CONFIG_RISCV_SBI_V01
+		err = PTR_ERR_OR_ZERO(hvc_alloc(0, 0, &hvc_sbi_v01_ops, 16));
+		if (err)
+			return err;
+		hvc_instantiate(0, 0, &hvc_sbi_v01_ops);
+#else
+		return -ENODEV;
+#endif
+	}
 
 	return 0;
 }
-console_initcall(hvc_sbi_console_init);
+device_initcall(hvc_sbi_init);
-- 
2.34.1

[PATCH 6/6] RISC-V: Enable SBI based earlycon support

[Anup Patel]

Let us enable SBI based earlycon support in defconfigs for both RV32
and RV64 so that "earlycon=sbi" can be used again.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arch/riscv/configs/defconfig      | 1 +
 arch/riscv/configs/rv32_defconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index ab86ec3b9eab..f82700da0056 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -132,6 +132,7 @@ CONFIG_SERIAL_8250_CONSOLE=y
 CONFIG_SERIAL_8250_DW=y
 CONFIG_SERIAL_OF_PLATFORM=y
 CONFIG_SERIAL_SH_SCI=y
+CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
 CONFIG_VIRTIO_CONSOLE=y
 CONFIG_HW_RANDOM=y
 CONFIG_HW_RANDOM_VIRTIO=y
diff --git a/arch/riscv/configs/rv32_defconfig b/arch/riscv/configs/rv32_defconfig
index 89b601e253a6..5721af39afd1 100644
--- a/arch/riscv/configs/rv32_defconfig
+++ b/arch/riscv/configs/rv32_defconfig
@@ -66,6 +66,7 @@ CONFIG_INPUT_MOUSEDEV=y
 CONFIG_SERIAL_8250=y
 CONFIG_SERIAL_8250_CONSOLE=y
 CONFIG_SERIAL_OF_PLATFORM=y
+CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
 CONFIG_VIRTIO_CONSOLE=y
 CONFIG_HW_RANDOM=y
 CONFIG_HW_RANDOM_VIRTIO=y
-- 
2.34.1

Re: [PATCH 5/6] tty: Add SBI debug console support to HVC SBI driver

[Greg Kroah-Hartman]

On Tue, Oct 10, 2023 at 10:35:02PM +0530, Anup Patel wrote:
> --- a/drivers/tty/hvc/hvc_riscv_sbi.c
> +++ b/drivers/tty/hvc/hvc_riscv_sbi.c
> @@ -15,6 +15,7 @@
>  
>  #include "hvc_console.h"
>  
> +#ifdef CONFIG_RISCV_SBI_V01

Please no #ifdef in a .c file, that's not a good style for Linux code at
all.

And what if you want to build the driver for both options here?  What
will happen?

> +static int hvc_sbi_dbcn_tty_put(uint32_t vtermno, const char *buf, int count)
>  {
> -	return PTR_ERR_OR_ZERO(hvc_alloc(0, 0, &hvc_sbi_ops, 16));
> +	phys_addr_t pa;
> +	struct sbiret ret;
> +
> +	if (is_vmalloc_addr(buf))
> +		pa = page_to_phys(vmalloc_to_page(buf)) + offset_in_page(buf);
> +	else
> +		pa = __pa(buf);
> +
> +	ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> +#ifdef CONFIG_32BIT
> +		  count, pa, (u64)pa >> 32,
> +#else
> +		  count, pa, 0,
> +#endif

This is not how to do an api, sorry, again, please no #ifdef if you want
to support this code for the next 20+ years.

thanks,

gre gk-h

Re: [PATCH 2/6] RISC-V: KVM: Change the SBI specification version to v2.0

[Greg Kroah-Hartman]

On Tue, Oct 10, 2023 at 10:34:59PM +0530, Anup Patel wrote:
> We will be implementing SBI DBCN extension for KVM RISC-V so let
> us change the KVM RISC-V SBI specification version to v2.0.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index cdcf0ff07be7..8d6d4dce8a5e 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -11,7 +11,7 @@
>  
>  #define KVM_SBI_IMPID 3
>  
> -#define KVM_SBI_VERSION_MAJOR 1
> +#define KVM_SBI_VERSION_MAJOR 2

What does this number mean?  Who checks it?  Why do you have to keep
incrementing it?

thanks,

greg k-h

Re: [PATCH 3/6] RISC-V: KVM: Forward SBI DBCN extension to user-space

[Greg Kroah-Hartman]

On Tue, Oct 10, 2023 at 10:35:00PM +0530, Anup Patel wrote:
> The SBI DBCN extension needs to be emulated in user-space

Why?

> so let
> us forward console_puts() call to user-space.

What could go wrong!

Why does userspace have to get involved in a console message?  Why is
this needed at all?  The kernel can not handle userspace consoles as
obviously they have to be re-entrant and irq safe.

> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_sbi.h |  1 +
>  arch/riscv/include/uapi/asm/kvm.h     |  1 +
>  arch/riscv/kvm/vcpu_sbi.c             |  4 ++++
>  arch/riscv/kvm/vcpu_sbi_replace.c     | 31 +++++++++++++++++++++++++++
>  4 files changed, 37 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index 8d6d4dce8a5e..a85f95eb6e85 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -69,6 +69,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_ipi;
>  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence;
>  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
>  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
> +extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
>  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
>  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
>  
> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> index 917d8cc2489e..60d3b21dead7 100644
> --- a/arch/riscv/include/uapi/asm/kvm.h
> +++ b/arch/riscv/include/uapi/asm/kvm.h
> @@ -156,6 +156,7 @@ enum KVM_RISCV_SBI_EXT_ID {
>  	KVM_RISCV_SBI_EXT_PMU,
>  	KVM_RISCV_SBI_EXT_EXPERIMENTAL,
>  	KVM_RISCV_SBI_EXT_VENDOR,
> +	KVM_RISCV_SBI_EXT_DBCN,
>  	KVM_RISCV_SBI_EXT_MAX,

You just broke a user/kernel ABI here, why?

thanks,

greg k-h

Re: [PATCH 4/6] tty/serial: Add RISC-V SBI debug console based earlycon

[Greg Kroah-Hartman]

On Tue, Oct 10, 2023 at 10:35:01PM +0530, Anup Patel wrote:
> We extend the existing RISC-V SBI earlycon support to use the new
> RISC-V SBI debug console extension.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  drivers/tty/serial/Kconfig              |  2 +-
>  drivers/tty/serial/earlycon-riscv-sbi.c | 35 ++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index bdc568a4ab66..cec46091a716 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -87,7 +87,7 @@ config SERIAL_EARLYCON_SEMIHOST
>  
>  config SERIAL_EARLYCON_RISCV_SBI
>  	bool "Early console using RISC-V SBI"
> -	depends on RISCV_SBI_V01
> +	depends on RISCV_SBI
>  	select SERIAL_CORE
>  	select SERIAL_CORE_CONSOLE
>  	select SERIAL_EARLYCON
> diff --git a/drivers/tty/serial/earlycon-riscv-sbi.c b/drivers/tty/serial/earlycon-riscv-sbi.c
> index 27afb0b74ea7..b1da34e8d8cd 100644
> --- a/drivers/tty/serial/earlycon-riscv-sbi.c
> +++ b/drivers/tty/serial/earlycon-riscv-sbi.c
> @@ -10,22 +10,49 @@
>  #include <linux/serial_core.h>
>  #include <asm/sbi.h>
>  
> +#ifdef CONFIG_RISCV_SBI_V01
>  static void sbi_putc(struct uart_port *port, unsigned char c)
>  {
>  	sbi_console_putchar(c);
>  }
>  
> -static void sbi_console_write(struct console *con,
> -			      const char *s, unsigned n)
> +static void sbi_0_1_console_write(struct console *con,
> +				  const char *s, unsigned int n)
>  {
>  	struct earlycon_device *dev = con->data;
>  	uart_console_write(&dev->port, s, n, sbi_putc);
>  }
> +#endif
> +
> +static void sbi_dbcn_console_write(struct console *con,
> +				   const char *s, unsigned int n)
> +{
> +	phys_addr_t pa = __pa(s);
> +
> +	sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> +#ifdef CONFIG_32BIT
> +		  n, pa, (u64)pa >> 32,
> +#else
> +		  n, pa, 0,
> +#endif

Again, no #ifdef in .c files please.

thanks,

greg k-h

Re: [PATCH 5/6] tty: Add SBI debug console support to HVC SBI driver

[Anup Patel]

On Tue, Oct 10, 2023 at 10:42 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 10, 2023 at 10:35:02PM +0530, Anup Patel wrote:
> > --- a/drivers/tty/hvc/hvc_riscv_sbi.c
> > +++ b/drivers/tty/hvc/hvc_riscv_sbi.c
> > @@ -15,6 +15,7 @@
> >
> >  #include "hvc_console.h"
> >
> > +#ifdef CONFIG_RISCV_SBI_V01
>
> Please no #ifdef in a .c file, that's not a good style for Linux code at
> all.
>
> And what if you want to build the driver for both options here?  What
> will happen?

Okay, I will remove all #ifdef from .c file

>
> > +static int hvc_sbi_dbcn_tty_put(uint32_t vtermno, const char *buf, int count)
> >  {
> > -     return PTR_ERR_OR_ZERO(hvc_alloc(0, 0, &hvc_sbi_ops, 16));
> > +     phys_addr_t pa;
> > +     struct sbiret ret;
> > +
> > +     if (is_vmalloc_addr(buf))
> > +             pa = page_to_phys(vmalloc_to_page(buf)) + offset_in_page(buf);
> > +     else
> > +             pa = __pa(buf);
> > +
> > +     ret = sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> > +#ifdef CONFIG_32BIT
> > +               count, pa, (u64)pa >> 32,
> > +#else
> > +               count, pa, 0,
> > +#endif
>
> This is not how to do an api, sorry, again, please no #ifdef if you want
> to support this code for the next 20+ years.

Sure, I will update like you suggested.

>
> thanks,
>
> gre gk-h

Thanks,
Anup

Re: [PATCH 4/6] tty/serial: Add RISC-V SBI debug console based earlycon

[Anup Patel]

On Tue, Oct 10, 2023 at 10:46 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 10, 2023 at 10:35:01PM +0530, Anup Patel wrote:
> > We extend the existing RISC-V SBI earlycon support to use the new
> > RISC-V SBI debug console extension.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  drivers/tty/serial/Kconfig              |  2 +-
> >  drivers/tty/serial/earlycon-riscv-sbi.c | 35 ++++++++++++++++++++++---
> >  2 files changed, 32 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> > index bdc568a4ab66..cec46091a716 100644
> > --- a/drivers/tty/serial/Kconfig
> > +++ b/drivers/tty/serial/Kconfig
> > @@ -87,7 +87,7 @@ config SERIAL_EARLYCON_SEMIHOST
> >
> >  config SERIAL_EARLYCON_RISCV_SBI
> >       bool "Early console using RISC-V SBI"
> > -     depends on RISCV_SBI_V01
> > +     depends on RISCV_SBI
> >       select SERIAL_CORE
> >       select SERIAL_CORE_CONSOLE
> >       select SERIAL_EARLYCON
> > diff --git a/drivers/tty/serial/earlycon-riscv-sbi.c b/drivers/tty/serial/earlycon-riscv-sbi.c
> > index 27afb0b74ea7..b1da34e8d8cd 100644
> > --- a/drivers/tty/serial/earlycon-riscv-sbi.c
> > +++ b/drivers/tty/serial/earlycon-riscv-sbi.c
> > @@ -10,22 +10,49 @@
> >  #include <linux/serial_core.h>
> >  #include <asm/sbi.h>
> >
> > +#ifdef CONFIG_RISCV_SBI_V01
> >  static void sbi_putc(struct uart_port *port, unsigned char c)
> >  {
> >       sbi_console_putchar(c);
> >  }
> >
> > -static void sbi_console_write(struct console *con,
> > -                           const char *s, unsigned n)
> > +static void sbi_0_1_console_write(struct console *con,
> > +                               const char *s, unsigned int n)
> >  {
> >       struct earlycon_device *dev = con->data;
> >       uart_console_write(&dev->port, s, n, sbi_putc);
> >  }
> > +#endif
> > +
> > +static void sbi_dbcn_console_write(struct console *con,
> > +                                const char *s, unsigned int n)
> > +{
> > +     phys_addr_t pa = __pa(s);
> > +
> > +     sbi_ecall(SBI_EXT_DBCN, SBI_EXT_DBCN_CONSOLE_WRITE,
> > +#ifdef CONFIG_32BIT
> > +               n, pa, (u64)pa >> 32,
> > +#else
> > +               n, pa, 0,
> > +#endif
>
> Again, no #ifdef in .c files please.

Okay, I will remove #ifdef from here as well.

>
> thanks,
>
> greg k-h

Thanks,
Anup

Re: [PATCH 2/6] RISC-V: KVM: Change the SBI specification version to v2.0

[Anup Patel]

On Tue, Oct 10, 2023 at 10:43 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 10, 2023 at 10:34:59PM +0530, Anup Patel wrote:
> > We will be implementing SBI DBCN extension for KVM RISC-V so let
> > us change the KVM RISC-V SBI specification version to v2.0.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > index cdcf0ff07be7..8d6d4dce8a5e 100644
> > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > @@ -11,7 +11,7 @@
> >
> >  #define KVM_SBI_IMPID 3
> >
> > -#define KVM_SBI_VERSION_MAJOR 1
> > +#define KVM_SBI_VERSION_MAJOR 2
>
> What does this number mean?  Who checks it?  Why do you have to keep
> incrementing it?

This number is the SBI specification version implemented by KVM RISC-V
for the Guest kernel.

The original sbi_console_putchar() and sbi_console_getchar() are legacy
functions (aka SBI v0.1) which were introduced a few years back along
with the Linux RISC-V port.

The latest SBI v2.0 specification (which is now frozen) introduces a new
SBI debug console extension which replaces legacy sbi_console_putchar()
and sbi_console_getchar() functions with better alternatives.
(Refer, https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/commit-fe4562532a9cc57e5743b6466946c5e5c98c73ca/riscv-sbi.pdf)

This series adds SBI debug console implementation in KVM RISC-V
so the SBI specification version advertised by KVM RISC-V must also be
upgraded to v2.0.

Regarding who checks its, the SBI client drivers in the Linux kernel
will check SBI specification version implemented by higher privilege
mode (M-mode firmware or HS-mode hypervisor) before probing
the SBI extension. For example, the HVC SBI driver (PATCH5)
will ensure SBI spec version to be at least v2.0 before probing
SBI debug console extension.

>
> thanks,
>
> greg k-h

Regards,
Anup

Re: [PATCH 3/6] RISC-V: KVM: Forward SBI DBCN extension to user-space

[Anup Patel]

On Tue, Oct 10, 2023 at 10:45 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 10, 2023 at 10:35:00PM +0530, Anup Patel wrote:
> > The SBI DBCN extension needs to be emulated in user-space
>
> Why?

The SBI debug console is similar to a console port available to
KVM Guest so the KVM user space tool (i.e. QEMU-KVM or
KVMTOOL) can redirect the input/output of SBI debug console
wherever it wants (e.g.  telnet, file, stdio, etc).

We forward SBI DBCN calls to KVM user space so that the
in-kernel KVM does not need to be aware of the guest
console devices.

>
> > so let
> > us forward console_puts() call to user-space.
>
> What could go wrong!
>
> Why does userspace have to get involved in a console message?  Why is
> this needed at all?  The kernel can not handle userspace consoles as
> obviously they have to be re-entrant and irq safe.

As mentioned above, these are KVM guest console messages which
the VMM (i.e. KVM user-space) can choose to manage on its own.

This is more about providing flexibility to KVM user-space which
allows it to manage guest console devices.

>
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  1 +
> >  arch/riscv/include/uapi/asm/kvm.h     |  1 +
> >  arch/riscv/kvm/vcpu_sbi.c             |  4 ++++
> >  arch/riscv/kvm/vcpu_sbi_replace.c     | 31 +++++++++++++++++++++++++++
> >  4 files changed, 37 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > index 8d6d4dce8a5e..a85f95eb6e85 100644
> > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > @@ -69,6 +69,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_ipi;
> >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence;
> >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
> >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
> > +extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
> >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
> >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
> >
> > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > index 917d8cc2489e..60d3b21dead7 100644
> > --- a/arch/riscv/include/uapi/asm/kvm.h
> > +++ b/arch/riscv/include/uapi/asm/kvm.h
> > @@ -156,6 +156,7 @@ enum KVM_RISCV_SBI_EXT_ID {
> >       KVM_RISCV_SBI_EXT_PMU,
> >       KVM_RISCV_SBI_EXT_EXPERIMENTAL,
> >       KVM_RISCV_SBI_EXT_VENDOR,
> > +     KVM_RISCV_SBI_EXT_DBCN,
> >       KVM_RISCV_SBI_EXT_MAX,
>
> You just broke a user/kernel ABI here, why?

The KVM_RISCV_SBI_EXT_MAX only represents the number
of entries in "enum KVM_RISCV_SBI_EXT_ID" so we are not
breaking "enum KVM_RISCV_SBI_EXT_ID" rather appending
new ID to existing enum.

>
> thanks,
>
> greg k-h

Thanks,
Anup

Re: [PATCH 3/6] RISC-V: KVM: Forward SBI DBCN extension to user-space

[Greg Kroah-Hartman]

On Wed, Oct 11, 2023 at 12:02:30PM +0530, Anup Patel wrote:
> On Tue, Oct 10, 2023 at 10:45 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Oct 10, 2023 at 10:35:00PM +0530, Anup Patel wrote:
> > > The SBI DBCN extension needs to be emulated in user-space
> >
> > Why?
> 
> The SBI debug console is similar to a console port available to
> KVM Guest so the KVM user space tool (i.e. QEMU-KVM or
> KVMTOOL) can redirect the input/output of SBI debug console
> wherever it wants (e.g.  telnet, file, stdio, etc).
> 
> We forward SBI DBCN calls to KVM user space so that the
> in-kernel KVM does not need to be aware of the guest
> console devices.

Hint, my "Why" was attempting to get you to write a better changelog
description, which would include the above information.  Please read the
kernel documentation for hints on how to do this so that we know what
why changes are being made.

> > > so let
> > > us forward console_puts() call to user-space.
> >
> > What could go wrong!
> >
> > Why does userspace have to get involved in a console message?  Why is
> > this needed at all?  The kernel can not handle userspace consoles as
> > obviously they have to be re-entrant and irq safe.
> 
> As mentioned above, these are KVM guest console messages which
> the VMM (i.e. KVM user-space) can choose to manage on its own.

If it chooses not to, what happens?

> This is more about providing flexibility to KVM user-space which
> allows it to manage guest console devices.

Why not use the normal virtio console device interface instead of making
a riscv-custom one?

Where is the userspace side of this interface at?  Where are the patches
to handle this new api you added?

> 
> >
> > >
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  1 +
> > >  arch/riscv/include/uapi/asm/kvm.h     |  1 +
> > >  arch/riscv/kvm/vcpu_sbi.c             |  4 ++++
> > >  arch/riscv/kvm/vcpu_sbi_replace.c     | 31 +++++++++++++++++++++++++++
> > >  4 files changed, 37 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > index 8d6d4dce8a5e..a85f95eb6e85 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > @@ -69,6 +69,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_ipi;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
> > > +extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
> > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
> > >
> > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > index 917d8cc2489e..60d3b21dead7 100644
> > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > +++ b/arch/riscv/include/uapi/asm/kvm.h
> > > @@ -156,6 +156,7 @@ enum KVM_RISCV_SBI_EXT_ID {
> > >       KVM_RISCV_SBI_EXT_PMU,
> > >       KVM_RISCV_SBI_EXT_EXPERIMENTAL,
> > >       KVM_RISCV_SBI_EXT_VENDOR,
> > > +     KVM_RISCV_SBI_EXT_DBCN,
> > >       KVM_RISCV_SBI_EXT_MAX,
> >
> > You just broke a user/kernel ABI here, why?
> 
> The KVM_RISCV_SBI_EXT_MAX only represents the number
> of entries in "enum KVM_RISCV_SBI_EXT_ID" so we are not
> breaking "enum KVM_RISCV_SBI_EXT_ID" rather appending
> new ID to existing enum.

So you are sure that userspace never actually tests or sends that _MAX
value anywhere?  If not, why is it even needed?

thanks,

greg k-h

Re: [PATCH 2/6] RISC-V: KVM: Change the SBI specification version to v2.0

[Greg Kroah-Hartman]

On Wed, Oct 11, 2023 at 11:49:14AM +0530, Anup Patel wrote:
> On Tue, Oct 10, 2023 at 10:43 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Oct 10, 2023 at 10:34:59PM +0530, Anup Patel wrote:
> > > We will be implementing SBI DBCN extension for KVM RISC-V so let
> > > us change the KVM RISC-V SBI specification version to v2.0.
> > >
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > index cdcf0ff07be7..8d6d4dce8a5e 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > @@ -11,7 +11,7 @@
> > >
> > >  #define KVM_SBI_IMPID 3
> > >
> > > -#define KVM_SBI_VERSION_MAJOR 1
> > > +#define KVM_SBI_VERSION_MAJOR 2
> >
> > What does this number mean?  Who checks it?  Why do you have to keep
> > incrementing it?
> 
> This number is the SBI specification version implemented by KVM RISC-V
> for the Guest kernel.
> 
> The original sbi_console_putchar() and sbi_console_getchar() are legacy
> functions (aka SBI v0.1) which were introduced a few years back along
> with the Linux RISC-V port.
> 
> The latest SBI v2.0 specification (which is now frozen) introduces a new
> SBI debug console extension which replaces legacy sbi_console_putchar()
> and sbi_console_getchar() functions with better alternatives.
> (Refer, https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/commit-fe4562532a9cc57e5743b6466946c5e5c98c73ca/riscv-sbi.pdf)
> 
> This series adds SBI debug console implementation in KVM RISC-V
> so the SBI specification version advertised by KVM RISC-V must also be
> upgraded to v2.0.
> 
> Regarding who checks its, the SBI client drivers in the Linux kernel
> will check SBI specification version implemented by higher privilege
> mode (M-mode firmware or HS-mode hypervisor) before probing
> the SBI extension. For example, the HVC SBI driver (PATCH5)
> will ensure SBI spec version to be at least v2.0 before probing
> SBI debug console extension.

Is this api backwards compatible, or did you just break existing
userspace that only expects version 1.0?

thanks,

greg k-h

Re: [PATCH 3/6] RISC-V: KVM: Forward SBI DBCN extension to user-space

[Anup Patel]

On Wed, Oct 11, 2023 at 12:56 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 11, 2023 at 12:02:30PM +0530, Anup Patel wrote:
> > On Tue, Oct 10, 2023 at 10:45 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 10:35:00PM +0530, Anup Patel wrote:
> > > > The SBI DBCN extension needs to be emulated in user-space
> > >
> > > Why?
> >
> > The SBI debug console is similar to a console port available to
> > KVM Guest so the KVM user space tool (i.e. QEMU-KVM or
> > KVMTOOL) can redirect the input/output of SBI debug console
> > wherever it wants (e.g.  telnet, file, stdio, etc).
> >
> > We forward SBI DBCN calls to KVM user space so that the
> > in-kernel KVM does not need to be aware of the guest
> > console devices.
>
> Hint, my "Why" was attempting to get you to write a better changelog
> description, which would include the above information.  Please read the
> kernel documentation for hints on how to do this so that we know what
> why changes are being made.

Okay, I will improve the commit description and cover-letter.

>
> > > > so let
> > > > us forward console_puts() call to user-space.
> > >
> > > What could go wrong!
> > >
> > > Why does userspace have to get involved in a console message?  Why is
> > > this needed at all?  The kernel can not handle userspace consoles as
> > > obviously they have to be re-entrant and irq safe.
> >
> > As mentioned above, these are KVM guest console messages which
> > the VMM (i.e. KVM user-space) can choose to manage on its own.
>
> If it chooses not to, what happens?

If KVM user-space chooses not to handle SBI DBCN calls then it can
disable SBI DBCN extension for Guest VCPUs using the ONE_REG
ioctl() interface.

>
> > This is more about providing flexibility to KVM user-space which
> > allows it to manage guest console devices.
>
> Why not use the normal virtio console device interface instead of making
> a riscv-custom one?

The SBI DBCN (or debug console) is only an early console used for
early prints and bootloaders.

Once the proper console (like virtio console) is detected by the Guest
kernel, it will switch the debug console to proper console.

>
> Where is the userspace side of this interface at?  Where are the patches
> to handle this new api you added?

As mentioned in the cover letter, I have implemented it in KVMTOOL first.

The patches can be found in riscv_sbi_dbcn_v1 branch at:
https://github.com/avpatel/kvmtool.git

More precisely, this commit:
https://github.com/avpatel/kvmtool/commit/06a373ee8991f882ef79de3845a4c8d63cb189a6

>
> >
> > >
> > > >
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > ---
> > > >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  1 +
> > > >  arch/riscv/include/uapi/asm/kvm.h     |  1 +
> > > >  arch/riscv/kvm/vcpu_sbi.c             |  4 ++++
> > > >  arch/riscv/kvm/vcpu_sbi_replace.c     | 31 +++++++++++++++++++++++++++
> > > >  4 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > index 8d6d4dce8a5e..a85f95eb6e85 100644
> > > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > @@ -69,6 +69,7 @@ extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_ipi;
> > > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence;
> > > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst;
> > > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm;
> > > > +extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_dbcn;
> > > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_experimental;
> > > >  extern const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_vendor;
> > > >
> > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > index 917d8cc2489e..60d3b21dead7 100644
> > > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > > +++ b/arch/riscv/include/uapi/asm/kvm.h
> > > > @@ -156,6 +156,7 @@ enum KVM_RISCV_SBI_EXT_ID {
> > > >       KVM_RISCV_SBI_EXT_PMU,
> > > >       KVM_RISCV_SBI_EXT_EXPERIMENTAL,
> > > >       KVM_RISCV_SBI_EXT_VENDOR,
> > > > +     KVM_RISCV_SBI_EXT_DBCN,
> > > >       KVM_RISCV_SBI_EXT_MAX,
> > >
> > > You just broke a user/kernel ABI here, why?
> >
> > The KVM_RISCV_SBI_EXT_MAX only represents the number
> > of entries in "enum KVM_RISCV_SBI_EXT_ID" so we are not
> > breaking "enum KVM_RISCV_SBI_EXT_ID" rather appending
> > new ID to existing enum.
>
> So you are sure that userspace never actually tests or sends that _MAX
> value anywhere?  If not, why is it even needed?
>
> thanks,
>
> greg k-h

Regards,
Anup

Re: [PATCH 2/6] RISC-V: KVM: Change the SBI specification version to v2.0

[Anup Patel]

On Wed, Oct 11, 2023 at 12:57 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 11, 2023 at 11:49:14AM +0530, Anup Patel wrote:
> > On Tue, Oct 10, 2023 at 10:43 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 10:34:59PM +0530, Anup Patel wrote:
> > > > We will be implementing SBI DBCN extension for KVM RISC-V so let
> > > > us change the KVM RISC-V SBI specification version to v2.0.
> > > >
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > ---
> > > >  arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > index cdcf0ff07be7..8d6d4dce8a5e 100644
> > > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > @@ -11,7 +11,7 @@
> > > >
> > > >  #define KVM_SBI_IMPID 3
> > > >
> > > > -#define KVM_SBI_VERSION_MAJOR 1
> > > > +#define KVM_SBI_VERSION_MAJOR 2
> > >
> > > What does this number mean?  Who checks it?  Why do you have to keep
> > > incrementing it?
> >
> > This number is the SBI specification version implemented by KVM RISC-V
> > for the Guest kernel.
> >
> > The original sbi_console_putchar() and sbi_console_getchar() are legacy
> > functions (aka SBI v0.1) which were introduced a few years back along
> > with the Linux RISC-V port.
> >
> > The latest SBI v2.0 specification (which is now frozen) introduces a new
> > SBI debug console extension which replaces legacy sbi_console_putchar()
> > and sbi_console_getchar() functions with better alternatives.
> > (Refer, https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/commit-fe4562532a9cc57e5743b6466946c5e5c98c73ca/riscv-sbi.pdf)
> >
> > This series adds SBI debug console implementation in KVM RISC-V
> > so the SBI specification version advertised by KVM RISC-V must also be
> > upgraded to v2.0.
> >
> > Regarding who checks its, the SBI client drivers in the Linux kernel
> > will check SBI specification version implemented by higher privilege
> > mode (M-mode firmware or HS-mode hypervisor) before probing
> > the SBI extension. For example, the HVC SBI driver (PATCH5)
> > will ensure SBI spec version to be at least v2.0 before probing
> > SBI debug console extension.
>
> Is this api backwards compatible, or did you just break existing
> userspace that only expects version 1.0?

The legacy sbi_console_putchar() and sbi_console_getchar()
functions have not changed so it does not break existing
user-space.

The new SBI DBCN functions to be implemented by KVM
user space are:
sbi_debug_console_write()
sbi_debug_console_read()
sbi_debug_console_write_byte()

>
> thanks,
>
> greg k-h

Regards,
Anup

Re: [PATCH 2/6] RISC-V: KVM: Change the SBI specification version to v2.0

[Greg Kroah-Hartman]

On Wed, Oct 11, 2023 at 04:32:22PM +0530, Anup Patel wrote:
> On Wed, Oct 11, 2023 at 12:57 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Oct 11, 2023 at 11:49:14AM +0530, Anup Patel wrote:
> > > On Tue, Oct 10, 2023 at 10:43 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Oct 10, 2023 at 10:34:59PM +0530, Anup Patel wrote:
> > > > > We will be implementing SBI DBCN extension for KVM RISC-V so let
> > > > > us change the KVM RISC-V SBI specification version to v2.0.
> > > > >
> > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > > ---
> > > > >  arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > index cdcf0ff07be7..8d6d4dce8a5e 100644
> > > > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > @@ -11,7 +11,7 @@
> > > > >
> > > > >  #define KVM_SBI_IMPID 3
> > > > >
> > > > > -#define KVM_SBI_VERSION_MAJOR 1
> > > > > +#define KVM_SBI_VERSION_MAJOR 2
> > > >
> > > > What does this number mean?  Who checks it?  Why do you have to keep
> > > > incrementing it?
> > >
> > > This number is the SBI specification version implemented by KVM RISC-V
> > > for the Guest kernel.
> > >
> > > The original sbi_console_putchar() and sbi_console_getchar() are legacy
> > > functions (aka SBI v0.1) which were introduced a few years back along
> > > with the Linux RISC-V port.
> > >
> > > The latest SBI v2.0 specification (which is now frozen) introduces a new
> > > SBI debug console extension which replaces legacy sbi_console_putchar()
> > > and sbi_console_getchar() functions with better alternatives.
> > > (Refer, https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/commit-fe4562532a9cc57e5743b6466946c5e5c98c73ca/riscv-sbi.pdf)
> > >
> > > This series adds SBI debug console implementation in KVM RISC-V
> > > so the SBI specification version advertised by KVM RISC-V must also be
> > > upgraded to v2.0.
> > >
> > > Regarding who checks its, the SBI client drivers in the Linux kernel
> > > will check SBI specification version implemented by higher privilege
> > > mode (M-mode firmware or HS-mode hypervisor) before probing
> > > the SBI extension. For example, the HVC SBI driver (PATCH5)
> > > will ensure SBI spec version to be at least v2.0 before probing
> > > SBI debug console extension.
> >
> > Is this api backwards compatible, or did you just break existing
> > userspace that only expects version 1.0?
> 
> The legacy sbi_console_putchar() and sbi_console_getchar()
> functions have not changed so it does not break existing
> user-space.
> 
> The new SBI DBCN functions to be implemented by KVM
> user space are:
> sbi_debug_console_write()
> sbi_debug_console_read()
> sbi_debug_console_write_byte()

And where exactly is that code for us to review that this is tested?

thanks,

greg k-h

Re: [PATCH 2/6] RISC-V: KVM: Change the SBI specification version to v2.0

[Anup Patel]

On Wed, Oct 11, 2023 at 8:56 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 11, 2023 at 04:32:22PM +0530, Anup Patel wrote:
> > On Wed, Oct 11, 2023 at 12:57 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Oct 11, 2023 at 11:49:14AM +0530, Anup Patel wrote:
> > > > On Tue, Oct 10, 2023 at 10:43 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Tue, Oct 10, 2023 at 10:34:59PM +0530, Anup Patel wrote:
> > > > > > We will be implementing SBI DBCN extension for KVM RISC-V so let
> > > > > > us change the KVM RISC-V SBI specification version to v2.0.
> > > > > >
> > > > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > > > ---
> > > > > >  arch/riscv/include/asm/kvm_vcpu_sbi.h | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > > index cdcf0ff07be7..8d6d4dce8a5e 100644
> > > > > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > > > > @@ -11,7 +11,7 @@
> > > > > >
> > > > > >  #define KVM_SBI_IMPID 3
> > > > > >
> > > > > > -#define KVM_SBI_VERSION_MAJOR 1
> > > > > > +#define KVM_SBI_VERSION_MAJOR 2
> > > > >
> > > > > What does this number mean?  Who checks it?  Why do you have to keep
> > > > > incrementing it?
> > > >
> > > > This number is the SBI specification version implemented by KVM RISC-V
> > > > for the Guest kernel.
> > > >
> > > > The original sbi_console_putchar() and sbi_console_getchar() are legacy
> > > > functions (aka SBI v0.1) which were introduced a few years back along
> > > > with the Linux RISC-V port.
> > > >
> > > > The latest SBI v2.0 specification (which is now frozen) introduces a new
> > > > SBI debug console extension which replaces legacy sbi_console_putchar()
> > > > and sbi_console_getchar() functions with better alternatives.
> > > > (Refer, https://github.com/riscv-non-isa/riscv-sbi-doc/releases/download/commit-fe4562532a9cc57e5743b6466946c5e5c98c73ca/riscv-sbi.pdf)
> > > >
> > > > This series adds SBI debug console implementation in KVM RISC-V
> > > > so the SBI specification version advertised by KVM RISC-V must also be
> > > > upgraded to v2.0.
> > > >
> > > > Regarding who checks its, the SBI client drivers in the Linux kernel
> > > > will check SBI specification version implemented by higher privilege
> > > > mode (M-mode firmware or HS-mode hypervisor) before probing
> > > > the SBI extension. For example, the HVC SBI driver (PATCH5)
> > > > will ensure SBI spec version to be at least v2.0 before probing
> > > > SBI debug console extension.
> > >
> > > Is this api backwards compatible, or did you just break existing
> > > userspace that only expects version 1.0?
> >
> > The legacy sbi_console_putchar() and sbi_console_getchar()
> > functions have not changed so it does not break existing
> > user-space.
> >
> > The new SBI DBCN functions to be implemented by KVM
> > user space are:
> > sbi_debug_console_write()
> > sbi_debug_console_read()
> > sbi_debug_console_write_byte()
>
> And where exactly is that code for us to review that this is tested?

The KVM selftests for KVM RISC-V are under development. Eventually,
we will have dedicated KVM selftests for the SBI extensions implemented
by KVM RISC-V.

Until then we have KVMTOOL implementation for SBI DBCN, which is
available in riscv_sbi_dbcn_v1 branch at:
https://github.com/avpatel/kvmtool.git

>
> thanks,
>
> greg k-h

Regards,
Anup