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

[Michal Simek]

On 31.5.2018 08:05, Siva Durga Prasad Paladugu wrote:
> 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.

I have not a problem with this.

M