Re: [PATCH 3/4] fpga mgr: zynq: Add support for encrypted bitstreams

[Moritz Fischer]

Hi SÃren,

On Tue, Nov 8, 2016 at 10:32 AM, SÃren Brinkmann
<soren.brinkmann@xxxxxxxxxx> wrote:
> On Sun, 2016-11-06 at 17:13:25 -0700, Moritz Fischer wrote:
>> Add new flag FPGA_MGR_DECRYPT_BISTREAM as well as a matching
>> capability FPGA_MGR_CAP_DECRYPT to allow for on-the-fly
>> decryption of an encrypted bitstream.
>>
>> If the system is not booted in secure mode AES & HMAC units
>> are disabled by the boot ROM, therefore the capability
>> is not available.
>>
>> Signed-off-by: Moritz Fischer <moritz.fischer@xxxxxxxxx>
>> Cc: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
>> Cc: Michal Simek <michal.simek@xxxxxxxxxx>
>> Cc: SÃren Brinkmann <soren.brinkmann@xxxxxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> ---
>>  drivers/fpga/fpga-mgr.c       |  7 +++++++
>>  drivers/fpga/zynq-fpga.c      | 21 +++++++++++++++++++--
>>  include/linux/fpga/fpga-mgr.h |  2 ++
>>  3 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index 98230b7..e4d08e1 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -61,6 +61,12 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
>>               return -ENOTSUPP;
>>       }
>>
>> +     if (flags & FPGA_MGR_DECRYPT_BITSTREAM &&
>> +         !fpga_mgr_has_cap(FPGA_MGR_CAP_DECRYPT, mgr->caps)) {
>> +             dev_err(dev, "Bitstream decryption not supported\n");
>> +             return -ENOTSUPP;
>> +     }
>> +
>>       /*
>>        * Call the low level driver's write_init function.  This will do the
>>        * device-specific things to get the FPGA into the state where it is
>> @@ -170,6 +176,7 @@ static const char * const state_str[] = {
>>  static const char * const cap_str[] = {
>>       [FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
>>       [FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
>> +     [FPGA_MGR_CAP_DECRYPT] = "Decrypt bitstream on the fly",
>>  };
>>
>>  static ssize_t name_show(struct device *dev,
>> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
>> index 1d37ff0..0aa4705 100644
>> --- a/drivers/fpga/zynq-fpga.c
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -71,6 +71,10 @@
>>  #define CTRL_PCAP_PR_MASK            BIT(27)
>>  /* Enable PCAP */
>>  #define CTRL_PCAP_MODE_MASK          BIT(26)
>> +/* Needed to reduce clock rate for secure config */
>> +#define CTRL_PCAP_RATE_EN_MASK               BIT(25)
>> +/* System booted in secure mode */
>> +#define CTRL_SEC_EN_MASK             BIT(7)
>>
>>  /* Miscellaneous Control Register bit definitions */
>>  /* Internal PCAP loopback */
>> @@ -252,12 +256,20 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
>>
>>       /* set configuration register with following options:
>>        * - enable PCAP interface
>> -      * - set throughput for maximum speed
>> +      * - set throughput for maximum speed (if we're not decrypting)
>>        * - set CPU in user mode
>>        */
>>       ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
>> -     zynq_fpga_write(priv, CTRL_OFFSET,
>> +     if (flags & FPGA_MGR_DECRYPT_BITSTREAM) {
>> +             zynq_fpga_write(priv, CTRL_OFFSET,
>> +                     (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK |
>> +                      CTRL_PCAP_RATE_EN_MASK | ctrl));
>> +
>> +     } else {
>> +             ctrl &= ~CTRL_PCAP_RATE_EN_MASK;
>> +             zynq_fpga_write(priv, CTRL_OFFSET,
>>                       (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl));
>> +     }
>
> Minor nit:
> Assuming that there may be more caps to check to come, wouldn't it be
> slightly easier to write this in a way like?:
>   if (flags & SOME_FLAG)
>      ctrl |= FOO;
>   if (flags & SOME_OTHER_FLAG)
>      ctrl |= BAR;
>   zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>
> i.e. moving the fpga_write outside of the conditionals.

Yeah, will do. Definitely better that way.

Thanks for the review,

Moritz