From: Richard Gong <[email protected]>
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
From: Richard Gong <[email protected]>
Add authenticate-fpga-config property to support FPGA bitstream
authentication.
Signed-off-by: Richard Gong <[email protected]>
---
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
From: Richard Gong <[email protected]>
Add authenticate-fpga-config property for FPGA bitstream authentication.
Signed-off-by: Richard Gong <[email protected]>
---
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
From: Richard Gong <[email protected]>
Add FPGA_MGR_BITSTREM_AUTHENTICATION flag for FPGA bitstream
authentication.
Signed-off-by: Richard Gong <[email protected]>
---
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
From: Richard Gong <[email protected]>
Exten FPGA manager driver to support FPGA bitstream authentication on
Intel SocFPGA platforms.
Signed-off-by: Richard Gong <[email protected]>
---
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
On 11/12/20 10:06 AM, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On 11/12/20 10:06 AM, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> Add authenticate-fpga-config property to support FPGA bitstream
> authentication.
>
> Signed-off-by: Richard Gong <[email protected]>
> ---
> 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 <[email protected]>
On 11/12/20 10:06 AM, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> Add authenticate-fpga-config property for FPGA bitstream authentication.
>
> Signed-off-by: Richard Gong <[email protected]>
> ---
> 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.
On 11/12/20 10:06 AM, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> Exten FPGA manager driver to support FPGA bitstream authentication on
> Intel SocFPGA platforms.
>
> Signed-off-by: Richard Gong <[email protected]>
> ---
> 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");
> }
Hi Tom,
Thanks for review!
On 11/13/20 2:24 PM, Tom Rix wrote:
>
> On 11/12/20 10:06 AM, [email protected] wrote:
>> From: Richard Gong <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>
Hi Tom,
On 11/13/20 2:28 PM, Tom Rix wrote:
>
> On 11/12/20 10:06 AM, [email protected] wrote:
>> From: Richard Gong <[email protected]>
>>
>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>
>> Signed-off-by: Richard Gong <[email protected]>
>> ---
>> 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.
>
Hi Tom.
On 11/13/20 2:31 PM, Tom Rix wrote:
>
> On 11/12/20 10:06 AM, [email protected] wrote:
>> From: Richard Gong <[email protected]>
>>
>> Exten FPGA manager driver to support FPGA bitstream authentication on
>> Intel SocFPGA platforms.
>>
>> Signed-off-by: Richard Gong <[email protected]>
>> ---
>> 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");
>> }
>
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
>>
>
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.
>>
>
Hi Richard,
On Thu, Nov 12, 2020 at 12:06:42PM -0600, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> Add authenticate-fpga-config property for FPGA bitstream authentication.
>
> Signed-off-by: Richard Gong <[email protected]>
> ---
> 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
On Thu, Nov 12, 2020 at 12:06:43PM -0600, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> Exten FPGA manager driver to support FPGA bitstream authentication on
Nit: Extend
> Intel SocFPGA platforms.
>
> Signed-off-by: Richard Gong <[email protected]>
> ---
> 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
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, [email protected] wrote:
> > From: Richard Gong <[email protected]>
> >
> > Add authenticate-fpga-config property for FPGA bitstream authentication.
> >
> > Signed-off-by: Richard Gong <[email protected]>
> > ---
> > 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
On Thu, Nov 12, 2020 at 12:06:39PM -0600, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> 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
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
>>>
>>
>
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.
>>>
>>
>
Hi Moritz,
On 11/15/20 1:21 PM, Moritz Fischer wrote:
> Hi Richard,
>
> On Thu, Nov 12, 2020 at 12:06:42PM -0600, [email protected] wrote:
>> From: Richard Gong <[email protected]>
>>
>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>
>> Signed-off-by: Richard Gong <[email protected]>
>> ---
>> 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
>
Hi Yilun,
On 11/15/20 8:41 PM, Xu Yilun wrote:
> On Thu, Nov 12, 2020 at 12:06:39PM -0600, [email protected] wrote:
>> From: Richard Gong <[email protected]>
>>
>> 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
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, [email protected] wrote:
>>> From: Richard Gong <[email protected]>
>>>
>>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>>
>>> Signed-off-by: Richard Gong <[email protected]>
>>> ---
>>> 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
Hi Moritz,
On 11/15/20 1:19 PM, Moritz Fischer wrote:
> On Thu, Nov 12, 2020 at 12:06:43PM -0600, [email protected] wrote:
>> From: Richard Gong <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>
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.
>>>>
>>>
>>
>
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, [email protected] wrote:
> >>>From: Richard Gong <[email protected]>
> >>>
> >>>Add authenticate-fpga-config property for FPGA bitstream authentication.
> >>>
> >>>Signed-off-by: Richard Gong <[email protected]>
> >>>---
> >>> 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
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, [email protected] wrote:
>>>>> From: Richard Gong <[email protected]>
>>>>>
>>>>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>>>>
>>>>> Signed-off-by: Richard Gong <[email protected]>
>>>>> ---
>>>>> 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
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, [email protected] wrote:
> >>>>>From: Richard Gong <[email protected]>
> >>>>>
> >>>>>Add authenticate-fpga-config property for FPGA bitstream authentication.
> >>>>>
> >>>>>Signed-off-by: Richard Gong <[email protected]>
> >>>>>---
> >>>>> 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
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, [email protected] wrote:
>>>>>>> From: Richard Gong <[email protected]>
>>>>>>>
>>>>>>> Add authenticate-fpga-config property for FPGA bitstream authentication.
>>>>>>>
>>>>>>> Signed-off-by: Richard Gong <[email protected]>
>>>>>>> ---
>>>>>>> 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
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, [email protected] wrote:
> >>>>>>>From: Richard Gong <[email protected]>
> >>>>>>>
> >>>>>>>Add authenticate-fpga-config property for FPGA bitstream authentication.
> >>>>>>>
> >>>>>>>Signed-off-by: Richard Gong <[email protected]>
> >>>>>>>---
> >>>>>>> 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