From: Tom Rix <trix@redhat.com>
To: Russ Weight <russell.h.weight@intel.com>,
mdf@kernel.org, lee.jones@linaro.org, linux-fpga@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: lgoncalv@redhat.com, yilun.xu@intel.com, hao.wu@intel.com,
matthew.gerlach@intel.com
Subject: Re: [PATCH v5 5/6] fpga: m10bmc-sec: add max10 secure update functions
Date: Sun, 15 Nov 2020 06:17:03 -0800 [thread overview]
Message-ID: <3c531b5d-0620-5239-06a7-02a01381c436@redhat.com> (raw)
In-Reply-To: <20201114005559.90860-6-russell.h.weight@intel.com>
On 11/13/20 4:55 PM, Russ Weight wrote:
> Extend the MAX10 BMC Secure Update driver to include
> the functions that enable secure updates of BMC images,
> FPGA images, etc.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v5:
> - No change
> v4:
> - No change
> v3:
> - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
> - Changed "MAX10 BMC Secure Engine driver" to "MAX10 BMC Secure Update
> driver"
> - Removed wrapper functions (m10bmc_raw_*, m10bmc_sys_*). The
> underlying functions are now called directly.
> - Changed calling functions of functions that return "enum fpga_sec_err"
> to check for (ret != FPGA_SEC_ERR_NONE) instead of (ret)
> v2:
> - Reworked the rsu_start_done() function to make it more readable
> - Reworked while-loop condition/content in rsu_prog_ready()
> - Minor code cleanup per review comments
> - Added a comment to the m10bmc_sec_poll_complete() function to
> explain the context (could take 30+ minutes to complete).
> - Added m10bmc_ prefix to functions in m10bmc_iops structure
> - Moved MAX10 BMC address and function definitions to a separate
> patch.
> ---
> drivers/fpga/intel-m10-bmc-secure.c | 305 +++++++++++++++++++++++++++-
> 1 file changed, 304 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
> index 689da5bc6461..4fa8a2256088 100644
> --- a/drivers/fpga/intel-m10-bmc-secure.c
> +++ b/drivers/fpga/intel-m10-bmc-secure.c
> @@ -174,7 +174,310 @@ static const struct attribute_group *m10bmc_sec_attr_groups[] = {
> NULL,
> };
>
> -static const struct fpga_sec_mgr_ops m10bmc_sops = { };
> +static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
> +{
> + u32 auth_result;
> +
> + dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
> +
> + if (!m10bmc_sys_read(sec->m10bmc, M10BMC_AUTH_RESULT, &auth_result))
> + dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
> +}
> +
> +static enum fpga_sec_err rsu_check_idle(struct m10bmc_sec *sec)
> +{
> + u32 doorbell;
> + int ret;
> +
> + ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
> + if (ret)
> + return FPGA_SEC_ERR_RW_ERROR;
> +
> + if (rsu_prog(doorbell) != RSU_PROG_IDLE &&
> + rsu_prog(doorbell) != RSU_PROG_RSU_DONE) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_BUSY;
> + }
> +
> + return FPGA_SEC_ERR_NONE;
> +}
> +
> +static inline bool rsu_start_done(u32 doorbell)
> +{
> + u32 status, progress;
> +
> + if (doorbell & DRBL_RSU_REQUEST)
> + return false;
> +
> + status = rsu_stat(doorbell);
> + if (status == RSU_STAT_ERASE_FAIL || status == RSU_STAT_WEAROUT)
> + return true;
> +
> + progress = rsu_prog(doorbell);
> + if (progress != RSU_PROG_IDLE && progress != RSU_PROG_RSU_DONE)
> + return true;
> +
> + return false;
> +}
> +
> +static enum fpga_sec_err rsu_update_init(struct m10bmc_sec *sec)
> +{
> + u32 doorbell, status;
> + int ret;
> +
> + ret = regmap_update_bits(sec->m10bmc->regmap,
> + M10BMC_SYS_BASE + M10BMC_DOORBELL,
> + DRBL_RSU_REQUEST | DRBL_HOST_STATUS,
> + DRBL_RSU_REQUEST |
> + FIELD_PREP(DRBL_HOST_STATUS,
> + HOST_STATUS_IDLE));
> + if (ret)
> + return FPGA_SEC_ERR_RW_ERROR;
> +
> + ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
> + M10BMC_SYS_BASE + M10BMC_DOORBELL,
> + doorbell,
> + rsu_start_done(doorbell),
> + NIOS_HANDSHAKE_INTERVAL_US,
> + NIOS_HANDSHAKE_TIMEOUT_US);
> +
> + if (ret == -ETIMEDOUT) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_TIMEOUT;
> + } else if (ret) {
> + return FPGA_SEC_ERR_RW_ERROR;
> + }
> +
> + status = rsu_stat(doorbell);
> + if (status == RSU_STAT_WEAROUT) {
> + dev_warn(sec->dev, "Excessive flash update count detected\n");
If wear out is going to flood logs, move this to a warn once.
Maybe make rsu_stat a function.
> + return FPGA_SEC_ERR_WEAROUT;
> + } else if (status == RSU_STAT_ERASE_FAIL) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_HW_ERROR;
> + }
> +
> + return FPGA_SEC_ERR_NONE;
> +}
> +
> +static enum fpga_sec_err rsu_prog_ready(struct m10bmc_sec *sec)
> +{
> + unsigned long poll_timeout;
> + u32 doorbell, progress;
> + int ret;
> +
> + ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
> + if (ret)
> + return FPGA_SEC_ERR_RW_ERROR;
> +
> + poll_timeout = jiffies + msecs_to_jiffies(RSU_PREP_TIMEOUT_MS);
> + while (rsu_prog(doorbell) == RSU_PROG_PREPARE) {
> + msleep(RSU_PREP_INTERVAL_MS);
> + if (time_after(jiffies, poll_timeout))
> + break;
> +
> + ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
> + if (ret)
> + return FPGA_SEC_ERR_RW_ERROR;
> + }
> +
> + progress = rsu_prog(doorbell);
> + if (progress == RSU_PROG_PREPARE) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_TIMEOUT;
> + } else if (progress != RSU_PROG_READY) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_HW_ERROR;
> + }
> +
> + return FPGA_SEC_ERR_NONE;
> +}
> +
> +static enum fpga_sec_err rsu_send_data(struct m10bmc_sec *sec)
> +{
> + u32 doorbell;
> + int ret;
> +
> + ret = regmap_update_bits(sec->m10bmc->regmap,
> + M10BMC_SYS_BASE + M10BMC_DOORBELL,
> + DRBL_HOST_STATUS,
> + FIELD_PREP(DRBL_HOST_STATUS,
> + HOST_STATUS_WRITE_DONE));
> + if (ret)
> + return FPGA_SEC_ERR_RW_ERROR;
> +
> + ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
> + M10BMC_SYS_BASE + M10BMC_DOORBELL,
> + doorbell,
> + rsu_prog(doorbell) != RSU_PROG_READY,
> + NIOS_HANDSHAKE_INTERVAL_US,
> + NIOS_HANDSHAKE_TIMEOUT_US);
> +
> + if (ret == -ETIMEDOUT) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_TIMEOUT;
> + } else if (ret) {
> + return FPGA_SEC_ERR_RW_ERROR;
> + }
> +
> + switch (rsu_stat(doorbell)) {
> + case RSU_STAT_NORMAL:
> + case RSU_STAT_NIOS_OK:
> + case RSU_STAT_USER_OK:
> + case RSU_STAT_FACTORY_OK:
wear out is ok below but not here. why ?
> + break;
> + default:
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_HW_ERROR;
> + }
> +
> + return FPGA_SEC_ERR_NONE;
> +}
> +
> +static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
> +{
> + if (m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, doorbell))
> + return -EIO;
> +
> + switch (rsu_stat(*doorbell)) {
> + case RSU_STAT_NORMAL:
> + case RSU_STAT_NIOS_OK:
> + case RSU_STAT_USER_OK:
> + case RSU_STAT_FACTORY_OK:
> + case RSU_STAT_WEAROUT:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + switch (rsu_prog(*doorbell)) {
> + case RSU_PROG_IDLE:
> + case RSU_PROG_RSU_DONE:
> + return 0;
> + case RSU_PROG_AUTHENTICATING:
> + case RSU_PROG_COPYING:
> + case RSU_PROG_UPDATE_CANCEL:
> + case RSU_PROG_PROGRAM_KEY_HASH:
> + return -EAGAIN;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static enum fpga_sec_err m10bmc_sec_prepare(struct fpga_sec_mgr *smgr)
> +{
> + struct m10bmc_sec *sec = smgr->priv;
> + enum fpga_sec_err ret;
> +
> + if (smgr->remaining_size > M10BMC_STAGING_SIZE)
> + return FPGA_SEC_ERR_INVALID_SIZE;
> +
> + ret = rsu_check_idle(sec);
> + if (ret != FPGA_SEC_ERR_NONE)
> + return ret;
> +
> + ret = rsu_update_init(sec);
> + if (ret != FPGA_SEC_ERR_NONE)
> + return ret;
> +
> + return rsu_prog_ready(sec);
> +}
> +
> +static enum fpga_sec_err
> +m10bmc_sec_write_blk(struct fpga_sec_mgr *smgr, u32 offset, u32 size)
> +{
> + struct m10bmc_sec *sec = smgr->priv;
> + unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
> + u32 doorbell;
> + int ret;
> +
> + ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
> + if (ret) {
> + return FPGA_SEC_ERR_RW_ERROR;
> + } else if (rsu_prog(doorbell) != RSU_PROG_READY) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_HW_ERROR;
> + }
> +
> + ret = regmap_bulk_write(sec->m10bmc->regmap,
> + M10BMC_STAGING_BASE + offset,
> + (void *)smgr->data + offset, size / stride);
Lose trailing bytes if (size % stride)
maybe change to (size + stride - 1) / stride
> +
> + return ret ? FPGA_SEC_ERR_RW_ERROR : FPGA_SEC_ERR_NONE;
> +}
> +
> +/*
> + * m10bmc_sec_poll_complete() is called after handing things off to
> + * the BMC firmware. Depending on the type of update, it could be
> + * 30+ minutes before the BMC firmware completes the update. The
> + * smgr->driver_unload check allows the driver to be unloaded,
> + * but the BMC firmware will continue the update and no further
> + * secure updates can be started for this device until the update
> + * is complete.
> + */
> +static enum fpga_sec_err m10bmc_sec_poll_complete(struct fpga_sec_mgr *smgr)
> +{
> + struct m10bmc_sec *sec = smgr->priv;
> + unsigned long poll_timeout;
> + enum fpga_sec_err result;
> + u32 doorbell;
> + int ret;
> +
> + result = rsu_send_data(sec);
> + if (result != FPGA_SEC_ERR_NONE)
> + return result;
> +
> + ret = rsu_check_complete(sec, &doorbell);
If the first iteration will never be successful, just replace call with
ret = -EAGAIN;
Tom
> + poll_timeout = jiffies + msecs_to_jiffies(RSU_COMPLETE_TIMEOUT_MS);
> +
> + while (ret == -EAGAIN && !time_after(jiffies, poll_timeout)) {
> + msleep(RSU_COMPLETE_INTERVAL_MS);
> + ret = rsu_check_complete(sec, &doorbell);
> + if (smgr->driver_unload)
> + return FPGA_SEC_ERR_CANCELED;
> + }
> +
> + if (ret == -EAGAIN) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_TIMEOUT;
> + } else if (ret == -EIO) {
> + return FPGA_SEC_ERR_RW_ERROR;
> + } else if (ret) {
> + log_error_regs(sec, doorbell);
> + return FPGA_SEC_ERR_HW_ERROR;
> + }
> +
> + return FPGA_SEC_ERR_NONE;
> +}
> +
> +static enum fpga_sec_err m10bmc_sec_cancel(struct fpga_sec_mgr *smgr)
> +{
> + struct m10bmc_sec *sec = smgr->priv;
> + u32 doorbell;
> + int ret;
> +
> + ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
> + if (ret)
> + return FPGA_SEC_ERR_RW_ERROR;
> +
> + if (rsu_prog(doorbell) != RSU_PROG_READY)
> + return FPGA_SEC_ERR_BUSY;
> +
> + ret = regmap_update_bits(sec->m10bmc->regmap,
> + M10BMC_SYS_BASE + M10BMC_DOORBELL,
> + DRBL_HOST_STATUS,
> + FIELD_PREP(DRBL_HOST_STATUS,
> + HOST_STATUS_ABORT_RSU));
> +
> + return ret ? FPGA_SEC_ERR_RW_ERROR : FPGA_SEC_ERR_NONE;
> +}
> +
> +static const struct fpga_sec_mgr_ops m10bmc_sops = {
> + .prepare = m10bmc_sec_prepare,
> + .write_blk = m10bmc_sec_write_blk,
> + .poll_complete = m10bmc_sec_poll_complete,
> + .cancel = m10bmc_sec_cancel,
> +};
>
> static int m10bmc_secure_probe(struct platform_device *pdev)
> {
next prev parent reply other threads:[~2020-11-15 14:17 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-14 0:55 [PATCH v5 0/6] Intel MAX10 BMC Secure Update Driver Russ Weight
2020-11-14 0:55 ` [PATCH v5 1/6] mfd: intel-m10-bmc: support for MAX10 BMC Secure Updates Russ Weight
2020-11-14 0:55 ` [PATCH v5 2/6] fpga: m10bmc-sec: create max10 bmc secure update driver Russ Weight
2020-11-15 13:44 ` Tom Rix
2020-11-17 23:38 ` Russ Weight
2020-11-18 15:47 ` Tom Rix
2020-11-14 0:55 ` [PATCH v5 3/6] fpga: m10bmc-sec: expose max10 flash update count Russ Weight
2020-11-15 20:03 ` Moritz Fischer
2020-11-17 23:45 ` Russ Weight
2020-11-14 0:55 ` [PATCH v5 4/6] fpga: m10bmc-sec: expose max10 canceled keys in sysfs Russ Weight
2020-11-14 0:55 ` [PATCH v5 5/6] fpga: m10bmc-sec: add max10 secure update functions Russ Weight
2020-11-15 14:17 ` Tom Rix [this message]
2020-11-18 0:10 ` Russ Weight
2020-11-18 15:57 ` Tom Rix
2020-11-14 0:55 ` [PATCH v5 6/6] fpga: m10bmc-sec: add max10 get_hw_errinfo callback func Russ Weight
2020-11-15 14:20 ` Tom Rix
2020-11-18 0:16 ` Russ Weight
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3c531b5d-0620-5239-06a7-02a01381c436@redhat.com \
--to=trix@redhat.com \
--cc=hao.wu@intel.com \
--cc=lee.jones@linaro.org \
--cc=lgoncalv@redhat.com \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matthew.gerlach@intel.com \
--cc=mdf@kernel.org \
--cc=russell.h.weight@intel.com \
--cc=yilun.xu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).