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
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 <[email protected]>
---
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
We extend the existing RISC-V SBI earlycon support to use the new
RISC-V SBI debug console extension.
Signed-off-by: Anup Patel <[email protected]>
---
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
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 <[email protected]>
---
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
We add SBI debug console extension related defines/enum to the
asm/sbi.h header.
Signed-off-by: Anup Patel <[email protected]>
---
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
From: Atish Patra <[email protected]>
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 <[email protected]>
Signed-off-by: Anup Patel <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
On Tue, Oct 10, 2023 at 10:42 PM Greg Kroah-Hartman
<[email protected]> 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
On Tue, Oct 10, 2023 at 10:46 PM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> > ---
> > 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
On Tue, Oct 10, 2023 at 10:43 PM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> > ---
> > 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
On Tue, Oct 10, 2023 at 10:45 PM Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
> > ---
> > 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
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
> <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
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
> <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On Wed, Oct 11, 2023 at 12:56 PM Greg Kroah-Hartman
<[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
On Wed, Oct 11, 2023 at 12:57 PM Greg Kroah-Hartman
<[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
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
> <[email protected]> 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
> > > <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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
On Wed, Oct 11, 2023 at 8:56 PM Greg Kroah-Hartman
<[email protected]> 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
> > <[email protected]> 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
> > > > <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > > > 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