From: Russ Weight <russell.h.weight@intel.com>
To: Tom Rix <trix@redhat.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 v2 1/6] mfd: intel-m10-bmc: support for MAX10 BMC Security Engine
Date: Thu, 8 Oct 2020 16:03:14 -0700 [thread overview]
Message-ID: <9e5b012d-c8e4-2c01-26e0-a6bf6dce6679@intel.com> (raw)
In-Reply-To: <53a712e8-63e5-8b75-a2a4-6bf7fa327462@intel.com>
I just realized that I missed a few questions on my first reply.
Please see my responses below.
On 10/7/20 5:52 PM, Russ Weight wrote:
>
> On 10/6/20 9:34 AM, Tom Rix wrote:
>> On 10/2/20 6:24 PM, Russ Weight wrote:
>>> Add macros and definitions required by the MAX10 BMC
>>> Security Engine driver.
>>>
>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>> ---
>>> v2:
>>> - These functions and macros were previously distributed among
>>> the patches that needed them. They are now grouped together
>>> in a single patch containing changes to the Intel MAX10 BMC
>>> driver.
>>> - Added DRBL_ prefix to some definitions
>>> - Some address definitions were moved here from the .c files that
>>> use them.
>>> ---
>>> include/linux/mfd/intel-m10-bmc.h | 134 ++++++++++++++++++++++++++++++
>>> 1 file changed, 134 insertions(+)
>>>
>>> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
>>> index c8ef2f1654a4..880f907302eb 100644
>>> --- a/include/linux/mfd/intel-m10-bmc.h
>>> +++ b/include/linux/mfd/intel-m10-bmc.h
>>> @@ -13,6 +13,9 @@
>>> #define M10BMC_SYS_BASE 0x300800
>>> #define M10BMC_MEM_END 0x200000fc
>>>
>>> +#define M10BMC_STAGING_BASE 0x18000000
>>> +#define M10BMC_STAGING_SIZE 0x3800000
>> The staging size is not used, please use it in m10bmc_sec_write_blk to
>>
>> check the input parameter 'size'
> It is used to check the input size in the prepare function:
> m10bmc_sec_prepare()
>
> if (smgr->remaining_size > M10BMC_STAGING_SIZE)
> return FPGA_SEC_ERR_INVALID_SIZE;
>
> - Russ
>
>>> +
>>> /* Register offset of system registers */
>>> #define NIOS2_FW_VERSION 0x0
>>> #define M10BMC_TEST_REG 0x3c
>>> @@ -21,6 +24,88 @@
>>> #define M10BMC_VER_PCB_INFO_MSK GENMASK(31, 24)
>>> #define M10BMC_VER_LEGACY_INVALID 0xffffffff
>>>
>>> +/* Secure update doorbell register, in system register region */
>>> +#define M10BMC_DOORBELL 0x400
>>> +
>>> +/* Authorization Result register, in system register region */
>>> +#define M10BMC_AUTH_RESULT 0x404
>>> +
>>> +/* Doorbell register fields */
>>> +#define DRBL_RSU_REQUEST BIT(0)
>>> +#define DRBL_RSU_PROGRESS GENMASK(7, 4)
>>> +#define DRBL_HOST_STATUS GENMASK(11, 8)
>>> +#define DRBL_RSU_STATUS GENMASK(23, 16)
>>> +#define DRBL_PKVL_EEPROM_LOAD_SEC BIT(24)
>>> +#define DRBL_PKVL1_POLL_EN BIT(25)
>>> +#define DRBL_PKVL2_POLL_EN BIT(26)
>> PKVL seems like it would be n3000 specific.
>>
>> For this and similar it may be good to add a _N3000_ in the name.
I'm hoping Yilun and Hao might chime in here. We have substituted PKVL
for Retimer in some places in the code.
>>
>>> +#define DRBL_CONFIG_SEL BIT(28)
>>> +#define DRBL_REBOOT_REQ BIT(29)
>>> +#define DRBL_REBOOT_DISABLED BIT(30)
>>> +
>>> +/* Progress states */
>>> +#define RSU_PROG_IDLE 0x0
>>> +#define RSU_PROG_PREPARE 0x1
>>> +#define RSU_PROG_READY 0x3
>>> +#define RSU_PROG_AUTHENTICATING 0x4
>>> +#define RSU_PROG_COPYING 0x5
>>> +#define RSU_PROG_UPDATE_CANCEL 0x6
>>> +#define RSU_PROG_PROGRAM_KEY_HASH 0x7
>>> +#define RSU_PROG_RSU_DONE 0x8
>>> +#define RSU_PROG_PKVL_PROM_DONE 0x9
>>> +
>>> +/* Device and error states */
>>> +#define RSU_STAT_NORMAL 0x0
>>> +#define RSU_STAT_TIMEOUT 0x1
>>> +#define RSU_STAT_AUTH_FAIL 0x2
>>> +#define RSU_STAT_COPY_FAIL 0x3
>>> +#define RSU_STAT_FATAL 0x4
>>> +#define RSU_STAT_PKVL_REJECT 0x5
>>> +#define RSU_STAT_NON_INC 0x6
>>> +#define RSU_STAT_ERASE_FAIL 0x7
>>> +#define RSU_STAT_WEAROUT 0x8
>>> +#define RSU_STAT_NIOS_OK 0x80
>>> +#define RSU_STAT_USER_OK 0x81
>>> +#define RSU_STAT_FACTORY_OK 0x82
>>> +#define RSU_STAT_USER_FAIL 0x83
>>> +#define RSU_STAT_FACTORY_FAIL 0x84
>>> +#define RSU_STAT_NIOS_FLASH_ERR 0x85
>>> +#define RSU_STAT_FPGA_FLASH_ERR 0x86
>>> +
>>> +#define HOST_STATUS_IDLE 0x0
>>> +#define HOST_STATUS_WRITE_DONE 0x1
>>> +#define HOST_STATUS_ABORT_RSU 0x2
>>> +
>>> +#define rsu_prog(doorbell) FIELD_GET(DRBL_RSU_PROGRESS, doorbell)
>>> +#define rsu_stat(doorbell) FIELD_GET(DRBL_RSU_STATUS, doorbell)
>>> +
>>> +/* interval 100ms and timeout 5s */
>>> +#define NIOS_HANDSHAKE_INTERVAL_US (100 * 1000)
>>> +#define NIOS_HANDSHAKE_TIMEOUT_US (5 * 1000 * 1000)
>>> +
>>> +/* RSU PREP Timeout (2 minutes) to erase flash staging area */
>>> +#define RSU_PREP_INTERVAL_MS 100
>>> +#define RSU_PREP_TIMEOUT_MS (2 * 60 * 1000)
>>> +
>>> +/* RSU Complete Timeout (40 minutes) for full flash update */
>>> +#define RSU_COMPLETE_INTERVAL_MS 1000
>>> +#define RSU_COMPLETE_TIMEOUT_MS (40 * 60 * 1000)
>> minutes is an unusual timeout unit.
>>
>> It may be worthwhile to spell out MINUTES to avoid confusing with micro seconds.
The _MS at the end of the constants means milliseconds. That is pretty
standard, isn't it? Although the wait is 40 minutes, the constant is a
millisecond count that corresponds to 40 minutes. I expanded the definition
to make things more clear: 40 (minutes) * 60 (seconds) * 1000 milliseconds =
(40 minutes expressed in milliseconds).
>> Tom
>>
>>> +
>>> +/* Addresses for security related data in FLASH */
>>> +#define BMC_REH_ADDR 0x17ffc004
>>> +#define BMC_PROG_ADDR 0x17ffc000
>>> +#define BMC_PROG_MAGIC 0x5746
>>> +
>>> +#define SR_REH_ADDR 0x17ffd004
>>> +#define SR_PROG_ADDR 0x17ffd000
>>> +#define SR_PROG_MAGIC 0x5253
>>> +
>>> +#define PR_REH_ADDR 0x17ffe004
>>> +#define PR_PROG_ADDR 0x17ffe000
>>> +#define PR_PROG_MAGIC 0x5250
>>> +
>>> +/* Address of inverted bit vector containing user the image FLASH count */
>>> +#define USER_FLASH_COUNT 0x17ffb000
>>> +
>>> /**
>>> * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
>>> * @dev: this device
>>> @@ -35,7 +120,11 @@ struct intel_m10bmc {
>>> * register access helper functions.
>>> *
>>> * m10bmc_raw_read - read m10bmc register per addr
>>> + * m10bmc_raw_bulk_read - bulk read max10 registers per addr
>>> + * m10bmc_raw_bulk_write - bulk write max10 registers per addr
>>> + * m10bmc_raw_update_bits - update max10 register per addr
>>> * m10bmc_sys_read - read m10bmc system register per offset
>>> + * m10bmc_sys_update_bits - update max10 system register per offset
>>> */
>>> static inline int
>>> m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>>> @@ -51,6 +140,48 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>>> return ret;
>>> }
>>>
>>> +static inline int
>>> +m10bmc_raw_bulk_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>>> + void *val, size_t cnt)
>>> +{
>>> + int ret;
>>> +
>>> + ret = regmap_bulk_read(m10bmc->regmap, addr, val, cnt);
>>> + if (ret)
>>> + dev_err(m10bmc->dev, "fail to read raw reg %x cnt %zx: %d\n",
>>> + addr, cnt, ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static inline int
>>> +m10bmc_raw_bulk_write(struct intel_m10bmc *m10bmc, unsigned int addr,
>>> + void *val, size_t cnt)
>>> +{
>>> + int ret;
>>> +
>>> + ret = regmap_bulk_write(m10bmc->regmap, addr, val, cnt);
>>> + if (ret)
>>> + dev_err(m10bmc->dev, "fail to write raw reg %x cnt %zx: %d\n",
>>> + addr, cnt, ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static inline int
>>> +m10bmc_raw_update_bits(struct intel_m10bmc *m10bmc, unsigned int addr,
>>> + unsigned int msk, unsigned int val)
>>> +{
>>> + int ret;
>>> +
>>> + ret = regmap_update_bits(m10bmc->regmap, addr, msk, val);
>>> + if (ret)
>>> + dev_err(m10bmc->dev, "fail to update raw reg %x: %d\n",
>>> + addr, ret);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> /*
>>> * The base of the system registers could be configured by HW developers, and
>>> * in HW SPEC, the base is not added to the addresses of the system registers.
>>> @@ -62,4 +193,7 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>>> #define m10bmc_sys_read(m10bmc, offset, val) \
>>> m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>>>
>>> +#define m10bmc_sys_update_bits(m10bmc, offset, msk, val) \
>>> + m10bmc_raw_update_bits(m10bmc, M10BMC_SYS_BASE + (offset), msk, val)
>>> +
>>> #endif /* __MFD_INTEL_M10_BMC_H */
next prev parent reply other threads:[~2020-10-08 23:03 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-03 1:24 [PATCH v2 0/6] Intel MAX10 BMC Security Engine Driver Russ Weight
2020-10-03 1:24 ` [PATCH v2 1/6] mfd: intel-m10-bmc: support for MAX10 BMC Security Engine Russ Weight
2020-10-06 16:34 ` Tom Rix
2020-10-08 0:52 ` Russ Weight
2020-10-08 23:03 ` Russ Weight [this message]
2020-10-09 20:04 ` Tom Rix
2020-10-07 7:00 ` Lee Jones
2020-10-08 0:49 ` Russ Weight
2020-10-08 7:23 ` Lee Jones
2020-10-03 1:24 ` [PATCH v2 2/6] fpga: m10bmc-sec: create max10 bmc security engine Russ Weight
2020-10-03 3:15 ` Randy Dunlap
2020-10-04 18:01 ` Russ Weight
2020-10-04 18:07 ` Randy Dunlap
2020-10-06 17:31 ` Tom Rix
2020-10-08 21:12 ` Russ Weight
2020-10-03 1:24 ` [PATCH v2 3/6] fpga: m10bmc-sec: expose max10 flash update counts Russ Weight
2020-10-06 17:35 ` Tom Rix
2020-10-03 1:24 ` [PATCH v2 4/6] fpga: m10bmc-sec: expose max10 canceled keys in sysfs Russ Weight
2020-10-06 17:41 ` Tom Rix
2020-10-03 1:24 ` [PATCH v2 5/6] fpga: m10bmc-sec: add max10 secure update functions Russ Weight
2020-10-06 19:08 ` Tom Rix
2020-10-08 23:06 ` Russ Weight
2020-10-09 20:15 ` Tom Rix
2020-10-03 1:24 ` [PATCH v2 6/6] fpga: m10bmc-sec: add max10 get_hw_errinfo callback func 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=9e5b012d-c8e4-2c01-26e0-a6bf6dce6679@intel.com \
--to=russell.h.weight@intel.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=trix@redhat.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).