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

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

[Alexei Filippov]

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>
---
 target/riscv/kvm/kvm-cpu.c         |  5 +++--
 target/riscv/sbi_ecall_interface.h | 11 +++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80..a4f84ad950 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1404,7 +1404,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
         if (ret == 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;
@@ -1412,7 +1412,8 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
         qemu_log_mask(LOG_UNIMP,
                       "%s: un-handled SBI EXIT, specific reasons is %lu\n",
                       __func__, run->riscv_sbi.extension_id);
-        ret = -1;
+        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 43899d08f6..0279e92a36 100644
--- a/target/riscv/sbi_ecall_interface.h
+++ b/target/riscv/sbi_ecall_interface.h
@@ -69,4 +69,15 @@
 #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
+
 #endif
-- 
2.34.1

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

[Daniel Henrique Barboza]

CCing Anup as well

On 3/25/24 07:14, 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.


Please add:

Fixes: 4eb47125 ("target/riscv: Handle KVM_EXIT_RISCV_SBI exit")

> 
> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   target/riscv/kvm/kvm-cpu.c         |  5 +++--
>   target/riscv/sbi_ecall_interface.h | 11 +++++++++++
>   2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 6a6c6cae80..a4f84ad950 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1404,7 +1404,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>           if (ret == 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;
> @@ -1412,7 +1412,8 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>           qemu_log_mask(LOG_UNIMP,
>                         "%s: un-handled SBI EXIT, specific reasons is %lu\n",
>                         __func__, run->riscv_sbi.extension_id);
> -        ret = -1;
> +        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 43899d08f6..0279e92a36 100644
> --- a/target/riscv/sbi_ecall_interface.h
> +++ b/target/riscv/sbi_ecall_interface.h
> @@ -69,4 +69,15 @@
>   #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
> +
>   #endif

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

[Alexei Filippov]

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")
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---

Changes since v1:
        -Add Fixes and Revied-by lines.
 target/riscv/kvm/kvm-cpu.c         |  5 +++--
 target/riscv/sbi_ecall_interface.h | 11 +++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80..a4f84ad950 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1404,7 +1404,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
         if (ret == 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;
@@ -1412,7 +1412,8 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
         qemu_log_mask(LOG_UNIMP,
                       "%s: un-handled SBI EXIT, specific reasons is %lu\n",
                       __func__, run->riscv_sbi.extension_id);
-        ret = -1;
+        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 43899d08f6..0279e92a36 100644
--- a/target/riscv/sbi_ecall_interface.h
+++ b/target/riscv/sbi_ecall_interface.h
@@ -69,4 +69,15 @@
 #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
+
 #endif
-- 
2.34.1

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

[Alistair Francis]

On Mon, Mar 25, 2024 at 11:46 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")
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
> Changes since v1:
>         -Add Fixes and Revied-by lines.
>  target/riscv/kvm/kvm-cpu.c         |  5 +++--
>  target/riscv/sbi_ecall_interface.h | 11 +++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 6a6c6cae80..a4f84ad950 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1404,7 +1404,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>          if (ret == 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;
> @@ -1412,7 +1412,8 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>          qemu_log_mask(LOG_UNIMP,
>                        "%s: un-handled SBI EXIT, specific reasons is %lu\n",
>                        __func__, run->riscv_sbi.extension_id);
> -        ret = -1;
> +        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 43899d08f6..0279e92a36 100644
> --- a/target/riscv/sbi_ecall_interface.h
> +++ b/target/riscv/sbi_ecall_interface.h
> @@ -69,4 +69,15 @@
>  #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
> +
>  #endif
> --
> 2.34.1
>
>

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

[Alexei Filippov]

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")
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---

Changes since v2:
        -Clear kvm_riscv_handle_sbi()
        -Add SBI_ERR_NO_SHMEM return code's define
 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 v2] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI

[Andrew Jones]

On Mon, Mar 25, 2024 at 04:01:16PM +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")
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> 
> Changes since v1:
>         -Add Fixes and Revied-by lines.
>  target/riscv/kvm/kvm-cpu.c         |  5 +++--
>  target/riscv/sbi_ecall_interface.h | 11 +++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 6a6c6cae80..a4f84ad950 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1404,7 +1404,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>          if (ret == 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;
> @@ -1412,7 +1412,8 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
>          qemu_log_mask(LOG_UNIMP,
>                        "%s: un-handled SBI EXIT, specific reasons is %lu\n",
>                        __func__, run->riscv_sbi.extension_id);

While changing this, can we also change this log to something like

 "%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;
> +        ret = 0;

We don't have any paths that set ret to anything other than zero now.
Let's return zero at the bottom of the function instead. And the top
of the function can then be cleaned up to

 unsigned char ch;
 int ret;

 switch (run->riscv_sbi.extension_id) {


>          break;
>      }
>      return ret;
> diff --git a/target/riscv/sbi_ecall_interface.h b/target/riscv/sbi_ecall_interface.h
> index 43899d08f6..0279e92a36 100644
> --- a/target/riscv/sbi_ecall_interface.h
> +++ b/target/riscv/sbi_ecall_interface.h
> @@ -69,4 +69,15 @@
>  #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

v2 of the spec has SBI_ERR_NO_SHMEM as well.

Thanks,
drew

> +
>  #endif
> -- 
> 2.34.1
> 
> 

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

[Alexei Filippov]

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

[Andrew Jones]

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>

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

[Alistair Francis]

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

[Andrew Jones]

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 v5] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific sbi.

[Alexei Filippov]

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.

[Aleksei Filippov]

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

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

[Alexei Filippov]

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

[Andrew Jones]

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

[Andrew Jones]

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
> > 

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

[Aleksei Filippov]

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.
-- 
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
>>>

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

[Andrew Jones]

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
> > > > 

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

[Alexei Filippov]

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 v6:
		- Add appropriate commit message.
		- Fix error handling according to Andrew Jones suggestion.
 target/riscv/kvm/kvm-cpu.c         | 11 +++++++----
 target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index f9dbc18a76..a84bcda9d9 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1173,16 +1173,19 @@ 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 {
-            run->riscv_sbi.ret[0] = -1;
+            if (ret == 0) {
+                run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
+            }
+            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 v7] target/riscv/kvm/kvm-cpu.c: kvm_riscv_handle_sbi() fail with vendor-specific SBI

[Andrew Jones]

On Mon, May 27, 2024 at 04:48:11PM 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 v6:
> 		- Add appropriate commit message.
> 		- Fix error handling according to Andrew Jones suggestion.
>  target/riscv/kvm/kvm-cpu.c         | 11 +++++++----
>  target/riscv/sbi_ecall_interface.h | 12 ++++++++++++
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index f9dbc18a76..a84bcda9d9 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1173,16 +1173,19 @@ 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;

ret is already zero here, so this assignment isn't necessary.

>          } else {
> -            run->riscv_sbi.ret[0] = -1;
> +            if (ret == 0) {
> +                run->riscv_sbi.ret[0] = SBI_ERR_FAILURE;
> +            }
> +            ret = -1;
>          }
> -        ret = 0;
>          break;

v6 was closer to being correct than this. It should be

@@ -1515,21 +1516,24 @@ 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 {
+        } else if (ret == 0) {
             run->riscv_sbi.ret[0] = -1;
+        } 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);
> -        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
>

Thanks,
drew

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

[Alexei Filippov]

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

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

[Andrew Jones]

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
> 

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

[Alexei Filippov]

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(-)

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
-- 
2.34.1

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

[Andrew Jones]

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
> 

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

[Andrew Jones]

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

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

[Aleksei Filippov]

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

-- 
Sincerely,
Aleksei Filippov

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

[Andrew Jones]

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