diff mbox series

[v4] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI

Message ID 20240413112526.8748-1-alexei.filippov@syntacore.com (mailing list archive)
State New
Headers show
Series [v4] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI | expand

Commit Message

Alexei Filippov April 13, 2024, 11:25 a.m. UTC 
kvm_riscv_handle_sbi() may return not supported return code to not trigger
qemu abort with vendor-specific sbi.

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 v3:
        -Clear Reviewed-by tags
 target/riscv/kvm/kvm-cpu.c         | 13 +++++--------
 target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
 2 files changed, 17 insertions(+), 8 deletions(-)

Comments

Andrew Jones April 15, 2024, 2:03 p.m. UTC | #1 
On Sat, Apr 13, 2024 at 02:25:26PM +0300, Alexei Filippov wrote:
> kvm_riscv_handle_sbi() may return not supported return code to not trigger
> qemu abort with vendor-specific sbi.
> 
> 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 v3:
>         -Clear Reviewed-by tags
>  target/riscv/kvm/kvm-cpu.c         | 13 +++++--------
>  target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
>  2 files changed, 17 insertions(+), 8 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Alistair Francis April 22, 2024, 3:55 a.m. UTC | #2 
On Sat, Apr 13, 2024 at 9:26 PM Alexei Filippov
<alexei.filippov@syntacore.com> wrote:
>
> kvm_riscv_handle_sbi() may return not supported return code to not trigger
> qemu abort with vendor-specific sbi.
>
> 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 v3:
>         -Clear Reviewed-by tags
>  target/riscv/kvm/kvm-cpu.c         | 13 +++++--------
>  target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
>  2 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 6a6c6cae80..844942d9ba 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1392,7 +1392,6 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
>
>  static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>  {
> -    int ret = 0;
>      unsigned char ch;
>      switch (run->riscv_sbi.extension_id) {
>      case SBI_EXT_0_1_CONSOLE_PUTCHAR:
> @@ -1400,22 +1399,20 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>          qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
>          break;
>      case SBI_EXT_0_1_CONSOLE_GETCHAR:
> -        ret = qemu_chr_fe_read_all(serial_hd(0)->be, &ch, sizeof(ch));
> -        if (ret == sizeof(ch)) {
> +        if (qemu_chr_fe_read_all(serial_hd(0)->be, &ch, sizeof(ch)) == sizeof(ch)) {
>              run->riscv_sbi.ret[0] = ch;
>          } else {
> -            run->riscv_sbi.ret[0] = -1;
> +            run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;

I'm not sure I follow. This seems like a failure but we report success
to the caller of this function?

Can you expand the commit message to explain why we want this change

Alistair

>          }
> -        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;
> +    return 0;
>  }
>
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> 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
>
>
Andrew Jones April 22, 2024, 8:12 a.m. UTC | #3 
On Mon, Apr 22, 2024 at 01:55:31PM +1000, Alistair Francis wrote:
> On Sat, Apr 13, 2024 at 9:26 PM Alexei Filippov
> <alexei.filippov@syntacore.com> wrote:
> >
> > kvm_riscv_handle_sbi() may return not supported return code to not trigger
> > qemu abort with vendor-specific sbi.
> >
> > 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 v3:
> >         -Clear Reviewed-by tags
> >  target/riscv/kvm/kvm-cpu.c         | 13 +++++--------
> >  target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
> >  2 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > index 6a6c6cae80..844942d9ba 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -1392,7 +1392,6 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
> >
> >  static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
> >  {
> > -    int ret = 0;
> >      unsigned char ch;
> >      switch (run->riscv_sbi.extension_id) {
> >      case SBI_EXT_0_1_CONSOLE_PUTCHAR:
> > @@ -1400,22 +1399,20 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
> >          qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
> >          break;
> >      case SBI_EXT_0_1_CONSOLE_GETCHAR:
> > -        ret = qemu_chr_fe_read_all(serial_hd(0)->be, &ch, sizeof(ch));
> > -        if (ret == sizeof(ch)) {
> > +        if (qemu_chr_fe_read_all(serial_hd(0)->be, &ch, sizeof(ch)) == sizeof(ch)) {
> >              run->riscv_sbi.ret[0] = ch;
> >          } else {
> > -            run->riscv_sbi.ret[0] = -1;
> > +            run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
> 
> I'm not sure I follow. This seems like a failure but we report success
> to the caller of this function?
> 
> Can you expand the commit message to explain why we want this change

Looking at this again, I think it would be more clear, and more correct,
if we only do the SBI_ERR_FAILURE path for a return value of exactly zero.

 ...
 ret = qemu_chr_fe_read_all(...);
 if (ret == sizeof(ch)) {
   run->riscv_sbi.ret[0] = ch;
   ret = 0;
 } else if (ret == 0) {
   run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
 }
 break;
 ...
 return ret;


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


> 
> Alistair
> 
> >          }
> > -        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;
> > +    return 0;
> >  }
> >
> >  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> > 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 mbox series

Patch

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80..844942d9ba 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1392,7 +1392,6 @@  bool kvm_arch_stop_on_emulation_error(CPUState *cs)
 
 static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
 {
-    int ret = 0;
     unsigned char ch;
     switch (run->riscv_sbi.extension_id) {
     case SBI_EXT_0_1_CONSOLE_PUTCHAR:
@@ -1400,22 +1399,20 @@  static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
         qemu_chr_fe_write(serial_hd(0)->be, &ch, sizeof(ch));
         break;
     case SBI_EXT_0_1_CONSOLE_GETCHAR:
-        ret = qemu_chr_fe_read_all(serial_hd(0)->be, &ch, sizeof(ch));
-        if (ret == sizeof(ch)) {
+        if (qemu_chr_fe_read_all(serial_hd(0)->be, &ch, sizeof(ch)) == sizeof(ch)) {
             run->riscv_sbi.ret[0] = ch;
         } else {
-            run->riscv_sbi.ret[0] = -1;
+            run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
         }
-        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;
+    return 0;
 }
 
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
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