Guest console read support via Debug Console Read SBI function

drivers/src/uart.rs

@@ -28,6 +28,42 @@ pub type Result<T> = core::result::Result<T, Error>;
 // Standard 16500a register set length.
 const UART_REGISTERS_LEN: u64 = 8;
 
+#[allow(dead_code)]
+enum UartRegister {
+    Thr,
+    Rbr,
+    Dll,
+    Ier,
+    Dlh,
+    Iir,
+    Fcr,
+    Lcr,
+    Mcr,
+    Lsr,
+    Msr,
+    Sr,
+}
+
+impl UartRegister {
+    fn offset(&self) -> usize {
+        use UartRegister::*;
+        match self {
+            Thr => 0,
+            Rbr => 0,
+            Dll => 0,
+            Ier => 1,
+            Dlh => 1,
+            Iir => 2,
+            Fcr => 2,
+            Lcr => 3,
+            Mcr => 4,
+            Lsr => 5,
+            Msr => 6,
+            Sr => 7,
+        }
+    }
+}
+
 static UART_DRIVER: Once<UartDriver> = Once::new();
 
 /// Driver for a standard UART.
@@ -65,12 +101,18 @@ impl UartDriver {
             base_address: Mutex::new(NonNull::new(base_address as _).unwrap()),
         };
         UART_DRIVER.call_once(|| uart);
-        Console::set_writer(UART_DRIVER.get().unwrap());
+        Console::set_driver(UART_DRIVER.get().unwrap());
         Ok(())
     }
+
+    fn read_reg(&self, reg: UartRegister) -> u8 {
+        let base_address = self.base_address.lock();
+        // Safety: We trust firmware to provide a valid base address in the device tree.
+        unsafe { core::ptr::read_volatile(base_address.as_ptr().byte_add(reg.offset())) }
+    }
 }
 
-impl ConsoleWriter for UartDriver {
+impl ConsoleDriver for UartDriver {
     /// Write an entire byte sequence to this UART.
     fn write_bytes(&self, bytes: &[u8]) {
         let base_address = self.base_address.lock();
@@ -80,6 +122,16 @@ impl ConsoleWriter for UartDriver {
             unsafe { core::ptr::write_volatile(base_address.as_ptr(), b) };
         }
     }
+
+    /// Read bytes from this UART.
+    fn read_bytes(&self, bytes: &mut [u8]) -> usize {
+        let mut n = 0;
+        while n < bytes.len() && self.read_reg(UartRegister::Lsr) & 1 != 0 {
+            bytes[n] = self.read_reg(UartRegister::Rbr);
+            n += 1;
+        }
+        n
+    }
 }
 
 // Safety: Access to the pointer to the UART's registers is guarded by a Mutex and the UartDriver

s-mode-utils/src/print.rs

@@ -7,24 +7,40 @@ use sync::Mutex;
 pub use crate::{print, println};
 
 /// Interface for a console driver.
-pub trait ConsoleWriter: Sync {
+pub trait ConsoleDriver: Sync {
     /// Writes `bytes` to the console.
     fn write_bytes(&self, bytes: &[u8]);
+
+    /// Read from the console into `bytes`. Returns the number of bytes read.
+    /// This default implementation doesn't produce any input.
+    fn read_bytes(&self, _bytes: &mut [u8]) -> usize {
+        0
+    }
 }
 
 /// Represents the system console, used by the `print!` and `println!` macros.
 pub struct Console {
-    writer: Option<&'static dyn ConsoleWriter>,
+    driver: Option<&'static dyn ConsoleDriver>,
 }
 
 impl Console {
     const fn new() -> Self {
-        Self { writer: None }
+        Self { driver: None }
+    }
+
+    /// Reads characters from the console into `buf`. Returns the number of bytes read, which may
+    /// be less than the buffer size if there is not enough input.
+    pub fn read(buf: &mut [u8]) -> usize {
+        CONSOLE
+            .lock()
+            .driver
+            .map(|d| d.read_bytes(buf))
+            .unwrap_or(0)
     }
 
-    /// Sets the writer for the system console.
-    pub fn set_writer(writer: &'static dyn ConsoleWriter) {
-        CONSOLE.lock().writer = Some(writer);
+    /// Sets the driver for the system console.
+    pub fn set_driver(driver: &'static dyn ConsoleDriver) {
+        CONSOLE.lock().driver = Some(driver);
     }
 }
 
@@ -55,7 +71,7 @@ macro_rules! println {
 
 impl core::fmt::Write for Console {
     fn write_str(&mut self, s: &str) -> core::fmt::Result {
-        if let Some(w) = self.writer {
+        if let Some(w) = self.driver {
             w.write_bytes(s.as_bytes());
         }
         Ok(())

s-mode-utils/src/sbi_console.rs

@@ -6,7 +6,7 @@ use sbi_rs::api::debug_console::console_puts;
 use sbi_rs::{ecall_send, SbiMessage};
 use sync::{Mutex, Once};
 
-use crate::print::{Console, ConsoleWriter};
+use crate::print::{Console, ConsoleDriver};
 
 /// Driver for an SBI based console.
 pub struct SbiConsole {
@@ -23,11 +23,11 @@ impl SbiConsole {
             buffer: Mutex::new(console_buffer),
         };
         SBI_CONSOLE.call_once(|| console);
-        Console::set_writer(SBI_CONSOLE.get().unwrap());
+        Console::set_driver(SBI_CONSOLE.get().unwrap());
     }
 }
 
-impl ConsoleWriter for SbiConsole {
+impl ConsoleDriver for SbiConsole {
     /// Write an entire byte sequence to the SBI console.
     fn write_bytes(&self, bytes: &[u8]) {
         let mut buffer = self.buffer.lock();
@@ -48,11 +48,11 @@ static SBI_CONSOLE_V01: SbiConsoleV01 = SbiConsoleV01 {};
 impl SbiConsoleV01 {
     /// Sets the SBI legacy console as the system console.
     pub fn set_as_console() {
-        Console::set_writer(&SBI_CONSOLE_V01);
+        Console::set_driver(&SBI_CONSOLE_V01);
     }
 }
 
-impl ConsoleWriter for SbiConsoleV01 {
+impl ConsoleDriver for SbiConsoleV01 {
     /// Write an entire byte sequence to the SBI console.
     fn write_bytes(&self, bytes: &[u8]) {
         for &b in bytes {

src/host_vm.rs

@@ -4,6 +4,7 @@
 
 use arrayvec::{ArrayString, ArrayVec};
 use core::{fmt, num, ops::ControlFlow, slice};
+use data_model::VolatileSlice;
 use device_tree::{DeviceTree, DeviceTreeResult, DeviceTreeSerializer};
 use drivers::{imsic::*, iommu::*, pci::*, CpuId, CpuInfo};
 use page_tracking::collections::PageBox;
@@ -21,7 +22,7 @@ use crate::guest_tracking::{GuestVm, Guests, Result as GuestTrackingResult};
 use crate::smp;
 use crate::vm::{FinalizedVm, Vm};
 use crate::vm_cpu::{VmCpu, VmCpuExitReporting, VmCpuParent, VmCpus};
-use crate::vm_pages::VmPages;
+use crate::vm_pages::{PinnedPages, Result as VmPagesResult, VmPages};
 
 // Page size of host VM pages.
 pub const HOST_VM_ALIGN: PageSize = PageSize::Size2M;
@@ -392,6 +393,20 @@ impl core::fmt::Display for MmioEmulationError {
     }
 }
 
+struct PinnedBuffer<'a> {
+    slice: VolatileSlice<'a>,
+    // `_pinned` must remain in scope to keep the buffer that is backing `slice` valid.
+    _pinned: PinnedPages,
+}
+
+impl<'a> core::ops::Deref for PinnedBuffer<'a> {
+    type Target = VolatileSlice<'a>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.slice
+    }
+}
+
 #[derive(Default)]
 struct HostVmRunner {
     scause: u64,
@@ -448,6 +463,18 @@ impl HostVmRunner {
                                 self.gprs.set_reg(GprIndex::A0, sbi_ret.error_code as u64);
                                 self.gprs.set_reg(GprIndex::A1, sbi_ret.return_value as u64);
                             }
+                            Ok(DebugConsole(DebugConsoleFunction::Read { len, addr, .. })) => {
+                                let sbi_ret = match self.handle_read(&vm, addr, len) {
+                                    Ok(n) => SbiReturn::success(n as i64),
+                                    Err(n) => SbiReturn {
+                                        error_code: SbiError::InvalidAddress as i64,
+                                        return_value: n as i64,
+                                    },
+                                };
+
+                                self.gprs.set_reg(GprIndex::A0, sbi_ret.error_code as u64);
+                                self.gprs.set_reg(GprIndex::A1, sbi_ret.return_value as u64);
+                            }
                             Ok(DebugConsole(DebugConsoleFunction::WriteByte { byte })) => {
                                 self.handle_write_byte(byte as u8);
                                 self.gprs.set_reg(GprIndex::A0, 0);
@@ -527,46 +554,91 @@ impl HostVmRunner {
         Ok(())
     }
 
-    fn handle_write<T: GuestStagePagingMode>(
+    fn pin_console_buffer<T: GuestStagePagingMode>(
         &mut self,
         vm: &FinalizedVm<T>,
         addr: u64,
-        len: u64,
-    ) -> core::result::Result<u64, u64> {
-        // Pin the pages that we'll be printing from. We assume that the buffer is physically
-        // contiguous, which should be the case since that's how we set up the host VM's address
-        // space.
+        len: usize,
+    ) -> VmPagesResult<PinnedBuffer> {
+        // Pin the pages that hold the buffer. We assume that the buffer is physically contiguous,
+        // which should be the case since that's how we set up the host VM's address space. Note
+        // that safety does not depend on this assumption because `pin_shared_pages` will return an
+        // error when the physical page range is fond to be non-contiguous.
         let page_addr = GuestPageAddr::with_round_down(
             GuestPhysAddr::guest(addr, vm.page_owner_id()),
             PageSize::Size4k,
         );
         let offset = addr - page_addr.bits();
-        let num_pages = PageSize::num_4k_pages(offset + len);
-        let pinned = vm
-            .vm_pages()
-            .pin_shared_pages(page_addr, num_pages)
-            .map_err(|_| 0u64)?;
+        let num_pages = PageSize::num_4k_pages(offset + len as u64);
+        let pinned = vm.vm_pages().pin_shared_pages(page_addr, num_pages)?;
+        let hyp_addr = pinned.range().base().bits() + offset;
+        // Safety: The buffer is valid and remains so as long as `pinned` remains in scope, which
+        // PinnedBuffer guarantees. byte pointers are always aligned correctly.
+        let slice = unsafe { VolatileSlice::from_raw_parts(hyp_addr as *mut u8, len) };
+        Ok(PinnedBuffer {
+            slice,
+            _pinned: pinned,
+        })
+    }
+
+    fn handle_write<T: GuestStagePagingMode>(
+        &mut self,
+        vm: &FinalizedVm<T>,
+        addr: u64,
+        len: u64,
+    ) -> core::result::Result<u64, u64> {
+        let len = len as usize;
+        let pinned = self.pin_console_buffer(vm, addr, len).map_err(|_| 0u64)?;
 
-        // Print the bytes in chunks. We copy to a temporary buffer as the bytes could be modified
-        // concurrently by the VM on another CPU.
         let mut copied = 0;
-        let mut hyp_addr = pinned.range().base().bits() + offset;
-        while copied != len {
+        while copied < len {
             let mut buf = [0u8; 256];
-            let to_copy = core::cmp::min(buf.len(), (len - copied) as usize);
-            for c in buf.iter_mut() {
-                // Safety: We've confirmed that the address is within a region of accessible memory
-                // and cannot be remapped as long as we hold the pin. `u8`s are always aligned and
-                // properly initialized.
-                *c = unsafe { core::ptr::read_volatile(hyp_addr as *const u8) };
-                hyp_addr += 1;
-            }
-            let s = core::str::from_utf8(&buf[..to_copy]).map_err(|_| copied)?;
+            let to_copy = core::cmp::min(buf.len(), len - copied);
+
+            // Unwrap OK: offset and length are within bounds by construction.
+            pinned
+                .sub_slice(copied, to_copy)
+                .unwrap()
+                .copy_to(&mut buf[..to_copy]);
+
+            let s = core::str::from_utf8(&buf[..to_copy]).map_err(|_| copied as u64)?;
             print!("{s}");
-            copied += to_copy as u64;
+
+            copied += to_copy;
+        }
+
+        Ok(copied as u64)
+    }
+
+    fn handle_read<T: GuestStagePagingMode>(
+        &mut self,
+        vm: &FinalizedVm<T>,
+        addr: u64,
+        len: u64,
+    ) -> core::result::Result<u64, u64> {
+        let len = len as usize;
+        let pinned = self.pin_console_buffer(vm, addr, len).map_err(|_| 0u64)?;
+
+        let mut copied = 0;
+        while copied < len {
+            let mut buf = [0u8; 256];
+            let to_copy = core::cmp::min(buf.len(), len - copied);
+
+            let read = core::cmp::min(to_copy, Console::read(&mut buf[..to_copy]));
+            if read == 0 {
+                break;
+            }
+
+            // Unwrap OK: offset and length are within bounds by construction.
+            pinned
+                .sub_slice(copied, read)
+                .unwrap()
+                .copy_from(&buf[..read]);
+
+            copied += read;
         }
 
-        Ok(len)
+        Ok(copied as u64)
     }
 
     fn handle_write_byte(&mut self, c: u8) {