[Skiboot] [RFC PATCH 5/6] libflash: Add ipmi-flash implementation
Andrew Jeffery
andrew at aj.id.au
Wed Sep 19 15:39:10 AEST 2018
On Wed, 19 Sep 2018, at 14:28, Stewart Smith wrote:
> Andrew Jeffery <andrew at aj.id.au> writes:
> > ipmi-flash adapts the "mbox" protocol to use IPMI as a transport. The
> > name "mbox" for the protocol is now meaningless - the ASPEED BMC mailbox
> > is no-longer relevant when using IPMI as a transport.
> >
> > The same commands and events are used, though the implementation assumes
> > v2 of the protocol is supported by the BMC
> >
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> > include/platform.h | 1 +
> > libflash/Makefile.inc | 2 +-
> > libflash/ipmi-flash.c | 792 ++++++++++++++++++++++++++++++++++++++
> > libflash/ipmi-flash.h | 29 ++
> > platforms/astbmc/common.c | 1 +
> > 5 files changed, 824 insertions(+), 1 deletion(-)
> > create mode 100644 libflash/ipmi-flash.c
> > create mode 100644 libflash/ipmi-flash.h
> >
> > diff --git a/include/platform.h b/include/platform.h
> > index 1a35a86a6f8b..edcbeac9a2cf 100644
> > --- a/include/platform.h
> > +++ b/include/platform.h
> > @@ -42,6 +42,7 @@ struct bmc_platform {
> > */
> > uint32_t ipmi_oem_partial_add_esel;
> > uint32_t ipmi_oem_pnor_access_status;
> > + uint32_t ipmi_oem_hiomap_cmd;
> > };
>
> Considering this, I'm thinking it may be more appropriate to have the
> filenames be hiomap rather than ipmi-flash ?
I went with ipmi-flash for consistency with mbox-flash, as both are really
implementations of the same thing, just over different transports. But,
the mbox transport is effectively deprecated, so calling the ipmi-based
implementation hiomap isn't too insane (as the mbox implementation
will eventually go away).
>
> >
> > /* OpenCAPI platform-specific I2C information */
> > diff --git a/libflash/Makefile.inc b/libflash/Makefile.inc
> > index 2474abfccc61..c384c4e91df9 100644
> > --- a/libflash/Makefile.inc
> > +++ b/libflash/Makefile.inc
> > @@ -1,4 +1,4 @@
> > -LIBFLASH_SRCS = libflash.c libffs.c ecc.c blocklevel.c mbox-flash.c
> > +LIBFLASH_SRCS = libflash.c libffs.c ecc.c blocklevel.c mbox-flash.c ipmi-flash.c
> > LIBFLASH_OBJS = $(LIBFLASH_SRCS:%.c=%.o)
> >
> > SUBDIRS += libflash
> > diff --git a/libflash/ipmi-flash.c b/libflash/ipmi-flash.c
> > new file mode 100644
> > index 000000000000..02e4d817791d
> > --- /dev/null
> > +++ b/libflash/ipmi-flash.c
> > @@ -0,0 +1,792 @@
> > +/* Copyright 2018 IBM Corp.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > + * implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#define pr_fmt(fmt) "IPMI-FLASH: " fmt
>
> as per filename discussion, I'm thinking HIOMAP rather than IPMI-FLASH
> here for the prefix
Yep.
>
> > +#include <inttypes.h>
> > +#include <lock.h>
> > +#include <lpc.h>
> > +#include <lpc-mbox.h>
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +#include <ipmi.h>
> > +
> > +#include <ccan/container_of/container_of.h>
> > +
> > +#include "errors.h"
> > +#include "ipmi-flash.h"
> > +
> > +#define CMD_OP_HIOMAP_EVENT 0x0f
> > +
> > +struct lpc_window {
> > + uint32_t lpc_addr; /* Offset into LPC space */
> > + uint32_t cur_pos; /* Current position of the window in the flash */
> > + uint32_t size; /* Size of the window into the flash */
> > + bool open;
> > +};
> > +
> > +struct ipmi_flash_data {
> > + uint8_t seq;
> > + uint8_t version;
> > + uint8_t block_size_shift;
> > + uint16_t timeout;
> > + struct blocklevel_device bl;
> > + struct lpc_window read;
> > + struct lpc_window write;
> > + uint32_t total_size;
> > + uint32_t erase_granule;
> > +
> > + struct lock lock;
> > + bool update;
> > + uint8_t status;
> > +};
> > +
> > +struct ipmi_hiomap_result {
> > + struct ipmi_flash_data *ctx;
> > + int16_t cc;
> > +};
> > +
> > +#define RESULT_INIT(_name, _ctx) struct ipmi_hiomap_result _name = { _ctx, -1 }
> > +
> > +static uint16_t msg_get_u16(void *src)
> > +{
> > + uint16_t arg;
> > +
> > + memcpy(&arg, src, sizeof(arg));
> > +
> > + return le16_to_cpu(arg);
> > +}
> > +
> > +static void msg_put_u16(void *dst, uint16_t val)
> > +{
> > + val = cpu_to_le16(val);
> > +
> > + memcpy(dst, &val, sizeof(val));
> > +}
> > +
> > +static uint32_t blocks_to_bytes(struct ipmi_flash_data *ctx, uint16_t blocks)
> > +{
> > + return blocks << ctx->block_size_shift;
> > +}
> > +
> > +static uint16_t bytes_to_blocks(struct ipmi_flash_data *ctx, uint32_t bytes)
> > +{
> > + return bytes >> ctx->block_size_shift;
> > +}
> > +
> > +#define HIOMAP_C_GET_INFO MBOX_C_GET_MBOX_INFO
> > +#define HIOMAP_C_GET_FLASH_INFO MBOX_C_GET_FLASH_INFO
> > +#define HIOMAP_C_CREATE_READ_WINDOW MBOX_C_CREATE_READ_WINDOW
> > +#define HIOMAP_C_CREATE_WRITE_WINDOW MBOX_C_CREATE_WRITE_WINDOW
> > +#define HIOMAP_C_CLOSE_WINDOW MBOX_C_CLOSE_WINDOW
> > +#define HIOMAP_C_MARK_DIRTY MBOX_C_MARK_WRITE_DIRTY
> > +#define HIOMAP_C_FLUSH MBOX_C_WRITE_FLUSH
> > +#define HIOMAP_C_ACK MBOX_C_BMC_EVENT_ACK
> > +#define HIOMAP_C_ERASE MBOX_C_MARK_WRITE_ERASED
> > +
> > +#define HIOMAP_E_ACK_MASK MBOX_ATTN_ACK_MASK
> > +#define HIOMAP_E_BMC_REBOOT MBOX_ATTN_BMC_REBOOT
> > +#define HIOMAP_E_WINDOW_RESET MBOX_ATTN_BMC_WINDOW_RESET
> > +#define HIOMAP_E_FLASH_LOST MBOX_ATTN_BMC_FLASH_LOST
> > +#define HIOMAP_E_DAEMON_READY
> > MBOX_ATTN_BMC_DAEMON_READY
>
> I wonder if we should just code the #defines here rather than defining
> it terms of the mbox defines? Or is that the better way to do the spec update?
This was a hack - using the MBOX_ defines requires the lpc-mbox.h header,
which we don't want anyway (as it's just confusing). I've already reworked this
so we have a `include/hiomap.h` with the HIOMAP_ macros mapping directly
to the relevant numbers, and I've redefined the MBOX_ in terms of the HIOMAP_
macros. That way everything should stay in sync and we can ignore-and-eventually
delete the MBOX_ macros without side-effects.
>
> > +/* Is the current window able perform the complete operation */
> > +static bool hiomap_window_valid(struct lpc_window *win, uint64_t pos,
> > + uint64_t len)
> > +{
> > + if (!win->open)
> > + return false;
> > + if (pos < win->cur_pos) /* start */
> > + return false;
> > + if ((pos + len) > (win->cur_pos + win->size)) /* end */
> > + return false;
> > + return true;
> > +}
> > +
> > +
> > +static void ipmi_flash_cmd_cb(struct ipmi_msg *msg)
> > +{
> > + struct ipmi_hiomap_result *res = msg->user_data;
> > + struct ipmi_flash_data *ctx = res->ctx;
> > +
> > + res->cc = msg->cc;
> > + if (msg->cc != IPMI_CC_NO_ERROR) {
> > + return;
> > + }
> > +
> > + /* We at least need the command and sequence */
> > + if (msg->resp_size < 2) {
> > + prerror("Illegal response size: %u\n", msg->resp_size);
> > + abort();
> > + }
>
> Should we be checking sequence number?
Yep! Not sure what happened there.
>
> > +
> > + switch (msg->data[0]) {
> > + case HIOMAP_C_GET_INFO:
> > + if (msg->resp_size != 6) {
> > + prerror("%u: Unexpected response size: %u\n", msg->data[0],
> > + msg->resp_size);
> > + abort();
> > + }
> > + ctx->version = msg->data[2];
> > + if (ctx->version < 2) {
> > + prerror("Failed to negotiate protocol v2 or higher: %d\n",
> > + ctx->version);
> > + abort();
> > + }
> > + ctx->block_size_shift = msg->data[3];
> > + ctx->timeout = msg_get_u16(&msg->data[4]);
> > + break;
> > + case HIOMAP_C_GET_FLASH_INFO:
> > + if (msg->resp_size != 6) {
> > + prerror("%u: Unexpected response size: %u\n", msg->data[0],
> > + msg->resp_size);
> > + abort();
> > + }
> > + ctx->total_size = blocks_to_bytes(ctx, msg_get_u16(&msg->data[2]));
> > + ctx->erase_granule = blocks_to_bytes(ctx, msg_get_u16(&msg->data[4]));
> > + break;
> > + case HIOMAP_C_CREATE_READ_WINDOW:
> > + if (msg->resp_size != 8) {
> > + prerror("%u: Unexpected response size: %u\n", msg->data[0],
> > + msg->resp_size);
> > + abort();
> > + }
> > + ctx->read.lpc_addr = blocks_to_bytes(ctx, msg_get_u16(&msg->data[2]));
> > + ctx->read.size = blocks_to_bytes(ctx, msg_get_u16(&msg->data[4]));
> > + ctx->read.cur_pos = blocks_to_bytes(ctx, msg_get_u16(&msg->data[6]));
> > + ctx->read.open = true;
> > + ctx->write.open = false;
> > + break;
> > + case HIOMAP_C_CREATE_WRITE_WINDOW:
> > + if (msg->resp_size != 8) {
> > + prerror("%u: Unexpected response size: %u\n", msg->data[0],
> > + msg->resp_size);
> > + abort();
> > + }
> > + ctx->write.lpc_addr = blocks_to_bytes(ctx, msg_get_u16(&msg->data[2]));
> > + ctx->write.size = blocks_to_bytes(ctx, msg_get_u16(&msg->data[4]));
> > + ctx->write.cur_pos = blocks_to_bytes(ctx, msg_get_u16(&msg->data[6]));
> > + ctx->write.open = true;
> > + ctx->read.open = false;
> > + break;
By unifying the read and write members of struct ipmi_flash_data the two cases
above are also unified.
> > + case HIOMAP_C_CLOSE_WINDOW:
> > + ctx->write.open = false;
> > + ctx->read.open = false;
> > + break;
> > + case HIOMAP_C_MARK_DIRTY:
> > + case HIOMAP_C_FLUSH:
> > + case HIOMAP_C_ACK:
> > + case HIOMAP_C_ERASE:
> > + break;
> > + default:
> > + prlog(PR_WARNING, "Unimplemented command handler: %u\n",
> > + msg->data[0]);
> > + break;
> > + };
> > +}
> > +
> > +static bool hiomap_get_info(struct ipmi_flash_data *ctx)
> > +{
> > + RESULT_INIT(res, ctx);
> > + unsigned char req[3];
> > + struct ipmi_msg *msg;
> > +
> > + ctx->status = 0;
> > +
> > + /* Negotiate protocol version 2 */
>
> Seeing as we're doing a clean break here, a thought on
> forwards/backwards compatibility: could we split it between a major and
> minor version? Minor increments are compatible with old messages, major
> version break something?
>
> Any reason starting at 2?
I'll tackle this first and it should answer your first question:
We're starting at 2 because the protocol itself hasn't changed, just the
means of communicating it.
So getting back to your first question, we can't change the versioning scheme
because we haven't actually changed the protocol, just the transport.
>
> > + req[0] = HIOMAP_C_GET_INFO;
> > + req[1] = ctx->seq++;
> > + req[2] = 2;
> > +
> > + msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> > + bmc_platform->ipmi_oem_hiomap_cmd, ipmi_flash_cmd_cb,
> > + &res, req, sizeof(req), 6);
> > + ipmi_queue_msg_sync(msg);
> > +
> > + if (res.cc != IPMI_CC_NO_ERROR) {
> > + prerror("%s failed: %d\n", __func__, res.cc);
> > + return false;
> > + }
> > +
> > + lock(&ctx->lock);
> > + ctx->status |= HIOMAP_E_DAEMON_READY;
> > + unlock(&ctx->lock);
>
> The locking here doesn't look quite right? ctx->status updated with the
> ctx lock held but the sequence number isn't?
No, the locking in this instance is fine: "seq" does not need to be protected.
The lock is to protect state that can be mutated by the SEL handler, which is
limited to the "status" member and what in this patch is the "open"
member of struct lpc_window. In this patch "open" is not properly
protected, and is the reason for the note about concurrency in the cover
letter.
Now, I've already reworked the locking and struct lpc_window to fix the
behaviour.
> Although a bunch is done with the blocklevel lock held, perhaps we need
> to be explicit in documenting what's under what lock?
Yes, I'll document that a bit better. As mentioned above the lock in struct
ipmi_flash_data only protects a few small bits of state, the rest of the struct
is protected by the blocklevel lock as you point out.
>
> > +
> > + return true;
> > +}
> > +
> > +static bool hiomap_get_flash_info(struct ipmi_flash_data *ctx)
> > +{
> > + RESULT_INIT(res, ctx);
> > + unsigned char req[2];
> > + struct ipmi_msg *msg;
> > +
> > + req[0] = HIOMAP_C_GET_FLASH_INFO;
> > + req[1] = ctx->seq++;
> > + msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> > + bmc_platform->ipmi_oem_hiomap_cmd, ipmi_flash_cmd_cb,
> > + &res, req, sizeof(req), 2 + 2 + 2);
> > + ipmi_queue_msg_sync(msg);
> > +
> > + if (res.cc != IPMI_CC_NO_ERROR) {
> > + prerror("%s failed: %d\n", __func__, res.cc);
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static bool hiomap_window_move(struct ipmi_flash_data *ctx,
> > + struct lpc_window *win, uint8_t command,
> > + uint64_t pos, uint64_t len, uint64_t *size)
> > +{
> > + struct lpc_window *lwin;
> > + RESULT_INIT(res, ctx);
> > + unsigned char req[6];
> > + struct ipmi_msg *msg;
> > + bool is_read;
> > +
> > + if (hiomap_window_valid(win, pos, len)) {
> > + *size = len;
> > + return true;
> > + }
> > +
> > + req[0] = command;
> > + req[1] = ctx->seq++;
> > + msg_put_u16(&req[2], bytes_to_blocks(ctx, pos));
> > + msg_put_u16(&req[4], bytes_to_blocks(ctx, len));
>
> Part of me is thinking that we could better do this pattern with
>
> struct hiomap_window_move_cmd {
> le16 pos;
> le16 len;
> } __packed;
>
> and do the one memcpy(). Although I get that this comes from the
> mbox-flash.c implementation and I probably should have made this point
> then :)
>
> Maybe it's just something for future cleanup then. Forget I said
> anything :)
I think the packed struct is the right approach. I'll fix that up.
>
> > + ctx->read.open = false;
> > + ctx->write.open = false;
> > +
> > + msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> > + bmc_platform->ipmi_oem_hiomap_cmd, ipmi_flash_cmd_cb,
> > + &res, req, sizeof(req), 2 + 2 + 2 + 2);
> > + ipmi_queue_msg_sync(msg);
> > +
> > + if (res.cc != IPMI_CC_NO_ERROR) {
> > + prerror("%s failed: %d\n", __func__, res.cc);
> > + return false;
> > + }
> > +
> > + *size = len;
> > + /* Is length past the end of the window? */
> > + if ((pos + len) > (win->cur_pos + win->size))
> > + /* Adjust size to meet current window */
> > + *size = (win->cur_pos + win->size) - pos;
> > +
> > + if (len != 0 && *size == 0) {
> > + prerror("Invalid window properties: len: %llu, size: %llu\n",
> > + len, *size);
> > + abort();
> > + }
> > +
> > + is_read = command == HIOMAP_C_CREATE_READ_WINDOW;
> > + lwin = is_read ? &ctx->read : &ctx->write;
> > + prlog(PR_DEBUG, "Opened %s window from 0x%x for %u bytes at 0x%x\n",
> > + is_read ? "read" : "write", lwin->cur_pos, lwin->size,
> > + lwin->lpc_addr);
> > +
> > + return true;
> > +}
> > +
> > +static bool hiomap_mark_dirty(struct ipmi_flash_data *ctx, uint64_t offset,
> > + uint64_t size)
> > +{
> > + RESULT_INIT(res, ctx);
> > + unsigned char req[6];
> > + struct ipmi_msg *msg;
> > +
> > + assert(ctx->write.open);
> > +
> > + req[0] = HIOMAP_C_MARK_DIRTY;
> > + req[1] = ctx->seq++;
> > + msg_put_u16(&req[2], bytes_to_blocks(ctx, offset - ctx->write.cur_pos));
> > + msg_put_u16(&req[4], bytes_to_blocks(ctx, size));
> > +
> > + msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> > + bmc_platform->ipmi_oem_hiomap_cmd, ipmi_flash_cmd_cb,
> > + &res, req, sizeof(req), 2);
> > + ipmi_queue_msg_sync(msg);
> > +
> > + if (res.cc != IPMI_CC_NO_ERROR) {
> > + prerror("%s failed: %d\n", __func__, res.cc);
> > + return false;
> > + }
> > +
> > + prlog(PR_DEBUG, "Marked flash dirty at 0x%" PRIx64 " for %" PRIu64 "\n",
> > + offset, size);
> > +
> > + return true;
> > +}
> > +
> > +static bool hiomap_flush(struct ipmi_flash_data *ctx)
> > +{
> > + RESULT_INIT(res, ctx);
> > + unsigned char req[2];
> > + struct ipmi_msg *msg;
> > +
> > + assert(ctx->write.open);
> > +
> > + req[0] = HIOMAP_C_FLUSH;
> > + req[1] = ctx->seq++;
> > +
> > + msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> > + bmc_platform->ipmi_oem_hiomap_cmd, ipmi_flash_cmd_cb,
> > + &res, req, sizeof(req), 2);
> > + ipmi_queue_msg_sync(msg);
> > +
> > + if (res.cc != IPMI_CC_NO_ERROR) {
> > + prerror("%s failed: %d\n", __func__, res.cc);
> > + return false;
> > + }
> > +
> > + prlog(PR_DEBUG, "Flushed writes");
> > +
> > + return true;
> > +}
> > +
> > +static bool hiomap_ack(struct ipmi_flash_data *ctx, uint8_t ack)
> > +{
> > + RESULT_INIT(res, ctx);
> > + unsigned char req[3];
> > + struct ipmi_msg *msg;
> > +
> > + req[0] = HIOMAP_C_ACK;
> > + req[1] = ctx->seq++;
> > + req[2] = ack;
> > +
> > + msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> > + bmc_platform->ipmi_oem_hiomap_cmd, ipmi_flash_cmd_cb,
> > + &res, req, sizeof(req), 2);
> > + ipmi_queue_msg_sync(msg);
> > +
> > + if (res.cc != IPMI_CC_NO_ERROR) {
> > + prerror("%s failed: %d\n", __func__, res.cc);
> > + return false;
> > + }
> > +
> > + prlog(PR_DEBUG, "Acked events: 0x%x\n", ack);
> > +
> > + return true;
> > +}
> > +
> > +static bool hiomap_erase(struct ipmi_flash_data *ctx, uint64_t offset,
> > + uint64_t size)
> > +{
> > + RESULT_INIT(res, ctx);
> > + unsigned char req[6];
> > + struct ipmi_msg *msg;
> > +
> > + assert(ctx->write.open);
> > +
> > + req[0] = HIOMAP_C_ERASE;
> > + req[1] = ctx->seq++;
> > + msg_put_u16(&req[2], bytes_to_blocks(ctx, offset - ctx->write.cur_pos));
> > + msg_put_u16(&req[4], bytes_to_blocks(ctx, size));
> > +
> > + msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> > + bmc_platform->ipmi_oem_hiomap_cmd, ipmi_flash_cmd_cb,
> > + &res, req, sizeof(req), 2);
> > + ipmi_queue_msg_sync(msg);
> > +
> > + if (res.cc != IPMI_CC_NO_ERROR) {
> > + prerror("%s failed: %d\n", __func__, res.cc);
> > + return false;
> > + }
> > +
> > + prlog(PR_DEBUG, "Erased flash at 0x%" PRIx64 " for %" PRIu64 "\n",
> > + offset, size);
> > +
> > + return true;
> > +}
> > +
> > +static void hiomap_event(uint8_t events, void *context)
> > +{
> > + struct ipmi_flash_data *ctx = context;
> > +
> > + lock(&ctx->lock);
> > + ctx->status = events;
> > + ctx->update = true;
> > + unlock(&ctx->lock);
> > +
> > + /* FIXME: mutual exclusion */
>
> ?
See comments above about locking.
>
> > + if (events & (HIOMAP_E_BMC_REBOOT | HIOMAP_E_WINDOW_RESET)) {
> > + ctx->read.open = false;
> > + ctx->write.open = false;
> > + }
> > +}
> > +
> > +static int lpc_window_read(struct ipmi_flash_data *ctx, uint32_t pos,
> > + void *buf, uint32_t len)
> > +{
> > + uint32_t off = ctx->read.lpc_addr + (pos - ctx->read.cur_pos);
> > + int rc;
> > +
> > + prlog(PR_TRACE, "Reading at 0x%08x for 0x%08x offset: 0x%08x\n",
> > + pos, len, off);
>
> I feel we should have an assert that the read operation fits in the window?
ack
>
> > +
> > + while(len) {
> > + uint32_t chunk;
> > + uint32_t dat;
> > +
> > + /* XXX: make this read until it's aligned */
> > + if (len > 3 && !(off & 3)) {
> > + rc = lpc_read(OPAL_LPC_FW, off, &dat, 4);
> > + if (!rc)
> > + *(uint32_t *)buf = dat;
> > + chunk = 4;
> > + } else {
> > + rc = lpc_read(OPAL_LPC_FW, off, &dat, 1);
> > + if (!rc)
> > + *(uint8_t *)buf = dat;
> > + chunk = 1;
> > + }
> > + if (rc) {
> > + prlog(PR_ERR, "lpc_read failure %d to FW 0x%08x\n", rc, off);
> > + return rc;
> > + }
> > + len -= chunk;
> > + off += chunk;
> > + buf += chunk;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int lpc_window_write(struct ipmi_flash_data *ctx, uint32_t pos,
> > + const void *buf, uint32_t len)
> > +{
> > + uint32_t off = ctx->write.lpc_addr + (pos - ctx->write.cur_pos);
> > + int rc;
>
> Feeling maybe an assert to ensure the write fits in the window?
ack
>
> > + prlog(PR_TRACE, "Writing at 0x%08x for 0x%08x offset: 0x%08x\n",
> > + pos, len, off);
> > +
> > + while(len) {
> > + uint32_t chunk;
> > +
> > + if (len > 3 && !(off & 3)) {
> > + rc = lpc_write(OPAL_LPC_FW, off,
> > + *(uint32_t *)buf, 4);
> > + chunk = 4;
> > + } else {
> > + rc = lpc_write(OPAL_LPC_FW, off,
> > + *(uint8_t *)buf, 1);
> > + chunk = 1;
> > + }
> > + if (rc) {
> > + prlog(PR_ERR, "lpc_write failure %d to FW 0x%08x\n", rc, off);
> > + return rc;
> > + }
> > + len -= chunk;
> > + off += chunk;
> > + buf += chunk;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* Best-effort asynchronous event handling by blocklevel callbacks */
> > +static int ipmi_flash_handle_events(struct ipmi_flash_data *ctx)
> > +{
> > + uint8_t status;
> > + bool update;
> > +
> > + lock(&ctx->lock);
> > + status = ctx->status;
> > + update = ctx->update;
> > + if (update) {
> > + ctx->status &= ~HIOMAP_E_ACK_MASK;
> > + ctx->update = false;
> > + }
> > + unlock(&ctx->lock);
> > +
> > + if (!update)
> > + return 0;
> > +
> > + if (status & HIOMAP_E_FLASH_LOST) {
> > + prlog(PR_INFO, "Lost control of flash device\n");
> > + return FLASH_ERR_AGAIN;
> > + }
> > +
> > + if (!(status & HIOMAP_E_DAEMON_READY)) {
> > + prerror("Daemon not ready\n");
> > + return FLASH_ERR_DEVICE_GONE;
> > + }
> > +
> > + if (status & HIOMAP_E_BMC_REBOOT) {
> > + /* Avoid handling acks if we're renegotiating, they're stale */
> > + if (!hiomap_get_info(ctx)) {
> > + prerror("Failure to renegotiate after BMC reboot\n");
> > + return FLASH_ERR_DEVICE_GONE;
> > + }
> > +
> > + return 0;
> > + }
> > +
> > + if (status & HIOMAP_E_ACK_MASK) {
> > + if (!hiomap_ack(ctx, status & HIOMAP_E_ACK_MASK)) {
> > + prerror("Failed to ack events: 0x%x\n",
> > + status & HIOMAP_E_ACK_MASK);
> > + return FLASH_ERR_AGAIN;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ipmi_flash_read(struct blocklevel_device *bl, uint64_t pos,
> > + void *buf, uint64_t len)
> > +{
> > + struct ipmi_flash_data *ctx;
> > + uint64_t size;
> > + int rc = 0;
> > +
> > + /* LPC is only 32bit */
> > + if (pos > UINT_MAX || len > UINT_MAX)
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + ctx = container_of(bl, struct ipmi_flash_data, bl);
> > +
> > + rc = ipmi_flash_handle_events(ctx);
> > + if (rc)
> > + return rc;
> > +
> > + prlog(PR_TRACE, "Flash read at %#" PRIx64 " for %#" PRIx64 "\n", pos, len);
> > + while (len > 0) {
> > + /* Move window and get a new size to read */
> > + if (!hiomap_window_move(ctx, &ctx->read,
> > + MBOX_C_CREATE_READ_WINDOW, pos, len,
> > + &size))
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + /* Perform the read for this window */
> > + rc = lpc_window_read(ctx, pos, buf, size);
> > + if (rc)
> > + return rc;
> > +
> > + len -= size;
> > + pos += size;
> > + buf += size;
> > + /*
> > + * Ensure my window is still open, if it isn't we can't trust
> > + * what we read
>
> "what we just read" to clarify tense
Yep. I've already reworked this.
>
> > + */
> > +#if 0
> > + if (!is_valid(mbox_flash, &mbox_flash->read))
> > + return FLASH_ERR_AGAIN;
> > +#endif
>
> fix the #if0?
Hah, yeah I forgot I had this in there when I sent it. I've fixed the check.
>
> > + }
> > + return rc;
> > +
> > +}
> > +
> > +static int ipmi_flash_write(struct blocklevel_device *bl, uint64_t pos,
> > + const void *buf, uint64_t len)
> > +{
> > + struct ipmi_flash_data *ctx;
> > + uint64_t size;
> > + int rc = 0;
> > +
> > + /* LPC is only 32bit */
> > + if (pos > UINT_MAX || len > UINT_MAX)
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + ctx = container_of(bl, struct ipmi_flash_data, bl);
> > +
> > + rc = ipmi_flash_handle_events(ctx);
> > + if (rc)
> > + return rc;
> > +
> > + prlog(PR_TRACE, "Flash write at %#" PRIx64 " for %#" PRIx64 "\n", pos, len);
> > + while (len > 0) {
> > + /* Move window and get a new size to read */
> > + if (!hiomap_window_move(ctx, &ctx->write,
> > + MBOX_C_CREATE_WRITE_WINDOW, pos, len,
> > + &size)) {
> > + return FLASH_ERR_PARM_ERROR;
> > + }
> > +
> > + /* Perform the write for this window */
> > + rc = lpc_window_write(ctx, pos, buf, size);
> > + if (rc)
> > + return rc;
> > +
> > + if (!hiomap_mark_dirty(ctx, pos, size)) {
> > + return FLASH_ERR_PARM_ERROR;
> > + }
> > +
> > + /* ARJ: This comment is incorrect, the BMC must flush any dirty
> > + * regions on close
> > + */
> > + /*
> > + * Must flush here as changing the window contents
> > + * without flushing entitles the BMC to throw away the
> > + * data. Unlike the read case there isn't a need to explicitly
> > + * validate the window, the flush command will fail if the
> > + * window was compromised.
> > + */
>
> Fix the comment and make it explicitly documented what the expectations are?
Yeah, that was more of a note than anything. It's tempting to just YOLO delete the
comment and the code, but then again that might be a bit much YOLO. On the flip-
side leaving would mean we wind up with a comment like "To be sure to be sure".
I'm feeling quite indecisive about it all.
>
> > + if (!hiomap_flush(ctx)) {
> > + return FLASH_ERR_PARM_ERROR;
> > + }
> > +
> > + len -= size;
> > + pos += size;
> > + buf += size;
> > + }
> > + return rc;
> > +
> > +}
> > +
> > +static int ipmi_flash_erase(struct blocklevel_device *bl, uint64_t pos,
> > + uint64_t len)
> > +{
> > + struct ipmi_flash_data *ctx;
> > + int rc;
> > +
> > + /* LPC is only 32bit */
> > + if (pos > UINT_MAX || len > UINT_MAX)
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + ctx = container_of(bl, struct ipmi_flash_data, bl);
> > +
> > + rc = ipmi_flash_handle_events(ctx);
> > + if (rc)
> > + return rc;
> > +
> > + prlog(PR_TRACE, "Flash erase at 0x%08x for 0x%08x\n", (u32) pos, (u32) len);
> > + while (len > 0) {
> > + uint64_t size;
> > +
> > + /* Move window and get a new size to erase */
> > + if (!hiomap_window_move(ctx, &ctx->write,
> > + MBOX_C_CREATE_WRITE_WINDOW, pos, len,
> > + &size))
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + if (!hiomap_erase(ctx, pos, size))
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + /*
> > + * Flush directly, don't mark that region dirty otherwise it
> > + * isn't clear if a write happened there or not
> > + */
> > +
> > + if (!hiomap_flush(ctx))
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + len -= size;
> > + pos += size;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ipmi_flash_get_flash_info(struct blocklevel_device *bl,
> > + const char **name, uint64_t *total_size,
> > + uint32_t *erase_granule)
> > +{
> > + struct ipmi_flash_data *ctx;
> > + int rc;
> > +
> > + ctx = container_of(bl, struct ipmi_flash_data, bl);
> > +
> > + rc = ipmi_flash_handle_events(ctx);
> > + if (rc)
> > + return rc;
> > +
> > + if (!hiomap_get_flash_info(ctx)) {
> > + abort();
> > + }
> > +
> > + ctx->bl.erase_mask = ctx->erase_granule - 1;
> > +
> > + if (name)
> > + *name = NULL;
> > + if (total_size)
> > + *total_size = ctx->total_size;
> > + if (erase_granule)
> > + *erase_granule = ctx->erase_granule;
> > +
> > + return 0;
> > +}
> > +
> > +int ipmi_flash_init(struct blocklevel_device **bl)
> > +{
> > + struct ipmi_flash_data *ctx;
> > + int rc;
> > +
> > + if (!bmc_platform->ipmi_oem_hiomap_cmd)
> > + /* FIXME: Find a better error code */
> > + return FLASH_ERR_DEVICE_GONE;
> > +
> > + if (!bl)
> > + return FLASH_ERR_PARM_ERROR;
> > +
> > + *bl = NULL;
> > +
> > + ctx = zalloc(sizeof(struct ipmi_flash_data));
> > + if (!ctx)
> > + return FLASH_ERR_MALLOC_FAILED;
> > +
> > + init_lock(&ctx->lock);
> > +
> > + ctx->bl.read = &ipmi_flash_read;
> > + ctx->bl.write = &ipmi_flash_write;
> > + ctx->bl.erase = &ipmi_flash_erase;
> > + ctx->bl.get_info = &ipmi_flash_get_flash_info;
> > +
> > + rc = ipmi_sel_register(CMD_OP_HIOMAP_EVENT, hiomap_event, ctx);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* Do a LPC2AHB protocol GET_INFO */
> > + if (!hiomap_get_info(ctx)) {
> > + prerror("Failed to get hiomap parameters\n");
> > + return FLASH_ERR_DEVICE_GONE;
> > + }
> > +
> > + if (!hiomap_get_flash_info(ctx)) {
> > + prerror("Failed to get flash parameters\n");
> > + return FLASH_ERR_DEVICE_GONE;
> > + }
> > +
> > + prlog(PR_NOTICE, "Negotiated hiomap protocol v%u\n", ctx->version);
> > + prlog(PR_NOTICE, "Block size is %uKiB\n",
> > + 1 << (ctx->block_size_shift - 10));
> > + prlog(PR_NOTICE, "BMC suggested flash timeout of %us\n", ctx->timeout);
> > + prlog(PR_NOTICE, "Flash size is %uMiB\n", ctx->total_size >> 20);
> > + prlog(PR_NOTICE, "Erase granule size is %uKiB\n",
> > + ctx->erase_granule >> 10);
> > +
> > + ctx->bl.keep_alive = 0;
> > +
> > + *bl = &(ctx->bl);
> > +
> > + return 0;
> > +}
> > +
> > +void ipmi_flash_exit(struct blocklevel_device *bl)
> > +{
> > + struct ipmi_flash_data *ctx;
> > + if (bl) {
> > + ctx = container_of(bl, struct ipmi_flash_data, bl);
> > + free(ctx);
> > + }
> > +}
> > diff --git a/libflash/ipmi-flash.h b/libflash/ipmi-flash.h
> > new file mode 100644
> > index 000000000000..4bec2ad8a092
> > --- /dev/null
> > +++ b/libflash/ipmi-flash.h
> > @@ -0,0 +1,29 @@
> > +/* Copyright 2018 IBM Corp.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > + * implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef __LIBFLASH_IPMI_FLASH_H
> > +#define __LIBFLASH_IPMI_FLASH_H
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +
> > +#include "blocklevel.h"
> > +
> > +int ipmi_flash_lock(struct blocklevel_device *bl, uint64_t pos, uint64_t len);
> > +int ipmi_flash_init(struct blocklevel_device **bl);
> > +void ipmi_flash_exit(struct blocklevel_device *bl);
> > +
> > +#endif /* __LIBFLASH_IPMI_FLASH_H */
> > diff --git a/platforms/astbmc/common.c b/platforms/astbmc/common.c
> > index 2c32db7295af..23550ef37a67 100644
> > --- a/platforms/astbmc/common.c
> > +++ b/platforms/astbmc/common.c
> > @@ -469,4 +469,5 @@ const struct bmc_platform astbmc_ami = {
> > const struct bmc_platform astbmc_openbmc = {
> > .name = "OpenBMC",
> > .ipmi_oem_partial_add_esel = IPMI_CODE(0x3a, 0xf0),
> > + .ipmi_oem_hiomap_cmd = IPMI_CODE(0x3a, 0x5a),
> > };
> > --
> > 2.17.1
> >
>
> --
> Stewart Smith
> OPAL Architect, IBM.
>
Thanks for taking a look!
Andrew
More information about the Skiboot
mailing list