commit 26660e9b26c0a9f54a80806244ab20d23e41f532
author Brandon Kim <brandonkim@google.com> Wed Jun 15 16:58:44 2022 -0700
committer Brandon Kim <brandonkim@google.com> Thu Jun 16 06:44:34 2022 +0000
tree 2d222ffe0523b94355f0ef7390972cf6b1753214
parent 40ce08e165579e1bc791cdf9278a44190f4d4014

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

src/buffer.cpp

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));