On 5/2/2025 11:40 am, Joel Stanley wrote:
On Fri, 29 Nov 2024 at 15:29, Cyril Bur <cyrilbur@xxxxxxxxxxxxxxx> wrote:
--- a/riscv/kvm-cpu.c
+++ b/riscv/kvm-cpu.c
@@ -178,15 +178,16 @@ static bool kvm_cpu_riscv_sbi(struct kvm_cpu *vcpu)
break;
}
vcpu->kvm_run->riscv_sbi.ret[1] = 0;
- while (str_start <= str_end) {
- if (vcpu->kvm_run->riscv_sbi.function_id ==
- SBI_EXT_DBCN_CONSOLE_WRITE) {
- term_putc(str_start, 1, 0);
- } else {
- if (!term_readable(0))
- break;
- *str_start = term_getc(vcpu->kvm, 0);
- }
+ if (vcpu->kvm_run->riscv_sbi.function_id ==
+ SBI_EXT_DBCN_CONSOLE_WRITE) {
+ int length = (str_end - str_start) + 1;
Much more readable than v1!
You could add a check that str_end > str_start where the if
(!str_start || !str_end) test is to avoid shenanigans.
Probably wise.
+
+ term_putc(str_start, length, 0);
+ vcpu->kvm_run->riscv_sbi.ret[1] += length;
term_putc returns the actual length written, should you be putting
that in ret[1] instead?
Yes I think this is a good idea.
+ break;
+ }
+ while (str_start <= str_end && term_readable(0)) {
Previously this would have only happened for function_id ==
SBI_EXT_DBCN_CONSOLE_READ. Should you add an else?
An else block would be redundant since the switch statement only lets
SBI_EXT_DBCN_CONSOLE_WRITE and SBI_EXT_DBCN_CONSOLE_READ in here. For
clarity a comment couldn't hurt though.
I will send a v3
Thanks for the review,
Cyril
+ *str_start = term_getc(vcpu->kvm, 0);
vcpu->kvm_run->riscv_sbi.ret[1]++;
str_start++;
}
--
2.34.1
