From d70003982c2041e11ea6c3c54032d0f98a00ed9d Mon Sep 17 00:00:00 2001 From: Matt Harvey Date: Thu, 30 Sep 2021 00:54:28 +0000 Subject: [PATCH] Merge "Aligns UART API with Rust Read and Write" GitOrigin-RevId: df407ae4bddb75b1bc43c35062947679c2b10c3a --- .../DebugConsole/DebugConsole.camkes | 6 +- .../kata-debug-console/src/run.rs | 4 +- .../DebugConsole/kata-uart-client/src/lib.rs | 90 +++++++----- .../OpenTitanUARTDriver.camkes | 18 +-- .../include/uart_driver_error.h | 19 +++ .../OpenTitanUARTDriver/src/driver.c | 135 +++++++++++------- apps/system/interfaces/RustIO.idl4 | 38 +++++ apps/system/interfaces/dataport_io.idl4 | 15 -- apps/system/system.camkes | 9 +- 9 files changed, 213 insertions(+), 121 deletions(-) create mode 100644 apps/system/components/OpenTitanUARTDriver/include/uart_driver_error.h create mode 100644 apps/system/interfaces/RustIO.idl4 delete mode 100644 apps/system/interfaces/dataport_io.idl4 diff --git a/apps/system/components/DebugConsole/DebugConsole.camkes b/apps/system/components/DebugConsole/DebugConsole.camkes index 18c8689..9362857 100644 --- a/apps/system/components/DebugConsole/DebugConsole.camkes +++ b/apps/system/components/DebugConsole/DebugConsole.camkes @@ -10,12 +10,10 @@ component DebugConsole { control; dataport Buf tx_dataport; - uses dataport_io_inf uart_tx; - has mutex tx_mutex; + uses rust_write_inf uart_write; dataport Buf rx_dataport; - uses dataport_io_inf uart_rx; - has mutex rx_mutex; + uses rust_read_inf uart_read; provides LoggerInterface logger; uses ProcessControlInterface proc_ctrl; diff --git a/apps/system/components/DebugConsole/kata-debug-console/src/run.rs b/apps/system/components/DebugConsole/kata-debug-console/src/run.rs index d85eb3f..043c521 100644 --- a/apps/system/components/DebugConsole/kata-debug-console/src/run.rs +++ b/apps/system/components/DebugConsole/kata-debug-console/src/run.rs @@ -44,7 +44,7 @@ pub extern "C" fn pre_init() { #[no_mangle] pub extern "C" fn run() -> ! { trace!("run"); - let mut tx = kata_uart_client::Tx {}; - let mut rx = kata_uart_client::Rx {}; + let mut tx = kata_uart_client::Tx::new(); + let mut rx = kata_uart_client::Rx::new(); kata_shell::repl(&mut tx, &mut rx); } diff --git a/apps/system/components/DebugConsole/kata-uart-client/src/lib.rs b/apps/system/components/DebugConsole/kata-uart-client/src/lib.rs index 2b1eda8..037cddf 100644 --- a/apps/system/components/DebugConsole/kata-uart-client/src/lib.rs +++ b/apps/system/components/DebugConsole/kata-uart-client/src/lib.rs @@ -4,19 +4,6 @@ use core::fmt::Write; use cstr_core::CStr; use kata_io as io; -// C interface to external UART driver. -extern "C" { - static rx_dataport: *mut cty::c_uchar; - fn uart_rx_update(n: cty::size_t); - fn rx_mutex_lock(); - fn rx_mutex_unlock(); - - static tx_dataport: *mut cty::c_uchar; - fn uart_tx_update(n: cty::size_t); - fn tx_mutex_lock(); - fn tx_mutex_unlock(); -} - // Console logging interface. #[no_mangle] pub extern "C" fn logger_log(level: u8, msg: *const cstr_core::c_char) { @@ -30,44 +17,83 @@ pub extern "C" fn logger_log(level: u8, msg: *const cstr_core::c_char) { }; if l <= log::max_level() { // TODO(sleffler): is the uart driver ok w/ multiple writers? - let output: &mut dyn io::Write = &mut self::Tx {}; + let output: &mut dyn io::Write = &mut self::Tx::new(); unsafe { let _ = writeln!(output, "{}", CStr::from_ptr(msg).to_str().unwrap()); } } } -pub struct Rx {} +const DATAPORT_SIZE: usize = 4096; + +pub struct Rx { + dataport: &'static [u8], +} + +impl Rx { + pub fn new() -> Rx { + extern "C" { + static rx_dataport: *mut cty::c_uchar; + } + Rx { + dataport: unsafe { core::slice::from_raw_parts(rx_dataport, DATAPORT_SIZE) }, + } + } +} impl io::Read for Rx { fn read(&mut self, buf: &mut [u8]) -> io::Result { - unsafe { - rx_mutex_lock(); - uart_rx_update(buf.len()); - let port = core::slice::from_raw_parts(rx_dataport, buf.len()); - buf.copy_from_slice(&port); - rx_mutex_unlock(); + extern "C" { + fn uart_read_read(limit: cty::size_t) -> cty::c_int; + } + let n = unsafe { uart_read_read(buf.len()) }; + if n >= 0 { + let s = n as usize; + buf[..s].copy_from_slice(&self.dataport[..s]); + Ok(s) + } else { + Err(io::Error) } - Ok(buf.len()) } } -pub struct Tx {} +pub struct Tx { + dataport: &'static mut [u8], +} + +impl Tx { + pub fn new() -> Tx { + extern "C" { + static tx_dataport: *mut cty::c_uchar; + } + Tx { + dataport: unsafe { core::slice::from_raw_parts_mut(tx_dataport, DATAPORT_SIZE) }, + } + } +} impl io::Write for Tx { fn write(&mut self, buf: &[u8]) -> io::Result { - unsafe { - tx_mutex_lock(); - let port = core::slice::from_raw_parts_mut(tx_dataport, buf.len()); - port.copy_from_slice(buf); - uart_tx_update(buf.len()); - tx_mutex_unlock(); + extern "C" { + fn uart_write_write(available: cty::size_t) -> cty::c_int; + } + self.dataport[..buf.len()].copy_from_slice(buf); + let n = unsafe { uart_write_write(buf.len()) }; + if n >= 0 { + Ok(n as usize) + } else { + Err(io::Error) } - Ok(buf.len()) } fn flush(&mut self) -> io::Result<()> { - // Do nothing. This implementation has no internal buffering. - Ok(()) + extern "C" { + fn uart_write_flush() -> cty::c_int; + } + if unsafe { uart_write_flush() } == 0 { + Ok(()) + } else { + Err(io::Error) + } } } diff --git a/apps/system/components/OpenTitanUARTDriver/OpenTitanUARTDriver.camkes b/apps/system/components/OpenTitanUARTDriver/OpenTitanUARTDriver.camkes index fcc306d..4f911c7 100644 --- a/apps/system/components/OpenTitanUARTDriver/OpenTitanUARTDriver.camkes +++ b/apps/system/components/OpenTitanUARTDriver/OpenTitanUARTDriver.camkes @@ -1,27 +1,23 @@ /* - * Copyright 2017, Data61 - * Commonwealth Scientific and Industrial Research Organisation (CSIRO) - * ABN 41 687 119 230. + * CAmkES component accessing an OpenTitan UART. * - * This software may be distributed and modified according to the terms of - * the BSD 2-Clause license. Note that NO WARRANTY is provided. - * See "LICENSE_BSD2.txt" for details. - * - * @TAG(DATA61_BSD) + * Copyright 2021, Google LLC + * Apache License 2.0 */ +import "../../interfaces/RustIO.idl4"; + component OpenTitanUARTDriver { dataport Buf mmio_region; dataport Buf tx_dataport; - provides dataport_io_inf tx; + provides rust_write_inf write; consumes Interrupt tx_watermark; consumes Interrupt tx_empty; - has semaphore tx_semaphore; has mutex tx_mutex; dataport Buf rx_dataport; - provides dataport_io_inf rx; + provides rust_read_inf read; consumes Interrupt rx_watermark; has semaphore rx_semaphore; has mutex rx_mutex; diff --git a/apps/system/components/OpenTitanUARTDriver/include/uart_driver_error.h b/apps/system/components/OpenTitanUARTDriver/include/uart_driver_error.h new file mode 100644 index 0000000..6aee72a --- /dev/null +++ b/apps/system/components/OpenTitanUARTDriver/include/uart_driver_error.h @@ -0,0 +1,19 @@ +/* + * Copyright 2021, Google LLC + * + * Error codes for the OpenTitanUARTDriver. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#pragma once + +// Return codes for errors on read() and write(). +// +// Normally these functions return the number of bytes actually read or written, +// with 0 indicating the end of the stream. If something goes wrong, the +// functions will return one of these negative values. +typedef enum UARTDriverError { + UARTDriver_AssertionFailed = -1, + UARTDriver_OutOfDataportBounds = -2, +} uart_driver_error_t; diff --git a/apps/system/components/OpenTitanUARTDriver/src/driver.c b/apps/system/components/OpenTitanUARTDriver/src/driver.c index e24ecf0..3787f7b 100644 --- a/apps/system/components/OpenTitanUARTDriver/src/driver.c +++ b/apps/system/components/OpenTitanUARTDriver/src/driver.c @@ -15,6 +15,7 @@ #include "circular_buffer.h" #include "opentitan/uart.h" +#include "uart_driver_error.h" // Referenced by macros in the generated file opentitan/uart.h. #define UART0_BASE_ADDR (void *)mmio_region @@ -46,6 +47,13 @@ ((value & UART_##regname##_##subfield##_MASK) \ << UART_##regname##_##subfield##_OFFSET) +#define LOCK(lockname) seL4_Assert(lockname##_lock() == 0); +#define UNLOCK(lockname) seL4_Assert(lockname##_unlock() == 0); +#define ASSERT_OR_RETURN(x) \ + if (!(bool)(x)) { \ + return UARTDriver_AssertionFailed; \ + } + // Driver-owned buffer to receive more than the FIFO size before the received // data is consumed by rx_update. static circular_buffer rx_buf; // guarded by rx_mutex @@ -84,7 +92,7 @@ static void uart_putchar(char c) { // This stops when the transmit FIFO is full or when tx_buf is empty, whichever // comes first. static void fill_tx_fifo() { - seL4_Assert(tx_mutex_lock() == 0); + LOCK(tx_mutex); while (tx_fifo_level() < UART_FIFO_CAPACITY) { char c; if (!circular_buffer_pop_front(&tx_buf, &c)) { @@ -93,10 +101,7 @@ static void fill_tx_fifo() { } uart_putchar(c); } - if (circular_buffer_remaining(&tx_buf) > 0) { - seL4_Assert(tx_semaphore_post() == 0); - } - seL4_Assert(tx_mutex_unlock() == 0); + UNLOCK(tx_mutex); } // CAmkES initialization hook. @@ -157,62 +162,86 @@ void pre_init() { BIT(UART_INTR_COMMON_TX_EMPTY)); } -// Implements the update method of the CAmkES dataport_inf rx. +// Implements Rust Read::read(). // -// Reads a given number of bytes from rx_buf into the CAmkES rx_dataport, -// blocking the RPC until the entire requested byte count has been read. -void rx_update(uint32_t num_to_read) { - // TODO(mattharvey): Error return value for num_to_read > - // TX_RX_DATAPORT_CAPACITY. - seL4_Assert(num_to_read <= TX_RX_DATAPORT_CAPACITY); +// Reads up to a given limit of bytes into the CAmkES rx_dataport, blocking +// until at least one byte is available. +int read_read(size_t limit) { + if (limit > TX_RX_DATAPORT_CAPACITY) { + return UARTDriver_OutOfDataportBounds; + } + char *cursor = (char *)rx_dataport; + char *const cursor_begin = cursor; + char *const cursor_limit = cursor_begin + limit; - char *dataport_cursor = (char *)rx_dataport; - char *const dataport_end = dataport_cursor + num_to_read; - while (dataport_cursor < dataport_end) { - seL4_Assert(rx_mutex_lock() == 0); + LOCK(rx_mutex); + { while (circular_buffer_empty(&rx_buf)) { - seL4_Assert(rx_mutex_unlock() == 0); + UNLOCK(rx_mutex); seL4_Assert(rx_semaphore_wait() == 0); - seL4_Assert(rx_mutex_lock() == 0); + LOCK(rx_mutex); } - for (; dataport_cursor < dataport_end; ++dataport_cursor) { - if (!circular_buffer_pop_front(&rx_buf, dataport_cursor)) { + while (cursor < cursor_limit) { + if (!circular_buffer_pop_front(&rx_buf, cursor)) { // The buffer is empty. break; } + ++cursor; } - seL4_Assert(rx_mutex_unlock() == 0); } + UNLOCK(rx_mutex); + + int num_read = cursor - cursor_begin; + ASSERT_OR_RETURN(num_read > 0); + return num_read; } -// Implements the update method of the CAmkES dataport_inf tx. +// Implements Rust Write::write(). // -// Writes the contents of the CAmkES tx_dataport to the UART, one at a time, -// blocking the RPC until the entire requested number of bytes has been written. -void tx_update(uint32_t num_valid_dataport_bytes) { - // TODO(mattharvey): Error return value for num_valid_dataport_bytes > - // TX_RX_DATAPORT_CAPACITY. - seL4_Assert(num_valid_dataport_bytes <= TX_RX_DATAPORT_CAPACITY); +// Writes as many bytes from tx_dataport as the hardware will accept, but not +// more than the number available (specified by the argument). Returns the +// number of bytes written or a negative value if there is any error. +int write_write(size_t available) { + if (available > TX_RX_DATAPORT_CAPACITY) { + return UARTDriver_OutOfDataportBounds; + } + const char *cursor = (const char *)tx_dataport; + const char *const cursor_begin = cursor; + const char *const cursor_limit = cursor_begin + available; - const char *dataport_cursor = (const char *)tx_dataport; - const char *const dataport_end = dataport_cursor + num_valid_dataport_bytes; - while (dataport_cursor < dataport_end) { - seL4_Assert(tx_mutex_lock() == 0); - while (circular_buffer_remaining(&tx_buf) == 0) { - seL4_Assert(tx_mutex_unlock() == 0); - seL4_Assert(tx_semaphore_wait() == 0); - seL4_Assert(tx_mutex_lock() == 0); - } - for (; dataport_cursor < dataport_end; ++dataport_cursor) { - if (!circular_buffer_push_back(&tx_buf, *dataport_cursor)) { - // The buffer is full. + while (cursor < cursor_limit) { + LOCK(tx_mutex); + { + if (circular_buffer_remaining(&tx_buf) == 0) { break; } + for (; cursor < cursor_limit; ++cursor) { + if (!circular_buffer_push_back(&tx_buf, *cursor)) { + // The buffer is full. + break; + } + } } - seL4_Assert(tx_mutex_unlock() == 0); + UNLOCK(tx_mutex); } fill_tx_fifo(); + + int num_written = cursor - cursor_begin; + ASSERT_OR_RETURN(num_written > 0); + return num_written; +} + +// Implements Rust Write::flush(). +// +// Drains tx_buf and TX_FIFO. Returns a negative value if there is any error. +int write_flush() { + LOCK(tx_mutex); + while (circular_buffer_remaining(&tx_buf)) { + fill_tx_fifo(); + } + UNLOCK(tx_mutex); + return 0; } // Handles a tx_watermark interrupt. @@ -247,7 +276,7 @@ void rx_watermark_handle(void) { } uint32_t num_read = 0; - seL4_Assert(rx_mutex_lock() == 0); + LOCK(rx_mutex); while (num_read < num_to_read) { if (!circular_buffer_push_back(&rx_buf, uart_getchar())) { // The buffer is full. @@ -255,7 +284,7 @@ void rx_watermark_handle(void) { } ++num_read; } - seL4_Assert(rx_mutex_unlock() == 0); + UNLOCK(rx_mutex); if (num_read > 0) { seL4_Assert(rx_semaphore_post() == 0); @@ -275,14 +304,16 @@ void rx_watermark_handle(void) { void tx_empty_handle(void) { fill_tx_fifo(); - seL4_Assert(tx_mutex_lock() == 0); - if (circular_buffer_empty(&tx_buf)) { - // Clears INTR_STATE for tx_empty. (INTR_STATE is write-1-to-clear.) We only - // do this if tx_buf is empty, since the TX FIFO might have become empty in - // the time from fill_tx_fifo having sent the last character until here. In - // that case, we want the interrupt to reassert. - REG(INTR_STATE) = BIT(UART_INTR_STATE_TX_EMPTY); + LOCK(tx_mutex); + { + if (circular_buffer_empty(&tx_buf)) { + // Clears INTR_STATE for tx_empty. (INTR_STATE is write-1-to-clear.) We + // only do this if tx_buf is empty, since the TX FIFO might have become + // empty in the time from fill_tx_fifo having sent the last character + // until here. In that case, we want the interrupt to reassert. + REG(INTR_STATE) = BIT(UART_INTR_STATE_TX_EMPTY); + } + seL4_Assert(tx_empty_acknowledge() == 0); } - seL4_Assert(tx_empty_acknowledge() == 0); - seL4_Assert(tx_mutex_unlock() == 0); + UNLOCK(tx_mutex); } diff --git a/apps/system/interfaces/RustIO.idl4 b/apps/system/interfaces/RustIO.idl4 new file mode 100644 index 0000000..9fa3f44 --- /dev/null +++ b/apps/system/interfaces/RustIO.idl4 @@ -0,0 +1,38 @@ +/* + * CAmkES backing for Rust Read and Write traits. + * + * Copyright 2021, Google LLC + * Apache License 2.0 + * + * These CAmkES interfaces express the standard Rust read() and write() + * signatures, assuming two separate, externally defined, and implicitly + * associated dataports for each of read and write. + * + * It is intended that Rust code be able to use extern "C" declarations + * referencing the camkes.h that this will generate as the core of + * implementations of the Read and Write traits. + */ + +procedure rust_read_inf { + // Reads up to limit bytes into the read dataport. + // + // Returns the number of bytes read or a negative value if there is any + // error. + int read(in size_t limit); +}; + +procedure rust_write_inf { + // Writes up to a given number of bytes from the write dataport. + // + // Returns the number of bytes actually written or a negative value if there + // is any error. For non-negative return values < available, the caller is + // reponsible for retrying with the remaining bytes at the beginning of the + // write dataport. + int write(in size_t available); + + // Blocks until all bytes so far written have been pushed to the real sink. + // + // The semantics are the same as Rust's Write::flush. Returns 0 on success + // and a negative value if there is any error. + int flush(); +} diff --git a/apps/system/interfaces/dataport_io.idl4 b/apps/system/interfaces/dataport_io.idl4 deleted file mode 100644 index 4f5c659..0000000 --- a/apps/system/interfaces/dataport_io.idl4 +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright 2017, Data61 - * Commonwealth Scientific and Industrial Research Organisation (CSIRO) - * ABN 41 687 119 230. - * - * This software may be distributed and modified according to the terms of - * the BSD 2-Clause license. Note that NO WARRANTY is provided. - * See "LICENSE_BSD2.txt" for details. - * - * @TAG(DATA61_BSD) - */ - -procedure dataport_io_inf { - void update(in size_t n); -}; diff --git a/apps/system/system.camkes b/apps/system/system.camkes index 15dbc1c..17826c9 100644 --- a/apps/system/system.camkes +++ b/apps/system/system.camkes @@ -13,7 +13,6 @@ import ; import ; -import "interfaces/dataport_io.idl4"; import "interfaces/VectorCoreInterface.camkes"; import "interfaces/VectorCoreReturnInterface.camkes"; import "components/OpenTitanUARTDriver/OpenTitanUARTDriver.camkes"; @@ -108,12 +107,12 @@ assembly { // Connect the DebugConsole to the OpenTitanUARTDriver. connection seL4SharedData tx_channel( from debug_console.tx_dataport, to uart_driver.tx_dataport); - connection seL4RPCCall tx_call( - from debug_console.uart_tx, to uart_driver.tx); + connection seL4RPCCall write_call( + from debug_console.uart_write, to uart_driver.write); connection seL4SharedData rx_channel( from debug_console.rx_dataport, to uart_driver.rx_dataport); - connection seL4RPCCall rx_call( - from debug_console.uart_rx, to uart_driver.rx); + connection seL4RPCCall read_call( + from debug_console.uart_read, to uart_driver.read); // Connect the LoggerInterface to each component that needs to log // to the console. Note this allocates a 4KB shared memory region to