Skip to contentSkip to conversation

target/riscv/kvm: Handle SBI console backend failures

PATCH Applied Zephyr Li 2 participants 3 messages Nov 9 – Nov 11 RISC-V

lore
Share & tools
lore

Zephyr Li

author
#0 lore raw

PATCH 1/1 target/riscv/kvm: Handle SBI console backend failures

The serial backend is optional, for example when QEMU is started with -serial none. The legacy SBI console and DBCN handlers nevertheless dereference serial_hd(0)->fe without checking whether the chardev or its frontend exists, causing QEMU to crash on a guest console call. DBCN also terminates QEMU when an existing backend reports an I/O error.

Resolve the frontend once per SBI exit and check it before use. Discard legacy output and return -1 for legacy input when no console is available. Explicitly return zero from legacy console putchar as required by SBI.

For DBCN, return SBI_ERR_DENIED with a zero value when the frontend is missing, before allocating a buffer or accessing guest memory. Convert backend I/O errors to SBI_ERR_FAILED instead of exiting QEMU, and do not copy a failed read into guest memory. Treat an incomplete single-byte write as a failure too, since that call must write the specified byte.

Preserve successful zero-length reads and writes, partial bulk transfers, and the unsupported-function response.

Fixes 4eb471258bd0 ("target/riscv: Handle KVM_EXIT_RISCV_SBI exit")
Fixes a6b53378f537 ("target/riscv/kvm: implement SBI debug console (DBCN) calls")

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4430

---
 target/riscv/kvm/kvm-cpu.c | 53 ++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 17 deletions(-)
target/riscv/kvm/kvm-cpu.c +36 -17
index 5beb602..0ebade6 100644
@@ -1686,7 +1686,8 @@ bool kvm_arch_stop_on_emulation_error(CPUState *cs)
     return true;
 }
 
-static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
+static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run,
+                                      CharFrontend *fe)
 {
     const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     g_autofree uint8_t *buf = NULL;
@@ -1707,6 +1708,12 @@ static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
             break;
         }
 
+        if (!fe) {
+            run->riscv_sbi.ret[0] = SBI_ERR_DENIED;
+            run->riscv_sbi.ret[1] = 0;
+            break;
+        }
+
         addr = run->riscv_sbi.args[1];
 
         /*
@@ -1720,22 +1727,22 @@ static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
         buf = g_malloc0(num_bytes);
 
         if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
-            ret = qemu_chr_fe_read_all(serial_hd(0)->fe, buf, num_bytes);
+            ret = qemu_chr_fe_read_all(fe, buf, num_bytes);
             if (ret < 0) {
-                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
-                             "reading chardev");
-                exit(1);
+                run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+                run->riscv_sbi.ret[1] = 0;
+                break;
             }
 
             address_space_write(cs->as, addr, attrs, buf, ret);
         } else {
             address_space_read(cs->as, addr, attrs, buf, num_bytes);
 
-            ret = qemu_chr_fe_write_all(serial_hd(0)->fe, buf, num_bytes);
+            ret = qemu_chr_fe_write_all(fe, buf, num_bytes);
             if (ret < 0) {
-                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
-                             "writing chardev");
-                exit(1);
+                run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+                run->riscv_sbi.ret[1] = 0;
+                break;
             }
         }
 
@@ -1743,13 +1750,19 @@ static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
         run->riscv_sbi.ret[1] = ret;
         break;
     case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
+        if (!fe) {
+            run->riscv_sbi.ret[0] = SBI_ERR_DENIED;
+            run->riscv_sbi.ret[1] = 0;
+            break;
+        }
+
         ch = run->riscv_sbi.args[0];
-        ret = qemu_chr_fe_write_all(serial_hd(0)->fe, &ch, sizeof(ch));
+        ret = qemu_chr_fe_write_all(fe, &ch, sizeof(ch));
 
-        if (ret < 0) {
-            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
-                         "writing chardev");
-            exit(1);
+        if (ret != sizeof(ch)) {
+            run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+            run->riscv_sbi.ret[1] = 0;
+            break;
         }
 
         run->riscv_sbi.ret[0] = SBI_SUCCESS;
@@ -1762,15 +1775,21 @@ static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
 
 static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
 {
+    Chardev *chr = serial_hd(0);
+    CharFrontend *fe = chr ? chr->fe : NULL;
     int ret = 0;
     unsigned char ch;
+
     switch (run->riscv_sbi.extension_id) {
     case SBI_EXT_0_1_CONSOLE_PUTCHAR:
         ch = run->riscv_sbi.args[0];
-        qemu_chr_fe_write(serial_hd(0)->fe, &ch, sizeof(ch));
+        if (fe) {
+            qemu_chr_fe_write(fe, &ch, sizeof(ch));
+        }
+        run->riscv_sbi.ret[0] = 0;
         break;
     case SBI_EXT_0_1_CONSOLE_GETCHAR:
-        ret = qemu_chr_fe_read_all(serial_hd(0)->fe, &ch, sizeof(ch));
+        ret = fe ? qemu_chr_fe_read_all(fe, &ch, sizeof(ch)) : 0;
         if (ret == sizeof(ch)) {
             run->riscv_sbi.ret[0] = ch;
         } else {
@@ -1779,7 +1798,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
         ret = 0;
         break;
     case SBI_EXT_DBCN:
-        kvm_riscv_handle_sbi_dbcn(cs, run);
+        kvm_riscv_handle_sbi_dbcn(cs, run, fe);
         break;
     default:
         qemu_log_mask(LOG_UNIMP,
Signature · 1 line
2.43.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 28 lines · Zephyr Li

The serial backend is optional, for example when QEMU is started with -serial none. The legacy SBI console and DBCN handlers nevertheless dereference serial_hd(0)->fe without checking whether the chardev or its frontend exists, causing QEMU to crash on a guest console call. DBCN also terminates QEMU when an existing backend reports an I/O error.

Resolve the frontend once per SBI exit and check it before use. Discard
legacy output and return -1 for legacy input when no console is available.
Explicitly return zero from legacy console putchar as required by SBI.

For DBCN, return SBI_ERR_DENIED with a zero value when the frontend is
missing, before allocating a buffer or accessing guest memory. Convert
backend I/O errors to SBI_ERR_FAILED instead of exiting QEMU, and do not
copy a failed read into guest memory. Treat an incomplete single-byte write as a failure too, since that call must write the specified byte.

Preserve successful zero-length reads and writes, partial bulk transfers,
and the unsupported-function response.

Quoted 5 lines
Fixes 4eb471258bd0 ("target/riscv: Handle KVM_EXIT_RISCV_SBI exit")
Fixes a6b53378f537 ("target/riscv/kvm: implement SBI debug console

(DBCN) calls")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4430

Alistair

Quoted 137 lines

---
 target/riscv/kvm/kvm-cpu.c | 53 ++++++++++++++++++++++++++---------- --

 1 file changed, 36 insertions(+), 17 deletions(-)
target/riscv/kvm/kvm-cpu.c +0 -0
index 5beb602..0ebade6 100644
@@ -1686,7 +1686,8 @@ bool kvm_arch_stop_on_emulation_error(CPUState
 *cs)
      return true;
  }


-static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
+static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run,
+                                      CharFrontend *fe) {
     const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; g_autofree uint8_t *buf = NULL;

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1707,6 +1708,12 @@ static void kvm_riscv_handle_sbi_dbcn(CPUState
 *cs, struct kvm_run *run)
              break;
          }


+        if (!fe) {
+            run->riscv_sbi.ret[0] = SBI_ERR_DENIED; +            run->riscv_sbi.ret[1] = 0;
+            break;
+        }
+
         addr = run->riscv_sbi.args[1];

         /*

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1720,22 +1727,22 @@ static void
 kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
          buf = g_malloc0(num_bytes);


         if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
-            ret = qemu_chr_fe_read_all(serial_hd(0)->fe, buf, num_bytes);
+            ret = qemu_chr_fe_read_all(fe, buf, num_bytes); if (ret < 0) {
-                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
-                             "reading chardev");
-                exit(1);
+                run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+                run->riscv_sbi.ret[1] = 0;
+                break;
             }

             address_space_write(cs->as, addr, attrs, buf, ret); } else {
             address_space_read(cs->as, addr, attrs, buf, num_bytes);
-            ret = qemu_chr_fe_write_all(serial_hd(0)->fe, buf, num_bytes);
+            ret = qemu_chr_fe_write_all(fe, buf, num_bytes); if (ret < 0) {
-                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
-                             "writing chardev");
-                exit(1);
+                run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+                run->riscv_sbi.ret[1] = 0;
+                break;
             }
         }

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1743,13 +1750,19 @@ static void
 kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
          run->riscv_sbi.ret[1] = ret;
          break;

     case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
+        if (!fe) {
+            run->riscv_sbi.ret[0] = SBI_ERR_DENIED;
+            run->riscv_sbi.ret[1] = 0;
+            break;
+        }
+
         ch = run->riscv_sbi.args[0];
-        ret = qemu_chr_fe_write_all(serial_hd(0)->fe, &ch, sizeof(ch));
+        ret = qemu_chr_fe_write_all(fe, &ch, sizeof(ch));
-        if (ret < 0) {
-            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
-                         "writing chardev");
-            exit(1);
+        if (ret != sizeof(ch)) {
+            run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+            run->riscv_sbi.ret[1] = 0;
+            break;
         }

         run->riscv_sbi.ret[0] = SBI_SUCCESS;

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1762,15 +1775,21 @@ static void
 kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)


 static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run) {
+    Chardev *chr = serial_hd(0);
+    CharFrontend *fe = chr ? chr->fe : NULL;
     int ret = 0;
     unsigned char ch;
+
     switch (run->riscv_sbi.extension_id) {
     case SBI_EXT_0_1_CONSOLE_PUTCHAR:
         ch = run->riscv_sbi.args[0];
-        qemu_chr_fe_write(serial_hd(0)->fe, &ch, sizeof(ch)); +        if (fe) {
+            qemu_chr_fe_write(fe, &ch, sizeof(ch));
+        }
+        run->riscv_sbi.ret[0] = 0;
         break;
     case SBI_EXT_0_1_CONSOLE_GETCHAR:
-        ret = qemu_chr_fe_read_all(serial_hd(0)->fe, &ch, sizeof(ch));
+        ret = fe ? qemu_chr_fe_read_all(fe, &ch, sizeof(ch)) : 0; if (ret == sizeof(ch)) {
             run->riscv_sbi.ret[0] = ch;
         } else {

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1779,7 +1798,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs,
 struct kvm_run *run)
          ret = 0;
          break;

     case SBI_EXT_DBCN:
-        kvm_riscv_handle_sbi_dbcn(cs, run);
+        kvm_riscv_handle_sbi_dbcn(cs, run, fe);
         break;
     default:
         qemu_log_mask(LOG_UNIMP,

#2 lore raw
In reply to Zephyr Li
Quoted 28 lines · Zephyr Li

The serial backend is optional, for example when QEMU is started with -serial none. The legacy SBI console and DBCN handlers nevertheless dereference serial_hd(0)->fe without checking whether the chardev or its frontend exists, causing QEMU to crash on a guest console call. DBCN also terminates QEMU when an existing backend reports an I/O error.

Resolve the frontend once per SBI exit and check it before use. Discard
legacy output and return -1 for legacy input when no console is available.
Explicitly return zero from legacy console putchar as required by SBI.

For DBCN, return SBI_ERR_DENIED with a zero value when the frontend is
missing, before allocating a buffer or accessing guest memory. Convert
backend I/O errors to SBI_ERR_FAILED instead of exiting QEMU, and do not
copy a failed read into guest memory. Treat an incomplete single-byte write as a failure too, since that call must write the specified byte.

Preserve successful zero-length reads and writes, partial bulk transfers,
and the unsupported-function response.

Quoted 5 lines
Fixes 4eb471258bd0 ("target/riscv: Handle KVM_EXIT_RISCV_SBI exit")
Fixes a6b53378f537 ("target/riscv/kvm: implement SBI debug console

(DBCN) calls")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4430

Thanks!

Applied to riscv-to-apply.next

Alistair

Quoted 137 lines

---
 target/riscv/kvm/kvm-cpu.c | 53 ++++++++++++++++++++++++++---------- --

 1 file changed, 36 insertions(+), 17 deletions(-)
target/riscv/kvm/kvm-cpu.c +0 -0
index 5beb602..0ebade6 100644
@@ -1686,7 +1686,8 @@ bool kvm_arch_stop_on_emulation_error(CPUState
 *cs)
      return true;
  }


-static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
+static void kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run,
+                                      CharFrontend *fe) {
     const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; g_autofree uint8_t *buf = NULL;

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1707,6 +1708,12 @@ static void kvm_riscv_handle_sbi_dbcn(CPUState
 *cs, struct kvm_run *run)
              break;
          }


+        if (!fe) {
+            run->riscv_sbi.ret[0] = SBI_ERR_DENIED; +            run->riscv_sbi.ret[1] = 0;
+            break;
+        }
+
         addr = run->riscv_sbi.args[1];

         /*

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1720,22 +1727,22 @@ static void
 kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
          buf = g_malloc0(num_bytes);


         if (run->riscv_sbi.function_id == SBI_EXT_DBCN_CONSOLE_READ) {
-            ret = qemu_chr_fe_read_all(serial_hd(0)->fe, buf, num_bytes);
+            ret = qemu_chr_fe_read_all(fe, buf, num_bytes); if (ret < 0) {
-                error_report("SBI_EXT_DBCN_CONSOLE_READ: error when "
-                             "reading chardev");
-                exit(1);
+                run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+                run->riscv_sbi.ret[1] = 0;
+                break;
             }

             address_space_write(cs->as, addr, attrs, buf, ret); } else {
             address_space_read(cs->as, addr, attrs, buf, num_bytes);
-            ret = qemu_chr_fe_write_all(serial_hd(0)->fe, buf, num_bytes);
+            ret = qemu_chr_fe_write_all(fe, buf, num_bytes); if (ret < 0) {
-                error_report("SBI_EXT_DBCN_CONSOLE_WRITE: error when "
-                             "writing chardev");
-                exit(1);
+                run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+                run->riscv_sbi.ret[1] = 0;
+                break;
             }
         }

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1743,13 +1750,19 @@ static void
 kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)
          run->riscv_sbi.ret[1] = ret;
          break;

     case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
+        if (!fe) {
+            run->riscv_sbi.ret[0] = SBI_ERR_DENIED;
+            run->riscv_sbi.ret[1] = 0;
+            break;
+        }
+
         ch = run->riscv_sbi.args[0];
-        ret = qemu_chr_fe_write_all(serial_hd(0)->fe, &ch, sizeof(ch));
+        ret = qemu_chr_fe_write_all(fe, &ch, sizeof(ch));
-        if (ret < 0) {
-            error_report("SBI_EXT_DBCN_CONSOLE_WRITE_BYTE: error when "
-                         "writing chardev");
-            exit(1);
+        if (ret != sizeof(ch)) {
+            run->riscv_sbi.ret[0] = SBI_ERR_FAILED;
+            run->riscv_sbi.ret[1] = 0;
+            break;
         }

         run->riscv_sbi.ret[0] = SBI_SUCCESS;

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1762,15 +1775,21 @@ static void
 kvm_riscv_handle_sbi_dbcn(CPUState *cs, struct kvm_run *run)


 static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run) {
+    Chardev *chr = serial_hd(0);
+    CharFrontend *fe = chr ? chr->fe : NULL;
     int ret = 0;
     unsigned char ch;
+
     switch (run->riscv_sbi.extension_id) {
     case SBI_EXT_0_1_CONSOLE_PUTCHAR:
         ch = run->riscv_sbi.args[0];
-        qemu_chr_fe_write(serial_hd(0)->fe, &ch, sizeof(ch)); +        if (fe) {
+            qemu_chr_fe_write(fe, &ch, sizeof(ch));
+        }
+        run->riscv_sbi.ret[0] = 0;
         break;
     case SBI_EXT_0_1_CONSOLE_GETCHAR:
-        ret = qemu_chr_fe_read_all(serial_hd(0)->fe, &ch, sizeof(ch));
+        ret = fe ? qemu_chr_fe_read_all(fe, &ch, sizeof(ch)) : 0; if (ret == sizeof(ch)) {
             run->riscv_sbi.ret[0] = ch;
         } else {

target/riscv/kvm/kvm-cpu.c +0 -0
@@ -1779,7 +1798,7 @@ static int kvm_riscv_handle_sbi(CPUState *cs,
 struct kvm_run *run)
          ret = 0;
          break;

     case SBI_EXT_DBCN:
-        kvm_riscv_handle_sbi_dbcn(cs, run);
+        kvm_riscv_handle_sbi_dbcn(cs, run, fe);
         break;
     default:
         qemu_log_mask(LOG_UNIMP,