On 31.5.2018 08:05, Siva Durga Prasad Paladugu wrote: > Hi, > >> -----Original Message----- >> From: Michal Simek [mailto:michal.si...@xilinx.com] >> Sent: Thursday, May 31, 2018 11:29 AM >> To: Siva Durga Prasad Paladugu <siva...@xilinx.com>; u- >> b...@lists.denx.de >> Cc: michal.si...@xilinx.com >> Subject: Re: [PATCH 2/3] cmd: fpga: Add support to load secure bitstreams >> >> On 29.5.2018 16:22, Siva Durga Prasad Paladugu wrote: >>> This patch adds support to load secure bitstreams(authenticated or >>> encrypted or both). As of now, this feature is added and tested only >>> for xilinx bitstreams and the secure bitstream was generated using >>> xilinx bootgen tool, but the command is defined in more generic way. >>> >>> Command example to load authenticated and device key encrypted >>> bitstream is as follows "fpga loads 0 100000 2000000 0 1" >>> >>> Signed-off-by: Siva Durga Prasad Paladugu >>> <siva.durga.palad...@xilinx.com> >>> --- >>> cmd/Kconfig | 7 ++++++ >>> cmd/fpga.c | 62 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> drivers/fpga/fpga.c | 29 +++++++++++++++++++++++++ >>> include/fpga.h | 11 ++++++++++ >>> 4 files changed, 108 insertions(+), 1 deletion(-) >>> >>> diff --git a/cmd/Kconfig b/cmd/Kconfig index 38406fc..9b9eb94 100644 >>> --- a/cmd/Kconfig >>> +++ b/cmd/Kconfig >>> @@ -697,6 +697,13 @@ config CMD_FPGA_LOADP >>> Supports loading an FPGA device from a bitstream buffer >> containing >>> a partial bitstream. >>> >>> +config CMD_FPGA_LOAD_SECURE >>> +bool "fpga loads - loads secure bitstreams (Xilinx only)" >>> +depends on CMD_FPGA >>> +help >>> + Enables the fpga loads command which is used to load secure >>> + (authenticated or encrypted or both) bitstreams on to FPGA. >>> + >>> config CMD_FPGAD >>> bool "fpgad - dump FPGA registers" >>> help >>> diff --git a/cmd/fpga.c b/cmd/fpga.c >>> index 0981826..ad716a0 100644 >>> --- a/cmd/fpga.c >>> +++ b/cmd/fpga.c >>> @@ -27,6 +27,7 @@ enum { >>> FPGA_LOADP, >>> FPGA_LOADBP, >>> FPGA_LOADFS, >>> +FPGA_LOADS, >>> }; >>> >>> /* >>> ------------------------------------------------------------------------- >>> */ @@ -54,6 +55,11 >> @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) >>> fpga_fs_info fpga_fsinfo; >>> fpga_fsinfo.fstype = FS_TYPE_ANY; >>> #endif >>> +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) >>> +struct fpga_secure_info fpga_sec_info; >>> + >>> +memset(&fpga_sec_info, 0, sizeof(fpga_sec_info)); #endif >>> >>> if (devstr) >>> dev = (int) simple_strtoul(devstr, NULL, 16); @@ -80,6 >> +86,19 @@ >>> int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) >>> argc = 5; >>> break; >>> #endif >>> +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) >>> +case FPGA_LOADS: >>> +if (argc < 7) >>> +return CMD_RET_USAGE; >>> +if (argc == 8) >>> +fpga_sec_info.userkey_addr = (u8 *)(uintptr_t) >>> + simple_strtoull(argv[7], >>> + NULL, 16); >>> +fpga_sec_info.encflag = (u8)simple_strtoul(argv[6], NULL, >> 16); >>> +fpga_sec_info.authflag = (u8)simple_strtoul(argv[5], NULL, >> 16); >>> +argc = 5; >>> +break; >>> +#endif >>> default: >>> break; >>> } >>> @@ -150,6 +169,22 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, >> char *const argv[]) >>> if (!fpga_fsinfo.interface || !fpga_fsinfo.dev_part || >>> !fpga_fsinfo.filename) >>> wrong_parms = 1; >>> +break; >>> +#endif >>> +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) >>> +case FPGA_LOADS: >>> +if (fpga_sec_info.authflag >= FPGA_NO_ENC_OR_NO_AUTH >> && >>> + fpga_sec_info.encflag >= FPGA_NO_ENC_OR_NO_AUTH) { >>> +puts("ERR: use <fpga load> for NonSecure >> bitstream\n"); >>> +wrong_parms = 1; >>> +} >>> + >>> +if (fpga_sec_info.encflag == FPGA_ENC_USR_KEY && >>> + !fpga_sec_info.userkey_addr) { >>> +wrong_parms = 1; >>> +puts("ERR:User key not provided\n"); >>> +} >>> +break; >> >> I have created some patches on the top of this series to clean this file >> more. >> And There is no reason to put this checking here. You can simply put it to >> code above and instead of wrong_parms just return CMD_RET_USAGE. > > I thought of putting the validation of received arguments here, that’s why I > placed it here. > Also, used wrong_parms just to maintain consistency with other command > handling. > > Are you taking care of this in your series, if so, will leave it as is in > this series. I think it would be > better if you can move this in your cleanup patches on top of this series if > it didn’t break any > functionality.
I have not a problem with this. M _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot