[PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

This is 5th submission of Intel service layer and FPGA patches, which
includes the missing standalone patch in the 4th submission.

This submission includes additional changes for Intel service layer driver
to get the firmware version running at FPGA SoC device. Then FPGA manager
driver, one of Intel service layer driver's client, can decide whether to
handle the newly added bitstream authentication function based on the
retrieved firmware version. So that we can maintain FPGA manager driver
the back compatible.

Bitstream authentication makes sure a signed bitstream has valid
signatures.

The customer sends the bitstream via FPGA framework and overlay, the
firmware will authenticate the bitstream but not program the bitstream to
device. If the authentication passes, the bitstream will be programmed into
QSPI flash and will be expected to boot without issues.

Extend Intel service layer, FPGA manager and region drivers to support the
bitstream authentication feature. 

Richard Gong (7):
  firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
  firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
  firmware: stratix10-svc: extend SVC driver to get the firmware version
  fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
  fpga: of-fpga-region: add authenticate-fpga-config property
  dt-bindings: fpga: add authenticate-fpga-config property
  fpga: stratix10-soc: extend driver for bitstream authentication

 .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
 drivers/firmware/stratix10-svc.c                   | 12 ++++-
 drivers/fpga/of-fpga-region.c                      | 24 ++++++---
 drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
 include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
 .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
 include/linux/fpga/fpga-mgr.h                      |  3 ++
 7 files changed, 125 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCHv5 1/7] firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
aligns with the firmware settings.

Fixes: 36847f9e3e56 ("firmware: stratix10-svc: correct reconfig flag and timeout values")
Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v5: new add, add the missing standalone patch
---
 include/linux/firmware/intel/stratix10-svc-client.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index a93d859..f843c6a 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -56,7 +56,7 @@
  * COMMAND_RECONFIG_FLAG_PARTIAL:
  * Set to FPGA configuration type (full or partial).
  */
-#define COMMAND_RECONFIG_FLAG_PARTIAL	1
+#define COMMAND_RECONFIG_FLAG_PARTIAL	0
 
 /**
  * Timeout settings for service clients:
-- 
2.7.4

[PATCHv5 2/7] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Add COMMAND_AUTHENTICATE_BITSTREAM command flag for new added bitstream
authentication feature. Authenticating a bitstream is to make sure a signed
bitstream has the valid signatures.

Except for the actual configuration of the device, the bitstream
authentication works the same way as FPGA configuration does. If the
authentication passes, the signed bitstream will be programmed into QSPI
flash memory and will be expected to boot without issues.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v5: no change
v4: remove change at COMMAND_RECONFIG_FLAG_PARTIAL flag & add that to a
    separate commit 27ad5309c247b6bde8a098e17e9bd9b1576b7f71.
v3: no change
v2: new added
---
 include/linux/firmware/intel/stratix10-svc-client.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index f843c6a..fa9581d 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -55,8 +55,13 @@
  *
  * COMMAND_RECONFIG_FLAG_PARTIAL:
  * Set to FPGA configuration type (full or partial).
+ *
+ * COMMAND_AUTHENTICATE_BITSTREAM:
+ * Set for bitstream authentication, which makes sure a signed bitstream
+ * has valid signatures before committing it to device.
  */
 #define COMMAND_RECONFIG_FLAG_PARTIAL	0
+#define COMMAND_AUTHENTICATE_BITSTREAM	1
 
 /**
  * Timeout settings for service clients:
-- 
2.7.4

[PATCHv5 3/7] firmware: stratix10-svc: extend SVC driver to get the firmware version

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Extend Intel service layer driver to get the firmware version running at
FPGA device. Therefore FPGA manager driver, one of Intel service layer
driver's client, can decide whether to handle the newly added bitstream
authentication function based on the retrieved firmware version.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v5: no change
v4: no change
v3: new added, changes for getting firmware version
---
 drivers/firmware/stratix10-svc.c                    | 12 ++++++++++--
 include/linux/firmware/intel/stratix10-smc.h        | 21 +++++++++++++++++++--
 include/linux/firmware/intel/stratix10-svc-client.h |  4 ++++
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 3aa489d..1443bbd 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -306,6 +306,7 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
 		break;
 	case COMMAND_RSU_RETRY:
 	case COMMAND_RSU_MAX_RETRY:
+	case COMMAND_FIRMWARE_VERSION:
 		cb_data->status = BIT(SVC_STATUS_OK);
 		cb_data->kaddr1 = &res.a1;
 		break;
@@ -422,6 +423,11 @@ static int svc_normal_to_secure_thread(void *data)
 			a1 = 0;
 			a2 = 0;
 			break;
+		case COMMAND_FIRMWARE_VERSION:
+			a0 = INTEL_SIP_SMC_FIRMWARE_VERSION;
+			a1 = 0;
+			a2 = 0;
+			break;
 		default:
 			pr_warn("it shouldn't happen\n");
 			break;
@@ -487,11 +493,13 @@ static int svc_normal_to_secure_thread(void *data)
 
 			/*
 			 * be compatible with older version firmware which
-			 * doesn't support RSU notify or retry
+			 * doesn't support RSU notify, retry or bitstream
+			 * authentication.
 			 */
 			if ((pdata->command == COMMAND_RSU_RETRY) ||
 			    (pdata->command == COMMAND_RSU_MAX_RETRY) ||
-				(pdata->command == COMMAND_RSU_NOTIFY)) {
+			    (pdata->command == COMMAND_RSU_NOTIFY) ||
+			    (pdata->command == COMMAND_FIRMWARE_VERSION)) {
 				cbdata->status =
 					BIT(SVC_STATUS_NO_SUPPORT);
 				cbdata->kaddr1 = NULL;
diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/linux/firmware/intel/stratix10-smc.h
index c3e5ab0..505fcca 100644
--- a/include/linux/firmware/intel/stratix10-smc.h
+++ b/include/linux/firmware/intel/stratix10-smc.h
@@ -321,8 +321,6 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
 #define INTEL_SIP_SMC_ECC_DBE \
 	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_ECC_DBE)
 
-#endif
-
 /**
  * Request INTEL_SIP_SMC_RSU_NOTIFY
  *
@@ -404,3 +402,22 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
 #define INTEL_SIP_SMC_FUNCID_RSU_MAX_RETRY 18
 #define INTEL_SIP_SMC_RSU_MAX_RETRY \
 	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_RSU_MAX_RETRY)
+
+/**
+ * Request INTEL_SIP_SMC_FIRMWARE_VERSION
+ *
+ * Sync call used to query the version of running firmware
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_FIRMWARE_VERSION
+ * a1-a7 not used
+ *
+ * Return status:
+ * a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_STATUS_ERROR
+ * a1 running firmware version
+ */
+#define INTEL_SIP_SMC_FUNCID_FIRMWARE_VERSION 31
+#define INTEL_SIP_SMC_FIRMWARE_VERSION \
+	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FIRMWARE_VERSION)
+
+#endif
diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index fa9581d..193a2cf 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -109,6 +109,9 @@ struct stratix10_svc_chan;
  *
  * @COMMAND_RSU_DCMF_VERSION: query firmware for the DCMF version, return status
  * is SVC_STATUS_OK or SVC_STATUS_ERROR
+ *
+ * @COMMAND_FIRMWARE_VERSION: query running firmware version, return status
+ * is SVC_STATUS_OK or SVC_STATUS_ERROR
  */
 enum stratix10_svc_command_code {
 	COMMAND_NOOP = 0,
@@ -122,6 +125,7 @@ enum stratix10_svc_command_code {
 	COMMAND_RSU_RETRY,
 	COMMAND_RSU_MAX_RETRY,
 	COMMAND_RSU_DCMF_VERSION,
+	COMMAND_FIRMWARE_VERSION,
 };
 
 /**
-- 
2.7.4

[PATCHv5 4/7] fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Add FPGA_MGR_BITSTREAM_AUTHENTICATE flag for FPGA bitstream
authentication, which makes sure a signed bitstream has valid signatures.

Except for the actual configuration of the device, the authentication works
the same way as FPGA configuration does. If the authentication passes, the
bitstream will be programmed into QSPI flash and will be expected to boot
without issues.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v5: no change
v4: s/FPGA_MGR_BITSTREM_AUTHENTICATION/FPGA_MGR_BITSTREAM_AUTHENTICATE
v3: no change
v2: align all FPGA_MGR_* flags
    update the commit messages
---
 include/linux/fpga/fpga-mgr.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 2bc3030..a81b3a7 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -67,12 +67,15 @@ enum fpga_mgr_states {
  * %FPGA_MGR_BITSTREAM_LSB_FIRST: SPI bitstream bit order is LSB first
  *
  * %FPGA_MGR_COMPRESSED_BITSTREAM: FPGA bitstream is compressed
+ *
+ * %FPGA_MGR_BITSTREAM_AUTHENTICATE: do FPGA bitstream authentication only
  */
 #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
 #define FPGA_MGR_EXTERNAL_CONFIG	BIT(1)
 #define FPGA_MGR_ENCRYPTED_BITSTREAM	BIT(2)
 #define FPGA_MGR_BITSTREAM_LSB_FIRST	BIT(3)
 #define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(4)
+#define FPGA_MGR_BITSTREAM_AUTHENTICATE	BIT(5)
 
 /**
  * struct fpga_image_info - information specific to a FPGA image
-- 
2.7.4

[PATCHv5 5/7] fpga: of-fpga-region: add authenticate-fpga-config property

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Add authenticate-fpga-config property to support FPGA bitstream
authentication, which makes sure a signed bitstream has valid signatures.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v5: no change
v4: add additional checks to make sure *only* authenticate
v3: no change
v2: changed in alphabetical order
---
 drivers/fpga/of-fpga-region.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index e405309..5074479 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -218,15 +218,25 @@ static struct fpga_image_info *of_fpga_region_parse_ov(
 
 	info->overlay = overlay;
 
-	/* Read FPGA region properties from the overlay */
-	if (of_property_read_bool(overlay, "partial-fpga-config"))
-		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
+	/*
+	 * Read FPGA region properties from the overlay.
+	 *
+	 * First check the integrity of the bitstream. If the
+	 * authentication is passed, the user can perform other
+	 * operations.
+	 */
+	if (of_property_read_bool(overlay, "authenticate-fpga-config")) {
+		info->flags |= FPGA_MGR_BITSTREAM_AUTHENTICATE;
+	} else {
+		if (of_property_read_bool(overlay, "partial-fpga-config"))
+			info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
 
-	if (of_property_read_bool(overlay, "external-fpga-config"))
-		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
+		if (of_property_read_bool(overlay, "external-fpga-config"))
+			info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
 
-	if (of_property_read_bool(overlay, "encrypted-fpga-config"))
-		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
+		if (of_property_read_bool(overlay, "encrypted-fpga-config"))
+			info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
+	}
 
 	if (!of_property_read_string(overlay, "firmware-name",
 				     &firmware_name)) {
-- 
2.7.4

[PATCHv5 6/7] dt-bindings: fpga: add authenticate-fpga-config property

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Add authenticate-fpga-config property for FPGA bitstream authentication,
which makes sure a signed bitstream has valid signatures.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v5: rewrite the description to highlight two things with
    authenticate-fpga-config flag
v4: explain authenticate-fpga-config flag further
v3: no change
v2: put authenticate-fpga-config above partial-fpga-config
    update commit messages
---
 Documentation/devicetree/bindings/fpga/fpga-region.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
index e811cf8..dca0e37 100644
--- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
@@ -182,6 +182,16 @@ Optional properties:
 	This property is optional if the FPGA Manager handles the bridges.
         If the fpga-region is  the child of a fpga-bridge, the list should not
         contain the parent bridge.
+- authenticate-fpga-config : boolean, set if do bitstream authentication only.
+	If 'authenticate-fpga-config' is added then adding a new node or another
+	operation is not allowed.
+	Flag authenticate-fpga-config is used to check the integrity of the
+	bitstream.
+	Except for the actual configuration of the device, the authentication
+	works in the same way as FPGA configuration. If the authentication passes,
+	other operations such as full or partial reconfiguration can be performed.
+	When the bitstream into QSPI flash memory at device is programmed, it is
+	expected that there will be no issue when starting the device.
 - partial-fpga-config : boolean, set if partial reconfiguration is to be done,
 	otherwise full reconfiguration is done.
 - external-fpga-config : boolean, set if the FPGA has already been configured
-- 
2.7.4

[PATCHv5 7/7] fpga: stratix10-soc: extend driver for bitstream authentication

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Extend FPGA manager driver to support FPGA bitstream authentication on
Intel SocFPGA platforms.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v5: no change
v4: s/FPGA_MGR_BITSTREM_AUTHENTICATION/FPGA_MGR_BITSTREAM_AUTHENTICATE
v3: add handle to retriev the firmware version to keep driver
    back compatible
v2: use flag defined in stratix10-svc driver
---
 drivers/fpga/stratix10-soc.c | 62 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 6 deletions(-)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index 657a70c..9ab7afd 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -24,6 +24,10 @@
 #define S10_BUFFER_TIMEOUT (msecs_to_jiffies(SVC_RECONFIG_BUFFER_TIMEOUT_MS))
 #define S10_RECONFIG_TIMEOUT (msecs_to_jiffies(SVC_RECONFIG_REQUEST_TIMEOUT_MS))
 
+#define INVALID_FIRMWARE_VERSION	0xFFFF
+typedef void (*s10_callback)(struct stratix10_svc_client *client,
+			     struct stratix10_svc_cb_data *data);
+
 /*
  * struct s10_svc_buf
  * buf:  virtual address of buf provided by service layer
@@ -40,11 +44,13 @@ struct s10_priv {
 	struct completion status_return_completion;
 	struct s10_svc_buf svc_bufs[NUM_SVC_BUFS];
 	unsigned long status;
+	unsigned int fw_version;
 };
 
 static int s10_svc_send_msg(struct s10_priv *priv,
 			    enum stratix10_svc_command_code command,
-			    void *payload, u32 payload_length)
+			    void *payload, u32 payload_length,
+			    s10_callback callback)
 {
 	struct stratix10_svc_chan *chan = priv->chan;
 	struct device *dev = priv->client.dev;
@@ -57,6 +63,7 @@ static int s10_svc_send_msg(struct s10_priv *priv,
 	msg.command = command;
 	msg.payload = payload;
 	msg.payload_length = payload_length;
+	priv->client.receive_cb = callback;
 
 	ret = stratix10_svc_send(chan, &msg);
 	dev_dbg(dev, "stratix10_svc_send returned status %d\n", ret);
@@ -134,6 +141,29 @@ static void s10_unlock_bufs(struct s10_priv *priv, void *kaddr)
 }
 
 /*
+ * s10_fw_version_callback - callback for the version of running firmware
+ * @client: service layer client struct
+ * @data: message from service layer
+ */
+static void s10_fw_version_callback(struct stratix10_svc_client *client,
+				    struct stratix10_svc_cb_data *data)
+{
+	struct s10_priv *priv = client->priv;
+	unsigned int *version = (unsigned int *)data->kaddr1;
+
+	if (data->status == BIT(SVC_STATUS_OK))
+		priv->fw_version = *version;
+	else if (data->status == BIT(SVC_STATUS_NO_SUPPORT))
+		dev_warn(client->dev,
+			 "FW doesn't support bitstream authentication\n");
+	else
+		dev_err(client->dev, "Failed to get FW version %lu\n",
+			BIT(data->status));
+
+	complete(&priv->status_return_completion);
+}
+
+/*
  * s10_receive_callback - callback for service layer to use to provide client
  * (this driver) messages received through the mailbox.
  * client: service layer client struct
@@ -186,13 +216,22 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
 	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
 		dev_dbg(dev, "Requesting partial reconfiguration.\n");
 		ctype.flags |= BIT(COMMAND_RECONFIG_FLAG_PARTIAL);
+	} else if (info->flags & FPGA_MGR_BITSTREAM_AUTHENTICATE) {
+		if (priv->fw_version == INVALID_FIRMWARE_VERSION) {
+			dev_err(dev, "FW doesn't support\n");
+			return -EINVAL;
+		}
+
+		dev_dbg(dev, "Requesting bitstream authentication.\n");
+		ctype.flags |= BIT(COMMAND_AUTHENTICATE_BITSTREAM);
 	} else {
 		dev_dbg(dev, "Requesting full reconfiguration.\n");
 	}
 
 	reinit_completion(&priv->status_return_completion);
 	ret = s10_svc_send_msg(priv, COMMAND_RECONFIG,
-			       &ctype, sizeof(ctype));
+			       &ctype, sizeof(ctype),
+			       s10_receive_callback);
 	if (ret < 0)
 		goto init_done;
 
@@ -259,7 +298,7 @@ static int s10_send_buf(struct fpga_manager *mgr, const char *buf, size_t count)
 	svc_buf = priv->svc_bufs[i].buf;
 	memcpy(svc_buf, buf, xfer_sz);
 	ret = s10_svc_send_msg(priv, COMMAND_RECONFIG_DATA_SUBMIT,
-			       svc_buf, xfer_sz);
+			       svc_buf, xfer_sz, s10_receive_callback);
 	if (ret < 0) {
 		dev_err(dev,
 			"Error while sending data to service layer (%d)", ret);
@@ -303,7 +342,7 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
 
 			ret = s10_svc_send_msg(
 				priv, COMMAND_RECONFIG_DATA_CLAIM,
-				NULL, 0);
+				NULL, 0, s10_receive_callback);
 			if (ret < 0)
 				break;
 		}
@@ -357,7 +396,8 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
 	do {
 		reinit_completion(&priv->status_return_completion);
 
-		ret = s10_svc_send_msg(priv, COMMAND_RECONFIG_STATUS, NULL, 0);
+		ret = s10_svc_send_msg(priv, COMMAND_RECONFIG_STATUS,
+				       NULL, 0, s10_receive_callback);
 		if (ret < 0)
 			break;
 
@@ -411,8 +451,9 @@ static int s10_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->fw_version = INVALID_FIRMWARE_VERSION;
 	priv->client.dev = dev;
-	priv->client.receive_cb = s10_receive_callback;
+	priv->client.receive_cb = NULL;
 	priv->client.priv = priv;
 
 	priv->chan = stratix10_svc_request_channel_byname(&priv->client,
@@ -440,6 +481,15 @@ static int s10_probe(struct platform_device *pdev)
 		goto probe_err;
 	}
 
+	/* get the running firmware version */
+	ret = s10_svc_send_msg(priv, COMMAND_FIRMWARE_VERSION,
+			       NULL, 0, s10_fw_version_callback);
+	if (ret) {
+		dev_err(dev, "couldn't get firmware version\n");
+		fpga_mgr_free(mgr);
+		goto probe_err;
+	}
+
 	platform_set_drvdata(pdev, mgr);
 	return ret;
 
-- 
2.7.4

Re: [PATCHv5 1/7] firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0

[Tom Rix]

On 2/9/21 2:20 PM, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
>
> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
> aligns with the firmware settings.
>
> Fixes: 36847f9e3e56 ("firmware: stratix10-svc: correct reconfig flag and timeout values")
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
> v5: new add, add the missing standalone patch
> ---
>  include/linux/firmware/intel/stratix10-svc-client.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> index a93d859..f843c6a 100644
> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> @@ -56,7 +56,7 @@
>   * COMMAND_RECONFIG_FLAG_PARTIAL:
>   * Set to FPGA configuration type (full or partial).
>   */
> -#define COMMAND_RECONFIG_FLAG_PARTIAL	1
> +#define COMMAND_RECONFIG_FLAG_PARTIAL	0

Is this the stand alone fix split from v3's patch 1 ?

https://lore.kernel.org/linux-fpga/YBFW50LPP%2FyEbvEW@kroah.com/

Tom

>  
>  /**
>   * Timeout settings for service clients:

Re: [PATCHv5 1/7] firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0

[Tom Rix]

On 2/15/21 6:41 AM, Richard Gong wrote:
> Hi Tom,
>
> On 2/13/21 9:44 AM, Tom Rix wrote:
>>
>> On 2/9/21 2:20 PM, richard.gong@linux.intel.com wrote:
>>> From: Richard Gong <richard.gong@intel.com>
>>>
>>> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
>>> aligns with the firmware settings.
>>>
>>> Fixes: 36847f9e3e56 ("firmware: stratix10-svc: correct reconfig flag and timeout values")
>>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>>> ---
>>> v5: new add, add the missing standalone patch
>>> ---
>>>   include/linux/firmware/intel/stratix10-svc-client.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
>>> index a93d859..f843c6a 100644
>>> --- a/include/linux/firmware/intel/stratix10-svc-client.h
>>> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
>>> @@ -56,7 +56,7 @@
>>>    * COMMAND_RECONFIG_FLAG_PARTIAL:
>>>    * Set to FPGA configuration type (full or partial).
>>>    */
>>> -#define COMMAND_RECONFIG_FLAG_PARTIAL    1
>>> +#define COMMAND_RECONFIG_FLAG_PARTIAL    0
>>
>> Is this the stand alone fix split from v3's patch 1 ?
>>
>> https://lore.kernel.org/linux-fpga/YBFW50LPP%2FyEbvEW@kroah.com/
>> Yes, it is a stand-alone patch.

Thanks.

Reviewed-by: Tom Rix <trix@redhat.com>

>
>> Tom
>>
>>>     /**
>>>    * Timeout settings for service clients:
>>
> Regards,
> Richard
>

Re: [PATCHv5 1/7] firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0

[Richard Gong]

Hi Tom,

On 2/13/21 9:44 AM, Tom Rix wrote:
> 
> On 2/9/21 2:20 PM, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
>> aligns with the firmware settings.
>>
>> Fixes: 36847f9e3e56 ("firmware: stratix10-svc: correct reconfig flag and timeout values")
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>> v5: new add, add the missing standalone patch
>> ---
>>   include/linux/firmware/intel/stratix10-svc-client.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
>> index a93d859..f843c6a 100644
>> --- a/include/linux/firmware/intel/stratix10-svc-client.h
>> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
>> @@ -56,7 +56,7 @@
>>    * COMMAND_RECONFIG_FLAG_PARTIAL:
>>    * Set to FPGA configuration type (full or partial).
>>    */
>> -#define COMMAND_RECONFIG_FLAG_PARTIAL	1
>> +#define COMMAND_RECONFIG_FLAG_PARTIAL	0
> 
> Is this the stand alone fix split from v3's patch 1 ?
> 
> https://lore.kernel.org/linux-fpga/YBFW50LPP%2FyEbvEW@kroah.com/
>Yes, it is a stand-alone patch.

> Tom
> 
>>   
>>   /**
>>    * Timeout settings for service clients:
> 
Regards,
Richard

RE: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Gong, Richard]

Hi Moritz,

Sorry for asking.

When you have chance, can you help review the version 5 patchset submitted on 02/09/21?

Regards,
Richard

-----Original Message-----
From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
Sent: Tuesday, February 9, 2021 4:20 PM
To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: Gong, Richard <richard.gong@intel.com>
Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

From: Richard Gong <richard.gong@intel.com>

This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.

This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.

Bitstream authentication makes sure a signed bitstream has valid signatures.

The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.

Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 

Richard Gong (7):
  firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
  firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
  firmware: stratix10-svc: extend SVC driver to get the firmware version
  fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
  fpga: of-fpga-region: add authenticate-fpga-config property
  dt-bindings: fpga: add authenticate-fpga-config property
  fpga: stratix10-soc: extend driver for bitstream authentication

 .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
 drivers/firmware/stratix10-svc.c                   | 12 ++++-
 drivers/fpga/of-fpga-region.c                      | 24 ++++++---
 drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
 include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
 .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
 include/linux/fpga/fpga-mgr.h                      |  3 ++
 7 files changed, 125 insertions(+), 18 deletions(-)

--
2.7.4

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Tom Rix]

The first patch is a fix that is targeted for stable.

Tom

On 2/25/21 5:07 AM, Gong, Richard wrote:
> Hi Moritz,
>
> Sorry for asking.
>
> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>
> Regards,
> Richard
>
> -----Original Message-----
> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
> Sent: Tuesday, February 9, 2021 4:20 PM
> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Gong, Richard <richard.gong@intel.com>
> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>
> From: Richard Gong <richard.gong@intel.com>
>
> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>
> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>
> Bitstream authentication makes sure a signed bitstream has valid signatures.
>
> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>
> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
>
> Richard Gong (7):
>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>   firmware: stratix10-svc: extend SVC driver to get the firmware version
>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>   fpga: of-fpga-region: add authenticate-fpga-config property
>   dt-bindings: fpga: add authenticate-fpga-config property
>   fpga: stratix10-soc: extend driver for bitstream authentication
>
>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>  include/linux/fpga/fpga-mgr.h                      |  3 ++
>  7 files changed, 125 insertions(+), 18 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Richard Gong]

The first patch of the version 5 patch set is a fix for the mainline, I 
submitted a separate patch for a fix at the stable.

Regards,
Richard


On 2/25/21 7:28 AM, Tom Rix wrote:
> The first patch is a fix that is targeted for stable.
> 
> Tom
> 
> On 2/25/21 5:07 AM, Gong, Richard wrote:
>> Hi Moritz,
>>
>> Sorry for asking.
>>
>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>
>> Regards,
>> Richard
>>
>> -----Original Message-----
>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>> Sent: Tuesday, February 9, 2021 4:20 PM
>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: Gong, Richard <richard.gong@intel.com>
>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>
>> From: Richard Gong <richard.gong@intel.com>
>>
>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>
>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>
>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>
>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>
>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature.
>>
>> Richard Gong (7):
>>    firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>    firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>    firmware: stratix10-svc: extend SVC driver to get the firmware version
>>    fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>    fpga: of-fpga-region: add authenticate-fpga-config property
>>    dt-bindings: fpga: add authenticate-fpga-config property
>>    fpga: stratix10-soc: extend driver for bitstream authentication
>>
>>   .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>   drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>   drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>   drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>   include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>   .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>   include/linux/fpga/fpga-mgr.h                      |  3 ++
>>   7 files changed, 125 insertions(+), 18 deletions(-)
>>
>> --
>> 2.7.4
>>
> 

Re: [PATCHv5 1/7] firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0

[Moritz Fischer]

On Mon, Feb 15, 2021 at 06:32:16AM -0800, Tom Rix wrote:
> 
> On 2/15/21 6:41 AM, Richard Gong wrote:
> > Hi Tom,
> >
> > On 2/13/21 9:44 AM, Tom Rix wrote:
> >>
> >> On 2/9/21 2:20 PM, richard.gong@linux.intel.com wrote:
> >>> From: Richard Gong <richard.gong@intel.com>
> >>>
> >>> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
> >>> aligns with the firmware settings.
> >>>
> >>> Fixes: 36847f9e3e56 ("firmware: stratix10-svc: correct reconfig flag and timeout values")
> >>> Signed-off-by: Richard Gong <richard.gong@intel.com>
> >>> ---
> >>> v5: new add, add the missing standalone patch
> >>> ---
> >>>   include/linux/firmware/intel/stratix10-svc-client.h | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> >>> index a93d859..f843c6a 100644
> >>> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> >>> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> >>> @@ -56,7 +56,7 @@
> >>>    * COMMAND_RECONFIG_FLAG_PARTIAL:
> >>>    * Set to FPGA configuration type (full or partial).
> >>>    */
> >>> -#define COMMAND_RECONFIG_FLAG_PARTIAL    1
> >>> +#define COMMAND_RECONFIG_FLAG_PARTIAL    0
> >>
> >> Is this the stand alone fix split from v3's patch 1 ?
> >>
> >> https://lore.kernel.org/linux-fpga/YBFW50LPP%2FyEbvEW@kroah.com/
> >> Yes, it is a stand-alone patch.
> 
> Thanks.
> 
> Reviewed-by: Tom Rix <trix@redhat.com>
> 
> >
> >> Tom
> >>
> >>>     /**
> >>>    * Timeout settings for service clients:
> >>
> > Regards,
> > Richard
> >
> 

Applied to fixes.

Thanks,
Moritz

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Richard Gong]

Hi Moritz,

Thanks for approving the 1st patch of my version 5 patchest, which 
submitted on 02/09/21.

Can you help review the remaining 6 patches from the same version 5 
patchset? I need your ACKs to move forward, or please let me know if 
additional work is need.

Many thanks for your time again!

Regards,
Richard


On 2/25/21 7:07 AM, Gong, Richard wrote:
> Hi Moritz,
> 
> Sorry for asking.
> 
> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
> 
> Regards,
> Richard
> 
> -----Original Message-----
> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
> Sent: Tuesday, February 9, 2021 4:20 PM
> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Gong, Richard <richard.gong@intel.com>
> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
> 
> From: Richard Gong <richard.gong@intel.com>
> 
> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
> 
> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
> 
> Bitstream authentication makes sure a signed bitstream has valid signatures.
> 
> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
> 
> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature.
> 
> Richard Gong (7):
>    firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>    firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>    firmware: stratix10-svc: extend SVC driver to get the firmware version
>    fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>    fpga: of-fpga-region: add authenticate-fpga-config property
>    dt-bindings: fpga: add authenticate-fpga-config property
>    fpga: stratix10-soc: extend driver for bitstream authentication
> 
>   .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>   drivers/firmware/stratix10-svc.c                   | 12 ++++-
>   drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>   drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>   include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>   .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>   include/linux/fpga/fpga-mgr.h                      |  3 ++
>   7 files changed, 125 insertions(+), 18 deletions(-)
> 
> --
> 2.7.4
> 

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Tom Rix]

On 3/19/21 4:22 PM, Richard Gong wrote:
>
> Hi Moritz,
>
> Thanks for approving the 1st patch of my version 5 patchest, which submitted on 02/09/21.

This change

e23bd83368af ("firmware: stratix10-svc: fix kernel-doc markups")

Makes a lot of formatting changes in the same files as this patchset, including the first patch.

It would be good to try applying this patchset to char-misc-next and resubmit if there are conflicts.

>
> Can you help review the remaining 6 patches from the same version 5 patchset? I need your ACKs to move forward, or please let me know if additional work is need.

These changes look good to me.

I was looking at the patchset again seeing if the firmware/ parts could be split out.

Even though stratix10 is a fpga, from the MAINTAINERS file it is not clear to me if linux-fpga owns them and they come in on Moritz's branch.  I think this change is needed to the MAINTAINERS file to make that clearer.

diff --git a/MAINTAINERS b/MAINTAINERS
index aa84121c5611..1f68e9ff76de 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9193,7 +9193,8 @@ F:    tools/power/x86/intel-speed-select/
 
 INTEL STRATIX10 FIRMWARE DRIVERS
 M:    Richard Gong <richard.gong@linux.intel.com>
-L:    linux-kernel@vger.kernel.org
+R:    Tom Rix <trix@redhat.com>
+L:    linux-fpga@vger.kernel.org
 S:    Maintained
 F:    Documentation/ABI/testing/sysfs-devices-platform-stratix10-rsu
 F:    Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt

I also added myself as a reviewer because I want to help out.

Tom


>
> Many thanks for your time again!
>
> Regards,
> Richard
>
>
> On 2/25/21 7:07 AM, Gong, Richard wrote:
>> Hi Moritz,
>>
>> Sorry for asking.
>>
>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>
>> Regards,
>> Richard
>>
>> -----Original Message-----
>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>> Sent: Tuesday, February 9, 2021 4:20 PM
>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: Gong, Richard <richard.gong@intel.com>
>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>
>> From: Richard Gong <richard.gong@intel.com>
>>
>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>
>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>
>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>
>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>
>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature.
>>
>> Richard Gong (7):
>>    firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>    firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>    firmware: stratix10-svc: extend SVC driver to get the firmware version
>>    fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>    fpga: of-fpga-region: add authenticate-fpga-config property
>>    dt-bindings: fpga: add authenticate-fpga-config property
>>    fpga: stratix10-soc: extend driver for bitstream authentication
>>
>>   .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>   drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>   drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>   drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>   include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>   .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>   include/linux/fpga/fpga-mgr.h                      |  3 ++
>>   7 files changed, 125 insertions(+), 18 deletions(-)
>>
>> -- 
>> 2.7.4
>>
>

Re: FW: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Richard Gong]

Hi Tom,

> 
> 
> On 3/19/21 4:22 PM, Richard Gong wrote:
>>
>> Hi Moritz,
>>
>> Thanks for approving the 1st patch of my version 5 patchest, which submitted on 02/09/21.
> 
> This change
> 
> e23bd83368af ("firmware: stratix10-svc: fix kernel-doc markups")

This patch e23bd83368af is not from my version 5 patch set.
> 
> Makes a lot of formatting changes in the same files as this patchset, including the first patch.
> 
> It would be good to try applying this patchset to char-misc-next and resubmit if there are conflicts.
> 
>>
>> Can you help review the remaining 6 patches from the same version 5 patchset? I need your ACKs to move forward, or please let me know if additional work is need.
> 
> These changes look good to me.
> 
> I was looking at the patchset again seeing if the firmware/ parts could be split out.

No, we can't split out the firmware parts.
> 
> Even though stratix10 is a fpga, from the MAINTAINERS file it is not clear to me if linux-fpga owns them and they come in on Moritz's branch.  I think this change is needed to the MAINTAINERS file to make that clearer.
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index aa84121c5611..1f68e9ff76de 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9193,7 +9193,8 @@ F:    tools/power/x86/intel-speed-select/
>   
>   INTEL STRATIX10 FIRMWARE DRIVERS
>   M:    Richard Gong <richard.gong@linux.intel.com>
> -L:    linux-kernel@vger.kernel.org
> +R:    Tom Rix <trix@redhat.com>
> +L:    linux-fpga@vger.kernel.org
>   S:    Maintained
>   F:    Documentation/ABI/testing/sysfs-devices-platform-stratix10-rsu
>   F:    Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt
> 
> I also added myself as a reviewer because I want to help out.
> 
> Tom
> 

Regards,
Richard

> 
>>
>> Many thanks for your time again!
>>
>> Regards,
>> Richard
>>
>>
>> On 2/25/21 7:07 AM, Gong, Richard wrote:
>>> Hi Moritz,
>>>
>>> Sorry for asking.
>>>
>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>>
>>> Regards,
>>> Richard
>>>
>>> -----Original Message-----
>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Cc: Gong, Richard <richard.gong@intel.com>
>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>>
>>> From: Richard Gong <richard.gong@intel.com>
>>>
>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>>
>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>>
>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>>
>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>>
>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature.
>>>
>>> Richard Gong (7):
>>>     firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>     firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>     firmware: stratix10-svc: extend SVC driver to get the firmware version
>>>     fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>     fpga: of-fpga-region: add authenticate-fpga-config property
>>>     dt-bindings: fpga: add authenticate-fpga-config property
>>>     fpga: stratix10-soc: extend driver for bitstream authentication
>>>
>>>    .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>    drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>    drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>    drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>>    include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>    .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>    include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>    7 files changed, 125 insertions(+), 18 deletions(-)
>>>
>>> -- 
>>> 2.7.4
>>>
>>
> 

Re: FW: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Tom Rix]

On 3/21/21 2:05 PM, Richard Gong wrote:
>
> Hi Tom,
>
>>
>>
>> On 3/19/21 4:22 PM, Richard Gong wrote:
>>>
>>> Hi Moritz,
>>>
>>> Thanks for approving the 1st patch of my version 5 patchest, which submitted on 02/09/21.
>>
>> This change
>>
>> e23bd83368af ("firmware: stratix10-svc: fix kernel-doc markups")
>
> This patch e23bd83368af is not from my version 5 patch set.

Correct.

But since it is already in char-misc-next, your version 5 patchset will conflict with it.

I could not apply this patchset to my unoffical fpga-testing.

I am suggesting you do a test application of your patchset against char-misc-next.

And if you find there are issues, rebase your patchset. 

>>
>> Makes a lot of formatting changes in the same files as this patchset, including the first patch.
>>
>> It would be good to try applying this patchset to char-misc-next and resubmit if there are conflicts.
>>
>>>
>>> Can you help review the remaining 6 patches from the same version 5 patchset? I need your ACKs to move forward, or please let me know if additional work is need.
>>
>> These changes look good to me.
>>
>> I was looking at the patchset again seeing if the firmware/ parts could be split out.
>
> No, we can't split out the firmware parts.

ok

Tom

>>
>> Even though stratix10 is a fpga, from the MAINTAINERS file it is not clear to me if linux-fpga owns them and they come in on Moritz's branch.  I think this change is needed to the MAINTAINERS file to make that clearer.
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index aa84121c5611..1f68e9ff76de 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9193,7 +9193,8 @@ F:    tools/power/x86/intel-speed-select/
>>     INTEL STRATIX10 FIRMWARE DRIVERS
>>   M:    Richard Gong <richard.gong@linux.intel.com>
>> -L:    linux-kernel@vger.kernel.org
>> +R:    Tom Rix <trix@redhat.com>
>> +L:    linux-fpga@vger.kernel.org
>>   S:    Maintained
>>   F:    Documentation/ABI/testing/sysfs-devices-platform-stratix10-rsu
>>   F:    Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt
>>
>> I also added myself as a reviewer because I want to help out.
>>
>> Tom
>>
>
> Regards,
> Richard
>
>>
>>>
>>> Many thanks for your time again!
>>>
>>> Regards,
>>> Richard
>>>
>>>
>>> On 2/25/21 7:07 AM, Gong, Richard wrote:
>>>> Hi Moritz,
>>>>
>>>> Sorry for asking.
>>>>
>>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>>>
>>>> Regards,
>>>> Richard
>>>>
>>>> -----Original Message-----
>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Cc: Gong, Richard <richard.gong@intel.com>
>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>>>
>>>> From: Richard Gong <richard.gong@intel.com>
>>>>
>>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>>>
>>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>>>
>>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>>>
>>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>>>
>>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature.
>>>>
>>>> Richard Gong (7):
>>>>     firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>>     firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>>     firmware: stratix10-svc: extend SVC driver to get the firmware version
>>>>     fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>>     fpga: of-fpga-region: add authenticate-fpga-config property
>>>>     dt-bindings: fpga: add authenticate-fpga-config property
>>>>     fpga: stratix10-soc: extend driver for bitstream authentication
>>>>
>>>>    .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>>    drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>>    drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>>    drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>>>    include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>>    .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>>    include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>>    7 files changed, 125 insertions(+), 18 deletions(-)
>>>>
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>
>

Re: FW: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Richard Gong]

Hi Tom,

On 3/22/21 8:53 AM, Tom Rix wrote:
> 
> On 3/21/21 2:05 PM, Richard Gong wrote:
>>
>> Hi Tom >>
>>>
>>>
>>> On 3/19/21 4:22 PM, Richard Gong wrote:
>>>>
>>>> Hi Moritz,
>>>>
>>>> Thanks for approving the 1st patch of my version 5 patchest, which submitted on 02/09/21.
>>>
>>> This change
>>>
>>> e23bd83368af ("firmware: stratix10-svc: fix kernel-doc markups")
>>
>> This patch e23bd83368af is not from my version 5 patch set.
> 
> Correct.
> 
> But since it is already in char-misc-next, your version 5 patchset will conflict with it.
> 
> I could not apply this patchset to my unoffical fpga-testing.
> 
> I am suggesting you do a test application of your patchset against char-misc-next.
> 
> And if you find there are issues, rebase your patchset.
> 

I tried to apply my patchset to the top of char-misc-next, but I didn't 
see any conflicts.

c7582d1 fpga: stratix10-soc: extend driver for bitstream authentication
2c9ecd3 dt-bindings: fpga: add authenticate-fpga-config property
6244115 fpga: of-fpga-region: add authenticate-fpga-config property
da274c9 fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
9f93cad firmware: stratix10-svc: extend SVC driver to get the firmware 
version
eda6b51 firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
91aff09 firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
83be46e Merge v5.12-rc3 into char-misc-next

Regards,
Richard

>>>
>>> Makes a lot of formatting changes in the same files as this patchset, including the first patch.
>>>
>>> It would be good to try applying this patchset to char-misc-next and resubmit if there are conflicts.
>>>
>>>>
>>>> Can you help review the remaining 6 patches from the same version 5 patchset? I need your ACKs to move forward, or please let me know if additional work is need.
>>>
>>> These changes look good to me.
>>>
>>> I was looking at the patchset again seeing if the firmware/ parts could be split out.
>>
>> No, we can't split out the firmware parts.
> 
> ok
> 
> Tom
> 
>>>
>>> Even though stratix10 is a fpga, from the MAINTAINERS file it is not clear to me if linux-fpga owns them and they come in on Moritz's branch.  I think this change is needed to the MAINTAINERS file to make that clearer.
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index aa84121c5611..1f68e9ff76de 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -9193,7 +9193,8 @@ F:    tools/power/x86/intel-speed-select/
>>>      INTEL STRATIX10 FIRMWARE DRIVERS
>>>    M:    Richard Gong <richard.gong@linux.intel.com>
>>> -L:    linux-kernel@vger.kernel.org
>>> +R:    Tom Rix <trix@redhat.com>
>>> +L:    linux-fpga@vger.kernel.org
>>>    S:    Maintained
>>>    F:    Documentation/ABI/testing/sysfs-devices-platform-stratix10-rsu
>>>    F:    Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt
>>>
>>> I also added myself as a reviewer because I want to help out.
>>>
>>> Tom
>>>
>>
>> Regards,
>> Richard
>>
>>>
>>>>
>>>> Many thanks for your time again!
>>>>
>>>> Regards,
>>>> Richard
>>>>
>>>>
>>>> On 2/25/21 7:07 AM, Gong, Richard wrote:
>>>>> Hi Moritz,
>>>>>
>>>>> Sorry for asking.
>>>>>
>>>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>>>>
>>>>> Regards,
>>>>> Richard
>>>>>
>>>>> -----Original Message-----
>>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>>>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>>>> Cc: Gong, Richard <richard.gong@intel.com>
>>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>>>>
>>>>> From: Richard Gong <richard.gong@intel.com>
>>>>>
>>>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>>>>
>>>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>>>>
>>>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>>>>
>>>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>>>>
>>>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature.
>>>>>
>>>>> Richard Gong (7):
>>>>>      firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>>>      firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>>>      firmware: stratix10-svc: extend SVC driver to get the firmware version
>>>>>      fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>>>      fpga: of-fpga-region: add authenticate-fpga-config property
>>>>>      dt-bindings: fpga: add authenticate-fpga-config property
>>>>>      fpga: stratix10-soc: extend driver for bitstream authentication
>>>>>
>>>>>     .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>>>     drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>>>     drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>>>     drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>>>>     include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>>>     .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>>>     include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>>>     7 files changed, 125 insertions(+), 18 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>>
>>>
>>
> 

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Moritz Fischer]

Hi Richard, Russ,

On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
> Hi Moritz,
> 
> Sorry for asking.
> 
> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
> 
> Regards,
> Richard
> 
> -----Original Message-----
> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
> Sent: Tuesday, February 9, 2021 4:20 PM
> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Gong, Richard <richard.gong@intel.com>
> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
> 
> From: Richard Gong <richard.gong@intel.com>
> 
> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
> 
> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
> 
> Bitstream authentication makes sure a signed bitstream has valid signatures.
> 
> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
> 
> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
> 
> Richard Gong (7):
>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>   firmware: stratix10-svc: extend SVC driver to get the firmware version
>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>   fpga: of-fpga-region: add authenticate-fpga-config property
>   dt-bindings: fpga: add authenticate-fpga-config property
>   fpga: stratix10-soc: extend driver for bitstream authentication
> 
>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>  include/linux/fpga/fpga-mgr.h                      |  3 ++
>  7 files changed, 125 insertions(+), 18 deletions(-)
> 
> --
> 2.7.4
> 

Apologies for the epic delay in getting back to this, I took another
look at this patchset and Russ' patchset.

TL;DR I'm not really a fan of using device-tree overlays for this (and
again, apologies, I should've voiced this earlier ...).

Anyways, let's find a common API for this and Russ' work, they're trying
to achieve the same / similar thing, they should use the same API.

I'd like to re-invetigate the possiblity to extend FPGA Manager with
'secure update' ops that work for both these use-cases (and I susspect
hte XRT patchset will follow with a similar requirement, right after).

- Moritz

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Tom Rix]

On 3/27/21 11:09 AM, Moritz Fischer wrote:
> Hi Richard, Russ,
>
> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
>> Hi Moritz,
>>
>> Sorry for asking.
>>
>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>
>> Regards,
>> Richard
>>
>> -----Original Message-----
>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
>> Sent: Tuesday, February 9, 2021 4:20 PM
>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: Gong, Richard <richard.gong@intel.com>
>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>
>> From: Richard Gong <richard.gong@intel.com>
>>
>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>
>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>
>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>
>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>
>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
>>
>> Richard Gong (7):
>>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>   firmware: stratix10-svc: extend SVC driver to get the firmware version
>>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>   fpga: of-fpga-region: add authenticate-fpga-config property
>>   dt-bindings: fpga: add authenticate-fpga-config property
>>   fpga: stratix10-soc: extend driver for bitstream authentication
>>
>>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>  include/linux/fpga/fpga-mgr.h                      |  3 ++
>>  7 files changed, 125 insertions(+), 18 deletions(-)
>>
>> --
>> 2.7.4
>>
> Apologies for the epic delay in getting back to this, I took another
> look at this patchset and Russ' patchset.
>
> TL;DR I'm not really a fan of using device-tree overlays for this (and
> again, apologies, I should've voiced this earlier ...).
>
> Anyways, let's find a common API for this and Russ' work, they're trying
> to achieve the same / similar thing, they should use the same API.
>
> I'd like to re-invetigate the possiblity to extend FPGA Manager with
> 'secure update' ops that work for both these use-cases (and I susspect
> hte XRT patchset will follow with a similar requirement, right after).

The xrt patchset makes heavy use of device trees.

What is the general guidance for device tree usage ?

Tom

>
> - Moritz
>

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Moritz Fischer]

Tom,

On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
> 
> On 3/27/21 11:09 AM, Moritz Fischer wrote:
> > Hi Richard, Russ,
> >
> > On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
> >> Hi Moritz,
> >>
> >> Sorry for asking.
> >>
> >> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
> >>
> >> Regards,
> >> Richard
> >>
> >> -----Original Message-----
> >> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
> >> Sent: Tuesday, February 9, 2021 4:20 PM
> >> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Cc: Gong, Richard <richard.gong@intel.com>
> >> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
> >>
> >> From: Richard Gong <richard.gong@intel.com>
> >>
> >> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
> >>
> >> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
> >>
> >> Bitstream authentication makes sure a signed bitstream has valid signatures.
> >>
> >> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
> >>
> >> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
> >>
> >> Richard Gong (7):
> >>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
> >>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
> >>   firmware: stratix10-svc: extend SVC driver to get the firmware version
> >>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
> >>   fpga: of-fpga-region: add authenticate-fpga-config property
> >>   dt-bindings: fpga: add authenticate-fpga-config property
> >>   fpga: stratix10-soc: extend driver for bitstream authentication
> >>
> >>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
> >>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
> >>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
> >>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
> >>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
> >>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
> >>  include/linux/fpga/fpga-mgr.h                      |  3 ++
> >>  7 files changed, 125 insertions(+), 18 deletions(-)
> >>
> >> --
> >> 2.7.4
> >>
> > Apologies for the epic delay in getting back to this, I took another
> > look at this patchset and Russ' patchset.
> >
> > TL;DR I'm not really a fan of using device-tree overlays for this (and
> > again, apologies, I should've voiced this earlier ...).
> >
> > Anyways, let's find a common API for this and Russ' work, they're trying
> > to achieve the same / similar thing, they should use the same API.
> >
> > I'd like to re-invetigate the possiblity to extend FPGA Manager with
> > 'secure update' ops that work for both these use-cases (and I susspect
> > hte XRT patchset will follow with a similar requirement, right after).
> 
> The xrt patchset makes heavy use of device trees.
> 
> What is the general guidance for device tree usage ?

I'm not generally against using device tree, it has its place. To
describe hardware (and hardware *changes* with overlays) :)

What I don't like about this particular implementation w.r.t device-tree
usage is that it uses DT overlays as a mechanism to program the flash --
in place of having an API to do so.

One could add device-nodes during the DT overlay application, while the
FPGA doesn't actually get programmed with a new runtime image -- meaning
live DT and actual hardware state diverged -- worst case it'd crash.

So when roughly at the same time (from the same company even) we have two
patchsets that do similar things with radically different APIs I think
we should pause, and reflect on whether we can come up with something
that works for both :)

TL;DR the firmware parts to authenticate the bitstream look fine to me, the
way we tie it into the FPGA region I'm not a fan of.

- Moritz

Re: [PATCHv5 3/7] firmware: stratix10-svc: extend SVC driver to get the firmware version

[Moritz Fischer]

On Tue, Feb 09, 2021 at 04:20:29PM -0600, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> Extend Intel service layer driver to get the firmware version running at
> FPGA device. Therefore FPGA manager driver, one of Intel service layer
> driver's client, can decide whether to handle the newly added bitstream
> authentication function based on the retrieved firmware version.
> 
> Signed-off-by: Richard Gong <richard.gong@intel.com>
Acked-by: Moritz Fischr <mdf@kernel.org>
> ---
> v5: no change
> v4: no change
> v3: new added, changes for getting firmware version
> ---
>  drivers/firmware/stratix10-svc.c                    | 12 ++++++++++--
>  include/linux/firmware/intel/stratix10-smc.h        | 21 +++++++++++++++++++--
>  include/linux/firmware/intel/stratix10-svc-client.h |  4 ++++
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index 3aa489d..1443bbd 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -306,6 +306,7 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
>  		break;
>  	case COMMAND_RSU_RETRY:
>  	case COMMAND_RSU_MAX_RETRY:
> +	case COMMAND_FIRMWARE_VERSION:
>  		cb_data->status = BIT(SVC_STATUS_OK);
>  		cb_data->kaddr1 = &res.a1;
>  		break;
> @@ -422,6 +423,11 @@ static int svc_normal_to_secure_thread(void *data)
>  			a1 = 0;
>  			a2 = 0;
>  			break;
> +		case COMMAND_FIRMWARE_VERSION:
> +			a0 = INTEL_SIP_SMC_FIRMWARE_VERSION;
> +			a1 = 0;
> +			a2 = 0;
> +			break;
>  		default:
>  			pr_warn("it shouldn't happen\n");
>  			break;
> @@ -487,11 +493,13 @@ static int svc_normal_to_secure_thread(void *data)
>  
>  			/*
>  			 * be compatible with older version firmware which
> -			 * doesn't support RSU notify or retry
> +			 * doesn't support RSU notify, retry or bitstream
> +			 * authentication.
>  			 */
>  			if ((pdata->command == COMMAND_RSU_RETRY) ||
>  			    (pdata->command == COMMAND_RSU_MAX_RETRY) ||
> -				(pdata->command == COMMAND_RSU_NOTIFY)) {
> +			    (pdata->command == COMMAND_RSU_NOTIFY) ||
> +			    (pdata->command == COMMAND_FIRMWARE_VERSION)) {
>  				cbdata->status =
>  					BIT(SVC_STATUS_NO_SUPPORT);
>  				cbdata->kaddr1 = NULL;
> diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/linux/firmware/intel/stratix10-smc.h
> index c3e5ab0..505fcca 100644
> --- a/include/linux/firmware/intel/stratix10-smc.h
> +++ b/include/linux/firmware/intel/stratix10-smc.h
> @@ -321,8 +321,6 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
>  #define INTEL_SIP_SMC_ECC_DBE \
>  	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_ECC_DBE)
>  
> -#endif
> -
>  /**
>   * Request INTEL_SIP_SMC_RSU_NOTIFY
>   *
> @@ -404,3 +402,22 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
>  #define INTEL_SIP_SMC_FUNCID_RSU_MAX_RETRY 18
>  #define INTEL_SIP_SMC_RSU_MAX_RETRY \
>  	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_RSU_MAX_RETRY)
> +
> +/**
> + * Request INTEL_SIP_SMC_FIRMWARE_VERSION
> + *
> + * Sync call used to query the version of running firmware
> + *
> + * Call register usage:
> + * a0 INTEL_SIP_SMC_FIRMWARE_VERSION
> + * a1-a7 not used
> + *
> + * Return status:
> + * a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_STATUS_ERROR
> + * a1 running firmware version
> + */
> +#define INTEL_SIP_SMC_FUNCID_FIRMWARE_VERSION 31
> +#define INTEL_SIP_SMC_FIRMWARE_VERSION \
> +	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FIRMWARE_VERSION)
> +
> +#endif
> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> index fa9581d..193a2cf 100644
> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> @@ -109,6 +109,9 @@ struct stratix10_svc_chan;
>   *
>   * @COMMAND_RSU_DCMF_VERSION: query firmware for the DCMF version, return status
>   * is SVC_STATUS_OK or SVC_STATUS_ERROR
> + *
> + * @COMMAND_FIRMWARE_VERSION: query running firmware version, return status
> + * is SVC_STATUS_OK or SVC_STATUS_ERROR
>   */
>  enum stratix10_svc_command_code {
>  	COMMAND_NOOP = 0,
> @@ -122,6 +125,7 @@ enum stratix10_svc_command_code {
>  	COMMAND_RSU_RETRY,
>  	COMMAND_RSU_MAX_RETRY,
>  	COMMAND_RSU_DCMF_VERSION,
> +	COMMAND_FIRMWARE_VERSION,
>  };
>  
>  /**
> -- 
> 2.7.4
> 

Thanks,
Moritz

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Russ Weight]

Moritz,

On 3/28/21 10:20 AM, Moritz Fischer wrote:
> Tom,
>
> On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
>> On 3/27/21 11:09 AM, Moritz Fischer wrote:
>>> Hi Richard, Russ,
>>>
>>> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
>>>> Hi Moritz,
>>>>
>>>> Sorry for asking.
>>>>
>>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>>>
>>>> Regards,
>>>> Richard
>>>>
>>>> -----Original Message-----
>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
>>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Cc: Gong, Richard <richard.gong@intel.com>
>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>>>
>>>> From: Richard Gong <richard.gong@intel.com>
>>>>
>>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>>>
>>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>>>
>>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>>>
>>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>>>
>>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
>>>>
>>>> Richard Gong (7):
>>>>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>>   firmware: stratix10-svc: extend SVC driver to get the firmware version
>>>>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>>   fpga: of-fpga-region: add authenticate-fpga-config property
>>>>   dt-bindings: fpga: add authenticate-fpga-config property
>>>>   fpga: stratix10-soc: extend driver for bitstream authentication
>>>>
>>>>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>>>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>>  include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>>  7 files changed, 125 insertions(+), 18 deletions(-)
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>> Apologies for the epic delay in getting back to this, I took another
>>> look at this patchset and Russ' patchset.
>>>
>>> TL;DR I'm not really a fan of using device-tree overlays for this (and
>>> again, apologies, I should've voiced this earlier ...).
>>>
>>> Anyways, let's find a common API for this and Russ' work, they're trying
>>> to achieve the same / similar thing, they should use the same API.
>>>
>>> I'd like to re-invetigate the possiblity to extend FPGA Manager with
>>> 'secure update' ops that work for both these use-cases (and I susspect
>>> hte XRT patchset will follow with a similar requirement, right after).

Richard and I had an initial conversation today. I'll start looking at how secure operations can be integrated into the fpga manager.

More to come...

Thanks,
- Russ

>> The xrt patchset makes heavy use of device trees.
>>
>> What is the general guidance for device tree usage ?
> I'm not generally against using device tree, it has its place. To
> describe hardware (and hardware *changes* with overlays) :)
>
> What I don't like about this particular implementation w.r.t device-tree
> usage is that it uses DT overlays as a mechanism to program the flash --
> in place of having an API to do so.
>
> One could add device-nodes during the DT overlay application, while the
> FPGA doesn't actually get programmed with a new runtime image -- meaning
> live DT and actual hardware state diverged -- worst case it'd crash.
>
> So when roughly at the same time (from the same company even) we have two
> patchsets that do similar things with radically different APIs I think
> we should pause, and reflect on whether we can come up with something
> that works for both :)
>
> TL;DR the firmware parts to authenticate the bitstream look fine to me, the
> way we tie it into the FPGA region I'm not a fan of.
>
> - Moritz

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Moritz Fischer]

Hi Russ,
On Wed, Mar 31, 2021 at 11:47:26AM -0700, Russ Weight wrote:
> Moritz,
> 
> On 3/28/21 10:20 AM, Moritz Fischer wrote:
> > Tom,
> >
> > On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
> >> On 3/27/21 11:09 AM, Moritz Fischer wrote:
> >>> Hi Richard, Russ,
> >>>
> >>> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
> >>>> Hi Moritz,
> >>>>
> >>>> Sorry for asking.
> >>>>
> >>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
> >>>>
> >>>> Regards,
> >>>> Richard
> >>>>
> >>>> -----Original Message-----
> >>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
> >>>> Sent: Tuesday, February 9, 2021 4:20 PM
> >>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
> >>>> Cc: Gong, Richard <richard.gong@intel.com>
> >>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
> >>>>
> >>>> From: Richard Gong <richard.gong@intel.com>
> >>>>
> >>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
> >>>>
> >>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
> >>>>
> >>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
> >>>>
> >>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
> >>>>
> >>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
> >>>>
> >>>> Richard Gong (7):
> >>>>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
> >>>>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
> >>>>   firmware: stratix10-svc: extend SVC driver to get the firmware version
> >>>>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
> >>>>   fpga: of-fpga-region: add authenticate-fpga-config property
> >>>>   dt-bindings: fpga: add authenticate-fpga-config property
> >>>>   fpga: stratix10-soc: extend driver for bitstream authentication
> >>>>
> >>>>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
> >>>>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
> >>>>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
> >>>>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
> >>>>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
> >>>>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
> >>>>  include/linux/fpga/fpga-mgr.h                      |  3 ++
> >>>>  7 files changed, 125 insertions(+), 18 deletions(-)
> >>>>
> >>>> --
> >>>> 2.7.4
> >>>>
> >>> Apologies for the epic delay in getting back to this, I took another
> >>> look at this patchset and Russ' patchset.
> >>>
> >>> TL;DR I'm not really a fan of using device-tree overlays for this (and
> >>> again, apologies, I should've voiced this earlier ...).
> >>>
> >>> Anyways, let's find a common API for this and Russ' work, they're trying
> >>> to achieve the same / similar thing, they should use the same API.
> >>>
> >>> I'd like to re-invetigate the possiblity to extend FPGA Manager with
> >>> 'secure update' ops that work for both these use-cases (and I susspect
> >>> hte XRT patchset will follow with a similar requirement, right after).
> 
> Richard and I had an initial conversation today. I'll start looking at how secure operations can be integrated into the fpga manager.
> 
> More to come...

Great, feel free to send RFCs ahead.

Cheers,
Moritz

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Russ Weight]

Hi Moritz,

On 3/31/21 3:16 PM, Moritz Fischer wrote:
> Hi Russ,
> On Wed, Mar 31, 2021 at 11:47:26AM -0700, Russ Weight wrote:
>> Moritz,
>>
>> On 3/28/21 10:20 AM, Moritz Fischer wrote:
>>> Tom,
>>>
>>> On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
>>>> On 3/27/21 11:09 AM, Moritz Fischer wrote:
>>>>> Hi Richard, Russ,
>>>>>
>>>>> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
>>>>>> Hi Moritz,
>>>>>>
>>>>>> Sorry for asking.
>>>>>>
>>>>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>>>>>
>>>>>> Regards,
>>>>>> Richard
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
>>>>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>>>>> Cc: Gong, Richard <richard.gong@intel.com>
>>>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>>>>>
>>>>>> From: Richard Gong <richard.gong@intel.com>
>>>>>>
>>>>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>>>>>
>>>>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>>>>>
>>>>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>>>>>
>>>>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>>>>>
>>>>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
>>>>>>
>>>>>> Richard Gong (7):
>>>>>>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>>>>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>>>>   firmware: stratix10-svc: extend SVC driver to get the firmware version
>>>>>>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>>>>   fpga: of-fpga-region: add authenticate-fpga-config property
>>>>>>   dt-bindings: fpga: add authenticate-fpga-config property
>>>>>>   fpga: stratix10-soc: extend driver for bitstream authentication
>>>>>>
>>>>>>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>>>>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>>>>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>>>>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>>>>>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>>>>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>>>>  include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>>>>  7 files changed, 125 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>> Apologies for the epic delay in getting back to this, I took another
>>>>> look at this patchset and Russ' patchset.
>>>>>
>>>>> TL;DR I'm not really a fan of using device-tree overlays for this (and
>>>>> again, apologies, I should've voiced this earlier ...).
>>>>>
>>>>> Anyways, let's find a common API for this and Russ' work, they're trying
>>>>> to achieve the same / similar thing, they should use the same API.
>>>>>
>>>>> I'd like to re-invetigate the possiblity to extend FPGA Manager with
>>>>> 'secure update' ops that work for both these use-cases (and I susspect
>>>>> hte XRT patchset will follow with a similar requirement, right after).
>> Richard and I had an initial conversation today. I'll start looking at how secure operations can be integrated into the fpga manager.
>>
>> More to come...
> Great, feel free to send RFCs ahead.
>
> Cheers,
> Moritz
I have completed a comparison of the security manager and the FPGA manager
to see how the secure update functions can be integrated into the FPGA
manager. I'll send that out separately as an RFC document (it is about 150
lines).

FYI: In my conversations with Richard, we have learned that what we are
trying to accomplish is not as similar as it seemed. Richard is effectively
wanting to do a "dry-run" of an existing FPGA Manager function to verify
authentication of an image. Based on the results, higher-level code may
choose to write the image to flash.

The security manager is all about providing a common user API (sysfs
interface) for tranferring an image to an FPGA card while managing a
completion time (including authentication and FLASH) of up to 40 minutes.

- Russ

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Richard Gong]

Hi Moritz,

On 3/28/21 12:20 PM, Moritz Fischer wrote:
> Tom,
> 
> On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
>>
>> On 3/27/21 11:09 AM, Moritz Fischer wrote:
>>> Hi Richard, Russ,
>>>
>>> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
>>>> Hi Moritz,
>>>>
>>>> Sorry for asking.
>>>>
>>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
>>>>
>>>> Regards,
>>>> Richard
>>>>
>>>> -----Original Message-----
>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Cc: Gong, Richard <richard.gong@intel.com>
>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
>>>>
>>>> From: Richard Gong <richard.gong@intel.com>
>>>>
>>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
>>>>
>>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
>>>>
>>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
>>>>
>>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
>>>>
>>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature.
>>>>
>>>> Richard Gong (7):
>>>>    firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>>    firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>>    firmware: stratix10-svc: extend SVC driver to get the firmware version
>>>>    fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>>    fpga: of-fpga-region: add authenticate-fpga-config property
>>>>    dt-bindings: fpga: add authenticate-fpga-config property
>>>>    fpga: stratix10-soc: extend driver for bitstream authentication
>>>>
>>>>   .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>>   drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>>   drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>>   drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
>>>>   include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>>   .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>>   include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>>   7 files changed, 125 insertions(+), 18 deletions(-)
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>> Apologies for the epic delay in getting back to this, I took another
>>> look at this patchset and Russ' patchset.
>>>
>>> TL;DR I'm not really a fan of using device-tree overlays for this (and
>>> again, apologies, I should've voiced this earlier ...).
>>>
>>> Anyways, let's find a common API for this and Russ' work, they're trying
>>> to achieve the same / similar thing, they should use the same API.
>>>
>>> I'd like to re-invetigate the possiblity to extend FPGA Manager with
>>> 'secure update' ops that work for both these use-cases (and I susspect
>>> hte XRT patchset will follow with a similar requirement, right after).
>>
>> The xrt patchset makes heavy use of device trees.
>>
>> What is the general guidance for device tree usage ?
> 
> I'm not generally against using device tree, it has its place. To
> describe hardware (and hardware *changes* with overlays) :)
> 
> What I don't like about this particular implementation w.r.t device-tree
> usage is that it uses DT overlays as a mechanism to program the flash --
> in place of having an API to do so.
> 
> One could add device-nodes during the DT overlay application, while the
> FPGA doesn't actually get programmed with a new runtime image -- meaning
> live DT and actual hardware state diverged -- worst case it'd crash.
> 
> So when roughly at the same time (from the same company even) we have two
> patchsets that do similar things with radically different APIs I think
> we should pause, and reflect on whether we can come up with something
> that works for both :)
>

I discussed with Russ and studies his patches, came to realize that the 
work we had to accomplish was not same or similar. What I want to 
achieve is to verify the identity of the bitstream, which is like doing 
a "dry-run" to FPGA configuration.

Performing FPGA configuration (full or partial) through the device tree 
overlay is a method widely used by our customers.

Russ's approach utilizes a different user API which is a set of sysfs files.

If we depart from device tree overlay, then the end-user must utilize 2 
different mechanism or APIs (device tree overlay is used for 
full/partial configuration, and sysfs is used for bitstream 
authentication). Similarly low-level FPGA manager driver also needs to 
add additional codes. For the end-user the single and simple mechanism 
is always better choice, device tree overlay should be a better way to 
achieve that goal.

Regards,
Richard

> TL;DR the firmware parts to authenticate the bitstream look fine to me, the
> way we tie it into the FPGA region I'm not a fan of.
> 
> - Moritz
> 

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Richard Gong]

Hi Moritz,

Not sure if you have chance to view Russ's comments and my comments. 
Pleas let us know what you think so I can act accordingly.

I had a few discussions with Russ, and we all realized that the goals we 
were trying to achieve were not as similar as they seemed.

Regards,
Richard

On 4/12/21 8:41 PM, Richard Gong wrote:
> 
> Hi Moritz,
> 
> On 3/28/21 12:20 PM, Moritz Fischer wrote:
>> Tom,
>>
>> On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
>>>
>>> On 3/27/21 11:09 AM, Moritz Fischer wrote:
>>>> Hi Richard, Russ,
>>>>
>>>> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
>>>>> Hi Moritz,
>>>>>
>>>>> Sorry for asking.
>>>>>
>>>>> When you have chance, can you help review the version 5 patchset 
>>>>> submitted on 02/09/21?
>>>>>
>>>>> Regards,
>>>>> Richard
>>>>>
>>>>> -----Original Message-----
>>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com>
>>>>> Sent: Tuesday, February 9, 2021 4:20 PM
>>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; 
>>>>> linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
>>>>> Cc: Gong, Richard <richard.gong@intel.com>
>>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and 
>>>>> region
>>>>>
>>>>> From: Richard Gong <richard.gong@intel.com>
>>>>>
>>>>> This is 5th submission of Intel service layer and FPGA patches, 
>>>>> which includes the missing standalone patch in the 4th submission.
>>>>>
>>>>> This submission includes additional changes for Intel service layer 
>>>>> driver to get the firmware version running at FPGA SoC device. Then 
>>>>> FPGA manager driver, one of Intel service layer driver's client, 
>>>>> can decide whether to handle the newly added bitstream 
>>>>> authentication function based on the retrieved firmware version. So 
>>>>> that we can maintain FPGA manager driver the back compatible.
>>>>>
>>>>> Bitstream authentication makes sure a signed bitstream has valid 
>>>>> signatures.
>>>>>
>>>>> The customer sends the bitstream via FPGA framework and overlay, 
>>>>> the firmware will authenticate the bitstream but not program the 
>>>>> bitstream to device. If the authentication passes, the bitstream 
>>>>> will be programmed into QSPI flash and will be expected to boot 
>>>>> without issues.
>>>>>
>>>>> Extend Intel service layer, FPGA manager and region drivers to 
>>>>> support the bitstream authentication feature.
>>>>>
>>>>> Richard Gong (7):
>>>>>    firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
>>>>>    firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
>>>>>    firmware: stratix10-svc: extend SVC driver to get the firmware 
>>>>> version
>>>>>    fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
>>>>>    fpga: of-fpga-region: add authenticate-fpga-config property
>>>>>    dt-bindings: fpga: add authenticate-fpga-config property
>>>>>    fpga: stratix10-soc: extend driver for bitstream authentication
>>>>>
>>>>>   .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
>>>>>   drivers/firmware/stratix10-svc.c                   | 12 ++++-
>>>>>   drivers/fpga/of-fpga-region.c                      | 24 ++++++---
>>>>>   drivers/fpga/stratix10-soc.c                       | 62 
>>>>> +++++++++++++++++++---
>>>>>   include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
>>>>>   .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
>>>>>   include/linux/fpga/fpga-mgr.h                      |  3 ++
>>>>>   7 files changed, 125 insertions(+), 18 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>> Apologies for the epic delay in getting back to this, I took another
>>>> look at this patchset and Russ' patchset.
>>>>
>>>> TL;DR I'm not really a fan of using device-tree overlays for this (and
>>>> again, apologies, I should've voiced this earlier ...).
>>>>
>>>> Anyways, let's find a common API for this and Russ' work, they're 
>>>> trying
>>>> to achieve the same / similar thing, they should use the same API.
>>>>
>>>> I'd like to re-invetigate the possiblity to extend FPGA Manager with
>>>> 'secure update' ops that work for both these use-cases (and I susspect
>>>> hte XRT patchset will follow with a similar requirement, right after).
>>>
>>> The xrt patchset makes heavy use of device trees.
>>>
>>> What is the general guidance for device tree usage ?
>>
>> I'm not generally against using device tree, it has its place. To
>> describe hardware (and hardware *changes* with overlays) :)
>>
>> What I don't like about this particular implementation w.r.t device-tree
>> usage is that it uses DT overlays as a mechanism to program the flash --
>> in place of having an API to do so.
>>
>> One could add device-nodes during the DT overlay application, while the
>> FPGA doesn't actually get programmed with a new runtime image -- meaning
>> live DT and actual hardware state diverged -- worst case it'd crash.
>>
>> So when roughly at the same time (from the same company even) we have two
>> patchsets that do similar things with radically different APIs I think
>> we should pause, and reflect on whether we can come up with something
>> that works for both :)
>>
> 
> I discussed with Russ and studies his patches, came to realize that the 
> work we had to accomplish was not same or similar. What I want to 
> achieve is to verify the identity of the bitstream, which is like doing 
> a "dry-run" to FPGA configuration.
> 
> Performing FPGA configuration (full or partial) through the device tree 
> overlay is a method widely used by our customers.
> 
> Russ's approach utilizes a different user API which is a set of sysfs 
> files.
> 
> If we depart from device tree overlay, then the end-user must utilize 2 
> different mechanism or APIs (device tree overlay is used for 
> full/partial configuration, and sysfs is used for bitstream 
> authentication). Similarly low-level FPGA manager driver also needs to 
> add additional codes. For the end-user the single and simple mechanism 
> is always better choice, device tree overlay should be a better way to 
> achieve that goal.
> 
> Regards,
> Richard
> 
>> TL;DR the firmware parts to authenticate the bitstream look fine to 
>> me, the
>> way we tie it into the FPGA region I'm not a fan of.
>>
>> - Moritz
>>

Re: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region

[Moritz Fischer]

On Wed, Apr 07, 2021 at 04:27:03PM -0700, Russ Weight wrote:
> Hi Moritz,
> 
> On 3/31/21 3:16 PM, Moritz Fischer wrote:
> > Hi Russ,
> > On Wed, Mar 31, 2021 at 11:47:26AM -0700, Russ Weight wrote:
> >> Moritz,
> >>
> >> On 3/28/21 10:20 AM, Moritz Fischer wrote:
> >>> Tom,
> >>>
> >>> On Sun, Mar 28, 2021 at 08:40:24AM -0700, Tom Rix wrote:
> >>>> On 3/27/21 11:09 AM, Moritz Fischer wrote:
> >>>>> Hi Richard, Russ,
> >>>>>
> >>>>> On Thu, Feb 25, 2021 at 01:07:14PM +0000, Gong, Richard wrote:
> >>>>>> Hi Moritz,
> >>>>>>
> >>>>>> Sorry for asking.
> >>>>>>
> >>>>>> When you have chance, can you help review the version 5 patchset submitted on 02/09/21?
> >>>>>>
> >>>>>> Regards,
> >>>>>> Richard
> >>>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: richard.gong@linux.intel.com <richard.gong@linux.intel.com> 
> >>>>>> Sent: Tuesday, February 9, 2021 4:20 PM
> >>>>>> To: mdf@kernel.org; trix@redhat.com; gregkh@linuxfoundation.org; linux-fpga@vger.kernel.org; linux-kernel@vger.kernel.org
> >>>>>> Cc: Gong, Richard <richard.gong@intel.com>
> >>>>>> Subject: [PATCHv5 0/7] Extend Intel service layer, FPGA manager and region
> >>>>>>
> >>>>>> From: Richard Gong <richard.gong@intel.com>
> >>>>>>
> >>>>>> This is 5th submission of Intel service layer and FPGA patches, which includes the missing standalone patch in the 4th submission.
> >>>>>>
> >>>>>> This submission includes additional changes for Intel service layer driver to get the firmware version running at FPGA SoC device. Then FPGA manager driver, one of Intel service layer driver's client, can decide whether to handle the newly added bitstream authentication function based on the retrieved firmware version. So that we can maintain FPGA manager driver the back compatible.
> >>>>>>
> >>>>>> Bitstream authentication makes sure a signed bitstream has valid signatures.
> >>>>>>
> >>>>>> The customer sends the bitstream via FPGA framework and overlay, the firmware will authenticate the bitstream but not program the bitstream to device. If the authentication passes, the bitstream will be programmed into QSPI flash and will be expected to boot without issues.
> >>>>>>
> >>>>>> Extend Intel service layer, FPGA manager and region drivers to support the bitstream authentication feature. 
> >>>>>>
> >>>>>> Richard Gong (7):
> >>>>>>   firmware: stratix10-svc: reset COMMAND_RECONFIG_FLAG_PARTIAL to 0
> >>>>>>   firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag
> >>>>>>   firmware: stratix10-svc: extend SVC driver to get the firmware version
> >>>>>>   fpga: fpga-mgr: add FPGA_MGR_BITSTREAM_AUTHENTICATE flag
> >>>>>>   fpga: of-fpga-region: add authenticate-fpga-config property
> >>>>>>   dt-bindings: fpga: add authenticate-fpga-config property
> >>>>>>   fpga: stratix10-soc: extend driver for bitstream authentication
> >>>>>>
> >>>>>>  .../devicetree/bindings/fpga/fpga-region.txt       | 10 ++++
> >>>>>>  drivers/firmware/stratix10-svc.c                   | 12 ++++-
> >>>>>>  drivers/fpga/of-fpga-region.c                      | 24 ++++++---
> >>>>>>  drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
> >>>>>>  include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
> >>>>>>  .../linux/firmware/intel/stratix10-svc-client.h    | 11 +++-
> >>>>>>  include/linux/fpga/fpga-mgr.h                      |  3 ++
> >>>>>>  7 files changed, 125 insertions(+), 18 deletions(-)
> >>>>>>
> >>>>>> --
> >>>>>> 2.7.4
> >>>>>>
> >>>>> Apologies for the epic delay in getting back to this, I took another
> >>>>> look at this patchset and Russ' patchset.
> >>>>>
> >>>>> TL;DR I'm not really a fan of using device-tree overlays for this (and
> >>>>> again, apologies, I should've voiced this earlier ...).
> >>>>>
> >>>>> Anyways, let's find a common API for this and Russ' work, they're trying
> >>>>> to achieve the same / similar thing, they should use the same API.
> >>>>>
> >>>>> I'd like to re-invetigate the possiblity to extend FPGA Manager with
> >>>>> 'secure update' ops that work for both these use-cases (and I susspect
> >>>>> hte XRT patchset will follow with a similar requirement, right after).
> >> Richard and I had an initial conversation today. I'll start looking at how secure operations can be integrated into the fpga manager.
> >>
> >> More to come...
> > Great, feel free to send RFCs ahead.
> >
> > Cheers,
> > Moritz
> I have completed a comparison of the security manager and the FPGA manager
> to see how the secure update functions can be integrated into the FPGA
> manager. I'll send that out separately as an RFC document (it is about 150
> lines).
> 
> FYI: In my conversations with Richard, we have learned that what we are
> trying to accomplish is not as similar as it seemed. Richard is effectively
> wanting to do a "dry-run" of an existing FPGA Manager function to verify
> authentication of an image. Based on the results, higher-level code may
> choose to write the image to flash.

Ok, as mentioned on the other thread. Let's replace the overlay code
with a sysfs entry for dry-run or verify-image or something of that
sort.

I don't see what the overlay mechanism adds in this case since we're not
trying to load drivers.

- Moritz