On Mon, Apr 22, 2024 at 02:31:36PM +0200, Andrew Jones wrote: > On Mon, Apr 22, 2024 at 02:42:54PM +0300, Alexei Filippov wrote: > > kvm_riscv_handle_sbi() may return not supported return code to not > > trigger qemu abort with vendor-specific sbi. > > > > Add new error path to provide proper error in case of > > qemu_chr_fe_read_all() may not return sizeof(ch). > > I think something more along the lines of what I wrote in my previous > reply will help clarify this more. Here's what I wrote > > """ > Exactly zero just means we failed to read input, which can happen, so > telling the SBI caller we failed to read, but telling the caller of this > function that we successfully emulated the SBI call, is correct. However, > anything else, other than sizeof(ch), means something unexpected happened, > so we should indeed return an error from this function. > """ > > Thanks, > drew > > > > > Added SBI related return code's defines. > > > > Signed-off-by: Alexei Filippov <alexei.filip...@syntacore.com> > > --- > > Changes since v4-5: > > -Added new error path in case of qemu_chr_fe_read_all() may not > > return sizeof(ch). > > -Added more comments in commit message. > > target/riscv/kvm/kvm-cpu.c | 10 ++++++---- > > target/riscv/sbi_ecall_interface.h | 12 ++++++++++++ > > 2 files changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c > > index f9dbc18a76..5bb7b74d03 100644 > > --- a/target/riscv/kvm/kvm-cpu.c > > +++ b/target/riscv/kvm/kvm-cpu.c > > @@ -1173,16 +1173,18 @@ static int kvm_riscv_handle_sbi(CPUState *cs, > > struct kvm_run *run) > > ret = qemu_chr_fe_read_all(serial_hd(0)->be, &ch, sizeof(ch)); > > if (ret == sizeof(ch)) { > > run->riscv_sbi.ret[0] = ch; > > + ret = 0; > > + } else if (ret == 0) { > > + run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
I'd prefer we still explicitly assign ret[0] to -1 here since that's what the spec explicitly says. Thanks, drew > > } else { > > - run->riscv_sbi.ret[0] = -1; > > + ret = -1; > > } > > - ret = 0; > > break; > > default: > > qemu_log_mask(LOG_UNIMP, > > - "%s: un-handled SBI EXIT, specific reasons is %lu\n", > > + "%s: Unhandled SBI exit with extension-id %lu\n" > > __func__, run->riscv_sbi.extension_id); > > - ret = -1; > > + run->riscv_sbi.ret[0] = SBI_ERR_NOT_SUPPORTED; > > break; > > } > > return ret; > > diff --git a/target/riscv/sbi_ecall_interface.h > > b/target/riscv/sbi_ecall_interface.h > > index 43899d08f6..a2e21d9b8c 100644 > > --- a/target/riscv/sbi_ecall_interface.h > > +++ b/target/riscv/sbi_ecall_interface.h > > @@ -69,4 +69,16 @@ > > #define SBI_EXT_VENDOR_END 0x09FFFFFF > > /* clang-format on */ > > > > +/* SBI return error codes */ > > +#define SBI_SUCCESS 0 > > +#define SBI_ERR_FAILURE -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 > > + > > #endif > > -- > > 2.34.1 > >