Re: [PATCH v14 4/4] fpga: m10bmc-sec: add max10 secure update functions

From: Tom Rix
Date: Sat Sep 11 2021 - 15:09:01 EST

Next message: Arnaldo Carvalho de Melo: "Re: [RFC PATCH v1 25/37] perf evsel: move event open in evsel__open_cpu to separate function"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH] perf tools: Add an option to build without libbfd"
In reply to: Russ Weight: "Re: [PATCH v14 4/4] fpga: m10bmc-sec: add max10 secure update functions"
Next in thread: Xu Yilun: "Re: [PATCH v14 4/4] fpga: m10bmc-sec: add max10 secure update functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 9/10/21 1:40 PM, Russ Weight wrote:

On 9/10/21 8:33 AM, Xu Yilun wrote:

On Thu, Sep 09, 2021 at 04:33:04PM -0700, Russ Weight wrote:

Invoke an instance of the FPGA Image Load driver and 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@xxxxxxxxx>
---
v14:
- Changed symbol names to reflect the renaming of the Security Manager
Class driver to FPGA Image Load.
v13:
- No change
v12:
- Updated Date and KernelVersion fields in ABI documentation
- Removed size parameter from the write_blk() op. m10bmc_sec_write_blk()
no longer has a size parameter, and the block size is determined
in this (the lower-level) driver.
v11:
- No change
v10:
- No change
v9:
- No change
v8:
- Previously patch 5/6, otherwise no change
v7:
- No change
v6:
- Changed (size / stride) calculation to ((size + stride - 1) / stride)
to ensure that the proper count is passed to regmap_bulk_write().
- Removed unnecessary call to rsu_check_complete() in
m10bmc_sec_poll_complete() and changed while loop to
do/while loop.
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 | 310 +++++++++++++++++++++++++++-
1 file changed, 309 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index ae1383d13bfc..025655684d00 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -181,7 +181,315 @@ static const struct attribute_group *m10bmc_sec_attr_groups[] = {
NULL,
};
-static const struct fpga_image_load_ops m10bmc_lops = { };
+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_image_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_IMAGE_ERR_RW_ERROR;
+
+ if (rsu_prog(doorbell) != RSU_PROG_IDLE &&
+ rsu_prog(doorbell) != RSU_PROG_RSU_DONE) {
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_BUSY;
+ }
+
+ return FPGA_IMAGE_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_image_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_IMAGE_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_IMAGE_ERR_TIMEOUT;
+ } else if (ret) {
+ return FPGA_IMAGE_ERR_RW_ERROR;
+ }
+
+ status = rsu_stat(doorbell);
+ if (status == RSU_STAT_WEAROUT) {
+ dev_warn(sec->dev, "Excessive flash update count detected\n");
+ return FPGA_IMAGE_ERR_WEAROUT;
+ } else if (status == RSU_STAT_ERASE_FAIL) {
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_HW_ERROR;
+ }
+
+ return FPGA_IMAGE_ERR_NONE;
+}
+
+static enum fpga_image_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_IMAGE_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_IMAGE_ERR_RW_ERROR;
+ }
+
+ progress = rsu_prog(doorbell);
+ if (progress == RSU_PROG_PREPARE) {
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_TIMEOUT;
+ } else if (progress != RSU_PROG_READY) {
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_HW_ERROR;
+ }
+
+ return FPGA_IMAGE_ERR_NONE;
+}
+
+static enum fpga_image_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_IMAGE_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_IMAGE_ERR_TIMEOUT;
+ } else if (ret) {
+ return FPGA_IMAGE_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:
+ break;
+ default:
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_HW_ERROR;
+ }
+
+ return FPGA_IMAGE_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:
+ 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_image_err m10bmc_sec_prepare(struct fpga_image_load *imgld)
+{
+ struct m10bmc_sec *sec = imgld->priv;
+ enum fpga_image_err ret;
+
+ if (imgld->remaining_size > M10BMC_STAGING_SIZE)
+ return FPGA_IMAGE_ERR_INVALID_SIZE;
+
+ ret = rsu_check_idle(sec);
+ if (ret != FPGA_IMAGE_ERR_NONE)
+ return ret;
+
+ ret = rsu_update_init(sec);
+ if (ret != FPGA_IMAGE_ERR_NONE)
+ return ret;
+
+ return rsu_prog_ready(sec);
+}
+
+#define WRITE_BLOCK_SIZE 0x4000 /* Update remaining_size every 0x4000 bytes */
+
+static enum fpga_image_err
+m10bmc_sec_write_blk(struct fpga_image_load *imgld, u32 offset)
+{
+ struct m10bmc_sec *sec = imgld->priv;
+ unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+ u32 doorbell, blk_size;
+ int ret;
+
+ ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+ if (ret) {
+ return FPGA_IMAGE_ERR_RW_ERROR;
+ } else if (rsu_prog(doorbell) != RSU_PROG_READY) {
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_HW_ERROR;
+ }
+
+ blk_size = min_t(u32, imgld->remaining_size, WRITE_BLOCK_SIZE);
+ ret = regmap_bulk_write(sec->m10bmc->regmap,
+ M10BMC_STAGING_BASE + offset,
+ (void *)imgld->data + offset,
+ (blk_size + stride - 1) / stride);
+
+ if (ret)
+ return FPGA_IMAGE_ERR_RW_ERROR;
+
+ imgld->remaining_size -= blk_size;
+ return FPGA_IMAGE_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
+ * imgld->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_image_err m10bmc_sec_poll_complete(struct fpga_image_load *imgld)
+{
+ struct m10bmc_sec *sec = imgld->priv;
+ unsigned long poll_timeout;
+ enum fpga_image_err result;
+ u32 doorbell;
+ int ret;
+
+ result = rsu_send_data(sec);
+ if (result != FPGA_IMAGE_ERR_NONE)
+ return result;
+
+ poll_timeout = jiffies + msecs_to_jiffies(RSU_COMPLETE_TIMEOUT_MS);
+ do {
+ msleep(RSU_COMPLETE_INTERVAL_MS);
+ ret = rsu_check_complete(sec, &doorbell);
+ if (imgld->driver_unload)
+ return FPGA_IMAGE_ERR_CANCELED;

Why we return CANCELED error code to user when driver unload is
requested?

Users may think the update is failed and the old image is kept,
but in fact, the update continues and finally the old image is
replaced by the new one.

I could return FPGA_IMAGE_ERR_NONE, and then userspace code would
think that the update completed without error, even though the
update is still in progress.

Or I could create a new FPGA_IMAGE_ERR_EXIT code to indicate that
the driver has exited while the update is still progressing.

I like the FPGA_IMAGE_ERR_EXIT option best of those two options.
Any other suggestions?

No device so, -ENODEV

Tom

Thanks,
- Russ

Thanks,
Yilun

+ } while (ret == -EAGAIN && !time_after(jiffies, poll_timeout));
+
+ if (ret == -EAGAIN) {
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_TIMEOUT;
+ } else if (ret == -EIO) {
+ return FPGA_IMAGE_ERR_RW_ERROR;
+ } else if (ret) {
+ log_error_regs(sec, doorbell);
+ return FPGA_IMAGE_ERR_HW_ERROR;
+ }
+
+ return FPGA_IMAGE_ERR_NONE;
+}
+
+static enum fpga_image_err m10bmc_sec_cancel(struct fpga_image_load *imgld)
+{
+ struct m10bmc_sec *sec = imgld->priv;
+ u32 doorbell;
+ int ret;
+
+ ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+ if (ret)
+ return FPGA_IMAGE_ERR_RW_ERROR;
+
+ if (rsu_prog(doorbell) != RSU_PROG_READY)
+ return FPGA_IMAGE_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_IMAGE_ERR_RW_ERROR : FPGA_IMAGE_ERR_NONE;
+}
+
+static const struct fpga_image_load_ops m10bmc_lops = {
+ .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)
{
--
2.25.1

Next message: Arnaldo Carvalho de Melo: "Re: [RFC PATCH v1 25/37] perf evsel: move event open in evsel__open_cpu to separate function"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH] perf tools: Add an option to build without libbfd"
In reply to: Russ Weight: "Re: [PATCH v14 4/4] fpga: m10bmc-sec: add max10 secure update functions"
Next in thread: Xu Yilun: "Re: [PATCH v14 4/4] fpga: m10bmc-sec: add max10 secure update functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]