From aa30685d307235ea70a3ac15e223dfe7b4ab3661 Mon Sep 17 00:00:00 2001 From: Mattias Nissler Date: Tue, 27 May 2025 03:19:31 -0700 Subject: [PATCH 1/4] console: Add read support This adds the infrastructure to support reading bytes from the console by renaming `ConsoleWriter` trait to `ConsoleDriver` and adding a function to read bytes. A default implementation that never returns any bytes is provided. Read implementations in individual console drivers are left for subsequent changes. --- drivers/src/uart.rs | 4 ++-- s-mode-utils/src/print.rs | 30 +++++++++++++++++++++++------- s-mode-utils/src/sbi_console.rs | 10 +++++----- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/drivers/src/uart.rs b/drivers/src/uart.rs index eff0170c..5d3af194 100644 --- a/drivers/src/uart.rs +++ b/drivers/src/uart.rs @@ -65,12 +65,12 @@ 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(()) } } -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(); diff --git a/s-mode-utils/src/print.rs b/s-mode-utils/src/print.rs index 38297d56..64b0863f 100644 --- a/s-mode-utils/src/print.rs +++ b/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(()) diff --git a/s-mode-utils/src/sbi_console.rs b/s-mode-utils/src/sbi_console.rs index 6de011cf..fea839e0 100644 --- a/s-mode-utils/src/sbi_console.rs +++ b/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 { From eb04b31689ce68336fe352256cc01cb557d0a6b7 Mon Sep 17 00:00:00 2001 From: Mattias Nissler Date: Wed, 4 Jun 2025 23:54:40 -0700 Subject: [PATCH 2/4] drivers/uart: Implement read support This change adds minimal logic to the UART driver for reading bytes. As is already the case with the write code path, the driver assumes that the UART is already set up correctly. --- drivers/src/uart.rs | 52 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/drivers/src/uart.rs b/drivers/src/uart.rs index 5d3af194..8650fc77 100644 --- a/drivers/src/uart.rs +++ b/drivers/src/uart.rs @@ -28,6 +28,42 @@ pub type Result = core::result::Result; // 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 = Once::new(); /// Driver for a standard UART. @@ -68,6 +104,12 @@ impl UartDriver { 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 ConsoleDriver for UartDriver { @@ -80,6 +122,16 @@ impl ConsoleDriver 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 From 70e5fafa86d4e85a842cb7a1444b263b484fd9c2 Mon Sep 17 00:00:00 2001 From: Mattias Nissler Date: Thu, 5 Jun 2025 00:29:58 -0700 Subject: [PATCH 3/4] sbi-rs: Uprev Bump the version to bring in a fix to Debug Console. --- sbi-rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sbi-rs b/sbi-rs index c0d8a85a..10247f52 160000 --- a/sbi-rs +++ b/sbi-rs @@ -1 +1 @@ -Subproject commit c0d8a85a53da5551e240226b2092a76c3ef6d03e +Subproject commit 10247f521a6070eb65c6fc2f52b8dc8b5fb121ca From acdf9ac666cf6ddbcbeacc7e361a164c9f1a1ed8 Mon Sep 17 00:00:00 2001 From: Mattias Nissler Date: Tue, 27 May 2025 03:20:27 -0700 Subject: [PATCH 4/4] Implement SBI DebugConsoleFunction::Read support A basic implementation analogous to the write implementation that reads characters from the UART. This has been tested successfully against with the Linux SBI hvc driver. While at it wrap access to the buffer in a `PinnedBuffer` helper that isolates the business logic from safety considerations by exposing the buffer as a `VolatileSlice`. --- src/host_vm.rs | 126 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 99 insertions(+), 27 deletions(-) diff --git a/src/host_vm.rs b/src/host_vm.rs index 355d8acf..337b2037 100644 --- a/src/host_vm.rs +++ b/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( + fn pin_console_buffer( &mut self, vm: &FinalizedVm, addr: u64, - len: u64, - ) -> core::result::Result { - // 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 { + // 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( + &mut self, + vm: &FinalizedVm, + addr: u64, + len: u64, + ) -> core::result::Result { + 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( + &mut self, + vm: &FinalizedVm, + addr: u64, + len: u64, + ) -> core::result::Result { + 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) {