[protocol] Modify DFU update to skip #172

esnguyen · 2025-12-22T17:18:50Z

No description provided.

stevenportley · 2025-12-22T18:23:30Z

examples/htool_dfu.c

 #include "protocol/dfu_hostcmd.h"
+#include "protocol/opentitan_version.h"
+
+void process_image(struct libhoth_device* dev, const uint8_t* image, struct opentitan_image_version* rom_ext, struct opentitan_image_version* app, struct opentitan_get_version_resp * resp ) {


I think it might be best to separate the processing of the file from fetching the version from the device. This way we can re-use it for other htool commands that just want to inspect the contents of a file.

Maybe rename this to something like: libhoth_extract_ot_bundle and move it into libhoth/protocol.

stevenportley · 2025-12-22T18:23:42Z

examples/htool_dfu.c

+  app->minor = image[offset_app + OPENTITAN_OFFSET_VERSION_MINOR] | image[offset_app + OPENTITAN_OFFSET_VERSION_MINOR + 1] << 8 | image[offset_app + OPENTITAN_OFFSET_VERSION_MINOR + 2] << 16 | image[offset_app + OPENTITAN_OFFSET_VERSION_MINOR + 3] << 24;
+
+  // Get the current version of the device
+  libhoth_opentitan_version(dev, resp);


I think you're dropping potential errors here.

stevenportley · 2025-12-22T18:25:06Z

examples/htool_dfu.c

+  // Process the current boot slot
+  uint32_t boot_slot = resp.primary_bl0_slot == kOpentitanBootSlotA ? 0 : 1;
+
+  if(resp.rom_ext.slots[boot_slot].major != rom_ext.major || resp.rom_ext.slots[boot_slot].minor != rom_ext.minor || resp.app.slots[boot_slot].major != app.major || resp.app.slots[boot_slot].minor != app.minor) {


Turn some of this conditional into a helper function, libhoth_ot_version_eq, that just compares opentitan_image_version for equality.

stevenportley · 2025-12-22T18:29:08Z

examples/htool_dfu.c

-    fprintf(stderr, "DFU update failed.\n");
-    goto cleanup2;
+  // Process the current boot slot
+  uint32_t boot_slot = resp.primary_bl0_slot == kOpentitanBootSlotA ? 0 : 1;


I think the logic that we want is to compare both boot slots and do what is necessary for both boot slots to be this new version.

If both boot slots match this new image, do nothing. If only the current boot slot already has the image, just update once. Otherwise, perform the dfu_update twice.

stevenportley · 2025-12-22T18:32:57Z

examples/htool_dfu.c

+    }
+  }
+  else {
+    printf("Version match\n");


We should probably make the messages here a little more informative. Something like:
"Image version {version} already matches running OT version, skipping DFU update..."

@stevenportley My logic has changed so I am not printing a msg. let me know if this message is still desired.

stevenportley · 2025-12-22T18:34:33Z

examples/htool_dfu.c

+  }
+  else {
+    printf("Version match\n");
+    return 0;


We don't want to exit immediately because we still need to run some of the cleanup logic for unmapping the file.

stevenportley · 2025-12-22T18:35:48Z

examples/htool_dfu.c

-  if (ret != 0) {
-    fprintf(stderr, "close error: %d\n", ret);
-  }
+  goto cleanup2;


I don't think you need these gotos

Will revisit this if needed. Logic change so please review this again if this is needed

For htools dfu, we seek to add some type of logic to skip an update. With the opentitan get version, we can compare the versions and make the decision to proceed or return with no actions. Signed-off-by: Ellis Sarza-Nguyen <sarzanguyen@google.com>

stevenportley · 2025-12-23T18:56:43Z

protocol/opentitan_version.c

+
+  // Extract the offset that contains the APP version information
+  // We will have the desired APP version be stored on slot index 0 and keep slot index 1 empty
+  uint32_t offset_app = offset + 65536;


We should name this constant

stevenportley · 2025-12-23T20:12:26Z

protocol/opentitan_version.c

+  return 0;
+}
+
+int libhoth_ot_version_eq(const struct opentitan_get_version_resp * a,


I think this should look more like:

int libhoth_ot_version_eq(const struct opentitan_image_version* a, const struct opentitan_image_version* b)

and just do the major / minor comparison

stevenportley · 2025-12-23T20:20:33Z

protocol/opentitan_version.c

  return rv;
 }

+int libhoth_extract_ot_bundle(const uint8_t* image,


You can't really extra the bundle file into an struct opentitan_get_version_resp, because they're logically different things:

The fwupdate bundle contains a ROM_EXT and APP firmware.
The struct opentitan_get_version_resp contains information about the state of the OT chip, what it booted, and some metadata (preferred boot slot, owner_config, etc.).

As a result, you have to stub a bunch of extra data here with default/invalid values and check for them in other places. I think it would make more sense to have something like:

int libhoth_extract_ot_bundle(const uint8_t* image, struct opentitan_image_version* rom_ext, struct opentitan_image_version* app_fw)

and then you don't need the stubs and the other checks for 0xDEADBEEF

stevenportley · 2025-12-23T20:24:02Z

examples/htool_dfu.c

-    goto cleanup2;
+  int boot_count = 0;
+  uint32_t current_slot = bootslot_int(resp.primary_bl0_slot);
+  uint32_t next_slot = (current_slot == 0) ? 1 : 0;


One subtlety here is that if we are currently booting the correct FW but the staging side is wrong, we only need to update once to fix the staging side.

However, if the staging side is correct but the currently running firmware is wrong, we still need to dfu update twice. The first will get us to boot into the other side, and the second will do that actual update.

stevenportley · 2025-12-23T20:26:37Z

examples/htool_dfu.c

-    fprintf(stderr, "DFU update failed.\n");
-    goto cleanup2;
+  int boot_count = 0;
+  uint32_t current_slot = bootslot_int(resp.primary_bl0_slot);


I think what you're looking for here is actually booted_slot in opentitan_image_boot_info

esnguyen force-pushed the esnguyen/add_dfu_skip_logic branch 2 times, most recently from 75d8434 to d2de66b Compare December 22, 2025 17:22

esnguyen requested review from korran and stevenportley December 22, 2025 17:23

stevenportley requested changes Dec 22, 2025

View reviewed changes

esnguyen force-pushed the esnguyen/add_dfu_skip_logic branch from d2de66b to ca07f28 Compare December 23, 2025 18:15

esnguyen requested a review from stevenportley December 23, 2025 18:15

stevenportley requested changes Dec 23, 2025

View reviewed changes

[protocol] Modify DFU update to skip #172

Are you sure you want to change the base?

[protocol] Modify DFU update to skip #172

Conversation

esnguyen commented Dec 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants