diff mbox series

[v1,02/12] fpga: create intel max10 bmc security engine

Message ID 20200904235305.6254-3-russell.h.weight@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series Intel FPGA Security Manager Class Driver | expand

Commit Message

Russ Weight Sept. 4, 2020, 11:52 p.m. UTC 
Create a platform driver that can be invoked as a sub
driver for the Intel MAX10 BMC in order to support
secure updates. This sub-driver will invoke an
instance of the Intel FPGA Security Manager class driver
in order to expose sysfs interfaces for managing and
monitoring secure updates to FPGA and BMC images.

This patch creates the MAX10 BMC Security Engine driver and
provides support for displaying the current root entry hashes
for the FPGA static region, the FPGA PR region, and the MAX10
BMC.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Wu Hao <hao.wu@intel.com>
---
 drivers/fpga/Kconfig                |  11 ++
 drivers/fpga/Makefile               |   3 +
 drivers/fpga/intel-m10-bmc-secure.c | 170 ++++++++++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h   |  15 +++
 4 files changed, 199 insertions(+)
 create mode 100644 drivers/fpga/intel-m10-bmc-secure.c

Comments

Randy Dunlap Sept. 5, 2020, 12:01 a.m. UTC | #1 
On 9/4/20 4:52 PM, Russ Weight wrote:
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 97c0a6cc2ba7..0f0bed68e618 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>  	  region and for the BMC. Select this option to enable
>  	  updates for secure FPGA devices.
>  
> +config IFPGA_M10_BMC_SECURE
> +        tristate "Intel MAX10 BMC security engine"
> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
> +        help
> +          Secure update support for the Intel MAX10 board management
> +	  controller.

Please consistently use one tab to indent Kconfig keywords (tristate, depends, help)
and one tab + 2 spaces to indent help text.
(as in Documentation/process/coding-style.rst)

> +
> +	  This is a subdriver of the Intel MAX10 board management controller
> +	  (BMC) and provides support for secure updates for the BMC image,
> +	  the FPGA image, the Root Entry Hashes, etc.
> +
>  endif # FPGA


thanks.
Russ Weight Sept. 5, 2020, 12:05 a.m. UTC | #2 
On 9/4/20 5:01 PM, Randy Dunlap wrote:
> On 9/4/20 4:52 PM, Russ Weight wrote:
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 97c0a6cc2ba7..0f0bed68e618 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>>   	  region and for the BMC. Select this option to enable
>>   	  updates for secure FPGA devices.
>>   
>> +config IFPGA_M10_BMC_SECURE
>> +        tristate "Intel MAX10 BMC security engine"
>> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
>> +        help
>> +          Secure update support for the Intel MAX10 board management
>> +	  controller.
> Please consistently use one tab to indent Kconfig keywords (tristate, depends, help)
> and one tab + 2 spaces to indent help text.
> (as in Documentation/process/coding-style.rst)

Thanks for the feedback. I'll fix these.

>> +
>> +	  This is a subdriver of the Intel MAX10 board management controller
>> +	  (BMC) and provides support for secure updates for the BMC image,
>> +	  the FPGA image, the Root Entry Hashes, etc.
>> +
>>   endif # FPGA
>
> thanks.
Tom Rix Sept. 5, 2020, 8:22 p.m. UTC | #3 
On 9/4/20 4:52 PM, Russ Weight wrote:
> Create a platform driver that can be invoked as a sub
> driver for the Intel MAX10 BMC in order to support
> secure updates. This sub-driver will invoke an
> instance of the Intel FPGA Security Manager class driver
> in order to expose sysfs interfaces for managing and
> monitoring secure updates to FPGA and BMC images.
>
> This patch creates the MAX10 BMC Security Engine driver and
> provides support for displaying the current root entry hashes
> for the FPGA static region, the FPGA PR region, and the MAX10
> BMC.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Wu Hao <hao.wu@intel.com>
> ---
>  drivers/fpga/Kconfig                |  11 ++
>  drivers/fpga/Makefile               |   3 +
>  drivers/fpga/intel-m10-bmc-secure.c | 170 ++++++++++++++++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h   |  15 +++
>  4 files changed, 199 insertions(+)
>  create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 97c0a6cc2ba7..0f0bed68e618 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>  	  region and for the BMC. Select this option to enable
>  	  updates for secure FPGA devices.
>  
> +config IFPGA_M10_BMC_SECURE
> +        tristate "Intel MAX10 BMC security engine"
> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
> +        help
> +          Secure update support for the Intel MAX10 board management
> +	  controller.
> +
> +	  This is a subdriver of the Intel MAX10 board management controller
> +	  (BMC) and provides support for secure updates for the BMC image,
> +	  the FPGA image, the Root Entry Hashes, etc.
> +
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index ec9fbacdedd8..451a23ec3168 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -24,6 +24,9 @@ obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>  # Intel FPGA Security Manager Framework
>  obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>  
> +# Intel Security Manager Drivers
> +obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.o
> +
>  # FPGA Bridge Drivers
>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
> new file mode 100644
> index 000000000000..1f86bfb694b4
> --- /dev/null
> +++ b/drivers/fpga/intel-m10-bmc-secure.c
> @@ -0,0 +1,170 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Max10 Board Management Controller Security Engine Driver
> + *
> + * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
> + *
> + */
> +#include <linux/device.h>
> +#include <linux/fpga/ifpga-sec-mgr.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/vmalloc.h>
> +
> +struct m10bmc_sec {
> +	struct device *dev;
> +	struct intel_m10bmc *m10bmc;
> +	struct ifpga_sec_mgr *imgr;
> +};
> +
> +#define SHA256_REH_SIZE		32
> +#define SHA384_REH_SIZE		48
> +
> +static int get_reh_size(struct ifpga_sec_mgr *imgr,
> +			u32 exp_magic, u32 prog_addr)
> +{
> +	struct m10bmc_sec *sec = imgr->priv;
> +	int sha_num_bytes, ret;
> +	u32 magic;
> +
> +	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(sec->dev, "%s magic 0x%08x\n", __func__, magic);
> +
> +	if ((magic & 0xffff) != exp_magic)
return -EINVAL ?
> +		return 0;
> +
> +	sha_num_bytes = ((magic >> 16) & 0xffff) / 8;
> +
> +	if (sha_num_bytes != SHA256_REH_SIZE &&
> +	    sha_num_bytes != SHA384_REH_SIZE)   {
> +		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
> +			sha_num_bytes);
> +		return -EINVAL;
> +	}
> +
> +	return sha_num_bytes;
> +}
> +
> +#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
Why shouldn't these #defines be collected in the intel-m10-bmc.h ?
> +
> +#define SYSFS_GET_REH_SIZE(_name, _exp_magic, _prog_addr) \
> +static int get_##_name##_reh_size(struct ifpga_sec_mgr *imgr) \
> +{ \
> +	return get_reh_size(imgr, _exp_magic, _prog_addr); \
> +}
Is this macro overkill for a 1 liner ?
> +
> +SYSFS_GET_REH_SIZE(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR)
> +SYSFS_GET_REH_SIZE(sr, SR_PROG_MAGIC, SR_PROG_ADDR)
> +SYSFS_GET_REH_SIZE(pr, PR_PROG_MAGIC, PR_PROG_ADDR)

These and similar below do not directly interact

with the sysfs so the sysfs_ prefix should not be needed.

> +
> +static int get_root_entry_hash(struct ifpga_sec_mgr *imgr,
> +			       u32 hash_addr, u8 *hash,
> +			       unsigned int size)
> +{
> +	struct m10bmc_sec *sec = imgr->priv;
> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
> +	int ret;
> +
> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, hash_addr,
> +				   hash, size / stride);
> +	if (ret)
> +		dev_err(sec->dev, "bulk_read of 0x%x failed %d",
> +			hash_addr, ret);
> +
> +	return ret;
> +}
> +
> +#define SYSFS_GET_REH(_name, _hash_addr) \
> +static int get_##_name##_root_entry_hash(struct ifpga_sec_mgr *imgr, \
> +					 u8 *hash, unsigned int size) \
> +{ \
> +	return get_root_entry_hash(imgr, _hash_addr, hash, size); \
> +}
> +
> +SYSFS_GET_REH(bmc, BMC_REH_ADDR)
> +SYSFS_GET_REH(sr, SR_REH_ADDR)
> +SYSFS_GET_REH(pr, PR_REH_ADDR)
> +
> +static const struct ifpga_sec_mgr_ops m10bmc_iops = {
> +	.bmc_root_entry_hash = get_bmc_root_entry_hash,
> +	.sr_root_entry_hash = get_sr_root_entry_hash,
> +	.pr_root_entry_hash = get_pr_root_entry_hash,
> +	.bmc_reh_size = get_bmc_reh_size,
> +	.sr_reh_size = get_sr_reh_size,
> +	.pr_reh_size = get_pr_reh_size,

The prefix of the ops should be similar to the file name.

so consider changing get_pr_reh_size to max10_pr_reh_size

> +};
> +
> +static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
> +{
> +	ifpga_sec_mgr_unregister(sec->imgr);
> +}
> +
> +static int ifpga_sec_mgr_init(struct m10bmc_sec *sec)
> +{
> +	struct ifpga_sec_mgr *imgr;
> +
> +	imgr = ifpga_sec_mgr_register(sec->dev, "Max10 BMC Security Manager",
> +				      &m10bmc_iops, sec);
> +	if (IS_ERR(imgr))
> +		return PTR_ERR(imgr);
> +
> +	sec->imgr = imgr;
> +	return 0;
> +}
> +
> +static int m10bmc_secure_probe(struct platform_device *pdev)
> +{
> +	struct m10bmc_sec *sec;
> +	int ret;
> +
> +	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
> +	if (!sec)
> +		return -ENOMEM;
> +
> +	sec->dev = &pdev->dev;
> +	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
> +	dev_set_drvdata(&pdev->dev, sec);
> +
> +	ret = ifpga_sec_mgr_init(sec);
> +	if (ret)
> +		dev_err(&pdev->dev,
> +			"Security manager failed to start: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int m10bmc_secure_remove(struct platform_device *pdev)
> +{
> +	struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
> +
> +	ifpga_sec_mgr_uinit(sec);
> +	return 0;
> +}
> +
> +static struct platform_driver intel_m10bmc_secure_driver = {
> +	.probe = m10bmc_secure_probe,
> +	.remove = m10bmc_secure_remove,
> +	.driver = {
> +		.name = "n3000bmc-secure",

From the filename, should this be "max10bmc-secure" ?

> +	},
> +};
> +module_platform_driver(intel_m10bmc_secure_driver);
> +
> +MODULE_ALIAS("platform:n3000bmc-secure");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("Intel MAX10 BMC secure engine");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index d4cb01b73357..7fe465c320c2 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -63,6 +63,7 @@ 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

second '_' should be removed so it reads like

bulk read max10 registers..

Tom

>   * m10bmc_sys_read - read m10bmc system register per offset
>   */
>  static inline int
> @@ -79,6 +80,20 @@ 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;
> +}
> +
>  #define m10bmc_sys_read(m10bmc, offset, val) \
>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>
Russ Weight Sept. 14, 2020, 7:07 p.m. UTC | #4 
On 9/5/20 1:22 PM, Tom Rix wrote:
> On 9/4/20 4:52 PM, Russ Weight wrote:
>> Create a platform driver that can be invoked as a sub
>> driver for the Intel MAX10 BMC in order to support
>> secure updates. This sub-driver will invoke an
>> instance of the Intel FPGA Security Manager class driver
>> in order to expose sysfs interfaces for managing and
>> monitoring secure updates to FPGA and BMC images.
>>
>> This patch creates the MAX10 BMC Security Engine driver and
>> provides support for displaying the current root entry hashes
>> for the FPGA static region, the FPGA PR region, and the MAX10
>> BMC.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> Reviewed-by: Wu Hao <hao.wu@intel.com>
>> ---
>>  drivers/fpga/Kconfig                |  11 ++
>>  drivers/fpga/Makefile               |   3 +
>>  drivers/fpga/intel-m10-bmc-secure.c | 170 ++++++++++++++++++++++++++++
>>  include/linux/mfd/intel-m10-bmc.h   |  15 +++
>>  4 files changed, 199 insertions(+)
>>  create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 97c0a6cc2ba7..0f0bed68e618 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>>  	  region and for the BMC. Select this option to enable
>>  	  updates for secure FPGA devices.
>>  
>> +config IFPGA_M10_BMC_SECURE
>> +        tristate "Intel MAX10 BMC security engine"
>> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
>> +        help
>> +          Secure update support for the Intel MAX10 board management
>> +	  controller.
>> +
>> +	  This is a subdriver of the Intel MAX10 board management controller
>> +	  (BMC) and provides support for secure updates for the BMC image,
>> +	  the FPGA image, the Root Entry Hashes, etc.
>> +
>>  endif # FPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index ec9fbacdedd8..451a23ec3168 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -24,6 +24,9 @@ obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>  # Intel FPGA Security Manager Framework
>>  obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>>  
>> +# Intel Security Manager Drivers
>> +obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.o
>> +
>>  # FPGA Bridge Drivers
>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
>> new file mode 100644
>> index 000000000000..1f86bfb694b4
>> --- /dev/null
>> +++ b/drivers/fpga/intel-m10-bmc-secure.c
>> @@ -0,0 +1,170 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Intel Max10 Board Management Controller Security Engine Driver
>> + *
>> + * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
>> + *
>> + */
>> +#include <linux/device.h>
>> +#include <linux/fpga/ifpga-sec-mgr.h>
>> +#include <linux/mfd/intel-m10-bmc.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/vmalloc.h>
>> +
>> +struct m10bmc_sec {
>> +	struct device *dev;
>> +	struct intel_m10bmc *m10bmc;
>> +	struct ifpga_sec_mgr *imgr;
>> +};
>> +
>> +#define SHA256_REH_SIZE		32
>> +#define SHA384_REH_SIZE		48
>> +
>> +static int get_reh_size(struct ifpga_sec_mgr *imgr,
>> +			u32 exp_magic, u32 prog_addr)
>> +{
>> +	struct m10bmc_sec *sec = imgr->priv;
>> +	int sha_num_bytes, ret;
>> +	u32 magic;
>> +
>> +	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(sec->dev, "%s magic 0x%08x\n", __func__, magic);
>> +
>> +	if ((magic & 0xffff) != exp_magic)
> return -EINVAL ?

The absence of the magic number indicates that a Root Entry Hash has
not been programmed. So a null string (string size of zero) is appropriate
here.

I'll add a comment.

>> +		return 0;
>> +
>> +	sha_num_bytes = ((magic >> 16) & 0xffff) / 8;
>> +
>> +	if (sha_num_bytes != SHA256_REH_SIZE &&
>> +	    sha_num_bytes != SHA384_REH_SIZE)   {
>> +		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
>> +			sha_num_bytes);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return sha_num_bytes;
>> +}
>> +
>> +#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
> Why shouldn't these #defines be collected in the intel-m10-bmc.h ?

Placing them in intel-m10-bmc-h would give them a broader scope and make them
available to all m10-bmc sub-drivers. I placed them here to limit the scope to
the only file that should care about these definitions.

Is this OK - or should they be moved to the header file?

>> +
>> +#define SYSFS_GET_REH_SIZE(_name, _exp_magic, _prog_addr) \
>> +static int get_##_name##_reh_size(struct ifpga_sec_mgr *imgr) \
>> +{ \
>> +	return get_reh_size(imgr, _exp_magic, _prog_addr); \
>> +}
> Is this macro overkill for a 1 liner ?

It gives a slight reduction in code. I could go either way. What do you think? Should I take out the macros? Or leave it as is?

>> +
>> +SYSFS_GET_REH_SIZE(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR)
>> +SYSFS_GET_REH_SIZE(sr, SR_PROG_MAGIC, SR_PROG_ADDR)
>> +SYSFS_GET_REH_SIZE(pr, PR_PROG_MAGIC, PR_PROG_ADDR)
> These and similar below do not directly interact
>
> with the sysfs so the sysfs_ prefix should not be needed.

OK - I'll remove the SYSFS_ prefix.

>
>> +
>> +static int get_root_entry_hash(struct ifpga_sec_mgr *imgr,
>> +			       u32 hash_addr, u8 *hash,
>> +			       unsigned int size)
>> +{
>> +	struct m10bmc_sec *sec = imgr->priv;
>> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>> +	int ret;
>> +
>> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, hash_addr,
>> +				   hash, size / stride);
>> +	if (ret)
>> +		dev_err(sec->dev, "bulk_read of 0x%x failed %d",
>> +			hash_addr, ret);
>> +
>> +	return ret;
>> +}
>> +
>> +#define SYSFS_GET_REH(_name, _hash_addr) \
>> +static int get_##_name##_root_entry_hash(struct ifpga_sec_mgr *imgr, \
>> +					 u8 *hash, unsigned int size) \
>> +{ \
>> +	return get_root_entry_hash(imgr, _hash_addr, hash, size); \
>> +}
>> +
>> +SYSFS_GET_REH(bmc, BMC_REH_ADDR)
>> +SYSFS_GET_REH(sr, SR_REH_ADDR)
>> +SYSFS_GET_REH(pr, PR_REH_ADDR)
>> +
>> +static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>> +	.bmc_root_entry_hash = get_bmc_root_entry_hash,
>> +	.sr_root_entry_hash = get_sr_root_entry_hash,
>> +	.pr_root_entry_hash = get_pr_root_entry_hash,
>> +	.bmc_reh_size = get_bmc_reh_size,
>> +	.sr_reh_size = get_sr_reh_size,
>> +	.pr_reh_size = get_pr_reh_size,
> The prefix of the ops should be similar to the file name.
>
> so consider changing get_pr_reh_size to max10_pr_reh_size

I'll switch to a prefix of m10bmc_ to be consistent with the probe and
remove functions.

>
>> +};
>> +
>> +static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
>> +{
>> +	ifpga_sec_mgr_unregister(sec->imgr);
>> +}
>> +
>> +static int ifpga_sec_mgr_init(struct m10bmc_sec *sec)
>> +{
>> +	struct ifpga_sec_mgr *imgr;
>> +
>> +	imgr = ifpga_sec_mgr_register(sec->dev, "Max10 BMC Security Manager",
>> +				      &m10bmc_iops, sec);
>> +	if (IS_ERR(imgr))
>> +		return PTR_ERR(imgr);
>> +
>> +	sec->imgr = imgr;
>> +	return 0;
>> +}
>> +
>> +static int m10bmc_secure_probe(struct platform_device *pdev)
>> +{
>> +	struct m10bmc_sec *sec;
>> +	int ret;
>> +
>> +	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
>> +	if (!sec)
>> +		return -ENOMEM;
>> +
>> +	sec->dev = &pdev->dev;
>> +	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
>> +	dev_set_drvdata(&pdev->dev, sec);
>> +
>> +	ret = ifpga_sec_mgr_init(sec);
>> +	if (ret)
>> +		dev_err(&pdev->dev,
>> +			"Security manager failed to start: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int m10bmc_secure_remove(struct platform_device *pdev)
>> +{
>> +	struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
>> +
>> +	ifpga_sec_mgr_uinit(sec);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver intel_m10bmc_secure_driver = {
>> +	.probe = m10bmc_secure_probe,
>> +	.remove = m10bmc_secure_remove,
>> +	.driver = {
>> +		.name = "n3000bmc-secure",
> From the filename, should this be "max10bmc-secure" ?

This driver file will also service other devices. A future patch will add support
for the d5005 card and the same driver code will be used, with some conditional
logic, to support the d5005 card under the name "d5005bmc-secure". See the following
link for a preview of these changes.

https://github.com/OPAE/linux-dfl/blob/8e94e1f41c1571a322aac0c8d6ab8ee282e45016/drivers/mfd/intel-m10-bmc-secure.c#L803

I chose to put n3000 in the name now so that the driver name (visible to user
space applications) will not have to change names the d5005 functionality is added.

The source filename is intel-m10-bmc-secure.c. Would it be better to call this driver
n3000-m10bmc-secure? 

Should I introduce the struct platform_device_id array (see the above link) now with
a single entry for the n3000? So that the initial version of this file will contain
the main driver name (intel-m10bmc-secure) as well as the n3000bmc-secure name?

>
>> +	},
>> +};
>> +module_platform_driver(intel_m10bmc_secure_driver);
>> +
>> +MODULE_ALIAS("platform:n3000bmc-secure");
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_DESCRIPTION("Intel MAX10 BMC secure engine");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
>> index d4cb01b73357..7fe465c320c2 100644
>> --- a/include/linux/mfd/intel-m10-bmc.h
>> +++ b/include/linux/mfd/intel-m10-bmc.h
>> @@ -63,6 +63,7 @@ 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
> second '_' should be removed so it reads like
>
> bulk read max10 registers..

Thanks for the comments! I'll fix this.

>
> Tom
>
>>   * m10bmc_sys_read - read m10bmc system register per offset
>>   */
>>  static inline int
>> @@ -79,6 +80,20 @@ 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;
>> +}
>> +
>>  #define m10bmc_sys_read(m10bmc, offset, val) \
>>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>>
Tom Rix Sept. 14, 2020, 8:48 p.m. UTC | #5 
On 9/14/20 12:07 PM, Russ Weight wrote:
>
> On 9/5/20 1:22 PM, Tom Rix wrote:
>> On 9/4/20 4:52 PM, Russ Weight wrote:
>>> Create a platform driver that can be invoked as a sub
>>> driver for the Intel MAX10 BMC in order to support
>>> secure updates. This sub-driver will invoke an
>>> instance of the Intel FPGA Security Manager class driver
>>> in order to expose sysfs interfaces for managing and
>>> monitoring secure updates to FPGA and BMC images.
>>>
>>> This patch creates the MAX10 BMC Security Engine driver and
>>> provides support for displaying the current root entry hashes
>>> for the FPGA static region, the FPGA PR region, and the MAX10
>>> BMC.
>>>
>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>> Reviewed-by: Wu Hao <hao.wu@intel.com>
>>> ---
>>>  drivers/fpga/Kconfig                |  11 ++
>>>  drivers/fpga/Makefile               |   3 +
>>>  drivers/fpga/intel-m10-bmc-secure.c | 170 ++++++++++++++++++++++++++++
>>>  include/linux/mfd/intel-m10-bmc.h   |  15 +++
>>>  4 files changed, 199 insertions(+)
>>>  create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
>>>
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index 97c0a6cc2ba7..0f0bed68e618 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>>>  	  region and for the BMC. Select this option to enable
>>>  	  updates for secure FPGA devices.
>>>  
>>> +config IFPGA_M10_BMC_SECURE
>>> +        tristate "Intel MAX10 BMC security engine"
>>> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
>>> +        help
>>> +          Secure update support for the Intel MAX10 board management
>>> +	  controller.
>>> +
>>> +	  This is a subdriver of the Intel MAX10 board management controller
>>> +	  (BMC) and provides support for secure updates for the BMC image,
>>> +	  the FPGA image, the Root Entry Hashes, etc.
>>> +
>>>  endif # FPGA
>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>> index ec9fbacdedd8..451a23ec3168 100644
>>> --- a/drivers/fpga/Makefile
>>> +++ b/drivers/fpga/Makefile
>>> @@ -24,6 +24,9 @@ obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>>  # Intel FPGA Security Manager Framework
>>>  obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>>>  
>>> +# Intel Security Manager Drivers
>>> +obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.o
>>> +
>>>  # FPGA Bridge Drivers
>>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>>> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
>>> new file mode 100644
>>> index 000000000000..1f86bfb694b4
>>> --- /dev/null
>>> +++ b/drivers/fpga/intel-m10-bmc-secure.c
>>> @@ -0,0 +1,170 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Intel Max10 Board Management Controller Security Engine Driver
>>> + *
>>> + * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
>>> + *
>>> + */
>>> +#include <linux/device.h>
>>> +#include <linux/fpga/ifpga-sec-mgr.h>
>>> +#include <linux/mfd/intel-m10-bmc.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/vmalloc.h>
>>> +
>>> +struct m10bmc_sec {
>>> +	struct device *dev;
>>> +	struct intel_m10bmc *m10bmc;
>>> +	struct ifpga_sec_mgr *imgr;
>>> +};
>>> +
>>> +#define SHA256_REH_SIZE		32
>>> +#define SHA384_REH_SIZE		48
>>> +
>>> +static int get_reh_size(struct ifpga_sec_mgr *imgr,
>>> +			u32 exp_magic, u32 prog_addr)
>>> +{
>>> +	struct m10bmc_sec *sec = imgr->priv;
>>> +	int sha_num_bytes, ret;
>>> +	u32 magic;
>>> +
>>> +	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	dev_dbg(sec->dev, "%s magic 0x%08x\n", __func__, magic);
>>> +
>>> +	if ((magic & 0xffff) != exp_magic)
>> return -EINVAL ?
> The absence of the magic number indicates that a Root Entry Hash has
> not been programmed. So a null string (string size of zero) is appropriate
> here.
>
> I'll add a comment.
>
>>> +		return 0;
>>> +
>>> +	sha_num_bytes = ((magic >> 16) & 0xffff) / 8;
>>> +
>>> +	if (sha_num_bytes != SHA256_REH_SIZE &&
>>> +	    sha_num_bytes != SHA384_REH_SIZE)   {
>>> +		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
>>> +			sha_num_bytes);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return sha_num_bytes;
>>> +}
>>> +
>>> +#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
>> Why shouldn't these #defines be collected in the intel-m10-bmc.h ?
> Placing them in intel-m10-bmc-h would give them a broader scope and make them
> available to all m10-bmc sub-drivers. I placed them here to limit the scope to
> the only file that should care about these definitions.
>
> Is this OK - or should they be moved to the header file?

My thinking keeping all the register defines in one place is

better than spreading it all out.  This would help with possible

future changes that effected the entire register set.

>
>>> +
>>> +#define SYSFS_GET_REH_SIZE(_name, _exp_magic, _prog_addr) \
>>> +static int get_##_name##_reh_size(struct ifpga_sec_mgr *imgr) \
>>> +{ \
>>> +	return get_reh_size(imgr, _exp_magic, _prog_addr); \
>>> +}
>> Is this macro overkill for a 1 liner ?
> It gives a slight reduction in code. I could go either way. What do you think? Should I take out the macros? Or leave it as is?
>
>>> +
>>> +SYSFS_GET_REH_SIZE(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR)
>>> +SYSFS_GET_REH_SIZE(sr, SR_PROG_MAGIC, SR_PROG_ADDR)
>>> +SYSFS_GET_REH_SIZE(pr, PR_PROG_MAGIC, PR_PROG_ADDR)
>> These and similar below do not directly interact
>>
>> with the sysfs so the sysfs_ prefix should not be needed.
> OK - I'll remove the SYSFS_ prefix.
>
>>> +
>>> +static int get_root_entry_hash(struct ifpga_sec_mgr *imgr,
>>> +			       u32 hash_addr, u8 *hash,
>>> +			       unsigned int size)
>>> +{
>>> +	struct m10bmc_sec *sec = imgr->priv;
>>> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>>> +	int ret;
>>> +
>>> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, hash_addr,
>>> +				   hash, size / stride);
>>> +	if (ret)
>>> +		dev_err(sec->dev, "bulk_read of 0x%x failed %d",
>>> +			hash_addr, ret);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +#define SYSFS_GET_REH(_name, _hash_addr) \
>>> +static int get_##_name##_root_entry_hash(struct ifpga_sec_mgr *imgr, \
>>> +					 u8 *hash, unsigned int size) \
>>> +{ \
>>> +	return get_root_entry_hash(imgr, _hash_addr, hash, size); \
>>> +}
>>> +
>>> +SYSFS_GET_REH(bmc, BMC_REH_ADDR)
>>> +SYSFS_GET_REH(sr, SR_REH_ADDR)
>>> +SYSFS_GET_REH(pr, PR_REH_ADDR)
>>> +
>>> +static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>>> +	.bmc_root_entry_hash = get_bmc_root_entry_hash,
>>> +	.sr_root_entry_hash = get_sr_root_entry_hash,
>>> +	.pr_root_entry_hash = get_pr_root_entry_hash,
>>> +	.bmc_reh_size = get_bmc_reh_size,
>>> +	.sr_reh_size = get_sr_reh_size,
>>> +	.pr_reh_size = get_pr_reh_size,
>> The prefix of the ops should be similar to the file name.
>>
>> so consider changing get_pr_reh_size to max10_pr_reh_size
> I'll switch to a prefix of m10bmc_ to be consistent with the probe and
> remove functions.
>
>>> +};
>>> +
>>> +static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
>>> +{
>>> +	ifpga_sec_mgr_unregister(sec->imgr);
>>> +}
>>> +
>>> +static int ifpga_sec_mgr_init(struct m10bmc_sec *sec)
>>> +{
>>> +	struct ifpga_sec_mgr *imgr;
>>> +
>>> +	imgr = ifpga_sec_mgr_register(sec->dev, "Max10 BMC Security Manager",
>>> +				      &m10bmc_iops, sec);
>>> +	if (IS_ERR(imgr))
>>> +		return PTR_ERR(imgr);
>>> +
>>> +	sec->imgr = imgr;
>>> +	return 0;
>>> +}
>>> +
>>> +static int m10bmc_secure_probe(struct platform_device *pdev)
>>> +{
>>> +	struct m10bmc_sec *sec;
>>> +	int ret;
>>> +
>>> +	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
>>> +	if (!sec)
>>> +		return -ENOMEM;
>>> +
>>> +	sec->dev = &pdev->dev;
>>> +	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
>>> +	dev_set_drvdata(&pdev->dev, sec);
>>> +
>>> +	ret = ifpga_sec_mgr_init(sec);
>>> +	if (ret)
>>> +		dev_err(&pdev->dev,
>>> +			"Security manager failed to start: %d\n", ret);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int m10bmc_secure_remove(struct platform_device *pdev)
>>> +{
>>> +	struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
>>> +
>>> +	ifpga_sec_mgr_uinit(sec);
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver intel_m10bmc_secure_driver = {
>>> +	.probe = m10bmc_secure_probe,
>>> +	.remove = m10bmc_secure_remove,
>>> +	.driver = {
>>> +		.name = "n3000bmc-secure",
>> From the filename, should this be "max10bmc-secure" ?
> This driver file will also service other devices. A future patch will add support
> for the d5005 card and the same driver code will be used, with some conditional
> logic, to support the d5005 card under the name "d5005bmc-secure". See the following
> link for a preview of these changes.
>
> https://github.com/OPAE/linux-dfl/blob/8e94e1f41c1571a322aac0c8d6ab8ee282e45016/drivers/mfd/intel-m10-bmc-secure.c#L803
>
> I chose to put n3000 in the name now so that the driver name (visible to user
> space applications) will not have to change names the d5005 functionality is added.
>
> The source filename is intel-m10-bmc-secure.c. Would it be better to call this driver
> n3000-m10bmc-secure? 
>
> Should I introduce the struct platform_device_id array (see the above link) now with
> a single entry for the n3000? So that the initial version of this file will contain
> the main driver name (intel-m10bmc-secure) as well as the n3000bmc-secure name?

Thanks for you explanation that this will also be used by d5005.

I think you have fine as-is.

Thanks

Tom

>
>>> +	},
>>> +};
>>> +module_platform_driver(intel_m10bmc_secure_driver);
>>> +
>>> +MODULE_ALIAS("platform:n3000bmc-secure");
>>> +MODULE_AUTHOR("Intel Corporation");
>>> +MODULE_DESCRIPTION("Intel MAX10 BMC secure engine");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
>>> index d4cb01b73357..7fe465c320c2 100644
>>> --- a/include/linux/mfd/intel-m10-bmc.h
>>> +++ b/include/linux/mfd/intel-m10-bmc.h
>>> @@ -63,6 +63,7 @@ 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
>> second '_' should be removed so it reads like
>>
>> bulk read max10 registers..
> Thanks for the comments! I'll fix this.
>
>> Tom
>>
>>>   * m10bmc_sys_read - read m10bmc system register per offset
>>>   */
>>>  static inline int
>>> @@ -79,6 +80,20 @@ 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;
>>> +}
>>> +
>>>  #define m10bmc_sys_read(m10bmc, offset, val) \
>>>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>>>
Russ Weight Sept. 14, 2020, 9:40 p.m. UTC | #6 
On 9/14/20 1:48 PM, Tom Rix wrote:
> On 9/14/20 12:07 PM, Russ Weight wrote:
>> On 9/5/20 1:22 PM, Tom Rix wrote:
>>> On 9/4/20 4:52 PM, Russ Weight wrote:
>>>> Create a platform driver that can be invoked as a sub
>>>> driver for the Intel MAX10 BMC in order to support
>>>> secure updates. This sub-driver will invoke an
>>>> instance of the Intel FPGA Security Manager class driver
>>>> in order to expose sysfs interfaces for managing and
>>>> monitoring secure updates to FPGA and BMC images.
>>>>
>>>> This patch creates the MAX10 BMC Security Engine driver and
>>>> provides support for displaying the current root entry hashes
>>>> for the FPGA static region, the FPGA PR region, and the MAX10
>>>> BMC.
>>>>
>>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>>> Reviewed-by: Wu Hao <hao.wu@intel.com>
>>>> ---
>>>>  drivers/fpga/Kconfig                |  11 ++
>>>>  drivers/fpga/Makefile               |   3 +
>>>>  drivers/fpga/intel-m10-bmc-secure.c | 170 ++++++++++++++++++++++++++++
>>>>  include/linux/mfd/intel-m10-bmc.h   |  15 +++
>>>>  4 files changed, 199 insertions(+)
>>>>  create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
>>>>
>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>>> index 97c0a6cc2ba7..0f0bed68e618 100644
>>>> --- a/drivers/fpga/Kconfig
>>>> +++ b/drivers/fpga/Kconfig
>>>> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>>>>  	  region and for the BMC. Select this option to enable
>>>>  	  updates for secure FPGA devices.
>>>>  
>>>> +config IFPGA_M10_BMC_SECURE
>>>> +        tristate "Intel MAX10 BMC security engine"
>>>> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
>>>> +        help
>>>> +          Secure update support for the Intel MAX10 board management
>>>> +	  controller.
>>>> +
>>>> +	  This is a subdriver of the Intel MAX10 board management controller
>>>> +	  (BMC) and provides support for secure updates for the BMC image,
>>>> +	  the FPGA image, the Root Entry Hashes, etc.
>>>> +
>>>>  endif # FPGA
>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>>> index ec9fbacdedd8..451a23ec3168 100644
>>>> --- a/drivers/fpga/Makefile
>>>> +++ b/drivers/fpga/Makefile
>>>> @@ -24,6 +24,9 @@ obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>>>  # Intel FPGA Security Manager Framework
>>>>  obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>>>>  
>>>> +# Intel Security Manager Drivers
>>>> +obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.o
>>>> +
>>>>  # FPGA Bridge Drivers
>>>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>>>> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
>>>> new file mode 100644
>>>> index 000000000000..1f86bfb694b4
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/intel-m10-bmc-secure.c
>>>> @@ -0,0 +1,170 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Intel Max10 Board Management Controller Security Engine Driver
>>>> + *
>>>> + * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
>>>> + *
>>>> + */
>>>> +#include <linux/device.h>
>>>> +#include <linux/fpga/ifpga-sec-mgr.h>
>>>> +#include <linux/mfd/intel-m10-bmc.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/vmalloc.h>
>>>> +
>>>> +struct m10bmc_sec {
>>>> +	struct device *dev;
>>>> +	struct intel_m10bmc *m10bmc;
>>>> +	struct ifpga_sec_mgr *imgr;
>>>> +};
>>>> +
>>>> +#define SHA256_REH_SIZE		32
>>>> +#define SHA384_REH_SIZE		48
>>>> +
>>>> +static int get_reh_size(struct ifpga_sec_mgr *imgr,
>>>> +			u32 exp_magic, u32 prog_addr)
>>>> +{
>>>> +	struct m10bmc_sec *sec = imgr->priv;
>>>> +	int sha_num_bytes, ret;
>>>> +	u32 magic;
>>>> +
>>>> +	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	dev_dbg(sec->dev, "%s magic 0x%08x\n", __func__, magic);
>>>> +
>>>> +	if ((magic & 0xffff) != exp_magic)
>>> return -EINVAL ?
>> The absence of the magic number indicates that a Root Entry Hash has
>> not been programmed. So a null string (string size of zero) is appropriate
>> here.
>>
>> I'll add a comment.
>>
>>>> +		return 0;
>>>> +
>>>> +	sha_num_bytes = ((magic >> 16) & 0xffff) / 8;
>>>> +
>>>> +	if (sha_num_bytes != SHA256_REH_SIZE &&
>>>> +	    sha_num_bytes != SHA384_REH_SIZE)   {
>>>> +		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
>>>> +			sha_num_bytes);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return sha_num_bytes;
>>>> +}
>>>> +
>>>> +#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
>>> Why shouldn't these #defines be collected in the intel-m10-bmc.h ?
>> Placing them in intel-m10-bmc-h would give them a broader scope and make them
>> available to all m10-bmc sub-drivers. I placed them here to limit the scope to
>> the only file that should care about these definitions.
>>
>> Is this OK - or should they be moved to the header file?
> My thinking keeping all the register defines in one place is
>
> better than spreading it all out.  This would help with possible
>
> future changes that effected the entire register set.

OK - I'll try to consolidate register definitions in the intel-m10-bmc-h header file.

>
>>>> +
>>>> +#define SYSFS_GET_REH_SIZE(_name, _exp_magic, _prog_addr) \
>>>> +static int get_##_name##_reh_size(struct ifpga_sec_mgr *imgr) \
>>>> +{ \
>>>> +	return get_reh_size(imgr, _exp_magic, _prog_addr); \
>>>> +}
>>> Is this macro overkill for a 1 liner ?
>> It gives a slight reduction in code. I could go either way. What do you think? Should I take out the macros? Or leave it as is?
>>
>>>> +
>>>> +SYSFS_GET_REH_SIZE(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR)
>>>> +SYSFS_GET_REH_SIZE(sr, SR_PROG_MAGIC, SR_PROG_ADDR)
>>>> +SYSFS_GET_REH_SIZE(pr, PR_PROG_MAGIC, PR_PROG_ADDR)
>>> These and similar below do not directly interact
>>>
>>> with the sysfs so the sysfs_ prefix should not be needed.
>> OK - I'll remove the SYSFS_ prefix.
>>
>>>> +
>>>> +static int get_root_entry_hash(struct ifpga_sec_mgr *imgr,
>>>> +			       u32 hash_addr, u8 *hash,
>>>> +			       unsigned int size)
>>>> +{
>>>> +	struct m10bmc_sec *sec = imgr->priv;
>>>> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>>>> +	int ret;
>>>> +
>>>> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, hash_addr,
>>>> +				   hash, size / stride);
>>>> +	if (ret)
>>>> +		dev_err(sec->dev, "bulk_read of 0x%x failed %d",
>>>> +			hash_addr, ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +#define SYSFS_GET_REH(_name, _hash_addr) \
>>>> +static int get_##_name##_root_entry_hash(struct ifpga_sec_mgr *imgr, \
>>>> +					 u8 *hash, unsigned int size) \
>>>> +{ \
>>>> +	return get_root_entry_hash(imgr, _hash_addr, hash, size); \
>>>> +}
>>>> +
>>>> +SYSFS_GET_REH(bmc, BMC_REH_ADDR)
>>>> +SYSFS_GET_REH(sr, SR_REH_ADDR)
>>>> +SYSFS_GET_REH(pr, PR_REH_ADDR)
>>>> +
>>>> +static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>>>> +	.bmc_root_entry_hash = get_bmc_root_entry_hash,
>>>> +	.sr_root_entry_hash = get_sr_root_entry_hash,
>>>> +	.pr_root_entry_hash = get_pr_root_entry_hash,
>>>> +	.bmc_reh_size = get_bmc_reh_size,
>>>> +	.sr_reh_size = get_sr_reh_size,
>>>> +	.pr_reh_size = get_pr_reh_size,
>>> The prefix of the ops should be similar to the file name.
>>>
>>> so consider changing get_pr_reh_size to max10_pr_reh_size
>> I'll switch to a prefix of m10bmc_ to be consistent with the probe and
>> remove functions.
>>
>>>> +};
>>>> +
>>>> +static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
>>>> +{
>>>> +	ifpga_sec_mgr_unregister(sec->imgr);
>>>> +}
>>>> +
>>>> +static int ifpga_sec_mgr_init(struct m10bmc_sec *sec)
>>>> +{
>>>> +	struct ifpga_sec_mgr *imgr;
>>>> +
>>>> +	imgr = ifpga_sec_mgr_register(sec->dev, "Max10 BMC Security Manager",
>>>> +				      &m10bmc_iops, sec);
>>>> +	if (IS_ERR(imgr))
>>>> +		return PTR_ERR(imgr);
>>>> +
>>>> +	sec->imgr = imgr;
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int m10bmc_secure_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct m10bmc_sec *sec;
>>>> +	int ret;
>>>> +
>>>> +	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
>>>> +	if (!sec)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	sec->dev = &pdev->dev;
>>>> +	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
>>>> +	dev_set_drvdata(&pdev->dev, sec);
>>>> +
>>>> +	ret = ifpga_sec_mgr_init(sec);
>>>> +	if (ret)
>>>> +		dev_err(&pdev->dev,
>>>> +			"Security manager failed to start: %d\n", ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int m10bmc_secure_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
>>>> +
>>>> +	ifpga_sec_mgr_uinit(sec);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver intel_m10bmc_secure_driver = {
>>>> +	.probe = m10bmc_secure_probe,
>>>> +	.remove = m10bmc_secure_remove,
>>>> +	.driver = {
>>>> +		.name = "n3000bmc-secure",
>>> From the filename, should this be "max10bmc-secure" ?
>> This driver file will also service other devices. A future patch will add support
>> for the d5005 card and the same driver code will be used, with some conditional
>> logic, to support the d5005 card under the name "d5005bmc-secure". See the following
>> link for a preview of these changes.
>>
>> https://github.com/OPAE/linux-dfl/blob/8e94e1f41c1571a322aac0c8d6ab8ee282e45016/drivers/mfd/intel-m10-bmc-secure.c#L803
>>
>> I chose to put n3000 in the name now so that the driver name (visible to user
>> space applications) will not have to change names the d5005 functionality is added.
>>
>> The source filename is intel-m10-bmc-secure.c. Would it be better to call this driver
>> n3000-m10bmc-secure? 
>>
>> Should I introduce the struct platform_device_id array (see the above link) now with
>> a single entry for the n3000? So that the initial version of this file will contain
>> the main driver name (intel-m10bmc-secure) as well as the n3000bmc-secure name?
> Thanks for you explanation that this will also be used by d5005.
>
> I think you have fine as-is.
>
> Thanks
>
> Tom
>
>>>> +	},
>>>> +};
>>>> +module_platform_driver(intel_m10bmc_secure_driver);
>>>> +
>>>> +MODULE_ALIAS("platform:n3000bmc-secure");
>>>> +MODULE_AUTHOR("Intel Corporation");
>>>> +MODULE_DESCRIPTION("Intel MAX10 BMC secure engine");
>>>> +MODULE_LICENSE("GPL v2");
>>>> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
>>>> index d4cb01b73357..7fe465c320c2 100644
>>>> --- a/include/linux/mfd/intel-m10-bmc.h
>>>> +++ b/include/linux/mfd/intel-m10-bmc.h
>>>> @@ -63,6 +63,7 @@ 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
>>> second '_' should be removed so it reads like
>>>
>>> bulk read max10 registers..
>> Thanks for the comments! I'll fix this.
>>
>>> Tom
>>>
>>>>   * m10bmc_sys_read - read m10bmc system register per offset
>>>>   */
>>>>  static inline int
>>>> @@ -79,6 +80,20 @@ 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;
>>>> +}
>>>> +
>>>>  #define m10bmc_sys_read(m10bmc, offset, val) \
>>>>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>>>>
Moritz Fischer Sept. 16, 2020, 8:33 p.m. UTC | #7 
Russ,

On Fri, Sep 04, 2020 at 04:52:55PM -0700, Russ Weight wrote:
> Create a platform driver that can be invoked as a sub
> driver for the Intel MAX10 BMC in order to support
> secure updates. This sub-driver will invoke an
> instance of the Intel FPGA Security Manager class driver
> in order to expose sysfs interfaces for managing and
> monitoring secure updates to FPGA and BMC images.
> 
> This patch creates the MAX10 BMC Security Engine driver and
> provides support for displaying the current root entry hashes
> for the FPGA static region, the FPGA PR region, and the MAX10
> BMC.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Wu Hao <hao.wu@intel.com>
> ---
>  drivers/fpga/Kconfig                |  11 ++
>  drivers/fpga/Makefile               |   3 +
>  drivers/fpga/intel-m10-bmc-secure.c | 170 ++++++++++++++++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h   |  15 +++
>  4 files changed, 199 insertions(+)
>  create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 97c0a6cc2ba7..0f0bed68e618 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>  	  region and for the BMC. Select this option to enable
>  	  updates for secure FPGA devices.
>  
> +config IFPGA_M10_BMC_SECURE
> +        tristate "Intel MAX10 BMC security engine"
> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
> +        help
> +          Secure update support for the Intel MAX10 board management
> +	  controller.
> +
> +	  This is a subdriver of the Intel MAX10 board management controller
> +	  (BMC) and provides support for secure updates for the BMC image,
> +	  the FPGA image, the Root Entry Hashes, etc.
> +
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index ec9fbacdedd8..451a23ec3168 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -24,6 +24,9 @@ obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>  # Intel FPGA Security Manager Framework
>  obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>  
> +# Intel Security Manager Drivers
> +obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.o
> +
>  # FPGA Bridge Drivers
>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
> new file mode 100644
> index 000000000000..1f86bfb694b4
> --- /dev/null
> +++ b/drivers/fpga/intel-m10-bmc-secure.c
> @@ -0,0 +1,170 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Max10 Board Management Controller Security Engine Driver
> + *
> + * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
> + *
> + */
> +#include <linux/device.h>
> +#include <linux/fpga/ifpga-sec-mgr.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/vmalloc.h>
> +
> +struct m10bmc_sec {
> +	struct device *dev;
> +	struct intel_m10bmc *m10bmc;
> +	struct ifpga_sec_mgr *imgr;
> +};
> +
> +#define SHA256_REH_SIZE		32
> +#define SHA384_REH_SIZE		48
> +
> +static int get_reh_size(struct ifpga_sec_mgr *imgr,
> +			u32 exp_magic, u32 prog_addr)
> +{
> +	struct m10bmc_sec *sec = imgr->priv;
> +	int sha_num_bytes, ret;
> +	u32 magic;
> +
> +	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(sec->dev, "%s magic 0x%08x\n", __func__, magic);
> +
> +	if ((magic & 0xffff) != exp_magic)
Consider using GENMASK() here.
> +		return 0;
> +
> +	sha_num_bytes = ((magic >> 16) & 0xffff) / 8;
And here.
> +
> +	if (sha_num_bytes != SHA256_REH_SIZE &&
> +	    sha_num_bytes != SHA384_REH_SIZE)   {
> +		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
> +			sha_num_bytes);
> +		return -EINVAL;
> +	}
> +
> +	return sha_num_bytes;
> +}
> +
> +#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
> +
> +#define SYSFS_GET_REH_SIZE(_name, _exp_magic, _prog_addr) \
> +static int get_##_name##_reh_size(struct ifpga_sec_mgr *imgr) \
> +{ \
> +	return get_reh_size(imgr, _exp_magic, _prog_addr); \
> +}
> +
> +SYSFS_GET_REH_SIZE(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR)
> +SYSFS_GET_REH_SIZE(sr, SR_PROG_MAGIC, SR_PROG_ADDR)
> +SYSFS_GET_REH_SIZE(pr, PR_PROG_MAGIC, PR_PROG_ADDR)
> +
> +static int get_root_entry_hash(struct ifpga_sec_mgr *imgr,
> +			       u32 hash_addr, u8 *hash,
> +			       unsigned int size)
> +{
> +	struct m10bmc_sec *sec = imgr->priv;
> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
> +	int ret;
> +
> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, hash_addr,
> +				   hash, size / stride);
> +	if (ret)
> +		dev_err(sec->dev, "bulk_read of 0x%x failed %d",
> +			hash_addr, ret);
> +
> +	return ret;
> +}
> +
> +#define SYSFS_GET_REH(_name, _hash_addr) \
> +static int get_##_name##_root_entry_hash(struct ifpga_sec_mgr *imgr, \
> +					 u8 *hash, unsigned int size) \
> +{ \
> +	return get_root_entry_hash(imgr, _hash_addr, hash, size); \
> +}
> +
> +SYSFS_GET_REH(bmc, BMC_REH_ADDR)
> +SYSFS_GET_REH(sr, SR_REH_ADDR)
> +SYSFS_GET_REH(pr, PR_REH_ADDR)

Tbh, I'd drop the macro and just have the functions as one liners,
and yes, please add the prefix.
> +
> +static const struct ifpga_sec_mgr_ops m10bmc_iops = {
> +	.bmc_root_entry_hash = get_bmc_root_entry_hash,
> +	.sr_root_entry_hash = get_sr_root_entry_hash,
> +	.pr_root_entry_hash = get_pr_root_entry_hash,
> +	.bmc_reh_size = get_bmc_reh_size,
> +	.sr_reh_size = get_sr_reh_size,
> +	.pr_reh_size = get_pr_reh_size,
> +};
> +
> +static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
> +{
> +	ifpga_sec_mgr_unregister(sec->imgr);
> +}
Consider dropping this function (see below).
> +
> +static int ifpga_sec_mgr_init(struct m10bmc_sec *sec)
> +{
> +	struct ifpga_sec_mgr *imgr;
> +
> +	imgr = ifpga_sec_mgr_register(sec->dev, "Max10 BMC Security Manager",
> +				      &m10bmc_iops, sec);
> +	if (IS_ERR(imgr))
> +		return PTR_ERR(imgr);
> +
> +	sec->imgr = imgr;
> +	return 0;
> +}
> +
> +static int m10bmc_secure_probe(struct platform_device *pdev)
> +{
> +	struct m10bmc_sec *sec;
> +	int ret;
> +
> +	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
> +	if (!sec)
> +		return -ENOMEM;
> +
> +	sec->dev = &pdev->dev;
> +	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
> +	dev_set_drvdata(&pdev->dev, sec);

If you implement the devm_ versions then this could be:

	imgr = devm_ifpga_sec_mgr_create(&pdev->dev, "Max10 BMC Security
					Manager", &m10bmc_ips, sec);
	if (IS_ERR(imgr))
		return PTR_ERR(imgr);
	sec->imgr = imgr;

	return ifpga_sec_mgr_register(imgr);

> +
> +	ret = ifpga_sec_mgr_init(sec);
> +	if (ret)
> +		dev_err(&pdev->dev,
> +			"Security manager failed to start: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int m10bmc_secure_remove(struct platform_device *pdev)
> +{
> +	struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
> +
> +	ifpga_sec_mgr_uinit(sec);

If you drop the above:
	ifpga_sec_mgr_remove(sec->imgr);
> +	return 0;
> +}
> +
> +static struct platform_driver intel_m10bmc_secure_driver = {
> +	.probe = m10bmc_secure_probe,
> +	.remove = m10bmc_secure_remove,
> +	.driver = {
> +		.name = "n3000bmc-secure",
> +	},
> +};
> +module_platform_driver(intel_m10bmc_secure_driver);
> +
> +MODULE_ALIAS("platform:n3000bmc-secure");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("Intel MAX10 BMC secure engine");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index d4cb01b73357..7fe465c320c2 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -63,6 +63,7 @@ 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_sys_read - read m10bmc system register per offset
>   */
>  static inline int
> @@ -79,6 +80,20 @@ 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;
> +}
> +
>  #define m10bmc_sys_read(m10bmc, offset, val) \
>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>  
> -- 
> 2.17.1
> 

Thanks,
Moritz
Russ Weight Sept. 30, 2020, 11:14 p.m. UTC | #8 
Hi Moritz,

On 9/16/20 1:33 PM, Moritz Fischer wrote:
> Russ,
>
> On Fri, Sep 04, 2020 at 04:52:55PM -0700, Russ Weight wrote:
>> Create a platform driver that can be invoked as a sub
>> driver for the Intel MAX10 BMC in order to support
>> secure updates. This sub-driver will invoke an
>> instance of the Intel FPGA Security Manager class driver
>> in order to expose sysfs interfaces for managing and
>> monitoring secure updates to FPGA and BMC images.
>>
>> This patch creates the MAX10 BMC Security Engine driver and
>> provides support for displaying the current root entry hashes
>> for the FPGA static region, the FPGA PR region, and the MAX10
>> BMC.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> Reviewed-by: Wu Hao <hao.wu@intel.com>
>> ---
>>  drivers/fpga/Kconfig                |  11 ++
>>  drivers/fpga/Makefile               |   3 +
>>  drivers/fpga/intel-m10-bmc-secure.c | 170 ++++++++++++++++++++++++++++
>>  include/linux/mfd/intel-m10-bmc.h   |  15 +++
>>  4 files changed, 199 insertions(+)
>>  create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 97c0a6cc2ba7..0f0bed68e618 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -244,4 +244,15 @@ config IFPGA_SEC_MGR
>>  	  region and for the BMC. Select this option to enable
>>  	  updates for secure FPGA devices.
>>  
>> +config IFPGA_M10_BMC_SECURE
>> +        tristate "Intel MAX10 BMC security engine"
>> +	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
>> +        help
>> +          Secure update support for the Intel MAX10 board management
>> +	  controller.
>> +
>> +	  This is a subdriver of the Intel MAX10 board management controller
>> +	  (BMC) and provides support for secure updates for the BMC image,
>> +	  the FPGA image, the Root Entry Hashes, etc.
>> +
>>  endif # FPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index ec9fbacdedd8..451a23ec3168 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -24,6 +24,9 @@ obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>  # Intel FPGA Security Manager Framework
>>  obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>>  
>> +# Intel Security Manager Drivers
>> +obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.o
>> +
>>  # FPGA Bridge Drivers
>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
>> new file mode 100644
>> index 000000000000..1f86bfb694b4
>> --- /dev/null
>> +++ b/drivers/fpga/intel-m10-bmc-secure.c
>> @@ -0,0 +1,170 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Intel Max10 Board Management Controller Security Engine Driver
>> + *
>> + * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
>> + *
>> + */
>> +#include <linux/device.h>
>> +#include <linux/fpga/ifpga-sec-mgr.h>
>> +#include <linux/mfd/intel-m10-bmc.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/vmalloc.h>
>> +
>> +struct m10bmc_sec {
>> +	struct device *dev;
>> +	struct intel_m10bmc *m10bmc;
>> +	struct ifpga_sec_mgr *imgr;
>> +};
>> +
>> +#define SHA256_REH_SIZE		32
>> +#define SHA384_REH_SIZE		48
>> +
>> +static int get_reh_size(struct ifpga_sec_mgr *imgr,
>> +			u32 exp_magic, u32 prog_addr)
>> +{
>> +	struct m10bmc_sec *sec = imgr->priv;
>> +	int sha_num_bytes, ret;
>> +	u32 magic;
>> +
>> +	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(sec->dev, "%s magic 0x%08x\n", __func__, magic);
>> +
>> +	if ((magic & 0xffff) != exp_magic)
> Consider using GENMASK() here.
Yes - will do.
>> +		return 0;
>> +
>> +	sha_num_bytes = ((magic >> 16) & 0xffff) / 8;
> And here.
Yes
>> +
>> +	if (sha_num_bytes != SHA256_REH_SIZE &&
>> +	    sha_num_bytes != SHA384_REH_SIZE)   {
>> +		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
>> +			sha_num_bytes);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return sha_num_bytes;
>> +}
>> +
>> +#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
>> +
>> +#define SYSFS_GET_REH_SIZE(_name, _exp_magic, _prog_addr) \
>> +static int get_##_name##_reh_size(struct ifpga_sec_mgr *imgr) \
>> +{ \
>> +	return get_reh_size(imgr, _exp_magic, _prog_addr); \
>> +}
>> +
>> +SYSFS_GET_REH_SIZE(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR)
>> +SYSFS_GET_REH_SIZE(sr, SR_PROG_MAGIC, SR_PROG_ADDR)
>> +SYSFS_GET_REH_SIZE(pr, PR_PROG_MAGIC, PR_PROG_ADDR)
>> +
>> +static int get_root_entry_hash(struct ifpga_sec_mgr *imgr,
>> +			       u32 hash_addr, u8 *hash,
>> +			       unsigned int size)
>> +{
>> +	struct m10bmc_sec *sec = imgr->priv;
>> +	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>> +	int ret;
>> +
>> +	ret = m10bmc_raw_bulk_read(sec->m10bmc, hash_addr,
>> +				   hash, size / stride);
>> +	if (ret)
>> +		dev_err(sec->dev, "bulk_read of 0x%x failed %d",
>> +			hash_addr, ret);
>> +
>> +	return ret;
>> +}
>> +
>> +#define SYSFS_GET_REH(_name, _hash_addr) \
>> +static int get_##_name##_root_entry_hash(struct ifpga_sec_mgr *imgr, \
>> +					 u8 *hash, unsigned int size) \
>> +{ \
>> +	return get_root_entry_hash(imgr, _hash_addr, hash, size); \
>> +}
>> +
>> +SYSFS_GET_REH(bmc, BMC_REH_ADDR)
>> +SYSFS_GET_REH(sr, SR_REH_ADDR)
>> +SYSFS_GET_REH(pr, PR_REH_ADDR)
> Tbh, I'd drop the macro and just have the functions as one liners,
> and yes, please add the prefix.
OK - I'll drop the macros and add the prefixes.

>> +
>> +static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>> +	.bmc_root_entry_hash = get_bmc_root_entry_hash,
>> +	.sr_root_entry_hash = get_sr_root_entry_hash,
>> +	.pr_root_entry_hash = get_pr_root_entry_hash,
>> +	.bmc_reh_size = get_bmc_reh_size,
>> +	.sr_reh_size = get_sr_reh_size,
>> +	.pr_reh_size = get_pr_reh_size,
>> +};
>> +
>> +static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
>> +{
>> +	ifpga_sec_mgr_unregister(sec->imgr);
>> +}
> Consider dropping this function (see below).
Sure - I can drop the init and uinit functions.
>> +
>> +static int ifpga_sec_mgr_init(struct m10bmc_sec *sec)
>> +{
>> +	struct ifpga_sec_mgr *imgr;
>> +
>> +	imgr = ifpga_sec_mgr_register(sec->dev, "Max10 BMC Security Manager",
>> +				      &m10bmc_iops, sec);
>> +	if (IS_ERR(imgr))
>> +		return PTR_ERR(imgr);
>> +
>> +	sec->imgr = imgr;
>> +	return 0;
>> +}
>> +
>> +static int m10bmc_secure_probe(struct platform_device *pdev)
>> +{
>> +	struct m10bmc_sec *sec;
>> +	int ret;
>> +
>> +	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
>> +	if (!sec)
>> +		return -ENOMEM;
>> +
>> +	sec->dev = &pdev->dev;
>> +	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
>> +	dev_set_drvdata(&pdev->dev, sec);
> If you implement the devm_ versions then this could be:
>
> 	imgr = devm_ifpga_sec_mgr_create(&pdev->dev, "Max10 BMC Security
> 					Manager", &m10bmc_ips, sec);
> 	if (IS_ERR(imgr))
> 		return PTR_ERR(imgr);
> 	sec->imgr = imgr;
>
> 	return ifpga_sec_mgr_register(imgr);
If ifpga_sec_mgr_register fails, I think it would be necessary to call
"put_device(&imgr->dev)" here to release the device created by
devm_ifpga_sec_mgr_create()?

Thanks,
- Russ
>
>> +
>> +	ret = ifpga_sec_mgr_init(sec);
>> +	if (ret)
>> +		dev_err(&pdev->dev,
>> +			"Security manager failed to start: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int m10bmc_secure_remove(struct platform_device *pdev)
>> +{
>> +	struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
>> +
>> +	ifpga_sec_mgr_uinit(sec);
> If you drop the above:
> 	ifpga_sec_mgr_remove(sec->imgr);
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver intel_m10bmc_secure_driver = {
>> +	.probe = m10bmc_secure_probe,
>> +	.remove = m10bmc_secure_remove,
>> +	.driver = {
>> +		.name = "n3000bmc-secure",
>> +	},
>> +};
>> +module_platform_driver(intel_m10bmc_secure_driver);
>> +
>> +MODULE_ALIAS("platform:n3000bmc-secure");
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_DESCRIPTION("Intel MAX10 BMC secure engine");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
>> index d4cb01b73357..7fe465c320c2 100644
>> --- a/include/linux/mfd/intel-m10-bmc.h
>> +++ b/include/linux/mfd/intel-m10-bmc.h
>> @@ -63,6 +63,7 @@ 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_sys_read - read m10bmc system register per offset
>>   */
>>  static inline int
>> @@ -79,6 +80,20 @@ 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;
>> +}
>> +
>>  #define m10bmc_sys_read(m10bmc, offset, val) \
>>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>>  
>> -- 
>> 2.17.1
>>
> Thanks,
> Moritz
diff mbox series

Patch

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 97c0a6cc2ba7..0f0bed68e618 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -244,4 +244,15 @@  config IFPGA_SEC_MGR
 	  region and for the BMC. Select this option to enable
 	  updates for secure FPGA devices.
 
+config IFPGA_M10_BMC_SECURE
+        tristate "Intel MAX10 BMC security engine"
+	depends on MFD_INTEL_M10_BMC && IFPGA_SEC_MGR
+        help
+          Secure update support for the Intel MAX10 board management
+	  controller.
+
+	  This is a subdriver of the Intel MAX10 board management controller
+	  (BMC) and provides support for secure updates for the BMC image,
+	  the FPGA image, the Root Entry Hashes, etc.
+
 endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index ec9fbacdedd8..451a23ec3168 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -24,6 +24,9 @@  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
 # Intel FPGA Security Manager Framework
 obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
 
+# Intel Security Manager Drivers
+obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.o
+
 # FPGA Bridge Drivers
 obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
 obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
new file mode 100644
index 000000000000..1f86bfb694b4
--- /dev/null
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -0,0 +1,170 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Max10 Board Management Controller Security Engine Driver
+ *
+ * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
+ *
+ */
+#include <linux/device.h>
+#include <linux/fpga/ifpga-sec-mgr.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/vmalloc.h>
+
+struct m10bmc_sec {
+	struct device *dev;
+	struct intel_m10bmc *m10bmc;
+	struct ifpga_sec_mgr *imgr;
+};
+
+#define SHA256_REH_SIZE		32
+#define SHA384_REH_SIZE		48
+
+static int get_reh_size(struct ifpga_sec_mgr *imgr,
+			u32 exp_magic, u32 prog_addr)
+{
+	struct m10bmc_sec *sec = imgr->priv;
+	int sha_num_bytes, ret;
+	u32 magic;
+
+	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
+	if (ret)
+		return ret;
+
+	dev_dbg(sec->dev, "%s magic 0x%08x\n", __func__, magic);
+
+	if ((magic & 0xffff) != exp_magic)
+		return 0;
+
+	sha_num_bytes = ((magic >> 16) & 0xffff) / 8;
+
+	if (sha_num_bytes != SHA256_REH_SIZE &&
+	    sha_num_bytes != SHA384_REH_SIZE)   {
+		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
+			sha_num_bytes);
+		return -EINVAL;
+	}
+
+	return sha_num_bytes;
+}
+
+#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
+
+#define SYSFS_GET_REH_SIZE(_name, _exp_magic, _prog_addr) \
+static int get_##_name##_reh_size(struct ifpga_sec_mgr *imgr) \
+{ \
+	return get_reh_size(imgr, _exp_magic, _prog_addr); \
+}
+
+SYSFS_GET_REH_SIZE(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR)
+SYSFS_GET_REH_SIZE(sr, SR_PROG_MAGIC, SR_PROG_ADDR)
+SYSFS_GET_REH_SIZE(pr, PR_PROG_MAGIC, PR_PROG_ADDR)
+
+static int get_root_entry_hash(struct ifpga_sec_mgr *imgr,
+			       u32 hash_addr, u8 *hash,
+			       unsigned int size)
+{
+	struct m10bmc_sec *sec = imgr->priv;
+	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+	int ret;
+
+	ret = m10bmc_raw_bulk_read(sec->m10bmc, hash_addr,
+				   hash, size / stride);
+	if (ret)
+		dev_err(sec->dev, "bulk_read of 0x%x failed %d",
+			hash_addr, ret);
+
+	return ret;
+}
+
+#define SYSFS_GET_REH(_name, _hash_addr) \
+static int get_##_name##_root_entry_hash(struct ifpga_sec_mgr *imgr, \
+					 u8 *hash, unsigned int size) \
+{ \
+	return get_root_entry_hash(imgr, _hash_addr, hash, size); \
+}
+
+SYSFS_GET_REH(bmc, BMC_REH_ADDR)
+SYSFS_GET_REH(sr, SR_REH_ADDR)
+SYSFS_GET_REH(pr, PR_REH_ADDR)
+
+static const struct ifpga_sec_mgr_ops m10bmc_iops = {
+	.bmc_root_entry_hash = get_bmc_root_entry_hash,
+	.sr_root_entry_hash = get_sr_root_entry_hash,
+	.pr_root_entry_hash = get_pr_root_entry_hash,
+	.bmc_reh_size = get_bmc_reh_size,
+	.sr_reh_size = get_sr_reh_size,
+	.pr_reh_size = get_pr_reh_size,
+};
+
+static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
+{
+	ifpga_sec_mgr_unregister(sec->imgr);
+}
+
+static int ifpga_sec_mgr_init(struct m10bmc_sec *sec)
+{
+	struct ifpga_sec_mgr *imgr;
+
+	imgr = ifpga_sec_mgr_register(sec->dev, "Max10 BMC Security Manager",
+				      &m10bmc_iops, sec);
+	if (IS_ERR(imgr))
+		return PTR_ERR(imgr);
+
+	sec->imgr = imgr;
+	return 0;
+}
+
+static int m10bmc_secure_probe(struct platform_device *pdev)
+{
+	struct m10bmc_sec *sec;
+	int ret;
+
+	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
+	if (!sec)
+		return -ENOMEM;
+
+	sec->dev = &pdev->dev;
+	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
+	dev_set_drvdata(&pdev->dev, sec);
+
+	ret = ifpga_sec_mgr_init(sec);
+	if (ret)
+		dev_err(&pdev->dev,
+			"Security manager failed to start: %d\n", ret);
+
+	return ret;
+}
+
+static int m10bmc_secure_remove(struct platform_device *pdev)
+{
+	struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
+
+	ifpga_sec_mgr_uinit(sec);
+	return 0;
+}
+
+static struct platform_driver intel_m10bmc_secure_driver = {
+	.probe = m10bmc_secure_probe,
+	.remove = m10bmc_secure_remove,
+	.driver = {
+		.name = "n3000bmc-secure",
+	},
+};
+module_platform_driver(intel_m10bmc_secure_driver);
+
+MODULE_ALIAS("platform:n3000bmc-secure");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel MAX10 BMC secure engine");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index d4cb01b73357..7fe465c320c2 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -63,6 +63,7 @@  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_sys_read - read m10bmc system register per offset
  */
 static inline int
@@ -79,6 +80,20 @@  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;
+}
+
 #define m10bmc_sys_read(m10bmc, offset, val) \
 	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)