Message-ID: <20210913095204.GD773070@yilunxu-OptiPlex-7050>
Date:   Mon, 13 Sep 2021 17:52:04 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     Russ Weight <russell.h.weight@...el.com>
Cc:     mdf@...nel.org, linux-fpga@...r.kernel.org,
        linux-kernel@...r.kernel.org, trix@...hat.com, lgoncalv@...hat.com,
        hao.wu@...el.com, matthew.gerlach@...el.com
Subject: Re: [PATCH v14 4/4] fpga: m10bmc-sec: add max10 secure update
  functions

On Fri, Sep 10, 2021 at 01:40:43PM -0700, 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@...el.com>
> >> ---
> >> 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?

I think if the driver could not actually cancel the update in some
progress, it should fail the driver unload request. Let the request
block.

If you just released the cancel call and let the module unload & machine
shutdown, the user image is corrupted.

Thanks,
Yilun

> 
> 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

Powered by blists - more mailing lists