Setting the transient pref. to current active slot clears it.

Given the current API for setting the transient boot preference there is
an implementation choice. While setting the transient selection to the
alternate image (currently non-active) is the common use case, we also
need to define what happens when the transient preference is specified
to be the currently active RoT Hubris image.

The simple case is that setting transient to active will clear the
transient selection. That is the choice this PR makes.

The other case, setting an actual transient preference to the active
gives rise to some more complex behavior that is just annoying.

Assume that A is active, B is the pending persistent preference, and A
is also the transient preference.

On the first reset, the ROM will promote the CFPA scratch page to
active, thus making B the persistent preference. However, Bootleby will
use the transient preference of A to select the active image. The RoT
will then be running Hubris image A. Bootleby has cleared the transient
selection.

On the second reset, there is no transient selection or pending
persistent. Bootleby will select Hubris image B.

These scenarios have been tested and analyzed and there is no motivation
found for enabling this sequence. If, after merging this PR, it is found
that we do want to enable this sequence of two resets in a row running
different images, we can change the API to an enum of `Select(SlotId)`
or `ClearSelection`.

---

Hubris issue 2093 was found in testing where setting the persistent
preference to different values in succession leads to the inability to
clear a pending persistent boot preference. This may be needed to
recover from a previous failed/abandoned RoT Hubris update where one
wants to avoid additional reset operations.
This is a corner case with a workaround, but it is also against the
intent of the API.

Author: lzrd

Cargo.lock

@@ -44,9 +44,6 @@ path = "sys/abi"
 [patch."https://github.com/oxidecomputer/hubris".counters]
 path = "lib/counters"
 
-[patch."https://github.com/oxidecomputer/management-gateway-service".gateway-messages]
-path = "/home/stoltz/Oxide/src/management-gateway-service/gateway-messages"
-
 [workspace.dependencies]
 anyhow = { version = "1.0.31", default-features = false, features = ["std"] }
 array-init = { version = "2.1.0" }

Cargo.toml

@@ -501,38 +501,44 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> {
         let image = match (component, slot) {
             (RotComponent::Hubris, SlotId::A)
             | (RotComponent::Hubris, SlotId::B) => {
-                let active = match bootstate().map_err(|_| UpdateError::MissingHandoffData)?.active {
+                let active = match bootstate()
+                    .map_err(|_| UpdateError::MissingHandoffData)?
+                    .active
+                {
                     stage0_handoff::RotSlot::A => SlotId::A,
                     stage0_handoff::RotSlot::B => SlotId::B,
                 };
                 if active == slot {
                     return Err(UpdateError::InvalidSlotIdForOperation.into());
                 }
-                // Since we will be enforcing rollback protection at the time when the
-                // boot preference is set, we cannot yet have the alternate image as the
-                // preferred image. The full image needs to be in place in order to
-                // evaluate the policy.
-                let (persistent, pending_persistent, transient) = self.boot_preferences()?;
+                // Rollback protection will be implemented by refusing to set
+                // boot preference to an image that has a lower EPOC vale in the
+                // caboose.
+                //
+                // Setting the boot preference before updating would sidestep that
+                // protection. So, we will fail the prepare step if any
+                // preference settings are selecting the update target image.
+                let (persistent, pending_persistent, transient) =
+                    self.boot_preferences()?;
                 if let Some(pref) = transient {
                     if active != pref {
                         return Err(UpdateError::InvalidPreferredSlotId.into());
                     }
                 }
                 if let Some(pref) = pending_persistent {
                     if active != pref {
-                        return Err(UpdateError::InvalidPreferredSlotId.into())
+                        return Err(UpdateError::InvalidPreferredSlotId.into());
                     }
                 }
                 if active != persistent {
-                        return Err(UpdateError::InvalidPreferredSlotId.into())
+                    return Err(UpdateError::InvalidPreferredSlotId.into());
                 }
                 Some((component, slot))
             }
             (RotComponent::Stage0, SlotId::B) => Some((component, slot)),
             _ => return Err(UpdateError::InvalidSlotIdForOperation.into()),
         };
 
-
         self.image = image;
         self.state = UpdateState::InProgress;
         ringbuf_entry!(Trace::State(self.state));
@@ -938,13 +944,32 @@ impl ServerImpl<'_> {
         slot: SlotId,
         duration: SwitchDuration,
     ) -> Result<(), RequestError<UpdateError>> {
+        // Note: Rollback policy will be checking epoch values before activating.
         match duration {
             SwitchDuration::Once => {
-                // TODO check Rollback policy vs epoch before activating.
-                // TODO: prep-image-update should clear transient selection.
-                //   e.g. update, activate, update, reboot should not have
-                //   transient boot set.
-                set_hubris_transient_override(Some(slot));
+                // Get the currently active slot.
+                let active_slot = match bootstate()
+                    .map_err(|_| UpdateError::MissingHandoffData)?
+                    .active
+                {
+                    stage0_handoff::RotSlot::A => SlotId::A,
+                    stage0_handoff::RotSlot::B => SlotId::B,
+                };
+
+                // If the requested slot is the same as the active slot,
+                // treat this as a request to CLEAR the transient override.
+                //
+                // If we did allow setting the active slot as the transient
+                // preference, then we get this confusing 2-boot scenario when
+                // pending persistent preference is also used:
+                // the next reset returns us to the original image and the 2nd
+                // reset has us running the new/alternate image.
+                if active_slot == slot {
+                    set_hubris_transient_override(None);
+                } else {
+                    // Otherwise, set the preference as requested.
+                    set_hubris_transient_override(Some(slot));
+                }
             }
             SwitchDuration::Forever => {
                 // Locate and return the authoritative CFPA flash word number
@@ -976,6 +1001,19 @@ impl ServerImpl<'_> {
                 let (cfpa_word_number, _) =
                     self.cfpa_word_number_and_version(CfpaPage::Active)?;
 
+                // TODO: Hubris issue #2093: If there is a pending update in the
+                // scratch page, it is not being considered.
+                // Consider:
+                //   - A is active
+                //   - persistent switch to B without reset
+                //       - scratch page is updated
+                //       - return Ok(())
+                //   - persistent switch to A without reset
+                //       - A is already active, no work performed
+                //       - return Ok(())
+                //   - reset
+                //   - B is active
+
                 // Read current CFPA contents.
                 let mut cfpa = [[0u32; 4]; 512 / 16];
                 indirect_flash_read_words(