1. Patchew
  2. QEMU
  3. View series

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

Alexei Filippov posted 1 patch 2 weeks, 2 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240413112526.8748-1-alexei.filippov@syntacore.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
There is a newer version of this series
target/riscv/kvm/kvm-cpu.c         | 13 +++++--------
target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
2 files changed, 17 insertions(+), 8 deletions(-)
[PATCH v4] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI
Posted by Alexei Filippov 2 weeks, 2 days ago 
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;
         }
-        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
Re: [PATCH v4] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI
Posted by Alistair Francis 1 week ago 
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
>
>
Re: [PATCH v4] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI
Posted by Andrew Jones 1 week ago 
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
> >
> >
[PATCH v6] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI
Posted by Alexei Filippov 1 week ago 
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(-)

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
Re: [PATCH v6] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI
Posted by Andrew Jones 1 week ago 
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
>
Re: [PATCH v6] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI
Posted by Andrew Jones 4 days, 8 hours ago 
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
> >
[PATCH v5] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific sbi.
Posted by Alexei Filippov 1 week ago 
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>
---
 target/riscv/kvm/kvm-cpu.c         | 9 +++++----
 target/riscv/sbi_ecall_interface.h | 1 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index aeca1e3e83..5bb7b74d03 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1173,17 +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;
-        } else {
+            ret = 0;
+        } else if (ret == 0) {
             run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
+        } else {
+            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);
         run->riscv_sbi.ret[0] = SBI_ERR_NOT_SUPPORTED;
-        ret = 0;
         break;
     }
     return ret;
diff --git a/target/riscv/sbi_ecall_interface.h b/target/riscv/sbi_ecall_interface.h
index 0279e92a36..a2e21d9b8c 100644
--- a/target/riscv/sbi_ecall_interface.h
+++ b/target/riscv/sbi_ecall_interface.h
@@ -79,5 +79,6 @@
 #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
Re: [PATCH v5] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific sbi.
Posted by Aleksei Filippov 1 week ago 

On 22.04.2024 14:24, 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).
> 
> Added SBI related return code's defines.
> 
> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
> ---
>   target/riscv/kvm/kvm-cpu.c         | 9 +++++----
>   target/riscv/sbi_ecall_interface.h | 1 +
>   2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index aeca1e3e83..5bb7b74d03 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1173,17 +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;
> -        } else {
> +            ret = 0;
> +        } else if (ret == 0) {
>               run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
> +        } else {
> +            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);
>           run->riscv_sbi.ret[0] = SBI_ERR_NOT_SUPPORTED;
> -        ret = 0;
>           break;
>       }
>       return ret;
> diff --git a/target/riscv/sbi_ecall_interface.h b/target/riscv/sbi_ecall_interface.h
> index 0279e92a36..a2e21d9b8c 100644
> --- a/target/riscv/sbi_ecall_interface.h
> +++ b/target/riscv/sbi_ecall_interface.h
> @@ -79,5 +79,6 @@
>   #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

Oh, my bad, wrong patch. Will resend properly.
-- 
Sincerely,
Aleksei Filippov
Re: [PATCH v4] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI
Posted by Andrew Jones 2 weeks ago 
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>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.