Date:   Tue, 17 Nov 2020 16:10:47 -0800
From:   Russ Weight <russell.h.weight@...el.com>
To:     Tom Rix <trix@...hat.com>, mdf@...nel.org, lee.jones@...aro.org,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     lgoncalv@...hat.com, yilun.xu@...el.com, hao.wu@...el.com,
        matthew.gerlach@...el.com
Subject: Re: [PATCH v5 5/6] fpga: m10bmc-sec: add max10 secure update
 functions



On 11/15/20 6:17 AM, Tom Rix wrote:
> 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@...el.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.
There is no danger of flooding. The WEAROUT error will only be seen after 1000
flash updates have occurred - an unlikely condition. It will also only occur
once on an update, and only if they attempt to flash within 60 seconds of
power-on or within 60 seconds of a previous flash.
>
> Maybe make rsu_stat a function.
Is there a reason to prefer a function in this case? Or should I change
rsu_stat() to RSU_STAT() to make it more clear that it is a macro?

>
>> +		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 ?
Actually, wearout should not be seen while waiting for completion
either. I'll remove it from the code below. Good catch!

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

Yes - I'll make this change.
>
>> +
>> +	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;
It is unlikely that it would ever be successful on the first iteration.
I'll make this change, or perhaps change it to a do-while loop.

Thanks!
- Russ
>
> 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)
>>  {

Powered by blists - more mailing lists