[U-Boot] [PATCH 2/3] cmd: fpga: Add support to load secure bitstreams

[Siva Durga Prasad Paladugu]

Hi,

> -----Original Message-----
> From: Michal Simek [mailto:michal.simek at xilinx.com]
> Sent: Thursday, May 31, 2018 11:29 AM
> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>; u-
> boot at lists.denx.de
> Cc: michal.simek at 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 at 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