diff --git a/smi/src/debug/clockwiz.cpp b/smi/src/debug/clockwiz.cpp index 93cf600d..87d94d54 100644 --- a/smi/src/debug/clockwiz.cpp +++ b/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; } diff --git a/vrt/vrtd/include/vrtd/wire.h b/vrt/vrtd/include/vrtd/wire.h index 996f95a0..89e3ef35 100644 --- a/vrt/vrtd/include/vrtd/wire.h +++ b/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)); /** diff --git a/vrt/vrtd/libvrtdpp/include/vrtd/device.hpp b/vrt/vrtd/libvrtdpp/include/vrtd/device.hpp index 40588e16..5cf72e20 100644 --- a/vrt/vrtd/libvrtdpp/include/vrtd/device.hpp +++ b/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; diff --git a/vrt/vrtd/src/CMakeLists.txt b/vrt/vrtd/src/CMakeLists.txt index 66a58c82..63fa4923 100644 --- a/vrt/vrtd/src/CMakeLists.txt +++ b/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 ) diff --git a/vrt/vrtd/src/clock.c b/vrt/vrtd/src/clock.c index 6daa81fa..b5d67a3f 100644 --- a/vrt/vrtd/src/clock.c +++ b/vrt/vrtd/src/clock.c @@ -75,11 +75,11 @@ * Xilinx Clocking Wizard AXI register offsets. * Sourced from xclk_wiz_hw.h in the Xilinx driver headers. */ +#define XCLK_WIZ_STATUS_OFFSET 0x00000004u /* Status register; bit 0 reports PLL/MMCM lock */ #define XCLK_WIZ_RECONFIG_OFFSET 0x00000014u /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG1_OFFSET 0x00000330u /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG2_OFFSET 0x00000334u /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG3_OFFSET 0x00000338u /* TODO(vserbu): explain this register offset/bit field */ -#define XCLK_WIZ_REG4_OFFSET 0x0000033Cu /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG11_OFFSET 0x00000378u /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG12_OFFSET 0x00000380u /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG13_OFFSET 0x00000384u /* TODO(vserbu): explain this register offset/bit field */ @@ -94,7 +94,7 @@ /* * Bit masks and shift constants for clock wizard register fields. */ -#define XCLK_WIZ_LOCK 0x1u /* Lock status bit in REG4 */ +#define XCLK_WIZ_LOCK 0x1u /* Lock bit in the status register */ #define XCLK_WIZ_RECONFIG_LOAD 0x1u /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_RECONFIG_SADDR 0x2u /* TODO(vserbu): explain this register offset/bit field */ @@ -105,6 +105,13 @@ #define XCLK_WIZ_EDGE_MASK (1u << 10) /* TODO(vserbu): explain this register offset/bit field */ +/* + * An MMIO read that does not reach the device completes with every bit set. + * None of the wizard registers this driver reads has all 32 bits defined, so + * the pattern identifies a register window that is not responding. + */ +#define XCLK_WIZ_REG_UNRESPONSIVE 0xFFFFFFFFu + #define XCLK_WIZ_REG3_PREDIV2 (1u << 11) /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG3_USED (1u << 12) /* TODO(vserbu): explain this register offset/bit field */ #define XCLK_WIZ_REG3_MX (1u << 9) /* TODO(vserbu): explain this register offset/bit field */ @@ -172,17 +179,14 @@ static int clock_driver_init(struct clock_driver *clk, struct slash_ctldev *ctl) }; /* - * Open the BAR that holds the clock wizard register windows. - * - * TEMPORARY. Try BAR4 (service/legacy layout); if it is not - * present, fall back to BAR2 (compute-only platform layout). Drop the - * fallback once both platforms use the same BAR. + * Open the BAR that holds the clock wizard register windows. Both the + * compute and the service shell map the wizards into the static shell + * window on BAR4, at the same offsets, so there is exactly one BAR to + * open and a failure to open it is a failure to construct the driver. */ clk->bar = slash_bar_file_open(ctl, CLOCK_DRIVER_BAR_NUMBER, O_CLOEXEC); if (clk->bar == NULL) { - clk->bar = slash_bar_file_open(ctl, CLOCK_DRIVER_BAR_NUMBER_FALLBACK, O_CLOEXEC); - } - if (clk->bar == NULL) { + LOG(LOG_ERR, "clock_driver: failed to open BAR%d: %m", CLOCK_DRIVER_BAR_NUMBER); return -1; } @@ -292,6 +296,41 @@ static inline void clock_driver_w32(struct clock_driver *clk, uint32_t offset, u clk->regs[offset / sizeof(uint32_t)] = value; } +/** + * Write a 32-bit value to a clock wizard register and confirm that it reads + * back unchanged. + * + * The M, D and O registers are plain read/write storage, so a readback that + * differs from the value written means the register window did not accept the + * access - typically because the BAR mapping no longer refers to the live + * device. Detecting that at the point of the write keeps a stale mapping from + * being mistaken for a successful reconfiguration. + * + * @param clk Clock driver with valid regs pointer. + * @param offset Byte offset into the BAR. + * @param value Value to write. + * @return 0 if the readback matches, -1 with errno set to EIO otherwise. + */ +static int clock_driver_w32_verify(struct clock_driver *clk, uint32_t offset, uint32_t value) +{ + clock_driver_w32(clk, offset, value); + + uint32_t readback = clock_driver_r32(clk, offset); + if (readback != value) { + LOG( + LOG_ERR, + "clock_driver: register 0x%08x read back as 0x%08x after writing 0x%08x", + offset, + readback, + value + ); + errno = EIO; + return -1; + } + + return 0; +} + /** * Compute the absolute BAR offset for a register within a given clock * wizard instance. @@ -314,6 +353,21 @@ static int clock_driver_check_wizard_bounds(const struct clock_driver *clk, uint return clock_driver_check_bounds(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG26_OFFSET)); } +/** + * Outcome of reading a clock configuration out of the wizard registers. + * + * The wizard's reconfiguration register file resets to zero and only reflects + * values software has written into it; the frequency a design comes up with is + * fixed at synthesis and is not readable through it. Cleared registers are + * therefore an ordinary state meaning "no rate has been programmed", and are + * distinct from a register window that is not answering at all. + */ +enum clock_read { + CLOCK_READ_OK = 0, /**< Registers hold a valid configuration. */ + CLOCK_READ_UNCONFIGURED = 1, /**< Registers are cleared; nothing programmed yet. */ + CLOCK_READ_FAULT = -1, /**< Register window did not respond. */ +}; + /** * Read the current VCO frequency from the clock wizard registers. * @@ -329,37 +383,62 @@ static int clock_driver_check_wizard_bounds(const struct clock_driver *clk, uint * * @param clk Clock driver. * @param wizard_offset Base offset of the wizard instance in BAR4. - * @return VCO frequency in Hz. + * @param[out] fvco_out Receives the VCO frequency in Hz, or 0 unless the + * result is @c CLOCK_READ_OK. + * @return See @c enum clock_read. */ -static uint64_t clock_driver_get_vco_hz(struct clock_driver *clk, uint32_t wizard_offset) +static enum clock_read clock_driver_get_vco_hz( + struct clock_driver *clk, + uint32_t wizard_offset, + uint64_t *fvco_out +) { + *fvco_out = 0; + /* Read the multiplier (M) from REG1 (edge bit) and REG2 (low/high counts). */ uint32_t reg = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG1_OFFSET)); + if (reg == XCLK_WIZ_REG_UNRESPONSIVE) { + return CLOCK_READ_FAULT; + } uint32_t edge = (reg & XCLK_WIZ_REG1_EDGE_MASK) ? 1u : 0u; /* TODO(vserbu): explain this register offset/bit field */ reg = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG2_OFFSET)); + if (reg == XCLK_WIZ_REG_UNRESPONSIVE) { + return CLOCK_READ_FAULT; + } uint32_t low = reg & XCLK_WIZ_CLKFBOUT_L_MASK; uint32_t high = (reg & XCLK_WIZ_CLKFBOUT_H_MASK) >> XCLK_WIZ_CLKFBOUT_H_SHIFT; uint32_t mult = low + high + edge; - if (mult == 0) { - mult = 1; - } /* Read the input divider (D) from REG13 (low/high counts) and REG12 (edge bit). */ reg = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG13_OFFSET)); + if (reg == XCLK_WIZ_REG_UNRESPONSIVE) { + return CLOCK_READ_FAULT; + } low = reg & XCLK_WIZ_CLKFBOUT_L_MASK; high = (reg & XCLK_WIZ_CLKFBOUT_H_MASK) >> XCLK_WIZ_CLKFBOUT_H_SHIFT; reg = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG12_OFFSET)); + if (reg == XCLK_WIZ_REG_UNRESPONSIVE) { + return CLOCK_READ_FAULT; + } edge = (reg & XCLK_WIZ_EDGE_MASK) ? 1u : 0u; /* TODO(vserbu): explain this register offset/bit field */ uint32_t div = low + high + edge; - if (div == 0) { - div = 1; + + /* + * A zero multiplier or divider is not a valid configuration. Substituting + * 1 for either would turn cleared registers into a plausible-looking + * frequency equal to the reference clock, which is how an unprogrammed + * wizard came to be reported as a working 100 MHz. + */ + if (mult == 0u || div == 0u) { + return CLOCK_READ_UNCONFIGURED; } /* f_VCO = f_primary_in * M / D */ - return ((uint64_t)clk->prim_in_hz * mult) / div; + *fvco_out = ((uint64_t)clk->prim_in_hz * mult) / div; + return CLOCK_READ_OK; } /** @@ -410,9 +489,8 @@ uint32_t clock_wizard_decode_leaf(uint32_t ctrl, uint32_t counts) uint32_t high = (counts & XCLK_WIZ_CLKFBOUT_H_MASK) >> XCLK_WIZ_CLKFBOUT_H_SHIFT; uint32_t leaf = high + low + edge; - uint32_t divo = (prediv + 1u) * leaf + (prediv * p5en); - return (divo == 0u) ? 1u : divo; + return (prediv + 1u) * leaf + (prediv * p5en); } /** @@ -428,18 +506,40 @@ uint32_t clock_wizard_decode_leaf(uint32_t ctrl, uint32_t counts) * @param wizard_offset Base offset of the wizard instance. * @param clock_id Output clock index (0-based). Outputs 0-2 use REG3-based * offsets; outputs 3+ use REG19-based offsets. - * @return Output frequency in Hz. + * @param[out] rate_out Receives the output frequency in Hz, or 0 unless the + * result is @c CLOCK_READ_OK. + * @return See @c enum clock_read. */ -static uint64_t clock_driver_get_rate_hz(struct clock_driver *clk, uint32_t wizard_offset, uint32_t clock_id) +static enum clock_read clock_driver_get_rate_hz( + struct clock_driver *clk, + uint32_t wizard_offset, + uint32_t clock_id, + uint64_t *rate_out +) { - uint64_t fvco = clock_driver_get_vco_hz(clk, wizard_offset); + *rate_out = 0; + + uint64_t fvco = 0; + enum clock_read vco_status = clock_driver_get_vco_hz(clk, wizard_offset, &fvco); + if (vco_status != CLOCK_READ_OK) { + return vco_status; + } uint32_t reg_off = clock_driver_reg(wizard_offset, clock_wizard_leaf_offset(clock_id)); uint32_t ctrl = clock_driver_r32(clk, reg_off); uint32_t counts = clock_driver_r32(clk, reg_off + 4u); + if (ctrl == XCLK_WIZ_REG_UNRESPONSIVE || counts == XCLK_WIZ_REG_UNRESPONSIVE) { + return CLOCK_READ_FAULT; + } + + uint32_t divo = clock_wizard_decode_leaf(ctrl, counts); + if (divo == 0u) { + return CLOCK_READ_UNCONFIGURED; + } - return fvco / clock_wizard_decode_leaf(ctrl, counts); + *rate_out = fvco / divo; + return CLOCK_READ_OK; } /** @@ -478,10 +578,7 @@ static void clock_driver_log_state( const char *stage ) { - uint32_t leaf_off = (clock_id < 3) - ? (XCLK_WIZ_REG3_OFFSET + clock_id * 8u) - : (XCLK_WIZ_REG19_OFFSET + clock_id * 8u); - uint32_t leaf_reg_off = clock_driver_reg(wizard_offset, leaf_off); + uint32_t leaf_reg_off = clock_driver_reg(wizard_offset, clock_wizard_leaf_offset(clock_id)); uint32_t reg1 = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG1_OFFSET)); uint32_t reg2 = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG2_OFFSET)); @@ -489,11 +586,13 @@ static void clock_driver_log_state( uint32_t reg13 = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG13_OFFSET)); uint32_t leaf0 = clock_driver_r32(clk, leaf_reg_off); uint32_t leaf1 = clock_driver_r32(clk, leaf_reg_off + 4u); - uint32_t status = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG4_OFFSET)); + uint32_t status = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_STATUS_OFFSET)); uint32_t reconfig = clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_RECONFIG_OFFSET)); - uint64_t fvco_hz = clock_driver_get_vco_hz(clk, wizard_offset); - uint64_t rate_hz = clock_driver_get_rate_hz(clk, wizard_offset, clock_id); + uint64_t fvco_hz = 0; + uint64_t rate_hz = 0; + (void) clock_driver_get_vco_hz(clk, wizard_offset, &fvco_hz); + (void) clock_driver_get_rate_hz(clk, wizard_offset, clock_id, &rate_hz); LOG( LOG_INFO, @@ -527,8 +626,9 @@ static void clock_driver_log_state( * @param clk Clock driver with clk->o set to the desired O value. * @param wizard_offset Base offset of the wizard instance. * @param clock_id Output clock index. + * @return 0 on success, -1 if a register did not read back as written. */ -static void clock_driver_update_o(struct clock_driver *clk, uint32_t wizard_offset, uint32_t clock_id) +static int clock_driver_update_o(struct clock_driver *clk, uint32_t wizard_offset, uint32_t clock_id) { uint32_t reg_off = clock_driver_reg(wizard_offset, clock_wizard_leaf_offset(clock_id)); @@ -536,8 +636,11 @@ static void clock_driver_update_o(struct clock_driver *clk, uint32_t wizard_offs uint32_t counts = 0; clock_wizard_encode_leaf(clk->o, &ctrl, &counts); - clock_driver_w32(clk, reg_off, ctrl); - clock_driver_w32(clk, reg_off + 4u, counts); + if (clock_driver_w32_verify(clk, reg_off, ctrl) != 0) { + return -1; + } + + return clock_driver_w32_verify(clk, reg_off + 4u, counts); } /** @@ -547,8 +650,9 @@ static void clock_driver_update_o(struct clock_driver *clk, uint32_t wizard_offs * * @param clk Clock driver with clk->d set to the desired D value. * @param wizard_offset Base offset of the wizard instance. + * @return 0 on success, -1 if a register did not read back as written. */ -static void clock_driver_update_d(struct clock_driver *clk, uint32_t wizard_offset) +static int clock_driver_update_d(struct clock_driver *clk, uint32_t wizard_offset) { uint32_t d = clk->d; uint32_t high_time = d / 2u; @@ -558,8 +662,11 @@ static void clock_driver_update_d(struct clock_driver *clk, uint32_t wizard_offs uint32_t div_edge = d % 2u; reg |= (div_edge << XCLK_WIZ_REG12_EDGE_SHIFT); - clock_driver_w32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG12_OFFSET), reg); - clock_driver_w32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG13_OFFSET), (high_time | (high_time << 8u))); /* TODO(vserbu): explain this register offset/bit field */ + if (clock_driver_w32_verify(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG12_OFFSET), reg) != 0) { + return -1; + } + + return clock_driver_w32_verify(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG13_OFFSET), (high_time | (high_time << 8u))); /* TODO(vserbu): explain this register offset/bit field */ } /** @@ -570,15 +677,18 @@ static void clock_driver_update_d(struct clock_driver *clk, uint32_t wizard_offs * * @param clk Clock driver with clk->m set to the desired M value. * @param wizard_offset Base offset of the wizard instance. + * @return 0 on success, -1 if a register did not read back as written. */ -static void clock_driver_update_m(struct clock_driver *clk, uint32_t wizard_offset) +static int clock_driver_update_m(struct clock_driver *clk, uint32_t wizard_offset) { uint32_t m = clk->m; clock_driver_w32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG25_OFFSET), 0); /* TODO(vserbu): explain this register offset/bit field */ uint32_t div_edge = m % 2u; uint32_t high_time = m / 2u; - clock_driver_w32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG2_OFFSET), (high_time | (high_time << 8u))); /* TODO(vserbu): explain this register offset/bit field */ + if (clock_driver_w32_verify(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG2_OFFSET), (high_time | (high_time << 8u))) != 0) { /* TODO(vserbu): explain this register offset/bit field */ + return -1; + } uint32_t reg = XCLK_WIZ_REG1_PREDIV2 | XCLK_WIZ_REG1_EN | XCLK_WIZ_REG1_MX; /* TODO(vserbu): explain this register offset/bit field */ if (div_edge) { @@ -586,7 +696,8 @@ static void clock_driver_update_m(struct clock_driver *clk, uint32_t wizard_offs } else { reg &= ~(1u << 8u); /* TODO(vserbu): explain this register offset/bit field */ } - clock_driver_w32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG1_OFFSET), reg); + + return clock_driver_w32_verify(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG1_OFFSET), reg); } /** @@ -645,8 +756,8 @@ static int clock_driver_wait_for_lock(struct clock_driver *clk, uint32_t wizard_ } for (;;) { - /* Check the LOCK bit in the status register (REG4). */ - if ((clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG4_OFFSET)) & XCLK_WIZ_LOCK) != 0u) { + /* Check the LOCK bit in the status register. */ + if ((clock_driver_r32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_STATUS_OFFSET)) & XCLK_WIZ_LOCK) != 0u) { return 0; } @@ -688,7 +799,10 @@ static int clock_driver_wait_for_lock(struct clock_driver *clk, uint32_t wizard_ * @param wizard_offset Base offset of the wizard instance. * @param clock_id Output clock index. * @param timeout_ms Lock timeout in milliseconds. - * @param ok Output: set to 0 on success, remains -1 on failure. + * @param ok Output: 0 on success, -1 if the configuration failed to + * lock within the timeout, -2 if the register window did + * not respond, in which case retrying another candidate + * is pointless. * @return Achieved output frequency in Hz (only valid when *ok == 0). */ static uint64_t clock_driver_program_mdo_and_reconfig( @@ -703,10 +817,19 @@ static uint64_t clock_driver_program_mdo_and_reconfig( clock_driver_w32(clk, clock_driver_reg(wizard_offset, XCLK_WIZ_REG25_OFFSET), 0); /* TODO(vserbu): explain this register offset/bit field */ - /* Program output divider, input divider, and feedback multiplier. */ - clock_driver_update_o(clk, wizard_offset, clock_id); - clock_driver_update_d(clk, wizard_offset); - clock_driver_update_m(clk, wizard_offset); + /* + * Program output divider, input divider, and feedback multiplier. Each + * write is confirmed by readback, so a window that has stopped responding + * is reported here rather than after the reconfiguration has been + * triggered against an unknown register state. + */ + if (clock_driver_update_o(clk, wizard_offset, clock_id) != 0 + || clock_driver_update_d(clk, wizard_offset) != 0 + || clock_driver_update_m(clk, wizard_offset) != 0) { + *ok = -2; + return 0; + } + clock_driver_program_common_tail(clk, wizard_offset); /* Trigger the dynamic reconfiguration and wait for PLL lock. */ @@ -716,8 +839,25 @@ static uint64_t clock_driver_program_mdo_and_reconfig( return 0; } + uint64_t rate_hz = 0; + if (clock_driver_get_rate_hz(clk, wizard_offset, clock_id, &rate_hz) != CLOCK_READ_OK) { + LOG( + LOG_ERR, + "clock_driver: wizard registers unreadable after reconfiguration" + " (wiz=0x%08x clk=%u m=%u d=%u o=%u)", + wizard_offset, + clock_id, + clk->m, + clk->d, + clk->o + ); + *ok = -2; + errno = EIO; + return 0; + } + *ok = 0; - return clock_driver_get_rate_hz(clk, wizard_offset, clock_id); + return rate_hz; } /** @@ -979,6 +1119,9 @@ static int clock_driver_try_set_rate_hz( uint64_t predicted_fvco_hz = ((uint64_t)clk->prim_in_hz * clk->m) / clk->d; uint32_t predicted_divo = clock_driver_effective_divo_from_o(clk->o); + if (predicted_divo == 0u) { + continue; + } uint64_t predicted_rate_hz = predicted_fvco_hz / predicted_divo; LOG( LOG_INFO, @@ -1020,6 +1163,36 @@ static int clock_driver_try_set_rate_hz( count ); + /* + * The rate read back must agree with the configuration just + * written. A disagreement means the register window did not take + * the programming - for example because the mapping no longer + * refers to the live device - so the reported value does not + * describe a frequency the hardware is producing. Fail rather + * than hand it to the caller as an achieved rate. + */ + uint64_t readback_delta_hz = (reported > predicted_rate_hz) + ? (reported - predicted_rate_hz) + : (predicted_rate_hz - reported); + if (readback_delta_hz > clk->min_err_hz) { + LOG( + LOG_ERR, + "clock_driver: readback disagrees with programmed configuration:" + " predicted_hz=%" PRIu64 " reported_hz=%" PRIu64 " delta_hz=%" PRIu64 + " m=%u d=%u o=%u wiz=0x%08x clk=%u", + predicted_rate_hz, + reported, + readback_delta_hz, + clk->m, + clk->d, + clk->o, + wizard_offset, + clock_id + ); + errno = EIO; + return -1; + } + /* * The first candidate that locks wins, which may be well below the * request if better-ranked candidates failed. Say so: the caller @@ -1049,6 +1222,19 @@ static int clock_driver_try_set_rate_hz( return 0; } + if (ok == -2) { + LOG( + LOG_ERR, + "clock_driver: aborting set_rate request_hz=%u: wizard register window" + " is not responding (wiz=0x%08x clk=%u)", + *rate_hz_inout, + wizard_offset, + clock_id + ); + errno = EIO; + return -1; + } + LOG( LOG_WARNING, "clock_driver: lock timeout request_hz=%u candidate=%zu/%zu m=%u d=%u o=%u timeout_ms=%u", @@ -1086,11 +1272,22 @@ int clock_driver_get_service_region_rate_hz(struct clock_driver *clk, uint32_t * return -1; } - uint64_t rate = clock_driver_get_rate_hz( + uint64_t rate = 0; + enum clock_read status = clock_driver_get_rate_hz( clk, CLOCK_DRIVER_SERVICE_REGION_WIZARD_OFFSET, - CLOCK_DRIVER_WIZARD_CLKOUT_ID + CLOCK_DRIVER_WIZARD_CLKOUT_ID, + &rate ); + if (status == CLOCK_READ_FAULT) { + LOG(LOG_ERR, "clock_driver: service region wizard registers unreadable"); + errno = EIO; + return -1; + } + if (status == CLOCK_READ_UNCONFIGURED) { + LOG(LOG_INFO, "clock_driver: service region clock has not been configured"); + } + /* Zero reports "unconfigured" to the caller; it is not a realizable rate. */ *rate_hz_out = (uint32_t)rate; return 0; } @@ -1149,11 +1346,22 @@ int clock_driver_get_user_region_rate_hz(struct clock_driver *clk, uint32_t *rat return -1; } - uint64_t rate = clock_driver_get_rate_hz( + uint64_t rate = 0; + enum clock_read status = clock_driver_get_rate_hz( clk, CLOCK_DRIVER_USER_REGION_WIZARD_OFFSET, - CLOCK_DRIVER_WIZARD_CLKOUT_ID + CLOCK_DRIVER_WIZARD_CLKOUT_ID, + &rate ); + if (status == CLOCK_READ_FAULT) { + LOG(LOG_ERR, "clock_driver: user region wizard registers unreadable"); + errno = EIO; + return -1; + } + if (status == CLOCK_READ_UNCONFIGURED) { + LOG(LOG_INFO, "clock_driver: user region clock has not been configured"); + } + /* Zero reports "unconfigured" to the caller; it is not a realizable rate. */ *rate_hz_out = (uint32_t)rate; return 0; } diff --git a/vrt/vrtd/src/clock.h b/vrt/vrtd/src/clock.h index 353f89fd..5ebb70d2 100644 --- a/vrt/vrtd/src/clock.h +++ b/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); diff --git a/vrt/vrtd/src/reset.c b/vrt/vrtd/src/reset.c index 0341fdf2..a0c96077 100644 --- a/vrt/vrtd/src/reset.c +++ b/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; } diff --git a/vrt/vrtd/src/serve.c b/vrt/vrtd/src/serve.c index 87a0f36d..6b80d281 100644 --- a/vrt/vrtd/src/serve.c +++ b/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); diff --git a/vrt/vrtd/src/shell_build_id.c b/vrt/vrtd/src/shell_build_id.c new file mode 100644 index 00000000..4f6efacd --- /dev/null +++ b/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 +#include +#include + +#include + +#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; +} diff --git a/vrt/vrtd/src/shell_build_id.h b/vrt/vrtd/src/shell_build_id.h new file mode 100644 index 00000000..8c59044e --- /dev/null +++ b/vrt/vrtd/src/shell_build_id.h @@ -0,0 +1,116 @@ +/** + * 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.h + * @brief Shell build-ID register: the shell variant as reported by hardware. + * + * The static shell instantiates an AXI GPIO holding a build-time constant that + * identifies the bitstream: the leading 60 bits of the source commit hash, a + * dirty-tree flag, and — the field this module exists for — the shell variant. + * Each shell's @c create_project.tcl forces the variant bit to its own value, + * so the bit is correct even for bitstreams built outside the linker, and it is + * the only shell identity that comes from the device rather than from vrtd's + * own bookkeeping. + * + * The register layout matches the reader in @c smi/src/shell_build_id.hpp. + */ + +#ifndef VRTD_SHELL_BUILD_ID_H +#define VRTD_SHELL_BUILD_ID_H + +#include + +#include + +#include "vrtd/wire.h" + +/** @brief PCI BAR index carrying the static-region build-ID GPIO. */ +#define BUILD_ID_BAR_NUMBER 4 + +/** + * @brief Offset of the build-ID AXI GPIO within BAR4. + * + * Its block-design address is 0x0204_0002_0000 and BAR4 maps the + * 0x0204_0000_0000 aperture. + */ +#define BUILD_ID_OFFSET 0x00020000u + +/** @brief AXI GPIO channel-1 data register: low 32 bits of the commit prefix. */ +#define BUILD_ID_REG_LO (BUILD_ID_OFFSET + 0x0u) + +/** + * @brief AXI GPIO channel-2 data register. + * + * bits[27:0] high hash bits, bit[28] shell variant, bits[30:29] reserved, + * bit[31] dirty flag. + */ +#define BUILD_ID_REG_HI (BUILD_ID_OFFSET + 0x8u) + +/** @brief Shell-variant flag within the high word: 0 = service, 1 = compute. */ +#define BUILD_ID_HI_SHELL_MASK 0x10000000u + +/** @brief Reserved bits within the high word; every shell leaves them clear. */ +#define BUILD_ID_HI_RESERVED_MASK 0x60000000u + +/** + * @brief Decode the shell variant from the build-ID high word. + * + * @param hi Value read from @c BUILD_ID_REG_HI. + * @return @c VRTD_SHELL_COMPUTE or @c VRTD_SHELL_SERVICE for a well-formed + * word, or @c VRTD_SHELL_UNKNOWN when the word cannot have come from a + * build-ID register: all bits set (the pattern an MMIO read returns + * when it does not reach the device) or any reserved bit set. + */ +enum vrtd_shell_type build_id_decode_shell(uint32_t hi); + +/** + * @brief Read the shell variant the device reports for itself. + * + * @param bar Open mapping of BAR4. May be NULL. + * @return The decoded variant, or @c VRTD_SHELL_UNKNOWN if the BAR is absent, + * too small to contain the register, or does not respond. + */ +enum vrtd_shell_type build_id_read_shell(const struct slash_bar_file *bar); + +/** + * @brief Verify that hardware reports the shell vrtd expects it to be running. + * + * A no-op returning success when @p expected is @c VRTD_SHELL_UNKNOWN: there is + * then no claim to contradict. Otherwise any disagreement — including a + * build-ID register that does not respond — is an error, because the caller has + * asserted a specific shell and the device is not confirming it. + * + * @param bar Open mapping of BAR4. May be NULL. + * @param expected The shell vrtd believes is loaded. + * @param context Short caller identifier used as the log prefix. + * @return 0 if hardware agrees (or there was nothing to check), -1 otherwise + * with @c errno set to @c EIO. + */ +int build_id_check_shell( + const struct slash_bar_file *bar, + enum vrtd_shell_type expected, + const char *context +); + +/** @brief Lowercase name of a shell variant, for log messages. */ +const char *build_id_shell_name(enum vrtd_shell_type shell); + +#endif // VRTD_SHELL_BUILD_ID_H diff --git a/vrt/vrtd/tests/CMakeLists.txt b/vrt/vrtd/tests/CMakeLists.txt index 208761d6..52c18902 100644 --- a/vrt/vrtd/tests/CMakeLists.txt +++ b/vrt/vrtd/tests/CMakeLists.txt @@ -35,3 +35,4 @@ add_vrtd_test(device_test device_test.cpp) add_vrtd_test(flash_worker_test flash_worker_test.cpp) add_vrtd_test(reset_test reset_test.cpp) add_vrtd_test(clock_test clock_test.cpp) +add_vrtd_test(shell_build_id_test shell_build_id_test.cpp) diff --git a/vrt/vrtd/tests/clock_test.cpp b/vrt/vrtd/tests/clock_test.cpp index 9f8821e6..36be3263 100644 --- a/vrt/vrtd/tests/clock_test.cpp +++ b/vrt/vrtd/tests/clock_test.cpp @@ -20,7 +20,9 @@ #include +#include #include +#include extern "C" { #include "clock.h" @@ -31,6 +33,61 @@ namespace { constexpr uint32_t kOMin = 2u; constexpr uint32_t kOMax = 511u; +// Wizard register offsets, mirrored from clock.c so the tests exercise the +// driver through its public interface rather than its internals. +constexpr uint32_t kUserWizard = CLOCK_DRIVER_USER_REGION_WIZARD_OFFSET; +constexpr uint32_t kServiceWizard = CLOCK_DRIVER_SERVICE_REGION_WIZARD_OFFSET; +constexpr uint32_t kReg1 = 0x330u; // M edge bit +constexpr uint32_t kReg2 = 0x334u; // M low/high counts +constexpr uint32_t kReg3 = 0x338u; // clk_out0 leaf pair (ctrl at +0, counts at +4) +constexpr uint32_t kReg12 = 0x380u; // D edge bit +constexpr uint32_t kReg13 = 0x384u; // D low/high counts + +constexpr uint32_t kPrimInHz = 100000000u; + +// Both wizard windows plus the full 0x400-byte register file of the higher one. +constexpr size_t kBarBytes = kServiceWizard + 0x400u; + +/** + * A clock_driver backed by an ordinary buffer instead of a mapped BAR. + * + * The read paths touch only regs, len and prim_in_hz, so a device is not + * needed to drive them. + */ +class FakeWizard { +public: + explicit FakeWizard(uint32_t fill = 0u) + : words_(kBarBytes / sizeof(uint32_t), fill) { + clk_.regs = words_.data(); + clk_.len = kBarBytes; + clk_.prim_in_hz = kPrimInHz; + } + + void poke(uint32_t offset, uint32_t value) { + words_[offset / sizeof(uint32_t)] = value; + } + + /** Program M, D and O for one wizard the way the hardware encodes them. */ + void program(uint32_t wizard, uint32_t m, uint32_t d, uint32_t o) { + poke(wizard + kReg1, 0u); + poke(wizard + kReg2, (m / 2u) | ((m - m / 2u) << 8u)); + poke(wizard + kReg12, 0u); + poke(wizard + kReg13, (d / 2u) | ((d - d / 2u) << 8u)); + + uint32_t ctrl = 0; + uint32_t counts = 0; + clock_wizard_encode_leaf(o, &ctrl, &counts); + poke(wizard + kReg3, ctrl); + poke(wizard + kReg3 + 4u, counts); + } + + struct clock_driver *get() { return &clk_; } + +private: + std::vector words_; + struct clock_driver clk_{}; +}; + } // namespace TEST(ClockWizardLeafTest, EncodeDecodeRoundTripsEveryDivider) { @@ -70,6 +127,69 @@ TEST(ClockWizardLeafTest, DecodesOddAndHalfStepDividers) { } } -TEST(ClockWizardLeafTest, DecodeNeverReturnsZero) { - EXPECT_EQ(clock_wizard_decode_leaf(0u, 0u), 1u); +TEST(ClockWizardLeafTest, DecodeReportsAnEmptyLeafPairAsInvalid) { + // Regression for SLASH issue 207: a cleared leaf pair was clamped to a + // divider of 1, so an unresponsive register window reported f_out = f_VCO + // instead of being rejected. + EXPECT_EQ(clock_wizard_decode_leaf(0u, 0u), 0u); +} + +TEST(ClockDriverGetRateTest, ReportsAProgrammedRate) { + FakeWizard wizard; + // M=12, D=1, O=6 => 100 MHz * 12 / 1 / 6 = 200 MHz. + wizard.program(kUserWizard, 12u, 1u, 6u); + wizard.program(kServiceWizard, 12u, 1u, 6u); + + uint32_t rate = 0; + ASSERT_EQ(clock_driver_get_user_region_rate_hz(wizard.get(), &rate), 0); + EXPECT_EQ(rate, 200000000u); + + rate = 0; + ASSERT_EQ(clock_driver_get_service_region_rate_hz(wizard.get(), &rate), 0); + EXPECT_EQ(rate, 200000000u); +} + +TEST(ClockDriverGetRateTest, ClearedRegistersReportUnconfiguredRatherThanTheReferenceClock) { + // The state a healthy V80 is actually in before anything programs a rate: + // the whole 0x330-0x39C range reads zero. Clamping M, D and O to 1 used to + // collapse f_out to f_in and report a confident 100 MHz for a clock nobody + // had set. Zero is the honest answer, and succeeds so callers can say so. + FakeWizard wizard(0u); + + uint32_t rate = 0xdeadbeefu; + ASSERT_EQ(clock_driver_get_user_region_rate_hz(wizard.get(), &rate), 0); + EXPECT_EQ(rate, 0u); + + rate = 0xdeadbeefu; + ASSERT_EQ(clock_driver_get_service_region_rate_hz(wizard.get(), &rate), 0); + EXPECT_EQ(rate, 0u); +} + +TEST(ClockDriverGetRateTest, ProgrammedMAndDWithAClearedLeafPairIsUnconfigured) { + // The SLASH issue 207 signature: the M/D writes landed but the output + // divider write did not. f_VCO alone is not an output rate, and reporting + // it gave the 4 GHz reading in the bug report. + FakeWizard wizard; + wizard.program(kUserWizard, 40u, 1u, 6u); + wizard.poke(kUserWizard + kReg3, 0u); + wizard.poke(kUserWizard + kReg3 + 4u, 0u); + + uint32_t rate = 0xdeadbeefu; + ASSERT_EQ(clock_driver_get_user_region_rate_hz(wizard.get(), &rate), 0); + EXPECT_EQ(rate, 0u); +} + +TEST(ClockDriverGetRateTest, AnUnresponsiveWindowIsAnError) { + // A register window that answers 0xFFFFFFFF is not answering. That is a + // fault, distinct from a wizard that simply has nothing programmed. + FakeWizard wizard(0xFFFFFFFFu); + + uint32_t rate = 0; + errno = 0; + EXPECT_EQ(clock_driver_get_user_region_rate_hz(wizard.get(), &rate), -1); + EXPECT_EQ(errno, EIO); + + errno = 0; + EXPECT_EQ(clock_driver_get_service_region_rate_hz(wizard.get(), &rate), -1); + EXPECT_EQ(errno, EIO); } diff --git a/vrt/vrtd/tests/shell_build_id_test.cpp b/vrt/vrtd/tests/shell_build_id_test.cpp new file mode 100644 index 00000000..66a10e89 --- /dev/null +++ b/vrt/vrtd/tests/shell_build_id_test.cpp @@ -0,0 +1,127 @@ +/** + * 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. + */ + +#include + +#include +#include +#include + +extern "C" { +#include "shell_build_id.h" +} + +namespace { + +// A BAR mapping large enough to hold the build-ID register, with the high word +// preset to `hi`. The low word is left at zero; nothing under test reads it. +class FakeBar { + public: + explicit FakeBar(uint32_t hi, size_t len = BUILD_ID_REG_HI + sizeof(uint32_t)) + : storage_(len, 0) { + if (len >= BUILD_ID_REG_HI + sizeof(uint32_t)) { + std::memcpy(&storage_[BUILD_ID_REG_HI], &hi, sizeof(hi)); + } + bar_.map = storage_.data(); + bar_.len = storage_.size(); + bar_.fd = -1; + bar_.mock = true; + bar_.mock_path = nullptr; + } + + const struct slash_bar_file *get() const { return &bar_; } + + private: + std::vector storage_; + struct slash_bar_file bar_{}; +}; + +// Build-ID high words as the two shells emit them: bits[27:0] carry the top of +// the commit hash, bit[28] the shell variant, bit[31] the dirty flag. +constexpr uint32_t kServiceHi = 0x0abcdef0u; +constexpr uint32_t kComputeHi = 0x1abcdef0u; + +} // namespace + +TEST(BuildIdDecodeTest, DecodesBothShellVariants) { + EXPECT_EQ(build_id_decode_shell(kServiceHi), VRTD_SHELL_SERVICE); + EXPECT_EQ(build_id_decode_shell(kComputeHi), VRTD_SHELL_COMPUTE); +} + +TEST(BuildIdDecodeTest, DirtyFlagDoesNotAffectTheShellVariant) { + EXPECT_EQ(build_id_decode_shell(kServiceHi | 0x80000000u), VRTD_SHELL_SERVICE); + EXPECT_EQ(build_id_decode_shell(kComputeHi | 0x80000000u), VRTD_SHELL_COMPUTE); +} + +TEST(BuildIdDecodeTest, AnInteractiveBuildWithNoHashStillIdentifiesItsShell) { + // create_project.tcl forces bit[28] regardless of SLASH_BUILD_ID_HI, so the + // variant is valid even when the hash bits are zero. + EXPECT_EQ(build_id_decode_shell(0x00000000u), VRTD_SHELL_SERVICE); + EXPECT_EQ(build_id_decode_shell(0x10000000u), VRTD_SHELL_COMPUTE); +} + +TEST(BuildIdDecodeTest, RejectsWordsThatCannotBeABuildId) { + // An MMIO read that does not reach the device returns all ones. + EXPECT_EQ(build_id_decode_shell(0xFFFFFFFFu), VRTD_SHELL_UNKNOWN); + // No shell sets the reserved bits. + EXPECT_EQ(build_id_decode_shell(kServiceHi | 0x20000000u), VRTD_SHELL_UNKNOWN); + EXPECT_EQ(build_id_decode_shell(kComputeHi | 0x40000000u), VRTD_SHELL_UNKNOWN); +} + +TEST(BuildIdReadTest, ReportsUnknownWithoutAUsableMapping) { + EXPECT_EQ(build_id_read_shell(nullptr), VRTD_SHELL_UNKNOWN); + + FakeBar truncated(kComputeHi, BUILD_ID_REG_HI); + EXPECT_EQ(build_id_read_shell(truncated.get()), VRTD_SHELL_UNKNOWN); +} + +TEST(BuildIdReadTest, ReadsTheVariantFromTheMapping) { + FakeBar service(kServiceHi); + FakeBar compute(kComputeHi); + + EXPECT_EQ(build_id_read_shell(service.get()), VRTD_SHELL_SERVICE); + EXPECT_EQ(build_id_read_shell(compute.get()), VRTD_SHELL_COMPUTE); +} + +TEST(BuildIdCheckTest, PassesWhenHardwareAgrees) { + FakeBar service(kServiceHi); + EXPECT_EQ(build_id_check_shell(service.get(), VRTD_SHELL_SERVICE, "test"), 0); +} + +TEST(BuildIdCheckTest, FailsWhenHardwareReportsTheOtherShell) { + // Regression for SLASH issue 207: vrtd drove the device believing it was + // running a shell the hardware was not running. + FakeBar service(kServiceHi); + EXPECT_EQ(build_id_check_shell(service.get(), VRTD_SHELL_COMPUTE, "test"), -1); +} + +TEST(BuildIdCheckTest, FailsWhenTheRegisterDoesNotRespond) { + FakeBar dead(0xFFFFFFFFu); + EXPECT_EQ(build_id_check_shell(dead.get(), VRTD_SHELL_SERVICE, "test"), -1); + EXPECT_EQ(build_id_check_shell(nullptr, VRTD_SHELL_COMPUTE, "test"), -1); +} + +TEST(BuildIdCheckTest, PassesWhenNoShellHasBeenClaimed) { + // Nothing to contradict: an unknown expectation is not a mismatch, even + // when the register itself is unreadable. + FakeBar dead(0xFFFFFFFFu); + EXPECT_EQ(build_id_check_shell(dead.get(), VRTD_SHELL_UNKNOWN, "test"), 0); + EXPECT_EQ(build_id_check_shell(nullptr, VRTD_SHELL_UNKNOWN, "test"), 0); +}