sw: Refactored pinmux driver #354
Conversation
The changes allow for easier access control and compartmentalisation.
There was a problem hiding this comment.
This looks like a big improvement to me, thanks for working on this @HU90m! Just a few comments.
I've tested with the manual pinmux check procedure and that still works fine. The simple pinmux check is broken however, due to some slightly incorrect logic when I wrote that (I wait for the transmit FIFO to be empty, but not necessarily for transmission to finish). The change from using pinmux.select...() to sink.disable() actually optimises the pinmux enough that it causes this bug to manifest. I fixed it with the following patch:
diff --git a/sw/cheri/checks/pinmux_check.cc b/sw/cheri/checks/pinmux_check.cc
index e6d71535..be8cd027 100644
--- a/sw/cheri/checks/pinmux_check.cc
+++ b/sw/cheri/checks/pinmux_check.cc
@@ -25,9 +25,7 @@ using namespace CHERI;
* Blocks until the UART transmit FIFO is empty.
*/
void block_until_uart_tx_done(Capability<volatile OpenTitanUart> uart) {
- while (uart->transmit_fifo_level() > 0) {
- asm volatile("");
- }
+ while (!(uart->status & uart->StatusTransmitIdle));
}
/**Haven't had a chance to run the RS485 checks but I think those only use Pinmux for the initial configuration so they should be fine.
| Debug::log("Selected option is not valid for this block."); | ||
| bool select(uint8_t source) { | ||
| if (source >= sources_number(sink)) { | ||
| Debug::log("Selected source not within the range of valid sources."); |
There was a problem hiding this comment.
Just a quick thought while we're making changes: it might be nicer to have this debug log actually report the maximum value, so that a user can match it against what they expect to see in the docs and more quickly/easily notice uses of the wrong pins, or outdated docs/drivers. Maybe something like "Selected source {source} not within the range of {num_sources} valid sources for sink at offset {sink}"?
| inline Sink<SinkEnum> _get_sink(volatile uint8_t *base_register, const SinkEnum sink) { | ||
| CHERI::Capability reg = {base_register + static_cast<ptrdiff_t>(sink)}; | ||
| reg.bounds() = sizeof(uint8_t); | ||
| return Sink<SinkEnum>{reg, sink}; |
There was a problem hiding this comment.
Out of curiosity, how do you plan to handle this in the RTOS version of the driver? Currently I think you'd have to get a capability to the entire Pinmux range and then modify that to only the range of the specific sink selector register, and so while the compartmentalisation would be nicer, auditing would still show access to the entire Pinmux MMIO region even if only a small subset is being used.
Or is the idea that you would just have one isolated auditable compartment that gets the entire capability, and provides specific sink capabilities to other compartments?
There was a problem hiding this comment.
There will be two contiguous MMIO regions described in the board file: block sinks and pin sinks. Any compartment could request access to either or both. This can be audited normally.
Yes, the idea of having {Pin,Block}Sinks::get() return a capability with a bound only covering the particular sink is that this allows a compartment to give another compartment permission to configure the particular sink, without giving access to the whole region.
There was a problem hiding this comment.
I see, that makes sense. I think it would be nicer if for auditing purposes no compartments were ever made to get access to the full MMIO regions, and instead only had to get access to the pins required, but I suppose that would either require defining 155 sink devices in the board description file, or (preferably) changes in upstream cheriot-rtos to support this kind of granular capability loading functionality (e.g. MMIO_CAPABILITY(uint8_t, pinmux, offset=0x012, bounds=0x8) would be nice for example).
The names in the driver can't have square brackets for IO indices.
Check only waited for an empty fifo and not for the transmission to be finished. New pinmux driver allowed optimisations caused the check to break because of this. Co-authored-by: Alex Jones <[email protected]>
|
Thanks for the speedy and thorough review Alex! |
The changes allow for easier access control and compartmentalisation.
Also the pinmux block input documentation names to match the driver. The names in the driver can't have square brackets for IO indices.