VRTD clock related safety improvements

smi/src/debug/clockwiz.cpp

@@ -155,6 +155,13 @@ int Clockwiz::run(const Options& options) {
 
     if (options.getMode) {
         const uint32_t currentRate = device.getClockRate(region);
+        if (currentRate == 0) {
+            // The wizard's reconfiguration registers reset to zero and only
+            // hold what software has written, so nothing has set this clock
+            // yet. Printing a number here would be inventing one.
+            std::cout << "unconfigured\n";
+            return 0;
+        }
         printValue(currentRate, options.hexMode);
         return 0;
     }

vrt/vrtd/include/vrtd/wire.h

@@ -539,7 +539,12 @@ struct vrtd_req_clock_op {
 } __attribute__((packed));
 
 struct vrtd_resp_clock_op {
-    uint32_t rate_hz; ///< Current/achieved rate for GET/SET.
+    /**
+     * Current/achieved rate for GET/SET. For GET, 0 means the region's clock
+     * has not been configured since the device came up; it is never a
+     * realizable rate, and SET rejects it as an argument.
+     */
+    uint32_t rate_hz;
 } __attribute__((packed));
 
 /**

vrt/vrtd/libvrtdpp/include/vrtd/device.hpp

@@ -325,7 +325,8 @@ class Device {
      * @brief Get the clock rate for a region.
      *
      * @param region Clock region.
-     * @return Current rate in Hz.
+     * @return Current rate in Hz, or 0 if the region's clock has not been
+     *         configured since the device came up.
      * @throws vrtd::Error on error.
      */
     uint32_t getClockRate(ClockRegion region) const;

vrt/vrtd/src/CMakeLists.txt

@@ -34,6 +34,7 @@ add_library(vrtd_core STATIC
   ${CMAKE_CURRENT_SOURCE_DIR}/hotplug.c
   ${CMAKE_CURRENT_SOURCE_DIR}/reset.c
   ${CMAKE_CURRENT_SOURCE_DIR}/serve.c
+  ${CMAKE_CURRENT_SOURCE_DIR}/shell_build_id.c
   ${CMAKE_CURRENT_SOURCE_DIR}/signals.c
   ${CMAKE_CURRENT_SOURCE_DIR}/utils.c
 )

vrt/vrtd/src/clock.h

@@ -45,16 +45,6 @@
 // BAR index used by the clock driver.
 #define CLOCK_DRIVER_BAR_NUMBER 4
 
-/**
- * @brief Fallback BAR index for the clock wizard registers.
- *
- * TEMPORARY. The compute-only platform exposes the clock wizards
- * on BAR2 rather than BAR4. Until the platforms agree on one BAR (or the BAR
- * is discovered from metadata), the driver tries BAR4 first and falls back
- * to this one.
- */
-#define CLOCK_DRIVER_BAR_NUMBER_FALLBACK 2
-
 /**
  * @name Clock wizard region offsets within BAR4.
  * Each region contains the AXI register set for one clock wizard instance.
@@ -119,7 +109,8 @@ void clock_wizard_encode_leaf(uint32_t o, uint32_t *ctrl_out, uint32_t *counts_o
  *
  * @param ctrl   First register of the leaf pair (flags).
  * @param counts Second register of the leaf pair (high/low counts).
- * @return Effective divider ratio (never 0).
+ * @return Effective divider ratio, or 0 if the register pair does not describe
+ *         a valid divider. Callers must reject 0 rather than divide by it.
  */
 uint32_t clock_wizard_decode_leaf(uint32_t ctrl, uint32_t counts);
 
@@ -153,9 +144,15 @@ void cleanup_clock_driverp(struct clock_driver **clkp)
 
 /**
  * @brief Read the current service-region clock frequency.
+ *
+ * A rate of 0 means the wizard has not been programmed since the device came
+ * up: its reconfiguration registers reset to zero and only reflect what
+ * software has written, so there is no rate to report rather than an error.
+ *
  * @param clk          The clock driver instance.
- * @param[out] rate_hz_out Receives the current frequency in Hz.
- * @return 0 on success, -1 on error.
+ * @param[out] rate_hz_out Receives the current frequency in Hz, or 0 if the
+ *                         clock is unconfigured.
+ * @return 0 on success, -1 if the register window did not respond.
  */
 int clock_driver_get_service_region_rate_hz(struct clock_driver *clk, uint32_t *rate_hz_out);
 
@@ -175,9 +172,14 @@ int clock_driver_set_service_region_rate_hz(struct clock_driver *clk, uint32_t *
 
 /**
  * @brief Read the current user-region clock frequency.
+ *
+ * As with the service region, a rate of 0 means unconfigured rather than
+ * failed. See @c clock_driver_get_service_region_rate_hz.
+ *
  * @param clk          The clock driver instance.
- * @param[out] rate_hz_out Receives the current frequency in Hz.
- * @return 0 on success, -1 on error.
+ * @param[out] rate_hz_out Receives the current frequency in Hz, or 0 if the
+ *                         clock is unconfigured.
+ * @return 0 on success, -1 if the register window did not respond.
  */
 int clock_driver_get_user_region_rate_hz(struct clock_driver *clk, uint32_t *rate_hz_out);
 

vrt/vrtd/src/reset.c

@@ -98,6 +98,7 @@
 
 #include "device.h"
 #include "hotplug.h"
+#include "shell_build_id.h"
 #include "utils.h"
 
 #define GPIO_ALLOW_SBR 0x1040000
@@ -497,6 +498,21 @@ uint16_t reset_with_ami_partition_progress(
     for (size_t i = 0; i < devices->len; i++) {
         struct device *new_device = devices->d[i];
         if (new_device != NULL && strcmp(new_device->pci_info.bdf, target_bdf) == 0) {
+            /*
+             * The boot partition states which shell was intended. Confirm the
+             * device agrees before recording it: a partition that did not take
+             * effect would otherwise leave vrtd asserting a shell the hardware
+             * is not running, and every later decision keyed on the shell —
+             * whether a reset is required, which register windows exist — would
+             * be made against the wrong design.
+             */
+            if (build_id_check_shell(
+                    new_device->bar_files[BUILD_ID_BAR_NUMBER],
+                    booted_shell,
+                    "reset_with_ami"
+                ) != 0) {
+                return VRTD_RET_INTERNAL_ERROR;
+            }
             new_device->current_shell = booted_shell;
             break;
         }

vrt/vrtd/src/serve.c

@@ -138,6 +138,7 @@
 #include "hotplug.h"
 #include "reset.h"
 #include "serve.h"
+#include "shell_build_id.h"
 #include "utils.h"
 #include "state.h"
 #include "vrtd/wire.h"
@@ -448,6 +449,16 @@ static uint16_t device_refresh_pf2_after_design_write(struct device *d)
         }
     }
 
+    /*
+     * The clock driver borrows d->ctl and holds its own mmap of the BAR window
+     * carrying the clock wizard registers. Both were established against the
+     * pre-PDI PF2 and are invalidated by the remove/rescan above. Tear the
+     * driver down before the handle it borrows, matching the order used by
+     * device_destroy().
+     */
+    cleanup_clock_driver(d->clock_driver);
+    d->clock_driver = NULL;
+
     /*
      * The stable character-device path survives PF2 remove+rescan, but the
      * existing handle still refers to the pre-PDI device. Close it and reopen
@@ -494,6 +505,28 @@ static uint16_t device_refresh_pf2_after_design_write(struct device *d)
         }
     }
 
+    /*
+     * A design write reconfigures the user region only; it cannot change the
+     * shell. If the device now reports a different shell than the one vrtd
+     * believes is loaded, the BAR window is not addressing the static shell we
+     * think it is, and every subsequent register access — starting with the
+     * clock driver recreated below — would be aimed at the wrong fabric.
+     */
+    if (build_id_check_shell(
+            d->bar_files[BUILD_ID_BAR_NUMBER],
+            d->current_shell,
+            "device_refresh_pf2"
+        ) != 0) {
+        return VRTD_RET_INTERNAL_ERROR;
+    }
+
+    /* Re-establish the clock driver against the freshly-probed PF2. */
+    d->clock_driver = clock_driver_create(d->ctl);
+    if (d->clock_driver == NULL) {
+        LOG(LOG_ERR, "device_refresh_pf2: failed to recreate clock driver on %s: %m", d->path);
+        return VRTD_RET_INTERNAL_ERROR;
+    }
+
     return VRTD_RET_OK;
 }
 
@@ -1437,8 +1470,14 @@ static int client_finalize_pending_design_write(struct client *client)
     uint16_t design_write_ret = VRTD_RET_OK;
     if (transfer_error == 0) {
         design_write_ret = device_refresh_pf2_after_design_write(d);
-        LOG(LOG_INFO, "Design write completed successfully for uid=%u conn_id=%llu",
-            (unsigned int)client->uid, (unsigned long long)client->conn_id);
+        if (design_write_ret == VRTD_RET_OK) {
+            LOG(LOG_INFO, "Design write completed successfully for uid=%u conn_id=%llu",
+                (unsigned int)client->uid, (unsigned long long)client->conn_id);
+        } else {
+            LOG(LOG_ERR, "Design write transferred but PF2 refresh failed (ret=%u) for "
+                "uid=%u conn_id=%llu", (unsigned int)design_write_ret,
+                (unsigned int)client->uid, (unsigned long long)client->conn_id);
+        }
     } else {
         LOG(LOG_WARNING, "Design write failed (error=%d) for uid=%u conn_id=%llu",
             transfer_error, (unsigned int)client->uid, (unsigned long long)client->conn_id);

vrt/vrtd/src/shell_build_id.c

@@ -0,0 +1,112 @@
+/**
+ * The MIT License (MIT)
+ * Copyright (c) 2026 Advanced Micro Devices, Inc. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of this software
+ * and associated documentation files (the "Software"), to deal in the Software without restriction,
+ * including without limitation the rights to use, copy, modify, merge, publish, distribute,
+ * sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all copies or
+ * substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT
+ * NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * @file shell_build_id.c
+ * @brief Implementation of the shell build-ID shell-variant check.
+ */
+
+#include "shell_build_id.h"
+
+#include <errno.h>
+#include <stddef.h>
+#include <syslog.h>
+
+#include <systemd/sd-journal.h>
+
+#include "utils.h"
+
+/*
+ * An MMIO read that does not reach the device completes with every bit set.
+ * The build-ID high word never holds this value: its reserved bits are always
+ * clear.
+ */
+#define BUILD_ID_REG_UNRESPONSIVE 0xFFFFFFFFu
+
+const char *build_id_shell_name(enum vrtd_shell_type shell)
+{
+    switch (shell) {
+    case VRTD_SHELL_SERVICE:
+        return "service";
+    case VRTD_SHELL_COMPUTE:
+        return "compute";
+    default:
+        return "unknown";
+    }
+}
+
+enum vrtd_shell_type build_id_decode_shell(uint32_t hi)
+{
+    if (hi == BUILD_ID_REG_UNRESPONSIVE || (hi & BUILD_ID_HI_RESERVED_MASK) != 0u) {
+        return VRTD_SHELL_UNKNOWN;
+    }
+
+    return (hi & BUILD_ID_HI_SHELL_MASK) != 0u ? VRTD_SHELL_COMPUTE : VRTD_SHELL_SERVICE;
+}
+
+enum vrtd_shell_type build_id_read_shell(const struct slash_bar_file *bar)
+{
+    if (bar == NULL || bar->map == NULL || bar->len < BUILD_ID_REG_HI + sizeof(uint32_t)) {
+        return VRTD_SHELL_UNKNOWN;
+    }
+
+    const volatile uint32_t *regs = (const volatile uint32_t *) bar->map;
+    return build_id_decode_shell(regs[BUILD_ID_REG_HI / sizeof(uint32_t)]);
+}
+
+int build_id_check_shell(
+    const struct slash_bar_file *bar,
+    enum vrtd_shell_type expected,
+    const char *context
+)
+{
+    if (expected == VRTD_SHELL_UNKNOWN) {
+        return 0;
+    }
+
+    enum vrtd_shell_type reported = build_id_read_shell(bar);
+    if (reported == VRTD_SHELL_UNKNOWN) {
+        LOG(
+            LOG_ERR,
+            "%s: shell build-ID register at BAR%d+0x%x did not respond; cannot confirm "
+            "the %s shell is loaded",
+            context,
+            BUILD_ID_BAR_NUMBER,
+            BUILD_ID_REG_HI,
+            build_id_shell_name(expected)
+        );
+        errno = EIO;
+        return -1;
+    }
+
+    if (reported != expected) {
+        LOG(
+            LOG_ERR,
+            "%s: hardware reports the %s shell but vrtd expected the %s shell",
+            context,
+            build_id_shell_name(reported),
+            build_id_shell_name(expected)
+        );
+        errno = EIO;
+        return -1;
+    }
+
+    return 0;
+}