[PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Daniel Henrique Barboza]

SBI defines a Debug Console extension "DBCN" that will, in time, replace
the legacy console putchar and getchar SBI extensions.

The appeal of the DBCN extension is that it allows multiple bytes to be
read/written in the SBI console in a single SBI call.

As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
module to userspace. But this will only happens if the KVM module
actually supports this SBI extension and we activate it.

We'll check for DBCN support during init time, checking if get-reg-list
is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
kvm_set_one_reg() during kvm_arch_init_vcpu().

Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
SBI_EXT_DBCN, reading and writing as required.

A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
host, takes around 20 seconds to boot without using DBCN. With this
patch we're taking around 14 seconds to boot due to the speed-up in the
terminal output.  There's no change in boot time if the guest isn't
using earlycon.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
 target/riscv/sbi_ecall_interface.h |  17 +++++
 2 files changed, 128 insertions(+)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 03e3fee607..54a9ab9fd7 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -409,6 +409,12 @@ static KVMCPUConfig kvm_v_vlenb = {
                    KVM_REG_RISCV_VECTOR_CSR_REG(vlenb)
 };
 
+static KVMCPUConfig kvm_sbi_dbcn = {
+    .name = "sbi_dbcn",
+    .kvm_reg_id = KVM_REG_RISCV | KVM_REG_SIZE_U64 |
+                  KVM_REG_RISCV_SBI_EXT | KVM_RISCV_SBI_EXT_DBCN
+};
+
 static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
 {
     CPURISCVState *env = &cpu->env;
@@ -1041,6 +1047,20 @@ static int uint64_cmp(const void *a, const void *b)
     return 0;
 }
 
+static void kvm_riscv_check_sbi_dbcn_support(RISCVCPU *cpu,
+                                             KVMScratchCPU *kvmcpu,
+                                             struct kvm_reg_list *reglist)
+{
+    struct kvm_reg_list *reg_search;
+
+    reg_search = bsearch(&kvm_sbi_dbcn.kvm_reg_id, reglist->reg, reglist->n,
+                         sizeof(uint64_t), uint64_cmp);
+
+    if (reg_search) {
+        kvm_sbi_dbcn.supported = true;
+    }
+}
+
 static void kvm_riscv_read_vlenb(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
                                  struct kvm_reg_list *reglist)
 {
@@ -1146,6 +1166,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
     if (riscv_has_ext(&cpu->env, RVV)) {
         kvm_riscv_read_vlenb(cpu, kvmcpu, reglist);
     }
+
+    kvm_riscv_check_sbi_dbcn_support(cpu, kvmcpu, reglist);
 }
 
 static void riscv_init_kvm_registers(Object *cpu_obj)
@@ -1320,6 +1342,17 @@ static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
     return ret;
 }
 
+static int kvm_vcpu_enable_sbi_dbcn(RISCVCPU *cpu, CPUState *cs)
+{
+    target_ulong reg = 1;
+
+    if (!kvm_sbi_dbcn.supported) {
+        return 0;
+    }
+
+    return kvm_set_one_reg(cs, kvm_sbi_dbcn.kvm_reg_id, &reg);
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     int ret = 0;
@@ -1337,6 +1370,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
     kvm_riscv_update_cpu_misa_ext(cpu, cs);
     kvm_riscv_update_cpu_cfg_isa_ext(cpu, cs);
 
+    ret = kvm_vcpu_enable_sbi_dbcn(cpu, cs);
+
     return ret;
 }
 
@@ -1394,6 +1429,79 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
     return true;
 }
 
+static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
+{
+    g_autofree uint8_t *buf = NULL;
+    RISCVCPU *cpu = RISCV_CPU(cs);
+    target_ulong num_bytes;
+    uint64_t addr;
+    unsigned char ch;
+    int ret;
+
+    switch (run->riscv_sbi.function_id) {
+    case SBI_EXT_DBCN_CONSOLE_READ:
+    case SBI_EXT_DBCN_CONSOLE_WRITE:
+        num_bytes = run->riscv_sbi.args[0];
+
+        if (num_bytes == 0) {
+            run->riscv_sbi.ret[0] = SBI_SUCCESS;
+            run->riscv_sbi.ret[1] = 0;
+            break;
+        }
+
+        addr = run->riscv_sbi.args[1];
+
+        /*
+         * Handle the case where a 32 bit CPU is running in a
+         * 64 bit addressing env.
+         */
+        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
+            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
+        }
+
+        buf = g_malloc0(num_bytes);
+
+        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
+            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, num_bytes);
+            if (ret < 0) {
+                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
+                             "reading chardev");
+                exit(1);
+            }
+
+            cpu_physical_memory_write(addr, buf, ret);
+        } else {
+            cpu_physical_memory_read(addr, buf, num_bytes);
+
+            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, num_bytes);
+            if (ret < 0) {
+                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
+                             "writing chardev");
+                exit(1);
+            }
+        }
+
+        run->riscv_sbi.ret[0] = SBI_SUCCESS;
+        run->riscv_sbi.ret[1] = ret;
+        break;
+    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
+        ch = run->riscv_sbi.args[0];
+        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
+
+        if (ret < 0) {
+            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
+                         "writing chardev");
+            exit(1);
+        }
+
+        run->riscv_sbi.ret[0] = SBI_SUCCESS;
+        run->riscv_sbi.ret[1] = 0;
+        break;
+    default:
+        run->riscv_sbi.ret[0] = SBI_ERR_NOT_SUPPORTED;
+    }
+}
+
 static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
 {
     int ret = 0;
@@ -1412,6 +1520,9 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
         }
         ret = 0;
         break;
+    case SBI_EXT_DBCN:
+        kvm_riscv_handle_sbi_dbcn(cs, run);
+        break;
     default:
         qemu_log_mask(LOG_UNIMP,
                       "%s: un-handled SBI EXIT, specific reasons is %lu\n",
diff --git a/target/riscv/sbi_ecall_interface.h b/target/riscv/sbi_ecall_interface.h
index 43899d08f6..7dfe5f72c6 100644
--- a/target/riscv/sbi_ecall_interface.h
+++ b/target/riscv/sbi_ecall_interface.h
@@ -12,6 +12,17 @@
 
 /* clang-format off */
 
+#define SBI_SUCCESS                      0
+#define SBI_ERR_FAILED                  -1
+#define SBI_ERR_NOT_SUPPORTED           -2
+#define SBI_ERR_INVALID_PARAM           -3
+#define SBI_ERR_DENIED                  -4
+#define SBI_ERR_INVALID_ADDRESS         -5
+#define SBI_ERR_ALREADY_AVAILABLE       -6
+#define SBI_ERR_ALREADY_STARTED         -7
+#define SBI_ERR_ALREADY_STOPPED         -8
+#define SBI_ERR_NO_SHMEM                -9
+
 /* SBI Extension IDs */
 #define SBI_EXT_0_1_SET_TIMER           0x0
 #define SBI_EXT_0_1_CONSOLE_PUTCHAR     0x1
@@ -27,6 +38,7 @@
 #define SBI_EXT_IPI                     0x735049
 #define SBI_EXT_RFENCE                  0x52464E43
 #define SBI_EXT_HSM                     0x48534D
+#define SBI_EXT_DBCN                    0x4442434E
 
 /* SBI function IDs for BASE extension */
 #define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
@@ -57,6 +69,11 @@
 #define SBI_EXT_HSM_HART_STOP           0x1
 #define SBI_EXT_HSM_HART_GET_STATUS     0x2
 
+/* SBI function IDs for DBCN extension */
+#define SBI_EXT_DBCN_CONSOLE_WRITE      0x0
+#define SBI_EXT_DBCN_CONSOLE_READ       0x1
+#define SBI_EXT_DBCN_CONSOLE_WRITE_BYTE 0x2
+
 #define SBI_HSM_HART_STATUS_STARTED     0x0
 #define SBI_HSM_HART_STATUS_STOPPED     0x1
 #define SBI_HSM_HART_STATUS_START_PENDING   0x2
-- 
2.44.0

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Andrew Jones]

On Thu, Apr 25, 2024 at 12:50:12PM GMT, Daniel Henrique Barboza wrote:
> SBI defines a Debug Console extension "DBCN" that will, in time, replace
> the legacy console putchar and getchar SBI extensions.
> 
> The appeal of the DBCN extension is that it allows multiple bytes to be
> read/written in the SBI console in a single SBI call.
> 
> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
> module to userspace. But this will only happens if the KVM module
> actually supports this SBI extension and we activate it.
> 
> We'll check for DBCN support during init time, checking if get-reg-list
> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
> kvm_set_one_reg() during kvm_arch_init_vcpu().
> 
> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
> SBI_EXT_DBCN, reading and writing as required.
> 
> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
> host, takes around 20 seconds to boot without using DBCN. With this
> patch we're taking around 14 seconds to boot due to the speed-up in the
> terminal output.  There's no change in boot time if the guest isn't
> using earlycon.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
>  target/riscv/sbi_ecall_interface.h |  17 +++++
>  2 files changed, 128 insertions(+)
>

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

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Alistair Francis]

On Fri, Apr 26, 2024 at 1:51 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> SBI defines a Debug Console extension "DBCN" that will, in time, replace
> the legacy console putchar and getchar SBI extensions.
>
> The appeal of the DBCN extension is that it allows multiple bytes to be
> read/written in the SBI console in a single SBI call.
>
> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
> module to userspace. But this will only happens if the KVM module
> actually supports this SBI extension and we activate it.
>
> We'll check for DBCN support during init time, checking if get-reg-list
> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
> kvm_set_one_reg() during kvm_arch_init_vcpu().
>
> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
> SBI_EXT_DBCN, reading and writing as required.
>
> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
> host, takes around 20 seconds to boot without using DBCN. With this
> patch we're taking around 14 seconds to boot due to the speed-up in the
> terminal output.  There's no change in boot time if the guest isn't
> using earlycon.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
>  target/riscv/sbi_ecall_interface.h |  17 +++++
>  2 files changed, 128 insertions(+)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 03e3fee607..54a9ab9fd7 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -409,6 +409,12 @@ static KVMCPUConfig kvm_v_vlenb = {
>                     KVM_REG_RISCV_VECTOR_CSR_REG(vlenb)
>  };
>
> +static KVMCPUConfig kvm_sbi_dbcn = {
> +    .name = "sbi_dbcn",
> +    .kvm_reg_id = KVM_REG_RISCV | KVM_REG_SIZE_U64 |
> +                  KVM_REG_RISCV_SBI_EXT | KVM_RISCV_SBI_EXT_DBCN
> +};
> +
>  static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>  {
>      CPURISCVState *env = &cpu->env;
> @@ -1041,6 +1047,20 @@ static int uint64_cmp(const void *a, const void *b)
>      return 0;
>  }
>
> +static void kvm_riscv_check_sbi_dbcn_support(RISCVCPU *cpu,
> +                                             KVMScratchCPU *kvmcpu,
> +                                             struct kvm_reg_list *reglist)
> +{
> +    struct kvm_reg_list *reg_search;
> +
> +    reg_search = bsearch(&kvm_sbi_dbcn.kvm_reg_id, reglist->reg, reglist->n,
> +                         sizeof(uint64_t), uint64_cmp);
> +
> +    if (reg_search) {
> +        kvm_sbi_dbcn.supported = true;
> +    }
> +}
> +
>  static void kvm_riscv_read_vlenb(RISCVCPU *cpu, KVMScratchCPU *kvmcpu,
>                                   struct kvm_reg_list *reglist)
>  {
> @@ -1146,6 +1166,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>      if (riscv_has_ext(&cpu->env, RVV)) {
>          kvm_riscv_read_vlenb(cpu, kvmcpu, reglist);
>      }
> +
> +    kvm_riscv_check_sbi_dbcn_support(cpu, kvmcpu, reglist);
>  }
>
>  static void riscv_init_kvm_registers(Object *cpu_obj)
> @@ -1320,6 +1342,17 @@ static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
>      return ret;
>  }
>
> +static int kvm_vcpu_enable_sbi_dbcn(RISCVCPU *cpu, CPUState *cs)
> +{
> +    target_ulong reg = 1;
> +
> +    if (!kvm_sbi_dbcn.supported) {
> +        return 0;
> +    }
> +
> +    return kvm_set_one_reg(cs, kvm_sbi_dbcn.kvm_reg_id, &reg);
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      int ret = 0;
> @@ -1337,6 +1370,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      kvm_riscv_update_cpu_misa_ext(cpu, cs);
>      kvm_riscv_update_cpu_cfg_isa_ext(cpu, cs);
>
> +    ret = kvm_vcpu_enable_sbi_dbcn(cpu, cs);
> +
>      return ret;
>  }
>
> @@ -1394,6 +1429,79 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
>      return true;
>  }
>
> +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
> +{
> +    g_autofree uint8_t *buf = NULL;
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +    target_ulong num_bytes;
> +    uint64_t addr;
> +    unsigned char ch;
> +    int ret;
> +
> +    switch (run->riscv_sbi.function_id) {
> +    case SBI_EXT_DBCN_CONSOLE_READ:
> +    case SBI_EXT_DBCN_CONSOLE_WRITE:
> +        num_bytes = run->riscv_sbi.args[0];
> +
> +        if (num_bytes == 0) {
> +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
> +            run->riscv_sbi.ret[1] = 0;
> +            break;
> +        }
> +
> +        addr = run->riscv_sbi.args[1];
> +
> +        /*
> +         * Handle the case where a 32 bit CPU is running in a
> +         * 64 bit addressing env.
> +         */
> +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
> +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
> +        }
> +
> +        buf = g_malloc0(num_bytes);
> +
> +        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
> +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, num_bytes);
> +            if (ret < 0) {
> +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
> +                             "reading chardev");
> +                exit(1);
> +            }
> +
> +            cpu_physical_memory_write(addr, buf, ret);
> +        } else {
> +            cpu_physical_memory_read(addr, buf, num_bytes);
> +
> +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, num_bytes);
> +            if (ret < 0) {
> +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
> +                             "writing chardev");
> +                exit(1);
> +            }
> +        }
> +
> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
> +        run->riscv_sbi.ret[1] = ret;
> +        break;
> +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> +        ch = run->riscv_sbi.args[0];
> +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
> +
> +        if (ret < 0) {
> +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
> +                         "writing chardev");
> +            exit(1);
> +        }
> +
> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
> +        run->riscv_sbi.ret[1] = 0;
> +        break;
> +    default:
> +        run->riscv_sbi.ret[0] = SBI_ERR_NOT_SUPPORTED;
> +    }
> +}
> +
>  static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>  {
>      int ret = 0;
> @@ -1412,6 +1520,9 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>          }
>          ret = 0;
>          break;
> +    case SBI_EXT_DBCN:
> +        kvm_riscv_handle_sbi_dbcn(cs, run);
> +        break;
>      default:
>          qemu_log_mask(LOG_UNIMP,
>                        "%s: un-handled SBI EXIT, specific reasons is %lu\n",
> diff --git a/target/riscv/sbi_ecall_interface.h b/target/riscv/sbi_ecall_interface.h
> index 43899d08f6..7dfe5f72c6 100644
> --- a/target/riscv/sbi_ecall_interface.h
> +++ b/target/riscv/sbi_ecall_interface.h
> @@ -12,6 +12,17 @@
>
>  /* clang-format off */
>
> +#define SBI_SUCCESS                      0
> +#define SBI_ERR_FAILED                  -1
> +#define SBI_ERR_NOT_SUPPORTED           -2
> +#define SBI_ERR_INVALID_PARAM           -3
> +#define SBI_ERR_DENIED                  -4
> +#define SBI_ERR_INVALID_ADDRESS         -5
> +#define SBI_ERR_ALREADY_AVAILABLE       -6
> +#define SBI_ERR_ALREADY_STARTED         -7
> +#define SBI_ERR_ALREADY_STOPPED         -8
> +#define SBI_ERR_NO_SHMEM                -9
> +
>  /* SBI Extension IDs */
>  #define SBI_EXT_0_1_SET_TIMER           0x0
>  #define SBI_EXT_0_1_CONSOLE_PUTCHAR     0x1
> @@ -27,6 +38,7 @@
>  #define SBI_EXT_IPI                     0x735049
>  #define SBI_EXT_RFENCE                  0x52464E43
>  #define SBI_EXT_HSM                     0x48534D
> +#define SBI_EXT_DBCN                    0x4442434E
>
>  /* SBI function IDs for BASE extension */
>  #define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
> @@ -57,6 +69,11 @@
>  #define SBI_EXT_HSM_HART_STOP           0x1
>  #define SBI_EXT_HSM_HART_GET_STATUS     0x2
>
> +/* SBI function IDs for DBCN extension */
> +#define SBI_EXT_DBCN_CONSOLE_WRITE      0x0
> +#define SBI_EXT_DBCN_CONSOLE_READ       0x1
> +#define SBI_EXT_DBCN_CONSOLE_WRITE_BYTE 0x2
> +
>  #define SBI_HSM_HART_STATUS_STARTED     0x0
>  #define SBI_HSM_HART_STATUS_STOPPED     0x1
>  #define SBI_HSM_HART_STATUS_START_PENDING   0x2
> --
> 2.44.0
>
>

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Philippe Mathieu-Daudé]

Hi Daniel,

(now merged as commit a6b53378f537)

On 25/4/24 17:50, Daniel Henrique Barboza wrote:
> SBI defines a Debug Console extension "DBCN" that will, in time, replace
> the legacy console putchar and getchar SBI extensions.
> 
> The appeal of the DBCN extension is that it allows multiple bytes to be
> read/written in the SBI console in a single SBI call.
> 
> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
> module to userspace. But this will only happens if the KVM module
> actually supports this SBI extension and we activate it.
> 
> We'll check for DBCN support during init time, checking if get-reg-list
> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
> kvm_set_one_reg() during kvm_arch_init_vcpu().
> 
> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
> SBI_EXT_DBCN, reading and writing as required.
> 
> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
> host, takes around 20 seconds to boot without using DBCN. With this
> patch we're taking around 14 seconds to boot due to the speed-up in the
> terminal output.  There's no change in boot time if the guest isn't
> using earlycon.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
>   target/riscv/sbi_ecall_interface.h |  17 +++++
>   2 files changed, 128 insertions(+)


> +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
> +{
> +    g_autofree uint8_t *buf = NULL;
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +    target_ulong num_bytes;
> +    uint64_t addr;
> +    unsigned char ch;
> +    int ret;
> +
> +    switch (run->riscv_sbi.function_id) {
> +    case SBI_EXT_DBCN_CONSOLE_READ:
> +    case SBI_EXT_DBCN_CONSOLE_WRITE:
> +        num_bytes = run->riscv_sbi.args[0];
> +
> +        if (num_bytes == 0) {
> +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
> +            run->riscv_sbi.ret[1] = 0;
> +            break;
> +        }
> +
> +        addr = run->riscv_sbi.args[1];
> +
> +        /*
> +         * Handle the case where a 32 bit CPU is running in a
> +         * 64 bit addressing env.
> +         */
> +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
> +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
> +        }
> +
> +        buf = g_malloc0(num_bytes);
> +
> +        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
> +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, num_bytes);
> +            if (ret < 0) {
> +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
> +                             "reading chardev");
> +                exit(1);
> +            }
> +
> +            cpu_physical_memory_write(addr, buf, ret);
> +        } else {
> +            cpu_physical_memory_read(addr, buf, num_bytes);
> +
> +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, num_bytes);
> +            if (ret < 0) {
> +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
> +                             "writing chardev");
> +                exit(1);
> +            }
> +        }
> +
> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
> +        run->riscv_sbi.ret[1] = ret;
> +        break;
> +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> +        ch = run->riscv_sbi.args[0];
> +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
> +
> +        if (ret < 0) {
> +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
> +                         "writing chardev");
> +            exit(1);
> +        }

We are ignoring partial writes (non-blocking call returning 0 byte
written), is that expected? If so, is it OK to add a comment we can
safely discard not-yet-written DBCN_CONSOLE_WRITE_BYTE?

> +
> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
> +        run->riscv_sbi.ret[1] = 0;
> +        break;
> +    default:
> +        run->riscv_sbi.ret[0] = SBI_ERR_NOT_SUPPORTED;
> +    }
> +}

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Daniel Henrique Barboza]

On 6/3/25 10:19 AM, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
> 
> (now merged as commit a6b53378f537)
> 
> On 25/4/24 17:50, Daniel Henrique Barboza wrote:
>> SBI defines a Debug Console extension "DBCN" that will, in time, replace
>> the legacy console putchar and getchar SBI extensions.
>>
>> The appeal of the DBCN extension is that it allows multiple bytes to be
>> read/written in the SBI console in a single SBI call.
>>
>> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
>> module to userspace. But this will only happens if the KVM module
>> actually supports this SBI extension and we activate it.
>>
>> We'll check for DBCN support during init time, checking if get-reg-list
>> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
>> kvm_set_one_reg() during kvm_arch_init_vcpu().
>>
>> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
>> SBI_EXT_DBCN, reading and writing as required.
>>
>> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
>> host, takes around 20 seconds to boot without using DBCN. With this
>> patch we're taking around 14 seconds to boot due to the speed-up in the
>> terminal output.  There's no change in boot time if the guest isn't
>> using earlycon.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
>>   target/riscv/sbi_ecall_interface.h |  17 +++++
>>   2 files changed, 128 insertions(+)
> 
> 
>> +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
>> +{
>> +    g_autofree uint8_t *buf = NULL;
>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>> +    target_ulong num_bytes;
>> +    uint64_t addr;
>> +    unsigned char ch;
>> +    int ret;
>> +
>> +    switch (run->riscv_sbi.function_id) {
>> +    case SBI_EXT_DBCN_CONSOLE_READ:
>> +    case SBI_EXT_DBCN_CONSOLE_WRITE:
>> +        num_bytes = run->riscv_sbi.args[0];
>> +
>> +        if (num_bytes == 0) {
>> +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
>> +            run->riscv_sbi.ret[1] = 0;
>> +            break;
>> +        }
>> +
>> +        addr = run->riscv_sbi.args[1];
>> +
>> +        /*
>> +         * Handle the case where a 32 bit CPU is running in a
>> +         * 64 bit addressing env.
>> +         */
>> +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
>> +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
>> +        }
>> +
>> +        buf = g_malloc0(num_bytes);
>> +
>> +        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
>> +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, num_bytes);
>> +            if (ret < 0) {
>> +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
>> +                             "reading chardev");
>> +                exit(1);
>> +            }
>> +
>> +            cpu_physical_memory_write(addr, buf, ret);
>> +        } else {
>> +            cpu_physical_memory_read(addr, buf, num_bytes);
>> +
>> +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, num_bytes);
>> +            if (ret < 0) {
>> +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
>> +                             "writing chardev");
>> +                exit(1);
>> +            }
>> +        }
>> +
>> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
>> +        run->riscv_sbi.ret[1] = ret;
>> +        break;
>> +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
>> +        ch = run->riscv_sbi.args[0];
>> +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
>> +
>> +        if (ret < 0) {
>> +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
>> +                         "writing chardev");
>> +            exit(1);
>> +        }
> 
> We are ignoring partial writes (non-blocking call returning 0 byte
> written), is that expected? If so, is it OK to add a comment we can
> safely discard not-yet-written DBCN_CONSOLE_WRITE_BYTE?

Not sure what you meant. IIUC qemu_chr_fe_write() returns the number
of bytes consumed, 0 if no chardev is found, and -1 on error. Are you
saying that we should do a loop when there's no chardev found (ret = 0)
and wait a certain time until there's one available?


In fact, seeing how SBI_EXT_DBCN_CONSOLE_WRITE is written, I wonder if
we could use qemu_chr_fe_write_all() in this case too. Thanks,


Daniel



> 
>> +
>> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
>> +        run->riscv_sbi.ret[1] = 0;
>> +        break;
>> +    default:
>> +        run->riscv_sbi.ret[0] = SBI_ERR_NOT_SUPPORTED;
>> +    }
>> +}

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Philippe Mathieu-Daudé]

On 3/6/25 20:04, Daniel Henrique Barboza wrote:
> 
> 
> On 6/3/25 10:19 AM, Philippe Mathieu-Daudé wrote:
>> Hi Daniel,
>>
>> (now merged as commit a6b53378f537)
>>
>> On 25/4/24 17:50, Daniel Henrique Barboza wrote:
>>> SBI defines a Debug Console extension "DBCN" that will, in time, replace
>>> the legacy console putchar and getchar SBI extensions.
>>>
>>> The appeal of the DBCN extension is that it allows multiple bytes to be
>>> read/written in the SBI console in a single SBI call.
>>>
>>> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
>>> module to userspace. But this will only happens if the KVM module
>>> actually supports this SBI extension and we activate it.
>>>
>>> We'll check for DBCN support during init time, checking if get-reg-list
>>> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
>>> kvm_set_one_reg() during kvm_arch_init_vcpu().
>>>
>>> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
>>> SBI_EXT_DBCN, reading and writing as required.
>>>
>>> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
>>> host, takes around 20 seconds to boot without using DBCN. With this
>>> patch we're taking around 14 seconds to boot due to the speed-up in the
>>> terminal output.  There's no change in boot time if the guest isn't
>>> using earlycon.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
>>>   target/riscv/sbi_ecall_interface.h |  17 +++++
>>>   2 files changed, 128 insertions(+)
>>
>>
>>> +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run 
>>> *run)
>>> +{
>>> +    g_autofree uint8_t *buf = NULL;
>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>> +    target_ulong num_bytes;
>>> +    uint64_t addr;
>>> +    unsigned char ch;
>>> +    int ret;
>>> +
>>> +    switch (run->riscv_sbi.function_id) {
>>> +    case SBI_EXT_DBCN_CONSOLE_READ:
>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE:
>>> +        num_bytes = run->riscv_sbi.args[0];
>>> +
>>> +        if (num_bytes == 0) {
>>> +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>> +            run->riscv_sbi.ret[1] = 0;
>>> +            break;
>>> +        }
>>> +
>>> +        addr = run->riscv_sbi.args[1];
>>> +
>>> +        /*
>>> +         * Handle the case where a 32 bit CPU is running in a
>>> +         * 64 bit addressing env.
>>> +         */
>>> +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
>>> +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
>>> +        }
>>> +
>>> +        buf = g_malloc0(num_bytes);
>>> +
>>> +        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
>>> +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, 
>>> num_bytes);
>>> +            if (ret < 0) {
>>> +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
>>> +                             "reading chardev");
>>> +                exit(1);
>>> +            }
>>> +
>>> +            cpu_physical_memory_write(addr, buf, ret);
>>> +        } else {
>>> +            cpu_physical_memory_read(addr, buf, num_bytes);
>>> +
>>> +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, 
>>> num_bytes);
>>> +            if (ret < 0) {
>>> +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
>>> +                             "writing chardev");
>>> +                exit(1);
>>> +            }
>>> +        }
>>> +
>>> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>> +        run->riscv_sbi.ret[1] = ret;
>>> +        break;
>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
>>> +        ch = run->riscv_sbi.args[0];
>>> +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
>>> +
>>> +        if (ret < 0) {
>>> +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
>>> +                         "writing chardev");
>>> +            exit(1);
>>> +        }
>>
>> We are ignoring partial writes (non-blocking call returning 0 byte
>> written), is that expected? If so, is it OK to add a comment we can
>> safely discard not-yet-written DBCN_CONSOLE_WRITE_BYTE?
> 
> Not sure what you meant. IIUC qemu_chr_fe_write() returns the number
> of bytes consumed, 0 if no chardev is found, and -1 on error.

I'm trying to address an issue Peter reported with qemu_chr_fe_write():
https://lore.kernel.org/qemu-devel/CAFEAcA_kEndvNtw4EHySXWwQPoGs029yAzZGGBcV=zGHaj7KUQ@mail.gmail.com/

Basically upon introduction in commit cd18720a294 in 2013
("char: introduce a blocking version of qemu_chr_fe_write") the API
contract was "Returns: the number of bytes consumed" which could be 0,
so some frontends return 0 for "wrote no bytes".

Later in 2016 in commit fa394ed6257 ("char: make some qemu_chr_fe
skip if no driver") the API documentation was changed:

- * Returns: the number of bytes consumed
+ * Returns: the number of bytes consumed (0 if no assicated CharDriver)

After this commit, some frontends started to handle '<=0' as error,
while 0 is not an error.

> Are you
> saying that we should do a loop when there's no chardev found (ret = 0)
> and wait a certain time until there's one available?
> 
> 
> In fact, seeing how SBI_EXT_DBCN_CONSOLE_WRITE is written, I wonder if
> we could use qemu_chr_fe_write_all() in this case too.

This is certainly simpler.

Regards,

Phil.

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Daniel Henrique Barboza]

On 6/4/25 4:32 AM, Philippe Mathieu-Daudé wrote:
> On 3/6/25 20:04, Daniel Henrique Barboza wrote:
>>
>>
>> On 6/3/25 10:19 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Daniel,
>>>
>>> (now merged as commit a6b53378f537)
>>>
>>> On 25/4/24 17:50, Daniel Henrique Barboza wrote:
>>>> SBI defines a Debug Console extension "DBCN" that will, in time, replace
>>>> the legacy console putchar and getchar SBI extensions.
>>>>
>>>> The appeal of the DBCN extension is that it allows multiple bytes to be
>>>> read/written in the SBI console in a single SBI call.
>>>>
>>>> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
>>>> module to userspace. But this will only happens if the KVM module
>>>> actually supports this SBI extension and we activate it.
>>>>
>>>> We'll check for DBCN support during init time, checking if get-reg-list
>>>> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
>>>> kvm_set_one_reg() during kvm_arch_init_vcpu().
>>>>
>>>> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
>>>> SBI_EXT_DBCN, reading and writing as required.
>>>>
>>>> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
>>>> host, takes around 20 seconds to boot without using DBCN. With this
>>>> patch we're taking around 14 seconds to boot due to the speed-up in the
>>>> terminal output.  There's no change in boot time if the guest isn't
>>>> using earlycon.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> ---
>>>>   target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
>>>>   target/riscv/sbi_ecall_interface.h |  17 +++++
>>>>   2 files changed, 128 insertions(+)
>>>
>>>
>>>> +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
>>>> +{
>>>> +    g_autofree uint8_t *buf = NULL;
>>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>>> +    target_ulong num_bytes;
>>>> +    uint64_t addr;
>>>> +    unsigned char ch;
>>>> +    int ret;
>>>> +
>>>> +    switch (run->riscv_sbi.function_id) {
>>>> +    case SBI_EXT_DBCN_CONSOLE_READ:
>>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE:
>>>> +        num_bytes = run->riscv_sbi.args[0];
>>>> +
>>>> +        if (num_bytes == 0) {
>>>> +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>>> +            run->riscv_sbi.ret[1] = 0;
>>>> +            break;
>>>> +        }
>>>> +
>>>> +        addr = run->riscv_sbi.args[1];
>>>> +
>>>> +        /*
>>>> +         * Handle the case where a 32 bit CPU is running in a
>>>> +         * 64 bit addressing env.
>>>> +         */
>>>> +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
>>>> +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
>>>> +        }
>>>> +
>>>> +        buf = g_malloc0(num_bytes);
>>>> +
>>>> +        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
>>>> +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, num_bytes);
>>>> +            if (ret < 0) {
>>>> +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
>>>> +                             "reading chardev");
>>>> +                exit(1);
>>>> +            }
>>>> +
>>>> +            cpu_physical_memory_write(addr, buf, ret);
>>>> +        } else {
>>>> +            cpu_physical_memory_read(addr, buf, num_bytes);
>>>> +
>>>> +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, num_bytes);
>>>> +            if (ret < 0) {
>>>> +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
>>>> +                             "writing chardev");
>>>> +                exit(1);
>>>> +            }
>>>> +        }
>>>> +
>>>> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>>> +        run->riscv_sbi.ret[1] = ret;
>>>> +        break;
>>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
>>>> +        ch = run->riscv_sbi.args[0];
>>>> +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
>>>> +
>>>> +        if (ret < 0) {
>>>> +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
>>>> +                         "writing chardev");
>>>> +            exit(1);
>>>> +        }
>>>
>>> We are ignoring partial writes (non-blocking call returning 0 byte
>>> written), is that expected? If so, is it OK to add a comment we can
>>> safely discard not-yet-written DBCN_CONSOLE_WRITE_BYTE?
>>
>> Not sure what you meant. IIUC qemu_chr_fe_write() returns the number
>> of bytes consumed, 0 if no chardev is found, and -1 on error.
> 
> I'm trying to address an issue Peter reported with qemu_chr_fe_write():
> https://lore.kernel.org/qemu-devel/CAFEAcA_kEndvNtw4EHySXWwQPoGs029yAzZGGBcV=zGHaj7KUQ@mail.gmail.com/
> 
> Basically upon introduction in commit cd18720a294 in 2013
> ("char: introduce a blocking version of qemu_chr_fe_write") the API
> contract was "Returns: the number of bytes consumed" which could be 0,
> so some frontends return 0 for "wrote no bytes".
> 
> Later in 2016 in commit fa394ed6257 ("char: make some qemu_chr_fe
> skip if no driver") the API documentation was changed:
> 
> - * Returns: the number of bytes consumed
> + * Returns: the number of bytes consumed (0 if no assicated CharDriver)
> 
> After this commit, some frontends started to handle '<=0' as error,
> while 0 is not an error.

I think I got the gist of it, thanks.

For this particular console call the spec says:

"This is a blocking SBI call and it will only return after writing the specified
byte to the debug console. It will also return, with SBI_ERR_FAILED, if there are
I/O errors."


So I think it pairs well with the blocking version qemu_chr_fe_write_all()
instead. I can do this change and get out of your way in changing the callers
of qemu_chr_fe_write().

But I still have questions, hehe. This blocking version has the following
doc:

"(...) Unlike @qemu_chr_fe_write, this function will block if the back end
cannot consume all of the data attempted to be written. This function is
thread-safe.

Returns: the number of bytes consumed (0 if no associated Chardev)
or -1 on error."

Do we have plans to change this API like we're doing with the non-blocking
version? Because being a blocking call that promises "block until all bytes
are written", and I have len  > 0, I don't expect a ret = 0 to be interpret
as "no bytes were written".  I am ok with ret = 0 being 'no associated chardev'
and not handling it as an error (for now at least) but I would like to confirm
that qemu_chr_fe_write_all() will not interpret ret = 0 as a zero byte write.
In other words, if for some reason other than "no chardev present" we ended up
with zero bytes written I would like a ret < 0 return.


Thanks,

Daniel



> 
>> Are you
>> saying that we should do a loop when there's no chardev found (ret = 0)
>> and wait a certain time until there's one available?
>>
>>
>> In fact, seeing how SBI_EXT_DBCN_CONSOLE_WRITE is written, I wonder if
>> we could use qemu_chr_fe_write_all() in this case too.
> 
> This is certainly simpler.
> 
> Regards,
> 
> Phil.

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Philippe Mathieu-Daudé]

(+Marc-André and Paolo who I forgot to Cc first)

On 4/6/25 11:17, Daniel Henrique Barboza wrote:
> 
> 
> On 6/4/25 4:32 AM, Philippe Mathieu-Daudé wrote:
>> On 3/6/25 20:04, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 6/3/25 10:19 AM, Philippe Mathieu-Daudé wrote:
>>>> Hi Daniel,
>>>>
>>>> (now merged as commit a6b53378f537)
>>>>
>>>> On 25/4/24 17:50, Daniel Henrique Barboza wrote:
>>>>> SBI defines a Debug Console extension "DBCN" that will, in time, 
>>>>> replace
>>>>> the legacy console putchar and getchar SBI extensions.
>>>>>
>>>>> The appeal of the DBCN extension is that it allows multiple bytes 
>>>>> to be
>>>>> read/written in the SBI console in a single SBI call.
>>>>>
>>>>> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
>>>>> module to userspace. But this will only happens if the KVM module
>>>>> actually supports this SBI extension and we activate it.
>>>>>
>>>>> We'll check for DBCN support during init time, checking if get-reg- 
>>>>> list
>>>>> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable 
>>>>> it via
>>>>> kvm_set_one_reg() during kvm_arch_init_vcpu().
>>>>>
>>>>> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls 
>>>>> for
>>>>> SBI_EXT_DBCN, reading and writing as required.
>>>>>
>>>>> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
>>>>> host, takes around 20 seconds to boot without using DBCN. With this
>>>>> patch we're taking around 14 seconds to boot due to the speed-up in 
>>>>> the
>>>>> terminal output.  There's no change in boot time if the guest isn't
>>>>> using earlycon.
>>>>>
>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>> ---
>>>>>   target/riscv/kvm/kvm-cpu.c         | 111 ++++++++++++++++++++++++ 
>>>>> +++++
>>>>>   target/riscv/sbi_ecall_interface.h |  17 +++++
>>>>>   2 files changed, 128 insertions(+)
>>>>
>>>>
>>>>> +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run 
>>>>> *run)
>>>>> +{
>>>>> +    g_autofree uint8_t *buf = NULL;
>>>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>>>> +    target_ulong num_bytes;
>>>>> +    uint64_t addr;
>>>>> +    unsigned char ch;
>>>>> +    int ret;
>>>>> +
>>>>> +    switch (run->riscv_sbi.function_id) {
>>>>> +    case SBI_EXT_DBCN_CONSOLE_READ:
>>>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE:
>>>>> +        num_bytes = run->riscv_sbi.args[0];
>>>>> +
>>>>> +        if (num_bytes == 0) {
>>>>> +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>>>> +            run->riscv_sbi.ret[1] = 0;
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>> +        addr = run->riscv_sbi.args[1];
>>>>> +
>>>>> +        /*
>>>>> +         * Handle the case where a 32 bit CPU is running in a
>>>>> +         * 64 bit addressing env.
>>>>> +         */
>>>>> +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
>>>>> +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
>>>>> +        }
>>>>> +
>>>>> +        buf = g_malloc0(num_bytes);
>>>>> +
>>>>> +        if (run->riscv_sbi.function_id == 
>>>>> SBI_EXT_DBCN_CONSOLE_READ) {
>>>>> +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, 
>>>>> num_bytes);
>>>>> +            if (ret < 0) {
>>>>> +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
>>>>> +                             "reading chardev");
>>>>> +                exit(1);
>>>>> +            }
>>>>> +
>>>>> +            cpu_physical_memory_write(addr, buf, ret);
>>>>> +        } else {
>>>>> +            cpu_physical_memory_read(addr, buf, num_bytes);
>>>>> +
>>>>> +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, 
>>>>> num_bytes);
>>>>> +            if (ret < 0) {
>>>>> +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error 
>>>>> when "
>>>>> +                             "writing chardev");
>>>>> +                exit(1);
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>>>> +        run->riscv_sbi.ret[1] = ret;
>>>>> +        break;
>>>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
>>>>> +        ch = run->riscv_sbi.args[0];
>>>>> +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
>>>>> +
>>>>> +        if (ret < 0) {
>>>>> +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error 
>>>>> when "
>>>>> +                         "writing chardev");
>>>>> +            exit(1);
>>>>> +        }
>>>>
>>>> We are ignoring partial writes (non-blocking call returning 0 byte
>>>> written), is that expected? If so, is it OK to add a comment we can
>>>> safely discard not-yet-written DBCN_CONSOLE_WRITE_BYTE?
>>>
>>> Not sure what you meant. IIUC qemu_chr_fe_write() returns the number
>>> of bytes consumed, 0 if no chardev is found, and -1 on error.
>>
>> I'm trying to address an issue Peter reported with qemu_chr_fe_write():
>> https://lore.kernel.org/qemu-devel/ 
>> CAFEAcA_kEndvNtw4EHySXWwQPoGs029yAzZGGBcV=zGHaj7KUQ@mail.gmail.com/
>>
>> Basically upon introduction in commit cd18720a294 in 2013
>> ("char: introduce a blocking version of qemu_chr_fe_write") the API
>> contract was "Returns: the number of bytes consumed" which could be 0,
>> so some frontends return 0 for "wrote no bytes".
>>
>> Later in 2016 in commit fa394ed6257 ("char: make some qemu_chr_fe
>> skip if no driver") the API documentation was changed:
>>
>> - * Returns: the number of bytes consumed
>> + * Returns: the number of bytes consumed (0 if no assicated CharDriver)
>>
>> After this commit, some frontends started to handle '<=0' as error,
>> while 0 is not an error.
> 
> I think I got the gist of it, thanks.
> 
> For this particular console call the spec says:
> 
> "This is a blocking SBI call and it will only return after writing the 
> specified
> byte to the debug console. It will also return, with SBI_ERR_FAILED, if 
> there are
> I/O errors."
> 
> 
> So I think it pairs well with the blocking version qemu_chr_fe_write_all()
> instead. I can do this change and get out of your way in changing the 
> callers
> of qemu_chr_fe_write().

I appreciate if you post the patch (this is a 1 line change, but what
matters here is the justification you just provided), but if you are
busy I can do it, I have enough information to write the commit desc.

> 
> But I still have questions, hehe. This blocking version has the following
> doc:
> 
> "(...) Unlike @qemu_chr_fe_write, this function will block if the back end
> cannot consume all of the data attempted to be written. This function is
> thread-safe.
> 
> Returns: the number of bytes consumed (0 if no associated Chardev)
> or -1 on error."
> 
> Do we have plans to change this API like we're doing with the non-blocking
> version? Because being a blocking call that promises "block until all bytes
> are written", and I have len  > 0, I don't expect a ret = 0 to be interpret
> as "no bytes were written".  I am ok with ret = 0 being 'no associated 
> chardev'
> and not handling it as an error (for now at least) but I would like to 
> confirm
> that qemu_chr_fe_write_all() will not interpret ret = 0 as a zero byte 
> write.
> In other words, if for some reason other than "no chardev present" we 
> ended up
> with zero bytes written I would like a ret < 0 return.

Correct. Clarifying this method is in my TODO. What about this example:

- frontend wants to block to write 8 bytes
- backend writes 5 bytes, hits an unrecoverable link error

Should the backend support be responsible to re-establish link and retry
(when possible) to complete?

If we return -1 for error, could the frontend try to reconnect and write
the 8 bytes, ending with the first 5 bytes being transmitted twice?

Meanwhile I'm thinking to document as:

"Returns @len on success, or -1 on error (some data might has been 
written)".

Simpler API could be: "Returns 0 on success, otherwise -errno" but
we'd need to rework all the callers.

> 
> 
> Thanks,
> 
> Daniel
> 
> 
> 
>>
>>> Are you
>>> saying that we should do a loop when there's no chardev found (ret = 0)
>>> and wait a certain time until there's one available?
>>>
>>>
>>> In fact, seeing how SBI_EXT_DBCN_CONSOLE_WRITE is written, I wonder if
>>> we could use qemu_chr_fe_write_all() in this case too.
>>
>> This is certainly simpler.
>>
>> Regards,
>>
>> Phil.
> 

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Daniel P. Berrangé]

On Wed, Jun 04, 2025 at 09:32:21AM +0200, Philippe Mathieu-Daudé wrote:
> On 3/6/25 20:04, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 6/3/25 10:19 AM, Philippe Mathieu-Daudé wrote:
> > > Hi Daniel,
> > > 
> > > (now merged as commit a6b53378f537)
> > > 
> > > On 25/4/24 17:50, Daniel Henrique Barboza wrote:
> > > > SBI defines a Debug Console extension "DBCN" that will, in time, replace
> > > > the legacy console putchar and getchar SBI extensions.
> > > > 
> > > > The appeal of the DBCN extension is that it allows multiple bytes to be
> > > > read/written in the SBI console in a single SBI call.
> > > > 
> > > > As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
> > > > module to userspace. But this will only happens if the KVM module
> > > > actually supports this SBI extension and we activate it.
> > > > 
> > > > We'll check for DBCN support during init time, checking if get-reg-list
> > > > is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
> > > > kvm_set_one_reg() during kvm_arch_init_vcpu().
> > > > 
> > > > Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
> > > > SBI_EXT_DBCN, reading and writing as required.
> > > > 
> > > > A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
> > > > host, takes around 20 seconds to boot without using DBCN. With this
> > > > patch we're taking around 14 seconds to boot due to the speed-up in the
> > > > terminal output.  There's no change in boot time if the guest isn't
> > > > using earlycon.
> > > > 
> > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > > ---
> > > >   target/riscv/kvm/kvm-cpu.c         | 111 +++++++++++++++++++++++++++++
> > > >   target/riscv/sbi_ecall_interface.h |  17 +++++
> > > >   2 files changed, 128 insertions(+)
> > > 
> > > 
> > > > +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct
> > > > kvm_run *run)
> > > > +{
> > > > +    g_autofree uint8_t *buf = NULL;
> > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > +    target_ulong num_bytes;
> > > > +    uint64_t addr;
> > > > +    unsigned char ch;
> > > > +    int ret;
> > > > +
> > > > +    switch (run->riscv_sbi.function_id) {
> > > > +    case SBI_EXT_DBCN_CONSOLE_READ:
> > > > +    case SBI_EXT_DBCN_CONSOLE_WRITE:
> > > > +        num_bytes = run->riscv_sbi.args[0];
> > > > +
> > > > +        if (num_bytes == 0) {
> > > > +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
> > > > +            run->riscv_sbi.ret[1] = 0;
> > > > +            break;
> > > > +        }
> > > > +
> > > > +        addr = run->riscv_sbi.args[1];
> > > > +
> > > > +        /*
> > > > +         * Handle the case where a 32 bit CPU is running in a
> > > > +         * 64 bit addressing env.
> > > > +         */
> > > > +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
> > > > +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
> > > > +        }
> > > > +
> > > > +        buf = g_malloc0(num_bytes);
> > > > +
> > > > +        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
> > > > +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf,
> > > > num_bytes);
> > > > +            if (ret < 0) {
> > > > +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
> > > > +                             "reading chardev");
> > > > +                exit(1);
> > > > +            }
> > > > +
> > > > +            cpu_physical_memory_write(addr, buf, ret);
> > > > +        } else {
> > > > +            cpu_physical_memory_read(addr, buf, num_bytes);
> > > > +
> > > > +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf,
> > > > num_bytes);
> > > > +            if (ret < 0) {
> > > > +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
> > > > +                             "writing chardev");
> > > > +                exit(1);
> > > > +            }
> > > > +        }
> > > > +
> > > > +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
> > > > +        run->riscv_sbi.ret[1] = ret;
> > > > +        break;
> > > > +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> > > > +        ch = run->riscv_sbi.args[0];
> > > > +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
> > > > +
> > > > +        if (ret < 0) {
> > > > +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
> > > > +                         "writing chardev");
> > > > +            exit(1);
> > > > +        }
> > > 
> > > We are ignoring partial writes (non-blocking call returning 0 byte
> > > written), is that expected? If so, is it OK to add a comment we can
> > > safely discard not-yet-written DBCN_CONSOLE_WRITE_BYTE?
> > 
> > Not sure what you meant. IIUC qemu_chr_fe_write() returns the number
> > of bytes consumed, 0 if no chardev is found, and -1 on error.
> 
> I'm trying to address an issue Peter reported with qemu_chr_fe_write():
> https://lore.kernel.org/qemu-devel/CAFEAcA_kEndvNtw4EHySXWwQPoGs029yAzZGGBcV=zGHaj7KUQ@mail.gmail.com/
> 
> Basically upon introduction in commit cd18720a294 in 2013
> ("char: introduce a blocking version of qemu_chr_fe_write") the API
> contract was "Returns: the number of bytes consumed" which could be 0,
> so some frontends return 0 for "wrote no bytes".
> 
> Later in 2016 in commit fa394ed6257 ("char: make some qemu_chr_fe
> skip if no driver") the API documentation was changed:
> 
> - * Returns: the number of bytes consumed
> + * Returns: the number of bytes consumed (0 if no assicated CharDriver)

IMHO those semantics are broken for the write methods.

0 for a write() attempt indicates that the caller must be prepared to
retry again later.

0 for a write_all() attempt should not be permitted - write_all()
must always write everythnig, or return an error.

When no CharDriver is present, these semantics falls down. We need to
be returning "len" when no driver is present, ie no associated CharDriver
should be like writing to /dev/null, nor /dev/full - everything should be
reported as consumed, but discarded internally.

> After this commit, some frontends started to handle '<=0' as error,
> while 0 is not an error.

Yep, that's wrong.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] target/riscv/kvm: implement SBI debug console (DBCN) calls

[Daniel Henrique Barboza]

On 6/4/25 6:38 AM, Philippe Mathieu-Daudé wrote:
> (+Marc-André and Paolo who I forgot to Cc first)
> 
> On 4/6/25 11:17, Daniel Henrique Barboza wrote:
>>
>>
>> On 6/4/25 4:32 AM, Philippe Mathieu-Daudé wrote:
>>> On 3/6/25 20:04, Daniel Henrique Barboza wrote:
>>>>
>>>>
>>>> On 6/3/25 10:19 AM, Philippe Mathieu-Daudé wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> (now merged as commit a6b53378f537)
>>>>>
>>>>> On 25/4/24 17:50, Daniel Henrique Barboza wrote:
>>>>>> SBI defines a Debug Console extension "DBCN" that will, in time, replace
>>>>>> the legacy console putchar and getchar SBI extensions.
>>>>>>
>>>>>> The appeal of the DBCN extension is that it allows multiple bytes to be
>>>>>> read/written in the SBI console in a single SBI call.
>>>>>>
>>>>>> As far as KVM goes, the DBCN calls are forwarded by an in-kernel KVM
>>>>>> module to userspace. But this will only happens if the KVM module
>>>>>> actually supports this SBI extension and we activate it.
>>>>>>
>>>>>> We'll check for DBCN support during init time, checking if get-reg- list
>>>>>> is advertising KVM_RISCV_SBI_EXT_DBCN. In that case, we'll enable it via
>>>>>> kvm_set_one_reg() during kvm_arch_init_vcpu().
>>>>>>
>>>>>> Finally, change kvm_riscv_handle_sbi() to handle the incoming calls for
>>>>>> SBI_EXT_DBCN, reading and writing as required.
>>>>>>
>>>>>> A simple KVM guest with 'earlycon=sbi', running in an emulated RISC-V
>>>>>> host, takes around 20 seconds to boot without using DBCN. With this
>>>>>> patch we're taking around 14 seconds to boot due to the speed-up in the
>>>>>> terminal output.  There's no change in boot time if the guest isn't
>>>>>> using earlycon.
>>>>>>
>>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>>> ---
>>>>>>   target/riscv/kvm/kvm-cpu.c         | 111 ++++++++++++++++++++++++ +++++
>>>>>>   target/riscv/sbi_ecall_interface.h |  17 +++++
>>>>>>   2 files changed, 128 insertions(+)
>>>>>
>>>>>
>>>>>> +static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
>>>>>> +{
>>>>>> +    g_autofree uint8_t *buf = NULL;
>>>>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>>>>> +    target_ulong num_bytes;
>>>>>> +    uint64_t addr;
>>>>>> +    unsigned char ch;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    switch (run->riscv_sbi.function_id) {
>>>>>> +    case SBI_EXT_DBCN_CONSOLE_READ:
>>>>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE:
>>>>>> +        num_bytes = run->riscv_sbi.args[0];
>>>>>> +
>>>>>> +        if (num_bytes == 0) {
>>>>>> +            run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>>>>> +            run->riscv_sbi.ret[1] = 0;
>>>>>> +            break;
>>>>>> +        }
>>>>>> +
>>>>>> +        addr = run->riscv_sbi.args[1];
>>>>>> +
>>>>>> +        /*
>>>>>> +         * Handle the case where a 32 bit CPU is running in a
>>>>>> +         * 64 bit addressing env.
>>>>>> +         */
>>>>>> +        if (riscv_cpu_mxl(&cpu->env) == MXL_RV32) {
>>>>>> +            addr |= (uint64_t)run->riscv_sbi.args[2] << 32;
>>>>>> +        }
>>>>>> +
>>>>>> +        buf = g_malloc0(num_bytes);
>>>>>> +
>>>>>> +        if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
>>>>>> +            ret = qemu_chr_fe_read_all(serial_hd(0)->be, buf, num_bytes);
>>>>>> +            if (ret < 0) {
>>>>>> +                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
>>>>>> +                             "reading chardev");
>>>>>> +                exit(1);
>>>>>> +            }
>>>>>> +
>>>>>> +            cpu_physical_memory_write(addr, buf, ret);
>>>>>> +        } else {
>>>>>> +            cpu_physical_memory_read(addr, buf, num_bytes);
>>>>>> +
>>>>>> +            ret = qemu_chr_fe_write_all(serial_hd(0)->be, buf, num_bytes);
>>>>>> +            if (ret < 0) {
>>>>>> +                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
>>>>>> +                             "writing chardev");
>>>>>> +                exit(1);
>>>>>> +            }
>>>>>> +        }
>>>>>> +
>>>>>> +        run->riscv_sbi.ret[0] = SBI_SUCCESS;
>>>>>> +        run->riscv_sbi.ret[1] = ret;
>>>>>> +        break;
>>>>>> +    case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
>>>>>> +        ch = run->riscv_sbi.args[0];
>>>>>> +        ret = qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
>>>>>> +
>>>>>> +        if (ret < 0) {
>>>>>> +            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
>>>>>> +                         "writing chardev");
>>>>>> +            exit(1);
>>>>>> +        }
>>>>>
>>>>> We are ignoring partial writes (non-blocking call returning 0 byte
>>>>> written), is that expected? If so, is it OK to add a comment we can
>>>>> safely discard not-yet-written DBCN_CONSOLE_WRITE_BYTE?
>>>>
>>>> Not sure what you meant. IIUC qemu_chr_fe_write() returns the number
>>>> of bytes consumed, 0 if no chardev is found, and -1 on error.
>>>
>>> I'm trying to address an issue Peter reported with qemu_chr_fe_write():
>>> https://lore.kernel.org/qemu-devel/ CAFEAcA_kEndvNtw4EHySXWwQPoGs029yAzZGGBcV=zGHaj7KUQ@mail.gmail.com/
>>>
>>> Basically upon introduction in commit cd18720a294 in 2013
>>> ("char: introduce a blocking version of qemu_chr_fe_write") the API
>>> contract was "Returns: the number of bytes consumed" which could be 0,
>>> so some frontends return 0 for "wrote no bytes".
>>>
>>> Later in 2016 in commit fa394ed6257 ("char: make some qemu_chr_fe
>>> skip if no driver") the API documentation was changed:
>>>
>>> - * Returns: the number of bytes consumed
>>> + * Returns: the number of bytes consumed (0 if no assicated CharDriver)
>>>
>>> After this commit, some frontends started to handle '<=0' as error,
>>> while 0 is not an error.
>>
>> I think I got the gist of it, thanks.
>>
>> For this particular console call the spec says:
>>
>> "This is a blocking SBI call and it will only return after writing the specified
>> byte to the debug console. It will also return, with SBI_ERR_FAILED, if there are
>> I/O errors."
>>
>>
>> So I think it pairs well with the blocking version qemu_chr_fe_write_all()
>> instead. I can do this change and get out of your way in changing the callers
>> of qemu_chr_fe_write().
> 
> I appreciate if you post the patch (this is a 1 line change, but what
> matters here is the justification you just provided), but if you are
> busy I can do it, I have enough information to write the commit desc.

I can handle this change, don't worry about it. I'll send it out today.

> 
>>
>> But I still have questions, hehe. This blocking version has the following
>> doc:
>>
>> "(...) Unlike @qemu_chr_fe_write, this function will block if the back end
>> cannot consume all of the data attempted to be written. This function is
>> thread-safe.
>>
>> Returns: the number of bytes consumed (0 if no associated Chardev)
>> or -1 on error."
>>
>> Do we have plans to change this API like we're doing with the non-blocking
>> version? Because being a blocking call that promises "block until all bytes
>> are written", and I have len  > 0, I don't expect a ret = 0 to be interpret
>> as "no bytes were written".  I am ok with ret = 0 being 'no associated chardev'
>> and not handling it as an error (for now at least) but I would like to confirm
>> that qemu_chr_fe_write_all() will not interpret ret = 0 as a zero byte write.
>> In other words, if for some reason other than "no chardev present" we ended up
>> with zero bytes written I would like a ret < 0 return.
> 
> Correct. Clarifying this method is in my TODO. What about this example:
> 
> - frontend wants to block to write 8 bytes
> - backend writes 5 bytes, hits an unrecoverable link error
> 
> Should the backend support be responsible to re-establish link and retry
> (when possible) to complete?
> 
> If we return -1 for error, could the frontend try to reconnect and write
> the 8 bytes, ending with the first 5 bytes being transmitted twice?
> 
> Meanwhile I'm thinking to document as:
> 
> "Returns @len on success, or -1 on error (some data might has been written)".
> 

This make sense to me. It also goes in line with what Daniel mentioned in
his reply. I'll make the change with this semantic in mind.


Thanks,

Daniel




> Simpler API could be: "Returns 0 on success, otherwise -errno" but
> we'd need to rework all the callers.
> 
>>
>>
>> Thanks,
>>
>> Daniel
>>
>>
>>
>>>
>>>> Are you
>>>> saying that we should do a loop when there's no chardev found (ret = 0)
>>>> and wait a certain time until there's one available?
>>>>
>>>>
>>>> In fact, seeing how SBI_EXT_DBCN_CONSOLE_WRITE is written, I wonder if
>>>> we could use qemu_chr_fe_write_all() in this case too.
>>>
>>> This is certainly simpler.
>>>
>>> Regards,
>>>
>>> Phil.
>>
>