[{"id":1766085,"web_url":"http://patchwork.ozlabs.org/comment/1766085/","msgid":"<1505113305.4080.1.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-11T07:01:45","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Thu, 2017-09-07 at 17:14 +1000, Suraj Jitindar Singh wrote:\n> Version 3 of the mbox protocol makes four protocol changes:\n>  - Add a requested block size argument to GET_MBOX_INFO\n>  - Add a no erase argument to MARK_DIRTY\n>  - Add a GET_FLASH_NAME command and support multiple flash devices\n>  - Add a MARK_LOCKED command\n> \n> Requested Block Size:\n> The requested block size argument has been added to the GET_MBOX_INFO\n> command to allow the host to request a specified block size which might\n> be required to allow data manipulation at a finer granularity. The\n> daemon should take this into account when choosing a block size for use\n> which it will specify in the response as before. The daemon has final\n> say and the host must use the block size the daemon chooses.\n> \n> No Erase:\n> The no erase argument to the mark dirty command allows a host to specify\n> that an area of flash should not be erased before being written to, as is\n> the default behaviour. This can be used when a host has already erased a\n> large area and then performs many small writes which would normally mean\n> many erases due to the implicit erase before write, making this slow.\n> \n> Add GET_FLASH_NAME command:\n> The ability to support multiple flash devices has been added so that the\n> mbox protocol can be used to arbitrate access from the host to a number\n> of flash devices which the daemon has access to. To facilitate this the\n> GET_FLASH_INFO, CREATE_{READ/WRITE}_WINDOW, and MARK_LOCKED commands now\n> take a flash ID, with the number of flash IDs allocated returned by the\n> GET_MBOX_INFO commands. There is also a new command GET_FLASH_NAME used\n> to convert a flash ID to a flash name.\n> \n> Add MARK_LOCKED command:\n> The MARK_LOCKED command has been added to allow an area(s) of flash to be\n> locked, that is that area must be treated as read only and the host is\n> not allowed to dirty or erase any windows which map that area of flash.\n> Additionally another error code LOCKED_ERROR was added to be returned\n> when the host does try to dirty or erase a locked area.\n> \n> The host cannot lock a currently dirty or erased area of the current\n> write window as it is not defined if the clean/dirty/erased value is\n> what should actually be \"locked\".\n> \n> A locked area of flash remains so until the daemon receives an\n> mboxctl --reset command and the locked areas are stored in a file on the\n> BMC filesystem to ensure persistence across BMC reboots/daemon crashes.\n> \n> Multiple flash device support proposed and defined by:\n> > William A. Kennington III \n> \n> > Signed-off-by: Suraj Jitindar Singh \n> ---\n>  Documentation/mbox_protocol.md | 127 ++++++++++++++++++++++++++++++-----------\n>  1 file changed, 93 insertions(+), 34 deletions(-)\n> \n> diff --git a/Documentation/mbox_protocol.md b/Documentation/mbox_protocol.md\n> index bcd70a8..74863a7 100644\n> --- a/Documentation/mbox_protocol.md\n> +++ b/Documentation/mbox_protocol.md\n> @@ -17,19 +17,21 @@ limitations under the License.\n>  This document describes a protocol for host to BMC communication via the\n>  mailbox registers present on the Aspeed 2400 and 2500 chips.\n>  This protocol is specifically designed to allow a host to request and manage\n> -access to the flash with the specifics of how the host is required to control\n> -this described below.\n> +access to a flash device with the specifics of how the host is required to\n> +control this described below.\n>  \n>  ## Version\n>  \n> -Both version 1 and version 2 of the protocol are described below with version 2\n> -specificities represented with V2 in brackets - (V2).\n> +Version specific protocol functionalities are represented by the version number\n> +in brackets next to the definition of the functionality. (e.g. (V2) for version\n> +2 specific funtionality). All version specific functionality must also be\n> +implemented by proceeding versions.\n\nI don't know that this is something we want to lock ourselves into. We\nnever want to remove functionality? What if a command turns out to be\ndangerous?\n\n>  \n>  ## Problem Overview\n>  \n>  \"mbox\" is the name we use to represent a protocol we have established between\n>  the host and the BMC via the Aspeed mailbox registers. This protocol is used\n> -for the host to control the flash.\n> +for the host to control access to the flash device(s).\n>  \n>  Prior to the mbox protocol, the host uses a backdoor into the BMC address space\n>  (the iLPC-to-AHB bridge) to directly manipulate the BMCs own flash controller.\n> @@ -280,6 +282,14 @@ The host is not required to perform an erase before a write command and the\n>  BMC must ensure that a write performs as expected - that is if an erase is\n>  required before a write then the BMC must perform this itself.\n>  \n> +The host may lock an area of flash using the MARK_LOCKED command. Any attempt\n> +to mark dirty or erased this area of flash must fail with the LOCKED_ERROR\n> +response code. The host may open a write window which contains a locked area\n> +of flash however changes to a locked area of flash must never be written back\n> +to the backing data source (i.e. that area of flash must be treated as read\n> +only). An attempt to lock an area of flash which is not clean in the current\n> +window must fail with PARAM_ERROR. (V3)\n\nI think we need to consider how locked regions interact with the in-memory\ncaching of the flash data.\n\nThe BMC doesn't know that the memory backing the LPC window has been\nwritten until it receives a MARK_DIRTY from the host. MARK_DIRTY is not\nvalid on a read window or a locked region, but that doesn't mean that\nchanges to a locked region can't persist in memory due to caching to\navoid flash access (which was a part of the motivation for memory-based \nbooting to begin with). This may naively fool host firmware into thinking it is\naccessing unmolested data as the region is locked, whereas the region could\ncontain anything, just it will never be written to flash.\n\nShould we require window requests intersecting a locked region always have at\nleast the locked region positively match the on-flash data? That way the\nfirmware that requested a region be locked could ensure it has whatever was on\nthe flash at the time it was locked by explicitly closing the window (read or\nwrite) once finished with it, which requires a window be opened again for any\nsubsequent access. At that point (open) we can do the locked region intesection\ncalculation and check the data in the window as necessary.\n\nLocked regions could thus be a performance hit if we always load the locked\nregions from flash, but surely we prefer (some) integrity over performance.\n\nAlternatively, you could cryptographically hash the per-block content of the\non-flash locked region during the MARK_LOCKED operation, then stash the hash\nvalues away. On a CREATE_{READ,WRITE}_WINDOW operation you could hash any\nintersecting, in-cache locked blocks and compare to the stashed hash values to\nincur only one set of flash accesses (initial hash calculations) rather than n.\n\n> +\n>  ### BMC Events\n>  \n>  The BMC can raise events with the host asynchronously to communicate to the\n> @@ -316,6 +326,8 @@ MARK_WRITE_DIRTY     0x07\n>  WRITE_FLUSH          0x08\n>  BMC_EVENT_ACK        0x09\n>  MARK_WRITE_ERASED    0x0a\t(V2)\n> +GET_FLASH_NAME       0x0b\t(V3)\n> +MARK_LOCKED          0x0c\t(V3)\n>  ```\n>  \n>  ### Responses\n> @@ -329,6 +341,7 @@ TIMEOUT\t\t5\n>  BUSY\t\t6\t(V2)\n>  WINDOW_ERROR\t7\t(V2)\n>  SEQ_ERROR\t8\t(V2)\n> +LOCKED_ERROR\t9\t(V3)\n>  ```\n>  \n>  ### Sequence Numbers\n> @@ -368,6 +381,10 @@ BUSY\t\t- Daemon in suspended state (currently unable to access flash)\n>  WINDOW_ERROR\t- Command not valid for active window or no active window\n>  \t\t- Try opening an appropriate window and retrying the command\n>  \n> +SEQ_ERROR\t- Invalid sequence number supplied with command\n> +\n> +LOCKED_ERROR\t- Tried to mark dirty or erased locked area of flash\n> +\n>  ### Information\n>  - All multibyte messages are LSB first (little endian)\n>  - All responses must have a valid return code in byte 13\n> @@ -394,9 +411,7 @@ Sizes and addresses specified in blocks must be converted to bytes by\n>  multiplying by the block size.\n>  ```\n>  Command:\n> -\tRESET_STATE\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n\nAddressing my concern above about locking ourselves in, I prefer we\nallow for a set of statements like:\n\n    Added in: V1\n    Removed in: V4\n\nI guess we continue to annotate the command argument registers as we have been\nalready.\n\n> +\tRESET_STATE (V1)\n>  \tArguments:\n>  \t\t-\n>  \tResponse:\n> @@ -408,9 +423,7 @@ Command:\n>  \t\tpre mailbox protocol. Final behavior is still TBD.\n>  \n>  Command:\n> -\tGET_MBOX_INFO\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n> +\tGET_MBOX_INFO (V1)\n>  \tArguments:\n>  \t\tV1:\n>  \t\tArgs 0: API version\n> @@ -418,6 +431,10 @@ Command:\n>  \t\tV2:\n>  \t\tArgs 0: API version\n>  \n> +\t\tV3:\n> +\t\tArgs 0: API version\n> +\t\tArgs 1: Requested block size (shift)\n> +\n>  \tResponse:\n>  \t\tV1:\n>  \t\tArgs 0: API version\n> @@ -430,6 +447,14 @@ Command:\n>  \t\tArgs 3-4: reserved\n>  \t\tArgs 5: Block size as power of two (encoded as a shift)\n>  \t\tArgs 6-7: Suggested Timeout (seconds)\n> +\n> +\t\tV3:\n> +\t\tArgs 0: API version\n> +\t\tArgs 1-2: reserved\n> +\t\tArgs 3-4: reserved\n> +\t\tArgs 5: Block size as power of two (encoded as a shift)\n> +\t\tArgs 6-7: Suggested Timeout (seconds)\n> +\t\tArgs 8: Num Allocated Flash IDs\n>  \tNotes:\n>  \t\tThe suggested timeout is a hint to the host as to how long\n>  \t\tit should wait after issuing a command to the BMC before it\n> @@ -439,25 +464,32 @@ Command:\n>  \t\tthe BMC\tdoes not wish to provide a hint in which case the host\n>  \t\tmust choose some reasonable value.\n>  \n> +\t\tThe host may require\n\nI'd use 'desire' instead of 'require', because the host must obey the\nBMC's response regarding the blocksize.\n\n>  a specific block size and thus can request\n> +\t\tthis by giving a hint to the daemon (may be zero). The daemon\n> +\t\tmay use this to select the block size which it will use however\n> +\t\tis free to ignore this.\n\nMaybe 'it' instead of 'this' at the end of the sentence?\n\n> The value in the response is the block\n> +\t\tsize which must be used for all further requests until a new\n> +\t\tsize is\tnegotiated by another call to GET_MBOX_INFO. (V3)\n> +\n>  Command:\n> -\tGET_FLASH_INFO\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n> +\tGET_FLASH_INFO (V1)\n>  \tArguments:\n> +\t\tV1, V2:\n>  \t\t-\n> +\n> +\t\tV3:\n> +\t\tArgs 0: Flash ID\n>  \tResponse:\n>  \t\tV1:\n>  \t\tArgs 0-3: Flash size (bytes)\n>  \t\tArgs 4-7: Erase granule (bytes)\n>  \n> -\t\tV2:\n> +\t\tV2, V3:\n>  \t\tArgs 0-1: Flash size (blocks)\n>  \t\tArgs 2-3: Erase granule (blocks)\n>  \n>  Command:\n> -\tCREATE_{READ/WRITE}_WINDOW\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n> +\tCREATE_{READ/WRITE}_WINDOW (V1)\n>  \tArguments:\n>  \t\tV1:\n>  \t\tArgs 0-1: Requested flash offset (blocks)\n> @@ -466,11 +498,15 @@ Command:\n>  \t\tArgs 0-1: Requested flash offset (blocks)\n>  \t\tArgs 2-3: Requested flash size to access (blocks)\n>  \n> +\t\tV3:\n> +\t\tArgs 0-1: Requested flash offset (blocks)\n> +\t\tArgs 2-3: Requested flash size to access (blocks)\n> +\t\tArgs 4: Flash ID\n>  \tResponse:\n>  \t\tV1:\n>  \t\tArgs 0-1: LPC bus address of window (blocks)\n>  \n> -\t\tV2:\n> +\t\tV2, V3:\n>  \t\tArgs 0-1: LPC bus address of window (blocks)\n>  \t\tArgs 2-3: Window size (blocks)\n>  \t\tArgs 4-5: Flash offset mapped by window (blocks)\n> @@ -504,9 +540,7 @@ Command:\n>  \t\twindow.\n>  \n>  Command:\n> -\tCLOSE_WINDOW\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n> +\tCLOSE_WINDOW (V1)\n>  \tArguments:\n>  \t\tV1:\n>  \t\t-\n> @@ -533,9 +567,7 @@ Command:\n>  \t\t\t\tevicted from the cache.\n>  \n>  Command:\n> -\tMARK_WRITE_DIRTY\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n> +\tMARK_WRITE_DIRTY (V1)\n>  \tArguments:\n>  \t\tV1:\n>  \t\tArgs 0-1: Flash offset to mark from base of flash (blocks)\n> @@ -544,6 +576,7 @@ Command:\n>  \t\tV2:\n>  \t\tArgs 0-1: Window offset to mark (blocks)\n>  \t\tArgs 2-3: Number to mark dirty at offset (blocks)\n> +\t\tArgs 4  : Don't Erase Before Write (V3)\n>  \n>  \tResponse:\n>  \t\t-\n> @@ -558,10 +591,15 @@ Command:\n>  \t\tblock. If the offset + number exceeds the size of the active\n>  \t\twindow then the command must not succeed.\n>  \n> +\t\tThe host can give a hint to the daemon that is doesn't have to\n> +\t\terase a flash area before writing to it by setting ARG[4]. This\n> +\t\tmeans that the daemon will blindly perform a write to that area\n> +\t\tand will not try to erase it before hand. This can be used if\n> +\t\tthe host knows that a large area has already been erased for\n> +\t\texample but then wants to perform many small writes.\n> +\n>  Command\n> -\tWRITE_FLUSH\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n> +\tWRITE_FLUSH (V1)\n>  \tArguments:\n>  \t\tV1:\n>  \t\tArgs 0-1: Flash offset to mark from base of flash (blocks)\n> @@ -585,9 +623,7 @@ Command\n>  \n>  \n>  Command:\n> -\tBMC_EVENT_ACK\n> -\tImplemented in Versions:\n> -\t\tV1, V2\n> +\tBMC_EVENT_ACK (V1)\n>  \tArguments:\n>  \t\tArgs 0:\tBits in the BMC status byte (mailbox data\n>  \t\t\tregister 15) to ack\n> @@ -598,9 +634,7 @@ Command:\n>  \t\tsupplied in mailbox register 15.\n>  \n>  Command:\n> -\tMARK_WRITE_ERASED\n> -\tImplemented in Versions:\n> -\t\tV2\n> +\tMARK_WRITE_ERASED (V2)\n>  \tArguments:\n>  \t\tV2:\n>  \t\tArgs 0-1: Window offset to erase (blocks)\n> @@ -617,6 +651,31 @@ Command:\n>  \t\tnumber is the number of blocks of the active window to erase\n>  \t\tstarting at offset. If the offset + number exceeds the size of\n>  \t\tthe active window then the command must not succeed.\n> +\n> +Command:\n> +\tGET_FLASH_NAME (V3)\n> +\tArguments:\n> +\t\tArgs 0: Flash ID\n> +\tResponse:\n> +\t\tArgs 0-10: Flash Name / UID\n\nHopefully 11 bytes is enough. Well, 10 I guess - should we specify that the\nvalue at index 10 be '\\0'? I feel like not requiring that could lead to\nundesirable implementations.\n\nI guess we don't want to go down the path of continued responses or magic\nwindows to accommodate larger names?\n\n> +\tNotes:\n> +\t\tDescribes a flash with some kind of identifier useful to the\n> +\t\thost system. This is typically a null-padded string.\n> +\n> +Command:\n> +\tMARK_LOCKED (V3)\n> +\tArguments:\n> +\t\tArgs 0-1: Flash offset to lock (blocks)\n> +\t\tArgs 2-3: Number to lock at offset (blocks)\n\nI guess it's hard to get away from using block-sized granuality. Do we\ncurrently have any partitions that we might lock that are less than a block\nsize? Or are all partitions expected to be block-size aligned?\n\nCheers,\n\nAndrew\n\n> +\t\tArgs 4: Flash ID\n> +\tResponse:\n> +\t\t-\n> +\tNotes:\n> +\t\tLock an area of flash so that the host can't mark it dirty or\n> +\t\terased. If the requested area is within the current window and\n> +\t\tthat area is currently marked dirty or erased then this command\n> +\t\tmust fail.\n> +\n>  ```\n>  \n>  ### BMC Events in Detail:","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrJl76nQVz9s2G\n\tfor ;\n\tMon, 11 Sep 2017 17:02:11 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xrJl75GcvzDrnt\n\tfor ;\n\tMon, 11 Sep 2017 17:02:11 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xrJkw1V60zDrZh;\n\tMon, 11 Sep 2017 17:01:59 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id AB2A320982;\n\tMon, 11 Sep 2017 03:01:54 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Mon, 11 Sep 2017 03:01:54 -0400","from keelia (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 96913248E6;\n\tMon, 11 Sep 2017 03:01:51 -0400 (EDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"FeEprU0M\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"CkI/JXIG\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"FeEprU0M\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"CkI/JXIG\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"FeEprU0M\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"CkI/JXIG\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=zh6/D+tN5axzw4cQnupvlEuD6nSFsEkbwztHYBa1G\n\td0=; b=FeEprU0MvUxN9CQ2P4WX0SVg1LCVgPXrA4Y88d/kpma18Xp0dP3jDntB1\n\t9ZNGkSEmyds1xJEBP5WPIjJT3AiTL9m3YcxiNMVS0Rm+dFdaC03w6JjPEaLp+WN4\n\tfO/3LD+b2vnHSyfUkyZuMCRJBspFxBwtiWYv/88DphiaRPo7nfH+0mOu+kVriWAF\n\tjPGHc01uUttMnmJnxUTXS+an1gVzf6Z+MZ7TltmYk9Sp6rxgr+qPMHY3n+K+L5qP\n\tZc8xc7Q1uoqjIn9ubYCbXUtauxPH37CVeYikVobWrh5WeiFBkc+EpZMvUuEWgD1A\n\trN8oijuw2jAkR5wPfnfZ9b0OlYF0A==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=zh6/D+tN5axzw4cQnu\n\tpvlEuD6nSFsEkbwztHYBa1Gd0=; b=CkI/JXIGgkADHccQeLuoM9H0u8ZtwRyycx\n\tM44GZBlPlXBEWzo2YiEHgnKAQYrTMnOFzB/KIL/bzb2Lme2dh0eQX0GwgmHm6t48\n\t7o73m/RWeAp5eM8iw63bcyGl8C9VMSnF33ikEaWP/Z/XlvxueebR1aXBu8hX2TKe\n\tUgT0CaGFNxiym16GjddVpMxuvJNj+omAUH2WrjbaXQhajcehYWOwCm0OgBcKntGD\n\tNlheJdI9tjmxPL+By9k7lL86yn1hXDnD/HRikwGFKlZjbl2Mu099p8JVN3Q4jBXJ\n\tJWosExtd29p8DINn9vUIYnvvTeGOupoMfs+HiGtG3z87Hw2XrywA=="],"X-ME-Sender":"","X-Sasl-enc":"ZYwVmRi66bO2VMUjZ3yhh3E66Qdx7U2tCfgwRLRe23oA 1505113313","Message-ID":"<1505113305.4080.1.camel@aj.id.au>","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","From":"Andrew Jeffery ","To":"Suraj Jitindar Singh ,\n\tskiboot@lists.ozlabs.org, openbmc@lists.ozlabs.org","Date":"Mon, 11 Sep 2017 16:31:45 +0930","In-Reply-To":"<20170907071409.7163-2-sjitindarsingh@gmail.com>","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-C8Sk2mxcmSR6FLLMDzTW\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, cyrilbur@gmail.comm, wak@google.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1766661,"web_url":"http://patchwork.ozlabs.org/comment/1766661/","msgid":"<1505189782.2222.1.camel@gmail.com>","list_archive_url":null,"date":"2017-09-12T04:16:22","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":68427,"url":"http://patchwork.ozlabs.org/api/people/68427/","name":"Suraj Jitindar Singh","email":"sjitindarsingh@gmail.com"},"content":"+Cyril because I can't type \".com\" :p\n\nHi,\n\nOn Mon, 2017-09-11 at 16:31 +0930, Andrew Jeffery wrote:\n> On Thu, 2017-09-07 at 17:14 +1000, Suraj Jitindar Singh wrote:\n> > Version 3 of the mbox protocol makes four protocol changes:\n> >  - Add a requested block size argument to GET_MBOX_INFO\n> >  - Add a no erase argument to MARK_DIRTY\n> >  - Add a GET_FLASH_NAME command and support multiple flash devices\n> >  - Add a MARK_LOCKED command\n> >  \n> > Requested Block Size:\n> > The requested block size argument has been added to the\n> > GET_MBOX_INFO\n> > command to allow the host to request a specified block size which\n> > might\n> > be required to allow data manipulation at a finer granularity. The\n> > daemon should take this into account when choosing a block size for\n> > use\n> > which it will specify in the response as before. The daemon has\n> > final\n> > say and the host must use the block size the daemon chooses.\n> >  \n> > No Erase:\n> > The no erase argument to the mark dirty command allows a host to\n> > specify\n> > that an area of flash should not be erased before being written to,\n> > as is\n> > the default behaviour. This can be used when a host has already\n> > erased a\n> > large area and then performs many small writes which would normally\n> > mean\n> > many erases due to the implicit erase before write, making this\n> > slow.\n> >  \n> > Add GET_FLASH_NAME command:\n> > The ability to support multiple flash devices has been added so\n> > that the\n> > mbox protocol can be used to arbitrate access from the host to a\n> > number\n> > of flash devices which the daemon has access to. To facilitate this\n> > the\n> > GET_FLASH_INFO, CREATE_{READ/WRITE}_WINDOW, and MARK_LOCKED\n> > commands now\n> > take a flash ID, with the number of flash IDs allocated returned by\n> > the\n> > GET_MBOX_INFO commands. There is also a new command GET_FLASH_NAME\n> > used\n> > to convert a flash ID to a flash name.\n> >  \n> > Add MARK_LOCKED command:\n> > The MARK_LOCKED command has been added to allow an area(s) of flash\n> > to be\n> > locked, that is that area must be treated as read only and the host\n> > is\n> > not allowed to dirty or erase any windows which map that area of\n> > flash.\n> > Additionally another error code LOCKED_ERROR was added to be\n> > returned\n> > when the host does try to dirty or erase a locked area.\n> >  \n> > The host cannot lock a currently dirty or erased area of the\n> > current\n> > write window as it is not defined if the clean/dirty/erased value\n> > is\n> > what should actually be \"locked\".\n> >  \n> > A locked area of flash remains so until the daemon receives an\n> > mboxctl --reset command and the locked areas are stored in a file\n> > on the\n> > BMC filesystem to ensure persistence across BMC reboots/daemon\n> > crashes.\n> >  \n> > Multiple flash device support proposed and defined by:\n> > > William A. Kennington III \n> > \n> >  \n> > > Signed-off-by: Suraj Jitindar Singh \n> > \n> > ---\n> >  Documentation/mbox_protocol.md | 127\n> > ++++++++++++++++++++++++++++++-----------\n> >  1 file changed, 93 insertions(+), 34 deletions(-)\n> >  \n> > diff --git a/Documentation/mbox_protocol.md\n> > b/Documentation/mbox_protocol.md\n> > index bcd70a8..74863a7 100644\n> > --- a/Documentation/mbox_protocol.md\n> > +++ b/Documentation/mbox_protocol.md\n> > @@ -17,19 +17,21 @@ limitations under the License.\n> >  This document describes a protocol for host to BMC communication\n> > via the\n> >  mailbox registers present on the Aspeed 2400 and 2500 chips.\n> >  This protocol is specifically designed to allow a host to request\n> > and manage\n> > -access to the flash with the specifics of how the host is required\n> > to control\n> > -this described below.\n> > +access to a flash device with the specifics of how the host is\n> > required to\n> > +control this described below.\n> >  \n> >  ## Version\n> >  \n> > -Both version 1 and version 2 of the protocol are described below\n> > with version 2\n> > -specificities represented with V2 in brackets - (V2).\n> > +Version specific protocol functionalities are represented by the\n> > version number\n> > +in brackets next to the definition of the functionality. (e.g.\n> > (V2) for version\n> > +2 specific funtionality). All version specific functionality must\n> > also be\n> > +implemented by proceeding versions.\n> \n> I don't know that this is something we want to lock ourselves into.\n> We\n> never want to remove functionality? What if a command turns out to be\n> dangerous?\n\nGood point :)\n\n> \n> >  \n> >  ## Problem Overview\n> >  \n> >  \"mbox\" is the name we use to represent a protocol we have\n> > established between\n> >  the host and the BMC via the Aspeed mailbox registers. This\n> > protocol is used\n> > -for the host to control the flash.\n> > +for the host to control access to the flash device(s).\n> >  \n> >  Prior to the mbox protocol, the host uses a backdoor into the BMC\n> > address space\n> >  (the iLPC-to-AHB bridge) to directly manipulate the BMCs own flash\n> > controller.\n> > @@ -280,6 +282,14 @@ The host is not required to perform an erase\n> > before a write command and the\n> >  BMC must ensure that a write performs as expected - that is if an\n> > erase is\n> >  required before a write then the BMC must perform this itself.\n> >  \n> > +The host may lock an area of flash using the MARK_LOCKED command.\n> > Any attempt\n> > +to mark dirty or erased this area of flash must fail with the\n> > LOCKED_ERROR\n> > +response code. The host may open a write window which contains a\n> > locked area\n> > +of flash however changes to a locked area of flash must never be\n> > written back\n> > +to the backing data source (i.e. that area of flash must be\n> > treated as read\n> > +only). An attempt to lock an area of flash which is not clean in\n> > the current\n> > +window must fail with PARAM_ERROR. (V3)\n> \n> I think we need to consider how locked regions interact with the in-\n> memory\n> caching of the flash data.\n> \n> The BMC doesn't know that the memory backing the LPC window has been\n> written until it receives a MARK_DIRTY from the host. MARK_DIRTY is\n> not\n> valid on a read window or a locked region, but that doesn't mean that\n> changes to a locked region can't persist in memory due to caching to\n> avoid flash access (which was a part of the motivation for memory-\n> based \n> booting to begin with). This may naively fool host firmware into\n> thinking it is\n> accessing unmolested data as the region is locked, whereas the region\n> could\n> contain anything, just it will never be written to flash.\n\nCorrect\n\n> \n> Should we require window requests intersecting a locked region always\n> have at\n> least the locked region positively match the on-flash data? That way\n> the\n> firmware that requested a region be locked could ensure it has\n> whatever was on\n> the flash at the time it was locked by explicitly closing the window\n> (read or\n> write) once finished with it, which requires a window be opened again\n> for any\n> subsequent access. At that point (open) we can do the locked region\n> intesection\n> calculation and check the data in the window as necessary.\n\nI guess this brings about an interesting question, when a window is\nrequested is it guaranteed that the window provided in return by the\nBMC contains the same contents as the flash for the requested area? It\nwould seem intuitively that it should. The first time a window is\nopened that is true, but with in memory caching the host could make a\nchange that it never tells the bmc about and the contents diverge, and\n(assuming the window isn't evicted) persist across window close/open\ncalls.\n\nIs this the desired behaviour? It's kind of the only behaviour which\nmakes the in-memory caching an effective performance improvement, if we\nreloaded every window every time then we may as well only have one\nwindow in memory anyway. Maybe we can do this a better way (see below).\n\nIt's worth noting that the current flash locking daemon implementation\nwill explicitly reload an entire window whenever one which contains any\nlocked regions is requested - ensuring (for at least as long as the\naccess to the flash device is exclusive) the integrity of the locked\nregions. This could of course be optimised to only reload the locked\nregion.\n\n> \n> Locked regions could thus be a performance hit if we always load the\n> locked\n> regions from flash, but surely we prefer (some) integrity over\n> performance.\n> \n> Alternatively, you could cryptographically hash the per-block content\n> of the\n> on-flash locked region during the MARK_LOCKED operation, then stash\n> the hash\n> values away. On a CREATE_{READ,WRITE}_WINDOW operation you could hash\n> any\n> intersecting, in-cache locked blocks and compare to the stashed hash\n> values to\n> incur only one set of flash accesses (initial hash calculations)\n> rather than n.\n\nThis seems like a nice balance between performance and data integrity.\n\nI guess this brings about the question again as to whether we care\nabout data changes we aren't told about. Do we store a hash of every\nwindow and before providing a cached copy verify the hash and reload\nthe entire window from flash again if there is a discrepancy. The host\nisn't allowed to rely on the persistence of data it never tells the BMC\nabout, but is this something the BMC should be checking?\n\n> \n> > +\n> >  ### BMC Events\n> >  \n> >  The BMC can raise events with the host asynchronously to\n> > communicate to the\n> > @@ -316,6 +326,8 @@ MARK_WRITE_DIRTY     0x07\n> >  WRITE_FLUSH          0x08\n> >  BMC_EVENT_ACK        0x09\n> >  MARK_WRITE_ERASED    0x0a\t(V2)\n> > +GET_FLASH_NAME       0x0b\t(V3)\n> > +MARK_LOCKED          0x0c\t(V3)\n> >  ```\n> >  \n> >  ### Responses\n> > @@ -329,6 +341,7 @@ TIMEOUT\t\t5\n> >  BUSY\t\t6\t(V2)\n> >  WINDOW_ERROR\t7\t(V2)\n> >  SEQ_ERROR\t8\t(V2)\n> > +LOCKED_ERROR\t9\t(V3)\n> >  ```\n> >  \n> >  ### Sequence Numbers\n> > @@ -368,6 +381,10 @@ BUSY\t\t- Daemon in suspended\n> > state (currently unable to access flash)\n> >  WINDOW_ERROR\t- Command not valid for active window or no\n> > active window\n> >  \t\t- Try opening an appropriate window and retrying\n> > the command\n> >  \n> > +SEQ_ERROR\t- Invalid sequence number supplied with command\n> > +\n> > +LOCKED_ERROR\t- Tried to mark dirty or erased locked area of\n> > flash\n> > +\n> >  ### Information\n> >  - All multibyte messages are LSB first (little endian)\n> >  - All responses must have a valid return code in byte 13\n> > @@ -394,9 +411,7 @@ Sizes and addresses specified in blocks must be\n> > converted to bytes by\n> >  multiplying by the block size.\n> >  ```\n> >  Command:\n> > -\tRESET_STATE\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> \n> Addressing my concern above about locking ourselves in, I prefer we\n> allow for a set of statements like:\n> \n>     Added in: V1\n>     Removed in: V4\n\nI support this idea.\n\n> \n> I guess we continue to annotate the command argument registers as we\n> have been\n> already.\n> \n> > +\tRESET_STATE (V1)\n> >  \tArguments:\n> >  \t\t-\n> >  \tResponse:\n> > @@ -408,9 +423,7 @@ Command:\n> >  \t\tpre mailbox protocol. Final behavior is still TBD.\n> >  \n> >  Command:\n> > -\tGET_MBOX_INFO\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> > +\tGET_MBOX_INFO (V1)\n> >  \tArguments:\n> >  \t\tV1:\n> >  \t\tArgs 0: API version\n> > @@ -418,6 +431,10 @@ Command:\n> >  \t\tV2:\n> >  \t\tArgs 0: API version\n> >  \n> > +\t\tV3:\n> > +\t\tArgs 0: API version\n> > +\t\tArgs 1: Requested block size (shift)\n> > +\n> >  \tResponse:\n> >  \t\tV1:\n> >  \t\tArgs 0: API version\n> > @@ -430,6 +447,14 @@ Command:\n> >  \t\tArgs 3-4: reserved\n> >  \t\tArgs 5: Block size as power of two (encoded as a\n> > shift)\n> >  \t\tArgs 6-7: Suggested Timeout (seconds)\n> > +\n> > +\t\tV3:\n> > +\t\tArgs 0: API version\n> > +\t\tArgs 1-2: reserved\n> > +\t\tArgs 3-4: reserved\n> > +\t\tArgs 5: Block size as power of two (encoded as a\n> > shift)\n> > +\t\tArgs 6-7: Suggested Timeout (seconds)\n> > +\t\tArgs 8: Num Allocated Flash IDs\n> >  \tNotes:\n> >  \t\tThe suggested timeout is a hint to the host as to\n> > how long\n> >  \t\tit should wait after issuing a command to the BMC\n> > before it\n> > @@ -439,25 +464,32 @@ Command:\n> >  \t\tthe BMC\tdoes not wish to provide a hint in\n> > which case the host\n> >  \t\tmust choose some reasonable value.\n> >  \n> > +\t\tThe host may require\n> \n> I'd use 'desire' instead of 'require', because the host must obey the\n> BMC's response regarding the blocksize.\n> \n> >   a specific block size and thus can request\n> > +\t\tthis by giving a hint to the daemon (may be zero).\n> > The daemon\n> > +\t\tmay use this to select the block size which it\n> > will use however\n> > +\t\tis free to ignore this.\n> \n> Maybe 'it' instead of 'this' at the end of the sentence?\n\nYep\n\n> \n> > The value in the response is the block\n> > +\t\tsize which must be used for all further requests\n> > until a new\n> > +\t\tsize is\tnegotiated by another call to\n> > GET_MBOX_INFO. (V3)\n> > +\n> >  Command:\n> > -\tGET_FLASH_INFO\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> > +\tGET_FLASH_INFO (V1)\n> >  \tArguments:\n> > +\t\tV1, V2:\n> >  \t\t-\n> > +\n> > +\t\tV3:\n> > +\t\tArgs 0: Flash ID\n> >  \tResponse:\n> >  \t\tV1:\n> >  \t\tArgs 0-3: Flash size (bytes)\n> >  \t\tArgs 4-7: Erase granule (bytes)\n> >  \n> > -\t\tV2:\n> > +\t\tV2, V3:\n> >  \t\tArgs 0-1: Flash size (blocks)\n> >  \t\tArgs 2-3: Erase granule (blocks)\n> >  \n> >  Command:\n> > -\tCREATE_{READ/WRITE}_WINDOW\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> > +\tCREATE_{READ/WRITE}_WINDOW (V1)\n> >  \tArguments:\n> >  \t\tV1:\n> >  \t\tArgs 0-1: Requested flash offset (blocks)\n> > @@ -466,11 +498,15 @@ Command:\n> >  \t\tArgs 0-1: Requested flash offset (blocks)\n> >  \t\tArgs 2-3: Requested flash size to access (blocks)\n> >  \n> > +\t\tV3:\n> > +\t\tArgs 0-1: Requested flash offset (blocks)\n> > +\t\tArgs 2-3: Requested flash size to access (blocks)\n> > +\t\tArgs 4: Flash ID\n> >  \tResponse:\n> >  \t\tV1:\n> >  \t\tArgs 0-1: LPC bus address of window (blocks)\n> >  \n> > -\t\tV2:\n> > +\t\tV2, V3:\n> >  \t\tArgs 0-1: LPC bus address of window (blocks)\n> >  \t\tArgs 2-3: Window size (blocks)\n> >  \t\tArgs 4-5: Flash offset mapped by window (blocks)\n> > @@ -504,9 +540,7 @@ Command:\n> >  \t\twindow.\n> >  \n> >  Command:\n> > -\tCLOSE_WINDOW\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> > +\tCLOSE_WINDOW (V1)\n> >  \tArguments:\n> >  \t\tV1:\n> >  \t\t-\n> > @@ -533,9 +567,7 @@ Command:\n> >  \t\t\t\tevicted from the cache.\n> >  \n> >  Command:\n> > -\tMARK_WRITE_DIRTY\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> > +\tMARK_WRITE_DIRTY (V1)\n> >  \tArguments:\n> >  \t\tV1:\n> >  \t\tArgs 0-1: Flash offset to mark from base of flash\n> > (blocks)\n> > @@ -544,6 +576,7 @@ Command:\n> >  \t\tV2:\n> >  \t\tArgs 0-1: Window offset to mark (blocks)\n> >  \t\tArgs 2-3: Number to mark dirty at offset (blocks)\n> > +\t\tArgs 4  : Don't Erase Before Write (V3)\n> >  \n> >  \tResponse:\n> >  \t\t-\n> > @@ -558,10 +591,15 @@ Command:\n> >  \t\tblock. If the offset + number exceeds the size of\n> > the active\n> >  \t\twindow then the command must not succeed.\n> >  \n> > +\t\tThe host can give a hint to the daemon that is\n> > doesn't have to\n> > +\t\terase a flash area before writing to it by setting\n> > ARG[4]. This\n> > +\t\tmeans that the daemon will blindly perform a write\n> > to that area\n> > +\t\tand will not try to erase it before hand. This can\n> > be used if\n> > +\t\tthe host knows that a large area has already been\n> > erased for\n> > +\t\texample but then wants to perform many small\n> > writes.\n> > +\n> >  Command\n> > -\tWRITE_FLUSH\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> > +\tWRITE_FLUSH (V1)\n> >  \tArguments:\n> >  \t\tV1:\n> >  \t\tArgs 0-1: Flash offset to mark from base of flash\n> > (blocks)\n> > @@ -585,9 +623,7 @@ Command\n> >  \n> >  \n> >  Command:\n> > -\tBMC_EVENT_ACK\n> > -\tImplemented in Versions:\n> > -\t\tV1, V2\n> > +\tBMC_EVENT_ACK (V1)\n> >  \tArguments:\n> >  \t\tArgs 0:\tBits in the BMC status byte\n> > (mailbox data\n> >  \t\t\tregister 15) to ack\n> > @@ -598,9 +634,7 @@ Command:\n> >  \t\tsupplied in mailbox register 15.\n> >  \n> >  Command:\n> > -\tMARK_WRITE_ERASED\n> > -\tImplemented in Versions:\n> > -\t\tV2\n> > +\tMARK_WRITE_ERASED (V2)\n> >  \tArguments:\n> >  \t\tV2:\n> >  \t\tArgs 0-1: Window offset to erase (blocks)\n> > @@ -617,6 +651,31 @@ Command:\n> >  \t\tnumber is the number of blocks of the active\n> > window to erase\n> >  \t\tstarting at offset. If the offset + number exceeds\n> > the size of\n> >  \t\tthe active window then the command must not\n> > succeed.\n> > +\n> > +Command:\n> > +\tGET_FLASH_NAME (V3)\n> > +\tArguments:\n> > +\t\tArgs 0: Flash ID\n> > +\tResponse:\n> > +\t\tArgs 0-10: Flash Name / UID\n> \n> Hopefully 11 bytes is enough. Well, 10 I guess - should we specify\n> that the\n> value at index 10 be '\\0'? I feel like not requiring that could lead\n> to\n> undesirable implementations.\n\nDepends on whether we require names be 11 bytes, be null terminated, or\njust contain trailing nulls for names shorter than 11 bytes.\n\nIf we're going to waste an argument on the null terminator we may as\nwell just make the first argument the length of the name.\n\nIs this something the protocol should care about. Maybe it's just up to\nthe daemon and host implementation to put something here that they both\nunderstand.\n\n> \n> I guess we don't want to go down the path of continued responses or\n> magic\n> windows to accommodate larger names?\n\nI'd prefer not :)\n\n> \n> > +\tNotes:\n> > +\t\tDescribes a flash with some kind of identifier\n> > useful to the\n> > +\t\thost system. This is typically a null-padded\n> > string.\n> > +\n> > +Command:\n> > +\tMARK_LOCKED (V3)\n> > +\tArguments:\n> > +\t\tArgs 0-1: Flash offset to lock (blocks)\n> > +\t\tArgs 2-3: Number to lock at offset (blocks)\n> \n> I guess it's hard to get away from using block-sized granuality. Do\n> we\n> currently have any partitions that we might lock that are less than a\n> block\n> size? Or are all partitions expected to be block-size aligned?\n\nThis was the motivation for allowing the host to request a block size,\nso that they can hopefully request something which allows locking at\nthe required granularity. Currently all partitions are aligned to flash\nerase size (which is what we set block size to anyway).\n\nIt's not really worth allowing locking at a finer granularity than\nblock size since this is how changes (mark dirty) are specified and so\nit would be impossible to write back any part of a block if a single\nbyte in that block were locked. I'm in favour of using block size for\nnow and changing it in a later version if this becomes an issue.\n\nThanks,\nSuraj\n\n> \n> Cheers,\n> \n> Andrew\n> \n> > +\t\tArgs 4: Flash ID\n> > +\tResponse:\n> > +\t\t-\n> > +\tNotes:\n> > +\t\tLock an area of flash so that the host can't mark\n> > it dirty or\n> > +\t\terased. If the requested area is within the\n> > current window and\n> > +\t\tthat area is currently marked dirty or erased then\n> > this command\n> > +\t\tmust fail.\n> > +\n> >  ```\n> >  \n> >  ### BMC Events in Detail:","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xrs1q042Pz9s83\n\tfor ;\n\tTue, 12 Sep 2017 14:16:47 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xrs1p51VlzDrJV\n\tfor ;\n\tTue, 12 Sep 2017 14:16:46 +1000 (AEST)","from mail-pg0-x230.google.com (mail-pg0-x230.google.com\n\t[IPv6:2607:f8b0:400e:c05::230])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xrs1W6SL3zDr5F;\n\tTue, 12 Sep 2017 14:16:31 +1000 (AEST)","by mail-pg0-x230.google.com with SMTP id d8so19220520pgt.4;\n\tMon, 11 Sep 2017 21:16:31 -0700 (PDT)","from surajjs1.ozlabs.ibm.com ([122.99.82.10])\n\tby smtp.googlemail.com with ESMTPSA id\n\tb76sm18889311pfd.69.2017.09.11.21.16.25\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 11 Sep 2017 21:16:28 -0700 (PDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"Qw0AA1ir\"; dkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"Qw0AA1ir\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=gmail.com\n\t(client-ip=2607:f8b0:400e:c05::230; helo=mail-pg0-x230.google.com;\n\tenvelope-from=sjitindarsingh@gmail.com; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"Qw0AA1ir\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=xvA1HNsuWJWGD4o4ZlcBIAvFaC5rbQn2/WCTEJxT0LI=;\n\tb=Qw0AA1irrG6BrmOI8g/rjI2fU/mGPf3yYtk2+kxk0awnlVdsd5NgIL1L0Dqszp7r4c\n\twItT84dIOfvUnerPbtet2F+jJ4B3pxrnr5ry0xsF7sSgBZ1+XZQlXGjkiRx6h2MRlQ6l\n\t5FWI/aAQMZjBg+iCxgHA/aPzF/0vO35Y8NB1wd/2XboGfOwUClegRqgIxbm8j5ysAClg\n\t4gqmDd85iG1AP9hZSjSIKk9G+biVXYlxdycqsIGhKZcrvrgx0VOdZvleMWdrAeGsvXJq\n\t6AXQAINOyHMZ2jn9mPhDiXuSvQ+hfYTK8RqIniD6Y0ueb3bleHy6+5ZJcYX0itVZ/+iz\n\tyVCw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=xvA1HNsuWJWGD4o4ZlcBIAvFaC5rbQn2/WCTEJxT0LI=;\n\tb=ANh+zht+gB6WsjFIPhnByXqKkzlOLI1bSvNezEjecawoU5zC1guWCP+LHzI3LxhQFJ\n\tSVY5HW+QGrGcis8xZZgNdX8GS9XgAwrjgDNawkdGE87jxE8tn3MHaXvKzxSRamsK0CSY\n\tX06enzlYuuEd4wxN8ommlV29wCAxpbreElBqv699gVnSDjjCAp0reyqGa/gtAHuFtefh\n\tC2EuXIwiKJi6iMSjbn+oltmgx9EzerAGChqDajU98BSDiNMr3vmaC924KWJCL+WCn1Br\n\tudLEKubwNC8ibqiwOlPTYQjj5BO+5UZpNwfznE8v8XrNN7r48W+jNIqQZJLss+Ras5MV\n\tUKew==","X-Gm-Message-State":"AHPjjUhZImHoBFQOsI9Fn5zmIbZnOd0mH2MvYVNnT1TZb6/PmaooKGvw\n\teXSST0eWv/zwAHaVEeY=","X-Google-Smtp-Source":"ADKCNb68Ol94PH/7MRwgkAQzLfmoTO9cahWKq86XoGd4nGMK90eo+ee8jX40LoB6EhnBr39ftZXruw==","X-Received":"by 10.84.210.12 with SMTP id z12mr15437861plh.433.1505189789126; \n\tMon, 11 Sep 2017 21:16:29 -0700 (PDT)","Message-ID":"<1505189782.2222.1.camel@gmail.com>","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","From":"Suraj Jitindar Singh ","To":"Andrew Jeffery , skiboot@lists.ozlabs.org, \n\topenbmc@lists.ozlabs.org","Date":"Tue, 12 Sep 2017 14:16:22 +1000","In-Reply-To":"<1505113305.4080.1.camel@aj.id.au>","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>\n\t<1505113305.4080.1.camel@aj.id.au>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.22.6 (3.22.6-2.fc25) ","Mime-Version":"1.0","Content-Transfer-Encoding":"8bit","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, cyrilbur@gmail.com, wak@google.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1768281,"web_url":"http://patchwork.ozlabs.org/comment/1768281/","msgid":"<1505354227.4080.11.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-14T01:57:07","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"> > > +The host may lock an area of flash using the MARK_LOCKED command.\n> > > Any attempt\n> > > +to mark dirty or erased this area of flash must fail with the\n> > > LOCKED_ERROR\n> > > +response code. The host may open a write window which contains a\n> > > locked area\n> > > +of flash however changes to a locked area of flash must never be\n> > > written back\n> > > +to the backing data source (i.e. that area of flash must be\n> > > treated as read\n> > > +only). An attempt to lock an area of flash which is not clean in\n> > > the current\n> > > +window must fail with PARAM_ERROR. (V3)\n> > \n> > I think we need to consider how locked regions interact with the in-\n> > memory\n> > caching of the flash data.\n> > \n> > The BMC doesn't know that the memory backing the LPC window has been\n> > written until it receives a MARK_DIRTY from the host. MARK_DIRTY is\n> > not\n> > valid on a read window or a locked region, but that doesn't mean that\n> > changes to a locked region can't persist in memory due to caching to\n> > avoid flash access (which was a part of the motivation for memory-\n> > based \n> > booting to begin with). This may naively fool host firmware into\n> > thinking it is\n> > accessing unmolested data as the region is locked, whereas the region\n> > could\n> > contain anything, just it will never be written to flash.\n> \n> Correct\n> \n> > \n> > Should we require window requests intersecting a locked region always\n> > have at\n> > least the locked region positively match the on-flash data? That way\n> > the\n> > firmware that requested a region be locked could ensure it has\n> > whatever was on\n> > the flash at the time it was locked by explicitly closing the window\n> > (read or\n> > write) once finished with it, which requires a window be opened again\n> > for any\n> > subsequent access. At that point (open) we can do the locked region\n> > intesection\n> > calculation and check the data in the window as necessary.\n> \n> I guess this brings about an interesting question, when a window is\n> requested is it guaranteed that the window provided in return by the\n> BMC contains the same contents as the flash for the requested area?\n\nI think expecting otherwise would be strange. What way would the host\nhave to validate that? It could keep its own hashes of window content,\nbut it would need to hash the content itself after opening the window\nwhich may be a circular problem (e.g. host reboot if the daemon doesn't\ninvalidate the cache across the reboot operation).\n\n> It\n> would seem intuitively that it should. The first time a window is\n> opened that is true, but with in memory caching the host could make a\n> change that it never tells the bmc about and the contents diverge, and\n> (assuming the window isn't evicted) persist across window close/open\n> calls.\n\nYep.\n\n> \n> Is this the desired behaviour? \n\nNo, not in my opinion. It doesn't fit the principle of least surprise.\n\n> It's kind of the only behaviour which\n> makes the in-memory caching an effective performance improvement, if we\n> reloaded every window every time then we may as well only have one\n> window in memory anyway. Maybe we can do this a better way (see below).\n> \n> It's worth noting that the current flash locking daemon implementation\n> will explicitly reload an entire window whenever one which contains any\n> locked regions is requested - ensuring (for at least as long as the\n> access to the flash device is exclusive) the integrity of the locked\n> regions. This could of course be optimised to only reload the locked\n> region.\n\nRight, and if we take up the techniques discussed below we may even be\nable to eliminate that.\n\n> \n> > \n> > Locked regions could thus be a performance hit if we always load the\n> > locked\n> > regions from flash, but surely we prefer (some) integrity over\n> > performance.\n> > \n> > Alternatively, you could cryptographically hash the per-block content\n> > of the\n> > on-flash locked region during the MARK_LOCKED operation, then stash\n> > the hash\n> > values away. On a CREATE_{READ,WRITE}_WINDOW operation you could hash\n> > any\n> > intersecting, in-cache locked blocks and compare to the stashed hash\n> > values to\n> > incur only one set of flash accesses (initial hash calculations)\n> > rather than n.\n> \n> This seems like a nice balance between performance and data integrity.\n> \n> I guess this brings about the question again as to whether we care\n> about data changes we aren't told about. Do we store a hash of every\n> window and before providing a cached copy verify the hash and reload\n> the entire window from flash again if there is a discrepancy. The host\n> isn't allowed to rely on the persistence of data it never tells the BMC\n> about, but is this something the BMC should be checking?\n\nI think so, again in support of the principle of least surprise, and\nthe possible security implications. I proposed verifying the integrity\nof only the locked regions but it might be worth extending that to\nentire windows depending on the performance. We should do some\nmeasurements.\n\nI think implementations also need to be noisy in the case of a\ndiscrepancy, logging on the BMC side and possibly issuing a BMC Event\n(taking another one of our reserved bits for the purpose). The\nmodifications are pretty much limited to accidental scribbling (bugs),\nor malicious intent (probably enabled by more bugs). Either way,\nsomeone should be notified.\n\n> > > +Command:\n> > > > > > +\tGET_FLASH_NAME (V3)\n> > > > > > +\tArguments:\n> > > > > > +\t\tArgs 0: Flash ID\n> > > > > > +\tResponse:\n> > > +\t\tArgs 0-10: Flash Name / UID\n> > \n> > Hopefully 11 bytes is enough. Well, 10 I guess - should we specify\n> > that the\n> > value at index 10 be '\\0'? I feel like not requiring that could lead\n> > to\n> > undesirable implementations.\n> \n> Depends on whether we require names be 11 bytes, be null terminated, or\n> just contain trailing nulls for names shorter than 11 bytes.\n> \n> If we're going to waste an argument on the null terminator we may as\n> well just make the first argument the length of the name.\n\nI think that's a better idea generally. It doesn't assume C-string\nsemantics, though it's likely that the implementations will require\nthem, which was what I was looking out for.\n\n> \n> Is this something the protocol should care about. Maybe it's just up to\n> the daemon and host implementation to put something here that they both\n> understand.\n\nProbably not, it was just a thought. It's a balance against avoiding\npotential vulnerabilities and over-specifying behaviour.\n\n> \n> > \n> > I guess we don't want to go down the path of continued responses or\n> > magic\n> > windows to accommodate larger names?\n> \n> I'd prefer not :)\n> \n\nSame :) Just throwing the question out there for discussion\n\n> > \n> > > > > > +\tNotes:\n> > > > > > +\t\tDescribes a flash with some kind of identifier\n> > > useful to the\n> > > > > > +\t\thost system. This is typically a null-padded\n> > > string.\n> > > +\n> > > +Command:\n> > > > > > +\tMARK_LOCKED (V3)\n> > > > > > +\tArguments:\n> > > > > > +\t\tArgs 0-1: Flash offset to lock (blocks)\n> > > +\t\tArgs 2-3: Number to lock at offset (blocks)\n> > \n> > I guess it's hard to get away from using block-sized granuality. Do\n> > we\n> > currently have any partitions that we might lock that are less than a\n> > block\n> > size? Or are all partitions expected to be block-size aligned?\n> \n> This was the motivation for allowing the host to request a block size,\n> so that they can hopefully request something which allows locking at\n> the required granularity. Currently all partitions are aligned to flash\n> erase size (which is what we set block size to anyway).\n\nOkay. It might be worth making a note in the documentation for\nMBOX_GET_INFO saying as much.\n\n> \n> It's not really worth allowing locking at a finer granularity than\n> block size since this is how changes (mark dirty) are specified and so\n> it would be impossible to write back any part of a block if a single\n> byte in that block were locked. I'm in favour of using block size for\n> now and changing it in a later version if this becomes an issue.\n\nRight, it's either blocks for everything or bytes for everything.\nConsistency is good. I don't think we want to go through the effort of\nchanging that aspect of the spec right now.\n\nCheers,\n\nAndrew","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xt1r82VKFz9s83\n\tfor ;\n\tThu, 14 Sep 2017 11:57:28 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xt1r818HCzDqsP\n\tfor ;\n\tThu, 14 Sep 2017 11:57:28 +1000 (AEST)","from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com\n\t[66.111.4.27])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xt1qz1FHczDqjS\n\tfor ; Thu, 14 Sep 2017 11:57:18 +1000 (AEST)","from compute3.internal (compute3.nyi.internal [10.202.2.43])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 90BB320BA3;\n\tWed, 13 Sep 2017 21:57:16 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute3.internal (MEProxy); Wed, 13 Sep 2017 21:57:16 -0400","from keelia16 (ppp14-2-0-125.bras21.adl4.internode.on.net\n\t[14.2.0.125])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 5D9DD24081;\n\tWed, 13 Sep 2017 21:57:13 -0400 (EDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"Qj3OQrdG\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"e6F9gNus\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"Qj3OQrdG\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"e6F9gNus\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.27; helo=out3-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"Qj3OQrdG\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"e6F9gNus\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=MRg588Ctumc40/f6RnAmp2g70G+oTbDLyFtzSdva/\n\tp0=; b=Qj3OQrdGP2hYBmvfD18/ouyQVO6lkloHWLDGrb7H7DG6uNGZ3gein+juS\n\t/FlB54RiZym5k7YPdaAufhSC9eZn7ryQ23mbT3ugHZ7puUkBpJQ3tn2fG56Z9CZh\n\tWnhdSNPiHXwYjHldN2Djg9A4ME2b+9Bc/y1KePq2WZTpkraItgWfEpHoVDMFmBCd\n\tLv182bfsJ1Y3pN6WlVb3+haW+wnt/bQX/szL6+MYmGRmMMjPwkTp7AgcTxuRY+zs\n\tz8kd2PkdCw2SoEJBFecP3x1wuDjTpY2G61dw5o7+boFjAu5TjNFNtAlzcuTuovZ0\n\tJ5DRiWqr1cI+XBgLz974n78kBIVmg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=MRg588Ctumc40/f6Rn\n\tAmp2g70G+oTbDLyFtzSdva/p0=; b=e6F9gNussVYxT0u7QQLf4aC0I9sYrwhxZn\n\tQnicFG2gDbLULIHxVU6sFxGm4xebZpbfA41z950DrwNbU+MDm6OP5zQxSF4uJXtp\n\tw3OaGgWKwBnAsUT9yr4jD15sbFUPonRsaI1Zu6lOa38g60fTyTCaUBY0dXvVyIC7\n\t752mH6l0J6Qs5El2VnCxefAsD5wKrjQAXaJKbrY9x06E9nxhR/BdUEcrw5OYQM1m\n\tCA94qxjvJddYNMN1/B4gD9ZgE4shrk/SIfuD7m0OCOjQNoVjq2QgUlKNc3dHi7EN\n\tdKD5ud4zDZP0z5Ok4vAz1npqThLZelUR+QUd+SSo5u89KXSyCzDQ=="],"X-ME-Sender":"","X-Sasl-enc":"eJOGbo2y31vHr8/9UuCLCDs0cgbwkLvG3767iv6Yy1F3 1505354235","Message-ID":"<1505354227.4080.11.camel@aj.id.au>","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","From":"Andrew Jeffery ","To":"Suraj Jitindar Singh ,\n\tskiboot@lists.ozlabs.org, openbmc@lists.ozlabs.org","Date":"Thu, 14 Sep 2017 11:27:07 +0930","In-Reply-To":"<1505189782.2222.1.camel@gmail.com>","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>\n\t<1505113305.4080.1.camel@aj.id.au>\n\t<1505189782.2222.1.camel@gmail.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-mjbNtiTUSg1npp71Qci2\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, cyrilbur@gmail.com, wak@google.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1770645,"web_url":"http://patchwork.ozlabs.org/comment/1770645/","msgid":"<1505796357.30609.1.camel@gmail.com>","list_archive_url":null,"date":"2017-09-19T04:45:57","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":68427,"url":"http://patchwork.ozlabs.org/api/people/68427/","name":"Suraj Jitindar Singh","email":"sjitindarsingh@gmail.com"},"content":"@wak:\n\nAs per Andrews suggestion below, how do you feel about making the first\narg in the response for get flash name the length?\n\n@andrew:\n\nSince your comments regarding flash integrity aren't really relevant at\nthe protocol level do you think it's something which needs to be\nmentioned in this document or is just common sense (unless you're me\nand didn't implement it that way :p)...\n\nOn Thu, 2017-09-14 at 11:27 +0930, Andrew Jeffery wrote:\n> > > > +The host may lock an area of flash using the MARK_LOCKED\n> > > > command.\n> > > > Any attempt\n> > > > +to mark dirty or erased this area of flash must fail with the\n> > > > LOCKED_ERROR\n> > > > +response code. The host may open a write window which contains\n> > > > a\n> > > > locked area\n> > > > +of flash however changes to a locked area of flash must never\n> > > > be\n> > > > written back\n> > > > +to the backing data source (i.e. that area of flash must be\n> > > > treated as read\n> > > > +only). An attempt to lock an area of flash which is not clean\n> > > > in\n> > > > the current\n> > > > +window must fail with PARAM_ERROR. (V3)\n> > > \n> > > I think we need to consider how locked regions interact with the\n> > > in-\n> > > memory\n> > > caching of the flash data.\n> > > \n> > > The BMC doesn't know that the memory backing the LPC window has\n> > > been\n> > > written until it receives a MARK_DIRTY from the host. MARK_DIRTY\n> > > is\n> > > not\n> > > valid on a read window or a locked region, but that doesn't mean\n> > > that\n> > > changes to a locked region can't persist in memory due to caching\n> > > to\n> > > avoid flash access (which was a part of the motivation for\n> > > memory-\n> > > based \n> > > booting to begin with). This may naively fool host firmware into\n> > > thinking it is\n> > > accessing unmolested data as the region is locked, whereas the\n> > > region\n> > > could\n> > > contain anything, just it will never be written to flash.\n> > \n> > Correct\n> > \n> > > \n> > > Should we require window requests intersecting a locked region\n> > > always\n> > > have at\n> > > least the locked region positively match the on-flash data? That\n> > > way\n> > > the\n> > > firmware that requested a region be locked could ensure it has\n> > > whatever was on\n> > > the flash at the time it was locked by explicitly closing the\n> > > window\n> > > (read or\n> > > write) once finished with it, which requires a window be opened\n> > > again\n> > > for any\n> > > subsequent access. At that point (open) we can do the locked\n> > > region\n> > > intesection\n> > > calculation and check the data in the window as necessary.\n> > \n> > I guess this brings about an interesting question, when a window is\n> > requested is it guaranteed that the window provided in return by\n> > the\n> > BMC contains the same contents as the flash for the requested area?\n> \n> I think expecting otherwise would be strange. What way would the host\n> have to validate that? It could keep its own hashes of window\n> content,\n> but it would need to hash the content itself after opening the window\n> which may be a circular problem (e.g. host reboot if the daemon\n> doesn't\n> invalidate the cache across the reboot operation).\n> \n> >  It\n> > would seem intuitively that it should. The first time a window is\n> > opened that is true, but with in memory caching the host could make\n> > a\n> > change that it never tells the bmc about and the contents diverge,\n> > and\n> > (assuming the window isn't evicted) persist across window\n> > close/open\n> > calls.\n> \n> Yep.\n> \n> > \n> > Is this the desired behaviour? \n> \n> No, not in my opinion. It doesn't fit the principle of least\n> surprise.\n> \n> > It's kind of the only behaviour which\n> > makes the in-memory caching an effective performance improvement,\n> > if we\n> > reloaded every window every time then we may as well only have one\n> > window in memory anyway. Maybe we can do this a better way (see\n> > below).\n> > \n> > It's worth noting that the current flash locking daemon\n> > implementation\n> > will explicitly reload an entire window whenever one which contains\n> > any\n> > locked regions is requested - ensuring (for at least as long as the\n> > access to the flash device is exclusive) the integrity of the\n> > locked\n> > regions. This could of course be optimised to only reload the\n> > locked\n> > region.\n> \n> Right, and if we take up the techniques discussed below we may even\n> be\n> able to eliminate that.\n> \n> > \n> > > \n> > > Locked regions could thus be a performance hit if we always load\n> > > the\n> > > locked\n> > > regions from flash, but surely we prefer (some) integrity over\n> > > performance.\n> > > \n> > > Alternatively, you could cryptographically hash the per-block\n> > > content\n> > > of the\n> > > on-flash locked region during the MARK_LOCKED operation, then\n> > > stash\n> > > the hash\n> > > values away. On a CREATE_{READ,WRITE}_WINDOW operation you could\n> > > hash\n> > > any\n> > > intersecting, in-cache locked blocks and compare to the stashed\n> > > hash\n> > > values to\n> > > incur only one set of flash accesses (initial hash calculations)\n> > > rather than n.\n> > \n> > This seems like a nice balance between performance and data\n> > integrity.\n> > \n> > I guess this brings about the question again as to whether we care\n> > about data changes we aren't told about. Do we store a hash of\n> > every\n> > window and before providing a cached copy verify the hash and\n> > reload\n> > the entire window from flash again if there is a discrepancy. The\n> > host\n> > isn't allowed to rely on the persistence of data it never tells the\n> > BMC\n> > about, but is this something the BMC should be checking?\n> \n> I think so, again in support of the principle of least surprise, and\n> the possible security implications. I proposed verifying the\n> integrity\n> of only the locked regions but it might be worth extending that to\n> entire windows depending on the performance. We should do some\n> measurements.\n> \n> I think implementations also need to be noisy in the case of a\n> discrepancy, logging on the BMC side and possibly issuing a BMC Event\n> (taking another one of our reserved bits for the purpose). The\n> modifications are pretty much limited to accidental scribbling\n> (bugs),\n> or malicious intent (probably enabled by more bugs). Either way,\n> someone should be notified.\n> \n> > > > +Command:\n> > > > > > > +\tGET_FLASH_NAME (V3)\n> > > > > > > +\tArguments:\n> > > > > > > +\t\tArgs 0: Flash ID\n> > > > > > > +\tResponse:\n> > > > \n> > > > +\t\tArgs 0-10: Flash Name / UID\n> > > \n> > > Hopefully 11 bytes is enough. Well, 10 I guess - should we\n> > > specify\n> > > that the\n> > > value at index 10 be '\\0'? I feel like not requiring that could\n> > > lead\n> > > to\n> > > undesirable implementations.\n> > \n> > Depends on whether we require names be 11 bytes, be null\n> > terminated, or\n> > just contain trailing nulls for names shorter than 11 bytes.\n> > \n> > If we're going to waste an argument on the null terminator we may\n> > as\n> > well just make the first argument the length of the name.\n> \n> I think that's a better idea generally. It doesn't assume C-string\n> semantics, though it's likely that the implementations will require\n> them, which was what I was looking out for.\n> \n> > \n> > Is this something the protocol should care about. Maybe it's just\n> > up to\n> > the daemon and host implementation to put something here that they\n> > both\n> > understand.\n> \n> Probably not, it was just a thought. It's a balance against avoiding\n> potential vulnerabilities and over-specifying behaviour.\n> \n> > \n> > > \n> > > I guess we don't want to go down the path of continued responses\n> > > or\n> > > magic\n> > > windows to accommodate larger names?\n> > \n> > I'd prefer not :)\n> > \n> \n> Same :) Just throwing the question out there for discussion\n> \n> > > \n> > > > > > > +\tNotes:\n> > > > > > > +\t\tDescribes a flash with some kind of\n> > > > > > > identifier\n> > > > \n> > > > useful to the\n> > > > > > > +\t\thost system. This is typically a null-\n> > > > > > > padded\n> > > > \n> > > > string.\n> > > > +\n> > > > +Command:\n> > > > > > > +\tMARK_LOCKED (V3)\n> > > > > > > +\tArguments:\n> > > > > > > +\t\tArgs 0-1: Flash offset to lock (blocks)\n> > > > \n> > > > +\t\tArgs 2-3: Number to lock at offset (blocks)\n> > > \n> > > I guess it's hard to get away from using block-sized granuality.\n> > > Do\n> > > we\n> > > currently have any partitions that we might lock that are less\n> > > than a\n> > > block\n> > > size? Or are all partitions expected to be block-size aligned?\n> > \n> > This was the motivation for allowing the host to request a block\n> > size,\n> > so that they can hopefully request something which allows locking\n> > at\n> > the required granularity. Currently all partitions are aligned to\n> > flash\n> > erase size (which is what we set block size to anyway).\n> \n> Okay. It might be worth making a note in the documentation for\n> MBOX_GET_INFO saying as much.\n> \n> > \n> > It's not really worth allowing locking at a finer granularity than\n> > block size since this is how changes (mark dirty) are specified and\n> > so\n> > it would be impossible to write back any part of a block if a\n> > single\n> > byte in that block were locked. I'm in favour of using block size\n> > for\n> > now and changing it in a later version if this becomes an issue.\n> \n> Right, it's either blocks for everything or bytes for everything.\n> Consistency is good. I don't think we want to go through the effort\n> of\n> changing that aspect of the spec right now.\n> \n> Cheers,\n> \n> Andrew","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xx9Ld2HfQz9s3w\n\tfor ;\n\tTue, 19 Sep 2017 14:46:17 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xx9Ld0wz1zDqJ0\n\tfor ;\n\tTue, 19 Sep 2017 14:46:17 +1000 (AEST)","from mail-it0-x22e.google.com (mail-it0-x22e.google.com\n\t[IPv6:2607:f8b0:4001:c0b::22e])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xx9LR2k59zDqFw;\n\tTue, 19 Sep 2017 14:46:06 +1000 (AEST)","by mail-it0-x22e.google.com with SMTP id w204so723725itc.4;\n\tMon, 18 Sep 2017 21:46:06 -0700 (PDT)","from surajjs1.ozlabs.ibm.com ([122.99.82.10])\n\tby smtp.googlemail.com with ESMTPSA id\n\tn63sm440044itb.16.2017.09.18.21.46.00\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 18 Sep 2017 21:46:03 -0700 (PDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"EqSF4f/C\"; dkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"EqSF4f/C\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=gmail.com\n\t(client-ip=2607:f8b0:4001:c0b::22e; helo=mail-it0-x22e.google.com;\n\tenvelope-from=sjitindarsingh@gmail.com; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"EqSF4f/C\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=17YyH9+sFP0BPIlL9Ap0bnp5hSps53NgWxCWQ1J6tm4=;\n\tb=EqSF4f/C2/mqvJmW4W40jCLgglp9YM+rAZ8IrIGDTkDo5hFr2bv2frlBQ1jG1WBJi1\n\tCdd1WteaSn5fzKWqDkH++uI5Nu1gTHlB10o/T3lBUavDP79ep3RngkngwjFJ/cIzihS6\n\tXbwIOr7MPSpCvtQgOTo9HUhnXOLauwv5Cj++V0gI0DB36IFqVFm7BychF62Mr3iJIIWh\n\t83YTEJfoB1KbYeTR2isDi8Df4FEtAwMV7Z1WATlcOeMMoMjhzVa6csKv3/gUyltSQIY8\n\ttbGwKo9w9ydA+TUAXsbtaPTsEgjEHP2OMlR1CS3vCvnXEJzvd1GcIjYK5AcIBvC7wwWJ\n\t3lqA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=17YyH9+sFP0BPIlL9Ap0bnp5hSps53NgWxCWQ1J6tm4=;\n\tb=GwymCNg43b62Ove/9n1VlUQO45QvDFMYPBO+iCysZSMe+8XvxB7GA/fcZglyMN/BQK\n\tbB5LrNLmwCHyvDs+PgFLE0kh7n2CFSv+5Qf6+CN34zCHrS4pohtwPFMNQEwFRPV1t6Eg\n\tFQFW93H5pPJAbbUKOkJOFFgdKntvE/peA0w5ZF9AfIjbJ/gs34qhjAL6CtioLCpcDRKI\n\t8foCpOop6z8MgKRX2Oc/yEW56gKhx13uFcxYsDQPfb4nmWwrDCU0J+FBjq1zvOT5StqS\n\tCJo80QFQgh3xXRbvNw00LxmJW42uWI3Jtdyno2fyS1TuY0xPqmSjiHq582mT4WVF+74A\n\t7Dsw==","X-Gm-Message-State":"AHPjjUg1jM65LlJlmAvkOdk2V0lNjYuMHgHtktwPYZIyheUt0KwNwY+h\n\tfAbIy/WmFD5TD6CIE9emCoY=","X-Google-Smtp-Source":"AOwi7QBeQRKS3yuIC7M4BOj14qHdz0c2SSDSQV2KARjq0TjYhezMSDpff2dAb1v9o9rote16BFPTBw==","X-Received":"by 10.36.219.132 with SMTP id c126mr709321itg.16.1505796364142; \n\tMon, 18 Sep 2017 21:46:04 -0700 (PDT)","Message-ID":"<1505796357.30609.1.camel@gmail.com>","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","From":"Suraj Jitindar Singh ","To":"Andrew Jeffery , skiboot@lists.ozlabs.org, \n\topenbmc@lists.ozlabs.org","Date":"Tue, 19 Sep 2017 14:45:57 +1000","In-Reply-To":"<1505354227.4080.11.camel@aj.id.au>","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>\n\t<1505113305.4080.1.camel@aj.id.au>\n\t<1505189782.2222.1.camel@gmail.com>\n\t<1505354227.4080.11.camel@aj.id.au>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.22.6 (3.22.6-2.fc25) ","Mime-Version":"1.0","Content-Transfer-Encoding":"8bit","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, cyrilbur@gmail.com, wak@google.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1770693,"web_url":"http://patchwork.ozlabs.org/comment/1770693/","msgid":"<1505805730.30138.1.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-19T07:22:10","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Tue, 2017-09-19 at 14:45 +1000, Suraj Jitindar Singh wrote:\n> @wak:\n> \n> As per Andrews suggestion below, how do you feel about making the first\n> arg in the response for get flash name the length?\n> \n> @andrew:\n> \n> Since your comments regarding flash integrity aren't really relevant at\n> the protocol level do you think it's something which needs to be\n> mentioned in this document or is just common sense (unless you're me\n> and didn't implement it that way :p)...\n> \n\nI'd say that the fact that you didn't implement it off the bat means\nthat others won't either, so I think it deserves some attention.\n\nIf we choose to do a host alert on the BMC detecting an error then that\nobviously needs to be specified here.\n\nBut winding back a bit, I'd suggest it's important to *specify* flash\nintegrity, even though the spec shouldn't enforce a particular\nimplementation. It should be enough to say something like:\n\n CREATE_{READ,WRITE}_WINDOW must provide a window containing data\n that is known to represent the state of the flash at the time the\n window is opened.\n\nMaybe we need to make provisions for implementation suggestions in the\nspec as well. I might do some reading around to see what other specs do\nin this regard.\n\nCheers,\n\nAndrew","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxDpp4G2Lz9sBZ\n\tfor ;\n\tTue, 19 Sep 2017 17:22:26 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xxDpp29RTzDqY8\n\tfor ;\n\tTue, 19 Sep 2017 17:22:26 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xxDpg01SBzDqNh;\n\tTue, 19 Sep 2017 17:22:18 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 65BD420DA5;\n\tTue, 19 Sep 2017 03:22:16 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Tue, 19 Sep 2017 03:22:16 -0400","from keelia16 (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 18A8C248D1;\n\tTue, 19 Sep 2017 03:22:13 -0400 (EDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"gob1FrGL\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"fU4Ib/IC\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"gob1FrGL\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"fU4Ib/IC\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"gob1FrGL\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"fU4Ib/IC\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=YDWfKuPEiq2IeGcb7RoxWmJ8IJYvLs1ZRcUvmRPcD\n\tZs=; b=gob1FrGLi8yN66HphzmIzRASbxIqZb6b0crl+pMqGC6Zj/Q85qyZQTM0S\n\t2m+paSD+ZEUgpYBsnqtsZb3DRmtL8Tq2cKs0t3oYCEpq1ONwf5PDQPQG4+/DAgda\n\t301AfJsR71kR3qq6OlNq8LGKsXdss/WKWJi99RuIlwHqkXvdPa7OdPPYzGP7FDBz\n\tY833F3QCtQ/geWKsvI0+4wS2EezDVJw6yxwr0NsgYI2ecADvkj84quJApfax4d1S\n\tWhoWNR6i7J5fsBvDUcauiR8+dcEDnNNYDQYzrM01YJNG6LwPa6uEvBkQvsyiYcFD\n\tdWHd3xSTVECTd1FSXaSKp4qrGKbzw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=YDWfKuPEiq2IeGcb7R\n\toxWmJ8IJYvLs1ZRcUvmRPcDZs=; b=fU4Ib/ICDS1+cOooCj7RzeJWOaSrITS1vJ\n\tvO63IDCJBnbX7WRkZaZ1Y0UHBJcMRip5MpJMqn6cQ+HK7MXoooBdjwvB/lSC7p7k\n\tZIIoxmx+yAe07uilmhLhZd1b4b5FdJ9yn/1r5xgRGZ4PYOHuVfXC5dWwWhI957Qn\n\tDXMh78AfjrGGGWstCnnSoTjgaHx/4A4o1Ey5DSVtpUxRXz+DUCFoqr6npSBgBEDy\n\tG6Q2T6gUk2qJx/1wXoieJbB358EEcATzD5JuAsA2R6fjxewUkiwVxVFtlWib7SI0\n\ttIkZJFs3j3BeIdp0lZbyZ/37m4zTRkw5xgZgtA4BSO5le3uyVvQw=="],"X-ME-Sender":"","X-Sasl-enc":"+k7eWC/UU5QPmceucFNY23mdExRHxiie9jJ4g3OBZY5u 1505805735","Message-ID":"<1505805730.30138.1.camel@aj.id.au>","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","From":"Andrew Jeffery ","To":"Suraj Jitindar Singh ,\n\tskiboot@lists.ozlabs.org, openbmc@lists.ozlabs.org","Date":"Tue, 19 Sep 2017 16:52:10 +0930","In-Reply-To":"<1505796357.30609.1.camel@gmail.com>","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>\n\t<1505113305.4080.1.camel@aj.id.au>\n\t<1505189782.2222.1.camel@gmail.com>\n\t<1505354227.4080.11.camel@aj.id.au>\n\t<1505796357.30609.1.camel@gmail.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-N/DVUc1cviKznLDSe2O1\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, cyrilbur@gmail.com, wak@google.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1778719,"web_url":"http://patchwork.ozlabs.org/comment/1778719/","msgid":"<1507004777.2329.1.camel@gmail.com>","list_archive_url":null,"date":"2017-10-03T04:26:17","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":68427,"url":"http://patchwork.ozlabs.org/api/people/68427/","name":"Suraj Jitindar Singh","email":"sjitindarsingh@gmail.com"},"content":"@wak ping,\n\nThoughts on using the first arg in the response of get flash name as a\nlength argument?\n\nOn Tue, 2017-09-19 at 14:45 +1000, Suraj Jitindar Singh wrote:\n> @wak:\n> \n> As per Andrews suggestion below, how do you feel about making the\n> first\n> arg in the response for get flash name the length?\n> \n> @andrew:\n> \n> Since your comments regarding flash integrity aren't really relevant\n> at\n> the protocol level do you think it's something which needs to be\n> mentioned in this document or is just common sense (unless you're me\n> and didn't implement it that way :p)...\n> \n> On Thu, 2017-09-14 at 11:27 +0930, Andrew Jeffery wrote:\n> > > > > +The host may lock an area of flash using the MARK_LOCKED\n> > > > > command.\n> > > > > Any attempt\n> > > > > +to mark dirty or erased this area of flash must fail with\n> > > > > the\n> > > > > LOCKED_ERROR\n> > > > > +response code. The host may open a write window which\n> > > > > contains\n> > > > > a\n> > > > > locked area\n> > > > > +of flash however changes to a locked area of flash must\n> > > > > never\n> > > > > be\n> > > > > written back\n> > > > > +to the backing data source (i.e. that area of flash must be\n> > > > > treated as read\n> > > > > +only). An attempt to lock an area of flash which is not\n> > > > > clean\n> > > > > in\n> > > > > the current\n> > > > > +window must fail with PARAM_ERROR. (V3)\n> > > > \n> > > > I think we need to consider how locked regions interact with\n> > > > the\n> > > > in-\n> > > > memory\n> > > > caching of the flash data.\n> > > > \n> > > > The BMC doesn't know that the memory backing the LPC window has\n> > > > been\n> > > > written until it receives a MARK_DIRTY from the host.\n> > > > MARK_DIRTY\n> > > > is\n> > > > not\n> > > > valid on a read window or a locked region, but that doesn't\n> > > > mean\n> > > > that\n> > > > changes to a locked region can't persist in memory due to\n> > > > caching\n> > > > to\n> > > > avoid flash access (which was a part of the motivation for\n> > > > memory-\n> > > > based \n> > > > booting to begin with). This may naively fool host firmware\n> > > > into\n> > > > thinking it is\n> > > > accessing unmolested data as the region is locked, whereas the\n> > > > region\n> > > > could\n> > > > contain anything, just it will never be written to flash.\n> > > \n> > > Correct\n> > > \n> > > > \n> > > > Should we require window requests intersecting a locked region\n> > > > always\n> > > > have at\n> > > > least the locked region positively match the on-flash data?\n> > > > That\n> > > > way\n> > > > the\n> > > > firmware that requested a region be locked could ensure it has\n> > > > whatever was on\n> > > > the flash at the time it was locked by explicitly closing the\n> > > > window\n> > > > (read or\n> > > > write) once finished with it, which requires a window be opened\n> > > > again\n> > > > for any\n> > > > subsequent access. At that point (open) we can do the locked\n> > > > region\n> > > > intesection\n> > > > calculation and check the data in the window as necessary.\n> > > \n> > > I guess this brings about an interesting question, when a window\n> > > is\n> > > requested is it guaranteed that the window provided in return by\n> > > the\n> > > BMC contains the same contents as the flash for the requested\n> > > area?\n> > \n> > I think expecting otherwise would be strange. What way would the\n> > host\n> > have to validate that? It could keep its own hashes of window\n> > content,\n> > but it would need to hash the content itself after opening the\n> > window\n> > which may be a circular problem (e.g. host reboot if the daemon\n> > doesn't\n> > invalidate the cache across the reboot operation).\n> > \n> > >  It\n> > > would seem intuitively that it should. The first time a window is\n> > > opened that is true, but with in memory caching the host could\n> > > make\n> > > a\n> > > change that it never tells the bmc about and the contents\n> > > diverge,\n> > > and\n> > > (assuming the window isn't evicted) persist across window\n> > > close/open\n> > > calls.\n> > \n> > Yep.\n> > \n> > > \n> > > Is this the desired behaviour? \n> > \n> > No, not in my opinion. It doesn't fit the principle of least\n> > surprise.\n> > \n> > > It's kind of the only behaviour which\n> > > makes the in-memory caching an effective performance improvement,\n> > > if we\n> > > reloaded every window every time then we may as well only have\n> > > one\n> > > window in memory anyway. Maybe we can do this a better way (see\n> > > below).\n> > > \n> > > It's worth noting that the current flash locking daemon\n> > > implementation\n> > > will explicitly reload an entire window whenever one which\n> > > contains\n> > > any\n> > > locked regions is requested - ensuring (for at least as long as\n> > > the\n> > > access to the flash device is exclusive) the integrity of the\n> > > locked\n> > > regions. This could of course be optimised to only reload the\n> > > locked\n> > > region.\n> > \n> > Right, and if we take up the techniques discussed below we may even\n> > be\n> > able to eliminate that.\n> > \n> > > \n> > > > \n> > > > Locked regions could thus be a performance hit if we always\n> > > > load\n> > > > the\n> > > > locked\n> > > > regions from flash, but surely we prefer (some) integrity over\n> > > > performance.\n> > > > \n> > > > Alternatively, you could cryptographically hash the per-block\n> > > > content\n> > > > of the\n> > > > on-flash locked region during the MARK_LOCKED operation, then\n> > > > stash\n> > > > the hash\n> > > > values away. On a CREATE_{READ,WRITE}_WINDOW operation you\n> > > > could\n> > > > hash\n> > > > any\n> > > > intersecting, in-cache locked blocks and compare to the stashed\n> > > > hash\n> > > > values to\n> > > > incur only one set of flash accesses (initial hash\n> > > > calculations)\n> > > > rather than n.\n> > > \n> > > This seems like a nice balance between performance and data\n> > > integrity.\n> > > \n> > > I guess this brings about the question again as to whether we\n> > > care\n> > > about data changes we aren't told about. Do we store a hash of\n> > > every\n> > > window and before providing a cached copy verify the hash and\n> > > reload\n> > > the entire window from flash again if there is a discrepancy. The\n> > > host\n> > > isn't allowed to rely on the persistence of data it never tells\n> > > the\n> > > BMC\n> > > about, but is this something the BMC should be checking?\n> > \n> > I think so, again in support of the principle of least surprise,\n> > and\n> > the possible security implications. I proposed verifying the\n> > integrity\n> > of only the locked regions but it might be worth extending that to\n> > entire windows depending on the performance. We should do some\n> > measurements.\n> > \n> > I think implementations also need to be noisy in the case of a\n> > discrepancy, logging on the BMC side and possibly issuing a BMC\n> > Event\n> > (taking another one of our reserved bits for the purpose). The\n> > modifications are pretty much limited to accidental scribbling\n> > (bugs),\n> > or malicious intent (probably enabled by more bugs). Either way,\n> > someone should be notified.\n> > \n> > > > > +Command:\n> > > > > > > > +\tGET_FLASH_NAME (V3)\n> > > > > > > > +\tArguments:\n> > > > > > > > +\t\tArgs 0: Flash ID\n> > > > > > > > +\tResponse:\n> > > > > \n> > > > > +\t\tArgs 0-10: Flash Name / UID\n> > > > \n> > > > Hopefully 11 bytes is enough. Well, 10 I guess - should we\n> > > > specify\n> > > > that the\n> > > > value at index 10 be '\\0'? I feel like not requiring that could\n> > > > lead\n> > > > to\n> > > > undesirable implementations.\n> > > \n> > > Depends on whether we require names be 11 bytes, be null\n> > > terminated, or\n> > > just contain trailing nulls for names shorter than 11 bytes.\n> > > \n> > > If we're going to waste an argument on the null terminator we may\n> > > as\n> > > well just make the first argument the length of the name.\n> > \n> > I think that's a better idea generally. It doesn't assume C-string\n> > semantics, though it's likely that the implementations will require\n> > them, which was what I was looking out for.\n> > \n> > > \n> > > Is this something the protocol should care about. Maybe it's just\n> > > up to\n> > > the daemon and host implementation to put something here that\n> > > they\n> > > both\n> > > understand.\n> > \n> > Probably not, it was just a thought. It's a balance against\n> > avoiding\n> > potential vulnerabilities and over-specifying behaviour.\n> > \n> > > \n> > > > \n> > > > I guess we don't want to go down the path of continued\n> > > > responses\n> > > > or\n> > > > magic\n> > > > windows to accommodate larger names?\n> > > \n> > > I'd prefer not :)\n> > > \n> > \n> > Same :) Just throwing the question out there for discussion\n> > \n> > > > \n> > > > > > > > +\tNotes:\n> > > > > > > > +\t\tDescribes a flash with some kind of\n> > > > > > > > identifier\n> > > > > \n> > > > > useful to the\n> > > > > > > > +\t\thost system. This is typically a null-\n> > > > > > > > padded\n> > > > > \n> > > > > string.\n> > > > > +\n> > > > > +Command:\n> > > > > > > > +\tMARK_LOCKED (V3)\n> > > > > > > > +\tArguments:\n> > > > > > > > +\t\tArgs 0-1: Flash offset to lock\n> > > > > > > > (blocks)\n> > > > > \n> > > > > +\t\tArgs 2-3: Number to lock at offset (blocks)\n> > > > \n> > > > I guess it's hard to get away from using block-sized\n> > > > granuality.\n> > > > Do\n> > > > we\n> > > > currently have any partitions that we might lock that are less\n> > > > than a\n> > > > block\n> > > > size? Or are all partitions expected to be block-size aligned?\n> > > \n> > > This was the motivation for allowing the host to request a block\n> > > size,\n> > > so that they can hopefully request something which allows locking\n> > > at\n> > > the required granularity. Currently all partitions are aligned to\n> > > flash\n> > > erase size (which is what we set block size to anyway).\n> > \n> > Okay. It might be worth making a note in the documentation for\n> > MBOX_GET_INFO saying as much.\n> > \n> > > \n> > > It's not really worth allowing locking at a finer granularity\n> > > than\n> > > block size since this is how changes (mark dirty) are specified\n> > > and\n> > > so\n> > > it would be impossible to write back any part of a block if a\n> > > single\n> > > byte in that block were locked. I'm in favour of using block size\n> > > for\n> > > now and changing it in a later version if this becomes an issue.\n> > \n> > Right, it's either blocks for everything or bytes for everything.\n> > Consistency is good. I don't think we want to go through the effort\n> > of\n> > changing that aspect of the spec right now.\n> > \n> > Cheers,\n> > \n> > Andrew","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y5mFQ3x1lz9t44\n\tfor ;\n\tTue, 3 Oct 2017 15:26:34 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y5mFQ2jxNzDql4\n\tfor ;\n\tTue, 3 Oct 2017 15:26:34 +1100 (AEDT)","from mail-pf0-x235.google.com (mail-pf0-x235.google.com\n\t[IPv6:2607:f8b0:400e:c00::235])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3y5mFG460tzDqhh;\n\tTue, 3 Oct 2017 15:26:25 +1100 (AEDT)","by mail-pf0-x235.google.com with SMTP id n14so2084691pfh.8;\n\tMon, 02 Oct 2017 21:26:25 -0700 (PDT)","from surajjs1.ozlabs.ibm.com ([122.99.82.10])\n\tby smtp.googlemail.com with ESMTPSA id\n\t77sm19147809pfi.103.2017.10.02.21.26.20\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 02 Oct 2017 21:26:22 -0700 (PDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"SgenY23J\"; dkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"SgenY23J\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=gmail.com\n\t(client-ip=2607:f8b0:400e:c00::235; helo=mail-pf0-x235.google.com;\n\tenvelope-from=sjitindarsingh@gmail.com; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"SgenY23J\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=Rjh4SP+5kopKaTq3Ed/agKWYuoZJiBrTEvIA9OL0JWk=;\n\tb=SgenY23JOt9lAdPkzycmLROMFBB35vnTh9O7501F1uG5Pslfq2OcpiZ5bnYJ+5oTs4\n\tnYsVwdwjjcBp+1cV01iJ/1Wxnyk25gqT3br0G3+eI8pupbpyeCnUN8ar4F3X91jp9QDm\n\tWPMgyNdyExYTdEd3XTCvwbTiFAxyrQFFtun/8mIdko2ZgM7neVr6iRRmVHck+8k52BCC\n\tmBNaD+7fTuwfeKkN5siscsIqnx+KYRb8mhl2BFWX76DwGmbU0m8LZ9UrkPMmuiG9Vx0r\n\t17ORIwQbTLi41fKeIFTExjWShDmvu4UYMPpWeH/gkl27cau/ndwmmpXMy/vbiFF4Nh6b\n\tujqg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=Rjh4SP+5kopKaTq3Ed/agKWYuoZJiBrTEvIA9OL0JWk=;\n\tb=FwQGWE3KFUKthxJ8oCxQ/v2KPTreqc0P40lrkgRHcVkmLq6UpM3O4Y1ZXb6zwYor7K\n\tK86ApKbWjGe+8THqxCYOVtLXAS6wvROruQHkW9ac+lugHwWU7pDsbmGOA2XgW6GxgB5e\n\t0OmMYSoz3j+bAA1+QGKILAs4Tgc0crjvo5W5tFjieICv2EE9320tVIcs4igiu5RDmmF+\n\t8WWg2zqj+d2CL+XZFekcYK8PdM+8FgpSZ6V3kGpavcwfLs3NA4BwlHstmQqejceTf1Ho\n\tOmyE1nGWqoUcbY9qAUswVB6RbZYKKvU50Gxy/7XI/INbFtirEFfp2WF8UzJRFPqM899n\n\tvoaQ==","X-Gm-Message-State":"AHPjjUhdQQjZyivksl0uR2APKWWlTas3+qUh0VO2PxgIk9bRZkdipF2x\n\tkfVgV7Ldg5wCyK3OqSchMfI=","X-Google-Smtp-Source":"AOwi7QC8ieVRKOc4PXDNVL1PDduyI7xHFC57R9aB4Uo1QWrKRYScW/ODyc3K34pOIRPdBE5EQZNJ4A==","X-Received":"by 10.84.143.195 with SMTP id 61mr15879891plz.251.1507004783617; \n\tMon, 02 Oct 2017 21:26:23 -0700 (PDT)","Message-ID":"<1507004777.2329.1.camel@gmail.com>","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","From":"Suraj Jitindar Singh ","To":"Andrew Jeffery , skiboot@lists.ozlabs.org, \n\topenbmc@lists.ozlabs.org","Date":"Tue, 03 Oct 2017 15:26:17 +1100","In-Reply-To":"<1505796357.30609.1.camel@gmail.com>","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>\n\t<1505113305.4080.1.camel@aj.id.au>\n\t<1505189782.2222.1.camel@gmail.com>\n\t<1505354227.4080.11.camel@aj.id.au>\n\t<1505796357.30609.1.camel@gmail.com>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.22.6 (3.22.6-2.fc25) ","Mime-Version":"1.0","Content-Transfer-Encoding":"8bit","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, cyrilbur@gmail.com, wak@google.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1779336,"web_url":"http://patchwork.ozlabs.org/comment/1779336/","msgid":"","list_archive_url":null,"date":"2017-10-03T17:39:19","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":70409,"url":"http://patchwork.ozlabs.org/api/people/70409/","name":"William Kennington","email":"wak@google.com"},"content":"Yeah, go ahead and change it to that\n\nOn Mon, Oct 2, 2017 at 9:26 PM Suraj Jitindar Singh <\nsjitindarsingh@gmail.com> wrote:\n\n> @wak ping,\n>\n> Thoughts on using the first arg in the response of get flash name as a\n> length argument?\n>\n> On Tue, 2017-09-19 at 14:45 +1000, Suraj Jitindar Singh wrote:\n> > @wak:\n> >\n> > As per Andrews suggestion below, how do you feel about making the\n> > first\n> > arg in the response for get flash name the length?\n> >\n> > @andrew:\n> >\n> > Since your comments regarding flash integrity aren't really relevant\n> > at\n> > the protocol level do you think it's something which needs to be\n> > mentioned in this document or is just common sense (unless you're me\n> > and didn't implement it that way :p)...\n> >\n> > On Thu, 2017-09-14 at 11:27 +0930, Andrew Jeffery wrote:\n> > > > > > +The host may lock an area of flash using the MARK_LOCKED\n> > > > > > command.\n> > > > > > Any attempt\n> > > > > > +to mark dirty or erased this area of flash must fail with\n> > > > > > the\n> > > > > > LOCKED_ERROR\n> > > > > > +response code. The host may open a write window which\n> > > > > > contains\n> > > > > > a\n> > > > > > locked area\n> > > > > > +of flash however changes to a locked area of flash must\n> > > > > > never\n> > > > > > be\n> > > > > > written back\n> > > > > > +to the backing data source (i.e. that area of flash must be\n> > > > > > treated as read\n> > > > > > +only). An attempt to lock an area of flash which is not\n> > > > > > clean\n> > > > > > in\n> > > > > > the current\n> > > > > > +window must fail with PARAM_ERROR. (V3)\n> > > > >\n> > > > > I think we need to consider how locked regions interact with\n> > > > > the\n> > > > > in-\n> > > > > memory\n> > > > > caching of the flash data.\n> > > > >\n> > > > > The BMC doesn't know that the memory backing the LPC window has\n> > > > > been\n> > > > > written until it receives a MARK_DIRTY from the host.\n> > > > > MARK_DIRTY\n> > > > > is\n> > > > > not\n> > > > > valid on a read window or a locked region, but that doesn't\n> > > > > mean\n> > > > > that\n> > > > > changes to a locked region can't persist in memory due to\n> > > > > caching\n> > > > > to\n> > > > > avoid flash access (which was a part of the motivation for\n> > > > > memory-\n> > > > > based\n> > > > > booting to begin with). This may naively fool host firmware\n> > > > > into\n> > > > > thinking it is\n> > > > > accessing unmolested data as the region is locked, whereas the\n> > > > > region\n> > > > > could\n> > > > > contain anything, just it will never be written to flash.\n> > > >\n> > > > Correct\n> > > >\n> > > > >\n> > > > > Should we require window requests intersecting a locked region\n> > > > > always\n> > > > > have at\n> > > > > least the locked region positively match the on-flash data?\n> > > > > That\n> > > > > way\n> > > > > the\n> > > > > firmware that requested a region be locked could ensure it has\n> > > > > whatever was on\n> > > > > the flash at the time it was locked by explicitly closing the\n> > > > > window\n> > > > > (read or\n> > > > > write) once finished with it, which requires a window be opened\n> > > > > again\n> > > > > for any\n> > > > > subsequent access. At that point (open) we can do the locked\n> > > > > region\n> > > > > intesection\n> > > > > calculation and check the data in the window as necessary.\n> > > >\n> > > > I guess this brings about an interesting question, when a window\n> > > > is\n> > > > requested is it guaranteed that the window provided in return by\n> > > > the\n> > > > BMC contains the same contents as the flash for the requested\n> > > > area?\n> > >\n> > > I think expecting otherwise would be strange. What way would the\n> > > host\n> > > have to validate that? It could keep its own hashes of window\n> > > content,\n> > > but it would need to hash the content itself after opening the\n> > > window\n> > > which may be a circular problem (e.g. host reboot if the daemon\n> > > doesn't\n> > > invalidate the cache across the reboot operation).\n> > >\n> > > > It\n> > > > would seem intuitively that it should. The first time a window is\n> > > > opened that is true, but with in memory caching the host could\n> > > > make\n> > > > a\n> > > > change that it never tells the bmc about and the contents\n> > > > diverge,\n> > > > and\n> > > > (assuming the window isn't evicted) persist across window\n> > > > close/open\n> > > > calls.\n> > >\n> > > Yep.\n> > >\n> > > >\n> > > > Is this the desired behaviour?\n> > >\n> > > No, not in my opinion. It doesn't fit the principle of least\n> > > surprise.\n> > >\n> > > > It's kind of the only behaviour which\n> > > > makes the in-memory caching an effective performance improvement,\n> > > > if we\n> > > > reloaded every window every time then we may as well only have\n> > > > one\n> > > > window in memory anyway. Maybe we can do this a better way (see\n> > > > below).\n> > > >\n> > > > It's worth noting that the current flash locking daemon\n> > > > implementation\n> > > > will explicitly reload an entire window whenever one which\n> > > > contains\n> > > > any\n> > > > locked regions is requested - ensuring (for at least as long as\n> > > > the\n> > > > access to the flash device is exclusive) the integrity of the\n> > > > locked\n> > > > regions. This could of course be optimised to only reload the\n> > > > locked\n> > > > region.\n> > >\n> > > Right, and if we take up the techniques discussed below we may even\n> > > be\n> > > able to eliminate that.\n> > >\n> > > >\n> > > > >\n> > > > > Locked regions could thus be a performance hit if we always\n> > > > > load\n> > > > > the\n> > > > > locked\n> > > > > regions from flash, but surely we prefer (some) integrity over\n> > > > > performance.\n> > > > >\n> > > > > Alternatively, you could cryptographically hash the per-block\n> > > > > content\n> > > > > of the\n> > > > > on-flash locked region during the MARK_LOCKED operation, then\n> > > > > stash\n> > > > > the hash\n> > > > > values away. On a CREATE_{READ,WRITE}_WINDOW operation you\n> > > > > could\n> > > > > hash\n> > > > > any\n> > > > > intersecting, in-cache locked blocks and compare to the stashed\n> > > > > hash\n> > > > > values to\n> > > > > incur only one set of flash accesses (initial hash\n> > > > > calculations)\n> > > > > rather than n.\n> > > >\n> > > > This seems like a nice balance between performance and data\n> > > > integrity.\n> > > >\n> > > > I guess this brings about the question again as to whether we\n> > > > care\n> > > > about data changes we aren't told about. Do we store a hash of\n> > > > every\n> > > > window and before providing a cached copy verify the hash and\n> > > > reload\n> > > > the entire window from flash again if there is a discrepancy. The\n> > > > host\n> > > > isn't allowed to rely on the persistence of data it never tells\n> > > > the\n> > > > BMC\n> > > > about, but is this something the BMC should be checking?\n> > >\n> > > I think so, again in support of the principle of least surprise,\n> > > and\n> > > the possible security implications. I proposed verifying the\n> > > integrity\n> > > of only the locked regions but it might be worth extending that to\n> > > entire windows depending on the performance. We should do some\n> > > measurements.\n> > >\n> > > I think implementations also need to be noisy in the case of a\n> > > discrepancy, logging on the BMC side and possibly issuing a BMC\n> > > Event\n> > > (taking another one of our reserved bits for the purpose). The\n> > > modifications are pretty much limited to accidental scribbling\n> > > (bugs),\n> > > or malicious intent (probably enabled by more bugs). Either way,\n> > > someone should be notified.\n> > >\n> > > > > > +Command:\n> > > > > > > > > + GET_FLASH_NAME (V3)\n> > > > > > > > > + Arguments:\n> > > > > > > > > + Args 0: Flash ID\n> > > > > > > > > + Response:\n> > > > > >\n> > > > > > + Args 0-10: Flash Name / UID\n> > > > >\n> > > > > Hopefully 11 bytes is enough. Well, 10 I guess - should we\n> > > > > specify\n> > > > > that the\n> > > > > value at index 10 be '\\0'? I feel like not requiring that could\n> > > > > lead\n> > > > > to\n> > > > > undesirable implementations.\n> > > >\n> > > > Depends on whether we require names be 11 bytes, be null\n> > > > terminated, or\n> > > > just contain trailing nulls for names shorter than 11 bytes.\n> > > >\n> > > > If we're going to waste an argument on the null terminator we may\n> > > > as\n> > > > well just make the first argument the length of the name.\n> > >\n> > > I think that's a better idea generally. It doesn't assume C-string\n> > > semantics, though it's likely that the implementations will require\n> > > them, which was what I was looking out for.\n> > >\n> > > >\n> > > > Is this something the protocol should care about. Maybe it's just\n> > > > up to\n> > > > the daemon and host implementation to put something here that\n> > > > they\n> > > > both\n> > > > understand.\n> > >\n> > > Probably not, it was just a thought. It's a balance against\n> > > avoiding\n> > > potential vulnerabilities and over-specifying behaviour.\n> > >\n> > > >\n> > > > >\n> > > > > I guess we don't want to go down the path of continued\n> > > > > responses\n> > > > > or\n> > > > > magic\n> > > > > windows to accommodate larger names?\n> > > >\n> > > > I'd prefer not :)\n> > > >\n> > >\n> > > Same :) Just throwing the question out there for discussion\n> > >\n> > > > >\n> > > > > > > > > + Notes:\n> > > > > > > > > + Describes a flash with some kind of\n> > > > > > > > > identifier\n> > > > > >\n> > > > > > useful to the\n> > > > > > > > > + host system. This is typically a null-\n> > > > > > > > > padded\n> > > > > >\n> > > > > > string.\n> > > > > > +\n> > > > > > +Command:\n> > > > > > > > > + MARK_LOCKED (V3)\n> > > > > > > > > + Arguments:\n> > > > > > > > > + Args 0-1: Flash offset to lock\n> > > > > > > > > (blocks)\n> > > > > >\n> > > > > > + Args 2-3: Number to lock at offset (blocks)\n> > > > >\n> > > > > I guess it's hard to get away from using block-sized\n> > > > > granuality.\n> > > > > Do\n> > > > > we\n> > > > > currently have any partitions that we might lock that are less\n> > > > > than a\n> > > > > block\n> > > > > size? Or are all partitions expected to be block-size aligned?\n> > > >\n> > > > This was the motivation for allowing the host to request a block\n> > > > size,\n> > > > so that they can hopefully request something which allows locking\n> > > > at\n> > > > the required granularity. Currently all partitions are aligned to\n> > > > flash\n> > > > erase size (which is what we set block size to anyway).\n> > >\n> > > Okay. It might be worth making a note in the documentation for\n> > > MBOX_GET_INFO saying as much.\n> > >\n> > > >\n> > > > It's not really worth allowing locking at a finer granularity\n> > > > than\n> > > > block size since this is how changes (mark dirty) are specified\n> > > > and\n> > > > so\n> > > > it would be impossible to write back any part of a block if a\n> > > > single\n> > > > byte in that block were locked. I'm in favour of using block size\n> > > > for\n> > > > now and changing it in a later version if this becomes an issue.\n> > >\n> > > Right, it's either blocks for everything or bytes for everything.\n> > > Consistency is good. I don't think we want to go through the effort\n> > > of\n> > > changing that aspect of the spec right now.\n> > >\n> > > Cheers,\n> > >\n> > > Andrew\n>\n
Yeah, go ahead and change it to that

On Mon, Oct 2, 2017 at 9:26 PM Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:
@wak ping,
\n
\nThoughts on using the first arg in the response of get flash name as a
\nlength argument?
\n
\nOn Tue, 2017-09-19 at 14:45 +1000, Suraj Jitindar Singh wrote:
\n> @wak:
\n>
\n> As per Andrews suggestion below, how do you feel about making the
\n> first
\n> arg in the response for get flash name the length?
\n>
\n> @andrew:
\n>
\n> Since your comments regarding flash integrity aren't really relevant
\n> at
\n> the protocol level do you think it's something which needs to be
\n> mentioned in this document or is just common sense (unless you're me
\n> and didn't implement it that way :p)...
\n>
\n> On Thu, 2017-09-14 at 11:27 +0930, Andrew Jeffery wrote:
\n> > > > > +The host may lock an area of flash using the MARK_LOCKED
\n> > > > > command.
\n> > > > > Any attempt
\n> > > > > +to mark dirty or erased this area of flash must fail with
\n> > > > > the
\n> > > > > LOCKED_ERROR
\n> > > > > +response code. The host may open a write window which
\n> > > > > contains
\n> > > > > a
\n> > > > > locked area
\n> > > > > +of flash however changes to a locked area of flash must
\n> > > > > never
\n> > > > > be
\n> > > > > written back
\n> > > > > +to the backing data source (i.e. that area of flash must be
\n> > > > > treated as read
\n> > > > > +only). An attempt to lock an area of flash which is not
\n> > > > > clean
\n> > > > > in
\n> > > > > the current
\n> > > > > +window must fail with PARAM_ERROR. (V3)
\n> > > >
\n> > > > I think we need to consider how locked regions interact with
\n> > > > the
\n> > > > in-
\n> > > > memory
\n> > > > caching of the flash data.
\n> > > >
\n> > > > The BMC doesn't know that the memory backing the LPC window has
\n> > > > been
\n> > > > written until it receives a MARK_DIRTY from the host.
\n> > > > MARK_DIRTY
\n> > > > is
\n> > > > not
\n> > > > valid on a read window or a locked region, but that doesn't
\n> > > > mean
\n> > > > that
\n> > > > changes to a locked region can't persist in memory due to
\n> > > > caching
\n> > > > to
\n> > > > avoid flash access (which was a part of the motivation for
\n> > > > memory-
\n> > > > based 
\n> > > > booting to begin with). This may naively fool host firmware
\n> > > > into
\n> > > > thinking it is
\n> > > > accessing unmolested data as the region is locked, whereas the
\n> > > > region
\n> > > > could
\n> > > > contain anything, just it will never be written to flash.
\n> > >
\n> > > Correct
\n> > >
\n> > > >
\n> > > > Should we require window requests intersecting a locked region
\n> > > > always
\n> > > > have at
\n> > > > least the locked region positively match the on-flash data?
\n> > > > That
\n> > > > way
\n> > > > the
\n> > > > firmware that requested a region be locked could ensure it has
\n> > > > whatever was on
\n> > > > the flash at the time it was locked by explicitly closing the
\n> > > > window
\n> > > > (read or
\n> > > > write) once finished with it, which requires a window be opened
\n> > > > again
\n> > > > for any
\n> > > > subsequent access. At that point (open) we can do the locked
\n> > > > region
\n> > > > intesection
\n> > > > calculation and check the data in the window as necessary.
\n> > >
\n> > > I guess this brings about an interesting question, when a window
\n> > > is
\n> > > requested is it guaranteed that the window provided in return by
\n> > > the
\n> > > BMC contains the same contents as the flash for the requested
\n> > > area?
\n> >
\n> > I think expecting otherwise would be strange. What way would the
\n> > host
\n> > have to validate that? It could keep its own hashes of window
\n> > content,
\n> > but it would need to hash the content itself after opening the
\n> > window
\n> > which may be a circular problem (e.g. host reboot if the daemon
\n> > doesn't
\n> > invalidate the cache across the reboot operation).
\n> >
\n> > >  It
\n> > > would seem intuitively that it should. The first time a window is
\n> > > opened that is true, but with in memory caching the host could
\n> > > make
\n> > > a
\n> > > change that it never tells the bmc about and the contents
\n> > > diverge,
\n> > > and
\n> > > (assuming the window isn't evicted) persist across window
\n> > > close/open
\n> > > calls.
\n> >
\n> > Yep.
\n> >
\n> > >
\n> > > Is this the desired behaviour? 
\n> >
\n> > No, not in my opinion. It doesn't fit the principle of least
\n> > surprise.
\n> >
\n> > > It's kind of the only behaviour which
\n> > > makes the in-memory caching an effective performance improvement,
\n> > > if we
\n> > > reloaded every window every time then we may as well only have
\n> > > one
\n> > > window in memory anyway. Maybe we can do this a better way (see
\n> > > below).
\n> > >
\n> > > It's worth noting that the current flash locking daemon
\n> > > implementation
\n> > > will explicitly reload an entire window whenever one which
\n> > > contains
\n> > > any
\n> > > locked regions is requested - ensuring (for at least as long as
\n> > > the
\n> > > access to the flash device is exclusive) the integrity of the
\n> > > locked
\n> > > regions. This could of course be optimised to only reload the
\n> > > locked
\n> > > region.
\n> >
\n> > Right, and if we take up the techniques discussed below we may even
\n> > be
\n> > able to eliminate that.
\n> >
\n> > >
\n> > > >
\n> > > > Locked regions could thus be a performance hit if we always
\n> > > > load
\n> > > > the
\n> > > > locked
\n> > > > regions from flash, but surely we prefer (some) integrity over
\n> > > > performance.
\n> > > >
\n> > > > Alternatively, you could cryptographically hash the per-block
\n> > > > content
\n> > > > of the
\n> > > > on-flash locked region during the MARK_LOCKED operation, then
\n> > > > stash
\n> > > > the hash
\n> > > > values away. On a CREATE_{READ,WRITE}_WINDOW operation you
\n> > > > could
\n> > > > hash
\n> > > > any
\n> > > > intersecting, in-cache locked blocks and compare to the stashed
\n> > > > hash
\n> > > > values to
\n> > > > incur only one set of flash accesses (initial hash
\n> > > > calculations)
\n> > > > rather than n.
\n> > >
\n> > > This seems like a nice balance between performance and data
\n> > > integrity.
\n> > >
\n> > > I guess this brings about the question again as to whether we
\n> > > care
\n> > > about data changes we aren't told about. Do we store a hash of
\n> > > every
\n> > > window and before providing a cached copy verify the hash and
\n> > > reload
\n> > > the entire window from flash again if there is a discrepancy. The
\n> > > host
\n> > > isn't allowed to rely on the persistence of data it never tells
\n> > > the
\n> > > BMC
\n> > > about, but is this something the BMC should be checking?
\n> >
\n> > I think so, again in support of the principle of least surprise,
\n> > and
\n> > the possible security implications. I proposed verifying the
\n> > integrity
\n> > of only the locked regions but it might be worth extending that to
\n> > entire windows depending on the performance. We should do some
\n> > measurements.
\n> >
\n> > I think implementations also need to be noisy in the case of a
\n> > discrepancy, logging on the BMC side and possibly issuing a BMC
\n> > Event
\n> > (taking another one of our reserved bits for the purpose). The
\n> > modifications are pretty much limited to accidental scribbling
\n> > (bugs),
\n> > or malicious intent (probably enabled by more bugs). Either way,
\n> > someone should be notified.
\n> >
\n> > > > > +Command:
\n> > > > > > > > +       GET_FLASH_NAME (V3)
\n> > > > > > > > +       Arguments:
\n> > > > > > > > +               Args 0: Flash ID
\n> > > > > > > > +       Response:
\n> > > > >
\n> > > > > +             Args 0-10: Flash Name / UID
\n> > > >
\n> > > > Hopefully 11 bytes is enough. Well, 10 I guess - should we
\n> > > > specify
\n> > > > that the
\n> > > > value at index 10 be '\\0'? I feel like not requiring that could
\n> > > > lead
\n> > > > to
\n> > > > undesirable implementations.
\n> > >
\n> > > Depends on whether we require names be 11 bytes, be null
\n> > > terminated, or
\n> > > just contain trailing nulls for names shorter than 11 bytes.
\n> > >
\n> > > If we're going to waste an argument on the null terminator we may
\n> > > as
\n> > > well just make the first argument the length of the name.
\n> >
\n> > I think that's a better idea generally. It doesn't assume C-string
\n> > semantics, though it's likely that the implementations will require
\n> > them, which was what I was looking out for.
\n> >
\n> > >
\n> > > Is this something the protocol should care about. Maybe it's just
\n> > > up to
\n> > > the daemon and host implementation to put something here that
\n> > > they
\n> > > both
\n> > > understand.
\n> >
\n> > Probably not, it was just a thought. It's a balance against
\n> > avoiding
\n> > potential vulnerabilities and over-specifying behaviour.
\n> >
\n> > >
\n> > > >
\n> > > > I guess we don't want to go down the path of continued
\n> > > > responses
\n> > > > or
\n> > > > magic
\n> > > > windows to accommodate larger names?
\n> > >
\n> > > I'd prefer not :)
\n> > >
\n> >
\n> > Same :) Just throwing the question out there for discussion
\n> >
\n> > > >
\n> > > > > > > > +       Notes:
\n> > > > > > > > +               Describes a flash with some kind of
\n> > > > > > > > identifier
\n> > > > >
\n> > > > > useful to the
\n> > > > > > > > +               host system. This is typically a null-
\n> > > > > > > > padded
\n> > > > >
\n> > > > > string.
\n> > > > > +
\n> > > > > +Command:
\n> > > > > > > > +       MARK_LOCKED (V3)
\n> > > > > > > > +       Arguments:
\n> > > > > > > > +               Args 0-1: Flash offset to lock
\n> > > > > > > > (blocks)
\n> > > > >
\n> > > > > +             Args 2-3: Number to lock at offset (blocks)
\n> > > >
\n> > > > I guess it's hard to get away from using block-sized
\n> > > > granuality.
\n> > > > Do
\n> > > > we
\n> > > > currently have any partitions that we might lock that are less
\n> > > > than a
\n> > > > block
\n> > > > size? Or are all partitions expected to be block-size aligned?
\n> > >
\n> > > This was the motivation for allowing the host to request a block
\n> > > size,
\n> > > so that they can hopefully request something which allows locking
\n> > > at
\n> > > the required granularity. Currently all partitions are aligned to
\n> > > flash
\n> > > erase size (which is what we set block size to anyway).
\n> >
\n> > Okay. It might be worth making a note in the documentation for
\n> > MBOX_GET_INFO saying as much.
\n> >
\n> > >
\n> > > It's not really worth allowing locking at a finer granularity
\n> > > than
\n> > > block size since this is how changes (mark dirty) are specified
\n> > > and
\n> > > so
\n> > > it would be impossible to write back any part of a block if a
\n> > > single
\n> > > byte in that block were locked. I'm in favour of using block size
\n> > > for
\n> > > now and changing it in a later version if this becomes an issue.
\n> >
\n> > Right, it's either blocks for everything or bytes for everything.
\n> > Consistency is good. I don't think we want to go through the effort
\n> > of
\n> > changing that aspect of the spec right now.
\n> >
\n> > Cheers,
\n> >
\n> > Andrew
\n
","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y6HZH5Nvbz9t2V\n\tfor ;\n\tWed, 4 Oct 2017 11:57:59 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y6HZH2dRZzDqnN\n\tfor ;\n\tWed, 4 Oct 2017 11:57:59 +1100 (AEDT)","from mail-qk0-x236.google.com (mail-qk0-x236.google.com\n\t[IPv6:2607:f8b0:400d:c09::236])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3y65rZ0mYRzDqks\n\tfor ; Wed, 4 Oct 2017 04:39:35 +1100 (AEDT)","by mail-qk0-x236.google.com with SMTP id d67so5603924qkg.5\n\tfor ; Tue, 03 Oct 2017 10:39:35 -0700 (PDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"rDU+7MOe\"; dkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"rDU+7MOe\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=google.com\n\t(client-ip=2607:f8b0:400d:c09::236; helo=mail-qk0-x236.google.com;\n\tenvelope-from=wak@google.com; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"rDU+7MOe\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Wt9AZQZT0T4uCD3BPeRHP56ToOD1r/80H3ahn4CYvq4=;\n\tb=rDU+7MOezwD81SxHMIFSpK/1DhCtXScuKyPTxqPYprR42HAV1DbE9tW3OCD4dRvyyq\n\tIhLF+MjV4dJMqJd4AGzQiLWfAAQZH8/JYEV8iyecJo+JXQyvAmyzw3Ktwt4IdN041J0K\n\tQKQdl/VwtvvSr1xQ9hVBtQRUTUE66tVSyZfb9FXaDOJwutH6yLnXtlb/L3OjnYKkPLq8\n\tp5Q06g2Od6WmyWXYgfCx+9dtMt2+xF4Hug5M16f4MQjDbdBrsE2P1iEedJlAeLePR9G/\n\tU+NxCtAv9xsAMOK6GDSMT28PPDQKkgCJ6hHQLzFQ/Qx7qdYXaC3lZklGzlUxX+/gbsrM\n\t8iCw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Wt9AZQZT0T4uCD3BPeRHP56ToOD1r/80H3ahn4CYvq4=;\n\tb=SmmWCmw0GtNwn6Cs+G19CrZrdIvq4bMP0sjtMFMxkGoLHbz5Yzvb+wh3iuGaUhkZMg\n\tkpNPOwM5+APGg1qFy8newKZTRjv0d0lmyNLku7XXOmFHTW/I0kkuLWhALptl0Cvsk8ZQ\n\tcLDD0ow7VT28z1wVKOLHxvA4OaMyCnxxWBf2NDvQXYvCEYGyWdBOYFGJ+5Nphu1w9oNR\n\tqTIgAWe7chMjzLTazGdVcb5mcsulFn9dPCoF+TrVy1Vb6RZ+yR2SxzV3kIgIDMGIViIX\n\tBFbT6VB3JppYLSSUX5Sm9PD/89Pz6iKPZ2gpDIAVveTMv9MGjD/dFe85Sb1g52ONkyJn\n\teytQ==","X-Gm-Message-State":"AMCzsaUP7pIUHVt1rcM5BH0wVKl0fa6vrg607TVYi42iO3YqWnVY9qiM\n\tVJoUHbvS1GiskoupG3dgMI2s/xVtgjjGJhEqJfbvfw==","X-Google-Smtp-Source":"AOwi7QB7YnFrRzta+S3CLEu76LN4N0kw0ZytJBKEukaxANYIToMXEwr8EoeduOdt4ORc5O9Qc63gWGUQEh8QU6U+RVE=","X-Received":"by 10.55.107.5 with SMTP id g5mr20536296qkc.344.1507052370665;\n\tTue, 03 Oct 2017 10:39:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>\n\t<1505113305.4080.1.camel@aj.id.au>\n\t<1505189782.2222.1.camel@gmail.com>\n\t<1505354227.4080.11.camel@aj.id.au>\n\t<1505796357.30609.1.camel@gmail.com>\n\t<1507004777.2329.1.camel@gmail.com>","In-Reply-To":"<1507004777.2329.1.camel@gmail.com>","From":"William Kennington ","Date":"Tue, 03 Oct 2017 17:39:19 +0000","Message-ID":"","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","To":"Suraj Jitindar Singh ,\n\tAndrew Jeffery , skiboot@lists.ozlabs.org, \n\topenbmc@lists.ozlabs.org","Content-Type":"multipart/alternative; boundary=\"001a114878cc36af71055aa7f901\"","X-Mailman-Approved-At":"Wed, 04 Oct 2017 11:57:55 +1100","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, cyrilbur@gmail.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1784146,"web_url":"http://patchwork.ozlabs.org/comment/1784146/","msgid":"<1C77CFF2-D774-4216-85F8-26736DE58BB9@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-10-10T20:59:58","subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","submitter":{"id":70939,"url":"http://patchwork.ozlabs.org/api/people/70939/","name":"Adriana Kobylak","email":"anoo@linux.vnet.ibm.com"},"content":"Acked-by: Adriana Kobylak .com>\n\n\n> On Oct 3, 2017, at 12:39 PM, William Kennington wrote:\n> \n> Yeah, go ahead and change it to that\n> \n> On Mon, Oct 2, 2017 at 9:26 PM Suraj Jitindar Singh > wrote:\n> @wak ping,\n> \n> Thoughts on using the first arg in the response of get flash name as a\n> length argument?\n> \n> On Tue, 2017-09-19 at 14:45 +1000, Suraj Jitindar Singh wrote:\n> > @wak:\n> >\n> > As per Andrews suggestion below, how do you feel about making the\n> > first\n> > arg in the response for get flash name the length?\n> >\n> > @andrew:\n> >\n> > Since your comments regarding flash integrity aren't really relevant\n> > at\n> > the protocol level do you think it's something which needs to be\n> > mentioned in this document or is just common sense (unless you're me\n> > and didn't implement it that way :p)...\n> >\n> > On Thu, 2017-09-14 at 11:27 +0930, Andrew Jeffery wrote:\n> > > > > > +The host may lock an area of flash using the MARK_LOCKED\n> > > > > > command.\n> > > > > > Any attempt\n> > > > > > +to mark dirty or erased this area of flash must fail with\n> > > > > > the\n> > > > > > LOCKED_ERROR\n> > > > > > +response code. The host may open a write window which\n> > > > > > contains\n> > > > > > a\n> > > > > > locked area\n> > > > > > +of flash however changes to a locked area of flash must\n> > > > > > never\n> > > > > > be\n> > > > > > written back\n> > > > > > +to the backing data source (i.e. that area of flash must be\n> > > > > > treated as read\n> > > > > > +only). An attempt to lock an area of flash which is not\n> > > > > > clean\n> > > > > > in\n> > > > > > the current\n> > > > > > +window must fail with PARAM_ERROR. (V3)\n> > > > >\n> > > > > I think we need to consider how locked regions interact with\n> > > > > the\n> > > > > in-\n> > > > > memory\n> > > > > caching of the flash data.\n> > > > >\n> > > > > The BMC doesn't know that the memory backing the LPC window has\n> > > > > been\n> > > > > written until it receives a MARK_DIRTY from the host.\n> > > > > MARK_DIRTY\n> > > > > is\n> > > > > not\n> > > > > valid on a read window or a locked region, but that doesn't\n> > > > > mean\n> > > > > that\n> > > > > changes to a locked region can't persist in memory due to\n> > > > > caching\n> > > > > to\n> > > > > avoid flash access (which was a part of the motivation for\n> > > > > memory-\n> > > > > based \n> > > > > booting to begin with). This may naively fool host firmware\n> > > > > into\n> > > > > thinking it is\n> > > > > accessing unmolested data as the region is locked, whereas the\n> > > > > region\n> > > > > could\n> > > > > contain anything, just it will never be written to flash.\n> > > >\n> > > > Correct\n> > > >\n> > > > >\n> > > > > Should we require window requests intersecting a locked region\n> > > > > always\n> > > > > have at\n> > > > > least the locked region positively match the on-flash data?\n> > > > > That\n> > > > > way\n> > > > > the\n> > > > > firmware that requested a region be locked could ensure it has\n> > > > > whatever was on\n> > > > > the flash at the time it was locked by explicitly closing the\n> > > > > window\n> > > > > (read or\n> > > > > write) once finished with it, which requires a window be opened\n> > > > > again\n> > > > > for any\n> > > > > subsequent access. At that point (open) we can do the locked\n> > > > > region\n> > > > > intesection\n> > > > > calculation and check the data in the window as necessary.\n> > > >\n> > > > I guess this brings about an interesting question, when a window\n> > > > is\n> > > > requested is it guaranteed that the window provided in return by\n> > > > the\n> > > > BMC contains the same contents as the flash for the requested\n> > > > area?\n> > >\n> > > I think expecting otherwise would be strange. What way would the\n> > > host\n> > > have to validate that? It could keep its own hashes of window\n> > > content,\n> > > but it would need to hash the content itself after opening the\n> > > window\n> > > which may be a circular problem (e.g. host reboot if the daemon\n> > > doesn't\n> > > invalidate the cache across the reboot operation).\n> > >\n> > > > It\n> > > > would seem intuitively that it should. The first time a window is\n> > > > opened that is true, but with in memory caching the host could\n> > > > make\n> > > > a\n> > > > change that it never tells the bmc about and the contents\n> > > > diverge,\n> > > > and\n> > > > (assuming the window isn't evicted) persist across window\n> > > > close/open\n> > > > calls.\n> > >\n> > > Yep.\n> > >\n> > > >\n> > > > Is this the desired behaviour? \n> > >\n> > > No, not in my opinion. It doesn't fit the principle of least\n> > > surprise.\n> > >\n> > > > It's kind of the only behaviour which\n> > > > makes the in-memory caching an effective performance improvement,\n> > > > if we\n> > > > reloaded every window every time then we may as well only have\n> > > > one\n> > > > window in memory anyway. Maybe we can do this a better way (see\n> > > > below).\n> > > >\n> > > > It's worth noting that the current flash locking daemon\n> > > > implementation\n> > > > will explicitly reload an entire window whenever one which\n> > > > contains\n> > > > any\n> > > > locked regions is requested - ensuring (for at least as long as\n> > > > the\n> > > > access to the flash device is exclusive) the integrity of the\n> > > > locked\n> > > > regions. This could of course be optimised to only reload the\n> > > > locked\n> > > > region.\n> > >\n> > > Right, and if we take up the techniques discussed below we may even\n> > > be\n> > > able to eliminate that.\n> > >\n> > > >\n> > > > >\n> > > > > Locked regions could thus be a performance hit if we always\n> > > > > load\n> > > > > the\n> > > > > locked\n> > > > > regions from flash, but surely we prefer (some) integrity over\n> > > > > performance.\n> > > > >\n> > > > > Alternatively, you could cryptographically hash the per-block\n> > > > > content\n> > > > > of the\n> > > > > on-flash locked region during the MARK_LOCKED operation, then\n> > > > > stash\n> > > > > the hash\n> > > > > values away. On a CREATE_{READ,WRITE}_WINDOW operation you\n> > > > > could\n> > > > > hash\n> > > > > any\n> > > > > intersecting, in-cache locked blocks and compare to the stashed\n> > > > > hash\n> > > > > values to\n> > > > > incur only one set of flash accesses (initial hash\n> > > > > calculations)\n> > > > > rather than n.\n> > > >\n> > > > This seems like a nice balance between performance and data\n> > > > integrity.\n> > > >\n> > > > I guess this brings about the question again as to whether we\n> > > > care\n> > > > about data changes we aren't told about. Do we store a hash of\n> > > > every\n> > > > window and before providing a cached copy verify the hash and\n> > > > reload\n> > > > the entire window from flash again if there is a discrepancy. The\n> > > > host\n> > > > isn't allowed to rely on the persistence of data it never tells\n> > > > the\n> > > > BMC\n> > > > about, but is this something the BMC should be checking?\n> > >\n> > > I think so, again in support of the principle of least surprise,\n> > > and\n> > > the possible security implications. I proposed verifying the\n> > > integrity\n> > > of only the locked regions but it might be worth extending that to\n> > > entire windows depending on the performance. We should do some\n> > > measurements.\n> > >\n> > > I think implementations also need to be noisy in the case of a\n> > > discrepancy, logging on the BMC side and possibly issuing a BMC\n> > > Event\n> > > (taking another one of our reserved bits for the purpose). The\n> > > modifications are pretty much limited to accidental scribbling\n> > > (bugs),\n> > > or malicious intent (probably enabled by more bugs). Either way,\n> > > someone should be notified.\n> > >\n> > > > > > +Command:\n> > > > > > > > > + GET_FLASH_NAME (V3)\n> > > > > > > > > + Arguments:\n> > > > > > > > > + Args 0: Flash ID\n> > > > > > > > > + Response:\n> > > > > >\n> > > > > > + Args 0-10: Flash Name / UID\n> > > > >\n> > > > > Hopefully 11 bytes is enough. Well, 10 I guess - should we\n> > > > > specify\n> > > > > that the\n> > > > > value at index 10 be '\\0'? I feel like not requiring that could\n> > > > > lead\n> > > > > to\n> > > > > undesirable implementations.\n> > > >\n> > > > Depends on whether we require names be 11 bytes, be null\n> > > > terminated, or\n> > > > just contain trailing nulls for names shorter than 11 bytes.\n> > > >\n> > > > If we're going to waste an argument on the null terminator we may\n> > > > as\n> > > > well just make the first argument the length of the name.\n> > >\n> > > I think that's a better idea generally. It doesn't assume C-string\n> > > semantics, though it's likely that the implementations will require\n> > > them, which was what I was looking out for.\n> > >\n> > > >\n> > > > Is this something the protocol should care about. Maybe it's just\n> > > > up to\n> > > > the daemon and host implementation to put something here that\n> > > > they\n> > > > both\n> > > > understand.\n> > >\n> > > Probably not, it was just a thought. It's a balance against\n> > > avoiding\n> > > potential vulnerabilities and over-specifying behaviour.\n> > >\n> > > >\n> > > > >\n> > > > > I guess we don't want to go down the path of continued\n> > > > > responses\n> > > > > or\n> > > > > magic\n> > > > > windows to accommodate larger names?\n> > > >\n> > > > I'd prefer not :)\n> > > >\n> > >\n> > > Same :) Just throwing the question out there for discussion\n> > >\n> > > > >\n> > > > > > > > > + Notes:\n> > > > > > > > > + Describes a flash with some kind of\n> > > > > > > > > identifier\n> > > > > >\n> > > > > > useful to the\n> > > > > > > > > + host system. This is typically a null-\n> > > > > > > > > padded\n> > > > > >\n> > > > > > string.\n> > > > > > +\n> > > > > > +Command:\n> > > > > > > > > + MARK_LOCKED (V3)\n> > > > > > > > > + Arguments:\n> > > > > > > > > + Args 0-1: Flash offset to lock\n> > > > > > > > > (blocks)\n> > > > > >\n> > > > > > + Args 2-3: Number to lock at offset (blocks)\n> > > > >\n> > > > > I guess it's hard to get away from using block-sized\n> > > > > granuality.\n> > > > > Do\n> > > > > we\n> > > > > currently have any partitions that we might lock that are less\n> > > > > than a\n> > > > > block\n> > > > > size? Or are all partitions expected to be block-size aligned?\n> > > >\n> > > > This was the motivation for allowing the host to request a block\n> > > > size,\n> > > > so that they can hopefully request something which allows locking\n> > > > at\n> > > > the required granularity. Currently all partitions are aligned to\n> > > > flash\n> > > > erase size (which is what we set block size to anyway).\n> > >\n> > > Okay. It might be worth making a note in the documentation for\n> > > MBOX_GET_INFO saying as much.\n> > >\n> > > >\n> > > > It's not really worth allowing locking at a finer granularity\n> > > > than\n> > > > block size since this is how changes (mark dirty) are specified\n> > > > and\n> > > > so\n> > > > it would be impossible to write back any part of a block if a\n> > > > single\n> > > > byte in that block were locked. I'm in favour of using block size\n> > > > for\n> > > > now and changing it in a later version if this becomes an issue.\n> > >\n> > > Right, it's either blocks for everything or bytes for everything.\n> > > Consistency is good. I don't think we want to go through the effort\n> > > of\n> > > changing that aspect of the spec right now.\n> > >\n> > > Cheers,\n> > >\n> > > Andrew\nAcked-by: Adriana Kobylak <anoo@us.ibm.com>


On Oct 3, 2017, at 12:39 PM, William Kennington <wak@google.com> wrote:

Yeah, go ahead and change it to that

On Mon, Oct 2, 2017 at 9:26 PM Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:
@wak ping,
\n
\nThoughts on using the first arg in the response of get flash name as a
\nlength argument?
\n
\nOn Tue, 2017-09-19 at 14:45 +1000, Suraj Jitindar Singh wrote:
\n> @wak:
\n>
\n> As per Andrews suggestion below, how do you feel about making the
\n> first
\n> arg in the response for get flash name the length?
\n>
\n> @andrew:
\n>
\n> Since your comments regarding flash integrity aren't really relevant
\n> at
\n> the protocol level do you think it's something which needs to be
\n> mentioned in this document or is just common sense (unless you're me
\n> and didn't implement it that way :p)...
\n>
\n> On Thu, 2017-09-14 at 11:27 +0930, Andrew Jeffery wrote:
\n> > > > > +The host may lock an area of flash using the MARK_LOCKED
\n> > > > > command.
\n> > > > > Any attempt
\n> > > > > +to mark dirty or erased this area of flash must fail with
\n> > > > > the
\n> > > > > LOCKED_ERROR
\n> > > > > +response code. The host may open a write window which
\n> > > > > contains
\n> > > > > a
\n> > > > > locked area
\n> > > > > +of flash however changes to a locked area of flash must
\n> > > > > never
\n> > > > > be
\n> > > > > written back
\n> > > > > +to the backing data source (i.e. that area of flash must be
\n> > > > > treated as read
\n> > > > > +only). An attempt to lock an area of flash which is not
\n> > > > > clean
\n> > > > > in
\n> > > > > the current
\n> > > > > +window must fail with PARAM_ERROR. (V3)
\n> > > >
\n> > > > I think we need to consider how locked regions interact with
\n> > > > the
\n> > > > in-
\n> > > > memory
\n> > > > caching of the flash data.
\n> > > >
\n> > > > The BMC doesn't know that the memory backing the LPC window has
\n> > > > been
\n> > > > written until it receives a MARK_DIRTY from the host.
\n> > > > MARK_DIRTY
\n> > > > is
\n> > > > not
\n> > > > valid on a read window or a locked region, but that doesn't
\n> > > > mean
\n> > > > that
\n> > > > changes to a locked region can't persist in memory due to
\n> > > > caching
\n> > > > to
\n> > > > avoid flash access (which was a part of the motivation for
\n> > > > memory-
\n> > > > based 
\n> > > > booting to begin with). This may naively fool host firmware
\n> > > > into
\n> > > > thinking it is
\n> > > > accessing unmolested data as the region is locked, whereas the
\n> > > > region
\n> > > > could
\n> > > > contain anything, just it will never be written to flash.
\n> > >
\n> > > Correct
\n> > >
\n> > > >
\n> > > > Should we require window requests intersecting a locked region
\n> > > > always
\n> > > > have at
\n> > > > least the locked region positively match the on-flash data?
\n> > > > That
\n> > > > way
\n> > > > the
\n> > > > firmware that requested a region be locked could ensure it has
\n> > > > whatever was on
\n> > > > the flash at the time it was locked by explicitly closing the
\n> > > > window
\n> > > > (read or
\n> > > > write) once finished with it, which requires a window be opened
\n> > > > again
\n> > > > for any
\n> > > > subsequent access. At that point (open) we can do the locked
\n> > > > region
\n> > > > intesection
\n> > > > calculation and check the data in the window as necessary.
\n> > >
\n> > > I guess this brings about an interesting question, when a window
\n> > > is
\n> > > requested is it guaranteed that the window provided in return by
\n> > > the
\n> > > BMC contains the same contents as the flash for the requested
\n> > > area?
\n> >
\n> > I think expecting otherwise would be strange. What way would the
\n> > host
\n> > have to validate that? It could keep its own hashes of window
\n> > content,
\n> > but it would need to hash the content itself after opening the
\n> > window
\n> > which may be a circular problem (e.g. host reboot if the daemon
\n> > doesn't
\n> > invalidate the cache across the reboot operation).
\n> >
\n> > >  It
\n> > > would seem intuitively that it should. The first time a window is
\n> > > opened that is true, but with in memory caching the host could
\n> > > make
\n> > > a
\n> > > change that it never tells the bmc about and the contents
\n> > > diverge,
\n> > > and
\n> > > (assuming the window isn't evicted) persist across window
\n> > > close/open
\n> > > calls.
\n> >
\n> > Yep.
\n> >
\n> > >
\n> > > Is this the desired behaviour? 
\n> >
\n> > No, not in my opinion. It doesn't fit the principle of least
\n> > surprise.
\n> >
\n> > > It's kind of the only behaviour which
\n> > > makes the in-memory caching an effective performance improvement,
\n> > > if we
\n> > > reloaded every window every time then we may as well only have
\n> > > one
\n> > > window in memory anyway. Maybe we can do this a better way (see
\n> > > below).
\n> > >
\n> > > It's worth noting that the current flash locking daemon
\n> > > implementation
\n> > > will explicitly reload an entire window whenever one which
\n> > > contains
\n> > > any
\n> > > locked regions is requested - ensuring (for at least as long as
\n> > > the
\n> > > access to the flash device is exclusive) the integrity of the
\n> > > locked
\n> > > regions. This could of course be optimised to only reload the
\n> > > locked
\n> > > region.
\n> >
\n> > Right, and if we take up the techniques discussed below we may even
\n> > be
\n> > able to eliminate that.
\n> >
\n> > >
\n> > > >
\n> > > > Locked regions could thus be a performance hit if we always
\n> > > > load
\n> > > > the
\n> > > > locked
\n> > > > regions from flash, but surely we prefer (some) integrity over
\n> > > > performance.
\n> > > >
\n> > > > Alternatively, you could cryptographically hash the per-block
\n> > > > content
\n> > > > of the
\n> > > > on-flash locked region during the MARK_LOCKED operation, then
\n> > > > stash
\n> > > > the hash
\n> > > > values away. On a CREATE_{READ,WRITE}_WINDOW operation you
\n> > > > could
\n> > > > hash
\n> > > > any
\n> > > > intersecting, in-cache locked blocks and compare to the stashed
\n> > > > hash
\n> > > > values to
\n> > > > incur only one set of flash accesses (initial hash
\n> > > > calculations)
\n> > > > rather than n.
\n> > >
\n> > > This seems like a nice balance between performance and data
\n> > > integrity.
\n> > >
\n> > > I guess this brings about the question again as to whether we
\n> > > care
\n> > > about data changes we aren't told about. Do we store a hash of
\n> > > every
\n> > > window and before providing a cached copy verify the hash and
\n> > > reload
\n> > > the entire window from flash again if there is a discrepancy. The
\n> > > host
\n> > > isn't allowed to rely on the persistence of data it never tells
\n> > > the
\n> > > BMC
\n> > > about, but is this something the BMC should be checking?
\n> >
\n> > I think so, again in support of the principle of least surprise,
\n> > and
\n> > the possible security implications. I proposed verifying the
\n> > integrity
\n> > of only the locked regions but it might be worth extending that to
\n> > entire windows depending on the performance. We should do some
\n> > measurements.
\n> >
\n> > I think implementations also need to be noisy in the case of a
\n> > discrepancy, logging on the BMC side and possibly issuing a BMC
\n> > Event
\n> > (taking another one of our reserved bits for the purpose). The
\n> > modifications are pretty much limited to accidental scribbling
\n> > (bugs),
\n> > or malicious intent (probably enabled by more bugs). Either way,
\n> > someone should be notified.
\n> >
\n> > > > > +Command:
\n> > > > > > > > +       GET_FLASH_NAME (V3)
\n> > > > > > > > +       Arguments:
\n> > > > > > > > +               Args 0: Flash ID
\n> > > > > > > > +       Response:
\n> > > > >
\n> > > > > +             Args 0-10: Flash Name / UID
\n> > > >
\n> > > > Hopefully 11 bytes is enough. Well, 10 I guess - should we
\n> > > > specify
\n> > > > that the
\n> > > > value at index 10 be '\\0'? I feel like not requiring that could
\n> > > > lead
\n> > > > to
\n> > > > undesirable implementations.
\n> > >
\n> > > Depends on whether we require names be 11 bytes, be null
\n> > > terminated, or
\n> > > just contain trailing nulls for names shorter than 11 bytes.
\n> > >
\n> > > If we're going to waste an argument on the null terminator we may
\n> > > as
\n> > > well just make the first argument the length of the name.
\n> >
\n> > I think that's a better idea generally. It doesn't assume C-string
\n> > semantics, though it's likely that the implementations will require
\n> > them, which was what I was looking out for.
\n> >
\n> > >
\n> > > Is this something the protocol should care about. Maybe it's just
\n> > > up to
\n> > > the daemon and host implementation to put something here that
\n> > > they
\n> > > both
\n> > > understand.
\n> >
\n> > Probably not, it was just a thought. It's a balance against
\n> > avoiding
\n> > potential vulnerabilities and over-specifying behaviour.
\n> >
\n> > >
\n> > > >
\n> > > > I guess we don't want to go down the path of continued
\n> > > > responses
\n> > > > or
\n> > > > magic
\n> > > > windows to accommodate larger names?
\n> > >
\n> > > I'd prefer not :)
\n> > >
\n> >
\n> > Same :) Just throwing the question out there for discussion
\n> >
\n> > > >
\n> > > > > > > > +       Notes:
\n> > > > > > > > +               Describes a flash with some kind of
\n> > > > > > > > identifier
\n> > > > >
\n> > > > > useful to the
\n> > > > > > > > +               host system. This is typically a null-
\n> > > > > > > > padded
\n> > > > >
\n> > > > > string.
\n> > > > > +
\n> > > > > +Command:
\n> > > > > > > > +       MARK_LOCKED (V3)
\n> > > > > > > > +       Arguments:
\n> > > > > > > > +               Args 0-1: Flash offset to lock
\n> > > > > > > > (blocks)
\n> > > > >
\n> > > > > +             Args 2-3: Number to lock at offset (blocks)
\n> > > >
\n> > > > I guess it's hard to get away from using block-sized
\n> > > > granuality.
\n> > > > Do
\n> > > > we
\n> > > > currently have any partitions that we might lock that are less
\n> > > > than a
\n> > > > block
\n> > > > size? Or are all partitions expected to be block-size aligned?
\n> > >
\n> > > This was the motivation for allowing the host to request a block
\n> > > size,
\n> > > so that they can hopefully request something which allows locking
\n> > > at
\n> > > the required granularity. Currently all partitions are aligned to
\n> > > flash
\n> > > erase size (which is what we set block size to anyway).
\n> >
\n> > Okay. It might be worth making a note in the documentation for
\n> > MBOX_GET_INFO saying as much.
\n> >
\n> > >
\n> > > It's not really worth allowing locking at a finer granularity
\n> > > than
\n> > > block size since this is how changes (mark dirty) are specified
\n> > > and
\n> > > so
\n> > > it would be impossible to write back any part of a block if a
\n> > > single
\n> > > byte in that block were locked. I'm in favour of using block size
\n> > > for
\n> > > now and changing it in a later version if this becomes an issue.
\n> >
\n> > Right, it's either blocks for everything or bytes for everything.
\n> > Consistency is good. I don't think we want to go through the effort
\n> > of
\n> > changing that aspect of the spec right now.
\n> >
\n> > Cheers,
\n> >
\n> > Andrew
\n
\n

","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yBV1P2160z9t6C\n\tfor ;\n\tWed, 11 Oct 2017 08:02:33 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yBV1P03zyzDr55\n\tfor ;\n\tWed, 11 Oct 2017 08:02:32 +1100 (AEDT)","from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com\n\t[148.163.156.1])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yBV146v5bzDqlv\n\tfor ; Wed, 11 Oct 2017 08:02:16 +1100 (AEDT)","from pps.filterd (m0098409.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv9AL0h0b026645\n\tfor ; Tue, 10 Oct 2017 17:02:14 -0400","from e33.co.us.ibm.com (e33.co.us.ibm.com [32.97.110.151])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2dh3k46m32-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Tue, 10 Oct 2017 17:02:13 -0400","from localhost\n\tby e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tTue, 10 Oct 2017 15:02:13 -0600","from b03cxnp07029.gho.boulder.ibm.com (9.17.130.16)\n\tby e33.co.us.ibm.com (192.168.1.133) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tTue, 10 Oct 2017 15:02:09 -0600","from b03ledav004.gho.boulder.ibm.com\n\t(b03ledav004.gho.boulder.ibm.com [9.17.130.235])\n\tby b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v9AL299I6226408; Tue, 10 Oct 2017 14:02:09 -0700","from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 2CEFC78047;\n\tTue, 10 Oct 2017 15:02:09 -0600 (MDT)","from adriana-mbp.austin.ibm.com (unknown [9.53.179.190])\n\tby b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTPS id\n\t7CE4B78038; Tue, 10 Oct 2017 15:02:08 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.156.1; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=anoo@linux.vnet.ibm.com; receiver=)","From":"Adriana Kobylak ","Content-Type":"multipart/alternative;\n\tboundary=\"Apple-Mail=_7AD270B5-A458-4A71-89DD-4A3AFBE67C3A\"","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","Subject":"Re: [RFC] docs: Specify V3 of the mbox protocol","Date":"Tue, 10 Oct 2017 15:59:58 -0500","In-Reply-To":"","To":"William Kennington ","References":"<20170907071409.7163-1-sjitindarsingh@gmail.com>\n\t<20170907071409.7163-2-sjitindarsingh@gmail.com>\n\t<1505113305.4080.1.camel@aj.id.au>\n\t<1505189782.2222.1.camel@gmail.com>\n\t<1505354227.4080.11.camel@aj.id.au>\n\t<1505796357.30609.1.camel@gmail.com>\n\t<1507004777.2329.1.camel@gmail.com>\n\t","X-Mailer":"Apple Mail (2.3273)","X-TM-AS-GCONF":"00","x-cbid":"17101021-0008-0000-0000-000008B1969C","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007874; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000235; SDB=6.00929270; UDB=6.00467703;\n\tIPR=6.00709541; \n\tBA=6.00005632; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017481;\n\tXFM=3.00000015; UTC=2017-10-10 21:02:11","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17101021-0009-0000-0000-0000444EEB8C","Message-Id":"<1C77CFF2-D774-4216-85F8-26736DE58BB9@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-10-10_06:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1710100299","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"stewart@linux.vnet.ibm.com, Andrew Jeffery ,\n\topenbmc@lists.ozlabs.org, skiboot@lists.ozlabs.org, cyrilbur@gmail.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]