From ae1edb940c65e13e610108203e6a1cba0ab424c7 Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Wed, 28 Feb 2018 23:16:48 +1030 Subject: 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 --- mboxd_flash_virtual.cpp | 53 ++++++++++++++++++++++++++++++++++------ pnor_partition_table.cpp | 19 +++++++------- pnor_partition_table.hpp | 33 +++++++++++++++++++++++++ test/vpnor/Makefile.am.include | 2 -- test/vpnor/toc_lookup_failed.cpp | 2 +- 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 #include @@ -27,6 +28,9 @@ extern "C" { #include #include +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(); + MSG_ERR("%s\n", e.what()); + phosphor::logging::commit(); 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(); + 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(); + 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(); + /* 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(); - 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; } -- cgit v1.2.1