Add support for waiting for an interrupt for revoker completion. (#106)

This allows the allocator to block waiting for an interrupt from the
revoker, rather than polling. This should improve the revocation
benchmark where there isn't any work to do in other threads while
yielding.

---------

Co-authored-by: Nathaniel Wesley Filardo <nfilardo@microsoft.com>
Co-authored-by: Nathaniel Filardo <105816689+nwf-msr@users.noreply.github.com>

compile_flags.txt

@@ -35,4 +35,4 @@ riscv32-unknown-unknown
 -DCONFIG_THREADS_NUM=3
 -DREVOKABLE_MEMORY_START=0x80000000
 -DCLANG_TIDY
--DCHERIOT_INTERRUPT_NAMES=FakeInterrupt=4,
+-DCHERIOT_INTERRUPT_NAMES=FakeInterrupt=4,RevokerInterrupt=5

sdk/core/allocator/main.cc

@@ -129,6 +129,92 @@ namespace
 		return (Capability{timeout}.is_valid() && timeout->may_block());
 	}
 
+	/**
+	 * Helper to reacquire the lock after sleeping.  This assumes that
+	 * `timeout` *was* valid but may have been freed and so checks only that it
+	 * has not been invalidated across yielding.  If `timeout` remains valid
+	 * then this adds any time passed as `elapsed` to the timeout and then
+	 * tries to reacquire the lock, blocking for no longer than the remaining
+	 * time on this timeout.
+	 *
+	 * This runs with interrupts disabled to ensure that the timeout remains
+	 * valid in between checking that it is valid and reacquiring the lock.
+	 * Once the lock is held, the timeout cannot be freed concurrently.
+	 *
+	 * Returns `true` if the lock has been successfully reacquired, `false`
+	 * otherwise.
+	 */
+	[[cheri::interrupt_state(disabled)]] bool
+	reacquire_lock(Timeout                   *timeout,
+	               LockGuard<decltype(lock)> &g,
+	               Ticks                      elapsed = 0)
+	{
+		if (Capability{timeout}.is_valid())
+		{
+			timeout->elapse(elapsed);
+			return g.try_lock(timeout);
+		}
+		return false;
+	}
+
+	/**
+	 * Wait for the background revoker, if the revoker supports
+	 * interrupt-driven notifications.
+	 *
+	 * Waits until either `timeout` expires or `epoch` has finished and then
+	 * tries to reacquire the lock. Returns true if the epoch has passed and the
+	 * lock has been reacquired, returns false and does *not* reacquire the lock
+	 * in the case of any error.
+	 *
+	 */
+	template<typename T = Revocation::Revoker>
+	bool wait_for_background_revoker(
+	  Timeout                   *timeout,
+	  uint32_t                   epoch,
+	  LockGuard<decltype(lock)> &g,
+	  T &r = revoker) requires(Revocation::SupportsInterruptNotification<T>)
+	{
+		// Release the lock before sleeping
+		g.unlock();
+		// Wait for the interrupt to fire, then try to reacquire the lock if
+		// the epoch is passed.
+		return r.wait_for_completion(timeout, epoch) &&
+		       reacquire_lock(timeout, g);
+	}
+
+	/**
+	 * Wait for the background revoker, if the revoker does not support
+	 * interrupt-driven notifications.  This will yield and retry if the revoker
+	 * has not yet finished.
+	 *
+	 * Waits until either `timeout` expires or `epoch` has finished and then
+	 * tries to reacquire the lock. Returns true if the epoch has passed and the
+	 * lock has been reacquired, returns false and does *not* reacquire the lock
+	 * in the case of any error.
+	 *
+	 */
+	template<typename T = Revocation::Revoker>
+	bool wait_for_background_revoker(
+	  Timeout                   *timeout,
+	  uint32_t                   epoch,
+	  LockGuard<decltype(lock)> &g,
+	  T &r = revoker) requires(!Revocation::SupportsInterruptNotification<T>)
+	{
+		// Yield while until a revocation pass has finished.
+		while (!revoker.has_revocation_finished_for_epoch<true>(epoch))
+		{
+			// Release the lock before sleeping
+			g.unlock();
+			Timeout smallSleep{1};
+			thread_sleep(&smallSleep);
+			if (!reacquire_lock(timeout, g, smallSleep.elapsed))
+			{
+				return false;
+			}
+		}
+		return true;
+	}
+
 	/**
 	 * Malloc implementation.  Allocates `bytes` bytes of memory.  If `timeout`
 	 * is greater than zero, may block for that many ticks.  If `timeout` is the
@@ -185,32 +271,8 @@ namespace
 
 					if constexpr (Revocation::Revoker::IsAsynchronous)
 					{
-						// Yield while until a revocation pass has finished.
-						while (!revoker.has_revocation_finished_for_epoch<true>(
-						  needsRevocation->waitingEpoch))
-						{
-							g.unlock();
-							Timeout smallSleep{1};
-							thread_sleep(&smallSleep);
-							// It's possible that, while we slept,
-							// `*timeout` was freed.  Check that the pointer
-							// is still valid so that we don't fault if this
-							// happens. We are not holding the lock at this
-							// point and so we must do the check with
-							// interrupts disabled to protect against
-							// concurrent free.
-							if (!with_interrupts_disabled([&]() {
-								    if (Capability{timeout}.is_valid())
-								    {
-									    timeout->elapse(smallSleep.elapsed);
-									    return g.try_lock(timeout);
-								    }
-								    return false;
-							    }))
-							{
-								return nullptr;
-							}
-						}
+						wait_for_background_revoker(
+						  timeout, needsRevocation->waitingEpoch, g);
 					}
 					else
 					{
@@ -219,21 +281,7 @@ namespace
 						g.unlock();
 						Timeout smallSleep{0};
 						thread_sleep(&smallSleep);
-						// It's possible that, while we slept,
-						// `*timeout` was freed.  Check that the pointer
-						// is still valid so that we don't fault if this
-						// happens. We are not holding the lock at this
-						// point and so we must do the check with
-						// interrupts disabled to protect against
-						// concurrent free.
-						if (!with_interrupts_disabled([&]() {
-							    if (Capability{timeout}.is_valid())
-							    {
-								    timeout->elapse(smallSleep.elapsed);
-								    return g.try_lock(timeout);
-							    }
-							    return false;
-						    }))
+						if (!reacquire_lock(timeout, g, smallSleep.elapsed))
 						{
 							return nullptr;
 						}
@@ -267,13 +315,7 @@ namespace
 					return nullptr;
 				}
 				Debug::log("Woke from futex wake");
-				if (!with_interrupts_disabled([&]() {
-					    if (Capability{timeout}.is_valid())
-					    {
-						    return g.try_lock(timeout);
-					    }
-					    return false;
-				    }))
+				if (!reacquire_lock(timeout, g))
 				{
 					return nullptr;
 				}

sdk/core/allocator/revoker.h

@@ -40,6 +40,22 @@ namespace Revocation
 			} -> std::same_as<void>;
 	};
 
+	/**
+	 * If this revoker supports an interrupt to notify of completion then it
+	 * must have a method that blocks waiting for the interrupt to fire.  This
+	 * method should return true if the epoch has been reached or false if the
+	 * timeout expired.
+	 */
+	template<typename T>
+	concept SupportsInterruptNotification = requires(T        v,
+	                                                 Timeout *timeout,
+	                                                 uint32_t epoch)
+	{
+		{
+			v.wait_for_completion(timeout, epoch)
+			} -> std::same_as<bool>;
+	};
+
 	/**
 	 * Class for interacting with the shadow bitmap.  This bitmap controls the
 	 * behaviour of a hardware load barrier, which will invalidate capabilities

sdk/core/loader/boot.cc

@@ -538,10 +538,6 @@ namespace
 			return buildMMIO();
 		}
 
-		// Privileged compartments don't have sealed objects.
-		if constexpr (!std::is_same_v<
-		                std::remove_cvref_t<decltype(sourceCompartment)>,
-		                ImgHdr::PrivilegedCompartment>)
 		{
 			if (contains(sourceCompartment.sealedObjects, target, size))
 			{

sdk/core/loader/types.h

@@ -460,6 +460,12 @@ namespace loader
 			 */
 			AddressRange exportTable;
 
+			/**
+			 * The range of statically allocated sealed objects for this
+			 * compartment.
+			 */
+			AddressRange sealedObjects;
+
 			/**
 			 * Returns the range of the import table.
 			 */

sdk/firmware.ldscript.in

@@ -101,12 +101,19 @@ SECTIONS
 	}
 	.scheduler_globals_end = .;
 
+	.allocator_sealed_objects : CAPALIGN
+	{
+		.allocator_sealed_objects = .;
+		*/cherimcu.allocator.compartment(.sealed_objects)
+	}
+	.allocator_sealed_objects_end = .;
+
 	.allocator_globals : CAPALIGN
 	{
 		.allocator_globals = .;
 		*/cherimcu.allocator.compartment(.data .data.* .sdata .sdata.*);
 		.allocator_bss_start = .;
-		*/cherimcu.allocator.compartment(.sbss .sbss.* .bss .bss.*)
+		*/cherimcu.allocator.compartment(.sbss .sbss.* .bss .bss.*);
 	}
 	.allocator_globals_end = .;
 
@@ -169,6 +176,7 @@ SECTIONS
 		# Switcher's copy of the scheduler's CSP
 		SHORT(switcher_scheduler_entry_csp - .compartment_switcher_start);
 
+		# sdk/core/loader/types.h:/PrivilegedCompartment
 		# Scheduler code start address
 		LONG(.scheduler_start);
 		# Scheduler code end
@@ -183,7 +191,11 @@ SECTIONS
 		LONG(.scheduler_export_table);
 		# Size of the scheduler export table
 		SHORT(.scheduler_export_table_end - .scheduler_export_table);
+		# No sealed objects
+		LONG(0);
+		SHORT(0);
 
+		# sdk/core/loader/types.h:/PrivilegedCompartment
 		# Allocator code start address
 		LONG(.allocator_start);
 		# Allocator code end
@@ -198,7 +210,11 @@ SECTIONS
 		LONG(.allocator_export_table);
 		# Size of the allocator export table
 		SHORT(.allocator_export_table_end - .allocator_export_table);
+		# The allocator may have sealed objects
+		LONG(.allocator_sealed_objects);
+		SHORT(SIZEOF(.allocator_sealed_objects));
 
+		# sdk/core/loader/types.h:/PrivilegedCompartment
 		# Token server compartment header
 		# Code
 		LONG(.token_library_start);
@@ -212,6 +228,9 @@ SECTIONS
 		LONG(.token_library_export_table);
 		# Size of the token server export table
 		SHORT(.token_library_export_table_end - .token_library_export_table);
+		# No sealed objects
+		LONG(0);
+		SHORT(0);
 
 		@software_revoker_header@
 

sdk/include/platform/ibex/platform-hardware_revoker.hh

@@ -5,6 +5,8 @@
 
 #include <cdefs.h>
 #include <compartment-macros.h>
+#include <futex.h>
+#include <interrupt.h>
 #include <riscvreg.h>
 #include <stddef.h>
 #include <stdint.h>
@@ -13,6 +15,10 @@
 #	error Memory map was not configured with a revoker device
 #endif
 
+DECLARE_AND_DEFINE_INTERRUPT_CAPABILITY(revokerInterruptCapability,
+                                        RevokerInterrupt,
+                                        true,
+                                        true);
 namespace Ibex
 {
 	class HardwareRevoker
@@ -37,6 +43,21 @@ namespace Ibex
 			 * start the revoker.
 			 */
 			uint32_t control;
+			/**
+			 * Padding to ensure that the later fields are at the correct
+			 * location
+			 */
+			uint32_t unused;
+			/**
+			 * Interrupt status word.  Reading this will return 0 if an
+			 * interrupt has not been requested, 1 otherwise.
+			 */
+			uint32_t interruptStatus;
+			/**
+			 * Interrupt request word.  Writing 1 here requests an interrupt to
+			 * fire when the current revocation has completed.
+			 */
+			uint32_t interruptRequested;
 		};
 
 		/**
@@ -81,6 +102,8 @@ namespace Ibex
 #endif
 		}
 
+		static inline const uint32_t *interruptFutex;
+
 		public:
 		/**
 		 * This is an asynchronous hardware revoker.
@@ -116,6 +139,9 @@ namespace Ibex
 			                 base,
 			                 top);
 #endif
+			// Get a pointer to the futex that we use to wait for interrupts.
+			interruptFutex = interrupt_futex_get(
+			  STATIC_SEALED_VALUE(revokerInterruptCapability));
 		}
 
 		/**
@@ -161,6 +187,43 @@ namespace Ibex
 			// value because we know it will be the last value + 1.
 			epoch++;
 		}
+
+		/**
+		 * Block until the revocation epoch specified by `epoch` has completed.
+		 */
+		bool wait_for_completion(Timeout *timeout, uint32_t epoch)
+		{
+			uint32_t interruptValue;
+			do
+			{
+				// Read the current interrupt futex word.  We want to retry if
+				// an interrupt happens after this point.
+				interruptValue = *interruptFutex;
+				// Make sure that the compiler doesn't reorder the read of the
+				// futex word with respect to the read of the revocation epoch.
+				__c11_atomic_signal_fence(__ATOMIC_SEQ_CST);
+				// If the requested epoch has finished, return success.
+				if (has_revocation_finished_for_epoch<true>(epoch))
+				{
+					return true;
+				}
+				// Request the interrupt
+				revoker_device().interruptRequested = 1;
+				// There is a possible race: if the revocation pass finished
+				// before we requested the interrupt, we won't get the
+				// interrupt.  Check again before we wait.
+				if (has_revocation_finished_for_epoch<true>(epoch))
+				{
+					return true;
+				}
+				// If the epoch hasn't finished, wait for an interrupt to fire
+				// and retry.
+			} while (
+			  futex_timed_wait(timeout, interruptFutex, interruptValue) == 0);
+			// Futex wait failed.  This could be a timeout or an invalid
+			// timeout parameter, we fail either way.
+			return false;
+		}
 	};
 } // namespace Ibex
 

sdk/privileged-compartment.ldscript

@@ -57,6 +57,10 @@ SECTIONS
 		*(.bss .bss.*);
 		*(.sbss .sbss.*);
 	}
+	.sealed_objects :
+	{
+		*(.sealed_objects .sealed_objects.*);
+	}
 	# Throw some stuff away that we don't need.
 	/DISCARD/ :
 	{