[Skiboot] [PATCH v3 3/4] libflash: blocklevel backend for MBOX flash access
Suraj Jitindar Singh
sjitindarsingh at gmail.com
Mon Feb 6 18:44:49 AEDT 2017
On Thu, 2017-02-02 at 17:54 +1100, Cyril Bur wrote:
> The use MBOX protocol to request flash access from the BMC. Then
> read/write to the 'flash' through the LPC bus.
>
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> libflash/Makefile.inc | 2 +-
> libflash/mbox-flash.c | 562
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> libflash/mbox-flash.h | 24 +++
> 3 files changed, 587 insertions(+), 1 deletion(-)
> create mode 100644 libflash/mbox-flash.c
> create mode 100644 libflash/mbox-flash.h
>
> diff --git a/libflash/Makefile.inc b/libflash/Makefile.inc
> index 4db02a1f..ea64eb46 100644
> --- a/libflash/Makefile.inc
> +++ b/libflash/Makefile.inc
> @@ -1,4 +1,4 @@
> -LIBFLASH_SRCS = libflash.c libffs.c ecc.c blocklevel.c
> +LIBFLASH_SRCS = libflash.c libffs.c ecc.c blocklevel.c mbox-flash.c
> LIBFLASH_OBJS = $(LIBFLASH_SRCS:%.c=%.o)
>
> SUBDIRS += libflash
> diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c
> new file mode 100644
> index 00000000..69456bcb
> --- /dev/null
> +++ b/libflash/mbox-flash.c
> @@ -0,0 +1,562 @@
> +/* Copyright 2017 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) "MBOX-FLASH: " fmt
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <skiboot.h>
> +#include <timebase.h>
> +#include <timer.h>
> +#include <libflash/libflash.h>
> +#include <libflash/mbox-flash.h>
> +#include <lpc.h>
> +#include <lpc-mbox.h>
> +
> +#include <ccan/container_of/container_of.h>
> +
> +#ifndef __SKIBOOT__
> +#error "This libflash backend must be compiled with skiboot"
> +#endif
> +
> +#define MBOX_DEFAULT_TIMEOUT 30
> +
> +struct lpc_window {
> + uint32_t lpc;
Can this be addr or lpc_addr?
> + uint32_t pos;
Can this be flash_pos or flash_addr or cur_pos?
> + uint32_t size;
> + bool open;
> +};
> +
> +struct mbox_flash_data {
> + uint32_t shift;
> + struct lpc_window read;
> + struct lpc_window write;
> + struct blocklevel_device bl;
> + uint32_t total_size;
> + uint32_t erase_granule;
> + int rc;
> + bool busy;
> + struct bmc_mbox_msg msg_mem;
> +};
> +
> +static uint16_t get_u16(uint8_t *ptr)
> +{
> + return le16_to_cpu(*(uint16_t *)ptr);
> +}
> +
> +static void put_u16(uint8_t *ptr, uint16_t val)
> +{
> + uint16_t valt = cpu_to_le16(val);
"valt"???
Only thing I can thing of is you meant vault? How about tmp or tmp_val
or i or something else?
> + memcpy(ptr, &valt, sizeof(val));
> +}
> +
> +static uint32_t get_u32(uint8_t *ptr)
> +{
> + return le32_to_cpu(*(uint32_t *)ptr);
> +}
> +
> +static void put_u32(uint8_t *ptr, uint32_t val)
> +{
> + uint32_t valt = cpu_to_le32(val);
see above
> + memcpy(ptr, &valt, sizeof(val));
> +}
> +
I would prefer get_msg_zeroed or something
> +static struct bmc_mbox_msg *get_msg_memory(struct mbox_flash_data
> *lpc)
> +{
> + /*
> + * Yes this causes *slow*.
Please reword for the better use english of
> + * This file and lpc-mbox have far greater slow points,
> zeroed
> + * data regs are VERY useful for debugging. Think twice if
> this is
> + * really the performance optimisation you want to make.
> + */
> + memset(&lpc->msg_mem, 0, sizeof(lpc->msg_mem));
> + return &lpc->msg_mem;
> +}
> +
> +static void free_msg_memory(struct bmc_mbox_msg *mem __unused)
> +{
> + /* Allocation is so simple this isn't required */
Can we remove this then?
> +}
> +
> +static bool wait_for_bmc(struct mbox_flash_data *lpc, unsigned int
> timeout_sec)
> +{
> + unsigned long last = 1, start = tb_to_secs(mftb());
> + prlog(PR_DEBUG, "Waiting for BMC\n");
> + while (lpc->busy && timeout_sec) {
> + long now = tb_to_secs(mftb());
> + if (now - start > last) {
> + timeout_sec--;
> + last = now - start;
> + if (last < timeout_sec / 2)
> + prlog(PR_TRACE, "Been waiting for
> the BMC for %lu secs\n", last);
> + else
> + prlog(PR_ERR, "BMC NOT RESPONDING
> %lu second wait\n", last);
> + }
> + /*
> + * Both functions are important.
> + * Well time_wait_ms() relaxes the spin... so... its
> nice
> + */
> + time_wait_ms(MBOX_DEFAULT_POLL_MS);
> + check_timers(false);
> + asm volatile ("" ::: "memory");
> + }
> +
> + return lpc->busy;
> +}
> +
> +static int lpc_window_read(struct mbox_flash_data *lpc, uint32_t
> pos, void *buf, uint32_t len)
> +{
> + int rc;
> + uint32_t off = lpc->read.lpc + (pos - lpc->read.pos);
> +
> + prlog(PR_TRACE, "Reading at 0x%08x for 0x%08x offset:
> 0x%08x\n",
> + pos, len, off);
> +
> + while(len) {
> + uint32_t chunk;
> + uint32_t dat;
> +
> + /* Choose access size */
> + if (len > 3 && !(off & 3)) {
> + rc = lpc_read(OPAL_LPC_FW, off, &dat, 4);
> + if (!rc)
> + *(uint32_t *)buf = dat;
> + chunk = 4;
You can only read/write 4 bytes at a time?! *ouch* Might want to note
this in a comment.
> + } 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 mbox_flash_data *lpc, uint32_t
> pos, const void *buf, uint32_t len)
> +{
> + uint32_t off = lpc->write.lpc + (pos - lpc->write.pos);
> + int rc;
> +
> +
> + 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;
> +}
> +
> +static void write_flush_cb(struct bmc_mbox_msg *msg)
> +{
> + struct mbox_flash_data *lpc;
> +
> + lpc = msg->priv;
> +
> + prlog(PR_DEBUG, "WRITE_FLUSH CB, BMC OK\n");
> +
> + if (msg->response != MBOX_R_SUCCESS) {
> + prlog(PR_ERR, "Bad response code from BMC %d\n",
> msg->response);
> + lpc->rc = msg->response;
> + goto out;
> + }
> +
> + lpc->rc = 0;
> +
> +out:
> + lpc->busy = false;
> +}
> +
> +static void write_cb(struct bmc_mbox_msg *msg)
> +{
> + struct mbox_flash_data *lpc;
> +
> + lpc = msg->priv;
> +
> + prlog(PR_DEBUG, "CREATE_WRITE_WINDOW CB, BMC OK\n");
> +
> + if (msg->response != MBOX_R_SUCCESS) {
> + prlog(PR_ERR, "Bad response code from BMC %d\n",
> msg->response);
> + lpc->rc = msg->response;
> + goto out;
> + }
> +
> + lpc->write.lpc = get_u16(&msg->data[0]) << lpc->shift;
> + lpc->write.open = true;
> + lpc->read.open = false;
> +
> + lpc->rc = 0;
> +
> +out:
> + lpc->busy = false;
> +}
> +
> +static int mbox_flash_write(struct blocklevel_device *bl, uint64_t
> pos,
> + const void *buf, uint64_t len)
> +{
> + struct mbox_flash_data *lpc;
> + struct bmc_mbox_msg *msg;
> + int rc;
> +
> + /* LPC is only 32bit */
> + if (pos > UINT_MAX || len > UINT_MAX)
> + return FLASH_ERR_PARM_ERROR;
> +
> + lpc = container_of(bl, struct mbox_flash_data, bl);
> + msg = get_msg_memory(lpc);
> + if (!msg)
> + return FLASH_ERR_MALLOC_FAILED;
> +
> + prlog(PR_TRACE, "Flash write at 0x%08llx for 0x%08llx\n",
> pos, len);
> + if (!lpc->write.open || pos < lpc->write.pos || pos + len >
> lpc->write.pos + lpc->write.size) {
See comments in read function below, I believe this will suffer from
the same problems.
> + prlog(PR_INFO, "Adjusting the write window\n");
> +
> + lpc->write.pos = pos & ~(lpc->shift - 1);
> + msg->priv = lpc;
> + msg->command = MBOX_C_CREATE_WRITE_WINDOW;
> + msg->callback = &write_cb;
> + put_u16(&msg->data[0], pos >> lpc->shift);
> +
> + lpc->busy = true;
> + rc = bmc_mbox_enqueue(msg);
> + if (rc) {
> + prlog(PR_ERR, "Failed to enqueue/send BMC
> MBOX message\n");
> + rc = FLASH_ERR_MALLOC_FAILED; /* Not
> necessarily the reason... */
> + goto out;
> + }
> +
> + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> + prlog(PR_ERR, "Timeout waiting for BMC\n");
> + rc = FLASH_ERR_CTRL_TIMEOUT;
> + return rc;
> + }
> +
> + if (lpc->rc) {
> + rc = lpc->rc;
> + goto out;
> + }
> + }
> +
> + rc = lpc_window_write(lpc, pos, buf, len);
> + if (rc)
> + return rc;
> +
Can the whole *flush* business be a separate function please?
> + msg->priv = lpc;
> + msg->command = MBOX_C_WRITE_FLUSH;
> + msg->callback = &write_flush_cb;
> + put_u16(&msg->data[0], pos >> lpc->shift);
> + put_u32(&msg->data[2], len);
> +
> + lpc->busy = true;
> + rc = bmc_mbox_enqueue(msg);
> + if (rc) {
> + prlog(PR_ERR, "Failed to enqueue/send BMC MBOX
> message\n");
> + rc = FLASH_ERR_MALLOC_FAILED; /* Not necessarily the
> reason... */
> + goto out;
> + }
> +
> + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> + prlog(PR_ERR, "Timeout waiting for BMC\n");
> + rc = FLASH_ERR_CTRL_TIMEOUT;
> + goto out;
> + }
> +
> + rc = lpc->rc;
> +
> +out:
> + free_msg_memory(msg);
> + return rc;
> +}
> +
> +static void read_cb(struct bmc_mbox_msg *msg)
> +{
> + struct mbox_flash_data *lpc;
> +
> + lpc = msg->priv;
> +
> + prlog(PR_DEBUG, "CREATE_READ_WINDOW CB, BMC OK\n");
> +
> + if (msg->response != MBOX_R_SUCCESS) {
> + prlog(PR_ERR, "Bad response code from BMC %d\n",
> msg->response);
> + lpc->rc = msg->response;
> + goto out;
> + }
> +
> + lpc->read.lpc = get_u16(&msg->data[0]) << lpc->shift;
> + lpc->read.open = true;
> + lpc->write.open = false;
> +
> + lpc->rc = 0;
> +out:
> + lpc->busy = false;
> +}
> +
> +static int mbox_flash_read(struct blocklevel_device *bl, uint64_t
> pos, void *buf, uint64_t len)
> +{
> + struct mbox_flash_data *lpc;
> + int rc = 0;
> +
> + /* LPC is only 32bit */
> + if (pos > UINT_MAX || len > UINT_MAX)
> + return FLASH_ERR_PARM_ERROR;
> +
> + lpc = container_of(bl, struct mbox_flash_data, bl);
> +
> + prlog(PR_TRACE, "Flash read at 0x%08llx for 0x%08llx\n",
> pos, len);
> + if (!lpc->read.open || pos < lpc->read.pos || pos + len >
> lpc->read.pos + lpc->read.size) {
What if len > lpc->read.size as far as I can tell this will currently
just keep reading past the end of the window into unknown lpc address
space. Shouldn't you check, if len < lpc->read.size then current code
is fine. Otherwise you need to open a window, read lpc->read.size bytes
into buffer, len -= lpc->read.size && pos+=lpc->read.size, close window
and repeat while len > 0. You also probably want to check total size to
be sure your not trying to open a window past the end of flash as well
I don't think anything currently stops you from doing this?! Maybe
return end of file if you try read past the end of flash?
Alternatively can you just return the amount you did read (so up to a
max of lpc->read.size e.g. len = len > lpc->read.size ? lpc->read.size
: len;) and the caller will auto retry with an incremented pos and a
decremented len link the linux read/write calls?
> + struct bmc_mbox_msg *msg = get_msg_memory(lpc);
> + if (!msg)
> + return FLASH_ERR_MALLOC_FAILED;
> +
> + prlog(PR_INFO, "Adjusting the read window\n");
> +
> + lpc->read.pos = pos & ~(lpc->shift - 1);
> + msg->priv = lpc;
> + msg->command = MBOX_C_CREATE_READ_WINDOW;
> + msg->callback = &read_cb;
> + put_u16(&msg->data[0], pos >> lpc->shift);
> +
> + lpc->busy = true;
> + rc = bmc_mbox_enqueue(msg);
> + if (rc) {
> + prlog(PR_ERR, "Failed to enqueue/send BMC
> MBOX message\n");
> + goto out;
> + }
> +
> + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> + prlog(PR_ERR, "Timeout waiting for BMC\n");
> + rc = FLASH_ERR_CTRL_TIMEOUT;
> + goto out;
> + }
> +
> + rc = lpc->rc;
> +out:
> + free_msg_memory(msg);
> + }
> +
> + return rc ? rc : lpc_window_read(lpc, pos, buf, len);
> +}
> +
> +static void get_info_cb(struct bmc_mbox_msg *msg)
> +{
> + struct mbox_flash_data *lpc;
> +
> + prlog(PR_DEBUG, "GET_FLASH_INFO CB, BMC OK\n");
> +
> + lpc = msg->priv;
> +
> + if (msg->response != MBOX_R_SUCCESS) {
> + prlog(PR_ERR, "Bad response code from BMC %d\n",
> msg->response);
> + lpc->rc = msg->response;
> + goto out;
> + }
> +
> + lpc->total_size = get_u32(&msg->data[0]);
> + lpc->erase_granule = get_u32(&msg->data[4]);
> +
> + lpc->rc = 0;
> +out:
> + lpc->busy = false;
> +}
> +
> +static int mbox_flash_get_info(struct blocklevel_device *bl, const
> char **name,
> + uint64_t *total_size, uint32_t *erase_granule)
> +{
> + struct mbox_flash_data *lpc;
> + struct bmc_mbox_msg *msg;
> + int rc;
> +
> + lpc = container_of(bl, struct mbox_flash_data, bl);
> + msg = get_msg_memory(lpc);
> + if (!msg)
> + return FLASH_ERR_MALLOC_FAILED;
Can this really happen?
> +
> + msg->command = MBOX_C_GET_FLASH_INFO;
> + msg->callback = &get_info_cb;
> + msg->priv = lpc;
I guess you want to be sure, but do you have to set this everytime?
> + msg->data[0] = 1; /* V1, do better */
Should the commands be defined somewhere? I don't like this *magic* 1.
> +
> + /*
> + * We want to avoid runtime mallocs in skiboot. The expected
> + * behavour to uses of libflash is that one can free() the
> memory
> + * returned.
> + * NULL will do for now.
> + */
> + if (name)
> + *name = NULL;
> +
> + lpc->busy = true;
> + rc = bmc_mbox_enqueue(msg);
> + if (rc) {
> + prlog(PR_ERR, "Failed to enqueue/send BMC MBOX
> message\n");
> + goto out;
> + }
> +
> + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> + prlog(PR_ERR, "Timeout waiting for BMC\n");
> + rc = FLASH_ERR_CTRL_TIMEOUT;
> + goto out;
> + }
> +
> + rc = lpc->rc;
> + if (rc)
> + goto out;
> +
> + lpc->bl.erase_mask = lpc->erase_granule - 1;
> +
> + if (total_size)
> + *total_size = lpc->total_size;
> + if (erase_granule)
> + *erase_granule = lpc->erase_granule;
> +
> +out:
> + free_msg_memory(msg);
> + return rc;
> +}
> +
> +static int mbox_flash_erase(struct blocklevel_device *bl __unused,
> uint64_t pos __unused, uint64_t len __unused)
> +{
> + /*
> + * We can probably get away with doing nothing.
> + * TODO: Rethink this, causes interesting behaviour in
> pflash.
> + * Users do expect pflash -{e,E} to do something
> + */
What is the expected behaviour of erase, should it be erasing the
flash? If so shouldn't this write zeros, dirty the blocks and request a
flush.
> + return 0;
> +}
> +
> +static void init_cb(struct bmc_mbox_msg *msg)
> +{
> + struct mbox_flash_data *lpc;
> +
> + prlog(PR_DEBUG, "INIT CB, BMC OK\n");
> +
> + lpc = msg->priv;
> +
> + if (msg->response != MBOX_R_SUCCESS) {
> + prlog(PR_ERR, "Bad response code from BMC %d\n",
> msg->response);
> + lpc->rc = msg->response;
> + goto out;
> + }
> +
> + lpc->read.size = get_u16(&msg->data[1]) << lpc->shift;
> + lpc->write.size = get_u16(&msg->data[3]) << lpc->shift;
> +
> + lpc->rc = 0;
> +out:
> + lpc->busy = false;
> +}
> +
> +int mbox_flash_init(struct blocklevel_device **bl)
> +{
> + struct mbox_flash_data *lpc;
As much as it's vague I'd prefer state to lpc, this isn't really
**only** the lpc as it holds other state information. How about
mbox_state?
(Update this everywhere)
> + struct bmc_mbox_msg *msg;
> + int rc;
> +
> + if (!bl)
> + return FLASH_ERR_PARM_ERROR;
> +
> + *bl = NULL;
> +
> + lpc = zalloc(sizeof(struct mbox_flash_data));
> + if (!lpc)
> + return FLASH_ERR_MALLOC_FAILED;
> +
> + /* For V1 of the protocol this is fixed. This could change
> */
> + lpc->shift = 12;
> +
> + msg = get_msg_memory(lpc);
> + if (!msg) {
As far as I can tell, this check is unnecessary, this can only fail if
the whole lpc allocation failed which would be caught above?!
> + rc = FLASH_ERR_MALLOC_FAILED;
> + goto out;
> + }
> +
> + msg->command = MBOX_C_GET_MBOX_INFO;
> + msg->callback = &init_cb;
Can the callback member be a pointer to an array of function pointers
indexed by the command? Then the call back wouldn't need to be set
everytime.
e.g.
typedef void (* msg_callback_t)(struct bmc_mbox_msg *msg)
struct bmc_mbox_msg {
<snip>
static msg_callback_t callbacks[NUM_CMDS]
<snip>
}
static msg_callback_t msg_callbacks[] = {
<snip>
&init_cb,
<snip>
}
msg->callbacks = msg_callbacks;
then in the driver you call:
msg->callbacks[CMD](msg);
Just an idea.
> + msg->priv = lpc;
> + msg->data[0] = 1; /* V1, do better */
> +
> + lpc->busy = true;
> + rc = bmc_mbox_enqueue(msg);
> + if (rc) {
> + prlog(PR_ERR, "Failed to enqueue/send BMC MBOX
> message\n");
> + goto out_msg;
> + }
> +
> + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> + prlog(PR_ERR, "Timeout waiting for BMC\n");
> + rc = FLASH_ERR_CTRL_TIMEOUT;
> + goto out_msg;
> + }
> +
> + rc = lpc->rc;
> + if (rc)
> + goto out_msg;
> +
> + lpc->bl.keep_alive = 0;
> + lpc->bl.read = &mbox_flash_read;
> + lpc->bl.write = &mbox_flash_write;
> + lpc->bl.erase = &mbox_flash_erase;
> + lpc->bl.get_info = &mbox_flash_get_info;
> +
> + *bl = &(lpc->bl);
> + return 0;
> +
> +out_msg:
> + free_msg_memory(msg);
> +out:
> + free(lpc);
> + return rc;
> +}
> +
> +void mbox_flash_exit(struct blocklevel_device *bl)
> +{
> + struct mbox_flash_data *lpc;
> + if (bl) {
> + lpc = container_of(bl, struct mbox_flash_data, bl);
> + free(lpc);
> + }
> +}
> diff --git a/libflash/mbox-flash.h b/libflash/mbox-flash.h
> new file mode 100644
> index 00000000..cd587d4c
> --- /dev/null
> +++ b/libflash/mbox-flash.h
> @@ -0,0 +1,24 @@
> +/* Copyright 2017 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_MBOX_FLASH_H
> +#define __LIBFLASH_MBOX_FLASH_H
> +
> +int mbox_flash_init(struct blocklevel_device **bl);
> +void mbox_flash_exit(struct blocklevel_device *bl);
> +#endif /* __LIBFLASH_MBOX_FLASH_H */
> +
> +
More information about the Skiboot
mailing list