Date: Tue, 8 Nov 2016 10:32:17 -0800
From: =?utf-8?B?U8O2cmVu?= Brinkmann <soren.brinkmann@xilinx.com>
To: Moritz Fischer <moritz.fischer@ettus.com>
CC: <linux-kernel@vger.kernel.org>, <moritz.fischer.private@gmail.com>,
        <atull@opensource.altera.com>, <michal.simek@xilinx.com>,
        <linux-arm-kernel@lists.infradead.org>, <julia@ni.com>
Subject: Re: [PATCH 3/4] fpga mgr: zynq: Add support for encrypted bitstreams
Message-ID: <20161108183217.GV14444@xsjsorenbubuntu>
References: <20161107001326.7395-1-moritz.fischer@ettus.com>
 <20161107001326.7395-4-moritz.fischer@ettus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20161107001326.7395-4-moritz.fischer@ettus.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Nov 2016 18:32:24.0353
 (UTC)
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.60.100];Helo=[xsj-pvapsmtpgw02]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR02MB014
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3481
Lines: 94

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@ettus.com>
> Cc: Alan Tull <atull@opensource.altera.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Sören Brinkmann <soren.brinkmann@xilinx.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  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.

	Sören