[PATCHv1 0/4] Extend FPGA manager and region drivers for

[PATCHv1 0/4] Extend FPGA manager and region drivers for

[richard.gong]

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

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

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 FPGA manager and region drivers to support the bitstream
authentication feature.

Richard Gong (4):
  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: entend driver for bitstream authentication

 Documentation/devicetree/bindings/fpga/fpga-region.txt | 1 +
 drivers/fpga/of-fpga-region.c                          | 3 +++
 drivers/fpga/stratix10-soc.c                           | 5 ++++-
 include/linux/fpga/fpga-mgr.h                          | 3 +++
 4 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCHv1 1/4] 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.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
 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..1d65814 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
  */
 #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

[PATCHv1 2/4] 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.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
 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..c7c6d1c 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -228,6 +228,9 @@ static struct fpga_image_info *of_fpga_region_parse_ov(
 	if (of_property_read_bool(overlay, "encrypted-fpga-config"))
 		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
 
+	if (of_property_read_bool(overlay, "authenticate-fpga-config"))
+		info->flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
+
 	if (!of_property_read_string(overlay, "firmware-name",
 				     &firmware_name)) {
 		info->firmware_name = devm_kstrdup(dev, firmware_name,
-- 
2.7.4

[PATCHv1 3/4] 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.

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
 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..7a512bc 100644
--- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
@@ -187,6 +187,7 @@ Optional properties:
 - external-fpga-config : boolean, set if the FPGA has already been configured
 	prior to OS boot up.
 - encrypted-fpga-config : boolean, set if the bitstream is encrypted
+- authenticate-fpga-config : boolean, set if do bitstream authentication
 - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
 	bridges to successfully become enabled after the region has been
 	programmed.
-- 
2.7.4

[PATCHv1 4/4] fpga: stratix10-soc: entend driver for bitstream authentication

[richard.gong]

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

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

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
 drivers/fpga/stratix10-soc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index 657a70c..8a59365 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -185,7 +185,10 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
 	ctype.flags = 0;
 	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
 		dev_dbg(dev, "Requesting partial reconfiguration.\n");
-		ctype.flags |= BIT(COMMAND_RECONFIG_FLAG_PARTIAL);
+		ctype.flags |= FPGA_MGR_PARTIAL_RECONFIG;
+	} else if (info->flags & FPGA_MGR_BITSTREM_AUTHENTICATION) {
+		dev_dbg(dev, "Requesting bitstream authentication.\n");
+		ctype.flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
 	} else {
 		dev_dbg(dev, "Requesting full reconfiguration.\n");
 	}
-- 
2.7.4

Re: [PATCHv1 1/4] fpga: fpga-mgr: add FPGA_MGR_BITSTREM_AUTHENTICATION flag

[Tom Rix]

On 11/12/20 10:06 AM, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
>
> Add FPGA_MGR_BITSTREM_AUTHENTICATION flag for FPGA bitstream
> authentication.

Should improve this commit so explain what you mean authentication.

it could mean 'it wrote correctly' or 'it was signed correctly' or something else.

>
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  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..1d65814 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
>   */
>  #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)

A whitespace issue, the new BIT(5) should align with the others, so add two spaces to the others.

Tom

>  
>  /**
>   * struct fpga_image_info - information specific to a FPGA image

Re: [PATCHv1 2/4] fpga: of-fpga-region: add authenticate-fpga-config property

[Tom Rix]

On 11/12/20 10:06 AM, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
>
> Add authenticate-fpga-config property to support FPGA bitstream
> authentication.
>
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  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..c7c6d1c 100644
> --- a/drivers/fpga/of-fpga-region.c
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -228,6 +228,9 @@ static struct fpga_image_info *of_fpga_region_parse_ov(
>  	if (of_property_read_bool(overlay, "encrypted-fpga-config"))
>  		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
>  
> +	if (of_property_read_bool(overlay, "authenticate-fpga-config"))
> +		info->flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
> +
>  	if (!of_property_read_string(overlay, "firmware-name",
>  				     &firmware_name)) {
>  		info->firmware_name = devm_kstrdup(dev, firmware_name,

This looks fine.

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

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Tom Rix]

On 11/12/20 10:06 AM, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
>
> Add authenticate-fpga-config property for FPGA bitstream authentication.
>
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  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..7a512bc 100644
> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> @@ -187,6 +187,7 @@ Optional properties:
>  - external-fpga-config : boolean, set if the FPGA has already been configured
>  	prior to OS boot up.
>  - encrypted-fpga-config : boolean, set if the bitstream is encrypted
> +- authenticate-fpga-config : boolean, set if do bitstream authentication

The list is mostly in alphabetical order so the new 'authenticate-... ' should go at the top.

Improve what you mean by 'authentication' similar to my comment in the first patch.

Tom

>  - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>  	bridges to successfully become enabled after the region has been
>  	programmed.

Re: [PATCHv1 4/4] fpga: stratix10-soc: entend driver for bitstream authentication

[Tom Rix]

On 11/12/20 10:06 AM, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
>
> Exten FPGA manager driver to support FPGA bitstream authentication on
> Intel SocFPGA platforms.
>
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  drivers/fpga/stratix10-soc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index 657a70c..8a59365 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -185,7 +185,10 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>  	ctype.flags = 0;
>  	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>  		dev_dbg(dev, "Requesting partial reconfiguration.\n");
> -		ctype.flags |= BIT(COMMAND_RECONFIG_FLAG_PARTIAL);
> +		ctype.flags |= FPGA_MGR_PARTIAL_RECONFIG;

The change does not match the commit log.

Add some comment like..

'Cleanup setting of partial reconfig flag'

to cover the change.

Tom

> +	} else if (info->flags & FPGA_MGR_BITSTREM_AUTHENTICATION) {
> +		dev_dbg(dev, "Requesting bitstream authentication.\n");
> +		ctype.flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
>  	} else {
>  		dev_dbg(dev, "Requesting full reconfiguration.\n");
>  	}

Re: [PATCHv1 1/4] fpga: fpga-mgr: add FPGA_MGR_BITSTREM_AUTHENTICATION flag

[Richard Gong]

Hi Tom,

Thanks for review!

On 11/13/20 2:24 PM, Tom Rix wrote:
> 
> On 11/12/20 10:06 AM, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Add FPGA_MGR_BITSTREM_AUTHENTICATION flag for FPGA bitstream
>> authentication.
> 
> Should improve this commit so explain what you mean authentication.
> 
> it could mean 'it wrote correctly' or 'it was signed correctly' or something else.
> 

Authentication = make sure a signed bitstream has valid signatures 
before committing it to QSPI memory. I will update the commit messages 
in version 2.

>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>>   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..1d65814 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
>>    */
>>   #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)
> 
> A whitespace issue, the new BIT(5) should align with the others, so add two spaces to the others.
> 

There is only one space, also I ran checkpatch with strict option and 
didn't see any whitespace issue.

In the original patch, BIT(0) to BIT(4) align themselves. I am not sure 
why we see differently in email.

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

To align BIT(5) with others, I have to use additional tab to BIT(0) to 
BIT(4). But I don't think I should make such change on them, agree?

Regards,
Richard

> Tom
> 
>>   
>>   /**
>>    * struct fpga_image_info - information specific to a FPGA image
> 

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Richard Gong]

Hi Tom,

On 11/13/20 2:28 PM, Tom Rix wrote:
> 
> On 11/12/20 10:06 AM, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>>   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..7a512bc 100644
>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>> @@ -187,6 +187,7 @@ Optional properties:
>>   - external-fpga-config : boolean, set if the FPGA has already been configured
>>   	prior to OS boot up.
>>   - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
> 
> The list is mostly in alphabetical order so the new 'authenticate-... ' should go at the top.
> 

The original list is not in alphabetical order. The order of 
partial-fpga-config, external-fpga-config and encrypted-fpga-config here 
follows the implementation in the of-fpga-region.c file.

So authenticate-fpga-config should follow the way, correct?

> Improve what you mean by 'authentication' similar to my comment in the first patch.
> 

Will do in the version 2 submission.

Regards,
Richard

> Tom
> 
>>   - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>   	bridges to successfully become enabled after the region has been
>>   	programmed.
> 

Re: [PATCHv1 4/4] fpga: stratix10-soc: entend driver for bitstream authentication

[Richard Gong]

Hi Tom.

On 11/13/20 2:31 PM, Tom Rix wrote:
> 
> On 11/12/20 10:06 AM, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Exten FPGA manager driver to support FPGA bitstream authentication on
>> Intel SocFPGA platforms.
>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>>   drivers/fpga/stratix10-soc.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
>> index 657a70c..8a59365 100644
>> --- a/drivers/fpga/stratix10-soc.c
>> +++ b/drivers/fpga/stratix10-soc.c
>> @@ -185,7 +185,10 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>>   	ctype.flags = 0;
>>   	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>>   		dev_dbg(dev, "Requesting partial reconfiguration.\n");
>> -		ctype.flags |= BIT(COMMAND_RECONFIG_FLAG_PARTIAL);
>> +		ctype.flags |= FPGA_MGR_PARTIAL_RECONFIG;
> 
> The change does not match the commit log.
> 
> Add some comment like..
> 
> 'Cleanup setting of partial reconfig flag'
> 
> to cover the change.

I will make the cleanup change separately in a different patch.

Regards,
Richard


> 
> Tom
> 
>> +	} else if (info->flags & FPGA_MGR_BITSTREM_AUTHENTICATION) {
>> +		dev_dbg(dev, "Requesting bitstream authentication.\n");
>> +		ctype.flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
>>   	} else {
>>   		dev_dbg(dev, "Requesting full reconfiguration.\n");
>>   	}
> 

Re: [PATCHv1 1/4] fpga: fpga-mgr: add FPGA_MGR_BITSTREM_AUTHENTICATION flag

[Tom Rix]

On 11/14/20 6:30 AM, Richard Gong wrote:
>
>> A whitespace issue, the new BIT(5) should align with the others, so add two spaces to the others.
>>
>
> There is only one space, also I ran checkpatch with strict option and didn't see any whitespace issue.
>
> In the original patch, BIT(0) to BIT(4) align themselves. I am not sure why we see differently in email.
>
>  #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)
>
> To align BIT(5) with others, I have to use additional tab to BIT(0) to BIT(4). But I don't think I should make such change on them, agree?

The existing table of #defines has aligned values for BIT(0) to BIT(4)

Your addition of BIT(5) value has an inconsistent alignment with the others BIT(0) to BIT(4)

The alignment of all the values should be consistent.

Tom

>
> Regards,
> Richard
>
>> Tom
>>
>>>     /**
>>>    * struct fpga_image_info - information specific to a FPGA image
>>
>

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Tom Rix]

On 11/14/20 6:52 AM, Richard Gong wrote:
> Hi Tom,
>
>>>       prior to OS boot up.
>>>   - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
>>
>> The list is mostly in alphabetical order so the new 'authenticate-... ' should go at the top.
>>
>
> The original list is not in alphabetical order. The order of partial-fpga-config, external-fpga-config and encrypted-fpga-config here follows the implementation in the of-fpga-region.c file.
>
> So authenticate-fpga-config should follow the way, correct?
>
This is why i say 'mostly' ..

In general when listing options for a user to read, you should make it easy for them to find

the option they are looking for.  Ordering them alphabetically is an obvious but not the only

way.  I am not asking for you to fix the whole table, just what you are adding. If there is

a better way to organize them please propose the method.

Tom

>> Improve what you mean by 'authentication' similar to my comment in the first patch.
>>
>
> Will do in the version 2 submission.
>
> Regards,
> Richard
>
>> Tom
>>
>>>   - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>>       bridges to successfully become enabled after the region has been
>>>       programmed.
>>
>

Re: [PATCHv1 4/4] fpga: stratix10-soc: entend driver for bitstream authentication

[Moritz Fischer]

On Thu, Nov 12, 2020 at 12:06:43PM -0600, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> Exten FPGA manager driver to support FPGA bitstream authentication on
Nit: Extend
> Intel SocFPGA platforms.
> 
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  drivers/fpga/stratix10-soc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index 657a70c..8a59365 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -185,7 +185,10 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>  	ctype.flags = 0;
>  	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>  		dev_dbg(dev, "Requesting partial reconfiguration.\n");
> -		ctype.flags |= BIT(COMMAND_RECONFIG_FLAG_PARTIAL);
> +		ctype.flags |= FPGA_MGR_PARTIAL_RECONFIG;
I think we had this discussion during the original review of the
stratix10-soc driver?

Wasn't the point of using the BIT() to not assume alignment of FPGA_MGR
flags and firmware structure?

The FPGA_MGR_* bits are kernel internal and can therefore change, it
would be unfortunate to end up in a situation where this breaks the FW
interface (assuming firmware uses the value in pass-through which it
looks like is what is happening).

> +	} else if (info->flags & FPGA_MGR_BITSTREM_AUTHENTICATION) {
> +		dev_dbg(dev, "Requesting bitstream authentication.\n");
> +		ctype.flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
Do you want to change this to BIT(COMMAND_AUTHENTICATE_BITSTREAM) or
something like that?
>  	} else {
>  		dev_dbg(dev, "Requesting full reconfiguration.\n");
>  	}
> -- 
> 2.7.4
> 

Thanks,
Moritz

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Moritz Fischer]

Hi Richard,

On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> Add authenticate-fpga-config property for FPGA bitstream authentication.
> 
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  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..7a512bc 100644
> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> @@ -187,6 +187,7 @@ Optional properties:
>  - external-fpga-config : boolean, set if the FPGA has already been configured
>  	prior to OS boot up.
>  - encrypted-fpga-config : boolean, set if the bitstream is encrypted
> +- authenticate-fpga-config : boolean, set if do bitstream authentication
It is unclear to me from the description whether this entails
authentication + reconfiguration or just authentication.

If the latter is the case this should probably be described as such.

>  - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>  	bridges to successfully become enabled after the region has been
>  	programmed.
> -- 
> 2.7.4
> 

Thanks

Re: [PATCHv1 0/4] Extend FPGA manager and region drivers for

[Xu Yilun]

On Thu, Nov 12, 2020 at 12:06:39PM -0600, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> The customer wants to verify that a FPGA bitstream can be started properly
> before saving the bitstream to the QSPI flash memory.
> 
> 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.

So when we have successfully reprogramed region with the
FPGA_MGR_BITSTREM_AUTHENTICATION flag, the bitstream in QSPI flash is
updated but not activated, we need an FPGA reboot to activate it, is it?

Thanks,
Yilun

> 
> Extend FPGA manager and region drivers to support the bitstream
> authentication feature.
> 
> Richard Gong (4):
>   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: entend driver for bitstream authentication
> 
>  Documentation/devicetree/bindings/fpga/fpga-region.txt | 1 +
>  drivers/fpga/of-fpga-region.c                          | 3 +++
>  drivers/fpga/stratix10-soc.c                           | 5 ++++-
>  include/linux/fpga/fpga-mgr.h                          | 3 +++
>  4 files changed, 11 insertions(+), 1 deletion(-)
> 
> -- 
> 2.7.4

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Xu Yilun]

On Sun, Nov 15, 2020 at 11:21:06AM -0800, Moritz Fischer wrote:
> Hi Richard,
> 
> On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
> > From: Richard Gong <richard.gong@intel.com>
> > 
> > Add authenticate-fpga-config property for FPGA bitstream authentication.
> > 
> > Signed-off-by: Richard Gong <richard.gong@intel.com>
> > ---
> >  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..7a512bc 100644
> > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > @@ -187,6 +187,7 @@ Optional properties:
> >  - external-fpga-config : boolean, set if the FPGA has already been configured
> >  	prior to OS boot up.
> >  - encrypted-fpga-config : boolean, set if the bitstream is encrypted
> > +- authenticate-fpga-config : boolean, set if do bitstream authentication
> It is unclear to me from the description whether this entails
> authentication + reconfiguration or just authentication.
> 
> If the latter is the case this should probably be described as such.

If it is just authentication, do we still need to disable bridges in
fpga_region_program_fpga?

I'm wondering if the FPGA functionalities could still be working when
the authenticating is ongoing, or when the authenticating is failed.

Thanks,
Yilun

> 
> >  - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
> >  	bridges to successfully become enabled after the region has been
> >  	programmed.
> > -- 
> > 2.7.4
> > 
> 
> Thanks

Re: [PATCHv1 1/4] fpga: fpga-mgr: add FPGA_MGR_BITSTREM_AUTHENTICATION flag

[Richard Gong]

Hi Tom,

On 11/14/20 9:53 AM, Tom Rix wrote:
> 
> On 11/14/20 6:30 AM, Richard Gong wrote:
>>
>>> A whitespace issue, the new BIT(5) should align with the others, so add two spaces to the others.
>>>
>>
>> There is only one space, also I ran checkpatch with strict option and didn't see any whitespace issue.
>>
>> In the original patch, BIT(0) to BIT(4) align themselves. I am not sure why we see differently in email.
>>
>>   #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)
>>
>> To align BIT(5) with others, I have to use additional tab to BIT(0) to BIT(4). But I don't think I should make such change on them, agree?
> 
> The existing table of #defines has aligned values for BIT(0) to BIT(4)
> 
> Your addition of BIT(5) value has an inconsistent alignment with the others BIT(0) to BIT(4)
> 
> The alignment of all the values should be consistent.
> 

OK, I will make them all aligned.

Regards,
Richard

> Tom
> 
>>
>> Regards,
>> Richard
>>
>>> Tom
>>>
>>>>      /**
>>>>     * struct fpga_image_info - information specific to a FPGA image
>>>
>>
> 

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Richard Gong]

Hi Tom,


On 11/14/20 9:59 AM, Tom Rix wrote:
> 
> On 11/14/20 6:52 AM, Richard Gong wrote:
>> Hi Tom,
>>
>>>>        prior to OS boot up.
>>>>    - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>>>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
>>>
>>> The list is mostly in alphabetical order so the new 'authenticate-... ' should go at the top.
>>>
>>
>> The original list is not in alphabetical order. The order of partial-fpga-config, external-fpga-config and encrypted-fpga-config here follows the implementation in the of-fpga-region.c file.
>>
>> So authenticate-fpga-config should follow the way, correct?
>>
> This is why i say 'mostly' ..
> 
> In general when listing options for a user to read, you should make it easy for them to find
> 
> the option they are looking for.  Ordering them alphabetically is an obvious but not the only
> 
> way.  I am not asking for you to fix the whole table, just what you are adding. If there is
> 
> a better way to organize them please propose the method.
> 

How about put authenticate-fpga-config above partial-fpga-config? I 
would like to group all xxx-fpga-config flags together.


Regards,
Richard

> Tom
> 
>>> Improve what you mean by 'authentication' similar to my comment in the first patch.
>>>
>>
>> Will do in the version 2 submission.
>>
>> Regards,
>> Richard
>>
>>> Tom
>>>
>>>>    - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>>>        bridges to successfully become enabled after the region has been
>>>>        programmed.
>>>
>>
> 

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Richard Gong]

Hi Moritz,


On 11/15/20 1:21 PM, Moritz Fischer wrote:
> Hi Richard,
> 
> On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>>   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..7a512bc 100644
>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>> @@ -187,6 +187,7 @@ Optional properties:
>>   - external-fpga-config : boolean, set if the FPGA has already been configured
>>   	prior to OS boot up.
>>   - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
> It is unclear to me from the description whether this entails
> authentication + reconfiguration or just authentication.
> 
> If the latter is the case this should probably be described as such.

It is for authentication only, just make the signed bitstream has the 
valid signatures.

Regards,
Richard

> 
>>   - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>   	bridges to successfully become enabled after the region has been
>>   	programmed.
>> -- 
>> 2.7.4
>>
> 
> Thanks
> 

Re: [PATCHv1 0/4] Extend FPGA manager and region drivers for

[Richard Gong]

Hi Yilun,

On 11/15/20 8:41 PM, Xu Yilun wrote:
> On Thu, Nov 12, 2020 at 12:06:39PM -0600, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> The customer wants to verify that a FPGA bitstream can be started properly
>> before saving the bitstream to the QSPI flash memory.
>>
>> 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.
> 
> So when we have successfully reprogramed region with the
> FPGA_MGR_BITSTREM_AUTHENTICATION flag, the bitstream in QSPI flash is
> updated but not activated, we need an FPGA reboot to activate it, is it?
> 

Correct. If the authentication passes, the bitstream will be programmed 
into QSPI flash and will be expected to boot without issues.

> Thanks,
> Yilun
> 
>>
>> Extend FPGA manager and region drivers to support the bitstream
>> authentication feature.
>>
>> Richard Gong (4):
>>    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: entend driver for bitstream authentication
>>
>>   Documentation/devicetree/bindings/fpga/fpga-region.txt | 1 +
>>   drivers/fpga/of-fpga-region.c                          | 3 +++
>>   drivers/fpga/stratix10-soc.c                           | 5 ++++-
>>   include/linux/fpga/fpga-mgr.h                          | 3 +++
>>   4 files changed, 11 insertions(+), 1 deletion(-)
>>
>> -- 
>> 2.7.4

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Richard Gong]

Hi Yilun,

On 11/15/20 8:47 PM, Xu Yilun wrote:
> On Sun, Nov 15, 2020 at 11:21:06AM -0800, Moritz Fischer wrote:
>> Hi Richard,
>>
>> On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
>>> From: Richard Gong <richard.gong@intel.com>
>>>
>>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>>
>>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>>> ---
>>>   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..7a512bc 100644
>>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>> @@ -187,6 +187,7 @@ Optional properties:
>>>   - external-fpga-config : boolean, set if the FPGA has already been configured
>>>   	prior to OS boot up.
>>>   - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
>> It is unclear to me from the description whether this entails
>> authentication + reconfiguration or just authentication.
>>
>> If the latter is the case this should probably be described as such.
> 
> If it is just authentication, do we still need to disable bridges in
> fpga_region_program_fpga?
> 

Yes.

Except for the actual configuration of the device, the authentication 
feature is the same as FPGA configuration.

Regards,
Richard

> I'm wondering if the FPGA functionalities could still be working when
> the authenticating is ongoing, or when the authenticating is failed.
> 



> Thanks,
> Yilun
> 
>>
>>>   - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>>   	bridges to successfully become enabled after the region has been
>>>   	programmed.
>>> -- 
>>> 2.7.4
>>>
>>
>> Thanks

Re: [PATCHv1 4/4] fpga: stratix10-soc: entend driver for bitstream authentication

[Richard Gong]

Hi Moritz,

On 11/15/20 1:19 PM, Moritz Fischer wrote:
> On Thu, Nov 12, 2020 at 12:06:43PM -0600, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Exten FPGA manager driver to support FPGA bitstream authentication on
> Nit: Extend

Sorry, I will fix that in version 2.

>> Intel SocFPGA platforms.
>>
>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>> ---
>>   drivers/fpga/stratix10-soc.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
>> index 657a70c..8a59365 100644
>> --- a/drivers/fpga/stratix10-soc.c
>> +++ b/drivers/fpga/stratix10-soc.c
>> @@ -185,7 +185,10 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>>   	ctype.flags = 0;
>>   	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>>   		dev_dbg(dev, "Requesting partial reconfiguration.\n");
>> -		ctype.flags |= BIT(COMMAND_RECONFIG_FLAG_PARTIAL);
>> +		ctype.flags |= FPGA_MGR_PARTIAL_RECONFIG;
> I think we had this discussion during the original review of the
> stratix10-soc driver?

Yes, we discussed before.

> 
> Wasn't the point of using the BIT() to not assume alignment of FPGA_MGR
> flags and firmware structure?
> 

Yes, we should use BIT().

> The FPGA_MGR_* bits are kernel internal and can therefore change, it
> would be unfortunate to end up in a situation where this breaks the FW
> interface (assuming firmware uses the value in pass-through which it
> looks like is what is happening).

In that case, I should use the flag defined in stratix10-soc driver driver.

> 
>> +	} else if (info->flags & FPGA_MGR_BITSTREM_AUTHENTICATION) {
>> +		dev_dbg(dev, "Requesting bitstream authentication.\n");
>> +		ctype.flags |= FPGA_MGR_BITSTREM_AUTHENTICATION;
> Do you want to change this to BIT(COMMAND_AUTHENTICATE_BITSTREAM) or
> something like that?

OK, I will change to BIT(COMMAND_AUTHENTICATE_BITSTREAM).

Regards,
Richard

>>   	} else {
>>   		dev_dbg(dev, "Requesting full reconfiguration.\n");
>>   	}
>> -- 
>> 2.7.4
>>
> 
> Thanks,
> Moritz
> 

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Tom Rix]

On 11/16/20 5:50 AM, Richard Gong wrote:
> Hi Tom,
>
>
> On 11/14/20 9:59 AM, Tom Rix wrote:
>>
>> On 11/14/20 6:52 AM, Richard Gong wrote:
>>> Hi Tom,
>>>
>>>>>        prior to OS boot up.
>>>>>    - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>>>>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
>>>>
>>>> The list is mostly in alphabetical order so the new 'authenticate-... ' should go at the top.
>>>>
>>>
>>> The original list is not in alphabetical order. The order of partial-fpga-config, external-fpga-config and encrypted-fpga-config here follows the implementation in the of-fpga-region.c file.
>>>
>>> So authenticate-fpga-config should follow the way, correct?
>>>
>> This is why i say 'mostly' ..
>>
>> In general when listing options for a user to read, you should make it easy for them to find
>>
>> the option they are looking for.  Ordering them alphabetically is an obvious but not the only
>>
>> way.  I am not asking for you to fix the whole table, just what you are adding. If there is
>>
>> a better way to organize them please propose the method.
>>
>
> How about put authenticate-fpga-config above partial-fpga-config? I would like to group all xxx-fpga-config flags together.

Ok that sounds fine.

Tom

>
>
> Regards,
> Richard
>
>> Tom
>>
>>>> Improve what you mean by 'authentication' similar to my comment in the first patch.
>>>>
>>>
>>> Will do in the version 2 submission.
>>>
>>> Regards,
>>> Richard
>>>
>>>> Tom
>>>>
>>>>>    - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>>>>        bridges to successfully become enabled after the region has been
>>>>>        programmed.
>>>>
>>>
>>
>

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Xu Yilun]

On Mon, Nov 16, 2020 at 08:14:52AM -0600, Richard Gong wrote:
> 
> Hi Yilun,
> 
> On 11/15/20 8:47 PM, Xu Yilun wrote:
> >On Sun, Nov 15, 2020 at 11:21:06AM -0800, Moritz Fischer wrote:
> >>Hi Richard,
> >>
> >>On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
> >>>From: Richard Gong <richard.gong@intel.com>
> >>>
> >>>Add authenticate-fpga-config property for FPGA bitstream authentication.
> >>>
> >>>Signed-off-by: Richard Gong <richard.gong@intel.com>
> >>>---
> >>>  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..7a512bc 100644
> >>>--- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>@@ -187,6 +187,7 @@ Optional properties:
> >>>  - external-fpga-config : boolean, set if the FPGA has already been configured
> >>>  	prior to OS boot up.
> >>>  - encrypted-fpga-config : boolean, set if the bitstream is encrypted
> >>>+- authenticate-fpga-config : boolean, set if do bitstream authentication
> >>It is unclear to me from the description whether this entails
> >>authentication + reconfiguration or just authentication.
> >>
> >>If the latter is the case this should probably be described as such.
> >
> >If it is just authentication, do we still need to disable bridges in
> >fpga_region_program_fpga?
> >
> 
> Yes.
> 
> Except for the actual configuration of the device, the authentication
> feature is the same as FPGA configuration.

FPGA Bridges gate bus signals between a host and FPGA. So the FPGA
region could not be accessed by host when doing configuration. But for
this authentication, we are just writing the flash, we don't actually
touch the FPGA soft logic. The host should still be able to operate on
the old logic before reboot, is it?

Thanks,
Yilun

> 
> Regards,
> Richard
> 
> >I'm wondering if the FPGA functionalities could still be working when
> >the authenticating is ongoing, or when the authenticating is failed.
> >
> 
> 
> 
> >Thanks,
> >Yilun
> >
> >>
> >>>  - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
> >>>  	bridges to successfully become enabled after the region has been
> >>>  	programmed.
> >>>-- 
> >>>2.7.4
> >>>
> >>
> >>Thanks

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Richard Gong]

On 11/16/20 8:24 PM, Xu Yilun wrote:
> On Mon, Nov 16, 2020 at 08:14:52AM -0600, Richard Gong wrote:
>>
>> Hi Yilun,
>>
>> On 11/15/20 8:47 PM, Xu Yilun wrote:
>>> On Sun, Nov 15, 2020 at 11:21:06AM -0800, Moritz Fischer wrote:
>>>> Hi Richard,
>>>>
>>>> On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
>>>>> From: Richard Gong <richard.gong@intel.com>
>>>>>
>>>>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>>>>
>>>>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>>>>> ---
>>>>>   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..7a512bc 100644
>>>>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>>> @@ -187,6 +187,7 @@ Optional properties:
>>>>>   - external-fpga-config : boolean, set if the FPGA has already been configured
>>>>>   	prior to OS boot up.
>>>>>   - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>>>>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
>>>> It is unclear to me from the description whether this entails
>>>> authentication + reconfiguration or just authentication.
>>>>
>>>> If the latter is the case this should probably be described as such.
>>>
>>> If it is just authentication, do we still need to disable bridges in
>>> fpga_region_program_fpga?
>>>
>>
>> Yes.
>>
>> Except for the actual configuration of the device, the authentication
>> feature is the same as FPGA configuration.
> 
> FPGA Bridges gate bus signals between a host and FPGA. So the FPGA
> region could not be accessed by host when doing configuration. But for
> this authentication, we are just writing the flash, we don't actually
> touch the FPGA soft logic. The host should still be able to operate on
> the old logic before reboot, is it?
>
Yes, it's feasible in theory but doesn't make much sense in practice. I 
prefer to keep fpga_region_program_fpga() unchanged.

Regards,
Richard

> Thanks,
> Yilun
> 
>>
>> Regards,
>> Richard
>>
>>> I'm wondering if the FPGA functionalities could still be working when
>>> the authenticating is ongoing, or when the authenticating is failed.
>>>
>>
>>
>>
>>> Thanks,
>>> Yilun
>>>
>>>>
>>>>>   - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>>>>   	bridges to successfully become enabled after the region has been
>>>>>   	programmed.
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>>
>>>> Thanks

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Xu Yilun]

On Tue, Nov 17, 2020 at 09:39:55AM -0600, Richard Gong wrote:
> 
> 
> On 11/16/20 8:24 PM, Xu Yilun wrote:
> >On Mon, Nov 16, 2020 at 08:14:52AM -0600, Richard Gong wrote:
> >>
> >>Hi Yilun,
> >>
> >>On 11/15/20 8:47 PM, Xu Yilun wrote:
> >>>On Sun, Nov 15, 2020 at 11:21:06AM -0800, Moritz Fischer wrote:
> >>>>Hi Richard,
> >>>>
> >>>>On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
> >>>>>From: Richard Gong <richard.gong@intel.com>
> >>>>>
> >>>>>Add authenticate-fpga-config property for FPGA bitstream authentication.
> >>>>>
> >>>>>Signed-off-by: Richard Gong <richard.gong@intel.com>
> >>>>>---
> >>>>>  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..7a512bc 100644
> >>>>>--- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>>>+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>>>@@ -187,6 +187,7 @@ Optional properties:
> >>>>>  - external-fpga-config : boolean, set if the FPGA has already been configured
> >>>>>  	prior to OS boot up.
> >>>>>  - encrypted-fpga-config : boolean, set if the bitstream is encrypted
> >>>>>+- authenticate-fpga-config : boolean, set if do bitstream authentication
> >>>>It is unclear to me from the description whether this entails
> >>>>authentication + reconfiguration or just authentication.
> >>>>
> >>>>If the latter is the case this should probably be described as such.
> >>>
> >>>If it is just authentication, do we still need to disable bridges in
> >>>fpga_region_program_fpga?
> >>>
> >>
> >>Yes.
> >>
> >>Except for the actual configuration of the device, the authentication
> >>feature is the same as FPGA configuration.
> >
> >FPGA Bridges gate bus signals between a host and FPGA. So the FPGA
> >region could not be accessed by host when doing configuration. But for
> >this authentication, we are just writing the flash, we don't actually
> >touch the FPGA soft logic. The host should still be able to operate on
> >the old logic before reboot, is it?
> >
> Yes, it's feasible in theory but doesn't make much sense in practice. I
> prefer to keep fpga_region_program_fpga() unchanged.

I'm thinking of the case of inband reprograming, that the QSPI flash
controller itself is embedded in FPGA soft logic, then maybe host still
need to access FPGA on authentication.

Thanks,
Yilun

> >>
> >>>I'm wondering if the FPGA functionalities could still be working when
> >>>the authenticating is ongoing, or when the authenticating is failed.
> >>>
> >>
> >>
> >>
> >>>Thanks,
> >>>Yilun
> >>>
> >>>>
> >>>>>  - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
> >>>>>  	bridges to successfully become enabled after the region has been
> >>>>>  	programmed.
> >>>>>-- 
> >>>>>2.7.4
> >>>>>
> >>>>
> >>>>Thanks

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Richard Gong]

On 11/17/20 11:47 PM, Xu Yilun wrote:
> On Tue, Nov 17, 2020 at 09:39:55AM -0600, Richard Gong wrote:
>>
>>
>> On 11/16/20 8:24 PM, Xu Yilun wrote:
>>> On Mon, Nov 16, 2020 at 08:14:52AM -0600, Richard Gong wrote:
>>>>
>>>> Hi Yilun,
>>>>
>>>> On 11/15/20 8:47 PM, Xu Yilun wrote:
>>>>> On Sun, Nov 15, 2020 at 11:21:06AM -0800, Moritz Fischer wrote:
>>>>>> Hi Richard,
>>>>>>
>>>>>> On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
>>>>>>> From: Richard Gong <richard.gong@intel.com>
>>>>>>>
>>>>>>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>>>>>>
>>>>>>> Signed-off-by: Richard Gong <richard.gong@intel.com>
>>>>>>> ---
>>>>>>>   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..7a512bc 100644
>>>>>>> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
>>>>>>> @@ -187,6 +187,7 @@ Optional properties:
>>>>>>>   - external-fpga-config : boolean, set if the FPGA has already been configured
>>>>>>>   	prior to OS boot up.
>>>>>>>   - encrypted-fpga-config : boolean, set if the bitstream is encrypted
>>>>>>> +- authenticate-fpga-config : boolean, set if do bitstream authentication
>>>>>> It is unclear to me from the description whether this entails
>>>>>> authentication + reconfiguration or just authentication.
>>>>>>
>>>>>> If the latter is the case this should probably be described as such.
>>>>>
>>>>> If it is just authentication, do we still need to disable bridges in
>>>>> fpga_region_program_fpga?
>>>>>
>>>>
>>>> Yes.
>>>>
>>>> Except for the actual configuration of the device, the authentication
>>>> feature is the same as FPGA configuration.
>>>
>>> FPGA Bridges gate bus signals between a host and FPGA. So the FPGA
>>> region could not be accessed by host when doing configuration. But for
>>> this authentication, we are just writing the flash, we don't actually
>>> touch the FPGA soft logic. The host should still be able to operate on
>>> the old logic before reboot, is it?
>>>
>> Yes, it's feasible in theory but doesn't make much sense in practice. I
>> prefer to keep fpga_region_program_fpga() unchanged.
> 
> I'm thinking of the case of inband reprograming, that the QSPI flash
> controller itself is embedded in FPGA soft logic, then maybe host still
> need to access FPGA on authentication.

We can decide whether we should update fpga_region_program_fpga() 
function when you update for inband reprogramming case.

Regards,
Richard
> 
> Thanks,
> Yilun
> 
>>>>
>>>>> I'm wondering if the FPGA functionalities could still be working when
>>>>> the authenticating is ongoing, or when the authenticating is failed.
>>>>>
>>>>
>>>>
>>>>
>>>>> Thanks,
>>>>> Yilun
>>>>>
>>>>>>
>>>>>>>   - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
>>>>>>>   	bridges to successfully become enabled after the region has been
>>>>>>>   	programmed.
>>>>>>> -- 
>>>>>>> 2.7.4
>>>>>>>
>>>>>>
>>>>>> Thanks

Re: [PATCHv1 3/4] dt-bindings: fpga: add authenticate-fpga-config property

[Xu Yilun]

On Wed, Nov 18, 2020 at 07:38:31AM -0600, Richard Gong wrote:
> 
> 
> On 11/17/20 11:47 PM, Xu Yilun wrote:
> >On Tue, Nov 17, 2020 at 09:39:55AM -0600, Richard Gong wrote:
> >>
> >>
> >>On 11/16/20 8:24 PM, Xu Yilun wrote:
> >>>On Mon, Nov 16, 2020 at 08:14:52AM -0600, Richard Gong wrote:
> >>>>
> >>>>Hi Yilun,
> >>>>
> >>>>On 11/15/20 8:47 PM, Xu Yilun wrote:
> >>>>>On Sun, Nov 15, 2020 at 11:21:06AM -0800, Moritz Fischer wrote:
> >>>>>>Hi Richard,
> >>>>>>
> >>>>>>On Thu, Nov 12, 2020 at 12:06:42PM -0600, richard.gong@linux.intel.com wrote:
> >>>>>>>From: Richard Gong <richard.gong@intel.com>
> >>>>>>>
> >>>>>>>Add authenticate-fpga-config property for FPGA bitstream authentication.
> >>>>>>>
> >>>>>>>Signed-off-by: Richard Gong <richard.gong@intel.com>
> >>>>>>>---
> >>>>>>>  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..7a512bc 100644
> >>>>>>>--- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>>>>>+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> >>>>>>>@@ -187,6 +187,7 @@ Optional properties:
> >>>>>>>  - external-fpga-config : boolean, set if the FPGA has already been configured
> >>>>>>>  	prior to OS boot up.
> >>>>>>>  - encrypted-fpga-config : boolean, set if the bitstream is encrypted
> >>>>>>>+- authenticate-fpga-config : boolean, set if do bitstream authentication
> >>>>>>It is unclear to me from the description whether this entails
> >>>>>>authentication + reconfiguration or just authentication.
> >>>>>>
> >>>>>>If the latter is the case this should probably be described as such.
> >>>>>
> >>>>>If it is just authentication, do we still need to disable bridges in
> >>>>>fpga_region_program_fpga?
> >>>>>
> >>>>
> >>>>Yes.
> >>>>
> >>>>Except for the actual configuration of the device, the authentication
> >>>>feature is the same as FPGA configuration.
> >>>
> >>>FPGA Bridges gate bus signals between a host and FPGA. So the FPGA
> >>>region could not be accessed by host when doing configuration. But for
> >>>this authentication, we are just writing the flash, we don't actually
> >>>touch the FPGA soft logic. The host should still be able to operate on
> >>>the old logic before reboot, is it?
> >>>
> >>Yes, it's feasible in theory but doesn't make much sense in practice. I
> >>prefer to keep fpga_region_program_fpga() unchanged.
> >
> >I'm thinking of the case of inband reprograming, that the QSPI flash
> >controller itself is embedded in FPGA soft logic, then maybe host still
> >need to access FPGA on authentication.
> 
> We can decide whether we should update fpga_region_program_fpga() function
> when you update for inband reprogramming case.

Sure, we could think about it later.

Thanks,
Yilun

> 
> Regards,
> Richard
> >
> >Thanks,
> >Yilun
> >
> >>>>
> >>>>>I'm wondering if the FPGA functionalities could still be working when
> >>>>>the authenticating is ongoing, or when the authenticating is failed.
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>>>Thanks,
> >>>>>Yilun
> >>>>>
> >>>>>>
> >>>>>>>  - region-unfreeze-timeout-us : The maximum time in microseconds to wait for
> >>>>>>>  	bridges to successfully become enabled after the region has been
> >>>>>>>  	programmed.
> >>>>>>>-- 
> >>>>>>>2.7.4
> >>>>>>>
> >>>>>>
> >>>>>>Thanks