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