buffer: Update the definition of QueueSize
There were confusion whether the QueueSize field represented just the
queue for the normal log or if it represented the whole circular buffer.
Settled on it being the whole circular buffer.
Tested: Unit tested and verified this works on a downstream machine
Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: I867e997b72500c7bcd03047d8eb3e4db146a8add
diff --git a/include/buffer.hpp b/include/buffer.hpp
index 9dba34c..29f3e2f 100644
--- a/include/buffer.hpp
+++ b/include/buffer.hpp
@@ -154,6 +154,13 @@
* @return vector of EntryPair which consists of entry header and entry
*/
virtual std::vector<EntryPair> readErrorLogs() = 0;
+
+ /**
+ * Get max offset for the queue
+ *
+ * * @return Queue size - UE region size - Queue header size
+ */
+ virtual size_t getMaxOffset() = 0;
};
/**
@@ -178,6 +185,7 @@
struct QueueEntryHeader readEntryHeader() override;
EntryPair readEntry() override;
std::vector<EntryPair> readErrorLogs() override;
+ size_t getMaxOffset() override;
private:
/** @brief The Error log queue starts after the UE region, which is where
diff --git a/src/buffer.cpp b/src/buffer.cpp
index 275a8c6..4f1812d 100644
--- a/src/buffer.cpp
+++ b/src/buffer.cpp
@@ -27,20 +27,18 @@
const std::array<uint32_t, 4>& magicNumber)
{
const size_t memoryRegionSize = dataInterface->getMemoryRegionSize();
- const size_t proposedBufferSize =
- sizeof(struct CircularBufferHeader) + ueRegionSize + queueSize;
- if (proposedBufferSize > memoryRegionSize)
+ if (queueSize > memoryRegionSize)
{
throw std::runtime_error(fmt::format(
- "[initialize] Proposed region size '{}' is bigger than the "
+ "[initialize] Proposed queue size '{}' is bigger than the "
"BMC's allocated MMIO region of '{}'",
- proposedBufferSize, memoryRegionSize));
+ queueSize, memoryRegionSize));
}
// Initialize the whole buffer with 0x00
- const std::vector<uint8_t> emptyVector(proposedBufferSize, 0);
+ const std::vector<uint8_t> emptyVector(queueSize, 0);
size_t byteWritten = dataInterface->write(0, emptyVector);
- if (byteWritten != proposedBufferSize)
+ if (byteWritten != queueSize)
{
throw std::runtime_error(
fmt::format("[initialize] Only erased '{}'", byteWritten));
@@ -151,31 +149,30 @@
std::vector<uint8_t> BufferImpl::wraparoundRead(const uint32_t relativeOffset,
const uint32_t length)
{
- const size_t queueSize =
- boost::endian::little_to_native(cachedBufferHeader.queueSize);
+ const size_t maxOffset = getMaxOffset();
- if (relativeOffset > queueSize)
+ if (relativeOffset > maxOffset)
{
throw std::runtime_error(
fmt::format("[wraparoundRead] relativeOffset '{}' was bigger "
- "than queueSize '{}'",
- relativeOffset, queueSize));
+ "than maxOffset '{}'",
+ relativeOffset, maxOffset));
}
- if (length > queueSize)
+ if (length > maxOffset)
{
throw std::runtime_error(fmt::format(
- "[wraparoundRead] length '{}' was bigger than queueSize '{}'",
- length, queueSize));
+ "[wraparoundRead] length '{}' was bigger than maxOffset '{}'",
+ length, maxOffset));
}
// Do a calculation to see if the read will wraparound
const size_t queueOffset = getQueueOffset();
- const size_t queueSizeToQueueEnd = queueSize - relativeOffset;
+ const size_t writableSpace = maxOffset - relativeOffset;
size_t numWraparoundBytesToRead = 0;
- if (length > queueSizeToQueueEnd)
+ if (length > writableSpace)
{
// This means we will wrap, count the bytes that are left to read
- numWraparoundBytesToRead = length - queueSizeToQueueEnd;
+ numWraparoundBytesToRead = length - writableSpace;
}
const size_t numBytesToReadTillQueueEnd = length - numWraparoundBytesToRead;
@@ -189,7 +186,7 @@
bytesRead.size(), numBytesToReadTillQueueEnd));
}
size_t updatedReadPtr = relativeOffset + numBytesToReadTillQueueEnd;
- if (updatedReadPtr == queueSize)
+ if (updatedReadPtr == maxOffset)
{
// If we read all the way up to the end of the queue, we need to
// manually wrap the updateReadPtr around to 0
@@ -263,25 +260,24 @@
// Reading the buffer header will update the cachedBufferHeader
readBufferHeader();
- const size_t queueSize =
- boost::endian::little_to_native(cachedBufferHeader.queueSize);
+ const size_t maxOffset = getMaxOffset();
size_t currentBiosWritePtr =
boost::endian::little_to_native(cachedBufferHeader.biosWritePtr);
- if (currentBiosWritePtr > queueSize)
+ if (currentBiosWritePtr > maxOffset)
{
throw std::runtime_error(fmt::format(
"[readErrorLogs] currentBiosWritePtr was '{}' which was bigger "
- "than queueSize '{}'",
- currentBiosWritePtr, queueSize));
+ "than maxOffset '{}'",
+ currentBiosWritePtr, maxOffset));
}
size_t currentReadPtr =
boost::endian::little_to_native(cachedBufferHeader.bmcReadPtr);
- if (currentReadPtr > queueSize)
+ if (currentReadPtr > maxOffset)
{
throw std::runtime_error(fmt::format(
"[readErrorLogs] currentReadPtr was '{}' which was bigger "
- "than queueSize '{}'",
- currentReadPtr, queueSize));
+ "than maxOffset '{}'",
+ currentReadPtr, maxOffset));
}
size_t bytesToRead;
@@ -298,9 +294,9 @@
}
else
{
- // Calculate the bytes to the "end" (QueueSize - ReadPtr) +
+ // Calculate the bytes to the "end" (maxOffset - ReadPtr) +
// bytes to read from the "beginning" (0 + WritePtr)
- bytesToRead = (queueSize - currentReadPtr) + currentBiosWritePtr;
+ bytesToRead = (maxOffset - currentReadPtr) + currentBiosWritePtr;
}
size_t byteRead = 0;
@@ -325,4 +321,14 @@
return entryPairs;
}
+size_t BufferImpl::getMaxOffset()
+{
+ size_t queueSize =
+ boost::endian::little_to_native(cachedBufferHeader.queueSize);
+ size_t ueRegionSize =
+ boost::endian::little_to_native(cachedBufferHeader.ueRegionSize);
+
+ return queueSize - ueRegionSize - sizeof(struct CircularBufferHeader);
+}
+
} // namespace bios_bmc_smm_error_logger
diff --git a/test/buffer_test.cpp b/test/buffer_test.cpp
index 0fff5ee..a78897a 100644
--- a/test/buffer_test.cpp
+++ b/test/buffer_test.cpp
@@ -44,7 +44,7 @@
// CircularBufferHeader size is 0x30, ensure the test region is bigger
static constexpr size_t testRegionSize = 0x200;
static constexpr uint32_t testBmcInterfaceVersion = 123;
- static constexpr uint32_t testQueueSize = 0x100;
+ static constexpr uint32_t testQueueSize = 0x200;
static constexpr uint16_t testUeRegionSize = 0x50;
static constexpr std::array<uint32_t, 4> testMagicNumber = {
0x12345678, 0x22345678, 0x32345678, 0x42345678};
@@ -67,15 +67,14 @@
EXPECT_THROW(
try {
// Test too big of a proposed buffer compared to the memori size
- uint16_t bigQueueSize = 0x181;
+ uint16_t bigQueueSize = 0x201;
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'");
+ "[initialize] Proposed queue size '513' is bigger than the BMC's allocated MMIO region of '512'");
throw;
},
std::runtime_error);
@@ -83,18 +82,16 @@
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
- size_t testProposedBufferSize =
- sizeof(struct CircularBufferHeader) + testUeRegionSize + testQueueSize;
- const std::vector<uint8_t> emptyArray(testProposedBufferSize, 0);
+ const std::vector<uint8_t> emptyArray(testQueueSize, 0);
// Return a smaller write than the intended testRegionSize to test the error
EXPECT_CALL(*dataInterfaceMockPtr, write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testProposedBufferSize - 1));
+ .WillOnce(Return(testQueueSize - 1));
EXPECT_THROW(
try {
bufferImpl->initialize(testBmcInterfaceVersion, testQueueSize,
testUeRegionSize, testMagicNumber);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(e.what(), "[initialize] Only erased '383'");
+ EXPECT_STREQ(e.what(), "[initialize] Only erased '511'");
throw;
},
std::runtime_error);
@@ -103,7 +100,7 @@
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
EXPECT_CALL(*dataInterfaceMockPtr, write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testProposedBufferSize));
+ .WillOnce(Return(testQueueSize));
// Return a smaller write than the intended initializationHeader to test the
// error
EXPECT_CALL(*dataInterfaceMockPtr, write(0, _)).WillOnce(Return(0));
@@ -125,11 +122,9 @@
InSequence s;
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
- size_t testProposedBufferSize =
- sizeof(struct CircularBufferHeader) + testUeRegionSize + testQueueSize;
- const std::vector<uint8_t> emptyArray(testProposedBufferSize, 0);
+ const std::vector<uint8_t> emptyArray(testQueueSize, 0);
EXPECT_CALL(*dataInterfaceMockPtr, write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testProposedBufferSize));
+ .WillOnce(Return(testQueueSize));
uint8_t* testInitializationHeaderPtr =
reinterpret_cast<uint8_t*>(&testInitializationHeader);
@@ -258,12 +253,10 @@
InSequence s;
EXPECT_CALL(*dataInterfaceMockPtr, getMemoryRegionSize())
.WillOnce(Return(testRegionSize));
- size_t testProposedBufferSize = sizeof(struct CircularBufferHeader) +
- testUeRegionSize + testQueueSize;
- const std::vector<uint8_t> emptyArray(testProposedBufferSize, 0);
+ const std::vector<uint8_t> emptyArray(testQueueSize, 0);
EXPECT_CALL(*dataInterfaceMockPtr,
write(0, ElementsAreArray(emptyArray)))
- .WillOnce(Return(testProposedBufferSize));
+ .WillOnce(Return(testQueueSize));
EXPECT_CALL(*dataInterfaceMockPtr, write(0, _))
.WillOnce(Return(bufferHeaderSize));
@@ -275,33 +268,39 @@
static constexpr uint8_t expectedBmcReadPtrOffset = 0x21;
static constexpr size_t expectedqueueOffset = 0x30 + testUeRegionSize;
+ static constexpr size_t testMaxOffset =
+ testQueueSize - testUeRegionSize - sizeof(struct CircularBufferHeader);
uint8_t* testInitializationHeaderPtr =
reinterpret_cast<uint8_t*>(&testInitializationHeader);
};
+TEST_F(BufferWraparoundReadTest, GetMaxOffsetTest)
+{
+ EXPECT_EQ(bufferImpl->getMaxOffset(), testMaxOffset);
+}
+
TEST_F(BufferWraparoundReadTest, ParamsTooBigFail)
{
InSequence s;
-
- size_t tooBigOffset = testQueueSize + 1;
+ size_t tooBigOffset = testMaxOffset + 1;
EXPECT_THROW(
try {
bufferImpl->wraparoundRead(tooBigOffset, /* length */ 1);
} catch (const std::runtime_error& e) {
EXPECT_STREQ(
e.what(),
- "[wraparoundRead] relativeOffset '257' was bigger than queueSize '256'");
+ "[wraparoundRead] relativeOffset '385' was bigger than maxOffset '384'");
throw;
},
std::runtime_error);
- size_t tooBigLength = testQueueSize + 1;
+ size_t tooBigLength = testMaxOffset + 1;
EXPECT_THROW(
try {
bufferImpl->wraparoundRead(/* relativeOffset */ 0, tooBigLength);
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(e.what(), "[wraparoundRead] length '257' was bigger "
- "than queueSize '256'");
+ EXPECT_STREQ(e.what(), "[wraparoundRead] length '385' was bigger "
+ "than maxOffset '384'");
throw;
},
std::runtime_error);
@@ -364,7 +363,7 @@
InSequence s;
size_t testBytesLeft = 3;
size_t testLength = 0x10;
- size_t testOffset = testQueueSize - (testLength - testBytesLeft);
+ size_t testOffset = testMaxOffset - (testLength - testBytesLeft);
// Read until the end of the queue
std::vector<std::uint8_t> testBytesReadShort(testLength - testBytesLeft);
@@ -395,7 +394,7 @@
InSequence s;
size_t testBytesLeft = 3;
size_t testLength = 0x10;
- size_t testOffset = testQueueSize - (testLength - testBytesLeft);
+ size_t testOffset = testMaxOffset - (testLength - testBytesLeft);
// Read to the end of the queue
std::vector<std::uint8_t> testBytesReadFirst{16, 15, 14, 13, 12, 11, 10,
@@ -431,7 +430,7 @@
InSequence s;
size_t testBytesLeft = 0;
size_t testLength = 4;
- size_t testOffset = testQueueSize - (testLength - testBytesLeft);
+ size_t testOffset = testMaxOffset - (testLength - testBytesLeft);
// Read to the very end of the queue
std::vector<std::uint8_t> testBytes{4, 3, 2, 1};
@@ -471,7 +470,7 @@
std::span<std::uint8_t> expetedBytesOutput)
{
InSequence s;
- const uint32_t queueSizeToQueueEnd = testQueueSize - relativeOffset;
+ const uint32_t queueSizeToQueueEnd = testMaxOffset - relativeOffset;
// This will wrap, split the read mocks in 2
if (expetedBytesOutput.size() > queueSizeToQueueEnd)
@@ -555,7 +554,7 @@
testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);
// Set testOffset so that we can test the wraparound here at the header and
// update the readPtr
- testOffset = testQueueSize - 1;
+ testOffset = testMaxOffset - 1;
EXPECT_CALL(*dataInterfaceMockPtr, write(expectedBmcReadPtrOffset, _))
.WillOnce(Return(expectedWriteSize));
EXPECT_NO_THROW(bufferImpl->updateReadPtr(testOffset));
@@ -575,7 +574,7 @@
// Set testOffset so that we can test the wraparound here as well on our
// second read for the entry (by 1 byte)
- testOffset = testQueueSize - entryHeaderSize - 1;
+ testOffset = testMaxOffset - entryHeaderSize - 1;
EXPECT_CALL(*dataInterfaceMockPtr, write(expectedBmcReadPtrOffset, _))
.WillOnce(Return(expectedWriteSize));
EXPECT_NO_THROW(bufferImpl->updateReadPtr(testOffset));
@@ -606,7 +605,7 @@
InSequence s;
// Set the biosWritePtr too big
testInitializationHeader.biosWritePtr =
- boost::endian::native_to_little((testQueueSize + 1));
+ boost::endian::native_to_little((testMaxOffset + 1));
initializeFuncMock();
EXPECT_CALL(*dataInterfaceMockPtr, read(0, bufferHeaderSize))
@@ -618,8 +617,8 @@
bufferImpl->readErrorLogs();
} catch (const std::runtime_error& e) {
EXPECT_STREQ(e.what(),
- "[readErrorLogs] currentBiosWritePtr was '257' "
- "which was bigger than queueSize '256'");
+ "[readErrorLogs] currentBiosWritePtr was '385' "
+ "which was bigger than maxOffset '384'");
throw;
},
std::runtime_error);
@@ -628,7 +627,7 @@
testInitializationHeader.biosWritePtr = 0;
initializeFuncMock();
testInitializationHeader.bmcReadPtr =
- boost::endian::native_to_little((testQueueSize + 1));
+ boost::endian::native_to_little((testMaxOffset + 1));
initializeFuncMock();
EXPECT_CALL(*dataInterfaceMockPtr, read(0, bufferHeaderSize))
@@ -639,8 +638,8 @@
try {
bufferImpl->readErrorLogs();
} catch (const std::runtime_error& e) {
- EXPECT_STREQ(e.what(), "[readErrorLogs] currentReadPtr was '257' "
- "which was bigger than queueSize '256'");
+ EXPECT_STREQ(e.what(), "[readErrorLogs] currentReadPtr was '385' "
+ "which was bigger than maxOffset '384'");
throw;
},
std::runtime_error);
@@ -685,7 +684,7 @@
{
InSequence s;
// Set the bmcReadPtr to 1 entryHeader + entry size from the "end" exactly
- uint32_t entryAndHeaderSizeAwayFromEnd = testQueueSize - entryAndHeaderSize;
+ uint32_t entryAndHeaderSizeAwayFromEnd = testMaxOffset - entryAndHeaderSize;
testInitializationHeader.bmcReadPtr =
boost::endian::native_to_little(entryAndHeaderSizeAwayFromEnd);
// Set the biosWritePtr to 1 entryHeader + entry size from the "beginning"