target/riscv/kvm/kvm-cpu.c | 9 +++++---- target/riscv/sbi_ecall_interface.h | 12 ++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-)
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), because 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 return an error.
Added SBI related return code's defines.
Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
Fixes: 4eb47125 ("target/riscv: Handle KVM_EXIT_RISCV_SBI exit")
---
Changes since v7:
- Fix error handling according to Andrew Jones suggestion.
target/riscv/kvm/kvm-cpu.c | 9 +++++----
target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 235e2cdaca..1afbabe19f 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1515,19 +1515,20 @@ 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;
+ } else if (ret == 0) {
+ run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
} else {
- run->riscv_sbi.ret[0] = -1;
+ ret = -1;
}
- 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",
+ "%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 7dfe5f72c6..4df0accd78 100644
--- a/target/riscv/sbi_ecall_interface.h
+++ b/target/riscv/sbi_ecall_interface.h
@@ -86,4 +86,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
On Tue, Jun 25, 2024 at 06:02:54PM GMT, 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), because 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 return an error. > > Added SBI related return code's defines. > > Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com> > Fixes: 4eb47125 ("target/riscv: Handle KVM_EXIT_RISCV_SBI exit") > --- > Changes since v7: > - Fix error handling according to Andrew Jones suggestion. > target/riscv/kvm/kvm-cpu.c | 9 +++++---- > target/riscv/sbi_ecall_interface.h | 12 ++++++++++++ > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c > index 235e2cdaca..1afbabe19f 100644 > --- a/target/riscv/kvm/kvm-cpu.c > +++ b/target/riscv/kvm/kvm-cpu.c > @@ -1515,19 +1515,20 @@ 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; > + } else if (ret == 0) { > + run->riscv_sbi.ret[0] = SBI_ERR_FAILURE; > } else { > - run->riscv_sbi.ret[0] = -1; > + ret = -1; > } > - ret = 0; It should be diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c index 235e2cdaca1a..9946afb4eade 100644 --- a/target/riscv/kvm/kvm-cpu.c +++ b/target/riscv/kvm/kvm-cpu.c @@ -1515,10 +1515,12 @@ 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; - } else { + ret = 0; + } else if (ret == 0) { run->riscv_sbi.ret[0] = -1; + } else { + ret = -1; } - ret = 0; break; case SBI_EXT_DBCN: kvm_riscv_handle_sbi_dbcn(cs, run); I misled you on that first 'ret = 0' addition, we need that, but I've pointed out a few times that we should use '-1' instead of SBI_ERR_FAILURE and why. Thanks, drew > 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", > + "%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 7dfe5f72c6..4df0accd78 100644 > --- a/target/riscv/sbi_ecall_interface.h > +++ b/target/riscv/sbi_ecall_interface.h > @@ -86,4 +86,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 >
© 2016 - 2024 Red Hat, Inc.