[PATCHv3 0/6] [PATCHv3 0/6] Extend Intel service layer, FPGA manager and region

[PATCHv3 0/6] [PATCHv3 0/6] Extend Intel service layer, FPGA manager and region

[richard.gong]

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

This is 3rd submission of Intel service layer and FPGA patches.

This submission include 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.

The customer wants to verify that a FPGA bitstream can be started properly
before saving the bitstream to the QSPI flash memory.

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 (6):
  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_BITSTREM_AUTHENTICATION 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       |  1 +
 drivers/firmware/stratix10-svc.c                   | 12 ++++-
 drivers/fpga/of-fpga-region.c                      |  3 ++
 drivers/fpga/stratix10-soc.c                       | 62 +++++++++++++++++++---
 include/linux/firmware/intel/stratix10-smc.h       | 21 +++++++-
 .../linux/firmware/intel/stratix10-svc-client.h    | 15 ++++--
 include/linux/fpga/fpga-mgr.h                      | 13 +++--
 7 files changed, 109 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCHv3 1/6] 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.

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

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
v3: no change
v2: new added
---
 include/linux/firmware/intel/stratix10-svc-client.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index ebc2956..7ada1f2 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -51,12 +51,17 @@
 #define SVC_STATUS_NO_SUPPORT		6
 
 /*
- * Flag bit for COMMAND_RECONFIG
+ * Flag for COMMAND_RECONFIG, in bit number
  *
  * COMMAND_RECONFIG_FLAG_PARTIAL:
- * Set to FPGA configuration type (full or partial).
+ * Set for partial FPGA configuration.
+ *
+ * COMMAND_AUTHENTICATE_BITSTREAM:
+ * Set for bitstream authentication, which makes sure a signed bitstream
+ * has valid signatures before committing it to QSPI flash memory.
  */
-#define COMMAND_RECONFIG_FLAG_PARTIAL	1
+#define COMMAND_RECONFIG_FLAG_PARTIAL	0
+#define COMMAND_AUTHENTICATE_BITSTREAM	1
 
 /*
  * Timeout settings for service clients:
-- 
2.7.4

[PATCHv3 2/6] 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>
---
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 7ada1f2..15e5477 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

[PATCHv3 3/6] fpga: fpga-mgr: add FPGA_MGR_BITSTREM_AUTHENTICATION flag

[richard.gong]

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

Add FPGA_MGR_BITSTREM_AUTHENTICATION 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>
---
v3: no change
v2: align all FPGA_MGR_* flags
    update the commit messages
---
 include/linux/fpga/fpga-mgr.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 2bc3030..4fb3400 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_BITSTREM_AUTHENTICATION: 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_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_BITSTREM_AUTHENTICATION	BIT(5)
 
 /**
  * struct fpga_image_info - information specific to a FPGA image
-- 
2.7.4

[PATCHv3 4/6] 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>
---
v3: no change
v2: changed in alphabetical order
---
 drivers/fpga/of-fpga-region.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index e405309..3840883 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -219,6 +219,9 @@ 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, "authenticate-fpga-config"))
+		info->flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
+
 	if (of_property_read_bool(overlay, "partial-fpga-config"))
 		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
 
-- 
2.7.4

[PATCHv3 5/6] 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>
---
v3: no change
v2: put authenticate-fpga-config above partial-fpga-config
    update commit messages
---
 Documentation/devicetree/bindings/fpga/fpga-region.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
index e811cf8..d0d3234 100644
--- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
@@ -182,6 +182,7 @@ 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.
 - 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

[PATCHv3 6/6] 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>
---
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..59d738c 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_BITSTREM_AUTHENTICATION) {
+		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: [PATCHv3 1/6] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

[Tom Rix]

On 1/25/21 12:56 PM, richard.gong@linux.intel.com wrote:
> 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.
>
> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
> aligns with the firmware settings.
>
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
> v3: no change
> v2: new added
> ---
>  include/linux/firmware/intel/stratix10-svc-client.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> index ebc2956..7ada1f2 100644
> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> @@ -51,12 +51,17 @@
>  #define SVC_STATUS_NO_SUPPORT		6
>  
>  /*

This patch fails to apply, i believe the conflict is because in mainline this is '/**' not '/*'

Please check or point me at the branch/tag you are using.

I am using char-misc-next.

Tom

Tom

Re: [PATCHv3 1/6] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

[Richard Gong]

Hi Tom,

On 1/25/21 4:56 PM, Tom Rix wrote:
> 
> On 1/25/21 12:56 PM, richard.gong@linux.intel.com wrote:
>> 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.
>>
>> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
>> aligns with the firmware settings.
>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>> v3: no change
>> v2: new added
>> ---
>>   include/linux/firmware/intel/stratix10-svc-client.h | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
>> index ebc2956..7ada1f2 100644
>> --- a/include/linux/firmware/intel/stratix10-svc-client.h
>> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
>> @@ -51,12 +51,17 @@
>>   #define SVC_STATUS_NO_SUPPORT		6
>>   
>>   /*
> 
> This patch fails to apply, i believe the conflict is because in mainline this is '/**' not '/*'
> 
> Please check or point me at the branch/tag you are using.
> 

I am using next-20210125 tag.

> I am using char-misc-next.
> 
> Tom
> 
> Tom
> 
Regards,
Richard

Re: [PATCHv3 2/6] firmware: stratix10-svc: extend SVC driver to get the firmware version

[Moritz Fischer]

Hi Richard,

On Mon, Jan 25, 2021 at 02:56:24PM -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>
> ---
> v3: new added, changes for getting firmware version
Thanks for doing that.
> ---
>  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 7ada1f2..15e5477 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
Is DCMF explaines somewhere? Maybe I missed it.
> + *
> + * @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: [PATCHv3 3/6] fpga: fpga-mgr: add FPGA_MGR_BITSTREM_AUTHENTICATION flag

[Moritz Fischer]

On Mon, Jan 25, 2021 at 02:56:25PM -0600, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> Add FPGA_MGR_BITSTREM_AUTHENTICATION 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>
> ---
> v3: no change
> v2: align all FPGA_MGR_* flags
>     update the commit messages
> ---
>  include/linux/fpga/fpga-mgr.h | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 2bc3030..4fb3400 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_BITSTREM_AUTHENTICATION: 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_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_BITSTREM_AUTHENTICATION	BIT(5)
Consider FPGA_MGR_BITSTREAM_AUTHENTICATE (and fix typo)
>  
>  /**
>   * struct fpga_image_info - information specific to a FPGA image
> -- 
> 2.7.4
> 

Thanks,
Moritz

Re: [PATCHv3 5/6] dt-bindings: fpga: add authenticate-fpga-config property

[Moritz Fischer]

On Mon, Jan 25, 2021 at 02:56:27PM -0600, richard.gong@linux.intel.com wrote:
> 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>
> ---
> v3: no change
> v2: put authenticate-fpga-config above partial-fpga-config
>     update commit messages
> ---
>  Documentation/devicetree/bindings/fpga/fpga-region.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> index e811cf8..d0d3234 100644
> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> @@ -182,6 +182,7 @@ 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.
I don't understand. Can I do authenticate-fpga-config AND
partial-fpga-config?
>  - 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
> 
Please clarify,

Moritz

Re: [PATCHv3 6/6] fpga: stratix10-soc: extend driver for bitstream authentication

[Moritz Fischer]

On Mon, Jan 25, 2021 at 02:56:28PM -0600, richard.gong@linux.intel.com wrote:
> 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>
> ---
> 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..59d738c 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_BITSTREM_AUTHENTICATION) {
BITSTREAM ;-)

Nit: Maybe the flag could be FPGA_MGR_BITSTREAM_AUTHENTICATE and/or
FPGA_MGR_BITSTREAM_AUTH_AND_PERSIST.
> +		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
> 
Thanks,
Moritz

Re: [PATCHv3 4/6] fpga: of-fpga-region: add authenticate-fpga-config property

[Moritz Fischer]

On Mon, Jan 25, 2021 at 02:56:26PM -0600, richard.gong@linux.intel.com wrote:
> 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>
> ---
> v3: no change
> v2: changed in alphabetical order
> ---
>  drivers/fpga/of-fpga-region.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
> index e405309..3840883 100644
> --- a/drivers/fpga/of-fpga-region.c
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -219,6 +219,9 @@ 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, "authenticate-fpga-config"))
> +		info->flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
> +
Should you check here that no new nodes are being added as you *only*
authenticate?

>  	if (of_property_read_bool(overlay, "partial-fpga-config"))
>  		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
>  
> -- 
> 2.7.4
> 

Thanks,
Moritz

Re: [PATCHv3 2/6] firmware: stratix10-svc: extend SVC driver to get the firmware version

[Richard Gong]

Hi Moritz,

Thanks for your reviews!

On 1/25/21 11:01 PM, Moritz Fischer wrote:
> Hi Richard,
> 
> On Mon, Jan 25, 2021 at 02:56:24PM -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>
>> ---
>> v3: new added, changes for getting firmware version
> Thanks for doing that.
>> ---
>>   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 7ada1f2..15e5477 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
> Is DCMF explaines somewhere? Maybe I missed it.

Documentation/ABI/testing/sysfs-devices-platform-stratix10-rsu.

RSU (Remote System Update) allows user to reliably update the QSPI 
configuration bitstream of an Intel Stratix10 SoC device.

When the user enables RSU function on the device, he/she need to use the 
Intel Quartus tools to build RSU flash image. The flash content should 
include DCMF and other components to make it have RSU function.

DCMF (Decision Configuration Management Firmware) serves as the decision 
maker to select the one of application images or factory image to load. 
When one of the application image failed, DCMF will locate and switch to 
the next application image. If all application failed, the factory image 
will be loaded.

>> + *
>> + * @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
> 
Regards,
Richard

Re: [PATCHv3 3/6] fpga: fpga-mgr: add FPGA_MGR_BITSTREM_AUTHENTICATION flag

[Richard Gong]

Hi Moritz,

On 1/25/21 11:04 PM, Moritz Fischer wrote:
> On Mon, Jan 25, 2021 at 02:56:25PM -0600, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Add FPGA_MGR_BITSTREM_AUTHENTICATION 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>
>> ---
>> v3: no change
>> v2: align all FPGA_MGR_* flags
>>      update the commit messages
>> ---
>>   include/linux/fpga/fpga-mgr.h | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 2bc3030..4fb3400 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_BITSTREM_AUTHENTICATION: 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_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_BITSTREM_AUTHENTICATION	BIT(5)
> Consider FPGA_MGR_BITSTREAM_AUTHENTICATE (and fix typo)

Thanks, I will correct that in next submission.

>>   
>>   /**
>>    * struct fpga_image_info - information specific to a FPGA image
>> -- 
>> 2.7.4
>>
> 
> Thanks,
> Moritz
> 
Regards,
Richard

Re: [PATCHv3 5/6] dt-bindings: fpga: add authenticate-fpga-config property

[Richard Gong]

On 1/25/21 11:05 PM, Moritz Fischer wrote:
> On Mon, Jan 25, 2021 at 02:56:27PM -0600, richard.gong@linux.intel.com wrote:
>> 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>
>> ---
>> v3: no change
>> v2: put authenticate-fpga-config above partial-fpga-config
>>      update commit messages
>> ---
>>   Documentation/devicetree/bindings/fpga/fpga-region.txt | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>> index e811cf8..d0d3234 100644
>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>> @@ -182,6 +182,7 @@ 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.
> I don't understand. Can I do authenticate-fpga-config AND
> partial-fpga-config?

Yes, but not simultaneously.

Flag authenticate-fpga-config is used to first check the integrity of 
the bitstream. If the authentication passes, the user can perform a full 
or partial configuration to actually configure the bistream to 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
>>
> Please clarify,
> 
> Moritz
> 
Regards,
Richard

Re: [PATCHv3 6/6] fpga: stratix10-soc: extend driver for bitstream authentication

[Richard Gong]

Hi Moritz,

On 1/25/21 11:09 PM, Moritz Fischer wrote:
> On Mon, Jan 25, 2021 at 02:56:28PM -0600, richard.gong@linux.intel.com wrote:
>> 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>
>> ---
>> 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..59d738c 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_BITSTREM_AUTHENTICATION) {
> BITSTREAM ;-)
> 
> Nit: Maybe the flag could be FPGA_MGR_BITSTREAM_AUTHENTICATE and/or
> FPGA_MGR_BITSTREAM_AUTH_AND_PERSIST.

Thanks, I will correct both in next submission.

>> +		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
>>
> Thanks,
> Moritz
> 
Regards,
Richard

Re: [PATCHv3 4/6] fpga: of-fpga-region: add authenticate-fpga-config property

[Richard Gong]

Hi Moritz,

On 1/25/21 11:10 PM, Moritz Fischer wrote:
> On Mon, Jan 25, 2021 at 02:56:26PM -0600, richard.gong@linux.intel.com wrote:
>> 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>
>> ---
>> v3: no change
>> v2: changed in alphabetical order
>> ---
>>   drivers/fpga/of-fpga-region.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
>> index e405309..3840883 100644
>> --- a/drivers/fpga/of-fpga-region.c
>> +++ b/drivers/fpga/of-fpga-region.c
>> @@ -219,6 +219,9 @@ 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, "authenticate-fpga-config"))
>> +		info->flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
>> +
> Should you check here that no new nodes are being added as you *only*
> authenticate?

Sure, I will add additional checks in next submission.

> 
>>   	if (of_property_read_bool(overlay, "partial-fpga-config"))
>>   		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
>>   
>> -- 
>> 2.7.4
>>
> 
> Thanks,
> Moritz
> 
Regards,
Richard

Re: [PATCHv3 1/6] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

[Greg KH]

On Mon, Jan 25, 2021 at 02:56:23PM -0600, richard.gong@linux.intel.com wrote:
> 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.
> 
> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
> aligns with the firmware settings.
> 
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
> v3: no change
> v2: new added
> ---
>  include/linux/firmware/intel/stratix10-svc-client.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> index ebc2956..7ada1f2 100644
> --- a/include/linux/firmware/intel/stratix10-svc-client.h
> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> @@ -51,12 +51,17 @@
>  #define SVC_STATUS_NO_SUPPORT		6
>  
>  /*
> - * Flag bit for COMMAND_RECONFIG
> + * Flag for COMMAND_RECONFIG, in bit number
>   *
>   * COMMAND_RECONFIG_FLAG_PARTIAL:
> - * Set to FPGA configuration type (full or partial).
> + * Set for partial FPGA configuration.
> + *
> + * COMMAND_AUTHENTICATE_BITSTREAM:
> + * Set for bitstream authentication, which makes sure a signed bitstream
> + * has valid signatures before committing it to QSPI flash memory.
>   */
> -#define COMMAND_RECONFIG_FLAG_PARTIAL	1
> +#define COMMAND_RECONFIG_FLAG_PARTIAL	0

So is this a bugfix, changing this value to the correct one?

If so, shouldn't this be a stand-alone patch and get backported to
stable kernel releases?

If not, then no one uses this flag today?

thanks,

greg k-h

Re: [PATCHv3 1/6] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

[Richard Gong]

Hi Greg,

Thanks for review!

On 1/27/21 6:04 AM, Greg KH wrote:
> On Mon, Jan 25, 2021 at 02:56:23PM -0600, richard.gong@linux.intel.com wrote:
>> 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.
>>
>> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
>> aligns with the firmware settings.
>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>> v3: no change
>> v2: new added
>> ---
>>   include/linux/firmware/intel/stratix10-svc-client.h | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
>> index ebc2956..7ada1f2 100644
>> --- a/include/linux/firmware/intel/stratix10-svc-client.h
>> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
>> @@ -51,12 +51,17 @@
>>   #define SVC_STATUS_NO_SUPPORT		6
>>   
>>   /*
>> - * Flag bit for COMMAND_RECONFIG
>> + * Flag for COMMAND_RECONFIG, in bit number
>>    *
>>    * COMMAND_RECONFIG_FLAG_PARTIAL:
>> - * Set to FPGA configuration type (full or partial).
>> + * Set for partial FPGA configuration.
>> + *
>> + * COMMAND_AUTHENTICATE_BITSTREAM:
>> + * Set for bitstream authentication, which makes sure a signed bitstream
>> + * has valid signatures before committing it to QSPI flash memory.
>>    */
>> -#define COMMAND_RECONFIG_FLAG_PARTIAL	1
>> +#define COMMAND_RECONFIG_FLAG_PARTIAL	0
> 
> So is this a bugfix, changing this value to the correct one?

Yes, it is a bug fix.
> 
> If so, shouldn't this be a stand-alone patch and get backported to
> stable kernel releases?

Sure, I will make change and submit again as a standalone patch.

> 
> If not, then no one uses this flag today?
> 
> thanks,
> 
> greg k-h
> 
Regards,
Richard

Re: [PATCHv3 1/6] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

[Moritz Fischer]

On Wed, Jan 27, 2021 at 07:05:41AM -0600, Richard Gong wrote:
> 
> Hi Greg,
> 
> Thanks for review!
> 
> On 1/27/21 6:04 AM, Greg KH wrote:
> > On Mon, Jan 25, 2021 at 02:56:23PM -0600, richard.gong@linux.intel.com wrote:
> > > 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.
> > > 
> > > Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
> > > aligns with the firmware settings.
> > > 
> > > Signed-off-by: Richard Gong <richard.gong@intel.com>
> > > ---
> > > v3: no change
> > > v2: new added
> > > ---
> > >   include/linux/firmware/intel/stratix10-svc-client.h | 11 ++++++++---
> > >   1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
> > > index ebc2956..7ada1f2 100644
> > > --- a/include/linux/firmware/intel/stratix10-svc-client.h
> > > +++ b/include/linux/firmware/intel/stratix10-svc-client.h
> > > @@ -51,12 +51,17 @@
> > >   #define SVC_STATUS_NO_SUPPORT		6
> > >   /*
> > > - * Flag bit for COMMAND_RECONFIG
> > > + * Flag for COMMAND_RECONFIG, in bit number
> > >    *
> > >    * COMMAND_RECONFIG_FLAG_PARTIAL:
> > > - * Set to FPGA configuration type (full or partial).
> > > + * Set for partial FPGA configuration.
> > > + *
> > > + * COMMAND_AUTHENTICATE_BITSTREAM:
> > > + * Set for bitstream authentication, which makes sure a signed bitstream
> > > + * has valid signatures before committing it to QSPI flash memory.
> > >    */
> > > -#define COMMAND_RECONFIG_FLAG_PARTIAL	1
> > > +#define COMMAND_RECONFIG_FLAG_PARTIAL	0
> > 
> > So is this a bugfix, changing this value to the correct one?
> 
> Yes, it is a bug fix.
Wat? This is a change in interface spec with the firmware. I thought the
whole point of the firmware version SVC call was to prevent breaking old
firmware?

Didn't we discuss this earlier?

> > 
> > If so, shouldn't this be a stand-alone patch and get backported to
> > stable kernel releases?
> 
> Sure, I will make change and submit again as a standalone patch.
> 
> > 
> > If not, then no one uses this flag today?
> > 
> > thanks,
> > 
> > greg k-h
> > 
> Regards,
> Richard

- Moritz

Re: [PATCHv3 1/6] firmware: stratix10-svc: add COMMAND_AUTHENTICATE_BITSTREAM flag

[Richard Gong]

Hi Moritz,

Sorry for the confusion.

On 1/27/21 3:41 PM, Moritz Fischer wrote:
> On Wed, Jan 27, 2021 at 07:05:41AM -0600, Richard Gong wrote:
>>
>> Hi Greg,
>>
>> Thanks for review!
>>
>> On 1/27/21 6:04 AM, Greg KH wrote:
>>> On Mon, Jan 25, 2021 at 02:56:23PM -0600, richard.gong@linux.intel.com wrote:
>>>> 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.
>>>>
>>>> Clean up COMMAND_RECONFIG_FLAG_PARTIAL flag by resetting it to 0, which
>>>> aligns with the firmware settings.
>>>>
>>>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>>>> ---
>>>> v3: no change
>>>> v2: new added
>>>> ---
>>>>    include/linux/firmware/intel/stratix10-svc-client.h | 11 ++++++++---
>>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
>>>> index ebc2956..7ada1f2 100644
>>>> --- a/include/linux/firmware/intel/stratix10-svc-client.h
>>>> +++ b/include/linux/firmware/intel/stratix10-svc-client.h
>>>> @@ -51,12 +51,17 @@
>>>>    #define SVC_STATUS_NO_SUPPORT		6
>>>>    /*
>>>> - * Flag bit for COMMAND_RECONFIG
>>>> + * Flag for COMMAND_RECONFIG, in bit number
>>>>     *
>>>>     * COMMAND_RECONFIG_FLAG_PARTIAL:
>>>> - * Set to FPGA configuration type (full or partial).
>>>> + * Set for partial FPGA configuration.
>>>> + *
>>>> + * COMMAND_AUTHENTICATE_BITSTREAM:
>>>> + * Set for bitstream authentication, which makes sure a signed bitstream
>>>> + * has valid signatures before committing it to QSPI flash memory.
>>>>     */
>>>> -#define COMMAND_RECONFIG_FLAG_PARTIAL	1
>>>> +#define COMMAND_RECONFIG_FLAG_PARTIAL	0
>>>
>>> So is this a bugfix, changing this value to the correct one?
>>
>> Yes, it is a bug fix.
> Wat? This is a change in interface spec with the firmware. I thought the
> whole point of the firmware version SVC call was to prevent breaking old
> firmware?
> 
> Didn't we discuss this earlier?
> 

We discussed before and I thought we were all aligned.

There are 2 aspects:
1. The purpose I changed COMMAND_RECONFIG_FLAG_PARTIAL to 0 from 1 is to 
align with the current firmware setting. This change will NOT break old 
firmware since always treats request with non-zero value as partial 
reconfiguration.

2. When we add new bitstream authentication function, the old firmware 
couldn't distinguish partial reconfiguration or bitstream authentication 
since the value of both requests were not zero. To address this back 
compatible issue, I extend Intel service layer driver for FPGA manager 
driver to get the running firmware version via SMC call. Then FPGA 
manager driver can decide whether to handle the newly added bitstream 
authentication based on the retrieved firmware version.

>>>
>>> If so, shouldn't this be a stand-alone patch and get backported to
>>> stable kernel releases?
>>
>> Sure, I will make change and submit again as a standalone patch.
>>
>>>
>>> If not, then no one uses this flag today?
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>> Regards,
>> Richard
> 
> - Moritz
> 
Regards,
Richard