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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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;
> +    }
> +}