buffer: bugfixes and cleanups
- Added clarifying comments regarding the different offsets
- Fixed an instance in wraparoundRead where we updated the readPtr
with the relative offset
- Added additional checks in initialization to check for buffer size
- Use the queueSize for buffer initialization opposed to the memory
region size for the whole MMIO
Tested: Unit tested
Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: Ib175ed1c9c129c0f9d2b4f9737e2d394f160434d
diff --git a/include/buffer.hpp b/include/buffer.hpp
index 240845c..7738339 100644
--- a/include/buffer.hpp
+++ b/include/buffer.hpp
@@ -105,14 +105,16 @@
/**
* Write to the bufferHeader and update the read pointer
- * @param[in] newReadPtr - read pointer to update to
+ * @param[in] newReadPtr - read pointer to update to.
+ * "readPtr"s (including this one) are offset relative to the "Error Log
+ * Queue region" = (sizeof(CircularBufferHeader) + UE reserved region)
*/
virtual void updateReadPtr(const uint32_t newReadPtr) = 0;
/**
* Wrapper for the dataInterface->read, performs wraparound read
*
- * @param[in] offset - offset to read from
+ * @param[in] offset - offset relative to the beginning of MMIO space
* @param[in] length - bytes to read
* @param[in] additionalBoundaryCheck - bytes to add to the boundary check
* for added restriction
@@ -124,7 +126,7 @@
/**
* Read the entry header from shared buffer
*
- * @param[in] offset - offset to read from
+ * @param[in] offset - offset relative to the beginning of MMIO space
* @return the entry header
*/
virtual struct QueueEntryHeader readEntryHeader(size_t offset) = 0;
@@ -132,7 +134,7 @@
/**
* Read the queue entry from the error log queue
*
- * @param[in] offset - offset to read from
+ * @param[in] offset - offset relative to the beginning of MMIO space
* @return entry header and entry pair read from buffer
*/
virtual EntryPair readEntry(size_t offset) = 0;
diff --git a/src/buffer.cpp b/src/buffer.cpp
index fca8837..04e9861 100644
--- a/src/buffer.cpp
+++ b/src/buffer.cpp
@@ -26,14 +26,24 @@
uint16_t ueRegionSize,
const std::array<uint32_t, 4>& magicNumber)
{
- // Initialize the whole buffer with 0x00
const size_t memoryRegionSize = dataInterface->getMemoryRegionSize();
- const std::vector<uint8_t> emptyVector(memoryRegionSize, 0);
+ const size_t proposedBufferSize =
+ sizeof(struct CircularBufferHeader) + ueRegionSize + queueSize;
+ if (proposedBufferSize > memoryRegionSize)
+ {
+ throw std::runtime_error(fmt::format(
+ "[initialize] Proposed region size '{}' is bigger than the "
+ "BMC's allocated MMIO region of '{}'",
+ proposedBufferSize, memoryRegionSize));
+ }
+
+ // Initialize the whole buffer with 0x00
+ const std::vector<uint8_t> emptyVector(proposedBufferSize, 0);
size_t byteWritten = dataInterface->write(0, emptyVector);
- if (byteWritten != memoryRegionSize)
+ if (byteWritten != proposedBufferSize)
{
throw std::runtime_error(
- fmt::format("Buffer initialization only erased '{}'", byteWritten));
+ fmt::format("[initialize] Only erased '{}'", byteWritten));
}
// Create an initial buffer header and write to it
@@ -59,8 +69,7 @@
if (byteWritten != initializationHeaderSize)
{
throw std::runtime_error(fmt::format(
- "Buffer initialization buffer header write only wrote '{}'",
- byteWritten));
+ "[initialize] Only wrote '{}' bytes of the header", byteWritten));
}
cachedBufferHeader = initializationHeader;
}
@@ -120,16 +129,15 @@
BufferImpl::wraparoundRead(const uint32_t offset, const uint32_t length,
const uint32_t additionalBoundaryCheck)
{
- const size_t memoryRegionSize = dataInterface->getMemoryRegionSize();
+ const size_t queueSize =
+ boost::endian::little_to_native(cachedBufferHeader.queueSize);
- size_t queueOffset = getQueueOffset();
- if (queueOffset + length + additionalBoundaryCheck > memoryRegionSize)
+ if (length + additionalBoundaryCheck > queueSize)
{
throw std::runtime_error(fmt::format(
- "[wraparoundRead] queueOffset '{}' + length '{}' "
- "+ additionalBoundaryCheck '{}' + was bigger "
- "than memoryRegionSize '{}'",
- queueOffset, length, additionalBoundaryCheck, memoryRegionSize));
+ "[wraparoundRead] length '{}' + additionalBoundaryCheck '{}' "
+ "was bigger than queueSize '{}'",
+ length, additionalBoundaryCheck, queueSize));
}
// Do a first read up to the end of the buffer (dataInerface->read should
@@ -140,6 +148,7 @@
// If there are any more bytes to be read beyond the buffer, wrap around and
// read from the beginning of the buffer (offset by the queueOffset)
+ size_t queueOffset = getQueueOffset();
if (bytesRemaining > 0)
{
std::vector<uint8_t> wrappedBytesRead =
@@ -154,7 +163,7 @@
}
bytesRead.insert(bytesRead.end(), wrappedBytesRead.begin(),
wrappedBytesRead.end());
- updatedReadOffset = queueOffset + wrappedBytesRead.size();
+ updatedReadOffset = wrappedBytesRead.size();
}
updateReadPtr(updatedReadOffset);
@@ -178,8 +187,9 @@
size_t entrySize = entryHeader.entrySize;
// wraparonudRead may throw if entrySize was bigger than the buffer or if it
- // was not able to read all bytes, let it propagate up the stack
- // - Use additionalBoundaryCheck parameter to tighten the boundary check
+ // was not able to read all the bytes, let it propagate up the stack
+ // - Use additionalBoundaryCheck parameter to add QueueEntryHeader size to
+ // boundary condition calculation
std::vector<uint8_t> entry =
wraparoundRead(offset + sizeof(struct QueueEntryHeader), entrySize,
sizeof(struct QueueEntryHeader));
diff --git a/test/buffer_test.cpp b/test/buffer_test.cpp
index 7218f8d..8649c27 100644
--- a/test/buffer_test.cpp
+++ b/test/buffer_test.cpp
@@ -62,19 +62,39 @@
TEST_F(BufferTest, BufferInitializeEraseFail)
{
InSequence s;
+ EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
+ .WillOnce(Return(testRegionSize));
+ EXPECT_THROW(
+ try {
+ // Test too big of a proposed buffer compared to the memori size
+ uint16_t bigQueueSize = 0x181;
+ uint16_t bigUeRegionSize = 0x50;
+ bufferImpl->initialize(testBmcInterfaceVersion, bigQueueSize,
+ bigUeRegionSize, testMagicNumber);
+ } catch (const std::runtime_error& e) {
+ EXPECT_STREQ(
+ e.what(),
+ "[initialize] Proposed region size '513' "
+ "is bigger than the BMC's allocated MMIO region of '512'");
+ throw;
+ },
+ std::runtime_error);
+ EXPECT_NE(bufferImpl->getCachedBufferHeader(), testInitializationHeader);
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
- const std::vector<uint8_t> emptyArray(testRegionSize, 0);
+ size_t testProposedBufferSize =
+ sizeof(struct CircularBufferHeader) + testUeRegionSize + testQueueSize;
+ const std::vector<uint8_t> emptyArray(testProposedBufferSize, 0);
// Return a smaller write than the intended testRegionSize to test the error
EXPECT_CALL(*dataInterfaceMockPtr, write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testRegionSize - 1));
+ .WillOnce(Return(testProposedBufferSize - 1));
EXPECT_THROW(
try {
bufferImpl->initialize(testBmcInterfaceVersion, testQueueSize,
testUeRegionSize, testMagicNumber);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(e.what(), "Buffer initialization only erased '511'");
+ EXPECT_STREQ(e.what(), "[initialize] Only erased '383'");
throw;
},
std::runtime_error);
@@ -83,7 +103,7 @@
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
EXPECT_CALL(*dataInterfaceMockPtr, write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testRegionSize));
+ .WillOnce(Return(testProposedBufferSize));
// Return a smaller write than the intended initializationHeader to test the
// error
EXPECT_CALL(*dataInterfaceMockPtr, write(0, _)).WillOnce(Return(0));
@@ -92,9 +112,8 @@
bufferImpl->initialize(testBmcInterfaceVersion, testQueueSize,
testUeRegionSize, testMagicNumber);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(
- e.what(),
- "Buffer initialization buffer header write only wrote '0'");
+ EXPECT_STREQ(e.what(),
+ "[initialize] Only wrote '0' bytes of the header");
throw;
},
std::runtime_error);
@@ -106,9 +125,11 @@
InSequence s;
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
- const std::vector<uint8_t> emptyArray(testRegionSize, 0);
+ size_t testProposedBufferSize =
+ sizeof(struct CircularBufferHeader) + testUeRegionSize + testQueueSize;
+ const std::vector<uint8_t> emptyArray(testProposedBufferSize, 0);
EXPECT_CALL(*dataInterfaceMockPtr, write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testRegionSize));
+ .WillOnce(Return(testProposedBufferSize));
uint8_t* testInitializationHeaderPtr =
reinterpret_cast<uint8_t*>(&testInitializationHeader);
@@ -193,10 +214,12 @@
InSequence s;
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
- const std::vector<uint8_t> emptyArray(testRegionSize, 0);
+ size_t testProposedBufferSize = sizeof(struct CircularBufferHeader) +
+ testUeRegionSize + testQueueSize;
+ const std::vector<uint8_t> emptyArray(testProposedBufferSize, 0);
EXPECT_CALL(*dataInterfaceMockPtr,
write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testRegionSize));
+ .WillOnce(Return(testProposedBufferSize));
uint8_t* testInitializationHeaderPtr =
reinterpret_cast<uint8_t*>(&testInitializationHeader);
@@ -216,17 +239,15 @@
TEST_F(BufferWraparoundReadTest, TooBigReadFail)
{
InSequence s;
- EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
- .WillOnce(Return(testRegionSize));
- size_t tooBigLength = testRegionSize - expectedqueueOffset + 1;
+ size_t tooBigLength = testQueueSize + 1;
EXPECT_THROW(
try {
bufferImpl->wraparoundRead(/* offset */ 0, tooBigLength);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(e.what(),
- "[wraparoundRead] queueOffset '128' + length '385' + "
- "additionalBoundaryCheck '0' + was "
- "bigger than memoryRegionSize '512'");
+ EXPECT_STREQ(
+ e.what(),
+ "[wraparoundRead] length '257' + additionalBoundaryCheck '0' "
+ "was bigger than queueSize '256'");
throw;
},
std::runtime_error);
@@ -235,21 +256,18 @@
TEST_F(BufferWraparoundReadTest, TooBigReadWithAdditionalBoundaryCheckFail)
{
InSequence s;
- EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
- .WillOnce(Return(testRegionSize));
- // Use additionalBoundaryCheck to still go over the memoryRegionSize by 1
+ // Use additionalBoundaryCheck to still go over the queueSize by 1
size_t additionalBoundaryCheck = 10;
- size_t tooBigLength =
- testRegionSize - expectedqueueOffset - additionalBoundaryCheck + 1;
+ size_t tooBigLength = testQueueSize - additionalBoundaryCheck + 1;
EXPECT_THROW(
try {
bufferImpl->wraparoundRead(/* offset */ 0, tooBigLength,
additionalBoundaryCheck);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(e.what(),
- "[wraparoundRead] queueOffset '128' + length '375' + "
- "additionalBoundaryCheck '10' + was "
- "bigger than memoryRegionSize '512'");
+ EXPECT_STREQ(
+ e.what(),
+ "[wraparoundRead] length '247' + additionalBoundaryCheck '10' "
+ "was bigger than queueSize '256'");
throw;
},
std::runtime_error);
@@ -258,8 +276,6 @@
TEST_F(BufferWraparoundReadTest, NoWrapAroundReadPass)
{
InSequence s;
- EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
- .WillOnce(Return(testRegionSize));
size_t testLength = 0x10;
size_t testOffset = 0x50;
@@ -282,11 +298,9 @@
TEST_F(BufferWraparoundReadTest, WrapAroundReadFails)
{
InSequence s;
- EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
- .WillOnce(Return(testRegionSize));
size_t testBytesLeft = 3;
size_t testLength = 0x10;
- size_t testOffset = testRegionSize - (testLength - testBytesLeft);
+ size_t testOffset = testQueueSize - (testLength - testBytesLeft);
// Read 3 bytes short
std::vector<std::uint8_t> testBytesReadShort(testLength - testBytesLeft);
@@ -314,11 +328,9 @@
TEST_F(BufferWraparoundReadTest, WrapAroundReadPasses)
{
InSequence s;
- EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
- .WillOnce(Return(testRegionSize));
size_t testBytesLeft = 3;
size_t testLength = 0x10;
- size_t testOffset = testRegionSize - (testLength - testBytesLeft);
+ size_t testOffset = testQueueSize - (testLength - testBytesLeft);
// Read 3 bytes short
std::vector<std::uint8_t> testBytesReadFirst{16, 15, 14, 13, 12, 11, 10,
@@ -332,7 +344,7 @@
// Call to updateReadPtr is triggered
const std::vector<uint8_t> expectedReadPtr{
- static_cast<uint8_t>(expectedqueueOffset + testBytesLeft), 0x0};
+ static_cast<uint8_t>(testBytesLeft), 0x0};
EXPECT_CALL(*dataInterfaceMockPtr, write(expectedBmcReadPtrOffset,
ElementsAreArray(expectedReadPtr)))
.WillOnce(Return(expectedWriteSize));
@@ -357,8 +369,6 @@
void wraparoundReadMock(std::span<std::uint8_t> expetedBytesOutput)
{
- EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
- .WillOnce(Return(testRegionSize));
EXPECT_CALL(*dataInterfaceMockPtr, read(_, _))
.WillOnce(Return(std::vector<std::uint8_t>(
expetedBytesOutput.begin(), expetedBytesOutput.end())));