Re: [dpdk-dev] [PATCH v18 13/19] raw/ifpga/base: add secure support

["Zhang, Tianfei" <tianfei.zhang@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, November 15, 2019 5:54 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; Xu, Rosen <rosen.xu@intel.com>;
> dev@dpdk.org
> Cc: Pei, Andy <andy.pei@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> Subject: Re: [PATCH v18 13/19] raw/ifpga/base: add secure support
> 
> On 11/14/2019 11:05 PM, Zhang, Tianfei wrote:
> >
> >> -----Original Message-----
> >> From: Xu, Rosen
> >> Sent: Thursday, November 14, 2019 5:03 PM
> >> To: dev@dpdk.org
> >> Cc: Xu, Rosen <rosen.xu@intel.com>; Zhang, Tianfei
> >> <tianfei.zhang@intel.com>; Pei, Andy <andy.pei@intel.com>; Ye,
> >> Xiaolong <xiaolong.ye@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>
> >> Subject: [PATCH v18 13/19] raw/ifpga/base: add secure support
> >>
> >> From: Tianfei zhang <tianfei.zhang@intel.com>
> >>
> >> Add secure max10 device support.
> >
> > In PAC N3000 Card, it implements the secure functionality on the MAX10
> Board Management Controller (BMC) as Root of Trust (RoT). It changes to
> MAX10 (RTL and Nios FW) to enable secure RSU (Remote System Update)
> authentication and integrity checks for FPGA Flat image, and FW updates to the
> card. The card's BMC continues to support features such as power sequence
> management, board monitoring via sensors, JTAG management and in-band SPI
> interface access. The external Flash on the card shall be programmed with the
> Intel root public key hash (for BMC images and FW) and Customer/User key (for
> FPGA Flat image) during manufacturing, so the image updates (RSU) shall be
> verified using ECDSA-256 P-256 and SHA2-256 before being written to the
> MAX10 Image Flash or FPGA Image Flash.
> >
> > This patch add RoT support for MAX10 because some registers and the content
> of Device Tree have changed between RoT solution and Non-RoT solution.
> 
> Thanks Tianfei for the additional information, I will update commit as following,
> please let me know if you have any objection:
> 
> raw/ifpga/base: support max10 security feature
> 
> In PAC N3000 Card, MAX10 Board Management Controller (BMC) implements
> the security functionality.
> 
> Security functionality adds secure Remote System Update (RSU) authentication
> and integrity checks for FPGA flat image, and FW updates to the card.
> 
> This patch adds security feature support for MAX10, in secure solution some
> registers and the content of the Device Tree changes.

It looks good for me, thanks a lot!

> 
> >
> >
> >>
> >> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> >> Signed-off-by: Andy Pei <andy.pei@intel.com>
> >> ---
> >>  drivers/raw/ifpga/base/ifpga_defines.h    |   2 +
> >>  drivers/raw/ifpga/base/ifpga_fme.c        |  26 ++++--
> >>  drivers/raw/ifpga/base/opae_intel_max10.c | 137
> >> +++++++++++++++++++++++++-----
> >> drivers/raw/ifpga/base/opae_intel_max10.h |  80 ++++++++++++-----
> >>  4 files changed, 198 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/raw/ifpga/base/ifpga_defines.h
> >> b/drivers/raw/ifpga/base/ifpga_defines.h
> >> index 8993cc6..1e84b15 100644
> >> --- a/drivers/raw/ifpga/base/ifpga_defines.h
> >> +++ b/drivers/raw/ifpga/base/ifpga_defines.h
> >> @@ -1698,6 +1698,8 @@ struct ifpga_fme_board_info {
> >>  	u32 patch_version;
> >>  	u32 minor_version;
> >>  	u32 major_version;
> >> +	u32 max10_version;
> >> +	u32 nios_fw_version;
> >>  	u32 nums_of_retimer;
> >>  	u32 ports_per_retimer;
> >>  	u32 nums_of_fvl;
> >> diff --git a/drivers/raw/ifpga/base/ifpga_fme.c
> >> b/drivers/raw/ifpga/base/ifpga_fme.c
> >> index 794ca09..87fa596 100644
> >> --- a/drivers/raw/ifpga/base/ifpga_fme.c
> >> +++ b/drivers/raw/ifpga/base/ifpga_fme.c
> >> @@ -825,6 +825,7 @@ static int board_type_to_info(u32 type,  static
> >> int fme_get_board_interface(struct ifpga_fme_hw *fme)  {
> >>  	struct fme_bitstream_id id;
> >> +	u32 val;
> >>
> >>  	if (fme_hdr_get_bitstream_id(fme, &id.id))
> >>  		return -EINVAL;
> >> @@ -850,6 +851,18 @@ static int fme_get_board_interface(struct
> >> ifpga_fme_hw *fme)
> >>  			fme->board_info.nums_of_fvl,
> >>  			fme->board_info.ports_per_fvl);
> >>
> >> +	if (max10_sys_read(MAX10_BUILD_VER, &val))
> >> +		return -EINVAL;
> >> +	fme->board_info.max10_version = val & 0xffffff;
> >> +
> >> +	if (max10_sys_read(NIOS2_FW_VERSION, &val))
> >> +		return -EINVAL;
> >> +	fme->board_info.nios_fw_version = val & 0xffffff;
> >> +
> >> +	dev_info(fme, "max10 version 0x%x, nios fw version 0x%x\n",
> >> +		fme->board_info.max10_version,
> >> +		fme->board_info.nios_fw_version);
> >> +
> >>  	return 0;
> >>  }
> >>
> >> @@ -858,16 +871,11 @@ static int spi_self_checking(void)
> >>  	u32 val;
> >>  	int ret;
> >>
> >> -	ret = max10_reg_read(0x30043c, &val);
> >> +	ret = max10_sys_read(MAX10_TEST_REG, &val);
> >>  	if (ret)
> >>  		return -EIO;
> >>
> >> -	if (val != 0x87654321) {
> >> -		dev_err(NULL, "Read MAX10 test register fail: 0x%x\n", val);
> >> -		return -EIO;
> >> -	}
> >> -
> >> -	dev_info(NULL, "Read MAX10 test register success, SPI self-test done\n");
> >> +	dev_info(NULL, "Read MAX10 test register 0x%x\n", val);
> >>
> >>  	return 0;
> >>  }
> >> @@ -1283,7 +1291,7 @@ int fme_mgr_get_retimer_status(struct
> >> ifpga_fme_hw *fme,
> >>  	if (!dev)
> >>  		return -ENODEV;
> >>
> >> -	if (max10_reg_read(PKVL_LINK_STATUS, &val)) {
> >> +	if (max10_sys_read(PKVL_LINK_STATUS, &val)) {
> >>  		dev_err(dev, "%s: read pkvl status fail\n", __func__);
> >>  		return -EINVAL;
> >>  	}
> >> @@ -1311,7 +1319,7 @@ int fme_mgr_get_sensor_value(struct
> >> ifpga_fme_hw *fme,
> >>  	if (!dev)
> >>  		return -ENODEV;
> >>
> >> -	if (max10_reg_read(sensor->value_reg, value)) {
> >> +	if (max10_sys_read(sensor->value_reg, value)) {
> >>  		dev_err(dev, "%s: read sensor value register 0x%x fail\n",
> >>  				__func__, sensor->value_reg);
> >>  		return -EINVAL;
> >> diff --git a/drivers/raw/ifpga/base/opae_intel_max10.c
> >> b/drivers/raw/ifpga/base/opae_intel_max10.c
> >> index ae7a8df..748ab56 100644
> >> --- a/drivers/raw/ifpga/base/opae_intel_max10.c
> >> +++ b/drivers/raw/ifpga/base/opae_intel_max10.c
> >> @@ -30,6 +30,22 @@ int max10_reg_write(unsigned int reg, unsigned int
> val)
> >>  			reg, 4, (unsigned char *)&tmp);
> >>  }
> >>
> >> +int max10_sys_read(unsigned int offset, unsigned int *val) {
> >> +	if (!g_max10)
> >> +		return -ENODEV;
> >> +
> >> +	return max10_reg_read(g_max10->base + offset, val); }
> >> +
> >> +int max10_sys_write(unsigned int offset, unsigned int val) {
> >> +	if (!g_max10)
> >> +		return -ENODEV;
> >> +
> >> +	return max10_reg_write(g_max10->base + offset, val); }
> >> +
> >>  static struct max10_compatible_id max10_id_table[] = {
> >>  	{.compatible = MAX10_PAC,},
> >>  	{.compatible = MAX10_PAC_N3000,},
> >> @@ -66,7 +82,8 @@ static void max10_check_capability(struct
> >> intel_max10_device *max10)
> >>  		max10->flags |= MAX10_FLAGS_NO_I2C2 |
> >>  				MAX10_FLAGS_NO_BMCIMG_FLASH;
> >>  		dev_info(max10, "found %s card\n", max10->id->compatible);
> >> -	}
> >> +	} else
> >> +		max10->flags |= MAX10_FLAGS_MAC_CACHE;
> >>  }
> >>
> >>  static int altera_nor_flash_read(u32 offset, @@ -100,7 +117,7 @@
> >> static int enable_nor_flash(bool on)
> >>  	unsigned int val = 0;
> >>  	int ret;
> >>
> >> -	ret = max10_reg_read(RSU_REG_OFF, &val);
> >> +	ret = max10_sys_read(RSU_REG, &val);
> >>  	if (ret) {
> >>  		dev_err(NULL "enabling flash error\n");
> >>  		return ret;
> >> @@ -111,7 +128,7 @@ static int enable_nor_flash(bool on)
> >>  	else
> >>  		val &= ~RSU_ENABLE;
> >>
> >> -	return max10_reg_write(RSU_REG_OFF, val);
> >> +	return max10_sys_write(RSU_REG, val);
> >>  }
> >>
> >>  static int init_max10_device_table(struct intel_max10_device *max10)
> >> @@
> >> -123,7 +140,7 @@ static int init_max10_device_table(struct
> >> intel_max10_device *max10)
> >>  	u32 dt_size, dt_addr, val;
> >>  	int ret;
> >>
> >> -	ret = max10_reg_read(DT_AVAIL_REG_OFF, &val);
> >> +	ret = max10_sys_read(DT_AVAIL_REG, &val);
> >>  	if (ret) {
> >>  		dev_err(max10 "cannot read DT_AVAIL_REG\n");
> >>  		return ret;
> >> @@ -134,7 +151,7 @@ static int init_max10_device_table(struct
> >> intel_max10_device *max10)
> >>  		return -EINVAL;
> >>  	}
> >>
> >> -	ret = max10_reg_read(DT_BASE_ADDR_REG_OFF, &dt_addr);
> >> +	ret = max10_sys_read(DT_BASE_ADDR_REG, &dt_addr);
> >>  	if (ret) {
> >>  		dev_info(max10 "cannot get base addr of device table\n");
> >>  		return ret;
> >> @@ -315,7 +332,7 @@ static int max10_add_sensor(struct
> >> raw_sensor_info *info,
> >>  		if (!sensor_reg_valid(&info->regs[i]))
> >>  			continue;
> >>
> >> -		ret = max10_reg_read(info->regs[i].regoff, &val);
> >> +		ret = max10_sys_read(info->regs[i].regoff, &val);
> >>  		if (ret)
> >>  			break;
> >>
> >> @@ -355,7 +372,8 @@ static int max10_add_sensor(struct
> >> raw_sensor_info *info,
> >>  	return ret;
> >>  }
> >>
> >> -static int max10_sensor_init(struct intel_max10_device *dev)
> >> +static int
> >> +max10_sensor_init(struct intel_max10_device *dev, int parent)
> >>  {
> >>  	int i, ret = 0, offset = 0;
> >>  	const fdt32_t *num;
> >> @@ -370,7 +388,7 @@ static int max10_sensor_init(struct
> >> intel_max10_device
> >> *dev)
> >>  		return 0;
> >>  	}
> >>
> >> -	fdt_for_each_subnode(offset, fdt_root, 0) {
> >> +	fdt_for_each_subnode(offset, fdt_root, parent) {
> >>  		ptr = fdt_get_name(fdt_root, offset, NULL);
> >>  		if (!ptr) {
> >>  			dev_err(dev, "failed to fdt get name\n"); @@ -417,7
> +435,16 @@
> >> static int max10_sensor_init(struct intel_max10_device *dev)
> >>  				continue;
> >>  			}
> >>
> >> -			raw->regs[i].regoff = start;
> >> +			/* This is a hack to compatible with non-secure
> >> +			 * solution. If sensors are included in root node,
> >> +			 * then it's non-secure dtb, which use absolute addr
> >> +			 * of non-secure solution.
> >> +			 */
> >> +			if (parent)
> >> +				raw->regs[i].regoff = start;
> >> +			else
> >> +				raw->regs[i].regoff = start -
> >> +					MAX10_BASE_ADDR;
> >>  			raw->regs[i].size = size;
> >>  		}
> >>
> >> @@ -469,6 +496,63 @@ static int max10_sensor_init(struct
> >> intel_max10_device *dev)
> >>  	return ret;
> >>  }
> >>
> >> +static int check_max10_version(struct intel_max10_device *dev) {
> >> +	unsigned int v;
> >> +
> >> +	if (!max10_reg_read(MAX10_SEC_BASE_ADDR + MAX10_BUILD_VER,
> >> +				&v)) {
> >> +		if (v != 0xffffffff) {
> >> +			dev_info(dev, "secure MAX10 detected\n");
> >> +			dev->base = MAX10_SEC_BASE_ADDR;
> >> +			dev->flags |= MAX10_FLAGS_SECURE;
> >> +		} else {
> >> +			dev_info(dev, "non-secure MAX10 detected\n");
> >> +			dev->base = MAX10_BASE_ADDR;
> >> +		}
> >> +		return 0;
> >> +	}
> >> +
> >> +	return -ENODEV;
> >> +}
> >> +
> >> +static int
> >> +max10_secure_hw_init(struct intel_max10_device *dev) {
> >> +	int offset, sysmgr_offset = 0;
> >> +	char *fdt_root;
> >> +
> >> +	fdt_root = dev->fdt_root;
> >> +	if (!fdt_root) {
> >> +		dev_debug(dev, "skip init as not find Device Tree\n");
> >> +		return 0;
> >> +	}
> >> +
> >> +	fdt_for_each_subnode(offset, fdt_root, 0) {
> >> +		if (!fdt_node_check_compatible(fdt_root, offset,
> >> +					"intel-max10,system-manager")) {
> >> +			sysmgr_offset = offset;
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	max10_check_capability(dev);
> >> +
> >> +	max10_sensor_init(dev, sysmgr_offset);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int
> >> +max10_non_secure_hw_init(struct intel_max10_device *dev) {
> >> +	max10_check_capability(dev);
> >> +
> >> +	max10_sensor_init(dev, 0);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  struct intel_max10_device *
> >>  intel_max10_device_probe(struct altera_spi_device *spi,
> >>  		int chipselect)
> >> @@ -492,32 +576,47 @@ struct intel_max10_device *
> >>  	/* set the max10 device firstly */
> >>  	g_max10 = dev;
> >>
> >> -	/* init the MAX10 device table */
> >> +	/* check the max10 version */
> >> +	ret = check_max10_version(dev);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to find max10 hardware!\n");
> >> +		goto free_dev;
> >> +	}
> >> +
> >> +	/* load the MAX10 device table */
> >>  	ret = init_max10_device_table(dev);
> >>  	if (ret) {
> >> -		dev_err(dev, "init max10 device table fail\n");
> >> +		dev_err(dev, "Init max10 device table fail\n");
> >>  		goto free_dev;
> >>  	}
> >>
> >> -	max10_check_capability(dev);
> >> +	/* init max10 devices, like sensor*/
> >> +	if (dev->flags & MAX10_FLAGS_SECURE)
> >> +		ret = max10_secure_hw_init(dev);
> >> +	else
> >> +		ret = max10_non_secure_hw_init(dev);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to init max10 hardware!\n");
> >> +		goto free_dtb;
> >> +	}
> >>
> >>  	/* read FPGA loading information */
> >> -	ret = max10_reg_read(FPGA_PAGE_INFO_OFF, &val);
> >> +	ret = max10_sys_read(FPGA_PAGE_INFO, &val);
> >>  	if (ret) {
> >>  		dev_err(dev, "fail to get FPGA loading info\n");
> >> -		goto spi_tran_fail;
> >> +		goto release_max10_hw;
> >>  	}
> >>  	dev_info(dev, "FPGA loaded from %s Image\n", val ? "User" :
> >> "Factory");
> >>
> >> -
> >> -	max10_sensor_init(dev);
> >> -
> >>  	return dev;
> >>
> >> -spi_tran_fail:
> >> +release_max10_hw:
> >> +	max10_sensor_uinit();
> >> +free_dtb:
> >>  	if (dev->fdt_root)
> >>  		opae_free(dev->fdt_root);
> >> -	spi_transaction_remove(dev->spi_tran_dev);
> >> +	if (dev->spi_tran_dev)
> >> +		spi_transaction_remove(dev->spi_tran_dev);
> >>  free_dev:
> >>  	g_max10 = NULL;
> >>  	opae_free(dev);
> >> diff --git a/drivers/raw/ifpga/base/opae_intel_max10.h
> >> b/drivers/raw/ifpga/base/opae_intel_max10.h
> >> index 90bf098..e632941 100644
> >> --- a/drivers/raw/ifpga/base/opae_intel_max10.h
> >> +++ b/drivers/raw/ifpga/base/opae_intel_max10.h
> >> @@ -23,6 +23,8 @@ struct max10_compatible_id {
> >>  #define MAX10_FLAGS_SPI                 BIT(3)
> >>  #define MAX10_FLGAS_NIOS_SPI            BIT(4)
> >>  #define MAX10_FLAGS_PKVL                BIT(5)
> >> +#define MAX10_FLAGS_SECURE		BIT(6)
> >> +#define MAX10_FLAGS_MAC_CACHE		BIT(7)
> >>
> >>  struct intel_max10_device {
> >>  	unsigned int flags; /*max10 hardware capability*/ @@ -30,6 +32,7
> @@
> >> struct intel_max10_device {
> >>  	struct spi_transaction_dev *spi_tran_dev;
> >>  	struct max10_compatible_id *id; /*max10 compatible*/
> >>  	char *fdt_root;
> >> +	unsigned int base; /* max10 base address */
> >>  };
> >>
> >>  /* retimer speed */
> >> @@ -74,30 +77,69 @@ struct opae_retimer_status {  #define FLASH_BASE
> >> 0x10000000  #define FLASH_OPTION_BITS 0x10000
> >>
> >> -#define NIOS2_FW_VERSION_OFF   0x300400
> >> -#define RSU_REG_OFF            0x30042c
> >> -#define FPGA_RP_LOAD		BIT(3)
> >> -#define NIOS2_PRERESET		BIT(4)
> >> -#define NIOS2_HANG		BIT(5)
> >> -#define RSU_ENABLE		BIT(6)
> >> -#define NIOS2_RESET		BIT(7)
> >> -#define NIOS2_I2C2_POLL_STOP	BIT(13)
> >> -#define FPGA_RECONF_REG_OFF	0x300430
> >> -#define COUNTDOWN_START		BIT(18)
> >> -#define MAX10_BUILD_VER_OFF	0x300468
> >> -#define PCB_INFO		GENMASK(31, 24)
> >> -#define MAX10_BUILD_VERION	GENMASK(23, 0)
> >> -#define FPGA_PAGE_INFO_OFF	0x30046c
> >> -#define DT_AVAIL_REG_OFF	0x300490
> >> -#define DT_AVAIL		BIT(0)
> >> -#define DT_BASE_ADDR_REG_OFF	0x300494
> >> -#define PKVL_POLLING_CTRL       0x300480
> >> -#define PKVL_LINK_STATUS        0x300564
> >> +/* System Registers */
> >> +#define MAX10_BASE_ADDR		0x300400
> >> +#define MAX10_SEC_BASE_ADDR	0x300800
> >> +/* Register offset of system registers */
> >> +#define NIOS2_FW_VERSION	0x0
> >> +#define MAX10_MACADDR1		0x10
> >> +#define   MAX10_MAC_BYTE4	GENMASK(7, 0)
> >> +#define   MAX10_MAC_BYTE3	GENMASK(15, 8)
> >> +#define   MAX10_MAC_BYTE2	GENMASK(23, 16)
> >> +#define   MAX10_MAC_BYTE1	GENMASK(31, 24)
> >> +#define MAX10_MACADDR2		0x14
> >> +#define   MAX10_MAC_BYTE6	GENMASK(7, 0)
> >> +#define   MAX10_MAC_BYTE5	GENMASK(15, 8)
> >> +#define   MAX10_MAC_COUNT	GENMASK(23, 16)
> >> +#define RSU_REG			0x2c
> >> +#define   FPGA_RECONF_PAGE	GENMASK(2, 0)
> >> +#define   FPGA_RP_LOAD		BIT(3)
> >> +#define   NIOS2_PRERESET	BIT(4)
> >> +#define   NIOS2_HANG		BIT(5)
> >> +#define   RSU_ENABLE		BIT(6)
> >> +#define   NIOS2_RESET		BIT(7)
> >> +#define   NIOS2_I2C2_POLL_STOP	BIT(13)
> >> +#define   PKVL_EEPROM_LOAD	BIT(31)
> >> +#define FPGA_RECONF_REG		0x30
> >> +#define MAX10_TEST_REG		0x3c
> >> +#define   COUNTDOWN_START	BIT(18)
> >> +#define MAX10_BUILD_VER		0x68
> >> +#define   MAX10_VERSION_MAJOR	GENMASK(23, 16)
> >> +#define   PCB_INFO		GENMASK(31, 24)
> >> +#define FPGA_PAGE_INFO		0x6c
> >> +#define DT_AVAIL_REG		0x90
> >> +#define   DT_AVAIL		BIT(0)
> >> +#define DT_BASE_ADDR_REG	0x94
> >> +#define MAX10_DOORBELL		0x400
> >> +#define   RSU_REQUEST		BIT(0)
> >> +#define   SEC_PROGRESS		GENMASK(7, 4)
> >> +#define   HOST_STATUS		GENMASK(11, 8)
> >> +#define   SEC_STATUS		GENMASK(23, 16)
> >> +
> >> +/* PKVL related registers, in system register region */
> >> +#define PKVL_POLLING_CTRL		0x80
> >> +#define   POLLING_MODE			GENMASK(15, 0)
> >> +#define   PKVL_A_PRELOAD		BIT(16)
> >> +#define   PKVL_A_PRELOAD_TIMEOUT	BIT(17)
> >> +#define   PKVL_A_DATA_TOO_BIG		BIT(18)
> >> +#define   PKVL_A_HDR_CHECKSUM		BIT(20)
> >> +#define   PKVL_B_PRELOAD		BIT(24)
> >> +#define   PKVL_B_PRELOAD_TIMEOUT	BIT(25)
> >> +#define   PKVL_B_DATA_TOO_BIG		BIT(26)
> >> +#define   PKVL_B_HDR_CHECKSUM		BIT(28)
> >> +#define   PKVL_EEPROM_UPG_STATUS	GENMASK(31, 16)
> >> +#define PKVL_LINK_STATUS		0x164
> >> +#define PKVL_A_VERSION			0x254
> >> +#define PKVL_B_VERSION			0x258
> >> +#define   SERDES_VERSION		GENMASK(15, 0)
> >> +#define   SBUS_VERSION			GENMASK(31, 16)
> >>
> >>  #define DFT_MAX_SIZE		0x7e0000
> >>
> >>  int max10_reg_read(unsigned int reg, unsigned int *val);  int
> >> max10_reg_write(unsigned int reg, unsigned int val);
> >> +int max10_sys_read(unsigned int offset, unsigned int *val); int
> >> +max10_sys_write(unsigned int offset, unsigned int val);
> >>  struct intel_max10_device *
> >>  intel_max10_device_probe(struct altera_spi_device *spi,
> >>  		int chipselect);
> >> --
> >> 1.8.3.1
> >