On 10/8/20 4:06 PM, Russ Weight wrote:
>
> On 10/6/20 12:08 PM, Tom Rix wrote:
>> On 10/2/20 6:24 PM, Russ Weight wrote:
>>> Extend the MAX10 BMC Security Engine driver to include
>>> the functions that enable secure updates of BMC images,
>>> FPGA images, etc.
>>>
>>> Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
>>> ---
>>> 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 | 298 ++++++++++++++++++++++++++++
>>> 1 file changed, 298 insertions(+)
>>>
>>> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
>>> index 5bb45499b332..a9617c5b3845 100644
>>> --- a/drivers/fpga/intel-m10-bmc-secure.c
>>> +++ b/drivers/fpga/intel-m10-bmc-secure.c
>>> @@ -201,6 +201,300 @@ static int m10bmc_pr_canceled_csks(struct ifpga_sec_mgr *imgr,
>>> csk_map, nbits);
>>> }
>>>
>>> +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 ifpga_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 IFPGA_SEC_ERR_RW_ERROR;
>>> +
>>> + if (rsu_prog(doorbell) != RSU_PROG_IDLE &&
>>> + rsu_prog(doorbell) != RSU_PROG_RSU_DONE) {
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_BUSY;
>>> + }
>>> +
>>> + return IFPGA_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 ifpga_sec_err rsu_update_init(struct m10bmc_sec *sec)
>>> +{
>>> + u32 doorbell, status;
>>> + int ret;
>>> +
>>> + ret = m10bmc_sys_update_bits(sec->m10bmc, M10BMC_DOORBELL,
>>> + DRBL_RSU_REQUEST | DRBL_HOST_STATUS,
>>> + DRBL_RSU_REQUEST |
>>> + FIELD_PREP(DRBL_HOST_STATUS,
>>> + HOST_STATUS_IDLE));
>>> + if (ret)
>>> + return IFPGA_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 IFPGA_SEC_ERR_TIMEOUT;
>>> + } else if (ret) {
>>> + return IFPGA_SEC_ERR_RW_ERROR;
>>> + }
>>> +
>>> + status = rsu_stat(doorbell);
>>> + if (status == RSU_STAT_WEAROUT) {
>>> + dev_warn(sec->dev, "Excessive flash update count detected\n");
>> Device is permanently failing, dev_err or higher is more appropriate than dev_warn.
>>
>> warn once to limit noisy logs.
> This is not a permanent/hard failure. When the flash count (for the staging area)
> exceeds 1000, a 30 second delay is imposed on subsequent flashes. When the count
> hits 2000, the delay goes to 60 seconds.
>
> Also, flash events shouldn't that often, so I don't think they are going to create
> a lot of noise in the logs.
>
> I think this is OK as is?
If the extra, worse case time is accounted for in the current timeout, yes this is fine.
>
>>> + return IFPGA_SEC_ERR_WEAROUT;
>>> + } else if (status == RSU_STAT_ERASE_FAIL) {
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_HW_ERROR;
>>> + }
>>> +
>>> + return IFPGA_SEC_ERR_NONE;
>>> +}
>>> +
>>> +static enum ifpga_sec_err (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 IFPGA_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 IFPGA_SEC_ERR_RW_ERROR;
>>> + }
>>> +
>>> + progress = rsu_prog(doorbell);
>>> + if (progress == RSU_PROG_PREPARE) {
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_TIMEOUT;
>>> + } else if (progress != RSU_PROG_READY) {
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_HW_ERROR;
>>> + }
>>> +
>>> + return IFPGA_SEC_ERR_NONE;
>>> +}
>>> +
>>> +static enum ifpga_sec_err rsu_send_data(struct m10bmc_sec *sec)
>>> +{
>>> + u32 doorbell;
>>> + int ret;
>>> +
>>> + ret = m10bmc_sys_update_bits(sec->m10bmc, M10BMC_DOORBELL,
>>> + DRBL_HOST_STATUS,
>>> + FIELD_PREP(DRBL_HOST_STATUS,
>>> + HOST_STATUS_WRITE_DONE));
>>> + if (ret)
>>> + return IFPGA_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 IFPGA_SEC_ERR_TIMEOUT;
>>> + } else if (ret) {
>>> + return IFPGA_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:
>>> + break;
>>> + default:
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_HW_ERROR;
>>> + }
>> This and similar below..
>>
>> switch can be converted to
>>
>> if (!rsu_stat(doorbell) & (RSU_STAT_NORMAL | ... ))
>>
>> fail
> These are not bit-flags. The rsu_stat() macro extracts an 8-bit field from
> the doorbell register. The current supported values run from 0 to 9.
> To do this with if-statements would require something like this:
>
> status = rsu_stat(doorbell);
>
> if ((status != RSU_STAT_NORMAL) && (status != RSU_STAT_NIOS_OK) && ... To me, the switch statement seems cleaner, but I'm willing to change it if you think the if statements are better.
Ah. ok fine as-is
>>> +
>>> + return IFPGA_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 ifpga_sec_err m10bmc_sec_prepare(struct ifpga_sec_mgr *imgr)
>>> +{
>>> + struct m10bmc_sec *sec = imgr->priv;
>>> + enum ifpga_sec_err ret;
>>> +
>>> + if (imgr->remaining_size > M10BMC_STAGING_SIZE)
>>> + return IFPGA_SEC_ERR_INVALID_SIZE;
>>> +
>>> + ret = rsu_check_idle(sec);
>>> + if (ret)
>> This needs to change, generally, to
>>
>> if (ret != IFPGA_SEC_ERR_NONE)
> Yes, I'll make this change. There are also a couple of places in the
> class driver where the same changes need to be made (for the update ops).
> I'll take care of that as well.
>
>>> + return ret;
>>> +
>>> + ret = rsu_update_init(sec);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return rsu_prog_ready(sec);
>>> +}
>>> +
>>> +static enum ifpga_sec_err
>>> +m10bmc_sec_write_blk(struct ifpga_sec_mgr *imgr, u32 offset, u32 size)
>>> +{
>>> + struct m10bmc_sec *sec = imgr->priv;
>>> + unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>>> + u32 doorbell;
>>> + int ret;
>>> +
>> size check here.
> The size check is done in the prepare function above at the beginning of
> the update process.
ok
>>> + ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
>> Wondering about the use of m10bmc_sys_read generally.
>>
>> If secure writing depends on new mmio region why not use the raw_read ?
>>
>> wondering if mixing old door bell regs with new sec regs would even work.
> We were able to share code between non-secure and secure hardware by using this
> approach. Instead of having a constant for the base address, the base address
> was determined based on the hardware. The register offsets were the same, so the
> code was generally the same for both secure and non-secure hardware - with a few
> exceptions.
>
> You are correct that the doorbell register has no application in the non-secure
> hardware, but it could potentially have meaning for a future device with a
> different base address for the register space.
>
>>> + if (ret) {
>>> + return IFPGA_SEC_ERR_RW_ERROR;
>>> + } else if (rsu_prog(doorbell) != RSU_PROG_READY) {
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_HW_ERROR;
>>> + }
>>> +
>>> + ret = m10bmc_raw_bulk_write(sec->m10bmc, M10BMC_STAGING_BASE + offset,
>>> + (void *)imgr->data + offset, size / stride);
>>> +
>>> + return ret ? IFPGA_SEC_ERR_RW_ERROR : IFPGA_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
>>> + * imgr->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 ifpga_sec_err m10bmc_sec_poll_complete(struct ifpga_sec_mgr *imgr)
>>> +{
>>> + struct m10bmc_sec *sec = imgr->priv;
>>> + unsigned long poll_timeout;
>>> + enum ifpga_sec_err result;
>>> + u32 doorbell;
>>> + int ret;
>>> +
>>> + result = rsu_send_data(sec);
>>> + if (result)
>>> + return result;
>>> +
>>> + ret = rsu_check_complete(sec, &doorbell);
>>> + 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 (imgr->driver_unload)
>>> + return IFPGA_SEC_ERR_CANCELED;
>> Instead of checking for complete could you check the progress ?
>>
>> hate for it to fail with 90% done.
> I'm not sure I'm understanding the question. Once the hardwarehas received the
> image data and begun the update process, there is no ability to handshake with
> the HW until the process is complete. All we can do is monitor the progress field,
> which is what the rsu_check_complete() function does. As long as there are no
> errors and the status looks OK, we continue to wait up to 40 minutes for the
> process to complete.
>
> Thanks for the comments!
ok, update is a reallly long atomic.
Tom
> - Russ
>
>> Tom
>>
>>> + }
>>> +
>>> + if (ret == -EAGAIN) {
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_TIMEOUT;
>>> + } else if (ret == -EIO) {
>>> + return IFPGA_SEC_ERR_RW_ERROR;
>>> + } else if (ret) {
>>> + log_error_regs(sec, doorbell);
>>> + return IFPGA_SEC_ERR_HW_ERROR;
>>> + }
>>> +
>>> + return IFPGA_SEC_ERR_NONE;
>>> +}
>>> +
>>> +static enum ifpga_sec_err m10bmc_sec_cancel(struct ifpga_sec_mgr *imgr)
>>> +{
>>> + struct m10bmc_sec *sec = imgr->priv;
>>> + u32 doorbell;
>>> + int ret;
>>> +
>>> + ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
>>> + if (ret)
>>> + return IFPGA_SEC_ERR_RW_ERROR;
>>> +
>>> + if (rsu_prog(doorbell) != RSU_PROG_READY)
>>> + return IFPGA_SEC_ERR_BUSY;
>>> +
>>> + ret = m10bmc_sys_update_bits(sec->m10bmc, M10BMC_DOORBELL,
>>> + DRBL_HOST_STATUS,
>>> + FIELD_PREP(DRBL_HOST_STATUS,
>>> + HOST_STATUS_ABORT_RSU));
>>> +
>>> + return ret ? IFPGA_SEC_ERR_RW_ERROR : IFPGA_SEC_ERR_NONE;
>>> +}
>>> +
>>> static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>>> .user_flash_count = m10bmc_user_flash_count,
>>> .bmc_root_entry_hash = m10bmc_bmc_root_entry_hash,
>>> @@ -215,6 +509,10 @@ static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>>> .bmc_canceled_csk_nbits = m10bmc_csk_cancel_nbits,
>>> .sr_canceled_csk_nbits = m10bmc_csk_cancel_nbits,
>>> .pr_canceled_csk_nbits = m10bmc_csk_cancel_nbits,
>>> + .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)
