[sw, spiflash] Dramatically increase transfer rate
The spiflash tool has relied on a fixed delay for spacing the writes
between frames. This hasn't been a problem for small binary deployments
to the FPGA, but Tock release binaries take ~3m23s to flash, and debug
binaries take even longer.
Add flow control to spiflash by waiting for until the hash of the
current frame can be read back before writing the next frame. This
successfully allows Tock to be flashed to the FPGA in 6.7s, a 96.7%
decrease over the previous implementation.
Signed-off-by: Jon Flatley <jflat@google.com>
diff --git a/sw/host/spiflash/ftdi_spi_interface.cc b/sw/host/spiflash/ftdi_spi_interface.cc
index 7413125..2c4aee9 100644
--- a/sw/host/spiflash/ftdi_spi_interface.cc
+++ b/sw/host/spiflash/ftdi_spi_interface.cc
@@ -6,9 +6,11 @@
#include <assert.h>
#include <fcntl.h>
+#include <openssl/sha.h>
#include <termios.h>
#include <unistd.h>
+#include <chrono>
#include <cstring>
#include <iostream>
#include <string>
@@ -23,9 +25,12 @@
namespace spiflash {
namespace {
-// Required delay to synchronize transactions with FPGA environment.
+// Time to wait between attempts to check the hash in nanoseconds.
// TODO: If transmission is not successful, adapt this by an argument.
-constexpr int kTransmitDelay = 1000000;
+constexpr int kHashReadDelayNs = 10000;
+
+// Time before giving up on looking for the correct hash.
+constexpr int kHashReadTimeoutNs = 1000000;
// FTDI Configuration. This can be made configurable later on if needed.
constexpr int kFrequency = 1000000; // 1MHz
@@ -58,7 +63,6 @@
usleep(100000);
PinLow(ctx, kGpioJtagSpiN);
}
-
} // namespace
// Wrapper struct used to hide mpsse_context since incomplete C struct
@@ -94,8 +98,7 @@
return true;
}
-bool FtdiSpiInterface::TransmitFrame(const uint8_t *tx, uint8_t *rx,
- size_t size) {
+bool FtdiSpiInterface::TransmitFrame(const uint8_t *tx, size_t size) {
assert(spi_ != nullptr);
// The mpsse library is more permissive than the SpiInteface. Copying tx
@@ -106,22 +109,78 @@
std::cerr << "Unable to start spi transaction." << std::endl;
return false;
}
+
uint8_t *tmp_rx = ::Transfer(spi_->ctx, tx_local.data(), size);
+ // We're not using the result of this read, so free it right away.
+ if (tmp_rx == nullptr) {
+ free(tmp_rx);
+ }
+
if (Stop(spi_->ctx)) {
std::cerr << "Unable to terminate spi transaction." << std::endl;
- if (tmp_rx) {
- free(tmp_rx);
- }
return false;
}
- if (tmp_rx == nullptr) {
- std::cerr << "Unable to transfer data. Frame size: " << size << std::endl;
- return false;
- }
- usleep(kTransmitDelay);
- memcpy(rx, tmp_rx, size);
- free(tmp_rx);
return true;
}
+
+bool FtdiSpiInterface::CheckHash(const uint8_t *tx, size_t size) {
+ uint8_t hash[SHA256_DIGEST_LENGTH];
+ SHA256_CTX sha256;
+ SHA256_Init(&sha256);
+ SHA256_Update(&sha256, tx, size);
+ SHA256_Final(hash, &sha256);
+
+ uint8_t *rx;
+
+ int hash_index = 0;
+ bool hash_correct = false;
+
+ if (Start(spi_->ctx)) {
+ std::cerr << "Unable to start spi transaction." << std::endl;
+ return false;
+ }
+
+ auto begin = std::chrono::steady_clock::now();
+ auto now = begin;
+ while (!hash_correct &&
+ std::chrono::duration_cast<std::chrono::microseconds>(now - begin)
+ .count() < kHashReadTimeoutNs) {
+ usleep(kHashReadDelayNs);
+ rx = nullptr;
+ rx = ::Read(spi_->ctx, size);
+ if (!rx) {
+ std::cerr << "Read failed, did not allocate buffer." << std::endl;
+ break;
+ }
+
+ // It appears that the hash is always the first 32 bytes in practice, but in
+ // testing I've seen the hash appear at random locations in the message.
+ // Checking for the hash at any location or even split between messages may
+ // not be necessary, but it is probably safer.
+ for (int i = 0; !hash_correct && i < SHA256_DIGEST_LENGTH; ++i) {
+ if (rx[i] == hash[hash_index]) {
+ ++hash_index;
+ if (hash_index == SHA256_DIGEST_LENGTH) {
+ hash_correct = true;
+ }
+ } else {
+ hash_index = 0;
+ }
+ }
+ free(rx);
+ now = std::chrono::steady_clock::now();
+ }
+
+ if (Stop(spi_->ctx)) {
+ std::cerr << "Unable to terminate spi transaction." << std::endl;
+ return false;
+ }
+
+ if (!hash_correct) {
+ std::cerr << "Didn't receive correct hash before timeout." << std::endl;
+ }
+
+ return hash_correct;
+}
} // namespace spiflash
} // namespace opentitan
diff --git a/sw/host/spiflash/ftdi_spi_interface.h b/sw/host/spiflash/ftdi_spi_interface.h
index 329c97b..dbce760 100644
--- a/sw/host/spiflash/ftdi_spi_interface.h
+++ b/sw/host/spiflash/ftdi_spi_interface.h
@@ -29,9 +29,10 @@
// Initialize interface.
bool Init() final;
- // Transmit bytes from |tx| buffer and read data back onto |rx| buffer. The
- // number of bytes are defined by |size|.
- bool TransmitFrame(const uint8_t *tx, uint8_t *rx, size_t size) final;
+ // Transmit bytes from |tx| buffer. The number of bytes are defined by |size|.
+ bool TransmitFrame(const uint8_t *tx, size_t size) final;
+
+ bool CheckHash(const uint8_t *tx, size_t size) final;
private:
std::unique_ptr<MpsseHandle> spi_;
diff --git a/sw/host/spiflash/spi_interface.h b/sw/host/spiflash/spi_interface.h
index feb48f6..483b2ce 100644
--- a/sw/host/spiflash/spi_interface.h
+++ b/sw/host/spiflash/spi_interface.h
@@ -25,9 +25,14 @@
// Initialize SPI interface. Returns true on success.
virtual bool Init() = 0;
- // Transmit bytes from |tx| buffer and read data back onto |rx| buffer. The
- // number of bytes transferred is defined by |size|.
- virtual bool TransmitFrame(const uint8_t *tx, uint8_t *rx, size_t size) = 0;
+ // Transmit bytes from |tx| buffer. The number of bytes transferred is defined
+ // by |size|.
+ virtual bool TransmitFrame(const uint8_t *tx, size_t size) = 0;
+
+ // Wait until the hash from the previously sent is able to be read. The
+ // previous frame to check the hash for should be provided in |tx| and the
+ // frame's length as |size|.
+ virtual bool CheckHash(const uint8_t *tx, size_t size) = 0;
};
} // namespace spiflash
diff --git a/sw/host/spiflash/updater.cc b/sw/host/spiflash/updater.cc
index 658521f..1d07590 100644
--- a/sw/host/spiflash/updater.cc
+++ b/sw/host/spiflash/updater.cc
@@ -44,15 +44,6 @@
SHA256_Final(f->hdr.hash, &sha256);
}
-// Check hash portion of |ack| against |ack_expected|.
-bool CheckAckHash(const std::string &ack, const std::string &ack_expected) {
- uint8_t result = 0;
- for (int i = 0; i < 32; ++i) {
- result |= ack[i] ^ ack_expected[i];
- }
- return (result == 0);
-}
-
} // namespace
bool Updater::Run() {
@@ -77,31 +68,16 @@
<< std::setw(8) << std::hex << f.hdr.offset << std::endl;
if (!spi_->TransmitFrame(reinterpret_cast<const uint8_t *>(&f),
- reinterpret_cast<uint8_t *>(&ack[0]),
sizeof(Frame))) {
std::cerr << "Failed to transmit frame no: 0x" << std::setfill('0')
<< std::setw(8) << std::hex << f.hdr.frame_num << std::endl;
}
- // When we send the next frame, we'll get the previous frame's hash as
- // the ack, so with each increment of |current_frame| we also need to
- // update the |ack_expected| value.
- uint32_t ack_expected_index = 0;
- if (current_frame == 0 || CheckAckHash(ack, ack_expected)) {
- ack_expected_index = current_frame;
+ // When we send each frame we wait for the correct hash before continuing.
+ if (current_frame == frames.size() - 1 ||
+ spi_->CheckHash(reinterpret_cast<const uint8_t *>(&f), sizeof(Frame))) {
current_frame++;
- } else {
- // TODO: Improve protocol by encoding NEXT frame number, current error,
- // ack marker and CRC.
- // The current implementation will send the previous frame if the current
- // ack doesn't match the expected response.
- if (current_frame >= 1) {
- current_frame--;
- }
- ack_expected_index = (current_frame == 0) ? 0 : current_frame - 1;
}
- SHA256(reinterpret_cast<const uint8_t *>(&frames[ack_expected_index]),
- sizeof(f), reinterpret_cast<uint8_t *>(&ack_expected[0]));
}
return true;
}
diff --git a/sw/host/spiflash/verilator_spi_interface.cc b/sw/host/spiflash/verilator_spi_interface.cc
index 77d5b95..6954d64 100644
--- a/sw/host/spiflash/verilator_spi_interface.cc
+++ b/sw/host/spiflash/verilator_spi_interface.cc
@@ -5,12 +5,14 @@
#include "sw/host/spiflash/verilator_spi_interface.h"
#include <fcntl.h>
+#include <openssl/sha.h>
#include <termios.h>
#include <unistd.h>
#include <cstring>
#include <iostream>
#include <string>
+#include <vector>
namespace opentitan {
namespace spiflash {
@@ -58,10 +60,10 @@
// Reads |size| bytes into |rx| buffer from |fd|. Returns the number of bytes
// read.
-int ReadBytes(int fd, uint8_t *rx, size_t size) {
+size_t ReadBytes(int fd, uint8_t *rx, size_t size) {
size_t bytes_read = 0;
while (bytes_read != size) {
- size_t read_size = read(fd, &rx[bytes_read], size - bytes_read);
+ ssize_t read_size = read(fd, &rx[bytes_read], size - bytes_read);
switch (read_size) {
case -1:
if (errno == EAGAIN || errno == EWOULDBLOCK) {
@@ -91,8 +93,7 @@
return true;
}
-bool VerilatorSpiInterface::TransmitFrame(const uint8_t *tx, uint8_t *rx,
- size_t size) {
+bool VerilatorSpiInterface::TransmitFrame(const uint8_t *tx, size_t size) {
size_t bytes_written = write(fd_, tx, size);
if (bytes_written != size) {
std::cerr << "Failed to write bytes to spi interface. Bytes written: "
@@ -100,12 +101,24 @@
return false;
}
usleep(kWriteReadDelay);
- size_t bytes_read = ReadBytes(fd_, rx, size);
+ return true;
+}
+
+bool VerilatorSpiInterface::CheckHash(const uint8_t *tx, size_t size) {
+ uint8_t hash[SHA256_DIGEST_LENGTH];
+ SHA256_CTX sha256;
+ SHA256_Init(&sha256);
+ SHA256_Update(&sha256, tx, size);
+ SHA256_Final(hash, &sha256);
+
+ std::vector<uint8_t> rx(size);
+ size_t bytes_read = ReadBytes(fd_, &rx[0], size);
if (bytes_read < size) {
std::cerr << "Failed to read bytes from spi interface. Bytes read: "
<< bytes_read << " expected: " << size << std::endl;
}
- return true;
+
+ return !std::memcmp(&rx[0], hash, SHA256_DIGEST_LENGTH);
}
} // namespace spiflash
} // namespace opentitan
diff --git a/sw/host/spiflash/verilator_spi_interface.h b/sw/host/spiflash/verilator_spi_interface.h
index 099755f..d1021ae 100644
--- a/sw/host/spiflash/verilator_spi_interface.h
+++ b/sw/host/spiflash/verilator_spi_interface.h
@@ -29,9 +29,10 @@
// Initialize interface.
bool Init() final;
- // Transmit bytes from |tx| buffer and read data back onto |rx| buffer. The
- // number of bytes are defined by |size|.
- bool TransmitFrame(const uint8_t *tx, uint8_t *rx, size_t size) final;
+ // Transmit bytes from |tx| buffer. The number of bytes are defined by |size|.
+ bool TransmitFrame(const uint8_t *tx, size_t size) final;
+
+ bool CheckHash(const uint8_t *tx, size_t size) final;
private:
std::string spi_filename_;
