| Message ID | 20240422114254.13839-1-alexei.filippov@syntacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI | expand |
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.filippov@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; > } 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 >
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.filippov@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 > >
On 25.04.2024 12:21, Andrew Jones wrote: > 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.filippov@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 Can you please explain it a little bit more, cz I believe SBI_ERR_FAILURE is -1 anyway. Defines was added at first place just to came along with Linux kernel SBI related defines.
On Fri, May 03, 2024 at 01:39:32PM GMT, Aleksei Filippov wrote: > > > On 25.04.2024 12:21, Andrew Jones wrote: > > 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.filippov@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 > > Can you please explain it a little bit more, cz I believe SBI_ERR_FAILURE is > -1 anyway. Defines was added at first place just to came along with Linux > kernel SBI related defines. Legacy SBI calls like SBI_EXT_0_1_CONSOLE_GETCHAR don't return a struct sbiret, they only return a function-specific long. The spec says for Getchar that it returns "...the byte on success, or -1 for failure." So we should explicitly set failure to -1, especially since SBI_ERR_FAILURE isn't defined for legacy SBI calls. Thanks, drew > -- > Sincerely, > Aleksei Filippov > > > > > } 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 > > > >
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; } 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
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). Added SBI related return code's defines. Signed-off-by: Alexei Filippov <alexei.filippov@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(-)