> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, November 15, 2019 5:54 PM
> To: Zhang, Tianfei <tianfei.zh...@intel.com>; Xu, Rosen <rosen...@intel.com>;
> dev@dpdk.org
> Cc: Pei, Andy <andy....@intel.com>; Ye, Xiaolong <xiaolong...@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...@intel.com>; Zhang, Tianfei
> >> <tianfei.zh...@intel.com>; Pei, Andy <andy....@intel.com>; Ye,
> >> Xiaolong <xiaolong...@intel.com>; Yigit, Ferruh
> >> <ferruh.yi...@intel.com>
> >> Subject: [PATCH v18 13/19] raw/ifpga/base: add secure support
> >>
> >> From: Tianfei zhang <tianfei.zh...@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.zh...@intel.com>
> >> Signed-off-by: Andy Pei <andy....@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
> >
