| Message ID | 1527603765-24481-2-git-send-email-siva.durga.paladugu@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | [U-Boot,1/3] cmd: fpga: Reorder the arguments parsing code | expand |
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.paladugu@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. M
Hi, > -----Original Message----- > From: Michal Simek [mailto:michal.simek@xilinx.com] > Sent: Thursday, May 31, 2018 11:29 AM > To: Siva Durga Prasad Paladugu <sivadur@xilinx.com>; u- > boot@lists.denx.de > Cc: michal.simek@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.paladugu@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. Thanks, Siva > > > M
On 31.5.2018 08:05, Siva Durga Prasad Paladugu wrote: > Hi, > >> -----Original Message----- >> From: Michal Simek [mailto:michal.simek@xilinx.com] >> Sent: Thursday, May 31, 2018 11:29 AM >> To: Siva Durga Prasad Paladugu <sivadur@xilinx.com>; u- >> boot@lists.denx.de >> Cc: michal.simek@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.paladugu@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
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; #endif case FPGA_LOAD: case FPGA_LOADP: @@ -206,6 +241,12 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) break; #endif +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) + case FPGA_LOADS: + rc = fpga_loads(dev, fpga_data, data_size, &fpga_sec_info); + break; +#endif + #if defined(CONFIG_CMD_FPGA_LOADMK) case FPGA_LOADMK: switch (genimg_get_format(fpga_data)) { @@ -339,6 +380,10 @@ static int fpga_get_op(char *opstr) #endif else if (!strcmp("dump", opstr)) op = FPGA_DUMP; +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) + else if (!strcmp("loads", opstr)) + op = FPGA_LOADS; +#endif if (op == FPGA_NONE) printf("Unknown fpga operation \"%s\"\n", opstr); @@ -346,7 +391,7 @@ static int fpga_get_op(char *opstr) return op; } -#if defined(CONFIG_CMD_FPGA_LOADFS) +#if defined(CONFIG_CMD_FPGA_LOADFS) || defined(CONFIG_CMD_FPGA_LOAD_SECURE) U_BOOT_CMD(fpga, 9, 1, do_fpga, #else U_BOOT_CMD(fpga, 6, 1, do_fpga, @@ -381,4 +426,19 @@ U_BOOT_CMD(fpga, 6, 1, do_fpga, "\tsubimage unit name in the form of addr:<subimg_uname>" #endif #endif +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) + "Load encrypted bitstream (Xilinx only)\n" + " loads [dev] [address] [size] [auth-OCM-0/DDR-1/noauth-2]\n" + " [enc-devkey(0)/userkey(1)/nenc(2) [Userkey address]\n" + "Loads the secure bistreams(authenticated/encrypted/both\n" + "authenticated and encrypted) of [size] from [address].\n" + "The auth-OCM/DDR flag specifies to perform authentication\n" + "in OCM or in DDR. 0 for OCM, 1 for DDR, 2 for no authentication.\n" + "The enc flag specifies which key to be used for decryption\n" + "0-device key, 1-user key, 2-no encryption.\n" + "The optional Userkey address specifies from which address key\n" + "has to be used for decryption if user key is selected.\n" + "NOTE: the sceure bitstream has to be created using xilinx\n" + "bootgen tool only.\n" +#endif ); diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c index 55bdf9e..7e8bd7e 100644 --- a/drivers/fpga/fpga.c +++ b/drivers/fpga/fpga.c @@ -217,6 +217,35 @@ int fpga_fsload(int devnum, const void *buf, size_t size, } #endif +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) +int fpga_loads(int devnum, const void *buf, size_t size, + struct fpga_secure_info *fpga_sec_info) +{ + int ret_val = FPGA_FAIL; + + const fpga_desc *desc = fpga_validate(devnum, buf, size, + (char *)__func__); + + if (desc) { + switch (desc->devtype) { + case fpga_xilinx: +#if defined(CONFIG_FPGA_XILINX) + ret_val = xilinx_loads(desc->devdesc, buf, size, + fpga_sec_info); +#else + fpga_no_sup((char *)__func__, "Xilinx devices"); +#endif + break; + default: + printf("%s: Invalid or unsupported device type %d\n", + __func__, desc->devtype); + } + } + + return ret_val; +} +#endif + /* * Generic multiplexing code */ diff --git a/include/fpga.h b/include/fpga.h index f444093..195f0bd 100644 --- a/include/fpga.h +++ b/include/fpga.h @@ -20,6 +20,9 @@ /* device numbers must be non-negative */ #define FPGA_INVALID_DEVICE -1 +#define FPGA_ENC_USR_KEY 1 +#define FPGA_NO_ENC_OR_NO_AUTH 2 + /* root data type defintions */ typedef enum { /* typedef fpga_type */ fpga_min_type, /* range check value */ @@ -42,6 +45,12 @@ typedef struct { /* typedef fpga_desc */ int fstype; } fpga_fs_info; +struct fpga_secure_info { + u8 *userkey_addr; + u8 authflag; + u8 encflag; +}; + typedef enum { BIT_FULL = 0, BIT_PARTIAL, @@ -58,6 +67,8 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype); int fpga_fsload(int devnum, const void *buf, size_t size, fpga_fs_info *fpga_fsinfo); +int fpga_loads(int devnum, const void *buf, size_t size, + struct fpga_secure_info *fpga_sec_info); int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, bitstream_type bstype); int fpga_dump(int devnum, const void *buf, size_t bsize);
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.paladugu@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(-) -- 2.7.4 This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.