[PATCH 0/4] fpga mgr: Add support for capabilities & encrypted bistreams

[PATCH 0/4] fpga mgr: Add support for capabilities & encrypted bistreams

[Moritz Fischer]

This patchset introduces FPGA capabilities, to model what operations
an FPGA Manager low level driver supports.
Exposing them via sysfs allows userland to make smart decisions which FPGA to deploy
an image to in case there are multiple FPGAs with different capabilities available.

As an example of a capability: Support for encrypted bitstreams to Zynq & SocFPGA.
Please note that [4/4] is separate as I couldn't find info on how encrypted bitstreams
work on socfpga platforms, however Alan mentioned during an offline discussion it should
'just work'.

On Zynq using the HMAC & AES units in the devcfg peripheral only work when the system
has been booted in Secure Mode, otherwise the boot rom will disable them.
The FPGA manager capability will only get exposed if available.

Thanks,

    Moritz

Moritz Fischer (4):
  fpga mgr: Introduce FPGA capabilities
  fpga mgr: Expose FPGA capabilities to userland via sysfs
  fpga mgr: zynq: Add support for encrypted bitstreams
  fpga mgr: socfpga: Expose support for encrypted bitstreams

 Documentation/ABI/testing/sysfs-class-fpga-manager | 16 +++++++
 drivers/fpga/fpga-mgr.c                            | 42 ++++++++++++++++++
 drivers/fpga/socfpga.c                             | 11 ++---
 drivers/fpga/zynq-fpga.c                           | 28 ++++++++++--
 include/linux/fpga/fpga-mgr.h                      | 50 +++++++++++++++++++++-
 5 files changed, 138 insertions(+), 9 deletions(-)

-- 
2.10.0

[PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[Moritz Fischer]

Add FPGA capabilities as a way to express the capabilities
of a given FPGA manager.

Removes code duplication by comparing the low-level driver's
capabilities at the framework level rather than having each driver
check for supported operations in the write_init() callback.

This allows for extending with additional capabilities, similar
to the the dmaengine framework's implementation.

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
---

Changes from RFC:
* in the RFC the caps weren't actually stored into the struct fpga_mgr

Note:

If people disagree on the typedef being a 'false positive' I can fix
that in a future rev of the patchset.

Thanks,

    Moritz

---
 drivers/fpga/fpga-mgr.c       | 15 ++++++++++++++
 drivers/fpga/socfpga.c        | 10 +++++-----
 drivers/fpga/zynq-fpga.c      |  7 ++++++-
 include/linux/fpga/fpga-mgr.h | 46 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 953dc91..ed57c17 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
 	struct device *dev = &mgr->dev;
 	int ret;
 
+	if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
+	    !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
+		dev_err(dev, "Partial reconfiguration not supported\n");
+		return -ENOTSUPP;
+	}
+
+	if (flags & FPGA_MGR_FULL_RECONFIG &&
+	    !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
+		dev_err(dev, "Full reconfiguration 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
@@ -245,12 +257,14 @@ EXPORT_SYMBOL_GPL(fpga_mgr_put);
  * @dev:	fpga manager device from pdev
  * @name:	fpga manager name
  * @mops:	pointer to structure of fpga manager ops
+ * @caps:	fpga manager capabilities
  * @priv:	fpga manager private data
  *
  * Return: 0 on success, negative error code otherwise.
  */
 int fpga_mgr_register(struct device *dev, const char *name,
 		      const struct fpga_manager_ops *mops,
+		      fpga_mgr_cap_mask_t caps,
 		      void *priv)
 {
 	struct fpga_manager *mgr;
@@ -282,6 +296,7 @@ int fpga_mgr_register(struct device *dev, const char *name,
 	mgr->name = name;
 	mgr->mops = mops;
 	mgr->priv = priv;
+	mgr->caps = caps;
 
 	/*
 	 * Initialize framework state by requesting low level driver read state
diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index 27d2ff2..fd9760c 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -413,10 +413,6 @@ static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
 	struct socfpga_fpga_priv *priv = mgr->priv;
 	int ret;
 
-	if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
-		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
-		return -EINVAL;
-	}
 	/* Steps 1 - 5: Reset the FPGA */
 	ret = socfpga_fpga_reset(mgr);
 	if (ret)
@@ -555,6 +551,7 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct socfpga_fpga_priv *priv;
 	struct resource *res;
+	fpga_mgr_cap_mask_t caps;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -580,8 +577,11 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	fpga_mgr_cap_zero(&caps);
+	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
+
 	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
-				 &socfpga_fpga_ops, priv);
+				 &socfpga_fpga_ops, caps, priv);
 }
 
 static int socfpga_fpga_remove(struct platform_device *pdev)
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index c2fb412..1d37ff0 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -410,6 +410,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct zynq_fpga_priv *priv;
 	struct resource *res;
+	fpga_mgr_cap_mask_t caps;
 	int err;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -461,9 +462,13 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
 
 	clk_disable(priv->clk);
+	fpga_mgr_cap_zero(&caps);
+	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
+	fpga_mgr_cap_set(FPGA_MGR_CAP_PARTIAL_RECONF, caps);
+
 
 	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
-				&zynq_fpga_ops, priv);
+				&zynq_fpga_ops, caps, priv);
 	if (err) {
 		dev_err(dev, "unable to register FPGA manager");
 		clk_unprepare(priv->clk);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 0940bf4..e73429c 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -67,6 +67,47 @@ enum fpga_mgr_states {
  * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported
  */
 #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
+#define FPGA_MGR_FULL_RECONFIG		BIT(1)
+
+enum fpga_mgr_capability {
+	FPGA_MGR_CAP_PARTIAL_RECONF,
+	FPGA_MGR_CAP_FULL_RECONF,
+
+/* last capability type for creation of the capabilities mask */
+	FPGA_MGR_CAP_END,
+};
+
+typedef struct { DECLARE_BITMAP(bits, FPGA_MGR_CAP_END); } fpga_mgr_cap_mask_t;
+
+#define fpga_mgr_has_cap(cap, mask) __fpga_mgr_has_cap((cap), &(mask))
+static inline int __fpga_mgr_has_cap(enum fpga_mgr_capability cap,
+				     fpga_mgr_cap_mask_t *mask)
+{
+	return test_bit(cap, mask->bits);
+}
+
+#define fpga_mgr_cap_zero(mask) __fpga_mgr_cap_zero(mask)
+static inline void __fpga_mgr_cap_zero(fpga_mgr_cap_mask_t *mask)
+{
+	bitmap_zero(mask->bits, FPGA_MGR_CAP_END);
+}
+
+#define fpga_mgr_cap_clear(cap, mask) __fpga_mgr_cap_clear((cap), &(mask))
+static inline void __fpga_mgr_cap_clear(enum fpga_mgr_capability cap,
+				       fpga_mgr_cap_mask_t *mask)
+
+{
+	clear_bit(cap, mask->bits);
+}
+
+#define fpga_mgr_cap_set(cap, mask) __fpga_mgr_cap_set((cap), &(mask))
+static inline void __fpga_mgr_cap_set(enum fpga_mgr_capability cap,
+				      fpga_mgr_cap_mask_t *mask)
+
+{
+	set_bit(cap, mask->bits);
+}
+
 
 /**
  * struct fpga_manager_ops - ops for low level fpga manager drivers
@@ -105,6 +146,7 @@ struct fpga_manager {
 	enum fpga_mgr_states state;
 	const struct fpga_manager_ops *mops;
 	void *priv;
+	fpga_mgr_cap_mask_t caps;
 };
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
@@ -120,7 +162,9 @@ struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 void fpga_mgr_put(struct fpga_manager *mgr);
 
 int fpga_mgr_register(struct device *dev, const char *name,
-		      const struct fpga_manager_ops *mops, void *priv);
+		      const struct fpga_manager_ops *mops,
+		      fpga_mgr_cap_mask_t caps,
+		      void *priv);
 
 void fpga_mgr_unregister(struct device *dev);
 
-- 
2.10.0

[PATCH 2/4] fpga mgr: Expose FPGA capabilities to userland via sysfs

[Moritz Fischer]

Expose FPGA capabilities to userland via sysfs.

Add Documentation for currently supported capabilities
that get exported via sysfs.

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
---
 Documentation/ABI/testing/sysfs-class-fpga-manager | 16 ++++++++++++++++
 drivers/fpga/fpga-mgr.c                            | 20 ++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h                      |  2 ++
 3 files changed, 38 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
index 23056c5..d9aee21 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-manager
+++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
@@ -35,3 +35,19 @@ Description:	Read fpga manager state as a string.
 		* write complete	= Doing post programming steps
 		* write complete error	= Error while doing post programming
 		* operating		= FPGA is programmed and operating
+
+What: 		/sys/class/fpga_manager/fpga/capabilities
+Date:		November 2016
+KernelVersion:	4.9
+Contact:	Moritz Fischer <moritz.fischer@ettus.com>
+Description:	Read fpga manager capabilities as a string.
+		The intent is to provide userspace with information on what
+		operations the particular instance can execute.
+
+		Each line expresses a capability that is available on the
+		particular instance of an fpga manager.
+		Supported so far:
+
+		* Full reconfiguration
+		* Partial reconfiguration
+		* Decrypt bitstream on the fly
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index ed57c17..98230b7 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -167,6 +167,11 @@ static const char * const state_str[] = {
 	[FPGA_MGR_STATE_OPERATING] =		"operating",
 };
 
+static const char * const cap_str[] = {
+	[FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
+	[FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
+};
+
 static ssize_t name_show(struct device *dev,
 			 struct device_attribute *attr, char *buf)
 {
@@ -183,10 +188,25 @@ static ssize_t state_show(struct device *dev,
 	return sprintf(buf, "%s\n", state_str[mgr->state]);
 }
 
+static ssize_t capabilities_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+	char *start = buf;
+	enum fpga_mgr_capability cap;
+
+	for_each_fpga_mgr_cap_mask(cap, mgr->caps)
+		buf += sprintf(buf, "%s\n", cap_str[cap]);
+
+	return buf - start;
+}
+
+static DEVICE_ATTR_RO(capabilities);
 static DEVICE_ATTR_RO(name);
 static DEVICE_ATTR_RO(state);
 
 static struct attribute *fpga_mgr_attrs[] = {
+	&dev_attr_capabilities.attr,
 	&dev_attr_name.attr,
 	&dev_attr_state.attr,
 	NULL,
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index e73429c..9bb96a5 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -108,6 +108,8 @@ static inline void __fpga_mgr_cap_set(enum fpga_mgr_capability cap,
 	set_bit(cap, mask->bits);
 }
 
+#define for_each_fpga_mgr_cap_mask(cap, mask) \
+	for_each_set_bit(cap, mask.bits, FPGA_MGR_CAP_END)
 
 /**
  * struct fpga_manager_ops - ops for low level fpga manager drivers
-- 
2.10.0

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

[Moritz Fischer]

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));
+	}
 
 	/* check that we have room in the command queue */
 	status = zynq_fpga_read(priv, STATUS_OFFSET);
@@ -412,6 +424,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	struct resource *res;
 	fpga_mgr_cap_mask_t caps;
 	int err;
+	u32 tmp;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -466,6 +479,10 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
 	fpga_mgr_cap_set(FPGA_MGR_CAP_PARTIAL_RECONF, caps);
 
+	/* only works if we booted in secure mode */
+	tmp = zynq_fpga_read(priv, CTRL_OFFSET);
+	if (tmp & CTRL_SEC_EN_MASK)
+		fpga_mgr_cap_set(FPGA_MGR_CAP_DECRYPT, caps);
 
 	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
 				&zynq_fpga_ops, caps, priv);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 9bb96a5..aabe258 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -68,10 +68,12 @@ enum fpga_mgr_states {
  */
 #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
 #define FPGA_MGR_FULL_RECONFIG		BIT(1)
+#define FPGA_MGR_DECRYPT_BITSTREAM	BIT(2)
 
 enum fpga_mgr_capability {
 	FPGA_MGR_CAP_PARTIAL_RECONF,
 	FPGA_MGR_CAP_FULL_RECONF,
+	FPGA_MGR_CAP_DECRYPT,
 
 /* last capability type for creation of the capabilities mask */
 	FPGA_MGR_CAP_END,
-- 
2.10.0

[PATCH 4/4] fpga mgr: socfpga: Expose support for encrypted bitstreams

[Moritz Fischer]

Expose support for on the fly decryption of bitstreams.
This needs no additional work or configuration,
so just expose the new capability.

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
---

Alan,

can you please let me know if that works this way, or where to find
information on encrypted bitstreams? I have a CycloneV SoCFPGA to test
on ...

Cheers,

Moritz
---
 drivers/fpga/socfpga.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index fd9760c..ab57ec0c 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -579,6 +579,7 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 
 	fpga_mgr_cap_zero(&caps);
 	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
+	fpga_mgr_cap_set(FPGA_MGR_CAP_DECRYPT, caps);
 
 	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
 				 &socfpga_fpga_ops, caps, priv);
-- 
2.10.0

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

[Sören Brinkmann]

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

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@xilinx.com> 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@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.

Yeah, will do. Definitely better that way.

Thanks for the review,

Moritz

Re: [PATCH 4/4] fpga mgr: socfpga: Expose support for encrypted bitstreams

[atull]

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Expose support for on the fly decryption of bitstreams.
> This needs no additional work or configuration,
> so just expose the new capability.

Hi Moritz,

When we talked about this, I was thinking about the arria10
support which I'd done more recently.  c5 and a10 are
quite different here.

The c5 datasheet:
  https://www.altera.com/literature/hb/cyclone-v/cv_5v4.pdf

Look for the 'stat' register on page 4-12 onwards.  This
register exposes the setting of the msel pins (are a dipswitch
on some boards).  The msel pins determine the programming
mode and whether it is expecting an encrypted and/or
compressed bitstream.  So you could read this reg and
set the capabilities accordingly.

For arria10, encryption is enabled and if the bitstream
says it's encrypted, the driver handles it.

Alan

> 
> 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
> ---
> 
> Alan,
> 
> can you please let me know if that works this way, or where to find
> information on encrypted bitstreams? I have a CycloneV SoCFPGA to test
> on ...
> 
> Cheers,
> 
> Moritz
> ---
>  drivers/fpga/socfpga.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index fd9760c..ab57ec0c 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -579,6 +579,7 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
>  
>  	fpga_mgr_cap_zero(&caps);
>  	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
> +	fpga_mgr_cap_set(FPGA_MGR_CAP_DECRYPT, caps);
>  
>  	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
>  				 &socfpga_fpga_ops, caps, priv);
> -- 
> 2.10.0
> 
> 

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

[-- Attachment #1: Type: text/plain, Size: 7861 bytes --]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Add FPGA capabilities as a way to express the capabilities
> of a given FPGA manager.
> 
> Removes code duplication by comparing the low-level driver's
> capabilities at the framework level rather than having each driver
> check for supported operations in the write_init() callback.
> 
> This allows for extending with additional capabilities, similar
> to the the dmaengine framework's implementation.
> 
> 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
> ---
> 
> Changes from RFC:
> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> 
> Note:
> 
> If people disagree on the typedef being a 'false positive' I can fix
> that in a future rev of the patchset.
> 
> Thanks,
> 
>     Moritz

Hi Moritz,

As I said at the Plumbers, I wasn't so sure about replacing
7 lines of code with 70 to reduce code duplication.  But it
looks useful to me and I guess I'm ok with it.  This will need
to be rebased onto the current linux-next master since my
device tree overlays stuff went in last week.

Alan

> 
> ---
>  drivers/fpga/fpga-mgr.c       | 15 ++++++++++++++
>  drivers/fpga/socfpga.c        | 10 +++++-----
>  drivers/fpga/zynq-fpga.c      |  7 ++++++-
>  include/linux/fpga/fpga-mgr.h | 46 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 953dc91..ed57c17 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
>  	struct device *dev = &mgr->dev;
>  	int ret;
>  
> +	if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> +	    !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> +		dev_err(dev, "Partial reconfiguration not supported\n");
> +		return -ENOTSUPP;
> +	}
> +
> +	if (flags & FPGA_MGR_FULL_RECONFIG &&
> +	    !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> +		dev_err(dev, "Full reconfiguration 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
> @@ -245,12 +257,14 @@ EXPORT_SYMBOL_GPL(fpga_mgr_put);
>   * @dev:	fpga manager device from pdev
>   * @name:	fpga manager name
>   * @mops:	pointer to structure of fpga manager ops
> + * @caps:	fpga manager capabilities
>   * @priv:	fpga manager private data
>   *
>   * Return: 0 on success, negative error code otherwise.
>   */
>  int fpga_mgr_register(struct device *dev, const char *name,
>  		      const struct fpga_manager_ops *mops,
> +		      fpga_mgr_cap_mask_t caps,
>  		      void *priv)
>  {
>  	struct fpga_manager *mgr;
> @@ -282,6 +296,7 @@ int fpga_mgr_register(struct device *dev, const char *name,
>  	mgr->name = name;
>  	mgr->mops = mops;
>  	mgr->priv = priv;
> +	mgr->caps = caps;
>  
>  	/*
>  	 * Initialize framework state by requesting low level driver read state
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index 27d2ff2..fd9760c 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -413,10 +413,6 @@ static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
>  	struct socfpga_fpga_priv *priv = mgr->priv;
>  	int ret;
>  
> -	if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
> -		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> -		return -EINVAL;
> -	}
>  	/* Steps 1 - 5: Reset the FPGA */
>  	ret = socfpga_fpga_reset(mgr);
>  	if (ret)
> @@ -555,6 +551,7 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct socfpga_fpga_priv *priv;
>  	struct resource *res;
> +	fpga_mgr_cap_mask_t caps;
>  	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -580,8 +577,11 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	fpga_mgr_cap_zero(&caps);
> +	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
> +
>  	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> -				 &socfpga_fpga_ops, priv);
> +				 &socfpga_fpga_ops, caps, priv);
>  }
>  
>  static int socfpga_fpga_remove(struct platform_device *pdev)
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index c2fb412..1d37ff0 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -410,6 +410,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct zynq_fpga_priv *priv;
>  	struct resource *res;
> +	fpga_mgr_cap_mask_t caps;
>  	int err;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -461,9 +462,13 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>  	zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
>  
>  	clk_disable(priv->clk);
> +	fpga_mgr_cap_zero(&caps);
> +	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
> +	fpga_mgr_cap_set(FPGA_MGR_CAP_PARTIAL_RECONF, caps);
> +
>  
>  	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
> -				&zynq_fpga_ops, priv);
> +				&zynq_fpga_ops, caps, priv);
>  	if (err) {
>  		dev_err(dev, "unable to register FPGA manager");
>  		clk_unprepare(priv->clk);
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 0940bf4..e73429c 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -67,6 +67,47 @@ enum fpga_mgr_states {
>   * FPGA_MGR_PARTIAL_RECONFIG: do partial reconfiguration if supported
>   */
>  #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
> +#define FPGA_MGR_FULL_RECONFIG		BIT(1)
> +
> +enum fpga_mgr_capability {
> +	FPGA_MGR_CAP_PARTIAL_RECONF,
> +	FPGA_MGR_CAP_FULL_RECONF,
> +
> +/* last capability type for creation of the capabilities mask */
> +	FPGA_MGR_CAP_END,
> +};
> +
> +typedef struct { DECLARE_BITMAP(bits, FPGA_MGR_CAP_END); } fpga_mgr_cap_mask_t;
> +
> +#define fpga_mgr_has_cap(cap, mask) __fpga_mgr_has_cap((cap), &(mask))
> +static inline int __fpga_mgr_has_cap(enum fpga_mgr_capability cap,
> +				     fpga_mgr_cap_mask_t *mask)
> +{
> +	return test_bit(cap, mask->bits);
> +}
> +
> +#define fpga_mgr_cap_zero(mask) __fpga_mgr_cap_zero(mask)
> +static inline void __fpga_mgr_cap_zero(fpga_mgr_cap_mask_t *mask)
> +{
> +	bitmap_zero(mask->bits, FPGA_MGR_CAP_END);
> +}
> +
> +#define fpga_mgr_cap_clear(cap, mask) __fpga_mgr_cap_clear((cap), &(mask))
> +static inline void __fpga_mgr_cap_clear(enum fpga_mgr_capability cap,
> +				       fpga_mgr_cap_mask_t *mask)
> +
> +{
> +	clear_bit(cap, mask->bits);
> +}
> +
> +#define fpga_mgr_cap_set(cap, mask) __fpga_mgr_cap_set((cap), &(mask))
> +static inline void __fpga_mgr_cap_set(enum fpga_mgr_capability cap,
> +				      fpga_mgr_cap_mask_t *mask)
> +
> +{
> +	set_bit(cap, mask->bits);
> +}
> +
>  
>  /**
>   * struct fpga_manager_ops - ops for low level fpga manager drivers
> @@ -105,6 +146,7 @@ struct fpga_manager {
>  	enum fpga_mgr_states state;
>  	const struct fpga_manager_ops *mops;
>  	void *priv;
> +	fpga_mgr_cap_mask_t caps;
>  };
>  
>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> @@ -120,7 +162,9 @@ struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>  void fpga_mgr_put(struct fpga_manager *mgr);
>  
>  int fpga_mgr_register(struct device *dev, const char *name,
> -		      const struct fpga_manager_ops *mops, void *priv);
> +		      const struct fpga_manager_ops *mops,
> +		      fpga_mgr_cap_mask_t caps,
> +		      void *priv);
>  
>  void fpga_mgr_unregister(struct device *dev);
>  
> -- 
> 2.10.0
> 
> 

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

> Add FPGA capabilities as a way to express the capabilities
> of a given FPGA manager.
> 
> Removes code duplication by comparing the low-level driver's
> capabilities at the framework level rather than having each driver
> check for supported operations in the write_init() callback.
> 
> This allows for extending with additional capabilities, similar
> to the the dmaengine framework's implementation.
> 
> 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
> ---
> 
> Changes from RFC:
> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> 
> Note:
> 
> If people disagree on the typedef being a 'false positive' I can fix
> that in a future rev of the patchset.
> 
> Thanks,
> 
>     Moritz
> 
> ---
>  drivers/fpga/fpga-mgr.c       | 15 ++++++++++++++
>  drivers/fpga/socfpga.c        | 10 +++++-----
>  drivers/fpga/zynq-fpga.c      |  7 ++++++-
>  include/linux/fpga/fpga-mgr.h | 46 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 71 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 953dc91..ed57c17 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
>  	struct device *dev = &mgr->dev;
>  	int ret;
>  
> +	if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> +	    !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> +		dev_err(dev, "Partial reconfiguration not supported\n");
> +		return -ENOTSUPP;
> +	}
> +
> +	if (flags & FPGA_MGR_FULL_RECONFIG &&
> +	    !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> +		dev_err(dev, "Full reconfiguration not supported\n");
> +		return -ENOTSUPP;
> +	}
> +

Could you move the checks to their own function like
'fpga_mgr_check_caps()' or something?  I really like it if we can keep
the functions short, like a screen or so where it's practicle to do
so and I could see the number of caps growing here.  The only
counter argument I could think of is if a cap affects the sequence
in this function.  Hmmm...

Alan

Re: [PATCH 2/4] fpga mgr: Expose FPGA capabilities to userland via sysfs

[atull]

[-- Attachment #1: Type: text/plain, Size: 3776 bytes --]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

One nit below.  Otherwise,

    Acked-by: Alan Tull <atull@opensource.altera.com>

Alan


> Expose FPGA capabilities to userland via sysfs.
> 
> Add Documentation for currently supported capabilities
> that get exported via sysfs.
> 
> 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
> ---
>  Documentation/ABI/testing/sysfs-class-fpga-manager | 16 ++++++++++++++++
>  drivers/fpga/fpga-mgr.c                            | 20 ++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h                      |  2 ++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
> index 23056c5..d9aee21 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
> @@ -35,3 +35,19 @@ Description:	Read fpga manager state as a string.
>  		* write complete	= Doing post programming steps
>  		* write complete error	= Error while doing post programming
>  		* operating		= FPGA is programmed and operating
> +
> +What: 		/sys/class/fpga_manager/fpga/capabilities
> +Date:		November 2016
> +KernelVersion:	4.9
> +Contact:	Moritz Fischer <moritz.fischer@ettus.com>
> +Description:	Read fpga manager capabilities as a string.
> +		The intent is to provide userspace with information on what
> +		operations the particular instance can execute.
> +
> +		Each line expresses a capability that is available on the
> +		particular instance of an fpga manager.
> +		Supported so far:
> +
> +		* Full reconfiguration
> +		* Partial reconfiguration
> +		* Decrypt bitstream on the fly

Decrypt gets added in a later patch.

> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index ed57c17..98230b7 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -167,6 +167,11 @@ static const char * const state_str[] = {
>  	[FPGA_MGR_STATE_OPERATING] =		"operating",
>  };
>  
> +static const char * const cap_str[] = {
> +	[FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
> +	[FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
> +};
> +
>  static ssize_t name_show(struct device *dev,
>  			 struct device_attribute *attr, char *buf)
>  {
> @@ -183,10 +188,25 @@ static ssize_t state_show(struct device *dev,
>  	return sprintf(buf, "%s\n", state_str[mgr->state]);
>  }
>  
> +static ssize_t capabilities_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_manager *mgr = to_fpga_manager(dev);
> +	char *start = buf;
> +	enum fpga_mgr_capability cap;
> +
> +	for_each_fpga_mgr_cap_mask(cap, mgr->caps)
> +		buf += sprintf(buf, "%s\n", cap_str[cap]);
> +
> +	return buf - start;
> +}
> +
> +static DEVICE_ATTR_RO(capabilities);
>  static DEVICE_ATTR_RO(name);
>  static DEVICE_ATTR_RO(state);
>  
>  static struct attribute *fpga_mgr_attrs[] = {
> +	&dev_attr_capabilities.attr,
>  	&dev_attr_name.attr,
>  	&dev_attr_state.attr,
>  	NULL,
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index e73429c..9bb96a5 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -108,6 +108,8 @@ static inline void __fpga_mgr_cap_set(enum fpga_mgr_capability cap,
>  	set_bit(cap, mask->bits);
>  }
>  
> +#define for_each_fpga_mgr_cap_mask(cap, mask) \
> +	for_each_set_bit(cap, mask.bits, FPGA_MGR_CAP_END)
>  
>  /**
>   * struct fpga_manager_ops - ops for low level fpga manager drivers
> -- 
> 2.10.0
> 
> 

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[Moritz Fischer]

Hi Alan,

On Mon, Nov 14, 2016 at 6:06 AM, atull <atull@opensource.altera.com> wrote:
> On Mon, 7 Nov 2016, Moritz Fischer wrote:
>
>> Add FPGA capabilities as a way to express the capabilities
>> of a given FPGA manager.
>>
>> Removes code duplication by comparing the low-level driver's
>> capabilities at the framework level rather than having each driver
>> check for supported operations in the write_init() callback.
>>
>> This allows for extending with additional capabilities, similar
>> to the the dmaengine framework's implementation.
>>
>> 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
>> ---
>>
>> Changes from RFC:
>> * in the RFC the caps weren't actually stored into the struct fpga_mgr
>>
>> Note:
>>
>> If people disagree on the typedef being a 'false positive' I can fix
>> that in a future rev of the patchset.
>>
>> Thanks,
>>
>>     Moritz
>>
>> ---
>>  drivers/fpga/fpga-mgr.c       | 15 ++++++++++++++
>>  drivers/fpga/socfpga.c        | 10 +++++-----
>>  drivers/fpga/zynq-fpga.c      |  7 ++++++-
>>  include/linux/fpga/fpga-mgr.h | 46 ++++++++++++++++++++++++++++++++++++++++++-
>>  4 files changed, 71 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index 953dc91..ed57c17 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
>>       struct device *dev = &mgr->dev;
>>       int ret;
>>
>> +     if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
>> +         !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
>> +             dev_err(dev, "Partial reconfiguration not supported\n");
>> +             return -ENOTSUPP;
>> +     }
>> +
>> +     if (flags & FPGA_MGR_FULL_RECONFIG &&
>> +         !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
>> +             dev_err(dev, "Full reconfiguration not supported\n");
>> +             return -ENOTSUPP;
>> +     }
>> +
>
> Could you move the checks to their own function like
> 'fpga_mgr_check_caps()' or something?  I really like it if we can keep
> the functions short, like a screen or so where it's practicle to do
> so and I could see the number of caps growing here.

Absolutely. Great suggestion.

> The only counter argument I could think of is if a cap affects the sequence
> in this function.  Hmmm...

Oh you mean the cap being there affecting the sequence in *this* function?
I'd suggest we address that when we run into a cap that requires this.

Cheers,

Moritz

Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities

[atull]

[-- Attachment #1: Type: text/plain, Size: 3009 bytes --]

On Mon, 14 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

> Hi Alan,
> 
> On Mon, Nov 14, 2016 at 6:06 AM, atull <atull@opensource.altera.com> wrote:
> > On Mon, 7 Nov 2016, Moritz Fischer wrote:
> >
> >> Add FPGA capabilities as a way to express the capabilities
> >> of a given FPGA manager.
> >>
> >> Removes code duplication by comparing the low-level driver's
> >> capabilities at the framework level rather than having each driver
> >> check for supported operations in the write_init() callback.
> >>
> >> This allows for extending with additional capabilities, similar
> >> to the the dmaengine framework's implementation.
> >>
> >> 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
> >> ---
> >>
> >> Changes from RFC:
> >> * in the RFC the caps weren't actually stored into the struct fpga_mgr
> >>
> >> Note:
> >>
> >> If people disagree on the typedef being a 'false positive' I can fix
> >> that in a future rev of the patchset.
> >>
> >> Thanks,
> >>
> >>     Moritz
> >>
> >> ---
> >>  drivers/fpga/fpga-mgr.c       | 15 ++++++++++++++
> >>  drivers/fpga/socfpga.c        | 10 +++++-----
> >>  drivers/fpga/zynq-fpga.c      |  7 ++++++-
> >>  include/linux/fpga/fpga-mgr.h | 46 ++++++++++++++++++++++++++++++++++++++++++-
> >>  4 files changed, 71 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> >> index 953dc91..ed57c17 100644
> >> --- a/drivers/fpga/fpga-mgr.c
> >> +++ b/drivers/fpga/fpga-mgr.c
> >> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
> >>       struct device *dev = &mgr->dev;
> >>       int ret;
> >>
> >> +     if (flags & FPGA_MGR_PARTIAL_RECONFIG &&
> >> +         !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) {
> >> +             dev_err(dev, "Partial reconfiguration not supported\n");
> >> +             return -ENOTSUPP;
> >> +     }
> >> +
> >> +     if (flags & FPGA_MGR_FULL_RECONFIG &&
> >> +         !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) {
> >> +             dev_err(dev, "Full reconfiguration not supported\n");
> >> +             return -ENOTSUPP;
> >> +     }
> >> +
> >
> > Could you move the checks to their own function like
> > 'fpga_mgr_check_caps()' or something?  I really like it if we can keep
> > the functions short, like a screen or so where it's practicle to do
> > so and I could see the number of caps growing here.
> 
> Absolutely. Great suggestion.
> 
> > The only counter argument I could think of is if a cap affects the sequence
> > in this function.  Hmmm...
> 
> Oh you mean the cap being there affecting the sequence in *this* function?
> I'd suggest we address that when we run into a cap that requires this.

Yes, I agree.

Alan

> 
> Cheers,
> 
> Moritz
> 

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

[atull]

[-- Attachment #1: Type: text/plain, Size: 5531 bytes --]

On Mon, 7 Nov 2016, Moritz Fischer wrote:

Hi Moritz,

This looks good.  Probably the socfpga changes could get
folded into this patch (was patch 4/4) unless you thought of
a reason not to (after that patch is changed to see if the
MSEL bits are set to enable decrypt).

There also could be a uncompress cap as well since cyclone 5
supports both compressed and encrypted images and has bits
in the MSEL for them (I mentioned separately in my comments
about patch 4/4).

For the Zynq, does the encrypt bit denote a requirement that
the part will only take an encrypted image or is it an
option that it supports?  IIRC (and my brain is currently
pretty tired), if the MSEL for Cyclone5 is set for
encryption, the bitsream must be encrypted (same for
compress).  That might change the meaning of this stuff a
bit but probably doesn't necessitate a change in the
implementation.  It also makes the sysfs that much more
useful as it allows the users to know what type of image
they are required to provide.

Thanks,
Alan

> 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));
> +	}
>  
>  	/* check that we have room in the command queue */
>  	status = zynq_fpga_read(priv, STATUS_OFFSET);
> @@ -412,6 +424,7 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	fpga_mgr_cap_mask_t caps;
>  	int err;
> +	u32 tmp;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -466,6 +479,10 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>  	fpga_mgr_cap_set(FPGA_MGR_CAP_FULL_RECONF, caps);
>  	fpga_mgr_cap_set(FPGA_MGR_CAP_PARTIAL_RECONF, caps);
>  
> +	/* only works if we booted in secure mode */
> +	tmp = zynq_fpga_read(priv, CTRL_OFFSET);
> +	if (tmp & CTRL_SEC_EN_MASK)
> +		fpga_mgr_cap_set(FPGA_MGR_CAP_DECRYPT, caps);
>  
>  	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
>  				&zynq_fpga_ops, caps, priv);
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 9bb96a5..aabe258 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -68,10 +68,12 @@ enum fpga_mgr_states {
>   */
>  #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
>  #define FPGA_MGR_FULL_RECONFIG		BIT(1)
> +#define FPGA_MGR_DECRYPT_BITSTREAM	BIT(2)
>  
>  enum fpga_mgr_capability {
>  	FPGA_MGR_CAP_PARTIAL_RECONF,
>  	FPGA_MGR_CAP_FULL_RECONF,
> +	FPGA_MGR_CAP_DECRYPT,
>  
>  /* last capability type for creation of the capabilities mask */
>  	FPGA_MGR_CAP_END,
> -- 
> 2.10.0
> 
> 

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

[Moritz Fischer]

Hi Alan,

On Mon, Nov 14, 2016 at 7:42 PM, atull <atull@opensource.altera.com> wrote:
> On Mon, 7 Nov 2016, Moritz Fischer wrote:
>
> Hi Moritz,
>
> This looks good.  Probably the socfpga changes could get
> folded into this patch (was patch 4/4) unless you thought of
> a reason not to (after that patch is changed to see if the
> MSEL bits are set to enable decrypt).

Yeah. Agreed. I had kept that one separate because i was less
sure on how the socfpga stuff works. Will merge it together for
next revision.

>
> There also could be a uncompress cap as well since cyclone 5
> supports both compressed and encrypted images and has bits
> in the MSEL for them (I mentioned separately in my comments
> about patch 4/4).

Ok.
>
> For the Zynq, does the encrypt bit denote a requirement that
> the part will only take an encrypted image or is it an
> option that it supports?  IIRC (and my brain is currently
> pretty tired), if the MSEL for Cyclone5 is set for
> encryption, the bitsream must be encrypted (same for
> compress).  That might change the meaning of this stuff a
> bit but probably doesn't necessitate a change in the
> implementation.  It also makes the sysfs that much more
> useful as it allows the users to know what type of image
> they are required to provide.

I don't think that's the case for Zynq. I think the actual reason for
different behavior is the fact that the AES unit now takes the
bitstream bytewise vs 4 bytes at a time, which is why the clock
needs to be divided by four. The TRM isn't overly specific on that.
I Will take another look in the Zynq 7000 TRM.

I do agree that the sysfs interface becomes more useful if
having an encrypted bitstream or compressed bitstream is now
mandatory. I'll need to think about this some more. Maybe to
make this useful there needs to be a distinction between
mandatory and optional capabilities. One could model it by
adding a PLAIN vs CRYPTED capability ... mhhh

Thanks for the review,

Moritz