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

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

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.