vpnor: Hijack protocol rather than transport

By hijacking the transport the changes in behaviour were limited to the
mailbox interface. Now that we have a DBus interface as well this would
lead to inconsistent behaviour dependent on the transport.

Instead of hooking the transport, push the hook down to the protocol
level where we will achieve consistent behaviour across all transports.

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

Makefile.am

@@ -19,7 +19,8 @@ if VIRTUAL_PNOR_ENABLED
 include vpnor/Makefile.am.include
 else
 mboxd_SOURCES += flash.c \
-	lpc_reset.c
+	lpc_reset.c \
+	protocol_negotiate_version.c
 endif
 
 mboxctl_SOURCES = mboxctl.c

protocol.c

@@ -448,55 +448,9 @@ int protocol_v2_close(struct mbox_context *context, struct protocol_close *io)
 	return 0;
 }
 
-static const struct protocol_ops protocol_ops_v1 = {
-	.reset = protocol_v1_reset,
-	.get_info = protocol_v1_get_info,
-	.get_flash_info = protocol_v1_get_flash_info,
-	.create_window = protocol_v1_create_window,
-	.mark_dirty = protocol_v1_mark_dirty,
-	.erase = NULL,
-	.flush = protocol_v1_flush,
-	.close = protocol_v1_close,
-	.ack = protocol_v1_ack,
-};
-
-static const struct protocol_ops protocol_ops_v2 = {
-	.reset = protocol_v1_reset,
-	.get_info = protocol_v2_get_info,
-	.get_flash_info = protocol_v2_get_flash_info,
-	.create_window = protocol_v2_create_window,
-	.mark_dirty = protocol_v2_mark_dirty,
-	.erase = protocol_v2_erase,
-	.flush = protocol_v2_flush,
-	.close = protocol_v2_close,
-	.ack = protocol_v1_ack,
-};
-
-static const struct protocol_ops *protocol_ops_map[] = {
-	[0] = NULL,
-	[1] = &protocol_ops_v1,
-	[2] = &protocol_ops_v2,
-};
-
-int protocol_negotiate_version(struct mbox_context *context,
-				   uint8_t requested)
-{
-	/* Check we support the version requested */
-	if (requested < API_MIN_VERSION)
-		return -EINVAL;
-
-	context->version = (requested > API_MAX_VERSION) ?
-				API_MAX_VERSION : requested;
-
-	context->protocol = protocol_ops_map[context->version];
-
-	return context->version;
-}
-
 int protocol_init(struct mbox_context *context)
 {
-	context->version = API_MAX_VERSION;
-	context->protocol = protocol_ops_map[context->version];
+	protocol_negotiate_version(context, API_MAX_VERSION);
 
 	return 0;
 }

protocol_negotiate_version.c

+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2018 IBM Corp.
+#include "config.h"
+
+#include <errno.h>
+
+#include "mbox.h"
+#include "protocol.h"
+
+static const struct protocol_ops protocol_ops_v1 = {
+	.reset = protocol_v1_reset,
+	.get_info = protocol_v1_get_info,
+	.get_flash_info = protocol_v1_get_flash_info,
+	.create_window = protocol_v1_create_window,
+	.mark_dirty = protocol_v1_mark_dirty,
+	.erase = NULL,
+	.flush = protocol_v1_flush,
+	.close = protocol_v1_close,
+	.ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops protocol_ops_v2 = {
+	.reset = protocol_v1_reset,
+	.get_info = protocol_v2_get_info,
+	.get_flash_info = protocol_v2_get_flash_info,
+	.create_window = protocol_v2_create_window,
+	.mark_dirty = protocol_v2_mark_dirty,
+	.erase = protocol_v2_erase,
+	.flush = protocol_v2_flush,
+	.close = protocol_v2_close,
+	.ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops *protocol_ops_map[] = {
+	[0] = NULL,
+	[1] = &protocol_ops_v1,
+	[2] = &protocol_ops_v2,
+};
+
+int protocol_negotiate_version(struct mbox_context *context,
+				   uint8_t requested)
+{
+	/* Check we support the version requested */
+	if (requested < API_MIN_VERSION)
+		return -EINVAL;
+
+	context->version = (requested > API_MAX_VERSION) ?
+				API_MAX_VERSION : requested;
+
+	context->protocol = protocol_ops_map[context->version];
+
+	return context->version;
+}

test/Makefile.am.include

@@ -25,7 +25,8 @@ TEST_MBOX_SRCS = \
 	lpc_reset.c \
 	common.c \
 	flash.c \
-	protocol.c
+	protocol.c \
+	protocol_negotiate_version.c
 
 TEST_MOCK_SRCS = %reldir%/tmpf.c %reldir%/mbox.c %reldir%/system.c
 

transport_mbox.c

@@ -47,7 +47,7 @@ static const struct errno_map errno_map_v1[] = {
 
 static const struct errno_map errno_map_v2[] = {
 	{ 0, MBOX_R_SUCCESS },
-	{ EACCES, MBOX_R_PARAM_ERROR },
+	{ EACCES, MBOX_R_WINDOW_ERROR },
 	{ EBUSY, MBOX_R_BUSY },
 	{ EINVAL, MBOX_R_PARAM_ERROR },
 	{ EPERM, MBOX_R_WINDOW_ERROR },

vpnor/Makefile.am.include

@@ -3,7 +3,8 @@ mboxd_SOURCES += %reldir%/pnor_partition_table.cpp \
 	%reldir%/flash.cpp \
 	%reldir%/pnor_partition.cpp \
 	%reldir%/lpc_reset.cpp \
-	%reldir%/transport_mbox.cpp
+	%reldir%/protocol.cpp \
+	%reldir%/protocol_negotiate_version.cpp
 
 mboxd_LDFLAGS += -lstdc++fs \
 	$(SDBUSPLUS_LIBS) \

vpnor/mboxd_pnor_partition_table.cpp

@@ -13,7 +13,6 @@ extern "C" {
 #include "xyz/openbmc_project/Common/error.hpp"
 #include <phosphor-logging/elog-errors.hpp>
 #include <experimental/filesystem>
-#include "vpnor/transport_mbox.hpp"
 
 int init_vpnor(struct mbox_context *context)
 {
@@ -48,8 +47,6 @@ int init_vpnor_from_paths(struct mbox_context *context)
 
     if (context && !context->vpnor)
     {
-        context->handlers = vpnor_mbox_handlers;
-
         try
         {
             context->vpnor = new vpnor_partition_table;

vpnor/transport_mbox.cpp → vpnor/protocol.cpp

+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2018 IBM Corp.
 #include "config.h"
 
 extern "C" {
 #include "mbox.h"
-#include "transport_mbox.h"
-};
+#include "protocol.h"
+#include "vpnor/protocol.h"
+}
 
-#include "vpnor/transport_mbox.hpp"
 #include "vpnor/pnor_partition_table.hpp"
 
-// clang-format off
-const mboxd_mbox_handler vpnor_mbox_handlers[NUM_MBOX_CMDS] =
-{
-	mbox_handle_reset,
-	mbox_handle_mbox_info,
-	mbox_handle_flash_info,
-	mbox_handle_read_window,
-	mbox_handle_close_window,
-	vpnor_handle_write_window,
-	mbox_handle_dirty_window,
-	mbox_handle_flush_window,
-	mbox_handle_ack,
-	mbox_handle_erase_window
-};
-// clang-format on
-
 /* XXX: Maybe this should be a method on a class? */
-static bool vpnor_partition_is_readonly(const pnor_partition& part)
+static bool vpnor_partition_is_readonly(const pnor_partition &part)
 {
     return part.data.user.data[1] & PARTITION_READONLY;
 }
 
-int vpnor_handle_write_window(struct mbox_context* context,
-                              union mbox_regs* req, struct mbox_msg* resp)
+typedef int (*create_window_fn)(struct mbox_context *context,
+                                struct protocol_create_window *io);
+
+static int generic_vpnor_create_window(struct mbox_context *context,
+                                       struct protocol_create_window *io,
+                                       create_window_fn create_window)
 {
-    size_t offset = get_u16(&req->msg.args[0]);
+    if (io->req.ro)
+    {
+        return create_window(context, io);
+    }
+
+    /* Only allow write windows on regions mapped by the ToC as writeable */
+    size_t offset = io->req.offset;
     offset <<= context->block_size_shift;
     try
     {
-        const pnor_partition& part = context->vpnor->table->partition(offset);
+        const pnor_partition &part = context->vpnor->table->partition(offset);
         if (vpnor_partition_is_readonly(part))
         {
-            return -MBOX_R_WINDOW_ERROR;
+            return -EPERM;
         }
     }
-    catch (const openpower::virtual_pnor::UnmappedOffset& e)
+    catch (const openpower::virtual_pnor::UnmappedOffset &e)
     {
         /*
          * Writes to unmapped areas are not meaningful, so deny the request.
          * This removes the ability for a compromised host to abuse unused
          * space if any data was to be persisted (which it isn't).
          */
-        return -MBOX_R_WINDOW_ERROR;
+        return -EACCES;
     }
 
-    /* Defer to the default handler */
-    return mbox_handle_write_window(context, req, resp);
+    return create_window(context, io);
+}
+
+int protocol_v1_vpnor_create_window(struct mbox_context *context,
+                                    struct protocol_create_window *io)
+{
+    return generic_vpnor_create_window(context, io, protocol_v1_create_window);
+}
+
+int protocol_v2_vpnor_create_window(struct mbox_context *context,
+                                    struct protocol_create_window *io)
+{
+    return generic_vpnor_create_window(context, io, protocol_v2_create_window);
 }

vpnor/protocol.h

+/* SPDX-License-Identifier: Apache-2.0 */
+/* Copyright (C) 2018 IBM Corp. */
+#ifndef VPNOR_PROTOCOL_H
+#define VPNOR_PROTOCOL_H
+
+#include "protocol.h"
+
+/* Protocol v1 */
+int protocol_v1_vpnor_create_window(struct mbox_context *context,
+                                    struct protocol_create_window *io);
+
+/* Protocol v2 */
+int protocol_v2_vpnor_create_window(struct mbox_context *context,
+			            struct protocol_create_window *io);
+
+#endif /* VPNOR_PROTOCOL_H */

vpnor/protocol_negotiate_version.cpp

+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2018 IBM Corp.
+#include "config.h"
+
+#include <errno.h>
+
+extern "C" {
+#include "mbox.h"
+#include "protocol.h"
+#include "vpnor/protocol.h"
+}
+
+/* We need to hijack create_window for vpnor */
+
+static const struct protocol_ops protocol_ops_v1 = {
+    .reset = protocol_v1_reset,
+    .get_info = protocol_v1_get_info,
+    .get_flash_info = protocol_v1_get_flash_info,
+    .create_window = protocol_v1_vpnor_create_window,
+    .mark_dirty = protocol_v1_mark_dirty,
+    .erase = NULL,
+    .flush = protocol_v1_flush,
+    .close = protocol_v1_close,
+    .ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops protocol_ops_v2 = {
+    .reset = protocol_v1_reset,
+    .get_info = protocol_v2_get_info,
+    .get_flash_info = protocol_v2_get_flash_info,
+    .create_window = protocol_v2_vpnor_create_window,
+    .mark_dirty = protocol_v2_mark_dirty,
+    .erase = protocol_v2_erase,
+    .flush = protocol_v2_flush,
+    .close = protocol_v2_close,
+    .ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops *protocol_ops_map[] = {
+    [0] = NULL,
+    [1] = &protocol_ops_v1,
+    [2] = &protocol_ops_v2,
+};
+
+int protocol_negotiate_version(struct mbox_context *context, uint8_t requested)
+{
+    /* Check we support the version requested */
+    if (requested < API_MIN_VERSION)
+        return -EINVAL;
+
+    uint8_t version =
+        (requested > API_MAX_VERSION) ? API_MAX_VERSION : requested;
+    context->version = static_cast<enum api_version>(version);
+
+    context->protocol = protocol_ops_map[context->version];
+
+    return context->version;
+}

vpnor/test/Makefile.am.include

@@ -12,9 +12,10 @@ TEST_MBOX_VPNOR_INTEG_SRCS = \
 	vpnor/lpc_reset.cpp \
 	vpnor/mboxd_pnor_partition_table.cpp \
 	vpnor/flash.cpp \
-	vpnor/transport_mbox.cpp \
 	vpnor/pnor_partition.cpp \
 	vpnor/pnor_partition_table.cpp \
+	vpnor/protocol.cpp \
+	vpnor/protocol_negotiate_version.cpp \
 	%reldir%/tmpd.cpp
 
 VPNOR_LDADD = -lstdc++fs \

vpnor/transport_mbox.hpp

-extern "C" {
-#include "mbox.h"
-
-extern const mboxd_mbox_handler vpnor_mbox_handlers[NUM_MBOX_CMDS];
-
-int vpnor_handle_write_window(struct mbox_context *context,
-                              union mbox_regs *req, struct mbox_msg *resp);
-};