pnor_partition_table: Raise exception for unmapped offsets

Allow reads and writes of offsets that don't map onto partitions defined
in the ToC. Do so by ignoring the mapping failure and filling a window
with 0xff in the hole from the requested offset to the following
partition.

This change also removes the reliance on InternalFailure as the
exception of choice for communicating failures. We can do better without
the teeth-pulling required by phosphor-logging by translating custom
exceptions into phosphor-logging exceptions at the edges.

Change-Id: Ibfa961a66b0b979354c6dc226ccbe7e9fbafc16d
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

5 files changed, 90 insertions, 19 deletions

diff --git a/mboxd_flash_virtual.cpp b/mboxd_flash_virtual.cpp
index 1b94ca9..42d4ee6 100644
--- a/mboxd_flash_virtual.cpp
+++ b/mboxd_flash_virtual.cpp
@@ -18,6 +18,7 @@ extern "C" {
 #include "mboxd_flash.h"
 #include "mboxd_pnor_partition_table.h"
 #include "pnor_partition.hpp"
+#include "pnor_partition_table.hpp"
 #include "xyz/openbmc_project/Common/error.hpp"
 #include <phosphor-logging/log.hpp>
 #include <phosphor-logging/elog-errors.hpp>
@@ -27,6 +28,9 @@ extern "C" {
 #include <exception>
 #include <stdexcept>
 
+namespace err = sdbusplus::xyz::openbmc_project::Common::Error;
+namespace vpnor = openpower::virtual_pnor;
+
 /** @brief unique_ptr functor to release a char* reference. */
 struct StringDeleter
 {
@@ -167,9 +171,25 @@ int64_t copy_flash(struct mbox_context* context, uint32_t offset, void* mem,
             munmap(mapped_mem, partitionInfo->data.actual);
         }
     }
-    catch (InternalFailure& e)
+    catch (vpnor::UnmappedOffset& e)
+    {
+        /*
+         * Hooo boy. Pretend that this is valid flash so we don't have
+         * discontiguous regions presented to the host. Instead, fill a window
+         * with 0xff so the 'flash' looks erased. Writes to such regions are
+         * dropped on the floor, see the implementation of write_flash() below.
+         */
+        MSG_INFO("Host requested unmapped region of %" PRId32
+                 " bytes at offset 0x%" PRIx32 "\n",
+                 size, offset);
+        uint32_t span = e.next - e.base;
+        rc = std::min(size, span);
+        memset(mem, 0xff, rc);
+    }
+    catch (std::exception& e)
     {
-        commit<InternalFailure>();
+        MSG_ERR("%s\n", e.what());
+        phosphor::logging::commit<err::InternalFailure>();
         rc = -MBOX_R_SYSTEM_ERROR;
     }
     return rc;
@@ -216,12 +236,16 @@ int write_flash(struct mbox_context* context, uint32_t offset, void* buf,
 
         // if the asked offset + no of bytes to write is greater
         // then size of the partition file then throw error.
+        //
+        // FIXME: Don't use .actual, use  (.size << ctx->block_size_shift),
+        // otherwise we can't grow the size of the data to fill the partition
         if ((offset + count) > (baseOffset + partitionInfo->data.actual))
         {
-            MSG_ERR("Offset is beyond the partition file length[0x%.8x]\n",
-                    partitionInfo->data.actual);
             munmap(mapped_mem, partitionInfo->data.actual);
-            elog<InternalFailure>();
+            std::stringstream err;
+            err << "Write extends beyond the partition length " << std::hex
+                << partitionInfo->data.actual;
+            throw vpnor::OutOfBoundsOffset(err.str());
         }
 
         auto diffOffset = offset - baseOffset;
@@ -230,9 +254,24 @@ int write_flash(struct mbox_context* context, uint32_t offset, void* buf,
 
         set_flash_bytemap(context, offset, count, FLASH_DIRTY);
     }
-    catch (InternalFailure& e)
+    catch (vpnor::UnmappedOffset& e)
+    {
+        /* Paper over the fact that the write isn't persistent */
+        MSG_INFO("Dropping %d bytes host wrote to unmapped offset 0x%" PRIx32
+                 "\n",
+                 count, offset);
+        return 0;
+    }
+    catch (const vpnor::OutOfBoundsOffset& e)
+    {
+        MSG_ERR("%s\n", e.what());
+        return -MBOX_R_PARAM_ERROR;
+    }
+    catch (const std::exception& e)
     {
-        rc = -1;
+        MSG_ERR("%s\n", e.what());
+        phosphor::logging::commit<err::InternalFailure>();
+        return -MBOX_R_SYSTEM_ERROR;
     }
     return rc;
 }
diff --git a/pnor_partition_table.cpp b/pnor_partition_table.cpp
index c9ef3a3..50d7d59 100644
--- a/pnor_partition_table.cpp
+++ b/pnor_partition_table.cpp
@@ -149,13 +149,15 @@ const pnor_partition& Table::partition(size_t offset) const
         {
             return part;
         }
-    }
 
-    MSG_ERR("Partition corresponding to offset 0x%zx not found", offset);
-    elog<InternalFailure>();
+        /* Are we in a hole between partitions? */
+        if (blockOffset < part.data.base)
+        {
+            throw UnmappedOffset(offset, part.data.base * blockSize);
+        }
+    }
 
-    static pnor_partition p{};
-    return p;
+    throw UnmappedOffset(offset, pnorSize);
 }
 
 const pnor_partition& Table::partition(const std::string& name) const
@@ -170,10 +172,9 @@ const pnor_partition& Table::partition(const std::string& name) const
         }
     }
 
-    MSG_ERR("Partition '%s' not found", name.c_str());
-    elog<InternalFailure>();
-    static pnor_partition p{};
-    return p;
+    std::stringstream err;
+    err << "Partition " << name << " is not listed in the table of contents";
+    throw UnknownPartition(err.str());
 }
 
 } // namespace partition
diff --git a/pnor_partition_table.hpp b/pnor_partition_table.hpp
index 730e75b..88c8fb9 100644
--- a/pnor_partition_table.hpp
+++ b/pnor_partition_table.hpp
@@ -163,6 +163,8 @@ class Table
      *
      *  @returns const reference to pnor_partition, if found, else an
      *           exception will be thrown.
+     *
+     *  Throws: UnmappedOffset
      */
     const pnor_partition& partition(size_t offset) const;
 
@@ -172,6 +174,8 @@ class Table
      *
      *  @returns const reference to pnor_partition, if found, else an
      *           exception will be thrown.
+     *
+     *  Throws: UnknownPartition
      */
     const pnor_partition& partition(const std::string& name) const;
 
@@ -304,6 +308,35 @@ class InvalidTocEntry : public TocEntryError
     }
 };
 
+class UnmappedOffset : public std::exception
+{
+  public:
+    UnmappedOffset(size_t base, size_t next) : base(base), next(next)
+    {
+    }
+
+    const size_t base;
+    const size_t next;
+};
+
+class OutOfBoundsOffset : public ReasonedError
+{
+  public:
+    OutOfBoundsOffset(const std::string&& reason) :
+        ReasonedError(std::move(reason))
+    {
+    }
+};
+
+class UnknownPartition : public ReasonedError
+{
+  public:
+    UnknownPartition(const std::string&& reason) :
+        ReasonedError(std::move(reason))
+    {
+    }
+};
+
 } // namespace virtual_pnor
 } // namespace openpower
 
diff --git a/test/vpnor/Makefile.am.include b/test/vpnor/Makefile.am.include
index e8d51ca..2db5b8e 100644
--- a/test/vpnor/Makefile.am.include
+++ b/test/vpnor/Makefile.am.include
@@ -227,6 +227,4 @@ check_PROGRAMS += \
 	%reldir%/create_read_window_oob \
 	%reldir%/create_read_window_toc \
 	%reldir%/create_read_window_straddle_partitions
-
-XFAIL_TESTS += %reldir%/create_read_window_oob
 endif
diff --git a/test/vpnor/toc_lookup_failed.cpp b/test/vpnor/toc_lookup_failed.cpp
index c38bcbd..a35975a 100644
--- a/test/vpnor/toc_lookup_failed.cpp
+++ b/test/vpnor/toc_lookup_failed.cpp
@@ -45,7 +45,7 @@ int main()
     {
         table.partition("TWO");
     }
-    catch (err::InternalFailure& e)
+    catch (vpnor::UnknownPartition& e)
     {
         return 0;
     }