| Message ID | 20240917115433.38503-1-alexei.filippov@syntacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v9] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI | expand |
On Tue, Sep 17, 2024 at 02:54:33PM 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") Fixes tag goes above s-o-b and 8 hex digits is a bit small. Most commit references in QEMU are using 10 or 12 digits. > --- > 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 f6e3156b8d..9f2ca67c9f 100644 > --- a/target/riscv/kvm/kvm-cpu.c > +++ b/target/riscv/kvm/kvm-cpu.c > @@ -1517,19 +1517,21 @@ 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; Looks good! > 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; This, along with the addition of the SBI_* defines below, should be a separate patch. If we were just naming the -1, then I wouldn't mind it slipping in with the same patch, but this is changing behavior since SBI_ERR_NOT_SUPPORTED is -2. I agree with the change, though, it just needs to be a separate patch. And the separate patch should have the same Fixes tag. Thanks, drew > 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, Sep 17, 2024 at 02:29:41PM GMT, Andrew Jones wrote: > On Tue, Sep 17, 2024 at 02:54:33PM 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") > > Fixes tag goes above s-o-b and 8 hex digits is a bit small. Most > commit references in QEMU are using 10 or 12 digits. > > > --- > > 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 f6e3156b8d..9f2ca67c9f 100644 > > --- a/target/riscv/kvm/kvm-cpu.c > > +++ b/target/riscv/kvm/kvm-cpu.c > > @@ -1517,19 +1517,21 @@ 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; > > Looks good! > > > 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; > > This, along with the addition of the SBI_* defines below, should be a > separate patch. If we were just naming the -1, then I wouldn't mind it > slipping in with the same patch, but this is changing behavior since > SBI_ERR_NOT_SUPPORTED is -2. I agree with the change, though, it just > needs to be a separate patch. And the separate patch should have the > same Fixes tag. > Actually it's even more of a difference than s/-1/-2/ since we're no long aborting the SBI call, but returning non-supported instead. Thanks, drew
On 17.09.2024 16:10, Andrew Jones wrote: > On Tue, Sep 17, 2024 at 02:29:41PM GMT, Andrew Jones wrote: >> On Tue, Sep 17, 2024 at 02:54:33PM 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") >> >> Fixes tag goes above s-o-b and 8 hex digits is a bit small. Most >> commit references in QEMU are using 10 or 12 digits. >> >>> --- >>> 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 f6e3156b8d..9f2ca67c9f 100644 >>> --- a/target/riscv/kvm/kvm-cpu.c >>> +++ b/target/riscv/kvm/kvm-cpu.c >>> @@ -1517,19 +1517,21 @@ 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; >> >> Looks good! >> >>> 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; >> >> This, along with the addition of the SBI_* defines below, should be a >> separate patch. If we were just naming the -1, then I wouldn't mind it >> slipping in with the same patch, but this is changing behavior since >> SBI_ERR_NOT_SUPPORTED is -2. I agree with the change, though, it just >> needs to be a separate patch. And the separate patch should have the >> same Fixes tag. >> > > Actually it's even more of a difference than s/-1/-2/ since we're no long > aborting the SBI call, but returning non-supported instead. I agreed, I'll split commits, but do you want me to resend patch as 2 patch series or 2 separate patches? > > Thanks, > drew
On Wed, Sep 18, 2024 at 04:37:12PM GMT, Aleksei Filippov wrote: > > > On 17.09.2024 16:10, Andrew Jones wrote: > > On Tue, Sep 17, 2024 at 02:29:41PM GMT, Andrew Jones wrote: > > > On Tue, Sep 17, 2024 at 02:54:33PM 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") > > > > > > Fixes tag goes above s-o-b and 8 hex digits is a bit small. Most > > > commit references in QEMU are using 10 or 12 digits. > > > > > > > --- > > > > 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 f6e3156b8d..9f2ca67c9f 100644 > > > > --- a/target/riscv/kvm/kvm-cpu.c > > > > +++ b/target/riscv/kvm/kvm-cpu.c > > > > @@ -1517,19 +1517,21 @@ 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; > > > > > > Looks good! > > > > > > > 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; > > > > > > This, along with the addition of the SBI_* defines below, should be a > > > separate patch. If we were just naming the -1, then I wouldn't mind it > > > slipping in with the same patch, but this is changing behavior since > > > SBI_ERR_NOT_SUPPORTED is -2. I agree with the change, though, it just > > > needs to be a separate patch. And the separate patch should have the > > > same Fixes tag. > > > > > > > Actually it's even more of a difference than s/-1/-2/ since we're no long > > aborting the SBI call, but returning non-supported instead. > > I agreed, I'll split commits, but do you want me to resend patch as 2 patch > series or 2 separate patches? Either way is fine by me. Thanks, drew
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c index f6e3156b8d..9f2ca67c9f 100644 --- a/target/riscv/kvm/kvm-cpu.c +++ b/target/riscv/kvm/kvm-cpu.c @@ -1517,19 +1517,21 @@ 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); 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
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") --- target/riscv/kvm/kvm-cpu.c | 10 ++++++---- target/riscv/sbi_ecall_interface.h | 12 ++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-)